diff --git a/docs/superpowers/plans/2026-07-07-wabot-modernization.md b/docs/superpowers/plans/2026-07-07-wabot-modernization.md index 6c37d8c..66def02 100644 --- a/docs/superpowers/plans/2026-07-07-wabot-modernization.md +++ b/docs/superpowers/plans/2026-07-07-wabot-modernization.md @@ -2907,6 +2907,92 @@ class TestHousekeeping: assert wabot.destroy("nope", store=store) is False +class TestResourceCleanup: + def test_managed_service_stopped_when_driver_creation_fails( + self, fake_selenium, store, monkeypatch + ): + from selenium.common.exceptions import WebDriverException + + stopped = [] + monkeypatch.setattr(wabot, "stop_service", lambda pid: stopped.append(pid)) + fake_selenium.Remote.side_effect = WebDriverException("startup failed") + with pytest.raises(WebDriverException): + wabot.browser(session="s1", browser="chromium", store=store) + assert stopped == [4321] # spawned managed service was stopped + assert store.get("s1") is None # nothing persisted + + def test_dead_managed_session_stops_old_service_before_recreating( + self, fake_selenium, store, monkeypatch + ): + from datetime import datetime, timezone + + stopped = [] + monkeypatch.setattr(wabot, "stop_service", lambda pid: stopped.append(pid)) + store.save(wabot.SessionRecord( + name="s1", executor_url="http://127.0.0.1:7777", session_id="old", + browser="chromium", created_at=datetime.now(timezone.utc).isoformat(), + service_pid=9999, service_port=7777, + )) + fake_selenium.service_alive.return_value = True + fake_selenium.attach.return_value = None # session dead on a live server + wabot.browser(session="s1", store=store) + assert 9999 in stopped # old managed service stopped, not leaked + fake_selenium.Remote.assert_called_once() # fresh browser created + assert store.get("s1").session_id == "new-session-id" + + +class TestDestroyEdges: + def _save(self, store, **kw): + from datetime import datetime, timezone + + defaults = dict( + name="s1", executor_url="http://127.0.0.1:7777", session_id="old", + browser="chromium", created_at=datetime.now(timezone.utc).isoformat(), + ) + defaults.update(kw) + store.save(wabot.SessionRecord(**defaults)) + + def test_destroy_external_session_does_not_stop_service( + self, fake_selenium, store, monkeypatch + ): + stopped = [] + monkeypatch.setattr(wabot, "stop_service", lambda pid: stopped.append(pid)) + self._save(store, name="ext", executor_url="http://grid:4444", service_pid=None) + fake_selenium.service_alive.return_value = True + assert wabot.destroy("ext", store=store) is True + assert stopped == [] # external server: not ours to stop + assert store.get("ext") is None + + def test_destroy_dead_session_still_stops_and_removes(self, fake_selenium, store, monkeypatch): + stopped = [] + monkeypatch.setattr(wabot, "stop_service", lambda pid: stopped.append(pid)) + self._save(store, service_pid=4321) + fake_selenium.service_alive.return_value = True + fake_selenium.attach.return_value = None # dead session + assert wabot.destroy("s1", store=store) is True + assert stopped == [4321] # service stopped despite dead session + assert store.get("s1") is None + + +class TestPlumbing: + def test_headless_and_user_agent_reach_build_options(self, fake_selenium, store, monkeypatch): + captured = {} + real_build = wabot.build_options + + def spy(browser, **kwargs): + captured.update(kwargs) + return real_build(browser, **kwargs) + + monkeypatch.setattr(wabot, "build_options", spy) + wabot.browser(browser="chromium", headless=True, user_agent="Bot/1.0", store=store) + assert captured == {"headless": True, "user_agent": "Bot/1.0"} + + def test_pacing_reaches_browser(self, fake_selenium, store): + pacing = wabot.NoPacing() + bot = wabot.browser(browser="chromium", pacing=pacing, store=store) + assert bot.pacing is pacing + + class TestPublicSurface: def test_all_exports_exist(self): for name in wabot.__all__: @@ -2936,6 +3022,7 @@ Quickstart:: from __future__ import annotations +import contextlib import logging from datetime import datetime, timezone @@ -2999,6 +3086,9 @@ def browser( ) -> Browser: """Create a Browser, reattaching to a saved session when one exists. + When reattaching to an existing session, ``headless`` and ``user_agent`` + are ignored (the browser already exists). + Args: session: Persistence name. None (default) = ephemeral: the browser is not saved and (without ``host``) dies with this process. @@ -3028,22 +3118,36 @@ def browser( LOGGER.info("reattached to session %r", session) return Browser(driver, pacing=pacing, session_name=session, store=store) LOGGER.warning("saved session %r is dead; creating a fresh browser", session) + if record.service_pid: + stop_service(record.service_pid) # don't leak the old managed service store.remove(session) host_obj = ExternalServer(host) if host else ManagedService(browser_name) url = host_obj.ensure_running() - driver = _new_remote(url, options) - store.save( - SessionRecord( - name=session, - executor_url=url, - session_id=driver.session_id, - browser=browser_name, - created_at=datetime.now(timezone.utc).isoformat(), - service_pid=getattr(host_obj, "pid", None), - service_port=getattr(host_obj, "port", None), + driver = None + try: + driver = _new_remote(url, options) + store.save( + SessionRecord( + name=session, + executor_url=url, + session_id=driver.session_id, + browser=browser_name, + created_at=datetime.now(timezone.utc).isoformat(), + service_pid=getattr(host_obj, "pid", None), + service_port=getattr(host_obj, "port", None), + ) ) - ) + except BaseException: + # startup or persistence failed: don't leak the driver session or the + # detached service we just spawned + if driver is not None: + with contextlib.suppress(Exception): + driver.quit() + pid = getattr(host_obj, "pid", None) + if pid: + stop_service(pid) + raise LOGGER.info("created persistent session %r on %s", session, url) return Browser(driver, pacing=pacing, session_name=session, store=store) @@ -3060,12 +3164,14 @@ def destroy(name: str, store: SessionStore | None = None) -> bool: if record is None: return False if service_alive(record.executor_url): - driver = attach(record.executor_url, record.session_id, record.browser) - if driver is not None: - try: + try: + driver = attach(record.executor_url, record.session_id, record.browser) + if driver is not None: driver.quit() - except WebDriverException as ex: - LOGGER.warning("quit failed while destroying %r: %s", name, ex) + except WebDriverException as ex: + LOGGER.warning("quit failed while destroying %r: %s", name, ex) + except RuntimeError as ex: # attach adoption guard can raise + LOGGER.warning("could not reattach to destroy %r: %s", name, ex) if record.service_pid: stop_service(record.service_pid) store.remove(name) @@ -3075,10 +3181,10 @@ def destroy(name: str, store: SessionStore | None = None) -> bool: - [ ] **Step 4: Run tests to verify they pass** Run: `uv run pytest tests/unit/test_api.py -v` -Expected: 11 passed +Expected: 17 passed Run: `uv run pytest` -Expected: full unit suite passes (134 tests: pacing 7, sessions 14, hosts 19, reattach 6, fields 17, page 36, screenshot 5, browser 19, api 11) +Expected: full unit suite passes (140 tests: pacing 7, sessions 14, hosts 19, reattach 6, fields 17, page 36, screenshot 5, browser 19, api 17) - [ ] **Step 5: Commit** diff --git a/src/wabot/__init__.py b/src/wabot/__init__.py index 49b51d9..2d53a7d 100644 --- a/src/wabot/__init__.py +++ b/src/wabot/__init__.py @@ -11,6 +11,7 @@ Quickstart:: from __future__ import annotations +import contextlib import logging from datetime import datetime, timezone @@ -74,6 +75,9 @@ def browser( ) -> Browser: """Create a Browser, reattaching to a saved session when one exists. + When reattaching to an existing session, ``headless`` and ``user_agent`` + are ignored (the browser already exists). + Args: session: Persistence name. None (default) = ephemeral: the browser is not saved and (without ``host``) dies with this process. @@ -103,22 +107,36 @@ def browser( LOGGER.info("reattached to session %r", session) return Browser(driver, pacing=pacing, session_name=session, store=store) LOGGER.warning("saved session %r is dead; creating a fresh browser", session) + if record.service_pid: + stop_service(record.service_pid) # don't leak the old managed service store.remove(session) host_obj = ExternalServer(host) if host else ManagedService(browser_name) url = host_obj.ensure_running() - driver = _new_remote(url, options) - store.save( - SessionRecord( - name=session, - executor_url=url, - session_id=driver.session_id, - browser=browser_name, - created_at=datetime.now(timezone.utc).isoformat(), - service_pid=getattr(host_obj, "pid", None), - service_port=getattr(host_obj, "port", None), + driver = None + try: + driver = _new_remote(url, options) + store.save( + SessionRecord( + name=session, + executor_url=url, + session_id=driver.session_id, + browser=browser_name, + created_at=datetime.now(timezone.utc).isoformat(), + service_pid=getattr(host_obj, "pid", None), + service_port=getattr(host_obj, "port", None), + ) ) - ) + except BaseException: + # startup or persistence failed: don't leak the driver session or the + # detached service we just spawned + if driver is not None: + with contextlib.suppress(Exception): + driver.quit() + pid = getattr(host_obj, "pid", None) + if pid: + stop_service(pid) + raise LOGGER.info("created persistent session %r on %s", session, url) return Browser(driver, pacing=pacing, session_name=session, store=store) @@ -135,12 +153,14 @@ def destroy(name: str, store: SessionStore | None = None) -> bool: if record is None: return False if service_alive(record.executor_url): - driver = attach(record.executor_url, record.session_id, record.browser) - if driver is not None: - try: + try: + driver = attach(record.executor_url, record.session_id, record.browser) + if driver is not None: driver.quit() - except WebDriverException as ex: - LOGGER.warning("quit failed while destroying %r: %s", name, ex) + except WebDriverException as ex: + LOGGER.warning("quit failed while destroying %r: %s", name, ex) + except RuntimeError as ex: # attach adoption guard can raise + LOGGER.warning("could not reattach to destroy %r: %s", name, ex) if record.service_pid: stop_service(record.service_pid) store.remove(name) diff --git a/tests/unit/test_api.py b/tests/unit/test_api.py index fd87afc..186a58b 100644 --- a/tests/unit/test_api.py +++ b/tests/unit/test_api.py @@ -137,6 +137,92 @@ class TestHousekeeping: assert wabot.destroy("nope", store=store) is False +class TestResourceCleanup: + def test_managed_service_stopped_when_driver_creation_fails( + self, fake_selenium, store, monkeypatch + ): + from selenium.common.exceptions import WebDriverException + + stopped = [] + monkeypatch.setattr(wabot, "stop_service", lambda pid: stopped.append(pid)) + fake_selenium.Remote.side_effect = WebDriverException("startup failed") + with pytest.raises(WebDriverException): + wabot.browser(session="s1", browser="chromium", store=store) + assert stopped == [4321] # spawned managed service was stopped + assert store.get("s1") is None # nothing persisted + + def test_dead_managed_session_stops_old_service_before_recreating( + self, fake_selenium, store, monkeypatch + ): + from datetime import datetime, timezone + + stopped = [] + monkeypatch.setattr(wabot, "stop_service", lambda pid: stopped.append(pid)) + store.save(wabot.SessionRecord( + name="s1", executor_url="http://127.0.0.1:7777", session_id="old", + browser="chromium", created_at=datetime.now(timezone.utc).isoformat(), + service_pid=9999, service_port=7777, + )) + fake_selenium.service_alive.return_value = True + fake_selenium.attach.return_value = None # session dead on a live server + wabot.browser(session="s1", store=store) + assert 9999 in stopped # old managed service stopped, not leaked + fake_selenium.Remote.assert_called_once() # fresh browser created + assert store.get("s1").session_id == "new-session-id" + + +class TestDestroyEdges: + def _save(self, store, **kw): + from datetime import datetime, timezone + + defaults = dict( + name="s1", executor_url="http://127.0.0.1:7777", session_id="old", + browser="chromium", created_at=datetime.now(timezone.utc).isoformat(), + ) + defaults.update(kw) + store.save(wabot.SessionRecord(**defaults)) + + def test_destroy_external_session_does_not_stop_service( + self, fake_selenium, store, monkeypatch + ): + stopped = [] + monkeypatch.setattr(wabot, "stop_service", lambda pid: stopped.append(pid)) + self._save(store, name="ext", executor_url="http://grid:4444", service_pid=None) + fake_selenium.service_alive.return_value = True + assert wabot.destroy("ext", store=store) is True + assert stopped == [] # external server: not ours to stop + assert store.get("ext") is None + + def test_destroy_dead_session_still_stops_and_removes(self, fake_selenium, store, monkeypatch): + stopped = [] + monkeypatch.setattr(wabot, "stop_service", lambda pid: stopped.append(pid)) + self._save(store, service_pid=4321) + fake_selenium.service_alive.return_value = True + fake_selenium.attach.return_value = None # dead session + assert wabot.destroy("s1", store=store) is True + assert stopped == [4321] # service stopped despite dead session + assert store.get("s1") is None + + +class TestPlumbing: + def test_headless_and_user_agent_reach_build_options(self, fake_selenium, store, monkeypatch): + captured = {} + real_build = wabot.build_options + + def spy(browser, **kwargs): + captured.update(kwargs) + return real_build(browser, **kwargs) + + monkeypatch.setattr(wabot, "build_options", spy) + wabot.browser(browser="chromium", headless=True, user_agent="Bot/1.0", store=store) + assert captured == {"headless": True, "user_agent": "Bot/1.0"} + + def test_pacing_reaches_browser(self, fake_selenium, store): + pacing = wabot.NoPacing() + bot = wabot.browser(browser="chromium", pacing=pacing, store=store) + assert bot.pacing is pacing + + class TestPublicSurface: def test_all_exports_exist(self): for name in wabot.__all__: