Comfy-Org/comfy-typescript-sdk

Fork 0

mirror of https://github.com/Comfy-Org/comfy-typescript-sdk.git synced 2026-11-04 23:47:27 +00:00

[GH-ISSUE #62] Error translation, SSE error handling, and 429 retry: three verified defects plus the test gaps that let them through (cross-SDK parity audit) #7

New issue

Closed

opened 2026-10-31 22:03:37 +00:00 by bsakaguchi · 1 comment

bsakaguchi commented

2026-10-31 22:03:37 +00:00

Owner

Originally created by @christian-byrne on GitHub (Aug 21, 2026).
Original GitHub issue: https://github.com/Comfy-Org/comfy-typescript-sdk/issues/62

Originally assigned to: @wei-hai on GitHub.

Cross-SDK parity audit against comfy-python-sdk (whose equivalent audit is Comfy-Org/comfy-python-sdk#62). Two of the Python defects have a sibling here, one does not, and the audit turned up a third defect that is TS-specific in impact. Everything below is verified at runtime against this repo's own test/support/stub-server.ts, each with a positive control on a code path that behaves correctly.

Ref audited: 54c0b40 (origin/main).

D1 — `outputs.ts` never calls `translate()`, so all three public download methods leak the raw `low.ApiError`

src/sdk/outputs.ts is the only file under src/sdk/ that touches low without importing translate from ./exceptions.js. assets.ts and jobs.ts both wrap (including both factory get()s — the spot the Python SDK missed). Outputs does not:

$ git grep -n "translate" -- src/sdk/ | grep -v test
src/sdk/assets.ts:19,167,198,257,270
src/sdk/jobs.ts:22,84,124,141,213
src/sdk/exceptions.ts:126
# src/sdk/outputs.ts — no hits

ComfyLow.getAssetContent and getAssetContentUrl both raise via errorFromEnvelope (transport.ts:262 parseOrRaise, reached from getAssetContent and from getAssetContentUrl:416), so those errors reach the caller untranslated.

Repro — the stub already 404s GET /assets/{id}/content for a deleted asset (test/support/stub-server.ts:299-303), so no new fixture is needed:

--- CONTROLS (paths that DO call translate) ---
client.assets.get(deleted)      name=NotFound  sdk.NotFound=true   ComfyError=true   low.ApiError=false
client.assets.delete(deleted)   name=NotFound  sdk.NotFound=true   ComfyError=true   low.ApiError=false
--- SUSPECTS ---
output.toFile(deleted)          name=NotFound  sdk.NotFound=false  ComfyError=false  low.ApiError=true
output.toBytes(deleted)         name=NotFound  sdk.NotFound=false  ComfyError=false  low.ApiError=true
output.getDownloadUrl(deleted)  name=NotFound  sdk.NotFound=false  ComfyError=false  low.ApiError=true

Why this is worse than it looks:

src/low/errors.ts and src/sdk/exceptions.ts both export a class named NotFound, and they are unrelated. err.name and err.constructor.name are "NotFound" in both rows above, so a consumer debugging this sees the class name they expected and concludes their own instanceof is broken.
err instanceof ComfyError is false, which contradicts the README's "All extend a shared ComfyError (code, httpStatus, details)" — the documented escape hatch for catching everything doesn't catch the download path either.
Output.getDownloadUrl's JSDoc says "Works on every backend and never throws." It does throw. The corresponding low JSDoc on ComfyLow.getAssetContentUrl was already corrected to "...never throws for either shape — only a genuine failure maps to the usual typed error", and transport.test.ts has a test literally named "getAssetContentUrl never throws for a real 404 — no, it maps to the usual typed error". The low layer knows; the sdk JSDoc above it was not updated.

Downloading an output is the most-used consumer call after run().

Fix: import translate in outputs.ts and wrap the three this.low.* calls; correct the getDownloadUrl JSDoc.

D2 — `Job.events()` swallows every `ApiError`, turning a contract-legal 501 into a 100 ms reconnect loop

src/sdk/jobs.ts:174-179:

} catch (exc) {
  if (signal?.aborted) throw exc;
  // Connection dropped mid-stream — reconnect below.
}

That catch is meant for transport drops, but it also catches every ApiError getJobEvents raises. spec/openapi.yaml:587 documents 501 not_implemented on GET /api/v2/jobs/{id}/events — "this deployment does not yet serve live event streaming. GET /api/v2/jobs/{id} remains available as a plain poll in the meantime" — and 429 too_many_streams with a Retry-After for the concurrent-stream limit. There is no reference to 501 anywhere in src/ or test/. The Python SDK handles it explicitly in both its sync and async events() (comfy_sdk/jobs.py:166, :278): if exc.http_status == 501: return.

Repro, on a job that takes just 2 seconds:

501 no-SSE surface     completed in 2007ms — sse attempts=21, polls=21, events yielded=["statusChange"]
429 too_many_streams   completed in 2004ms — sse attempts=21, polls=21, events yielded=["statusChange"]

10 SSE connection attempts + 10 polls per second, sustained, for the whole life of the job. A realistic 5-minute generation is ~3000 SSE attempts and ~3000 polls where one attempt would do. On the 429 path this is self-inflicted: the server is publishing a Retry-After to shed the exact load the client then reconnects into at a fixed 100 ms.

Note the outcome is semantically fine — events() does yield a correct terminal statusChange off the poll backstop, which is arguably better than Python's bare return. This is purely a request-amplification bug, and it targets the self-hosted comfy-api-proxy surface the README advertises, which is precisely the deployment most likely to answer 501.

Fix: in the catch, re-check exc instanceof ApiError; return on 501, and on a 429 honour retryAfter instead of RECONNECT_PAUSE_MS. Everything else keeps the existing reconnect behaviour.

D3 — `submit()` retries only `queue_full`, but the spec requires backing off on any 429 + `Retry-After`

spec/openapi.yaml:994-1018 (ErrorEnvelope) states:

Deployment-scoped surfaces add: deployment_not_ready (429 + Retry-After — the deployment can still reach ready; retry) [...] A 429 is disambiguated by error.code alone; clients should treat any 429 + Retry-After as "back off and retry".

client.ts:194 gates the retry on err instanceof QueueFull, i.e. on code === "queue_full" alone. deployment_not_ready is not in low/errors.ts's BY_CODE, so it falls through to a bare ApiError → bare ComfyError, and the retryAfter the transport already parsed is discarded.

Repro (real Comfy client, injected fetch, 429 + Retry-After: 0 four times then success):

429 queue_full             RETRIED     -> submitted after 5 attempts        <- CONTROL
429 deployment_not_ready   NOT RETRIED -> threw ComfyError after 1 attempt; QueueFull=false code=deployment_not_ready
429 too_many_streams       NOT RETRIED -> threw ComfyError after 1 attempt
429 rate_limited           NOT RETRIED -> threw ComfyError after 1 attempt

A cold serverless deployment answering 429 deployment_not_ready fails the submit outright instead of waiting out the cold start — on the serverless surface the README explicitly targets.

comfy-python-sdk has the identical gap (comfy_sdk/client.py:170, :253), so this is a shared contract gap rather than a TS regression; worth fixing in lockstep.

Fix: retry on httpStatus === 429 && retryAfter != null rather than on the QueueFull class, or add the deployment-scoped codes to BY_CODE and widen the predicate.

Test gaps

These are what let D1–D3 through. Assertion density was counted per file rather than per test block; no file in the suite asserts nothing, and the suite is strong overall (see "clean" below).

T1 — no error-path test on any Output method. outputs.test.ts has 7 tests / 19 assertions, all happy-path. The stub already has the 404 branch, so this is two lines and it fails today:
```
server.state.deletedAssets.add("asset_out_01");
await expect(output().toBytes()).rejects.toBeInstanceOf(NotFound); // sdk NotFound
```
T2 — no test where GET /jobs/{id}/events returns a non-200. StubServer.state.sseMode is "normal" | "reconnect" — there is no way to make the events endpoint answer 501/429/401 at all. The reconnect tests are genuinely good (monotonic progress, no-replay, poll backstop, abort-during-pause); the whole error dimension is simply unreachable from the fixture. Add an eventsStatus knob.
T3 — no test asserts a blake3 digest is correct. src/sdk/hashing.ts has no direct test, and assets.test.ts:36-37 seeds the stub from the SDK's own output (server.state.knownHashes.add(await asset.hash())). There is not one 64-hex digest literal anywhere in the suite (grep -roE '[0-9a-f]{64}' --include='*.test.ts' → 0; control: the same regex at 40 chars finds all 8 pinned action SHAs in ci.yml). A hashBytes that returned a wrong-but-consistent value — a hash-wasm API change, a wrong prefix, hex vs base64 — passes every test in this repo while making the server reject expected_hash on every upload and never hitting the dedup fast path. Also nothing asserts hashFile(p) === hashBytes(readFileSync(p)), so the streaming path could disagree with the buffered one undetected. Add one known blake3 vector.
T4 — no completeness guard between the two BY_CODE tables. low/errors.ts and sdk/exceptions.ts each hand-maintain a code→class map, and each test file hand-repeats the list. The tables are currently consistent (low \ sdk = {queue_full}, handled specially in toSdkError; sdk \ low = {}), so this is a guard gap, not a live bug — but adding a code to low alone silently degrades it to a bare ComfyError, which is exactly how D3 reads today. spec-coverage.test.ts already does this beautifully for operationIds; copy the pattern.
T5 — coverage is measured but not enforced. vitest.config.ts has a well-considered coverage block (notably include: ["src/**/*.ts"] so an untested module shows as 0% instead of vanishing) but no thresholds, and CI runs pnpm test, not pnpm test:coverage, so nothing reads the numbers.

Zero-direct-coverage modules judged not worth filing (adequately exercised end-to-end and pure): core.ts's newIdempotencyKey / looksLikeUiFormat, Job.refresh (covered via wait), ComfyLow.request (covered via every typed method).

Checked and clean — please don't re-audit these

Codegen drift is guarded and enforced. scripts/check-spec-drift.mjs regenerates into a tempdir and compares the three generated files by exact content, catching both a spec edit without a regen and a hand-edit of generated output. It runs as a step in the test job, and Test (node 22) / Test (node 24) are required status checks on main — verified at the API, on classic branch protection, not inferred from the workflow. The repository's only ruleset is "CLA Check" (cla-assistant only), so checking rulesets alone would have produced a false "unguarded" finding. @hey-api/openapi-ts is pinned exactly (0.99.0), matching the Python SDK's rationale that a byte-exact comparison needs a pinned generator. As in the Python SDK, the one unguarded link is upstream: spec/openapi.yaml is a one-way vendored copy with a hand-maintained spec/VERSION, so canonical-vs-vendored drift is not detectable from inside this repo.
The Python SDK's sync/async asymmetry does not exist here, structurally. Diffing public member sets across all 8 TS↔Python class pairs: Output is identical to Python's AsyncOutput. Python's missing-to_stream defect is a sync-only method that AsyncOutput never had; because this SDK collapses to one async surface there is nothing to drift out of parity with. AssetFactory/JobFactory identical; Comfy's only difference is Python's aclose, which is N/A for a fetch-based client with no pooled client to close; fromString vs Python's from_str is a deliberate idiomatic rename. The only real surface nit: Asset.adopt() is public and ships in the emitted .d.ts even though its own JSDoc says "Not part of the public surface" — Python's equivalent has no public counterpart at all. Worth a private/@internal, but not a defect.
Auth. Asserts the header actually sent, not just the outcome: no key → no Authorization header at all; key → Bearer <key> observed server-side. Cross-origin non-leak is covered for server-supplied urls.self/events/cancel and for a content-download 302, plus CRLF header injection via clientInfo and a SDK_VERSION-vs-package.json drift guard. Stronger than most SDKs.
Retry. queue_full → Retry-After → retry → success is covered end to end, as is Retry-After in HTTP-date form falling back to null rather than NaN. (The gap is which codes trigger it — D3 — not the mechanism.)
Pagination — N/A, not a gap. 11 operations, no list endpoint; the spec declares three parameters total (assets, jobs tags and Range) and no page/cursor/limit/offset anywhere.
SSE decoding. Multi-line data:, comment/keepalive, name-without-data, mid-UTF-8 chunk splits, non-object and non-JSON payloads, default message type, and a stalled-"zombie"-stream timeout.
Idempotency. Reject-on-duplicate asserted at both layers; fresh-key-per-submit asserted; extra_data present/absent/empty-object all asserted on the wire.
No mock-only assertions. The suite drives a real node:http stub server. No vi.mock, no spies on the code under test.

/cc @wei-hai — git blame puts outputs.ts's three download methods, the events() catch, and the QueueFull retry predicate all on you (dca70f88, 01ac0c79, #16), and you're the one who added translate() correctly to assets.ts and jobs.ts, so the pattern is clearly known and outputs was just missed.

Originally created by @christian-byrne on GitHub (Aug 21, 2026). Original GitHub issue: https://github.com/Comfy-Org/comfy-typescript-sdk/issues/62 Originally assigned to: @wei-hai on GitHub. Cross-SDK parity audit against `comfy-python-sdk` (whose equivalent audit is Comfy-Org/comfy-python-sdk#62). Two of the Python defects have a sibling here, one does not, and the audit turned up a third defect that is TS-specific in impact. Everything below is verified at runtime against this repo's own `test/support/stub-server.ts`, each with a positive control on a code path that behaves correctly. Ref audited: `54c0b40` (origin/main). --- ## D1 — `outputs.ts` never calls `translate()`, so all three public download methods leak the raw `low.ApiError` `src/sdk/outputs.ts` is the only file under `src/sdk/` that touches `low` without importing `translate` from `./exceptions.js`. `assets.ts` and `jobs.ts` both wrap (including both factory `get()`s — the spot the Python SDK missed). Outputs does not: ``` $ git grep -n "translate" -- src/sdk/ | grep -v test src/sdk/assets.ts:19,167,198,257,270 src/sdk/jobs.ts:22,84,124,141,213 src/sdk/exceptions.ts:126 # src/sdk/outputs.ts — no hits ``` `ComfyLow.getAssetContent` and `getAssetContentUrl` both raise via `errorFromEnvelope` (`transport.ts:262` `parseOrRaise`, reached from `getAssetContent` and from `getAssetContentUrl:416`), so those errors reach the caller untranslated. **Repro** — the stub already 404s `GET /assets/{id}/content` for a deleted asset (`test/support/stub-server.ts:299-303`), so no new fixture is needed: ``` --- CONTROLS (paths that DO call translate) --- client.assets.get(deleted) name=NotFound sdk.NotFound=true ComfyError=true low.ApiError=false client.assets.delete(deleted) name=NotFound sdk.NotFound=true ComfyError=true low.ApiError=false --- SUSPECTS --- output.toFile(deleted) name=NotFound sdk.NotFound=false ComfyError=false low.ApiError=true output.toBytes(deleted) name=NotFound sdk.NotFound=false ComfyError=false low.ApiError=true output.getDownloadUrl(deleted) name=NotFound sdk.NotFound=false ComfyError=false low.ApiError=true ``` Why this is worse than it looks: 1. `src/low/errors.ts` and `src/sdk/exceptions.ts` **both export a class named `NotFound`**, and they are unrelated. `err.name` and `err.constructor.name` are `"NotFound"` in both rows above, so a consumer debugging this sees the class name they expected and concludes their own `instanceof` is broken. 2. `err instanceof ComfyError` is `false`, which contradicts the README's "All extend a shared `ComfyError` (`code`, `httpStatus`, `details`)" — the documented escape hatch for catching everything doesn't catch the download path either. 3. `Output.getDownloadUrl`'s JSDoc says "**Works on every backend and never throws.**" It does throw. The corresponding `low` JSDoc on `ComfyLow.getAssetContentUrl` was already corrected to "...never throws for either shape — only a genuine failure maps to the usual typed error", and `transport.test.ts` has a test literally named *"getAssetContentUrl never throws for a real 404 — no, it maps to the usual typed error"*. The `low` layer knows; the `sdk` JSDoc above it was not updated. Downloading an output is the most-used consumer call after `run()`. **Fix:** import `translate` in `outputs.ts` and wrap the three `this.low.*` calls; correct the `getDownloadUrl` JSDoc. ## D2 — `Job.events()` swallows every `ApiError`, turning a contract-legal 501 into a 100 ms reconnect loop `src/sdk/jobs.ts:174-179`: ```ts } catch (exc) { if (signal?.aborted) throw exc; // Connection dropped mid-stream — reconnect below. } ``` That catch is meant for transport drops, but it also catches every `ApiError` `getJobEvents` raises. `spec/openapi.yaml:587` documents `501 not_implemented` on `GET /api/v2/jobs/{id}/events` — *"this deployment does not yet serve live event streaming. GET /api/v2/jobs/{id} remains available as a plain poll in the meantime"* — and `429 too_many_streams` with a `Retry-After` for the concurrent-stream limit. **There is no reference to `501` anywhere in `src/` or `test/`.** The Python SDK handles it explicitly in both its sync and async `events()` (`comfy_sdk/jobs.py:166`, `:278`): `if exc.http_status == 501: return`. **Repro**, on a job that takes just 2 seconds: ``` 501 no-SSE surface completed in 2007ms — sse attempts=21, polls=21, events yielded=["statusChange"] 429 too_many_streams completed in 2004ms — sse attempts=21, polls=21, events yielded=["statusChange"] ``` 10 SSE connection attempts + 10 polls per second, sustained, for the whole life of the job. A realistic 5-minute generation is ~3000 SSE attempts and ~3000 polls where one attempt would do. On the 429 path this is self-inflicted: the server is publishing a `Retry-After` to shed the exact load the client then reconnects into at a fixed 100 ms. Note the outcome is *semantically* fine — `events()` does yield a correct terminal `statusChange` off the poll backstop, which is arguably better than Python's bare `return`. This is purely a request-amplification bug, and it targets the self-hosted `comfy-api-proxy` surface the README advertises, which is precisely the deployment most likely to answer 501. **Fix:** in the catch, re-check `exc instanceof ApiError`; `return` on 501, and on a 429 honour `retryAfter` instead of `RECONNECT_PAUSE_MS`. Everything else keeps the existing reconnect behaviour. ## D3 — `submit()` retries only `queue_full`, but the spec requires backing off on *any* 429 + `Retry-After` `spec/openapi.yaml:994-1018` (`ErrorEnvelope`) states: > Deployment-scoped surfaces add: `deployment_not_ready` (429 + Retry-After — the deployment can still reach ready; retry) [...] A 429 is disambiguated by `error.code` alone; **clients should treat any 429 + Retry-After as "back off and retry"**. `client.ts:194` gates the retry on `err instanceof QueueFull`, i.e. on `code === "queue_full"` alone. `deployment_not_ready` is not in `low/errors.ts`'s `BY_CODE`, so it falls through to a bare `ApiError` → bare `ComfyError`, and the `retryAfter` the transport already parsed is discarded. **Repro** (real `Comfy` client, injected `fetch`, 429 + `Retry-After: 0` four times then success): ``` 429 queue_full RETRIED -> submitted after 5 attempts <- CONTROL 429 deployment_not_ready NOT RETRIED -> threw ComfyError after 1 attempt; QueueFull=false code=deployment_not_ready 429 too_many_streams NOT RETRIED -> threw ComfyError after 1 attempt 429 rate_limited NOT RETRIED -> threw ComfyError after 1 attempt ``` A cold serverless deployment answering `429 deployment_not_ready` fails the submit outright instead of waiting out the cold start — on the serverless surface the README explicitly targets. `comfy-python-sdk` has the identical gap (`comfy_sdk/client.py:170`, `:253`), so this is a shared contract gap rather than a TS regression; worth fixing in lockstep. **Fix:** retry on `httpStatus === 429 && retryAfter != null` rather than on the `QueueFull` class, or add the deployment-scoped codes to `BY_CODE` and widen the predicate. --- ## Test gaps These are what let D1–D3 through. Assertion density was counted per file rather than per test block; no file in the suite asserts nothing, and the suite is strong overall (see "clean" below). - **T1 — no error-path test on any `Output` method.** `outputs.test.ts` has 7 tests / 19 assertions, all happy-path. The stub already has the 404 branch, so this is two lines and it fails today: ```ts server.state.deletedAssets.add("asset_out_01"); await expect(output().toBytes()).rejects.toBeInstanceOf(NotFound); // sdk NotFound ``` - **T2 — no test where `GET /jobs/{id}/events` returns a non-200.** `StubServer.state.sseMode` is `"normal" | "reconnect"` — there is no way to make the events endpoint answer 501/429/401 at all. The reconnect tests are genuinely good (monotonic progress, no-replay, poll backstop, abort-during-pause); the whole error dimension is simply unreachable from the fixture. Add an `eventsStatus` knob. - **T3 — no test asserts a blake3 digest is correct.** `src/sdk/hashing.ts` has no direct test, and `assets.test.ts:36-37` seeds the stub from the SDK's own output (`server.state.knownHashes.add(await asset.hash())`). There is not one 64-hex digest literal anywhere in the suite (`grep -roE '[0-9a-f]{64}' --include='*.test.ts'` → 0; control: the same regex at 40 chars finds all 8 pinned action SHAs in `ci.yml`). A `hashBytes` that returned a wrong-but-consistent value — a `hash-wasm` API change, a wrong prefix, hex vs base64 — passes every test in this repo while making the server reject `expected_hash` on every upload and never hitting the dedup fast path. Also nothing asserts `hashFile(p) === hashBytes(readFileSync(p))`, so the streaming path could disagree with the buffered one undetected. Add one known blake3 vector. - **T4 — no completeness guard between the two `BY_CODE` tables.** `low/errors.ts` and `sdk/exceptions.ts` each hand-maintain a code→class map, and each test file hand-repeats the list. The tables are currently consistent (`low \ sdk = {queue_full}`, handled specially in `toSdkError`; `sdk \ low = {}`), so this is a guard gap, not a live bug — but adding a code to `low` alone silently degrades it to a bare `ComfyError`, which is exactly how D3 reads today. `spec-coverage.test.ts` already does this beautifully for operationIds; copy the pattern. - **T5 — coverage is measured but not enforced.** `vitest.config.ts` has a well-considered `coverage` block (notably `include: ["src/**/*.ts"]` so an untested module shows as 0% instead of vanishing) but no `thresholds`, and CI runs `pnpm test`, not `pnpm test:coverage`, so nothing reads the numbers. Zero-direct-coverage modules judged **not** worth filing (adequately exercised end-to-end and pure): `core.ts`'s `newIdempotencyKey` / `looksLikeUiFormat`, `Job.refresh` (covered via `wait`), `ComfyLow.request` (covered via every typed method). ## Checked and clean — please don't re-audit these - **Codegen drift is guarded *and* enforced.** `scripts/check-spec-drift.mjs` regenerates into a tempdir and compares the three generated files by exact content, catching both a spec edit without a regen and a hand-edit of generated output. It runs as a step in the `test` job, and `Test (node 22)` / `Test (node 24)` are **required status checks** on `main` — verified at the API, on *classic branch protection*, not inferred from the workflow. The repository's only ruleset is "CLA Check" (`cla-assistant` only), so checking rulesets alone would have produced a false "unguarded" finding. `@hey-api/openapi-ts` is pinned exactly (`0.99.0`), matching the Python SDK's rationale that a byte-exact comparison needs a pinned generator. As in the Python SDK, the one unguarded link is upstream: `spec/openapi.yaml` is a one-way vendored copy with a hand-maintained `spec/VERSION`, so canonical-vs-vendored drift is not detectable from inside this repo. - **The Python SDK's sync/async asymmetry does not exist here, structurally.** Diffing public member sets across all 8 TS↔Python class pairs: `Output` is **identical** to Python's `AsyncOutput`. Python's missing-`to_stream` defect is a *sync-only* method that `AsyncOutput` never had; because this SDK collapses to one async surface there is nothing to drift out of parity with. `AssetFactory`/`JobFactory` identical; `Comfy`'s only difference is Python's `aclose`, which is N/A for a `fetch`-based client with no pooled client to close; `fromString` vs Python's `from_str` is a deliberate idiomatic rename. The only real surface nit: `Asset.adopt()` is `public` and ships in the emitted `.d.ts` even though its own JSDoc says "Not part of the public surface" — Python's equivalent has no public counterpart at all. Worth a `private`/`@internal`, but not a defect. - **Auth.** Asserts the header actually sent, not just the outcome: no key → no `Authorization` header at all; key → `Bearer <key>` observed server-side. Cross-origin non-leak is covered for server-supplied `urls.self`/`events`/`cancel` **and** for a content-download 302, plus CRLF header injection via `clientInfo` and a `SDK_VERSION`-vs-`package.json` drift guard. Stronger than most SDKs. - **Retry.** `queue_full` → `Retry-After` → retry → success is covered end to end, as is `Retry-After` in HTTP-date form falling back to `null` rather than `NaN`. (The gap is which *codes* trigger it — D3 — not the mechanism.) - **Pagination — N/A, not a gap.** 11 operations, no list endpoint; the spec declares three parameters total (`assets`, `jobs` tags and `Range`) and no `page`/`cursor`/`limit`/`offset` anywhere. - **SSE decoding.** Multi-line `data:`, comment/keepalive, name-without-data, mid-UTF-8 chunk splits, non-object and non-JSON payloads, default `message` type, and a stalled-"zombie"-stream timeout. - **Idempotency.** Reject-on-duplicate asserted at both layers; fresh-key-per-submit asserted; `extra_data` present/absent/empty-object all asserted on the wire. - **No mock-only assertions.** The suite drives a real `node:http` stub server. No `vi.mock`, no spies on the code under test. /cc @wei-hai — `git blame` puts `outputs.ts`'s three download methods, the `events()` catch, and the `QueueFull` retry predicate all on you (`dca70f88`, `01ac0c79`, `#16`), and you're the one who added `translate()` correctly to `assets.ts` and `jobs.ts`, so the pattern is clearly known and outputs was just missed.

bsakaguchi

2026-10-31 22:03:37 +00:00

closed this issue
added the
bug
label

bsakaguchi commented

2026-10-31 22:03:37 +00:00

Author

Owner

@christian-byrne commented on GitHub (Aug 21, 2026):

Follow-up sweep on the same ref (54c0b40), different lens: which merges never got a human read, and what came in through them. Two more verified defects, both in the same files this issue already covers, both the same shape as D1 — a guard that exists at one call site and not at its sibling. Adding them here rather than opening new issues.

Runtime-verified the same way as D1–D3: real source under node --experimental-strip-types, stubs only for the three modules with uninstalled deps (hash-wasm, zod, eventsource-parser), every claim with control arms that are known-non-zero.

D4 — `Output.toFile` / `toBytes` inherit a 30-second whole-transfer deadline, and no caller can raise or disable it

ComfyLow.getJobEvents deliberately disables the client's default timeout for a long-lived stream:

// src/low/transport.ts:488-491
const response = await this.request("GET", path, {
  headers: { Accept: "text/event-stream" },
  signal: options.signal,
  timeoutMs: options.timeoutMs ?? null,   // <-- the guard
});

with the JSDoc right above it at :477-479: "No default timeout: an idle stream must not time out mid-job."

The other long-lived streaming path in the same class has no such guard. getAssetContent (transport.ts:363-378) takes { range?, signal? } and no timeoutMs at all, so request() applies DEFAULT_TIMEOUT_MS = 30_000 (transport.ts:41) through resolveSignal (:230-239). AbortSignal.timeout(30_000) is a wall-clock deadline covering the entire request including body consumption — it is not an idle timeout.

The sdk layer forwards nothing. outputs.ts:76 toFile(path, { range? }) and :90 toBytes({ range? }) accept no signal and no timeoutMs. And the client-level knob cannot rescue it: ComfyOptions.timeoutMs is typed number (client.ts:94), ComfyLowOptions.timeoutMs likewise (transport.ts:61), and this.defaultTimeoutMs = options.timeoutMs ?? DEFAULT_TIMEOUT_MS. A TypeScript caller can raise the global deadline but never disable it, and cannot scope the change to downloads. The only route to timeoutMs: null is the untyped ComfyLow.request escape hatch.

Repro — real ComfyLow + real Output against a node:http server streaming 6 chunks 250 ms apart (~1.5 s total). Client timeoutMs: 400 stands to that 1.5 s stream as the shipped 30 s default stands to a >30 s download.

ARM A  CONTROL low.request(timeoutMs:null) slow body   OK after 1758ms  -> 48 bytes read
ARM B  CONTROL output.toFile() fast body               OK after 3ms
ARM C  SUSPECT output.toFile() slow body               FAILED after 402ms -> TimeoutError: The operation was aborted due to timeout
ARM D  SUSPECT output.toBytes() slow body              FAILED after 400ms -> TimeoutError: The operation was aborted due to timeout

Both controls are known-non-zero successes — A proves the slow body is readable once the timeout is off, B proves toFile works at all — so C and D are attributable to the deadline, not the harness.

Impact. Any output download exceeding 30 s wall-clock aborts mid-stream. Because it is outputs.ts, D1 applies on top: the TimeoutError arrives untranslated, not as a ComfyError. This contradicts the JSDoc at outputs.ts:71 — "Bytes are piped straight to disk, so an output larger than memory is fine." An output larger than memory is precisely the one that takes longer than 30 seconds. On Cloud the request 302s to signed object storage and undici follows it, so the 30 s covers the redirect chain plus the whole body.

Coverage. timeoutMs appears 0 times in outputs.test.ts and transport.test.ts. Control: the same search across all src/**/*.test.ts returns 2 hits, both in client.test.ts — so the zero is a real absence, not a broken query.

Suggested shape, matching what getJobEvents already does: give getAssetContent a timeoutMs parameter, default it to null (or to an idle-based deadline), and let toFile/toBytes accept signal and timeoutMs. Worth noting getJobEvents also accepts signal and the Output methods do not, so a caller currently cannot cancel a large download at all.

D5 — `AssetFactory.fromUrl` bypasses the injected `fetch` and has no timeout or abort

// src/sdk/assets.ts:244-253
async fromUrl(url: string): Promise<Asset> {
  const response = await fetch(url, { redirect: "follow" });
  ...
}

This is the only raw fetch( in the whole hand-written source — searching src/sdk and src/low with tests excluded returns exactly this one line, against a control search for fetchImpl that returns three known-non-zero hits at transport.ts:162,169,254.

Three consequences, none of which apply to any other network call in the SDK:

new Comfy({ fetch }) is silently ignored here. ComfyOptions.fetch (client.ts:95) is a documented public option and is threaded into ComfyLow at client.ts:136, but AssetFactory receives only low and fetchImpl is private. A caller injecting a proxying, instrumenting or offline fetch gets unexpected direct egress from this one method.
No timeout. Every other call gets DEFAULT_TIMEOUT_MS = 30_000; this one gets none.
No abort. The signature takes no signal, unlike its siblings Asset.commit(signal), Asset.delete(signal) and AssetFactory.delete(id, signal).

Repro — real Comfy client, { fetch: countingFetch, timeoutMs: 300 }, two local origins:

ARM A  CONTROL assets.get()          injectedFetchCalls=1      <- must be >0
ARM B  SUSPECT assets.fromUrl()      injectedFetchCalls=0      <- injected fetch bypassed
ARM C  CONTROL assets.get(stalled)   aborted after 301ms (TimeoutError)   <- timeoutMs honored
ARM D  SUSPECT fromUrl(stalled)      after 2000ms -> STILL-HANGING

ARM D was still hanging when the 2 s race timer cut it off — it has no deadline to hit.

The earlier audit checked fromUrl through a coverage lens and correctly found it tested (rebutting the Python SDK's G3). The option-plumbing and deadline gap is orthogonal to that.

Provenance, and why nobody caught either one

Both defects trace by git blame to 01ac0c79 ("Build the full two-layer Comfy API v2 SDK"), which merged as PR #3 — 6122 additions across 45 files with zero reviews of any kind. The getJobEvents guard in D4 arrived later still, in PR #15, also with zero reviews. The timeout lesson was learned on the SSE path and never carried across to the download path, and no reviewer was ever in a position to notice the asymmetry.

That is the general pattern rather than a one-off: of 34 merged PRs in this repo, 7 carried a standing human approving review at merge. One (#27) had a real human approval that a push dismissed 33 seconds before it merged. Three (#39, #43, #51) merged on a CodeRabbit approval alone, which GitHub counts toward required_approving_review_count with no bot filter — including #43, a declared breaking change to the public constructor. The remaining 23 had no approving review at all. By volume that is 8682 of 9052 added lines (95.9%) merged with no standing human approval.

Worth being precise about the mechanism, because the obvious reading is wrong: branch protection here is correctly configured. /branches/main/protection requires 1 approval, code-owner review, stale-review dismissal and 4 status checks. (/rulesets carries only "CLA Check" — reading that alone would give a false "wide open" answer.) The gate is real; it simply has enforce_admins: false, and the author of ~92% of the code is an admin. Nothing to fix in the diff — flagging it as the reason two guard-sibling gaps sat unnoticed, and as an org-level posture question rather than a repo one.

Two other things checked under this lens and cleared, recorded so nobody re-derives them:

Cross-origin bearer-token leak on getAssetContent — looked like a textbook instance (its sibling getAssetContentUrl uses redirect: "manual" explicitly so the token is never attached to object storage, while getAssetContent follows redirects). It does not leak: undici implements the WHATWG rule stripping Authorization across an origin change, verified with both same-origin control arms showing the header present and the cross-origin hop showing it absent.
PRs #44/#46/#48 closed unmerged, features re-landed inside the zero-review #49 — checked whether review feedback got laundered away. It did not. #46's substantive finding was fixed and shipped (jobBasePath() at transport.ts:100-103), and #44's embedWorkflow never landed at all (0 hits for embedWorkflow/extra_pnginfo on main, against a control of 7 files hitting extra_data).

@christian-byrne commented on GitHub (Aug 21, 2026): Follow-up sweep on the same ref (`54c0b40`), different lens: **which merges never got a human read, and what came in through them.** Two more verified defects, both in the same files this issue already covers, both the same shape as D1 — a guard that exists at one call site and not at its sibling. Adding them here rather than opening new issues. Runtime-verified the same way as D1–D3: real source under `node --experimental-strip-types`, stubs only for the three modules with uninstalled deps (`hash-wasm`, `zod`, `eventsource-parser`), every claim with control arms that are known-non-zero. --- ## D4 — `Output.toFile` / `toBytes` inherit a 30-second **whole-transfer** deadline, and no caller can raise or disable it `ComfyLow.getJobEvents` deliberately disables the client's default timeout for a long-lived stream: ```ts // src/low/transport.ts:488-491 const response = await this.request("GET", path, { headers: { Accept: "text/event-stream" }, signal: options.signal, timeoutMs: options.timeoutMs ?? null, // <-- the guard }); ``` with the JSDoc right above it at `:477-479`: *"No default timeout: an idle stream must not time out mid-job."* The other long-lived streaming path in the same class has no such guard. `getAssetContent` (`transport.ts:363-378`) takes `{ range?, signal? }` and **no `timeoutMs` at all**, so `request()` applies `DEFAULT_TIMEOUT_MS = 30_000` (`transport.ts:41`) through `resolveSignal` (`:230-239`). `AbortSignal.timeout(30_000)` is a wall-clock deadline covering the entire request **including body consumption** — it is not an idle timeout. The sdk layer forwards nothing. `outputs.ts:76` `toFile(path, { range? })` and `:90` `toBytes({ range? })` accept no `signal` and no `timeoutMs`. And the client-level knob cannot rescue it: `ComfyOptions.timeoutMs` is typed `number` (`client.ts:94`), `ComfyLowOptions.timeoutMs` likewise (`transport.ts:61`), and `this.defaultTimeoutMs = options.timeoutMs ?? DEFAULT_TIMEOUT_MS`. A TypeScript caller can **raise** the global deadline but never **disable** it, and cannot scope the change to downloads. The only route to `timeoutMs: null` is the untyped `ComfyLow.request` escape hatch. **Repro** — real `ComfyLow` + real `Output` against a `node:http` server streaming 6 chunks 250 ms apart (~1.5 s total). Client `timeoutMs: 400` stands to that 1.5 s stream as the shipped 30 s default stands to a >30 s download. ``` ARM A CONTROL low.request(timeoutMs:null) slow body OK after 1758ms -> 48 bytes read ARM B CONTROL output.toFile() fast body OK after 3ms ARM C SUSPECT output.toFile() slow body FAILED after 402ms -> TimeoutError: The operation was aborted due to timeout ARM D SUSPECT output.toBytes() slow body FAILED after 400ms -> TimeoutError: The operation was aborted due to timeout ``` Both controls are known-non-zero **successes** — A proves the slow body is readable once the timeout is off, B proves `toFile` works at all — so C and D are attributable to the deadline, not the harness. **Impact.** Any output download exceeding 30 s wall-clock aborts mid-stream. Because it is `outputs.ts`, D1 applies on top: the `TimeoutError` arrives untranslated, not as a `ComfyError`. This contradicts the JSDoc at `outputs.ts:71` — *"Bytes are piped straight to disk, so an output larger than memory is fine."* An output larger than memory is precisely the one that takes longer than 30 seconds. On Cloud the request 302s to signed object storage and undici follows it, so the 30 s covers the redirect chain plus the whole body. **Coverage.** `timeoutMs` appears **0 times** in `outputs.test.ts` and `transport.test.ts`. Control: the same search across all `src/**/*.test.ts` returns 2 hits, both in `client.test.ts` — so the zero is a real absence, not a broken query. Suggested shape, matching what `getJobEvents` already does: give `getAssetContent` a `timeoutMs` parameter, default it to `null` (or to an idle-based deadline), and let `toFile`/`toBytes` accept `signal` and `timeoutMs`. Worth noting `getJobEvents` also accepts `signal` and the `Output` methods do not, so a caller currently cannot cancel a large download at all. --- ## D5 — `AssetFactory.fromUrl` bypasses the injected `fetch` and has no timeout or abort ```ts // src/sdk/assets.ts:244-253 async fromUrl(url: string): Promise<Asset> { const response = await fetch(url, { redirect: "follow" }); ... } ``` This is the **only** raw `fetch(` in the whole hand-written source — searching `src/sdk` and `src/low` with tests excluded returns exactly this one line, against a control search for `fetchImpl` that returns three known-non-zero hits at `transport.ts:162,169,254`. Three consequences, none of which apply to any other network call in the SDK: 1. **`new Comfy({ fetch })` is silently ignored here.** `ComfyOptions.fetch` (`client.ts:95`) is a documented public option and is threaded into `ComfyLow` at `client.ts:136`, but `AssetFactory` receives only `low` and `fetchImpl` is `private`. A caller injecting a proxying, instrumenting or offline `fetch` gets unexpected direct egress from this one method. 2. **No timeout.** Every other call gets `DEFAULT_TIMEOUT_MS = 30_000`; this one gets none. 3. **No abort.** The signature takes no `signal`, unlike its siblings `Asset.commit(signal)`, `Asset.delete(signal)` and `AssetFactory.delete(id, signal)`. **Repro** — real `Comfy` client, `{ fetch: countingFetch, timeoutMs: 300 }`, two local origins: ``` ARM A CONTROL assets.get() injectedFetchCalls=1 <- must be >0 ARM B SUSPECT assets.fromUrl() injectedFetchCalls=0 <- injected fetch bypassed ARM C CONTROL assets.get(stalled) aborted after 301ms (TimeoutError) <- timeoutMs honored ARM D SUSPECT fromUrl(stalled) after 2000ms -> STILL-HANGING ``` ARM D was still hanging when the 2 s race timer cut it off — it has no deadline to hit. The earlier audit checked `fromUrl` through a coverage lens and correctly found it tested (rebutting the Python SDK's G3). The option-plumbing and deadline gap is orthogonal to that. --- ## Provenance, and why nobody caught either one Both defects trace by `git blame` to `01ac0c79` ("Build the full two-layer Comfy API v2 SDK"), which merged as **PR #3 — 6122 additions across 45 files with zero reviews of any kind**. The `getJobEvents` guard in D4 arrived later still, in **PR #15**, also with zero reviews. The timeout lesson was learned on the SSE path and never carried across to the download path, and no reviewer was ever in a position to notice the asymmetry. That is the general pattern rather than a one-off: of 34 merged PRs in this repo, **7 carried a standing human approving review at merge**. One (#27) had a real human approval that a push dismissed 33 seconds before it merged. Three (#39, #43, #51) merged on a CodeRabbit approval alone, which GitHub counts toward `required_approving_review_count` with no bot filter — including #43, a declared breaking change to the public constructor. The remaining 23 had no approving review at all. By volume that is **8682 of 9052 added lines (95.9%) merged with no standing human approval**. Worth being precise about the mechanism, because the obvious reading is wrong: branch protection here is **correctly configured**. `/branches/main/protection` requires 1 approval, code-owner review, stale-review dismissal and 4 status checks. (`/rulesets` carries only "CLA Check" — reading that alone would give a false "wide open" answer.) The gate is real; it simply has `enforce_admins: false`, and the author of ~92% of the code is an admin. Nothing to fix in the diff — flagging it as the reason two guard-sibling gaps sat unnoticed, and as an org-level posture question rather than a repo one. Two other things checked under this lens and **cleared**, recorded so nobody re-derives them: - **Cross-origin bearer-token leak on `getAssetContent`** — looked like a textbook instance (its sibling `getAssetContentUrl` uses `redirect: "manual"` explicitly so the token is never attached to object storage, while `getAssetContent` follows redirects). **It does not leak**: undici implements the WHATWG rule stripping `Authorization` across an origin change, verified with both same-origin control arms showing the header present and the cross-origin hop showing it absent. - **PRs #44/#46/#48 closed unmerged, features re-landed inside the zero-review #49** — checked whether review feedback got laundered away. It did not. #46's substantive finding was fixed and shipped (`jobBasePath()` at `transport.ts:100-103`), and #44's `embedWorkflow` never landed at all (0 hits for `embedWorkflow`/`extra_pnginfo` on `main`, against a control of 7 files hitting `extra_data`).

bsakaguchi referenced this issue

2026-10-31 22:03:52 +00:00

[PR #7] [MERGED] chore: set placeholder version to 0.1.0 for a consistent SDK launch #17

No labels

priority:nice-to-have

priority:recommended

pull-request

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Comfy-Org/comfy-typescript-sdk#7

No description provided.

Rows
Columns

[GH-ISSUE #62] Error translation, SSE error handling, and 429 retry: three verified defects plus the test gaps that let them through (cross-SDK parity audit) #7

D1 — outputs.ts never calls translate(), so all three public download methods leak the raw low.ApiError

D2 — Job.events() swallows every ApiError, turning a contract-legal 501 into a 100 ms reconnect loop

D3 — submit() retries only queue_full, but the spec requires backing off on any 429 + Retry-After

Test gaps

Checked and clean — please don't re-audit these

D4 — Output.toFile / toBytes inherit a 30-second whole-transfer deadline, and no caller can raise or disable it

D5 — AssetFactory.fromUrl bypasses the injected fetch and has no timeout or abort

Provenance, and why nobody caught either one

D1 — `outputs.ts` never calls `translate()`, so all three public download methods leak the raw `low.ApiError`

D2 — `Job.events()` swallows every `ApiError`, turning a contract-legal 501 into a 100 ms reconnect loop

D3 — `submit()` retries only `queue_full`, but the spec requires backing off on any 429 + `Retry-After`

D4 — `Output.toFile` / `toBytes` inherit a 30-second whole-transfer deadline, and no caller can raise or disable it

D5 — `AssetFactory.fromUrl` bypasses the injected `fetch` and has no timeout or abort