fix(jobs): emit prompt_not_found when local cancel targets an unknown id (BE-4218) - #580
fix(jobs): emit prompt_not_found when local cancel targets an unknown id (BE-4218)#580mattmillerai wants to merge 2 commits into
Conversation
… id (BE-4218)
`comfy jobs cancel <bogus-id>` on local exited 0 with an ok envelope for an
id that never existed: ComfyUI's `POST /queue {"delete": [id]}` returns 200
regardless, and the interrupt is skipped when the id isn't running, so both
sub-results read as success. An agent could not tell "cancelled" from
"already finished" from "typo'd the id".
Probe existence before mutating anything: one `GET /queue` (shared with the
interrupt gate) covers running + pending, `GET /history/<id>` covers terminal
prompts, and the local state store covers prompts we submitted that the server
has since forgotten. Known ids in any state — including terminal — keep the
documented idempotent ok and now carry a `found` field; an id that is nowhere
gets a `prompt_not_found` error envelope + exit 1, matching what the cloud
path already does with a 404. Probe failures never produce prompt_not_found:
absence of evidence is not evidence of absence.
dosubot
Bot
added
the
size:M
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 7 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 2 |
| 🟢 Low | 4 |
Panel: 6/8 reviewers contributed findings.
Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)
…tate (BE-4218) Follow-up to the prompt_not_found existence probe, addressing the cursor-review panel on #580: - The probe reused its pre-delete /queue snapshot to gate /interrupt, widening the window the gate exists to close: a prompt that goes pending->running during the delete round-trip was neither deleted nor interrupted, and a prompt that lost the running slot could get an unrelated job killed in its place. Re-read /queue immediately before interrupting; fall back to the snapshot only when that read fails (server down, where /interrupt would no-op anyway). - The final cancel_failed gate judged reachability from the same stale pre-delete probe, so a server dying mid-cancel reported a clean cancel. It now uses the post-delete read. - Cancelling an already-terminal job no longer rewrites its recorded status to 'cancelled' (jobs ls showed completed runs as cancelled), and the state file is re-read right before the write so a concurrent `jobs watch` update isn't clobbered. - An empty/whitespace id is rejected up front: quoted into the probe it became `GET /history/`, the list-all endpoint, whose non-empty body read as "found". - _http_get_json wraps JSONDecodeError (a ValueError) into RuntimeError, so a non-JSON 200 from a proxy is a probe failure, not a traceback. Six regression tests, each verified to fail against the previous commit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dosubot
Bot
added
size:L
1 participant
ELI-5
comfy jobs cancel <id>on a local server used to say "ok!" no matter what you typed. Ask it to cancel a job that never existed — a typo, a stale id — and it still exited 0 with a success envelope, because ComfyUI answersPOST /queue {"delete": [id]}with 200 for any id, and the interrupt step is skipped for anything that isn't currently running. So an agent could not tell "I cancelled it", "it had already finished", and "I made up that id" apart.Now the command looks before it leaps: it asks the server what's running/pending, checks the server's history for the id, and checks our own local state store. If the id is known anywhere — including already-finished jobs — nothing changes: you still get the documented idempotent
ok: true. If the id is nowhere, you geterror.code == "prompt_not_found"and exit 1, exactly like the cloud path already does with a 404.What changed
All in
comfy_cli/command/jobs.py,_local_cancel:GET /queuethat already existed (for the interrupt gate) now also collectsqueue_pendingids, and it moved above the delete — otherwise the delete would erase the only evidence that a pending prompt ever existed. No extra round-trip in the common case.GET /history/<id>(only when the id isn't in the queue) covers terminal prompts. ComfyUI returns{}for an unknown id and a dict keyed by the id for a known one.jobs_state.read(prompt_id)covers prompts we submitted that the server has since forgotten (restart, trimmed history).prompt_not_found+ exit 1. The code was already registered (used by the cloud path); its registry meaning ("Asked about a prompt_id the server doesn't know") already covers both paths, so no registry change.GET /queueor the history probe fails, the command falls through to the old idempotent behavior — absence of evidence is not evidence of absence. Total failure still surfacescancel_failed, unchanged.foundfield; thecancelhelp string now mentions the not-found error.Falsification of the deny path
This diff adds a user-facing "no such job" dead-end, so the premise ("nothing real can be cancelled for an id that fails all four probes") was checked against ComfyUI's own server code rather than only against tests I wrote:
POST /queuedelete →PromptQueue.delete_queue_item()scans only the pending queue and the route returnsweb.Response(status=200)unconditionally (server.pypost_queue,execution.py:1339). Confirms both the bug premise (200 for unknown ids) and that any id the delete could actually act on is inqueue_pending— which the probe reads.POST /interruptonly ever stops the running prompt (server.pypost_interrupt) — also probed, viaqueue_running.GET /history/<id>→PromptQueue.get_history(prompt_id=...)returns{prompt_id: entry}when known and{}when not (execution.py:1369-1377) — exactly the semantics the probe relies on.GET /queuesanitizes rows asitem[:5], so index 1 is still the prompt_id for both lists (server.py:67).So there is no path through the product where an id failing all four probes would have had a real cancel performed on it. (No live ComfyUI server was available on this machine to exercise it end-to-end; the evidence above is from the server source at
Comfy-Org/ComfyUIserver.py/execution.py.)Judgment calls
foundis emitted as the real boolean, not a hardcodedtrue. The ticket asked for"found": trueon the success payload. It'strueon every path that can prove existence; the one case it comes backfalseis the unreachable-probe path (ok, idempotent, but existence unproven) — which is more useful for debugging than a constant would be._cloud_cancelturns a 404 into aprompt_not_foundenvelope — the mirror claim was false, and this makes it true.test_jobs_cancel_local_pending_job_does_not_interruptcancelledprompt-pendingwhile its mockedqueue_pendingwas empty — the fixture contradicted the test's own name. It now actually lists the prompt as pending./queueread and the delete would 404. It has no state file and isn't in the queue snapshot, so it's indistinguishable from a typo; the alternative (probe after mutating) loses the evidence entirely./interruptaccepts an optionalprompt_idfor targeted interruption; comfy-cli still uses the global form gated onqueue_running. Left alone.Downstream
comfy-local-mcpneeds no change —_unwrap_envelopealready turns the error envelope into aComfyCliErrorcarryingcode="prompt_not_found", whichcancel_jobpropagates. Its docstring's claim that "cancelling an unknown prompt_id surfaces comfy-cli's error envelope" becomes true with this.Testing
pytest— 2774 passed, 37 skipped (full suite).ruff check ./ruff format --diff .clean with the CI-pinnedruff==0.15.15. (Heads-up unrelated to this PR: the versionuv syncresolves, 0.12.7, flags 15 pre-existingUP038errors in untouched files — CI's pin is the one that matters.)prompt_not_found(and asserts no POST happened before the verdict); history-only id → ok +found; state-file-only id → ok without a history probe; unreachable/queueand unreachable/history→ still idempotent ok, neverprompt_not_found. Existing running/pending/both-fail cancel tests unchanged in intent.