From 4da45265aad9d114a3b7a5bc1c855d6996417d96 Mon Sep 17 00:00:00 2001 From: Mohamed Ibrahim Date: Tue, 12 May 2026 15:43:11 -0400 Subject: [PATCH] fix(nimbus): populate config_slug on mobile --- components/nimbus/src/enrollment.rs | 114 ++++++++++++++++- components/nimbus/src/metrics.rs | 9 +- components/nimbus/src/stateful/persistence.rs | 29 ++++- .../src/tests/stateful/test_persistence.rs | 119 +++++++++++++----- .../nimbus/src/tests/test_enrollment.rs | 2 +- .../src/tests/test_enrollment_bw_compat.rs | 56 ++++++++- 6 files changed, 285 insertions(+), 44 deletions(-) diff --git a/components/nimbus/src/enrollment.rs b/components/nimbus/src/enrollment.rs index 5e73f36906..d591fa9591 100644 --- a/components/nimbus/src/enrollment.rs +++ b/components/nimbus/src/enrollment.rs @@ -57,7 +57,7 @@ pub enum NotEnrolledReason { /// The experiment enrollment is paused. EnrollmentsPaused, /// The experiment used a feature that was already under experiment. - FeatureConflict, + FeatureConflict { conflict_slug: Option }, /// The evaluator bucketing did not choose us. NotSelected, /// We are not being targeted for this experiment. @@ -66,6 +66,108 @@ pub enum NotEnrolledReason { OptOut, } +#[cfg(feature = "stateful")] +pub mod v3 { + use super::*; + use serde::{Deserialize, Serialize}; + + #[derive(Deserialize, Serialize, Debug, Clone, Hash, Eq, PartialEq)] + pub enum LegacyNotEnrolledReason { + DifferentAppName, + DifferentChannel, + EnrollmentsPaused, + FeatureConflict, + NotSelected, + NotTargeted, + OptOut, + } + + #[derive(Deserialize, Serialize, Debug, Clone, PartialEq, Eq)] + pub struct LegacyExperimentEnrollment { + pub slug: String, + pub status: LegacyEnrollmentStatus, + } + + #[derive(Deserialize, Serialize, Debug, Clone, Hash, Eq, PartialEq)] + pub enum LegacyEnrollmentStatus { + Enrolled { + reason: EnrolledReason, + branch: String, + #[serde(skip_serializing_if = "Option::is_none")] + prev_gecko_pref_states: Option>, + }, + NotEnrolled { + reason: LegacyNotEnrolledReason, + }, + Disqualified { + reason: DisqualifiedReason, + branch: String, + }, + WasEnrolled { + branch: String, + experiment_ended_at: u64, + }, + Error { + reason: String, + }, + } + + impl From for NotEnrolledReason { + fn from(value: LegacyNotEnrolledReason) -> Self { + match value { + LegacyNotEnrolledReason::DifferentAppName => NotEnrolledReason::DifferentAppName, + LegacyNotEnrolledReason::DifferentChannel => NotEnrolledReason::DifferentChannel, + LegacyNotEnrolledReason::EnrollmentsPaused => NotEnrolledReason::EnrollmentsPaused, + LegacyNotEnrolledReason::FeatureConflict => NotEnrolledReason::FeatureConflict { + conflict_slug: None, + }, + LegacyNotEnrolledReason::NotSelected => NotEnrolledReason::NotSelected, + LegacyNotEnrolledReason::NotTargeted => NotEnrolledReason::NotTargeted, + LegacyNotEnrolledReason::OptOut => NotEnrolledReason::OptOut, + } + } + } + + impl From for EnrollmentStatus { + fn from(value: LegacyEnrollmentStatus) -> Self { + match value { + LegacyEnrollmentStatus::Enrolled { + reason, + branch, + prev_gecko_pref_states, + } => EnrollmentStatus::Enrolled { + reason, + branch, + prev_gecko_pref_states, + }, + LegacyEnrollmentStatus::NotEnrolled { reason } => EnrollmentStatus::NotEnrolled { + reason: reason.into(), + }, + LegacyEnrollmentStatus::Disqualified { reason, branch } => { + EnrollmentStatus::Disqualified { reason, branch } + } + LegacyEnrollmentStatus::WasEnrolled { + branch, + experiment_ended_at, + } => EnrollmentStatus::WasEnrolled { + branch, + experiment_ended_at, + }, + LegacyEnrollmentStatus::Error { reason } => EnrollmentStatus::Error { reason }, + } + } + } + + impl From for ExperimentEnrollment { + fn from(value: LegacyExperimentEnrollment) -> Self { + ExperimentEnrollment { + slug: value.slug, + status: value.status.into(), + } + } + } +} + impl Display for NotEnrolledReason { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { Display::fmt( @@ -73,7 +175,7 @@ impl Display for NotEnrolledReason { NotEnrolledReason::DifferentAppName => "DifferentAppName", NotEnrolledReason::DifferentChannel => "DifferentChannel", NotEnrolledReason::EnrollmentsPaused => "EnrollmentsPaused", - NotEnrolledReason::FeatureConflict => "FeatureConflict", + NotEnrolledReason::FeatureConflict { .. } => "FeatureConflict", NotEnrolledReason::NotSelected => "NotSelected", NotEnrolledReason::NotTargeted => "NotTargeted", NotEnrolledReason::OptOut => "OptOut", @@ -905,7 +1007,7 @@ impl<'a> EnrollmentsEvolver<'a> { if matches!( prev_enrollment.status, EnrollmentStatus::NotEnrolled { - reason: NotEnrolledReason::FeatureConflict + reason: NotEnrolledReason::FeatureConflict { .. }, } ) { continue; @@ -985,7 +1087,9 @@ impl<'a> EnrollmentsEvolver<'a> { next_enrollments.push(ExperimentEnrollment { slug: slug.clone(), status: EnrollmentStatus::NotEnrolled { - reason: NotEnrolledReason::FeatureConflict, + reason: NotEnrolledReason::FeatureConflict { + conflict_slug: Some(needed_features_in_use[0].slug.clone()), + }, }, }); @@ -1014,7 +1118,7 @@ impl<'a> EnrollmentsEvolver<'a> { || matches!( prev_enrollment.unwrap().status, EnrollmentStatus::NotEnrolled { - reason: NotEnrolledReason::FeatureConflict + reason: NotEnrolledReason::FeatureConflict { .. } } ) { diff --git a/components/nimbus/src/metrics.rs b/components/nimbus/src/metrics.rs index 89d5521ce6..48794197d1 100644 --- a/components/nimbus/src/metrics.rs +++ b/components/nimbus/src/metrics.rs @@ -30,6 +30,7 @@ impl From for EnrollmentStatusExtraDef { let mut branch_value: Option = None; let mut reason_value: Option = None; let mut error_value: Option = None; + let mut conflict_slug_value: Option = None; match &enrollment.status { EnrollmentStatus::Enrolled { reason, branch, .. } => { branch_value = Some(branch.to_owned()); @@ -41,6 +42,12 @@ impl From for EnrollmentStatusExtraDef { } EnrollmentStatus::NotEnrolled { reason } => { reason_value = Some(reason.to_string()); + conflict_slug_value = match reason { + crate::enrollment::NotEnrolledReason::FeatureConflict { conflict_slug } => { + conflict_slug.to_owned() + } + _ => None, + }; } EnrollmentStatus::WasEnrolled { branch, .. } => branch_value = Some(branch.to_owned()), EnrollmentStatus::Error { reason } => { @@ -49,7 +56,7 @@ impl From for EnrollmentStatusExtraDef { } EnrollmentStatusExtraDef { branch: branch_value, - conflict_slug: None, + conflict_slug: conflict_slug_value, error_string: error_value, reason: reason_value, slug: Some(enrollment.slug), diff --git a/components/nimbus/src/stateful/persistence.rs b/components/nimbus/src/stateful/persistence.rs index 0d00d7a855..33e008083f 100644 --- a/components/nimbus/src/stateful/persistence.rs +++ b/components/nimbus/src/stateful/persistence.rs @@ -10,6 +10,8 @@ use std::fs; use std::path::Path; use std::sync::Arc; +use crate::enrollment::ExperimentEnrollment; +use crate::enrollment::v3; use crate::error::{ErrorCode, NimbusError, Result, debug, info, warn}; use crate::metrics::{DatabaseLoadExtraDef, DatabaseMigrationExtraDef, MetricsHandler}; @@ -27,7 +29,7 @@ use crate::metrics::{DatabaseLoadExtraDef, DatabaseMigrationExtraDef, MetricsHan pub(crate) const DB_KEY_DB_VERSION: &str = "db_version"; /// The current database version. -pub(crate) const DB_VERSION: u16 = 3; +pub(crate) const DB_VERSION: u16 = 4; pub(crate) const DB_KEY_DB_WAS_CORRUPT: &str = "db-was-corrupt"; @@ -554,6 +556,14 @@ impl Database { DatabaseMigrationReason::Upgrade, )?; + self.apply_migration( + writer, + |writer| self.migrate_v3_to_v4(writer), + &mut current_version, + 4, + DatabaseMigrationReason::Upgrade, + )?; + Ok(()) } @@ -677,6 +687,23 @@ impl Database { Ok(()) } + fn migrate_v3_to_v4(&self, writer: &mut Writer) -> Result<()> { + info!("Upgrading from version 3 to version 4"); + + let enrollment_store = self.get_store(StoreId::Enrollments); + let enrollments: Vec = + enrollment_store.try_collect_all(writer)?; + + let enrollments: Vec = + enrollments.into_iter().map(Into::into).collect(); + + for enrollment in enrollments { + enrollment_store.put(writer, &enrollment.slug, &enrollment)?; + } + + Ok(()) + } + /// Gets a Store object, which used with the writer returned by /// `self.write()` to update the database in a transaction. pub fn get_store(&self, store_id: StoreId) -> &SingleStore { diff --git a/components/nimbus/src/tests/stateful/test_persistence.rs b/components/nimbus/src/tests/stateful/test_persistence.rs index 8ea26429d5..498657caed 100644 --- a/components/nimbus/src/tests/stateful/test_persistence.rs +++ b/components/nimbus/src/tests/stateful/test_persistence.rs @@ -82,6 +82,12 @@ fn test_db_upgrade_unknown_version() -> Result<()> { to_version: 3, error: None, }, + DatabaseMigrationExtraDef { + reason: DatabaseMigrationReason::Upgrade.to_string(), + from_version: 3, + to_version: 4, + error: None, + }, ] ); @@ -150,6 +156,12 @@ fn test_corrupt_db() -> Result<()> { from_version: 2, to_version: 3, error: None, + }, + DatabaseMigrationExtraDef { + reason: DatabaseMigrationReason::Upgrade.to_string(), + from_version: 3, + to_version: 4, + error: None, } ] ); @@ -248,6 +260,12 @@ fn test_corrupt_db_get_calculated_attributes() -> Result<()> { from_version: 2, to_version: 3, error: None, + }, + DatabaseMigrationExtraDef { + reason: DatabaseMigrationReason::Upgrade.to_string(), + from_version: 3, + to_version: 4, + error: None, } ] ); @@ -289,7 +307,7 @@ fn test_corrupt_db_get_calculated_attributes() -> Result<()> { } #[test] -fn test_migrate_db_v2_to_v3_user_opted_out() -> Result<()> { +fn test_migrate_db_v2_to_v4_user_opted_out() -> Result<()> { error_support::init_for_tests(); let tmp_dir = tempfile::tempdir()?; @@ -300,8 +318,8 @@ fn test_migrate_db_v2_to_v3_user_opted_out() -> Result<()> { let metrics = TestMetrics::new(); let db = Database::new(&tmp_dir, metrics.clone())?; - // Check the database was upgraded to v3 - assert_eq!(db.get(StoreId::Meta, DB_KEY_DB_VERSION)?, Some(3u16)); + // Check the database was upgraded to v4 + assert_eq!(db.get(StoreId::Meta, DB_KEY_DB_VERSION)?, Some(4u16)); // Check that separate flags were set correctly for opted-out user let reader = db.read()?; @@ -324,26 +342,34 @@ fn test_migrate_db_v2_to_v3_user_opted_out() -> Result<()> { corrupt: Some(false), error: None, initial_version: Some(2), - migrated_version: Some(3), + migrated_version: Some(4), migration_error: None, }], ); assert_eq!( metrics.get_database_migration_events(), - [DatabaseMigrationExtraDef { - reason: DatabaseMigrationReason::Upgrade.to_string(), - from_version: 2, - to_version: 3, - error: None, - }], + [ + DatabaseMigrationExtraDef { + reason: DatabaseMigrationReason::Upgrade.to_string(), + from_version: 2, + to_version: 3, + error: None, + }, + DatabaseMigrationExtraDef { + reason: DatabaseMigrationReason::Upgrade.to_string(), + from_version: 3, + to_version: 4, + error: None, + } + ], ); Ok(()) } #[test] -fn test_migrate_db_v2_to_v3_user_opted_in() -> Result<()> { +fn test_migrate_db_v2_to_v4_user_opted_in() -> Result<()> { error_support::init_for_tests(); let tmp_dir = tempfile::tempdir()?; @@ -353,8 +379,8 @@ fn test_migrate_db_v2_to_v3_user_opted_in() -> Result<()> { let metrics = TestMetrics::new(); let db = Database::new(&tmp_dir, metrics.clone())?; - // Check the database was upgraded to v3 - assert_eq!(db.get(StoreId::Meta, DB_KEY_DB_VERSION)?, Some(3u16)); + // Check the database was upgraded to v4 + assert_eq!(db.get(StoreId::Meta, DB_KEY_DB_VERSION)?, Some(4u16)); // Check that separate flags were set correctly for opted-in user let reader = db.read()?; @@ -377,19 +403,27 @@ fn test_migrate_db_v2_to_v3_user_opted_in() -> Result<()> { corrupt: Some(false), error: None, initial_version: Some(2), - migrated_version: Some(3), + migrated_version: Some(4), migration_error: None, }], ); assert_eq!( metrics.get_database_migration_events(), - [DatabaseMigrationExtraDef { - reason: DatabaseMigrationReason::Upgrade.to_string(), - from_version: 2, - to_version: 3, - error: None, - }], + [ + DatabaseMigrationExtraDef { + reason: DatabaseMigrationReason::Upgrade.to_string(), + from_version: 2, + to_version: 3, + error: None, + }, + DatabaseMigrationExtraDef { + reason: DatabaseMigrationReason::Upgrade.to_string(), + from_version: 3, + to_version: 4, + error: None, + } + ], ); Ok(()) @@ -405,7 +439,7 @@ fn test_migrate_empty() -> Result<()> { let db = Database::new(&tmp_dir, metrics.clone())?; let meta = db.get_store(StoreId::Meta); let reader = db.read()?; - assert_eq!(meta.get::(&reader, DB_KEY_DB_VERSION)?, Some(3)); + assert_eq!(meta.get::(&reader, DB_KEY_DB_VERSION)?, Some(4)); assert_eq!( meta.get::(&reader, DB_KEY_GLOBAL_USER_PARTICIPATION)?, None @@ -425,7 +459,7 @@ fn test_migrate_empty() -> Result<()> { corrupt: Some(false), error: None, initial_version: Some(0), - migrated_version: Some(3), + migrated_version: Some(4), migration_error: None, }], ); @@ -445,6 +479,12 @@ fn test_migrate_empty() -> Result<()> { to_version: 3, error: None, }, + DatabaseMigrationExtraDef { + reason: DatabaseMigrationReason::Upgrade.to_string(), + from_version: 3, + to_version: 4, + error: None, + }, ], ); @@ -452,7 +492,7 @@ fn test_migrate_empty() -> Result<()> { } #[test] -fn test_migrate_db_v1_to_v3_cumulative_participation_enabled() -> Result<()> { +fn test_migrate_db_v1_to_v4_cumulative_participation_enabled() -> Result<()> { error_support::init_for_tests(); let tmp_dir = tempfile::tempdir()?; @@ -472,7 +512,7 @@ fn test_migrate_db_v1_to_v3_cumulative_participation_enabled() -> Result<()> { let db = Database::new(&tmp_dir, metrics.clone())?; let meta = db.get_store(StoreId::Meta); let reader = db.read()?; - assert_eq!(meta.get::(&reader, DB_KEY_DB_VERSION)?, Some(3)); + assert_eq!(meta.get::(&reader, DB_KEY_DB_VERSION)?, Some(4)); assert_eq!( meta.get::(&reader, DB_KEY_GLOBAL_USER_PARTICIPATION)?, None @@ -492,7 +532,7 @@ fn test_migrate_db_v1_to_v3_cumulative_participation_enabled() -> Result<()> { corrupt: Some(false), error: None, initial_version: Some(1), - migrated_version: Some(3), + migrated_version: Some(4), migration_error: None, }], ); @@ -511,6 +551,12 @@ fn test_migrate_db_v1_to_v3_cumulative_participation_enabled() -> Result<()> { from_version: 2, to_version: 3, error: None, + }, + DatabaseMigrationExtraDef { + reason: DatabaseMigrationReason::Upgrade.to_string(), + from_version: 3, + to_version: 4, + error: None, } ], ); @@ -519,7 +565,7 @@ fn test_migrate_db_v1_to_v3_cumulative_participation_enabled() -> Result<()> { } #[test] -fn test_migrate_db_v1_to_v3_cumulative_participation_disabled() -> Result<()> { +fn test_migrate_db_v1_to_v4_cumulative_participation_disabled() -> Result<()> { error_support::init_for_tests(); let tmp_dir = tempfile::tempdir()?; @@ -539,7 +585,7 @@ fn test_migrate_db_v1_to_v3_cumulative_participation_disabled() -> Result<()> { let db = Database::new(&tmp_dir, metrics.clone())?; let meta = db.get_store(StoreId::Meta); let reader = db.read()?; - assert_eq!(meta.get::(&reader, DB_KEY_DB_VERSION)?, Some(3)); + assert_eq!(meta.get::(&reader, DB_KEY_DB_VERSION)?, Some(4)); assert_eq!( meta.get::(&reader, DB_KEY_GLOBAL_USER_PARTICIPATION)?, None @@ -559,7 +605,7 @@ fn test_migrate_db_v1_to_v3_cumulative_participation_disabled() -> Result<()> { corrupt: Some(false), error: None, initial_version: Some(1), - migrated_version: Some(3), + migrated_version: Some(4), migration_error: None, }], ); @@ -578,6 +624,12 @@ fn test_migrate_db_v1_to_v3_cumulative_participation_disabled() -> Result<()> { from_version: 2, to_version: 3, error: None, + }, + DatabaseMigrationExtraDef { + reason: DatabaseMigrationReason::Upgrade.to_string(), + from_version: 3, + to_version: 4, + error: None, } ], ); @@ -586,7 +638,7 @@ fn test_migrate_db_v1_to_v3_cumulative_participation_disabled() -> Result<()> { } #[test] -fn test_migrate_db_v3_idempotent() -> Result<()> { +fn test_migrate_db_idempotent() -> Result<()> { error_support::init_for_tests(); let tmp_dir = tempfile::tempdir()?; @@ -596,7 +648,7 @@ fn test_migrate_db_v3_idempotent() -> Result<()> { let meta = SingleStore::new(rkv.open_single("meta", StoreOptions::create())?); let mut writer = rkv.write()?; - meta.put(&mut writer, DB_KEY_DB_VERSION, &3)?; + meta.put(&mut writer, DB_KEY_DB_VERSION, &DB_VERSION)?; meta.put(&mut writer, DB_KEY_EXPERIMENT_PARTICIPATION, &false)?; meta.put(&mut writer, DB_KEY_ROLLOUT_PARTICIPATION, &true)?; writer.commit()?; @@ -607,7 +659,10 @@ fn test_migrate_db_v3_idempotent() -> Result<()> { let db = Database::new(&tmp_dir, metrics.clone())?; let meta = db.get_store(StoreId::Meta); let reader = db.read()?; - assert_eq!(meta.get::(&reader, DB_KEY_DB_VERSION)?, Some(3)); + assert_eq!( + meta.get::(&reader, DB_KEY_DB_VERSION)?, + Some(DB_VERSION) + ); assert_eq!( meta.get::(&reader, DB_KEY_GLOBAL_USER_PARTICIPATION)?, None @@ -626,7 +681,7 @@ fn test_migrate_db_v3_idempotent() -> Result<()> { [DatabaseLoadExtraDef { corrupt: Some(false), error: None, - initial_version: Some(3), + initial_version: Some(DB_VERSION), migrated_version: None, migration_error: None, }], diff --git a/components/nimbus/src/tests/test_enrollment.rs b/components/nimbus/src/tests/test_enrollment.rs index 92888177bb..0b776f4943 100644 --- a/components/nimbus/src/tests/test_enrollment.rs +++ b/components/nimbus/src/tests/test_enrollment.rs @@ -1468,7 +1468,7 @@ fn test_evolver_experiment_not_enrolled_feature_conflict() -> Result<()> { matches!( e.status, EnrollmentStatus::NotEnrolled { - reason: NotEnrolledReason::FeatureConflict + reason: NotEnrolledReason::FeatureConflict { .. } } ) }) diff --git a/components/nimbus/src/tests/test_enrollment_bw_compat.rs b/components/nimbus/src/tests/test_enrollment_bw_compat.rs index fd70cc818b..8d07c472d8 100644 --- a/components/nimbus/src/tests/test_enrollment_bw_compat.rs +++ b/components/nimbus/src/tests/test_enrollment_bw_compat.rs @@ -2,10 +2,24 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. +#[cfg(feature = "stateful")] +use rkv::StoreOptions; + use serde_json::json; use crate::enrollment::*; +use cfg_if::cfg_if; + +cfg_if! { + if #[cfg(feature = "stateful")] { + use crate::error::Result; + use crate::metrics::DatabaseMigrationExtraDef; + use crate::tests::helpers::TestMetrics; + use crate::stateful::persistence::{Database, SingleStore, StoreId, DB_KEY_DB_VERSION, DatabaseMigrationReason}; + } +} + /// A suite of tests for b/w compat of data storage schema. /// /// We use the `Serialize/`Deserialize` impls on various structs in order to persist them @@ -57,18 +71,52 @@ fn test_experiment_schema_with_feature_ids() { // In SDK-260 we added a FeatureConflict variant to the NotEnrolledReason // schema. #[test] -fn test_not_enrolled_reason_schema_with_feature_conflict() { +#[cfg(feature = "stateful")] +fn test_not_enrolled_reason_schema_with_feature_conflict() -> Result<()> { + let tmp_dir = tempfile::tempdir()?; + let rkv = Database::open_rkv(&tmp_dir)?.0; + let meta_store = SingleStore::new(rkv.open_single("meta", StoreOptions::create())?); + let enrollment_store: SingleStore = + SingleStore::new(rkv.open_single("enrollments", StoreOptions::create())?); + let mut writer: rkv::Writer> = rkv.write()?; + // ⚠️ Warning : Do not change the JSON data used by this test. ⚠️ - let non_enrollment: ExperimentEnrollment = serde_json::from_value(json!({ + let non_enrollment: v3::LegacyExperimentEnrollment = serde_json::from_value(json!({ "slug": "secure-gold", "status": {"NotEnrolled": { "reason": "FeatureConflict", }} })) .unwrap(); - assert!( - matches!(non_enrollment.status, EnrollmentStatus::NotEnrolled{ ref reason, ..} if reason == &NotEnrolledReason::FeatureConflict) + + meta_store.put(&mut writer, DB_KEY_DB_VERSION, &3)?; + enrollment_store.put(&mut writer, &non_enrollment.slug, &non_enrollment)?; + writer.commit()?; + + let metrics = TestMetrics::new(); + let db = Database::new(&tmp_dir, metrics.clone())?; + + assert_eq!( + metrics.get_database_migration_events(), + [DatabaseMigrationExtraDef { + reason: DatabaseMigrationReason::Upgrade.to_string(), + from_version: 3, + to_version: 4, + error: None, + },] ); + + let enrollments: Vec = + db.collect_all::(StoreId::Enrollments)?; + assert!(matches!( + enrollments[0].status, + EnrollmentStatus::NotEnrolled { + reason: NotEnrolledReason::FeatureConflict { .. }, + .. + } + )); + + Ok(()) } // In bug 1997373, we added a `prev_gecko_pref_states` field to the EnrollmentStatus schema.