diff --git a/audit/stage2/plan_RELEASE_FIX_B.md b/audit/stage2/plan_RELEASE_FIX_B.md index b687679..7770451 100644 --- a/audit/stage2/plan_RELEASE_FIX_B.md +++ b/audit/stage2/plan_RELEASE_FIX_B.md @@ -1,6 +1,10 @@ # plan_RELEASE_FIX_B — B1: per-session app_lifespan destroys all browser state over the real wire path -**Status: PROPOSED (awaiting human go)** +**Status: EXECUTED** — human go 2026-07-24 ("I trust your execution"); C1 = f81ff8f +(session-reentrant lifespan; if-guard in `finally` instead of the planned early +return, deliberately — a `return` inside `finally` would suppress a propagating +session exception), C2 = 585ebf2 (xfail flipped; transport journey 3/3 green over +real stdio + independent orchestrator re-run). Merge gate: human, PR pending. **Found by:** W1's real-stdio transport gate (`tests/test_e2e_transport.py`), first run. **Severity:** Tier-A-equivalent, release-blocking. Invisible to the entire in-process test suite; reproduced 3/3 over the real user path. diff --git a/src/stealth_chrome_devtools_mcp/embedded/server.py b/src/stealth_chrome_devtools_mcp/embedded/server.py index 5c78bed..1a577d0 100644 --- a/src/stealth_chrome_devtools_mcp/embedded/server.py +++ b/src/stealth_chrome_devtools_mcp/embedded/server.py @@ -211,6 +211,17 @@ def from_json(json_obj: dict[str, Any]): DEBUG_LOGGING_ENABLED = get_settings().stealth_browser_debug or get_settings().debug +# B1 (RELEASE-FIX-B): FastMCP runs the server ``lifespan`` once PER MCP SESSION +# over streamable HTTP, not once per process. Startup must therefore be guarded +# to the first entry per process, and the destructive teardown must be bound to +# *process* end (stdio standalone), never *session* end — otherwise every probe +# session's exit tears down all live browsers. ``_SERVE_TRANSPORT`` is stamped by +# the ``__main__`` entrypoint from the parsed ``--transport``; the default keeps +# the standalone-stdio contract. A boolean guard (not a refcount) is deliberate: +# an idle HTTP backend crossing back to zero sessions must NOT re-arm startup. +_LIFESPAN_STARTED = False +_SERVE_TRANSPORT = "stdio" + @asynccontextmanager async def app_lifespan(server): @@ -220,55 +231,69 @@ async def app_lifespan(server): Args: server (Any): The server instance for which the lifespan is being managed. """ - _install_asyncio_close_noise_filter() - _install_nodriver_cookie_compat() - debug_logger.log_info( - "server", "startup", "Starting Browser Automation MCP Server..." - ) - process_cleanup.activate() - try: + global _LIFESPAN_STARTED + if not _LIFESPAN_STARTED: + _LIFESPAN_STARTED = True + _install_asyncio_close_noise_filter() + _install_nodriver_cookie_compat() + debug_logger.log_info( + "server", "startup", "Starting Browser Automation MCP Server..." + ) + process_cleanup.activate() await browser_manager.start_idle_reaper() # Reclaim leaked auto-clones and trim oversized idle named profiles left # by a previous run. Fire-and-forget so a large first sweep never delays # server readiness. clone_storage.spawn_background_sweep("startup") + try: yield finally: - debug_logger.log_info( - "server", "shutdown", "Shutting down Browser Automation MCP Server..." - ) - try: - await browser_manager.stop_idle_reaper() - except Exception as e: - debug_logger.log_error("server", "cleanup", e) - try: - await browser_manager.close_all() - debug_logger.log_info("server", "cleanup", "All browser instances closed") - except Exception as e: - debug_logger.log_error("server", "cleanup", e) - - try: - process_cleanup._cleanup_all_tracked() - debug_logger.log_info("server", "cleanup", "Process cleanup complete") - except Exception as e: - debug_logger.log_error("server", "cleanup", f"Process cleanup failed: {e}") - try: - persistent_instances = in_memory_storage.list_instances() - if persistent_instances.get("instances"): + # HTTP session exit is a no-op: instances are shared across sessions and + # process termination is already reaped by process_cleanup's atexit/signal + # handlers. Only the standalone-stdio process (one session == process + # lifetime) runs the destructive teardown, preserving the 1.x contract. + # An ``if`` guard (not an early ``return``) is deliberate: a ``return`` in + # a ``finally`` would suppress an exception propagating from the session. + if _SERVE_TRANSPORT != "http": + debug_logger.log_info( + "server", "shutdown", "Shutting down Browser Automation MCP Server..." + ) + try: + await browser_manager.stop_idle_reaper() + except Exception as e: + debug_logger.log_error("server", "cleanup", e) + try: + await browser_manager.close_all() debug_logger.log_info( - "server", - "storage_cleanup", - f"Clearing in-memory storage with {len(persistent_instances['instances'])} instances...", + "server", "cleanup", "All browser instances closed" ) - in_memory_storage.clear_all() - debug_logger.log_info( - "server", "storage_cleanup", "In-memory storage cleared" + except Exception as e: + debug_logger.log_error("server", "cleanup", e) + + try: + process_cleanup._cleanup_all_tracked() + debug_logger.log_info("server", "cleanup", "Process cleanup complete") + except Exception as e: + debug_logger.log_error( + "server", "cleanup", f"Process cleanup failed: {e}" ) - except Exception as e: - debug_logger.log_error("server", "storage_cleanup", e) - debug_logger.log_info( - "server", "shutdown", "Browser Automation MCP Server shutdown complete" - ) + try: + persistent_instances = in_memory_storage.list_instances() + if persistent_instances.get("instances"): + debug_logger.log_info( + "server", + "storage_cleanup", + f"Clearing in-memory storage with {len(persistent_instances['instances'])} instances...", + ) + in_memory_storage.clear_all() + debug_logger.log_info( + "server", "storage_cleanup", "In-memory storage cleared" + ) + except Exception as e: + debug_logger.log_error("server", "storage_cleanup", e) + debug_logger.log_info( + "server", "shutdown", "Browser Automation MCP Server shutdown complete" + ) mcp = FastMCP( @@ -3324,6 +3349,10 @@ def build_arg_parser(): # Ship errors to Sentry when SENTRY_DSN is set (no-op otherwise). sentry_init() + # B1: bind app_lifespan's teardown policy to the serve transport. HTTP runs + # the lifespan per MCP session, so session-exit teardown must be a no-op. + _SERVE_TRANSPORT = args.transport + if args.transport == "http": mcp.run(transport="http", host=args.host, port=args.port) else: diff --git a/tests/test_e2e_transport.py b/tests/test_e2e_transport.py index 407b5c3..8b879f4 100644 --- a/tests/test_e2e_transport.py +++ b/tests/test_e2e_transport.py @@ -10,19 +10,6 @@ Marked ``integration`` + ``transport``; skipped when Chrome / the server is unavailable (same guard as the other e2e modules). - -KNOWN RED (finding B1, fix owned by RELEASE-FIX-B): the journey currently dies -mid-flight because FastMCP runs ``app_lifespan`` PER MCP SESSION over streamable -HTTP, and the stdio proxy's liveness watchdog opens+closes a probe session -(``_backend_http_ready``: real ``initialize`` then DELETE) every 2s — each probe's -lifespan entry re-runs orphan recovery (killing freshly spawned Chrome) and each -exit runs the full server cleanup ("All browser instances closed"). Every browser -instance on the backend dies within ~2s of any probe. The in-process E2E seam -bypasses the proxy+HTTP entirely, which is why only this transport gate sees it. -With the watchdog quieted locally the full journey passes end-to-end, so the -xfail below pins exactly this defect and nothing else. ``strict=False`` because -the failure point depends on where the 2s tick lands in the journey (a lucky -run could sneak through). RELEASE-FIX-B must remove the marker. """ from __future__ import annotations @@ -44,13 +31,6 @@ pytestmark.append(pytest.mark.skip("Chrome not available or server failed to load")) -@pytest.mark.xfail( - reason=( - "B1 (RELEASE-FIX-B): per-MCP-session app_lifespan + the proxy's 2s " - "watchdog probe sessions close all browser instances over real stdio" - ), - strict=False, -) async def test_real_stdio_release_gate_journey(tmp_path): """Foundation proof + handshake/schema + canonical journey over real stdio.""" launcher = resolve_launcher() # this env's absolute installed console launcher diff --git a/tests/test_lifespan_reentrancy.py b/tests/test_lifespan_reentrancy.py new file mode 100644 index 0000000..aec9eee --- /dev/null +++ b/tests/test_lifespan_reentrancy.py @@ -0,0 +1,144 @@ +"""RELEASE-FIX-B C1 (B1) — app_lifespan must be session-reentrant. + +FastMCP serves streamable HTTP by running the low-level MCP ``Server.run()`` — +and thus the server ``lifespan`` — **once per MCP session**, not once per +process. ``app_lifespan`` was written for once-per-process semantics, so over +HTTP every probe session's lifespan EXIT ran the full destructive teardown +(``browser_manager.close_all`` / ``process_cleanup._cleanup_all_tracked`` / +``in_memory_storage.clear_all``) and every ENTRY re-armed orphan recovery +(``process_cleanup.activate``) — killing live browsers within ~2s (finding B1). + +These hermetic tests drive ``app_lifespan`` directly (no Chrome, no backend) and +pin the fix: startup runs once per process, destructive teardown is bound to +process end (stdio standalone), and http-mode session exit is a no-op. The +process-end path is already covered by ``process_cleanup.activate``'s +atexit/signal reaping, so http-mode has nothing to clean at session exit. +""" + +from __future__ import annotations + +from typing import Any + +import pytest + +from stealth_chrome_devtools_mcp.embedded import server + + +class _SpyBrowserManager: + def __init__(self) -> None: + self.start_calls = 0 + self.stop_calls = 0 + self.close_all_calls = 0 + + async def start_idle_reaper(self) -> None: + self.start_calls += 1 + + async def stop_idle_reaper(self) -> None: + self.stop_calls += 1 + + async def close_all(self) -> None: + self.close_all_calls += 1 + + +class _SpyProcessCleanup: + def __init__(self) -> None: + self.activate_calls = 0 + self.cleanup_all_calls = 0 + + def activate(self) -> None: + self.activate_calls += 1 + + def _cleanup_all_tracked(self) -> None: + self.cleanup_all_calls += 1 + + +class _SpyStorage: + def __init__(self, instances: list[Any] | None = None) -> None: + self.clear_all_calls = 0 + self._instances = instances if instances is not None else ["seeded"] + + def list_instances(self) -> dict[str, Any]: + return {"instances": self._instances} + + def clear_all(self) -> None: + self.clear_all_calls += 1 + + +class _SpyCloneStorage: + def __init__(self) -> None: + self.sweep_calls = 0 + + def spawn_background_sweep(self, reason: str) -> None: + self.sweep_calls += 1 + + +@pytest.fixture() +def spies(monkeypatch): + """Swap the four teardown/startup singletons for spies and reset the + once-per-process startup guard so each test drives a fresh lifespan.""" + bm = _SpyBrowserManager() + pc = _SpyProcessCleanup() + ims = _SpyStorage() + cs = _SpyCloneStorage() + monkeypatch.setattr(server, "browser_manager", bm) + monkeypatch.setattr(server, "process_cleanup", pc) + monkeypatch.setattr(server, "in_memory_storage", ims) + monkeypatch.setattr(server, "clone_storage", cs) + # raising=False so the RED run (before the guard exists) fails on the + # behavioral assertions below, not on a missing-attribute setup error. + monkeypatch.setattr(server, "_LIFESPAN_STARTED", False, raising=False) + return {"bm": bm, "pc": pc, "ims": ims, "cs": cs} + + +async def test_second_lifespan_cycle_does_not_run_teardown_in_http_mode( + monkeypatch, spies +): + """In http mode, a probe session's enter+exit must NOT run any destructive + teardown while an earlier session (lifespan A) is still open.""" + monkeypatch.setattr(server, "_SERVE_TRANSPORT", "http", raising=False) + bm, pc, ims = spies["bm"], spies["pc"], spies["ims"] + + async with server.app_lifespan(None): # lifespan A — stays open + async with server.app_lifespan(None): # lifespan B — probe shape + pass + # B has fully exited; A is still open. No teardown may have fired. + assert bm.close_all_calls == 0 + assert pc.cleanup_all_calls == 0 + assert ims.clear_all_calls == 0 + + # Even A's exit is a no-op in http mode (process end reaps via atexit). + assert bm.close_all_calls == 0 + assert pc.cleanup_all_calls == 0 + assert ims.clear_all_calls == 0 + + +async def test_lifespan_reentry_does_not_rearm_orphan_recovery(monkeypatch, spies): + """``process_cleanup.activate`` (orphan recovery) must run exactly once across + two sequential lifespan cycles — a re-entry must not re-arm it.""" + monkeypatch.setattr(server, "_SERVE_TRANSPORT", "http", raising=False) + pc, cs, bm = spies["pc"], spies["cs"], spies["bm"] + + async with server.app_lifespan(None): + pass + async with server.app_lifespan(None): + pass + + assert pc.activate_calls == 1 + # The rest of the startup block is likewise once-per-process. + assert cs.sweep_calls == 1 + assert bm.start_calls == 1 + + +async def test_stdio_mode_exit_still_runs_full_teardown(monkeypatch, spies): + """The 1.x standalone-stdio contract: a single enter+exit cycle runs the full + destructive teardown (close_all / _cleanup_all_tracked / clear_all).""" + monkeypatch.setattr(server, "_SERVE_TRANSPORT", "stdio", raising=False) + bm, pc, ims = spies["bm"], spies["pc"], spies["ims"] + + async with server.app_lifespan(None): + pass + + assert bm.stop_calls == 1 + assert bm.close_all_calls == 1 + assert pc.cleanup_all_calls == 1 + assert ims.clear_all_calls == 1