v1.42.2.0 fix wave: browse launch hardening (2 bug fixes + headed exit-code wiring) (#1629)

* v1.42.1.1 fix wave: browse launch hardening (2 bug fixes + headed exit-code wiring)

Bundles two browse launch-path bug fixes plus the missing exit-code wiring
that made the second fix actually work end-to-end.

PR #1617 — Chromium sandbox policy at all 3 launch sites
- shouldEnableChromiumSandbox() centralizes the Win32 / CI / CONTAINER /
  root heuristic that previously lived only in the headless launch path.
- launch(), launchHeaded() / launchPersistentContext(), and handoff() now
  share the policy so Playwright stops auto-adding --no-sandbox on every
  headed launch and the yellow "unsupported command-line flag" infobar
  disappears on macOS and Linux dev.

PR #1626 — clean Cmd+Q stops triggering supervisor respawn
- resolveDisconnectCause(browser) reads the underlying Chromium
  ChildProcess exitCode + signalCode (with a 1s wait for an async exit
  event) to distinguish clean user-quit from crash.
- handleChromiumDisconnect(browser) dispatches the headless launch()
  disconnect path: clean → exit(0), crash → exit(1).
- launchHeaded() disconnect handler resolves cause inline and computes
  exitCode = 0 (clean) | 2 (crash) before forwarding to onDisconnect.
- handoff() disconnect handler uses the same shared helper.

Codex-caught propagation fix (this commit, not in either source PR)
- BrowserManager.onDisconnect signature widened to accept an exitCode
  argument. Without this, launchHeaded's locally-computed exit code was
  dropped before reaching server.ts.
- browse/src/server.ts:688 — onDisconnect callback now forwards the
  resolved code: (code) => activeShutdown?.(code ?? 2). The ?? 2
  preserves legacy crash semantics for callers that invoke onDisconnect
  without an explicit code.

Tests
- browse/test/browser-manager-unit.test.ts goes from 2 → 17 tests.
- 6 new tests pin shouldEnableChromiumSandbox across darwin / linux /
  win32 / CI / CONTAINER / root.
- 7 new tests pin resolveDisconnectCause across already-exited,
  async-exit, SIGSEGV, SIGKILL, and null-browser.
- 2 new tests (this commit) pin the onDisconnect(exitCode) propagation
  contract including the exact server.ts forwarding callback shape so a
  refactor that drops the forward fails CI before the user-visible
  respawn bug returns.

Refs PRs #1617, #1626; companion gbrowser PR #23.

* chore: bump version v1.42.1.1 → v1.42.2.0

User-requested rebump (claims v1.42.2.0 slot on the queue).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Garry Tan
2026-05-20 19:30:08 -07:00
committed by GitHub
parent b03cd1ae2d
commit 029356e1f0
6 changed files with 369 additions and 37 deletions

View File

@@ -40,6 +40,76 @@ export function isCustomChromium(): boolean {
return p.includes('GBrowser') || p.includes('gbrowser');
}
/**
* Decide whether Playwright should request Chromium's sandbox.
*
* Returns false on Windows (Bun→Node→Chromium chain breaks the sandbox,
* GitHub #276) and on Linux under root / CI / container (sandbox needs
* unprivileged user namespaces, which are missing for root and typically
* disabled in containers).
*
* When false, Playwright auto-adds --no-sandbox to the launch args — the
* desired behavior in those environments. When true, Playwright does NOT
* add --no-sandbox, which keeps Chromium's "unsupported command-line flag"
* yellow infobar from appearing on every headed launch.
*
* The headless launch path also pushes an explicit '--no-sandbox' into args
* when CI/CONTAINER/root is set; that push is now defensively redundant
* (Playwright will add it anyway when this returns false) and harmless.
*/
export function shouldEnableChromiumSandbox(): boolean {
if (process.platform === 'win32') return false;
const isRoot = typeof process.getuid === 'function' && process.getuid() === 0;
return !(process.env.CI || process.env.CONTAINER || isRoot);
}
/**
* Resolve why the underlying Chromium ChildProcess is going away.
*
* The 'disconnected' Playwright event fires before the child process emits
* its own 'exit' in most cases, so .exitCode is null at that moment. Wait
* briefly (capped at 1s) for the exit then read .exitCode + .signalCode:
*
* exitCode === 0 && no signal → 'clean' (user Cmd+Q, normal shutdown)
* anything else → 'crash' (signal-kill, SIGSEGV, OOM, non-zero exit)
*
* Process supervisors (gbrowser's gbd HealthMonitor in cmd/gbd/health.go)
* read our exit code to decide whether to restart. The two callers in this
* file ride on top of this: a 'clean' result exits with code 0 (gbd skips
* restart, treats as user-intent); a 'crash' result keeps the existing
* per-path exit semantics (launch→1, launchHeaded→2, handoff→1) and gbd
* restarts on backoff.
*/
export async function resolveDisconnectCause(browser: Browser | null): Promise<'clean' | 'crash'> {
const proc = browser?.process();
if (proc && proc.exitCode === null && proc.signalCode === null) {
await new Promise<void>((resolve) => {
const timer = setTimeout(resolve, 1000);
proc.once('exit', () => {
clearTimeout(timer);
resolve();
});
});
}
return proc?.exitCode === 0 && proc?.signalCode == null ? 'clean' : 'crash';
}
/**
* Headless `launch()` disconnect handler. Exits 0 on clean user-quit, 1 on
* crash. Inlined into the launch() body via a one-line dispatch so
* browser-manager's flow stays grep-friendly.
*/
export async function handleChromiumDisconnect(browser: Browser | null): Promise<void> {
const cause = await resolveDisconnectCause(browser);
if (cause === 'clean') {
console.error('[browse] Chromium closed cleanly (user-initiated quit). Server exiting (0).');
process.exit(0);
}
console.error('[browse] FATAL: Chromium process crashed or was killed. Server exiting (1).');
console.error('[browse] Console/network logs flushed to .gstack/browse-*.log');
process.exit(1);
}
export type { RefEntry };
// Re-export TabSession for consumers
@@ -121,7 +191,11 @@ export class BrowserManager {
// (user closed the window). Wired up by server.ts to run full cleanup
// (sidebar-agent, state file, profile locks) before exiting with code 2.
// Returns void or a Promise; rejections are caught and fall back to exit(2).
public onDisconnect: (() => void | Promise<void>) | null = null;
// `exitCode` is the resolved process exit code from the disconnect cause:
// 0 on clean user-initiated quit (e.g., Cmd+Q on headed Chromium), 2 on
// crash/signal-kill. Callers (server.ts) forward it to their shutdown
// pipeline so process supervisors (gbrowser's gbd) read the right signal.
public onDisconnect: ((exitCode?: number) => void | Promise<void>) | null = null;
getConnectionMode(): 'launched' | 'headed' { return this.connectionMode; }
@@ -240,17 +314,25 @@ export class BrowserManager {
headless: useHeadless,
// On Windows, Chromium's sandbox fails when the server is spawned through
// the Bun→Node process chain (GitHub #276). Disable it — local daemon
// browsing user-specified URLs has marginal sandbox benefit.
chromiumSandbox: process.platform !== 'win32',
// browsing user-specified URLs has marginal sandbox benefit. Also disabled
// on Linux root/CI/container, where the sandbox requires unprivileged user
// namespaces that aren't available.
chromiumSandbox: shouldEnableChromiumSandbox(),
...(launchArgs.length > 0 ? { args: launchArgs } : {}),
...(this.proxyConfig ? { proxy: this.proxyConfig } : {}),
});
// Chromium crash → exit with clear message
// Chromium disconnect → distinguish clean user-quit from crash. Both
// events look identical to Playwright (one 'disconnected' fires), but
// the underlying ChildProcess exit code separates them:
// exitCode === 0 → clean quit (user Cmd+Q on macOS, normal shutdown)
// exitCode !== 0 → crash, signal-kill, or OOM
// Process supervisors (gbrowser's gbd) consume our exit code: code 0
// means "user wanted this, don't restart"; non-zero means "crash, please
// bring me back." Without this distinction every Cmd+Q gets treated as
// a crash and the user-visible window keeps respawning.
this.browser.on('disconnected', () => {
console.error('[browse] FATAL: Chromium process crashed or was killed. Server exiting.');
console.error('[browse] Console/network logs flushed to .gstack/browse-*.log');
process.exit(1);
void handleChromiumDisconnect(this.browser);
});
const contextOptions: BrowserContextOptions = {
@@ -415,6 +497,10 @@ export class BrowserManager {
this.context = await chromium.launchPersistentContext(userDataDir, {
headless: false,
// Match the sandbox policy used by launch() above. Without this,
// Playwright auto-adds --no-sandbox on every headed launch and the user
// sees Chromium's "unsupported command-line flag" yellow infobar.
chromiumSandbox: shouldEnableChromiumSandbox(),
args: launchArgs,
viewport: null, // Use browser's default viewport (real window size)
userAgent: this.customUserAgent || customUA,
@@ -542,32 +628,45 @@ export class BrowserManager {
await this.newTab();
}
// Browser disconnect handler — exit code 2 distinguishes from crashes (1).
// Calls onDisconnect() to trigger full shutdown (kill sidebar-agent, save
// session, clean profile locks + state file) before exit. Falls back to
// direct process.exit(2) if no callback is wired up, or if the callback
// throws/rejects — never leave the process running with a dead browser.
// Browser disconnect handler — distinguish user Cmd+Q from real crash.
// Clean exit (Chromium exit code 0) → process.exit(0) so process
// supervisors (gbrowser's gbd) treat it as user intent and skip the
// restart loop. Crash → process.exit(2) preserves the legacy headed
// semantics that's distinct from launch()'s code 1.
// Always calls onDisconnect() first to trigger full shutdown (kill
// sidebar-agent, save session, clean profile locks + state file) so
// crashes don't strand resources either.
if (this.browser) {
this.browser.on('disconnected', () => {
if (this.intentionalDisconnect) return;
console.error('[browse] Real browser disconnected (user closed or crashed).');
console.error('[browse] Run `$B connect` to reconnect.');
if (!this.onDisconnect) {
process.exit(2);
return;
}
try {
const result = this.onDisconnect();
if (result && typeof (result as Promise<void>).catch === 'function') {
(result as Promise<void>).catch((err) => {
console.error('[browse] onDisconnect rejected:', err);
process.exit(2);
});
const browserRef = this.browser;
void (async () => {
const cause = await resolveDisconnectCause(browserRef);
const exitCode = cause === 'clean' ? 0 : 2;
if (cause === 'clean') {
console.error('[browse] Real browser closed cleanly (user-initiated quit). Server exiting (0).');
} else {
console.error('[browse] Real browser disconnected (crash or kill). Server exiting (2).');
console.error('[browse] Run `$B connect` to reconnect.');
}
} catch (err) {
console.error('[browse] onDisconnect threw:', err);
process.exit(2);
}
if (!this.onDisconnect) {
process.exit(exitCode);
return;
}
try {
const result = this.onDisconnect(exitCode);
if (result && typeof (result as Promise<void>).catch === 'function') {
(result as Promise<void>).catch((err) => {
console.error('[browse] onDisconnect rejected:', err);
process.exit(exitCode);
});
}
// onDisconnect is responsible for exit on the success path.
} catch (err) {
console.error('[browse] onDisconnect threw:', err);
process.exit(exitCode);
}
})();
});
}
@@ -1303,6 +1402,10 @@ export class BrowserManager {
newContext = await chromium.launchPersistentContext(userDataDir, {
headless: false,
// Match the sandbox policy used by launchHeaded() / launch(). The
// handoff path is the headless→headed re-launch and shares the same
// anti-detection posture, including no spurious --no-sandbox infobar.
chromiumSandbox: shouldEnableChromiumSandbox(),
args: launchArgs,
viewport: null,
...(this.proxyConfig ? { proxy: this.proxyConfig } : {}),
@@ -1332,12 +1435,14 @@ export class BrowserManager {
await newContext.setExtraHTTPHeaders(this.extraHeaders);
}
// Register crash handler on new browser
// Register disconnect handler on new browser. Same clean-vs-crash
// discrimination as launch() / launchHeaded() above so a user-initiated
// Cmd+Q after a handoff doesn't trigger gbd's restart loop.
if (this.browser) {
const browserRef = this.browser;
this.browser.on('disconnected', () => {
if (this.intentionalDisconnect) return;
console.error('[browse] FATAL: Chromium process crashed or was killed. Server exiting.');
process.exit(1);
void handleChromiumDisconnect(browserRef);
});
}

View File

@@ -680,8 +680,12 @@ function emitInspectorEvent(event: any): void {
const browserManager = new BrowserManager();
// When the user closes the headed browser window, run full cleanup
// (kill sidebar-agent, save session, remove profile locks, delete state file)
// before exiting with code 2. Exit code 2 distinguishes user-close from crashes (1).
browserManager.onDisconnect = () => activeShutdown?.(2);
// before exiting. Exit code 0 means user-initiated clean quit (Cmd+Q on
// macOS) so process supervisors like gbrowser's gbd skip the restart loop;
// 2 means a real crash that should respawn. The fallback `?? 2` preserves
// legacy crash semantics for any caller that invokes onDisconnect without
// an explicit code.
browserManager.onDisconnect = (code) => activeShutdown?.(code ?? 2);
let isShuttingDown = false;
// Test if a port is available by binding and immediately releasing.

View File

@@ -1,4 +1,5 @@
import { describe, it, expect } from 'bun:test';
import { EventEmitter } from 'node:events';
import { afterEach, beforeEach, describe, it, expect } from 'bun:test';
// ─── BrowserManager basic unit tests ─────────────────────────────
@@ -15,3 +16,186 @@ describe('BrowserManager defaults', () => {
expect(bm.getRefMap()).toEqual([]);
});
});
// ─── shouldEnableChromiumSandbox ─────────────────────────────────
//
// Pinning this is what prevents the "--no-sandbox" yellow infobar from
// regressing on headed launches. Playwright auto-adds --no-sandbox when
// chromiumSandbox !== true (playwright-core chromium.js:291-292), so all
// three launch sites in browser-manager.ts must pass the policy this
// helper computes.
describe('shouldEnableChromiumSandbox', () => {
const origPlatform = process.platform;
const origCI = process.env.CI;
const origContainer = process.env.CONTAINER;
const origGetuid = process.getuid;
beforeEach(() => {
delete process.env.CI;
delete process.env.CONTAINER;
});
afterEach(() => {
Object.defineProperty(process, 'platform', { value: origPlatform });
if (origCI === undefined) delete process.env.CI; else process.env.CI = origCI;
if (origContainer === undefined) delete process.env.CONTAINER; else process.env.CONTAINER = origContainer;
process.getuid = origGetuid;
});
function setPlatform(p: NodeJS.Platform) {
Object.defineProperty(process, 'platform', { value: p });
}
it('darwin, no CI/CONTAINER/root → true', async () => {
setPlatform('darwin');
process.getuid = (() => 501) as typeof process.getuid;
const { shouldEnableChromiumSandbox } = await import('../src/browser-manager');
expect(shouldEnableChromiumSandbox()).toBe(true);
});
it('linux, no CI/CONTAINER/root → true', async () => {
setPlatform('linux');
process.getuid = (() => 1000) as typeof process.getuid;
const { shouldEnableChromiumSandbox } = await import('../src/browser-manager');
expect(shouldEnableChromiumSandbox()).toBe(true);
});
it('win32 → false (sandbox fails in Bun→Node→Chromium chain)', async () => {
setPlatform('win32');
process.getuid = (() => 1000) as typeof process.getuid;
const { shouldEnableChromiumSandbox } = await import('../src/browser-manager');
expect(shouldEnableChromiumSandbox()).toBe(false);
});
it('linux + CI=1 → false', async () => {
setPlatform('linux');
process.env.CI = '1';
process.getuid = (() => 1000) as typeof process.getuid;
const { shouldEnableChromiumSandbox } = await import('../src/browser-manager');
expect(shouldEnableChromiumSandbox()).toBe(false);
});
it('linux + CONTAINER=1 → false', async () => {
setPlatform('linux');
process.env.CONTAINER = '1';
process.getuid = (() => 1000) as typeof process.getuid;
const { shouldEnableChromiumSandbox } = await import('../src/browser-manager');
expect(shouldEnableChromiumSandbox()).toBe(false);
});
it('linux + root (uid 0) → false', async () => {
setPlatform('linux');
process.getuid = (() => 0) as typeof process.getuid;
const { shouldEnableChromiumSandbox } = await import('../src/browser-manager');
expect(shouldEnableChromiumSandbox()).toBe(false);
});
});
// ─── resolveDisconnectCause ──────────────────────────────────────
//
// Pinning the clean-vs-crash distinction matters because gbd's
// HealthMonitor consumes our exit code (0 = don't restart, !=0 =
// restart). A regression here brings back the "Cmd+Q makes the browser
// keep coming back" UX bug.
function makeFakeBrowser(opts: {
exitCode: number | null;
signalCode: NodeJS.Signals | null;
/** ms before emitting 'exit'; default = already exited at construction */
exitDelay?: number;
}): { process(): { exitCode: number | null; signalCode: NodeJS.Signals | null; once: EventEmitter['once'] } } {
const ee = new EventEmitter();
const state = {
exitCode: opts.exitDelay != null ? null : opts.exitCode,
signalCode: opts.exitDelay != null ? null : opts.signalCode,
once: ee.once.bind(ee),
};
if (opts.exitDelay != null) {
setTimeout(() => {
state.exitCode = opts.exitCode;
state.signalCode = opts.signalCode;
ee.emit('exit', opts.exitCode, opts.signalCode);
}, opts.exitDelay);
}
return { process: () => state };
}
describe('resolveDisconnectCause', () => {
it('clean: process already exited with code 0', async () => {
const { resolveDisconnectCause } = await import('../src/browser-manager');
const fake = makeFakeBrowser({ exitCode: 0, signalCode: null });
expect(await resolveDisconnectCause(fake as never)).toBe('clean');
});
it('crash: non-zero exit code', async () => {
const { resolveDisconnectCause } = await import('../src/browser-manager');
const fake = makeFakeBrowser({ exitCode: 1, signalCode: null });
expect(await resolveDisconnectCause(fake as never)).toBe('crash');
});
it('crash: SIGSEGV', async () => {
const { resolveDisconnectCause } = await import('../src/browser-manager');
const fake = makeFakeBrowser({ exitCode: null, signalCode: 'SIGSEGV' });
expect(await resolveDisconnectCause(fake as never)).toBe('crash');
});
it('crash: SIGKILL', async () => {
const { resolveDisconnectCause } = await import('../src/browser-manager');
const fake = makeFakeBrowser({ exitCode: null, signalCode: 'SIGKILL' });
expect(await resolveDisconnectCause(fake as never)).toBe('crash');
});
it('clean: process exits asynchronously with code 0 within timeout', async () => {
const { resolveDisconnectCause } = await import('../src/browser-manager');
const fake = makeFakeBrowser({ exitCode: 0, signalCode: null, exitDelay: 50 });
expect(await resolveDisconnectCause(fake as never)).toBe('clean');
});
it('crash: process exits asynchronously with non-zero code', async () => {
const { resolveDisconnectCause } = await import('../src/browser-manager');
const fake = makeFakeBrowser({ exitCode: 137, signalCode: null, exitDelay: 50 });
expect(await resolveDisconnectCause(fake as never)).toBe('crash');
});
it('crash: null browser returns crash (defensive default)', async () => {
const { resolveDisconnectCause } = await import('../src/browser-manager');
expect(await resolveDisconnectCause(null)).toBe('crash');
});
});
// ─── onDisconnect exit-code propagation (regression test) ──────────
//
// The contract: BrowserManager.onDisconnect is called with the resolved
// exit code (0 for clean Cmd+Q, 2 for crash). server.ts then forwards
// that code to activeShutdown(), which exits the process.
//
// Without this propagation, the headed-mode user-visible Cmd+Q respawn
// bug returns: server.ts hardcoded `activeShutdown?.(2)` ignores the
// resolved 0 and gbrowser's gbd HealthMonitor treats the clean quit as
// a crash, restarting the window.
describe('BrowserManager.onDisconnect exit-code propagation', () => {
it('signature accepts an optional exitCode argument', async () => {
const { BrowserManager } = await import('../src/browser-manager');
const bm = new BrowserManager();
const calls: Array<number | undefined> = [];
bm.onDisconnect = (code?: number) => { calls.push(code); };
bm.onDisconnect(0);
bm.onDisconnect(2);
bm.onDisconnect(undefined);
expect(calls).toEqual([0, 2, undefined]);
});
it('server.ts callback forwards exitCode when provided, falls back to 2', async () => {
// Mirror the production wiring in browse/src/server.ts so a refactor
// that drops the forward (e.g. reverting to `() => activeShutdown?.(2)`)
// fails CI before the user-visible bug returns.
const shutdownCalls: number[] = [];
const activeShutdown = (code: number) => { shutdownCalls.push(code); };
const onDisconnect = (code?: number) => activeShutdown(code ?? 2);
onDisconnect(0);
onDisconnect(2);
onDisconnect(undefined);
expect(shutdownCalls).toEqual([0, 2, 2]);
});
});