Comfy-Org/comfy-python-sdk

Fork 0

mirror of https://github.com/Comfy-Org/comfy-python-sdk.git synced 2026-11-04 23:37:47 +00:00

[PR #97] [MERGED] feat: carry the Idempotency-Key and request id on every exception models.run raises #98

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-python-sdk/pull/97
Author: @mattmillerai
Created: 8/27/2026
Status: ✅ Merged
Merged: 8/27/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-9834-run-exception-idempotency-key

📝 Commits (4)

0742a44 feat: carry the Idempotency-Key and request id on every exception models.run raises
7c2b0bf fix: address the review panel on the exception idempotency-key surface
c4d757e docs: scope the changelog claim to failed calls rather than every exception
11a9f60 Merge origin/main into matt/be-9834-run-exception-idempotency-key

📊 Changes

10 files changed (+940 additions, -16 deletions)

View changed files

📝 CHANGELOG.md (+44 -0)
📝 README.md (+58 -0)
📝 src/comfy_low/errors.py (+35 -0)
📝 src/comfy_low/transport.py (+38 -3)
📝 src/comfy_sdk/exceptions.py (+120 -3)
📝 src/comfy_sdk/models.py (+35 -4)
📝 src/comfy_sdk/router_exceptions.py (+6 -1)
📝 tests/conftest.py (+62 -1)
📝 tests/test_error_contract.py (+188 -1)
📝 tests/test_models_run.py (+354 -3)

📄 Description

ELI-5

When you call client.models.run(...), the SDK quietly makes up a one-time Idempotency-Key and sends it with the request. That key is your receipt: if the answer never comes back — the server gives up holding the connection and returns a 504, or your connection drops mid-generation — the generation may well have finished and been billed, and asking again under the same key is how you collect it. The problem: you never saw the key. It lived in a local variable inside run(), and when run() raised, it went away with the call. So the receipt was shredded at exactly the moment you needed it, and your paid-for generation was unreachable. Only people who had passed their own idempotency_key= and written it down somewhere could recover.

Now every exception run() raises hands you the receipt back, on exc.idempotency_key. Re-run with idempotency_key=exc.idempotency_key and you get your generation instead of paying for a second one.

What changed

comfy_sdk.exceptions.translating() takes an optional idempotency_key=, and stamps it onto whatever leaves the block. Both Models.run and AsyncModels.run pass the key they minted. That is the whole mechanism.

Stamping at the translation boundary rather than in each exception's constructor is the load-bearing choice, and it buys three things a per-subclass approach would miss:

an error_type this SDK version has never heard of falls through to the bare RouterError base — and still carries the key;
a transport failure has no response to translate at all (httpx.ConnectError, a read timeout on a run the server is probably still generating), and it is exactly the "lost the response" case the recovery contract is about — it carries the key too;
there is one place to read, not fifteen constructors to keep in step.

_STAMPABLE is (ComfyError, httpx.HTTPError, asyncio.CancelledError) — deliberately not "everything". A ZeroDivisionError escaping the block is a bug in the SDK or in caller code, not a failed call, and a key means nothing on it; the same reasoning models._CANDIDATE_FAILURES already documents for the retry classifier. It also means KeyboardInterrupt is never touched. Tested both ways.

asyncio.CancelledError is the one BaseException in that tuple, added in review: cancelling an in-flight AsyncModels.run — which is what the asyncio.wait_for a caller wraps a ten-minute call in does — abandons a generation that may already be dispatched and billed, and that is exactly the case the key exists to collect. It is re-raised bare like the rest of that branch, so nothing is swallowed and the cancellation still propagates.

request_id rides along for a closely related reason. X-Comfy-Request-Id was read off a router error response but dropped on the floor by the shared error envelope, so the id a user quotes in a support request was unreachable once the response object was gone. parse_or_raise now reads it, ApiError carries it beside retry_after (same pattern, same reason), and to_sdk_error passes it through. RouterError's existing request_id is untouched — the base now just declares the same attribute so it is uniform.

retry_after joined them in review. to_sdk_error had been dropping ApiError.retry_after for every code except queue_full, which left the flow this PR documents incoherent: the docs tell a caller to replay after the pace the server named, and a deadline_exceeded 504 carrying Retry-After: 2 had nowhere to surface it. ComfyError now carries it for every code. This does not touch the retry path — RetryPolicy.should_retry and client._retry_delay both read the protocol-level ApiError before translation, so what to_sdk_error produces has never fed a retry decision.

All three attributes are declared on ComfyError with a None default, and _stamp defaults the readable ones onto exceptions this SDK does not build (httpx's, CancelledError) without ever overwriting a real value — so exc.request_id on a dropped connection reads None rather than raising AttributeError, and no caller is forced into getattr(exc, ..., None).

Additive, verified as such

translating() with no key behaves exactly as it did before the parameter existed. The new except _STAMPABLE branch does a bare raise, which preserves the original traceback and exception identity; contextlib sees exc is value and does not suppress. Pinned by a test.
Nothing changes about what is retried — the retry policy is untouched.
Nothing changes on the wire — no new header, no changed header, no changed body. The existing wire assertions in tests/test_models_run.py and tests/test_models_run_retry.py still pass unchanged.
No public name was removed or repurposed. tests/test_error_contract.py now pins the base error's whole attribute surface as a written-out list, so adding one is a deliberate edit to a reviewable list and removing one is a failing test rather than a silent break of somebody's except block.

Tests

New coverage in tests/test_models_run.py (sync and async throughout):

a 504 deadline_exceeded from run() exposes the exact auto-minted key — compared against the key the stub server actually received, not merely asserted truthy, because a freshly minted key is precisely the value that cannot collect the generation;
a caller-supplied key round-trips onto the exception;
a transport-level failure (httpx.ConnectError, httpx.ReadTimeout) carries the key;
a real client-side timeout against the live stub server carries the key;
an unrecognised error_type lands on the base RouterError with the key, and a typed subclass does too;
when retries exhaust, the key on the exception is the one every attempt reused — not the first attempt's, not a fresh one;
end to end: the docstring's recovery idiom actually collects the generation against a stub configured as a key-replaying deployment;
request_id present when the response carried the header, None when it did not, independent of the key.

tests/conftest.py gains a model_run_request_id knob so the stub can emit X-Comfy-Request-Id on a failed run. No httpx mocking — server.state only, per the repo's convention.

tests/test_error_contract.py gains the attribute-surface pins described above, translating()'s stamp behaviour with and without a key, the not-stamped bug case, and a guard that comfy_low and comfy_sdk spell X-Comfy-Request-Id identically (each layer needs its own copy — comfy_low never imports comfy_sdk — so a drift would silently drop the id on one of the two surfaces).

Review round (`7c2b0bf`, `c4d757e`)

Ten findings from the review panel, all addressed. Eight in code, two answered:

The README recovery snippet caught only ComfyError — the high-severity one, and correct: a dropped connection or read timeout is one of the two headline cases the section exists for, and it arrives as an httpx error with no response to translate. A reader copying that snippet never reached the replay for exactly the failure it is about. Now except (ComfyError, httpx.HTTPError), and it checks the key for None before passing it back.
_stamp wrote only idempotency_key, so exc.request_id on an httpx failure was an AttributeError rather than the documented None — on precisely the no-response failures the pair matters most on. It now defaults the whole readable surface, pinned against the base's attribute list so a fourth attribute cannot be added without it.
to_sdk_error dropped Retry-After for every code but queue_full — described above.
The ok path called resp.json() unguarded, so a 200 with a non-JSON body (a proxy interstitial, a truncated response) escaped as json.JSONDecodeError from outside the translated surface, and so unstamped. On models.run that is a generation that ran and was billed with the result lost — the exact failure the key has to ride out on. It is now an ApiError(code="invalid_response") raised at the decode site, which also gives it a request_id.
asyncio.CancelledError passed unstamped — described above.
X-Comfy-Request-Id was stored verbatim from a server-controlled header, with no length bound and no filtering, and the docs tell users to display it and paste it into support tickets. Now bounded and filtered through a single clean_request_id in comfy_low.errors that both error surfaces call, so an id cannot be safe to display on one and not the other. It matches the leading run of allowed characters rather than deleting disallowed ones, which gives the right answer for the duplicate-header join httpx.Headers.get performs: "a1, a2" yields "a1", not a spliced "a1a2" that identifies no call at all.
The end-to-end replay test did not actually exercise replay. The old stub skipped claiming the key on the 5xx but stored no prior result, so the second same-key request re-ran the model and the payload comparison passed for a brand-new second generation — the double charge the feature prevents, indistinguishable from a real replay. The stub now models a lost response properly (records the result against the key, serves it back without re-running) behind a separate model_run_replays_lost_result knob, so the existing retry tests keep the old flag's narrower meaning. The assertion is now one generation across two requests, with the failure knob deliberately left set so the second call succeeds because the key collected the recording, not because the error was switched off.
The run docstring named a 409 "still in progress" refusal the mapping does not deliver: _CODE_BY_STATUS maps a body-less 409 to hash_mismatch, so a caller would read a bytes-integrity failure. The docstring no longer promises a class and points at exc.retry_after instead. Fixing the mapping is deferred — see Residual.

Answered rather than changed:

A sentinel default so an explicit idempotency_key=None is rejected. Declined: None is the documented default meaning "mint one", so run(m, a, idempotency_key=maybe_key) would start raising with no deprecation. The underlying footgun is real, so the idiom changed instead — the snippet and docstring both say to check for None first, and test_replaying_without_the_key_would_start_a_second_generation asserts the outcome directly so the guard is not silently dropped later.
Stamping submit()'s errors too. Out of scope here and filed separately — see Residual.

Every one of the ten threads has a reply explaining what was done and why, and is resolved.

CodeRabbit then raised one more against the changelog, and was right: "every exception" was literally overbroad. _STAMPABLE is deliberately not "everything" — a programming error escaping the call reaches the caller untouched, and KeyboardInterrupt is never touched either, both with tests asserting it. The entry now names the boundary as the failed call rather than the exception type, which is the property the design actually holds and the one that stays true if another call-failure type is added later. Fixed in c4d757e; thread resolved.

Judgment calls

"Every exception" is scoped to failures of the call. The acceptance wording says every exception, then enumerates router errors, ComfyError, and transport errors — which is what _STAMPABLE implements. A programming error propagates unstamped, on purpose, and there is a test asserting that. Flagging it because it is a readable-two-ways criterion, not because I think the other reading is right.
request_id was added at parse_or_raise, which every transport method shares — not just the model-run path. It is one line and purely additive (a new attribute defaulting to None), and scoping it to one route would have meant the same header meaning two different things on two surfaces.
The base-RouterError-carries-the-key assertion lives in tests/test_models_run.py and tests/test_error_contract.py, not in tests/test_router_exceptions.py which the issue also names. That file tests error_from_response in isolation, where no key exists yet; the property under test is about what run() produces, so it belongs where run() is driven.
_stamp uses setattr, which is the riskiest line in the diff. It is safe because BaseException does not define __slots__, so every exception instance — including httpx's, which this SDK does not construct — has a __dict__ and cannot raise AttributeError here. A raise inside that except handler would have masked the caller's real error, which is why it is worth stating.

Negative-claim falsification

Not applicable, checked deliberately: this diff denies no capability. It adds one, contains no not supported / unavailable / dead-end path, and flips no test to assert an absence. Every new test asserts a capability now works.

Provenance

Authored by: agent-work loop
Verified: at 7c2b0bf (c4d757e is changelog prose only, no code) — uv run --extra dev pytest: 598 passed, 4 skipped (integration suite skips without live credentials, as designed); ruff check .: all checks passed; ruff format --check .: 51 files already formatted; mypy src: no issues in 19 source files; scripts/check_public_repo_hygiene.py: OK. The 20 new tests are the review round's coverage: the request-id sanitiser (including the duplicate-header join and a terminal escape), both layers cleaning through the same function, retry_after forwarded for a non-throttled code, every stamped attribute reading None on a transport failure, the stamped-attribute list pinned against the base's readable surface, a cancelled call stamped and still cancelled, KeyboardInterrupt still untouched, the undecodable success body, and the rewritten replay test plus its negative.
Deviations: none against the stated acceptance criteria; the readable-two-ways one is judgment call 1 above. Two review findings were answered rather than implemented, both recorded in the Review round section with the reasoning — the sentinel default (declined as a breaking change to a documented default, mitigated in the idiom and pinned by a test) and stamping submit() (deferred to the filed follow-up). CodeRabbit's changelog finding was accepted and fixed in c4d757e.

Residual

Three of the five auto-minting sites are not covered by this change, and the same gap exists on them. I swept new_idempotency_key across src/ to size the fix: five call sites mint a key the caller never supplied. Two are Models.run / AsyncModels.run and are fixed here. The other three are not, and an exception from any of them still drops the key:

src/comfy_sdk/client.py — Comfy.submit() and AsyncComfy.submit() (two sites). These do not go through translating(); they hand-roll to_sdk_error inside their own 429 retry loop, so the one-line fix used here does not apply and it is a separate edit.
src/comfy_sdk/assets.py — the asset uploader mints one key per uploader instance.

I left them alone deliberately rather than by oversight: the recovery story differs. A submitted job is created and pollable by id, so the route back is jobs.get(...) rather than a same-key replay, and whether submit()'s exception surface should grow the attribute is a real API decision rather than a mechanical extension. It is worth its own issue, and the count above is the size of it. A follow-up has now been filed for the two submit() sites, raised again by the review panel from the other direction: because submit() mints and sends a key but reports .idempotency_key is None, a caller who reads None as "no key was sent, resend freely" duplicates the job. The docs half of that is fixed here — the attribute's docstring and the README now say plainly that None means "this SDK did not record a key for you", never "no key reached the server".

The server-side contract this change is written against was not exercised. The recovery idiom in the run() docstring and the README — a same-key resend returning the original result (200 plus an Idempotent-Replayed marker) or being refused while the original generation is still running, with a Retry-After — is the router idempotency middleware's behaviour as described in the source issue. That middleware lives in a private repository that is not reachable from this environment, so I read neither it nor its tests. What I verified is the SDK half: the key survives onto the exception, and the resend carries it verbatim. The end-to-end replay test drives the repo's own stub server configured as a key-replaying deployment, which asserts the SDK's side of the handshake, not the real server's. If the real middleware's replay semantics differ from that description, the prose in the docstring and README needs correcting — the attribute and the plumbing do not.

The 409 status fallback is left as it is, and a follow-up filed. _CODE_BY_STATUS maps a 409 carrying no error.code to hash_mismatch, so any body-less 409 — from the router, from an intermediary — surfaces as comfy_sdk.HashMismatch with the response's own detail discarded. It is reachable today. I did not change it here because it is a wire-contract decision rather than a mechanical fix: 409 genuinely is hash_mismatch on the assets path, so the entry is right for the surface it was written for and wrong for this one. The follow-up carries the three candidate approaches for a human to pick between.

Two named artifacts could not be read. The tracking issues linked from the source issue (the replay contract, the deadline-survival reservation, the release cutoff, the cross-SDK parity check, and the companion retry-default issue) are internal and I hold no access to them, so this change is built from the source issue's own quoted evidence rather than from those. Likewise the TypeScript SDK twin, which the source issue puts out of scope for this change and asks to be filed separately: I did not check Comfy-Org/comfy-typescript-sdk for the same gap, so whether it has one is still unestablished.

_{🔄 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/97 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/27/2026 **Status:** ✅ Merged **Merged:** 8/27/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-9834-run-exception-idempotency-key` --- ### 📝 Commits (4) - [`0742a44`](https://github.com/Comfy-Org/comfy-python-sdk/commit/0742a448caec4eded5ae373f68bdacf6a8b1653e) feat: carry the Idempotency-Key and request id on every exception models.run raises - [`7c2b0bf`](https://github.com/Comfy-Org/comfy-python-sdk/commit/7c2b0bfe6913f74ce02a4f62ef7e632c1f09891a) fix: address the review panel on the exception idempotency-key surface - [`c4d757e`](https://github.com/Comfy-Org/comfy-python-sdk/commit/c4d757eb348073929f55fcc7921a0ae8bd5afaea) docs: scope the changelog claim to failed calls rather than every exception - [`11a9f60`](https://github.com/Comfy-Org/comfy-python-sdk/commit/11a9f605d1fa1fa5fe16dc3c5e132e1bf2f97bb1) Merge origin/main into matt/be-9834-run-exception-idempotency-key ### 📊 Changes **10 files changed** (+940 additions, -16 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+44 -0) 📝 `README.md` (+58 -0) 📝 `src/comfy_low/errors.py` (+35 -0) 📝 `src/comfy_low/transport.py` (+38 -3) 📝 `src/comfy_sdk/exceptions.py` (+120 -3) 📝 `src/comfy_sdk/models.py` (+35 -4) 📝 `src/comfy_sdk/router_exceptions.py` (+6 -1) 📝 `tests/conftest.py` (+62 -1) 📝 `tests/test_error_contract.py` (+188 -1) 📝 `tests/test_models_run.py` (+354 -3) </details> ### 📄 Description ## ELI-5 When you call `client.models.run(...)`, the SDK quietly makes up a one-time `Idempotency-Key` and sends it with the request. That key is your receipt: if the answer never comes back — the server gives up holding the connection and returns a `504`, or your connection drops mid-generation — the generation may well have finished and been billed, and asking again *under the same key* is how you collect it. The problem: you never saw the key. It lived in a local variable inside `run()`, and when `run()` raised, it went away with the call. So the receipt was shredded at exactly the moment you needed it, and your paid-for generation was unreachable. Only people who had passed their own `idempotency_key=` and written it down somewhere could recover. Now every exception `run()` raises hands you the receipt back, on `exc.idempotency_key`. Re-run with `idempotency_key=exc.idempotency_key` and you get your generation instead of paying for a second one. ## What changed `comfy_sdk.exceptions.translating()` takes an optional `idempotency_key=`, and stamps it onto whatever leaves the block. Both `Models.run` and `AsyncModels.run` pass the key they minted. That is the whole mechanism. Stamping at the **translation boundary** rather than in each exception's constructor is the load-bearing choice, and it buys three things a per-subclass approach would miss: - an `error_type` this SDK version has never heard of falls through to the bare `RouterError` base — and still carries the key; - a transport failure has no response to translate at all (`httpx.ConnectError`, a read timeout on a run the server is probably still generating), and it is exactly the "lost the response" case the recovery contract is about — it carries the key too; - there is one place to read, not fifteen constructors to keep in step. `_STAMPABLE` is `(ComfyError, httpx.HTTPError, asyncio.CancelledError)` — deliberately not "everything". A `ZeroDivisionError` escaping the block is a bug in the SDK or in caller code, not a failed call, and a key means nothing on it; the same reasoning `models._CANDIDATE_FAILURES` already documents for the retry classifier. It also means `KeyboardInterrupt` is never touched. Tested both ways. `asyncio.CancelledError` is the one `BaseException` in that tuple, added in review: cancelling an in-flight `AsyncModels.run` — which is what the `asyncio.wait_for` a caller wraps a ten-minute call in does — abandons a generation that may already be dispatched and billed, and that is exactly the case the key exists to collect. It is re-raised **bare** like the rest of that branch, so nothing is swallowed and the cancellation still propagates. `request_id` rides along for a closely related reason. `X-Comfy-Request-Id` was read off a *router* error response but dropped on the floor by the shared error envelope, so the id a user quotes in a support request was unreachable once the response object was gone. `parse_or_raise` now reads it, `ApiError` carries it beside `retry_after` (same pattern, same reason), and `to_sdk_error` passes it through. `RouterError`'s existing `request_id` is untouched — the base now just declares the same attribute so it is uniform. `retry_after` joined them in review. `to_sdk_error` had been dropping `ApiError.retry_after` for every code except `queue_full`, which left the flow this PR documents incoherent: the docs tell a caller to replay after the pace the server named, and a `deadline_exceeded` `504` carrying `Retry-After: 2` had nowhere to surface it. `ComfyError` now carries it for every code. This does not touch the retry path — `RetryPolicy.should_retry` and `client._retry_delay` both read the protocol-level `ApiError` *before* translation, so what `to_sdk_error` produces has never fed a retry decision. All three attributes are declared on `ComfyError` with a `None` default, and `_stamp` defaults the readable ones onto exceptions this SDK does not build (httpx's, `CancelledError`) without ever overwriting a real value — so `exc.request_id` on a dropped connection reads `None` rather than raising `AttributeError`, and no caller is forced into `getattr(exc, ..., None)`. ### Additive, verified as such - `translating()` with no key behaves exactly as it did before the parameter existed. The new `except _STAMPABLE` branch does a bare `raise`, which preserves the original traceback and exception identity; `contextlib` sees `exc is value` and does not suppress. Pinned by a test. - Nothing changes about **what is retried** — the retry policy is untouched. - Nothing changes **on the wire** — no new header, no changed header, no changed body. The existing wire assertions in `tests/test_models_run.py` and `tests/test_models_run_retry.py` still pass unchanged. - No public name was removed or repurposed. `tests/test_error_contract.py` now pins the base error's *whole* attribute surface as a written-out list, so adding one is a deliberate edit to a reviewable list and removing one is a failing test rather than a silent break of somebody's `except` block. ## Tests New coverage in `tests/test_models_run.py` (sync and async throughout): - a `504 deadline_exceeded` from `run()` exposes the **exact** auto-minted key — compared against the key the stub server actually received, not merely asserted truthy, because a freshly minted key is precisely the value that *cannot* collect the generation; - a caller-supplied key round-trips onto the exception; - a transport-level failure (`httpx.ConnectError`, `httpx.ReadTimeout`) carries the key; - a real client-side timeout against the live stub server carries the key; - an unrecognised `error_type` lands on the base `RouterError` **with** the key, and a typed subclass does too; - when retries exhaust, the key on the exception is the one every attempt reused — not the first attempt's, not a fresh one; - end to end: the docstring's recovery idiom actually collects the generation against a stub configured as a key-replaying deployment; - `request_id` present when the response carried the header, `None` when it did not, independent of the key. `tests/conftest.py` gains a `model_run_request_id` knob so the stub can emit `X-Comfy-Request-Id` on a failed run. No httpx mocking — `server.state` only, per the repo's convention. `tests/test_error_contract.py` gains the attribute-surface pins described above, `translating()`'s stamp behaviour with and without a key, the not-stamped bug case, and a guard that `comfy_low` and `comfy_sdk` spell `X-Comfy-Request-Id` identically (each layer needs its own copy — `comfy_low` never imports `comfy_sdk` — so a drift would silently drop the id on one of the two surfaces). ## Review round (7c2b0bf, c4d757e) Ten findings from the review panel, all addressed. Eight in code, two answered: - **The README recovery snippet caught only `ComfyError`** — the high-severity one, and correct: a dropped connection or read timeout is one of the two headline cases the section exists for, and it arrives as an `httpx` error with no response to translate. A reader copying that snippet never reached the replay for exactly the failure it is about. Now `except (ComfyError, httpx.HTTPError)`, and it checks the key for `None` before passing it back. - **`_stamp` wrote only `idempotency_key`**, so `exc.request_id` on an httpx failure was an `AttributeError` rather than the documented `None` — on precisely the no-response failures the pair matters most on. It now defaults the whole readable surface, pinned against the base's attribute list so a fourth attribute cannot be added without it. - **`to_sdk_error` dropped `Retry-After`** for every code but `queue_full` — described above. - **The ok path called `resp.json()` unguarded**, so a `200` with a non-JSON body (a proxy interstitial, a truncated response) escaped as `json.JSONDecodeError` from *outside* the translated surface, and so unstamped. On `models.run` that is a generation that ran and was billed with the result lost — the exact failure the key has to ride out on. It is now an `ApiError(code="invalid_response")` raised at the decode site, which also gives it a `request_id`. - **`asyncio.CancelledError` passed unstamped** — described above. - **`X-Comfy-Request-Id` was stored verbatim** from a server-controlled header, with no length bound and no filtering, and the docs tell users to display it and paste it into support tickets. Now bounded and filtered through a single `clean_request_id` in `comfy_low.errors` that *both* error surfaces call, so an id cannot be safe to display on one and not the other. It matches the leading run of allowed characters rather than deleting disallowed ones, which gives the right answer for the duplicate-header join `httpx.Headers.get` performs: `"a1, a2"` yields `"a1"`, not a spliced `"a1a2"` that identifies no call at all. - **The end-to-end replay test did not actually exercise replay.** The old stub skipped *claiming* the key on the 5xx but stored no prior result, so the second same-key request re-ran the model and the payload comparison passed for a brand-new second generation — the double charge the feature prevents, indistinguishable from a real replay. The stub now models a lost response properly (records the result against the key, serves it back without re-running) behind a separate `model_run_replays_lost_result` knob, so the existing retry tests keep the old flag's narrower meaning. The assertion is now one generation across two requests, with the failure knob deliberately left set so the second call succeeds because the key collected the recording, not because the error was switched off. - **The `run` docstring named a `409` "still in progress" refusal** the mapping does not deliver: `_CODE_BY_STATUS` maps a body-less 409 to `hash_mismatch`, so a caller would read a bytes-integrity failure. The docstring no longer promises a class and points at `exc.retry_after` instead. Fixing the mapping is deferred — see Residual. Answered rather than changed: - **A sentinel default so an explicit `idempotency_key=None` is rejected.** Declined: `None` is the documented default meaning "mint one", so `run(m, a, idempotency_key=maybe_key)` would start raising with no deprecation. The underlying footgun is real, so the *idiom* changed instead — the snippet and docstring both say to check for `None` first, and `test_replaying_without_the_key_would_start_a_second_generation` asserts the outcome directly so the guard is not silently dropped later. - **Stamping `submit()`'s errors too.** Out of scope here and filed separately — see Residual. Every one of the ten threads has a reply explaining what was done and why, and is resolved. CodeRabbit then raised one more against the changelog, and was right: **"every exception" was literally overbroad.** `_STAMPABLE` is deliberately not "everything" — a programming error escaping the call reaches the caller untouched, and `KeyboardInterrupt` is never touched either, both with tests asserting it. The entry now names the boundary as the *failed call* rather than the exception type, which is the property the design actually holds and the one that stays true if another call-failure type is added later. Fixed in c4d757e; thread resolved. ## Judgment calls 1. **"Every exception" is scoped to failures of the call.** The acceptance wording says every exception, then enumerates router errors, `ComfyError`, and transport errors — which is what `_STAMPABLE` implements. A programming error propagates unstamped, on purpose, and there is a test asserting that. Flagging it because it is a readable-two-ways criterion, not because I think the other reading is right. 2. **`request_id` was added at `parse_or_raise`, which every transport method shares** — not just the model-run path. It is one line and purely additive (a new attribute defaulting to `None`), and scoping it to one route would have meant the same header meaning two different things on two surfaces. 3. **The base-`RouterError`-carries-the-key assertion lives in `tests/test_models_run.py` and `tests/test_error_contract.py`,** not in `tests/test_router_exceptions.py` which the issue also names. That file tests `error_from_response` in isolation, where no key exists yet; the property under test is about what `run()` produces, so it belongs where `run()` is driven. 4. **`_stamp` uses `setattr`**, which is the riskiest line in the diff. It is safe because `BaseException` does not define `__slots__`, so every exception instance — including httpx's, which this SDK does not construct — has a `__dict__` and cannot raise `AttributeError` here. A raise inside that `except` handler would have masked the caller's real error, which is why it is worth stating. ## Negative-claim falsification Not applicable, checked deliberately: this diff denies no capability. It adds one, contains no `not supported` / `unavailable` / dead-end path, and flips no test to assert an absence. Every new test asserts a capability now works. ## Provenance - **Authored by:** agent-work loop - **Verified:** at 7c2b0bf (c4d757e is changelog prose only, no code) — `uv run --extra dev pytest`: 598 passed, 4 skipped (integration suite skips without live credentials, as designed); `ruff check .`: all checks passed; `ruff format --check .`: 51 files already formatted; `mypy src`: no issues in 19 source files; `scripts/check_public_repo_hygiene.py`: OK. The 20 new tests are the review round's coverage: the request-id sanitiser (including the duplicate-header join and a terminal escape), both layers cleaning through the same function, `retry_after` forwarded for a non-throttled code, every stamped attribute reading `None` on a transport failure, the stamped-attribute list pinned against the base's readable surface, a cancelled call stamped and still cancelled, `KeyboardInterrupt` still untouched, the undecodable success body, and the rewritten replay test plus its negative. - **Deviations:** none against the stated acceptance criteria; the readable-two-ways one is judgment call 1 above. Two review findings were answered rather than implemented, both recorded in the Review round section with the reasoning — the sentinel default (declined as a breaking change to a documented default, mitigated in the idiom and pinned by a test) and stamping `submit()` (deferred to the filed follow-up). CodeRabbit's changelog finding was accepted and fixed in c4d757e. ## Residual **Three of the five auto-minting sites are not covered by this change, and the same gap exists on them.** I swept `new_idempotency_key` across `src/` to size the fix: five call sites mint a key the caller never supplied. Two are `Models.run` / `AsyncModels.run` and are fixed here. The other three are **not**, and an exception from any of them still drops the key: - `src/comfy_sdk/client.py` — `Comfy.submit()` and `AsyncComfy.submit()` (two sites). These do *not* go through `translating()`; they hand-roll `to_sdk_error` inside their own `429` retry loop, so the one-line fix used here does not apply and it is a separate edit. - `src/comfy_sdk/assets.py` — the asset uploader mints one key per uploader instance. I left them alone deliberately rather than by oversight: the recovery story differs. A submitted job is created and pollable by id, so the route back is `jobs.get(...)` rather than a same-key replay, and whether `submit()`'s exception surface should grow the attribute is a real API decision rather than a mechanical extension. It is worth its own issue, and the count above is the size of it. **A follow-up has now been filed for the two `submit()` sites**, raised again by the review panel from the other direction: because `submit()` mints and sends a key but reports `.idempotency_key is None`, a caller who reads `None` as "no key was sent, resend freely" duplicates the job. The docs half of that is fixed here — the attribute's docstring and the README now say plainly that `None` means "this SDK did not record a key for you", never "no key reached the server". **The server-side contract this change is written against was not exercised.** The recovery idiom in the `run()` docstring and the README — a same-key resend returning the original result (`200` plus an `Idempotent-Replayed` marker) or being refused while the original generation is still running, with a `Retry-After` — is the router idempotency middleware's behaviour as described in the source issue. That middleware lives in a private repository that is not reachable from this environment, so I read neither it nor its tests. What I verified is the *SDK* half: the key survives onto the exception, and the resend carries it verbatim. The end-to-end replay test drives the repo's own stub server configured as a key-replaying deployment, which asserts the SDK's side of the handshake, not the real server's. If the real middleware's replay semantics differ from that description, the prose in the docstring and README needs correcting — the attribute and the plumbing do not. **The `409` status fallback is left as it is, and a follow-up filed.** `_CODE_BY_STATUS` maps a 409 carrying no `error.code` to `hash_mismatch`, so any body-less 409 — from the router, from an intermediary — surfaces as `comfy_sdk.HashMismatch` with the response's own detail discarded. It is reachable today. I did not change it here because it is a wire-contract decision rather than a mechanical fix: 409 genuinely *is* `hash_mismatch` on the assets path, so the entry is right for the surface it was written for and wrong for this one. The follow-up carries the three candidate approaches for a human to pick between. **Two named artifacts could not be read.** The tracking issues linked from the source issue (the replay contract, the deadline-survival reservation, the release cutoff, the cross-SDK parity check, and the companion retry-default issue) are internal and I hold no access to them, so this change is built from the source issue's own quoted evidence rather than from those. Likewise the TypeScript SDK twin, which the source issue puts out of scope for this change and asks to be filed separately: I did not check `Comfy-Org/comfy-typescript-sdk` for the same gap, so whether it has one is still unestablished. --- <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:21 +00:00