Skip to content

v1.42.1.1 fix wave: browse launch hardening (2 bug fixes + headed exit-code wiring)#1629

Open
garrytan wants to merge 1 commit into
mainfrom
garrytan/tel-aviv-v3
Open

v1.42.1.1 fix wave: browse launch hardening (2 bug fixes + headed exit-code wiring)#1629
garrytan wants to merge 1 commit into
mainfrom
garrytan/tel-aviv-v3

Conversation

@garrytan
Copy link
Copy Markdown
Owner

Summary

Bundles two open browse launch-path bug fix PRs into one fix-wave landing at v1.42.1.1, plus the missing exit-code wiring that Codex caught in PR #1626 (which made the second fix not actually work end-to-end without this change).

Bundled source PRs (left open as fallbacks until this merges):

Companion (gbrowser side): garrytan/gbrowser#23 — gbd's HealthMonitor reads the exit code this PR now correctly emits.

What this fixes for the user

  1. Yellow --no-sandbox infobar on every headed Chromium launch on macOS and Linux dev — gone. All three launch sites (launch, launchHeaded / launchPersistentContext, handoff) now share shouldEnableChromiumSandbox() so Playwright stops auto-adding --no-sandbox when the sandbox is actually wanted.
  2. gbrowser respawn loop after Cmd+Q — gone. Headed clean quit now exits the browse server with code 0; gbd's HealthMonitor treats it as user intent and skips the restart. Crash semantics preserved: SIGKILL / SIGSEGV / OOM still exit 2 (headed) / 1 (headless + handoff) so supervisors still restart on backoff.

Codex-caught fix (the difference between this PR and just rebasing #1626)

PR #1626 as written doesn't actually fix the headed-mode respawn bugbrowse/src/server.ts:684 hardcodes browserManager.onDisconnect = () => activeShutdown?.(2) and the locally-computed clean exit code (0) inside launchHeaded() was dropped on the floor by this.onDisconnect() having no parameter.

This PR adds the missing wire:

  • BrowserManager.onDisconnect signature widened to ((exitCode?: number) => void | Promise<void>) | null
  • launchHeaded() disconnect handler now calls this.onDisconnect(exitCode)
  • browse/src/server.ts:688(code) => activeShutdown?.(code ?? 2) (the ?? 2 preserves legacy crash semantics)
  • New regression test in browse/test/browser-manager-unit.test.ts pins the full propagation contract so a refactor that drops the forward fails CI before the user-visible respawn bug returns.

Tests

  • bun test browse/test/browser-manager-unit.test.ts → 17 tests, all green (up from 2 baseline; 6 for shouldEnableChromiumSandbox, 7 for resolveDisconnectCause, 2 for onDisconnect(exitCode) propagation)
  • Manual macOS verification deferred to canary post-merge.

Test plan

  • bun test browse/test/browser-manager-unit.test.ts — 17/17 pass
  • Full E2E eval suite on CI
  • Manual macOS: bun run dev launch headed, Cmd+Q, echo $? reports 0
  • Manual macOS: kill -9 <chromium pid>, echo $? reports 2
  • Post-merge canary via /land-and-deploy

🤖 Generated with Claude Code

…t-code wiring)

Bundles two browse launch-path bug fixes plus the missing exit-code wiring
that made the second fix actually work end-to-end.

PR #1617 — Chromium sandbox policy at all 3 launch sites
- shouldEnableChromiumSandbox() centralizes the Win32 / CI / CONTAINER /
  root heuristic that previously lived only in the headless launch path.
- launch(), launchHeaded() / launchPersistentContext(), and handoff() now
  share the policy so Playwright stops auto-adding --no-sandbox on every
  headed launch and the yellow "unsupported command-line flag" infobar
  disappears on macOS and Linux dev.

PR #1626 — clean Cmd+Q stops triggering supervisor respawn
- resolveDisconnectCause(browser) reads the underlying Chromium
  ChildProcess exitCode + signalCode (with a 1s wait for an async exit
  event) to distinguish clean user-quit from crash.
- handleChromiumDisconnect(browser) dispatches the headless launch()
  disconnect path: clean → exit(0), crash → exit(1).
- launchHeaded() disconnect handler resolves cause inline and computes
  exitCode = 0 (clean) | 2 (crash) before forwarding to onDisconnect.
- handoff() disconnect handler uses the same shared helper.

Codex-caught propagation fix (this commit, not in either source PR)
- BrowserManager.onDisconnect signature widened to accept an exitCode
  argument. Without this, launchHeaded's locally-computed exit code was
  dropped before reaching server.ts.
- browse/src/server.ts:688 — onDisconnect callback now forwards the
  resolved code: (code) => activeShutdown?.(code ?? 2). The ?? 2
  preserves legacy crash semantics for callers that invoke onDisconnect
  without an explicit code.

Tests
- browse/test/browser-manager-unit.test.ts goes from 2 → 17 tests.
- 6 new tests pin shouldEnableChromiumSandbox across darwin / linux /
  win32 / CI / CONTAINER / root.
- 7 new tests pin resolveDisconnectCause across already-exited,
  async-exit, SIGSEGV, SIGKILL, and null-browser.
- 2 new tests (this commit) pin the onDisconnect(exitCode) propagation
  contract including the exact server.ts forwarding callback shape so a
  refactor that drops the forward fails CI before the user-visible
  respawn bug returns.

Refs PRs #1617, #1626; companion gbrowser PR #23.
@github-actions
Copy link
Copy Markdown

E2E Evals: ✅ PASS

7/7 tests passed | $1.47 total cost | 12 parallel runners

Suite Result Status Cost
e2e-browse 2/2 $0.13
e2e-deploy 2/2 $0.32
e2e-qa-workflow 1/1 $0.5
llm-judge 1/1 $0.02
e2e-qa-workflow 1/1 $0.5

12x ubicloud-standard-8 (Docker: pre-baked toolchain + deps) | wall clock ≈ slowest suite

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