Bug 1623465 - Use suitabilities for preference experiments r=Gijs

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

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

toolkit/components/normandy/actions/BaseAction.jsm

@@ -295,6 +295,7 @@ BaseAction.STATE_DISABLED = "ACTION_DISABLED";
 BaseAction.STATE_FAILED = "ACTION_FAILED";
 BaseAction.STATE_FINALIZED = "ACTION_FINALIZED";
 
+// Make sure to update the docs in ../docs/suitabilities.rst when changing this.
 BaseAction.suitability = {
   /**
    * The recipe's signature is not valid. If any action is taken this recipe

toolkit/components/normandy/actions/PreferenceExperimentAction.jsm

@@ -17,6 +17,11 @@ ChromeUtils.defineModuleGetter(
   "ActionSchemas",
   "resource://normandy/actions/schemas/index.js"
 );
+ChromeUtils.defineModuleGetter(
+  this,
+  "BaseAction",
+  "resource://normandy/actions/BaseAction.jsm"
+);
 ChromeUtils.defineModuleGetter(
   this,
   "ClientEnvironment",
@@ -45,7 +50,7 @@ class PreferenceExperimentAction extends BaseStudyAction {
     this.seenExperimentSlugs = [];
   }
 
-  async _run(recipe) {
+  async _processRecipe(recipe, suitability) {
     const {
       branches,
       isHighPopulation,
@@ -53,61 +58,126 @@ class PreferenceExperimentAction extends BaseStudyAction {
       slug,
       userFacingName,
       userFacingDescription,
-    } = recipe.arguments;
+    } = recipe.arguments || {};
 
-    this.seenExperimentSlugs.push(slug);
+    let experiment;
+    // Slug might not exist, because if suitability is ARGUMENTS_INVALID, the
+    // arguments is not guaranteed to match the schema.
+    if (slug) {
+      this.seenExperimentSlugs.push(slug);
 
-    // If we're not in the experiment, enroll!
-    const hasSlug = await PreferenceExperiments.has(slug);
-    if (!hasSlug) {
-      // Check all preferences that could be used by this experiment.
-      // If there's already an active experiment that has set that preference, abort.
-      const activeExperiments = await PreferenceExperiments.getAllActive();
-      for (const branch of branches) {
-        const conflictingPrefs = Object.keys(branch.preferences).filter(
-          preferenceName => {
-            return activeExperiments.some(exp =>
-              exp.preferences.hasOwnProperty(preferenceName)
-            );
-          }
-        );
-        if (conflictingPrefs.length) {
-          throw new Error(
-            `Experiment ${slug} ignored; another active experiment is already using the
-            ${conflictingPrefs[0]} preference.`
-          );
+      try {
+        experiment = await PreferenceExperiments.get(slug);
+      } catch (err) {
+        // This is probably that the experiment doesn't exist. If that's not the
+        // case, re-throw the error.
+        if (!(err instanceof PreferenceExperiments.NotFoundError)) {
+          throw err;
         }
       }
+    }
 
-      // Determine if enrollment is currently paused for this experiment.
-      if (isEnrollmentPaused) {
-        this.log.debug(`Enrollment is paused for experiment "${slug}"`);
-        return;
+    switch (suitability) {
+      case BaseAction.suitability.SIGNATURE_ERROR: {
+        this._considerTemporaryError({ experiment, reason: "signature-error" });
+        break;
       }
 
-      // Otherwise, enroll!
-      const branch = await this.chooseBranch(slug, branches);
-      const experimentType = isHighPopulation ? "exp-highpop" : "exp";
-      await PreferenceExperiments.start({
-        slug,
-        actionName: this.name,
-        branch: branch.slug,
-        preferences: branch.preferences,
-        experimentType,
-        userFacingName,
-        userFacingDescription,
-      });
-    } else {
-      // If the experiment exists, and isn't expired, bump the lastSeen date.
-      const experiment = await PreferenceExperiments.get(slug);
-      if (experiment.expired) {
-        this.log.debug(`Experiment ${slug} has expired, aborting.`);
-      } else {
-        await PreferenceExperiments.markLastSeen(slug);
+      case BaseAction.suitability.CAPABILITES_MISMATCH: {
+        if (experiment) {
+          await PreferenceExperiments.stop(slug, {
+            resetValue: true,
+            reason: "capability-mismatch",
+          });
+        }
+        break;
+      }
+
+      case BaseAction.suitability.FILTER_MATCH: {
+        // If we're not in the experiment, try to enroll
+        if (!experiment) {
+          // Check all preferences that could be used by this experiment.
+          // If there's already an active experiment that has set that preference, abort.
+          const activeExperiments = await PreferenceExperiments.getAllActive();
+          for (const branch of branches) {
+            const conflictingPrefs = Object.keys(branch.preferences).filter(
+              preferenceName => {
+                return activeExperiments.some(exp =>
+                  exp.preferences.hasOwnProperty(preferenceName)
+                );
+              }
+            );
+            if (conflictingPrefs.length) {
+              throw new Error(
+                `Experiment ${slug} ignored; another active experiment is already using the
+            ${conflictingPrefs[0]} preference.`
+              );
+            }
+          }
+
+          // Determine if enrollment is currently paused for this experiment.
+          if (isEnrollmentPaused) {
+            this.log.debug(`Enrollment is paused for experiment "${slug}"`);
+            return;
+          }
+
+          // Otherwise, enroll!
+          const branch = await this.chooseBranch(slug, branches);
+          const experimentType = isHighPopulation ? "exp-highpop" : "exp";
+          await PreferenceExperiments.start({
+            slug,
+            actionName: this.name,
+            branch: branch.slug,
+            preferences: branch.preferences,
+            experimentType,
+            userFacingName,
+            userFacingDescription,
+          });
+        } else if (experiment.expired) {
+          this.log.debug(`Experiment ${slug} has expired, aborting.`);
+        } else {
+          experiment.temporaryErrorDeadline = null;
+          await PreferenceExperiments.update(experiment);
+          await PreferenceExperiments.markLastSeen(slug);
+        }
+        break;
+      }
+
+      case BaseAction.suitability.FILTER_MISMATCH: {
+        if (experiment) {
+          await PreferenceExperiments.stop(slug, {
+            resetValue: true,
+            reason: "filter-mismatch",
+          });
+        }
+        break;
+      }
+
+      case BaseAction.suitability.FILTER_ERROR: {
+        this._considerTemporaryError({ experiment, reason: "filter-error" });
+        break;
+      }
+
+      case BaseAction.suitability.ARGUMENTS_INVALID: {
+        if (experiment) {
+          await PreferenceExperiments.stop(slug, {
+            resetValue: true,
+            reason: "arguments-invalid",
+          });
+        }
+        break;
+      }
+
+      default: {
+        throw new Error(`Unknown recipe suitability "${suitability}".`);
       }
     }
   }
 
+  async _run(recipe) {
+    throw new Error("_run shouldn't be called anymore");
+  }
+
   async chooseBranch(slug, branches) {
     const ratios = branches.map(branch => branch.ratio);
     const userId = ClientEnvironment.userId;
@@ -153,4 +223,52 @@ class PreferenceExperimentAction extends BaseStudyAction {
       })
     );
   }
+
+  /**
+   * Given that a temporary error has occured for an experiment, check if it
+   * should be temporarily ignored, or if the deadline has passed. If the
+   * deadline is passed, the experiment will be ended. If this is the first
+   * temporary error, a deadline will be generated. Otherwise, nothing will
+   * happen.
+   *
+   * If a temporary deadline exists but cannot be parsed, a new one will be
+   * made.
+   *
+   * The deadline is 7 days from the first time that recipe failed, as
+   * reckoned by the client's clock.
+   *
+   * @param {Object} args
+   * @param {Experiment} args.experiment The enrolled experiment to potentially unenroll.
+   * @param {String} args.reason If the recipe should end, the reason it is ending.
+   */
+  async _considerTemporaryError({ experiment, reason }) {
+    if (!experiment) {
+      return;
+    }
+
+    let now = Date.now(); // milliseconds-since-epoch
+    let day = 24 * 60 * 60 * 1000;
+    let newDeadline = new Date(now + 7 * day).toJSON();
+
+    if (experiment.temporaryErrorDeadline) {
+      let deadline = new Date(experiment.temporaryErrorDeadline);
+      // if deadline is an invalid date, set it to one week from now.
+      if (isNaN(deadline)) {
+        experiment.temporaryErrorDeadline = newDeadline;
+        await PreferenceExperiments.update(experiment);
+        return;
+      }
+
+      if (now > deadline) {
+        await PreferenceExperiments.stop(experiment.slug, {
+          resetValue: true,
+          reason,
+        });
+      }
+    } else {
+      // there is no deadline, so set one
+      experiment.temporaryErrorDeadline = newDeadline;
+      await PreferenceExperiments.update(experiment);
+    }
+  }
 }

toolkit/components/normandy/docs/execution-model.rst

@@ -23,6 +23,8 @@ their suitability for this client. Recipes contain information about which
 clients should execute the recipe. All recipes are processed by all clients,
 and all filtering happens in the client.
 
+For more information, see `the suitabilities docs <./suitabilities.html>`_.
+
 Signature
 ~~~~~~~~~
 

toolkit/components/normandy/docs/index.rst

@@ -26,3 +26,5 @@ and then executes them.
 
    data-collection
    execution-model
+   suitabilities
+   services

toolkit/components/normandy/docs/services.rst

@@ -0,0 +1,22 @@
+Normandy Services
+=================
+The Normandy Client relies on several external services for correct operation.
+
+Normandy Server
+---------------
+This is the place where recipes are created, edited and approved. Normandy
+Client interacts with it to get an index of services, and to fetch extensions
+and their metadata for add-on studies and rollout.
+
+Remote Settings
+---------------
+This is the primary way that recipes are loaded from the internet by
+Normandy. It manages keeping the local list of recipes on the client up to
+date and notifying Normandy Client of changes.
+
+Classify Client
+---------------
+This is a service that helps Normandy with filtering. It determines the
+region a user is in, based on IP. It also includes an up-to-date time and
+date. This allows Normandy to perform location and time based filtering
+without having to rely on the local clock.

toolkit/components/normandy/docs/suitabilities.rst

@@ -0,0 +1,74 @@
+Suitabilities
+=============
+
+When Normandy's core passes a recipe to an action, it also passes a
+*suitability*, which is the result of evaluating a recipe's filters,
+compatibility, signatures, and other checks. This gives actions more
+information to make decisions with, which is especially important for
+experiments.
+
+Temporary errors
+----------------
+Some of the suitabilities below represent *temporary errors*. These could be
+caused by infrastructure problems that prevent the recipe from working
+correctly, or are otherwise not the fault of the recipe itself. These
+suitabilities should not immediately cause a change in state. If the problem
+persists, then eventually it should be considered permanent and state should
+be changed.
+
+In the case of a permanent failure, action such as unenrollment should happen
+immediately. For temporary failures, that action should be delayed until the
+failure persists longer than some threshold. It is up to individual actions
+to track and manage this transition.
+
+List of Suitabilities
+---------------------
+
+``FILTER_MATCH``
+~~~~~~~~~~~~~~~~
+All checks have passed, and the recipe is suitable to execute in this client.
+Experiments and Rollouts should enroll or update. Heartbeats should be shown
+to the user, etc.
+
+``SIGNATURE_ERROR``
+~~~~~~~~~~~~~~~~~~~
+The recipe's signature is not valid. If any action is taken this recipe
+should be treated with extreme suspicion.
+
+This should be considered a temporary error, because it may be related to
+server errors, local clocks, or other temporary problems.
+
+``CAPABILITES_MISMATCH``
+~~~~~~~~~~~~~~~~~~~~~~~~
+The recipe requires capabilities that this recipe runner does not have. Use
+caution when interacting with this recipe, as it may not match the expected
+schema.
+
+This should be considered a permanent error, because it is the result of a
+choice made on the server.
+
+``FILTER_MISMATCH``
+~~~~~~~~~~~~~~~~~~~
+This client does not match the recipe's filter, but it is otherwise a
+suitable recipe.
+
+This should be considered a permanent error, since the filter explicitly does
+not match the client.
+
+``FILTER_ERROR``
+~~~~~~~~~~~~~~~~
+There was an error while evaluating the filter. It is unknown if this client
+matches this filter. This may be temporary, due to network errors, or
+permanent due to syntax errors.
+
+This should be considered a temporary error, because it may be the result of
+infrastructure, such as `Classify Client <./services.html#classify-client>`_,
+temporarily failing.
+
+``ARGUMENTS_INVALID``
+~~~~~~~~~~~~~~~~~~~~~
+The arguments of the recipe do not match the expected schema for the named
+action.
+
+This should be considered a permanent error, since the arguments are generally validated by the server. This likely represents an unrecogonized compatibility error.
+

toolkit/components/normandy/lib/PreferenceExperiments.jsm

@@ -37,6 +37,10 @@
  * @property {string} lastSeen
  *   ISO-formatted date string of when the experiment was last seen from the
  *   recipe server.
+ * @property {string|null} temporaryErrorDeadline
+ *   ISO-formatted date string of when temporary errors with this experiment
+ *   should not longer be considered temporary. After this point, further errors
+ *   will result in unenrollment.
  * @property {Object} preferences
  *   An object consisting of all the preferences that are set by this experiment.
  *   Keys are the name of each preference affected by this experiment. Values are
@@ -772,7 +776,10 @@ var PreferenceExperiments = {
     }
 
     experiment.expired = true;
-    store.saveSoon();
+    if (experiment.temporaryErrorDeadline) {
+      experiment.temporaryErrorDeadline = null;
+    }
+    await store.saveSoon();
 
     TelemetryEnvironment.setExperimentInactive(experimentSlug);
     TelemetryEvents.sendEvent("unenroll", "preference_study", experimentSlug, {
@@ -783,6 +790,11 @@ var PreferenceExperiments = {
         experiment.enrollmentId || TelemetryEvents.NO_ENROLLMENT_ID_MARKER,
     });
     await this.saveStartupPrefs();
+    Services.obs.notifyObservers(
+      null,
+      "normandy:preference-experiment:stopped",
+      experimentSlug
+    );
   },
 
   /**
@@ -812,7 +824,7 @@ var PreferenceExperiments = {
     log.debug(`PreferenceExperiments.get(${experimentSlug})`);
     const store = await ensureStorage();
     if (!(experimentSlug in store.data.experiments)) {
-      throw new Error(
+      throw new PreferenceExperiments.NotFoundError(
         `Could not find a preference experiment with the slug "${experimentSlug}"`
       );
     }
@@ -853,6 +865,28 @@ var PreferenceExperiments = {
     return experimentSlug in store.data.experiments;
   },
 
+  /**
+   * Update an experiment in the data store. If an experiment with the given
+   * slug is not already in the store, an error will be thrown.
+   *
+   * @param experiment {Experiment} The experiment to update
+   * @param experiment.slug {String} The experiment must have a slug
+   */
+  async update(experiment) {
+    const store = await ensureStorage();
+
+    if (!(experiment.slug in store.data.experiments)) {
+      throw new Error(
+        `Could not update a preference experiment with the slug "${experiment.slug}"`
+      );
+    }
+
+    store.data.experiments[experiment.slug] = experiment;
+    store.saveSoon();
+  },
+
+  NotFoundError: class extends Error {},
+
   /**
    * These migrations should only be called from `NormandyMigrations.jsm` and tests.
    */

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

@@ -1878,8 +1878,13 @@ decorate_task(
     // setting the preference on the user branch should trigger the observer to stop the experiment
     mockPreferences.set("fake.preference", "uservalue", "user");
 
-    // let the event loop tick to run the observer
-    await Promise.resolve();
+    // Wait until the change is processed
+    await TestUtils.topicObserved(
+      "normandy:preference-experiment:stopped",
+      (subject, message) => {
+        return message == "test";
+      }
+    );
 
     sendEventStub.assertEvents([
       [

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

@@ -18,6 +18,7 @@ ChromeUtils.import(
   "resource://normandy/actions/PreferenceExperimentAction.jsm",
   this
 );
+ChromeUtils.import("resource://testing-common/NormandyTestUtils.jsm", this);
 
 function branchFactory(opts = {}) {
   const defaultPreferences = {
@@ -43,7 +44,7 @@ function branchFactory(opts = {}) {
 }
 
 function argumentsFactory(args) {
-  const defaultBranches = (args && args.branches) || [{}];
+  const defaultBranches = (args && args.branches) || [{ preferences: [] }];
   const branches = defaultBranches.map(branchFactory);
   return {
     slug: "test",
@@ -476,3 +477,274 @@ decorate_task(
     Assert.deepEqual(await PreferenceExperiments.getAllActive(), []);
   }
 );
+
+// Check that the appropriate set of suitabilities are considered temporary errors
+decorate_task(
+  withStudiesEnabled,
+  PreferenceExperiments.withMockExperiments([]),
+  async function test_temporary_errors_set_deadline() {
+    let suitabilities = [
+      {
+        suitability: BaseAction.suitability.SIGNATURE_ERROR,
+        isTemporaryError: true,
+      },
+      {
+        suitability: BaseAction.suitability.CAPABILITES_MISMATCH,
+        isTemporaryError: false,
+      },
+      {
+        suitability: BaseAction.suitability.FILTER_MATCH,
+        isTemporaryError: false,
+      },
+      {
+        suitability: BaseAction.suitability.FILTER_MISMATCH,
+        isTemporaryError: false,
+      },
+      {
+        suitability: BaseAction.suitability.FILTER_ERROR,
+        isTemporaryError: true,
+      },
+      {
+        suitability: BaseAction.suitability.ARGUMENTS_INVALID,
+        isTemporaryError: false,
+      },
+    ];
+
+    Assert.deepEqual(
+      suitabilities.map(({ suitability }) => suitability).sort(),
+      Array.from(Object.values(BaseAction.suitability)).sort(),
+      "This test covers all suitabilities"
+    );
+
+    // The action should set a deadline 1 week from now. To avoid intermittent
+    // failures, give this a generous bound of 1 hour on either side.
+    let now = Date.now();
+    let hour = 60 * 60 * 1000;
+    let expectedDeadline = now + 7 * 24 * hour;
+    let minDeadline = new Date(expectedDeadline - hour);
+    let maxDeadline = new Date(expectedDeadline + hour);
+
+    // For each suitability, build a decorator that sets up a suitabilty
+    // environment, and then call that decorator with a sub-test that asserts
+    // the suitability is handled correctly.
+    for (const { suitability, isTemporaryError } of suitabilities) {
+      const decorator = PreferenceExperiments.withMockExperiments([
+        { slug: `test-for-suitability-${suitability}` },
+      ]);
+      await decorator(async ([experiment]) => {
+        let action = new PreferenceExperimentAction();
+        let recipe = preferenceExperimentFactory({ slug: experiment.slug });
+        await action.processRecipe(recipe, suitability);
+        let modifiedExperiment = await PreferenceExperiments.get(
+          experiment.slug
+        );
+        if (isTemporaryError) {
+          is(
+            typeof modifiedExperiment.temporaryErrorDeadline,
+            "string",
+            `A temporary failure deadline should be set as a string for suitability ${suitability}`
+          );
+          let deadline = new Date(modifiedExperiment.temporaryErrorDeadline);
+          ok(
+            deadline >= minDeadline && deadline <= maxDeadline,
+            `The temporary failure deadline should be in the expected range for ` +
+              `suitability ${suitability} (got ${deadline})`
+          );
+        } else {
+          ok(
+            !modifiedExperiment.temporaryErrorDeadline,
+            `No temporary failure deadline should be set for suitability ${suitability}`
+          );
+        }
+      })();
+    }
+  }
+);
+
+// Check that if there is an existing deadline, temporary errors don't overwrite it
+decorate_task(
+  withStudiesEnabled,
+  withMockPreferences,
+  PreferenceExperiments.withMockExperiments([]),
+  async function test_temporary_errors_dont_overwrite_deadline() {
+    let temporaryFailureSuitabilities = [
+      BaseAction.suitability.SIGNATURE_ERROR,
+      BaseAction.suitability.FILTER_ERROR,
+    ];
+
+    // A deadline one hour in the future won't be hit during the test.
+    let now = Date.now();
+    let hour = 60 * 60 * 1000;
+    let unhitDeadline = new Date(now + hour).toJSON();
+
+    // For each suitability, build a decorator that sets up a suitabilty
+    // environment, and then call that decorator with a sub-test that asserts
+    // the suitability is handled correctly.
+    for (const suitability of temporaryFailureSuitabilities) {
+      const decorator = PreferenceExperiments.withMockExperiments([
+        {
+          slug: `test-for-suitability-${suitability}`,
+          expired: false,
+          temporaryErrorDeadline: unhitDeadline,
+        },
+      ]);
+      await decorator(async ([experiment]) => {
+        let action = new PreferenceExperimentAction();
+        let recipe = preferenceExperimentFactory({ slug: experiment.slug });
+        await action.processRecipe(recipe, suitability);
+        let modifiedExperiment = await PreferenceExperiments.get(
+          experiment.slug
+        );
+        is(
+          modifiedExperiment.temporaryErrorDeadline,
+          unhitDeadline,
+          `The temporary failure deadline should not be cleared for suitability ${suitability}`
+        );
+      })();
+    }
+  }
+);
+
+// Check that if the deadline is past, temporary errors end the experiment.
+decorate_task(
+  withStudiesEnabled,
+  withMockPreferences,
+  PreferenceExperiments.withMockExperiments([]),
+  async function test_temporary_errors_hit_deadline() {
+    let temporaryFailureSuitabilities = [
+      BaseAction.suitability.SIGNATURE_ERROR,
+      BaseAction.suitability.FILTER_ERROR,
+    ];
+
+    // Set a deadline of an hour in the past, so that the experiment expires.
+    let now = Date.now();
+    let hour = 60 * 60 * 1000;
+    let hitDeadline = new Date(now - hour).toJSON();
+
+    // For each suitability, build a decorator that sets up a suitabilty
+    // environment, and then call that decorator with a sub-test that asserts
+    // the suitability is handled correctly.
+    for (const suitability of temporaryFailureSuitabilities) {
+      const decorator = PreferenceExperiments.withMockExperiments([
+        {
+          slug: `test-for-suitability-${suitability}`,
+          expired: false,
+          temporaryErrorDeadline: hitDeadline,
+          preferences: [],
+          branch: "test-branch",
+        },
+      ]);
+      await decorator(async ([experiment]) => {
+        let action = new PreferenceExperimentAction();
+        let recipe = preferenceExperimentFactory({ slug: experiment.slug });
+        await action.processRecipe(recipe, suitability);
+        let modifiedExperiment = await PreferenceExperiments.get(
+          experiment.slug
+        );
+        ok(
+          modifiedExperiment.expired,
+          `The experiment should be expired for suitability ${suitability}`
+        );
+      })();
+    }
+  }
+);
+
+// Check that non-error suitabilities clear the temporary deadline
+decorate_task(
+  withStudiesEnabled,
+  withMockPreferences,
+  PreferenceExperiments.withMockExperiments([]),
+  async function test_non_temporary_error_clears_temporary_error_deadline() {
+    let suitabilitiesThatShouldClearDeadline = [
+      BaseAction.suitability.CAPABILITES_MISMATCH,
+      BaseAction.suitability.FILTER_MATCH,
+      BaseAction.suitability.FILTER_MISMATCH,
+      BaseAction.suitability.ARGUMENTS_INVALID,
+    ];
+
+    // Use a deadline in the past to demonstrate that even if the deadline has
+    // passed, only a temporary error suitability ends the experiment.
+    let now = Date.now();
+    let hour = 60 * 60 * 1000;
+    let hitDeadline = new Date(now - hour).toJSON();
+
+    // For each suitability, build a decorator that sets up a suitabilty
+    // environment, and then call that decorator with a sub-test that asserts
+    // the suitability is handled correctly.
+    for (const suitability of suitabilitiesThatShouldClearDeadline) {
+      const decorator = PreferenceExperiments.withMockExperiments([
+        NormandyTestUtils.factories.preferenceStudyFactory({
+          slug: `test-for-suitability-${suitability}`.toLocaleLowerCase(),
+          expired: false,
+          temporaryErrorDeadline: hitDeadline,
+        }),
+      ]);
+      await decorator(async ([experiment]) => {
+        let action = new PreferenceExperimentAction();
+        let recipe = preferenceExperimentFactory({ slug: experiment.slug });
+        await action.processRecipe(recipe, suitability);
+        let modifiedExperiment = await PreferenceExperiments.get(
+          experiment.slug
+        );
+        ok(
+          !modifiedExperiment.temporaryErrorDeadline,
+          `The temporary failure deadline should be cleared for suitabilitiy ${suitability}`
+        );
+      })();
+    }
+  }
+);
+
+// Check that invalid deadlines are reset
+decorate_task(
+  withStudiesEnabled,
+  withMockPreferences,
+  PreferenceExperiments.withMockExperiments([]),
+  async function test_non_temporary_error_clears_temporary_error_deadline() {
+    let temporaryFailureSuitabilities = [
+      BaseAction.suitability.SIGNATURE_ERROR,
+      BaseAction.suitability.FILTER_ERROR,
+    ];
+
+    // The action should set a deadline 1 week from now. To avoid intermittent
+    // failures, give this a generous bound of 2 hours on either side.
+    let invalidDeadline = "not a valid date";
+    let now = Date.now();
+    let hour = 60 * 60 * 1000;
+    let expectedDeadline = now + 7 * 24 * hour;
+    let minDeadline = new Date(expectedDeadline - 2 * hour);
+    let maxDeadline = new Date(expectedDeadline + 2 * hour);
+
+    // For each suitability, build a decorator that sets up a suitabilty
+    // environment, and then call that decorator with a sub-test that asserts
+    // the suitability is handled correctly.
+    for (const suitability of temporaryFailureSuitabilities) {
+      const decorator = PreferenceExperiments.withMockExperiments([
+        NormandyTestUtils.factories.preferenceStudyFactory({
+          slug: `test-for-suitability-${suitability}`.toLocaleLowerCase(),
+          expired: false,
+          temporaryErrorDeadline: invalidDeadline,
+        }),
+      ]);
+      await decorator(async ([experiment]) => {
+        let action = new PreferenceExperimentAction();
+        let recipe = preferenceExperimentFactory({ slug: experiment.slug });
+        await action.processRecipe(recipe, suitability);
+        is(action.lastError, null, "No errors should be reported");
+        let modifiedExperiment = await PreferenceExperiments.get(
+          experiment.slug
+        );
+        ok(
+          modifiedExperiment.temporaryErrorDeadline != invalidDeadline,
+          `The temporary failure deadline should be reset for suitabilitiy ${suitability}`
+        );
+        let deadline = new Date(modifiedExperiment.temporaryErrorDeadline);
+        ok(
+          deadline >= minDeadline && deadline <= maxDeadline,
+          `The temporary failure deadline should be reset to a valid deadline for ${suitability}`
+        );
+      })();
+    }
+  }
+);