fix(appstate): bound pairing key-share wait by the 180s critical deadline

[jlucaso1]

Problem

At fresh pairing the client fetches the critical app-state snapshots (contacts + push name) as soon as the stream is up. But the app-state sync-key-share — the E2E message the primary phone auto-sends carrying the key that decrypts those snapshots — can land later when a heavy history sync saturates the stream.

The old path waited a fixed 5s for the key, then ran the critical batched IQ regardless. When the key-share arrived after that window:

• the critical snapshot failed to decrypt with KeyNotFound ("didn't find app state key") and a "patch snapshot MAC mismatch",
• the critical sync failed,
• and only the 180s watchdog eventually forced a reconnect — during which the user's saved contacts never synced.

The deeper root cause: the on-connection recovery — request_keys_and_wait, which sends an explicit AppStateSyncKeyRequest and waits for the re-share — used a fixed 10s single-shot window, too short when the stream is saturated. After 10s it gave up and deferred the whole critical sync.

Fix

A single 180s critical-sync deadline (matching WhatsApp Web's WAWebSyncBootstrap) now bounds the entire critical path, and the recovery is placed where it actually runs:

1. Watchdog armed first, against a shared critical_deadline (Instant), so every step below is bounded by the same clock.
2. Pre-IQ wait is a brief 10s grace for the auto-shared key (WA Web's primary path) — purely an optimization to skip a redundant explicit request in the common fast case. The listener is registered before the flag check (the notifier isn't sticky, so a key-share landing in the load→listen gap would otherwise be missed). Correctness no longer depends on this grace's length.
3. The missing-key fallback is bounded by the deadline, not a fixed 10s. request_keys_and_wait now takes an explicit timeout; the initial critical bootstrap threads the shared deadline through sync_collections_batched, so a late or never-auto-shared key recovers via the explicit AppStateSyncKeyRequest on the same connection, any time within 180s — instead of stalling to the watchdog + reconnect.
4. Subscribe-then-recheck in the fallback. request_keys_and_wait registers its listener, then re-checks the store and returns immediately if the keys are already present — so a key persisted in the gap before listen() (whose non-sticky notify would be lost) can't burn the now-180s timeout.
5. Watchdog survives the failure path. On critical-sync Err the early return must not cancel the watchdog (it's the reconnect safety net), but AbortHandle aborts its task on drop — so the return was silently killing the watchdog. The handle is now detach()ed before returning, so a failed critical sync actually reconnects as intended.
6. Non-critical callers (background collections, group server_sync, ib dirty-resync) pass None and keep the existing fixed 10s wait — unchanged.

WhatsApp Web parity

Verified against the captured WAWebSyncBootstrap bundle:

• ue = 180, setTimeout(…, ue*1e3) → the 180s critical-data deadline (our CRITICAL_SYNC_TIMEOUT_SECS).
• syncCriticalData arms the timeout (this.$15()) before markCollectionsForSync([CriticalBlock, CriticalUnblockLow]) → our watchdog-first ordering, same two collections.
• Completion gate $16() = SettingPushName action Success → our push_name watchdog proxy.
• HandleMissingKeys.requestAllMissingKeys explicitly requests the specific missing key ids from snapshot/patch records → our AppStateSyncKeyRequest fallback.

(WA Web's syncd engine is notification-driven and has no pre-IQ grace; our 10s grace is a benign adaptation of that async model to our synchronous IQ path, with the deadline-bounded fallback as the real guarantee. On timeout WA Web socketLogouts; we reconnect to preserve credentials — an existing, intentional divergence.)

Test

Deterministic repro at the process_snapshot layer (no server, no mock): the same critical snapshot fails with KeyNotFound while the key is missing and decodes cleanly once the key is present — proving the failure is purely key-share ordering, not snapshot contents or MACs.

Validation

• cargo fmt --all clean
• cargo clippy --all --tests clean
• cargo test -p wacore-appstate — 38 passed (incl. the repro)
• cargo test -p whatsapp-rust --lib — 917 passed

Review notes

Thanks to the review bots for four genuine catches, all fixed — the design converged on one deadline with a lost-wakeup-safe wait at every step and a watchdog that actually survives failure:

• lost-wakeup at the pairing grace wait (Codex/CodeRabbit): listener registered before the flag check.
• starved fallback (Codex): the earlier draft blocked the full deadline on the pre-IQ wait, starving the explicit-request recovery; the deadline now lives on the fallback itself.
• lost-wakeup inside the fallback (Codex/cubic): subscribe-then-recheck.
• watchdog cancelled on the failure path (Codex): AbortHandle aborts on drop, so the Err return was killing the watchdog; now detach()ed.

🤖 Generated with Claude Code

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• ✅ Review completed - (🔄 Check again to review again)

📝 Walkthrough

Walkthrough

Critical app-state sync now carries an explicit deadline through batching, caller wiring, and fresh-pairing waits. The missing-key path is bounded, and a regression test covers the key-not-found ordering race.

Changes

Critical sync deadline plumbing

Layer / File(s)	Summary
Batch sync deadline API src/client/app_state.rs	sync_collections_batched gains an optional deadline, propagates it into the inner loop, computes bounded wait durations, and passes explicit timeouts into the missing-key helper.
Critical sync timing and callers src/client/node_io.rs, src/handlers/ib.rs, src/handlers/notification/groups.rs	Fresh-pairing critical sync now arms the deadline before key-share waiting, uses the notifier-based grace path, and passes the new batch-sync argument at all call sites.
Key-not-found regression test wacore/appstate/src/processor.rs	Adds a deterministic test for the snapshot decode race before and after the key becomes available.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client as Client::handle_success
  participant Notifier as initial_keys_synced_notifier
  participant AppState as sync_collections_batched

  Client->>Client: arm critical_deadline
  Client->>Notifier: listen for initial_app_state_keys_received
  Notifier-->>Client: key-share arrives
  Client->>AppState: sync_collections_batched(..., Some(critical_deadline))
  Client->>AppState: sync_collections_batched(..., None)

Loading

Possibly related PRs

• oxidezap/whatsapp-rust#297: Both PRs adjust the fresh-pairing critical app-state sync flow around initial synchronization and reconnect timing.
• oxidezap/whatsapp-rust#511: Both PRs touch the app-state missing-key sync path and the key-request/retry behavior.
• oxidezap/whatsapp-rust#911: Both PRs overlap on app-state key-share waiting and missing-key handling in the same sync flow.

Suggested labels: api-design

Suggested reviewers: greptile-apps, cubic-dev-ai

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title directly names the main appstate pairing fix and the 180s critical deadline.
Description check	✅ Passed	The description accurately explains the key-share wait fix, watchdog behavior, and the added test.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/appstate-key-race-repro

---

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 fresh-pairing race where the critical app-state snapshots (contacts, push name) could fail to decrypt because the E2E key-share arrived after the old fixed 5 s pre-IQ grace had already expired, leaving the client in a bad state until the 180 s watchdog forced a reconnect.

• Replaces the fixed 5 s/10 s key-share waits with a single shared 180 s critical_deadline (Instant) that bounds the entire critical path — pre-IQ grace, batched IQ, and the explicit AppStateSyncKeyRequest fallback — so a late or never-auto-shared key can still recover on the same connection.
• Fixes the AbortHandle-on-drop footgun in the failure path by calling detach() before returning, ensuring the reconnect watchdog actually fires when the critical sync fails.
• Adds a deterministic regression test in wacore/appstate that reproduces the KeyNotFound failure (same snapshot, missing key → error; present key → clean decode).

Confidence Score: 5/5

Safe to merge. The change is well-scoped, the logic is sound, and all four previously identified edge cases are demonstrably addressed.

The critical path restructuring is mechanically correct: the watchdog arms before any wait, the subscribe-then-recheck pattern in request_keys_and_wait prevents lost wakeups, the deadline flows through every step, and detach() ensures the watchdog survives the failure return. The regression test is deterministic and directly reproduces the field failure. Non-critical callers pass None and are unaffected.

No files require special attention.

Important Files Changed

Filename	Overview
src/client/node_io.rs	Reworks the critical sync orchestration: arms the 180s watchdog first, replaces the push_name proxy check with an explicit AtomicBool flag, adds a 10s pre-IQ grace (listener registered before the flag check to prevent lost wakeup), threads the critical_deadline into sync_collections_batched, and fixes the detach() bug on the failure path.
src/client/app_state.rs	Adds key_wait_deadline parameter to sync_collections_batched/inner; rewrites request_keys_and_wait to loop with subscribe-then-recheck (lost-wakeup safe) and deadline-bounded wait; extracts all_sync_keys_present helper.
wacore/appstate/src/processor.rs	Adds a deterministic regression test reproducing the KeyNotFound race: same snapshot fails without the key and succeeds once the key is present.
src/handlers/ib.rs	Mechanical update: passes None to sync_collections_batched for the dirty-resync path, keeping the fixed 10s default.
src/handlers/notification/groups.rs	Mechanical update: passes None to sync_collections_batched for the server_sync group notification path.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Main as Critical Sync Task
    participant WD as Watchdog (180s)
    participant Syncd as sync_collections_batched
    participant RKW as request_keys_and_wait
    participant Primary as Primary Phone

    Main->>WD: spawn watchdog (critical_deadline)
    Main->>Main: listen() for key-share
    Main->>Main: check initial_app_state_keys_received
    alt Key not yet received
        Main->>Main: wait up to 10s (KEY_SHARE_GRACE_SECS)
        Primary-->>Main: key-share arrives (fast path)
    end
    Main->>Syncd: sync_collections_batched([CriticalBlock, CriticalUnblockLow], Some(critical_deadline))
    Syncd->>Syncd: send batched IQ (30s timeout)
    alt Missing keys detected
        Syncd->>RKW: request_keys_and_wait(missing, remaining_deadline)
        RKW->>RKW: listen() before store check (lost-wakeup safe)
        RKW->>Primary: AppStateSyncKeyRequest
        loop Until all keys present or deadline
            Primary-->>RKW: key-share (partial or full)
            RKW->>RKW: all_sync_keys_present()?
        end
    end
    alt Critical sync Ok
        Main->>WD: critical_sync_done.store(true) + abort()
        Main->>Main: dispatch_connected()
    else Critical sync Err
        Main->>WD: detach() (watchdog stays alive)
        WD->>Main: reconnect_immediately() after 180s
    end

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 Main as Critical Sync Task
    participant WD as Watchdog (180s)
    participant Syncd as sync_collections_batched
    participant RKW as request_keys_and_wait
    participant Primary as Primary Phone

    Main->>WD: spawn watchdog (critical_deadline)
    Main->>Main: listen() for key-share
    Main->>Main: check initial_app_state_keys_received
    alt Key not yet received
        Main->>Main: wait up to 10s (KEY_SHARE_GRACE_SECS)
        Primary-->>Main: key-share arrives (fast path)
    end
    Main->>Syncd: sync_collections_batched([CriticalBlock, CriticalUnblockLow], Some(critical_deadline))
    Syncd->>Syncd: send batched IQ (30s timeout)
    alt Missing keys detected
        Syncd->>RKW: request_keys_and_wait(missing, remaining_deadline)
        RKW->>RKW: listen() before store check (lost-wakeup safe)
        RKW->>Primary: AppStateSyncKeyRequest
        loop Until all keys present or deadline
            Primary-->>RKW: key-share (partial or full)
            RKW->>RKW: all_sync_keys_present()?
        end
    end
    alt Critical sync Ok
        Main->>WD: critical_sync_done.store(true) + abort()
        Main->>Main: dispatch_connected()
    else Critical sync Err
        Main->>WD: detach() (watchdog stays alive)
        WD->>Main: reconnect_immediately() after 180s
    end

Loading

_{Reviews (8): Last reviewed commit: "fix(appstate): gate the critical watchdo..." | Re-trigger Greptile}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2641c9aa44

ℹ️ 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".

[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/client/node_io.rs`:
- Around line 956-981: The app-state wait in node_io::NodeIo can miss a
notification because it checks initial_app_state_keys_received before creating
the initial_keys_synced_notifier listener. Update this branch to follow the
request_keys_and_wait pattern by registering the listener first, then checking
the flag and only awaiting if keys are still not received; keep the wait logic
in NodeIo and use initial_keys_synced_notifier.listen() so a notify from
src/message/special.rs cannot be lost in the gap.

🪄 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: 2653ff2d-549b-4d32-bc14-2e6eeb3ad9a9

📥 Commits

Reviewing files that changed from the base of the PR and between 51030e6 and 2641c9a.

📒 Files selected for processing (2)

• src/client/node_io.rs
• wacore/appstate/src/processor.rs

[coderabbitai]

♻️ Duplicate comments (1)

src/client/node_io.rs (1)

953-969: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

This load-then-listen ordering can drop the key-share wakeup — and now it costs you 180 seconds, not 5.

Here's the thing: you check initial_app_state_keys_received at Line 953, but you don't create the listener until Line 967 inside rt_timeout. The producer in src/message/special.rs does it stores initial_app_state_keys_received true and then notifies initial_keys_synced_notifier. event_listener is fire-and-forget — if the key-share lands in the gap between the load and listen(), no listener is registered yet, so the wakeup is lost.

Move fast, but the connection has to actually work. Previously a missed wakeup wasted 5s; with the extended deadline it now stalls the full 180s, and the watchdog at Line 924 will observe an empty push_name and force a spurious reconnect_immediately(). Register the listener first, then check the flag — mirror the request_keys_and_wait pattern.

🔒️ Proposed fix: register the listener before the flag check

+                let key_share_listener =
+                    client_clone.initial_keys_synced_notifier.listen();
                 if !client_clone
                     .initial_app_state_keys_received
                     .load(Ordering::Relaxed)
                 {
                     // Bounded by the critical deadline, not a fixed 5s window: a late
                     // key-share (under heavy history sync) would otherwise lose the race
                     // and fail the critical snapshot with KeyNotFound.
                     debug!(
                         target: "Client/AppState",
                         "Waiting up to {CRITICAL_SYNC_TIMEOUT_SECS}s for app state keys..."
                     );
                     let _ = rt_timeout(
                         &*client_clone.runtime,
                         Duration::from_secs(CRITICAL_SYNC_TIMEOUT_SECS),
-                        client_clone.initial_keys_synced_notifier.listen(),
+                        key_share_listener,
                     )
                     .await;

                     // Check if connection was replaced while waiting
                     check_generation!();
                 }

🤖 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 953 - 969, The app-state key wait in the
client can miss a wakeup because `initial_app_state_keys_received` is checked
before `initial_keys_synced_notifier.listen()` is registered, so a key-share
arriving in that gap is lost and the timeout can stretch unnecessarily. Update
the `Client/AppState` wait block in `node_io.rs` to follow the same ordering as
`request_keys_and_wait`: create/register the listener first, then re-check the
`initial_app_state_keys_received` flag and only wait if it is still false. Keep
the change localized around the `rt_timeout`/`initial_keys_synced_notifier` path
so the wakeup cannot be dropped.

🤖 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.

Duplicate comments:
In `@src/client/node_io.rs`:
- Around line 953-969: The app-state key wait in the client can miss a wakeup
because `initial_app_state_keys_received` is checked before
`initial_keys_synced_notifier.listen()` is registered, so a key-share arriving
in that gap is lost and the timeout can stretch unnecessarily. Update the
`Client/AppState` wait block in `node_io.rs` to follow the same ordering as
`request_keys_and_wait`: create/register the listener first, then re-check the
`initial_app_state_keys_received` flag and only wait if it is still false. Keep
the change localized around the `rt_timeout`/`initial_keys_synced_notifier` path
so the wakeup cannot be dropped.

---

ℹ️ Review info

⚙️ Run configuration

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

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 33db4fbf-0c51-4b7e-9989-1ed400f0d026

📥 Commits

Reviewing files that changed from the base of the PR and between 2641c9a and be3da4e.

📒 Files selected for processing (1)

• src/client/node_io.rs

[cubic-dev-ai]

1 issue found and verified against the latest diff

Confidence score: 3/5

• In src/client/node_io.rs, the wait logic around initial_app_state_keys_received can miss an already-delivered key-share signal, causing a full 180s stall and then an unnecessary reconnect even though keys are available; this is a concrete user-facing latency/regression risk—adjust the signaling/check order (or re-check under the same synchronization point) so the wake-up cannot be missed before merging.

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

0 issues found across 1 file (changes from recent commits).

<sub>Requires human review: Changes modify critical sync timing logic in the app state bootstrap path, including wait duration and listener registration.

Re-trigger cubic</sub>

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c3d00faee7

ℹ️ 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".

[github-actions]

📦 Binary size report

Metric	main	PR	Δ
bin size (stripped)	10.74 MiB	10.74 MiB	+1.09 KiB (+0.01%) 🔺
bin .text	8.75 MiB	8.75 MiB	+1.12 KiB (+0.01%) 🔺
bin allocated (text+data+bss)	10.74 MiB	10.74 MiB	+32 B (+0.00%) 🔺
llvm-lines wacore	503,173	503,173	0
llvm-lines wacore copies	17,243	17,243	0
llvm-lines whatsapp-rust lib	737,937	738,253	+316 (+0.04%) 🔺
llvm-lines whatsapp-rust lib copies	23,865	23,869	+4 (+0.02%) 🔺
deps crates (Cargo.lock)	466	466	0

.text per crate

Crate	main	PR	Δ
.text whatsapp_rust	1.58 MiB	1.58 MiB	+1.21 KiB (+0.07%) 🔺
.text wacore	530.93 KiB	530.93 KiB	0
.text wacore_binary	157.49 KiB	157.49 KiB	0
.text wacore_libsignal	178.30 KiB	178.30 KiB	0
.text wacore_appstate	156.41 KiB	156.41 KiB	0
.text wacore_noise	26.05 KiB	26.05 KiB	0
.text waproto	1.60 MiB	1.60 MiB	0
.text whatsapp_rust_sqlite_storage	509.25 KiB	509.25 KiB	0
.text whatsapp_rust_tokio_transport	43.61 KiB	43.61 KiB	0
.text whatsapp_rust_ureq_http_client	8.95 KiB	8.95 KiB	0
.text std	1021.47 KiB	1021.59 KiB	+126 B (+0.01%) 🔺
.text other deps	2.94 MiB	2.94 MiB	-209 B (-0.01%) 🔽

Top movers (cargo-bloat attribution)

Crate	main	PR	Δ
whatsapp_rust	1.58 MiB	1.58 MiB	+1.21 KiB (+0.07%)

Baseline: 51030e623 (latest main run) · Head: 7e51c1bf1 · Graphs

Dismissed because a newer commit was pushed; Greptile will re-review the current head.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1ca8d53186

ℹ️ 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".

1 issue found across 4 files (changes from recent commits).

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

Dismissed because a newer commit was pushed; Greptile will re-review the current head.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/client/app_state.rs (2)

615-628: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Clean up the stale doc comment — it contradicts the current contract.

Look, when I'm reading a function's doc comment, I need it to tell me the truth about what the function returns — no exceptions. Lines 615-619 say this helper returns true "iff the caller should refetch (a request was sent and we waited)", but the second block (620-628) — which matches the actual code below it — says true means every key landed and processing may proceed. These are opposite contracts stacked on the same function. Whoever refactors this next without reading the code closely could easily invert the check.

🧹 Proposed fix

-    /// Shared missing-key repair step for both sync paths: request the given keys and,
-    /// only if a fresh request actually went out (the per-key dedup didn't suppress it),
-    /// wait briefly for the primary to re-share. Returns true iff the caller should
-    /// refetch (a request was sent and we waited); false means nothing was requested
-    /// (empty or deduped), so the caller proceeds without stalling.
     /// Request the missing decode keys, wait briefly for the re-share, then VERIFY they
     /// actually landed. Returns true only when every requested key is now stored (the

🤖 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 615 - 628, Update the doc comment on
the shared missing-key repair helper in app_state.rs so it matches the actual
return contract used by the implementation. Remove the stale “caller should
refetch” wording and keep only the description that true means all requested
keys were successfully stored and the caller may proceed, while false means the
share did not arrive in time and the caller must skip processing. Use the nearby
helper’s comment block and its return behavior as the source of truth.

---

651-660: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Sequential per-key store lookups — fine at current scale.

Each id in all_sync_keys_present is awaited one at a time. With the typical small number of missing keys this is a non-issue, but if this list ever grows meaningfully, futures::future::join_all would cut the wall-clock cost.

🤖 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 651 - 660, The `all_sync_keys_present`
method in `AppState` does sequential `backend.get_sync_key` awaits, which can
become slow if the `ids` list grows. Update this check to run the per-key
lookups concurrently, using `futures::future::join_all` (or equivalent) over the
`get_sync_key` calls, then preserve the existing “missing if any lookup returns
none/error” behavior.

🤖 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 615-628: Update the doc comment on the shared missing-key repair
helper in app_state.rs so it matches the actual return contract used by the
implementation. Remove the stale “caller should refetch” wording and keep only
the description that true means all requested keys were successfully stored and
the caller may proceed, while false means the share did not arrive in time and
the caller must skip processing. Use the nearby helper’s comment block and its
return behavior as the source of truth.
- Around line 651-660: The `all_sync_keys_present` method in `AppState` does
sequential `backend.get_sync_key` awaits, which can become slow if the `ids`
list grows. Update this check to run the per-key lookups concurrently, using
`futures::future::join_all` (or equivalent) over the `get_sync_key` calls, then
preserve the existing “missing if any lookup returns none/error” behavior.

---

ℹ️ Review info

⚙️ Run configuration

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

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 822f63e8-f788-4031-9f27-991044512e72

📥 Commits

Reviewing files that changed from the base of the PR and between 1ca8d53 and e717083.

📒 Files selected for processing (1)

• src/client/app_state.rs

Dismissed because a newer commit was pushed; Greptile will re-review the current head.

0 issues found across 1 file (changes from recent commits).

<sub>Requires human review: Modifies core app state sync timing logic, adds deadline-based waiting and recovery for key-share race condition. Touches critical pairing path with non-trivial logic changes.

Re-trigger cubic</sub>

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7a812ea4ec

ℹ️ 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".

Dismissed because a newer commit was pushed; Greptile will re-review the current head.

0 issues found across 1 file (changes from recent commits).

<sub>Requires human review: This is a high-impact refactor of the critical app-state sync path that needs human validation due to its broad impact on recovery, deadlines, and abort handling.

Re-trigger cubic</sub>

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)

624-644: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Loop until all requested keys are present or the deadline expires. This waits for only one notification, but a share can cover just part of missing, so a partial or unrelated wake returns false while time is still left. That defers the whole sync; re-listen and re-check until every key lands.

🤖 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 624 - 644, The request_keys_and_wait
flow in app_state::request_keys_and_wait only waits on a single notifier wake
and then returns based on one re-check, which can miss cases where only part of
missing arrives. Change the logic to keep looping on
initial_keys_synced_notifier.listen() and re-check all_sync_keys_present until
every requested key is present or rt_timeout reaches the deadline, and keep
using request_missing_keys_with_dedup for the initial request. Ensure the loop
preserves the existing fast-path check and the final timeout behavior while
handling partial or unrelated notifications correctly.

🤖 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 624-644: The request_keys_and_wait flow in
app_state::request_keys_and_wait only waits on a single notifier wake and then
returns based on one re-check, which can miss cases where only part of missing
arrives. Change the logic to keep looping on
initial_keys_synced_notifier.listen() and re-check all_sync_keys_present until
every requested key is present or rt_timeout reaches the deadline, and keep
using request_missing_keys_with_dedup for the initial request. Ensure the loop
preserves the existing fast-path check and the final timeout behavior while
handling partial or unrelated notifications correctly.

---

ℹ️ Review info

⚙️ Run configuration

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

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 408fd64b-3cfb-4a1d-b6f7-0d4ef3ce9840

📥 Commits

Reviewing files that changed from the base of the PR and between e717083 and 9bedde9.

📒 Files selected for processing (2)

• src/client/app_state.rs
• src/client/node_io.rs

Dismissed because a newer commit was pushed; Greptile will re-review the current head.

0 issues found across 1 file (changes from recent commits).

<sub>Requires human review: Core business-logic change to app-state sync and key-share wait/fallout handling, affecting connection recovery; tests pass, but potential impact on reconnect timing warrants human review.

Re-trigger cubic</sub>

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 11b22b5216

ℹ️ 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".

Dismissed because a newer commit was pushed; Greptile will re-review the current head.

0 issues found across 1 file (changes from recent commits).

<sub>Requires human review: Modifies critical app state sync logic including new deadline, reordered key wait, and watchdog changes. Touches core pairing and sync paths; blast radius is high and a bug could break contact syncing for all users.

Re-trigger cubic</sub>