Skip to content

Commit 046528f

Browse files
Fix stale ComfyUI
Signed-off-by: Leslie Lazzarino <leslie.lazzarino@tngtech.com>
1 parent 9a63df2 commit 046528f

3 files changed

Lines changed: 182 additions & 42 deletions

File tree

WebUI/electron/subprocesses/comfyUIBackendService.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import {
2828
type ComfyUiDepsMarker,
2929
} from './comfyUiRevision.ts'
3030
import { ProcessError } from './osProcessHelper.ts'
31+
import { killStaleProcessesByCommandLine } from './processLifecycle.ts'
3132
import { getMediaDir } from '../util.ts'
3233
import {
3334
clearLevelZeroRuntimeCache,
@@ -1646,6 +1647,17 @@ except Exception as e:
16461647
process: ChildProcess
16471648
didProcessExitEarlyTracker: Promise<boolean>
16481649
}> {
1650+
// Kill any ComfyUI left over from a previous session (e.g. after a hard
1651+
// crash / force-quit that skipped our clean shutdown). We match on this
1652+
// backend's python binary path, which is unique to ComfyUI's env dir. The
1653+
// port is picked fresh each launch, so an orphan sits on a *different* port
1654+
// and would otherwise run beside the new instance → GPU out-of-memory.
1655+
await killStaleProcessesByCommandLine(this.getPythonBinaryPath(), {
1656+
name: this.name,
1657+
label: 'ComfyUI',
1658+
appLogger: this.appLogger,
1659+
})
1660+
16491661
// Clear any stale SQLite WAL/SHM sidecars from a crashed run that would
16501662
// otherwise block ComfyUI from opening its database.
16511663
this.cleanupStaleComfyUiDbLocks()
Lines changed: 163 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,163 @@
1+
import { ChildProcess } from 'node:child_process'
2+
import * as childProcess from 'node:child_process'
3+
import { promisify } from 'node:util'
4+
import { appLoggerInstance } from '../logging/logger.ts'
5+
6+
const execAsync = promisify(childProcess.exec)
7+
8+
type AppLogger = Pick<typeof appLoggerInstance, 'info' | 'warn' | 'error'>
9+
10+
export interface TerminateProcessTreeOptions {
11+
/** Backend name, used as the logging tag. */
12+
name: string
13+
/** Optional extra label (e.g. 'LLM', 'ComfyUI') appended to the tag. */
14+
label?: string
15+
/** Grace period for a cooperative SIGTERM shutdown (POSIX only). Default 2000ms. */
16+
gracefulMs?: number
17+
/** How long to wait for the OS to reap the process after the force kill. Default 5000ms. */
18+
forceMs?: number
19+
appLogger?: AppLogger
20+
}
21+
22+
/**
23+
* Reliably tear down a spawned backend process AND its descendants.
24+
*
25+
* The important fix over a plain `proc.kill()` is Windows: Node has no real
26+
* SIGTERM, and `ChildProcess.kill()` only signals the direct child, leaving the
27+
* descendant tree (ComfyUI's python workers, the uv subprocesses spawned by
28+
* ComfyUI-Manager, …) orphaned. An orphaned ComfyUI keeps holding its port and
29+
* GPU memory, so the next app launch — which picks a fresh free port — starts a
30+
* SECOND instance beside it and runs the GPU out of memory. We therefore go
31+
* straight to `taskkill /T /F` while the pid is still valid so the whole tree is
32+
* reaped in one shot (this also avoids the pid-reuse risk of killing after a
33+
* reported graceful exit).
34+
*/
35+
export async function terminateProcessTree(
36+
proc: ChildProcess,
37+
opts: TerminateProcessTreeOptions,
38+
): Promise<void> {
39+
const { name, label, gracefulMs = 2000, forceMs = 5000 } = opts
40+
const appLogger = opts.appLogger ?? appLoggerInstance
41+
const tag = label ? `${name} ${label}` : name
42+
43+
// Already gone.
44+
if (proc.exitCode !== null || proc.signalCode !== null) return
45+
46+
const waitForExit = (ms: number): Promise<boolean> =>
47+
new Promise<boolean>((resolve) => {
48+
if (proc.exitCode !== null || proc.signalCode !== null) {
49+
resolve(true)
50+
return
51+
}
52+
const timeout = setTimeout(() => resolve(false), ms)
53+
proc.once('exit', () => {
54+
clearTimeout(timeout)
55+
resolve(true)
56+
})
57+
})
58+
59+
if (process.platform === 'win32' && proc.pid !== undefined) {
60+
// No graceful SIGTERM dance on Windows: signalling the parent leaves the
61+
// tree running, and if the parent happens to exit we'd never reach the tree
62+
// kill. Reap the whole tree while the pid is valid.
63+
try {
64+
await execAsync(`taskkill /PID ${proc.pid} /T /F`)
65+
} catch (e) {
66+
// taskkill exits non-zero when the process is already gone — not fatal.
67+
appLogger.warn(`taskkill for ${tag} reported: ${e}`, name)
68+
}
69+
if (!(await waitForExit(forceMs))) {
70+
appLogger.warn(`${tag} not confirmed exited after taskkill`, name)
71+
}
72+
return
73+
}
74+
75+
// POSIX: try a cooperative shutdown first, then SIGKILL the child.
76+
proc.kill('SIGTERM')
77+
if (await waitForExit(gracefulMs)) return
78+
79+
appLogger.warn(`${tag} did not exit within ${gracefulMs}ms, force killing`, name)
80+
proc.kill('SIGKILL')
81+
if (!(await waitForExit(forceMs))) {
82+
appLogger.warn(`${tag} not confirmed exited after SIGKILL`, name)
83+
}
84+
}
85+
86+
export interface KillStaleProcessesOptions {
87+
name: string
88+
label?: string
89+
appLogger?: AppLogger
90+
}
91+
92+
/**
93+
* Startup singleton guard: kill any process left over from a previous app
94+
* session whose command line contains `signature` (typically the backend's
95+
* python binary path, which is unique to that backend's environment directory).
96+
*
97+
* A clean shutdown reaps everything via terminateProcessTree(), but a hard crash
98+
* or force-quit of Electron can still leave a backend running. Calling this
99+
* BEFORE spawning guarantees a new launch never coexists with a stale instance
100+
* that would hold a port + GPU memory and cause an out-of-memory.
101+
*/
102+
export async function killStaleProcessesByCommandLine(
103+
signature: string,
104+
opts: KillStaleProcessesOptions,
105+
): Promise<void> {
106+
const { name, label } = opts
107+
const appLogger = opts.appLogger ?? appLoggerInstance
108+
const tag = label ? `${name} ${label}` : name
109+
110+
try {
111+
const pids = await findPidsByCommandLine(signature)
112+
if (pids.length === 0) return
113+
appLogger.warn(
114+
`Found ${pids.length} stale ${tag} process(es) (${pids.join(', ')}); terminating before start`,
115+
name,
116+
)
117+
for (const pid of pids) {
118+
try {
119+
if (process.platform === 'win32') {
120+
await execAsync(`taskkill /PID ${pid} /T /F`)
121+
} else {
122+
process.kill(pid, 'SIGKILL')
123+
}
124+
} catch (e) {
125+
appLogger.warn(`Failed to kill stale ${tag} pid ${pid}: ${e}`, name)
126+
}
127+
}
128+
} catch (e) {
129+
// Best-effort guard — never block startup on it.
130+
appLogger.warn(`Stale-${tag}-process scan failed: ${e}`, name)
131+
}
132+
}
133+
134+
async function findPidsByCommandLine(signature: string): Promise<number[]> {
135+
if (process.platform === 'win32') {
136+
// Escape single quotes for the PowerShell string literal.
137+
const escaped = signature.replace(/'/g, "''")
138+
const { stdout } = await execAsync(
139+
`powershell -NoProfile -Command "Get-CimInstance Win32_Process | Where-Object { $_.CommandLine -like '*${escaped}*' } | Select-Object -ExpandProperty ProcessId"`,
140+
)
141+
return parsePids(stdout).filter((pid) => pid !== process.pid)
142+
}
143+
// POSIX: pgrep -f matches against the full command line. Fixed-string match.
144+
try {
145+
const { stdout } = await execAsync(`pgrep -f -- ${shellQuote(signature)}`)
146+
return parsePids(stdout).filter((pid) => pid !== process.pid)
147+
} catch (e) {
148+
// pgrep exits 1 when nothing matches — that's not an error for us.
149+
if ((e as { code?: number }).code === 1) return []
150+
throw e
151+
}
152+
}
153+
154+
function parsePids(stdout: string): number[] {
155+
return stdout
156+
.split(/\r?\n/)
157+
.map((line) => Number.parseInt(line.trim(), 10))
158+
.filter((pid) => Number.isInteger(pid) && pid > 0)
159+
}
160+
161+
function shellQuote(value: string): string {
162+
return `'${value.replace(/'/g, `'\\''`)}'`
163+
}

WebUI/electron/subprocesses/service.ts

Lines changed: 7 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import { promisify } from 'util'
1414
import { Arch, getArchPriority, getDeviceArch } from './deviceArch.ts'
1515
import { z } from 'zod'
1616
import { LocalSettings } from '../main.ts'
17+
import { terminateProcessTree } from './processLifecycle.ts'
1718

1819
const exec = promisify(childProcess.exec)
1920

@@ -718,48 +719,12 @@ export abstract class LongLivedPythonApiService implements ApiService {
718719
return 'stopped'
719720
}
720721

721-
const waitForExit = (ms: number): Promise<boolean> =>
722-
Promise.race([
723-
new Promise<boolean>((resolve) => proc.once('exit', () => resolve(true))),
724-
new Promise<boolean>((resolve) => setTimeout(() => resolve(false), ms)),
725-
])
726-
727-
// Try graceful shutdown first with SIGTERM
728-
proc.kill('SIGTERM')
729-
730-
// Wait up to 2 seconds for the process to exit gracefully
731-
let exited = await waitForExit(2000)
732-
733-
if (!exited) {
734-
this.appLogger.warn(
735-
`Backend ${this.name} did not exit gracefully within 2s, force killing`,
736-
this.name,
737-
)
738-
// On Windows, ChildProcess.kill() only signals the direct child and leaves
739-
// descendant processes (e.g. ComfyUI's python and the uv subprocesses spawned
740-
// by ComfyUI-Manager) running. Those descendants keep handles on the service
741-
// directory — and python's CWD is the service dir itself — which makes a
742-
// subsequent reinstall fail to delete it (EPERM). taskkill /T /F tears down
743-
// the whole tree.
744-
if (process.platform === 'win32' && proc.pid !== undefined) {
745-
try {
746-
await exec(`taskkill /PID ${proc.pid} /T /F`)
747-
} catch (e) {
748-
// taskkill exits non-zero when the process is already gone — not fatal.
749-
this.appLogger.warn(`taskkill for backend ${this.name} reported: ${e}`, this.name)
750-
}
751-
} else {
752-
proc.kill('SIGKILL')
753-
}
754-
// Wait for the OS to actually reap the process and release file handles.
755-
exited = await waitForExit(5000)
756-
if (!exited) {
757-
this.appLogger.warn(
758-
`Backend ${this.name} still not confirmed exited after force kill`,
759-
this.name,
760-
)
761-
}
762-
}
722+
// Reliably tear down the whole process tree. On Windows this is critical:
723+
// ChildProcess.kill() only signals the direct child, leaving descendants
724+
// (ComfyUI's python, the uv subprocesses spawned by ComfyUI-Manager, …)
725+
// running. Orphans keep the port + GPU memory (→ OOM on the next launch)
726+
// and handles on the service directory (→ EPERM on reinstall).
727+
await terminateProcessTree(proc, { name: this.name, appLogger: this.appLogger })
763728

764729
this.encapsulatedProcess = null
765730
this.setStatus('stopped')

0 commit comments

Comments
 (0)