fix: subagent_panel root eviction + browser_close cleanup hang

[AndrewTilson]

Summary

Two independent bugs discovered together during a live-repro debug session, both with the same root-cause shape: an unbounded await masquerading as bounded, guarded by except Exception: pass (which catches raised exceptions but not hangs).

The two subsystems are unrelated (subagent_panel vs. browser_manager), so this could reasonably be split into two PRs — happy to do that if reviewers prefer. Kept as one for now because the root-cause pattern is instructive across both.

---

Part 1 — subagent_panel/state.py idle-prune evicts live root rows

Bug. state.snapshot() silently deletes live root sub-agent rows whose only crime is a slow tool call. The stale-row filter reads:

if not e.get("done") \
        and s not in busy_parents \
        and e.get("parent") not in ids \   # <-- trivially True when parent is None
        and now - e["last_seen"] > IDLE_PRUNE_S:
    del _AGENTS[s]

For a root row (parent=None) with no live children, all four conditions hold after IDLE_PRUNE_S seconds (default 600s / 10 min) of stream-event silence. The row is dropped from _AGENTS with no _handle_frozen call, no completion signal, and no scrollback trace. Any sibling that did complete cleanly is stranded in state as done=True with no flush trigger — _maybe_flush_group is only invoked from _handle_frozen.

Visible symptom: in high-output mode, a sub-agent panel shows a " completed" row that never scrolls up into transcript history, and the whole session appears to hang.

Fix. Add and e.get("parent") is not None to the stale-row filter. Root rows now exit only via completion, cancel, or end-of-turn state.clear(). Orphan non-root rows (child whose parent has vanished from _AGENTS) are still eligible for idle-prune — the original safety net is intact. Docstring rewritten to state the invariant.

Two new tests in tests/plugins/subagent_panel/test_state_invariants.py:

• test_snapshot_does_not_idle_prune_stale_root
• test_snapshot_still_prunes_stale_orphan_child

---

Part 2 — browser_manager.py _cleanup() has unbounded awaits

Bug. BrowserManager._cleanup() had four sequential await calls (storage_state(), context.close(), browser.close(), and now playwright.stop() — see below) with no timeouts. Each was wrapped in try/except Exception: pass, which does nothing for hangs. Playwright's context.close() in particular can wait indefinitely on unresponsive service workers, unhandled beforeunload prompts, or stale CDP WebSockets (e.g. laptop slept mid-run). Real-world 10+ minute hangs observed.

Fix, three layers:

1. Bounded awaits. Every cleanup step wrapped in asyncio.wait_for with env-configurable per-step timeouts:

   Env var	Default
   BROWSER_CLEANUP_STATE_TIMEOUT_S	10s
   BROWSER_CLEANUP_CONTEXT_TIMEOUT_S	10s
   BROWSER_CLEANUP_BROWSER_TIMEOUT_S	5s
   BROWSER_CLEANUP_PW_TIMEOUT_S	5s

   Worst-case _cleanup drops from unbounded to ~30s.

2. Driver-process kill fallback. If playwright.stop() times out, SIGKILL the node driver subprocess via _playwright._impl_obj._connection._transport._proc. Verified against Playwright 1.61.0 by live introspection — Browser has no process attribute in the Python bindings, but the driver DOES expose an asyncio.subprocess.Process. Killing the driver transitively reaps the browser child process, which is the only reliable escape hatch when CDP is completely unresponsive. Private-API walk is best-effort: any hop that goes missing → silent no-op, no raise.

3. Preventive dialog dismisser. Before context.close(), attach page.on("dialog", lambda d: asyncio.ensure_future(d.dismiss())) on every open page. Unhandled beforeunload prompts now auto-dismiss instead of blocking teardown. Per-page failures are swallowed (a closed page raises when you touch it).

Bonus fixes bundled in

• Latent driver-leak bug. The Playwright instance returned by async_playwright().start() in _initialize_browser was assigned to a local variable and never .stop()'d — the node driver leaked until Python GC eventually caught it. Now stored as self._playwright and torn down properly on cleanup. This is prerequisite for the driver-kill fallback anyway, so it fell out naturally.
• _env_float rejects inf/nan/non-finite values via math.isfinite, so BROWSER_CLEANUP_*_TIMEOUT_S=inf can't silently reintroduce unbounded awaits. Explicit test covers inf, -inf, nan, Infinity.
• silent=True is now honored uniformly across all emit paths — previously the outer catch-all leaked emit_warning during shutdown.

Five new tests in tests/tools/browser/test_browser_manager.py:

• test_cleanup_returns_within_timeout_when_context_close_hangs
• test_cleanup_kills_playwright_driver_when_stop_times_out (mirrors real private-attribute chain hop-for-hop, so a Playwright upgrade that moves the path fails loudly instead of silently no-opping)
• test_cleanup_installs_dialog_handler_on_pages
• test_cleanup_silent_flag_suppresses_all_emits
• test_env_float_parses_valid_positive_float / _falls_back_on_garbage / _falls_back_on_non_positive / _rejects_infinite_and_nan

Also fixed a latent mock_page.on fixture bug (it was an AsyncMock where real Playwright's page.on is sync) that was producing "coroutine was never awaited" warnings in unrelated tests. Bonus quiet.

---

Design principles honored

• DRY: timeout policy centralized in module-level constants + one _env_float helper. silent handling aliased to no-op lambdas once rather than sprinkling if not silent: at every emit site.
• YAGNI: did NOT add psutil as a dep, did NOT rewrite atexit/signal handling, did NOT parallelize cleanup_all_browsers. All valid future work; none needed for this bug.
• SOLID: every change is drop-in behind the existing _cleanup / snapshot contract. No caller signatures moved.
• Zen of Python: errors never pass silently (they emit or get suppressed by explicit silent=True); in the face of ambiguity, we refuse the temptation to guess (inf env var → fall back to default).

---

Test results

Focused (touched files only):

tests/plugins/subagent_panel/test_state_invariants.py  5 passed
tests/tools/browser/test_browser_manager.py           42 passed

Broader sweep (tests/tools/browser/ + tests/plugins/subagent_panel/):

423 passed, 2 skipped

Zero new warnings introduced (fewer overall thanks to the mock_page.on fixture fix).

---

Notes for reviewers

• If you'd prefer two smaller PRs (Part 1 vs Part 2), say the word and I'll split.
• The _playwright._impl_obj._connection._transport._proc walk is admittedly ugly, but I've verified it against Playwright 1.61.0 and it's the only path to a real subprocess handle from the Python API. If it drifts in a future Playwright release we degrade to a no-op rather than crash. The included test locks the shape in.
• Two independent code reviews (Claude 4.7 Opus + GPT-5.4) both flagged the original blocker before I fixed the API path — they made me verify against a live probe. Included that as a test to keep future me honest.