[PR #132] [MERGED] fix(errors): drop 409 from the status->code fallback so a code-less 409 raises ComfyError #132

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-9933-drop-409-status-fallback

📝 Commits (4)

fe623ed fix(errors): drop 409 from the status->code fallback
8059666 Merge remote-tracking branch 'origin/main' into matt/be-9933-drop-409-status-fallback
86d7950 fix: address bot review comments
fd8eab6 Merge remote-tracking branch 'origin/main' into matt/be-9933-drop-409-status-fallback

📊 Changes

3 files changed (+150 additions, -9 deletions)

View changed files

📝 CHANGELOG.md (+17 -0)
📝 src/comfy_low/errors.py (+54 -7)
📝 tests/test_error_mapping.py (+79 -2)

📄 Description

ELI-5

When the server sends back an error, it normally says what went wrong in a
machine-readable error.code. If it doesn't, the SDK guesses the code from the
HTTP status. For 409 Conflict the guess was hash_mismatch — "the bytes you
uploaded don't match the hash you declared, upload them again."

That guess was only ever consulted for responses that came from somewhere other
than the surface where a real hash mismatch happens, and it told those callers to
re-upload bytes over a conflict that had nothing to do with bytes. This removes
the guess. A 409 that names no code is now just a ComfyError that says 409.
Real hash mismatches are unaffected whenever their body decodes — error.code names them and wins outright. (The exception is a body that will not parse at all; see The one path this does not cover below.)

Why the guess was never reachable for a real hash mismatch

_CODE_BY_STATUS is consulted only when the response named no code of its own
(src/comfy_low/errors.py): no envelope error.code, and no Router bucket from
X-Comfy-Error-Type / a top-level body error_type. Verified against the
vendored specs:

ErrorEnvelope lists code and message under required, so every v2
envelope response carries a code and the table never decides one.
spec/openapi.yaml documents exactly two 409s, both answering in
ErrorEnvelope: hash_mismatch on POST /api/v2/assets and asset_in_use on
DELETE /api/v2/assets/{id}. So the contract itself already spells the status
two ways — the status alone cannot say which even on the compliant surface.
spec/router-openapi.yaml documents one 409 (RouterIdempotencyConflict
on POST /v2/models/{provider}/{model}), whose body is RouterErrorResponse
— {detail, error_type}, with error_type required and no error.code. That
path is decided by the bucket, which already outranks the table, so it cannot
regress here.

What is left reaching the table on a 409 is therefore Router-shaped bodies
whose bucket was stripped, and intermediaries — neither bound by what any route
documents for the status.

The one path this does not cover

transport.py sets body=None whenever the error body fails to JSON-decode — an
HTML proxy page, a truncated response. Such a 409 names no code, so it reaches
the table, and after this change a genuine POST /assets mismatch behind a broken
intermediary raises ComfyError rather than HashMismatch.

That is the trade this change makes rather than an oversight: on a body that will
not parse there is nothing that distinguishes the two documented 409s, and
inventing the re-upload one is exactly the guess being removed. Scoping the drop
to the ambiguous route is not available — error_from_response sees status, body
and headers, not the route. Raised by the Cursor panel; the docstring now states
this explicitly instead of claiming the assets path is unconditionally unaffected.

Change

409: "hash_mismatch" removed from _CODE_BY_STATUS. A code-less 409 now
gets code = "error" and stays a bare ApiError carrying the real
http_status, the response's own message (or HTTP 409), and retry_after;
to_sdk_error surfaces it as a plain ComfyError.
The table gains the admission rule it now applies, so the next status
added is judged the same way: only a status with ONE meaning across every
documented surface gets a typed guess. 422 and 429 are kept deliberately
— their possible misreadings stay inside the right action class (terminal
refusal / back-off-and-retry), unlike HashMismatch, which tells the caller
to re-upload bytes.
Tests in tests/test_error_mapping.py (module docstring widened from its 404
scope) pin: a code-less 409 is a plain ApiError with code == "error" and
http_status == 409 and to_sdk_error gives exactly ComfyError; a code-less
409 keeps retry_after on both layers; an enveloped hash_mismatch 409 is
still HashMismatch on both layers; a body-less 401 is still Unauthorized.
CHANGELOG entry under Unreleased → Fixed.

Riskiest line, and why it is safe

The deletion itself. Three things independently keep it from changing anything a
caller relies on:

The assets path. Enveloped hash_mismatch still wins outright
(error.code is read before the table). tests/test_assets.py's
test_hash_mismatch_surfaced_without_blind_retry — which drives the stub
server's real 409 hash_mismatch and asserts exactly one upload attempt —
passes unchanged.
Retry behaviour. is_collectable gates a 409 on
bucket == "concurrency_limit_exceeded". The old code-less bucket was
hash_mismatch; the new one is error. Neither matches, so a code-less 409
stays a terminal refusal under RetryPolicy exactly as before — no new
same-key resend, no new billed generation.
The router path. Decided by the bucket, which outranks the table on every
status; the existing (409, "invalid_input") and
concurrency_limit_exceeded → ConcurrencyLimitExceeded cases still pass.

_CODE_BY_STATUS has exactly one reader (error_from_envelope), so there are no
other call sites to update.

Sweep of what is not changed

I re-read all 6 remaining entries in the table against the admission rule this
PR writes down. None fails it the way 409 did: where a status is spelled
differently by the two surfaces (404 not_found vs Router's model_not_found;
403 forbidden vs not_enabled) the readings stay inside one action class —
terminal, fix-the-request — and none of them directs the caller at a distinct
mutating action the way HashMismatch directs a re-upload. So no further entry
is dropped here, and this is a deliberate finding rather than an unexamined
remainder.

Behaviour change for callers

A caller catching HashMismatch around a 409 that arrives without an
error.code now sees ComfyError instead. That is the intended fix, and it is
documented in the CHANGELOG. Every 409 the contracts actually document — both
enveloped ones and the Router one — is unaffected.

Verification

Full required CI set locally on the worktree, plus the two non-test-job gates:

ruff check . — clean
ruff format --check . — 51 files already formatted
mypy src — no issues in 19 source files
pytest — 722 passed, 4 skipped (the 4 are the env-gated live gateway suite)
scripts/check_public_repo_hygiene.py — no internal-only references
scripts/check_drift.py — models in sync, all 15 router error types covered

Residual

The Router {detail, error_type} → typed RouterError work is out of scope
here and is only partly landed on main already. error_from_envelope
preserves the bucket and to_sdk_error selects the typed subclass, so
except NotEnabled and friends do fire — but that depends on the bucket
actually reaching this function via the X-Comfy-Error-Type header or a
top-level body error_type. A Router-shaped 409 that reaches the SDK with
its bucket stripped (an intermediary that drops response headers and rewrites
the body) still decodes off the status table, which after this PR means a plain
ComfyError rather than the ConcurrencyLimitExceeded it really was. This PR
deliberately does not try to recover a bucket that is not on the wire; it only
stops the table from asserting a wrong specific cause. Anyone picking this up
should look at whether {detail, error_type} bodies on the /models/run path
need a stronger identification path than the bucket alone.
Hedge on the sweep above. The 6-entry re-read is a judgement against the
documented surfaces in the two vendored specs, not a measurement of live
traffic. If an intermediary is in practice minting bucket-less 403s or 404s
the way the record shows it mints 409s, the same argument would apply to
those entries and it would be visible in production error telemetry, which I
cannot query from here.
Unexercised artifacts. The investigation this work came from, and its
findings write-up, are in an internal tracker I have no access to — I could not
read them and reconstructed the reasoning from the vendored specs and the code
instead (which is what the empirical section above is). The prior change that
added request_id and made to_sdk_error forward retry_after was open when
this work was specified; I confirmed it is merged by reading main
(src/comfy_sdk/exceptions.py), not by reading that PR, and added the
to_sdk_error retry_after assertion accordingly. No live server was called —
the suite is stdlib-stub-driven by design.

Provenance

Authored by: agent-work loop
Verified: ruff check: clean; ruff format --check: 51 files already formatted; mypy src: no issues in 19 files; pytest: 722 passed, 4 skipped; check_public_repo_hygiene.py: clean; check_drift.py: models in sync, 15/15 router error types covered
Deviations: none — all acceptance criteria met

Summary by CodeRabbit

Bug Fixes
- Corrected handling for conflict (409) responses without an explicit error code: they now remain generic errors instead of being classified as hash mismatches.
- Responses with explicit error codes continue to use the specified classification.
- Preserved existing retry metadata and behavior for 422, 429, and Retry-After responses.
- Bodyless unauthorized (401) responses continue to be identified correctly.

_{🔄 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/132 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/5/2026 **Status:** ✅ Merged **Merged:** 9/14/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-9933-drop-409-status-fallback` --- ### 📝 Commits (4) - [`fe623ed`](https://github.com/Comfy-Org/comfy-python-sdk/commit/fe623eddd757c741a95247e2fe0289e7b32eef67) fix(errors): drop 409 from the status->code fallback - [`8059666`](https://github.com/Comfy-Org/comfy-python-sdk/commit/805966634d6412f3d97a19710a6ec01b2f04134e) Merge remote-tracking branch 'origin/main' into matt/be-9933-drop-409-status-fallback - [`86d7950`](https://github.com/Comfy-Org/comfy-python-sdk/commit/86d79503b9a2cadea737821aaf0fadb23254d25b) fix: address bot review comments - [`fd8eab6`](https://github.com/Comfy-Org/comfy-python-sdk/commit/fd8eab65a2764a882a6d021c0b980828ae9e7a49) Merge remote-tracking branch 'origin/main' into matt/be-9933-drop-409-status-fallback ### 📊 Changes **3 files changed** (+150 additions, -9 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+17 -0) 📝 `src/comfy_low/errors.py` (+54 -7) 📝 `tests/test_error_mapping.py` (+79 -2) </details> ### 📄 Description ## ELI-5 When the server sends back an error, it normally says *what* went wrong in a machine-readable `error.code`. If it doesn't, the SDK guesses the code from the HTTP status. For `409 Conflict` the guess was `hash_mismatch` — "the bytes you uploaded don't match the hash you declared, upload them again." That guess was only ever consulted for responses that came from somewhere *other* than the surface where a real hash mismatch happens, and it told those callers to re-upload bytes over a conflict that had nothing to do with bytes. This removes the guess. A `409` that names no code is now just a `ComfyError` that says `409`. Real hash mismatches are unaffected whenever their body decodes — `error.code` names them and wins outright. (The exception is a body that will not parse at all; see *The one path this does not cover* below.) ## Why the guess was never reachable for a real hash mismatch `_CODE_BY_STATUS` is consulted **only** when the response named no code of its own (`src/comfy_low/errors.py`): no envelope `error.code`, and no Router bucket from `X-Comfy-Error-Type` / a top-level body `error_type`. Verified against the vendored specs: - `ErrorEnvelope` lists `code` and `message` under `required`, so **every** v2 envelope response carries a code and the table never decides one. - `spec/openapi.yaml` documents exactly **two** `409`s, both answering in `ErrorEnvelope`: `hash_mismatch` on `POST /api/v2/assets` and `asset_in_use` on `DELETE /api/v2/assets/{id}`. So the contract itself already spells the status two ways — the status alone cannot say which even on the compliant surface. - `spec/router-openapi.yaml` documents **one** `409` (`RouterIdempotencyConflict` on `POST /v2/models/{provider}/{model}`), whose body is `RouterErrorResponse` — `{detail, error_type}`, with `error_type` required and no `error.code`. That path is decided by the bucket, which already outranks the table, so it cannot regress here. What is left reaching the table on a `409` is therefore Router-shaped bodies whose bucket was stripped, and intermediaries — neither bound by what any route documents for the status. ## The one path this does not cover `transport.py` sets `body=None` whenever the error body fails to JSON-decode — an HTML proxy page, a truncated response. Such a `409` names no code, so it reaches the table, and after this change a genuine `POST /assets` mismatch behind a broken intermediary raises `ComfyError` rather than `HashMismatch`. That is the trade this change makes rather than an oversight: on a body that will not parse there is nothing that distinguishes the two documented `409`s, and inventing the re-upload one is exactly the guess being removed. Scoping the drop to the ambiguous route is not available — `error_from_response` sees status, body and headers, not the route. Raised by the Cursor panel; the docstring now states this explicitly instead of claiming the assets path is unconditionally unaffected. ## Change 1. `409: "hash_mismatch"` removed from `_CODE_BY_STATUS`. A code-less `409` now gets `code = "error"` and stays a bare `ApiError` carrying the real `http_status`, the response's own message (or `HTTP 409`), and `retry_after`; `to_sdk_error` surfaces it as a plain `ComfyError`. 2. The table gains the **admission rule** it now applies, so the next status added is judged the same way: only a status with ONE meaning across every documented surface gets a typed guess. `422` and `429` are kept deliberately — their possible misreadings stay inside the right *action class* (terminal refusal / back-off-and-retry), unlike `HashMismatch`, which tells the caller to re-upload bytes. 3. Tests in `tests/test_error_mapping.py` (module docstring widened from its 404 scope) pin: a code-less `409` is a plain `ApiError` with `code == "error"` and `http_status == 409` and `to_sdk_error` gives exactly `ComfyError`; a code-less `409` keeps `retry_after` on both layers; an enveloped `hash_mismatch` `409` is still `HashMismatch` on both layers; a body-less `401` is still `Unauthorized`. 4. CHANGELOG entry under Unreleased → Fixed. ## Riskiest line, and why it is safe The deletion itself. Three things independently keep it from changing anything a caller relies on: - **The assets path.** Enveloped `hash_mismatch` still wins outright (`error.code` is read before the table). `tests/test_assets.py`'s `test_hash_mismatch_surfaced_without_blind_retry` — which drives the stub server's real `409 hash_mismatch` and asserts exactly one upload attempt — passes unchanged. - **Retry behaviour.** `is_collectable` gates a `409` on `bucket == "concurrency_limit_exceeded"`. The old code-less bucket was `hash_mismatch`; the new one is `error`. Neither matches, so a code-less `409` stays a terminal refusal under `RetryPolicy` exactly as before — no new same-key resend, no new billed generation. - **The router path.** Decided by the bucket, which outranks the table on every status; the existing `(409, "invalid_input")` and `concurrency_limit_exceeded → ConcurrencyLimitExceeded` cases still pass. `_CODE_BY_STATUS` has exactly one reader (`error_from_envelope`), so there are no other call sites to update. ## Sweep of what is *not* changed I re-read all **6** remaining entries in the table against the admission rule this PR writes down. None fails it the way `409` did: where a status is spelled differently by the two surfaces (`404` `not_found` vs Router's `model_not_found`; `403` `forbidden` vs `not_enabled`) the readings stay inside one action class — terminal, fix-the-request — and none of them directs the caller at a distinct *mutating* action the way `HashMismatch` directs a re-upload. So no further entry is dropped here, and this is a deliberate finding rather than an unexamined remainder. ## Behaviour change for callers A caller catching `HashMismatch` around a `409` that arrives **without** an `error.code` now sees `ComfyError` instead. That is the intended fix, and it is documented in the CHANGELOG. Every `409` the contracts actually document — both enveloped ones and the Router one — is unaffected. ## Verification Full required CI set locally on the worktree, plus the two non-`test`-job gates: - `ruff check .` — clean - `ruff format --check .` — 51 files already formatted - `mypy src` — no issues in 19 source files - `pytest` — **722 passed, 4 skipped** (the 4 are the env-gated live gateway suite) - `scripts/check_public_repo_hygiene.py` — no internal-only references - `scripts/check_drift.py` — models in sync, all 15 router error types covered ## Residual - **The Router `{detail, error_type}` → typed `RouterError` work is out of scope here** and is only *partly* landed on `main` already. `error_from_envelope` preserves the bucket and `to_sdk_error` selects the typed subclass, so `except NotEnabled` and friends do fire — but that depends on the bucket actually reaching this function via the `X-Comfy-Error-Type` header or a top-level body `error_type`. A Router-shaped `409` that reaches the SDK with its bucket stripped (an intermediary that drops response headers and rewrites the body) still decodes off the status table, which after this PR means a plain `ComfyError` rather than the `ConcurrencyLimitExceeded` it really was. This PR deliberately does not try to recover a bucket that is not on the wire; it only stops the table from asserting a *wrong specific cause*. Anyone picking this up should look at whether `{detail, error_type}` bodies on the `/models/run` path need a stronger identification path than the bucket alone. - **Hedge on the sweep above.** The 6-entry re-read is a judgement against the documented surfaces in the two vendored specs, not a measurement of live traffic. If an intermediary is in practice minting bucket-less `403`s or `404`s the way the record shows it mints `409`s, the same argument would apply to those entries and it would be visible in production error telemetry, which I cannot query from here. - **Unexercised artifacts.** The investigation this work came from, and its findings write-up, are in an internal tracker I have no access to — I could not read them and reconstructed the reasoning from the vendored specs and the code instead (which is what the empirical section above is). The prior change that added `request_id` and made `to_sdk_error` forward `retry_after` was open when this work was specified; I confirmed it is merged by reading `main` (`src/comfy_sdk/exceptions.py`), not by reading that PR, and added the `to_sdk_error` retry_after assertion accordingly. No live server was called — the suite is stdlib-stub-driven by design. ## Provenance - **Authored by:** agent-work loop - **Verified:** ruff check: clean; ruff format --check: 51 files already formatted; mypy src: no issues in 19 files; pytest: 722 passed, 4 skipped; check_public_repo_hygiene.py: clean; check_drift.py: models in sync, 15/15 router error types covered - **Deviations:** none — all acceptance criteria met  ## Summary by CodeRabbit - **Bug Fixes** - Corrected handling for conflict (`409`) responses without an explicit error code: they now remain generic errors instead of being classified as hash mismatches. - Responses with explicit error codes continue to use the specified classification. - Preserved existing retry metadata and behavior for `422`, `429`, and `Retry-After` responses. - Bodyless unauthorized (`401`) responses continue to be identified correctly.  --- <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:39 +00:00