Skip to content

v1.40.0.2 fix(browse): Cmd+Q on managed Chromium stops triggering supervisor respawn#1626

Open
garrytan wants to merge 1 commit into
mainfrom
garrytan/clean-quit-no-restart
Open

v1.40.0.2 fix(browse): Cmd+Q on managed Chromium stops triggering supervisor respawn#1626
garrytan wants to merge 1 commit into
mainfrom
garrytan/clean-quit-no-restart

Conversation

@garrytan
Copy link
Copy Markdown
Owner

Summary

Three `browser.on('disconnected')` handlers in `browse/src/browser-manager.ts` exited with non-zero codes for every disconnect — including user-initiated Cmd+Q. Process supervisors that consume our exit code (gbrowser's `gbd` HealthMonitor) treated this as a crash and respawned the whole stack on exponential backoff. Result: user closes the browser, window pops back 1s later, then 2s, then 4s.

This PR adds `resolveDisconnectCause(browser)` that reads the underlying ChildProcess's `exitCode` + `signalCode` (waiting up to 1s for the exit event if `disconnected` fires first). The three handlers consume it:

  • `launch()` (headless): clean → 0, crash → 1 (was always 1)
  • `launchHeaded()` (headed): clean → 0, crash → 2 (preserved). `onDisconnect()` cleanup still fires in both cases.
  • `handoff()` (mid-session re-launch): clean → 0, crash → 1.

Crash-recovery is preserved: SIGSEGV / SIGKILL / OOM / non-zero exits all still exit non-zero and supervisors restart with backoff. Only the clean Cmd+Q path is new.

Test plan

  • `bun test browse/test/browser-manager-unit.test.ts` — 9 tests pass (7 new for resolver + 2 existing)
  • `bun test browse/test/browser-manager-custom-chromium.test.ts` — still green
  • Manual: macOS, launch headed Chromium via `bun run dev`, Cmd+Q the window, verify CLI exits 0 (`echo $?`)
  • Manual: SIGKILL the Chromium PID, verify CLI exits 2 (headed) so supervisor still restarts

Related

🤖 Generated with Claude Code

…ervisor respawn

Three browser.on('disconnected') handlers in browse/src/browser-manager.ts
(launch, launchHeaded, handoff) each exited with a non-zero code on every
disconnect, regardless of cause. Process supervisors that consume our exit
code (gbrowser's gbd HealthMonitor in cmd/gbd/health.go) treated user
Cmd+Q identical to a Chromium crash and respawned with exponential
backoff, so the visible browser kept reappearing after the user closed it.

Add resolveDisconnectCause(browser) that reads the underlying ChildProcess
exitCode + signalCode (waiting up to 1s for the exit event if the
disconnected event fired first). Exit code 0 + no signal = clean user
quit; anything else = crash, signal-kill, or OOM.

Wire the resolver into all three disconnect handlers:
- launch() (headless): clean → exit 0, crash → exit 1 (was always 1)
- launchHeaded() (headed): clean → exit 0, crash → exit 2 (was always 2)
  onDisconnect() cleanup callback still runs in both cases.
- handoff() (re-launch): same as launch() via the helper.

Preserve the per-path crash codes (1 vs 2) so any supervisor that
differentiated headed vs headless crashes keeps working.

Seven new unit tests in browse-manager-unit.test.ts cover the resolver
across already-exited, signal-killed (SIGSEGV / SIGKILL), async exits,
and null-browser inputs.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@garrytan
Copy link
Copy Markdown
Owner Author

Bundled into #1629 as the v1.42.1.1 fix wave, along with PR #1617 and an additional exit-code propagation fix that Codex caught: this PR's launchHeaded() disconnect handler computes exitCode correctly but server.ts:684 hardcoded activeShutdown?.(2), dropping the resolved 0 before it could reach the shutdown pipeline. The bundled PR widens BrowserManager.onDisconnect to accept exitCode, threads it through launchHeaded (this.onDisconnect(exitCode)), and updates the server.ts hookup to (code) => activeShutdown?.(code ?? 2). New regression test pins the full propagation path.

Leaving this PR open until #1629 merges so we have a working fallback. Will close with a final "merged via #1629" once landed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant