[PR #207] fix(assets): map a bodiless HEAD 429 on the dedup probe to rate_limited #208

New issue

Open

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

bsakaguchi commented

2026-10-31 22:05:42 +00:00

Owner

📋 Pull Request Information

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

Base: main ← Head: be-19948-head-429-rate-limited

📝 Commits (2)

02a2f4e fix(assets): map a bodiless HEAD 429 on the dedup probe to rate_limited
cd9bbc8 fix(assets): give a bodiless HEAD 401 on the dedup probe its Unauthorized message

📊 Changes

5 files changed (+104 additions, -2 deletions)

View changed files

📝 CHANGELOG.md (+10 -0)
📝 src/low/transport.test.ts (+33 -0)
📝 src/low/transport.ts (+27 -1)
📝 src/sdk/assets.test.ts (+14 -1)
📝 test/support/stub-server.ts (+20 -0)

📄 Description

ELI-5

Before uploading a file, the SDK asks the server "do you already have these bytes?" using a HEAD request. When the gateway rate-limits that question, it replies with a bare 429 and no body (HEAD responses never have one). The SDK then guessed what the 429 meant and picked "the job queue is full" (QueueFull), which is wrong. Now it reports a rate limit (code: "rate_limited") and keeps the server's Retry-After.

What changed

src/low/transport.ts — headAssetByHash: after the 200/404 checks, a bodiless 401, 403 or 429 is looked up in a small status table (HEAD_REFUSALS) and thrown through errorFromEnvelope with a synthesized envelope plus retryAfter from Retry-After. A 429 becomes { code: "rate_limited", message: "Rate limited" }. Because rate_limited has no BY_CODE entry, this is a plain ApiError, which the SDK layer turns into a generic ComfyError with code: "rate_limited". That matches how a 429 rate_limited with a body already surfaces. A bodiless 401 / 403 is still Unauthorized / Forbidden, but its message is now Unauthorized / Forbidden instead of HTTP 401 / HTTP 403. The 401 was added after review: it is the refusal the spec actually declares for this operation. The doc comment explains that the code is built from the status because the refusal has no body.
test/support/stub-server.ts: new headStatus: 401 | 403 | 429 | null and omitHeadRetryAfter settings. With them, the HEAD route answers with an empty body, and the 429 reuses retryAfterHeader.
Tests: four transport cases (429 with Retry-After: 7, 429 without it, 401, 403) and one assets.fromBytes(...).commit() case. That case checks the result is a ComfyError with rate_limited, is not QueueFull, has retryAfter 7, and that no upload happened.
CHANGELOG.md: added under Unreleased / Fixed.

Not changed, as required: CODE_BY_STATUS, retryThrottled, and the exported error classes. No RateLimited class was added, so the surface-parity test is unchanged.

Judgment calls

I also did the ticket's optional 403 change (code forbidden, message Forbidden), and after review the same for 401 (unauthorized, Unauthorized). The classes and codes are the same as before; only the message text changed.
The 401/403 paths also pass retryAfter through. It is null unless the server sends the header, which matches what parseOrRaise already did.

Provenance

Authored by: agent-work loop
Verified:
- Tests: all green — 790 passed, 5 skipped (pnpm test), including the 5 new cases
- Lint / format / typecheck / spec-drift / build: clean
- Mutation-tested on the first commit: with the transport.ts change reverted, the new 429/403 tests fail
- Surface-parity test unchanged and passing (no new exported class)
- Public-repo hygiene: the in-tree script is gone from main, so added lines were checked by hand for ticket-shaped IDs and internal links
- Not run locally: pnpm check:sdk-parity (network), the Node 22/24 CI matrix
Deviations: none

Residual

Not tested against a live gateway. The bodiless 429/403 on HEAD /api/v2/assets/by-hash/{hash} is reproduced only by the stub server. I did not send traffic to the real platform gateway to trigger its rate limit or SSO refusal, because that is not a safe read-only check.
A bodiless 403 sso_required on this probe is indistinguishable from a generic 403. With no body, the SDK reports forbidden. Telling sso_required apart would need something other than the body, such as a response header. Bodied sso_required handling is tracked separately. This PR does not change it.
The Python SDK likely has the same bug. If its existence probe also falls back from status to queue_full on a bodiless 429, it needs the same change. I did not check the Python SDK here.
Spec declarations. The vendored spec/openapi.yaml does not declare 403/429 on this operation. A separate upstream spec change adds them. This fix does not depend on it, and I did not edit the vendored spec.
Review follow-ups proposed, not done here: retrying a throttled dedup probe on its Retry-After (today it aborts commit()/submit(), as before this PR), and a typed error class for v2 rate_limited (its obvious name collides with the router RateLimited export).
Upstream investigation notes not read. The ticket's evidence comes from an earlier investigation whose findings were not available to this run. I worked only from the description reproduced in the ticket.

🤖 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-typescript-sdk/pull/207 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 10/9/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `be-19948-head-429-rate-limited` --- ### 📝 Commits (2) - [`02a2f4e`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/02a2f4e9d68a9c2c44700b584875ba6f0dfd560d) fix(assets): map a bodiless HEAD 429 on the dedup probe to rate_limited - [`cd9bbc8`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/cd9bbc83f674c0773e96427573ff20307cb5e020) fix(assets): give a bodiless HEAD 401 on the dedup probe its Unauthorized message ### 📊 Changes **5 files changed** (+104 additions, -2 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+10 -0) 📝 `src/low/transport.test.ts` (+33 -0) 📝 `src/low/transport.ts` (+27 -1) 📝 `src/sdk/assets.test.ts` (+14 -1) 📝 `test/support/stub-server.ts` (+20 -0) </details> ### 📄 Description ## ELI-5 Before uploading a file, the SDK asks the server "do you already have these bytes?" using a `HEAD` request. When the gateway rate-limits that question, it replies with a bare `429` and no body (`HEAD` responses never have one). The SDK then guessed what the `429` meant and picked "the job queue is full" (`QueueFull`), which is wrong. Now it reports a rate limit (`code: "rate_limited"`) and keeps the server's `Retry-After`. ## What changed - `src/low/transport.ts` — `headAssetByHash`: after the 200/404 checks, a bodiless `401`, `403` or `429` is looked up in a small status table (`HEAD_REFUSALS`) and thrown through `errorFromEnvelope` with a synthesized envelope plus `retryAfter` from `Retry-After`. A `429` becomes `{ code: "rate_limited", message: "Rate limited" }`. Because `rate_limited` has no `BY_CODE` entry, this is a plain `ApiError`, which the SDK layer turns into a generic `ComfyError` with `code: "rate_limited"`. That matches how a `429 rate_limited` with a body already surfaces. A bodiless `401` / `403` is still `Unauthorized` / `Forbidden`, but its message is now `Unauthorized` / `Forbidden` instead of `HTTP 401` / `HTTP 403`. The `401` was added after review: it is the refusal the spec actually declares for this operation. The doc comment explains that the code is built from the status because the refusal has no body. - `test/support/stub-server.ts`: new `headStatus: 401 | 403 | 429 | null` and `omitHeadRetryAfter` settings. With them, the HEAD route answers with an empty body, and the 429 reuses `retryAfterHeader`. - Tests: four transport cases (429 with `Retry-After: 7`, 429 without it, 401, 403) and one `assets.fromBytes(...).commit()` case. That case checks the result is a `ComfyError` with `rate_limited`, is not `QueueFull`, has `retryAfter` 7, and that no upload happened. - `CHANGELOG.md`: added under Unreleased / Fixed. Not changed, as required: `CODE_BY_STATUS`, `retryThrottled`, and the exported error classes. No `RateLimited` class was added, so the surface-parity test is unchanged. ## Judgment calls - I also did the ticket's optional 403 change (code `forbidden`, message `Forbidden`), and after review the same for 401 (`unauthorized`, `Unauthorized`). The classes and codes are the same as before; only the message text changed. - The 401/403 paths also pass `retryAfter` through. It is `null` unless the server sends the header, which matches what `parseOrRaise` already did. ## Provenance - **Authored by:** agent-work loop - **Verified:** - Tests: all green — 790 passed, 5 skipped (`pnpm test`), including the 5 new cases - Lint / format / typecheck / spec-drift / build: clean - Mutation-tested on the first commit: with the `transport.ts` change reverted, the new 429/403 tests fail - Surface-parity test unchanged and passing (no new exported class) - Public-repo hygiene: the in-tree script is gone from main, so added lines were checked by hand for ticket-shaped IDs and internal links - Not run locally: `pnpm check:sdk-parity` (network), the Node 22/24 CI matrix - **Deviations:** none ## Residual - **Not tested against a live gateway.** The bodiless `429`/`403` on `HEAD /api/v2/assets/by-hash/{hash}` is reproduced only by the stub server. I did not send traffic to the real platform gateway to trigger its rate limit or SSO refusal, because that is not a safe read-only check. - **A bodiless `403 sso_required` on this probe is indistinguishable from a generic `403`.** With no body, the SDK reports `forbidden`. Telling `sso_required` apart would need something other than the body, such as a response header. Bodied `sso_required` handling is tracked separately. This PR does not change it. - **The Python SDK likely has the same bug.** If its existence probe also falls back from status to `queue_full` on a bodiless `429`, it needs the same change. I did not check the Python SDK here. - **Spec declarations.** The vendored `spec/openapi.yaml` does not declare `403`/`429` on this operation. A separate upstream spec change adds them. This fix does not depend on it, and I did not edit the vendored spec. - **Review follow-ups proposed, not done here:** retrying a throttled dedup probe on its `Retry-After` (today it aborts `commit()`/`submit()`, as before this PR), and a typed error class for v2 `rate_limited` (its obvious name collides with the router `RateLimited` export). - **Upstream investigation notes not read.** The ticket's evidence comes from an earlier investigation whose findings were not available to this run. I worked only from the description reproduced in the ticket. 🤖 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:05:42 +00:00