codex flagged that the SIGTERM gate could never pass on Windows because
_process_start_time() returned None there, while ipc.identify() also
returns None during the slow-shutdown window (the daemon's serve()
tears down the IPC endpoint before stop_remote() runs its 15s-timeout
PATCH to api.browser-use.com). Combined: a Windows user with a remote
daemon would lose force-kill, leaving an orphan that may keep a billed
cloud browser alive.
Implemented the Windows branch via ctypes:
OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, pid)
GetProcessTimes -> creation FILETIME (100-ns intervals since 1601)
CloseHandle on the way out
The combined high/low DWORDs of the creation FILETIME serve as the
opaque fingerprint. PROCESS_QUERY_LIMITED_INFORMATION is sufficient
for GetProcessTimes (Vista+) and avoids needing PROCESS_QUERY_INFORMATION
which is more privilege-sensitive.
Also tightened the top guard from isinstance(pid, int) to
type(pid) is int — bool subclasses int in Python and was previously
slipping through (though the f-string formatting in the Linux branch
saved us by hitting /proc/True/stat which doesn't exist).
Test coverage: extended test_process_start_time_returns_stable_fingerprint_for_self
to also run on win32. Full suite: 75 passed locally on macOS.
Two more codex findings on ad39a95, both real:
1. _ipc.identify() accepted any positive int as pid, including values
too large for C pid_t (typically signed 32-bit). os.kill(huge_pid, 0)
then raises OverflowError, which propagates out of restart_daemon()
before its cleanup runs. Bounded the accepted range to 0 < pid <
2**31. Linux pid_max is also <2**22 in practice. Defense-in-depth:
added OverflowError to the except lists wrapping the os.kill calls
in restart_daemon, in case a different code path ever feeds it a
too-large pid.
2. The daemon's serve() tears down the IPC socket BEFORE the daemon
process actually exits — the daemon then runs slow cleanup work
(notably stop_remote()'s 15s-timeout PATCH to api.browser-use.com
to release a cloud browser). During that window identify() returns
None even though the process is still our daemon, so the strict
identify-only re-verification skipped SIGTERM and let the orphan
keep running. The next bh invocation would then spawn a new daemon
competing with the old one for the same Chrome.
Fix: snapshot a process-start-time fingerprint at the top of
restart_daemon, and accept it as a secondary identity signal in
the SIGTERM gate. Two reads returning the same fingerprint means
the PID still refers to the same process; a different fingerprint
means PID reuse, in which case SIGTERM is skipped (preserving the
protection from cubic's earlier finding). Implementation uses
/proc/<pid>/stat field 22 on Linux and ps -o lstart= on macOS;
on Windows / unsupported platforms the helper returns None and
restart_daemon falls back to the strict identify-only check.
Tests in tests/unit/test_admin.py and tests/unit/test_ipc.py:
- restart_daemon SIGTERMs via start-time match when socket is gone
(slow-shutdown recovery)
- restart_daemon skips SIGTERM when start-time has changed (PID
reuse during the wait window)
- _process_start_time returns a stable fingerprint for the current
process and None for invalid pids (None/0/negatives/non-int/dead)
- identify rejects oversized ints (covered via the bounded check)
Full suite: 75 passed.
Two issues raised on the initial commit:
1. Pre-upgrade daemons that don't include 'pid' in their ping reply made
identify() return None, which short-circuited the entire shutdown
path. The function would still delete the socket and pid file,
orphaning the still-running daemon. Fix: keep daemon_pid (verified
PID, used for signaling) separate from daemon_alive (any pong reply
counts, falls back to ipc.ping for the alive check). Shutdown IPC
is now sent whenever daemon_alive is true, regardless of whether
we got a verifiable PID; SIGTERM still requires a verifiable PID.
2. A single identify() at the top didn't prevent PID reuse during
the 15-second wait loop. If the daemon exited and the kernel
reused its PID before the loop timed out, SIGTERM would land on
the new owner. Fix: re-call identify() right before SIGTERM and
only signal if it still returns the same PID we've been waiting
on — any other state (None, different PID) means PID reuse is
possible and we skip the kill.
Two new tests in tests/unit/test_admin.py lock in both:
- test_restart_daemon_sends_shutdown_to_pre_upgrade_daemon_without_pid_in_ping
- test_restart_daemon_skips_sigterm_if_pid_was_reused_during_wait
Existing tests updated to also stub ipc.ping. Full suite: 61 passed.
The previous restart_daemon() read the daemon's PID from the
/tmp/<name>.pid file and could end up calling os.kill(pid, SIGTERM)
on whatever process now owned that PID — a real hazard whenever the
daemon had crashed without removing the file and the OS subsequently
reused the PID for an unrelated local process (browser, editor,
language server, ...). The 15-second polling loop made the issue
worse: any time os.kill(pid, 0) kept succeeding (because the new
owner stayed alive), the loop would fall through to SIGTERM.
Fix: ask the live daemon for its PID over the existing IPC ping
channel before signaling anything. The 'ping' meta-handler now
returns {pong: True, pid: os.getpid()}, and a new ipc.identify(name)
helper returns that PID or None if the daemon is unreachable.
restart_daemon() now signals only the PID returned by identify(),
never the contents of the pid file. If the daemon is unreachable we
just clean up the socket and pid file and return — we never escalate
to a kill-by-pid-file.
Adds two unit tests in tests/unit/test_admin.py:
- the unreachable-daemon path performs zero os.kill calls (the
exact regression: previously the stale pid file would still get
signaled);
- when identify() returns a different PID than the pid file, only
the identify-supplied PID is signaled.
Identified via codex code review (P0). All 59 unit tests pass.
Cross-checked AGENTS.md, README.md, SKILL.md, install.md, and profile-sync.md for accuracy. Tightened the sync_local_profile docstring and the chrome://inspect stderr message to match. Cloud-bootstrap test updated to set BU_AUTOSPAWN. All unit tests pass.
Three improvements to the cross-platform IPC layer, lifted from #104:
- meta:'ping' handshake replaces bare TCP connect in daemon_alive() and already_running(). A connect-only check on Windows can succeed against an unrelated process that grabbed our ephemeral port after a daemon crash; the ping/pong response confirms the listener is actually our daemon.
- Per-daemon random token (secrets.token_hex(32)) gates every request on Windows. AF_UNIX + chmod 600 is the boundary on POSIX, but TCP loopback has no chmod-equivalent; without a token any local process could connect and issue CDP commands.
- Atomic .port write (write .port.tmp, os.replace) so a concurrent reader never sees a half-written file.
Adds rohitdutt108 to VOUCHED.td.
Co-authored-by: Rohit Dutt <rohit.dutt@iyc.ishafoundation.org>
When BH_TMP_DIR is set, the caller owns the directory and is expected to
give each daemon its own — so the bu-<NAME> filename prefix is redundant.
Dropping it cuts ~30 chars off the AF_UNIX path on POSIX, which matters
because sun_path is 104 bytes on macOS / 108 on Linux and a long BU_NAME
(e.g. an opencode-style session id) can blow past it.
Concretely the failing path on macOS for a 30-char session id under
~/.local/share/bcode/sessions/<sessionID>/bu-<sessionID>.sock was 117
bytes; with this change it becomes ~83 bytes.
When BH_TMP_DIR is unset, _TMP is the shared default (/tmp on POSIX,
gettempdir() on Windows) and the bu-<NAME> prefix is kept to disambiguate
multiple daemons sharing the dir. Backward compatible by default.
admin._daemon_endpoint_names: when BH_TMP_DIR is set, the dir is per-daemon
by construction, so glob discovery doesn't apply — just check whether our
local endpoint exists. Without BH_TMP_DIR, fall back to the existing
shared-dir glob.
Validation (_check on BU_NAME) still runs in both modes to catch garbage
names early.
On Windows, `os.kill(pid, 0)` does not behave like its POSIX
counterpart. Instead of returning silently when the process
exists or raising ProcessLookupError when it doesn't, CPython
on Windows raises:
SystemError: <built-in function kill> returned a result
with an exception set
This happens because the underlying Win32 TerminateProcess API
does not accept signal 0 as an "is alive?" probe — Python's C
implementation hits an internal error path that doesn't set a
proper exception, and the interpreter surfaces SystemError.
`restart_daemon()` already catches `(ProcessLookupError, OSError)`
around both `os.kill(pid, 0)` and `os.kill(pid, signal.SIGTERM)`,
but SystemError isn't a subclass of either, so the harness crashes
on every second invocation on Windows: the daemon's stale pid file
points at a no-longer-running pid, the probe raises SystemError,
and the whole `browser-harness -c '...'` call dies with a stack
trace.
Repro on Windows 11 (Python 3.12, browser-harness 0.1.0):
browser-harness --reload # ok
browser-harness -c 'print(page_info())' # ok
browser-harness -c 'print(page_info())' # CRASH
Fix: add SystemError to the existing except clauses in
restart_daemon(). One-word change in two places, matches the
intent of the existing handlers (treat any "couldn't probe/signal
the pid" failure as "process is gone, move on").
The stale-daemon probe in ensure_daemon used raw socket.AF_UNIX, which:
1. Doesn't exist on uv-bundled Python on Windows (AttributeError).
2. Would point at a TCP display string '127.0.0.1:<port>', not a socket
path, even if AF_UNIX existed.
Either way the probe always raised, was swallowed by 'except Exception',
and fell through to restart_daemon — killing and respawning the daemon
on every warm call. Symptom is most visible on Windows where the warm
path is the common case.
Fix: use ipc.connect(name) which already does the right thing per
platform (AF_UNIX on POSIX, TCP loopback on Windows). Same call
daemon_alive and restart_daemon already use.
Also drop the now-unused _paths() helper; restart_daemon only needed
the pid_path half, inline that.
When remote-debugging is not enabled, `run_setup` opens chrome://inspect
once, then retries `ensure_daemon` in a loop. But `ensure_daemon` itself
also calls `_open_chrome_inspect()` on failure — opening a new tab every
~7 seconds and flooding the browser.
Add `_open_inspect` parameter to `ensure_daemon` so the retry loop in
`run_setup` can suppress redundant tab opens after the first one.
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Saurav Panda <sgp65@cornell.edu>
Add browser_connections() / active_browser_connections() to admin, which probe
each daemon socket via a new connection_status meta message. The daemon tracks
target_id (set on attach and on set_session) and returns the live page title/URL
via Target.getTargetInfo. switch_tab now forwards target_id in set_session so the
daemon stays in sync after tab switches. run_doctor shows the count, each daemon
name, and the truncated active-page title and URL.
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
* refactor(tests): reorganize into tests/unit and tests/integration
Moves all root-level test_*.py files into a structured tests/ directory:
- tests/unit/ — admin, helpers (was test_screenshot), run
- tests/integration/ — js expression tests
- tests/conftest.py — shared fake_png pytest fixture, eliminating duplication
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* refactor: move to src layout, agent-workspace, and fix SKILL.md invocation format
- Move package to src/browser_harness/ and domain-skills/interaction-skills to agent-workspace/
- Fix all browser-harness <<'PY' heredoc examples in SKILL.md and run.py HELP string to use the correct -c '...' flag format (heredoc was never supported by the CLI)
- Update SKILL.md path references from domain-skills/ to agent-workspace/domain-skills/
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
---------
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Saurav Panda