[PR #172] [MERGED] fix(models): read the cancel route's 202 CANCELLATION_REQUESTED per contract #172

New issue

Closed

opened 2026-10-31 22:02:01 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 22:02:01 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-python-sdk/pull/172
Author: @mattmillerai
Created: 9/19/2026
Status: ✅ Merged
Merged: 9/20/2026
Merged by: @mattmillerai

Base: matt/be-14561-subscribe-timeout-detach ← Head: matt/be-15945-cancel-202-cancellation-requested

📝 Commits (1)

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

📊 Changes

7 files changed (+812 additions, -180 deletions)

View changed files

📝 CHANGELOG.md (+15 -3)
📝 README.md (+33 -18)
📝 src/comfy_sdk/__init__.py (+4 -0)
📝 src/comfy_sdk/model_requests.py (+285 -88)
📝 src/comfy_sdk/models.py (+29 -17)
📝 tests/conftest.py (+43 -22)
📝 tests/test_models_queue.py (+403 -32)

📄 Description

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 #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: #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.

_{🔄 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-python-sdk/pull/172 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/19/2026 **Status:** ✅ Merged **Merged:** 9/20/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `matt/be-14561-subscribe-timeout-detach` ← **Head:** `matt/be-15945-cancel-202-cancellation-requested` --- ### 📝 Commits (1) - [`aeeb082`](https://github.com/Comfy-Org/comfy-python-sdk/commit/aeeb08225d3adef0793596d0b4ea842a51f2a25f) fix(models): read the cancel route's 202 CANCELLATION_REQUESTED per contract ### 📊 Changes **7 files changed** (+812 additions, -180 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+15 -3) 📝 `README.md` (+33 -18) 📝 `src/comfy_sdk/__init__.py` (+4 -0) 📝 `src/comfy_sdk/model_requests.py` (+285 -88) 📝 `src/comfy_sdk/models.py` (+29 -17) 📝 `tests/conftest.py` (+43 -22) 📝 `tests/test_models_queue.py` (+403 -32) </details> ### 📄 Description **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 #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:** #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. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 22:02:01 +00:00