fix stale app-state recovery for chat writes#299
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed July 17, 2026, 8:04 AM ET / 12:04 UTC. Summary Reproducibility: yes. Authenticated corrupted-store runs reproduced missing-key and divergent-LTHash failures, and final-head attempts exercised the retained-recovery-debt failure path; only successful final-head completion remains unobserved. Review metrics: 3 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review detailsBest possible solution: Ship the durable replay-intent design after a clean exact-head authenticated run proves successful corrupted-store recovery, persistence of the recovered remote mutation, safe clearing of only the matching generation, and completion of the requested chat write. Do we have a high-confidence way to reproduce the issue? Yes. Authenticated corrupted-store runs reproduced missing-key and divergent-LTHash failures, and final-head attempts exercised the retained-recovery-debt failure path; only successful final-head completion remains unobserved. Is this the best way to solve the issue? Yes. Durable per-generation replay intents and a shared persistence sequencer are narrower and safer than synchronizing correctness to upstream callback-drain timing, and the final branch covers the previously identified collection, persistence, and race failures. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 8e4f8935edb9. Label changesLabel justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (8 earlier review cycles)
|
|
@clawsweeper re-review Added redacted live runtime proof for missing-key delivery, the primary-device recovery snapshot, bounded backoff without a 429, and successful archive/unarchive mutations. The branch now includes focused regression coverage for all three paths. |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper re-review Removed the PR-owned |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ca72ee981
ℹ️ 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".
|
@codex review Addressed both findings in
Validation passes for standard and |
|
@clawsweeper re-review The two app-state review findings are addressed in |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Maintainer review (deep pass with independent verification): the recovery direction is right and the corrupted-store live proof is appreciated, but the current head has an actionable race we can't ship. The temporary persistence handler can be removed after recovery advances durable whatsmeow app-state, but before asynchronous event dispatch begins. In that window, events that only exist because recovery advanced the state are dropped, which can permanently desynchronize wacli.db from the advanced upstream state. We confirmed the pre-dispatch removal window against upstream FetchAppState/recovery/event-dispatch source. To land this we need one of: durable replay-marker semantics (events persisted with a marker so a re-run replays anything dispatched after recovery advanced state), or explicit recovery-completion synchronization that keeps the handler installed until dispatch has drained. Plus a repeat of the corrupted-store live proof on the fixed head. Happy to land quickly once that hole is closed - full test/lint/race gates already pass on this head otherwise. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8a29be009
ℹ️ 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".
| err := a.wa.FetchAppState(ctx, string(collection), false, false) | ||
| if err == nil { | ||
| return nil |
There was a problem hiding this comment.
Propagate catch-up persistence errors
When this normal delta catch-up fetch succeeds after decoding app-state events, the added handler can return a database error (for example while storing an archive/mute/star/delete event), but the tracker is never active around this fetch so record ignores it and the method returns nil. In that failure case whatsmeow has advanced the app-state cursor while wacli.db missed the remote mutation, so a one-shot chat write can still leave local state permanently stale; bracket this fetch with the tracker and check it before returning success.
Useful? React with 👍 / 👎.
| if !errors.Is(fetchErr, appstate.ErrKeyNotFound) { | ||
| return fmt.Errorf("replay WhatsApp app state recovery for %s: %w", collection, fetchErr) |
There was a problem hiding this comment.
Handle LTHash mismatches during full replay
If the initial delta fetch hits ErrMismatchingLTHash, this path enters full replay, but a full replay that also returns ErrMismatchingLTHash immediately exits here without sending RequestAppStateRecovery or waiting for the primary-device snapshot. For stores whose corrupted LTHash cannot be repaired by a server full sync, the persisted recovery marker makes every later archive/mute/read write repeat the same failing full replay and the chat-state command remains blocked.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0a227d307
ℹ️ 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".
| // durable recovery intent remains after return because canonical sends | ||
| // dispatch their resulting events asynchronously. | ||
| beforeApply() | ||
| return nil, c.SendAppState(ctx, patch) |
There was a problem hiding this comment.
Return post-send app-state events
When a one-shot chat-state command runs, there is no sync event handler installed to catch whatsmeow's asynchronous post-send app-state dispatch, and this real client path always returns nil for postSendEvents. That means completeLocalAppStateWrite never persists extra mutations produced by the send's follow-up fetch (for example archive's automatic unpin mutation or conflict catch-up events), so the command can return success while wacli.db remains stale until some later full replay.
Useful? React with 👍 / 👎.
|
Maintainer repair pushed directly to the contributor branch. No contributor action needed. Exact head: Design: durable per-generation replay intents, not handler-drain timing. A marker is written before a local app-state write or replay can advance state, fetched/live/local mutations share one persistence sequencer, and only the matching completed generation is cleared. Delayed dispatch and newer generations therefore remain replayable after restart. This is cleaner than coupling correctness to upstream callback timing. Read-self receipts stay synchronous and outside app-state replay because they are not replayable collection events. Regression coverage includes the pre-dispatch removal window, delayed post-write dispatch, post-send/live ordering, generation-safe clearing, failed persistence retaining debt, request cancellation, and canonical send boundaries. Focused race proof passed 700 executions: Full exact-head gate passed: Branch AutoReview passed with no findings. Hosted exact-head checks are green: CI and Docker. Live corrupted-store rerun used the exact clean binary (SHA-256 The commands failed before remote mutation. The failure-path evidence shows the exact-head durable marker remains set and the corrupted local state is not advanced or falsely audited. A second authenticated live store was not available, so successful exact-head recovery remains externally blocked; I am not claiming the earlier success-path proof as final-head proof. Contributor commits remain intact. |
Summary
regular_lowfor archive, pin, and read state;regular_highfor mute)syncRoot cause
whatsmeowrequests missing app-state keys asynchronously when decoding a patch fails withappstate.ErrKeyNotFound. Chat-state commands previously proceeded directly toSendAppState, so a stale collection could receive a 409 conflict and the one-shot process could exit before the primary device delivered the key.Live validation exposed a second state of the same failure path: the local collection can have the expected version but a divergent LTHash. A normal delta cannot repair that condition; the primary device must provide its recovery snapshot. That response can take more than two minutes. Retrying every 250 ms while waiting also reproduced a server
429 rate-overlimit.The shared pre-write helper now targets the collection used by each mutation, waits for missing keys, requests one official recovery snapshot on an LTHash mismatch, and backs retries off to a five-second cap. During catch-up it reuses the same app-state persistence handlers as normal sync, so decoded remote mutations are not consumed without updating
wacli.db.Redacted live proof
The test used a private temporary copy of an authenticated store. Phone numbers, JIDs, request IDs, endpoints, and key IDs are redacted. The temporary stores were deleted after verification.
Missing-key delivery and connection retention
Nine recent app-state keys were deliberately removed from the temporary copy while its collection remained at version 87:
The one-shot process remained connected for 120.90 seconds. Before it exited on the independently discovered LTHash mismatch, the primary device had restored eight requested keys:
This demonstrates that the command no longer exits before asynchronous key delivery. It also provided the live reproduction for the recovery-snapshot addition.
Recovery snapshot and successful mutation
With one recovery request and exponential backoff enabled, the same stale version-87 store waited for the primary-device snapshot and completed the native archive command:
No
429occurred. An immediate native unarchive then succeeded and left the test chat in its requested final state:Propagation of the archive path was also manually confirmed in both WhatsApp Desktop and WhatsApp for iPhone.
Final-head
regular_highrecovery and persistenceThis test ran the exact clean final head and used only a private temporary copy of the authenticated store:
The temporary copy started from
regular_highversion 3. Its 128-byte LTHash was deliberately replaced with zeros, while the redacted test chat was locally unmuted. A temporary SQLite trigger, scoped to that chat, recorded every actualmuted_untiltransition:A reference mute was first written from the main store:
The final-head binary then ran an unmute from the corrupted version-3 copy. It recovered
regular_high, persisted the fetched remote mute event, and completed the unmute:Because the requested command was unmute, the first
0 -> -1transition can only be the remote mute event emitted during pre-write recovery and persisted by the temporary handler. The second-1 -> 0transition is the requested local unmute. This demonstrates both final-headregular_highrecovery andwacli.dbpersistence before the write.A final idempotent unmute reconciled the main store and left the requested user-visible state:
The authenticated temporary store, audit database, safety backup, and generated build artifact were deleted after verification.
Validation
Passed:
go test ./internal/app -count=1go test ./... -count=1go test -tags sqlite_fts5 ./... -count=1pnpm format:checkpnpm lintpnpm buildgit diff --checkpnpm testalso completed its Go,sqlite_fts5, Windows-lock, and CGO-required stages successfully. Its documentation/release-workflow stage passed 40/41 tests; the sole failure was the known local endpoint-protection block on the temporary Windowsa.out.exe(operation not permitted).