feat: carry the Idempotency-Key on every exception submit() raises - #133
feat: carry the Idempotency-Key on every exception submit() raises#133mattmillerai wants to merge 1 commit into
Conversation
`Comfy.submit` / `AsyncComfy.submit` mint an `Idempotency-Key` per call and send it on `POST /jobs`, but the key was a local of the submitting frame: any exception the call raised took the caller's only record of what was sent with it. A transport failure escaped `submit()` untranslated as well, so the documented `.request_id` / `.retry_after` pair raised `AttributeError` there rather than reading `None`. Wrap each retry loop in `translating(idempotency_key=key)` — the same boundary `models.run` already uses. The inner `except ApiError` handler still owns what is retried and what surfaces; the outer stamp only attaches the key to whatever it lets out, so nothing about the retry policy, the key minting or the wire changed. The semantics differ from `models.run`'s and the docs say so: `POST /jobs` rejects a reused key (`422 idempotency_key_reuse`) rather than replaying it, so the key on a `submit()` error is a record of what was sent — poll or list for the job the first attempt may have created — not a replay handle.
|
Warning Review limit reachedNext included review available in 47 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 136 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (6)
Comment |
![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 10 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 4 |
| 🟢 Low | 4 |
| ⚪ Nit | 2 |
Panel: 6/6 reviewers contributed findings.
| @@ -321,17 +329,23 @@ def submit( | |||
| key = idempotency_key or _core.new_idempotency_key() | |||
There was a problem hiding this comment.
🟡 Medium — submit takes a caller key by truthiness (idempotency_key or _core.new_idempotency_key()) instead of routing it through _core.validate_idempotency_key the way models.run does, so idempotency_key="" silently mints a fresh key — the caller's dedup is disabled and a retry can create a second billed job — while over-length or control/non-ASCII keys reach httpx and surface as an opaque LocalProtocolError. The new stamp compounds it: the exception now reports a key the caller never passed. Same at line 440 in AsyncComfy.submit. 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).
| #: :meth:`comfy_sdk.models.Models.run` and its async twin, and by | ||
| #: :meth:`comfy_sdk.client.Comfy.submit` / | ||
| #: :meth:`comfy_sdk.client.AsyncComfy.submit` — and therefore by | ||
| #: ``Comfy.run`` / ``AsyncComfy.run``, which submit through them. It is |
There was a problem hiding this comment.
🟡 Medium — "and therefore by Comfy.run / AsyncComfy.run" holds only for the submit phase: translating(idempotency_key=key) exits when submit returns, so the most common run() failures — a JobFailed from polling, or the TimeoutError from _run_with_timeout — still read .idempotency_key as None, and neither carries the Job handle either. Scope the sentence to the submit phase, or carry the key through the polling phase. Raised by 3 of 6 reviewers (gpt-5.6-sol-max adversarial, claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case).
| with translating(idempotency_key=key): | ||
| while True: | ||
| try: | ||
| model = self._low.post_jobs(graph, idempotency_key=key, extra_data=extra_data) |
There was a problem hiding this comment.
🟡 Medium — If POST /jobs answers 201 with a body that fails Job.model_validate, the resulting Pydantic ValidationError is neither an ApiError, a ComfyError, nor an httpx error, so translating lets it escape unstamped — exactly when the server may already have durably created the job. Normalize response-validation failures into a stampable invalid-response error. Raised by 2 of 6 reviewers (gpt-5.6-sol-max adversarial, gpt-5.6-sol-max edge-case).
|
|
||
|
|
||
| async def test_cancelling_an_in_flight_submit_still_yields_the_key(server) -> None: | ||
| # A submit cancelled mid-flight — what `asyncio.wait_for` around it does — |
There was a problem hiding this comment.
🟡 Medium — The test raises CancelledError from a stubbed post_jobs and catches it directly, so it never exercises the asyncio.wait_for path this comment cites: wait_for swallows the stamped cancellation and raises a fresh TimeoutError that has no .idempotency_key, leaving a wait_for caller without the key after a possibly-accepted submit. Add a case that actually wraps submit in wait_for (and expose the key on that caller-visible failure), or drop the wait_for claim. Raised by 2 of 6 reviewers (gpt-5.6-sol-max adversarial, gpt-5.6-sol-max edge-case).
| #: record a key for you", never "no key reached the server": do not infer | ||
| #: from it that a resend is safe. | ||
| #: | ||
| #: What the key is *good for* differs by surface, so read it with the |
There was a problem hiding this comment.
🟢 Low — .idempotency_key now spans two surfaces with opposite reuse semantics — replayable for models.run, single-use for POST /jobs — and the only discriminator is prose about which call raised. A handler that catches both (the natural generalization of the README snippet at lines 548-568) will take the replay branch on a submit() failure and feed a jobs key back into models.run(idempotency_key=...); consider a machine-readable flag on the exception (e.g. replayable) rather than documentation alone. Raised by 1 of 6 reviewers (claude-opus-5-thinking-max adversarial).
| instead **rejects** a reused key with `422 idempotency_key_reuse` (see the | ||
| [`IdempotencyKeyReuse`](#typed-errors) bullet below): keys there are single-use | ||
| and there is no replay. So on a `submit()` failure `exc.idempotency_key` tells | ||
| you a key *was* sent — do not resubmit blindly under it, poll or list for the |
There was a problem hiding this comment.
🟢 Low — "a key was sent — do not resubmit blindly under it, poll or list for the job" is wrong for the two failures this feature most targets: an httpx.ConnectError (which the new test pins as stamped) means no bytes reached the server, and an exhausted QueueFull means every attempt was rejected, so no job exists and the key was never claimed. Describe the stamp as "a key was minted for this attempt" and say when resubmitting under it is still valid. Raised by 2 of 6 reviewers (claude-opus-5-thinking-max adversarial, gpt-5.6-sol-max edge-case).
| reused key is *rejected*, not replayed: on reuse, catch the error and | ||
| poll/list for the job the first attempt already created. | ||
|
|
||
| Every exception a failed submit raises carries the key it was made |
There was a problem hiding this comment.
🟢 Low — "Every exception a failed submit raises carries the key" is false for failures that happen before the key exists: _guard_ui_format (WorkflowFormatUi) and _materialize (asset commit) both run above line 329 and outside translating, so they surface with .idempotency_key is None. The behavior is correct; narrow this sentence and the matching CHANGELOG bullet to failures of the POST /jobs attempt itself. Raised by 3 of 6 reviewers (kimi-k3-high adversarial, gpt-5.6-sol-max edge-case, claude-opus-5-thinking-max edge-case).
| you a key *was* sent — do not resubmit blindly under it, poll or list for the | ||
| job the first attempt may already have created. | ||
|
|
||
| `idempotency_key` is still `None` on errors from every other surface — one that |
There was a problem hiding this comment.
🟢 Low — "idempotency_key is still None on errors from every other surface" overstates the read-never-raises guarantee for transport errors: asset upload calls translating() with no key and _stamp returns before setting any attribute, so exc.idempotency_key on an httpx error from that path raises AttributeError rather than reading None. Either stamp None explicitly on keyless scopes or say the guarantee covers only key-carrying ones. Raised by 1 of 6 reviewers (gpt-5.6-sol-max adversarial).
| return client.workflows.from_json(_GRAPH) | ||
|
|
||
|
|
||
| def _closed_port() -> int: |
There was a problem hiding this comment.
⚪ Nit — _closed_port() releases the port when the with block exits, so between that and the connect in the three tests that use it anything on the machine (including a parallel test run) can bind it — those tests would then hit an unrelated listener instead of raising httpx.ConnectError. Holding a bound-but-not-listening socket open for the test's duration removes the window. Raised by 1 of 6 reviewers (claude-opus-5-thinking-max adversarial).
| await client.submit(_wf(client)) | ||
| sent = server.state.jobs_idempotency_keys | ||
| assert len(sent) > 1 | ||
| assert set(sent) == {excinfo.value.idempotency_key} |
There was a problem hiding this comment.
⚪ Nit — The async twin omits the assert excinfo.value.idempotency_key is not None that line 127 adds to the sync test, so if the header were ever dropped on the wire sent would be all None and this assertion would compare {None} == {None} — passing against a completely unstamped exception. Add the same guard. Raised by 1 of 6 reviewers (claude-opus-5-thinking-max edge-case).
1 participant
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 ifsubmit()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 forclient.models.run().What changed
Comfy.submit/AsyncComfy.submitwrap theirwhile True:retry loop intranslating(idempotency_key=key)— the same boundarymodels.runhas used since #97. That is the whole product change.except ApiErrorhandler is untouched: it still converts, still consults_retry_delay, stilltime.sleep/await asyncio.sleeps and retries. The outer context manager only stamps what the inner handler lets out (aComfyErrorre-raised bare throughtranslating's_STAMPABLEbranch).httpx.HTTPError) previously escapedsubmit()untranslated. It now carries.idempotency_key, and.request_id/.retry_afterdefault toNoneon it rather than raisingAttributeError— the documented trio finally reads uniformly on the failure it is most needed on.AsyncComfy.submit()carries the key too, and still propagates bare (same contract asAsyncModels.run).Comfy.run/AsyncComfy.runinherit it for free: they submit throughsubmit.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.runsends 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 /jobsinstead rejects a reused key with422 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 rejected422idempotency_key_reuse(reject-on-duplicate, no response replay)", againstspec/router-openapi.yaml:102, where a replayed response "carriesIdempotent-Replayed: trueand is not charged a second time". So on asubmit()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 existingIdempotencyKeyReusebullet;ComfyError.idempotency_key's docstring and theCHANGELOGentry carry the same distinction.Tests
New module
tests/test_client_submit_idempotency.py(14 tests, sync and async throughout), mirroring the stamping tests intests/test_models_run.pyagainst thePOST /jobsstub:422 validation_error, deliberately an unmapped code so the stamp is proven on the baseComfyError) equals the header the server actually received.idempotency_key="abc"round-trips onto the exception and onto the wire.QueueFullcarries 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.httpx.ConnectErrorcarries the key and reads.request_id is None,.retry_after is None.run()surfaces the key of a failed submit phase.AsyncComfy.submit()yields the key, reads.request_idasNone, and theCancelledErrorstill propagates.tests/conftest.pygains one additive knob,ServerState.jobs_idempotency_keys— everyIdempotency-Keyseen onPOST /jobsin arrival order, mirroring the existingmodel_run_idempotency_keys. See the judgment call below for why.Judgment calls
exc.idempotency_key == server.state.idempotency, which cannot work.ServerState.idempotencyis adictofkey -> job_idand 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._client_module._nowwith 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 needslen(sent) > 1to mean anything._guard_ui_formatand_materializeare 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. Oncesubmitreturns, 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".client.py:161-173andREADME.md:523from the feat: carry the Idempotency-Key and request id on every exception models.run raises #97 branch; on today'smainthey areclient.py:329/README.md:587, and the import line isMissingApiKey, WorkflowFormatUi, to_sdk_error, not theQueueFull, ...the ticket quotes). I matched the code, not the line numbers.mainrather than stacking. feat: collect a Router deadline 504 under the same Idempotency-Key by default #99 is also merged and touchesmodels.py/retry.pyonly — no overlap.Residual
Idempotency-Keywithout stamping it.AssetHandlemints one per handle (src/comfy_sdk/assets.py:69) and passes it topost_assets(assets.py:139,assets.py:185) inside a barewith 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 tosubmit), but it makes the flat sentence the ticket asked for in the docs (Noneon operations that "send no key") untrue, so I did not write it: theComfyError.idempotency_keydocstring 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.spec/openapi.yamlandspec/router-openapi.yaml, not observed against Comfy Cloud.tests/integrationneeds a live target and a credential and was not run.Provenance
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/integrationnot run (needs a live deployment + credential).server.state.idempotencyassertion 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.