fix(appstate): report what a batched sync actually achieved - #1207
Conversation
The batched sync answered `Ok(())` in three cases where a collection had not synced: a per-collection fatal error, the iteration cap running out, and every collection being filtered by the in-flight dedup. The initial bootstrap reads that `Ok` as permission to cancel its retry watchdog and dispatch Connected, so a `critical_block` that failed any of those ways left the session connected without the settings it carries, push name included, and with nothing scheduled to retry. Turning them into `Err` is not the fix: the bootstrap never clears `needs_initial_full_sync` on that path, so a 400/404 would reconnect into the same state every 180s, and `needs_pushname_from_sync` is derived from the persisted push name, so it would survive a restart too. So the call now reports a per-collection outcome and each caller decides. The bootstrap connects degraded on a refusal and emits AppStateSyncFailed instead of looping; WA Web notifies the primary and logs out there, which a library must not do on a consumer's behalf, so it hands over the facts and keeps the session. Retryable and skipped keep the watchdog armed. Also from re-reading WAWebSyncdServerSync: - ErrorRetry belongs in doneCollections, never refetchCollections. We refetched it inside the same loop, which the cap below would have turned into 500 attempts at a collection that just failed. - The cap is 500, not 5. The loop reads `(l < y || (i.length > 0 && l < C))` with y = 5 and C = 500, and since the body only runs while there is something to refetch, that collapses to `l < 500`. Rounds are back-to-back there too; the syncd backoff spaces retries of a failed collection, not pages of one still making progress. SyncInFlight now records whether a sync or a patch send holds a reservation. Skipping is only sound behind an equivalent sync, and the batched path was skipping behind patch sends, which never fetch. Waits are bounded, because the sync worker's intake loop runs non-history tasks inline and a wedged holder would stall everything behind it. Finally, history sync's `-` placeholder no longer becomes our push name. Storing it makes push_name non-empty for good, so the bootstrap stops looking for the real name and the account announces `-` on every reconnect. The authoritative setting_pushName mutation does not come through that path, so a genuine `-` still arrives.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughApp-state synchronization now uses typed reservations, bounded waits, retries, and structured collection outcomes. Connection and background handlers report failures through events. History-sync pushname parsing filters the exact absent-name sentinel while preserving dashed names. ChangesApp-state synchronization
History-sync pushname parsing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Connection
participant Client
participant SyncInFlight
participant EventBus
Connection->>Client: sync_collections_batched
Client->>SyncInFlight: acquire holder-aware reservations
Client-->>Connection: BatchedSyncOutcome
alt fatal collections
Connection->>EventBus: dispatch AppStateSyncFailed
Connection-->>Connection: continue without refused collections
else retryable or incomplete collections
Connection->>EventBus: report sync failure
Connection-->>Connection: retain reconnect retry
else all collections synced
Connection-->>Connection: complete connection setup
end
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
| Filename | Overview |
|---|---|
| src/client/app_state.rs | Introduces scoped admission, collection-level outcomes, typed reservations, bootstrap settlement, and retry scheduling for app-state synchronization. |
| src/client/node_io.rs | Updates critical bootstrap handling so fatal and recoverable partial outcomes receive distinct connection and retry policies. |
| wacore/src/appstate_sync.rs | Extracts per-collection patch-list processing so callers can enforce lifecycle admission between collection applications. |
| wacore/src/types/events.rs | Adds the structured app-state synchronization failure event and pins its append-only event-kind value. |
| wacore/src/history_sync.rs | Prevents the history-sync placeholder value from becoming the persisted push name. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Critical app-state bootstrap] --> B[sync_collections_batched]
B --> C{Outcome}
C -->|All synced| D[Settle bootstrap and publish Connected]
C -->|Fatal present| E[Keep bootstrap armed and connect degraded]
E --> F[Retry recoverable misses in background]
C -->|Retryable or skipped only| G[Keep watchdog and bootstrap armed]
G --> H[Reconnect and retry]
F --> I{Connection generation changed?}
I -->|Yes| J[Rebind work and relinquish bootstrap authority]
I -->|No| K[Report outcome and settle when complete]
Reviews (19): Last reviewed commit: "fix(appstate): give the gate a tag and s..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
coderabbitai
Bot
added
api-design
breaking-change
size-increase-ok
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/client/node_io.rs`:
- Around line 1378-1393: Update the critical app-state sync outcome handling
around dispatch_app_state_sync_failed so the terminal degraded-connect path runs
only when all missing collections are fatal. If outcome.retryable or
outcome.skipped is non-empty, preserve the retry/watchdog flow and do not clear
needs_initial_full_sync; abort the watchdog and dispatch_connected only for an
all-fatal outcome.
🪄 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: 173eaea5-e7ca-4a23-b63a-ef9d84786e6c
📒 Files selected for processing (6)
src/client/app_state.rssrc/client/node_io.rssrc/handlers/ib.rssrc/handlers/notification/groups.rswacore/src/history_sync.rswacore/src/types/events.rs
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a97a85995b
ℹ️ 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".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Ta pouco de review ou quer mais? |
Five findings, all real and all verified against the code rather than taken at face value. A mixed outcome lost the recoverable collection. The refused arm connects and falls through to the background spawn, which clears needs_initial_full_sync, so a critical_block 404 alongside a critical_unblock_low 503 left the 503 unsynced with nothing to retry it. Making the arm terminal only when every miss is fatal, as suggested, brings back the reconnect loop the whole change exists to avoid: the refusal fails again every time. So the refused path now hands its recoverable misses to the background sync that already follows. A retryable collection had no retry at all. Routing ErrorRetry out of the refetch loop matches WA Web, but WA Web hands it to a retry state machine afterwards and we had none, so a single 500 left the collection stale until an unrelated trigger. schedule_app_state_retry is that machine, with WA Web's spacing (1s, doubling, capped at an hour) and bound to the connection it was scheduled on. The persisted two-day expiry still isn't here; that needs a store column. Outcomes could be published from a retired socket. Both bootstrap arms dispatched AppStateSyncFailed before checking the generation, so a consumer following the documented fatal policy could have logged out a healthy replacement session. Both now check first. A response that omits a requested collection classified it as nothing at all, so all_synced() reported a batch that never covered it — the same false success this change is about. The response is now reconciled against what was asked for, with duplicates ignored rather than applied twice. Two smaller ones: the reservation wait was shorter than a healthy patch send, which can legitimately run five attempts of DEFAULT_IQ_TIMEOUT, so it is now derived from those two constants instead of guessed; and report_background_sync claimed the session was connected when a dirty-bit sync can run before Connected, so it reads the readiness flag. A wait that times out is now retryable rather than skipped. Skipped means an equivalent sync already has the collection, which is the only case a caller may ignore.
coderabbitai
Bot
removed
the
size-increase-ok
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/client/node_io.rs (1)
1456-1461: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep the initial-sync requirement after a global background failure.
sync_collections_batchedcan returnErrfor a global failure.report_background_synconly logs that error. Line 1461 then clearsneeds_initial_full_sync, so a later reconnect does not retry the deferred critical collection.Clear
needs_initial_full_synconly after anOk(...)outcome. Keep it set whenresultisErr.🤖 Prompt for 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. In `@src/client/node_io.rs` around lines 1456 - 1461, Update the background sync flow around sync_collections_batched and report_background_sync so needs_initial_full_sync is cleared only when the sync result is Ok. Preserve the flag when result is Err, allowing a later reconnect to retry the deferred critical collection.src/client/app_state.rs (2)
1540-1544: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the patch-send reservation wait.
SyncInFlightRegistry::beginwaits indefinitely. If a sync holder leaks,send_app_state_patchnever returns to its caller. This bypasses the reservation timeout added for sync requests.Wrap this acquisition with
rt_timeout(..., APP_STATE_RESERVATION_WAIT, ...). Return an error when the wait expires.🤖 Prompt for 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. In `@src/client/app_state.rs` around lines 1540 - 1544, Update the patch-send acquisition in send_app_state_patch to wrap app_state_syncing.begin(name, SyncHolder::PatchSend) with rt_timeout using APP_STATE_RESERVATION_WAIT, and return an error when the timeout expires; preserve the existing Some(name) handling and successful reservation result.
901-907: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject response collections that were not requested.
process_patch_listsreceives every parsed response collection. An unrequested collection has no reservation inguards. The later loop can dispatch its mutations and persist its version concurrently with a sync or patch send for that collection.Build a requested-name set from
pending. Filter or reject unrequestedpatch_listsbeforeprocess_patch_lists. Continue to mark requested-but-omitted collections as retryable.Proposed fix
+ let requested: HashSet<WAPatchName> = pending.iter().copied().collect(); let mut patch_lists = wacore::appstate::patch_decode::parse_patch_lists_ref(resp.get())?; + patch_lists.retain(|list| { + if requested.contains(&list.name) { + true + } else { + warn!( + target: "Client/AppState", + "Batched sync: response included unrequested collection {:?}", + list.name + ); + false + } + });Also applies to: 969-977
🤖 Prompt for 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. In `@src/client/app_state.rs` around lines 901 - 907, Update process_patch_lists to derive the requested collection-name set from pending and filter or reject parsed patch_lists entries not present in that set before calling process_patch_lists. Preserve the existing retryable handling for requested collections omitted from the response, and apply the same validation to the related flow around the later referenced logic.
🤖 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/client/app_state.rs`:
- Around line 706-716: Update the scheduled retry handling around
client.sync_collections_batched so that, after the await, it revalidates the
connection generation and dispatches an AppStateSyncFailed event for
outcome.fatal before returning when no retryable items remain. Preserve the
existing retryable assignment and terminal handling for refusal and skip
outcomes.
---
Outside diff comments:
In `@src/client/app_state.rs`:
- Around line 1540-1544: Update the patch-send acquisition in
send_app_state_patch to wrap app_state_syncing.begin(name,
SyncHolder::PatchSend) with rt_timeout using APP_STATE_RESERVATION_WAIT, and
return an error when the timeout expires; preserve the existing Some(name)
handling and successful reservation result.
- Around line 901-907: Update process_patch_lists to derive the requested
collection-name set from pending and filter or reject parsed patch_lists entries
not present in that set before calling process_patch_lists. Preserve the
existing retryable handling for requested collections omitted from the response,
and apply the same validation to the related flow around the later referenced
logic.
In `@src/client/node_io.rs`:
- Around line 1456-1461: Update the background sync flow around
sync_collections_batched and report_background_sync so needs_initial_full_sync
is cleared only when the sync result is Ok. Preserve the flag when result is
Err, allowing a later reconnect to retry the deferred critical collection.
🪄 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: 9a96608a-dde6-440f-9333-61c0e01366ee
📒 Files selected for processing (4)
src/client/app_state.rssrc/client/node_io.rssrc/request.rswacore/src/types/events.rs
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a778769090
ℹ️ 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".
Seven findings, several raised by more than one reviewer. Duplicate collections were reconciled too late. The processor persists every list it is handed, so a repeated <collection> was applied twice before the bookkeeping guard saw it, and the second application could move the MAC store past the version the first then wrote back. Duplicates are now dropped from the parsed lists before anything applies them. The retry loop swallowed a later refusal. A round that turns a transient failure into a 400/404 left the consumer holding only the earlier transient report, which is the opposite of the event's contract. Each round now publishes a non-synced outcome before deciding whether it is finished, and re-checks the generation first because it just awaited a round trip. The background sync reported unconditionally. It checked the generation before its await but not before publishing, so a retired socket's outcome could reach a consumer who would apply its fatal policy to the live session. It now re-checks after the await, like the critical arms. needs_initial_full_sync was cleared even when the background run left something unsynced, which makes the next connection skip the bootstrap while the retry that would have covered the gap dies with this generation. It is now cleared only on a complete run. The refused arm claimed connected: true before Connected was published, so a disconnect during the presence resubscribe would have made that a lie. It now reports after the readiness transition and reads the flag. WaitTimedOut is no longer lost on the single-collection task path, which dropped the request outright. It goes to the retry scheduler. That also settles the reservation bound: a holder's honest worst case is not derivable, since a patch send keeps the reservation across a re-sync that may page up to MAX_PAGINATION_ITERATIONS, so what makes the bound safe is that expiring is never lossy rather than the number itself. Event::AppStateSyncFailed moved to the end of the enum. Event derives Serialize, and an index-based format keys variants by position, so inserting it beside its relatives renumbered every later variant.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
coderabbitai
Bot
added
the
size-increase-ok
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/app_state.rs (1)
1584-1588: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the patch-send reservation wait.
send_app_state_patchholdsapp_state_send_lockbefore callingself.app_state_syncing.begin(name, SyncHolder::PatchSend), and thatbegin()loop has no timeout. A stalled holder will block later patch sends. Wrap the reservation acquisition withrt_timeout(&*self.runtime, APP_STATE_RESERVATION_WAIT, ...), return the timeout as retryable work, and only send the patch after obtaining the reservation. No patch send should proceed without it.🤖 Prompt for 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. In `@src/client/app_state.rs` around lines 1584 - 1588, Update send_app_state_patch around app_state_syncing.begin(name, SyncHolder::PatchSend) to acquire the reservation through rt_timeout using APP_STATE_RESERVATION_WAIT. Treat a timeout as retryable work, and ensure the patch-send path proceeds only after the reservation is successfully obtained.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/client/app_state.rs`:
- Around line 1584-1588: Update send_app_state_patch around
app_state_syncing.begin(name, SyncHolder::PatchSend) to acquire the reservation
through rt_timeout using APP_STATE_RESERVATION_WAIT. Treat a timeout as
retryable work, and ensure the patch-send path proceeds only after the
reservation is successfully obtained.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c7ababa6-deab-4242-aca9-fff9d3feca89
📒 Files selected for processing (3)
src/client/app_state.rssrc/client/node_io.rswacore/src/types/events.rs
… buckets The timeout test only constructed an outcome and asserted bucket semantics, so it passed whether or not a real timed-out wait landed in `retryable` — the same false coverage this branch called out elsewhere. It now holds a patch-send reservation that never releases and drives the batched sync through the bound, under paused time so it does not sit there for the full wait. Flipping the classification back to `skipped` fails it. Also: DEFAULT_IQ_TIMEOUT's comment claimed a ceiling it does not have, since both IQ entry points take a caller-supplied timeout. It documents the default path, which is the basis app-state derives its wait from.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 667d22e866
ℹ️ 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".
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The server_sync handler checked the generation before its sync and not after, so a connection replaced while blobs or patches were still being processed would still have its outcome published — and a consumer acting on a fatal AppStateSyncFailed would apply logout or recovery to the healthy session that replaced it. The dirty-bit handler had the same exposure and did not track a generation at all. Rather than repeat the guard at each call site, report_background_sync takes the generation the sync started on and drops anything else. Every caller awaits a round trip before reporting, so the one that forgets is the one that matters; making it a parameter means a new call site cannot skip it silently. The background bootstrap sync keeps its own check for the flag it clears afterwards, which the report no longer reaches.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c46440502e
ℹ️ 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".
…error A reservation wait that ran out consumed a retry round even though it never reached the server, so a writer holding the collection through the whole budget could drop the caller's request without the sync being tried once. Attempts and rounds are counted separately now: only a reservation actually taken spends an attempt, and the loop is still bounded overall. The critical sync's Err arm left the bootstrap flag alone. A batch can fail partway, after critical_block already dispatched and persisted setting_pushName, and the watchdog's reconnect would then see a populated push name with a clear flag, take the ordinary path, and never retry the rest. Armed here as in the incomplete arm beside it.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52e46b49d4
ℹ️ 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".
…onnection is_shutting_down() is true for a planned reconnect as well as a stop, so a retry waking inside a 515 or reconnect_immediately window returned and dropped its collections for good — the rebinding branch added for exactly that case sat below the check and was never reached. Both retry loops now end only when the client is actually stopping, and the reconnect is handled by rebinding. The task retry discarded its request on a generation change. Nothing on the replacement connection re-issues a consumer's task, and a full_sync: true one is the snapshot request that scheduler exists to preserve, so a reconnect was silently losing it. It rebinds too. Re-arming the bootstrap on a critical-sync error was not generation-aware. A late error from a socket already replaced would re-arm a gate the replacement had just stood down, costing it a 180s bootstrap it did not need.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b5c6b3de2
ℹ️ 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".
| "Retrying app state {pending:?} (round {}/{APP_STATE_RETRY_MAX_ROUNDS})", | ||
| round + 1 | ||
| ); | ||
| match client.sync_collections_batched(pending.clone(), None).await { |
There was a problem hiding this comment.
When an ordinary reconnect enters a long backoff, is_running remains true while no transport exists, so this call immediately fails on every round rather than waiting for the replacement socket. The eight delays total only 255 seconds, while src/client/lifecycle.rs permits reconnect backoffs up to 900 seconds; all attempts can therefore be exhausted and pending dropped before reconnection, leaving a consumed dirty-bit or server_sync request stale indefinitely. Fresh evidence beyond the earlier reconnect comment is that the new generation-rebinding logic still does not wait for connection readiness.
Useful? React with 👍 / 👎.
| client | ||
| .needs_initial_full_sync | ||
| .store(false, Ordering::Relaxed); |
There was a problem hiding this comment.
If another runtime thread retires the connection after the generation check at line 915 but before this store, the old retry can clear needs_initial_full_sync for the replacement generation. In particular, it can overwrite a conservative re-arm made after the replacement's critical sync fails, causing a later reconnect with a populated push name to skip the unfinished bootstrap. Fresh evidence beyond the earlier settlement-race comment is that the analogous node_io.rs settlement now rechecks after clearing, while this newly added retry settlement remains unguarded.
Useful? React with 👍 / 👎.
Every app-state path awaits round trips, and between them the socket can retire or the bootstrap's watchdog can fire. That was checked by hand at each boundary, and the checks drifted: some paths grew one the next forgot, the same load answered two different questions, and the last nine review rounds were almost entirely this one class of mistake — including several introduced while fixing others in it. SyncScope carries the connection the work belongs to and the clock it runs against. Client::admits is the only place either question is asked, and it returns why, so callers can tell "this socket is gone" from "this ran out of time" — a retired scope must not write, an expired one on a live connection still may. settle_bootstrap is the same idea for the shared gate. It was written from five places, each responsible for remembering the generation check; two forgot, in opposite directions. There are now no direct writes to needs_initial_full_sync outside it. The apply boundary is per collection. process_patch_lists persists each list as it goes, so a batch admitted at the start could still be writing its third collection well outside the scope. process_one_patch_list exposes that step so the client can stop between collections, capping what a lost scope can commit at one instead of the whole batch. Counts, before and after: ten ad-hoc generation reads and three separate deadline checks become two reads inside the scope machinery, three explicit rebinds, and one predicate. Also fixed while reading the diff whole, none of which any review caught: schedule_app_state_task_retry carried schedule_app_state_retry's doc comment while the latter had no summary line; three rationale blocks had accumulated superseded copies of themselves, documenting one branch with two contradictory explanations; and one doc still promised that retries never fire against a new socket, which stopped being true when they started rebinding.
process_app_state_sync_task answers Ok(()) at its own shutdown guard without contacting the server, and expected_disconnect makes that guard true for an ordinary reconnect as well as a stop. Swapping the retry loop's guard to is_running let it attempt during that window and take the no-op for a completed sync, dropping the consumer's request — a full_sync snapshot included — without ever asking for it. The retry now holds through a planned reconnect instead of spending an attempt on it. That window is exactly what the rebind below it was added to survive.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d10dbc6fdd
ℹ️ 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".
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Six findings, all of the same shape now that the structure is in place: one more await that had no admission check after it. The results loop dispatched mutations and wrote versions without re-asking. process_one_patch_list awaits, so a scope could be lost between a collection being decoded and its mutations reaching consumers. Everything decoded so far is persisted by its own call, so stopping leaves a consistent prefix. The task retry's reservation wait is an await too, and the reconnect can start inside it — after which the callee answers Ok(()) at its shutdown guard and the loop reads a no-op as a completed sync. Re-asked before an attempt is counted. settle_bootstrap's check and store are two operations with the generation moving under a lock it does not hold. It now undoes a write it discovers was meant for a connection that retired between them, so the live one keeps the value it had. The dirty-bit resync rebinds after the clean IQ instead of returning. The server has already accepted the bit as clean and will not raise it again, and an ordinary reconnect with a known push name runs no bootstrap, so returning left those collections stale indefinitely. The fatal-critical branch arms the gate before publishing its event. Publishing runs consumer handlers synchronously, and one forcing a reconnect would retire the task that settles it — leaving the flag false with the push name already populated, so the replacement skips a bootstrap it still owes. A retry sequence that exhausts its budget now says so. Consumers heard about every round that produced buckets, but a sequence whose rounds all failed before producing any finished in silence with the collections still stale. Also fixes the Rustdoc CI failure: an intra-doc link to a sibling method needs the Self:: prefix. cargo clippy does not run rustdoc lints, which is why the local checks were clean — cargo doc is now part of them.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5f669bc73
ℹ️ 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".
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Confidence score: 2/5
- In
src/client/app_state.rs, the generation handoff has a race because admission and bootstrap-gate update are split across separate atomics, so a replacement connection can miss bootstrap or have its gate overwritten by an old scope; this can leave client state improperly initialized and cause hard-to-reproduce connection regressions — make the handoff atomic (e.g., single CAS/critical section tied to generation) so only the active scope can set the bootstrap gate.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/client/app_state.rs">
<violation number="1" location="src/client/app_state.rs:851">
P1: A replacement connection can skip its initial bootstrap, or have its bootstrap gate clobbered, during a generation handoff. The admission check and gate update are separate atomics, so an old scope can write between the new connection's generation/read checks and the rollback can overwrite a newer value; synchronizing the generation and gate update or storing a generation-tagged gate would make this transition atomic.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The post-apply admission check I added last round loses data. process_one_patch_list persists the collection's version and mutation MACs before returning, so by the time the results loop runs the cursor has already advanced. Skipping the dispatch there and marking the collection retryable means the retry asks from the new version, the server never resends, and the decoded mutations are gone — setting_pushName and the NCT salt among them. I took the suggestion to check later without establishing whether the callee persisted. It does. The scope is checked before the apply, which is the last moment a collection can still be declined; after it, dispatching is not optional. The comment there now says so, since the next reader will have the same instinct I did. Three more, all the bootstrap gate losing track of what it owes: The fatal arm armed the gate after dispatch_connected, with a generation check between them that returns the whole task — so a Connected handler forcing a reconnect meant the gate was never armed at all. A refusal makes the bootstrap unfinished whatever happens next, so it is armed first, before anything a consumer handler can interrupt. A refused collection was absent from the background settlement's accounting: it is not in critical_retry, because retrying it is pointless, so a clean background run stood the gate down for a collection that never synced. It is carried explicitly now. The dirty-bit resync rebound before its batch but not after, so a reconnect during the sync had the outcome — and the retry with it — discarded for collections whose dirty bit the server already considers clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 459c5a6655
ℹ️ 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".
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Two things had now been patched three times each, at three different lines, and each patch introduced the next hazard. Both were the same shape: a value that cannot express what callers need to say. The bootstrap gate is generation-tagged. A plain flag could not be settled safely — deciding whether the writer still owns the connection and writing were two operations, and bridging them by checking first missed a retirement in the gap while rolling back afterwards clobbered whatever the replacement had written. The tag makes the write a single compare-and-swap that only ever moves forward, so an older connection is refused outright. The admission check stays: the tag stops a stale write overwriting a newer one, the check stops a retired writer having a say at all, and neither covers the other's case. is_shutting_down answered two questions at once — going away for good, and between connections — and every caller that outlives a connection wanted only one of them. It kept reading "reconnect" as "stop" and throwing work away. There are now is_stopping and is_reconnecting, and the callers that must survive a reconnect use the first. The remaining four follow from those: A retry whose attempt was cut short is no longer a completed one. process_app_state_sync_task breaks its pagination and answers Ok(()) as soon as it observes a shutdown, so a reconnect starting mid-attempt ended the scheduler and lost the request, full_sync included. The dirty-bit resync reports through a planned reconnect instead of being dropped by it. The server already considers the bit clean and nothing would ask again. A refused critical collection is carried into the retry scheduler's stranded state. It is not in the requested set — retrying it is pointless — but it is why the bootstrap is unfinished, and a later clean round would otherwise settle the gate on its behalf. A run that crossed its deadline no longer reports its collections as synced. They are applied and persisted, but calling the run clean let the bootstrap abort its watchdog and dispatch Connected on an expired scope, which is what the deadline exists to prevent; they go down the retry path and resume from the versions already stored.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a2c35c9f2
ℹ️ 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".
| self.0 | ||
| .store(Self::encode(u64::MAX >> 1, true), Ordering::Release); |
There was a problem hiding this comment.
arm_for_pairing stores the largest representable generation tag, but settle rejects every writer whose generation is lower. Consequently, after any fresh pairing, the successful bootstrap's settle_bootstrap(scope, false) cannot clear the gate for any practical connection_generation; every later reconnect is misclassified as an initial sync and repeatedly runs the critical bootstrap, potentially delaying readiness by its 180-second watchdog. Use a pairing marker that the next connection can consume rather than a tag that permanently outranks all connections.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
4 issues found across 6 files (changes from recent commits).
Confidence score: 3/5
- In
src/client/node_io.rs(settlevs pairing inarm_for_pairing), the stored bootstrap generation can be initialized above any real writer generation, so post-pair connections never satisfy the gate and the client can stay stuck disconnected — align the pairing seed and settle comparison so a valid first connection can progress. - In
src/client/lifecycle.rs(reconnect()), planned reconnects are not marked withexpected_disconnect, so retry logic treats an intentional transport swap as an offline failure and runs against a dead socket — set the reconnect intent flag before teardown so retries remain quarantined during handoff. - In
src/client/lifecycle.rs(is_running) andsrc/handlers/ib.rs(is_stopping()path), lifecycle state can flip retry tasks off too early for direct-connect clients, while shutdown/error handling can still emit dirty-sync failure + retry during terminal logout/conflict — tighten state predicates around terminal-stop vs reconnect transitions and gate retry/failure publication on the same terminal flag.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/client/node_io.rs">
<violation number="1" location="src/client/node_io.rs:1248">
P2: After a fresh pairing (pair.rs `arm_for_pairing` writes generation `u64::MAX >> 1`), no real connection can ever settle the bootstrap gate, because `settle` refuses any writer whose generation is lower than the stored one. The result is that `is_armed()` here (this changed line) stays true for the life of the session, so every reconnect re-enters the Full App State Sync path: re-fetching all critical collections, re-arming watchdogs and re-dispatching `Connected` even after a fully successful bootstrap. Root cause is in `BootstrapGate::arm_for_pairing` in app_state.rs (deviates from the 'generation zero owns the first write' contract on `new`); consider arming with a low generation the first real connection outranks, e.g. `Self::encode(0, true)`, or otherwise ensuring a post-pairing connection can stand the gate down.</violation>
</file>
<file name="src/handlers/ib.rs">
<violation number="1" location="src/handlers/ib.rs:166">
P2: Terminal logout/conflict handling can still publish a dirty-sync failure and enqueue a retry before `is_running` is cleared, because `is_stopping()` does not reflect the earlier `enable_auto_reconnect = false` transition. The guard should also reject the terminal auto-reconnect-disabled state while continuing to allow planned reconnects.</violation>
</file>
<file name="src/client/lifecycle.rs">
<violation number="1" location="src/client/lifecycle.rs:92">
P2: Direct-connect clients are now treated as permanently stopping, so app-state retry tasks cancel even while their socket is usable. `is_running` tracks the supervision loop rather than terminal shutdown; this predicate should use the terminal shutdown signal or a separate terminal-state flag instead.</violation>
<violation number="2" location="src/client/lifecycle.rs:98">
P2: Ordinary planned reconnects are not recognized as reconnecting: `reconnect()` tears down the transport without setting `expected_disconnect`, so app-state retries run against the offline socket instead of remaining queued for the replacement connection. Include the intentional/disconnected state here while excluding terminal shutdown.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| check_generation!(); | ||
|
|
||
| let flag_set = client_clone.needs_initial_full_sync.load(Ordering::Relaxed); | ||
| let flag_set = client_clone.needs_initial_full_sync.is_armed(); |
There was a problem hiding this comment.
P2: After a fresh pairing (pair.rs arm_for_pairing writes generation u64::MAX >> 1), no real connection can ever settle the bootstrap gate, because settle refuses any writer whose generation is lower than the stored one. The result is that is_armed() here (this changed line) stays true for the life of the session, so every reconnect re-enters the Full App State Sync path: re-fetching all critical collections, re-arming watchdogs and re-dispatching Connected even after a fully successful bootstrap. Root cause is in BootstrapGate::arm_for_pairing in app_state.rs (deviates from the 'generation zero owns the first write' contract on new); consider arming with a low generation the first real connection outranks, e.g. Self::encode(0, true), or otherwise ensuring a post-pairing connection can stand the gate down.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/client/node_io.rs, line 1248:
<comment>After a fresh pairing (pair.rs `arm_for_pairing` writes generation `u64::MAX >> 1`), no real connection can ever settle the bootstrap gate, because `settle` refuses any writer whose generation is lower than the stored one. The result is that `is_armed()` here (this changed line) stays true for the life of the session, so every reconnect re-enters the Full App State Sync path: re-fetching all critical collections, re-arming watchdogs and re-dispatching `Connected` even after a fully successful bootstrap. Root cause is in `BootstrapGate::arm_for_pairing` in app_state.rs (deviates from the 'generation zero owns the first write' contract on `new`); consider arming with a low generation the first real connection outranks, e.g. `Self::encode(0, true)`, or otherwise ensuring a post-pairing connection can stand the gate down.</comment>
<file context>
@@ -1245,7 +1245,7 @@ impl Client {
check_generation!();
- let flag_set = client_clone.needs_initial_full_sync.load(Ordering::Relaxed);
+ let flag_set = client_clone.needs_initial_full_sync.is_armed();
let needs_initial_sync = flag_set || needs_pushname_from_sync;
</file context>
| // considers the dirty bit clean, so nothing would | ||
| // ask again. The scheduler rebinds once the | ||
| // replacement is live. | ||
| if !client_clone.is_stopping() { |
There was a problem hiding this comment.
P2: Terminal logout/conflict handling can still publish a dirty-sync failure and enqueue a retry before is_running is cleared, because is_stopping() does not reflect the earlier enable_auto_reconnect = false transition. The guard should also reject the terminal auto-reconnect-disabled state while continuing to allow planned reconnects.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/handlers/ib.rs, line 166:
<comment>Terminal logout/conflict handling can still publish a dirty-sync failure and enqueue a retry before `is_running` is cleared, because `is_stopping()` does not reflect the earlier `enable_auto_reconnect = false` transition. The guard should also reject the terminal auto-reconnect-disabled state while continuing to allow planned reconnects.</comment>
<file context>
@@ -157,7 +157,13 @@ async fn handle_ib_impl(client: Arc<Client>, node: &wacore_binary::NodeRef<'_>)
+ // considers the dirty bit clean, so nothing would
+ // ask again. The scheduler rebinds once the
+ // replacement is live.
+ if !client_clone.is_stopping() {
client_clone.report_background_sync(
"app state re-sync after dirty notification",
</file context>
| if !client_clone.is_stopping() { | |
| if !client_clone.is_stopping() | |
| && client_clone | |
| .enable_auto_reconnect | |
| .load(std::sync::atomic::Ordering::Relaxed) | |
| { |
| /// Whether a planned reconnect is in flight: the client is staying, but the | ||
| /// socket is not usable and anything sent now is a no-op. | ||
| pub(crate) fn is_reconnecting(&self) -> bool { | ||
| self.expected_disconnect.load(Ordering::Relaxed) && !self.is_stopping() |
There was a problem hiding this comment.
P2: Ordinary planned reconnects are not recognized as reconnecting: reconnect() tears down the transport without setting expected_disconnect, so app-state retries run against the offline socket instead of remaining queued for the replacement connection. Include the intentional/disconnected state here while excluding terminal shutdown.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/client/lifecycle.rs, line 98:
<comment>Ordinary planned reconnects are not recognized as reconnecting: `reconnect()` tears down the transport without setting `expected_disconnect`, so app-state retries run against the offline socket instead of remaining queued for the replacement connection. Include the intentional/disconnected state here while excluding terminal shutdown.</comment>
<file context>
@@ -79,6 +79,25 @@ impl Client {
+ /// Whether a planned reconnect is in flight: the client is staying, but the
+ /// socket is not usable and anything sent now is a no-op.
+ pub(crate) fn is_reconnecting(&self) -> bool {
+ self.expected_disconnect.load(Ordering::Relaxed) && !self.is_stopping()
+ }
+
</file context>
| /// survive a reconnect want this; callers that must not touch the socket | ||
| /// right now want `is_shutting_down`. | ||
| pub(crate) fn is_stopping(&self) -> bool { | ||
| !self.is_running.load(Ordering::Relaxed) |
There was a problem hiding this comment.
P2: Direct-connect clients are now treated as permanently stopping, so app-state retry tasks cancel even while their socket is usable. is_running tracks the supervision loop rather than terminal shutdown; this predicate should use the terminal shutdown signal or a separate terminal-state flag instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/client/lifecycle.rs, line 92:
<comment>Direct-connect clients are now treated as permanently stopping, so app-state retry tasks cancel even while their socket is usable. `is_running` tracks the supervision loop rather than terminal shutdown; this predicate should use the terminal shutdown signal or a separate terminal-state flag instead.</comment>
<file context>
@@ -79,6 +79,25 @@ impl Client {
+ /// survive a reconnect want this; callers that must not touch the socket
+ /// right now want `is_shutting_down`.
+ pub(crate) fn is_stopping(&self) -> bool {
+ !self.is_running.load(Ordering::Relaxed)
+ }
+
</file context>
…strap (#473) * docs(appstate): document AppStateSyncFailed and degraded-connect bootstrap whatsapp-rust#1207 reworked sync_collections_batched to report a per-collection outcome (synced/fatal/retryable/skipped) instead of a bare Ok(()), and added Event::AppStateSyncFailed. A fatal critical_block failure during the pairing bootstrap now connects in a degraded state rather than looping forever, and retryable collections back off on WhatsApp Web's schedule instead of being refetched within the same batch. Ref: oxidezap/whatsapp-rust#1207 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014fRFZef6tyPkRzqP2hrsb4 * docs(appstate): fix review findings on AppStateSyncFailed docs - connected: define independently of the fatal bucket (bootstrap vs. background paths set it for different reasons; a purely retryable/skipped background sync also reports connected: true, contradicting the previous "true only when fatal" wording) - fatal recovery: a fatal outcome does not retry automatically on the same connection (WA Web gates COMPANION_SYNCD_SNAPSHOT_FATAL_RECOVERY off for critical_block, and this client implements no alternative) — replace the "later background retry" claim with "needs a fresh connection", and scope the backoff-schedule claim to retryable collections only - add the critical_unblock_low wire name to the collection list (confirmed in wacore/appstate/src/schemas.rs) - push name/presence: note the push name may already be known via Bot::with_push_name or an earlier session, so a fatal critical_block doesn't unconditionally leave presence unavailable - syncd IQ error codes (400/404) are IQ-level, not HTTP statuses - drop "permanently unsynced" from the example log line, which overstated fatal's terminality Addresses review feedback from Codex and CodeRabbit on PR #473. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014fRFZef6tyPkRzqP2hrsb4 * docs(appstate): split the dense fatal-outcome bullet into sub-points The "Any collection fatal" bullet had grown to four distinct ideas (dispatch behavior, recovery timing, user-facing impact, WA Web comparison) in one paragraph after the prior review round. Split into nested sub-bullets, no content change. Addresses Codex review feedback on PR #473. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014fRFZef6tyPkRzqP2hrsb4 --------- Co-authored-by: Claude <noreply@anthropic.com>
2 participants
Follow-up to #1203, #1205 and #1206, closing the gaps they left and the ones #1204 surfaced but did not fix.
The false success
sync_collections_batchedreturnedOk(())in cases where a collection had not synced:CollectionSyncError::Fatal(400/404), logged and skippedMAX_ITERATIONSexhausted with patches still outstanding<collection>, which parses fine and lands nowhereThe initial bootstrap reads that
Okas permission to storecritical_sync_done, abort the retry watchdog anddispatch_connected(). Acritical_blockfailing any of those ways left the session connected without the settings it carries,setting_pushNameincluded, and with nothing scheduled to retry.#1204 proposed turning them into
Err. That is worse: the bootstrap returns beforeneeds_initial_full_syncis cleared, so a 400/404 reconnects into the same state every 180s, forever, and sinceneeds_pushname_from_syncderives from the persisted push name it survives a restart.What this does instead
sync_collections_batchedreturns aBatchedSyncOutcomewith four buckets (synced,fatal,retryable,skipped) and each caller decides what a partial result means.Erris reserved for genuinely global failures.The buckets are not interchangeable.
skippedmeans an equivalent sync already holds the collection and is doing this work — the only case a caller may ignore. Everything else is a real miss.fatalAppStateSyncFailed, hand the batch's recoverable misses to the background syncretryable/skippedonlyEverything else goes through
report_background_sync, which logs, emits the event, and schedules a spaced retry.Why degraded rather than logout or retry
WAWebSyncdFatal): waits 5s, sendsAppStateFatalExceptionNotificationto the primary, thensocketLogout(SyncdFailure). It can — it owns the re-link UI.appstate.go): returns the error to the caller. ExposesBuildFatalAppStateExceptionNotificationandBuildAppStateRecoveryRequestas builders and never calls them; the first one's doc warns it logs out every linked device.src/appstate/): classifies into the same states and returns the outcome, no automatic destructive action.Two of three keep the policy out of the library. Ending a consumer's session is not ours to decide, and
node_io.rs:1296already recorded that choice.Recovery via
COMPANION_SYNCD_SNAPSHOT_FATAL_RECOVERYis not a route here — WA Web gates it off for exactly the collection that matters:Two bugs from re-reading
WAWebSyncdServerSyncErrorRetrymust not be refetched in the same run. WA Web routesErrorRetry,ErrorFatalandBlockedtodoneCollections; onlyConflictHasMore,SuccessHasMoreand a conflict with pending mutations go torefetchCollections. We pushed retryable errors back into the refetch list, which the corrected cap would have turned into 500 attempts at a collection that had just failed.The cap is 500, not 5. The loop is
(l < y || (i.length > 0 && l < C))withy = 5,C = 500; the body only runs while something is left to refetch, so it collapses tol < 500. Rounds are back-to-back there too — the syncd backoff spaces retries of a failed collection, not pages of one still making progress.Taking
ErrorRetryout of the loop matches WA Web, but WA Web hands it to a retry state machine afterwards and we had none, so on its own that change would strand a collection after a single 500.schedule_app_state_retryis that machine, with WA Web's spacing (1s, doubling, capped at an hour). The persisted two-day expiry is not here; it needs a store column.The redesign
Nine review rounds on this branch were almost entirely one class of mistake: work that survives an await, and the connection or the deadline moving underneath it. Generation was read ad hoc in ten places, the deadline checked in three, and the shared bootstrap gate written from five — each caller responsible for remembering. Two forgot, in opposite directions, and several of the fixes introduced the next round's bug.
SyncScopecarries the connection a piece of work belongs to and the clock it runs against.Client::admitsis the only place either question is asked, and it says why:RetiredandExpiredare not interchangeable — a retired scope must not write or publish, an expired one on a live connection still may, which is exactly when the bootstrap has to stay armed.settle_bootstrapdoes the same for the shared gate. There are now no direct writes toneeds_initial_full_syncoutside it.The apply boundary is per collection.
process_patch_listspersists each list as it goes, so a batch admitted at the start could still be writing its third collection well outside the scope.process_one_patch_list(a pure extraction inwacore, no behaviour change) lets the client stop between collections, capping what a lost scope can commit at one instead of the whole batch.Ten ad-hoc generation reads and three deadline checks became two reads inside the scope machinery, three explicit rebinds, and one predicate.
Work that must outlive a planned reconnect rebinds instead of being cancelled — nothing re-issues a dirty-bit or
server_syncrequest once its trigger is consumed. What does not carry over is authority: a rebound run can no longer settle the bootstrap it was scheduled by. And a retry holds through the reconnect window rather than attempting inside it, becauseprocess_app_state_sync_taskanswersOk(())at its own shutdown guard without contacting the server, and reading that as success would drop the request silently.Reservations
SyncInFlightrecords whether a sync or a patch send holds a collection. Skipping is only sound behind an equivalent sync; the batched path was skipping behind patch sends, which take the same reservation and never fetch, so the sync was dropped and the patches that prompted it went unfetched.The bootstrap never skips: it has to know a collection is synced before dispatching Connected, and "someone else is trying" is not that. Waits are bounded, because the worker's intake loop runs non-history tasks inline. A wait that runs out is
retryable, notskipped— nobody is covering that collection.Push name
History sync's
-placeholder no longer becomes our push name. Storing it makespush_namenon-empty for good, soneeds_pushname_from_syncgoes false, the bootstrap stops looking for the real name, and the account re-announces-on every reconnect. whatsmeow skips the same value in two places. The authoritativesetting_pushNamemutation has its own path and does not come through here, so an account genuinely named-still gets it.Found by reading the diff whole, not by review
schedule_app_state_task_retrycarriedschedule_app_state_retry's doc comment while the latter had no summary line.Tests
Each verified to fail without its fix rather than merely pass with it. The scope invariants:
a_scope_stops_admitting_when_its_connection_or_clock_goes— and that retired outranks having time lefta_retired_scope_cannot_move_the_bootstrap_gate— in both directionsan_expired_scope_may_still_arm_the_gaterebinding_reports_whether_the_connection_movedonly_a_deadline_marks_the_bootstrapa_planned_reconnect_is_not_a_completed_syncThe outcome and request-set behaviour:
a_refused_collection_is_reported_fatal_not_synceda_retryable_collection_is_not_refetched_in_the_same_run— asserts exactly one IQ reaches the wirecollections_held_by_another_sync_are_reported_skipped— and nothing reaches the wirea_patch_send_holder_is_waited_out_not_skippeda_collection_the_response_omits_is_not_reported_syncedan_empty_response_leaves_every_collection_unsynceda_duplicate_collection_is_dropped_before_it_is_applieda_collection_requested_twice_is_reserved_oncean_unrequested_collection_in_the_response_is_droppedan_expired_deadline_stops_the_batch_before_it_reservesan_outcome_from_a_retired_connection_is_not_publishedonly_a_fully_synced_outcome_settles_the_bootstrapthe_absent_pushname_sentinel_is_not_a_name/a_name_containing_a_dash_is_keptTwo things are deliberately unasserted and say so in the code: that the scheduler refuses to run for a retired generation (two probes both passed with the guard removed — the wrongly-attempted sync fails anyway on a socketless client), and the requeue of a top-level batch failure (the observable is a detached task that sleeps first). A test that passes without its fix is worse than none.
Also not covered: the bootstrap's own branch, which needs a full connection to drive. The outcome it reads is covered; the branch on it is not.
Verification
cargo test -p wacore --lib(1340),cargo test -p whatsapp-rust --lib(1346),cargo clippy --workspace --all-targets -- -D warnings,cargo fmt --all --check— all clean.Left for later
The persisted finite-retry expiry. WA Web keeps a
finiteFailureStartTimeper collection and promotes to fatal after two days of continuous failure. That needs a store schema change, so the retry here is per-connection only.