mirror of
https://git.zavage.net/Zavage-Software/wabot.git
synced 2026-07-21 13:06:08 -06:00
fix: ManagedService terminates unresponsive drivers; stop_service reaps children
This commit is contained in:
parent
f82b77ae62
commit
65d31e8821
@ -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**
|
||||
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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:
|
||||
|
||||
Loading…
Reference in New Issue
Block a user