Skip to content

fix(1632): cancel_queued reports the removal it observed, not the one it requested - #1636

Draft
artokun wants to merge 3 commits into
mainfrom
fix/1632-cancel-queued-verify
Draft

fix(1632): cancel_queued reports the removal it observed, not the one it requested#1636
artokun wants to merge 3 commits into
mainfrom
fix/1632-cancel-queued-verify

Conversation

@artokun

@artokun artokun commented Aug 16, 2026

Copy link
Copy Markdown
Owner

Fixes #1632.

The bug

queue action:"cancel_queued" said Queued job <id> removed successfully. unconditionally. ComfyUI's /queue delete silently no-ops for a prompt it has already started, so a job that won the race kept rendering and still delivered its outputs while the agent told the user it had been cancelled. In the reporter's session the job started ~1s before the cancel landed, and four stale images arrived 70s after the "success".

Two unconditional paths, and the first made the second unavoidable:

  • cancelQueuedJob() returned Promise<void> — it fired the delete and read nothing. The caller had nothing to branch on.
  • The cancel_queued case therefore hardcoded the success string.

This is the one queue mutation that never got the verify-then-report contract action:"cancel" right above it already has.

The fix

cancelQueuedJob now reads /queue on both sides of the delete and returns { removed, state, verified }. The tool returns isError:true for any removed:false, with prose naming what the job is actually doing — for running, explicitly that the outputs will still be delivered and that action:"cancel" is the tool for it.

The verification needed a read that can FAIL

The first version of this fix built the check on client.getQueue(), and that was worse than the bug. Measured against a real HTTP server, getQueue() never throws: it goes through the vendored client, whose failure path resolves a document with no Running/Pending key, and the ?? [] normalizer turns that into an empty queue.

/queue answers getQueue() resolves
500 {queue_running:[],queue_pending:[]}
502 HTML proxy page {queue_running:[],queue_pending:[]}
200 {} {queue_running:[],queue_pending:[]}
dead port (ECONNREFUSED) {queue_running:[],queue_pending:[]}

So "this job is not pending" and "I could not look" were the same value. With ComfyUI merely unreachable, a genuinely pending job read as absent"it already finished, its outputs already exist" — the #1632 lie with a new cause — and the delete was never even attempted, which the old code at least always did. A failed after-read was the same collapse pointed the other way: empty looks like a successful removal, re-earning removed successfully. now stamped verified:true.

This adds getQueueVerified() in client.ts — same normalization, but over the guarded JSON path (comfyApiFetch + readComfyJson + expectShape) that every other verifying read here uses. Network errors, non-2xx, HTML, and a JSON body that is not a queue document all throw. null in cancelQueuedJob now means ignorance and nothing else: it never decides an early return, never skips the delete, and never counts toward verified.

getQueue() is unchanged — collapsing is fine for a summary, where an unreachable ComfyUI genuinely has no jobs to list.

Comfy Cloud

cloudClient.getQueue() returns a hardcoded empty queue with no network call — there is no /queue endpoint. Verifying against it would report every cloud job as absent and swallow the delete, replacing a correct CLOUD_UNSUPPORTED error that names action:"cancel" with a false one. Cloud mode goes straight to the delete, exactly as before.

Proof

Deleted-fix verified, not just green. Five mutations against committed code:

mutation result
restore the unconditional success string at the tool layer 4 tool tests fail
delete the after-delete read 3 service tests fail, incl. the exact #1632 race
delete the before-delete read 5 service tests fail
delete the cloud guard 2 cloud tests fail ({removed:false,state:"absent"} — the exact regression)
swap getQueueVerified() back to getQueue() 3 unreachable-server tests fail

queue-manager-cancel-queued-unreachable.test.ts mocks nothing — real queue-manager, real client.ts, real fetch, real sockets, retargeted via setComfyuiTarget. A stubbed client is exactly what hid this defect: the first version's suite was green over it by construction.

Full suite: 519 files, 9700 passed, 3 skipped. tsc --noEmit, check:vocabulary (1464 files), i18n:check, check:docs-links, check:docs-locale all clean.

Two files (late-mutation-e2e, ui-bridge) failed once under parallel load and pass in isolation (212 tests) and on a clean re-run — the documented wall-clock flake, not queue-related.

Codex gate

codex was unavailable (account exhausted until 2026-08-19); gated by an independent adversarial review instead — .claude/autopilot/claude-gate.mjs, which spawns a fresh reviewer that has not seen this reasoning. It returned NO-SHIP twice, and both findings were real and are fixed above: the Comfy Cloud stub (round 1) and the never-throwing /queue read (round 2, P0). Verdicts recorded in comments below.

Observed, deliberately not fixed

  • cancelRunningJobEscalating (action:"cancel") has the same collapsing read. It verifies via getQueueSummary()getQueue(), so an unreachable ComfyUI reports an empty queue and its "nothing is running" check can settle on that. Pre-existing, a different action, and out of scope for queue cancel_queued reports "removed successfully" for a job that is already running (and still completes) #1632 — flagged here rather than widened into this diff.
  • clearAllQueued / action:"clear" — the reporter suggested it as the same shape. It isn't: its message is already scoped to pending items, and a job that started running is no longer a pending item, so nothing in that sentence is falsified by the race.
  • Residual race: a job that starts and completes between the delete and the after-read still reads as removed. Real, but the window is one HTTP round trip and closing it costs a /history read on every cancel.
  • Browser verification — not applicable and not run. cancel_queued is orchestrator-only; grep -rn "cancel_queued" over comfyui-mcp-panel returns zero hits.

🤖 Generated with Claude Code

Copilot AI balanced review requested due to automatic review settings August 16, 2026 01:12

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

queue cancel_queued reports "removed successfully" for a job that is already running (and still completes)

2 participants