Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revisit some TODO items around keys_changed_at consistency checks. #176

Merged
merged 1 commit into from
Jun 2, 2020

Conversation

rfk
Copy link
Contributor

@rfk rfk commented Mar 6, 2020

Past-rfkelly helpfully left a TODO item in the code for checking consistency of keys_changed_at, which I went to revisit in light of Bug 1613032.

Unfortunately for Past-rfkelly, his reasoning that we could add this check once all servers are running the latest code is not sound. There's a (very very small) chance that a user could have:

  • Synced once against a new server, recording a keys_changed_at timestamp
  • Reset their account password, then synced once against an older server during the rollout of the keys_changed_at feature, recording an updated generation but leaving the old keys_changed_at.
  • Never synced again after that.

When this user suddenly comes back and syncs against a server with the proposed additional consistency checks, we would see an update to keys_changed_at but no corresponding update to generation, and incorrectly lock them out of their sync data.

I briefly toyed with the idea of adding a check that says "for records created after X date, ensure that a change to keys_changed_at is accompanied by a change to generation. But it seemed like too much effort for too little gain, so instead I have:

  • Added some more documentation about the consistency relationships we actually expect to see between these values.
  • Remove the "TODO" about adding more checks once all servers are deployed.
  • Added a different, less restrictive check that ensures we never see a keys_changed_at that is outright ahead of the corresponding generation.

@rfk rfk force-pushed the more-keys-changed-at-consistency branch from 92ee316 to 37ca001 Compare March 6, 2020 00:56
@tublitzed
Copy link
Contributor

hey, @rfk are you still wanting to merge this? We're starting the port to Rust here in a few weeks so probably good if we get it in before then if so.

@rfk
Copy link
Contributor Author

rfk commented May 5, 2020

Yes please, I think it'll be worth having for completeness and comparison when you do the rust version.

@rfk rfk requested a review from fzzzy May 5, 2020 21:48
@fzzzy
Copy link
Contributor

fzzzy commented May 27, 2020

I'm going to add this to the services engineering board so we don't keep forgetting it.

Copy link
Contributor

@fzzzy fzzzy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. Thanks for writing the extensive explanatory comment.

@fzzzy fzzzy merged commit f038656 into master Jun 2, 2020
@fzzzy fzzzy deleted the more-keys-changed-at-consistency branch June 2, 2020 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants