Bug 1732914 - Add new unenrollment reason when targeting is no longer satisfied r=k88hudson

Differential Revision: https://phabricator.services.mozilla.com/D128716

toolkit/components/nimbus/lib/ExperimentManager.jsm

@@ -126,8 +126,9 @@ class _ExperimentManager {
   /**
    * Runs when the all recipes been processed during an update, including at first run.
    * @param {string} sourceToCheck
+   * @param {object} options Extra context used in telemetry reporting
    */
-  onFinalize(sourceToCheck) {
+  onFinalize(sourceToCheck, { recipeMismatches } = { recipeMismatches: [] }) {
     if (!sourceToCheck) {
       throw new Error("When calling onFinalize, you must specify a source.");
     }
@@ -141,7 +142,10 @@ class _ExperimentManager {
       if (!this.sessions.get(source)?.has(slug)) {
         log.debug(`Stopping study for recipe ${slug}`);
         try {
-          this.unenroll(slug, "recipe-not-seen");
+          let reason = recipeMismatches.includes(slug)
+            ? "targeting-mismatch"
+            : "recipe-not-seen";
+          this.unenroll(slug, reason);
         } catch (err) {
           Cu.reportError(err);
         }

toolkit/components/nimbus/lib/RemoteSettingsExperimentLoader.jsm

@@ -321,6 +321,7 @@ class _RemoteSettingsExperimentLoader {
     }
 
     let matches = 0;
+    let recipeMismatches = [];
     if (recipes && !loadingError) {
       for (const r of recipes) {
         if (await this.checkTargeting(r)) {
@@ -329,11 +330,12 @@ class _RemoteSettingsExperimentLoader {
           await this.manager.onRecipe(r, "rs-loader");
         } else {
           log.debug(`${r.id} did not match due to targeting`);
+          recipeMismatches.push(r.slug);
         }
       }
 
       log.debug(`${matches} recipes matched. Finalizing ExperimentManager.`);
-      this.manager.onFinalize("rs-loader");
+      this.manager.onFinalize("rs-loader", { recipeMismatches });
     }
 
     if (trigger !== "timer") {

toolkit/components/nimbus/test/unit/test_ExperimentManager_lifecycle.js

@@ -225,3 +225,51 @@ add_task(async function test_onFinalize_unenroll() {
     "should clear sessions[test]"
   );
 });
+
+add_task(async function test_onFinalize_unenroll_mismatch() {
+  const manager = ExperimentFakes.manager();
+  const sandbox = sinon.createSandbox();
+  sandbox.spy(manager, "unenroll");
+
+  await manager.onStartup();
+
+  // Add an experiment to the store without calling .onRecipe
+  // This simulates an enrollment having happened in the past.
+  let recipe0 = ExperimentFakes.experiment("foo", {
+    experimentType: "unittest",
+    userFacingName: "foo",
+    userFacingDescription: "foo",
+    lastSeen: Date.now().toLocaleString(),
+    source: "test",
+  });
+  await manager.store.addExperiment(recipe0);
+
+  const recipe1 = ExperimentFakes.recipe("bar");
+  // Unique features to prevent overlap
+  recipe1.branches[0].features[0].featureId = "red";
+  recipe1.branches[1].features[0].featureId = "red";
+  await manager.onRecipe(recipe1, "test");
+  const recipe2 = ExperimentFakes.recipe("baz");
+  recipe2.branches[0].features[0].featureId = "green";
+  recipe2.branches[1].features[0].featureId = "green";
+  await manager.onRecipe(recipe2, "test");
+
+  // Finalize
+  manager.onFinalize("test", { recipeMismatches: [recipe0.slug] });
+
+  Assert.equal(
+    manager.unenroll.callCount,
+    1,
+    "should only call unenroll for the unseen recipe"
+  );
+  Assert.equal(
+    manager.unenroll.calledWith("foo", "targeting-mismatch"),
+    true,
+    "should unenroll a experiment whose recipe wasn't seen in the current session"
+  );
+  Assert.equal(
+    manager.sessions.has("test"),
+    false,
+    "should clear sessions[test]"
+  );
+});

toolkit/components/nimbus/test/unit/test_RemoteSettingsExperimentLoader.js

@@ -134,6 +134,41 @@ add_task(async function test_updateRecipes() {
   );
 });
 
+add_task(async function test_updateRecipes_someMismatch() {
+  const loader = ExperimentFakes.rsLoader();
+
+  const PASS_FILTER_RECIPE = ExperimentFakes.recipe("foo", {
+    targeting: "true",
+  });
+  const FAIL_FILTER_RECIPE = ExperimentFakes.recipe("foo", {
+    targeting: "false",
+  });
+  sinon.stub(loader, "setTimer");
+  sinon.spy(loader, "updateRecipes");
+
+  sinon
+    .stub(loader.remoteSettingsClient, "get")
+    .resolves([PASS_FILTER_RECIPE, FAIL_FILTER_RECIPE]);
+  sinon.stub(loader.manager, "onRecipe").resolves();
+  sinon.stub(loader.manager, "onFinalize");
+
+  Services.prefs.setBoolPref(ENABLED_PREF, true);
+  await loader.init();
+  ok(loader.updateRecipes.calledOnce, "should call .updateRecipes");
+  equal(
+    loader.manager.onRecipe.callCount,
+    1,
+    "should call .onRecipe only for recipes that pass"
+  );
+  ok(loader.manager.onFinalize.calledOnce, "Should call onFinalize.");
+  ok(
+    loader.manager.onFinalize.calledWith("rs-loader", {
+      recipeMismatches: [FAIL_FILTER_RECIPE.slug],
+    }),
+    "should call .onFinalize with the recipes that failed targeting"
+  );
+});
+
 add_task(async function test_updateRecipes_forFirstStartup() {
   const loader = ExperimentFakes.rsLoader();
   const PASS_FILTER_RECIPE = ExperimentFakes.recipe("foo", {