[PR #133] [MERGED] feat: carry the Idempotency-Key on every exception submit() raises #133

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/133
Author: @mattmillerai
Created: 9/5/2026
Status: ✅ Merged
Merged: 9/10/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-9932-submit-exception-idempotency-key

📝 Commits (2)

239ea18 feat: carry the Idempotency-Key on every exception submit() raises
bfe04f6 fix: address bot review comments

📊 Changes

6 files changed (+429 additions, -37 deletions)

View changed files

📝 CHANGELOG.md (+27 -0)
📝 README.md (+26 -6)
📝 src/comfy_sdk/client.py (+66 -25)
📝 src/comfy_sdk/exceptions.py (+23 -6)
📝 tests/conftest.py (+6 -0)
➕ tests/test_client_submit_idempotency.py (+281 -0)

📄 Description

ELI-5

submit() puts a one-time ticket number on every job it sends, so the server can refuse an accidental duplicate. If the send blew up, that number vanished with the error — and if submit() had minted it for you, you never saw it at all, so you couldn't go look up whether a job had been created. Now the ticket number rides out on the exception, exactly as it already does for client.models.run().

What changed

Comfy.submit / AsyncComfy.submit wrap their while True: retry loop in translating(idempotency_key=key) — the same boundary models.run has used since #97. That is the whole product change.

The inner except ApiError handler is untouched: it still converts, still consults _retry_delay, still time.sleep / await asyncio.sleeps and retries. The outer context manager only stamps what the inner handler lets out (a ComfyError re-raised bare through translating's _STAMPABLE branch).
A transport failure (httpx.HTTPError) previously escaped submit() untranslated. It now carries .idempotency_key, and .request_id / .retry_after default to None on it rather than raising AttributeError — the documented trio finally reads uniformly on the failure it is most needed on.
Cancelling an in-flight AsyncComfy.submit() carries the key too, and still propagates bare (same contract as AsyncModels.run).
Comfy.run / AsyncComfy.run inherit it for free: they submit through submit.
Key minting (key = idempotency_key or _core.new_idempotency_key()), what is retried, and the bytes on the wire are all unchanged.

The semantic difference is documented, not glossed. models.run sends its key to a surface that replays a claimed key, which is what makes that key a handle on a generation you were already billed for. POST /jobs instead rejects a reused key with 422 idempotency_key_reuse — spec/openapi.yaml:645: "Single-use: the first request to present a key is processed; any later request with the same key is rejected 422 idempotency_key_reuse (reject-on-duplicate, no response replay)", against spec/router-openapi.yaml:102, where a replayed response "carries Idempotent-Replayed: true and is not charged a second time". So on a submit() error the key tells you a key was sent: poll or list for the job the first attempt may have created, rather than resubmitting under it. The README paragraph says exactly that and cross-references the existing IdempotencyKeyReuse bullet; ComfyError.idempotency_key's docstring and the CHANGELOG entry carry the same distinction.

Tests

New module tests/test_client_submit_idempotency.py (14 tests, sync and async throughout), mirroring the stamping tests in tests/test_models_run.py against the POST /jobs stub:

Auto-minted key on a non-retryable error (422 validation_error, deliberately an unmapped code so the stamp is proven on the base ComfyError) equals the header the server actually received.
A caller-supplied idempotency_key="abc" round-trips onto the exception and onto the wire.
Queue-full budget exhaustion: the QueueFull carries the key, and it is the same key on every retried request.
422 idempotency_key_reuse — driven by really reusing a key against the stub, not by a stubbed envelope — carries the key that was rejected.
Transport failure against a closed port: httpx.ConnectError carries the key and reads .request_id is None, .retry_after is None.
run() surfaces the key of a failed submit phase.
Cancelling an in-flight AsyncComfy.submit() yields the key, reads .request_id as None, and the CancelledError still propagates.

tests/conftest.py gains one additive knob, ServerState.jobs_idempotency_keys — every Idempotency-Key seen on POST /jobs in arrival order, mirroring the existing model_run_idempotency_keys. See the judgment call below for why.

Judgment calls

The ticket's acceptance for test 1 asserts exc.idempotency_key == server.state.idempotency, which cannot work. ServerState.idempotency is a dict of key -> job_id and it is only written on an accepted submit, so it is empty on exactly the error paths these tests exercise. I added the ordered-capture list instead (the same shape the model-run side already uses) and asserted against the key the server genuinely received, which is what the criterion was reaching for.
The queue-full retry budget is spent by a stepped fake clock, not by wall time. Monkeypatching _client_module._now with a clock that advances 1.0s per read (budget 3.0) makes "the budget ran out after N attempts" exact. Timing it against a real 20–50 ms budget would have been a race on a loaded machine, and the assertion needs len(sent) > 1 to mean anything.
The retry loop is wrapped; _guard_ui_format and _materialize are not, per the ticket. Both run before the key is minted, so a failure there genuinely has no key to carry.
run()'s polling phase is deliberately not stamped. Once submit returns, the job exists and is addressed by its id; a submit key there would be noise, and the CHANGELOG scopes the guarantee to "a failed call".
Line references in the ticket had drifted (it names client.py:161-173 and README.md:523 from the #97 branch; on today's main they are client.py:329 / README.md:587, and the import line is MissingApiKey, WorkflowFormatUi, to_sdk_error, not the QueueFull, ... the ticket quotes). I matched the code, not the line numbers.
The prerequisite (#97) is merged, so this branches off main rather than stacking. #99 is also merged and touches models.py/retry.py only — no overlap.

Residual

Asset uploads still send an Idempotency-Key without stamping it. AssetHandle mints one per handle (src/comfy_sdk/assets.py:69) and passes it to post_assets (assets.py:139, assets.py:185) inside a bare with translating(): — no key argument — so every exception an upload raises still reads .idempotency_key is None. That is out of this change's scope (the ticket scopes to submit), but it makes the flat sentence the ticket asked for in the docs (None on operations that "send no key") untrue, so I did not write it: the ComfyError.idempotency_key docstring and the README paragraph both name the asset-upload surface explicitly as the remaining key-sending-but-unrecording one. Fixing it is a one-line change per call site (with translating(idempotency_key=self._idempotency_key):) plus its own tests, and it deserves a decision first, because an upload key is deduplicated by content hash and its recovery story is not the same as a job's.
No live deployment was exercised. Everything here runs against the in-repo stub server; the reject-not-replay semantics this PR documents are cited from the vendored spec/openapi.yaml and spec/router-openapi.yaml, not observed against Comfy Cloud. tests/integration needs a live target and a credential and was not run.
The ticket's linked spike issue and its findings comment are named but not reachable from this environment, so the evidence behind the ticket was taken as given rather than re-read.

Provenance

Authored by: agent-work loop
Verified: uv run --extra dev pytest -q: 732 passed, 4 skipped, 0 failed (full suite, under the shared per-repo suite lock); uv run --extra dev ruff check .: all checks passed; uv run --extra dev ruff format --check .: 52 files already formatted; uv run --extra dev mypy src: no issues in 19 source files. tests/integration not run (needs a live deployment + credential).
Deviations: ticket test-criterion 1's server.state.idempotency assertion replaced with an ordered header capture (see Judgment calls); the docs sentence the ticket dictated was amended so it does not make a false claim about asset uploads (see Residual). No other criterion skipped.

_{🔄 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/133 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/5/2026 **Status:** ✅ Merged **Merged:** 9/10/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-9932-submit-exception-idempotency-key` --- ### 📝 Commits (2) - [`239ea18`](https://github.com/Comfy-Org/comfy-python-sdk/commit/239ea184bca54f1237b6e63a910fe7b963ef01fd) feat: carry the Idempotency-Key on every exception submit() raises - [`bfe04f6`](https://github.com/Comfy-Org/comfy-python-sdk/commit/bfe04f61c4a861d5880f1929d7cb4d6da22dd8e8) fix: address bot review comments ### 📊 Changes **6 files changed** (+429 additions, -37 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+27 -0) 📝 `README.md` (+26 -6) 📝 `src/comfy_sdk/client.py` (+66 -25) 📝 `src/comfy_sdk/exceptions.py` (+23 -6) 📝 `tests/conftest.py` (+6 -0) ➕ `tests/test_client_submit_idempotency.py` (+281 -0) </details> ### 📄 Description ## ELI-5 `submit()` puts a one-time ticket number on every job it sends, so the server can refuse an accidental duplicate. If the send blew up, that number vanished with the error — and if `submit()` had minted it for you, you never saw it at all, so you couldn't go look up whether a job had been created. Now the ticket number rides out on the exception, exactly as it already does for `client.models.run()`. ## What changed `Comfy.submit` / `AsyncComfy.submit` wrap their `while True:` retry loop in `translating(idempotency_key=key)` — the same boundary `models.run` has used since #97. That is the whole product change. - The inner `except ApiError` handler is untouched: it still converts, still consults `_retry_delay`, still `time.sleep` / `await asyncio.sleep`s and retries. The outer context manager only *stamps* what the inner handler lets out (a `ComfyError` re-raised bare through `translating`'s `_STAMPABLE` branch). - A transport failure (`httpx.HTTPError`) previously escaped `submit()` untranslated. It now carries `.idempotency_key`, and `.request_id` / `.retry_after` default to `None` on it rather than raising `AttributeError` — the documented trio finally reads uniformly on the failure it is most needed on. - Cancelling an in-flight `AsyncComfy.submit()` carries the key too, and still propagates bare (same contract as `AsyncModels.run`). - `Comfy.run` / `AsyncComfy.run` inherit it for free: they submit through `submit`. - Key minting (`key = idempotency_key or _core.new_idempotency_key()`), what is retried, and the bytes on the wire are all unchanged. **The semantic difference is documented, not glossed.** `models.run` sends its key to a surface that *replays* a claimed key, which is what makes that key a handle on a generation you were already billed for. `POST /jobs` instead *rejects* a reused key with `422 idempotency_key_reuse` — `spec/openapi.yaml:645`: "Single-use: the first request to present a key is processed; any later request with the same key is rejected `422` `idempotency_key_reuse` (reject-on-duplicate, no response replay)", against `spec/router-openapi.yaml:102`, where a replayed response "carries `Idempotent-Replayed: true` and is not charged a second time". So on a `submit()` error the key tells you a key *was* sent: poll or list for the job the first attempt may have created, rather than resubmitting under it. The README paragraph says exactly that and cross-references the existing `IdempotencyKeyReuse` bullet; `ComfyError.idempotency_key`'s docstring and the `CHANGELOG` entry carry the same distinction. ## Tests New module `tests/test_client_submit_idempotency.py` (14 tests, sync and async throughout), mirroring the stamping tests in `tests/test_models_run.py` against the `POST /jobs` stub: 1. Auto-minted key on a non-retryable error (`422 validation_error`, deliberately an *unmapped* code so the stamp is proven on the base `ComfyError`) equals the header the server actually received. 2. A caller-supplied `idempotency_key="abc"` round-trips onto the exception and onto the wire. 3. Queue-full budget exhaustion: the `QueueFull` carries the key, and it is the *same* key on every retried request. 4. `422 idempotency_key_reuse` — driven by really reusing a key against the stub, not by a stubbed envelope — carries the key that was rejected. 5. Transport failure against a closed port: `httpx.ConnectError` carries the key and reads `.request_id is None`, `.retry_after is None`. 6. `run()` surfaces the key of a failed submit phase. 7. Cancelling an in-flight `AsyncComfy.submit()` yields the key, reads `.request_id` as `None`, and the `CancelledError` still propagates. `tests/conftest.py` gains one additive knob, `ServerState.jobs_idempotency_keys` — every `Idempotency-Key` seen on `POST /jobs` in arrival order, mirroring the existing `model_run_idempotency_keys`. See the judgment call below for why. ## Judgment calls - **The ticket's acceptance for test 1 asserts `exc.idempotency_key == server.state.idempotency`, which cannot work.** `ServerState.idempotency` is a `dict` of `key -> job_id` and it is only written on an *accepted* submit, so it is empty on exactly the error paths these tests exercise. I added the ordered-capture list instead (the same shape the model-run side already uses) and asserted against the key the server genuinely received, which is what the criterion was reaching for. - **The queue-full retry budget is spent by a stepped fake clock, not by wall time.** Monkeypatching `_client_module._now` with a clock that advances 1.0s per read (budget 3.0) makes "the budget ran out after N attempts" exact. Timing it against a real 20–50 ms budget would have been a race on a loaded machine, and the assertion needs `len(sent) > 1` to mean anything. - **The retry loop is wrapped; `_guard_ui_format` and `_materialize` are not**, per the ticket. Both run before the key is minted, so a failure there genuinely has no key to carry. - **`run()`'s polling phase is deliberately not stamped.** Once `submit` returns, the job exists and is addressed by its id; a submit key there would be noise, and the CHANGELOG scopes the guarantee to "a failed call". - **Line references in the ticket had drifted** (it names `client.py:161-173` and `README.md:523` from the #97 branch; on today's `main` they are `client.py:329` / `README.md:587`, and the import line is `MissingApiKey, WorkflowFormatUi, to_sdk_error`, not the `QueueFull, ...` the ticket quotes). I matched the code, not the line numbers. - The prerequisite (#97) is merged, so this branches off `main` rather than stacking. #99 is also merged and touches `models.py`/`retry.py` only — no overlap. ## Residual - **Asset uploads still send an `Idempotency-Key` without stamping it.** `AssetHandle` mints one per handle (`src/comfy_sdk/assets.py:69`) and passes it to `post_assets` (`assets.py:139`, `assets.py:185`) inside a bare `with translating():` — no key argument — so every exception an upload raises still reads `.idempotency_key is None`. That is out of this change's scope (the ticket scopes to `submit`), but it makes the flat sentence the ticket asked for in the docs (`None` on operations that "send no key") **untrue**, so I did not write it: the `ComfyError.idempotency_key` docstring and the README paragraph both name the asset-upload surface explicitly as the remaining key-sending-but-unrecording one. Fixing it is a one-line change per call site (`with translating(idempotency_key=self._idempotency_key):`) plus its own tests, and it deserves a decision first, because an upload key is deduplicated by content hash and its recovery story is not the same as a job's. - **No live deployment was exercised.** Everything here runs against the in-repo stub server; the reject-not-replay semantics this PR documents are cited from the vendored `spec/openapi.yaml` and `spec/router-openapi.yaml`, not observed against Comfy Cloud. `tests/integration` needs a live target and a credential and was not run. - The ticket's linked spike issue and its findings comment are named but not reachable from this environment, so the evidence behind the ticket was taken as given rather than re-read. ## Provenance - **Authored by:** agent-work loop - **Verified:** `uv run --extra dev pytest -q`: 732 passed, 4 skipped, 0 failed (full suite, under the shared per-repo suite lock); `uv run --extra dev ruff check .`: all checks passed; `uv run --extra dev ruff format --check .`: 52 files already formatted; `uv run --extra dev mypy src`: no issues in 19 source files. `tests/integration` not run (needs a live deployment + credential). - **Deviations:** ticket test-criterion 1's `server.state.idempotency` assertion replaced with an ordered header capture (see Judgment calls); the docs sentence the ticket dictated was amended so it does not make a false claim about asset uploads (see Residual). No other criterion skipped. --- <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