perf(signal): lease outbound counters in batches instead of flushing every send - #1026
Merged
Conversation
…every send Every send used to serialize the whole SessionRecord and commit a storage transaction before the stanza could hit the wire, because outbound message keys/IVs derive deterministically from the sender-chain counter and a crash between wire-write and persistence would re-derive a spent (key, IV) pair. At 100k messages that was ~229 MB of protobuf encoding and one SQLite transaction per message on the hot path. The durable requirement is not the ratchet state itself — it is the promise that no counter ever repeats. That promise can be bought in batches: the record now leases SENDER_CHAIN_RESERVATION_BATCH counters ahead (reservedSenderChainIndex, a record-level field hand-encoded next to the RecordStructure fields, since the whatspec proto cannot carry local fields), and deserialize fast-forwards the loaded chain past the whole lease, making every possibly-spent counter underivable after a reload. Send-path effect: only the send that raises the lease (or advances a group sender-key chain, which has no lease yet) still flushes synchronously before the wire; every lease-covered send just schedules the same coalesced write-behind the receive path uses. Steady-state ping-pong (counter 0 of a freshly ratcheted chain every time) never re-raises the lease, so the per-message serialize + transaction disappears entirely from that profile. Safety interleavings covered by tests: - crash/reload mid-lease resumes past the ceiling; the peer decrypts across the burned gap (bounded, well under MAX_FORWARD_JUMPS) - a DH ratchet whose new chain never reached storage is unrecoverable by construction (fresh random ephemeral), and the reloaded old chain resumes past its own lease - archived-state promotion burns the lease into the promoted chain (promote_state); freshly ratcheted states reset it (promote_fresh_state) - a raised lease gates the wire until a flush SUCCEEDS (failed flushes keep the gate closed; checked-out sessions stay gated across a flush) - group sender-key encrypts still gate the wire, but group decrypt dirtiness no longer forces a sync flush onto unrelated DM sends - legacy records (no lease field) load with a zero reservation; the encoding round-trips and rejects implausible (corrupt) reservations Old readers skip the unknown record field, but a downgrade after a crash would ignore the lease — release notes should flag that.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughOutbound Signal sessions now reserve sender-chain counter leases, persist lease metadata, and fast-forward after reloads. Session and sender-key cache entries track pending wire gates. Send paths synchronously persist required gates and coalesce other flushes. ChangesSender-chain durability leases
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SignalEncrypt
participant Client
participant SignalStoreCache
participant Backend
SignalEncrypt->>Client: persist_signal_state_pre_wire
Client->>SignalStoreCache: needs_pre_wire_flush
alt Gate pending
Client->>SignalStoreCache: flush_signal_cache_batch_safe
SignalStoreCache->>Backend: Persist session or sender-key state
else Lease-covered advance
Client->>Client: schedule_signal_flush
end
Client-->>SignalEncrypt: Continue outbound transmission
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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 |
|---|---|
| src/client/adapters.rs | Adds the pre-wire durability decision between synchronous flushing and coalesced persistence. |
| src/send/mod.rs | Routes message and status sends through the new durability gate. |
| wacore/src/store/signal_cache.rs | Tracks pending session reservations and outbound sender-key gates across cache flushes. |
| wacore/libsignal/src/protocol/state/session.rs | Serializes counter reservations and fast-forwards restored or promoted sender chains. |
| wacore/libsignal/src/protocol/session_cipher.rs | Extends the sender-counter reservation when encryption reaches the current limit. |
| wacore/libsignal/src/protocol/group_cipher.rs | Marks outbound group sender-key advances for synchronous pre-wire persistence. |
| tests/e2e/tests/session_reuse.rs | Updates end-to-end coverage for leased counters and storage failure behavior. |
Reviews (3): Last reviewed commit: "fix(signal): fail closed on an unreadabl..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 13 files
Confidence score: 1/5
wacore/libsignal/src/protocol/session_cipher.rshas a high-confidence downgrade path where a crash plus rollback can reuse a sender-chain counter because older readers ignore the lease field, which can cause cryptographic state reuse and message security/integrity regressions if merged as-is. Add a downgrade-safe migration or explicit rejection of stale snapshots on downgrade (and verify with a crash→downgrade test) before merging.
You’re at about 98% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="wacore/libsignal/src/protocol/session_cipher.rs">
<violation number="1" location="wacore/libsignal/src/protocol/session_cipher.rs:249">
P0: A crash followed by library downgrade can reuse a sender-chain counter: older readers ignore the lease field and resume from the stale chain snapshot. Add a downgrade-safe migration/rejection mechanism (or block this format from being opened by older builds) before emitting leased records.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wacore/src/store/signal_cache.rs`:
- Around line 2043-2067: Add a sender-key failed-flush test alongside
only_encrypt_marked_sender_keys_gate_the_wire that marks an outbound
SenderKeyRecord as wire-gated, uses a backend configured to fail the sender-key
write, asserts flush returns an error and needs_pre_wire_flush() remains true,
then verifies a subsequent successful flush clears the gate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d4d275e4-bd20-4c2d-9082-9610922f03dc
📒 Files selected for processing (13)
src/client/adapters.rssrc/features/signal.rssrc/send/mod.rssrc/signal_flush.rstests/e2e/tests/session_reuse.rswacore/libsignal/src/protocol/consts.rswacore/libsignal/src/protocol/group_cipher.rswacore/libsignal/src/protocol/sender_keys.rswacore/libsignal/src/protocol/session.rswacore/libsignal/src/protocol/session_cipher.rswacore/libsignal/src/protocol/state/session.rswacore/libsignal/tests/counter_lease.rswacore/src/store/signal_cache.rs
Mirrors the session-lease failed-flush test: a flush that errors writing the chain advance must leave the gate closed. Adds the matching fail_sender_key_writes hook to the in-memory backend.
…ate's edges Review follow-ups on the counter-lease PR. Real fix — decode_reserved_index skipped a field-100 tag it could not read (wrong wire type), and let a duplicate win under last-one-wins. Both paths silently yield reservation 0, which disables the load-time fast-forward and re-enables every counter the lease had already spent: fail-open in exactly the place the mechanism exists to prevent reuse. Both now fail the load, which is recoverable where a reused (key, IV) pair is not. Documented, not changed: - clear() dropping a pending gate is safe only because no clear can race a live wire: every caller runs before the transport exists (connect) or after cleanup_connection_state took the noise socket, and a send resolves that socket after its pre-wire gate — so a send that sees the cleared set fails NotConnected instead of reaching a peer. That ordering is load-bearing and was undocumented; moving a clear ahead of the socket teardown would reintroduce reuse for lease-raising sends. - the pre-wire gate is a global predicate, so another session's unpersisted lease can still make this send flush (and its failure abort this send). Erring toward an extra flush is the safe direction; scoping it to the stanza's addresses would need them threaded back up the send path. - downgrading the library across a crash resumes a leased chain from its stale snapshot: already-released readers skip unknown fields by definition, so no code here can gate it. Release-note constraint. set_states_for_testing now resets wire_gated with the states it replaces. Adds guards for both fixture and format: peers_generate_independent_keys pins that make_rng seeds from OS entropy (a deterministic fixture would make the crash assertions pass for the wrong reason), and the decode test covers the wrong-wire-type and duplicate-field cases.
This was referenced Jul 14, 2026
This was referenced Jul 15, 2026
This was referenced Aug 5, 2026
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
1 participant
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.
Problem
Every send serialized the entire
SessionRecord(protobuf) and committed a storage transaction before the stanza went to the wire: ~2.26 KB/msg, 229 MB per 100k messages in the pingpong profile, plus one SQLite fsync per message on the hot path. The pre-wire flush is mandatory because the message key and IV derive deterministically from the sender-chain counter, so a crash between the wire write and persistence would re-derive an already-used (key, IV) pair.Insight
What has to be durable per operation is not the ratchet state — it is the promise that no counter repeats. That promise can be bought in bulk.
Mechanics
SessionRecordgains a counter lease (reservedSenderChainIndex): when the reservation is exhausted, encrypt raises the ceiling bySENDER_CHAIN_RESERVATION_BATCH(64) counters and marks the record pending; only that send flushes synchronously before the wire.RecordStructurelevel (the encoder was already hand-rolled inserialize_into), so the vendored whatspecwhatsapp.protois untouched. Field number 100, far from the upstream fields (1, 2); old readers skip the unknown field.SessionRecord::deserializefast-forwards the sender chain to the lease ceiling, so every possibly-spent counter becomes underivable after any reload (crash OR reconnect). Cost is <= 64 HMACs, only on a load with an active lease.Client::persist_signal_state_pre_wire.mark_wire_gated), so group decrypt dirtiness no longer forces a synchronous flush on unrelated DM sends.Measured effect (A/B vs
main, pingpong)dhat, in-memory backend, 20k-message monologue at rate 1000 (client keeps up,
pong_failed=0on both sides):mainSessionRecord::serialize_into/ msgflush_signal_cache/ msgThe per-message serialize and per-message signal-cache flush both drop ~95% (to roughly 1-in-64, as designed). A second run at rate 12000 / 100k reproduced the same delta (229.2 -> 220.6 allocs/msg, serialize_into 4.01 -> 0.19).
Throughput is rate-limited in this harness, so at a fixed rate both sides sustain the same ~11.4k msg/s with
pong_failed=0; client CPU trends lower but sits inside run-to-run noise on a loaded box, so no CPU number is claimed. Note the bench uses the in-memory backend, so the per-message SQLite fsync this PR also removes is not exercised here — the allocation reduction is the part this harness can measure.Safety interleavings (all tested)
MAX_FORWARD_JUMPS)promote_stateburns the lease on the promoted chain;promote_fresh_state(new ratchets) zeroes instead of burningTests
wacore/libsignal/tests/counter_lease.rs— 6 end-to-end Alice/Bob scenarios (crash-sim via serialize/reload)SignalStoreCachegate (5 tests)src/signal_flush.rs— the client-side gate: a lease send flushes synchronously, a covered send coalescessession_reuse.rsrewritten for the new contract: the durable resume position always covers spent counters; a send that raises the lease aborts pre-wire if persistence fails; a covered send survives a storage failure (new test)cargo fmt/clippy --all --testsclean; 2604 unit/integration tests passing. E2e run in CI (mock server).Caveats