Skip to content

Commit 8b6a91b

Browse files
wei-haiclaude
andcommitted
fix: translate errors on every public entry point, add AsyncOutput.to_stream, widen 429 retry
Three defects behind one untested guarantee: nothing asserted that the public surface only ever raises comfy_sdk exceptions. - 10 public entry points skipped translating() and leaked the raw comfy_low.errors.ApiError: Output/AsyncOutput's download methods, both AssetFactory.get, both JobFactory.get, and the non-501 raise in Job.events()/AsyncJob.events(). Because comfy_low.errors and comfy_sdk.exceptions both export a NotFound and they are unrelated, the documented `except NotFound` around a download silently never fired. - AsyncOutput was missing to_stream entirely, breaking the documented "swap the import and add await" parity. - submit() gated its 429 retry on isinstance(err, QueueFull), i.e. on the server's error code. The contract disambiguates a retryable 429 by status + Retry-After, so a 429 named deployment_not_ready -- a serverless cold start -- hard-failed on the first attempt. The retry delay is now clamped to what remains of the retry budget and floored at zero. Retry-After is untrusted server input: it was previously passed to sleep() unbounded, so one response could park a caller far past _QUEUE_RETRY_BUDGET, and a negative value raised an uncaught ValueError on the sync path while spinning the async path with no pause. Also pins an incidental fix: the old `retry_after or _DEFAULT_RETRY_AFTER` treated Retry-After: 0 as absent and slept the full default. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VaAqCLd2mvvwsYkDTHMyBX
1 parent c96eb09 commit 8b6a91b

11 files changed

Lines changed: 443 additions & 34 deletions

src/comfy_sdk/assets.py

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -270,7 +270,8 @@ def from_url(self, url: str) -> Asset:
270270

271271
def get(self, asset_id: str) -> Asset:
272272
"""Rehydrate an already-committed asset by UUID."""
273-
model = self._low.get_asset(asset_id)
273+
with translating():
274+
model = self._low.get_asset(asset_id)
274275
asset = Asset(self._low, _rehydrated_source(model, asset_id))
275276
asset._apply(model)
276277
return asset
@@ -319,7 +320,8 @@ async def from_url(self, url: str) -> AsyncAsset:
319320
return AsyncAsset(self._low, _bytes_source(content, filename, ct))
320321

321322
async def get(self, asset_id: str) -> AsyncAsset:
322-
model = await self._low.get_asset(asset_id)
323+
with translating():
324+
model = await self._low.get_asset(asset_id)
323325
asset = AsyncAsset(self._low, _rehydrated_source(model, asset_id))
324326
asset._apply(model)
325327
return asset

src/comfy_sdk/client.py

Lines changed: 31 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@
3030

3131
from . import _core
3232
from .assets import AssetFactory, AsyncAssetFactory
33-
from .exceptions import QueueFull, WorkflowFormatUi, to_sdk_error
33+
from .exceptions import WorkflowFormatUi, to_sdk_error
3434
from .jobs import AsyncJob, AsyncJobFactory, Job, JobFactory
3535
from .workflows import Workflow, WorkflowFactory
3636

@@ -140,7 +140,7 @@ def submit(
140140
api_key: str | None = None,
141141
idempotency_key: str | None = None,
142142
) -> Job:
143-
"""Submit a workflow. Retries ``queue_full`` with ``Retry-After``.
143+
"""Submit a workflow. Retries any 429 that carries ``Retry-After``.
144144
145145
Sends an auto-generated ``Idempotency-Key`` so the server rejects an
146146
accidental exact resend of *this* request (``422 idempotency_key_reuse``)
@@ -166,11 +166,24 @@ def submit(
166166
model = self._low.post_jobs(graph, idempotency_key=key, extra_data=extra_data)
167167
return Job(self._low, model)
168168
except ApiError as exc:
169-
err = to_sdk_error(exc)
170-
if isinstance(err, QueueFull) and time.monotonic() < deadline:
171-
time.sleep(err.retry_after or _DEFAULT_RETRY_AFTER)
169+
# Disambiguated by status + Retry-After, not `code` alone
170+
# (e.g. `deployment_not_ready`); a bare `queue_full` 429 (no
171+
# header) must still retry on the default pause — dropping
172+
# that fallback is the regression this predicate already hit.
173+
retryable = exc.http_status == 429 and (
174+
exc.retry_after is not None or exc.code == "queue_full"
175+
)
176+
remaining = deadline - time.monotonic()
177+
if retryable and remaining > 0:
178+
raw_delay = (
179+
exc.retry_after if exc.retry_after is not None else _DEFAULT_RETRY_AFTER
180+
)
181+
# Clamp: a server-supplied Retry-After is untrusted input —
182+
# never sleep past the retry budget, and never negative.
183+
delay = max(0.0, min(raw_delay, remaining))
184+
time.sleep(delay)
172185
continue
173-
raise err from exc
186+
raise to_sdk_error(exc) from exc
174187

175188
def run(
176189
self,
@@ -249,11 +262,19 @@ async def submit(
249262
model = await self._low.post_jobs(graph, idempotency_key=key, extra_data=extra_data)
250263
return AsyncJob(self._low, model)
251264
except ApiError as exc:
252-
err = to_sdk_error(exc)
253-
if isinstance(err, QueueFull) and time.monotonic() < deadline:
254-
await asyncio.sleep(err.retry_after or _DEFAULT_RETRY_AFTER)
265+
# See the sync `submit` above for the predicate and clamp.
266+
retryable = exc.http_status == 429 and (
267+
exc.retry_after is not None or exc.code == "queue_full"
268+
)
269+
remaining = deadline - time.monotonic()
270+
if retryable and remaining > 0:
271+
raw_delay = (
272+
exc.retry_after if exc.retry_after is not None else _DEFAULT_RETRY_AFTER
273+
)
274+
delay = max(0.0, min(raw_delay, remaining))
275+
await asyncio.sleep(delay)
255276
continue
256-
raise err from exc
277+
raise to_sdk_error(exc) from exc
257278

258279
async def run(
259280
self,

src/comfy_sdk/jobs.py

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@
2424

2525
from . import _core
2626
from .events import Event, StatusChange, event_from_raw
27-
from .exceptions import JobFailed, translating
27+
from .exceptions import JobFailed, to_sdk_error, translating
2828
from .outputs import AsyncOutput, Output
2929

3030
_RECONNECT_PAUSE = 0.1
@@ -165,7 +165,7 @@ def events(self) -> Iterator[Event]:
165165
except ApiError as exc:
166166
if exc.http_status == 501:
167167
return # surface has no SSE — poll paths remain authoritative
168-
raise
168+
raise to_sdk_error(exc) from exc
169169
except (httpx.HTTPError, httpx.StreamError):
170170
pass # connection dropped mid-stream — reconnect below
171171
if terminal_seen:
@@ -277,7 +277,7 @@ async def events(self) -> AsyncIterator[Event]:
277277
except ApiError as exc:
278278
if exc.http_status == 501:
279279
return # surface has no SSE — poll paths remain authoritative
280-
raise
280+
raise to_sdk_error(exc) from exc
281281
except (httpx.HTTPError, httpx.StreamError):
282282
pass
283283
if terminal_seen:
@@ -299,12 +299,14 @@ def __init__(self, low: ComfyLow) -> None:
299299
self._low = low
300300

301301
def get(self, job_id: str) -> Job:
302-
return Job(self._low, self._low.get_job(job_id))
302+
with translating():
303+
return Job(self._low, self._low.get_job(job_id))
303304

304305

305306
class AsyncJobFactory:
306307
def __init__(self, low: AsyncComfyLow) -> None:
307308
self._low = low
308309

309310
async def get(self, job_id: str) -> AsyncJob:
310-
return AsyncJob(self._low, await self._low.get_job(job_id))
311+
with translating():
312+
return AsyncJob(self._low, await self._low.get_job(job_id))

src/comfy_sdk/outputs.py

Lines changed: 28 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@
1717
from comfy_low.models import Output as LowOutput
1818
from comfy_low.transport import AsyncComfyLow, ComfyLow
1919

20+
from .exceptions import translating
21+
2022
_CHUNK = 64 * 1024
2123

2224

@@ -80,7 +82,7 @@ def to_file(self, path: str | PathLike[str], *, range: tuple[int, int] | None =
8082
``range=(0, 4)`` yields the first five bytes.
8183
"""
8284
dest = Path(path)
83-
with self._low.get_asset_content(self._model.id, range=range) as resp:
85+
with translating(), self._low.get_asset_content(self._model.id, range=range) as resp:
8486
with open(dest, "wb") as fh:
8587
for chunk in resp.iter_bytes(_CHUNK):
8688
fh.write(chunk)
@@ -93,7 +95,7 @@ def to_stream(self, stream: BinaryIO, *, range: tuple[int, int] | None = None) -
9395
closed — that stays the caller's. See :meth:`to_file` for ``range``.
9496
"""
9597
written = 0
96-
with self._low.get_asset_content(self._model.id, range=range) as resp:
98+
with translating(), self._low.get_asset_content(self._model.id, range=range) as resp:
9799
for chunk in resp.iter_bytes(_CHUNK):
98100
stream.write(chunk)
99101
written += len(chunk)
@@ -107,7 +109,7 @@ def to_bytes(self, *, range: tuple[int, int] | None = None) -> bytes:
107109
See :meth:`to_file` for ``range``.
108110
"""
109111
buf = bytearray()
110-
with self._low.get_asset_content(self._model.id, range=range) as resp:
112+
with translating(), self._low.get_asset_content(self._model.id, range=range) as resp:
111113
for chunk in resp.iter_bytes(_CHUNK):
112114
buf.extend(chunk)
113115
return bytes(buf)
@@ -122,7 +124,8 @@ def get_download_url(self) -> DownloadUrl:
122124
``expires_at`` is ``None``. (A genuine failure — e.g. an unknown output
123125
id — still raises the same typed error as any other call.)
124126
"""
125-
url, expires_at = self._low.get_asset_content_url(self._model.id)
127+
with translating():
128+
url, expires_at = self._low.get_asset_content_url(self._model.id)
126129
return DownloadUrl(url=url, expires_at=expires_at)
127130

128131
def __repr__(self) -> str:
@@ -171,23 +174,36 @@ async def to_file(
171174
) -> Path:
172175
"""Async :meth:`Output.to_file` — same chunked write and inclusive ``range``."""
173176
dest = Path(path)
174-
async with self._low.get_asset_content(self._model.id, range=range) as resp:
175-
with open(dest, "wb") as fh:
176-
async for chunk in resp.aiter_bytes(_CHUNK):
177-
fh.write(chunk)
177+
with translating():
178+
async with self._low.get_asset_content(self._model.id, range=range) as resp:
179+
with open(dest, "wb") as fh:
180+
async for chunk in resp.aiter_bytes(_CHUNK):
181+
fh.write(chunk)
178182
return dest
179183

184+
async def to_stream(self, stream: BinaryIO, *, range: tuple[int, int] | None = None) -> int:
185+
"""Async :meth:`Output.to_stream` — same write-only semantics."""
186+
written = 0
187+
with translating():
188+
async with self._low.get_asset_content(self._model.id, range=range) as resp:
189+
async for chunk in resp.aiter_bytes(_CHUNK):
190+
stream.write(chunk)
191+
written += len(chunk)
192+
return written
193+
180194
async def to_bytes(self, *, range: tuple[int, int] | None = None) -> bytes:
181195
"""Async :meth:`Output.to_bytes` — buffers the whole body in memory."""
182196
buf = bytearray()
183-
async with self._low.get_asset_content(self._model.id, range=range) as resp:
184-
async for chunk in resp.aiter_bytes(_CHUNK):
185-
buf.extend(chunk)
197+
with translating():
198+
async with self._low.get_asset_content(self._model.id, range=range) as resp:
199+
async for chunk in resp.aiter_bytes(_CHUNK):
200+
buf.extend(chunk)
186201
return bytes(buf)
187202

188203
async def get_download_url(self) -> DownloadUrl:
189204
"""See the sync ``Output.get_download_url`` for the redirect/inline split."""
190-
url, expires_at = await self._low.get_asset_content_url(self._model.id)
205+
with translating():
206+
url, expires_at = await self._low.get_asset_content_url(self._model.id)
191207
return DownloadUrl(url=url, expires_at=expires_at)
192208

193209
def __repr__(self) -> str:

tests/conftest.py

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,28 @@ class ServerState:
3535
require_auth: bool = False
3636
# POST /jobs returns 429 queue_full this many times before succeeding.
3737
queue_full_times: int = 0
38+
# Like `queue_full_times`, but the 429 carries no Retry-After header at
39+
# all — the bare-`queue_full` path, which retries using the client's
40+
# default pause rather than a server-given delay.
41+
queue_full_times_no_retry_after: int = 0
42+
# POST /jobs answers 429 queue_full with this literal Retry-After header
43+
# value once, then succeeds — for a test to send an out-of-range value
44+
# (e.g. a huge number, to prove the client clamps to its retry budget,
45+
# or a negative one, to prove a malformed header doesn't crash the
46+
# sync loop or busy-loop the async one).
47+
queue_full_retry_after_header: str | None = None
48+
# POST /jobs returns a 429 naming a code OTHER than `queue_full` (with a
49+
# Retry-After header) this many times before succeeding — the contract
50+
# disambiguates a retryable 429 by status + Retry-After, not by `code`
51+
# (e.g. `deployment_not_ready` on a serverless cold start).
52+
retryable_429_times: int = 0
53+
retryable_429_code: str = "deployment_not_ready"
3854
# POST /jobs returns this error envelope (status, code) instead of 201.
3955
job_error: tuple[int, str] | None = None
56+
# GET /jobs/{id} answers 404 job_not_found instead of the job.
57+
job_not_found: bool = False
58+
# GET /jobs/{id}/events answers this (status, code) instead of connecting.
59+
events_error: tuple[int, str] | None = None
4060
# Number of GET /jobs/{id} polls before the job reports succeeded.
4161
polls_to_succeed: int = 1
4262
# Terminal status the job reaches.
@@ -267,6 +287,9 @@ def _serve_content(self) -> None:
267287
self.wfile.write(data)
268288

269289
def _serve_job(self, job_id: str) -> None:
290+
if state.job_not_found:
291+
self._err(404, "job_not_found", "no such job")
292+
return
270293
state.job_poll_count += 1
271294
if state.job_poll_count >= state.polls_to_succeed:
272295
status = state.terminal_status
@@ -292,6 +315,10 @@ def _serve_job_workflow(self, job_id: str) -> None:
292315

293316
def _serve_events(self, job_id: str) -> None:
294317
state.events_connect_count += 1
318+
if state.events_error is not None:
319+
status, code = state.events_error
320+
self._err(status, code, f"events error {code}")
321+
return
295322
if state.events_not_implemented:
296323
self._err(501, "not_implemented", "SSE is not supported on this surface")
297324
return
@@ -383,6 +410,30 @@ def _post_jobs(self) -> None:
383410
)
384411
return
385412

413+
if state.queue_full_times_no_retry_after > 0:
414+
state.queue_full_times_no_retry_after -= 1
415+
self._json(429, {"error": {"code": "queue_full", "message": "full"}})
416+
return
417+
418+
if state.queue_full_retry_after_header is not None:
419+
header = state.queue_full_retry_after_header
420+
state.queue_full_retry_after_header = None
421+
self._json(
422+
429,
423+
{"error": {"code": "queue_full", "message": "full"}},
424+
headers={"Retry-After": header},
425+
)
426+
return
427+
428+
if state.retryable_429_times > 0:
429+
state.retryable_429_times -= 1
430+
self._json(
431+
429,
432+
{"error": {"code": state.retryable_429_code, "message": "warming up"}},
433+
headers={"Retry-After": "0"},
434+
)
435+
return
436+
386437
if state.job_error is not None:
387438
status, code = state.job_error
388439
self._err(status, code, f"job error {code}")

tests/test_assets.py

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,7 @@
77

88
import pytest
99

10-
from comfy_low.errors import NotFound
11-
from comfy_sdk import Comfy, HashMismatch
10+
from comfy_sdk import Comfy, HashMismatch, NotFound
1211

1312

1413
def test_dedup_fast_path_skips_upload(server, tmp_path) -> None:

tests/test_async.py

Lines changed: 43 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,12 @@
22

33
from __future__ import annotations
44

5+
import asyncio
6+
57
import pytest
68

7-
from comfy_low.errors import NotFound
8-
from comfy_sdk import AsyncComfy, MissingAsset, Progress, StatusChange
9+
import comfy_sdk.client as _client_module
10+
from comfy_sdk import AsyncComfy, MissingAsset, NotFound, Progress, StatusChange
911

1012

1113
def _wf(client: AsyncComfy):
@@ -150,6 +152,45 @@ async def test_async_queue_full_retries_with_retry_after(server) -> None:
150152
assert server.state.submit_count == 3
151153

152154

155+
async def test_async_submit_clamps_huge_retry_after_to_remaining_budget(
156+
server, monkeypatch
157+
) -> None:
158+
# Async counterpart of the sync clamp test — same predicate, same clamp,
159+
# both loops must bound a single sleep to what's left of the budget.
160+
monkeypatch.setattr(_client_module, "_QUEUE_RETRY_BUDGET", 5.0)
161+
sleeps: list[float] = []
162+
163+
async def _fake_sleep(seconds: float) -> None:
164+
sleeps.append(seconds)
165+
166+
monkeypatch.setattr(asyncio, "sleep", _fake_sleep)
167+
server.state.queue_full_retry_after_header = "10000000"
168+
async with AsyncComfy() as client:
169+
job = await client.submit(_wf(client))
170+
assert job.id.startswith("job_")
171+
assert server.state.submit_count == 2
172+
assert len(sleeps) == 1
173+
assert 0 <= sleeps[0] <= 5.0
174+
175+
176+
async def test_async_submit_negative_retry_after_does_not_storm(server, monkeypatch) -> None:
177+
# Where the sync loop crashes on `time.sleep(-5)` (ValueError), the async
178+
# loop's `asyncio.sleep(-5)` returns instantly and would busy-loop the
179+
# server for the whole retry budget with no pause. The clamp floors the
180+
# delay at 0 either way; this proves the async side specifically.
181+
sleeps: list[float] = []
182+
183+
async def _fake_sleep(seconds: float) -> None:
184+
sleeps.append(seconds)
185+
186+
monkeypatch.setattr(asyncio, "sleep", _fake_sleep)
187+
server.state.queue_full_retry_after_header = "-5"
188+
async with AsyncComfy() as client:
189+
job = await client.submit(_wf(client))
190+
assert job.id.startswith("job_")
191+
assert sleeps == [0.0]
192+
193+
153194
async def test_async_delete_asset_by_id(server) -> None:
154195
async with AsyncComfy() as client:
155196
await client.assets.delete("asset_uuid_01")

0 commit comments

Comments
 (0)