mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-15 09:25:28 +02:00
fix(browse): isProcessAlive uses signal-0 on every platform — no more tasklist probe (#1952)
Replace the Windows tasklist shell-out in isProcessAlive with process.kill(pid, 0), unifying all platforms on the POSIX idiom. Node maps signal-0 to an OpenProcess existence check on Windows — and the Windows daemon runs under Node (dist/server-node.mjs + bun-polyfill, the documented oven-sh/bun#4253 fallback) — so the probe is portable. Why the shell-out had to go, beyond the cosmetic conhost flash the watchdog blinked into the foreground every 60s (#1952): a Bun.spawnSync that hits its timeout still RETURNS with partial stdout, so the `.includes()` PID match answered "dead" for LIVE processes under load — the false-negative half of the #2414/#2295 leak chain. Signal 0 spawns nothing, cannot time out, and is ~5 orders of magnitude faster (measurements in #2414). EPERM still reports alive (process exists, we just can't signal it). Layered on the post-#2414-absorb shape: test 3 in process-liveness-windows.test.ts now asserts the probe is subprocess-free on ANY platform (win32 exemption dropped), test 4's static tripwire loses its error-handling.ts exemption (a `tasklist … PID eq` existence probe anywhere in src/ now fails CI), and windows-spawn-hide.test.ts drops its tasklist-in-error-handling needle (nothing spawns, which is stronger than hiding the window). Tests: process-liveness-windows + windows-spawn-hide + error-handling — 17 pass, 0 fail. Fixes #1952. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
b3a27173fe
commit
7171f10364
@@ -7,8 +7,6 @@
|
|||||||
|
|
||||||
import * as fs from 'fs';
|
import * as fs from 'fs';
|
||||||
|
|
||||||
const IS_WINDOWS = process.platform === 'win32';
|
|
||||||
|
|
||||||
// ─── Filesystem ────────────────────────────────────────────────
|
// ─── Filesystem ────────────────────────────────────────────────
|
||||||
|
|
||||||
/** Remove a file, ignoring ENOENT (already gone). Rethrows other errors. */
|
/** Remove a file, ignoring ENOENT (already gone). Rethrows other errors. */
|
||||||
@@ -39,24 +37,32 @@ export function safeKill(pid: number, signal: NodeJS.Signals | number): void {
|
|||||||
/**
|
/**
|
||||||
* Check if a PID is alive. Pure boolean probe — never throws.
|
* Check if a PID is alive. Pure boolean probe — never throws.
|
||||||
*
|
*
|
||||||
|
* Signal 0 on EVERY platform (#1952). Node maps `process.kill(pid, 0)` to an
|
||||||
|
* OpenProcess existence check on Windows — and on Windows the browse daemon
|
||||||
|
* runs under Node (dist/server-node.mjs + bun-polyfill, the documented
|
||||||
|
* fallback for oven-sh/bun#4253) — so the POSIX idiom is portable here.
|
||||||
|
*
|
||||||
|
* Windows used to shell out to `tasklist /FI "PID eq <pid>"` and
|
||||||
|
* string-match the CSV. That was wrong in two ways, both hit in production:
|
||||||
|
*
|
||||||
|
* 1. FALSE NEGATIVES UNDER LOAD (#2414/#2295): tasklist takes ~700-1700ms
|
||||||
|
* on an idle box and far longer under memory pressure. A Bun.spawnSync
|
||||||
|
* that hits its `timeout` still RETURNS, carrying partial stdout — so
|
||||||
|
* the `.includes()` match came back false and a LIVE process was
|
||||||
|
* reported dead. Callers that validate liveness before killing
|
||||||
|
* (killAgentByRecord, the terminal-agent watchdog) then skipped the
|
||||||
|
* kill and respawned around the survivor — one leaked terminal-agent
|
||||||
|
* per tick, self-reinforcing (each orphan slows the next tasklist).
|
||||||
|
* 2. A console window per probe (#1952): the watchdog blinked a conhost
|
||||||
|
* window into the foreground every 60s for the whole session.
|
||||||
|
*
|
||||||
|
* Signal 0 spawns nothing, cannot time out, and is orders of magnitude
|
||||||
|
* faster (~0.004ms vs ~270ms measured in #2414).
|
||||||
|
*
|
||||||
* EPERM means the process EXISTS but we lack rights to signal it. That is
|
* EPERM means the process EXISTS but we lack rights to signal it. That is
|
||||||
* alive — returning false there makes callers that validate liveness before
|
* alive — returning false there would reintroduce failure mode 1.
|
||||||
* killing (killAgentByRecord, the terminal-agent watchdog) skip the kill and
|
|
||||||
* respawn around a survivor, leaking one process per watchdog tick (#2414,
|
|
||||||
* #2295).
|
|
||||||
*/
|
*/
|
||||||
export function isProcessAlive(pid: number): boolean {
|
export function isProcessAlive(pid: number): boolean {
|
||||||
if (IS_WINDOWS) {
|
|
||||||
try {
|
|
||||||
const result = Bun.spawnSync(
|
|
||||||
['tasklist', '/FI', `PID eq ${pid}`, '/NH', '/FO', 'CSV'],
|
|
||||||
{ stdout: 'pipe', stderr: 'pipe', timeout: 3000, windowsHide: true }
|
|
||||||
);
|
|
||||||
return result.stdout.toString().includes(`"${pid}"`);
|
|
||||||
} catch {
|
|
||||||
return false;
|
|
||||||
}
|
|
||||||
}
|
|
||||||
try {
|
try {
|
||||||
process.kill(pid, 0);
|
process.kill(pid, 0);
|
||||||
return true;
|
return true;
|
||||||
|
|||||||
@@ -54,16 +54,15 @@ describe('process liveness probe (Windows terminal-agent leak)', () => {
|
|||||||
expect(isProcessAlive(2147483646)).toBe(false);
|
expect(isProcessAlive(2147483646)).toBe(false);
|
||||||
});
|
});
|
||||||
|
|
||||||
test('3. isProcessAlive spawns NO subprocess on POSIX (signal-0 path)', () => {
|
test('3. isProcessAlive spawns NO subprocess on ANY platform (signal-0, #1952)', () => {
|
||||||
// The heart of the bug: a liveness probe that forks is slow enough to
|
// The heart of the bug: a liveness probe that forks is slow enough to
|
||||||
// time out, and a timed-out probe silently answers "dead". Signal 0
|
// time out, and a timed-out probe silently answers "dead". Signal 0
|
||||||
// cannot time out because it never leaves the process.
|
// cannot time out because it never leaves the process. Node maps
|
||||||
//
|
// process.kill(pid, 0) to an OpenProcess existence check on Windows —
|
||||||
// Merged design note: on win32 the helper DOES keep a single hardened
|
// and the Windows daemon runs under Node (server-node.mjs +
|
||||||
// tasklist probe (windowsHide, bounded timeout, quoted-CSV PID match)
|
// bun-polyfill), so the POSIX idiom is portable and the win32 tasklist
|
||||||
// because Bun's process.kill(pid, 0) throws ESRCH for live Windows PIDs
|
// branch is GONE (it caused both the false negatives above and the
|
||||||
// in compiled binaries. The POSIX path stays subprocess-free.
|
// per-tick console flash of #1952).
|
||||||
if (process.platform === 'win32') return;
|
|
||||||
const origSpawn = (Bun as any).spawn;
|
const origSpawn = (Bun as any).spawn;
|
||||||
const origSpawnSync = (Bun as any).spawnSync;
|
const origSpawnSync = (Bun as any).spawnSync;
|
||||||
const spawns: string[] = [];
|
const spawns: string[] = [];
|
||||||
@@ -79,16 +78,14 @@ describe('process liveness probe (Windows terminal-agent leak)', () => {
|
|||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
test('4. no source file probes liveness via tasklist outside the central helper', () => {
|
test('4. no source file probes liveness via tasklist — signal-0 is the only probe (#1952)', () => {
|
||||||
// Static tripwire: ad-hoc tasklist existence checks scattered across src/
|
// Static tripwire: a tasklist existence check ANYWHERE in src/
|
||||||
// resurrect the false-negative class (each call site re-invents the
|
// resurrects both the false-negative class (#2414: a timed-out spawnSync
|
||||||
// timeout/parse handling and gets it subtly wrong). The ONE sanctioned
|
// still returns, with partial stdout, so a live process reads as dead)
|
||||||
// site is error-handling.ts's isProcessAlive win32 branch — centralized,
|
// and the per-tick console flash (#1952). isProcessAlive uses
|
||||||
// windowsHide, bounded timeout, quoted-CSV `"${pid}"` match. Every other
|
// process.kill(pid, 0) on every platform; nothing gets an exemption.
|
||||||
// file must route through the helper.
|
|
||||||
const offenders: string[] = [];
|
const offenders: string[] = [];
|
||||||
for (const { file, content } of readAllSourceFiles()) {
|
for (const { file, content } of readAllSourceFiles()) {
|
||||||
if (file === 'error-handling.ts') continue; // the canonical helper
|
|
||||||
const code = stripComments(content);
|
const code = stripComments(content);
|
||||||
// `PID eq` is the existence-probe form specifically. Other tasklist
|
// `PID eq` is the existence-probe form specifically. Other tasklist
|
||||||
// uses (e.g. IMAGENAME filters for browser detection) are unaffected.
|
// uses (e.g. IMAGENAME filters for browser detection) are unaffected.
|
||||||
|
|||||||
@@ -39,8 +39,9 @@ describe('windowsHide on Windows-reachable spawns (#1835)', () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
test('Windows-only process probes pass windowsHide', () => {
|
test('Windows-only process probes pass windowsHide', () => {
|
||||||
// tasklist in isProcessAlive — runs in polling loops.
|
// isProcessAlive no longer spawns anything (signal-0 on every platform,
|
||||||
expectHideNearEvery(SRC('error-handling.ts'), "'tasklist'");
|
// #1952) — process-liveness-windows.test.ts pins that it stays
|
||||||
|
// subprocess-free, which is stronger than hiding a window.
|
||||||
// powershell DPAPI + tasklist in cookie import.
|
// powershell DPAPI + tasklist in cookie import.
|
||||||
const cookie = SRC('cookie-import-browser.ts');
|
const cookie = SRC('cookie-import-browser.ts');
|
||||||
expectHideNearEvery(cookie, "'powershell'");
|
expectHideNearEvery(cookie, "'powershell'");
|
||||||
|
|||||||
Reference in New Issue
Block a user