fix(signal): persist retry advances before wire

[jlucaso1]

Summary

• gate DM and group retry stanzas with pre-wire Signal persistence
• share the ordering in one retry send path
• keep retries covered by an existing durable lease on the write-behind path

Why

Retry encryption advances a pairwise Signal session. The previous order sent the retry stanza before persisting that advance, so a crash or persistence failure could reload the old chain state after ciphertext had already reached the wire. A send failure also skipped the post-send flush entirely.

The retry path now follows the same durability rule as regular sends: release the session lock, persist only when crossing a lease boundary, then write the stanza. This also removes the unconditional synchronous full flush after every retry.

Closes #1035

Validation

• cargo fmt --all
• cargo clippy -p whatsapp-rust --tests -- -D warnings
• cargo test -p whatsapp-rust retry_ (52 passed)
• cargo test -p whatsapp-rust --quiet (997 unit tests passed, 1 ignored; integration and doc tests passed)

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

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

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 8faa0ec5-9f12-44a4-ad58-2730de83e7c7

📥 Commits

Reviewing files that changed from the base of the PR and between 1b25262 and f629889.

📒 Files selected for processing (1)

• src/retry.rs

---

📝 Walkthrough

Summary by CodeRabbit

• Bug Fixes
  • Improved reliability of retry resend delivery by ensuring required Signal session state is persisted before resend attempts.
  • Standardized retry resend behavior across group and direct messages to reduce failed or incomplete retries.
• Tests
  • Expanded and refactored retry durability and leased session coverage, adding shared test helpers and validating behavior for both “pre-wire” and “durable lease” scenarios.

Walkthrough

Retry resend handling now persists Signal state before sending through a shared helper. Related tests use shared mock socket and leased-session setup helpers to validate pre-wire flushing and durable-lease behavior.

Changes

Retry resend durability

Layer / File(s)	Summary
Pre-wire retry sending src/retry.rs	Group and DM retry resends route through send_retry_stanza, which persists Signal state before send_node and removes the explicit post-send batch flush.
Retry durability validation src/retry.rs	Durability and lease tests add shared socket/session helpers and verify send behavior under pre-wire flush and durable-lease conditions.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant RetryReceiptHandler
  participant SignalState
  participant Transport
  RetryReceiptHandler->>SignalState: persist_signal_state_pre_wire()
  SignalState-->>RetryReceiptHandler: persistence result
  RetryReceiptHandler->>Transport: send retry stanza

Loading

Possibly related PRs

• oxidezap/whatsapp-rust#1026 — Shares the retry-send durability gate and pre-wire persistence behavior.
• oxidezap/whatsapp-rust#549 — Modifies the DM retry resend path in src/retry.rs.
• oxidezap/whatsapp-rust#559 — Refactors retry receipt handling and adjacent resend/session behavior.

Suggested labels: performance

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly matches the main change: persisting retry advances before writing retry stanzas.
Description check	✅ Passed	The description matches the code changes and explains the durability and flush ordering fix.
Linked Issues check	✅ Passed	Retry stanzas now persist Signal advances pre-wire in both DM and group paths, with tests covering lease and flush behavior.
Out of Scope Changes check	✅ Passed	The changes stay focused on retry durability ordering and regression tests, with no unrelated scope visible.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/retry-pre-wire-durability

---

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

This PR fixes a durability ordering bug on the retry send path: Signal session advances were previously written to the wire before the ratchet counter lease was persisted, so a crash between send_node and the unconditional post-send flush_signal_cache_batch_safe could leave the old chain state on disk while the ciphertext was already delivered. The fix introduces a small send_retry_stanza helper that calls persist_signal_state_pre_wire before send_node, matching the durability rule used by regular sends; a failed persistence now aborts before the stanza reaches the wire.

• Both the group and DM retry branches are unified through the new send_retry_stanza helper, eliminating duplicated send+flush sequences.
• The unconditional synchronous full-flush after every retry is removed; sessions covered by an existing durable counter lease now ride the coalesced write-behind worker instead of paying a serialize+transaction per retry.
• Two new unit tests cover the critical invariants: a persistence failure must abort before send_node is observed, and a durable-lease-covered retry must not trigger a synchronous flush.

Confidence Score: 5/5

Safe to merge; the change correctly moves Signal state persistence before the wire send, and the two new tests directly verify the pre-wire abort and durable-lease-skip paths.

The ordering fix is narrow and well-contained: both retry branches are unified through a single helper, the helper calls the same pre-wire gate already used by regular sends, and the session lock is released before any call that could acquire the processing permit. The two new tests exercise the exact failure modes the old code left open. No pre-existing durability contracts are weakened.

No files require special attention.

Important Files Changed

Filename	Overview
src/retry.rs	Extracts send_retry_stanza helper that gates both group and DM retry paths on persist_signal_state_pre_wire before send_node, fixing the ordering bug; adds two unit tests that verify the failure-abort and durable-lease-skip behaviours

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant H as handle_retry_receipt
    participant L as session_lock
    participant E as encrypt (prepare_*_retry_stanza)
    participant G as send_retry_stanza
    participant P as persist_signal_state_pre_wire
    participant B as flush_signal_cache_batch_safe
    participant W as send_node (wire)

    H->>L: acquire session lock
    H->>E: encrypt stanza (advances Signal chain)
    E-->>H: stanza
    H->>L: drop session lock (pre-wire gate may take processing permit)
    H->>G: send_retry_stanza(stanza)
    G->>P: persist_signal_state_pre_wire()
    alt "needs_pre_wire_flush() == true"
        P->>B: flush_signal_cache_batch_safe()
        B-->>P: Ok / Err
        P-->>G: propagate result
        G-->>H: Err abort, never reaches wire
    else covered by durable lease
        P->>P: schedule_signal_flush (coalesced)
        P-->>G: Ok
    end
    G->>W: send_node(stanza)
    W-->>G: Ok
    G-->>H: Ok

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant H as handle_retry_receipt
    participant L as session_lock
    participant E as encrypt (prepare_*_retry_stanza)
    participant G as send_retry_stanza
    participant P as persist_signal_state_pre_wire
    participant B as flush_signal_cache_batch_safe
    participant W as send_node (wire)

    H->>L: acquire session lock
    H->>E: encrypt stanza (advances Signal chain)
    E-->>H: stanza
    H->>L: drop session lock (pre-wire gate may take processing permit)
    H->>G: send_retry_stanza(stanza)
    G->>P: persist_signal_state_pre_wire()
    alt "needs_pre_wire_flush() == true"
        P->>B: flush_signal_cache_batch_safe()
        B-->>P: Ok / Err
        P-->>G: propagate result
        G-->>H: Err abort, never reaches wire
    else covered by durable lease
        P->>P: schedule_signal_flush (coalesced)
        P-->>G: Ok
    end
    G->>W: send_node(stanza)
    W-->>G: Ok
    G-->>H: Ok

Loading

_{Reviews (2): Last reviewed commit: "refactor(test): pass client directly to ..." | Re-trigger Greptile}

[github-actions]

📦 Binary size report

Metric	main	PR	Δ
bin size (stripped)	10.89 MiB	10.89 MiB	+128 B (+0.00%) 🔺
bin .text	8.88 MiB	8.88 MiB	0
bin allocated (text+data+bss)	10.89 MiB	10.89 MiB	-16 B (-0.00%) 🔽
llvm-lines wacore	508,537	508,537	0
llvm-lines wacore copies	17,438	17,438	0
llvm-lines whatsapp-rust lib	774,302	774,379	+77 (+0.01%) 🔺
llvm-lines whatsapp-rust lib copies	25,188	25,193	+5 (+0.02%) 🔺
deps crates (Cargo.lock)	472	472	0

.text per crate

Crate	main	PR	Δ
.text whatsapp_rust	1.67 MiB	1.67 MiB	+235 B (+0.01%) 🔺
.text wacore	530.46 KiB	530.46 KiB	0
.text wacore_binary	148.45 KiB	148.45 KiB	0
.text wacore_libsignal	195.25 KiB	195.25 KiB	0
.text wacore_appstate	158.25 KiB	158.25 KiB	0
.text wacore_noise	26.03 KiB	26.03 KiB	0
.text waproto	1.60 MiB	1.60 MiB	0
.text whatsapp_rust_sqlite_storage	513.08 KiB	513.08 KiB	0
.text whatsapp_rust_tokio_transport	43.69 KiB	43.69 KiB	0
.text whatsapp_rust_ureq_http_client	10.47 KiB	10.47 KiB	0
.text std	1.01 MiB	1.01 MiB	-238 B (-0.02%) 🔽
.text other deps	2.95 MiB	2.95 MiB	0

Baseline: 0e1db5762 (latest main run) · Head: b3e2eb3a9 · Graphs

[coderabbitai]

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/retry.rs`:
- Around line 1321-1340: Update the test helpers attach_mock_noise_socket and
seed_retry_lease to accept &Client instead of &Arc<Client>. Keep their existing
behavior unchanged and rely on deref coercion at current call sites.

🪄 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: e3d03abb-a2ea-43c9-8bfc-3182d5cf4d17

📥 Commits

Reviewing files that changed from the base of the PR and between 0e1db57 and 1b25262.

📒 Files selected for processing (1)

• src/retry.rs

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: f62988997f

ℹ️ 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]

Keep retry leases gated until they are actually persisted

When another retry/send for the same device starts after the branch drops _session_guard but before this pre-wire flush takes the sessions-cache lock, it can check out the just-advanced session. flush_signal_cache_batch_safe() then skips that CheckedOut session and returns Ok with reservation_pending still set, so this line can proceed to send_node even though the raised counter lease was not written; a crash before the other operation checks the session back in and flushes can reload the pre-lease counter and reuse outbound key material. The retry path needs to fail/retry if the gate leaves a pending reservation, or otherwise prevent the session from being checked out across the gate.

Useful? React with 👍 / 👎.