mirror of https://github.com/garrytan/gstack.git
fix(browse): resolveDisconnectCause crashes on persistent-context disconnect
`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>
This commit is contained in:
parent
bacc4c9066
commit
b66d8b35fe
|
|
@ -88,7 +88,22 @@ export function shouldEnableChromiumSandbox(): boolean {
|
|||
* restarts on backoff.
|
||||
*/
|
||||
export async function resolveDisconnectCause(browser: Browser | null): Promise<'clean' | 'crash'> {
|
||||
const proc = browser?.process();
|
||||
// Null browser → 'crash' (defensive default, matches existing contract
|
||||
// pinned by the unit test "null browser returns crash").
|
||||
if (browser === null) return 'crash';
|
||||
|
||||
// Persistent contexts (headed mode) expose a Browser object via
|
||||
// BrowserContext.browser(), but Playwright doesn't surface the underlying
|
||||
// Chromium process through it — `browser.process` is undefined on that
|
||||
// object. Pre-fix, `browser?.process()` evaluated to `undefined()` and
|
||||
// threw an unhandled rejection, crashing the bun process and putting gbd
|
||||
// into a respawn loop. Without process introspection we can't distinguish
|
||||
// "user pressed Cmd+Q or closed all tabs" from "Chromium crashed". In
|
||||
// headed mode the user controls the lifecycle, so the right default is
|
||||
// 'clean': exit 0, gbd does not restart, user re-launches if they want.
|
||||
if (typeof browser.process !== 'function') return 'clean';
|
||||
|
||||
const proc = browser.process();
|
||||
if (proc && proc.exitCode === null && proc.signalCode === null) {
|
||||
await new Promise<void>((resolve) => {
|
||||
const timer = setTimeout(resolve, 1000);
|
||||
|
|
|
|||
|
|
@ -190,6 +190,20 @@ describe('resolveDisconnectCause', () => {
|
|||
const { resolveDisconnectCause } = await import('../src/browser-manager');
|
||||
expect(await resolveDisconnectCause(null)).toBe('crash');
|
||||
});
|
||||
|
||||
// Regression: persistent contexts in headed mode expose a Browser stub
|
||||
// whose `.process` is undefined (not a function). Pre-fix, calling
|
||||
// `browser?.process()` on such a value threw an unhandled rejection
|
||||
// ("browser?.process is not a function"), crashed the bun process, and
|
||||
// put gbd into a respawn loop on every tab close. The contract for that
|
||||
// case is: default to 'clean' so gbd exits 0 and the user keeps lifecycle
|
||||
// control. Live evidence in gbrowser amsterdam-v7's browse-server.log
|
||||
// before this fix landed.
|
||||
it('clean: browser without .process method (persistent context)', async () => {
|
||||
const { resolveDisconnectCause } = await import('../src/browser-manager');
|
||||
const fake = {} as never; // truthy, no .process → would have thrown pre-fix
|
||||
expect(await resolveDisconnectCause(fake)).toBe('clean');
|
||||
});
|
||||
});
|
||||
|
||||
// ─── onDisconnect exit-code propagation (regression test) ──────────
|
||||
|
|
|
|||
Loading…
Reference in New Issue