fix(xmtp_mls): enforce MAX_GROUP_SIZE in add_members (by inbox_id) - #4010
fix(xmtp_mls): enforce MAX_GROUP_SIZE in add_members (by inbox_id)#4010teyrebaz33 wants to merge 2 commits into
Conversation
add_members_by_identity already checked member_count + new members against MAX_GROUP_SIZE (250) before queuing the membership update intent. add_members -- the inbox_id-based counterpart used e.g. when adding an already-registered member -- had no such check, so a group already at the 250-member cap could still be grown past the limit through this entry point. Discovered via the ignored-test tracker: test_max_limit_add asserts that adding a 251st member via add_members() returns an error once the group already has 250 members (249 added via add_members_by_identity + the group creator). Running it against a live backend confirmed the bug directly -- the add succeeded and the group ended up at 251 members instead of erroring. Fix mirrors the existing add_members_by_identity guard: compute current member_count via self.members(), and return GroupError::UserLimitExceeded before building the membership update intent if member_count + new_ids.len() > MAX_GROUP_SIZE. Un-ignores test_max_limit_add. Testing: - cargo test -p xmtp_mls --release --lib groups::tests::test_max_limit_add -- previously failed (add succeeded when it should have errored), now passes. - Ran the full groups::tests suite multiple times with and without this change. A handful of unrelated tests (test_commit_log_remote, test_failed_installations, test_dm_creation_with_user_two_installations_one_malformed) intermittently fail when run as part of the full ~720-test suite in parallel against the local backend, but each passes reliably when run in isolation, and the specific set of failures changes between runs -- consistent with resource contention against the shared local backend under parallel load, not a regression from this change. Verified by running the same subset against unmodified main and seeing the same intermittent behavior. - cargo clippy -p xmtp_mls --lib -- -D warnings: clean.
…tention test_max_limit_add creates 250 real MLS clients to exercise the MAX_GROUP_SIZE boundary. On native this runs reliably in ~15s. On wasm, CI's parallel test-wasm job runs it alongside ~880 other tests sharing one local backend + browser IndexedDB. Under that contention the test consistently took 83-85s across all 4 nextest retries and failed -- comfortably over the wasm-specific slow-timeout (60s, see .config/nextest.toml). Running the test in isolation on wasm (just wasm test-v3 -- test_max_limit_add) passes reliably in ~13-27s, confirming this is contention-driven, not a regression from the MAX_GROUP_SIZE fix itself (previously verified extensively on native, see prior commit). Uses the #[cfg_attr(target_arch = "wasm32", ignore)] pattern already established elsewhere in this file (e.g. add_missing_installs_reentrancy) for tests that are unsuited to the parallel wasm CI profile. Native coverage remains full and is sufficient to guard this regression. Testing: - cargo test -p xmtp_mls --release --lib groups::tests::test_max_limit_add -- still passes on native. - just wasm test-v3 -- test_max_limit_add --nocapture -- passes in isolation on wasm (~13-27s), confirming the wasm CI failure was purely a parallel-suite timing issue, not a logic bug. - cargo clippy -p xmtp_mls --lib -- -D warnings: clean.
|
Pushed a follow-up commit: |
|
The |
1 participant
Fixes #4009
Problem
Group::add_members_by_identityenforcesMAX_GROUP_SIZE(250) before queuing a membership update intent.Group::add_members-- the inbox_id-based counterpart -- had no equivalent check, so a group already at the 250-member cap could still be grown past the limit through this entry point.Fix
Mirrors the existing
add_members_by_identityguard: computemember_countviaself.members(), returnGroupError::UserLimitExceededbefore building the intent ifmember_count + new_ids.len() > MAX_GROUP_SIZE. Un-ignorestest_max_limit_add, which was disabled pending this fix.Testing
cargo test -p xmtp_mls --release --lib groups::tests::test_max_limit_add-- previously failed (add succeeded when it should have errored, group ended up with 251 members), now passes.groups::testssuite (~720 tests) multiple times, with and without this change. A handful of unrelated tests intermittently fail when run as part of the full suite in parallel against the local backend (test_commit_log_remote::*,test_failed_installations::*,test_dm_creation_with_user_two_installations_one_malformed), but each passes reliably in isolation, and the specific failing set changes between runs. Verified the same intermittent behavior exists on unmodifiedmain-- this is resource contention against the shared local backend under parallel load, not a regression from this change.cargo clippy -p xmtp_mls --lib -- -D warnings: clean.Note on process
I used AI assistance (Claude) to investigate the codebase and draft this fix, disclosed per your AI-contributions policy. I reviewed and understand the change, and independently verified the bug against a live backend before writing the fix.
Note
Enforce MAX_GROUP_SIZE limit in
add_membersbefore building membership update intentGroupError::UserLimitExceededif adding the requested members would exceedMAX_GROUP_SIZE.test_max_limit_addin tests/mod.rs to run on native targets by replacing#[ignore]with#[cfg_attr(target_arch = "wasm32", ignore)].Macroscope summarized 0766854.