Skip to content

Commit f2033b5

Browse files
Bug 2045853 - Trigger postEnrolmentCalculation() on all enrollment changes
`postEnrolmentCalculation()` is responsible for recording the active experiments and triggering the `onUpdatesApplied` observer -- which is in turn used to invalidate the FML feature cache. Several enrolment change mechanisms were not properly triggering `onUpdatesApplied`, which has lead to stale feature value caching issues (see-also mozilla-mobile/fenix#21838). These issues have been band-aided by calling applyPendingExperiments() in most cases, which results in ~correct behaviour but does a double update. Now we trigger `postEnrolmentCalculation()` and therefore feature invalidation on all enrollment changes.
1 parent fcc5382 commit f2033b5

2 files changed

Lines changed: 39 additions & 20 deletions

File tree

  • components/nimbus/android/src

components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/Nimbus.kt

Lines changed: 37 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ open class Nimbus(
165165
get() = nimbusClient.getExperimentParticipation()
166166
set(active) {
167167
dbScope.launch {
168-
nimbusClient.setExperimentParticipation(active)
168+
setExperimentParticipationOnThisThread(active)
169169
applyPendingExperimentsOnThisThread()
170170
}
171171
}
@@ -174,7 +174,7 @@ open class Nimbus(
174174
get() = nimbusClient.getRolloutParticipation()
175175
set(active) {
176176
dbScope.launch {
177-
nimbusClient.setRolloutParticipation(active)
177+
setRolloutParticipationOnThisThread(active)
178178
applyPendingExperimentsOnThisThread()
179179
}
180180
}
@@ -458,9 +458,7 @@ open class Nimbus(
458458
prefUnenrollReason: PrefUnenrollReason,
459459
) {
460460
dbScope.launch {
461-
withCatchAll("unenrollForGeckoPref") {
462-
unenrollForGeckoPrefOnThisThread(geckoPrefState, prefUnenrollReason)
463-
}
461+
unenrollForGeckoPrefOnThisThread(geckoPrefState, prefUnenrollReason)
464462
}
465463
}
466464

@@ -469,10 +467,13 @@ open class Nimbus(
469467
internal fun unenrollForGeckoPrefOnThisThread(
470468
geckoPrefState: GeckoPrefState,
471469
prefUnenrollReason: PrefUnenrollReason,
472-
): List<EnrollmentChangeEvent> {
473-
val events = nimbusClient.unenrollForGeckoPref(geckoPrefState, prefUnenrollReason)
474-
recordExperimentTelemetryEvents(events)
475-
return events
470+
): List<EnrollmentChangeEvent>? {
471+
return withCatchAll("unenrollForGeckoPref") {
472+
val enrollmentChangeEvents = nimbusClient.unenrollForGeckoPref(geckoPrefState, prefUnenrollReason)
473+
recordExperimentTelemetryEvents(enrollmentChangeEvents)
474+
postEnrolmentCalculation()
475+
enrollmentChangeEvents
476+
}
476477
}
477478

478479
override fun registerPreviousGeckoPrefStates(geckoPrefStates: List<GeckoPrefState>) {
@@ -491,25 +492,42 @@ open class Nimbus(
491492

492493
@WorkerThread
493494
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
494-
internal fun optOutOnThisThread(experimentId: String) = withCatchAll("optOut") {
495-
nimbusClient.optOut(experimentId).also(::recordExperimentTelemetryEvents)
495+
internal fun optOutOnThisThread(experimentId: String) {
496+
withCatchAll("optOut") {
497+
nimbusClient.optOut(experimentId).also(::recordExperimentTelemetryEvents)
498+
postEnrolmentCalculation()
499+
}
496500
}
497501

502+
@AnyThread
498503
override fun resetTelemetryIdentifiers() {
499504
dbScope.launch {
500-
withCatchAll("resetTelemetryIdentifiers") {
501-
nimbusClient.resetTelemetryIdentifiers().also { enrollmentChangeEvents ->
502-
recordExperimentTelemetryEvents(enrollmentChangeEvents)
503-
}
504-
}
505+
resetTelemetryIdentifiersOnThisThread()
506+
}
507+
}
508+
509+
@WorkerThread
510+
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
511+
internal fun resetTelemetryIdentifiersOnThisThread() {
512+
withCatchAll("resetTelemetryIdentifiers") {
513+
val enrollmentChangeEvents = nimbusClient.resetTelemetryIdentifiers()
514+
recordExperimentTelemetryEvents(enrollmentChangeEvents)
515+
postEnrolmentCalculation()
505516
}
506517
}
507518

508519
override fun optInWithBranch(experimentId: String, branch: String) {
509520
dbScope.launch {
510-
withCatchAll("optIn") {
511-
nimbusClient.optInWithBranch(experimentId, branch).also(::recordExperimentTelemetryEvents)
512-
}
521+
optInWithBranchOnThisThread(experimentId, branch)
522+
}
523+
}
524+
525+
@WorkerThread
526+
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
527+
internal fun optInWithBranchOnThisThread(experimentId: String, branch: String) {
528+
withCatchAll("optIn") {
529+
nimbusClient.optInWithBranch(experimentId, branch).also(::recordExperimentTelemetryEvents)
530+
postEnrolmentCalculation()
513531
}
514532
}
515533

components/nimbus/android/src/test/java/org/mozilla/experiments/nimbus/NimbusTests.kt

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -942,7 +942,8 @@ class NimbusTests {
942942
PrefUnenrollReason.FAILED_TO_SET,
943943
)
944944

945-
assertEquals(1, events.size)
945+
assertNotNull(events)
946+
assertEquals(1, events!!.size)
946947
assertEquals(EnrollmentChangeEventType.DISQUALIFICATION, events[0].change)
947948
assertEquals(0, handler.setValues?.size)
948949
}

0 commit comments

Comments
 (0)