Skip to content
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

When marking newly-backed up sessions, only fetch ones that are not backed up #2922

Conversation

andybalaam
Copy link
Member

@andybalaam andybalaam commented Dec 8, 2023

Fixes element-hq/element-web#26488

(No public API changes.)

Explanation

When we receive a response saying that a key backup is complete, we must mark the sessions whose keys are not backed up as such, so we don't back them up again.

In the current code. we call store.get_inbound_group_sessions(), then filter the results to find those sessions that are associated with this backup. This is very slow as there can be a very large number of sessions.

This PR is to call store.inbound_group_sessions_for_backup() instead. This method only fetches sessions that are not already backed up, which seems, if I understand it correctly, to be the exact set of sessions that we might want to mark as backed up at this time.

What limit to use?

The big question is what maximum limit to pass in (get_inbound_group_sessions has no limit, which is part of the cause of this problem). The point is that there may be new sessions that are not part of this backup, so we can't just use Self::BACKUP_BATCH_SIZE, which is the maximum number of sessions in this backup. I chose a large number that should mean that we usually get all the relevant sessions.

If we ever don't mark sessions as backed up even though they are, I am not sure of the consequences - I think maybe we will back them up again in the next backup, which would hopefully not be a problem.

If we ever had so many new sessions that the limit meant we never found any sessions to mark as backed up, that would be a problem, because we would never stop trying to back up the same sessions over and over again. I added an error log so we can at least identify the problem if it happens.

@andybalaam andybalaam changed the title When marking newly-backed up sessions, only fetch ones that are not b… When marking newly-backed up sessions, only fetch ones that are not backed up Dec 8, 2023
@andybalaam andybalaam force-pushed the andybalaam/only-fetch-unbacked-up-sessions-to-mark-as-backed-up branch from 8dd2938 to dd5e121 Compare December 8, 2023 12:45
…acked up

Signed-off-by: Andy Balaam <andy.balaam@matrix.org>
@andybalaam andybalaam force-pushed the andybalaam/only-fetch-unbacked-up-sessions-to-mark-as-backed-up branch from dd5e121 to b0a8904 Compare December 8, 2023 13:00
@andybalaam andybalaam marked this pull request as ready for review December 8, 2023 13:00
@andybalaam andybalaam requested a review from a team as a code owner December 8, 2023 13:00
@andybalaam andybalaam requested review from bnjbvr and removed request for a team December 8, 2023 13:00
Copy link

codecov bot commented Dec 8, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (e652069) 82.77% compared to head (b0a8904) 82.75%.

Files Patch % Lines
crates/matrix-sdk-crypto/src/backups/mod.rs 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2922      +/-   ##
==========================================
- Coverage   82.77%   82.75%   -0.03%     
==========================================
  Files         220      220              
  Lines       22462    22464       +2     
==========================================
- Hits        18593    18589       -4     
- Misses       3869     3875       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@BillCarsonFr BillCarsonFr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommand to create a new store method mark_session_as_backed_up(roomId, sessionId, sender_key).

Then in mark_request_as_sent you take the sessions in PendingBackup.session and iterate to mark them all as marked.

As for now we might need to decrypt to check any mismatch on sender_key

@andybalaam andybalaam marked this pull request as draft December 8, 2023 14:18
@andybalaam
Copy link
Member Author

OK, after discussion, we prefer to attempt to add a mark_sessions_as_backed_up method to the crypto store. Hopefully, this won't require us to decrypt any sessions at all.

@andybalaam andybalaam closed this Dec 13, 2023
@andybalaam
Copy link
Member Author

Closed in favour of #2934

@andybalaam andybalaam deleted the andybalaam/only-fetch-unbacked-up-sessions-to-mark-as-backed-up branch March 20, 2024 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Element R: App freezes while backing up megolm keys
2 participants