Provide CryptoStore::mark_inbound_group_sessions_as_backed_up on stores and use it in BackupMachine::mark_request_as_sent
#2934
Conversation
andybalaam
force-pushed
the
andybalaam/mark_sessions_as_backed_up
branch
2 times, most recently
from
81dad3c
to
f58f34f
Compare
…in BackupMachine::mark_request_as_sent Signed-off-by: Andy Balaam <andy.balaam@matrix.org>
andybalaam
force-pushed
the
andybalaam/mark_sessions_as_backed_up
branch
from
f58f34f
to
4ebb8e2
Compare
andybalaam
changed the title
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2934 +/- ##
==========================================
- Coverage 82.86% 82.84% -0.02%
==========================================
Files 220 220
Lines 22554 22583 +29
==========================================
+ Hits 18689 18710 +21
- Misses 3865 3873 +8 ☔ View full report in Codecov by Sentry. |
Hywan
changed the title
CryptoStore::mark_inbound_group_sessions_as_backed_up on stores and use it in BackupMachine::mark_request_as_sent
There was a problem hiding this comment.
There is code duplication with an existing code (chunk_large_query_over and repeat_vars) in matrix-sdk-sqlite. Reading the existing code, I've found a bug (see the comments). Ideally, you can refactor your PR to use the existing code, fix the bugs, re-use your tests if you feel that's necessary (I believe the previous code wasn't unit tested).
Apart from that, the rest of the PR looks good to me.
Great, thanks - I will refactor to use |
Signed-off-by: Andy Balaam <andy.balaam@matrix.org>
Signed-off-by: Andy Balaam <andy.balaam@matrix.org>
…_as_backed_up Signed-off-by: Andy Balaam <andy.balaam@matrix.org>
|
|
||
| let session_ids_len = session_ids.len(); | ||
|
|
||
| self.chunk_large_query_over(session_ids, None, move |session_ids| { |
There was a problem hiding this comment.
@Hywan I've attempted to refactor it to use chunk_large_query_over - what do you think?
I couldn't find a way to do it in one transaction - I'm not sure whether that's needed or not.
I think I would need to add a new with_transaction_sync and that would need a new interact_async - do you agree?
There was a problem hiding this comment.
After discussing with @richvdh and @BillCarsonFr we think not having it all in a single transaction is OK.
Signed-off-by: Andy Balaam <andy.balaam@matrix.org>
Signed-off-by: Andy Balaam <andy.balaam@matrix.org>
Fixes element-hq/element-web#26488
I am not certain I've added a public API, but am guessing I have, so I documented it in the changelog.
Previously, in
BackupMachine::mark_request_as_sentwe fetched, deserialised and decrypted all existing sessions by callingget_inbound_group_sessions. Now, via a new methodCryptoStore::mark_inbound_group_sessions_as_backed_up, we avoid fetching any sessions except the ones we actually want to mark as backed up, and we mark them as backed up without decrypting them at all.