diff --git a/.changeset/fix-slow-first-encrypted-send.md b/.changeset/fix-slow-first-encrypted-send.md new file mode 100644 index 000000000..4ff51140c --- /dev/null +++ b/.changeset/fix-slow-first-encrypted-send.md @@ -0,0 +1,5 @@ +--- +default: patch +--- + +Fix sending the first message after startup taking up to 40 seconds in encrypted rooms: outgoing crypto requests and key backup checks now coalesce instead of queueing one full run per caller, and repeated undecryptable events no longer re-request the key backup version from the server. diff --git a/src/app/crypto/engineCrypto/EngineCrypto.ts b/src/app/crypto/engineCrypto/EngineCrypto.ts index 6185709c2..d24ccdf4d 100644 --- a/src/app/crypto/engineCrypto/EngineCrypto.ts +++ b/src/app/crypto/engineCrypto/EngineCrypto.ts @@ -39,6 +39,7 @@ import { } from '../verification/state'; import { engineInvoke, type EngineIdentity } from '../olmMachine/engineInvoke'; import { sendOutgoingRequest, type OutgoingRequest } from './outgoing'; +import { createCoalescedRunner } from './coalescedRunner'; import type { BackupDecryptor, CryptoBackend, @@ -320,13 +321,31 @@ export class EngineCrypto /** Live requests, keyed by flow id, so the synchronous CryptoApi getters can answer. */ readonly #verificationRequests = new Map(); - #flushing: Promise = Promise.resolve(); + readonly #outgoingFlush = createCoalescedRunner( + () => + this.#drainOutgoingRequests().catch((error: unknown) => { + engineCryptoLog.error('general', 'Draining outgoing crypto requests failed', error); + }), + () => this.#stopped + ); readonly #roomsWithTrackedMembers = new Set(); - #backingUp: Promise = Promise.resolve(); + readonly #backupUpload = createCoalescedRunner( + () => + this.#uploadRoomKeysToBackup().catch((error: unknown) => { + engineCryptoLog.error('general', 'Uploading room keys to backup failed', error); + }), + () => this.#stopped + ); + + #keyBackupCheck: Promise | undefined; + + #serverBackupInfo: KeyBackupInfo | null | undefined; - #checkingKeyBackup: Promise = Promise.resolve(null); + #deviceCreationTimeMs: number | null | undefined; + + #hasBackupDecryptionKey: boolean | undefined; readonly #eventsPendingKey = new Map>(); @@ -348,11 +367,7 @@ export class EngineCrypto } #scheduleKeyBackup(): void { - this.#backingUp = this.#backingUp.then(() => - this.#uploadRoomKeysToBackup().catch((error: unknown) => { - engineCryptoLog.error('general', 'Uploading room keys to backup failed', error); - }) - ); + void this.#backupUpload.schedule(); } async #uploadRoomKeysToBackup(): Promise { @@ -677,12 +692,7 @@ export class EngineCrypto } #flushOutgoingRequests(): Promise { - this.#flushing = this.#flushing.then(() => - this.#drainOutgoingRequests().catch((error: unknown) => { - engineCryptoLog.error('general', 'Draining outgoing crypto requests failed', error); - }) - ); - return this.#flushing; + return this.#outgoingFlush.schedule(); } /** matrix-sdk-crypto only clears a request once told it was sent, so a failure here @@ -726,9 +736,14 @@ export class EngineCrypto const processed = await this.#receiveSyncChanges({ toDeviceEvents: events }); const received: ReceivedToDeviceMessage[] = []; - const messages = processed.map( - (event) => [event, JSON.parse(event.rawEvent) as IToDeviceEvent] as const - ); + const messages = processed.flatMap((event) => { + try { + return [[event, JSON.parse(event.rawEvent) as IToDeviceEvent] as const]; + } catch (error) { + engineCryptoLog.warn('general', 'Dropping an unparseable to-device event', error); + return []; + } + }); if ( messages.some( @@ -831,6 +846,8 @@ export class EngineCrypto stop(): void { this.#stopped = true; + this.#outgoingFlush.cancel(); + this.#backupUpload.cancel(); this.#eventsPendingKey.clear(); this.#roomsWithTrackedMembers.clear(); } @@ -880,7 +897,9 @@ export class EngineCrypto const members = await room.getEncryptionTargetMembers(); const users = members.map((member) => member.userId); - if (!this.#roomsWithTrackedMembers.has(room.roomId)) { + if (this.#roomsWithTrackedMembers.has(room.roomId)) { + void this.#flushOutgoingRequests(); + } else { await this.#trackUsers(users); await this.#flushOutgoingRequests(); this.#roomsWithTrackedMembers.add(room.roomId); @@ -888,7 +907,6 @@ export class EngineCrypto const claim = (await this.#call('getMissingSessions', { users })) as OutgoingRequest | null; await this.#sendTracked(claim); - await this.#flushOutgoingRequests(); const shared = ((await this.#call('shareRoomKey', { roomId: room.roomId, @@ -899,7 +917,6 @@ export class EngineCrypto // eslint-disable-next-line no-await-in-loop await this.#sendTracked(request); } - await this.#flushOutgoingRequests(); const encrypted = (await this.#call('encryptRoomEvent', { roomId: room.roomId, @@ -1021,8 +1038,22 @@ export class EngineCrypto } } + async #deviceCreationTime(): Promise { + if (this.#deviceCreationTimeMs === undefined) { + this.#deviceCreationTimeMs = (await this.#call('deviceCreationTimeMs')) as number | null; + } + return this.#deviceCreationTimeMs; + } + + async #hasSessionBackupKey(): Promise { + if (this.#hasBackupDecryptionKey === undefined) { + this.#hasBackupDecryptionKey = (await this.getSessionBackupPrivateKey()) !== null; + } + return this.#hasBackupDecryptionKey; + } + async #throwIfHistorical(event: MatrixEvent, details: Record): Promise { - const createdAt = (await this.#call('deviceCreationTimeMs')) as number | null; + const createdAt = await this.#deviceCreationTime(); if (createdAt === null || event.getTs() > createdAt) return; const backupInfo = await this.getKeyBackupInfo().catch(() => null); @@ -1034,7 +1065,7 @@ export class EngineCrypto ); } - const usable = (await this.getSessionBackupPrivateKey()) !== null; + const usable = await this.#hasSessionBackupKey(); throw new DecryptionError( usable ? DecryptionFailureCode.HISTORICAL_MESSAGE_WORKING_BACKUP @@ -1460,12 +1491,8 @@ export class EngineCrypto async isCrossSigningReady(): Promise { const status = await this.getCrossSigningStatus(); const cached = status.privateKeysCachedLocally; - return ( - status.publicKeysOnDevice && - cached.masterKey && - cached.selfSigningKey && - cached.userSigningKey - ); + const cachedLocally = cached.masterKey && cached.selfSigningKey && cached.userSigningKey; + return status.publicKeysOnDevice && (cachedLocally || status.privateKeysInSecretStorage); } async getCrossSigningKeyId( @@ -1658,10 +1685,13 @@ export class EngineCrypto hasSelfSigning: boolean; hasUserSigning: boolean; }; + const inStorage = await Promise.all( + SECRETS_IN_STORAGE.map(async (name) => Boolean(await this.#mx.secretStorage.isStored(name))) + ); return { publicKeysOnDevice: status.hasMaster && status.hasSelfSigning && status.hasUserSigning, - privateKeysInSecretStorage: false, + privateKeysInSecretStorage: inStorage.every(Boolean), privateKeysCachedLocally: { masterKey: status.hasMaster, selfSigningKey: status.hasSelfSigning, @@ -1772,6 +1802,7 @@ export class EngineCrypto async storeSessionBackupPrivateKey(key: Uint8Array, version: string): Promise { await this.#call('saveBackupDecryptionKey', { decryptionKey: encodeBase64(key), version }); + this.#hasBackupDecryptionKey = true; this.emit(CryptoEvent.KeyBackupDecryptionKeyCached, version); } @@ -1790,7 +1821,7 @@ export class EngineCrypto async #handleSecretReceived(name: string, value: string): Promise { if (name !== 'm.megolm_backup.v1') return false; - const backupInfo = await this.getKeyBackupInfo(); + const backupInfo = await this.#requestKeyBackupVersion(); if (!backupInfo?.version) { engineCryptoLog.warn('general', 'Received a backup key with no server-side backup'); return false; @@ -1827,7 +1858,7 @@ export class EngineCrypto const encoded = await this.#mx.secretStorage.get('m.megolm_backup.v1'); if (!encoded) throw new Error('No session backup key in secret storage'); - const backupInfo = await this.getKeyBackupInfo(); + const backupInfo = await this.#requestKeyBackupVersion(); if (!backupInfo?.version) throw new Error('No key backup version to attach the key to'); await this.storeSessionBackupPrivateKey(decodeBase64(encoded), backupInfo.version); @@ -1859,8 +1890,13 @@ export class EngineCrypto } async getKeyBackupInfo(): Promise { + if (this.#serverBackupInfo !== undefined) return this.#serverBackupInfo; + return this.#requestKeyBackupVersion(); + } + + async #requestKeyBackupVersion(): Promise { try { - return await this.#mx.http.authedRequest( + this.#serverBackupInfo = await this.#mx.http.authedRequest( Method.Get, '/room_keys/version', undefined, @@ -1868,9 +1904,10 @@ export class EngineCrypto { prefix: ClientPrefix.V3 } ); } catch (error) { - if ((error as { errcode?: string }).errcode === 'M_NOT_FOUND') return null; - throw error; + if ((error as { errcode?: string }).errcode !== 'M_NOT_FOUND') throw error; + this.#serverBackupInfo = null; } + return this.#serverBackupInfo; } async #getKeyBackupInfoForVersion(version: string): Promise { @@ -1889,14 +1926,14 @@ export class EngineCrypto } async checkKeyBackupAndEnable(): Promise { - this.#checkingKeyBackup = this.#checkingKeyBackup - .catch(() => null) - .then(() => this.#checkKeyBackupAndEnable()); - return this.#checkingKeyBackup; + this.#keyBackupCheck ??= this.#checkKeyBackupAndEnable().finally(() => { + this.#keyBackupCheck = undefined; + }); + return this.#keyBackupCheck; } async #checkKeyBackupAndEnable(): Promise { - const backupInfo = await this.getKeyBackupInfo(); + const backupInfo = await this.#requestKeyBackupVersion(); const activeVersion = await this.getActiveSessionBackupVersion(); if (!backupInfo?.version) { @@ -1929,6 +1966,7 @@ export class EngineCrypto async #disableKeyBackup(): Promise { await this.#call('disableBackup'); + this.#hasBackupDecryptionKey = undefined; this.emit(CryptoEvent.KeyBackupStatus, false); } @@ -1961,6 +1999,7 @@ export class EngineCrypto { prefix: ClientPrefix.V3 } ); + this.#serverBackupInfo = undefined; await this.#call('enableBackupV1', { publicKeyBase64: publicKey, version: created.version }); await this.storeSessionBackupPrivateKey(key.privateKey, created.version); await this.#pushSecretToVerifiedDevices('m.megolm_backup.v1'); @@ -1995,7 +2034,7 @@ export class EngineCrypto for (let attempt = 0; attempt < MAX_BACKUP_VERSIONS_TO_DELETE; attempt += 1) { // eslint-disable-next-line no-await-in-loop - const info = await this.getKeyBackupInfo(); + const info = await this.#requestKeyBackupVersion(); if (!info?.version || seen.has(info.version)) return; seen.add(info.version); // eslint-disable-next-line no-await-in-loop @@ -2026,6 +2065,7 @@ export class EngineCrypto undefined, { prefix: ClientPrefix.V3 } ); + this.#serverBackupInfo = undefined; if (active === version) await this.#disableKeyBackup(); } @@ -2087,7 +2127,7 @@ export class EngineCrypto passphrase: string, opts?: KeyBackupRestoreOpts ): Promise { - const backupInfo = await this.getKeyBackupInfo(); + const backupInfo = await this.#requestKeyBackupVersion(); const passphraseInfo = backupInfo?.auth_data?.private_key_salt ? backupInfo.auth_data : undefined; diff --git a/src/app/crypto/engineCrypto/backupInfoCache.test.ts b/src/app/crypto/engineCrypto/backupInfoCache.test.ts new file mode 100644 index 000000000..473093fab --- /dev/null +++ b/src/app/crypto/engineCrypto/backupInfoCache.test.ts @@ -0,0 +1,62 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import type { MatrixClient } from '$types/matrix-sdk'; +import { engineInvoke } from '../olmMachine/engineInvoke'; +import { EngineCrypto } from './EngineCrypto'; + +vi.mock('../olmMachine/engineInvoke', () => ({ + engineInvoke: vi.fn<(...args: never[]) => Promise>(), +})); + +const mockInvoke = vi.mocked(engineInvoke); + +const notFound = Object.assign(new Error('no backup'), { errcode: 'M_NOT_FOUND' }); + +const setup = () => { + const versionGets: unknown[] = []; + const authedRequest = vi.fn<(...args: never[]) => Promise>( + async (_method: unknown, url: unknown) => { + if (url === '/room_keys/version') { + versionGets.push(url); + throw notFound; + } + return {}; + } + ); + const mx = { + http: { authedRequest }, + secretStorage: { isStored: async () => null }, + } as unknown as MatrixClient; + return { mx, versionGets }; +}; + +describe('key backup info caching', () => { + beforeEach(() => mockInvoke.mockReset()); + + it('serves repeated getKeyBackupInfo calls from one request', async () => { + const { mx, versionGets } = setup(); + mockInvoke.mockImplementation(async () => null); + const crypto = new EngineCrypto(mx, { userId: '@me:e.org', deviceId: 'D' }); + + await vi.waitFor(() => expect(versionGets.length).toBe(1)); + + await crypto.getKeyBackupInfo(); + await crypto.getKeyBackupInfo(); + await crypto.getKeyBackupInfo(); + + expect(versionGets).toHaveLength(1); + }); + + it('re-reads the version after a backup is deleted', async () => { + const { mx, versionGets } = setup(); + mockInvoke.mockImplementation(async () => null); + const crypto = new EngineCrypto(mx, { userId: '@me:e.org', deviceId: 'D' }); + await vi.waitFor(() => expect(versionGets.length).toBe(1)); + + await crypto.getKeyBackupInfo(); + expect(versionGets).toHaveLength(1); + + await crypto.deleteKeyBackupVersion('1'); + await crypto.getKeyBackupInfo(); + expect(versionGets).toHaveLength(2); + }); +}); diff --git a/src/app/crypto/engineCrypto/coalescedRunner.ts b/src/app/crypto/engineCrypto/coalescedRunner.ts new file mode 100644 index 000000000..bf90b56d8 --- /dev/null +++ b/src/app/crypto/engineCrypto/coalescedRunner.ts @@ -0,0 +1,35 @@ +/** Runs `task` serially, collapsing requests that arrive mid-run into one further run. */ +export const createCoalescedRunner = (task: () => Promise, stopped = () => false) => { + let next: PromiseWithResolvers | undefined; + let running = false; + + const loop = async (): Promise => { + running = true; + try { + while (!stopped() && next) { + const pending = next; + next = undefined; + // eslint-disable-next-line no-await-in-loop + await task().catch(() => undefined); + pending.resolve(); + } + } finally { + running = false; + next?.resolve(); + next = undefined; + } + }; + + return { + schedule: (): Promise => { + next ??= Promise.withResolvers(); + const done = next.promise; + if (!running) void loop(); + return done; + }, + cancel: (): void => { + next?.resolve(); + next = undefined; + }, + }; +}; diff --git a/src/app/crypto/engineCrypto/engineShapes.test.ts b/src/app/crypto/engineCrypto/engineShapes.test.ts index d8f88efff..06cb2466c 100644 --- a/src/app/crypto/engineCrypto/engineShapes.test.ts +++ b/src/app/crypto/engineCrypto/engineShapes.test.ts @@ -9,8 +9,19 @@ vi.mock('../olmMachine/engineInvoke', () => ({ const mockInvoke = vi.mocked(engineInvoke); -const crypto = () => - new EngineCrypto({} as MatrixClient, { userId: '@me:example.org', deviceId: 'DEVICE' }); +const crypto = (stored: string[] = []) => + new EngineCrypto( + { + secretStorage: { isStored: async (name: string) => (stored.includes(name) ? {} : null) }, + } as unknown as MatrixClient, + { userId: '@me:example.org', deviceId: 'DEVICE' } + ); + +const CROSS_SIGNING_SECRETS = [ + 'm.cross_signing.master', + 'm.cross_signing.self_signing', + 'm.cross_signing.user_signing', +]; /** Mirrors the exact JSON the Rust side emits; a rename there breaks these loudly. */ describe('engine payload shapes', () => { @@ -105,4 +116,19 @@ describe('engine payload shapes', () => { userSigningKey: false, }); }); + + it('reports cross-signing keys held in secret storage', async () => { + mockInvoke.mockResolvedValue({ + hasMaster: false, + hasSelfSigning: false, + hasUserSigning: false, + }); + + await expect(crypto(CROSS_SIGNING_SECRETS).getCrossSigningStatus()).resolves.toMatchObject({ + privateKeysInSecretStorage: true, + }); + await expect( + crypto(CROSS_SIGNING_SECRETS.slice(0, 2)).getCrossSigningStatus() + ).resolves.toMatchObject({ privateKeysInSecretStorage: false }); + }); }); diff --git a/src/app/crypto/engineCrypto/outgoingCoalesce.test.ts b/src/app/crypto/engineCrypto/outgoingCoalesce.test.ts new file mode 100644 index 000000000..dcc514ac3 --- /dev/null +++ b/src/app/crypto/engineCrypto/outgoingCoalesce.test.ts @@ -0,0 +1,94 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import type { MatrixClient } from '$types/matrix-sdk'; +import { engineInvoke } from '../olmMachine/engineInvoke'; +import { EngineCrypto } from './EngineCrypto'; + +vi.mock('../olmMachine/engineInvoke', () => ({ + engineInvoke: vi.fn<(...args: never[]) => Promise>(), +})); + +const mockInvoke = vi.mocked(engineInvoke); + +const identity = { userId: '@me:e.org', deviceId: 'D' }; + +const settle = () => + new Promise((resolve) => { + setTimeout(resolve, 0); + }); + +describe('coalesced crypto work', () => { + beforeEach(() => mockInvoke.mockReset()); + + it('collapses flushes that arrive while a drain is running into one extra drain', async () => { + const gate = Promise.withResolvers(); + let drains = 0; + mockInvoke.mockImplementation(async (_identity, method) => { + if (method !== 'outgoingRequests') return null; + drains += 1; + await gate.promise; + return []; + }); + + const mx = { + http: { authedRequest: vi.fn<() => Promise>() }, + } as unknown as MatrixClient; + const crypto = new EngineCrypto(mx, identity); + for (let i = 0; i < 5; i += 1) crypto.onSyncCompleted({}); + + await Promise.resolve(); + expect(drains).toBe(1); + + gate.resolve(); + await vi.waitFor(() => expect(drains).toBe(2)); + + await settle(); + expect(drains).toBe(2); + }); + + it('shares one in-flight key backup check between concurrent callers', async () => { + const gate = Promise.withResolvers(); + let versionGets = 0; + const authedRequest = vi.fn<(...args: never[]) => Promise>( + async (_method: unknown, url: unknown) => { + if (url === '/room_keys/version') { + versionGets += 1; + await gate.promise; + } + return {}; + } + ); + mockInvoke.mockImplementation(async () => null); + + const crypto = new EngineCrypto( + { http: { authedRequest } } as unknown as MatrixClient, + identity + ); + const checks = Array.from({ length: 5 }, () => crypto.checkKeyBackupAndEnable()); + + await Promise.resolve(); + gate.resolve(); + await Promise.all(checks); + await settle(); + + expect(versionGets).toBe(1); + }); + + it('drops an unparseable to-device event instead of failing the batch', async () => { + mockInvoke.mockImplementation(async (_identity, method) => { + if (method !== 'receiveSyncChanges') return null; + return [ + { type: 2, rawEvent: 'not json' }, + { type: 2, rawEvent: JSON.stringify({ type: 'm.room_key', sender: '@a:e.org' }) }, + ]; + }); + + const mx = { + http: { authedRequest: vi.fn<() => Promise>() }, + } as unknown as MatrixClient; + const crypto = new EngineCrypto(mx, identity); + + const received = await crypto.preprocessToDeviceMessages([]); + expect(received).toHaveLength(1); + expect(received[0]?.message.type).toBe('m.room_key'); + }); +});