Filed by the groom sweep — Comfy-Org/comfy-cli (whole-repo) · verdict DOWNGRADE · run.
Verified -- the Windows gap is real
- command/run/watcher.py:72 _spawn_watcher promises in its docstring a new session so a controlling terminal closing does not kill it, but passes only start_new_session=True (plus DEVNULL stdio and close_fds=True). start_new_session is POSIX-only and CPython ignores it on Windows, so on Windows the watcher stays in the parent's console and process group and the documented liveness property does not hold. The repo does support and test Windows (.github/workflows/test-windows.yml).
- command/models/models.py:679 _spawn_download_worker gets it right: creationflags of DETACHED_PROCESS or CREATE_NEW_PROCESS_GROUP on win32, start_new_session=True otherwise, with getattr lookups because those constants do not exist on POSIX.
- download_state.py:487 kill_worker documents the contract only one spawner satisfies: POSIX workers are detached with start_new_session so a single killpg reaches the whole tree; Windows workers get CREATE_NEW_PROCESS_GROUP and fall back to killing the process and its children.
Verdict -- it is a bug fix, not an abstraction
Do: add the platform branch to _spawn_watcher -- on win32 pass creationflags built from getattr-looked-up DETACHED_PROCESS and CREATE_NEW_PROCESS_GROUP instead of start_new_session, exactly as models.py:679 does -- and state the Windows behavior in the docstring. Keep the existing except OSError returning False so a spawn failure still degrades to poll-manually.
DOWNGRADED AWAY: spawn_detached(argv, stdout, stderr, stdin) in utils. Two call sites, about 12 lines each, and they differ in more than the platform branch: the watcher sends all three streams to DEVNULL and returns a bool, while the download worker appends stdout and stderr to a per-download log file handle (which it must also close) and returns the pid. A helper is defensible but its entire content is the four-line platform branch; extract it when a third background worker appears, or extract just that branch as a detached_popen_kwargs() returning a kwargs dict if you want the constant lookups in one place.
Steelman
Two short Popen calls are not much duplication, and the Windows gap is a one-line fix rather than a new abstraction -- premature helper-building for two call sites.
Counter (accepted): correct, which is why the fix is the deliverable and the helper is optional.
Risk
Low. Hard to test cross-platform: assert the kwargs passed to a patched subprocess.Popen under a monkeypatched sys.platform for both branches, since real behavior is only observable on Windows CI. Do not regress the POSIX path -- kill_worker's killpg depends on start_new_session there.
Filed by the groom sweep — Comfy-Org/comfy-cli (
whole-repo) · verdict DOWNGRADE · run.Verified -- the Windows gap is real
Verdict -- it is a bug fix, not an abstraction
Do: add the platform branch to _spawn_watcher -- on win32 pass creationflags built from getattr-looked-up DETACHED_PROCESS and CREATE_NEW_PROCESS_GROUP instead of start_new_session, exactly as models.py:679 does -- and state the Windows behavior in the docstring. Keep the existing except OSError returning False so a spawn failure still degrades to poll-manually.
DOWNGRADED AWAY: spawn_detached(argv, stdout, stderr, stdin) in utils. Two call sites, about 12 lines each, and they differ in more than the platform branch: the watcher sends all three streams to DEVNULL and returns a bool, while the download worker appends stdout and stderr to a per-download log file handle (which it must also close) and returns the pid. A helper is defensible but its entire content is the four-line platform branch; extract it when a third background worker appears, or extract just that branch as a detached_popen_kwargs() returning a kwargs dict if you want the constant lookups in one place.
Steelman
Two short Popen calls are not much duplication, and the Windows gap is a one-line fix rather than a new abstraction -- premature helper-building for two call sites.
Counter (accepted): correct, which is why the fix is the deliverable and the helper is optional.
Risk
Low. Hard to test cross-platform: assert the kwargs passed to a patched subprocess.Popen under a monkeypatched sys.platform for both branches, since real behavior is only observable on Windows CI. Do not regress the POSIX path -- kill_worker's killpg depends on start_new_session there.