Skip to content
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.

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

Fix members being loaded from server on initial sync (defeating lazy loading) #3830

Merged
merged 19 commits into from
Nov 3, 2023

Conversation

BillCarsonFr
Copy link
Member

@BillCarsonFr BillCarsonFr commented Oct 24, 2023

Fixes element-hq/element-web#26418
Fixes element-hq/element-web#26439

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)

Here's what your changelog entry will look like:

🐛 Bug Fixes

@@ -1232,10 +1232,6 @@ export class RustCrypto extends TypedEventEmitter<RustCryptoEvents, RustCryptoEv
config,
);
}

// start tracking devices for any users already known to be in this room.
const members = await room.getEncryptionTargetMembers();
Copy link
Member Author

Choose a reason for hiding this comment

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

This is what was triggering the loadMembersIfNeeded. Was called during initial sync when the m.room.encryption was processed.
I moved that code in the RoomEncryptor, it's now only getting the known members from existing state and not loading if neeed

});
}

// Query keys in case we don't have them for newly tracked members.
Copy link
Member Author

@BillCarsonFr BillCarsonFr Oct 24, 2023

Choose a reason for hiding this comment

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

It's not really related to the current problem, but I had to fix it to make the lazy loading test pass on both crypto backend. It's anyhow a real problem, and we can see several occurence of it in rageshakes. The (full) rust sdk as a similar mecanism to ensure the download of user keys for non tracked/dirty users.

This would need to be fixed in mobile too
element-hq/element-web#26439

@@ -670,18 +670,21 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string,
});

it("prepareToEncrypt", async () => {
expectAliceKeyQuery({ device_keys: { "@alice:localhost": {} }, failures: {} });
const homeserverUrl = "https://alice-server.com";
Copy link
Member Author

Choose a reason for hiding this comment

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

I started to modernize a bit the tests by using E2EKeyResponder, because we are requesting keys a bit differently and the existing test were using fragile tactis Alice will check Bob's device list (twice, because reasons).
I haven't done it all because it's a bit strange. In these test the olmTestDevice is sometime an alice device and sometimes a bob device.
If ok, I'd like to clean all this file in a following PR.

@BillCarsonFr BillCarsonFr requested a review from richvdh October 25, 2023 07:56
@BillCarsonFr BillCarsonFr marked this pull request as ready for review October 25, 2023 07:56
@BillCarsonFr BillCarsonFr requested a review from a team as a code owner October 25, 2023 07:56
@richvdh richvdh changed the title Fix members loaded from server on intitial sync (defeating lazy loading) Fix members being loaded from server on initial sync (defeating lazy loading) Oct 25, 2023
src/rust-crypto/RoomEncryptor.ts Outdated Show resolved Hide resolved
src/rust-crypto/RoomEncryptor.ts Outdated Show resolved Hide resolved
src/rust-crypto/RoomEncryptor.ts Outdated Show resolved Hide resolved
src/rust-crypto/RoomEncryptor.ts Outdated Show resolved Hide resolved
src/rust-crypto/RoomEncryptor.ts Outdated Show resolved Hide resolved
spec/integ/crypto/crypto.spec.ts Outdated Show resolved Hide resolved
spec/integ/crypto/crypto.spec.ts Outdated Show resolved Hide resolved
@germain-gg germain-gg removed their request for review October 25, 2023 12:43
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

a few comments on the other bits

src/rust-crypto/RoomEncryptor.ts Outdated Show resolved Hide resolved
spec/unit/rust-crypto/OutgoingRequestsManager.spec.ts Outdated Show resolved Hide resolved
spec/unit/rust-crypto/OutgoingRequestsManager.spec.ts Outdated Show resolved Hide resolved
spec/unit/rust-crypto/OutgoingRequestsManager.spec.ts Outdated Show resolved Hide resolved
spec/unit/rust-crypto/OutgoingRequestsManager.spec.ts Outdated Show resolved Hide resolved
spec/unit/rust-crypto/OutgoingRequestsManager.spec.ts Outdated Show resolved Hide resolved
spec/unit/rust-crypto/OutgoingRequestsManager.spec.ts Outdated Show resolved Hide resolved
src/rust-crypto/OutgoingRequestsManager.ts Outdated Show resolved Hide resolved
src/rust-crypto/rust-crypto.ts Show resolved Hide resolved
src/rust-crypto/OutgoingRequestsManager.ts Outdated Show resolved Hide resolved
src/rust-crypto/OutgoingRequestsManager.ts Outdated Show resolved Hide resolved
src/rust-crypto/OutgoingRequestsManager.ts Outdated Show resolved Hide resolved
src/rust-crypto/OutgoingRequestsManager.ts Outdated Show resolved Hide resolved
src/rust-crypto/OutgoingRequestsManager.ts Outdated Show resolved Hide resolved
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

If you're keen to merge this, go ahead. I'd prefer it to be rewritten without the recursion but won't insist.

@richvdh richvdh added this pull request to the merge queue Nov 3, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 3, 2023
@richvdh richvdh added this pull request to the merge queue Nov 3, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 3, 2023
@richvdh richvdh enabled auto-merge November 3, 2023 12:08
@richvdh richvdh added this pull request to the merge queue Nov 3, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 3, 2023
@richvdh richvdh added this pull request to the merge queue Nov 3, 2023
Merged via the queue into develop with commit 7921fee Nov 3, 2023
21 checks passed
@richvdh richvdh deleted the valere/element-r/fix_lazy_loading branch November 3, 2023 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants