fix(sse): kill entire process tree on Linux for adobe firefly sign-in to prevent orphan browser instances (#11387)

Merged via consolidated batch validation. Fixes orphaned browser processes on Linux for Adobe Firefly sign-in: spawns Chrome as a process-group leader (detached:true) and kills -pid instead of the single PID, with self-termination guards. Own test passes.
This commit is contained in:
Bob.Hou
2026-08-24 11:13:40 -04:00
committed by GitHub
parent 095f424658
commit 3c2906a80e
2 changed files with 159 additions and 8 deletions

View File

@@ -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 <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 {

View File

@@ -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 <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 }));
});