Comfy-Org/comfy-python-sdk

Fork 0

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

[GH-ISSUE #62] Public error contract is untested, and two defects hide behind it: translating() skipped on all download paths, AsyncOutput.to_stream missing #7

New issue

Closed

opened 2026-10-26 22:40:26 +00:00 by bsakaguchi · 1 comment

bsakaguchi commented

2026-10-26 22:40:26 +00:00

Owner

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

Originally assigned to: @wei-hai on GitHub.

Summary

A test-coverage audit of origin/main (c96eb09) turned up two verified defects that share one
root cause: the guarantees the README and docstrings make about the public surface have no test
behind them. Everything else in the audit was clean — including codegen drift, which is guarded
and enforced as a required check (details at the bottom).

Both defects trace to the same code and the same guarantee, so they are batched here rather than
filed per-module.

Defect 1 — `translating()` is applied inconsistently; 10 public entry points leak the raw `comfy_low.ApiError`

comfy_sdk/exceptions.py::translating documents the contract:

Wrap the SDK-level operations that call comfy_low with this so integrators only ever catch
comfy_sdk exceptions (MissingAsset, HashMismatch, NotFound, ...), never the raw protocol error.

The README's "Typed errors" section makes the same promise: every error is a ComfyError subclass.

These entry points do not wrap, and raise comfy_low.errors.ApiError straight through:

Entry point	File
`Output.to_file` / `to_stream` / `to_bytes` / `get_download_url`	`src/comfy_sdk/outputs.py` — the module never imports `translating` at all
`AsyncOutput.to_file` / `to_bytes` / `get_download_url`	same
`AssetFactory.get` / `AsyncAssetFactory.get`	`src/comfy_sdk/assets.py:271`, `:322`
`JobFactory.get` / `AsyncJobFactory.get`	`src/comfy_sdk/jobs.py:301`, `:309`
`Job.events()` / `AsyncJob.events()` — the non-501 `raise`	`src/comfy_sdk/jobs.py:516`, `:628`

Why this bites harder than a normal type mismatch: comfy_low.errors and
comfy_sdk.exceptions both export a class named NotFound, and they are unrelated
(ApiError vs ComfyError). So a consumer writing the documented

from comfy_sdk import NotFound
try:
    out.to_file("result.png")
except NotFound:
    ...

silently never catches. And for the entity-specific server codes (job_not_found,
asset_not_found), comfy_low._BY_CODE has no entry either, so what escapes is a bare
comfy_low.errors.ApiError — not even the protocol-level NotFound.

This is the output-download path, i.e. the most-used consumer call after run().

Reproduction

Driven by this repo's own stub server (tests/conftest.py), no mocks. Controls first — two
entry points that are wrapped, on the same code paths, to show the harness detects correct
behaviour:

import os, sys
sys.path.insert(0, "tests")
import conftest as C

srv = C._start_server()
os.environ[C.BASE_URL_ENV_VAR] = srv.base_url

from comfy_sdk import Comfy
import comfy_sdk.exceptions as sdkx
import comfy_low.errors as lowx

client = Comfy()

def probe(label, fn):
    try:
        fn(); print(f"  {label:46s} no error")
    except sdkx.ComfyError as e:
        print(f"  {label:46s} comfy_sdk.{type(e).__name__}  OK")
    except lowx.ApiError as e:
        print(f"  {label:46s} comfy_low.{type(e).__name__}  *** LEAK ***")

wf = client.workflows.from_json({"3": {"class_type": "KSampler", "inputs": {}}})

print("controls (expected: comfy_sdk.*):")
srv.state.job_error = (422, "invalid_workflow")
probe("client.submit(wf)               [wrapped]", lambda: client.submit(wf))
srv.state.job_error = None
srv.state.job_workflow_not_found = True
job = client.submit(wf)
probe("job.get_workflow()              [wrapped]", lambda: job.get_workflow())
srv.state.job_workflow_not_found = False

print("suspects:")
client.assets.delete("asset_out_01")          # stub 404s a deleted asset thereafter
probe("client.assets.get(deleted id)", lambda: client.assets.get("asset_out_01"))

out = client.submit(wf).result().get_outputs("13")[0]
import io
probe("output.to_bytes()   (deleted asset)", lambda: out.to_bytes())
probe("output.to_file(...)  (deleted asset)", lambda: out.to_file("/dev/null"))
probe("output.to_stream(...) (deleted asset)", lambda: out.to_stream(io.BytesIO()))
probe("output.get_download_url() (deleted)", lambda: out.get_download_url())

client.close(); C._stop_server(srv)

Output:

controls (expected: comfy_sdk.*):
  client.submit(wf)               [wrapped]      comfy_sdk.InvalidWorkflow  OK
  job.get_workflow()              [wrapped]      comfy_sdk.NotFound  OK
suspects:
  client.assets.get(deleted id)                  comfy_low.NotFound  *** LEAK ***
  output.to_bytes()   (deleted asset)            comfy_low.NotFound  *** LEAK ***
  output.to_file(...)  (deleted asset)           comfy_low.NotFound  *** LEAK ***
  output.to_stream(...) (deleted asset)          comfy_low.NotFound  *** LEAK ***
  output.get_download_url() (deleted)            comfy_low.NotFound  *** LEAK ***

JobFactory.get needs a server that 404s a job (the stub returns a job for any id); against a
minimal 404-everything handler it leaks the same way:

  client.jobs.get(id)                        -> comfy_low.ApiError    *** LEAKED ***
  client.assets.get(id)                      -> comfy_low.ApiError    *** LEAKED ***

Suggested fix

Wrap the four outputs.py download methods (sync + async) and both factories' get() in
with translating():, and translate before the raise in events(). Then add the test that
would have caught it — one that asserts no comfy_low.ApiError escapes any public entry point.

Defect 2 — `Output.to_stream` has no `AsyncOutput` counterpart, and no test

src/comfy_sdk/outputs.py:89 defines Output.to_stream(...). AsyncOutput has no to_stream.

The README states:

Comfy and AsyncComfy expose the identical surface — swap the import and add await / async for.

and closes the downloads section with "(AsyncOutput mirrors all of the above with await.)".

A public-surface diff over every sync/async pair — 6 of 7 are identical, one is not:

Comfy           /AsyncComfy         sync-only=[] async-only=[]
Asset           /AsyncAsset         sync-only=[] async-only=[]
AssetFactory    /AsyncAssetFactory  sync-only=[] async-only=[]
Job             /AsyncJob           sync-only=[] async-only=[]
JobFactory      /AsyncJobFactory    sync-only=[] async-only=[]
Output          /AsyncOutput        sync-only=['to_stream'] async-only=[]  <-- ASYMMETRY
ComfyLow        /AsyncComfyLow      sync-only=[] async-only=[]

(the check normalises the intentional close/aclose rename)

to_stream is also the only download method with zero test references anywhere in tests/ —
which is why the asymmetry went unnoticed. Fix is async def to_stream on AsyncOutput plus a
parity test asserting the sync and async public method sets match.

The coverage checklist behind them

Ordered by consumer impact. These are gaps, not known bugs.

A test asserting the translating() guarantee across every public entry point — the one that catches Defect 1 and any future regression of it.
A sync/async parity test over the 7 class pairs — the one that catches Defect 2.
AssetFactory.from_url / AsyncAssetFactory.from_url — zero tests. It is one of four README-documented asset constructors and the only one that opens its own httpx client, so it silently ignores the client's timeout= and client_info.
Preview.to_pil() — zero tests. README shows show(pv.to_pil()) and the repo ships a pil extra for it; Pillow is already in [dev].
comfy_low.errors.error_from_envelope — zero direct tests, and the _CODE_BY_STATUS status-derived fallback is entirely dark. tests/conftest.py::_err always writes a well-formed JSON envelope, so the "bare 401/404/429 with no JSON body" branch that the docstring exists for never runs. That is exactly the branch that fires when an nginx/LB/proxy answers instead of the API — the self-hosted comfy-api-proxy deployment the README targets.
to_sdk_error — 3 of 13 code mappings tested. insufficient_credits, forbidden, blob_not_found appear nowhere in tests/. The table is currently complete, so this is a guard gap rather than a live bug — worth closing the same way test_spec_coverage.py already closes it for operationIds: assert comfy_low.errors._BY_CODE's code set is fully mapped by comfy_sdk.exceptions.
The four escape hatches the README's architecture section promises — raw_request, open, all-headers, per-request timeout/abort — have no contract test. Only SSE timeout=None is exercised.
_core.backoff_schedule — zero direct tests; wait() never polls long enough to reach cap.
head_asset_by_hash's non-200/404 branch — zero tests; the stub only answers 200/404.

What was checked and is clean

Recording these so nobody re-runs them:

Codegen drift is guarded and enforced. scripts/check_drift.py regenerates into a tempdir and compares byte-for-byte, so both a spec edit without a regen and a hand-edit of _generated.py are caught. The comfy_low codegen drift job runs it on every PR and push, and it is a required status check on main (verified at repos/.../branches/main/protection, not inferred from the workflow file). The generator is pinned and both ruff and mypy exclude the generated file, so a formatter cannot perturb the byte comparison. Worth noting for whoever checks next: the ruleset list contains only "CLA Check" — the real gate is classic branch protection, so checking rulesets alone yields a false "unguarded" reading.
Auth is unusually well covered — test_auth_headers.py asserts the header actually sent rather than the 401/200 outcome, and test_transport_security.py / test_follow_up_links.py / test_content_redirect_security.py cover cross-origin bearer-token non-leak.
Retry: queue_full → Retry-After → retry → success, plus budget exhaustion, sync and async.
Pagination: N/A, not a gap — the vendored contract has 11 operations and no list endpoint.
SSE: decoder unit tests cover comment/keepalive, stray blank line, malformed JSON, non-object JSON, multi-line data:; integration-level covers reconnect-without-replay, the zombie stalled connection, and the 501 no-SSE surface.
No mock-only assertions. The suite drives a real stdlib HTTP stub; the only two monkeypatch.setattr calls target module constants, not the code under test.
Assertion density: all 22 test files have assertions; no file reports as covered while asserting nothing. tests/integration/ is correctly skipif-gated.

Originally created by @christian-byrne on GitHub (Aug 21, 2026). Original GitHub issue: https://github.com/Comfy-Org/comfy-python-sdk/issues/62 Originally assigned to: @wei-hai on GitHub. ## Summary A test-coverage audit of `origin/main` (`c96eb09`) turned up two verified defects that share one root cause: **the guarantees the README and docstrings make about the public surface have no test behind them.** Everything else in the audit was clean — including codegen drift, which is guarded *and* enforced as a required check (details at the bottom). Both defects trace to the same code and the same guarantee, so they are batched here rather than filed per-module. --- ## Defect 1 — `translating()` is applied inconsistently; 10 public entry points leak the raw `comfy_low.ApiError` `comfy_sdk/exceptions.py::translating` documents the contract: > Wrap the SDK-level operations that call `comfy_low` with this so integrators only ever catch > `comfy_sdk` exceptions (`MissingAsset`, `HashMismatch`, `NotFound`, ...), never the raw protocol error. The README's "Typed errors" section makes the same promise: every error is a `ComfyError` subclass. These entry points do **not** wrap, and raise `comfy_low.errors.ApiError` straight through: | Entry point | File | |---|---| | `Output.to_file` / `to_stream` / `to_bytes` / `get_download_url` | `src/comfy_sdk/outputs.py` — the module never imports `translating` at all | | `AsyncOutput.to_file` / `to_bytes` / `get_download_url` | same | | `AssetFactory.get` / `AsyncAssetFactory.get` | `src/comfy_sdk/assets.py:271`, `:322` | | `JobFactory.get` / `AsyncJobFactory.get` | `src/comfy_sdk/jobs.py:301`, `:309` | | `Job.events()` / `AsyncJob.events()` — the non-501 `raise` | `src/comfy_sdk/jobs.py:516`, `:628` | **Why this bites harder than a normal type mismatch:** `comfy_low.errors` and `comfy_sdk.exceptions` both export a class named `NotFound`, and they are unrelated (`ApiError` vs `ComfyError`). So a consumer writing the documented ```python from comfy_sdk import NotFound try: out.to_file("result.png") except NotFound: ... ``` silently never catches. And for the entity-specific server codes (`job_not_found`, `asset_not_found`), `comfy_low._BY_CODE` has no entry either, so what escapes is a bare `comfy_low.errors.ApiError` — not even the protocol-level `NotFound`. This is the output-download path, i.e. the most-used consumer call after `run()`. ### Reproduction Driven by this repo's own stub server (`tests/conftest.py`), no mocks. **Controls first** — two entry points that *are* wrapped, on the same code paths, to show the harness detects correct behaviour: ```python import os, sys sys.path.insert(0, "tests") import conftest as C srv = C._start_server() os.environ[C.BASE_URL_ENV_VAR] = srv.base_url from comfy_sdk import Comfy import comfy_sdk.exceptions as sdkx import comfy_low.errors as lowx client = Comfy() def probe(label, fn): try: fn(); print(f" {label:46s} no error") except sdkx.ComfyError as e: print(f" {label:46s} comfy_sdk.{type(e).__name__} OK") except lowx.ApiError as e: print(f" {label:46s} comfy_low.{type(e).__name__} *** LEAK ***") wf = client.workflows.from_json({"3": {"class_type": "KSampler", "inputs": {}}}) print("controls (expected: comfy_sdk.*):") srv.state.job_error = (422, "invalid_workflow") probe("client.submit(wf) [wrapped]", lambda: client.submit(wf)) srv.state.job_error = None srv.state.job_workflow_not_found = True job = client.submit(wf) probe("job.get_workflow() [wrapped]", lambda: job.get_workflow()) srv.state.job_workflow_not_found = False print("suspects:") client.assets.delete("asset_out_01") # stub 404s a deleted asset thereafter probe("client.assets.get(deleted id)", lambda: client.assets.get("asset_out_01")) out = client.submit(wf).result().get_outputs("13")[0] import io probe("output.to_bytes() (deleted asset)", lambda: out.to_bytes()) probe("output.to_file(...) (deleted asset)", lambda: out.to_file("/dev/null")) probe("output.to_stream(...) (deleted asset)", lambda: out.to_stream(io.BytesIO())) probe("output.get_download_url() (deleted)", lambda: out.get_download_url()) client.close(); C._stop_server(srv) ``` Output: ``` controls (expected: comfy_sdk.*): client.submit(wf) [wrapped] comfy_sdk.InvalidWorkflow OK job.get_workflow() [wrapped] comfy_sdk.NotFound OK suspects: client.assets.get(deleted id) comfy_low.NotFound *** LEAK *** output.to_bytes() (deleted asset) comfy_low.NotFound *** LEAK *** output.to_file(...) (deleted asset) comfy_low.NotFound *** LEAK *** output.to_stream(...) (deleted asset) comfy_low.NotFound *** LEAK *** output.get_download_url() (deleted) comfy_low.NotFound *** LEAK *** ``` `JobFactory.get` needs a server that 404s a job (the stub returns a job for any id); against a minimal 404-everything handler it leaks the same way: ``` client.jobs.get(id) -> comfy_low.ApiError *** LEAKED *** client.assets.get(id) -> comfy_low.ApiError *** LEAKED *** ``` ### Suggested fix Wrap the four `outputs.py` download methods (sync + async) and both factories' `get()` in `with translating():`, and translate before the `raise` in `events()`. Then add the test that would have caught it — one that asserts *no* `comfy_low.ApiError` escapes any public entry point. --- ## Defect 2 — `Output.to_stream` has no `AsyncOutput` counterpart, and no test `src/comfy_sdk/outputs.py:89` defines `Output.to_stream(...)`. `AsyncOutput` has no `to_stream`. The README states: > `Comfy` and `AsyncComfy` expose the identical surface — swap the import and add `await` / `async for`. and closes the downloads section with "(`AsyncOutput` mirrors all of the above with `await`.)". A public-surface diff over every sync/async pair — 6 of 7 are identical, one is not: ``` Comfy /AsyncComfy sync-only=[] async-only=[] Asset /AsyncAsset sync-only=[] async-only=[] AssetFactory /AsyncAssetFactory sync-only=[] async-only=[] Job /AsyncJob sync-only=[] async-only=[] JobFactory /AsyncJobFactory sync-only=[] async-only=[] Output /AsyncOutput sync-only=['to_stream'] async-only=[] <-- ASYMMETRY ComfyLow /AsyncComfyLow sync-only=[] async-only=[] ``` (the check normalises the intentional `close`/`aclose` rename) `to_stream` is also the **only** download method with zero test references anywhere in `tests/` — which is why the asymmetry went unnoticed. Fix is `async def to_stream` on `AsyncOutput` plus a parity test asserting the sync and async public method sets match. --- ## The coverage checklist behind them Ordered by consumer impact. These are gaps, not known bugs. - [ ] **A test asserting the `translating()` guarantee** across every public entry point — the one that catches Defect 1 and any future regression of it. - [ ] **A sync/async parity test** over the 7 class pairs — the one that catches Defect 2. - [ ] `AssetFactory.from_url` / `AsyncAssetFactory.from_url` — zero tests. It is one of four README-documented asset constructors and the only one that opens its own `httpx` client, so it silently ignores the client's `timeout=` and `client_info`. - [ ] `Preview.to_pil()` — zero tests. README shows `show(pv.to_pil())` and the repo ships a `pil` extra for it; Pillow is already in `[dev]`. - [ ] `comfy_low.errors.error_from_envelope` — zero direct tests, and the `_CODE_BY_STATUS` status-derived fallback is entirely dark. `tests/conftest.py::_err` always writes a well-formed JSON envelope, so the "bare 401/404/429 with no JSON body" branch that the docstring exists for never runs. That is exactly the branch that fires when an nginx/LB/proxy answers instead of the API — the self-hosted `comfy-api-proxy` deployment the README targets. - [ ] `to_sdk_error` — 3 of 13 code mappings tested. `insufficient_credits`, `forbidden`, `blob_not_found` appear nowhere in `tests/`. The table is currently complete, so this is a guard gap rather than a live bug — worth closing the same way `test_spec_coverage.py` already closes it for operationIds: assert `comfy_low.errors._BY_CODE`'s code set is fully mapped by `comfy_sdk.exceptions`. - [ ] The four escape hatches the README's architecture section promises — `raw_request`, `open`, all-headers, per-request timeout/abort — have no contract test. Only SSE `timeout=None` is exercised. - [ ] `_core.backoff_schedule` — zero direct tests; `wait()` never polls long enough to reach `cap`. - [ ] `head_asset_by_hash`'s non-200/404 branch — zero tests; the stub only answers 200/404. ## What was checked and is clean Recording these so nobody re-runs them: - **Codegen drift is guarded and enforced.** `scripts/check_drift.py` regenerates into a tempdir and compares byte-for-byte, so both a spec edit without a regen and a hand-edit of `_generated.py` are caught. The `comfy_low codegen drift` job runs it on every PR and push, and it is a **required status check** on `main` (verified at `repos/.../branches/main/protection`, not inferred from the workflow file). The generator is pinned and both ruff and mypy exclude the generated file, so a formatter cannot perturb the byte comparison. Worth noting for whoever checks next: the *ruleset* list contains only "CLA Check" — the real gate is classic branch protection, so checking rulesets alone yields a false "unguarded" reading. - **Auth** is unusually well covered — `test_auth_headers.py` asserts the header actually sent rather than the 401/200 outcome, and `test_transport_security.py` / `test_follow_up_links.py` / `test_content_redirect_security.py` cover cross-origin bearer-token non-leak. - **Retry**: `queue_full` → `Retry-After` → retry → success, plus budget exhaustion, sync and async. - **Pagination**: N/A, not a gap — the vendored contract has 11 operations and no list endpoint. - **SSE**: decoder unit tests cover comment/keepalive, stray blank line, malformed JSON, non-object JSON, multi-line `data:`; integration-level covers reconnect-without-replay, the zombie stalled connection, and the 501 no-SSE surface. - **No mock-only assertions.** The suite drives a real stdlib HTTP stub; the only two `monkeypatch.setattr` calls target module constants, not the code under test. - **Assertion density**: all 22 test files have assertions; no file reports as covered while asserting nothing. `tests/integration/` is correctly `skipif`-gated.

bsakaguchi closed this issue

2026-10-26 22:40:26 +00:00

bsakaguchi commented

2026-10-26 22:40:27 +00:00

Author

Owner

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

Third defect behind the same untested contract: `submit()` gives up on a retryable 429

Found while auditing comfy-typescript-sdk against this issue's findings — the same gap is in both
SDKs, so I'm adding it here rather than opening a second issue.

submit() retries by error code, but the contract disambiguates 429 by status + Retry-After.
The SDK's own generated model says so:

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".

— src/comfy_low/models/_generated.py:176-182

The retry loop gates on the translated type:

# src/comfy_sdk/client.py:170
if isinstance(err, QueueFull) and time.monotonic() < deadline:
    time.sleep(err.retry_after or _DEFAULT_RETRY_AFTER)
    continue
raise err from exc

and QueueFull is produced for exactly one code:

# src/comfy_sdk/exceptions.py:124
if exc.code == "queue_full":

Why this is easy to dismiss, and isn't

_CODE_BY_STATUS maps 429: "queue_full" (errors.py:140), so a first read suggests every 429
retries. It doesn't — that table is a fallback, reached only when the envelope carries no code:

# src/comfy_low/errors.py:113-118
code = (err or {}).get("code") if isinstance(err, dict) else None
...
if code is None:
    code = _CODE_BY_STATUS.get(http_status, "error")

So the behaviour splits on whether the server bothered to name the code:

429 response	resulting code	`submit()`
bare, no JSON envelope	falls back to `queue_full`	retries ✅
`{"error": {"code": "deployment_not_ready"}}`	`deployment_not_ready` → `ComfyError`	raises on attempt 1 ❌

A deployment that is merely warming up — the one case the contract explicitly calls out as
retryable — is the case that fails, and it fails on a serverless cold start, which is exactly when a
caller least expects a hard error. _QUEUE_RETRY_BUDGET is never consulted.

Fix

retry_after is already parsed off the header (transport.py:76) and carried on the ApiError, so
the information needed is present at the branch — it's the predicate that's wrong. Gate on the
status and the header rather than the code:

if err.http_status == 429 and err.retry_after is not None and time.monotonic() < deadline:

Both paths need it — sync client.py:171 and async client.py:254.

That also makes the behaviour forward-compatible: any future 429 code the server adds becomes
retryable without an SDK release, which is what "disambiguated by error.code alone" is asking for.

Test

Fits the contract-test table this issue already proposes. The case that matters is a 429 whose
envelope names a code other than queue_full — deployment_not_ready is the one in the spec.
A test that only exercises a bare 429 passes today and proves nothing, which is presumably how this
survived.

Same defect exists in comfy-typescript-sdk (filed as Comfy-Org/comfy-typescript-sdk#62) — worth
fixing in one pass so the two SDKs don't drift further.

@christian-byrne commented on GitHub (Aug 21, 2026): ### Third defect behind the same untested contract: `submit()` gives up on a retryable 429 Found while auditing `comfy-typescript-sdk` against this issue's findings — the same gap is in both SDKs, so I'm adding it here rather than opening a second issue. `submit()` retries by **error code**, but the contract disambiguates 429 by **status + `Retry-After`**. The SDK's own generated model says so: > `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". > > — `src/comfy_low/models/_generated.py:176-182` The retry loop gates on the translated type: ```python # src/comfy_sdk/client.py:170 if isinstance(err, QueueFull) and time.monotonic() < deadline: time.sleep(err.retry_after or _DEFAULT_RETRY_AFTER) continue raise err from exc ``` and `QueueFull` is produced for exactly one code: ```python # src/comfy_sdk/exceptions.py:124 if exc.code == "queue_full": ``` #### Why this is easy to dismiss, and isn't `_CODE_BY_STATUS` maps `429: "queue_full"` (`errors.py:140`), so a first read suggests every 429 retries. It doesn't — that table is a **fallback**, reached only when the envelope carries no code: ```python # src/comfy_low/errors.py:113-118 code = (err or {}).get("code") if isinstance(err, dict) else None ... if code is None: code = _CODE_BY_STATUS.get(http_status, "error") ``` So the behaviour splits on whether the server bothered to name the code: | 429 response | resulting code | `submit()` | | --- | --- | --- | | bare, no JSON envelope | falls back to `queue_full` | retries ✅ | | `{"error": {"code": "deployment_not_ready"}}` | `deployment_not_ready` → `ComfyError` | **raises on attempt 1** ❌ | A deployment that is merely *warming up* — the one case the contract explicitly calls out as retryable — is the case that fails, and it fails on a serverless cold start, which is exactly when a caller least expects a hard error. `_QUEUE_RETRY_BUDGET` is never consulted. #### Fix `retry_after` is already parsed off the header (`transport.py:76`) and carried on the `ApiError`, so the information needed is present at the branch — it's the predicate that's wrong. Gate on the status and the header rather than the code: ```python if err.http_status == 429 and err.retry_after is not None and time.monotonic() < deadline: ``` Both paths need it — sync `client.py:171` and async `client.py:254`. That also makes the behaviour forward-compatible: any future 429 code the server adds becomes retryable without an SDK release, which is what "disambiguated by `error.code` alone" is asking for. #### Test Fits the contract-test table this issue already proposes. The case that matters is a 429 whose envelope names a code **other than** `queue_full` — `deployment_not_ready` is the one in the spec. A test that only exercises a bare 429 passes today and proves nothing, which is presumably how this survived. Same defect exists in `comfy-typescript-sdk` (filed as Comfy-Org/comfy-typescript-sdk#62) — worth fixing in one pass so the two SDKs don't drift further.

bsakaguchi referenced this issue

2026-10-26 22:40:42 +00:00

[PR #7] [MERGED] test + fix: async parity / decoder coverage, and dead Unauthorized branch removed #16