Fix fs bad encryption revision in sync monitor #1730
Conversation
touilleMan
force-pushed
the
fix-FSBadEncryptionRevision-in-sync_monitor
branch
from
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.
|
touilleMan
force-pushed
the
fix-FSBadEncryptionRevision-in-sync_monitor
branch
from
9cac41d
to
f6be853
Compare
touilleMan
force-pushed
the
fix-FSBadEncryptionRevision-in-sync_monitor
branch
from
f6be853
to
e19b244
Compare
…on occurs before we process sharing.reencrypted message
touilleMan
force-pushed
the
fix-FSBadEncryptionRevision-in-sync_monitor
branch
from
e19b244
to
4322c23
Compare
| @@ -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.
This test seems a bit sensitive with the arbitrary number of spaces _______.
There was a problem hiding this comment.
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.
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.
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