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

docs/tools/workflow-execution.mdx

@@ -243,7 +243,7 @@ Inspect and manage the ComfyUI execution queue. Driven by the `action` parameter
 - action:"move" — Move a PENDING queue item to the front or back by removing it and re-enqueuing its saved workflow payload; `position` ("front"|"back") is required. The job receives a NEW prompt_id; the old prompt_id is removed. Running jobs cannot be moved.
 - action:"edit" — Edit a PENDING queue item by removing it and re-enqueuing an updated workflow. Provide either a complete replacement `workflow` or `node_inputs` patches keyed by node id; `position` selects where to requeue (default back). The job receives a NEW prompt_id; the old prompt_id is removed. Running jobs cannot be edited.
 - action:"cancel" — Stop the CURRENTLY RUNNING job ROBUSTLY. Sends an interrupt, then WAITS and verifies the job actually stopped — ComfyUI only honors interrupts BETWEEN steps, so a long single step (e.g. a high-res video sampler) can ignore a plain cancel. If the interrupt isn't honored it escalates to freeing VRAM (POST /free) and re-checks; if it STILL won't die it reports the job as WEDGED and tells you to restart_comfyui (an HTTP cancel cannot kill a stuck step). Set clear_pending:true to also drop ALL pending jobs in the same call — the correct "reset the queue" action, since cancelling alone leaves pending jobs that would run next. The partial result is discarded. With `prompt_id` given, only interrupts the running job when its prompt_id matches; omit to interrupt whatever is currently running. Use action:"cancel_queued" to remove one specific PENDING job instead.
-- action:"cancel_queued" — Remove one specific PENDING job from the queue by prompt_id. Does not affect running jobs.
+- action:"cancel_queued" — Remove one specific PENDING job from the queue by prompt_id, then VERIFY the removal against a live /queue read on both sides of it. Only PENDING jobs can be removed this way: ComfyUI silently ignores the request for a job it has already started, so if the job won the race and is now RUNNING this reports isError and tells you the outputs will still be delivered — use action:"cancel" to interrupt that. Also reports isError when the prompt_id was not in the queue at all (it already finished, or was never queued) rather than calling that a removal.
 - action:"clear" — Clear ALL pending jobs from the queue. Does not affect the currently running job.
 
 ### Parameters

src/__tests__/services/queue-manager-cancel-queued-cloud.test.ts

@@ -0,0 +1,38 @@
+import { describe, expect, it, vi } from "vitest";
+
+/**
+ * The verify-both-sides fix for #1632 must not fire in Comfy Cloud mode.
+ *
+ * `cloudClient.getQueue()` returns a HARDCODED empty queue and makes no
+ * request — there is no /queue endpoint to read. That empty result is
+ * byte-identical to the local "this prompt_id is not pending" case, so a
+ * verification built on it would call every cloud job `absent`, skip the
+ * delete, and answer with "it already finished, its outputs already exist"
+ * about a job that is genuinely queued — the same false report #1632 is
+ * about, re-aimed at cloud users, and it would also lose the CLOUD_UNSUPPORTED
+ * error that names the action that DOES work there.
+ *
+ * Deliberately mocks ONLY `isCloudMode`: the real queue-manager, real
+ * client.ts and real cloud-client.ts run, because the defect lives in exactly
+ * the seam a stubbed client would hide. (Neither cloud function called here
+ * touches the network.)
+ */
+
+vi.mock("../../config.js", async () => {
+  const actual = await vi.importActual<typeof import("../../config.js")>("../../config.js");
+  return { ...actual, isCloudMode: () => true };
+});
+
+const { cancelQueuedJob } = await import("../../services/queue-manager.js");
+
+describe("cancelQueuedJob in Comfy Cloud mode", () => {
+  it("surfaces CLOUD_UNSUPPORTED instead of verifying against the empty-queue stub", async () => {
+    await expect(cancelQueuedJob("cloud-job-1")).rejects.toMatchObject({
+      code: "CLOUD_UNSUPPORTED",
+    });
+  });
+
+  it("keeps pointing at the action that actually works in cloud mode", async () => {
+    await expect(cancelQueuedJob("cloud-job-1")).rejects.toThrow(/action:"cancel"/);
+  });
+});

src/__tests__/services/queue-manager-cancel-queued-unreachable.test.ts

@@ -0,0 +1,164 @@
+import { afterEach, describe, expect, it } from "vitest";
+import http from "node:http";
+
+/**
+ * The read has to be able to say "I could not look".
+ *
+ * `client.getQueue()` cannot. It goes through the vendored client, whose
+ * failure path resolves a document with no Running/Pending key, which the
+ * `?? []` normalizer turns into an EMPTY QUEUE — measured here against a real
+ * HTTP server for a 500, a 502 HTML proxy page, a 200 `{}` and a dead port.
+ * Nothing throws.
+ *
+ * Built on that read, the #1632 fix inverts into a worse bug than the one it
+ * replaced: with ComfyUI merely unreachable, a genuinely PENDING job reads as
+ * "not pending" → `absent` → "it already finished, its outputs already exist",
+ * and the delete is never even attempted (the old code always attempted it).
+ * A failed read AFTER the delete is the same collapse pointed the other way:
+ * an empty result looks like a successful removal and re-earns the exact
+ * "removed successfully." of #1632, now stamped `verified`.
+ *
+ * So these tests use NO client mock at all — real queue-manager, real
+ * client.ts, real fetch, real sockets. A stubbed client is precisely what
+ * hides this.
+ */
+
+const servers: http.Server[] = [];
+
+async function serve(handler: http.RequestListener): Promise<number> {
+  const server = http.createServer(handler);
+  servers.push(server);
+  await new Promise<void>((r) => server.listen(0, "127.0.0.1", () => r()));
+  return (server.address() as { port: number }).port;
+}
+
+/**
+ * `config` resolves its target once at module load, so setting COMFYUI_URL here
+ * would do nothing. setComfyuiTarget + resetClient is the documented runtime
+ * retarget path (the panel orchestrator uses it for the same reason).
+ */
+async function connect(port: number) {
+  const { setComfyuiTarget } = await import("../../config.js");
+  const client = await import("../../comfyui/client.js");
+  expect(setComfyuiTarget(`http://127.0.0.1:${port}`)).toBe(true);
+  client.resetClient();
+  return await import("../../services/queue-manager.js");
+}
+
+afterEach(async () => {
+  await Promise.all(servers.splice(0).map((s) => new Promise<void>((r) => s.close(() => r()))));
+});
+
+/** A real /queue document with one job pending — what the server really holds. */
+const PENDING_BODY = JSON.stringify({
+  queue_running: [],
+  queue_pending: [[1, "my-job", { "1": { class_type: "SaveImage", inputs: {} } }, {}, []]],
+});
+
+describe("cancelQueuedJob when /queue cannot be read", () => {
+  it("does not call a live pending job 'absent' when a proxy answers HTML", async () => {
+    const seen: string[] = [];
+    const port = await serve((req, res) => {
+      seen.push(`${req.method} ${req.url?.split("?")[0]}`);
+      if (req.url?.startsWith("/queue") && req.method === "GET") {
+        res.writeHead(502, { "content-type": "text/html" });
+        res.end("<html><body>502 Bad Gateway</body></html>");
+        return;
+      }
+      res.writeHead(200, { "content-type": "application/json" });
+      res.end("{}");
+    });
+    const { cancelQueuedJob } = await connect(port);
+
+    const result = await cancelQueuedJob("my-job");
+
+    expect(result.state).not.toBe("absent");
+    expect(result.verified).toBe(false);
+    // The delete must still be ATTEMPTED — an unreadable queue is not grounds
+    // for silently doing nothing, which is what the absent short-circuit did.
+    expect(seen).toContain("POST /queue");
+  });
+
+  it("does not report a removal it could not see after the delete", async () => {
+    let queueReads = 0;
+    const port = await serve((req, res) => {
+      if (req.url?.startsWith("/queue") && req.method === "GET") {
+        queueReads += 1;
+        if (queueReads === 1) {
+          // Before: the job really is pending.
+          res.writeHead(200, { "content-type": "application/json" });
+          res.end(PENDING_BODY);
+          return;
+        }
+        // After: the read fails. An empty answer here is what used to read as
+        // "gone, therefore removed".
+        res.writeHead(502, { "content-type": "text/html" });
+        res.end("<html><body>502 Bad Gateway</body></html>");
+        return;
+      }
+      res.writeHead(200, { "content-type": "application/json" });
+      res.end("{}");
+    });
+    const { cancelQueuedJob } = await connect(port);
+
+    const result = await cancelQueuedJob("my-job");
+
+    expect(result.verified).toBe(false);
+    expect(queueReads).toBe(2);
+  });
+
+  it("attempts the delete rather than short-circuiting when ComfyUI is down", async () => {
+    const port = await serve(() => {});
+    await new Promise<void>((r) => servers[servers.length - 1].close(() => r()));
+    servers.pop();
+    const { cancelQueuedJob } = await connect(port);
+
+    // Nothing is listening, so the delete itself fails — which is honest. What
+    // must NOT happen is a resolved `{state:"absent"}` claiming the job
+    // finished, which is what an empty-on-failure read produced.
+    await expect(cancelQueuedJob("my-job")).rejects.toThrow();
+  });
+
+  it("still verifies normally against a healthy server", async () => {
+    let queueReads = 0;
+    let deleted = false;
+    const port = await serve((req, res) => {
+      if (req.url?.startsWith("/queue") && req.method === "GET") {
+        queueReads += 1;
+        res.writeHead(200, { "content-type": "application/json" });
+        res.end(deleted ? JSON.stringify({ queue_running: [], queue_pending: [] }) : PENDING_BODY);
+        return;
+      }
+      if (req.url?.startsWith("/queue") && req.method === "POST") {
+        deleted = true;
+        res.writeHead(200, { "content-type": "application/json" });
+        res.end("{}");
+        return;
+      }
+      res.writeHead(200, { "content-type": "application/json" });
+      res.end("{}");
+    });
+    const { cancelQueuedJob } = await connect(port);
+
+    await expect(cancelQueuedJob("my-job")).resolves.toEqual({
+      removed: true,
+      state: "removed",
+      verified: true,
+    });
+    expect(queueReads).toBe(2);
+  });
+
+  it("reports a job that is genuinely absent from a healthy server", async () => {
+    const port = await serve((req, res) => {
+      res.writeHead(200, { "content-type": "application/json" });
+      res.end(JSON.stringify({ queue_running: [], queue_pending: [] }));
+    });
+    const { cancelQueuedJob } = await connect(port);
+
+    await expect(cancelQueuedJob("my-job")).resolves.toEqual({
+      removed: false,
+      state: "absent",
+      verified: true,
+    });
+  });
+});

src/__tests__/services/queue-manager-cancel-queued.test.ts

@@ -0,0 +1,173 @@
+import { beforeEach, describe, expect, it, vi } from "vitest";
+
+/**
+ * #1632: `cancel_queued` reported "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 was told it had been cancelled. The service fired the delete
+ * and returned void — the caller had nothing to branch on, so the false report
+ * was not even avoidable one level up.
+ *
+ * These tests pin the OBSERVED state: /queue is read on both sides of the
+ * delete, and the result says what the job is actually doing.
+ */
+
+vi.mock("../../config.js", async () => {
+  const actual = await vi.importActual<typeof import("../../config.js")>("../../config.js");
+  return { ...actual, isCloudMode: () => false };
+});
+
+const getQueueVerifiedMock = vi.fn();
+const deleteQueueItemMock = vi.fn();
+
+vi.mock("../../comfyui/client.js", () => ({
+  getClient: vi.fn(),
+  getHistory: vi.fn(),
+  getQueue: vi.fn(),
+  getQueueVerified: (...args: unknown[]) => getQueueVerifiedMock(...args),
+  interrupt: vi.fn(),
+  deleteQueueItem: (...args: unknown[]) => deleteQueueItemMock(...args),
+  clearQueue: vi.fn(),
+  enqueuePrompt: vi.fn(),
+  freeMemory: vi.fn(),
+}));
+
+import { cancelQueuedJob } from "../../services/queue-manager.js";
+import type { QueueStatus } from "../../comfyui/types.js";
+
+const TARGET = "35ba95d3-8f1b-4312-b0e7-567fc81af8f5";
+const OTHER = "3d3e4162-0000-4000-8000-000000000000";
+
+const workflow = { "1": { class_type: "SaveImage", inputs: {} } };
+
+function queue(running: string[], pending: string[]): QueueStatus {
+  return {
+    queue_running: running.map((id, i) => [i, id, workflow, {}, []]),
+    queue_pending: pending.map((id, i) => [100 + i, id, workflow, {}, []]),
+  } as QueueStatus;
+}
+
+beforeEach(() => {
+  getQueueVerifiedMock.mockReset();
+  deleteQueueItemMock.mockReset();
+  deleteQueueItemMock.mockResolvedValue(undefined);
+});
+
+describe("cancelQueuedJob reports the state it observed", () => {
+  it("removes a pending job and confirms it is gone", async () => {
+    getQueueVerifiedMock
+      .mockResolvedValueOnce(queue([OTHER], [TARGET]))
+      .mockResolvedValueOnce(queue([OTHER], []));
+
+    await expect(cancelQueuedJob(TARGET)).resolves.toEqual({
+      removed: true,
+      state: "removed",
+      verified: true,
+    });
+    expect(deleteQueueItemMock).toHaveBeenCalledWith(TARGET);
+  });
+
+  /**
+   * The exact #1632 timeline: the job is pending when we look, ComfyUI starts
+   * it, and the delete lands too late. Before the fix this returned void and
+   * the tool said "removed successfully."
+   */
+  it("does NOT claim a removal for a job that started between the check and the delete", async () => {
+    getQueueVerifiedMock
+      .mockResolvedValueOnce(queue([OTHER], [TARGET]))
+      .mockResolvedValueOnce(queue([TARGET], []));
+
+    await expect(cancelQueuedJob(TARGET)).resolves.toEqual({
+      removed: false,
+      state: "running",
+      verified: true,
+    });
+    // The delete WAS issued — it just did not do anything, which is the point.
+    expect(deleteQueueItemMock).toHaveBeenCalledWith(TARGET);
+  });
+
+  it("does not even issue the delete for a job already running", async () => {
+    getQueueVerifiedMock.mockResolvedValue(queue([TARGET], [OTHER]));
+
+    await expect(cancelQueuedJob(TARGET)).resolves.toEqual({
+      removed: false,
+      state: "running",
+      verified: true,
+    });
+    expect(deleteQueueItemMock).not.toHaveBeenCalled();
+  });
+
+  it("reports a prompt_id that is not in the queue as absent, not removed", async () => {
+    getQueueVerifiedMock.mockResolvedValue(queue([OTHER], ["someone-else"]));
+
+    await expect(cancelQueuedJob(TARGET)).resolves.toEqual({
+      removed: false,
+      state: "absent",
+      verified: true,
+    });
+    expect(deleteQueueItemMock).not.toHaveBeenCalled();
+  });
+
+  it("reports a delete that did not take effect as still pending", async () => {
+    getQueueVerifiedMock
+      .mockResolvedValueOnce(queue([OTHER], [TARGET]))
+      .mockResolvedValueOnce(queue([OTHER], [TARGET]));
+
+    await expect(cancelQueuedJob(TARGET)).resolves.toEqual({
+      removed: false,
+      state: "pending",
+      verified: true,
+    });
+  });
+
+  /**
+   * An unreadable /queue must not block a removal the caller can still make —
+   * but it costs `verified`, so the caller discloses that "removed" is the
+   * delete returning rather than something we saw.
+   */
+  it("still removes when /queue cannot be read, but marks the result unverified", async () => {
+    getQueueVerifiedMock.mockRejectedValue(new Error("ECONNREFUSED"));
+
+    await expect(cancelQueuedJob(TARGET)).resolves.toEqual({
+      removed: true,
+      state: "removed",
+      verified: false,
+    });
+    expect(deleteQueueItemMock).toHaveBeenCalledWith(TARGET);
+  });
+
+  it("marks the result unverified when only the after-read fails", async () => {
+    getQueueVerifiedMock
+      .mockResolvedValueOnce(queue([OTHER], [TARGET]))
+      .mockRejectedValueOnce(new Error("ECONNREFUSED"));
+
+    await expect(cancelQueuedJob(TARGET)).resolves.toMatchObject({
+      removed: true,
+      verified: false,
+    });
+  });
+
+  /**
+   * With no before-read there is nothing to distinguish "we removed it" from
+   * "it was never queued", so an absent job after the delete is not a verified
+   * removal — that would be the same false report in a narrower window.
+   */
+  it("marks the result unverified when only the before-read fails", async () => {
+    getQueueVerifiedMock
+      .mockRejectedValueOnce(new Error("ECONNREFUSED"))
+      .mockResolvedValueOnce(queue([OTHER], []));
+
+    await expect(cancelQueuedJob(TARGET)).resolves.toMatchObject({
+      removed: true,
+      verified: false,
+    });
+  });
+
+  it("propagates a failed delete instead of reporting a removal", async () => {
+    getQueueVerifiedMock.mockResolvedValue(queue([OTHER], [TARGET]));
+    deleteQueueItemMock.mockRejectedValueOnce(new Error("HTTP 500"));
+
+    await expect(cancelQueuedJob(TARGET)).rejects.toThrow("HTTP 500");
+  });
+});