[PR #78] [MERGED] feat: retry comfy.models.run on transport failures and 5xx, bounded by elapsed time #80

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-typescript-sdk/pull/78
Author: @mattmillerai
Created: 8/24/2026
Status: ✅ Merged
Merged: 8/25/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-8505-client-retry-abort-idempotency

📝 Commits (2)

2e04993 feat: retry comfy.models.run on transport failures and 5xx, bounded by elapsed time
9e64b66 test: bound the deadline-spans-attempts case by elapsed time, not by which terminal error wins

📊 Changes

8 files changed (+760 additions, -59 deletions)

View changed files

📝 CHANGELOG.md (+14 -1)
📝 README.md (+37 -11)
📝 src/sdk/index.ts (+6 -0)
📝 src/sdk/models.test.ts (+241 -2)
📝 src/sdk/models.ts (+127 -44)
➕ src/sdk/retry.test.ts (+120 -0)
➕ src/sdk/retry.ts (+164 -0)
📝 test/support/router-stub-server.ts (+51 -1)

📄 Description

ELI-5

A model call can take minutes, because the server holds the connection open until the picture is finished. That makes two things ordinary rather than exceptional: the connection breaking after the model already ran, and a user wanting to press cancel. This PR handles both — the SDK now retries a broken call, and it retries it in a way that cannot pay for the same generation twice, because every attempt carries the same Idempotency-Key and the server answers the repeat with the original result.

What changed

comfy.models.run gains a retry loop and a documented retry policy. The policy itself (src/sdk/retry.ts) is sans-IO — pure decisions, unit-tested without a socket — and models.ts holds the loop that acts on it.

Same key across attempts. The Idempotency-Key is minted once per run, outside the loop, so the first attempt and every retry send the same one. A fresh run is a fresh key and a new logical call. Both halves are asserted by test.
Only transport failures and 5xx are retried. A 404, 409, 422 or content_policy_violation is the server's answer about the request, not a sign the server is unwell, so resending it buys the same verdict twice. A terminal bucket that arrives under a 5xx (X-Comfy-Error-Type: content_policy_violation) is treated the same way — status alone would read it as transient.
Bounded by total elapsed time, not attempt count. retry.budgetMs (default 2 minutes) is measured against the clock from the first attempt, and a further attempt starts only if its backoff ends inside the budget. On a route whose attempts take minutes, "3 retries" says nothing at all about how long a call can run.
Backoff with jitter. Exponential from retry.baseDelayMs (500ms), capped at retry.maxDelayMs (8s), with equal jitter — half the delay fixed so a struggling server still gets a real pause, half random so clients that failed against one incident do not re-land together.
timeoutMs now spans the whole call rather than restarting per attempt, so retrying cannot make a call outlive the deadline its caller asked for.
Abort. An abort mid-backoff stops the loop instead of sleeping it out and sending one more attempt. The abort still reaches the socket — a new stub-server counter proves the server observes a disconnect rather than a client that merely stopped listening — and it still rejects with the standard AbortError.
retry: false makes a call a single attempt. Defaults are documented in the README with a table.

Acceptance criteria

All eight are met; two were already true before this PR and are now covered by a test rather than by inspection.

Criterion	Where
Same key on all attempts; a new call mints a new one	`models.test.ts` "sends the SAME Idempotency-Key…", "mints a new key for the next call…"
Bounded by total elapsed time, not attempt count	`retry.test.ts` "bounds by total elapsed time, not by an attempt count"; `models.test.ts` "a bigger budget buys more attempts"
Only transport failures and 5xx retried; `422`/`409`/`404`/`content_policy_violation` not	`retry.test.ts` "never retries a verdict about the request itself"; `models.test.ts` "does not retry a verdict about the request…"
Backoff with jitter	`retry.test.ts` "jitters…", "actually varies…"
Abort aborts the underlying connection	`models.test.ts` "aborts the underlying connection, so the server sees a disconnect"
Distinguishable abort error	Already true — a caller's abort is re-thrown untouched as `AbortError`, told apart from a transport `TypeError` and from this SDK's `ComfyError`/`request_timeout`. Now pinned by "rejects with a distinguishable abort, not a generic network error"
Abort mid-retry stops the loop	`models.test.ts` "stops the retry loop mid-backoff…"
Defaults documented; retry can be disabled	README "Retries" table; `models.test.ts` "makes the call a single attempt when retries are disabled"

Judgment calls

Scoped to comfy.models.run. That is the surface that holds a connection through server-side polling, which is what makes retry-with-a-stable-key and cancellation load-bearing here rather than nice-to-have. The class client (new Comfy().run(workflow)) submits and then polls, so each of its requests is short and it already has its own orchestration retries (queue-full backoff with Retry-After, an SSE reconnect loop, an abortable poll backoff) plus signal on every method. Adding a second, transport-level retry underneath those would double-retry the queue-full path. It is not covered here and is worth its own change if wanted.
signal, not abortSignal. The option is already signal across this SDK and fetch itself; renaming it would break a published surface for no behavioral gain.
Retry defaults to on. Off-by-default would leave the documented defaults inert. Disable per call with retry: false.
Retry-After is not honoured. The jittered backoff is used even when a 5xx carries the header. 429 is not retried at all, which is where that header usually lives on this surface.
A 5xx is retried even when it may follow a completed generation. This is deliberate and is the whole reason the key is unconditional — but see the dependency below.

Known limitation, and why it is not fixed here

Retrying a 5xx is only free once the server stores the first response against the Idempotency-Key and replays it for a repeat. That server-side replay is separate, in-flight work and is explicitly out of scope for this change. Until it ships, a retried 5xx may re-run the model rather than replay it, so a call that failed after the generation completed could be billed twice. The client-side half has to land first — the key has to be on every attempt before there is anything for the server to key a replay off — which is why this ships now, but a reviewer should weigh that ordering rather than read "idempotent retry" as already end-to-end. retry: false is the escape hatch in the meantime.

Also unfixed and not attempted: content_policy_violation is refused by status plus the X-Comfy-Error-Type header, never by reading the error body, so a policy verdict delivered under a 5xx with no such header would be retried once per budget.

Unexercised artifacts

The ticket driving this work links four provenance sources (an internal doc, a chat permalink, and two interactive-session notes) and names a parent epic and a paired server-side issue. None of their contents are reachable from this environment, so this change was built against the ticket's own description and acceptance criteria plus the code already on main. The paired server-side work in particular was not read — the limitation above is stated from the ticket's own summary of it, not from that issue's text.

Everything the change itself touches was exercised: the stub router was driven through 5xx sequences, socket resets, a hung request with a real client abort, budget exhaustion, and a deadline that spans attempts.

Test plan

pnpm lint && pnpm format:check && pnpm typecheck && pnpm check:spec-drift && pnpm test && pnpm build
node scripts/check-public-repo-hygiene.mjs

Two pre-existing tests (504 provider_timeout mapping, 502 HTML body) now pass retry: false: they assert error mapping, and a 5xx is now retryable, so left alone they would spend the whole retry budget before raising the error under test. No assertion in them changed.

The second commit fixes a race in one test I wrote: pinning err.code to request_timeout for the deadline-spans-attempts case was wrong, because when the last 503 lands just before the deadline the loop correctly falls through and raises the server's own error instead. Both outcomes prove the deadline stopped the loop; the elapsed time and the attempt count are what prove it was the deadline rather than the 60x-larger retry budget, so those are what the test asserts now.

Provenance

Authored by: agent-work loop
Verified: pnpm test: 278 passed, 0 failed (19 files); pnpm lint, pnpm format:check, pnpm typecheck, pnpm check:spec-drift, pnpm build, node scripts/check-public-repo-hygiene.mjs: all clean locally. CI green on the pushed head: Test (node 22), Test (node 24), build-check, public-repo-hygiene all pass
Deviations: none of the eight acceptance criteria were skipped. Scope was read as comfy.models.run rather than the whole SDK — see Judgment calls. The retry-is-free premise depends on server-side idempotency replay that has not shipped — see Known limitation.

_{🔄 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/78 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/24/2026 **Status:** ✅ Merged **Merged:** 8/25/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-8505-client-retry-abort-idempotency` --- ### 📝 Commits (2) - [`2e04993`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/2e04993180bcd3b637e43272f82ceb065afbe17c) feat: retry comfy.models.run on transport failures and 5xx, bounded by elapsed time - [`9e64b66`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/9e64b66e49fb99b9b83ee9417dcd2b1a87e8d762) test: bound the deadline-spans-attempts case by elapsed time, not by which terminal error wins ### 📊 Changes **8 files changed** (+760 additions, -59 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+14 -1) 📝 `README.md` (+37 -11) 📝 `src/sdk/index.ts` (+6 -0) 📝 `src/sdk/models.test.ts` (+241 -2) 📝 `src/sdk/models.ts` (+127 -44) ➕ `src/sdk/retry.test.ts` (+120 -0) ➕ `src/sdk/retry.ts` (+164 -0) 📝 `test/support/router-stub-server.ts` (+51 -1) </details> ### 📄 Description ## ELI-5 A model call can take minutes, because the server holds the connection open until the picture is finished. That makes two things ordinary rather than exceptional: the connection breaking after the model already ran, and a user wanting to press cancel. This PR handles both — the SDK now retries a broken call, and it retries it in a way that cannot pay for the same generation twice, because every attempt carries the same `Idempotency-Key` and the server answers the repeat with the original result. ## What changed `comfy.models.run` gains a retry loop and a documented retry policy. The policy itself (`src/sdk/retry.ts`) is sans-IO — pure decisions, unit-tested without a socket — and `models.ts` holds the loop that acts on it. - **Same key across attempts.** The `Idempotency-Key` is minted once per `run`, outside the loop, so the first attempt and every retry send the same one. A fresh `run` is a fresh key and a new logical call. Both halves are asserted by test. - **Only transport failures and 5xx are retried.** A `404`, `409`, `422` or `content_policy_violation` is the server's answer about the request, not a sign the server is unwell, so resending it buys the same verdict twice. A terminal bucket that arrives *under* a 5xx (`X-Comfy-Error-Type: content_policy_violation`) is treated the same way — status alone would read it as transient. - **Bounded by total elapsed time, not attempt count.** `retry.budgetMs` (default 2 minutes) is measured against the clock from the first attempt, and a further attempt starts only if its backoff ends inside the budget. On a route whose attempts take minutes, "3 retries" says nothing at all about how long a call can run. - **Backoff with jitter.** Exponential from `retry.baseDelayMs` (500ms), capped at `retry.maxDelayMs` (8s), with equal jitter — half the delay fixed so a struggling server still gets a real pause, half random so clients that failed against one incident do not re-land together. - **`timeoutMs` now spans the whole call** rather than restarting per attempt, so retrying cannot make a call outlive the deadline its caller asked for. - **Abort.** An abort mid-backoff stops the loop instead of sleeping it out and sending one more attempt. The abort still reaches the socket — a new stub-server counter proves the server observes a disconnect rather than a client that merely stopped listening — and it still rejects with the standard `AbortError`. - **`retry: false`** makes a call a single attempt. Defaults are documented in the README with a table. ## Acceptance criteria All eight are met; two were already true before this PR and are now covered by a test rather than by inspection. | Criterion | Where | | --- | --- | | Same key on all attempts; a new call mints a new one | `models.test.ts` "sends the SAME Idempotency-Key…", "mints a new key for the next call…" | | Bounded by total elapsed time, not attempt count | `retry.test.ts` "bounds by total elapsed time, not by an attempt count"; `models.test.ts` "a bigger budget buys more attempts" | | Only transport failures and 5xx retried; `422`/`409`/`404`/`content_policy_violation` not | `retry.test.ts` "never retries a verdict about the request itself"; `models.test.ts` "does not retry a verdict about the request…" | | Backoff with jitter | `retry.test.ts` "jitters…", "actually varies…" | | Abort aborts the underlying connection | `models.test.ts` "aborts the underlying connection, so the server sees a disconnect" | | Distinguishable abort error | Already true — a caller's abort is re-thrown untouched as `AbortError`, told apart from a transport `TypeError` and from this SDK's `ComfyError`/`request_timeout`. Now pinned by "rejects with a distinguishable abort, not a generic network error" | | Abort mid-retry stops the loop | `models.test.ts` "stops the retry loop mid-backoff…" | | Defaults documented; retry can be disabled | README "Retries" table; `models.test.ts` "makes the call a single attempt when retries are disabled" | ## Judgment calls - **Scoped to `comfy.models.run`.** That is the surface that holds a connection through server-side polling, which is what makes retry-with-a-stable-key and cancellation load-bearing here rather than nice-to-have. The class client (`new Comfy().run(workflow)`) submits and then *polls*, so each of its requests is short and it already has its own orchestration retries (queue-full backoff with `Retry-After`, an SSE reconnect loop, an abortable poll backoff) plus `signal` on every method. Adding a second, transport-level retry underneath those would double-retry the queue-full path. It is not covered here and is worth its own change if wanted. - **`signal`, not `abortSignal`.** The option is already `signal` across this SDK and `fetch` itself; renaming it would break a published surface for no behavioral gain. - **Retry defaults to on.** Off-by-default would leave the documented defaults inert. Disable per call with `retry: false`. - **`Retry-After` is not honoured.** The jittered backoff is used even when a 5xx carries the header. `429` is not retried at all, which is where that header usually lives on this surface. - **A 5xx is retried even when it may follow a completed generation.** This is deliberate and is the whole reason the key is unconditional — but see the dependency below. ## Known limitation, and why it is not fixed here Retrying a 5xx is only *free* once the server stores the first response against the `Idempotency-Key` and replays it for a repeat. That server-side replay is separate, in-flight work and is explicitly out of scope for this change. **Until it ships, a retried 5xx may re-run the model rather than replay it**, so a call that failed after the generation completed could be billed twice. The client-side half has to land first — the key has to be on every attempt before there is anything for the server to key a replay off — which is why this ships now, but a reviewer should weigh that ordering rather than read "idempotent retry" as already end-to-end. `retry: false` is the escape hatch in the meantime. Also unfixed and not attempted: `content_policy_violation` is refused by *status* plus the `X-Comfy-Error-Type` header, never by reading the error body, so a policy verdict delivered under a 5xx with no such header would be retried once per budget. ## Unexercised artifacts The ticket driving this work links four provenance sources (an internal doc, a chat permalink, and two interactive-session notes) and names a parent epic and a paired server-side issue. None of their contents are reachable from this environment, so this change was built against the ticket's own description and acceptance criteria plus the code already on `main`. The paired server-side work in particular was not read — the limitation above is stated from the ticket's own summary of it, not from that issue's text. Everything the change itself touches was exercised: the stub router was driven through 5xx sequences, socket resets, a hung request with a real client abort, budget exhaustion, and a deadline that spans attempts. ## Test plan ``` pnpm lint && pnpm format:check && pnpm typecheck && pnpm check:spec-drift && pnpm test && pnpm build node scripts/check-public-repo-hygiene.mjs ``` Two pre-existing tests (`504 provider_timeout` mapping, `502` HTML body) now pass `retry: false`: they assert error *mapping*, and a 5xx is now retryable, so left alone they would spend the whole retry budget before raising the error under test. No assertion in them changed. The second commit fixes a race in one test I wrote: pinning `err.code` to `request_timeout` for the deadline-spans-attempts case was wrong, because when the last 503 lands just before the deadline the loop correctly falls through and raises the server's own error instead. Both outcomes prove the deadline stopped the loop; the elapsed time and the attempt count are what prove it was the deadline rather than the 60x-larger retry budget, so those are what the test asserts now. ## Provenance - **Authored by:** agent-work loop - **Verified:** `pnpm test`: 278 passed, 0 failed (19 files); `pnpm lint`, `pnpm format:check`, `pnpm typecheck`, `pnpm check:spec-drift`, `pnpm build`, `node scripts/check-public-repo-hygiene.mjs`: all clean locally. CI green on the pushed head: Test (node 22), Test (node 24), build-check, public-repo-hygiene all pass - **Deviations:** none of the eight acceptance criteria were skipped. Scope was read as `comfy.models.run` rather than the whole SDK — see Judgment calls. The retry-is-free premise depends on server-side idempotency replay that has not shipped — see Known limitation. --- <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:22 +00:00