[PR #27] [MERGED] refactor(transport): share best-effort cancelJob-on-cancelled tail between WS and polling producers #29

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-2356-share-cancel-on-cancelled

📝 Commits (1)

e235281 refactor(transport): share best-effort cancelJob-on-cancelled tail between WS and polling producers

📊 Changes

2 files changed (+20 additions, -10 deletions)

View changed files

📝 Sources/ComfySwiftSDK/Internal/PollingFallback.swift (+19 -5)
📝 Sources/ComfySwiftSDK/Internal/WebSocketSession.swift (+1 -5)

📄 Description

ELI-5

When you cancel a job stream from your side, the SDK also tells the server to cancel the job — a nice "cancel means cancel everywhere" guarantee. Two different stream producers (the WebSocket path and the polling-fallback path) had the exact same little block of code doing this, copy-pasted. This moves that block into one shared helper both call, so there's a single place for that behavior instead of two identical copies.

What changed

The WebSocket and polling stream producers each carried an identical onTermination snippet:

if case .cancelled = reason {
    Task.detached { await transport.cancelJob(id: jobId) }
}

Extracted verbatim into one shared static helper:

PollingFallback.fireCancelJobIfCancelled(_ reason:, transport:, jobId:)

WebSocketSession.eventStream now calls PollingFallback.fireCancelJobIfCancelled(...).
PollingFallback.eventStream now calls Self.fireCancelJobIfCancelled(...).
The helper lives on PollingFallback, the existing home for statics already shared across the stream producers (e.g. isTransient).

Scope / judgment calls

ReattachCoordinator is intentionally left untouched. Its onTermination is just { _ in task.cancel() } — reattaching to an already-running job must not fire a server-side cancel, so it deliberately never had this snippet and still doesn't.
Deliberately narrow. Only the cancelJob side-effect is shared. The surrounding AsyncThrowingStream scaffold is genuinely different across producers (WS registers into a WebSocketStreamRegistry and wraps a withTaskCancellationHandler with a WS-close onCancel; polling does not), so it is not genericized here — that would be premature abstraction.
Pure refactor: behavior is byte-identical; no functional change.

Testing

swift build — clean.
swift test — all 150 tests across 20 suites pass.

_{🔄 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/27 **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-2356-share-cancel-on-cancelled` --- ### 📝 Commits (1) - [`e235281`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/e2352810925999bbb66eef1a5ef6ab3473dd47e5) refactor(transport): share best-effort cancelJob-on-cancelled tail between WS and polling producers ### 📊 Changes **2 files changed** (+20 additions, -10 deletions) <details> <summary>View changed files</summary> 📝 `Sources/ComfySwiftSDK/Internal/PollingFallback.swift` (+19 -5) 📝 `Sources/ComfySwiftSDK/Internal/WebSocketSession.swift` (+1 -5) </details> ### 📄 Description ## ELI-5 When you cancel a job stream from your side, the SDK also tells the server to cancel the job — a nice "cancel means cancel everywhere" guarantee. Two different stream producers (the WebSocket path and the polling-fallback path) had the exact same little block of code doing this, copy-pasted. This moves that block into one shared helper both call, so there's a single place for that behavior instead of two identical copies. ## What changed The WebSocket and polling stream producers each carried an identical `onTermination` snippet: ```swift if case .cancelled = reason { Task.detached { await transport.cancelJob(id: jobId) } } ``` Extracted verbatim into one shared static helper: ```swift PollingFallback.fireCancelJobIfCancelled(_ reason:, transport:, jobId:) ``` - `WebSocketSession.eventStream` now calls `PollingFallback.fireCancelJobIfCancelled(...)`. - `PollingFallback.eventStream` now calls `Self.fireCancelJobIfCancelled(...)`. - The helper lives on `PollingFallback`, the existing home for statics already shared across the stream producers (e.g. `isTransient`). ## Scope / judgment calls - **`ReattachCoordinator` is intentionally left untouched.** Its `onTermination` is just `{ _ in task.cancel() }` — reattaching to an already-running job must **not** fire a server-side cancel, so it deliberately never had this snippet and still doesn't. - **Deliberately narrow.** Only the `cancelJob` side-effect is shared. The surrounding `AsyncThrowingStream` scaffold is genuinely different across producers (WS registers into a `WebSocketStreamRegistry` and wraps a `withTaskCancellationHandler` with a WS-close `onCancel`; polling does not), so it is **not** genericized here — that would be premature abstraction. - Pure refactor: behavior is byte-identical; no functional change. ## Testing - `swift build` — clean. - `swift test` — all 150 tests across 20 suites pass. --- <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:07 +00:00