From 0c87a553c75c42c3c112eb66f203d6785ef2b8a6 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Mon, 19 Oct 2026 14:30:33 -0700 Subject: [PATCH] v1.91.155.0 fix(browse): a busy tab no longer blocks every command with "Daemon busy" (port of #3250) (#3301) * fix(browse): /health status is daemon liveness; active tab reported apart (#3247) /health answered `status` from a page.evaluate('1') race on the active tab, so one page stuck in a long script made the whole daemon read as unhealthy. The CLI gates every command on that answer, and its 2000 ms fetch timeout equalled the server's 2000 ms race, so it could not even read the reply: status, tabs, tab, newtab and stop all failed with "Daemon busy" until --force-restart threw the session away. - BrowserManager.isHealthy() is liveness only (Chromium connected). - probeActiveTab() reports { id, responsive }, bounded by TAB_PROBE_TIMEOUT_MS (1s); /health returns it as `activeTab`. - `status` adds a line naming a not-responding active tab and the commands that leave it. - The CLI reads /health through one helper whose timeout, HEALTH_REQUEST_TIMEOUT_MS (2s), stays above the probe. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Wb6Wntntmwau19fSuSbuaL (cherry picked from commit 322d2553545c15795da7271a6f70122bc36aa561) * fix(browse): bound per-tab reads in `tabs` and `state save` (#3247) A renderer stuck in a long script never answers page.title() or page.evaluate(). `tabs` awaited each title in turn and `state save` read each tab's storage unbounded, so both hung on one busy tab even though neither needs it. - getTabListWithTitles() reads titles in parallel, each bounded by TAB_PROBE_TIMEOUT_MS; a tab that misses it is listed as "(not responding)". - saveState() skips the storage of a tab that does not answer within 5s, the same outcome as a tab whose read fails. busy-active-tab.test.ts holds a real Chromium tab in a busy loop next to an idle one and checks /health, tabs, status, saveState and closetab against it. On main, /health takes 2004 ms and says unhealthy, `tabs` hangs past the test timeout and `status` never names the tab. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Wb6Wntntmwau19fSuSbuaL (cherry picked from commit d010d7b243a7c9039dfb06469d66a193e7e37556) * fix(browse): name the busy tab when a command times out (#3247) A command that needs a stuck tab still waits out the CLI's 30s reply timeout, and the report should say why. It never ran: Bun rejects an AbortSignal.timeout() fetch with TimeoutError, the generic branch only matched AbortError, so a timeout fell through to "[browse] The operation timed out." - isFetchTimeout() accepts both names; the cookie-import branch uses it too. - commandTimeoutReport() reads /health: when `activeTab.responsive` is false it names the tab and points at `browse tab ` and `browse closetab `; a live daemon otherwise keeps the busy-not-restarting line and a dead one the plain timeout line. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Wb6Wntntmwau19fSuSbuaL (cherry picked from commit ef70b7d88094f64c6d57d37191045a83bb7d834c) * docs(browse): /health liveness vs active-tab responsiveness (#3247) BROWSER.md's busy-vs-dead lifecycle note, ARCHITECTURE.md's daemon model and the REMOTE_BROWSER_ACCESS /health reference now say that `status` is daemon liveness, that `activeTab` reports the active tab separately, and which commands keep working while a tab is busy. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Wb6Wntntmwau19fSuSbuaL (cherry picked from commit 3dbb256796cf3c99e687c4622c255f9dca7f8f96) * fix(browse): one pending probe per stuck tab, not one per poll (#3247) Review on #3250: the 1s race stops waiting but leaves page.evaluate() queued in the stuck renderer, so every /health poll (the extension polls every 10s), `status` and `tabs` call added another pending read, all drained at once when the page recovered. Playwright cannot cancel an in-flight evaluate, so the active-tab probe and the `tabs` title read now join the page's pending read of the same kind instead of starting a new one. busy-active-tab.test.ts counts page.evaluate calls across three probes of the stuck tab: 3 before this change, at most 1 after. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Wb6Wntntmwau19fSuSbuaL (cherry picked from commit 19fc9b97093dd954c6f69cbe0b700c25a11ef771) * fix(browse): keep a busy tab's last good storage in saved state (#3247) Review on #3250: with BROWSE_PERSIST_STATE=1 the periodic snapshot used to hang on a stuck tab, so session-state.json kept its last good copy. The 5s storage bound let the snapshot finish and write `storage: null` for that tab over it, so a restart lost its localStorage and sessionStorage even with persistence on. saveState() now records the storage each page answers with, together with its origin. A read that times out returns that last good storage when the origin still matches, and null otherwise. A failed read stays null, as before. The storage read also joins the page's pending read instead of queueing a new one on every persist tick. busy-active-tab.test.ts takes a good snapshot, hangs the tab, and checks that persistSessionState() still writes its localStorage. Live, with BROWSE_PERSIST_STATE=1 and a 2s interval, session-state.json kept {"probe":"kept"} for the busy tab, and its savedAt stayed current through 12s of busy page. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Wb6Wntntmwau19fSuSbuaL (cherry picked from commit 60cbaebee5dd11befa8348212d6a6041b6acf354) * docs(changelog): busy-tab lockup fix from #3250 under [Unreleased] Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --------- Co-authored-by: Andrei Vershinin Co-authored-by: Claude Opus 5.5 Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- ARCHITECTURE.md | 2 +- BROWSER.md | 8 ++ CHANGELOG.md | 11 ++ VERSION | 2 +- agents-digest/gstack-AGENTS.md | 2 +- browse/src/browser-manager.ts | 152 +++++++++++++++++---- browse/src/cli.ts | 98 ++++++++----- browse/src/meta-commands.ts | 7 +- browse/src/routes/core.ts | 8 +- browse/test/busy-active-tab.test.ts | 132 ++++++++++++++++++ browse/test/command-timeout-report.test.ts | 66 +++++++++ browse/test/terminal-agent.test.ts | 7 +- docs/REMOTE_BROWSER_ACCESS.md | 4 +- package.json | 2 +- 14 files changed, 429 insertions(+), 72 deletions(-) create mode 100644 browse/test/busy-active-tab.test.ts create mode 100644 browse/test/command-timeout-report.test.ts diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 4ff86a04f..d9e8082cd 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -91,7 +91,7 @@ The server writes `.gstack/browse.json` (atomic write via tmp + rename, mode 0o6 { "pid": 12345, "port": 34567, "token": "uuid-v4", "startedAt": "...", "binaryVersion": "abc123" } ``` -The CLI reads this file to find the server. If the file is missing or the daemon process is dead, the CLI spawns a new server. A process that is alive but not answering `/health` is busy, not dead: the CLI probes for a bounded ~8s, then reports busy with a nonzero exit — only an explicit `--force-restart` kills a live daemon. Process liveness uses signal-0 (`isProcessAlive`, EPERM counts as alive) on every platform, with the health check (GET /health) as the responsiveness signal. Daemon stdout/stderr persists to `/.gstack/browse-daemon.log`. +The CLI reads this file to find the server. If the file is missing or the daemon process is dead, the CLI spawns a new server. A process that is alive but not answering `/health` is busy, not dead: the CLI probes for a bounded ~8s, then reports busy with a nonzero exit — only an explicit `--force-restart` kills a live daemon. Process liveness uses signal-0 (`isProcessAlive`, EPERM counts as alive) on every platform, with the health check (GET /health) as the responsiveness signal. Its `status` covers the daemon and its Chromium connection only; the active tab's main thread is reported separately (`activeTab.responsive`, a probe bounded to 1s), so one page stuck in a long script never makes the CLI refuse commands. Daemon stdout/stderr persists to `/.gstack/browse-daemon.log`. ### Port selection diff --git a/BROWSER.md b/BROWSER.md index 396bb6e9b..3047da8f5 100644 --- a/BROWSER.md +++ b/BROWSER.md @@ -626,6 +626,14 @@ Storage stays intact by default. With explicit approval on a Chromium target, `- | `tab-each [args...]` | Fan out a command across every open tab; returns JSON | | `frame ` | Switch to iframe context (or back to main); clears refs | +**A busy tab is not a busy daemon.** `/health` `status` is daemon liveness +(Chromium connected); the active tab's main thread is reported separately as +`activeTab: { id, responsive }`, probed for at most 1s. While one page is stuck +in a long script, `status`, `tabs`, `tab `, `newtab`, `closetab`, +`state save` and `stop` keep working, and `status` and `tabs` mark the tab as +not responding. A command that needs the stuck tab times out after 30s and +names it, so you can switch away or close it without `--force-restart`. + ### Extraction | Command | Description | diff --git a/CHANGELOG.md b/CHANGELOG.md index 74a29736d..b6fc8e044 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,16 @@ # Changelog +## [1.91.155.0] - 2026-10-19 + +**A busy page no longer locks up the browse daemon.** When one tab kept its main thread busy (a heavy dev bundle, a big grid render, a runaway script), every `browse` command, including `status`, `tabs`, `tab `, `closetab` and `stop`, failed with "Daemon busy" after about 9.6 s, and the only way out was `--force-restart`, which threw away cookies, tabs and logins. Fixes #3247. From #3250 by @AndreIntelas. + +### Fixed +- **`/health` `status` reports daemon liveness (Chromium connected)**, and the active tab's responsiveness is reported separately as `activeTab: {id, responsive}`. The CLI no longer gates every command on a 2 s `page.evaluate` race against the active tab. +- **`tabs` and `state save` bound their per-tab reads**, so a stuck tab shows as `(not responding)` instead of hanging the command, and one pending probe is shared per stuck tab rather than one per poll. A busy tab keeps its last good storage in saved state. +- **A command that really needs the stuck tab times out with an error naming that tab** and how to get out (switch tabs or close it). The CLI's timeout branch now matches Bun's `TimeoutError`; the old `AbortError` check never fired. + +### Changed +- **Gate integrity:** `browse/test/terminal-agent.test.ts` adds `activeTab` to the exact `/health` keys and updates its stub for the now-synchronous `isHealthy` and the new `probeActiveTab`; its no-token assertions are unchanged. `browse/test/busy-active-tab.test.ts` and `browse/test/command-timeout-report.test.ts` are new. No threshold, trial count or case kind changed. ## [1.91.154.0] - 2026-10-19 **/cso can verify Rails repairs in a read-only checkout, its retained Git history no longer brings back credential files the snapshot leaves out, a repair whose external assertions fail is now reported as `ASSERTION_FAILED` instead of a misleading `INCOMPATIBLE_INPUT`, and a security assertion can accept several safe rejection statuses while still requiring the secret to be absent.** The helper keeps `.env`, `master.key`, keys and agent configuration out of the readable tree, but `history.txt` captured `git log -p` in full, so a committed `.env` came back through its diff whenever the value wasn't credential-shaped enough for the redactor. Paid qualification run 39186328569 caught this when the private corpus's planted `.env` value showed up in the judge's helper state. diff --git a/VERSION b/VERSION index 7836617d0..29c1e5ddc 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.91.154.0 +1.91.155.0 diff --git a/agents-digest/gstack-AGENTS.md b/agents-digest/gstack-AGENTS.md index 4222e853e..4e3b32732 100644 --- a/agents-digest/gstack-AGENTS.md +++ b/agents-digest/gstack-AGENTS.md @@ -1,4 +1,4 @@ -# gstack digest v1.91.154.0 — regenerate/re-copy after upgrading gstack +# gstack digest v1.91.155.0 — regenerate/re-copy after upgrading gstack Behavioral rules from gstack (https://github.com/garrytan/gstack), compressed for agent hosts without a full skill install. The full skills add workflows, diff --git a/browse/src/browser-manager.ts b/browse/src/browser-manager.ts index 338c6dd6b..7ad480813 100644 --- a/browse/src/browser-manager.ts +++ b/browse/src/browser-manager.ts @@ -290,6 +290,44 @@ 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; + +/** + * Budget for saveState()'s per-tab storage read. Longer than the probe: a tab + * that misses it falls back to the last storage it answered with, which may + * be stale, so only a tab that is really stuck takes that path. + */ +const TAB_STORAGE_READ_TIMEOUT_MS = 5_000; + +type TabStorage = { localStorage: Record; sessionStorage: Record }; +type TabReadKind = 'probe' | 'title' | 'storage'; + +function urlOrigin(url: string): string { + return URL.canParse(url) ? new URL(url).origin : ''; +} + +/** + * 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 @@ -300,7 +338,7 @@ export interface BrowserState { pages: Array<{ url: string; isActive: boolean; - storage: { localStorage: Record; sessionStorage: Record } | null; + storage: TabStorage | null; /** * HTML content loaded via load-html (setContent), replayed after context recreation. * In-memory only — never persisted to disk (HTML may contain secrets or customer data). @@ -360,6 +398,13 @@ export class BrowserManager { private extensionContext = false; /** Pending and recorded navigation-guard verdicts per page (D2). */ private navigationGuards = new WeakMap>; blocked: string | null }>(); + /** One pending read per page and kind (#3247). A read that outlives its + * budget stays queued in the renderer until the page frees up, so a stuck + * tab reuses it instead of collecting a new one on every /health poll, + * `tabs` call or persist tick. */ + private pendingTabReads = new WeakMap>>(); + /** The last storage each page answered with, and the origin it came from. */ + private lastTabStorage = new WeakMap(); // Our own 'disconnected' listeners per browser. Detach only these: removing // every 'disconnected' listener also strips Playwright's internal one that // resolves browser.close(), so each close waited the full race timeout (and @@ -1090,20 +1135,43 @@ 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 probe = this.sharedTabRead(page, 'probe', () => page.evaluate('1').then(() => true, () => false)); + return { id, responsive: (await withinTabBudget(probe, timeoutMs)) === true }; + } + + /** Join the page's pending read of this kind, or start one. `read` must not + * reject. */ + private sharedTabRead(page: Page, kind: TabReadKind, read: () => Promise): Promise { + let reads = this.pendingTabReads.get(page); + if (!reads) { + reads = new Map(); + this.pendingTabReads.set(page, reads); } + const pending = reads.get(kind); + if (pending) return pending as Promise; + const started = read().finally(() => reads.delete(kind)); + reads.set(kind, started); + return started; } // ─── Tab Management ──────────────────────────────────────── @@ -1301,17 +1369,23 @@ export class BrowserManager { return released; } - async getTabListWithTitles(): Promise> { - const tabs: Array<{ id: number; url: string; title: string; active: boolean }> = []; - for (const [id, page] of this.pages) { - tabs.push({ + /** + * Every tab with its title. Titles are read in parallel, each bounded by + * TAB_PROBE_TIMEOUT_MS: a tab whose renderer is stuck comes back with + * `busy: true` and an empty title instead of hanging the list (#3247). + */ + async getTabListWithTitles(): Promise> { + return Promise.all([...this.pages].map(async ([id, page]) => { + const title = await withinTabBudget( + this.sharedTabRead(page, 'title', () => page.title().catch(() => '')), TAB_PROBE_TIMEOUT_MS); + return { id, url: page.url(), - title: await page.title().catch(() => ''), + title: title ?? '', active: id === this.activeTabId, - }); - } - return tabs; + busy: title === undefined, + }; + })); } // ─── Session Access ──────────────────────────────────────── @@ -1715,7 +1789,9 @@ export class BrowserManager { // ─── State Save/Restore (shared by recreateContext + handoff) ─ /** * Capture browser state: cookies, localStorage, sessionStorage, URLs, active tab. - * Skips pages that fail storage reads (e.g., already closed). + * Skips the storage of pages that fail the read (e.g., already closed). + * A page whose renderer is stuck past TAB_STORAGE_READ_TIMEOUT_MS does not + * hold up `state save` (#3247); see readTabStorage() for what it gets. */ async saveState(): Promise { if (!this.context) throw new Error('Browser not launched'); @@ -1725,13 +1801,7 @@ export class BrowserManager { for (const [id, page] of this.pages) { const url = page.url(); - let storage = null; - try { - storage = await page.evaluate(() => ({ - localStorage: { ...localStorage }, - sessionStorage: { ...sessionStorage }, - })); - } catch {} + const storage = await this.readTabStorage(page, url); // Capture load-html content so a later context recreation (viewport --scale) // can replay it via setTabContent. Never persisted to disk. @@ -1753,6 +1823,28 @@ export class BrowserManager { return { cookies, pages }; } + /** + * One tab's localStorage/sessionStorage for saveState(). A failed read + * (closed page, opaque origin) is null. A read that times out returns the + * last storage this page gave for the same origin: a periodic + * BROWSE_PERSIST_STATE snapshot must not replace good storage on disk with + * null just because the page is busy. + */ + private async readTabStorage(page: Page, url: string): Promise { + const origin = urlOrigin(url); + const read = await withinTabBudget(this.sharedTabRead(page, 'storage', () => page.evaluate(() => ({ + localStorage: { ...localStorage }, + sessionStorage: { ...sessionStorage }, + })).catch(() => null)), TAB_STORAGE_READ_TIMEOUT_MS); + if (read === undefined) { + const last = this.lastTabStorage.get(page); + return last?.origin === origin ? last.storage : null; + } + if (read) this.lastTabStorage.set(page, { origin, storage: read }); + else this.lastTabStorage.delete(page); + return read; + } + /** * Restore browser state into the current context: cookies, pages, storage. * Navigates to saved URLs, restores storage, wires page events. diff --git a/browse/src/cli.ts b/browse/src/cli.ts index d774d23d5..556947f0d 100644 --- a/browse/src/cli.ts +++ b/browse/src/cli.ts @@ -215,39 +215,50 @@ 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; +} + +/** The active tab's id when /health reports its main thread as not + * responding (#3247), else null — including daemons that predate the field. */ +async function fetchBusyActiveTab(port: number): Promise { + const activeTab = (await fetchDaemonHealth(port))?.activeTab as { id?: unknown; responsive?: unknown } | null | undefined; + return activeTab?.responsive === false && typeof activeTab.id === 'number' ? activeTab.id : null; +} + // ─── Process Management ───────────────────────────────────────── async function killServer(pid: number): Promise { if (!isProcessAlive(pid)) return; @@ -420,7 +431,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; } } @@ -1034,6 +1045,32 @@ export function extractTabId(args: string[]): { tabId: number | undefined; args: return { tabId, args: stripped }; } +/** Did this fetch fail because its AbortSignal fired? Bun rejects an + * AbortSignal.timeout() fetch with TimeoutError, not AbortError — matching + * only AbortError left the timeout reports below unreachable. */ +export function isFetchTimeout(err: unknown): boolean { + const name = (err as { name?: unknown } | null)?.name; + return name === 'AbortError' || name === 'TimeoutError'; +} + +/** + * What to print when a command's reply did not arrive within `timeoutMs`. + * #1781: a timeout on a heavy page usually means busy, not dead, so a live + * daemon is never restarted here. #3247: when /health names the active tab + * as not responding, say which tab and the commands that leave it without + * losing the session. + */ +export async function commandTimeoutReport(state: { pid: number; port: number } | null, timeoutMs: number): Promise { + const after = `[browse] Command timed out after ${timeoutMs / 1000}s`; + if (!state?.pid || !isProcessAlive(state.pid)) return [after]; + const busyTab = await fetchBusyActiveTab(state.port); + if (busyTab === null) return [`${after} (server still alive — busy, not restarting). Retry, or raise load.`]; + return [ + `${after}: tab ${busyTab} is not responding (its main thread is busy).`, + `[browse] The daemon, its other tabs and cookies still work: switch with 'browse tab ', close this one with 'browse closetab ${busyTab}', or retry once the page settles.`, + ]; +} + // ─── Command Dispatch ────────────────────────────────────────── export async function sendCommand(state: ServerState, command: string, args: string[], retries = 0): Promise { // Precedence: CLI --tab-id flag > BROWSE_TAB env var. @@ -1085,19 +1122,14 @@ export async function sendCommand(state: ServerState, command: string, args: str process.exit(1); } } catch (err: any) { - if (command === 'cookie-import-browser' && (['AbortError', 'TimeoutError'].includes(err.name) + if (command === 'cookie-import-browser' && (isFetchTimeout(err) || ['ECONNREFUSED', 'ECONNRESET'].includes(err.code) || err.message?.includes('fetch failed'))) { throw new Error('Cookie import response was lost or timed out. It may have partially completed; inspect the destination before retrying manually. The command was not replayed.'); } - if (err.name === 'AbortError') { - // #1781: a 30s timeout on a heavy page usually means busy, not dead. - // Don't kill a live server (that's what triggered the crash-loop) — report - // and exit so the user can retry rather than losing their (headed) window. - const ts = readState(); - const alive = ts?.pid ? isProcessAlive(ts.pid) : false; - console.error(alive - ? '[browse] Command timed out after 30s (server still alive — busy, not restarting). Retry, or raise load.' - : '[browse] Command timed out after 30s'); + if (isFetchTimeout(err)) { + // Report and exit so the user can retry rather than losing their + // (headed) window — killing a live server triggered the crash-loop. + for (const line of await commandTimeoutReport(readState(), timeoutMs)) console.error(line); process.exit(1); } // Connection error — server may have crashed, OR may just be busy. diff --git a/browse/src/meta-commands.ts b/browse/src/meta-commands.ts index 8cc9e2e6c..af49910a9 100644 --- a/browse/src/meta-commands.ts +++ b/browse/src/meta-commands.ts @@ -265,7 +265,7 @@ export async function handleMetaCommand( case 'tabs': { const tabs = await bm.getTabListWithTitles(); return tabs.map(t => - `${t.active ? '→ ' : ' '}[${t.id}] ${t.title || '(untitled)'} — ${t.url}` + `${t.active ? '→ ' : ' '}[${t.id}] ${t.busy ? '(not responding)' : t.title || '(untitled)'} — ${t.url}` ).join('\n'); } @@ -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/busy-active-tab.test.ts b/browse/test/busy-active-tab.test.ts new file mode 100644 index 000000000..04a15861b --- /dev/null +++ b/browse/test/busy-active-tab.test.ts @@ -0,0 +1,132 @@ +/** + * #3247: one busy tab must not make the daemon look dead. A renderer stuck in + * a long script never answers page.evaluate() or page.title(). /health folded + * that into its `status`, so the CLI refused every command (status, tabs, + * tab, newtab, stop) with "Daemon busy" until --force-restart threw the + * session away, and `tabs` and `state save` hung on the tab as well. + * + * A real Chromium tab is held in a busy loop while a second tab stays idle; + * each surface must answer within its bound and name the busy tab, without + * piling up reads on the stuck renderer or dropping its persisted storage. + */ +import { afterAll, beforeAll, describe, expect, test } from 'bun:test'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import type { Page } from 'playwright'; +import { BrowserManager, TAB_PROBE_TIMEOUT_MS } from '../src/browser-manager'; +import { HEALTH_REQUEST_TIMEOUT_MS } from '../src/cli'; +import { handleMetaCommand } from '../src/meta-commands'; +import { persistSessionState } from '../src/session-persist'; +import { callRoute, stubRouteContext } from './route-test-harness'; +import { startTestServer } from './test-server'; + +// Outlasts every check below; closing the busy tab at the end stops it sooner. +const BUSY_MS = 30_000; + +let testServer: ReturnType; +let bm: BrowserManager; +let busyId = 0; +let idleId = 0; +let busyPage: Page; +let profileDir = ''; +const originalProfile = process.env.CHROMIUM_PROFILE; + +beforeAll(async () => { + // Per-file profile, set in a hook (see commands.test.ts for why). + profileDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-busy-tab-profile-')); + process.env.CHROMIUM_PROFILE = profileDir; + testServer = startTestServer(0); + bm = new BrowserManager(); + await bm.launch(); + busyId = bm.getActiveTabId(); + await bm.getPage().goto(`${testServer.url}/basic.html`); + idleId = await bm.newTab(`${testServer.url}/basic.html`); + bm.switchTab(busyId, { bringToFront: false }); + busyPage = bm.getPage(); + // The good snapshot a BROWSE_PERSIST_STATE tick takes before the page hangs. + await busyPage.evaluate(() => localStorage.setItem('probe', 'kept')); + await bm.saveState(); + await busyPage.evaluate((ms) => { + setTimeout(() => { const end = Date.now() + ms; while (Date.now() < end) { /* busy */ } }, 50); + }, BUSY_MS); + await Bun.sleep(300); // let the loop start +}, 30_000); + +afterAll(async () => { + try { testServer.server.stop(true); } catch {} + try { await Promise.race([bm?.close(), Bun.sleep(3000)]); } catch {} + if (originalProfile === undefined) delete process.env.CHROMIUM_PROFILE; + else process.env.CHROMIUM_PROFILE = originalProfile; + if (profileDir) fs.rmSync(profileDir, { recursive: true, force: true }); +}); + +describe('busy active tab (#3247)', () => { + test('/health says the daemon is alive and names the busy tab before the CLI gives up', async () => { + // Equal budgets were the bug: the CLI aborted just before the reply landed. + expect(TAB_PROBE_TIMEOUT_MS).toBeLessThan(HEALTH_REQUEST_TIMEOUT_MS); + const ctx = stubRouteContext({ + browserManager: bm, + terminal: { readPort: () => null, grantToken: async () => true, restartSession: async () => true }, + }); + const started = Date.now(); + const body = await (await callRoute('GET', '/health', ctx)).json(); + expect(Date.now() - started).toBeLessThan(HEALTH_REQUEST_TIMEOUT_MS); + expect(body.status).toBe('healthy'); + expect(body.activeTab).toEqual({ id: busyId, responsive: false }); + }); + + test('tabs lists every tab without waiting on the busy one', async () => { + const started = Date.now(); + const out = await handleMetaCommand('tabs', [], bm, async () => {}); + expect(Date.now() - started).toBeLessThan(HEALTH_REQUEST_TIMEOUT_MS); + expect(out).toContain(`[${busyId}] (not responding)`); + expect(out).toContain(`[${idleId}] Test Page - Basic`); + }); + + test('status stays healthy and names the busy tab with a way out', async () => { + const out = await handleMetaCommand('status', [], bm, async () => {}); + expect(out).toContain('Status: healthy'); + expect(out).toContain(`Active tab ${busyId}`); + expect(out).toContain(`closetab ${busyId}`); + }); + + test('a stuck tab gets one pending probe, not one per poll', async () => { + let evaluates = 0; + const evaluate = busyPage.evaluate.bind(busyPage); + Object.assign(busyPage, { evaluate: (...args: Parameters) => { evaluates++; return evaluate(...args); } }); + try { + for (let i = 0; i < 3; i++) expect(await bm.probeActiveTab()).toEqual({ id: busyId, responsive: false }); + } finally { + delete (busyPage as { evaluate?: unknown }).evaluate; // back to the prototype method + } + // The /health check above may already hold the pending probe. + expect(evaluates).toBeLessThanOrEqual(1); + }, 10_000); + + test('a persisted snapshot taken while the tab is busy keeps its last good storage', async () => { + const file = path.join(profileDir, 'session-state.json'); + await persistSessionState(bm, file); + const saved = JSON.parse(fs.readFileSync(file, 'utf8')); + expect(saved.pages).toHaveLength(2); + expect(saved.pages.find((p: { isActive: boolean }) => p.isActive).storage.localStorage).toEqual({ probe: 'kept' }); + expect(saved.pages.find((p: { isActive: boolean }) => !p.isActive).storage).not.toBeNull(); + }, 15_000); + + test('the idle tab still answers', async () => { + bm.switchTab(idleId, { bringToFront: false }); + try { + expect(await bm.probeActiveTab()).toEqual({ id: idleId, responsive: true }); + expect(await bm.getPage().title()).toBe('Test Page - Basic'); + } finally { + bm.switchTab(busyId, { bringToFront: false }); + } + }); + + test('closetab closes the busy tab and the session carries on', async () => { + const out = await handleMetaCommand('closetab', [String(busyId)], bm, async () => {}); + expect(out).toContain(`Closed tab ${busyId}`); + expect(bm.getTabCount()).toBe(1); + expect(await bm.probeActiveTab()).toEqual({ id: idleId, responsive: true }); + }, 15_000); +}); diff --git a/browse/test/command-timeout-report.test.ts b/browse/test/command-timeout-report.test.ts new file mode 100644 index 000000000..0505c0737 --- /dev/null +++ b/browse/test/command-timeout-report.test.ts @@ -0,0 +1,66 @@ +/** + * What the CLI prints when a command's reply never arrives (#1781, #3247). + * + * Bun rejects an AbortSignal.timeout() fetch with TimeoutError, so the old + * AbortError-only check skipped the busy report and printed "The operation + * timed out." instead. When the daemon's /health names its active tab as not + * responding, the report names that tab and the commands that leave it + * without a restart; a dead daemon gets the plain line. + */ +import { afterEach, describe, expect, test } from 'bun:test'; +import { commandTimeoutReport, isFetchTimeout } from '../src/cli'; + +let servers: Array> = []; +afterEach(() => { for (const s of servers) s.stop(true); servers = []; }); + +/** A fake daemon whose /health answers `body`. */ +function fakeHealth(body: unknown): number { + const srv = Bun.serve({ port: 0, hostname: '127.0.0.1', fetch: () => Response.json(body) }); + servers.push(srv); + if (!srv.port) throw new Error('fake /health: no port'); + return srv.port; +} + +/** A pid that no longer runs: a child that has already exited. */ +function deadPid(): number { + return Bun.spawnSync([process.execPath, '-e', '']).pid; +} + +describe('isFetchTimeout', () => { + test('a real fetch whose AbortSignal.timeout fires counts as a timeout', async () => { + const silent = Bun.serve({ port: 0, hostname: '127.0.0.1', fetch: () => new Promise(() => {}) }); + servers.push(silent); + const err = await fetch(`http://127.0.0.1:${silent.port}/`, { signal: AbortSignal.timeout(50) }) + .then(() => null, (e: unknown) => e); + expect(isFetchTimeout(err)).toBe(true); + expect(isFetchTimeout(new TypeError('fetch failed'))).toBe(false); + }); +}); + +describe('commandTimeoutReport', () => { + test('names the busy tab and the way out, never a restart', async () => { + const port = fakeHealth({ status: 'healthy', activeTab: { id: 7, responsive: false } }); + const out = (await commandTimeoutReport({ pid: process.pid, port }, 30_000)).join('\n'); + expect(out).toContain('tab 7'); + expect(out).toContain('closetab 7'); + expect(out).not.toContain('--force-restart'); + }); + + test('a live daemon with a responsive or unreported tab gets the busy-not-restarting line', async () => { + for (const body of [ + { status: 'healthy', activeTab: { id: 1, responsive: true } }, + { status: 'healthy' }, // daemon from before activeTab existed + ]) { + const out = (await commandTimeoutReport({ pid: process.pid, port: fakeHealth(body) }, 30_000)).join('\n'); + expect(out).toContain('still alive'); + expect(out).not.toContain('closetab'); + } + }); + + test('a dead daemon gets only the timeout line', async () => { + const port = fakeHealth({ status: 'healthy', activeTab: { id: 7, responsive: false } }); + const out = await commandTimeoutReport({ pid: deadPid(), port }, 30_000); + expect(out).toHaveLength(1); + expect(out[0]).toContain('timed out after 30s'); + }); +}); 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'); }); diff --git a/docs/REMOTE_BROWSER_ACCESS.md b/docs/REMOTE_BROWSER_ACCESS.md index c5565ba0e..902df6efb 100644 --- a/docs/REMOTE_BROWSER_ACCESS.md +++ b/docs/REMOTE_BROWSER_ACCESS.md @@ -81,7 +81,9 @@ Response: (plain text result of the command) ``` #### GET /health -Server status. No auth required. Returns status, tabs, mode, uptime. +Server status. No auth required. Returns status, activeTab, tabs, mode, uptime. +`status` is daemon liveness; `activeTab` (`{ id, responsive }`, or null with no +page) says whether the active tab's main thread answered within 1s. Never carries a token — extension token bootstrap is `POST /extension-token` (local listener only, validates the pinned `chrome-extension://` Origin and a loopback Host; 403 otherwise). Not reachable over the tunnel (404). diff --git a/package.json b/package.json index e11883f58..de7b7c9d4 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "gstack", - "version": "1.91.154", + "version": "1.91.155", "description": "Garry's Stack — Claude Code skills + fast headless browser. One repo, one install, entire AI engineering workflow.", "license": "MIT", "type": "module",