diff --git a/lib/skill_bench/tools/run_command.rb b/lib/skill_bench/tools/run_command.rb index 6eae708..6460374 100644 --- a/lib/skill_bench/tools/run_command.rb +++ b/lib/skill_bench/tools/run_command.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true require 'open3' -require 'timeout' require 'shellwords' require_relative '../config' require_relative '../constants' @@ -27,6 +26,10 @@ class RunCommand HOST_EXECUTION_WARNING = 'Warning: running command directly on the host with NO sandbox isolation ' \ '(allow_host_execution is enabled). Commands are not isolated from your machine.' + # Seconds to wait after SIGTERM before escalating to SIGKILL when a command + # exceeds its execution deadline. + TERM_GRACE_PERIOD = 2 + # @return [Hash] The tool definition for the LLM API. def self.definition { @@ -81,43 +84,105 @@ def self.call(command, working_dir_path, container_id = nil) # Runs the resolved command and formats its result, enforcing the # configured execution timeout. # + # The command is spawned in its own process group so that, on timeout, the + # whole group (the command and any children it forked) can be signalled — + # something `Timeout.timeout` around `Open3.capture3` could not do, because + # `capture3`'s `ensure` blocks on `wait_thr.value` and never signals the + # child. + # # @param argv [Array] The tokenized command and arguments. # @param working_dir_path [Pathname] The host directory for host execution. # @param container_id [String, nil] The Docker container ID for isolated execution. # @return [String] Formatted exit status, STDOUT, and STDERR, or a timeout message. def self.execute(argv, working_dir_path, container_id) max_time = SkillBench::Config.max_execution_time - Timeout.timeout(max_time) do - stdout_str, stderr_str, status = capture(argv, working_dir_path, container_id) - <<~RESULT - Exit Status: #{status.exitstatus} - STDOUT: - #{stdout_str} - STDERR: - #{stderr_str} - RESULT - end - rescue Timeout::Error - "Error: Command execution timed out after #{max_time} seconds." + command, spawn_opts = resolve_invocation(argv, working_dir_path, container_id) + result = capture(command, spawn_opts, max_time) + return "Error: Command execution timed out after #{max_time} seconds." if result == :timed_out + + stdout_str, stderr_str, status = result + format_result(status, stdout_str, stderr_str) end private_class_method :execute - # Captures the command output, in the container when one is active or on - # the host otherwise. + # Formats the captured command output into the standard result string. + # + # @param status [Process::Status] The exit status of the command. + # @param stdout_str [String] The captured standard output. + # @param stderr_str [String] The captured standard error. + # @return [String] Formatted exit status, STDOUT, and STDERR. + def self.format_result(status, stdout_str, stderr_str) + <<~RESULT + Exit Status: #{status.exitstatus} + STDOUT: + #{stdout_str} + STDERR: + #{stderr_str} + RESULT + end + private_class_method :format_result + + # Builds the command array and spawn options for either container or host + # execution. Both run in their own process group (`pgroup: true`) so the + # watchdog can kill the whole group on timeout. # # @param argv [Array] The tokenized command and arguments. # @param working_dir_path [Pathname] The host directory for host execution. # @param container_id [String, nil] The Docker container ID for isolated execution. - # @return [Array(String, String, Process::Status)] STDOUT, STDERR, and status. - def self.capture(argv, working_dir_path, container_id) - if container_id - Open3.capture3('docker', 'exec', '-w', '/sandbox', container_id, *argv) - else - Open3.capture3(*argv, chdir: working_dir_path.to_s) + # @return [Array(Array, Hash)] The full command array and spawn options. + def self.resolve_invocation(argv, working_dir_path, container_id) + return [['docker', 'exec', '-w', '/sandbox', container_id, *argv], { pgroup: true }] if container_id + + [argv, { chdir: working_dir_path.to_s, pgroup: true }] + end + private_class_method :resolve_invocation + + # Spawns the command, draining STDOUT/STDERR on separate threads so a chatty + # or hung child never deadlocks the reader, and enforces the deadline with a + # watchdog that kills the process group when the command overruns. + # + # @param command [Array] The full command array (no shell). + # @param spawn_opts [Hash] Options passed to the spawner (includes `pgroup`). + # @param max_time [Integer] Maximum execution time in seconds. + # @return [Array(String, String, Process::Status), Symbol] STDOUT, STDERR, and + # status on completion, or `:timed_out` when the deadline is exceeded. + def self.capture(command, spawn_opts, max_time) + Open3.popen3(*command, **spawn_opts) do |stdin, stdout, stderr, wait_thr| + stdin.close + readers = [Thread.new { stdout.read }, Thread.new { stderr.read }] + completed = wait_thr.join(max_time) + terminate_process_group(wait_thr) unless completed + stdout_str, stderr_str = readers.map(&:value) + completed ? [stdout_str, stderr_str, wait_thr.value] : :timed_out end end private_class_method :capture + # Terminates the command's entire process group: SIGTERM first, then SIGKILL + # after a short grace period if it has not exited. Signalling the negated + # process group id reaches the command and any children it forked. + # + # @param wait_thr [Process::Waiter] The wait thread for the spawned process group leader. + # @return [void] + def self.terminate_process_group(wait_thr) + pgid = wait_thr.pid + signal_group('TERM', pgid) + signal_group('KILL', pgid) unless wait_thr.join(TERM_GRACE_PERIOD) + end + private_class_method :terminate_process_group + + # Sends a signal to a whole process group, ignoring an already-exited group. + # + # @param signal [String] The signal name (e.g. "TERM", "KILL"). + # @param pgid [Integer] The process group id (leader pid) to signal. + # @return [void] + def self.signal_group(signal, pgid) + Process.kill(signal, -pgid) + rescue Errno::ESRCH + nil + end + private_class_method :signal_group + # Emits a single warning that the command will run un-isolated on the host, # honoring the test-suite stderr suppression convention. # diff --git a/test/evaluator/tools/run_command_test.rb b/test/evaluator/tools/run_command_test.rb index 825e2ac..85e7f14 100644 --- a/test/evaluator/tools/run_command_test.rb +++ b/test/evaluator/tools/run_command_test.rb @@ -30,8 +30,8 @@ def test_call_refuses_host_execution_when_not_isolated_and_not_allowed Dir.mktmpdir do |dir| working_dir = Pathname.new(dir).expand_path - # Nothing must be executed when fail-closed refusal triggers. - Open3.expects(:capture3).never + # Nothing must be spawned when the fail-closed refusal triggers. + Open3.expects(:popen3).never result = RunCommand.call('echo test', working_dir) @@ -48,10 +48,6 @@ def test_call_executes_on_host_with_warning_when_explicitly_allowed Dir.mktmpdir do |dir| working_dir = Pathname.new(dir).expand_path - status = mock('Process::Status') - status.stubs(:exitstatus).returns(0) - Open3.expects(:capture3).with('echo', 'test', chdir: working_dir.to_s).returns(['test', '', status]) - result = nil _out, err = capture_io do result = RunCommand.call('echo test', working_dir) @@ -69,10 +65,13 @@ def test_call_executes_in_container working_dir = Pathname.new(dir).expand_path container_id = 'mock-container-id' - # Mock Open3.capture3 to verify docker call + # Stub the spawn/watchdog seam to verify the docker invocation is built + # correctly without requiring a real Docker daemon. status = mock('Process::Status') status.stubs(:exitstatus).returns(0) - Open3.expects(:capture3).with('docker', 'exec', '-w', '/sandbox', container_id, 'echo', 'test').returns(['test', '', status]) + RunCommand.expects(:capture) + .with(['docker', 'exec', '-w', '/sandbox', container_id, 'echo', 'test'], { pgroup: true }, SkillBench::Config.max_execution_time) + .returns(['test', '', status]) result = nil _out, err = capture_io do @@ -85,6 +84,45 @@ def test_call_executes_in_container assert_empty err end end + + def test_call_returns_timeout_result_without_waiting_for_full_runtime + SkillBench::Config.allow_host_execution = true + SkillBench::Config.allowed_commands = %w[sleep] + SkillBench::Config.max_execution_time = 1 + + Dir.mktmpdir do |dir| + working_dir = Pathname.new(dir).expand_path + + started = Process.clock_gettime(Process::CLOCK_MONOTONIC) + result = RunCommand.call('sleep 30', working_dir) + elapsed = Process.clock_gettime(Process::CLOCK_MONOTONIC) - started + + # Same timeout shape as before — keyed on by callers/tests. + assert_equal 'Error: Command execution timed out after 1 seconds.', result.strip + # The watchdog must return shortly after the deadline rather than + # blocking on the child for the full 30s sleep (the old bug). + assert_operator elapsed, :<, 10, 'timeout must not block for the full child runtime' + end + end + + def test_call_kills_runaway_child_process_on_timeout + SkillBench::Config.allow_host_execution = true + SkillBench::Config.allowed_commands = %w[sleep] + SkillBench::Config.max_execution_time = 1 + + Dir.mktmpdir do |dir| + working_dir = Pathname.new(dir).expand_path + + result = RunCommand.call('sleep 30', working_dir) + + assert_match(/timed out after 1 seconds/, result) + # Proof the child is actually terminated and reaped: no lingering + # `sleep` child remains under this process after the call returns. + lingering = `pgrep -P #{Process.pid} sleep`.split + + assert_empty lingering, 'the runaway sleep child must be killed and reaped on timeout' + end + end end end end