[PR #63] [MERGED] fix: translate download errors, stop the SSE 501/429 reconnect storm, widen the 429 retry, unbound the download timeout #65

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-typescript-sdk/pull/63
Author: @wei-hai
Created: 8/21/2026
Status: ✅ Merged
Merged: 8/23/2026
Merged by: @wei-hai

Base: main ← Head: fix/error-contract-timeouts-and-429-retry

📝 Commits (5)

7c0f520 fix: translate download errors, stop the SSE 501/429 reconnect storm, widen the 429 retry, unbound the download timeout
71735e5 fix: normalize the fromUrl target, and assert the SSE Retry-After delay
52474c2 fix: harden SSE retries and download handling
437b98a docs: call out nullable QueueFull retry delay
91889e4 Merge branch 'main' into fix/error-contract-timeouts-and-429-retry

📊 Changes

14 files changed (+525 additions, -51 deletions)

View changed files

📝 README.md (+10 -2)
📝 src/low/transport.test.ts (+50 -3)
📝 src/low/transport.ts (+13 -6)
📝 src/sdk/assets.test.ts (+74 -1)
📝 src/sdk/assets.ts (+18 -5)
📝 src/sdk/client.test.ts (+65 -0)
📝 src/sdk/client.ts (+17 -6)
📝 src/sdk/exceptions.test.ts (+7 -0)
📝 src/sdk/exceptions.ts (+4 -4)
📝 src/sdk/jobs.test.ts (+117 -2)
📝 src/sdk/jobs.ts (+24 -3)
📝 src/sdk/outputs.test.ts (+42 -1)
📝 src/sdk/outputs.ts (+35 -8)
📝 test/support/stub-server.ts (+49 -10)

📄 Description

Fixes #62. Five defects, all the same shape: a guard that existed at one call site and was never carried to its sibling.

D1 — `outputs.ts` never called `translate()`

The only file under src/sdk/ touching low without importing it, so toFile/toBytes/getDownloadUrl leaked the raw low.ApiError. Both layers export an unrelated class named NotFound, so a consumer saw the class name they expected while their instanceof check silently never fired — and instanceof ComfyError was false, contradicting the README. getDownloadUrl's JSDoc claimed it never throws; corrected.

D2 — `Job.events()` swallowed every `ApiError` as a transport drop

flowchart LR
  subgraph AFTER["after"]
    direction LR
    S2["GET /events → 501"] --> C2["ApiError, 501"] --> R2["return"]
  end
  subgraph BEFORE["before"]
    direction LR
    S1["GET /events → 501"] --> C1["treated as a drop"] --> P1["poll + 100ms pause"] --> S1
  end

Measured at 21 SSE attempts and 21 polls on a 2-second job — roughly 3000 of each on a 5-minute generation, aimed at the self-hosted deployment most likely to answer 501. On the 429 path it was self-inflicted: the server publishes Retry-After to shed load and the client reconnected into it at a fixed 100 ms. Now returns on 501 and honours Retry-After on 429.

D3 — `submit()` retried only `queue_full`

The spec requires treating any 429 with Retry-After as back-off-and-retry, and names deployment_not_ready for exactly the serverless cold start this SDK targets. That case failed outright on attempt one; queue_full without the header still retries on the default pause.

D4 — downloads inherited a 30-second whole-transfer deadline

getJobEvents disables the default timeout so a long stream is not killed; getAssetContent never got the same treatment, and AbortSignal.timeout(30_000) covers body consumption, not just the response. Any download over 30 s aborted mid-transfer, and no caller could raise or disable it — ComfyOptions.timeoutMs is typed number, so the only route to null was the untyped escape hatch. This contradicted toFile's own JSDoc that an output larger than memory is fine; such an output is precisely the one that takes over 30 seconds.

D5 — `AssetFactory.fromUrl` used a raw global `fetch`

The only one in the hand-written source. It ignored an injected fetch (so an integrator using one as a proxy or allowlist was unprotected on this one method), had no timeout, and could not be cancelled. Now routed through low.request, which gates Authorization behind isSameOrigin.

Behaviour changes, not just bug fixes

Downloads now default to no timeout. This is the point of D4 and matches getJobEvents, but a caller relying on a stuck download being killed automatically must now pass timeoutMs or signal. A client-level new Comfy({ timeoutMs }) does not act as a blanket cap — pass it per call.
events() on a 501 deployment now yields nothing and returns, where before it eventually yielded a terminal statusChange off the poll backstop. Deliberate, and matches the Python SDK. Use job.wait() on a deployment without SSE.

Hardening

Retry-After is untrusted input and this diff widens where it lands. Neither call site bounded it — the retry budget was checked before the sleep and never against its duration, so Retry-After: 86400 slept 24 hours through a 60-second budget, and the SSE loop has no deadline of its own. Now clamped to the remaining budget in submit() and to MAX_RECONNECT_PAUSE_MS in the reconnect loop.

Tests

outputs.test.ts was 100% happy-path and now covers the error path. An eventsStatus knob lets the events endpoint answer 501/429 at all — previously unreachable from the fixture — and those tests assert the connection attempt count, since amplification rather than event correctness is the defect. Also: a deployment_not_ready retry, a queue_full with no header, the clamps (asserted by spying the sleep argument, not by waiting), and fromUrl routing through the injected fetch without attaching Authorization cross-origin, with a same-origin positive control so it cannot pass vacuously.

Each new test was confirmed to fail against the pre-fix source — the 501 test times out rather than merely asserting, which is what proves the loop never terminates.

Verification

vitest run (181 passed), tsc --noEmit, oxlint, oxfmt --check, check-spec-drift.mjs. Spec and generated output untouched.

Out of scope, left as follow-ups from the issue's checklist: the blake3 known-answer vector, the BY_CODE completeness guard, coverage thresholds, and marking Asset.adopt() internal.

Summary by CodeRabbit

New Features
- Added cancellation and configurable timeout support for asset and output downloads.
- Improved URL downloads, including uppercase schemes, custom fetch clients, redirects, and secure credential handling.
Bug Fixes
- Improved handling of inline asset responses, slow transfers, and invalid retry delays.
- Added robust retries for queue and event-stream requests, with bounded backoff and cancellation support.
- High-level APIs now return typed SDK errors consistently.
Documentation
- Clarified typed errors, nullable retry information, and retry behavior.

_{🔄 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/63 **Author:** [@wei-hai](https://github.com/wei-hai) **Created:** 8/21/2026 **Status:** ✅ Merged **Merged:** 8/23/2026 **Merged by:** [@wei-hai](https://github.com/wei-hai) **Base:** `main` ← **Head:** `fix/error-contract-timeouts-and-429-retry` --- ### 📝 Commits (5) - [`7c0f520`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/7c0f5202bf625765b818d0969762f903ac11b98d) fix: translate download errors, stop the SSE 501/429 reconnect storm, widen the 429 retry, unbound the download timeout - [`71735e5`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/71735e57de815c3b54b024ee226671142cf8fa73) fix: normalize the fromUrl target, and assert the SSE Retry-After delay - [`52474c2`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/52474c23f78e6be264131ecf7035480d091f3901) fix: harden SSE retries and download handling - [`437b98a`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/437b98a8f717a0e8bee36a2b001688a8822d2bd2) docs: call out nullable QueueFull retry delay - [`91889e4`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/91889e46c89bf1fe857185d87b9a1388bb2b93de) Merge branch 'main' into fix/error-contract-timeouts-and-429-retry ### 📊 Changes **14 files changed** (+525 additions, -51 deletions) <details> <summary>View changed files</summary> 📝 `README.md` (+10 -2) 📝 `src/low/transport.test.ts` (+50 -3) 📝 `src/low/transport.ts` (+13 -6) 📝 `src/sdk/assets.test.ts` (+74 -1) 📝 `src/sdk/assets.ts` (+18 -5) 📝 `src/sdk/client.test.ts` (+65 -0) 📝 `src/sdk/client.ts` (+17 -6) 📝 `src/sdk/exceptions.test.ts` (+7 -0) 📝 `src/sdk/exceptions.ts` (+4 -4) 📝 `src/sdk/jobs.test.ts` (+117 -2) 📝 `src/sdk/jobs.ts` (+24 -3) 📝 `src/sdk/outputs.test.ts` (+42 -1) 📝 `src/sdk/outputs.ts` (+35 -8) 📝 `test/support/stub-server.ts` (+49 -10) </details> ### 📄 Description Fixes #62. Five defects, all the same shape: a guard that existed at one call site and was never carried to its sibling. ### D1 — `outputs.ts` never called `translate()` The only file under `src/sdk/` touching `low` without importing it, so `toFile`/`toBytes`/`getDownloadUrl` leaked the raw `low.ApiError`. Both layers export an unrelated class named `NotFound`, so a consumer saw the class name they expected while their `instanceof` check silently never fired — and `instanceof ComfyError` was `false`, contradicting the README. `getDownloadUrl`'s JSDoc claimed it never throws; corrected. ### D2 — `Job.events()` swallowed every `ApiError` as a transport drop ```mermaid flowchart LR subgraph AFTER["after"] direction LR S2["GET /events → 501"] --> C2["ApiError, 501"] --> R2["return"] end subgraph BEFORE["before"] direction LR S1["GET /events → 501"] --> C1["treated as a drop"] --> P1["poll + 100ms pause"] --> S1 end ``` Measured at 21 SSE attempts and 21 polls on a *2-second* job — roughly 3000 of each on a 5-minute generation, aimed at the self-hosted deployment most likely to answer 501. On the 429 path it was self-inflicted: the server publishes `Retry-After` to shed load and the client reconnected into it at a fixed 100 ms. Now returns on 501 and honours `Retry-After` on 429. ### D3 — `submit()` retried only `queue_full` The spec requires treating any 429 with `Retry-After` as back-off-and-retry, and names `deployment_not_ready` for exactly the serverless cold start this SDK targets. That case failed outright on attempt one; `queue_full` without the header still retries on the default pause. ### D4 — downloads inherited a 30-second whole-transfer deadline `getJobEvents` disables the default timeout so a long stream is not killed; `getAssetContent` never got the same treatment, and `AbortSignal.timeout(30_000)` covers body consumption, not just the response. Any download over 30 s aborted mid-transfer, and no caller could raise or disable it — `ComfyOptions.timeoutMs` is typed `number`, so the only route to `null` was the untyped escape hatch. This contradicted `toFile`'s own JSDoc that an output larger than memory is fine; such an output is precisely the one that takes over 30 seconds. ### D5 — `AssetFactory.fromUrl` used a raw global `fetch` The only one in the hand-written source. It ignored an injected `fetch` (so an integrator using one as a proxy or allowlist was unprotected on this one method), had no timeout, and could not be cancelled. Now routed through `low.request`, which gates `Authorization` behind `isSameOrigin`. ## Behaviour changes, not just bug fixes - **Downloads now default to no timeout.** This is the point of D4 and matches `getJobEvents`, but a caller relying on a stuck download being killed automatically must now pass `timeoutMs` or `signal`. A client-level `new Comfy({ timeoutMs })` does not act as a blanket cap — pass it per call. - **`events()` on a 501 deployment now yields nothing and returns**, where before it eventually yielded a terminal `statusChange` off the poll backstop. Deliberate, and matches the Python SDK. Use `job.wait()` on a deployment without SSE. ## Hardening `Retry-After` is untrusted input and this diff widens where it lands. Neither call site bounded it — the retry budget was checked before the sleep and never against its duration, so `Retry-After: 86400` slept 24 hours through a 60-second budget, and the SSE loop has no deadline of its own. Now clamped to the remaining budget in `submit()` and to `MAX_RECONNECT_PAUSE_MS` in the reconnect loop. ## Tests `outputs.test.ts` was 100% happy-path and now covers the error path. An `eventsStatus` knob lets the events endpoint answer 501/429 at all — previously unreachable from the fixture — and those tests assert the **connection attempt count**, since amplification rather than event correctness is the defect. Also: a `deployment_not_ready` retry, a `queue_full` with no header, the clamps (asserted by spying the sleep argument, not by waiting), and `fromUrl` routing through the injected `fetch` without attaching `Authorization` cross-origin, with a same-origin positive control so it cannot pass vacuously. Each new test was confirmed to fail against the pre-fix source — the 501 test times out rather than merely asserting, which is what proves the loop never terminates. ## Verification `vitest run` (181 passed), `tsc --noEmit`, `oxlint`, `oxfmt --check`, `check-spec-drift.mjs`. Spec and generated output untouched. Out of scope, left as follow-ups from the issue's checklist: the blake3 known-answer vector, the `BY_CODE` completeness guard, coverage thresholds, and marking `Asset.adopt()` internal.  ## Summary by CodeRabbit * **New Features** * Added cancellation and configurable timeout support for asset and output downloads. * Improved URL downloads, including uppercase schemes, custom fetch clients, redirects, and secure credential handling. * **Bug Fixes** * Improved handling of inline asset responses, slow transfers, and invalid retry delays. * Added robust retries for queue and event-stream requests, with bounded backoff and cancellation support. * High-level APIs now return typed SDK errors consistently. * **Documentation** * Clarified typed errors, nullable retry information, and retry behavior.  --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

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