mirror of
https://github.com/Comfy-Org/comfy-swift-sdk.git
synced 2026-11-04 23:47:26 +00:00
[PR #77] [MERGED] feat(router): add client.models.submit / subscribe / handle for queued delivery #78
Labels
No labels
enhancement
pull-request
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Comfy-Org/comfy-swift-sdk#78
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
📋 Pull Request Information
Original PR: https://github.com/Comfy-Org/comfy-swift-sdk/pull/77
Author: @mattmillerai
Created: 9/16/2026
Status: ✅ Merged
Merged: 9/18/2026
Merged by: @mattmillerai
Base:
main← Head:matt/be-14639-router-queue-surface📝 Commits (5)
18d2b71feat(router): add client.models.submit / subscribe / handle for queued deliveryfebb80bfix(router): guard the acknowledgement status, keep the failure signal, and stop wasting the last poll pause1698b1fMerge origin/main into matt/be-14639-router-queue-surface7cc5079fix(router): retry transient status polls and give the result fetch a download-sized budget9bef73dMerge branch 'main' into matt/be-14639-router-queue-surface📊 Changes
11 files changed (+3363 additions, -30 deletions)
View changed files
📝
README.md(+87 -0)📝
Scripts/contract/check_router_contract.py(+160 -2)📝
Sources/ComfySwiftSDK/Internal/RouterConstants.swift(+24 -8)📝
Sources/ComfySwiftSDK/Internal/RouterErrorMapping.swift(+25 -1)➕
Sources/ComfySwiftSDK/Internal/RouterQueueTransport.swift(+968 -0)📝
Sources/ComfySwiftSDK/Internal/RouterTransport.swift(+61 -17)📝
Sources/ComfySwiftSDK/Internal/SDKLog.swift(+46 -0)📝
Sources/ComfySwiftSDK/Public/RouterModels.swift(+200 -1)➕
Sources/ComfySwiftSDK/Public/RouterRequests.swift(+366 -0)➕
Tests/ComfySwiftSDKTests/RouterQueueTests.swift(+1425 -0)📝
spec/README.md(+1 -1)📄 Description
ELI-5
Comfy Router can run a partner model two ways: hold the connection open until it finishes (
client.models.run, which this SDK already had), or queue it — the server takes the request, hands back an id, and you check back later. Only the second survives a dropped network, a backgrounded app, or a relaunch. The Python and TypeScript SDKs have had the queued form for a while; Swift was the only Router SDK without it. This adds it:client.models.submit,client.models.subscribe,client.models.handle, and the handle those hand back.What this adds
client.models.submit(_:input:idempotencyKey:timeout:)—POST …/requests, returns aRouterRequestHandlewithrequestId,model,status(),result(),cancel()and anAsyncThrowingStreamfromevents().client.models.subscribe(_:input:onQueueUpdate:timeout:idempotencyKey:)— submit + poll + collect in one call, returning the sameRouterRunResultthatrundoes.client.models.handle(_:requestId:)— rebuilds a handle from the two ids with no request made; both validated locally, so a bad row in a caller's database throws before anything reaches the wire.RouterRequestState/RouterRequestStatus/RouterCancelOutcome— the queue's own types, alongside the existingRouterErrortaxonomy, which already carriednotEnabledand needed no change.Scripts/contract/check_router_contract.py: pins the four queue operations — path and HTTP method — against the vendored spec.No new dependencies (Foundation only), and no client-side feature flag: the gate is server-side (
403 not_enabled), matching the decision recorded on the TypeScript side.Design notes worth a reviewer's time
One transport, not two. The queue routes live in
RouterQueueTransport.swiftas anextension RouterTransport, sharing the run route's base-URL validation, redirect refusal, credential injection, proactive-OAuth/401-refresh, attempt cap andwithWallClockDeadline. That required widening a handful ofprivatemembers inRouterTransport.swifttointernal— the whole visible cost of not forking a second transport that could drift on auth or on which base URLs are safe.runURLwas extracted into a parameterisedrouteURL(baseURL:template:path:requestId:); the run route's own behaviour is byte-identical (its existing tests cover it unchanged).URLs are composed, never taken from the response. The submit response carries
status_url/response_url/cancel_url. This SDK ignores all three and builds every route from the contract-pinned templates. Each of those calls stamps the caller's credential onto the request, so a URL read out of a response body is a place the server could have it sent. A test asserts that an acknowledgement namingevil.testfor all three still produces requests only toapi.comfy.org. The server-suppliedrequest_idgets the same treatment: it is run through the same path-segment validation a caller's would be, before it reaches a URL.Poll-authoritative. The status route decides when a request is done — not the result route's
202, not the submit acknowledgement. The schedule starts at 0.5 s, backs off ×1.5 to a 10 s ceiling, and a serverRetry-Afterbeats it outright, capped at 60 s (RouterRequestHandle.maximumRetryAfter) before it is slept on. The pause is also clamped to the remaining budget, so a 60 s hint with 2 s left does not report the timeout 58 s late.Terminal is not successful, and who throws depends on who is asking. A provider failure, a content refusal, and a cancellation that took effect all read
COMPLETEDwith anerror_type. The collecting calls (subscribe,handle.result()) throw the matchingRouterErrorbucket — a200from the result route is never returned as success when the completion reported a failure. The watching calls (handle.status(),handle.events()) report it as data on the observation. That split is not invented here: it is exactly what the Python and TypeScript SDKs do, and it exists because reading a status is how a caller discovers a failure in the first place.Giving up cancels, once, best-effort.
subscribe's elapsed timeout and aTask.cancel()each issue exactly onePUTcancel, sent once with no retries and a 5 s bound, before throwing.timeout/.cancelled. The cancel's own failure is swallowed — the error the caller can act on is the one they get. It runs in a detached task because on the cancellation path every inlineawaitwould throw before reaching the wire, which is the path it matters most on.handle.result()andhandle.events()deliberately do not cancel: they did not submit the request and do not own it.Verifying against the sibling implementations
The two SDK PRs the ticket names are public, so I read them rather than assuming the shape. Four divergences turned up in my first pass and are fixed in this branch — each one would have made the Swift surface behave differently from the other two:
submitaccepted only201; both siblings accept200,201or202as an acceptance, with the body'srequest_idas the actual proof. Widened, with therequest_idstill required (a2xxwithout one throws rather than producing a handle that addresses nothing).handle.result()was a single fetch that threw on a202; both siblings'handle.get()poll to completion, then collect. Changed.events()threw on a completion carrying anerror_type; both siblings deliberately yield it (TypeScript's README spells out why). Changed, along with the stream element — it now yields the fullRouterRequestStatus, mirroringQueueUpdate, soerrorTypeis reachable from an observation.timeout: 0was rejected; TypeScript documentstimeoutMs: 0as "look once". The polling entry points now take a separatevalidatePollTimeoutthat admits zero — a poll is a cheapGET, so the run route's 1-second floor (which exists because a sub-second budget buys one billable POST too short to answer in) does not transfer.submitandsubscribe, which do dispatch a billable request, keep the existing floor.The vendored spec, and what the contract check does about it
spec/router-openapi.yamlis a one-way vendored copy (spec/README.md,AGENTS.md) and I did not hand-edit it. When this branch was opened its sync predated the queue paths, so the drift check was written to arm itself. Mergingorigin/mainbrought the sync that declares them (via #80): the spec now carries all seven paths, andcheck_router_contract.pyruns the queue half for real and passes — all four operation IDs, their paths, and their methods (the cancelPUTincluded) matchRouterConstants.The stand-down is still in the check, and still matters for the next spec sync that might drop the operations: it asserts each of
submitRouterModelRequest/getRouterModelRequestStatus/getRouterModelRequestResult/cancelRouterModelRequestagainstRouterConstants, path and method (the cancel is aPUT, the one route here a reader will assume is aPOST) — but stands down with a::notice::and a pass when the spec declares none of the four, and fails hard on a partial set. Failing today would make the job red for something no PR in this repo can fix, which is how a check gets disabled.Verified by temporarily patching a local copy of the spec (reverted, not committed): all four declared → pass;
cancelRouterModelRequestmoved topost→ fail naming the method; one operation removed → fail naming the partial set.The
{request_id}path-parameter spelling in the four new templates is my assumption. If upstream spells it differently, the drift check fails loudly on the sync that brings it in and the fix is the four constants — which is the two-step changespec/README.mdalready describes.Verification
swift buildandswift test --no-parallelon the macOS target, after mergingorigin/main(which brought #80's run-route query parameters intoRouterTransport). 442 tests in 34 suites, 0 failures, of whichRouterQueueTestscontributes 52. All run against the recorded-serverTestURLProtocolharness in the style ofRouterRunTests— never a live server.python3 Scripts/contract/check_router_contract.pyexits 0.Residual
The queue routes have not been exercised against a live Comfy Router. Every test here runs against a
TestURLProtocolstub that answers what I believe the contract says, so what is proven is that the client behaves correctly given that contract — not that the contract is what the server serves. Two things follow, and both need a human with preview access before this is cut into a release:{request_id}parameter spelling, the201acknowledgement body's field names (request_id,queue_position,status), the status body's (status,queue_position,error_type), and theALREADY_COMPLETEDspelling on the cancel400are all transcribed from the ticket's prose and the sibling SDKs, not observed. The drift check catches the route half on the next spec sync; the body field names are pinned by nothing — a mismatch there surfaces asRouterInvalidResponseErroror a silently-absentqueuePosition, not as a build failure. Worth one live smoke run against the preview before tagging.403 not_enabledoutside the preview is reproduced from the contract, not observed. The code path is tested (it throws.notEnabledand is not retried), but no call has been made to a server that actually answers it.The vendoredResolved by the merge fromspec/router-openapi.yamlstill carries only three paths, so the queue half of the contract check is dormant on this branch.origin/main. The sync that #80 carried declares all four queue operations, so the check is armed and green on this branch, and the four route constants are now pinned by CI.Artifacts I could not open. The ticket's parent,
BE-12682(Epic: Comfy Router Queueing POC), was named to me by identifier and title only — its body and comments were not available, so any acceptance detail recorded there and nowhere else is unexercised. The three attachments were reachable and I read all three (comfy-python-sdk#137,comfy-typescript-sdk#133,docs#1688); the first two produced the four parity fixes above.A
subscribethat gives up while the polls themselves are failing does not cancel.subscribeissues its one best-effort cancel onComfyError.timeoutand.cancelledonly. The poll loop now throws the last swallowed transient failure at the deadline rather than a bare.timeout, so a watch that ran out of budget against a persistent503or a down link rethrows that failure and issues no cancel: the request stays queued and may still be billed. Deliberate — a cancel over a link that is evidently down either cannot be delivered (and costs the callerbestEffortCancelTimeouton the way out) or abandons a generation that was probably fine. Callhandle.cancel()explicitly if your app would rather give up the generation than pay for it.The request id is not attached to a non-terminal transport failure out of
subscribe. A.network,.offlineor.authExpiredthrown mid-poll is rethrown unchanged, so a caller who reached forsubscriberather thansubmit+handle.result()has no id in hand to collect or cancel the queued run with. Both remedies were considered and rejected as changes to contracted behaviour: attaching the id changes the publicComfyErrorshape (a new associated value on cases every existingcatchpattern-matches, or re-wrapping a transport failure as aRouterErrorit is not), and extending the best-effort cancel to those three cases cancels requests that are probably fine over a link that is probably down.submitreturns the handle before any of this can happen and is the documented flow for anything that has to survive a dropped connection;subscribeis the convenience call that trades that for oneawait.The result fetch deliberately overruns the caller's
timeout. It is floored atRouterTransport.resultFetchMinimumBudget(=RouterModels.defaultRequestTimeout, 60 s) rather than at the 1-second attempt floor, because that leg is a transfer of the provider's payload rather than a wait — andresult(timeout: 0)would otherwise give a multi-megabyte download exactly one second on every call, reporting a timeout for a generation that had completed and been billed. The overrun is bounded by that constant and only applies to the one fetch; atimeoutabove 60 s bounds it as stated. Documented onresult(timeout:), onsubscribe, and in the README.RouterRunResult.idempotencyKeychanged fromStringtoString?— a source-breaking change on a pre-1.0 public property. A result collected through a handle thathandle(_:requestId:)rebuilt has no submit and therefore no key, and an invented one would be actively harmful because the documented recovery flow is to re-send that value.runandsubscribestill always populate it. Flagging it because it is the only break in this PR and it needs to be in release notes.Out of scope, as the ticket states, and untouched here: the docs PR adding Swift tabs to the queued-delivery page; the Swift tab on the 209 generated per-model code pages; the Go SDK; webhooks, SSE, priority and queue list/flush; any client-side queue behaviour; any server contract change; and the version bump and tag, which stay a human step once the production server serves these routes outside the preview.
Provenance
swift build: clean, 0 warnings in changed files;swift test --no-parallel: 442 tests in 34 suites, 0 failures;python3 Scripts/contract/check_router_contract.py: exit 0, queue half armed and verifying all four routes against the post-merge spec, plus three synthetic-spec runs proving it arms, catches a moved method, and catches a partial setsubmit/subscribekeep the run route's 1-second minimumtimeoutwhere the TypeScript SDK allows zero — they dispatch a billable request;result/events, which only poll, accept zero.submittakes a defaultedtimeout:the ticket's signature does not name. The stream element isRouterRequestStatusrather than a slimmer event enum, to match the siblings'QueueUpdateand keeperrorTypereachable from an observation.🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.