[PR #185] feat(models): return the binary arm from a queued result with a non-JSON Content-Type #185

New issue

Open

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-typescript-sdk/pull/185
Author: @mattmillerai
Created: 9/25/2026
Status: 🔄 Open

Base: main ← Head: matt/be-17267-queued-binary-result

📝 Commits (10+)

fb1451f feat(models): branch the queued result on Content-Type and return the binary arm
028161a fix(models): give the queued cap breach advice it can act on, and keep invalid_response's cause
beb0623 fix(models): drop a retryable queued response unread, and keep the cap advice honest
06168a5 docs(changelog): note the headerless non-JSON and cap outcome changes on queued results
a0a7470 feat(models): take a per-call maxBytes on the queued result read
115eb07 fix(models): read only a queued 200 under the cap, and hand subscribe's cap breach the id to retry with
39376bf fix(models): keep a small cap from cutting error envelopes, and drop a non-200 2xx body unread
b84db14 test(models): prove collect() applies the default cap, and assert the error class before reading it
4fc3e1f fix(models): keep the identifiers on a non-shape JSON parse failure, and report the cap actually applied
aab199a Merge origin/main into matt/be-17267-queued-binary-result

📊 Changes

8 files changed (+1333 additions, -91 deletions)

View changed files

📝 CHANGELOG.md (+42 -0)
📝 README.md (+2 -0)
📝 src/sdk/index.ts (+1 -0)
📝 src/sdk/modelRequests.test.ts (+494 -4)
📝 src/sdk/modelRequests.ts (+310 -43)
📝 src/sdk/models.test.ts (+241 -0)
📝 src/sdk/models.ts (+225 -41)
📝 test/support/router-stub-server.ts (+18 -3)

📄 Description

ELI-5

When you queue a model with submit and collect it later, the SDK used to assume the result was always JSON. The Router contract says a queued result can also be raw bytes, such as an audio file, and that clients should look at Content-Type to tell which one they got. models.run already does that. With this change RequestHandle.get() and models.subscribe() do it too: a non-JSON result comes back as { kind: "binary", data: Uint8Array, contentType }.

Honest scope: today's Router can't produce this. It refuses binary models at submit time, and its result handler only ever writes JSON. So this doesn't recover any bytes that are being lost now. It brings the SDK in line with the contract, and it matches the Python SDK change that makes RequestHandle._collect return dict | BinaryResult.

What changed

src/sdk/models.ts: readBodyWithin, isTooLarge, mediaTypeOf, isJsonMediaType, decodeUtf8 and UTF8_STRICT are now exported from the module. src/sdk/index.ts doesn't re-export them, so the public surface is unchanged. readBodyWithin / tooLarge now take a preformatted subject instead of the model id, and resolveMaxBytes takes the calling surface's name for its rejection message, so the cap-breach message names the queued request (the result of model request <id> response body …). idempotencyKey is widened to string | null because the result route doesn't send one. run's behaviour is unchanged: it passes models.run("<model>") as before.
src/sdk/modelRequests.ts:
- send() takes two new optional fields, accept and bytes. When bytes is set, the body is read with readBodyWithin inside the same try as the fetch, so the deadline/abort translation still covers reading the body. A response_too_large error is thrown straight away and not retried, just as in run. As in run, a retryable status is detected before the body is read, and that body is cancelled rather than downloaded on every attempt. Only a non-2xx body gets UTF-8 decoded, because the error envelope is the only thing that needs text. That decode happens after the retry try. If it fails (an error page too long to hold as a string), the typed error comes from the status line and X-Comfy-Error-Type alone, and nothing is retried (review round 10).
- decode() keeps the JSON parse error as the cause of its invalid_response, and only a SyntaxError is reported as "not JSON" (review round 9). invalidResponse sets cause only when one was given.
- A request_timeout raised while the result body is streaming carries details.queuedRequestId (review round 9). Every queue request_timeout also carries the call's idempotencyKey (review round 10).
- collect() now:
  - sends Accept: application/json, */*;q=0.9 and reads the result within the caller's maxBytes (new on GetOptions, which is WaitOptions plus maxBytes and is taken by get() only, and on SubscribeOptions; same semantics as RunOptions.maxBytes, default DEFAULT_MAX_RESPONSE_BYTES, validated before any request and again inside collect()). Only a result 200 is refused past the cap. A 202 (or any other non-200 2xx) is diagnosed from its status line with the body dropped unread, and an error answer is truncated rather than refused, never below 1 MiB, so it still reports its own diagnosis. A cap breach carries the queued request's id as details.queuedRequestId, because subscribe() returns no handle on that path.
  - handles non-2xx responses and 202 as before, except that every invalid_response it raises after the result fetch now carries details.queuedRequestId.
  - rejects an empty 200 with invalid_response.
  - returns a declared non-JSON media type as the binary arm.
  - returns a body with no Content-Type as binary with contentType: "" if it isn't valid JSON. The decode in this case is strict UTF-8. Any throw falls through to binary: invalid UTF-8, a body too long to hold as a string, or a parse RangeError from nesting too deep for the parser. Nothing declared a document, and a re-collect would fail the same way, so the bytes are the only lossless answer. This is how the base already behaved for run (review round 10).
  - still rejects a declared-JSON body that doesn't parse with invalid_response. If the failure is not a SyntaxError, the message says whether decoding or parsing ran out of resources, and the error carries details.queuedRequestId with the original error as cause.
  - otherwise parses JSON and runs raiseForCompletion exactly as before.
  The comment that said the queued path "has no binary branch" is gone. The get() doc comment now describes the binary arm and the cap.
test/support/router-stub-server.ts: ScriptedResponse.body accepts a Uint8Array and sends it byte for byte, and a new per-response contentType option overrides the global one (null omits the header). RecordedRequest also records the request's accept header.
README (queue section) and CHANGELOG (Unreleased → Changed).

Judgment calls / behaviour changes for JSON results

The ticket also asks for "no behaviour change for any JSON result". Three edge cases do change, all on purpose, to match finish and the ticket's own test list:

Empty 200 body. It used to resolve to { kind: "json", data: {} }. It now rejects with invalid_response.
Result body over 64 MiB. The queued read had no cap before. It now rejects with response_too_large by default, as the ticket asks. get() / subscribe() take maxBytes (null disables the cap), and the request stays collectable after a breach.
No Content-Type and invalid UTF-8. This used to be decoded lossily and parsed. It is now decoded strictly and, if that fails, returned as binary.

For every ordinary JSON result with Content-Type: application/json, nothing changes: same parse, same raiseForCompletion call, same error codes, same retry and timeout budget.

Tests

There's a new describe("RequestHandle on a binary result") block in src/sdk/modelRequests.test.ts. It uses the MP3-head fixture from models.test.ts, including its invalid-UTF-8 bytes, and covers:

get() resolving to binary with the exact bytes and the request id
subscribe() resolving to binary
audio/mpeg; rate=44100 keeping the full contentType string
a body with no Content-Type that isn't JSON → binary with contentType: ""
a JSON body with no Content-Type → json
Accept sent as application/json on the status request and application/json, */*;q=0.9 on the result request
an empty 200 → invalid_response
a Content-Length over DEFAULT_MAX_RESPONSE_BYTES → response_too_large with details.maxBytes, the request id in the message, "raise maxBytes" advice, and no retry
a per-call maxBytes on get() and subscribe() is honoured, the breach carries details.queuedRequestId, a later get({ maxBytes: null }) collects the same request (reached from subscribe's error through that id), and an invalid maxBytes rejects before any request
a 202 from the result route with maxBytes: 4 → invalid_response "answered 202" carrying details.queuedRequestId, not response_too_large
a 422 whose error_type and detail[] are in the body only, under maxBytes: 4 → InvalidInput with its detail[] intact
collect() called directly rejects a NaN cap and applies the default cap when maxBytes is omitted
a declared-JSON body that doesn't parse → invalid_response with the SyntaxError as its cause
a retryable 503 on the result route whose declared body never arrives → the body is dropped unread and the retry returns the result
a RangeError parsing a declared-JSON result → invalid_response with details.queuedRequestId and the RangeError as cause (review round 7)
review round 10: a headerless result whose parse throws a RangeError → binary; a 403 not_enabled whose error body cannot be decoded → NotEnabled after exactly one request
review round 9: a headerless result whose decode throws a RangeError → binary; a status body whose parse throws a RangeError → no "not JSON" message; a deadline that expires while the result streams → request_timeout with details.queuedRequestId; and the 202 error has no own cause

In src/sdk/models.test.ts (review round 5), two tests cover run: a body-only 422 envelope under maxBytes: 64 keeps invalid_input and its detail[], and an oversized 202 under maxBytes: 16 reports unexpected_response rather than response_too_large. Review round 7 adds a third: a RangeError parsing a declared-JSON 200 → unexpected_response with requestId, idempotencyKey and the RangeError as cause. Review round 8 adds a gzip test (a body declared past the cap that decodes inside it resolves; one declared inside it that decodes past it is still refused), a fractional maxBytes rejection on run and get(), and assertions on the RangeError message on both paths. Review round 9 adds a headerless body whose decode throws a RangeError (→ binary) and a Content-Encoding that fetch does not decode (x-unknown, gzip, x-unknown, identity, identity, identity), which is still refused on Content-Length before the body is read. Review round 10 adds two more: a headerless 200 whose parse throws a RangeError (→ binary), and a declared-JSON 200 whose decode throws, which reports "decoding".

The existing test "rejects when the result body is not JSON at all" (application/json + <html>) is unchanged and still passes. Apart from that, the only edit to existing tests is the comment about the json discriminant.

Residual

Overlapping PRs #152, #157 and #164 have all merged, and main is merged into this branch. #152 (error-path idempotency stamping) wraps the models.run loop and stamps the queue request_timeout; the merge keeps both sides (see Provenance). From #157 (credits-used) and #164 (replayed): creditsUsed and replayed are on all three returns from collect() (both binary returns and json), each typed as the internal BuiltRunResult so a missing field fails tsc.
New public option: maxBytes on GetOptions (new exported type, WaitOptions plus maxBytes) / SubscribeOptions. It is not on WaitOptions itself, because events() fetches no result and would ignore it. I first left this out as a separate API decision. Review round 3 showed the cap also applies to JSON results, so without it a result over 64 MiB could never be collected. The option mirrors RunOptions.maxBytes exactly. The Python queued surface doesn't have an equivalent yet, and the parity test compares names, not options.
Buffer pre-sizing. readBodyWithin (shared with run) now pre-sizes from a declared Content-Length only up to 1 MiB, so a stalled peer can't commit a 64 MiB allocation up front. Past that it grows by doubling, in step with the bytes received, and a vetted declared length only clamps the last step to an exact fit.
Changes that also reach models.run (review round 5). readBodyWithin reads an error body to at least 1 MiB before truncating it, so a small maxBytes no longer cuts the envelope that a body-only error_type or detail[] is parsed from. run drops the body of a 2xx other than 200 unread, so an oversized 202 reports unexpected_response instead of response_too_large. For a declared-JSON body, a parse RangeError leaves finish as an unexpected_response ComfyError that keeps requestId and idempotencyKey. Its message says resources ran out while decoding or parsing; it does not say the body "is not JSON". A headerless body goes back to the bytes arm on any throw, as on main. The queued path works the same way. An allocation failure on a truncated error body reports the 1 MiB-floored limit it applied, and MIN_ERROR_BODY_BYTES is its own 1 MiB literal. A declared-JSON body is still decoded leniently on both paths, as Response.json() does. Review round 8 adds three more that reach run: a Content-Length sent with a Content-Encoding that fetch decodes is ignored (it counts encoded bytes, while the cap counts the decoded bytes fetch hands over, and the streamed count still enforces it). Since review round 9 that applies only when every listed coding is one fetch decodes (gzip/x-gzip/deflate/br/zstd); any other token makes fetch pass the encoded bytes through, so the header is still honoured; a fractional maxBytes is a call-site TypeError; and a RangeError while parsing no longer says the body "is not JSON", on either path.
Deferred: the queued status, submit and cancel reads are still unbounded response.text(). This came before this PR and is recorded as a follow-up.
Deferred (review round 6): if reading a non-2xx body fails partway on the last attempt, both models.run and the queued send() raise the raw reader error rather than the typed HTTP error. main behaves the same way, and this is recorded as a follow-up.
Deferred (review round 9): under maxBytes: null, a body too large for arrayBuffer() throws a bare RangeError that is retried instead of being terminal. main behaves the same way, and this is recorded as a follow-up.
Deferred (review round 10): a transport error under retry: false after a COMPLETED status, and status-poll failures, still do not carry queuedRequestId. This is recorded as a follow-up.
Deferred (review round 7): when the queued result fetch ends on a typed RouterError (for example a final 5xx), the error does not carry queuedRequestId, because RouterError has no details. main behaves the same way. Fixing it needs a cross-SDK surface decision, so it is recorded as a follow-up.
src/sdk/router-spec-contract.test.ts doesn't currently pin the queued 200's media types, so I didn't add an assertion there, per the ticket's "if it already pins" condition. A pin that reads getRouterModelRequestResult's 200 content keys from spec/router-openapi.yaml and checks for both application/json and */* would be a small follow-up.
Parity with the Python SDK change (open). I didn't re-sync parity/python-surface.json. The Python change hasn't merged, and the parity test compares method, class and error_type names, not return types, so it shouldn't flag this change.
Not exercised live. Router refuses binary models at submit, so the binary arm is tested only against the stub server, never against a real queue. The Router-side references the ticket cites (its queue handler and result classification) are in a private repo that I didn't read. I relied on the ticket's description of them.

Provenance

Authored by: agent-work loop
Verified:
- Head (merge of main at ccae1fe, then oxfmt 0.68 reformat of two test files): pnpm typecheck, pnpm lint, pnpm format:check, pnpm build and pnpm check:spec-drift pass.
- pnpm test: 818 passed, 5 skipped, 0 failed.
- The main merge (oxfmt 0.68 bump, Router spec sync) applied cleanly; under oxfmt 0.68 the merged tree failed format:check on modelRequests.test.ts / models.test.ts, so those two files carry formatter-only output.
- Earlier merge with #152 kept both sides: its stamping wrapper around the models.run loop keeps this PR's non-200 2xx body drop and labelled capped read, and the queue request_timeout carries both #152's idempotencyKey and this PR's details.
- node scripts/sync-python-surface.mjs and the public-repo hygiene check were not run locally on this head (CI runs both).
Deviations:
- Empty-200 and 64 MiB-cap rejections and the strict decode with no Content-Type change three JSON edge cases (listed above).
- Adds a public maxBytes option to the queued surface on a new GetOptions type.
- Also changes models.run: 1 MiB error-body floor, a non-200 2xx body dropped unread, Content-Length ignored only for a coding fetch decodes, a fractional maxBytes rejected, and a declared-JSON parse RangeError no longer reported as "not JSON".
- No router-spec-contract assertion for the queued 200's media types (not pinned today), and the parity snapshot is not re-synced (the Python change is still open).

🤖 Generated with Claude Code

Summary by CodeRabbit

New Features
- Queued result retrieval and subscriptions support binary responses, preserving response bytes and content type.
- get(), subscribe(), and collect() accept a per-call response-size limit, defaulting to 64 MiB; the cap can be disabled. Oversized results can be collected later with a higher limit.
Bug Fixes
- Empty successful result responses return an invalid_response error.
- Oversized results return response_too_large; retryable responses are retried without reading their bodies.
- Invalid JSON declared as JSON returns an invalid_response error, while undeclared non-JSON content is treated as binary.
- Oversized error responses are truncated while preserving their error details.
Documentation
- Clarified how queued results handle JSON and binary responses.

_{🔄 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/185 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/25/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `matt/be-17267-queued-binary-result` --- ### 📝 Commits (10+) - [`fb1451f`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/fb1451fa5a539f3ceb10e6897b1b6eaaa4895d6d) feat(models): branch the queued result on Content-Type and return the binary arm - [`028161a`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/028161ac8f394b30f009d0eaa6de0e4620a698eb) fix(models): give the queued cap breach advice it can act on, and keep invalid_response's cause - [`beb0623`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/beb0623268ac614b22b10c37612fe5b9372cf986) fix(models): drop a retryable queued response unread, and keep the cap advice honest - [`06168a5`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/06168a55e56116e438c1eb26b576d85a174ccffb) docs(changelog): note the headerless non-JSON and cap outcome changes on queued results - [`a0a7470`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/a0a7470e393a159fbc51048c62262e98bbcb5dc3) feat(models): take a per-call maxBytes on the queued result read - [`115eb07`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/115eb07f3a917d40df85e27f0bb8298ce8ef1e82) fix(models): read only a queued 200 under the cap, and hand subscribe's cap breach the id to retry with - [`39376bf`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/39376bf662219f00df5e1455bd757b05e3c64575) fix(models): keep a small cap from cutting error envelopes, and drop a non-200 2xx body unread - [`b84db14`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/b84db14aaad9777fd98da0cb79ef693104c6d770) test(models): prove collect() applies the default cap, and assert the error class before reading it - [`4fc3e1f`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/4fc3e1ff5028cdd7a8daceed498e7c7499841d83) fix(models): keep the identifiers on a non-shape JSON parse failure, and report the cap actually applied - [`aab199a`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/aab199a757d120fbacbbcd0888bd21d6058739f9) Merge origin/main into matt/be-17267-queued-binary-result ### 📊 Changes **8 files changed** (+1333 additions, -91 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+42 -0) 📝 `README.md` (+2 -0) 📝 `src/sdk/index.ts` (+1 -0) 📝 `src/sdk/modelRequests.test.ts` (+494 -4) 📝 `src/sdk/modelRequests.ts` (+310 -43) 📝 `src/sdk/models.test.ts` (+241 -0) 📝 `src/sdk/models.ts` (+225 -41) 📝 `test/support/router-stub-server.ts` (+18 -3) </details> ### 📄 Description ## ELI-5 When you queue a model with `submit` and collect it later, the SDK used to assume the result was always JSON. The Router contract says a queued result can also be raw bytes, such as an audio file, and that clients should look at `Content-Type` to tell which one they got. `models.run` already does that. With this change `RequestHandle.get()` and `models.subscribe()` do it too: a non-JSON result comes back as `{ kind: "binary", data: Uint8Array, contentType }`. **Honest scope:** today's Router can't produce this. It refuses binary models at submit time, and its result handler only ever writes JSON. So this doesn't recover any bytes that are being lost now. It brings the SDK in line with the contract, and it matches the Python SDK change that makes `RequestHandle._collect` return `dict | BinaryResult`. ## What changed - `src/sdk/models.ts`: `readBodyWithin`, `isTooLarge`, `mediaTypeOf`, `isJsonMediaType`, `decodeUtf8` and `UTF8_STRICT` are now exported from the module. `src/sdk/index.ts` doesn't re-export them, so the public surface is unchanged. `readBodyWithin` / `tooLarge` now take a preformatted `subject` instead of the model id, and `resolveMaxBytes` takes the calling surface's name for its rejection message, so the cap-breach message names the queued request (`the result of model request <id> response body …`). `idempotencyKey` is widened to `string | null` because the result route doesn't send one. `run`'s behaviour is unchanged: it passes `models.run("<model>")` as before. - `src/sdk/modelRequests.ts`: - `send()` takes two new optional fields, `accept` and `bytes`. When `bytes` is set, the body is read with `readBodyWithin` inside the same `try` as the fetch, so the deadline/abort translation still covers reading the body. A `response_too_large` error is thrown straight away and not retried, just as in `run`. As in `run`, a retryable status is detected before the body is read, and that body is cancelled rather than downloaded on every attempt. Only a non-2xx body gets UTF-8 decoded, because the error envelope is the only thing that needs text. That decode happens after the retry `try`. If it fails (an error page too long to hold as a string), the typed error comes from the status line and `X-Comfy-Error-Type` alone, and nothing is retried (review round 10). - `decode()` keeps the JSON parse error as the `cause` of its `invalid_response`, and only a `SyntaxError` is reported as "not JSON" (review round 9). `invalidResponse` sets `cause` only when one was given. - A `request_timeout` raised while the result body is streaming carries `details.queuedRequestId` (review round 9). Every queue `request_timeout` also carries the call's `idempotencyKey` (review round 10). - `collect()` now: - sends `Accept: application/json, */*;q=0.9` and reads the result within the caller's `maxBytes` (new on `GetOptions`, which is `WaitOptions` plus `maxBytes` and is taken by `get()` only, and on `SubscribeOptions`; same semantics as `RunOptions.maxBytes`, default `DEFAULT_MAX_RESPONSE_BYTES`, validated before any request and again inside `collect()`). Only a result `200` is refused past the cap. A `202` (or any other non-200 2xx) is diagnosed from its status line with the body dropped unread, and an error answer is truncated rather than refused, never below 1 MiB, so it still reports its own diagnosis. A cap breach carries the queued request's id as `details.queuedRequestId`, because `subscribe()` returns no handle on that path. - handles non-2xx responses and 202 as before, except that every `invalid_response` it raises after the result fetch now carries `details.queuedRequestId`. - rejects an empty 200 with `invalid_response`. - returns a declared non-JSON media type as the `binary` arm. - returns a body with no `Content-Type` as `binary` with `contentType: ""` if it isn't valid JSON. The decode in this case is strict UTF-8. Any throw falls through to `binary`: invalid UTF-8, a body too long to hold as a string, or a parse `RangeError` from nesting too deep for the parser. Nothing declared a document, and a re-collect would fail the same way, so the bytes are the only lossless answer. This is how the base already behaved for `run` (review round 10). - still rejects a declared-JSON body that doesn't parse with `invalid_response`. If the failure is not a `SyntaxError`, the message says whether decoding or parsing ran out of resources, and the error carries `details.queuedRequestId` with the original error as `cause`. - otherwise parses JSON and runs `raiseForCompletion` exactly as before. The comment that said the queued path "has no binary branch" is gone. The `get()` doc comment now describes the binary arm and the cap. - `test/support/router-stub-server.ts`: `ScriptedResponse.body` accepts a `Uint8Array` and sends it byte for byte, and a new per-response `contentType` option overrides the global one (`null` omits the header). `RecordedRequest` also records the request's `accept` header. - README (queue section) and CHANGELOG (Unreleased → Changed). ## Judgment calls / behaviour changes for JSON results The ticket also asks for "no behaviour change for any JSON result". Three edge cases do change, all on purpose, to match `finish` and the ticket's own test list: 1. **Empty 200 body.** It used to resolve to `{ kind: "json", data: {} }`. It now rejects with `invalid_response`. 2. **Result body over 64 MiB.** The queued read had no cap before. It now rejects with `response_too_large` by default, as the ticket asks. `get()` / `subscribe()` take `maxBytes` (`null` disables the cap), and the request stays collectable after a breach. 3. **No `Content-Type` and invalid UTF-8.** This used to be decoded lossily and parsed. It is now decoded strictly and, if that fails, returned as `binary`. For every ordinary JSON result with `Content-Type: application/json`, nothing changes: same parse, same `raiseForCompletion` call, same error codes, same retry and timeout budget. ## Tests There's a new `describe("RequestHandle on a binary result")` block in `src/sdk/modelRequests.test.ts`. It uses the MP3-head fixture from `models.test.ts`, including its invalid-UTF-8 bytes, and covers: - `get()` resolving to `binary` with the exact bytes and the request id - `subscribe()` resolving to `binary` - `audio/mpeg; rate=44100` keeping the full `contentType` string - a body with no `Content-Type` that isn't JSON → `binary` with `contentType: ""` - a JSON body with no `Content-Type` → `json` - `Accept` sent as `application/json` on the status request and `application/json, */*;q=0.9` on the result request - an empty 200 → `invalid_response` - a `Content-Length` over `DEFAULT_MAX_RESPONSE_BYTES` → `response_too_large` with `details.maxBytes`, the request id in the message, "raise maxBytes" advice, and no retry - a per-call `maxBytes` on `get()` and `subscribe()` is honoured, the breach carries `details.queuedRequestId`, a later `get({ maxBytes: null })` collects the same request (reached from `subscribe`'s error through that id), and an invalid `maxBytes` rejects before any request - a `202` from the result route with `maxBytes: 4` → `invalid_response` "answered 202" carrying `details.queuedRequestId`, not `response_too_large` - a `422` whose `error_type` and `detail[]` are in the body only, under `maxBytes: 4` → `InvalidInput` with its `detail[]` intact - `collect()` called directly rejects a `NaN` cap and applies the default cap when `maxBytes` is omitted - a declared-JSON body that doesn't parse → `invalid_response` with the `SyntaxError` as its `cause` - a retryable 503 on the result route whose declared body never arrives → the body is dropped unread and the retry returns the result - a `RangeError` parsing a declared-JSON result → `invalid_response` with `details.queuedRequestId` and the `RangeError` as `cause` (review round 7) - review round 10: a headerless result whose parse throws a `RangeError` → `binary`; a 403 `not_enabled` whose error body cannot be decoded → `NotEnabled` after exactly one request - review round 9: a headerless result whose decode throws a `RangeError` → `binary`; a status body whose parse throws a `RangeError` → no "not JSON" message; a deadline that expires while the result streams → `request_timeout` with `details.queuedRequestId`; and the `202` error has no own `cause` In `src/sdk/models.test.ts` (review round 5), two tests cover `run`: a body-only `422` envelope under `maxBytes: 64` keeps `invalid_input` and its `detail[]`, and an oversized `202` under `maxBytes: 16` reports `unexpected_response` rather than `response_too_large`. Review round 7 adds a third: a `RangeError` parsing a declared-JSON 200 → `unexpected_response` with `requestId`, `idempotencyKey` and the `RangeError` as `cause`. Review round 8 adds a gzip test (a body declared past the cap that decodes inside it resolves; one declared inside it that decodes past it is still refused), a fractional `maxBytes` rejection on `run` and `get()`, and assertions on the `RangeError` message on both paths. Review round 9 adds a headerless body whose decode throws a `RangeError` (→ `binary`) and a `Content-Encoding` that `fetch` does not decode (`x-unknown`, `gzip, x-unknown`, `identity`, `identity, identity`), which is still refused on `Content-Length` before the body is read. Review round 10 adds two more: a headerless 200 whose parse throws a `RangeError` (→ `binary`), and a declared-JSON 200 whose decode throws, which reports "decoding". The existing test "rejects when the result body is not JSON at all" (`application/json` + `<html>`) is unchanged and still passes. Apart from that, the only edit to existing tests is the comment about the `json` discriminant. ## Residual - **Overlapping PRs #152, #157 and #164 have all merged, and `main` is merged into this branch.** #152 (error-path idempotency stamping) wraps the `models.run` loop and stamps the queue `request_timeout`; the merge keeps both sides (see Provenance). From #157 (credits-used) and #164 (`replayed`): `creditsUsed` and `replayed` are on all three returns from `collect()` (both `binary` returns and `json`), each typed as the internal `BuiltRunResult` so a missing field fails `tsc`. - **New public option: `maxBytes` on `GetOptions` (new exported type, `WaitOptions` plus `maxBytes`) / `SubscribeOptions`.** It is not on `WaitOptions` itself, because `events()` fetches no result and would ignore it. I first left this out as a separate API decision. Review round 3 showed the cap also applies to JSON results, so without it a result over 64 MiB could never be collected. The option mirrors `RunOptions.maxBytes` exactly. The Python queued surface doesn't have an equivalent yet, and the parity test compares names, not options. - **Buffer pre-sizing.** `readBodyWithin` (shared with `run`) now pre-sizes from a declared `Content-Length` only up to 1 MiB, so a stalled peer can't commit a 64 MiB allocation up front. Past that it grows by doubling, in step with the bytes received, and a vetted declared length only clamps the last step to an exact fit. - **Changes that also reach `models.run` (review round 5).** `readBodyWithin` reads an error body to at least 1 MiB before truncating it, so a small `maxBytes` no longer cuts the envelope that a body-only `error_type` or `detail[]` is parsed from. `run` drops the body of a 2xx other than 200 unread, so an oversized `202` reports `unexpected_response` instead of `response_too_large`. For a declared-JSON body, a parse `RangeError` leaves `finish` as an `unexpected_response` `ComfyError` that keeps `requestId` and `idempotencyKey`. Its message says resources ran out while decoding or parsing; it does not say the body "is not JSON". A headerless body goes back to the bytes arm on any throw, as on `main`. The queued path works the same way. An allocation failure on a truncated error body reports the 1 MiB-floored limit it applied, and `MIN_ERROR_BODY_BYTES` is its own 1 MiB literal. A declared-JSON body is still decoded leniently on both paths, as `Response.json()` does. Review round 8 adds three more that reach `run`: a `Content-Length` sent with a `Content-Encoding` that `fetch` decodes is ignored (it counts encoded bytes, while the cap counts the decoded bytes `fetch` hands over, and the streamed count still enforces it). Since review round 9 that applies only when every listed coding is one `fetch` decodes (`gzip`/`x-gzip`/`deflate`/`br`/`zstd`); any other token makes `fetch` pass the encoded bytes through, so the header is still honoured; a fractional `maxBytes` is a call-site `TypeError`; and a `RangeError` while parsing no longer says the body "is not JSON", on either path. - **Deferred:** the queued status, submit and cancel reads are still unbounded `response.text()`. This came before this PR and is recorded as a follow-up. - **Deferred (review round 6):** if reading a non-2xx body fails partway on the last attempt, both `models.run` and the queued `send()` raise the raw reader error rather than the typed HTTP error. `main` behaves the same way, and this is recorded as a follow-up. - **Deferred (review round 9):** under `maxBytes: null`, a body too large for `arrayBuffer()` throws a bare `RangeError` that is retried instead of being terminal. `main` behaves the same way, and this is recorded as a follow-up. - **Deferred (review round 10):** a transport error under `retry: false` after a COMPLETED status, and status-poll failures, still do not carry `queuedRequestId`. This is recorded as a follow-up. - **Deferred (review round 7):** when the queued result fetch ends on a typed `RouterError` (for example a final 5xx), the error does not carry `queuedRequestId`, because `RouterError` has no `details`. `main` behaves the same way. Fixing it needs a cross-SDK surface decision, so it is recorded as a follow-up. - **`src/sdk/router-spec-contract.test.ts`** doesn't currently pin the queued 200's media types, so I didn't add an assertion there, per the ticket's "if it already pins" condition. A pin that reads `getRouterModelRequestResult`'s 200 `content` keys from `spec/router-openapi.yaml` and checks for both `application/json` and `*/*` would be a small follow-up. - **Parity with the Python SDK change (open).** I didn't re-sync `parity/python-surface.json`. The Python change hasn't merged, and the parity test compares method, class and `error_type` names, not return types, so it shouldn't flag this change. - **Not exercised live.** Router refuses binary models at submit, so the binary arm is tested only against the stub server, never against a real queue. The Router-side references the ticket cites (its queue handler and result classification) are in a private repo that I didn't read. I relied on the ticket's description of them. ## Provenance - **Authored by:** agent-work loop - **Verified:** - Head (merge of `main` at ccae1fe, then oxfmt 0.68 reformat of two test files): `pnpm typecheck`, `pnpm lint`, `pnpm format:check`, `pnpm build` and `pnpm check:spec-drift` pass. - `pnpm test`: 818 passed, 5 skipped, 0 failed. - The `main` merge (oxfmt 0.68 bump, Router spec sync) applied cleanly; under oxfmt 0.68 the merged tree failed `format:check` on `modelRequests.test.ts` / `models.test.ts`, so those two files carry formatter-only output. - Earlier merge with #152 kept both sides: its `stamping` wrapper around the `models.run` loop keeps this PR's non-200 2xx body drop and labelled capped read, and the queue `request_timeout` carries both #152's `idempotencyKey` and this PR's `details`. - `node scripts/sync-python-surface.mjs` and the public-repo hygiene check were not run locally on this head (CI runs both). - **Deviations:** - Empty-200 and 64 MiB-cap rejections and the strict decode with no `Content-Type` change three JSON edge cases (listed above). - Adds a public `maxBytes` option to the queued surface on a new `GetOptions` type. - Also changes `models.run`: 1 MiB error-body floor, a non-200 2xx body dropped unread, `Content-Length` ignored only for a coding `fetch` decodes, a fractional `maxBytes` rejected, and a declared-JSON parse `RangeError` no longer reported as "not JSON". - No router-spec-contract assertion for the queued 200's media types (not pinned today), and the parity snapshot is not re-synced (the Python change is still open). 🤖 Generated with [Claude Code](https://claude.com/claude-code)  ## Summary by CodeRabbit * **New Features** * Queued result retrieval and subscriptions support binary responses, preserving response bytes and content type. * `get()`, `subscribe()`, and `collect()` accept a per-call response-size limit, defaulting to 64 MiB; the cap can be disabled. Oversized results can be collected later with a higher limit. * **Bug Fixes** * Empty successful result responses return an `invalid_response` error. * Oversized results return `response_too_large`; retryable responses are retried without reading their bodies. * Invalid JSON declared as JSON returns an `invalid_response` error, while undeclared non-JSON content is treated as binary. * Oversized error responses are truncated while preserving their error details. * **Documentation** * Clarified how queued results handle JSON and binary responses.  --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi added the

pull-request

label

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