v1.43.3.0 fix(browse): resolveDisconnectCause crashes on persistent-context disconnect#1659
Open
garrytan wants to merge 6 commits into
Open
v1.43.3.0 fix(browse): resolveDisconnectCause crashes on persistent-context disconnect#1659garrytan wants to merge 6 commits into
garrytan wants to merge 6 commits into
Conversation
…indirection Module-level idleCheckTick, parent watchdog, SIGTERM handler, and buildFetchHandler's onDisconnect wire all read the module-level BrowserManager directly. For embedders (gbrowser) that pass their own instance into buildFetchHandler, the module-level instance never has launchHeaded() called on it — connectionMode stays 'launched' forever, headed-mode early-returns never fire, and after 30 min of HTTP idle the server self-terminates out from under the overlay. Adds `let activeBrowserManager: BrowserManager` at module scope (symmetric with the existing `let activeShutdown` pattern). buildFetchHandler retargets it at cfg.browserManager and CHAINS cfg.browserManager.onDisconnect to activeShutdown, preserving any caller-installed handler instead of clobbering it. Six edit sites in browse/src/server.ts: - Edit 1 (~705): declare activeBrowserManager - Edit 2 (~596): extract idleCheckTick + __testInternals__ export - Edit 3 (~658): parent watchdog reads activeBrowserManager - Edit 4 (~1387): retarget + chain cfgBrowserManager.onDisconnect - Edit 5 (verify): line 714 default stays in place - Edit 6 (~1212): SIGTERM handler reads activeBrowserManager
…rally Adds 5 behavioral tests to browse/test/server-factory.test.ts under a new 'idle timer + onDisconnect dual-instance fix' describe block: - T1 (CRITICAL — REGRESSION): headed embedder does not auto-shutdown at idle. Pins the bug this PR fixes. - T2 (paired defensive): headless still auto-shuts down at idle. Catches a future refactor that breaks the inverse case. - T3 (chain semantics): buildFetchHandler chains cfgBrowserManager.onDisconnect, preserving any caller-set handler. Uses .rejects.toThrow for the async shutdown path. - T4 (tunnelActive): tunnel-active blocks idle-shutdown even in headless mode. - T5 (static guard): exactly 3 module-level lifecycle sites use activeBrowserManager.getConnectionMode() — idleCheckTick, parent watchdog, SIGTERM. Catches refactor-introduced regressions before CI. Reuses existing makeMinimalConfig() + __resetRegistry() patterns from the factory contract tests. New makeMockBrowserManager() helper. beforeEach also resets module state via setTunnelActive, setLastActivity, and resetShutdownState from __testInternals__. Also deletes the old 'idle check skips in headed mode' string-grep test from browse/test/sidebar-ux.test.ts at line 1596. That test would have passed even with the dual-instance bug present (grepped for "=== 'headed'" + 'return' in the same window). Behavioral coverage moved to server-factory.test.ts. Verified: 33/33 tests pass in browse/test/server-factory.test.ts.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…wser # Conflicts: # CHANGELOG.md # VERSION
…wser # Conflicts: # CHANGELOG.md # VERSION
…connect
`browser?.process()` in headed mode reaches a BrowserContext-owned Browser
stub whose `.process` is undefined (not a function). The optional-chain
`browser?.process()` does NOT short-circuit on undefined methods — only
on null/undefined receivers — so it evaluates to `undefined()` and throws
an unhandled rejection. The throw crashes the bun process, gbd respawns
it, the next tab close hits the same path, loop forever.
Reproducer (live in gbrowser amsterdam-v7 right now):
[overlay] Local listener bound on 127.0.0.1:35300 (PID: 19445)
[browse] Tab closed (id=1, remaining=0)
[stderr] [overlay] FATAL unhandled rejection: browser?.process is
not a function. (In 'browser?.process()', 'browser?.process'
is undefined)
[browse] Shutting down...
...respawn, same crash, repeat...
Fix: split the null case from the no-process case.
- null browser → 'crash' (preserves the existing contract pinned by the
"null browser returns crash" test)
- truthy browser without callable .process → 'clean' (persistent contexts
in headed mode; the user controls the lifecycle so the right default
is exit 0 / gbd does not restart)
- truthy browser with callable .process → unchanged exit-code introspection
In headed mode we genuinely cannot distinguish "user pressed Cmd+Q or
closed all tabs" from "Chromium crashed" because Playwright doesn't
expose the underlying Chromium PID through a persistent context. The
tradeoff is: if Chromium genuinely crashes in headed mode we now exit 0
and don't auto-restart. That's preferable to the respawn loop, and the
user can re-launch manually if they want.
Test: added "clean: browser without .process method (persistent context)"
which would have caught this bug. All 21 browser-manager-unit tests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
github-actions
Bot
changed the title
E2E Evals: ✅ PASS8/8 tests passed | $1.40 total cost | 12 parallel runners
12x ubicloud-standard-8 (Docker: pre-baked toolchain + deps) | wall clock ≈ slowest suite |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
browse/src/browser-manager.ts—resolveDisconnectCauseis called on every Playwrightdisconnectedevent. In headed mode the only launcher we use ischromium.launchPersistentContext, which exposes a Browser object whose.processis undefined (Playwright doesn't surface the underlying Chromium process for persistent contexts).Pre-fix:
```ts
const proc = browser?.process();
```
The optional chain `browser?.process` short-circuits to `undefined` only when `browser` itself is null/undefined. With a truthy browser and a missing method, the expression evaluates to `undefined()` and throws an unhandled rejection. The throw crashes the bun process; downstream supervisors (gbrowser's `gbd`) respawn it; the next tab close hits the same path; loop forever.
Live evidence captured in `gbrowser/amsterdam-v7`'s `~/Library/Logs/GBrowser/workspaces//browse-server.log`:
```
[overlay] Local listener bound on 127.0.0.1:35300 (PID: 19445)
[browse] Tab closed (id=1, remaining=0)
[stderr] [overlay] FATAL unhandled rejection: browser?.process is not a function.
(In 'browser?.process()', 'browser?.process' is undefined)
[browse] Shutting down...
...respawn, same crash, repeat...
```
Fix
Split the null case from the no-callable-process case:
In headed mode we genuinely can't distinguish "user pressed Cmd+Q or closed all tabs" from "Chromium crashed" because Playwright doesn't expose the underlying Chromium PID through a persistent context. The tradeoff: if Chromium genuinely crashes in headed mode we now exit 0 and don't auto-restart. That's preferable to the respawn loop, and the user can re-launch manually.
Test plan
🤖 Generated with Claude Code