From 04af79412a1173e0e501c62703039b800b03638e Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Sat, 5 Sep 2026 13:26:35 -0700 Subject: [PATCH] fix(models): raise the original failure when a same-key retry is refused 422 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Models.run` / `AsyncModels.run` re-raised whatever the *last* attempt raised. When a same-`Idempotency-Key` retry was rejected `422 idempotency_key_reuse`, that refusal replaced the failure that caused the retry — the `deadline_exceeded` 504 the default collect loop resends under, or the 500 a `retry_possibly_in_flight=True` policy resends under. The refusal is an artefact of the retry loop rather than an answer about the request, so the only diagnosable error was lost. Both loops now remember the FIRST retryable failure and, if a later attempt is refused for key reuse, raise that failure with the refusal chained on as `__cause__`. Both halves are translated to the idiomatic SDK types and carry the call's `Idempotency-Key`, and each keeps the traceback of the attempt that produced it. Only key reuse substitutes: every other terminal failure is still raised as-is, and nothing about *which* failures are retried changed. Behaviour change for callers: `except IdempotencyKeyReuse` around `models.run` no longer catches the rejected-resend case — inspect `exc.__cause__` instead. A key refusal on the FIRST attempt is unchanged. --- CHANGELOG.md | 13 ++++ README.md | 13 ++-- src/comfy_sdk/models.py | 85 +++++++++++++++++++++++++- src/comfy_sdk/retry.py | 15 +++-- tests/test_models_run_retry.py | 105 ++++++++++++++++++++++++++++++++- 5 files changed, 216 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f4968d..ce2cd8f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -147,6 +147,19 @@ notes for each version. ### Changed +- **Behaviour change: when a `client.models.run()` retry is refused `422` + `idempotency_key_reuse`, the failure that *caused* the retry is what is + raised** — the `deadline_exceeded` `504` the default collect loop resent + under, or the `500` a `retry_possibly_in_flight=True` policy resent under — + with the key refusal chained onto it as `__cause__`. The refusal is an + artefact of the retry loop rather than an answer about the request, and it + used to be the only error the caller saw, so the real failure was lost. **An + `except IdempotencyKeyReuse` around `models.run` no longer catches this + case**: catch the failure you actually care about (or `ComfyError`) and + inspect `exc.__cause__` to tell a rejected resend apart from a first-attempt + refusal. Nothing about *which* failures are retried changed, and no other + terminal failure is substituted — a `500` followed by a `404` still raises + the `404`. - **Breaking (wire): `client.models.run` now posts to Comfy Router.** It sends `POST {COMFY_ROUTER_BASE_URL}/v1/models/{provider}/{model}` — the route `spec/router-openapi.yaml` declares as `runRouterModel` — with the partner diff --git a/README.md b/README.md index ac04659..802bea7 100644 --- a/README.md +++ b/README.md @@ -474,7 +474,7 @@ The default policy: | Retried, at the server's pace | a `429` carrying `Retry-After` (queue full, out of credits, a concurrency limit) — a reject that started no work, so the key is released. The delay is the one the server named, not a guess | | Retried, at the server's pace | the answers that pace a resend of the *same* key for work already running: a `deadline_exceeded` `504` carrying `Retry-After` (Comfy stopped holding the connection at its own bound; the contract says to retry with the same key, which collects that generation rather than dispatching another), and an in-flight-key `concurrency_limit_exceeded` `409` carrying `Retry-After` (another call is already running under the same key; the resend collects it — the same bucket on a `429` is plain throttling and takes the ordinary retry path). One `run()` rides that loop to the finished result | | Not retried | every other 4xx — `400`/`content_policy_violation`, `404`, any `409` that is not the paced in-flight `concurrency_limit_exceeded` one above (`hash_mismatch` carries a `Retry-After` and is still deterministic, and the router's `invalid_input` key cases are answered by a NEW key), `422`, `401`, `402` — because asking again cannot change a deterministic refusal. A `429` with no `Retry-After` is not asking to be asked again either | -| Not retried by default | anything whose outcome is unknown: any **other 5xx response** — including the router's `service_unavailable` `503` (which asks a caller to retry with backoff but says nothing about the key), a `504` carrying no `Retry-After` (the router sends it only when it holds a generation to collect), and a `504` that is `provider_timeout` rather than `deadline_exceeded` — and a client-side timeout where the server may still be generating. The key stays claimed for these, so a same-key retry comes back `422 idempotency_key_reuse` and hides the real error — while a fresh-key retry is the second billed generation the one-key rule exists to prevent | +| Not retried by default | anything whose outcome is unknown: any **other 5xx response** — including the router's `service_unavailable` `503` (which asks a caller to retry with backoff but says nothing about the key), a `504` carrying no `Retry-After` (the router sends it only when it holds a generation to collect), and a `504` that is `provider_timeout` rather than `deadline_exceeded` — and a client-side timeout where the server may still be generating. The key stays claimed for these, so a same-key retry only comes back `422 idempotency_key_reuse` (that refusal no longer hides the real error — see below — but it is still a wasted request) — while a fresh-key retry is the second billed generation the one-key rule exists to prevent | | Budget | 60 seconds of **total elapsed time** from the first attempt, not a number of attempts. The collect loop gets its own, longer budget: 1200 seconds, two server deadline windows, so it can outlast the deadline that started it | | Backoff | 0.5s doubling to a 15s ceiling, with full jitter (each wait is drawn from `[0, ceiling]`), clamped to whatever is left of the budget. A `Retry-After` the server named is used as given instead | @@ -510,8 +510,10 @@ A note on what the default trades: the collect rule is `spec/router-openapi.yaml`'s, so it binds Comfy Router — but `COMFY_ROUTER_BASE_URL` can name a deployment that applies the v2 rule instead and keeps the key claimed across the `504`. There the collect resend comes back -`422 idempotency_key_reuse` in place of the real `504`. Set -`retry_collectable=False` on such a deployment. +`422 idempotency_key_reuse` — one wasted request, but not a lost diagnosis: +`run` raises the real `504` and chains the refusal onto it as `__cause__`, so +`except IdempotencyKeyReuse` around `run` does *not* catch it. Set +`retry_collectable=False` on such a deployment to skip the resend entirely. Other 5xx responses and client-side timeouts are the cases left out by default, and for the same reason. `run` holds the connection open while the server @@ -631,7 +633,10 @@ asset, job, event, and output helpers translate protocol errors, so catches of no replay — so if you pass your own `idempotency_key=` and reuse it, the second call raises this. After an ambiguous failure (e.g. a timeout where you don't know if the job was created), poll or list your jobs rather than resubmitting - with the same key. + with the same key. One exception: when `models.run`'s *own* retry is refused + for key reuse, it raises the failure that caused the retry and chains this + exception onto it as `__cause__`, since the refusal says nothing about why the + call failed. - `InsufficientCredits` — the account can't afford the job. - `QueueFull` — backpressure; carries `.retry_after` seconds. `client.submit` retries 429 responses with `Retry-After` for a bounded budget (including diff --git a/src/comfy_sdk/models.py b/src/comfy_sdk/models.py index d79b9d9..d08dd0e 100644 --- a/src/comfy_sdk/models.py +++ b/src/comfy_sdk/models.py @@ -49,10 +49,11 @@ import httpx from comfy_low.errors import ApiError +from comfy_low.errors import IdempotencyKeyReuse as ProtocolIdempotencyKeyReuse from comfy_low.transport import MODEL_RUN_TIMEOUT, AsyncComfyLow, ComfyLow from ._core import new_idempotency_key, validate_idempotency_key -from .exceptions import translating +from .exceptions import IdempotencyKeyReuse, _stamp, to_sdk_error, translating from .retry import DEFAULT_RETRY, Retrier, RetryPolicy from .router_exceptions import RouterError @@ -72,9 +73,55 @@ #: raising them, with no test failing. _CANDIDATE_FAILURES = (ApiError, RouterError, httpx.TransportError) +#: The answer that means the resend was never going to work: the server refused +#: the *key*, not the request. ``422`` is the v2 jobs rule +#: (single-use, reject-on-duplicate), which a deployment named by +#: ``COMFY_ROUTER_BASE_URL`` may apply to this route even though the router +#: contract does not. +_KEY_REUSE_STATUS = 422 +_KEY_REUSE_CODE = "idempotency_key_reuse" + _now = time.monotonic +def _is_key_reuse(exc: BaseException) -> bool: + """Whether ``exc`` is the server refusing a repeated ``Idempotency-Key``. + + Matched on the wire facts — ``422`` plus the ``idempotency_key_reuse`` + code — rather than on a class alone, because the retry loop runs *inside* + ``translating()``: what it catches is the raw + :class:`comfy_low.errors.ApiError`, and only the copy that leaves the block + is the idiomatic :class:`~comfy_sdk.exceptions.IdempotencyKeyReuse`. Either + layer's typed class is accepted outright so the answer does not depend on + which one raised. + """ + if isinstance(exc, (IdempotencyKeyReuse, ProtocolIdempotencyKeyReuse)): + return True + return ( + getattr(exc, "http_status", None) == _KEY_REUSE_STATUS + and getattr(exc, "code", None) == _KEY_REUSE_CODE + ) + + +def _as_sdk_error(exc: BaseException, idempotency_key: str) -> BaseException: + """``exc`` on the surface a caller catches, carrying ``idempotency_key``. + + ``translating()`` converts and stamps the exception it is *handed*, and + nothing else — so an ``ApiError`` chained onto that one as ``__cause__`` + would reach the caller as a raw protocol type this SDK otherwise never + shows. Both halves of the substitution below therefore go through here + first. Anything already idiomatic (a ``RouterError``, an ``httpx`` failure + with no response to translate) is only stamped. + + The translated copy inherits the original's traceback, so the chain still + points at the attempt that produced each half rather than at the one line + of this module that re-raised them. + """ + if not isinstance(exc, ApiError): + return _stamp(exc, idempotency_key) + return _stamp(to_sdk_error(exc).with_traceback(exc.__traceback__), idempotency_key) + + class _ModelsBase: """Read-only view of the configuration inherited from the host client.""" @@ -193,6 +240,15 @@ def run( ``RetryPolicy(retry_possibly_in_flight=True)``. See :mod:`comfy_sdk.retry`, and ``Comfy(retry=NO_RETRY)`` to switch it off. + **When a resend is refused because the key was already claimed + (``422`` ``idempotency_key_reuse``), what is raised is the failure that + caused the retry** — the ``504``, the ``500`` — with the key refusal + chained onto it as ``__cause__``. That refusal is an artefact of this + retry loop rather than an answer about the request, so surfacing it in + place of the real failure would hide the only error worth diagnosing. + Every other terminal failure is raised exactly as the server sent it, + first or last. + **Every exception this raises carries the key it sent** on ``.idempotency_key`` (and the server's ``.request_id`` when the response named one), so a caller who lost the response — a ``deadline_exceeded`` @@ -244,6 +300,11 @@ def run( # the wire, so everything legal in it is deep-copyable. payload = deepcopy(dict(arguments)) retrier = Retrier(self._retry, now=_now) + # The first retryable failure, kept so a key refusal on a later attempt + # cannot bury it. Only the first: every later one is the same call + # failing again, and the one the caller has to diagnose is the one that + # started the retrying. + first: BaseException | None = None # The key is stamped onto whatever this raises: it is a local of this # frame, so an exception that propagates past it would otherwise take # the caller's only route back to an already-billed generation with it. @@ -252,9 +313,18 @@ def run( try: return low.post_model_run(model, payload, idempotency_key=key, timeout=timeout) except _CANDIDATE_FAILURES as exc: + if first is not None and _is_key_reuse(exc): + # The resend could never have succeeded — the server + # refused the key, not the request — so raising it + # would replace the real failure with an artefact of + # this loop. Chained, not discarded: the 422 stays + # reachable on `__cause__` and in the traceback. + raise _as_sdk_error(first, key) from _as_sdk_error(exc, key) delay = retrier.delay_before_retry(exc) if delay is None: raise + if first is None: + first = exc time.sleep(delay) @@ -277,8 +347,9 @@ async def run( This *is* the async form of ``run``: awaiting it on ``AsyncComfy`` is the whole difference from the sync client — including the model-id - rule, the retry policy, the one-key-per-call rule, and the - ``.idempotency_key`` every exception it raises carries for the replay. + rule, the retry policy, the one-key-per-call rule, the failure raised + when a resend is refused for key reuse, and the ``.idempotency_key`` + every exception it raises carries for the replay. See :meth:`Models.run`. """ low = cast(AsyncComfyLow, self._low) @@ -293,6 +364,8 @@ async def run( # nested values included. payload = deepcopy(dict(arguments)) retrier = Retrier(self._retry, now=_now) + # The first retryable failure — see :meth:`Models.run`. + first: BaseException | None = None # The key is stamped onto whatever this raises: it is a local of this # frame, so an exception that propagates past it would otherwise take # the caller's only route back to an already-billed generation with it. @@ -303,7 +376,13 @@ async def run( model, payload, idempotency_key=key, timeout=timeout ) except _CANDIDATE_FAILURES as exc: + if first is not None and _is_key_reuse(exc): + # The rejected resend replaces nothing — see + # :meth:`Models.run`. + raise _as_sdk_error(first, key) from _as_sdk_error(exc, key) delay = retrier.delay_before_retry(exc) if delay is None: raise + if first is None: + first = exc await asyncio.sleep(delay) diff --git a/src/comfy_sdk/retry.py b/src/comfy_sdk/retry.py index 0b4ffdf..f58af79 100644 --- a/src/comfy_sdk/retry.py +++ b/src/comfy_sdk/retry.py @@ -81,11 +81,13 @@ on a run is the important member: the generation-sized client timeout expired with no answer, which is precisely when the server is most likely still generating). Nothing on the wire says the key survives this, so a same-key - retry may be a ``422`` that replaces the real error — and a *fresh*-key retry + retry may be refused ``422`` — and a *fresh*-key retry is the second billed generation this module exists to prevent. Not retried by default. :attr:`RetryPolicy.retry_possibly_in_flight` opts in, and it is correct exactly when a deployment replays a repeated key instead of rejecting - it. + it. That refusal no longer costs the caller the diagnosis: ``models.run`` + raises the failure that caused the retry and chains the ``422`` onto it as + ``__cause__``, so the real error is what surfaces. 5. **Everything else** — every other 4xx is the server's considered answer about *this* request, and asking again spends money to be refused again. Never retried. @@ -273,8 +275,9 @@ def is_unknown_outcome_status(status: int) -> bool: ``invalid_input``. That is why this class sits behind :attr:`RetryPolicy.retry_possibly_in_flight` rather than being retried by default: unless the deployment replays a claimed key, - the same-key retry cannot succeed and *replaces* the genuine 5xx with a - confusing key-reuse error. + the same-key retry cannot succeed at all. What it costs is one wasted + request rather than the diagnosis — ``models.run`` raises the genuine 5xx + and chains the key-reuse refusal onto it as ``__cause__``. A ``502``/``504`` from an intermediary belongs here for the same reason: the proxy's response completed, which says nothing about whether the origin @@ -421,7 +424,9 @@ class RetryPolicy: #: caution: a key whose recorded answer cannot be replayed is answered #: ``409`` ``invalid_input`` on the router surface (use a NEW key), and the #: v2 jobs contract makes ``Idempotency-Key`` single-use outright — so a - #: same-key retry here can surface a key refusal that hides the real error. + #: same-key retry here buys a key refusal rather than an answer. It no + #: longer hides the real error: ``models.run`` raises the failure that + #: caused the retry, with the refusal chained on as ``__cause__``. #: Turn it on for a deployment that replays a repeated #: key instead of rejecting it — and raise ``max_elapsed`` when you do, since #: one full-length client timeout on a run spends the whole default budget on diff --git a/tests/test_models_run_retry.py b/tests/test_models_run_retry.py index 7f646b5..cfb4207 100644 --- a/tests/test_models_run_retry.py +++ b/tests/test_models_run_retry.py @@ -1132,8 +1132,10 @@ def test_the_default_collect_loop_against_a_non_collecting_deployment(server) -> # rule is `spec/router-openapi.yaml`'s, so it binds Comfy Router -- but # `COMFY_ROUTER_BASE_URL` can name a deployment that applies the v2 rule # instead: key stays claimed across an unknown-outcome 5xx, and the resend - # is rejected. The caller then sees `422 - # idempotency_key_reuse` in place of the real 504. That is the trade the + # is rejected `422 idempotency_key_reuse`. What the caller sees is still the + # real 504: the refusal is an artefact of the collect loop, not an answer + # about the request, so it is chained onto the 504 as `__cause__` rather + # than raised in its place. The wasted second request is the trade the # default makes; `retry_collectable=False` is the way out of it, and this # pins both halves so neither can change silently. server.state.model_run_collects_after_deadline = False @@ -1141,9 +1143,15 @@ def test_the_default_collect_loop_against_a_non_collecting_deployment(server) -> server.state.model_run_error = (504, "deadline_exceeded") server.state.model_run_retry_after = "0" with Comfy(retry=FAST) as client: - with pytest.raises(IdempotencyKeyReuse): + with pytest.raises(ComfyError) as excinfo: client.models.run(MODEL, ARGS) assert server.state.model_run_count == 2 + assert excinfo.value.http_status == 504 + assert isinstance(excinfo.value.__cause__, IdempotencyKeyReuse) + # The substitution does not cost the caller the key: both halves of the + # chain carry the one key the call was made under. + assert excinfo.value.idempotency_key is not None + assert excinfo.value.__cause__.idempotency_key == excinfo.value.idempotency_key server.state.model_run_count = 0 server.state.model_run_idempotency.clear() @@ -1157,6 +1165,97 @@ def test_the_default_collect_loop_against_a_non_collecting_deployment(server) -> assert excinfo.value.http_status == 504 +# --- a rejected resend must not replace the failure that caused the retry --- + + +async def test_the_refused_collect_resend_is_chained_on_the_async_client(server) -> None: + # The async loop makes the same substitution as the sync one. The two loops + # are kept structurally identical, and only a test driven through + # `AsyncComfy` proves the second copy was edited too. + server.state.model_run_collects_after_deadline = False + server.state.model_run_v2_key_rule = True + server.state.model_run_error = (504, "deadline_exceeded") + server.state.model_run_retry_after = "0" + async with AsyncComfy(retry=FAST) as client: + with pytest.raises(ComfyError) as excinfo: + await client.models.run(MODEL, ARGS) + assert server.state.model_run_count == 2 + assert excinfo.value.http_status == 504 + assert isinstance(excinfo.value.__cause__, IdempotencyKeyReuse) + + +def test_the_opt_in_path_also_raises_the_5xx_that_caused_the_retry(server) -> None: + # The same substitution on the other route into it, which needs no collect + # rule at all: `retry_possibly_in_flight` resends a plain 500 under the one + # key, and a deployment applying the v2 rule refuses it 422. The 500 is the + # error the caller has to act on; the 422 only says the resend was pointless. + server.state.model_run_v2_key_rule = True + server.state.model_run_error = (500, "internal_error") + with Comfy(retry=FAST_OPTED_IN) as client: + with pytest.raises(ComfyError) as excinfo: + client.models.run(MODEL, ARGS) + assert server.state.model_run_count == 2 + assert excinfo.value.http_status == 500 + assert isinstance(excinfo.value.__cause__, IdempotencyKeyReuse) + # One key across both attempts, exactly as before: what changed is which + # exception is raised, never what goes on the wire. + assert len(set(server.state.model_run_idempotency_keys)) == 1 + + +def test_only_a_key_refusal_substitutes_and_every_other_last_failure_wins(server) -> None: + # The negative half. Last-wins is still the rule for everything but key + # reuse: a 5xx followed by a genuinely different terminal answer raises + # that answer, because a 404 *is* the server's verdict on this request. + # The deployment replays a repeated key, so the second attempt reaches the + # route rather than being refused for the key. + server.state.model_run_replays_idempotency_key = True + server.state.model_run_transient_error = (500, "internal_error") + server.state.model_run_fail_times = 1 + server.state.model_run_error = (404, "model_not_found") + with Comfy(retry=FAST_OPTED_IN) as client: + with pytest.raises(ComfyError) as excinfo: + client.models.run(MODEL, ARGS) + assert server.state.model_run_count == 2 + assert excinfo.value.http_status == 404 + assert not isinstance(excinfo.value.__cause__, IdempotencyKeyReuse) + + +def test_a_first_attempt_key_refusal_is_still_raised_as_itself(server) -> None: + # The substitution is conditional on there having *been* a retry. A caller + # presenting a key the server already consumed gets a straight answer about + # that key on the first attempt — there is no earlier failure to restore, + # and `except IdempotencyKeyReuse` still catches this one. + server.state.model_run_v2_key_rule = True + server.state.model_run_idempotency["already-consumed-key-1"] = "done" + with Comfy(retry=FAST) as client: + with pytest.raises(IdempotencyKeyReuse) as excinfo: + client.models.run(MODEL, ARGS, idempotency_key="already-consumed-key-1") + assert server.state.model_run_count == 1 + assert excinfo.value.http_status == 422 + + +def test_the_first_retryable_failure_is_the_one_kept() -> None: + # Only the *first* is remembered. Two different 5xx answers before the key + # refusal, and what surfaces is the one that started the retrying -- the + # later ones are the same call failing again, not new information. + + class _ThenRefusedLow(_FlakyLow): + def _attempt(self, arguments: Mapping[str, Any], key: str | None) -> dict[str, Any]: + self.keys.append(key) + if len(self.keys) == 1: + raise ApiError("first", code="internal_error", http_status=500) + if len(self.keys) == 2: + raise ApiError("second", code="internal_error", http_status=503) + raise ApiError("no", code="idempotency_key_reuse", http_status=422) + + low = _ThenRefusedLow() + with pytest.raises(ComfyError) as excinfo: + _models(low, FAST_OPTED_IN).run(MODEL, ARGS) + assert len(low.keys) == 3 + assert excinfo.value.http_status == 500 + assert isinstance(excinfo.value.__cause__, IdempotencyKeyReuse) + + # --- the body snapshot the same-key rule rests on ---