fix(browser): close automation children on exit

[keweichen]

What changed, and why

src/browser.mjs called process.exit(1) from inside its catch block, which bypassed finally; it also had no SIGINT/SIGTERM handlers or whole-command timeout. A failed or interrupted browser command could therefore leave its Chrome process tree alive.

This change:

• gives every non-interactive command a 45-second overall limit, with a bounded --timeout=<ms> override;
• handles errors, overall timeout, SIGINT, and SIGTERM without bypassing cleanup;
• closes the Playwright page, persistent context, and browser independently and idempotently;
• closes a persistent context that finishes launching after cancellation;
• adds deterministic fake-Playwright regressions for error, timeout, late launch, SIGINT, and SIGTERM, while retaining the real Chrome smoke test.

Review first

1. src/browser.mjs — bounded lifecycle and cleanup ordering
2. tests/browser-persistent.test.py — subprocess-level exit/cleanup assertions
3. tests/fixtures/fake-playwright.mjs — deterministic hanging/error/late-launch fixture

User behavior / bug proved

Browser QA can fail, time out, or be interrupted without leaving owner-owned headless Chrome roots behind. Successful commands retain the existing text/action/screenshot behavior.

Documentation consistency

docs/built-in-tools.md documents the default limit, override, and cleanup contract. docs/catalog.json advances that canonical document's last_verified date to 2026-08-09. Navigation is unchanged, so docs/README.md needs no change.

Before / after evidence

Parent implementation deaaaf50 with the initial lifecycle regressions applied:

$ python3 tests/browser-persistent.test.py \
    PersistentBrowserTests.test_error_closes_page_context_and_browser \
    PersistentBrowserTests.test_overall_timeout_closes_page_context_and_browser \
    PersistentBrowserTests.test_interrupts_close_page_context_and_browser
FFFF
FAIL: test_error_closes_page_context_and_browser
AssertionError: 'page.close' not found in ['context.launch', 'page.goto']
FAIL: test_overall_timeout_closes_page_context_and_browser
AssertionError: 13 != 1
FAIL: test_interrupts_close_page_context_and_browser (signal=SIGINT)
AssertionError: 13 != 130
FAIL: test_interrupts_close_page_context_and_browser (signal=SIGTERM)
AssertionError: 13 != 143
FAILED (failures=4)

HEAD aa6d04c7d4475dbf216caf682915754da441dd64:

$ python3 tests/browser-persistent.test.py
........
Ran 8 tests in 1.811s
OK

$ npx eslint src/browser.mjs tests/fixtures/browser-playwright-*.mjs tests/fixtures/fake-playwright.mjs
(no output)

$ python3 -m py_compile tests/browser-persistent.test.py
(no output)

$ git diff --check origin/main...HEAD
(no output)

The late-launch regression proves cleanup completes inside the original command deadline. Additional regressions prove an over-cap timeout is rejected and a second signal retains Node's normal immediate-termination behavior.

Review follow-up

Addressed Sonichi's 7a4704c4 cold-review findings in aa6d04c7:

• reserve cleanup time inside the original command deadline and pass the remaining operation budget to Playwright launch;
• adopt and close a context that completes launching during that bounded cleanup window;
• detach signal handlers on the first signal so a second signal is never swallowed;
• reject --timeout values above 300,000 ms rather than silently clamping them.

Real managed-browser lifecycle proof

Using the exact HEAD wrapper, a fresh temporary profile per run, and an outer Python subprocess timeout:

success_exit=0
success_headless_roots=0
error_exit=1
error_headless_roots=0
timeout_exit=1
timeout_headless_roots=0
sigint_exit=130
sigint_headless_roots=0
sigterm_exit=143
sigterm_headless_roots=0

The error case used a missing selector after Chrome launched; the timeout case interrupted a pending wait; the signal cases interrupted a 30-second page wait. Every audit was scoped to the current macOS UID. No private files or another user's processes were inspected or terminated.

Tests and checks

• python3 tests/browser-persistent.test.py — 8 passed (includes real Chrome smoke)
• exact-head real Chrome success/error/timeout/SIGINT/SIGTERM matrix — expected exits, zero owner headless roots after every case
• npx eslint src/browser.mjs tests/fixtures/*.mjs — passed
• python3 -m py_compile tests/browser-persistent.test.py — passed
• python3 -m json.tool docs/catalog.json — passed
• git diff --check — passed
• added-lines hardcoded-path scan — clean with positive control
• current-head CI and CLA are the merge gate; no bypass is requested

Edge cases checked

• navigation/action error after a browser exists
• whole-command timeout while a page operation is pending
• timeout before persistent-context launch finishes
• SIGINT exit code 130
• SIGTERM exit code 143
• resource close calls remain independent if a parent resource already closed a child
• normal real-Chrome text action and persistent profile

Worst-case disruption and mitigation

The new default timeout could end an unusually long non-interactive action sequence at 45 seconds. Callers that intentionally need longer can use --timeout=<ms> up to five minutes; over-cap values are rejected instead of silently clamped. Interactive setup intentionally has no overall timeout because the user must finish sign-in and close the visible window; signals and errors still use the same cleanup path.

Stack, migration, config, permissions, rollback

Not stacked. No migration, durable config, new permission, or cross-user process handling. The timeout flag is additive; existing commands receive a finite default. Rollback is a single commit revert.

[github-actions]

Coverage Gate

❌ Test suite failed under instrumentation — coverage not measurable. See the job log.

[sonichi]

Cold review at 7a4704c4, 17/17 green. The core fix is right and worth saying so precisely: replacing process.exit(1) with process.exitCode is what actually lets finally run, and closing page/context/browser through an idempotent closeQuietly with a memoized cleanupPromise is the correct shape. The fake-Playwright fixture earns its keep — the late-launch case is the one a real-Chrome test could never make deterministic.

Two findings, both in the new finally, and they compound.

1. The documented 45s bound can be exceeded by up to 30s.

docs/built-in-tools.md now states "Non-interactive commands are bounded to 45 seconds by default." But after the timeout rejects the race, finally does:

if (!context && launchPromise) {
  const launchedContext = await launchPromise.catch(() => null);

That await is bounded only by Playwright's own launch timeout, Math.min(operationTimeoutMs, 30000) — which runs in addition to the command timeout that already fired. A launch still pending at t=45s can hold the process to t≈75s. The bound the docs promise is on the operation, not on the command; those differ by up to the launch timeout.

If the intent is a real command-level bound, the launch await needs its own deadline derived from the remaining budget rather than a fresh 30s.

2. After the first signal, SIGINT and SIGTERM are swallowed until cleanup finishes.

The handler is:

const handler = () => {
  if (receivedSignal) return;
  ...
};

and handlers are detached on the last line of finally, after both awaits. So from the moment the first Ctrl-C lands until cleanup completes, every subsequent SIGINT/SIGTERM hits a registered no-op handler — and a registered handler suppresses Node's default terminate. The operator's escape hatch is gone precisely when they are most likely to reach for it, and finding 1 can stretch that window to ~30s.

Worth deciding explicitly: either let the second signal process.exit() immediately (accepting a possibly-orphaned Chrome, which is the state this PR exists to prevent), or keep swallowing it but bound the window hard so it cannot outlast a few seconds. Both are defensible; silently unkillable-for-30s is the one that isn't.

Nit. Math.min(..., 300000) clamps silently, so --timeout=600000 runs for 5 minutes with no diagnostic, while a malformed value gets a clear error. Rejecting an over-cap value the same way the regex rejects a malformed one would be more consistent than quietly halving it.

Not gating on any of this — no approval from me either way, since my gh credential is the owner's identity. Flagging for whoever does approve.

[github-actions]

@cla-assistant check

[bassilkhilo-ag2]

Verified in an isolated worktree:

• python3 tests/browser-persistent.test.py -v at HEAD: 7/7 pass (1 real-Chrome smoke test skipped — playwright not installed on this host, expected).
• Reverted only src/browser.mjs to origin/main while keeping the PR's new tests: 6 failures + 1 error, including the exact ones the PR body cites ('page.close' not found, signal handling not reaching cleanup, the --timeout= cap not being enforced) — confirms both the defect and that the new tests actually pin the fix rather than passing vacuously.
• npx eslint src/browser.mjs: clean.
• Read the change: catch-block process.exit(1) bypassing finally was the root cause of the leak; the fix routes errors, the 45s overall timeout, and SIGINT/SIGTERM all through the same cleanup path, and closes page/context/browser independently and idempotently — including the late-launch-after-cancellation case, which is the one that's easy to miss (a persistent context that finishes launching after the command already decided to exit).

Well-scoped, evidence matches the diff, before/after reproduced independently. Approving.

[sonichi]

Cold review at head f7473387. I verified the defect and the fix by running them rather than reading them; details below so you can check my work.

The defect is real at the merge-base. deaaaf50:src/browser.mjs:

121:  process.exit(1);
122: } finally {

The exit sits inside the catch, one line above the finally, so the cleanup block is unreachable on the error path. That matches the PR body exactly.

The new tests fail in the broken state — which is the part worth knowing. I reverted only src/browser.mjs to the merge-base inside a worktree, kept the PR's tests, and re-ran:

tests/browser-persistent.test.py
  at head f7473387            8 tests, OK (skipped=1)
  with merge-base browser.mjs FAILED (failures=6, errors=1, skipped=1)

Seven of eight fail on the old source. That is the assertion actually failing in the broken state, so these are regressions rather than tests that would pass either way. Restored the file afterwards; git diff clean.

SIGINT -> 130 and SIGTERM -> 143 are the conventional 128 + signum, so a supervisor reading exit codes will classify these correctly.

One nit, not a blocker — the real-Chrome case does not run anywhere automated.

test_headless_action_uses_persistent_profile ... skipped 'playwright dependency not installed'
grep -rli playwright .github/workflows/   ->  no matches

So the "retained real Chrome smoke test" skips on this host and in CI, and the deterministic fake-Playwright fixtures are carrying the entire suite. That is a reasonable trade — they clearly work, per the control above — but worth one line in the PR body, otherwise a green matrix later reads as "real browser verified" when nothing exercised a real browser. Same shape as the live-cadence point on #109: a gate that runs only on your machine should say so.

What I did not verify: I did not send a real SIGINT/SIGTERM to a live Chrome process tree, and I did not test the 45s deadline against a genuinely hanging real browser — the fixtures simulate both, so my evidence covers the logic and not the OS-level teardown. Also BEHIND, and I would hold the update-branch until #2782 lands, or you will import the currently-stale docs/src-map.md from main and pick up three unrelated red checks.

No formal approval from me — approvals and merges here are the owner's call.