fix(comfyui): make the task instance own its runtime with a job object #894

Merged

coilysiren merged 1 commit from fix/comfyui-supervisor-job-object into main

2026-08-20 07:06:38 +00:00

coilyco-ops commented

2026-08-20 07:03:27 +00:00

Owner

Closes the third #888 option from #883: drive the runtime entirely through the task surface, so session and elevation stop mattering.

Why not the other two options

Return the task to Interactive. That re-introduces exactly what #888 fixed. A session disconnect kills the task with 0xC000013A, and GpuHeartbeat had been up 5.5 days before a disconnect took it down. It trades a supervision problem for a liveness problem on a host whose job is unattended uptime.

Give the converger an elevated path. The converger runs unelevated today, which is what lets ansible and an agent run it with nobody at the keyboard. Elevation puts a UAC prompt in the middle of unattended convergence, and it elevates the whole script (downloads, 7z extraction, writes into $HOME) to buy one Stop-Process call.

The actual defect

The converger reaches past the task surface to Stop-Process because Stop-ScheduledTask does not free the port. That is a process-tree problem, not a permissions problem. Reaching for the process is what put the converger on the wrong side of the S4U session boundary.

cmd.exe runs python synchronously, so the instance stays attached and Running. But stopping the instance kills only cmd.exe. The python child survives holding 127.0.0.1:8188, no task owns it, and RestartCount=5 has no instance to act on.

The change

The task action is now a generated supervisor. It puts itself in a job object limited to KILL_ON_JOB_CLOSE and launches serve.cmd as a child. Job membership is inherited, so cmd.exe and python join it. When the instance stops, the supervisor dies, its last job handle closes, and the tree goes with it.

serve.cmd is unchanged. The wrapper exists for the >> comfyui.log 2>&1 redirect, and ComfyUI 0.29.0 has no --log-file (only --verbose and --log-stdout in comfy/cli_args.py), so dropping it would cost the log this whole investigation ran on.

Stop-ScheduledTask is then sufficient. It is unprivileged and session-agnostic, so the converger manages an S4U runtime without ever signalling it. The restart path no longer calls Stop-Process.

Migration safety

Replacing a pre-supervisor task still stops an instance that does not own its runtime, and Unregister-ScheduledTask does that before the restart path is reached. So a pre-flight now refuses the whole converge when a legacy task is being replaced and the port is held and cannot be freed. That preserves what #892 bought: never detach a runtime without a proven way back.

On kai-tower-3026 this pre-flight will fire, because the runtime there is already an orphan from before either fix. That is correct. The tower needs the elevated cleanup first either way.

Verification

Done on kai-tower-3026 without touching the live runtime:

Extracted the supervisor-generation block from this script, ran it against a dummy launcher, and parsed the generated file clean.
Ran that generated supervisor, then stopped it the way Stop-ScheduledTask does. cmd.exe, conhost.exe, and the grandchild were all reaped, no orphans.
Ran the same test against the current cmd.exe shape. The child survived, reproducing the orphan.

The script itself parses clean (3387 tokens). PSScriptAnalyzer is not installed on this host.

Not exercised end to end against the live ComfyUI. Running the converger against the live runtime is the destructive act at the centre of #883, and the tower's runtime is currently an orphan that this branch's pre-flight will correctly refuse to touch. The first real run wants an operator present.

Closes the third #888 option from https://forgejo.coilysiren.me/coilyco-flight-deck/infrastructure/issues/883: drive the runtime entirely through the task surface, so session and elevation stop mattering. ## Why not the other two options **Return the task to Interactive.** That re-introduces exactly what #888 fixed. A session disconnect kills the task with `0xC000013A`, and `GpuHeartbeat` had been up 5.5 days before a disconnect took it down. It trades a supervision problem for a liveness problem on a host whose job is unattended uptime. **Give the converger an elevated path.** The converger runs unelevated today, which is what lets ansible and an agent run it with nobody at the keyboard. Elevation puts a UAC prompt in the middle of unattended convergence, and it elevates the whole script (downloads, 7z extraction, writes into `$HOME`) to buy one `Stop-Process` call. ## The actual defect The converger reaches past the task surface to `Stop-Process` because `Stop-ScheduledTask` does not free the port. That is a process-tree problem, not a permissions problem. Reaching for the process is what put the converger on the wrong side of the S4U session boundary. `cmd.exe` runs python synchronously, so the instance stays attached and `Running`. But stopping the instance kills only `cmd.exe`. The python child survives holding `127.0.0.1:8188`, no task owns it, and `RestartCount=5` has no instance to act on. ## The change The task action is now a generated supervisor. It puts itself in a job object limited to `KILL_ON_JOB_CLOSE` and launches `serve.cmd` as a child. Job membership is inherited, so `cmd.exe` and python join it. When the instance stops, the supervisor dies, its last job handle closes, and the tree goes with it. `serve.cmd` is unchanged. The wrapper exists for the `>> comfyui.log 2>&1` redirect, and ComfyUI 0.29.0 has no `--log-file` (only `--verbose` and `--log-stdout` in `comfy/cli_args.py`), so dropping it would cost the log this whole investigation ran on. `Stop-ScheduledTask` is then sufficient. It is unprivileged and session-agnostic, so the converger manages an S4U runtime without ever signalling it. The restart path no longer calls `Stop-Process`. ## Migration safety Replacing a pre-supervisor task still stops an instance that does not own its runtime, and `Unregister-ScheduledTask` does that before the restart path is reached. So a pre-flight now refuses the whole converge when a legacy task is being replaced and the port is held and cannot be freed. That preserves what #892 bought: never detach a runtime without a proven way back. On kai-tower-3026 this pre-flight will fire, because the runtime there is already an orphan from before either fix. That is correct. The tower needs the elevated cleanup first either way. ## Verification Done on kai-tower-3026 without touching the live runtime: * Extracted the supervisor-generation block from this script, ran it against a dummy launcher, and parsed the generated file clean. * Ran that generated supervisor, then stopped it the way `Stop-ScheduledTask` does. `cmd.exe`, `conhost.exe`, and the grandchild were all reaped, no orphans. * Ran the same test against the current `cmd.exe` shape. The child survived, reproducing the orphan. The script itself parses clean (3387 tokens). PSScriptAnalyzer is not installed on this host. **Not exercised end to end against the live ComfyUI.** Running the converger against the live runtime is the destructive act at the centre of #883, and the tower's runtime is currently an orphan that this branch's pre-flight will correctly refuse to touch. The first real run wants an operator present.

coilyco-ops added 1 commit

2026-08-20 07:03:27 +00:00

fix(comfyui): make the task instance own its runtime with a job object

TruffleHog / Scan for secrets (pull_request) Successful in 5s

Details

CI / lint (pull_request) Successful in 41s

Details

e126a1e525

The converger reached past the task surface to Stop-Process because
Stop-ScheduledTask did not free the port. That is a process-tree problem,
not a permissions problem, and reaching for the process is what put the
converger on the wrong side of the S4U session boundary in #883.

cmd.exe runs python synchronously, so the instance stays attached, but
stopping the instance kills only cmd.exe. Measured on kai-tower-3026:
kill the cmd.exe wrapper and its child survives. That orphan holds
127.0.0.1:8188, no task owns it, and RestartCount has no instance to act
on.

The task action is now a generated supervisor. It puts itself in a job
object limited to KILL_ON_JOB_CLOSE and launches serve.cmd as a child.
Job membership is inherited, so cmd.exe and python join it, and when the
instance stops the supervisor dies, its last job handle closes, and the
tree goes with it. serve.cmd is unchanged, because the redirect into
comfyui.log is why the wrapper exists.

Stop-ScheduledTask is then sufficient, which is unprivileged and
session-agnostic, so the converger manages an S4U runtime without needing
to signal it. The restart path no longer calls Stop-Process at all.

Replacing a pre-supervisor task still stops an instance that does not own
its runtime, so a pre-flight refuses the whole converge when the port is
held and cannot be freed. That keeps the #892 guarantee: never detach a
runtime without a proven way back.

Verified on kai-tower-3026 without touching the live runtime. Generated
the supervisor from this script against a dummy launcher, parsed it, ran
it, and confirmed stopping it reaps cmd.exe, conhost, and the grandchild.
The same test against the current cmd.exe shape leaves the child alive.

Refs: #883

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

coilyco-ops requested review from coilysiren

2026-08-20 07:03:27 +00:00

coilysiren merged commit 4c607e7a77 into main

2026-08-20 07:06:38 +00:00

coilysiren referenced this pull request from a commit

2026-08-20 07:06:40 +00:00

Merge pull request 'fix(comfyui): make the task instance own its runtime with a job object' (#894) from fix/comfyui-supervisor-job-object into main

coilysiren referenced this pull request from a commit

2026-08-20 07:15:19 +00:00

fix(comfyui): keep the pre-supervisor pre-flight out of -WhatIf's way

coilyco-ops referenced this pull request

2026-08-20 07:15:44 +00:00