e2ee: commit a store transaction just after generating one-time keys #2930
Conversation
To make it easier to see it's a test function when looking up callers/callees.
bnjbvr
changed the title
You're right, this is made up vocabulary as of today, to disambiguate from sqlite- (or indexeddb, for that matter) level transactions. Thanks for the review! |
|
Maybe "application-level"? "applicative" has an existing meaning in functional programming. |
|
Ah yeah, I would argue people would think more immediately of applicative as "relating to an application" :-) |
bnjbvr
force-pushed
the
commit-transaction-after-generating-otk
branch
from
5f8ad7e
to
de7acb6
Compare
We're using applicative transactions to make sure that the account is properly synchronized in the cache vs in the database. Before this commit, the transaction would be committed only when *all* the operations in it succeeded. This was based on the assumption that most encryption requests could be replayed, by re-sending them to the server. Unfortunately, this assumption doesn't hold for when generating one-time keys: it could be that one time-keys would be generated by the client, then the applicative transaction would fail, resulting in the client "forgetting" about the one time keys it uploaded. The server rejects reuploads of existing one-time keys, so that would end up wedging a device, causing unable-to-decrypt events, without a proper way out. Here, we propose to commit the account just after one-time keys have been generated. One of a few things can occur: - the transaction succeeds, or the transactions fails *after* handling the server response of an upload of OTK; all good. - the transaction fails *before* uploading the one-time keys: we can upload them later. - the transaction fails *after* handling the response from the server: we can re-upload the same one-time keys, because it's the same set and it hasn't changed since the previous time. This doesn't go against the spirit of using transactions, since the in-memory caches are always properly synchronized in that case, and the lock used to make sure at most one process writes to the storage is still hold by the transaction object.
bnjbvr
force-pushed
the
commit-transaction-after-generating-otk
branch
from
de7acb6
to
038acc2
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2930 +/- ##
==========================================
- Coverage 82.75% 82.75% -0.01%
==========================================
Files 220 220
Lines 22462 22465 +3
==========================================
+ Hits 18588 18590 +2
- Misses 3874 3875 +1 ☔ View full report in Codecov by Sentry. |
|
|
||
| // Flush the applicative transaction as soon as the keys have been generated. If | ||
| // we don't do it here, then it's possible that the | ||
| // `StoreTransaction` fails, but we've uploaded keys in the |
There was a problem hiding this comment.
How would it happen? If the store transaction fails, then the receive_sync_changes will fail, then the whole sync will fail and the keys wont be uploaded?
I think that in that case the sync token wouldn't be persisted, so the app will try again the same sync?
|
Discussed on chat, it's probably not a fix to the issue I've observed, since the OTKs aren't uploaded at the same time a store transaction is alive. Instead, we'll add more logs to try to understand which keys are generated twice. |
|
This was an attempt to fix #1415 |
See commit messages for details. I've gotten several EX devices wedged, and this is my biggest guess so far why it has happened in the first place.