recovery: Fix the recovery state not being updated after backups are enabled via secret send #3623
Conversation
This file contains 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
poljar
force-pushed
the
poljar/invalid-recovery-state-secret-send
branch
from
5ec246b
to
56e4035
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3623 +/- ##
=======================================
Coverage 84.19% 84.20%
=======================================
Files 256 256
Lines 26555 26576 +21
=======================================
+ Hits 22358 22378 +20
- Misses 4197 4198 +1 ☔ View full report in Codecov by Sentry. |
bnjbvr
reviewed
Jul 1, 2024
There was a problem hiding this comment.
Looks good at first sight, but:
- the commit message and the PR description mention an event handler being removed, and no event handler seems to be removed here. Should the commit message be updated?
- Can you explain the changes in the test, and confirm they're testing the new change in behavior, and if so, how/why, please?
Oh, removal might be the bad word for this. Previously we relied on the
The tests now allow some requests to be done more times, i.e. now we're updating the state when:
|
bnjbvr
approved these changes
Jul 1, 2024
poljar
changed the title
This patch switches the way we update the recovery state upon changes in
the backup state. Previously two places updated the recovery state after
the backup state changed:
1. A method living in the recovery subsystem that the backup
subsystem itself calls.
2. An event handler which is called when we receive a m.secret.send
event.
The first method is a hack because it introduces a circular dependency
between the recovery and backup subsystems.
More importantly, the second method can miss updates, because the backup
subsystem has a similar event handler which then processes the secret we
received and if the secret was a backup recovery key, enables backups.
Depending on the order these event handlers are called, the recovery
subsystem might update the recovery state before the secret has been
handled.
The backup subsystem provides an async stream which broadcasts updates
to the backup state, letting the recovery subsystem listen to this
stream and update its state if we notice such updates fixes both
problems we listed above. The method in the first bullet point was
completely removed, the event handler is kept for other secret types but
we don't rely on it for the backup state anymore.
poljar
force-pushed
the
poljar/invalid-recovery-state-secret-send
branch
from
56e4035
to
44a4505
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
A review commit by commit is the best way to go through this PR, please take a look at the commit message of the last commit for a full explanation on how the fix works.
A test for this was written in #3583, but sadly it doesn't reproduce the issue since the issue is only triggered if event handlers are run in the wrong order. This PR doesn't rely on the event handler for the backup state, so the order doesn't matter anymore.
This closes: #3445.