From bda42edf2c8abde00a1bad1cf955ad98f95050df Mon Sep 17 00:00:00 2001 From: Olivia Hall Date: Wed, 13 May 2026 17:15:10 -0400 Subject: [PATCH] Bug 2038991 - Update Cache when calling register_previous_gecko_pref_states Bug 2021137 removed end_initialize and set `writer.commit()` to prevent a loop. However, this change had a side-effect of a stale cache. This patch fixes it by: * Adding a param of `update_gecko_prefs` to `commit_and_update` * Having `register_previous_gecko_pref_states` call `commit_and_update` with `update_gecko_prefs=false` * Updating `map_gecko_prefs_to_enrollment_slugs_and_update_store` to take a param of `update_gecko_prefs`, when false, it will not set information with Gecko --- components/nimbus/src/stateful/dbcache.rs | 6 ++++- components/nimbus/src/stateful/gecko_prefs.rs | 25 +++++++++++-------- .../nimbus/src/stateful/nimbus_client.rs | 18 ++++++++++++- components/nimbus/src/tests/helpers.rs | 10 +++++--- .../src/tests/stateful/test_gecko_prefs.rs | 2 ++ .../nimbus/src/tests/stateful/test_nimbus.rs | 19 ++++++++++++++ 6 files changed, 65 insertions(+), 15 deletions(-) diff --git a/components/nimbus/src/stateful/dbcache.rs b/components/nimbus/src/stateful/dbcache.rs index f63b744cfa..1a02425d23 100644 --- a/components/nimbus/src/stateful/dbcache.rs +++ b/components/nimbus/src/stateful/dbcache.rs @@ -43,18 +43,21 @@ impl DatabaseCache { // // This function must be passed a `&Database` and a `Writer`, which it // will commit before updating the in-memory cache. This is a slightly weird - // API but it helps encorce two important properties: + // API but it helps enforce two important properties: // // * By requiring a `Writer`, we ensure mutual exclusion of other db writers // and thus prevent the possibility of caching stale data. // * By taking ownership of the `Writer`, we ensure that the calling code // updates the cache after all of its writes have been performed. + // * `update_gecko_prefs` - Pass true for regular enrollment changes. Pass false + // when the Gecko prefs do not need to be synced with Gecko. pub fn commit_and_update( &self, db: &Database, writer: Writer, coenrolling_ids: &HashSet<&str>, gecko_pref_store: Option>, + update_gecko_prefs: bool, ) -> Result<()> { // By passing in the active `writer` we read the state of enrollments // as written by the calling code, before it's committed to the db. @@ -80,6 +83,7 @@ impl DatabaseCache { &experiments, &enrollments, &experiments_by_slug, + update_gecko_prefs, ) }); diff --git a/components/nimbus/src/stateful/gecko_prefs.rs b/components/nimbus/src/stateful/gecko_prefs.rs index 5824823e12..42aaa77ae2 100644 --- a/components/nimbus/src/stateful/gecko_prefs.rs +++ b/components/nimbus/src/stateful/gecko_prefs.rs @@ -221,6 +221,8 @@ impl GeckoPrefStore { enrollments: &[ExperimentEnrollment], // contains slug of enrolled branch experiments_by_slug: &HashMap, + // when true, sets Gecko prefs using the handler. when false, only updates internal store state + update_gecko_prefs: bool, ) -> HashMap> { struct RecipeData<'a> { experiment: &'a Experiment, @@ -325,17 +327,20 @@ impl GeckoPrefStore { } } - // obtain a list of all Gecko pref states for which there is an enrollment value - let mut set_state_list = Vec::new(); - state.gecko_prefs_with_state.iter().for_each(|(_, props)| { - props.iter().for_each(|(_, pref_state)| { - if pref_state.enrollment_value.is_some() { - set_state_list.push(pref_state.clone()); - } + // setting with Gecko is used for true updates, sometimes the state just needs to be recalculated (e.g., registering the original value on the enrollment) + if update_gecko_prefs { + // obtain a list of all Gecko pref states for which there is an enrollment value + let mut set_state_list = Vec::new(); + state.gecko_prefs_with_state.iter().for_each(|(_, props)| { + props.iter().for_each(|(_, pref_state)| { + if pref_state.enrollment_value.is_some() { + set_state_list.push(pref_state.clone()); + } + }); }); - }); - // tell the handler to set the aforementioned Gecko prefs - self.handler.set_gecko_prefs_state(set_state_list); + // tell the handler to set the aforementioned Gecko prefs + self.handler.set_gecko_prefs_state(set_state_list); + } results } diff --git a/components/nimbus/src/stateful/nimbus_client.rs b/components/nimbus/src/stateful/nimbus_client.rs index f101b82ac5..fce9d4e297 100644 --- a/components/nimbus/src/stateful/nimbus_client.rs +++ b/components/nimbus/src/stateful/nimbus_client.rs @@ -227,6 +227,7 @@ impl NimbusClient { writer, &coenrolling_ids, self.gecko_prefs.clone(), + true, )?; Ok(()) } @@ -846,7 +847,22 @@ impl NimbusClient { )?; } - writer.commit()?; + let coenrolling_ids = self + .coenrolling_feature_ids + .iter() + .map(|s| s.as_str()) + .collect(); + + // Registering the Gecko original values does not require a Gecko update, + // but the cache does need to be refreshed. + self.database_cache.commit_and_update( + db, + writer, + &coenrolling_ids, + self.gecko_prefs.clone(), + false, + )?; + Ok(()) } diff --git a/components/nimbus/src/tests/helpers.rs b/components/nimbus/src/tests/helpers.rs index fee54de08e..89c9c21c07 100644 --- a/components/nimbus/src/tests/helpers.rs +++ b/components/nimbus/src/tests/helpers.rs @@ -177,6 +177,7 @@ impl TestMetrics { pub struct TestGeckoPrefHandlerState { pub prefs_set: Option>, pub original_prefs_state: Option>, + pub set_gecko_prefs_state_call_count: u32, } #[cfg(feature = "stateful")] @@ -193,6 +194,7 @@ impl TestGeckoPrefHandler { state: Mutex::new(TestGeckoPrefHandlerState { prefs_set: None, original_prefs_state: None, + set_gecko_prefs_state_call_count: 0, }), } } @@ -205,10 +207,12 @@ impl GeckoPrefHandler for TestGeckoPrefHandler { } fn set_gecko_prefs_state(&self, new_prefs_state: Vec) { - self.state + let mut state = self + .state .lock() - .expect("Unable to lock TestGeckoPrefHandler state") - .prefs_set = Some(new_prefs_state); + .expect("Unable to lock TestGeckoPrefHandler state"); + state.prefs_set = Some(new_prefs_state); + state.set_gecko_prefs_state_call_count += 1; } fn set_gecko_prefs_original_values(&self, original_prefs_state: Vec) { diff --git a/components/nimbus/src/tests/stateful/test_gecko_prefs.rs b/components/nimbus/src/tests/stateful/test_gecko_prefs.rs index 2a9d046383..d13ff59584 100644 --- a/components/nimbus/src/tests/stateful/test_gecko_prefs.rs +++ b/components/nimbus/src/tests/stateful/test_gecko_prefs.rs @@ -56,6 +56,7 @@ fn test_gecko_pref_store_map_gecko_prefs_to_enrollment_slugs_and_update_store() &experiments, &experiment_enrollments, &experiments_by_slug, + true, ); let handler = unsafe { @@ -147,6 +148,7 @@ fn test_gecko_pref_store_map_gecko_prefs_to_enrollment_slugs_and_update_store_ex &experiments, &experiment_enrollments, &experiments_by_slug, + true, ); let handler = unsafe { diff --git a/components/nimbus/src/tests/stateful/test_nimbus.rs b/components/nimbus/src/tests/stateful/test_nimbus.rs index 2132626f39..86f11f1601 100644 --- a/components/nimbus/src/tests/stateful/test_nimbus.rs +++ b/components/nimbus/src/tests/stateful/test_nimbus.rs @@ -2174,9 +2174,28 @@ fn register_previous_gecko_pref_states() -> Result<()> { gecko_pref_state_2.clone(), gecko_pref_state_3.clone(), ]; + + let call_count_before = client + .get_gecko_pref_store() + .state + .lock() + .unwrap() + .set_gecko_prefs_state_call_count; + let registration = client.register_previous_gecko_pref_states(&gecko_pref_states); assert!(registration.is_ok()); + // Registration must not send pref values to Gecko. + assert_eq!( + call_count_before, + client + .get_gecko_pref_store() + .state + .lock() + .unwrap() + .set_gecko_prefs_state_call_count + ); + let db = client.db()?; let reader = db.read()?; let mut enrollments: Vec =