Bug 1994757 - Store the client ID in an additional plaintext file and report on regeneration#3292
Merged
Bug 1994757 - Store the client ID in an additional plaintext file and report on regeneration#3292
Conversation
badboy
force-pushed
the
push-lnryykzvpyqt
branch
8 times, most recently
from
333e836 to
8e10346
Compare
badboy
force-pushed
the
push-lnryykzvpyqt
branch
from
8e10346 to
e2931ad
Compare
badboy
requested review from
chutten and
travis79
and removed request for
a team
travis79
approved these changes
Oct 28, 2025
Member
travis79
left a comment
travis79
left a comment
There was a problem hiding this comment.
Nothing concerning here, r+ from me pending data-review
chutten
requested changes
Oct 28, 2025
Contributor
|
Oh, and also needs docs |
badboy
force-pushed
the
push-lnryykzvpyqt
branch
4 times, most recently
from
c39f7ea to
7405896
Compare
chutten
reviewed
Oct 31, 2025
Comment on lines
+378
to
+381
| glean | ||
| .core_metrics | ||
| .client_id | ||
| .set_from_uuid_sync(&glean, stored_client_id); |
Contributor
There was a problem hiding this comment.
Would this be considered a "mitigation" that we should avoid until later?
Member
Author
There was a problem hiding this comment.
So I guess technically?
But let's think through it:
- We assume a client right now can contain a c0ffee ID
- We also assume no
client_id.txtexists. - Glean inits, writes the client ID (
c0ffee) from the DB into the file. - Glean initializes core metrics. Detects
c0ffeein the database. Generates a new UUID. Stores that into the database and the file.
If for whatever reason (4) doesn't happen, the file will also include the c0ffee ID.
So "restoring" means we don't change anything.
If (4) happens, both the database and the file should have the same, valid ID (except if it happens to crash right in between these two operations, which I guess ... at our scale could happen?). So maybe we should check that stored_client_id is not the c0ffee ID either? It's a tiny edge case, but I had enough with our edge cases 🙈
In a test then ensure the additional uploader thread is not launched
chutten
approved these changes
Nov 3, 2025
Contributor
chutten
left a comment
chutten
left a comment
There was a problem hiding this comment.
I can't think of any way to make this better, well done
badboy
force-pushed
the
push-lnryykzvpyqt
branch
from
33c0f20 to
2ff3401
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a first approach at putting this state diagram into code.
This is not yet pretty. or organized. or ready to be looked at.
But it's not not wrong and I will expand upon that.