feat: improve process termination and directory cleanup logic

.rubocop.yml

@@ -6,6 +6,10 @@ AllCops:
 Layout/FirstArrayElementIndentation:
   EnforcedStyle: consistent
 
+Layout/LineLength:
+  Exclude:
+    - spec/**/*
+
 Naming/PredicateMethod:
   Enabled: false
 

CHANGELOG.md

@@ -1,6 +1,10 @@
 ## [Unreleased](https://github.com/rubycdp/ferrum/compare/v0.18.0...main) ##
 
 ### Added
+- `Ferrum::Browser#quit`/`Ferrum::Browser::Process#stop` accept `wait: false` to return immediately and run process
+  killing and user-data-directory cleanup on a background thread instead of blocking; the call returns the `Thread`
+  so callers can `#join` it if they need cleanup to have finished, e.g. before process exit or before reusing a
+  fixed port. Default (`wait: true`) keeps the previous synchronous behavior; `#restart` always waits.
 
 ### Changed
 
@@ -9,6 +13,16 @@
   A worker's attach or service-worker detach that timed out (e.g. on a loaded CI runner) would escape unrescued
   into `Client::Subscriber`'s dispatch thread and kill it, silently breaking further `Target.*` event delivery
   for the rest of that browser's life, raising `Ferrum::NoSuchTargetError`
+- `Ferrum::Browser::Process` killed the browser's leader pid alone with `SIGUSR1`, a signal Chromium has no shutdown
+  handler for; it now sends `SIGTERM`, escalating to `SIGKILL`, to the whole process group, so renderer/GPU/zygote
+  child processes are no longer orphaned after `#quit`. A leader process that exited promptly on `TERM` used to
+  short-circuit escalation to `KILL` for the rest of its process group, so a child that ignored `TERM`
+  (e.g. a stuck renderer) was left running forever; termination now keeps polling the group until it's actually empty
+  or the timeout fires.
+- Removing the user data directory after `#quit` silently gave up on any error, potentially leaking the temp
+  directory forever with no indication; it now retries with exponential backoff on transient errors
+  (`Errno::ENOTEMPTY`/`EBUSY`/`EACCES`/`EPERM`, since Chrome can briefly hold file locks right after being killed)
+  and warns if it still can't be removed.
 
 ### Removed
 

docs/1-introduction.md

@@ -118,6 +118,18 @@ browser.reset
 browser.quit
 ```
 
+`#quit` blocks by default until the browser process is confirmed dead and its user data directory removed. Killing a
+stubborn process (one that ignores `TERM` and needs `KILL`) can take a couple of seconds, which matters if you're
+quitting many browsers in a hot path. Pass `wait: false` to return immediately and run that cleanup on a background
+thread instead:
+
+```ruby
+browser = Ferrum::Browser.new
+thread = browser.quit(wait: false)
+# ... do other work while the browser is killed and its directory removed in the background ...
+thread.join # only needed if you must wait for cleanup to finish, e.g. before reusing a fixed port
+```
+
 ## Thread safety
 
 Ferrum is fully thread-safe. You can create one browser or a few as you wish and

lib/ferrum/browser.rb

@@ -238,14 +238,23 @@ def restart
     #
     # Terminates the browser process and closes the client connection.
     #
-    def quit
+    # @param [Boolean] wait
+    #   Whether to block until the process is confirmed dead and its user
+    #   data directory removed (the default), or return immediately and run
+    #   that cleanup on a background thread instead. See {Process#stop}.
+    #
+    # @return [Thread, nil]
+    #   The background cleanup thread when `wait: false`, `nil` otherwise.
+    #
+    def quit(wait: true)
       return unless @client
 
       contexts.close_connections
 
       @client.close
-      @process.stop
+      thread = @process.stop(wait: wait)
       @client = @process = @contexts = nil
+      thread
     end
 
     #

lib/ferrum/browser/process.rb

@@ -8,9 +8,11 @@
 require "ferrum/browser/options/base"
 require "ferrum/browser/options/chrome"
 require "ferrum/browser/options/firefox"
+require "ferrum/browser/process/killer"
 require "ferrum/browser/command"
 require "ferrum/utils/elapsed_time"
 require "ferrum/utils/platform"
+require "ferrum/utils/thread"
 
 module Ferrum
   class Browser
@@ -21,9 +23,6 @@ class Browser
     # stopping/restarting the process and cleaning up its user data directory.
     #
     class Process
-      KILL_TIMEOUT = 2
-      WAIT_KILLED = 0.05
-
       extend Forwardable
 
       delegate path: :command
@@ -40,54 +39,6 @@ def self.start(*args)
         new(*args).tap(&:start)
       end
 
-      #
-      # Builds a finalizer proc that kills the process with the given pid.
-      #
-      # @param [Integer] pid
-      #   Process id to kill.
-      #
-      # @return [Proc]
-      #
-      def self.process_killer(pid)
-        proc do
-          if Utils::Platform.windows?
-            # Process.kill is unreliable on Windows
-            ::Process.kill("KILL", pid) unless system("taskkill /f /t /pid #{pid} >NUL 2>NUL")
-          else
-            ::Process.kill("USR1", pid)
-            start = Utils::ElapsedTime.monotonic_time
-            while ::Process.wait(pid, ::Process::WNOHANG).nil?
-              sleep(WAIT_KILLED)
-              next unless Utils::ElapsedTime.timeout?(start, KILL_TIMEOUT)
-
-              ::Process.kill("KILL", pid)
-              ::Process.wait(pid)
-              break
-            end
-          end
-        rescue Errno::ESRCH, Errno::ECHILD
-          # nop
-        end
-      end
-
-      #
-      # Builds a finalizer proc that removes the directory at the given path.
-      #
-      # @param [String] path
-      #   Directory to remove.
-      #
-      # @return [Proc]
-      #
-      def self.directory_remover(path)
-        proc {
-          begin
-            FileUtils.remove_entry(path)
-          rescue StandardError
-            Errno::ENOENT
-          end
-        }
-      end
-
       attr_reader :host, :port, :ws_url, :pid, :command,
                   :default_user_agent, :browser_version, :protocol_version,
                   :v8_version, :webkit_version, :xvfb
@@ -108,7 +59,7 @@ def initialize(options)
         @env = Hash(options.env)
 
         tmpdir = Dir.mktmpdir("ferrum_user_data_dir_")
-        ObjectSpace.define_finalizer(self, self.class.directory_remover(tmpdir))
+        ObjectSpace.define_finalizer(self, Killer.directory_remover(tmpdir))
         @user_data_dir = tmpdir
         @command = Command.build(options, tmpdir)
       end
@@ -130,12 +81,12 @@ def start
 
           if @command.xvfb?
             @xvfb = Xvfb.start(@command.options)
-            ObjectSpace.define_finalizer(self, self.class.process_killer(@xvfb.pid))
+            ObjectSpace.define_finalizer(self, Killer.process_killer(@xvfb.pid))
           end
 
           env = Hash(@xvfb&.to_env).merge(@env)
           @pid = ::Process.spawn(env, *@command.to_a, process_options)
-          ObjectSpace.define_finalizer(self, self.class.process_killer(@pid))
+          ObjectSpace.define_finalizer(self, Killer.process_killer(@pid))
 
           parse_ws_url(read_io, @process_timeout)
           parse_json_version(ws_url)
@@ -148,17 +99,26 @@ def start
       # Kills the browser process (and Xvfb, if running) and removes the user
       # data directory.
       #
-      # @return [void]
-      #
-      def stop
-        if @pid
-          kill(@pid)
-          kill(@xvfb.pid) if @xvfb&.pid
-          @pid = nil
-        end
-
-        remove_user_data_dir if @user_data_dir
-        ObjectSpace.undefine_finalizer(self)
+      # @param [Boolean] wait
+      #   Whether to block until the process is confirmed dead and its user
+      #   data directory removed (the default), or return immediately and
+      #   run that cleanup on a background thread instead. Killing a
+      #   stubborn process group can block for up to {Killer::KILL_TIMEOUT}
+      #   seconds, plus retries removing its directory, which matters when quitting
+      #   many browsers in a hot path. `wait: false` is not used by
+      #   {#restart}, which always waits so the old process is fully gone
+      #   before the new one starts.
+      #
+      # @return [Thread, nil]
+      #   The background cleanup thread when `wait: false`; the caller can
+      #   `#join` it if they need cleanup to have finished, e.g. before
+      #   process exit or before reusing a fixed port. `nil` when `wait:
+      #   true`.
+      #
+      def stop(wait: true)
+        return sync_stop if wait
+
+        async_stop
       end
 
       #
@@ -189,12 +149,40 @@ def inspect
 
       private
 
-      def kill(pid)
-        self.class.process_killer(pid).call
+      def sync_stop
+        if @pid
+          Killer.kill(@pid)
+          Killer.kill(@xvfb.pid) if @xvfb&.pid
+          @pid = nil
+        end
+
+        remove_user_data_dir if @user_data_dir
+        ObjectSpace.undefine_finalizer(self)
+        nil
+      end
+
+      #
+      # Snapshots what needs killing/removing, clears instance state so the
+      # object looks stopped right away, and does the actual work on a
+      # background thread. The finalizer is left in place as a backup until
+      # that thread finishes, in case the process exits before it does.
+      #
+      def async_stop
+        pid = @pid
+        xvfb_pid = @xvfb&.pid
+        user_data_dir = @user_data_dir
+        @pid = @user_data_dir = nil
+
+        Utils::Thread.spawn(abort_on_exception: false) do
+          Killer.kill(pid) if pid
+          Killer.kill(xvfb_pid) if xvfb_pid
+          Killer.remove_directory(user_data_dir) if user_data_dir
+          ObjectSpace.undefine_finalizer(self)
+        end
       end
 
       def remove_user_data_dir
-        self.class.directory_remover(@user_data_dir).call
+        Killer.remove_directory(@user_data_dir)
         @user_data_dir = nil
       end