feat(client): report a 429 stream error as a StreamError event - #1263
Conversation
A `<stream:error code="429">` was the one coded branch that adjusted connection state without telling anyone: it clears `is_logged_in`, adds 5 to the reconnect error count and suppresses the stability reset, which parks the client for minutes with nothing but an absent connection to explain it. Dispatch `Event::StreamError` there, as every other coded branch already does. This is not fidelity to WA Web. `Handle/StreamError.js` special-cases only 500..600 (515 and 516 by hand, the rest via `onStreamErrorReceived`), so 429 falls through to a bare `CLOSE_SOCKET` and is indistinguishable from any other reconnect — workable when a human is watching the UI, not when an embedder is the only observer. whatsmeow reaches the same event through its default arm. Also record, at the pre-key retry loop, that its missing attempt limit is the mirror and not a gap: `WAWebUploadPreKeysJob` ends only on success, and its >=500 / 406 / unrecognized error arms all fall through to the same retry, so a rate-limited upload retries there exactly as it does here.
|
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 (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe client now emits ChangesStream-error event handling
Pre-key retry backoff
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 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/node_io.rs | Adds the 429 StreamError dispatch after the relevant session and backoff state updates. |
| src/client/tests.rs | Verifies the 429 event payload and confirms neighboring error codes retain their existing event behavior. |
| src/prekeys.rs | Documents the intentional unbounded retry policy and safely clamps internal backoff arithmetic without changing sleep timing. |
Reviews (2): Last reviewed commit: "fix(prekeys): saturate the retry loop's ..." | Re-trigger Greptile
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/prekeys.rs`:
- Around line 727-733: Update the retry loop’s Fibonacci delay-state calculation
in the surrounding prekey upload logic to use saturating addition before storing
delay_a + delay_b in u64. Keep the existing MAX_DELAY_SECS cap and
unlimited-retry behavior unchanged, while ensuring overflow cannot panic in
debug builds or wrap in release builds.
🪄 Autofix
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: 204469a2-89fa-406c-91e2-330f5c6594d0
📒 Files selected for processing (3)
src/client/node_io.rssrc/client/tests.rssrc/prekeys.rs
There was a problem hiding this comment.
All reported issues were addressed across 3 files
You’re at about 95% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
📦 Binary size report
.text per crate
Baseline: |
The sleep was capped at MAX_DELAY_SECS but the state behind it was not, so `delay_a + delay_b` overflows u64 after about 90 consecutive failures — a panic in debug builds, a wrap back to a one-second retry in release. Clamp the stored value the way `fibonacci_backoff` already does; past the cap the state only exists to overflow, so nothing observable changes. Also drop an inaccurate clause from the 429 comment: the neighbouring arms dispatch their own specialized events rather than `StreamError`, so there is no general convention to appeal to — only the reason 429 needs one.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
|
Both review points were valid; fixed in
On the failing Generated by Claude Code |
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
You’re at about 95% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Requires human review: The 429 change intentionally adds a consumer-visible StreamError event, altering the public event contract/behavior embedders observe; that product/API decision should be human-approved. The prekeys clamp is separate, but the PR takes the restrictive path.
Re-trigger cubic
Summary
Two behaviours were flagged as possible defects. Ground truth settles them differently.
Changes: the
429arm ofhandle_stream_errornow dispatchesEvent::StreamError, like every other coded arm. No internal behaviour moves —is_logged_in, the+5, and the reset suppression are untouched.Does not change: the pre-key retry loop. WA Web's loop has no attempt limit either, and for the same reasons. Recorded in a doc comment so the next investigation does not reopen it.
Not changed but worth a decision: the 429 backoff ladder diverges hugely from WA Web. Evidence below, no code touched — see Unchanged, and why.
Ground truth
Bundle:
waVersion 2.3000.1044659339, the 497 files pinned in whatspec'sgenerated/bundles.lock.json, each fetched fromstatic.whatsapp.netand sha256-verified against the lockfile.docs/captured-js/was not present locally, so the pinned set is the bundle. whatspec IR was queried for the WAM catalogue; every control-flow answer below comes from the bundle, because the IR models shape and not sequencing.Observation A —
<stream:error code="429">WAWebHandleStreamErrorparses the stanza intoconflict/code/ack/xml-not-well-formed/other, then:type === "code" && code >= 500 && code < 600:515restarts login,516forces logout, everything else callsWAComms.onStreamErrorReceived().device_removed,replaced,xml-not-well-formed.return "CLOSE_SOCKET".429 is not in
500..600. It matches no arm at all and reaches the fallthrough. What WA Web makes observable for a rate-limited session, in full:WARN "Unrecognized stream:error"belongs to the parser'sotherbranch, which a coded stanza never reaches),[comms] job response is CLOSE_SOCKETand[comms] Socket N closedinWAComms— emitted for every socket close,onConnectionChange, whose vocabulary isdisconnected/in_handshake/connected(WANotifyConnectionChangeFactory) and carries no reason,WebcSocketConnect's only reason field iswebcSocketConnectReason, written on connect byWAWebOpenChatSocket.So: nothing distinguishes "rate limited" from "disconnected" in WA Web. The missing event is not a fidelity defect. It is a gap in our contract, since
Event::StreamErroris the channel every other coded branch reports through and 429/503 are the two silent exclusions — hence the change, marked in the code as choice rather than fidelity.Two further answers to the questions asked:
WAComms.socketLoop, aPromiseRetryLoopoversocketLoopIteration.onStreamErrorReceived()— the only per-code backoff lever, reached by 5xx and never by 429 — issocketLoop.cancelReset(), which nulls the early-reset deadline armed byresetTimeoutAfter(1e4)when a socket connects. It does not touchresetDelay: 3e4. There is no per-code counter increment anywhere.WAPromiseBackoffs.createTimer, held by aPromiseRetryLoopinstance created at comms construction. A page reload starts at zero.The production ladder config,
WAWebCommsConfigBase, confirms our constants exactly:fibonacci_backoff's doc comment states this verbatim, and the sequence matches to the millisecond (createTimer's first call returns0and is consumed priming the loop, so the retry delays are 1s, 1s, 2s, 3s, 5s, 8s…, capped at 900s). One cosmetic deviation: WA Web's jitter isMath.ceil(i*(1+0.1*Math.random())), i.e. 0..+10%; ours is ±10%. Left alone.Observation B — the pre-key upload retry loop
WAWebUploadPreKeysJob, in full:Answering each question:
maxis a delay clamp, and provably so —WAPromiseBackoffs'sd()reads it aso != null && i > o && (i = o), and nothing else reads the field.PromiseRetryLoopcarries no attempt counter; its only terminating exit isendWithValue, called onsuccess. OurMAX_DELAY_SECS = 610is the correct reading ofmax: 61e4._uploadPreKeysbranches the error three ways for logging only —>=500"server requested backoff",406"uploaded invalid keys", anything else (where 429 lands) "unrecognized error" — and all threereturn {errorCode, errorText}, which is notsuccess, so all three retry identically. A disconnect mid-upload is caught, logged "disconnected, server state unknown", and returnsundefined, which also retries.g = 812, exported asUPLOAD_KEYS_COUNT. OurDEFAULT_WANTED_PRE_KEY_COUNTis exact. Baileys independently agrees (INITIAL_PREKEY_COUNT = 812).Two places where we already differ, both in the safer direction, both left alone: our
!is_logged_inbail is an exit WA Web does not have (it awaitsWAComms.waitForConnection()and keeps going), and WA Web's IQ send isdeprecatedSendIqWithoutRetry, so the loop is its only retry — as here.A related suspicion also checked and cleared: the window is abandoned on failure, so every retry ships 812 fresh keys rather than re-offering the same ones. That is WA Web's behaviour too —
markKeyAsUploaded(lastKeyId)runs in the promise chain beforedeprecatedSendIqWithoutRetry, moving the watermark past the batch whatever the IQ answers.Measurements
The 429 reconnect ladder (
handle_stream_error+fibonacci_backoff), reproducing the numbers this task was filed with:auto_reconnect_errorsWA Web, from the same event and no penalty: 1s, 1s, 2s, 3s — about 7s of cumulative downtime over four consecutive 429s, against our 1872s. Its ladder still escalates on repeated failures, because each short-lived socket is one un-reset loop iteration; what it lacks is the
+5jump-start.The pre-key loop against a permanent 429, which the earlier investigation could not exercise. Harness:
CapturingMockTransport+ a test noise socket, with a responder subscribed toEvent::SentFramethat answers every<iq xmlns="encrypt">with<iq type="error"><error code="429" text="rate-overlimit"/></iq>, ontokio::timepaused so an hour runs in milliseconds.is_logged_in, so the disconnect bail never fires — as suspected, and as WA Web behaves.These numbers are WA Web's numbers too: same count, same schedule, same 812 keys.
Second opinions
CLOSE_SOCKET, no reason surfaced, no penalty; proactively closes the socketdefault:arm →ERROR "Unknown stream error"+dispatchEvent(events.StreamError{Code, Raw}); clearsisLoggedInfor every code; no penaltyERROR "stream errored out"+end(Boom(statusCode)), surfaced to the consumer viaconnection.update; no per-code armretryCount < 3,min(1000·2^n, 10000)backoff, raced againstUPLOAD_TIMEOUT;INITIAL_PREKEY_COUNT = 812Where they disagree: on observability, both second opinions hand the embedder something the moment a 429 arrives, and WA Web hands its UI nothing — which is the split this PR resolves in the embedder's favour. On the retry loop they disagree with each other, and WA Web breaks the tie: Baileys' 3-attempt cap has no counterpart in the official client (the task's note that Baileys has no retry at all is out of date — it gained one, just a bounded one).
Unchanged, and why
upload_pre_keys_with_retry. No test, since nothing changed.429backoff ladder — flagged, not touched. This is the one place ground truth says we diverge, and it diverges by 1872s against 7s over four errors. WA Web has no+5and nocancelResetfor 429;cancelResetis reached only by 5xx. Left alone because the task scoped ladder changes out and because aligning it properly means touching the shared reconnect path and the503arm together, not the 429 arm alone. It is the highest-value follow-up here, and I'd suggest it get its own task.503arm. Also silent, and the conclusion for 429 does apply to it. It is worse than a mirror image: WA Web's 5xx arm does callonStreamErrorReceived()while ours applies no penalty at all, so the two arms are inverted relative to ground truth. Deliberately left out of this diff — its own task.fibonacci_backoff. WA Web's is one-sided upward, ours is symmetric. Cosmetic at this amplitude.Validation
cargo nextestis not installed in this environment, so the same tests ran undercargo test.cargo clippy --workspacecannot complete here:alsa-sysfails to build for want ofalsa.pc, unrelated to this diff — the two touched crates are clean. E2E was not run (no mock server).Both new tests were verified to fail with the dispatch removed and pass with it restored.
Generated by Claude Code