Element-R: Refactor per-session key backup download #3929

BillCarsonFr · 2023-11-29T10:50:17Z

Fixes element-hq/element-web#26535
Fixes element-hq/element-web#26312

Refactoring for the code that is downloading keys from backup per session when a decryption error occurs.
Will fix cache/4S server desync as well as partial history decryption problems

Note for reviewer:

Previously when a decryption error was encountered (in EventDecryptor) a call was made to startQueryKeyBackupRateLimited. This method was querying the backup and importing the key on success.

This mechanism was too limited, because if the call was made before the backup was set up it would fail and it was never retried later on (unless the app is refreshed). It was also always trying to use the decryption key that is in cache even if it's not valid any more.
The current implementation would fail to properly request a lot of sessions, and was probably saved by key_requesting that is going on in parallel.

This has been refactored by adding a new Object dedicated to per session downloads:
The PerSessionKeyBackupDownloader will queue up all requests to backup and process them one by one. It is resistant to invalid configurations (not trusted, no decryption key) and resistant to configuration changes. It will automatically resume querying when the backup is configured correctly.

Checklist

Tests written for new code (and old code if feasible)
Linter and other CI checks pass
Sign-off given on the changes (see CONTRIBUTING.md)

This change is marked as an internal change (Task), so will not be included in the changelog.

src/rust-crypto/PerSessionKeyBackupDownloader.ts

robintown

Was halfway through review when I saw Rich's comments come in, so I'll defer to him

src/rust-crypto/PerSessionKeyBackupDownloader.ts

richvdh

I think this is broadly sensible, though I haven't yet managed to follow all the logic through from start to finish. I've made a bunch of suggestions to make it easier to follow: please could you try them, and generally make sure that everything is consistent, and then I'll take another check on the logic here.

richvdh · 2023-12-05T21:57:49Z

src/rust-crypto/backup.ts

+
+    /**
+     * Creates a new backup decryptor for the given private key.
+     * @param decryptionKey - The private key to use for decryption.
+     */
+    public createBackupDecryptor(decryptionKey: RustSdkCryptoJs.BackupDecryptionKey): BackupDecryptor {
+        return new RustBackupDecryptor(decryptionKey);


what's the point of this? why not inline it?

It abstracts the implem RustBackupDecryptor from BackupDecryptor, also make it easier to test. just have to mock that instead of mocking somehow the constructor?

You can mock entire classes with something like https://jestjs.io/docs/es6-class-mocks#mocking-non-default-class-exports, which might be better if you can be bothered. Otherwise this is ok for now.

src/rust-crypto/backup.ts

richvdh · 2023-12-05T21:58:50Z

src/rust-crypto/rust-crypto.ts

@@ -82,7 +81,7 @@ interface ISignableObject {
    unsigned?: object;
 }

-const KEY_BACKUP_CHECK_RATE_LIMIT = 5000; // ms
+const KEY_BACKUP_BACKOFF = 5000; // ms


suggest moving this into PerSessionKeyBackupDownloader

still think we should move this. It feels odd that we have to keep this const here and pass in an extra parameter to PerSessionKeyBackupDownloader.

(just make it the default value for backOffDuration?)

src/rust-crypto/rust-crypto.ts

richvdh · 2023-12-05T22:01:44Z

src/rust-crypto/PerSessionKeyBackupDownloader.ts

+ *
+ * The PerSessionKeyBackupDownloader is resistant to backup configuration changes: it will automatically resume querying when
+ * the backup is configured correctly.
+ *


Suggested change

*

Again can this be a lint thing? why do that manually as untold convention

maybe, I don't know.

src/rust-crypto/PerSessionKeyBackupDownloader.ts

richvdh · 2023-12-05T22:46:58Z

src/rust-crypto/PerSessionKeyBackupDownloader.ts

+    CONFIGURATION_ERROR = "CONFIGURATION_ERROR",
+}
+
+/** Helper type for requested session*/


We can give more helpful documentation here:

Suggested change

/** Helper type for requested session*/

/** Details of a megolm session whose key we are trying to fetch. */

src/rust-crypto/PerSessionKeyBackupDownloader.ts

richvdh

This is looking much clearer and easier to follow now; I think generally it looks good. Still quite a few things which could be cleaned up but will leave it up to you how many you do.

I'd particularly like to get rid of forceCheck if possible: I think it's an area of significant confusion.

Also I think pauseLoop is very badly named.

src/rust-crypto/PerSessionKeyBackupDownloader.ts

richvdh · 2023-12-07T11:49:00Z

src/rust-crypto/rust-crypto.ts

@@ -82,7 +81,7 @@ interface ISignableObject {
    unsigned?: object;
 }

-const KEY_BACKUP_CHECK_RATE_LIMIT = 5000; // ms
+const KEY_BACKUP_BACKOFF = 5000; // ms


still think we should move this. It feels odd that we have to keep this const here and pass in an extra parameter to PerSessionKeyBackupDownloader.

(just make it the default value for backOffDuration?)

src/rust-crypto/PerSessionKeyBackupDownloader.ts

richvdh · 2023-12-07T11:51:24Z

src/rust-crypto/PerSessionKeyBackupDownloader.ts

+    private async getBackupDecryptionKey(): Promise<RustSdkCryptoJs.BackupKeys | null> {
+        try {
+            return await this.olmMachine.getBackupKeys();
+        } catch (e) {
+            return null;
+        }
+    }


I still think this should be inlined

src/rust-crypto/PerSessionKeyBackupDownloader.ts

spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts

richvdh · 2023-12-08T10:08:39Z

src/rust-crypto/PerSessionKeyBackupDownloader.ts

+                    const configuration = await this.getOrCreateBackupConfiguration();
+                    if (!configuration) {
+                        // Backup is not configured correctly, so stop the loop.
+                        this.downloadLoopRunning = false;


this is redundant FWIW: the finally below` will handle it.

Likewise the other one on line 298.

BillCarsonFr added 5 commits November 29, 2023 09:56

initial commit

328b85e

Merge branch 'develop' into valere/element-r/backup/per_session_download

7f1809b

new interation test

00df273

more comments

3f8c99e

Merge branch 'develop' into valere/element-r/backup/per_session_download

bcdc67f

BillCarsonFr added the T-Task Tasks for the team like planning label Nov 29, 2023

BillCarsonFr added 5 commits November 30, 2023 12:13

Merge branch 'develop' into valere/element-r/backup/per_session_download

260620f

fix test, quick refactor on request version

c9ad13b

cleaning and logs

c58f3d1

fix type

f091251

Merge branch 'develop' into valere/element-r/backup/per_session_download

c528e52

github-actions bot deployed to PR Documentation Preview December 1, 2023 11:03 View deployment

cleaning

f1235c8

BillCarsonFr marked this pull request as ready for review December 1, 2023 11:12

BillCarsonFr requested a review from a team as a code owner December 1, 2023 11:12

BillCarsonFr requested review from richvdh and robintown December 1, 2023 11:12

richvdh changed the title ~~Valere/element r/backup/per session download~~ Element-R: Refactor per-session key backup download Dec 1, 2023

richvdh reviewed Dec 1, 2023

View reviewed changes

robintown reviewed Dec 1, 2023

View reviewed changes

src/rust-crypto/PerSessionKeyBackupDownloader.ts Outdated Show resolved Hide resolved

BillCarsonFr added 5 commits December 5, 2023 14:12

remove delegate stuff

d503411

remove events and use timer mocks

d6d1a7d

Merge branch 'develop' into valere/element-r/backup/per_session_download

9823cb0

fix import

2ad906a

ts ignore in tests

26ea4ae

github-actions bot deployed to PR Documentation Preview December 5, 2023 17:59 View deployment

Quick cleaning

2951a9a

BillCarsonFr requested a review from richvdh December 5, 2023 19:08

github-actions bot deployed to PR Documentation Preview December 5, 2023 19:12 View deployment

richvdh requested changes Dec 5, 2023

View reviewed changes

github-actions bot deployed to PR Documentation Preview December 6, 2023 13:41 View deployment

BillCarsonFr added 2 commits December 7, 2023 11:51

Use Errors instead of Results

c5abd6f

Merge branch 'develop' into valere/element-r/backup/per_session_download

09505d2

github-actions bot deployed to PR Documentation Preview December 7, 2023 10:56 View deployment

cleaning

2245bc2

github-actions bot deployed to PR Documentation Preview December 7, 2023 11:21 View deployment

richvdh self-requested a review December 7, 2023 11:43

richvdh approved these changes Dec 7, 2023

View reviewed changes

BillCarsonFr added 9 commits December 7, 2023 17:53

Merge branch 'develop' into valere/element-r/backup/per_session_download

f9ad97d

review

c0cd300

remove forceCheck as not useful

ddbd3b5

bad naming

5418dc0

inline pauseLoop

9d05476

mark as paused in finally

1545127

code review

e890727

post merge fix

b244fd0

Merge branch 'develop' into valere/element-r/backup/per_session_download

e067b61

github-actions bot deployed to PR Documentation Preview December 7, 2023 17:31 View deployment

rename KeyDownloadRateLimit

b34f15f

github-actions bot deployed to PR Documentation Preview December 7, 2023 17:36 View deployment

BillCarsonFr added 2 commits December 7, 2023 18:58

use same config in loop and pass along

336e9f6

Merge branch 'develop' into valere/element-r/backup/per_session_download

3396635

github-actions bot deployed to PR Documentation Preview December 7, 2023 18:02 View deployment

BillCarsonFr added this pull request to the merge queue Dec 7, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 7, 2023

richvdh reviewed Dec 8, 2023

View reviewed changes

BillCarsonFr added this pull request to the merge queue Dec 8, 2023

Merged via the queue into develop with commit 13c7e0e Dec 8, 2023
25 checks passed

BillCarsonFr deleted the valere/element-r/backup/per_session_download branch December 8, 2023 14:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Element-R: Refactor per-session key backup download #3929

Element-R: Refactor per-session key backup download #3929

BillCarsonFr commented Nov 29, 2023 •

edited by github-actions bot

Loading

robintown left a comment

richvdh left a comment

richvdh Dec 5, 2023

BillCarsonFr Dec 6, 2023

richvdh Dec 7, 2023

richvdh Dec 5, 2023

richvdh Dec 7, 2023 •

edited

Loading

richvdh Dec 5, 2023

BillCarsonFr Dec 6, 2023

richvdh Dec 6, 2023

richvdh Dec 5, 2023

richvdh left a comment •

edited

Loading

richvdh Dec 7, 2023 •

edited

Loading

richvdh Dec 7, 2023

richvdh Dec 8, 2023

	/** Helper type for requested session*/
	/** Details of a megolm session whose key we are trying to fetch. */

Element-R: Refactor per-session key backup download #3929

Element-R: Refactor per-session key backup download #3929

Conversation

BillCarsonFr commented Nov 29, 2023 • edited by github-actions bot Loading

Note for reviewer:

Checklist

robintown left a comment

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh left a comment • edited Loading

Choose a reason for hiding this comment

richvdh Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillCarsonFr commented Nov 29, 2023 •

edited by github-actions bot

Loading

richvdh Dec 7, 2023 •

edited

Loading

richvdh left a comment •

edited

Loading

richvdh Dec 7, 2023 •

edited

Loading