[PR #126] [MERGED] feat: collect a still-running generation under the same Idempotency-Key #128

New issue

Closed

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

bsakaguchi commented

2026-10-31 22:04:49 +00:00

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-11972-collect-same-key-409-504

📝 Commits (3)

a784af3 feat: collect a still-running generation under the same Idempotency-Key
eef1fcf fix: address bot review comments
f15729e Merge remote-tracking branch 'origin/main' into matt/be-11972-collect-same-key-409-504

📊 Changes

17 files changed (+1309 additions, -57 deletions)

View changed files

📝 AGENTS.md (+14 -5)
📝 CHANGELOG.md (+41 -0)
📝 README.md (+63 -9)
📝 parity/python-surface.json (+13 -2)
📝 scripts/python-surface.mjs (+54 -1)
📝 scripts/router-route-contract.mjs (+45 -0)
📝 src/sdk/exceptions.ts (+48 -4)
📝 src/sdk/index.ts (+1 -0)
📝 src/sdk/models.test.ts (+261 -2)
📝 src/sdk/models.ts (+144 -19)
📝 src/sdk/retry.test.ts (+116 -1)
📝 src/sdk/retry.ts (+157 -6)
📝 src/sdk/router-spec-contract.test.ts (+25 -0)
📝 src/sdk/routerErrors.test.ts (+87 -0)
📝 src/sdk/routerErrors.ts (+105 -6)
📝 src/sdk/surface-parity.test.ts (+94 -1)
📝 test/support/router-stub-server.ts (+41 -1)

📄 Description

ELI-5

You ask Comfy to make a video. It takes ten minutes, and your connection to it dies at minute nine — or Comfy itself gives up holding the line at its own ten-minute limit. The video is still being made, and you are still paying for it. Comfy knows this and says so: it answers with "that one is still running, wait N seconds and ask me again using the same ticket number". This SDK was hanging up instead — it threw those answers straight at you and your video was gone. Now it waits the seconds it was told to wait, hands back the same ticket, and gives you the video when it arrives. Nothing to turn on.

Description

Comfy Router pairs exactly two failures with a Retry-After, and that pairing means one specific thing: the generation your Idempotency-Key already names has not finished, so re-send THAT key and collect it rather than dispatching a second one.

409 + X-Comfy-Error-Type: concurrency_limit_exceeded — another call is already in flight for this key. It is what a same-key re-send after a dropped connection meets.
504 + X-Comfy-Error-Type: deadline_exceeded — Comfy stopped holding the connection at its own bound while the provider carried on generating.

Both were terminal in this SDK. The 409 because isRetryableStatus refuses every sub-500 status; the 504 because DEFAULT_RETRY_BUDGET_MS (120 s) is measured from the first attempt and is long spent by the time a ten-minute deadline produces one, and DEFAULT_RUN_TIMEOUT_MS (600 s) equalled that deadline besides. So DeadlineExceeded's own docstring promised a collect the SDK could not perform, and a caller lost a generation that had already been dispatched and charged. The sibling Python SDK has had this loop for a while; this closes that gap.

What changed

Retry-After is parsed once and rides onto the errors. A strict parser (parseRetryAfter, exported from routerErrors.ts) reads only the integer-seconds form the contract pins (minimum: 1) — deliberately not parseInt, which reads "2 hours" as a two-second pace nothing named — and an HTTP-date, decimal or negative reads as absent. 0 is KEPT rather than flattened to null, because the two say different things: null means nothing to collect, 0 means there is a handle but the pace is nonsense. It lands on RouterError.retryAfter and on ComfyError.retryAfter, and models.ts and toRouterError share the one helper.
isCollectable(status, errorType, retryAfter) in retry.ts, sans-IO beside isRetryableStatus. Every gate is load-bearing: the bucket is required on both statuses because each is shared with a deterministic refusal (504/provider_timeout; the 409 invalid_input key cases, answered by a NEW key), and the header is required because Comfy sends it only where it holds something to collect.
A separate budget. retry.collectBudgetMs, default 1_200_000 — two Comfy deadline windows, one to reach the 504 and one to collect what it left running, since a 504 arrives AT the first window's end. Measured from the same first attempt as budgetMs; ordinary retries still give up at budgetMs. collectBudgetMs: 0 switches the collect off alone; retry: false / budgetMs: 0 still means exactly one attempt.
nextCollectDelayMs honours the server's pace verbatim (it knows more about the generation than any schedule here), falls back to the jittered backoff on a Retry-After: 0 so a confused value can never become a spin of full model-run POSTs, and CLAMPS to what is left of the collect budget rather than refusing — one unlucky draw must not end a call that still has budget.
Wired into models.run, collect-first and exclusively. A deadline_exceeded 504 is also a 5xx, so both branches would take it; the server's verdict about what it is holding wins over this module's guess, and a collect that exhausts its budget raises rather than falling back into the ordinary backoff. When it does raise, it is the server's own last 409/504 — never a synthetic "retries exhausted".
DEFAULT_RUN_TIMEOUT_MS 600 s → 1 200 s. Not a drive-by: the deadline covers every attempt of a call rather than each one afresh, so a ten-minute default is exactly spent when a deadline_exceeded 504 lands and step 5 could never start under it. The deadline stays the OUTERMOST bound, ahead of collectBudgetMs — a caller-supplied timeoutMs shorter than Comfy's ten minutes therefore forfeits the 504 collect, which is documented on the option. (The 409 collect works fine inside a short deadline; that answer arrives in milliseconds, not at a ten-minute bound. The rejected alternative was making the deadline per-attempt like Python's timeout, which contradicts composeSignal's stated contract and its test.)
retryAfter and idempotencyKey on every ComfyError, so a caller who outlived the budget holds both halves of a manual re-ask. The key one run mints for you was previously not visible anywhere, which meant an interrupted call could not be re-asked for at all.

Chesterton's fence

DEFAULT_RUN_TIMEOUT_MS = 600_000 traces to #76, whose commit message states the reasoning: minutes rather than seconds "because the finished generation is the response". This change extends that same reasoning by one Comfy deadline window rather than contradicting it — a deadline is a ceiling, not a wait, so no successful call is slowed by it. isRetryableStatus itself is UNCHANGED: it still answers false for every sub-500 status, so a header-less 409 and an invalid_input 409 stay terminal exactly as before. The 409 was removed only from that function's prose never-retry list, because the status now covers two opposite answers and only one of them is isRetryableStatus's business.

The status set is pinned, not chosen

Which statuses may be collected is not this SDK's decision, so nothing here restates it unchecked. scripts/router-route-contract.mjs grows a reader for which runRouterModel responses declare a Retry-After header, and router-spec-contract.test.ts asserts isCollectable accepts exactly those ([409, 504]) — a Router sync that moved the header onto a 429, or off the 409, reddens by name instead of leaving the SDK re-sending an answer nothing blesses. AGENTS.md's spec-coupled list gains the entry.

Cross-SDK: parity/python-surface.json now records the Python RetryPolicy dataclass's fields and defaults, and surface-parity.test.ts compares collect_max_elapsed (1200.0 s) against DEFAULT_COLLECT_BUDGET_MS (1 200 000 ms) as a VALUE, so the one number whose correctness is the whole feature cannot drift between the two languages. The differing off-switch (Python's retry_collectable=False vs. collectBudgetMs: 0) is declared in INTENTIONAL_ASYMMETRIES with its reason, and the rot guard fails if either side stops matching. The manifest was regenerated with pnpm sync:python-surface against live main, and pnpm check:sdk-parity (what the daily CI job runs) confirms it.

Negative-claim falsification

Not triggered, and stated so it is not assumed away: this diff ADDS a capability and denies none. It introduces no "not supported"/"unavailable"/dead-end string, no new throw path, and no test flipped to assert a dead-end. The one denial-shaped assertion — a header-less 409 stays one attempt — is the PRE-EXISTING behaviour on main, which models.test.ts already pinned; it was split out of a table into its own test, not newly introduced.

How this was tested

The artifacts the work names were exercised directly rather than only around the diff:

The vendored contract (spec/router-openapi.yaml) — read, and now machine-read by the new contract test: RouterRetryAfterHeader is type: integer, minimum: 1; RouterIdempotencyConflict (409) and RouterDeadlineExceeded (504) are the only runRouterModel responses declaring the header, and they are the only two whose prose says to re-send the same key.
The Python reference implementation (src/comfy_sdk/retry.py on the public sibling repo) — fetched and read at main. Its is_collectable gates, its min(delay, remaining) clamp, its Retry-After: 0 fallback and its collect_max_elapsed = 1200.0 are what this mirrors; the parity test now reads that value out of it on every run.
The wire behaviour, through the existing stub-server pattern in models.test.ts: a 409 collect twice then 200 with Idempotent-Replayed: true (3 requests, one key across all three), a 504 collect, a 504 with NO Retry-After taking the ordinary 5xx path (asserted on the clock, since the outcome alone cannot tell them apart), a 409 loop outliving collectBudgetMs, an abort mid-collect-wait, retry: false, collectBudgetMs: 0, and a short timeoutMs beating a generous collect budget.

Provenance

Authored by: agent-work loop
Verified: pnpm test: 462 passed, 0 failed (24 files); pnpm typecheck, pnpm lint, pnpm format:check, pnpm build: clean; pnpm check:spec-drift: both checks OK; pnpm check:sdk-parity: snapshot matches the Python SDK's live main; node scripts/check-public-repo-hygiene.mjs: no internal-only references.
Deviations: one, named under Residual below (the class a budget-exhausted 409 rejects with).

Residual

Not fixed here, and worth its own follow-up:

A budget-exhausted collect rejects with ComfyError, not routerErrors.ConcurrencyLimitExceeded. The plan for this work asked the new test to assert the latter. It cannot today, and the reason is structural rather than a shortcut: toRouterError — the factory that produces the routerErrors.* classes — is not on models.run's path at all. models.ts has its own errorFromResponse, whose BY_ERROR_TYPE maps four buckets (unauthorized, forbidden, insufficient_credits, model_not_found) to ComfyError subclasses and leaves everything else a ComfyError carrying the bucket as code. Routing models.run through toRouterError would change the class every existing models.run failure throws, which is a breaking change to the public error surface and well outside this change. So the test asserts code === "concurrency_limit_exceeded" plus httpStatus 409, retryAfter 1 and the server's requestId — every value the criterion named except the class. Unifying models.run's error factory with routerErrors.toRouterError is the real follow-up, and it is a semver-major decision about the catchable surface, not a cleanup: today a caller of models.run catches ComfyError and branches on .code, while routerErrors.* is a parallel hierarchy nothing on that path constructs. Whoever picks it up should decide deliberately whether the two converge or stay separate, and say so in routerErrors.ts's module docstring either way — right now nothing in the repo states which is intended.
Two Retry-After parsers now exist in this repo, with different strictness. The one added here (routerErrors.parseRetryAfter) is strict; src/low/transport.ts keeps its own parseRetryAfter, which uses Number.parseInt and so reads "2 hours" as a two-second pace. That one serves the Comfy API v2 job/asset surface and feeds QueueFull.retryAfter and the SSE reconnect pause — a different contract, whose Retry-After this change does not touch. Unifying them would change v2 submit()/reconnect behaviour for malformed headers and was deliberately left alone; it is a small, self-contained follow-up.
Deliberately out of scope, per the plan for this work, and unchanged here: retrying a 429 + Retry-After in models.run; changing this SDK's default of retrying a thrown fetch or a bucket-less 5xx under the same key (Python does not); and the cloud end-to-end harness classifier, where a 409 that survives the collect budget is still classified correctly.
Unexercised artifacts. No call was made against a live Comfy Router: this environment has no Router credential, and the acceptance shapes are a mid-generation 409 and a ten-minute 504, neither of which can be provoked without dispatching (and paying for) real partner-model generations — so they are verified against the vendored contract and the stub server rather than in production. The pnpm check:spec-drift and pnpm check:sdk-parity gates are what keep the stub's fixtures honest against the two upstream sources. Two further items named in the plan for this work could not be read from here: the cloud-side middleware said to raise the in-flight 409, which lives in a private repo, and a linked spike ticket whose findings comment is not reachable from this sandbox. Both are cited in that plan only as evidence for gates this change reads directly out of spec/router-openapi.yaml, which IS in this repo and IS now machine-checked — so nothing rests on the unread copies, but neither was independently confirmed.

Summary by CodeRabbit

New Features
- Added server-directed collection retries for eligible 409 and 504 responses using Retry-After.
- Collection retries reuse the same idempotency key and have a separate 20-minute budget.
- Errors now expose retryAfter and idempotencyKey metadata.
- Router errors include parsed Retry-After delays.
Bug Fixes
- Improved retry handling for concurrency-limit and deadline-exceeded responses.
- Preserved retry metadata across timeout, queued, and invalid-response errors.
Documentation
- Updated model-run retry guidance, deadlines, collection controls, and manual recovery instructions.
- Increased the default model-run deadline from 10 to 20 minutes.

_{🔄 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/126 **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-11972-collect-same-key-409-504` --- ### 📝 Commits (3) - [`a784af3`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/a784af39e95b4ce2b39d3cd4b448d7db8e82e2ec) feat: collect a still-running generation under the same Idempotency-Key - [`eef1fcf`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/eef1fcf0876884ea28de12fae3260c5cad6d9c4a) fix: address bot review comments - [`f15729e`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/f15729e68166b84a0bbf1571334db18108db7b20) Merge remote-tracking branch 'origin/main' into matt/be-11972-collect-same-key-409-504 ### 📊 Changes **17 files changed** (+1309 additions, -57 deletions) <details> <summary>View changed files</summary> 📝 `AGENTS.md` (+14 -5) 📝 `CHANGELOG.md` (+41 -0) 📝 `README.md` (+63 -9) 📝 `parity/python-surface.json` (+13 -2) 📝 `scripts/python-surface.mjs` (+54 -1) 📝 `scripts/router-route-contract.mjs` (+45 -0) 📝 `src/sdk/exceptions.ts` (+48 -4) 📝 `src/sdk/index.ts` (+1 -0) 📝 `src/sdk/models.test.ts` (+261 -2) 📝 `src/sdk/models.ts` (+144 -19) 📝 `src/sdk/retry.test.ts` (+116 -1) 📝 `src/sdk/retry.ts` (+157 -6) 📝 `src/sdk/router-spec-contract.test.ts` (+25 -0) 📝 `src/sdk/routerErrors.test.ts` (+87 -0) 📝 `src/sdk/routerErrors.ts` (+105 -6) 📝 `src/sdk/surface-parity.test.ts` (+94 -1) 📝 `test/support/router-stub-server.ts` (+41 -1) </details> ### 📄 Description ## ELI-5 You ask Comfy to make a video. It takes ten minutes, and your connection to it dies at minute nine — or Comfy itself gives up holding the line at its own ten-minute limit. The video is still being made, and you are still paying for it. Comfy knows this and says so: it answers with "that one is still running, wait N seconds and ask me again using the same ticket number". This SDK was hanging up instead — it threw those answers straight at you and your video was gone. Now it waits the seconds it was told to wait, hands back the same ticket, and gives you the video when it arrives. Nothing to turn on. ## Description Comfy Router pairs exactly two failures with a `Retry-After`, and that pairing means one specific thing: the generation your `Idempotency-Key` already names has not finished, so re-send THAT key and collect it rather than dispatching a second one. - **`409` + `X-Comfy-Error-Type: concurrency_limit_exceeded`** — another call is already in flight for this key. It is what a same-key re-send after a dropped connection meets. - **`504` + `X-Comfy-Error-Type: deadline_exceeded`** — Comfy stopped holding the connection at its own bound while the provider carried on generating. Both were terminal in this SDK. The `409` because `isRetryableStatus` refuses every sub-500 status; the `504` because `DEFAULT_RETRY_BUDGET_MS` (120 s) is measured from the first attempt and is long spent by the time a ten-minute deadline produces one, and `DEFAULT_RUN_TIMEOUT_MS` (600 s) equalled that deadline besides. So `DeadlineExceeded`'s own docstring promised a collect the SDK could not perform, and a caller lost a generation that had already been dispatched and charged. The sibling Python SDK has had this loop for a while; this closes that gap. ### What changed 1. **`Retry-After` is parsed once and rides onto the errors.** A strict parser (`parseRetryAfter`, exported from `routerErrors.ts`) reads only the integer-seconds form the contract pins (`minimum: 1`) — deliberately not `parseInt`, which reads `"2 hours"` as a two-second pace nothing named — and an HTTP-date, decimal or negative reads as absent. `0` is KEPT rather than flattened to `null`, because the two say different things: `null` means nothing to collect, `0` means there is a handle but the pace is nonsense. It lands on `RouterError.retryAfter` and on `ComfyError.retryAfter`, and `models.ts` and `toRouterError` share the one helper. 2. **`isCollectable(status, errorType, retryAfter)`** in `retry.ts`, sans-IO beside `isRetryableStatus`. Every gate is load-bearing: the bucket is required on both statuses because each is shared with a deterministic refusal (`504`/`provider_timeout`; the `409` `invalid_input` key cases, answered by a NEW key), and the header is required because Comfy sends it only where it holds something to collect. 3. **A separate budget.** `retry.collectBudgetMs`, default `1_200_000` — two Comfy deadline windows, one to reach the `504` and one to collect what it left running, since a `504` arrives AT the first window's end. Measured from the same first attempt as `budgetMs`; ordinary retries still give up at `budgetMs`. `collectBudgetMs: 0` switches the collect off alone; `retry: false` / `budgetMs: 0` still means exactly one attempt. 4. **`nextCollectDelayMs`** honours the server's pace verbatim (it knows more about the generation than any schedule here), falls back to the jittered backoff on a `Retry-After: 0` so a confused value can never become a spin of full model-run POSTs, and CLAMPS to what is left of the collect budget rather than refusing — one unlucky draw must not end a call that still has budget. 5. **Wired into `models.run`**, collect-first and exclusively. A `deadline_exceeded` `504` is also a 5xx, so both branches would take it; the server's verdict about what it is holding wins over this module's guess, and a collect that exhausts its budget raises rather than falling back into the ordinary backoff. When it does raise, it is the server's own last `409`/`504` — never a synthetic "retries exhausted". 6. **`DEFAULT_RUN_TIMEOUT_MS` 600 s → 1 200 s.** Not a drive-by: the deadline covers every attempt of a call rather than each one afresh, so a ten-minute default is exactly spent when a `deadline_exceeded` `504` lands and step 5 could never start under it. The deadline stays the OUTERMOST bound, ahead of `collectBudgetMs` — a caller-supplied `timeoutMs` shorter than Comfy's ten minutes therefore forfeits the `504` collect, which is documented on the option. (The `409` collect works fine inside a short deadline; that answer arrives in milliseconds, not at a ten-minute bound. The rejected alternative was making the deadline per-attempt like Python's `timeout`, which contradicts `composeSignal`'s stated contract and its test.) 7. **`retryAfter` and `idempotencyKey` on every `ComfyError`**, so a caller who outlived the budget holds both halves of a manual re-ask. The key one `run` mints for you was previously not visible anywhere, which meant an interrupted call could not be re-asked for at all. ### Chesterton's fence `DEFAULT_RUN_TIMEOUT_MS = 600_000` traces to #76, whose commit message states the reasoning: minutes rather than seconds "because the finished generation is the response". This change extends that same reasoning by one Comfy deadline window rather than contradicting it — a deadline is a ceiling, not a wait, so no successful call is slowed by it. `isRetryableStatus` itself is UNCHANGED: it still answers `false` for every sub-500 status, so a header-less `409` and an `invalid_input` `409` stay terminal exactly as before. The `409` was removed only from that function's prose never-retry list, because the status now covers two opposite answers and only one of them is `isRetryableStatus`'s business. ### The status set is pinned, not chosen Which statuses may be collected is not this SDK's decision, so nothing here restates it unchecked. `scripts/router-route-contract.mjs` grows a reader for which `runRouterModel` responses declare a `Retry-After` header, and `router-spec-contract.test.ts` asserts `isCollectable` accepts exactly those (`[409, 504]`) — a Router sync that moved the header onto a `429`, or off the `409`, reddens by name instead of leaving the SDK re-sending an answer nothing blesses. `AGENTS.md`'s spec-coupled list gains the entry. Cross-SDK: `parity/python-surface.json` now records the Python `RetryPolicy` dataclass's fields and defaults, and `surface-parity.test.ts` compares `collect_max_elapsed` (1200.0 s) against `DEFAULT_COLLECT_BUDGET_MS` (1 200 000 ms) as a VALUE, so the one number whose correctness is the whole feature cannot drift between the two languages. The differing off-switch (Python's `retry_collectable=False` vs. `collectBudgetMs: 0`) is declared in `INTENTIONAL_ASYMMETRIES` with its reason, and the rot guard fails if either side stops matching. The manifest was regenerated with `pnpm sync:python-surface` against live `main`, and `pnpm check:sdk-parity` (what the daily CI job runs) confirms it. ### Negative-claim falsification Not triggered, and stated so it is not assumed away: this diff ADDS a capability and denies none. It introduces no "not supported"/"unavailable"/dead-end string, no new throw path, and no test flipped to assert a dead-end. The one denial-shaped assertion — a header-less `409` stays one attempt — is the PRE-EXISTING behaviour on `main`, which `models.test.ts` already pinned; it was split out of a table into its own test, not newly introduced. ## How this was tested The artifacts the work names were exercised directly rather than only around the diff: - **The vendored contract** (`spec/router-openapi.yaml`) — read, and now machine-read by the new contract test: `RouterRetryAfterHeader` is `type: integer, minimum: 1`; `RouterIdempotencyConflict` (`409`) and `RouterDeadlineExceeded` (`504`) are the only `runRouterModel` responses declaring the header, and they are the only two whose prose says to re-send the same key. - **The Python reference implementation** (`src/comfy_sdk/retry.py` on the public sibling repo) — fetched and read at `main`. Its `is_collectable` gates, its `min(delay, remaining)` clamp, its `Retry-After: 0` fallback and its `collect_max_elapsed = 1200.0` are what this mirrors; the parity test now reads that value out of it on every run. - **The wire behaviour**, through the existing stub-server pattern in `models.test.ts`: a `409` collect twice then `200` with `Idempotent-Replayed: true` (3 requests, one key across all three), a `504` collect, a `504` with NO `Retry-After` taking the ordinary 5xx path (asserted on the clock, since the outcome alone cannot tell them apart), a `409` loop outliving `collectBudgetMs`, an abort mid-collect-wait, `retry: false`, `collectBudgetMs: 0`, and a short `timeoutMs` beating a generous collect budget. ## Provenance - **Authored by:** agent-work loop - **Verified:** `pnpm test`: 462 passed, 0 failed (24 files); `pnpm typecheck`, `pnpm lint`, `pnpm format:check`, `pnpm build`: clean; `pnpm check:spec-drift`: both checks OK; `pnpm check:sdk-parity`: snapshot matches the Python SDK's live `main`; `node scripts/check-public-repo-hygiene.mjs`: no internal-only references. - **Deviations:** one, named under Residual below (the class a budget-exhausted `409` rejects with). ## Residual Not fixed here, and worth its own follow-up: - **A budget-exhausted collect rejects with `ComfyError`, not `routerErrors.ConcurrencyLimitExceeded`.** The plan for this work asked the new test to assert the latter. It cannot today, and the reason is structural rather than a shortcut: `toRouterError` — the factory that produces the `routerErrors.*` classes — is **not on `models.run`'s path at all.** `models.ts` has its own `errorFromResponse`, whose `BY_ERROR_TYPE` maps four buckets (`unauthorized`, `forbidden`, `insufficient_credits`, `model_not_found`) to `ComfyError` subclasses and leaves everything else a `ComfyError` carrying the bucket as `code`. Routing `models.run` through `toRouterError` would change the class every existing `models.run` failure throws, which is a breaking change to the public error surface and well outside this change. So the test asserts `code === "concurrency_limit_exceeded"` plus `httpStatus 409`, `retryAfter 1` and the server's `requestId` — every value the criterion named except the class. **Unifying `models.run`'s error factory with `routerErrors.toRouterError` is the real follow-up**, and it is a semver-major decision about the catchable surface, not a cleanup: today a caller of `models.run` catches `ComfyError` and branches on `.code`, while `routerErrors.*` is a parallel hierarchy nothing on that path constructs. Whoever picks it up should decide deliberately whether the two converge or stay separate, and say so in `routerErrors.ts`'s module docstring either way — right now nothing in the repo states which is intended. - **Two `Retry-After` parsers now exist in this repo, with different strictness.** The one added here (`routerErrors.parseRetryAfter`) is strict; `src/low/transport.ts` keeps its own `parseRetryAfter`, which uses `Number.parseInt` and so reads `"2 hours"` as a two-second pace. That one serves the Comfy API v2 job/asset surface and feeds `QueueFull.retryAfter` and the SSE reconnect pause — a different contract, whose `Retry-After` this change does not touch. Unifying them would change v2 `submit()`/reconnect behaviour for malformed headers and was deliberately left alone; it is a small, self-contained follow-up. - **Deliberately out of scope, per the plan for this work, and unchanged here:** retrying a `429` + `Retry-After` in `models.run`; changing this SDK's default of retrying a thrown fetch or a bucket-less 5xx under the same key (Python does not); and the cloud end-to-end harness classifier, where a `409` that survives the collect budget is still classified correctly. - **Unexercised artifacts.** No call was made against a live Comfy Router: this environment has no Router credential, and the acceptance shapes are a mid-generation `409` and a ten-minute `504`, neither of which can be provoked without dispatching (and paying for) real partner-model generations — so they are verified against the vendored contract and the stub server rather than in production. The `pnpm check:spec-drift` and `pnpm check:sdk-parity` gates are what keep the stub's fixtures honest against the two upstream sources. Two further items named in the plan for this work could not be read from here: the cloud-side middleware said to raise the in-flight `409`, which lives in a private repo, and a linked spike ticket whose findings comment is not reachable from this sandbox. Both are cited in that plan only as evidence for gates this change reads directly out of `spec/router-openapi.yaml`, which IS in this repo and IS now machine-checked — so nothing rests on the unread copies, but neither was independently confirmed.  ## Summary by CodeRabbit - **New Features** - Added server-directed collection retries for eligible `409` and `504` responses using `Retry-After`. - Collection retries reuse the same idempotency key and have a separate 20-minute budget. - Errors now expose `retryAfter` and `idempotencyKey` metadata. - Router errors include parsed `Retry-After` delays. - **Bug Fixes** - Improved retry handling for concurrency-limit and deadline-exceeded responses. - Preserved retry metadata across timeout, queued, and invalid-response errors. - **Documentation** - Updated model-run retry guidance, deadlines, collection controls, and manual recovery instructions. - Increased the default model-run deadline from 10 to 20 minutes.  --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 22:04:49 +00:00