[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 one exit hook

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Replace per-service at_exit closures with a single per-process exit hook and running registry.
• Untrack services on stop so stopped services and ChildProcess objects can be garbage-collected.
• Add unit coverage for tracking, untracking, exit-hook arming, and fork PID isolation.

Diagram

graph TD
  A["ServiceManager#start"] --> B["ServiceManager.track"] --> C{{"claim_for_this_process"}} --> D["Platform.exit_hook"]
  D --> E["ServiceManager.stop_running"] --> F["ServiceManager#stop"]
  F --> G["ServiceManager.untrack"]

  subgraph Legend
    direction LR
    _m["Method"] ~~~ _d{{"Decision"}} ~~~ _ext["Platform hook"]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. WeakRef/ObjectSpace::WeakMap for running services

• ➕ Automatically releases stopped services without explicit untrack calls
• ➕ Simplifies registry management (no need to delete on stop)
• ➖ Running services could be GC’d before process exit, defeating the exit hook’s purpose
• ➖ Introduces GC-timing-dependent behavior that can be flaky/hard to reason about

2. Keep per-service at_exit and try to unregister on stop

• ➕ Minimal code restructuring; preserves current mental model
• ➖ Ruby provides no API to remove a registered at_exit handler
• ➖ Still risks retaining service managers if unregistration isn’t possible

3. Retain service manager but aggressively clear internal references on stop

• ➕ Reduces memory impact even if the object remains reachable
• ➕ Lower behavioral risk than changing exit-hook strategy
• ➖ Does not fix unbounded growth (still one retained object per started service)
• ➖ Easy to miss retained references and regress over time

Recommendation: The chosen approach (single per-process exit hook + mutex-guarded running registry + explicit untrack on stop) best addresses the root cause: retention via per-service at_exit closures. The PID-guarded claim_for_this_process design is the right tradeoff for forked environments, ensuring children arm their own hook and do not stop parent-owned services.

Files changed (3) +159 / -1

Bug fix (1) +47 / -1

service_manager.rbTrack running services with a single per-process exit hook +47/-1

Track running services with a single per-process exit hook

• Introduces a class-level, mutex-protected registry of running ServiceManager instances and a single at-exit hook armed lazily per process. start now tracks the instance instead of capturing it in a per-service exit hook, and stop untracks the instance to allow GC. Adds PID-based reset to avoid a forked child stopping parent services and to ensure the child arms its own hook.

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

Tests (1) +98 / -0

service_manager_spec.rbAdd unit tests for service tracking, exit hook, and fork PID behavior +98/-0

Add unit tests for service tracking, exit hook, and fork PID behavior

• Adds specs validating that started services are tracked, a single exit hook is registered across multiple starts, stopped services are untracked, and stop_running does not stop services inherited from a parent process after a fork. Resets class-level state between examples to avoid cross-test contamination.

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

Other (1) +14 / -0

service_manager.rbsAdd RBS signatures for new tracking API and private claim method +14/-0

Add RBS signatures for new tracking API and private claim method

• Declares the new class instance variables and singleton methods (stop_running/track/untrack) and marks claim_for_this_process as a private singleton method to match Ruby visibility.

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

[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

✅ Compliance rules (platform): 19 rules

Tip of the day

💡 Did you know, you can enable the Remediation agent and Qodo fixes findings in a dedicated fix PR

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

---

Results up to commit bf2a605 ⚖️ Balanced

No changes from previous review

[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.

[titusfortner]

Ah, why did you close this? I prepared some feedback on it if you still want to pursue it, it's a needed improvement.

[qodo-code-review]

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

[ikraamg]

Hey @titusfortner, I am happy to continue as you think its needed. I thought it was a minor leak and did not want to waste any repo maintainer time on it.

[titusfortner]

Well, maybe I'm getting wires crossed on issues, leave it open until I can review it tomorrow at least.