[PR #145] [MERGED] feat(models): cap the response body models.run will buffer #148

New issue

Closed

opened 2026-10-31 22:05:02 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 22:05:02 +00:00

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-14155-models-run-max-bytes

📝 Commits (2)

1ab5cfe feat(models): cap the response body models.run will buffer
58507d2 Merge main into the response-size-cap branch, and answer the review

📊 Changes

6 files changed (+873 additions, -51 deletions)

View changed files

📝 CHANGELOG.md (+35 -0)
📝 README.md (+19 -1)
📝 src/sdk/index.ts (+1 -0)
📝 src/sdk/models.test.ts (+317 -0)
📝 src/sdk/models.ts (+434 -47)
📝 test/support/router-stub-server.ts (+67 -3)

📄 Description

ELI-5

comfy.models.run reads the whole response into memory before it hands you anything, and until now nothing said how big "the whole response" was allowed to be. This adds a ceiling: maxBytes, defaulting to 64 MiB. If the response says up front that it is bigger, the SDK hangs up before downloading it; if it never says, the SDK counts the bytes as they arrive and stops when they go past the limit. Either way you get a clear error you can branch on — and it is deliberately not retried, because asking again would just download the same too-big body all over again.

The ceiling never changes what a response means, though. If the SDK was going to ask again anyway (a retryable 503, a 409 where the server is still finishing your generation), it does not read the body at all, so a giant error page from some proxy in the middle cannot turn "try again" into "give up". And an error the SDK does hand you keeps its identity — an oversized 402 is still InsufficientCredits, just with a shortened message.

What changed

RunOptions.maxBytes?: number | null — the largest response body one call will buffer. Omitted, it is the new exported DEFAULT_MAX_RESPONSE_BYTES (64 MiB); null disables the cap entirely. Documented on the option, on the constant, in the module header, and in the README next to timeoutMs.
Two checks, because they catch different responses. A Content-Length over the cap is refused before the body is read at all, and the body stream is cancelled so the oversized response is never downloaded — that is the cheap exit. The bytes actually read are then counted against the same cap, since a chunked response declares no length, a declared length is the encoded length when a body arrives compressed, and in any case it is a claim by the sender rather than a bound on it.
Its own error bucket. A breach raises a ComfyError with code: "response_too_large" rather than unexpected_response, carrying { maxBytes, contentLength } or { maxBytes, bytesRead } on details, plus the usual requestId / idempotencyKey / httpStatus / retryAfter. details.maxBytes is the discriminator, not the code — errorFromResponse derives code verbatim from the server-controlled X-Comfy-Error-Type, so an upstream can mint the same string; only a local breach carries maxBytes.
Not retried. The breach leaves the retry loop immediately instead of being classified as a transport failure. That is the amplification half of the problem: the body read sits in the same try as the fetch, so without this the loop re-downloads the same oversized body on every attempt until the budget expires.
maxBytes is validated at the call site, before a request goes out, so a process that asked for a cap it cannot have finds out before a model runs and is billed. NaN is the case worth the explicit check — every comparison against it is false, so an unchecked NaN would read as "no cap" and silently undo the ceiling the caller thought they set.
Classification comes first, and the body is only read when it is going to be used. A response run will retry or collect is decided from its status line and headers, and its body is dropped unread — so an oversized upstream error page cannot make a retryable 502 fatal with the budget unspent, or abandon a generation the server is still holding behind a collectable 409/504. An error response run does surface is truncated at the cap rather than refused, because the bucket a caller branches on lives in the status and X-Comfy-Error-Type and the body only enriches the message. Only a 200 result past the cap raises.
The read accumulates into one growable buffer, sized from Content-Length when there is one and doubling (clamped to the cap) when there is not, so each chunk is copied in and dropped as it arrives. A chunk list would bound the payload but not the per-chunk object overhead nor the socket-read buffer each view pins, and its final concatenation would hold every chunk and the copy at once. A failed allocation inside the read leaves by the same non-retryable door the cap breach does, rather than as a bare RangeError the retry branch would amplify.
The decode happens once over the whole buffer — in finish, on the JSON branch only, since #139 made the run route's 200 sometimes binary — so a multi-byte character split across a chunk boundary survives where a per-chunk decode would replace it with U+FFFD. That is pinned by its own test.

Test fixtures: test/support/router-stub-server.ts gains stallBodyContentLength (the stall fixture's declared length, default unchanged at 4096) and a chunkedBody fixture that writes N chunks with no Content-Length, respecting backpressure so a client that stops reading actually stops the transfer.

Judgment calls

The default is 64 MiB. Comfortably above what the catalog returns today (a JSON document is kilobytes; an ElevenLabs mp3 is about a megabyte a minute) and far below the size at which buffering is the process's problem rather than the response's. A genuinely larger generation — video, once the catalog has it — raises maxBytes or passes null; both are documented in the README with examples.
The cap bounds what is buffered; it never overrules what a response means. An error body is truncated at the cap rather than refused, and a body on a path run is going to re-ask about is never read. The alternative — one cap check over a shared read — is what the first round of review rejected, on three counts that are all real: a retryable 502 behind a CDN error page became permanently fatal, a collectable 409/504 abandoned a billed generation, and an oversized 401/402 body lost Unauthorized/InsufficientCredits for a bare ComfyError. Stated on the option's JSDoc and in the README.
response_too_large is a client-side code, not a Router error_type bucket, so it is deliberately not added to src/sdk/routerErrors.ts — that set is closed against spec/router-openapi.yaml and only the contract may grow it. No new exception class either: the ticket asked for a code to branch on, and a new class would be a cross-SDK surface change the Python twin has not made.
A Content-Length header this cannot parse is treated as absent (a header sent twice reaches Headers.get as "n, n"; a proxy can send something that is not a number). Refusing a response over an unparseable length would be guessing; the read-side count is what covers it.

Verification of the denial path

This diff adds a throw that denies a capability (buffering a body past the ceiling), so before shipping it I went and confirmed the capability is still reachable rather than dead-ended, through every path this SDK offers: maxBytes: <larger> and maxBytes: null. Both were exercised empirically, not asserted — disables the cap entirely on maxBytes: null runs the exact fixture the cap rejects and resolves it successfully, and admits a body of exactly maxBytes and refuses one byte more pins the boundary from both sides. The denial is conditional and caller-configurable, with its escape hatch in the same change, tested and documented.

I also mutation-checked the two claims this PR makes that a green suite alone would not prove:

Disabling the Content-Length pre-check makes both pre-check tests time out rather than fail an assertion — proving they reject before the body is read, not after.
Removing the not-retried classification makes the breach retry 255 times on one call against a 5 s budget, which is the amplification the change exists to prevent.

Test plan

pnpm test — 620 passed, 0 failed (26 files); 15 tests under comfy.models.run response size cap, including the four that pin the classification order (retryable status kept retryable behind an oversized body, a collect that completes behind an oversized 409, an oversized 402 that stays InsufficientCredits, and a 200 result that still raises).
pnpm typecheck, pnpm lint, pnpm format:check, pnpm check:spec-drift, pnpm check:sdk-parity, node scripts/check-public-repo-hygiene.mjs, pnpm build — all clean.

Residual

Not fixed here, and worth its own follow-up:

Three other body-buffering call sites in this package are still uncapped, and are out of scope by the ticket's own instruction to keep the change to the run path. The full sweep of src/ for .text() / .json() / .arrayBuffer() outside tests returns four sites; this PR caps one. The other three: src/low/transport.ts:327,332 (the v2 JSON envelope — documents, not generated media), src/sdk/outputs.ts:119 (Output.toBytes() — genuinely large by design, and the caller explicitly asked for the bytes), and src/sdk/assets.ts:301 (AssetFactory.fromUrl — buffers an arbitrary caller-supplied URL into memory with timeoutMs: null, which is the one of the three most worth a cap of its own).
The unfixed-capability check above is scoped to this SDK's own surface. Whether Comfy Router itself bounds what a partner response can be was not investigated — that is a server-side question this repo cannot answer.

Provenance

Authored by: agent-work loop
Verified: pnpm test: 620 passed, 0 failed; pnpm typecheck / lint / format:check: clean; pnpm check:spec-drift: in sync (generated code + Router route); pnpm check:sdk-parity: snapshot matches the Python SDK's default branch; check-public-repo-hygiene: clean; pnpm build: clean, no dirty files. Two mutation checks run and reverted, as described above.
Deviations: none — every acceptance criterion is met. The remainder is in ## Residual, which is scope the change deliberately does not cover rather than criteria it missed.
Review round: main merged in (#133, #139, #138 all landed in src/sdk/models.ts after this branch cut) and all seven bot review findings addressed — the classification-before-read restructure, the growable buffer, the non-retryable allocation failure, details.maxBytes as the discriminator, retryAfter on the breach, got NaN, and the abandoned-read wording.

_{🔄 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/145 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/14/2026 **Status:** ✅ Merged **Merged:** 9/14/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-14155-models-run-max-bytes` --- ### 📝 Commits (2) - [`1ab5cfe`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/1ab5cfe0898d38c72e6bfe143d979c501bd4e4de) feat(models): cap the response body models.run will buffer - [`58507d2`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/58507d2a3f3fc22c46e250c739a201426947536a) Merge main into the response-size-cap branch, and answer the review ### 📊 Changes **6 files changed** (+873 additions, -51 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+35 -0) 📝 `README.md` (+19 -1) 📝 `src/sdk/index.ts` (+1 -0) 📝 `src/sdk/models.test.ts` (+317 -0) 📝 `src/sdk/models.ts` (+434 -47) 📝 `test/support/router-stub-server.ts` (+67 -3) </details> ### 📄 Description ## ELI-5 `comfy.models.run` reads the whole response into memory before it hands you anything, and until now nothing said how big "the whole response" was allowed to be. This adds a ceiling: `maxBytes`, defaulting to 64 MiB. If the response says up front that it is bigger, the SDK hangs up before downloading it; if it never says, the SDK counts the bytes as they arrive and stops when they go past the limit. Either way you get a clear error you can branch on — and it is deliberately not retried, because asking again would just download the same too-big body all over again. The ceiling never changes what a response *means*, though. If the SDK was going to ask again anyway (a retryable `503`, a `409` where the server is still finishing your generation), it does not read the body at all, so a giant error page from some proxy in the middle cannot turn "try again" into "give up". And an error the SDK does hand you keeps its identity — an oversized `402` is still `InsufficientCredits`, just with a shortened message. ## What changed - **`RunOptions.maxBytes?: number | null`** — the largest response body one call will buffer. Omitted, it is the new exported `DEFAULT_MAX_RESPONSE_BYTES` (64 MiB); `null` disables the cap entirely. Documented on the option, on the constant, in the module header, and in the README next to `timeoutMs`. - **Two checks, because they catch different responses.** A `Content-Length` over the cap is refused *before the body is read at all*, and the body stream is cancelled so the oversized response is never downloaded — that is the cheap exit. The bytes actually read are then counted against the same cap, since a chunked response declares no length, a declared length is the *encoded* length when a body arrives compressed, and in any case it is a claim by the sender rather than a bound on it. - **Its own error bucket.** A breach raises a `ComfyError` with `code: "response_too_large"` rather than `unexpected_response`, carrying `{ maxBytes, contentLength }` or `{ maxBytes, bytesRead }` on `details`, plus the usual `requestId` / `idempotencyKey` / `httpStatus` / `retryAfter`. **`details.maxBytes` is the discriminator, not the code** — `errorFromResponse` derives `code` verbatim from the server-controlled `X-Comfy-Error-Type`, so an upstream can mint the same string; only a local breach carries `maxBytes`. - **Not retried.** The breach leaves the retry loop immediately instead of being classified as a transport failure. That is the amplification half of the problem: the body read sits in the same `try` as the `fetch`, so without this the loop re-downloads the same oversized body on every attempt until the budget expires. - **`maxBytes` is validated at the call site**, before a request goes out, so a process that asked for a cap it cannot have finds out before a model runs and is billed. `NaN` is the case worth the explicit check — every comparison against it is false, so an unchecked `NaN` would read as "no cap" and silently undo the ceiling the caller thought they set. - **Classification comes first, and the body is only read when it is going to be used.** A response `run` will retry or collect is decided from its status line and headers, and its body is dropped unread — so an oversized upstream error page cannot make a retryable `502` fatal with the budget unspent, or abandon a generation the server is still holding behind a collectable `409`/`504`. An error response `run` *does* surface is truncated at the cap rather than refused, because the bucket a caller branches on lives in the status and `X-Comfy-Error-Type` and the body only enriches the message. **Only a `200` result past the cap raises.** - **The read accumulates into one growable buffer**, sized from `Content-Length` when there is one and doubling (clamped to the cap) when there is not, so each chunk is copied in and dropped as it arrives. A chunk list would bound the payload but not the per-chunk object overhead nor the socket-read buffer each view pins, and its final concatenation would hold every chunk *and* the copy at once. A failed allocation inside the read leaves by the same non-retryable door the cap breach does, rather than as a bare `RangeError` the retry branch would amplify. - The decode happens **once over the whole buffer** — in `finish`, on the JSON branch only, since #139 made the run route's `200` sometimes binary — so a multi-byte character split across a chunk boundary survives where a per-chunk decode would replace it with U+FFFD. That is pinned by its own test. Test fixtures: `test/support/router-stub-server.ts` gains `stallBodyContentLength` (the stall fixture's declared length, default unchanged at 4096) and a `chunkedBody` fixture that writes N chunks with **no** `Content-Length`, respecting backpressure so a client that stops reading actually stops the transfer. ## Judgment calls - **The default is 64 MiB.** Comfortably above what the catalog returns today (a JSON document is kilobytes; an ElevenLabs mp3 is about a megabyte a minute) and far below the size at which buffering is the process's problem rather than the response's. A genuinely larger generation — video, once the catalog has it — raises `maxBytes` or passes `null`; both are documented in the README with examples. - **The cap bounds what is buffered; it never overrules what a response means.** An error body is truncated at the cap rather than refused, and a body on a path `run` is going to re-ask about is never read. The alternative — one cap check over a shared read — is what the first round of review rejected, on three counts that are all real: a retryable `502` behind a CDN error page became permanently fatal, a collectable `409`/`504` abandoned a billed generation, and an oversized `401`/`402` body lost `Unauthorized`/`InsufficientCredits` for a bare `ComfyError`. Stated on the option's JSDoc and in the README. - **`response_too_large` is a client-side code, not a Router `error_type` bucket**, so it is deliberately *not* added to `src/sdk/routerErrors.ts` — that set is closed against `spec/router-openapi.yaml` and only the contract may grow it. No new exception class either: the ticket asked for a code to branch on, and a new class would be a cross-SDK surface change the Python twin has not made. - **A `Content-Length` header this cannot parse is treated as absent** (a header sent twice reaches `Headers.get` as `"n, n"`; a proxy can send something that is not a number). Refusing a response over an unparseable length would be guessing; the read-side count is what covers it. ## Verification of the denial path This diff adds a `throw` that denies a capability (buffering a body past the ceiling), so before shipping it I went and confirmed the capability is still reachable rather than dead-ended, through every path this SDK offers: `maxBytes: <larger>` and `maxBytes: null`. Both were exercised empirically, not asserted — `disables the cap entirely on maxBytes: null` runs the exact fixture the cap rejects and resolves it successfully, and `admits a body of exactly maxBytes and refuses one byte more` pins the boundary from both sides. The denial is conditional and caller-configurable, with its escape hatch in the same change, tested and documented. I also mutation-checked the two claims this PR makes that a green suite alone would not prove: - Disabling the `Content-Length` pre-check makes both pre-check tests **time out** rather than fail an assertion — proving they reject before the body is read, not after. - Removing the not-retried classification makes the breach retry **255 times** on one call against a 5 s budget, which is the amplification the change exists to prevent. ## Test plan - `pnpm test` — 620 passed, 0 failed (26 files); 15 tests under `comfy.models.run response size cap`, including the four that pin the classification order (retryable status kept retryable behind an oversized body, a collect that completes behind an oversized `409`, an oversized `402` that stays `InsufficientCredits`, and a `200` result that still raises). - `pnpm typecheck`, `pnpm lint`, `pnpm format:check`, `pnpm check:spec-drift`, `pnpm check:sdk-parity`, `node scripts/check-public-repo-hygiene.mjs`, `pnpm build` — all clean. ## Residual Not fixed here, and worth its own follow-up: - **Three other body-buffering call sites in this package are still uncapped**, and are out of scope by the ticket's own instruction to keep the change to the run path. The full sweep of `src/` for `.text()` / `.json()` / `.arrayBuffer()` outside tests returns four sites; this PR caps one. The other three: `src/low/transport.ts:327,332` (the v2 JSON envelope — documents, not generated media), `src/sdk/outputs.ts:119` (`Output.toBytes()` — genuinely large by design, and the caller explicitly asked for the bytes), and `src/sdk/assets.ts:301` (`AssetFactory.fromUrl` — buffers an arbitrary caller-supplied URL into memory with `timeoutMs: null`, which is the one of the three most worth a cap of its own). - **The unfixed-capability check above is scoped to this SDK's own surface.** Whether Comfy Router itself bounds what a partner response can be was not investigated — that is a server-side question this repo cannot answer. ## Provenance - **Authored by:** agent-work loop - **Verified:** pnpm test: 620 passed, 0 failed; pnpm typecheck / lint / format:check: clean; pnpm check:spec-drift: in sync (generated code + Router route); pnpm check:sdk-parity: snapshot matches the Python SDK's default branch; check-public-repo-hygiene: clean; pnpm build: clean, no dirty files. Two mutation checks run and reverted, as described above. - **Deviations:** none — every acceptance criterion is met. The remainder is in `## Residual`, which is scope the change deliberately does not cover rather than criteria it missed. - **Review round:** `main` merged in (#133, #139, #138 all landed in `src/sdk/models.ts` after this branch cut) and all seven bot review findings addressed — the classification-before-read restructure, the growable buffer, the non-retryable allocation failure, `details.maxBytes` as the discriminator, `retryAfter` on the breach, `got NaN`, and the abandoned-read wording. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 22:05:02 +00:00