fix(signal): stop a DH ratchet stranding the counter lease - #1149
Conversation
…hain The lease is a record-level ceiling, but the chain it bounds restarts at zero on every ratchet. A long monologue followed by one peer reply left the ceiling thousands of counters above the fresh chain, and a recovery reload then refused the record with "implausibly far ahead", stranding the address for every path that has to load it. The rebase runs in the same mutation that swaps the chain, and only ever lowers, so no counter is published under a ceiling that is not durable. Fixes #1146
Deserialization is a pure function of the bytes, so a row that fails once fails forever, on every path that could replace it. Reporting it absent lets the ordinary pre-key rebuild overwrite it instead of waiting for an operator to delete the row. A record that cannot be decoded derives no key material, so it cannot repeat a counter. Counted by wa_session_record_quarantined_total; steady state is zero.
|
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 changes propagate sender-chain reset state through decryption, rebase sender-chain leases after DH ratchets, quarantine undecodable session rows as absent with telemetry, and add restart, crash, and chaos-harness coverage. ChangesSender-chain durability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Peer
participant SessionCipher
participant RecordDecryptTransaction
participant SessionRecord
participant SignalStoreCache
Peer->>SessionCipher: decrypt ratcheted message
SessionCipher->>RecordDecryptTransaction: commit sender_chain_reset
RecordDecryptTransaction->>SessionRecord: rebase sender-chain lease
RecordDecryptTransaction->>SignalStoreCache: persist updated session
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 |
|---|---|
| wacore/libsignal/src/protocol/session_cipher.rs | Tracks whether authenticated decryption replaced the sender chain and selects the corresponding lease-aware commit path. |
| wacore/libsignal/src/protocol/state/session.rs | Adds the downward-only reservation rebase used after an in-place sender-chain replacement. |
| wacore/src/store/signal_cache.rs | Treats persisted sessions that cannot be decoded as absent across load, existence, inspection, and prekey-durability paths. |
| wacore/src/store/signal_cache_durability_chaos.rs | Adds DH-ratchet transitions to the durability fault-injection state machine. |
| wacore/libsignal/tests/counter_lease.rs | Adds regression coverage for ratchet lease rebasing, restart recovery, write-behind coverage, and crash counter uniqueness. |
| wacore/src/telemetry.rs | Adds a counter for persisted session records quarantined during decoding. |
| agent_docs/signal_durability.md | Documents per-chain lease semantics, ratchet rebasing, archived-state promotion, and unreadable-row recovery. |
Sequence Diagram
sequenceDiagram
participant Peer
participant Cipher as Session cipher
participant Record as Session record
participant Cache as Signal cache
participant Store as Durable store
Peer->>Cipher: Authenticated message with new DH key
Cipher->>Record: Install fresh sender chain
Cipher->>Record: Rebase reservation to one batch
Cipher->>Cache: Return committed record
Cache->>Store: Persist ratcheted state and rebased lease
alt Existing row cannot be decoded after recovery
Cache->>Store: Load session bytes
Store-->>Cache: Unreadable session row
Cache-->>Cache: Cache address as session-absent
Cache-->>Peer: Trigger ordinary pre-key recovery
end
Reviews (3): Last reviewed commit: "fix(store): do not retire a prekey behin..." | Re-trigger Greptile
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wacore/src/store/signal_cache.rs (1)
906-929: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake
has_sessionquarantine-aware.
ensure_sessions_inner()can callSignalStoreCache::has_session()on a cold cache for addresses that only have an unreadable quarantined session row. Right now,has_session()returnstruefrombackend.has_session(key), so the session setup skips the no-session recovery even thoughpeek_session/checkout_sessionwould treat the row as absent. Make the async path use the same decoded-absent logic, or this kills the recovery story.🤖 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/store/signal_cache.rs` around lines 906 - 929, Update SignalStoreCache::has_session so its cold-cache backend path applies the same quarantine-aware decoded-absent semantics as peek_session/checkout_session, returning false and caching SessionEntry::Absent for unreadable quarantined rows. Preserve the existing cache checks, backend error propagation, and normal true result for readable sessions.
🤖 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.
Outside diff comments:
In `@wacore/src/store/signal_cache.rs`:
- Around line 906-929: Update SignalStoreCache::has_session so its cold-cache
backend path applies the same quarantine-aware decoded-absent semantics as
peek_session/checkout_session, returning false and caching SessionEntry::Absent
for unreadable quarantined rows. Preserve the existing cache checks, backend
error propagation, and normal true result for readable sessions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4901a116-9fdf-4c93-82c5-722712629f22
📒 Files selected for processing (7)
agent_docs/signal_durability.mdwacore/libsignal/src/protocol/session_cipher.rswacore/libsignal/src/protocol/state/session.rswacore/libsignal/tests/counter_lease.rswacore/src/store/signal_cache.rswacore/src/store/signal_cache_durability_chaos.rswacore/src/telemetry.rs
![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: ac335627de
ℹ️ 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".
has_session asked the backend whether the row existed, which answers true for a row the next checkout discards. That probe is what decides whether a send fetches a pre-key bundle, so the recovery was skipped and the send failed or dropped that recipient from the fan-out. It now decodes through the same quarantine path as the other two loads. The decode is not extra work: the record is cached for the checkout that follows.
jlucaso1
changed the title
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 926-940: Update flush’s pre-key deletion check to use the same
decode-aware session decision as has_session, treating undecodable backend rows
as absent rather than durable. Reuse get_session and decode_stored_session with
the existing session state/incarnation, and perform this while retaining the
current sessions lock instead of reacquiring it. Only delete the consumed
pre-key when a usable session is confirmed.
🪄 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: ba9caa32-9b67-4114-aa40-f0df9f9b0f5c
📒 Files selected for processing (1)
wacore/src/store/signal_cache.rs
The flush asked the backend whether the session row existed before deleting the consumed prekey. A row that does not decode is no session at all, so that deleted the prekey against nothing and left a redelivered pkmsg with neither a usable session nor the prekey to rebuild one, which is the outcome the deferral rule exists to prevent. Decoded through the same path as the other loads, under the sessions lock already held, so the decision stays atomic against a decrypt storing its own session.
Merging this PR will degrade performance by 85.26%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| 👁 | Memory | bench_unpack_compressed |
8 KB | 54.5 KB | -85.26% |
Comparing fix/stranded-counter-lease-after-ratchet (3906aed) with main (f3ef3d2)
|
@codspeedbot why this regret? |
Short answer: this regression is a measurement artifact, not something your change caused. Nothing needs fixing on the code side. What the data shows
Why it got flagged The base and head runs did not execute in the same environment — that's the "Different runtime environments detected" warning at the top of the report. Specifically, for this benchmark:
Memory mode measures allocations, and those can shift with the toolchain/runtime/CPU even on identical source. A jump from 8 KB to 54.5 KB on a benchmark this PR does not touch, on a different CPU, is the signature of a cross-environment comparison rather than a real regression. See https://codspeed.io/docs/instruments/memory for details. Recommendation Treat this as a false positive. You can safely acknowledge it on CodSpeed. It should settle back on its own once base and head are compared on matching hardware — re-running the benchmarks so both sides land on the same runner would confirm it. Summary
Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot. |
1 participant
Fixes #1146, and explains both symptoms reported there as one cause.
The bug
The sender-chain counter lease is a record-level ceiling. The chain it bounds is per-ratchet-epoch: a DH ratchet derives a replacement sender chain from a fresh random ephemeral, overwrites the old one in place at counter zero, and drops it without archiving. The ceiling was left describing the retired chain.
For ping-pong traffic that gap is one batch and nobody notices. For a peer we only ever monologue at, it is not: the account's own primary device gets a copy of every message we send and rarely replies, so the chain climbs to a few thousand before one reply arrives, and that reply strands the ceiling thousands of counters above a chain that just restarted at zero.
Nothing fails yet — a live reload skips the fast-forward entirely. It fails on recovery: a restart, or any lossy cache reset, which is what changes the store incarnation. The reload then has to fast-forward across a gap no send ever created, refuses it at
MAX_RESERVATION_FAST_FORWARD, and fails the record load withreserved sender chain index implausibly far ahead— the exact string in the report.From there the address is stranded, because every path that could repair the session has to load it first: inbound decrypt, the group-send fan-out, and the retry receipt handler all fail with
backend store error in backend. Meanwhilehas_sessionchecks row existence without decoding, so it keeps reporting the session present and no rebuild is ever triggered. Deleting the row by hand was the only way out, which is exactly the workaround in the issue.That accounts for both reported symptoms:
<owner-lid>@liddevice 0, the one peer that fits the monologue shape, and every group send logs an encrypt failure against that single address while the other several hundred devices are fine.The reporter's own hypothesis in the issue — one peer device with a corrupted session row, with symptom 1 as the loud special case — is correct. This is the mechanism behind it.
The fix
1. Rebase the lease when the chain is retired.
rebase_lease_after_sender_chain_resetlowers the ceiling back to one batch as part of the same mutation that swaps the chain, so no snapshot can pair the retired chain with the rebased ceiling. It only ever lowers (min), so a counter is never published under a ceiling that is not yet durable. It keeps one batch rather than dropping to zero, so the fresh chain's first sends stay lease-covered and the write-behind send path is unchanged.An archived-state promotion that ratcheted now takes
promote_fresh_state, which burns the outgoing chain before resetting, instead of having a stale ceiling burned into a chain that never spent anything.2. Let an already-damaged database heal. The cache reports an undecodable session row as absent rather than surfacing a load error, so the ordinary no-session recovery fetches a pre-key bundle and overwrites it. This is what repairs the fleets that are already broken, since fix 1 only prevents new cases. A record that cannot be decoded derives no key material, so it cannot repeat a counter either.
wa_session_record_quarantined_totalcounts them; steady state is zero, and a non-zero rate is worth investigating rather than ignoring.Why the tests never caught it
The durability chaos state machine only ever created fresh sessions and advanced chains. It had no DH-ratchet action, so the interaction between an in-place chain swap and a record-level lease was never exercised. That action is added here.
Verification
Four new tests, all failing without fix 1:
a_dh_ratchet_rebases_the_lease_onto_the_fresh_chainthe retired chain's ceiling (2048) must not survive onto the fresh chaina_ratcheted_record_still_loads_after_a_restartInvalidSessionStructure("reserved sender chain index implausibly far ahead")the_rebased_lease_still_covers_the_fresh_chain_without_a_flusha_rebased_lease_never_republishes_a_counter_across_a_crashThe second reproduces the operator-visible failure verbatim, including that it only appears after a restart: the same record round-trips fine under a live incarnation, which is why this survived so long in normal operation.
cargo fmt --all,cargo clippy --workspace --all-targetswith zero warnings, and green suites:whatsapp-rust1238,wacore1269,wacore-libsignal199 plus the 11counter_leaseintegration tests. Both commits build and pass on their own.For the reporter
The workaround in the issue (
DELETE FROM sessions WHERE address = '<owner-lid>.0') stops being necessary with fix 2 — an already-rotten row is discarded and renegotiated on its own. Fix 1 stops new rows from rotting. The blob size difference noted in the issue (~1096 bytes rotten vs ~414 fresh) is consistent with this: the rotten record carries archived previous sessions the fresh one has not accumulated yet.