[PR #208] fix(assets): read a bodiless HEAD 429 on the by-hash probe as rate_limited, not queue_full #208

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/208
Author: @mattmillerai
Created: 10/9/2026
Status: 🔄 Open

Base: main ← Head: matt/be-19949-head-429-rate-limited

📝 Commits (2)

e659071 fix(assets): read a bodiless HEAD 429 on the by-hash probe as rate_limited, not queue_full
a1bc9a5 fix(assets): keep a named bucket on a bodiless HEAD 429 and pin the SDK class

📊 Changes

4 files changed (+187 additions, -3 deletions)

View changed files

📝 CHANGELOG.md (+9 -0)
📝 src/comfy_low/transport.py (+34 -3)
📝 tests/conftest.py (+10 -0)
➕ tests/test_head_refusal.py (+134 -0)

📄 Description

ELI-5

Before uploading a file, the SDK asks the server "do you already have these bytes?" with a HEAD request. When the gateway refuses that request because the account is rate-limited, it sends a 429 with no body (a HEAD response can't have one). With no body to read, the SDK guessed from the status code alone and reported a full job queue (QueueFull). That's the wrong error, and it sends callers off to debug a queue that has nothing to do with the problem. Now a bodiless 429 on that probe is reported as rate_limited, and the Retry-After wait is still included.

What changed

comfy_low/transport.py: new _Prepared.raise_for_head_refusal(resp). On a 429 it builds the same error error_from_envelope would build for {"error": {"code": "rate_limited", "message": "Rate limited"}}, with retry_after from the existing _retry_after header parser and request_id from the header. A 429 that names its own bucket on X-Comfy-Error-Type (for example concurrency_limit_exceeded) takes the existing generic path instead, so it keeps that bucket. Every other status also goes through the generic path, so a bodiless 403 is still Forbidden. ComfyLow.head_asset_by_hash and AsyncComfyLow.head_asset_by_hash call it after their 200/404 checks, and the docstring now says the code is synthesized from the status.
tests/conftest.py: new ServerState.head_refusal: (status, headers) | None setting. When it is set, the stub answers HEAD /assets/by-hash with that status and those headers and an empty body.
tests/test_head_refusal.py: tests for the low layer, sync and async:
- a bodiless 429 with Retry-After: 7 gives code == "rate_limited", http_status == 429, retry_after == 7, and the error is not QueueFull
- a bodiless 429 without the header gives retry_after is None
- a bodiless 429 with X-Comfy-Error-Type: concurrency_limit_exceeded keeps that code and error_type
- a bodiless 403 gives Forbidden
SDK tests: commit() (sync and async) against the 429 stub raises exactly RateLimited (code == "rate_limited", retry_after == 7), not QueueFull, and uploads nothing. commit() against the bucket-header 429 raises ConcurrencyLimitExceeded, and against the 403 stub raises Forbidden.
CHANGELOG.md: added an entry under Unreleased → Fixed.

Judgment calls

The SDK class a caller sees is the existing rate_limited mapping. comfy_sdk.exceptions._class_for already maps the code rate_limited through the Router bucket table, so commit() raises comfy_sdk.router_exceptions.RateLimited. That class is a ComfyError subclass with code == "rate_limited" and carries retry_after. A bodied rate_limited on any v2 route already surfaces this way, so this change matches that behavior instead of adding a class. No new exception class is exported and exportedErrorClasses is unchanged.
The 403 keeps the generic message HTTP 403. Changing it to Forbidden was optional, and I left it alone so this PR only changes the 429 case.
_CODE_BY_STATUS and the client's 429 retry helper (_retry_delay) are unchanged, so the bodiless queue_full fallback on POST /jobs still works.

Provenance

Authored by: agent-work loop
Verified:
- Tests: all green on the current head (1266 passed, 9 skipped; the skips include the env-gated gateway e2e suite), on Python 3.12 locally
- Lint/format/type check: ruff check, ruff format --check and mypy src all clean
- Public-repo hygiene script: clean
- Mutation checks: with the 429 branch disabled, the rate-limit tests fail; with the bucket-header guard removed, the 3 bucket tests fail
- Not run locally: the 3.10/3.11/3.13 matrix, build-check, codegen drift (no generated or spec file touched)
Deviations:
- The bucket-header case was added after review: a bodiless 429 that names a bucket is no longer relabeled rate_limited

Residual

The SDK class is a RouterError subclass. A rate_limited from the v2 gateway now arrives as comfy_sdk.router_exceptions.RateLimited, so except RouterError also catches a non-Router refusal. This is the existing translation for every bodied rate_limited, not something this PR introduces. Whether v2-surface rate_limited should become a plain ComfyError belongs with the separate audit of the status/code fallback table.
No live gateway check. The bodiless HEAD 429/403 was reproduced against the in-repo stub server only. A real gateway refusal was not triggered, because that means deliberately exhausting an account's rate limit.
Retry-After in HTTP-date form is still read as None. That comes from the shared _retry_after parser, which this PR reuses and does not change.

_{🔄 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/208 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 10/9/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `matt/be-19949-head-429-rate-limited` --- ### 📝 Commits (2) - [`e659071`](https://github.com/Comfy-Org/comfy-python-sdk/commit/e65907141679ffa9f84b4fd16e16bcb4b9cc3b49) fix(assets): read a bodiless HEAD 429 on the by-hash probe as rate_limited, not queue_full - [`a1bc9a5`](https://github.com/Comfy-Org/comfy-python-sdk/commit/a1bc9a59affb83498503c5b6f58e1cbd1a440f33) fix(assets): keep a named bucket on a bodiless HEAD 429 and pin the SDK class ### 📊 Changes **4 files changed** (+187 additions, -3 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+9 -0) 📝 `src/comfy_low/transport.py` (+34 -3) 📝 `tests/conftest.py` (+10 -0) ➕ `tests/test_head_refusal.py` (+134 -0) </details> ### 📄 Description ## ELI-5 Before uploading a file, the SDK asks the server "do you already have these bytes?" with a `HEAD` request. When the gateway refuses that request because the account is rate-limited, it sends a `429` with no body (a `HEAD` response can't have one). With no body to read, the SDK guessed from the status code alone and reported a full job queue (`QueueFull`). That's the wrong error, and it sends callers off to debug a queue that has nothing to do with the problem. Now a bodiless `429` on that probe is reported as `rate_limited`, and the `Retry-After` wait is still included. ## What changed - `comfy_low/transport.py`: new `_Prepared.raise_for_head_refusal(resp)`. On a `429` it builds the same error `error_from_envelope` would build for `{"error": {"code": "rate_limited", "message": "Rate limited"}}`, with `retry_after` from the existing `_retry_after` header parser and `request_id` from the header. A `429` that names its own bucket on `X-Comfy-Error-Type` (for example `concurrency_limit_exceeded`) takes the existing generic path instead, so it keeps that bucket. Every other status also goes through the generic path, so a bodiless `403` is still `Forbidden`. `ComfyLow.head_asset_by_hash` and `AsyncComfyLow.head_asset_by_hash` call it after their 200/404 checks, and the docstring now says the code is synthesized from the status. - `tests/conftest.py`: new `ServerState.head_refusal: (status, headers) | None` setting. When it is set, the stub answers `HEAD /assets/by-hash` with that status and those headers and an empty body. - `tests/test_head_refusal.py`: tests for the low layer, sync and async: - a bodiless `429` with `Retry-After: 7` gives `code == "rate_limited"`, `http_status == 429`, `retry_after == 7`, and the error is not `QueueFull` - a bodiless `429` without the header gives `retry_after is None` - a bodiless `429` with `X-Comfy-Error-Type: concurrency_limit_exceeded` keeps that code and `error_type` - a bodiless `403` gives `Forbidden` - SDK tests: `commit()` (sync and async) against the `429` stub raises exactly `RateLimited` (`code == "rate_limited"`, `retry_after == 7`), not `QueueFull`, and uploads nothing. `commit()` against the bucket-header `429` raises `ConcurrencyLimitExceeded`, and against the `403` stub raises `Forbidden`. - `CHANGELOG.md`: added an entry under Unreleased → Fixed. ### Judgment calls - **The SDK class a caller sees is the existing `rate_limited` mapping.** `comfy_sdk.exceptions._class_for` already maps the code `rate_limited` through the Router bucket table, so `commit()` raises `comfy_sdk.router_exceptions.RateLimited`. That class is a `ComfyError` subclass with `code == "rate_limited"` and carries `retry_after`. A bodied `rate_limited` on any v2 route already surfaces this way, so this change matches that behavior instead of adding a class. No new exception class is exported and `exportedErrorClasses` is unchanged. - **The `403` keeps the generic message `HTTP 403`.** Changing it to `Forbidden` was optional, and I left it alone so this PR only changes the `429` case. - `_CODE_BY_STATUS` and the client's 429 retry helper (`_retry_delay`) are unchanged, so the bodiless `queue_full` fallback on `POST /jobs` still works. ## Provenance - **Authored by:** agent-work loop - **Verified:** - Tests: all green on the current head (1266 passed, 9 skipped; the skips include the env-gated gateway e2e suite), on Python 3.12 locally - Lint/format/type check: `ruff check`, `ruff format --check` and `mypy src` all clean - Public-repo hygiene script: clean - Mutation checks: with the `429` branch disabled, the rate-limit tests fail; with the bucket-header guard removed, the 3 bucket tests fail - Not run locally: the 3.10/3.11/3.13 matrix, `build-check`, codegen drift (no generated or spec file touched) - **Deviations:** - The bucket-header case was added after review: a bodiless `429` that names a bucket is no longer relabeled `rate_limited` ## Residual - **The SDK class is a `RouterError` subclass.** A `rate_limited` from the v2 gateway now arrives as `comfy_sdk.router_exceptions.RateLimited`, so `except RouterError` also catches a non-Router refusal. This is the existing translation for every bodied `rate_limited`, not something this PR introduces. Whether v2-surface `rate_limited` should become a plain `ComfyError` belongs with the separate audit of the status/code fallback table. - **No live gateway check.** The bodiless `HEAD` 429/403 was reproduced against the in-repo stub server only. A real gateway refusal was not triggered, because that means deliberately exhausting an account's rate limit. - **`Retry-After` in HTTP-date form is still read as `None`.** That comes from the shared `_retry_after` parser, which this PR reuses and does not change. --- <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

bsakaguchi referenced this issue

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

[PR #210] docs(errors): list rate_limited explicitly as the fourth code both surfaces share #211

bsakaguchi referenced this issue

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

[PR #213] feat(errors): export RateLimited as a fourth shared bucket and map v2 rate_limited to it #213