From f3ea49ff6e705a8a9dc3553a9b05d08ba74230a5 Mon Sep 17 00:00:00 2001 From: Shawn Reddy <19191746+Screddyice@users.noreply.github.com> Date: Fri, 14 Aug 2026 21:28:08 +0800 Subject: [PATCH] fix(browse): cancel the parent watchdog when handoff promotes a daemon to headed The parent-process watchdog assumes connection mode is fixed at boot: headless daemons outlive their parent, headed ones do not. The env guards (BROWSE_PARENT_PID=0, BROWSE_HEADED=1) only cover daemons that were headed when they started. handoff breaks that assumption. It swaps in a headed context on a RUNNING daemon and sets connectionMode = 'headed' without a restart, so a daemon that legitimately registered a watchdog lands on the fatal side of the branch. The parent is usually a short-lived shell, and Claude Code's Bash tool kills one after every invocation, so the next 15s poll shuts the daemon down. The user-visible effect is that handoff destroys the thing it just created. It exists so a human can log in, solve a CAPTCHA, or clear an MFA prompt; the browser disappears about fifteen seconds later and takes the session with it. Observed while driving two registrar control panels: five daemon deaths and three logins, each one discarding the authenticated session. BrowserManager now exposes onHeadedPromotion, fired only on runtime promotion and not on a headed boot, and the server binds it to a canceller for the interval it already owned but previously discarded. Bound on both the module-level manager and any embedder-supplied one, since the watchdog reads activeBrowserManager and binding only the default would let embedders promote silently. The binding sits next to the browserManager declaration rather than next to clearParentWatchdog. Placing it with the function, which lives with the watchdog it cancels, reads better but touches browserManager in its temporal dead zone, which aborts module evaluation and leaves every later const uninitialized. findport tests catch that immediately. Tests: watchdog.test.ts already noted in its header that its three cases all fix mode via env at spawn time, so none reaches the headed branch. Driving a real handoff needs a headed Chromium, so the wiring is pinned with static tripwires instead, matching cdp-session-cleanup.test.ts and server-auth.test.ts. Verified they fail when the notification call is removed and pass when restored. Full `bun test` shows the same 6 pre-existing failures on this branch and on main (gstack-gbrain-detect, gstack-artifacts-init), which pass in isolation on both, so they are test-order pollution rather than a regression here. --- browse/src/browser-manager.ts | 17 +++++++++++++++ browse/src/server.ts | 39 +++++++++++++++++++++++++++++++++- browse/test/watchdog.test.ts | 40 +++++++++++++++++++++++++++++++++++ 3 files changed, 95 insertions(+), 1 deletion(-) diff --git a/browse/src/browser-manager.ts b/browse/src/browser-manager.ts index 4b378cc4f..65597ce45 100644 --- a/browse/src/browser-manager.ts +++ b/browse/src/browser-manager.ts @@ -195,6 +195,15 @@ export class BrowserManager { // ─── Headed State ──────────────────────────────────────── private connectionMode: 'launched' | 'headed' = 'launched'; + + /** + * Fired when a RUNNING daemon is promoted to headed mode (see handoff()), + * as opposed to starting headed. The server uses it to cancel the + * parent-process watchdog, which was registered on the assumption that mode + * is fixed at boot and would otherwise kill the freshly handed-off browser + * the next time the spawning shell exits. + */ + onHeadedPromotion?: () => void; private intentionalDisconnect = false; // ─── Tab Count Guardrail (D5 + Codex single-tab flag) ─────── @@ -1603,6 +1612,14 @@ export class BrowserManager { this.tabSessions.clear(); this.connectionMode = 'headed'; + // Promotion, not a headed boot. The server registered a parent-process + // watchdog because this daemon started headless, and that watchdog kills + // headed daemons when their parent exits — which for a CLI-spawned daemon + // is immediately. Without this the handed-off browser dies ~15s later, + // taking whatever the user was mid-way through (a login, an MFA prompt) + // with it. + this.onHeadedPromotion?.(); + // Same Layer C stealth as launch()/launchHeaded(). Must run BEFORE // restoreState() navigates so the init scripts apply to the restored // pages — without this the handed-off browser had cmdline args but no diff --git a/browse/src/server.ts b/browse/src/server.ts index 7bf5f439c..42130df84 100644 --- a/browse/src/server.ts +++ b/browse/src/server.ts @@ -696,9 +696,14 @@ const BROWSE_PARENT_PID = parseInt(process.env.BROWSE_PARENT_PID || '0', 10); // the closure every 15s. The CLI's connect path sets BROWSE_HEADED=1 + PID=0, // so this branch is the normal path for /open-gstack-browser. const IS_HEADED_WATCHDOG = process.env.BROWSE_HEADED === '1'; +// Kept so a runtime promotion to headed can cancel it. The env guards above only +// cover daemons that were headed at BOOT; `handoff` promotes a running headless +// daemon in place, and the watchdog registered here would then kill it on the +// next parent death. See clearParentWatchdog() below. +let parentWatchdogTimer: ReturnType | null = null; if (BROWSE_PARENT_PID > 0 && !IS_HEADED_WATCHDOG) { let parentGone = false; - setInterval(() => { + parentWatchdogTimer = setInterval(() => { try { process.kill(BROWSE_PARENT_PID, 0); // signal 0 = existence check only, no signal sent } catch { @@ -730,6 +735,28 @@ if (BROWSE_PARENT_PID > 0 && !IS_HEADED_WATCHDOG) { console.log('[browse] Parent-process watchdog disabled (BROWSE_PARENT_PID=0)'); } +/** + * Cancel the parent-process watchdog after a runtime promotion to headed mode. + * + * The watchdog's contract is "headless daemons outlive their parent, headed ones + * do not" — reasonable at boot, when mode is fixed by env. `handoff` breaks that + * assumption: it swaps in a headed context on a RUNNING daemon + * (browser-manager.ts, connectionMode = 'headed') without a restart, so a daemon + * that legitimately registered a watchdog is suddenly on the fatal side of the + * branch. The parent is typically a short-lived shell — Claude Code's Bash tool + * kills one after every invocation — so the next 15s poll shuts the daemon down, + * discarding whatever the user was handed off to do, such as a login. + * + * Once promoted, the user owns the window lifecycle exactly as if the daemon had + * been started headed, which is the case the env guards already exempt. + */ +function clearParentWatchdog(): void { + if (!parentWatchdogTimer) return; + clearInterval(parentWatchdogTimer); + parentWatchdogTimer = null; + console.log('[browse] Parent-process watchdog cleared (promoted to headed at runtime)'); +} + // ─── Command Sets (from commands.ts — single source of truth) ─── import { READ_COMMANDS, WRITE_COMMANDS, META_COMMANDS } from './commands'; export { READ_COMMANDS, WRITE_COMMANDS, META_COMMANDS }; @@ -772,6 +799,11 @@ function emitInspectorEvent(event: any): void { // ─── Server ──────────────────────────────────────────────────── const browserManager = new BrowserManager(); +// Declared here rather than beside clearParentWatchdog: that function sits with +// the watchdog it cancels, which is above this line, and binding it up there +// would touch `browserManager` in its temporal dead zone — aborting module +// evaluation and leaving every later const uninitialized. +browserManager.onHeadedPromotion = clearParentWatchdog; // Indirection for embedders. Module-level handlers (idleCheckTick, parent // watchdog, SIGTERM) read activeBrowserManager so that buildFetchHandler can // retarget them at a caller-supplied BrowserManager. Symmetric with the @@ -1651,6 +1683,11 @@ export function buildFetchHandler(cfg: ServerConfig): ServerHandle { // after 30 min of HTTP idle because the dead module-level instance still // reports connectionMode === 'launched'. activeBrowserManager = cfgBrowserManager; + // Same reason as above: the watchdog reads activeBrowserManager, so the + // instance that can promote itself to headed must be the one that can cancel + // it. An embedder-supplied manager otherwise promotes silently and the + // watchdog keeps running against a mode it can no longer see. + cfgBrowserManager.onHeadedPromotion = clearParentWatchdog; // Wire the cfg-instance's onDisconnect to run shutdown when the user // closes the headed browser window. CHAIN any caller-provided handler diff --git a/browse/test/watchdog.test.ts b/browse/test/watchdog.test.ts index 42faa262a..15ce8e096 100644 --- a/browse/test/watchdog.test.ts +++ b/browse/test/watchdog.test.ts @@ -155,3 +155,43 @@ describe('parent-process watchdog (v0.18.1.0)', () => { expect(isProcessAlive(serverPid)).toBe(true); }, 45_000); }); + +// The three tests above all fix the mode via env at SPAWN time, so none of them +// reaches the headed branch of the watchdog. That branch is only reachable by a +// RUNTIME promotion, which `handoff` performs: it swaps in a headed context on a +// running daemon without a restart, moving a daemon that legitimately registered +// a watchdog onto the fatal side of the check. The parent is usually a +// short-lived shell (Claude Code's Bash tool kills one after every invocation), +// so the next poll shut the daemon down and discarded whatever the user had been +// handed off to do — observed as repeated session loss mid-login. +// +// Driving a real `handoff` needs a headed Chromium, which does not belong in the +// free tier, so this pins the WIRING instead — the same static-tripwire approach +// used by cdp-session-cleanup.test.ts and server-auth.test.ts. If either half of +// the contract is dropped, the crash returns silently and these fail. +describe('watchdog is cancelled on runtime promotion to headed', () => { + const read = (rel: string) => fs.readFileSync(path.join(ROOT, rel), 'utf-8'); + + test('handoff() notifies the server that it promoted the daemon', () => { + const src = read('src/browser-manager.ts'); + const promote = src.indexOf("this.connectionMode = 'headed';", src.indexOf('async handoff(')); + expect(promote).toBeGreaterThan(-1); + // The notification must follow the promotion closely; a call left far away + // (or removed) is the regression this guards. + expect(src.slice(promote, promote + 800)).toContain('this.onHeadedPromotion?.()'); + }); + + test('the server binds that callback to the watchdog canceller', () => { + const src = read('src/server.ts'); + // The timer must be reachable — `setInterval(` with its return value dropped + // cannot be cleared, which was the original defect. + expect(src).toContain('parentWatchdogTimer = setInterval('); + expect(src).toContain('function clearParentWatchdog()'); + expect(src).toContain('clearInterval(parentWatchdogTimer)'); + // Bound on BOTH the module-level manager and any embedder-supplied one; the + // watchdog reads activeBrowserManager, so binding only the default instance + // leaves embedders (e.g. gbrowser) promoting silently. + expect(src).toContain('browserManager.onHeadedPromotion = clearParentWatchdog'); + expect(src).toContain('cfgBrowserManager.onHeadedPromotion = clearParentWatchdog'); + }); +});