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
Fix fs bad encryption revision in sync monitor #1730
Fix fs bad encryption revision in sync monitor #1730
Conversation
79a3615
to
9cac41d
Compare
Codecov Report
@@ Coverage Diff @@
## master #1730 +/- ##
==========================================
- Coverage 87.83% 87.82% -0.01%
==========================================
Files 243 243
Lines 23564 23564
==========================================
- Hits 20697 20695 -2
- Misses 2867 2869 +2
Continue to review full report at Codecov.
|
9cac41d
to
f6be853
Compare
f6be853
to
e19b244
Compare
…on occurs before we process sharing.reencrypted message
e19b244
to
4322c23
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Just a few comments.
@@ -123,6 +123,9 @@ async def _bad_monitor(*, task_status=trio.TASK_STATUS_IGNORED): | |||
{"status": BackendConnStatus.CRASHED, "status_exc": spy.ANY}, | |||
) | |||
assert conn.status == BackendConnStatus.CRASHED | |||
caplog.assert_occured( | |||
"[exception] Unhandled exception [parsec.core.backend_connection.authenticated]" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test seems a bit sensitive with the arbitrary number of spaces _______
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes totally agree, I though about improving _remove_colors
to also trip the spaces, but it seems a bit complicated to do so I consider we will handle this in another PR when we will hit the issue (typically if updating structlog version breaks the tests)
# Not the right time for the sync, retry later. | ||
# `FSBadEncryptionRevision` occurs if the reencryption is quick | ||
# enough to start and finish before we process the sharing.reencrypted | ||
# message so we try a sync with the old encryption revision. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fix seems OK, but I'm a bit worried that we can no longer tell the difference between an acceptable race condition (accessing the realm while the new encryption key hasn't been received) and bug in the reencryption logic. Is there a way to tell those appart and maybe log the latter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point, however I don't see how we could tell each other apart...
We could try to check if the local encryption revision has changed between this sync and the next one, but it feel like a great way to add more complexity (and so more bugs !) into this code.
The good thing is we send at most 1 request every MAINTENANCE_MIN_WAIT
(i.e. 30s) so we are not flooding the server.
fix #1725