fix: bound every outbound request with a client-side deadline - #83
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
@justinwlin Great work 🎉 , just a few comments here:
|
|
Reviewed this locally and found two gaps. Both are fixed in 66b3a38. The per-request deadline didn't bound a whole invocationFour handlers issue two sequential calls, and each was getting its own fresh 30s:
4s pre-flight + 30s + 30s = 64s against a 60s
That rests on the runtime being rebuilt per request, which A ceiling that has decayed to zero is floored at 1s instead of aborting before the socket opens: The queued-job diagnosis also gets an explicit 5s on top of the budget. It decorates a status already in hand and is discarded on any failure, so spending the caller's remaining seconds to enrich a reply that was ready is the wrong trade even when it can't cause a 504. A timed-out GraphQL read was described as a possible writeThe retry advice keys off the method, and GraphQL is always POST on the wire. So a stalled Worth noting the write half is only reachable behind a preceding read in both call sites, so the classifier is pinned directly rather than by stalling a handler's second call. Verification568 pass / 0 fail / 0 cancelled on both Node 18.20 and 24.2 (was 558; +10 new). Type-check, lint, build clean. The new tests assert the clamp arithmetic as pure units rather than waiting out real deadlines, so the suite is still ~2.4s. One thing left for a follow-up
But the ladder assertion ( |
node-fetch applies no timeout, so a host that accepts the connection then goes silent leaves a tool call pending until Vercel reaps the function at 60s — a bare 504 with collected output discarded. Every request through createHttpClient and graphqlRequest now carries an AbortSignal with a 30s default, surfacing a named RequestTimeoutError that reports the API, the deadline, and method-appropriate retry advice. runsync-endpoint derives its deadline from the wait it requested (90s server default when omitted), clamped to 50s so the backstop clears #81's 45s server-side hold but stays under the platform limit. On the hosted transport the ceiling is a thunk over the remaining invocation budget, so multi-call handlers cannot spend two full deadlines back to back.
66b3a38 to
a39b304
Compare
Three review findings. stream-job set each poll's deadline to the remaining budget, so one wedged socket spent the whole run (45s hosted, 5 minutes on stdio) in a single attempt and MAX_CONSECUTIVE_STREAM_ERRORS never engaged — on stdio that was worse than the flat 30s default it replaced. A poll now gets the hold the server was asked for plus a round trip: above the hold so a reply in flight is never aborted, well inside the budget so the loop can reconnect. stdio's hold is the upstream /stream default (10s), which it never sent as ?wait=. The loop itself moves out of the handler into collectJobStream. api/index.ts still made unbounded fetches on the OAuth path — the one flow where a blank 504 leaves the user with no credential to retry with. Each flash-backend call now carries its own deadline (MCP_FLASH_TIMEOUT_MS), clamped to what is left of a 45s /token poll budget, and the install wizard's key check no longer hangs on "Verifying…". The changeset and the RequestInitLike comment were claiming coverage those paths did not have; both now say what is actually true. Ten tests: the poll deadline observed rather than inferred, both bounds pinned against the real constants on both transports, a retry loop that reaches its error cap, and a wedged socket driven end to end through the real handler to pollingTimedOut with lastError surfaced.
Review pass over the two commits below. `consecutiveErrors = 0` on a successful poll had no test: six failures spread across a recovery would have ended a healthy stream and every existing test still passed, since none of them ever succeeded after a failure. Covered now, along with the cross-file invariant on TOKEN_POLL_BUDGET_MS that matched HTTP_LONG_POLL_BUDGET_MS's assertion in name only, and MCP_FLASH_TIMEOUT_MS parsing, which CLAUDE.md promises cannot be disabled by a typo. `MIN_FLASH_GRAPHQL_TIMEOUT_MS` claimed to floor the per-call deadline when it floors the remaining budget; it is `MIN_TOKEN_POLL_REMAINDER_MS`, and the disclaimer under it shrinks to nothing. The clamp it fed becomes `tokenPollDeadlineMs`, the sibling `streamPollTimeoutMs` already was, and is now unit-testable rather than inline in a call argument. `FLASH_GRAPHQL_TIMEOUT_MS` is a default, so it says so. Comments: the stdio poll comment still said "no deadline to race" after this branch gave it one; `streamPollTimeoutMs` opened by denying the budget was a bound three lines above a `Math.min` against it; the five constants exported for tests said nothing about why, unlike the two this file already exported; and nothing in `handleToken` recorded that a failed read is deliberately fatal because an APPROVED read consumes the code upstream — only the test knew. `collectJobStream` no longer annotates the object `poll` returned, now that `poll` is caller-supplied. The wedged-socket test waits on the recorded AbortSignal instead of sleeping 3× the deadline and hoping, and `useFakeClock` restores `Date.now` through `t.after`, so a throw mid-drive cannot leave the rest of the file on a frozen clock. tests/handlers.test.ts is prettier-clean; it was already failing on main, including on two lines this branch added.
Second review pass. The reworded `streamPollTimeoutMs` opener said "never past what is left of the budget", which is false in the one case the floor exists for: below 2s remaining it overshoots by design, as its own test asserts. A summary the next paragraph has to walk back is the defect it replaced, inverted. "One read, then we stop" sat above a loop that reads up to 20 times — it is one FAILED read that ends the poll. The vercel.json lookup took the first function declaring a maxDuration rather than the one both budgets run in, so adding a function ahead of `api/index.ts` would have asserted against a limit governing neither. tests/http.test.ts already keys it correctly. Both direct drives of the poll loop awaited a promise that a non-terminating loop never settles — node:test has no default timeout, so that stalls the rest of the file instead of failing. `settled()` now carries the guard `driveStreamJobToBudget` had, and the driver uses it too. `DEFAULT_FLASH_GRAPHQL_TIMEOUT_MS` is exported so the parsing test stops restating 10_000, and that test's env mutation is restored in a finally like its neighbour. Also removes the node_modules symlink `git add -A` re-tracked in 1c8e2b7. `.gitignore` lists `node_modules/`, which does not match a symlink, so it is staged by name rather than caught by the ignore.
…amping the last OAuth read Correctness review of the two commits below. `getFlashTimeoutMs` accepted any positive finite number, but `AbortSignal.timeout` takes a uint32 and throws ERR_OUT_OF_RANGE synchronously on anything else. `MCP_FLASH_TIMEOUT_MS=10000.5` therefore 500s BOTH /authorize and /token — sign-in impossible — and a value just above int32 does not throw at all: it warns and fires immediately, so turning the dial up past ~2.1e9 turns the deadline into 1ms. CLAUDE.md promises a typo cannot disable the deadline; now only a positive integer inside the timer's range is honored, and the test drives the value into AbortSignal.timeout rather than only reading it back. /token no longer clamps a read down to the last second of its budget. An APPROVED read consumes the code atomically upstream, so a read abandoned mid-flight can burn it and leave the user with "already used" on retry. The loop now declines to start a read below MIN_TOKEN_POLL_REMAINDER_MS (5s) and falls through to authorization_pending, which is retryable and consumes nothing. It also sleeps the full interval every time: the old "skip the sleep if it would spend the rest of the budget" re-read the backend back to back for the final ~2s, which bought the user no extra approval time. stream-job's transport choice moves into `streamPollPlan`, which bundles budget, hold and ?wait= — replacing the stdio hold with the http constant left the whole suite green while giving every stdio poll a 6s deadline against a server holding 10s, i.e. aborting a reply in flight five times and ending the run at the error cap in ~30s. Bundled with the budget, that mistake now fails the stdio budget test as well as its own. `lastError` is cleared on a successful poll, so a blip in the first second is no longer reported as the trailing error five minutes later, and the error-cap exit carries the same resume guidance the budget exit does — chunks with no way forward is not an answer. `verifyApiKey` takes its deadline as a parameter so the wizard's fetch, which the changeset advertises as bounded, is actually covered.
…nging node:test has no default timeout, so without the deadline this test held the file open until CI killed the job. Same treatment the OAuth stall test already has.
|
All three findings fixed, plus four more from a self-review pass. Rebased onto Your findings1. The stream-job deadline was worse than what it replaced.
2. flashGraphql. Fixed rather than scoped down — the OAuth handshake is the one place a blank 504 is unrecoverable, since the user has no credential yet to retry with. Every flash call now carries a deadline ( 3. Tests. Ten, in three layers: the deadline observed ( Found while reviewing thisA bug I introduced.
A regression the suite was blind to. Giving stdio the http hold kept 660/660 green while breaking every stdio poll (6s deadline against a 10s hold → aborts a reply in flight → dies at the error cap in ~30s). The transport choice now goes through Nothing exercised the real fetch. Every client test injects a fake, and Node 18 couldn't load the new test file — Smaller: Not taken: retrying a timed-out pre-approval poll. The read may already have consumed the code, so the retry would say "already used" — worse than the honest failure. That reasoning now lives at the call site. Verification675 tests, 667 pass / 0 fail / 8 skipped, on Node 18.20.8 and 24.2. Type-check, lint, build clean. Mutation-verified rather than assumed — each of these turns the suite red:
|
CI Node 18 failed to LOAD tests/install-clients.test.ts: importing wizard.ts pulls in @clack/prompts, and @clack/core imports `styleText` from node:util, which does not exist until Node 20. The whole file died before a single test ran — Node 20, 22 and Windows were all green, and so was my local Node 24. verifyApiKey moves to src/install/verify-key.ts, which has no interactive dependencies, and the wizard imports it from there. Behaviour is unchanged; the deadline is now reachable from a test on every supported runtime. Verified on Node 18.20.8 as well as 24.2: 673 tests, 665 pass, 0 fail.
Every client test injects a fake fetch, and `defaultFetch = fetch as HttpFetch` (src/tools/runtime.ts) is a cast — nothing checks the init object this release added `signal` to against node-fetch's own RequestInit. If node-fetch ignored that field, every deadline here would be inert in production and the whole suite would still be green. So one test builds the real client over a real socket: a server that answers, and one that accepts and never does. Removing `signal` from createHttpClient now hangs this test instead of passing.
2 participants
Follow-up to #81 (merged, and merged in here).
In plain terms
The problem. When our server asks the Runpod API for something, it waits for an answer with no time limit. Almost always fine — answers come back in well under a second. But if a machine on the other end accepts the connection and then goes quiet (a stuck worker, a load balancer holding the socket), there is nothing that ever says "give up." The request just waits.
On the hosted server that matters, because Vercel kills the whole function at 60 seconds. So instead of a useful error, the user gets a blank
504, and anything the tool had already collected is thrown away. Nothing in the message says which call got stuck.The fix. Give every request a deadline. If no answer comes back in 30 seconds, stop waiting and return a real error that names the API, the deadline, and what to do next. The user gets an explanation in 30 seconds instead of a blank failure at 60.
The one exception.
runsync-endpointasks the Runpod API to hold the line while a job runs — waiting is the whole point of it. It gets a longer deadline derived from the wait it requested, so the shared 30-second limit does not cut off a call that was working as intended.The subtle part. Deadlines have to be layered so they fire in the right order — the server-side wait first, our deadline next, the platform's kill last. Get that wrong and the "fix" hangs up on a reply that was already on its way. That is what the 45 / 50 / 60 second ladder below is about.
The gap #81 couldn't close
No outbound request has a client-side timeout.
RequestInitLikeinsrc/_shared/http.tsis{method, headers, body?}— nosignal— and node-fetch applies no default. A host that accepts the connection then goes silent never errors and never responds, so the call hangs until Vercel reaps it: bare 504, collected work discarded, nothing naming which call stalled.#81 fixed the budgets that were arithmetically guaranteed to exceed 60s. It can't fix this one: a budget checked between polls never runs if a single poll never returns.
How
withRequestTimeoutwraps every call throughcreateHttpClient, following the pattern_shared/credential-check.tsand_shared/backend.tsalready use — the general client was the gap.runsync-endpointopts out, deriving its deadline from the wait it actually sent plus slack. Sincewaitis a server-side parameter with its own 90s default, an omittedwaitneeds the long deadline too — a flat 30s would truncate the common case.RequestTimeoutErrornames the API, the deadline, and the remedy instead of a bareAbortError.graphqlRequesthad the same bug; same wrapper covers it.Why the cap is 50s
The deadline is a backstop for a wedged socket, not a budget — so it must sit above the wait it brackets:
Two ways to get this wrong, both found in review and both now asserted by tests that read the real constants:
runsyncwould abort a response already in flight, turning fix(tools): clamp long-poll budgets to 45s on the hosted HTTP transport #81's job-ID-to-poll path back into an error.Tests
11 new: signal on every request; default fires with the right message; per-call override both shortens and lengthens;
maxTimeoutMscaps; a network failure isn't relabelled a timeout; end-to-end against a stalling fake server,runsyncwith and withoutwait.557 pass / 0 fail / 0 cancelled on Node 18.20 and 24.2. Type-check, lint, prettier, build clean.
Two failures found during review, and what caused them
The 45s collision. The first draft capped at 45s, matching #81's hold exactly. Neither branch's tests caught it because the failure only exists in the merged state. Mutation-verified now: at 45s the guard fails with "does not clear the 45000ms server-side hold by a round trip."
18 tests cancelled on Node 18 (exit 1) while Node 24 was green.
AbortSignal.timeoutuses an unref'd timer — correct in production, where a real socket keeps the loop alive, but the test fakes do no I/O, so Node 18's runner drained the loop and cancelled rather than letting the deadline fire ("Promise resolution is still pending but the event loop has already resolved"). Node 20+ holds the loop, so it only reproduced on the oldest lane. Fixed in the fakes with a ref'd keep-alive; production behavior unchanged.