commit 15612057bb9f84a543fc512bff64943952975e47 parent 8e8a13aaf431e7956c240d2dbabce5b9cbeff7c9 Author: Beth Rennie <beth@brennie.ca> Date: Tue, 7 Oct 2025 13:13:19 +0000 Bug 1985079 - Keep track of Remote Settings collection lastModified timestamps r=nimbus-reviewers,profiles-reviewers,jhirsch,relud,nalexander Differential Revision: https://phabricator.services.mozilla.com/D264295 Diffstat:
11 files changed, 1318 insertions(+), 161 deletions(-)
diff --git a/browser/components/profiles/SelectableProfileService.sys.mjs b/browser/components/profiles/SelectableProfileService.sys.mjs @@ -1369,6 +1369,13 @@ class SelectableProfileServiceClass extends EventEmitter { profileId: lazy.ExperimentAPI.profileId, } ); + + await db.execute( + "DELETE FROM NimbusSyncTimestamps WHERE profileId = :profileId;", + { + profileId: lazy.ExperimentAPI.profileId, + } + ); } ); diff --git a/toolkit/components/backgroundtasks/BackgroundTask_message.sys.mjs b/toolkit/components/backgroundtasks/BackgroundTask_message.sys.mjs @@ -61,6 +61,7 @@ const lazy = {}; ChromeUtils.defineESModuleGetters(lazy, { ClientEnvironmentBase: "resource://gre/modules/components-utils/ClientEnvironment.sys.mjs", + ExperimentAPI: "resource://nimbus/ExperimentAPI.sys.mjs", ExtensionUtils: "resource://gre/modules/ExtensionUtils.sys.mjs", IndexedDB: "resource://gre/modules/IndexedDB.sys.mjs", RemoteSettings: "resource://services-settings/remote-settings.sys.mjs", @@ -82,10 +83,44 @@ outputInfo = (sentinel, info) => { dump(`${sentinel}${JSON.stringify(info)}${sentinel}\n`); }; -function monkeyPatchRemoteSettingsClient({ data = [] }) { - lazy.RemoteSettingsClient.prototype.get = async (options = {}) => { - outputInfo({ "RemoteSettingsClient.get": { options, response: { data } } }); - return data; +/** + * Monkey patch RemoteSettings clients to prevent HTTP connections and return + * static data from the Nimbus collections. + * + * @param {object} options + * @param {object[]} options.experiments + * The experiments to return from the Nimbus experiments collections. + */ +function monkeyPatchRemoteSettingsClients({ experiments = [] }) { + const now = Date.now(); + + const nimbusClients = lazy.ExperimentAPI._rsLoader.remoteSettingsClients; + + // We aren't actually syncing these collections. In order for Nimbus to accept + // recipes from these collections, they *must* have non-null value returned by + // `getLastModified`. Otherwise, Nimbus will reject the recipes. + const getLastModified = () => Promise.resolve(now); + nimbusClients.experiments.db.getLastModified = getLastModified; + nimbusClients.secureExperiments.db.getLastModified = getLastModified; + + // Prevent RemoteSettings clients from making HTTP connections to the Remote + // Settings server, which will cause test failures. Additionally, return + // `experiments` from both Nimbus collections. These collections each only + // support a subset of features -- the secureExperiments collection, e.g., + // only supports recipes that have at least one of `newtabTrainhopAddon` and + // `prefFlips` -- but the `RemoteSettingsExperimentLoader` will filter out the + // recipes from each collection based on their supported features. + lazy.RemoteSettingsClient.prototype.get = async function (options = {}) { + const responseData = + this === nimbusClients.experiments || + this === nimbusClients.secureExperiments + ? experiments + : []; + + outputInfo({ + "RemoteSettingsClient.get": { options, response: { data: responseData } }, + }); + return responseData; }; } @@ -218,7 +253,7 @@ async function handleCommandLine(commandLine) { data = [data]; } - monkeyPatchRemoteSettingsClient({ data }); + monkeyPatchRemoteSettingsClients({ experiments: data }); console.log(`Saw --experiments, read recipes from ${experimentsPath}`); } @@ -228,7 +263,7 @@ async function handleCommandLine(commandLine) { !experiments && commandLine.handleFlag("no-experiments", CASE_INSENSITIVE) ) { - monkeyPatchRemoteSettingsClient({ data: [] }); + monkeyPatchRemoteSettingsClients({ experiments: [] }); console.log(`Saw --no-experiments, returning [] recipes`); } diff --git a/toolkit/components/nimbus/lib/Enrollments.sys.mjs b/toolkit/components/nimbus/lib/Enrollments.sys.mjs @@ -11,8 +11,11 @@ const lazy = {}; ChromeUtils.defineESModuleGetters(lazy, { DeferredTask: "resource://gre/modules/DeferredTask.sys.mjs", ExperimentAPI: "resource://nimbus/ExperimentAPI.sys.mjs", + NimbusTelemetry: "resource://nimbus/lib/Telemetry.sys.mjs", ProfilesDatastoreService: "moz-src:///toolkit/profile/ProfilesDatastoreService.sys.mjs", + RemoteSettingsSyncError: + "resource://nimbus/lib/RemoteSettingsExperimentLoader.sys.mjs", }); ChromeUtils.defineLazyGetter(lazy, "log", () => { @@ -49,6 +52,88 @@ let SYNC_ENROLLMENTS_ENABLED = Services.prefs.getBoolPref( false ); +export class PendingWrites { + /** + * Create a new `PendingWrites` object. + * + * @param {PendingWrites} other A `PendingWrites` object to clone. + */ + constructor(other = undefined) { + /** + * Enrollment changes to flush to the database. + * + * The keys are enrollment slugs and the values are optional recipes. If the + * recipe is present, a new enrollment will be created in the database. + * Otherwise, an existing enrollment will be updated to be inactive (or + * deleted if it cannot be found in the `ExperimentStore`). + * + * @type {Map<string, object | null>} + */ + this.enrollments = new Map(other?.enrollments.entries() ?? []); + + /** + * Sync timestamps to flush to the database. + * + * @type {Map<string, object | null>} + */ + this.syncTimestamps = new Map(other?.syncTimestamps.entries() ?? []); + } + + /** + * Whether or not there are pending writes. + * + * @returns {boolean} + */ + get hasPendingWrites() { + return this.enrollments.size > 0 || this.syncTimestamps.size > 0; + } + + /** + * Register an enrollment update. + * + * @param {string} slug The slug of the enrollment that is changing. + * @param {object | undefined} recipe If this update is for an enrollment event, + * the recipe that resulted in the enrollment. + */ + updateEnrollment(slug, recipe) { + // Don't overwrite a pending entry that has a recipe with one that has none + // or we will try to do the wrong query (UPDATE instead of INSERT). + // + // We explicitly check for the presence of the value, not the key, in case + // this is a re-enrollment following an unenrollment. + if (!this.enrollments.get(slug)) { + this.enrollments.set(slug, recipe); + } + } + + /** + * Merge the two collections of pending writes together. + * + * @param {PendingWrites} first The first set of pending writes to apply. + * @param {PendingWrites} second The second set of pending writes to apply. + * Pending syncTimestamps in this set will override those set in `first`. + * + * @returns {PendingWrites} + */ + static merge(first, second) { + if (!first.hasPendingWrites) { + return second; + } else if (!second.hasPendingWrites) { + return first; + } + + const merged = new PendingWrites(first); + for (const [slug, recipe] of second.enrollments.entries()) { + merged.updateEnrollment(slug, recipe); + } + for (const [collection, lastModified] of second.syncTimestamps.entries()) { + merged.syncTimestamps.set(collection, lastModified); + } + + return merged; + } +} + /** * Handles queueing changes to the NimbusEnrollments database table in the * shared profiles database. @@ -93,14 +178,19 @@ export class NimbusEnrollments { /** * Pending writes that will be flushed in `#flush()`. * - * The keys are enrollment slugs and the values are optional recipes. If the - * recipe is present, a new enrollment will be created in the database. - * Otherwise, an existing enrollment will be updated to be inactive (or - * deleted if it cannot be found in the `ExperimentStore`). + * @type {PendingWrites} + */ + #pendingWrites; + + /** + * The lastModified times of the Nimbus Remote Settings collections. + * + * This will always contain the most up to date set of timestamps, even if + * they are not flushed to the database. * - * @type {Map<string, object | null>} + * @type {Map<string, number> | null} */ - #pending; + #syncTimestamps; constructor(store) { this.#initialized = false; @@ -118,7 +208,8 @@ export class NimbusEnrollments { ); this.#finalized = false; - this.#pending = new Map(); + this.#pendingWrites = new PendingWrites(); + this.#syncTimestamps = null; } async init() { @@ -130,7 +221,13 @@ export class NimbusEnrollments { const conn = await lazy.ProfilesDatastoreService.getConnection(); return conn.executeTransaction(async txn => { - return NimbusEnrollments.loadEnrollments(txn); + // Only load enrollments if the database is the source-of-truth for reads. + const enrollments = NimbusEnrollments.readFromDatabaseEnabled + ? await NimbusEnrollments.loadEnrollments(txn) + : null; + this.#syncTimestamps = await NimbusEnrollments.loadSyncTimestamps(txn); + + return enrollments; }); } @@ -138,7 +235,7 @@ export class NimbusEnrollments { * The number of pending writes. */ get pendingWrites() { - return this.#pending.size; + return this.#pendingWrites.enrollments.size; } /** @@ -158,14 +255,57 @@ export class NimbusEnrollments { lazy.log.debug(`Queued update for enrollment ${slug}`); - // Don't overwrite a pending entry that has a recipe with one that has none - // or we will try to do the wrong query (UPDATE instead of INSERT). - // - // We explicitly check for the presence of the value, not the key, in case - // this is a re-enrollment following an unenrollment. - if (!this.#pending.get(slug)) { - this.#pending.set(slug, recipe); + this.#pendingWrites.updateEnrollment(slug, recipe); + this.#flushSoon(); + } + + /** + * Update the known lastModified timestamps for the given collections. + * + * Omitted timestamps will not change. + * + * @param {Map<string, number>} timestamps The timestamps to update. + * + * @throws {RemoteSettingsSyncError} If any timestamps are behind currently known + * timestamps or if any timestamps are invalid. + */ + updateSyncTimestamps(timestamps) { + if (!this.#initialized) { + throw new Error("Not initialized"); + } + + const mergedTimestamps = new Map(this.#syncTimestamps.entries()); + let timestampsChanged = false; + + for (const [collection, timestamp] of timestamps) { + const lastTimestamp = this.#syncTimestamps.get(collection); + + if (typeof timestamp !== "number" || isNaN(timestamp) || timestamp < 0) { + throw new lazy.RemoteSettingsSyncError( + collection, + lazy.NimbusTelemetry.RemoteSettingsSyncErrorReason.INVALID_LAST_MODIFIED + ); + } else if ( + typeof lastTimestamp === "undefined" || + timestamp > lastTimestamp + ) { + mergedTimestamps.set(collection, timestamp); + this.#pendingWrites.syncTimestamps.set(collection, timestamp); + + timestampsChanged = true; + } else if (timestamp < lastTimestamp) { + throw new lazy.RemoteSettingsSyncError( + collection, + lazy.NimbusTelemetry.RemoteSettingsSyncErrorReason.BACKWARDS_SYNC + ); + } + } + + if (timestampsChanged) { + this.#syncTimestamps = mergedTimestamps; this.#flushSoon(); + + lazy.log.debug("Timestamps updated"); } } @@ -199,7 +339,8 @@ export class NimbusEnrollments { } /** - * Flush all pending updates to the NimbusEnrollments table. + * Flush all pending updates to the NimbusEnrollments and NimbusSyncTimestamps + * tables. * * The updates are done as a single transaction to ensure the database stays * in a consistent state. @@ -214,15 +355,21 @@ export class NimbusEnrollments { * we've started shutting down. */ async #flush({ retryOnFailure = true } = {}) { - if (!this.#pending.size) { + if (!this.#pendingWrites.hasPendingWrites) { lazy.log.debug(`Not flushing: no changes`); return; } - const pending = this.#pending; - this.#pending = new Map(); + // Swap the set of pending writes, if there are any, with default values. + // While we are waiting for the writes to the database complete we may + // receive more pending writes. If our write to the database fails, we need + // to reconcile those changes by merging them. + const pendingWrites = this.#pendingWrites; + this.#pendingWrites = new PendingWrites(); - lazy.log.debug(`Flushing ${pending.size} changes to database`); + lazy.log.debug( + `Flushing ${pendingWrites.enrollments.size} enrollments, ${pendingWrites.syncTimestamps.size} timestamps to database` + ); let success = true; try { @@ -240,35 +387,35 @@ export class NimbusEnrollments { return; } - await conn.executeTransaction(async () => { - for (const [slug, recipe] of pending.entries()) { + await conn.executeTransaction(async txn => { + for (const [slug, recipe] of pendingWrites.enrollments.entries()) { const enrollment = this.#store.get(slug); if (enrollment) { - await this.#insertOrUpdateEnrollment(conn, enrollment, recipe); + await NimbusEnrollments.#insertOrUpdateEnrollment( + txn, + enrollment, + recipe + ); } else { - await this.#deleteEnrollment(conn, slug); + await NimbusEnrollments.#deleteEnrollment(txn, slug); } } + + await NimbusEnrollments.#flushSyncTimestamps( + txn, + pendingWrites.syncTimestamps + ); }); } catch (e) { success = false; if (retryOnFailure) { - // Re-queue all the pending writes that failed. - if (this.#pending.size) { - // If there have been store changes since the call to flush, we need to - // include all of those and the failed writes. - const newPending = this.#pending; - this.#pending = pending; - - for (const [slug, recipe] of newPending.entries()) { - this.updateEnrollment(slug, recipe); - } - } else { - // If there have been no pending changes, we can just replace the set of - // pending writes. - this.#pending = pending; - } + // Re-queue all the pending writes that failed and merge any new changes + // that happened while we attempted to save. + this.#pendingWrites = PendingWrites.merge( + pendingWrites, + this.#pendingWrites + ); if (!this.#finalized) { lazy.log.error( @@ -308,7 +455,7 @@ export class NimbusEnrollments { * @param {object | null} recipe The recipe for the enrollment. Only non-null * when the initial enrollment has not already been flushed. */ - async #insertOrUpdateEnrollment(conn, enrollment, recipe) { + static async #insertOrUpdateEnrollment(conn, enrollment, recipe) { if (recipe) { // This was a new enrollment at the time. It may have since unenrolled. await conn.executeCached( @@ -401,7 +548,7 @@ export class NimbusEnrollments { * @param {OpenedConnection} conn The connection to the database. * @param {string} slug The slug of the enrollment to delete. */ - async #deleteEnrollment(conn, slug) { + static async #deleteEnrollment(conn, slug) { await conn.execute( ` DELETE FROM NimbusEnrollments @@ -440,6 +587,33 @@ export class NimbusEnrollments { this.#shutdownBlocker = null; } + static async #flushSyncTimestamps(conn, timestamps) { + for (const [collection, lastModified] of timestamps) { + await conn.executeCached( + ` + INSERT INTO NimbusSyncTimestamps( + profileId, + collection, + lastModified + ) + VALUES( + :profileId, + :collection, + :lastModified + ) + ON CONFLICT(profileId, collection) + DO UPDATE SET + lastModified = excluded.lastModified; + `, + { + profileId: lazy.ExperimentAPI.profileId, + collection, + lastModified, + } + ); + } + } + /** * Whether or not writing to the NimbusEnrollments table is enabled. * @@ -565,6 +739,40 @@ export class NimbusEnrollments { } /** + * Load the last known lastModified timestamps of Nimbus collections from the + * NimbusSyncTimestamps table. + * + * @param {OpenedConnection} txn An option connection, used when this is + * called within a transaction. + * + * @returns {Promise<Map<string, number>>} The lastModified timestamps of the + * Nimbus collections. + */ + static async loadSyncTimestamps(txn) { + const conn = txn ?? (await lazy.ProfilesDatastoreService.getConnection()); + const rows = await conn.execute( + ` + SELECT + collection, + lastModified + FROM NimbusSyncTimestamps + WHERE + profileId = :profileId; + `, + { + profileId: lazy.ExperimentAPI.profileId, + } + ); + + return new Map( + rows.map(row => [ + row.getResultByName("collection"), + row.getResultByName("lastModified"), + ]) + ); + } + + /** * Load the slugs of all experiments from other profiles that have unenrolled. * * @returns {Set<string>} The slugs of the experiments. diff --git a/toolkit/components/nimbus/lib/Migrations.sys.mjs b/toolkit/components/nimbus/lib/Migrations.sys.mjs @@ -12,6 +12,8 @@ ChromeUtils.defineESModuleGetters(lazy, { NimbusTelemetry: "resource://nimbus/lib/Telemetry.sys.mjs", ProfilesDatastoreService: "moz-src:///toolkit/profile/ProfilesDatastoreService.sys.mjs", + RemoteSettingsSyncError: + "resource://nimbus/lib/RemoteSettingsExperimentLoader.sys.mjs", }); ChromeUtils.defineLazyGetter(lazy, "log", () => { @@ -129,10 +131,18 @@ async function migrateEnrollmentsToSql() { return; } - const recipes = - await lazy.ExperimentAPI._rsLoader.getRecipesFromAllCollections({ + let recipes; + try { + recipes = await lazy.ExperimentAPI._rsLoader.getRecipesFromAllCollections({ trigger: "migration", }); + } catch (e) { + if (e instanceof lazy.RemoteSettingsSyncError) { + throw new MigrationError(e.reason); + } + + throw e; + } const recipesBySlug = new Map(recipes.map(r => [r.slug, r])); @@ -320,6 +330,7 @@ async function migrateFirefoxLabsEnrollments() { { mode: "shared" } ); } + export class MigrationError extends Error { static Reason = Object.freeze({ UNKNOWN: "unknown", diff --git a/toolkit/components/nimbus/lib/RemoteSettingsExperimentLoader.sys.mjs b/toolkit/components/nimbus/lib/RemoteSettingsExperimentLoader.sys.mjs @@ -2,6 +2,8 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ +/** @import { RemoteSettingsSyncErrorReason } from "./Telemetry.sys.mjs" */ + import { XPCOMUtils } from "resource://gre/modules/XPCOMUtils.sys.mjs"; const lazy = {}; @@ -171,6 +173,13 @@ export const CheckRecipeResult = { }, }; +/** + * @typedef {object} RecipeCollection + * @property {string} collectionName + * @property {object[]} recipes + * @property {number} lastModified + */ + export class RemoteSettingsExperimentLoader { /** * A shutdown blocker that will try to ensure that any ongoing update will @@ -401,19 +410,17 @@ export class RemoteSettingsExperimentLoader { ); } - let allRecipes; - let loadingError = false; + let allRecipes = null; try { allRecipes = await this.getRecipesFromAllCollections({ forceSync, trigger, }); } catch (e) { - loadingError = true; - lazy.log.error("Failed to update from Remote Settings", e); + lazy.log.debug("Failed to update", e); } - if (!loadingError) { + if (allRecipes !== null) { const unenrolledExperimentSlugs = lazy.NimbusEnrollments .syncEnrollmentsEnabled ? await lazy.NimbusEnrollments.loadUnenrolledExperimentSlugsFromOtherProfiles() @@ -458,7 +465,7 @@ export class RemoteSettingsExperimentLoader { Services.prefs.setIntPref(TIMER_LAST_UPDATE_PREF, lastUpdateTime); } - if (!loadingError) { + if (allRecipes !== null) { // Enrollments have not changed, so we don't need to notify. Services.obs.notifyObservers(null, "nimbus:enrollments-updated"); } @@ -479,34 +486,78 @@ export class RemoteSettingsExperimentLoader { * update. * * @returns {Promise<object[]>} The recipes from Remote Settings. - * @throws {Error} If we fail to retrieve recipes from any collection. + * + * @throws {RemoteSettingsSyncError} + * @throws {BackwardsSyncError} + * @throws {InvalidLastModifiedError} */ async getRecipesFromAllCollections({ forceSync = false, trigger } = {}) { - let experiments = null; - let secureExperiments = null; - try { - experiments = await this.getRecipesFromCollection({ - forceSync, - client: this.remoteSettingsClients[EXPERIMENTS_COLLECTION], - ...RS_COLLECTION_OPTIONS[EXPERIMENTS_COLLECTION], - }); + const recipes = []; + + // We may be in an xpcshell test that has not initialized the + // ProfilesDatastoreService. + // + // TODO(bug 1967779): require the ProfilesDatastoreService to be initialized + // and remove this check. + const timestamps = lazy.NimbusEnrollments.databaseEnabled + ? new Map() + : null; + + for (const collectionKind of [ + EXPERIMENTS_COLLECTION, + SECURE_EXPERIMENTS_COLLECTION, + ]) { + const client = this.remoteSettingsClients[collectionKind]; + const collectionOptions = RS_COLLECTION_OPTIONS[collectionKind]; + + const collection = await this.getRecipesFromCollection({ + forceSync, + client, + ...collectionOptions, + }); - 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 - ); - } + // It is much more likely for the secure experiments collection to be + // empty, so we do not emit telemetry when that is the case. + if ( + collection.recipes.length === 0 && + collectionKind !== SECURE_EXPERIMENTS_COLLECTION + ) { + lazy.NimbusTelemetry.recordRemoteSettingsSyncError( + client.collectionName, + lazy.NimbusTelemetry.RemoteSettingsSyncErrorReason.EMPTY, + { forceSync, trigger } + ); + } + + timestamps?.set(client.collectionName, collection.lastModified); - return [...experiments, ...secureExperiments]; + recipes.push(...collection.recipes); + } + + if (timestamps) { + // We may be in an xpcshell test that has not initialized the + // ProfilesDatastoreService. + // + // TODO(bug 1967779): require the ProfilesDatastoreService to be initialized + // and remove this check. + await this.manager.store._db.updateSyncTimestamps(timestamps); + } + + return recipes; + } catch (e) { + lazy.log.error("Failed to retrieve recipes from Remote Settings", e); + + if (e instanceof RemoteSettingsSyncError) { + lazy.NimbusTelemetry.recordRemoteSettingsSyncError( + e.collectionName, + e.reason, + { forceSync, trigger } + ); + } + + throw e; + } } /** @@ -523,13 +574,12 @@ export class RemoteSettingsExperimentLoader { * @param {Set<string> | undefined} options.disallowedFeatureIds * If a recipe uses any features in this list, it will be rejected. * - * @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. + * @returns {Promise<RecipeCollection>} The recipes and last modified + * timestamp from the collection, filtered based on `requiredFeatureIds` and + * `disallowedFeatureIds`. * - * @throws {Error} If we fail to get the recipes from the Remote Settings - * client. + * @throws {RemoteSettingsSyncError} If we fail to get the recipes from the + * Remote Settings client. */ async getRecipesFromCollection({ client, @@ -537,18 +587,42 @@ export class RemoteSettingsExperimentLoader { requiredFeatureIds = undefined, disallowedFeatureIds = undefined, } = {}) { - const recipes = await client.get({ - forceSync, - emptyListFallback: false, // Throw instead of returning an empty list. - }); + let recipes; + try { + recipes = await client.get({ + forceSync, + emptyListFallback: false, // Throw instead of returning an empty list. + }); + } catch (e) { + throw new RemoteSettingsSyncError( + client.collectionName, + lazy.NimbusTelemetry.RemoteSettingsSyncErrorReason.GET_EXCEPTION, + { cause: e } + ); + } if (!Array.isArray(recipes)) { - throw new Error("Remote Settings did not return an array"); + throw new RemoteSettingsSyncError( + client.collectionName, + lazy.NimbusTelemetry.RemoteSettingsSyncErrorReason.INVALID_DATA + ); } - 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)" + let lastModified; + try { + lastModified = await client.db.getLastModified(); + } catch (e) { + throw new RemoteSettingsSyncError( + client.collectionName, + lazy.NimbusTelemetry.RemoteSettingsSyncErrorReason.LAST_MODIFIED_EXCEPTION, + { cause: e } + ); + } + + if (recipes.length === 0 && lastModified === null) { + throw new RemoteSettingsSyncError( + client.collectionName, + lazy.NimbusTelemetry.RemoteSettingsSyncErrorReason.NULL_LAST_MODIFIED ); } @@ -556,7 +630,7 @@ export class RemoteSettingsExperimentLoader { `Got ${recipes.length} recipes from ${client.collectionName}` ); - return recipes.filter(recipe => { + const filteredRecipes = recipes.filter(recipe => { if ( requiredFeatureIds && !recipe.featureIds.some(featureId => requiredFeatureIds.has(featureId)) @@ -580,6 +654,12 @@ export class RemoteSettingsExperimentLoader { return true; }); + + return { + collectionName: client.collectionName, + recipes: filteredRecipes, + lastModified, + }; } async _optInToExperiment({ @@ -1183,3 +1263,44 @@ export class EnrollmentsContext { return schema; } } + +export class RemoteSettingsSyncError extends Error { + static getMessage(reason) { + const { RemoteSettingsSyncErrorReason } = lazy.NimbusTelemetry; + + switch (reason) { + case RemoteSettingsSyncErrorReason.BACKWARDS_SYNC: + return "would sync backwards"; + + case RemoteSettingsSyncErrorReason.GET_EXCEPTION: + return "RemoteSettings client threw an error"; + + case RemoteSettingsSyncErrorReason.INVALID_DATA: + return "did not return an array"; + + case RemoteSettingsSyncErrorReason.INVALID_LAST_MODIFIED: + return "invalid lastModified"; + + case RemoteSettingsSyncErrorReason.LAST_MODIFIED_EXCEPTION: + return "client threw when retrieving lastModified"; + + case RemoteSettingsSyncErrorReason.NULL_LAST_MODIFIED: + return "returned an empty list but lastModified was null"; + + default: + return "unknown error"; + } + } + + /** + * @param {string} collectionName The name of the collection. + * @param {RemoteSettingsSyncErrorReason} reason The reason for the error. + * @param {ErrorOptions | undefined} options Arguments to pass to the Error constructor. + */ + constructor(collectionName, reason, options) { + super(`Could not sync ${collectionName}: ${reason}`, options); + + this.collectionName = collectionName; + this.reason = reason; + } +} diff --git a/toolkit/components/nimbus/lib/SharedDataMap.sys.mjs b/toolkit/components/nimbus/lib/SharedDataMap.sys.mjs @@ -76,6 +76,17 @@ export class SharedDataMap extends EventEmitter { this._data = await this._db.init(); } else { this._data = this._jsonFile.data; + + // We still need to optionally load the database so that we have sync + // timestamp information. + if (lazy.NimbusEnrollments.databaseEnabled) { + // We may be in an xpcshell test that has not initialized the + // ProfilesDatastoreService. + // + // TODO(bug 1967779): require the ProfilesDatastoreService to be initialized + // and remove this check. + await this._db.init(); + } } this._syncToChildren({ flush: true }); diff --git a/toolkit/components/nimbus/lib/Telemetry.sys.mjs b/toolkit/components/nimbus/lib/Telemetry.sys.mjs @@ -44,6 +44,23 @@ const EnrollmentSource = Object.freeze({ FORCE_ENROLLMENT: "force-enrollment", }); +const RemoteSettingsSyncErrorReason = Object.freeze({ + BACKWARDS_SYNC: "backwards-sync", + EMPTY: "empty", + GET_EXCEPTION: "get-exception", + INVALID_DATA: "invalid-data", + INVALID_LAST_MODIFIED: "invalid-last-modified", + LAST_MODIFIED_EXCEPTION: "last-modified-exception", + NULL_LAST_MODIFIED: "null-last-modified", +}); + +/** + * @typedef {T[keyof T]} EnumValuesOf + * @template {type} T + */ + +/** @typedef {EnumValuesOf<typeof RemoteSettingsSyncErrorReason>} RemoteSettingsSyncErrorReason */ + const UnenrollmentFailureReason = Object.freeze({ ALREADY_UNENROLLED: "already-unenrolled", DOES_NOT_EXIST: "does-not-exist", @@ -88,6 +105,7 @@ export const NimbusTelemetry = { EnrollmentSource, EnrollmentStatus, EnrollmentStatusReason, + RemoteSettingsSyncErrorReason, UnenrollReason, UnenrollmentFailureReason, ValidationFailureReason, @@ -176,31 +194,31 @@ export const NimbusTelemetry = { ); }, - recordRemoteSettingsSync(forceSync, experiments, secureExperiments, trigger) { - // Do not record when all collections succeed and experiments isn't empty. - if ( - secureExperiments !== null && - experiments !== null && - experiments.length !== 0 - ) { - return; - } - + /** + * Record an error that occurred during + * + * @param {string} collection The name of the collection. + * @param {RemoteSettingsSyncErrorReason} reason The reason why the sync failed. + * @param {object} options + * @param {boolean} options.forceSync Whether or not the sync was forced. + * @param {string | undefined} options.trigger What event triggered the sync. + */ + recordRemoteSettingsSyncError( + collection, + reason, + { forceSync = false, trigger } = {} + ) { const event = { force_sync: forceSync, - experiments_success: experiments !== null, - secure_experiments_success: secureExperiments !== null, + collection, + reason, }; - if (experiments !== null) { - event.experiments_empty = experiments.length === 0; - } - if (secureExperiments !== null) { - event.secure_experiments_empty = secureExperiments.length === 0; - } - if (trigger != null) { + + if (typeof trigger === "string") { event.trigger = trigger; } - Glean.nimbusEvents.remoteSettingsSync.record(event); + + Glean.nimbusEvents.remoteSettingsSyncError.record(event); }, recordUnenrollment(enrollment, cause) { diff --git a/toolkit/components/nimbus/metrics.yaml b/toolkit/components/nimbus/metrics.yaml @@ -1034,6 +1034,8 @@ nimbus_events: Reasons include: - "unknown": an unknown exception occurred. + - one of the reasons documented in + `nimbus_events.remote_settings_sync_error`. enrollments: type: string @@ -1041,8 +1043,10 @@ nimbus_events: An optional string that includes any enrollments triggered by this relation bugs: - https://bugzilla.mozilla.org/show_bug.cgi?id=1937169 + - https://bugzilla.mozilla.org/show_bug.cgi?id=1985079 data_reviews: - https://bugzilla.mozilla.org/show_bug.cgi?id=1937169 + - https://bugzilla.mozilla.org/show_bug.cgi?id=1985079 data_sensitivity: - technical notification_emails: @@ -1127,48 +1131,51 @@ nimbus_events: - technical expires: never - remote_settings_sync: + remote_settings_sync_error: type: event description: > - Triggered whenever Nimbus attempts to get recipes from remote settings - collections, whether or not it succeeds. - extra_keys: - force_sync: - type: boolean - description: > - Whether or not a sync was forced when fetching recipes. + Triggered when Nimbus fails to get recipes from remote settings collections. - experiments_success: - type: boolean - description: > - Whether or not loading the experiments collection succeeded. - - secure_experiments_success: - type: boolean - description: > - Whether or not loading the secure experiments collection succeeded. - - experiments_empty: - type: boolean - description: > - Whether or not the experiments collection was empty. + extra_keys: + collection: + type: string + description: The name of the collection. - secure_experiments_empty: + force_sync: type: boolean - description: > - Whether or not the secure experiments collection was empty. + description: Whether or not the sync was forced. trigger: type: string + description: What triggered the sync. + + reason: + type: string description: > - The name of the event that triggered the sync. + What caused the error. One of: + + - `backwards-sync`: The collection would have synced backwards. + - `empty`: The collection is empty. (This is not strictly speaking an + error, but there is usually at least one live experiment or rollout + in each collection.) + - `get-exception`: RemoteSettingsClient.get() threw an exception. + - `invalid-data`: The collection did not return an array. + - `invalid-last-modified`: The collection returned an invalid value + for lastModified. + - `last-modified-exception`: Asking the collection for the lastModified + triggered an exception. + - `null-last-modified`: The collection was empty but lastModified was + null (i.e., not synced). + bugs: - - https://bugzilla.mozilla.org/show_bug.cgi?id=1968792 + - https://bugzilla.mozilla.org/show_bug.cgi?id=1985079 data_reviews: - - https://bugzilla.mozilla.org/show_bug.cgi?id=1968792 + - https://bugzilla.mozilla.org/show_bug.cgi?id=1985079 + data_sensitivity: - technical notification_emails: + - beth@mozilla.com - relud@mozilla.com - project-nimbus@mozilla.com expires: never diff --git a/toolkit/components/nimbus/test/NimbusTestUtils.sys.mjs b/toolkit/components/nimbus/test/NimbusTestUtils.sys.mjs @@ -722,6 +722,15 @@ export const NimbusTestUtils = { `, { profileId } ); + + await conn.execute( + ` + DELETE FROM NimbusSyncTimestamps + WHERE + profileId = :profileId; + `, + { profileId } + ); }, /** @@ -1170,8 +1179,17 @@ export const NimbusTestUtils = { .stub(loader.remoteSettingsClients.experiments, "get") .resolves(Array.isArray(experiments) ? experiments : []); sandbox + .stub(loader.remoteSettingsClients.experiments.db, "getLastModified") + .resolves(0); + sandbox .stub(loader.remoteSettingsClients.secureExperiments, "get") .resolves(Array.isArray(secureExperiments) ? secureExperiments : []); + sandbox + .stub( + loader.remoteSettingsClients.secureExperiments.db, + "getLastModified" + ) + .resolves(0); if (migrationState) { for (const [phase, value] of Object.entries(migrationState)) { diff --git a/toolkit/components/nimbus/test/unit/test_RemoteSettingsExperimentLoader_updateRecipes.js b/toolkit/components/nimbus/test/unit/test_RemoteSettingsExperimentLoader_updateRecipes.js @@ -1,5 +1,8 @@ "use strict"; +const { BrowserUtils } = ChromeUtils.importESModule( + "resource://gre/modules/BrowserUtils.sys.mjs" +); const { EnrollmentsContext, MatchStatus } = ChromeUtils.importESModule( "resource://nimbus/lib/RemoteSettingsExperimentLoader.sys.mjs" ); @@ -12,9 +15,6 @@ const { PanelTestProvider } = ChromeUtils.importESModule( const { TelemetryEnvironment } = ChromeUtils.importESModule( "resource://gre/modules/TelemetryEnvironment.sys.mjs" ); -const { TestUtils } = ChromeUtils.importESModule( - "resource://testing-common/TestUtils.sys.mjs" -); const { UnenrollmentCause } = ChromeUtils.importESModule( "resource://nimbus/lib/ExperimentManager.sys.mjs" ); @@ -42,6 +42,35 @@ function assertEnrollments(store, expectedActive, expectedInactive) { } } +async function assertSyncTimestamps(expectedTimestamps, message) { + await NimbusTestUtils.flushStore(); + + const timestamps = await NimbusEnrollments.loadSyncTimestamps(); + + Assert.deepEqual( + Object.fromEntries(timestamps.entries()), + expectedTimestamps, + message + ); +} + +async function updateAndAssertEnrollmentsUpdatedNotified( + loader, + shouldNotify, + message +) { + const didObserve = await Promise.race([ + BrowserUtils.promiseObserved("nimbus:enrollments-updated").then(() => true), + loader.updateRecipes("test").then(() => false), + ]); + + Assert.equal( + didObserve, + shouldNotify, + `${shouldNotify ? "should" : "should not"} send nimbus:enrollments-updated: ${message}` + ); +} + add_setup(async function setup() { Services.fog.initializeFOG(); }); @@ -2299,9 +2328,6 @@ async function testRsClientGetThrows(collectionName) { loader.remoteSettingsClients[collectionName].get.throws(); - // This topic is only notified if we reach the end of updateRecipes(). - const updatePromise = TestUtils.topicObserved("nimbus:enrollments-updated"); - await manager.enroll( NimbusTestUtils.factories.recipe.withFeatureConfig("recipe", { featureId: "no-feature-firefox-desktop", @@ -2309,24 +2335,17 @@ async function testRsClientGetThrows(collectionName) { "rs-loader" ); - await loader.updateRecipes(); - await updatePromise; + await updateAndAssertEnrollmentsUpdatedNotified( + loader, + false, + "collection threw" + ); Assert.ok( loader.getRecipesFromAllCollections.calledOnce, "getRecipesFromAllCollections called once" ); - Assert.deepEqual( - // We can't use .returned() because it is an async function and therefore - // returns a promise. Instead, we just call the function again. - await loader.getRecipesFromAllCollections(), - { - loadingError: true, - recipes: [], - } - ); - Assert.ok( manager.updateEnrollment.notCalled, "updateEnrollment never called" @@ -2652,3 +2671,685 @@ add_task(async function testUnenrolledInAnotherProfileBetweenUpdates() { await cleanup(); resetEnrollmentPrefs(); }); + +add_task(async function test_remoteSettingsSyncError_backwardsSync() { + const experiment = NimbusTestUtils.factories.recipe("experiment"); + const secureExperiment = NimbusTestUtils.factories.recipe.withFeatureConfig( + "secureExperiment", + { + featureId: "prefFlips", + value: { + prefs: {}, + }, + } + ); + + await assertSyncTimestamps({}, "No timestamps"); + + const { manager, loader, cleanup } = await NimbusTestUtils.setupTest({ + clearTelemetry: true, + }); + Services.fog.testResetFOG(); // Clear the empty events that were triggered during initialization. + + await assertSyncTimestamps( + { + "nimbus-desktop-experiments (stubbed)": 0, + "nimbus-secure-experiments (stubbed)": 0, + }, + "timestamps updated" + ); + + loader.remoteSettingsClients.experiments.db.getLastModified.resolves(12345); + loader.remoteSettingsClients.experiments.get.resolves([experiment]); + loader.remoteSettingsClients.secureExperiments.db.getLastModified.resolves( + 23456 + ); + loader.remoteSettingsClients.secureExperiments.get.resolves([ + secureExperiment, + ]); + + await updateAndAssertEnrollmentsUpdatedNotified( + loader, + true, + "collections healthy" + ); + await assertSyncTimestamps( + { + "nimbus-desktop-experiments (stubbed)": 12345, + "nimbus-secure-experiments (stubbed)": 23456, + }, + "timestamps updated" + ); + Assert.ok(manager.store.get("experiment")?.active, "experiment enrolled"); + Assert.ok( + manager.store.get("secureExperiment").active, + "secureExperiment enrolled" + ); + + Services.fog.testResetFOG(); + + // If a collections goes backwards, we will not sync. + loader.remoteSettingsClients.experiments.db.getLastModified.resolves(1); + loader.remoteSettingsClients.experiments.get.resolves([]); + loader.remoteSettingsClients.secureExperiments.db.getLastModified.resolves( + 34567 + ); + loader.remoteSettingsClients.secureExperiments.get.resolves([]); + + await updateAndAssertEnrollmentsUpdatedNotified( + loader, + false, + "experiments moved backwards" + ); + await assertSyncTimestamps( + { + "nimbus-desktop-experiments (stubbed)": 12345, + "nimbus-secure-experiments (stubbed)": 23456, + }, + "timestamps not updated" + ); + Assert.ok(manager.store.get("experiment")?.active, "experiment enrolled"); + Assert.ok( + manager.store.get("secureExperiment")?.active, + "secureExperiment enrolled" + ); + Assert.deepEqual( + Glean.nimbusEvents.remoteSettingsSyncError + .testGetValue("events") + ?.map(ev => ev.extra) ?? [], + [ + { + collection: "nimbus-desktop-experiments (stubbed)", + force_sync: "false", + trigger: "test", + reason: "empty", + }, + { + collection: "nimbus-desktop-experiments (stubbed)", + force_sync: "false", + trigger: "test", + reason: "backwards-sync", + }, + ], + "experiments moved backwards" + ); + + Services.fog.testResetFOG(); + + loader.remoteSettingsClients.experiments.db.getLastModified.resolves(45678); + loader.remoteSettingsClients.secureExperiments.db.getLastModified.resolves(1); + + await updateAndAssertEnrollmentsUpdatedNotified( + loader, + false, + "experiments moved backwards" + ); + await assertSyncTimestamps( + { + "nimbus-desktop-experiments (stubbed)": 12345, + "nimbus-secure-experiments (stubbed)": 23456, + }, + "timestamps not updated" + ); + Assert.ok(manager.store.get("experiment")?.active, "experiment enrolled"); + Assert.ok( + manager.store.get("secureExperiment")?.active, + "secureExperiment enrolled" + ); + Assert.deepEqual( + Glean.nimbusEvents.remoteSettingsSyncError + .testGetValue("events") + ?.map(ev => ev.extra) ?? [], + [ + { + collection: "nimbus-desktop-experiments (stubbed)", + force_sync: "false", + trigger: "test", + reason: "empty", + }, + { + collection: "nimbus-secure-experiments (stubbed)", + force_sync: "false", + trigger: "test", + reason: "backwards-sync", + }, + ], + "secureExperiments moved backwards" + ); + + Services.fog.testResetFOG(); + + // If both collections move backwards, we will only see a single error. + loader.remoteSettingsClients.experiments.db.getLastModified.resolves(1); + loader.remoteSettingsClients.secureExperiments.db.getLastModified.resolves(1); + + await updateAndAssertEnrollmentsUpdatedNotified( + loader, + false, + "both collections moved backwards" + ); + await assertSyncTimestamps( + { + "nimbus-desktop-experiments (stubbed)": 12345, + "nimbus-secure-experiments (stubbed)": 23456, + }, + "timestamps not updated" + ); + Assert.deepEqual( + Glean.nimbusEvents.remoteSettingsSyncError + .testGetValue("events") + ?.map(ev => ev.extra) ?? [], + [ + { + collection: "nimbus-desktop-experiments (stubbed)", + force_sync: "false", + trigger: "test", + reason: "empty", + }, + { + collection: "nimbus-desktop-experiments (stubbed)", + force_sync: "false", + trigger: "test", + reason: "backwards-sync", + }, + ], + "experiments moved backwards" + ); + + Services.fog.testResetFOG(); + + // If both collections are the same we won't see any errors. + loader.remoteSettingsClients.experiments.db.getLastModified.resolves(12345); + loader.remoteSettingsClients.experiments.get.resolves([experiment]); + loader.remoteSettingsClients.secureExperiments.db.getLastModified.resolves( + 23456 + ); + loader.remoteSettingsClients.secureExperiments.get.resolves([ + secureExperiment, + ]); + + await updateAndAssertEnrollmentsUpdatedNotified( + loader, + true, + "collections healthy" + ); + await assertSyncTimestamps( + { + "nimbus-desktop-experiments (stubbed)": 12345, + "nimbus-secure-experiments (stubbed)": 23456, + }, + "timestamps not updated" + ); + Assert.ok(manager.store.get("experiment").active, "experiment enrolled"); + Assert.ok( + manager.store.get("secureExperiment").active, + "secureExperiment enrolled" + ); + Assert.deepEqual( + Glean.nimbusEvents.remoteSettingsSyncError + .testGetValue("events") + ?.map(ev => ev.extra) ?? [], + [], + "No events" + ); + + // Both collections should be allowed to move forwards. + loader.remoteSettingsClients.experiments.db.getLastModified.resolves(23456); + loader.remoteSettingsClients.experiments.get.resolves([]); + + await updateAndAssertEnrollmentsUpdatedNotified( + loader, + true, + "collections healthy" + ); + await assertSyncTimestamps( + { + "nimbus-desktop-experiments (stubbed)": 23456, + "nimbus-secure-experiments (stubbed)": 23456, + }, + "timestamps updated" + ); + Assert.ok(!manager.store.get("experiment").active, "experiment unenrolled"); + Assert.ok( + manager.store.get("secureExperiment").active, + "secureExperiment enrolled" + ); + Assert.deepEqual( + Glean.nimbusEvents.remoteSettingsSyncError + .testGetValue("events") + ?.map(ev => ev.extra) ?? [], + [ + { + collection: "nimbus-desktop-experiments (stubbed)", + force_sync: "false", + trigger: "test", + reason: "empty", + }, + ], + "Experiment collection is empty" + ); + + Services.fog.testResetFOG(); + + loader.remoteSettingsClients.secureExperiments.db.getLastModified.resolves( + 34567 + ); + loader.remoteSettingsClients.secureExperiments.get.resolves([]); + + await updateAndAssertEnrollmentsUpdatedNotified( + loader, + true, + "collections healthy" + ); + await assertSyncTimestamps( + { + "nimbus-desktop-experiments (stubbed)": 23456, + "nimbus-secure-experiments (stubbed)": 34567, + }, + "timestamps updated" + ); + Assert.ok(!manager.store.get("experiment").active, "experiment unenrolled"); + Assert.ok( + !manager.store.get("secureExperiment").active, + "secureExperiment unenrolled" + ); + Assert.deepEqual( + Glean.nimbusEvents.remoteSettingsSyncError + .testGetValue("events") + ?.map(ev => ev.extra) ?? [], + [ + { + collection: "nimbus-desktop-experiments (stubbed)", + force_sync: "false", + trigger: "test", + reason: "empty", + }, + ], + "Collections empty" + ); + + await cleanup(); +}); + +add_task(async function test_remoteSettingsSyncError_empty() { + const experiment = NimbusTestUtils.factories.recipe("experiment"); + const secureExperiment = NimbusTestUtils.factories.recipe.withFeatureConfig( + "secureExperiment", + { + featureId: "prefFlips", + value: { + prefs: {}, + }, + } + ); + + Services.fog.testResetFOG(); + const { manager, loader, cleanup } = await NimbusTestUtils.setupTest({ + clearTelemetry: true, + }); + + // If both collections are empty we will see telemetry for both. There is a + // migration event and an enabled event during startup. + Assert.deepEqual( + Glean.nimbusEvents.remoteSettingsSyncError + .testGetValue("events") + ?.map(ev => ev.extra) ?? [], + [ + { + collection: "nimbus-desktop-experiments (stubbed)", + force_sync: "false", + trigger: "enabled", + reason: "empty", + }, + ], + "Submitted initial remoteSettingsSyncError telemetry" + ); + Services.fog.testResetFOG(); + + // We should observe an enrollment update. + await updateAndAssertEnrollmentsUpdatedNotified( + loader, + true, + "empty collections don't prevent enrollment update" + ); + Assert.deepEqual( + Glean.nimbusEvents.remoteSettingsSyncError + .testGetValue("events") + ?.map(ev => ev.extra) ?? [], + [ + { + collection: "nimbus-desktop-experiments (stubbed)", + force_sync: "false", + trigger: "test", + reason: "empty", + }, + ], + "Empty event telemetry submitted" + ); + + Services.fog.testResetFOG(); + + // If secureExperiments is empty we will see a single event and observe that + // enrollments updated. + loader.remoteSettingsClients.experiments.get.resolves([experiment]); + await updateAndAssertEnrollmentsUpdatedNotified( + loader, + true, + "empty collections don't prevent enrollment update" + ); + + Assert.ok(manager.store.get("experiment")?.active, "experiment is active"); + Assert.ok( + !manager.store.has("secureExperiment"), + "secureExperiment is not enrolled" + ); + + Assert.deepEqual( + Glean.nimbusEvents.remoteSettingsSyncError + .testGetValue("events") + ?.map(ev => ev.extra) ?? [], + [], + "Empty secureExperiments collection does not emit telemetry" + ); + + Services.fog.testResetFOG(); + + // If experiments is empty we will see a single event and observe that + // enrollments updated. + loader.remoteSettingsClients.experiments.get.resolves([]); + loader.remoteSettingsClients.secureExperiments.get.resolves([ + secureExperiment, + ]); + await updateAndAssertEnrollmentsUpdatedNotified( + loader, + true, + "empty collections don't prevent enrollment update" + ); + + Assert.ok( + !manager.store.get("experiment").active, + "experiment is not active" + ); + Assert.ok( + manager.store.get("secureExperiment")?.active, + "secureExperiment is active" + ); + + Assert.deepEqual( + Glean.nimbusEvents.remoteSettingsSyncError + .testGetValue("events") + ?.map(ev => ev.extra) ?? [], + [ + { + collection: "nimbus-desktop-experiments (stubbed)", + force_sync: "false", + trigger: "test", + reason: "empty", + }, + ], + "Empty experiments collection" + ); + + Services.fog.testResetFOG(); + + // If both collections contain recipes we will see no events. + loader.remoteSettingsClients.experiments.get.resolves([ + { ...experiment, slug: "experiment2" }, + ]); + await updateAndAssertEnrollmentsUpdatedNotified( + loader, + true, + "healthy collections" + ); + + Assert.deepEqual( + Glean.nimbusEvents.remoteSettingsSyncError + .testGetValue("events") + ?.map(ev => ev.extra) ?? [], + [], + "No events" + ); + Assert.ok( + !manager.store.get("experiment").active, + "experiment is not active (ended)" + ); + Assert.ok(manager.store.get("experiment2")?.active, "experiment2 is active"); + Assert.ok( + manager.store.get("secureExperiment")?.active, + "secureExperiment is active" + ); + + await NimbusTestUtils.cleanupManager(["experiment2", "secureExperiment"]); + await cleanup(); +}); + +add_task(async function test_remoteSettingsSyncError_getException() { + const { loader, cleanup } = await NimbusTestUtils.setupTest({ + clearTelemetry: true, + }); + Services.fog.testResetFOG(); // Clear the empty events that were triggered during initialization. + + loader.remoteSettingsClients.experiments.get.rejects(new Error("ruh roh")); + await updateAndAssertEnrollmentsUpdatedNotified(loader, false, "get threw"); + + Assert.deepEqual( + Glean.nimbusEvents.remoteSettingsSyncError + .testGetValue("events") + ?.map(ev => ev.extra) ?? [], + [ + { + collection: "nimbus-desktop-experiments (stubbed)", + force_sync: "false", + trigger: "test", + reason: "get-exception", + }, + ], + "get() threw" + ); + + await cleanup(); +}); + +add_task(async function test_remoteSettingsSyncError_invalidLastModified() { + const { loader, cleanup } = await NimbusTestUtils.setupTest({ + clearTelemetry: true, + }); + Services.fog.testResetFOG(); // Clear the empty events that were triggered during initialization. + + loader.remoteSettingsClients.experiments.db.getLastModified.resolves("never"); + await updateAndAssertEnrollmentsUpdatedNotified( + loader, + false, + "lastModified invalid" + ); + Assert.deepEqual( + Glean.nimbusEvents.remoteSettingsSyncError + .testGetValue("events") + ?.map(ev => ev.extra) ?? [], + [ + { + collection: "nimbus-desktop-experiments (stubbed)", + force_sync: "false", + trigger: "test", + reason: "empty", + }, + { + collection: "nimbus-desktop-experiments (stubbed)", + force_sync: "false", + trigger: "test", + reason: "invalid-last-modified", + }, + ], + "getLastModified should have thrown" + ); + + await cleanup(); +}); + +add_task(async function test_remoteSettingsSyncError_lastModifiedException() { + const { loader, cleanup } = await NimbusTestUtils.setupTest({ + clearTelemetry: true, + }); + Services.fog.testResetFOG(); // Clear the empty events that were triggered during initialization. + + loader.remoteSettingsClients.experiments.db.getLastModified.rejects( + new Error("ruh roh") + ); + + await updateAndAssertEnrollmentsUpdatedNotified( + loader, + false, + "lastModified threw" + ); + Assert.deepEqual( + Glean.nimbusEvents.remoteSettingsSyncError + .testGetValue("events") + ?.map(ev => ev.extra) ?? [], + [ + { + collection: "nimbus-desktop-experiments (stubbed)", + force_sync: "false", + trigger: "test", + reason: "last-modified-exception", + }, + ], + "getLastModified should have thrown" + ); + + await cleanup(); +}); + +add_task(async function test_remoteSettingsSyncError_nullLastModified() { + const experiment = NimbusTestUtils.factories.recipe("experiment"); + const secureExperiment = NimbusTestUtils.factories.recipe.withFeatureConfig( + "secureExperiment", + { + featureId: "prefFlips", + value: { + prefs: {}, + }, + } + ); + + const { manager, loader, cleanup } = await NimbusTestUtils.setupTest({ + clearTelemetry: true, + }); + Services.fog.testResetFOG(); // Clear the empty events that were triggered during initialization. + + // If the experiments collection contains an experiment but the + // secureExperiments collection does not and has null lastModified then + // enrollments will not update. + loader.remoteSettingsClients.experiments.get.resolves([experiment]); + loader.remoteSettingsClients.secureExperiments.db.getLastModified.resolves( + null + ); + await updateAndAssertEnrollmentsUpdatedNotified( + loader, + false, + "secureExperiments has null lastModified" + ); + + Assert.equal(manager.store.getAll().length, 0, "Enrollments did not update"); + Assert.deepEqual( + Glean.nimbusEvents.remoteSettingsSyncError + .testGetValue("events") + ?.map(ev => ev.extra) ?? [], + [ + { + collection: "nimbus-secure-experiments (stubbed)", + force_sync: "false", + trigger: "test", + reason: "null-last-modified", + }, + ], + "secureExperiments has null lastModified" + ); + Services.fog.testResetFOG(); + + // If the secureExperiments collection contains an experiment but the + // experiments collection does not and has null lastModified then enrollments + // will not update. + loader.remoteSettingsClients.experiments.get.resolves([]); + loader.remoteSettingsClients.experiments.db.getLastModified.resolves(null); + loader.remoteSettingsClients.secureExperiments.get.resolves([ + secureExperiment, + ]); + await updateAndAssertEnrollmentsUpdatedNotified( + loader, + false, + "experiments has null lastModified" + ); + + Assert.equal(manager.store.getAll().length, 0, "Enrollments did not update"); + Assert.deepEqual( + Glean.nimbusEvents.remoteSettingsSyncError + .testGetValue("events") + ?.map(ev => ev.extra) ?? [], + [ + { + collection: "nimbus-desktop-experiments (stubbed)", + force_sync: "false", + trigger: "test", + reason: "null-last-modified", + }, + ], + "Submitted failure telemetry" + ); + Services.fog.testResetFOG(); + + // If both collections are empty with null lastModified then enrollments will + // not update but we will only see a single null-last-modified event. + loader.remoteSettingsClients.secureExperiments.get.resolves([]); + loader.remoteSettingsClients.secureExperiments.db.getLastModified.resolves( + null + ); + await updateAndAssertEnrollmentsUpdatedNotified( + loader, + false, + "lastModified is not null for both collections" + ); + + Assert.equal(manager.store.getAll().length, 0, "Enrollments did not update"); + Assert.deepEqual( + Glean.nimbusEvents.remoteSettingsSyncError + .testGetValue("events") + ?.map(ev => ev.extra) ?? [], + [ + { + collection: "nimbus-desktop-experiments (stubbed)", + force_sync: "false", + trigger: "test", + reason: "null-last-modified", + }, + ], + "Submitted failure telemetry" + ); + Services.fog.testResetFOG(); + + // If neither collection has null lastModified we should see no + // null-last-modified events, but we will see empty events. + loader.remoteSettingsClients.experiments.db.getLastModified.resolves(0); + loader.remoteSettingsClients.secureExperiments.db.getLastModified.resolves(0); + await updateAndAssertEnrollmentsUpdatedNotified( + loader, + true, + "empty collections don't prevent enrollment update" + ); + + Assert.deepEqual( + Glean.nimbusEvents.remoteSettingsSyncError + .testGetValue("events") + ?.map(ev => ev.extra) ?? [], + [ + { + collection: "nimbus-desktop-experiments (stubbed)", + force_sync: "false", + trigger: "test", + reason: "empty", + }, + ], + "Submitted failure telemetry" + ); + + await cleanup(); +}); diff --git a/toolkit/profile/ProfilesDatastoreService.sys.mjs b/toolkit/profile/ProfilesDatastoreService.sys.mjs @@ -72,7 +72,7 @@ class ProfilesDatastoreServiceClass { async createTables() { // TODO: (Bug 1902320) Handle exceptions on connection opening let currentVersion = await this.#connection.getSchemaVersion(); - if (currentVersion == 6) { + if (currentVersion == 7) { return; } @@ -110,6 +110,7 @@ class ProfilesDatastoreServiceClass { if (currentVersion < 2) { await this.#connection.executeTransaction(async () => { + // NB: The profileId field is actually TEXT (a string UUID). const createEnrollmentsTable = ` CREATE TABLE IF NOT EXISTS "NimbusEnrollments" ( id INTEGER NOT NULL, @@ -188,6 +189,25 @@ class ProfilesDatastoreServiceClass { await this.#connection.setSchemaVersion(6); } + + if (currentVersion < 7) { + await this.#connection.executeTransaction(async () => { + const createNimbusSyncTable = ` + CREATE TABLE IF NOT EXISTS "NimbusSyncTimestamps" ( + id INTEGER NOT NULL, + profileId TEXT NOT NULL, + collection TEXT NOT NULL, + lastModified INTEGER NOT NULL, + PRIMARY KEY(id), + UNIQUE (profileId, collection) ON CONFLICT FAIL + ); + `; + + await this.#connection.execute(createNimbusSyncTable); + }); + + await this.#connection.setSchemaVersion(7); + } } /**