Bug 1517475 - Execute recipe runner on Remote Settings "sync" event r=mythmon

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

--HG--
extra : moz-landing-system : lando

toolkit/components/normandy/lib/RecipeRunner.jsm

@@ -41,6 +41,10 @@ const DEV_MODE_PREF = `${PREF_PREFIX}.dev_mode`;
 const API_URL_PREF = `${PREF_PREFIX}.api_url`;
 const LAZY_CLASSIFY_PREF = `${PREF_PREFIX}.experiments.lazy_classify`;
 
+// Timer last update preference.
+// see https://searchfox.org/mozilla-central/rev/11cfa0462/toolkit/components/timermanager/UpdateTimerManager.jsm#8
+const TIMER_LAST_UPDATE_PREF = `app.update.lastUpdateTime.${TIMER_NAME}`;
+
 const PREFS_TO_WATCH = [
   RUN_INTERVAL_PREF,
   TELEMETRY_ENABLED_PREF,
@@ -71,9 +75,13 @@ function cacheProxy(target) {
 
 var RecipeRunner = {
   async init() {
+    this.running = false;
     this.enabled = null;
+    this.loadFromRemoteSettings = false;
+
     this.checkPrefs(); // sets this.enabled
     this.watchPrefs();
+    await this.setUpRemoteSettings();
 
     // Run if enabled immediately on first run, or if dev mode is enabled.
     const firstRun = Services.prefs.getBoolPref(FIRST_RUN_PREF, true);
@@ -190,6 +198,43 @@ var RecipeRunner = {
     timerManager.unregisterTimer(TIMER_NAME);
   },
 
+  async setUpRemoteSettings() {
+    const feature = "normandy-remote-settings";
+
+    if (await FeatureGate.isEnabled(feature)) {
+      this.attachRemoteSettings();
+    }
+    const observer = {
+      onEnable: this.attachRemoteSettings.bind(this),
+      onDisable: this.detachRemoteSettings.bind(this),
+    };
+    await FeatureGate.addObserver(feature, observer);
+    CleanupManager.addCleanupHandler(() => FeatureGate.removeObserver(feature, observer));
+  },
+
+  attachRemoteSettings() {
+    this.loadFromRemoteSettings = true;
+    if (!this._onSync) {
+      this._onSync = async () => {
+        if (!this.enabled) {
+          return;
+        }
+        this.run({ trigger: "sync" });
+      };
+
+      gRemoteSettingsClient.on("sync", this._onSync);
+    }
+  },
+
+  detachRemoteSettings() {
+    this.loadFromRemoteSettings = false;
+    if (this._onSync) {
+      // Ignore if no event listener was setup or was already removed (ie. pref changed while enabled).
+      gRemoteSettingsClient.off("sync", this._onSync);
+    }
+    this._onSync = null;
+  },
+
   updateRunInterval() {
     // Run once every `runInterval` wall-clock seconds. This is managed by setting a "last ran"
     // timestamp, and running if it is more than `runInterval` seconds ago. Even with very short
@@ -198,51 +243,69 @@ var RecipeRunner = {
     timerManager.registerTimer(TIMER_NAME, () => this.run(), runInterval);
   },
 
-  async run() {
-    Services.obs.notifyObservers(null, "recipe-runner:start");
-    this.clearCaches();
-    // Unless lazy classification is enabled, prep the classify cache.
-    if (!Services.prefs.getBoolPref(LAZY_CLASSIFY_PREF, false)) {
-      try {
-        await ClientEnvironment.getClientClassification();
-      } catch (err) {
-        // Try to go on without this data; the filter expressions will
-        // gracefully fail without this info if they need it.
-      }
-    }
+  async run(options = {}) {
+    const { trigger = "timer" } = options;
 
-    // Fetch recipes before execution in case we fail and exit early.
-    let recipesToRun;
-    try {
-      recipesToRun = await this.loadRecipes();
-    } catch (e) {
-      // Either we failed at fetching the recipes from server (legacy),
-      // or the recipes signature verification failed.
-      let status = Uptake.RUNNER_SERVER_ERROR;
-      if (/NetworkError/.test(e)) {
-        status = Uptake.RUNNER_NETWORK_ERROR;
-      } else if (e instanceof NormandyApi.InvalidSignatureError) {
-        status = Uptake.RUNNER_INVALID_SIGNATURE;
-      }
-      await Uptake.reportRunner(status);
+    if (this.running) {
+      // Do nothing if already running.
       return;
     }
+    try {
+      this.running = true;
 
-    const actions = new ActionsManager();
+      Services.obs.notifyObservers(null, "recipe-runner:start");
+      this.clearCaches();
+      // Unless lazy classification is enabled, prep the classify cache.
+      if (!Services.prefs.getBoolPref(LAZY_CLASSIFY_PREF, false)) {
+        try {
+          await ClientEnvironment.getClientClassification();
+        } catch (err) {
+          // Try to go on without this data; the filter expressions will
+          // gracefully fail without this info if they need it.
+        }
+      }
 
-    // Execute recipes, if we have any.
-    if (recipesToRun.length === 0) {
-      log.debug("No recipes to execute");
-    } else {
-      for (const recipe of recipesToRun) {
-        await actions.runRecipe(recipe);
+      // Fetch recipes before execution in case we fail and exit early.
+      let recipesToRun;
+      try {
+        recipesToRun = await this.loadRecipes();
+      } catch (e) {
+        // Either we failed at fetching the recipes from server (legacy),
+        // or the recipes signature verification failed.
+        let status = Uptake.RUNNER_SERVER_ERROR;
+        if (/NetworkError/.test(e)) {
+          status = Uptake.RUNNER_NETWORK_ERROR;
+        } else if (e instanceof NormandyApi.InvalidSignatureError) {
+          status = Uptake.RUNNER_INVALID_SIGNATURE;
+        }
+        await Uptake.reportRunner(status);
+        return;
+      }
+
+      const actions = new ActionsManager();
+
+      // Execute recipes, if we have any.
+      if (recipesToRun.length === 0) {
+        log.debug("No recipes to execute");
+      } else {
+        for (const recipe of recipesToRun) {
+          await actions.runRecipe(recipe);
+        }
+      }
+
+      await actions.finalize();
+
+      await Uptake.reportRunner(Uptake.RUNNER_SUCCESS);
+      Services.obs.notifyObservers(null, "recipe-runner:end");
+    } finally {
+      this.running = false;
+      if (trigger != "timer") {
+        // `run()` was executed outside the scheduled timer.
+        // Update the last time it ran to make sure it is rescheduled later.
+        const lastUpdateTime = Math.round(Date.now() / 1000);
+        Services.prefs.setIntPref(TIMER_LAST_UPDATE_PREF, lastUpdateTime);
       }
     }
-
-    await actions.finalize();
-
-    await Uptake.reportRunner(Uptake.RUNNER_SUCCESS);
-    Services.obs.notifyObservers(null, "recipe-runner:end");
   },
 
   /**
@@ -251,7 +314,7 @@ var RecipeRunner = {
   async loadRecipes() {
     // If RemoteSettings is enabled, we read the list of recipes from there.
     // The JEXL filtering is done via the provided callback (see `gRemoteSettingsClient`).
-    if (await FeatureGate.isEnabled("normandy-remote-settings")) {
+    if (this.loadFromRemoteSettings) {
       // First, fetch recipes whose JEXL filters match.
       const entries = await gRemoteSettingsClient.get();
       // Then, verify the signature of each recipe. It will throw if invalid.

toolkit/components/normandy/test/browser/browser_RecipeRunner.js

@@ -455,3 +455,129 @@ decorate_task(
     }
   }
 );
+
+decorate_task(
+  withPrefEnv({
+    set: [
+      ["features.normandy-remote-settings.enabled", false],
+    ],
+  }),
+  withStub(RecipeRunner, "run"),
+  async function testRunOnSyncRemoteSettings(
+    runStub,
+  ) {
+    const rsClient = RecipeRunner._remoteSettingsClientForTesting;
+
+    // Runner disabled + pref off.
+    RecipeRunner.disable();
+    await rsClient.emit("sync", {});
+    ok(!runStub.called, "run() should not be called if disabled");
+    runStub.reset();
+
+    // Runner enabled + pref off.
+    RecipeRunner.enable();
+    await rsClient.emit("sync", {});
+    ok(!runStub.called, "run() should not be called if pref not set");
+    runStub.reset();
+
+    await SpecialPowers.pushPrefEnv({ set: [["features.normandy-remote-settings.enabled", true]] });
+
+    // Runner enabled + pref on.
+    await rsClient.emit("sync", {});
+    ok(runStub.called, "run() should be called if pref is set");
+    runStub.reset();
+
+    // Runner disabled + pref on.
+    RecipeRunner.disable();
+    await rsClient.emit("sync", {});
+    ok(!runStub.called, "run() should not be called if disabled with pref set");
+    runStub.reset();
+
+    // Runner re-enabled + pref on.
+    RecipeRunner.enable();
+    await rsClient.emit("sync", {});
+    ok(runStub.called, "run() should be called at most once if runner is re-enabled");
+    runStub.reset();
+
+    await SpecialPowers.pushPrefEnv({ set: [["features.normandy-remote-settings.enabled", false]] });
+
+    // Runner enabled + pref off.
+    await rsClient.emit("sync", {});
+    ok(!runStub.called, "run() should not be called if pref is unset");
+    runStub.reset();
+
+    // Runner disabled + pref off.
+    RecipeRunner.disable();
+    await rsClient.emit("sync", {});
+    ok(!runStub.called, "run() should still not be called if disabled");
+    RecipeRunner.enable();
+  }
+);
+
+decorate_task(
+  withStub(RecipeRunner, "loadRecipes"),
+  async function testRunCanRunOnlyOnce(
+    loadRecipesStub,
+  ) {
+    // eslint-disable-next-line mozilla/no-arbitrary-setTimeout
+    loadRecipesStub.returns(new Promise((resolve) => setTimeout(() => resolve([]), 10)));
+
+    // // Run 2 in parallel.
+    await Promise.all([
+      RecipeRunner.run(),
+      RecipeRunner.run(),
+    ]);
+
+    is(loadRecipesStub.callCount, 1, "run() is no-op if already running");
+  }
+);
+
+decorate_task(
+  withPrefEnv({
+    set: [
+      ["features.normandy-remote-settings.enabled", true],
+    ],
+  }),
+  withStub(RecipeRunner, "loadRecipes"),
+  withStub(ActionsManager.prototype, "finalize"),
+  withStub(Uptake, "reportRunner"),
+  async function testSyncDelaysTimer(
+    loadRecipesStub,
+    finalizeStub,
+    reportRecipeStub,
+  ) {
+    loadRecipesStub.returns(Promise.resolve([]));
+    // Set a timer interval as small as possible so that the UpdateTimerManager
+    // will pick the recipe runner as the most imminent timer to run on `notify()`.
+    Services.prefs.setIntPref("app.normandy.run_interval_seconds", 1);
+    // This will refresh the timer interval.
+    RecipeRunner.unregisterTimer();
+    RecipeRunner.registerTimer();
+
+    // Simulate timer notification.
+    const service = Cc["@mozilla.org/updates/timer-manager;1"].getService(Ci.nsITimerCallback);
+    const newTimer = () => {
+      const t = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
+      t.initWithCallback(() => { }, 10, Ci.nsITimer.TYPE_ONE_SHOT);
+      return t;
+    };
+    // Run timer once, to make sure this test works as expected.
+    const startTime = Date.now();
+    const endPromise = TestUtils.topicObserved("recipe-runner:end");
+    service.notify(newTimer());
+    await endPromise; // will timeout if run() not called.
+    const timerLatency = Date.now() - startTime;
+
+    // Run once from sync event.
+    const rsClient = RecipeRunner._remoteSettingsClientForTesting;
+    await rsClient.emit("sync", {}); // waits for listeners to run.
+
+    // Run timer again.
+    service.notify(newTimer());
+    // Wait at least as long as the latency we had above. Ten times as a margin.
+    // eslint-disable-next-line mozilla/no-arbitrary-setTimeout
+    await new Promise((resolve) => setTimeout(resolve, timerLatency * 10));
+
+    is(loadRecipesStub.callCount, 2, "run() does not run again from timer after sync");
+  }
+);