diff --git a/browse/src/cli.ts b/browse/src/cli.ts index 4df950190..59327b792 100644 --- a/browse/src/cli.ts +++ b/browse/src/cli.ts @@ -211,6 +211,86 @@ function cleanupLegacyState(): void { } } +// ─── Chromium profile lock helpers (#1781) ───────────────────── +/** Profile dir used by headed/connect Chromium sessions. */ +function chromiumProfileDir(): string { + return path.join(process.env.HOME || '/tmp', '.gstack', 'chromium-profile'); +} + +/** Remove Chromium SingletonLock/Socket/Cookie so a relaunch can acquire the + * profile. Safe to call when absent. */ +function cleanChromiumProfileLocks(profileDir: string = chromiumProfileDir()): void { + for (const lockFile of ['SingletonLock', 'SingletonSocket', 'SingletonCookie']) { + safeUnlinkQuiet(path.join(profileDir, lockFile)); + } +} + +/** Kill an orphaned Chromium that still holds the profile's SingletonLock. The + * lock symlink target is "hostname-PID"; killing that PID tears down its + * renderer tree so the next launch starts clean. No-op when absent/stale. */ +async function killOrphanChromium(profileDir: string = chromiumProfileDir()): Promise { + try { + const lockTarget = fs.readlinkSync(path.join(profileDir, 'SingletonLock')); // "hostname-12345" + const orphanPid = parseInt(lockTarget.split('-').pop() || '', 10); + if (orphanPid && isProcessAlive(orphanPid)) { + safeKill(orphanPid, 'SIGTERM'); + await new Promise(r => setTimeout(r, 1000)); + if (isProcessAlive(orphanPid)) { + safeKill(orphanPid, 'SIGKILL'); + await new Promise(r => setTimeout(r, 500)); + } + } + } catch (err: any) { + if (err?.code !== 'ENOENT' && err?.code !== 'EINVAL') throw err; + } +} + +/** Bounded /health probe. Returns true if the server answers within `attempts` + * tries spaced `backoffMs` apart — distinguishes a busy-but-alive daemon from a + * dead one (#1781) so a slow server isn't killed and restarted into a crash-loop. */ +async function probeHealthWithBackoff(port: number, attempts = 3, backoffMs = 250): Promise { + for (let i = 0; i < attempts; i++) { + if (await isServerHealthy(port)) return true; + if (i < attempts - 1) await Bun.sleep(backoffMs); + } + return false; +} + +/** + * Build the env for an auto-restart after a crash. headed/proxy/configHash are + * reapplied from THIS invocation OR the persisted server state, so a restart + * triggered by a plain command (goto/status, no --headed flag) never silently + * downgrades a headed session to headless (#1781). Pure + exported for tests. + */ +export function buildRestartEnv( + globalFlags: GlobalFlags | null | undefined, + oldState: ServerState | null, +): Record { + const env: Record = {}; + if (globalFlags?.proxyUrl) env.BROWSE_PROXY_URL = globalFlags.proxyUrl; + if (globalFlags?.headed || oldState?.mode === 'headed') env.BROWSE_HEADED = '1'; + const configHash = globalFlags?.configHash || oldState?.configHash; + if (configHash) env.BROWSE_CONFIG_HASH = configHash; + return env; +} + +/** macOS only: pull the headed Chromium window to the user's current Space. + * "Google Chrome for Testing" frequently opens behind the active window or on + * another Space — the first thing users read as "I can't see the browser" + * (#1781). Best-effort, fire-and-forget, never throws. The app name is a fixed + * literal (no interpolation). */ +function raiseHeadedWindowMacOS(): void { + if (process.platform !== 'darwin') return; + try { + nodeSpawn('osascript', ['-e', 'tell application "Google Chrome for Testing" to activate'], { + stdio: 'ignore', + detached: true, + }).unref(); + } catch { + // osascript missing or app not present — non-fatal + } +} + // ─── Server Lifecycle ────────────────────────────────────────── async function startServer(extraEnv?: Record): Promise { ensureStateDir(config); @@ -219,6 +299,13 @@ async function startServer(extraEnv?: Record): Promise= 1) throw new Error('[browse] Server unresponsive after retry — aborting'); + console.error('[browse] Server was briefly unresponsive (busy); retrying command...'); + return sendCommand(oldState, command, args, retries + 1); + } + // Truly dead (or health never recovered) → restart. if (retries >= 1) throw new Error('[browse] Server crashed twice in a row — aborting'); console.error('[browse] Server connection lost. Restarting...'); - // Kill the old server to avoid orphaned chromium processes - const oldState = readState(); if (oldState && oldState.pid) { await killServer(oldState.pid); } - // Reapply --proxy / --headed flags from this invocation when restarting - // after a crash. Without this, a proxied daemon that dies mid-command - // would silently restart in default direct/headless mode and bypass - // the SOCKS bridge. - const restartEnv: Record = {}; - if (_globalFlags?.proxyUrl) restartEnv.BROWSE_PROXY_URL = _globalFlags.proxyUrl; - if (_globalFlags?.headed) restartEnv.BROWSE_HEADED = '1'; - if (_globalFlags?.configHash) restartEnv.BROWSE_CONFIG_HASH = _globalFlags.configHash; + // startServer() now clears the Chromium SingletonLock + reaps the orphan, + // so the relaunch isn't blocked by the dead Chromium's profile lock (#1781). + // + // Reapply --proxy / --headed when restarting. headed comes from THIS + // invocation OR the persisted server mode, so a restart triggered by a + // plain command (goto/status, no --headed) never silently downgrades a + // headed session to headless (#1781). Same for proxy/configHash. + const restartEnv = buildRestartEnv(_globalFlags, oldState); const newState = await startServer(Object.keys(restartEnv).length ? restartEnv : undefined); return sendCommand(newState, command, args, retries + 1); } @@ -966,30 +1069,11 @@ Refs: After 'snapshot', use @e1, @e2... as selectors: } } - // Kill orphaned Chromium processes that may still hold the profile lock. - // The server PID is the Bun process; Chromium is a child that can outlive it - // if the server is killed abruptly (SIGKILL, crash, manual rm of state file). - const profileDir = path.join(process.env.HOME || '/tmp', '.gstack', 'chromium-profile'); - try { - const singletonLock = path.join(profileDir, 'SingletonLock'); - const lockTarget = fs.readlinkSync(singletonLock); // e.g. "hostname-12345" - const orphanPid = parseInt(lockTarget.split('-').pop() || '', 10); - if (orphanPid && isProcessAlive(orphanPid)) { - safeKill(orphanPid, 'SIGTERM'); - await new Promise(resolve => setTimeout(resolve, 1000)); - if (isProcessAlive(orphanPid)) { - safeKill(orphanPid, 'SIGKILL'); - await new Promise(resolve => setTimeout(resolve, 500)); - } - } - } catch (err: any) { - if (err?.code !== 'ENOENT' && err?.code !== 'EINVAL') throw err; - } - - // Clean up Chromium profile locks (can persist after crashes) - for (const lockFile of ['SingletonLock', 'SingletonSocket', 'SingletonCookie']) { - safeUnlinkQuiet(path.join(profileDir, lockFile)); - } + // 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 + // lock files so the launch is clean. Shared with the auto-restart path (#1781). + await killOrphanChromium(); + cleanChromiumProfileLocks(); // Delete stale state file safeUnlinkQuiet(config.stateFile); @@ -1027,6 +1111,11 @@ Refs: After 'snapshot', use @e1, @e2... as selectors: }); const status = await resp.text(); console.log(`Connected to real Chrome\n${status}`); + // #1781: surface the window — it often opens behind/on another Space. + raiseHeadedWindowMacOS(); + if (process.platform === 'darwin') { + console.log('(If you still don\'t see it, check Mission Control / other Spaces.)'); + } // sidebar-agent.ts spawn was here. Ripped alongside the chat queue — // the Terminal pane runs an interactive PTY now, no more one-shot @@ -1194,11 +1283,11 @@ Refs: After 'snapshot', use @e1, @e2... as selectors: safeKill(existingState.pid, 'SIGKILL'); } } - // Clean profile locks and state file - const profileDir = path.join(process.env.HOME || '/tmp', '.gstack', 'chromium-profile'); - for (const lockFile of ['SingletonLock', 'SingletonSocket', 'SingletonCookie']) { - safeUnlinkQuiet(path.join(profileDir, lockFile)); - } + // #1781: killing the daemon can orphan its Chromium child tree, which keeps + // holding the SingletonLock and makes the next `connect` fail to launch. + // Reap the orphan via the lock, then clear the lock files + state. + await killOrphanChromium(); + cleanChromiumProfileLocks(); // Xvfb orphan cleanup: if the recorded PID still matches our Xvfb (by // cmdline AND start-time), kill it. PID-only would risk killing a // recycled PID belonging to an unrelated process. @@ -1258,6 +1347,11 @@ Refs: After 'snapshot', use @e1, @e2... as selectors: } await sendCommand(state, command, commandArgs); + + // #1781: `focus` means "show me the window". The server-side focus activates + // the page via CDP, but on macOS the app can still sit on another Space — pull + // it to the user's current Space too. + if (command === 'focus') raiseHeadedWindowMacOS(); } if (import.meta.main) { diff --git a/browse/test/restart-env.test.ts b/browse/test/restart-env.test.ts new file mode 100644 index 000000000..5cf7502e1 --- /dev/null +++ b/browse/test/restart-env.test.ts @@ -0,0 +1,39 @@ +import { describe, test, expect } from "bun:test"; +import { buildRestartEnv } from "../src/cli"; + +// #1781: an auto-restart triggered by a plain command (no --headed flag) must +// NOT silently downgrade a headed session to headless. buildRestartEnv reapplies +// headed/proxy/configHash from this invocation OR the persisted server state. +describe("buildRestartEnv (#1781 headed persistence)", () => { + const headedState = { pid: 1, port: 9, token: "t", startedAt: "", serverPath: "", mode: "headed" as const }; + const launchedState = { pid: 1, port: 9, token: "t", startedAt: "", serverPath: "", mode: "launched" as const }; + + test("headed flag on this invocation → BROWSE_HEADED=1", () => { + expect(buildRestartEnv({ headed: true } as any, null).BROWSE_HEADED).toBe("1"); + }); + + test("plain command + persisted headed state → still BROWSE_HEADED=1 (the regression)", () => { + const env = buildRestartEnv({} as any, headedState as any); + expect(env.BROWSE_HEADED).toBe("1"); + }); + + test("plain command + headless state → no BROWSE_HEADED (no spurious headed)", () => { + const env = buildRestartEnv({} as any, launchedState as any); + expect(env.BROWSE_HEADED).toBeUndefined(); + }); + + test("nothing set → empty env", () => { + expect(buildRestartEnv(null, null)).toEqual({}); + }); + + test("proxy + configHash reapplied from flags", () => { + const env = buildRestartEnv({ proxyUrl: "socks5://x", configHash: "abc" } as any, null); + expect(env.BROWSE_PROXY_URL).toBe("socks5://x"); + expect(env.BROWSE_CONFIG_HASH).toBe("abc"); + }); + + test("configHash falls back to persisted state", () => { + const env = buildRestartEnv({} as any, { ...launchedState, configHash: "fromstate" } as any); + expect(env.BROWSE_CONFIG_HASH).toBe("fromstate"); + }); +});