[PR #206] fix(events): raise a typed error on the terminal SSE error frame instead of reconnecting #207

New issue

Open

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

bsakaguchi commented

2026-10-31 22:02:19 +00:00

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-19942-sse-error-frame

📝 Commits (2)

5cfb314 fix(events): raise a typed error on the terminal SSE error frame instead of reconnecting
5f9e8fd fix(events): leave an error frame with an unknown code to the poll fallback

📊 Changes

10 files changed (+259 additions, -4 deletions)

View changed files

📝 CHANGELOG.md (+4 -0)
📝 README.md (+8 -0)
📝 src/comfy_low/__init__.py (+2 -0)
📝 src/comfy_low/errors.py (+42 -0)
📝 src/comfy_sdk/events.py (+3 -0)
📝 src/comfy_sdk/exceptions.py (+15 -2)
📝 src/comfy_sdk/jobs.py (+32 -2)
📝 tests/conftest.py (+11 -0)
📝 tests/test_error_mapping.py (+52 -0)
📝 tests/test_events.py (+90 -0)

📄 Description

Summary

The server ends a job's event stream with event: error and an error envelope (credential_expired, forbidden, or job_not_found) when it stops streaming for a reason other than the job finishing. event_from_raw returned None for that frame, so Job.events() / AsyncJob.events() skipped it, saw the stream close without a terminal status, polled GET /jobs/{id}, slept 0.1 s and reconnected. For job_not_found, while GET /jobs/{id} still answers, that loop never ends and keeps using up the concurrent-stream quota. For every code, the caller never learned why the stream ended.

Change

comfy_low.errors.sse_error_from_frame(data) (also exported from comfy_low): for the three documented codes it reads the frame's envelope with error_from_envelope, taking the status from the code (credential_expired→401, forbidden→403, job_not_found→404) and keeping code exactly as sent. It returns None for any other code or for a malformed frame (no error, or a non-dict or non-string code). Such a frame isn't known to end the stream for good (a later server might drain or cap a stream's lifetime), so it is left to the existing poll-and-reconnect path instead of being raised as an untyped HTTP 0 error.
credential_expired → Unauthorized is now mapped in both comfy_low.errors._BY_CODE and comfy_sdk.exceptions._BY_CODE. The caller gets comfy_sdk.exceptions.Unauthorized with .code == "credential_expired". to_sdk_error keeps .error_type == "unauthorized" (the class's Router bucket) for this alias, so error_type never names a value outside the closed Router set.
Both events() bodies check for raw.event == "error" before event_from_raw. For a known code they raise inside the existing try, and for any other code they skip the frame. The existing except ApiError sends it through to_sdk_error because the status is never 501. There is no refresh() and no sleep. Both docstrings are updated.
events.py: a comment on case _: explains why error is not an Event subclass.
CHANGELOG Unreleased → Fixed entry, plus a short paragraph in the README's Live progress section saying a server-ended stream raises instead of reconnecting.

Judgment calls

The ticket asked for the sse_error_from_frame unit test in tests/test_errors.py. That file does not exist, so the tests went into the existing tests/test_error_mapping.py, which already covers error_from_envelope.
The ticket's state.poll_count is state.job_poll_count in the stub. The tests get the job from client.submit(...), which does not poll, and record the poll count after that.
Each stream test stops after 4 events. Without the fix, the reconnect loop made the test hang instead of fail.
I also exported sse_error_from_frame from comfy_low, next to error_from_envelope. The ticket did not ask for this.

Residual

The vendored spec/openapi.yaml does not list an error event under getJobEvents' x-sse-events. This PR handles the frame because the server already sends it, but the contract does not document it yet. The fix belongs in the canonical spec, followed by a re-sync. The vendored spec is not edited here (one-way copy).
Not run against a live server. The frame shape {"error": {"code", "message"}} and the three codes come from the server-side description of the change. They are exercised only through the stdlib test stub. tests/integration/test_gateway_e2e.py was skipped as usual (no credentials set) and does not cover this path anyway.
Async stream cleanup is unchanged. Raising out of async for leaves the low-level async generator for the GC to finalize instead of explicitly calling aclose(). The existing terminal-status return in the same loop already behaves this way, so this is not a regression, but it could be its own small cleanup.

Provenance

Authored by: agent-work loop
Verified:
- Tests: all green — 1273 passed, 9 skipped (integration, no credentials)
- Lint / format / mypy src: clean
- Codegen drift + Router error-type drift: OK; public-repo hygiene: OK
- Mutation-tested: the new stream tests fail with the raw.event == "error" check disabled, and the unknown-code fallback tests fail when every error frame is raised
- Not run locally: the 3.10/3.11/3.13 matrix (ran on 3.12); build-check
Deviations: the sse_error_from_frame tests are in tests/test_error_mapping.py, not tests/test_errors.py, which does not exist. Unlike the original ticket, only the three documented codes raise, and any other error frame falls back to polling (review feedback).

🤖 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-python-sdk/pull/206 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 10/9/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `matt/be-19942-sse-error-frame` --- ### 📝 Commits (2) - [`5cfb314`](https://github.com/Comfy-Org/comfy-python-sdk/commit/5cfb31475e704484e90ccc8dfb8c6ad2c2b3d351) fix(events): raise a typed error on the terminal SSE error frame instead of reconnecting - [`5f9e8fd`](https://github.com/Comfy-Org/comfy-python-sdk/commit/5f9e8fd33eb75470f36029d6efb901f55aa2af5e) fix(events): leave an error frame with an unknown code to the poll fallback ### 📊 Changes **10 files changed** (+259 additions, -4 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+4 -0) 📝 `README.md` (+8 -0) 📝 `src/comfy_low/__init__.py` (+2 -0) 📝 `src/comfy_low/errors.py` (+42 -0) 📝 `src/comfy_sdk/events.py` (+3 -0) 📝 `src/comfy_sdk/exceptions.py` (+15 -2) 📝 `src/comfy_sdk/jobs.py` (+32 -2) 📝 `tests/conftest.py` (+11 -0) 📝 `tests/test_error_mapping.py` (+52 -0) 📝 `tests/test_events.py` (+90 -0) </details> ### 📄 Description ## Summary The server ends a job's event stream with `event: error` and an error envelope (`credential_expired`, `forbidden`, or `job_not_found`) when it stops streaming for a reason other than the job finishing. `event_from_raw` returned `None` for that frame, so `Job.events()` / `AsyncJob.events()` skipped it, saw the stream close without a terminal status, polled `GET /jobs/{id}`, slept 0.1 s and reconnected. For `job_not_found`, while `GET /jobs/{id}` still answers, that loop never ends and keeps using up the concurrent-stream quota. For every code, the caller never learned why the stream ended. ## Change - `comfy_low.errors.sse_error_from_frame(data)` (also exported from `comfy_low`): for the three documented codes it reads the frame's envelope with `error_from_envelope`, taking the status from the code (`credential_expired`→401, `forbidden`→403, `job_not_found`→404) and keeping `code` exactly as sent. It returns `None` for any other code or for a malformed frame (no `error`, or a non-dict or non-string code). Such a frame isn't known to end the stream for good (a later server might drain or cap a stream's lifetime), so it is left to the existing poll-and-reconnect path instead of being raised as an untyped `HTTP 0` error. - `credential_expired` → `Unauthorized` is now mapped in both `comfy_low.errors._BY_CODE` and `comfy_sdk.exceptions._BY_CODE`. The caller gets `comfy_sdk.exceptions.Unauthorized` with `.code == "credential_expired"`. `to_sdk_error` keeps `.error_type == "unauthorized"` (the class's Router bucket) for this alias, so `error_type` never names a value outside the closed Router set. - Both `events()` bodies check for `raw.event == "error"` before `event_from_raw`. For a known code they raise inside the existing `try`, and for any other code they skip the frame. The existing `except ApiError` sends it through `to_sdk_error` because the status is never 501. There is no `refresh()` and no sleep. Both docstrings are updated. - `events.py`: a comment on `case _:` explains why `error` is not an `Event` subclass. - CHANGELOG `Unreleased → Fixed` entry, plus a short paragraph in the README's *Live progress* section saying a server-ended stream raises instead of reconnecting. ## Judgment calls - The ticket asked for the `sse_error_from_frame` unit test in `tests/test_errors.py`. That file does not exist, so the tests went into the existing `tests/test_error_mapping.py`, which already covers `error_from_envelope`. - The ticket's `state.poll_count` is `state.job_poll_count` in the stub. The tests get the job from `client.submit(...)`, which does not poll, and record the poll count after that. - Each stream test stops after 4 events. Without the fix, the reconnect loop made the test hang instead of fail. - I also exported `sse_error_from_frame` from `comfy_low`, next to `error_from_envelope`. The ticket did not ask for this. ## Residual - **The vendored `spec/openapi.yaml` does not list an `error` event** under `getJobEvents`' `x-sse-events`. This PR handles the frame because the server already sends it, but the contract does not document it yet. The fix belongs in the canonical spec, followed by a re-sync. The vendored spec is not edited here (one-way copy). - **Not run against a live server.** The frame shape `{"error": {"code", "message"}}` and the three codes come from the server-side description of the change. They are exercised only through the stdlib test stub. `tests/integration/test_gateway_e2e.py` was skipped as usual (no credentials set) and does not cover this path anyway. - **Async stream cleanup is unchanged.** Raising out of `async for` leaves the low-level async generator for the GC to finalize instead of explicitly calling `aclose()`. The existing terminal-status `return` in the same loop already behaves this way, so this is not a regression, but it could be its own small cleanup. ## Provenance - **Authored by:** agent-work loop - **Verified:** - Tests: all green — 1273 passed, 9 skipped (integration, no credentials) - Lint / format / mypy `src`: clean - Codegen drift + Router error-type drift: OK; public-repo hygiene: OK - Mutation-tested: the new stream tests fail with the `raw.event == "error"` check disabled, and the unknown-code fallback tests fail when every error frame is raised - Not run locally: the 3.10/3.11/3.13 matrix (ran on 3.12); `build-check` - **Deviations:** the `sse_error_from_frame` tests are in `tests/test_error_mapping.py`, not `tests/test_errors.py`, which does not exist. Unlike the original ticket, only the three documented codes raise, and any other `error` frame falls back to polling (review feedback). 🤖 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:02:19 +00:00