fix(browser): browser tools unusable after Hermes restart — zombie daemon holds port - #65701
fix(browser): browser tools unusable after Hermes restart — zombie daemon holds port#65701LiteSoul wants to merge 1 commit into
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment
Overview
- Browser daemon zombie process retry + session cleanup
- 593 additions, 4 deletions
Assessment
- Fix adds EADDRINUSE retry logic for zombie daemon port conflicts
- Session cleanup (
~/.agent-browser) is a reasonable cleanup strategy - No obvious security issues
Note
reap ~/.agent-browser sessions— this clears browser state on retry; ensure this won't cause data loss for active browser sessions
Reviewed by Hermes Agent
|
Thanks for the review @tonydwb — happy to clarify the safety property here, since the framing in the original description could read as "clears browser state on retry" but that's not what happens. Short answerNo active browser session is touched. The retry path only fires when the daemon has already failed to start — a daemon that can't bind its port never reached the point of opening Chromium, so there's no browser context to lose. Why each piece is safe
|
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the detailed Windows investigation. The synchronous retry is a plausible salvage direction, but the new app-directory sweep has a blocking ownership problem.
Problems
tools/browser_tool.py:1726-1745terminates any live~/.agent-browserdaemon that passes identity and session-name binding. The PR documents these as direct CLI sessions without anowner_pid(tools/browser_tool.py:1687-1695), so those checks cannot prove the daemon is orphaned or Hermes-owned. The added test intentionally terminates that live daemon attests/tools/test_browser_orphan_reaper.py:586-611.- The stated v0.17.1 idle-timeout premise is stale on current main:
scripts/install.sh:2480installsagent-browser@^0.26.0, and284e084bcc06decc5c1eab0855731cdf7169c38fwired daemon idle timeout.
Suggested changes
- Drop the app-directory reaper unless it can establish dead-Hermes ownership before
_terminate_host_pid; add a regression that a live direct CLI daemon survives. - Revalidate the Windows repro on the current dependency, then retain the scoped retry only if still needed.
This is an automated hermes-sweeper review.
| # Live PID — verify it's genuinely an agent-browser daemon | ||
| # before killing it. For app-dir sessions there is no socket | ||
| # dir, so we pass the app dir itself as the binding context. | ||
| if not _verify_reapable_browser_daemon( |
There was a problem hiding this comment.
Blocking: this guard proves the PID is an agent-browser daemon for this session name, but not that it is orphaned or Hermes-owned. App-directory sessions have no owner_pid; a live direct CLI daemon therefore reaches _terminate_host_pid. Please exclude app-dir sessions or add reliable dead-owner provenance before killing.
There was a problem hiding this comment.
Resolved in 9351efd: dropped the app-dir reaper entirely. You were right on both counts — explicitly acknowledged below.
Blocking ownership concern (the inline line comment)
Resolved by deletion, not by tightening the guard. The whole _reap_app_dir_sessions code path is removed from this iteration of the PR — _agent_browser_app_dir, _reap_app_dir_sessions, the AGENT_BROWSER_SESSION fallback inside _verify_reapable_browser_daemon, the two _reap_app_dir_sessions() call sites in _reap_orphaned_browser_sessions, and the _try_unlink helper that only those sites used.
_reap_orphaned_browser_sessions now only globs agent-browser-h_* / agent-browser-cdp_* / agent-browser-hermes_* socket dirs in the system tmpdir — exactly as it did before this PR. Direct CLI (app-dir) daemons live in ~/.agent-browser/, which by design is outside that glob, so the retry core's inline call to that reaper also cannot reach them.
The regression test you asked for: TestAppDirSessionsAreLeftAlone::test_live_app_dir_daemon_survives_reaper in tests/tools/test_browser_orphan_reaper.py. It stages a .agent-browser/live_session.pid pointing at PID 4242, mocks the identity guard to (deliberately) permit reap, calls _reap_orphaned_browser_sessions(), and asserts 4242 not in terminate_calls and terminate_calls == [] and the .pid file remains "4242". The structural call-site removal is what makes this pass — the reaper code path never reaches the file at all.
Stale v0.17.1 premise — and a separate upstream gap I found while revalidating
Also right on the version premise. Revalidating wasn't a no-op — it surfaced a separate install-flow bug that I'll file as its own PR shortly (linking back here). Honest findings:
Repro on this Windows machine, this session:
browser_navigatereturned"Daemon failed to start (port: 127.0.0.1:55478)"with a live zombiedaemon.js(PID 39924) atC:\Users\LiteSoul\AppData\Roaming\nvm\v24.13.1\node_modules\agent-browser\dist\daemon.js— agent-browser v0.17.1. Killed, retry succeeded.- The Hermes process's PATH includes
C:\Users\LiteSoul\AppData\Local\hermes\node\bin(the bundled prefixInstall-AgentBrowserinscripts/install.ps1L355-364 populates vianpm install -g --prefix $HERMES_HOME\node "agent-browser@^0.26.0"), but that directory does not exist on disk — so bare-PATH wins and resolves to NVM's 0.17.1.
Why this isn't just my machine being weird: I traced the install/update path.
- The desktop "Update" button in
apps/desktop/electron/bootstrap-runner.ts(L779) drivesinstall.ps1per-stage viainstall.ps1 -Stage <name> -NonInteractive -Json, iterating stages fetched frominstall.ps1 -Manifest. install.ps1's stage list$InstallStages(L3501-3527) has 13 stages:uv, python, git, node, system-packages, repository, venv, dependencies, node-deps, [desktop if -IncludeDesktop], path, config-templates, platform-sdks, bootstrap-marker, configure, gateway. There is nobrowser/agent-browserstage.Install-AgentBrowser(L355) is invoked only byInvoke-PostInstallMode(-PostInstallflag) orInvoke-EnsureMode -Deps "browser". Neither is in the auto-driven stage sequence. So a Windows user installing/updating Hermes exclusively via the desktop app gets theiragent-browserfrom whatever global PATH provides, not from the^0.26.0the install.ps1 code presumably intends.- For this Windows desktop population, your
284e084bccidle-timeout upgrade genuinely doesn't apply — they're not on 0.26.0. The zombie problem persists as the production reality, not as a legacy edge.
I did not directly verify the repro on a healthily-installed 0.26.0 on this machine — that would need a separate install.ps1 -PostInstall run + force-restart repro. But the install-flow gap above already explains the 0.17.1 steady-state for any desktop-driven Windows install, so the implication for this PR is:
- The retry path here is the correct fix for the production reality of the Windows-desktop population — not merely defense-in-depth for SIGKILL / Hyper-V (feat: allow custom endpoints to use responses api #1041) races on 0.26.0. The framing in your review ("retain retry only if still needed") stays correct modulo the separate install-flow bug being open: until a
browserstage reaches the install manifest, the retry path is the only thing standing between desktop-driven Windows users and the same broken-tool-symptom on every Hermes restart. - The fix scope of
#65701itself is unchanged: app-dir reaper deleted (Blocking concern resolved),TestAppDirSessionsAreLeftAloneregression pinned as you asked, scoped retry retained.
The PR body's stale AGENT_BROWSER_IDLE_TIMEOUT_MS paragraph is dropped; the body now explicitly mentions the install-flow gap.
I'll post the new PR for the browser stage addition separately and link it back here so the desktop owners have the discovery trail. Both of my earlier top-level comments on this thread are superseded by this commit. Thanks for the review — your "revalidate on current dep" ask is what surfaced the install-flow bug, which is materially more useful to the project than the original PR revision was alone.
When Hermes is force-killed (taskkill, power loss, app close) the agent-browser daemon on Windows holds its deterministic TCP port as an orphan. The next Hermes session's first browser command hits EADDRINUSE and surfaces as ``"Daemon failed to start (port: ...)"`` — the browser tool becomes completely unusable until the user finds and kills the zombie node.exe in Task Manager. Reproduced live on Windows: ``browser_navigate`` returned ``"Daemon failed to start (port: 127.0.0.1:55478)"`` with a zombie ``daemon.js`` (PID 39924) holding the port. After killing the zombie, ``browser_navigate`` succeeded. See PR NousResearch#65701 for full transcript. ## What this iteration does (per teknium1's review) Two scoped changes only: 1. **Retry on daemon-start failure** — when ``_run_browser_command`` detects ``"Daemon failed to start"``, it synchronously runs ``_reap_orphaned_browser_sessions`` (kills Hermes-managed zombies only), resets the session so the retry gets a fresh ``h_<uuid>`` port, and retries the command exactly once. ``_daemon_retried`` parameter prevents recursion beyond one attempt. The retry path only fires *after* the daemon has failed to start — there is no active browser session to lose; a daemon that can't bind its port never opened Chromium. 2. **Dropped the app-directory sweep entirely** — the original submission also swept ``~/.agent-browser/`` for zombies from direct CLI invocations outside Hermes. teknium1 marked this Blocking on review: those sessions have no ``owner_pid`` file the guard can prove ownership through, so identity+session-name binding only proves the PID is an agent-browser daemon for that session *name*, not that it is orphaned or Hermes-owned. A live ``agent-browser open`` in a user's terminal would be terminated by the next Hermes restart. The new regression test ``TestAppDirSessionsAreLeftAlone::test_live_app_dir_daemon_survives_reaper`` pins the removal. The retry calls ``_reap_orphaned_browser_sessions`` unchanged — it only globs ``agent-browser-h_*``/``-cdp_*``/``-hermes_*`` socket dirs in the system tmp dir. Direct CLI (app-dir) sessions live in ``~/.agent-browser/`` which is intentionally outside that glob, so the retry can never reach them either. ## Revalidation on current main teknium1's second ask was to revalidate the Windows repro on the upgraded dependency (``agent-browser@^0.26.0``, commit 284e084 which wired daemon idle-timeout). Honest findings: - On this Windows machine, ``agent-browser`` resolves via bare PATH to the user-managed NVM install at 0.17.1 — the Hermes-managed bundled prefix ``$HERMES_HOME/node/bin/agent-browser`` was never populated (``ensure_browser`` in ``scripts/install.sh`` did not run successfully for this user). The idle-timeout commit does NOT apply to a 0.17.1 daemon. The zombie-on-restart problem still reproduces here, exactly as the PR describes. - A correctly-installed Hermes that ran ``ensure_browser`` should have 0.26.0 (via ``npm install -g --prefix "$HERMES_HOME/node"``) prepended to PATH, and the daemon idle-timeout should reduce zombie incidence in the steady state. The retry path therefore becomes defense-in-depth for two remaining edge cases: (a) the failed-install edge case (this machine), where 0.17.1 is still spawned; and (b) races where a busy daemon is SIGKILLed or hits a Hyper-V port conflict (NousResearch#1041 per the upstream commit) before idle-timeout fires. I did NOT directly verify the repro on a healthily-installed 0.26.0; if the maintainers want that, it requires a fully-clean ``hermes setup --ensure browser`` run on this machine and a force-restart repro, which I can attempt but haven't done. Flagging the gap rather than claiming false-green. ## Tests ``tests/tools/test_browser_orphan_reaper.py`` — 32 tests: - ``TestAppDirSessionsAreLeftAlone`` (1 new) — keystone regression teknium1 asked for: a live direct-CLI (.pid in ``~/.agent-browser/``) daemon survives ``_reap_orphaned_browser_sessions`` even when the identity guard is mocked to permit it. - ``TestDaemonStartFailureDetection`` (3) — detector behavior. - ``TestDaemonRetryOnStartFailure`` (2) — retry calls reaper + resets session; second failure does not retry again. - All 26 pre-existing tests in the file unchanged. Verified on Windows 11 / Python 3.11.15 / pytest 9.0.2: pytest tests/tools/test_browser_orphan_reaper.py # 32 passed pytest tests/tools/test_browser_cdp_override.py \ tests/tools/test_browser_cdp_tool.py \ tests/tools/test_browser_cleanup.py \ tests/tools/test_browser_chromium_check.py \ tests/tools/test_browser_console.py # 101 passed ## Files - ``tools/browser_tool.py`` — retry logic, session reset helper, daemon-start-failure detector. The app-dir reaper code from the first iteration is removed. - ``tests/tools/test_browser_orphan_reaper.py`` — new regression test; the app-dir test classes TestAppDirReaping and TestAppDirBindingGuard are removed (their target code no longer exists). ## Out of scope The original submission mentioned ``AGENT_BROWSER_IDLE_TIMEOUT_MS`` not being implemented in agent-browser v0.17.1 — that was accurate for 0.17.1 but stale for current main (commit 284e084 wired it for 0.26.0). That paragraph is removed from the PR body; the retry is now justified by the SIGKILL/Hyper-V/failed-install edge cases above, not by absence of idle-timeout.
1213517 to
9351efd
Compare
|
Resolved in Blocking ownership concern (the inline line comment)Resolved by deletion, not by tightening the guard. The whole
The regression test you asked for: Stale v0.17.1 premise — and a separate upstream gap I found while revalidatingAlso right on the version premise. Revalidating wasn't a no-op — it surfaced a separate install-flow bug that I'll file as its own PR shortly (linking back here). Honest findings: Repro on this Windows machine, this session:
Why this isn't just my machine being weird: I traced the install/update path.
I did not directly verify the repro on a healthily-installed 0.26.0 on this machine — that would need a separate
The PR body's stale I'll post the new PR for the |
|
Update for readers: the separate install-flow PR has been filed as #67835 — (As promised above — "I'll post the new PR for the This is a sibling fix; #65701 itself is unchanged. The new PR adds the missing Summary of the install-flow gap (full discovery trail in #67835's PR body, including live repro:
So:
Thanks — the review on #65701 is what surfaced the install-flow bug to begin with. |
Problem
On Windows, when Hermes is force-killed (taskkill, power loss, app close and reopen), the agent-browser daemon holds its deterministic TCP port as an orphan. The next Hermes session's first browser command hits
EADDRINUSEbefore the background reaper thread has had a chance to sweep, surfacing as:At that point every browser tool is unusable —
browser_navigate,browser_click,browser_snapshotetc. all return the error — until the user manually finds and kills the zombienode.exein Task Manager.Reproduced live on Windows (this session):
browser_navigatereturned"Daemon failed to start (port: 127.0.0.1:55478)"with a zombiedaemon.js(PID 39924) holding the port. After killing the zombie viaStop-Process -Id 39924 -Force,browser_navigatesucceeded. PID 39924's daemon was spawned fromC:\Users\LiteSoul\AppData\Roaming\nvm\v24.13.1\node_modules\agent-browser\dist\daemon.js— agent-browser v0.17.1.Fix (scoped, per teknium1's review)
Two pieces after a substantive revision responding to teknium1's Blocking feedback:
1. Retry on daemon-start failure
When
_run_browser_commanddetects"Daemon failed to start":_is_daemon_start_failure(result)._reap_orphaned_browser_sessions()to kill the Hermes-managed zombie holding the port. The reaper only globsagent-browser-h_*/agent-browser-cdp_*/agent-browser-hermes_*socket dirs in the system tmp dir; it does not touch~/.agent-browser/._reset_session_for_retry(task_id)drops the failed session from_active_sessionsand removes its socket dir, so the retry gets a freshh_<uuid4().hex[:10]>session name → new port → no collision with the just-killed zombie._run_browser_commandexactly once more with_daemon_retried=True. A second failure returns the error without retrying again.Why this is safe: the retry path only fires after the daemon has already failed to start — a daemon that can't bind its port never opened Chromium, never accepted a CDP connection, so there is no active browser session to lose.
_reap_orphaned_browser_sessionsis the existing cross-process-safe reaper (owner_pid-based ownership check, #21561); the retry just calls it inline instead of waiting for the lazy background cleanup thread.2. Dropped the
~/.agent-browser/sweep entirelyThe first iteration of this PR also swept
~/.agent-browser/for zombies from direct CLI invocations outside Hermes. teknium1 marked this Blocking on review: app-dir sessions have noowner_pidfile, so identity + session-name binding only proves the PID is an agent-browser daemon for that session name — not that it is orphaned or Hermes-owned. A liveagent-browser openin a user's separate terminal would be terminated by the next Hermes restart. The app-dir machinery is removed entirely from this iteration. New regression testTestAppDirSessionsAreLeftAlone::test_live_app_dir_daemon_survives_reaperpins the removal.The retry calls
_reap_orphaned_browser_sessionsunchanged — its glob misses~/.agent-browser/by design, so the retry can never reach live direct-CLI daemons either.Revalidation on current
mainteknium1's second ask was to revalidate the Windows repro on the upgraded dependency. Honest findings:
agent-browser@^0.26.0viaInstall-AgentBrowserinscripts/install.ps1(commit284e084bccwired the daemon idle-timeout). On Linux/macOS the equivalent isensure_browser()inscripts/install.sh.$HERMES_HOME/node/bin/agent-browser— the locationInstall-AgentBrowserpopulates — does not exist on disk. Hermes falls through to bare PATH, which resolves to a user-managed NVM install of agent-browser v0.17.1. The idle-timeout commit does NOT apply to a 0.17.1 daemon. The zombie-on-restart problem still reproduces here, exactly as the PR describes.install.ps1's auto-driven$InstallStageslist does not include abrowserstage, so the desktop Update button (which iteratesinstall.ps1 -Stage <n>per-stage viabootstrap-runner.ts) never invokesInstall-AgentBrowser. It's reachable only viainstall.ps1 -PostInstall(a one-off post-install bootstrapper) orinstall.ps1 -Ensure browser(an explicit manual invocation). Windows users installing/updating exclusively via the desktop app get whateveragent-browsertheir bare PATH provides. The zombie problem persists for that population as the production reality, not as a legacy edge — until abrowserstage reaches the install manifest (a separate PR I'll file shortly).agent-browser@^0.26.0(that would need a separateinstall.ps1 -PostInstallrun + force-restart repro). Flagging the gap rather than claiming false-green. The original submission's premise aboutAGENT_BROWSER_IDLE_TIMEOUT_MSbeing unimplemented in v0.17.1 was accurate for 0.17.1 but stale for current main — that paragraph is dropped from this body.Test Coverage (32 tests total)
TestReapOrphanedBrowserSessionsTestAppDirSessionsAreLeftAlone(1, NEW)_reap_orphaned_browser_sessionseven when identity guard is mocked to permit it — keystone pin against re-introducing the ownership bugTestOwnerPidCrossProcessowner_pidTestReaperIdentityGuardTestEmergencyCleanupRunsReaperTestDaemonStartFailureDetectionTestDaemonRetryOnStartFailureTwo test classes from the first iteration are removed:
TestAppDirReaping(6 tests) andTestAppDirBindingGuard(3 tests) — their target code no longer exists.Test Plan
pytest tests/tools/test_browser_orphan_reaper.py— 32/32 pass on Windows 11 / Python 3.11.15 / pytest 9.0.2pytest tests/tools/test_browser_cdp_override.py tests/tools/test_browser_cdp_tool.py tests/tools/test_browser_cleanup.py tests/tools/test_browser_chromium_check.py tests/tools/test_browser_console.py— 101/101 passpy_compileclean ontools/browser_tool.pymain(no conflicts —tools/browser_tool.pyauto-merged cleanly;tests/tools/test_browser_orphan_reaper.pysaw zero upstream changes since the original base)browser_navigatereturned"Daemon failed to start (port: 127.0.0.1:55478)"), killed zombie PID 39924 holding the port, confirmedbrowser_navigatesucceeded afterwardagent-browser@^0.26.0to confirm whether retry is still needed in that specific steady-state (defense-in-depth for SIGKILL/Hyper-V races is a separate, narrower argument)Files Changed
tools/browser_tool.py— retry logic (_is_daemon_start_failure,_reset_session_for_retry, the retry block in_run_browser_command); app-dir reaper code from first iteration removedtests/tools/test_browser_orphan_reaper.py— new regression test;TestAppDirReapingandTestAppDirBindingGuardremoved