perf(signal): coalesce receive flushes and persist outbound state pre-wire - #1022
Conversation
The live receive path and the send epilogue flushed the whole dirty Signal cache once per stanza: a session re-serialize plus a SQLite transaction per message, dominated by the record the ratchet re-dirties every time. Route both through a trailing-edge debounced scheduler (25 ms): one storage write covers a burst of messages. Ordering is unchanged — live acks already preceded the per-stanza flush — so this widens the existing crash-replay window from one stanza to at most one debounce, bounded and recoverable (receive chains re-derive forward; consumed prekeys stay buffered until their session is durable, the flush-internal atomicity is untouched). The offline drain, retry recovery, identity-change recovery and teardown keep their synchronous flushes: those gate acks, receipts or follow-up reads on durability.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSignal cache persistence now uses a generation-scoped, single-flight scheduler and drain-aware flush API. Receive handling schedules coalesced flushes, outbound sending flushes synchronously before transmission, and tests cover retries, durability, reconnects, and connection readiness. ChangesSignal cache flushing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MessageReceiver
participant Client
participant SignalFlushWorker
participant SignalCacheBackend
MessageReceiver->>Client: schedule_signal_flush()
Client->>SignalFlushWorker: coalesce flush request
SignalFlushWorker->>SignalCacheBackend: persist batch-safe Signal state
SignalCacheBackend-->>SignalFlushWorker: success or failure
SignalFlushWorker->>SignalFlushWorker: retry or await dirty window
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Confidence score: 5/5
- Safe to merge after the addressed issues were fixed.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
The on-the-fly migration test moved the LID session to PN in the backend right after a verification message, while that message's ratchet advance was still in the write-behind signal cache — the next reconnect's teardown flush then resurrected the LID session the test had just deleted, and the migration under test never ran. Settle with a reconnect before the surgery, like the durability test already does: the scenario this models (a legacy PN-only DB) arises with a quiescent client anyway.
…nology A fire whose flush failed cleared the pending flag and exited, leaving dirty cache state unwritten until unrelated traffic scheduled again — the documented bounded window became unbounded under a transient storage failure. Re-arm on error so the window doubles as the retry backoff. Also rename the docs: this is a fixed coalescing window, not a trailing-edge debounce (deliberately — extending the deadline per request would defer the flush indefinitely under continuous traffic).
The re-arm-on-error path called schedule_signal_flush from inside its own fire, making the async fn recursive (not Send). Loop inside the fire task instead, holding only the Weak across each window; a concurrent request that re-armed meanwhile owns the retry.
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 `@src/signal_flush.rs`:
- Around line 69-74: Update the retry path around flush_signal_cache_batch_safe
and schedule_signal_flush to track consecutive failures, apply exponential retry
backoff with a defined maximum, and reset the backoff after a successful flush.
Rate-limit the log::error reporting so persistent failures do not emit one error
per retry, while preserving coalescing and retry 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: 1d3df37a-3ce8-4e72-ae9d-ff75676ed0a5
📒 Files selected for processing (1)
src/signal_flush.rs
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
A persistent storage failure turned the re-arm loop into ~40 attempts and error logs per second. Double the backoff per consecutive failure up to a 5s ceiling, which rate-limits the log with it; a success (or a concurrent re-arm) still exits the loop.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
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)
src/signal_flush.rs (1)
67-92: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftKeep the retry backoff shared across fires. The pending flag is cleared before the flush await, so a slow failed flush can be overlapped by a new request that arms a fresh task with
SIGNAL_FLUSH_WINDOWagain. Under steady traffic plus an outage, that resets the delay/log ceiling and brings back the retry storm; move the backoff/failure counter ontoClientor another shared state.🤖 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 `@src/signal_flush.rs` around lines 67 - 92, Move the retry backoff/failure state from the local loop in the signal-flush task to shared state on Client (or equivalent shared state), and update the retry logic around flush_signal_cache_batch_safe to read, increase, and cap that shared value across fires. Ensure a newly armed task reuses the accumulated backoff during ongoing failures and resets the shared state only after a successful flush.
🤖 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 `@src/signal_flush.rs`:
- Around line 67-92: Move the retry backoff/failure state from the local loop in
the signal-flush task to shared state on Client (or equivalent shared state),
and update the retry logic around flush_signal_cache_batch_safe to read,
increase, and cap that shared value across fires. Ensure a newly armed task
reuses the accumulated backoff during ongoing failures and resets the shared
state only after a successful flush.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e2f7812f-880d-4c58-8f43-c34fb2d3f523
📒 Files selected for processing (1)
src/signal_flush.rs
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Inject flush failures via a cfg(test) counter (same pattern as the commit batcher's fail_flushes): with two injected failures the fire must consume both error attempts, re-arm through the backoff, and still persist the pre-existing dirty entry on the third attempt.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Requires human review: Performance optimization introduces coalescing write-behind for signal cache flushes, modifying critical message receive/send paths and durability model. Requires human review to verify correctness and safety of the new timing-based flush mechanism.
Re-trigger cubic
The shared connect helper waited for Event::Connected in a fixed 30s event-loop, then fell back to offline_sync_completed — an orthogonal signal. Under concurrent CI load the mock server was slow to serve the critical app-state IQs, so Connected raced past 30s and the fallback timed out on a signal that says nothing about readiness, failing 9 unrelated app-state tests at once. Gate on wait_for_connected instead: it resolves on is_ready (set at the same point Event::Connected is dispatched, after the critical sync) via a notifier with a TOCTOU re-check, so it does not depend on event arrival order, on who drains the channel, or on offline sync. The startup-sync drain is now best-effort — readiness is already guaranteed, and a message backlog must not fail the connect. The determinism comes from waiting on the right signal, not from a larger timeout.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Modifies core Signal cache flushing logic with new coalescing, affecting all message send/receive paths and data durability. Not safe to auto-approve.
Re-trigger cubic
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
You’re at about 90% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…ression) The generation-scoped worker CAS protected against a stale WORKER, but schedule_signal_flush itself read the generation once and its take-over CAS accepted any state — so a call carrying a torn-down connection's generation could CAS a newer generation's RUNNING|DIRTY state back down to its own, dropping the pending DIRTY and orphaning the state (both the new worker and the demoted one then stand down). Pass the caller's already-validated lane_generation, no-op if it is not the live generation, and never move the state to an older generation. Two tests: a stale schedule leaves gen-1 state bit-for-bit unchanged, and a stale schedule during a new worker's flush cannot clear the pending DIRTY.
…econnect signal_cache and flush_signal_cache no longer flush unconditionally per message (send is synchronous, receive coalesces) — correct their docs. In the migration test, settle via flush_pending_signal_state instead of a full reconnect: faster and isolates the scenario.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
0 issues found across 5 files (changes from recent commits).
You’re at about 90% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Requires human review: Introduces a new concurrency scheduler for Signal cache flush, altering durability guarantees on receive path and adding synchronous flush on send. High-impact, requires human review.
Re-trigger cubic
Adds two InMemoryBackend test hooks: a put_sessions_batch call counter and a fail switch. The new e2e test fails session persistence, then asserts the send returns Err, the flush was attempted (counter moved, so the flush runs pre-wire), the peer receives nothing, and delivery resumes once persistence recovers. This is the deterministic proof of the send-path durability ordering the coalescer relies on. TestClient retains the concrete backend to reach the hooks.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
jlucaso1
changed the title
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
You’re at about 95% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Requires human review: This PR modifies core signal cache flushing logic with concurrency, durability, and new synchronization primitives. Such critical changes require human review despite passing automated checks.
Re-trigger cubic
…-util The put_sessions_batch call counter and fault switch were unconditional fields plus a fetch_add/load on every batch write, so every production build carried them and any in-memory benchmark saw instrumentation the SQLite path does not. Move both behind a new off-by-default test-util feature (and cfg(test) for wacore own tests); the e2e crate enables it. Normal builds regain a zero-cost, two-field struct with no fault- injection surface.
The stale-schedule starvation test only checked that the pending DIRTY survived; the first (unblocked) flush would persist the second session on its own, so it never proved a second window ran. Require the attempt counter to reach 2 after release. The pre-wire abort e2e now registers a sent-node waiter and asserts it stays pending after the failed send: no message node was marshaled, so send_node (and the wire) was never reached, replacing the sole reliance on a negative timed assertion. Also correct the flush_signal_cache comment (no longer per-message, and the backend is generic, not SQLite).
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
0 issues found across 6 files (changes from recent commits).
You’re at about 95% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Requires human review: Significant change to Signal cache flush strategy: introduces coalesced receive-path scheduling and synchronous pre-wire send flushes. Affects core durability model and error handling. Requires human review of concurrency, crash recovery, and backoff logic.
Re-trigger cubic
The generation-scoped atomic ordered only signal_flush_state, not the backend writes. A worker that passed its pre-flush generation check could be preempted (e.g. blocked on the sessions lock teardown holds), and resume its flush after teardown settled the cache and the next connection's drain dirtied it — persisting that drain's rowless ratchet advances out of band, the silent-loss class the commit batcher prevents. Add a signal_flush_lifecycle mutex: the worker holds it only across the flush (never across sleep/backoff) and re-checks the generation under it; teardown holds it across the whole cache settle. So a worker either wins the gate and flushes its own generation before any settle, or acquires it after teardown and stands down on the generation check. Lock order is always gate -> permit/sessions-lock, so no inversion. Two deterministic tests: the gate blocks a worker flush while held, and a worker reaching the gate after the bump stands down without writing.
send_node resolves the waiter before marshaling the node, not "when marshaled"; a pending waiter proves a pre-wire abort.
jlucaso1
changed the title
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
You’re at about 96% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/client/lifecycle.rs">
<violation number="1" location="src/client/lifecycle.rs:850">
P1: Disconnect/reconnect can hang indefinitely when a pre-existing coalesced worker is stalled acquiring the processing permit or writing storage. Include lifecycle-gate acquisition in the teardown deadline (or otherwise cancel/bound the worker) so the existing bounded-settle guarantee still covers this wait.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // backend write between our commit and the next connection's drain, or it | ||
| // could persist that drain's rowless advances. The worker re-checks the | ||
| // generation (bumped above) once it gets the gate, so it stands down. | ||
| let flush_gate = self.signal_flush_lifecycle.lock().await; |
There was a problem hiding this comment.
P1: Disconnect/reconnect can hang indefinitely when a pre-existing coalesced worker is stalled acquiring the processing permit or writing storage. Include lifecycle-gate acquisition in the teardown deadline (or otherwise cancel/bound the worker) so the existing bounded-settle guarantee still covers this wait.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/client/lifecycle.rs, line 850:
<comment>Disconnect/reconnect can hang indefinitely when a pre-existing coalesced worker is stalled acquiring the processing permit or writing storage. Include lifecycle-gate acquisition in the teardown deadline (or otherwise cancel/bound the worker) so the existing bounded-settle guarantee still covers this wait.</comment>
<file context>
@@ -840,6 +841,13 @@ impl Client {
+ // backend write between our commit and the next connection's drain, or it
+ // could persist that drain's rowless advances. The worker re-checks the
+ // generation (bumped above) once it gets the gate, so it stands down.
+ let flush_gate = self.signal_flush_lifecycle.lock().await;
if let Some(client) = self.self_weak.get().and_then(|w| w.upgrade()) {
client
</file context>
1 participant
Summary
The live receive path flushed the whole dirty Signal cache to storage once per stanza — a session re-serialize plus a storage transaction per message, dominated by the record the ratchet re-dirties every time. It now routes through a single-flight coalescer: a burst of receives folds into one flush per window.
Only the receive path coalesces. A lost receive-side advance re-derives forward (the receiving chain derives
CK_n → CK_n+1), and a consumed prekey stays buffered until its session is durable, so a crash inside the window is recoverable. The send path flushes synchronously, before the stanza reaches the wire, with error propagation: reusing an outbound counter reuses its message key + IV, so that advance must be durable before anyone can act on the ciphertext — and a persistence failure aborts the send instead of transmitting an advance we couldn't save.send_status_messagefollows the same rule.Parity with WhatsApp Web
WA Web flushes its Signal store before the send hits the wire (
DirectMsgToDeviceList,GroupSkmsgJob,AndSendStatusMsg) — the send path here matches that exactly. On receive, WA Web flushes after decrypt but before the receipt (ProcessingDecryptApi,Handle/Msg); this PR deliberately diverges there, deferring the receive-side flush into a coalescing window. The divergence is safe precisely because a lost receive advance re-derives forward, which the outbound direction cannot do — hence the asymmetry.Single-flight scheduler, generation-scoped. The state is one atomic
(connection_generation << 2) | RUNNING/DIRTY. Only the idle→running transition for the live generation spawns a worker (one worker per generation); requests mid-flush just markDIRTYand that worker runs another window. A failing flush is retried by the same worker with exponential backoff (25 ms → 5 s cap), so concurrent traffic can't reset it to the base delay and detached tasks can't pile up. The generation is embedded so a reconnect during an in-flight flush is safe with no scheduler reset: a stale worker cannot mutatesignal_flush_statea new-generation worker owns, and a stale schedule call is rejected outright. Across a reconnect a stale worker and the new one can briefly coexist; the stale one stands down at its generation check.Flush-vs-teardown barrier. The generation-scoped atomic orders only
signal_flush_state, not the backend writes. Asignal_flush_lifecyclemutex closes that gap: the worker holds it only across the flush (never across sleep/backoff) and re-checks the generation under it, while teardown holds it across the entire Signal-cache settle. So a worker either wins the gate and flushes its own generation before any settle, or acquires it after teardown and stands down — it can never interleave a raw backend write between teardown's commit and the next connection's drain (which would persist that drain's rowless ratchet advances out of band). Lock order is always gate → permit/sessions-lock.Impact
A/B with the same benchmark-client source, rebuilt separately for each revision (the Rust client statically links the library, so the two binaries necessarily differ — this is not a same-binary comparison). The PR side was built with the bench client's path-dep pointed at the
ef1cf76dworktree;mainatd9e693f5. Mock server = barback on:8080,CHATSTATE_TTL_SECS=3.Provenance caveats: (a) the harness
meta.envrecords the base checkout's SHA (d9e693f5) andharness_git_dirty=yes, not the worktree HEAD, so the artifacts do not self-attest thatef1cf76dwas measured — the PR library revision isef1cf76donly by construction of the path-dep (symbol names in the PR profile do contain the new scheduler). (b) These runs predate the flush-vs-teardown gate and the P3 doc/test commits at the current head; those change teardown ordering and comments, not the steady-state receive allocation, so the deltas below still hold, but the numbers areef1cf76d, not the final head. Allocation is measured againstInMemoryBackend, so these are allocation deltas (session re-serialize + map bookkeeping), not SQLite I/O — the production write cost is separate and not measured here. DHAT is deterministic (run-to-run spread ~0.3%); connect-subtracted, per 2000-cycle ping-pong.put_sessions_batchalloc bytes/cycle¹¹ Allocation attributed to the
put_sessions_batchframe in the 500-cycle symbolic profile. It is an allocation proxy for receive-path flush volume, not a direct call count, andInMemoryBackendhas no transaction — the SQLite write cost is separate.Throughput: no clear regression, but the sample is too small to call. Three normal-mode runs per side:
Medians match, but one PR run was ~5% lower and two PR runs showed elevated ack/pong latency (up to ~153 ms vs ~19 ms on
main), so this is inconclusive — it needs more interleaved samples with p50/p95 latency and a separate max-throughput sweep before claiming parity. Context: an earlier revision's much lower dhat-rate came from a different bench-client binary across those runs, not a library change (in steady-state ping-pong the generation guards are no-ops, though the receive path does add loads/CAS, a timer task and the Weak upgrade overmain). All runs: 2000/2000 pongs, 0 lost.Honest framing: the byte gain is modest. Coalescing the send flush too gave a larger number on this benchmark, but that came from relaxing outbound durability — the exact hazard this revision refuses. What remains is the safe receive-side win (largest under bursty inbound traffic: high-rate ping-pong, group fan-in) plus the cut in receive-path flush allocation.
Durability model
Client::flush_pending_signal_stateis a public "settle now" for read-after-write durability; its doc carries the permit precondition (self-deadlock if awaited from an event handler / durability hook under the drain permit — pre-existing onmain) and notes the lag is a nominal window, not a hard wall-clock bound: it can extend under runtime starvation or slow/failing storage (the retry loop backs off up to 5 s).Tests
test_send_aborts_before_wire_when_persist_fails(e2e): fails session persistence, then asserts the send returnsErr, the flush was attempted (theput_sessions_batchcounter moved), and — the deterministic core — a pre-registered sent-node waiter is still pending afterward.send_noderesolves that waiter beforesend_raw_bytes, so a pending waiter proves the stanza was never marshaled for the wire. Peer-receives-nothing and post-recovery delivery are kept as end-to-end corroboration.test_outbound_ratchet_is_durable_when_send_returns(e2e): reads the backend immediately aftersend_message, no delivery wait, no settle — a coalesced/deferred send flush would leave the sender-chain counter stale and fail it.RUNNINGbit — no second spawn). Retry: an injected-failure flush is retried by the same worker until it persists (eventual persistence; the test does not measure the exact backoff curve). Generation safety: a worker held inside the flush across a bump cannot clobber the new generation's state; a stale schedule call leaves that state bit-for-bit unchanged and the pending DIRTY still forces a second flush attempt (asserted via the attempt counter reaching 2, so it proves the second window ran). Barrier: the lifecycle gate blocks a worker's flush while held, and a worker reaching the gate after a generation bump stands down without writing.InMemoryBackendcounter/fault hooks these tests use are behind an off-by-defaultwacore/test-utilfeature (enabled only by the e2e crate), so normal and benchmark builds carry no extra fields or per-write bookkeeping.wait_for_connectedand keepswait_for_startup_syncas a hard requirement (fixes the earlier CI flake without weakening the suite).Validation
cargo test -p whatsapp-rust --lib(988 passed, incl. 10 scheduler tests) andcargo test -p wacore --libsession_reuse(8),lid_sessions(9), 0 failurescargo clippy -p whatsapp-rust -p wacore -p e2e-tests --tests -- -D warningsRUSTFLAGS='--cfg getrandom_backend="wasm_js"' cargo check --target wasm32-unknown-unknown --no-default-features