Skip to content

Fix key backup cached event ordering #5281

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.

Sign up for GitHub

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

Open
gumadeiras wants to merge 3 commits into
base: develop
Choose a base branch
from
+185 −5
Open

Fix key backup cached event ordering #5281

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c13345d
Fix key backup cached event ordering
gumadeiras Apr 16, 2026
3a076f4
Use created backup state for cached event
gumadeiras Apr 16, 2026
91a7e6d
Keep backup activation state coherent
gumadeiras Apr 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 71 additions & 0 deletions spec/unit/rust-crypto/backup.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ describe("Upload keys to backup", () => {
backupRoomKeys: vi.fn(),
isBackupEnabled: vi.fn().mockResolvedValue(true),
enableBackupV1: vi.fn(),
saveBackupDecryptionKey: vi.fn(),
verifyBackup: vi.fn().mockResolvedValue({
trusted: vi.fn().mockResolvedValue(true),
} as unknown as RustSdkCryptoJs.SignatureVerification),
Expand Down Expand Up @@ -140,4 +141,74 @@ describe("Upload keys to backup", () => {
expect(outgoingRequestProcessor.makeOutgoingRequest).toHaveBeenCalledTimes(1);
expect(mockOlmMachine.roomKeyCounts).toHaveBeenCalledTimes(0);
});

it("Should not emit key cached while creating a new backup", async () => {
fetchMock.hardReset();
fetchMock.mockGlobal();
fetchMock.get("path:/_matrix/client/v3/room_keys/version", {
status: 404,
body: {
errcode: "M_NOT_FOUND",
error: "No backup found",
},
});
fetchMock.post("path:/_matrix/client/v3/room_keys/version", { version: "42" });

await rustBackupManager.checkKeyBackupAndEnable(false);

const keyCachedListener = vi.fn();
rustBackupManager.on(CryptoEvent.KeyBackupDecryptionKeyCached, keyCachedListener);

await rustBackupManager.setupKeyBackup(async () => {});

expect(mockOlmMachine.saveBackupDecryptionKey).toHaveBeenCalledWith(expect.anything(), "42");
expect(mockOlmMachine.enableBackupV1).not.toHaveBeenCalled();
expect(keyCachedListener).not.toHaveBeenCalled();
});

it("Should emit a received backup key after caching the current backup info", async () => {
const keyBackupStatusState = Promise.withResolvers<{
enabled: boolean;
serverBackupVersion: string | undefined;
}>();
rustBackupManager.on(CryptoEvent.KeyBackupStatus, async (enabled) => {
const serverBackupInfo = await rustBackupManager.getServerBackupInfo();
keyBackupStatusState.resolve({
enabled,
serverBackupVersion: serverBackupInfo?.version,
});
});

const keyCachedEventState = Promise.withResolvers<{
activeBackupVersion: string | null;
eventVersion: string;
serverBackupVersion: string | undefined;
}>();
rustBackupManager.on(CryptoEvent.KeyBackupDecryptionKeyCached, async (eventVersion) => {
const [activeBackupVersion, serverBackupInfo] = await Promise.all([
rustBackupManager.getActiveBackupVersion(),
rustBackupManager.getServerBackupInfo(),
]);
keyCachedEventState.resolve({
activeBackupVersion,
eventVersion,
serverBackupVersion: serverBackupInfo?.version,
});
});

await expect(rustBackupManager.handleBackupSecretReceived(TestData.BACKUP_DECRYPTION_KEY_BASE64)).resolves.toBe(
true,
);

await expect(keyBackupStatusState.promise).resolves.toEqual({
enabled: true,
serverBackupVersion: "1",
});
await expect(keyCachedEventState.promise).resolves.toEqual({
activeBackupVersion: "1",
eventVersion: "1",
serverBackupVersion: "1",
});
expect(mockOlmMachine.enableBackupV1).toHaveBeenCalledTimes(1);
});
});
38 changes: 38 additions & 0 deletions spec/unit/rust-crypto/rust-crypto.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -811,13 +811,51 @@ describe("RustCrypto", () => {
expect(storeSpy).toHaveBeenCalledWith("m.megolm_backup.v1", expect.anything());
});

it("resetKeyBackup emits key cached with coherent backup state after a stale no-backup check", async () => {
const rustCrypto = await makeTestRustCrypto(
fetchMock as unknown as MatrixHttpApi<any>,
testData.TEST_USER_ID,
undefined,
secretStorage,
);

await expect(rustCrypto.getKeyBackupInfo()).resolves.toBeNull();

const keyBackupIsCached = new Promise<{
activeBackupVersion: string | null;
eventVersion: string;
serverBackupVersion: string | undefined;
}>((resolve) => {
rustCrypto.on(CryptoEvent.KeyBackupDecryptionKeyCached, async (eventVersion) => {
const [activeBackupVersion, serverBackupInfo] = await Promise.all([
rustCrypto.getActiveSessionBackupVersion(),
rustCrypto.getKeyBackupInfo(),
]);
resolve({
activeBackupVersion,
eventVersion,
serverBackupVersion: serverBackupInfo?.version,
});
});
});

await rustCrypto.resetKeyBackup();

await expect(keyBackupIsCached).resolves.toEqual({
activeBackupVersion: "1",
eventVersion: "1",
serverBackupVersion: "1",
});
});

it("bootstrapSecretStorage doesn't try to save megolm backup key not in cache", async () => {
const mockOlmMachine = {
isBackupEnabled: vi.fn().mockResolvedValue(false),
sign: vi.fn().mockResolvedValue({
asJSON: vi.fn().mockReturnValue("{}"),
}),
saveBackupDecryptionKey: vi.fn(),
enableBackupV1: vi.fn(),
exportCrossSigningKeys: vi.fn().mockResolvedValue({
masterKey: "sosecret",
userSigningKey: "secrets",
Expand Down
78 changes: 75 additions & 3 deletions src/rust-crypto/backup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ export class RustBackupManager extends TypedEventEmitter<RustBackupCryptoEvents,
this.logger.info(
`handleBackupSecretReceived: Valid decryption key for the current server-side backup version (${latestBackupInfo.version}) received`,
);
await this.saveBackupDecryptionKey(backupDecryptionKey, latestBackupInfo.version);
await this.saveBackupDecryptionKeyForCurrentBackup(backupDecryptionKey, latestBackupInfo);
Copy link
Copy Markdown
Member

@richvdh richvdh Apr 21, 2026

Choose a reason for hiding this comment

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

I'm not really following why this needs to change. Your description only mentions a problem happening during setupKeyBackup, but this is unrelated to setupKeyBackup. Does a similar problem also affect receiving the backup decryption key from other devices? I'm not sure why it would, because we don't update RustBackupManager.serverBackupInfo here (as far as I can tell), so there is nothing to race against?

return true;
} catch (e) {
this.logger.warn("handleBackupSecretReceived: Unable to validate backup decryption key", e);
Expand All @@ -214,16 +214,88 @@ export class RustBackupManager extends TypedEventEmitter<RustBackupCryptoEvents,
return false;
}

public async saveBackupDecryptionKey(
private async storeBackupDecryptionKey(
backupDecryptionKey: RustSdkCryptoJs.BackupDecryptionKey,
version: string,
): Promise<void> {
await this.olmMachine.saveBackupDecryptionKey(backupDecryptionKey, version);
}
Comment on lines +217 to +222
Copy link
Copy Markdown
Member

@richvdh richvdh Apr 21, 2026

Choose a reason for hiding this comment

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

I don't think there's any point in creating a dedicated function for this single line of code.


public async saveBackupDecryptionKey(
backupDecryptionKey: RustSdkCryptoJs.BackupDecryptionKey,
version: string,
): Promise<void> {
await this.storeBackupDecryptionKey(backupDecryptionKey, version);
// Emit an event that we have a new backup decryption key, so that the sdk can start
// importing keys from backup if needed.
this.emitBackupDecryptionKeyCached(version);
}

private emitBackupDecryptionKeyCached(version: string): void {
this.emit(CryptoEvent.KeyBackupDecryptionKeyCached, version);
}
Comment on lines +234 to 236
Copy link
Copy Markdown
Member

@richvdh richvdh Apr 21, 2026

Choose a reason for hiding this comment

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

likewise: this.emit(CryptoEvent.KeyBackupDecryptionKeyCached, version); really isn't very much longer than this.emitBackupDecryptionKeyCached(version);


/**
* Mark a newly-created backup as the current usable backup.
*
* The server has already accepted this backup, so this uses the creation
* response to update local backup state instead of racing a follow-up GET
* against eventual consistency.
*
* @param backupInfo - The newly-created backup details.
*/
public async enableKeyBackupFromCreation(backupInfo: KeyBackupCreationInfo): Promise<void> {
await this.enableKeyBackupFromInfo({
algorithm: backupInfo.algorithm,
auth_data: backupInfo.authData,
version: backupInfo.version,
});
Comment on lines +248 to +252
Copy link
Copy Markdown
Member

@richvdh richvdh Apr 21, 2026

Choose a reason for hiding this comment

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

why not just this.enableKeyBackupFromInfo(backupInfo) ?


this.emitBackupDecryptionKeyCached(backupInfo.version);
}

private async enableKeyBackupFromInfo(backupInfo: KeyBackupInfo): Promise<void> {
Copy link
Copy Markdown
Member

@richvdh richvdh Apr 21, 2026

Choose a reason for hiding this comment

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

this could also do with a doc comment. How does it differ from enableKeyBackup (which also takes a backupInfo)?

const version = backupInfo.version;
if (!version) {
throw new Error("Cannot enable key backup without version");
}
Comment on lines +259 to +261
Copy link
Copy Markdown
Member

@richvdh richvdh Apr 22, 2026

Choose a reason for hiding this comment

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

I think you can enforce this via the type system: declare the backupInfo parameter to have the type KeyBackupInfo & { version: string }


const previousServerBackupInfo = this.serverBackupInfo;
const previousCheckedForBackup = this.checkedForBackup;
this.serverBackupInfo = backupInfo;
this.checkedForBackup = true;

try {
const activeVersion = await this.getActiveBackupVersion();
if (activeVersion === null) {
await this.enableKeyBackup(backupInfo);
} else if (activeVersion !== version) {
await this.disableKeyBackup();
await this.enableKeyBackup(backupInfo);
}
Comment on lines +269 to +275
Copy link
Copy Markdown
Member

@richvdh richvdh Apr 22, 2026

Choose a reason for hiding this comment

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

This is (a subset of) the same logic as in doCheckKeyBackup, right? Can we factor it out and reuse it?

Comment on lines +269 to +275
Copy link
Copy Markdown
Member

@richvdh richvdh Apr 22, 2026

Choose a reason for hiding this comment

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

What will happen if this races with an ongoing call to doCheckKeyBackup, or another call to doCheckKeyBackup happens while this is in progress? You might have to wait for keyBackupCheckInProgress to complete, or something.

} catch (e) {
this.serverBackupInfo = previousServerBackupInfo;
Copy link
Copy Markdown
Member

@richvdh richvdh Apr 22, 2026

Choose a reason for hiding this comment

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

I somewhat wonder if this is necessary, given we know that these are in fact the details of the backup on the server.

this.checkedForBackup = previousCheckedForBackup;
throw e;
}
}

private async saveBackupDecryptionKeyForCurrentBackup(
backupDecryptionKey: RustSdkCryptoJs.BackupDecryptionKey,
backupInfo: KeyBackupInfo,
): Promise<void> {
Comment on lines +283 to +286
Copy link
Copy Markdown
Member

@richvdh richvdh Apr 21, 2026

Choose a reason for hiding this comment

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

This could do with a doc comment. How does it differ from saveBackupDecryptionKey, and why would I want one rather than the other?

Copy link
Copy Markdown
Member

@richvdh richvdh Apr 22, 2026

Choose a reason for hiding this comment

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

Since it seems to be a helper for handleBackupSecretReceived, I'd inline it, which means you can get rid of the check that backupInfo.version is defined.

const version = backupInfo.version;
if (!version) {
throw new Error("Cannot save backup decryption key for backup without version");
}

await this.storeBackupDecryptionKey(backupDecryptionKey, version);
await this.enableKeyBackupFromInfo(backupInfo);

// Emit after the backup is configured so listeners can immediately use the new backup.
this.emitBackupDecryptionKeyCached(version);
}

/**
* Import a list of room keys previously exported by exportRoomKeys
*
Expand Down Expand Up @@ -568,7 +640,7 @@ export class RustBackupManager extends TypedEventEmitter<RustBackupCryptoEvents,
},
);

await this.saveBackupDecryptionKey(randomKey, res.version);
await this.storeBackupDecryptionKey(randomKey, res.version);
Copy link
Copy Markdown
Member

@richvdh richvdh Apr 21, 2026

Choose a reason for hiding this comment

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

Note to self: this change is the main point of the PR. It avoids emitting a KeyBackupDecryptionKeyCached event, meaning that PerSessionKeyBackupDownloader will not immediately query for the new backup state.


return {
version: res.version,
Expand Down
3 changes: 1 addition & 2 deletions src/rust-crypto/rust-crypto.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1382,8 +1382,7 @@ export class RustCrypto extends TypedEventEmitter<RustCryptoEvents, CryptoEventH
await this.secretStorage.store("m.megolm_backup.v1", backupInfo.decryptionKey.toBase64());
}

// we can check and start async
this.checkKeyBackupAndEnable();
Copy link
Copy Markdown
Member

@andybalaam andybalaam Apr 20, 2026

Choose a reason for hiding this comment

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

I'm not sure why it's OK to remove this call to checkKeyBackupAndEnable. I don't think I see equivalent code in enableKeyBackupFromCreation. The old code ends up in doCheckKeyBackup which does a bunch of stuff that presumably is useful?

Copy link
Copy Markdown
Author

@gumadeiras gumadeiras Apr 20, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

this removes it from the just created backup path in resetKeyBackup() only; it is still called through other paths

i dropped it because resetKeyBackup() has just created the backup itself so my assumption is that it doesn't need to be rechecked since it was just created? if the server accepted the create request and returned the new version, we can use that creation result as the current backup state instead of immediately doing a second check

the new helper still does the important local enable work: cache serverBackupInfo, mark checkedForBackup, call the existing enableKeyBackup(...), set activeBackupVersion, emit KeyBackupStatus, and start backupKeysLoop()

so the intent is not to remove backup enabling; only to avoid the extra check before triggering KeyBackupDecryptionKeyCached

if checkKeyBackupAndEnable() has another required function which i may have totally missed, then yeah i should rework this back in; let me know!

paths that still call RustCrypto.checkKeyBackupAndEnable:
startup calls RustCrypto.checkKeyBackupAndEnable()
public API calls RustCrypto.checkKeyBackupAndEnable()
own-user trust changes call RustCrypto.checkKeyBackupAndEnable()
getServerBackupInfo() calls RustBackupManager.checkKeyBackupAndEnable(false)
upload recovery calls RustBackupManager.checkKeyBackupAndEnable(true)

await this.backupManager.enableKeyBackupFromCreation(backupInfo);
}

/**
Expand Down
Loading