commit 8e8a13aaf431e7956c240d2dbabce5b9cbeff7c9
parent e4bfb45d8f897ae1fe9371b3a67affb0bdb9258d
Author: Beth Rennie <beth@brennie.ca>
Date: Tue, 7 Oct 2025 13:13:19 +0000
Bug 1985079 - Throw in RemoteSettingsExperimentLoader when fetching records r=nimbus-reviewers,relud
Differential Revision: https://phabricator.services.mozilla.com/D264294
Diffstat:
3 files changed, 60 insertions(+), 143 deletions(-)
diff --git a/toolkit/components/nimbus/lib/Migrations.sys.mjs b/toolkit/components/nimbus/lib/Migrations.sys.mjs
@@ -129,7 +129,7 @@ async function migrateEnrollmentsToSql() {
return;
}
- const { recipes } =
+ const recipes =
await lazy.ExperimentAPI._rsLoader.getRecipesFromAllCollections({
trigger: "migration",
});
diff --git a/toolkit/components/nimbus/lib/RemoteSettingsExperimentLoader.sys.mjs b/toolkit/components/nimbus/lib/RemoteSettingsExperimentLoader.sys.mjs
@@ -400,8 +400,18 @@ export class RemoteSettingsExperimentLoader {
await SCHEMAS.NimbusExperiment
);
}
- const { recipes: allRecipes, loadingError } =
- await this.getRecipesFromAllCollections({ forceSync, trigger });
+
+ let allRecipes;
+ let loadingError = false;
+ try {
+ allRecipes = await this.getRecipesFromAllCollections({
+ forceSync,
+ trigger,
+ });
+ } catch (e) {
+ loadingError = true;
+ lazy.log.error("Failed to update from Remote Settings", e);
+ }
if (!loadingError) {
const unenrolledExperimentSlugs = lazy.NimbusEnrollments
@@ -448,7 +458,10 @@ export class RemoteSettingsExperimentLoader {
Services.prefs.setIntPref(TIMER_LAST_UPDATE_PREF, lastUpdateTime);
}
- Services.obs.notifyObservers(null, "nimbus:enrollments-updated");
+ if (!loadingError) {
+ // Enrollments have not changed, so we don't need to notify.
+ Services.obs.notifyObservers(null, "nimbus:enrollments-updated");
+ }
}
/**
@@ -465,45 +478,35 @@ export class RemoteSettingsExperimentLoader {
* @param {string} options.trigger The name of the event that triggered the
* update.
*
- * @returns {Promise<{ recipes: object[]; loadingError: boolean; }>} The
- * recipes from Remote Settings.
+ * @returns {Promise<object[]>} The recipes from Remote Settings.
+ * @throws {Error} If we fail to retrieve recipes from any collection.
*/
async getRecipesFromAllCollections({ forceSync = false, trigger } = {}) {
- const recipes = [];
- let loadingError = false;
-
- const experiments = await this.getRecipesFromCollection({
- forceSync,
- client: this.remoteSettingsClients[EXPERIMENTS_COLLECTION],
- ...RS_COLLECTION_OPTIONS[EXPERIMENTS_COLLECTION],
- });
+ let experiments = null;
+ let secureExperiments = null;
- if (experiments !== null) {
- recipes.push(...experiments);
- } else {
- loadingError = true;
- }
-
- const secureExperiments = await this.getRecipesFromCollection({
- forceSync,
- client: this.remoteSettingsClients[SECURE_EXPERIMENTS_COLLECTION],
- ...RS_COLLECTION_OPTIONS[SECURE_EXPERIMENTS_COLLECTION],
- });
+ try {
+ experiments = await this.getRecipesFromCollection({
+ forceSync,
+ client: this.remoteSettingsClients[EXPERIMENTS_COLLECTION],
+ ...RS_COLLECTION_OPTIONS[EXPERIMENTS_COLLECTION],
+ });
- if (secureExperiments !== null) {
- recipes.push(...secureExperiments);
- } else {
- loadingError = true;
+ secureExperiments = await this.getRecipesFromCollection({
+ forceSync,
+ client: this.remoteSettingsClients[SECURE_EXPERIMENTS_COLLECTION],
+ ...RS_COLLECTION_OPTIONS[SECURE_EXPERIMENTS_COLLECTION],
+ });
+ } finally {
+ lazy.NimbusTelemetry.recordRemoteSettingsSync(
+ forceSync,
+ experiments,
+ secureExperiments,
+ trigger
+ );
}
- lazy.NimbusTelemetry.recordRemoteSettingsSync(
- forceSync,
- experiments,
- secureExperiments,
- trigger
- );
-
- return { recipes, loadingError };
+ return [...experiments, ...secureExperiments];
}
/**
@@ -520,10 +523,13 @@ export class RemoteSettingsExperimentLoader {
* @param {Set<string> | undefined} options.disallowedFeatureIds
* If a recipe uses any features in this list, it will be rejected.
*
- * @returns {object[] | null}
+ * @returns {object[]}
* Recipes from the collection, filtered to match the allowed and
* disallowed feature IDs, or null if there was an error syncing the
* collection.
+ *
+ * @throws {Error} If we fail to get the recipes from the Remote Settings
+ * client.
*/
async getRecipesFromCollection({
client,
@@ -531,34 +537,25 @@ export class RemoteSettingsExperimentLoader {
requiredFeatureIds = undefined,
disallowedFeatureIds = undefined,
} = {}) {
- let recipes;
- try {
- recipes = await client.get({
- forceSync,
- emptyListFallback: false, // Throw instead of returning an empty list.
- });
- if (!Array.isArray(recipes)) {
- throw new Error("Remote Settings did not return an array");
- }
- if (
- recipes.length === 0 &&
- (await client.db.getLastModified()) === null
- ) {
- throw new Error(
- "Remote Settings returned an empty list but should have thrown (no last modified)"
- );
- }
- lazy.log.debug(
- `Got ${recipes.length} recipes from ${client.collectionName}`
- );
- } catch (e) {
- lazy.log.debug(
- `Error getting recipes from Remote Settings collection ${client.collectionName}: ${e}`
- );
+ const recipes = await client.get({
+ forceSync,
+ emptyListFallback: false, // Throw instead of returning an empty list.
+ });
- return null;
+ if (!Array.isArray(recipes)) {
+ throw new Error("Remote Settings did not return an array");
}
+ if (recipes.length === 0 && (await client.db.getLastModified()) === null) {
+ throw new Error(
+ "Remote Settings returned an empty list but should have thrown (no last modified)"
+ );
+ }
+
+ lazy.log.debug(
+ `Got ${recipes.length} recipes from ${client.collectionName}`
+ );
+
return recipes.filter(recipe => {
if (
requiredFeatureIds &&
diff --git a/toolkit/components/nimbus/test/unit/test_RemoteSettingsExperimentLoader.js b/toolkit/components/nimbus/test/unit/test_RemoteSettingsExperimentLoader.js
@@ -124,86 +124,6 @@ add_task(async function test_updateRecipes() {
await cleanup();
});
-add_task(async function test_loadingErrorOnEmptyRecipesWithNullLastModified() {
- Services.fog.testResetFOG();
- const { sandbox, loader, cleanup } = await NimbusTestUtils.setupTest({
- clearTelemetry: true,
- });
-
- Assert.deepEqual(
- Glean.nimbusEvents.remoteSettingsSync
- .testGetValue("events")
- ?.map(ev => ev.extra) ?? [],
- [
- {
- force_sync: "false",
- experiments_success: "true",
- secure_experiments_success: "true",
- experiments_empty: "true",
- secure_experiments_empty: "true",
- trigger: "enabled",
- },
- ],
- "Submitted initial remoteSettingsSync telemetry"
- );
- Services.fog.testResetFOG();
-
- sandbox
- .stub(loader.remoteSettingsClients.experiments.db, "getLastModified")
- .resolves(null);
-
- let { loadingError } = await loader.getRecipesFromAllCollections({
- trigger: "test",
- });
-
- Assert.ok(
- loadingError,
- "should error when loading empty recipes collection with null last modified"
- );
-
- Assert.deepEqual(
- Glean.nimbusEvents.remoteSettingsSync
- .testGetValue("events")
- ?.map(ev => ev.extra) ?? [],
- [
- {
- force_sync: "false",
- experiments_success: "false",
- secure_experiments_success: "true",
- secure_experiments_empty: "true",
- trigger: "test",
- },
- ],
- "Submitted failure telemetry"
- );
-
- Services.fog.testResetFOG();
-
- loader.remoteSettingsClients.experiments.get.resolves([
- NimbusTestUtils.factories.recipe("test"),
- ]);
-
- ({ loadingError } = await loader.getRecipesFromAllCollections({
- trigger: "test",
- }));
-
- Assert.strictEqual(
- loadingError,
- false,
- "should not error when loading nonempty recipes collection"
- );
-
- Assert.deepEqual(
- Glean.nimbusEvents.remoteSettingsSync
- .testGetValue("events")
- ?.map(ev => ev.extra) ?? [],
- [],
- "Didn't submit success telemetry"
- );
-
- await cleanup();
-});
-
add_task(async function test_enrollmentsContextFirstStartup() {
const { sandbox, manager, cleanup } = await NimbusTestUtils.setupTest();
