test(send): pin that a warm group send encodes nothing per recipient - #1281
Merged
Conversation
Profiling a client reported the binary encoder growing with group size -- `classify_string_hint`, `write_node`, and `node_encoded_size_with_cache` all rising from 8 to 512 members -- and named the recipient list as the thing being serialized on every message. On the warm path it is not, and nothing in the tree said so, so the claim was neither confirmable nor falsifiable without re-reading `prepare_group_stanza` end to end. A warm send -- no sender-key distribution, which is what a group in ordinary conversation does for every message between topology changes -- takes the `distribution_list == None` path: no `<participants>` fan-out is built, the phash is served from its memo as a fixed-width hash, and `stale_users_for` returns empty without walking anything. What reaches the wire is one `<enc type="skmsg">` for the whole group plus a reporting token, and neither knows the member count. These tests hold that shut at 8, 32, 128 and 512 members: the marshalled stanza is byte-identical across all four once the skmsg ciphertext is discounted (it cannot be compared directly -- `pad_with_context_from_encoded` appends a random 1..16-byte pad by design), plus the phash is present and fixed-width at every size. The session/identity/prekey stores are `unreachable!()` rather than stubs: a warm send reaching for a pairwise session is the regression this is here to catch, and it should fail loudly instead of being absorbed. What grows is the *distributing* send, and inherently: each device needs its own copy of the sender key under its own ratcheting session, so `<participants>` carries one pairwise `<enc>` per target and there is no cache to add -- a stale participant list is a message delivered to the wrong device, not a slow one. `mark_full_distribution_list` already covers that side. No production code changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019JXjxfWyfvxB6WEgiCLVF3
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds warm group-send tests. The tests use sender-key-only state, reject pairwise-store access, verify omitted recipient fan-out, compare serialized sizes across group sizes, and validate ChangesWarm group-send serialization coverage
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Filename | Overview |
|---|---|
| wacore/src/send/tests.rs | Adds isolated group-send encoding-scale fixtures and assertions without changing production behavior. |
Reviews (2): Last reviewed commit: "test(send): warm the phash memo in the f..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91696f46be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+5319
to
+5320
| let group_info = GroupInfo::new(members.clone(), AddressingMode::Pn); | ||
| let resolved = std::sync::Arc::new(ResolvedGroupDevices::new(members)); |
There was a problem hiding this comment.
Each helper invocation constructs a fresh ResolvedGroupDevices and immediately passes it to prepare_group_stanza, so its OnceLock is empty and phash() performs the O(member_count) computation during the supposedly warm send. Production repeat sends—and the existing setup_group_send benchmark—reuse an already-populated memo, so this fixture neither exercises that path nor catches a regression that recomputes the phash on every send; call resolved.phash(&own) during setup before preparing the stanza.
Useful? React with 👍 / 👎.
The helper built a fresh `ResolvedGroupDevices` and handed it straight to `prepare_group_stanza`, so its `OnceLock` was empty and the first send paid the O(member_count) hash inside the path the test calls warm. That measured the cold path under a warm name, and it left the regression it should catch -- recomputing the phash on every send -- invisible, since a cold memo looks the same as no memo. `setup_group_send` in the benchmark already warms and asserts it; this matches. The size assertions were unaffected either way (the phash is fixed-width), but the fixture now exercises what it claims to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019JXjxfWyfvxB6WEgiCLVF3
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
📦 Binary size report
.text per crate
Baseline: |
jlucaso1
pushed a commit
that referenced
this pull request
Aug 11, 2026
Brings in #1281, whose `warm_group_send_encoding_scale` this section cites. The reference dangled while this branch sat on a pre-merge base.
jlucaso1
pushed a commit
that referenced
this pull request
Aug 11, 2026
Three more from review, all confirmed against the fixtures. The cited regression test was not in this branch's tree. `perf/group-allocs` was cut before #1281 landed, so `warm_group_send_encoding_scale` existed only on main and the reference dangled for anyone reading the PR. Merged main in. The distributing row is a first-message fan-out, not a steady rotation. `establish_session` runs `process_prekey_bundle` alone -- unlike `establish_bidirectional` it never completes the round trip -- so every session still carries its `pending_pre_key` and each SKDM encryption emits a `pkmsg`, with prekey wrapping and device-identity serialization attached. 6,816 is that shape. A later rotation or reset over acknowledged sessions emits plain `SignalMessage`s and costs less; nothing here measures it. Labelled in the table and in the prose, replacing a paragraph that implied the figure covered redistribution generally. The extrapolation was in the wrong unit. `setup_group_send(n)` creates exactly one device per member, while SKDM fan-out scales with resolved devices -- a real group resolves to more targets than members. The ~3.4K figure is now stated per 128 *targets*, and the comparison notes that the external number is quoted in members whose device count is unknown, which is one more reason the decomposition is not claimable from here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019JXjxfWyfvxB6WEgiCLVF3
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Profiling a client reported the binary encoder growing with group size —
classify_string_hint+6.5K Ir/msg,Encoder::write_node+4.8K,node_encoded_size_with_cache+5.2K going from 8 to 512 members — and named the recipient list as the thing being serialized on every message.GcmGhash::updatecalls rising 60 → 217 while no key operation moved was read as confirmation: more bytes authenticated, same crypto.On the warm path that is not what happens, and this closes the target as a refutation. The warm group stanza is 133 bytes at 8 members and 133 bytes at 512. What grows is the distributing send, and it grows for a reason no cache can remove.
The tests are the deliverable: the property was true but unstated, so the claim was neither confirmable nor falsifiable without re-reading
prepare_group_stanzaend to end — and nothing stopped a future change from making it false.Changes
wacore/src/send/tests.rswarm_group_send_encoding_scalemodule — two tests, +301 linesNo production code changes.
Cost
Nothing to trade: this PR adds tests only. The measurements are what closes the target.
Encoded stanza size, warm send —
marshal(stanza).len()minus the skmsg ciphertext. That payload cannot be compared directly:pad_with_context_from_encodedappends a random 1..16-byte pad by design, so two encodes of the same message differ in length on purpose. Everything else in the stanza is deterministic, and everything else is what "does the recipient list reach the wire" actually asks.Zero bytes per recipient, across a 64× range. Allocation counts are unchanged; no allocating code is touched.
Why it is flat. A warm send takes the
distribution_list == Nonepath inprepare_group_stanza, so:<participants>fan-out is built — that node lives inside the distribution branch;ResolvedGroupDevicesmemo (perf(send): memoize the group phash on the device-list memo entry #840) as a fixed-width hash, not recomputed per send;stale_users_forreturns empty without walking anything.What reaches the wire is one
<enc type="skmsg">for the whole group plus a reporting token. Neither knows the member count.Why the distributing send is inherently linear. Each device needs its own copy of the sender key under its own ratcheting Signal session, so
<participants>carries one pairwise<enc>per target by construction. There is no cache to add: a stale participant list is a message delivered to a device that should not have it, or withheld from one that should — a correctness bug wearing a performance change's clothes.mark_full_distribution_listalready covers that side.Checked and not changed
node_encoded_size_with_cacheis not a cross-message cache, so there is no miss to fix. The name invites the reading that a cache is thrashing. It is the sizing pass of a two-pass encoder, and itsStringHintCacheis a replay tape: the plan pass classifies each string once and appends the hint, the write pass consumes them in the samewrite_nodeorder (debug-asserted, withfully_consumed()checked after). It is scoped to a single encode and never outlives one. Nothing to bound, invalidate, or grow.resolve_skdm_targets_memoizeduntouched — its semantics are correctness, not performance.bench_group_send_*was rejected as the instrument. Divan regenerates inputs per iteration andsetup_group_send(N)establishes N Signal sessions, so the measurement comes out at ~54M Ir/iter against a ~45 µs send — the fixture is inherently O(N) and swamps the thing under test by two orders of magnitude. Encoded stanza size is deterministic and measures the actual question.Validation
cargo test -p wacore --lib warm_group_send_encoding_scale— 2 passed, 0 failed.cargo fmt --all,cargo clippy -p wacore --all-targets— clean.unreachable!()rather than stubs, on purpose: a warm send reaching for a pairwise session is precisely the regression these tests exist to catch, and it should fail loudly instead of being quietly absorbed by a stub answer.