[PR #133] [MERGED] feat(models): queue a model request with submit, subscribe and handle #135

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-13232-models-queue-submit-subscribe-handle

📝 Commits (5)

7d087e7 feat(models): queue a model request with submit, subscribe and handle
50c7026 Merge remote-tracking branch 'origin/main' into matt/be-13232-models-queue-submit-subscribe-handle
eb41e54 Merge branch 'main' into matt/be-13232-models-queue-submit-subscribe-handle
ebf1d2b fix: address the CodeRabbit review on the queued model surface
7e37ed8 docs: exclude onQueueUpdate callback time from subscribe's timeout contract

📊 Changes

15 files changed (+5827 additions, -98 deletions)

View changed files

📝 AGENTS.md (+29 -9)
📝 CHANGELOG.md (+26 -0)
📝 README.md (+68 -1)
➕ package-lock.json (+2993 -0)
📝 src/sdk/credentials.ts (+24 -0)
📝 src/sdk/index.ts (+14 -0)
➕ src/sdk/modelRequests.test.ts (+935 -0)
➕ src/sdk/modelRequests.ts (+1067 -0)
➕ src/sdk/modelRoutes.ts (+173 -0)
📝 src/sdk/models.ts (+131 -84)
📝 src/sdk/router-spec-contract.test.ts (+76 -1)
📝 src/sdk/routerErrors.test.ts (+92 -0)
📝 src/sdk/routerErrors.ts (+64 -1)
📝 src/sdk/surface-parity.test.ts (+68 -2)
📝 test/support/router-stub-server.ts (+67 -0)

📄 Description

ELI-5

comfy.models.run waits on the phone until your picture is finished. That is fine until you cannot stay on the phone — a web request that has to answer now, or a worker that starts the job in one place and picks it up somewhere else. This adds the other way of doing it: comfy.models.submit hands you a ticket the moment the server takes the job, and you come back for the picture later with that ticket. comfy.models.subscribe does both halves for you and tells you where you are in the queue while you wait, and comfy.models.handle rebuilds the ticket from just its number, so another process can collect a job it never started.

What this adds

comfy.models.submit(model, input, options?) — POSTs the model's own native body to a requests collection under the same /v2/models/{provider}/{model} prefix run uses, and resolves to a RequestHandle as soon as the server accepts. The handle carries requestId, model, status(), get(options?), cancel() and an async-iterable events(options?) — the TypeScript spelling of the Python SDK's iter_events. get() resolves to the same { data, requestId } run returns.
comfy.models.subscribe(model, input, { onQueueUpdate?, timeoutMs?, signal?, idempotencyKey?, retry? }) — submit + poll + collect in one call.
comfy.models.handle(model, requestId) — rebuilds a handle from the two ids with no request made; both are validated locally.
routerErrors.errorFromCompletion(body, requestId) — the typed exception a completed queued request reports, or null.

The server owns the queue. Ordering, admission, retries, timeouts, billing and expiry are all decided server-side; this adds polling and ergonomics and nothing else.

Behaviour worth reviewing

A 200 is not a success. The server reports a failed and a cancelled request as COMPLETED carrying an error_type. Every path that reads a completion runs it through errorFromCompletion, which maps the bucket through the same table toRouterError uses and leaves httpStatus null (the poll that found it was a 200). It runs unconditionally on the authoritative status read, and on the result body only inside the queue's own envelope — a partner model whose native output happens to have an error_type field must not have a successful generation failed for it.
Bounds cover the whole wait. timeoutMs/signal bound the poll requests themselves, their retries, the pauses between them and the result fetch — not only the pauses. The first poll is always made, so timeoutMs: 0 reads "look once". Each individual request is floored at 1 s (MIN_REQUEST_TIMEOUT_MS) so the last poll before a deadline can still complete a handshake rather than being a certain transport failure; the loop's own bound is unfloored, so a spent budget still stops the next request and the wait overruns by at most one. This mirrors the Python SDK's _MIN_HTTP_TIMEOUT.
Pacing. Adaptive backoff, with a server Retry-After beating the schedule and capped at 60 s before it is slept on. QueueUpdate.retryAfterMs reports it uncapped — the cap applies to what is slept on, not to what the server is reported as having said.
Cleanup cancel. subscribe's own timeout and an abort via signal each issue exactly one best-effort cancel: a single attempt (retry: false), a 10 s bound, and not the caller's already-aborted signal. Its failure is swallowed so it cannot mask the timeout or abort — asserted, including the request count. A failure in the caller's own onQueueUpdate deliberately does not cancel: a bug in a progress renderer is not a reason to throw away a paid-for generation.
Idempotency. Each submit call mints one fresh key; every retry inside that call reuses it, so a retry replays the original acceptance rather than queueing a second generation. idempotencyKey overrides. Cancel is sent as PUT.

Judgment calls

The queued surface raises routerErrors.*, not the ComfyError family run maps into. That is an asymmetry inside this SDK, and it is deliberate: a queue failure arrives as an error_type in a 200 body where there is no status to classify, so the class keyed by the bucket is the only honest answer — and it is what the Python SDK does. run's own mapping is untouched. Flagging it because a reviewer will reasonably ask.
The four queue routes are not in spec/router-openapi.yaml. The operations are authored upstream but held, and the one-way sync strips a held operation, so there is nothing to compare them against. They are pinned by their relation to RUN_ROUTE_TEMPLATE (each is the run route plus a fixed suffix), which is how the Python SDK binds the same four — plus a rot guard that fails the day the vendored contract does declare the collection, naming the replacement to make.
parseModelId/fillRoute moved to a new leaf src/sdk/modelRoutes.ts alongside the new parseRequestId. models.ts imports the queue methods to assemble the frozen namespace; leaving the helpers where they were would have made the queue module import it back, and that runtime cycle is safe only until someone adds a const read at module-evaluation time. The built output confirms the dependency runs one way (dist/sdk/modelRequests.js has no models.js import — RunResult is a type-only import and is erased). RUN_ROUTE_TEMPLATE stays in models.ts because scripts/router-route-contract.mjs reads it out of that file's source text.
The credential gate moved to credentials.ts as requireCredentials() so both surfaces cannot drift into two messages for the same misconfiguration. run's behaviour is byte-identical: same class, same message, same code.
Cross-SDK parity is recorded as a LEAD, not an asymmetry — see Residual.
Negative-claim check: this diff denies no capability. It adds three methods and no dead-end path; the only throws are the local id/input validation the change was asked for (parseRequestId, the non-object-body guards), and NotEnabled is the server's gate surfaced through the existing typed hierarchy, not a client-side refusal. No not supported / unavailable / STOP string is introduced.

Residual

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

The cross-SDK parity snapshot could not be refreshed from a merged Python change, so it is unmet as originally specified. The Python SDK's queued surface is still an open pull request (Comfy-Org/comfy-python-sdk#137, verified open with mergedAt: null while writing this), so pnpm sync:python-surface against that repo's default branch still yields models methods of ["run"] alone — and pnpm check:sdk-parity confirms the committed snapshot already matches it. Adding submit/subscribe/handle therefore makes this SDK lead. Rather than allowlist that as an intentional asymmetry (which it is not — the two surfaces are the same shape, method for method), it is recorded with a new modelsMethodsAheadOfPython field in surface-parity.test.ts: the same one-direction LEAD mechanism routerErrorClassesAheadOfPython already provides, with a rot guard that fails the moment the refreshed snapshot grows any of the three names. The follow-up work is mechanical and must happen once the Python change lands on its default branch: run pnpm sync:python-surface, commit the refreshed parity/python-surface.json, and delete the models-queue-lead entry the rot guard then names. Until that is done, the three method names are not actually being diffed against Python.
Comfy-Org/docs#1648's TypeScript tab is not updated by this PR. The docs preview page (development/comfy-router/queue.mdx) still shows raw fetch calls against the four routes with a comment saying the SDK methods land next. That is a change in a different repository and is not in this diff; it becomes actionable once this is published under a pre-release tag. I read the PR's title only — its content is in a repo this change does not touch, so I did not exercise it.
Nothing here has been exercised against a live server, by design and by constraint. Every test runs against test/support/router-stub-server.ts (a real Node http stub, extended with a respond hook so one call sequence can script submit → status → result → cancel), which is what the repo's test style requires. The four queue routes are served only on the queue POC stack today, which this environment cannot reach, so the wire contract is asserted against a recording of it and not against the server itself. In particular the 403 not_enabled gate, the 400 ALREADY_COMPLETED cancel answer and the result route's 202 are all stubbed rather than observed.
The npm pre-release publish and the decision to promote it are the human steps and are not part of this PR. Publishing before the production server serves these routes is what the pre-release tag is for.
Two server-contract questions remain open upstream (cancel-after-complete as 400 vs 409; what the result read gives a cancelled or timed-out request). Neither changes this client surface, but both mean the cancel and result paths here are written against the shape as it stands today: cancel() surfaces whatever the server answers through the typed hierarchy rather than special-casing either status, and the result route's 202 is reported as an invalid response rather than silently retried.
The parent epic was named to me but not readable, so this PR is scoped to what the change description itself specifies; if the epic carries constraints beyond that, they are not reflected here.

Provenance

Authored by: agent-work loop
Verified: pnpm test: 25 files, 495 passed, 0 failed (50 new in src/sdk/modelRequests.test.ts, 7 new in src/sdk/routerErrors.test.ts, 3 new in src/sdk/router-spec-contract.test.ts, 1 new in src/sdk/surface-parity.test.ts); pnpm typecheck: clean; pnpm lint: clean; pnpm format:check: clean; pnpm check:spec-drift: both checks OK; pnpm check:sdk-parity: snapshot matches the Python default branch; node scripts/check-public-repo-hygiene.mjs: no internal-only references; pnpm build && pnpm pack: tarball carries package/dist/index.js and package/dist/index.d.ts.
Deviations: the parity snapshot could not be refreshed from a merged Python change because that change has not merged — recorded as a declared LEAD with a rot guard instead of as an intentional asymmetry, and written up under Residual above.

Summary by CodeRabbit

New Features
- Added queued model execution with submit, subscribe, and request handles for polling, status updates, cancellation, timeouts, and result collection.
- Added support for rehydrating requests and subscribing to queue events.
- Added credential validation with clear setup guidance when credentials are missing.
- Added typed errors for failures reported after queued requests complete.
Documentation
- Documented synchronous and queued model execution, request lifecycle options, cancellation, retries, timeouts, idempotency, and error handling.
- Updated the changelog for the new queued model APIs.

_{🔄 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/133 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/10/2026 **Status:** ✅ Merged **Merged:** 9/14/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-13232-models-queue-submit-subscribe-handle` --- ### 📝 Commits (5) - [`7d087e7`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/7d087e78a75763e2f60bbde583deb58c1ba436fb) feat(models): queue a model request with submit, subscribe and handle - [`50c7026`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/50c7026e3fd7ab767996fe1eebd4173c0259d868) Merge remote-tracking branch 'origin/main' into matt/be-13232-models-queue-submit-subscribe-handle - [`eb41e54`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/eb41e5472ab4152aef2817da98151476b75a3014) Merge branch 'main' into matt/be-13232-models-queue-submit-subscribe-handle - [`ebf1d2b`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/ebf1d2b95a93758980e6860b0081a7f76863b01e) fix: address the CodeRabbit review on the queued model surface - [`7e37ed8`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/7e37ed84a3484953489cdde331a983ca7350eb40) docs: exclude onQueueUpdate callback time from subscribe's timeout contract ### 📊 Changes **15 files changed** (+5827 additions, -98 deletions) <details> <summary>View changed files</summary> 📝 `AGENTS.md` (+29 -9) 📝 `CHANGELOG.md` (+26 -0) 📝 `README.md` (+68 -1) ➕ `package-lock.json` (+2993 -0) 📝 `src/sdk/credentials.ts` (+24 -0) 📝 `src/sdk/index.ts` (+14 -0) ➕ `src/sdk/modelRequests.test.ts` (+935 -0) ➕ `src/sdk/modelRequests.ts` (+1067 -0) ➕ `src/sdk/modelRoutes.ts` (+173 -0) 📝 `src/sdk/models.ts` (+131 -84) 📝 `src/sdk/router-spec-contract.test.ts` (+76 -1) 📝 `src/sdk/routerErrors.test.ts` (+92 -0) 📝 `src/sdk/routerErrors.ts` (+64 -1) 📝 `src/sdk/surface-parity.test.ts` (+68 -2) 📝 `test/support/router-stub-server.ts` (+67 -0) </details> ### 📄 Description ## ELI-5 `comfy.models.run` waits on the phone until your picture is finished. That is fine until you cannot stay on the phone — a web request that has to answer now, or a worker that starts the job in one place and picks it up somewhere else. This adds the other way of doing it: `comfy.models.submit` hands you a ticket the moment the server takes the job, and you come back for the picture later with that ticket. `comfy.models.subscribe` does both halves for you and tells you where you are in the queue while you wait, and `comfy.models.handle` rebuilds the ticket from just its number, so another process can collect a job it never started. ## What this adds - **`comfy.models.submit(model, input, options?)`** — POSTs the model's own native body to a `requests` collection under the same `/v2/models/{provider}/{model}` prefix `run` uses, and resolves to a `RequestHandle` as soon as the server accepts. The handle carries `requestId`, `model`, `status()`, `get(options?)`, `cancel()` and an async-iterable `events(options?)` — the TypeScript spelling of the Python SDK's `iter_events`. `get()` resolves to the same `{ data, requestId }` `run` returns. - **`comfy.models.subscribe(model, input, { onQueueUpdate?, timeoutMs?, signal?, idempotencyKey?, retry? })`** — submit + poll + collect in one call. - **`comfy.models.handle(model, requestId)`** — rebuilds a handle from the two ids with **no request made**; both are validated locally. - **`routerErrors.errorFromCompletion(body, requestId)`** — the typed exception a completed queued request reports, or `null`. The server owns the queue. Ordering, admission, retries, timeouts, billing and expiry are all decided server-side; this adds polling and ergonomics and nothing else. ## Behaviour worth reviewing - **A `200` is not a success.** The server reports a failed *and* a cancelled request as `COMPLETED` carrying an `error_type`. Every path that reads a completion runs it through `errorFromCompletion`, which maps the bucket through the same table `toRouterError` uses and leaves `httpStatus` null (the poll that found it *was* a `200`). It runs unconditionally on the authoritative status read, and on the result body **only inside the queue's own envelope** — a partner model whose native output happens to have an `error_type` field must not have a successful generation failed for it. - **Bounds cover the whole wait.** `timeoutMs`/`signal` bound the poll requests themselves, their retries, the pauses between them and the result fetch — not only the pauses. The first poll is always made, so `timeoutMs: 0` reads "look once". Each individual request is floored at 1 s (`MIN_REQUEST_TIMEOUT_MS`) so the last poll before a deadline can still complete a handshake rather than being a certain transport failure; the loop's own bound is unfloored, so a spent budget still stops the *next* request and the wait overruns by at most one. This mirrors the Python SDK's `_MIN_HTTP_TIMEOUT`. - **Pacing.** Adaptive backoff, with a server `Retry-After` beating the schedule and capped at 60 s before it is slept on. `QueueUpdate.retryAfterMs` reports it **uncapped** — the cap applies to what is slept on, not to what the server is reported as having said. - **Cleanup cancel.** `subscribe`'s own timeout and an abort via `signal` each issue exactly one best-effort cancel: a single attempt (`retry: false`), a 10 s bound, and **not** the caller's already-aborted signal. Its failure is swallowed so it cannot mask the timeout or abort — asserted, including the request count. A failure in the caller's own `onQueueUpdate` deliberately does **not** cancel: a bug in a progress renderer is not a reason to throw away a paid-for generation. - **Idempotency.** Each `submit` **call** mints one fresh key; every retry inside that call reuses it, so a retry replays the original acceptance rather than queueing a second generation. `idempotencyKey` overrides. Cancel is sent as `PUT`. ## Judgment calls 1. **The queued surface raises `routerErrors.*`, not the `ComfyError` family `run` maps into.** That is an asymmetry *inside* this SDK, and it is deliberate: a queue failure arrives as an `error_type` in a `200` body where there is no status to classify, so the class keyed by the bucket is the only honest answer — and it is what the Python SDK does. `run`'s own mapping is untouched. Flagging it because a reviewer will reasonably ask. 2. **The four queue routes are not in `spec/router-openapi.yaml`.** The operations are authored upstream but held, and the one-way sync strips a held operation, so there is nothing to compare them against. They are pinned by their **relation** to `RUN_ROUTE_TEMPLATE` (each is the run route plus a fixed suffix), which is how the Python SDK binds the same four — plus a **rot guard** that fails the day the vendored contract *does* declare the collection, naming the replacement to make. 3. **`parseModelId`/`fillRoute` moved to a new leaf `src/sdk/modelRoutes.ts`** alongside the new `parseRequestId`. `models.ts` imports the queue methods to assemble the frozen namespace; leaving the helpers where they were would have made the queue module import it back, and that runtime cycle is safe only until someone adds a `const` read at module-evaluation time. The built output confirms the dependency runs one way (`dist/sdk/modelRequests.js` has no `models.js` import — `RunResult` is a type-only import and is erased). `RUN_ROUTE_TEMPLATE` stays in `models.ts` because `scripts/router-route-contract.mjs` reads it out of that file's source text. 4. **The credential gate moved to `credentials.ts` as `requireCredentials()`** so both surfaces cannot drift into two messages for the same misconfiguration. `run`'s behaviour is byte-identical: same class, same message, same code. 5. **Cross-SDK parity is recorded as a LEAD, not an asymmetry** — see Residual. 6. **Negative-claim check:** this diff denies no capability. It adds three methods and no dead-end path; the only throws are the local id/input validation the change was asked for (`parseRequestId`, the non-object-body guards), and `NotEnabled` is the *server's* gate surfaced through the existing typed hierarchy, not a client-side refusal. No `not supported` / `unavailable` / `STOP` string is introduced. ## Residual Not fixed here, and worth its own follow-up: - **The cross-SDK parity snapshot could not be refreshed from a merged Python change, so it is unmet as originally specified.** The Python SDK's queued surface is still an **open** pull request (`Comfy-Org/comfy-python-sdk#137`, verified open with `mergedAt: null` while writing this), so `pnpm sync:python-surface` against that repo's default branch still yields `models` methods of `["run"]` alone — and `pnpm check:sdk-parity` confirms the committed snapshot already matches it. Adding `submit`/`subscribe`/`handle` therefore makes this SDK **lead**. Rather than allowlist that as an intentional asymmetry (which it is not — the two surfaces are the same shape, method for method), it is recorded with a new `modelsMethodsAheadOfPython` field in `surface-parity.test.ts`: the same one-direction LEAD mechanism `routerErrorClassesAheadOfPython` already provides, with a rot guard that **fails** the moment the refreshed snapshot grows any of the three names. **The follow-up work is mechanical and must happen once the Python change lands on its default branch:** run `pnpm sync:python-surface`, commit the refreshed `parity/python-surface.json`, and delete the `models-queue-lead` entry the rot guard then names. Until that is done, the three method names are not actually being diffed against Python. - **`Comfy-Org/docs#1648`'s TypeScript tab is not updated by this PR.** The docs preview page (`development/comfy-router/queue.mdx`) still shows raw `fetch` calls against the four routes with a comment saying the SDK methods land next. That is a change in a different repository and is not in this diff; it becomes actionable once this is published under a pre-release tag. I read the PR's *title* only — its content is in a repo this change does not touch, so I did not exercise it. - **Nothing here has been exercised against a live server, by design and by constraint.** Every test runs against `test/support/router-stub-server.ts` (a real Node `http` stub, extended with a `respond` hook so one call sequence can script submit → status → result → cancel), which is what the repo's test style requires. The four queue routes are served only on the queue POC stack today, which this environment cannot reach, so the wire contract is asserted against a recording of it and not against the server itself. In particular the `403 not_enabled` gate, the `400 ALREADY_COMPLETED` cancel answer and the result route's `202` are all stubbed rather than observed. - **The npm pre-release publish and the decision to promote it are the human steps** and are not part of this PR. Publishing before the production server serves these routes is what the pre-release tag is for. - **Two server-contract questions remain open upstream** (cancel-after-complete as `400` vs `409`; what the result read gives a cancelled or timed-out request). Neither changes this client surface, but both mean the cancel and result paths here are written against the shape as it stands today: `cancel()` surfaces whatever the server answers through the typed hierarchy rather than special-casing either status, and the result route's `202` is reported as an invalid response rather than silently retried. - **The parent epic was named to me but not readable**, so this PR is scoped to what the change description itself specifies; if the epic carries constraints beyond that, they are not reflected here. ## Provenance - **Authored by:** agent-work loop - **Verified:** `pnpm test`: 25 files, 495 passed, 0 failed (50 new in `src/sdk/modelRequests.test.ts`, 7 new in `src/sdk/routerErrors.test.ts`, 3 new in `src/sdk/router-spec-contract.test.ts`, 1 new in `src/sdk/surface-parity.test.ts`); `pnpm typecheck`: clean; `pnpm lint`: clean; `pnpm format:check`: clean; `pnpm check:spec-drift`: both checks OK; `pnpm check:sdk-parity`: snapshot matches the Python default branch; `node scripts/check-public-repo-hygiene.mjs`: no internal-only references; `pnpm build && pnpm pack`: tarball carries `package/dist/index.js` and `package/dist/index.d.ts`. - **Deviations:** the parity snapshot could not be refreshed from a merged Python change because that change has not merged — recorded as a declared LEAD with a rot guard instead of as an intentional asymmetry, and written up under Residual above.  ## Summary by CodeRabbit * **New Features** * Added queued model execution with `submit`, `subscribe`, and request handles for polling, status updates, cancellation, timeouts, and result collection. * Added support for rehydrating requests and subscribing to queue events. * Added credential validation with clear setup guidance when credentials are missing. * Added typed errors for failures reported after queued requests complete. * **Documentation** * Documented synchronous and queued model execution, request lifecycle options, cancellation, retries, timeouts, idempotency, and error handling. * Updated the changelog for the new queued model APIs.  --- <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:53 +00:00