Conversation
The initial critical app-state sync waits a fixed 5s for the encrypted app-state key-share before attempting the snapshot fetch. On a fresh pairing where a large concurrent history sync competes for the stream, the key-share can land after that window, so the snapshot is fetched before the key is stored and critical sync fails with "didn't find app state key" — dropping the account's saved contact names. Make the wait configurable via BotBuilder::with_app_state_key_wait / Client::set_app_state_key_wait, defaulting to the existing 5s so behavior is unchanged unless opted in. Mirrors the existing wanted_pre_key_count / resend_rate_limit builder-to-client tuning pattern. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a configurable timeout for the initial critical app-state sync wait for an encrypted app-state key-share. ChangesApp-State Key Wait Configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant BotBuilder
participant Client
participant handle_success
BotBuilder->>Client: set_app_state_key_wait(wait)
Client->>Client: store app_state_key_wait_ms
handle_success->>Client: app_state_key_wait()
Client-->>handle_success: key_wait Duration
handle_success->>handle_success: rt_timeout(key_wait)
Look, this is a small but important change, and I need it done right — timeouts matter, ok? We're giving people control over how long we wait for the encrypted app-state key-share instead of hardcoding 5 seconds like it's 2004. Clean, tested, wired end-to-end. This is the kind of precision I expect. Ship it. 🚥 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/accessors.rs | Adds set_app_state_key_wait/app_state_key_wait accessors; as u64 cast from u128 is safe in practice but loses Clippy truncation guard. |
| src/client.rs | Adds app_state_key_wait_ms: AtomicU64 field; field-level doc is more verbose than the project style guide mandates. |
| src/client/lifecycle.rs | Initializes app_state_key_wait_ms to 5_000 in the constructor, matching the existing hard-coded default. |
| src/client/node_io.rs | Replaces the hard-coded 5 s timeout with the runtime-read app_state_key_wait() value; log message updated to match. |
| src/bot.rs | Adds with_app_state_key_wait builder method, wires it through the constructor, and adds two tests covering both the custom and the default value. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Caller
participant BotBuilder
participant Client
participant node_io as run_initial_sync
Caller->>BotBuilder: with_app_state_key_wait(Duration)
BotBuilder->>BotBuilder: "store Option<Duration>"
BotBuilder->>Client: "build() -> set_app_state_key_wait(wait)"
Client->>Client: app_state_key_wait_ms.store(ms, Relaxed)
note over Client,node_io: On connect / initial sync
node_io->>Client: app_state_key_wait()
Client-->>node_io: Duration (from AtomicU64 ms)
node_io->>node_io: rt_timeout(key_wait, keys_notifier.listen())
alt keys arrive within key_wait
node_io->>node_io: proceed with keys
else timeout
node_io->>node_io: proceed without keys (may fail critical sync)
end
%%{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 Caller
participant BotBuilder
participant Client
participant node_io as run_initial_sync
Caller->>BotBuilder: with_app_state_key_wait(Duration)
BotBuilder->>BotBuilder: "store Option<Duration>"
BotBuilder->>Client: "build() -> set_app_state_key_wait(wait)"
Client->>Client: app_state_key_wait_ms.store(ms, Relaxed)
note over Client,node_io: On connect / initial sync
node_io->>Client: app_state_key_wait()
Client-->>node_io: Duration (from AtomicU64 ms)
node_io->>node_io: rt_timeout(key_wait, keys_notifier.listen())
alt keys arrive within key_wait
node_io->>node_io: proceed with keys
else timeout
node_io->>node_io: proceed without keys (may fail critical sync)
end
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/client/accessors.rs:76-77
`wait.as_millis()` returns `u128`, and `as u64` silently truncates values beyond `u64::MAX` milliseconds. While no realistic timeout would overflow (u64::MAX ms ≈ 584 million years), the cast defeats Clippy's `cast_possible_truncation` pedantic lint and is inconsistent with the codebase's general avoidance of lossy casts. A saturating conversion makes the intent explicit.
```suggestion
self.app_state_key_wait_ms
.store(u64::try_from(wait.as_millis()).unwrap_or(u64::MAX), Ordering::Relaxed);
```
### Issue 2 of 2
src/client.rs:795-801
The field doc repeats everything already covered in the public-API doc comments (`set_app_state_key_wait`, `app_state_key_wait`, and the builder method), and the AGENTS.md rule says to be concise and explain *why*, not *what*. Private/crate-visible fields should carry only the "why" that isn't apparent from the name or public API.
```suggestion
/// Milliseconds; see [`Client::set_app_state_key_wait`]. Raising this lets the
/// key-share win the race against a heavy history sync at pairing time.
pub(crate) app_state_key_wait_ms: AtomicU64,
```
Reviews (1): Last reviewed commit: "feat(bot): configurable initial app-stat..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| self.app_state_key_wait_ms | ||
| .store(wait.as_millis() as u64, Ordering::Relaxed); |
There was a problem hiding this comment.
wait.as_millis() returns u128, and as u64 silently truncates values beyond u64::MAX milliseconds. While no realistic timeout would overflow (u64::MAX ms ≈ 584 million years), the cast defeats Clippy's cast_possible_truncation pedantic lint and is inconsistent with the codebase's general avoidance of lossy casts. A saturating conversion makes the intent explicit.
| self.app_state_key_wait_ms | |
| .store(wait.as_millis() as u64, Ordering::Relaxed); | |
| self.app_state_key_wait_ms | |
| .store(u64::try_from(wait.as_millis()).unwrap_or(u64::MAX), Ordering::Relaxed); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/client/accessors.rs
Line: 76-77
Comment:
`wait.as_millis()` returns `u128`, and `as u64` silently truncates values beyond `u64::MAX` milliseconds. While no realistic timeout would overflow (u64::MAX ms ≈ 584 million years), the cast defeats Clippy's `cast_possible_truncation` pedantic lint and is inconsistent with the codebase's general avoidance of lossy casts. A saturating conversion makes the intent explicit.
```suggestion
self.app_state_key_wait_ms
.store(u64::try_from(wait.as_millis()).unwrap_or(u64::MAX), Ordering::Relaxed);
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| /// How long the initial critical app-state sync waits for the encrypted | ||
| /// app-state key-share before attempting the snapshot fetch. Default 5s; set | ||
| /// via [`BotBuilder::with_app_state_key_wait`] or [`Client::set_app_state_key_wait`]. | ||
| /// Raise it when a large concurrent history sync can delay the key-share past | ||
| /// the default, which otherwise fails critical sync with "didn't find app | ||
| /// state key" and drops the account's saved contact names. | ||
| pub(crate) app_state_key_wait_ms: AtomicU64, |
There was a problem hiding this comment.
set_app_state_key_wait, app_state_key_wait, and the builder method), and the AGENTS.md rule says to be concise and explain why, not what. Private/crate-visible fields should carry only the "why" that isn't apparent from the name or public API.
| /// How long the initial critical app-state sync waits for the encrypted | |
| /// app-state key-share before attempting the snapshot fetch. Default 5s; set | |
| /// via [`BotBuilder::with_app_state_key_wait`] or [`Client::set_app_state_key_wait`]. | |
| /// Raise it when a large concurrent history sync can delay the key-share past | |
| /// the default, which otherwise fails critical sync with "didn't find app | |
| /// state key" and drops the account's saved contact names. | |
| pub(crate) app_state_key_wait_ms: AtomicU64, | |
| /// Milliseconds; see [`Client::set_app_state_key_wait`]. Raising this lets the | |
| /// key-share win the race against a heavy history sync at pairing time. | |
| pub(crate) app_state_key_wait_ms: AtomicU64, |
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/client.rs
Line: 795-801
Comment:
The field doc repeats everything already covered in the public-API doc comments (`set_app_state_key_wait`, `app_state_key_wait`, and the builder method), and the AGENTS.md rule says to be concise and explain *why*, not *what*. Private/crate-visible fields should carry only the "why" that isn't apparent from the name or public API.
```suggestion
/// Milliseconds; see [`Client::set_app_state_key_wait`]. Raising this lets the
/// key-share win the race against a heavy history sync at pairing time.
pub(crate) app_state_key_wait_ms: AtomicU64,
```
**Context Used:** AGENTS.md ([source](https://app.greptile.com/oxidezap/github/oxidezap/whatsapp-rust/-/custom-context?memory=26029e85-0dae-44f2-ab23-b8de43e5e9c7))
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files
Confidence score: 5/5
- In
src/client/accessors.rs, castingwait.as_millis()(u128) directly tou64can silently truncate extremely large durations, which could make timeout/backoff behavior unexpectedly shorter if those edge values are ever hit; this looks low-risk in normal usage but worth tightening before merge—switch to a checked or saturating conversion so overflow handling is explicit.
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/accessors.rs">
<violation number="1" location="src/client/accessors.rs:77">
P3: `wait.as_millis()` is `u128`, so the direct `as u64` cast can silently truncate very large durations. A checked/saturating conversion makes the bound explicit and avoids a lossy cast.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| /// initial sync starts, so set it before connecting. | ||
| pub fn set_app_state_key_wait(&self, wait: Duration) { | ||
| self.app_state_key_wait_ms | ||
| .store(wait.as_millis() as u64, Ordering::Relaxed); |
There was a problem hiding this comment.
P3: wait.as_millis() is u128, so the direct as u64 cast can silently truncate very large durations. A checked/saturating conversion makes the bound explicit and avoids a lossy cast.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/client/accessors.rs, line 77:
<comment>`wait.as_millis()` is `u128`, so the direct `as u64` cast can silently truncate very large durations. A checked/saturating conversion makes the bound explicit and avoids a lossy cast.</comment>
<file context>
@@ -69,6 +69,19 @@ impl Client {
+ /// initial sync starts, so set it before connecting.
+ pub fn set_app_state_key_wait(&self, wait: Duration) {
+ self.app_state_key_wait_ms
+ .store(wait.as_millis() as u64, Ordering::Relaxed);
+ }
+
</file context>
| .store(wait.as_millis() as u64, Ordering::Relaxed); | |
| .store(u64::try_from(wait.as_millis()).unwrap_or(u64::MAX), Ordering::Relaxed); |
|
Hi @gergesh I'll investigate that, but I'm unable to reproduce this locally. Can you provide logs with debug mode and with PII? Also what version or commit hash of the library are you trying? |
|
Also, can you check if this another solution works for you? #974 |
2 participants
What
Makes the initial critical app-state sync's wait for the app-state key-share configurable, defaulting to the current hard-coded 5s.
BotBuilder::with_app_state_key_wait(Duration)Client::set_app_state_key_wait(Duration)/Client::app_state_key_wait()Why
On a fresh pairing,
run_initial_sync(node_io.rs) waits up to a hard-codedDuration::from_secs(5)forinitial_keys_synced_notifierbefore it proceeds to fetch the critical (critical_unblock_lowetc.) app-state snapshot. The key-share is an encrypted peer message; when a large concurrent history sync is saturating the stream at pairing time, it can arrive after that 5s window. The snapshot is then fetched before the key is stored, and critical sync fails withdidn't find app state key(and the follow-uppatch snapshot MAC mismatch). For a companion client that reconstructs the address book fromcritical_unblock_low, that means the account's saved contact names never sync.listen()still returns the instant the keys arrive, so a larger value only extends the worst case (no key-share) — it doesn't slow down a healthy pairing. A consumer with a deliberately deep history-sync window can raise this to, say, 30s to let the key-share win the race.How
Stored as an
AtomicU64(ms) onClient, initialized to5_000, read where the fixed5swas used. Wired through the builder exactly like the existingwanted_pre_key_count/resend_rate_limittuning. Default is unchanged, so this is fully backward-compatible.Tests
test_bot_builder_app_state_key_wait— override applies to the clienttest_bot_builder_default_app_state_key_wait— default is 5scargo fmt --all,cargo clippy -p whatsapp-rust --lib --tests, and the new tests all pass.