fix(models): raise the original failure when a same-key retry is refused 422 - #130
fix(models): raise the original failure when a same-key retry is refused 422#130mattmillerai wants to merge 1 commit into
Conversation
…sed 422 `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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (5)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. 📝 WalkthroughWalkthroughThe SDK now preserves the original retry-triggering failure when a repeated idempotency key is rejected. It chains ChangesRetry error propagation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to Model retries now retain and raise the original retryable failure when a repeated idempotency key is refused, with the refusal chained as its cause. The documented behavior and covered retry cases indicate the change is ready to merge. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 41.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 3 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 8 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 2 |
| 🟡 Medium | 2 |
| 🟢 Low | 2 |
| ⚪ Nit | 2 |
Panel: 6/6 reviewers contributed findings.
| 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): |
There was a problem hiding this comment.
🟠 High — first is not None only proves a retry happened, not that this loop claimed the key: a prior never-delivered transport failure (ConnectError, ConnectTimeout, PoolTimeout, ProxyError) or a paced 429 explicitly releases or never claims it, so a following 422 idempotency_key_reuse is a genuine refusal of an already-consumed caller-supplied key — yet it is demoted to __cause__ while the transient error is raised, and a caller whose wrapper retries that transport error loops forever on a key that can never succeed. Gate the substitution on the retained failure being one that could actually have claimed the key (unknown-outcome/collectable). Same guard at line 379 in AsyncModels.run.
Raised by 4 of 6 reviewers (gpt-5.6-sol-max adversarial, gpt-5.6-sol-max edge-case, claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case).
| delay = retrier.delay_before_retry(exc) | ||
| if delay is None: | ||
| raise | ||
| if first is None: |
There was a problem hiding this comment.
🟠 High — First-wins keeps the wrong failure in mixed sequences. Under the default policy 429 (key released, no work started) → 504 deadline_exceeded (a generation is now held under the key) → 422 raises QueueFull, whose semantics contradict the chained 422 and tell the caller nothing was started — inviting a fresh-key retry, i.e. the second billed generation. Retaining the most recent claim-capable failure instead of the very first would make the raised error describe the server state the 422 implies. The async loop retains identically at line 386.
Raised by 3 of 6 reviewers (gpt-5.6-sol-max adversarial, gpt-5.6-sol-max edge-case, kimi-k3-high edge-case).
| if isinstance(exc, (IdempotencyKeyReuse, ProtocolIdempotencyKeyReuse)): | ||
| return True | ||
| return ( | ||
| getattr(exc, "http_status", None) == _KEY_REUSE_STATUS |
There was a problem hiding this comment.
🟡 Medium — _is_key_reuse matches only 422/idempotency_key_reuse, but on the default Router surface a consumed, non-replayable key is refused 409 invalid_input — as retry.py's own note in this diff states — and that terminal 409 still propagates in place of the real 500/504. The new prose in retry.py, the README and the run docstring promising that a key refusal "no longer hides the real error" therefore does not hold for the default deployment; either widen the match to the router's 409 key-refusal case or scope the claim to v2-rule deployments. Same for the async path.
Raised by 2 of 6 reviewers (claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case).
| # 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) |
There was a problem hiding this comment.
🟡 Medium — Raising the 5xx in place of the terminal 422 turns an unambiguously non-retryable answer into one that generic outer retry wrappers (tenacity, hand-rolled except ComfyError keyed on http_status >= 500) will retry, and every re-entry into run() mints a fresh Idempotency-Key — the duplicate billed generation this module exists to prevent, since a wrapper that reads only http_status never inspects __cause__. Consider marking the raised error (e.g. a resend_refused attribute) so wrappers can tell it apart. Same at line 382 in the async loop.
Raised by 1 of 6 reviewers (claude-opus-5-thinking-max adversarial).
| of this module that re-raised them. | ||
| """ | ||
| if not isinstance(exc, ApiError): | ||
| return _stamp(exc, idempotency_key) |
There was a problem hiding this comment.
🟢 Low — When the retained failure is not an ApiError (an httpx.ReadTimeout under retry_possibly_in_flight=True, or a default-retried ConnectError), _as_sdk_error only stamps it, so this path can raise a bare httpx exception where the caller previously received an IdempotencyKeyReuse. If translating() does not itself convert httpx failures, an except ComfyError handler that used to catch this case stops firing — worth a test covering a transport-error first failure followed by the 422.
Raised by 1 of 6 reviewers (claude-opus-5-thinking-max edge-case).
| **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 |
There was a problem hiding this comment.
🟢 Low — The re-raised first failure keeps its original retry_after, and for deadline_exceeded 504 the documented advice is to retry with the same key — advice this loop has already disproved by having the resend refused. The docstring (and the README's collect-after-a-lost-response recipe) should say that a 504 arriving with an IdempotencyKeyReuse on __cause__ means stop resending rather than follow retry_after.
Raised by 1 of 6 reviewers (claude-opus-5-thinking-max edge-case).
| # 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) |
There was a problem hiding this comment.
⚪ Nit — raise X from Y executes inside the except ... as exc handler, so Python also sets __context__ on the substituted exception to the raw comfy_low.errors.ApiError still being handled — precisely the protocol-layer type _as_sdk_error exists to keep off the caller's surface for anything walking the full chain. Clearing __context__ on the substituted exception, or re-raising outside the handler, closes it.
Raised by 1 of 6 reviewers (claude-opus-5-thinking-max adversarial).
| layer's typed class is accepted outright so the answer does not depend on | ||
| which one raised. | ||
| """ | ||
| if isinstance(exc, (IdempotencyKeyReuse, ProtocolIdempotencyKeyReuse)): |
There was a problem hiding this comment.
⚪ Nit — The SDK-level IdempotencyKeyReuse arm of this isinstance check is unreachable: _CANDIDATE_FAILURES is (ApiError, RouterError, httpx.TransportError) and comfy_sdk.exceptions.IdempotencyKeyReuse derives from ComfyError only, so the except clause that calls this can never bind one. The docstring's "either layer's typed class is accepted" reads as if both can arrive here; drop the arm or note it as deliberately defensive.
Raised by 1 of 6 reviewers (claude-opus-5-thinking-max edge-case).
1 participant
ELI-5
client.models.run()retries some failures by re-sending the request under the sameIdempotency-Key. Against a deployment that treats a key as single-use, that re-send comes back422 idempotency_key_reuse— and until now that refusal was the exception the caller got, throwing away the504/500that caused the retry in the first place. The refusal only says "you asked twice"; it says nothing about why the call failed. Nowrunraises the original failure and hangs the422off 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:_is_key_reusematches the wire facts (http_status == 422andcode == "idempotency_key_reuse") and also accepts either layer's typedIdempotencyKeyReuseoutright, so the answer does not depend on which layer raised.translating(), which converts and stamps only the exception it is handed — anApiErrorchained on as__cause__would have reached the caller as a raw protocol type._as_sdk_errortherefore translates and stamps both halves before the raise, rather than changingtranslating()'s contract for its other 20 call sites.to_sdk_erroris already called directly this way injobs.pyandclient.py.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_reusefires only on a422, whichRetryPolicy.should_retryalready answeredFalsefor, so short-circuitingdelay_before_retrycannot change any retry decision.Docs updated to match: the
retry_possibly_in_flightdocstring, the two module-docstring sentences inretry.pythat asserted the old behaviour ("a422that replaces the real error", "replaces the genuine 5xx"), theModels.run/AsyncModels.rundocstrings, three stale README passages, and a### Changedentry inCHANGELOG.mdflagging the behaviour change forexcept IdempotencyKeyReusecallers.Behaviour change
except IdempotencyKeyReusearoundmodels.runno longer catches the rejected-resend case. Catch the failure you care about (orComfyError) and inspectexc.__cause__. A key refusal on the first attempt — a caller passing a key the server already consumed — is unchanged and still raisesIdempotencyKeyReusedirectly; a test pins that.No capability is removed: the
422remains 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:test_the_default_collect_loop_against_a_non_collecting_deployment— it pinnedpytest.raises(IdempotencyKeyReuse), which was the bug. Now assertsComfyErrorwithhttp_status == 504,isinstance(exc.__cause__, IdempotencyKeyReuse), both halves carrying the same key, andmodel_run_count == 2. Theretry_collectable=Falsehalf is unchanged.AsyncComfy, which is what proves the second loop was edited too.500underretry_possibly_in_flight=Trueagainst a key-rejecting deployment —http_status == 500,__cause__isIdempotencyKeyReuse,model_run_count == 2, one key on the wire.500followed by a404still raises the404— last-wins survives for everything but key reuse.500→503→422surfaces the500.tests/conftest.pyneeded no change —ServerState.model_run_v2_key_rulealready answers a repeated key422 idempotency_key_reuse, andmodel_run_collects_after_deadlinealready models the non-collecting deployment.tests/test_sync_async_parity.pywas 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 fromawait/asyncio.sleep.Provenance
uv run --extra dev ruff check .clean;ruff format --check .51 files already formatted;mypy srcno issues in 19 source files;uv run --extra dev pytest -q723 passed / 4 skipped (the skips are the network-gatedtests/integration/test_gateway_e2e.py);python3 scripts/check_public_repo_hygiene.pyOK; plus a hand-run repro printing the chained traceback shown above.retry.pymodule-docstring sentence at lines 33/54/72; on currentmainthat sentence lives at line 84, and two further sentences asserting the old behaviour (inis_unknown_outcome_statusand theretry_possibly_in_flightfield 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.submitwere 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 oneIdempotency-Keyand re-raises the last attempt's error. Today it retries only a429carryingRetry-After, which the v2 contract says releases the key, so a same-key resend there should not meet a422— 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 a429,submithas exactly the bug this PR fixes inmodels.runand needs the same treatment._stampis imported across modules fromcomfy_sdk.exceptionsintocomfy_sdk.models(same package, private name). The alternative was wideningtranslating()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 intoexceptions.pyas a named public helper is a mechanical follow-up.504same-key-retry work (feat: collect a Router deadline 504 under the same Idempotency-Key by default #99) and the exception key/request-id work (feat: carry the Idempotency-Key and request id on every exception models.run raises #97); both are merged, so this branches frommainand no stacking or rebase was needed, and no residual note could be added to either since neither is open.Summary by CodeRabbit
Bug Fixes
Documentation