From 5f49e461c55d57316ac004abdd2ca4072ba33583 Mon Sep 17 00:00:00 2001 From: Ikraam Ghoor Date: Sun, 9 Aug 2026 22:58:56 +0200 Subject: [PATCH 1/3] [rb] Release a stopped service instead of holding it until the process exits ServiceManager#start registered an at_exit block per service, and the block captured the service, so every service ever started stayed reachable for the life of the process along with its ChildProcess. Stopping the service did not release it. Starting 500 services and stopping all of them left 500 alive. Services are now tracked in one list, with a single exit hook per process, and #stop removes the service from it. Co-Authored-By: Claude Opus 5 (1M context) --- .../webdriver/common/service_manager.rb | 38 ++++++++- .../webdriver/common/service_manager.rbs | 12 +++ .../webdriver/common/service_manager_spec.rb | 82 +++++++++++++++++++ 3 files changed, 131 insertions(+), 1 deletion(-) create mode 100644 rb/spec/unit/selenium/webdriver/common/service_manager_spec.rb diff --git a/rb/lib/selenium/webdriver/common/service_manager.rb b/rb/lib/selenium/webdriver/common/service_manager.rb index 45fbf0b5011da..cc7b7fe5c0b34 100644 --- a/rb/lib/selenium/webdriver/common/service_manager.rb +++ b/rb/lib/selenium/webdriver/common/service_manager.rb @@ -30,6 +30,41 @@ class ServiceManager SOCKET_LOCK_TIMEOUT = 45 STOP_TIMEOUT = 20 + @running = [] + @running_mutex = Mutex.new + @exit_hook_pid = nil + + class << self + # + # Stops every service still running in this process. + # + + def stop_running + @running_mutex.synchronize { @running.dup }.each(&:stop) + end + + # + # Tracks a started service so it can be stopped at exit. One exit hook covers + # all of them, so a service that has been stopped is not held onto. + # + + def track(manager) + @running_mutex.synchronize do + unless @exit_hook_pid == Process.pid + @exit_hook_pid = Process.pid + @running.clear # anything inherited through a fork belongs to the parent + Platform.exit_hook { stop_running } + end + + @running << manager + end + end + + def untrack(manager) + @running_mutex.synchronize { @running.delete(manager) } + end + end + # # End users should use a class method for the desired driver, rather than using this directly. # @@ -50,7 +85,7 @@ def initialize(config) def start raise "already started: #{uri.inspect} #{@executable_path.inspect}" if process_running? - Platform.exit_hook { stop } # make sure we don't leave the server running + self.class.track(self) # make sure we don't leave the server running socket_lock.locked do find_free_port @@ -69,6 +104,7 @@ def stop nil # noop ensure stop_process + self.class.untrack(self) end def uri diff --git a/rb/sig/lib/selenium/webdriver/common/service_manager.rbs b/rb/sig/lib/selenium/webdriver/common/service_manager.rbs index 503e27c7aea4a..8e9b54707c9b8 100644 --- a/rb/sig/lib/selenium/webdriver/common/service_manager.rbs +++ b/rb/sig/lib/selenium/webdriver/common/service_manager.rbs @@ -43,6 +43,18 @@ module Selenium STOP_TIMEOUT: Integer + self.@running: Array[ServiceManager] + + self.@running_mutex: Thread::Mutex + + self.@exit_hook_pid: Integer? + + def self.stop_running: () -> void + + def self.track: (ServiceManager manager) -> void + + def self.untrack: (ServiceManager manager) -> void + def initialize: (untyped config) -> void def start: () -> untyped diff --git a/rb/spec/unit/selenium/webdriver/common/service_manager_spec.rb b/rb/spec/unit/selenium/webdriver/common/service_manager_spec.rb new file mode 100644 index 0000000000000..b3fcd6549e5cc --- /dev/null +++ b/rb/spec/unit/selenium/webdriver/common/service_manager_spec.rb @@ -0,0 +1,82 @@ +# frozen_string_literal: true + +# Licensed to the Software Freedom Conservancy (SFC) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The SFC licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +require File.expand_path('../spec_helper', __dir__) + +module Selenium + module WebDriver + describe ServiceManager do + subject(:manager) { build_manager } + + let(:running) { described_class.instance_variable_get(:@running) } + + # The service itself is never launched; only the bookkeeping around it is under test. + def build_manager(port: 4444) + 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, + start_process: nil, connect_until_stable: nil, + stop_process: nil) + end + end + + def yielding_lock + instance_double(SocketLock).tap { |lock| allow(lock).to receive(:locked).and_yield } + end + + after { described_class.stop_running } + + describe '.track' do + it 'holds a started service so it can be stopped at exit' do + manager.start + + expect(running).to include(manager) + end + + it 'registers a single exit hook however many services start' do + allow(Platform).to receive(:exit_hook) + + 2.times { build_manager.start } + + expect(Platform).to have_received(:exit_hook).at_most(:once) + end + end + + describe '.untrack' do + it 'releases a service once it is stopped' do + manager.start + manager.stop + + expect(running).not_to include(manager) + end + end + + describe '.stop_running' do + it 'stops a service that is still running' do + manager.start + + described_class.stop_running + + expect(manager).to have_received(:stop_process) + end + end + end + end +end From b6b834d3fda5f4eb27d7cda8c2db690c93687521 Mon Sep 17 00:00:00 2001 From: Ikraam Ghoor Date: Mon, 10 Aug 2026 01:08:14 +0200 Subject: [PATCH 2/3] [rb] Guard stop_running against services inherited through a fork stop_running is public, so a child could call it before starting anything of its own and stop services belonging to its parent. The check that drops inherited state now runs there too, rather than only in track. The exit hook spec asserted at_most(:once), which a call count of zero satisfies. Because the pid that armed the hook outlived an example, that is what it was measuring. The tracking state is now reset per example and the count is exact. Co-Authored-By: Claude Opus 5 (1M context) --- .../webdriver/common/service_manager.rb | 24 +++++++++++++------ .../webdriver/common/service_manager.rbs | 2 ++ .../webdriver/common/service_manager_spec.rb | 18 +++++++++++++- 3 files changed, 36 insertions(+), 8 deletions(-) diff --git a/rb/lib/selenium/webdriver/common/service_manager.rb b/rb/lib/selenium/webdriver/common/service_manager.rb index cc7b7fe5c0b34..b852cf99ad9db 100644 --- a/rb/lib/selenium/webdriver/common/service_manager.rb +++ b/rb/lib/selenium/webdriver/common/service_manager.rb @@ -40,7 +40,10 @@ class << self # def stop_running - @running_mutex.synchronize { @running.dup }.each(&:stop) + @running_mutex.synchronize { + claim_for_this_process + @running.dup + }.each(&:stop) end # @@ -50,12 +53,7 @@ def stop_running def track(manager) @running_mutex.synchronize do - unless @exit_hook_pid == Process.pid - @exit_hook_pid = Process.pid - @running.clear # anything inherited through a fork belongs to the parent - Platform.exit_hook { stop_running } - end - + claim_for_this_process @running << manager end end @@ -63,6 +61,18 @@ def track(manager) def untrack(manager) @running_mutex.synchronize { @running.delete(manager) } end + + private + + # Services tracked before a fork belong to the parent, which stops them itself. + # The exit hook is armed here rather than at load so a child gets one of its own. + def claim_for_this_process + return if @exit_hook_pid == Process.pid + + @exit_hook_pid = Process.pid + @running.clear + Platform.exit_hook { stop_running } + end end # diff --git a/rb/sig/lib/selenium/webdriver/common/service_manager.rbs b/rb/sig/lib/selenium/webdriver/common/service_manager.rbs index 8e9b54707c9b8..94cf580dd76d0 100644 --- a/rb/sig/lib/selenium/webdriver/common/service_manager.rbs +++ b/rb/sig/lib/selenium/webdriver/common/service_manager.rbs @@ -55,6 +55,8 @@ module Selenium def self.untrack: (ServiceManager manager) -> void + def self.claim_for_this_process: () -> void + def initialize: (untyped config) -> void def start: () -> untyped diff --git a/rb/spec/unit/selenium/webdriver/common/service_manager_spec.rb b/rb/spec/unit/selenium/webdriver/common/service_manager_spec.rb index b3fcd6549e5cc..397ea34a34698 100644 --- a/rb/spec/unit/selenium/webdriver/common/service_manager_spec.rb +++ b/rb/spec/unit/selenium/webdriver/common/service_manager_spec.rb @@ -41,6 +41,13 @@ def yielding_lock instance_double(SocketLock).tap { |lock| allow(lock).to receive(:locked).and_yield } end + # The tracking list and the pid that armed the exit hook outlive an example, so a + # later one would otherwise see the hook as already armed. + before do + described_class.instance_variable_set(:@running, []) + described_class.instance_variable_set(:@exit_hook_pid, nil) + end + after { described_class.stop_running } describe '.track' do @@ -55,7 +62,7 @@ def yielding_lock 2.times { build_manager.start } - expect(Platform).to have_received(:exit_hook).at_most(:once) + expect(Platform).to have_received(:exit_hook).exactly(:once) end end @@ -76,6 +83,15 @@ def yielding_lock expect(manager).to have_received(:stop_process) end + + it 'leaves a service inherited through a fork to the parent' do + manager.start + described_class.instance_variable_set(:@exit_hook_pid, Process.pid - 1) + + described_class.stop_running + + expect(manager).not_to have_received(:stop_process) + end end end end From bf2a605f2ab650bc975fa14d112d76ada96225f8 Mon Sep 17 00:00:00 2001 From: Ikraam Ghoor Date: Mon, 10 Aug 2026 08:56:13 +0200 Subject: [PATCH 3/3] [rb] Declare claim_for_this_process private in RBS It is a private singleton method in Ruby, so the signature should not offer it as part of the class surface. Co-Authored-By: Claude Opus 5 (1M context) --- rb/sig/lib/selenium/webdriver/common/service_manager.rbs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rb/sig/lib/selenium/webdriver/common/service_manager.rbs b/rb/sig/lib/selenium/webdriver/common/service_manager.rbs index 94cf580dd76d0..e25ff45f1442c 100644 --- a/rb/sig/lib/selenium/webdriver/common/service_manager.rbs +++ b/rb/sig/lib/selenium/webdriver/common/service_manager.rbs @@ -55,7 +55,7 @@ module Selenium def self.untrack: (ServiceManager manager) -> void - def self.claim_for_this_process: () -> void + private def self.claim_for_this_process: () -> void def initialize: (untyped config) -> void