[PR #63] [MERGED] fix: keep protocol errors off the public surface, add AsyncOutput.to_stream, widen the 429 retry #63

New issue

Closed

opened 2026-10-31 22:01:06 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 22:01:06 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-python-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-and-429-retry

📝 Commits (4)

28f68a9 fix: keep protocol errors off the public surface, add AsyncOutput.to_stream, widen the 429 retry
0a5bc48 fix: tighten retry budgets and error contract coverage
852a21b fix: preserve final retries and partial stream writes
8e977b4 Merge branch 'main' into fix/error-contract-and-429-retry

📊 Changes

12 files changed (+513 additions, -41 deletions)

View changed files

📝 README.md (+11 -3)
📝 src/comfy_sdk/assets.py (+4 -2)
📝 src/comfy_sdk/client.py (+33 -12)
📝 src/comfy_sdk/jobs.py (+7 -5)
📝 src/comfy_sdk/outputs.py (+42 -15)
📝 tests/conftest.py (+51 -0)
📝 tests/test_assets.py (+1 -2)
📝 tests/test_async.py (+59 -2)
📝 tests/test_download_and_workflows.py (+30 -0)
➕ tests/test_error_contract.py (+143 -0)
📝 tests/test_jobs.py (+85 -0)
➕ tests/test_sync_async_parity.py (+47 -0)

📄 Description

Fixes #62. Three defects, one untested guarantee: nothing asserted that the public surface only raises comfy_sdk exceptions.

1. Ten entry points leaked `comfy_low.errors.ApiError`

translating() was skipped on all four Output/AsyncOutput download methods, both AssetFactory.get, both JobFactory.get, and the non-501 re-raise in events().

flowchart LR
  subgraph AFTER["after"]
    direction LR
    X["submit() · get() · to_file() · events()"] --> TB["translating()"] --> XB["comfy_sdk.NotFound"]
  end
  subgraph BEFORE["before"]
    direction LR
    A["submit()"] --> TA["translating()"] --> AB["comfy_sdk.NotFound"]
    B["get() · to_file() · events()"] --> BB["comfy_low.NotFound"]
  end

Both modules export an unrelated class named NotFound, so the documented pattern silently never fired:

from comfy_sdk import NotFound
try:
    out.to_file("result.png")
except NotFound:   # never caught — a comfy_low.NotFound was raised
    ...

2. `AsyncOutput` was missing `to_stream`

Six of the seven sync/async class pairs matched; this one was off by exactly that method, against a README promising the surfaces are identical.

3. `submit()` gave up on a retryable 429

The gate was isinstance(err, QueueFull) — the server's error code — but the contract disambiguates a retryable 429 by status + Retry-After.

429 response	before	after
`queue_full`, with header	retries	retries
`queue_full`, no header	retries (default pause)	retries (default pause)
`deployment_not_ready` (cold start)	raises on attempt 1	retries
any 429, no header, other code	raises	raises

The new predicate is exc.http_status == 429 and (exc.retry_after is not None or exc.code == "queue_full"), in both loops — forward-compatible with any future 429 code.

Hardening

Retry-After is untrusted input and reached sleep() unbounded: the budget was checked before the sleep, never against its duration, so Retry-After: 10000000 produced one ~115-day sleep past a 60s budget. A negative value diverged — time.sleep(-5) raised an uncaught ValueError, asyncio.sleep(-5) spun the loop with no pause. Now clamped to the remaining budget and floored at zero.

Also fixes Retry-After: 0 being treated as absent by the old or idiom (it slept the full default), now pinned by an assertion.

Risk

Callers catching comfy_low.errors.ApiError around those ten entry points must catch the comfy_sdk exception instead — what the README already documents. Two pre-existing tests were asserting the leak itself and were retargeted.

Verification

ruff, mypy, pytest (163 passed, 4 skipped), pytest --cov, codegen drift. Each new test confirmed to fail against the pre-fix source. New tests assert on a monkeypatched sleep argument — none wait.

Out of scope, left as follow-ups from the issue's checklist: from_url, Preview.to_pil(), the _CODE_BY_STATUS fallback branch, the README escape hatches, backoff_schedule, head_asset_by_hash, and the _BY_CODE/to_sdk_error completeness guard.

Summary by CodeRabbit

Bug Fixes
- Improved handling of temporary service capacity and deployment warm-up responses with bounded retries.
- Standardized typed error reporting across asset, job, output, and event-stream operations.
- Improved reliability for synchronous and asynchronous output downloads, including streaming to existing binary streams.
- Ensured retry delays handle missing, negative, and overly long server-provided values safely.
Documentation
- Added synchronous and asynchronous examples for streaming downloads.
- Clarified typed-error handling and retry behavior.
Tests
- Expanded coverage for retry behavior, error consistency, streaming, and synchronous/asynchronous feature parity.

_{🔄 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-python-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-and-429-retry` --- ### 📝 Commits (4) - [`28f68a9`](https://github.com/Comfy-Org/comfy-python-sdk/commit/28f68a9f401d03693efacfc9e60f251ef75d65a3) fix: keep protocol errors off the public surface, add AsyncOutput.to_stream, widen the 429 retry - [`0a5bc48`](https://github.com/Comfy-Org/comfy-python-sdk/commit/0a5bc4860b479b7684fe7a8ab2865c5031519f1f) fix: tighten retry budgets and error contract coverage - [`852a21b`](https://github.com/Comfy-Org/comfy-python-sdk/commit/852a21b622696e96a06b34bbdd17f65fcb26350e) fix: preserve final retries and partial stream writes - [`8e977b4`](https://github.com/Comfy-Org/comfy-python-sdk/commit/8e977b4fcc5cc68ae728dbfb01c263a5b906aa76) Merge branch 'main' into fix/error-contract-and-429-retry ### 📊 Changes **12 files changed** (+513 additions, -41 deletions) <details> <summary>View changed files</summary> 📝 `README.md` (+11 -3) 📝 `src/comfy_sdk/assets.py` (+4 -2) 📝 `src/comfy_sdk/client.py` (+33 -12) 📝 `src/comfy_sdk/jobs.py` (+7 -5) 📝 `src/comfy_sdk/outputs.py` (+42 -15) 📝 `tests/conftest.py` (+51 -0) 📝 `tests/test_assets.py` (+1 -2) 📝 `tests/test_async.py` (+59 -2) 📝 `tests/test_download_and_workflows.py` (+30 -0) ➕ `tests/test_error_contract.py` (+143 -0) 📝 `tests/test_jobs.py` (+85 -0) ➕ `tests/test_sync_async_parity.py` (+47 -0) </details> ### 📄 Description Fixes #62. Three defects, one untested guarantee: nothing asserted that the public surface only raises `comfy_sdk` exceptions. ### 1. Ten entry points leaked `comfy_low.errors.ApiError` `translating()` was skipped on all four `Output`/`AsyncOutput` download methods, both `AssetFactory.get`, both `JobFactory.get`, and the non-501 re-raise in `events()`. ```mermaid flowchart LR subgraph AFTER["after"] direction LR X["submit() · get() · to_file() · events()"] --> TB["translating()"] --> XB["comfy_sdk.NotFound"] end subgraph BEFORE["before"] direction LR A["submit()"] --> TA["translating()"] --> AB["comfy_sdk.NotFound"] B["get() · to_file() · events()"] --> BB["comfy_low.NotFound"] end ``` Both modules export an unrelated class named `NotFound`, so the documented pattern silently never fired: ```python from comfy_sdk import NotFound try: out.to_file("result.png") except NotFound: # never caught — a comfy_low.NotFound was raised ... ``` ### 2. `AsyncOutput` was missing `to_stream` Six of the seven sync/async class pairs matched; this one was off by exactly that method, against a README promising the surfaces are identical. ### 3. `submit()` gave up on a retryable 429 The gate was `isinstance(err, QueueFull)` — the server's error *code* — but the contract disambiguates a retryable 429 by **status + `Retry-After`**. | 429 response | before | after | |---|---|---| | `queue_full`, with header | retries | retries | | `queue_full`, no header | retries (default pause) | retries (default pause) | | `deployment_not_ready` (cold start) | **raises on attempt 1** | retries | | any 429, no header, other code | raises | raises | The new predicate is `exc.http_status == 429 and (exc.retry_after is not None or exc.code == "queue_full")`, in both loops — forward-compatible with any future 429 code. ### Hardening `Retry-After` is untrusted input and reached `sleep()` unbounded: the budget was checked *before* the sleep, never against its duration, so `Retry-After: 10000000` produced one ~115-day sleep past a 60s budget. A negative value diverged — `time.sleep(-5)` raised an uncaught `ValueError`, `asyncio.sleep(-5)` spun the loop with no pause. Now clamped to the remaining budget and floored at zero. Also fixes `Retry-After: 0` being treated as absent by the old `or` idiom (it slept the full default), now pinned by an assertion. ### Risk Callers catching `comfy_low.errors.ApiError` around those ten entry points must catch the `comfy_sdk` exception instead — what the README already documents. Two pre-existing tests were asserting the leak itself and were retargeted. ### Verification `ruff`, `mypy`, `pytest` (163 passed, 4 skipped), `pytest --cov`, codegen drift. Each new test confirmed to fail against the pre-fix source. New tests assert on a monkeypatched sleep argument — none wait. Out of scope, left as follow-ups from the issue's checklist: `from_url`, `Preview.to_pil()`, the `_CODE_BY_STATUS` fallback branch, the README escape hatches, `backoff_schedule`, `head_asset_by_hash`, and the `_BY_CODE`/`to_sdk_error` completeness guard.  ## Summary by CodeRabbit - **Bug Fixes** - Improved handling of temporary service capacity and deployment warm-up responses with bounded retries. - Standardized typed error reporting across asset, job, output, and event-stream operations. - Improved reliability for synchronous and asynchronous output downloads, including streaming to existing binary streams. - Ensured retry delays handle missing, negative, and overly long server-provided values safely. - **Documentation** - Added synchronous and asynchronous examples for streaming downloads. - Clarified typed-error handling and retry behavior. - **Tests** - Expanded coverage for retry behavior, error consistency, streaming, and synchronous/asynchronous feature parity.  --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 22:01:06 +00:00