fix(signal): reject cold loads that span a flush and eviction - #1229
Conversation
checkout_session, peek_session, has_session and get_identity read the backend outside the store lock and then re-check only whether an entry exists. That cannot separate "never written" from "written, flushed and removed": a clean removal keeps the incarnation, so bytes read before the write are adopted as an exact reload. In checkout_session the record goes straight to the cipher, so the chain index rewinds to one already published, which repeats a message key and IV. peek_session and has_session cache the same stale record for the checkout that follows, and a probe that missed a row written behind it negative-caches a live session into a pre-key recovery. get_identity caches a superseded identity key, hiding the peer's change. Stamp the removal sequence before releasing the lock and re-check it with removed_since, as get_sender_key already does: re-read while the stamp says the key may have moved, then fall back to a read under the lock, which cannot be raced. The retry bound is now one constant shared by all five cold reads. The chaos state machine gains an action that holds a cold session read open across a commit, a flush and the removal after it, then sends with whatever comes back, so the published-key invariant covers this race.
|
| Filename | Overview |
|---|---|
| wacore/src/store/signal_cache.rs | Adds bounded, removal-aware cold-read retries and locked fallbacks for sessions and identities, with comprehensive race coverage and no blocking defect identified. |
| wacore/src/store/signal_cache_durability_chaos.rs | Extends the durability state machine with a cold-read-across-flush action that exercises the real sender-chain key/IV uniqueness invariant. |
Sequence Diagram
sequenceDiagram
participant R as Cold reader
participant C as SignalStoreCache
participant B as Backend
participant W as Concurrent writer
R->>C: Probe key and capture removal/incarnation stamp
C-->>R: Cache miss
R->>B: Read outside cache lock
W->>C: Write newer record
W->>B: Flush newer record
W->>C: Remove clean cache entry
B-->>R: Return pre-flush bytes
R->>C: Re-lock and validate stamp
alt Removal or incarnation changed
C-->>R: Reject stale result and retry
else No intervening invalidation
C->>C: Decode and install result
C-->>R: Return current value
end
Reviews (2): Last reviewed commit: "fix(signal): spell out the peek probe's ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe cache now validates cold reads against concurrent writes, removals, and incarnation changes. Session, identity, and sender-key paths share retry and locked fallback behavior. New tests and a chaos scenario cover reads across flushes and cache eviction. ChangesCold-read race handling
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
🚥 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: 3
🤖 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_durability_chaos.rs`:
- Around line 708-720: Bound the concurrent read/write operation in the chaos
action around tokio::join! so a missed cold read cannot hang indefinitely; on
timeout, fail with a clear diagnostic indicating that
GatedSessionRead::wait_for_read was not released. Preserve the existing cache
update, flush, cleanup, and release behavior on successful completion.
In `@wacore/src/store/signal_cache.rs`:
- Line 1109: Update the documentation comment for checkout_loaded_session so it
has a complete sentence matching the twin wording: describe decoding what a cold
read fetched and caching it.
- Around line 1158-1163: Update the match in the cache lookup around
SessionEntry::Present to explicitly handle SessionEntry::Absent and
SessionEntry::CheckedOut { .. } instead of using _. Keep the existing
Ok(Some(record.clone())) and Ok(None) results, matching the exhaustive variant
handling used at the other SessionEntry sites.
🪄 Autofix
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: 4ba47715-db79-4eaf-934c-1806fd542e7a
📒 Files selected for processing (2)
wacore/src/store/signal_cache.rswacore/src/store/signal_cache_durability_chaos.rs
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
An exhaustive match makes a new SessionEntry variant a compile error at every probe instead of silently answering None at this one.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
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)
1095-1106: 🩺 Stability & Availability | 🔵 TrivialBound backend latency in the locked fallback.
These fallback branches await
SignalStorewhile holding the cache mutex. If a backend call stalls, all session, identity, or sender-key operations on this cache wait behind it. Ensure the backend implementations enforce finite timeouts, or add a timeout at this boundary. Add a stalled-backend test before relying on this fallback for availability.Also applies to: 1183-1192, 1296-1304, 1346-1354, 1444-1458
🤖 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 1095 - 1106, Bound backend awaits in the fallback paths that hold the cache mutex, including the flow around lock_sessions, backend.get_session, and the analogous session, identity, and sender-key lookups. Enforce finite backend timeouts or apply a timeout at these boundaries while preserving existing error handling. Add a stalled-backend test verifying operations do not remain blocked indefinitely behind the mutex.
🤖 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 1095-1106: Bound backend awaits in the fallback paths that hold
the cache mutex, including the flow around lock_sessions, backend.get_session,
and the analogous session, identity, and sender-key lookups. Enforce finite
backend timeouts or apply a timeout at these boundaries while preserving
existing error handling. Add a stalled-backend test verifying operations do not
remain blocked indefinitely behind the mutex.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 090e34bc-b888-4bab-b9b9-cab7011072a3
📒 Files selected for processing (1)
wacore/src/store/signal_cache.rs
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
|
On the out-of-diff note about the locked fallback awaiting the backend under the store mutex: that is the shape #1226 already established for Generated by Claude Code |
2 participants
Summary
Four reads in
SignalStoreCacheshare one shape: probe under the store lock, miss, do the backend I/O outside the lock, re-lock, re-check. The re-check asks only whether an entry is there now, and that question cannot distinguish "never written" from "written, flushed, and then removed from the cache". A clean removal keeps the incarnation, so bytes read before the write decode as an exact reload and are installed as if nothing had happened.The sequence, all four steps concurrent with one cold read:
evict_if_needed(or teardown) drops the now-clean entry;checkout_sessionis the severe one: the record it returns is what the cipher advances, so installing the older version rewinds the sender chain index to one already on the wire. That is message key and IV reuse, whichagent_docs/signal_durability.mdtreats as the non-negotiable property.The other three are milder but the same kind:
peek_sessioncaches the stale record, and the nextcheckout_sessiongets it out of the cache — the same rewind, one step later.has_sessioncaches the record it decoded, so it feeds that same checkout. It also has the opposite direction: a probe that found no row while a session was being written and flushed behind it negative-cachesAbsent, and the next send fetches a pre-key bundle and replaces a live session, discarding the peer's chain.get_identitycaches a superseded identity key, so the next comparison does not see the peer's identity change.Scope of the bug
This is in
main, not something a recent PR introduced. The four unlocked cold reads and the incarnation-based exact-reload trust are both present in the oldest revision ofsignal_cache.rsin this repo's history (c27aae0, 2026-07-28), so the race has been reachable for as long as those two have coexisted. #1224 and #1226 did not create it; #1226 moved the sender-key I/O out of the lock and added exactly the guard this PR applies to the remaining four, which is how it surfaced.Found while reviewing #1222, which widens the window (its read-pool snapshot opens before the commit rather than after the permit). #1222 depends on this PR and is untouched here.
Design
One shape per call site, not a generic helper. The guard itself is two calls on
UserIndexedCache—removal_seq()before releasing the lock,removed_since(key, since)after — plus the incarnation comparison for the stores that have one. Both primitives and the whole rationale already live in one place: the doc comments onremoval_seqandremoved_since. The call sites point there instead of restating it.A shared helper would have to abstract over three different state structs behind three different mutexes, over probes that return different things (a checkout mints a token and can fail with
Busy; the others return a value), over three different installed entry types, and over the fact thatByteStoreStatehas no incarnation at all (identities carry no counters, and their lossy reset goes throughUserIndexedCache::clear, which is already an opaque removal). That is a trait over the three states plus three closures, andget_sender_key— whose decode happens outside the lock — still would not fit it. The cost of the side I dropped is real: the condition now appears at five call sites, so a future third term has to be added five times. What holds that together is the tests: each of the four has a race test that fails the moment its guard goes, verified by pointing all four back at their pre-change bodies (below).What I did factor out is the install step, which was duplicated between the retry loop and the locked fallback:
checkout_loaded_sessionandinstall_loaded_session. Those are mechanical (decode, insert, evict) and have no ordering subtlety.One shared retry constant.
SENDER_KEY_UNLOCKED_READ_ATTEMPTSbecomesUNLOCKED_COLD_READ_ATTEMPTS. The bound answers the same question on every path ("how many unlocked attempts before I take the lock and read there") and the losing condition is the same event everywhere: a removal for that key landing inside the window. Keeping the sender-key name while using it in four more places would make the name lie; giving each path its own constant with the same value would suggest they can be tuned apart, which nothing in the design supports.On how often the bounded retry runs out: reaching the locked fallback needs a removal of that exact key (or an opaque removal, or an incarnation change) inside each of two consecutive backend round-trips. Nothing in the normal flush/drain cycle removes the same address twice that fast — in the test suite the only way to reach the fallback was to force a removal on every attempt — so I have no measured frequency to report, only the shape of what it takes. The fallback is correct at any frequency; it just holds the lock across one read.
Changes
checkout_session,peek_session,has_session,get_identity: stampremoval_seq(and, where the store has one, the incarnation) before releasing the lock; on re-check, install only if neither moved, otherwise re-read. Backend I/O stays outside the lock — that is the point of the guard.checkout_loaded_session/install_loaded_session: the install step, so the retry body and the locked fallback cannot drift apart.SENDER_KEY_UNLOCKED_READ_ATTEMPTSrenamed toUNLOCKED_COLD_READ_ATTEMPTS; same value, now used by all five cold reads.cold_read_race_tests: the four-step race per function, the chain-index property at the level the cipher sees, the opaque-removal branch driven through the realflush+clear_after_flush, the removal-window boundary, retry exhaustion into the locked fallback, and the two happy cases (unraced install serves later reads; a racer that wins keeps its value).ColdDmReadAcrossFlushaction that holds a cold session read open across a commit, a flush and the removal after it, then sends with whatever the read returned — so the existingpublished_dminvariant covers this race instead of a cache-level assertion.Cost
Temporary
divanbench, both arms in one binary (the pre-change bodies kept as twins next to the new ones),taskset -c 2, 3 rounds, medians in ns, cold miss = no row on the backend so the shape runs without a decode. "full" = removal window at its 64-entry worst case for theremoved_sincescan. The bench is not part of this PR.checkout_sessioncheckout_session(full window)peek_sessionpeek_session(full window)has_sessionhas_session(full window)get_identityget_identity(full window)Smaller than the equivalent measured in #1226 (307 → 355 ns on
get_sender_key), and for a different reason: these four already took the mutex twice onmain, so the guard adds a stamp read and the window check, not an acquisition. A full window costs nothing measurable over an empty one — the scan stops at the sequence comparison.Warm hits, same runs:
peek_session88.4 → 87.9,has_session78.3 → 77.8,get_identity90.2 → 91.3. All inside the run-to-run spread (±3 ns), which matches the code: a warm hit returns from the first probe, before the stamp is taken, so it is still a single acquisition.Validation
The new tests were confirmed to fail on the previous behavior: pointing the four public entry points at their pre-change bodies and re-running gives 8 failures out of 11 —
a_cold_checkout_does_not_install_a_record_that_predates_a_flush(index 5 where 40 was published),a_checkout_after_the_race_never_rewinds_the_chain, the peek, both probe directions, the identity read, theclear_after_flushcase, and the window boundary — while the three happy tests still pass. The retry-exhaustion test does not fail there, it hangs: without the guard the read installs on its first attempt and never takes the second one the test drives.The chaos extension fails on the previous behavior too, with the invariant that matters rather than a cache assertion:
No existing test needed adapting. Full matrix left to CI; e2e not run (no mock server here).
Generated by Claude Code