diff --git a/CONTEXT.md b/CONTEXT.md index 0714217..1aaaf27 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -152,6 +152,20 @@ that, `EADDRINUSE` stops it. The control socket file that used to carry both jobs is gone, and with it the watchdog that self-exited a daemon whose socket was replaced — there is nothing left to replace. +### daemon ownership +Which process owns the single **endpoint**: `launchd` (an installed LaunchAgent +plist runs `serve`, ADR-0012 rules 1–3) or `cli` (the CLI started the same +`serve` detached — `browserwright-daemon restart` / `serve`, or +`daemon_lifecycle.ensure`'s on-demand spawn — with no plist anywhere). Both are +supported modes; `restart` works in both, takes the plist case through launchd +unchanged, and reports which one it bounced (`owner`). + +**Trap:** this is not the **owner** above, which is about *browsers*. This one +is about the *daemon process*, and it is what decides where durable bind config +lives: in the plist's `ProgramArguments` (launchd-owned) or in the config file +(`--config`, else `$BD_CONFIG`, else `$BS_HOME/config.toml`) — where a flag the +plist passes still tops the file. + ### downstream Everything that connects **into** the daemon: the CLI, the skill client, a Playwright client on the cdp surface. Downstream must never branch on backend — @@ -348,11 +362,11 @@ just before it is the attribution. ### daemon lifecycle The client side's one owner of "is the daemon up, and make it so" (`src/browserwright/daemon_lifecycle.py`). Three calls: `diagnose()` returns -one `DaemonVerdict` (`up` · `stale` · `down` · `foreign` · `unreachable` · -`undecided`), `ensure(reason)` is the **only** Layer 2 code that starts or -replaces the daemon (executor handoff, initiator, `LIFECYCLE` line, child env), -and `unreachable_fix()` builds the agent-facing diagnosis text. Every -`browserwright-daemon ` a Layer 2 module runs goes through its +one `DaemonVerdict` (`up` · `stale` · `down` · `foreign` · `unresponsive` · +`unreachable` · `undecided`), `ensure(reason)` is the **only** Layer 2 code +that starts or replaces the daemon (executor handoff, initiator, `LIFECYCLE` +line, child env), and `unreachable_fix()` builds the agent-facing diagnosis +text. Every `browserwright-daemon ` a Layer 2 module runs goes through its `run_verb` adapter, which carries the resolved endpoint into the child. **Trap:** constructing a `Session` (or its `ModeBClient`) has no lifecycle side @@ -363,8 +377,13 @@ configured endpoint (see `endpoint`) is diagnosed, never started or replaced. **Trap:** `diagnose(confirm=True)` takes two probes and concludes nothing when they disagree (`undecided`); only `down` and `stale` on the default endpoint -are `replaceable`. `foreign` (something that is not browserwright holds the -port) is never replaced by `recover` — ADR-0013 rule 2. +are `replaceable`. `foreign` (something *answered* HTTP that is not our pong) +and `unresponsive` (the socket opened and nothing came back, for the whole +escalated probe window) are never replaced by `recover` — ADR-0013 rule 2. +Folding silence into `foreign` is exactly the bug that replaced a live daemon +mid-call on 2026-10-19: two agreeing timeouts are not evidence of anything. +A probe that times out is therefore re-asked with a much larger budget +(`_ipc.probes_until_answered`), so a merely slow daemon comes back `up`/`stale`. ### activity gate The daemon's own answer to "would interrupting me hurt someone right now": @@ -375,11 +394,17 @@ and the e2e runner all consult it and refuse while it says busy; `restart --force` is the only override, for humans. ADR-0012 rules 2 and 4. ### lifecycle event -A daemon start / stop / restart / spawn, written as one `LIFECYCLE ` -line into the daemon log (`_ipc.log_lifecycle`) by whichever process caused -it, so the record exists even when the daemon being replaced never logs its -exit. Distinct from the extension's `hello` / relay reconnects, which are -connection events, not process events. +A daemon start / stop / restart / spawn / signal, written as one +`LIFECYCLE ` line into the daemon log (`_ipc.log_lifecycle`) by +whichever process caused it, so the record exists even when the daemon being +replaced never logs its exit. Distinct from the extension's `hello` / relay +reconnects, which are connection events, not process events. + +**Trap:** a *signal-driven* stop is the one event only the daemon can record +(`LIFECYCLE signal sig=… pid=… initiator=…`): whoever ran `kill` writes no +log line. It is skipped when the `stop` verb already recorded that stop, so +one stop is one line. A stop with neither line is a stop the daemon never got +to write — SIGKILL, or a crash — not an ordinary shutdown. ### Router The frame-routing engine (`daemon/server/proxy.py`). Owns request-id rewriting, diff --git a/docs/adr/0012-development-never-touches-the-global-daemon.md b/docs/adr/0012-development-never-touches-the-global-daemon.md index 2d2ce7c..851de28 100644 --- a/docs/adr/0012-development-never-touches-the-global-daemon.md +++ b/docs/adr/0012-development-never-touches-the-global-daemon.md @@ -1,6 +1,6 @@ # Development never touches the global daemon, the global binary, or the daily Chrome -Status: accepted (2026-09-02). Rule 5 implemented (#89, via #92); rules 1–4 and 6 implemented (#91). Amended the same day: remote use over the tailnet is a real requirement (a VPS drives this Mac's Chrome), so rule 3 keeps the non-loopback bind and makes it durable instead of removing it; rule 4 became an activity gate rather than a concurrency cap (rule 4 was later withdrawn, 2026-10-05); rule 6 was added after the maintainer's own verification run stopped the global daemon. Supersedes the partial +Status: accepted (2026-09-02). Rule 5 implemented (#89, via #92); rules 1–4 and 6 implemented (#91). Amended the same day: remote use over the tailnet is a real requirement (a VPS drives this Mac's Chrome), so rule 3 keeps the non-loopback bind and makes it durable instead of removing it; rule 4 became an activity gate rather than a concurrency cap (rule 4 was later withdrawn, 2026-10-05); rule 6 was added after the maintainer's own verification run stopped the global daemon. Amended 2026-10-19: rule 7 — the endpoint has exactly one owner, and a CLI-owned daemon with no LaunchAgent is a supported owner. Supersedes the partial isolation in `tests/conftest.py` (kept) by extending the same rule to every developer-facing verb. @@ -156,6 +156,62 @@ Rules, one per leak found: `stop` skips the guard when the override is port 0 (the per-test "bind anywhere" scheme), where the state file is the only truth. +### Amendment (2026-10-19, ownership): a CLI-owned daemon is the other supported owner + +Rules 1–6 above are all written against an *installed* LaunchAgent, and read +together they made the plist look mandatory: rule 3 gave durable bind config no +home but the plist, and `install` was "the" way to run the daemon as a service. +Three gaps followed, each found by hitting it: + +- **`restart` required a plist.** It refused with "no LaunchAgent installed" + (exit 2) before doing anything, and `mise run upgrade-global` and `mise run + restart-daemon` both end on `browserwright-daemon restart` — so on a machine + that runs a CLI-owned daemon, a release upgrade died at the restart step with + the new package already installed. +- **Durable bind config lived only in the plist.** `facade_host` (the tailnet + address a remote consumer connects to, with loopback co-bound) could only be + made durable by installing a LaunchAgent. `upgrade-global` scrubs + `BD_CONFIG`/`BD_FACADE_HOST` before it touches the global install (rule 2), + so an environment variable cannot be the durable home either. +- **A signal-driven stop was unattributable.** Rule 5's `LIFECYCLE stop` line is + written by the `stop` verb, never by the daemon, so a daemon that something + else SIGTERMed left nothing behind but `shutdown requested` — the exact + evidence gap rule 5 exists to close, and the reason the 2026-10-19 incident + (a dev-checkout script killing the machine-global daemon mid-call) was + expensive to reconstruct. + +7. **The endpoint has exactly one owner, and "no LaunchAgent" is a supported + mode.** The owner is either launchd (an installed plist, rules 1–3) or the + CLI that started `serve` (a *CLI-owned* daemon: `browserwright-daemon + restart`/`serve`, or `daemon_lifecycle.ensure`'s on-demand spawn). Both are + supported; neither is a degraded form of the other. Concretely: + - **Durable bind config lives in the config file.** The toml is `--config`, + else `$BD_CONFIG`, else `$BS_HOME/config.toml` (default + `~/.browserwright/config.toml`, beside `global.md` / `sessions/` / + `site-skills/`). A missing file is an empty config, never an error, and + the precedence is unchanged: a flag the plist passes still tops the file + (rule 3), so the two homes cannot silently fight over one setting. + - **`restart` never requires a LaunchAgent.** With a plist it does what it + always did. Without one it stops the incumbent and starts the same + `serve` detached, then proves the *same* post-condition: a pid exists, it + differs from the incumbent's, and `/__status__` reports the expected + version. A `serve` that cannot bind exits at once (status 2) and the + incumbent is already stopped, so every failure is reported with its exit + status — never a silent ok. `install`/`uninstall` are untouched: a plist + is still how you ask launchd to own the daemon. + - **Both owners write the same lifecycle trail.** A signal-driven shutdown + records its own `LIFECYCLE signal` line (signal, pid, initiator from the + daemon's environment) before the shutdown sequence starts, and skips it + when the `stop` verb already recorded that stop — one event, one line. + - **The mode is visible, not merely tolerated.** `browserwright-daemon + list` reports `service: manual` when no plist is installed, and + `restart`'s report carries `owner: cli | launchd`. + + What this does NOT change: the isolation rules themselves (a development + daemon still owns its own ports, runtime dir and ledger), the diagnosis and + activity gates on `restart`, or who may signal what — a CLI-owned restart + still stops only a locally signalable endpoint. + ## What this does NOT change - ADR-0011's single-endpoint transport stays. Whether a daemon restart should diff --git a/docs/adr/0013-daemon-owns-session-recovery.md b/docs/adr/0013-daemon-owns-session-recovery.md index 799a090..40d7f61 100644 --- a/docs/adr/0013-daemon-owns-session-recovery.md +++ b/docs/adr/0013-daemon-owns-session-recovery.md @@ -112,6 +112,18 @@ is logged with its reason and both probe results (ADR-0012 rule 5). If the ladde ends in `needs-human`, `recover` says so with the diagnosis and stops; it never loops. +Amendment (2026-10-19, incident): a **probe timeout is not evidence about a +process**. The first implementation classified an unanswered `GET /__ping__` as +`foreign` ("something else holds the port"), so two agreeing 1.5s timeouts +authorized a replacement — and, through `serve`'s port reclaim, a SIGTERM — +against a machine-global daemon that was alive but momentarily slow (still +accepting ws clients, graceful on the signal). Silence now has its own verdict, +`unresponsive`: a timeout (or non-HTTP bytes) is escalated for a bounded window +before anything is concluded, `ensure` never spawns or stops on it, and the +implicit reclaim that SIGTERMs a port holder requires the holder to have stayed +unanswered for that whole window. `foreign` keeps its original meaning — +something *answered*, just not with our pong. + ### 3. Agent-visible error text follows a banned-word list Text an agent can see (exceptions, `doctor`, skill documents' runtime-error diff --git a/docs/architecture.md b/docs/architecture.md index 931d13b..251c8a2 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -55,9 +55,9 @@ semantics, tab creation, facade behavior, or teardown: --port --profile /tmp/...` or the e2e harness. Never short-connect to the user's daily Chrome. 2. *Filesystem*: any code path that writes outside the temp dir - (e.g. `~/.config/browserwright-daemon/config.toml`) must accept a - `*_PATH` env override so tests can redirect to `tmp_path`. The wizard's - `BS_DAEMON_CONFIG_PATH` is the canonical example. + (e.g. `$BS_HOME/config.toml`) must accept a + `*_PATH` env override so tests can redirect to `tmp_path`. The daemon's + config resolves through `BS_HOME` for exactly this reason. - **Develop without touching global state.** The host machine very likely already runs a global `browserwright`/`browserwright-daemon` plus a loaded Chrome extension; this checkout must coexist with that, not fight it. The @@ -87,7 +87,7 @@ touching any of the above. | Daemon endpoint | `127.0.0.1:19990` | two daemons cannot share the port — the second refuses to start | | Extension relay port | `127.0.0.1:19989` | starting a second relay there steals connections from the global extension | | CDP port | `9222` (Chrome's default) | a stray `--remote-debugging-port=9222` collides with your daily Chrome and pops Allow dialogs | -| Daemon TOML config | `~/.config/browserwright-daemon/config.toml` (override with `BD_CONFIG`) | inherited backend / port settings leak into your dev daemon | +| Daemon TOML config | `~/.browserwright/config.toml` (`$BS_HOME`; override with `BD_CONFIG`) | inherited backend / port settings — including a durable `facade_host` — leak into your dev daemon | | Skill home | `~/.browserwright/` (override with `BS_HOME`) | site-skill writes and memory leak across instances | | Chrome profile | the user's daily profile | `--load-extension` / `--remote-debugging-port` ruin it on Chrome 144+ | @@ -129,7 +129,7 @@ which port is which: | daemon endpoint host | `127.0.0.1` (loopback) | tailnet/LAN IP or `0.0.0.0` to reach off-box | `--facade-host` / `BD_FACADE_HOST` | | which daemon a client talks to | `http://127.0.0.1:19990` | the test daemon's endpoint URL | `BW_DAEMON_URL` / `--daemon-url` | | runtime dir (pid, executor sockets, log) | `${XDG_RUNTIME_DIR:-/tmp}` | `$(mktemp -d)` | `XDG_RUNTIME_DIR` | -| daemon TOML config | `~/.config/browserwright-daemon/config.toml` | none | `BD_CONFIG=""` | +| daemon TOML config | `$BS_HOME/config.toml` | nothing (redirected `BS_HOME` has no file) | redirect `BS_HOME`; `BD_CONFIG` for a file of your own | | skill home | `~/.browserwright/` | tmpdir | `BS_HOME` | Isolation is **port-based** (ADR-0011): give the test daemon its own @@ -236,7 +236,7 @@ End-to-end isolation matrix (kept in sync with `tests/daemon/e2e/README.md`): | Chrome `user-data-dir` | the user's daily profile | per-test tmpdir | | Chrome binary | Google Chrome | Chrome for Testing | | extension `RELAY_URL` | `ws://127.0.0.1:19989/` | `ws://127.0.0.1:29989/` (patched copy) | -| `BD_CONFIG` | `~/.config/browserwright-daemon/config.toml` | `""` (no file) | +| daemon TOML config | `$BS_HOME/config.toml` (when `BD_CONFIG` is unset) | `tests/daemon/e2e/_bs_home/…/config.toml` (via `BS_HOME`) | | `BS_HOME` | `~/.browserwright/` | `tests/daemon/e2e/_bs_home/...` | Nothing escapes the boundary. You can have your daily Chrome + global @@ -306,7 +306,7 @@ lsof -iTCP:29989 -sTCP:LISTEN ls -la ~/.browserwright/ # Daemon config untouched? -stat -f '%m' ~/.config/browserwright-daemon/config.toml 2>/dev/null +stat -f '%m' ~/.browserwright/config.toml 2>/dev/null ``` If `lsof :19989` and `lsof :29989` print **different** PIDs (or diff --git a/docs/daemon.md b/docs/daemon.md index 2dba6fa..2e49a5a 100644 --- a/docs/daemon.md +++ b/docs/daemon.md @@ -171,7 +171,10 @@ daemon 会让已连接的扩展调用 `chrome.runtime.reload()` 从磁盘重新 ## 配置（可选） -`~/.config/browserwright-daemon/config.toml`： +`$BS_HOME/config.toml`（默认 `~/.browserwright/config.toml`，与 `global.md`、`sessions/`、 +`site-skills/` 同一个 home；`--config` / `BD_CONFIG` 可指定别的文件，文件不存在等于空配置）。 +装 LaunchAgent 的机器上也可以把同样的条目写进 plist 的 `ProgramArguments`——那是另一种 +endpoint 归属模式（ADR-0012 规则 7），两种模式只需要选一种： ```toml # 覆盖 daemon shared upstream（与 `BD_BACKEND` 等价； diff --git a/scripts/replay-daemon-silence-incident.py b/scripts/replay-daemon-silence-incident.py new file mode 100644 index 0000000..139c647 --- /dev/null +++ b/scripts/replay-daemon-silence-incident.py @@ -0,0 +1,184 @@ +#!/usr/bin/env python3 +"""Replay the 2026-10-19 daemon incident — and show it no longer reproduces. + +What happened that day: a machine-global daemon that was *alive but momentarily +slow* was SIGTERMed two seconds after three concurrent CLI calls missed two 1.5s +``/__ping__`` probes. The daemon was never wedged — it was still accepting ws +clients and it shut down gracefully on the signal — but one negative short probe +was all the evidence the port reclaim needed. + +Two questions the incident turned on, asked here for four endpoint shapes: + +1. what one 1.5s probe says — the only evidence the old code acted on, and +2. what ``daemon_lifecycle.ensure`` concludes — the decision that starts or + stops a daemon. + +Expected: the slow endpoint reads as ``timeout`` to the single probe and ``up`` +to ``ensure`` (no spawn, no stop); a silent holder is ``unresponsive`` (no +spawn, no stop); a healthy daemon costs nothing extra; and a genuinely dead +endpoint still spawns, because recovery must keep working. + +Hermetic by construction: this script sets HOME-adjacent state aside *before* +importing the package, and it replaces the three side-effecting seams +(`daemon_endpoint`, `_spawn_detached`, `run_verb`) with fakes. It therefore +cannot contact, stop, or start the machine-global daemon. That is not +decoration: an earlier version of this replay called ``ensure()`` with the +default endpoint while running a dev checkout (version ``0.0.0``), which the +lifecycle correctly read as a *stale* daemon and replaced — killing the user's +live daemon. ADR-0012 is why the test suite has a wall for exactly this. + +Usage: uv run python scripts/replay-daemon-silence-incident.py +""" +from __future__ import annotations + +import json +import os +import socket +import sys +import tempfile +import threading +import time + +# --- isolate before importing the package (ADR-0012) ------------------------ +# The daemon log, the sessions ledger and the default config file all resolve +# from these; pointing them at a scratch dir keeps a replay from writing into +# the real browserwright home. +_SCRATCH = tempfile.mkdtemp(prefix="bw-replay-") +os.environ["BS_HOME"] = _SCRATCH +os.environ["TMPDIR"] = _SCRATCH + +from browserwright import daemon_lifecycle as lifecycle # noqa: E402 +from browserwright.daemon._ipc import probe_endpoint_sync # noqa: E402 +from browserwright.daemon_url import DaemonEndpoint # noqa: E402 +from browserwright.version import package_version # noqa: E402 + +SHORT_PROBE_S = 1.5 +VERSION = package_version() + + +def fake_daemon(delay: float | None): + """A listener answering /__ping__ after ``delay`` seconds, or never.""" + srv = socket.socket() + srv.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) + srv.bind(("127.0.0.1", 0)) + srv.listen(64) + + def accept_loop() -> None: + while True: + try: + conn, _ = srv.accept() + except OSError: + return + threading.Thread(target=answer, args=(conn,), daemon=True).start() + + def answer(conn) -> None: + with conn: + try: + conn.recv(4096) + if delay is None: + time.sleep(3600) # hold the socket, never answer + return + time.sleep(delay) + body = json.dumps( + {"pong": True, "pid": 4242, "version": VERSION} + ).encode() + conn.sendall( + b"HTTP/1.1 200 OK\r\nContent-Type: application/json\r\n" + + f"Content-Length: {len(body)}\r\nConnection: close\r\n\r\n".encode() + + body + ) + except OSError: + pass + + threading.Thread(target=accept_loop, daemon=True).start() + return srv, srv.getsockname()[1] + + +def probe_that_never_listens() -> int: + """A port nothing is listening on (bound, then closed).""" + sock = socket.socket() + sock.bind(("127.0.0.1", 0)) + port = sock.getsockname()[1] + sock.close() + return port + + +def run_case(label, port, *, expect_state, expect_spawn, expect_stop, + max_s=None) -> bool: + endpoint = DaemonEndpoint( + url=f"http://127.0.0.1:{port}", explicit=False, source="replay") + + started: list = [] + stopped: list = [] + saved = (lifecycle.daemon_endpoint, lifecycle._spawn_detached, + lifecycle.run_verb) + lifecycle.daemon_endpoint = lambda: endpoint + lifecycle._spawn_detached = lambda args, **_kw: (started.append(args), None)[1] + lifecycle.run_verb = lambda args, **_kw: (stopped.append(args), None)[1] + try: + began = time.monotonic() + short = probe_endpoint_sync("127.0.0.1", port, timeout=SHORT_PROBE_S) + short_ms = (time.monotonic() - began) * 1000 + began = time.monotonic() + verdict = lifecycle.ensure(f"replay {label}") + elapsed = time.monotonic() - began + finally: + (lifecycle.daemon_endpoint, lifecycle._spawn_detached, + lifecycle.run_verb) = saved + + ok = ( + verdict.state == expect_state + and bool(started) == expect_spawn + and bool(stopped) == expect_stop + and (max_s is None or elapsed <= max_s) + ) + print(f" {label}") + print(f" one short probe -> {short.kind:12s} ({short_ms:6.0f}ms)" + " <- all the evidence the old code had") + print(f" ensure() -> {verdict.state:12s} ({elapsed:5.2f}s) " + f"spawn={'yes' if started else 'no':3s} stop={'yes' if stopped else 'no'}") + print(f" {'PASS' if ok else 'FAIL'} expected state={expect_state}, " + f"spawn={'yes' if expect_spawn else 'no'}, " + f"stop={'yes' if expect_stop else 'no'}") + print() + return ok + + +def main() -> int: + print("Replaying the 2026-10-19 incident: the daemon was slow, not dead.\n") + results = [] + + # The incident: alive, answers in 3s. One short probe times out — the old + # code called that FOREIGN, replaced the daemon, and the replacement's port + # reclaim SIGTERMed it on one more negative ping. + srv, port = fake_daemon(delay=3.0) + results.append(run_case("slow (3s) but alive — the incident", port, + expect_state="up", expect_spawn=False, + expect_stop=False)) + srv.close() + + # Not slow — silent. Still must not be replaced, and must not be stopped. + srv, port = fake_daemon(delay=None) + results.append(run_case("accepts the connection, never answers", port, + expect_state="unresponsive", expect_spawn=False, + expect_stop=False)) + srv.close() + + # Healthy: no escalation tax. + srv, port = fake_daemon(delay=0.0) + results.append(run_case("healthy (no escalation cost)", port, + expect_state="up", expect_spawn=False, + expect_stop=False, max_s=0.5)) + srv.close() + + # Genuinely dead: recovery must still work. + results.append(run_case("nothing listening (truly gone)", probe_that_never_listens(), + expect_state="down", expect_spawn=True, + expect_stop=False, max_s=0.5)) + + print(f"{sum(results)}/{len(results)} passed") + return 0 if all(results) else 1 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/src/browserwright/_executor/client.py b/src/browserwright/_executor/client.py index d4c008e..2aa37e3 100644 --- a/src/browserwright/_executor/client.py +++ b/src/browserwright/_executor/client.py @@ -20,22 +20,39 @@ daemon's event loop, and a daemon restart severs a live data plane (it used to survive one). The client turns that severing into ``ExecutorUnavailable`` with a "daemon restarted or unreachable" message rather than a bare ws error. + +A *replacement* daemon is not a failure, though: it adopts the session's +executor and its browser. So the two seams that run strictly before the +request can reach the executor — the ``ensureExecutor`` verb and the ``/exec`` +connect — retry a daemon that went away, re-resolving the endpoint on every +attempt (`_call_until_daemon_returns`). When those retries run out, the text +names the daemon and what happened to it, because the alternative — blaming a +missing extension for a browser that is connected and healthy — is what sent +three agent calls to `chrome://extensions` on 2026-10-19 (ADR-0013 rule 3). """ from __future__ import annotations import json import signal import threading -from collections.abc import Iterator +import time +from collections.abc import Callable, Iterator from contextlib import contextmanager from dataclasses import dataclass +from typing import TypeVar from websockets.exceptions import ConnectionClosed, WebSocketException from websockets.sync.client import connect as ws_connect from .. import session_registry as reg +from ..daemon.errors import DaemonShuttingDown from ..daemon_url import daemon_endpoint -from ..errors import BrowserwrightError, DaemonUnavailable +from ..errors import ( + BrowserwrightError, + CDPError, + CDPTransportClosed, + DaemonUnavailable, +) from .protocol import ( DEFAULT_TIMEOUT_MS, _MAX_FRAME, @@ -48,6 +65,24 @@ # browser cold-start is part of the request deadline, not hidden extra time. _RESPONSE_DELIVERY_SLACK_S = 2.0 +#: How long a *departed* daemon gets to come back before the client stops +#: retrying and says what happened. A replacement daemon needs the old one +#: stopped and its own endpoint bound before it can adopt the session's +#: executor, so this window is the whole difference between surviving a daemon +#: replacement and an `ExecutorUnavailable` that blames the wrong layer. It is +#: deliberately the same budget as the executor's cold-start connect +#: (13 x 0.75s ≈ 9s, `_executor/process.py`) so the two waits on one call cannot +#: stack into an apparent hang, and ADR-0014 caps it further by whatever the +#: caller's `--timeout` has left. +_DAEMON_RETURN_ATTEMPTS = 13 +_DAEMON_RETURN_BACKOFF_S = 0.75 + +#: What `ws_connect` raises when the daemon's front door is not open (yet): +#: nothing listening, or nothing speaking the ws handshake. +_DAEMON_DOOR_ERRORS = (WebSocketException, ConnectionClosed, OSError) + +_T = TypeVar("_T") + class ExecutorUnavailable(BrowserwrightError): """The session's executor could not be ensured/reached. @@ -75,22 +110,51 @@ class ExecutorLease: executor_id: str -def _ensure_executor_lease(sess) -> ExecutorLease: - """Ask the daemon to ensure the session's executor and return its socket - path. Uses the session's mode_b CDP client (``sess.cdp``) to send the - control-plane verb.""" +def _ensure_executor_lease(sess, *, deadline: float | None = None) -> ExecutorLease: + """Ask the daemon to ensure the session's executor and return its identity. + + Uses the session's mode_b CDP client (``sess.cdp``) to send the + control-plane verb. ``deadline`` is the caller's ``time.monotonic()`` + instant (ADR-0014), which caps the retries below. + """ sid = _session_id(sess) - try: + + def _attempt() -> dict: # The browserwright session is already bound on the websocket query # (`?session=`). Do not pass it as CDP's top-level `sessionId`; # that field means "attached target session" inside the proxy mux. - res = sess.cdp.send("BrowserwrightDaemon.ensureExecutor", bsSession=sid) - except DaemonUnavailable: - # "the daemon is not there" already says everything, with the endpoint - # named and the auto-start rule explained. Wrapping it would bury that + return sess.cdp.send("BrowserwrightDaemon.ensureExecutor", bsSession=sid) + + def _give_up(exc: BaseException, attempts: int, + elapsed: float) -> BrowserwrightError: + if isinstance(exc, DaemonUnavailable): + # "nothing answered at " is already the better diagnosis + # (probed, and it explains the auto-start rule). Wrapping it here + # would bury that under a second, less specific story. + return exc + return ExecutorUnavailable( + f"the daemon serving session {sid!r} went away mid-call (it was " + f"replaced or is shutting down): {attempts} attempt(s) over " + f"{elapsed:.1f}s could not reach it (last: {exc}). " + f"{_endpoint_state_now()} The browser and the extension are not " + f"involved; a replacement daemon adopts the session's executor.", + fix="retry the command", + ) + + try: + res = _call_until_daemon_returns( + _attempt, + deadline=deadline, + departed=_daemon_departed, + before_retry=lambda: _forget_daemon_endpoint(sess), + give_up=_give_up, + ) + except (DaemonUnavailable, ExecutorUnavailable): + # "nothing answered at the endpoint" / "it left mid-call" already say + # everything, with the daemon named; wrapping them would bury that # under a second, less specific fix about stale executors. raise - except Exception as e: + except Exception as e: # noqa: BLE001 - keep one agent-facing envelope raise ExecutorUnavailable( f"ensureExecutor failed for session {sid!r}: {e}" ) from e @@ -109,6 +173,135 @@ def _ensure_executor_lease(sess) -> ExecutorLease: return ExecutorLease(session_id=sid, executor_id=executor_id) +def _forget_daemon_endpoint(sess) -> None: + """Make the next attempt dial the daemon that answers *now* (ADR-0013). + + ``ModeBClient.ws_url()`` caches its answer until ``invalidate()``, and + `Session.cdp` re-creates its `CDPSession` only once the old one is closed — + so both halves are needed: drop the cached url, and the dead socket is then + replaced (and re-resolved) by the property itself rather than reused. + Retrying over the socket that just died would retry the daemon that just + left, which is the entire failure this absorbs. + """ + invalidate = getattr(getattr(sess, "daemon", None), "invalidate", None) + if callable(invalidate): + invalidate() + + +def _daemon_departed(exc: BaseException, attempt: int) -> bool: + """True only when ``exc`` says the *daemon* left — never on "any error". + + Three shapes qualify, and they are exactly the ways a client learns its + daemon is gone: the request's socket died with no reply + (`CDPTransportClosed`), the daemon answered in its own words that it is + shutting down (`DaemonShuttingDown`, carried through the verb's JSON-RPC + error envelope), or nothing answers the endpoint any more + (`DaemonUnavailable`) *after* an earlier attempt already saw it leave. + + A refusal on the first attempt is deliberately NOT a departure: nothing was + ever there, which `DaemonUnavailable` diagnoses on its own (probed endpoint, + auto-start rule) and which `session new` / `recover` own. Retrying it would + make every "no daemon is running" call 9s slower to say the same thing. + + Everything else — the no-extension diagnosis, a -32601 from a stale daemon, + a rejected attach — would fail identically on the next attempt, so treating + it as retryable would burn the caller's deadline and then report the wrong + layer. That the daemon can tell these apart (`DaemonShuttingDown` vs the + extension text) is what makes this list exist; do not flatten it here. + """ + if isinstance(exc, CDPTransportClosed): + return True + if isinstance(exc, DaemonUnavailable): + return attempt > 1 + if isinstance(exc, CDPError): + return DaemonShuttingDown.MARKER in str(exc.cdp_message) + return False + + +def _endpoint_is_the_daemons(exc: BaseException, _attempt: int) -> bool: + """`_connect`'s classifier: the `/exec` surface lives only in the daemon. + + No request frame exists yet, so a refused/closed connect is always "the + daemon is not answering yet" and never the browser or the extension — and a + first-attempt refusal here still means the daemon left, because + `ensureExecutor` reached that same daemon moments earlier. Anything that is + not a transport failure (a broken URL resolution, a bad handshake argument) + would fail identically on every attempt, so it is not retried. + """ + return isinstance(exc, _DAEMON_DOOR_ERRORS) + + +def _endpoint_state_now() -> str: + """ADR-0013 rule 3: probe the endpoint once and say what answered. + + "Nothing is listening" and "something that is not browserwright answered" + are different diagnoses with different next steps, and neither may be + guessed from a socket error — the case that rule was written for is a local + proxy answering 503 on the endpoint while the daemon is fine. So the last + words of a failed call carry the observation, not an inference. + """ + from ..daemon import _ipc + + ep = daemon_endpoint() + return f"Right now {_ipc.probe_endpoint_sync(ep.host, ep.port).describe()}." + + +def _daemon_return_budget(deadline: float | None) -> float: + """~9s of retries, or less when the caller's deadline leaves less.""" + full = _DAEMON_RETURN_BACKOFF_S * _DAEMON_RETURN_ATTEMPTS + if deadline is None: + return full + return max(0.0, min(full, deadline - time.monotonic())) + + +def _call_until_daemon_returns( + attempt: Callable[[], _T], + *, + deadline: float | None, + departed: Callable[[BaseException, int], bool], + give_up: Callable[[BaseException, int, float], BrowserwrightError], + before_retry: Callable[[], None] | None = None, +) -> _T: + """Run ``attempt``, retrying only while the *daemon* is what went missing. + + **The retry scope is a correctness rule, not a convenience.** Both callers + sit strictly before the session's request can have reached the executor: + the `ensureExecutor` verb, and the `/exec` connect, which has no frame yet. + Replaying either cannot execute the agent's code twice. The send/recv pair + in `_run_request_on_executor` deliberately has no retry at all — once a + request frame may be on the wire a replay would run the caller's code a + second time, so that path reaps the exact executor instance instead. + + ``departed`` decides what counts as the daemon leaving, and sees the + attempt number too (see `_daemon_departed`: a refused endpoint is the + departure *continuing* only once an earlier attempt saw it leave). + ``before_retry`` re-points the client at the endpoint that answers now. + ``deadline`` (a ``time.monotonic()`` instant) caps the whole loop: + ADR-0014 forbids an inner wait outliving the call deadline, and this wait + is the innermost one on the call's path. + """ + started = time.monotonic() + budget_end = started + _daemon_return_budget(deadline) + attempts = 0 + while True: + attempts += 1 + try: + return attempt() + except Exception as e: # noqa: BLE001 - classified on the next line + if not departed(e, attempts): + raise + last = e + remaining = budget_end - time.monotonic() + if remaining <= 0 or attempts >= _DAEMON_RETURN_ATTEMPTS: + final = give_up(last, attempts, time.monotonic() - started) + if final is last: + raise final + raise final from last + if before_retry is not None: + before_retry() + time.sleep(min(_DAEMON_RETURN_BACKOFF_S, remaining)) + + def ensure_executor(sess) -> str: """Ensure the session's executor and return its instance id.""" return _ensure_executor_lease(sess).executor_id @@ -180,14 +373,18 @@ def _run_request_on_executor( # wedged or long-running executor cannot prevent the durable idle clock # from reflecting that a new instruction arrived. reg.touch(sid) - lease = _ensure_executor_lease(sess) + # One absolute deadline for the pre-request work (ADR-0014): the retries in + # `_ensure_executor_lease` / `_connect` are inner waits, so they are capped + # by what the caller's `timeout_ms` leaves rather than by their own budget. + deadline = time.monotonic() + max(request.timeout_ms, 1) / 1000.0 + lease = _ensure_executor_lease(sess, deadline=deadline) # The executor owns the exact outer deadline, including first-call # cold-start. Give only a small framing/delivery margin so its terminal # response wins the race against the local socket timeout. recv_timeout = ( max(request.timeout_ms, 1) / 1000.0 + _RESPONSE_DELIVERY_SLACK_S ) - conn = _connect(sid, timeout=recv_timeout) + conn = _connect(sid, timeout=recv_timeout, deadline=deadline) sent = False interrupted: BaseException | None = None transport_error: Exception | None = None @@ -333,31 +530,56 @@ def _recv_frame(ws, timeout: float | None = None) -> dict: return msg -def _connect(session_id: str, *, timeout: float = 30.0): +def _connect(session_id: str, *, timeout: float = 30.0, + deadline: float | None = None): """Open the data plane: a ws to the daemon's `/exec` relay. ``max_size`` must clear the executor's own frame cap — a screenshot-bearing response is legitimately large and the daemon relays it whole. - No connect retry loop here any more: the daemon accepted our - `ensureExecutor` a moment ago, so it is listening, and the executor-socket - bind race the old retry absorbed is now the *daemon's* problem, handled - inside `exec_relay`. A refusal here means the daemon itself is gone. + A refusal here is the daemon's front door not being open, which is exactly + what a *replacement* daemon looks like for a second or two while it binds + and adopts the session's executor. Retrying is safe (no request frame + exists yet, so nothing can run twice) and every attempt re-resolves the + endpoint through `_exec_ws_url` — reading the state file the replacement + publishes — so a daemon that came back on another port is dialed at its new + address instead of the dead one. The executor-socket bind race the old + client-side loop absorbed is still the *daemon's* problem + (`exec_relay._dial_executor`); this loop is about the daemon itself, and it + is capped by the caller's deadline (ADR-0014). """ - url = _exec_ws_url(session_id) - try: + # The endpoint actually dialed last, for the failure message; the first + # attempt fills it, so it is never empty when `_give_up` runs. + attempted: list[str] = [] + + def _open(): + attempted.append(_exec_ws_url(session_id)) return ws_connect( - url, + attempted[-1], open_timeout=timeout, close_timeout=timeout, max_size=_MAX_FRAME, proxy=None, compression=None, ) - except (WebSocketException, ConnectionClosed, OSError) as e: - raise ExecutorUnavailable( - f"could not open the executor data plane at {url}: {e}" - ) from e + + def _give_up(exc: BaseException, attempts: int, + elapsed: float) -> BrowserwrightError: + return ExecutorUnavailable( + f"could not open the executor data plane at {attempted[-1]} after " + f"{attempts} attempt(s) over {elapsed:.1f}s: {exc}. The daemon " + f"serving session {session_id!r} was replaced or is shutting " + f"down; nothing reached the executor, so retrying is safe. " + f"{_endpoint_state_now()}", + fix="retry the command", + ) + + return _call_until_daemon_returns( + _open, + deadline=deadline, + departed=_endpoint_is_the_daemons, + give_up=_give_up, + ) def _session_id(sess) -> str: diff --git a/src/browserwright/cdp.py b/src/browserwright/cdp.py index 93e91cb..46c3276 100644 --- a/src/browserwright/cdp.py +++ b/src/browserwright/cdp.py @@ -21,7 +21,7 @@ from websockets.exceptions import ConnectionClosed from websockets.sync.client import connect as ws_connect -from .errors import CDPError +from .errors import CDPError, CDPTransportClosed def _rpc_error_fix(method: str, err: object) -> str: @@ -98,8 +98,8 @@ def __init__(self, ws_url: str, connect_timeout: float = 8.0): def send(self, method: str, *, session: Optional[str] = None, **params) -> dict: if self._closed: - raise CDPError(method=method, params=params, - cdp_message=f"ws closed: {self._closed_reason}") + raise CDPTransportClosed(method=method, params=params, + cdp_message=f"ws closed: {self._closed_reason}") with self._lock: mid = self._next_id self._next_id += 1 @@ -113,7 +113,8 @@ def send(self, method: str, *, session: Optional[str] = None, **params) -> dict: self._ws.send(payload) except ConnectionClosed as e: self._closed, self._closed_reason = True, str(e) - raise CDPError(method=method, params=params, cdp_message=str(e)) from e + raise CDPTransportClosed(method=method, params=params, + cdp_message=str(e)) from e # Wait for the reply. deadline = time.monotonic() + 30.0 with self._inflight_cv: @@ -128,8 +129,8 @@ def send(self, method: str, *, session: Optional[str] = None, **params) -> dict: entry = self._inflight.pop(mid, None) self._inflight_meta.pop(mid, None) if self._closed and not entry: - raise CDPError(method=method, params=params, - cdp_message=f"ws closed: {self._closed_reason}") + raise CDPTransportClosed(method=method, params=params, + cdp_message=f"ws closed: {self._closed_reason}") if "error" in entry: err = entry["error"] raise CDPError(method=method, params=params, diff --git a/src/browserwright/daemon/_ipc.py b/src/browserwright/daemon/_ipc.py index a96f778..6ad89d4 100644 --- a/src/browserwright/daemon/_ipc.py +++ b/src/browserwright/daemon/_ipc.py @@ -27,7 +27,7 @@ import stat import tempfile import time -from dataclasses import dataclass +from dataclasses import dataclass, replace from pathlib import Path @@ -693,11 +693,43 @@ class EndpointProbe: status_line: str = "" server: str = "" detail: str = "" + #: How long the probe took, in milliseconds — set by + #: :func:`probe_endpoint_sync`, ``None`` for a probe a caller built by hand + #: (tests, or a subclassed observation). Carried so a record of a decision + #: made on a probe can say what the probes actually cost, which is the + #: difference between "it timed out" and "it timed out in 1501ms". + latency_ms: float | None = None @property def answered(self) -> bool: return self.kind == "ours" + @property + def silent(self) -> bool: + """The socket opened and nothing usable came back. + + ``timeout`` (nothing came back in time) and ``garbage`` (bytes that are + not HTTP at all) both mean the port never gave an answer we could read. + Distinct from ``foreign`` (something answered HTTP, just not our pong) + and from ``refused`` (nothing is listening). + """ + return self.kind in SILENT_PROBE_KINDS + + @property + def timed_out(self) -> bool: + """Nothing came back within the budget — the one kind escalation re-asks. + + ``garbage`` is not slowness: something spoke, it just did not speak + HTTP. Asking again would burn the window for an answer we already have. + """ + return self.kind == "timeout" + + def evidence(self) -> str: + """``timeout@1501ms`` — the kind plus its measured cost, for a record.""" + if self.latency_ms is None: + return self.kind + return f"{self.kind}@{self.latency_ms:.0f}ms" + def describe(self) -> str: """One clause, suitable for inlining into an error message.""" where = f"{self.host}:{self.port}" @@ -715,12 +747,90 @@ def describe(self) -> str: return f"{where} answered with something that is not HTTP" +#: The escalated-probe window: how long the port may stay silent before +#: "unresponsive" is a conclusion rather than a guess. +#: +#: A single short ``/__ping__`` cannot tell a dead daemon from a busy one: on +#: 2026-10-19 two consecutive 1.5s probes timed out on a machine-global daemon +#: that was still accepting ws clients, and a fresh ``serve`` SIGTERMed it two +#: seconds later — it was never wedged, just slow. Silence is therefore measured +#: across this window, which every escalated attempt may spend in full: anything +#: that answers inside it is alive, and only a holder silent for the WHOLE +#: window is called unresponsive. Bounded on purpose — the probe sits on the +#: CLI's hot path, so the worst case for a port that accepts and then says +#: nothing is ``2 x 1.5s`` (``confirm=True``) plus this window. Nothing else pays +#: it: a responsive daemon answers the short probes, a dead one is ``refused``, +#: and neither escalates. +ESCALATED_PROBE_WINDOW_S = 6.0 + +#: Upper bound on escalated attempts. An attempt may spend what is left of the +#: window, so a real timeout normally runs once; the bound exists so an +#: observation that returns *early* can never turn the escalation into a spin. +ESCALATED_PROBE_ATTEMPTS = 3 + +#: Probe kinds that mean "nothing usable came back": a socket that opened and +#: then said nothing, or bytes that were not HTTP. These are exactly the kinds +#: that must never be read as ``foreign`` — "something else is on the port" is +#: an observation, not an inference from silence. +SILENT_PROBE_KINDS = ("timeout", "garbage") + + +def probes_until_answered( + host: str, + port: int, + *, + window: float | None = None, + probe=None, +) -> list[EndpointProbe]: + """Re-probe ``host:port`` inside a bounded window until something answers. + + The one escalation both liveness callers use: ``daemon_lifecycle.diagnose`` + before it calls a silent endpoint unresponsive, and ``serve``'s port reclaim + before it SIGTERMs whoever holds the port. One short probe is not evidence — + a daemon that is merely busy misses it (see + :data:`ESCALATED_PROBE_WINDOW_S`) — so the question is asked again, with a + much larger budget, until either something comes back or the window runs + out. Only a ``timeout`` is re-asked (see :attr:`EndpointProbe.timed_out`): + bytes that were not HTTP are an answer, and asking again would spend the + window for information we already have. + + Returns every probe taken, in order: the last one carries the observation, + and the whole list is the evidence a caller records (kind plus measured + latency) before it acts on it. Never raises. + """ + window = ESCALATED_PROBE_WINDOW_S if window is None else window + probe = probe_endpoint_sync if probe is None else probe + observed: list[EndpointProbe] = [] + deadline = time.monotonic() + window + while len(observed) < ESCALATED_PROBE_ATTEMPTS: + remaining = deadline - time.monotonic() + if remaining <= 0: + break + # The whole remainder, deliberately not a divided slice: a daemon that + # answers 5s into a 6s window must be caught by the attempt that is + # waiting when it answers, and a divided budget would have given up on + # it three times over. + observed.append(probe(host, port, timeout=remaining)) + if not observed[-1].timed_out: + break + return observed + + def probe_endpoint_sync(host: str, port: int, timeout: float = 1.5) -> EndpointProbe: """Classify whatever is on ``host:port``. Never raises. Same request as :func:`ping_status_sync`, but keeps the response instead - of collapsing every non-pong into "no daemon". + of collapsing every non-pong into "no daemon", and stamps how long the + probe took (``latency_ms``). """ + started = time.monotonic() + observed = _probe_endpoint_once(host, port, timeout) + return replace(observed, + latency_ms=round((time.monotonic() - started) * 1000.0, 1)) + + +def _probe_endpoint_once(host: str, port: int, timeout: float) -> EndpointProbe: + """One classification attempt inside ``timeout``. No latency stamping.""" deadline = time.monotonic() + timeout try: sock = socket.create_connection((host, port), timeout=timeout) @@ -852,6 +962,105 @@ def log_lifecycle(event: str, **fields: object) -> None: pass +# ---- attributed shutdown (ADR-0012 rule 5) -------------------------------- + +#: How recently the ``stop`` verb must have recorded its own ``LIFECYCLE stop`` +#: for a SIGTERM to count as that verb's doing. The verb writes the line and +#: then signals, so the real gap is milliseconds; the window only has to +#: survive a loaded machine. +VERB_STOP_ATTRIBUTION_S = 10.0 + +#: Bytes read back from the daemon log when looking for that line. Read from +#: the end: this file is multi-megabyte on a machine that has been up a while, +#: and only the last few seconds can matter. +_LOG_SCAN_BYTES = 65536 + + +def _log_tail_text(limit: int = _LOG_SCAN_BYTES) -> str: + """The last *limit* bytes of the daemon log, decoded (empty when absent).""" + start = 0 + try: + with log_path().open("rb") as fh: + fh.seek(0, os.SEEK_END) + size = fh.tell() + start = max(0, size - limit) + fh.seek(start) + data = fh.read() + except OSError: + return "" + text = data.decode("utf-8", "replace") + # Drop the partial first line of a windowed read. + return text.split("\n", 1)[-1] if start else text + + +def log_tail(lines: int = 12) -> str: + """The last few non-empty lines of the daemon log. + + The CLI-owned daemon's channel: unlike a launchd-owned one it is spawned + with ``stderr=DEVNULL`` (``daemon_lifecycle._spawn_detached``), so this file + is the only place its startup says anything. + """ + tail = [ln for ln in _log_tail_text().splitlines() if ln.strip()][-lines:] + return "\n".join(tail) + + +def stop_recorded_by_a_verb(pid: int, *, now: float | None = None) -> bool: + """Whether a verb's own ``LIFECYCLE stop pid=`` line is still fresh. + + ``stop`` records the stop *before* it signals, so the daemon on the + receiving end can tell "a verb asked for this" from "someone signalled + me" by reading the trail that already exists, instead of inventing a + second mechanism beside it. + """ + import datetime as _dt + import re as _re + + now = time.time() if now is None else now + # `pid=` must end the field: `pid=1234` is not evidence about 12345. + stop_line = _re.compile(rf" LIFECYCLE stop pid={pid}(?=\s|$)") + for line in _log_tail_text().splitlines(): + if not stop_line.search(line): + continue + stamp = line[:24] # _iso_now(): "%Y-%m-%dT%H:%M:%S%z" + try: + at = _dt.datetime.strptime(stamp, "%Y-%m-%dT%H:%M:%S%z").timestamp() + except ValueError: + continue + if 0.0 <= now - at <= VERB_STOP_ATTRIBUTION_S: + return True + return False + + +def record_signal_shutdown(signum: int) -> None: + """ADR-0012 rule 5 for the one stop no verb can record: a signal. + + A stop a verb asked for is already attributed by that verb's own + ``LIFECYCLE stop`` line. A SIGTERM from anywhere else — a script, a shell, + ``kill`` — used to leave nothing behind but ``shutdown requested``: on + 2026-10-19 a dev-checkout script took the machine-global daemon down + mid-call and reconstructing who did it cost hours. So the daemon writes its + own line, naming the signal, its pid and the initiator from its + environment, and skips it when a fresh ``LIFECYCLE stop`` for this pid is + already on disk — a verb's record is never duplicated. + + A ``restart``/``replace`` SIGTERM still gets this line: those verbs record + the *request* before they signal, so the daemon observing the signal is the + only proof it arrived — a different fact from who asked for it. + + Called from the signal handler, before the shutdown sequence starts. + """ + import signal as _signal + + pid = os.getpid() + if stop_recorded_by_a_verb(pid): + return + try: + name = _signal.Signals(signum).name + except ValueError: + name = str(signum) + log_lifecycle("signal", sig=name, pid=pid, initiator=initiator_from_env()) + + #: Collapse window for the "already running" line. launchd's KeepAlive #: respawns a daemon that exits 1 every few seconds forever, and each spawn is #: a fresh process, so the collapse state has to live on disk. diff --git a/src/browserwright/daemon/_stale.py b/src/browserwright/daemon/_stale.py index ee34c02..a233523 100644 --- a/src/browserwright/daemon/_stale.py +++ b/src/browserwright/daemon/_stale.py @@ -199,6 +199,42 @@ def confirmed_stale_holder(ports: list[int]) -> int | None: return None +def unresponsive_daemon(port: int) -> int | None: + """The pid of a *confirmed browserwright* daemon holding TCP ``port`` on OUR + runtime dir (excluding ourselves), or None. + + The identity proof for a daemon that cannot prove itself by answering. Both + of this module's signal rules apply, unchanged and in full: + + - lsof names the pid as holding the port AND its command line is a + browserwright daemon (:func:`pid_is_browserwright_daemon`); + - it shares our runtime dir (:func:`same_runtime_dir_as_us`) — a daemon from + another runtime dir is someone else's live one (the machine-global daemon, + a sibling worktree's e2e daemon), never a target. + + ``port`` must be the endpoint's own port — the socket that should have + answered — never a configured guess, so a stop aimed at one daemon can never + nominate the holder of another's port. + + For the **explicit** verbs only (``stop``, and ``restart``'s incumbent stop): + they were asked for directly, so "this daemon holds the port that should be + answering, is provably browserwright, sits on our runtime dir, and does not + answer" is what their caller needs. The **implicit** path (`serve`'s reclaim) + additionally has to prove the holder *stale* — a single negative ping is not + evidence — so it does not use this. + """ + me = os.getpid() + for pid in port_holder_pids(port): + if pid == me: + continue + if not pid_is_browserwright_daemon(pid): + continue + if not same_runtime_dir_as_us(pid): + continue + return pid + return None + + def reclaim_ports(pid: int, ports: list[int], *, timeout: float = 5.0) -> bool: """SIGTERM ``pid`` and wait until every port in ``ports`` frees; escalate to SIGKILL as a last resort. Returns True once the ports are free. diff --git a/src/browserwright/daemon/cli.py b/src/browserwright/daemon/cli.py index 8dc82a6..0e84eb4 100644 --- a/src/browserwright/daemon/cli.py +++ b/src/browserwright/daemon/cli.py @@ -152,7 +152,8 @@ def _build_parser() -> argparse.ArgumentParser: p_restart = sub.add_parser( "restart", - help="restart the installed LaunchAgent daemon after an upgrade") + help=("restart the daemon after an upgrade: the LaunchAgent if one is " + "installed, else the CLI-owned `serve`")) p_restart.add_argument("--timeout", type=float, default=5.0, help="seconds to wait for the replacement daemon to " "come up and report its version") @@ -480,6 +481,17 @@ def _cmd_stop(args, cfg: Config) -> int: Death is measured by the *ping*, not by the process table: a daemon that has stopped answering has stopped serving, which is all `stop` promises. + **A daemon that stopped answering is still stoppable.** The ping is how this + verb finds the pid; when it fails, the pid is looked up the other way — the + process holding the endpoint's own TCP port, confirmed to be a browserwright + daemon on our runtime dir (`_stale.unresponsive_daemon`). That is the + explicit path acting on proven identity, which is exactly the asymmetry this + closes: it used to SIGTERM nothing at all, so a genuinely wedged daemon could + not be stopped by any verb. `stop`'s identity rule is unchanged and absolute: + a pid is signalled only when it is either the one our endpoint's pong named, + or one lsof names as holding our port with a browserwright cmdline on our + runtime dir. + **Refused for a remote endpoint (ADR-0011).** The whole mechanism rests on "the pid the endpoint reports is a pid on this machine", which was guaranteed while the endpoint was a unix socket in our own runtime dir and @@ -519,15 +531,26 @@ def _cmd_stop(args, cfg: Config) -> int: return 3 pid = _ipc.ping_sync(timeout=1.0) + wedged = False if pid is None: - # No live daemon. Still clean up stale files so the next `serve` can - # bind freshly without manual intervention. - _ipc.cleanup_endpoint() - print("no live daemon; cleaned up stale files", file=sys.stderr) - return 0 + # Nothing answered. Either no daemon, or one holding our port that will + # not serve. The second still has to be stoppable — a wedged daemon is + # the single most likely reason to want `stop` — but only on proof of + # identity, which is what this lookup is (never a stranger). + from . import _stale + + pid = _stale.unresponsive_daemon(endpoint.port) + if pid is None: + # No live daemon. Still clean up stale files so the next `serve` can + # bind freshly without manual intervention. + _ipc.cleanup_endpoint() + print("no live daemon; cleaned up stale files", file=sys.stderr) + return 0 + wedged = True start0 = platforms.proc_start_time(pid) _ipc.log_lifecycle("stop", pid=pid, + probes="ping" if not wedged else "unresponsive", initiator=_ipc.describe_initiator("stop")) def _same_process() -> bool: @@ -537,7 +560,7 @@ def _same_process() -> bool: outcome = supervise.terminate( pid, - is_dead=lambda: _ipc.ping_sync(timeout=0.3) is None, + is_dead=_dead_when_not_serving(wedged, pid), grace=args.timeout, # Nothing left to do after the SIGKILL but clean up and exit, so don't # spend another ping confirming it. @@ -562,6 +585,21 @@ def _same_process() -> bool: return 0 +def _dead_when_not_serving(wedged: bool, pid: int): + """`stop`'s death observable: the daemon no longer serves its endpoint. + + Normally one 0.3s ping. For a daemon that was already not serving when we + found it — the wedged case, whose pid came from the port lookup — that ping + would answer "not serving" both before and after the SIGTERM, so death is + measured against the process table instead: the pid is gone. + """ + from . import _ipc, _stale + + if not wedged: + return lambda: _ipc.ping_sync(timeout=0.3) is None + return lambda: not _stale.pid_alive(pid) + + def _cmd_backend_info(args, cfg: Config) -> int: """Probe the running daemon for its backend identity. Same shape as `BrowserwrightDaemon.getBackendInfo`'s ws response so the mode_b_client diff --git a/src/browserwright/daemon/config.py b/src/browserwright/daemon/config.py index 3bf5ceb..a699e08 100644 --- a/src/browserwright/daemon/config.py +++ b/src/browserwright/daemon/config.py @@ -8,6 +8,13 @@ The toml file is *optional*. If it doesn't parse, we report a UserError up-stream so the CLI can show a clean message instead of a stack trace. + +The toml is the file `--config` names, else `$BD_CONFIG`, else the default +:func:`default_config_path` — `$BS_HOME/config.toml` (default +`~/.browserwright/config.toml`). The default exists so a bind setting has a +durable, per-user home that is neither the LaunchAgent plist nor an environment +variable (ADR-0012 amendment, 2026-10-19); "toml" in the precedence line above +means whichever of the three names it. A missing file is an empty config. """ from __future__ import annotations @@ -302,6 +309,36 @@ def resolved_facade_port(self) -> int: return self.facade_port +#: Filename of the default config, inside the browserwright home. +DEFAULT_CONFIG_FILENAME = "config.toml" + + +def default_config_path() -> Path: + """The config file read when neither `--config` nor `$BD_CONFIG` names one. + + ``$BS_HOME/config.toml`` — beside ``global.md``, ``sessions/`` and + ``site-skills/``, and resolved through + :func:`browserwright.memory.global_mem.home_dir` so this file lands in the + same home as everything else the product persists. + + It exists because a bind setting had nowhere durable to live otherwise: + `facade_host` (the tailnet address a remote consumer dials, with loopback + co-bound) could only be made durable by installing a LaunchAgent, and + `upgrade-global` scrubs `BD_CONFIG`/`BD_FACADE_HOST` before it touches the + global install (ADR-0012 rule 2), so an env var cannot be the home either. + Reading the path from ``$BS_HOME`` is also what keeps the test suite off a + developer's real config (``tests/conftest.py`` redirects `BS_HOME`). + + ``daemon_url`` is deliberately NOT one of the keys this file can set for a + client: endpoint resolution keeps its own sources (`daemon_url._from_toml` + still requires a *named* file), so an auto-discovered file can never + silently re-point every client at another daemon. + """ + from ..memory.global_mem import home_dir + + return home_dir() / DEFAULT_CONFIG_FILENAME + + def _read_toml(path: Path) -> dict: from .errors import UserError @@ -330,8 +367,10 @@ def load( """ e = os.environ if env is None else env - # 1) Optional toml - cfg_path = cli_config_path or e.get("BD_CONFIG") + # 1) Optional toml: a named file wins, else the default one. `$BS_HOME` is + # read from the process environment (`home_dir()`), not from `e` — the same + # way every other consumer of the browserwright home resolves it. + cfg_path = cli_config_path or e.get("BD_CONFIG") or str(default_config_path()) toml: dict = {} if cfg_path: toml = _read_toml(Path(cfg_path).expanduser()) diff --git a/src/browserwright/daemon/errors.py b/src/browserwright/daemon/errors.py index 652e1ed..f749041 100644 --- a/src/browserwright/daemon/errors.py +++ b/src/browserwright/daemon/errors.py @@ -30,5 +30,30 @@ def __init__(self, message: str, attempts: dict[str, str] | None = None): self.attempts = attempts or {} +class DaemonShuttingDown(Unavailable): + """This daemon is on its way out, so it refuses to prepare new work. + + Deliberately its own type with its own words (ADR-0013 rule 3). While a + daemon is replaced, closing the relay makes every readiness wait look like + "no extension is connected" — a lie an agent acts on at chrome://extensions + while the extension was never involved (2026-10-19: three calls died that + way with the browser connected and healthy throughout). + + ``MARKER`` is the half that survives the JSON-RPC error envelope as text; + `_executor/client.py` keys its (safe, pre-request) retry on it, so the + phrase and the client's classifier must stay in step. + """ + + MARKER = "the daemon is shutting down" + + def __init__(self, session_id: str): + super().__init__( + f"{self.MARKER} (it is being replaced or stopped), so it will not " + f"prepare an executor for session {session_id!r}; the browser and " + f"the extension are not involved. Retry the command once it " + f"answers again.", + ) + + class ChromeBinaryNotFound(Unavailable): """launch-chrome could not locate a Chrome binary. Exit code 6.""" diff --git a/src/browserwright/daemon/launchagent.py b/src/browserwright/daemon/launchagent.py index 62133a9..44e1c71 100644 --- a/src/browserwright/daemon/launchagent.py +++ b/src/browserwright/daemon/launchagent.py @@ -421,16 +421,24 @@ def _stop_incumbent(timeout: float) -> dict: This used to be safe by construction: the control socket lived under *our* runtime dir, so anything answering it was our daemon by definition. ADR-0011 replaced that socket with a URL, and a URL can name another machine — where - the pid on the pong is a pid over there. The caller (:func:`restart`) is a - LaunchAgent operation on this machine anyway, so an endpoint that is not - locally signalable (see `DaemonEndpoint.is_locally_signalable`) is refused - outright rather than signalled. A LOCAL daemon bound to a tailnet IP stays - stoppable — it publishes its endpoint into our runtime dir, which is what - makes it ours. + the pid on the pong is a pid over there. The caller (:func:`restart`) + replaces the daemon on *this* machine in both ownership modes (ADR-0012 + rule 7), so an endpoint that is not locally signalable (see + `DaemonEndpoint.is_locally_signalable`) is refused outright rather than + signalled. A LOCAL daemon bound to a tailnet IP stays stoppable — it + publishes its endpoint into our runtime dir, which is what makes it ours. (Issue #44 B is a different case — the relay *ports*, where a stranger's daemon can hold 19989 without being ours. That one is caught by the version check in :func:`restart`'s verification, where refusing is right.) + + **A daemon that stopped answering is still stoppable**, on proven identity: + when the ping fails, the pid comes from `_stale.unresponsive_daemon` — the + process holding the endpoint's own TCP port, whose cmdline is a browserwright + daemon, sharing our runtime dir. That is the same class of proof the implicit + reclaim demands, so the "never signal a process we cannot prove is + browserwright" rule is not weakened by an inch; what changes is that + `restart` no longer has to *fail* on the daemon `restart` exists to fix. """ from . import _ipc, platforms, supervise from ..daemon_url import daemon_endpoint, not_ours_to_signal_message @@ -441,8 +449,14 @@ def _stop_incumbent(timeout: float) -> dict: not_ours_to_signal_message(endpoint, "restart"), exit_code=3) pid = _ipc.ping_sync(timeout=1.0) + wedged = False if pid is None: - return {"stopped": None} + from . import _stale + + pid = _stale.unresponsive_daemon(endpoint.port) + if pid is None: + return {"stopped": None} + wedged = True start0 = platforms.proc_start_time(pid) def _same_process() -> bool: @@ -452,9 +466,17 @@ def _same_process() -> bool: return True return platforms.proc_start_time(pid) == start0 + # Death is normally "the ping stopped answering". For a daemon that was + # already not answering when we found it, that observable is constant, so + # fall back to the process table: the pid is gone. This pid is not one we + # own (launchd does), which is why the start-time `guard` stays armed for + # the escalation either way. + is_dead = ((lambda: _ipc.ping_sync(timeout=0.3) is None) if not wedged + else (lambda: not _stale.pid_alive(pid))) + supervise.terminate( pid, - is_dead=lambda: _ipc.ping_sync(timeout=0.3) is None, + is_dead=is_dead, grace=timeout, kill_grace=0, interval=0.1, @@ -495,20 +517,30 @@ def restart(cfg, *, force: bool = False, timeout: float = 5.0) -> dict: So the post-condition is all three: the job has a pid, it is a different pid, and `/__status__` reports our version. Anything else raises. + **No LaunchAgent is not a failure.** The endpoint has exactly one owner + (ADR-0012 amendment, 2026-10-19), and a *CLI-owned* daemon — the same + `serve`, started detached by this verb or by `daemon_lifecycle.ensure`, + with no plist anywhere — is a supported owner. So the plist is asked for, + not required: with one, the launchd path below runs unchanged; without one, + :func:`_restart_cli_owned` stops the incumbent and starts a `serve`, and + then proves the same three-part post-condition. Requiring `install` here is + what made `mise run upgrade-global` and `mise run restart-daemon` die on + the restart step of a plist-less machine. + ``force`` overrides the "someone is working" gate (:mod:`restart_guard`) and nothing else — it never makes us signal a daemon that is not ours. """ require_darwin("restart") path = plist_path() - if not path.exists(): - raise LaunchAgentError( - "no LaunchAgent installed; run `browserwright-daemon install` " - "or restart your foreground `serve` process manually", 2) + launchd_owned = path.exists() from . import restart_guard from .. import __version__ # The LaunchAgent's binary, not the checkout invoking this command, defines - # the version a healthy incumbent must report (issue #57). + # the version a healthy incumbent must report (issue #57). With no plist + # there is no third-party binary in the picture: the daemon this command + # starts is the `browserwright-daemon` this process is, so the caller's + # version is the expected one. want = expected_version(path, __version__) # ADR-0013 rule 5: restart is gated on DIAGNOSIS, not on session count. @@ -544,6 +576,10 @@ def restart(cfg, *, force: bool = False, timeout: float = 5.0) -> dict: # Resolve what "up to date" means BEFORE tearing anything down, so a plist # we cannot interrogate fails fast instead of after the daemon is already # stopped. + if not launchd_owned: + return _restart_cli_owned(cfg, want=want, force=force, + timeout=timeout, activity=activity) + before = job_state() from . import _ipc _ipc.log_lifecycle( @@ -571,6 +607,7 @@ def restart(cfg, *, force: bool = False, timeout: float = 5.0) -> dict: return { "ok": True, "restarted": str(path), + "owner": "launchd", "pid_before": before.get("pid"), "pid_after": pid, "daemon_version": live_version, @@ -585,6 +622,122 @@ def restart(cfg, *, force: bool = False, timeout: float = 5.0) -> dict: expected=want, timeout=timeout), 3) +def _restart_cli_owned(cfg, *, want: str, force: bool, timeout: float, + activity) -> dict: + """Bounce a CLI-owned daemon: stop the incumbent, start `serve`, prove it. + + The plist-less half of :func:`restart`. Only the *start mechanism* differs: + the launchd path unloads and loads a job, this one spawns `serve` detached + through :func:`browserwright.daemon_lifecycle._spawn_detached` — the one + entry point that starts a daemon without a supervisor — so the new process + inherits this CLI's resolved endpoint and carries an initiator into its own + ``LIFECYCLE start`` line (ADR-0012 rule 5). + + The post-condition is deliberately the launchd path's: an incumbent was + stopped, the pid answering now is a different one, and `/__status__` + reports *want*. A `serve` that cannot bind exits at once (`run_serve` + returns 2) and the incumbent is already gone, so every way of failing here + raises a ``LaunchAgentError`` naming the way — never a silent ok. + """ + from . import _ipc + from .. import daemon_lifecycle + + initiator = _ipc.describe_initiator("restart (cli-owned daemon)") + before_pid = _ipc.ping_sync(timeout=1.0) + _ipc.log_lifecycle("restart", owner="cli", pid_before=before_pid, + forced=bool(force), initiator=initiator) + if before_pid is not None: + # A replacement, not an operator stop: leave resident executors for the + # new daemon to adopt (ADR-0013). + _ipc.request_executor_handoff(before_pid) + stopped = _stop_incumbent(timeout) + child = daemon_lifecycle._spawn_detached( # noqa: SLF001 + ["serve"], initiator=initiator) + + live_pid: int | None = None + live_version: str | None = None + deadline = time.monotonic() + max(timeout, 1.0) + # An exited child is proof enough: `serve` exits 2 when it cannot bind, so + # there is nothing left to wait for. + while child is not None and child.poll() is None: + live_pid = _ipc.ping_sync(timeout=0.3) + if live_pid is not None and live_pid != before_pid: + live_version = _live_daemon_version(cfg) + if live_version == want: + return { + "ok": True, + "restarted": "serve (cli-owned)", + "owner": "cli", + "pid_before": before_pid, + "pid_after": live_pid, + "daemon_version": live_version, + "stopped_incumbent": stopped.get("stopped"), + "forced": bool(force), + "interrupted": list(activity.reasons) if force else [], + } + if time.monotonic() >= deadline: + break + time.sleep(0.2) + + raise LaunchAgentError(_cli_owned_failure_message( + before_pid=before_pid, live_pid=live_pid, live_version=live_version, + expected=want, timeout=timeout, + spawn_missing=child is None, + spawn_exit=None if child is None else child.poll()), 3) + + +def _cli_owned_failure_message(*, before_pid: int | None, + live_pid: int | None, + live_version: str | None, expected: str, + timeout: float, spawn_missing: bool, + spawn_exit: int | None) -> str: + """Name the specific way a CLI-owned restart failed. + + The same reasoning as :func:`_restart_failure_message` — each branch is a + different fix — minus launchd, because there is no job state here to read. + """ + if spawn_missing: + return ( + "restart failed: `browserwright-daemon` is not on PATH, so the " + "CLI-owned daemon could not be started (the incumbent is already " + "stopped). Install it, or run `browserwright-daemon serve` " + "yourself." + ) + if live_pid is None: + tail = _daemon_log_tail() + hint = f"\nLast daemon log lines:\n{tail}" if tail else "" + if spawn_exit is not None: + return ( + f"restart failed: the CLI-owned `serve` exited with status " + f"{spawn_exit} without answering the endpoint — that is what a " + f"bind failure looks like (the port may be held by something " + f"this command may not signal), and the incumbent is already " + f"stopped.{hint}" + ) + return ( + f"restart failed: nothing answered the endpoint within " + f"{timeout:g}s of starting the CLI-owned `serve`.{hint}" + ) + if live_pid == before_pid: + return (f"restart failed: pid {live_pid} is unchanged after " + f"{timeout:g}s — nothing was replaced.") + if live_version is None: + return (f"restart failed: a new process started (pid {live_pid}) but " + f"nothing answered `/__status__` within {timeout:g}s.") + return (f"restart failed: a new process started (pid {live_pid}) but the " + f"daemon answering `/__status__` reports {live_version}, not " + f"{expected}. Either it is still loading old code, or the relay " + f"port is held by a daemon that is not ours (issue #44 B) — this " + f"command will not signal that one.") + + +def _daemon_log_tail(lines: int = 12) -> str: + """Last lines of the daemon's own log file — the CLI-owned daemon's channel.""" + from . import _ipc + + return _ipc.log_tail(lines) + + def restart_endpoint(cfg): """The endpoint `restart` diagnoses: THIS command's configured port. diff --git a/src/browserwright/daemon/probe.py b/src/browserwright/daemon/probe.py index c87cefd..c9e7c07 100644 --- a/src/browserwright/daemon/probe.py +++ b/src/browserwright/daemon/probe.py @@ -172,6 +172,19 @@ def confirmed_stale_holder(self, ports: list[int]) -> int | None: from . import _stale return _stale.confirmed_stale_holder(ports) + def escalated_probes(self, host: str, port: int) -> list: + """Re-probe ``host:port`` until something answers, or the window runs out. + + The escalated evidence `serve`'s reclaim decides on: a single short ping + cannot tell a dead daemon from a busy one (issue #15's reclaim SIGTERMed + a live daemon on exactly that basis on 2026-10-19), so the question is + asked again with a much larger budget. Returns every + :class:`_ipc.EndpointProbe` taken, in order, each carrying its measured + latency; an empty list means there was no address to ask. + """ + from . import _ipc + return _ipc.probes_until_answered(host, port) + def live_pid_file_pid(self) -> int | None: """Best-effort fallback holder hint: the pid file's pid, if still alive. diff --git a/src/browserwright/daemon/server/daemon.py b/src/browserwright/daemon/server/daemon.py index 5bb6cfe..a9b0722 100644 --- a/src/browserwright/daemon/server/daemon.py +++ b/src/browserwright/daemon/server/daemon.py @@ -21,6 +21,7 @@ from ... import session_registry from ..config import Config +from ..errors import DaemonShuttingDown from .upstream_context import ( UnroutableRecord, UpstreamContext, @@ -92,6 +93,13 @@ def __init__(self, *, cfg: Config, shared_context: UpstreamContext, self._session_phases: dict[str, str] = {} self._session_results: dict[str, dict] = {} self._termination_locks: dict[str, asyncio.Lock] = {} + #: Set the instant SIGTERM's graceful shutdown starts, and never + #: cleared. SIGTERM pulls the relay away a few lines later, after which + #: every readiness wait fails — so without this flag the drivable path + #: blames the extension for a browser that is still connected and + #: healthy. The daemon owns it because only the daemon knows why its + #: own relay went quiet (ADR-0013 rule 3). + self.shutting_down = False self._register(shared_context) def _register(self, ctx: UpstreamContext) -> None: @@ -163,6 +171,17 @@ def context_for_required(self, session_id: str) -> UpstreamContext: """Resolve an explicitly session-bound context, failing closed.""" return self.context_for(session_id, require_known=True) + def _refuse_if_shutting_down(self, session_id: str) -> None: + """Refuse new browser work once this process has been told to stop. + + SIGTERM sets ``shutting_down`` before it pulls the relay away, so a + request landing in the shutdown window is answered with the daemon's + own departure instead of the extension diagnosis the readiness wait + would invent afterwards (ADR-0013 rule 3). + """ + if self.shutting_down: + raise DaemonShuttingDown(session_id) + # ---- the drivable path (ADR-0013 rule 1) ------------------------------ async def ensure_session_drivable(self, session_id: str, *, @@ -180,9 +199,14 @@ async def ensure_session_drivable(self, session_id: str, *, runs this inside the executor registry's per-session lifecycle lock. """ ctx = self.context_for_required(session_id) + self._refuse_if_shutting_down(session_id) try: await ctx.upstream.await_browser(session_id) except Exception as e: # noqa: BLE001 + # The readiness wait can straddle the start of shutdown: report + # the daemon's departure rather than the browser its relay can no + # longer reach. + self._refuse_if_shutting_down(session_id) raise RuntimeError(f"upstream readiness: {e}") from e try: await ctx.holder.ensure_open() diff --git a/src/browserwright/daemon/server/listener.py b/src/browserwright/daemon/server/listener.py index 5e66e8a..fea375f 100644 --- a/src/browserwright/daemon/server/listener.py +++ b/src/browserwright/daemon/server/listener.py @@ -55,6 +55,26 @@ def _reclaim_stale_daemon_ports(cfg: Config, *, probe=None) -> None: runtime dir (issue #44 B — that is someone else's live daemon, e.g. the machine-global one; we never SIGTERM a stranger). + Two gates stand between "the ports are held" and a SIGTERM, and they answer + different questions: + + - **which process** (``confirmed_stale_holder`` + ``same_runtime_dir_as_us``): + the pid must be a proven browserwright daemon holding our port on our + runtime dir. Unchanged, and deliberately never loosened. + - **is it actually stale** (``_reclaim_decision``): a negative short ping is + not evidence — on 2026-10-19 one 1.0s ping was negative on a daemon that + was still accepting ws clients, and this function SIGTERMed it two + seconds into the next CLI call. The incumbent is therefore re-probed + across the escalated window (``_ipc.probes_until_answered``, the same + helper and budget ``daemon_lifecycle`` escalates with) and is reclaimed + only when it stays unanswered for all of it, or when nothing is listening + on the endpoint port at all. + + Every decision is written as a ``LIFECYCLE reclaim`` line through + ``_ipc.log_lifecycle`` — a direct file append, so it survives a serve + spawned with ``stdout=DEVNULL, stderr=DEVNULL`` (``daemon_lifecycle.ensure``) + and a daemon whose logging is not wired yet. + The write-side twin of `cli status`: both read the world through the same `probe.DaemonProbe` so they can never disagree about which ports matter or who counts as a confirmed holder. Only the reclaim itself is local, because @@ -70,6 +90,7 @@ def _reclaim_stale_daemon_ports(cfg: Config, *, probe=None) -> None: return pid = p.confirmed_stale_holder(held) if pid is None: + _record_reclaim_decision("no-holder", cfg, ports=held) logger.warning( "ports %s are in use but no confirmed browserwright daemon holds " "them; leaving them alone (bind will surface a clear error)", held) @@ -82,6 +103,9 @@ def _reclaim_stale_daemon_ports(cfg: Config, *, probe=None) -> None: # daemon — never do that; refuse loudly and let the bind (if we actually # need the port) fail with an actionable message instead. if not _stale.same_runtime_dir_as_us(pid): + _record_reclaim_decision( + "refused", cfg, pid=pid, ports=held, + reason="different-runtime-dir") logger.warning( "ports %s are held by browserwright daemon pid %d from a DIFFERENT " "runtime dir (%s) — not a stale daemon of ours, so refusing to " @@ -90,9 +114,19 @@ def _reclaim_stale_daemon_ports(cfg: Config, *, probe=None) -> None: "that daemon yourself.", held, pid, _stale.pid_runtime_dir(pid)) return + observed = _rechallenge_incumbent(p, cfg) + decision, reason, detail = _reclaim_decision(observed) + _record_reclaim_decision(decision, cfg, pid=pid, ports=held, + probes=observed, reason=reason) + if decision != "reclaimed": + logger.warning( + "refusing to reclaim ports %s from browserwright daemon pid %d: %s " + "(%s); leaving it alone, so the bind may fail", + held, pid, detail, _probe_evidence(observed)) + return logger.warning( - "reclaiming ports %s from stale browserwright daemon pid %d (issue #15)", - held, pid) + "reclaiming ports %s from stale browserwright daemon pid %d (issue #15): " + "%s (%s)", held, pid, detail, _probe_evidence(observed)) if _stale.reclaim_ports(pid, held): logger.info("reclaimed ports %s from pid %d", held, pid) else: @@ -100,6 +134,88 @@ def _reclaim_stale_daemon_ports(cfg: Config, *, probe=None) -> None: held, pid) +def _rechallenge_incumbent(p, cfg: Config) -> list: + """Re-probe the endpoint we are about to bind, with the escalated budget. + + Targets the port THIS daemon will bind, on the address local clients use for + it (ADR-0012 rule 6) — the port the holder must answer on if it is alive. + A config that binds an ephemeral port has no address to re-probe, so it + returns nothing and :func:`_reclaim_decision` refuses: nothing can prove + that holder stale. + """ + port = cfg.resolved_facade_port() + if not port: + return [] + return p.escalated_probes(_local_probe_host(cfg.facade_host), port) + + +def _probe_evidence(observed) -> str: + """``timeout@2000ms,timeout@1999ms`` — kinds plus what they cost.""" + return ", ".join(pr.evidence() for pr in observed) or "no probe taken" + + +def _reclaim_decision(observed) -> tuple[str, str, str]: + """``(decision, reason-slug, english)`` from the escalated probes. + + Two shapes authorize a SIGTERM, and nothing else does: + + - the endpoint port stays unanswered for the whole escalated window — a + holder that owns the port but will not serve on it; + - nothing is listening on the endpoint port at all. That is not "slow": a + live process that owns its listening socket cannot refuse a connection, so + this is the half-alive shape (endpoint listener gone, relay port still + bound). It is also what the reclaim did before, and it is what keeps a + genuinely dead daemon's ports reclaimable. + + Anything that *answered* — our pong, someone else's HTTP, non-HTTP bytes — is + never a reclaim target. That is the correction: the short-probe timeout that + used to land here is not an answer of any kind. + """ + kinds = [pr.kind for pr in observed] + if not kinds: + return ("refused", "no-endpoint-port", + "no endpoint port was available to re-probe") + if "ours" in kinds: + return ("refused", "holder-answered", + "the holder answered its ping and is alive") + if "foreign" in kinds: + return ("refused", "non-pong-answer", + "something other than a browserwright pong answered") + if "garbage" in kinds: + return ("refused", "non-http-answer", + "the port answered with bytes that are not HTTP") + if "refused" in kinds: + return ("reclaimed", "nothing-listening", + "nothing is listening on the endpoint port, so it serves " + "nobody") + return ("reclaimed", "silent-window", + "it stayed unanswered for the whole escalated probe window") + + +def _record_reclaim_decision(decision: str, cfg: Config, *, pid: int | None = None, + ports=(), probes=(), reason: str = "") -> None: + """Append the reclaim decision to the daemon log, cost what it may. + + ``_ipc.log_lifecycle`` writes the file directly instead of going through + ``logging``, which is the only reason this record exists when the serve was + spawned with ``stdout=DEVNULL, stderr=DEVNULL``: before it, an + ensure-spawned ``serve`` could SIGTERM a live daemon and leave no trace + anywhere (the warnings went to ``logging.lastResort`` on a devnull stderr, + and the INFO lines had no handler yet). + """ + port = cfg.resolved_facade_port() + _ipc.log_lifecycle( + "reclaim", + decision=decision, + pid=pid if pid is not None else "-", + endpoint=f"{_local_probe_host(cfg.facade_host)}:{port or '-'}", + ports=",".join(str(p) for p in ports) or "-", + probes=",".join(pr.evidence() for pr in probes) or "-", + reason=reason or "-", + initiator=_ipc.initiator_from_env(), + ) + + def _local_probe_host(facade_host: str) -> str: """The address a client ON THIS MACHINE uses for a daemon bound to ``facade_host``: loopback whenever the facade co-binds it (specific @@ -111,11 +227,44 @@ def _local_probe_host(facade_host: str) -> str: return facade_host +def _on_stop_signal(stop: asyncio.Event, signum: int) -> None: + """SIGTERM / SIGINT: attribute the stop, then ask the loop to finish. + + Attribution happens here, before the shutdown sequence begins (ADR-0012 + rule 5): a daemon killed by something other than its own `stop` verb used + to leave no record of it, and reconstructing "who signalled the global + daemon" on 2026-10-19 is what cost hours. `record_signal_shutdown` skips + the line when the `stop` verb already recorded that stop. + """ + _ipc.record_signal_shutdown(signum) + stop.set() + + +def _install_stop_signals(loop, stop: asyncio.Event) -> None: + """Arm SIGTERM and SIGINT on *loop*, both carrying their own signum.""" + for sig in (signal.SIGTERM, signal.SIGINT): + try: + loop.add_signal_handler(sig, _on_stop_signal, stop, sig) + except NotImplementedError: + pass # e.g. inside pytest event loop + + async def run_serve(cfg: Config) -> int: """Run a Mode B daemon until SIGTERM / Ctrl-C / shutdown. Returns exit code. There is exactly one global daemon on a fixed socket — no instance name. """ + # Logging first, before anything that can take a decision worth recording. + # This used to run a few hundred lines in, which left the startup path + # (orphan sweeps, the ledger sweep, the port reclaim and every warning they + # emit) with only `logging.lastResort` — undated, unlabelled, and on an + # ensure-spawned daemon's devnull stderr (ADR-0012 rule 5). The daemon that + # was SIGTERMed mid-call on 2026-10-19 left nothing behind but its own + # stderr startup line. + _wire_logging() + # v0.5: opt-in JSON log formatter. After _wire_logging adds the + # file/console handlers, swap formatters in place if BD_LOG_JSON=1. + install_json_logging_if_requested() # Stale-detect: ping the endpoint before binding. If something answers, # refuse to start a second copy of ourselves (enforces the "at most one # global daemon" invariant). ADR-0011 re-keyed this from the control socket @@ -194,12 +343,6 @@ async def run_serve(cfg: Config) -> int: # swap no longer severs every session's executor. adoptable_executors = cleanup_orphan_executors() - # Log file is best-effort — we route Python logging to it but never crash - # the daemon over a write failure. - _wire_logging() - # v0.5: opt-in JSON log formatter. After _wire_logging adds the - # file/console handlers, swap formatters in place if BD_LOG_JSON=1. - install_json_logging_if_requested() # `kill -USR1 ` dumps every thread's stack into the daemon log. # `ps` answers "who is waiting"; this answers "on what line". Armed before # anything can hang, and a no-op until someone signals. @@ -243,15 +386,7 @@ async def run_serve(cfg: Config) -> int: # we still need to run the graceful shutdown sequence (close clients with # 1011 + emit upstreamClosed + close upstream). stop = asyncio.Event() - - def _on_signal(): - stop.set() - loop = asyncio.get_running_loop() - for sig in (signal.SIGTERM, signal.SIGINT): - try: - loop.add_signal_handler(sig, _on_signal) - except NotImplementedError: - pass # e.g. inside pytest event loop + _install_stop_signals(asyncio.get_running_loop(), stop) # PID file (best-effort). _ipc.write_pid(os.getpid()) @@ -436,20 +571,29 @@ def _cleanup_orphan_cdp_chrome() -> None: def _wire_logging() -> None: - """Route the daemon's logger to a file under TMPDIR. Best-effort.""" + """Route the daemon's logger to a file under TMPDIR. Best-effort. + + Idempotent: a process that calls it twice (a long test run, a serve retried + in-process) keeps one handler per sink instead of doubling every line. + """ try: log_p = _ipc.log_path() log_p.parent.mkdir(parents=True, exist_ok=True) - handler = logging.FileHandler(str(log_p), encoding="utf-8") - handler.setFormatter(logging.Formatter(_LOG_FORMAT, datefmt=_LOG_DATEFMT)) root = logging.getLogger() root.setLevel(logging.INFO) + if any(getattr(h, "_bw_log_path", None) == str(log_p) + for h in root.handlers): + return + handler = logging.FileHandler(str(log_p), encoding="utf-8") + handler.setFormatter(logging.Formatter(_LOG_FORMAT, datefmt=_LOG_DATEFMT)) + handler._bw_log_path = str(log_p) # Also echo to stderr. Under launchd that is the captured # StandardErrorPath file; without a handler here, warnings fell # through to logging's lastResort handler — undated, unlabelled, # which is what the launchd log looked like (ADR-0012 rule 5). echo = logging.StreamHandler(sys.stderr) echo.setFormatter(logging.Formatter(_LOG_FORMAT, datefmt=_LOG_DATEFMT)) + echo._bw_log_path = str(log_p) root.addHandler(echo) root.addHandler(handler) except OSError: @@ -737,6 +881,10 @@ async def _idle_watchdog( async def _graceful_shutdown(daemon: "Daemon") -> None: """Called on SIGTERM. Drain in-flight teardowns, run close etiquette on every context, then close the listener.""" + # First line on purpose: from here the relay (and every readiness wait that + # depends on it) is living on borrowed time, and a request arriving in the + # window must be told the daemon is going, not that the extension is gone. + daemon.shutting_down = True # Consume replacement intent immediately. Draining a teardown may take # longer than the marker's anti-staleness window; validation is about when # SIGTERM arrived, not how long graceful shutdown etiquette takes. diff --git a/src/browserwright/daemon_lifecycle.py b/src/browserwright/daemon_lifecycle.py index 3e77f72..80f81dc 100644 --- a/src/browserwright/daemon_lifecycle.py +++ b/src/browserwright/daemon_lifecycle.py @@ -6,7 +6,9 @@ - :func:`diagnose` → one :class:`DaemonVerdict`. Probes the resolved endpoint (once, or twice with ``confirm=True`` — ADR-0013's two agreeing probes) and - classifies what it found. Side-effect free. + classifies what it found. A probe that ends in silence is escalated before it + is concluded, so a daemon that is merely slow is diagnosed ``up`` / ``stale`` + rather than ``unresponsive``. Side-effect free. - :func:`ensure` → the **only** code that starts or replaces the daemon. It owns the executor handoff, the initiator attribution (ADR-0012 rule 5), the ``LIFECYCLE`` log line, and the child environment. An explicitly configured @@ -33,6 +35,7 @@ from dataclasses import dataclass, field from typing import Optional +from .daemon._ipc import SILENT_PROBE_KINDS from .daemon_url import ( DEFAULT_DAEMON_URL, ENV_VAR, @@ -57,10 +60,20 @@ DOWN = "down" #: Something answers on the port, but it cannot be proven to be browserwright. FOREIGN = "foreign" +#: The port opened a socket and then never answered HTTP at all. NOT +#: :data:`FOREIGN` (which means something *did* answer, and it was not our +#: pong): a silent holder is evidence of nothing yet, so it is never an +#: automatic kill or replacement target. The probes escalate before this is +#: concluded — a daemon that is merely slow answers inside the window and is +#: diagnosed ``up`` / ``stale`` — and it never authorizes a spawn or a stop. +UNRESPONSIVE = "unresponsive" #: Nothing listens on an explicitly configured endpoint (never auto-started). UNREACHABLE = "unreachable" #: ``confirm=True`` and the two probes disagreed; nothing is concluded. UNDECIDED = "undecided" +#: The states :func:`ensure` never acts on: neither one is evidence about a +#: process it would be allowed to start or signal. +NO_ACTION_STATES = (UNRESPONSIVE, UNDECIDED) @dataclass(frozen=True) @@ -110,18 +123,74 @@ def _classify(pr, ep: DaemonEndpoint, installed: str) -> tuple[str, str]: f"is {installed}") return UP, (f"the daemon answers at {ep.authority} (pid {pr.pid}, " f"version {pr.version})") + if pr.kind in SILENT_PROBE_KINDS: + # "Nothing came back" is not "something else is there". Folding the + # two together is what turned one 1.5s timeout into a replacement — + # and a SIGTERM — on a daemon that was alive but slow. + return UNRESPONSIVE, pr.describe() if pr.kind == "refused": return (UNREACHABLE if ep.explicit else DOWN), pr.describe() return FOREIGN, pr.describe() +def _escalate(ep: DaemonEndpoint) -> list: + """Re-probe ``ep`` with the escalated budget, through the ``probe`` seam. + + A short-probe timeout is escalated, never concluded (see + ``_ipc.probes_until_answered``, the helper `serve`'s reclaim uses too). + """ + from .daemon._ipc import probes_until_answered + + return probes_until_answered(ep.host, ep.port, probe=probe) + + +def _diagnose_silent(ep: DaemonEndpoint, installed: str, + silent: list) -> DaemonVerdict: + """The endpoint said nothing: ask again with the escalating budget. + + A daemon that answers inside the window was merely slow, and the verdict is + whatever it says (``up`` / ``stale``). Only silence for the whole window is + concluded — as :data:`UNRESPONSIVE`, whose detail carries the endpoint and + every probe taken (kind and measured latency). A negative found later + (``refused`` in a window that was already silent) does not turn into a + spawn either: a ``serve`` started against a port that answered nothing while + something held it would have refused or lost the race anyway, and the + caller's next probe settles a genuine death. + """ + from .daemon._ipc import ESCALATED_PROBE_WINDOW_S + + escalated = _escalate(ep) + answer = next((p for p in escalated if p.kind == "ours"), None) + if answer is not None: + state, detail = _classify(answer, ep, installed) + return DaemonVerdict( + state=state, endpoint=ep, installed=installed, + detail=(f"{detail}; the first {len(silent)} probe(s) timed out " + "before it answered"), + probes=tuple(p.kind for p in [*silent, *escalated]), + pid=answer.pid, version=answer.version) + observed = [*silent, *escalated] + evidence = ", ".join(p.evidence() for p in observed) or "no probe taken" + return DaemonVerdict( + state=UNRESPONSIVE, endpoint=ep, installed=installed, + detail=(f"no HTTP answer at {ep.authority} over the " + f"{ESCALATED_PROBE_WINDOW_S:.1f}s escalated window " + f"({evidence}); a holder that says nothing is not proven to be " + "browserwright, so nothing is started or signalled on it"), + probes=tuple(p.kind for p in observed)) + + def diagnose(*, confirm: bool = False, endpoint: DaemonEndpoint | None = None, expected_version: str | None = None, timeout: float = 1.5) -> DaemonVerdict: """Classify the daemon at ``endpoint`` (default: the resolved one). ``confirm`` takes two probes and only concludes when they agree — the - ADR-0013 rule for anything that may lead to a replacement. The installed + ADR-0013 rule for anything that may lead to a replacement. A probe that + comes back silent (``timeout`` / ``garbage``) is escalated before anything + is concluded from it: :func:`_diagnose_silent` re-asks with a much larger + budget, so a daemon that is merely slow answers and is classified + ``up`` / ``stale`` instead of being called unresponsive. The installed version is this package's (``expected_version`` overrides it for the LaunchAgent, whose binary may differ from the checkout asking). """ @@ -131,6 +200,8 @@ def diagnose(*, confirm: bool = False, endpoint: DaemonEndpoint | None = None, installed = expected_version or package_version() observed = [probe(ep.host, ep.port, timeout=timeout) for _ in range(2 if confirm else 1)] + if observed and all(p.kind in SILENT_PROBE_KINDS for p in observed): + return _diagnose_silent(ep, installed, observed) kinds = tuple(p.kind for p in observed) conclusions = [_classify(p, ep, installed) for p in observed] if len({c[0] for c in conclusions}) > 1: @@ -163,10 +234,16 @@ def ensure(reason: str, *, wait: float = 10.0) -> DaemonVerdict: repaired: that daemon belongs to whoever configured the URL. - **default endpoint** — ours. ``DOWN`` / ``FOREIGN`` spawn ``serve`` (which refuses beside a live daemon and reclaims ports only from a - process it proves is browserwright, issue #15); ``STALE`` hands the - resident executors over, stops the old daemon and spawns the installed - one; ``UNDECIDED`` does nothing — a daemon mid-transition is not piled - on. After a spawn, waits up to ``wait`` seconds for ``UP``. + process it proves is browserwright and that stayed silent for the whole + escalated probe window, issue #15); ``STALE`` hands the resident + executors over, stops the old daemon and spawns the installed one; + ``UNDECIDED`` and ``UNRESPONSIVE`` do nothing — a daemon + mid-transition, and a port that accepts connections and then says + nothing, are both things no replacement may be started against. (The + unresponsive case used to spawn: the port holder that refused to answer + was mistaken for a foreign one, and on 2026-10-19 that killed a live + machine-global daemon mid-call.) After a spawn, waits up to ``wait`` + seconds for ``UP``. Never raises for the default endpoint: the returned verdict says whether it worked, and each caller decides what a failure means for it. @@ -186,7 +263,7 @@ def ensure(reason: str, *, wait: float = 10.0) -> DaemonVerdict: "configured explicitly, browserwright will not replace it — " "update the daemon on that machine.", file=sys.stderr) return verdict - if verdict.state == UNDECIDED: + if verdict.state in NO_ACTION_STATES: return verdict initiator = _ipc.describe_initiator(f"auto-start ({reason})") @@ -336,7 +413,7 @@ def unreachable_fix(ep: DaemonEndpoint) -> str: """The ``fix`` for "nothing answered" on a NON-explicitly-configured endpoint, built from what the probes observed. - Four observed shapes, each with its own next step; none of them is + Five observed shapes, each with its own next step; none of them is "restart the daemon" — a client that could not connect has no evidence the daemon is at fault, and on 2026-09-01 that advice restarted a healthy daemon out from under another agent (ADR-0012, ADR-0013 rule 3). @@ -366,6 +443,21 @@ def unreachable_fix(ep: DaemonEndpoint) -> str: "it, or move the daemon to another port. `browserwright-daemon " "status` reports what browserwright itself believes is running." ) + # 2b. The port accepted the connection and then said nothing at all — a + # holder that is unresponsive rather than absent, or a listener that is + # not speaking HTTP. Neither is a daemon this client may start over: + # a silent holder can be a live daemon that is merely slow, and this + # message is built on one probe, so it describes rather than concludes. + if first.silent: + return ( + f"{findings}. A process holds {first.host}:{first.port} without " + "serving on it — busy, stuck, or not browserwright at all. That is " + "not a daemon this client will start over; retry the command (a " + "slow daemon answers on its own), and if it keeps happening " + f"`lsof -nP -iTCP:{first.port} -sTCP:LISTEN` names the holder while " + "`browserwright-daemon status` and `browserwright doctor` report " + "what browserwright sees there." + ) # 3. A daemon answers at the resolved address after all (a transient # failure between the caller's attempt and this probe), possibly on # the wrong version. diff --git a/src/browserwright/errors.py b/src/browserwright/errors.py index 5053c51..de87132 100644 --- a/src/browserwright/errors.py +++ b/src/browserwright/errors.py @@ -375,6 +375,17 @@ def __init__(self, method: str = "", params=None, cdp_message: str = "", fix: st super().__init__(f"CDP {method} failed: {cdp_message}", fix=fix) +class CDPTransportClosed(CDPError): + """The daemon's socket died under a request: no reply ever came. + + Distinct from `CDPError` proper, which carries an error object the daemon + *sent* and therefore proves it is alive and said no. That difference is + what a client may base a retry on: a transport death means the daemon went + away (a replacement daemon adopts the session's executor), while a protocol + error would be reproduced by the next attempt against the same daemon. + """ + + class NeedsUserConfirm(BrowserwrightError): """Raised by remember_preference (and similar) when the agent must surface a confirm prompt to the user before re-calling with confirm=False.""" diff --git a/src/browserwright/health.py b/src/browserwright/health.py index 430a75f..043b1ef 100644 --- a/src/browserwright/health.py +++ b/src/browserwright/health.py @@ -140,9 +140,10 @@ def _daemon_fix(info: dict) -> str: "exited before assuming it is stuck" ) return ( - "no LaunchAgent is installed, so nothing keeps the daemon up; " - "`browserwright install` sets one up, and `browserwright session " - "new` starts one on demand on the default endpoint" + "no LaunchAgent is installed, so nothing supervises the daemon — it " + "is CLI-owned, which is a supported mode: `browserwright session new` " + "starts one on demand on the default endpoint, and `browserwright " + "install` makes launchd own it instead (ADR-0012 rule 7)" ) diff --git a/tests/daemon/test_adr0011_exec_client.py b/tests/daemon/test_adr0011_exec_client.py index b16ad6c..52a80bb 100644 --- a/tests/daemon/test_adr0011_exec_client.py +++ b/tests/daemon/test_adr0011_exec_client.py @@ -145,15 +145,22 @@ def test_run_on_executor_round_trips_through_the_relay(harness): def test_a_severed_relay_surfaces_as_executor_unavailable(harness): """A daemon restart now takes live data planes with it. The client must say - so in words an agent can act on, not leak a websockets error.""" + so in words an agent can act on, not leak a websockets error. + + The connect retries a departed daemon inside the caller's deadline + (ADR-0014), so the call budget below is deliberately small: it is the + retry budget here, and the assertions are about the message.""" sess = _Sess() harness.stop_endpoint() with pytest.raises(exec_client.ExecutorUnavailable) as ei: - exec_client.run_on_executor(sess, "print('hi')", timeout_ms=5000) + exec_client.run_on_executor(sess, "print('hi')", timeout_ms=600) msg = str(ei.value) assert "/exec" in msg assert f"127.0.0.1:{harness.port}" in msg + # Names the daemon, and ends with the one instruction that applies. + assert "was replaced or is shutting down" in msg + assert ei.value.fix == "retry the command" def test_ensure_executor_without_readiness_is_refused(harness): diff --git a/tests/daemon/test_adr0012_signal_attribution.py b/tests/daemon/test_adr0012_signal_attribution.py new file mode 100644 index 0000000..b2bc557 --- /dev/null +++ b/tests/daemon/test_adr0012_signal_attribution.py @@ -0,0 +1,197 @@ +"""ADR-0012 rule 5 for the one stop no verb can record: a signal. + +`browserwright-daemon stop` writes its own `LIFECYCLE stop` line before it +signals, so a stop a *verb* asked for is attributable. A SIGTERM from anywhere +else — a script, a shell, `kill` — left the daemon log with only a bare +`shutdown requested`: the daemon that was SIGTERMed mid-call on 2026-10-19 left +nothing behind that named a signal, a pid, or who was running it, and +reconstructing the sequence cost hours. + +These tests pin the daemon's own line, and pin that the CLI's record is not +duplicated when the CLI was the one asking. +""" +from __future__ import annotations + +import asyncio +import os +import re +import signal + +import pytest + +from browserwright.daemon import _ipc +from browserwright.daemon.server import listener + + +@pytest.fixture(autouse=True) +def one_log_file_per_test(monkeypatch, tmp_path): + """`TMPDIR` is redirected suite-wide, but one directory for the whole + session — and these tests count `LIFECYCLE` lines by hand.""" + monkeypatch.setenv("TMPDIR", str(tmp_path)) + return tmp_path + + +def _lifecycle_events() -> list[str]: + """Every `LIFECYCLE …` payload in the daemon log, timestamps + checked on the way past.""" + try: + text = _ipc.log_path().read_text() + except FileNotFoundError: + return [] + events = [] + for line in text.splitlines(): + assert re.match( + r"^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}[+-]\d{4} LIFECYCLE \w+ ", + line), line + events.append(line.split(" LIFECYCLE ", 1)[1]) + return events + + +def _write_stale_stop_line(pid: int) -> None: + """A `LIFECYCLE stop` for *pid* from long before the signal arrives.""" + path = _ipc.log_path() + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text( + f"2000-01-01T00:00:00+0000 LIFECYCLE stop pid={pid} probes=ping " + "initiator=cli:stop cwd=/elsewhere parent='zsh'\n") + + +def test_a_signalled_daemon_records_the_signal_pid_and_initiator(monkeypatch): + """Nobody else will ever record this: the killer writes no log line.""" + monkeypatch.setenv(_ipc.INITIATOR_ENV, "cli:stop cwd=/w parent='zsh'") + stop = asyncio.Event() + + listener._on_stop_signal(stop, signal.SIGTERM) + + assert stop.is_set() + assert _lifecycle_events() == [ + f"signal sig=SIGTERM pid={os.getpid()} initiator=cli:stop cwd=/w " + "parent='zsh'" + ] + + +def test_a_sigint_is_named_as_sigint(monkeypatch): + """Ctrl-C in a terminal is a stop too, and it is a different signal.""" + monkeypatch.setenv(_ipc.INITIATOR_ENV, "cli:serve cwd=/w parent='zsh'") + + listener._on_stop_signal(asyncio.Event(), signal.SIGINT) + + assert "signal sig=SIGINT " in _lifecycle_events()[0] + + +def test_a_stop_the_verb_asked_for_is_not_recorded_twice(monkeypatch): + """`stop` logs `LIFECYCLE stop pid=` and *then* signals (see + `cli._cmd_stop`). The daemon must read that trail rather than write a + second, competing attribution for one event.""" + monkeypatch.setenv(_ipc.INITIATOR_ENV, "cli:stop cwd=/w parent='zsh'") + _ipc.log_lifecycle("stop", pid=os.getpid(), probes="ping", + initiator="cli:stop cwd=/w parent='zsh'") + + stop = asyncio.Event() + listener._on_stop_signal(stop, signal.SIGTERM) + + assert stop.is_set() + assert [event.split()[0] for event in _lifecycle_events()] == ["stop"] + + +def test_a_stop_recorded_for_another_pid_is_not_evidence(monkeypatch): + """A pid that merely starts with ours (`1234` vs `12349`) is not us.""" + monkeypatch.setenv(_ipc.INITIATOR_ENV, "launchd") + _ipc.log_lifecycle("stop", pid=int(f"{os.getpid()}9"), probes="ping", + initiator="cli:stop cwd=/w parent='zsh'") + + listener._on_stop_signal(asyncio.Event(), signal.SIGTERM) + + assert [event.split()[0] for event in _lifecycle_events()] == ["stop", "signal"] + + +def test_a_stale_stop_line_is_not_evidence(monkeypatch): + """The window is what makes it evidence: a stop recorded minutes ago says + nothing about *this* signal.""" + monkeypatch.setenv(_ipc.INITIATOR_ENV, "launchd") + _write_stale_stop_line(os.getpid()) + + listener._on_stop_signal(asyncio.Event(), signal.SIGTERM) + + assert [event.split()[0] for event in _lifecycle_events()] == ["stop", "signal"] + + +def test_both_stop_signals_are_wired_to_the_attributed_handler(): + """`run_serve` arms SIGTERM *and* SIGINT; a handler that only sets the + event is exactly the silence this file exists to prevent.""" + + class _RecordingLoop: + def __init__(self): + self.handlers: dict = {} + + def add_signal_handler(self, sig, callback, *args): + self.handlers[sig] = (callback, args) + + loop, stop = _RecordingLoop(), asyncio.Event() + listener._install_stop_signals(loop, stop) + + assert set(loop.handlers) == {signal.SIGTERM, signal.SIGINT} + for sig, (callback, args) in loop.handlers.items(): + assert callback is listener._on_stop_signal + assert args == (stop, sig) + + callback, args = loop.handlers[signal.SIGINT] + callback(*args) + assert stop.is_set() + assert _lifecycle_events()[0].startswith("signal sig=SIGINT ") + + +def test_a_loop_that_cannot_hold_signal_handlers_is_not_fatal(): + """Unchanged behaviour for a loop without signal support (Windows, and + pytest's own loop): serve continues, it just has no signal handler.""" + + class _NoSignalLoop: + def add_signal_handler(self, *args): + raise NotImplementedError + + listener._install_stop_signals(_NoSignalLoop(), asyncio.Event()) + + +@pytest.mark.asyncio +async def test_run_serve_arms_the_attributed_handler(monkeypatch): + """Wiring, through the real `run_serve`: the handler above is only worth + anything if the daemon arms it, on the event it then waits on. + + Reuses the sibling stubs of `test_stale_daemon_wiring.py:: + test_serve_wires_logging_before_it_can_reclaim` to stop serve at its + endpoint bind. + """ + from browserwright.daemon.config import Config + + armed = [] + + async def fake_ping(timeout=1.0, *, host=None, port=None): + return _ipc.NO_PONG + + class _RefusedEndpoint: + def __init__(self, **kwargs): + pass + + async def start(self): + raise OSError("address already in use") + + monkeypatch.setattr(listener._ipc, "ping_status_async", fake_ping) + monkeypatch.setattr(listener._ipc, "cleanup_endpoint", lambda: None) + # `run_serve` writes the pid file before it binds, and `DaemonProbe` + # reads a pid file as "a daemon died here" (`daemon_traces`) — left behind, + # it would push every later `status` probe in this session into the + # half-alive branch. + monkeypatch.setattr(listener._ipc, "write_pid", lambda pid: None) + monkeypatch.setattr(listener, "_wire_logging", lambda: None) + monkeypatch.setattr(listener, "install_json_logging_if_requested", + lambda: None) + monkeypatch.setattr(listener, "_reclaim_stale_daemon_ports", lambda cfg: None) + monkeypatch.setattr(listener, "_cleanup_orphan_cdp_chrome", lambda: None) + monkeypatch.setattr(listener, "PlaywrightFacade", _RefusedEndpoint) + monkeypatch.setattr(listener, "_install_stop_signals", + lambda loop, stop: armed.append(stop)) + + assert await listener.run_serve(Config(backend="extension")) == 2 + + assert len(armed) == 1 + assert isinstance(armed[0], asyncio.Event) diff --git a/tests/daemon/test_ensure_executor_extension_fastfail.py b/tests/daemon/test_ensure_executor_extension_fastfail.py index 4700264..35b33fd 100644 --- a/tests/daemon/test_ensure_executor_extension_fastfail.py +++ b/tests/daemon/test_ensure_executor_extension_fastfail.py @@ -20,6 +20,7 @@ from browserwright.daemon.config import Config from browserwright.daemon.errors import Unavailable from browserwright.daemon.server import executor_registry as registry_mod +from browserwright.daemon.server import listener from browserwright.daemon.server import extension_upstream as ext_mod from browserwright.daemon.server.daemon import Daemon from browserwright.daemon.server.extension_upstream import ExtensionUpstream @@ -185,3 +186,65 @@ async def test_cdp_skips_extension_fastfail(monkeypatch, tmp_path): assert reply["result"]["ready"] is True assert opens == ["cdp"] assert spawned == [client.session_id] + + +# ---- a shutting-down daemon is not a missing extension --------------------- +# +# 2026-10-19: three agent calls were told "no browserwright extension is +# connected ... chrome://extensions" while the extension was connected and +# healthy — the daemon was being replaced under the call and had pulled its own +# relay away. The daemon now owns that reason explicitly, and these two tests +# hold the two ways a call can meet it: before the readiness wait, and during. + + +@pytest.mark.asyncio +async def test_shutdown_names_the_daemon_never_the_extension(monkeypatch, tmp_path): + """Same session, same unready relay, before and after SIGTERM: only the + reason changed, and the earlier text is the unchanged extension wording.""" + relay = _FakeRelay(ready=False) + ctx, client, sent, spawned, _ = _build( + monkeypatch, tmp_path, "extension", relay=relay) + daemon = ctx.router.daemon + + await _ensure_executor(ctx, client) + before = sent[-1]["error"]["message"] + assert "no browserwright extension is connected" in before + assert "chrome://extensions" in before + assert relay.wait_calls == 1 + + await listener._graceful_shutdown(daemon) + assert daemon.shutting_down is True + + await _ensure_executor(ctx, client) + after = sent[-1]["error"]["message"] + assert "the daemon is shutting down" in after + assert "no browserwright extension is connected" not in after + assert "chrome://extensions" not in after + # The refusal happens before the readiness grace: no second wait_ready. + assert relay.wait_calls == 1 + assert spawned == [] + + +@pytest.mark.asyncio +async def test_shutdown_during_the_readiness_wait_still_names_the_daemon( + monkeypatch, tmp_path): + """A request can straddle the start of shutdown: the wait has already + begun when SIGTERM lands, so the failure it produces must still be + attributed to the daemon that is leaving.""" + relay = _FakeRelay(ready=False) + ctx, client, sent, spawned, _ = _build( + monkeypatch, tmp_path, "extension", relay=relay) + daemon = ctx.router.daemon + + async def _shutdown_mid_wait(timeout: float = 30.0) -> None: + daemon.shutting_down = True + raise asyncio.TimeoutError + + relay.wait_ready = _shutdown_mid_wait + + await _ensure_executor(ctx, client) + + msg = sent[-1]["error"]["message"] + assert "the daemon is shutting down" in msg + assert "no browserwright extension is connected" not in msg + assert spawned == [] diff --git a/tests/daemon/test_issue57_restart_verification.py b/tests/daemon/test_issue57_restart_verification.py index c1d159e..84d77d7 100644 --- a/tests/daemon/test_issue57_restart_verification.py +++ b/tests/daemon/test_issue57_restart_verification.py @@ -12,9 +12,10 @@ import pytest from browserwright.daemon import __version__ as INSTALLED -from browserwright.daemon import launchagent +from browserwright.daemon import _ipc, launchagent from browserwright.daemon.launchagent import LaunchAgentError from browserwright.daemon.restart_guard import Activity +from browserwright import daemon_lifecycle #: A version that is definitely not the installed one. Derived rather than @@ -262,10 +263,172 @@ def test_expected_version_falls_back_when_the_binary_fails(tmp_path): assert launchagent.expected_version(plist, "fallback") == "fallback" -def test_restart_without_a_plist_is_a_distinct_error(harness, tmp_path, - monkeypatch): - missing = tmp_path / "gone.plist" - monkeypatch.setattr(launchagent, "plist_path", lambda: missing) +def test_restart_with_a_plist_still_goes_through_launchd(harness, monkeypatch): + """ADR-0012 amendment: the plist-less mode is new, the launchd path is not. + + With a plist installed, `restart` must keep unloading/loading the job and + must never start a `serve` of its own. + """ + def _must_not_spawn(*args, **kwargs): + raise AssertionError("restart with a plist must not spawn a serve") + + monkeypatch.setattr(daemon_lifecycle, "_spawn_detached", _must_not_spawn) + harness["job_pids"] = [222] + + report = launchagent.restart(_Cfg(), timeout=2.0) + + assert report["ok"] is True + assert report["owner"] == "launchd" + assert report["restarted"] == str(launchagent.plist_path()) + assert [c[0] for c in harness["calls"]] == ["unload", "load"] + + +# ---- CLI-owned mode: no LaunchAgent is a supported owner ------------------- +# +# ADR-0012 amendment (2026-10-19): the endpoint has exactly one owner, and a +# CLI-owned daemon (no plist anywhere) is a supported one. `restart` used to +# refuse with "no LaunchAgent installed" (exit 2) — the step `mise run +# upgrade-global` and `mise run restart-daemon` end on. + + +class _SpawnedServe: + """What `daemon_lifecycle._spawn_detached` hands back.""" + + def __init__(self, exit_status=None): + self._exit = exit_status + + def poll(self): + return self._exit + + +@pytest.fixture +def cli_owned(harness, monkeypatch, tmp_path): + """The plist-less shape: scriptable spawn, pings and stop.""" + monkeypatch.setattr(launchagent, "plist_path", + lambda: tmp_path / "absent.plist") + monkeypatch.setattr(launchagent, "_daemon_log_tail", lambda lines=12: "") + monkeypatch.setattr(launchagent, "_stop_incumbent", + lambda timeout: harness.__setitem__("stops", 1) + or {"stopped": 111}) + harness.update({"spawned": [], "stops": 0, "pings": [111], + "child": _SpawnedServe()}) + + def fake_ping(timeout=1.0): + pings = harness["pings"] + return pings.pop(0) if len(pings) > 1 else pings[0] + + def fake_spawn(args, *, initiator): + harness["spawned"].append((args, initiator)) + return harness["child"] + + monkeypatch.setattr(_ipc, "ping_sync", fake_ping) + monkeypatch.setattr(_ipc, "request_executor_handoff", + lambda pid, **kw: harness.__setitem__("handoff", pid)) + monkeypatch.setattr(daemon_lifecycle, "_spawn_detached", fake_spawn) + return harness + + +def test_restart_without_a_plist_bounces_a_cli_owned_daemon(cli_owned): + """No plist, and the same three-part post-condition as the launchd path: + the incumbent stopped, a different pid answers, `/__status__` reports the + version this CLI is.""" + cli_owned["pings"] = [111, 222] + + report = launchagent.restart(_Cfg(), timeout=2.0) + + assert report["ok"] is True + assert report["owner"] == "cli" + assert report["restarted"] == "serve (cli-owned)" + assert report["pid_before"] == 111 + assert report["pid_after"] == 222 + assert report["daemon_version"] == INSTALLED + assert report["stopped_incumbent"] == 111 + # The incumbent was stopped, its executors were left for adoption, and the + # replacement is a `serve` that names restart as its initiator. + assert cli_owned["stops"] == 1 + assert cli_owned["handoff"] == 111 + assert len(cli_owned["spawned"]) == 1 + args, initiator = cli_owned["spawned"][0] + assert args == ["serve"] + assert initiator.startswith("cli:restart") + # launchd is not involved on a machine that has no job. + assert cli_owned["calls"] == [] + + +def test_cli_owned_restart_still_refuses_a_healthy_daemon(cli_owned, monkeypatch, + make_verdict): + """ADR-0013's diagnosis gate is not a launchd gate.""" + monkeypatch.setattr(daemon_lifecycle, "diagnose", lambda **kw: make_verdict( + detail="the daemon answered twice")) + with pytest.raises(LaunchAgentError) as e: - launchagent.restart(_Cfg()) - assert e.value.exit_code == 2 + launchagent.restart(_Cfg(), timeout=2.0) + + assert e.value.exit_code == 4 + assert "refusing to restart" in str(e.value) + assert cli_owned["stops"] == 0 + assert cli_owned["spawned"] == [] + + +def test_cli_owned_restart_still_refuses_while_someone_is_working(cli_owned): + cli_owned["activity"] = Activity( + blocked=True, determinate=True, + reasons=["executor(s) running: 7"]) + + with pytest.raises(LaunchAgentError) as e: + launchagent.restart(_Cfg(), timeout=2.0) + + assert e.value.exit_code == 4 + assert "nothing was replaced" not in str(e.value) + assert cli_owned["stops"] == 0 + assert cli_owned["spawned"] == [] + + +def test_cli_owned_restart_fails_loudly_when_the_serve_cannot_bind(cli_owned): + """`run_serve` returns 2 when it cannot bind, and the incumbent is already + gone — reporting anything but a failure here would leave the machine with + no daemon and no message.""" + cli_owned["child"] = _SpawnedServe(exit_status=2) + cli_owned["pings"] = [111] + + with pytest.raises(LaunchAgentError) as e: + launchagent.restart(_Cfg(), timeout=2.0) + + assert e.value.exit_code == 3 + assert "exited with status 2" in str(e.value) + assert "bind failure" in str(e.value) + + +def test_cli_owned_restart_fails_when_nothing_answers_after_the_spawn(cli_owned): + cli_owned["pings"] = [None] + + with pytest.raises(LaunchAgentError) as e: + launchagent.restart(_Cfg(), timeout=2.0) + + assert e.value.exit_code == 3 + assert "nothing answered the endpoint" in str(e.value) + + +def test_cli_owned_restart_fails_when_the_serve_binary_is_missing(cli_owned): + cli_owned["child"] = None # `_spawn_detached` returns None when not on PATH + + with pytest.raises(LaunchAgentError) as e: + launchagent.restart(_Cfg(), timeout=2.0) + + assert e.value.exit_code == 3 + assert "not on PATH" in str(e.value) + + +def test_cli_owned_restart_fails_when_the_new_daemon_reports_another_version( + cli_owned): + """A new pid is not new code, plist or no plist.""" + cli_owned["pings"] = [111, 222] + cli_owned["live_version"] = STALE + + with pytest.raises(LaunchAgentError) as e: + launchagent.restart(_Cfg(), timeout=2.0) + + msg = str(e.value) + assert e.value.exit_code == 3 + assert STALE in msg and INSTALLED in msg + assert "#44 B" in msg diff --git a/tests/daemon/test_lifecycle_slow_daemon_is_not_replaced.py b/tests/daemon/test_lifecycle_slow_daemon_is_not_replaced.py new file mode 100644 index 0000000..3d6896d --- /dev/null +++ b/tests/daemon/test_lifecycle_slow_daemon_is_not_replaced.py @@ -0,0 +1,389 @@ +"""A slow daemon is not a dead daemon, and is never replaced on a timeout. + +The 2026-10-28 incident: a machine-global `browserwright-daemon` that was alive +— still accepting ws clients, shutting down gracefully on the signal — was +replaced because two consecutive 1.5s `/__ping__` probes timed out. It had been +idle for ~53 minutes and was momentarily slow. Two agreeing timeouts were read +as "someone else holds the port" (`FOREIGN`), which `ensure()` treats as +"spawn a replacement". + +The verdict states are the contract: + +- ``foreign`` — something answered HTTP that is not our pong. Never replaced. +- ``down`` — nothing is listening (`refused`). Replaced. +- ``stale`` — our daemon, wrong version, answered. Replaced. +- ``unresponsive`` — the socket opened and nothing came back, for the whole + escalated window. **Nothing is started or signalled**, the same early return + `undecided` gets. A merely-slow daemon answers inside that window and is + diagnosed `up` / `stale` instead. + +`ensure` is the only code allowed to start or replace a daemon, so these drive +the genuine one with its side-effecting adapters (`_spawn_detached`, `run_verb`, +`_ipc.log_lifecycle`, `_ipc.request_executor_handoff`) substituted — the shape +`tests/skill/test_adr0011_daemon_url.py` established for the same reason: the +repo-wide wall in `tests/conftest.py` replaces `ensure` itself. +""" +from __future__ import annotations + +import json +import socket +import threading + +import pytest + +from browserwright import daemon_lifecycle as lifecycle +from browserwright.daemon import _ipc +from browserwright.daemon._ipc import EndpointProbe, probe_endpoint_sync + +#: The genuine `ensure`, captured before the wall stubs it out. +_REAL_ENSURE = lifecycle.ensure + +#: A budget that is short enough to keep the suite fast and wide enough that a +#: hand-built fake listener must genuinely answer *late*. +_ESCALATED_WINDOW_S = 1.0 +_SLOW_ANSWER_AFTER_S = 0.25 + + +@pytest.fixture(autouse=True) +def _fast_escalation(monkeypatch): + """Shrink the escalated window. The real 6s is a latency budget, not a + contract; the behaviour under test is \"answers inside the window or not\".""" + monkeypatch.setattr(_ipc, "ESCALATED_PROBE_WINDOW_S", _ESCALATED_WINDOW_S, + raising=False) + + +@pytest.fixture(autouse=True) +def _record_adapters(monkeypatch): + """Substitute `ensure`'s side-effecting adapters; return what they saw.""" + actions: list = [] + monkeypatch.setattr(lifecycle, "_spawn_detached", + lambda args, *, initiator: actions.append( + ("spawn", args, initiator))) + monkeypatch.setattr(lifecycle, "run_verb", + lambda args, **kw: actions.append( + ("run", args, kw.get("initiator")))) + monkeypatch.setattr(_ipc, "request_executor_handoff", + lambda pid: actions.append(("handoff", pid))) + monkeypatch.setattr(_ipc, "log_lifecycle", + lambda event, **fields: actions.append( + ("log", event, fields))) + return actions + + +def _timeout_probe(host, port, timeout=1.5): + return EndpointProbe(kind="timeout", host=host, port=port, + latency_ms=timeout * 1000.0) + + +def _refused_probe(host, port, timeout=1.5): + return EndpointProbe(kind="refused", host=host, port=port, latency_ms=1.0) + + +def _silent_then(monkeypatch, answer: EndpointProbe): + """Script `probe` so the first two calls time out and the escalated re-probe + finds `answer`.""" + calls = {"n": 0} + + def fake_probe(host, port, timeout=1.5): + calls["n"] += 1 + if calls["n"] <= 2: + return EndpointProbe(kind="timeout", host=host, port=port, + latency_ms=timeout * 1000.0) + return answer + + monkeypatch.setattr(lifecycle, "probe", fake_probe) + + +# ---- (a) a live-but-slow endpoint is not replaceable ----------------------- + + +def test_a_silent_endpoint_is_unresponsive_not_foreign(monkeypatch): + """The defect, at the classifier: silence used to be folded into FOREIGN.""" + monkeypatch.delenv("BW_DAEMON_URL", raising=False) + monkeypatch.setattr(lifecycle, "probe", _timeout_probe) + + verdict = lifecycle.diagnose(confirm=True) + + assert verdict.state == lifecycle.UNRESPONSIVE + assert verdict.state != lifecycle.FOREIGN + assert verdict.replaceable is False + assert verdict.up is False + # The detail is evidence: the endpoint, every probe kind, and its latency. + assert verdict.endpoint.authority in verdict.detail + assert verdict.detail.count("timeout@") == len(verdict.probes) + assert len(verdict.probes) > 2 # the short pair, then the escalated re-asks + assert "1500ms" in verdict.detail + assert set(verdict.probes) == {"timeout"} + + +def test_ensure_starts_nothing_for_an_unresponsive_endpoint(monkeypatch, + _record_adapters): + monkeypatch.delenv("BW_DAEMON_URL", raising=False) + monkeypatch.setattr(lifecycle, "probe", _timeout_probe) + + verdict = _REAL_ENSURE("session new") + + assert verdict.state == lifecycle.UNRESPONSIVE + assert _record_adapters == [] # no spawn, no stop, no LIFECYCLE line + + +def test_ensure_starts_nothing_for_an_endpoint_answering_garbage(monkeypatch, + _record_adapters): + """Bytes that are not HTTP are an answer too — never a replacement cue.""" + monkeypatch.delenv("BW_DAEMON_URL", raising=False) + monkeypatch.setattr(lifecycle, "probe", lambda h, p, timeout=1.5: EndpointProbe( + kind="garbage", host=h, port=p, detail="SSH-2.0-OpenSSH")) + + verdict = _REAL_ENSURE("session new") + + assert verdict.state == lifecycle.UNRESPONSIVE + assert _record_adapters == [] + + +# ---- (b) escalation rescues a slow-but-answering daemon ------------------- + + +@pytest.mark.parametrize(("reported_version", "state"), [ + (None, lifecycle.UP), # `None` → the installed version + ("0.0.1-old", lifecycle.STALE), +]) +def test_escalation_diagnoses_a_slow_daemon_up(monkeypatch, reported_version, + state): + from browserwright.version import package_version + + monkeypatch.delenv("BW_DAEMON_URL", raising=False) + version = package_version() if reported_version is None else reported_version + _silent_then(monkeypatch, EndpointProbe( + kind="ours", host="127.0.0.1", port=1, pid=4321, version=version, + latency_ms=612.0)) + + verdict = lifecycle.diagnose(confirm=True) + + assert verdict.state == state + assert verdict.up is True + assert verdict.pid == 4321 + assert "timed out" in verdict.detail and "before it answered" in verdict.detail + + +def test_escalation_saves_a_slow_daemon_from_a_version_skew_replacement( + monkeypatch, _record_adapters): + """The incident's shape exactly: version skew is a *replace* trigger, so a + timeout misread as `foreign`/`stale` replaces a daemon that answers late.""" + from browserwright.version import package_version + + monkeypatch.delenv("BW_DAEMON_URL", raising=False) + _silent_then(monkeypatch, EndpointProbe( + kind="ours", host="127.0.0.1", port=1, pid=4321, + version=package_version(), latency_ms=900.0)) + + verdict = _REAL_ENSURE("session new") + + assert verdict.healthy is True + assert _record_adapters == [] + + +def test_a_real_late_answering_listener_is_diagnosed_up(monkeypatch, + tmp_path): + """Unstubbed: a real socket that accepts and answers only after the short + probe's budget has already expired. The first probe must time out and the + escalated one must find the pong — otherwise this is not testing anything.""" + monkeypatch.delenv("BW_DAEMON_URL", raising=False) + # Own runtime dir: the endpoint state file this publishes is how `diagnose` + # finds the server WITHOUT making the endpoint explicit (an explicit one is + # never started or replaced, which is the opposite of the case here). + monkeypatch.setenv("XDG_RUNTIME_DIR", str(tmp_path)) + host, port, thread = _late_pong_server(_SLOW_ANSWER_AFTER_S) + try: + _ipc.write_endpoint_state(f"http://{host}:{port}") + monkeypatch.setattr(lifecycle, "probe", probe_endpoint_sync) + + first = probe_endpoint_sync(host, port, timeout=0.05) + assert first.kind == "timeout", first + + verdict = lifecycle.diagnose(confirm=True, expected_version="1.2.3") + assert verdict.state == lifecycle.UP + assert verdict.pid == 987 + finally: + thread.join(2.0) + + +def _late_pong_server(delay: float): + """A loopback server that accepts and then answers the pong ``delay`` later. + + One connection per probe attempt, so the escalated re-probe is a genuinely + separate request.""" + srv = socket.socket() + srv.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) + srv.bind(("127.0.0.1", 0)) + srv.listen(8) + srv.settimeout(5.0) + host, port = srv.getsockname() + + def serve_one(conn): + with conn: + conn.settimeout(2.0) + try: + conn.recv(4096) + except OSError: + pass + threading.Event().wait(delay) + body = json.dumps({"pong": True, "pid": 987, "version": "1.2.3"}).encode() + try: + conn.sendall( + b"HTTP/1.1 200 OK\r\nContent-Type: application/json\r\n" + b"Content-Length: " + str(len(body)).encode() + b"\r\n\r\n" + + body) + except OSError: + pass + + def run(): + try: + while True: + try: + conn, _ = srv.accept() + except OSError: + return + threading.Thread(target=serve_one, args=(conn,), + daemon=True).start() + finally: + srv.close() + + t = threading.Thread(target=run, daemon=True) + t.start() + return host, port, t + + + +def test_probes_until_answered_stops_at_the_first_answer(): + observed = [ + EndpointProbe(kind="timeout", host="127.0.0.1", port=1, latency_ms=1000.0), + EndpointProbe(kind="ours", host="127.0.0.1", port=1, pid=7, + version="1.0.0", latency_ms=12.0), + ] + calls = [] + + def fake_probe(host, port, *, timeout): + calls.append(timeout) + return observed[len(calls) - 1] + + result = _ipc.probes_until_answered("127.0.0.1", 1, probe=fake_probe) + + assert [pr.kind for pr in result] == ["timeout", "ours"] + assert len(calls) == 2 + + +def test_probes_until_answered_is_bounded_and_never_asks_garbage_twice(): + garbage = EndpointProbe(kind="garbage", host="127.0.0.1", port=1, + latency_ms=3.0) + calls = [] + + def fake_probe(host, port, *, timeout): + calls.append(timeout) + return garbage + + result = _ipc.probes_until_answered("127.0.0.1", 1, probe=fake_probe) + + assert len(result) == 1 and len(calls) == 1 + + silent = EndpointProbe(kind="timeout", host="127.0.0.1", port=1, + latency_ms=1.0) + calls.clear() + result = _ipc.probes_until_answered("127.0.0.1", 1, probe=( + lambda h, p, *, timeout: calls.append(timeout) or silent)) + assert len(calls) == _ipc.ESCALATED_PROBE_ATTEMPTS + assert len(result) == _ipc.ESCALATED_PROBE_ATTEMPTS + + +# ---- (d) the legitimate paths are untouched ------------------------------- + + +def test_a_refused_endpoint_is_still_down_and_spawns(monkeypatch, + _record_adapters): + monkeypatch.delenv("BW_DAEMON_URL", raising=False) + monkeypatch.setattr(lifecycle, "probe", _refused_probe) + + verdict = _REAL_ENSURE("session new") + + assert verdict.state == lifecycle.DOWN + assert [a[:2] for a in _record_adapters] == [ + ("log", "spawn"), ("spawn", ["serve"])] + + +def test_a_version_skewed_daemon_is_still_replaced(monkeypatch, + _record_adapters): + monkeypatch.delenv("BW_DAEMON_URL", raising=False) + monkeypatch.setattr(lifecycle, "probe", lambda h, p, timeout=1.5: EndpointProbe( + kind="ours", host=h, port=p, pid=31, version="0.0.1-old")) + + _REAL_ENSURE("recover session=7") + + assert [a[:2] for a in _record_adapters] == [ + ("log", "replace"), ("handoff", 31), ("run", ["stop"]), + ("spawn", ["serve"])] + + +# ---- (e) a real HTTP answer is still FOREIGN, which is never replaced ----- + + +def test_a_real_http_answer_is_still_foreign_and_never_replaced( + monkeypatch, tmp_path, _record_adapters): + """A proxy answering 503 on our port is still FOREIGN — diagnosed, and never + a stop or a replacement target. + + ``ensure`` does still try a ``serve`` for it, exactly as before: the new + process is what politely refuses beside a live daemon, and `serve`'s own + reclaim will not signal an unproven holder (`_reclaim_decision` in + `listener.py`). What must never happen — and is what `recover` gates on via + ``replaceable`` — is a stop or a version replacement. + """ + monkeypatch.delenv("BW_DAEMON_URL", raising=False) + monkeypatch.setenv("XDG_RUNTIME_DIR", str(tmp_path)) + host, port, thread = _http_503_server() + try: + _ipc.write_endpoint_state(f"http://{host}:{port}") + monkeypatch.setattr(lifecycle, "probe", probe_endpoint_sync) + + verdict = _REAL_ENSURE("session new", wait=0.0) + assert verdict.state == lifecycle.FOREIGN + assert verdict.replaceable is False + assert "503" in verdict.detail + assert set(verdict.probes) == {"foreign"} + assert [a for a in _record_adapters if a[0] in ("run", "handoff")] == [] + finally: + thread.join(2.0) + + +def _http_503_server(): + """A real one-shot server that answers an HTTP 503 to every request.""" + srv = socket.socket() + srv.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) + srv.bind(("127.0.0.1", 0)) + srv.listen(8) + srv.settimeout(5.0) + host, port = srv.getsockname() + + def run(): + try: + while True: + try: + conn, _ = srv.accept() + except OSError: + return + with conn: + conn.settimeout(2.0) + try: + conn.recv(4096) + except OSError: + pass + try: + conn.sendall( + b"HTTP/1.1 503 Service Unavailable\r\n" + b"Server: Surge/5.0\r\nContent-Length: 0\r\n\r\n") + except OSError: + pass + finally: + srv.close() + + t = threading.Thread(target=run, daemon=True) + t.start() + return host, port, t diff --git a/tests/daemon/test_phase_b_executor_unit.py b/tests/daemon/test_phase_b_executor_unit.py index 0e2e9cb..77f2052 100644 --- a/tests/daemon/test_phase_b_executor_unit.py +++ b/tests/daemon/test_phase_b_executor_unit.py @@ -87,7 +87,7 @@ class _Sess: monkeypatch.setattr( executor_client, "_ensure_executor_lease", - lambda sess: order.append("ensure") or (_ for _ in ()).throw( + lambda sess, **_kw: order.append("ensure") or (_ for _ in ()).throw( ExecutorUnavailable("stop after ensure") ), ) @@ -112,12 +112,12 @@ def close(self): monkeypatch.setattr( executor_client, "_ensure_executor_lease", - lambda _sess: executor_client.ExecutorLease("sess-env", "executor-env"), + lambda _sess, **_kw: executor_client.ExecutorLease("sess-env", "executor-env"), ) monkeypatch.setattr( executor_client, "_connect", - lambda _sid, *, timeout: _Connection(), + lambda _sid, **_kw: _Connection(), ) monkeypatch.setattr( executor_client, @@ -168,7 +168,7 @@ def close(self): monkeypatch.setattr( executor_client, "_ensure_executor_lease", - lambda _sess: executor_client.ExecutorLease("sess-client", "executor-client"), + lambda _sess, **_kw: executor_client.ExecutorLease("sess-client", "executor-client"), ) monkeypatch.setattr(executor_client, "_connect", lambda *_a, **_kw: _Connection()) monkeypatch.setattr(executor_client, "_send_frame", lambda *_a: None) diff --git a/tests/daemon/test_phase_c_foundation_unit.py b/tests/daemon/test_phase_c_foundation_unit.py index fc82446..b66512c 100644 --- a/tests/daemon/test_phase_c_foundation_unit.py +++ b/tests/daemon/test_phase_c_foundation_unit.py @@ -20,6 +20,7 @@ DEFAULT_FACADE_HOST, DEFAULT_FACADE_PORT, Config, + default_config_path, load, ) @@ -89,6 +90,61 @@ def test_facade_host_precedence_cli_over_env_over_toml(tmp_path): assert cfg_cli.facade_host == "100.72.20.32" +# ---- the default config file (ADR-0012 amendment: durable bind config) ---- +# +# `facade_host` (the tailnet address a remote consumer dials, with loopback +# co-bound) had no durable home that was neither the LaunchAgent plist nor an +# environment variable: `upgrade-global` scrubs BD_CONFIG / BD_FACADE_HOST +# before it touches the global install. `$BS_HOME/config.toml` is that home, +# and it must follow `BS_HOME` or the suite reads the developer's real config. + + +def test_default_config_file_follows_BS_HOME(monkeypatch, tmp_path): + """The suite's isolation depends on this: `tests/conftest.py` redirects + `BS_HOME`, so a default that ignored it would make `mise run test` read + `~/.browserwright/config.toml`.""" + monkeypatch.setenv("BS_HOME", str(tmp_path)) + assert default_config_path() == (tmp_path / "config.toml").resolve() + + +def test_default_config_file_is_honoured_when_nothing_names_one( + monkeypatch, tmp_path): + monkeypatch.setenv("BS_HOME", str(tmp_path)) + (tmp_path / "config.toml").write_text('facade_host = "100.72.20.32"\n') + + cfg = load(env={}) + assert cfg.facade_host == "100.72.20.32" + + +def test_default_config_file_is_ignored_when_BD_CONFIG_names_another( + monkeypatch, tmp_path): + home = tmp_path / "home" + home.mkdir() + (home / "config.toml").write_text('facade_host = "10.0.0.1"\n') + named = tmp_path / "named.toml" + named.write_text('facade_host = "10.0.0.9"\n') + monkeypatch.setenv("BS_HOME", str(home)) + + assert load(env={"BD_CONFIG": str(named)}).facade_host == "10.0.0.9" + assert load(cli_config_path=str(named), env={}).facade_host == "10.0.0.9" + + +def test_a_missing_default_config_file_is_an_empty_config(monkeypatch, tmp_path): + monkeypatch.setenv("BS_HOME", str(tmp_path / "nothing-here")) + + cfg = load(env={}) + assert cfg.facade_host == DEFAULT_FACADE_HOST + assert cfg.facade_port is None + + +def test_env_and_cli_still_top_the_default_config_file(monkeypatch, tmp_path): + monkeypatch.setenv("BS_HOME", str(tmp_path)) + (tmp_path / "config.toml").write_text('facade_host = "10.0.0.1"\n') + + assert load(env={"BD_FACADE_HOST": "10.0.0.2"}).facade_host == "10.0.0.2" + assert load(env={}, cli_facade_host="10.0.0.3").facade_host == "10.0.0.3" + + def test_session_idle_prune_loads_from_toml_and_env(tmp_path): from browserwright.daemon.config import load diff --git a/tests/daemon/test_stale_daemon_wiring.py b/tests/daemon/test_stale_daemon_wiring.py index 73a482d..6a4d6ac 100644 --- a/tests/daemon/test_stale_daemon_wiring.py +++ b/tests/daemon/test_stale_daemon_wiring.py @@ -15,9 +15,13 @@ from __future__ import annotations import json +import re from types import SimpleNamespace +import pytest + from browserwright.daemon import _ipc, _stale, cli +from browserwright.daemon._ipc import EndpointProbe from browserwright.daemon.config import Config from browserwright.daemon.probe import DaemonProbe from browserwright.daemon.server import listener @@ -122,12 +126,17 @@ def live_pid_file_pid(self): class _PortProbe(DaemonProbe): - def __init__(self, *, ports, held=None, holder=None): + def __init__(self, *, ports, held=None, holder=None, escalated=None): super().__init__(Config()) self._ports = list(ports) self._held = list(ports if held is None else held) self._holder = holder + #: What the escalated re-probe observes. The default is a holder that + #: stayed unanswered for the whole window — the only shape besides + #: "nothing is listening" that authorizes a reclaim. + self._escalated = ([_silent()] if escalated is None else list(escalated)) self.holder_lookups = [] + self.escalations = [] def daemon_ports(self): return list(self._ports) @@ -139,6 +148,20 @@ def confirmed_stale_holder(self, ports): self.holder_lookups.append(list(ports)) return self._holder + def escalated_probes(self, host, port): + self.escalations.append((host, port)) + return list(self._escalated) + + +def _silent(latency_ms=2000.0, kind="timeout"): + return EndpointProbe(kind=kind, host="127.0.0.1", port=39990, + latency_ms=latency_ms) + + +def _answered(pid=999, version="9.9.9"): + return EndpointProbe(kind="ours", host="127.0.0.1", port=39990, pid=pid, + version=version, latency_ms=41.0) + def test_reclaim_kills_confirmed_stale_daemon(monkeypatch): reclaimed = [] @@ -201,6 +224,203 @@ def test_reclaim_noop_when_ports_free(): assert probe.holder_lookups == [] # never even looked for a holder +# ---- 2.2b: a negative short ping is not enough to SIGTERM a holder -------- +# +# The 2026-10-28 incident: a machine-global daemon that was alive (still +# accepting ws clients, shutting down gracefully on the signal) got SIGTERMed +# 2s after two 1.5s `/__ping__` probes timed out. The reclaim's only staleness +# evidence was the caller's own single 1.0s ping. These pin the escalating +# evidence it now needs, and the record that outlives a /dev/null stderr. + + +def test_reclaim_refuses_a_holder_that_answers_the_escalated_probe(monkeypatch): + """The incident, replayed: short ping silent, daemon answers on re-probe. + + A slow-but-live daemon must never be SIGTERMed, however little its first + ping said. + """ + signalled = [] + monkeypatch.setattr(_stale, "reclaim_ports", + lambda pid, ports: signalled.append(pid) or True) + monkeypatch.setattr(_stale, "same_runtime_dir_as_us", lambda pid: True) + + listener._reclaim_stale_daemon_ports( + Config(), probe=_PortProbe(ports=[29971], holder=999, + escalated=[_silent(1500.0), _answered()])) + + assert signalled == [] + + +def test_reclaim_refuses_an_incumbent_that_answered_non_http(monkeypatch): + """Bytes that are not HTTP are an ANSWER, not silence — never a kill.""" + signalled = [] + monkeypatch.setattr(_stale, "reclaim_ports", + lambda pid, ports: signalled.append(pid) or True) + monkeypatch.setattr(_stale, "same_runtime_dir_as_us", lambda pid: True) + + listener._reclaim_stale_daemon_ports( + Config(), probe=_PortProbe(ports=[29971], holder=999, + escalated=[_silent(kind="garbage")])) + + assert signalled == [] + + +def test_reclaim_still_reclaims_a_half_alive_holder(monkeypatch): + """The shape issue #15 exists for: the endpoint listener is gone (refused) + while the relay port is still bound by a provably stale daemon. + + A live process cannot refuse a connection to a socket it holds, so this is + the half-alive case, not the slow one — reclaiming is correct and must stay + working. + """ + reclaimed = [] + monkeypatch.setattr(_stale, "reclaim_ports", + lambda pid, ports: reclaimed.append((pid, ports)) or True) + monkeypatch.setattr(_stale, "same_runtime_dir_as_us", lambda pid: True) + monkeypatch.setattr(_stale, "pid_runtime_dir", lambda pid: "/tmp") + + listener._reclaim_stale_daemon_ports( + Config(), probe=_PortProbe( + ports=[29971, 29972], holder=999, + escalated=[EndpointProbe(kind="refused", host="127.0.0.1", + port=29972, latency_ms=1.0)])) + + assert reclaimed == [(999, [29971, 29972])] + + +def test_reclaim_names_the_endpoint_it_rechallenged(monkeypatch): + """The re-probe targets the port and local client address THIS serve binds + (ADR-0012 rule 6), not whatever endpoint the shell resolves.""" + monkeypatch.setattr(_stale, "reclaim_ports", lambda pid, ports: True) + monkeypatch.setattr(_stale, "same_runtime_dir_as_us", lambda pid: True) + monkeypatch.setattr(_stale, "pid_runtime_dir", lambda pid: "/tmp") + cfg = Config() + cfg.facade_host = "100.72.20.32" + cfg.facade_port = 39990 + probe = _PortProbe(ports=[39990], holder=999) + + listener._reclaim_stale_daemon_ports(cfg, probe=probe) + + # The facade co-binds loopback, so that is what a local client sees. + assert probe.escalations == [("127.0.0.1", 39990)] + + +def test_reclaim_records_its_decision_even_with_the_logger_unwired( + monkeypatch, tmp_path): + """The record must exist without a logging handler. + + An ensure-spawned `serve` gets stdout/stderr = DEVNULL, and the reclaim runs + before logging is wired; before this, an automatic SIGTERM left no trace + anywhere. The decision carries the pid, the probe kinds with their measured + latencies, and which way it went. + """ + monkeypatch.setenv("TMPDIR", str(tmp_path)) + monkeypatch.setattr(_stale, "reclaim_ports", lambda pid, ports: True) + monkeypatch.setattr(_stale, "same_runtime_dir_as_us", lambda pid: True) + + listener._reclaim_stale_daemon_ports( + Config(), probe=_PortProbe(ports=[29971], holder=999, + escalated=[_silent(2001.0), + _silent(1999.0)])) + + line = _reclaim_lines(tmp_path)[-1] + assert "decision=reclaimed" in line + assert "pid=999" in line + assert "probes=timeout@2001ms,timeout@1999ms" in line + assert "reason=silent-window" in line + assert "ports=29971" in line + + +def test_reclaim_records_a_refusal_with_the_evidence(monkeypatch, tmp_path): + monkeypatch.setenv("TMPDIR", str(tmp_path)) + monkeypatch.setattr(_stale, "reclaim_ports", + lambda pid, ports: pytest.fail("must not signal")) + monkeypatch.setattr(_stale, "same_runtime_dir_as_us", lambda pid: True) + + listener._reclaim_stale_daemon_ports( + Config(), probe=_PortProbe(ports=[29971], holder=999, + escalated=[_answered()])) + + line = _reclaim_lines(tmp_path)[-1] + assert "decision=refused" in line + assert "reason=holder-answered" in line + assert "probes=ours@41ms" in line + + +def test_reclaim_records_the_different_runtime_dir_refusal( + monkeypatch, tmp_path): + """The strict identity gate is untouched, and its refusal is now on disk.""" + monkeypatch.setenv("TMPDIR", str(tmp_path)) + monkeypatch.setattr(_stale, "same_runtime_dir_as_us", lambda pid: False) + monkeypatch.setattr(_stale, "pid_runtime_dir", lambda pid: "/tmp") + monkeypatch.setattr(_stale, "reclaim_ports", + lambda pid, ports: pytest.fail("must not signal")) + + listener._reclaim_stale_daemon_ports( + Config(), probe=_PortProbe(ports=[29971], holder=999)) + + line = _reclaim_lines(tmp_path)[-1] + assert "decision=refused" in line + assert "reason=different-runtime-dir" in line + + +def test_reclaim_records_a_no_holder_decision(monkeypatch, tmp_path): + monkeypatch.setenv("TMPDIR", str(tmp_path)) + listener._reclaim_stale_daemon_ports( + Config(), probe=_PortProbe(ports=[29971], holder=None)) + + line = _reclaim_lines(tmp_path)[-1] + assert "decision=no-holder" in line + + +def _reclaim_lines(tmp_path): + text = (tmp_path / "browserwright-daemon.log").read_text() + lines = [ln for ln in text.splitlines() if " LIFECYCLE reclaim " in ln] + assert lines, text + for line in lines: + assert re.match( + r"^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}[+-]\d{4} LIFECYCLE reclaim ", + line), line + return lines + + +@pytest.mark.asyncio +async def test_serve_wires_logging_before_it_can_reclaim(monkeypatch): + """Ordering, through the real `run_serve`. + + The reclaim must not run before logging is wired: with only + `logging.lastResort` its warnings are undated, unlabelled and — on an + ensure-spawned serve, whose stderr is DEVNULL — lost. The LIFECYCLE line + survives either way, but the two are meant to agree in one file. + """ + order: list[str] = [] + + async def fake_ping(timeout=1.0, *, host=None, port=None): + return _ipc.NO_PONG + + class _RefusedEndpoint: + def __init__(self, **kwargs): + pass + + async def start(self): + raise OSError("address already in use") + + monkeypatch.setattr(listener._ipc, "ping_status_async", fake_ping) + monkeypatch.setattr(listener._ipc, "cleanup_endpoint", lambda: None) + monkeypatch.setattr(listener, "_wire_logging", + lambda: order.append("logging")) + monkeypatch.setattr(listener, "install_json_logging_if_requested", + lambda: order.append("json")) + monkeypatch.setattr(listener, "_reclaim_stale_daemon_ports", + lambda cfg: order.append("reclaim")) + monkeypatch.setattr(listener, "_cleanup_orphan_cdp_chrome", lambda: None) + monkeypatch.setattr(listener, "PlaywrightFacade", _RefusedEndpoint) + + await listener.run_serve(Config(backend="extension")) + + assert order[:3] == ["logging", "json", "reclaim"] + + # ---- 2.4: the control-socket watchdog is gone (ADR-0011) -------------------- # # It self-exited the daemon when its socket file was removed or replaced, so a diff --git a/tests/daemon/test_stop_signals_a_wedged_daemon.py b/tests/daemon/test_stop_signals_a_wedged_daemon.py new file mode 100644 index 0000000..7a3137d --- /dev/null +++ b/tests/daemon/test_stop_signals_a_wedged_daemon.py @@ -0,0 +1,226 @@ +"""The explicit verbs may stop a daemon that stopped answering. + +The asymmetry this closes: `stop` and `restart` refused to signal anything they +could not ping (`_ipc.ping_sync`, a live daemon at the resolved endpoint is +mandatory), so a genuinely wedged daemon could not be stopped by any CLI verb — +while `serve`'s *implicit* reclaim SIGTERMed on the weakest possible evidence and +killed a live one. The end state is the other way round: the implicit path is the +strict one (escalating evidence, `listener._reclaim_decision`), and the explicit +path may act on **already-confirmed identity**. + +Confirmed identity, unchanged and in full, is `_stale.unresponsive_daemon`: a +pid lsof names as holding the endpoint's own TCP port, whose command line is a +browserwright daemon, and sharing our runtime dir. "Never signal a process we +cannot prove is browserwright" stays absolute — these tests pin both halves. +""" +from __future__ import annotations + +import os +from types import SimpleNamespace + +import pytest + +from browserwright.daemon import _ipc, _stale, cli, launchagent, supervise +from browserwright.daemon.config import Config + + +# ---- the identity lookup --------------------------------------------------- + + +def test_unresponsive_daemon_needs_both_proofs(monkeypatch): + """A proven browserwright holder on our runtime dir. Miss either → None.""" + monkeypatch.setattr(_stale, "port_holder_pids", lambda port: [999]) + monkeypatch.setattr(_stale, "pid_is_browserwright_daemon", lambda pid: True) + monkeypatch.setattr(_stale, "same_runtime_dir_as_us", lambda pid: True) + + assert _stale.unresponsive_daemon(39990) == 999 + + # lsof/cmdline cannot name a browserwright daemon → never a kill target. + monkeypatch.setattr(_stale, "pid_is_browserwright_daemon", lambda pid: False) + assert _stale.unresponsive_daemon(39990) is None + + # A browserwright daemon from ANOTHER runtime dir is someone else's live + # daemon (the machine-global one) — the issue #44 B rule, unchanged. + monkeypatch.setattr(_stale, "pid_is_browserwright_daemon", lambda pid: True) + monkeypatch.setattr(_stale, "same_runtime_dir_as_us", lambda pid: False) + assert _stale.unresponsive_daemon(39990) is None + + +def test_unresponsive_daemon_skips_itself_and_looks_only_at_the_endpoint_port( + monkeypatch): + """The lookup asks about ONE port — the endpoint that should have answered — + so a stop aimed at one daemon can never nominate the holder of another's.""" + seen = [] + + def port_holder_pids(port): + seen.append(port) + return [os.getpid()] + + monkeypatch.setattr(_stale, "port_holder_pids", port_holder_pids) + monkeypatch.setattr(_stale, "pid_is_browserwright_daemon", lambda pid: True) + monkeypatch.setattr(_stale, "same_runtime_dir_as_us", lambda pid: True) + + assert _stale.unresponsive_daemon(39990) is None + assert seen == [39990] + + +# ---- `stop` ---------------------------------------------------------------- + + +@pytest.fixture +def stopped(monkeypatch): + """Stub the signalling layer; return the pids `terminate` was called with.""" + signalled = [] + monkeypatch.setattr(_ipc, "cleanup_endpoint", lambda: None) + monkeypatch.setattr(_ipc, "log_lifecycle", + lambda event, **fields: signalled.append((event, fields))) + monkeypatch.setattr("browserwright.daemon.platforms.proc_start_time", + lambda pid: 111) + + def fake_terminate(pid, **kwargs): + signalled.append(pid) + return supervise.Outcome.EXITED + + monkeypatch.setattr(supervise, "terminate", fake_terminate) + return signalled + + +def test_stop_signals_a_confirmed_wedged_daemon(monkeypatch, stopped, tmp_path): + """Nothing answers the ping, but our ports are provably held by a + browserwright daemon on our runtime dir → stop it.""" + monkeypatch.setenv("XDG_RUNTIME_DIR", str(tmp_path)) + monkeypatch.setattr(_ipc, "pid_path", lambda: tmp_path / "browserwright-daemon.pid") + (tmp_path / "browserwright-daemon.pid").write_text("4242\n") + monkeypatch.setattr(_ipc, "ping_sync", lambda timeout: None) + monkeypatch.setattr(_stale, "port_holder_pids", lambda port: [4242]) + monkeypatch.setattr(_stale, "pid_is_browserwright_daemon", lambda pid: True) + monkeypatch.setattr(_stale, "pid_env", lambda pid, name: str(tmp_path)) + + # An unmodified Config: its facade port is unset, so ADR-0012 rule 6's + # port/endpoint guard does not fire and this test is about the lookup. + assert cli._cmd_stop(SimpleNamespace(timeout=0.0), Config()) == 0 + + assert 4242 in stopped + events = [a for a in stopped if isinstance(a, tuple)] + assert events and events[0][0] == "stop" + assert events[0][1]["probes"] == "unresponsive" + + +def test_stop_still_refuses_a_stranger_holding_the_port(monkeypatch, stopped, + tmp_path, capsys): + """A port holder that is not a browserwright daemon is never signalled — the + explicit path does not get to skip the identity proof.""" + monkeypatch.setenv("XDG_RUNTIME_DIR", str(tmp_path)) + monkeypatch.setattr(_ipc, "pid_path", lambda: tmp_path / "browserwright-daemon.pid") + (tmp_path / "browserwright-daemon.pid").write_text("4242\n") + monkeypatch.setattr(_ipc, "ping_sync", lambda timeout: None) + monkeypatch.setattr(_stale, "port_holder_pids", lambda port: [4242]) + monkeypatch.setattr(_stale, "pid_is_browserwright_daemon", lambda pid: False) + + assert cli._cmd_stop(SimpleNamespace(timeout=0.0), Config()) == 0 + + assert stopped == [] # nothing signalled; only the "no live daemon" path + assert "no live daemon" in capsys.readouterr().err + + +def test_stop_still_refuses_a_daemon_from_another_runtime_dir( + monkeypatch, stopped, tmp_path, capsys): + """The machine-global daemon's case: provably browserwright, holds our + ports, but lives under /tmp. Never signal it (issue #44 B).""" + monkeypatch.setenv("XDG_RUNTIME_DIR", str(tmp_path)) + monkeypatch.setattr(_ipc, "pid_path", lambda: tmp_path / "browserwright-daemon.pid") + (tmp_path / "browserwright-daemon.pid").write_text("4242\n") + monkeypatch.setattr(_ipc, "ping_sync", lambda timeout: None) + monkeypatch.setattr(_stale, "port_holder_pids", lambda port: [4242]) + monkeypatch.setattr(_stale, "pid_is_browserwright_daemon", lambda pid: True) + monkeypatch.setattr(_stale, "pid_env", lambda pid, name: "/tmp") + + assert cli._cmd_stop(SimpleNamespace(timeout=0.0), Config()) == 0 + + assert stopped == [] + assert "no live daemon" in capsys.readouterr().err + + +def test_stop_wedged_death_is_measured_by_the_process_table( + monkeypatch, tmp_path): + """A daemon that was already not answering cannot prove death by ping.""" + seen = {} + + def fake_terminate(pid, **kwargs): + seen["is_dead"] = kwargs["is_dead"] + seen["guard"] = kwargs["guard"] + return supervise.Outcome.EXITED + + monkeypatch.setattr(supervise, "terminate", fake_terminate) + monkeypatch.setattr(_ipc, "cleanup_endpoint", lambda: None) + monkeypatch.setattr(_ipc, "log_lifecycle", lambda event, **fields: None) + monkeypatch.setattr("browserwright.daemon.platforms.proc_start_time", + lambda pid: 111) + monkeypatch.setattr(_ipc, "ping_sync", lambda timeout: None) + monkeypatch.setattr(_stale, "unresponsive_daemon", lambda port: 4242) + alive = {"v": True} + monkeypatch.setattr(_stale, "pid_alive", lambda pid: alive["v"]) + + assert cli._cmd_stop(SimpleNamespace(timeout=0.0), Config()) == 0 + + assert seen["is_dead"]() is False + alive["v"] = False + assert seen["is_dead"]() is True + # The pid-reuse guard stays armed: this pid is not one we own. + assert seen["guard"]() is True + + +# ---- `restart`'s incumbent stop ------------------------------------------- + + +def test_restart_stops_a_confirmed_wedged_incumbent(monkeypatch): + signalled = [] + monkeypatch.setattr(_ipc, "ping_sync", lambda timeout: None) + monkeypatch.setattr(_stale, "unresponsive_daemon", lambda port: 4242) + monkeypatch.setattr(_stale, "pid_alive", lambda pid: False) + monkeypatch.setattr("browserwright.daemon.platforms.proc_start_time", + lambda pid: 111) + monkeypatch.setattr(supervise, "terminate", + lambda pid, **kw: signalled.append(pid) + or supervise.Outcome.EXITED) + + assert launchagent._stop_incumbent(0.0) == {"stopped": 4242} + assert signalled == [4242] + + +def test_restart_still_signals_nothing_it_cannot_identify(monkeypatch): + monkeypatch.setattr(_ipc, "ping_sync", lambda timeout: None) + monkeypatch.setattr(_stale, "unresponsive_daemon", lambda port: None) + monkeypatch.setattr(supervise, "terminate", + lambda pid, **kw: pytest.fail("must not signal")) + + assert launchagent._stop_incumbent(0.0) == {"stopped": None} + + +def test_restart_still_refuses_a_non_local_endpoint(monkeypatch): + """ADR-0011: a remote endpoint's pid is a pid over there, whoever holds it.""" + monkeypatch.setenv("BW_DAEMON_URL", "http://100.72.20.32:19990") + monkeypatch.setattr(_stale, "unresponsive_daemon", + lambda port: pytest.fail("must not look for a pid")) + with pytest.raises(launchagent.LaunchAgentError) as ei: + launchagent._stop_incumbent(0.0) + assert ei.value.exit_code == 3 + + +def test_stop_never_touches_the_lookup_for_a_healthy_daemon(monkeypatch): + """The ordinary path is untouched: a pong pinpoints the pid, and the port + lookup is never even consulted.""" + signalled = [] + monkeypatch.setattr(_ipc, "ping_sync", lambda timeout: 4242) + monkeypatch.setattr(_ipc, "cleanup_endpoint", lambda: None) + monkeypatch.setattr(_ipc, "log_lifecycle", lambda event, **fields: None) + monkeypatch.setattr("browserwright.daemon.platforms.proc_start_time", + lambda pid: os.getpid()) + monkeypatch.setattr(_stale, "unresponsive_daemon", + lambda port: pytest.fail("ping answered; no lookup")) + monkeypatch.setattr(supervise, "terminate", + lambda pid, **kw: signalled.append(pid) + or supervise.Outcome.EXITED) + + assert cli._cmd_stop(SimpleNamespace(timeout=0.0), Config()) == 0 + assert signalled == [4242] diff --git a/tests/daemon/test_task_executor_data_plane.py b/tests/daemon/test_task_executor_data_plane.py index bd6b633..8dcd56c 100644 --- a/tests/daemon/test_task_executor_data_plane.py +++ b/tests/daemon/test_task_executor_data_plane.py @@ -93,7 +93,7 @@ class _Session: monkeypatch.setattr( client, "_ensure_executor_lease", - lambda _sess: client.ExecutorLease("sess-task", "executor-task"), + lambda _sess, **_kw: client.ExecutorLease("sess-task", "executor-task"), ) monkeypatch.setattr(client, "_connect", lambda *_args, **_kwargs: _Connection()) monkeypatch.setattr( diff --git a/tests/skill/test_adr0013_diagnose_before_blame.py b/tests/skill/test_adr0013_diagnose_before_blame.py index 64b2628..6e90ced 100644 --- a/tests/skill/test_adr0013_diagnose_before_blame.py +++ b/tests/skill/test_adr0013_diagnose_before_blame.py @@ -226,7 +226,11 @@ def _agent_visible_sources() -> list[Path]: root / "session.py", root / "mode_b_client.py", root / "daemon_lifecycle.py", root / "_executor" / "client.py", - root / "_executor" / "process.py"] + root / "_executor" / "process.py", + # Daemon-raised text crosses the JSON-RPC envelope and reaches the + # agent (a shutdown, a recovery refusal), so its words are bound by + # the same list as the client's. + root / "daemon" / "errors.py"] def test_agent_visible_remediation_text_has_no_banned_words(): diff --git a/tests/skill/test_daemon_replaced_mid_call.py b/tests/skill/test_daemon_replaced_mid_call.py new file mode 100644 index 0000000..a0e0c9d --- /dev/null +++ b/tests/skill/test_daemon_replaced_mid_call.py @@ -0,0 +1,392 @@ +"""A daemon that is replaced mid-call must be survived, or reported honestly. + +2026-10-19, observed verbatim: three agent calls died with + + ensureExecutor failed for session '2378': CDP + BrowserwrightDaemon.ensureExecutor failed: ensureExecutor failed: + RuntimeError('upstream readiness: no browserwright extension is connected + to the daemon (session 2378). Open the browser where the extension is + installed and ensure it is enabled; ... chrome://extensions ...') + +while the extension was connected and healthy throughout — the *daemon* was +being replaced under the call. H2's half of the fix is pinned here: + + - the two seams that run strictly before a request can reach the executor + (the `ensureExecutor` verb, the `/exec` connect) retry a daemon that went + away, and re-resolve the endpoint between attempts, so a replacement is + invisible to the agent and its code still runs exactly once; + - a failure once a request frame may be on the wire is NOT retried (a replay + would run the agent's code twice) — it reaps the exact executor instead; + - the retry decision is "the daemon went away", never "any error": the + no-extension diagnosis is answered immediately, with its wording untouched; + - when the retries run out, the text names the daemon and ends with the one + instruction that applies (ADR-0013 rule 3, the banned-word list). +""" +from __future__ import annotations + +import json +import time + +import pytest + +import browserwright.session as session_mod +from browserwright._executor import client as exec_client +from browserwright._executor import protocol +from browserwright.daemon.errors import DaemonShuttingDown +from browserwright.daemon_url import DaemonEndpoint +from browserwright.errors import CDPError, CDPTransportClosed, DaemonUnavailable + +SID = "s-1" + +#: The shape the daemon produced on 2026-10-19 for a *connected* extension, +#: because its own shutdown had pulled the relay away. It must never be the +#: answer to a daemon departure, and it must not be retried. +NO_EXTENSION_ENVELOPE = ( + "ensureExecutor failed: RuntimeError(\"upstream readiness: no browserwright " + f"extension is connected to the daemon (session '{SID}'). Open the browser " + "where the extension is installed and ensure it is enabled; if you just " + "installed or upgraded browserwright, reload the extension at " + "chrome://extensions so its service worker reconnects to the daemon relay. " + "Then retry. (Use --backend=cdp --create for an isolated Chrome that needs " + "no extension.)\")" +) + + +class _FakeDaemon: + """`ModeBClient`'s two-method contract, scripted. + + ``endpoints`` is consumed one entry per resolve (the last one repeats): a + string is returned, an exception is raised. A replacement publishes a new + endpoint, so a retry that re-resolves can be told apart from one that dials + the dead address again. + """ + + def __init__(self, *endpoints): + self._endpoints = list(endpoints) or ["ws://127.0.0.1:19990/control"] + self.resolves = 0 + self.invalidated = 0 + + def resolve_ws_url(self) -> str: + item = (self._endpoints.pop(0) if len(self._endpoints) > 1 + else self._endpoints[0]) + self.resolves += 1 + if isinstance(item, BaseException): + raise item + return item + + def invalidate(self) -> None: + self.invalidated += 1 + + +class _ExecLink: + """The `/exec` ws: records the frames the client shipped, answers one.""" + + def __init__(self, fail_on_recv=None): + self.frames = [] + self._reply = protocol.ExecuteResponse(console="ran once\n") + self._fail_on_recv = fail_on_recv + + def send(self, payload): + self.frames.append(json.loads(payload)) + + def recv(self, timeout=None): + if self._fail_on_recv is not None: + raise self._fail_on_recv + return json.dumps(self._reply.to_dict()) + + def close(self): + return None + + +def _session(monkeypatch, *endpoints): + """A real `Session` over a scripted `ModeBClient` (its `CDPSession` is + replaced by `_install_control`), with the ledger write neutered.""" + daemon = _FakeDaemon(*endpoints) + monkeypatch.setattr(exec_client.reg, "touch", lambda _sid: None) + return session_mod.Session(daemon, record={"id": SID}), daemon + + +def _install_control(monkeypatch, outcomes): + """Point `Session.cdp` at a scripted control socket. + + Returns the sockets created, in order: the count is how many times the + client re-dialed the daemon, and each ``ws_url`` shows which endpoint that + attempt used (so "re-resolved" is observable, not assumed). + """ + sockets = [] + + class _ControlSocket: + def __init__(self, ws_url): + self.ws_url = ws_url + self._closed = False + self.methods = [] + sockets.append(self) + + def send(self, method, **params): + self.methods.append(method) + if method == "BrowserwrightDaemon.killExecutor": + return {"reaped": True} + assert method == "BrowserwrightDaemon.ensureExecutor", method + outcome = outcomes.pop(0) + if isinstance(outcome, CDPTransportClosed): + # The daemon serving this socket exited mid-call. + self._closed = True + if isinstance(outcome, BaseException): + raise outcome + return outcome + + def close(self): + self._closed = True + + monkeypatch.setattr(session_mod, "CDPSession", _ControlSocket) + return sockets + + +def _install_exec_link(monkeypatch, link, *, fail_first=False): + """Stand in for the data plane's ``ws_connect``, recording each attempt.""" + connects = [] + + def _connect(url, **_kwargs): + connects.append(url) + if fail_first and len(connects) == 1: + raise OSError(61, "Connection refused") + return link + + monkeypatch.setattr(exec_client, "ws_connect", _connect) + return connects + + +# ---- the replacement is survived ------------------------------------------- + + +def test_a_replaced_daemon_is_retried_against_the_re_resolved_endpoint(monkeypatch): + """Attempt 1's socket dies because the daemon was replaced; attempt 2 must + reach the daemon that answers *now*, and the caller's code must still cross + the wire exactly once.""" + sess, daemon = _session( + monkeypatch, + "ws://127.0.0.1:19990/control?session=s-1", # the daemon that leaves + "ws://127.0.0.1:29990/control?session=s-1", # its replacement + ) + sockets = _install_control(monkeypatch, [ + CDPTransportClosed( + method="BrowserwrightDaemon.ensureExecutor", + cdp_message="sent 1000 (OK); then received 1001 (going away)"), + {"ready": True, "executor_id": "exec-2"}, + ]) + link = _ExecLink() + connects = _install_exec_link(monkeypatch, link) + + response = exec_client.run_on_executor( + sess, "print('once')", timeout_ms=5000) + + # Re-resolved, not reused: the cached url was dropped and the dead socket + # replaced, so attempt 2 dialed the endpoint the replacement published. + assert daemon.invalidated == 1 + assert [s.ws_url for s in sockets] == [ + "ws://127.0.0.1:19990/control?session=s-1", + "ws://127.0.0.1:29990/control?session=s-1", + ] + # The caller's code ran once, carrying the identity attempt 2 confirmed. + assert len(connects) == 1 + assert len(link.frames) == 1 + assert link.frames[0]["code"] == "print('once')" + assert link.frames[0]["executor_id"] == "exec-2" + assert response.console == "ran once\n" + + +def test_a_shutting_down_daemon_answer_is_retried(monkeypatch): + """The daemon's own words for its departure must be read as + "it left" — retried against the replacement, not reported as the browser's + problem. The phrase comes from `DaemonShuttingDown` itself so the marker + and this classifier cannot drift apart.""" + sess, daemon = _session( + monkeypatch, + "ws://127.0.0.1:19990/control?session=s-1", + "ws://127.0.0.1:29990/control?session=s-1", + ) + sockets = _install_control(monkeypatch, [ + CDPError(method="BrowserwrightDaemon.ensureExecutor", + cdp_message=f"ensureExecutor failed: {DaemonShuttingDown(SID)!r}"), + {"ready": True, "executor_id": "exec-3"}, + ]) + + assert exec_client.ensure_executor(sess) == "exec-3" + # It retried (the second scripted answer is what came back) and re-armed + # endpoint resolution for the next dial. An *open* control socket is + # reused on purpose; only a dead one is re-dialed (`Session.cdp`). + assert sockets[0].methods == ["BrowserwrightDaemon.ensureExecutor"] * 2 + assert daemon.invalidated == 1 + + +def test_a_replacement_still_binding_the_endpoint_is_retried(monkeypatch): + """After a departure, a *refused* endpoint is the same event continuing (the + replacement has not bound yet) — not a new, hopeless cause. One call spans + leave → refused → bound, and still ships the code once.""" + sess, daemon = _session( + monkeypatch, + "ws://127.0.0.1:19990/control?session=s-1", # the daemon that leaves + DaemonUnavailable("no browserwright daemon answered at " + "ws://127.0.0.1:19990: Connection refused"), + DaemonUnavailable("no browserwright daemon answered at " + "ws://127.0.0.1:19990: Connection refused"), + "ws://127.0.0.1:29990/control?session=s-1", # the replacement, bound + ) + sockets = _install_control(monkeypatch, [ + CDPTransportClosed( + method="BrowserwrightDaemon.ensureExecutor", + cdp_message="received 1001 (going away)"), + {"ready": True, "executor_id": "exec-5"}, + ]) + link = _ExecLink() + _install_exec_link(monkeypatch, link) + + response = exec_client.run_on_executor( + sess, "print('once')", timeout_ms=5000) + + assert [s.ws_url for s in sockets] == [ + "ws://127.0.0.1:19990/control?session=s-1", + "ws://127.0.0.1:29990/control?session=s-1", + ] + assert daemon.resolves >= 3 # the refused attempt re-asked, and retried + assert len(link.frames) == 1 + assert link.frames[0]["executor_id"] == "exec-5" + assert response.console == "ran once\n" + + +def test_the_exec_connect_retries_a_replacement_on_its_new_endpoint(monkeypatch): + """The data plane's half: a `/exec` ws that will not open is the + replacement still binding. Each attempt re-resolves the endpoint (the state + file the replacement publishes), so a daemon that came back on another port + is dialed at the new address instead of the dead one.""" + sess, daemon = _session(monkeypatch, "ws://127.0.0.1:19990/control?session=s-1") + _install_control(monkeypatch, [{"ready": True, "executor_id": "exec-4"}]) + endpoints = ["http://127.0.0.1:19990", "http://127.0.0.1:29990"] + + def _endpoint(**_kw): + # The replacement publishes a *new* address; a retry that re-resolved + # must dial it instead of the dead one. + return DaemonEndpoint( + url=(endpoints.pop(0) if len(endpoints) > 1 else endpoints[0]), + explicit=False, source="state") + + monkeypatch.setattr(exec_client, "daemon_endpoint", _endpoint) + link = _ExecLink() + connects = _install_exec_link(monkeypatch, link, fail_first=True) + + response = exec_client.run_on_executor( + sess, "print('once')", timeout_ms=5000) + + assert connects == [ + "ws://127.0.0.1:19990/exec?session=s-1", + "ws://127.0.0.1:29990/exec?session=s-1", + ] + assert len(link.frames) == 1 + assert link.frames[0]["code"] == "print('once')" + assert response.console == "ran once\n" + + +# ---- and never at the cost of running the code twice ----------------------- + + +def test_a_failure_after_the_request_was_sent_is_not_retried(monkeypatch): + """The retry rule's other half. Once a request frame may be on the wire, a + replay could execute the agent's code a second time — worse than an error — + so this path reaps that exact executor and tells the agent to retry.""" + sess, _daemon = _session( + monkeypatch, "ws://127.0.0.1:19990/control?session=s-1") + sockets = _install_control(monkeypatch, [ + {"ready": True, "executor_id": "exec-7"}, + ]) + link = _ExecLink(fail_on_recv=ConnectionError("relay went away")) + connects = _install_exec_link(monkeypatch, link) + + with pytest.raises(exec_client.ExecutorUnavailable) as ei: + exec_client.run_on_executor(sess, "print('twice?')", timeout_ms=5000) + + # One frame, one connect, one ensureExecutor: nothing was replayed. + assert len(link.frames) == 1 + assert len(connects) == 1 + assert sockets[0].methods == [ + "BrowserwrightDaemon.ensureExecutor", + "BrowserwrightDaemon.killExecutor", + ] + msg = str(ei.value) + assert "the daemon" in msg # names the layer that went away + assert "retry the command" in msg # ... and one thing to do about it + assert "chrome://extensions" not in msg + + +# ---- the distinction decides, not "any error" ------------------------------ + + +def test_the_no_extension_diagnosis_is_never_retried(monkeypatch): + """The daemon answered; it did not leave. The next attempt would fail the + same way, so retrying would only burn the caller's deadline and then report + the wrong layer — and the extension wording stays exactly as it was.""" + sess, daemon = _session(monkeypatch, "ws://127.0.0.1:19990/control?session=s-1") + sockets = _install_control(monkeypatch, [ + CDPError(method="BrowserwrightDaemon.ensureExecutor", + cdp_message=NO_EXTENSION_ENVELOPE), + ]) + + with pytest.raises(exec_client.ExecutorUnavailable) as ei: + exec_client.run_on_executor(sess, "print(1)") + + assert len(sockets) == 1 # one attempt: not a daemon departure + assert daemon.invalidated == 0 + assert "no browserwright extension is connected" in str(ei.value) + assert "chrome://extensions" in str(ei.value) + + +# ---- what the agent sees when the retries run out -------------------------- + + +def test_exhausted_retries_name_the_daemon_and_one_instruction(monkeypatch): + sess, _daemon = _session( + monkeypatch, "ws://127.0.0.1:19990/control?session=s-1") + sockets = _install_control(monkeypatch, [ + CDPTransportClosed( + method="BrowserwrightDaemon.ensureExecutor", + cdp_message="received 1001 (going away)") + for _ in range(20) + ]) + started = time.monotonic() + with pytest.raises(exec_client.ExecutorUnavailable) as ei: + exec_client.run_on_executor(sess, "print(1)", timeout_ms=250) + elapsed = time.monotonic() - started + + err = ei.value + msg = str(err) + assert len(sockets) >= 2 # it really did retry + assert elapsed < 2.0 # ... inside the caller's 250ms budget + assert "the daemon" in msg + assert "was replaced or is shutting down" in msg + assert "no browserwright extension is connected" not in msg + assert "chrome://extensions" not in msg + # ADR-0013 rule 3: the message carries what the endpoint answers *now* + # (probed once), not only our inference about who left. + assert "Right now nothing is listening at 127.0.0.1:" in msg + # One instruction, and it is the one that applies to a replaced daemon. + assert err.fix == "retry the command" + assert msg.endswith("[fix: retry the command]") + + +def test_an_endpoint_that_was_never_there_is_reported_without_retrying( + monkeypatch): + """The retry is for a daemon that *left*, not for one that never was: a + first-attempt refusal is a standing condition, already diagnosed by + `DaemonUnavailable` (probed endpoint + the auto-start rule), and `session + new` / `recover` own starting a daemon. Waiting 9s to say the same thing + would just make every "no daemon" call feel hung.""" + gone = "no browserwright daemon answered at ws://127.0.0.1:19990" + sess, daemon = _session(monkeypatch, DaemonUnavailable(f"{gone}: refused")) + started = time.monotonic() + + with pytest.raises(DaemonUnavailable) as ei: + exec_client.run_on_executor(sess, "print(1)", timeout_ms=9000) + + assert gone in str(ei.value) + assert time.monotonic() - started < 1.0 + # `Session._resolve_ws_url`'s own single in-place retry, and nothing more. + assert daemon.resolves == 2