From b74dbdea4a6b7e029b9b656880ce1528dcea0daa Mon Sep 17 00:00:00 2001 From: Nicolas Werner Date: Mon, 7 Oct 2024 19:00:58 +0200 Subject: [PATCH 1/6] fix: Don't wait for 5 milliseconds on every sync in our tests This was added in 77be6102f6cbb2e01adc28f9caa3aa583f914235 without much documentation. I am pretty sure the intent was never to slow down every test by 5 seconds, so let's get rid of it and do more careful delays where it is useful (like specific sync requests for example). Makes the tests run 5 times faster (the whole suite!) on my device. --- lib/fake_matrix_api.dart | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/fake_matrix_api.dart b/lib/fake_matrix_api.dart index c78d1aae..d6951ea1 100644 --- a/lib/fake_matrix_api.dart +++ b/lib/fake_matrix_api.dart @@ -127,10 +127,6 @@ class FakeMatrixApi extends BaseClient { //print('\$method request to $action with Data: $data'); - // Sync requests with timeout - if (data is Map && data['timeout'] is String) { - await Future.delayed(Duration(seconds: 5)); - } if (!servers.contains(request.url.origin)) { return Response( 'Not found ${request.url.origin}...', @@ -202,6 +198,11 @@ class FakeMatrixApi extends BaseClient { '/client/v3/rooms/!1234%3AfakeServer.notExisting/state/')) { res = {'event_id': '\$event${_eventCounter++}'}; } else if (action.contains('/client/v3/sync')) { + // Sync requests with timeout + final timeout = request.url.queryParameters['timeout']; + if (timeout != null && timeout != '0') { + await Future.delayed(Duration(milliseconds: 50)); + } res = { 'next_batch': DateTime.now().millisecondsSinceEpoch.toString(), }; From 583be5ece79711b85e3f5ba301282ac7601ff5d1 Mon Sep 17 00:00:00 2001 From: Nicolas Werner Date: Mon, 7 Oct 2024 19:03:33 +0200 Subject: [PATCH 2/6] fix: by default don't uplaod new keys in our tests This reduces CPU load of the uploadKeys callback by a lot, since we don't upload new keys on every empty sync in the tests. --- lib/fake_matrix_api.dart | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/fake_matrix_api.dart b/lib/fake_matrix_api.dart index d6951ea1..8d96a4b3 100644 --- a/lib/fake_matrix_api.dart +++ b/lib/fake_matrix_api.dart @@ -204,7 +204,13 @@ class FakeMatrixApi extends BaseClient { await Future.delayed(Duration(milliseconds: 50)); } res = { - 'next_batch': DateTime.now().millisecondsSinceEpoch.toString(), + // So that it is clear which sync we are processing prefix it with 'empty_' + 'next_batch': 'empty_${DateTime.now().millisecondsSinceEpoch}', + // ensure we don't generate new keys for no reason + 'device_one_time_keys_count': { + 'curve25519': 10, + 'signed_curve25519': 100 + }, }; } else if (method == 'PUT' && _client != null && @@ -1038,7 +1044,7 @@ class FakeMatrixApi extends BaseClient { '@bob:example.com', ], }, - 'device_one_time_keys_count': {'curve25519': 10, 'signed_curve25519': 20}, + 'device_one_time_keys_count': {'curve25519': 10, 'signed_curve25519': 100}, }; static Map archiveSyncResponse = { From 6a28ab05d0d66049747573e7695abced7a777cac Mon Sep 17 00:00:00 2001 From: Nicolas Werner Date: Mon, 7 Oct 2024 19:05:54 +0200 Subject: [PATCH 3/6] fix: Deduplicate key OTK uploads We do the key upload asynchronously without awaiting it. This means we may do multiple syncs before the key upload finishes. So we may generate more keys than we should. To fix that prevent multiple key uploads from running at once. This may lead to outdated key uploads in some cases if we miss an OTK being used in sync. However, the next sync will still tell us about that so in the worst case this might delay key uploads by 30s (with the default sync timeout), which for normal usage should be completely acceptable. --- lib/encryption/olm_manager.dart | 64 ++++++++++++++++++--------------- 1 file changed, 36 insertions(+), 28 deletions(-) diff --git a/lib/encryption/olm_manager.dart b/lib/encryption/olm_manager.dart index d44116d3..37bce332 100644 --- a/lib/encryption/olm_manager.dart +++ b/lib/encryption/olm_manager.dart @@ -28,6 +28,7 @@ import 'package:matrix/encryption/utils/json_signature_check_extension.dart'; import 'package:matrix/encryption/utils/olm_session.dart'; import 'package:matrix/matrix.dart'; import 'package:matrix/msc_extensions/msc_3814_dehydrated_devices/api.dart'; +import 'package:matrix/src/utils/run_benchmarked.dart'; import 'package:matrix/src/utils/run_in_root.dart'; class OlmManager { @@ -326,43 +327,50 @@ class OlmManager { return false; } + final _otkUpdateDedup = AsyncCache.ephemeral(); + Future handleDeviceOneTimeKeysCount( Map? countJson, List? unusedFallbackKeyTypes) async { if (!enabled) { return; } - final haveFallbackKeys = encryption.isMinOlmVersion(3, 2, 0); - // Check if there are at least half of max_number_of_one_time_keys left on the server - // and generate and upload more if not. - // If the server did not send us a count, assume it is 0 - final keyCount = countJson?.tryGet('signed_curve25519') ?? 0; + await _otkUpdateDedup.fetch(() => + runBenchmarked('handleOtkUpdate', () async { + final haveFallbackKeys = encryption.isMinOlmVersion(3, 2, 0); + // Check if there are at least half of max_number_of_one_time_keys left on the server + // and generate and upload more if not. - // If the server does not support fallback keys, it will not tell us about them. - // If the server supports them but has no key, upload a new one. - var unusedFallbackKey = true; - if (unusedFallbackKeyTypes?.contains('signed_curve25519') == false) { - unusedFallbackKey = false; - } + // If the server did not send us a count, assume it is 0 + final keyCount = countJson?.tryGet('signed_curve25519') ?? 0; - // fixup accidental too many uploads. We delete only one of them so that the server has time to update the counts and because we will get rate limited anyway. - if (keyCount > _olmAccount!.max_number_of_one_time_keys()) { - final requestingKeysFrom = { - client.userID!: {ourDeviceId!: 'signed_curve25519'} - }; - await client.claimKeys(requestingKeysFrom, timeout: 10000); - } + // If the server does not support fallback keys, it will not tell us about them. + // If the server supports them but has no key, upload a new one. + var unusedFallbackKey = true; + if (unusedFallbackKeyTypes?.contains('signed_curve25519') == false) { + unusedFallbackKey = false; + } - // Only upload keys if they are less than half of the max or we have no unused fallback key - if (keyCount < (_olmAccount!.max_number_of_one_time_keys() / 2) || - !unusedFallbackKey) { - await uploadKeys( - oldKeyCount: keyCount < (_olmAccount!.max_number_of_one_time_keys() / 2) - ? keyCount - : null, - unusedFallbackKey: haveFallbackKeys ? unusedFallbackKey : null, - ); - } + // fixup accidental too many uploads. We delete only one of them so that the server has time to update the counts and because we will get rate limited anyway. + if (keyCount > _olmAccount!.max_number_of_one_time_keys()) { + final requestingKeysFrom = { + client.userID!: {ourDeviceId!: 'signed_curve25519'} + }; + await client.claimKeys(requestingKeysFrom, timeout: 10000); + } + + // Only upload keys if they are less than half of the max or we have no unused fallback key + if (keyCount < (_olmAccount!.max_number_of_one_time_keys() / 2) || + !unusedFallbackKey) { + await uploadKeys( + oldKeyCount: + keyCount < (_olmAccount!.max_number_of_one_time_keys() / 2) + ? keyCount + : null, + unusedFallbackKey: haveFallbackKeys ? unusedFallbackKey : null, + ); + } + })); } Future storeOlmSession(OlmSession session) async { From ec36d9c8fe095525e76aca5078e7cf2443fb6f65 Mon Sep 17 00:00:00 2001 From: Nicolas Werner Date: Mon, 7 Oct 2024 20:37:22 +0200 Subject: [PATCH 4/6] fix: Race conditions in tests now that they are running faster --- test/calls_test.dart | 1 + test/client_test.dart | 26 ++++++++++++++++------ test/device_keys_list_test.dart | 1 + test/encryption/cross_signing_test.dart | 1 + test/encryption/key_request_test.dart | 2 ++ test/encryption/key_verification_test.dart | 5 +++++ test/fake_client.dart | 1 + test/room_test.dart | 1 + test/timeline_test.dart | 1 + test/user_test.dart | 1 + 10 files changed, 33 insertions(+), 7 deletions(-) diff --git a/test/calls_test.dart b/test/calls_test.dart index 7ecbace7..2e3c4372 100644 --- a/test/calls_test.dart +++ b/test/calls_test.dart @@ -16,6 +16,7 @@ void main() { Logs().level = Level.info; setUp(() async { matrix = await getClient(); + await matrix.abortSync(); voip = VoIP(matrix, MockWebRTCDelegate()); VoIP.customTxid = '1234'; diff --git a/test/client_test.dart b/test/client_test.dart index 461f43c7..55445635 100644 --- a/test/client_test.dart +++ b/test/client_test.dart @@ -45,11 +45,13 @@ void main() { final dbPath = join(Directory.current.path, 'test.sqlite'); setUp(() async { - expect(await File(dbPath).exists(), false); + expect(await File(dbPath).exists(), false, + reason: '$dbPath should not exist'); clientOnPath = await getClient( databasePath: dbPath, ); - expect(await File(dbPath).exists(), true); + await clientOnPath.abortSync(); + expect(await File(dbPath).exists(), true, reason: '$dbPath should exist'); }); test('logout', () async { expect(await File(dbPath).exists(), true); @@ -137,11 +139,12 @@ void main() { newOlmAccount: pickledOlmAccount, ); - await Future.delayed(Duration(milliseconds: 50)); - final loginState = await loginStateFuture; final sync = await syncFuture; + // to ensure our state doesn't get overwritten once we manually inject SyncUpdates + await matrix.abortSync(); + expect(loginState, LoginState.loggedIn); expect(matrix.onSync.value != null, true); expect(matrix.encryptionEnabled, true); @@ -185,7 +188,7 @@ void main() { PresenceType.online); expect(presenceCounter, 1); expect(accountDataCounter, 10); - await Future.delayed(Duration(milliseconds: 50)); + expect(matrix.userDeviceKeys.keys.toSet(), { '@alice:example.com', '@othertest:fakeServer.notExisting', @@ -1134,7 +1137,7 @@ void main() { newOlmAccount: pickledOlmAccount, ); - await Future.delayed(Duration(milliseconds: 500)); + await client1.abortSync(); expect(client1.isLogged(), true); expect(client1.rooms.length, 3); @@ -1146,7 +1149,8 @@ void main() { ); await client2.init(); - await Future.delayed(Duration(milliseconds: 500)); + + await client2.abortSync(); expect(client2.isLogged(), true); expect(client2.accessToken, client1.accessToken); @@ -1197,6 +1201,7 @@ void main() { await client.getWellknown(); expect( client.wellKnown?.mHomeserver.baseUrl.host, 'fakeserver.notexisting'); + await client.dispose(); }); test('refreshAccessToken', () async { @@ -1204,6 +1209,7 @@ void main() { expect(client.accessToken, 'abcd'); await client.refreshAccessToken(); expect(client.accessToken, 'a_new_token'); + await client.dispose(); }); test('handleSoftLogout', () async { @@ -1226,6 +1232,7 @@ void main() { storedClient?.tryGet('refresh_token'), 'another_new_token', ); + await client.dispose(); }); test('object equality', () async { @@ -1274,6 +1281,7 @@ void main() { final client = await getClient(); client.backgroundSync = true; await client.clearCache(); + await client.dispose(); }); test('dispose', () async { @@ -1311,6 +1319,7 @@ void main() { await hiveClient.init(); await Future.delayed(Duration(milliseconds: 200)); expect(hiveClient.isLogged(), true); + await hiveClient.dispose(closeDatabase: false); }); test('getEventByPushNotification', () async { @@ -1402,6 +1411,8 @@ void main() { null, true, reason: '!5345234235:example.com not found as archived room'); + + await client.dispose(); }); test( @@ -1430,6 +1441,7 @@ void main() { expect(error.userId, '@user:server'); expect(error.toString(), 'Exception: BAD_ACCOUNT_KEY'); } + await customClient.dispose(closeDatabase: true); }, ); diff --git a/test/device_keys_list_test.dart b/test/device_keys_list_test.dart index c0663d7b..3332aa74 100644 --- a/test/device_keys_list_test.dart +++ b/test/device_keys_list_test.dart @@ -32,6 +32,7 @@ void main() { test('setupClient', () async { client = await getClient(); + await client.abortSync(); }); test('fromJson', () async { diff --git a/test/encryption/cross_signing_test.dart b/test/encryption/cross_signing_test.dart index 8ac7178b..43592edd 100644 --- a/test/encryption/cross_signing_test.dart +++ b/test/encryption/cross_signing_test.dart @@ -34,6 +34,7 @@ void main() { await olm.init(); olm.get_library_version(); client = await getClient(); + await client.abortSync(); }); test('basic things', () async { diff --git a/test/encryption/key_request_test.dart b/test/encryption/key_request_test.dart index 307aee7b..4fe24e66 100644 --- a/test/encryption/key_request_test.dart +++ b/test/encryption/key_request_test.dart @@ -76,6 +76,8 @@ void main() { }); test('Reply To Request', () async { final matrix = await getClient(); + await matrix.abortSync(); + matrix.setUserId('@alice:example.com'); // we need to pretend to be alice FakeMatrixApi.calledEndpoints.clear(); await matrix diff --git a/test/encryption/key_verification_test.dart b/test/encryption/key_verification_test.dart index 8bfdea0a..76676203 100644 --- a/test/encryption/key_verification_test.dart +++ b/test/encryption/key_verification_test.dart @@ -95,8 +95,13 @@ void main() async { KeyVerificationMethod.reciprocate }; + // cancel sync to reduce background load and prevent sync overwriting which keys are tracked. + await client1.abortSync(); + await client2.abortSync(); + // get client2 device keys to start verification await client1.updateUserDeviceKeys(additionalUsers: {client2.userID!}); + await client2.updateUserDeviceKeys(additionalUsers: {client1.userID!}); }); tearDown(() async { diff --git a/test/fake_client.dart b/test/fake_client.dart index 42911fe5..fcf46da4 100644 --- a/test/fake_client.dart +++ b/test/fake_client.dart @@ -53,6 +53,7 @@ Future getClient({ newOlmAccount: pickledOlmAccount, ); await Future.delayed(Duration(milliseconds: 10)); + await client.abortSync(); return client; } diff --git a/test/room_test.dart b/test/room_test.dart index 3e08e2cc..3854dcb1 100644 --- a/test/room_test.dart +++ b/test/room_test.dart @@ -55,6 +55,7 @@ void main() { Logs().level = Level.error; test('Login', () async { matrix = await getClient(); + await matrix.abortSync(); }); test('Create from json', () async { diff --git a/test/timeline_test.dart b/test/timeline_test.dart index 5421dcf7..e87a1144 100644 --- a/test/timeline_test.dart +++ b/test/timeline_test.dart @@ -67,6 +67,7 @@ void main() { client = await getClient( sendTimelineEventTimeout: const Duration(seconds: 5), ); + await client.abortSync(); final poison = Random().nextInt(2 ^ 32); currentPoison = poison; diff --git a/test/user_test.dart b/test/user_test.dart index 309cacb4..85d51f90 100644 --- a/test/user_test.dart +++ b/test/user_test.dart @@ -48,6 +48,7 @@ void main() { await client.login(LoginType.mLoginPassword, identifier: AuthenticationUserIdentifier(user: 'test'), password: '1234'); + await client.abortSync(); }); tearDown(() async { await client.logout(); From e32a6ac0371f4aa8cae2dcf122762f5ca2b5c3df Mon Sep 17 00:00:00 2001 From: Nicolas Werner Date: Mon, 7 Oct 2024 21:56:49 +0200 Subject: [PATCH 5/6] chore: bump dart version to fix tests not exiting sometimes --- .github/workflows/versions.env | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/versions.env b/.github/workflows/versions.env index a78fec4b..a1149f57 100644 --- a/.github/workflows/versions.env +++ b/.github/workflows/versions.env @@ -1,2 +1,2 @@ flutter_version=3.22.2 -dart_version=3.4.3 +dart_version=3.5.3 From a6e8e40c99dbd748a6e4df4d95bf97cbf6cecc3c Mon Sep 17 00:00:00 2001 From: Nicolas Werner Date: Tue, 8 Oct 2024 14:41:36 +0200 Subject: [PATCH 6/6] chore: Switch to cheaper github runner Might also fix our weird test hangs --- .github/workflows/app.yml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/workflows/app.yml b/.github/workflows/app.yml index 5593a080..91e5529d 100644 --- a/.github/workflows/app.yml +++ b/.github/workflows/app.yml @@ -50,14 +50,15 @@ jobs: retention-days: 1 coverage: - runs-on: arm-ubuntu-latest-16core + #runs-on: arm-ubuntu-latest-16core + runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - run: cat .github/workflows/versions.env >> $GITHUB_ENV - uses: dart-lang/setup-dart@a57a6c04cf7d4840e88432aad6281d1e125f0d46 with: sdk: ${{ env.dart_version }} - architecture: "arm64" + #architecture: "arm64" - name: Run tests run: | sudo apt-get update && sudo apt-get install --no-install-recommends --no-install-suggests -y lcov libsqlite3-0 libsqlite3-dev libolm3 libssl3