commit 13f93f1e5837c306fb7fe568a03332f5b5f0cb04 parent 4d481912a543658833e1a779de45fda3d2f9f226 Author: iulian moraru <imoraru@mozilla.com> Date: Mon, 6 Oct 2025 22:47:57 +0300 Revert "Bug 1985079 - Keep track of Remote Settings collection lastModified timestamps r=nimbus-reviewers,profiles-reviewers,jhirsch,relud,nalexander" for causing xpcshell failures on test_RemoteSettingsExperimentLoader_updateRecipes.js. This reverts commit fadfd89de8477f3e298496c1b1d91d8825d93a8f. Revert "Bug 1985079 - Throw in RemoteSettingsExperimentLoader when fetching records r=nimbus-reviewers,relud" This reverts commit 4c2c7eb7c08e925fac654dc484fa99980047ac90. Revert "Bug 1985079 - Move NimbusEnrollments ownership to SharedDataMap r=nimbus-reviewers,relud" This reverts commit 1d2aca103c890e0a33f863a1bb7ec5d431c4364d. Diffstat:
13 files changed, 271 insertions(+), 1369 deletions(-)
diff --git a/browser/components/profiles/SelectableProfileService.sys.mjs b/browser/components/profiles/SelectableProfileService.sys.mjs @@ -1369,13 +1369,6 @@ 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,7 +61,6 @@ 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", @@ -83,44 +82,10 @@ outputInfo = (sentinel, info) => { dump(`${sentinel}${JSON.stringify(info)}${sentinel}\n`); }; -/** - * 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; +function monkeyPatchRemoteSettingsClient({ data = [] }) { + lazy.RemoteSettingsClient.prototype.get = async (options = {}) => { + outputInfo({ "RemoteSettingsClient.get": { options, response: { data } } }); + return data; }; } @@ -253,7 +218,7 @@ async function handleCommandLine(commandLine) { data = [data]; } - monkeyPatchRemoteSettingsClients({ experiments: data }); + monkeyPatchRemoteSettingsClient({ data }); console.log(`Saw --experiments, read recipes from ${experimentsPath}`); } @@ -263,7 +228,7 @@ async function handleCommandLine(commandLine) { !experiments && commandLine.handleFlag("no-experiments", CASE_INSENSITIVE) ) { - monkeyPatchRemoteSettingsClients({ experiments: [] }); + monkeyPatchRemoteSettingsClient({ data: [] }); 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,11 +11,8 @@ 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", () => { @@ -52,101 +49,12 @@ 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. */ export class NimbusEnrollments { /** - * Whether the NimbusEvents instance is initialized or not. - * - * @type {boolean} - */ - #initialized; - - /** * The ExperimentStore. * * @type {ExperimentStore} @@ -178,22 +86,16 @@ export class NimbusEnrollments { /** * Pending writes that will be flushed in `#flush()`. * - * @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. + * 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, number> | null} + * @type {Map<string, object | null>} */ - #syncTimestamps; + #pending; constructor(store) { - this.#initialized = false; this.#store = store; this.#flushTask = new lazy.DeferredTask( @@ -208,34 +110,14 @@ export class NimbusEnrollments { ); this.#finalized = false; - this.#pendingWrites = new PendingWrites(); - this.#syncTimestamps = null; - } - - async init() { - if (this.#initialized) { - throw new Error("Already initialized"); - } - - this.#initialized = true; - - const conn = await lazy.ProfilesDatastoreService.getConnection(); - return conn.executeTransaction(async 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; - }); + this.#pending = new Map(); } /** * The number of pending writes. */ get pendingWrites() { - return this.#pendingWrites.enrollments.size; + return this.#pending.size; } /** @@ -255,57 +137,14 @@ export class NimbusEnrollments { lazy.log.debug(`Queued update for enrollment ${slug}`); - 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; + // 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.#flushSoon(); - - lazy.log.debug("Timestamps updated"); } } @@ -339,8 +178,7 @@ export class NimbusEnrollments { } /** - * Flush all pending updates to the NimbusEnrollments and NimbusSyncTimestamps - * tables. + * Flush all pending updates to the NimbusEnrollments table. * * The updates are done as a single transaction to ensure the database stays * in a consistent state. @@ -355,21 +193,15 @@ export class NimbusEnrollments { * we've started shutting down. */ async #flush({ retryOnFailure = true } = {}) { - if (!this.#pendingWrites.hasPendingWrites) { + if (!this.#pending.size) { lazy.log.debug(`Not flushing: no changes`); return; } - // 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(); + const pending = this.#pending; + this.#pending = new Map(); - lazy.log.debug( - `Flushing ${pendingWrites.enrollments.size} enrollments, ${pendingWrites.syncTimestamps.size} timestamps to database` - ); + lazy.log.debug(`Flushing ${pending.size} changes to database`); let success = true; try { @@ -387,35 +219,35 @@ export class NimbusEnrollments { return; } - await conn.executeTransaction(async txn => { - for (const [slug, recipe] of pendingWrites.enrollments.entries()) { + await conn.executeTransaction(async () => { + for (const [slug, recipe] of pending.entries()) { const enrollment = this.#store.get(slug); if (enrollment) { - await NimbusEnrollments.#insertOrUpdateEnrollment( - txn, - enrollment, - recipe - ); + await this.#insertOrUpdateEnrollment(conn, enrollment, recipe); } else { - await NimbusEnrollments.#deleteEnrollment(txn, slug); + await this.#deleteEnrollment(conn, slug); } } - - await NimbusEnrollments.#flushSyncTimestamps( - txn, - pendingWrites.syncTimestamps - ); }); } catch (e) { success = false; if (retryOnFailure) { - // 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 - ); + // 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; + } if (!this.#finalized) { lazy.log.error( @@ -455,7 +287,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. */ - static async #insertOrUpdateEnrollment(conn, enrollment, recipe) { + async #insertOrUpdateEnrollment(conn, enrollment, recipe) { if (recipe) { // This was a new enrollment at the time. It may have since unenrolled. await conn.executeCached( @@ -548,7 +380,7 @@ export class NimbusEnrollments { * @param {OpenedConnection} conn The connection to the database. * @param {string} slug The slug of the enrollment to delete. */ - static async #deleteEnrollment(conn, slug) { + async #deleteEnrollment(conn, slug) { await conn.execute( ` DELETE FROM NimbusEnrollments @@ -587,33 +419,6 @@ 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. * @@ -644,14 +449,11 @@ export class NimbusEnrollments { /** * Load the enrollments from the NimbusEnrollments table. - - * @param {OpenedConnection} txn An optional connection, used when this is - * called within a transaction. * * @returns {Promise<Record<string, object>>} The enrollments from the * NimbusEnrollments table. */ - static async loadEnrollments(txn) { + static async loadEnrollments() { function copyProperties(target, src, properties) { for (const property of properties) { target[property] = src[property]; @@ -709,7 +511,7 @@ export class NimbusEnrollments { return [enrollment.slug, enrollment]; } - const conn = txn ?? (await lazy.ProfilesDatastoreService.getConnection()); + const conn = await lazy.ProfilesDatastoreService.getConnection(); const rows = await conn.execute( ` SELECT @@ -739,40 +541,6 @@ 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/ExperimentStore.sys.mjs b/toolkit/components/nimbus/lib/ExperimentStore.sys.mjs @@ -214,12 +214,28 @@ ChromeUtils.defineLazyGetter(lazy, "syncDataStore", () => { const DEFAULT_STORE_ID = "ExperimentStoreData"; +const IS_MAIN_PROCESS = + Services.appinfo.processType === Services.appinfo.PROCESS_TYPE_DEFAULT; + export class ExperimentStore extends SharedDataMap { static SYNC_DATA_PREF_BRANCH = SYNC_DATA_PREF_BRANCH; static SYNC_DEFAULTS_PREF_BRANCH = SYNC_DEFAULTS_PREF_BRANCH; constructor(sharedDataKey, options) { super(sharedDataKey ?? DEFAULT_STORE_ID, options); + + this._db = null; + + if (IS_MAIN_PROCESS) { + 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. + this._db = new lazy.NimbusEnrollments(this); + } + } } /** diff --git a/toolkit/components/nimbus/lib/Migrations.sys.mjs b/toolkit/components/nimbus/lib/Migrations.sys.mjs @@ -12,8 +12,6 @@ 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", () => { @@ -131,18 +129,10 @@ async function migrateEnrollmentsToSql() { return; } - let recipes; - try { - recipes = await lazy.ExperimentAPI._rsLoader.getRecipesFromAllCollections({ + const { 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])); @@ -330,7 +320,6 @@ 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,8 +2,6 @@ * 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 = {}; @@ -173,13 +171,6 @@ 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 @@ -409,18 +400,10 @@ export class RemoteSettingsExperimentLoader { await SCHEMAS.NimbusExperiment ); } + const { recipes: allRecipes, loadingError } = + await this.getRecipesFromAllCollections({ forceSync, trigger }); - let allRecipes = null; - try { - allRecipes = await this.getRecipesFromAllCollections({ - forceSync, - trigger, - }); - } catch (e) { - lazy.log.debug("Failed to update", e); - } - - if (allRecipes !== null) { + if (!loadingError) { const unenrolledExperimentSlugs = lazy.NimbusEnrollments .syncEnrollmentsEnabled ? await lazy.NimbusEnrollments.loadUnenrolledExperimentSlugsFromOtherProfiles() @@ -465,10 +448,7 @@ export class RemoteSettingsExperimentLoader { Services.prefs.setIntPref(TIMER_LAST_UPDATE_PREF, lastUpdateTime); } - if (allRecipes !== null) { - // Enrollments have not changed, so we don't need to notify. - Services.obs.notifyObservers(null, "nimbus:enrollments-updated"); - } + Services.obs.notifyObservers(null, "nimbus:enrollments-updated"); } /** @@ -485,79 +465,45 @@ export class RemoteSettingsExperimentLoader { * @param {string} options.trigger The name of the event that triggered the * update. * - * @returns {Promise<object[]>} The recipes from Remote Settings. - * - * @throws {RemoteSettingsSyncError} - * @throws {BackwardsSyncError} - * @throws {InvalidLastModifiedError} + * @returns {Promise<{ recipes: object[]; loadingError: boolean; }>} The + * recipes from Remote Settings. */ async getRecipesFromAllCollections({ forceSync = false, trigger } = {}) { - try { - 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, - }); + const recipes = []; + let loadingError = false; - // 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); + const experiments = await this.getRecipesFromCollection({ + forceSync, + client: this.remoteSettingsClients[EXPERIMENTS_COLLECTION], + ...RS_COLLECTION_OPTIONS[EXPERIMENTS_COLLECTION], + }); - recipes.push(...collection.recipes); - } + if (experiments !== null) { + recipes.push(...experiments); + } else { + loadingError = true; + } - 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); - } + const secureExperiments = await this.getRecipesFromCollection({ + forceSync, + client: this.remoteSettingsClients[SECURE_EXPERIMENTS_COLLECTION], + ...RS_COLLECTION_OPTIONS[SECURE_EXPERIMENTS_COLLECTION], + }); - return recipes; - } catch (e) { - lazy.log.error("Failed to retrieve recipes from Remote Settings", e); + if (secureExperiments !== null) { + recipes.push(...secureExperiments); + } else { + loadingError = true; + } - if (e instanceof RemoteSettingsSyncError) { - lazy.NimbusTelemetry.recordRemoteSettingsSyncError( - e.collectionName, - e.reason, - { forceSync, trigger } - ); - } + lazy.NimbusTelemetry.recordRemoteSettingsSync( + forceSync, + experiments, + secureExperiments, + trigger + ); - throw e; - } + return { recipes, loadingError }; } /** @@ -574,12 +520,10 @@ export class RemoteSettingsExperimentLoader { * @param {Set<string> | undefined} options.disallowedFeatureIds * If a recipe uses any features in this list, it will be rejected. * - * @returns {Promise<RecipeCollection>} The recipes and last modified - * timestamp from the collection, filtered based on `requiredFeatureIds` and - * `disallowedFeatureIds`. - * - * @throws {RemoteSettingsSyncError} If we fail to get the recipes from the - * Remote Settings client. + * @returns {object[] | null} + * Recipes from the collection, filtered to match the allowed and + * disallowed feature IDs, or null if there was an error syncing the + * collection. */ async getRecipesFromCollection({ client, @@ -593,44 +537,29 @@ export class RemoteSettingsExperimentLoader { 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 RemoteSettingsSyncError( - client.collectionName, - lazy.NimbusTelemetry.RemoteSettingsSyncErrorReason.INVALID_DATA + 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}` ); - } - - let lastModified; - try { - lastModified = await client.db.getLastModified(); } catch (e) { - throw new RemoteSettingsSyncError( - client.collectionName, - lazy.NimbusTelemetry.RemoteSettingsSyncErrorReason.LAST_MODIFIED_EXCEPTION, - { cause: e } + lazy.log.debug( + `Error getting recipes from Remote Settings collection ${client.collectionName}: ${e}` ); - } - if (recipes.length === 0 && lastModified === null) { - throw new RemoteSettingsSyncError( - client.collectionName, - lazy.NimbusTelemetry.RemoteSettingsSyncErrorReason.NULL_LAST_MODIFIED - ); + return null; } - lazy.log.debug( - `Got ${recipes.length} recipes from ${client.collectionName}` - ); - - const filteredRecipes = recipes.filter(recipe => { + return recipes.filter(recipe => { if ( requiredFeatureIds && !recipe.featureIds.some(featureId => requiredFeatureIds.has(featureId)) @@ -654,12 +583,6 @@ export class RemoteSettingsExperimentLoader { return true; }); - - return { - collectionName: client.collectionName, - recipes: filteredRecipes, - lastModified, - }; } async _optInToExperiment({ @@ -1263,44 +1186,3 @@ 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 @@ -22,7 +22,6 @@ export class SharedDataMap extends EventEmitter { this._isReady = false; this._readyDeferred = Promise.withResolvers(); this._data = null; - this._db = null; if (IS_MAIN_PROCESS) { this._shutdownBlocker = () => { @@ -49,15 +48,6 @@ export class SharedDataMap extends EventEmitter { } return null; }); - - 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. - this._db = new lazy.NimbusEnrollments(this); - } } else { this._syncFromParent(); Services.cpmm.sharedData.addEventListener("change", this); @@ -73,20 +63,9 @@ export class SharedDataMap extends EventEmitter { await this._jsonFile.load(); if (lazy.NimbusEnrollments.readFromDatabaseEnabled) { - this._data = await this._db.init(); + this._data = await lazy.NimbusEnrollments.loadEnrollments(); } 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,23 +44,6 @@ 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", @@ -105,7 +88,6 @@ export const NimbusTelemetry = { EnrollmentSource, EnrollmentStatus, EnrollmentStatusReason, - RemoteSettingsSyncErrorReason, UnenrollReason, UnenrollmentFailureReason, ValidationFailureReason, @@ -194,31 +176,31 @@ export const NimbusTelemetry = { ); }, - /** - * 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 } = {} - ) { + 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; + } + const event = { force_sync: forceSync, - collection, - reason, + experiments_success: experiments !== null, + secure_experiments_success: secureExperiments !== null, }; - - if (typeof trigger === "string") { + if (experiments !== null) { + event.experiments_empty = experiments.length === 0; + } + if (secureExperiments !== null) { + event.secure_experiments_empty = secureExperiments.length === 0; + } + if (trigger != null) { event.trigger = trigger; } - - Glean.nimbusEvents.remoteSettingsSyncError.record(event); + Glean.nimbusEvents.remoteSettingsSync.record(event); }, recordUnenrollment(enrollment, cause) { diff --git a/toolkit/components/nimbus/metrics.yaml b/toolkit/components/nimbus/metrics.yaml @@ -1034,8 +1034,6 @@ nimbus_events: Reasons include: - "unknown": an unknown exception occurred. - - one of the reasons documented in - `nimbus_events.remote_settings_sync_error`. enrollments: type: string @@ -1043,10 +1041,8 @@ 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: @@ -1131,51 +1127,48 @@ nimbus_events: - technical expires: never - remote_settings_sync_error: + remote_settings_sync: type: event description: > - Triggered when Nimbus fails to get recipes from remote settings collections. - + Triggered whenever Nimbus attempts to get recipes from remote settings + collections, whether or not it succeeds. extra_keys: - collection: - type: string - description: The name of the collection. - force_sync: type: boolean - description: Whether or not the sync was forced. + description: > + Whether or not a sync was forced when fetching recipes. - trigger: - type: string - description: What triggered the sync. + experiments_success: + type: boolean + description: > + Whether or not loading the experiments collection succeeded. - reason: - type: string + secure_experiments_success: + type: boolean description: > - 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). + Whether or not loading the secure experiments collection succeeded. + experiments_empty: + type: boolean + description: > + Whether or not the experiments collection was empty. + + secure_experiments_empty: + type: boolean + description: > + Whether or not the secure experiments collection was empty. + + trigger: + type: string + description: > + The name of the event that triggered the sync. bugs: - - https://bugzilla.mozilla.org/show_bug.cgi?id=1985079 + - https://bugzilla.mozilla.org/show_bug.cgi?id=1968792 data_reviews: - - https://bugzilla.mozilla.org/show_bug.cgi?id=1985079 - + - https://bugzilla.mozilla.org/show_bug.cgi?id=1968792 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,15 +722,6 @@ export const NimbusTestUtils = { `, { profileId } ); - - await conn.execute( - ` - DELETE FROM NimbusSyncTimestamps - WHERE - profileId = :profileId; - `, - { profileId } - ); }, /** @@ -1179,17 +1170,8 @@ 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.js b/toolkit/components/nimbus/test/unit/test_RemoteSettingsExperimentLoader.js @@ -124,6 +124,86 @@ 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(); diff --git a/toolkit/components/nimbus/test/unit/test_RemoteSettingsExperimentLoader_updateRecipes.js b/toolkit/components/nimbus/test/unit/test_RemoteSettingsExperimentLoader_updateRecipes.js @@ -1,8 +1,5 @@ "use strict"; -const { BrowserUtils } = ChromeUtils.importESModule( - "resource://gre/modules/BrowserUtils.sys.mjs" -); const { EnrollmentsContext, MatchStatus } = ChromeUtils.importESModule( "resource://nimbus/lib/RemoteSettingsExperimentLoader.sys.mjs" ); @@ -15,6 +12,9 @@ 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,35 +42,6 @@ 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(); }); @@ -2328,6 +2299,9 @@ 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", @@ -2335,17 +2309,24 @@ async function testRsClientGetThrows(collectionName) { "rs-loader" ); - await updateAndAssertEnrollmentsUpdatedNotified( - loader, - false, - "collection threw" - ); + await loader.updateRecipes(); + await updatePromise; 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" @@ -2671,691 +2652,3 @@ 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: "migration", - reason: "empty", - }, - { - 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 == 7) { + if (currentVersion == 6) { return; } @@ -110,7 +110,6 @@ 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, @@ -189,25 +188,6 @@ 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); - } } /**