[PR #152] [MERGED] fix(models): stamp the Idempotency-Key onto every error models.run / models.submit throws #154

New issue

Closed

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

bsakaguchi commented

2026-10-31 22:05:05 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-typescript-sdk/pull/152
Author: @mattmillerai
Created: 9/17/2026
Status: ✅ Merged
Merged: 10/9/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-15123-stamp-idempotency-key

📝 Commits (7)

fddafc6 fix(models): stamp the Idempotency-Key onto every error models.run / models.submit throws
9017304 Merge origin/main into matt/be-15123-stamp-idempotency-key
d0725c1 fix(models): never stamp a key onto an abort reason other calls share
1825a93 Merge origin/main into matt/be-15123-stamp-idempotency-key
a10627b fix(exceptions): leave a caller's own Error subclass unreplicated
e076784 docs(readme): say that run/submit transport failures and aborts carry idempotencyKey, and list it on RouterError
7a730ef Merge origin/main into matt/be-15123-stamp-idempotency-key

📊 Changes

9 files changed (+940 additions, -118 deletions)

View changed files

📝 CHANGELOG.md (+1 -0)
📝 README.md (+14 -3)
📝 src/sdk/exceptions.test.ts (+264 -0)
📝 src/sdk/exceptions.ts (+239 -0)
📝 src/sdk/modelRequests.test.ts (+96 -0)
📝 src/sdk/modelRequests.ts (+30 -13)
📝 src/sdk/models.test.ts (+146 -0)
📝 src/sdk/models.ts (+131 -101)
📝 src/sdk/routerErrors.ts (+19 -1)

📄 Description

ELI-5

When a comfy.models.run(...) or comfy.models.submit(...) call blows up, you often want to know which request it was so you can look it up on the server or safely re-ask for the generation it may still be running. The Idempotency-Key is that handle. Until now only ONE kind of failure carried it — the case where the server answered with an error. The scarier failures — the network dropping the socket mid-request, an already-aborted signal, or a bare 502 on the queue path — threw a raw error with no key on it at all, and on a dropped connection the server never even minted a request id, so there was literally nothing left to correlate the failure with. This PR stamps the Idempotency-Key onto every error those two calls can throw, so a caller (and the cloud E2E harness that reads err.idempotencyKey) always has it.

What changed

New internal helper pair in src/sdk/exceptions.ts (not exported from the package root — the public surface is unchanged, so surface-parity.test.ts needs no edit):
- stampIdempotencyKey(exc, key, options?) — attaches key as an own idempotencyKey property on the raw throwable, in place, returning the same object with the same stack. It never overwrites a value already set (so a ComfyError built WITH a key keeps it), a null key writes nothing, and a non-object, a non-extensible throwable (a string, a frozen object) or one whose write raises passes through untouched. Except when the throwable is one the caller owns and other calls share — see "Shared abort reasons" below. It also defaults requestId/retryAfter to null when absent, so a stamped transport error reads every attribute a caller expects — without clobbering one it already carried.
- stamping(key, fn) — runs fn, stamping anything it throws. Mirrors the second arm of the Python SDK's translating(idempotency_key=…).
models.run wraps its whole retry loop in stamping(idempotencyKey, …). The pre-mint input/credential checks stay outside and unstamped (there is no key yet), exactly as in Python. The three existing ComfyError constructions already carry the key, so the stamp is a no-op on them.
models.submit wraps send() + decode() + new RequestHandle(...) in stamping(...), so both the transport re-throw and the RouterError from decode leave stamped. models.subscribe calls submit and inherits it. handle() / RequestHandle methods are addressed by requestId, not the key, and are left alone. The request_timeout ComfyError in send() now also receives call.idempotencyKey directly.
routerErrors.RouterError gains a typed readonly idempotencyKey: string | null field (plus the options entry and a class doc mention), so the stamped queue-path error is typed rather than duck-typed.
Doc + CHANGELOG.md (Unreleased → Fixed) updated.

The rationale for stamping in place rather than through a constructor argument: the transport failures are undici's own TypeError / DOMException, which this SDK does not construct and cannot catch by subclass. ComfyError.idempotencyKey is readonly at the type level only; the write through the Record cast is deliberate and guarded so it never overwrites a non-null value.

Shared abort reasons (review round)

The panel raised one High finding that changed the design, and it was right. fetch rejects with signal.reason, and AbortSignal.any propagates the SOURCE signal's reason object, not a copy — verified directly: AbortSignal.any([caller, timeout]).reason === caller.signal.reason is true. So N concurrent run/submit calls sharing one AbortController all reject with one DOMException, the first call to reach the stamp decided the key every caller then read, and the stamp permanently mutated caller-owned state. A manual collect under someone else's key re-asks for someone else's generation while your own stays uncollectable.

stampIdempotencyKey now takes StampOptions.callerSignal, wired from both models.run and modelRequests.submit. When the throwable is that signal's reason, the call receives a private stand-in carrying its own key, and controller.signal.reason is left exactly as the caller built it. The stand-in keeps the same prototype, name, message, stack and own properties, so everything a caller branches on — err.name === "AbortError", instanceof DOMException, instanceof TypeError — reads identically; only object identity differs. An error already carrying a different call's key takes the same path, as belt and braces for a shared throwable arriving by a route not plumbed.

Worth recording: the stand-in cannot be a descriptor copy. DOMException keeps name/message in internal slots behind prototype accessors, so Object.defineProperties(Object.create(proto), descriptors) yields an object that throws Value of `this` must be of DOMException the moment anyone reads err.name — precisely the property callers branch on. It is rebuilt through the constructor instead, and stack (an accessor bound to its original on both Error and DOMException) is pinned across as a data property. A throwable that cannot be faithfully reproduced travels on UNSTAMPED: no key beats another generation's key. That includes a caller's own Error subclass: a descriptor copy never runs its constructor, so #private state is missing and an accessor reading it would throw on the stand-in. Only the built-in error classes, this SDK's own ComfyError classes and plain objects are replicated (follow-up to a CodeRabbit finding).

The other three findings are fixed too — the stamp's writes are wrapped (an own non-writable idempotencyKey holding undefined, a getter-only accessor or a Proxy with a throwing set trap used to raise from inside stamping's catch and REPLACE the transport failure); a transport failure that exhausts the collect budget now carries the Retry-After pace collectingAt holds rather than defaulting it to null; and requestId/retryAfter use value checks like idempotencyKey, so an own requestId: undefined reads back as null.

How this was verified

pnpm typecheck — clean.
pnpm lint (oxlint) — clean.
pnpm exec oxfmt --check . — clean.
pnpm test — 27 files, 671 tests passed (0 failed), including the new cases:
- exceptions.test.ts — the stamp defaults requestId/retryAfter to null on a bare Error; never overwrites an existing retryAfter/idempotencyKey; a null key writes nothing; a frozen object and a string throwable pass through unchanged. These are the TS twins of the Python test_error_contract.py test_a_stamped_transport_error_reads_every_attribute_as_none and test_the_stamp_never_overwrites_an_attribute_that_is_already_set.
- models.test.ts — a mid-request socket destroy (retry: false) rejects with a TypeError whose message is "fetch failed", idempotencyKey is the caller's key (and, in a second case, the SDK-minted key), and requestId/retryAfter are null; a pre-aborted signal rejects with a DOMException AbortError carrying the key; a bare 502 still carries the key AND the retryAfter read off the header (the stamp does not overwrite it).
- modelRequests.test.ts — the same transport + abort cases on models.submit; a 502 → ProviderError with idempotencyKey set; a spent deadline → ComfyError code request_timeout with idempotencyKey set.

Twelve further tests were added in the review round, each verified to FAIL against the pre-fix code: concurrent run/submit on one AbortController reading their own keys (pre-fix: expected 'k-one' to be 'k-two'), stand-in fidelity, plain-object and exotic (Map) abort reasons, the unwritable and throwing-set-trap throwables, the uniform undefined -> null default, and the collect-budget failure carrying the pace (pre-fix: expected null to be 1).

Provenance

Authored by: agent-work loop
Verified:
- Head a10627b (origin/main @ 9d2fa64 merged in): pnpm lint, pnpm format:check, pnpm typecheck, pnpm check:spec-drift all clean.
- pnpm test: 714 passed, 0 failed, 5 skipped. The new custom-Error-subclass test fails against the previous replicate.
- sdk-parity is red on main as well, so this PR doesn't cause it. #167 taught the extractor about the Python SDK's RouterRunResult, but the committed Python snapshot has gone stale since then. #201 refreshes it.
Deviations:
- Where the caller's abort reason is shared across concurrent calls, each call gets its own equivalent error instead of having the shared object mutated. Every other error is stamped in place, so its identity is preserved.
- An error whose state can't be faithfully reproduced (an exotic object, or a caller's own Error subclass) is returned UNSTAMPED rather than mutated.

Residual

Not defects in this change — verification I could not perform from the sandbox, named so a reviewer can close the loop:

Cloud Router E2E harness (Comfy-Org/cloud, testing/e2e/router_sdk/typescript/src/harness.ts, observe(exc)). The motivating consumer reads err.idempotencyKey by property; this PR exposes exactly that own property on every error path, but I could not run that harness (separate private repo, no access here), so the end-to-end fix that the EDGE rows stop recording null is asserted by unit test rather than exercised against the harness itself.
Python SDK parity (comfy-python-sdk src/comfy_sdk/exceptions.py translating/_stamp, tests/test_error_contract.py). The behaviour mirrors the Python contract as described, and the two new TS tests are named as twins of the two Python tests, but I could not read the Python source or run its suite to confirm the assertions match byte-for-byte.
Repro method. The original report measured this by building dist/index.js and driving it against a closed loopback port. I reproduced the same raw-transport-failure path in-tree via the existing RouterStubServer mid-request socket destroy (resetTimes) rather than a built dist against a real closed port; the code paths exercised are identical.
Overlapping PR router-provider-params (#150). RESOLVED — it merged as 21454a3 and origin/main is merged into this branch. The only textual conflict was CHANGELOG.md (both added an Unreleased section); both are kept. models.run composes correctly: #150's routerRunQuery is built pre-mint and so stays outside the stamping wrapper, matching this branch's rule that pre-mint checks are unstamped. Full suite green on the merged tree.

Summary by CodeRabbit

New Features
- Errors from comfy.models.run and comfy.models.submit now include that call’s idempotency key, including transport, abort, timeout, and Router failures. This makes failed requests easier to identify.
- Router errors expose the idempotency key, or null when none applies.
- When generation collection encounters a transport failure after Router provides a Retry-After pace, the failure retains that value.
- Shared abort reasons remain unchanged; calls receive separate keyed errors when the reason can be safely copied.

_{🔄 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-typescript-sdk/pull/152 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/17/2026 **Status:** ✅ Merged **Merged:** 10/9/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-15123-stamp-idempotency-key` --- ### 📝 Commits (7) - [`fddafc6`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/fddafc65c4133ec6cd6fb0af8eab56871a69b960) fix(models): stamp the Idempotency-Key onto every error models.run / models.submit throws - [`9017304`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/9017304133f631ddcf63c01731002ddb1edac336) Merge origin/main into matt/be-15123-stamp-idempotency-key - [`d0725c1`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/d0725c1f9928e61de9d4b040973d308e6aad9f85) fix(models): never stamp a key onto an abort reason other calls share - [`1825a93`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/1825a93989e03342d49868cfeecf96646f144be4) Merge origin/main into matt/be-15123-stamp-idempotency-key - [`a10627b`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/a10627bd36bd4d9ae9fa2c8ecfc39ef8c66b9150) fix(exceptions): leave a caller's own Error subclass unreplicated - [`e076784`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/e07678450eb8e9f8f447e5bf24ee116d2d0c81eb) docs(readme): say that run/submit transport failures and aborts carry idempotencyKey, and list it on RouterError - [`7a730ef`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/7a730ef40de2e3226fabbab8a0fceff7f87da439) Merge origin/main into matt/be-15123-stamp-idempotency-key ### 📊 Changes **9 files changed** (+940 additions, -118 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+1 -0) 📝 `README.md` (+14 -3) 📝 `src/sdk/exceptions.test.ts` (+264 -0) 📝 `src/sdk/exceptions.ts` (+239 -0) 📝 `src/sdk/modelRequests.test.ts` (+96 -0) 📝 `src/sdk/modelRequests.ts` (+30 -13) 📝 `src/sdk/models.test.ts` (+146 -0) 📝 `src/sdk/models.ts` (+131 -101) 📝 `src/sdk/routerErrors.ts` (+19 -1) </details> ### 📄 Description ## ELI-5 When a `comfy.models.run(...)` or `comfy.models.submit(...)` call blows up, you often want to know which request it was so you can look it up on the server or safely re-ask for the generation it may still be running. The `Idempotency-Key` is that handle. Until now only ONE kind of failure carried it — the case where the server answered with an error. The scarier failures — the network dropping the socket mid-request, an already-aborted signal, or a bare `502` on the queue path — threw a raw error with no key on it at all, and on a dropped connection the server never even minted a request id, so there was literally nothing left to correlate the failure with. This PR stamps the `Idempotency-Key` onto every error those two calls can throw, so a caller (and the cloud E2E harness that reads `err.idempotencyKey`) always has it. ## What changed - **New internal helper pair in `src/sdk/exceptions.ts`** (not exported from the package root — the public surface is unchanged, so `surface-parity.test.ts` needs no edit): - `stampIdempotencyKey(exc, key, options?)` — attaches `key` as an own `idempotencyKey` property on the raw throwable, in place, returning the same object with the same stack. It never overwrites a value already set (so a `ComfyError` built WITH a key keeps it), a `null` key writes nothing, and a non-object, a non-extensible throwable (a string, a frozen object) or one whose write raises passes through untouched. **Except when the throwable is one the caller owns and other calls share** — see "Shared abort reasons" below. It also defaults `requestId`/`retryAfter` to `null` when absent, so a stamped transport error reads every attribute a caller expects — without clobbering one it already carried. - `stamping(key, fn)` — runs `fn`, stamping anything it throws. Mirrors the second arm of the Python SDK's `translating(idempotency_key=…)`. - **`models.run`** wraps its whole retry loop in `stamping(idempotencyKey, …)`. The pre-mint input/credential checks stay outside and unstamped (there is no key yet), exactly as in Python. The three existing `ComfyError` constructions already carry the key, so the stamp is a no-op on them. - **`models.submit`** wraps `send()` + `decode()` + `new RequestHandle(...)` in `stamping(...)`, so both the transport re-throw and the `RouterError` from `decode` leave stamped. `models.subscribe` calls `submit` and inherits it. `handle()` / `RequestHandle` methods are addressed by `requestId`, not the key, and are left alone. The `request_timeout` `ComfyError` in `send()` now also receives `call.idempotencyKey` directly. - **`routerErrors.RouterError`** gains a typed `readonly idempotencyKey: string | null` field (plus the options entry and a class doc mention), so the stamped queue-path error is typed rather than duck-typed. - Doc + `CHANGELOG.md` (Unreleased → Fixed) updated. The rationale for stamping in place rather than through a constructor argument: the transport failures are undici's own `TypeError` / `DOMException`, which this SDK does not construct and cannot catch by subclass. `ComfyError.idempotencyKey` is `readonly` at the type level only; the write through the `Record` cast is deliberate and guarded so it never overwrites a non-null value. ## Shared abort reasons (review round) The panel raised one High finding that changed the design, and it was right. `fetch` rejects with `signal.reason`, and `AbortSignal.any` propagates the SOURCE signal's reason **object**, not a copy — verified directly: `AbortSignal.any([caller, timeout]).reason === caller.signal.reason` is `true`. So N concurrent `run`/`submit` calls sharing one `AbortController` all reject with one `DOMException`, the first call to reach the stamp decided the key **every** caller then read, and the stamp permanently mutated caller-owned state. A manual collect under someone else's key re-asks for someone else's generation while your own stays uncollectable. `stampIdempotencyKey` now takes `StampOptions.callerSignal`, wired from both `models.run` and `modelRequests.submit`. When the throwable is that signal's reason, the call receives a private **stand-in** carrying its own key, and `controller.signal.reason` is left exactly as the caller built it. The stand-in keeps the same prototype, `name`, message, stack and own properties, so everything a caller branches on — `err.name === "AbortError"`, `instanceof DOMException`, `instanceof TypeError` — reads identically; only object identity differs. An error already carrying a *different* call's key takes the same path, as belt and braces for a shared throwable arriving by a route not plumbed. Worth recording: the stand-in **cannot** be a descriptor copy. `DOMException` keeps `name`/`message` in internal slots behind prototype accessors, so `Object.defineProperties(Object.create(proto), descriptors)` yields an object that throws ``Value of `this` must be of DOMException`` the moment anyone reads `err.name` — precisely the property callers branch on. It is rebuilt through the constructor instead, and `stack` (an accessor bound to its original on both `Error` and `DOMException`) is pinned across as a data property. A throwable that cannot be faithfully reproduced travels on UNSTAMPED: no key beats another generation's key. That includes a caller's own `Error` subclass: a descriptor copy never runs its constructor, so `#private` state is missing and an accessor reading it would throw on the stand-in. Only the built-in error classes, this SDK's own `ComfyError` classes and plain objects are replicated (follow-up to a CodeRabbit finding). The other three findings are fixed too — the stamp's writes are wrapped (an own non-writable `idempotencyKey` holding `undefined`, a getter-only accessor or a `Proxy` with a throwing `set` trap used to raise from inside `stamping`'s catch and REPLACE the transport failure); a transport failure that exhausts the collect budget now carries the `Retry-After` pace `collectingAt` holds rather than defaulting it to `null`; and `requestId`/`retryAfter` use value checks like `idempotencyKey`, so an own `requestId: undefined` reads back as `null`. ## How this was verified - `pnpm typecheck` — clean. - `pnpm lint` (oxlint) — clean. - `pnpm exec oxfmt --check .` — clean. - `pnpm test` — 27 files, 671 tests passed (0 failed), including the new cases: - `exceptions.test.ts` — the stamp defaults `requestId`/`retryAfter` to `null` on a bare `Error`; never overwrites an existing `retryAfter`/`idempotencyKey`; a `null` key writes nothing; a frozen object and a string throwable pass through unchanged. These are the TS twins of the Python `test_error_contract.py` `test_a_stamped_transport_error_reads_every_attribute_as_none` and `test_the_stamp_never_overwrites_an_attribute_that_is_already_set`. - `models.test.ts` — a mid-request socket destroy (`retry: false`) rejects with a `TypeError` whose `message` is `"fetch failed"`, `idempotencyKey` is the caller's key (and, in a second case, the SDK-minted key), and `requestId`/`retryAfter` are `null`; a pre-aborted signal rejects with a `DOMException` `AbortError` carrying the key; a bare `502` still carries the key AND the `retryAfter` read off the header (the stamp does not overwrite it). - `modelRequests.test.ts` — the same transport + abort cases on `models.submit`; a `502` → `ProviderError` with `idempotencyKey` set; a spent deadline → `ComfyError code request_timeout` with `idempotencyKey` set. Twelve further tests were added in the review round, each verified to FAIL against the pre-fix code: concurrent `run`/`submit` on one `AbortController` reading their own keys (pre-fix: ``expected 'k-one' to be 'k-two'``), stand-in fidelity, plain-object and exotic (`Map`) abort reasons, the unwritable and throwing-set-trap throwables, the uniform `undefined` -> `null` default, and the collect-budget failure carrying the pace (pre-fix: `expected null to be 1`). ## Provenance - **Authored by:** agent-work loop - **Verified:** - Head `a10627b` (`origin/main` @ `9d2fa64` merged in): `pnpm lint`, `pnpm format:check`, `pnpm typecheck`, `pnpm check:spec-drift` all clean. - `pnpm test`: 714 passed, 0 failed, 5 skipped. The new custom-`Error`-subclass test fails against the previous `replicate`. - `sdk-parity` is red on `main` as well, so this PR doesn't cause it. #167 taught the extractor about the Python SDK's `RouterRunResult`, but the committed Python snapshot has gone stale since then. #201 refreshes it. - **Deviations:** - Where the caller's abort reason is shared across concurrent calls, each call gets its own equivalent error instead of having the shared object mutated. Every other error is stamped in place, so its identity is preserved. - An error whose state can't be faithfully reproduced (an exotic object, or a caller's own `Error` subclass) is returned UNSTAMPED rather than mutated. ## Residual Not defects in this change — verification I could not perform from the sandbox, named so a reviewer can close the loop: - **Cloud Router E2E harness (`Comfy-Org/cloud`, `testing/e2e/router_sdk/typescript/src/harness.ts`, `observe(exc)`).** The motivating consumer reads `err.idempotencyKey` by property; this PR exposes exactly that own property on every error path, but I could not run that harness (separate private repo, no access here), so the end-to-end fix that the EDGE rows stop recording `null` is asserted by unit test rather than exercised against the harness itself. - **Python SDK parity (`comfy-python-sdk` `src/comfy_sdk/exceptions.py` `translating`/`_stamp`, `tests/test_error_contract.py`).** The behaviour mirrors the Python contract as described, and the two new TS tests are named as twins of the two Python tests, but I could not read the Python source or run its suite to confirm the assertions match byte-for-byte. - **Repro method.** The original report measured this by building `dist/index.js` and driving it against a closed loopback port. I reproduced the same raw-transport-failure path in-tree via the existing `RouterStubServer` mid-request socket destroy (`resetTimes`) rather than a built `dist` against a real closed port; the code paths exercised are identical. - **Overlapping PR `router-provider-params` (#150).** RESOLVED — it merged as `21454a3` and `origin/main` is merged into this branch. The only textual conflict was `CHANGELOG.md` (both added an `Unreleased` section); both are kept. `models.run` composes correctly: #150's `routerRunQuery` is built pre-mint and so stays outside the `stamping` wrapper, matching this branch's rule that pre-mint checks are unstamped. Full suite green on the merged tree.  ## Summary by CodeRabbit * **New Features** * Errors from `comfy.models.run` and `comfy.models.submit` now include that call’s idempotency key, including transport, abort, timeout, and Router failures. This makes failed requests easier to identify. * Router errors expose the idempotency key, or `null` when none applies. * When generation collection encounters a transport failure after Router provides a `Retry-After` pace, the failure retains that value. * Shared abort reasons remain unchanged; calls receive separate keyed errors when the reason can be safely copied.  --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 22:05:05 +00:00