[PR #3] [MERGED] fix(transport): retry transient errors on the execution_success output fetch #8

New issue

Closed

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

bsakaguchi commented

2026-10-26 22:33:50 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-swift-sdk/pull/3
Author: @mattmillerai
Created: 6/19/2026
Status: ✅ Merged
Merged: 6/19/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-1606-comfy-ios-backgrounded-fast-job-recovers-but-the-output

📝 Commits (3)

39c1eae fix(transport): treat POSIX socket-drop errors as transient so jobs recover via polling
d94aaeb fix(transport): retry transient errors on the execution_success output fetch
ea35614 merge: main into BE-1606 branch to drop redundant BE-1594 base (it squash-merged)

📊 Changes

3 files changed (+428 additions, -22 deletions)

View changed files

📝 Sources/ComfySwiftSDK/Internal/PollingFallback.swift (+59 -11)
📝 Sources/ComfySwiftSDK/Internal/WebSocketSession.swift (+25 -11)
➕ Tests/ComfySwiftSDKTests/OutputFetchTransientRetryTests.swift (+344 -0)

📄 Description

ELI5

The previous fix (merged as PR #2) stopped a backgrounded job from showing
an immediate false-failure on resume — but a job that finished while the
app was asleep could still fail ~20s later: fetching the output image over
the just-resumed connection hits a POSIX socket-drop (ECONNRESET), the
single try/catch gave up, and the user saw an error on a job that actually
succeeded. This PR wraps that output-fetch step in a bounded retry so the
result loads instead of erroring.

What

PollingFallback.swift: added static func withTransientRetry<T> — 3
attempts, 250 ms pause after the first failure, immediate propagation of
non-transient errors. Applied to every downloadView /
downloadViewToTempFile call inside buildOutput so the polling-recovered
path also survives a stale-connection fetch.
WebSocketSession.swift: the execution_success output-build catch
block now wraps each downloadView / downloadViewToTempFile call in
PollingFallback.withTransientRetry. Non-transient errors still reach
continuation.yield(.failed(...)) exactly as before; transient errors
retry up to 3× before giving up.
OutputFetchTransientRetryTests.swift (new): 16 swift-testing tests
covering the retry helper directly, buildOutput via TestURLProtocol
(first /api/view throws ECONNRESET, second succeeds), and the end-to-end
polling stream. Regression assertions confirm that empty output, auth
failures, .cancelled, and .jobFailed are unchanged.

Why

ECONNRESET on a freshly resumed socket is by definition transient —
Transport.translate (from PR #2) already maps it to .network (transient),
PollingFallback.isTransient already returns true for it — but neither
the WS output-build path nor buildOutput used those classifications.
This completes the picture: transient errors are retried at every
output-fetch boundary, not just at the poll-loop boundary.

Testing

swift build   # clean (one pre-existing warning in ReattachCoordinator, unchanged)
swift test    # 122 tests in 14 suites — all pass, 0 failures

Note: this fix was built stacked on #2; since #2 merged before this PR was
opened, the diff lands cleanly against main with the POSIX mapping already
present — the ECONNRESET-is-transient assertion holds without any
additional changes.

_{🔄 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/3 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 6/19/2026 **Status:** ✅ Merged **Merged:** 6/19/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-1606-comfy-ios-backgrounded-fast-job-recovers-but-the-output` --- ### 📝 Commits (3) - [`39c1eae`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/39c1eae04d176c729bcb5b553b3e006b4acb77dd) fix(transport): treat POSIX socket-drop errors as transient so jobs recover via polling - [`d94aaeb`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/d94aaeb62be03a5dc9d4a592adc0a9fd85247f15) fix(transport): retry transient errors on the execution_success output fetch - [`ea35614`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/ea35614f722ef4a4ac8c398c0949cd49bec0a25f) merge: main into BE-1606 branch to drop redundant BE-1594 base (it squash-merged) ### 📊 Changes **3 files changed** (+428 additions, -22 deletions) <details> <summary>View changed files</summary> 📝 `Sources/ComfySwiftSDK/Internal/PollingFallback.swift` (+59 -11) 📝 `Sources/ComfySwiftSDK/Internal/WebSocketSession.swift` (+25 -11) ➕ `Tests/ComfySwiftSDKTests/OutputFetchTransientRetryTests.swift` (+344 -0) </details> ### 📄 Description ## ELI5 The previous fix (merged as PR #2) stopped a backgrounded job from showing an immediate false-failure on resume — but a job that *finished* while the app was asleep could still fail ~20s later: fetching the output image over the just-resumed connection hits a POSIX socket-drop (ECONNRESET), the single `try/catch` gave up, and the user saw an error on a job that actually succeeded. This PR wraps that output-fetch step in a bounded retry so the result loads instead of erroring. ## What - **`PollingFallback.swift`**: added `static func withTransientRetry<T>` — 3 attempts, 250 ms pause after the first failure, immediate propagation of non-transient errors. Applied to every `downloadView` / `downloadViewToTempFile` call inside `buildOutput` so the polling-recovered path also survives a stale-connection fetch. - **`WebSocketSession.swift`**: the `execution_success` output-build `catch` block now wraps each `downloadView` / `downloadViewToTempFile` call in `PollingFallback.withTransientRetry`. Non-transient errors still reach `continuation.yield(.failed(...))` exactly as before; transient errors retry up to 3× before giving up. - **`OutputFetchTransientRetryTests.swift`** (new): 16 swift-testing tests covering the retry helper directly, `buildOutput` via `TestURLProtocol` (first `/api/view` throws `ECONNRESET`, second succeeds), and the end-to-end polling stream. Regression assertions confirm that empty output, auth failures, `.cancelled`, and `.jobFailed` are unchanged. ## Why `ECONNRESET` on a freshly resumed socket is by definition transient — `Transport.translate` (from PR #2) already maps it to `.network` (transient), `PollingFallback.isTransient` already returns `true` for it — but neither the WS output-build path nor `buildOutput` used those classifications. This completes the picture: transient errors are retried at every output-fetch boundary, not just at the poll-loop boundary. ## Testing ``` swift build # clean (one pre-existing warning in ReattachCoordinator, unchanged) swift test # 122 tests in 14 suites — all pass, 0 failures ``` Note: this fix was built stacked on #2; since #2 merged before this PR was opened, the diff lands cleanly against main with the POSIX mapping already present — the `ECONNRESET`-is-transient assertion holds without any additional changes. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:33:50 +00:00