Skip to content

Commit 02a0bee

Browse files
committed
Fix first encrypted message delay and key query ordering
1 parent 4dd969e commit 02a0bee

4 files changed

Lines changed: 477 additions & 25 deletions

File tree

‎src/app/crypto/engineCrypto/EngineCrypto.ts‎

Lines changed: 30 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ import {
4545
type EngineVerificationState,
4646
} from '../verification/state';
4747
import { engineInvoke, type EngineIdentity } from '../olmMachine/engineInvoke';
48-
import { sendOutgoingRequest, type OutgoingRequest } from './outgoing';
48+
import { RequestType, sendOutgoingRequest, type OutgoingRequest } from './outgoing';
4949
import { createCoalescedRunner } from './coalescedRunner';
5050
import type {
5151
BackupDecryptor,
@@ -358,6 +358,8 @@ export class EngineCrypto
358358

359359
#claimChain: Promise<unknown> = Promise.resolve();
360360

361+
#keyQueryChain: Promise<unknown> = Promise.resolve();
362+
361363
readonly #encryptionChains = new Map<string, Promise<unknown>>();
362364

363365
readonly #backupUpload = createCoalescedRunner(
@@ -754,13 +756,20 @@ export class EngineCrypto
754756

755757
async #sendTracked(request: unknown): Promise<void> {
756758
if (!isOutgoingRequest(request)) return;
757-
const response = await sendOutgoingRequest(this.#mx, request);
758-
if (typeof request.id !== 'string') return;
759-
await this.#call('markRequestAsSent', {
760-
requestId: request.id,
761-
requestType: request.type,
762-
response,
763-
});
759+
const send = async () => {
760+
const response = await sendOutgoingRequest(this.#mx, request);
761+
if (typeof request.id !== 'string') return;
762+
await this.#call('markRequestAsSent', {
763+
requestId: request.id,
764+
requestType: request.type,
765+
response,
766+
});
767+
};
768+
if (request.type !== RequestType.KeysQuery) return send();
769+
770+
const next = this.#keyQueryChain.catch(() => undefined).then(send);
771+
this.#keyQueryChain = next;
772+
await next;
764773
}
765774

766775
async onIncomingKeyVerificationRequest(sender: string, transactionId: string): Promise<boolean> {
@@ -832,13 +841,7 @@ export class EngineCrypto
832841
// Sequential: the engine's queue is ordered and later requests can depend on
833842
// earlier ones having landed.
834843
// eslint-disable-next-line no-await-in-loop
835-
const response = await sendOutgoingRequest(this.#mx, request);
836-
// eslint-disable-next-line no-await-in-loop
837-
await this.#call('markRequestAsSent', {
838-
requestId: request.id,
839-
requestType: request.type,
840-
response,
841-
});
844+
await this.#sendTracked(request);
842845
sent += 1;
843846
} catch (error) {
844847
// Loud: a request the engine never marks sent is retried on every sync forever.
@@ -1089,19 +1092,24 @@ export class EngineCrypto
10891092
return this.#serializeForRoom(room.roomId, () => this.#encryptEventInner(event, room));
10901093
}
10911094

1092-
async #encryptEventInner(event: MatrixEvent, room: Room): Promise<void> {
1093-
// The megolm session has to reach every device in the room before the event does.
1095+
async #prepareRoomForEncryption(room: Room): Promise<string[]> {
10941096
const members = await room.getEncryptionTargetMembers();
10951097
const users = members.map((member) => member.userId);
10961098

1097-
if (this.#roomsWithTrackedMembers.has(room.roomId)) {
1098-
void this.#flushOutgoingRequests();
1099-
} else {
1099+
if (!this.#roomsWithTrackedMembers.has(room.roomId)) {
11001100
await this.#trackUsers(users);
1101-
await this.#flushOutgoingRequests();
1101+
await this.#sendTracked(await this.#call('queryKeysForUsers', { users }));
11021102
this.#roomsWithTrackedMembers.add(room.roomId);
11031103
}
11041104

1105+
void this.#flushOutgoingRequests();
1106+
return users;
1107+
}
1108+
1109+
async #encryptEventInner(event: MatrixEvent, room: Room): Promise<void> {
1110+
// The megolm session has to reach every device in the room before the event does.
1111+
const users = await this.#prepareRoomForEncryption(room);
1112+
11051113
await this.#ensureSessionsForUsers(users);
11061114

11071115
const shared = ((await this.#call('shareRoomKey', {
@@ -1493,11 +1501,8 @@ export class EngineCrypto
14931501

14941502
prepareToEncrypt(room: Room): void {
14951503
void this.#serializeForRoom(room.roomId, async () => {
1496-
const members = await room.getEncryptionTargetMembers();
1497-
const users = members.map((member) => member.userId);
1498-
await this.#trackUsers(users);
1504+
const users = await this.#prepareRoomForEncryption(room);
14991505
await this.#ensureSessionsForUsers(users);
1500-
await this.#flushOutgoingRequests();
15011506
}).catch((error: unknown) => engineCryptoLog.warn('general', 'prepareToEncrypt failed', error));
15021507
}
15031508

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
import { afterEach, describe, expect, it, vi } from 'vitest';
2+
import type { MatrixClient, MatrixEvent, Room } from '$types/matrix-sdk';
3+
import { engineInvoke } from '../olmMachine/engineInvoke';
4+
import { EngineCrypto } from './EngineCrypto';
5+
6+
vi.mock('../olmMachine/engineInvoke', () => ({
7+
engineInvoke: vi.fn<(...args: never[]) => Promise<unknown>>(),
8+
}));
9+
10+
const mockInvoke = vi.mocked(engineInvoke);
11+
const identity = { userId: '@me:e.org', deviceId: 'D' };
12+
13+
const room = {
14+
roomId: '!room:e.org',
15+
getEncryptionTargetMembers: async () => [{ userId: '@a:e.org' }],
16+
getHistoryVisibility: () => 'shared',
17+
getBlacklistUnverifiedDevices: () => false,
18+
currentState: { getStateEvents: () => null },
19+
} as unknown as Room;
20+
21+
const event = () =>
22+
({
23+
getType: () => 'm.room.message',
24+
getContent: () => ({ body: 'hello' }),
25+
makeEncrypted: vi.fn<() => void>(),
26+
}) as unknown as MatrixEvent;
27+
28+
describe('keys/query ordering', () => {
29+
afterEach(() => mockInvoke.mockReset());
30+
31+
it('waits for a background query HTTP response and acknowledgement before a first-room query', async () => {
32+
const http = Promise.withResolvers<string>();
33+
const acknowledgement = Promise.withResolvers<void>();
34+
let backgroundAcknowledged = false;
35+
let queries = 0;
36+
const authedRequest = vi.fn<(...args: never[]) => Promise<string>>(async (_method, url) => {
37+
if (url !== '/_matrix/client/v3/keys/query') return '{}';
38+
queries += 1;
39+
if (queries === 1) return http.promise;
40+
return '{}';
41+
});
42+
mockInvoke.mockImplementation(async (_identity, method, args) => {
43+
if (method === 'outgoingRequests')
44+
return backgroundAcknowledged ? [] : [{ id: 'background', type: 1, body: '{}' }];
45+
if (method === 'markRequestAsSent') {
46+
if ((args as { requestId: string }).requestId === 'background') {
47+
await acknowledgement.promise;
48+
backgroundAcknowledged = true;
49+
}
50+
return null;
51+
}
52+
if (method === 'queryKeysForUsers') return { id: 'room', type: 1, body: '{}' };
53+
if (method === 'getMissingSessions') return null;
54+
if (method === 'shareRoomKey') return [];
55+
if (method === 'encryptRoomEvent') return '{}';
56+
if (method === 'identityKeys') return { ed25519: 'ed', curve25519: 'curve' };
57+
return null;
58+
});
59+
const crypto = new EngineCrypto(
60+
{ http: { authedRequest } } as unknown as MatrixClient,
61+
identity
62+
);
63+
64+
crypto.onSyncCompleted({});
65+
const keyQueries = () =>
66+
authedRequest.mock.calls.filter(([, url]) => url === '/_matrix/client/v3/keys/query');
67+
await vi.waitFor(() => expect(keyQueries()).toHaveLength(1));
68+
69+
const send = crypto.encryptEvent(event(), room);
70+
try {
71+
await vi.waitFor(() =>
72+
expect(mockInvoke).toHaveBeenCalledWith(
73+
expect.anything(),
74+
'queryKeysForUsers',
75+
expect.anything()
76+
)
77+
);
78+
await new Promise((resolve) => setTimeout(resolve, 0));
79+
expect(keyQueries()).toHaveLength(1);
80+
81+
http.resolve('{}');
82+
await vi.waitFor(() =>
83+
expect(mockInvoke).toHaveBeenCalledWith(
84+
expect.anything(),
85+
'markRequestAsSent',
86+
expect.anything()
87+
)
88+
);
89+
expect(keyQueries()).toHaveLength(1);
90+
91+
acknowledgement.resolve();
92+
await vi.waitFor(() => expect(keyQueries()).toHaveLength(2));
93+
} finally {
94+
http.resolve('{}');
95+
acknowledgement.resolve();
96+
await send;
97+
}
98+
});
99+
});
Lines changed: 188 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,188 @@
1+
import { afterEach, describe, expect, it, vi } from 'vitest';
2+
import type { MatrixClient, MatrixEvent, Room } from '$types/matrix-sdk';
3+
import { engineInvoke } from '../olmMachine/engineInvoke';
4+
import { EngineCrypto } from './EngineCrypto';
5+
6+
vi.mock('../olmMachine/engineInvoke', () => ({
7+
engineInvoke: vi.fn<(...args: never[]) => Promise<unknown>>(),
8+
}));
9+
10+
const mockInvoke = vi.mocked(engineInvoke);
11+
12+
const room = (roomId: string) =>
13+
({
14+
roomId,
15+
getEncryptionTargetMembers: async () => [{ userId: `@${roomId.slice(1, 2)}:e.org` }],
16+
getHistoryVisibility: () => 'shared',
17+
getBlacklistUnverifiedDevices: () => false,
18+
currentState: { getStateEvents: () => null },
19+
}) as unknown as Room;
20+
21+
const event = (body: string) =>
22+
({
23+
getType: () => 'm.room.message',
24+
getContent: () => ({ body }),
25+
makeEncrypted: vi.fn<() => void>(),
26+
}) as unknown as MatrixEvent;
27+
28+
const cryptoWith = (authedRequest: ReturnType<typeof vi.fn>) =>
29+
new EngineCrypto({ http: { authedRequest } } as unknown as MatrixClient, {
30+
userId: '@me:e.org',
31+
deviceId: 'D',
32+
});
33+
34+
describe('encrypted send concurrency', () => {
35+
afterEach(() => mockInvoke.mockReset());
36+
37+
it('holds same-room sends behind preparation response and acknowledgement', async () => {
38+
const queryResponse = Promise.withResolvers<string>();
39+
const queryAcknowledged = Promise.withResolvers<void>();
40+
const authedRequest = vi.fn<(...args: never[]) => Promise<string>>(async (_method, url) => {
41+
if (url === '/_matrix/client/v3/keys/query') return queryResponse.promise;
42+
return '{}';
43+
});
44+
mockInvoke.mockImplementation(async (_identity, method, args) => {
45+
if (method === 'queryKeysForUsers') return { id: 'query', type: 1, body: '{}' };
46+
if (method === 'markRequestAsSent' && (args as { requestId: string }).requestId === 'query') {
47+
await queryAcknowledged.promise;
48+
}
49+
if (method === 'encryptRoomEvent') return '{}';
50+
if (method === 'identityKeys') return { ed25519: 'ed', curve25519: 'curve' };
51+
return null;
52+
});
53+
54+
const crypto = cryptoWith(authedRequest);
55+
const target = room('!room:e.org');
56+
crypto.prepareToEncrypt(target);
57+
const sends = [
58+
crypto.encryptEvent(event('first'), target),
59+
crypto.encryptEvent(event('second'), target),
60+
];
61+
62+
try {
63+
await vi.waitFor(() => expect(authedRequest).toHaveBeenCalledOnce());
64+
expect(
65+
mockInvoke.mock.calls.filter(([, method]) => method === 'getMissingSessions')
66+
).toHaveLength(0);
67+
68+
queryResponse.resolve('{}');
69+
await vi.waitFor(() =>
70+
expect(mockInvoke.mock.calls).toContainEqual([
71+
expect.anything(),
72+
'markRequestAsSent',
73+
expect.objectContaining({ requestId: 'query' }),
74+
])
75+
);
76+
expect(
77+
mockInvoke.mock.calls.filter(([, method]) => method === 'getMissingSessions')
78+
).toHaveLength(0);
79+
80+
queryAcknowledged.resolve();
81+
await Promise.all(sends);
82+
83+
expect(
84+
mockInvoke.mock.calls.filter(([, method]) => method === 'queryKeysForUsers')
85+
).toHaveLength(1);
86+
expect(
87+
mockInvoke.mock.calls
88+
.filter(([, method]) => method === 'encryptRoomEvent')
89+
.map(([, , args]) => JSON.parse((args as { content: string }).content).body)
90+
).toEqual(['first', 'second']);
91+
} finally {
92+
queryResponse.resolve('{}');
93+
queryAcknowledged.resolve();
94+
await Promise.allSettled(sends);
95+
}
96+
});
97+
98+
it('keeps claims for separate rooms serialized until their response is acknowledged', async () => {
99+
const firstClaimResponse = Promise.withResolvers<string>();
100+
const firstClaimAcknowledged = Promise.withResolvers<void>();
101+
let claimsSent = 0;
102+
let heldClaimId: string | undefined;
103+
let queries = 0;
104+
const authedRequest = vi.fn<(...args: never[]) => Promise<string>>(async (_method, url) => {
105+
if (url === '/_matrix/client/v3/keys/claim' && claimsSent++ === 0) {
106+
return firstClaimResponse.promise;
107+
}
108+
return '{}';
109+
});
110+
mockInvoke.mockImplementation(async (_identity, method, args) => {
111+
if (method === 'queryKeysForUsers') return { id: `query-${queries++}`, type: 1, body: '{}' };
112+
if (method === 'getMissingSessions') {
113+
const user = (args as { users: string[] }).users[0];
114+
return { id: `claim-${user}`, type: 2, body: '{}' };
115+
}
116+
if (method === 'markRequestAsSent') {
117+
const requestId = (args as { requestId: string }).requestId;
118+
if (requestId.startsWith('claim-') && !heldClaimId) {
119+
heldClaimId = requestId;
120+
await firstClaimAcknowledged.promise;
121+
}
122+
}
123+
if (method === 'encryptRoomEvent') return '{}';
124+
if (method === 'identityKeys') return { ed25519: 'ed', curve25519: 'curve' };
125+
return null;
126+
});
127+
128+
const crypto = cryptoWith(authedRequest);
129+
const sends = [
130+
crypto.encryptEvent(event('a'), room('!a:e.org')),
131+
crypto.encryptEvent(event('b'), room('!b:e.org')),
132+
];
133+
134+
try {
135+
await vi.waitFor(() => expect(claimsSent).toBe(1));
136+
expect(
137+
mockInvoke.mock.calls.filter(([, method]) => method === 'getMissingSessions')
138+
).toHaveLength(1);
139+
140+
firstClaimResponse.resolve('{}');
141+
await vi.waitFor(() => expect(heldClaimId).toBeDefined());
142+
expect(
143+
mockInvoke.mock.calls.filter(([, method]) => method === 'getMissingSessions')
144+
).toHaveLength(1);
145+
146+
firstClaimAcknowledged.resolve();
147+
await Promise.all(sends);
148+
expect(
149+
mockInvoke.mock.calls.filter(([, method]) => method === 'getMissingSessions')
150+
).toHaveLength(2);
151+
} finally {
152+
firstClaimResponse.resolve('{}');
153+
firstClaimAcknowledged.resolve();
154+
await Promise.allSettled(sends);
155+
}
156+
});
157+
158+
it('lets a queued send retry after preparation query failure', async () => {
159+
const failedQuery = Promise.withResolvers<string>();
160+
let queries = 0;
161+
const authedRequest = vi.fn<(...args: never[]) => Promise<string>>(async (_method, url) => {
162+
if (url === '/_matrix/client/v3/keys/query' && queries++ === 0) return failedQuery.promise;
163+
return '{}';
164+
});
165+
mockInvoke.mockImplementation(async (_identity, method) => {
166+
if (method === 'queryKeysForUsers') return { id: 'query', type: 1, body: '{}' };
167+
if (method === 'encryptRoomEvent') return '{}';
168+
if (method === 'identityKeys') return { ed25519: 'ed', curve25519: 'curve' };
169+
return null;
170+
});
171+
172+
const crypto = cryptoWith(authedRequest);
173+
const target = room('!room:e.org');
174+
crypto.prepareToEncrypt(target);
175+
let send: Promise<void> | undefined;
176+
try {
177+
await vi.waitFor(() => expect(queries).toBe(1));
178+
send = crypto.encryptEvent(event('after failure'), target);
179+
failedQuery.reject(new Error('query failed'));
180+
181+
await expect(send).resolves.toBeUndefined();
182+
expect(queries).toBe(2);
183+
} finally {
184+
failedQuery.reject(new Error('query failed'));
185+
await send?.catch(() => undefined);
186+
}
187+
});
188+
});

0 commit comments

Comments
 (0)