fix(run): never report a job-state write failure as a lost server connection (#1023)

Author: christian-byrne

CHANGELOG.md

@@ -135,6 +135,18 @@ history.
 
 ### Fixed
 
+- `comfy run` no longer reports a failure to write its own job-state file as a
+  lost connection to the ComfyUI server. The state file is a convenience
+  record, written after the server has already accepted the prompt, so a
+  config directory the CLI cannot write — an unreachable or read-only home, a
+  sandboxed process, a full disk — used to surface as `ws_disconnected` with a
+  "check the server is still running; re-run the command" hint. Re-running on
+  that advice submits the same workflow a second time, and an automated caller
+  obeying the hint could queue several GPU jobs for one request. A submit the
+  server accepted now reports `queued` as usual, with `state_file: null` to say
+  no record was kept; follow the job by its `prompt_id` instead, and run with
+  `LOG_LEVEL=DEBUG` to see what the write failed on. Real transport failures
+  are reported exactly as before.
 - `comfy deploy run --wait` no longer loses a finished job's outputs to a 401
   when the sign-in token it started with expires mid-run. The token lasts
   fifteen minutes, and the run sent the one it started with for every request

comfy_cli/command/run/__init__.py

@@ -8,6 +8,7 @@
 """
 
 import json
+import logging
 import sys
 import time
 import uuid
@@ -55,6 +56,8 @@
 
 workspace_manager = WorkspaceManager()
 
+logger = logging.getLogger(__name__)
+
 # Bounds on the `partner_nodes` telemetry property and on the partner-node names
 # echoed in the missing-credential error. class_type strings are attacker- (or
 # just accident-) controlled workflow JSON, so cap the list length and each name.
@@ -575,7 +578,14 @@ def execute(
                 host=host,
                 port=port,
             )
-            watcher_spawned = _spawn_watcher(execution.prompt_id, where="local", host=host, port=port, notify=notify)
+            # No record means nothing for a watcher to update: `_watch-job`
+            # opens with `jobs_state.read` and exits immediately (or dies on
+            # the same error). Spawning it anyway would also report
+            # `watcher_spawned: true`, which tells a consumer something is
+            # keeping the file fresh when nothing is.
+            watcher_spawned = state_file is not None and _spawn_watcher(
+                execution.prompt_id, where="local", host=host, port=port, notify=notify
+            )
             # Emitted after the state file and the watcher spawn attempt, so a
             # consumer reading this line can already `comfy jobs status` it.
             _emit_queued_event(renderer, execution)
@@ -759,7 +769,12 @@ def _write_state(state):
     """
     try:
         return jobs_state.write(state)
-    except (OSError, ValueError):
+    except (OSError, ValueError) as e:
+        # The caller gets a null `state_file` and no error, so this log line is
+        # the only trail a swallowed failure leaves — and the one that names
+        # the resolved path when the config root is unwritable. `LOG_LEVEL`
+        # (comfy_cli.logging) surfaces it.
+        logger.debug("job state write failed for %r, continuing: %s", getattr(state, "prompt_id", None), e)
         return None
 
 
@@ -875,7 +890,20 @@ def _init_queued_state(prompt_id, client_id, workflow_name, *, where, compose_me
     watcher pid + create_time before the first write.
 
     ``where_fields`` carries the target-specific locators (``host``/``port`` for
-    local, ``base_url`` for cloud). Returns the ``(state, state_file)`` pair."""
+    local, ``base_url`` for cloud). Returns the ``(state, state_file)`` pair,
+    whose second element is None when the write could not land.
+
+    The write goes through ``_write_state``, not ``jobs_state.write`` directly,
+    and that is load-bearing: both callers run *after* the server accepted the
+    prompt, and the local one runs inside the ``except (WebSocketException,
+    ConnectionError, OSError)`` block's try. A raw call let a purely local
+    filesystem error — a config root resolved outside the sandbox, a read-only
+    or missing directory — surface to the caller as ``ws_disconnected`` with a
+    "re-run the command" hint, which an agent caller obeys, submitting another
+    GPU job for a prompt that was already queued. On the cloud side, where
+    there is no handler at all, the same raw call turned an accepted billable
+    job into an uncaught traceback. A state-file failure costs the caller the
+    durable record (``state_file`` comes back null) and nothing else."""
     state = jobs_state.new(
         prompt_id=prompt_id,
         client_id=client_id,
@@ -884,7 +912,7 @@ def _init_queued_state(prompt_id, client_id, workflow_name, *, where, compose_me
         **where_fields,
     )
     state.item_map = (compose_meta or {}).get("items")
-    state_file = jobs_state.write(state)
+    state_file = _write_state(state)
     return state, state_file
 
 
@@ -915,11 +943,21 @@ def _announce_async_queued(
     if renderer.is_pretty():
         from comfy_cli.output.glyphs import status_glyph
 
+        # `state_file` is None when the record could not be written. Printing
+        # the bare value gives a human "state    None" with no explanation, on
+        # a run that otherwise succeeded.
+        state_line = state_file if state_file else "[dim]not written — track by prompt_id[/dim]"
         pprint(
-            f"{status_glyph('queued')} [dim]{prompt_id}[/dim]\n"
+            # `prompt_id` is server-chosen, and it appears TWICE here: on its
+            # own line, and again inside `watch_command`, which both callers
+            # build as `comfy jobs watch <prompt_id>`. A value carrying Rich
+            # markup raises MarkupError at either sink — aborting pretty output
+            # for a job that was already accepted — or injects into the
+            # terminal, so both are escaped.
+            f"{status_glyph('queued')} [dim]{sanitize_markup(prompt_id)}[/dim]\n"
             f"  [dim]workflow [/dim]{workflow_name}\n"
-            f"  [dim]watch    [/dim][cyan]{watch_command}[/cyan]\n"
-            f"  [dim]state    [/dim]{state_file}"
+            f"  [dim]watch    [/dim][cyan]{sanitize_markup(watch_command)}[/cyan]\n"
+            f"  [dim]state    [/dim]{state_line}"
         )
         if not watcher_spawned:
             pprint("[yellow]⚠ Background watcher could not start; poll manually with `comfy jobs status`[/yellow]")
@@ -1244,7 +1282,11 @@ def _emit_queued_cloud() -> None:
             base_url=target.base_url,
         )
         _journal_run(workflow_name, submit.prompt_id, "cloud")
-        watcher_spawned = _spawn_watcher(submit.prompt_id, where="cloud", notify=notify)
+        # Same rule as the local async branch, and it matters more here: the
+        # watcher inherits COMFY_CLOUD_AUTH_TOKEN / COMFY_CLOUD_API_KEY and
+        # outlives us, so launching one that can read no record is a
+        # credential-holding orphan for nothing.
+        watcher_spawned = state_file is not None and _spawn_watcher(submit.prompt_id, where="cloud", notify=notify)
         # Durable record + watcher exist: safe to announce.
         _emit_queued_cloud()
         _announce_async_queued(

comfy_cli/command/run/watcher.py

@@ -41,8 +41,18 @@ def _tail_state_file(prompt_id: str, *, seconds: float = 8.0) -> None:
     writing after we return — we just give the human a few seconds of
     "alive" feedback before the foreground exits.
 
-    Always safe to call; no-ops if the renderer isn't pretty or the user
-    Ctrl-Cs out.
+    Always safe to call; no-ops if the renderer isn't pretty, the state file
+    can't be read at all, or the user Ctrl-Cs out.
+
+    That "can't be read at all" case is load-bearing, not defensive padding.
+    This runs from ``_announce_async_queued``, i.e. inside ``execute``'s
+    ``except (WebSocketException, ConnectionError, OSError)`` block's try and
+    *after* the server accepted the prompt. An ``OSError`` escaping here —
+    ``state_dir()``'s mkdir under a config root that resolved outside the
+    sandbox, a read-only directory — would therefore be reported to the caller
+    as ``ws_disconnected`` with a "re-run the command" hint, for a job that is
+    already queued. See the same guarantee on ``_write_state`` in
+    ``comfy_cli/command/run/__init__.py``.
     """
     import time as _t
 
@@ -56,16 +66,41 @@ def _tail_state_file(prompt_id: str, *, seconds: float = 8.0) -> None:
     from comfy_cli import jobs_state
     from comfy_cli.output.glyphs import status_glyph
 
+    # Truncate first, then escape: slicing can split a markup sequence, and
+    # `prompt_id` is server-chosen. Unsafe ids used to stop at the submit-time
+    # `jobs_state.write`; now that `_write_state` swallows that rejection they
+    # reach this render, where raw markup raises MarkupError (or injects into
+    # the terminal) on a job the server already accepted.
+    short_id = sanitize_markup(prompt_id[:8])
+
     def render(status: str, elapsed: float) -> Text:
-        return Text.from_markup(f"  {status_glyph(status)}  [dim]· {elapsed:.1f}s · {prompt_id[:8]}…[/dim]")
+        return Text.from_markup(f"  {status_glyph(status)}  [dim]· {elapsed:.1f}s · {short_id}…[/dim]")
+
+    def read_state():
+        """``(state, readable)``. ``readable`` is False only when the record
+        could not be consulted at all, which is unrecoverable for the whole
+        tail — distinct from ``state is None``, the ordinary "nothing written
+        yet" the poll loop is built to wait through.
+
+        ``ValueError`` is caught alongside ``OSError`` to match
+        ``_write_state``: ``state_path()`` raises it for a server-supplied
+        prompt_id that fails ``_SAFE_PROMPT_ID``, and since ``_write_state``
+        now swallows that at submit, this read is where it would otherwise
+        surface — as a traceback, uncovered by either caller's handler."""
+        try:
+            return jobs_state.read(prompt_id), True
+        except (OSError, ValueError):
+            return None, False
 
     deadline = _t.time() + seconds
     last_status = "queued"
     start = _t.time()
     try:
         with Live(render(last_status, 0.0), console=renderer.console(), refresh_per_second=4, transient=True) as live:
             while _t.time() < deadline:
-                state = jobs_state.read(prompt_id)
+                state, readable = read_state()
+                if not readable:
+                    return
                 if state is not None:
                     last_status = state.status
                     live.update(render(last_status, _t.time() - start))
@@ -75,7 +110,7 @@ def render(status: str, elapsed: float) -> Text:
     except KeyboardInterrupt:
         pass
 
-    final_state = jobs_state.read(prompt_id)
+    final_state, _readable = read_state()
     if final_state is None:
         return
     elapsed = _t.time() - start

docs/json-output.md

@@ -504,7 +504,7 @@ On `--wait` success, `data` carries:
 | `elapsed_seconds`   | float \| null   | Wall-clock duration (null when not waiting)            |
 | `host` / `port`     | str / int       | **Local only.** Target server                          |
 | `base_url`          | str             | **Cloud only.** Target cloud endpoint                  |
-| `state_file`        | str \| null     | Path of the job state file (poll with `comfy jobs status`) |
+| `state_file`        | str \| null     | Path of the job state file (poll with `comfy jobs status`); null when no record could be written — see [below](#known-limit-a-state-file-failure-is-never-a-run-failure) |
 
 `cached_node_ids` and `executed_node_ids` may overlap: a cached
 output-bearing node emits both `execution_cached` and `executed`. Agents
@@ -521,6 +521,39 @@ the same on both targets, `watcher_spawned` included (add `--where cloud` to
 the follow-up commands for a cloud job). The async envelope carries no
 `outputs_by_node` / `outputs_by_item` / `warnings` — nothing has run yet.
 
+### Known limit: a state-file failure is never a run failure
+
+Writing the job's state file is best-effort on an **async submit** (the default,
+no `--wait`, local or cloud) and on **local `--wait`**. If the state directory
+can't be written — a config root that resolved somewhere unreachable, a
+read-only or sandboxed home, a full disk — the run still reports `ok: true`,
+because by then the server has already accepted the prompt. What the envelope
+says depends on the path, and `data.state_file` is `null` on both:
+
+- **async submit** — `data.status: "queued"`, exactly as it would be with the
+  record written.
+- **local `--wait`** — the run goes on to completion in the foreground and
+  finishes `data.status: "completed"` with its outputs, so a failed state write
+  costs you only the on-disk record, not the result.
+
+**`state_file: null` is not a failed run**, and in particular it is not a
+transport error: there is nothing to retry, and re-submitting queues a second
+job for a prompt that is already running. Follow an async job by `prompt_id`
+(`comfy jobs status <prompt_id>`) and treat a missing record as "unknown", not
+"did not run".
+
+With no record there is also nothing for a background watcher to update, so the
+spawn is skipped and `data.watcher_spawned` is `false` — poll
+`comfy jobs status` yourself, as that field already instructs.
+
+Run with `LOG_LEVEL=DEBUG` to see the swallowed cause.
+
+**Cloud `--wait` is not yet covered.** That path still writes its state record
+directly, including inside its own error handlers, so an unwritable state
+directory there can surface as a traceback after the job was accepted rather
+than as a clean envelope. Treat the guarantee above as scoped to the paths
+named in the first paragraph.
+
 ### Known limit: `--wait` has no background watcher
 
 `--wait` writes the job's state file at submit and finalizes it in the