[PR #123] [MERGED] fix: derive undici's inactivity limits from the caller's timeout #125

New issue

Closed

opened 2026-10-31 22:04:47 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 22:04:47 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-typescript-sdk/pull/123
Author: @mattmillerai
Created: 9/3/2026
Status: ✅ Merged
Merged: 9/4/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-11864-transport-headers-timeout

📝 Commits (2)

6cf1957 fix: derive undici's inactivity limits from the caller's timeout
9c92fb7 fix: delegate to a carried dispatcher, and leave an injected fetch alone

📊 Changes

7 files changed (+759 additions, -11 deletions)

View changed files

📝 CHANGELOG.md (+16 -0)
➕ src/low/dispatcher.test.ts (+343 -0)
➕ src/low/dispatcher.ts (+193 -0)
📝 src/low/transport.ts (+47 -8)
📝 src/sdk/models.test.ts (+42 -0)
📝 src/sdk/models.ts (+16 -3)
➕ test/support/dispatchers.ts (+102 -0)

📄 Description

ELI-5

You can ask this SDK to wait ten minutes for a slow model. It wasn't waiting ten minutes. Node's fetch has a second, hidden stopwatch that stops at five, and no amount of asking politely (an AbortSignal) can move it — it lives on the connection pool, not on the request. So anything slower than five minutes died with TypeError: fetch failed and nothing else: no HTTP status, no server request id to quote in a support ticket. This PR sets that hidden stopwatch from the deadline you actually asked for.

The defect

AbortSignal.timeout(effective) is not the only clock on a fetch. On Node — the only runtime this package supports (engines.node >= 22, no browser field, no browser export condition) — fetch is undici, and undici keeps two timers of its own on the dispatcher, where no signal can reach them: headersTimeout (how long it waits for response headers, default 300 s) and bodyTimeout (the gap it tolerates between body chunks, default 300 s). Anything above 300 s was therefore silently capped at 300 s, and it failed in the worst possible shape for the caller: TypeError: fetch failed, cause.code = UND_ERR_HEADERS_TIMEOUT, no status, no request id — while the generation itself had already been dispatched and billed.

Two things sharpen this beyond "callers who ask for a long timeout":

comfy.models.run is made entirely of long requests. The server polls the provider inside the call, so nothing at all arrives on the connection — not even response headers — until the generation finishes. Its own default deadline is DEFAULT_RUN_TIMEOUT_MS = 600_000, so the default path was already capped at half its own default; you did not have to ask for anything unusual to hit this.
bodyTimeout is the second half. Raising only headersTimeout relocates the wall onto the response body rather than removing it, so both are set.

The fix

src/low/dispatcher.ts (new) derives both limits from the same effective deadline that builds the abort signal — deadline + 30 s, so the signal always wins the race and the caller gets their own TimeoutError rather than an opaque headers timeout — and hands fetch a dispatcher carrying them. null (no deadline) disables both, which is what timeoutMs: null already means for the signal.

The dispatcher is a delegate, not an Agent:

No setGlobalDispatcher. A library must not change the behaviour of the host application's own unrelated fetches. The delegate is created per request and dies with it.
No Agent is constructed at all, so there is no connection pool to build, cache or churn, and no cache to bound. The delegate forwards to the dispatcher the process is already using, adding the two limits to the per-request dispatch options (undici reads them there, and they take precedence over the dispatcher's own defaults). A host that installed a proxy agent keeps it, untouched.
It degrades rather than fails. Off Node, or on a future runtime that parks its dispatcher elsewhere, the lookup returns nothing and the init is returned unchanged — exactly today's behaviour.

Applied at both of the package's fetch call sites. I swept src/ for call sites: there are exactly two — src/low/transport.ts (the choke point every typed low method wraps) and src/sdk/models.ts (the router run, which does not go through ComfyLow.request and would have kept the bug had only the transport been fixed). Both are fixed; zero unfixed call sites remain.

Verification

Two scaled reproductions, run against the built dist/, with the ambient dispatcher's default set to 250 ms standing in for undici's 300 s and a 3 000 ms deadline standing in for a 660 s budget (a 1/1200-scale model of the reported failure — a server that withholds headers behaves identically at either scale):

# ComfyLow.request
pre-fix   failed after 1007ms  name=TypeError  message=fetch failed  cause=HeadersTimeoutError/UND_ERR_HEADERS_TIMEOUT
post-fix  failed after 3002ms  name=TimeoutError  message=The operation was aborted due to timeout  cause=none

# comfy.models.run (retries off, so the first failure is the one that surfaces)
pre-fix   failed after 1009ms  name=TypeError  message=fetch failed  cause=HeadersTimeoutError/UND_ERR_HEADERS_TIMEOUT
post-fix  failed after 3002ms  name=ComfyError  code=request_timeout  message=models.run(...) exceeded its 3000ms deadline...

That is the reported signature before, and the caller's own deadline after. Those scripts were throwaway and are not committed; what is committed is the equivalent as tests.

Incidental confirmation of the reported shape, from the models.run reproduction: with the default retry budget (DEFAULT_RETRY_BUDGET_MS = 120_000) the headers timeout is retryable, so it is retried and eventually surfaces as this SDK's own request_timeout. A first attempt that fails at ~300 s is already past that 120 s budget, so no retry is started and the bare TypeError: fetch failed is what reaches the caller — which is exactly what the long-running failures reported.

Committed regression tests (src/low/dispatcher.test.ts, plus two in src/sdk/models.test.ts), none of which wait five real minutes:

A local server that withholds response headers forever. A short timeoutMs aborts on schedule, and the failure is the caller's TimeoutError — asserted to not be UND_ERR_HEADERS_TIMEOUT.
The runtime assumption is asserted, not believed. One test forces a 250 ms limit through the delegate against the real process dispatcher and requires UND_ERR_HEADERS_TIMEOUT back. If a future Node stops accepting this delegate, or stops honouring per-request limits, that test reddens instead of the 300 s cap quietly returning.
Table-driven across the 300 s boundary, both on the derivation and end-to-end through ComfyLow.request: 120 000 and 400 000 / 660 000 ms, plus the client default and null. The rows above 300 s assert the limit the request is actually handed (> 300 000 and > the deadline) rather than spending 300 s of wall clock proving it fires.
Elapsed deadlines, negative remaining budgets, NaN/Infinity, and a deadline past the timer ceiling (where a clamped limit would sit below the deadline — the defect again — so no limit is imposed instead).

Gates: pnpm typecheck, pnpm lint (oxlint), pnpm format:check (oxfmt), pnpm check:spec-drift, node scripts/check-public-repo-hygiene.mjs and pnpm build all clean; pnpm test 430 passed / 0 failed, run on Node 22, 24 and 26 (CI covers 22 and 24).

Judgment calls

Symbol.for("undici.globalDispatcher.1") is the riskiest line here. It is undocumented Node internals. It is also the key undici's own getGlobalDispatcher() reads — the contract that lets a separately-installed undici share Node's built-in one — and it resolves on Node 22, 24 and 26. It is duck-typed on .dispatch before use, its absence degrades to today's behaviour rather than throwing, and CI asserts on every run that it resolves and that the limits it carries are the ones undici enforces. The alternatives were worse: setGlobalDispatcher is forbidden for a library, and constructing an Agent means adding undici as a runtime dependency, duplicating the copy Node already ships, discarding the host's proxy configuration, and pooling connections per distinct timeout. It also does not work: an Agent from the npm package is rejected by Node's built-in fetch (InvalidArgumentError: invalid onRequestStart method — the two copies speak different handler protocols), which I confirmed before choosing this design.
timeoutMs: null now disables undici's inactivity timers entirely. This is a deliberate behaviour change beyond the reported defect and it is the one thing worth a second opinion. null is documented as "disables the default timeout entirely" and is what the SSE stream (getJobEvents) and asset downloads pass, so this makes the code match the contract — a stream must not die on its own while idle mid-job, and today one silently could at 300 s. The flip side: a timeoutMs: null caller on a connection that goes silent forever now hangs until their own signal fires, where undici's 300 s default used to end it. Raising it to some other constant instead would just be a new wall, which the whole change is about not doing.
The dispatcher is attached even when a caller injects their own fetch, because a wrapper around the platform fetch should get the fix rather than silently lose it, and an implementation that does not understand the key ignores it. The one case that breaks is a fetch from a separately installed undici, which cannot be handed a delegate for Node's copy; that caller wants setGlobalDispatcher on their own undici, which this delegates to untouched. Documented at the call site.
The 30 s grace is derived, not a new ceiling. It only has to exceed undici's timer granularity (~1 s, measured) so the caller's signal reliably wins; the limit still tracks the deadline.

Residual

Not fixed here, and not derivable from this diff:

The four end-to-end rows that surfaced this — beeble/switchx, luma/ray-flash-2, kling/kling-v1 and runway/aleph2, each failing at 300.75–300.78 s while the Python leg passed — are unexercised artifacts for this PR. They live in a separate end-to-end harness repo, need live Router credentials and would spend real generation budget to run, so exercising them here would be both out of reach and a costly mutation; the scaled reproduction above stands in for them. They are expected to go green on their own once that harness's pinned SDK commit moves past this change. Nothing in the harness needs editing — it already passes the correct 660 s budget through to timeoutMs. Someone has to move that pin, and it is not moved by this PR.
A prior run's report attributes beeble/switchx (and, in the same finding, luma/ray-flash-2) to an environment cause — a staging rollout cutting in-flight calls. That attribution is wrong for these two and is superseded by this diagnosis: four independent network events do not land inside a 25 ms band, and the clean re-run of switchx passed because a single uncontended case finished under 300 s, not because a rollout was the cause. The rollout explanation still holds for the two failures it was originally drawn from (at 377.0 s and 257.1 s), neither of which sits at the wall. That report's stored attribution is in another repo and is not corrected by this PR.
A second, distinct defect on kling/kling-v1 is untouched: an earlier run failed that same case at 106.6 s with an HTTP 409 concurrency_limit_exceeded carrying a request id, which this SDK treats as terminal rather than retrying the same-key "already in progress" answer. A 409 with a request id is the service answering; a headers timeout is the connection dying before it answers anything. Same case, two defects — they should not be merged.
Whether the service should hold one connection open for an entire generation at all, rather than exposing a poll handle, is untouched and is a service API-shape question rather than an SDK bug. Long-held connections carry their own load-balancer exposure, and this change makes them longer.
The Python SDK is not touched; it honours its timeout correctly and has no equivalent cliff (one Python failure in the same run sat at 377.0 s, well past where the TypeScript leg gave up).

Provenance

Authored by: agent-work loop
Verified: pnpm test 430 passed / 0 failed (Node 22, 24 and 26); pnpm typecheck, pnpm lint, pnpm format:check, pnpm check:spec-drift, node scripts/check-public-repo-hygiene.mjs, pnpm build all clean; two scaled pre/post reproductions against the built dist/ (output quoted above). pnpm check:sdk-parity not run — it is the one gate that reaches the network and this change touches no public surface.
Deviations: none against the stated acceptance criteria. Two are met by a different mechanism than the criterion anticipated, both stricter rather than looser: the criterion allowing an Agent cached per distinct timeout is met by constructing no Agent at all, and the above-300 s half of the table-driven regression test asserts the limit the request is handed rather than spending 300 s of wall clock — the criterion's own "without waiting five real minutes" constraint.

Summary by CodeRabbit

New Features
- Requests with timeouts longer than five minutes now honor the caller’s full deadline.
- Network inactivity limits are derived from each request’s remaining timeout.
- Setting timeoutMs: null disables both the request deadline and inactivity limits.
- Model requests now apply these timeout settings consistently across retries.
Bug Fixes
- Improved consistency between request cancellation and connection inactivity handling.
- Existing request options and retry behavior remain preserved.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/comfy-typescript-sdk/pull/123 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/3/2026 **Status:** ✅ Merged **Merged:** 9/4/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-11864-transport-headers-timeout` --- ### 📝 Commits (2) - [`6cf1957`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/6cf195778dce61f2bde53d0fe49226580811b51c) fix: derive undici's inactivity limits from the caller's timeout - [`9c92fb7`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/9c92fb7b60449e8a19f58176c3bf89ef9befca92) fix: delegate to a carried dispatcher, and leave an injected fetch alone ### 📊 Changes **7 files changed** (+759 additions, -11 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+16 -0) ➕ `src/low/dispatcher.test.ts` (+343 -0) ➕ `src/low/dispatcher.ts` (+193 -0) 📝 `src/low/transport.ts` (+47 -8) 📝 `src/sdk/models.test.ts` (+42 -0) 📝 `src/sdk/models.ts` (+16 -3) ➕ `test/support/dispatchers.ts` (+102 -0) </details> ### 📄 Description ## ELI-5 You can ask this SDK to wait ten minutes for a slow model. It wasn't waiting ten minutes. Node's `fetch` has a second, hidden stopwatch that stops at five, and no amount of asking politely (an `AbortSignal`) can move it — it lives on the connection pool, not on the request. So anything slower than five minutes died with `TypeError: fetch failed` and nothing else: no HTTP status, no server request id to quote in a support ticket. This PR sets that hidden stopwatch from the deadline you actually asked for. ## The defect `AbortSignal.timeout(effective)` is not the only clock on a `fetch`. On Node — the only runtime this package supports (`engines.node >= 22`, no `browser` field, no browser export condition) — `fetch` is undici, and undici keeps two timers of its own on the **dispatcher**, where no signal can reach them: `headersTimeout` (how long it waits for response headers, default 300 s) and `bodyTimeout` (the gap it tolerates between body chunks, default 300 s). Anything above 300 s was therefore silently capped at 300 s, and it failed in the worst possible shape for the caller: `TypeError: fetch failed`, `cause.code = UND_ERR_HEADERS_TIMEOUT`, no status, no request id — while the generation itself had already been dispatched and billed. Two things sharpen this beyond "callers who ask for a long timeout": - **`comfy.models.run` is made entirely of long requests.** The server polls the provider inside the call, so nothing at all arrives on the connection — not even response headers — until the generation finishes. Its own default deadline is `DEFAULT_RUN_TIMEOUT_MS = 600_000`, so **the default path was already capped at half its own default**; you did not have to ask for anything unusual to hit this. - **`bodyTimeout` is the second half.** Raising only `headersTimeout` relocates the wall onto the response body rather than removing it, so both are set. ## The fix `src/low/dispatcher.ts` (new) derives both limits from the same effective deadline that builds the abort signal — `deadline + 30 s`, so the signal always wins the race and the caller gets their own `TimeoutError` rather than an opaque headers timeout — and hands `fetch` a dispatcher carrying them. `null` (no deadline) disables both, which is what `timeoutMs: null` already means for the signal. The dispatcher is a **delegate**, not an `Agent`: - **No `setGlobalDispatcher`.** A library must not change the behaviour of the host application's own unrelated fetches. The delegate is created per request and dies with it. - **No `Agent` is constructed at all**, so there is no connection pool to build, cache or churn, and no cache to bound. The delegate forwards to the dispatcher the process is *already* using, adding the two limits to the per-request dispatch options (undici reads them there, and they take precedence over the dispatcher's own defaults). A host that installed a proxy agent keeps it, untouched. - **It degrades rather than fails.** Off Node, or on a future runtime that parks its dispatcher elsewhere, the lookup returns nothing and the init is returned unchanged — exactly today's behaviour. Applied at **both** of the package's `fetch` call sites. I swept `src/` for call sites: there are exactly two — `src/low/transport.ts` (the choke point every typed `low` method wraps) and `src/sdk/models.ts` (the router `run`, which does **not** go through `ComfyLow.request` and would have kept the bug had only the transport been fixed). Both are fixed; **zero** unfixed call sites remain. ## Verification Two scaled reproductions, run against the built `dist/`, with the ambient dispatcher's default set to 250 ms standing in for undici's 300 s and a 3 000 ms deadline standing in for a 660 s budget (a 1/1200-scale model of the reported failure — a server that withholds headers behaves identically at either scale): ``` # ComfyLow.request pre-fix failed after 1007ms name=TypeError message=fetch failed cause=HeadersTimeoutError/UND_ERR_HEADERS_TIMEOUT post-fix failed after 3002ms name=TimeoutError message=The operation was aborted due to timeout cause=none # comfy.models.run (retries off, so the first failure is the one that surfaces) pre-fix failed after 1009ms name=TypeError message=fetch failed cause=HeadersTimeoutError/UND_ERR_HEADERS_TIMEOUT post-fix failed after 3002ms name=ComfyError code=request_timeout message=models.run(...) exceeded its 3000ms deadline... ``` That is the reported signature before, and the caller's own deadline after. Those scripts were throwaway and are not committed; what is committed is the equivalent as tests. Incidental confirmation of the reported shape, from the `models.run` reproduction: with the **default** retry budget (`DEFAULT_RETRY_BUDGET_MS = 120_000`) the headers timeout is retryable, so it is retried and eventually surfaces as this SDK's own `request_timeout`. A first attempt that fails at ~300 s is already past that 120 s budget, so no retry is started and the bare `TypeError: fetch failed` is what reaches the caller — which is exactly what the long-running failures reported. Committed regression tests (`src/low/dispatcher.test.ts`, plus two in `src/sdk/models.test.ts`), none of which wait five real minutes: - **A local server that withholds response headers forever.** A short `timeoutMs` aborts on schedule, and the failure is the caller's `TimeoutError` — asserted to *not* be `UND_ERR_HEADERS_TIMEOUT`. - **The runtime assumption is asserted, not believed.** One test forces a 250 ms limit through the delegate against the real process dispatcher and requires `UND_ERR_HEADERS_TIMEOUT` back. If a future Node stops accepting this delegate, or stops honouring per-request limits, that test reddens instead of the 300 s cap quietly returning. - **Table-driven across the 300 s boundary**, both on the derivation and end-to-end through `ComfyLow.request`: `120 000` and `400 000` / `660 000` ms, plus the client default and `null`. The rows above 300 s assert the limit the request is actually handed (> 300 000 and > the deadline) rather than spending 300 s of wall clock proving it fires. - Elapsed deadlines, negative remaining budgets, `NaN`/`Infinity`, and a deadline past the timer ceiling (where a clamped limit would sit *below* the deadline — the defect again — so no limit is imposed instead). Gates: `pnpm typecheck`, `pnpm lint` (oxlint), `pnpm format:check` (oxfmt), `pnpm check:spec-drift`, `node scripts/check-public-repo-hygiene.mjs` and `pnpm build` all clean; `pnpm test` 430 passed / 0 failed, run on Node 22, 24 and 26 (CI covers 22 and 24). ## Judgment calls - **`Symbol.for("undici.globalDispatcher.1")` is the riskiest line here.** It is undocumented Node internals. It is also the key undici's own `getGlobalDispatcher()` reads — the contract that lets a separately-installed undici share Node's built-in one — and it resolves on Node 22, 24 and 26. It is duck-typed on `.dispatch` before use, its absence degrades to today's behaviour rather than throwing, and CI asserts on every run that it resolves *and* that the limits it carries are the ones undici enforces. The alternatives were worse: `setGlobalDispatcher` is forbidden for a library, and constructing an `Agent` means adding `undici` as a runtime dependency, duplicating the copy Node already ships, discarding the host's proxy configuration, and pooling connections per distinct timeout. It also does not work: an `Agent` from the npm package is rejected by Node's built-in `fetch` (`InvalidArgumentError: invalid onRequestStart method` — the two copies speak different handler protocols), which I confirmed before choosing this design. - **`timeoutMs: null` now disables undici's inactivity timers entirely.** This is a deliberate behaviour change beyond the reported defect and it is the one thing worth a second opinion. `null` is documented as "disables the default timeout entirely" and is what the SSE stream (`getJobEvents`) and asset downloads pass, so this makes the code match the contract — a stream must not die on its own while idle mid-job, and today one silently could at 300 s. The flip side: a `timeoutMs: null` caller on a connection that goes silent forever now hangs until their own `signal` fires, where undici's 300 s default used to end it. Raising it to some other constant instead would just be a new wall, which the whole change is about not doing. - **The dispatcher is attached even when a caller injects their own `fetch`**, because a wrapper around the platform `fetch` should get the fix rather than silently lose it, and an implementation that does not understand the key ignores it. The one case that breaks is a `fetch` from a *separately installed* undici, which cannot be handed a delegate for Node's copy; that caller wants `setGlobalDispatcher` on their own undici, which this delegates to untouched. Documented at the call site. - **The 30 s grace is derived, not a new ceiling.** It only has to exceed undici's timer granularity (~1 s, measured) so the caller's signal reliably wins; the limit still tracks the deadline. ## Residual Not fixed here, and not derivable from this diff: - **The four end-to-end rows that surfaced this** — `beeble/switchx`, `luma/ray-flash-2`, `kling/kling-v1` and `runway/aleph2`, each failing at 300.75–300.78 s while the Python leg passed — are **unexercised artifacts** for this PR. They live in a separate end-to-end harness repo, need live Router credentials and would spend real generation budget to run, so exercising them here would be both out of reach and a costly mutation; the scaled reproduction above stands in for them. They are expected to go green on their own once that harness's pinned SDK commit moves past this change. Nothing in the harness needs editing — it already passes the correct 660 s budget through to `timeoutMs`. **Someone has to move that pin**, and it is not moved by this PR. - **A prior run's report attributes `beeble/switchx` (and, in the same finding, `luma/ray-flash-2`) to an environment cause — a staging rollout cutting in-flight calls.** That attribution is wrong for these two and is superseded by this diagnosis: four independent network events do not land inside a 25 ms band, and the clean re-run of `switchx` passed because a single uncontended case finished under 300 s, not because a rollout was the cause. The rollout explanation still holds for the two failures it was originally drawn from (at 377.0 s and 257.1 s), neither of which sits at the wall. **That report's stored attribution is in another repo and is not corrected by this PR.** - **A second, distinct defect on `kling/kling-v1` is untouched:** an earlier run failed that same case at 106.6 s with an HTTP 409 `concurrency_limit_exceeded` carrying a request id, which this SDK treats as terminal rather than retrying the same-key "already in progress" answer. A 409 with a request id is the service answering; a headers timeout is the connection dying before it answers anything. Same case, two defects — they should not be merged. - **Whether the service should hold one connection open for an entire generation at all**, rather than exposing a poll handle, is untouched and is a service API-shape question rather than an SDK bug. Long-held connections carry their own load-balancer exposure, and this change makes them longer. - **The Python SDK is not touched**; it honours its timeout correctly and has no equivalent cliff (one Python failure in the same run sat at 377.0 s, well past where the TypeScript leg gave up). ## Provenance - **Authored by:** agent-work loop - **Verified:** `pnpm test` 430 passed / 0 failed (Node 22, 24 and 26); `pnpm typecheck`, `pnpm lint`, `pnpm format:check`, `pnpm check:spec-drift`, `node scripts/check-public-repo-hygiene.mjs`, `pnpm build` all clean; two scaled pre/post reproductions against the built `dist/` (output quoted above). `pnpm check:sdk-parity` not run — it is the one gate that reaches the network and this change touches no public surface. - **Deviations:** none against the stated acceptance criteria. Two are met by a different mechanism than the criterion anticipated, both stricter rather than looser: the criterion allowing an `Agent` cached per distinct timeout is met by constructing no `Agent` at all, and the above-300 s half of the table-driven regression test asserts the limit the request is handed rather than spending 300 s of wall clock — the criterion's own "without waiting five real minutes" constraint.  ## Summary by CodeRabbit * **New Features** * Requests with timeouts longer than five minutes now honor the caller’s full deadline. * Network inactivity limits are derived from each request’s remaining timeout. * Setting `timeoutMs: null` disables both the request deadline and inactivity limits. * Model requests now apply these timeout settings consistently across retries. * **Bug Fixes** * Improved consistency between request cancellation and connection inactivity handling. * Existing request options and retry behavior remain preserved.  --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 22:04:47 +00:00