refactor: derive the process-group isolation decision from a single source (#191)

lib/process_executer/commands/spawn_with_timeout.rb

@@ -48,7 +48,8 @@ def initialize(command, options)
       #
       def call
         begin
-          @pid = Process.spawn(*command, **spawn_options)
+          @effective_spawn_options = spawn_options
+          @pid = Process.spawn(*command, **effective_spawn_options)
         rescue StandardError => e
           raise ProcessExecuter::SpawnError, "Failed to spawn process: #{e.message}"
         end
@@ -132,6 +133,17 @@ def call
       #
       def spawn_options = options.spawn_options.merge(process_group_options)
 
+      # The spawn options that were passed to Process.spawn
+      #
+      # Captured once by {#call} -- after any {#spawn_options} additions a
+      # subclass contributed -- so the kill path inspects the options actually
+      # used instead of recomputing the merge. nil until {#call} spawns the
+      # subprocess; the kill path only runs after that.
+      #
+      # @return [Hash, nil]
+      #
+      attr_reader :effective_spawn_options
+
       # Spawn options that place the subprocess into its own process group
       #
       # When `timeout_after` is set to a value that can fire (`nil` and `0`
@@ -141,13 +153,25 @@ def spawn_options = options.spawn_options.merge(process_group_options)
       # direct child. Empty when no timeout can fire or when the caller gave a
       # `pgroup`/`new_pgroup` option themselves (their setting is honored).
       #
+      # This method never reflects a subclass's {#spawn_options} override, so
+      # {#isolated_in_new_process_group?} never counts an option a subclass
+      # contributes as isolation by this class -- though such an option can
+      # still make the subprocess a process group leader (see
+      # {#process_group_leader?}) -- and a subclass that removes the option
+      # added here prevents the isolation (and its cleanup) altogether.
+      #
       # A new process group is a background group for any terminal the
       # subprocess inherits, so an interactive subprocess that reads the
       # terminal is stopped by `SIGTTIN` and then killed when the timeout
       # fires -- which is the bound `timeout_after` promises. A caller who
       # needs an interactive subprocess to stay in the foreground process
       # group can pass their own `pgroup` option.
       #
+      # Deterministic: the result depends only on {#options} -- not mutated
+      # during {#call} -- and the platform, so the kill path's
+      # {#isolated_in_new_process_group?} re-read agrees with the value that
+      # was merged into the spawn options.
+      #
       # @return [Hash]
       #
       def process_group_options
@@ -302,19 +326,25 @@ def kill_subprocess
       # @return [Boolean]
       #
       def process_group_leader?
-        [true, 0].include?(spawn_options[:pgroup]) || spawn_options[:new_pgroup] == true
+        [true, 0].include?(effective_spawn_options[:pgroup]) || effective_spawn_options[:new_pgroup] == true
       end
 
       # Whether this class isolated the subprocess into its own process group
       #
-      # True when the subprocess is a new process group leader and that came
-      # from {#process_group_options} rather than from a `pgroup`/`new_pgroup`
-      # option the caller supplied.
+      # True when {#process_group_options} -- the single source of truth for
+      # the isolation decision -- added a process group option and the
+      # subprocess actually became a new process group leader
+      # ({#process_group_leader?} over the captured options). The leader
+      # check matters only when a subclass's {#spawn_options} override
+      # removed or overrode the added option: then no isolation happened and
+      # the abandoned-wait cleanup must leave the subprocess alone. False
+      # when the subprocess's process group (if any) came from a
+      # `pgroup`/`new_pgroup` option the caller supplied.
       #
       # @return [Boolean]
       #
       def isolated_in_new_process_group?
-        process_group_leader? && options.pgroup == :not_set && options.new_pgroup == :not_set
+        !process_group_options.empty? && process_group_leader?
       end
 
       # Send SIGKILL to the subprocess's process group

spec/process_executer_spawn_with_timeout_spec.rb

@@ -129,16 +129,39 @@ def spawn_options_for(**options_hash)
         expect(spawn_options_for(timeout_after: 1, new_pgroup: false)[:new_pgroup]).to eq(false)
         expect(spawn_options_for(timeout_after: 1, new_pgroup: false)).not_to have_key(:pgroup)
       end
+
+      it 'should not add its own option when the caller supplied pgroup: true' do
+        expect(spawn_options_for(timeout_after: 1, pgroup: true)[:pgroup]).to eq(true)
+        expect(spawn_options_for(timeout_after: 1, pgroup: true)).not_to have_key(:new_pgroup)
+      end
+
+      it 'should not add its own option when the caller supplied pgroup: 0' do
+        expect(spawn_options_for(timeout_after: 1, pgroup: 0)[:pgroup]).to eq(0)
+        expect(spawn_options_for(timeout_after: 1, pgroup: 0)).not_to have_key(:new_pgroup)
+      end
+
+      it 'should not add its own option when the caller supplied new_pgroup: true' do
+        expect(spawn_options_for(timeout_after: 1, new_pgroup: true)[:new_pgroup]).to eq(true)
+        expect(spawn_options_for(timeout_after: 1, new_pgroup: true)).not_to have_key(:pgroup)
+      end
     end
 
     context 'killing a timed out subprocess' do
-      # A command object with a fake pid whose Process.kill calls are stubbed,
-      # so the group kill and fallback paths can be tested deterministically
-      # on every platform
-      def command_with_stubbed_kill(**options_hash)
+      # A command object that has spawned and reaped a fake pid (Process.spawn
+      # and Process.wait2 are stubbed), so the kill path can be driven against
+      # the spawn state a real call captures
+      def command_with_stubbed_spawn(**options_hash)
         options = ProcessExecuter::Options::SpawnWithTimeoutOptions.new(**options_hash)
         command = ProcessExecuter::Commands::SpawnWithTimeout.new(['exit 0'], options)
-        allow(command).to receive(:pid).and_return(12_345)
+        allow(Process).to receive(:spawn).and_return(12_345)
+        allow(Process).to receive(:wait2).with(12_345).and_return([12_345, instance_double(Process::Status)])
+        command.tap(&:call)
+      end
+
+      # The same, with Process.kill stubbed so the group kill and fallback
+      # paths can be tested deterministically on every platform
+      def command_with_stubbed_kill(**options_hash)
+        command = command_with_stubbed_spawn(**options_hash)
         allow(Process).to receive(:kill).with('KILL', -12_345)
         allow(Process).to receive(:kill).with('KILL', 12_345)
         command
@@ -164,6 +187,112 @@ def command_with_stubbed_kill(**options_hash)
         expect(Process).to have_received(:kill).with('KILL', 12_345)
         expect(Process).not_to have_received(:kill).with('KILL', -12_345)
       end
+
+      it 'should kill the process group when the caller supplied pgroup: true' do
+        command = command_with_stubbed_kill(timeout_after: 1, pgroup: true)
+        command.send(:kill_subprocess)
+        expect(Process).to have_received(:kill).with('KILL', -12_345)
+        expect(Process).not_to have_received(:kill).with('KILL', 12_345)
+      end
+
+      it 'should kill the process group when the caller supplied pgroup: 0' do
+        command = command_with_stubbed_kill(timeout_after: 1, pgroup: 0)
+        command.send(:kill_subprocess)
+        expect(Process).to have_received(:kill).with('KILL', -12_345)
+        expect(Process).not_to have_received(:kill).with('KILL', 12_345)
+      end
+
+      it 'should kill the process group when the caller supplied new_pgroup: true' do
+        command = command_with_stubbed_kill(timeout_after: 1, new_pgroup: true)
+        command.send(:kill_subprocess)
+        expect(Process).to have_received(:kill).with('KILL', -12_345)
+        expect(Process).not_to have_received(:kill).with('KILL', 12_345)
+      end
+    end
+
+    context 'capturing the effective spawn options at spawn time' do
+      # spawn_options is stubbed to return a different hash on each call --
+      # standing in for a subclass override whose per-call state changes --
+      # so a kill path that recomputed the merge instead of reusing the
+      # captured hash would see a later hash and make the wrong decision
+      it 'should make the group-kill decision from the options captured at spawn time' do
+        options = ProcessExecuter::Options::SpawnWithTimeoutOptions.new
+        command = ProcessExecuter::Commands::SpawnWithTimeout.new(['exit 0'], options)
+        allow(command).to receive(:spawn_options).and_return({ pgroup: true }, {})
+        allow(Process).to receive(:spawn).and_return(12_345)
+        allow(Process).to receive(:wait2).with(12_345).and_return([12_345, instance_double(Process::Status)])
+
+        command.call
+
+        expect(Process).to have_received(:spawn).with('exit 0', pgroup: true)
+        expect(command.send(:process_group_leader?)).to eq(true)
+      end
+    end
+
+    context 'cleaning up an abandoned wait when the caller supplied new_pgroup: true' do
+      # A real spawn with new_pgroup is only possible on Windows, so the
+      # cleanup decision is tested with a stubbed pid on every platform
+      it 'should leave the subprocess alone' do
+        options = ProcessExecuter::Options::SpawnWithTimeoutOptions.new(timeout_after: 10, new_pgroup: true)
+        command = ProcessExecuter::Commands::SpawnWithTimeout.new(['exit 0'], options)
+        allow(command).to receive(:pid).and_return(12_345)
+        allow(Process).to receive(:kill)
+        allow(Process).to receive(:wait2)
+
+        command.send(:kill_and_reap_abandoned_subprocess)
+
+        expect(Process).not_to have_received(:kill)
+        expect(Process).not_to have_received(:wait2)
+      end
+    end
+
+    context 'cleaning up an abandoned wait when a subclass added its own process group option' do
+      # A subclass #spawn_options override that contributes pgroup: true when
+      # this class adds nothing (no timeout) makes the subprocess a group
+      # leader, but not one isolated by this class, so the cleanup must leave
+      # it alone
+      it 'should leave the subprocess alone' do
+        subclass = Class.new(ProcessExecuter::Commands::SpawnWithTimeout) do
+          private
+
+          def spawn_options = super.merge(pgroup: true)
+        end
+        options = ProcessExecuter::Options::SpawnWithTimeoutOptions.new
+        command = subclass.new(['exit 0'], options)
+        allow(Process).to receive(:spawn).and_return(12_345)
+        allow(Process).to receive(:wait2).with(12_345).and_return([12_345, instance_double(Process::Status)])
+        command.call
+        allow(Process).to receive(:kill)
+
+        command.send(:kill_and_reap_abandoned_subprocess)
+
+        expect(Process).not_to have_received(:kill)
+      end
+    end
+
+    context 'cleaning up an abandoned wait when a subclass removed the added process group option' do
+      # A subclass #spawn_options override that overrides the process group
+      # option this class added (both platform keys, so whichever one was
+      # added is removed) leaves the subprocess with its ordinary signal
+      # semantics, so the cleanup must not kill a subprocess that was never
+      # actually isolated
+      it 'should leave the subprocess alone' do
+        subclass = Class.new(ProcessExecuter::Commands::SpawnWithTimeout) do
+          private
+
+          def spawn_options = super.merge(pgroup: false, new_pgroup: false)
+        end
+        options = ProcessExecuter::Options::SpawnWithTimeoutOptions.new(timeout_after: 10)
+        command = subclass.new(['exit 0'], options)
+        allow(Process).to receive(:spawn).and_return(12_345)
+        allow(Process).to receive(:wait2).with(12_345).and_return([12_345, instance_double(Process::Status)])
+        command.call
+        allow(Process).to receive(:kill)
+
+        command.send(:kill_and_reap_abandoned_subprocess)
+
+        expect(Process).not_to have_received(:kill)
+      end
     end
 
     context 'when the timeout fires after the subprocess was already reaped' do
@@ -234,6 +363,18 @@ def spawn_command(**options_hash)
         ProcessExecuter::Commands::SpawnWithTimeout.new(['sleep 60'], options)
       end
 
+      # Stop and reap the given subprocess so it does not outlive its example
+      def stop_and_reap(pid, signal: 'KILL')
+        return unless pid
+
+        Process.kill(signal, pid)
+        Process.wait2(pid)
+      rescue Errno::ESRCH, Errno::ECHILD
+        # :nocov: only reached if the subprocess exited on its own
+        nil
+        # :nocov:
+      end
+
       it 'should kill and reap a subprocess that was isolated into its own process group' do
         command = spawn_command
 
@@ -255,16 +396,33 @@ def spawn_command(**options_hash)
         expect(process_dead?(command.pid, within: 0.25)).to eq(false), failure_message
       ensure
         # Do not leave the subprocess running past this example
-        begin
-          if command&.pid
-            Process.kill('KILL', command.pid)
-            Process.wait2(command.pid)
-          end
-        rescue Errno::ESRCH, Errno::ECHILD
-          # :nocov: only reached if the subprocess exited on its own
-          nil
-          # :nocov:
-        end
+        stop_and_reap(command&.pid)
+      end
+
+      it 'should leave the subprocess alone when the caller made it a group leader with pgroup: true' do
+        command = spawn_command(pgroup: true)
+
+        expect { command.call }.to raise_error(Interrupt)
+
+        failure_message = 'expected the subprocess to be left running because its process group ' \
+                          'came from the caller\'s own pgroup: true, not from this class'
+        expect(process_dead?(command.pid, within: 0.25)).to eq(false), failure_message
+      ensure
+        # Do not leave the subprocess running past this example
+        stop_and_reap(command&.pid)
+      end
+
+      it 'should leave the subprocess alone when the caller made it a group leader with pgroup: 0' do
+        command = spawn_command(pgroup: 0)
+
+        expect { command.call }.to raise_error(Interrupt)
+
+        failure_message = 'expected the subprocess to be left running because its process group ' \
+                          'came from the caller\'s own pgroup: 0, not from this class'
+        expect(process_dead?(command.pid, within: 0.25)).to eq(false), failure_message
+      ensure
+        # Do not leave the subprocess running past this example
+        stop_and_reap(command&.pid)
       end
 
       it 'should let the original exception propagate when the cleanup itself fails' do
@@ -279,17 +437,8 @@ def spawn_command(**options_hash)
 
         expect { command.call }.to raise_error(Interrupt)
       ensure
-        # The stubbed KILLs left the subprocess running; stop and reap it
-        begin
-          if command&.pid
-            Process.kill('TERM', command.pid)
-            Process.wait2(command.pid)
-          end
-        rescue Errno::ESRCH, Errno::ECHILD
-          # :nocov: only reached if the subprocess exited on its own
-          nil
-          # :nocov:
-        end
+        # The stubbed KILLs left the subprocess running; TERM passes through
+        stop_and_reap(command&.pid, signal: 'TERM')
       end
     end