fix stale app-state recovery for chat writes#299
Conversation
|
Codex review: needs maintainer review before merge. Reviewed July 13, 2026, 11:36 AM ET / 15:36 UTC. Summary Reproducibility: yes. The contributor reproduced missing-key and mismatching-LTHash failures with deliberately stale authenticated store copies, then repeated the recovery and persistence path on the exact clean final SHA. 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:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the focused recovery and persistence changes after normal maintainer review confirms the temporary handler lifecycle, while retaining the collection-specific routing and regression coverage. Do we have a high-confidence way to reproduce the issue? Yes. The contributor reproduced missing-key and mismatching-LTHash failures with deliberately stale authenticated store copies, then repeated the recovery and persistence path on the exact clean final SHA. Is this the best way to solve the issue? Yes. A shared collection-aware pre-write catch-up that reuses established persistence handlers is narrower and safer than duplicating recovery logic in each command or proceeding with writes against stale app-state. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against b1eb33538df3. Label changesLabel changes:
Label 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 (6 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". |
|
@clawsweeper re-review Added redacted final-head live proof for the exact clean SHA The new PR-body section demonstrates:
All authenticated temporary stores, the audit database, safety backup, and generated build artifact were deleted after verification. |
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).