[py] terminate driver service process when start() fails to connect

[titusfortner]

🔗 Related Issues

Spotted by the Code Review agent in #17649.

💥 What does this PR do?

Service.start() left the launched driver process running if it never became connectable. This terminates it before re-raising.

🔧 Implementation Notes

Fixed in base Service.start() so all bindings' drivers are covered in one place. No API change. New unit test fails without the fix, passes with it.

🤖 AI assistance

• AI assisted (complete below)
  • Tool(s): Claude Code (Opus 4.8)
  • What was generated: the start() cleanup and the unit test
  • I reviewed all AI output and can explain the change

🔄 Types of changes

• Bug fix (backwards compatible)

[qodo-code-review]

Review Summary by Qodo

Terminate driver service process when start() fails to connect

🐞 Bug fix

Walkthroughs

Description

• Wraps Service.start() connection loop in try-except to terminate process on failure
• Ensures driver process cleanup when connection cannot be established
• Adds unit test verifying process termination on connection timeout

Diagram

flowchart LR
  A["Service.start() called"] --> B["Launch process"]
  B --> C["Try connection loop"]
  C --> D{Connection successful?}
  D -->|Yes| E["Return successfully"]
  D -->|No| F["Exception raised"]
  F --> G["Catch exception"]
  G --> H["Call stop() to terminate"]
  H --> I["Re-raise exception"]

Loading

File Changes

1. py/selenium/webdriver/common/service.py 🐞 Bug fix +13/-9

Add process cleanup on connection failure

• Wraps the connection retry loop in a try-except block
• Calls self.stop() to terminate the process before re-raising any exception
• Ensures cleanup occurs when connection fails or process dies unexpectedly

py/selenium/webdriver/common/service.py

---

2. py/test/unit/selenium/webdriver/common/service_tests.py 🧪 Tests +42/-0

Add unit test for process termination on failure

• Creates new test file for Service class unit tests
• Implements _UnreachableService subclass that launches but never serves /status
• Tests that start() terminates the process when connection times out
• Mocks sleep to speed up test execution

py/test/unit/selenium/webdriver/common/service_tests.py

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (1)

1. Cleanup shutdown lacks timeout 🐞 Bug ☼ Reliability

Description

When Service.start() fails it now calls stop(), which calls send_remote_shutdown_command();
that method uses urlopen() without a timeout, so start() can hang indefinitely during cleanup if
the process accepts a connection but doesn’t respond to /shutdown.

Code

py/selenium/webdriver/common/service.py[R121-126]

+        except BaseException:
+            try:
+                self.stop()
+            except Exception:
+                logger.error("Error stopping service after a failed start.", exc_info=True)
+            raise

Evidence

The new start() exception handler triggers stop() on startup failures. stop() calls
send_remote_shutdown_command(), which currently uses urlopen() without a timeout, unlike the
/status probe that sets timeout=1, making an indefinite hang possible in the new cleanup path.

py/selenium/webdriver/common/service.py[111-126]
py/selenium/webdriver/common/service.py[164-170]
py/selenium/webdriver/common/service.py[144-155]
py/selenium/webdriver/common/utils.py[135-169]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`send_remote_shutdown_command()` calls `urllib.request.urlopen()` without a timeout. Because `start()` now calls `stop()` on failure, a hung or partially-responsive driver can cause `start()` to block indefinitely during cleanup.

### Issue Context
- `/status` probing uses an explicit timeout, but `/shutdown` does not.
- This code path is now exercised on start failures due to the new exception handler.

### Fix Focus Areas
- py/selenium/webdriver/common/service.py[121-126]
- py/selenium/webdriver/common/service.py[144-155]
- py/selenium/webdriver/common/service.py[164-170]

### Suggested fix
- Add a timeout to the shutdown request, e.g. `request.urlopen(f"{self.service_url}/shutdown", timeout=1)` (or a small configurable value).
- Consider using the same proxy-disabled opener pattern used by `utils.is_url_connectable()` to keep localhost behavior consistent.
- Ensure timeouts/errors are caught and fall through to `_terminate_process()`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Test lacks cleanup guard 🐞 Bug ☼ Reliability

Description

The new unit test spawns a 30s-sleeping subprocess and does not guarantee cleanup if the test fails
(e.g., if start() unexpectedly succeeds or the final assertion fails). This can leave stray Python
processes and make subsequent tests flaky.

Code

py/test/unit/selenium/webdriver/common/service_tests.py[R34-42]

+def test_start_terminates_process_when_never_connectable(monkeypatch):
+    monkeypatch.setattr("selenium.webdriver.common.service.sleep", lambda _: None)
+
+    service = _UnreachableService(executable_path=sys.executable, port=utils.free_port())
+
+    with pytest.raises(WebDriverException):
+        service.start()
+
+    assert service.process.poll() is not None

Evidence

The test creates an _UnreachableService that runs a 30-second sleep subprocess and never calls
service.stop() in a finally/finalizer, so a failing assertion or unexpected behavior can leave
the process alive.

py/test/unit/selenium/webdriver/common/service_tests.py[27-42]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The test starts a real subprocess (`sys.executable -c "...sleep(30)"`) but does not add a safety cleanup to terminate it if the test fails.

### Issue Context
Even though the expected passing path should terminate the process, failures/regressions can leave the child process running and interfere with the rest of the test suite.

### Fix Focus Areas
- py/test/unit/selenium/webdriver/common/service_tests.py[34-42]

### Suggested fix
Add a `try/finally` (or `request.addfinalizer(service.stop)`) to always call `service.stop()` at the end of the test. Optionally, to reduce test flakiness/overhead, consider monkeypatching `Service.is_connectable` to always return `False` rather than performing 70 real socket connection attempts.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Interrupt leaks driver process 🐞 Bug ☼ Reliability

Description

The new cleanup in Service.start() only runs for Exception, so KeyboardInterrupt/SystemExit during
start() bypasses stop() and can leave the spawned driver process running. This can orphan processes
during Ctrl-C or interpreter shutdown while starting.

Code

py/selenium/webdriver/common/service.py[R121-123]

+        except Exception:
+            self.stop()
+            raise

Evidence

start() now catches Exception (not BaseException) around the connect loop, so interrupts will
not trigger the new self.stop() cleanup path.

py/selenium/webdriver/common/service.py[111-123]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Service.start()` cleanup is under `except Exception`, which does not catch `KeyboardInterrupt`/`SystemExit` (both inherit from `BaseException`). This leaves the started child process running if the user interrupts during startup.

### Issue Context
The PR’s goal is to prevent orphaned driver processes on startup failure; user interrupts are another common startup-failure path.

### Fix Focus Areas
- py/selenium/webdriver/common/service.py[111-123]

### Suggested fix
Change the handler to `except BaseException:` (or use a `try: ... except BaseException: ...` pattern) so `stop()` executes on interrupts as well, while still re-raising the original exception afterwards. Combine with suppressing `stop()` errors so the original interrupt/failure propagates.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. start() masks original error 📘 Rule violation ☼ Reliability

Description

Service.start() calls self.stop() inside an except Exception path without guarding against
failures in stop(). If stop() raises during cleanup (e.g., while closing log_output via
log_output.close() or os.close() on an invalid/int fd), it can mask the original
connection/startup exception and lead to misleading errors or incomplete teardown.

Code

py/selenium/webdriver/common/service.py[R121-123]

+        except Exception:
+            self.stop()
+            raise

Evidence

PR Compliance ID 14 requires exception-safe cleanup paths, but the new except block in start()
unconditionally calls self.stop() and then intends to re-raise the original failure; because
stop() itself can raise during resource cleanup (notably when closing log_output, including
os.close(self.log_output) when log_output is an int or otherwise invalid), that cleanup
exception can override the original startup/connect error (e.g., a WebDriverException from the
connection loop) and violate exception-safe teardown expectations.

py/selenium/webdriver/common/service.py[121-123]
py/selenium/webdriver/common/service.py[153-160]
py/selenium/webdriver/common/service.py[99-123]
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Service.start()` triggers cleanup by calling `self.stop()` inside an exception handler, but `stop()` is not exception-safe and may itself raise (e.g., during `log_output.close()` or `os.close()` on an invalid/int fd). This can replace the original startup/connection exception with a cleanup failure and may also abort cleanup early, yielding misleading errors and breaking exception-safe teardown expectations.

## Issue Context
The new behavior appears intended to ensure cleanup when startup fails (per PR Compliance ID 14), but cleanup code should not override the primary failure cause from `start()` (including connection loop failures like `WebDriverException`). `Service.stop()` performs resource cleanup that can raise, particularly when closing `log_output` (including the `os.close(self.log_output)` case when `log_output` is an `int`). Because this call site is in the error path of `start()`, masking the original exception is especially harmful.

## Fix Focus Areas
- py/selenium/webdriver/common/service.py[111-123]
- py/selenium/webdriver/common/service.py[121-123]
- py/selenium/webdriver/common/service.py[153-168]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Previous review results

Review updated until commit 60fc0c7

Results up to commit 2d2f70f

🐞 Bugs (2) 📘 Rule violations (1) 📎 Requirement gaps (0) 🎨 UX issues (0)

1. start() masks original error 📘 Rule violation ☼ Reliability

Description

Service.start() calls self.stop() inside an except Exception path without guarding against
failures in stop(). If stop() raises during cleanup (e.g., while closing log_output via
log_output.close() or os.close() on an invalid/int fd), it can mask the original
connection/startup exception and lead to misleading errors or incomplete teardown.

Code

py/selenium/webdriver/common/service.py[R121-123]

+        except Exception:
+            self.stop()
+            raise

Evidence

PR Compliance ID 14 requires exception-safe cleanup paths, but the new except block in start()
unconditionally calls self.stop() and then intends to re-raise the original failure; because
stop() itself can raise during resource cleanup (notably when closing log_output, including
os.close(self.log_output) when log_output is an int or otherwise invalid), that cleanup
exception can override the original startup/connect error (e.g., a WebDriverException from the
connection loop) and violate exception-safe teardown expectations.

py/selenium/webdriver/common/service.py[121-123]
py/selenium/webdriver/common/service.py[153-160]
py/selenium/webdriver/common/service.py[99-123]
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Service.start()` triggers cleanup by calling `self.stop()` inside an exception handler, but `stop()` is not exception-safe and may itself raise (e.g., during `log_output.close()` or `os.close()` on an invalid/int fd). This can replace the original startup/connection exception with a cleanup failure and may also abort cleanup early, yielding misleading errors and breaking exception-safe teardown expectations.

## Issue Context
The new behavior appears intended to ensure cleanup when startup fails (per PR Compliance ID 14), but cleanup code should not override the primary failure cause from `start()` (including connection loop failures like `WebDriverException`). `Service.stop()` performs resource cleanup that can raise, particularly when closing `log_output` (including the `os.close(self.log_output)` case when `log_output` is an `int`). Because this call site is in the error path of `start()`, masking the original exception is especially harmful.

## Fix Focus Areas
- py/selenium/webdriver/common/service.py[111-123]
- py/selenium/webdriver/common/service.py[121-123]
- py/selenium/webdriver/common/service.py[153-168]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Interrupt leaks driver process 🐞 Bug ☼ Reliability

Description

The new cleanup in Service.start() only runs for Exception, so KeyboardInterrupt/SystemExit during
start() bypasses stop() and can leave the spawned driver process running. This can orphan processes
during Ctrl-C or interpreter shutdown while starting.

Code

py/selenium/webdriver/common/service.py[R121-123]

+        except Exception:
+            self.stop()
+            raise

Evidence

start() now catches Exception (not BaseException) around the connect loop, so interrupts will
not trigger the new self.stop() cleanup path.

py/selenium/webdriver/common/service.py[111-123]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Service.start()` cleanup is under `except Exception`, which does not catch `KeyboardInterrupt`/`SystemExit` (both inherit from `BaseException`). This leaves the started child process running if the user interrupts during startup.

### Issue Context
The PR’s goal is to prevent orphaned driver processes on startup failure; user interrupts are another common startup-failure path.

### Fix Focus Areas
- py/selenium/webdriver/common/service.py[111-123]

### Suggested fix
Change the handler to `except BaseException:` (or use a `try: ... except BaseException: ...` pattern) so `stop()` executes on interrupts as well, while still re-raising the original exception afterwards. Combine with suppressing `stop()` errors so the original interrupt/failure propagates.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Test lacks cleanup guard 🐞 Bug ☼ Reliability

Description

The new unit test spawns a 30s-sleeping subprocess and does not guarantee cleanup if the test fails
(e.g., if start() unexpectedly succeeds or the final assertion fails). This can leave stray Python
processes and make subsequent tests flaky.

Code

py/test/unit/selenium/webdriver/common/service_tests.py[R34-42]

+def test_start_terminates_process_when_never_connectable(monkeypatch):
+    monkeypatch.setattr("selenium.webdriver.common.service.sleep", lambda _: None)
+
+    service = _UnreachableService(executable_path=sys.executable, port=utils.free_port())
+
+    with pytest.raises(WebDriverException):
+        service.start()
+
+    assert service.process.poll() is not None

Evidence

The test creates an _UnreachableService that runs a 30-second sleep subprocess and never calls
service.stop() in a finally/finalizer, so a failing assertion or unexpected behavior can leave
the process alive.

py/test/unit/selenium/webdriver/common/service_tests.py[27-42]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The test starts a real subprocess (`sys.executable -c "...sleep(30)"`) but does not add a safety cleanup to terminate it if the test fails.

### Issue Context
Even though the expected passing path should terminate the process, failures/regressions can leave the child process running and interfere with the rest of the test suite.

### Fix Focus Areas
- py/test/unit/selenium/webdriver/common/service_tests.py[34-42]

### Suggested fix
Add a `try/finally` (or `request.addfinalizer(service.stop)`) to always call `service.stop()` at the end of the test. Optionally, to reduce test flakiness/overhead, consider monkeypatching `Service.is_connectable` to always return `False` rather than performing 70 real socket connection attempts.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Results up to commit 1fcc88a

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0)

1. Cleanup shutdown lacks timeout 🐞 Bug ☼ Reliability

Description

When Service.start() fails it now calls stop(), which calls send_remote_shutdown_command();
that method uses urlopen() without a timeout, so start() can hang indefinitely during cleanup if
the process accepts a connection but doesn’t respond to /shutdown.

Code

py/selenium/webdriver/common/service.py[R121-126]

+        except BaseException:
+            try:
+                self.stop()
+            except Exception:
+                logger.error("Error stopping service after a failed start.", exc_info=True)
+            raise

Evidence

The new start() exception handler triggers stop() on startup failures. stop() calls
send_remote_shutdown_command(), which currently uses urlopen() without a timeout, unlike the
/status probe that sets timeout=1, making an indefinite hang possible in the new cleanup path.

py/selenium/webdriver/common/service.py[111-126]
py/selenium/webdriver/common/service.py[164-170]
py/selenium/webdriver/common/service.py[144-155]
py/selenium/webdriver/common/utils.py[135-169]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`send_remote_shutdown_command()` calls `urllib.request.urlopen()` without a timeout. Because `start()` now calls `stop()` on failure, a hung or partially-responsive driver can cause `start()` to block indefinitely during cleanup.

### Issue Context
- `/status` probing uses an explicit timeout, but `/shutdown` does not.
- This code path is now exercised on start failures due to the new exception handler.

### Fix Focus Areas
- py/selenium/webdriver/common/service.py[121-126]
- py/selenium/webdriver/common/service.py[144-155]
- py/selenium/webdriver/common/service.py[164-170]

### Suggested fix
- Add a timeout to the shutdown request, e.g. `request.urlopen(f"{self.service_url}/shutdown", timeout=1)` (or a small configurable value).
- Consider using the same proxy-disabled opener pattern used by `utils.is_url_connectable()` to keep localhost behavior consistent.
- Ensure timeouts/errors are caught and fall through to `_terminate_process()`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-code-review]

Code review by qodo was updated up to the latest commit 1fcc88a

[qodo-code-review]

Code review by qodo was updated up to the latest commit 60fc0c7