fix(daemon): make the socket timeout a liveness budget - #270
Conversation
Co-authored-by: Umut Keltek <umut.keltek@gmail.com>
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: fix(daemon): make the socket timeout a liveness budget This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f11611f4b5
ℹ️ 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".
| typeof requestTimeoutMs === 'number' && Number.isFinite(requestTimeoutMs) && requestTimeoutMs > 0 | ||
| ? requestTimeoutMs | ||
| : DEFAULT_REQUEST_TIMEOUT_MSEC; | ||
| return resolveOAuthTimeoutFromEnv() + requestPhase; |
There was a problem hiding this comment.
Use the supplied OAuth timeout in the daemon ceiling
When a keep-alive auth request supplies params.timeoutMs from --oauth-timeout, runtime.listTools uses that value for both OAuth connection setup and the MCP listTools request, but this daemon-side guard still adds the env/default OAuth timeout here. If the override is only a CLI flag and is larger than the env/default budget, the daemon can return operation_timeout before the runtime’s own OAuth-plus-request budgets have elapsed (for example, a 10-minute flag allows 10 minutes of OAuth plus 10 minutes of listing, while the daemon caps the whole operation at 15 minutes). Please base the OAuth side of this ceiling on the effective per-request OAuth timeout, or carry it separately for auth/listTools requests.
Useful? React with 👍 / 👎.
Reimplements the design from #241 by @umutkeltek, which could not be rebased after the SDK v2 migration rewrote the daemon, runtime and CLI underneath it. Commits carry
Co-authored-by: Umut Keltek.The bug, confirmed on 0.13.0
listToolsreachesinvoke()with no timeout, so it falls back to a flatDEFAULT_DAEMON_TIMEOUT_MSof 30s on the daemon socket. The daemon-side handler meanwhile callsruntime.listTools(...)withautoAuthorize, which can open a browser and wait up toDEFAULT_OAUTH_CODE_TIMEOUT_MS— 300s — for consent. Nothing in the daemon protocol proved liveness in between.So a user who spends more than 30 seconds at the consent screen trips the client deadline,
invoke()classifies it as a transport error, restarts the daemon and re-sends — producing exactly the second OAuth prompt #241 set out to eliminate.The predecessor
resolveOperationSocketBudget(2 * timeoutMs + 5s) is gone, but the flat 30s that replaced it is the same mistake in simpler clothing: a deadline encoding an assumption about how long legitimate work takes.The fix
The socket deadline becomes an idle budget rather than an operation budget:
progressframes at a bounded interval while a request is in flight, so a working daemon proves it is alive.Version skew is explicit in both directions, since a daemon started by an older build can outlive it: a new client talking to an old daemon sees no progress frames and retains the flat deadline; an old client never receives progress at all, because emission requires an explicit v2 opt-in, so it still reads exactly one JSON response.
Verification
Mutation-checked rather than assumed — suppressing the daemon's progress emission fails precisely the test that claims to prove liveness (
Daemon request timed out), and restoring it passes. Worth noting one subtlety found while verifying: Node'ssocket.setTimeoutis an inactivity timer, so incoming progress data resets it natively; the explicit re-arm in the client is redundant belt-and-braces, and the real mechanism is the daemon writing at all.Also proven: a silent daemon is still torn down, a v1 client receives old-parser-safe output, and removing the no-retry handling replays the request twice.
Black-box CLI check: a 100 ms socket budget survived 880 ms and 986 ms operations.
pnpm checkclean, 1,277 tests passing, coverage 91.55% statements. Closes #241.🤖 Generated with Claude Code