diff --git a/browse/src/browser-manager.ts b/browse/src/browser-manager.ts index 338c6dd6b..419299859 100644 --- a/browse/src/browser-manager.ts +++ b/browse/src/browser-manager.ts @@ -290,6 +290,30 @@ export async function handleChromiumDisconnect(browser: Browser | null): Promise process.exit(1); } +/** + * How long a tab-independent read waits on one tab's renderer (#3247). A page + * stuck in a long script never answers page.evaluate() or page.title(); before + * this bound, one such tab made /health call the whole daemon unhealthy and + * hung `tabs`. Kept well under the CLI's /health request timeout + * (HEALTH_REQUEST_TIMEOUT_MS in cli.ts) so the CLI reads the answer instead of + * timing out on it. + */ +export const TAB_PROBE_TIMEOUT_MS = 1_000; + +/** + * Resolve with `work`'s value, or with undefined once `ms` passes. The losing + * read settles on its own when the renderer frees up, so callers pass a promise + * that cannot reject. + */ +function withinTabBudget(work: Promise, ms: number): Promise { + let timer: ReturnType | undefined; + const deadline = new Promise((resolve) => { + timer = setTimeout(() => resolve(undefined), ms); + (timer as { unref?: () => void }).unref?.(); + }); + return Promise.race([work, deadline]).finally(() => clearTimeout(timer)); +} + export type { RefEntry }; // Re-export TabSession for consumers @@ -1090,20 +1114,28 @@ export class BrowserManager { this.closeOwnedDisplay(); } - /** Health check — verifies Chromium is connected AND responsive */ - async isHealthy(): Promise { - if (!this.browser || !this.browser.isConnected()) return false; - try { - const page = this.pages.get(this.activeTabId); - if (!page) return true; // connected but no pages — still healthy - await Promise.race([ - page.evaluate('1'), - new Promise((_, reject) => setTimeout(() => reject(new Error('timeout')), 2000)), - ]); - return true; - } catch { - return false; - } + /** + * Daemon liveness for /health: Chromium is connected. The active tab is NOT + * part of it (#3247): a page stuck in a long script is one busy tab, not a + * dead daemon, and the CLI gates every command on this answer — including + * the tabs, tab, closetab and stop commands that get out of that state. + * probeActiveTab() reports the tab on its own. + */ + isHealthy(): boolean { + return Boolean(this.browser?.isConnected()); + } + + /** + * Does the active tab's main thread answer within `timeoutMs`? Null when + * there is no active page. A rejected evaluate (crashed or closing target) + * also counts as not responsive. + */ + async probeActiveTab(timeoutMs = TAB_PROBE_TIMEOUT_MS): Promise<{ id: number; responsive: boolean } | null> { + const id = this.activeTabId; + const page = this.pages.get(id); + if (!page) return null; + const answered = await withinTabBudget(page.evaluate('1').then(() => true, () => false), timeoutMs); + return { id, responsive: answered === true }; } // ─── Tab Management ──────────────────────────────────────── diff --git a/browse/src/cli.ts b/browse/src/cli.ts index d774d23d5..120f90f76 100644 --- a/browse/src/cli.ts +++ b/browse/src/cli.ts @@ -215,39 +215,43 @@ function readState(): ServerState | null { // isProcessAlive is imported from ./error-handling -/** - * HTTP health check — definitive proof the server is alive and responsive. - * Used in all polling loops instead of isProcessAlive() (which is slow on Windows). - */ -export async function isServerHealthy(port: number, timeoutMs = 2000): Promise { - try { - const resp = await fetch(`http://127.0.0.1:${port}/health`, { - signal: AbortSignal.timeout(timeoutMs), - }); - if (!resp.ok) return false; - const health = await resp.json() as any; - return health.status === 'healthy'; - } catch { - return false; - } -} +/** Client timeout for one GET /health. Must stay above the daemon's active-tab + * probe (TAB_PROBE_TIMEOUT_MS in browser-manager.ts): when the two were equal, + * the CLI aborted just before a busy tab's reply arrived and could not tell a + * busy tab from a hung daemon (#3247). */ +export const HEALTH_REQUEST_TIMEOUT_MS = 2000; -/** Best-effort tab count via GET /health (no auth, bounded). Returns null - * when the daemon doesn't answer in time or predates the `tabs` field — - * callers degrade to a countless phrasing, never block on this. */ -async function fetchDaemonTabCount(port: number, timeoutMs = 2000): Promise { +/** GET /health (no auth, bounded). Null when the daemon doesn't answer in + * time or the reply is not a 200. */ +async function fetchDaemonHealth(port: number, timeoutMs = HEALTH_REQUEST_TIMEOUT_MS): Promise | null> { try { const resp = await fetch(`http://127.0.0.1:${port}/health`, { signal: AbortSignal.timeout(timeoutMs), }); if (!resp.ok) return null; - const health = await resp.json() as any; - return typeof health.tabs === 'number' ? health.tabs : null; + return await resp.json() as Record; } catch { return null; } } +/** + * HTTP health check — definitive proof the server is alive and responsive. + * Used in all polling loops instead of isProcessAlive() (which is slow on Windows). + * `status` is daemon liveness only; a busy active tab does not fail it (#3247). + */ +export async function isServerHealthy(port: number, timeoutMs = HEALTH_REQUEST_TIMEOUT_MS): Promise { + return (await fetchDaemonHealth(port, timeoutMs))?.status === 'healthy'; +} + +/** Best-effort tab count via GET /health. Returns null when the daemon + * doesn't answer in time or predates the `tabs` field — callers degrade to a + * countless phrasing, never block on this. */ +async function fetchDaemonTabCount(port: number): Promise { + const tabs = (await fetchDaemonHealth(port))?.tabs; + return typeof tabs === 'number' ? tabs : null; +} + // ─── Process Management ───────────────────────────────────────── async function killServer(pid: number): Promise { if (!isProcessAlive(pid)) return; @@ -420,7 +424,7 @@ async function probeHealthWithBackoff( await Bun.sleep(intervalMs); const remainingMs = deadline - Date.now(); if (remainingMs <= 0) return false; - if (await isServerHealthy(port, Math.min(2000, remainingMs))) return true; + if (await isServerHealthy(port, Math.min(HEALTH_REQUEST_TIMEOUT_MS, remainingMs))) return true; } } diff --git a/browse/src/meta-commands.ts b/browse/src/meta-commands.ts index 8cc9e2e6c..d79ee91b4 100644 --- a/browse/src/meta-commands.ts +++ b/browse/src/meta-commands.ts @@ -404,12 +404,17 @@ export async function handleMetaCommand( case 'status': { const tabs = bm.getTabCount(); const mode = bm.getConnectionMode(); + const activeTab = await bm.probeActiveTab(); return [ `Status: healthy`, `Mode: ${mode}`, `URL: ${bm.getCurrentUrl()}`, `Tabs: ${tabs}`, `PID: ${process.pid}`, + // #3247: name the busy tab and the ways out that keep the session. + ...(activeTab && !activeTab.responsive + ? [`Active tab ${activeTab.id}: not responding. Switch with: browse tab , or close it: browse closetab ${activeTab.id}`] + : []), ].join('\n'); } diff --git a/browse/src/routes/core.ts b/browse/src/routes/core.ts index 491fc5f76..611448594 100644 --- a/browse/src/routes/core.ts +++ b/browse/src/routes/core.ts @@ -77,13 +77,19 @@ export const coreRoutes: RouteEntry[] = [ // Health check — no auth required, does NOT reset idle timer. NEVER carries // a token in any mode: token bootstrap is POST /extension-token and shell // auth is POST /pty-session. Liveness/status only. + // + // `status` is daemon liveness (Chromium connected); the CLI gates every + // command on it. `activeTab.responsive` is the active tab's main thread, + // probed for at most TAB_PROBE_TIMEOUT_MS and reported beside it: one busy + // page must not read as a dead daemon (#3247). { method: '*', path: '/health', auth: 'none', surfaces: ['local'], handler: async (_req, _r, ctx) => { const { browserManager } = ctx; - const healthy = await browserManager.isHealthy(); + const healthy = browserManager.isHealthy(); return json({ status: healthy ? 'healthy' : 'unhealthy', + activeTab: healthy ? await browserManager.probeActiveTab() : null, mode: browserManager.getConnectionMode(), uptime: Math.floor((Date.now() - ctx.startTime) / 1000), tabs: browserManager.getTabCount(), diff --git a/browse/test/terminal-agent.test.ts b/browse/test/terminal-agent.test.ts index f066d1a23..c5b247ac1 100644 --- a/browse/test/terminal-agent.test.ts +++ b/browse/test/terminal-agent.test.ts @@ -94,13 +94,16 @@ describe('/health does NOT surface ptyToken', () => { test('/health response body does not include ptyToken', async () => { // It's allowed to include `terminalPort` (a port number, not auth). const ctx = stubRouteContext({ - browserManager: { isHealthy: async () => true, getConnectionMode: () => 'launched', getTabCount: () => 1 } as any, + browserManager: { + isHealthy: () => true, probeActiveTab: async () => ({ id: 1, responsive: true }), + getConnectionMode: () => 'launched', getTabCount: () => 1, + } as any, terminal: { readPort: () => 4242, grantToken: async () => true, restartSession: async () => true }, }); const text = await (await callRoute('GET', '/health', ctx)).text(); const body = JSON.parse(text); expect(body.terminalPort).toBe(4242); - expect(Object.keys(body).sort()).toEqual(['mode', 'status', 'tabs', 'terminalPort', 'uptime']); + expect(Object.keys(body).sort()).toEqual(['activeTab', 'mode', 'status', 'tabs', 'terminalPort', 'uptime']); expect(text).not.toContain('ptyToken'); expect(text).not.toContain('gstack_pty'); });