[PR #29] [MERGED] refactor(transport): unify the two fixed-schedule transient retriers #33

New issue

Closed

opened 2026-10-26 22:34:10 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:34:10 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-swift-sdk/pull/29
Author: @mattmillerai
Created: 7/4/2026
Status: ✅ Merged
Merged: 7/5/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-2353-consolidate-transient-retriers

📝 Commits (3)

51bdb3b refactor(transport): unify the two fixed-schedule transient retriers
c54f85a fix(transport): propagate cancellation during output-retrier delay
bd8e783 Merge remote-tracking branch 'origin/main' into matt/be-2353-consolidate-transient-retriers

📊 Changes

2 files changed (+44 additions, -15 deletions)

View changed files

📝 Sources/ComfySwiftSDK/Internal/PollingFallback.swift (+34 -4)
📝 Sources/ComfySwiftSDK/Internal/ReattachCoordinator.swift (+10 -11)

📄 Description

ELI-5

We had two little "try again if the network hiccups" loops that looked almost the same: one for downloading a job's output files, one for the reattach catch-up status fetch. Both try 3 times and give up. They only really differed in how long they waited between tries. This PR replaces the two copy-pasted loops with a single shared helper and just hands each caller its own wait-schedule, so the exact timing each one had before is unchanged.

What changed

Added one shared retrier PollingFallback.withTransientRetry(delays:sleep:body:):
retries transient ComfyErrors, attempt budget = delays.count + 1, non-transient
errors propagate immediately, last transient error rethrown once exhausted.
Both the schedule and the sleep primitive are injected, so each call site keeps
its exact prior timing:
- Output download (assembleOutput → withTransientRetry(body:)): [250ms, .zero],
  sleeping on Task (real clock). The convenience withTransientRetry(body:) overload is
  kept so the existing call sites and tests are untouched; outputFetchMaxAttempts now
  derives from the schedule.
- Reattach catch-up (ReattachCoordinator.fetchJobStatusWithTransientRetry):
  [250ms, 750ms], sleeping on the test-injectable Clock.
A non-positive delay (the .zero pad) advances the attempt budget without a sleep,
so the output retrier's original "no pause before the 3rd attempt" is preserved exactly —
no phantom zero-length suspension.

Explicitly out of scope

The poll loop's steady-state backoff ladder (backoffLadder 2/4/8s) is not merged into
this helper. It is a polling-cadence concern, not bounded error recovery, and consolidating
it would conflate two different behaviors. Left untouched.

Judgment calls

Pure refactor: no behavior/timing change intended. Verified by the existing
Output fetch transient retry, Reattach transient-retry, and ReattachCoordinator
suites, all green (full suite: 150 tests / 20 suites pass).
outputFetchMaxAttempts changed from a stored let 3 to a computed property derived from
the schedule length, to keep a single source of truth. Value is still 3; it stays
internal and the test that reads it is unaffected.

Testing

swift build clean; swift test --no-parallel → 150 tests in 20 suites passed.

_{🔄 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-swift-sdk/pull/29 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 7/4/2026 **Status:** ✅ Merged **Merged:** 7/5/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-2353-consolidate-transient-retriers` --- ### 📝 Commits (3) - [`51bdb3b`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/51bdb3ba8473f4a2ca53f535e8fa2448e4e33eb2) refactor(transport): unify the two fixed-schedule transient retriers - [`c54f85a`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/c54f85a0f5eaa445cfd568d96d2d240d3d124683) fix(transport): propagate cancellation during output-retrier delay - [`bd8e783`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/bd8e783bab52fa16c8e6f748e0301c2fa7688608) Merge remote-tracking branch 'origin/main' into matt/be-2353-consolidate-transient-retriers ### 📊 Changes **2 files changed** (+44 additions, -15 deletions) <details> <summary>View changed files</summary> 📝 `Sources/ComfySwiftSDK/Internal/PollingFallback.swift` (+34 -4) 📝 `Sources/ComfySwiftSDK/Internal/ReattachCoordinator.swift` (+10 -11) </details> ### 📄 Description ## ELI-5 We had two little \"try again if the network hiccups\" loops that looked almost the same: one for downloading a job's output files, one for the reattach catch-up status fetch. Both try 3 times and give up. They only really differed in how long they waited between tries. This PR replaces the two copy-pasted loops with a single shared helper and just hands each caller its own wait-schedule, so the exact timing each one had before is unchanged. ## What changed - Added one shared retrier `PollingFallback.withTransientRetry(delays:sleep:body:)`: retries transient `ComfyError`s, attempt budget = `delays.count + 1`, non-transient errors propagate immediately, last transient error rethrown once exhausted. - Both the **schedule** and the **sleep primitive** are injected, so each call site keeps its exact prior timing: - **Output download** (`assembleOutput` → `withTransientRetry(body:)`): `[250ms, .zero]`, sleeping on `Task` (real clock). The convenience `withTransientRetry(body:)` overload is kept so the existing call sites and tests are untouched; `outputFetchMaxAttempts` now derives from the schedule. - **Reattach catch-up** (`ReattachCoordinator.fetchJobStatusWithTransientRetry`): `[250ms, 750ms]`, sleeping on the test-injectable `Clock`. - A non-positive delay (the `.zero` pad) advances the attempt budget **without** a sleep, so the output retrier's original "no pause before the 3rd attempt" is preserved exactly — no phantom zero-length suspension. ## Explicitly out of scope The poll loop's steady-state backoff ladder (`backoffLadder` 2/4/8s) is **not** merged into this helper. It is a polling-cadence concern, not bounded error recovery, and consolidating it would conflate two different behaviors. Left untouched. ## Judgment calls - Pure refactor: no behavior/timing change intended. Verified by the existing `Output fetch transient retry`, `Reattach transient-retry`, and `ReattachCoordinator` suites, all green (full suite: 150 tests / 20 suites pass). - `outputFetchMaxAttempts` changed from a stored `let 3` to a computed property derived from the schedule length, to keep a single source of truth. Value is still `3`; it stays `internal` and the test that reads it is unaffected. ## Testing `swift build` clean; `swift test --no-parallel` → **150 tests in 20 suites passed**. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:34:10 +00:00