[PR #204] fix(jobs): end job.events() with a typed error on the terminal SSE error frame #203

New issue

Open

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-typescript-sdk/pull/204
Author: @mattmillerai
Created: 10/9/2026
Status: 🔄 Open

Base: main ← Head: mattmiller/be-19941-sse-error-frame

📝 Commits (3)

d3d2a74 fix(jobs): end job.events() with a typed error on the terminal SSE error frame
642dc54 fix(jobs): harden the SSE error-frame path against prototype codes and abort races
2229088 fix(jobs): keep the SSE error frame through a rejected iterator close, and keep its reason

📊 Changes

12 files changed (+334 additions, -5 deletions)

View changed files

📝 CHANGELOG.md (+3 -0)
📝 src/low/errors.test.ts (+85 -0)
📝 src/low/errors.ts (+75 -1)
📝 src/low/spec-coverage.test.ts (+26 -0)
📝 src/low/sse.test.ts (+5 -0)
📝 src/low/sse.ts (+3 -1)
📝 src/sdk/events.ts (+1 -0)
📝 src/sdk/exceptions.test.ts (+10 -0)
📝 src/sdk/exceptions.ts (+5 -1)
📝 src/sdk/jobs.test.ts (+88 -1)
📝 src/sdk/jobs.ts (+21 -1)
📝 test/support/stub-server.ts (+12 -0)

📄 Description

Summary

GET /api/v2/jobs/{id}/events now ends a stream with event: error plus an error envelope (credential_expired, forbidden, job_not_found) when the server's periodic re-poll permanently fails. eventFromRaw had no case for that frame, so Job.events() dropped it, never saw a terminal status, and ran refresh() → 100 ms pause → reconnect. For job_not_found, while GET /jobs/{id} still answers non-terminal, that loop never ends and keeps using the caller's concurrent-stream quota. In all three cases the caller never saw the server's explanation.

Change

src/sdk/jobs.ts: inside the for await frame loop of Job.events(), and before eventFromRaw, an error frame is turned into a typed error via sseErrorFromFrame(raw.data) and the loop breaks. The error is thrown as toSdkError(...) after the try, so the reconnect dispatch (501/429 handling, and the abort rethrow that would leak the untranslated low error) never sees it. The catch also checks for the frame first, because break closes the iterator inside the try, and a rejection from that close (an already-errored body) would otherwise land there and drop or replace the frame. There is no refresh, no pause and no reconnect. Frames that arrived before the error frame are still yielded. The JSDoc now names the three rejections.
src/low/errors.ts: adds the exported sseErrorFromFrame(data). It maps unauthorized/credential_expired → 401, forbidden → 403, and not_found/job_not_found/asset_not_found → 404, then calls errorFromEnvelope, keeping the code verbatim. An unknown code gives a bare ApiError with httpStatus: 0, which still ends the iterator. A frame with no envelope message keeps what the server did say: a string error, a non-JSON line, a non-object JSON value, or the payload itself. If there is none, it names the frame rather than reporting HTTP 0. That reason has its line breaks flattened and is capped at 500 characters before it goes into Error.message. credential_expired is added to BY_CODE as Unauthorized. Code lookups here and in errorFromEnvelope check for an own property (Object.hasOwn), so a server code like constructor can't resolve through Object.prototype.
src/low/sse.ts: iterateSse no longer drops an error frame whose data: line is empty (other empty frames are still skipped). A bare event: error with no data: line at all is never dispatched by eventsource-parser, as the SSE spec requires, so that shape still falls through to the poll-and-reconnect backstop.
src/sdk/exceptions.ts: credential_expired → Unauthorized, so toSdkError keeps the type. Its code lookup also checks for an own property now.
src/low/spec-coverage.test.ts: a rot guard that fails once the vendored spec's getJobEvents x-sse-events declares error. That is the signal to check the frame's declared shape and replace the guard with a real comparison.
src/sdk/events.ts: adds a comment noting that error is handled upstream and is not a ComfyEvent kind.
test/support/stub-server.ts: adds sseErrorFrameCode state. When it is set, the stub sends a running status frame, then the error frame, then closes.
CHANGELOG: adds an Unreleased → Fixed entry.

Judgment calls

The ticket's test plan names server.state.pollCount. The stub's counter is actually called jobPollCount, so the tests use that.
The SDK test sets pollsToSucceed = 1_000_000, so the poll backstop never reports terminal. That reproduces the unbounded job_not_found cycle. It also asserts abortableSleep was never called (no reconnect pause), on top of the ticket's eventsConnectCount === 1 and unchanged poll count.
Negative-claim check: the new throw path does not deny a capability. It passes on a failure the server has already declared, where it has closed the stream itself. Before this change the same situation ended in a silent reconnect loop rather than success.

Residual

Python SDK twin not changed. The Python SDK's Job.events() likely has the same drop-and-reconnect behaviour for the terminal error frame. This PR only fixes the TypeScript SDK. The Python twin needs the same change (an SSE error-frame → typed error helper, plus credential_expired → Unauthorized in both code tables). surface-parity.test.ts does not flag it, because it does not cover the low-layer error helpers.
The vendored spec does not declare the frame yet. The error event's entry in spec/openapi.yaml arrives with the pending v2 spec sync, which is a separate open PR. This change deliberately does not depend on it, because the server already emits the frame. The rot guard in src/low/spec-coverage.test.ts fails when that sync lands, which is the cue to pin the declared codes against SSE_STATUS_BY_CODE.
Not tested against the live server. The server-side emitter (the public-api events handler's re-poll → error path) is not public, and I didn't run it. The behaviour is tested only against the stub server's copy of the wire format named in the change request: event: error with an {error: {code, message}} envelope.
Overlap with the open PR for the 429 reconnect-pause floor. That PR edits the catch block of Job.events() and the stub's 429 state. This PR edits the frame loop and adds a separate stub field, so the two should rebase cleanly. Whichever lands second should re-run src/sdk/jobs.test.ts.

Provenance

Authored by: agent-work loop
Verified:
- Tests: pnpm test 818 passed / 5 skipped. This includes prototype-key, generic-code, message-fallback and sanitizing cases, an empty-data error frame, and a rejected iterator close (that case fails without the catch re-check)
- Test files type-checked with a temporary tsconfig that includes them (tsc --noEmit excludes tests)
- Lint/fmt/typecheck: pnpm lint, pnpm format:check, pnpm typecheck clean
- Drift: pnpm check:spec-drift OK (generated + Router route)
- Build: pnpm build OK
- Merged with current main locally: lint, format, typecheck, spec-drift and pnpm test (822 passed / 5 skipped) all clean
- CI sdk-parity is red on a shared drift, not on this branch: the Python SDK added QueueBacklogFull, which open PR #209 refreshes the snapshot for
Deviations: the ticket's test plan names pollCount; the stub's counter is jobPollCount. All other acceptance criteria are met as written.

🤖 Generated with Claude Code

_{🔄 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/204 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 10/9/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `mattmiller/be-19941-sse-error-frame` --- ### 📝 Commits (3) - [`d3d2a74`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/d3d2a7406a45d73b752d94f0a04e6f5f00be70e3) fix(jobs): end job.events() with a typed error on the terminal SSE error frame - [`642dc54`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/642dc54827fe668df5bcc5a9532b1ae5c27739f0) fix(jobs): harden the SSE error-frame path against prototype codes and abort races - [`2229088`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/22290881d355b9735914d8147ba724793292d992) fix(jobs): keep the SSE error frame through a rejected iterator close, and keep its reason ### 📊 Changes **12 files changed** (+334 additions, -5 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+3 -0) 📝 `src/low/errors.test.ts` (+85 -0) 📝 `src/low/errors.ts` (+75 -1) 📝 `src/low/spec-coverage.test.ts` (+26 -0) 📝 `src/low/sse.test.ts` (+5 -0) 📝 `src/low/sse.ts` (+3 -1) 📝 `src/sdk/events.ts` (+1 -0) 📝 `src/sdk/exceptions.test.ts` (+10 -0) 📝 `src/sdk/exceptions.ts` (+5 -1) 📝 `src/sdk/jobs.test.ts` (+88 -1) 📝 `src/sdk/jobs.ts` (+21 -1) 📝 `test/support/stub-server.ts` (+12 -0) </details> ### 📄 Description ## Summary `GET /api/v2/jobs/{id}/events` now ends a stream with `event: error` plus an error envelope (`credential_expired`, `forbidden`, `job_not_found`) when the server's periodic re-poll permanently fails. `eventFromRaw` had no case for that frame, so `Job.events()` dropped it, never saw a terminal status, and ran `refresh()` → 100 ms pause → reconnect. For `job_not_found`, while `GET /jobs/{id}` still answers non-terminal, that loop never ends and keeps using the caller's concurrent-stream quota. In all three cases the caller never saw the server's explanation. ## Change - **`src/sdk/jobs.ts`**: inside the `for await` frame loop of `Job.events()`, and before `eventFromRaw`, an `error` frame is turned into a typed error via `sseErrorFromFrame(raw.data)` and the loop breaks. The error is thrown as `toSdkError(...)` *after* the `try`, so the reconnect dispatch (501/429 handling, and the abort rethrow that would leak the untranslated low error) never sees it. The `catch` also checks for the frame first, because `break` closes the iterator inside the `try`, and a rejection from that close (an already-errored body) would otherwise land there and drop or replace the frame. There is no refresh, no pause and no reconnect. Frames that arrived before the `error` frame are still yielded. The JSDoc now names the three rejections. - **`src/low/errors.ts`**: adds the exported `sseErrorFromFrame(data)`. It maps `unauthorized`/`credential_expired` → 401, `forbidden` → 403, and `not_found`/`job_not_found`/`asset_not_found` → 404, then calls `errorFromEnvelope`, keeping the `code` verbatim. An unknown code gives a bare `ApiError` with `httpStatus: 0`, which still ends the iterator. A frame with no envelope message keeps what the server did say: a string `error`, a non-JSON line, a non-object JSON value, or the payload itself. If there is none, it names the frame rather than reporting `HTTP 0`. That reason has its line breaks flattened and is capped at 500 characters before it goes into `Error.message`. `credential_expired` is added to `BY_CODE` as `Unauthorized`. Code lookups here and in `errorFromEnvelope` check for an own property (`Object.hasOwn`), so a server code like `constructor` can't resolve through `Object.prototype`. - **`src/low/sse.ts`**: `iterateSse` no longer drops an `error` frame whose `data:` line is empty (other empty frames are still skipped). A bare `event: error` with no `data:` line at all is never dispatched by `eventsource-parser`, as the SSE spec requires, so that shape still falls through to the poll-and-reconnect backstop. - **`src/sdk/exceptions.ts`**: `credential_expired` → `Unauthorized`, so `toSdkError` keeps the type. Its code lookup also checks for an own property now. - **`src/low/spec-coverage.test.ts`**: a rot guard that fails once the vendored spec's `getJobEvents` `x-sse-events` declares `error`. That is the signal to check the frame's declared shape and replace the guard with a real comparison. - **`src/sdk/events.ts`**: adds a comment noting that `error` is handled upstream and is not a `ComfyEvent` kind. - **`test/support/stub-server.ts`**: adds `sseErrorFrameCode` state. When it is set, the stub sends a `running` status frame, then the `error` frame, then closes. - **CHANGELOG**: adds an Unreleased → Fixed entry. ## Judgment calls - The ticket's test plan names `server.state.pollCount`. The stub's counter is actually called `jobPollCount`, so the tests use that. - The SDK test sets `pollsToSucceed = 1_000_000`, so the poll backstop never reports terminal. That reproduces the unbounded `job_not_found` cycle. It also asserts `abortableSleep` was never called (no reconnect pause), on top of the ticket's `eventsConnectCount === 1` and unchanged poll count. - Negative-claim check: the new throw path does not deny a capability. It passes on a failure the server has already declared, where it has closed the stream itself. Before this change the same situation ended in a silent reconnect loop rather than success. ## Residual - **Python SDK twin not changed.** The Python SDK's `Job.events()` likely has the same drop-and-reconnect behaviour for the terminal `error` frame. This PR only fixes the TypeScript SDK. The Python twin needs the same change (an SSE error-frame → typed error helper, plus `credential_expired` → `Unauthorized` in both code tables). `surface-parity.test.ts` does not flag it, because it does not cover the low-layer error helpers. - **The vendored spec does not declare the frame yet.** The `error` event's entry in `spec/openapi.yaml` arrives with the pending v2 spec sync, which is a separate open PR. This change deliberately does not depend on it, because the server already emits the frame. The rot guard in `src/low/spec-coverage.test.ts` fails when that sync lands, which is the cue to pin the declared codes against `SSE_STATUS_BY_CODE`. - **Not tested against the live server.** The server-side emitter (the public-api events handler's re-poll → error path) is not public, and I didn't run it. The behaviour is tested only against the stub server's copy of the wire format named in the change request: `event: error` with an `{error: {code, message}}` envelope. - **Overlap with the open PR for the 429 reconnect-pause floor.** That PR edits the `catch` block of `Job.events()` and the stub's 429 state. This PR edits the frame loop and adds a separate stub field, so the two should rebase cleanly. Whichever lands second should re-run `src/sdk/jobs.test.ts`. ## Provenance - **Authored by:** agent-work loop - **Verified:** - Tests: `pnpm test` 818 passed / 5 skipped. This includes prototype-key, generic-code, message-fallback and sanitizing cases, an empty-data `error` frame, and a rejected iterator close (that case fails without the catch re-check) - Test files type-checked with a temporary tsconfig that includes them (`tsc --noEmit` excludes tests) - Lint/fmt/typecheck: `pnpm lint`, `pnpm format:check`, `pnpm typecheck` clean - Drift: `pnpm check:spec-drift` OK (generated + Router route) - Build: `pnpm build` OK - Merged with current `main` locally: lint, format, typecheck, spec-drift and `pnpm test` (822 passed / 5 skipped) all clean - CI `sdk-parity` is red on a shared drift, not on this branch: the Python SDK added `QueueBacklogFull`, which open PR #209 refreshes the snapshot for - **Deviations:** the ticket's test plan names `pollCount`; the stub's counter is `jobPollCount`. All other acceptance criteria are met as written. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi added the

pull-request

label

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