fix(models): read the cancel route's 202 CANCELLATION_REQUESTED per contract

[mattmillerai]

STACKED — merging lands on matt/be-14561-subscribe-timeout-detach (owned by Matt Miller, PR #158), NOT main. Do not read anything below as "ready to merge" into the default branch: #158 has to land first.

ELI-5

When subscribe(timeout=N) runs out of patience it asks the server to cancel. The server answers 202 CANCELLATION_REQUESTED — "I took your ask". The SDK had never heard of that word, so it filed it under "some status I don't recognise", detached, then polled, found the request COMPLETED with the cancelled bucket, and treated that like any other failed run: it raised Cancelled — "the model refused the request" — for a stop the SDK itself had just asked for. This teaches it the word, and makes one confirming poll decide the ending honestly.

Description

The vendored contract (spec/router-openapi.yaml, cancelRouterModelRequest) declares exactly two answers on the cancel route: a 202 naming CANCELLATION_REQUESTED, and a 409 naming ALREADY_COMPLETED. The cancellation write is guarded on the request being non-terminal and lands before the 202 is written, so a request in either live state is cancelled rather than refused — there is no in-flight 409. RouterQueueStatus is a deliberately closed enum of IN_QUEUE / IN_PROGRESS / COMPLETED.

Before this, grep CANCELLATION_REQUESTED src tests returned nothing, and the timeout teardown's reading of a 2xx cancel had no branch for it.

What changed, all in src/comfy_sdk/model_requests.py unless noted:

• The statuses are named. IN_QUEUE and IN_PROGRESS join COMPLETED in __all__ and in comfy_sdk/__init__.py — they are contract values a caller comparing QueueUpdate.status legitimately wants, and each carries the spec line it comes from and the note that the enum is closed on purpose. CANCELLATION_REQUESTED stays private (_CANCELLATION_REQUESTED): it is a value of the cancel body's own vocabulary, not a queue state, and the status route can never answer it.
• The 202 is read as "accepted, confirm". A new _CancelReading.ACCEPTED, compared against the raw string case-sensitively. STOPPED still covers the body-less 204 and COMPLETED+bucket, FINISHED still covers COMPLETED with no bucket, and UNSTOPPED still covers a 2xx echoing a live queue state.
• One confirming poll decides the ending. _detach_report (both handles) now answers four ways: COMPLETED carrying the cancelled bucket → None, the cancelled ending (SubscribeTimeout(cancelled=True)); any other completion → the update, unchanged; IN_QUEUE → a ComfyError coded cancel_not_applied; any other live status → a detach. The cancelled case deliberately does not route through _collect_or_detach, whose error_type is not None branch re-raises the typed error — right for a run that failed on its own, wrong for a stop we asked for. _is_cancelled_completion is shared by both handles so they cannot drift (tests/test_sync_async_parity.py walks both).
• IN_QUEUE is never reported as a detach. The spec's cancelled meaning pins a request cancelled in that state as "never dispatched and cannot be charged", which is the exact opposite of what a DetachedRequest claims. The invariant is enforced in _DetachedBase.__post_init__ rather than in one _detached, so it covers the sync handle, the async handle, and direct construction alike. No capability is lost by that raise: SubscribeTimeout carries .request_id and .model, so client.models.handle(model, request_id) still reaches the run exactly as the detach report's handle would have.
• The bucket-less-409 fallback stays, and now says what it is for. It has no producer today; the module comment no longer describes an in-flight refusal as "the shape the queue's state refusals have today" and instead says the route emits one 409, that it is typed, and that this clause is kept because it fails closed.
• Stub defaults follow the contract (tests/conftest.py): queue_cancel_status 200 → 202, queue_cancel_accept_status "COMPLETED" → "CANCELLATION_REQUESTED", queue_cancel_error_type "client_disconnected" → None. An accepted cancel marks the row cancelled for either accept status (the real row is terminal before the 202), and the status route answers COMPLETED + a new queue_cancelled_error_type knob (default "cancelled"). The legacy 204 knob is unchanged. One knob is new — queue_cancel_applies — because a conforming stub cannot produce the violated-write-order case at all, and that branch needs a test.
• Docs: the README endings table gains the 202 confirmation rule; the sentence "the queue honours a cancel only while the request is still waiting to be dispatched" is gone, because the server cancels IN_PROGRESS rows too and the spec says a mid-flight cancel "may still be charged". DetachedRequest, _DetachedBase.status, SubscribeTimeout.cancelled, RequestHandle.cancel and Models.subscribe lose the same premise. CHANGELOG ### Changed entry under feat!: subscribe's timeout detaches from an in-flight run instead of erroring #158's.

Every existing test that inherited a non-contract default now states the shape it is testing explicitly. IN_FLIGHT_REFUSAL is renamed UNSHIPPED_BUCKETLESS_REFUSAL and its comment says it models a hypothetical; the CANCELING fixture stays as the unknown-live-status case with a docstring that no longer claims the server sends it; and the three outcome.status == server.state.queue_pending_status assertions are now explicit IN_PROGRESS setups.

Falsification of the new deny path

Per the negative-claim rule, this diff adds a raise (cancel_not_applied) and a ValueError guard, so the premise behind them was checked against the artifact rather than assumed. Read-only, against the deployed handler and its repository layer at origin/main (f76b2f9906):

• the 202 CANCELLATION_REQUESTED is returned only on the branch where the guarded cancellation write reported rows affected — confirmed in the handler;
• that write is documented in the repository layer as transitioning "from any non-terminal state to COMPLETED carrying error_type=cancelled", i.e. the guard covers both live states — so the ticket's premise holds;
• the handler's only 409 is ALREADY_COMPLETED, reached when the guarded write matched nothing. There is no in-flight 409, confirming the fallback clause has no producer;
• the enum IN_QUEUE / IN_PROGRESS / COMPLETED is the persisted column's own enum.

Because the write precedes the 202, an IN_QUEUE row after an accepted cancel is genuinely unreachable on the shipped path — which is why it is reported as a server-side ordering violation rather than silently detached, and why it needs a stub knob to test at all. Source paths are deliberately not quoted here; this repository is public.

How has this been tested?

uv run --extra dev pytest -q — 1021 passed, 9 skipped, 0 failed. ruff check ., ruff format --check ., mypy src and scripts/check_drift.py all clean.

New coverage in tests/test_models_queue.py, sync and async each: the contract path on a queued request (202 → COMPLETED/cancelled → cancelled is True, queue_cancel_count == 1, queue_status_count == 2, queue_result_count == 0); the same wire shape on an in-flight request, with the spec's charging caveat in the docstring; 202 then a row still IN_QUEUE (cancelled is False, .cancel_error.code == "cancel_not_applied", no DetachedRequest, the request id in str(exc), and the error not chained onto a poll failure); 202 then IN_PROGRESS → DetachedRequest pinned to the enum value; 409 ALREADY_COMPLETED racing an earlier cancel → the cancelled ending rather than Cancelled; the narrow-side test that any other terminal bucket still raises its typed router error; a parametrised unit test of every 2xx cancel-body shape including a lower-case echo that must not read as the contract's accept; the IN_QUEUE-detach invariant; and the spike's exact stub reproduction, which now raises SubscribeTimeout with cancelled is True.

Documentation

README "A timeout detaches; it does not reliably cancel" and the handle.cancel() row above it; CHANGELOG ### Added and ### Changed; the docstrings listed above.

Residual

• Deviation from the plan as written, on the public-repo guardrail. The plan asked for the module comment to cite a private backend source path and line range as the only 409 the route emits. This repository is public, and the fleet's public-repo guardrail forbids private backend paths, schema or handler names in code, commits or PR text — so the comment cites the vendored spec/router-openapi.yaml instead and describes the behaviour (the write is guarded on the request being non-terminal, the 409 is reached only when the guard matched nothing). The claim is identical and was verified directly against the backend source; only the citation moved. The falsification section above names no paths for the same reason. Nothing else in this repository carried such a reference before this change, and nothing does now.
• The IN_QUEUE-after-refusal case changed behaviour too, beyond the 202. A bucket-less 409 refusal whose confirming poll finds the request still IN_QUEUE previously produced DetachedRequest(status="IN_QUEUE"); it now raises cancel_not_applied like the accepted-cancel path. That is the same invariant applied consistently and it fixes the same false billing claim, but it is a second behavioural change and worth a reviewer's eye. No shipped deployment produces that refusal, so no real caller is expected to be on this path.
• _CancelReading.ACCEPTED is never branched on. It falls through to _detach_report exactly as UNSTOPPED does, per the plan. It is distinguished because the two mean genuinely different things and the parametrised reading test pins both; a reviewer who prefers one member for both readings should say so.
• Unexercised artifacts. The cancel route's 202 and 409 are exercised only against the in-repo stub — no live or staging deployment was called, and nothing here was verified end to end on the wire. The related cloud PR the plan names measures the cancel 409 envelope end to end but does not assert this SDK's exception class, so it neither covers nor conflicts with this; it was not run or read as part of this change. The upstream investigation ticket's findings comment, which carries the original evidence, was not readable from this environment — the backend verification above was done independently against the deployed source rather than inherited from it.
• Not re-filed here: feat!: subscribe's timeout detaches from an in-flight run instead of erroring #158's detach surface itself, which this builds on and does not revisit.

Provenance

• Authored by: agent-work loop
• Verified: uv run --extra dev pytest -q: 1021 passed, 9 skipped, 0 failed; ruff check .: all checks passed; ruff format --check .: 57 files already formatted; mypy src: no issues in 21 source files; scripts/check_drift.py: 3 OK, 0 drift
• Deviations: the module comment cites the vendored spec rather than the private backend source path the plan named — public-repo guardrail; the claim itself was verified against that source and is unchanged. The IN_QUEUE invariant is enforced in _DetachedBase.__post_init__ rather than in _detached, which the plan offered as an alternative — it is strictly broader, covering both handles and direct construction.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

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

Review profile: ASSERTIVE

Plan: Team

Run ID: 0b756580-11ca-4468-a7e4-fcd38af9525d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

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

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

⚠️ Panel did not produce any findings.

Every reviewer in the matrix failed to contribute — see the panel summary for which cells errored, and the run logs for the underlying cause.

Panel: 0/6 reviewers contributed findings.

Reviewers that did not contribute: claude-opus-5-thinking-max:adversarial (error), gpt-5.6-sol-max:adversarial (error), kimi-k3-high:adversarial (error), claude-opus-5-thinking-max:edge-case (error), gpt-5.6-sol-max:edge-case (error), kimi-k3-high:edge-case (error)