diff --git a/docs/superpowers/plans/2026-07-07-wabot-modernization.md b/docs/superpowers/plans/2026-07-07-wabot-modernization.md index 7ed468e..94e6ac3 100644 --- a/docs/superpowers/plans/2026-07-07-wabot-modernization.md +++ b/docs/superpowers/plans/2026-07-07-wabot-modernization.md @@ -1126,7 +1126,6 @@ git commit -m "feat: ManagedService spawns detached driver services that outlive `tests/unit/test_reattach.py`. Constructing a `Remote` performs **no HTTP until the first command** (verified), so adoption can be unit-tested without a server; the dead-session path monkeypatches the probe: ```python -import pytest from selenium.common.exceptions import WebDriverException from wabot.hosts import ReattachingRemote, attach, build_options @@ -1134,6 +1133,8 @@ from wabot.hosts import ReattachingRemote, attach, build_options class TestReattachingRemote: def test_adopts_session_id_without_creating_a_session(self): + # constructing against a closed port (127.0.0.1:1) proves no HTTP happens + # until the first command driver = ReattachingRemote( "http://127.0.0.1:1", "saved-session-id", options=build_options("chromium") ) @@ -1168,6 +1169,19 @@ class TestAttach: def test_returns_none_when_server_is_unreachable(self): # no server on port 1: connection refused is immediate; attach must not raise assert attach("http://127.0.0.1:1", "sid", "chromium") is None + + def test_dead_session_closes_executor(self, monkeypatch): + from selenium.webdriver.remote.remote_connection import RemoteConnection + + closed = [] + monkeypatch.setattr(RemoteConnection, "close", lambda self: closed.append(1)) + + def boom(self): + raise WebDriverException("invalid session id") + + monkeypatch.setattr(ReattachingRemote, "current_url", property(boom)) + assert attach("http://127.0.0.1:1", "sid", "chromium") is None + assert closed == [1] # executor was closed, not leaked ``` - [ ] **Step 2: Run tests to verify they fail** @@ -1182,8 +1196,14 @@ Append to `src/wabot/hosts.py`; add these imports to the top block: ```python from selenium.common.exceptions import WebDriverException from selenium.webdriver.remote.webdriver import WebDriver as RemoteWebDriver +from urllib3.exceptions import HTTPError as _Urllib3HTTPError ``` +(The `urllib3` import is the resolved form of the note below: selenium's HTTP +layer surfaces `test_returns_none_when_server_is_unreachable`'s connection +refusal as a `urllib3.exceptions.MaxRetryError`, which is **not** an `OSError`, +so it must be caught explicitly. This was confirmed against selenium 4.45.) + Appended code: ```python @@ -1201,6 +1221,11 @@ class ReattachingRemote(RemoteWebDriver): # calling start_session, so stash the id under a private name. self._reattach_session_id = session_id super().__init__(command_executor=command_executor, options=options) + if self.session_id != session_id: + raise RuntimeError( + "session adoption failed: selenium's start_session contract " + "changed; pin selenium <5 or update ReattachingRemote" + ) def start_session(self, capabilities: dict) -> None: # Skip Command.NEW_SESSION entirely; adopt the saved session. @@ -1216,31 +1241,28 @@ def attach(executor_url: str, session_id: str, browser: str): return driver except WebDriverException as ex: LOGGER.warning("saved session %s is dead: %s", session_id, ex) + driver.command_executor.close() # do not leak the keep-alive socket return None - except OSError as ex: # server itself unreachable (connection refused, timeout) + except (OSError, _Urllib3HTTPError) as ex: # server unreachable (connection refused, timeout) LOGGER.warning("no server at %s: %s", executor_url, ex) + driver.command_executor.close() # do not leak the keep-alive socket return None ``` -Note: selenium's HTTP layer (urllib3) can surface connection failures as -`urllib3.exceptions.HTTPError` subclasses that are **not** `OSError`. If -`test_returns_none_when_server_is_unreachable` fails with an unhandled -`MaxRetryError`, add it explicitly: - -```python -from urllib3.exceptions import HTTPError as _Urllib3HTTPError -``` - -and change the last except clause to `except (OSError, _Urllib3HTTPError) as ex:`. -Whichever variant makes the test pass is the correct final code. +`driver.quit()` cannot be used to release the executor on the failure paths: +on a dead session it issues a DELETE that re-raises. Closing the executor +directly (`driver.command_executor.close()`) releases urllib3's pooled +keep-alive connection without another round-trip. The `session_id` tripwire in +`__init__` raises (not `assert`, so it survives `python -O`) to turn a future +silent adoption failure into a loud one at construction time. - [ ] **Step 4: Run tests to verify they pass** Run: `uv run pytest tests/unit/test_reattach.py -v` -Expected: 5 passed +Expected: 6 passed Run: `uv run pytest` -Expected: all unit tests so far pass (pacing 7, sessions 14, hosts 19, reattach 5 = 45) +Expected: all unit tests so far pass (pacing 7, sessions 14, hosts 19, reattach 6 = 46) - [ ] **Step 5: Commit** @@ -2765,7 +2787,7 @@ Run: `uv run pytest tests/unit/test_api.py -v` Expected: 11 passed Run: `uv run pytest` -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) +Expected: full unit suite passes (107 tests: pacing 7, sessions 14, hosts 19, reattach 6, 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 16f5c90..ce8dac5 100644 --- a/src/wabot/hosts.py +++ b/src/wabot/hosts.py @@ -236,6 +236,11 @@ class ReattachingRemote(RemoteWebDriver): # calling start_session, so stash the id under a private name. self._reattach_session_id = session_id super().__init__(command_executor=command_executor, options=options) + if self.session_id != session_id: + raise RuntimeError( + "session adoption failed: selenium's start_session contract " + "changed; pin selenium <5 or update ReattachingRemote" + ) def start_session(self, capabilities: dict) -> None: # Skip Command.NEW_SESSION entirely; adopt the saved session. @@ -251,7 +256,9 @@ def attach(executor_url: str, session_id: str, browser: str): return driver except WebDriverException as ex: LOGGER.warning("saved session %s is dead: %s", session_id, ex) + driver.command_executor.close() return None except (OSError, _Urllib3HTTPError) as ex: # server unreachable (connection refused, timeout) LOGGER.warning("no server at %s: %s", executor_url, ex) + driver.command_executor.close() return None diff --git a/tests/unit/test_reattach.py b/tests/unit/test_reattach.py index ca677a5..8b3c1c7 100644 --- a/tests/unit/test_reattach.py +++ b/tests/unit/test_reattach.py @@ -5,6 +5,8 @@ from wabot.hosts import ReattachingRemote, attach, build_options class TestReattachingRemote: def test_adopts_session_id_without_creating_a_session(self): + # constructing against a closed port (127.0.0.1:1) proves no HTTP happens + # until the first command driver = ReattachingRemote( "http://127.0.0.1:1", "saved-session-id", options=build_options("chromium") ) @@ -39,3 +41,16 @@ class TestAttach: def test_returns_none_when_server_is_unreachable(self): # no server on port 1: connection refused is immediate; attach must not raise assert attach("http://127.0.0.1:1", "sid", "chromium") is None + + def test_dead_session_closes_executor(self, monkeypatch): + from selenium.webdriver.remote.remote_connection import RemoteConnection + + closed = [] + monkeypatch.setattr(RemoteConnection, "close", lambda self: closed.append(1)) + + def boom(self): + raise WebDriverException("invalid session id") + + monkeypatch.setattr(ReattachingRemote, "current_url", property(boom)) + assert attach("http://127.0.0.1:1", "sid", "chromium") is None + assert closed == [1] # executor was closed, not leaked