fix: improve 408 timeout handling with socket retry in waitForWebLogin#47710
fix: improve 408 timeout handling with socket retry in waitForWebLogin#47710xi7ang wants to merge 1 commit into
Conversation
- Remove duplicate 408/DisconnectReason.timedOut condition - Add 408 timeout handling with socket restart in login-qr.ts - Similar to existing 515 (logged out) restart logic
openclaw-barnacle
Bot
added
channel: whatsapp-web
Greptile SummaryThis PR adds 408 (WebSocket timeout) handling to the WhatsApp login flow. Key observations:
Confidence Score: 3/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/whatsapp/src/login-qr.ts
Line: 283
Comment:
**Redundant string check and prefer constant over magic number**
`login.errorStatus` is typed as `number | undefined`, so `String(login.errorStatus).includes('408')` is redundant — if `errorStatus` is `408`, the preceding strict equality check already handles it. The string form could also accidentally match status codes like `4080` or `1408`.
Additionally, `DisconnectReason.timedOut` (already imported) equals `408` and is used consistently elsewhere in this codebase (e.g. `session.ts`). Using the named constant makes the intent clearer and avoids a magic number.
```suggestion
if (login.errorStatus === DisconnectReason.timedOut) {
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/whatsapp/src/login-qr.ts
Line: 282-298
Comment:
**No retry limit — potential tight retry loop on persistent 408**
Unlike the 515 restart path which is guarded by `login.restartAttempted`, the 408 retry has no counter or flag. If the WhatsApp server consistently returns 408 (e.g. due to a prolonged network issue or server-side rate limiting), this block will fire on every loop iteration until the `deadline` expires (up to 120 s by default). Each iteration creates a new socket, connects, and gets torn down — potentially dozens of times — which could exhaust file descriptors, flood logs, and hammer the server.
Consider adding a per-login retry counter (similar to `restartAttempted`) to bound the number of 408 retries:
```ts
// in the ActiveLogin type
timeoutRetries: number; // initialised to 0
// in the 408 block
if (login.timeoutRetries >= 3) {
// fall through to the generic failure path
} else {
login.timeoutRetries += 1;
// existing retry logic…
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/whatsapp/src/session.ts
Line: 149-152
Comment:
**Misleading comment — reconnect is not triggered here**
The inline comment says `"log and trigger reconnect"` but this handler only logs. The actual reconnect happens in `login-qr.ts`. Leaving the misleading comment risks confusing future maintainers into thinking reconnection is already handled at the socket level when it is not.
```suggestion
} else if (status === DisconnectReason.timedOut) {
// 408 timeout — log only; callers (e.g. waitForWebLogin) are responsible for reconnecting
sessionLogger.error({ status }, 'WhatsApp WebSocket connection timed out');
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 41c12e8 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
| } | ||
| // Handle 408 timeout - retry with fresh socket | ||
| if (login.errorStatus === 408 || String(login.errorStatus).includes('408')) { |
There was a problem hiding this comment.
Redundant string check and prefer constant over magic number
login.errorStatus is typed as number | undefined, so String(login.errorStatus).includes('408') is redundant — if errorStatus is 408, the preceding strict equality check already handles it. The string form could also accidentally match status codes like 4080 or 1408.
Additionally, DisconnectReason.timedOut (already imported) equals 408 and is used consistently elsewhere in this codebase (e.g. session.ts). Using the named constant makes the intent clearer and avoids a magic number.
| if (login.errorStatus === 408 || String(login.errorStatus).includes('408')) { | |
| if (login.errorStatus === DisconnectReason.timedOut) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/whatsapp/src/login-qr.ts
Line: 283
Comment:
**Redundant string check and prefer constant over magic number**
`login.errorStatus` is typed as `number | undefined`, so `String(login.errorStatus).includes('408')` is redundant — if `errorStatus` is `408`, the preceding strict equality check already handles it. The string form could also accidentally match status codes like `4080` or `1408`.
Additionally, `DisconnectReason.timedOut` (already imported) equals `408` and is used consistently elsewhere in this codebase (e.g. `session.ts`). Using the named constant makes the intent clearer and avoids a magic number.
```suggestion
if (login.errorStatus === DisconnectReason.timedOut) {
```
How can I resolve this? If you propose a fix, please make it concise.| // Handle 408 timeout - retry with fresh socket | ||
| if (login.errorStatus === 408 || String(login.errorStatus).includes('408')) { | ||
| runtime.log(info('WhatsApp login timed out (408), retrying with new socket...')); | ||
| closeSocket(login.sock); | ||
| try { | ||
| login.sock = await createWaSocket(false, login.verbose, { | ||
| authDir: login.authDir, | ||
| }); | ||
| login.error = undefined; | ||
| login.errorStatus = undefined; | ||
| login.restartAttempted = false; | ||
| attachLoginWaiter(account.accountId, login); | ||
| continue; | ||
| } catch (err) { | ||
| runtime.log(danger(`Failed to retry after 408: ${err}`)); | ||
| } | ||
| } |
There was a problem hiding this comment.
No retry limit — potential tight retry loop on persistent 408
Unlike the 515 restart path which is guarded by login.restartAttempted, the 408 retry has no counter or flag. If the WhatsApp server consistently returns 408 (e.g. due to a prolonged network issue or server-side rate limiting), this block will fire on every loop iteration until the deadline expires (up to 120 s by default). Each iteration creates a new socket, connects, and gets torn down — potentially dozens of times — which could exhaust file descriptors, flood logs, and hammer the server.
Consider adding a per-login retry counter (similar to restartAttempted) to bound the number of 408 retries:
// in the ActiveLogin type
timeoutRetries: number; // initialised to 0
// in the 408 block
if (login.timeoutRetries >= 3) {
// fall through to the generic failure path
} else {
login.timeoutRetries += 1;
// existing retry logic…
}Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/whatsapp/src/login-qr.ts
Line: 282-298
Comment:
**No retry limit — potential tight retry loop on persistent 408**
Unlike the 515 restart path which is guarded by `login.restartAttempted`, the 408 retry has no counter or flag. If the WhatsApp server consistently returns 408 (e.g. due to a prolonged network issue or server-side rate limiting), this block will fire on every loop iteration until the `deadline` expires (up to 120 s by default). Each iteration creates a new socket, connects, and gets torn down — potentially dozens of times — which could exhaust file descriptors, flood logs, and hammer the server.
Consider adding a per-login retry counter (similar to `restartAttempted`) to bound the number of 408 retries:
```ts
// in the ActiveLogin type
timeoutRetries: number; // initialised to 0
// in the 408 block
if (login.timeoutRetries >= 3) {
// fall through to the generic failure path
} else {
login.timeoutRetries += 1;
// existing retry logic…
}
```
How can I resolve this? If you propose a fix, please make it concise.| } else if (status === DisconnectReason.timedOut) { | ||
| // Handle 408 timeout - log and trigger reconnect | ||
| sessionLogger.error({ status }, 'WhatsApp WebSocket connection timed out'); | ||
| } |
There was a problem hiding this comment.
Misleading comment — reconnect is not triggered here
The inline comment says "log and trigger reconnect" but this handler only logs. The actual reconnect happens in login-qr.ts. Leaving the misleading comment risks confusing future maintainers into thinking reconnection is already handled at the socket level when it is not.
| } else if (status === DisconnectReason.timedOut) { | |
| // Handle 408 timeout - log and trigger reconnect | |
| sessionLogger.error({ status }, 'WhatsApp WebSocket connection timed out'); | |
| } | |
| } else if (status === DisconnectReason.timedOut) { | |
| // 408 timeout — log only; callers (e.g. waitForWebLogin) are responsible for reconnecting | |
| sessionLogger.error({ status }, 'WhatsApp WebSocket connection timed out'); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/whatsapp/src/session.ts
Line: 149-152
Comment:
**Misleading comment — reconnect is not triggered here**
The inline comment says `"log and trigger reconnect"` but this handler only logs. The actual reconnect happens in `login-qr.ts`. Leaving the misleading comment risks confusing future maintainers into thinking reconnection is already handled at the socket level when it is not.
```suggestion
} else if (status === DisconnectReason.timedOut) {
// 408 timeout — log only; callers (e.g. waitForWebLogin) are responsible for reconnecting
sessionLogger.error({ status }, 'WhatsApp WebSocket connection timed out');
```
How can I resolve this? If you propose a fix, please make it concise.![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: 41c12e84a5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| login.sock = await createWaSocket(false, login.verbose, { | ||
| authDir: login.authDir, | ||
| }); |
There was a problem hiding this comment.
In waitForWebLogin, the 408 retry path recreates the socket without passing an onQr handler, so any new QR emitted after the timeout is never stored back into the active login state. When the original QR has expired (the common 408 case), callers can only keep seeing the stale qrDataUrl from the first attempt, which leaves users unable to scan the fresh code and the login flow stalls until they manually restart from scratch.
Useful? React with 👍 / 👎.
- openclaw#47710 fix: improve 408 timeout handling with socket retry in waitForWebLogin
|
This pull request has been automatically marked as stale due to inactivity. |
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Keep open, but this PR is not merge-ready. Current main still lacks a bounded WhatsApp QR-login 408 retry, while this branch retries in the wrong layer, drops the shared QR/timing socket contract, can churn sockets on persistent 408s, conflicts with main, and has no after-fix real WhatsApp proof. Canonical path: Close this stale PR. The latest review rated it F, the branch still lacks merge-ready proof, and there has been no human follow-up after the durable review. So I’m closing this here because the remaining work is already tracked in the canonical issue. Review detailsBest possible solution: Close this stale PR. The latest review rated it F, the branch still lacks merge-ready proof, and there has been no human follow-up after the durable review. Do we have a high-confidence way to reproduce the issue? Yes, source-level: mock Is this the best way to solve the issue? No. The fix idea is valid, but the PR retries too late in Security review: Security review cleared: The diff only changes WhatsApp socket retry/logging code and does not touch dependencies, workflows, package resolution, scripts, or secret handling. What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 6981051682f2. |
|
Codex review: needs changes before merge. What this changes: The PR adds WhatsApp Web QR-login handling that closes and recreates the socket after a 408 timeout in Required change before merge: This is a concrete replacement/update candidate: current main still treats QR-login 408 as terminal, the source PR has specific fixable defects, and the likely files plus validation path are narrow. Review findings:
Review detailsBest possible solution: Port the idea into the current shared WhatsApp login waiter as a bounded 408/timedOut socket-restart path that preserves Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against e46dccb35374. |
clawsweeper
Bot
added
the
P1
openclaw-barnacle
Bot
added
the
triage: needs-real-behavior-proof
clawsweeper
Bot
added
impact:auth-provider
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
|
Thanks for working on this. I’m closing this PR as superseded by #88183. The behavior claim is valid: current main can treat a WhatsApp QR-login 408 opening-handshake timeout as a terminal failure before a scannable QR is shown. The replacement PR keeps the fix in the shared WhatsApp login waiter instead of the QR polling layer, preserves the existing socket timing and QR callback contract, bounds repeated 408 recovery, and keeps the existing 515 post-pairing restart available after a 408 recovery. Validation for the replacement: |
2 participants
Summary
This PR improves the WhatsApp QR code fix based on code review feedback.
Review Feedback Applied
status === 408 || status === DisconnectReason.timedOutChanges Made
extensions/whatsapp/src/session.tsextensions/whatsapp/src/login-qr.tsRelated Issue: #47367