[PR #203] fix(jobs): floor job.events()'s 429 reconnect pause at one second #202

New issue

Open

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-19930-floor-events-429-pause

📝 Commits (1)

26f9f96 fix(jobs): floor job.events()'s 429 reconnect pause at one second

📊 Changes

5 files changed (+53 additions, -8 deletions)

View changed files

📝 CHANGELOG.md (+4 -0)
📝 src/sdk/client.ts (+1 -3)
📝 src/sdk/jobs.test.ts (+24 -0)
📝 src/sdk/jobs.ts (+14 -5)
➕ src/sdk/retry-pause.ts (+10 -0)

📄 Description

Summary

Job.events() turned a 429 carrying Retry-After: 0 into a reconnect pause of 0 * 1000 ms. Only the 60 s ceiling (MAX_RECONNECT_PAUSE_MS) bounded that value, so the loop ran SSE connect (429) → refresh() → abortableSleep(0) → repeat. It had no retry budget and kept going as long as the job was non-terminal and the caller's signal stayed un-aborted. submit() and listJobs() already floor the same value at MIN_RETRY_PAUSE_MS = 1_000 through retryThrottled. This PR applies that floor to events() too.

src/sdk/retry-pause.ts (new): a leaf module exporting MIN_RETRY_PAUSE_MS. client.ts imports jobs.ts, so jobs.ts can't import the constant from client.ts. client.ts now imports it from here, with no change in behaviour.
src/sdk/jobs.ts: the 429 pause is now min(max(retryAfterMs, MIN_RETRY_PAUSE_MS), MAX_RECONNECT_PAUSE_MS), and the constant comment says the pause is bounded on both sides. The non-429 mid-stream reconnect pause (RECONNECT_PAUSE_MS = 100) is untouched. The omitted-header and invalid-header fallback (2 s) is already above the floor, so it doesn't change.
src/sdk/jobs.test.ts: it.each(["0", "1"]) asserts the pause is 1_000. It also checks request amplification: exactly one SSE connect and one extra poll inside a 300 ms window. "1" is the boundary case.
CHANGELOG.md: added an entry under Unreleased → Fixed.

Comfy Cloud sends Retry-After: 5 today, so this is defence in depth for COMFY_BASE_URL deployments and proxies. It follows the same "misbehaving server" reasoning as the existing ceiling.

The Python SDK needs no change. Its Job.events() doesn't retry a 429 at all, so it has no equivalent loop to floor. No public surface or spec change, so the parity and spec-coverage tests are unaffected.

Self-review notes

Riskiest line: the new Math.min(Math.max(...)). The floor (1 s) is below the ceiling (60 s), so the clamp order can't invert. The null → 2 s default path is unchanged.
parseRetryAfter uses parseInt, so a sub-second value like 0.5 reaches this code as 0 and is covered by the "0" case.
Hygiene check: AGENTS.md section 4 says node scripts/check-public-repo-hygiene.mjs runs the public-repo hygiene check. That script is no longer in the repo; CI now calls a shared reusable workflow. Instead I grepped the diff by hand for the ticket-ID pattern and for internal links, and found nothing. The stale AGENTS.md line is out of scope for this PR.

Residual

AGENTS.md (sections 3 and 4) still tells contributors to run node scripts/check-public-repo-hygiene.mjs locally. That script was replaced by the shared reusable workflow .github/workflows/public-repo-hygiene.yml, so the documented local command fails with MODULE_NOT_FOUND. A docs follow-up should point it at the new mechanism. This PR doesn't fix that.

Provenance

Authored by: agent-work loop
Verified:
- Tests: all green, 787 passed / 5 skipped (pnpm test)
- Lint, format and typecheck: clean (pnpm lint, pnpm format:check, pnpm typecheck)
- Spec drift: in sync (pnpm check:spec-drift)
- Mutation-tested: with the floor reverted, the new Retry-After: 0 test fails and the "1" boundary case passes, as expected
- Confirmed listJobs() gets the existing floor through retryThrottled, as the changelog wording says
Deviations: none. The optional "1" boundary case was included.

🤖 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/203 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 10/9/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `matt/be-19930-floor-events-429-pause` --- ### 📝 Commits (1) - [`26f9f96`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/26f9f96a4ecedf18d57eace8f0c15fb7a61fbbf8) fix(jobs): floor job.events()'s 429 reconnect pause at one second ### 📊 Changes **5 files changed** (+53 additions, -8 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+4 -0) 📝 `src/sdk/client.ts` (+1 -3) 📝 `src/sdk/jobs.test.ts` (+24 -0) 📝 `src/sdk/jobs.ts` (+14 -5) ➕ `src/sdk/retry-pause.ts` (+10 -0) </details> ### 📄 Description ## Summary `Job.events()` turned a `429` carrying `Retry-After: 0` into a reconnect pause of `0 * 1000` ms. Only the 60 s ceiling (`MAX_RECONNECT_PAUSE_MS`) bounded that value, so the loop ran SSE connect (429) → `refresh()` → `abortableSleep(0)` → repeat. It had no retry budget and kept going as long as the job was non-terminal and the caller's signal stayed un-aborted. `submit()` and `listJobs()` already floor the same value at `MIN_RETRY_PAUSE_MS = 1_000` through `retryThrottled`. This PR applies that floor to `events()` too. - **`src/sdk/retry-pause.ts` (new):** a leaf module exporting `MIN_RETRY_PAUSE_MS`. `client.ts` imports `jobs.ts`, so `jobs.ts` can't import the constant from `client.ts`. `client.ts` now imports it from here, with no change in behaviour. - **`src/sdk/jobs.ts`:** the 429 pause is now `min(max(retryAfterMs, MIN_RETRY_PAUSE_MS), MAX_RECONNECT_PAUSE_MS)`, and the constant comment says the pause is bounded on both sides. The non-429 mid-stream reconnect pause (`RECONNECT_PAUSE_MS = 100`) is untouched. The omitted-header and invalid-header fallback (2 s) is already above the floor, so it doesn't change. - **`src/sdk/jobs.test.ts`:** `it.each(["0", "1"])` asserts the pause is `1_000`. It also checks request amplification: exactly one SSE connect and one extra poll inside a 300 ms window. `"1"` is the boundary case. - **`CHANGELOG.md`:** added an entry under Unreleased → Fixed. Comfy Cloud sends `Retry-After: 5` today, so this is defence in depth for `COMFY_BASE_URL` deployments and proxies. It follows the same "misbehaving server" reasoning as the existing ceiling. The Python SDK needs no change. Its `Job.events()` doesn't retry a 429 at all, so it has no equivalent loop to floor. No public surface or spec change, so the parity and spec-coverage tests are unaffected. ## Self-review notes - **Riskiest line:** the new `Math.min(Math.max(...))`. The floor (1 s) is below the ceiling (60 s), so the clamp order can't invert. The `null` → 2 s default path is unchanged. - `parseRetryAfter` uses `parseInt`, so a sub-second value like `0.5` reaches this code as `0` and is covered by the `"0"` case. - **Hygiene check:** AGENTS.md section 4 says `node scripts/check-public-repo-hygiene.mjs` runs the public-repo hygiene check. That script is no longer in the repo; CI now calls a shared reusable workflow. Instead I grepped the diff by hand for the ticket-ID pattern and for internal links, and found nothing. The stale AGENTS.md line is out of scope for this PR. ## Residual - AGENTS.md (sections 3 and 4) still tells contributors to run `node scripts/check-public-repo-hygiene.mjs` locally. That script was replaced by the shared reusable workflow `.github/workflows/public-repo-hygiene.yml`, so the documented local command fails with `MODULE_NOT_FOUND`. A docs follow-up should point it at the new mechanism. This PR doesn't fix that. ## Provenance - **Authored by:** agent-work loop - **Verified:** - Tests: all green, 787 passed / 5 skipped (`pnpm test`) - Lint, format and typecheck: clean (`pnpm lint`, `pnpm format:check`, `pnpm typecheck`) - Spec drift: in sync (`pnpm check:spec-drift`) - Mutation-tested: with the floor reverted, the new `Retry-After: 0` test fails and the `"1"` boundary case passes, as expected - Confirmed `listJobs()` gets the existing floor through `retryThrottled`, as the changelog wording says - **Deviations:** none. The optional `"1"` boundary case was included. 🤖 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:38 +00:00

bsakaguchi referenced this issue

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

[PR #202] fix: land the cloud@b8907cf v2 spec sync (format, regenerate, listJobs) #205