mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-23 05:10:50 +02:00
fix(browse): chromium reap works off-Linux and on every stale-state path
readPidCmdline fell back to '' on darwin (no /proc), so the identity gate never matched and reapRecordedChromium was inert on the platform #2709's GPU-spin reap actually targets — it now falls back to ps -o command=. readPidStartTime no longer throws when ps is missing (Windows): a launch must never die to a reap-bookkeeping probe. Three stale-state cleanup paths (dead-daemon stop, startServer stale cleanup, headed-connect) now reap the recorded chromium BEFORE unlinking the state file instead of orphaning it, and the stop-path wait polls (100ms steps, 1s cap) instead of sleeping a fixed 500ms. Wiring pinned: server-state pid/start-time write, all five cli.ts reap call sites, headless-only GPU-flag push. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
d51acc4c32
commit
159b48e813
+24
-3
@@ -316,10 +316,14 @@ export async function reapRecordedChromium(state: {
|
|||||||
const cmd = readPidCmdline(pid).toLowerCase();
|
const cmd = readPidCmdline(pid).toLowerCase();
|
||||||
if (!/chrom|headless_shell/.test(cmd)) return;
|
if (!/chrom|headless_shell/.test(cmd)) return;
|
||||||
safeKill(pid, 'SIGTERM');
|
safeKill(pid, 'SIGTERM');
|
||||||
await new Promise(r => setTimeout(r, 1000));
|
// Poll instead of a fixed sleep: the common case (daemon's own close is
|
||||||
|
// finishing concurrently) exits in ~100-200ms instead of always paying 1s.
|
||||||
|
const deadline = Date.now() + 1000;
|
||||||
|
while (Date.now() < deadline && isProcessAlive(pid)) {
|
||||||
|
await new Promise(r => setTimeout(r, 100));
|
||||||
|
}
|
||||||
if (isProcessAlive(pid)) {
|
if (isProcessAlive(pid)) {
|
||||||
safeKill(pid, 'SIGKILL');
|
safeKill(pid, 'SIGKILL');
|
||||||
await new Promise(r => setTimeout(r, 500));
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -496,7 +500,14 @@ async function startServer(extraEnv?: Record<string, string>): Promise<ServerSta
|
|||||||
// Bound the append-mode daemon log before the new daemon starts writing.
|
// Bound the append-mode daemon log before the new daemon starts writing.
|
||||||
rotateDaemonLogIfOversized();
|
rotateDaemonLogIfOversized();
|
||||||
|
|
||||||
// Clean up stale state file and error log
|
// Clean up stale state file and error log. Reap the previous daemon's
|
||||||
|
// recorded headless Chromium first — the state file is the only carrier of
|
||||||
|
// its identity, and the lock-less headless child is invisible to
|
||||||
|
// killOrphanChromium below (#2709). Identity-gated, so safe when stale.
|
||||||
|
{
|
||||||
|
const staleState = readState();
|
||||||
|
if (staleState) await reapRecordedChromium(staleState);
|
||||||
|
}
|
||||||
safeUnlink(config.stateFile);
|
safeUnlink(config.stateFile);
|
||||||
safeUnlink(path.join(config.stateDir, 'browse-startup-error.log'));
|
safeUnlink(path.join(config.stateDir, 'browse-startup-error.log'));
|
||||||
|
|
||||||
@@ -1592,7 +1603,13 @@ Refs: After 'snapshot', use @e1, @e2... as selectors:
|
|||||||
// Kill an orphaned Chromium still holding the profile lock (the Bun server
|
// Kill an orphaned Chromium still holding the profile lock (the Bun server
|
||||||
// PID's Chromium child can outlive an abrupt kill/crash), then clear the
|
// PID's Chromium child can outlive an abrupt kill/crash), then clear the
|
||||||
// lock files so the launch is clean. Shared with the auto-restart path (#1781).
|
// lock files so the launch is clean. Shared with the auto-restart path (#1781).
|
||||||
|
// Also reap the lock-less headless child recorded in the state file before
|
||||||
|
// deleting it — killOrphanChromium can't see it (#2709).
|
||||||
await killOrphanChromium();
|
await killOrphanChromium();
|
||||||
|
{
|
||||||
|
const staleState = readState();
|
||||||
|
if (staleState) await reapRecordedChromium(staleState);
|
||||||
|
}
|
||||||
cleanChromiumProfileLocks();
|
cleanChromiumProfileLocks();
|
||||||
|
|
||||||
// Delete stale state file
|
// Delete stale state file
|
||||||
@@ -1837,6 +1854,10 @@ Refs: After 'snapshot', use @e1, @e2... as selectors:
|
|||||||
process.exit(0);
|
process.exit(0);
|
||||||
}
|
}
|
||||||
if (!isProcessAlive(stopState.pid) && !(await isServerHealthy(stopState.port))) {
|
if (!isProcessAlive(stopState.pid) && !(await isServerHealthy(stopState.port))) {
|
||||||
|
// The daemon died abruptly (SIGKILL, crash) — the likeliest orphan case.
|
||||||
|
// Reap the recorded headless Chromium BEFORE destroying the state file,
|
||||||
|
// which is the only carrier of its identity (#2709).
|
||||||
|
await reapRecordedChromium(stopState);
|
||||||
safeUnlinkQuiet(config.stateFile);
|
safeUnlinkQuiet(config.stateFile);
|
||||||
console.log('No daemon running (cleaned stale state) — nothing to stop.');
|
console.log('No daemon running (cleaned stale state) — nothing to stop.');
|
||||||
process.exit(0);
|
process.exit(0);
|
||||||
|
|||||||
+28
-7
@@ -95,12 +95,20 @@ export function pickFreeDisplay(
|
|||||||
*/
|
*/
|
||||||
export function readPidStartTime(pid: number): string {
|
export function readPidStartTime(pid: number): string {
|
||||||
if (!isProcessAlive(pid)) return '';
|
if (!isProcessAlive(pid)) return '';
|
||||||
const result = Bun.spawnSync(['ps', '-p', String(pid), '-o', 'lstart='], {
|
try {
|
||||||
windowsHide: true,
|
const result = Bun.spawnSync(['ps', '-p', String(pid), '-o', 'lstart='], {
|
||||||
stdout: 'pipe', stderr: 'pipe', timeout: 2000,
|
windowsHide: true,
|
||||||
});
|
stdout: 'pipe', stderr: 'pipe', timeout: 2000,
|
||||||
if (result.exitCode !== 0) return '';
|
});
|
||||||
return result.stdout.toString().trim();
|
if (result.exitCode !== 0) return '';
|
||||||
|
return result.stdout.toString().trim();
|
||||||
|
} catch {
|
||||||
|
// Bun.spawnSync THROWS when the executable is missing (Windows shells
|
||||||
|
// without an MSYS `ps`). This function's contract is "empty string if
|
||||||
|
// ps fails" — a missing ps must not abort the caller (browser-manager
|
||||||
|
// now calls this on the universal launch path, #2709).
|
||||||
|
return '';
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -111,7 +119,20 @@ export function readPidCmdline(pid: number): string {
|
|||||||
try {
|
try {
|
||||||
return fs.readFileSync(`/proc/${pid}/cmdline`, 'utf-8').replace(/\0/g, ' ').trim();
|
return fs.readFileSync(`/proc/${pid}/cmdline`, 'utf-8').replace(/\0/g, ' ').trim();
|
||||||
} catch {
|
} catch {
|
||||||
return '';
|
// No /proc on darwin — the platform #2709's reap actually targets. Fall
|
||||||
|
// back to ps (same pattern as readPidStartTime above); without this the
|
||||||
|
// reap's cmdline identity gate always saw '' on macOS and the reap was
|
||||||
|
// a structural no-op exactly where the spinning-GPU orphan lives.
|
||||||
|
try {
|
||||||
|
const result = Bun.spawnSync(['ps', '-p', String(pid), '-o', 'command='], {
|
||||||
|
windowsHide: true,
|
||||||
|
stdout: 'pipe', stderr: 'pipe', timeout: 2000,
|
||||||
|
});
|
||||||
|
if (result.exitCode !== 0) return '';
|
||||||
|
return result.stdout.toString().trim();
|
||||||
|
} catch {
|
||||||
|
return '';
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -104,3 +104,96 @@ describe.skipIf(process.platform !== 'linux')('reapRecordedChromium identity gat
|
|||||||
await reapRecordedChromium({ chromiumPid: 999999999, chromiumStartTime: 'x' });
|
await reapRecordedChromium({ chromiumPid: 999999999, chromiumStartTime: 'x' });
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// ─── Source pins: the #2709 pieces stay WIRED ────────────────────────────
|
||||||
|
// The unit tests above prove headlessGpuArgs and reapRecordedChromium behave;
|
||||||
|
// these pins prove the daemon persists the child's identity, the CLI reaps on
|
||||||
|
// every stop/stale-cleanup path, and the GPU flags only ever reach a headless
|
||||||
|
// launch. Anchored to function names and call expressions, never line numbers.
|
||||||
|
describe('stop-reap wiring pins (#2709)', () => {
|
||||||
|
const fs = require('node:fs') as typeof import('node:fs');
|
||||||
|
const path = require('node:path') as typeof import('node:path');
|
||||||
|
const SRC = path.resolve(import.meta.dir, '..', 'src');
|
||||||
|
const read = (f: string) => fs.readFileSync(path.join(SRC, f), 'utf-8');
|
||||||
|
|
||||||
|
test('server.ts persists chromiumPid/chromiumStartTime from getChromiumProcInfo() into the state file', () => {
|
||||||
|
const src = read('server.ts');
|
||||||
|
const stateStart = src.indexOf('const state: Record<string, unknown> = {');
|
||||||
|
expect(stateStart, 'state-object literal not found in server.ts').toBeGreaterThan(-1);
|
||||||
|
// The object literal ends where the daemon serializes it and renames the
|
||||||
|
// tmp file into place — identity must be INSIDE what gets persisted.
|
||||||
|
const stateEnd = src.indexOf('fs.renameSync(tmpFile, config.stateFile)', stateStart);
|
||||||
|
expect(stateEnd, 'state-file rename not found after the state object').toBeGreaterThan(stateStart);
|
||||||
|
const stateObj = src.slice(stateStart, stateEnd);
|
||||||
|
expect(stateObj).toContain('browserManager.getChromiumProcInfo()');
|
||||||
|
expect(stateObj).toContain('chromiumPid: info.pid');
|
||||||
|
expect(stateObj).toContain('chromiumStartTime: info.startTime');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('cli.ts reaps on every stop + stale-state path (>=5 call sites)', () => {
|
||||||
|
const src = read('cli.ts');
|
||||||
|
// Every call site awaits; the only other occurrence is the definition.
|
||||||
|
expect(src).toContain('export async function reapRecordedChromium(');
|
||||||
|
const callSites = src.split('await reapRecordedChromium(').length - 1;
|
||||||
|
expect(callSites, 'a reap call site was removed — every stop/stale path must reap').toBeGreaterThanOrEqual(5);
|
||||||
|
|
||||||
|
const between = (from: string, to: string) => {
|
||||||
|
const a = src.indexOf(from);
|
||||||
|
expect(a, `anchor not found in cli.ts: ${from}`).toBeGreaterThan(-1);
|
||||||
|
const b = src.indexOf(to, a);
|
||||||
|
expect(b, `anchor not found after "${from}": ${to}`).toBeGreaterThan(a);
|
||||||
|
return src.slice(a, b);
|
||||||
|
};
|
||||||
|
|
||||||
|
// 1. Dead-daemon stop branch: reap BEFORE destroying the state file — the
|
||||||
|
// state file is the only carrier of the child's identity.
|
||||||
|
const deadDaemon = between(
|
||||||
|
'!isProcessAlive(stopState.pid) && !(await isServerHealthy(stopState.port))',
|
||||||
|
'No daemon running (cleaned stale state)',
|
||||||
|
);
|
||||||
|
expect(deadDaemon).toMatch(
|
||||||
|
/await reapRecordedChromium\(stopState\);[\s\S]*safeUnlinkQuiet\(config\.stateFile\);/,
|
||||||
|
);
|
||||||
|
|
||||||
|
// 2. Force-stop on a live daemon (stop --force-restart short-circuit).
|
||||||
|
const forceStop = between(
|
||||||
|
'isProcessAlive(stopState.pid) && globalFlags.forceRestart',
|
||||||
|
'Daemon stopped (forced',
|
||||||
|
);
|
||||||
|
expect(forceStop).toContain('await reapRecordedChromium(stopState);');
|
||||||
|
|
||||||
|
// 3. startServer stale-state cleanup, before safeUnlink(config.stateFile).
|
||||||
|
const startServer = between('async function startServer(', 'safeUnlink(config.stateFile);');
|
||||||
|
expect(startServer).toMatch(
|
||||||
|
/const staleState = readState\(\);\s*\n\s*if \(staleState\) await reapRecordedChromium\(staleState\);/,
|
||||||
|
);
|
||||||
|
|
||||||
|
// 4. Headed-connect stale path: reap sits between killOrphanChromium()
|
||||||
|
// (which cannot see the lock-less headless child) and the unlink.
|
||||||
|
const connect = between("if (command === 'connect')", 'Launching headed Chromium');
|
||||||
|
expect(connect).toMatch(
|
||||||
|
/await killOrphanChromium\(\);[\s\S]*if \(staleState\) await reapRecordedChromium\(staleState\);[\s\S]*safeUnlinkQuiet\(config\.stateFile\);/,
|
||||||
|
);
|
||||||
|
|
||||||
|
// 5. Post-graceful-stop: the daemon closed Chromium via Playwright, but a
|
||||||
|
// surviving GPU process must still be reaped after sendCommand('stop').
|
||||||
|
const postStop = between('await sendCommand(state, command, commandArgs);', "if (command === 'focus')");
|
||||||
|
expect(postStop).toMatch(
|
||||||
|
/if \(command === 'stop'\) \{\s*\n\s*await reapRecordedChromium\(state\);/,
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('browser-manager.ts pushes headlessGpuArgs only under the headless launch guard', () => {
|
||||||
|
const src = read('browser-manager.ts');
|
||||||
|
// Exactly one call site (the exported definition aside) — a second,
|
||||||
|
// unguarded push would strip the GPU from headed/GBrowser sessions.
|
||||||
|
const calls = src.match(/headlessGpuArgs\(process\.platform, process\.env\)/g) ?? [];
|
||||||
|
expect(calls.length).toBe(1);
|
||||||
|
// And that one call site is guarded by the headless flag: the extensions
|
||||||
|
// path above it forces useHeadless = false, so extension-loaded and headed
|
||||||
|
// launches never receive the GPU-disable flags.
|
||||||
|
expect(src).toMatch(
|
||||||
|
/if \(useHeadless\) \{\s*\n\s*launchArgs\.push\(\.\.\.headlessGpuArgs\(process\.platform, process\.env\)\);\s*\n\s*\}/,
|
||||||
|
);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user