perf(signal): move backend I/O and user scans out of the global cache locks

[jlucaso1]

Summary

Three call sites held a process-wide lock across work that did not need it, so an operation on one chain or one address serialized every unrelated Signal read behind it.

get_sender_key held the global sender-key mutex across the backend round-trip and the record decode. The other four cold-read paths in the same file (checkout_session, peek_session, has_session, get_identity) already probe under the lock, drop it, do the I/O, then re-lock and re-check; delete_sender_key_durable states the intent outright ("unrelated chains must not queue behind backend latency on the global cache mutex"). This was the one that didn't.

Correctness here does not rest on the per-chain sender_key_lock, and I want to flag that explicitly because it's tempting to assume it does. The audit note I started from claimed the callers always hold that lock, citing the documented precondition on group_cipher's group_encrypt/group_decrypt/process_sender_key_distribution_message. That precondition is real for the load_sender_key trait path, but several production callers reach the cache directly without it — SignalFeature::has_sender_key (src/features/signal.rs) takes no chain lock at all, and the force_skdm probe in src/send/mod.rs holds only group_distribution_lock, which is per-group, not per-chain. So the safety argument has to be the re-check protocol itself.

Getting that protocol right took several passes, and the review threads carry the reasoning.

The obvious version — re-lock, and if the slot holds anything, defer to it — handles a concurrent put or delete (the delete case is the sharp one: the cache stores a tombstone as Some(None), and deferring to it is what stops a retired chain being resurrected). But it is not sufficient, because an absent slot is ambiguous. A newer record can be written, flushed, and then dropped by a capacity eviction or clear_after_flush() while the read is in flight, leaving the slot absent again. A clean removal keeps the cache incarnation, so the bytes read before that write would deserialize as a trusted exact reload rather than fast-forwarding to the stored reservation ceiling, and the chain could resume an iteration that has already been published.

So the map keeps a bounded window of its most recent removals plus a sequence number. A cold read stamps the sequence before releasing the lock and, on install, asks whether its own key was removed since that stamp. A read that loses falls back to reading under the lock, which cannot be raced at all.

Two properties of that shape matter. It is per key, so churn on unrelated chains — the normal state of a cache sitting at its eviction watermark — costs a reader nothing. And it holds no per-reader state, so a future cancelled mid-backend leaves nothing to reclaim and the bookkeeping stays fixed-size regardless of reader behavior. Two cases answer conservatively: a removal that cannot name its keys (clear, retain), and a reader older than the retained window. Both report "removed", which costs a re-read rather than admitting bytes that predate a write.

Capturing the incarnation up front also lets the decode move outside the mutex, so a cold chain carrying a full skipped-key backlog no longer parses under the global lock. The decode error is held rather than raised, so an unreadable row cannot fail an operation a concurrent write already answered.

The Signal store adapters took the device RwLock on every operation (11 sites) only to reach &*device.backend, and held that read guard across the backend round-trip. Device::backend is set once in Device::new and never reassigned (load_from_serializable replaces only core), so the guard bought nothing; because async_lock::RwLock is write-preferring, a single process_command arriving mid-round-trip queued ahead of every later reader and blocked all of them. is_trusted_identity had already been converted with this exact reasoning in its comment; the rest hadn't.

has_state_for_user walked every key of both the session and identity caches (2000 entries each) while holding both global mutexes, once per mapping in migrate_lid_pn_batch, which deliberately runs outside the processing permit and therefore concurrently with encrypt/decrypt.

Design

Item 2 — per-call snapshot, not a pinned one. I first pinned one Arc<Device> snapshot per adapter, reasoning that the only fields read off the device rather than the backend (identity_key, registration_id) are registration-time constants. That reasoning was incomplete: signed_pre_key_id is not constant. Rotation promotes the new id into the device field and then deletes its staged backend row, so an adapter pinned before the promotion resolves the new id neither in its snapshot nor in storage, and a pre-key message naming it fails with InvalidSignedPreKeyId.

So the adapters now hold Arc<PersistenceManager> and call get_device_snapshot() per operation — the alternative design this change was weighed against at the start, with the signed-pre-key case deciding it. It is still lock-free (a std read guard held just long enough to clone an Arc, never across an await) and strictly fresher than the read guard it replaces, which only ever observed state as of its own acquisition. The cost over a pinned snapshot is one atomic refcount bump per store call.

Per-call snapshotting alone does not close that window, because it is also intra-call: the promotion can land between the snapshot and the backend lookup. get_signed_pre_key therefore re-reads the snapshot after a miss, which always resolves for the promoted key — once the staged row is gone, the device field holds it. (It does not resolve the pruned retained key case; see below.)

Item 3 — conservative superset, not an exact counter. The session and identity maps are wrapped in a UserIndexedCache newtype owning both the map and a HashSet of users. An exact per-user counter would need every removal path to decrement, and a single missed decrement is a false negative — the unsafe direction, silently skipping a migration for a user that has state. The superset inverts that: removals leave the set alone, so its only error is a stale true, which costs one migration pass that finds nothing (both production callers treat true as "may have state, do the scan"). Worth stating precisely, since it is easy to get backwards: a stale true short-circuits before the backend probe, so the cost is a redundant migration pass, not a redundant probe.

Making it a newtype rather than a convention is the point — insert is the only way into the map, so the compiler enforces that no entry lands without registering its user, removing the missed-increment failure mode by construction.

The query is normalized through the same function that derives the keys. A matching address begins with the query, so a separator inside the query is also the address's first one, and an addressed 19995551006:5 and a bare 19995551006 collapse to one key. (I did not take the suggestion to split on @ first — that would key the index on 111:5 and make the device-less 111 that real callers pass miss, which is the unsafe direction. Reasoning is in the thread.)

Drift is bounded two ways: compact_users rebuilds from live keys once the set exceeds both the eviction high watermark and the live key count, and teardown's clear_clean_entries compacts unconditionally. The second condition is what makes the rebuild self-limiting — a rebuild always lands at or below the live key count, so it cannot re-fire immediately, which matters for a store holding more distinct users than the watermark in entries eviction cannot trim. After compaction the index equals the original "any cache key for this user" predicate exactly.

I did not change has_pending_pairwise_writes_for_user, despite it being in the same audit item — see below.

Changes

• get_sender_key: probe under lock, drop, backend I/O, decode, re-lock, re-check, install — matching the four sibling paths. A racer's value wins the re-check, and an install additionally requires that this key was not removed and the incarnation did not change, so an absent slot cannot be mistaken for "never written".
• get_sender_key: bounded unlocked attempts, then a fallback read under the lock, so the retry cannot spin. Decode errors are deferred past the re-check.
• UserIndexedCache<V> newtype backing the session, identity and sender-key maps: user index for has_state_for_user, plus a fixed-size window of recent removals for the above, both maintained on the only mutation entry points.
• get_signed_pre_key re-reads the device snapshot after a miss, closing the promotion window described above.
• Breaking (internal): SignalProtocolStoreAdapter::new and SenderKeyAdapter::new take Arc<PersistenceManager> instead of Arc<RwLock<Device>>. Migration: pass the PersistenceManager handle where you previously passed get_device_arc().await. Client::signal_adapter() / sender_key_adapter() / signal_adapter_from() are pub(crate) and became synchronous.
• memory_stats counts the user index and the removal window in each store's total.
• InMemoryBackend gains gate_next_signed_prekey_read, a test hook alongside its existing ones, so the rotation race can be driven deterministically.
• get_device_arc's doc no longer claims store adapters need it — they no longer do.

Cost

Measured in-process with a temporary harness (removed before this PR; nothing bench-shaped is in the diff). Backend latency is a deterministic gate — a held async_lock::Mutex released on a timer, not a wall-clock sleep. Parallelism is std::thread + futures::executor::block_on, so wacore stays Tokio-free. Baseline and patched forms ran in the same binary as two functions, so codegen differences between builds can't masquerade as an effect.

Every number is victim latency: a warm cache-hit operation needing the same mutex, timed while a slow operation holds (baseline) or does not hold (patched) it. That is the quantity these items change; a single-threaded ns/op bench cannot show it.

Three rounds, each with an unmodified control. Control p50 was 118 / 116 / 118 ns across rounds — stable, so the board is signal, not noise.

Item 1 — victim latency, warm read of an unrelated chain during a 20 ms cold miss

	p50	p99
baseline	20.33 ms	20.61 ms
patched	1.12 µs	4.13 µs

Baseline victim latency equals the backend latency, because that is what it is: the victim waits out the whole round-trip. Patched is independent of it. Under the baseline the concurrency tests don't merely fail, they deadlock — verified by pointing them at the old implementation.

Item 1 — cost side, uncontended cold miss (the extra lock acquisition)

	p50	p99
baseline (1 acquire)	307 ns	834 ns
patched (2 acquires)	355 ns	1002 ns

A cold miss takes the mutex twice instead of once: ~50 ns, ~16%. Warm hits are unchanged at one acquisition. These numbers predate the removal window, which adds a u64 read and a scan of at most 64 entries inside critical sections already being taken — below this measurement's resolution.

Item 2 — victim latency, model of a read guard held across a round-trip with one writer arriving mid-flight

	p50	p99
RwLock guard held	20.49 ms	20.55 ms
Arc snapshot	313 ns	339 ns

Honest qualifier: this is a model of the write-preferring head-of-line behavior, not an end-to-end adapter measurement — the two adapter forms can't coexist in one binary. It confirms the mechanism; it does not by itself size the end-to-end win.

Item 3 — partly refuted. Direct per-call cost is a real and stable win:

entries	baseline	patched
2000	16.7 µs	203 ns
200	1.69 µs	203 ns

But the contention premise did not reproduce. Victim latency for a warm session read taken while the scan runs was 118 / 118 / 120 ns baseline versus 117 / 144 / 121 ns patched — inside the control's own spread, i.e. no measurable effect. A 16.7 µs scan holds each mutex too briefly, and runs too rarely (once per new mapping), for a concurrent reader to collide with it often enough to matter.

So: the O(n) → O(1) is confirmed, the "stalls concurrent encrypt/decrypt" claim is not. I kept the change because the per-call win is real, it removes a cost that scales with cache occupancy from a path running concurrently with message processing, and a large migration batch pays it per mapping (a 300-mapping batch is ~5 ms of global-mutex hold time). Anyone hoping this fixes encrypt/decrypt tail latency should not expect it to.

Where the gain does and doesn't appear: items 1 and 2 pay off when backend latency is non-trivial and concurrency is real — a cold group send fanning out over slow storage, an offline drain, a device write landing mid-round-trip. On a warm cache with fast local SQLite and no concurrency they do nothing, and item 1 costs ~50 ns per cold miss.

Caveat on methodology: a single pinned core is impossible for a contention benchmark — the victims need to run in parallel. This is a 4-vCPU container, not a hybrid P/E machine, so I pinned to a fixed set (taskset -c 0-3) for stable affinity instead. The stable control across rounds is the evidence that this was good enough.

Checked and not changed

• Signed pre-key pruning can still race a lookup. Dropping the device guard also removed an incidental serialization: rotation takes the device write lock in process_command before pruning retained keys, so a held read guard used to block it. A lookup for the oldest retained id can now be overtaken by that pruning, and unlike the promotion case the retry cannot help — a pruned row is gone, not relocated. I did not fix it here. There is no cheap correct fix: closing it means serializing signed-prekey lookups against rotation, either reinstating the coupling item 2 removes or adding a dedicated rotation lock, which has its own ordering questions against the flush and the staged-row delete and deserves its own change. The exposure is narrow and self-limiting: the only id at risk is the one crossing out of the retention window at that instant (SIGNED_PRE_KEY_RETENTION is 3), and a message naming it would fail on the next rotation regardless, recovering the same way an expired retained key already does.
• has_pending_pairwise_writes_for_user — same audit item, left alone. It scans dirty ∪ deleted, bounded by writes since the last flush rather than by cache capacity, and cleared on every flush; in steady state they are small, and the 2000-entry measurement that motivated the index does not transfer. Serving it would need a second index over sets that genuinely shrink, reintroducing the missed-decrement failure mode the newtype was chosen to avoid. An O(1) early-out on both sets being empty wouldn't help either: the documented caller reaches it after a failed flush, when they are non-empty by construction.
• The session cold-read paths (peek_session, has_session, checkout_session) — these have the same probe/drop/re-check shape and, as far as I can tell, the same absent-slot ambiguity the removal window now closes for sender keys: SessionStoreState::clear() also preserves the incarnation. I did not extend the fix there. It is pre-existing rather than introduced here, sessions have their own checkout-token and recovery-generation machinery I have not fully traced against this specific interleaving, and changing those paths is outside this change's scope and unmeasured. Flagging it explicitly so it isn't mistaken for "reviewed and fine".
• flush()'s lock scope — load-bearing and untouched. The lock spans snapshot, I/O and clear precisely so dirty sets clear only after successful writes; splitting it would open a snapshot/clear race. Sessions and consumed pre-keys must stay in one scope so the prekey delete is atomic with the session put.
• delete_sender_key_durable — already drops the cache mutex around the backend delete and holds the per-chain lock instead. Correct as-is; it was the model for item 1.
• The per-chain sender_key_lock — still required and still load-bearing for the paths that mutate a chain. Item 1 neither weakens nor depends on it.
• std::sync::Mutex conversion — not attempted. Guards returned by lock_sessions() cross function boundaries and would become !Send, and wacore targets single-threaded wasm32/ESP32.
• SessionEntry::Absent counting as state in has_state_for_user — preserved. Key presence, not value presence, is the predicate.

Validation

cargo fmt --all
cargo test -p wacore --lib                  # 1351 passed
cargo test -p whatsapp-rust --lib           # 1369 passed
cargo test -p wacore signal_durability_chaos_smoke   # 1 passed
cargo clippy -p wacore --lib --tests --features test-util -- -D warnings
cargo clippy -p whatsapp-rust --lib --tests -- -D warnings

No existing test needed adaptation for behavior. Test edits are confined to signal_adapter.rs's own module and follow mechanically from the constructor signature change: six constructors now build a PersistenceManager, and three #[test] fns became #[tokio::test] because that constructor is async. The assertions are unchanged.

New tests, in wacore: a cold sender-key miss loading from the backend; two concurrent cold readers both reaching the backend and converging on one cached value; a concurrent put not overwritten by the late reader; a concurrent delete not resurrected by it; a write dropped by a keyed removal not replaced by the stale in-flight read; the same for an opaque removal, driven through the real flush-then-clear_after_flush sequence; a cancelled cold read leaving no bookkeeping behind; unrelated-chain churn not forcing a re-read; a read that loses every unlocked attempt falling back to the locked path; the user index answering across every public mutation path including an addressed-device query; and the index surviving eviction, compaction and a lossy clear while deferring to the backend when cold. In whatsapp-rust: a promoted signed pre-key being resolvable only from a fresh snapshot, and a promotion landing mid-lookup being resolved by the re-read.

Every one of these was verified to fail (or deadlock) against the implementation it guards, so none passes vacuously. That includes pairs pinning opposite directions of the same mechanism — disabling the install guard fails the keyed stale-read test, widening the removal check back to cache-wide fails the unrelated-churn test, dropping the opaque_removal_seq bump fails the opaque test, and deleting the signed-pre-key re-read fails the mid-lookup test.

cargo clippy --workspace --all-targets could not run locally — alsa-sys fails to build for the VoIP targets without ALSA dev headers in this container, unrelated to this change. Full matrix left to CI.

[coderabbitai]

📝 Walkthrough

Summary by CodeRabbit

• Performance

  • Improved responsiveness across message encryption, decryption, sending, receiving, retries, and calling.
  • Improved signal-state lookup efficiency, especially for accounts with many sessions or keys.

• Reliability

  • Strengthened concurrent key handling to prevent stale data from resurfacing.
  • Improved consistency during cache updates, evictions, simultaneous reads, and key rotation.
  • Improved resilience when signal-state data is accessed or updated concurrently.

• Maintenance

  • Simplified signal-state access while preserving existing messaging and encryption behavior.

Walkthrough

Signal adapter construction is now synchronous and persistence-manager-backed. Adapter operations use fresh device snapshots. Signal caches add indexed user tracking, memory accounting, and race-aware concurrent sender-key reads with expanded tests.

Changes

Signal storage and adapter flow

Layer / File(s)	Summary
Persistence-backed adapter construction src/client/adapters.rs, src/store/signal_adapter.rs, src/store/persistence_manager.rs	Adapters retain Arc<PersistenceManager> and create device snapshots for operations. Adapter tests use persistence-manager-backed fixtures.
Synchronous adapter call sites src/client/..., src/features/signal.rs, src/message/..., src/retry.rs, src/send/mod.rs, src/test_utils.rs, src/voip/facade.rs	Signal and sender-key adapter access no longer awaits construction across client, messaging, retry, sending, testing, and VoIP flows.
Indexed and concurrent signal cache wacore/src/store/signal_cache.rs, wacore/src/store/in_memory.rs	Session, sender-key, and identity caches use UserIndexedCache. Sender-key cold reads release the mutex during backend I/O, validate epochs and incarnations, retry races, and update statistics. Tests cover concurrency, stale data, indexing, eviction, and compaction.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

• oxidezap/whatsapp-rust#326: Extends the same signal-cache implementation with indexed and race-safe storage.
• oxidezap/whatsapp-rust#503: Updates SenderKeyAdapter sender-key loading for shared, snapshot-based backend reads.
• oxidezap/whatsapp-rust#1131: Refactors related Signal adapter construction and tests.

Suggested labels: performance, api-design

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly summarizes the main performance change: moving backend I/O and user scans outside global cache locks.
Description check	✅ Passed	The description directly explains the cache-lock, adapter, indexing, correctness, testing, and validation changes in the pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch perf/signal-backend-io-outside-locks

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Greptile Summary

The PR removes unnecessary global-lock hold time from Signal backend reads and user-state checks while adding race-aware cache installation.

• Signal adapters now acquire a fresh Device snapshot for each operation rather than retaining the device read lock across backend I/O.
• Sender-key cold reads perform backend loading and decoding outside the global cache lock, with removal tracking and a bounded locked fallback to prevent stale installation.
• Session, identity, and sender-key caches now maintain a conservative user index for constant-time state checks.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
wacore/src/store/signal_cache.rs	Adds indexed cache bookkeeping and a bounded race-aware sender-key cold-read protocol without an eligible blocking follow-up issue.
src/store/signal_adapter.rs	Replaces device read guards with per-operation snapshots and retries signed-pre-key misses against a fresh snapshot.
src/store/persistence_manager.rs	Updates documentation to reflect that store adapters no longer require direct mutable device access.
src/client/adapters.rs	Makes adapter construction synchronous and passes the shared persistence manager into each adapter.
wacore/src/store/in_memory.rs	Adds a test-only gate for deterministically exercising the signed-pre-key promotion race.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant Cache as Sender-key cache
    participant Backend
    Caller->>Cache: Probe and stamp incarnation/removal sequence
    Cache-->>Caller: Miss
    Caller->>Backend: Load and decode without global cache lock
    Backend-->>Caller: Stored record
    Caller->>Cache: Re-lock and re-check
    alt Racer populated or deleted slot
        Cache-->>Caller: Return racer's cached result
    else Key removed or incarnation changed
        Caller->>Backend: Retry, then locked fallback if needed
        Backend-->>Caller: Current durable record
    else Slot remained untouched
        Caller->>Cache: Install decoded record
    end

Loading

_{Reviews (7): Last reviewed commit: "test(signal): cover the opaque removal b..." | Re-trigger Greptile}

Dismissed because a newer commit was pushed; Greptile will re-review the current head.

[cubic-dev-ai]

All reported issues were addressed across 1 file (changes from recent commits).

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fde9ad8a64

ℹ️ 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".

[chatgpt-codex-connector]

Refresh device state when loading a signed pre-key

When an adapter is constructed just before a concurrent signed-pre-key rotation promotes a new key, it permanently retains the old Device snapshot. After rotation removes the newly promoted key's staged backend row, Device::load_signed_prekey sees that key as neither current in this snapshot nor retained in storage, so decrypting a pre-key message for the new ID returns InvalidSignedPreKeyId; the previous per-call read guard observed the promoted device state. Keep live device access for this mutable signed-pre-key state or refresh the snapshot at this call.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Count the user indexes in Signal memory estimates

Each UserIndexedCache now retains a second hash table plus separately allocated user strings, but SignalStoreCache::memory_stats() still sums only the primary map's keys and record payloads. At the configured watermarks this omits thousands of allocations across the session, identity, and sender-key stores, materially understating Client::memory_report() and obscuring cache-growth attribution; include the index's retained bytes in the corresponding collection estimates.

AGENTS.md reference: AGENTS.md:L59-L59

Useful? React with 👍 / 👎.

[jlucaso1]

Fixed in 4119696 — memory_stats now adds users_bytes() into each of the three stores' totals.

---

Generated by Claude Code

[chatgpt-codex-connector]

Detect clean eviction before installing a sender-key read

If a cold direct sender-key probe overlaps a same-chain write that is flushed and then removed by capacity eviction or clear_after_flush(), this unlocked backend read can capture the old row while the later re-check finds no cache entry and installs those stale bytes. Clean eviction preserves the cache incarnation, so the obsolete record is treated as an exact reload rather than fast-forwarded; a subsequent group encryption can therefore resume an already-published iteration and reuse sender-key material. Preserve a per-key generation across removal or otherwise reject reads that began before the intervening write.

AGENTS.md reference: AGENTS.md:L57-L57

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Move sender-key deserialization outside the global mutex

After the backend round-trip finishes, this code reacquires sender_keys before calling SenderKeyRecord::deserialize_for_store, so a cold record containing up to MAX_MESSAGE_KEYS (2000) skipped keys still blocks every unrelated sender-key read and write while it is parsed and allocated. This leaves the CPU-heavy half of the contention targeted by the change under the process-wide lock; snapshot the incarnation or a generation, decode without the guard, then re-lock and validate before installing the result.

Useful? React with 👍 / 👎.

[cubic-dev-ai]

1 issue found and verified against the latest diff

Confidence score: 4/5

• In wacore/src/store/signal_cache.rs, the user-ID split logic in has_state_for_user can mis-parse LID addresses containing :, so entries like 111:5@lid.0 miss the cache and repeatedly hit the backend; this risks unnecessary load and latency for affected users — update the parsing to preserve the full local part (split only at @, or otherwise handle : safely) and add a regression test for LID-formatted IDs.

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/src/store/signal_cache.rs">

<violation number="1" location="wacore/src/store/signal_cache.rs:100">
P2: LID users can contain `:`, but this split truncates them before `@`, so cached state for addresses such as `111:5@lid.0` is not recognized by `has_state_for_user` and every check falls through to the backend. Splitting at `@` first, and treating `:` as a delimiter only when no `@` is present, preserves the existing prefix-matching semantics.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: LID users can contain :, but this split truncates them before @, so cached state for addresses such as 111:5@lid.0 is not recognized by has_state_for_user and every check falls through to the backend. Splitting at @ first, and treating : as a delimiter only when no @ is present, preserves the existing prefix-matching semantics.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At wacore/src/store/signal_cache.rs, line 100:

<comment>LID users can contain `:`, but this split truncates them before `@`, so cached state for addresses such as `111:5@lid.0` is not recognized by `has_state_for_user` and every check falls through to the backend. Splitting at `@` first, and treating `:` as a delimiter only when no `@` is present, preserves the existing prefix-matching semantics.</comment>

<file context>
@@ -84,6 +94,112 @@ fn protocol_address_matches_user(address: &str, user: &str) -> bool {
+/// The user half of a protocol address, matching the prefix
+/// [`protocol_address_matches_user`] tests.
+fn user_of_protocol_address(address: &str) -> &str {
+    match address.find(['@', ':']) {
+        Some(end) => &address[..end],
+        None => address,
</file context>

Suggested change

	match address.find(['@', ':']) {
	Some(end) => &address[..end],
	None => address,
	}
	if let Some(end) = address.find('@') {
	&address[..end]
	} else {
	address.split(':').next().unwrap_or(address)
	}

[jlucaso1]

Not taking this one — splitting at @ first would introduce the false negative it's trying to avoid.

For 111:5@lid.0, the existing scan predicate matches both 111 and 111:5: strip_prefix("111") leaves :5@lid.0, which starts with :. And 111 is what the real callers pass — migrate_signal_sessions_on_lid_discovery gets a device-less Jid user. With the suggested split the key becomes 111:5, so a query for 111 misses the index and reports no state for a user that has it, which silently skips the migration.

The current split (first @ or :) is what keeps the key equal to the query on that hot path. The 111:5 form is covered by the sibling comment on has_user, which I did take: the query is now normalized through the same function, so 111:5 and 111 collapse to one key and neither falls through to the backend. Test is the_user_index_never_misses_state_across_the_mutation_paths, which asserts the scan predicate really does match that address shape.

---

Generated by Claude Code

[github-actions]

📦 Binary size report

Metric	main	PR	Δ
bin size (stripped)	9.98 MiB	9.99 MiB	+5.72 KiB (+0.06%) 🔺
bin .text	8.00 MiB	8.01 MiB	+5.50 KiB (+0.07%) 🔺
bin allocated (text+data+bss)	9.98 MiB	9.99 MiB	+7.96 KiB (+0.08%) 🔺
llvm-lines wacore	513,103	515,292	+2,189 (+0.43%) 🔺
llvm-lines wacore copies	16,751	16,822	+71 (+0.42%) 🔺
llvm-lines whatsapp-rust lib	736,307	735,908	-399 (-0.05%) 🔽
llvm-lines whatsapp-rust lib copies	23,173	23,173	0
deps crates (Cargo.lock)	462	462	0

.text per crate

Crate	main	PR	Δ
.text whatsapp_rust	1.83 MiB	1.83 MiB	-792 B (-0.04%) 🔽
.text wacore	686.39 KiB	694.87 KiB	+8.48 KiB (+1.24%) ⚠️
.text wacore_binary	88.60 KiB	88.60 KiB	0
.text wacore_libsignal	173.44 KiB	173.44 KiB	0
.text wacore_appstate	22.35 KiB	22.35 KiB	0
.text wacore_noise	20.94 KiB	20.94 KiB	0
.text waproto	1.74 MiB	1.74 MiB	0
.text whatsapp_rust_sqlite_storage	515.62 KiB	515.62 KiB	0
.text whatsapp_rust_tokio_transport	40.49 KiB	40.49 KiB	0
.text whatsapp_rust_ureq_http_client	11.83 KiB	11.83 KiB	0
.text std	988.88 KiB	986.64 KiB	-2.25 KiB (-0.23%) 🔽
.text other deps	1.90 MiB	1.90 MiB	0

Top movers (cargo-bloat attribution)

Crate	main	PR	Δ
wacore	686.39 KiB	694.87 KiB	+8.48 KiB (+1.24%)
std	988.88 KiB	986.64 KiB	-2.25 KiB (-0.23%)

Baseline: f8165f282 (latest main run) · Head: 1159a7969 · Graphs

Dismissed because a newer commit was pushed; Greptile will re-review the current head.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 41196969bc

ℹ️ 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".

[chatgpt-codex-connector]

Retry signed pre-key lookup after concurrent rotation

When an inbound decrypt snapshots the old device immediately before rotate_signed_pre_key_inner promotes new_id, the rotation can complete and remove the staged row at src/features/rotate_key.rs:259-276 before this lookup reaches the backend. The old snapshot then classifies new_id as non-current, the removed backend row yields None, and a valid pre-key message fails with InvalidSignedPreKeyId; the former device read guard forced rotation to wait. Although refreshing per call fixes the previously reported adapter-pinning case, the current rotation ordering is fresh evidence of this intra-call TOCTOU, so refresh/retry after a backend miss or otherwise synchronize lookup with promotion and staged-row removal.

Useful? React with 👍 / 👎.

[coderabbitai]

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/src/store/signal_cache.rs (1)

1171-1226: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift

The removal epoch is cache-wide, so the "bounded races" argument does not hold under load — and the fallback puts backend I/O back under the global mutex.

Look, the race handling itself is right. Epoch plus incarnation catches the put-flush-evict shape, and the tests prove it. No argument there. My problem is the bound.

removal_epoch is one counter for the entire sender-key cache. It is bumped by any removal of any key. Line 78-80 justifies SENDER_KEY_UNLOCKED_READ_ATTEMPTS = 2 on the claim that losing twice "needs a flush plus an eviction each time." That reasoning assumes the epoch tracks this chain. It does not. evict_if_needed runs on every put_sender_key, every delete_sender_key_durable, and every flush. Once the cache sits above the high watermark, unrelated group traffic bumps the epoch continuously. A cold reader for one chain then loses both attempts to churn it has nothing to do with, and falls through.

Now look at where it falls through to. Line 1212-1224 takes the mutex and then awaits backend.get_sender_key(key) and runs deserialize_for_store while still holding it. That is the process-wide sender-key lock held across disk I/O plus a MAX_MESSAGE_KEYS parse. This whole PR exists to stop doing that. Under a busy cache it starts doing it again, and it starts doing it exactly when the cache is hottest — which is the worst possible time.

Two things need to happen. Scope the removal signal to the key so unrelated churn stops invalidating good reads. And stop deserializing under the lock in the fallback.

♻️ Narrow the fallback so decode does not run under the lock

         // Repeatedly raced. Read under the lock, which cannot be raced at all.
         let mut state = self.sender_keys.lock().await;
         if let Some(cached) = state.cache.get(key) {
             return Ok(cached.clone());
         }
-        let record = match backend.get_sender_key(key).await? {
-            Some(bytes) => Some(Arc::new(SenderKeyRecord::deserialize_for_store(
-                &bytes,
-                &state.incarnation,
-            )?)),
-            None => None,
-        };
+        let incarnation = state.incarnation;
+        let bytes = backend.get_sender_key(key).await?;
+        // Decoding a cold chain walks up to MAX_MESSAGE_KEYS skipped keys.
+        // Holding the store mutex across that stalls every unrelated chain.
+        let record = match bytes {
+            Some(bytes) => Some(Arc::new(SenderKeyRecord::deserialize_for_store(
+                &bytes,
+                &incarnation,
+            )?)),
+            None => None,
+        };
         state.cache.insert(Arc::from(key), record.clone());

For the epoch scoping, a per-key removal counter (or a small HashMap<Arc<str>, u64> of removal generations consulted only on the install path) keeps the two-attempt bound honest. Want me to draft that?

🤖 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 1171 - 1226, Scope
removal-generation tracking to the requested sender key instead of using the
cache-wide removal_epoch, updating the relevant removal paths and install
validation around the sender-key cache flow so unrelated evictions do not
consume SENDER_KEY_UNLOCKED_READ_ATTEMPTS. Refactor the retry fallback after the
bounded loop to fetch and deserialize the backend record without holding
sender_keys, then reacquire the lock only to recheck the cache and install the
result using the same incarnation/key-generation validation.

🤖 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 2149-2206: Add a regression test for the locked fallback path in
the existing signal-cache race tests, using the gated backend fixture with
gated_reads set to 2. Coordinate both read windows so the cache epoch changes
during each attempt, forcing execution through the fallback around the locked
lookup path, then assert the returned and subsequently cached sender-key record
remains the latest expected value.

---

Outside diff comments:
In `@wacore/src/store/signal_cache.rs`:
- Around line 1171-1226: Scope removal-generation tracking to the requested
sender key instead of using the cache-wide removal_epoch, updating the relevant
removal paths and install validation around the sender-key cache flow so
unrelated evictions do not consume SENDER_KEY_UNLOCKED_READ_ATTEMPTS. Refactor
the retry fallback after the bounded loop to fetch and deserialize the backend
record without holding sender_keys, then reacquire the lock only to recheck the
cache and install the result using the same incarnation/key-generation
validation.

🪄 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: 3b95db21-ad3f-4d3b-8104-8a3abe32610b

📥 Commits

Reviewing files that changed from the base of the PR and between f8165f2 and 4119696.

📒 Files selected for processing (13)

• src/client/adapters.rs
• src/client/device_registry.rs
• src/client/sessions.rs
• src/features/signal.rs
• src/message/receive.rs
• src/message/tests.rs
• src/retry.rs
• src/send/mod.rs
• src/store/persistence_manager.rs
• src/store/signal_adapter.rs
• src/test_utils.rs
• src/voip/facade.rs
• wacore/src/store/signal_cache.rs

[cubic-dev-ai]

All reported issues were addressed across 6 files (changes from recent commits).

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

Dismissed because a newer commit was pushed; Greptile will re-review the current head.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 47db86d145

ℹ️ 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".

[chatgpt-codex-connector]

Clean up cold-read bookkeeping when reads are cancelled

When a get_sender_key future is dropped while awaiting the backend, this increment is never paired with finish_cold_read, leaving the key permanently retained in cold_reads; even clear() preserves that tracking map. Repeated cancellation of unique cold lookups therefore grows the cache without bound, and after a later removal the stranded count also keeps the key marked invalid so every subsequent lookup retries and falls back to backend I/O under the global sender-key mutex. Use cancellation-safe/RAII bookkeeping or otherwise remove registrations when the future is dropped.

AGENTS.md reference: AGENTS.md:L59-L59

Useful? React with 👍 / 👎.

[jlucaso1]

Right, and the fix I'd already made only covered half of it — an insert cleared the stranded mark, but nothing reclaimed the cold_reads entry itself, and you're correct that clear() deliberately preserves that map.

Rather than add RAII around an async mutex, I dropped the per-reader registration entirely in f0e22c7. There's now a fixed window of the most recent removals plus a sequence number: a reader stamps the sequence before releasing the lock and asks removed_since(key, stamp). No per-reader state exists, so a dropped future leaves nothing to reclaim, and the bookkeeping is bounded by the window rather than by reader behavior. It stays per key, so the unrelated-churn property survives.

Two cases answer conservatively — a removal that can't name its keys (clear, retain), and a reader older than the retained window. Both say "removed", which costs a re-read rather than admitting bytes that predate a write.

a_cancelled_cold_read_leaves_no_bookkeeping aborts a reader parked inside the backend and asserts the next read of that chain installs on its first attempt.

---

Generated by Claude Code

Dismissed because a newer commit was pushed; Greptile will re-review the current head.

[cubic-dev-ai]

All reported issues were addressed across 2 files (changes from recent commits).

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f0e22c7156

ℹ️ 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".

[chatgpt-codex-connector]

Compact only after the user index has drifted

When more than the high watermark of distinct users remain live because their entries are dirty or checked out—for example, while backend flushes keep failing—compact_users() cannot reduce users_len() below this threshold. Consequently, every subsequent cache update performs a full O(n) rebuild and reallocates every indexed user while holding the global store mutex. Require the index to be larger than the live map as well as over the watermark before compacting.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Protect retained signed pre-keys from concurrent pruning

When this requests the oldest retained signed pre-key while another task rotates, the lookup can take its snapshot and then be overtaken by promotion and the pruning at src/features/rotate_key.rs:279-289; its backend read then returns None, and the retry also fails because the requested ID is neither the new current ID nor retained anymore. The retained-key pruning path is fresh evidence not covered by the earlier promoted-key retry: the previous device read guard prevented a rotation that started later from pruning the row before this lookup completed, so a valid in-flight pre-key message can now fail with InvalidSignedPreKeyId; synchronize retained-key lookup with pruning or preserve the selected record through the rotation.

Useful? React with 👍 / 👎.

[jlucaso1]

The mechanism checks out — rotation takes the device write lock in process_command before it prunes, so the old read guard did incidentally serialize this, and the retry can't help because a pruned row is gone rather than relocated.

I'm not fixing it in this PR, and I'd rather say why than paper over it.

There's no cheap correct fix. Once remove_signed_prekey lands, the bytes don't exist anywhere, so no amount of re-reading recovers them. Closing it properly means serializing signed-prekey lookups against rotation — either reinstating the coupling item 2 exists to remove, or introducing a dedicated rotation lock. That's a design change with its own ordering questions against the flush and the staged-row delete, and it deserves its own PR rather than being bolted onto one that has already turned over four times.

The impact is bounded in a specific way: the only id at risk is the one crossing out of the retention window at that instant (SIGNED_PRE_KEY_RETENTION is 3, so current plus two rotated-out). A pre-key message naming that id is already at the boundary and would fail on the next rotation regardless — the race costs it one rotation interval of remaining life, and the recovery path is the same one that already handles an expired retained key.

Added to ## Checked and not changed with this reasoning so it stays visible instead of being rediscovered.

The sibling findings in this round I did take, both in 4858351: compaction now also requires the index to exceed the live key count, so a store full of dirty entries stops rebuilding on every update; and memory_stats now covers the removal window.

---

Generated by Claude Code

Dismissed because a newer commit was pushed; Greptile will re-review the current head.

[coderabbitai]

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/src/store/signal_cache.rs (1)

2224-2274: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

The opaque removal path has no concurrency test, and that is the branch where a mistake corrupts a chain.

Every race test here drives removals through drop_clean_sender_key_for_test, which calls UserIndexedCache::remove — the keyed path. Nothing exercises note_opaque_removal, so retain at Line 264 and clear at Line 273 are uncovered against a concurrent cold reader.

Think about which direction the bug runs. The opaque check at Line 179 over-reports, so a bug there just costs re-reads. But if retain ever stopped bumping opaque_removal_seq — someone adds an early return, someone reorders the length comparison — a stale reader's pre-write bytes get installed and trusted as an exact reload. That resumes an already-published chain iteration. Same corruption this whole mechanism exists to prevent, and the test suite would stay green.

Model it the way a_write_dropped_by_eviction_is_not_replaced_by_the_stale_read already does, but drive the removal through the real clear_after_flush path instead of the keyed test helper, then assert the stale bytes still do not land.

Want me to write it?

🤖 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 2224 - 2274, Add a concurrency
regression test alongside
a_write_dropped_by_eviction_is_not_replaced_by_the_stale_read that races a cold
get_sender_key reader with clear_after_flush, exercising the opaque removal path
through retain and clear rather than drop_clean_sender_key_for_test. Use the
existing gated backend and newer-chain setup, then assert the reader and
subsequent cache load return the post-removal record, confirming stale pre-write
bytes are not installed.

🤖 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 `@src/store/signal_adapter.rs`:
- Around line 582-590: Rework the test around SignalProtocolStoreAdapter::new so
the adapter is created before promotion and its first signed-pre-key load is
deliberately held on the pre-promotion snapshot. Use the existing gated-backend
test pattern from signal_cache.rs to park the initial load, perform promotion,
release the load, and then verify the retry resolves the promoted ID; remove the
currently unexercised retry block.

---

Outside diff comments:
In `@wacore/src/store/signal_cache.rs`:
- Around line 2224-2274: Add a concurrency regression test alongside
a_write_dropped_by_eviction_is_not_replaced_by_the_stale_read that races a cold
get_sender_key reader with clear_after_flush, exercising the opaque removal path
through retain and clear rather than drop_clean_sender_key_for_test. Use the
existing gated backend and newer-chain setup, then assert the reader and
subsequent cache load return the post-removal record, confirming stale pre-write
bytes are not installed.

🪄 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: e233a9d0-baea-4222-b561-bee20a089764

📥 Commits

Reviewing files that changed from the base of the PR and between 4119696 and 4858351.

📒 Files selected for processing (2)

• src/store/signal_adapter.rs
• wacore/src/store/signal_cache.rs

[cubic-dev-ai]

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.

<sub>Requires human review: The PR restructures core cache locking (probe/drop/re-check with removal window, per-call device snapshots) and introduces a new UserIndexedCache with concurrency-sensitive removal tracking. These are subtle architectural tradeoffs requiring human review to verify correctness in all interleavings.

Re-trigger cubic</sub>

Dismissed because a newer commit was pushed; Greptile will re-review the current head.

[coderabbitai]

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)

2064-2064: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Use the required fictitious NANP format.

The new 1999555... JIDs use NPA 999. Several line numbers also fall outside 0100 through 0199. Replace these fixtures with 1 + a real NPA + 555 + a line number in the reserved range, such as 12025550119. Restrict the churn loop to that line-number range.

As per coding guidelines, test data must not contain real PII. Based on learnings, fictional NANP numbers must use a real NPA, the 555 exchange, and line numbers from 0100 through 0199.

Also applies to: 2099-2100, 2146-2147, 2190-2191, 2239-2240, 2297-2298, 2343-2344, 2390-2393, 2442-2443, 2490-2640

🤖 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` at line 2064, Update the SenderKeyName test
fixtures and related churn-loop values around SenderKeyName::from_parts to use
fictional NANP numbers with a real NPA, the 555 exchange, and line numbers only
from 0100 through 0199 (for example, 12025550119); replace every listed fixture
consistently and constrain the loop to that reserved line-number range.

Sources: Coding guidelines, Learnings

🤖 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`:
- Line 2064: Update the SenderKeyName test fixtures and related churn-loop
values around SenderKeyName::from_parts to use fictional NANP numbers with a
real NPA, the 555 exchange, and line numbers only from 0100 through 0199 (for
example, 12025550119); replace every listed fixture consistently and constrain
the loop to that reserved line-number range.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: f7277e08-ce50-4698-9da3-62111172dc02

📥 Commits

Reviewing files that changed from the base of the PR and between 4858351 and da63a52.

📒 Files selected for processing (3)

• src/store/signal_adapter.rs
• wacore/src/store/in_memory.rs
• wacore/src/store/signal_cache.rs

[cubic-dev-ai]

0 issues found across 3 files (changes from recent commits).

Confidence score: 5/5

• Automated review surfaced no issues in the provided summaries.
• No files require special attention.

<sub>Requires human review: Sender-key cache locking becomes a release/re-check protocol with a removal-window sequence; PR acknowledges a signed-pre-key pruning race remains. The truncated diff cannot establish correctness, so this concurrency/security tradeoff needs sign-off.

Re-trigger cubic</sub>