[PR #130] [MERGED] fix(models): raise the original failure when a same-key retry is refused 422 #131

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-python-sdk/pull/130
Author: @mattmillerai
Created: 9/5/2026
Status: ✅ Merged
Merged: 9/14/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-9949-run-raise-original-on-key-reuse

📝 Commits (3)

04af794 fix(models): raise the original failure when a same-key retry is refused 422
999db21 fix(models): only let a claim-capable failure suppress a key refusal (BE-9949)
8dae76a Merge remote-tracking branch 'origin/main' into matt/be-9949-run-raise-original-on-key-reuse

📊 Changes

6 files changed (+502 additions, -16 deletions)

View changed files

📝 CHANGELOG.md (+39 -0)
📝 README.md (+14 -4)
📝 src/comfy_sdk/exceptions.py (+24 -0)
📝 src/comfy_sdk/models.py (+139 -4)
📝 src/comfy_sdk/retry.py (+45 -5)
📝 tests/test_models_run_retry.py (+241 -3)

📄 Description

ELI-5

client.models.run() retries some failures by re-sending the request under the same Idempotency-Key. Against a deployment that treats a key as single-use, that re-send comes back 422 idempotency_key_reuse — and until now that refusal was the exception the caller got, throwing away the 504/500 that caused the retry in the first place. The refusal only says "you asked twice"; it says nothing about why the call failed. Now run raises the original failure and hangs the 422 off it as __cause__, so nothing is lost and the useful error is the one you see.

What changed

Both retry loops in src/comfy_sdk/models.py (sync and async, kept structurally identical) now remember the first retryable failure. If a later attempt is refused for key reuse, that first failure is raised with the refusal chained on:

except _CANDIDATE_FAILURES as exc:
    if first is not None and _is_key_reuse(exc):
        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)

_is_key_reuse matches the wire facts (http_status == 422 and code == "idempotency_key_reuse") and also accepts either layer's typed IdempotencyKeyReuse outright, so the answer does not depend on which layer raised.
The loop runs inside translating(), which converts and stamps only the exception it is handed — an ApiError chained on as __cause__ would have reached the caller as a raw protocol type. _as_sdk_error therefore translates and stamps both halves before the raise, rather than changing translating()'s contract for its other 20 call sites. to_sdk_error is already called directly this way in jobs.py and client.py.
Each translated half inherits the original's traceback, so the chain points at the attempt that produced it. Verified by hand:

comfy_sdk.exceptions.IdempotencyKeyReuse: key already used
  ... models.py line 314, in run  ->  low.post_model_run(...)

The above exception was the direct cause of the following exception:

comfy_sdk.router_exceptions.DeadlineExceeded: router deadline
  ... models.py line 322, in run  ->  raise _as_sdk_error(first, key) from _as_sdk_error(exc, key)
  ... models.py line 314, in run  ->  low.post_model_run(...)

Both halves carry the call's Idempotency-Key, so the replay idiom still works off either.

Only key reuse substitutes, and only after there has already been a retry. Nothing about which failures are retried changed: _is_key_reuse fires only on a 422, which RetryPolicy.should_retry already answered False for, so short-circuiting delay_before_retry cannot change any retry decision.

Docs updated to match: the retry_possibly_in_flight docstring, the two module-docstring sentences in retry.py that asserted the old behaviour ("a 422 that replaces the real error", "replaces the genuine 5xx"), the Models.run / AsyncModels.run docstrings, three stale README passages, and a ### Changed entry in CHANGELOG.md flagging the behaviour change for except IdempotencyKeyReuse callers.

Behaviour change

except IdempotencyKeyReuse around models.run no longer catches the rejected-resend case. Catch the failure you care about (or ComfyError) and inspect exc.__cause__. A key refusal on the first attempt — a caller passing a key the server already consumed — is unchanged and still raises IdempotencyKeyReuse directly; a test pins that.

No capability is removed: the 422 remains fully reachable (type, status, message, key, traceback) via __cause__, and three tests assert it is there rather than merely asserting it is gone.

Tests

tests/test_models_run_retry.py, one updated and five added:

Updated test_the_default_collect_loop_against_a_non_collecting_deployment — it pinned pytest.raises(IdempotencyKeyReuse), which was the bug. Now asserts ComfyError with http_status == 504, isinstance(exc.__cause__, IdempotencyKeyReuse), both halves carrying the same key, and model_run_count == 2. The retry_collectable=False half is unchanged.
Async mirror of (1) through AsyncComfy, which is what proves the second loop was edited too.
Opt-in path: persistent 500 under retry_possibly_in_flight=True against a key-rejecting deployment — http_status == 500, __cause__ is IdempotencyKeyReuse, model_run_count == 2, one key on the wire.
Negative: a 500 followed by a 404 still raises the 404 — last-wins survives for everything but key reuse.
A first-attempt key refusal is still raised as itself.
Only the first retryable failure is kept: 500 → 503 → 422 surfaces the 500.

tests/conftest.py needed no change — ServerState.model_run_v2_key_rule already answers a repeated key 422 idempotency_key_reuse, and model_run_collects_after_deadline already models the non-collecting deployment.

tests/test_sync_async_parity.py was checked for a loop-source-shape assertion: it has none (it compares namespace/method names, kinds and signatures), so nothing there needed updating, and both loops are byte-for-byte identical apart from await / asyncio.sleep.

Provenance

Authored by: agent-work loop
Verified: uv run --extra dev ruff check . clean; ruff format --check . 51 files already formatted; mypy src no issues in 19 source files; uv run --extra dev pytest -q 723 passed / 4 skipped (the skips are the network-gated tests/integration/test_gateway_e2e.py); python3 scripts/check_public_repo_hygiene.py OK; plus a hand-run repro printing the chained traceback shown above.
Deviations: the task described the retry.py module-docstring sentence at lines 33/54/72; on current main that sentence lives at line 84, and two further sentences asserting the old behaviour (in is_unknown_outcome_status and the retry_possibly_in_flight field doc) were updated alongside it. Three README passages also asserted the old behaviour and were corrected, which was not in the described scope but would otherwise have shipped stale.

Residual

Comfy.submit / AsyncComfy.submit were not changed and not exercised for this defect. They carry a structurally similar retry loop (src/comfy_sdk/client.py, _retry_delay) that re-sends under one Idempotency-Key and re-raises the last attempt's error. Today it retries only a 429 carrying Retry-After, which the v2 contract says releases the key, so a same-key resend there should not meet a 422 — the exposure is theoretical rather than demonstrated, and I did not build a repro for it. If a deployment is found that keeps a key claimed across a 429, submit has exactly the bug this PR fixes in models.run and needs the same treatment.
_stamp is imported across modules from comfy_sdk.exceptions into comfy_sdk.models (same package, private name). The alternative was widening translating() to translate an explicit __cause__ chain, which would have altered a helper used by ~20 call sites to serve one of them. Worth a reviewer's opinion; if the private import is unwanted, moving the translate-and-stamp pair into exceptions.py as a named public helper is a mechanical follow-up.
Source material I could not read. This work derives from a read-only investigation recorded in the issue tracker; its findings comment and the linked spike are not reachable from the environment this PR was written in, so the implementation was built from the description of the defect rather than from that evidence directly. Everything asserted here was re-verified against the code and the test stub. Two pull requests were named as sequencing dependencies — the router-deadline 504 same-key-retry work (#99) and the exception key/request-id work (#97); both are merged, so this branches from main and no stacking or rebase was needed, and no residual note could be added to either since neither is open.

Summary by CodeRabbit

Bug Fixes
- Model runs now preserve the most recent eligible failure when a same-key retry is rejected due to idempotency-key reuse.
- The rejection remains available as the underlying cause, with a resend_refused indicator for clearer diagnosis.
- Behavior is consistent across synchronous and asynchronous runs, including gateway and server retries.
New Features
- Added a helper for custom retry logic to identify failures that may have claimed an idempotency key.
Documentation
- Updated retry and typed-error documentation to explain these behaviors.

_{🔄 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/130 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/5/2026 **Status:** ✅ Merged **Merged:** 9/14/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-9949-run-raise-original-on-key-reuse` --- ### 📝 Commits (3) - [`04af794`](https://github.com/Comfy-Org/comfy-python-sdk/commit/04af79412a1173e0e501c62703039b800b03638e) fix(models): raise the original failure when a same-key retry is refused 422 - [`999db21`](https://github.com/Comfy-Org/comfy-python-sdk/commit/999db21923e3c7aad82bd4c38aa89825cbb69094) fix(models): only let a claim-capable failure suppress a key refusal (BE-9949) - [`8dae76a`](https://github.com/Comfy-Org/comfy-python-sdk/commit/8dae76a98a4550abe4d7fe09a393ad254b67416d) Merge remote-tracking branch 'origin/main' into matt/be-9949-run-raise-original-on-key-reuse ### 📊 Changes **6 files changed** (+502 additions, -16 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+39 -0) 📝 `README.md` (+14 -4) 📝 `src/comfy_sdk/exceptions.py` (+24 -0) 📝 `src/comfy_sdk/models.py` (+139 -4) 📝 `src/comfy_sdk/retry.py` (+45 -5) 📝 `tests/test_models_run_retry.py` (+241 -3) </details> ### 📄 Description ## ELI-5 `client.models.run()` retries some failures by re-sending the request under the *same* `Idempotency-Key`. Against a deployment that treats a key as single-use, that re-send comes back `422 idempotency_key_reuse` — and until now that refusal was the exception the caller got, throwing away the `504`/`500` that caused the retry in the first place. The refusal only says "you asked twice"; it says nothing about why the call failed. Now `run` raises the original failure and hangs the `422` off it as `__cause__`, so nothing is lost and the useful error is the one you see. ## What changed Both retry loops in `src/comfy_sdk/models.py` (sync and async, kept structurally identical) now remember the **first** retryable failure. If a later attempt is refused for key reuse, that first failure is raised with the refusal chained on: ```python except _CANDIDATE_FAILURES as exc: if first is not None and _is_key_reuse(exc): 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) ``` - `_is_key_reuse` matches the wire facts (`http_status == 422` **and** `code == "idempotency_key_reuse"`) and also accepts either layer's typed `IdempotencyKeyReuse` outright, so the answer does not depend on which layer raised. - The loop runs inside `translating()`, which converts and stamps only the exception it is *handed* — an `ApiError` chained on as `__cause__` would have reached the caller as a raw protocol type. `_as_sdk_error` therefore translates and stamps **both** halves before the raise, rather than changing `translating()`'s contract for its other 20 call sites. `to_sdk_error` is already called directly this way in `jobs.py` and `client.py`. - Each translated half inherits the original's traceback, so the chain points at the attempt that produced it. Verified by hand: ``` comfy_sdk.exceptions.IdempotencyKeyReuse: key already used ... models.py line 314, in run -> low.post_model_run(...) The above exception was the direct cause of the following exception: comfy_sdk.router_exceptions.DeadlineExceeded: router deadline ... models.py line 322, in run -> raise _as_sdk_error(first, key) from _as_sdk_error(exc, key) ... models.py line 314, in run -> low.post_model_run(...) ``` Both halves carry the call's `Idempotency-Key`, so the replay idiom still works off either. **Only** key reuse substitutes, and **only** after there has already been a retry. Nothing about *which* failures are retried changed: `_is_key_reuse` fires only on a `422`, which `RetryPolicy.should_retry` already answered `False` for, so short-circuiting `delay_before_retry` cannot change any retry decision. Docs updated to match: the `retry_possibly_in_flight` docstring, the two module-docstring sentences in `retry.py` that asserted the old behaviour ("a `422` that replaces the real error", "*replaces* the genuine 5xx"), the `Models.run` / `AsyncModels.run` docstrings, three stale README passages, and a `### Changed` entry in `CHANGELOG.md` flagging the behaviour change for `except IdempotencyKeyReuse` callers. ## Behaviour change `except IdempotencyKeyReuse` around `models.run` no longer catches the **rejected-resend** case. Catch the failure you care about (or `ComfyError`) and inspect `exc.__cause__`. A key refusal on the **first** attempt — a caller passing a key the server already consumed — is unchanged and still raises `IdempotencyKeyReuse` directly; a test pins that. No capability is removed: the `422` remains fully reachable (type, status, message, key, traceback) via `__cause__`, and three tests assert it is there rather than merely asserting it is gone. ## Tests `tests/test_models_run_retry.py`, one updated and five added: 1. **Updated** `test_the_default_collect_loop_against_a_non_collecting_deployment` — it pinned `pytest.raises(IdempotencyKeyReuse)`, which was the bug. Now asserts `ComfyError` with `http_status == 504`, `isinstance(exc.__cause__, IdempotencyKeyReuse)`, both halves carrying the same key, and `model_run_count == 2`. The `retry_collectable=False` half is unchanged. 2. Async mirror of (1) through `AsyncComfy`, which is what proves the second loop was edited too. 3. Opt-in path: persistent `500` under `retry_possibly_in_flight=True` against a key-rejecting deployment — `http_status == 500`, `__cause__` is `IdempotencyKeyReuse`, `model_run_count == 2`, one key on the wire. 4. Negative: a `500` followed by a `404` still raises the `404` — last-wins survives for everything but key reuse. 5. A first-attempt key refusal is still raised as itself. 6. Only the *first* retryable failure is kept: `500` → `503` → `422` surfaces the `500`. `tests/conftest.py` needed no change — `ServerState.model_run_v2_key_rule` already answers a repeated key `422 idempotency_key_reuse`, and `model_run_collects_after_deadline` already models the non-collecting deployment. `tests/test_sync_async_parity.py` was checked for a loop-source-shape assertion: it has none (it compares namespace/method names, kinds and signatures), so nothing there needed updating, and both loops are byte-for-byte identical apart from `await` / `asyncio.sleep`. ## Provenance - **Authored by:** agent-work loop - **Verified:** `uv run --extra dev ruff check .` clean; `ruff format --check .` 51 files already formatted; `mypy src` no issues in 19 source files; `uv run --extra dev pytest -q` 723 passed / 4 skipped (the skips are the network-gated `tests/integration/test_gateway_e2e.py`); `python3 scripts/check_public_repo_hygiene.py` OK; plus a hand-run repro printing the chained traceback shown above. - **Deviations:** the task described the `retry.py` module-docstring sentence at lines 33/54/72; on current `main` that sentence lives at line 84, and two further sentences asserting the old behaviour (in `is_unknown_outcome_status` and the `retry_possibly_in_flight` field doc) were updated alongside it. Three README passages also asserted the old behaviour and were corrected, which was not in the described scope but would otherwise have shipped stale. ## Residual - **`Comfy.submit` / `AsyncComfy.submit` were not changed and not exercised for this defect.** They carry a structurally similar retry loop (`src/comfy_sdk/client.py`, `_retry_delay`) that re-sends under one `Idempotency-Key` and re-raises the last attempt's error. Today it retries only a `429` carrying `Retry-After`, which the v2 contract says *releases* the key, so a same-key resend there should not meet a `422` — the exposure is theoretical rather than demonstrated, and I did not build a repro for it. If a deployment is found that keeps a key claimed across a `429`, `submit` has exactly the bug this PR fixes in `models.run` and needs the same treatment. - **`_stamp` is imported across modules from `comfy_sdk.exceptions` into `comfy_sdk.models`** (same package, private name). The alternative was widening `translating()` to translate an explicit `__cause__` chain, which would have altered a helper used by ~20 call sites to serve one of them. Worth a reviewer's opinion; if the private import is unwanted, moving the translate-and-stamp pair into `exceptions.py` as a named public helper is a mechanical follow-up. - **Source material I could not read.** This work derives from a read-only investigation recorded in the issue tracker; its findings comment and the linked spike are not reachable from the environment this PR was written in, so the implementation was built from the description of the defect rather than from that evidence directly. Everything asserted here was re-verified against the code and the test stub. Two pull requests were named as sequencing dependencies — the router-deadline `504` same-key-retry work (#99) and the exception key/request-id work (#97); both are merged, so this branches from `main` and no stacking or rebase was needed, and no residual note could be added to either since neither is open.  ## Summary by CodeRabbit * **Bug Fixes** * Model runs now preserve the most recent eligible failure when a same-key retry is rejected due to idempotency-key reuse. * The rejection remains available as the underlying cause, with a `resend_refused` indicator for clearer diagnosis. * Behavior is consistent across synchronous and asynchronous runs, including gateway and server retries. * **New Features** * Added a helper for custom retry logic to identify failures that may have claimed an idempotency key. * **Documentation** * Updated retry and typed-error documentation to explain these behaviors.  --- <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:39 +00:00