-
Notifications
You must be signed in to change notification settings - Fork 0
docs: reflect retry pre-wire Signal persistence (whatsapp-rust#1041) #405
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
da30551
232ad0d
501c223
1935c11
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1145,6 +1145,10 @@ impl SessionStore for SessionAdapter { | |
| Downgrading to a version that predates the counter lease after running a leased version: the older version ignores the lease field(s) and could reuse counters/iterations that were only reserved (not yet actually sent) by the lease. This applies to `SenderKeyRecord` (group/status sends) as well as `SessionRecord` (DM sends). Avoid downgrading a device's local state across this boundary. | ||
| </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. | ||
| </Warning> | ||
|
|
||
| ### Clean reload vs. crash recovery | ||
|
|
||
| Fast-forwarding past a lease's reserved ceiling on every reload is the safe default, but it's also overly conservative for the common case: a clean reconnect or a same-process store re-creation never actually risked losing an in-flight send, yet unconditionally fast-forwarding still burned a full unused batch every time. 32 clean reconnects could push a sender-key chain 2,048 iterations ahead and get rejected once a peer who missed the intervening messages hit `MAX_FORWARD_JUMPS` (2,000). | ||
|
|
@@ -1160,7 +1164,9 @@ This adds no field to the public `Device` struct and no synchronous I/O — a ma | |
|
|
||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For teardown during an active or stalled offline drain, Useful? React with 👍 / 👎. |
||
|
|
||
| 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()` runs — flushing synchronously if the retry's Signal advance crossed an unpersisted lease boundary — before the stanza is written to the wire. An `Err` from that flush aborts the retry instead of transmitting an advance it couldn't save; see the point-in-time caveat above for the narrower case where the flush reports success without having actually persisted this retry's own checked-out session. 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. | ||
|
|
||
| Call [`Client::flush_pending_signal_state()`](/api/client#flush_pending_signal_state) to force a deterministic settle — e.g. before reading persisted Signal state directly, or ahead of a non-graceful shutdown. Never call it from inside an `InboundDurabilityHook` or a synchronous, inline `EventHandler::handle_event` implementation, since settling re-enters the processing permit those run under and would deadlock during an offline-sync drain. Ordinary `Bot` closure handlers are unaffected — both default delivery modes run the callback in a detached task off the permit. | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This warning now says
flush()skips sender-key entries checked out byload_sender_key, but the implementation only has a checked-out state for sessions:SenderKeyStoreStatestoresOption<Arc<SenderKeyRecord>>, andget_sender_keyreturns a sharedArcwithout 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 👍 / 👎.