From 65d31e88210a0f2cbbf8dbc0443141becbd6e710 Mon Sep 17 00:00:00 2001 From: Mathew Sir Guest the best Date: Tue, 7 Jul 2026 22:23:09 -0600 Subject: [PATCH] fix: ManagedService terminates unresponsive drivers; stop_service reaps children --- .../plans/2026-07-07-wabot-modernization.md | 97 ++++++++++++++++--- src/wabot/hosts.py | 52 ++++++++-- tests/unit/test_hosts.py | 38 +++++++- 3 files changed, 161 insertions(+), 26 deletions(-) diff --git a/docs/superpowers/plans/2026-07-07-wabot-modernization.md b/docs/superpowers/plans/2026-07-07-wabot-modernization.md index 3be14ff..7ed468e 100644 --- a/docs/superpowers/plans/2026-07-07-wabot-modernization.md +++ b/docs/superpowers/plans/2026-07-07-wabot-modernization.md @@ -840,7 +840,6 @@ Append to `tests/unit/test_hosts.py`. The tests use a stub "driver" executable ```python import os -import signal import stat import sys import time @@ -868,6 +867,21 @@ def stub_driver(tmp_path): return path +SILENT_DRIVER = """\ +#!{python} +import sys, time +time.sleep(300) # accepts --port=N like a real driver but never serves /status +""" + + +@pytest.fixture +def silent_driver(tmp_path): + path = tmp_path / "chromedriver" + path.write_text(SILENT_DRIVER.format(python=sys.executable)) + path.chmod(path.stat().st_mode | stat.S_IEXEC) + return path + + class TestFindDriverBinary: def test_env_var_override_wins(self, monkeypatch): monkeypatch.setenv("WABOT_CHROMEDRIVER", "/custom/chromedriver") @@ -894,13 +908,30 @@ class TestManagedService: finally: stop_service(svc.pid) - def test_startup_timeout_raises(self, tmp_path): - # /bin/true exits immediately and never serves /status + def test_early_exit_raises_with_returncode(self, tmp_path): + # /bin/true exits immediately: distinct from a slow timeout svc = ManagedService( - "chromium", log_dir=tmp_path, driver_binary="/bin/true", startup_timeout=1.0 + "chromium", log_dir=tmp_path, driver_binary="/bin/true", startup_timeout=5.0 + ) + with pytest.raises(RuntimeError, match="exited early"): + svc.ensure_running() + + def test_startup_timeout_terminates_and_does_not_leak(self, silent_driver, tmp_path): + svc = ManagedService( + "chromium", log_dir=tmp_path, driver_binary=str(silent_driver), startup_timeout=1.0 ) with pytest.raises(TimeoutError, match="did not answer /status"): svc.ensure_running() + # the unresponsive driver must have been terminated + reaped, not leaked + deadline = time.monotonic() + 5 + while time.monotonic() < deadline: + try: + os.kill(svc.pid, 0) + except ProcessLookupError: + break + time.sleep(0.05) + with pytest.raises(ProcessLookupError): + os.kill(svc.pid, 0) class TestStopService: @@ -948,8 +979,12 @@ from selenium import webdriver ```python def find_driver_binary(browser: str) -> str: - """Locate chromedriver/geckodriver: env override, PATH, then Selenium Manager.""" - name = DRIVER_BINARIES[normalize_browser(browser)] + """Locate chromedriver/geckodriver: env override, PATH, then Selenium Manager. + + The Selenium Manager fallback may download a driver over the network. + """ + browser = normalize_browser(browser) + name = DRIVER_BINARIES[browser] override = os.environ.get(f"WABOT_{name.upper()}") if override: return override @@ -966,7 +1001,8 @@ def find_driver_binary(browser: str) -> str: if browser == "chromium" and chromium: args += ["--browser-path", chromium] return SeleniumManager().binary_paths(args)["driver_path"] - except Exception as ex: # noqa: BLE001 — beta API, failure modes unknown + # Selenium Manager is beta; any failure folds into a clean FileNotFoundError + except Exception as ex: raise FileNotFoundError( f"could not find {name!r} on PATH (set WABOT_{name.upper()} to override); " f"Selenium Manager fallback also failed: {ex}" @@ -980,7 +1016,10 @@ def _free_port() -> int: class ManagedService: - """A driver service wabot spawns detached, so it outlives this process.""" + """A driver service wabot spawns detached, so it outlives this process. + + ensure_running() is not idempotent: each call spawns a new service. + """ def __init__( self, @@ -1016,9 +1055,22 @@ class ManagedService: while time.monotonic() < deadline: if service_alive(url): return url - if process.poll() is not None: - break # exited early; fall through to the error + returncode = process.poll() + if returncode is not None: + raise RuntimeError( + f"{binary} exited early with code {returncode} before " + f"answering /status on {url} (see {log_path})" + ) time.sleep(0.2) + # deadline expired but the driver is still running and unresponsive: + # terminate and reap it so we never leak a process that would otherwise + # outlive the interpreter. + process.terminate() + try: + process.wait(timeout=5) + except subprocess.TimeoutExpired: + process.kill() + process.wait() raise TimeoutError( f"{binary} did not answer /status on {url} within " f"{self.startup_timeout}s (see {log_path})" @@ -1026,18 +1078,33 @@ class ManagedService: def stop_service(pid: int) -> bool: - """SIGTERM a managed driver service. False if it was already gone.""" + """SIGTERM a managed driver service. False if it was already gone. + + If the service is a child of THIS process (the same-process spawn→stop + lifecycle), it is reaped to avoid a zombie. In the cross-process reattach + case the driver is not our child and init reaps it. A driver that ignores + SIGTERM is left running (no SIGKILL escalation). + """ try: os.kill(pid, signal.SIGTERM) - return True except (ProcessLookupError, PermissionError): return False + deadline = time.monotonic() + 5.0 + while time.monotonic() < deadline: + try: + reaped, _ = os.waitpid(pid, os.WNOHANG) + except ChildProcessError: + break # not our child (cross-process) — init reaps it + if reaped: + break + time.sleep(0.05) + return True ``` - [ ] **Step 4: Run tests to verify they pass** Run: `uv run pytest tests/unit/test_hosts.py -v` -Expected: 18 passed (13 from Task 4 + 5 new) +Expected: 19 passed (13 from Task 4 + 6 new) - [ ] **Step 5: Commit** @@ -1173,7 +1240,7 @@ Run: `uv run pytest tests/unit/test_reattach.py -v` Expected: 5 passed Run: `uv run pytest` -Expected: all unit tests so far pass (pacing 7, sessions 14, hosts 18, reattach 5 = 44) +Expected: all unit tests so far pass (pacing 7, sessions 14, hosts 19, reattach 5 = 45) - [ ] **Step 5: Commit** @@ -2698,7 +2765,7 @@ Run: `uv run pytest tests/unit/test_api.py -v` Expected: 11 passed Run: `uv run pytest` -Expected: full unit suite passes (105 tests: pacing 7, sessions 14, hosts 18, reattach 5, fields 11, page 23, screenshot 3, browser 13, api 11) +Expected: full unit suite passes (106 tests: pacing 7, sessions 14, hosts 19, reattach 5, fields 11, page 23, screenshot 3, browser 13, api 11) - [ ] **Step 5: Commit** diff --git a/src/wabot/hosts.py b/src/wabot/hosts.py index 0cf7b3a..b6b70f9 100644 --- a/src/wabot/hosts.py +++ b/src/wabot/hosts.py @@ -97,8 +97,12 @@ class ExternalServer: def find_driver_binary(browser: str) -> str: - """Locate chromedriver/geckodriver: env override, PATH, then Selenium Manager.""" - name = DRIVER_BINARIES[normalize_browser(browser)] + """Locate chromedriver/geckodriver: env override, PATH, then Selenium Manager. + + The Selenium Manager fallback may download a driver over the network. + """ + browser = normalize_browser(browser) + name = DRIVER_BINARIES[browser] override = os.environ.get(f"WABOT_{name.upper()}") if override: return override @@ -115,7 +119,8 @@ def find_driver_binary(browser: str) -> str: if browser == "chromium" and chromium: args += ["--browser-path", chromium] return SeleniumManager().binary_paths(args)["driver_path"] - except Exception as ex: # noqa: BLE001 — beta API, failure modes unknown + # Selenium Manager is beta; any failure folds into a clean FileNotFoundError + except Exception as ex: raise FileNotFoundError( f"could not find {name!r} on PATH (set WABOT_{name.upper()} to override); " f"Selenium Manager fallback also failed: {ex}" @@ -129,7 +134,10 @@ def _free_port() -> int: class ManagedService: - """A driver service wabot spawns detached, so it outlives this process.""" + """A driver service wabot spawns detached, so it outlives this process. + + ensure_running() is not idempotent: each call spawns a new service. + """ def __init__( self, @@ -165,9 +173,22 @@ class ManagedService: while time.monotonic() < deadline: if service_alive(url): return url - if process.poll() is not None: - break # exited early; fall through to the error + returncode = process.poll() + if returncode is not None: + raise RuntimeError( + f"{binary} exited early with code {returncode} before " + f"answering /status on {url} (see {log_path})" + ) time.sleep(0.2) + # deadline expired but the driver is still running and unresponsive: + # terminate and reap it so we never leak a process that would otherwise + # outlive the interpreter. + process.terminate() + try: + process.wait(timeout=5) + except subprocess.TimeoutExpired: + process.kill() + process.wait() raise TimeoutError( f"{binary} did not answer /status on {url} within " f"{self.startup_timeout}s (see {log_path})" @@ -175,9 +196,24 @@ class ManagedService: def stop_service(pid: int) -> bool: - """SIGTERM a managed driver service. False if it was already gone.""" + """SIGTERM a managed driver service. False if it was already gone. + + If the service is a child of THIS process (the same-process spawn→stop + lifecycle), it is reaped to avoid a zombie. In the cross-process reattach + case the driver is not our child and init reaps it. A driver that ignores + SIGTERM is left running (no SIGKILL escalation). + """ try: os.kill(pid, signal.SIGTERM) - return True except (ProcessLookupError, PermissionError): return False + deadline = time.monotonic() + 5.0 + while time.monotonic() < deadline: + try: + reaped, _ = os.waitpid(pid, os.WNOHANG) + except ChildProcessError: + break # not our child (cross-process) — init reaps it + if reaped: + break + time.sleep(0.05) + return True diff --git a/tests/unit/test_hosts.py b/tests/unit/test_hosts.py index 7686b1b..88971d7 100644 --- a/tests/unit/test_hosts.py +++ b/tests/unit/test_hosts.py @@ -154,6 +154,21 @@ def stub_driver(tmp_path): return path +SILENT_DRIVER = """\ +#!{python} +import sys, time +time.sleep(300) # accepts --port=N like a real driver but never serves /status +""" + + +@pytest.fixture +def silent_driver(tmp_path): + path = tmp_path / "chromedriver" + path.write_text(SILENT_DRIVER.format(python=sys.executable)) + path.chmod(path.stat().st_mode | stat.S_IEXEC) + return path + + class TestFindDriverBinary: def test_env_var_override_wins(self, monkeypatch): monkeypatch.setenv("WABOT_CHROMEDRIVER", "/custom/chromedriver") @@ -180,13 +195,30 @@ class TestManagedService: finally: stop_service(svc.pid) - def test_startup_timeout_raises(self, tmp_path): - # /bin/true exits immediately and never serves /status + def test_early_exit_raises_with_returncode(self, tmp_path): + # /bin/true exits immediately: distinct from a slow timeout svc = ManagedService( - "chromium", log_dir=tmp_path, driver_binary="/bin/true", startup_timeout=1.0 + "chromium", log_dir=tmp_path, driver_binary="/bin/true", startup_timeout=5.0 + ) + with pytest.raises(RuntimeError, match="exited early"): + svc.ensure_running() + + def test_startup_timeout_terminates_and_does_not_leak(self, silent_driver, tmp_path): + svc = ManagedService( + "chromium", log_dir=tmp_path, driver_binary=str(silent_driver), startup_timeout=1.0 ) with pytest.raises(TimeoutError, match="did not answer /status"): svc.ensure_running() + # the unresponsive driver must have been terminated + reaped, not leaked + deadline = time.monotonic() + 5 + while time.monotonic() < deadline: + try: + os.kill(svc.pid, 0) + except ProcessLookupError: + break + time.sleep(0.05) + with pytest.raises(ProcessLookupError): + os.kill(svc.pid, 0) class TestStopService: