[rb] Release a stopped service instead of holding it until the process exits

[ikraamg]

🔗 Related Issues

None open that I could find.

💥 What does this PR do?

ServiceManager#start registers an at_exit block per service, and the block captures the service, so every service ever started stays reachable for the life of the process along with its ChildProcess. Stopping the service does not release it, so the growth is unbounded in a process that starts many drivers: a suite that starts one per spec file, or a pool that recycles browsers.

I found it running the Firefox WebDriver BiDi render pipeline for trmnl.com in production.

Starting 500 services and stopping every one of them, then running GC:

before   live ServiceManagers: 500
after    live ServiceManagers: 1

One retained ServiceManager holds 720 bytes across 12 objects once you follow the whole reachable graph, with a real ChildProcess attached, after a normal stop.

Services are now tracked in one list with a single exit hook per process, and #stop removes the service from it. A service that is still running is still held, which is what lets the exit hook stop it.

• adds ServiceManager.track, .untrack and .stop_running
• registers the exit hook on first start rather than at load, so a service started in a forked child is still stopped when that child exits
• clears the list when the pid changes, so a child does not stop services belonging to its parent
• adds rb/spec/unit/selenium/webdriver/common/service_manager_spec.rb

🔧 Implementation Notes

The per-service at_exit was doing two jobs: making sure a running service gets stopped, and, as a side effect, keeping the service alive to do it. Only the first is wanted. A single class-level list gives the exit hook everything it needs to stop, and #stop removing itself from that list is what lets a stopped service be collected.

Alternatives I considered:

• a WeakRef or an ObjectSpace::WeakMap of services. A stopped service would be collected, but so could a running one before the process exits, which is the case the hook exists for.
• unregistering the individual at_exit block on stop. Ruby has no API for that.
• leaving start alone and having stop clear the service's own state so the retained object is small. It shrinks the leak instead of removing it, and still grows without bound.

Registering the hook lazily on the first start is the part I would look at hardest. Registering at load would mean a process that forks after loading selenium-webdriver has the hook only in the parent, so a driver started in the child is never stopped. Arming it inside claim_for_this_process, which also clears the list when the pid has changed, gives the child its own hook and its own list. I verified this with a real fork: with a service started in the parent and another in a forked child, the child tracks only its own and the parent is unaffected.

The list is guarded by a mutex because drivers are commonly started from several threads, and stop_running iterates a copy so a service stopping itself during the walk cannot mutate the list underneath it.

🤖 AI assistance

• No substantial AI assistance used
• AI assisted (complete below)
  • Tool(s): Claude Code (Claude Opus)
  • What was generated: a first draft of the tracking methods and the spec. The leak, the retained-size measurement and the fork behaviour are mine, and the fork case was found by testing rather than by the draft.
  • I reviewed all AI output and can explain the change

💡 Additional Considerations

• stop_running is public because the exit hook calls it, and it is useful to a pool that wants to shut everything down; happy to make it private if you would rather not add surface.
• No behaviour change for the ordinary case: services still get stopped at exit, and a service that is already stopped is simply no longer stopped twice.

🔄 Types of changes

• Bug fix (backwards compatible)

[qodo-code-review]

PR Summary by Qodo

Release stopped ServiceManagers by tracking running services with a single exit hook

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Replace per-service at_exit hooks with process-level tracking of running services.
• Untrack services on stop to prevent unbounded retention of ChildProcess objects.
• Add unit coverage for tracking/untracking and exit-time stop behavior.

Diagram

graph TD
  A["ServiceManager instance"] --> B["ServiceManager.track"] --> C[("@running list")]
  B --> D["Platform.exit_hook"] --> E["stop_running"] --> F["ServiceManager#stop"]
  F --> G["ServiceManager.untrack"] --> C
  B --> H["PID change clears list"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. WeakRef-based running registry

• ➕ Avoids retaining managers even if untrack is missed
• ➕ Still allows an exit hook to iterate live references
• ➖ WeakRef/ObjectSpace behavior can be surprising across Ruby versions
• ➖ Exit-time behavior becomes less deterministic under GC pressure

2. Keep per-service exit hooks but avoid capturing manager

• ➕ Minimal structural change; preserves existing mental model
• ➕ Could reduce need for shared global state/mutex
• ➖ Harder to implement safely (must not capture the object; needs indirection)
• ➖ More hooks to manage; higher overhead and complexity on repeated starts

Recommendation: The PR’s single per-process registry + single exit hook is the most robust and predictable approach: it prevents unbounded retention by design (untrack on stop), keeps exit-time cleanup reliable (still holds running services), and explicitly handles fork semantics by resetting state on PID changes. The WeakRef/finalizer-style alternatives add nondeterminism and VM/version risk without clear benefit here.

Files changed (3) +131 / -1

Bug fix (1) +37 / -1

service_manager.rbTrack running services and untrack on stop to avoid at_exit retention +37/-1

Track running services and untrack on stop to avoid at_exit retention

• Introduces a class-level registry of running ServiceManager instances guarded by a mutex, with a single exit hook per process/PID. Replaces per-instance at_exit registration in #start with .track, and ensures #stop calls .untrack so stopped managers can be garbage collected. Adds PID-change handling to clear inherited state after fork to avoid stopping parent services from a child process.

rb/lib/selenium/webdriver/common/service_manager.rb

Refactor (1) +12 / -0

service_manager.rbsAdd RBS signatures for service tracking and exit-stop helpers +12/-0

Add RBS signatures for service tracking and exit-stop helpers

• Declares new class instance variables (@running, @running_mutex, @exit_hook_pid) and class methods (.track, .untrack, .stop_running) to match the updated implementation. Keeps type surface aligned for Steep/RBS consumers.

rb/sig/lib/selenium/webdriver/common/service_manager.rbs

Tests (1) +82 / -0

service_manager_spec.rbAdd unit tests for tracking/untracking and stop_running behavior +82/-0

Add unit tests for tracking/untracking and stop_running behavior

• Adds specs verifying that starting a manager adds it to the running registry, stopping removes it, and stop_running triggers stop behavior for still-tracked services. Also asserts that Platform.exit_hook is registered at most once even when multiple services start.

rb/spec/unit/selenium/webdriver/common/service_manager_spec.rb

[qodo-code-review]

Code Review by Qodo

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

1. claim_for_this_process public in RBS ✓ Resolved 📘 Rule violation ≡ Correctness

Description

ServiceManager.claim_for_this_process is declared as a public class method in the .rbs, but the
Ruby implementation defines it as a private singleton method. This mismatch makes the published type
surface diverge from the actual non-public runtime API and can mislead typed callers/tooling into
making calls that will raise NoMethodError.

Code

rb/sig/lib/selenium/webdriver/common/service_manager.rbs[R58-59]

+      def self.claim_for_this_process: () -> void
+

Evidence

The compliance requirement is that .rbs signatures reflect the implementation’s public API, yet
the Ruby code defines claim_for_this_process inside class << self under an explicit private
section, making it non-callable from outside. In contrast, the .rbs declares it as a normal `def
self.claim_for_this_process`, which implies public visibility, so static type-checking would allow
external calls even though Ruby will reject them at runtime due to private method visibility.

Rule 389239: Keep Ruby .rbs signatures in sync with public API changes
rb/sig/lib/selenium/webdriver/common/service_manager.rbs[58-59]
rb/lib/selenium/webdriver/common/service_manager.rb[65-75]
rb/sig/lib/selenium/webdriver/common/service_manager.rbs[52-60]

Agent prompt

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

## Issue description
`ServiceManager.claim_for_this_process` is private in the Ruby implementation but is currently declared as a public class method in the `.rbs`, exposing a non-public API in the type surface and enabling type-checked external calls that can fail at runtime.

## Issue Context
The Ruby implementation defines `claim_for_this_process` under `class << self` and marks it `private`. The RBS should reflect this visibility (e.g., `private def self.claim_for_this_process: ...`), using existing repo patterns such as `private`/`public` blocks around class methods if needed to ensure subsequent method visibility is correct.

## Fix Focus Areas
- rb/sig/lib/selenium/webdriver/common/service_manager.rbs[58-59]
- rb/lib/selenium/webdriver/common/service_manager.rb[65-75]

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

---

2. Fork stops parent services ✓ Resolved 🐞 Bug ≡ Correctness

Description

ServiceManager.stop_running stops all managers in @running without checking that the list
belongs to the current PID, so a forked child that calls stop_running before calling track can
stop services started in its parent. This violates the method’s “in this process” semantics and can
unexpectedly kill the parent’s driver services.

Code

rb/lib/selenium/webdriver/common/service_manager.rb[R42-44]

+        def stop_running
+          @running_mutex.synchronize { @running.dup }.each(&:stop)
+        end

Evidence

stop_running iterates and stops everything in @running without PID validation, while PID-based
cleanup of inherited state only happens inside track. Platform.exit_hook already PID-guards exit
hooks, so the remaining hazard is direct stop_running invocation in a forked child before track
clears inherited entries.

rb/lib/selenium/webdriver/common/service_manager.rb[42-60]
rb/lib/selenium/webdriver/common/platform.rb[147-151]

Agent prompt

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

### Issue description
`ServiceManager.stop_running` uses the inherited `@running` list as-is. After a `fork`, the child inherits the parent’s `@running` entries, and if the child calls `stop_running` before calling `track`, it can stop services that the parent process still needs.

### Issue Context
`Platform.exit_hook` already prevents the parent’s exit hook from running in the child, but `stop_running` is callable directly and has no PID isolation.

### Fix Focus Areas
- rb/lib/selenium/webdriver/common/service_manager.rb[42-66]

### Suggested fix
Add a PID check similar to `track` at the start of `stop_running` (and potentially a shared helper), e.g. inside the mutex:
- if `@exit_hook_pid != Process.pid`, clear `@running` (inherited entries) and return without stopping anything (or reset `@exit_hook_pid`/state appropriately).
This ensures `stop_running` can’t act on services that were tracked in a different process.

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

---

3. RSpec mocks in new spec 📘 Rule violation ▣ Testability

Description

The new unit spec relies on RSpec mocking (instance_double, allow(...).to receive_messages)
rather than real or contract-driven integrations, which risks tests diverging from real interfaces
over time.

Code

rb/spec/unit/selenium/webdriver/common/service_manager_spec.rb[R31-34]

+        config = instance_double(Service, executable_path: '/path/to/service', port: port,
+                                          log: nil, args: [], shutdown_supported: true)
+        described_class.new(config).tap do |service_manager|
+          allow(service_manager).to receive_messages(socket_lock: yielding_lock, find_free_port: nil,

Evidence

PR Compliance ID 389270 disallows using mocking frameworks in tests unless the mock is
contract-driven. The added spec constructs dependencies via instance_double and stubs multiple
methods with allow(...).to receive_messages, which is direct use of RSpec mocking rather than a
real or contract-verified integration.

Rule 389270: Avoid mocks in tests; use real or contract-driven integrations
rb/spec/unit/selenium/webdriver/common/service_manager_spec.rb[31-36]

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 newly added unit spec uses RSpec mocks/doubles (e.g., `instance_double` and `allow(...).to receive_messages`) instead of using real implementations or simple in-memory fakes.

## Issue Context
Per compliance guidance, mocking frameworks should be avoided unless backed by a machine-checked contract; otherwise, tests can drift from the real API.

## Fix Focus Areas
- rb/spec/unit/selenium/webdriver/common/service_manager_spec.rb[31-42]

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

---

4. Exit-hook spec state leakage ✓ Resolved 🐞 Bug ☼ Reliability

Description

The new .track spec can pass without actually asserting exit-hook registration because
@exit_hook_pid persists across examples, causing track to skip calling Platform.exit_hook in
later examples. This makes the test order-dependent and reduces its ability to detect regressions in
exit-hook registration.

Code

rb/spec/unit/selenium/webdriver/common/service_manager_spec.rb[R56-58]

+          2.times { build_manager.start }
+
+          expect(Platform).to have_received(:exit_hook).at_most(:once)

Evidence

track conditionally registers the exit hook based on @exit_hook_pid, but the spec only calls
stop_running in cleanup and never resets @exit_hook_pid, so later examples may not trigger
Platform.exit_hook at all while still satisfying at_most(:once).

rb/lib/selenium/webdriver/common/service_manager.rb[51-57]
rb/spec/unit/selenium/webdriver/common/service_manager_spec.rb[44-59]

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 spec for “registers a single exit hook…” uses `have_received(:exit_hook).at_most(:once)`, but `ServiceManager.track` may not call `Platform.exit_hook` at all if a prior example already set `@exit_hook_pid`. Because the spec doesn’t reset class-level state, the assertion can pass regardless.

### Issue Context
`ServiceManager.track` only calls `Platform.exit_hook` when `@exit_hook_pid != Process.pid`, and the current spec cleanup stops services but does not reset `@exit_hook_pid`.

### Fix Focus Areas
- rb/spec/unit/selenium/webdriver/common/service_manager_spec.rb[44-59]
- rb/lib/selenium/webdriver/common/service_manager.rb[51-60]

### Suggested fix
In the spec, reset `described_class` class-instance state in a `before`/`after` hook (e.g., set `@exit_hook_pid` to `nil` and `@running` to `[]`), and strengthen the expectation to assert the call happens when expected (e.g., `once` for first registration, and still `once` after multiple starts within the same example).

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

---

Context used

✅ Compliance rules (platform): 19 rules

Tip of the day

💡 Did you know, you can reply 'qodo' on any finding to push back, ask questions, or dig deeper

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous review results

Review updated until commit bf2a605 ⚖️ Balanced

Results up to commit 5f49e46 ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (1) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. RSpec mocks in new spec 📘 Rule violation ▣ Testability

Description

The new unit spec relies on RSpec mocking (instance_double, allow(...).to receive_messages)
rather than real or contract-driven integrations, which risks tests diverging from real interfaces
over time.

Code

rb/spec/unit/selenium/webdriver/common/service_manager_spec.rb[R31-34]

+        config = instance_double(Service, executable_path: '/path/to/service', port: port,
+                                          log: nil, args: [], shutdown_supported: true)
+        described_class.new(config).tap do |service_manager|
+          allow(service_manager).to receive_messages(socket_lock: yielding_lock, find_free_port: nil,

Evidence

PR Compliance ID 389270 disallows using mocking frameworks in tests unless the mock is
contract-driven. The added spec constructs dependencies via instance_double and stubs multiple
methods with allow(...).to receive_messages, which is direct use of RSpec mocking rather than a
real or contract-verified integration.

Rule 389270: Avoid mocks in tests; use real or contract-driven integrations
rb/spec/unit/selenium/webdriver/common/service_manager_spec.rb[31-36]

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 newly added unit spec uses RSpec mocks/doubles (e.g., `instance_double` and `allow(...).to receive_messages`) instead of using real implementations or simple in-memory fakes.

## Issue Context
Per compliance guidance, mocking frameworks should be avoided unless backed by a machine-checked contract; otherwise, tests can drift from the real API.

## Fix Focus Areas
- rb/spec/unit/selenium/webdriver/common/service_manager_spec.rb[31-42]

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

---

2. Fork stops parent services ✓ Resolved 🐞 Bug ≡ Correctness

Description

ServiceManager.stop_running stops all managers in @running without checking that the list
belongs to the current PID, so a forked child that calls stop_running before calling track can
stop services started in its parent. This violates the method’s “in this process” semantics and can
unexpectedly kill the parent’s driver services.

Code

rb/lib/selenium/webdriver/common/service_manager.rb[R42-44]

+        def stop_running
+          @running_mutex.synchronize { @running.dup }.each(&:stop)
+        end

Evidence

stop_running iterates and stops everything in @running without PID validation, while PID-based
cleanup of inherited state only happens inside track. Platform.exit_hook already PID-guards exit
hooks, so the remaining hazard is direct stop_running invocation in a forked child before track
clears inherited entries.

rb/lib/selenium/webdriver/common/service_manager.rb[42-60]
rb/lib/selenium/webdriver/common/platform.rb[147-151]

Agent prompt

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

### Issue description
`ServiceManager.stop_running` uses the inherited `@running` list as-is. After a `fork`, the child inherits the parent’s `@running` entries, and if the child calls `stop_running` before calling `track`, it can stop services that the parent process still needs.

### Issue Context
`Platform.exit_hook` already prevents the parent’s exit hook from running in the child, but `stop_running` is callable directly and has no PID isolation.

### Fix Focus Areas
- rb/lib/selenium/webdriver/common/service_manager.rb[42-66]

### Suggested fix
Add a PID check similar to `track` at the start of `stop_running` (and potentially a shared helper), e.g. inside the mutex:
- if `@exit_hook_pid != Process.pid`, clear `@running` (inherited entries) and return without stopping anything (or reset `@exit_hook_pid`/state appropriately).
This ensures `stop_running` can’t act on services that were tracked in a different process.

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

---

3. Exit-hook spec state leakage ✓ Resolved 🐞 Bug ☼ Reliability

Description

The new .track spec can pass without actually asserting exit-hook registration because
@exit_hook_pid persists across examples, causing track to skip calling Platform.exit_hook in
later examples. This makes the test order-dependent and reduces its ability to detect regressions in
exit-hook registration.

Code

rb/spec/unit/selenium/webdriver/common/service_manager_spec.rb[R56-58]

+          2.times { build_manager.start }
+
+          expect(Platform).to have_received(:exit_hook).at_most(:once)

Evidence

track conditionally registers the exit hook based on @exit_hook_pid, but the spec only calls
stop_running in cleanup and never resets @exit_hook_pid, so later examples may not trigger
Platform.exit_hook at all while still satisfying at_most(:once).

rb/lib/selenium/webdriver/common/service_manager.rb[51-57]
rb/spec/unit/selenium/webdriver/common/service_manager_spec.rb[44-59]

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 spec for “registers a single exit hook…” uses `have_received(:exit_hook).at_most(:once)`, but `ServiceManager.track` may not call `Platform.exit_hook` at all if a prior example already set `@exit_hook_pid`. Because the spec doesn’t reset class-level state, the assertion can pass regardless.

### Issue Context
`ServiceManager.track` only calls `Platform.exit_hook` when `@exit_hook_pid != Process.pid`, and the current spec cleanup stops services but does not reset `@exit_hook_pid`.

### Fix Focus Areas
- rb/spec/unit/selenium/webdriver/common/service_manager_spec.rb[44-59]
- rb/lib/selenium/webdriver/common/service_manager.rb[51-60]

### Suggested fix
In the spec, reset `described_class` class-instance state in a `before`/`after` hook (e.g., set `@exit_hook_pid` to `nil` and `@running` to `[]`), and strengthen the expectation to assert the call happens when expected (e.g., `once` for first registration, and still `once` after multiple starts within the same example).

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

---

Results up to commit b6b834d ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. claim_for_this_process public in RBS ✓ Resolved 📘 Rule violation ≡ Correctness

Description

ServiceManager.claim_for_this_process is declared as a public class method in the .rbs, but the
Ruby implementation defines it as a private singleton method. This mismatch makes the published type
surface diverge from the actual non-public runtime API and can mislead typed callers/tooling into
making calls that will raise NoMethodError.

Code

rb/sig/lib/selenium/webdriver/common/service_manager.rbs[R58-59]

+      def self.claim_for_this_process: () -> void
+

Evidence

The compliance requirement is that .rbs signatures reflect the implementation’s public API, yet
the Ruby code defines claim_for_this_process inside class << self under an explicit private
section, making it non-callable from outside. In contrast, the .rbs declares it as a normal `def
self.claim_for_this_process`, which implies public visibility, so static type-checking would allow
external calls even though Ruby will reject them at runtime due to private method visibility.

Rule 389239: Keep Ruby .rbs signatures in sync with public API changes
rb/sig/lib/selenium/webdriver/common/service_manager.rbs[58-59]
rb/lib/selenium/webdriver/common/service_manager.rb[65-75]
rb/sig/lib/selenium/webdriver/common/service_manager.rbs[52-60]

Agent prompt

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

## Issue description
`ServiceManager.claim_for_this_process` is private in the Ruby implementation but is currently declared as a public class method in the `.rbs`, exposing a non-public API in the type surface and enabling type-checked external calls that can fail at runtime.

## Issue Context
The Ruby implementation defines `claim_for_this_process` under `class << self` and marks it `private`. The RBS should reflect this visibility (e.g., `private def self.claim_for_this_process: ...`), using existing repo patterns such as `private`/`public` blocks around class methods if needed to ensure subsequent method visibility is correct.

## Fix Focus Areas
- rb/sig/lib/selenium/webdriver/common/service_manager.rbs[58-59]
- rb/lib/selenium/webdriver/common/service_manager.rb[65-75]

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

---

[qodo-code-review]

1. Rspec mocks in new spec 📘 Rule violation ▣ Testability

The new unit spec relies on RSpec mocking (instance_double, allow(...).to receive_messages)
rather than real or contract-driven integrations, which risks tests diverging from real interfaces
over time.

Agent Prompt

## Issue description
The newly added unit spec uses RSpec mocks/doubles (e.g., `instance_double` and `allow(...).to receive_messages`) instead of using real implementations or simple in-memory fakes.

## Issue Context
Per compliance guidance, mocking frameworks should be avoided unless backed by a machine-checked contract; otherwise, tests can drift from the real API.

## Fix Focus Areas
- rb/spec/unit/selenium/webdriver/common/service_manager_spec.rb[31-42]

ⓘ 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 b6b834d

[qodo-code-review]

Code review by qodo was updated up to the latest commit bf2a605

[diemol]

Can you use the PR template and explain what is being done here? Letting AI do the work is not enough for us to accept a PR.