[py] Ensure driver service subprocess resources are cleaned up

[cgoldberg]

🔗 Related Issues

Fixes #17887

💥 What does this PR do?

This PR refactors the driver service lifecycle stop/cleanup to manage subprocess resources (stdin/sdtdout/stderr), so they are cleaned up even if the child subprocess has already exited.

Previously, stop() only performed subprocess cleanup when the child was still running. If the process somehow exited or was terminated externally before stop() was called, the parent-side pipe handles were never closed.

This change makes stop() always invoke the subprocess cleanup routine. The cleanup routine now only attempts to terminate the process if it is still running, but always closes the parent-side streams afterward. This prevents resource leaks while preserving the existing shutdown behavior for running processes.

🤖 AI assistance

• No substantial AI assistance used
• AI assisted (complete below)
  • Tool(s):
  • What was generated:
  • I reviewed all AI output and can explain the change

💡 Additional Considerations

🔄 Types of changes

• Bug fix (backwards compatible)

[qodo-code-review]

PR Summary by Qodo

Ensure driver service stop() always closes subprocess streams

🐞 Bug fix 🕐 10-20 Minutes

AI Description

• Always run subprocess cleanup in Service.stop(), even if the child already exited
• Only attempt termination when the subprocess is still running, preserving current shutdown
 behavior
• Close stdin/stdout/stderr reliably to prevent parent-side file descriptor leaks

Diagram

graph TD
  SVC["Service class"] --> STOP["stop()"] --> TERM["_terminate_process()"] --> CHILD(["Child subprocess"])
  STOP --> LOG[("log_output fd")]
  TERM --> STREAMS[("stdin/stdout/stderr")]

  subgraph Legend
    direction LR
    _fn["Method"] ~~~ _proc(["Subprocess"]) ~~~ _res[("FDs/Streams")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Use context-managed stream ownership (ExitStack)

• ➕ Centralizes stream acquisition/cleanup, reducing the chance of future leaks
• ➕ Makes lifecycle responsibilities explicit and testable
• ➖ More invasive refactor across service startup/pipe wiring
• ➖ Harder to adopt without broader API changes

2. Rely on Popen(close_fds=True) / avoid PIPEs where possible

• ➕ Reduces likelihood of FD leaks by limiting inherited handles
• ➕ May simplify resource management in some environments
• ➖ Does not eliminate the need to close pipes explicitly when PIPEs are used
• ➖ Potential behavior changes for consumers that depend on captured output

Recommendation: The PR’s approach is the right minimal fix: always invoke the cleanup routine from stop(), and gate only the termination logic on process liveness while unconditionally closing stdin/stdout/stderr. Alternatives add complexity or don’t fully address parent-side pipe closure.

Files changed (1) +20 / -20

Bug fix (1) +20 / -20

service.pyAlways cleanup subprocess streams even when service already exited +20/-20

Always cleanup subprocess streams even when service already exited

• Adjusts Service.stop() to always run subprocess cleanup when a process exists, rather than only when it is still running. Updates _terminate_process() to terminate/wait/kill only if the child is alive, but always closes stdin/stdout/stderr to prevent FD leaks.

py/selenium/webdriver/common/service.py

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 📜 Skill insights (0)

1. Waits before closing pipes ✗ Dismissed 🐞 Bug ☼ Reliability

Description

Service._terminate_process() now calls process.wait(60) before closing stdout/stderr, which
can delay shutdown (up to the full timeout) when the service was started with log_output=PIPE and
the child does not promptly exit on SIGTERM while writing to those pipes. Previously, closing the
streams occurred before waiting, reducing the chance that undrained pipes contribute to a prolonged
teardown.

Code

py/selenium/webdriver/common/service.py[R181-185]

+            if self.process.poll() is None:
+                self.process.terminate()
+                try:
+                    self.process.wait(60)
+                except subprocess.TimeoutExpired:

Evidence

The codebase allows stdout/stderr to be configured as PIPE via log_output, but does not
provide any consumer that drains these pipes. With this PR, _terminate_process() blocks in
wait(60) before closing those streams, which can prolong teardown in the piped-output
configuration when the child does not promptly terminate on SIGTERM.

py/selenium/webdriver/common/service.py[52-72]
py/selenium/webdriver/common/service.py[156-171]
py/selenium/webdriver/common/service.py[173-199]
py/selenium/webdriver/common/service.py[214-240]

Agent prompt

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

### Issue description
`_terminate_process()` now waits for the child to exit *before* closing `stdin/stdout/stderr`. If `stdout/stderr` are set to `subprocess.PIPE` (via `log_output=PIPE`), Selenium does not drain these streams anywhere, and a child that delays/ignores SIGTERM while still writing can prolong shutdown until the 60s timeout.

### Issue Context
- `Service._start_process()` wires `stdout` and `stderr` to `self.log_output`, which may be `PIPE`.
- `Service.stop()` always calls `_terminate_process()` now.

### Fix Focus Areas
- py/selenium/webdriver/common/service.py[173-200]
- py/selenium/webdriver/common/service.py[214-240]

### Suggested fix approach
One of:
1) Close/drain streams before `wait()`:
  - After `terminate()`, immediately close `stdin/stdout/stderr` (or at least `stdout/stderr` when they are pipes) before calling `wait(60)`.

2) Use `communicate()` to drain pipes safely:
  - After `terminate()`, call `self.process.communicate(timeout=60)` (ignore returned output), then close streams in a `finally`.
  - Keep the existing timeout/kill escalation logic.

Ensure the pipe-closing/draining happens before blocking on the process exiting when `stdout/stderr` are piped.

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

---

2. stop() cleanup lacks test 📘 Rule violation ▣ Testability

Description

The PR changes Service.stop()/_terminate_process() to always close subprocess streams even when
the child has already exited, but there is no regression test exercising this new cleanup behavior.
Without a test, future changes could reintroduce the file-descriptor leak or error-on-stop scenario
unnoticed.

Code

py/selenium/webdriver/common/service.py[R169-171]

+                        pass
            finally:
                self._terminate_process()

Evidence

PR changes now always call _terminate_process() from stop() and _terminate_process() closes
stdin/stdout/stderr regardless of whether the subprocess is still running, which is a
behavioral bug fix that should be covered by a regression test. The only existing unit test in
service_tests.py exercises start() failure and does not assert any stop() cleanup behavior.

Rule 389273: Require tests for all new functionality and bug fixes
py/selenium/webdriver/common/service.py[156-200]
py/test/unit/selenium/webdriver/common/service_tests.py[27-45]

Agent prompt

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

## Issue description
A bug fix changed `Service.stop()` / `_terminate_process()` to always perform subprocess stream cleanup, but there is no regression test that verifies streams are closed when the subprocess has already exited.

## Issue Context
The change is intended to prevent FD/handle leaks when the child process exits before `stop()` is called. The existing unit test for `Service` does not assert any `stop()` cleanup behavior.

## Fix Focus Areas
- py/selenium/webdriver/common/service.py[156-200]
- py/test/unit/selenium/webdriver/common/service_tests.py[27-45]

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

---

Context used

✅ Compliance rules (platform): 17 rules

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}