Uh oh!
There was an error while loading. Please reload this page.
AIT-540: bound every outbound CLI request - #78
Conversation
Node's fetch has no default timeout. None of the three fetches in api/client.ts carried an AbortSignal, so a server that accepts the connection and never answers pinned the process indefinitely. An MCP client spawns mcp-headers per connection attempt and cannot reap what it spawned: a customer accumulated 8 hung helpers holding 21 GB. - api/client.ts: 30s AbortSignal on apiClient, the WorkOS refresh and /auth/rescope; a caller-supplied signal still wins. isNetworkFailure now recognises TimeoutError, so the existing catch blocks map the abort to NetworkError (exit 5) with no new error path. - commands/mcp.ts: unref'd 45s deadline on mcp-headers, above the request timeout so a real network failure reports itself first. The backstop for a hang that is not a request. - notifications-nudge.ts: bound the detached refresh child's fetch; nothing watches it, so an unbounded request orphans a node process. Verified against published 0.14.20: hung past 3m34s on a never-answering server. This build exits 5 in 45s; not-logged-in still exits 4 immediately.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:2a32ceb2d9
ℹ️ 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".
| ...fetchOptions, | ||
| headers, | ||
| // After the spread: a caller that brought its own signal keeps it. | ||
| signal: fetchOptions.signal ?? AbortSignal.timeout(API_FETCH_TIMEOUT_MS), |
There was a problem hiding this comment.
When a server or proxy sends 2xx headers but then stalls the response body, this signal remains active and res.json() later rejects with TimeoutError; however, that read is outside the network-error catch, and the blanket catch at lines 424–429 converts it into a successful undefined result. Callers such as workspace new then dereference result.id after the server may already have created the resource, producing a misleading crash and potentially encouraging a duplicate retry. Distinguish malformed/empty JSON from timeout errors, or keep body consumption inside the timeout-aware NetworkError mapping.
Useful? React with 👍 / 👎.
The first commit bounded api/client.ts. Nine more raw fetches carried no timeout at all, so the same hang class was still live outside the mcp-headers path — doctor's health probe worst of all, since that is the command you run when the network is already broken. New api/timed-fetch.ts is the single place allowed to call fetch, with two shapes because one timeout does not fit both: timedFetch bounds the whole exchange. JSON calls take the 30s default; byte transfers (media up/download, cloudflared) take a generous 10m cap so a stall still ends without ever firing on a real transfer. connectTimedFetch bounds only the wait for headers, then releases the body. Required for the two text/event-stream call sites: a total timeout would cut off a working SSE stream, which is the second bug this fix could easily have shipped. A caller's own signal always wins, so support watch's long-poll budget and Ctrl+C handling are untouched. Also: - mcp-headers deadline 45s -> 15s. First use mints over two sequential requests, so a per-request bound alone allowed 60s: longer than any MCP client waits. 15s covers a cold handshake plus both round trips. - doctor probes drop to 10s each. Measured against a never-answering server: 51s -> 31s, and it used to hang forever. - agent-auth.ts's duplicate local timedFetch deleted in favour of the shared one. Tests: 8 new. The SSE case pins that the signal is NOT aborted after headers arrive. no-raw-fetch.test.ts fails the build on any new raw fetch — verified it catches a planted one, since a guard that cannot fail is not a guard.
relative() returns api\timed-fetch.ts on Windows, so the POSIX-written allowlist missed its own exemption and the guard failed CI on the one platform the bug was reported from.
Codex P2 on the first commit, still live in the sweep. The timeout can fire during the body read, which happens after the fetch catch: headers came back 2xx and the bytes never did. The blanket catch around res.json() then turned that into a successful `undefined`, so a caller like `workspace new` dereferences result.id for a request that never completed, on a resource the server may already have created. Guarded the three body reads where an abort was being swallowed: - apiClient: was returning undefined, now NetworkError. - the WorkOS refresh: was falling into the shape guard and reporting a malformed response, which the caller reads as "session gone" and tells the user to log in again over what is a network stall. - sandbox send: was reporting a message as delivered on an empty body. gatewayRequest already reads its body inside the mapped try/catch, so it was never affected. Test asserts the exact shape Codex described; verified it fails without the guard.
Good catch on the P2 — fixed in d9b00bb, and the finding was still live after the sweep. The abort lands inside
Added a test for the exact shape you described (2xx headers, body rejects with @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:d9b00bbde1
ℹ️ 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".
| const data = await res.json().catch((err: unknown) => { | ||
| if (isNetworkFailure(err)) { | ||
| throw new NetworkError('Lost the connection to the sign-in service. Try again.'); | ||
| } |
There was a problem hiding this comment.
When WorkOS returns 2xx headers but the body times out during forceTokenRefresh()—the path used by channels connect—this guard throws a NetworkError, but forceTokenRefresh catches every error at lines 141–145 and replaces it with AuthError('Session expired...'). The user is therefore still incorrectly told to log in again for this network-stall scenario; preserve NetworkError there as validAccessToken already does.
Useful? React with 👍 / 👎.
| res = await timedFetch(`${getEffectiveApiUrl()}/auth/rescope`, { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json', ...buildVersionHeaders() }, | ||
| body: JSON.stringify({ refreshToken: creds.refreshToken, workspaceId }), |
There was a problem hiding this comment.
When /auth/rescope sends successful headers and then stalls its body, the timeout introduced here fires inside the later res.json(), whose blanket catch at line 182 converts it to null; the shape guard then reports RESCOPE_FAILED instead of a retryable NetworkError. This affects workspace use and workspace new, and the login wizard silently ignores the misclassified failure and continues with an unscoped token, so the body read needs the same timeout guard as the other JSON reads.
Useful? React with 👍 / 👎.
…dentity Both P2s from the second Codex pass, both correct. The first fix guarded three body reads one at a time and still missed two paths, so the three in this file now share one readJsonBody helper instead of three blanket .catch(() => empty) clauses: - /auth/rescope was reporting RESCOPE_FAILED for a stalled body, so workspace use and workspace new blamed the server for a network stall. - forceTokenRefresh caught EVERY error and replaced it with 'Session expired. Run: hookmyapp login'. validAccessToken already re-threw NetworkError; this path did not, so channels connect on a stalled network sent the user to re-login over a network blip. Test asserts forceTokenRefresh surfaces NETWORK_ERROR; verified it fails without the re-throw.
Both correct, fixed in 14d31ae. The first pass guarded three body reads one at a time and still missed two paths, which is the tell that it wanted to be one thing rather than three. The three reads in
Added a test for the @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:14d31ae91d
ℹ️ 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".
| let res: Response; | ||
| try { | ||
| res = await fetch(`${baseUrl}/auth/bootstrap/exchange`, { | ||
| res = await timedFetch(`${baseUrl}/auth/bootstrap/exchange`, { |
There was a problem hiding this comment.
When the bootstrap exchange returns successful headers but stalls while sending the JSON body, the timeout fires in res.json() at line 452, outside this try, so the CLI reports an UNKNOWN_ERROR rather than the intended retryable NetworkError. This is particularly misleading because the one-time bootstrap code may already have been consumed; read the response body inside the network-aware guard, as the main API client now does.
Useful? React with 👍 / 👎.
| res = await fetch(url, { method: 'POST', headers: { Authorization: `Bearer ${token}` }, body: form }); | ||
| // Media upload: bytes, so the generous transfer budget rather than the | ||
| // JSON one — big enough never to fire on a working upload. | ||
| res = await timedFetch(url, { method: 'POST', headers: { Authorization: `Bearer ${token}` }, body: form }, TRANSFER_TIMEOUT_MS); |
There was a problem hiding this comment.
When the gateway accepts an upload and sends response headers but stalls before completing the response body, this timeout rejects the later res.text() at line 236, after the try has ended. The resulting raw TimeoutError is rendered as UNKNOWN_ERROR instead of the NetworkError used for the same timeout before headers; keep the body read inside the network-failure mapping, as gatewayRequest already does.
Useful? React with 👍 / 👎.
Third Codex pass found two more of the same class (bootstrap exchange, media upload), which is two rounds too many for a rule enforced by hand. The rule now lives in the fetch layer: timed-fetch.ts owns isNetworkFailure (client.ts re-exports it for existing callers) and adds readBody, which keeps a mid-body transport failure a transport failure and lets a JSON parse error through untouched. Swept every response body read in src: login (4), agent-auth (2), gateway upload + the download stream loop, cloudflared arrayBuffer, version-check, sandbox send. gatewayRequest's read was already inside a mapped try; it uses readBody now anyway, so its safety no longer depends on which catch it happens to sit in. Two deliberate exemptions, both allowlisted with reasons: mapApiError / parseClientOutdated read an ALREADY-failed response, where the HTTP status is more useful than a NetworkError, and the nudge child's read lives in a node -e string that swallows everything by design. no-raw-fetch.test.ts now pins both halves — no raw fetch, no unguarded body read. Verified each half fails on a planted violation.
ord669
commented
Sep 2, 2026
Both correct again, fixed in b9c6d65. Three rounds of the same finding means the rule was wrong to enforce by hand, so it moved into the fetch layer. Swept every response body read in Two deliberate exemptions, allowlisted with reasons:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:b9c6d65216
ℹ️ 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".
| } | ||
| const err = await res.json().catch(() => ({})); | ||
| const err = await readBody(res.json(), 'Lost the connection to the sign-in service. Try again.').catch(() => ({})); |
There was a problem hiding this comment.
When WorkOS returns a non-2xx response and its body stalls or the connection drops, readBody correctly creates a NetworkError, but this blanket .catch(() => ({})) immediately swallows it; the poll then exits with AuthError('Login failed: unknown error') instead of reporting retryable network trouble. Fresh evidence in the latest fix is that the fallback is attached after readBody, bypassing the new transport-preservation behavior; rethrow NetworkError here while retaining the empty-object fallback for JSON parse failures.
Useful? React with 👍 / 👎.
Mine, not a pre-existing one: the previous commit wrapped this read in
readBody and left the blanket .catch(() => ({})) sitting right after it,
so a dropped connection mid-poll still reported 'Login failed: unknown
error'. The success read in the same loop already propagates transport
failures; this now matches it. Tolerating blips across poll iterations
would be a real improvement, but it is a behaviour change this ticket
should not make quietly.
Third guard in the same test: wrap the read correctly, then discard the
error you just raised. Verified it fails on a planted swallow.ord669
commented
Sep 2, 2026
Right, and that one is mine rather than pre-existing — the previous commit wrapped the read in The success read in the same poll loop already propagates transport failures, so this now matches it rather than inventing a third behaviour. Worth noting for later: a device-code poll running for minutes arguably should tolerate a transient blip and keep polling to its deadline instead of aborting the login. That is a real improvement, but it is a behaviour change I did not want to make quietly inside a timeout ticket — happy to open a follow-up. Added a third clause to the guard test for exactly this shape (a @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:c08505cc72
ℹ️ 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".
| // AbortSignal.timeout and connectTimedFetch both abort with a DOMException | ||
| // named TimeoutError. | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| if ((err as any)?.name === 'TimeoutError') return true; |
There was a problem hiding this comment.
When a caller supplies an AbortController signal and aborts after response headers arrive, Node rejects the body read with a DOMException named AbortError, not TimeoutError. Because this predicate returns false for that error, readBody rethrows it untyped and readJsonBody catches it as if it were malformed JSON, causing apiClient to return undefined for a cancelled request. Recognize AbortError so caller cancellation cannot be reported as a successful empty response.
Useful? React with 👍 / 👎.
| const res = await fetch('https://api.workos.com/user_management/authenticate', { | ||
| // Bounded per poll: the loop's own deadline only advances between | ||
| // requests, so one wedged request would outlive it (AIT-540). | ||
| const res = await timedFetch('https://api.workos.com/user_management/authenticate', { |
There was a problem hiding this comment.
When a device-code poll reaches a server or proxy that accepts the connection but never returns headers, timedFetch rejects here with TimeoutError, but this call has no network-aware catch. The error therefore reaches the top-level handler as UNKNOWN_ERROR with exit code 1, unlike the newly guarded body-read failures, which become retryable NETWORK_ERROR failures with exit code 5. Wrap this request with the same isNetworkFailure/NetworkError mapping used by the other login requests.
Useful? React with 👍 / 👎.
| const { url, body } = buildSandboxSendRequest(session, message); | ||
| const res = await fetch(url, { | ||
| const res = await timedFetch(url, { |
There was a problem hiding this comment.
When the sandbox proxy never returns response headers, the new 30-second deadline rejects this call with a raw TimeoutError; only the later body read is converted to NetworkError. Consequently sandbox send renders the header-timeout case as UNKNOWN_ERROR and exits 1 instead of reporting retryable network trouble and exiting 5. Catch transport failures around this timedFetch call as the gateway request paths do.
Useful? React with 👍 / 👎.
Fourth Codex pass, all three correct. - isNetworkFailure only knew TimeoutError. A caller's own controller aborts with AbortError, so an exchange killed that way fell past the predicate and readBody's callers turned it back into an empty success. Nothing in this CLI aborts deliberately except the fetch helpers, so there is no user-cancellation case to mislabel. - The login device poll and sandbox send called timedFetch with no catch at all, so the deadline surfaced as UNKNOWN_ERROR: a login or a send looked like a CLI bug rather than an unreachable host. - Swept the rest rather than fixing only the two named: the cloudflared download had the same gap and now reports BINARY_DOWNLOAD_FAILED with the cause instead of a bare TimeoutError. Windows CI failure on the previous commit was a slow-runner flake (a pure formatRelativeTime test timing out); green on re-run.
ord669
commented
Sep 2, 2026
All three correct, fixed in 999a91c.
Tests at 1271. The Windows failure on the previous commit was a slow-runner flake (a pure @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:999a91ce58
ℹ️ 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".
| const res = await apiClient('/workspaces', { | ||
| signal: AbortSignal.timeout(HEALTH_TIMEOUT_MS), |
There was a problem hiding this comment.
When stored WorkOS credentials need refreshing and the WorkOS endpoint stalls, this 10-second signal does not bound the doctor probe: apiClient completes validAccessToken() before forwarding these options to the workspace request, while the refresh uses its own 30-second timeout. Consequently doctor can still pause for roughly 30 seconds despite the stated fail-fast budget; enforce the deadline around the entire apiClient operation or propagate it through token refresh.
Useful? React with 👍 / 👎.
| } | ||
| const buf = Buffer.from(await res.arrayBuffer()); | ||
| const buf = Buffer.from(await readBody(res.arrayBuffer())); |
There was a problem hiding this comment.
When GitHub returns successful headers but the binary body later stalls or disconnects, the transfer timeout rejects res.arrayBuffer() here and readBody converts it to a NetworkError outside the preceding catch. This bypasses the documented BINARY_DOWNLOAD_FAILED/exit-4 mapping and can misleadingly mention the HookMyApp API; include the body read in the download-error wrapper so all transfer failures retain the URL and cause.
Useful? React with 👍 / 👎.
Fifth Codex pass, both correct. doctor's 10s probe bounded only the request it could see. apiClient refreshes the token first, inside the same call, so a stalled WorkOS endpoint still allowed 10 + 30. The signal now threads through validAccessToken into refreshToken, so a caller's deadline covers the whole call rather than the part of it the caller happens to touch. The cloudflared body read reported NetworkError while the connect failure beside it reported BINARY_DOWNLOAD_FAILED; a body that stalls halfway is still a failed download, so both say the same thing now. Test asserts the WorkOS refresh carries the caller's signal; verified it fails without the threading.
ord669
commented
Sep 2, 2026
Both correct, fixed in b2264ab.
Test asserts the WorkOS refresh carries the caller's signal; verified it fails without the threading. 1272 tests, both platforms green. @codex review |
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Uh oh!
There was an error while loading. Please reload this page.
Fixes AIT-540.
The bug
Support report, CLI 0.14.20 on Windows:
mcp-headersinvocations spawned by an MCP client never exited. 8 concurrent processes reached 1.5-3.9 GB RSS each, exhausting 21 GB of RAM.Node's
fetchhas no default timeout. A server that accepts the connection and never answers (blackholing proxy, VPN, captive portal) pins the process forever.mcp-headershits it on first use viagetMcpAccessToken()->mint()->apiClient('/agent/credentials'). The hang lands beforeflushAndExit, so the 2sprocess.exitnet from AIT-395 never arms, and an MCP client cannot reap the helpers it spawns — so they pile up.Twelve fetch call sites had no timeout, not one.
doctor's health probe was the worst: the command you run because the network is broken hung on a broken network.The fix
src/api/timed-fetch.tsis now the only place allowed to callfetch. Two shapes, because one timeout does not fit both:timedFetchconnectTimedFetchtext/event-streamThe SSE distinction matters: both stream call sites (
channels-logs/api.ts,sandbox/logs.ts) are long-lived by design, and a total timeout would have cut off working streams — a second bug shipped inside the first.connectTimedFetchclears its timer once headers arrive, so the body streams untouched.A caller's own signal always wins, so
support watch's long-poll budget and Ctrl+C handling are unchanged.Call sites:
api/client.ts(3),api/gateway.ts(3),auth/login.ts(3),doctor.ts,sandbox/send.ts,sandbox/logs.ts,channels-logs/api.ts,sandbox-listen/binary.ts,sandbox-listen/version-check.ts,notifications-nudge.ts(detached child, inline).agent-auth.ts's duplicate localtimedFetchdeleted.isNetworkFailurenow recognisesTimeoutError, so every existing catch block maps an abort toNetworkError(exit 5) with no new error path.Tuning:
mcp-headersdeadline 45s -> 15s. First use mints over two sequential requests, so a per-request bound alone allowed 60s — longer than any MCP client waits. 15s covers a cold handshake plus both round trips.doctorprobes 30s -> 10s each. It exists to report a broken network, not to sit on one.Verification
Against a local server that accepts and never responds:
mcp-headersdoctorNot-logged-in still exits 4 immediately. Full suite green: 1266 passed, 8 new tests. The SSE test pins that the signal is not aborted after headers arrive.
no-raw-fetch.test.tsfails the build on any newly added rawfetch— verified it catches a planted one, since a guard that cannot fail is not a guard.Ruled out as causes: an unbounded/streaming response body (exits fast) and a plain unauthenticated state (exits cleanly).