From c93baa4e48b1bf0093cdffa2e865b41fc66a8cc6 Mon Sep 17 00:00:00 2001 From: AminDhouib Date: Mon, 14 Sep 2026 23:50:59 -0400 Subject: [PATCH 1/3] fix(F-868): status reports the backend this shell would use, not the first record MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `~/.stealth-mcp/server.json` holds one entry per display context and nothing prunes a dead one. On a machine recording a dead `win-session-2` (7169), a dead `headless` (19222) and a healthy `win-session-1` (52554) serving 56 proxies, `stealth-chrome-devtools status` run from that same session-1 shell printed `backend : not running` and `pid : 89892` — the pid of a process gone for hours. `singleton._probe_backend_status` selected its record with `backend_registry.first_backend`, which under schema v2 is dict insertion order and says in its own docstring that it carries no preference. Discovery had been walking `adoption_candidates` — the one home for "which backend would THIS client use" — all along, which is why every proxy in that shell was correctly on 52554. The probe now walks that same list and reports the first candidate that answers (wedged over down when none does). No second selection policy: it still only says which of the offered candidates is up. `status`, `doctor`'s summary lines, `stop` and `kill-orphans`'s live-backend guard all already consumed this ONE function. The CLI status block now selects once and passes `(status, port)` down, so its lines cannot describe different backends: `_format_backend_status` is pure formatting, `_recorded_backend_pid(port)` and `_doctor_port_occupant_line(port)` read the entry on that port via `backend_on_port` instead of each making its own `first_backend` read. Two independent selections deleted, none added. `status` gained one `other records:` line naming the display contexts the summary is not about, and only when the record holds more than one. RED first: 7 tests, the CLI ones reproducing the observed output byte-for-byte. Stale records are still pruned by nobody — the argument for leaving that alone is in the finding. Finding: audit/stage2/finding_F868_cli_status_reports_a_dead_record.md --- CHANGELOG.md | 23 ++ CLAUDE.md | 4 +- RUNBOOK.md | 16 +- ...g_F868_cli_status_reports_a_dead_record.md | 229 ++++++++++++++++++ src/stealth_chrome_devtools_mcp/cli.py | 99 +++++--- .../embedded/singleton.py | 51 ++-- tests/test_cli_status_wedged.py | 142 ++++++++++- tests/test_probe_backend_status.py | 87 +++++++ 8 files changed, 596 insertions(+), 55 deletions(-) create mode 100644 audit/stage2/finding_F868_cli_status_reports_a_dead_record.md diff --git a/CHANGELOG.md b/CHANGELOG.md index dd81c56..9d4cacc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,28 @@ # Changelog +## Unreleased + +### Fixed — `status` reported a dead sibling's record as "the backend" (F-868) + +On a machine whose `server.json` recorded three display contexts — two dead +(`win-session-2` on 7169, `headless` on 19222) and one healthy backend serving 56 +proxies (`win-session-1` on 52554) — `stealth-chrome-devtools status`, run from that +same session-1 shell, printed `backend : not running` and `pid : 89892`, the pid of a +process that had been gone for hours. `singleton._probe_backend_status` selected the +record with `backend_registry.first_backend`, which under schema v2 is dict insertion +order and carries no preference of its own, while discovery had been walking +`adoption_candidates` — the one home for "which backend would THIS client use" — all +along. The probe now walks that same list and reports the first candidate that answers +(wedged over down when none does), which fixes `status`, `doctor`'s summary lines, +`stop` and `kill-orphans`'s live-backend guard at once, since all four already consumed +it. The CLI status block now also selects once and passes the answer down: the pid and +log lines read the entry on the port just reported (`backend_on_port`) instead of making +their own `first_backend` read, and `doctor`'s port-occupant line takes the same port — +two independent record selections deleted rather than a third added. `status` gained one +line naming the display contexts it is NOT speaking about when the record holds more than +one, so a summary over a multi-context record no longer reads as "this is all there is". +Stale records are still pruned by nobody; see `audit/stage2/finding_F868_cli_status_reports_a_dead_record.md` §6. + ## 2.1.5 ### Fixed — the backend escapes the MCP client's Job Object (F-867) diff --git a/CLAUDE.md b/CLAUDE.md index 0fbf3b6..6a62aad 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -47,7 +47,7 @@ Package root: `src/stealth_chrome_devtools_mcp/`. Two console scripts (`pyprojec |---|---| | `server.py` | thin entrypoint — loads `embedded/server.py` as `__main__` via `runpy` (`main()` shim); its stdio branch is also **THE one place the PROXY process bootstraps its own observability** (`configure_logging("proxy")` + `_start_proxy_error_reporting`, F-827) — in the branch, never at the top of `main()`, or the runpy path would double-init | | `__main__.py` | `python -m stealth_chrome_devtools_mcp` → `server.main()` | -| `cli.py` | the `stealth-chrome-devtools` ops CLI verbs (`status`/`doctor`/`stop`/`restart`/`cleanup`/`kill-orphans`/`serve`) | +| `cli.py` | the `stealth-chrome-devtools` ops CLI verbs (`status`/`doctor`/`stop`/`restart`/`cleanup`/`kill-orphans`/`serve`). The status block **selects the backend ONCE** (F-868): `_probe_backend_status()` is called by the command, and its `(status, port)` is passed down to every line — `_format_backend_status` (now pure formatting, no I/O), `_recorded_backend_pid(port)` and `_doctor_port_occupant_line(port)` (both read the entry on THAT port via `backend_on_port`, never `first_backend`), and `_other_records_note(port)`, the one line that names the display contexts the summary is *not* about. Two independent record reads were deleted to get here; do not re-add one — a per-line read is how a live backend's status came to sit above a dead sibling's pid | | `settings.py` | **the one env home** — pydantic `Settings` + `get_settings()`; every `STEALTH_MCP_*` knob is a typed field here | | `observability.py` | Sentry error shipping — hardcoded DSN, on by default, never raises (no-op under `STEALTH_MCP_NO_ERROR_REPORTING`); **the one PII scrubber** — `_scrub_event` (Sentry's `before_send`); **the one non-exception report** — `capture_lifecycle` (F-827: proxy transitions that are decisions, not crashes) | @@ -57,7 +57,7 @@ Package root: `src/stealth_chrome_devtools_mcp/`. Two console scripts (`pyprojec | File | Owns | |---|---| | `server.py` | the real MCP server — **ZERO tool bodies**; all 94 live in `tool_sections/` (plan_SERVERSPLIT, complete at slice 12). What this file owns, and nothing else: the per-execution FastMCP app (`mcp`/`registry`), `app_lifespan`, the four `@mcp.resource` handlers, the xpool-safe gate, `build_arg_parser`, the `__main__` block, and the **binding loop** that drives registration from THIS file's module body, once per execution of it (so the canonical import, the bare-name spec load and the runpy `__main__` load each get a full 94-tool app — a section module that decorated itself would register into the first execution only). **No migration alias block**: every singleton, knob and guard this file drives is read as `rt.` against `tool_runtime`, the one patchable home, exactly as a tool body does. The lone import left is `clone_storage`, which this file never touches — it is the positive delegation handle `tests/test_clone_storage.py`'s F-201 negative-surface pin needs. At 523 LOC it is governed by the 1000-LOC default; its `GRANDFATHER` row is deleted, not merely satisfied | -| `singleton.py` | **backend lifecycle + the stdio proxy** — liveness probes (`_backend_http_ready`, `_probe_backend_status`), port selection (`_select_backend_port`, `DEFAULT_PORT`), the one identity+readiness reuse gate (`_same_identity_backend_ready`, `_source_fingerprint`, `REUSE_PATIENCE_SECONDS` — which spends its patience through `scheduling_lag.FairWindow`, F-856), cold-start lock (`_start_backend_holding_lock`), `run_stdio_proxy`. The `server.json` record moved to `backend_registry.py` (path names re-exported here for legacy callers); the proxy's *recovery from a dead backend* moved to `proxy_selfheal.py`; the watchdog LOOP moved to `backend_watchdog.py` — `_watch_backend_liveness` stays here as the wiring that knows which probes are ours; and **the spawn itself moved to `backend_launch.py` (F-867)** — `_start_server_process` still owns WHAT to start (`_server_process_cmd`, `_backend_interpreter`'s F-866 base-interpreter choice, the child env, F-830's boot-log roll) and what to RECORD afterwards (`PORT_FILE`, `_write_server_state`), and makes exactly ONE call, `backend_launch.spawn(cmd, child_env, boot_log) -> Launched(pid, rung)`; it no longer imports `subprocess` | +| `singleton.py` | **backend lifecycle + the stdio proxy** — liveness probes (`_backend_http_ready`; `_probe_backend_status`, which is **THE one answer to "what state is the backend THIS process would be served by in"** — since F-868 it walks `backend_registry.adoption_candidates` in the same order `_find_running_server` does and reports the first candidate that answers, wedged over down when none does, never `first_backend`'s "whichever entry the record lists first"; `status`, `doctor`, `stop` and `kill-orphans`'s live-backend guard all consume this ONE function, so the record selection is fixed in exactly one place), port selection (`_select_backend_port`, `DEFAULT_PORT`), the one identity+readiness reuse gate (`_same_identity_backend_ready`, `_source_fingerprint`, `REUSE_PATIENCE_SECONDS` — which spends its patience through `scheduling_lag.FairWindow`, F-856), cold-start lock (`_start_backend_holding_lock`), `run_stdio_proxy`. The `server.json` record moved to `backend_registry.py` (path names re-exported here for legacy callers); the proxy's *recovery from a dead backend* moved to `proxy_selfheal.py`; the watchdog LOOP moved to `backend_watchdog.py` — `_watch_backend_liveness` stays here as the wiring that knows which probes are ours; and **the spawn itself moved to `backend_launch.py` (F-867)** — `_start_server_process` still owns WHAT to start (`_server_process_cmd`, `_backend_interpreter`'s F-866 base-interpreter choice, the child env, F-830's boot-log roll) and what to RECORD afterwards (`PORT_FILE`, `_write_server_state`), and makes exactly ONE call, `backend_launch.spawn(cmd, child_env, boot_log) -> Launched(pid, rung)`; it no longer imports `subprocess` | | `backend_launch.py` | **THE one home for "spawn the backend where no MCP client's Job Object can reach it"** (F-867) — `spawn`, `Launched(pid, rung)` and nothing else in the tree may create a backend. POSIX is one rung (`posix`, `start_new_session=True`, unchanged). Windows climbs three, and the rung that served is logged through the ONE `_log_rung` line on `stealth.proxy` so a post-mortem can read it: (1) `breakaway` — `CREATE_BREAKAWAY_FROM_JOB`, accepted ONLY when `_proven_in_a_job` says the new process is in NO job, because under a nested job chain the flag leaves the innermost job only and a "successful" breakaway can still sit in the client's; `ERROR_ACCESS_DENIED` (what the SDK's job returns) drops a rung while any other `OSError` propagates. A PARTIAL escape is never discarded blindly — rung 2's viability (`_scheduler_plan`) is decided FIRST, and when there is no scheduler rung the child is KEPT as the `breakaway-partial` rung at WARNING, because out of one job beats out of none; conversely, breakaway once proven PERMITTED is re-asked on rung 3. (2) `scheduler` — a one-shot Task Scheduler task through `desktop_launch._schtasks` (the F-810 seam, not a second home) running `pythonw.exe` (no console, so no window can flash) on the BASE interpreter, never a venv's `Scripts\pythonw.exe`, which is F-866's redirector and would rebuild the kill-on-close job; the stdlib-only `_LAUNCHER_SCRIPT` reads argv + the ENTIRE child env + the boot-log path from a JSON spec in `~/.stealth-mcp/backend-launch/` (the command line carries two paths only because **`schtasks` stores at most 253 chars of `/TR` and truncates beyond that with exit 0** — measured, not F-810's documented "~261"; the silent truncation costs a Last-Result-2 task, the whole 20 s pid deadline and a drop to `plain`, which is why the per-attempt token is 12 hex chars, not 32), re-opens `backend-boot.log` itself and gives the backend stdout AND stderr (F-303 survives the hand-off), and returns the SERVING pid through an `os.replace`d pid file. **Gated on `_same_session_as_console`**: taken only when the spawner is already in the logged-on console session, so the backend lands where it would have anyway and the display context `singleton` records stays true (F-808 — the tool still never PICKS a session). Task and scratch files deleted in a `finally`, and the orphan sweep that runs before a scheduler spawn is a FILESYSTEM sweep, not a `/Query`: any `.json` spec (token = 12 hex) older than 2× the pid deadline gets its `stealth-mcp-backend-` task deleted BY NAME and its files removed. Deliberately not "delete tasks whose spec is absent" — `_cleanup` deletes the task first, so a killed spawner leaves task AND spec, which is the only orphan class there is and the one that predicate missed. A live sibling's spec is younger and untouched; a clean dir costs zero `schtasks` calls. (3) `plain` — today's `DETACHED_PROCESS \| CREATE_NEW_PROCESS_GROUP` (plus the breakaway bit when rung 1 proved it permitted), for a runner with no console session; its line names F-867 and says the backend is inside this client's job. A leaf: `backend_registry` for the state dir, `desktop_launch` lazily for the ONE `schtasks` seam, the ONE pid-file reader and the ONE task teardown, so none of them gets a second home. Never raises for a rung's own failure — only a genuine `Popen` error reaches the caller | | `backend_watchdog.py` | **THE one home for the proxy's mid-session liveness watchdog** — the SLOW witness (`watch_liveness`): F-820's strikes plus the confirmation phase. A leaf: both probes arrive as arguments, so it never imports `singleton` and the dead-vs-busy policy stays single-homed in the reuse gate | | `scheduling_lag.py` | **THE one home for "was this process scheduled fairly, and what does a time budget owe it when it was not"** (F-856) — `FairWindow`, whose budget is charged in fair seconds (elapsed ÷ the lag its own naps measured), plus `MAX_STRETCH`, `REPORT_FACTOR` and the `proxy: patience extended under starvation` lifecycle report. It never decides alive-or-dead: only "has this window been spent", so `proxy_selfheal`'s ONE heal path is untouched. A leaf; the `_now`/`_wait` module functions are its single timing seam | diff --git a/RUNBOOK.md b/RUNBOOK.md index f9c2a57..35427eb 100644 --- a/RUNBOOK.md +++ b/RUNBOOK.md @@ -51,8 +51,20 @@ browser-session cap : 20.0 GB [STEALTH_MCP_BROWSER_SESSION_STORAGE_CAP_GB] - **`backend`** is the real liveness state (`singleton._probe_backend_status`): `responsive` = answers a real MCP `initialize`; `wedged` = socket open but not answering (→ `restart`); `down` = recorded but nothing there; "not running" = no - recorded backend. The port shown is the **chosen** port, which may differ from - `19222` if that was taken (see "Port already in use" below). + recorded backend this shell could adopt. The port shown is the **chosen** port, which + may differ from `19222` if that was taken (see "Port already in use" below). +- **Which backend is it about?** The one *this shell* would be served by — the same + adoption order discovery uses (F-868), not whichever entry `server.json` lists first. + `pid` and `log` name that same backend, so the four lines can never describe different + processes. `server.json` can hold one entry per display context, and dead ones are + never pruned, so an `other records:` line appears when there are others: + + ``` + other records: 2 (win-session-2, headless) — run `doctor` for each one's state + ``` + + `doctor`'s `contexts :` block probes every recorded backend on its own port; that is + the place to look when you want all of them rather than yours. - **`browser-session root`** and **`browser-session cap`** are about **disk** — the directory holding named browser-session profiles/clones and the cap that trims idle ones. They are named "browser-session" deliberately: this cap trims *named diff --git a/audit/stage2/finding_F868_cli_status_reports_a_dead_record.md b/audit/stage2/finding_F868_cli_status_reports_a_dead_record.md new file mode 100644 index 0000000..b4798fa --- /dev/null +++ b/audit/stage2/finding_F868_cli_status_reports_a_dead_record.md @@ -0,0 +1,229 @@ +# F-868 — `status` reports the FIRST recorded backend, so an operator with a healthy backend is told "not running" and handed a dead pid + +**Status:** FIXED in this PR (product defect; reproduced hermetically from the exact +observed record, root cause confirmed in source) +**Opened by:** the 2026-09-14 ~23:20 local ops-CLI observation on the maintainer's +workstation (PyPI 2.1.5, Windows 11, `~/.stealth-mcp/server.json` schema 2, three entries) +**Source at:** `main` = `267bac8` +**Severity:** MEDIUM. No data loss and no backend is harmed, but it is a truthfulness +defect in the ONE surface an operator consults to decide whether the product is broken — +and the answer it gives ("not running") is the one that invites a `restart`, a +`kill-orphans`, or a bug report, in the exact state where nothing is wrong. + +--- + +## 1. What was observed + +`~/.stealth-mcp/server.json`, schema 2, three entries, in this recorded order: + +| context | port | version | pid | actual state at 23:20 | +|---|---|---|---|---| +| `win-session-2` | 7169 | 2.1.1 | 89892 | pid DEAD, nothing listening | +| `headless` | 19222 | 2.1.3 | 67720 | pid DEAD, nothing listening | +| `win-session-1` | 52554 | 2.1.5 | 53836 | pid ALIVE, listening on 127.0.0.1:52554, healthy, serving 56 proxies | + +The shell running the CLI was in Windows session 1 — the active console session — so +`display_context.display_context()` answers `win-session-1` for it, and +`singleton._find_running_server` adopts and proxies to 52554 from that same shell. + +`stealth-chrome-devtools status` printed: + +``` +backend : not running +pid : 89892 +log : C:\Users\amind\.stealth-mcp\logs\backend-89892.log +version : 2.1.5 +``` + +Three separate untruths in four lines: the backend this shell would be served by was +responsive; the pid named a process that had not existed for hours; and the log path +pointed at a dead backend's file instead of the live one's. + +**Reproduced hermetically** (`tests/test_cli_status_wedged.py::TestCliStatusReportsTheBackendThisClientWouldUse`, +the record above verbatim, both liveness primitives stubbed so no socket is opened): +RED output before the fix was byte-identical to the observation — +`'backend : not running\npid : 89892\n…'`. + +## 2. Root cause + +**One function picked the record, and it picked by position rather than by relevance:** +`singleton._probe_backend_status` (`src/stealth_chrome_devtools_mcp/embedded/singleton.py:160` +at `267bac8`) read + +```python +entry = backend_registry.first_backend(_read_server_state()) # singleton.py:171 +``` + +`first_backend` (`embedded/backend_registry.py:135`) is honest about what it is and says +so in its own docstring: *"'First' preserves the pre-v2 single-backend behaviour exactly… +It carries no preference of its own — a caller that wants the backend most likely to be +usable asks `window_capable_first`."* Under schema v2 "first" is dict insertion order, +i.e. **whichever context happened to record itself earliest and has not been superseded +by port since** — here `win-session-2`, a backend from 2.1.1 that had been dead for two +releases. The probe therefore reported that entry's port (7169) as `down`, and +`cli._format_backend_status` maps `down` and `none` alike to `"not running"`. + +**Why the own-context record was not preferred.** The ordering that answers "which +backend would THIS client actually use" already exists and is already the one home for +that policy: `backend_registry.adoption_candidates(path, own_context)` +(`backend_registry.py:178`), asymmetric by design — a client that can PROVE it has a +desktop adopts only its own context's entry plus `UNVERIFIED` ones; a client that cannot +adopts anything, window-capable first. `singleton._find_running_server` +(`singleton.py:231`) walks exactly that list at `singleton.py:242`, which is why the +proxies in that same shell were all correctly on 52554. `_probe_backend_status` simply +never consulted it. For `own_context == "win-session-1"` the candidate list is a single +entry — the live one — so the right answer was one call away. + +**The pid line was a SECOND, independent selection**, which is why it could disagree with +the status line rather than merely inherit its error: `cli._recorded_backend_pid` +(`cli.py:144`) did its own `backend_registry.first_backend(singleton._read_server_state())` +at `cli.py:154`. `cli._doctor_port_occupant_line` (`cli.py:274`) made a third at +`cli.py:280`. Three reads of "the backend", none of them agreeing by construction. +(`singleton.stop_backend:607` and `restart_backend:660` had already learned this lesson +and read `backend_on_port(…, port)` — "the entry recorded ON THIS PORT, not merely the +first" — so the pattern the CLI needed was already in the tree.) + +## 3. What is and is not affected + +**Affected — all through the ONE shared `_probe_backend_status`:** + +| verb | consumed at | symptom on the observed record | +|---|---|---| +| `status` | `cli.py:128` (via `_format_backend_status`) | "not running" + dead pid + dead log path | +| `doctor`'s `backend :` / `pid :` / `log :` / `port :` summary lines | `cli.py:381`+ | same four lines, same wrongness | +| `stop` | `singleton.py:600` | targets `win-session-2`'s record: terminates nothing, reports "already stopped", and FORGETS the dead sibling's entry while the live own-context backend keeps running. Not destructive, but it is not the backend the operator asked to stop | +| `kill-orphans` | `cli.py:502` (gate), `cli.py:504` (the pid it names) | the "a backend is running — use restart, or pass `--force`" guard reads `down` where the true answer is `responsive`, so the verb runs its reaper beside a live backend and then prints "reaped any browsers left over from a dead backend" while that backend is serving. The live backend's OWN browsers are still spared — `_recover_orphaned_processes` re-checks recorded ownership through `_owner_backend_alive` (`process_cleanup.py:319`, the F-808 fix) and only `--force` bypasses that — so this is a defeated outer gate, not a fleet kill. The residual is real but narrower: anything recorded under an owner that is NOT alive is reaped, and the operator is told the backend is dead | + +**Not affected:** + +- **Discovery / the proxies.** `_find_running_server` already walks + `adoption_candidates`; every one of the 56 proxies was correctly on 52554. This was + never a routing bug — only a reporting one. +- **`doctor`'s `contexts :` block.** `_doctor_backend_lines` (`cli.py:193`) walks + `window_capable_first` over the WHOLE record and probes each entry on its own port + (`cli.py:231`), so it listed all three backends with per-port liveness correctly. The + information was on screen four lines below the lie. +- **`restart`.** It selects its own port through `_select_backend_port` / + `own_or_first_port` (own-context first) and terminates exactly the port it selected, so + it never aimed at the sibling. Only its post-restart REPORT came through + `_probe_backend_status`. +- **The record itself.** No writer is wrong; `record_backend`'s supersede-by-port rule is + working as designed. What is wrong is only who READS it for a single-value answer. + +**Stale records are pruned by nobody.** A dead backend's entry leaves `server.json` in +exactly two ways: `stop_backend` → `forget_backend` for the context it stopped +(`singleton.py:617`), and `record_backend`'s supersede-by-port when some LATER backend +claims the same port (`backend_registry.py:381-384`). A backend that is killed, crashes, +or whose desktop logs out leaves its entry behind forever — nothing sweeps on pid +liveness, and `2.1.1` / `2.1.3` entries surviving under a `2.1.5` install is the proof. +That is deliberate to a point (a record is a never-raise cache, and "pid gone" is not +"port free"), but it means the dict grows monotonically per display context and the +FIRST entry becomes steadily less likely to be the relevant one over a machine's +lifetime. **Deliberately NOT changed here — see §6.** + +## 4. The fix + +**One home, no second way.** `_probe_backend_status` now walks +`backend_registry.adoption_candidates(SERVER_STATE_FILE, display_context.display_context())` +— the same list, from the same function, in the same order that `_find_running_server` +uses — and reports the first candidate that ANSWERS. With none responsive it reports the +most informative verdict it saw, `wedged` over `down`, because a wedged backend holds a +port and will be evicted while a `down` record names nothing running at all. No selection +policy is introduced: this function still only says which of the offered candidates is +up, and `adoption_candidates` remains the sole home for WHICH candidates those are. +Fixing it here fixes `status`, `doctor`, `stop` and `kill-orphans` at once, because all +four already consumed this one function. + +**The CLI status block now selects once and passes the answer down**, so its lines cannot +disagree with each other: + +- `_format_backend_status(status, port)` is pure formatting and performs no I/O. +- `_recorded_backend_pid(port)` reads `backend_on_port` — the entry on the port just + reported — instead of `first_backend`. This deletes a second selection rather than + adding a parallel one, and adopts the rule `stop_backend`/`restart_backend` already use. +- `_doctor_port_occupant_line(port)` takes that same port (falling back to + `DEFAULT_PORT` when nothing is reported, exactly as before), deleting the third. + +**`status` now says what it is not speaking about.** `_other_records_note(port)` adds one +line when — and only when — the record holds entries besides the reported one: + +``` +backend : running (responsive) on port 52554 +pid : 53836 +log : C:\Users\amind\.stealth-mcp\logs\backend-53836.log +other records: 2 (win-session-2, headless) — run `doctor` for each one's state +version : 2.1.5 +``` + +It never re-decides which backend to report; it names what the reported one is not and +points at the verb that probes them all. A single-entry record prints no such line. + +No new `STEALTH_MCP_*` knob, no new env read, no `typing.Any`, no LOC-budget change +(`singleton.py` 979 → 994 of its 1000 default; `cli.py` 645 → 680; neither is +grandfathered). + +## 5. Verification + +- `tests/test_cli_status_wedged.py::TestCliStatusReportsTheBackendThisClientWouldUse` + (4 tests, hermetic — `SERVER_STATE_FILE` redirected to `tmp_path`, both liveness + primitives patched so no socket is opened and no real process is touched): the observed + three-entry record reports 52554/53836 and its log path for a `win-session-1` client; + the same record reports the live backend for a `headless` client (whose adoption list + tries the dead capable entry FIRST); the other-records line appears with its contexts + and the `doctor` pointer; a single-entry record prints no such line. +- `tests/test_probe_backend_status.py::TestProbeWalksAdoptionOrder` (3 tests, hermetic, + real listeners on OS-assigned ports via the file's existing `responsive_stub` / + `wedged_stub` fixtures): a dead first entry does not hide the live own-context one; a + dead window-capable entry does not end an unproven client's search; a wedged candidate + outranks a dead one. +- RED first: all 7 failed before the product change, the CLI ones with output identical + to the observation (§1). GREEN after. +- Regression scope run: `test_cli.py`, `test_cli_status_wedged.py`, + `test_probe_backend_status.py`, `test_backend_registry.py`, + `test_singleton_stop_restart.py`, `test_singleton_display_routing.py`, + `test_singleton_version_aware.py`, `test_singleton_port_fallback.py`, + `test_singleton_fast_handshake.py`, `test_singleton_backend_logging.py`, + `test_singleton_cold_start_logging.py`, `test_singleton_starvation_patience.py`, + `test_find_running_server_app_probe.py`, `test_proxy_selfheal.py`, + `test_no_silent_excepts.py`, `test_silent_excepts_log.py`, `test_doc_claims.py`, + `test_doc_examples.py`, `test_release_contract.py` — all green. + `ruff check` / `ruff format --check` / `tools/check_file_budgets.py` clean. + +## 6. Not claimed / follow-ups + +- **Pruning stale records is NOT done here, deliberately.** It is a separate decision + with a real safety argument on both sides, and it needs an owner: + - *Who would prune?* The only process that can prune honestly is one that has just + probed. A CLI verb (`status`/`doctor`) must stay read-only by contract (module + docstring, `STEALTH_MCP_NO_AUTO_RECOVERY=1`), so it may not. `stop` + already forgets what it stopped. The natural candidate is the cold-start path under + `_exclusive_lock` — it is the only writer that holds the lock — but a backend whose + pid is gone is not necessarily a backend whose PORT is free, and dropping the entry + loses the record that would let a later spawn step around a squatter. + - *What is the cost of not pruning?* Bounded: one small JSON object per display context + per machine lifetime, and (after this fix) no reader is misled by it. The cost of + pruning wrong is a live sibling made undiscoverable, which F-808 already paid for + once. +- **`stop` on a record with no adoptable entry** now reports `not running` where it + previously reported (and forgot) a foreign proven context's entry. That is the intended + consequence of the same rule — an operator's `stop` should not reach across into + another desktop's backend — but it is a behaviour change, not merely a bug fix, and it + is stated here rather than left to be discovered. +- **`restart`'s post-restart REPORT is still not pinned to the port it spawned on.** + `restart_backend` (`singleton.py:658-661`) takes its `status` from + `_probe_backend_status()` and its `pid` from `backend_on_port(state, port)` for the port + it actually spawned, so on a multi-context record the two halves can describe different + backends — a pre-existing split (`first_backend` could diverge exactly the same way), + narrowed but not closed by this fix, since our own fresh entry now sorts first for a + proven context. Closing it means reporting the SPAWNED port's own liveness, which is a + change to `restart`'s contract, not a report fix; left for its own change. +- **The probe now costs up to one connect attempt per adoptable candidate** instead of + exactly one. Refused loopback connects are immediate; the only slow case is a candidate + whose socket is open but silent (one `LIVENESS_PROBE_TIMEOUT`, 2 s), and the walk stops + at the first responsive one. Not measured on a pathological record — `status` is an + interactive verb with no deadline. +- **`singleton.py` is now at 994 of its 1000-LOC default.** Not a violation, but the next + change to that file will have to pay its way; the module is a standing candidate for + another extraction (`_probe_backend_status` and the per-entry form in + `cli._probe_recorded_backend` are the same ladder read twice, and a `backend_liveness` + leaf would hold both). diff --git a/src/stealth_chrome_devtools_mcp/cli.py b/src/stealth_chrome_devtools_mcp/cli.py index a4d0afc..c8d2d85 100644 --- a/src/stealth_chrome_devtools_mcp/cli.py +++ b/src/stealth_chrome_devtools_mcp/cli.py @@ -113,19 +113,19 @@ def _gb_to_bytes(gb: float | None, fallback: int) -> int: # ── commands ──────────────────────────────────────────────────────────────── -def _format_backend_status() -> str: - """Human-readable backend status for status/doctor, driven by - `_probe_backend_status` (plan_M1 SS2.1-D) instead of `_find_running_ - server`'s binary reuse-or-not answer. Closes F-301's "status prints - *running* through the whole outage" half: a wedged backend (socket open, - dispatch loop dead) now reports UNRESPONSIVE instead of a plain - "running" indistinguishable from a genuinely healthy one. Read-only - - this performs a single initialize+DELETE probe, self-cleaning, same as - every other consumer of `_backend_http_ready` (never evicts or spawns). +def _format_backend_status(status: str, port: int | None) -> str: + """Human-readable backend status for status/doctor, formatting what + `_probe_backend_status` (plan_M1 SS2.1-D) reported instead of + `_find_running_server`'s binary reuse-or-not answer. Closes F-301's "status + prints *running* through the whole outage" half: a wedged backend (socket + open, dispatch loop dead) now reports UNRESPONSIVE instead of a plain + "running" indistinguishable from a genuinely healthy one. + + The probe is the CALLER's (F-868): every line of the status block — + liveness, pid, log path, port occupant — must describe ONE backend, and the + only way to guarantee that is to select it once and pass it down. This + function is pure formatting and performs no I/O at all. """ - from stealth_chrome_devtools_mcp.embedded import singleton - - status, port = singleton._probe_backend_status() # "down" (a stale record but nothing actually listening) and "none" (no # record at all) both read as "not running" to an operator - there is no # live process to reconnect to either way; plan_M1 SS2.1-D's three @@ -141,20 +141,44 @@ def _format_backend_status() -> str: return f"running (responsive) on port {port}" -def _recorded_backend_pid() -> int | None: - """The pid singleton last recorded for the backend (server.json), or None - if there is no record. Independent of liveness — status/doctor combine +def _recorded_backend_pid(port: int | None) -> int | None: + """The pid singleton recorded for the backend on ``port``, or None when + that port names no entry. Independent of liveness — status/doctor combine this with `_format_backend_status()`'s liveness read separately (F-305). - The FIRST recorded backend: the record can now hold one per display context - (F-808), and naming them all belongs to `_doctor_backend_lines`, not to this - one-value line.""" + The backend on THAT port, never the first recorded one (F-868). The record + holds one entry per display context (F-808), so "first" is routinely a + different backend from the one the status line just reported — that split + is what printed a dead sibling's pid under a live backend's status. Same + agree-on-one-port rule `singleton.stop_backend` and `restart_backend` + already apply; naming every entry belongs to `_doctor_backend_lines`.""" from stealth_chrome_devtools_mcp.embedded import backend_registry, singleton - entry = backend_registry.first_backend(singleton._read_server_state()) + entry = backend_registry.backend_on_port(singleton._read_server_state(), port) return backend_registry.recorded_int(entry, "pid") +def _other_records_note(port: int | None) -> str: + """The display contexts recorded BESIDE the one reported, or "" when the + reported backend is the only entry there is (F-868). + + The status block is one summary line over a record that can hold an entry + per display context, and a summary that silently drops the rest reads as + "this is all there is". It never re-decides which backend to report — it + only names what the reported one is not, and points at the verb that probes + them all.""" + from stealth_chrome_devtools_mcp.embedded import backend_registry, singleton + + others = [ + str(entry.get("display_context")) + for entry in backend_registry.backends_in(singleton._read_server_state()) + if backend_registry.recorded_int(entry, "port") != port + ] + if not others: + return "" + return f"{len(others)} ({', '.join(others)}) — run `doctor` for each one's state" + + def _backend_log_location(pid: int | None) -> str: """Where to look for backend logs (F-503's log-path half: M3 delivered "there is now a log"; this delivers "here is where"). Names the exact @@ -271,15 +295,17 @@ def _doctor_backend_lines() -> list[str]: return lines -def _doctor_port_occupant_line() -> str: +def _doctor_port_occupant_line(port: int | None) -> str: """F-509 visibility: is the target port free, ours, or a NON-stealth process squatting it (which would otherwise silently block a backend - from binding)? Uses only existing helpers — no new port logic.""" - from stealth_chrome_devtools_mcp.embedded import backend_registry, singleton + from binding)? Uses only existing helpers — no new port logic. + + ``port`` is the one the status block is already about (F-868), so this line + cannot describe a different backend's port than the two lines above it; with + nothing reported it falls back to the default the next spawn would prefer.""" + from stealth_chrome_devtools_mcp.embedded import singleton - entry = backend_registry.first_backend(singleton._read_server_state()) - recorded = entry.get("port") if entry else None - port = recorded if isinstance(recorded, int) else singleton.DEFAULT_PORT + port = port if port is not None else singleton.DEFAULT_PORT our_pid = singleton._backend_pid_on_port(port) if our_pid is not None: return f"port {port} held by our backend (pid {our_pid})" @@ -293,10 +319,16 @@ def _cmd_status(_args) -> int: from stealth_chrome_devtools_mcp.embedded import singleton root = cs.default_session_root() - pid = _recorded_backend_pid() - print(f"backend : {_format_backend_status()}") + # ONE selection for the whole block (F-868): probe first, then report that + # backend's pid, its log and the records this line is not about. + status, port = singleton._probe_backend_status() + pid = _recorded_backend_pid(port) + others = _other_records_note(port) + print(f"backend : {_format_backend_status(status, port)}") print(f"pid : {pid if pid is not None else '-'}") print(f"log : {_backend_log_location(pid)}") + if others: + print(f"other records: {others}") print(f"version : {singleton._server_version()}") print(f"browser-session root: {root} (exists: {root.exists()})") print( @@ -381,6 +413,8 @@ def _cmd_cleanup(args) -> int: def _cmd_doctor(_args) -> int: import platform + from stealth_chrome_devtools_mcp.embedded import singleton + cs = _clone_storage() ok = True @@ -388,11 +422,12 @@ def _cmd_doctor(_args) -> int: print(f"platform : {platform.platform()}") root = cs.default_session_root() print(f"browser-session root: {root} (exists: {root.exists()})") - pid = _recorded_backend_pid() - print(f"backend : {_format_backend_status()}") + status, port = singleton._probe_backend_status() + pid = _recorded_backend_pid(port) + print(f"backend : {_format_backend_status(status, port)}") print(f"pid : {pid if pid is not None else '-'}") print(f"log : {_backend_log_location(pid)}") - print(f"port : {_doctor_port_occupant_line()}") + print(f"port : {_doctor_port_occupant_line(port)}") print("contexts :") for line in _doctor_backend_lines(): print(f" {line}") @@ -499,9 +534,9 @@ def _cmd_kill_orphans(args) -> int: _server() from stealth_chrome_devtools_mcp.embedded import process_cleanup, singleton - status, _ = singleton._probe_backend_status() + status, port = singleton._probe_backend_status() if status in ("responsive", "wedged") and not args.force: - pid = _recorded_backend_pid() + pid = _recorded_backend_pid(port) print( f"a backend is running (pid {pid if pid is not None else '-'}); " "use restart to recover it, or pass --force." diff --git a/src/stealth_chrome_devtools_mcp/embedded/singleton.py b/src/stealth_chrome_devtools_mcp/embedded/singleton.py index 3083543..8314f7e 100644 --- a/src/stealth_chrome_devtools_mcp/embedded/singleton.py +++ b/src/stealth_chrome_devtools_mcp/embedded/singleton.py @@ -158,27 +158,42 @@ def _clear_server_state() -> None: def _probe_backend_status() -> tuple[str, int | None]: - """Report the recorded backend's actual state for display (CLI status/ - doctor), distinguishing what `_find_running_server`'s binary answer - collapses: not running, socket-dead, and wedged (the F-301 state a bare - socket check cannot see). Read-only: never evicts, never spawns. Doctor - runs this same ladder per-entry in `cli._probe_recorded_backend`. - - Returns ("none", None) no recorded backend | ("down", port) socket closed | + """Report the state of the backend THIS process would be served by, for + display (CLI status/doctor) and for `stop`, distinguishing what + `_find_running_server`'s binary answer collapses: not running, socket-dead, + and wedged (the F-301 state a bare socket check cannot see). Read-only: + never evicts, never spawns. Doctor runs this same ladder per-entry in + `cli._probe_recorded_backend`. + + Candidates come in ADOPTION order (F-868) — `adoption_candidates`, the one + home `_find_running_server` already walks — never "whichever entry the + record lists first", which under one-entry-per-display-context is routinely + a dead sibling's: the 2026-09-14 record listed a dead `win-session-2` ahead + of the healthy own-context backend, so `status` said "not running" beside a + backend serving 56 proxies and `stop` aimed at the dead one's record. The + first candidate that ANSWERS wins, else the most informative verdict — + wedged over down, because a wedged backend holds a port and will be evicted + while a down record names nothing running. Selection is not decided here. + + Returns ("none", None) no adoptable record | ("down", port) socket closed | ("wedged", port) socket open, no real MCP initialize answer | ("responsive", port) socket open AND initialize answers 200. """ - entry = backend_registry.first_backend(_read_server_state()) - if entry is None: - return "none", None - port = entry.get("port") - if not isinstance(port, int): - return "none", None - if not _server_is_healthy(port): - return "down", port - if not _backend_http_ready(port): - return "wedged", port - return "responsive", port + best: tuple[str, int | None] = ("none", None) + own = display_context.display_context() + for entry in backend_registry.adoption_candidates(SERVER_STATE_FILE, own): + port = backend_registry.recorded_int(entry, "port") + if port is None: + continue + if not _server_is_healthy(port): + verdict = ("down", port) + elif not _backend_http_ready(port): + verdict = ("wedged", port) + else: + return "responsive", port + if best[0] == "none" or (best[0] == "down" and verdict[0] == "wedged"): + best = verdict + return best def _same_identity_backend_ready(port: int, patience: float | None = None) -> bool: diff --git a/tests/test_cli_status_wedged.py b/tests/test_cli_status_wedged.py index 1e1ad43..31086b1 100644 --- a/tests/test_cli_status_wedged.py +++ b/tests/test_cli_status_wedged.py @@ -21,7 +21,11 @@ import pytest from stealth_chrome_devtools_mcp import cli -from stealth_chrome_devtools_mcp.embedded import desktop_launch, singleton +from stealth_chrome_devtools_mcp.embedded import ( + desktop_launch, + display_context, + singleton, +) @pytest.fixture(autouse=True) @@ -578,3 +582,139 @@ def test_no_record_says_none_recorded_without_a_remedy( assert "backend (none recorded)" in out assert "no live backend can display a window" not in out + + +@contextmanager +def _live_only_on(port: int): + """Both liveness primitives answering for ONE port and no other. + + `_probe_backend_status` is left REAL on purpose — which recorded backend it + speaks about is exactly what F-868 is about, so stubbing it would stub the + defect out. Nothing here opens a socket: both probes are replaced, so the + port numbers are record data, never a connection. + """ + with ( + patch.object(singleton, "_server_is_healthy", lambda p: p == port), + patch.object(singleton, "_backend_http_ready", lambda p, **_kw: p == port), + ): + yield + + +class TestCliStatusReportsTheBackendThisClientWouldUse: + """F-868: `status` must answer for the backend THIS shell would be served + by — the one `_find_running_server` adopts — not for whichever entry the + record happens to list first. + + The record observed on 2026-09-14 (2.1.5, Windows 11) held three entries; + the first two named dead pids on ports nothing listened on, and the third + was the healthy backend serving 56 proxies in the operator's own session. + `status` reported "not running" and the first entry's dead pid. + """ + + def test_status_reports_the_own_context_live_backend_not_a_dead_sibling( + self, fake_server, recorded_backends, capsys, monkeypatch, tmp_path + ): + recorded_backends( + _v2( + win_session_2={"port": 7169, "pid": 89892, "version": "2.1.1"}, + headless={"port": 19222, "pid": 67720, "version": "2.1.3"}, + win_session_1={"port": 52554, "pid": 53836, "version": "2.1.5"}, + ) + ) + monkeypatch.setattr(display_context, "display_context", lambda: "win-session-1") + with ( + patch.object(cli, "_clone_storage", return_value=fake_server), + patch( + "stealth_chrome_devtools_mcp.embedded.logging_setup.resolve_log_dir", + return_value=tmp_path, + ), + _live_only_on(52554), + ): + cli._cmd_status(None) + out = capsys.readouterr().out + + assert "running (responsive) on port 52554" in out + assert "not running" not in out + # The pid and the log path must describe the SAME backend the status + # line is about — two independent record reads is how they diverged. + assert "53836" in out + assert "backend-53836.log" in out + assert "89892" not in out + + def test_status_reports_the_live_backend_for_a_client_that_adopts_anything( + self, fake_server, recorded_backends, capsys, monkeypatch, tmp_path + ): + """The other half of adoption's asymmetry: a HEADLESS/UNVERIFIED client + adopts any recorded backend, capable-first — so the dead window-capable + entry it tries first must not end the search.""" + recorded_backends( + _v2( + win_session_2={"port": 7169, "pid": 89892, "version": "2.1.1"}, + win_session_1={"port": 52554, "pid": 53836, "version": "2.1.5"}, + ) + ) + monkeypatch.setattr(display_context, "display_context", lambda: "headless") + with ( + patch.object(cli, "_clone_storage", return_value=fake_server), + patch( + "stealth_chrome_devtools_mcp.embedded.logging_setup.resolve_log_dir", + return_value=tmp_path, + ), + _live_only_on(52554), + ): + cli._cmd_status(None) + out = capsys.readouterr().out + + assert "running (responsive) on port 52554" in out + assert "53836" in out + + def test_status_names_the_records_it_is_not_speaking_about( + self, fake_server, recorded_backends, capsys, monkeypatch, tmp_path + ): + """One summary line over a three-entry record silently drops two of + them. It must say they exist and where to read them.""" + recorded_backends( + _v2( + win_session_2={"port": 7169, "pid": 89892, "version": "2.1.1"}, + headless={"port": 19222, "pid": 67720, "version": "2.1.3"}, + win_session_1={"port": 52554, "pid": 53836, "version": "2.1.5"}, + ) + ) + monkeypatch.setattr(display_context, "display_context", lambda: "win-session-1") + with ( + patch.object(cli, "_clone_storage", return_value=fake_server), + patch( + "stealth_chrome_devtools_mcp.embedded.logging_setup.resolve_log_dir", + return_value=tmp_path, + ), + _live_only_on(52554), + ): + cli._cmd_status(None) + out = capsys.readouterr().out + + assert "other records" in out + assert "win-session-2" in out + assert "headless" in out + assert "doctor" in out + + def test_a_single_recorded_backend_gets_no_other_records_line( + self, fake_server, recorded_backends, capsys, monkeypatch, tmp_path + ): + """The note is evidence, not decoration: with nothing else recorded + there is nothing it could honestly say.""" + recorded_backends( + _v2(win_session_1={"port": 52554, "pid": 53836, "version": "2.1.5"}) + ) + monkeypatch.setattr(display_context, "display_context", lambda: "win-session-1") + with ( + patch.object(cli, "_clone_storage", return_value=fake_server), + patch( + "stealth_chrome_devtools_mcp.embedded.logging_setup.resolve_log_dir", + return_value=tmp_path, + ), + _live_only_on(52554), + ): + cli._cmd_status(None) + out = capsys.readouterr().out + + assert "other records" not in out diff --git a/tests/test_probe_backend_status.py b/tests/test_probe_backend_status.py index 1d083a7..67923fb 100644 --- a/tests/test_probe_backend_status.py +++ b/tests/test_probe_backend_status.py @@ -132,3 +132,90 @@ def test_responsive_backend_reports_responsive( status, reported_port = singleton._probe_backend_status() assert status == "responsive" assert reported_port == responsive_stub + + +def _v2(**backends) -> dict: + """A schema-v2 record from `context=entry` kwargs (`_` reads as `-`).""" + return { + "schema": 2, + "backends": {ctx.replace("_", "-"): entry for ctx, entry in backends.items()}, + } + + +class TestProbeWalksAdoptionOrder: + """F-868: the reporter must speak about the backend THIS process would be + served by — `backend_registry.adoption_candidates`' order, the same one + `_find_running_server` walks — not about whichever entry the record lists + first. Reading the first entry made `status` answer "not running" beside a + healthy own-context backend, and made `stop` aim at a dead sibling's record. + + There is no second selection policy here: the order arrives from the one + home, and this function only says which of those candidates is up. + """ + + def test_a_dead_first_entry_does_not_hide_the_live_own_context_one( + self, isolated_state, monkeypatch, responsive_stub + ): + dead = _free_closed_port() + (isolated_state / "server.json").write_text( + json.dumps( + _v2( + win_session_2={"port": dead, "version": "2.1.1", "pid": 89892}, + win_session_1={ + "port": responsive_stub, + "version": "2.1.5", + "pid": 53836, + }, + ) + ) + ) + monkeypatch.setattr( + singleton.display_context, "display_context", lambda: "win-session-1" + ) + + assert singleton._probe_backend_status() == ("responsive", responsive_stub) + + def test_a_dead_capable_entry_does_not_end_an_unproven_clients_search( + self, isolated_state, monkeypatch, responsive_stub + ): + """A HEADLESS/UNVERIFIED client adopts anything, window-capable first — + so the dead capable entry it tries first must not be its answer.""" + dead = _free_closed_port() + (isolated_state / "server.json").write_text( + json.dumps( + _v2( + win_session_2={"port": dead, "version": "2.1.1", "pid": 89892}, + headless={ + "port": responsive_stub, + "version": "2.1.5", + "pid": 67720, + }, + ) + ) + ) + monkeypatch.setattr( + singleton.display_context, "display_context", lambda: "headless" + ) + + assert singleton._probe_backend_status() == ("responsive", responsive_stub) + + def test_a_wedged_candidate_outranks_a_dead_one( + self, isolated_state, monkeypatch, wedged_stub + ): + """With nothing responsive the report still has to name the most + informative state: a wedged backend holds a port and will be evicted; + a down record names nothing running at all.""" + dead = _free_closed_port() + (isolated_state / "server.json").write_text( + json.dumps( + _v2( + win_session_2={"port": dead, "version": "2.1.1", "pid": 89892}, + headless={"port": wedged_stub, "version": "2.1.5", "pid": 67720}, + ) + ) + ) + monkeypatch.setattr( + singleton.display_context, "display_context", lambda: "unverified" + ) + + assert singleton._probe_backend_status() == ("wedged", wedged_stub) From c6d27d0d579583fc4426a8df9c8e01dcc02c04a5 Mon Sep 17 00:00:00 2001 From: AminDhouib Date: Tue, 15 Sep 2026 00:13:21 -0400 Subject: [PATCH 2/3] fix(F-868): one liveness ladder, and restart reports the port it spawned on Review follow-up on the F-868 branch. Three product changes, all of them consequences of the same selection bug the first commit fixed. `singleton._probe_port(port)` is now THE one home for the socket -> `initialize` -> down/wedged/responsive ladder. It was four lines copied into `cli._probe_recorded_backend` under a comment justifying the copy with two claims: that `_probe_backend_status` "reads the FIRST recorded backend" and so could not answer per-entry, and that singleton.py sat at its budget. The first stopped being true in the previous commit; the second was never a reason to have two answers to one question. The CLI form is now a thin adapter keeping only the one word the ladder cannot reach ("no port recorded"). `restart_backend` reports `_probe_port(port)` for the port it spawned on. It took `status` from the record-wide adoption walk while `pid` came from `backend_on_port(state, port)`, so on a multi-context record a responsive SIBLING reported "responsive" beside the pid of a backend that had just come up wedged - two processes in one return, contradicting the docstring's own promise that a wedged restart must be visible. Pinned RED-first: the new test fails against the old reporter with ('responsive', 4242) == ('wedged', 4242), and asserts both halves so it cannot pass by the sibling merely being invisible. `_other_records_note` compares on display context, not port: `backends_in` stamps a context on every entry, whereas a hand-edited non-int port reads as None and would have matched a None reported port - hiding itself in exactly the "nothing is running" case that needs it. Its label is `others :`, aligned with every other label in the block, and the RUNBOOK sample is now byte-identical to what ships. Also: `stop`'s narrowing to adoptable contexts is pinned (green both sides by construction - it exists so the behaviour change is not reverted by accident, and the finding says so rather than claiming a RED); the `_v2` record builder moved to tests/fakes.py as `v2_record`, where the two test modules that had copies now import it along with the existing `pretend_display_context`; and the finding's RED claim is corrected to 6 of 7, the single-entry no-note case being green on both sides. singleton.py lands at 999/1000 LOC. Two lines were paid for honestly - the F-856 paragraph in `_same_identity_backend_ready` was retelling what `scheduling_lag.FairWindow` is THE one home for and now points at it - but one line of headroom is the most fragile thing here. The next change to that file needs the `backend_liveness` extraction (probes handed in as arguments, the way backend_watchdog already does), not more prose-trimming; recorded in CLAUDE.md and finding section 6. --- CHANGELOG.md | 15 +- CLAUDE.md | 4 +- RUNBOOK.md | 4 +- ...g_F868_cli_status_reports_a_dead_record.md | 93 ++++++++--- src/stealth_chrome_devtools_mcp/cli.py | 55 ++++--- .../embedded/singleton.py | 81 +++++----- tests/fakes.py | 20 +++ tests/test_cli_status_wedged.py | 57 +++---- tests/test_probe_backend_status.py | 27 +--- tests/test_singleton_stop_restart.py | 147 +++++++++++++++--- 10 files changed, 341 insertions(+), 162 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9d4cacc..1a787a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,8 +19,19 @@ it. The CLI status block now also selects once and passes the answer down: the p log lines read the entry on the port just reported (`backend_on_port`) instead of making their own `first_backend` read, and `doctor`'s port-occupant line takes the same port — two independent record selections deleted rather than a third added. `status` gained one -line naming the display contexts it is NOT speaking about when the record holds more than -one, so a summary over a multi-context record no longer reads as "this is all there is". +`others :` line naming the display contexts it is NOT speaking about when the record +holds more than one, so a summary over a multi-context record no longer reads as "this is +all there is". + +Two things the same selection bug was hiding are fixed with it. The socket→`initialize` +ladder is now `singleton._probe_port`, one home with three callers, instead of four lines +copied into `cli._probe_recorded_backend` under a comment justifying the copy with a claim +about `_probe_backend_status` that this release makes false. And `restart` now reports +that ladder's verdict for **the port it spawned on**: it took its `status` from the +record-wide walk while its `pid` came from the spawned port, so a responsive sibling could +report "responsive" beside the pid of a backend that had just come up wedged — both halves +of one return describing two processes. + Stale records are still pruned by nobody; see `audit/stage2/finding_F868_cli_status_reports_a_dead_record.md` §6. ## 2.1.5 diff --git a/CLAUDE.md b/CLAUDE.md index 6a62aad..acfc71b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -47,7 +47,7 @@ Package root: `src/stealth_chrome_devtools_mcp/`. Two console scripts (`pyprojec |---|---| | `server.py` | thin entrypoint — loads `embedded/server.py` as `__main__` via `runpy` (`main()` shim); its stdio branch is also **THE one place the PROXY process bootstraps its own observability** (`configure_logging("proxy")` + `_start_proxy_error_reporting`, F-827) — in the branch, never at the top of `main()`, or the runpy path would double-init | | `__main__.py` | `python -m stealth_chrome_devtools_mcp` → `server.main()` | -| `cli.py` | the `stealth-chrome-devtools` ops CLI verbs (`status`/`doctor`/`stop`/`restart`/`cleanup`/`kill-orphans`/`serve`). The status block **selects the backend ONCE** (F-868): `_probe_backend_status()` is called by the command, and its `(status, port)` is passed down to every line — `_format_backend_status` (now pure formatting, no I/O), `_recorded_backend_pid(port)` and `_doctor_port_occupant_line(port)` (both read the entry on THAT port via `backend_on_port`, never `first_backend`), and `_other_records_note(port)`, the one line that names the display contexts the summary is *not* about. Two independent record reads were deleted to get here; do not re-add one — a per-line read is how a live backend's status came to sit above a dead sibling's pid | +| `cli.py` | the `stealth-chrome-devtools` ops CLI verbs (`status`/`doctor`/`stop`/`restart`/`cleanup`/`kill-orphans`/`serve`). The status block **selects the backend ONCE** (F-868): `_probe_backend_status()` is called by the command, and its `(status, port)` is passed down to every line — `_format_backend_status` (now pure formatting, no I/O), `_recorded_backend_pid(port)` and `_doctor_port_occupant_line(port)` (both read the entry on THAT port via `backend_on_port`, never `first_backend`), and `_other_records_note(port)`, the `others :` line that names the display contexts the summary is *not* about (compared on display context, never on port — a hand-edited non-int port reads as `None` and would hide itself). Two independent record reads were deleted to get here; do not re-add one — a per-line read is how a live backend's status came to sit above a dead sibling's pid. `_probe_recorded_backend` is a thin adapter over `singleton._probe_port`, not a second ladder | | `settings.py` | **the one env home** — pydantic `Settings` + `get_settings()`; every `STEALTH_MCP_*` knob is a typed field here | | `observability.py` | Sentry error shipping — hardcoded DSN, on by default, never raises (no-op under `STEALTH_MCP_NO_ERROR_REPORTING`); **the one PII scrubber** — `_scrub_event` (Sentry's `before_send`); **the one non-exception report** — `capture_lifecycle` (F-827: proxy transitions that are decisions, not crashes) | @@ -57,7 +57,7 @@ Package root: `src/stealth_chrome_devtools_mcp/`. Two console scripts (`pyprojec | File | Owns | |---|---| | `server.py` | the real MCP server — **ZERO tool bodies**; all 94 live in `tool_sections/` (plan_SERVERSPLIT, complete at slice 12). What this file owns, and nothing else: the per-execution FastMCP app (`mcp`/`registry`), `app_lifespan`, the four `@mcp.resource` handlers, the xpool-safe gate, `build_arg_parser`, the `__main__` block, and the **binding loop** that drives registration from THIS file's module body, once per execution of it (so the canonical import, the bare-name spec load and the runpy `__main__` load each get a full 94-tool app — a section module that decorated itself would register into the first execution only). **No migration alias block**: every singleton, knob and guard this file drives is read as `rt.` against `tool_runtime`, the one patchable home, exactly as a tool body does. The lone import left is `clone_storage`, which this file never touches — it is the positive delegation handle `tests/test_clone_storage.py`'s F-201 negative-surface pin needs. At 523 LOC it is governed by the 1000-LOC default; its `GRANDFATHER` row is deleted, not merely satisfied | -| `singleton.py` | **backend lifecycle + the stdio proxy** — liveness probes (`_backend_http_ready`; `_probe_backend_status`, which is **THE one answer to "what state is the backend THIS process would be served by in"** — since F-868 it walks `backend_registry.adoption_candidates` in the same order `_find_running_server` does and reports the first candidate that answers, wedged over down when none does, never `first_backend`'s "whichever entry the record lists first"; `status`, `doctor`, `stop` and `kill-orphans`'s live-backend guard all consume this ONE function, so the record selection is fixed in exactly one place), port selection (`_select_backend_port`, `DEFAULT_PORT`), the one identity+readiness reuse gate (`_same_identity_backend_ready`, `_source_fingerprint`, `REUSE_PATIENCE_SECONDS` — which spends its patience through `scheduling_lag.FairWindow`, F-856), cold-start lock (`_start_backend_holding_lock`), `run_stdio_proxy`. The `server.json` record moved to `backend_registry.py` (path names re-exported here for legacy callers); the proxy's *recovery from a dead backend* moved to `proxy_selfheal.py`; the watchdog LOOP moved to `backend_watchdog.py` — `_watch_backend_liveness` stays here as the wiring that knows which probes are ours; and **the spawn itself moved to `backend_launch.py` (F-867)** — `_start_server_process` still owns WHAT to start (`_server_process_cmd`, `_backend_interpreter`'s F-866 base-interpreter choice, the child env, F-830's boot-log roll) and what to RECORD afterwards (`PORT_FILE`, `_write_server_state`), and makes exactly ONE call, `backend_launch.spawn(cmd, child_env, boot_log) -> Launched(pid, rung)`; it no longer imports `subprocess` | +| `singleton.py` | **backend lifecycle + the stdio proxy** — liveness probes (`_backend_http_ready`; **`_probe_port(port)`, THE one home for the socket→`initialize`→`down`/`wedged`/`responsive` ladder** (F-868) with three callers — the candidate walk, `restart_backend`, and `cli._probe_recorded_backend`, which adds only the one word the ladder cannot reach, `"no port recorded"`; never copy those four lines again; and `_probe_backend_status`, **THE one answer to "what state is the backend THIS process would be served by in"** — it walks `backend_registry.adoption_candidates` in the same order `_find_running_server` does and reports the first candidate that answers, wedged over down when none does, never `first_backend`'s "whichever entry the record lists first"; `status`, `doctor`, `stop` and `kill-orphans`'s live-backend guard all consume this ONE function, so the record selection is fixed in exactly one place. `restart_backend` deliberately does NOT: it reports `_probe_port` for the port it spawned on, because the adoption walk answers "is there a backend for me" and a responsive SIBLING must never speak for a restart's own fresh backend). **At 999/1000 LOC this file has one line of headroom** — the next change to it needs the `backend_liveness` extraction (`_probe_port` + the walk, probes handed in as arguments the way `backend_watchdog` already does), not more prose-trimming), port selection (`_select_backend_port`, `DEFAULT_PORT`), the one identity+readiness reuse gate (`_same_identity_backend_ready`, `_source_fingerprint`, `REUSE_PATIENCE_SECONDS` — which spends its patience through `scheduling_lag.FairWindow`, F-856), cold-start lock (`_start_backend_holding_lock`), `run_stdio_proxy`. The `server.json` record moved to `backend_registry.py` (path names re-exported here for legacy callers); the proxy's *recovery from a dead backend* moved to `proxy_selfheal.py`; the watchdog LOOP moved to `backend_watchdog.py` — `_watch_backend_liveness` stays here as the wiring that knows which probes are ours; and **the spawn itself moved to `backend_launch.py` (F-867)** — `_start_server_process` still owns WHAT to start (`_server_process_cmd`, `_backend_interpreter`'s F-866 base-interpreter choice, the child env, F-830's boot-log roll) and what to RECORD afterwards (`PORT_FILE`, `_write_server_state`), and makes exactly ONE call, `backend_launch.spawn(cmd, child_env, boot_log) -> Launched(pid, rung)`; it no longer imports `subprocess` | | `backend_launch.py` | **THE one home for "spawn the backend where no MCP client's Job Object can reach it"** (F-867) — `spawn`, `Launched(pid, rung)` and nothing else in the tree may create a backend. POSIX is one rung (`posix`, `start_new_session=True`, unchanged). Windows climbs three, and the rung that served is logged through the ONE `_log_rung` line on `stealth.proxy` so a post-mortem can read it: (1) `breakaway` — `CREATE_BREAKAWAY_FROM_JOB`, accepted ONLY when `_proven_in_a_job` says the new process is in NO job, because under a nested job chain the flag leaves the innermost job only and a "successful" breakaway can still sit in the client's; `ERROR_ACCESS_DENIED` (what the SDK's job returns) drops a rung while any other `OSError` propagates. A PARTIAL escape is never discarded blindly — rung 2's viability (`_scheduler_plan`) is decided FIRST, and when there is no scheduler rung the child is KEPT as the `breakaway-partial` rung at WARNING, because out of one job beats out of none; conversely, breakaway once proven PERMITTED is re-asked on rung 3. (2) `scheduler` — a one-shot Task Scheduler task through `desktop_launch._schtasks` (the F-810 seam, not a second home) running `pythonw.exe` (no console, so no window can flash) on the BASE interpreter, never a venv's `Scripts\pythonw.exe`, which is F-866's redirector and would rebuild the kill-on-close job; the stdlib-only `_LAUNCHER_SCRIPT` reads argv + the ENTIRE child env + the boot-log path from a JSON spec in `~/.stealth-mcp/backend-launch/` (the command line carries two paths only because **`schtasks` stores at most 253 chars of `/TR` and truncates beyond that with exit 0** — measured, not F-810's documented "~261"; the silent truncation costs a Last-Result-2 task, the whole 20 s pid deadline and a drop to `plain`, which is why the per-attempt token is 12 hex chars, not 32), re-opens `backend-boot.log` itself and gives the backend stdout AND stderr (F-303 survives the hand-off), and returns the SERVING pid through an `os.replace`d pid file. **Gated on `_same_session_as_console`**: taken only when the spawner is already in the logged-on console session, so the backend lands where it would have anyway and the display context `singleton` records stays true (F-808 — the tool still never PICKS a session). Task and scratch files deleted in a `finally`, and the orphan sweep that runs before a scheduler spawn is a FILESYSTEM sweep, not a `/Query`: any `.json` spec (token = 12 hex) older than 2× the pid deadline gets its `stealth-mcp-backend-` task deleted BY NAME and its files removed. Deliberately not "delete tasks whose spec is absent" — `_cleanup` deletes the task first, so a killed spawner leaves task AND spec, which is the only orphan class there is and the one that predicate missed. A live sibling's spec is younger and untouched; a clean dir costs zero `schtasks` calls. (3) `plain` — today's `DETACHED_PROCESS \| CREATE_NEW_PROCESS_GROUP` (plus the breakaway bit when rung 1 proved it permitted), for a runner with no console session; its line names F-867 and says the backend is inside this client's job. A leaf: `backend_registry` for the state dir, `desktop_launch` lazily for the ONE `schtasks` seam, the ONE pid-file reader and the ONE task teardown, so none of them gets a second home. Never raises for a rung's own failure — only a genuine `Popen` error reaches the caller | | `backend_watchdog.py` | **THE one home for the proxy's mid-session liveness watchdog** — the SLOW witness (`watch_liveness`): F-820's strikes plus the confirmation phase. A leaf: both probes arrive as arguments, so it never imports `singleton` and the dead-vs-busy policy stays single-homed in the reuse gate | | `scheduling_lag.py` | **THE one home for "was this process scheduled fairly, and what does a time budget owe it when it was not"** (F-856) — `FairWindow`, whose budget is charged in fair seconds (elapsed ÷ the lag its own naps measured), plus `MAX_STRETCH`, `REPORT_FACTOR` and the `proxy: patience extended under starvation` lifecycle report. It never decides alive-or-dead: only "has this window been spent", so `proxy_selfheal`'s ONE heal path is untouched. A leaf; the `_now`/`_wait` module functions are its single timing seam | diff --git a/RUNBOOK.md b/RUNBOOK.md index 35427eb..86fd00a 100644 --- a/RUNBOOK.md +++ b/RUNBOOK.md @@ -57,10 +57,10 @@ browser-session cap : 20.0 GB [STEALTH_MCP_BROWSER_SESSION_STORAGE_CAP_GB] adoption order discovery uses (F-868), not whichever entry `server.json` lists first. `pid` and `log` name that same backend, so the four lines can never describe different processes. `server.json` can hold one entry per display context, and dead ones are - never pruned, so an `other records:` line appears when there are others: + never pruned, so an `others` line appears when there are others: ``` - other records: 2 (win-session-2, headless) — run `doctor` for each one's state + others : 2 backends recorded (win-session-2, headless) — run `doctor` for each one's state ``` `doctor`'s `contexts :` block probes every recorded backend on its own port; that is diff --git a/audit/stage2/finding_F868_cli_status_reports_a_dead_record.md b/audit/stage2/finding_F868_cli_status_reports_a_dead_record.md index b4798fa..05007d5 100644 --- a/audit/stage2/finding_F868_cli_status_reports_a_dead_record.md +++ b/audit/stage2/finding_F868_cli_status_reports_a_dead_record.md @@ -123,6 +123,15 @@ lifetime. **Deliberately NOT changed here — see §6.** ## 4. The fix +**The liveness ladder now has one home.** `singleton._probe_port(port)` — socket, then a +real MCP `initialize`, returning `down` / `wedged` / `responsive` — was four lines +duplicated in `cli._probe_recorded_backend`, justified there by a note saying +`_probe_backend_status` "reads the FIRST recorded backend" and so could not answer +per-entry. That justification dies with this fix, and a duplicated liveness ladder was a +second way to answer one question regardless. It now has three callers: the candidate +walk, `restart_backend`, and the CLI form, which keeps only the one word the ladder +cannot reach ("no port recorded"). + **One home, no second way.** `_probe_backend_status` now walks `backend_registry.adoption_candidates(SERVER_STATE_FILE, display_context.display_context())` — the same list, from the same function, in the same order that `_find_running_server` @@ -144,6 +153,16 @@ disagree with each other: - `_doctor_port_occupant_line(port)` takes that same port (falling back to `DEFAULT_PORT` when nothing is reported, exactly as before), deleting the third. +**`restart`'s report is pinned to the port it spawned on.** `restart_backend` took its +`status` from `_probe_backend_status()` while its `pid` came from `backend_on_port(…, +port)`, so on a multi-context record a responsive SIBLING could report "responsive" beside +the pid of a backend that had just come up wedged — the two halves of one return +describing two processes, and a direct contradiction of the docstring's promise that "a +restart that comes back wedged or down must be visible". Both halves now read the one +selected port via `_probe_port(port)`, so they agree by construction. The adoption walk +answers "is there a backend for me", which is the right question for `status` and the +wrong one here. + **`status` now says what it is not speaking about.** `_other_records_note(port)` adds one line when — and only when — the record holds entries besides the reported one: @@ -151,16 +170,20 @@ line when — and only when — the record holds entries besides the reported on backend : running (responsive) on port 52554 pid : 53836 log : C:\Users\amind\.stealth-mcp\logs\backend-53836.log -other records: 2 (win-session-2, headless) — run `doctor` for each one's state +others : 2 backends recorded (win-session-2, headless) — run `doctor` for each one's state version : 2.1.5 ``` It never re-decides which backend to report; it names what the reported one is not and points at the verb that probes them all. A single-entry record prints no such line. +"Other" is decided on DISPLAY CONTEXT, not on port: `backends_in` stamps a context on +every entry, so the comparison is always against a real value, whereas a hand-edited entry +whose `port` is a string reads as `None` and would have matched a `None` reported port — +hiding itself in precisely the "nothing is running" case that needs it most. -No new `STEALTH_MCP_*` knob, no new env read, no `typing.Any`, no LOC-budget change -(`singleton.py` 979 → 994 of its 1000 default; `cli.py` 645 → 680; neither is -grandfathered). +No new `STEALTH_MCP_*` knob, no new env read, no `typing.Any`, no LOC-budget change: +`cli.py` 645 → 691, and `singleton.py` 979 → **999 of its 1000 default** — see §6, this +is now the binding constraint on the file. ## 5. Verification @@ -176,8 +199,27 @@ grandfathered). `wedged_stub` fixtures): a dead first entry does not hide the live own-context one; a dead window-capable entry does not end an unproven client's search; a wedged candidate outranks a dead one. -- RED first: all 7 failed before the product change, the CLI ones with output identical - to the observation (§1). GREEN after. +- `tests/test_singleton_stop_restart.py::TestRestartReportsTheSpawnedPort` (1 test): a + responsive SIBLING must not answer for a restart whose own fresh backend came up + wedged. RED against the pre-fix reporter with `('responsive', 4242) == ('wedged', 4242)` + — the sibling speaking for the wrong process — and it asserts BOTH halves, so it cannot + pass by the sibling merely being invisible: `_probe_backend_status()` is separately + shown to say `("responsive", sibling_port)` at the same moment. +- `tests/test_singleton_stop_restart.py::TestStopIsPerDisplayContext` (1 test): a record + holding only a FOREIGN proven context, own context a different proven desktop → + `stop_backend()` is `("not running", None)`, `_terminate_backend` is never called, and + the foreign entry is still recorded afterward. **Not RED-first, and deliberately so** — + it is green on this branch by construction, because the adoption walk IS the fix. It is + pinned because it is a BEHAVIOUR CHANGE riding on a bug fix (§6), and an unpinned + behaviour change is the kind a later "simplification" reverts without noticing. +- RED first: **6 of the 7 status tests failed before the product change** (plus the + restart pin above, 7 of 8 overall), the CLI ones with output identical to the + observation (§1). The seventh, + `test_a_single_recorded_backend_gets_no_other_records_line`, is green on BOTH sides by + construction and is not claimed as a RED: with a single-entry record `first_backend` and + the adoption walk select the same entry, and the assertion is a negative one about a + line that did not exist before. It earns its place as the boundary of the new note, not + as evidence of the defect. GREEN after. - Regression scope run: `test_cli.py`, `test_cli_status_wedged.py`, `test_probe_backend_status.py`, `test_backend_registry.py`, `test_singleton_stop_restart.py`, `test_singleton_display_routing.py`, @@ -209,21 +251,34 @@ grandfathered). consequence of the same rule — an operator's `stop` should not reach across into another desktop's backend — but it is a behaviour change, not merely a bug fix, and it is stated here rather than left to be discovered. -- **`restart`'s post-restart REPORT is still not pinned to the port it spawned on.** - `restart_backend` (`singleton.py:658-661`) takes its `status` from - `_probe_backend_status()` and its `pid` from `backend_on_port(state, port)` for the port - it actually spawned, so on a multi-context record the two halves can describe different - backends — a pre-existing split (`first_backend` could diverge exactly the same way), - narrowed but not closed by this fix, since our own fresh entry now sorts first for a - proven context. Closing it means reporting the SPAWNED port's own liveness, which is a - change to `restart`'s contract, not a report fix; left for its own change. +- **`restart`'s post-restart report: CLOSED here** (it was scoped out of the first draft + and put back on review). `_probe_port` made it a one-line change rather than a contract + change — see §4 and the `TestRestartReportsTheSpawnedPort` pin in §5. +- **`stop`'s narrowing is a behaviour change, and is now pinned** rather than only + described: `TestStopIsPerDisplayContext` (§5). On a record whose only entries are + foreign PROVEN contexts, `stop` reports `not running` and leaves them alone, where it + previously reported one, terminated nothing, and then forgot the record — quietly making + a possibly-live sibling undiscoverable. That pin is green on both sides of the fix by + construction; it exists to stop the change being reverted by accident, not as evidence. - **The probe now costs up to one connect attempt per adoptable candidate** instead of exactly one. Refused loopback connects are immediate; the only slow case is a candidate whose socket is open but silent (one `LIVENESS_PROBE_TIMEOUT`, 2 s), and the walk stops at the first responsive one. Not measured on a pathological record — `status` is an interactive verb with no deadline. -- **`singleton.py` is now at 994 of its 1000-LOC default.** Not a violation, but the next - change to that file will have to pay its way; the module is a standing candidate for - another extraction (`_probe_backend_status` and the per-entry form in - `cli._probe_recorded_backend` are the same ladder read twice, and a `backend_liveness` - leaf would hold both). +- **`singleton.py` is at 999 of its 1000-LOC default — one line of headroom. This is the + most fragile thing in this change and it needs a decision, not a note.** The gate passes + and nothing here is over budget, but the next contributor to that file has nowhere to + put a line. Two of the +20 were paid for honestly (the F-856 paragraph in + `_same_identity_backend_ready` was retelling what `scheduling_lag.FairWindow` is THE one + home for, and now points at it instead); the rest is the F-868 reasoning, which belongs + with the code it justifies. + + The right next change is an extraction, and it should be its own PR rather than more + prose-trimming, which is just padding a cap from the other side. The shape is already + proven in this tree: `backend_watchdog.py` takes **both probes as arguments** so it never + imports `singleton` and the dead-vs-busy policy stays single-homed. A `backend_liveness` + leaf holding `_probe_port` plus the adoption walk, with `_server_is_healthy` / + `_backend_http_ready` handed in, would move ~35 lines out and leave thin wrappers on + `singleton` so every existing `monkeypatch.setattr(singleton, …)` in the suite keeps + working. I did NOT do it here: it lands mid-review-cycle, touches patch surfaces across + a dozen test modules, and would bury a four-line truthfulness fix under a refactor. diff --git a/src/stealth_chrome_devtools_mcp/cli.py b/src/stealth_chrome_devtools_mcp/cli.py index c8d2d85..f18561a 100644 --- a/src/stealth_chrome_devtools_mcp/cli.py +++ b/src/stealth_chrome_devtools_mcp/cli.py @@ -166,17 +166,31 @@ def _other_records_note(port: int | None) -> str: per display context, and a summary that silently drops the rest reads as "this is all there is". It never re-decides which backend to report — it only names what the reported one is not, and points at the verb that probes - them all.""" + them all. + + "Other" is decided on DISPLAY CONTEXT, not on the port: `backends_in` + stamps a context on every entry (the v2 key, or `UNVERIFIED` for a v1 + record), so the comparison is always against a real value, whereas a + hand-edited entry whose `port` is a string reads as `None` and would have + matched a `None` reported port — silently hiding itself in exactly the + "nothing is running" case where the operator most needs to see it. With + nothing reported at all, every recorded entry is correctly an "other".""" from stealth_chrome_devtools_mcp.embedded import backend_registry, singleton + state = singleton._read_server_state() + reported = backend_registry.backend_on_port(state, port) + mine = reported.get("display_context") if reported else None others = [ str(entry.get("display_context")) - for entry in backend_registry.backends_in(singleton._read_server_state()) - if backend_registry.recorded_int(entry, "port") != port + for entry in backend_registry.backends_in(state) + if entry.get("display_context") != mine ] if not others: return "" - return f"{len(others)} ({', '.join(others)}) — run `doctor` for each one's state" + return ( + f"{len(others)} backends recorded ({', '.join(others)}) — " + "run `doctor` for each one's state" + ) def _backend_log_location(pid: int | None) -> str: @@ -191,27 +205,22 @@ def _backend_log_location(pid: int | None) -> str: def _probe_recorded_backend(port: int | None) -> str: """One recorded backend's liveness on a port the caller already holds, in - `_probe_backend_status`'s exact vocabulary: down / wedged / responsive — - plus "no port recorded" for an entry naming nothing usable as a port, a - state that function cannot reach (it reports such a record as no backend - at all, and so has no word for it). - - Same two primitives in the same order producing the same three words as - `singleton._probe_backend_status` (ONE liveness vocabulary, plan_M8 - SS2.1-B) — all that differs is where the port comes from. That function - reads the FIRST recorded backend, which cannot answer for a record holding - one per display context (F-808), and singleton.py sits at its LOC budget, - so the per-port form lives here rather than beside it. It reaches the - primitives THROUGH the module (`singleton._server_is_healthy`), never by - importing their names, so a test that patches singleton still reaches them. + the ONE liveness vocabulary (plan_M8 SS2.1-B): down / wedged / responsive, + plus "no port recorded" for an entry naming nothing usable as a port. + + That fourth word is the whole of what this adds. The ladder itself is + `singleton._probe_port` and is CALLED, not copied (F-868) — it used to be + the same four lines in both files, justified by a note that + `_probe_backend_status` "reads the FIRST recorded backend" and so could not + answer per-entry. It no longer does, and duplicating a liveness ladder was + a second way to answer one question regardless. Reached THROUGH the module, + never by importing the name, so a test that patches singleton still wins. """ from stealth_chrome_devtools_mcp.embedded import singleton if port is None: return "no port recorded" - if not singleton._server_is_healthy(port): - return "down" - return "responsive" if singleton._backend_http_ready(port) else "wedged" + return singleton._probe_port(port) def _doctor_backend_lines() -> list[str]: @@ -224,7 +233,9 @@ def _doctor_backend_lines() -> list[str]: is refused when the backend serving it can neither display a window nor hand the launch to a logged-on desktop (F-810), and that refusal points the operator at this command, so it must be able to name every context — the - summary line, which reports the first recorded backend only, cannot. + summary line above, which reports the ONE backend this shell would be served + by (`_probe_backend_status`'s adoption walk, F-868), cannot: every entry a + proven-capable client will not adopt is missing from it by design. Ordering is `backend_registry.window_capable_first`'s, so doctor presents the same preference discovery applies rather than re-deriving one. @@ -328,7 +339,7 @@ def _cmd_status(_args) -> int: print(f"pid : {pid if pid is not None else '-'}") print(f"log : {_backend_log_location(pid)}") if others: - print(f"other records: {others}") + print(f"others : {others}") print(f"version : {singleton._server_version()}") print(f"browser-session root: {root} (exists: {root.exists()})") print( diff --git a/src/stealth_chrome_devtools_mcp/embedded/singleton.py b/src/stealth_chrome_devtools_mcp/embedded/singleton.py index 8314f7e..63d0d97 100644 --- a/src/stealth_chrome_devtools_mcp/embedded/singleton.py +++ b/src/stealth_chrome_devtools_mcp/embedded/singleton.py @@ -157,27 +157,34 @@ def _clear_server_state() -> None: backend_registry.clear_record(SERVER_STATE_FILE, PORT_FILE) +def _probe_port(port: int) -> str: + """THE liveness ladder for ONE port — socket, then a real MCP `initialize`: + "down" | "wedged" | "responsive" (F-301's third state, which a bare socket + check cannot see). Read-only. THE one home for those four lines (F-868), + with three readers: the candidate walk below, `restart_backend`'s report of + the port it spawned on, and doctor's `cli._probe_recorded_backend`, which + adds only the one word this cannot reach ("no port recorded") and was a + verbatim copy of this ladder until now. + """ + if not _server_is_healthy(port): + return "down" + return "responsive" if _backend_http_ready(port) else "wedged" + + def _probe_backend_status() -> tuple[str, int | None]: """Report the state of the backend THIS process would be served by, for - display (CLI status/doctor) and for `stop`, distinguishing what - `_find_running_server`'s binary answer collapses: not running, socket-dead, - and wedged (the F-301 state a bare socket check cannot see). Read-only: - never evicts, never spawns. Doctor runs this same ladder per-entry in - `cli._probe_recorded_backend`. + display (CLI status/doctor) and for `stop`: `_probe_port`'s verdict and the + port it was reached on, or ("none", None) when no adoptable entry names a + port. What this adds over that ladder is WHICH port to ask about. Candidates come in ADOPTION order (F-868) — `adoption_candidates`, the one home `_find_running_server` already walks — never "whichever entry the record lists first", which under one-entry-per-display-context is routinely - a dead sibling's: the 2026-09-14 record listed a dead `win-session-2` ahead - of the healthy own-context backend, so `status` said "not running" beside a - backend serving 56 proxies and `stop` aimed at the dead one's record. The + a dead sibling's: that is how `status` came to report "not running" beside + a backend serving 56 proxies, and `stop` to aim at the dead record. The first candidate that ANSWERS wins, else the most informative verdict — - wedged over down, because a wedged backend holds a port and will be evicted - while a down record names nothing running. Selection is not decided here. - - Returns ("none", None) no adoptable record | ("down", port) socket closed | - ("wedged", port) socket open, no real MCP initialize answer | - ("responsive", port) socket open AND initialize answers 200. + wedged over down: a wedged backend holds a port and will be evicted, a down + record names nothing running. The ORDER itself is not decided here. """ best: tuple[str, int | None] = ("none", None) own = display_context.display_context() @@ -185,14 +192,11 @@ def _probe_backend_status() -> tuple[str, int | None]: port = backend_registry.recorded_int(entry, "port") if port is None: continue - if not _server_is_healthy(port): - verdict = ("down", port) - elif not _backend_http_ready(port): - verdict = ("wedged", port) - else: - return "responsive", port - if best[0] == "none" or (best[0] == "down" and verdict[0] == "wedged"): - best = verdict + verdict = _probe_port(port) + if verdict == "responsive": + return verdict, port + if best[0] == "none" or (best[0] == "down" and verdict == "wedged"): + best = (verdict, port) return best @@ -213,14 +217,11 @@ def _same_identity_backend_ready(port: int, patience: float | None = None) -> bo ``patience=0.0`` (discovery) probes once and never sleeps; ``None`` means ``REUSE_PATIENCE_SECONDS``, read at call time so tests can shrink it. - F-856: the window is spent in FAIRLY SCHEDULED seconds, not wall seconds. - A probe timeout is evidence about the backend only while this process is - itself being scheduled, and on a machine at 100% CPU it is not — the - 2026-09-02 incident condemned a backend that had answered its six previous - confirmations. ``scheduling_lag.FairWindow`` measures that lateness from - the loop's own naps and discounts the budget by it, bounded at - ``MAX_STRETCH``. On an idle machine the measurement is 1.0 and this is - exactly the wall-clock deadline it replaced. + F-856: the window is spent in FAIRLY SCHEDULED seconds, not wall seconds — + a probe timeout is evidence about the backend only while this process is + itself being scheduled. The measurement, its bound and the 2026-09-02 + incident behind it are ``scheduling_lag.FairWindow``'s, THE one home for + that question; on an idle machine it is the wall-clock deadline it replaced. """ # The entry recorded ON THIS PORT, not merely the first: under F-808's # per-context record another desktop's backend says nothing about `port`. @@ -651,12 +652,17 @@ def restart_backend() -> tuple[str, int | None]: instead of a repeat 120s outage — the fallback port stays recorded (SSA1.5); `stop` clears `server.json`, the reset path to `DEFAULT_PORT`. Lock contention reports "busy" so the operator retries instead of racing. The - post-restart state is reported via `_probe_backend_status()` (binding - ruling: ONE liveness vocabulary) — a restart that comes back wedged or down - must be visible, not assumed "responsive". + post-restart state is `_probe_port`'s verdict for THE PORT WE SPAWNED ON + (binding ruling: ONE liveness vocabulary) — a restart that comes back + wedged or down must be visible, not assumed "responsive". + + That port, never `_probe_backend_status()`'s (F-868): the adoption walk + answers "is there a backend for ME", so on a multi-context record a + RESPONSIVE sibling reported "responsive" for a restart whose own backend + came up wedged. Both halves now read the one selected port. - Returns ``(status, pid)``: `_probe_backend_status`'s status or "busy"; - ``pid`` is the freshly recorded pid once the lock is acquired, else None. + Returns ``(status, pid)``: `_probe_port`'s verdict or "busy"; ``pid`` is the + freshly recorded pid once the lock is acquired, else None. """ # A PREFERENCE only: selection re-derives our own context's port itself. own = display_context.display_context() @@ -670,10 +676,9 @@ def restart_backend() -> tuple[str, int | None]: _start_server_process(port) _wait_for_server(port) - status, _ = _probe_backend_status() - # The port WE spawned on, not first_backend's - same agree-on-one-port rule. + # Both halves read the port WE spawned on - the agree-on-one-port rule. fresh = backend_registry.backend_on_port(_read_server_state(), port) - return (status, backend_registry.recorded_int(fresh, "pid")) + return (_probe_port(port), backend_registry.recorded_int(fresh, "pid")) def _port_is_foreign_held(port: int) -> bool: diff --git a/tests/fakes.py b/tests/fakes.py index 237c33e..d7811a7 100644 --- a/tests/fakes.py +++ b/tests/fakes.py @@ -145,6 +145,26 @@ def pretend_display_context(monkeypatch: Any, token: str) -> None: monkeypatch.setattr(display_context, "display_context", lambda: token) +def v2_record(**backends: dict) -> dict: + """A schema-v2 ``server.json`` record from ``context=entry`` kwargs, where + ``_`` in a kwarg name reads as ``-`` (``win_session_1`` → ``win-session-1``, + the token `display_context()` actually produces). + + Here rather than in a test module because both + `test_cli_status_wedged.py` and `test_probe_backend_status.py` build the + same record shape (F-868), and a record builder that disagreed with itself + between two files is exactly the drift this module exists to prevent. It + writes the schema literally, deliberately: `backend_registry.record_backend` + is the code under test in several of those cases, so a fixture that went + through it could not express a record that function would never write — a + hand-edited one, or a pre-supersede pair. + """ + return { + "schema": 2, + "backends": {ctx.replace("_", "-"): entry for ctx, entry in backends.items()}, + } + + # --------------------------------------------------------------------------- # Fake DOM tab — covers BOTH cloner seams (JS-eval + CDP) # --------------------------------------------------------------------------- diff --git a/tests/test_cli_status_wedged.py b/tests/test_cli_status_wedged.py index 31086b1..3293d5d 100644 --- a/tests/test_cli_status_wedged.py +++ b/tests/test_cli_status_wedged.py @@ -20,12 +20,9 @@ import pytest +from fakes import pretend_display_context, v2_record from stealth_chrome_devtools_mcp import cli -from stealth_chrome_devtools_mcp.embedded import ( - desktop_launch, - display_context, - singleton, -) +from stealth_chrome_devtools_mcp.embedded import desktop_launch, singleton @pytest.fixture(autouse=True) @@ -318,14 +315,6 @@ def _record(state: dict | None) -> None: return _record -def _v2(**backends) -> dict: - """A schema-v2 record from `context=entry` kwargs (`_` reads as `-`).""" - return { - "schema": 2, - "backends": {ctx.replace("_", "-"): entry for ctx, entry in backends.items()}, - } - - @contextmanager def _doctor_probes(fake_server, *, healthy=True, ready=True): """Run doctor with every live probe stubbed, so the ONLY input that varies @@ -391,7 +380,7 @@ def test_every_recorded_backend_is_listed_with_its_context( self, fake_server, recorded_backends, capsys ): recorded_backends( - _v2( + v2_record( win_session_1={"port": 55296, "pid": 4242, "version": "2.0.4"}, headless={"port": 19222, "pid": 909, "version": "2.0.4"}, ) @@ -411,7 +400,7 @@ def test_window_capable_backends_are_listed_first( """Recorded headless-first, printed capable-first: the order is `backend_registry.window_capable_first`'s, not the file's.""" recorded_backends( - _v2( + v2_record( headless={"port": 19222, "pid": 909, "version": "2.0.4"}, win_session_1={"port": 55296, "pid": 4242, "version": "2.0.4"}, ) @@ -428,7 +417,7 @@ def test_each_backend_is_probed_on_its_own_port( """Three recorded backends, three different liveness answers — a once-per-record probe would report one verdict for all of them.""" recorded_backends( - _v2( + v2_record( win_session_1={"port": 55296, "pid": 4242, "version": "2.0.4"}, win_session_2={"port": 55297, "pid": 4243, "version": "2.0.4"}, headless={"port": 19222, "pid": 909, "version": "2.0.4"}, @@ -452,7 +441,9 @@ def test_remedy_line_when_no_backend_can_show_a_window( """The actionable half: a headless-only machine with nobody logged on must be told what to do, in the same words `spawn_browser`'s refusal uses — that refusal is what sends the operator here.""" - recorded_backends(_v2(headless={"port": 19222, "pid": 909, "version": "2.0.4"})) + recorded_backends( + v2_record(headless={"port": 19222, "pid": 909, "version": "2.0.4"}) + ) with _doctor_probes(fake_server): cli._cmd_doctor(None) out = capsys.readouterr().out @@ -471,7 +462,9 @@ def test_the_remedy_does_not_claim_failure_when_delegation_will_deliver( would be doctor lying about the operator's own machine — the advice degrades from a fix to an optimisation, and must read that way.""" monkeypatch.setattr(desktop_launch, "available", lambda: True) - recorded_backends(_v2(headless={"port": 19222, "pid": 909, "version": "2.0.4"})) + recorded_backends( + v2_record(headless={"port": 19222, "pid": 909, "version": "2.0.4"}) + ) with _doctor_probes(fake_server): cli._cmd_doctor(None) out = capsys.readouterr().out @@ -488,7 +481,7 @@ def test_no_remedy_line_when_a_window_capable_backend_is_responsive( self, fake_server, recorded_backends, capsys ): recorded_backends( - _v2( + v2_record( headless={"port": 19222, "pid": 909, "version": "2.0.4"}, win_session_1={"port": 55296, "pid": 4242, "version": "2.0.4"}, ) @@ -514,7 +507,7 @@ def test_a_dead_window_capable_backend_does_not_suppress_the_remedy( operator's headed spawn is still refused, because discovery finds no LIVE capable backend to adopt.""" recorded_backends( - _v2(win_session_1={"port": 55296, "pid": 4242, "version": "2.0.4"}) + v2_record(win_session_1={"port": 55296, "pid": 4242, "version": "2.0.4"}) ) with _doctor_probes(fake_server, healthy=False): cli._cmd_doctor(None) @@ -530,7 +523,7 @@ def test_a_wedged_window_capable_backend_does_not_suppress_the_remedy( """Same ruling's other half: a wedged backend holds its socket open but cannot serve a spawn either.""" recorded_backends( - _v2(win_session_1={"port": 55296, "pid": 4242, "version": "2.0.4"}) + v2_record(win_session_1={"port": 55296, "pid": 4242, "version": "2.0.4"}) ) with _doctor_probes(fake_server, healthy=True, ready=False): cli._cmd_doctor(None) @@ -546,7 +539,7 @@ def test_an_entry_with_no_usable_port_says_so( is nothing to probe, and claiming "down" would assert a liveness we never tested.""" recorded_backends( - _v2(win_session_1={"port": "55296", "pid": 4242, "version": "2.0.4"}) + v2_record(win_session_1={"port": "55296", "pid": 4242, "version": "2.0.4"}) ) with _doctor_probes(fake_server): cli._cmd_doctor(None) @@ -615,13 +608,13 @@ def test_status_reports_the_own_context_live_backend_not_a_dead_sibling( self, fake_server, recorded_backends, capsys, monkeypatch, tmp_path ): recorded_backends( - _v2( + v2_record( win_session_2={"port": 7169, "pid": 89892, "version": "2.1.1"}, headless={"port": 19222, "pid": 67720, "version": "2.1.3"}, win_session_1={"port": 52554, "pid": 53836, "version": "2.1.5"}, ) ) - monkeypatch.setattr(display_context, "display_context", lambda: "win-session-1") + pretend_display_context(monkeypatch, "win-session-1") with ( patch.object(cli, "_clone_storage", return_value=fake_server), patch( @@ -648,12 +641,12 @@ def test_status_reports_the_live_backend_for_a_client_that_adopts_anything( adopts any recorded backend, capable-first — so the dead window-capable entry it tries first must not end the search.""" recorded_backends( - _v2( + v2_record( win_session_2={"port": 7169, "pid": 89892, "version": "2.1.1"}, win_session_1={"port": 52554, "pid": 53836, "version": "2.1.5"}, ) ) - monkeypatch.setattr(display_context, "display_context", lambda: "headless") + pretend_display_context(monkeypatch, "headless") with ( patch.object(cli, "_clone_storage", return_value=fake_server), patch( @@ -674,13 +667,13 @@ def test_status_names_the_records_it_is_not_speaking_about( """One summary line over a three-entry record silently drops two of them. It must say they exist and where to read them.""" recorded_backends( - _v2( + v2_record( win_session_2={"port": 7169, "pid": 89892, "version": "2.1.1"}, headless={"port": 19222, "pid": 67720, "version": "2.1.3"}, win_session_1={"port": 52554, "pid": 53836, "version": "2.1.5"}, ) ) - monkeypatch.setattr(display_context, "display_context", lambda: "win-session-1") + pretend_display_context(monkeypatch, "win-session-1") with ( patch.object(cli, "_clone_storage", return_value=fake_server), patch( @@ -692,7 +685,7 @@ def test_status_names_the_records_it_is_not_speaking_about( cli._cmd_status(None) out = capsys.readouterr().out - assert "other records" in out + assert "others : 2 backends recorded" in out assert "win-session-2" in out assert "headless" in out assert "doctor" in out @@ -703,9 +696,9 @@ def test_a_single_recorded_backend_gets_no_other_records_line( """The note is evidence, not decoration: with nothing else recorded there is nothing it could honestly say.""" recorded_backends( - _v2(win_session_1={"port": 52554, "pid": 53836, "version": "2.1.5"}) + v2_record(win_session_1={"port": 52554, "pid": 53836, "version": "2.1.5"}) ) - monkeypatch.setattr(display_context, "display_context", lambda: "win-session-1") + pretend_display_context(monkeypatch, "win-session-1") with ( patch.object(cli, "_clone_storage", return_value=fake_server), patch( @@ -717,4 +710,4 @@ def test_a_single_recorded_backend_gets_no_other_records_line( cli._cmd_status(None) out = capsys.readouterr().out - assert "other records" not in out + assert "others :" not in out diff --git a/tests/test_probe_backend_status.py b/tests/test_probe_backend_status.py index 67923fb..07b8a30 100644 --- a/tests/test_probe_backend_status.py +++ b/tests/test_probe_backend_status.py @@ -14,6 +14,7 @@ import pytest +from fakes import pretend_display_context, v2_record from stealth_chrome_devtools_mcp.embedded import singleton @@ -134,14 +135,6 @@ def test_responsive_backend_reports_responsive( assert reported_port == responsive_stub -def _v2(**backends) -> dict: - """A schema-v2 record from `context=entry` kwargs (`_` reads as `-`).""" - return { - "schema": 2, - "backends": {ctx.replace("_", "-"): entry for ctx, entry in backends.items()}, - } - - class TestProbeWalksAdoptionOrder: """F-868: the reporter must speak about the backend THIS process would be served by — `backend_registry.adoption_candidates`' order, the same one @@ -159,7 +152,7 @@ def test_a_dead_first_entry_does_not_hide_the_live_own_context_one( dead = _free_closed_port() (isolated_state / "server.json").write_text( json.dumps( - _v2( + v2_record( win_session_2={"port": dead, "version": "2.1.1", "pid": 89892}, win_session_1={ "port": responsive_stub, @@ -169,9 +162,7 @@ def test_a_dead_first_entry_does_not_hide_the_live_own_context_one( ) ) ) - monkeypatch.setattr( - singleton.display_context, "display_context", lambda: "win-session-1" - ) + pretend_display_context(monkeypatch, "win-session-1") assert singleton._probe_backend_status() == ("responsive", responsive_stub) @@ -183,7 +174,7 @@ def test_a_dead_capable_entry_does_not_end_an_unproven_clients_search( dead = _free_closed_port() (isolated_state / "server.json").write_text( json.dumps( - _v2( + v2_record( win_session_2={"port": dead, "version": "2.1.1", "pid": 89892}, headless={ "port": responsive_stub, @@ -193,9 +184,7 @@ def test_a_dead_capable_entry_does_not_end_an_unproven_clients_search( ) ) ) - monkeypatch.setattr( - singleton.display_context, "display_context", lambda: "headless" - ) + pretend_display_context(monkeypatch, "headless") assert singleton._probe_backend_status() == ("responsive", responsive_stub) @@ -208,14 +197,12 @@ def test_a_wedged_candidate_outranks_a_dead_one( dead = _free_closed_port() (isolated_state / "server.json").write_text( json.dumps( - _v2( + v2_record( win_session_2={"port": dead, "version": "2.1.1", "pid": 89892}, headless={"port": wedged_stub, "version": "2.1.5", "pid": 67720}, ) ) ) - monkeypatch.setattr( - singleton.display_context, "display_context", lambda: "unverified" - ) + pretend_display_context(monkeypatch, "unverified") assert singleton._probe_backend_status() == ("wedged", wedged_stub) diff --git a/tests/test_singleton_stop_restart.py b/tests/test_singleton_stop_restart.py index 369ad2d..c57ddbc 100644 --- a/tests/test_singleton_stop_restart.py +++ b/tests/test_singleton_stop_restart.py @@ -31,6 +31,7 @@ import psutil import pytest +from fakes import pretend_display_context from stealth_chrome_devtools_mcp.embedded import backend_registry, singleton @@ -384,6 +385,9 @@ def test_none_reports_not_running(self, isolated_state, monkeypatch): assert pid is None def test_lock_contended_reports_busy(self, isolated_state, monkeypatch): + # A STOP test: stop_backend still reads _probe_backend_status (which + # record to act on), unlike restart, which since F-868 reports + # _probe_port for the port it spawned on. monkeypatch.setattr( singleton, "_probe_backend_status", lambda: ("responsive", 19222) ) @@ -436,10 +440,14 @@ class TestRestartBackend: the outage is survived, same as cold start - see TestRestartPortSelection below for that behavior's own pinning tests. - The final state reported is _probe_backend_status()'s, read AFTER the - lock releases (M1's one liveness vocabulary - binding ruling: no new - health check anywhere) - a restart that comes back wedged/down must be - visible, not assumed "responsive". Pinned by the third test below. + The final state reported is _probe_port(port)'s for THE PORT WE SPAWNED ON, + read AFTER the lock releases (M1's one liveness vocabulary - binding + ruling: no new health check anywhere) - a restart that comes back + wedged/down must be visible, not assumed "responsive". Pinned by the third + test below. It was _probe_backend_status() until F-868: that walks the + ADOPTION order, so on a multi-context record a responsive sibling answered + for a restart whose own backend came up wedged - pinned by the last test in + TestRestartReportsTheSpawnedPort. """ def test_terminate_then_spawn_ordering_under_the_lock( @@ -475,9 +483,7 @@ def _fake_spawn(port): monkeypatch.setattr( singleton, "_wait_for_server", lambda port: calls.append("wait") ) - monkeypatch.setattr( - singleton, "_probe_backend_status", lambda: ("responsive", 19222) - ) + monkeypatch.setattr(singleton, "_probe_port", lambda port: "responsive") result = singleton.restart_backend() @@ -531,9 +537,7 @@ def _fake_spawn(port): monkeypatch.setattr(singleton, "_wait_for_server", lambda port: None) # The backend comes back wedged - restart_backend must report that, # not the "responsive" the ordering test above pinned. - monkeypatch.setattr( - singleton, "_probe_backend_status", lambda: ("wedged", 19222) - ) + monkeypatch.setattr(singleton, "_probe_port", lambda port: "wedged") result = singleton.restart_backend() @@ -573,9 +577,7 @@ def test_recorded_port_is_rebound_when_still_free( ) monkeypatch.setattr(singleton, "_terminate_backend", lambda port: None) monkeypatch.setattr(singleton, "_wait_for_server", lambda port: None) - monkeypatch.setattr( - singleton, "_probe_backend_status", lambda: ("responsive", recorded_port) - ) + monkeypatch.setattr(singleton, "_probe_port", lambda port: "responsive") spawned_on = {} @@ -616,9 +618,7 @@ def test_recorded_port_now_foreign_held_falls_back( # touched by this test. monkeypatch.setattr(singleton, "_terminate_backend", lambda port: None) monkeypatch.setattr(singleton, "_wait_for_server", lambda port: None) - monkeypatch.setattr( - singleton, "_probe_backend_status", lambda: ("responsive", 0) - ) + monkeypatch.setattr(singleton, "_probe_port", lambda port: "responsive") spawned_on = {} @@ -660,9 +660,7 @@ def test_normal_case_return_shape_unchanged_vs_m8_5( ) monkeypatch.setattr(singleton, "_terminate_backend", lambda port: None) monkeypatch.setattr(singleton, "_wait_for_server", lambda port: None) - monkeypatch.setattr( - singleton, "_probe_backend_status", lambda: ("responsive", recorded_port) - ) + monkeypatch.setattr(singleton, "_probe_port", lambda port: "responsive") def _fake_spawn(port): singleton._write_server_state( @@ -730,9 +728,7 @@ def test_restart_terminates_our_own_context_and_leaves_the_sibling_alone( singleton, "_terminate_backend", lambda port: terminated.append(port) ) monkeypatch.setattr(singleton, "_wait_for_server", lambda port: None) - monkeypatch.setattr( - singleton, "_probe_backend_status", lambda: ("responsive", our_port) - ) + monkeypatch.setattr(singleton, "_probe_port", lambda port: "responsive") spawned: list[int] = [] @@ -801,9 +797,7 @@ def test_restart_with_no_entry_of_our_own_diverts_instead_of_killing_a_sibling( singleton, "_terminate_backend", lambda port: terminated.append(port) ) monkeypatch.setattr(singleton, "_wait_for_server", lambda port: None) - monkeypatch.setattr( - singleton, "_probe_backend_status", lambda: ("responsive", diverted_port) - ) + monkeypatch.setattr(singleton, "_probe_port", lambda port: "responsive") spawned: list[int] = [] @@ -824,3 +818,106 @@ def _fake_spawn(port): for e in backend_registry.read_backends(record) } assert surviving["headless"] == sibling_port + + +class TestStopIsPerDisplayContext: + """F-868's consequence for `stop`, stated as a contract rather than left to + be discovered. + + `stop` acts on whatever `_probe_backend_status` reports, and that now walks + `adoption_candidates`, which for a PROVEN-capable client excludes every + FOREIGN proven context. So an operator's `stop` no longer reaches across + into another desktop's backend — it says "not running", because for this + shell there is nothing it may act on, and it leaves the sibling's record + alone. Previously `first_backend` handed it that entry: it terminated + nothing (the port is not ours to hold) and then FORGOT the record, quietly + making a possibly-live sibling undiscoverable. + + This is green on the branch by construction — the adoption walk is the fix, + not something added for this test. It is pinned because it is a BEHAVIOUR + CHANGE riding on a bug fix, and an unpinned one is the kind a later + "simplification" reverts without noticing. + """ + + def test_stop_will_not_act_on_a_foreign_proven_context( + self, isolated_state, monkeypatch + ): + backend_registry.record_backend( + singleton.SERVER_STATE_FILE, + port=6000, + version="1.2.1", + pid=2222, + source_fingerprint="fp", + display_context="win-session-OTHER", + ) + pretend_display_context(monkeypatch, "win-session-OURS") + terminated: list[int] = [] + monkeypatch.setattr( + singleton, "_terminate_backend", lambda port: terminated.append(port) + ) + + # "none" short-circuits before the lock, so nothing is probed either: + # a foreign desktop's backend is not even asked whether it is alive. + assert singleton.stop_backend() == ("not running", None) + assert terminated == [] + assert [ + (e["display_context"], e["port"]) + for e in backend_registry.read_backends(singleton.SERVER_STATE_FILE) + ] == [("win-session-OTHER", 6000)] + + +class TestRestartReportsTheSpawnedPort: + """F-868: restart's status must be about the backend restart just brought + up, never about a sibling that happens to answer. + + `_probe_backend_status` asks "is there a backend for me" — the adoption + walk — which is the right question for `status` and the wrong one here: a + RESPONSIVE sibling would report "responsive" for a restart whose own + backend came up wedged, i.e. a promise about a different process than the + `pid` beside it. `restart_backend` now reports `_probe_port(port)` for the + port it selected, so both halves of its return describe one backend. + """ + + def test_a_responsive_sibling_does_not_answer_for_a_wedged_fresh_backend( + self, isolated_state, monkeypatch + ): + sibling_port = _free_closed_port() + our_port = _free_closed_port() + backend_registry.record_backend( + singleton.SERVER_STATE_FILE, + port=sibling_port, + version="1.2.1", + pid=1111, + source_fingerprint="fp", + display_context="win-session-1", + ) + # HEADLESS adopts anything, window-capable first — so the sibling is + # the adoption walk's answer, which is the whole point of the case. + pretend_display_context(monkeypatch, "headless") + monkeypatch.setattr( + singleton, "_exclusive_lock", lambda: _tracking_lock([], True) + ) + monkeypatch.setattr(singleton, "_select_backend_port", lambda port: our_port) + monkeypatch.setattr(singleton, "_terminate_backend", lambda port: None) + monkeypatch.setattr(singleton, "_wait_for_server", lambda port: None) + + def _fake_spawn(port): + singleton._write_server_state( + port=port, version="1.2.1", pid=4242, source_fingerprint="" + ) + + monkeypatch.setattr(singleton, "_start_server_process", _fake_spawn) + # The real `_probe_port` runs; only its two primitives are stated. The + # sibling answers, ours holds its socket open and says nothing. + monkeypatch.setattr(singleton, "_server_is_healthy", lambda port: True) + monkeypatch.setattr( + singleton, "_backend_http_ready", lambda port, **_kw: port == sibling_port + ) + + result = singleton.restart_backend() + + assert result == ("wedged", 4242) + # Both halves of the contrast, so this cannot pass by the sibling + # simply being invisible: the adoption walk DOES say responsive here, + # and restart deliberately does not use it. + assert singleton._probe_backend_status() == ("responsive", sibling_port) From df0152f87c0110ac801db9aa3a6848351cd1ece3 Mon Sep 17 00:00:00 2001 From: AminDhouib Date: Tue, 15 Sep 2026 00:23:14 -0400 Subject: [PATCH 3/3] refactor(F-868): move the liveness ladder and the adoption walk to a leaf Pure move, no behaviour change. singleton.py came out of the F-868 fix at 999 of its 1000-LOC default - a gate that passes and a file nobody can edit. embedded/backend_liveness.py is now THE one home for "is the backend on this port alive, and which recorded backend would THIS client be served by": probe_port (the socket -> initialize -> down/wedged/responsive ladder) and probe_recorded (the adoption-order walk). Both docstrings moved verbatim. A leaf on backend_watchdog's proven pattern: the two liveness primitives arrive as ARGUMENTS (is_healthy / http_ready) and the record arrives as a PATH, so the module never imports singleton and never decides WHICH record either - the adoption ORDER stays backend_registry's policy, merely consumed here. Importing the leaf alone does not pull singleton into sys.modules. singleton keeps thin _probe_port / _probe_backend_status wrappers that bind OUR probes, OUR record path and OUR display context. That is the point, not a hop to delete: the suite patches singleton._server_is_healthy, singleton._backend_http_ready and singleton._probe_port by name, and a wrapper resolving those module globals at CALL time is what keeps every existing monkeypatch.setattr(singleton, ...) reaching this code. probe_recorded takes the per-port probe as a parameter for the same reason, so the walk asks singleton._probe_port rather than its own module-level function. A direct import would bind at import time and silently stop seeing such a patch. singleton.py 999 -> 985; the leaf is 91 lines; no cap ratcheted up. Also folds in the review nit: _cmd_restart's trailing comment still described a "none" verdict, which _probe_port cannot return - that word belonged to the record-wide walk restart stopped using. The branch is unchanged (it is the "down" arm); only the comment is corrected. --- CLAUDE.md | 3 +- ...g_F868_cli_status_reports_a_dead_record.md | 54 +++++------ src/stealth_chrome_devtools_mcp/cli.py | 11 ++- .../embedded/backend_liveness.py | 91 +++++++++++++++++++ .../embedded/singleton.py | 56 +++++------- 5 files changed, 149 insertions(+), 66 deletions(-) create mode 100644 src/stealth_chrome_devtools_mcp/embedded/backend_liveness.py diff --git a/CLAUDE.md b/CLAUDE.md index acfc71b..5c1f45d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -57,9 +57,10 @@ Package root: `src/stealth_chrome_devtools_mcp/`. Two console scripts (`pyprojec | File | Owns | |---|---| | `server.py` | the real MCP server — **ZERO tool bodies**; all 94 live in `tool_sections/` (plan_SERVERSPLIT, complete at slice 12). What this file owns, and nothing else: the per-execution FastMCP app (`mcp`/`registry`), `app_lifespan`, the four `@mcp.resource` handlers, the xpool-safe gate, `build_arg_parser`, the `__main__` block, and the **binding loop** that drives registration from THIS file's module body, once per execution of it (so the canonical import, the bare-name spec load and the runpy `__main__` load each get a full 94-tool app — a section module that decorated itself would register into the first execution only). **No migration alias block**: every singleton, knob and guard this file drives is read as `rt.` against `tool_runtime`, the one patchable home, exactly as a tool body does. The lone import left is `clone_storage`, which this file never touches — it is the positive delegation handle `tests/test_clone_storage.py`'s F-201 negative-surface pin needs. At 523 LOC it is governed by the 1000-LOC default; its `GRANDFATHER` row is deleted, not merely satisfied | -| `singleton.py` | **backend lifecycle + the stdio proxy** — liveness probes (`_backend_http_ready`; **`_probe_port(port)`, THE one home for the socket→`initialize`→`down`/`wedged`/`responsive` ladder** (F-868) with three callers — the candidate walk, `restart_backend`, and `cli._probe_recorded_backend`, which adds only the one word the ladder cannot reach, `"no port recorded"`; never copy those four lines again; and `_probe_backend_status`, **THE one answer to "what state is the backend THIS process would be served by in"** — it walks `backend_registry.adoption_candidates` in the same order `_find_running_server` does and reports the first candidate that answers, wedged over down when none does, never `first_backend`'s "whichever entry the record lists first"; `status`, `doctor`, `stop` and `kill-orphans`'s live-backend guard all consume this ONE function, so the record selection is fixed in exactly one place. `restart_backend` deliberately does NOT: it reports `_probe_port` for the port it spawned on, because the adoption walk answers "is there a backend for me" and a responsive SIBLING must never speak for a restart's own fresh backend). **At 999/1000 LOC this file has one line of headroom** — the next change to it needs the `backend_liveness` extraction (`_probe_port` + the walk, probes handed in as arguments the way `backend_watchdog` already does), not more prose-trimming), port selection (`_select_backend_port`, `DEFAULT_PORT`), the one identity+readiness reuse gate (`_same_identity_backend_ready`, `_source_fingerprint`, `REUSE_PATIENCE_SECONDS` — which spends its patience through `scheduling_lag.FairWindow`, F-856), cold-start lock (`_start_backend_holding_lock`), `run_stdio_proxy`. The `server.json` record moved to `backend_registry.py` (path names re-exported here for legacy callers); the proxy's *recovery from a dead backend* moved to `proxy_selfheal.py`; the watchdog LOOP moved to `backend_watchdog.py` — `_watch_backend_liveness` stays here as the wiring that knows which probes are ours; and **the spawn itself moved to `backend_launch.py` (F-867)** — `_start_server_process` still owns WHAT to start (`_server_process_cmd`, `_backend_interpreter`'s F-866 base-interpreter choice, the child env, F-830's boot-log roll) and what to RECORD afterwards (`PORT_FILE`, `_write_server_state`), and makes exactly ONE call, `backend_launch.spawn(cmd, child_env, boot_log) -> Launched(pid, rung)`; it no longer imports `subprocess` | +| `singleton.py` | **backend lifecycle + the stdio proxy** — the liveness PRIMITIVES (`_backend_http_ready`, `_server_is_healthy`) and the two thin bindings that hand them to `backend_liveness` (`_probe_port`, `_probe_backend_status` — see that row for the policy; they are wrappers on purpose, because the suite patches these names and a wrapper resolves them at CALL time). `status`, `doctor`, `stop` and `kill-orphans`'s live-backend guard all consume `_probe_backend_status`, so record selection is fixed in one place; `restart_backend` deliberately does NOT — it reports `_probe_port` for the port it spawned on, because the adoption walk answers "is there a backend for me" and a responsive SIBLING must never speak for a restart's own fresh backend. Port selection (`_select_backend_port`, `DEFAULT_PORT`), the one identity+readiness reuse gate (`_same_identity_backend_ready`, `_source_fingerprint`, `REUSE_PATIENCE_SECONDS` — which spends its patience through `scheduling_lag.FairWindow`, F-856), cold-start lock (`_start_backend_holding_lock`), `run_stdio_proxy`. The `server.json` record moved to `backend_registry.py` (path names re-exported here for legacy callers); the proxy's *recovery from a dead backend* moved to `proxy_selfheal.py`; the watchdog LOOP moved to `backend_watchdog.py` — `_watch_backend_liveness` stays here as the wiring that knows which probes are ours; and **the spawn itself moved to `backend_launch.py` (F-867)** — `_start_server_process` still owns WHAT to start (`_server_process_cmd`, `_backend_interpreter`'s F-866 base-interpreter choice, the child env, F-830's boot-log roll) and what to RECORD afterwards (`PORT_FILE`, `_write_server_state`), and makes exactly ONE call, `backend_launch.spawn(cmd, child_env, boot_log) -> Launched(pid, rung)`; it no longer imports `subprocess` | | `backend_launch.py` | **THE one home for "spawn the backend where no MCP client's Job Object can reach it"** (F-867) — `spawn`, `Launched(pid, rung)` and nothing else in the tree may create a backend. POSIX is one rung (`posix`, `start_new_session=True`, unchanged). Windows climbs three, and the rung that served is logged through the ONE `_log_rung` line on `stealth.proxy` so a post-mortem can read it: (1) `breakaway` — `CREATE_BREAKAWAY_FROM_JOB`, accepted ONLY when `_proven_in_a_job` says the new process is in NO job, because under a nested job chain the flag leaves the innermost job only and a "successful" breakaway can still sit in the client's; `ERROR_ACCESS_DENIED` (what the SDK's job returns) drops a rung while any other `OSError` propagates. A PARTIAL escape is never discarded blindly — rung 2's viability (`_scheduler_plan`) is decided FIRST, and when there is no scheduler rung the child is KEPT as the `breakaway-partial` rung at WARNING, because out of one job beats out of none; conversely, breakaway once proven PERMITTED is re-asked on rung 3. (2) `scheduler` — a one-shot Task Scheduler task through `desktop_launch._schtasks` (the F-810 seam, not a second home) running `pythonw.exe` (no console, so no window can flash) on the BASE interpreter, never a venv's `Scripts\pythonw.exe`, which is F-866's redirector and would rebuild the kill-on-close job; the stdlib-only `_LAUNCHER_SCRIPT` reads argv + the ENTIRE child env + the boot-log path from a JSON spec in `~/.stealth-mcp/backend-launch/` (the command line carries two paths only because **`schtasks` stores at most 253 chars of `/TR` and truncates beyond that with exit 0** — measured, not F-810's documented "~261"; the silent truncation costs a Last-Result-2 task, the whole 20 s pid deadline and a drop to `plain`, which is why the per-attempt token is 12 hex chars, not 32), re-opens `backend-boot.log` itself and gives the backend stdout AND stderr (F-303 survives the hand-off), and returns the SERVING pid through an `os.replace`d pid file. **Gated on `_same_session_as_console`**: taken only when the spawner is already in the logged-on console session, so the backend lands where it would have anyway and the display context `singleton` records stays true (F-808 — the tool still never PICKS a session). Task and scratch files deleted in a `finally`, and the orphan sweep that runs before a scheduler spawn is a FILESYSTEM sweep, not a `/Query`: any `.json` spec (token = 12 hex) older than 2× the pid deadline gets its `stealth-mcp-backend-` task deleted BY NAME and its files removed. Deliberately not "delete tasks whose spec is absent" — `_cleanup` deletes the task first, so a killed spawner leaves task AND spec, which is the only orphan class there is and the one that predicate missed. A live sibling's spec is younger and untouched; a clean dir costs zero `schtasks` calls. (3) `plain` — today's `DETACHED_PROCESS \| CREATE_NEW_PROCESS_GROUP` (plus the breakaway bit when rung 1 proved it permitted), for a runner with no console session; its line names F-867 and says the backend is inside this client's job. A leaf: `backend_registry` for the state dir, `desktop_launch` lazily for the ONE `schtasks` seam, the ONE pid-file reader and the ONE task teardown, so none of them gets a second home. Never raises for a rung's own failure — only a genuine `Popen` error reaches the caller | | `backend_watchdog.py` | **THE one home for the proxy's mid-session liveness watchdog** — the SLOW witness (`watch_liveness`): F-820's strikes plus the confirmation phase. A leaf: both probes arrive as arguments, so it never imports `singleton` and the dead-vs-busy policy stays single-homed in the reuse gate | +| `backend_liveness.py` | **THE one home for "is the backend on this port alive, and which recorded backend would THIS client be served by"** (F-868) — `probe_port`, the socket→`initialize`→`down`/`wedged`/`responsive` ladder (never copy those four lines again; `cli._probe_recorded_backend` is an adapter that adds only the one word the ladder cannot reach, `"no port recorded"`), and `probe_recorded`, the ADOPTION-order walk that reports the first candidate which answers — wedged over down when none does, `("none", None)` when no adoptable entry names a port at all — never `first_backend`'s "whichever entry the record lists first". A leaf on `backend_watchdog`'s pattern: the two primitives arrive as ARGUMENTS (`is_healthy`/`http_ready`) and the record as a PATH, so it never imports `singleton` and the order itself stays `backend_registry`'s policy. `probe_recorded` takes the per-port probe as an argument too, so `singleton._probe_port` — the name the suite patches — is what the walk actually asks | | `scheduling_lag.py` | **THE one home for "was this process scheduled fairly, and what does a time budget owe it when it was not"** (F-856) — `FairWindow`, whose budget is charged in fair seconds (elapsed ÷ the lag its own naps measured), plus `MAX_STRETCH`, `REPORT_FACTOR` and the `proxy: patience extended under starvation` lifecycle report. It never decides alive-or-dead: only "has this window been spent", so `proxy_selfheal`'s ONE heal path is untouched. A leaf; the `_now`/`_wait` module functions are its single timing seam | | `session_hygiene.py` | **THE one home for "this MCP session was abandoned by its client — reap it"** (F-862) — `HygienicSessionManager`, the MCP streamable-HTTP session manager plus a sweep: a session with NO standing GET event stream (a live proxy always holds one) and no request for `ABANDONED_AFTER_SECONDS` is terminated through the transport's own `terminate()`, so a liveness probe whose DELETE was lost or a proxy that died no longer costs the backend ~0.11 MB forever. `install()` binds the class to the name FastMCP constructs by module attribute — the one seam, called from `server.py`'s http branch as `rt.session_hygiene.install()` BEFORE `mcp.run()`. A leaf: imports no other embedded module | | `serve_startup.py` | **THE one home for "startup work that must not delay the backend's first serve"** (F-856) — `after_serving`, which runs one idempotent, best-effort startup job on a daemon thread. Its docstring carries the safety argument for reaping orphans CONCURRENTLY with serving. Deliberately not a general background-task runner: `clone_storage.spawn_background_sweep` keeps its own asyncio task, dedupe and trigger-time root capture | diff --git a/audit/stage2/finding_F868_cli_status_reports_a_dead_record.md b/audit/stage2/finding_F868_cli_status_reports_a_dead_record.md index 05007d5..7d8be32 100644 --- a/audit/stage2/finding_F868_cli_status_reports_a_dead_record.md +++ b/audit/stage2/finding_F868_cli_status_reports_a_dead_record.md @@ -123,14 +123,16 @@ lifetime. **Deliberately NOT changed here — see §6.** ## 4. The fix -**The liveness ladder now has one home.** `singleton._probe_port(port)` — socket, then a -real MCP `initialize`, returning `down` / `wedged` / `responsive` — was four lines -duplicated in `cli._probe_recorded_backend`, justified there by a note saying -`_probe_backend_status` "reads the FIRST recorded backend" and so could not answer -per-entry. That justification dies with this fix, and a duplicated liveness ladder was a -second way to answer one question regardless. It now has three callers: the candidate -walk, `restart_backend`, and the CLI form, which keeps only the one word the ladder -cannot reach ("no port recorded"). +**The liveness ladder now has one home.** The socket → `initialize` → `down` / `wedged` / +`responsive` ladder was four lines duplicated in `cli._probe_recorded_backend`, justified +there by a note saying `_probe_backend_status` "reads the FIRST recorded backend" and so +could not answer per-entry. That justification dies with this fix, and a duplicated +liveness ladder was a second way to answer one question regardless. It now has three +callers: the candidate walk, `restart_backend`, and the CLI form, which keeps only the one +word the ladder cannot reach ("no port recorded"). It lives in +`embedded/backend_liveness.py` as `probe_port`, beside the adoption walk +(`probe_recorded`), reached through `singleton._probe_port` / `_probe_backend_status` — +see §6 for why those wrappers exist and why the leaf takes its probes as arguments. **One home, no second way.** `_probe_backend_status` now walks `backend_registry.adoption_candidates(SERVER_STATE_FILE, display_context.display_context())` @@ -182,8 +184,8 @@ whose `port` is a string reads as `None` and would have matched a `None` reporte hiding itself in precisely the "nothing is running" case that needs it most. No new `STEALTH_MCP_*` knob, no new env read, no `typing.Any`, no LOC-budget change: -`cli.py` 645 → 691, and `singleton.py` 979 → **999 of its 1000 default** — see §6, this -is now the binding constraint on the file. +`cli.py` 645 → 691, `singleton.py` 979 → 999 → **985** once the `backend_liveness` +extraction landed (§6), and the new leaf is 91. ## 5. Verification @@ -265,20 +267,18 @@ is now the binding constraint on the file. whose socket is open but silent (one `LIVENESS_PROBE_TIMEOUT`, 2 s), and the walk stops at the first responsive one. Not measured on a pathological record — `status` is an interactive verb with no deadline. -- **`singleton.py` is at 999 of its 1000-LOC default — one line of headroom. This is the - most fragile thing in this change and it needs a decision, not a note.** The gate passes - and nothing here is over budget, but the next contributor to that file has nowhere to - put a line. Two of the +20 were paid for honestly (the F-856 paragraph in - `_same_identity_backend_ready` was retelling what `scheduling_lag.FairWindow` is THE one - home for, and now points at it instead); the rest is the F-868 reasoning, which belongs - with the code it justifies. - - The right next change is an extraction, and it should be its own PR rather than more - prose-trimming, which is just padding a cap from the other side. The shape is already - proven in this tree: `backend_watchdog.py` takes **both probes as arguments** so it never - imports `singleton` and the dead-vs-busy policy stays single-homed. A `backend_liveness` - leaf holding `_probe_port` plus the adoption walk, with `_server_is_healthy` / - `_backend_http_ready` handed in, would move ~35 lines out and leave thin wrappers on - `singleton` so every existing `monkeypatch.setattr(singleton, …)` in the suite keeps - working. I did NOT do it here: it lands mid-review-cycle, touches patch surfaces across - a dozen test modules, and would bury a four-line truthfulness fix under a refactor. +- **`singleton.py`'s LOC: DONE, in its own commit.** The fix left that file at 999 of its + 1000-LOC default — one line of headroom, which is a gate that passes and a file nobody + can edit. On review this was escalated and the extraction was done as a SEPARATE commit, + so "the fix" and "the move" read independently: `embedded/backend_liveness.py` is now THE + one home for the ladder (`probe_port`) and the adoption walk (`probe_recorded`), on + `backend_watchdog`'s proven leaf pattern — the two primitives arrive as ARGUMENTS and the + record as a PATH, so it never imports `singleton`. `singleton` keeps thin + `_probe_port` / `_probe_backend_status` wrappers that bind OUR probes, OUR record path + and OUR display context, which is what keeps every existing + `monkeypatch.setattr(singleton, …)` in the suite reaching the code (a direct import would + bind at import time and silently stop seeing the patch — the standing lesson from the + "moving module globals breaks monkeypatch" incident). `singleton.py` 999 → **985**; + the new leaf is 91 lines. Two lines of the original growth were also paid for honestly: + the F-856 paragraph in `_same_identity_backend_ready` was retelling what + `scheduling_lag.FairWindow` is THE one home for, and now points at it instead. diff --git a/src/stealth_chrome_devtools_mcp/cli.py b/src/stealth_chrome_devtools_mcp/cli.py index f18561a..cea0342 100644 --- a/src/stealth_chrome_devtools_mcp/cli.py +++ b/src/stealth_chrome_devtools_mcp/cli.py @@ -517,9 +517,14 @@ def _cmd_restart(_args) -> int: "respawn it, or try `restart` again." ) return 1 - # "down" (spawned but the socket never came up) or "none" (no state at - # all afterward) - both mean the restart did not produce a running - # backend. Report honestly rather than implying success. + # "down": spawned, but the socket on the port we spawned on never came up - + # the restart did not produce a running backend, so report honestly rather + # than implying success. It used to read "or 'none' (no state at all + # afterward)"; since F-868 restart reports `singleton._probe_port` for that + # one port, whose vocabulary is down/wedged/responsive and has no "none" - + # that word belonged to the record-wide walk, which restart no longer uses. + # The branch stays as written: it is the "down" arm, and a status this + # function does not recognise still has to land somewhere truthful. print(f"backend restart did not bring the backend up (state: {status}, pid {pid}).") return 1 diff --git a/src/stealth_chrome_devtools_mcp/embedded/backend_liveness.py b/src/stealth_chrome_devtools_mcp/embedded/backend_liveness.py new file mode 100644 index 0000000..92dfa56 --- /dev/null +++ b/src/stealth_chrome_devtools_mcp/embedded/backend_liveness.py @@ -0,0 +1,91 @@ +"""THE one home for "is the backend on this port alive, and which recorded +backend would THIS client be served by". + +Extracted from ``singleton`` by F-868, which left that file at 999 of its 1000 +LOC — the same gate that forced ``backend_watchdog`` out, and the same answer: +this is a self-contained pair that takes every collaborator through an +argument, so it belongs beside the module that wires it, not inside it. + +A leaf. The two liveness primitives arrive as PARAMETERS (``is_healthy`` / +``http_ready``), exactly as ``backend_watchdog`` takes its probes, so nothing +here imports ``singleton``; the record arrives as a path, so nothing here +decides WHICH record either. ``backend_registry`` — itself a leaf — is the only +import, because the adoption ORDER is its policy and is merely consumed here. + +``singleton`` keeps thin wrappers (``_probe_port`` / ``_probe_backend_status``) +that bind OUR probes, OUR record path and OUR display context to these two. +That is deliberate and not ceremony: the suite patches +``singleton._server_is_healthy``, ``singleton._backend_http_ready`` and +``singleton._probe_port`` by name, and a wrapper resolving those module globals +at CALL time is what keeps every existing ``monkeypatch.setattr(singleton, …)`` +reaching this code. A caller that imported these names directly would bind them +at import time and silently stop seeing such a patch. + +The vocabulary is one closed set, shared with ``cli._probe_recorded_backend`` +(which adds the single word this cannot reach, "no port recorded", for an entry +naming nothing usable as a port): ``down`` | ``wedged`` | ``responsive``, plus +``none`` for "no adoptable entry names a port at all". +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +from stealth_chrome_devtools_mcp.embedded import backend_registry + +if TYPE_CHECKING: + from collections.abc import Callable + from pathlib import Path + + +def probe_port( + port: int, + *, + is_healthy: Callable[[int], bool], + http_ready: Callable[[int], bool], +) -> str: + """THE liveness ladder for ONE port — socket, then a real MCP `initialize`: + "down" | "wedged" | "responsive" (F-301's third state, which a bare socket + check cannot see). Read-only. THE one home for those four lines (F-868), + with three readers: the candidate walk below, `restart_backend`'s report of + the port it spawned on, and doctor's `cli._probe_recorded_backend`, which + adds only the one word this cannot reach ("no port recorded") and was a + verbatim copy of this ladder until now. + """ + if not is_healthy(port): + return "down" + return "responsive" if http_ready(port) else "wedged" + + +def probe_recorded( + path: Path, own_context: str, *, probe: Callable[[int], str] +) -> tuple[str, int | None]: + """Report the state of the backend THIS process would be served by, for + display (CLI status/doctor) and for `stop`: `probe`'s verdict and the + port it was reached on, or ("none", None) when no adoptable entry names a + port. What this adds over that ladder is WHICH port to ask about. + + Candidates come in ADOPTION order (F-868) — `adoption_candidates`, the one + home `_find_running_server` already walks — never "whichever entry the + record lists first", which under one-entry-per-display-context is routinely + a dead sibling's: that is how `status` came to report "not running" beside + a backend serving 56 proxies, and `stop` to aim at the dead record. The + first candidate that ANSWERS wins, else the most informative verdict — + wedged over down: a wedged backend holds a port and will be evicted, a down + record names nothing running. The ORDER itself is not decided here. + + ``probe`` is a parameter rather than :func:`probe_port` called directly so + that ``singleton._probe_port`` — the binding the suite patches — stays the + thing this walk actually asks. + """ + best: tuple[str, int | None] = ("none", None) + for entry in backend_registry.adoption_candidates(path, own_context): + port = backend_registry.recorded_int(entry, "port") + if port is None: + continue + verdict = probe(port) + if verdict == "responsive": + return verdict, port + if best[0] == "none" or (best[0] == "down" and verdict == "wedged"): + best = (verdict, port) + return best diff --git a/src/stealth_chrome_devtools_mcp/embedded/singleton.py b/src/stealth_chrome_devtools_mcp/embedded/singleton.py index 63d0d97..c48db11 100644 --- a/src/stealth_chrome_devtools_mcp/embedded/singleton.py +++ b/src/stealth_chrome_devtools_mcp/embedded/singleton.py @@ -25,6 +25,7 @@ import psutil from stealth_chrome_devtools_mcp.embedded import ( + backend_liveness, backend_registry, backend_watchdog, display_context, @@ -158,46 +159,31 @@ def _clear_server_state() -> None: def _probe_port(port: int) -> str: - """THE liveness ladder for ONE port — socket, then a real MCP `initialize`: - "down" | "wedged" | "responsive" (F-301's third state, which a bare socket - check cannot see). Read-only. THE one home for those four lines (F-868), - with three readers: the candidate walk below, `restart_backend`'s report of - the port it spawned on, and doctor's `cli._probe_recorded_backend`, which - adds only the one word this cannot reach ("no port recorded") and was a - verbatim copy of this ladder until now. + """OUR binding of `backend_liveness.probe_port` — the ladder, with THIS + module's two probes handed in. The reasoning lives with the leaf. + + A wrapper on purpose, not a hop to delete: both probe names are resolved + HERE at call time, so `monkeypatch.setattr(singleton, "_server_is_healthy", + …)` still reaches the ladder. A caller importing the leaf's names directly + would bind them at import time and stop seeing such a patch. """ - if not _server_is_healthy(port): - return "down" - return "responsive" if _backend_http_ready(port) else "wedged" + return backend_liveness.probe_port( + port, is_healthy=_server_is_healthy, http_ready=_backend_http_ready + ) def _probe_backend_status() -> tuple[str, int | None]: - """Report the state of the backend THIS process would be served by, for - display (CLI status/doctor) and for `stop`: `_probe_port`'s verdict and the - port it was reached on, or ("none", None) when no adoptable entry names a - port. What this adds over that ladder is WHICH port to ask about. - - Candidates come in ADOPTION order (F-868) — `adoption_candidates`, the one - home `_find_running_server` already walks — never "whichever entry the - record lists first", which under one-entry-per-display-context is routinely - a dead sibling's: that is how `status` came to report "not running" beside - a backend serving 56 proxies, and `stop` to aim at the dead record. The - first candidate that ANSWERS wins, else the most informative verdict — - wedged over down: a wedged backend holds a port and will be evicted, a down - record names nothing running. The ORDER itself is not decided here. + """OUR binding of `backend_liveness.probe_recorded`: this module's record + path, this process's display context, and `_probe_port` above as the + per-port probe (so a test that patches THAT still drives the walk). + + `stop_backend` and the CLI's status/doctor/kill-orphans verbs all call it + through this name; the adoption-order policy is the leaf's, and the order + itself is `backend_registry`'s. """ - best: tuple[str, int | None] = ("none", None) - own = display_context.display_context() - for entry in backend_registry.adoption_candidates(SERVER_STATE_FILE, own): - port = backend_registry.recorded_int(entry, "port") - if port is None: - continue - verdict = _probe_port(port) - if verdict == "responsive": - return verdict, port - if best[0] == "none" or (best[0] == "down" and verdict == "wedged"): - best = (verdict, port) - return best + return backend_liveness.probe_recorded( + SERVER_STATE_FILE, display_context.display_context(), probe=_probe_port + ) def _same_identity_backend_ready(port: int, patience: float | None = None) -> bool: