From b3751d9f847350a94d5bc59a631357990f394d94 Mon Sep 17 00:00:00 2001 From: glary Date: Sun, 4 Oct 2026 07:38:36 +0000 Subject: [PATCH 01/22] feat(run): add --client-id so a run can submit as an already-connected client ComfyUI addresses a prompt's execution events only to the socket whose clientId submitted it. An agent that runs a workflow on the user's behalf therefore leaves the user's canvas dark: no node highlights, no progress, no outputs (PM-1875). --client-id submits under a caller-supplied clientId instead of a freshly minted one, so those events reach the client that is actually watching. Borrowing a LIVE client's id makes ComfyUI's session-resume behaviour a hazard: its /ws handler pops any socket already registered under an incoming clientId, so anything that reconnects as the borrowed id would silence the client the flag exists to feed. Both attach paths are closed: - --wait is refused outright (client_id_rejected), and connect() raises if it is ever reached another way; - the run is recorded with client_id_borrowed, and jobs watch declines to re-attach to a borrowed id, degrading to /history instead. The detached watcher is poll-only, so it is unaffected. --client-id is local-only; cloud fans execution events out per user/workspace regardless of submitter. --- comfy_cli/cmdline.py | 35 ++++ comfy_cli/command/jobs.py | 7 + comfy_cli/command/run/__init__.py | 4 + comfy_cli/command/run/execution.py | 12 +- comfy_cli/error_codes.py | 10 + comfy_cli/jobs_state.py | 9 +- tests/comfy_cli/command/test_run_client_id.py | 176 ++++++++++++++++++ 7 files changed, 251 insertions(+), 2 deletions(-) create mode 100644 tests/comfy_cli/command/test_run_client_id.py diff --git a/comfy_cli/cmdline.py b/comfy_cli/cmdline.py index dc8bf06f6..88e3600da 100644 --- a/comfy_cli/cmdline.py +++ b/comfy_cli/cmdline.py @@ -1206,6 +1206,20 @@ def run( ), ), ] = False, + client_id: Annotated[ + str | None, + typer.Option( + "--client-id", + show_default=False, + help=( + "Local-only: submit as this already-connected WebSocket clientId instead of a fresh " + "one. ComfyUI sends a prompt's execution events only to the socket that submitted it, " + "so passing a browser tab's clientId is what makes that tab show node highlights, " + "progress and outputs for a run it did not start. This invocation then cannot watch " + "the run itself, so it cannot be combined with --wait." + ), + ), + ] = None, ): # Snapshot kwargs before the body mutates api_key/host/port — analytics should record what user actually supplied. _track_props = tracking.filter_command_kwargs(dict(locals())) @@ -1251,6 +1265,26 @@ def run( # `where` kwarg is None then). Rides on the execution_success/_error events. _track_props["target"] = "cloud" if decision.target is where_module.WhereTarget.CLOUD else "local" + client_id = (client_id or "").strip() or None + if client_id: + rejection = None + if decision.target is where_module.WhereTarget.CLOUD: + rejection = ("cloud", "--client-id addresses a local ComfyUI socket and has no meaning on cloud") + elif wait: + rejection = ( + "wait", + "--client-id hands this run's events to another client, so --wait would watch a silent socket", + ) + if rejection is not None: + reason, message = rejection + renderer.error( + code="client_id_rejected", + message=message, + hint="drop --client-id, or drop the conflicting flag", + details={"reason": reason}, + ) + raise typer.Exit(code=1) + # Default for --notify: on when a human is at the terminal, off for # agents (they shouldn't get surprise side-channel processes they didn't # ask for). The user can override either way with --notify/--no-notify. @@ -1348,6 +1382,7 @@ def run( print_prompt=print_prompt, preloaded=preloaded, allow_spend=allow_spend, + client_id=client_id, ) except typer.Exit as e: if (e.exit_code or 0) == 0: diff --git a/comfy_cli/command/jobs.py b/comfy_cli/command/jobs.py index d9efae5d7..47f4d916a 100644 --- a/comfy_cli/command/jobs.py +++ b/comfy_cli/command/jobs.py @@ -1879,6 +1879,11 @@ def _resolve_watch_client_id(host: str, port: int, prompt_id: str) -> str | None still holding that socket (a browser tab, a blocking ``comfy run``) stops receiving events until it reconnects; pass ``--client-id`` to override the resolution when that matters. + + A run submitted via ``comfy run --client-id`` is the one case where that + caveat is a certainty rather than a risk: the recorded id belongs to a live + client that is being fed on purpose. Those resolve to None so the watch + degrades to ``/history`` rather than taking the client's socket away. """ from comfy_cli import jobs_state @@ -1886,6 +1891,8 @@ def _resolve_watch_client_id(host: str, port: int, prompt_id: str) -> str | None job = jobs_state.read(prompt_id) except (ValueError, OSError): # unsafe prompt_id / unreadable state dir job = None + if job is not None and job.client_id_borrowed: + return None if job is not None and isinstance(job.client_id, str) and job.client_id.strip(): return job.client_id diff --git a/comfy_cli/command/run/__init__.py b/comfy_cli/command/run/__init__.py index cce9f165a..5994ed44d 100644 --- a/comfy_cli/command/run/__init__.py +++ b/comfy_cli/command/run/__init__.py @@ -215,6 +215,7 @@ def execute( print_prompt: bool = False, preloaded: tuple[dict, str, bool, bool] | None = None, allow_spend: bool = False, + client_id: str | None = None, ): # `0.0.0.0` is a wildcard bind, not a connect address. macOS / Windows # clients can't reach it; on Linux it happens to resolve to a loopback. @@ -437,6 +438,7 @@ def execute( local_paths, timeout, extra_data=extra_data, + client_id=client_id, ) # Wire SIGINT → close the WebSocket so the loop exits promptly. token = cancellation.get_token() @@ -498,6 +500,7 @@ def execute( where="local", host=host, port=port, + client_id_borrowed=execution.borrowed_client_id, ) wait_state.item_map = (compose_meta or {}).get("items") wait_state.status = "running" @@ -581,6 +584,7 @@ def execute( where="local", host=host, port=port, + client_id_borrowed=execution.borrowed_client_id, ) state.item_map = (compose_meta or {}).get("items") state_file = jobs_state.write(state) diff --git a/comfy_cli/command/run/execution.py b/comfy_cli/command/run/execution.py index 3b0273ece..f233f3629 100644 --- a/comfy_cli/command/run/execution.py +++ b/comfy_cli/command/run/execution.py @@ -128,12 +128,17 @@ def __init__( *, extra_data: dict | None = None, api_key: str | None = None, + client_id: str | None = None, ): self.workflow = workflow self.host = host self.port = port self.verbose = verbose - self.client_id = str(uuid.uuid4()) + # ComfyUI's /ws handler pops any socket already registered under an + # incoming clientId, so opening one with a BORROWED id would evict the + # client we are submitting on behalf of. Callers gate on this flag. + self.borrowed_client_id = bool(client_id) + self.client_id = client_id or str(uuid.uuid4()) self.outputs: list = [] # Node-keyed companion to the flat `outputs` URLs — one # {"node_id", "url", "filename", "type"} entry per recorded URL, in @@ -177,6 +182,11 @@ def __init__( self.last_error: dict | None = None def connect(self): + if self.borrowed_client_id: + raise RuntimeError( + f"refusing to open a websocket as borrowed clientId {self.client_id!r}: " + "it would evict the client this run submits on behalf of" + ) # Resolve via the package namespace so tests can patch # ``comfy_cli.command.run.WebSocket`` and have it take effect here. from comfy_cli.command import run as _run_pkg diff --git a/comfy_cli/error_codes.py b/comfy_cli/error_codes.py index 0f2be4ecb..f878cc377 100644 --- a/comfy_cli/error_codes.py +++ b/comfy_cli/error_codes.py @@ -307,6 +307,16 @@ class ErrorCode: "pass `--host ` and `--port 1-65535`; if you passed neither, the saved " "background server is bad — clear it with `comfy stop`", ), + ErrorCode( + "client_id_rejected", + "`--client-id` was combined with something it cannot serve. The flag submits the prompt as an " + "ALREADY-CONNECTED client (normally a browser tab) so ComfyUI addresses the run's execution " + "events to that client instead of this invocation. It is therefore local-only, and this " + "invocation cannot also watch the run: ComfyUI's `/ws` handler evicts any socket already " + "registered under an incoming clientId, so opening one here would silence the very client the " + "flag exists to feed. `details.reason` is `wait` or `cloud`.", + "drop `--client-id` to watch the run from the CLI, or drop `--wait` and poll `comfy jobs status`", + ), ErrorCode( "host_flag_cloud", "`--host`/`--port` were combined with an effective `cloud` target. They address a local " diff --git a/comfy_cli/jobs_state.py b/comfy_cli/jobs_state.py index acb4b24d2..0feface4d 100644 --- a/comfy_cli/jobs_state.py +++ b/comfy_cli/jobs_state.py @@ -25,7 +25,8 @@ "watcher_pid": | null, "watcher_pid_create_time": | null, "record": {} | null, - "item_map": {: {"nodes": [...], "save_node": "...", "prefix": "..."}} | null + "item_map": {: {"nodes": [...], "save_node": "...", "prefix": "..."}} | null, + "client_id_borrowed": true | false } ``record`` is the node-keyed history record stashed when a cloud job reaches @@ -142,6 +143,10 @@ class JobState: # foreach item -> {"nodes": [...], "save_node": ..., "prefix": ...} map, # written at submit time by `comfy run` for composed workflows. item_map: dict[str, Any] | None = None + # True when `client_id` was borrowed from a live client via + # `comfy run --client-id` rather than minted for this run. Re-attaching to a + # borrowed id would evict that client's socket, so watchers must not. + client_id_borrowed: bool = False @property def is_terminal(self) -> bool: @@ -287,6 +292,7 @@ def new( host: str | None = None, port: int | None = None, base_url: str | None = None, + client_id_borrowed: bool = False, ) -> JobState: """Build a fresh JobState in ``queued`` status. Call ``write()`` to persist.""" now = _now_iso() @@ -301,4 +307,5 @@ def new( submitted_at=now, updated_at=now, status="queued", + client_id_borrowed=client_id_borrowed, ) diff --git a/tests/comfy_cli/command/test_run_client_id.py b/tests/comfy_cli/command/test_run_client_id.py new file mode 100644 index 000000000..e2613cc31 --- /dev/null +++ b/tests/comfy_cli/command/test_run_client_id.py @@ -0,0 +1,176 @@ +"""`comfy run --client-id` — submitting as an already-connected client. + +ComfyUI addresses a prompt's execution events to the socket whose clientId +submitted it. Borrowing a live client's id is how an out-of-band submitter +(the in-app agent) gets that client's canvas to light up. The same mechanism +is a footgun in reverse: ComfyUI's `/ws` handler pops any socket already +registered under an incoming clientId, so anything here that reconnects as a +borrowed id would silence the client the flag exists to feed. These tests pin +both halves. +""" + +from __future__ import annotations + +import json +import os +import tempfile +import uuid +from unittest.mock import MagicMock, patch + +import pytest +import typer + +from comfy_cli import jobs_state +from comfy_cli.cmdline import run as run_command +from comfy_cli.command.jobs import _resolve_watch_client_id +from comfy_cli.command.run import WorkflowExecution, execute +from comfy_cli.output import Renderer, set_renderer +from comfy_cli.output.renderer import OutputMode, reset_renderer_for_testing + +BORROWED = "32ef0f50a7dc41dc8ab9e0107eedb88e" + + +@pytest.fixture(autouse=True) +def ndjson_renderer(): + renderer = Renderer(mode=OutputMode.NDJSON, command="run") + set_renderer(renderer) + yield renderer + reset_renderer_for_testing() + + +@pytest.fixture +def simple_workflow(): + return { + "1": { + "class_type": "EmptyLatentImage", + "inputs": {"width": 64, "height": 64, "batch_size": 1}, + }, + "2": { + "class_type": "SaveImage", + "inputs": {"filename_prefix": "x", "images": ["1", 0]}, + }, + } + + +@pytest.fixture +def workflow_file(simple_workflow): + with tempfile.NamedTemporaryFile(mode="w", suffix=".json", delete=False) as f: + json.dump(simple_workflow, f) + path = f.name + yield path + os.unlink(path) + + +def _envelope(out: str) -> dict: + lines = [json.loads(ln) for ln in out.splitlines() if ln.strip()] + assert lines, "expected at least one NDJSON line" + assert lines[-1]["type"] == "envelope", lines[-1] + return lines[-1] + + +class TestWorkflowExecutionClientId: + def test_borrowed_id_is_used_verbatim(self, simple_workflow): + ex = WorkflowExecution(simple_workflow, "127.0.0.1", 8188, False, None, False, 30, client_id=BORROWED) + assert ex.client_id == BORROWED + assert ex.borrowed_client_id is True + + def test_absent_id_is_minted(self, simple_workflow): + ex = WorkflowExecution(simple_workflow, "127.0.0.1", 8188, False, None, False, 30) + assert uuid.UUID(ex.client_id) + assert ex.borrowed_client_id is False + + def test_borrowed_id_rides_the_prompt_post(self, simple_workflow): + ex = WorkflowExecution(simple_workflow, "127.0.0.1", 8188, False, None, False, 30, client_id=BORROWED) + with patch("comfy_cli.command.run.execution.no_redirect_urlopen") as mock_open: + mock_open.return_value.__enter__.return_value.read.return_value = json.dumps({"prompt_id": "p"}).encode() + ex.queue() + body = json.loads(mock_open.call_args[0][0].data.decode()) + assert body["client_id"] == BORROWED + + def test_connect_refuses_to_evict_the_borrowed_client(self, simple_workflow): + ex = WorkflowExecution(simple_workflow, "127.0.0.1", 8188, False, None, False, 30, client_id=BORROWED) + with pytest.raises(RuntimeError, match="borrowed clientId"): + ex.connect() + + +class TestExecuteRecordsBorrowedId: + def test_async_run_persists_the_borrowed_flag(self, workflow_file, capsys, tmp_path): + written: list[jobs_state.JobState] = [] + + def capture(state): + written.append(state) + return tmp_path / f"{state.prompt_id}.json" + + with ( + patch("comfy_cli.command.run.check_comfy_server_running", return_value=True), + patch("comfy_cli.command.run._fetch_object_info", return_value={}), + patch("comfy_cli.command.run._preflight_validate"), + patch("comfy_cli.command.run._journal_run"), + patch("comfy_cli.command.run._spawn_watcher", return_value=True), + patch("comfy_cli.command.run._tail_state_file"), + patch("comfy_cli.jobs_state.write", side_effect=capture), + patch("comfy_cli.http._AUTHED_OPENER.open") as mock_open, + ): + mock_open.return_value.__enter__.return_value.read.return_value = json.dumps({"prompt_id": "p"}).encode() + execute( + workflow_file, + "127.0.0.1", + 8188, + wait=False, + timeout=30, + client_id=BORROWED, + ) + + assert written, "expected a job state file write" + state = written[0] + assert state.client_id == BORROWED + assert state.client_id_borrowed is True + assert _envelope(capsys.readouterr().out)["data"]["client_id"] == BORROWED + + +class TestCliRejections: + @pytest.mark.parametrize( + ("kwargs", "reason"), + [ + ({"wait": True, "where": "local"}, "wait"), + ({"wait": False, "where": "cloud"}, "cloud"), + ], + ) + def test_conflicting_flags_are_refused(self, workflow_file, capsys, kwargs, reason): + with ( + patch("comfy_cli.cmdline.tracking", MagicMock()), + pytest.raises(typer.Exit) as exc, + ): + run_command(workflow=workflow_file, client_id=BORROWED, **kwargs) + assert exc.value.exit_code == 1 + env = _envelope(capsys.readouterr().out) + assert env["ok"] is False + assert env["error"]["code"] == "client_id_rejected" + assert env["error"]["details"]["reason"] == reason + + +class TestWatchDoesNotStealTheSocket: + def test_borrowed_record_resolves_to_no_client_id(self, monkeypatch): + state = jobs_state.new( + prompt_id="p", + client_id=BORROWED, + workflow="w.json", + where="local", + host="127.0.0.1", + port=8188, + client_id_borrowed=True, + ) + monkeypatch.setattr(jobs_state, "read", lambda _: state) + assert _resolve_watch_client_id("127.0.0.1", 8188, "p") is None + + def test_minted_record_still_resolves(self, monkeypatch): + state = jobs_state.new( + prompt_id="p", + client_id="cli-minted", + workflow="w.json", + where="local", + host="127.0.0.1", + port=8188, + ) + monkeypatch.setattr(jobs_state, "read", lambda _: state) + assert _resolve_watch_client_id("127.0.0.1", 8188, "p") == "cli-minted" From 587d0e0648723f5c5b2c3689321469a9b660d1ae Mon Sep 17 00:00:00 2001 From: glary Date: Sun, 4 Oct 2026 08:21:50 +0000 Subject: [PATCH 02/22] fix(run): redact client_id from telemetry; make withheld watches honest Review follow-ups on --client-id: - A ComfyUI clientId is capability-bearing (it both receives a client's execution events and evicts its socket), but tracking's parameter snapshot shipped it verbatim on execution_start/_success/_error. Add it to _SENSITIVE_EXACT so it is redacted and still counted. - jobs watch told a user whose run was deliberately withheld that it "could not resolve the submitting client_id" and suggested passing --client-id, which performs exactly the eviction the guard prevents. Distinguish withheld from unresolvable and say the right thing. - A withheld watch holds a fresh-id socket nothing will ever address, so it sat out the full --timeout before its first /history read. Reconcile on a 2s cadence instead. - Record that the borrowed guard only reaches as far as our own state file: /queue and /history hand the id back with no borrowed marker. - Document client_id_rejected in docs/json-output.md and the flag in CHANGELOG.md. --- CHANGELOG.md | 9 ++++ comfy_cli/command/jobs.py | 46 +++++++++++++++++-- comfy_cli/tracking.py | 5 ++ docs/json-output.md | 1 + tests/comfy_cli/command/test_run_client_id.py | 35 +++++++++++++- 5 files changed, 92 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 42540738a..47a5db8d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,15 @@ history. validate said the graph was valid. - `comfy workflow validate --full-options` and `comfy nodes show --all-choices` (also on `nodes search --expand-top`) list a long option list in full. +- `comfy run --client-id ` submits as an already-connected WebSocket client + instead of a freshly minted one, so ComfyUI addresses the run's execution + events — node highlights, progress, outputs — to that client. This is what + lets a tool run a workflow on a user's behalf and still have the user's open + canvas light up. Local only, and refused with `client_id_rejected` alongside + `--wait` or `--where cloud`: ComfyUI evicts whatever socket already holds an + incoming clientId, so this invocation must not attach as the borrowed one. + `comfy jobs watch` declines to re-attach to such a run for the same reason + and polls status instead. ### Changed diff --git a/comfy_cli/command/jobs.py b/comfy_cli/command/jobs.py index 47f4d916a..b51cc1652 100644 --- a/comfy_cli/command/jobs.py +++ b/comfy_cli/command/jobs.py @@ -1861,6 +1861,30 @@ def _client_id_from_extra_data(extra: Any) -> str | None: return None +def _client_id_withheld(prompt_id: str) -> bool: + """True if this prompt's client_id is one we deliberately refuse to attach + as — a run submitted with ``comfy run --client-id``. + + Distinguishes the two ways ``_resolve_watch_client_id`` returns None, which + want opposite advice: an unresolvable id is worth passing ``--client-id`` + for, a withheld one must never be. + """ + from comfy_cli import jobs_state + + try: + job = jobs_state.read(prompt_id) + except (ValueError, OSError): + return False + return job is not None and job.client_id_borrowed + + +# How long a withheld watch waits on its (deliberately eventless) socket before +# reconciling against /history. The ordinary `--timeout` is a silence budget for +# a socket that SHOULD be receiving events; here nothing will ever arrive, so it +# would just be dead air before the first status read. +_WITHHELD_WATCH_POLL_S = 2.0 + + def _resolve_watch_client_id(host: str, port: int, prompt_id: str) -> str | None: """Find the ``client_id`` this prompt was submitted with, or None. @@ -1883,7 +1907,13 @@ def _resolve_watch_client_id(host: str, port: int, prompt_id: str) -> str | None A run submitted via ``comfy run --client-id`` is the one case where that caveat is a certainty rather than a risk: the recorded id belongs to a live client that is being fed on purpose. Those resolve to None so the watch - degrades to ``/history`` rather than taking the client's socket away. + polls status instead of taking the client's socket away. + + That guard reaches only as far as our own state file. Steps 2 and 3 read the + id back out of the SERVER, which records no borrowed marker, so a watch that + cannot see the submitting run's state — another machine, another config + root, a cleared state dir — still resolves a borrowed id and attaches as it. + Closing that needs a server-side signal ComfyUI does not currently emit. """ from comfy_cli import jobs_state @@ -2222,7 +2252,11 @@ def watch_cmd( token = cancellation.get_token() token.on_cancel(lambda: _safe_close_ws(ws)) - ws.settimeout(timeout) + withheld = attached_client_id is None and _client_id_withheld(prompt_id) + # A withheld watch holds a fresh-id socket the server will never address, so + # /history is the only thing that can move it. Reconcile on the poll cadence + # rather than sitting out the full silence budget first. + ws.settimeout(min(timeout, _WITHHELD_WATCH_POLL_S) if withheld else timeout) state = _WatchState(renderer=renderer, prompt_id=prompt_id, host=h, port=p) saw_any_event = False @@ -2231,7 +2265,13 @@ def watch_cmd( if renderer.is_pretty(): renderer.console().print(f"[bold]Watching prompt[/bold] {prompt_id} on {h}:{p} [dim](Ctrl-C to stop)[/dim]") - if attached_client_id is None: + if withheld: + renderer.console().print( + "[yellow]![/yellow] [dim]this run was submitted for another client (comfy run " + "--client-id), which is still receiving its live events; attaching as it would cut " + "it off, so this watch polls status instead[/dim]" + ) + elif attached_client_id is None: renderer.console().print( "[yellow]![/yellow] [dim]could not resolve the submitting client_id — the server " "may not send live events for this prompt; pass --client-id if you know it[/dim]" diff --git a/comfy_cli/tracking.py b/comfy_cli/tracking.py index 213683743..1be2bcfa0 100644 --- a/comfy_cli/tracking.py +++ b/comfy_cli/tracking.py @@ -112,6 +112,11 @@ def _resolve_posthog_token() -> str: "from_workflow", "workflow", "capability", + # A ComfyUI clientId is capability-bearing: anyone who can reach the + # server and knows it both receives that client's execution events and + # evicts its socket by connecting as it. Redacted rather than dropped, + # so "was --client-id used" stays measurable. + "client_id", } ) diff --git a/docs/json-output.md b/docs/json-output.md index ea794f9e6..6ad26e445 100644 --- a/docs/json-output.md +++ b/docs/json-output.md @@ -637,6 +637,7 @@ lists what the cloud adds and which of the codes below cannot occur there. | `workflow_unknown_nodes` | Pre-submit validation found unknown class_types / shape mismatches | `errors` (array), `warnings` (array) | 1 | | `partner_node_requires_credential` | Workflow uses a partner-API node and no `api_key_comfy_org` credential is available | `partner_nodes` (array of str, capped at 20 entries × 64 chars each), `partner_node_count` (int, the exact total — read this, not `len(partner_nodes)`), `host`, `port` | 1 | | `spend_consent_required` | Workflow embeds partner-API (paid) nodes and `--allow-spend` was not passed (machine mode) or interactive consent was declined; re-run with `--allow-spend`. Free (non-partner) workflows are unaffected. | `partner_nodes` (array of str, capped at 20 entries × 64 chars each), `partner_node_count` (int, the exact total — read this, not `len(partner_nodes)`); local path also carries `host`, `port`, the cloud path carries `where: "cloud"` | 1 | +| `client_id_rejected` | `--client-id` was combined with `--wait` or with an effective `cloud` target. The flag hands the run's execution events to an already-connected client, so this invocation cannot also watch them, and it addresses a local ComfyUI socket only. Refused before submit — nothing was queued | `reason` (str, `"wait"` or `"cloud"`) | 1 | | `prompt_rejected` | Server returned HTTP 400 with `node_errors` | `status` (400), `node_errors` (array — [shape](#node_errors-shape)) | 1 | | `client_error` | Server returned another HTTP 4xx response (including 429; `cloud_rate_limited` is Cloud only) | `status` (int, 4xx), `body` (str) | 1 | | `server_error` | Server returned an HTTP 5xx response | `status` (int, 5xx), `body` (str) | 1 | diff --git a/tests/comfy_cli/command/test_run_client_id.py b/tests/comfy_cli/command/test_run_client_id.py index e2613cc31..033d63272 100644 --- a/tests/comfy_cli/command/test_run_client_id.py +++ b/tests/comfy_cli/command/test_run_client_id.py @@ -22,7 +22,7 @@ from comfy_cli import jobs_state from comfy_cli.cmdline import run as run_command -from comfy_cli.command.jobs import _resolve_watch_client_id +from comfy_cli.command.jobs import _client_id_withheld, _resolve_watch_client_id from comfy_cli.command.run import WorkflowExecution, execute from comfy_cli.output import Renderer, set_renderer from comfy_cli.output.renderer import OutputMode, reset_renderer_for_testing @@ -149,6 +149,17 @@ def test_conflicting_flags_are_refused(self, workflow_file, capsys, kwargs, reas assert env["error"]["details"]["reason"] == reason +class TestTelemetryDoesNotLeakTheSocketID: + # A clientId is capability-bearing in ComfyUI's model: knowing it is enough + # to receive another client's execution events and to evict its socket. + def test_client_id_is_redacted_but_still_counted(self): + from comfy_cli import tracking + + props = tracking.filter_command_kwargs({"client_id": BORROWED, "wait": False}) + assert BORROWED not in str(props) + assert "client_id" in props, "the key must survive so flag usage stays measurable" + + class TestWatchDoesNotStealTheSocket: def test_borrowed_record_resolves_to_no_client_id(self, monkeypatch): state = jobs_state.new( @@ -174,3 +185,25 @@ def test_minted_record_still_resolves(self, monkeypatch): ) monkeypatch.setattr(jobs_state, "read", lambda _: state) assert _resolve_watch_client_id("127.0.0.1", 8188, "p") == "cli-minted" + + # Both reasons `_resolve_watch_client_id` answers None want OPPOSITE advice: + # an unresolvable id is worth passing --client-id for, a withheld one must + # never be, because doing so performs the eviction the guard exists to stop. + @pytest.mark.parametrize( + ("borrowed", "expected"), + [(True, True), (False, False)], + ) + def test_withheld_is_distinguishable_from_unresolvable(self, monkeypatch, borrowed, expected): + state = jobs_state.new( + prompt_id="p", + client_id=BORROWED, + workflow="w.json", + where="local", + client_id_borrowed=borrowed, + ) + monkeypatch.setattr(jobs_state, "read", lambda _: state) + assert _client_id_withheld("p") is expected + + def test_no_state_file_is_not_withheld(self, monkeypatch): + monkeypatch.setattr(jobs_state, "read", lambda _: None) + assert _client_id_withheld("p") is False From eb5e12c0039c3a77bf2a00ab8fb429f0c18b6d48 Mon Sep 17 00:00:00 2001 From: glary Date: Sun, 4 Oct 2026 09:20:49 +0000 Subject: [PATCH 03/22] fix(run): stamp the borrowed-client marker on the prompt, not just on disk The guard added with --client-id read client_id_borrowed only from our own job state file, which made it inert for exactly the runs it protects: the in-app agent runs comfy-cli under a sandboxed HOME it deletes at turn end, so the user's own `comfy jobs watch` can never see that run's state. It then recovered the borrowed id from /queue or /history and attached as it. ComfyUI pops the displaced socket WITHOUT closing it, so the tab fires no close event, never reconnects, and goes dark for that run and every run after it until the page is reloaded. Stamp the marker into the submitted extra_data as well. ComfyUI preserves unknown extra_data keys (only the two credential keys are stripped) and serves the slot back from both /queue and /history, so every watcher can read it regardless of config root or machine. Verified against a live ComfyUI: the marker round-trips through both endpoints, and with the state file deleted the watch still refuses the id while an ordinary run still resolves its own. The previous docstring claimed closing this needed a server-side signal ComfyUI does not emit. That was wrong, and is corrected. --- CHANGELOG.md | 6 +- comfy_cli/command/jobs.py | 97 +++++++++++-------- comfy_cli/command/run/execution.py | 4 +- comfy_cli/jobs_state.py | 11 +++ tests/comfy_cli/command/test_run_client_id.py | 60 ++++++++++-- 5 files changed, 128 insertions(+), 50 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 47a5db8d4..fe0b98957 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,8 +41,10 @@ history. canvas light up. Local only, and refused with `client_id_rejected` alongside `--wait` or `--where cloud`: ComfyUI evicts whatever socket already holds an incoming clientId, so this invocation must not attach as the borrowed one. - `comfy jobs watch` declines to re-attach to such a run for the same reason - and polls status instead. + Such a run is marked both in its job state file and in the submitted + `extra_data`, so `comfy jobs watch` declines to re-attach to it — from any + machine, including one that cannot see the submitter's state file — and + polls status instead. ### Changed diff --git a/comfy_cli/command/jobs.py b/comfy_cli/command/jobs.py index b51cc1652..b9919180b 100644 --- a/comfy_cli/command/jobs.py +++ b/comfy_cli/command/jobs.py @@ -1861,7 +1861,22 @@ def _client_id_from_extra_data(extra: Any) -> str | None: return None -def _client_id_withheld(prompt_id: str) -> bool: +def _borrowed_from_extra_data(extra: Any) -> bool: + """Read ``comfy run --client-id``'s marker back off a queue/history entry. + + The marker rides ``extra_data`` rather than only our own state file because + the submitter and the watcher are routinely different processes with + different config roots — the in-app agent runs the CLI under a sandboxed + ``HOME`` it deletes at turn end, so the user's own `jobs watch` can never + see that run's state file. ComfyUI preserves unknown ``extra_data`` keys + (only the two credential keys are stripped) and exposes the slot through + both ``/queue`` and ``/history``, so this is the one signal every watcher + can read. + """ + return isinstance(extra, dict) and extra.get(jobs_state.BORROWED_CLIENT_ID_KEY) is True + + +def _client_id_withheld(host: str, port: int, prompt_id: str) -> bool: """True if this prompt's client_id is one we deliberately refuse to attach as — a run submitted with ``comfy run --client-id``. @@ -1869,22 +1884,48 @@ def _client_id_withheld(prompt_id: str) -> bool: want opposite advice: an unresolvable id is worth passing ``--client-id`` for, a withheld one must never be. """ - from comfy_cli import jobs_state - try: job = jobs_state.read(prompt_id) except (ValueError, OSError): - return False - return job is not None and job.client_id_borrowed + job = None + if job is not None and job.client_id_borrowed: + return True + return _borrowed_from_extra_data(_submitted_extra_data(host, port, prompt_id)) # How long a withheld watch waits on its (deliberately eventless) socket before -# reconciling against /history. The ordinary `--timeout` is a silence budget for -# a socket that SHOULD be receiving events; here nothing will ever arrive, so it -# would just be dead air before the first status read. +# reconciling. The ordinary `--timeout` is a silence budget for a socket that +# SHOULD be receiving events; here nothing ever will, so it would be dead air. +# This raises the reconcile cadence for the whole watch, not just the first +# read, which is affordable against a loopback/LAN ComfyUI. _WITHHELD_WATCH_POLL_S = 2.0 +def _submitted_extra_data(host: str, port: int, prompt_id: str) -> Any: + """This prompt's ``extra_data`` as the SERVER has it: from ``/queue`` while + it is pending or running, then ``/history`` once it has finished.""" + try: + q = _http_get_json(f"http://{host}:{port}/queue") + except RuntimeError: + q = {} + if isinstance(q, dict): + for key in ("queue_running", "queue_pending"): + for entry in q.get(key) or []: + # entry layout: number, prompt_id, prompt, extra_data, outputs_to_execute + if isinstance(entry, list) and len(entry) > 3 and entry[1] == prompt_id: + return entry[3] + + try: + h = _http_get_json(f"http://{host}:{port}/history/{prompt_id}") + except RuntimeError: + return None + body = h.get(prompt_id) if isinstance(h, dict) else None + prompt = body.get("prompt") if isinstance(body, dict) else None + if isinstance(prompt, list) and len(prompt) > 3: + return prompt[3] + return None + + def _resolve_watch_client_id(host: str, port: int, prompt_id: str) -> str | None: """Find the ``client_id`` this prompt was submitted with, or None. @@ -1906,17 +1947,11 @@ def _resolve_watch_client_id(host: str, port: int, prompt_id: str) -> str | None A run submitted via ``comfy run --client-id`` is the one case where that caveat is a certainty rather than a risk: the recorded id belongs to a live - client that is being fed on purpose. Those resolve to None so the watch - polls status instead of taking the client's socket away. - - That guard reaches only as far as our own state file. Steps 2 and 3 read the - id back out of the SERVER, which records no borrowed marker, so a watch that - cannot see the submitting run's state — another machine, another config - root, a cleared state dir — still resolves a borrowed id and attaches as it. - Closing that needs a server-side signal ComfyUI does not currently emit. + client that is being fed on purpose. Those resolve to None — at every step, + because ``run`` stamps the marker into the submitted ``extra_data`` as well + as the state file — so the watch polls status instead of displacing a tab + that would then receive nothing until the page is reloaded. """ - from comfy_cli import jobs_state - try: job = jobs_state.read(prompt_id) except (ValueError, OSError): # unsafe prompt_id / unreadable state dir @@ -1926,28 +1961,10 @@ def _resolve_watch_client_id(host: str, port: int, prompt_id: str) -> str | None if job is not None and isinstance(job.client_id, str) and job.client_id.strip(): return job.client_id - try: - q = _http_get_json(f"http://{host}:{port}/queue") - except RuntimeError: - q = {} - if isinstance(q, dict): - for key in ("queue_running", "queue_pending"): - for entry in q.get(key) or []: - # entry layout: number, prompt_id, prompt, extra_data, outputs_to_execute - if isinstance(entry, list) and len(entry) > 3 and entry[1] == prompt_id: - cid = _client_id_from_extra_data(entry[3]) - if cid: - return cid - - try: - h = _http_get_json(f"http://{host}:{port}/history/{prompt_id}") - except RuntimeError: + extra = _submitted_extra_data(host, port, prompt_id) + if _borrowed_from_extra_data(extra): return None - body = h.get(prompt_id) if isinstance(h, dict) else None - prompt = body.get("prompt") if isinstance(body, dict) else None - if isinstance(prompt, list) and len(prompt) > 3: - return _client_id_from_extra_data(prompt[3]) - return None + return _client_id_from_extra_data(extra) def _history_completed_nodes(host: str, port: int, prompt_id: str) -> set[str]: @@ -2252,7 +2269,7 @@ def watch_cmd( token = cancellation.get_token() token.on_cancel(lambda: _safe_close_ws(ws)) - withheld = attached_client_id is None and _client_id_withheld(prompt_id) + withheld = attached_client_id is None and _client_id_withheld(h, p, prompt_id) # A withheld watch holds a fresh-id socket the server will never address, so # /history is the only thing that can move it. Reconcile on the poll cadence # rather than sitting out the full silence budget first. diff --git a/comfy_cli/command/run/execution.py b/comfy_cli/command/run/execution.py index f233f3629..2af029559 100644 --- a/comfy_cli/command/run/execution.py +++ b/comfy_cli/command/run/execution.py @@ -36,7 +36,7 @@ from rich.progress import BarColumn, Progress, TimeElapsedColumn from rich.table import Column, Table -from comfy_cli import execution_errors +from comfy_cli import execution_errors, jobs_state from comfy_cli.caller import usage_source from comfy_cli.command.run.loader import _MAX_BODY_PREVIEW, _node_errors_to_list from comfy_cli.http import no_redirect_urlopen @@ -214,6 +214,8 @@ def queue(self): # and, via the caller-derived value, human-driven CLI runs apart from # agent-driven ones. `self.extra_data` still overwrites it below. data["extra_data"] = {"comfy_usage_source": usage_source()} + if self.borrowed_client_id: + data["extra_data"][jobs_state.BORROWED_CLIENT_ID_KEY] = True if self.extra_data: data["extra_data"].update(self.extra_data) elif self.api_key: diff --git a/comfy_cli/jobs_state.py b/comfy_cli/jobs_state.py index 0feface4d..94ba28247 100644 --- a/comfy_cli/jobs_state.py +++ b/comfy_cli/jobs_state.py @@ -57,6 +57,17 @@ TERMINAL_STATUSES = frozenset({"completed", "error", "cancelled"}) +# The ``extra_data`` key ``comfy run --client-id`` stamps on a submitted prompt +# to say its client_id was BORROWED from a live client rather than minted for +# the run. It rides the prompt because the state file below is not a shared +# channel: the submitter and a later watcher are routinely different processes +# under different config roots (the in-app agent runs the CLI under a sandboxed +# HOME it deletes at turn end), and re-attaching to a borrowed id silently +# strands the client it was borrowed from. ComfyUI preserves unknown extra_data +# keys and serves the slot back from /queue and /history, so every watcher can +# read it. +BORROWED_CLIENT_ID_KEY = "comfy_client_id_borrowed" + # Cloud's /api/jobs status enum (ingest ``toFilterStatus``: pending, # in_progress, completed, failed, cancelled) -> the CLI's published jobs # vocabulary (``comfy_cli/schemas/jobs.json``). Legacy raw-jobstate spellings diff --git a/tests/comfy_cli/command/test_run_client_id.py b/tests/comfy_cli/command/test_run_client_id.py index 033d63272..760594386 100644 --- a/tests/comfy_cli/command/test_run_client_id.py +++ b/tests/comfy_cli/command/test_run_client_id.py @@ -22,6 +22,7 @@ from comfy_cli import jobs_state from comfy_cli.cmdline import run as run_command +from comfy_cli.command import jobs from comfy_cli.command.jobs import _client_id_withheld, _resolve_watch_client_id from comfy_cli.command.run import WorkflowExecution, execute from comfy_cli.output import Renderer, set_renderer @@ -160,7 +161,37 @@ def test_client_id_is_redacted_but_still_counted(self): assert "client_id" in props, "the key must survive so flag usage stays measurable" +class TestBorrowedMarkerRidesThePrompt: + # The state file is NOT a shared channel: the in-app agent runs the CLI + # under a sandboxed HOME it deletes at turn end, so the user's own + # `jobs watch` can never see the state file of the very runs that borrow an + # id. The marker has to travel on the prompt or the guard is inert for + # every run it exists to protect. + @pytest.mark.parametrize("borrowed", [True, False]) + def test_marker_is_submitted_with_the_prompt(self, simple_workflow, borrowed): + ex = WorkflowExecution( + simple_workflow, + "127.0.0.1", + 8188, + False, + None, + False, + 30, + client_id=BORROWED if borrowed else None, + ) + with patch("comfy_cli.command.run.execution.no_redirect_urlopen") as mock_open: + mock_open.return_value.__enter__.return_value.read.return_value = json.dumps({"prompt_id": "p"}).encode() + ex.queue() + extra = json.loads(mock_open.call_args[0][0].data.decode())["extra_data"] + assert extra.get(jobs_state.BORROWED_CLIENT_ID_KEY) is (True if borrowed else None) + + class TestWatchDoesNotStealTheSocket: + @pytest.fixture(autouse=True) + def no_server(self, monkeypatch): + """Default: the server knows nothing. Tests that care override it.""" + monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: None) + def test_borrowed_record_resolves_to_no_client_id(self, monkeypatch): state = jobs_state.new( prompt_id="p", @@ -189,11 +220,8 @@ def test_minted_record_still_resolves(self, monkeypatch): # Both reasons `_resolve_watch_client_id` answers None want OPPOSITE advice: # an unresolvable id is worth passing --client-id for, a withheld one must # never be, because doing so performs the eviction the guard exists to stop. - @pytest.mark.parametrize( - ("borrowed", "expected"), - [(True, True), (False, False)], - ) - def test_withheld_is_distinguishable_from_unresolvable(self, monkeypatch, borrowed, expected): + @pytest.mark.parametrize("borrowed", [True, False]) + def test_withheld_is_distinguishable_from_unresolvable(self, monkeypatch, borrowed): state = jobs_state.new( prompt_id="p", client_id=BORROWED, @@ -202,8 +230,26 @@ def test_withheld_is_distinguishable_from_unresolvable(self, monkeypatch, borrow client_id_borrowed=borrowed, ) monkeypatch.setattr(jobs_state, "read", lambda _: state) - assert _client_id_withheld("p") is expected + assert _client_id_withheld("127.0.0.1", 8188, "p") is borrowed def test_no_state_file_is_not_withheld(self, monkeypatch): monkeypatch.setattr(jobs_state, "read", lambda _: None) - assert _client_id_withheld("p") is False + assert _client_id_withheld("127.0.0.1", 8188, "p") is False + + # The case the state-file-only guard missed entirely: a watcher that cannot + # see the submitting run's state still has to refuse the id. + def test_server_marker_is_honoured_without_a_state_file(self, monkeypatch): + monkeypatch.setattr(jobs_state, "read", lambda _: None) + monkeypatch.setattr( + jobs, + "_submitted_extra_data", + lambda *_: {"client_id": BORROWED, jobs_state.BORROWED_CLIENT_ID_KEY: True}, + ) + assert _resolve_watch_client_id("127.0.0.1", 8188, "p") is None + assert _client_id_withheld("127.0.0.1", 8188, "p") is True + + def test_an_unmarked_server_record_still_resolves(self, monkeypatch): + monkeypatch.setattr(jobs_state, "read", lambda _: None) + monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: {"client_id": "cli-minted"}) + assert _resolve_watch_client_id("127.0.0.1", 8188, "p") == "cli-minted" + assert _client_id_withheld("127.0.0.1", 8188, "p") is False From 86113281b5b08a28b9ebe32af1acf5d24419f28e Mon Sep 17 00:00:00 2001 From: glary Date: Sun, 4 Oct 2026 09:52:48 +0000 Subject: [PATCH 04/22] docs(jobs): warn against forcing --client-id on a borrowed run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolution withholds a borrowed id on purpose, but the agent-facing doc still said "use --client-id to force a specific one" with no caveat — the one action that takes the user's tab off its own socket. Also correct the CHANGELOG: the cloud rejection is on the RESOLVED target, so COMFY_WHERE or a saved default triggers it with no flag present. --- CHANGELOG.md | 3 ++- docs/json-output.md | 6 +++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fe0b98957..adb2a7598 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,7 +39,8 @@ history. events — node highlights, progress, outputs — to that client. This is what lets a tool run a workflow on a user's behalf and still have the user's open canvas light up. Local only, and refused with `client_id_rejected` alongside - `--wait` or `--where cloud`: ComfyUI evicts whatever socket already holds an + `--wait` or an effective `cloud` target (the flag, `COMFY_WHERE`, or a saved + default): ComfyUI evicts whatever socket already holds an incoming clientId, so this invocation must not attach as the borrowed one. Such a run is marked both in its job state file and in the submitted `extra_data`, so `comfy jobs watch` declines to re-attach to it — from any diff --git a/docs/json-output.md b/docs/json-output.md index 6ad26e445..92343b2b9 100644 --- a/docs/json-output.md +++ b/docs/json-output.md @@ -29,7 +29,11 @@ A local ComfyUI server does **not** broadcast execution events: it addresses state file `comfy run` wrote, else `/queue`, else `/history` — and reconnects under it; the terminal envelope reports which id it used (`data.client_id`) and whether it was the real submitter (`data.attached`). Use `--client-id` to force a -specific one. Both keys are present on *every* `jobs watch` terminal envelope: a +specific one — but NOT on a run submitted by `comfy run --client-id`: that id +belongs to a live client being fed on purpose, resolution withholds it +deliberately (`attached: false` with the id recoverable from `/queue`), and +forcing it takes that client off its own socket until it reconnects. Both +`data.client_id` and `data.attached` are present on *every* terminal envelope: a watch of an already-finished prompt short-circuits without opening a socket, and reports `client_id: null` / `attached: false`. Reconnecting under an existing id is ComfyUI's own session-resume path, so a submitter that is *still* holding that From a2adb5d1996cf9dd05fdb483d39870f55e353bef Mon Sep 17 00:00:00 2001 From: bymyself Date: Sun, 4 Oct 2026 03:38:18 -0700 Subject: [PATCH 05/22] fix(run): close borrowed client id guard gaps --- comfy_cli/cmdline.py | 10 ++++++- comfy_cli/command/jobs.py | 29 +++++++++++++++++-- comfy_cli/command/run/execution.py | 6 ++-- tests/comfy_cli/command/test_run_client_id.py | 20 +++++++++++-- 4 files changed, 57 insertions(+), 8 deletions(-) diff --git a/comfy_cli/cmdline.py b/comfy_cli/cmdline.py index 88e3600da..84ef38bb3 100644 --- a/comfy_cli/cmdline.py +++ b/comfy_cli/cmdline.py @@ -1265,7 +1265,15 @@ def run( # `where` kwarg is None then). Rides on the execution_success/_error events. _track_props["target"] = "cloud" if decision.target is where_module.WhereTarget.CLOUD else "local" - client_id = (client_id or "").strip() or None + if client_id is not None and not client_id.strip(): + renderer.error( + code="client_id_rejected", + message="--client-id must not be empty", + hint="pass the connected browser client's id, or omit --client-id", + details={"reason": "empty"}, + ) + raise typer.Exit(code=1) + client_id = client_id.strip() if client_id is not None else None if client_id: rejection = None if decision.target is where_module.WhereTarget.CLOUD: diff --git a/comfy_cli/command/jobs.py b/comfy_cli/command/jobs.py index b9919180b..a8157bcba 100644 --- a/comfy_cli/command/jobs.py +++ b/comfy_cli/command/jobs.py @@ -1967,6 +1967,20 @@ def _resolve_watch_client_id(host: str, port: int, prompt_id: str) -> str | None return _client_id_from_extra_data(extra) +def _select_watch_client_id(host: str, port: int, prompt_id: str, requested: str | None) -> tuple[str | None, bool]: + """Select a safe socket identity and report whether the submitter was withheld. + + An explicit override must not bypass the borrowed-client marker. A marked + prompt exists specifically to feed another live socket; attaching under any + requested identity would defeat the safe poll-only behavior and can evict + that live client when the caller supplies its id. + """ + withheld = _client_id_withheld(host, port, prompt_id) + if withheld and requested: + raise ValueError("a borrowed-client run is poll-only") + return requested or _resolve_watch_client_id(host, port, prompt_id), withheld + + def _history_completed_nodes(host: str, port: int, prompt_id: str) -> set[str]: """Nodes the server itself records as having run, straight from ``/history``. @@ -2251,10 +2265,20 @@ def watch_cmd( _emit_terminal(renderer, snap, command="jobs watch") return - ws = WebSocket() # Re-attach as the submitting session, otherwise the server addresses every # execution event to a socket we are not holding and this watch sees nothing. - attached_client_id = client_id or _resolve_watch_client_id(h, p, prompt_id) + try: + attached_client_id, withheld = _select_watch_client_id(h, p, prompt_id, client_id) + except ValueError: + renderer.error( + code="client_id_rejected", + message="This run borrowed another client's live socket and can only be watched by polling", + hint="drop --client-id; comfy jobs watch will poll this run safely", + details={"reason": "borrowed"}, + ) + raise typer.Exit(code=1) + + ws = WebSocket() ws_client_id = attached_client_id or str(uuid.uuid4()) try: ws.connect(f"ws://{h}:{p}/ws?clientId={urllib.parse.quote(ws_client_id, safe='')}") @@ -2269,7 +2293,6 @@ def watch_cmd( token = cancellation.get_token() token.on_cancel(lambda: _safe_close_ws(ws)) - withheld = attached_client_id is None and _client_id_withheld(h, p, prompt_id) # A withheld watch holds a fresh-id socket the server will never address, so # /history is the only thing that can move it. Reconcile on the poll cadence # rather than sitting out the full silence budget first. diff --git a/comfy_cli/command/run/execution.py b/comfy_cli/command/run/execution.py index 2af029559..adb458080 100644 --- a/comfy_cli/command/run/execution.py +++ b/comfy_cli/command/run/execution.py @@ -214,12 +214,14 @@ def queue(self): # and, via the caller-derived value, human-driven CLI runs apart from # agent-driven ones. `self.extra_data` still overwrites it below. data["extra_data"] = {"comfy_usage_source": usage_source()} - if self.borrowed_client_id: - data["extra_data"][jobs_state.BORROWED_CLIENT_ID_KEY] = True if self.extra_data: data["extra_data"].update(self.extra_data) elif self.api_key: data["extra_data"]["api_key_comfy_org"] = self.api_key + # This is a safety property, not caller metadata: keep it authoritative + # even if a future extra_data caller happens to use the same key. + if self.borrowed_client_id: + data["extra_data"][jobs_state.BORROWED_CLIENT_ID_KEY] = True req = request.Request(f"http://{self.host}:{self.port}/prompt", json.dumps(data).encode("utf-8")) req.add_header("Comfy-Usage-Source", "comfy-cli") try: diff --git a/tests/comfy_cli/command/test_run_client_id.py b/tests/comfy_cli/command/test_run_client_id.py index 760594386..424c3b8a0 100644 --- a/tests/comfy_cli/command/test_run_client_id.py +++ b/tests/comfy_cli/command/test_run_client_id.py @@ -23,7 +23,7 @@ from comfy_cli import jobs_state from comfy_cli.cmdline import run as run_command from comfy_cli.command import jobs -from comfy_cli.command.jobs import _client_id_withheld, _resolve_watch_client_id +from comfy_cli.command.jobs import _client_id_withheld, _resolve_watch_client_id, _select_watch_client_id from comfy_cli.command.run import WorkflowExecution, execute from comfy_cli.output import Renderer, set_renderer from comfy_cli.output.renderer import OutputMode, reset_renderer_for_testing @@ -111,6 +111,7 @@ def capture(state): patch("comfy_cli.command.run._tail_state_file"), patch("comfy_cli.jobs_state.write", side_effect=capture), patch("comfy_cli.http._AUTHED_OPENER.open") as mock_open, + patch("comfy_cli.command.run.WebSocket") as mock_ws, ): mock_open.return_value.__enter__.return_value.read.return_value = json.dumps({"prompt_id": "p"}).encode() execute( @@ -126,6 +127,10 @@ def capture(state): state = written[0] assert state.client_id == BORROWED assert state.client_id_borrowed is True + body = json.loads(mock_open.call_args[0][0].data.decode()) + assert body["client_id"] == BORROWED + assert body["extra_data"][jobs_state.BORROWED_CLIENT_ID_KEY] is True + mock_ws.assert_not_called() assert _envelope(capsys.readouterr().out)["data"]["client_id"] == BORROWED @@ -135,6 +140,7 @@ class TestCliRejections: [ ({"wait": True, "where": "local"}, "wait"), ({"wait": False, "where": "cloud"}, "cloud"), + ({"wait": False, "where": "local", "client_id": " "}, "empty"), ], ) def test_conflicting_flags_are_refused(self, workflow_file, capsys, kwargs, reason): @@ -142,7 +148,7 @@ def test_conflicting_flags_are_refused(self, workflow_file, capsys, kwargs, reas patch("comfy_cli.cmdline.tracking", MagicMock()), pytest.raises(typer.Exit) as exc, ): - run_command(workflow=workflow_file, client_id=BORROWED, **kwargs) + run_command(workflow=workflow_file, client_id=kwargs.pop("client_id", BORROWED), **kwargs) assert exc.value.exit_code == 1 env = _envelope(capsys.readouterr().out) assert env["ok"] is False @@ -248,6 +254,16 @@ def test_server_marker_is_honoured_without_a_state_file(self, monkeypatch): assert _resolve_watch_client_id("127.0.0.1", 8188, "p") is None assert _client_id_withheld("127.0.0.1", 8188, "p") is True + def test_explicit_override_cannot_bypass_borrowed_marker(self, monkeypatch): + monkeypatch.setattr(jobs_state, "read", lambda _: None) + monkeypatch.setattr( + jobs, + "_submitted_extra_data", + lambda *_: {"client_id": BORROWED, jobs_state.BORROWED_CLIENT_ID_KEY: True}, + ) + with pytest.raises(ValueError, match="poll-only"): + _select_watch_client_id("127.0.0.1", 8188, "p", BORROWED) + def test_an_unmarked_server_record_still_resolves(self, monkeypatch): monkeypatch.setattr(jobs_state, "read", lambda _: None) monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: {"client_id": "cli-minted"}) From 83eda0b725b233435a05097a6627599ab6aaebb0 Mon Sep 17 00:00:00 2001 From: glary Date: Sun, 4 Oct 2026 10:51:09 +0000 Subject: [PATCH 06/22] docs(run): document every client_id_rejected reason and both sources The registry and the run error table listed only `wait` and `cloud`. The code emits four reasons across two commands: `empty`, `cloud` and `wait` from `comfy run`, and `borrowed` from `comfy jobs watch` on a prompt that borrowed a live client's id. A consumer branching on `details.reason` would have met two values the docs never mentioned. Also correct the jobs watch prose: it still said forcing --client-id on a borrowed run 'takes that client off its own socket'. Since a2adb5d that is refused outright before any socket opens, which is the opposite outcome. Both are documentation of behaviour added in a2adb5d; no behaviour change. --- comfy_cli/error_codes.py | 21 ++++++++++++++------- docs/json-output.md | 12 +++++++----- 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/comfy_cli/error_codes.py b/comfy_cli/error_codes.py index f878cc377..221efdcec 100644 --- a/comfy_cli/error_codes.py +++ b/comfy_cli/error_codes.py @@ -309,13 +309,20 @@ class ErrorCode: ), ErrorCode( "client_id_rejected", - "`--client-id` was combined with something it cannot serve. The flag submits the prompt as an " - "ALREADY-CONNECTED client (normally a browser tab) so ComfyUI addresses the run's execution " - "events to that client instead of this invocation. It is therefore local-only, and this " - "invocation cannot also watch the run: ComfyUI's `/ws` handler evicts any socket already " - "registered under an incoming clientId, so opening one here would silence the very client the " - "flag exists to feed. `details.reason` is `wait` or `cloud`.", - "drop `--client-id` to watch the run from the CLI, or drop `--wait` and poll `comfy jobs status`", + "`--client-id` was combined with something it cannot serve. The flag names an " + "ALREADY-CONNECTED client (normally a browser tab) so ComfyUI addresses a run's execution " + "events to that client rather than to the invocation that submitted it. ComfyUI's `/ws` " + "handler evicts any socket already registered under an incoming clientId, so nothing may " + "attach as a borrowed id without silencing the very client the flag exists to feed. " + "Raised by two commands, and `details.reason` says which case: on `comfy run`, `empty` (the " + "flag was passed with a blank value), `cloud` (local-only — cloud fans execution events out " + "per user/workspace regardless of submitter) and `wait` (`--wait` would watch a socket this " + "run's events are deliberately not sent to); on `comfy jobs watch`, `borrowed` (the prompt is " + "marked as having borrowed a live client's id, so it is poll-only and no override is " + "honoured). Refused before anything is submitted or any socket is opened.", + "pass a non-empty id, or drop the conflicting flag: `--client-id` cannot be combined with " + "`--wait` or a cloud target, and `comfy jobs watch` on a borrowed run takes no `--client-id` " + "at all — poll it with `comfy jobs status`", ), ErrorCode( "host_flag_cloud", diff --git a/docs/json-output.md b/docs/json-output.md index 92343b2b9..084b8c107 100644 --- a/docs/json-output.md +++ b/docs/json-output.md @@ -29,10 +29,12 @@ A local ComfyUI server does **not** broadcast execution events: it addresses state file `comfy run` wrote, else `/queue`, else `/history` — and reconnects under it; the terminal envelope reports which id it used (`data.client_id`) and whether it was the real submitter (`data.attached`). Use `--client-id` to force a -specific one — but NOT on a run submitted by `comfy run --client-id`: that id -belongs to a live client being fed on purpose, resolution withholds it -deliberately (`attached: false` with the id recoverable from `/queue`), and -forcing it takes that client off its own socket until it reconnects. Both +specific one — except on a run submitted by `comfy run --client-id`, where the +id belongs to a live client being fed on purpose. Resolution withholds it +deliberately (`attached: false`) and the watch polls status instead; passing +`--client-id` on such a run is refused with `client_id_rejected` +(`reason: "borrowed"`) before any socket opens, so there is no way to take that +client off its own socket. Both `data.client_id` and `data.attached` are present on *every* terminal envelope: a watch of an already-finished prompt short-circuits without opening a socket, and reports `client_id: null` / `attached: false`. Reconnecting under an existing id @@ -641,7 +643,7 @@ lists what the cloud adds and which of the codes below cannot occur there. | `workflow_unknown_nodes` | Pre-submit validation found unknown class_types / shape mismatches | `errors` (array), `warnings` (array) | 1 | | `partner_node_requires_credential` | Workflow uses a partner-API node and no `api_key_comfy_org` credential is available | `partner_nodes` (array of str, capped at 20 entries × 64 chars each), `partner_node_count` (int, the exact total — read this, not `len(partner_nodes)`), `host`, `port` | 1 | | `spend_consent_required` | Workflow embeds partner-API (paid) nodes and `--allow-spend` was not passed (machine mode) or interactive consent was declined; re-run with `--allow-spend`. Free (non-partner) workflows are unaffected. | `partner_nodes` (array of str, capped at 20 entries × 64 chars each), `partner_node_count` (int, the exact total — read this, not `len(partner_nodes)`); local path also carries `host`, `port`, the cloud path carries `where: "cloud"` | 1 | -| `client_id_rejected` | `--client-id` was combined with `--wait` or with an effective `cloud` target. The flag hands the run's execution events to an already-connected client, so this invocation cannot also watch them, and it addresses a local ComfyUI socket only. Refused before submit — nothing was queued | `reason` (str, `"wait"` or `"cloud"`) | 1 | +| `client_id_rejected` | `--client-id` was combined with something it cannot serve. On `comfy run`: the flag was blank (`empty`), the effective target is cloud (`cloud` — the flag addresses a local ComfyUI socket, and cloud fans execution events out per user/workspace anyway), or `--wait` was passed (`wait` — the run's events go to the named client, so this invocation would watch a silent socket). Also raised by `comfy jobs watch` when the prompt is marked as having borrowed a live client's id (`borrowed`): that run is poll-only and no `--client-id` override is honoured. Refused before anything is submitted and before any socket is opened | `reason` (str, `"empty"`, `"cloud"`, `"wait"` or `"borrowed"`) | 1 | | `prompt_rejected` | Server returned HTTP 400 with `node_errors` | `status` (400), `node_errors` (array — [shape](#node_errors-shape)) | 1 | | `client_error` | Server returned another HTTP 4xx response (including 429; `cloud_rate_limited` is Cloud only) | `status` (int, 4xx), `body` (str) | 1 | | `server_error` | Server returned an HTTP 5xx response | `status` (int, 5xx), `body` (str) | 1 | From 6ed449ad8eec67d1693b1fd59fd8712c2ac4e244 Mon Sep 17 00:00:00 2001 From: bymyself Date: Sun, 4 Oct 2026 03:54:43 -0700 Subject: [PATCH 07/22] test(jobs): prove borrowed watch opens no socket --- tests/comfy_cli/command/test_run_client_id.py | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/tests/comfy_cli/command/test_run_client_id.py b/tests/comfy_cli/command/test_run_client_id.py index 424c3b8a0..570c3e0b3 100644 --- a/tests/comfy_cli/command/test_run_client_id.py +++ b/tests/comfy_cli/command/test_run_client_id.py @@ -19,6 +19,7 @@ import pytest import typer +from typer.testing import CliRunner from comfy_cli import jobs_state from comfy_cli.cmdline import run as run_command @@ -264,6 +265,34 @@ def test_explicit_override_cannot_bypass_borrowed_marker(self, monkeypatch): with pytest.raises(ValueError, match="poll-only"): _select_watch_client_id("127.0.0.1", 8188, "p", BORROWED) + def test_watch_rejects_explicit_override_before_opening_a_socket(self, monkeypatch): + monkeypatch.setattr(jobs, "_server_or_error", lambda *_args, **_kwargs: True) + monkeypatch.setattr( + jobs, + "_snapshot", + lambda *_: {"prompt_id": "p", "status": "running", "outputs": []}, + ) + monkeypatch.setattr(jobs_state, "read", lambda _: None) + monkeypatch.setattr( + jobs, + "_submitted_extra_data", + lambda *_: {"client_id": BORROWED, jobs_state.BORROWED_CLIENT_ID_KEY: True}, + ) + + def fail_if_constructed(*_args, **_kwargs): + raise AssertionError("borrowed watch constructed a WebSocket") + + monkeypatch.setattr(jobs, "WebSocket", fail_if_constructed) + result = CliRunner().invoke( + jobs.app, + ["watch", "p", "--where", "local", "--client-id", BORROWED], + ) + + assert result.exit_code == 1, result.output + env = _envelope(result.output) + assert env["error"]["code"] == "client_id_rejected" + assert env["error"]["details"]["reason"] == "borrowed" + def test_an_unmarked_server_record_still_resolves(self, monkeypatch): monkeypatch.setattr(jobs_state, "read", lambda _: None) monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: {"client_id": "cli-minted"}) From 1b91e801b0332c79432f39293d42367081d3a9bb Mon Sep 17 00:00:00 2001 From: bymyself Date: Sun, 4 Oct 2026 04:16:09 -0700 Subject: [PATCH 08/22] fix(jobs): poll borrowed watches without a socket --- comfy_cli/command/jobs.py | 77 +++++++++++-------- tests/comfy_cli/command/test_run_client_id.py | 32 ++++++++ 2 files changed, 75 insertions(+), 34 deletions(-) diff --git a/comfy_cli/command/jobs.py b/comfy_cli/command/jobs.py index a8157bcba..8d77b4dcb 100644 --- a/comfy_cli/command/jobs.py +++ b/comfy_cli/command/jobs.py @@ -2278,25 +2278,23 @@ def watch_cmd( ) raise typer.Exit(code=1) - ws = WebSocket() - ws_client_id = attached_client_id or str(uuid.uuid4()) - try: - ws.connect(f"ws://{h}:{p}/ws?clientId={urllib.parse.quote(ws_client_id, safe='')}") - except (WebSocketException, ConnectionError, OSError) as e: - renderer.error( - code="ws_disconnected", - message=f"Could not open WebSocket: {e}", - hint="check the server is reachable; try `comfy jobs status` instead", - ) - raise typer.Exit(code=1) - token = cancellation.get_token() - token.on_cancel(lambda: _safe_close_ws(ws)) - - # A withheld watch holds a fresh-id socket the server will never address, so - # /history is the only thing that can move it. Reconcile on the poll cadence - # rather than sitting out the full silence budget first. - ws.settimeout(min(timeout, _WITHHELD_WATCH_POLL_S) if withheld else timeout) + ws = None + ws_client_id = None + if not withheld: + ws = WebSocket() + ws_client_id = attached_client_id or str(uuid.uuid4()) + try: + ws.connect(f"ws://{h}:{p}/ws?clientId={urllib.parse.quote(ws_client_id, safe='')}") + except (WebSocketException, ConnectionError, OSError) as e: + renderer.error( + code="ws_disconnected", + message=f"Could not open WebSocket: {e}", + hint="check the server is reachable; try `comfy jobs status` instead", + ) + raise typer.Exit(code=1) + token.on_cancel(lambda: _safe_close_ws(ws)) + ws.settimeout(timeout) state = _WatchState(renderer=renderer, prompt_id=prompt_id, host=h, port=p) saw_any_event = False @@ -2319,9 +2317,31 @@ def watch_cmd( try: while True: - try: - raw = ws.recv() - except WebSocketTimeoutException: + raw = None + poll_now = withheld + if withheld: + if token.wait(_WITHHELD_WATCH_POLL_S): + state.end_reason = "cancelled" + break + else: + try: + raw = ws.recv() + except WebSocketTimeoutException: + poll_now = True + except (WebSocketException, ConnectionError, OSError) as e: + # Cancellation closes the socket out from under recv(). Check + # the token before classifying as "server disconnected". + if token.is_set(): + state.end_reason = "cancelled" + break + renderer.error( + code="ws_disconnected", + message=f"Lost connection while watching {prompt_id}: {e}", + hint="re-run `comfy jobs status` to check final state", + ) + raise typer.Exit(code=1) from e + + if poll_now: # If the job moved to completed between recvs, exit cleanly. snap = _snapshot(h, p, prompt_id) if snap and snap["status"] in {"completed", "error", "cancelled"}: @@ -2348,18 +2368,6 @@ def watch_cmd( else: missing_deadline = None continue - except (WebSocketException, ConnectionError, OSError) as e: - # Cancellation closes the socket out from under recv(). Check - # the token before classifying as "server disconnected". - if token.is_set(): - state.end_reason = "cancelled" - break - renderer.error( - code="ws_disconnected", - message=f"Lost connection while watching {prompt_id}: {e}", - hint="re-run `comfy jobs status` to check final state", - ) - raise typer.Exit(code=1) from e if not isinstance(raw, str): continue try: @@ -2380,7 +2388,8 @@ def watch_cmd( if state.terminal: break finally: - _safe_close_ws(ws) + if ws is not None: + _safe_close_ws(ws) elapsed = time.time() - start final_status = state.end_reason or ("completed" if state.completed_nodes else "unknown") diff --git a/tests/comfy_cli/command/test_run_client_id.py b/tests/comfy_cli/command/test_run_client_id.py index 570c3e0b3..ab0cdf383 100644 --- a/tests/comfy_cli/command/test_run_client_id.py +++ b/tests/comfy_cli/command/test_run_client_id.py @@ -293,6 +293,38 @@ def fail_if_constructed(*_args, **_kwargs): assert env["error"]["code"] == "client_id_rejected" assert env["error"]["details"]["reason"] == "borrowed" + def test_watch_polls_borrowed_run_without_opening_a_socket(self, monkeypatch): + snapshots = iter( + [ + {"prompt_id": "p", "status": "running", "outputs": []}, + {"prompt_id": "p", "status": "completed", "outputs": ["result.png"]}, + ] + ) + monkeypatch.setattr(jobs, "_server_or_error", lambda *_args, **_kwargs: True) + monkeypatch.setattr(jobs, "_snapshot", lambda *_: next(snapshots)) + monkeypatch.setattr(jobs, "_history_completed_nodes", lambda *_: {"1"}) + monkeypatch.setattr(jobs, "_WITHHELD_WATCH_POLL_S", 0) + monkeypatch.setattr(jobs_state, "read", lambda _: None) + monkeypatch.setattr( + jobs, + "_submitted_extra_data", + lambda *_: {"client_id": BORROWED, jobs_state.BORROWED_CLIENT_ID_KEY: True}, + ) + + def fail_if_constructed(*_args, **_kwargs): + raise AssertionError("borrowed watch constructed a WebSocket") + + monkeypatch.setattr(jobs, "WebSocket", fail_if_constructed) + result = CliRunner().invoke(jobs.app, ["watch", "p", "--where", "local"]) + + assert result.exit_code == 0, result.output + env = _envelope(result.output) + assert env["data"]["status"] == "completed" + assert env["data"]["outputs"] == ["result.png"] + assert env["data"]["completed_nodes"] == ["1"] + assert env["data"]["client_id"] is None + assert env["data"]["attached"] is False + def test_an_unmarked_server_record_still_resolves(self, monkeypatch): monkeypatch.setattr(jobs_state, "read", lambda _: None) monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: {"client_id": "cli-minted"}) From c1579289c1555c4c5e5ff49c3b24072062319294 Mon Sep 17 00:00:00 2001 From: bymyself Date: Sun, 4 Oct 2026 04:23:02 -0700 Subject: [PATCH 09/22] fix(jobs): preserve borrowed poll failures --- comfy_cli/command/jobs.py | 5 ++- tests/comfy_cli/command/test_run_client_id.py | 36 +++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/comfy_cli/command/jobs.py b/comfy_cli/command/jobs.py index 8d77b4dcb..2037db456 100644 --- a/comfy_cli/command/jobs.py +++ b/comfy_cli/command/jobs.py @@ -2424,7 +2424,10 @@ def watch_cmd( "attached": attached_client_id is not None, } if state.end_details is not None: - payload["details"] = state.end_details if isinstance(state.end_details, dict) else {"raw": state.end_details} + details = dict(state.end_details) if isinstance(state.end_details, dict) else {"raw": state.end_details} + if final_status == "error" and isinstance(details.get("error"), dict): + payload["execution_error"] = details.pop("error") + payload["details"] = details if not saw_any_event and final_status == "unknown": payload["hint"] = "watch returned without events; the prompt may already have completed" _emit_terminal(renderer, payload, command="jobs watch") diff --git a/tests/comfy_cli/command/test_run_client_id.py b/tests/comfy_cli/command/test_run_client_id.py index ab0cdf383..c313e33d0 100644 --- a/tests/comfy_cli/command/test_run_client_id.py +++ b/tests/comfy_cli/command/test_run_client_id.py @@ -325,6 +325,42 @@ def fail_if_constructed(*_args, **_kwargs): assert env["data"]["client_id"] is None assert env["data"]["attached"] is False + def test_borrowed_poll_preserves_a_failed_runs_error(self, monkeypatch): + snapshots = iter( + [ + {"prompt_id": "p", "status": "running", "outputs": []}, + { + "prompt_id": "p", + "status": "error", + "outputs": [], + "error": {"exception_message": "node 5 exploded", "node_id": "5"}, + }, + ] + ) + monkeypatch.setattr(jobs, "_server_or_error", lambda *_args, **_kwargs: True) + monkeypatch.setattr(jobs, "_snapshot", lambda *_: next(snapshots)) + monkeypatch.setattr(jobs, "_history_completed_nodes", lambda *_: set()) + monkeypatch.setattr(jobs, "_WITHHELD_WATCH_POLL_S", 0) + monkeypatch.setattr(jobs_state, "read", lambda _: None) + monkeypatch.setattr( + jobs, + "_submitted_extra_data", + lambda *_: {"client_id": BORROWED, jobs_state.BORROWED_CLIENT_ID_KEY: True}, + ) + monkeypatch.setattr( + jobs, + "WebSocket", + lambda *_args, **_kwargs: (_ for _ in ()).throw(AssertionError("borrowed watch constructed a WebSocket")), + ) + + result = CliRunner().invoke(jobs.app, ["watch", "p", "--where", "local"]) + + assert result.exit_code == 1, result.output + env = _envelope(result.output) + assert env["error"]["code"] == "execution_error" + assert env["error"]["message"] == "node 5 exploded" + assert "error" not in env["error"]["details"] + def test_an_unmarked_server_record_still_resolves(self, monkeypatch): monkeypatch.setattr(jobs_state, "read", lambda _: None) monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: {"client_id": "cli-minted"}) From ec887c378d95c86601685a51d6d942a1524575c8 Mon Sep 17 00:00:00 2001 From: bymyself Date: Sun, 4 Oct 2026 05:54:48 -0700 Subject: [PATCH 10/22] fix(jobs): make borrowed client selection atomic --- comfy_cli/command/jobs.py | 17 +++++++++++++++-- tests/comfy_cli/command/test_run_client_id.py | 16 ++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/comfy_cli/command/jobs.py b/comfy_cli/command/jobs.py index 2037db456..230f5a2da 100644 --- a/comfy_cli/command/jobs.py +++ b/comfy_cli/command/jobs.py @@ -1974,11 +1974,24 @@ def _select_watch_client_id(host: str, port: int, prompt_id: str, requested: str prompt exists specifically to feed another live socket; attaching under any requested identity would defeat the safe poll-only behavior and can evict that live client when the caller supplies its id. + + Read the state file and server record once each. Looking up the marker and + client id in separate requests leaves a queue-to-history transition between + them where an explicit borrowed id can be accepted after the marker vanishes + from the first endpoint but before it appears in the second. """ - withheld = _client_id_withheld(host, port, prompt_id) + try: + job = jobs_state.read(prompt_id) + except (ValueError, OSError): + job = None + extra = _submitted_extra_data(host, port, prompt_id) + withheld = bool(job is not None and job.client_id_borrowed) or _borrowed_from_extra_data(extra) if withheld and requested: raise ValueError("a borrowed-client run is poll-only") - return requested or _resolve_watch_client_id(host, port, prompt_id), withheld + if withheld: + return None, True + recorded = job.client_id if job is not None and isinstance(job.client_id, str) and job.client_id.strip() else None + return requested or recorded or _client_id_from_extra_data(extra), False def _history_completed_nodes(host: str, port: int, prompt_id: str) -> set[str]: diff --git a/tests/comfy_cli/command/test_run_client_id.py b/tests/comfy_cli/command/test_run_client_id.py index c313e33d0..064853b93 100644 --- a/tests/comfy_cli/command/test_run_client_id.py +++ b/tests/comfy_cli/command/test_run_client_id.py @@ -265,6 +265,22 @@ def test_explicit_override_cannot_bypass_borrowed_marker(self, monkeypatch): with pytest.raises(ValueError, match="poll-only"): _select_watch_client_id("127.0.0.1", 8188, "p", BORROWED) + def test_selection_reads_server_marker_once(self, monkeypatch): + monkeypatch.setattr(jobs_state, "read", lambda _: None) + reads = 0 + + def server_extra(*_args): + nonlocal reads + reads += 1 + if reads > 1: + raise AssertionError("selection split marker and client-id reads") + return {"client_id": BORROWED, jobs_state.BORROWED_CLIENT_ID_KEY: True} + + monkeypatch.setattr(jobs, "_submitted_extra_data", server_extra) + with pytest.raises(ValueError, match="poll-only"): + _select_watch_client_id("127.0.0.1", 8188, "p", BORROWED) + assert reads == 1 + def test_watch_rejects_explicit_override_before_opening_a_socket(self, monkeypatch): monkeypatch.setattr(jobs, "_server_or_error", lambda *_args, **_kwargs: True) monkeypatch.setattr( From 5dff0c62243df6fb39098eb67232c6255330df9c Mon Sep 17 00:00:00 2001 From: Glary-Bot Date: Tue, 6 Oct 2026 20:10:40 +0000 Subject: [PATCH 11/22] fix(jobs): don't read a failed lookup as an unmarked client id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `jobs watch` refuses to attach to a run that borrowed a live client's socket, reading the marker from the job state file and from the extra_data the server echoes back. Both can be unavailable at once: the state file is routinely invisible to the watcher (the in-app agent submits under a sandboxed HOME it deletes at turn end), and every `_http_get_json` failure — timeout, refused, oversize, non-JSON — collapsed to None, which `_borrowed_from_extra_data` then reported as "no marker". A transient blip therefore made a borrowed run look ordinary, and an explicit `--client-id` was attached: the exact eviction the marker exists to prevent, against the safety property the docstring states. Absence of the marker is evidence only once somewhere it could have been recorded actually answered. `_submitted_extra_data` now returns (extra_data, conclusive): a positive hit in either store is enough, a negative needs both endpoints to have answered, and a wrongly shaped body or a matched entry with no extra_data slot is not an answer. When nothing settles ownership the watch polls under its own `indeterminate` reason — not `borrowed`, which would misreport the cause and give the wrong advice ("drop --client-id" vs "retry") — and an explicit override is refused rather than honoured. A readable state file still settles it on its own, so the common case is untouched. This mirrors `_server_confirms_no_record`, which already demands a positive confirmation rather than trusting an absence of evidence. `_http_get_json` promised that every failure leaves as a RuntimeError, the single family every call site catches, but urllib only wraps what the request raises: a server that accepts the connection and hangs up surfaces a raw http.client.RemoteDisconnected out of getresponse(), which crashed `jobs watch` mid-decision with a traceback. Widen the net to OSError (URLError is a subclass) plus HTTPException for the siblings that are not OSError at all. Fold `_resolve_watch_client_id` and `_client_id_withheld` into `_select_watch_client_id`. Both lost their last production call site when selection became atomic, leaving tests that monkeypatched the former to steer a watch path that no longer called it, and both carried the same flaw: bool and str|None cannot distinguish borrowed from indeterminate. --- comfy_cli/command/jobs.py | 247 +++++++++++------- comfy_cli/error_codes.py | 7 +- docs/json-output.md | 9 +- tests/comfy_cli/command/test_run_client_id.py | 230 ++++++++++++---- tests/comfy_cli/jobs/test_jobs.py | 94 +++++-- 5 files changed, 422 insertions(+), 165 deletions(-) diff --git a/comfy_cli/command/jobs.py b/comfy_cli/command/jobs.py index 230f5a2da..7504c917c 100644 --- a/comfy_cli/command/jobs.py +++ b/comfy_cli/command/jobs.py @@ -11,7 +11,7 @@ ``execution.py``, delivered by ``PromptServer.send_json`` only to that one ``sid``). A watcher that connects with a *fresh* ``clientId`` therefore receives nothing at all — so ``jobs watch`` re-attaches with the submitting client_id -(see ``_resolve_watch_client_id``). +(see ``_select_watch_client_id``). Three subcommands: @@ -27,6 +27,7 @@ from __future__ import annotations +import http.client import json import time import urllib.error @@ -35,7 +36,7 @@ import uuid from dataclasses import dataclass, field, replace from datetime import datetime, timezone -from typing import TYPE_CHECKING, Annotated, Any +from typing import TYPE_CHECKING, Annotated, Any, Literal import typer from websocket import WebSocket, WebSocketException, WebSocketTimeoutException @@ -156,12 +157,21 @@ def _http_get_json(url: str, *, timeout: float = 10.0) -> Any: ``/history`` can't OOM the CLI. Every failure — unreachable, oversize, non-JSON — leaves as a ``RuntimeError``, which is the single family every call site below already catches. + + That promise is why the net is ``OSError`` rather than ``URLError`` (a + subclass of it). urllib only wraps what the *request* raises; a server that + accepts the connection and hangs up before answering surfaces as a raw + ``http.client.RemoteDisconnected`` out of ``getresponse()``, which is an + ordinary transient — a restarting ComfyUI, a reset connection — and has no + business reaching a caller as an uncaught traceback. ``HTTPException`` + covers its siblings that are not ``OSError`` at all (``IncompleteRead``, + ``BadStatusLine``). """ req = urllib.request.Request(url) try: with plain_urlopen(req, timeout=timeout) as resp: return json.loads(read_capped(resp, url)) - except urllib.error.URLError as e: + except (OSError, http.client.HTTPException) as e: raise RuntimeError(f"failed to GET {url}: {e}") from e except ResponseTooLarge as e: raise RuntimeError(str(e)) from e @@ -1876,122 +1886,153 @@ def _borrowed_from_extra_data(extra: Any) -> bool: return isinstance(extra, dict) and extra.get(jobs_state.BORROWED_CLIENT_ID_KEY) is True -def _client_id_withheld(host: str, port: int, prompt_id: str) -> bool: - """True if this prompt's client_id is one we deliberately refuse to attach - as — a run submitted with ``comfy run --client-id``. +# Why a watch polls instead of attaching. `borrowed` is a positive finding: the +# prompt carries the marker. `indeterminate` is the absence of one we could not +# confirm — see `_select_watch_client_id`. They are not interchangeable: only +# the first licenses "drop --client-id", the second asks for a retry. +_PollReason = Literal["borrowed", "indeterminate"] + + +class _ClientIdRejected(ValueError): + """An explicit ``--client-id`` the watch must not honour, and why. - Distinguishes the two ways ``_resolve_watch_client_id`` returns None, which - want opposite advice: an unresolvable id is worth passing ``--client-id`` - for, a withheld one must never be. + Subclasses ValueError so the reason rides the exception rather than being + re-derived at the call site, where the second lookup it would need could + disagree with the one that made the decision. """ - try: - job = jobs_state.read(prompt_id) - except (ValueError, OSError): - job = None - if job is not None and job.client_id_borrowed: - return True - return _borrowed_from_extra_data(_submitted_extra_data(host, port, prompt_id)) + + def __init__(self, reason: _PollReason) -> None: + super().__init__(reason) + self.reason: _PollReason = reason -# How long a withheld watch waits on its (deliberately eventless) socket before -# reconciling. The ordinary `--timeout` is a silence budget for a socket that -# SHOULD be receiving events; here nothing ever will, so it would be dead air. -# This raises the reconcile cadence for the whole watch, not just the first -# read, which is affordable against a loopback/LAN ComfyUI. -_WITHHELD_WATCH_POLL_S = 2.0 +# How long a poll-only watch waits before reconciling. The ordinary `--timeout` +# is a silence budget for a socket that SHOULD be receiving events; here there +# is no socket at all, so it would be dead air. This raises the reconcile +# cadence for the whole watch, not just the first read, which is affordable +# against a loopback/LAN ComfyUI. +_POLL_ONLY_WATCH_POLL_S = 2.0 -def _submitted_extra_data(host: str, port: int, prompt_id: str) -> Any: - """This prompt's ``extra_data`` as the SERVER has it: from ``/queue`` while - it is pending or running, then ``/history`` once it has finished.""" +def _get_json_object(url: str) -> tuple[dict[str, Any], bool]: + """GET a JSON **object**, reporting whether the server actually answered with one. + + Separates "the server told us" from "we failed to ask", which a bare + ``{}``/``None`` fallback destroys. A 200 carrying valid JSON of the wrong + shape (a proxy error page, a captive portal) is a non-answer too: it cannot + be read as the server reporting an empty queue. + """ try: - q = _http_get_json(f"http://{host}:{port}/queue") + body = _http_get_json(url) except RuntimeError: - q = {} - if isinstance(q, dict): + return {}, False + return (body, True) if isinstance(body, dict) else ({}, False) + + +def _submitted_extra_data(host: str, port: int, prompt_id: str) -> tuple[Any, bool]: + """This prompt's ``extra_data`` as the SERVER has it, and whether that answer + is CONCLUSIVE. + + Reads ``/queue`` while the prompt is pending or running, then ``/history`` + once it has finished. The second element is the part that matters for + safety: a caller deciding whether a prompt is marked borrowed must not read + a failed fetch as "no marker". Mirrors ``_server_confirms_no_record`` — a + negative needs a positive confirmation, never an absence of evidence. + + Conclusive when either store positively yielded the prompt's ``extra_data`` + slot (one endpoint is enough — that IS the submitted record), or both + answered and neither holds the prompt. Inconclusive when a fetch failed, a + body came back the wrong shape, or the matched entry's ``extra_data`` slot + was missing: a prompt absent from a ``/history`` we could read may still be + live in a ``/queue`` we could not. + """ + queue, queue_answered = _get_json_object(f"http://{host}:{port}/queue") + if queue_answered: for key in ("queue_running", "queue_pending"): - for entry in q.get(key) or []: + entries = queue.get(key) + if entries is None: + continue + if not isinstance(entries, list): + # Can't walk it, so can't conclude the prompt is absent from it. + queue_answered = False + continue + for entry in entries: # entry layout: number, prompt_id, prompt, extra_data, outputs_to_execute - if isinstance(entry, list) and len(entry) > 3 and entry[1] == prompt_id: - return entry[3] + if isinstance(entry, list) and len(entry) > 1 and entry[1] == prompt_id: + return (entry[3], True) if len(entry) > 3 else (None, False) - try: - h = _http_get_json(f"http://{host}:{port}/history/{prompt_id}") - except RuntimeError: - return None - body = h.get(prompt_id) if isinstance(h, dict) else None - prompt = body.get("prompt") if isinstance(body, dict) else None - if isinstance(prompt, list) and len(prompt) > 3: - return prompt[3] - return None + history, history_answered = _get_json_object(f"http://{host}:{port}/history/{prompt_id}") + if history_answered and prompt_id in history: + body = history[prompt_id] + prompt = body.get("prompt") if isinstance(body, dict) else None + if isinstance(prompt, list) and len(prompt) > 3: + return prompt[3], True + return None, False + + return None, queue_answered and history_answered -def _resolve_watch_client_id(host: str, port: int, prompt_id: str) -> str | None: - """Find the ``client_id`` this prompt was submitted with, or None. +def _select_watch_client_id( + host: str, port: int, prompt_id: str, requested: str | None +) -> tuple[str | None, _PollReason | None]: + """Pick the socket identity to attach as, or a reason to poll instead. ComfyUI delivers execution events only to the submitting session's socket (see the module docstring), so watching with a fresh id yields silence. The - submitting id is recoverable from three places, cheapest first: - - 1. our own on-disk job state, written at submit time by ``comfy run``; - 2. ``/queue`` ``extra_data`` while the prompt is running or pending; - 3. ``/history`` ``prompt[3]`` once it has finished. + submitting id is recoverable from our own on-disk job state, written at + submit time by ``comfy run``, and from the ``extra_data`` the server echoes + back through ``/queue`` and ``/history``. Reconnecting under an existing id is ComfyUI's own session-resume mechanism (its ``/ws`` handler pops the previous socket for that id), which is exactly what we want for a prompt whose submitter has exited — the common case, since ``comfy run --async`` returns immediately. The caveat is that a submitter still holding that socket (a browser tab, a blocking ``comfy run``) stops - receiving events until it reconnects; pass ``--client-id`` to override the - resolution when that matters. + receiving events until it reconnects. A run submitted via ``comfy run --client-id`` is the one case where that caveat is a certainty rather than a risk: the recorded id belongs to a live - client that is being fed on purpose. Those resolve to None — at every step, - because ``run`` stamps the marker into the submitted ``extra_data`` as well - as the state file — so the watch polls status instead of displacing a tab - that would then receive nothing until the page is reloaded. + client that is being fed on purpose. Those poll instead, and an explicit + override is refused rather than honoured — a marked prompt exists precisely + to feed another live socket, so attaching under the caller's id performs the + eviction the marker exists to prevent. + + **Absence of that marker is evidence only once every place it could have + been read answered.** A failed read is not an unmarked record: the state + file is routinely invisible to the watcher (submitter and watcher run under + different config roots — the in-app agent uses a sandboxed ``HOME`` it + deletes at turn end), so a transient ``/queue`` + ``/history`` failure would + otherwise leave a borrowed run looking ordinary and let an explicit + ``--client-id`` evict the live client. Unverifiable ownership polls too, and + says so with its own reason, because the alternatives are asymmetric: a + retry costs a moment, a wrong attach silences the tab the run was for. + + Reads the state file and the server record once each. Looking up the marker + and the client id separately leaves a queue-to-history transition between + them where an explicit borrowed id can be accepted after the marker vanishes + from the first endpoint but before it appears in the second. """ try: job = jobs_state.read(prompt_id) except (ValueError, OSError): # unsafe prompt_id / unreadable state dir job = None - if job is not None and job.client_id_borrowed: - return None - if job is not None and isinstance(job.client_id, str) and job.client_id.strip(): - return job.client_id + extra, server_conclusive = _submitted_extra_data(host, port, prompt_id) - extra = _submitted_extra_data(host, port, prompt_id) - if _borrowed_from_extra_data(extra): - return None - return _client_id_from_extra_data(extra) - - -def _select_watch_client_id(host: str, port: int, prompt_id: str, requested: str | None) -> tuple[str | None, bool]: - """Select a safe socket identity and report whether the submitter was withheld. + if (job is not None and job.client_id_borrowed) or _borrowed_from_extra_data(extra): + if requested: + raise _ClientIdRejected("borrowed") + return None, "borrowed" - An explicit override must not bypass the borrowed-client marker. A marked - prompt exists specifically to feed another live socket; attaching under any - requested identity would defeat the safe poll-only behavior and can evict - that live client when the caller supplies its id. + # A state file is its own positive evidence: `comfy run` wrote it, and it + # says this run did not borrow. Only when it is absent does the server's + # record have to carry the answer on its own. + if job is None and not server_conclusive: + if requested: + raise _ClientIdRejected("indeterminate") + return None, "indeterminate" - Read the state file and server record once each. Looking up the marker and - client id in separate requests leaves a queue-to-history transition between - them where an explicit borrowed id can be accepted after the marker vanishes - from the first endpoint but before it appears in the second. - """ - try: - job = jobs_state.read(prompt_id) - except (ValueError, OSError): - job = None - extra = _submitted_extra_data(host, port, prompt_id) - withheld = bool(job is not None and job.client_id_borrowed) or _borrowed_from_extra_data(extra) - if withheld and requested: - raise ValueError("a borrowed-client run is poll-only") - if withheld: - return None, True recorded = job.client_id if job is not None and isinstance(job.client_id, str) and job.client_id.strip() else None - return requested or recorded or _client_id_from_extra_data(extra), False + return requested or recorded or _client_id_from_extra_data(extra), None def _history_completed_nodes(host: str, port: int, prompt_id: str) -> set[str]: @@ -2281,20 +2322,30 @@ def watch_cmd( # Re-attach as the submitting session, otherwise the server addresses every # execution event to a socket we are not holding and this watch sees nothing. try: - attached_client_id, withheld = _select_watch_client_id(h, p, prompt_id, client_id) - except ValueError: + attached_client_id, poll_reason = _select_watch_client_id(h, p, prompt_id, client_id) + except _ClientIdRejected as e: + if e.reason == "borrowed": + message = "This run borrowed another client's live socket and can only be watched by polling" + hint = "drop --client-id; comfy jobs watch will poll this run safely" + else: + message = ( + "Could not verify that --client-id is safe to attach as: the server did not answer " + "the lookup that says whether this run borrowed a live client's socket" + ) + hint = "retry the command, or drop --client-id; comfy jobs watch will poll this run safely" renderer.error( code="client_id_rejected", - message="This run borrowed another client's live socket and can only be watched by polling", - hint="drop --client-id; comfy jobs watch will poll this run safely", - details={"reason": "borrowed"}, + message=message, + hint=hint, + details={"reason": e.reason}, ) raise typer.Exit(code=1) + poll_only = poll_reason is not None token = cancellation.get_token() ws = None ws_client_id = None - if not withheld: + if not poll_only: ws = WebSocket() ws_client_id = attached_client_id or str(uuid.uuid4()) try: @@ -2316,12 +2367,18 @@ def watch_cmd( if renderer.is_pretty(): renderer.console().print(f"[bold]Watching prompt[/bold] {prompt_id} on {h}:{p} [dim](Ctrl-C to stop)[/dim]") - if withheld: + if poll_reason == "borrowed": renderer.console().print( "[yellow]![/yellow] [dim]this run was submitted for another client (comfy run " "--client-id), which is still receiving its live events; attaching as it would cut " "it off, so this watch polls status instead[/dim]" ) + elif poll_reason == "indeterminate": + renderer.console().print( + "[yellow]![/yellow] [dim]could not verify whether this run was submitted for another " + "client — the server did not answer that lookup, so no socket was opened and this " + "watch polls status instead; retry for live events[/dim]" + ) elif attached_client_id is None: renderer.console().print( "[yellow]![/yellow] [dim]could not resolve the submitting client_id — the server " @@ -2331,9 +2388,9 @@ def watch_cmd( try: while True: raw = None - poll_now = withheld - if withheld: - if token.wait(_WITHHELD_WATCH_POLL_S): + poll_now = poll_only + if poll_only: + if token.wait(_POLL_ONLY_WATCH_POLL_S): state.end_reason = "cancelled" break else: diff --git a/comfy_cli/error_codes.py b/comfy_cli/error_codes.py index 221efdcec..360cc6d5a 100644 --- a/comfy_cli/error_codes.py +++ b/comfy_cli/error_codes.py @@ -319,10 +319,13 @@ class ErrorCode: "per user/workspace regardless of submitter) and `wait` (`--wait` would watch a socket this " "run's events are deliberately not sent to); on `comfy jobs watch`, `borrowed` (the prompt is " "marked as having borrowed a live client's id, so it is poll-only and no override is " - "honoured). Refused before anything is submitted or any socket is opened.", + "honoured) and `indeterminate` (the server did not answer the lookup that reads that marker, " + "so the run could not be shown to be safe to attach to — a failed read is not an unmarked " + "record, and is retryable). Refused before anything is submitted or any socket is opened.", "pass a non-empty id, or drop the conflicting flag: `--client-id` cannot be combined with " "`--wait` or a cloud target, and `comfy jobs watch` on a borrowed run takes no `--client-id` " - "at all — poll it with `comfy jobs status`", + "at all — poll it with `comfy jobs status`; on `indeterminate`, retry once the server answers " + "again, or drop `--client-id` to poll safely now", ), ErrorCode( "host_flag_cloud", diff --git a/docs/json-output.md b/docs/json-output.md index 084b8c107..ff0e57d5d 100644 --- a/docs/json-output.md +++ b/docs/json-output.md @@ -34,7 +34,12 @@ id belongs to a live client being fed on purpose. Resolution withholds it deliberately (`attached: false`) and the watch polls status instead; passing `--client-id` on such a run is refused with `client_id_rejected` (`reason: "borrowed"`) before any socket opens, so there is no way to take that -client off its own socket. Both +client off its own socket. That marker is read from the state file *and* from the +`extra_data` the server echoes back, and when neither can be reached — a failed +`/queue` + `/history` read with no local state file — the run is not assumed +unmarked: the watch polls (`attached: false`) and an explicit `--client-id` is +refused with `reason: "indeterminate"`, which a retry clears once the server +answers again. Both `data.client_id` and `data.attached` are present on *every* terminal envelope: a watch of an already-finished prompt short-circuits without opening a socket, and reports `client_id: null` / `attached: false`. Reconnecting under an existing id @@ -643,7 +648,7 @@ lists what the cloud adds and which of the codes below cannot occur there. | `workflow_unknown_nodes` | Pre-submit validation found unknown class_types / shape mismatches | `errors` (array), `warnings` (array) | 1 | | `partner_node_requires_credential` | Workflow uses a partner-API node and no `api_key_comfy_org` credential is available | `partner_nodes` (array of str, capped at 20 entries × 64 chars each), `partner_node_count` (int, the exact total — read this, not `len(partner_nodes)`), `host`, `port` | 1 | | `spend_consent_required` | Workflow embeds partner-API (paid) nodes and `--allow-spend` was not passed (machine mode) or interactive consent was declined; re-run with `--allow-spend`. Free (non-partner) workflows are unaffected. | `partner_nodes` (array of str, capped at 20 entries × 64 chars each), `partner_node_count` (int, the exact total — read this, not `len(partner_nodes)`); local path also carries `host`, `port`, the cloud path carries `where: "cloud"` | 1 | -| `client_id_rejected` | `--client-id` was combined with something it cannot serve. On `comfy run`: the flag was blank (`empty`), the effective target is cloud (`cloud` — the flag addresses a local ComfyUI socket, and cloud fans execution events out per user/workspace anyway), or `--wait` was passed (`wait` — the run's events go to the named client, so this invocation would watch a silent socket). Also raised by `comfy jobs watch` when the prompt is marked as having borrowed a live client's id (`borrowed`): that run is poll-only and no `--client-id` override is honoured. Refused before anything is submitted and before any socket is opened | `reason` (str, `"empty"`, `"cloud"`, `"wait"` or `"borrowed"`) | 1 | +| `client_id_rejected` | `--client-id` was combined with something it cannot serve. On `comfy run`: the flag was blank (`empty`), the effective target is cloud (`cloud` — the flag addresses a local ComfyUI socket, and cloud fans execution events out per user/workspace anyway), or `--wait` was passed (`wait` — the run's events go to the named client, so this invocation would watch a silent socket). Also raised by `comfy jobs watch` when the prompt is marked as having borrowed a live client's id (`borrowed`): that run is poll-only and no `--client-id` override is honoured. `comfy jobs watch` raises it with `indeterminate` when the `/queue` + `/history` read that carries that marker failed and no job state file was readable either: absence of the marker is only evidence once somewhere it could have been recorded actually answered, so the override is refused rather than risking the eviction — retry once the server answers. Refused before anything is submitted and before any socket is opened | `reason` (str, `"empty"`, `"cloud"`, `"wait"`, `"borrowed"` or `"indeterminate"`) | 1 | | `prompt_rejected` | Server returned HTTP 400 with `node_errors` | `status` (400), `node_errors` (array — [shape](#node_errors-shape)) | 1 | | `client_error` | Server returned another HTTP 4xx response (including 429; `cloud_rate_limited` is Cloud only) | `status` (int, 4xx), `body` (str) | 1 | | `server_error` | Server returned an HTTP 5xx response | `status` (int, 5xx), `body` (str) | 1 | diff --git a/tests/comfy_cli/command/test_run_client_id.py b/tests/comfy_cli/command/test_run_client_id.py index 064853b93..21b14af68 100644 --- a/tests/comfy_cli/command/test_run_client_id.py +++ b/tests/comfy_cli/command/test_run_client_id.py @@ -24,7 +24,7 @@ from comfy_cli import jobs_state from comfy_cli.cmdline import run as run_command from comfy_cli.command import jobs -from comfy_cli.command.jobs import _client_id_withheld, _resolve_watch_client_id, _select_watch_client_id +from comfy_cli.command.jobs import _ClientIdRejected, _select_watch_client_id, _submitted_extra_data from comfy_cli.command.run import WorkflowExecution, execute from comfy_cli.output import Renderer, set_renderer from comfy_cli.output.renderer import OutputMode, reset_renderer_for_testing @@ -193,13 +193,22 @@ def test_marker_is_submitted_with_the_prompt(self, simple_workflow, borrowed): assert extra.get(jobs_state.BORROWED_CLIENT_ID_KEY) is (True if borrowed else None) +def _marked(**extra): + """A server record for a prompt submitted by `comfy run --client-id`.""" + return {"client_id": BORROWED, jobs_state.BORROWED_CLIENT_ID_KEY: True, **extra} + + class TestWatchDoesNotStealTheSocket: @pytest.fixture(autouse=True) - def no_server(self, monkeypatch): - """Default: the server knows nothing. Tests that care override it.""" - monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: None) + def answered_empty_server(self, monkeypatch): + """Default: the server ANSWERED and holds no record of this prompt. - def test_borrowed_record_resolves_to_no_client_id(self, monkeypatch): + Conclusive, which is a different thing from a read that failed — the + class below pins that difference. + """ + monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: (None, True)) + + def test_borrowed_record_yields_no_client_id(self, monkeypatch): state = jobs_state.new( prompt_id="p", client_id=BORROWED, @@ -210,7 +219,7 @@ def test_borrowed_record_resolves_to_no_client_id(self, monkeypatch): client_id_borrowed=True, ) monkeypatch.setattr(jobs_state, "read", lambda _: state) - assert _resolve_watch_client_id("127.0.0.1", 8188, "p") is None + assert _select_watch_client_id("127.0.0.1", 8188, "p", None) == (None, "borrowed") def test_minted_record_still_resolves(self, monkeypatch): state = jobs_state.new( @@ -222,13 +231,13 @@ def test_minted_record_still_resolves(self, monkeypatch): port=8188, ) monkeypatch.setattr(jobs_state, "read", lambda _: state) - assert _resolve_watch_client_id("127.0.0.1", 8188, "p") == "cli-minted" + assert _select_watch_client_id("127.0.0.1", 8188, "p", None) == ("cli-minted", None) - # Both reasons `_resolve_watch_client_id` answers None want OPPOSITE advice: - # an unresolvable id is worth passing --client-id for, a withheld one must + # The two reasons a watch ends up with no id want OPPOSITE advice: an + # unresolvable id is worth passing --client-id for, a borrowed one must # never be, because doing so performs the eviction the guard exists to stop. @pytest.mark.parametrize("borrowed", [True, False]) - def test_withheld_is_distinguishable_from_unresolvable(self, monkeypatch, borrowed): + def test_borrowed_is_distinguishable_from_unresolvable(self, monkeypatch, borrowed): state = jobs_state.new( prompt_id="p", client_id=BORROWED, @@ -237,33 +246,26 @@ def test_withheld_is_distinguishable_from_unresolvable(self, monkeypatch, borrow client_id_borrowed=borrowed, ) monkeypatch.setattr(jobs_state, "read", lambda _: state) - assert _client_id_withheld("127.0.0.1", 8188, "p") is borrowed + _cid, reason = _select_watch_client_id("127.0.0.1", 8188, "p", None) + assert (reason == "borrowed") is borrowed - def test_no_state_file_is_not_withheld(self, monkeypatch): + def test_an_answered_empty_server_is_attachable(self, monkeypatch): monkeypatch.setattr(jobs_state, "read", lambda _: None) - assert _client_id_withheld("127.0.0.1", 8188, "p") is False + assert _select_watch_client_id("127.0.0.1", 8188, "p", None) == (None, None) # The case the state-file-only guard missed entirely: a watcher that cannot # see the submitting run's state still has to refuse the id. def test_server_marker_is_honoured_without_a_state_file(self, monkeypatch): monkeypatch.setattr(jobs_state, "read", lambda _: None) - monkeypatch.setattr( - jobs, - "_submitted_extra_data", - lambda *_: {"client_id": BORROWED, jobs_state.BORROWED_CLIENT_ID_KEY: True}, - ) - assert _resolve_watch_client_id("127.0.0.1", 8188, "p") is None - assert _client_id_withheld("127.0.0.1", 8188, "p") is True + monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: (_marked(), True)) + assert _select_watch_client_id("127.0.0.1", 8188, "p", None) == (None, "borrowed") def test_explicit_override_cannot_bypass_borrowed_marker(self, monkeypatch): monkeypatch.setattr(jobs_state, "read", lambda _: None) - monkeypatch.setattr( - jobs, - "_submitted_extra_data", - lambda *_: {"client_id": BORROWED, jobs_state.BORROWED_CLIENT_ID_KEY: True}, - ) - with pytest.raises(ValueError, match="poll-only"): + monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: (_marked(), True)) + with pytest.raises(_ClientIdRejected) as excinfo: _select_watch_client_id("127.0.0.1", 8188, "p", BORROWED) + assert excinfo.value.reason == "borrowed" def test_selection_reads_server_marker_once(self, monkeypatch): monkeypatch.setattr(jobs_state, "read", lambda _: None) @@ -274,10 +276,10 @@ def server_extra(*_args): reads += 1 if reads > 1: raise AssertionError("selection split marker and client-id reads") - return {"client_id": BORROWED, jobs_state.BORROWED_CLIENT_ID_KEY: True} + return _marked(), True monkeypatch.setattr(jobs, "_submitted_extra_data", server_extra) - with pytest.raises(ValueError, match="poll-only"): + with pytest.raises(_ClientIdRejected): _select_watch_client_id("127.0.0.1", 8188, "p", BORROWED) assert reads == 1 @@ -289,11 +291,7 @@ def test_watch_rejects_explicit_override_before_opening_a_socket(self, monkeypat lambda *_: {"prompt_id": "p", "status": "running", "outputs": []}, ) monkeypatch.setattr(jobs_state, "read", lambda _: None) - monkeypatch.setattr( - jobs, - "_submitted_extra_data", - lambda *_: {"client_id": BORROWED, jobs_state.BORROWED_CLIENT_ID_KEY: True}, - ) + monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: (_marked(), True)) def fail_if_constructed(*_args, **_kwargs): raise AssertionError("borrowed watch constructed a WebSocket") @@ -319,13 +317,9 @@ def test_watch_polls_borrowed_run_without_opening_a_socket(self, monkeypatch): monkeypatch.setattr(jobs, "_server_or_error", lambda *_args, **_kwargs: True) monkeypatch.setattr(jobs, "_snapshot", lambda *_: next(snapshots)) monkeypatch.setattr(jobs, "_history_completed_nodes", lambda *_: {"1"}) - monkeypatch.setattr(jobs, "_WITHHELD_WATCH_POLL_S", 0) + monkeypatch.setattr(jobs, "_POLL_ONLY_WATCH_POLL_S", 0) monkeypatch.setattr(jobs_state, "read", lambda _: None) - monkeypatch.setattr( - jobs, - "_submitted_extra_data", - lambda *_: {"client_id": BORROWED, jobs_state.BORROWED_CLIENT_ID_KEY: True}, - ) + monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: (_marked(), True)) def fail_if_constructed(*_args, **_kwargs): raise AssertionError("borrowed watch constructed a WebSocket") @@ -356,13 +350,9 @@ def test_borrowed_poll_preserves_a_failed_runs_error(self, monkeypatch): monkeypatch.setattr(jobs, "_server_or_error", lambda *_args, **_kwargs: True) monkeypatch.setattr(jobs, "_snapshot", lambda *_: next(snapshots)) monkeypatch.setattr(jobs, "_history_completed_nodes", lambda *_: set()) - monkeypatch.setattr(jobs, "_WITHHELD_WATCH_POLL_S", 0) + monkeypatch.setattr(jobs, "_POLL_ONLY_WATCH_POLL_S", 0) monkeypatch.setattr(jobs_state, "read", lambda _: None) - monkeypatch.setattr( - jobs, - "_submitted_extra_data", - lambda *_: {"client_id": BORROWED, jobs_state.BORROWED_CLIENT_ID_KEY: True}, - ) + monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: (_marked(), True)) monkeypatch.setattr( jobs, "WebSocket", @@ -379,6 +369,150 @@ def test_borrowed_poll_preserves_a_failed_runs_error(self, monkeypatch): def test_an_unmarked_server_record_still_resolves(self, monkeypatch): monkeypatch.setattr(jobs_state, "read", lambda _: None) - monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: {"client_id": "cli-minted"}) - assert _resolve_watch_client_id("127.0.0.1", 8188, "p") == "cli-minted" - assert _client_id_withheld("127.0.0.1", 8188, "p") is False + monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: ({"client_id": "cli-minted"}, True)) + assert _select_watch_client_id("127.0.0.1", 8188, "p", None) == ("cli-minted", None) + + +class TestUnverifiableOwnershipFailsSafe: + """A failed read is not an unmarked record. + + The guard above reads the borrowed marker from two places, and BOTH can be + unavailable at once: the state file is routinely invisible to the watcher + (the in-app agent submits under a sandboxed HOME it deletes at turn end), + and `/queue` + `/history` can blip. Treating that as "no marker" let an + explicit --client-id attach to a run that may well be feeding a live tab — + the exact eviction the marker exists to prevent. Absence of the marker is + evidence only once somewhere it could have been recorded actually answered. + """ + + @pytest.fixture(autouse=True) + def unverifiable(self, monkeypatch): + monkeypatch.setattr(jobs_state, "read", lambda _: None) + monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: (None, False)) + + def test_explicit_override_is_refused_rather_than_attached(self): + with pytest.raises(_ClientIdRejected) as excinfo: + _select_watch_client_id("127.0.0.1", 8188, "p", BORROWED) + # NOT "borrowed": we never established that, and the advice differs — + # a borrowed run says "drop --client-id", this one says "retry". + assert excinfo.value.reason == "indeterminate" + + def test_no_override_polls_rather_than_attaching(self): + assert _select_watch_client_id("127.0.0.1", 8188, "p", None) == (None, "indeterminate") + + def test_a_readable_state_file_still_settles_it(self, monkeypatch): + """`comfy run` wrote that file and recorded that it did not borrow, so a + server that cannot be reached does not make the run unverifiable.""" + state = jobs_state.new(prompt_id="p", client_id="cli-minted", workflow="w.json", where="local") + monkeypatch.setattr(jobs_state, "read", lambda _: state) + assert _select_watch_client_id("127.0.0.1", 8188, "p", None) == ("cli-minted", None) + + def test_watch_refuses_the_override_before_opening_a_socket(self, monkeypatch): + monkeypatch.setattr(jobs, "_server_or_error", lambda *_args, **_kwargs: True) + monkeypatch.setattr(jobs, "_snapshot", lambda *_: {"prompt_id": "p", "status": "running", "outputs": []}) + + def fail_if_constructed(*_args, **_kwargs): + raise AssertionError("unverifiable watch constructed a WebSocket") + + monkeypatch.setattr(jobs, "WebSocket", fail_if_constructed) + result = CliRunner().invoke(jobs.app, ["watch", "p", "--where", "local", "--client-id", BORROWED]) + + assert result.exit_code == 1, result.output + env = _envelope(result.output) + assert env["error"]["code"] == "client_id_rejected" + assert env["error"]["details"]["reason"] == "indeterminate" + assert "retry" in env["error"]["hint"] + + def test_watch_polls_without_opening_a_socket(self, monkeypatch): + snapshots = iter( + [ + {"prompt_id": "p", "status": "running", "outputs": []}, + {"prompt_id": "p", "status": "completed", "outputs": ["result.png"]}, + ] + ) + monkeypatch.setattr(jobs, "_server_or_error", lambda *_args, **_kwargs: True) + monkeypatch.setattr(jobs, "_snapshot", lambda *_: next(snapshots)) + monkeypatch.setattr(jobs, "_history_completed_nodes", lambda *_: {"1"}) + monkeypatch.setattr(jobs, "_POLL_ONLY_WATCH_POLL_S", 0) + + def fail_if_constructed(*_args, **_kwargs): + raise AssertionError("unverifiable watch constructed a WebSocket") + + monkeypatch.setattr(jobs, "WebSocket", fail_if_constructed) + result = CliRunner().invoke(jobs.app, ["watch", "p", "--where", "local"]) + + assert result.exit_code == 0, result.output + env = _envelope(result.output) + assert env["data"]["status"] == "completed" + assert env["data"]["client_id"] is None + assert env["data"]["attached"] is False + + +class TestSubmittedExtraDataReportsConclusiveness: + """`(extra_data, conclusive)` — the second element is the safety-critical one. + + A positive hit in either store IS the submitted record, so one endpoint is + enough. A negative needs BOTH to have answered: a prompt absent from a + `/history` we could read may still be live in a `/queue` we could not. + """ + + @staticmethod + def _server(monkeypatch, *, queue, history): + def fake_get(url, **_kw): + answer = queue if url.endswith("/queue") else history + if isinstance(answer, Exception): + raise answer + return answer + + monkeypatch.setattr(jobs, "_http_get_json", fake_get) + + BOOM = RuntimeError("failed to GET: connection refused") + EMPTY_QUEUE = {"queue_running": [], "queue_pending": []} + QUEUED = {"queue_running": [[0, "p", {}, {"client_id": "cid"}, {}]], "queue_pending": []} + IN_HISTORY = {"p": {"prompt": [0, "p", {}, {"client_id": "cid"}, {}]}} + + def test_a_queue_hit_is_conclusive_even_if_history_is_unreachable(self, monkeypatch): + self._server(monkeypatch, queue=self.QUEUED, history=self.BOOM) + assert _submitted_extra_data("127.0.0.1", 8188, "p") == ({"client_id": "cid"}, True) + + def test_a_history_hit_is_conclusive_even_if_the_queue_was_unreachable(self, monkeypatch): + self._server(monkeypatch, queue=self.BOOM, history=self.IN_HISTORY) + assert _submitted_extra_data("127.0.0.1", 8188, "p") == ({"client_id": "cid"}, True) + + def test_both_answering_empty_is_a_conclusive_absence(self, monkeypatch): + self._server(monkeypatch, queue=self.EMPTY_QUEUE, history={}) + assert _submitted_extra_data("127.0.0.1", 8188, "p") == (None, True) + + def test_an_unreadable_history_is_not_an_absence(self, monkeypatch): + self._server(monkeypatch, queue=self.EMPTY_QUEUE, history=self.BOOM) + assert _submitted_extra_data("127.0.0.1", 8188, "p") == (None, False) + + def test_an_unreadable_queue_is_not_an_absence(self, monkeypatch): + """The prompt may be running right now in the queue we could not read.""" + self._server(monkeypatch, queue=self.BOOM, history={}) + assert _submitted_extra_data("127.0.0.1", 8188, "p") == (None, False) + + def test_neither_answering_is_not_an_absence(self, monkeypatch): + self._server(monkeypatch, queue=self.BOOM, history=self.BOOM) + assert _submitted_extra_data("127.0.0.1", 8188, "p") == (None, False) + + def test_a_wrongly_shaped_body_is_not_an_answer(self, monkeypatch): + """A 200 carrying valid JSON of the wrong shape (a proxy error page, a + captive portal) cannot be read as the server reporting an empty queue.""" + self._server(monkeypatch, queue=["not", "a", "dict"], history={}) + assert _submitted_extra_data("127.0.0.1", 8188, "p") == (None, False) + + def test_an_unwalkable_queue_section_is_not_an_answer(self, monkeypatch): + self._server(monkeypatch, queue={"queue_running": "nonsense"}, history={}) + assert _submitted_extra_data("127.0.0.1", 8188, "p") == (None, False) + + def test_a_matched_entry_with_no_extra_data_slot_is_not_an_answer(self, monkeypatch): + """Found the prompt but cannot read where the marker would live.""" + self._server(monkeypatch, queue={"queue_running": [[0, "p"]]}, history={}) + assert _submitted_extra_data("127.0.0.1", 8188, "p") == (None, False) + + def test_a_marked_record_round_trips(self, monkeypatch): + self._server(monkeypatch, queue={"queue_running": [[0, "p", {}, _marked(), {}]]}, history={}) + extra, conclusive = _submitted_extra_data("127.0.0.1", 8188, "p") + assert conclusive is True + assert extra[jobs_state.BORROWED_CLIENT_ID_KEY] is True diff --git a/tests/comfy_cli/jobs/test_jobs.py b/tests/comfy_cli/jobs/test_jobs.py index 9f9a9ddcf..5bc604874 100644 --- a/tests/comfy_cli/jobs/test_jobs.py +++ b/tests/comfy_cli/jobs/test_jobs.py @@ -6,6 +6,7 @@ from __future__ import annotations +import http.client import json import os import subprocess @@ -3416,21 +3417,23 @@ def test_watch_executing_escapes_server_controlled_node_markup(): # --------------------------------------------------------------------------- -def test_resolve_watch_client_id_prefers_the_job_state_file(monkeypatch): - """`comfy run` records the submitting client_id on disk — cheapest source, - and no HTTP call should be needed when it is there.""" +def test_select_watch_client_id_prefers_the_job_state_file(monkeypatch): + """`comfy run` records the submitting client_id on disk, and that record + wins over whatever the server's copy of extra_data says.""" from comfy_cli import jobs_state jobs_state.write(jobs_state.new(prompt_id="pid-a", client_id="cid-from-state", workflow="w", where="local")) - def _no_http(url, **kw): - raise AssertionError(f"should not have queried the server: {url}") + def fake_get(url, **kw): + if url.endswith("/queue"): + return {"queue_running": [[0, "pid-a", {}, {"client_id": "cid-from-queue"}, {}]]} + raise AssertionError(f"unexpected url: {url}") - monkeypatch.setattr(jobs_mod, "_http_get_json", _no_http) - assert jobs_mod._resolve_watch_client_id("127.0.0.1", 8188, "pid-a") == "cid-from-state" + monkeypatch.setattr(jobs_mod, "_http_get_json", fake_get) + assert jobs_mod._select_watch_client_id("127.0.0.1", 8188, "pid-a", None) == ("cid-from-state", None) -def test_resolve_watch_client_id_falls_back_to_queue_extra_data(monkeypatch): +def test_select_watch_client_id_falls_back_to_queue_extra_data(monkeypatch): """A prompt submitted by something else (browser, older CLI) has no state file — /queue's extra_data still carries the submitting client_id.""" @@ -3443,10 +3446,10 @@ def fake_get(url, **kw): raise AssertionError(f"unexpected url: {url}") monkeypatch.setattr(jobs_mod, "_http_get_json", fake_get) - assert jobs_mod._resolve_watch_client_id("127.0.0.1", 8188, "pid-b") == "cid-from-queue" + assert jobs_mod._select_watch_client_id("127.0.0.1", 8188, "pid-b", None) == ("cid-from-queue", None) -def test_resolve_watch_client_id_falls_back_to_history_then_none(monkeypatch): +def test_select_watch_client_id_falls_back_to_history_then_none(monkeypatch): def fake_get(url, **kw): if url.endswith("/queue"): return {"queue_running": [], "queue_pending": []} @@ -3455,17 +3458,21 @@ def fake_get(url, **kw): return {} monkeypatch.setattr(jobs_mod, "_http_get_json", fake_get) - assert jobs_mod._resolve_watch_client_id("127.0.0.1", 8188, "pid-c") == "cid-from-history" - # Nothing anywhere -> None, so the caller can warn instead of pretending. - assert jobs_mod._resolve_watch_client_id("127.0.0.1", 8188, "pid-missing") is None + assert jobs_mod._select_watch_client_id("127.0.0.1", 8188, "pid-c", None) == ("cid-from-history", None) + # Both endpoints answered and neither holds it: a positively empty server, + # so there is no id to attach as but nothing unsafe about saying so. + assert jobs_mod._select_watch_client_id("127.0.0.1", 8188, "pid-missing", None) == (None, None) -def test_resolve_watch_client_id_survives_an_unreachable_server(monkeypatch): +def test_select_watch_client_id_polls_when_the_server_is_unreachable(monkeypatch): + """An unreachable server leaves borrowed-ness UNKNOWN, not disproved, so the + watch polls rather than attaching to an id it could not vet.""" + def boom(url, **kw): raise RuntimeError("connection refused") monkeypatch.setattr(jobs_mod, "_http_get_json", boom) - assert jobs_mod._resolve_watch_client_id("127.0.0.1", 8188, "pid-x") is None + assert jobs_mod._select_watch_client_id("127.0.0.1", 8188, "pid-x", None) == (None, "indeterminate") def test_history_completed_nodes_unions_cached_executed_and_output_nodes(monkeypatch): @@ -3604,7 +3611,7 @@ def test_watch_terminal_envelope_backfills_completed_nodes_without_events(monkey ] ) monkeypatch.setattr(jobs_mod, "_snapshot", lambda h, p, pid: next(snapshots, None)) - monkeypatch.setattr(jobs_mod, "_resolve_watch_client_id", lambda h, p, pid: None) + monkeypatch.setattr(jobs_mod, "_select_watch_client_id", lambda h, p, pid, req: (None, None)) monkeypatch.setattr(jobs_mod, "_history_completed_nodes", lambda h, p, pid: {"4", "1"}) result, ws, lines = _run_local_watch(monkeypatch, capsys, messages=[]) @@ -3643,7 +3650,7 @@ def test_watch_already_terminal_job_still_lists_completed_nodes(monkeypatch, cap def test_watch_client_id_flag_overrides_resolution(monkeypatch, capsys): monkeypatch.setattr(jobs_mod, "_snapshot", lambda h, p, pid: {"prompt_id": pid, "status": "running", "outputs": []}) - monkeypatch.setattr(jobs_mod, "_resolve_watch_client_id", lambda h, p, pid: "resolved") + monkeypatch.setattr(jobs_mod, "_select_watch_client_id", lambda h, p, pid, req: (req or "resolved", None)) monkeypatch.setattr(jobs_mod, "_history_completed_nodes", lambda h, p, pid: set()) messages = [{"type": "execution_success", "data": {"prompt_id": "pid-w"}}] _result, ws, lines = _run_local_watch( @@ -3659,7 +3666,7 @@ def test_watch_terminal_envelope_validates_against_the_jobs_schema(monkeypatch, import jsonschema monkeypatch.setattr(jobs_mod, "_snapshot", lambda h, p, pid: {"prompt_id": pid, "status": "running", "outputs": []}) - monkeypatch.setattr(jobs_mod, "_resolve_watch_client_id", lambda h, p, pid: "cid") + monkeypatch.setattr(jobs_mod, "_select_watch_client_id", lambda h, p, pid, req: ("cid", None)) monkeypatch.setattr(jobs_mod, "_history_completed_nodes", lambda h, p, pid: {"1"}) messages = [{"type": "execution_success", "data": {"prompt_id": "pid-w"}}] _result, _ws, lines = _run_local_watch(monkeypatch, capsys, messages=messages) @@ -4080,6 +4087,57 @@ def test_http_get_json_oversize_body_is_a_runtime_error_not_a_truncated_parse(mo assert "http://127.0.0.1:8188/history" in str(exc_info.value) +@pytest.mark.parametrize( + "raised", + [ + http.client.RemoteDisconnected("Remote end closed connection without response"), + http.client.IncompleteRead(b"half"), + http.client.BadStatusLine("not a status line"), + ConnectionResetError("reset by peer"), + ], + ids=["remote-disconnected", "incomplete-read", "bad-status-line", "connection-reset"], +) +def test_http_get_json_turns_a_hangup_into_the_one_failure_family(monkeypatch: pytest.MonkeyPatch, raised): + """A server that accepts the connection and then hangs up is an ordinary + transient, not a traceback. + + urllib only wraps what the *request* raises, so these surface raw out of + `getresponse()` — and `URLError` does not cover them. Every call site + catches RuntimeError and nothing else, so anything escaping as its own type + crashes the command; `jobs watch` in particular depends on this to decide a + run's ownership is unverifiable instead of dying mid-decision. + """ + import comfy_cli.http as http_mod + + def boom(req, timeout=None): + raise raised + + monkeypatch.setattr(http_mod._PLAIN_OPENER, "open", boom) + + with pytest.raises(RuntimeError) as exc_info: + jobs_mod._http_get_json("http://127.0.0.1:8188/queue") + assert "http://127.0.0.1:8188/queue" in str(exc_info.value) + + +def test_watch_polls_when_a_hangup_hides_the_borrowed_marker(monkeypatch: pytest.MonkeyPatch): + """End to end: a mid-read hangup leaves ownership unverifiable, and an + explicit --client-id is refused rather than attached.""" + import comfy_cli.http as http_mod + from comfy_cli import jobs_state + + monkeypatch.setattr(jobs_state, "read", lambda _: None) + + def hangup(req, timeout=None): + raise http.client.RemoteDisconnected("Remote end closed connection without response") + + monkeypatch.setattr(http_mod._PLAIN_OPENER, "open", hangup) + + assert jobs_mod._select_watch_client_id("127.0.0.1", 8188, "pid-h", None) == (None, "indeterminate") + with pytest.raises(jobs_mod._ClientIdRejected) as exc_info: + jobs_mod._select_watch_client_id("127.0.0.1", 8188, "pid-h", "someone-elses-live-id") + assert exc_info.value.reason == "indeterminate" + + def test_jobs_ls_survives_an_oversize_queue_response(monkeypatch: pytest.MonkeyPatch): # End to end: `jobs ls` catches RuntimeError from `_http_get_json`, so an # oversize /queue degrades to the on-disk fallback instead of a traceback. From a0552323bd862ed59fa86a7b6af895ae0661824d Mon Sep 17 00:00:00 2001 From: Glary-Bot Date: Tue, 6 Oct 2026 20:59:52 +0000 Subject: [PATCH 12/22] fix(jobs): require the prompt's own record before honouring --client-id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review caught the guard still failing open on a neighbouring flavour of the same fallacy it was built to fix. `_submitted_extra_data` tracked whether each endpoint "answered" and reported a conclusive absence when both did — but a 200 whose body merely lacks `queue_running` / `queue_pending`, or a proxy error page, counted as an answer. The queue walk only cleared the flag for a non-list section, not a missing one, so `/queue` returning `{}` while a borrowed run was LIVE read as "no marker", and an explicit --client-id was attached: the eviction the marker exists to prevent, reachable with no record loss at all. Drop the endpoint bookkeeping rather than patch it. The flag now answers one question — did we get this prompt's own record in our hands? A match in either store with a reachable extra_data slot is the submitted record and counts; everything else does not, including a server that answered and simply never mentioned the prompt. That removes the whole class of "this body didn't say so, therefore it isn't so" reasoning instead of fixing one instance of it, and makes the absolute claim the docs and error registry already made about borrowed runs actually true. A prompt neither store mentions now polls instead of opening a fresh-uuid socket. That socket could never receive anything, and the poll path reaches the same prompt_not_found deadline, so the only change is that the "pass --client-id if you know it" advice is no longer printed where passing one would be refused. Also from review: - `jobs watch --client-id` rejects a blank value and strips a padded one, as `comfy run` does; " " used to be attached as a literal client_id. - Its help text names the borrowed/indeterminate refusals. - The borrowed banner no longer asserts the other client is "still receiving" events, which nothing here establishes. - Document that an already-terminal prompt short-circuits before this runs, so --client-id is ignored rather than refused there (no socket is opened on that path). - test_borrowed_poll_preserves_a_failed_runs_error asserted on a mapping that never held the key, passing vacuously; pin the real relocation. - Cover the ordinary case through the REAL selector — the integration tests monkeypatch it away, so nothing pinned that a normal run still opens a socket. --- CHANGELOG.md | 17 +- comfy_cli/command/jobs.py | 126 ++++++----- comfy_cli/error_codes.py | 23 +- docs/json-output.md | 18 +- tests/comfy_cli/command/test_run_client_id.py | 203 ++++++++++++++---- tests/comfy_cli/jobs/test_jobs.py | 6 +- 6 files changed, 280 insertions(+), 113 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index adb2a7598..4348398e5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,7 +45,22 @@ history. Such a run is marked both in its job state file and in the submitted `extra_data`, so `comfy jobs watch` declines to re-attach to it — from any machine, including one that cannot see the submitter's state file — and - polls status instead. + polls status instead. An override is honoured only once one of those two + sources has positively vouched for the run: with no readable state file and + no readable server record for the prompt, `--client-id` is refused with + `reason: "indeterminate"` rather than risking the eviction, and the watch + polls. A blank `--client-id` is refused (`reason: "empty"`) as it is on + `comfy run`. + +### Fixed + +- `comfy jobs watch` no longer dies with a traceback when the ComfyUI server + accepts a connection and hangs up before answering (a restarting server, a + reset connection). That surfaced as a raw `http.client.RemoteDisconnected`, + which `urllib` does not wrap, past the `RuntimeError` net every caller in + `jobs` catches. +- A failed `comfy jobs watch` carries the node's failure under + `error.details.execution_error` instead of `error.details.details.error`. ### Changed diff --git a/comfy_cli/command/jobs.py b/comfy_cli/command/jobs.py index 7504c917c..ba1c29698 100644 --- a/comfy_cli/command/jobs.py +++ b/comfy_cli/command/jobs.py @@ -1914,62 +1914,57 @@ def __init__(self, reason: _PollReason) -> None: _POLL_ONLY_WATCH_POLL_S = 2.0 -def _get_json_object(url: str) -> tuple[dict[str, Any], bool]: - """GET a JSON **object**, reporting whether the server actually answered with one. +def _get_json_object(url: str) -> dict[str, Any]: + """GET a JSON **object**, or an empty one if anything at all went wrong. - Separates "the server told us" from "we failed to ask", which a bare - ``{}``/``None`` fallback destroys. A 200 carrying valid JSON of the wrong - shape (a proxy error page, a captive portal) is a non-answer too: it cannot - be read as the server reporting an empty queue. + A failed fetch and a 200 carrying valid JSON of the wrong shape (a proxy + error page, a captive portal) collapse to the same empty mapping on + purpose: neither is a record, and the only caller treats "no record" as "we + did not establish anything". """ try: body = _http_get_json(url) except RuntimeError: - return {}, False - return (body, True) if isinstance(body, dict) else ({}, False) + return {} + return body if isinstance(body, dict) else {} def _submitted_extra_data(host: str, port: int, prompt_id: str) -> tuple[Any, bool]: - """This prompt's ``extra_data`` as the SERVER has it, and whether that answer - is CONCLUSIVE. + """This prompt's ``extra_data`` as the SERVER has it, and whether that record + was POSITIVELY READ. Reads ``/queue`` while the prompt is pending or running, then ``/history`` - once it has finished. The second element is the part that matters for - safety: a caller deciding whether a prompt is marked borrowed must not read - a failed fetch as "no marker". Mirrors ``_server_confirms_no_record`` — a - negative needs a positive confirmation, never an absence of evidence. - - Conclusive when either store positively yielded the prompt's ``extra_data`` - slot (one endpoint is enough — that IS the submitted record), or both - answered and neither holds the prompt. Inconclusive when a fetch failed, a - body came back the wrong shape, or the matched entry's ``extra_data`` slot - was missing: a prompt absent from a ``/history`` we could read may still be - live in a ``/queue`` we could not. + once it has finished. The flag is the part that matters for safety: a + caller deciding whether a prompt is marked borrowed must not read the + failure to find the record as proof the marker is not on it. Mirrors + ``_server_confirms_no_record`` — a negative needs a positive confirmation, + never an absence of evidence. + + Only one thing counts as having read the record: matching ``prompt_id`` in + one of the two stores AND being able to reach its ``extra_data`` slot. Every + other outcome is reported as "not read" — the fetch failed, the body was the + wrong shape, a queue section was missing or unwalkable, the prompt was + nowhere to be found, or the matched entry was too short to carry the slot. + Deliberately NOT reasoning about which endpoints answered: that invites + treating a body that merely failed to mention the prompt as a conclusive + absence, which is the very inference this exists to refuse. """ - queue, queue_answered = _get_json_object(f"http://{host}:{port}/queue") - if queue_answered: - for key in ("queue_running", "queue_pending"): - entries = queue.get(key) - if entries is None: - continue - if not isinstance(entries, list): - # Can't walk it, so can't conclude the prompt is absent from it. - queue_answered = False - continue - for entry in entries: - # entry layout: number, prompt_id, prompt, extra_data, outputs_to_execute - if isinstance(entry, list) and len(entry) > 1 and entry[1] == prompt_id: - return (entry[3], True) if len(entry) > 3 else (None, False) - - history, history_answered = _get_json_object(f"http://{host}:{port}/history/{prompt_id}") - if history_answered and prompt_id in history: - body = history[prompt_id] - prompt = body.get("prompt") if isinstance(body, dict) else None - if isinstance(prompt, list) and len(prompt) > 3: - return prompt[3], True - return None, False + queue = _get_json_object(f"http://{host}:{port}/queue") + for key in ("queue_running", "queue_pending"): + entries = queue.get(key) + if not isinstance(entries, list): + continue + for entry in entries: + # entry layout: number, prompt_id, prompt, extra_data, outputs_to_execute + if isinstance(entry, list) and len(entry) > 1 and entry[1] == prompt_id: + return (entry[3], True) if len(entry) > 3 else (None, False) - return None, queue_answered and history_answered + history = _get_json_object(f"http://{host}:{port}/history/{prompt_id}") + body = history.get(prompt_id) + prompt = body.get("prompt") if isinstance(body, dict) else None + if isinstance(prompt, list) and len(prompt) > 3: + return prompt[3], True + return None, False def _select_watch_client_id( @@ -2016,7 +2011,7 @@ def _select_watch_client_id( job = jobs_state.read(prompt_id) except (ValueError, OSError): # unsafe prompt_id / unreadable state dir job = None - extra, server_conclusive = _submitted_extra_data(host, port, prompt_id) + extra, record_read = _submitted_extra_data(host, port, prompt_id) if (job is not None and job.client_id_borrowed) or _borrowed_from_extra_data(extra): if requested: @@ -2024,9 +2019,10 @@ def _select_watch_client_id( return None, "borrowed" # A state file is its own positive evidence: `comfy run` wrote it, and it - # says this run did not borrow. Only when it is absent does the server's - # record have to carry the answer on its own. - if job is None and not server_conclusive: + # says this run did not borrow. Failing that, only the prompt's own server + # record can clear it — a record we could not read says nothing, and a + # server that simply never mentioned the prompt says nothing either. + if job is None and not record_read: if requested: raise _ClientIdRejected("indeterminate") return None, "indeterminate" @@ -2288,7 +2284,9 @@ def watch_cmd( help=( "Local-only: attach as this WS client_id instead of the submitting one. " "ComfyUI sends execution events only to the submitting session, so watch " - "resolves that id automatically; override it if resolution picks wrong." + "resolves that id automatically; override it if resolution picks wrong. " + "Refused on a run that borrowed a live client's socket, and on one whose " + "record cannot be read — attaching could cut that client off." ), ), ] = None, @@ -2301,6 +2299,19 @@ def watch_cmd( h, p = _resolve_host_port(host, port) _server_or_error(h, p) + # Same handling `comfy run` gives the flag: a blank id is a mistake worth + # naming, and a padded one is the id the user meant. Left unstripped, " " + # would be attached as a literal client_id that can never receive anything. + if client_id is not None and not client_id.strip(): + renderer.error( + code="client_id_rejected", + message="--client-id must not be empty", + hint="pass the submitting client's id, or omit --client-id to resolve it automatically", + details={"reason": "empty"}, + ) + raise typer.Exit(code=1) + client_id = client_id.strip() if client_id is not None else None + # If the job already finished, just print status and return — there will # be no more WS events. snap = _snapshot(h, p, prompt_id) @@ -2329,10 +2340,13 @@ def watch_cmd( hint = "drop --client-id; comfy jobs watch will poll this run safely" else: message = ( - "Could not verify that --client-id is safe to attach as: the server did not answer " - "the lookup that says whether this run borrowed a live client's socket" + "Could not confirm that --client-id is safe to attach as: this prompt has no record " + "we could read, so whether it borrowed a live client's socket is unknown" + ) + hint = ( + "drop --client-id and comfy jobs watch will poll this run safely; if the server was " + "briefly unreachable, retry — if the prompt_id is a typo or already pruned, no id works" ) - hint = "retry the command, or drop --client-id; comfy jobs watch will poll this run safely" renderer.error( code="client_id_rejected", message=message, @@ -2370,14 +2384,14 @@ def watch_cmd( if poll_reason == "borrowed": renderer.console().print( "[yellow]![/yellow] [dim]this run was submitted for another client (comfy run " - "--client-id), which is still receiving its live events; attaching as it would cut " + "--client-id) so that client receives its live events; attaching as it would cut " "it off, so this watch polls status instead[/dim]" ) elif poll_reason == "indeterminate": renderer.console().print( - "[yellow]![/yellow] [dim]could not verify whether this run was submitted for another " - "client — the server did not answer that lookup, so no socket was opened and this " - "watch polls status instead; retry for live events[/dim]" + "[yellow]![/yellow] [dim]could not confirm whether this run was submitted for another " + "client — no record of it could be read, so no socket was opened and this watch polls " + "status instead[/dim]" ) elif attached_client_id is None: renderer.console().print( diff --git a/comfy_cli/error_codes.py b/comfy_cli/error_codes.py index 360cc6d5a..ee03fb491 100644 --- a/comfy_cli/error_codes.py +++ b/comfy_cli/error_codes.py @@ -314,18 +314,23 @@ class ErrorCode: "events to that client rather than to the invocation that submitted it. ComfyUI's `/ws` " "handler evicts any socket already registered under an incoming clientId, so nothing may " "attach as a borrowed id without silencing the very client the flag exists to feed. " - "Raised by two commands, and `details.reason` says which case: on `comfy run`, `empty` (the " - "flag was passed with a blank value), `cloud` (local-only — cloud fans execution events out " - "per user/workspace regardless of submitter) and `wait` (`--wait` would watch a socket this " - "run's events are deliberately not sent to); on `comfy jobs watch`, `borrowed` (the prompt is " - "marked as having borrowed a live client's id, so it is poll-only and no override is " - "honoured) and `indeterminate` (the server did not answer the lookup that reads that marker, " - "so the run could not be shown to be safe to attach to — a failed read is not an unmarked " - "record, and is retryable). Refused before anything is submitted or any socket is opened.", + "Raised by two commands, and `details.reason` says which case. Both raise `empty` (the flag " + "was passed with a blank value). `comfy run` adds `cloud` (local-only — cloud fans execution " + "events out per user/workspace regardless of submitter) and `wait` (`--wait` would watch a " + "socket this run's events are deliberately not sent to). `comfy jobs watch` adds `borrowed` " + "(the prompt is marked as having borrowed a live client's id, so it is poll-only and no " + "override is honoured) and `indeterminate` (nothing vouched for the run — no readable job " + "state file, and the prompt's own server record could not be read, whether because the fetch " + "failed, the body was the wrong shape, or neither `/queue` nor `/history` mentions it, so the " + "marker was never ruled out). An override is honoured only once the state file or the " + "prompt's server record has positively cleared it. Refused before anything is submitted or " + "any socket is opened; a prompt that is already terminal short-circuits earlier, where the " + "flag is ignored rather than refused because no socket is opened at all.", "pass a non-empty id, or drop the conflicting flag: `--client-id` cannot be combined with " "`--wait` or a cloud target, and `comfy jobs watch` on a borrowed run takes no `--client-id` " "at all — poll it with `comfy jobs status`; on `indeterminate`, retry once the server answers " - "again, or drop `--client-id` to poll safely now", + "again, or drop `--client-id` to poll safely now — but if the prompt_id is a typo or already " + "pruned it stays unreadable and no id will work", ), ErrorCode( "host_flag_cloud", diff --git a/docs/json-output.md b/docs/json-output.md index ff0e57d5d..d9a9129c7 100644 --- a/docs/json-output.md +++ b/docs/json-output.md @@ -35,11 +35,17 @@ deliberately (`attached: false`) and the watch polls status instead; passing `--client-id` on such a run is refused with `client_id_rejected` (`reason: "borrowed"`) before any socket opens, so there is no way to take that client off its own socket. That marker is read from the state file *and* from the -`extra_data` the server echoes back, and when neither can be reached — a failed -`/queue` + `/history` read with no local state file — the run is not assumed -unmarked: the watch polls (`attached: false`) and an explicit `--client-id` is -refused with `reason: "indeterminate"`, which a retry clears once the server -answers again. Both +`extra_data` the server echoes back, and an override is honoured only once one of +them has actually vouched for the run. If there is no local state file and the +prompt's own server record cannot be read — the fetch failed, the body came back +the wrong shape, or neither `/queue` nor `/history` mentions the prompt — then +nothing has ruled the marker out, so the run is not assumed unmarked: the watch +polls (`attached: false`) and an explicit `--client-id` is refused with +`reason: "indeterminate"`. A retry clears that once the server answers again; +a `prompt_id` that is a typo or already pruned stays unreadable, and no id works +for it. One gap in the guarantee: a prompt that is *already terminal* short-circuits +below before any of this runs, so `--client-id` is ignored rather than refused +there — harmless, because that path opens no socket at all. Both `data.client_id` and `data.attached` are present on *every* terminal envelope: a watch of an already-finished prompt short-circuits without opening a socket, and reports `client_id: null` / `attached: false`. Reconnecting under an existing id @@ -648,7 +654,7 @@ lists what the cloud adds and which of the codes below cannot occur there. | `workflow_unknown_nodes` | Pre-submit validation found unknown class_types / shape mismatches | `errors` (array), `warnings` (array) | 1 | | `partner_node_requires_credential` | Workflow uses a partner-API node and no `api_key_comfy_org` credential is available | `partner_nodes` (array of str, capped at 20 entries × 64 chars each), `partner_node_count` (int, the exact total — read this, not `len(partner_nodes)`), `host`, `port` | 1 | | `spend_consent_required` | Workflow embeds partner-API (paid) nodes and `--allow-spend` was not passed (machine mode) or interactive consent was declined; re-run with `--allow-spend`. Free (non-partner) workflows are unaffected. | `partner_nodes` (array of str, capped at 20 entries × 64 chars each), `partner_node_count` (int, the exact total — read this, not `len(partner_nodes)`); local path also carries `host`, `port`, the cloud path carries `where: "cloud"` | 1 | -| `client_id_rejected` | `--client-id` was combined with something it cannot serve. On `comfy run`: the flag was blank (`empty`), the effective target is cloud (`cloud` — the flag addresses a local ComfyUI socket, and cloud fans execution events out per user/workspace anyway), or `--wait` was passed (`wait` — the run's events go to the named client, so this invocation would watch a silent socket). Also raised by `comfy jobs watch` when the prompt is marked as having borrowed a live client's id (`borrowed`): that run is poll-only and no `--client-id` override is honoured. `comfy jobs watch` raises it with `indeterminate` when the `/queue` + `/history` read that carries that marker failed and no job state file was readable either: absence of the marker is only evidence once somewhere it could have been recorded actually answered, so the override is refused rather than risking the eviction — retry once the server answers. Refused before anything is submitted and before any socket is opened | `reason` (str, `"empty"`, `"cloud"`, `"wait"`, `"borrowed"` or `"indeterminate"`) | 1 | +| `client_id_rejected` | `--client-id` was combined with something it cannot serve. On `comfy run`: the flag was blank (`empty`), the effective target is cloud (`cloud` — the flag addresses a local ComfyUI socket, and cloud fans execution events out per user/workspace anyway), or `--wait` was passed (`wait` — the run's events go to the named client, so this invocation would watch a silent socket). Also raised by `comfy jobs watch` when the prompt is marked as having borrowed a live client's id (`borrowed`): that run is poll-only and no `--client-id` override is honoured. `comfy jobs watch` raises it with `indeterminate` when no job state file was readable and the prompt's own server record could not be read either (the `/queue` + `/history` fetch failed, returned the wrong shape, or simply never mentioned the prompt): nothing has ruled the marker out, so the override is refused rather than risking the eviction — retry once the server answers. `comfy jobs watch` also raises `empty` for a blank `--client-id`, as `comfy run` does. Refused before anything is submitted and before any socket is opened | `reason` (str, `"empty"`, `"cloud"`, `"wait"`, `"borrowed"` or `"indeterminate"`) | 1 | | `prompt_rejected` | Server returned HTTP 400 with `node_errors` | `status` (400), `node_errors` (array — [shape](#node_errors-shape)) | 1 | | `client_error` | Server returned another HTTP 4xx response (including 429; `cloud_rate_limited` is Cloud only) | `status` (int, 4xx), `body` (str) | 1 | | `server_error` | Server returned an HTTP 5xx response | `status` (int, 5xx), `body` (str) | 1 | diff --git a/tests/comfy_cli/command/test_run_client_id.py b/tests/comfy_cli/command/test_run_client_id.py index 21b14af68..49662b5b6 100644 --- a/tests/comfy_cli/command/test_run_client_id.py +++ b/tests/comfy_cli/command/test_run_client_id.py @@ -365,7 +365,11 @@ def test_borrowed_poll_preserves_a_failed_runs_error(self, monkeypatch): env = _envelope(result.output) assert env["error"]["code"] == "execution_error" assert env["error"]["message"] == "node 5 exploded" - assert "error" not in env["error"]["details"] + # The failure detail is lifted out of the snapshot it arrived in and + # published under its own key. Asserting on `details` alone would pass + # vacuously: that mapping is the whole payload, which never held `error`. + assert env["error"]["details"]["execution_error"] == {"exception_message": "node 5 exploded", "node_id": "5"} + assert "error" not in env["error"]["details"]["details"] def test_an_unmarked_server_record_still_resolves(self, monkeypatch): monkeypatch.setattr(jobs_state, "read", lambda _: None) @@ -448,12 +452,14 @@ def fail_if_constructed(*_args, **_kwargs): assert env["data"]["attached"] is False -class TestSubmittedExtraDataReportsConclusiveness: - """`(extra_data, conclusive)` — the second element is the safety-critical one. +class TestSubmittedExtraDataReportsWhetherTheRecordWasRead: + """`(extra_data, record_read)` — the second element is the safety-critical one. - A positive hit in either store IS the submitted record, so one endpoint is - enough. A negative needs BOTH to have answered: a prompt absent from a - `/history` we could read may still be live in a `/queue` we could not. + It answers exactly one question: did we get this prompt's own record in our + hands? A hit in either store is enough, because that IS the submitted + record. Everything else is False, including a server that answered + perfectly well and simply never mentioned the prompt — "not in the body I + read" is not "was never marked". """ @staticmethod @@ -471,48 +477,169 @@ def fake_get(url, **_kw): QUEUED = {"queue_running": [[0, "p", {}, {"client_id": "cid"}, {}]], "queue_pending": []} IN_HISTORY = {"p": {"prompt": [0, "p", {}, {"client_id": "cid"}, {}]}} - def test_a_queue_hit_is_conclusive_even_if_history_is_unreachable(self, monkeypatch): + def test_a_queue_hit_counts_even_if_history_is_unreachable(self, monkeypatch): self._server(monkeypatch, queue=self.QUEUED, history=self.BOOM) assert _submitted_extra_data("127.0.0.1", 8188, "p") == ({"client_id": "cid"}, True) - def test_a_history_hit_is_conclusive_even_if_the_queue_was_unreachable(self, monkeypatch): + def test_a_history_hit_counts_even_if_the_queue_was_unreachable(self, monkeypatch): self._server(monkeypatch, queue=self.BOOM, history=self.IN_HISTORY) assert _submitted_extra_data("127.0.0.1", 8188, "p") == ({"client_id": "cid"}, True) - def test_both_answering_empty_is_a_conclusive_absence(self, monkeypatch): - self._server(monkeypatch, queue=self.EMPTY_QUEUE, history={}) - assert _submitted_extra_data("127.0.0.1", 8188, "p") == (None, True) + @pytest.mark.parametrize( + ("queue", "history", "why"), + [ + (EMPTY_QUEUE, {}, "answered, prompt in neither store"), + (EMPTY_QUEUE, BOOM, "history unreadable"), + (BOOM, {}, "queue unreadable, so the prompt may be running right now"), + (BOOM, BOOM, "neither store readable"), + (["not", "a", "dict"], {}, "a 200 of the wrong shape is not a queue"), + ({"error": "bad gateway"}, {}, "a proxy error page mentions no prompt"), + ({}, {}, "an object with no queue sections places nothing"), + ({"queue_running": "nonsense"}, {}, "an unwalkable section"), + ({"queue_running": [[0, "p"]]}, {}, "matched, but too short to hold extra_data"), + (EMPTY_QUEUE, {"p": {}}, "in history but no prompt tuple"), + ], + ids=[ + "absent-from-both", + "history-unreadable", + "queue-unreadable", + "both-unreadable", + "wrong-shaped-body", + "proxy-error-page", + "no-queue-sections", + "unwalkable-section", + "entry-missing-extra-slot", + "history-entry-malformed", + ], + ) + def test_everything_else_is_not_a_record(self, monkeypatch, queue, history, why): + self._server(monkeypatch, queue=queue, history=history) + assert _submitted_extra_data("127.0.0.1", 8188, "p") == (None, False), why - def test_an_unreadable_history_is_not_an_absence(self, monkeypatch): - self._server(monkeypatch, queue=self.EMPTY_QUEUE, history=self.BOOM) - assert _submitted_extra_data("127.0.0.1", 8188, "p") == (None, False) + def test_a_marked_record_round_trips(self, monkeypatch): + self._server(monkeypatch, queue={"queue_running": [[0, "p", {}, _marked(), {}]]}, history={}) + extra, record_read = _submitted_extra_data("127.0.0.1", 8188, "p") + assert record_read is True + assert extra[jobs_state.BORROWED_CLIENT_ID_KEY] is True - def test_an_unreadable_queue_is_not_an_absence(self, monkeypatch): - """The prompt may be running right now in the queue we could not read.""" - self._server(monkeypatch, queue=self.BOOM, history={}) - assert _submitted_extra_data("127.0.0.1", 8188, "p") == (None, False) - def test_neither_answering_is_not_an_absence(self, monkeypatch): - self._server(monkeypatch, queue=self.BOOM, history=self.BOOM) - assert _submitted_extra_data("127.0.0.1", 8188, "p") == (None, False) +class TestABodyThatMentionsNothingIsNotAnAllClear: + """A server can answer and still tell us nothing about THIS prompt. - def test_a_wrongly_shaped_body_is_not_an_answer(self, monkeypatch): - """A 200 carrying valid JSON of the wrong shape (a proxy error page, a - captive portal) cannot be read as the server reporting an empty queue.""" - self._server(monkeypatch, queue=["not", "a", "dict"], history={}) - assert _submitted_extra_data("127.0.0.1", 8188, "p") == (None, False) + The first fix taught the lookup to distrust a failed fetch. These are the + neighbouring flavour: `/queue` comes back 200 with a body that simply does + not place the prompt -- an object missing the queue sections, a proxy error + page, or a genuinely empty queue once the record has been pruned. Reading + any of those as "no marker, therefore safe" hands the caller's id straight + to the socket while the borrowed run may still be live. + """ - def test_an_unwalkable_queue_section_is_not_an_answer(self, monkeypatch): - self._server(monkeypatch, queue={"queue_running": "nonsense"}, history={}) - assert _submitted_extra_data("127.0.0.1", 8188, "p") == (None, False) + @pytest.fixture(autouse=True) + def no_state_file(self, monkeypatch): + monkeypatch.setattr(jobs_state, "read", lambda _: None) - def test_a_matched_entry_with_no_extra_data_slot_is_not_an_answer(self, monkeypatch): - """Found the prompt but cannot read where the marker would live.""" - self._server(monkeypatch, queue={"queue_running": [[0, "p"]]}, history={}) - assert _submitted_extra_data("127.0.0.1", 8188, "p") == (None, False) + @pytest.mark.parametrize( + "queue_body", + [{}, {"error": "bad gateway"}, {"queue_running": [], "queue_pending": []}], + ids=["no-queue-sections", "proxy-error-page", "empty-queue-record-pruned"], + ) + def test_an_explicit_override_is_refused(self, monkeypatch, queue_body): + monkeypatch.setattr( + jobs, + "_http_get_json", + lambda url, **_kw: queue_body if url.endswith("/queue") else {}, + ) + with pytest.raises(_ClientIdRejected) as excinfo: + _select_watch_client_id("127.0.0.1", 8188, "p", BORROWED) + assert excinfo.value.reason == "indeterminate" - def test_a_marked_record_round_trips(self, monkeypatch): - self._server(monkeypatch, queue={"queue_running": [[0, "p", {}, _marked(), {}]]}, history={}) - extra, conclusive = _submitted_extra_data("127.0.0.1", 8188, "p") - assert conclusive is True - assert extra[jobs_state.BORROWED_CLIENT_ID_KEY] is True + @pytest.mark.parametrize( + "queue_body", + [{}, {"error": "bad gateway"}, {"queue_running": [], "queue_pending": []}], + ids=["no-queue-sections", "proxy-error-page", "empty-queue-record-pruned"], + ) + def test_without_an_override_it_polls(self, monkeypatch, queue_body): + monkeypatch.setattr( + jobs, + "_http_get_json", + lambda url, **_kw: queue_body if url.endswith("/queue") else {}, + ) + assert _select_watch_client_id("127.0.0.1", 8188, "p", None) == (None, "indeterminate") + + +class TestTheOrdinaryCaseStillAttaches: + """The guard must not turn every watch into a poll. + + Drives the REAL selector -- the integration tests elsewhere monkeypatch it + away, so without this nothing pins that an ordinary run still opens a + socket at all. + """ + + @pytest.fixture(autouse=True) + def no_state_file(self, monkeypatch): + monkeypatch.setattr(jobs_state, "read", lambda _: None) + + def test_a_queued_unmarked_run_resolves_and_attaches(self, monkeypatch): + monkeypatch.setattr( + jobs, + "_http_get_json", + lambda url, **_kw: ( + {"queue_running": [[0, "p", {}, {"client_id": "submitter"}, {}]]} if url.endswith("/queue") else {} + ), + ) + assert _select_watch_client_id("127.0.0.1", 8188, "p", None) == ("submitter", None) + + def test_an_override_is_honoured_once_the_record_vouches(self, monkeypatch): + monkeypatch.setattr( + jobs, + "_http_get_json", + lambda url, **_kw: ( + {"queue_running": [[0, "p", {}, {"client_id": "submitter"}, {}]]} if url.endswith("/queue") else {} + ), + ) + assert _select_watch_client_id("127.0.0.1", 8188, "p", "my-own-id") == ("my-own-id", None) + + def test_the_watch_opens_a_socket_for_it(self, monkeypatch): + """End to end through the real selector: a socket is actually opened, + under the resolved submitting id.""" + opened = {} + + class _WS: + def connect(self, url): + opened["url"] = url + + def settimeout(self, _t): + pass + + def recv(self): + raise AssertionError("stop") + + def close(self): + pass + + monkeypatch.setattr(jobs, "_server_or_error", lambda *_a, **_k: True) + monkeypatch.setattr(jobs, "_snapshot", lambda *_: {"prompt_id": "p", "status": "running", "outputs": []}) + monkeypatch.setattr(jobs, "_history_completed_nodes", lambda *_: set()) + monkeypatch.setattr( + jobs, + "_http_get_json", + lambda url, **_kw: ( + {"queue_running": [[0, "p", {}, {"client_id": "submitter"}, {}]]} if url.endswith("/queue") else {} + ), + ) + monkeypatch.setattr(jobs, "WebSocket", lambda *_a, **_k: _WS()) + + CliRunner().invoke(jobs.app, ["watch", "p", "--where", "local", "--timeout", "1"]) + assert "clientId=submitter" in opened.get("url", "") + + +class TestABlankOverrideIsRejectedLikeRun: + @pytest.mark.parametrize("value", ["", " "]) + def test_blank_is_refused_rather_than_used_verbatim(self, monkeypatch, value): + monkeypatch.setattr(jobs, "_server_or_error", lambda *_a, **_k: True) + monkeypatch.setattr(jobs, "_snapshot", lambda *_: {"prompt_id": "p", "status": "running", "outputs": []}) + result = CliRunner().invoke(jobs.app, ["watch", "p", "--where", "local", "--client-id", value]) + assert result.exit_code == 1, result.output + env = _envelope(result.output) + assert env["error"]["code"] == "client_id_rejected" + assert env["error"]["details"]["reason"] == "empty" diff --git a/tests/comfy_cli/jobs/test_jobs.py b/tests/comfy_cli/jobs/test_jobs.py index 5bc604874..ab9037793 100644 --- a/tests/comfy_cli/jobs/test_jobs.py +++ b/tests/comfy_cli/jobs/test_jobs.py @@ -3459,9 +3459,9 @@ def fake_get(url, **kw): monkeypatch.setattr(jobs_mod, "_http_get_json", fake_get) assert jobs_mod._select_watch_client_id("127.0.0.1", 8188, "pid-c", None) == ("cid-from-history", None) - # Both endpoints answered and neither holds it: a positively empty server, - # so there is no id to attach as but nothing unsafe about saying so. - assert jobs_mod._select_watch_client_id("127.0.0.1", 8188, "pid-missing", None) == (None, None) + # A prompt neither store mentions leaves its record unread, so nothing has + # ruled out the borrowed marker and the watch polls rather than attaching. + assert jobs_mod._select_watch_client_id("127.0.0.1", 8188, "pid-missing", None) == (None, "indeterminate") def test_select_watch_client_id_polls_when_the_server_is_unreachable(monkeypatch): From c68f4e42f8401cd0999c8336de5bc3af4828774b Mon Sep 17 00:00:00 2001 From: Glary-Bot Date: Tue, 6 Oct 2026 21:52:38 +0000 Subject: [PATCH 13/22] fix(jobs): validate --client-id before the paths that drop it; true docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 2. The implementation held, but text this branch wrote overstated it in four places, and the blank-id check sat behind two early returns. Ordering: the `empty` refusal ran after the cloud branch and after the server probe, so `jobs watch --where cloud --client-id ""` and the same against a stopped server discarded the flag in silence — while the registry entry claimed BOTH commands raise `empty` and that the refusal precedes any socket. It needs neither host nor server, only the string, so it now runs first, as `comfy run` already does it. Docs, now matching the code rather than the intent: - The guard is PER PROMPT. It reads the marker on the prompt being watched, not on the id passed, so naming a borrowed client's id while watching a different unmarked prompt is still honoured and still evicts it. "There is no way to take that client off its own socket" claimed otherwise, eleven lines above the paragraph conceding the general case. - The terminal short-circuit ignores a NON-EMPTY id; `empty` is still refused there. - schemas/jobs.json described `attached: false` as "the id could not be resolved" and `client_id` as falling back to a fresh id. This branch gave both a second meaning: on a poll-only watch the id was resolved and deliberately withheld, and no socket was opened at all. - `--timeout` does not pace a poll-only watch (fixed 2s); it only sizes that watch's not-found deadline. - The cloud-target enumeration listed three of the five sources `where.resolve` actually uses. - Merge the duplicate `### Fixed` heading I added to Unreleased. Not taken: short-circuiting the server read when the state file already names the submitter, to save a round-trip. It would put the borrowed refusal in two places, and this guard has now failed review twice precisely because the marker check was spread across conditions. One loopback GET is the cheaper mistake. --- CHANGELOG.md | 22 ++++---- comfy_cli/command/jobs.py | 33 +++++++---- comfy_cli/error_codes.py | 7 ++- comfy_cli/schemas/jobs.json | 4 +- docs/json-output.md | 32 ++++++----- tests/comfy_cli/command/test_run_client_id.py | 56 +++++++++++++++++++ 6 files changed, 115 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4348398e5..8119d931d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,8 +39,9 @@ history. events — node highlights, progress, outputs — to that client. This is what lets a tool run a workflow on a user's behalf and still have the user's open canvas light up. Local only, and refused with `client_id_rejected` alongside - `--wait` or an effective `cloud` target (the flag, `COMFY_WHERE`, or a saved - default): ComfyUI evicts whatever socket already holds an + `--wait` or an effective `cloud` target (the flag, `COMFY_WHERE`, a project + `comfy.yaml` default, a saved default, or auto-detection from configured + cloud credentials): ComfyUI evicts whatever socket already holds an incoming clientId, so this invocation must not attach as the borrowed one. Such a run is marked both in its job state file and in the submitted `extra_data`, so `comfy jobs watch` declines to re-attach to it — from any @@ -52,16 +53,6 @@ history. polls. A blank `--client-id` is refused (`reason: "empty"`) as it is on `comfy run`. -### Fixed - -- `comfy jobs watch` no longer dies with a traceback when the ComfyUI server - accepts a connection and hangs up before answering (a restarting server, a - reset connection). That surfaced as a raw `http.client.RemoteDisconnected`, - which `urllib` does not wrap, past the `RuntimeError` net every caller in - `jobs` catches. -- A failed `comfy jobs watch` carries the node's failure under - `error.details.execution_error` instead of `error.details.details.error`. - ### Changed - An `unknown_enum_value` finding (validate, set-widget, edit batches) names the @@ -80,6 +71,13 @@ history. ### Fixed +- `comfy jobs watch` no longer dies with a traceback when the ComfyUI server + accepts a connection and hangs up before answering (a restarting server, a + reset connection). That surfaced as a raw `http.client.RemoteDisconnected`, + which `urllib` does not wrap, past the `RuntimeError` net every caller in + `jobs` catches. +- A failed `comfy jobs watch` carries the node's failure under + `error.details.execution_error` instead of `error.details.details.error`. - `templates fetch` checks the template's model files when an offline catalog is set (`--input` or `COMFY_OBJECT_INFO_FILE`). A file the server lacks is replaced by the one installed file that is the same model in another diff --git a/comfy_cli/command/jobs.py b/comfy_cli/command/jobs.py index ba1c29698..427289784 100644 --- a/comfy_cli/command/jobs.py +++ b/comfy_cli/command/jobs.py @@ -2264,7 +2264,16 @@ def watch_cmd( prompt_id: Annotated[str, typer.Argument(help="The prompt_id returned by `comfy run`.")], host: Annotated[str | None, typer.Option()] = None, port: Annotated[int | None, typer.Option()] = None, - timeout: Annotated[int, typer.Option(help="Per-recv (or per-poll) timeout in seconds.")] = 30, + timeout: Annotated[ + int, + typer.Option( + help=( + "Per-recv (or per-poll) timeout in seconds. A watch that polls instead of " + "attaching (a borrowed run, or one whose record cannot be read) has no recv to " + "time out: it reconciles every 2s, and this only sizes its not-found deadline." + ) + ), + ] = 30, where: Annotated[ str | None, typer.Option("--where", help="'local' (WebSocket) or 'cloud' (HTTP polling)."), @@ -2292,16 +2301,13 @@ def watch_cmd( ] = None, ): renderer = get_renderer() - if _stamp_where(renderer, where) == "cloud": - return _cloud_watch(prompt_id, poll_interval=poll_interval, max_wait=max_wait) - with report_usage_error(renderer, command="jobs watch"): - h, p = _resolve_host_port(host, port) - _server_or_error(h, p) - - # Same handling `comfy run` gives the flag: a blank id is a mistake worth - # naming, and a padded one is the id the user meant. Left unstripped, " " - # would be attached as a literal client_id that can never receive anything. + # Ahead of the cloud branch and the server probe, because this needs + # neither: it reads one string. `comfy run` validates it just as early, and + # anywhere later the flag is silently dropped on the paths that return + # first — a cloud target, or a local server that is down. Left unstripped, + # " " would be attached as a literal client_id that can never receive + # anything. if client_id is not None and not client_id.strip(): renderer.error( code="client_id_rejected", @@ -2312,6 +2318,13 @@ def watch_cmd( raise typer.Exit(code=1) client_id = client_id.strip() if client_id is not None else None + if _stamp_where(renderer, where) == "cloud": + return _cloud_watch(prompt_id, poll_interval=poll_interval, max_wait=max_wait) + + with report_usage_error(renderer, command="jobs watch"): + h, p = _resolve_host_port(host, port) + _server_or_error(h, p) + # If the job already finished, just print status and return — there will # be no more WS events. snap = _snapshot(h, p, prompt_id) diff --git a/comfy_cli/error_codes.py b/comfy_cli/error_codes.py index ee03fb491..17ee04bfa 100644 --- a/comfy_cli/error_codes.py +++ b/comfy_cli/error_codes.py @@ -324,8 +324,11 @@ class ErrorCode: "failed, the body was the wrong shape, or neither `/queue` nor `/history` mentions it, so the " "marker was never ruled out). An override is honoured only once the state file or the " "prompt's server record has positively cleared it. Refused before anything is submitted or " - "any socket is opened; a prompt that is already terminal short-circuits earlier, where the " - "flag is ignored rather than refused because no socket is opened at all.", + "any socket is opened. Two limits, both by design: the guard is per prompt, reading the " + "marker on the prompt being watched rather than on the id passed, so naming a borrowed " + "client's id while watching a different unmarked prompt is still honoured; and a prompt " + "that is already terminal short-circuits earlier, where a NON-EMPTY flag is ignored rather " + "than refused because no socket is opened at all (`empty` is still raised there).", "pass a non-empty id, or drop the conflicting flag: `--client-id` cannot be combined with " "`--wait` or a cloud target, and `comfy jobs watch` on a borrowed run takes no `--client-id` " "at all — poll it with `comfy jobs status`; on `indeterminate`, retry once the server answers " diff --git a/comfy_cli/schemas/jobs.json b/comfy_cli/schemas/jobs.json index 9f13e45b7..518aa507c 100644 --- a/comfy_cli/schemas/jobs.json +++ b/comfy_cli/schemas/jobs.json @@ -54,11 +54,11 @@ "completed_nodes": {"type": "array", "items": {"type": "string"}}, "client_id": { "type": ["string", "null"], - "description": "`jobs watch` local terminal: the WS session id this watch listened as. ComfyUI addresses execution events to the submitting session only, so watch re-attaches with the prompt's own client_id when it can resolve one (job state file, /queue, /history) and falls back to a fresh id otherwise." + "description": "`jobs watch` local terminal: the WS session id this watch listened as, or null when it opened no socket at all. ComfyUI addresses execution events to the submitting session only, so watch re-attaches with the prompt's own client_id when it can resolve one (job state file, /queue, /history) and falls back to a fresh id otherwise. Null on the poll-only paths — a run that borrowed a live client's socket, a run whose record could not be read, and a prompt that was already terminal when the watch started." }, "attached": { "type": "boolean", - "description": "`jobs watch` local terminal: true when `client_id` is the prompt's real submitting session — i.e. live execution events could reach this watch. False means the id could not be resolved and the envelope is snapshot-derived." + "description": "`jobs watch` local terminal: true when `client_id` is the prompt's real submitting session — i.e. live execution events could reach this watch. False means no live stream and a snapshot-derived envelope, for any of three reasons: the id could not be resolved, or it was resolved and deliberately withheld because attaching would cut off the live client the run was submitted for, or the prompt had already finished. `error.details.reason` on a `client_id_rejected` failure distinguishes the withheld cases." }, "count": {"type": "integer"}, "jobs": { diff --git a/docs/json-output.md b/docs/json-output.md index d9a9129c7..4f04ed029 100644 --- a/docs/json-output.md +++ b/docs/json-output.md @@ -33,19 +33,25 @@ specific one — except on a run submitted by `comfy run --client-id`, where the id belongs to a live client being fed on purpose. Resolution withholds it deliberately (`attached: false`) and the watch polls status instead; passing `--client-id` on such a run is refused with `client_id_rejected` -(`reason: "borrowed"`) before any socket opens, so there is no way to take that -client off its own socket. That marker is read from the state file *and* from the -`extra_data` the server echoes back, and an override is honoured only once one of -them has actually vouched for the run. If there is no local state file and the -prompt's own server record cannot be read — the fetch failed, the body came back -the wrong shape, or neither `/queue` nor `/history` mentions the prompt — then -nothing has ruled the marker out, so the run is not assumed unmarked: the watch -polls (`attached: false`) and an explicit `--client-id` is refused with -`reason: "indeterminate"`. A retry clears that once the server answers again; -a `prompt_id` that is a typo or already pruned stays unreadable, and no id works -for it. One gap in the guarantee: a prompt that is *already terminal* short-circuits -below before any of this runs, so `--client-id` is ignored rather than refused -there — harmless, because that path opens no socket at all. Both +(`reason: "borrowed"`) before any socket opens. That marker is read from the job +state file *and* from the `extra_data` the server echoes back, and an override is +honoured only once one of them has actually vouched for the run. If there is no +local state file and the prompt's own server record cannot be read — the fetch +failed, the body came back the wrong shape, or neither `/queue` nor `/history` +mentions the prompt — then nothing has ruled the marker out, so the run is not +assumed unmarked: the watch polls (`attached: false`) and an explicit +`--client-id` is refused with `reason: "indeterminate"`. A retry clears that once +the server answers again; a `prompt_id` that is a typo or already pruned stays +unreadable, and no id works for it. + +Two limits on that guard, both by design. It is **per prompt**: it reads the +marker on the prompt being watched, not on the id you pass, so naming a borrowed +client's id as `--client-id` while watching a *different*, unmarked prompt is +still honoured — and still evicts whatever holds that id, exactly as the last +paragraph of this section describes for any re-attach. And a prompt that is +*already terminal* short-circuits before the guard runs, so a non-empty +`--client-id` is ignored rather than refused there; harmless, because that path +opens no socket at all. Both `data.client_id` and `data.attached` are present on *every* terminal envelope: a watch of an already-finished prompt short-circuits without opening a socket, and reports `client_id: null` / `attached: false`. Reconnecting under an existing id diff --git a/tests/comfy_cli/command/test_run_client_id.py b/tests/comfy_cli/command/test_run_client_id.py index 49662b5b6..f5043a6f8 100644 --- a/tests/comfy_cli/command/test_run_client_id.py +++ b/tests/comfy_cli/command/test_run_client_id.py @@ -643,3 +643,59 @@ def test_blank_is_refused_rather_than_used_verbatim(self, monkeypatch, value): env = _envelope(result.output) assert env["error"]["code"] == "client_id_rejected" assert env["error"]["details"]["reason"] == "empty" + + # Validating it late would let the paths that return first swallow it: a + # cloud watch never forwards the flag, and an unreachable local server + # exits at the probe. `comfy run` rejects it just as early. + def test_a_cloud_target_still_refuses_it(self, monkeypatch): + monkeypatch.setattr( + jobs, + "_cloud_watch", + lambda *_a, **_k: (_ for _ in ()).throw(AssertionError("cloud watch swallowed a blank --client-id")), + ) + result = CliRunner().invoke(jobs.app, ["watch", "p", "--where", "cloud", "--client-id", " "]) + assert result.exit_code == 1, result.output + env = _envelope(result.output) + assert env["error"]["code"] == "client_id_rejected" + assert env["error"]["details"]["reason"] == "empty" + + def test_a_server_that_is_down_still_refuses_it(self, monkeypatch): + monkeypatch.setattr( + jobs, + "_server_or_error", + lambda *_a, **_k: (_ for _ in ()).throw(AssertionError("probed the server before validating the flag")), + ) + result = CliRunner().invoke(jobs.app, ["watch", "p", "--where", "local", "--client-id", ""]) + assert result.exit_code == 1, result.output + assert _envelope(result.output)["error"]["details"]["reason"] == "empty" + + def test_a_padded_id_is_stripped_not_attached_verbatim(self, monkeypatch): + monkeypatch.setattr(jobs_state, "read", lambda _: None) + monkeypatch.setattr( + jobs, + "_http_get_json", + lambda url, **_kw: ( + {"queue_running": [[0, "p", {}, {"client_id": "submitter"}, {}]]} if url.endswith("/queue") else {} + ), + ) + monkeypatch.setattr(jobs, "_server_or_error", lambda *_a, **_k: True) + monkeypatch.setattr(jobs, "_snapshot", lambda *_: {"prompt_id": "p", "status": "running", "outputs": []}) + monkeypatch.setattr(jobs, "_history_completed_nodes", lambda *_: set()) + opened = {} + + class _WS: + def connect(self, url): + opened["url"] = url + + def settimeout(self, _t): + pass + + def recv(self): + raise AssertionError("stop") + + def close(self): + pass + + monkeypatch.setattr(jobs, "WebSocket", lambda *_a, **_k: _WS()) + CliRunner().invoke(jobs.app, ["watch", "p", "--where", "local", "--timeout", "1", "--client-id", " pad "]) + assert "clientId=pad" in opened.get("url", "") From 41ed58b6b3b2a32dcf9739996b1beaadf0e5370d Mon Sep 17 00:00:00 2001 From: Glary-Bot Date: Tue, 6 Oct 2026 22:53:45 +0000 Subject: [PATCH 14/22] fix(jobs): key the borrowed check on the id, not only on the prompt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 3 found the guard asking the wrong question. The marker rides a PROMPT, but what it protects is a SOCKET, and the two come apart: `jobs watch P_ORDINARY --client-id TAB`, where TAB is a live client some OTHER queued run is feeding, passed every check — P_ORDINARY is clean — and attached as TAB, evicting the tab exactly as if the borrowed run had been watched directly. An agent that sees `attached: false` on a quiet run and retries with the tab id it already holds walks straight into it. `_submitted_extra_data` already walks the whole queue, so it now returns a `_SubmittedRecord` carrying the borrowed client_ids it passed on the way, from the same body as the prompt's own record — one read, so the two cannot disagree. An explicit id in that set is refused as `borrowed`. An unrelated override and the no-flag path are untouched. The residual bound is real and now stated rather than implied: the ids come from `/queue`, so a run already out of the queue cannot be consulted. Separately, this only ever protected runs comfy-cli submitted — a browser tab submitting its own prompt stamps no marker — and closing THAT would mean never re-attaching to any prompt, which is the whole feature. Docs, registry and --help now say both. Also from review: the already-terminal exit is a failed `jobs watch` too, but it handed `_snapshot`'s raw failure to `_emit_terminal` under `error` rather than `execution_error`. Trimming and redaction key on the latter, so that envelope carried the full traceback and a `current_inputs` that can hold an api_key — and did not have the shape the CHANGELOG claimed for a failed watch. Promote it as the live path does; it then gets both. The envelope also gains `poll_reason`. Pretty mode explained in prose why no socket was opened; `--json` showed `attached: false` with no way to tell `borrowed` (drop --client-id) from `indeterminate` (retry). --- CHANGELOG.md | 7 +- comfy_cli/command/jobs.py | 69 +++++++-- comfy_cli/error_codes.py | 12 +- comfy_cli/schemas/jobs.json | 7 +- docs/json-output.md | 21 ++- tests/comfy_cli/command/test_run_client_id.py | 140 ++++++++++++++++-- 6 files changed, 216 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8119d931d..7e35a571d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,8 +50,11 @@ history. sources has positively vouched for the run: with no readable state file and no readable server record for the prompt, `--client-id` is refused with `reason: "indeterminate"` rather than risking the eviction, and the watch - polls. A blank `--client-id` is refused (`reason: "empty"`) as it is on - `comfy run`. + polls. The refusal is keyed on the id too, so naming an id another queued run + borrowed is refused even when the prompt being watched is ordinary. A blank + `--client-id` is refused (`reason: "empty"`) as it is on `comfy run`. The + watch envelope carries `poll_reason` (`borrowed`, `indeterminate` or null) so + a `--json` consumer can tell why no socket was opened. ### Changed diff --git a/comfy_cli/command/jobs.py b/comfy_cli/command/jobs.py index 427289784..ce80cf859 100644 --- a/comfy_cli/command/jobs.py +++ b/comfy_cli/command/jobs.py @@ -36,7 +36,7 @@ import uuid from dataclasses import dataclass, field, replace from datetime import datetime, timezone -from typing import TYPE_CHECKING, Annotated, Any, Literal +from typing import TYPE_CHECKING, Annotated, Any, Literal, NamedTuple import typer from websocket import WebSocket, WebSocketException, WebSocketTimeoutException @@ -1929,7 +1929,22 @@ def _get_json_object(url: str) -> dict[str, Any]: return body if isinstance(body, dict) else {} -def _submitted_extra_data(host: str, port: int, prompt_id: str) -> tuple[Any, bool]: +class _SubmittedRecord(NamedTuple): + """Everything one pass over the server's two stores established. + + ``borrowed_ids`` comes from the same ``/queue`` body as ``extra`` because + the two answer different questions and both are needed before a socket + opens: the marker rides a PROMPT, but what it protects is a SOCKET, and a + caller can name a live client's id while watching a different prompt + entirely. Collected in one pass so the two cannot disagree. + """ + + extra: Any + read: bool + borrowed_ids: frozenset[str] + + +def _submitted_extra_data(host: str, port: int, prompt_id: str) -> _SubmittedRecord: """This prompt's ``extra_data`` as the SERVER has it, and whether that record was POSITIVELY READ. @@ -1950,21 +1965,33 @@ def _submitted_extra_data(host: str, port: int, prompt_id: str) -> tuple[Any, bo absence, which is the very inference this exists to refuse. """ queue = _get_json_object(f"http://{host}:{port}/queue") + borrowed: set[str] = set() + hit: tuple[Any, bool] | None = None for key in ("queue_running", "queue_pending"): entries = queue.get(key) if not isinstance(entries, list): continue for entry in entries: # entry layout: number, prompt_id, prompt, extra_data, outputs_to_execute - if isinstance(entry, list) and len(entry) > 1 and entry[1] == prompt_id: - return (entry[3], True) if len(entry) > 3 else (None, False) + if not (isinstance(entry, list) and len(entry) > 1): + continue + extra = entry[3] if len(entry) > 3 else None + if _borrowed_from_extra_data(extra): + fed = _client_id_from_extra_data(extra) + if fed: + borrowed.add(fed) + if hit is None and entry[1] == prompt_id: + hit = (extra, True) if len(entry) > 3 else (None, False) + + if hit is not None: + return _SubmittedRecord(hit[0], hit[1], frozenset(borrowed)) history = _get_json_object(f"http://{host}:{port}/history/{prompt_id}") body = history.get(prompt_id) prompt = body.get("prompt") if isinstance(body, dict) else None if isinstance(prompt, list) and len(prompt) > 3: - return prompt[3], True - return None, False + return _SubmittedRecord(prompt[3], True, frozenset(borrowed)) + return _SubmittedRecord(None, False, frozenset(borrowed)) def _select_watch_client_id( @@ -2011,13 +2038,23 @@ def _select_watch_client_id( job = jobs_state.read(prompt_id) except (ValueError, OSError): # unsafe prompt_id / unreadable state dir job = None - extra, record_read = _submitted_extra_data(host, port, prompt_id) + record = _submitted_extra_data(host, port, prompt_id) + extra, record_read = record.extra, record.read if (job is not None and job.client_id_borrowed) or _borrowed_from_extra_data(extra): if requested: raise _ClientIdRejected("borrowed") return None, "borrowed" + # The watched prompt is clean, but the id the caller named may not be: it + # can belong to a client some OTHER queued run is feeding on purpose. + # Attaching would evict it just the same, so the id is checked against + # every borrowed run the queue is carrying, not only against this prompt. + # Bounded by what the queue can still show — a run already gone from it + # cannot be consulted, which is why the id is only ever a last resort. + if requested and requested in record.borrowed_ids: + raise _ClientIdRejected("borrowed") + # A state file is its own positive evidence: `comfy run` wrote it, and it # says this run did not borrow. Failing that, only the prompt's own server # record can clear it — a record we could not read says nothing, and a @@ -2293,9 +2330,10 @@ def watch_cmd( help=( "Local-only: attach as this WS client_id instead of the submitting one. " "ComfyUI sends execution events only to the submitting session, so watch " - "resolves that id automatically; override it if resolution picks wrong. " - "Refused on a run that borrowed a live client's socket, and on one whose " - "record cannot be read — attaching could cut that client off." + "resolves that id automatically; override it as a last resort if resolution " + "picks wrong. Refused on a run that borrowed a live client's socket, on an id " + "another queued run borrowed, and on a run whose record cannot be read — " + "attaching could cut that client off." ), ), ] = None, @@ -2337,6 +2375,12 @@ def watch_cmd( # ended, so no session was attached and there is no id to report. snap["client_id"] = None snap["attached"] = False + # Same promotion the live path does when it ends. `_emit_terminal` + # keys its trimming and redaction on `execution_error`, so a failure + # left where `_snapshot` puts it reaches the envelope whole — full + # traceback, and a `current_inputs` that can hold an api_key. + if isinstance(snap.get("error"), dict): + snap["execution_error"] = snap.pop("error") if renderer.is_pretty(): renderer.console().print(f"[dim]Prompt {prompt_id} already {snap['status']}; nothing more to watch.[/dim]") _render_status_pretty(snap, host=h, port=p) @@ -2519,6 +2563,11 @@ def watch_cmd( # silence, so it belongs in the envelope rather than only in the logs. "client_id": ws_client_id, "attached": attached_client_id is not None, + # Why no socket was opened, when none was. Pretty mode explains this in + # prose; without it here a --json consumer sees `attached: false` and + # cannot tell the two apart, though they want opposite responses — + # `borrowed` means drop --client-id, `indeterminate` means retry. + "poll_reason": poll_reason, } if state.end_details is not None: details = dict(state.end_details) if isinstance(state.end_details, dict) else {"raw": state.end_details} diff --git a/comfy_cli/error_codes.py b/comfy_cli/error_codes.py index 17ee04bfa..a21887b9f 100644 --- a/comfy_cli/error_codes.py +++ b/comfy_cli/error_codes.py @@ -324,11 +324,13 @@ class ErrorCode: "failed, the body was the wrong shape, or neither `/queue` nor `/history` mentions it, so the " "marker was never ruled out). An override is honoured only once the state file or the " "prompt's server record has positively cleared it. Refused before anything is submitted or " - "any socket is opened. Two limits, both by design: the guard is per prompt, reading the " - "marker on the prompt being watched rather than on the id passed, so naming a borrowed " - "client's id while watching a different unmarked prompt is still honoured; and a prompt " - "that is already terminal short-circuits earlier, where a NON-EMPTY flag is ignored rather " - "than refused because no socket is opened at all (`empty` is still raised there).", + "any socket is opened. `borrowed` covers the id as well as the prompt: naming a borrowed " + "client's id while watching a different, ordinary prompt is refused too, since attaching " + "evicts that client either way. Bounded by what `/queue` still shows — an id fed by a run " + "that has already left the queue cannot be recognised — and limited to runs comfy-cli " + "submitted, since a browser tab submitting its own prompt stamps no marker. A prompt that " + "is already terminal short-circuits earlier, where a NON-EMPTY flag is ignored rather than " + "refused because no socket is opened at all (`empty` is still raised there).", "pass a non-empty id, or drop the conflicting flag: `--client-id` cannot be combined with " "`--wait` or a cloud target, and `comfy jobs watch` on a borrowed run takes no `--client-id` " "at all — poll it with `comfy jobs status`; on `indeterminate`, retry once the server answers " diff --git a/comfy_cli/schemas/jobs.json b/comfy_cli/schemas/jobs.json index 518aa507c..0eef5e98a 100644 --- a/comfy_cli/schemas/jobs.json +++ b/comfy_cli/schemas/jobs.json @@ -58,7 +58,12 @@ }, "attached": { "type": "boolean", - "description": "`jobs watch` local terminal: true when `client_id` is the prompt's real submitting session — i.e. live execution events could reach this watch. False means no live stream and a snapshot-derived envelope, for any of three reasons: the id could not be resolved, or it was resolved and deliberately withheld because attaching would cut off the live client the run was submitted for, or the prompt had already finished. `error.details.reason` on a `client_id_rejected` failure distinguishes the withheld cases." + "description": "`jobs watch` local terminal: true when a socket was opened under an id believed to be the prompt's submitting session, so live execution events could reach this watch. An explicit --client-id is taken at its word and also reports true, without being checked against the real submitter. False means no live stream and a snapshot-derived envelope, for any of three reasons: the id could not be resolved, or it was resolved and deliberately withheld because attaching would cut off the live client the run was submitted for, or the prompt had already finished. See `poll_reason` to tell the withheld cases apart." + }, + "poll_reason": { + "type": ["string", "null"], + "enum": ["borrowed", "indeterminate", null], + "description": "`jobs watch` local terminal: why this watch polled instead of opening a socket, or null when it attached (or when the prompt was already terminal). \"borrowed\" — the run was submitted for another client that is receiving its events, so attaching would cut that client off; drop --client-id and keep polling. \"indeterminate\" — nothing could vouch for the run (no readable job state file, and no readable server record for the prompt), so the marker was never ruled out; retry once the server answers, unless the prompt_id is a typo or already pruned." }, "count": {"type": "integer"}, "jobs": { diff --git a/docs/json-output.md b/docs/json-output.md index 4f04ed029..837d67244 100644 --- a/docs/json-output.md +++ b/docs/json-output.md @@ -44,14 +44,19 @@ assumed unmarked: the watch polls (`attached: false`) and an explicit the server answers again; a `prompt_id` that is a typo or already pruned stays unreadable, and no id works for it. -Two limits on that guard, both by design. It is **per prompt**: it reads the -marker on the prompt being watched, not on the id you pass, so naming a borrowed -client's id as `--client-id` while watching a *different*, unmarked prompt is -still honoured — and still evicts whatever holds that id, exactly as the last -paragraph of this section describes for any re-attach. And a prompt that is -*already terminal* short-circuits before the guard runs, so a non-empty -`--client-id` is ignored rather than refused there; harmless, because that path -opens no socket at all. Both +The check is on the id as well as on the prompt: naming a borrowed client's id +as `--client-id` while watching some *other*, perfectly ordinary prompt is +refused the same way, because attaching evicts that client either way. What +bounds it is what the server can still show — the borrowed ids are read from +`/queue`, so a run that has already left the queue cannot be consulted, and an +id it was feeding is honoured again. Watch by `prompt_id` and let resolution do +its job; `--client-id` is a last resort. One more gap: a prompt that is +*already terminal* short-circuits before any of this runs, so a non-empty +`--client-id` is ignored rather than refused there — harmless, because that path +opens no socket at all. Separately, and by design, this protects only runs +*comfy-cli* submitted with `--client-id`: a browser tab that submits its own +prompt stamps no marker, so watching that prompt re-attaches as the tab, exactly +as the last paragraph of this section describes for any re-attach. Both `data.client_id` and `data.attached` are present on *every* terminal envelope: a watch of an already-finished prompt short-circuits without opening a socket, and reports `client_id: null` / `attached: false`. Reconnecting under an existing id diff --git a/tests/comfy_cli/command/test_run_client_id.py b/tests/comfy_cli/command/test_run_client_id.py index f5043a6f8..0bd9d21ca 100644 --- a/tests/comfy_cli/command/test_run_client_id.py +++ b/tests/comfy_cli/command/test_run_client_id.py @@ -24,7 +24,12 @@ from comfy_cli import jobs_state from comfy_cli.cmdline import run as run_command from comfy_cli.command import jobs -from comfy_cli.command.jobs import _ClientIdRejected, _select_watch_client_id, _submitted_extra_data +from comfy_cli.command.jobs import ( + _ClientIdRejected, + _select_watch_client_id, + _submitted_extra_data, + _SubmittedRecord, +) from comfy_cli.command.run import WorkflowExecution, execute from comfy_cli.output import Renderer, set_renderer from comfy_cli.output.renderer import OutputMode, reset_renderer_for_testing @@ -198,6 +203,11 @@ def _marked(**extra): return {"client_id": BORROWED, jobs_state.BORROWED_CLIENT_ID_KEY: True, **extra} +def _record(extra=None, *, read=True, borrowed_ids=()): + """A `_submitted_extra_data` result, for stubbing it out.""" + return _SubmittedRecord(extra, read, frozenset(borrowed_ids)) + + class TestWatchDoesNotStealTheSocket: @pytest.fixture(autouse=True) def answered_empty_server(self, monkeypatch): @@ -206,7 +216,7 @@ def answered_empty_server(self, monkeypatch): Conclusive, which is a different thing from a read that failed — the class below pins that difference. """ - monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: (None, True)) + monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: _record()) def test_borrowed_record_yields_no_client_id(self, monkeypatch): state = jobs_state.new( @@ -257,12 +267,12 @@ def test_an_answered_empty_server_is_attachable(self, monkeypatch): # see the submitting run's state still has to refuse the id. def test_server_marker_is_honoured_without_a_state_file(self, monkeypatch): monkeypatch.setattr(jobs_state, "read", lambda _: None) - monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: (_marked(), True)) + monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: _record(_marked())) assert _select_watch_client_id("127.0.0.1", 8188, "p", None) == (None, "borrowed") def test_explicit_override_cannot_bypass_borrowed_marker(self, monkeypatch): monkeypatch.setattr(jobs_state, "read", lambda _: None) - monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: (_marked(), True)) + monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: _record(_marked())) with pytest.raises(_ClientIdRejected) as excinfo: _select_watch_client_id("127.0.0.1", 8188, "p", BORROWED) assert excinfo.value.reason == "borrowed" @@ -276,7 +286,7 @@ def server_extra(*_args): reads += 1 if reads > 1: raise AssertionError("selection split marker and client-id reads") - return _marked(), True + return _record(_marked()) monkeypatch.setattr(jobs, "_submitted_extra_data", server_extra) with pytest.raises(_ClientIdRejected): @@ -291,7 +301,7 @@ def test_watch_rejects_explicit_override_before_opening_a_socket(self, monkeypat lambda *_: {"prompt_id": "p", "status": "running", "outputs": []}, ) monkeypatch.setattr(jobs_state, "read", lambda _: None) - monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: (_marked(), True)) + monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: _record(_marked())) def fail_if_constructed(*_args, **_kwargs): raise AssertionError("borrowed watch constructed a WebSocket") @@ -319,7 +329,7 @@ def test_watch_polls_borrowed_run_without_opening_a_socket(self, monkeypatch): monkeypatch.setattr(jobs, "_history_completed_nodes", lambda *_: {"1"}) monkeypatch.setattr(jobs, "_POLL_ONLY_WATCH_POLL_S", 0) monkeypatch.setattr(jobs_state, "read", lambda _: None) - monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: (_marked(), True)) + monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: _record(_marked())) def fail_if_constructed(*_args, **_kwargs): raise AssertionError("borrowed watch constructed a WebSocket") @@ -352,7 +362,7 @@ def test_borrowed_poll_preserves_a_failed_runs_error(self, monkeypatch): monkeypatch.setattr(jobs, "_history_completed_nodes", lambda *_: set()) monkeypatch.setattr(jobs, "_POLL_ONLY_WATCH_POLL_S", 0) monkeypatch.setattr(jobs_state, "read", lambda _: None) - monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: (_marked(), True)) + monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: _record(_marked())) monkeypatch.setattr( jobs, "WebSocket", @@ -373,7 +383,7 @@ def test_borrowed_poll_preserves_a_failed_runs_error(self, monkeypatch): def test_an_unmarked_server_record_still_resolves(self, monkeypatch): monkeypatch.setattr(jobs_state, "read", lambda _: None) - monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: ({"client_id": "cli-minted"}, True)) + monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: _record({"client_id": "cli-minted"})) assert _select_watch_client_id("127.0.0.1", 8188, "p", None) == ("cli-minted", None) @@ -392,7 +402,7 @@ class TestUnverifiableOwnershipFailsSafe: @pytest.fixture(autouse=True) def unverifiable(self, monkeypatch): monkeypatch.setattr(jobs_state, "read", lambda _: None) - monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: (None, False)) + monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: _record(read=False)) def test_explicit_override_is_refused_rather_than_attached(self): with pytest.raises(_ClientIdRejected) as excinfo: @@ -479,11 +489,11 @@ def fake_get(url, **_kw): def test_a_queue_hit_counts_even_if_history_is_unreachable(self, monkeypatch): self._server(monkeypatch, queue=self.QUEUED, history=self.BOOM) - assert _submitted_extra_data("127.0.0.1", 8188, "p") == ({"client_id": "cid"}, True) + assert _submitted_extra_data("127.0.0.1", 8188, "p")[:2] == ({"client_id": "cid"}, True) def test_a_history_hit_counts_even_if_the_queue_was_unreachable(self, monkeypatch): self._server(monkeypatch, queue=self.BOOM, history=self.IN_HISTORY) - assert _submitted_extra_data("127.0.0.1", 8188, "p") == ({"client_id": "cid"}, True) + assert _submitted_extra_data("127.0.0.1", 8188, "p")[:2] == ({"client_id": "cid"}, True) @pytest.mark.parametrize( ("queue", "history", "why"), @@ -514,11 +524,11 @@ def test_a_history_hit_counts_even_if_the_queue_was_unreachable(self, monkeypatc ) def test_everything_else_is_not_a_record(self, monkeypatch, queue, history, why): self._server(monkeypatch, queue=queue, history=history) - assert _submitted_extra_data("127.0.0.1", 8188, "p") == (None, False), why + assert _submitted_extra_data("127.0.0.1", 8188, "p")[:2] == (None, False), why def test_a_marked_record_round_trips(self, monkeypatch): self._server(monkeypatch, queue={"queue_running": [[0, "p", {}, _marked(), {}]]}, history={}) - extra, record_read = _submitted_extra_data("127.0.0.1", 8188, "p") + extra, record_read, _ids = _submitted_extra_data("127.0.0.1", 8188, "p") assert record_read is True assert extra[jobs_state.BORROWED_CLIENT_ID_KEY] is True @@ -567,6 +577,108 @@ def test_without_an_override_it_polls(self, monkeypatch, queue_body): assert _select_watch_client_id("127.0.0.1", 8188, "p", None) == (None, "indeterminate") +class TestTheIdIsCheckedToNotJustThePrompt: + """The marker rides a prompt; what it protects is a socket. + + Watching an ordinary, fully vouched-for prompt while naming a live + client's id as `--client-id` evicts that client just as thoroughly as + attaching to the borrowed run would have. Checking only the watched + prompt's marker misses it entirely, so the id is checked against every + borrowed run the queue is still carrying. + """ + + QUEUE = { + "queue_running": [ + [0, "P_BORROWED", {}, {"client_id": BORROWED, jobs_state.BORROWED_CLIENT_ID_KEY: True}, {}], + [1, "P_OTHER", {}, {"client_id": "ordinary"}, {}], + ], + "queue_pending": [], + } + + @pytest.fixture(autouse=True) + def live_queue(self, monkeypatch): + monkeypatch.setattr(jobs_state, "read", lambda _: None) + monkeypatch.setattr( + jobs, + "_http_get_json", + lambda url, **_kw: self.QUEUE if url.endswith("/queue") else {}, + ) + + def test_another_runs_borrowed_id_is_refused(self): + with pytest.raises(_ClientIdRejected) as excinfo: + _select_watch_client_id("127.0.0.1", 8188, "P_OTHER", BORROWED) + assert excinfo.value.reason == "borrowed" + + def test_the_borrowed_run_itself_is_still_refused(self): + with pytest.raises(_ClientIdRejected) as excinfo: + _select_watch_client_id("127.0.0.1", 8188, "P_BORROWED", BORROWED) + assert excinfo.value.reason == "borrowed" + + def test_an_unrelated_override_is_still_honoured(self): + """The check must not turn --client-id into a no-op.""" + assert _select_watch_client_id("127.0.0.1", 8188, "P_OTHER", "my-own-id") == ("my-own-id", None) + + def test_the_ordinary_watch_is_untouched(self): + assert _select_watch_client_id("127.0.0.1", 8188, "P_OTHER", None) == ("ordinary", None) + + def test_the_borrowed_set_comes_from_the_same_read(self, monkeypatch): + """One `/queue` body answers both questions, so they cannot disagree.""" + reads = [] + monkeypatch.setattr( + jobs, + "_http_get_json", + lambda url, **_kw: (reads.append(url), self.QUEUE if url.endswith("/queue") else {})[1], + ) + with pytest.raises(_ClientIdRejected): + _select_watch_client_id("127.0.0.1", 8188, "P_OTHER", BORROWED) + assert [u for u in reads if u.endswith("/queue")] == ["http://127.0.0.1:8188/queue"] + + +class TestAnAlreadyTerminalFailureIsTrimmedToo: + """The short-circuit exit is a failed `jobs watch` as much as the live one. + + `_emit_terminal` keys its trimming and redaction on `execution_error`, so a + failure left where `_snapshot` puts it reached the envelope whole. + """ + + SNAP = { + "prompt_id": "p", + "status": "error", + "outputs": [], + "error": { + "exception_message": "boom", + "node_id": "5", + "traceback": [f"frame{i}" for i in range(12)], + "current_inputs": {"api_key": "sk-SECRET-123"}, + }, + } + + @pytest.fixture(autouse=True) + def terminal_failure(self, monkeypatch): + monkeypatch.setattr(jobs, "_server_or_error", lambda *_a, **_k: True) + monkeypatch.setattr(jobs, "_snapshot", lambda *_a: dict(self.SNAP)) + monkeypatch.setattr(jobs, "_history_completed_nodes", lambda *_a: set()) + + def _env(self): + result = CliRunner().invoke(jobs.app, ["watch", "p", "--where", "local"]) + assert result.exit_code == 1, result.output + return _envelope(result.output) + + def test_the_failure_is_published_where_the_live_path_publishes_it(self): + env = self._env() + assert env["error"]["code"] == "execution_error" + assert env["error"]["details"]["execution_error"]["exception_message"] == "boom" + assert "error" not in env["error"]["details"] + + def test_a_secret_in_current_inputs_does_not_reach_the_envelope(self): + assert "sk-SECRET-123" not in json.dumps(self._env()) + + def test_the_traceback_is_capped_to_its_tail(self): + env = self._env() + assert env["error"]["details"]["execution_error"]["traceback"] == ["frame10", "frame11"] + assert "frame0" not in json.dumps(env) + + class TestTheOrdinaryCaseStillAttaches: """The guard must not turn every watch into a poll. From 9cb5d8b65e8def9205cab1412f5ca306b1a79f61 Mon Sep 17 00:00:00 2001 From: Glary-Bot Date: Tue, 6 Oct 2026 23:37:30 +0000 Subject: [PATCH 15/22] fix(jobs): a census nobody ran is not a census of nobody MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 3 added the queue-wide borrowed-id check; round 4 found I had given it no way to say whether it ran. `borrowed_ids` is empty both when no run has borrowed anything and when `/queue` never came back, and the `indeterminate` net was gated on `job is None` — so a watched prompt with a readable state file skipped it entirely and an explicit `--client-id` sailed through to `ws.connect`. A blipped lookup silently evicted a live tab on a run squarely inside the protected set, which is the same absence-of-evidence reasoning this module keeps being fixed for. `_SubmittedRecord` now carries `queue_read` beside `read`, because they are different facts: one covers this prompt's own record, the other the queue-wide sweep. An explicit id is refused as `indeterminate` when the sweep did not run, independently of the state file — a local record vouches for THIS run and says nothing about whose socket the caller just named. `queue_read` needs BOTH sections walkable: a borrowed run in the one we could not read would be missing from the census without the census knowing. The unrequested path is untouched, so an ordinary run still resolves its recorded id and attaches. Also: - `read=True` required only that the `extra_data` slot be present, not that it be a dict, while the marker test needs a dict — so a non-dict slot counted as a record positively read and unmarked. Stock ComfyUI cannot produce one, but a safety primitive should not depend on that. - The terminal short-circuit published `client_id` and `attached` but not `poll_reason`, which the schema promised was null there; a consumer following the schema got a KeyError on the path it was pointed down. - `poll_reason` had no test at all, which is how that shipped. Both poll paths now assert it and validate against the published schema, so the enum is exercised with a non-null value. - The `answered_empty_server` fixture described `read=True, extra=None` as "the server answered and holds no record", which production reports as `read=False`. It documented a contract that does not exist. - Five text sites bounded the id check by "a run that has already left the queue" and omitted "the queue read failed". Both now stated. --- CHANGELOG.md | 4 +- comfy_cli/command/jobs.py | 45 ++++++--- comfy_cli/error_codes.py | 9 +- docs/json-output.md | 12 ++- tests/comfy_cli/command/test_run_client_id.py | 99 ++++++++++++++++--- 5 files changed, 136 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7e35a571d..a7f81f41b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,7 +51,9 @@ history. no readable server record for the prompt, `--client-id` is refused with `reason: "indeterminate"` rather than risking the eviction, and the watch polls. The refusal is keyed on the id too, so naming an id another queued run - borrowed is refused even when the prompt being watched is ordinary. A blank + borrowed is refused even when the prompt being watched is ordinary — and + refused as `indeterminate` when the `/queue` read that would show those ids + did not come back whole, rather than read as nobody borrowing. A blank `--client-id` is refused (`reason: "empty"`) as it is on `comfy run`. The watch envelope carries `poll_reason` (`borrowed`, `indeterminate` or null) so a `--json` consumer can tell why no socket was opened. diff --git a/comfy_cli/command/jobs.py b/comfy_cli/command/jobs.py index ce80cf859..0e564deca 100644 --- a/comfy_cli/command/jobs.py +++ b/comfy_cli/command/jobs.py @@ -1937,11 +1937,19 @@ class _SubmittedRecord(NamedTuple): opens: the marker rides a PROMPT, but what it protects is a SOCKET, and a caller can name a live client's id while watching a different prompt entirely. Collected in one pass so the two cannot disagree. + + Each has its own "we actually read it" flag, and they are NOT the same + fact. ``read`` covers this prompt's own record; ``queue_read`` covers the + queue-wide census behind ``borrowed_ids``, which is empty both when no run + has borrowed anything and when the fetch failed. Collapsing them would let + a blipped ``/queue`` pass for "nobody is borrowing" — the same absence-of- + evidence reasoning the rest of this module exists to refuse. """ extra: Any read: bool borrowed_ids: frozenset[str] + queue_read: bool def _submitted_extra_data(host: str, port: int, prompt_id: str) -> _SubmittedRecord: @@ -1965,10 +1973,14 @@ def _submitted_extra_data(host: str, port: int, prompt_id: str) -> _SubmittedRec absence, which is the very inference this exists to refuse. """ queue = _get_json_object(f"http://{host}:{port}/queue") + sections = [queue.get(key) for key in ("queue_running", "queue_pending")] + # Both, not either: a borrowed run sitting in the section we could not walk + # would be missing from the census without the census knowing it. + queue_read = all(isinstance(entries, list) for entries in sections) + borrowed: set[str] = set() hit: tuple[Any, bool] | None = None - for key in ("queue_running", "queue_pending"): - entries = queue.get(key) + for entries in sections: if not isinstance(entries, list): continue for entry in entries: @@ -1981,17 +1993,17 @@ def _submitted_extra_data(host: str, port: int, prompt_id: str) -> _SubmittedRec if fed: borrowed.add(fed) if hit is None and entry[1] == prompt_id: - hit = (extra, True) if len(entry) > 3 else (None, False) + hit = (extra, True) if isinstance(extra, dict) else (None, False) if hit is not None: - return _SubmittedRecord(hit[0], hit[1], frozenset(borrowed)) + return _SubmittedRecord(hit[0], hit[1], frozenset(borrowed), queue_read) history = _get_json_object(f"http://{host}:{port}/history/{prompt_id}") body = history.get(prompt_id) prompt = body.get("prompt") if isinstance(body, dict) else None - if isinstance(prompt, list) and len(prompt) > 3: - return _SubmittedRecord(prompt[3], True, frozenset(borrowed)) - return _SubmittedRecord(None, False, frozenset(borrowed)) + if isinstance(prompt, list) and len(prompt) > 3 and isinstance(prompt[3], dict): + return _SubmittedRecord(prompt[3], True, frozenset(borrowed), queue_read) + return _SubmittedRecord(None, False, frozenset(borrowed), queue_read) def _select_watch_client_id( @@ -2050,11 +2062,18 @@ def _select_watch_client_id( # can belong to a client some OTHER queued run is feeding on purpose. # Attaching would evict it just the same, so the id is checked against # every borrowed run the queue is carrying, not only against this prompt. - # Bounded by what the queue can still show — a run already gone from it - # cannot be consulted, which is why the id is only ever a last resort. if requested and requested in record.borrowed_ids: raise _ClientIdRejected("borrowed") + # ...and that check is worth exactly as much as the read behind it. An + # empty census means "nobody is borrowing" only once the queue was actually + # walked; otherwise it means "we have no idea", which must not read as + # consent. Deliberately not folded into the state-file test below: a local + # record vouches for THIS run, and says nothing about whose socket the + # caller just named. + if requested and not record.queue_read: + raise _ClientIdRejected("indeterminate") + # A state file is its own positive evidence: `comfy run` wrote it, and it # says this run did not borrow. Failing that, only the prompt's own server # record can clear it — a record we could not read says nothing, and a @@ -2332,8 +2351,8 @@ def watch_cmd( "ComfyUI sends execution events only to the submitting session, so watch " "resolves that id automatically; override it as a last resort if resolution " "picks wrong. Refused on a run that borrowed a live client's socket, on an id " - "another queued run borrowed, and on a run whose record cannot be read — " - "attaching could cut that client off." + "another queued run borrowed, and whenever the records that would show either " + "cannot be read — attaching could cut that client off." ), ), ] = None, @@ -2375,6 +2394,10 @@ def watch_cmd( # ended, so no session was attached and there is no id to report. snap["client_id"] = None snap["attached"] = False + # No socket, but not withheld either — the prompt simply ended first. + # The key is still published so a consumer that branches on it never + # hits a missing one, as with `client_id` and `attached`. + snap["poll_reason"] = None # Same promotion the live path does when it ends. `_emit_terminal` # keys its trimming and redaction on `execution_error`, so a failure # left where `_snapshot` puts it reaches the envelope whole — full diff --git a/comfy_cli/error_codes.py b/comfy_cli/error_codes.py index a21887b9f..6f5720d0f 100644 --- a/comfy_cli/error_codes.py +++ b/comfy_cli/error_codes.py @@ -326,9 +326,12 @@ class ErrorCode: "prompt's server record has positively cleared it. Refused before anything is submitted or " "any socket is opened. `borrowed` covers the id as well as the prompt: naming a borrowed " "client's id while watching a different, ordinary prompt is refused too, since attaching " - "evicts that client either way. Bounded by what `/queue` still shows — an id fed by a run " - "that has already left the queue cannot be recognised — and limited to runs comfy-cli " - "submitted, since a browser tab submitting its own prompt stamps no marker. A prompt that " + "evicts that client either way. Those ids come from `/queue`, and an override is refused " + "as `indeterminate` when that read did not come back whole, since an empty list of them " + "means 'nobody is borrowing' only once the queue was walked. Bounded by what a READ " + "`/queue` still shows — an id fed by a run that has already left the queue cannot be " + "recognised — and limited to runs comfy-cli submitted, since a browser tab submitting its " + "own prompt stamps no marker. A prompt that " "is already terminal short-circuits earlier, where a NON-EMPTY flag is ignored rather than " "refused because no socket is opened at all (`empty` is still raised there).", "pass a non-empty id, or drop the conflicting flag: `--client-id` cannot be combined with " diff --git a/docs/json-output.md b/docs/json-output.md index 837d67244..c646fd4e2 100644 --- a/docs/json-output.md +++ b/docs/json-output.md @@ -46,11 +46,13 @@ unreadable, and no id works for it. The check is on the id as well as on the prompt: naming a borrowed client's id as `--client-id` while watching some *other*, perfectly ordinary prompt is -refused the same way, because attaching evicts that client either way. What -bounds it is what the server can still show — the borrowed ids are read from -`/queue`, so a run that has already left the queue cannot be consulted, and an -id it was feeding is honoured again. Watch by `prompt_id` and let resolution do -its job; `--client-id` is a last resort. One more gap: a prompt that is +refused the same way, because attaching evicts that client either way. Those +ids are read from `/queue`, and if that read does not come back whole the +override is refused as `indeterminate` rather than waved through — an empty +list of borrowed ids counts as "nobody is borrowing" only when the queue was +actually walked. What it cannot see is a run that has already *left* the queue: +an id that run was feeding is honoured again. Watch by `prompt_id` and let +resolution do its job; `--client-id` is a last resort. One more gap: a prompt that is *already terminal* short-circuits before any of this runs, so a non-empty `--client-id` is ignored rather than refused there — harmless, because that path opens no socket at all. Separately, and by design, this protects only runs diff --git a/tests/comfy_cli/command/test_run_client_id.py b/tests/comfy_cli/command/test_run_client_id.py index 0bd9d21ca..96ae66e68 100644 --- a/tests/comfy_cli/command/test_run_client_id.py +++ b/tests/comfy_cli/command/test_run_client_id.py @@ -13,6 +13,7 @@ import json import os +import pathlib import tempfile import uuid from unittest.mock import MagicMock, patch @@ -203,20 +204,33 @@ def _marked(**extra): return {"client_id": BORROWED, jobs_state.BORROWED_CLIENT_ID_KEY: True, **extra} -def _record(extra=None, *, read=True, borrowed_ids=()): +def _validate_watch_envelope(data): + """Check a watch envelope against the published schema. + + The `poll_reason` enum is only worth publishing if something exercises it + with a non-null value. + """ + import jsonschema + + schema_path = pathlib.Path(jobs.__file__).parent.parent / "schemas" / "jobs.json" + jsonschema.Draft202012Validator(json.loads(schema_path.read_text())).validate(data) + + +def _record(extra=None, *, read=True, borrowed_ids=(), queue_read=True): """A `_submitted_extra_data` result, for stubbing it out.""" - return _SubmittedRecord(extra, read, frozenset(borrowed_ids)) + return _SubmittedRecord(extra, read, frozenset(borrowed_ids), queue_read) class TestWatchDoesNotStealTheSocket: @pytest.fixture(autouse=True) - def answered_empty_server(self, monkeypatch): - """Default: the server ANSWERED and holds no record of this prompt. + def record_read_but_empty(self, monkeypatch): + """Default: this prompt's record WAS read, and carries no marker. - Conclusive, which is a different thing from a read that failed — the - class below pins that difference. + Note `read=True` with `extra=None` is the shape for a record that was + found and is unmarked — NOT for a server that holds no record at all, + which reports `read=False` and is pinned in the classes below. """ - monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: _record()) + monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: _record({})) def test_borrowed_record_yields_no_client_id(self, monkeypatch): state = jobs_state.new( @@ -259,7 +273,7 @@ def test_borrowed_is_distinguishable_from_unresolvable(self, monkeypatch, borrow _cid, reason = _select_watch_client_id("127.0.0.1", 8188, "p", None) assert (reason == "borrowed") is borrowed - def test_an_answered_empty_server_is_attachable(self, monkeypatch): + def test_a_read_unmarked_record_is_attachable(self, monkeypatch): monkeypatch.setattr(jobs_state, "read", lambda _: None) assert _select_watch_client_id("127.0.0.1", 8188, "p", None) == (None, None) @@ -344,6 +358,10 @@ def fail_if_constructed(*_args, **_kwargs): assert env["data"]["completed_nodes"] == ["1"] assert env["data"]["client_id"] is None assert env["data"]["attached"] is False + # The reason rides the envelope too: `attached: false` alone cannot + # tell a --json consumer to drop --client-id rather than retry. + assert env["data"]["poll_reason"] == "borrowed" + _validate_watch_envelope(env["data"]) def test_borrowed_poll_preserves_a_failed_runs_error(self, monkeypatch): snapshots = iter( @@ -460,6 +478,8 @@ def fail_if_constructed(*_args, **_kwargs): assert env["data"]["status"] == "completed" assert env["data"]["client_id"] is None assert env["data"]["attached"] is False + assert env["data"]["poll_reason"] == "indeterminate" + _validate_watch_envelope(env["data"]) class TestSubmittedExtraDataReportsWhetherTheRecordWasRead: @@ -507,6 +527,10 @@ def test_a_history_hit_counts_even_if_the_queue_was_unreachable(self, monkeypatc ({}, {}, "an object with no queue sections places nothing"), ({"queue_running": "nonsense"}, {}, "an unwalkable section"), ({"queue_running": [[0, "p"]]}, {}, "matched, but too short to hold extra_data"), + ({"queue_running": [[0, "p", {}, None, {}]]}, {}, "extra_data slot present but null"), + ({"queue_running": [[0, "p", {}, [], {}]]}, {}, "extra_data slot is a list"), + ({"queue_running": [[0, "p", {}, "x", {}]]}, {}, "extra_data slot is a string"), + (EMPTY_QUEUE, {"p": {"prompt": [0, "p", {}, 7, {}]}}, "history slot is a number"), (EMPTY_QUEUE, {"p": {}}, "in history but no prompt tuple"), ], ids=[ @@ -520,6 +544,10 @@ def test_a_history_hit_counts_even_if_the_queue_was_unreachable(self, monkeypatc "unwalkable-section", "entry-missing-extra-slot", "history-entry-malformed", + "slot-null", + "slot-list", + "slot-string", + "history-slot-number", ], ) def test_everything_else_is_not_a_record(self, monkeypatch, queue, history, why): @@ -528,7 +556,7 @@ def test_everything_else_is_not_a_record(self, monkeypatch, queue, history, why) def test_a_marked_record_round_trips(self, monkeypatch): self._server(monkeypatch, queue={"queue_running": [[0, "p", {}, _marked(), {}]]}, history={}) - extra, record_read, _ids = _submitted_extra_data("127.0.0.1", 8188, "p") + extra, record_read, _ids, _q = _submitted_extra_data("127.0.0.1", 8188, "p") assert record_read is True assert extra[jobs_state.BORROWED_CLIENT_ID_KEY] is True @@ -621,6 +649,43 @@ def test_an_unrelated_override_is_still_honoured(self): def test_the_ordinary_watch_is_untouched(self): assert _select_watch_client_id("127.0.0.1", 8188, "P_OTHER", None) == ("ordinary", None) + def test_an_unread_queue_refuses_the_override_rather_than_waving_it_through(self, monkeypatch): + """An empty census means "nobody is borrowing" only once the queue was + walked. A readable state file vouches for THIS run and says nothing + about whose socket the caller just named, so it must not unlock it.""" + state = jobs_state.new(prompt_id="P_OTHER", client_id="ordinary", workflow="w.json", where="local") + monkeypatch.setattr(jobs_state, "read", lambda _: state) + + def boom(url, **_kw): + if url.endswith("/queue"): + raise RuntimeError("connection refused") + return {} + + monkeypatch.setattr(jobs, "_http_get_json", boom) + with pytest.raises(_ClientIdRejected) as excinfo: + _select_watch_client_id("127.0.0.1", 8188, "P_OTHER", BORROWED) + assert excinfo.value.reason == "indeterminate" + # The no-flag path names no foreign id, so it still attaches. + assert _select_watch_client_id("127.0.0.1", 8188, "P_OTHER", None) == ("ordinary", None) + + @pytest.mark.parametrize( + "queue_body", + [{"queue_running": []}, {"queue_pending": []}, {}, {"queue_running": [], "queue_pending": "x"}], + ids=["only-running", "only-pending", "neither", "pending-unwalkable"], + ) + def test_a_half_walkable_queue_is_not_a_census(self, monkeypatch, queue_body): + """A borrowed run in the section we could not walk would be missing + from the census without the census knowing it.""" + monkeypatch.setattr(jobs_state, "read", lambda _: None) + monkeypatch.setattr( + jobs, + "_http_get_json", + lambda url, **_kw: queue_body if url.endswith("/queue") else {}, + ) + with pytest.raises(_ClientIdRejected) as excinfo: + _select_watch_client_id("127.0.0.1", 8188, "P_OTHER", BORROWED) + assert excinfo.value.reason == "indeterminate" + def test_the_borrowed_set_comes_from_the_same_read(self, monkeypatch): """One `/queue` body answers both questions, so they cannot disagree.""" reads = [] @@ -696,7 +761,9 @@ def test_a_queued_unmarked_run_resolves_and_attaches(self, monkeypatch): jobs, "_http_get_json", lambda url, **_kw: ( - {"queue_running": [[0, "p", {}, {"client_id": "submitter"}, {}]]} if url.endswith("/queue") else {} + {"queue_running": [[0, "p", {}, {"client_id": "submitter"}, {}]], "queue_pending": []} + if url.endswith("/queue") + else {} ), ) assert _select_watch_client_id("127.0.0.1", 8188, "p", None) == ("submitter", None) @@ -706,7 +773,9 @@ def test_an_override_is_honoured_once_the_record_vouches(self, monkeypatch): jobs, "_http_get_json", lambda url, **_kw: ( - {"queue_running": [[0, "p", {}, {"client_id": "submitter"}, {}]]} if url.endswith("/queue") else {} + {"queue_running": [[0, "p", {}, {"client_id": "submitter"}, {}]], "queue_pending": []} + if url.endswith("/queue") + else {} ), ) assert _select_watch_client_id("127.0.0.1", 8188, "p", "my-own-id") == ("my-own-id", None) @@ -736,7 +805,9 @@ def close(self): jobs, "_http_get_json", lambda url, **_kw: ( - {"queue_running": [[0, "p", {}, {"client_id": "submitter"}, {}]]} if url.endswith("/queue") else {} + {"queue_running": [[0, "p", {}, {"client_id": "submitter"}, {}]], "queue_pending": []} + if url.endswith("/queue") + else {} ), ) monkeypatch.setattr(jobs, "WebSocket", lambda *_a, **_k: _WS()) @@ -787,7 +858,9 @@ def test_a_padded_id_is_stripped_not_attached_verbatim(self, monkeypatch): jobs, "_http_get_json", lambda url, **_kw: ( - {"queue_running": [[0, "p", {}, {"client_id": "submitter"}, {}]]} if url.endswith("/queue") else {} + {"queue_running": [[0, "p", {}, {"client_id": "submitter"}, {}]], "queue_pending": []} + if url.endswith("/queue") + else {} ), ) monkeypatch.setattr(jobs, "_server_or_error", lambda *_a, **_k: True) From b5dd436a97c60bba3ccb27b5811c0d0741897710 Mon Sep 17 00:00:00 2001 From: Glary-Bot Date: Wed, 7 Oct 2026 00:32:34 +0000 Subject: [PATCH 16/22] fix(jobs): say which refusal this is, and what dropping the flag does Round 5 (PASS, non-blocking polish). Four situations reach the two published `reason` values, and `watch_cmd` rendered them from a two-way branch, so half the refusals described the other half's situation and gave advice that did the opposite of what the command does. Dropping `--client-id` makes a run carrying its OWN marker poll; for the other three it makes the watch ATTACH under its own resolved id. A prompt with a perfectly readable state file was also told it "has no record we could read" when it was the /queue census that failed. `_ClientIdRejected` now carries a four-way cause and the message/hint come from a table keyed on it. The published `reason` stays the documented pair, so the machine contract is unchanged. Rest of the round, all text or test quality: - docs claimed client_id/attached on "every" terminal envelope; cloud watch emits neither. Restore the `local` qualifier that was there. - schema still described `execution_error` as cloud-only; this branch made local watch publish it too, from /history. - changelog named only one of the two envelope moves, and did not say `error.details.error` is gone from the already-terminal path. - the pretty panel renders its error row from the key that path promotes, so render before promoting, not after. - `test_a_half_walkable_queue_is_not_a_census` left the record-side gate able to raise the same `indeterminate`, so it passed with the census guard deleted. Give it the readable state file that isolates it. - `_record()`'s default modelled `read=True, extra=None`, a pair `_submitted_extra_data` never returns; derive it as production does. - the parametrised ids drifted from their data at case 10, naming the wrong scenario on failure. --- CHANGELOG.md | 7 +- comfy_cli/command/jobs.py | 78 +++++++++---- comfy_cli/schemas/jobs.json | 2 +- docs/json-output.md | 3 +- tests/comfy_cli/command/test_run_client_id.py | 110 ++++++++++++++++-- 5 files changed, 165 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a7f81f41b..480cfa7c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -82,7 +82,12 @@ history. which `urllib` does not wrap, past the `RuntimeError` net every caller in `jobs` catches. - A failed `comfy jobs watch` carries the node's failure under - `error.details.execution_error` instead of `error.details.details.error`. + `error.details.execution_error` on both of its exits — instead of + `error.details.details.error` on the live path, and instead of + `error.details.error` on the one that short-circuits because the prompt had + already finished. Neither old key is published any more. The second exit was + also skipping the trimming and redaction that key drives, so it emitted the + full traceback and a `current_inputs` that can hold an api_key. - `templates fetch` checks the template's model files when an offline catalog is set (`--input` or `COMFY_OBJECT_INFO_FILE`). A file the server lacks is replaced by the one installed file that is the same model in another diff --git a/comfy_cli/command/jobs.py b/comfy_cli/command/jobs.py index 0e564deca..361386bd7 100644 --- a/comfy_cli/command/jobs.py +++ b/comfy_cli/command/jobs.py @@ -1893,17 +1893,59 @@ def _borrowed_from_extra_data(extra: Any) -> bool: _PollReason = Literal["borrowed", "indeterminate"] +# Four distinct situations collapse into the two published `reason` values, and +# the advice differs even where the reason does not: dropping `--client-id` +# makes a run whose own marker is set POLL, but makes the other three ATTACH +# under their own resolved id. A single message per reason told half of them the +# opposite of what happens. `reason` stays the documented pair; the cause only +# picks the wording. +_RejectCause = Literal["prompt_borrowed", "id_borrowed", "census_unread", "nothing_vouched"] + +_REJECTION_TEXT: dict[_RejectCause, tuple[_PollReason, str, str]] = { + "prompt_borrowed": ( + "borrowed", + "This run borrowed another client's live socket and can only be watched by polling", + "drop --client-id; comfy jobs watch will poll this run safely", + ), + "id_borrowed": ( + "borrowed", + "--client-id names a client that another queued run is feeding on purpose; attaching as it " + "would cut that client off", + "drop --client-id — this prompt resolves its own submitting client and attaches normally", + ), + "census_unread": ( + "indeterminate", + "Could not confirm --client-id is free to attach as: the /queue read that lists the clients " + "other runs are feeding did not come back whole", + "retry once the server answers, or drop --client-id to attach as this prompt's own submitting client", + ), + "nothing_vouched": ( + "indeterminate", + "Could not confirm that --client-id is safe to attach as: this prompt has no record we " + "could read, so whether it borrowed a live client's socket is unknown", + "drop --client-id and comfy jobs watch will poll this run safely; if the server was " + "briefly unreachable, retry — if the prompt_id is a typo or already pruned, no id works", + ), +} + + class _ClientIdRejected(ValueError): """An explicit ``--client-id`` the watch must not honour, and why. - Subclasses ValueError so the reason rides the exception rather than being + Subclasses ValueError so the cause rides the exception rather than being re-derived at the call site, where the second lookup it would need could disagree with the one that made the decision. """ - def __init__(self, reason: _PollReason) -> None: - super().__init__(reason) - self.reason: _PollReason = reason + def __init__(self, cause: _RejectCause) -> None: + super().__init__(cause) + self.cause: _RejectCause = cause + self.reason: _PollReason = _REJECTION_TEXT[cause][0] + + @property + def text(self) -> tuple[str, str]: + """The message and hint this cause should be reported with.""" + return _REJECTION_TEXT[self.cause][1:] # How long a poll-only watch waits before reconciling. The ordinary `--timeout` @@ -2055,7 +2097,7 @@ def _select_watch_client_id( if (job is not None and job.client_id_borrowed) or _borrowed_from_extra_data(extra): if requested: - raise _ClientIdRejected("borrowed") + raise _ClientIdRejected("prompt_borrowed") return None, "borrowed" # The watched prompt is clean, but the id the caller named may not be: it @@ -2063,7 +2105,7 @@ def _select_watch_client_id( # Attaching would evict it just the same, so the id is checked against # every borrowed run the queue is carrying, not only against this prompt. if requested and requested in record.borrowed_ids: - raise _ClientIdRejected("borrowed") + raise _ClientIdRejected("id_borrowed") # ...and that check is worth exactly as much as the read behind it. An # empty census means "nobody is borrowing" only once the queue was actually @@ -2072,7 +2114,7 @@ def _select_watch_client_id( # record vouches for THIS run, and says nothing about whose socket the # caller just named. if requested and not record.queue_read: - raise _ClientIdRejected("indeterminate") + raise _ClientIdRejected("census_unread") # A state file is its own positive evidence: `comfy run` wrote it, and it # says this run did not borrow. Failing that, only the prompt's own server @@ -2080,7 +2122,7 @@ def _select_watch_client_id( # server that simply never mentioned the prompt says nothing either. if job is None and not record_read: if requested: - raise _ClientIdRejected("indeterminate") + raise _ClientIdRejected("nothing_vouched") return None, "indeterminate" recorded = job.client_id if job is not None and isinstance(job.client_id, str) and job.client_id.strip() else None @@ -2398,15 +2440,16 @@ def watch_cmd( # The key is still published so a consumer that branches on it never # hits a missing one, as with `client_id` and `attached`. snap["poll_reason"] = None + if renderer.is_pretty(): + renderer.console().print(f"[dim]Prompt {prompt_id} already {snap['status']}; nothing more to watch.[/dim]") + # Before the promotion below, which moves the key this renders from. + _render_status_pretty(snap, host=h, port=p) # Same promotion the live path does when it ends. `_emit_terminal` # keys its trimming and redaction on `execution_error`, so a failure # left where `_snapshot` puts it reaches the envelope whole — full # traceback, and a `current_inputs` that can hold an api_key. if isinstance(snap.get("error"), dict): snap["execution_error"] = snap.pop("error") - if renderer.is_pretty(): - renderer.console().print(f"[dim]Prompt {prompt_id} already {snap['status']}; nothing more to watch.[/dim]") - _render_status_pretty(snap, host=h, port=p) _emit_terminal(renderer, snap, command="jobs watch") return @@ -2415,18 +2458,7 @@ def watch_cmd( try: attached_client_id, poll_reason = _select_watch_client_id(h, p, prompt_id, client_id) except _ClientIdRejected as e: - if e.reason == "borrowed": - message = "This run borrowed another client's live socket and can only be watched by polling" - hint = "drop --client-id; comfy jobs watch will poll this run safely" - else: - message = ( - "Could not confirm that --client-id is safe to attach as: this prompt has no record " - "we could read, so whether it borrowed a live client's socket is unknown" - ) - hint = ( - "drop --client-id and comfy jobs watch will poll this run safely; if the server was " - "briefly unreachable, retry — if the prompt_id is a typo or already pruned, no id works" - ) + message, hint = e.text renderer.error( code="client_id_rejected", message=message, diff --git a/comfy_cli/schemas/jobs.json b/comfy_cli/schemas/jobs.json index 0eef5e98a..1143b6992 100644 --- a/comfy_cli/schemas/jobs.json +++ b/comfy_cli/schemas/jobs.json @@ -42,7 +42,7 @@ }, "execution_error": { "type": ["object", "null"], - "description": "Cloud `jobs status`/`jobs watch`: the failed job's structured cause from `/api/jobs/` (`JobDetailResponse.execution_error` — `node_id`, `node_type`, `exception_message`, `exception_type`, `traceback`). Null on every job the server reports no execution error for. `current_inputs` is always redacted — the failing node's widget values routinely carry API keys — and on the `jobs watch` terminal envelope `traceback` is capped at its last two frames, the same budget `error.details.traceback_tail` uses; `jobs status` carries it in full. The top-level `error_message` is the same record flattened to one human line; this key is what a `--json` consumer reads to get the fields back without re-parsing that line.", + "description": "`jobs status`/`jobs watch`, cloud from `/api/jobs/` and local from `/history`: the failed job's structured cause from `/api/jobs/` (`JobDetailResponse.execution_error` — `node_id`, `node_type`, `exception_message`, `exception_type`, `traceback`). Null on every job the server reports no execution error for. `current_inputs` is always redacted — the failing node's widget values routinely carry API keys — and on the `jobs watch` terminal envelope `traceback` is capped at its last two frames, the same budget `error.details.traceback_tail` uses; `jobs status` carries it in full. The top-level `error_message` is the same record flattened to one human line; this key is what a `--json` consumer reads to get the fields back without re-parsing that line.", "additionalProperties": true }, "scope": { diff --git a/docs/json-output.md b/docs/json-output.md index c646fd4e2..6c228e923 100644 --- a/docs/json-output.md +++ b/docs/json-output.md @@ -59,7 +59,8 @@ opens no socket at all. Separately, and by design, this protects only runs *comfy-cli* submitted with `--client-id`: a browser tab that submits its own prompt stamps no marker, so watching that prompt re-attaches as the tab, exactly as the last paragraph of this section describes for any re-attach. Both -`data.client_id` and `data.attached` are present on *every* terminal envelope: a +`data.client_id` and `data.attached` are present on every *local* `jobs watch` +terminal envelope (cloud watch emits neither): a watch of an already-finished prompt short-circuits without opening a socket, and reports `client_id: null` / `attached: false`. Reconnecting under an existing id is ComfyUI's own session-resume path, so a submitter that is *still* holding that diff --git a/tests/comfy_cli/command/test_run_client_id.py b/tests/comfy_cli/command/test_run_client_id.py index 96ae66e68..502a61fdc 100644 --- a/tests/comfy_cli/command/test_run_client_id.py +++ b/tests/comfy_cli/command/test_run_client_id.py @@ -216,9 +216,19 @@ def _validate_watch_envelope(data): jsonschema.Draft202012Validator(json.loads(schema_path.read_text())).validate(data) -def _record(extra=None, *, read=True, borrowed_ids=(), queue_read=True): - """A `_submitted_extra_data` result, for stubbing it out.""" - return _SubmittedRecord(extra, read, frozenset(borrowed_ids), queue_read) +def _record(extra=None, *, read=None, borrowed_ids=(), queue_read=True): + """A `_submitted_extra_data` result, for stubbing it out. + + `read` defaults the way production derives it — a dict slot is a record + that was read, anything else is not — so a stub cannot model a pair + `_submitted_extra_data` would never return. + """ + return _SubmittedRecord( + extra, + isinstance(extra, dict) if read is None else read, + frozenset(borrowed_ids), + queue_read, + ) class TestWatchDoesNotStealTheSocket: @@ -226,9 +236,10 @@ class TestWatchDoesNotStealTheSocket: def record_read_but_empty(self, monkeypatch): """Default: this prompt's record WAS read, and carries no marker. - Note `read=True` with `extra=None` is the shape for a record that was - found and is unmarked — NOT for a server that holds no record at all, - which reports `read=False` and is pinned in the classes below. + Note `read=True` goes with a dict `extra` — `{}` is a record found and + unmarked. It is NOT the shape for a server that holds no record at all, + nor for a non-dict slot; both report `read=False` and are pinned in the + classes below. """ monkeypatch.setattr(jobs, "_submitted_extra_data", lambda *_: _record({})) @@ -543,11 +554,11 @@ def test_a_history_hit_counts_even_if_the_queue_was_unreachable(self, monkeypatc "no-queue-sections", "unwalkable-section", "entry-missing-extra-slot", - "history-entry-malformed", "slot-null", "slot-list", "slot-string", "history-slot-number", + "history-entry-malformed", ], ) def test_everything_else_is_not_a_record(self, monkeypatch, queue, history, why): @@ -675,8 +686,14 @@ def boom(url, **_kw): ) def test_a_half_walkable_queue_is_not_a_census(self, monkeypatch, queue_body): """A borrowed run in the section we could not walk would be missing - from the census without the census knowing it.""" - monkeypatch.setattr(jobs_state, "read", lambda _: None) + from the census without the census knowing it. + + The state file is READABLE on purpose: without it the record-side gate + would raise `indeterminate` too and the census guard could be deleted + with this test still green. + """ + state = jobs_state.new(prompt_id="P_OTHER", client_id="ordinary", workflow="w.json", where="local") + monkeypatch.setattr(jobs_state, "read", lambda _: state) monkeypatch.setattr( jobs, "_http_get_json", @@ -744,6 +761,81 @@ def test_the_traceback_is_capped_to_its_tail(self): assert "frame0" not in json.dumps(env) +class TestEachRefusalGivesAdviceThatMatchesWhatHappens: + """Four situations reach two published reasons, and the advice differs. + + Dropping `--client-id` makes a run carrying its OWN marker poll; it makes + the other three ATTACH under their own resolved id. One message per reason + told half of them the opposite of what the command actually does. + """ + + CASES = { + "prompt_borrowed": ( + {"queue_running": [[0, "p", {}, _marked(), {}]], "queue_pending": []}, + "borrowed", + "poll this run safely", + ), + "id_borrowed": ( + { + "queue_running": [ + [0, "P_B", {}, _marked(), {}], + [1, "p", {}, {"client_id": "ordinary"}, {}], + ], + "queue_pending": [], + }, + "borrowed", + "attaches normally", + ), + "nothing_vouched": ( + {"queue_running": [], "queue_pending": []}, + "indeterminate", + "poll this run safely", + ), + } + + @pytest.fixture(autouse=True) + def live_watch(self, monkeypatch): + monkeypatch.setattr(jobs, "_server_or_error", lambda *_a, **_k: True) + monkeypatch.setattr(jobs, "_snapshot", lambda *_: {"prompt_id": "p", "status": "running", "outputs": []}) + monkeypatch.setattr(jobs_state, "read", lambda _: None) + + @pytest.mark.parametrize("cause", list(CASES)) + def test_the_hint_names_what_dropping_the_flag_does(self, monkeypatch, cause): + queue, reason, expected_hint = self.CASES[cause] + monkeypatch.setattr(jobs, "_http_get_json", lambda u, **_k: queue if u.endswith("/queue") else {}) + result = CliRunner().invoke(jobs.app, ["watch", "p", "--where", "local", "--client-id", BORROWED]) + assert result.exit_code == 1, result.output + err = _envelope(result.output)["error"] + assert err["details"]["reason"] == reason + assert expected_hint in err["hint"] + + def test_an_unread_census_says_so_rather_than_claiming_no_record(self, monkeypatch): + """This one has a readable record — the census is what failed — so the + `nothing_vouched` wording would be a flat lie here.""" + state = jobs_state.new(prompt_id="p", client_id="ordinary", workflow="w.json", where="local") + monkeypatch.setattr(jobs_state, "read", lambda _: state) + + def boom(url, **_kw): + if url.endswith("/queue"): + raise RuntimeError("connection refused") + return {} + + monkeypatch.setattr(jobs, "_http_get_json", boom) + result = CliRunner().invoke(jobs.app, ["watch", "p", "--where", "local", "--client-id", BORROWED]) + assert result.exit_code == 1, result.output + err = _envelope(result.output)["error"] + assert err["details"]["reason"] == "indeterminate" + assert "/queue" in err["message"] + assert "no record" not in err["message"] + assert "retry once the server answers" in err["hint"] + + def test_every_cause_has_its_own_wording(self): + """A cause added later must not silently reuse another's advice.""" + texts = [text for _reason, *text in jobs._REJECTION_TEXT.values()] + assert len(texts) == len(jobs._REJECTION_TEXT) + assert len({tuple(t) for t in texts}) == len(texts) + + class TestTheOrdinaryCaseStillAttaches: """The guard must not turn every watch into a poll. From 0ce680ecd41ff9b59abfec076f385542e97d63a8 Mon Sep 17 00:00:00 2001 From: Christian Byrne Date: Tue, 6 Oct 2026 23:07:48 -0700 Subject: [PATCH 17/22] test: avoid public hygiene false positive on client-id carrier (#979) --- tests/comfy_cli/command/test_run_client_id.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/comfy_cli/command/test_run_client_id.py b/tests/comfy_cli/command/test_run_client_id.py index 502a61fdc..f283004d1 100644 --- a/tests/comfy_cli/command/test_run_client_id.py +++ b/tests/comfy_cli/command/test_run_client_id.py @@ -731,7 +731,7 @@ class TestAnAlreadyTerminalFailureIsTrimmedToo: "exception_message": "boom", "node_id": "5", "traceback": [f"frame{i}" for i in range(12)], - "current_inputs": {"api_key": "sk-SECRET-123"}, + "current_inputs": {"api_key": "sk-redact-me"}, }, } @@ -753,7 +753,7 @@ def test_the_failure_is_published_where_the_live_path_publishes_it(self): assert "error" not in env["error"]["details"] def test_a_secret_in_current_inputs_does_not_reach_the_envelope(self): - assert "sk-SECRET-123" not in json.dumps(self._env()) + assert "sk-redact-me" not in json.dumps(self._env()) def test_the_traceback_is_capped_to_its_tail(self): env = self._env() From b2c8d5cedc891e5ecf8ee672fa1edfde91d65912 Mon Sep 17 00:00:00 2001 From: Glary-Bot Date: Wed, 7 Oct 2026 06:58:56 +0000 Subject: [PATCH 18/22] docs(json-output): execution_error and poll_reason on a watch envelope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review pointed out that the `execution_error` promotion applies to every error-ending `jobs watch`, not only the borrowed/poll-only one my commit message named, and that the reference doc described it only for the `--client-id` case. Agreed on both, and the broader scope is the point: the shared classifier reads that key to produce error.code/message/hint, and the trimming that scrubs `current_inputs` and caps `traceback` keys on it too, so a failure left anywhere else arrives unclassified AND unredacted. Document it as a general field on both of the command's exits, name the two earlier locations as gone, and say `data.details` keeps the rest. `poll_reason` had the same gap in the other direction: added to the envelope and to schemas/jobs.json last round, never described here — which is where a --json consumer would look to learn that `borrowed` and `indeterminate` want opposite responses. Every claim checked against a real envelope rather than the source: both exits publish `execution_error`, neither publishes `details.error` or `data.error`, `current_inputs` comes back scrubbed and `traceback` capped to its last two frames, the live path keeps `data.details`, and `poll_reason` is null on the already-terminal short-circuit. --- docs/json-output.md | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/docs/json-output.md b/docs/json-output.md index 6c228e923..67aad60a6 100644 --- a/docs/json-output.md +++ b/docs/json-output.md @@ -71,6 +71,28 @@ it reconnects. is the union of what the watch observed and what `/history` records for the prompt, so it is populated even for a watch that attached after the job ended. +`data.poll_reason` says why a watch polled instead of opening a socket, and is +`null` when it attached (and on a prompt that was already terminal, which opens +no socket either). `"borrowed"` means the run was submitted for another client +that is receiving its events — drop `--client-id` and keep polling. +`"indeterminate"` means nothing could vouch for the run — retry once the server +answers. `data.attached` alone cannot separate those two, and they want opposite +responses, which is why the reason is published rather than only printed. + +**`data.execution_error` on a failed watch.** Any `jobs watch` that ends in +`status: "error"` — not just a borrowed or poll-only one — carries the node's +structured failure under `data.execution_error`, never under +`data.details.error`. Both of the command's exits do this: the live/poll path +and the short-circuit for a prompt that had already failed before the watch +started. `data.details` keeps the rest of the record on the paths that have one. +The key is what the shared error classifier reads to produce +`error.code` / `error.message` / `error.hint` — the same path `comfy run` uses — +so a failure left anywhere else reaches the envelope unclassified, and also +unredacted: `current_inputs` is scrubbed and `traceback` capped to its last two +frames only for the value under this key. Earlier builds of this branch put it +at `data.details.error` (live path) and `data.error` (terminal path); neither is +published any more. + ## Overview When `--json` is passed, `comfy run` switches into a strict From 9adbb5bb2346f7832fad59b10db53c8798af4286 Mon Sep 17 00:00:00 2001 From: bymyself Date: Tue, 6 Oct 2026 19:30:44 +0000 Subject: [PATCH 19/22] fix(jobs): URL-encode prompt_id in every /history/{prompt_id} request https://github.com/Comfy-Org/comfy-cli/pull/970#discussion_r_jobs1919 prompt_id reaches the URL via raw f-string interpolation at four call sites (two already fixed call sites elsewhere in this file use urllib.parse.quote, which is the established convention here). An id containing ?, #, or ../ would retarget the request against the local ComfyUI server instead of raising a clear error. The review flagged one instance; the other three are the identical pattern and get the same fix. --- comfy_cli/command/jobs.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/comfy_cli/command/jobs.py b/comfy_cli/command/jobs.py index 361386bd7..0ffd63c86 100644 --- a/comfy_cli/command/jobs.py +++ b/comfy_cli/command/jobs.py @@ -1082,7 +1082,7 @@ def _server_confirms_no_record(host: str, port: int, prompt_id: str) -> bool: """ try: q = _http_get_json(f"http://{host}:{port}/queue") - hist = _http_get_json(f"http://{host}:{port}/history/{prompt_id}") + hist = _http_get_json(f"http://{host}:{port}/history/{urllib.parse.quote(prompt_id, safe='')}") except RuntimeError: return False if not isinstance(q, dict) or not isinstance(hist, dict): @@ -1271,7 +1271,7 @@ def _snapshot(host: str, port: int, prompt_id: str) -> dict | None: # Then: history. try: - h = _http_get_json(f"http://{host}:{port}/history/{prompt_id}") + h = _http_get_json(f"http://{host}:{port}/history/{urllib.parse.quote(prompt_id, safe='')}") except RuntimeError: return None if not isinstance(h, dict) or prompt_id not in h: @@ -1970,7 +1970,6 @@ def _get_json_object(url: str) -> dict[str, Any]: return {} return body if isinstance(body, dict) else {} - class _SubmittedRecord(NamedTuple): """Everything one pass over the server's two stores established. @@ -2040,7 +2039,7 @@ def _submitted_extra_data(host: str, port: int, prompt_id: str) -> _SubmittedRec if hit is not None: return _SubmittedRecord(hit[0], hit[1], frozenset(borrowed), queue_read) - history = _get_json_object(f"http://{host}:{port}/history/{prompt_id}") + history = _get_json_object(f"http://{host}:{port}/history/{urllib.parse.quote(prompt_id, safe='')}") body = history.get(prompt_id) prompt = body.get("prompt") if isinstance(body, dict) else None if isinstance(prompt, list) and len(prompt) > 3 and isinstance(prompt[3], dict): @@ -2140,7 +2139,7 @@ def _history_completed_nodes(host: str, port: int, prompt_id: str) -> set[str]: """ nodes: set[str] = set() try: - h = _http_get_json(f"http://{host}:{port}/history/{prompt_id}") + h = _http_get_json(f"http://{host}:{port}/history/{urllib.parse.quote(prompt_id, safe='')}") except RuntimeError: return nodes body = h.get(prompt_id) if isinstance(h, dict) else None From 232ddcca71ee9619060bce1b044f3f31d2742dfc Mon Sep 17 00:00:00 2001 From: bymyself Date: Wed, 7 Oct 2026 07:16:43 +0000 Subject: [PATCH 20/22] fix(run): harden the borrowed-client-id guard against edge cases Three review findings on WorkflowExecution's borrowed-client handling: https://github.com/Comfy-Org/comfy-cli/pull/970#discussion_r_execution140 bool(client_id) treated a whitespace-only id as borrowed. Strip before the bool() check, matching the .strip() convention cmdline.py already uses at the CLI entry point -- this class is a reusable surface a programmatic caller can construct directly, bypassing that validation. https://github.com/Comfy-Org/comfy-cli/pull/970#discussion_r_execution185 execution.connect()'s borrowed-client RuntimeError had no catcher in execute(); cmdline.py already rejects --wait + --client-id before dispatch so this was unreachable through the CLI, but a programmatic caller bypassing that validation would crash with a raw traceback. Checked before the try/except instead of adding a new except clause, since execute() is already at mccabe's ratcheted complexity ceiling (pyproject.toml: 48, "never raise it") -- extracted to _reject_borrowed_wait so the check costs one call site, not one branch. https://github.com/Comfy-Org/comfy-cli/pull/970#discussion_r_execution223 The borrowed-client marker write was one-directional: only ever set to True, never cleared. A caller-supplied extra_data already carrying the key would survive into a genuinely non-borrowed submission, marking every future watcher poll-only for a run that was never borrowed. Pop the key in the non-borrowed branch. Both test files mock WorkflowExecution with bare MagicMock(), which auto-vivifies an unconfigured .borrowed_client_id as a truthy child mock -- 41 tests across test_run.py/test_run_prompt.py were tripping the new _reject_borrowed_wait check as a false positive. Set borrowed_client_id=False explicitly on every such fixture. --- comfy_cli/command/run/__init__.py | 28 ++++++++++++++++++++++ comfy_cli/command/run/execution.py | 13 +++++++++- tests/comfy_cli/command/test_run.py | 25 +++++++++++++++++++ tests/comfy_cli/command/test_run_prompt.py | 5 ++-- 4 files changed, 68 insertions(+), 3 deletions(-) diff --git a/comfy_cli/command/run/__init__.py b/comfy_cli/command/run/__init__.py index 5994ed44d..3a77a346c 100644 --- a/comfy_cli/command/run/__init__.py +++ b/comfy_cli/command/run/__init__.py @@ -457,6 +457,8 @@ def execute( # `completed` record to `error` and flipping a successful run's exit to 1. completed_payload: dict | None = None + _reject_borrowed_wait(renderer, execution, wait) + try: if wait: execution.connect() @@ -794,6 +796,32 @@ def _write_state(state): return None +def _reject_borrowed_wait(renderer, execution, wait: bool) -> None: + """Refuse --wait on a borrowed client_id before execute() ever tries to + connect, so the condition doesn't add a branch to that function's already + at-ceiling cyclomatic complexity (see the mccabe ratchet note in + pyproject.toml). Without this, execution.connect() would raise + RuntimeError instead -- it refuses to open a websocket as a borrowed + clientId, since that would evict the client this run submits on behalf + of. cmdline.py already rejects --wait combined with --client-id before + dispatch, so this is unreachable through the CLI; it exists for a + programmatic caller that constructs WorkflowExecution directly with + wait=True and a borrowed client_id, bypassing that validation. + """ + if not (wait and execution.borrowed_client_id): + return + renderer.error( + code="client_id_rejected", + message=( + f"refusing to open a websocket as borrowed clientId {execution.client_id!r}: " + "it would evict the client this run submits on behalf of" + ), + hint="drop --client-id, or call without wait=True", + details={"reason": "borrowed"}, + ) + raise typer.Exit(code=1) + + def _mark_cancelled(state): """Record a user cancellation on the job state file (best effort). diff --git a/comfy_cli/command/run/execution.py b/comfy_cli/command/run/execution.py index adb458080..dd4f57860 100644 --- a/comfy_cli/command/run/execution.py +++ b/comfy_cli/command/run/execution.py @@ -137,6 +137,11 @@ def __init__( # ComfyUI's /ws handler pops any socket already registered under an # incoming clientId, so opening one with a BORROWED id would evict the # client we are submitting on behalf of. Callers gate on this flag. + # cmdline.run already rejects a blank --client-id, but this class is a + # reusable surface a programmatic caller can construct directly without + # going through that validation, so strip here too rather than trust + # the caller — a whitespace-only id must not look like a real one. + client_id = client_id.strip() if isinstance(client_id, str) else client_id self.borrowed_client_id = bool(client_id) self.client_id = client_id or str(uuid.uuid4()) self.outputs: list = [] @@ -219,9 +224,15 @@ def queue(self): elif self.api_key: data["extra_data"]["api_key_comfy_org"] = self.api_key # This is a safety property, not caller metadata: keep it authoritative - # even if a future extra_data caller happens to use the same key. + # in both directions, not just when setting it. A caller-supplied + # extra_data that happens to carry this key (forwarded from another + # prompt, or any other source) must not survive into a genuinely + # non-borrowed submission -- that would mark every future watcher of + # this prompt poll-only for a run that was never borrowed. if self.borrowed_client_id: data["extra_data"][jobs_state.BORROWED_CLIENT_ID_KEY] = True + else: + data["extra_data"].pop(jobs_state.BORROWED_CLIENT_ID_KEY, None) req = request.Request(f"http://{self.host}:{self.port}/prompt", json.dumps(data).encode("utf-8")) req.add_header("Comfy-Usage-Source", "comfy-cli") try: diff --git a/tests/comfy_cli/command/test_run.py b/tests/comfy_cli/command/test_run.py index 2006a9ec9..28ea1df2f 100644 --- a/tests/comfy_cli/command/test_run.py +++ b/tests/comfy_cli/command/test_run.py @@ -449,6 +449,7 @@ def test_timeout_exits_with_code_1(self, workflow_file): patch("comfy_cli.command.run.WorkflowExecution") as MockExec, ): mock_exec = MagicMock() + mock_exec.borrowed_client_id = False MockExec.return_value = mock_exec mock_exec.watch_execution.side_effect = WebSocketTimeoutException("timed out") @@ -462,6 +463,7 @@ def test_connection_error_exits_with_code_1(self, workflow_file): patch("comfy_cli.command.run.WorkflowExecution") as MockExec, ): mock_exec = MagicMock() + mock_exec.borrowed_client_id = False MockExec.return_value = mock_exec mock_exec.connect.side_effect = ConnectionError("Connection refused") @@ -475,6 +477,7 @@ def test_websocket_exception_exits_with_code_1(self, workflow_file): patch("comfy_cli.command.run.WorkflowExecution") as MockExec, ): mock_exec = MagicMock() + mock_exec.borrowed_client_id = False MockExec.return_value = mock_exec mock_exec.watch_execution.side_effect = WebSocketException("Connection lost") @@ -490,6 +493,7 @@ def test_successful_execution(self, workflow_file): mock_progress = MagicMock() MockProgress.return_value = mock_progress mock_exec = MagicMock() + mock_exec.borrowed_client_id = False MockExec.return_value = mock_exec mock_exec.outputs = [] @@ -510,6 +514,7 @@ def test_websocket_closed_on_watch_failure(self, workflow_file): patch("comfy_cli.command.run.WorkflowExecution") as MockExec, ): mock_exec = MagicMock() + mock_exec.borrowed_client_id = False MockExec.return_value = mock_exec mock_exec.watch_execution.side_effect = WebSocketTimeoutException("timed out") @@ -580,6 +585,7 @@ def test_progress_stopped_on_error(self, workflow_file): mock_progress = MagicMock() MockProgress.return_value = mock_progress mock_exec = MagicMock() + mock_exec.borrowed_client_id = False MockExec.return_value = mock_exec mock_exec.watch_execution.side_effect = WebSocketTimeoutException("timed out") @@ -606,8 +612,14 @@ def fresh_token(self): def _mock_exec(self, prompt_id): mock_exec = MagicMock() + mock_exec.borrowed_client_id = False mock_exec.prompt_id = prompt_id mock_exec.client_id = "cid-wait" + # Explicit, not the MagicMock default: an unconfigured attribute + # auto-vivifies as a truthy MagicMock(), which would make every test + # using this fixture look like a borrowed-client run to + # _reject_borrowed_wait. + mock_exec.borrowed_client_id = False mock_exec.outputs = [] mock_exec.output_entries = [] mock_exec.cached_node_ids = [] @@ -1623,6 +1635,7 @@ def test_proceeds_and_injects_credential_when_available(self, tmp_path, monkeypa patch("comfy_cli.command.run.WorkflowExecution") as MockExec, ): mock_exec = MagicMock() + mock_exec.borrowed_client_id = False MockExec.return_value = mock_exec mock_exec.outputs = [] # allow_spend=True: consent granted, so the spend gate is a no-op @@ -1652,6 +1665,7 @@ def test_non_partner_workflow_skips_preflight(self, workflow_file, monkeypatch): patch("comfy_cli.command.run.WorkflowExecution") as MockExec, ): mock_exec = MagicMock() + mock_exec.borrowed_client_id = False MockExec.return_value = mock_exec mock_exec.outputs = [] execute(workflow_file, host="127.0.0.1", port=8188, wait=True, timeout=30) @@ -1710,6 +1724,7 @@ def test_fires_with_credential_present_when_api_key_supplied(self, tmp_path, mon patch("comfy_cli.tracking.track_event") as mock_track, ): mock_exec = MagicMock() + mock_exec.borrowed_client_id = False MockExec.return_value = mock_exec mock_exec.outputs = [] execute(wf_file, host="127.0.0.1", port=8188, wait=True, timeout=30, api_key="k", allow_spend=True) @@ -1738,6 +1753,7 @@ def test_fires_with_credential_present_when_env_key_available(self, tmp_path, mo patch("comfy_cli.tracking.track_event") as mock_track, ): mock_exec = MagicMock() + mock_exec.borrowed_client_id = False MockExec.return_value = mock_exec mock_exec.outputs = [] execute(wf_file, host="127.0.0.1", port=8188, wait=True, timeout=30, allow_spend=True) @@ -1762,6 +1778,7 @@ def test_does_not_fire_for_partner_free_workflow(self, workflow_file): patch("comfy_cli.tracking.track_event") as mock_track, ): mock_exec = MagicMock() + mock_exec.borrowed_client_id = False MockExec.return_value = mock_exec mock_exec.outputs = [] execute(workflow_file, host="127.0.0.1", port=8188, wait=True, timeout=30) @@ -2071,6 +2088,7 @@ def test_paid_node_with_allow_spend_proceeds(self, tmp_path, monkeypatch): patch("comfy_cli.command.run.WorkflowExecution") as MockExec, ): mock_exec = MagicMock() + mock_exec.borrowed_client_id = False MockExec.return_value = mock_exec mock_exec.outputs = [] # A stored api_key means the missing-credential path is satisfied, @@ -2096,6 +2114,7 @@ def test_no_paid_nodes_never_gates(self, workflow_file, monkeypatch): patch("comfy_cli.command.run.WorkflowExecution") as MockExec, ): mock_exec = MagicMock() + mock_exec.borrowed_client_id = False MockExec.return_value = mock_exec mock_exec.outputs = [] execute(workflow_file, host="127.0.0.1", port=8188, wait=True, timeout=30) @@ -2145,6 +2164,7 @@ def test_tty_confirm_accepted_proceeds(self, tmp_path, monkeypatch): patch("comfy_cli.command.run.WorkflowExecution") as MockExec, ): mock_exec = MagicMock() + mock_exec.borrowed_client_id = False MockExec.return_value = mock_exec mock_exec.outputs = [] execute(wf_file, host="127.0.0.1", port=8188, wait=True, timeout=30, api_key="k") @@ -2285,6 +2305,7 @@ def test_ui_workflow_is_converted_then_executed(self, ui_workflow_file): patch("comfy_cli.command.run.WorkflowExecution") as MockExec, ): mock_exec = MagicMock() + mock_exec.borrowed_client_id = False MockExec.return_value = mock_exec mock_exec.outputs = [] @@ -2334,6 +2355,7 @@ def test_ui_workflow_plumbs_api_key_through_to_execution(self, ui_workflow_file) patch("comfy_cli.command.run.WorkflowExecution") as MockExec, ): mock_exec = MagicMock() + mock_exec.borrowed_client_id = False MockExec.return_value = mock_exec mock_exec.outputs = [] @@ -2970,6 +2992,7 @@ def test_local_execute_strips_meta_before_submit(self, tmp_path): patch("comfy_cli.command.run.WorkflowExecution") as MockExec, ): mock_exec = MagicMock() + mock_exec.borrowed_client_id = False MockExec.return_value = mock_exec mock_exec.outputs = [] execute(str(path), host="127.0.0.1", port=8188, wait=True, timeout=30) @@ -3008,6 +3031,7 @@ def composed_workflow_file(self, tmp_path): def _mock_exec(self, prompt_id): mock_exec = MagicMock() + mock_exec.borrowed_client_id = False mock_exec.prompt_id = prompt_id mock_exec.client_id = "cid-local" mock_exec.outputs = [] @@ -3173,6 +3197,7 @@ def test_cloud_submit_journals_inside_project(self, proj_dir, fake_target): def test_local_submit_journals_inside_project(self, proj_dir): workflow_file = self._workflow_file(proj_dir) mock_exec = MagicMock() + mock_exec.borrowed_client_id = False mock_exec.prompt_id = "prompt-local-journal" mock_exec.client_id = "cid" mock_exec.outputs = [] diff --git a/tests/comfy_cli/command/test_run_prompt.py b/tests/comfy_cli/command/test_run_prompt.py index f5a8086fd..c67139558 100644 --- a/tests/comfy_cli/command/test_run_prompt.py +++ b/tests/comfy_cli/command/test_run_prompt.py @@ -43,6 +43,7 @@ def test_preloaded_graph_is_submitted(self): patch("comfy_cli.command.run.WorkflowExecution") as MockExec, ): mock_exec = MagicMock() + mock_exec.borrowed_client_id = False MockExec.return_value = mock_exec mock_exec.outputs = [] @@ -68,7 +69,7 @@ def test_preloaded_skips_file_loading(self): patch("comfy_cli.command.run.ExecutionProgress"), patch("comfy_cli.command.run.WorkflowExecution") as MockExec, ): - MockExec.return_value = MagicMock(outputs=[]) + MockExec.return_value = MagicMock(outputs=[], borrowed_client_id=False) # Positional workflow is a bogus path — must be ignored. execute( "/no/such/file.json", @@ -169,7 +170,7 @@ def _run_local(self, preloaded, object_info, patch_pprint=False): patch("comfy_cli.command.run.WorkflowExecution"), ] with stack[0], stack[1], stack[2], stack[3], stack[4] as MockExec: - MockExec.return_value = MagicMock(outputs=[]) + MockExec.return_value = MagicMock(outputs=[], borrowed_client_id=False) if patch_pprint: with patch("comfy_cli.command.run.preflight.pprint") as mock_pprint: execute(None, host="127.0.0.1", port=8188, wait=True, timeout=30, preloaded=preloaded) From c1169ac851866ed7b8a34415c190c6068fdffd5e Mon Sep 17 00:00:00 2001 From: bymyself Date: Wed, 7 Oct 2026 07:44:44 +0000 Subject: [PATCH 21/22] style: restore blank line before _SubmittedRecord (ruff format) --- comfy_cli/command/jobs.py | 1 + 1 file changed, 1 insertion(+) diff --git a/comfy_cli/command/jobs.py b/comfy_cli/command/jobs.py index 0ffd63c86..270e30183 100644 --- a/comfy_cli/command/jobs.py +++ b/comfy_cli/command/jobs.py @@ -1970,6 +1970,7 @@ def _get_json_object(url: str) -> dict[str, Any]: return {} return body if isinstance(body, dict) else {} + class _SubmittedRecord(NamedTuple): """Everything one pass over the server's two stores established. From 722446f9e8a73b95a171af9f17066d8eb55d0ee9 Mon Sep 17 00:00:00 2001 From: Glary-Bot Date: Wed, 14 Oct 2026 02:45:14 +0000 Subject: [PATCH 22/22] fix(jobs): promote a live watch's execution_error event to execution_error --- comfy_cli/command/jobs.py | 10 +++++++++ tests/comfy_cli/jobs/test_jobs.py | 37 +++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+) diff --git a/comfy_cli/command/jobs.py b/comfy_cli/command/jobs.py index 270e30183..5c52667b9 100644 --- a/comfy_cli/command/jobs.py +++ b/comfy_cli/command/jobs.py @@ -2184,6 +2184,10 @@ class _WatchState: outputs: list[str] = field(default_factory=list) end_reason: str | None = None end_details: Any = None + # The WS `execution_error` event payload, when the watch ended on one. The + # poll/snapshot exits carry the same record under `end_details["error"]`; + # both are surfaced as the envelope's `execution_error`. + execution_error: dict[str, Any] | None = None terminal: bool = False # Nodes whose final (100% / errored) `progress` line has already been # emitted. `progress_state` repeats EVERY non-pending node on every @@ -2320,6 +2324,7 @@ def _watch_execution_error(state: _WatchState, data: dict[str, Any]) -> None: _absorb_executed_list(state, data) state.end_reason = "error" state.end_details = data + state.execution_error = data state.terminal = True @@ -2629,6 +2634,11 @@ def watch_cmd( if final_status == "error" and isinstance(details.get("error"), dict): payload["execution_error"] = details.pop("error") payload["details"] = details + if final_status == "error" and "execution_error" not in payload and isinstance(state.execution_error, dict): + # Live exit: the event itself is the structured record (the same shape + # `/history` stores under `status.messages`), so promote it too — one + # key for the failure whichever way the watch ended. + payload["execution_error"] = dict(state.execution_error) if not saw_any_event and final_status == "unknown": payload["hint"] = "watch returned without events; the prompt may already have completed" _emit_terminal(renderer, payload, command="jobs watch") diff --git a/tests/comfy_cli/jobs/test_jobs.py b/tests/comfy_cli/jobs/test_jobs.py index ab9037793..d55916a70 100644 --- a/tests/comfy_cli/jobs/test_jobs.py +++ b/tests/comfy_cli/jobs/test_jobs.py @@ -3601,6 +3601,43 @@ def test_watch_streams_events_and_reports_completed_nodes(monkeypatch, capsys): assert env["data"]["attached"] is True and env["data"]["client_id"] == "cid-sub" +def test_watch_live_execution_error_is_promoted_to_execution_error(monkeypatch, capsys): + """An attached watch that ends on the WS `execution_error` event publishes + the failure under `execution_error`, the same key the poll exits use — the + event payload itself has no `error` key to lift.""" + from comfy_cli import jobs_state + + jobs_state.write(jobs_state.new(prompt_id="pid-w", client_id="cid-sub", workflow="w", where="local")) + monkeypatch.setattr(jobs_mod, "_snapshot", lambda h, p, pid: {"prompt_id": pid, "status": "running", "outputs": []}) + monkeypatch.setattr(jobs_mod, "_history_completed_nodes", lambda h, p, pid: set()) + frames = [f"frame{i}" for i in range(6)] + event = { + "prompt_id": "pid-w", + "node_id": "5", + "node_type": "KSampler", + "exception_message": "boom", + "exception_type": "RuntimeError", + "traceback": frames, + "current_inputs": {"seed": 42}, + "executed": ["1"], + } + result, ws, lines = _run_local_watch(monkeypatch, capsys, messages=[{"type": "execution_error", "data": event}]) + + assert result.exit_code == 1, result.output + assert "clientId=cid-sub" in ws.url + env = lines[-1] + assert env["ok"] is False and env["error"]["code"] == "execution_error" + payload = env["error"]["details"] + assert payload["status"] == "error" and payload["attached"] is True + record = payload["execution_error"] + assert record["node_type"] == "KSampler" and record["node_id"] == "5" + assert record["exception_message"] == "boom" + # Same envelope hygiene as the poll exit: widget values redacted, traceback + # capped to its tail. + assert "current_inputs" not in record + assert record["traceback"] == frames[-2:] + + def test_watch_terminal_envelope_backfills_completed_nodes_without_events(monkeypatch, capsys): """Symptom 2 is independent of streaming: even with zero WS events, the envelope's `completed_nodes` comes from the server's own /history record."""