Bug 1621806 - Reduce frequency of client-side extension-storage syncs. r=markh

Reduce frequency of client-side extension-storage syncs

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

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

modules/libpref/init/all.js

@@ -4838,10 +4838,13 @@ pref("services.common.log.logger.tokenserverclient", "Debug");
 
   // The maximum number of immediate resyncs to trigger for changes made during
   // a sync.
-  pref("services.sync.maxResyncs", 5);
+  pref("services.sync.maxResyncs", 1);
 
   // The URL of the Firefox Accounts auth server backend
   pref("identity.fxaccounts.auth.uri", "https://api.accounts.firefox.com/v1");
+
+  // Percentage chance we skip an extension storage sync (kinto life support).
+  pref("services.sync.extension-storage.skipPercentageChance", 20);
 #endif // MOZ_SERVICES_SYNC
 
 // Marionette is the remote protocol that lets OOP programs communicate with,

services/sync/modules/engines.js

@@ -977,6 +977,17 @@ SyncEngine.prototype = {
     return this.ensureCurrentSyncID(Utils.makeGUID());
   },
 
+  /**
+   * Allows overriding scheduler logic -- added to help reduce kinto server
+   * getting hammered because our scheduler never got tuned for it.
+   *
+   * Note: Overriding engines must take resyncs into account -- score will not
+   * be cleared.
+   */
+  shouldSkipSync(syncReason) {
+    return false;
+  },
+
   /*
    * lastSync is a timestamp in server time.
    */

services/sync/modules/engines/extension-storage.js

@@ -6,13 +6,16 @@
 
 var EXPORTED_SYMBOLS = ["ExtensionStorageEngine"];
 
-const { SCORE_INCREMENT_MEDIUM } = ChromeUtils.import(
+const { SCORE_INCREMENT_MEDIUM, MULTI_DEVICE_THRESHOLD } = ChromeUtils.import(
   "resource://services-sync/constants.js"
 );
 const { SyncEngine, Tracker } = ChromeUtils.import(
   "resource://services-sync/engines.js"
 );
 const { Svc } = ChromeUtils.import("resource://services-sync/util.js");
+const { XPCOMUtils } = ChromeUtils.import(
+  "resource://gre/modules/XPCOMUtils.jsm"
+);
 ChromeUtils.defineModuleGetter(
   this,
   "extensionStorageSync",
@@ -29,6 +32,12 @@ ChromeUtils.defineModuleGetter(
  */
 function ExtensionStorageEngine(service) {
   SyncEngine.call(this, "Extension-Storage", service);
+  XPCOMUtils.defineLazyPreferenceGetter(
+    this,
+    "_skipPercentageChance",
+    "services.sync.extension-storage.skipPercentageChance",
+    0
+  );
 }
 ExtensionStorageEngine.prototype = {
   __proto__: SyncEngine.prototype,
@@ -64,6 +73,33 @@ ExtensionStorageEngine.prototype = {
   _wipeClient() {
     return extensionStorageSync.clearAll();
   },
+
+  shouldSkipSync(syncReason) {
+    if (syncReason == "user" || syncReason == "startup") {
+      this._log.info(
+        `Not skipping extension storage sync: reason == ${syncReason}`
+      );
+      // Always sync if a user clicks the button, or if we're starting up.
+      return false;
+    }
+    // Ensure this wouldn't cause a resync...
+    if (this._tracker.score >= MULTI_DEVICE_THRESHOLD) {
+      this._log.info(
+        "Not skipping extension storage sync: Would trigger resync anyway"
+      );
+      return false;
+    }
+
+    let probability = this._skipPercentageChance / 100.0;
+    // Math.random() returns a value in the interval [0, 1), so `>` is correct:
+    // if `probability` is 1 skip every time, and if it's 0, never skip.
+    let shouldSkip = probability > Math.random();
+
+    this._log.info(
+      `Skipping extension-storage sync with a chance of ${probability}: ${shouldSkip}`
+    );
+    return shouldSkip;
+  },
 };
 
 function ExtensionStorageTracker(name, engine) {

services/sync/modules/policies.js

@@ -361,14 +361,19 @@ SyncScheduler.prototype = {
         }
 
         let sync_interval;
+        let nextSyncReason = "schedule";
         this.updateGlobalScore();
-        if (this.globalScore > 0 && Status.service == STATUS_OK) {
-          // The global score should be 0 after a sync. If it's not, items were
-          // changed during the last sync, and we should schedule an immediate
-          // follow-up sync.
+        if (
+          this.globalScore > this.syncThreshold &&
+          Status.service == STATUS_OK
+        ) {
+          // The global score should be 0 after a sync. If it's not, either
+          // items were changed during the last sync (and we should schedule an
+          // immediate follow-up sync), or an engine skipped
           this._resyncs++;
           if (this._resyncs <= this.maxResyncs) {
             sync_interval = 0;
+            nextSyncReason = "resync";
           } else {
             this._log.warn(
               `Resync attempt ${this._resyncs} exceeded ` +
@@ -388,7 +393,7 @@ SyncScheduler.prototype = {
           this._log.trace("Scheduling a sync at interval NO_SYNC_NODE_FOUND.");
           sync_interval = NO_SYNC_NODE_INTERVAL;
         }
-        this.scheduleNextSync(sync_interval, { why: "schedule" });
+        this.scheduleNextSync(sync_interval, { why: nextSyncReason });
         break;
       case "weave:engine:sync:finish":
         if (data == "clients") {

services/sync/modules/stages/enginesync.js

@@ -171,6 +171,10 @@ EngineSynchronizer.prototype = {
       // We don't bother validating engines that failed to sync.
       let enginesToValidate = [];
       for (let engine of enginesToSync) {
+        if (engine.shouldSkipSync(why)) {
+          this._log.info(`Engine ${engine.name} asked to be skipped`);
+          continue;
+        }
         // If there's any problems with syncing the engine, report the failure
         if (
           !(await this._syncEngine(engine)) ||

services/sync/tests/unit/test_engine_changes_during_sync.js

@@ -355,6 +355,16 @@ add_task(async function test_bookmark_change_during_sync() {
   _("Ensure that we track bookmark changes made during a sync.");
 
   enableValidationPrefs();
+  let schedulerProto = Object.getPrototypeOf(Service.scheduler);
+  let syncThresholdDescriptor = Object.getOwnPropertyDescriptor(
+    schedulerProto,
+    "syncThreshold"
+  );
+  Object.defineProperty(Service.scheduler, "syncThreshold", {
+    // Trigger resync if any changes exist, rather than deciding based on the
+    // normal sync threshold.
+    get: () => 0,
+  });
 
   let engine = Service.engineManager.get("bookmarks");
   let server = await serverForEnginesWithKeys({ foo: "password" }, [engine]);
@@ -609,6 +619,11 @@ add_task(async function test_bookmark_change_during_sync() {
       "Buffered and legacy engines should validate after second sync"
     );
   } finally {
+    Object.defineProperty(
+      schedulerProto,
+      "syncThreshold",
+      syncThresholdDescriptor
+    );
     engine._uploadOutgoing = uploadOutgoing;
     await cleanup(engine, server);
   }

services/sync/tests/unit/test_extension_storage_engine.js

@@ -28,11 +28,19 @@ function mock(options) {
   return ret;
 }
 
+function setSkipChance(v) {
+  Services.prefs.setIntPref(
+    "services.sync.extension-storage.skipPercentageChance",
+    v
+  );
+}
+
 add_task(async function setup() {
   await Service.engineManager.register(ExtensionStorageEngine);
   engine = Service.engineManager.get("extension-storage");
   do_get_profile(); // so we can use FxAccounts
   loadWebExtensionTestFunctions();
+  setSkipChance(0);
 });
 
 add_task(async function test_calling_sync_calls__sync() {
@@ -51,6 +59,39 @@ add_task(async function test_calling_sync_calls__sync() {
   equal(syncMock.calls.length, 1);
 });
 
+add_task(async function test_sync_skip() {
+  try {
+    // Do a few times to ensure we aren't getting "lucky" WRT Math.random()
+    for (let i = 0; i < 10; ++i) {
+      setSkipChance(100);
+      engine._tracker._score = 0;
+      ok(
+        !engine.shouldSkipSync("user"),
+        "Should allow explicitly requested syncs"
+      );
+      ok(!engine.shouldSkipSync("startup"), "Should allow startup syncs");
+      ok(
+        engine.shouldSkipSync("schedule"),
+        "Should skip scheduled syncs if skipProbability is 100"
+      );
+      engine._tracker._score = MULTI_DEVICE_THRESHOLD;
+      ok(
+        !engine.shouldSkipSync("schedule"),
+        "should allow scheduled syncs if tracker score is high"
+      );
+      engine._tracker._score = 0;
+      setSkipChance(0);
+      ok(
+        !engine.shouldSkipSync("schedule"),
+        "Should allow scheduled syncs if probability is 0"
+      );
+    }
+  } finally {
+    engine._tracker._score = 0;
+    setSkipChance(0);
+  }
+});
+
 add_task(async function test_calling_wipeClient_calls_clearAll() {
   let oldClearAll = extensionStorageSync.clearAll;
   let clearMock = (extensionStorageSync.clearAll = mock({

services/sync/tests/unit/test_syncscheduler.js

@@ -182,6 +182,54 @@ add_test(function test_prefAttributes() {
   run_next_test();
 });
 
+add_task(async function test_sync_skipped_low_score_no_resync() {
+  enableValidationPrefs();
+  let server = await sync_httpd_setup();
+
+  function SkipEngine() {
+    SyncEngine.call(this, "Skip", Service);
+    this.syncs = 0;
+  }
+
+  SkipEngine.prototype = {
+    __proto__: SyncEngine.prototype,
+    _sync() {
+      do_throw("Should have been skipped");
+    },
+    shouldSkipSync() {
+      return true;
+    },
+  };
+  await Service.engineManager.register(SkipEngine);
+
+  let engine = Service.engineManager.get("skip");
+  engine.enabled = true;
+  engine._tracker._score = 30;
+
+  Assert.equal(Status.sync, SYNC_SUCCEEDED);
+
+  Assert.ok(await setUp(server));
+
+  let resyncDoneObserver = promiseOneObserver("weave:service:resyncs-finished");
+
+  let synced = false;
+  function onSyncStarted() {
+    Assert.ok(!synced, "Only should sync once");
+    synced = true;
+  }
+
+  await Service.sync();
+
+  Assert.equal(Status.sync, SYNC_SUCCEEDED);
+
+  Svc.Obs.add("weave:service:sync:start", onSyncStarted);
+  await resyncDoneObserver;
+
+  Svc.Obs.remove("weave:service:sync:start", onSyncStarted);
+  engine._tracker._store = 0;
+  await cleanUpAndGo(server);
+});
+
 add_task(async function test_updateClientMode() {
   _(
     "Test updateClientMode adjusts scheduling attributes based on # of clients appropriately"
@@ -817,7 +865,7 @@ add_task(async function test_sync_failed_partial_noresync() {
   let engine = Service.engineManager.get("catapult");
   engine.enabled = true;
   engine.exception = "Bad news";
-  engine._tracker._score = 10;
+  engine._tracker._score = MULTI_DEVICE_THRESHOLD + 1;
 
   Assert.equal(Status.sync, SYNC_SUCCEEDED);