[PR #157] [MERGED] fix(errors): make `except RouterError` catch every Router refusal #157

New issue

Closed

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

bsakaguchi commented

2026-10-31 22:01:52 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-python-sdk/pull/157
Author: @mattmillerai
Created: 9/16/2026
Status: ✅ Merged
Merged: 9/19/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-14632-router-error-hierarchy

📝 Commits (6)

868df8d fix(errors): make except RouterError catch every Router refusal
9253dce Merge remote-tracking branch 'origin/main' into matt/be-14632-router-error-hierarchy
ecffbb1 docs(errors): state the untyped fallbacks and the constructor break
20ef7c5 Merge remote-tracking branch 'origin/main' into matt/be-14632-router-error-hierarchy
e41dcfe test(errors): cover the three queue buckets in the refusal inventory
5c8bab7 fix(errors): gate the cancel-refusal lookup on 409

📊 Changes

10 files changed (+868 additions, -168 deletions)

View changed files

📝 CHANGELOG.md (+35 -0)
📝 README.md (+62 -1)
📝 src/comfy_low/errors.py (+73 -3)
📝 src/comfy_sdk/__init__.py (+12 -0)
➕ src/comfy_sdk/_errors.py (+104 -0)
📝 src/comfy_sdk/exceptions.py (+142 -138)
📝 src/comfy_sdk/router_exceptions.py (+105 -18)
📝 tests/conftest.py (+11 -0)
➕ tests/test_exception_modules.py (+309 -0)
📝 tests/test_router_exceptions.py (+15 -8)

📄 Description

ELI-5

The SDK had two sets of exception classes that shared three names. The one you
could import and would obviously write a handler for was not the one the SDK
actually raised. So this:

try:
    client.models.subscribe(model, params)
except RouterError:          # never fired
    ...

compiled, type-checked, read as a correct catch-all, and caught nothing. A
workspace with no credits got an unhandled exception in production instead.

The three shared buckets are now one class each, so either import path
catches what the other does and except RouterError catches every Router
refusal.

The collision, before and after

comfy_sdk.exceptions and comfy_sdk.router_exceptions both exported
Unauthorized, Forbidden and InsufficientCredits — as different classes.
to_sdk_error raised the exceptions ones, which descended from ComfyError
but not from RouterError, so the two hierarchies were siblings and neither
caught the other.

before:  E.InsufficientCredits is R.InsufficientCredits  ->  False
after:   E.InsufficientCredits is R.InsufficientCredits  ->  True

The root cause was a circular import. router_exceptions needs ComfyError
from exceptions; exceptions therefore could not name a RouterError
subclass, so it grew its own three beside the Router ones. ComfyError moves
into a new, tiny comfy_sdk/_errors.py, which breaks the cycle and lets the
dependency run one way: _errors → router_exceptions → exceptions. Nothing
about ComfyError itself changed, and comfy_sdk.exceptions.ComfyError is the
same object it was.

What changed

Three buckets, one class each. Unauthorized, Forbidden,
InsufficientCredits are defined in router_exceptions and re-exported from
exceptions. Every existing import keeps working and keeps catching.
An unknown Router bucket now raises RouterError, not a bare
ComfyError. ApiError.error_type is new: it records the bucket the response
named (X-Comfy-Error-Type, or the body's error_type), which is also how
the SDK tells a Router response from a v2 one. Without it, except RouterError would still have had a hole on exactly the refusals nobody can
enumerate in advance.
RouterError is exported from the package root, with CancelRefused and
AlreadyCompleted. The fifteen per-bucket classes stay in
comfy_sdk.router_exceptions — one import path for the whole closed set
rather than half of it in each place.
A refused cancel raises a named exception. 409 {"status": "ALREADY_COMPLETED"} names no bucket and no code at all, so it arrived as an
untyped 409 that could only be identified by substring-matching the response
body. It now raises AlreadyCompleted, under a CancelRefused base so a
second refusal shape is one table entry plus one subclass rather than a second
substring match.
README gains a Catching Comfy Router errors section with the correct
import path, and states the consequence below.

The consequence worth reviewing

One class cannot descend from RouterError on one surface and not on the other.
So a workflow call that fails 401/403/402 now raises a RouterError
subclass too. except Unauthorized / except Forbidden /
except InsufficientCredits (from either module) and except ComfyError are
unchanged; only except RouterError sees more than its name suggests, and only
for those three buckets. This is the smaller of the two wrongs — the alternative
leaves a documented, importable, inert handler in place — and it is called out
in the README, the CHANGELOG and both module docstrings.

The one other behaviour interaction, spelled out at ApiError.error_type:
retry.error_bucket_of reads error_type before code, so it now answers with
this attribute on a bucketed response. That is the same string either way — the
two error contracts are mutually exclusive on the wire (Router's body is
{detail, error_type} and carries no error.code; a v2 envelope carries
error.code and no bucket), so code is set from the bucket whenever there
is one. should_retry itself is status-driven and references no exception class,
so nothing about what is retried moved. The 1,300-line retry suite is unchanged
and green.

Acceptance

except RouterError catches every bucket, asserted per bucket —
tests/test_exception_modules.py drives all 15 contract buckets plus a
deferred one and an invented one through a real stubbed call, on
models.submit, models.run and (for the shape the report reproduced)
models.subscribe with a 402. Sync and async.
Both import paths resolve to what is raised — asserted directly, in both
directions, from a real refusal.
A test enumerates both modules' exports and fails on any shared name that
is not the same object — over __all__ and dir(), because a class left
out of __all__ is still importable and still as dead a handler. Plus a
guard against that test passing vacuously if the modules stopped sharing
names, and a separate assertion that the merge landed on the RouterError
side (identity alone would be satisfied by merging the wrong way).
ALREADY_COMPLETED raises AlreadyCompleted, sync and async, with no
test matching on body text. Covered alongside: a 409 naming a real bucket
is still decided by that bucket, and a 409 naming no status at all stays
untyped rather than being guessed at.
Sync and async are covered above. TypeScript was read; see Residual.
README shows the import path and the example.

Corpus swept: 3 colliding names across the two modules, 3 fixed, 0
remaining (that is the assertion in criterion 3). Router buckets reachable as a
RouterError through to_sdk_error: 15 of 15, was 12 of 15, plus unknown
buckets which were 0 of ∞ and are now covered by the base class.

Residual

The TypeScript SDK has the identical defect and is NOT fixed here (it is a
different repository; this PR does not cross repo boundaries). Confirmed by
reading it, not assumed: sdk/exceptions.ts defines InsufficientCredits,
Unauthorized and Forbidden extending ComfyError, sdk/routerErrors.ts
defines three more of the same names extending RouterError, and
BY_CODE.insufficient_credits points at the exceptions.ts one — so
models.run throws it and catch (e instanceof routerErrors.RouterError) is
dead there exactly as it was here. sdk/models.test.ts pins that behaviour.
It is worse in one respect: RouterError extends Error, not ComfyError, so
the catch ComfyError escape hatch this SDK offered does not exist there.
The same three names, same three classes. Worth its own ticket against that
repo.
No Go SDK was found to check — no sibling checkout and no reference to one
from this repo. Reported as not-found rather than as absent: if one exists
elsewhere it has not been checked.
The report's own reproduction was not re-run. It was made against
production with a key bound to a credit-less workspace; this environment has
no such credential, and a live Router call is not something to conjure for a
test. The refusal is reproduced instead against the repo's stub server in the
exact shape Router sends (bucket on the header and in the body, no v2
envelope), on the same call the report used.
The reference script the report points at is not present on this machine
(a path under a reporter's own home directory), so the workaround it documents
was not read first-hand. The workaround it is described as using — catching
ComfyError and branching on type(exc).__name__ — is made unnecessary by
this change, but that is inferred from the description, not verified against
the file.
Two related tracker items referenced by the report could not be read from
this environment. One is described as adding a second cancel-refusal shape
("in-flight tasks cannot be cancelled"); its wire shape is unknown, so no
entry was invented for it. CancelRefused plus the one-entry-per-shape table
in comfy_low.errors is the seam it lands in. The other is described as e2e
credential coverage that asserts on the wire because of this bug; whether
those assertions can now be rewritten against classes was not checked.
CancelRefused is not itself raised by anything today, deliberately.
Unlike an unknown bucket — where the response still named a bucket, so
RouterError is an honest answer — an unrecognised refusal shape carries
nothing identifying it as a refusal: it is a 409 with no bucket and no code,
indistinguishable from any other. Those stay untyped ComfyErrors rather than
being guessed at. The class is there as the catchable base for the family.

Provenance

Authored by: agent-work loop
Verified: uv run --extra dev pytest: 931 passed, 4 skipped, 0 failed
(65 of those in the new tests/test_exception_modules.py);
ruff check .: clean; ruff format --check .: 56 files already formatted;
mypy src: no issues in 21 files; scripts/check_drift.py: models in sync,
all 15 router error types covered and blessed, run route matches the spec;
scripts/check_public_repo_hygiene.py: no internal-only references. One
pre-existing test was inverted rather than deleted:
test_the_shared_names_are_not_the_workflow_surface_classes asserted the
collision this PR removes, and now asserts the identity, with a comment saying
why it flipped.
Deviations: the TypeScript half of acceptance criterion 5 is checked and
reported, not fixed — see Residual. No Go SDK was found to check.

Summary by CodeRabbit

New Features
- Added typed Router errors for authentication, credit, and cancellation-refusal scenarios.
- Added AlreadyCompleted for cancellation attempts after work has finished.
- Router error categories and details are now preserved for more precise error handling.
- Shared exception classes are consistently available across supported import paths.
Documentation
- Expanded guidance for handling Router errors, authentication and credit failures, unknown error categories, and cancellation refusals.

_{🔄 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/157 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/16/2026 **Status:** ✅ Merged **Merged:** 9/19/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-14632-router-error-hierarchy` --- ### 📝 Commits (6) - [`868df8d`](https://github.com/Comfy-Org/comfy-python-sdk/commit/868df8d721d582dda65ba00fe32b5080a8bd8540) fix(errors): make `except RouterError` catch every Router refusal - [`9253dce`](https://github.com/Comfy-Org/comfy-python-sdk/commit/9253dce14fc339506963991f41e7e0661b2ca32b) Merge remote-tracking branch 'origin/main' into matt/be-14632-router-error-hierarchy - [`ecffbb1`](https://github.com/Comfy-Org/comfy-python-sdk/commit/ecffbb1a02fe89c50bf164780ca570a4f200606f) docs(errors): state the untyped fallbacks and the constructor break - [`20ef7c5`](https://github.com/Comfy-Org/comfy-python-sdk/commit/20ef7c5a3cee7318d6b55bd40b3a8fb12cf3560a) Merge remote-tracking branch 'origin/main' into matt/be-14632-router-error-hierarchy - [`e41dcfe`](https://github.com/Comfy-Org/comfy-python-sdk/commit/e41dcfecdd565e040b0f126e4d2702134dc35b6e) test(errors): cover the three queue buckets in the refusal inventory - [`5c8bab7`](https://github.com/Comfy-Org/comfy-python-sdk/commit/5c8bab7d3ad059be812490f63ccda93ad1d23b92) fix(errors): gate the cancel-refusal lookup on 409 ### 📊 Changes **10 files changed** (+868 additions, -168 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+35 -0) 📝 `README.md` (+62 -1) 📝 `src/comfy_low/errors.py` (+73 -3) 📝 `src/comfy_sdk/__init__.py` (+12 -0) ➕ `src/comfy_sdk/_errors.py` (+104 -0) 📝 `src/comfy_sdk/exceptions.py` (+142 -138) 📝 `src/comfy_sdk/router_exceptions.py` (+105 -18) 📝 `tests/conftest.py` (+11 -0) ➕ `tests/test_exception_modules.py` (+309 -0) 📝 `tests/test_router_exceptions.py` (+15 -8) </details> ### 📄 Description ## ELI-5 The SDK had two sets of exception classes that shared three names. The one you could import and would obviously write a handler for was not the one the SDK actually raised. So this: ```python try: client.models.subscribe(model, params) except RouterError: # never fired ... ``` compiled, type-checked, read as a correct catch-all, and caught nothing. A workspace with no credits got an unhandled exception in production instead. The three shared buckets are now **one class each**, so either import path catches what the other does and `except RouterError` catches every Router refusal. ## The collision, before and after `comfy_sdk.exceptions` and `comfy_sdk.router_exceptions` both exported `Unauthorized`, `Forbidden` and `InsufficientCredits` — as *different* classes. `to_sdk_error` raised the `exceptions` ones, which descended from `ComfyError` but not from `RouterError`, so the two hierarchies were siblings and neither caught the other. ``` before: E.InsufficientCredits is R.InsufficientCredits -> False after: E.InsufficientCredits is R.InsufficientCredits -> True ``` The root cause was a **circular import**. `router_exceptions` needs `ComfyError` from `exceptions`; `exceptions` therefore could not name a `RouterError` subclass, so it grew its own three beside the Router ones. `ComfyError` moves into a new, tiny `comfy_sdk/_errors.py`, which breaks the cycle and lets the dependency run one way: `_errors` → `router_exceptions` → `exceptions`. Nothing about `ComfyError` itself changed, and `comfy_sdk.exceptions.ComfyError` is the same object it was. ## What changed - **Three buckets, one class each.** `Unauthorized`, `Forbidden`, `InsufficientCredits` are defined in `router_exceptions` and re-exported from `exceptions`. Every existing import keeps working and keeps catching. - **An unknown Router bucket now raises `RouterError`,** not a bare `ComfyError`. `ApiError.error_type` is new: it records the bucket the response named (`X-Comfy-Error-Type`, or the body's `error_type`), which is also how the SDK tells a Router response from a v2 one. Without it, `except RouterError` would still have had a hole on exactly the refusals nobody can enumerate in advance. - **`RouterError` is exported from the package root,** with `CancelRefused` and `AlreadyCompleted`. The fifteen per-bucket classes stay in `comfy_sdk.router_exceptions` — one import path for the whole closed set rather than half of it in each place. - **A refused cancel raises a named exception.** `409 {"status": "ALREADY_COMPLETED"}` names no bucket and no code at all, so it arrived as an untyped `409` that could only be identified by substring-matching the response body. It now raises `AlreadyCompleted`, under a `CancelRefused` base so a second refusal shape is one table entry plus one subclass rather than a second substring match. - **README** gains a *Catching Comfy Router errors* section with the correct import path, and states the consequence below. ## The consequence worth reviewing One class cannot descend from `RouterError` on one surface and not on the other. So a **workflow** call that fails `401`/`403`/`402` now raises a `RouterError` subclass too. `except Unauthorized` / `except Forbidden` / `except InsufficientCredits` (from either module) and `except ComfyError` are unchanged; only `except RouterError` sees more than its name suggests, and only for those three buckets. This is the smaller of the two wrongs — the alternative leaves a documented, importable, inert handler in place — and it is called out in the README, the CHANGELOG and both module docstrings. The one other behaviour interaction, spelled out at `ApiError.error_type`: `retry.error_bucket_of` reads `error_type` before `code`, so it now answers with this attribute on a bucketed response. That is the same string either way — the two error contracts are mutually exclusive on the wire (Router's body is `{detail, error_type}` and carries no `error.code`; a v2 envelope carries `error.code` and no bucket), so `code` is *set from* the bucket whenever there is one. `should_retry` itself is status-driven and references no exception class, so nothing about what is retried moved. The 1,300-line retry suite is unchanged and green. ## Acceptance 1. **`except RouterError` catches every bucket, asserted per bucket** — `tests/test_exception_modules.py` drives all 15 contract buckets plus a deferred one and an invented one through a real stubbed call, on `models.submit`, `models.run` and (for the shape the report reproduced) `models.subscribe` with a `402`. Sync and async. 2. **Both import paths resolve to what is raised** — asserted directly, in both directions, from a real refusal. 3. **A test enumerates both modules' exports and fails on any shared name that is not the same object** — over `__all__` *and* `dir()`, because a class left out of `__all__` is still importable and still as dead a handler. Plus a guard against that test passing vacuously if the modules stopped sharing names, and a separate assertion that the merge landed on the `RouterError` side (identity alone would be satisfied by merging the wrong way). 4. **`ALREADY_COMPLETED` raises `AlreadyCompleted`**, sync and async, with no test matching on body text. Covered alongside: a `409` naming a real bucket is still decided by that bucket, and a `409` naming no status at all stays untyped rather than being guessed at. 5. **Sync and async** are covered above. **TypeScript** was read; see Residual. 6. **README** shows the import path and the example. Corpus swept: **3** colliding names across the two modules, **3** fixed, **0** remaining (that is the assertion in criterion 3). Router buckets reachable as a `RouterError` through `to_sdk_error`: **15 of 15**, was 12 of 15, plus unknown buckets which were 0 of ∞ and are now covered by the base class. ## Residual - **The TypeScript SDK has the identical defect and is NOT fixed here** (it is a different repository; this PR does not cross repo boundaries). Confirmed by reading it, not assumed: `sdk/exceptions.ts` defines `InsufficientCredits`, `Unauthorized` and `Forbidden` extending `ComfyError`, `sdk/routerErrors.ts` defines three more of the same names extending `RouterError`, and `BY_CODE.insufficient_credits` points at the `exceptions.ts` one — so `models.run` throws it and `catch (e instanceof routerErrors.RouterError)` is dead there exactly as it was here. `sdk/models.test.ts` pins that behaviour. It is worse in one respect: `RouterError extends Error`, not `ComfyError`, so the `catch ComfyError` escape hatch this SDK offered does not exist there. The same three names, same three classes. Worth its own ticket against that repo. - **No Go SDK was found** to check — no sibling checkout and no reference to one from this repo. Reported as not-found rather than as absent: if one exists elsewhere it has not been checked. - **The report's own reproduction was not re-run.** It was made against production with a key bound to a credit-less workspace; this environment has no such credential, and a live Router call is not something to conjure for a test. The refusal is reproduced instead against the repo's stub server in the exact shape Router sends (bucket on the header and in the body, no v2 envelope), on the same call the report used. - **The reference script the report points at is not present** on this machine (a path under a reporter's own home directory), so the workaround it documents was not read first-hand. The workaround it is described as using — catching `ComfyError` and branching on `type(exc).__name__` — is made unnecessary by this change, but that is inferred from the description, not verified against the file. - **Two related tracker items referenced by the report could not be read** from this environment. One is described as adding a *second* cancel-refusal shape ("in-flight tasks cannot be cancelled"); its wire shape is unknown, so no entry was invented for it. `CancelRefused` plus the one-entry-per-shape table in `comfy_low.errors` is the seam it lands in. The other is described as e2e credential coverage that asserts on the wire *because* of this bug; whether those assertions can now be rewritten against classes was not checked. - **`CancelRefused` is not itself raised by anything today**, deliberately. Unlike an unknown *bucket* — where the response still named a bucket, so `RouterError` is an honest answer — an unrecognised refusal shape carries nothing identifying it as a refusal: it is a `409` with no bucket and no code, indistinguishable from any other. Those stay untyped `ComfyError`s rather than being guessed at. The class is there as the catchable base for the family. ## Provenance - **Authored by:** agent-work loop - **Verified:** `uv run --extra dev pytest`: 931 passed, 4 skipped, 0 failed (65 of those in the new `tests/test_exception_modules.py`); `ruff check .`: clean; `ruff format --check .`: 56 files already formatted; `mypy src`: no issues in 21 files; `scripts/check_drift.py`: models in sync, all 15 router error types covered and blessed, run route matches the spec; `scripts/check_public_repo_hygiene.py`: no internal-only references. One pre-existing test was inverted rather than deleted: `test_the_shared_names_are_not_the_workflow_surface_classes` asserted the collision this PR removes, and now asserts the identity, with a comment saying why it flipped. - **Deviations:** the TypeScript half of acceptance criterion 5 is checked and reported, not fixed — see Residual. No Go SDK was found to check.  ## Summary by CodeRabbit - **New Features** - Added typed Router errors for authentication, credit, and cancellation-refusal scenarios. - Added `AlreadyCompleted` for cancellation attempts after work has finished. - Router error categories and details are now preserved for more precise error handling. - Shared exception classes are consistently available across supported import paths. - **Documentation** - Expanded guidance for handling Router errors, authentication and credit failures, unknown error categories, and cancellation refusals.  --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 22:01:52 +00:00