feat!: subscribe's timeout detaches from an in-flight run instead of erroring

[mattmillerai]

ELI-5

When models.subscribe(..., timeout=N) runs out of time it quietly asks the server to cancel the run. The queue only accepts that cancel while the request is still waiting in line — once it has been dispatched, the run is served to the end and billed whatever you do, and the cancel is refused. Until now the SDK swallowed that refusal along with every other cancel failure and raised a bare TimeoutError, which reads as "the run stopped" — the one thing that did not happen. Now a timeout on an in-flight run is reported as what it is: a detach. You stop waiting, the generation carries on, and you get back the ids and a live handle to collect the thing you are being billed for.

What changed

models.subscribe(timeout=N) now has three endings, told apart by type and never by a message string:

ending	what happened	how it surfaces
cancelled	the request was still queued, the cancel was accepted, nothing ran and nothing is billed	raises SubscribeTimeout (a TimeoutError subclass) with .cancelled True
detached	the cancel did not stop the run — it was refused because the run was in flight, or it was accepted while reporting a live status — and a confirming poll found it still going. It continues, completes and is billed	returns a DetachedRequest / AsyncDetachedRequest carrying .request_id, .model, .status and a live .handle
completed during teardown	the same refusal, but the run had just finished. The result exists and is paid for	collects it and returns it like any other result

A cancel that fails for any other reason — a transport failure, a 401, a 500 — is no longer swallowed. SubscribeTimeout is raised with .cancelled False and the cancel's own failure on .cancel_error and __cause__, because a cancel that never landed means the run may still be going. Before this change all three of those looked identical to a successful cancellation.

Sync and async, with the same names and the same arguments; the async side returns AsyncDetachedRequest so the handle it carries is the awaitable one.

New public names, all exported from comfy_sdk: DetachedRequest, AsyncDetachedRequest, SubscribeTimeout.

How the refusal is recognised

One predicate, _refused_on_state, and it asks the typed question first. Rebasing onto main picked up #157, which types the cancel route's refusals as CancelRefused / AlreadyCompleted — so the "branching on a typed class is strictly better than branching on a status" this originally deferred is now simply what it does:

if isinstance(exc, CancelRefused):
    return True
if getattr(exc, "http_status", None) != _CANCEL_REFUSED_STATUS:
    return False
bucket = error_bucket_of(exc)
return bucket is None or bucket == _UNIDENTIFIED_REFUSAL_CODE

The status clause is the fallback for the one refusal the contract has not named yet — the in-flight one, which today carries no bucket on the header or in the body. It fails closed: a 409 that names a bucket the contract does document (invalid_input, concurrency_limit_exceeded with its Retry-After) is not the state refusal and surfaces on .cancel_error rather than being reported as a detach. That matches two nearby precedents — retry.is_collectable gates 409 on status and bucket, and comfy_low.errors._CODE_BY_STATUS omits 409 outright as ambiguous.

"Names no bucket" has two spellings, because the layer below fills the gap rather than leaving it: comfy_low.errors synthesises code = f"http_{http_status}" for a response that identified itself with nothing, and documents that shape as unable to collide with a wire code or a Router bucket. So a bucket-less 409 arrives carrying http_409, not None, and both are read as "named nothing".

No body-text match anywhere.

How an ACCEPTED cancel is read

A 2xx says the route took the message, not that the run stopped — the binding accepts 200, 202 and 204 alike. The cleanup cancel now hands back the QueueUpdate it was answered with, and that body decides:

the cancel answered	reading
a terminal COMPLETED with an error_type	stopped. This queue has no CANCELED status; it expresses a stop as COMPLETED plus a bucket
a terminal COMPLETED without a bucket	it finished on its own, and is billed — so the result is collected, not discarded
any live status (202/CANCELING, or a 200 on a request that won the race into flight)	not stopped — confirmed against the server and reported as a detach
no body at all (the ordinary 204)	stopped, unconfirmed. Nothing in an empty body contradicts the accept, and charging every timeout an extra round trip to re-confirm the common case is the wrong trade

Because the refusal names nothing about the request's state, the detach is confirmed against the server: one unretried, 10s-bounded status poll, which is what separates "still running" from "finished while we were tearing down". That follows the rule the surface already states — the authoritative state of a request is the next status read, never what a cancel answered. A poll that itself fails does not undo what the refusal established (the request was not cancelled), so it still reports a detach, with status left empty to say the state was unconfirmed.

Compatibility

• SubscribeTimeout subclasses TimeoutError, so every existing except TimeoutError around subscribe keeps catching the cases that still raise, with the same message plus a clause naming the outcome.
• Breaking: models.subscribe is now annotated dict[str, Any] | DetachedRequest. A typed caller that indexes the result directly has to check the type first. This is the behaviour change the detach is — the alternative is telling a caller their run was cancelled when it was not — and the queued surface shipped two days ago behind a server-side gate, so the blast radius is small. Recorded under ### Changed in the changelog.
• SubscribeTimeout pickles and copies with every field intact (__reduce__), so .request_id survives reaching another process — which is the workflow the queued surface exists for.
• timeout bounds the wait, not the teardown. Deciding which of the three endings happened costs up to three further round trips past the deadline (the cancel, the confirming poll, and on the completed path the result fetch), each bounded at 10s — so a subscribe(timeout=N) can return up to ~30s after N in the worst case. Now stated on the method and in the README; it was previously ~10s and undocumented.
• handle.cancel(), handle.get() and handle.iter_events() are untouched. iter_events still never cancels — a for loop with a break must not be destructive.

Verification of the negative claim

This diff's user-facing outcome does not deny a capability, and the empirical check says so rather than the prose: the cleanup cancel is still attempted on every timeout via exactly the path it used before, and the tests assert queue_cancel_count == 1 on each of the three endings. Nothing that could be cancelled before is refused here — a cancel the queue accepts still cancels, and handle.cancel() is unchanged. What the diff removes is a raise, not a capability: the previously-dead-ended timeout now hands back a working handle, and the re-attach path it points at is exercised (models.handle(model, request_id).get() returns the completed result, sync and async).

Judgment calls

• Detach returns, cancelled raises. Mixing a return and a raise across one method's outcomes is unusual, and it is the honest split: a detached run is healthy and collectable (there is something to hand back), while a cancelled one is gone (there is not). It is also what makes the two impossible to confuse without reading a message.
• Cancelled now raises SubscribeTimeout rather than a bare TimeoutError. A subclass, so no handler breaks; it is what carries .request_id, .model and .cancelled for the outcomes that still raise.
• A non-benign cancel failure is chained onto the timeout rather than replacing it. The existing code's reasoning still holds — the timeout is the failure worth reporting, and masking it sends the caller looking in the wrong place — but it is no longer silent: the failure rides out on .cancel_error and __cause__.
• Completed-during-teardown collects the result rather than raising, overrunning the caller's deadline by one bounded fetch. Discarding a generation that has already been generated and billed is the worse trade.
• One test-guard exemption: the sync/async parity walk fails a discovered pair with zero public methods as "introspection broke?". DetachedRequest/AsyncDetachedRequest are frozen dataclasses whose whole surface is fields, so the exemption is declared in a new _DATA_ONLY table with its reason, alongside the file's existing _RENAMES / _ALLOWED_ASYMMETRY / _SYNC_ON_BOTH tables, and it is itself checked for staleness (it fails if the pair ever grows a method the exemption would then hide).

Residual

• The in-flight refusal's wire shape is still not pinned by any contract, and the status clause remains a guess. The typed half is now real (CancelRefused / AlreadyCompleted, from fix(errors): make except RouterError catch every Router refusal #157), but the in-flight refusal has not shipped server side; its 409 is the documented state-conflict status the cancel route already uses for cancel-after-completion, not a value read off the new refusal. If the server ships it with a different status, every timeout raises as it does today — no regression, but the detach never fires — and if it ships a named bucket, that bucket becomes a CancelRefused subclass and the status clause can be deleted outright. _refused_on_state in src/comfy_sdk/model_requests.py is the single predicate to reconcile when the server PR lands. This is the follow-up: re-pin the predicate against the shipped refusal.
• A confirming poll that reports the request as still QUEUED is reported as a detach anyway, which claims a run that may never have started is running and billed. Distinguishing it needs a status vocabulary the contract does not pin (QueueUpdate.status is a deliberately open string; COMPLETED is the only value the SDK names) and a behaviour decision about what subscribe should do instead. Raised in review, deferred to a follow-up alongside the re-pin above, and tracked on that thread.
• The cancel endpoint the report names — PUT /v2/models/{provider}/{model}/requests/{id}/cancel — was exercised only against this repo's in-tree stub, never a live deployment. The test suite has no network dependency by design and this environment has no credentials for the router; the refusal is therefore a stub the SDK was written against, not a recorded response.
• The production request the report cites as having produced this path was not examined. It lives on an internal deployment this environment cannot reach, and its identifiers are internal, so it is named here only as an unexercised artifact. The same applies to the linked decision issue and the two linked end-to-end coverage issues, whose bodies were not available to this change.
• One of the three cancel-swallow sites on this surface is deliberately left as it was. Sweeping src/ for places that suppress a cleanup cancel's failure finds three: the sync subscribe timeout and the async subscribe timeout (both fixed here), and the async subscribe's asyncio.CancelledError path (src/comfy_sdk/models.py), which still swallows every cancel failure alike, refusal included. It cannot report a detach — the caller is receiving no value from that call, and replacing their CancelledError would break the cancellation they asked for — so a task cancelled from outside still loses the request id of a run that may be billing. Reaching it needs submit rather than subscribe. Worth its own decision; a comment in the code now says so. The workflow-jobs surface has no equivalent site (Comfy.run does not cancel on timeout), so the count there is zero.
• Out of scope, per the ticket: the server-side refusal itself, the TypeScript and Go SDKs (which carry the same default and will need matching changes once the wire shape is settled), and any change to the default timeout or to making cancel-on-timeout opt-out.

Provenance

• Authored by: agent-work loop
• Verified: uv run --extra dev pytest -q: 1004 passed, 9 skipped, 0 failed (12 new tests this round); ruff check .: all checks passed; ruff format --check .: 57 files already formatted; mypy src: no issues in 21 source files; python3 scripts/check_public_repo_hygiene.py: no internal-only references; uv run --extra codegen python scripts/check_drift.py: models in sync, all 18 router error types covered, run route matches the spec. All re-run on the merged tree (main merged in at 21a7b0b), not on the branch alone.
• Deviations: the ticket's first acceptance criterion asks that the detached timeout "returns normally, does not raise" — met — while its second asks that an accepted cancel "keeps today's cancelled semantics"; that one now raises SubscribeTimeout rather than a bare TimeoutError. The subclass keeps every existing except TimeoutError working and is what carries the request id, but it is a narrowing of the exact class raised. The review round narrowed it further: an accepted cancel is no longer assumed to have stopped the run, so a 2xx answering a live status now returns a DetachedRequest where it previously raised with cancelled=True — closer to the criterion's intent (only a real cancellation reports one), but a second change to the exact outcome. No criterion was skipped; see ## Residual for what could not be exercised.

[coderabbitai]

Warning

Review paused — included plan limit reached

Keep your review moving with free on-demand reviews.

• Run this review for free

On-demand reviews are free for the next 21 days.

• Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Promotion and pricing details

On-demand reviews are free for the next 21 days. After that, they cost $0.25 per reviewed file.

Review limit details

Or wait 25 minutes for your next included review.

Check out review usage here.

Limit details: You’ve used the included review currently available. Your 139 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: Comfy-Org/comfy-python-sdk/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Team

Run ID: b8fd587c-75d6-4841-9f08-ab7da07debd4

📥 Commits

Reviewing files that changed from the base of the PR and between e4773c7 and c76ebd4.

📒 Files selected for processing (8)

• CHANGELOG.md
• README.md
• src/comfy_sdk/__init__.py
• src/comfy_sdk/model_requests.py
• src/comfy_sdk/models.py
• tests/conftest.py
• tests/test_models_queue.py
• tests/test_sync_async_parity.py

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 8 finding(s).

Severity	Count
🟠 High	1
🟡 Medium	2
🟢 Low	5

Panel: 6/6 reviewers contributed findings.

[mattmillerai]

🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:

• BE-15941 — Tell a still-queued request apart from an in-flight one in subscribe's detach report — filed as agent-spike (premise unverified)

The following carry agent-spike instead of agent-ok because their reachability claim was not backed by evidence (BE-5378) — the claim is investigated before any code is written, and "the premise does not hold" is a valid, successful outcome:

• Tell a still-queued request apart from an in-flight one in subscribe's detach report — no reachability block in the proposal