diff --git a/open-sse/services/adobeFireflyBrowserLogin.ts b/open-sse/services/adobeFireflyBrowserLogin.ts index a3ab0443b1..e69884689d 100644 --- a/open-sse/services/adobeFireflyBrowserLogin.ts +++ b/open-sse/services/adobeFireflyBrowserLogin.ts @@ -1008,27 +1008,63 @@ async function captureViaCdp(opts: { } } -function killProcessTree(child: ChildProcess | null): void { +/** + * Terminate a spawned browser process and all of its descendants. + * + * Windows uses `taskkill /pid /T /F` to walk the process tree and terminate descendants. + * Linux/POSIX sends SIGTERM/SIGKILL to the process group (`-pid`) when detached/group leader, + * falling back to direct child kill if the process group is unavailable. + */ +export function killProcessTree( + child: + | ChildProcess + | { pid?: number; kill?: (signal?: NodeJS.Signals | number | string) => boolean | void } + | null + | undefined, + options?: { + platform?: string; + processKill?: (pid: number, signal?: NodeJS.Signals | string) => void; + spawnFn?: typeof spawn; + } +): void { if (!child?.pid) return; const pid = child.pid; // Never taskkill our own Node/pkg process or its parent (would kill the backend mid-login). if (pid === process.pid || (typeof process.ppid === "number" && pid === process.ppid)) { return; } + const platform = options?.platform || process.platform; + const processKill = options?.processKill || process.kill.bind(process); + const spawnFn = options?.spawnFn || spawn; + try { - if (process.platform === "win32") { + if (platform === "win32") { // /T kills only this PID's descendants — not system Chrome profiles we did not spawn. - const killer = spawn("taskkill", ["/pid", String(pid), "/T", "/F"], { + const killer = spawnFn("taskkill", ["/pid", String(pid), "/T", "/F"], { stdio: "ignore", windowsHide: true, detached: true, }); - killer.unref?.(); + killer?.unref?.(); } else { - child.kill("SIGTERM"); + let killedGroup = false; + try { + processKill(-pid, "SIGTERM"); + killedGroup = true; + } catch { + try { + child.kill?.("SIGTERM"); + } catch { + /* ignore */ + } + } setTimeout(() => { try { - child.kill("SIGKILL"); + if (killedGroup) { + processKill(-pid, "SIGKILL"); + } else { + child.kill?.("SIGKILL"); + } } catch { /* ignore */ } @@ -1036,7 +1072,7 @@ function killProcessTree(child: ChildProcess | null): void { } } catch { try { - child.kill(); + child.kill?.(); } catch { /* ignore */ } @@ -1175,12 +1211,15 @@ async function runAdobeFireflyCdpBrowser(opts: { // detach so a long Forter wait does not pin the Node process refcount. // Host job SILENT_BREAKAWAY_OK still prevents Chrome from joining the backend job // (that was killing/wedging VibeProxyServices on Sign in with browser). + // On POSIX: detached creates a new process group leader so killProcessTree(-pid) + // can terminate Chrome and all its child processes (zygote/renderer/GPU). + const isDetached = process.platform !== "win32" || !opts.interactive; child = spawn(browserPath, args, { stdio: "ignore", // Interactive sign-in: show Chrome. Background warm: hide spawn console/window // host; headless flags already suppress the browser UI. windowsHide: !opts.interactive, - detached: !opts.interactive, + detached: isDetached, }); if (!opts.interactive) { try { diff --git a/tests/unit/adobe-firefly-browser-login.test.ts b/tests/unit/adobe-firefly-browser-login.test.ts index d22cfb4e89..012bd2e344 100644 --- a/tests/unit/adobe-firefly-browser-login.test.ts +++ b/tests/unit/adobe-firefly-browser-login.test.ts @@ -19,6 +19,7 @@ import { isAdobeRiskCookieName, resolveAdobeAccountLabel, resolveSystemBrowserExecutable, + killProcessTree, } from "../../open-sse/services/adobeFireflyBrowserLogin.ts"; test("clampAdobeFireflyLoginTimeout defaults and clamps", () => { @@ -237,3 +238,114 @@ test("error path does not mention Playwright (packaged backend has no Playwright else process.env.OMNIROUTE_LOGIN_BROWSER_PATH = prev; } }); + +test("killProcessTree on Linux targets process group (-pid) with SIGTERM and schedules SIGKILL", () => { + const killedSignals: Array<{ pid: number; signal: NodeJS.Signals | string }> = []; + const mockProcessKill = (pid: number, signal?: NodeJS.Signals | string) => { + if (signal) killedSignals.push({ pid, signal }); + }; + let procKillCalled = false; + const fakeChild = { + pid: 54321, + kill: (_sig?: NodeJS.Signals | number | string) => { + procKillCalled = true; + return true; + }, + }; + + killProcessTree(fakeChild, { + platform: "linux", + processKill: mockProcessKill, + }); + + assert.equal(killedSignals.length, 1, "expected immediate SIGTERM call to process group"); + assert.equal(killedSignals[0].pid, -54321, "Linux must target process group with negative PID"); + assert.equal(killedSignals[0].signal, "SIGTERM"); + assert.equal(procKillCalled, false, "should not call direct child.kill when process group kill succeeds"); +}); + +test("killProcessTree falls back to child.kill on Linux when process group kill fails", () => { + let childKilledWith: string | undefined; + const fakeChild = { + pid: 54322, + kill: (sig?: NodeJS.Signals | number | string) => { + childKilledWith = typeof sig === "string" ? sig : undefined; + return true; + }, + }; + const mockProcessKill = () => { + throw new Error("ESRCH: no such process group"); + }; + + killProcessTree(fakeChild, { + platform: "linux", + processKill: mockProcessKill, + }); + + assert.equal(childKilledWith, "SIGTERM", "must fall back to direct child.kill('SIGTERM')"); +}); + +test("killProcessTree ignores self PID and parent PID to prevent killing backend", () => { + let killCalled = false; + const selfChild = { + pid: process.pid, + kill: () => { + killCalled = true; + return true; + }, + }; + killProcessTree(selfChild, { platform: "linux" }); + assert.equal(killCalled, false, "must never kill own process.pid"); + + if (process.ppid) { + const parentChild = { + pid: process.ppid, + kill: () => { + killCalled = true; + return true; + }, + }; + killProcessTree(parentChild, { platform: "linux" }); + assert.equal(killCalled, false, "must never kill process.ppid"); + } +}); + +test("killProcessTree on win32 uses taskkill /pid /T /F with detached and windowsHide", () => { + const spawnCalls: Array<{ cmd: string; args: readonly string[]; opts: unknown }> = []; + let unrefCalled = false; + const mockSpawn = ((cmd: string, args: readonly string[], opts: unknown) => { + spawnCalls.push({ cmd, args, opts }); + return { + unref: () => { + unrefCalled = true; + }, + }; + }) as unknown as typeof import("node:child_process").spawn; + + const fakeChild = { + pid: 7788, + kill: () => true, + }; + + killProcessTree(fakeChild, { + platform: "win32", + spawnFn: mockSpawn, + }); + + assert.equal(spawnCalls.length, 1); + assert.equal(spawnCalls[0].cmd, "taskkill"); + assert.deepEqual(spawnCalls[0].args, ["/pid", "7788", "/T", "/F"]); + const opts = spawnCalls[0].opts as { windowsHide?: boolean; detached?: boolean }; + assert.equal(opts.windowsHide, true); + assert.equal(opts.detached, true); + assert.equal(unrefCalled, true); +}); + +test("killProcessTree handles null / undefined / pid-less gracefully without throwing", () => { + assert.doesNotThrow(() => killProcessTree(null)); + assert.doesNotThrow(() => killProcessTree(undefined)); + assert.doesNotThrow(() => killProcessTree({})); + assert.doesNotThrow(() => killProcessTree({ pid: undefined })); +}); + +