diff --git a/CHANGELOG.md b/CHANGELOG.md index 8d285ce0fb..1de772d07c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ * Stop reporting db file sizes during init phase ([#3331](https://github.com/mozilla/glean/pull/3331)) * Tiny performance improvement for putting tasks on the dispatcher ([#3318](https://github.com/mozilla/glean/pull/3318)) * Instrument the case when the `client_id.txt` file does not exist yet ([#3339](https://github.com/mozilla/glean/pull/3339)) + * When a missing client ID in the database is detected, Glean now restores the backup client ID ([#3334](https://github.com/mozilla/glean/pull/3334)) # v66.1.2 (2025-11-25) diff --git a/docs/dev/core/internal/client_id_recovery.md b/docs/dev/core/internal/client_id_recovery.md index a48dd31760..2c3856daad 100644 --- a/docs/dev/core/internal/client_id_recovery.md +++ b/docs/dev/core/internal/client_id_recovery.md @@ -14,8 +14,7 @@ Any inconsistencies in that data compared to the database will be reported and, if applicable, the client ID restored. **Note:** Glean v66.1.0 will only report the inconsistency, but will not restore a recovered client ID. -This allows us to measure the impact. -The mitigation will be enabled in a later release ([bug 1996862](https://bugzilla.mozilla.org/show_bug.cgi?id=1996862)). +From Glean v66.2.0 on we apply the mitigation and restore the client ID. The exact flow of decisions is depicted in the chart below. The implementation is in [`glean-core/src/core/mod.rs`](https://github.com/mozilla/glean/blob/HEAD/glean-core/src/core/mod.rs#L264) diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/DeletionPingTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/DeletionPingTest.kt index f27e30e59e..6d5f058f33 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/DeletionPingTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/DeletionPingTest.kt @@ -122,16 +122,21 @@ class DeletionPingTest { val pendingPingDir = File(Glean.getDataDir(), PENDING_PING_DIR) pendingPingDir.mkdirs() + // We manually disable upload and we don't want the ID to be restored, or this will trigger another + // deletion-request ping. + val clientIdTxt = File(Glean.getDataDir(), "client_id.txt") + assertTrue(clientIdTxt.delete()) + // Write a deletion-request ping file - var pingId = "775b6590-7f21-11ea-92e3-479998edf98c" - var submitPath = "/submit/org-mozilla-samples-gleancore/deletion-request/1/$pingId" + var deletionPingId = "775b6590-7f21-11ea-92e3-479998edf98c" + var submitPath = "/submit/org-mozilla-samples-gleancore/deletion-request/1/$deletionPingId" var content = "$submitPath\n{}" - var pingFile = File(pendingDeletionRequestDir, pingId) + var pingFile = File(pendingDeletionRequestDir, deletionPingId) assertTrue(pingFile.createNewFile()) pingFile.writeText(content) // Write a baseline ping file - pingId = "899b0ab8-7f20-11ea-ac03-ff76f2a19f1c" + var pingId = "899b0ab8-7f20-11ea-ac03-ff76f2a19f1c" submitPath = "/submit/org-mozilla-samples-gleancore/baseline/1/$pingId" content = "$submitPath\n{}" pingFile = File(pendingPingDir, pingId) @@ -153,7 +158,9 @@ class DeletionPingTest { var request = server.takeRequest(20L, TimeUnit.SECONDS)!! var docType = request.path!!.split("/")[3] + var docId = request.path!!.split("/")[5] assertEquals("Should have received a deletion-request ping", "deletion-request", docType) + assertEquals("Should be the manually constructed ping", deletionPingId, docId) // deletion-request ping is gone assertEquals(0, pendingDeletionRequestDir.listFiles()?.size) diff --git a/glean-core/rlb/tests/health_ping_file_overwrite.rs b/glean-core/rlb/tests/health_ping_file_overwrite.rs index 4f018ab36b..9fbb7d2764 100644 --- a/glean-core/rlb/tests/health_ping_file_overwrite.rs +++ b/glean-core/rlb/tests/health_ping_file_overwrite.rs @@ -79,7 +79,6 @@ fn test_pre_post_init_health_pings_exist() { let exception_uuid = &payload["metrics"]["uuid"]["glean.health_recovered_client_id"]; assert_eq!(&JsonValue::Null, exception_uuid); - // TODO(bug 1996862): We don't run the mitigation yet. - //let ping_client_id = &payload["client_info"]["client_id"]; - //assert_eq!(client_id, ping_client_id); + let ping_client_id = &payload["client_info"]["client_id"]; + assert_eq!(client_id, ping_client_id); } diff --git a/glean-core/src/core/mod.rs b/glean-core/src/core/mod.rs index fd5866a80f..1f50f50e48 100644 --- a/glean-core/src/core/mod.rs +++ b/glean-core/src/core/mod.rs @@ -359,6 +359,10 @@ impl Glean { .set_sync(&glean, ExceptionState::EmptyDb); // state (e) -- mitigation: store recovered client ID in DB + glean + .core_metrics + .client_id + .set_from_uuid_sync(&glean, stored_client_id); } else { let db_client_id = glean .core_metrics @@ -375,6 +379,10 @@ impl Glean { .set_sync(&glean, ExceptionState::RegenDb); // state (e) -- mitigation: store recovered client ID in DB + glean + .core_metrics + .client_id + .set_from_uuid_sync(&glean, stored_client_id); } Some(db_client_id) if db_client_id == *KNOWN_CLIENT_ID => { // state (i) @@ -392,6 +400,10 @@ impl Glean { // If we have a recovered client ID we also overwrite the database. // state (e) + glean + .core_metrics + .client_id + .set_from_uuid_sync(&glean, stored_client_id); } Some(db_client_id) if db_client_id == stored_client_id => { // all valid. nothing to do diff --git a/glean-core/tests/clientid_textfile.rs b/glean-core/tests/clientid_textfile.rs index 11133f4a36..991512aa3d 100644 --- a/glean-core/tests/clientid_textfile.rs +++ b/glean-core/tests/clientid_textfile.rs @@ -71,9 +71,8 @@ fn reused_clientid_from_file() { write_clientid_to_file(temp.path(), &new_uuid).unwrap(); let (glean, temp) = new_glean(Some(temp)); - // TODO(bug 1996862): We don't run the mitigation yet - //let db_client_id = clientid_metric().get_value(&glean, None).unwrap(); - //assert_eq!(new_uuid, db_client_id); + let db_client_id = clientid_metric().get_value(&glean, None).unwrap(); + assert_eq!(new_uuid, db_client_id); glean.submit_ping_by_name("health", Some("pre_init")); let mut pending = get_queued_pings(temp.path()).unwrap(); @@ -141,10 +140,8 @@ fn clientid_regen_issue_with_existing_db() { let (glean, temp) = new_glean(Some(temp)); - // TODO(bug 1996862): We don't run the mitigation yet - _ = file_client_id; - //let db_client_id = clientid_metric().get_value(&glean, None).unwrap(); - //assert_eq!(file_client_id, db_client_id); + let db_client_id = clientid_metric().get_value(&glean, None).unwrap(); + assert_eq!(file_client_id, db_client_id); glean.submit_ping_by_name("health", Some("pre_init")); let mut pending = get_queued_pings(temp.path()).unwrap(); @@ -225,10 +222,8 @@ fn c0ffee_in_db_gets_overwritten_by_stored_client_id() { let (glean, temp) = new_glean(Some(temp)); - // TODO(bug 1996862): We don't run the mitigation yet - _ = file_client_id; - //let db_client_id = clientid_metric().get_value(&glean, None).unwrap(); - //assert_eq!(file_client_id, db_client_id); + let db_client_id = clientid_metric().get_value(&glean, None).unwrap(); + assert_eq!(file_client_id, db_client_id); glean.submit_ping_by_name("health", Some("pre_init")); let mut pending = get_queued_pings(temp.path()).unwrap(); @@ -244,10 +239,9 @@ fn c0ffee_in_db_gets_overwritten_by_stored_client_id() { let exception_uuid = &payload["metrics"]["uuid"]["glean.health.recovered_client_id"].as_str(); assert_eq!(&Some(file_client_id), exception_uuid); - // TODO(bug 1996862): We don't run the mitigation yet - //let exception_uuid = &payload["client_info"]["client_id"].as_str(); - //assert_eq!(&Some(file_client_id), exception_uuid); - // instead we ensure we don't see the c0ffee ID either. + let exception_uuid = &payload["client_info"]["client_id"].as_str(); + assert_eq!(&Some(file_client_id), exception_uuid); + // We ensure we don't see the c0ffee ID either. let client_id = payload["client_info"]["client_id"].as_str().unwrap(); assert_ne!("c0ffeec0-ffee-c0ff-eec0-ffeec0ffeec0", client_id); }