fix(signal): gate the group sender-key advance before the wire - #1027
Conversation
|
| Filename | Overview |
|---|---|
| wacore/src/send/encrypt.rs | Removes the duplicate sender-key encrypt implementation and delegates to group_encrypt; adds .context() to preserve SignalProtocolError as a downcasted source for the SKDM-redistribution retry path. |
| wacore/libsignal/src/protocol/sender_keys.rs | Adds reserved_iteration field and lease logic to SenderKeyRecord; moves sender chain key to a Copy field (eliminating per-clone Bytes allocation); adds incarnation-aware serialize/deserialize_for_store paths and fast_forward_sender_chain for recovery. |
| wacore/libsignal/src/protocol/local_field.rs | New shared decoder/encoder for local-only protobuf fields (reservation + store incarnation); fails closed on malformed or duplicate markers so a corrupt lease cannot re-enable spent counters. |
| wacore/src/store/signal_cache.rs | Adds store incarnation to SessionStoreState and SenderKeyStoreState; introduces clear_after_flush() that preserves incarnation trust on clean eviction/reconnect while rotating it under any dirty or pending-gate state. |
| wacore/libsignal/src/protocol/state/session.rs | Refactors decode_reserved_index into the shared local_field decoder; adds deserialize_for_store / serialize_into_for_store incarnation paths for DM sessions; mirrors sender-key clean-reload trust. |
| src/store/signal.rs | Direct Device store now uses a process-wide OnceLock incarnation for both SessionRecord and SenderKeyRecord serialization, giving same-process Device reloads exact behavior while cross-process restarts remain recovery-conservative. |
| wacore/libsignal/src/protocol/group_cipher.rs | Adds copy_out() to CryptoBuffer (right-sized Box output with capacity retention), generalises group_encrypt to S: SenderKeyStore + ?Sized, and replaces unconditional mark_wire_gated with the iteration lease gate. |
| src/message/commit_batch.rs | Single-line change: replaces clear() with clear_after_flush() after a successful inbound-commit flush so clean reconnects preserve incarnation trust. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant App
participant encrypt_group_message
participant group_encrypt
participant SenderKeyStore
participant flush_signal_cache
participant Backend
App->>encrypt_group_message: plaintext
encrypt_group_message->>group_encrypt: delegate (single implementation)
group_encrypt->>SenderKeyStore: load_sender_key
SenderKeyStore-->>group_encrypt: SenderKeyRecord
group_encrypt->>group_encrypt: derive message keys from chain
group_encrypt->>group_encrypt: AES-CBC encrypt → copy_out()
alt "spent_iteration >= reserved_iteration"
group_encrypt->>group_encrypt: "reserve_iterations → wire_gated=true"
group_encrypt->>SenderKeyStore: store_sender_key (wire_gated)
SenderKeyStore->>flush_signal_cache: pre-wire gate required
flush_signal_cache->>Backend: persist record with incarnation marker
Backend-->>flush_signal_cache: ok
flush_signal_cache-->>App: gate released → ciphertext to wire
else within lease
group_encrypt->>SenderKeyStore: store_sender_key (write-behind)
SenderKeyStore-->>App: ciphertext to wire immediately
end
App->>flush_signal_cache: clear_after_flush()
alt no dirty / no pending gate
flush_signal_cache->>flush_signal_cache: clear() — preserve incarnation
Note over flush_signal_cache: next reload is exact, no fast-forward
else dirty or wire_gate_pending
flush_signal_cache->>flush_signal_cache: retain state
end
Note over Backend: crash/restart: new incarnation → fast-forward past ceiling
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant App
participant encrypt_group_message
participant group_encrypt
participant SenderKeyStore
participant flush_signal_cache
participant Backend
App->>encrypt_group_message: plaintext
encrypt_group_message->>group_encrypt: delegate (single implementation)
group_encrypt->>SenderKeyStore: load_sender_key
SenderKeyStore-->>group_encrypt: SenderKeyRecord
group_encrypt->>group_encrypt: derive message keys from chain
group_encrypt->>group_encrypt: AES-CBC encrypt → copy_out()
alt "spent_iteration >= reserved_iteration"
group_encrypt->>group_encrypt: "reserve_iterations → wire_gated=true"
group_encrypt->>SenderKeyStore: store_sender_key (wire_gated)
SenderKeyStore->>flush_signal_cache: pre-wire gate required
flush_signal_cache->>Backend: persist record with incarnation marker
Backend-->>flush_signal_cache: ok
flush_signal_cache-->>App: gate released → ciphertext to wire
else within lease
group_encrypt->>SenderKeyStore: store_sender_key (write-behind)
SenderKeyStore-->>App: ciphertext to wire immediately
end
App->>flush_signal_cache: clear_after_flush()
alt no dirty / no pending gate
flush_signal_cache->>flush_signal_cache: clear() — preserve incarnation
Note over flush_signal_cache: next reload is exact, no fast-forward
else dirty or wire_gate_pending
flush_signal_cache->>flush_signal_cache: retain state
end
Note over Backend: crash/restart: new incarnation → fast-forward past ceiling
Reviews (12): Last reviewed commit: "fix(signal): retain post-flush state dur..." | Re-trigger Greptile
![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: d6282a17fc
ℹ️ 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".
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
📝 WalkthroughWalkthroughSender-key records now persist reservation ceilings with store incarnations, restore chains safely after reload, and reset leases on rotation. Group encryption renews leases at spent-iteration boundaries, reuses ciphertext buffers, and delegates through the send path with recovery and compatibility coverage. ChangesSender-chain reservation leases
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SendPath
participant group_encrypt
participant SenderKeyRecord
participant SenderKeyStore
participant SignalCache
SendPath->>group_encrypt: encrypt group message
group_encrypt->>SenderKeyRecord: derive keys and evaluate reservation
group_encrypt->>SenderKeyStore: persist reservation at lease boundary
SignalCache->>SenderKeyRecord: reload with store incarnation
SenderKeyRecord-->>SignalCache: fast-forward untrusted sender chain
group_encrypt-->>SendPath: return encrypted message
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/libsignal/src/protocol/sender_keys.rs`:
- Around line 548-582: Extract the duplicated tag-scanning logic from
SenderKeyRecord::decode_reserved_iteration and
SessionRecord::decode_reserved_index into a shared internal
decode_local_only_u32_field helper accepting bytes and field_number. Preserve
the existing fail-closed behavior for malformed tags, wrong wire types,
duplicate fields, invalid varints, and values outside u32, while returning 0
when the target field is absent; update both callers to use it.
- Around line 662-667: Update the state replacement logic around
process_sender_key_distribution_message so reserved_iteration is reset only when
the incoming sender-key state has a different chain_id from the current chain.
Preserve the existing reservation when duplicate SKDMs re-add the same chain,
while still resetting it for an actual chain rotation.
🪄 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: 54109142-e63f-4d3f-bf49-90d9d132bbcd
📒 Files selected for processing (2)
wacore/libsignal/src/protocol/group_cipher.rswacore/libsignal/src/protocol/sender_keys.rs
jlucaso1
changed the title
d6282a1 to
2d99c4e
Compare
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/send/encrypt.rs`:
- Around line 29-31: Update the error mapping around
crate::libsignal::protocol::group_encrypt to wrap the original
SignalProtocolError as the anyhow source while adding the “group encrypt failed”
context. Preserve the existing async result flow and ensure callers can still
downcast the underlying protocol error via
downcast_ref::<SignalProtocolError>().
🪄 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: 27f26a5e-e607-4c06-8b8c-eb733a3333a1
📒 Files selected for processing (5)
wacore/libsignal/src/protocol/group_cipher.rswacore/libsignal/src/protocol/sender_keys.rswacore/src/send.rswacore/src/send/encrypt.rswacore/src/send/tests.rs
Merging this PR will improve performance by ×2.8
Performance Changes
Tip Curious why this is faster? Comment Comparing 
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d99c4e078
ℹ️ 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".
There was a problem hiding this comment.
♻️ Duplicate comments (2)
wacore/libsignal/src/protocol/sender_keys.rs (2)
674-679: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winOnly reset the reservation on an actual chain rotation.
We process billions of messages, and performance matters. You're unconditionally resetting
reserved_iterationto0here. If we process a duplicate SKDM that re-adds the exact same current chain, you're wiping out a live reservation and forcing an entirely unnecessary wire gate.Update this logic so we only clear the reservation when the incoming sender-key state actually has a different
chain_idfrom our current active chain. We need things to work right and efficiently.⚡ Proposed fix
- self.states.push_front(state); - // The current chain changed: any reservation belonged to the old current - // chain. Reset so the next send re-reserves and gates against the new - // one instead of treating stale-covered iterations as durable. - self.reserved_iteration = 0; + let is_new_current_chain = self.states.front().map_or(true, |s| s.chain_id() != chain_id); + self.states.push_front(state); + // If the current chain changed, any reservation belonged to the old current + // chain. Reset so the next send re-reserves and gates against the new + // one instead of treating stale-covered iterations as durable. + if is_new_current_chain { + self.reserved_iteration = 0; + }🤖 Prompt for 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. In `@wacore/libsignal/src/protocol/sender_keys.rs` around lines 674 - 679, Update the state replacement logic around self.states.push_front(state) to compare the incoming state's chain_id with the current active chain's chain_id before resetting reserved_iteration. Clear the reservation only when the chain IDs differ; preserve the existing reservation for duplicate SKDMs that re-add the same current chain.
569-593: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract this duplicated varint-scanning logic to a shared helper.
Look, at Meta we don't just copy-paste critical protocol parsing code. This
decode_reserved_iterationlogic is structurally identical to the DM side'sSessionRecord::decode_reserved_index.If these two independently drift, we end up with an inconsistent security posture between the DM and group paths. Extract a shared
decode_local_only_u32_field(bytes, field_number)helper so a future fix or tightening of the parsing rules automatically protects both paths. We need this codebase to be maintainable and rock-solid.🤖 Prompt for 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. In `@wacore/libsignal/src/protocol/sender_keys.rs` around lines 569 - 593, Extract the varint-scanning implementation from decode_reserved_iteration into a shared decode_local_only_u32_field(bytes, field_number) helper, colocated with the existing SessionRecord::decode_reserved_index logic or another common protocol utility. Update both decode_reserved_iteration and SessionRecord::decode_reserved_index to delegate to it while preserving duplicate-field rejection, Varint wire-type validation, u32 conversion, unknown-field skipping, and the zero default.
🤖 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.
Duplicate comments:
In `@wacore/libsignal/src/protocol/sender_keys.rs`:
- Around line 674-679: Update the state replacement logic around
self.states.push_front(state) to compare the incoming state's chain_id with the
current active chain's chain_id before resetting reserved_iteration. Clear the
reservation only when the chain IDs differ; preserve the existing reservation
for duplicate SKDMs that re-add the same current chain.
- Around line 569-593: Extract the varint-scanning implementation from
decode_reserved_iteration into a shared decode_local_only_u32_field(bytes,
field_number) helper, colocated with the existing
SessionRecord::decode_reserved_index logic or another common protocol utility.
Update both decode_reserved_iteration and SessionRecord::decode_reserved_index
to delegate to it while preserving duplicate-field rejection, Varint wire-type
validation, u32 conversion, unknown-field skipping, and the zero default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 56c9c6b6-5c15-4549-bb73-c87054b57380
📒 Files selected for processing (1)
wacore/libsignal/src/protocol/sender_keys.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e8e69421f
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wacore/libsignal/src/protocol/sender_keys.rs (1)
305-310: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn a
Resultinstead of silently dropping invalid seeds.Look, we connect billions of people on WhatsApp, and we don't silently swallow crypto key errors. You're doing a silent
.ok()?to parse the 32-byte seed here, relying entirely on the caller (deserialize) to have pre-validated it. That's a massive footgun. If another engineer calls this crate-public method and forgets the validation loop upstream, a corrupted 15-byte seed gets treated exactly like a missing seed, silently dropping the sender chain and breaking encryption.Move fast, but don't break my platform. Let's move the seed validation inside
from_protobufand make it return aResult<Self, SignalProtocolError>. That way the compiler enforces the invariant, and we can remove the redundant eager-validation loops currently sitting indeserialize.🤖 Prompt for 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. In `@wacore/libsignal/src/protocol/sender_keys.rs` around lines 305 - 310, Update from_protobuf to return Result<Self, SignalProtocolError> and validate the sender chain seed there, propagating an explicit error instead of using try_into().ok()? to silently discard invalid seeds. Adjust callers such as deserialize to handle the Result, and remove their redundant eager-validation loops while preserving valid protobuf conversion behavior.
🤖 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/send/encrypt.rs`:
- Around line 23-32: Update the group encryption call in the surrounding
send/encrypt flow to remove the redundant Ok(...?) wrapper and attach the
context message "group encrypt failed" with anyhow::Context. Preserve the
underlying error source so callers can still downcast SignalProtocolError.
---
Outside diff comments:
In `@wacore/libsignal/src/protocol/sender_keys.rs`:
- Around line 305-310: Update from_protobuf to return Result<Self,
SignalProtocolError> and validate the sender chain seed there, propagating an
explicit error instead of using try_into().ok()? to silently discard invalid
seeds. Adjust callers such as deserialize to handle the Result, and remove their
redundant eager-validation loops while preserving valid protobuf conversion
behavior.
🪄 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: 199923d9-74ae-4aa2-b78d-88479abfd57e
📒 Files selected for processing (5)
wacore/libsignal/src/protocol/local_field.rswacore/libsignal/src/protocol/mod.rswacore/libsignal/src/protocol/sender_keys.rswacore/libsignal/src/protocol/state/session.rswacore/src/send/encrypt.rs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
wacore/src/send/encrypt.rs (1)
22-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
.context()to preserve the operational trace.Look, WhatsApp connects billions of people, and when group encryption fails in production, it's a Sev 1. I need my engineers to have full operational visibility to debug and fix things fast. Wrapping this in
Ok(...?)completely swallows the context.
anyhow::Contextattaches a contextual message but perfectly preserves the original error source, so yourdowncast_ref::<SignalProtocolError>()retry logic will still work exactly as intended. We do not compromise on reliability or observability here. Drop theOk(...?)wrapper, add.context("group encrypt failed"), and let's get this right.🔧 Proposed refactor
- Ok(crate::libsignal::protocol::group_encrypt( + crate::libsignal::protocol::group_encrypt( sender_key_store, sender_key_name, plaintext, csprng, ) - .await?) + .await + .context("group encrypt failed")🤖 Prompt for 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. In `@wacore/src/send/encrypt.rs` around lines 22 - 32, Update the group encryption call in the surrounding send/encrypt flow to remove the redundant Ok wrapper and attach the operational message “group encrypt failed” with anyhow::Context. Preserve propagation of the original error so existing SignalProtocolError downcasting and retry behavior remain intact.
🤖 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/libsignal/src/protocol/local_field.rs`:
- Around line 23-43: Update decode_local_only_u32_field to declare the bytes
parameter as mutable and remove the redundant local buf binding; keep the
existing loop and decoding logic using the mutable parameter unchanged.
---
Duplicate comments:
In `@wacore/src/send/encrypt.rs`:
- Around line 22-32: Update the group encryption call in the surrounding
send/encrypt flow to remove the redundant Ok wrapper and attach the operational
message “group encrypt failed” with anyhow::Context. Preserve propagation of the
original error so existing SignalProtocolError downcasting and retry behavior
remain intact.
🪄 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: eeb7558e-5d48-4926-b0a8-c11215f0f4cb
📒 Files selected for processing (5)
wacore/libsignal/src/protocol/local_field.rswacore/libsignal/src/protocol/mod.rswacore/libsignal/src/protocol/sender_keys.rswacore/libsignal/src/protocol/state/session.rswacore/src/send/encrypt.rs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wacore/libsignal/src/protocol/group_cipher.rs (1)
499-503: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueProvide meaningful
expectmessages.Look, I need things to work right, and part of that is knowing why they broke. Why are you putting
expect("test")everywhere in these tests? Write properexpectmessages so when this fails in CI, our engineers actually know what blew up without digging through the backtrace. Move fast, but leave helpful breadcrumbs for your team.Please update this and the other instances of
expect("test")in this file to something descriptive.♻️ Proposed refactor for this instance
let skdm = block_on(create_sender_key_distribution_message( &name, &mut bob, &mut rng, )) - .expect("test"); + .expect("failed to create distribution message");🤖 Prompt for 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. In `@wacore/libsignal/src/protocol/group_cipher.rs` around lines 499 - 503, Replace every expect("test") in the group cipher test code with a descriptive message that identifies the failed operation and relevant test context, including the create_sender_key_distribution_message call near skdm. Preserve the existing behavior and error propagation while ensuring each failure message helps diagnose the specific operation that failed.
🤖 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/libsignal/src/protocol/group_cipher.rs`:
- Around line 407-408: Replace the temporary zero-filled Vec construction in the
get_buffer() call with a direct resize on the returned buffer, increasing its
length by CryptoBuffer::MAX_RETAINED_CAPACITY * 4 and filling new elements with
zero.
---
Outside diff comments:
In `@wacore/libsignal/src/protocol/group_cipher.rs`:
- Around line 499-503: Replace every expect("test") in the group cipher test
code with a descriptive message that identifies the failed operation and
relevant test context, including the create_sender_key_distribution_message call
near skdm. Preserve the existing behavior and error propagation while ensuring
each failure message helps diagnose the specific operation that failed.
🪄 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: 634fbbd0-562c-42b0-85c7-a5252629810b
📒 Files selected for processing (2)
wacore/libsignal/src/protocol/group_cipher.rswacore/libsignal/src/protocol/local_field.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9644e24f4b
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c557537b9
ℹ️ 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".
A warm group send advanced the sender-key chain and stored the record without gating the advance on durability. Sender-key message keys and IVs derive deterministically from the chain iteration, so the ciphertext could reach the wire before the advance was persisted; a crash in that window re-derives the same iteration on reload, a nonce reuse toward every group member. Confirmed empirically: 196 of 200 warm group sends took no pre-wire flush (neither a session reservation nor a sender-key gate). The production encrypt path (wacore::send::encrypt_group_message) was a second copy of the sender-key encrypt that never marked the advance wire-gated; the gate added for groups lived only in the library primitive group_encrypt, which production does not call. De-duplicate: the production path now delegates to group_encrypt, so the advance, the wire gate, and the durability logic live in one place. Gate cheaply via an iteration lease (mirrors the DM counter lease in SessionRecord). SenderKeyRecord durably reserves SENDER_CHAIN_RESERVATION_BATCH iterations at a time; only the send that raises the ceiling flushes synchronously (1 in 64), the rest ride the coalesced write-behind, and a reload fast-forwards the current chain past the reserved ceiling so no possibly-spent iteration is re-derivable. The lease field is local-only (appended after the generated encoding, field 100), so the vendored proto is untouched and old readers skip it; the decoder scans it out fail-closed. Reset to 0 on rotation so the next send re-reserves against the new chain. Cost: about +2 allocations/msg on the group-send bench (the 1-in-64 flush the buggy path skipped), versus gating every warm send which would flush on all of them. Not a throughput change (group send is chain-lock bound). Tests: crash-mid-lease skips spent iterations and the peer decrypts across the gap; the gate amortizes to one per batch; rotation resets the lease; legacy records load byte-identical with reservation 0; a corrupt reservation fails closed; and the production encrypt_group_message leases the chain (regression guard for the dead-gate defect). Full workspace suite (excluding e2e) green.
The sender chain seed is a Bytes in the generated SenderKeyStateStructure, so every SenderKeyRecord clone and every copy-on-write on an encrypt/decrypt advance promoted that Bytes to a shared allocation. Mirror the message_keys trick: keep the current chain key in a Copy field on SenderKeyState, leave the protobuf field empty in memory, and reassemble it only at as_protobuf. Measured on the group-send bench (dhat, 50-member group): 214.7 -> 206.4 allocations/msg (-3.8%), more than offsetting the +2/msg the durability gate adds; net -6.6/msg (-3.1%) vs main. Serialize/deserialize round-trips and the crash-mid-lease decrypt (which needs the exact reloaded seed) cover the new representation.
Routing the production group encrypt through the shared primitive brought its thread-local ENCRYPTION_BUFFER along, whose take_buffer hands the buffer away and re-allocates a fresh 1 KiB capacity every call. A group skmsg is small, so that was about +1 KiB of allocated bytes per send (CodSpeed memory: bench_group_send_10 3.5 -> 4.5 KB, _50 6 -> 7 KB). Copy out a right-sized ciphertext and keep the buffer for reuse instead: one allocation per send sized to the message, and the thread-local retains its capacity. dhat, group-send bench: -1.0 KB/msg (the take_buffer 1 KiB gone), -1 alloc/msg.
…gation
The delegation wrapped the libsignal error with anyhow!("...{e:?}"), which
erases the concrete SignalProtocolError type. The warm-send recovery in
src/send/mod.rs downcasts NoSenderKeyState to clear stale sender-key device
tracking and retry with SKDM redistribution; a string-formatted error made that
downcast miss, so a missing local SenderKeyRecord failed the send instead of
self-healing. Propagate the error unwrapped via ? so the concrete type (and its
downcast) survives.
The DM session record and the group sender-key record both append a u32 reservation as a top-level field the generated protobuf structures skip, then scan it back out of the raw stream fail-closed. The two scanners were byte-for-byte twins differing only in the field number and error type, so a tightened validation rule on one could silently drift from the other, an inconsistent fail-closed posture for the exact class of bug the reservation prevents. Extract decode_local_only_u32_field, generic over the error via a closure so each record keeps its own error type. Also clarify why the sender-key reservation reset is unconditional (fail-safe, and a no-op on every production path).
The delegation returned Ok(group_encrypt(...)?), which propagates the error but
carries no hint about which stage failed. Use anyhow's .context("group encrypt
failed") instead: it keeps the concrete SignalProtocolError as the source (so the
warm-send recovery in src/send/mod.rs can still downcast NoSenderKeyState and
retry with SKDM redistribution) while adding a diagnostic layer, and drops the
redundant Ok(...?) wrapper. Add a regression guard asserting the downcast
survives the delegation, which would have caught the earlier anyhow!("{e:?}")
type erasure too.
Take the byte slice by mutable value and advance it directly instead of copying it into a local cursor binding first. No behavior change.
The reusable ENCRYPTION_BUFFER copies out a right-sized ciphertext and keeps the buffer, so a small-message workload no longer reallocates 1 KiB per send. But it also retained whatever capacity a one-off large message grew it to, pinning that allocation on the thread-local for the rest of the process. Release capacity past MAX_RETAINED_CAPACITY after copying the result out: small messages (the common case) still reuse the buffer with no reallocation, while a large message's excess is dropped instead of retained.
Every expect in the lease tests said "test", so a failure named neither the operation nor where in the flow it broke. Name the operation instead (whose SKDM, which iteration, which side of the reload), and identify the failing send by index in the amortization loop. Also fill the oversized buffer with a direct resize rather than materializing a throwaway zeroed Vec to copy from.
Per AGENTS.md, comments explain why and stay brief. These recounted the review thread and the prior implementation instead of the durability invariant each test protects.
6c55753 to
1a1a7be
Compare
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 990-1012: Update clear_after_flush to preserve concurrent dirty
records and pending gates: when sessions or sender_keys are not fully clean,
rotate the recovery incarnation without calling discard, so reservation_pending
and wire_gate_pending remain resident. Continue clearing only fully clean
stores, while retaining the existing recovery-incarnation behavior for dirty
state.
🪄 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: 2b08daa7-a7b6-4fa6-80e9-56998b799d11
📒 Files selected for processing (12)
src/client/tests.rssrc/message/commit_batch.rssrc/store/signal.rswacore/libsignal/src/protocol/group_cipher.rswacore/libsignal/src/protocol/local_field.rswacore/libsignal/src/protocol/mod.rswacore/libsignal/src/protocol/sender_keys.rswacore/libsignal/src/protocol/state/session.rswacore/src/send.rswacore/src/send/encrypt.rswacore/src/send/tests.rswacore/src/store/signal_cache.rs
1 participant
Problem
A warm group send advanced the sender-key chain and stored the record without gating the advance on durability. Sender-key message keys and IVs derive deterministically from the chain iteration, so ciphertext could reach the wire before the advance was persisted; a crash in that window re-derived the same iteration on reload, causing nonce reuse toward every group member.
Instrumentation on the real send path confirmed the gap: 196 of 200 warm group sends took no pre-wire flush. Neither a session reservation nor a sender-key gate covered them.
Root cause
The production path (
wacore::send::encrypt_group_message) was a second copy of sender-key encryption. It advanced and stored the chain but did not mark the advance wire-gated. The group gate added in #1026 lived only in the library primitivegroup_encrypt, which production did not call.Primary fix
group_encrypt, so chain advance, wire gate, and durability logic have one implementation.SenderKeyRecordreserves 64 iterations at a time, mirroring the DM counter lease. Only the send that raises the durable ceiling gates the wire; covered sends use write-behind. Recovery fast-forwards past the ceiling so a possibly spent iteration cannot be re-derived.Copyfield instead of the generated protobuf allocation, and the thread-local encrypt buffer is retained while only a right-sized ciphertext is returned.Follow-up: clean reloads must not look like crashes
The P2 review was valid. The three explicit lifecycle
signal_cache.clear()calls are conditional, but a successful clean teardown also cleared the cache throughteardown_inbound_commits_bounded()incommit_batch. Clean reconnects and clean eviction could therefore burn an unused 64-iteration lease on every reload. After 32 such burns, a peer that missed the intervening messages could see a 2,048-step jump and reject it aboveMAX_FORWARD_JUMPS(2,000).The follow-up fixes this for both DM sessions and group sender keys without weakening crash recovery:
SignalStoreCacheowns a random 128-bit store incarnation.clear_after_flush()clears only fully settled stores. Any session, sender key, identity, prekey removal, or wire gate created after the preceding flush remains resident until durable; only a real lossy discard rotates reload trust.This adds no synchronous I/O and no per-message RNG. A marker is generated when the cache is created or when a lossy boundary invalidates trust.
Direct
DevicestoresThe later direct-store P2 was also valid:
Devicestill used genericserialize/deserializeon every database operation, so its sender-key path burned a 64-iteration lease per send. The same defect existed in the directSessionStore.Direct stores now use the incarnation-aware encoders with one random 128-bit process incarnation held in
OnceLock. Unlike write-back caches, these stores await the backend write before ciphertext can be returned, so a newDevicein the same process is a clean reload; a process restart gets a new marker and remains recovery-conservative. This avoids adding a field to the publicDevice, performs no per-operation RNG or heap allocation, and removes the oldSenderKeyRecord::serialize()allocation used only to test whether a loaded record was empty.Teardown race after a successful flush
A final review found a separate critical interleaving: an outbound operation could install a raised reservation after
flush()released a store lock but before teardown calledclear_after_flush(). The old dirty branch useddiscard(), which erased the newly dirty record and its pending gate;needs_pre_wire_flush()could then return false and let ciphertext leave before the new ceiling was durable.Cleanup now clears a lane only when it is fully settled. Post-flush session, sender-key, identity, and consumed-prekey state remains resident, and reservation/wire gates remain closed until a later successful flush. The incarnation is deliberately not rotated while dirty state is retained: dirty/deleted/checked-out entries cannot be evicted or reloaded, while restart, a new cache, and every actual lossy discard already rotate trust. This avoids turning an unrelated concurrent write into lease burns for clean records.
Safety interleavings
Devicein the same processPerformance validation
The original durability work still pays for itself versus
mainin the 50-member group DHAT run:mainmainThe clean-reload follow-up was tested A/B in isolated worktrees before the dependency-only rebase:
7489036d(before follow-up) versus6c557537(after follow-up), using the samewhatsapp-benchsharness and separately built binaries.Normal group-send: 50 members, 2,000 messages, 200 msg/s, alternating A/B/B/A; averages include two 2,000/2,000 runs per side.
DHAT was rate-limited to 100 msg/s so both instrumented runs completed 2,000/2,000 without mock timeouts:
Reconnect A/B also completed 20/20 clean cycles on both revisions (21 authentications including the initial connection).
The direct-store follow-up was then measured on the rebased commits
18845345(before) and1a1a7be9(after): release builds, in-memory backend, 5,000 directgroup_encryptcalls per pass, one warm-up plus four measured runs in alternating A/B/B/A order.The final teardown-race fix was measured on separately built release binaries,
1a1a7be9versus72b8e4a0, using the reconnect scenario in A/B/B/A order with 20 reconnects per run. Every run completed 20/20 ready cycles and 21/21 authentications.Measured costs for the cache follow-up were explicit: local persistence adds 19 bytes only to records with an active lease, and its isolated A/B release binary grew by 18,800 bytes (+0.071%). On the final rebased head (
72b8e4a0), production binary-size CI passed againstmain(a5029cec): stripped binary +13.28 KiB (+0.12%),.text+11.19 KiB (+0.12%), allocated sections +13.48 KiB (+0.12%), with dependencies unchanged at 472 crates. The direct-store follow-up adds no heap allocation orDevicesize, and its hot path is faster as measured above. One 200 msg/s harness run had a known mock-server pong timeout (all 2,000 ACKs arrived); it was excluded and immediately rerun clean.Regression coverage
0..32; a peer missing the first 32 decrypts the final message. The old behavior emitted0, 64, ... 2048and failed.0 -> 1instead of0 -> 64.0..32across per-operation database reloads, a newDevicein the same process emits33, and an untrusted/restart load advances to64.Validation
cargo fmt --all -- --checkcargo clippy --all --tests --quietcargo test --workspace --exclude e2e-tests --quietcargo check -p whatsapp-rust --no-default-featureswasm_jsgetrandom backendcargo test -p e2e-tests -- --nocaptureagainst the local mock server withCHATSTATE_TTL_SECS=3whatsapp-benchs: normal A/B group-send, DHAT A/B group-send, and reconnect A/BAll completed successfully; default stress/soak tests remain ignored as designed.