docs: reflect retry pre-wire Signal persistence (whatsapp-rust#1041) - #405
Conversation
whatsapp-rust#1041 gated the resend issued for a DM/group retry receipt on the same pre-wire lease durability rule regular sends use, via a shared send_retry_stanza helper, replacing an unconditional full flush that previously ran only after the stanza had already hit the wire. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mpevz37YPKV1SB8GNmwCqB
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
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 |
The group/status send bullet said sender-key leasing was "not implemented yet," but wacore::store::signal_cache::needs_pre_wire_flush already gates on sender_keys.wire_gate_pending (from whatsapp-rust#1027, merged before #1041) alongside the DM session lease. Left uncorrected, it also contradicted the new retry-recovery paragraph's claim that retries share "the DM/group send durability rule." Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mpevz37YPKV1SB8GNmwCqB
|
@claude please also fix the conflicts |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da30551aa9
ℹ️ 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".
| The offline drain, retry-receipt recovery, identity-change recovery, and teardown all keep their own **synchronous** flushes — they gate acks, receipts, or follow-up reads on durability and are not routed through the receive coalescer. See [Inbound Durability Hook](/advanced/inbound-durability) for the drain-batch commit ordering, which this coalescing does not change. | ||
| The offline drain, identity-change recovery, and teardown all keep their own **unconditional synchronous** flushes — they gate acks, receipts, or follow-up reads on durability and are not routed through the receive coalescer. See [Inbound Durability Hook](/advanced/inbound-durability) for the drain-batch commit ordering, which this coalescing does not change. | ||
|
|
||
| Retry-receipt recovery (`handle_retry_receipt` resending to a DM or group requester, `src/retry.rs`) instead shares the DM/group send durability rule above through one helper, `send_retry_stanza`: the session lock is released first, then `persist_signal_state_pre_wire()` flushes synchronously only if the retry's Signal advance crossed an unpersisted lease boundary, and the stanza is written to the wire only after that flush succeeds. A failed pre-wire flush aborts the retry instead of transmitting an advance it couldn't save. This replaced an earlier unconditional full flush that ran only *after* the retry stanza had already reached the wire — a crash or persistence failure in that window could reload the old chain state after the ciphertext was already sent. |
There was a problem hiding this comment.
Avoid overstating retry pre-wire durability
I checked the merged src/retry.rs/wacore/src/store/signal_cache.rs behavior behind whatsapp-rust#1041, and this guarantee is still too strong: after handle_retry_receipt drops the per-session lock, another send/retry for that device can check out the same dirty session before persist_signal_state_pre_wire() runs. The pre-wire flush skips checked-out sessions and can return Ok while the pending reservation remains, so send_retry_stanza can still write the stanza before the raised lease is actually durable. Until upstream fails/retries that case or prevents checkout across the gate, the docs should not state that the stanza is only written after a successful durable flush.
Useful? React with 👍 / 👎.
#404 (whatsapp-rust#1027 docs) merged to main and rewrote the same "Flush scheduling" bullets this branch touched. Kept #404's version of the DM/group-send bullets, downgrade warning, and new "Clean reload vs. crash recovery" section, and re-layered this branch's retry-recovery paragraph on top. Also added a caveat, prompted by review, that the pre-wire flush gate is a point-in-time check: a session checked out by a concurrent operation is skipped by flush() without failing it, so a successful flush doesn't guarantee that specific session's reservation cleared. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mpevz37YPKV1SB8GNmwCqB
|
Conflicts resolved (merged While resolving, I also verified @chatgpt-codex-connector's point above — it's correct. I confirmed against the actual code ( I added a caveat Note this is a real (if narrow) durability gap in Generated by Claude Code |
|
To use Codex here, create an environment for this repo. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 501c223400
ℹ️ 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".
| </Warning> | ||
|
|
||
| <Warning> | ||
| The pre-wire gate is a point-in-time check, not a lock held across the flush: `needs_pre_wire_flush()` inspects pending reservations once, and `flush()` skips any session or sender-key entry that is currently checked out by a concurrent `load_session`/`load_sender_key` call, still returning `Ok` for the entries it did persist. If another task checks out the same session between this send's lock release and its flush, that session's reservation can remain pending even though the flush "succeeded" — the caller proceeds to write its stanza regardless. This is a property of `SignalStoreCache` itself, not specific to retries; it affects any pre-wire-gated send that races a concurrent operation on the same session. |
There was a problem hiding this comment.
Don't describe sender keys as checked out
This warning now says flush() skips sender-key entries checked out by load_sender_key, but the implementation only has a checked-out state for sessions: SenderKeyStoreState stores Option<Arc<SenderKeyRecord>>, and get_sender_key returns a shared Arc without removing the record. In group/status send troubleshooting, this overstates the race and contradicts the immutable sender-key loading docs, so readers may believe sender-key pre-wire flushes can succeed while leaving a checked-out sender-key reservation unpersisted when that path cannot occur.
Useful? React with 👍 / 👎.
| The scheduler is generation-scoped (embeds the connection generation in its atomic state), so a reconnect during an in-flight flush needs no explicit reset: a stale worker from the previous connection cannot mutate the new generation's state, and stands down when it observes a foreign generation. | ||
|
|
||
| The offline drain, retry-receipt recovery, identity-change recovery, and teardown all keep their own **synchronous** flushes — they gate acks, receipts, or follow-up reads on durability and are not routed through the receive coalescer. See [Inbound Durability Hook](/advanced/inbound-durability) for the drain-batch commit ordering, which this coalescing does not change. | ||
| The offline drain, identity-change recovery, and teardown all keep their own **unconditional synchronous** flushes — they gate acks, receipts, or follow-up reads on durability and are not routed through the receive coalescer. See [Inbound Durability Hook](/advanced/inbound-durability) for the drain-batch commit ordering, which this coalescing does not change. |
There was a problem hiding this comment.
Don't call teardown flushes unconditional
For teardown during an active or stalled offline drain, teardown_inbound_commits_bounded does not unconditionally flush: it only flushes after a durable drain commit with no restored entries, and on timeout or non-durable entries it clears or preserves state so the server can redeliver. The new unconditional synchronous wording can make operators think disconnect/teardown always gates Signal state on a flush, which is exactly the case the teardown path intentionally avoids to prevent rowless ratchet advances.
Useful? React with 👍 / 👎.
- The point-in-time pre-wire-gate caveat wrongly extended the session checked-out skip to sender keys. SenderKeyStoreState has no checked-out variant (get_sender_key clones an Arc in place), so flush() never skips a dirty sender-key entry; scoped the caveat to DM sessions only and noted group/status sends aren't exposed. - "Teardown ... unconditional synchronous flushes" overstated teardown_inbound_commits_bounded, which only flushes on a durable drain with no outstanding entries and otherwise clears without flushing. Dropped "unconditional" and added a parenthetical. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mpevz37YPKV1SB8GNmwCqB
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
|
Verified and fixed both of @chatgpt-codex-connector's latest points, confirmed against
Generated by Claude Code |
|
To use Codex here, create an environment for this repo. |
Summary
Reflects whatsapp-rust#1041 ("fix(signal): persist retry advances before wire"), which:
handle_retry_receiptsends (for both DM and group retries) on the same pre-wire lease-durability rule that regular DM/group sends already follow, via a new shared helper,send_retry_stanza(persist_signal_state_pre_wire()→send_node()).Changes
advanced/signal-protocol.mdx— in "Flush scheduling: send vs. receive", removed "retry-receipt recovery" from the list of paths that keep their own unconditional synchronous flush (that's no longer accurate), and added a paragraph describing the new pre-wire, lease-gated durability rule the retry-resend path now shares with regular sends.Not touched:
changelog/(per project convention, that's maintained by a human).Note: whatsapp-rust-docs#404 (documenting whatsapp-rust#1027) is still open and touches nearby bullets in the same "Flush scheduling" section (the group/status lease bullet). This PR only edits the separate "retry-receipt recovery" sentence and the new paragraph following it, so the two should not conflict, but a rebase may be needed depending on merge order.
Test plan
src/retry.rsdiff for whatsapp-rust#1041 to verify the newsend_retry_stanzahelper and its call topersist_signal_state_pre_wire()against the doc wording.mint broken-links(not run locally in this environment).🤖 Generated with Claude Code
https://claude.ai/code/session_01Mpevz37YPKV1SB8GNmwCqB
Generated by Claude Code
Summary by cubic
Update
advanced/signal-protocol.mdxto reflect whatsapp-rust#1041: retries now follow the same pre‑wire, lease‑gated persistence as regular DM/group sends viasend_retry_stanza, removing the old post‑wire flush claim. Adds a DM‑only caveat that the pre‑wire gate is a point‑in‑time check (group/status sends aren’t exposed), keeps sender‑key leasing and downgrade guidance aligned with whatsapp‑rust#1027, and corrects teardown flush docs to note it is conditional.Written for commit 1935c11. Summary will update on new commits.