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

New issue

Open

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: mattmiller/be-19965-export-ratelimited

📝 Commits (4)

602b405 feat(errors): export RateLimited as a fourth shared bucket and map v2 rate_limited to it
5ce9ad2 docs(errors): note RateLimited.retry_after can be None; count rate_limited among shared codes
da7a435 chore: merge main into mattmiller/be-19965-export-ratelimited
cff9382 Merge remote-tracking branch 'origin/main' into mattmiller/be-19965-export-ratelimited

📊 Changes

10 files changed (+100 additions, -30 deletions)

View changed files

📝 CHANGELOG.md (+5 -0)
📝 README.md (+12 -4)
📝 src/comfy_low/__init__.py (+2 -0)
📝 src/comfy_low/errors.py (+12 -4)
📝 src/comfy_sdk/__init__.py (+7 -2)
📝 src/comfy_sdk/exceptions.py (+9 -5)
📝 src/comfy_sdk/router_exceptions.py (+11 -8)
📝 tests/test_error_mapping.py (+33 -3)
📝 tests/test_exception_modules.py (+8 -2)
📝 tests/test_job_metadata.py (+1 -2)

📄 Description

ELI5

When the server says "slow down, you have sent too many requests for now", the SDK already raised a dedicated rate-limit error, but you could only import it from the router-specific module. Now you can import it straight from the SDK's top-level package, the same way the other shared errors already work. It is the same error either way, so code that catches one import also catches the other.

What changed

comfy_sdk.exceptions: RateLimited (re-exported from router_exceptions, the same object) is listed explicitly in _BY_CODE as "rate_limited", beside insufficient_credits. It is also added to __all__. The comments and docstrings that counted "three" shared buckets now say four.
comfy_sdk root: imports and exports RateLimited. The __all__ comment now says that the four shared buckets are lifted and the Router-only buckets stay in router_exceptions.
comfy_sdk.router_exceptions: only the module docstring changed, to count four shared names and to describe what the package root exports. The class itself is unchanged.
comfy_low.errors: adds class RateLimited(ApiError): code = "rate_limited", with a docstring that contrasts it with queue_full, and adds it to _BY_CODE. _CODE_BY_STATUS[429] = "queue_full" is unchanged, so a 429 with no code still decodes as QueueFull.
Tests:
- test_exception_modules.py: the shared-name and descends-from-both checks now include RateLimited.
- test_error_mapping.py: rate_limited moved from the Router-only parametrize list to test_a_bucket_both_surfaces_spell_keeps_its_v2_class. Two new tests:
  - SDK: to_sdk_error(ApiError("slow down", code="rate_limited", http_status=429, retry_after=3)) returns an instance of comfy_sdk.RateLimited, which is the same object as exceptions.RateLimited and router_exceptions.RateLimited. retry_after is 3.
  - Low layer: error_from_envelope(429, {"error": {"code": "rate_limited", ...}}, retry_after=3) returns a comfy_low.errors.RateLimited, and a 429 with no code still returns QueueFull.
- test_job_metadata.py now imports RateLimited from comfy_sdk, which proves the root export through the existing pytest.raises cases.
Docs: the README typed-errors list gains a RateLimited entry. It says .retry_after is None when the response sent no delta-seconds Retry-After, because unlike QueueFull's it is not defaulted to 0. The comfy_low/errors.py comments that counted three identically spelled codes now count four. The Router section's shared-names paragraph now counts four. CHANGELOG has a new entry under Unreleased → Added.

Judgment calls

comfy_low root export. I also exported RateLimited from the comfy_low package root. Every other class in comfy_low.errors is exported there, and the TypeScript low layer is meant to mirror this one.
Low-layer behaviour change. A Router rate_limited response, or a v2 one, now becomes comfy_low.errors.RateLimited at the low layer instead of a bare ApiError. It is still an ApiError subclass, and I found nothing in src/ that checks type(exc) is ApiError or relies on that code being untyped.
No SDK-level behaviour change. _class_for already resolved rate_limited to this class through the _BY_ERROR_TYPE fallback. Listing it in _BY_CODE documents the mapping rather than changing it, so removing the _BY_CODE entry would not fail the new SDK test. The test pins the outcome, not which lookup produced it.

Provenance

Authored by: agent-work loop
Verified: (on head cff9382, after merging main at 3fa8ac1)
- Tests are green: pytest -q gives 1277 passed and 9 skipped. The skips include the env-gated gateway e2e.
- ruff check ., ruff format --check . and mypy src are clean.
- scripts/check_drift.py is OK: models are in sync, and all 20 Router error types are covered and blessed.
- scripts/check_public_repo_hygiene.py is OK.
- Not run locally: the 3.10–3.13 matrix (only one interpreter was used) and build-check. CI runs both on push.
Deviations: none from the plan, apart from the extra comfy_low root export described above. The CHANGELOG entry sits under Unreleased → Added, next to the ResultExpired entry that main added while this PR was open.

Residual

Overlap with open PRs. The open PR that maps sso_required → Forbidden (#205) also edits src/comfy_sdk/exceptions.py and src/comfy_low/errors.py. Whichever lands second needs a rebase, which should be mechanical: different _BY_CODE entries and adjacent comment lines. The HEAD-429 rate_limited PR (#208) edits only comfy_low/transport.py. Its bodiless HEAD probe will produce the new comfy_low.errors.RateLimited / comfy_sdk.RateLimited once both are merged, but I did not exercise that combination here.
TypeScript twin. The TypeScript SDK's root RateLimited export, and its surface-parity.test.ts check, are a separate follow-up that this PR unblocks. That repo was not touched or run here.

Summary by CodeRabbit

New Features
- Added a shared RateLimited exception for rate-limit responses, available from the SDK and low-level package.
- Rate-limit errors include their retry delay and HTTP status. HTTP 429 responses without a rate-limit code continue to map to QueueFull.
Documentation
- Clarified how RateLimited relates to other workflow and Router errors, and documented its retry_after attribute.

_{🔄 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/213 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 10/9/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `mattmiller/be-19965-export-ratelimited` --- ### 📝 Commits (4) - [`602b405`](https://github.com/Comfy-Org/comfy-python-sdk/commit/602b4056ae68ee579774fa99646f56abbbefcbdd) feat(errors): export RateLimited as a fourth shared bucket and map v2 rate_limited to it - [`5ce9ad2`](https://github.com/Comfy-Org/comfy-python-sdk/commit/5ce9ad2d73299b5a4995d297c34295ba1d6f03ee) docs(errors): note RateLimited.retry_after can be None; count rate_limited among shared codes - [`da7a435`](https://github.com/Comfy-Org/comfy-python-sdk/commit/da7a43580b7cb8af43421b39459e2c68bf5e9c79) chore: merge main into mattmiller/be-19965-export-ratelimited - [`cff9382`](https://github.com/Comfy-Org/comfy-python-sdk/commit/cff938207441068d319b4b48bdb8a771205dbfe5) Merge remote-tracking branch 'origin/main' into mattmiller/be-19965-export-ratelimited ### 📊 Changes **10 files changed** (+100 additions, -30 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+5 -0) 📝 `README.md` (+12 -4) 📝 `src/comfy_low/__init__.py` (+2 -0) 📝 `src/comfy_low/errors.py` (+12 -4) 📝 `src/comfy_sdk/__init__.py` (+7 -2) 📝 `src/comfy_sdk/exceptions.py` (+9 -5) 📝 `src/comfy_sdk/router_exceptions.py` (+11 -8) 📝 `tests/test_error_mapping.py` (+33 -3) 📝 `tests/test_exception_modules.py` (+8 -2) 📝 `tests/test_job_metadata.py` (+1 -2) </details> ### 📄 Description ## ELI5 When the server says "slow down, you have sent too many requests for now", the SDK already raised a dedicated rate-limit error, but you could only import it from the router-specific module. Now you can import it straight from the SDK's top-level package, the same way the other shared errors already work. It is the same error either way, so code that catches one import also catches the other. ## What changed - **`comfy_sdk.exceptions`**: `RateLimited` (re-exported from `router_exceptions`, the same object) is listed explicitly in `_BY_CODE` as `"rate_limited"`, beside `insufficient_credits`. It is also added to `__all__`. The comments and docstrings that counted "three" shared buckets now say four. - **`comfy_sdk` root**: imports and exports `RateLimited`. The `__all__` comment now says that the four shared buckets are lifted and the Router-only buckets stay in `router_exceptions`. - **`comfy_sdk.router_exceptions`**: only the module docstring changed, to count four shared names and to describe what the package root exports. The class itself is unchanged. - **`comfy_low.errors`**: adds `class RateLimited(ApiError): code = "rate_limited"`, with a docstring that contrasts it with `queue_full`, and adds it to `_BY_CODE`. `_CODE_BY_STATUS[429] = "queue_full"` is unchanged, so a 429 with no code still decodes as `QueueFull`. - **Tests**: - `test_exception_modules.py`: the shared-name and descends-from-both checks now include `RateLimited`. - `test_error_mapping.py`: `rate_limited` moved from the Router-only parametrize list to `test_a_bucket_both_surfaces_spell_keeps_its_v2_class`. Two new tests: - SDK: `to_sdk_error(ApiError("slow down", code="rate_limited", http_status=429, retry_after=3))` returns an instance of `comfy_sdk.RateLimited`, which is the same object as `exceptions.RateLimited` and `router_exceptions.RateLimited`. `retry_after` is 3. - Low layer: `error_from_envelope(429, {"error": {"code": "rate_limited", ...}}, retry_after=3)` returns a `comfy_low.errors.RateLimited`, and a 429 with no code still returns `QueueFull`. - `test_job_metadata.py` now imports `RateLimited` from `comfy_sdk`, which proves the root export through the existing `pytest.raises` cases. - **Docs**: the README typed-errors list gains a `RateLimited` entry. It says `.retry_after` is `None` when the response sent no delta-seconds `Retry-After`, because unlike `QueueFull`'s it is not defaulted to `0`. The `comfy_low/errors.py` comments that counted three identically spelled codes now count four. The Router section's shared-names paragraph now counts four. CHANGELOG has a new entry under Unreleased → Added. ### Judgment calls - **`comfy_low` root export.** I also exported `RateLimited` from the `comfy_low` package root. Every other class in `comfy_low.errors` is exported there, and the TypeScript low layer is meant to mirror this one. - **Low-layer behaviour change.** A Router `rate_limited` response, or a v2 one, now becomes `comfy_low.errors.RateLimited` at the low layer instead of a bare `ApiError`. It is still an `ApiError` subclass, and I found nothing in `src/` that checks `type(exc) is ApiError` or relies on that code being untyped. - **No SDK-level behaviour change.** `_class_for` already resolved `rate_limited` to this class through the `_BY_ERROR_TYPE` fallback. Listing it in `_BY_CODE` documents the mapping rather than changing it, so removing the `_BY_CODE` entry would not fail the new SDK test. The test pins the outcome, not which lookup produced it. ## Provenance - **Authored by:** agent-work loop - **Verified:** (on head `cff9382`, after merging `main` at `3fa8ac1`) - Tests are green: `pytest -q` gives 1277 passed and 9 skipped. The skips include the env-gated gateway e2e. - `ruff check .`, `ruff format --check .` and `mypy src` are clean. - `scripts/check_drift.py` is OK: models are in sync, and all 20 Router error types are covered and blessed. - `scripts/check_public_repo_hygiene.py` is OK. - Not run locally: the 3.10–3.13 matrix (only one interpreter was used) and `build-check`. CI runs both on push. - **Deviations:** none from the plan, apart from the extra `comfy_low` root export described above. The CHANGELOG entry sits under Unreleased → Added, next to the `ResultExpired` entry that `main` added while this PR was open. ## Residual - **Overlap with open PRs.** The open PR that maps `sso_required` → `Forbidden` (#205) also edits `src/comfy_sdk/exceptions.py` and `src/comfy_low/errors.py`. Whichever lands second needs a rebase, which should be mechanical: different `_BY_CODE` entries and adjacent comment lines. The HEAD-429 `rate_limited` PR (#208) edits only `comfy_low/transport.py`. Its bodiless HEAD probe will produce the new `comfy_low.errors.RateLimited` / `comfy_sdk.RateLimited` once both are merged, but I did not exercise that combination here. - **TypeScript twin.** The TypeScript SDK's root `RateLimited` export, and its `surface-parity.test.ts` check, are a separate follow-up that this PR unblocks. That repo was not touched or run here.  ## Summary by CodeRabbit * **New Features** * Added a shared `RateLimited` exception for rate-limit responses, available from the SDK and low-level package. * Rate-limit errors include their retry delay and HTTP status. HTTP 429 responses without a rate-limit code continue to map to `QueueFull`. * **Documentation** * Clarified how `RateLimited` relates to other workflow and Router errors, and documented its `retry_after` attribute.  --- <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:22 +00:00