From d3807ec1fa92e5afe2b2e4e640a40a7b5edb8464 Mon Sep 17 00:00:00 2001 From: James Couball Date: Mon, 31 Aug 2026 13:26:11 -0700 Subject: [PATCH] refactor: derive the process-group isolation decision from a single source (#191) Commands::SpawnWithTimeout derived the decision "did this class isolate the subprocess into its own process group?" twice: #process_group_options computed it directly, and #isolated_in_new_process_group? re-derived it from the merged spawn options via #process_group_leader? plus repeated :not_set checks. The two were equivalent, but a reader had to prove it. Now #process_group_options is the single source of truth for whether this class asked for isolation, and #isolated_in_new_process_group? is !process_group_options.empty? && process_group_leader? -- the ask, plus confirmation (over the options actually used) that it took effect, so a subclass #spawn_options override that removes the added group option cannot make the abandoned-wait cleanup kill a child whose signal semantics this class never changed. The effective spawn options are captured once in #call -- after any subclass #spawn_options additions, such as Commands::Run's redirection overrides -- as a plain attr_reader written only by #call, so the kill path inspects the options actually passed to Process.spawn instead of rebuilding the merged hash on each check. #process_group_options is documented as deterministic (it depends only on the options, not mutated during #call, and the platform) and as never reflecting a subclass override, so an option a subclass contributes can make the subprocess a group leader for the timeout kill but never counts as isolation by this class. Characterization tests pin the behavior this refactor could silently break: caller-supplied pgroup: true / pgroup: 0 / new_pgroup: true still group-kill on timeout, pass through unchanged, and are left alone by the abandoned-wait cleanup; a subclass that removes the added group option (both platform keys) prevents the cleanup; and a subclass-contributed group option with no timeout is not isolation -- the one case where the new definition and the old one disagree, which makes the definitional change detectable by the suite. The kill-path unit specs drive a real (stubbed) spawn-then-kill lifecycle instead of a fabricated pre-spawn state. No public API or documented behavior changes; the only semantic difference from the old derivation is in the subclass-override corner documented above, which no subclass exercises. --- .../commands/spawn_with_timeout.rb | 42 +++- ...rocess_executer_spawn_with_timeout_spec.rb | 201 +++++++++++++++--- 2 files changed, 211 insertions(+), 32 deletions(-) diff --git a/lib/process_executer/commands/spawn_with_timeout.rb b/lib/process_executer/commands/spawn_with_timeout.rb index bf9298f..ffd4dcb 100644 --- a/lib/process_executer/commands/spawn_with_timeout.rb +++ b/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,6 +153,13 @@ 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 @@ -148,6 +167,11 @@ def spawn_options = options.spawn_options.merge(process_group_options) # 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 diff --git a/spec/process_executer_spawn_with_timeout_spec.rb b/spec/process_executer_spawn_with_timeout_spec.rb index 30ed755..b84937c 100644 --- a/spec/process_executer_spawn_with_timeout_spec.rb +++ b/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