fix: browser()/destroy() never leak a managed driver service

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,10 +3118,14 @@ 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 = None
+    try:
         driver = _new_remote(url, options)
         store.save(
             SessionRecord(
@@ -3044,6 +3138,16 @@ def browser(
                 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):
+        try:
             driver = attach(record.executor_url, record.session_id, record.browser)
             if driver is not None:
-            try:
                 driver.quit()
         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**
 

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,10 +107,14 @@ 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 = None
+    try:
         driver = _new_remote(url, options)
         store.save(
             SessionRecord(
@@ -119,6 +127,16 @@ def browser(
                 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):
+        try:
             driver = attach(record.executor_url, record.session_id, record.browser)
             if driver is not None:
-            try:
                 driver.quit()
         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)

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__: