Comfy-Org/comfy-swift-sdk

Fork 0

mirror of https://github.com/Comfy-Org/comfy-swift-sdk.git synced 2026-11-04 23:47:26 +00:00

[PR #71] [MERGED] fix(router): make run's timeout a real wall-clock stop, not an idle one #72

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-swift-sdk/pull/71
Author: @mattmillerai
Created: 9/11/2026
Status: ✅ Merged
Merged: 9/13/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-13560-router-run-wall-clock-deadline

📝 Commits (1)

1c8d2c7 fix(router): make run's timeout a real wall-clock stop, not an idle one

📊 Changes

3 files changed (+435 additions, -16 deletions)

View changed files

📝 Sources/ComfySwiftSDK/Internal/RouterTransport.swift (+127 -12)
📝 Sources/ComfySwiftSDK/Public/RouterModels.swift (+12 -4)
📝 Tests/ComfySwiftSDKTests/RouterRunTests.swift (+296 -0)

📄 Description

STACKED — merging lands on matt/be-10180-router-models-run (owned by @mattmillerai, PR #68), NOT main. RouterTransport/RouterModels do not exist on main yet; they arrive with #68, so this change has nowhere else to sit. Do not merge this before #68.

ELI-5

run(...) promises "this call will take at most N seconds". It kept that promise between retries, but not during one. The per-request timeout it used is an idle timer — it counts the gap between bytes and starts over every time a byte arrives — so a server that dribbles something every few seconds could hold one attempt open forever while the code that watches the clock sat waiting for that attempt to finish. Now the whole call races a real stopwatch: when the stopwatch wins, the request is cancelled and you get ComfyError.timeout at roughly the time you asked for.

What changed

RouterTransport.run wraps the entire withAuthRetry/collect seam in a new private withWallClockDeadline(_:operation:): a withThrowingTaskGroup with a work child and a Task.sleep-until-deadline child, whichever finishes first cancelling the other.

Transport.withAuthRetry is unchanged — the wrap sits at the run seam, so the ComfyUI workflow surface keeps the shared helper exactly as it was.
No public API change, no new error case, no feature flag.
The request.timeoutInterval = remaining comment that documented the gap as accepted has been rewritten: it now says the clamp is one of three locks on the same door, and why it stays (it is the only one that stops the socket at the deadline rather than shortly after it, and it keeps a silent hold off the session's 60s default).
The public timeout doc now states the bound is a hard stop rather than an idle timeout, and carries the one caveat below.

Evidence the bug is real (measured, not assumed)

Against the pre-change code, a stub trickling one byte every 100ms at a 1s URLRequest.timeoutInterval was still running at 6s and would have run indefinitely; a fully silent stub at the same 1s interval failed at 1.15s. That is the idle-timeout reset, reproduced directly. With the fix, the same trickling stub stops at the 0.75s budget.

Answers to the three questions the ticket asks

1. Does cancelling mid-flight leave a billable run server-side? Yes, potentially — and the existing prose still reads true, so it is tightened rather than corrected. Hanging up on the socket is not a server-side cancel; Router has no cancel endpoint, and the generation the key names may keep running and be charged. The - Throws: block already said the outcome of a client-side timeout is unknown and that the remedy is to re-run under the same idempotencyKey:; it now says explicitly that this applies to timeout in particular and why. Nothing about the idempotency-key contract changes: a later re-run with the same key collects the same run rather than starting a second one.

2. Which racing construct? withThrowingTaskGroup with two children, as suggested. The loser is cancelled via group.cancelAll(). The sleeping child cannot leak when the work finishes first: cancelAll() cancels it, its Task.sleep throws, it returns .timerCancelled, and the group awaits it before the scope exits.

3. The trap — deadline expiry must not masquerade as caller cancellation. The decision is taken on the first outcome out of the group, before anything is cancelled. Once .expired is in hand the work child's (now .cancelled) error is never awaited — it is discarded with the group — so ComfyError.timeout is the only thing that can propagate. The timer child's cancellation is encoded as a value (.timerCancelled) rather than thrown, because rethrowing a bare CancellationError there would race the work child's real outcome and reach the caller as .unknown. Caller-initiated cancellation is unaffected: both children are cancelled, the timer yields, and collect's .cancelled is what comes out. Both directions have their own test.

Tests

Three added to RouterRunTests, on a new file-scoped HoldingURLProtocol stub that answers on its own schedule (the existing TestURLProtocol returns its response synchronously and so cannot model an attempt that is still open at the deadline). It records every stopLoading, which is how "the in-flight request was actually cancelled" is asserted.

Test	Shape	Fails with the new bound reverted?
an attempt that never answers stops at the deadline rather than hanging	silent hold	No — see Residual
an attempt that trickles data forever still stops at the deadline	100ms trickle	Yes — 8.02s elapsed against a 0.75s budget
cancelling while a request is in flight still surfaces `.cancelled`, not `.timeout`	caller cancel mid-flight	No (no-regression guard, by design)

Each deadline test runs under its own watchdog, so an unenforced bound is a test failure rather than a hung suite.

Judgment calls

The silent-hold test is a pin, not a proof. It is annotated as such in its own doc comment so a future reader does not mistake it for the load-bearing one.
The teardown assertion is polled, not read once. stopLoading is delivered by URLSession's own machinery and can land just after the cancellation error reaches the awaiting task; reading the counter immediately was flaky (1 failure in 4 runs). It is now a bounded poll. The RouterRun suite was re-run 3× after the change, green each time.
maxSleepInterval clamp. Task.sleep(nanoseconds:) takes a UInt64 and UInt64(_:) traps rather than throwing. validateTimeout only requires the budget to be finite and positive, so TimeInterval.greatestFiniteMagnitude is a legal timeout that would have crashed the caller's process on the nanosecond conversion. Clamped to a literal just below 2^64 ns — deliberately not Double(UInt64.max) / 1e9, which rounds up to exactly 2^64 and traps for the same reason.
The group awaits both children before the throw leaves its scope. That is what makes "the in-flight request is cancelled" true rather than "a detached request is still running somewhere" — and it is also the source of the first Residual item below.

Residual

1. A hung OAuth refreshProvider still outlives the budget. This is not fixed here, and it is part of the original review finding. CodeRabbit's comment on #68 (#discussion_r3992307474) asks to "enforce the same deadline around the complete withAuthRetry/collect operation, including refresh". The refresh half does not hold. Transport.performRefresh coalesces concurrent refreshes behind one unstructured Task and awaits its .value; that await does not end when a single waiter is cancelled — deliberately, since one caller giving up must not cancel a refresh other callers are waiting on. So the deadline fires on time, but run cannot return until the refresh does. Measured: a refreshProvider that sleeps 30s against a timeout: of 0.75 throws ComfyError.timeout — after 31.09s. I wrote that test, watched it fail, and removed it rather than ship a red assertion; the limit is instead documented on withWallClockDeadline and in the public timeout doc. Closing it means making the shared Transport refresh await cancellable (e.g. racing task.value against Task.isCancelled, leaving the shared refresh itself running), which changes behaviour for the ComfyUI workflow surface too — explicitly out of scope for this change, and worth its own ticket. Note this contradicts the ticket's Evidence 5, which asserts that wrapping at the run seam "therefore covers the refresh path": it covers the refresh path's budget, not its cancellability.

2. The same review comment also asks to apply authentication first and assign timeoutInterval afterwards. Not done. collect still sets request.timeoutInterval = remaining before applyAuth(to:), so a proactive refresh performed inside applyAuth makes that interval stale by however long the refresh took, and the socket-level bound overshoots the deadline by that much. The new wall-clock stop makes this invisible to the documented guarantee — the call still ends at the deadline — so it is a tightening rather than a defect, and it is left out to keep this diff to the one behaviour the ticket names.

3. The silent-hold test does not fail when the new bound is reverted. A fully silent attempt is exactly the shape URLRequest.timeoutInterval already catches (it is an idle timeout and nothing resets it), so the merged narrow fix covers that case on its own. The test is kept as a regression pin for the documented bound and the error identity, and is annotated in-file so it is not mistaken for proof of this change. The trickle test is the mutation-load-bearing one.

4. Unexercised artifacts. The ticket's https://linear.app/... review link was not opened (no access from this environment). The two GitHub review permalinks it names were read via the API and are quoted above. No live Comfy Router host was exercised — all verification is against the repo's stubbed transport, which is what the ticket's acceptance criteria specify.

Provenance

Authored by: agent-work loop
Verified: swift build: clean. swift test --no-parallel: 316 tests in 32 suites passed, 0 failed (313 before this change, 3 added). swift test --no-parallel --filter RouterRun re-run 3× for flake: 35 tests passed each time. Mutation run with the withWallClockDeadline wrap removed: the trickle test fails (elapsed 8.02s against a 0.75s budget, teardown never observed), the silent-hold and in-flight-cancellation tests still pass. python3 Scripts/contract/check_router_contract.py: OK, 15 error types, run route in sync with the vendored spec.
Deviations: the refresh-path half of the deadline is not enforced, and the applyAuth-before-timeoutInterval reordering is not done — both under ## Residual with the reasons.

_{🔄 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/71 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/11/2026 **Status:** ✅ Merged **Merged:** 9/13/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-13560-router-run-wall-clock-deadline` --- ### 📝 Commits (1) - [`1c8d2c7`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/1c8d2c7cbd91e241a965bd08f85478b4752c3fe1) fix(router): make run's timeout a real wall-clock stop, not an idle one ### 📊 Changes **3 files changed** (+435 additions, -16 deletions) <details> <summary>View changed files</summary> 📝 `Sources/ComfySwiftSDK/Internal/RouterTransport.swift` (+127 -12) 📝 `Sources/ComfySwiftSDK/Public/RouterModels.swift` (+12 -4) 📝 `Tests/ComfySwiftSDKTests/RouterRunTests.swift` (+296 -0) </details> ### 📄 Description **STACKED — merging lands on `matt/be-10180-router-models-run` (owned by @mattmillerai, PR #68), NOT `main`.** `RouterTransport`/`RouterModels` do not exist on `main` yet; they arrive with #68, so this change has nowhere else to sit. Do not merge this before #68. ## ELI-5 `run(...)` promises "this call will take at most N seconds". It kept that promise between retries, but not during one. The per-request timeout it used is an *idle* timer — it counts the gap between bytes and starts over every time a byte arrives — so a server that dribbles something every few seconds could hold one attempt open forever while the code that watches the clock sat waiting for that attempt to finish. Now the whole call races a real stopwatch: when the stopwatch wins, the request is cancelled and you get `ComfyError.timeout` at roughly the time you asked for. ## What changed `RouterTransport.run` wraps the entire `withAuthRetry`/`collect` seam in a new private `withWallClockDeadline(_:operation:)`: a `withThrowingTaskGroup` with a work child and a `Task.sleep`-until-deadline child, whichever finishes first cancelling the other. - `Transport.withAuthRetry` is **unchanged** — the wrap sits at the `run` seam, so the ComfyUI workflow surface keeps the shared helper exactly as it was. - No public API change, no new error case, no feature flag. - The `request.timeoutInterval = remaining` comment that documented the gap as accepted has been rewritten: it now says the clamp is one of three locks on the same door, and why it stays (it is the only one that stops the *socket* at the deadline rather than shortly after it, and it keeps a silent hold off the session's 60s default). - The public `timeout` doc now states the bound is a hard stop rather than an idle timeout, and carries the one caveat below. ## Evidence the bug is real (measured, not assumed) Against the pre-change code, a stub trickling one byte every 100ms at a 1s `URLRequest.timeoutInterval` was **still running at 6s** and would have run indefinitely; a fully silent stub at the same 1s interval failed at 1.15s. That is the idle-timeout reset, reproduced directly. With the fix, the same trickling stub stops at the 0.75s budget. ## Answers to the three questions the ticket asks **1. Does cancelling mid-flight leave a billable run server-side?** Yes, potentially — and the existing prose still reads true, so it is tightened rather than corrected. Hanging up on the socket is not a server-side cancel; Router has no cancel endpoint, and the generation the key names may keep running and be charged. The `- Throws:` block already said the outcome of a client-side `timeout` is unknown and that the remedy is to re-run under the same `idempotencyKey:`; it now says explicitly that this applies to `timeout` in particular and why. Nothing about the idempotency-key contract changes: a later re-run with the same key collects the same run rather than starting a second one. **2. Which racing construct?** `withThrowingTaskGroup` with two children, as suggested. The loser is cancelled via `group.cancelAll()`. The sleeping child cannot leak when the work finishes first: `cancelAll()` cancels it, its `Task.sleep` throws, it returns `.timerCancelled`, and the group awaits it before the scope exits. **3. The trap — deadline expiry must not masquerade as caller cancellation.** The decision is taken on the **first** outcome out of the group, *before* anything is cancelled. Once `.expired` is in hand the work child's (now `.cancelled`) error is never awaited — it is discarded with the group — so `ComfyError.timeout` is the only thing that can propagate. The timer child's cancellation is *encoded as a value* (`.timerCancelled`) rather than thrown, because rethrowing a bare `CancellationError` there would race the work child's real outcome and reach the caller as `.unknown`. Caller-initiated cancellation is unaffected: both children are cancelled, the timer yields, and `collect`'s `.cancelled` is what comes out. Both directions have their own test. ## Tests Three added to `RouterRunTests`, on a new file-scoped `HoldingURLProtocol` stub that answers on its own schedule (the existing `TestURLProtocol` returns its response synchronously and so cannot model an attempt that is still open at the deadline). It records every `stopLoading`, which is how "the in-flight request was actually cancelled" is asserted. | Test | Shape | Fails with the new bound reverted? | | --- | --- | --- | | an attempt that never answers stops at the deadline rather than hanging | silent hold | **No** — see Residual | | an attempt that trickles data forever still stops at the deadline | 100ms trickle | **Yes** — 8.02s elapsed against a 0.75s budget | | cancelling while a request is in flight still surfaces `.cancelled`, not `.timeout` | caller cancel mid-flight | No (no-regression guard, by design) | Each deadline test runs under its own watchdog, so an unenforced bound is a test **failure** rather than a hung suite. ## Judgment calls - **The silent-hold test is a pin, not a proof.** It is annotated as such in its own doc comment so a future reader does not mistake it for the load-bearing one. - **The teardown assertion is polled, not read once.** `stopLoading` is delivered by `URLSession`'s own machinery and can land just after the cancellation error reaches the awaiting task; reading the counter immediately was flaky (1 failure in 4 runs). It is now a bounded poll. The RouterRun suite was re-run 3× after the change, green each time. - **`maxSleepInterval` clamp.** `Task.sleep(nanoseconds:)` takes a `UInt64` and `UInt64(_:)` *traps* rather than throwing. `validateTimeout` only requires the budget to be finite and positive, so `TimeInterval.greatestFiniteMagnitude` is a legal `timeout` that would have crashed the caller's process on the nanosecond conversion. Clamped to a literal just below 2^64 ns — deliberately not `Double(UInt64.max) / 1e9`, which rounds *up* to exactly 2^64 and traps for the same reason. - **The group awaits both children before the throw leaves its scope.** That is what makes "the in-flight request is cancelled" true rather than "a detached request is still running somewhere" — and it is also the source of the first Residual item below. ## Residual **1. A hung OAuth `refreshProvider` still outlives the budget. This is not fixed here, and it is part of the original review finding.** CodeRabbit's comment on #68 ([`#discussion_r3992307474`](https://github.com/Comfy-Org/comfy-swift-sdk/pull/68#discussion_r3992307474)) asks to "enforce the same deadline around the complete `withAuthRetry`/`collect` operation, **including refresh**". The refresh half does not hold. `Transport.performRefresh` coalesces concurrent refreshes behind one *unstructured* `Task` and awaits its `.value`; that await does not end when a single waiter is cancelled — deliberately, since one caller giving up must not cancel a refresh other callers are waiting on. So the deadline fires on time, but `run` cannot return until the refresh does. Measured: a `refreshProvider` that sleeps 30s against a `timeout:` of 0.75 throws `ComfyError.timeout` — after 31.09s. I wrote that test, watched it fail, and removed it rather than ship a red assertion; the limit is instead documented on `withWallClockDeadline` and in the public `timeout` doc. Closing it means making the shared `Transport` refresh await cancellable (e.g. racing `task.value` against `Task.isCancelled`, leaving the shared refresh itself running), which changes behaviour for the ComfyUI workflow surface too — explicitly out of scope for this change, and worth its own ticket. Note this contradicts the ticket's Evidence 5, which asserts that wrapping at the `run` seam "therefore covers the refresh path": it covers the refresh path's *budget*, not its *cancellability*. **2. The same review comment also asks to apply authentication first and assign `timeoutInterval` afterwards. Not done.** `collect` still sets `request.timeoutInterval = remaining` *before* `applyAuth(to:)`, so a proactive refresh performed inside `applyAuth` makes that interval stale by however long the refresh took, and the socket-level bound overshoots the deadline by that much. The new wall-clock stop makes this invisible to the documented guarantee — the call still ends at the deadline — so it is a tightening rather than a defect, and it is left out to keep this diff to the one behaviour the ticket names. **3. The silent-hold test does not fail when the new bound is reverted.** A fully silent attempt is exactly the shape `URLRequest.timeoutInterval` already catches (it is an idle timeout and nothing resets it), so the merged narrow fix covers that case on its own. The test is kept as a regression pin for the documented bound and the error identity, and is annotated in-file so it is not mistaken for proof of this change. The trickle test is the mutation-load-bearing one. **4. Unexercised artifacts.** The ticket's `https://linear.app/...` review link was not opened (no access from this environment). The two GitHub review permalinks it names *were* read via the API and are quoted above. No live Comfy Router host was exercised — all verification is against the repo's stubbed transport, which is what the ticket's acceptance criteria specify. ## Provenance - **Authored by:** agent-work loop - **Verified:** `swift build`: clean. `swift test --no-parallel`: 316 tests in 32 suites passed, 0 failed (313 before this change, 3 added). `swift test --no-parallel --filter RouterRun` re-run 3× for flake: 35 tests passed each time. Mutation run with the `withWallClockDeadline` wrap removed: the trickle test fails (elapsed 8.02s against a 0.75s budget, teardown never observed), the silent-hold and in-flight-cancellation tests still pass. `python3 Scripts/contract/check_router_contract.py`: OK, 15 error types, run route in sync with the vendored spec. - **Deviations:** the refresh-path half of the deadline is not enforced, and the `applyAuth`-before-`timeoutInterval` reordering is not done — both under `## Residual` with the reasons. --- <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:39 +00:00

closed this issue
added the
pull-request
label

No labels

enhancement

pull-request

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Comfy-Org/comfy-swift-sdk#72

No description provided.

Rows
Columns