diff --git a/lib/puppet/agent.rb b/lib/puppet/agent.rb index df6adc8762..4f215505c8 100644 --- a/lib/puppet/agent.rb +++ b/lib/puppet/agent.rb @@ -146,8 +146,55 @@ def run_in_fork(forking = true) atForkHandler.parent end - exit_code = Process.waitpid2(child_pid) - exit_code[1].exitstatus + _, status = wait_for_child(child_pid) + status.exitstatus + end + + # Wait for the forked child to exit. The child enforces `runtimeout` on its + # own run, but it may fail to exit afterwards (a stranded thread, a hung + # subprocess, etc.). Without a deadline here, the parent blocks forever and + # the agent stops checking in. Once the run timeout plus a grace period has + # elapsed, the child is killed so the daemon can carry on. + def wait_for_child(child_pid) + deadline = child_deadline + return Process.waitpid2(child_pid) unless deadline + + loop do + result = Process.waitpid2(child_pid, Process::WNOHANG) + return result if result + + if monotonic_now >= deadline + Puppet.err _("Agent run (pid %{pid}) did not exit within %{timeout} seconds of the run timeout, killing it") % + { pid: child_pid, timeout: child_grace_period } + begin + Process.kill(:KILL, child_pid) + rescue Errno::ESRCH + # The child exited between the last check and the kill. + end + return Process.waitpid2(child_pid) + end + + sleep 1 + end + end + + # After the run timeout fires inside the child, it may still need to send + # its report, which is bounded by the HTTP connect and read timeouts. + def child_grace_period + Puppet[:http_connect_timeout] + Puppet[:http_read_timeout] + end + + # Returns the absolute monotonic deadline for the child, or nil when + # `runtimeout` is disabled. + def child_deadline + runtimeout = Puppet[:runtimeout] + return nil if runtimeout.nil? || runtimeout <= 0 + + monotonic_now + runtimeout + child_grace_period + end + + def monotonic_now + Process.clock_gettime(Process::CLOCK_MONOTONIC) end private diff --git a/spec/unit/agent_spec.rb b/spec/unit/agent_spec.rb index 0dc635b47f..4026f60be9 100644 --- a/spec/unit/agent_spec.rb +++ b/spec/unit/agent_spec.rb @@ -315,6 +315,60 @@ def controlled_run(&block) end end + describe "when waiting for the forked child", :if => Puppet.features.posix? && RUBY_PLATFORM != 'java' do + let(:child_pid) { 1234 } + let(:status) { instance_double(Process::Status, exitstatus: 3) } + + before do + @agent = Puppet::Agent.new(AgentTestClient, true) + allow(Kernel).to receive(:fork).and_return(child_pid) + allow(@agent).to receive(:sleep) + end + + it "returns the child's exit status when it exits before the deadline" do + expect(Process).to receive(:waitpid2).with(child_pid, Process::WNOHANG).and_return(nil, [child_pid, status]) + expect(Process).not_to receive(:kill) + + expect(@agent.run_in_fork { 0 }).to eq(3) + end + + it "blocks without a deadline when runtimeout is disabled" do + Puppet[:runtimeout] = 0 + + expect(Process).to receive(:waitpid2).with(child_pid).and_return([child_pid, status]) + expect(Process).not_to receive(:kill) + + expect(@agent.run_in_fork { 0 }).to eq(3) + end + + it "kills the child once runtimeout plus the grace period has elapsed" do + Puppet[:runtimeout] = 10 + Puppet[:http_connect_timeout] = 5 + Puppet[:http_read_timeout] = 5 + # now, first deadline check, second deadline check + allow(Process).to receive(:clock_gettime).with(Process::CLOCK_MONOTONIC).and_return(100, 105, 120) + + expect(Process).to receive(:waitpid2).with(child_pid, Process::WNOHANG).twice.and_return(nil) + expect(Process).to receive(:kill).with(:KILL, child_pid) + expect(Process).to receive(:waitpid2).with(child_pid).and_return([child_pid, instance_double(Process::Status, exitstatus: nil)]) + expect(Puppet).to receive(:err).with(/did not exit within 10 seconds of the run timeout/) + + expect(@agent.run_in_fork { 0 }).to be_nil + end + + it "reaps the child if it exits between the deadline check and the kill" do + Puppet[:runtimeout] = 10 + allow(Process).to receive(:clock_gettime).with(Process::CLOCK_MONOTONIC).and_return(100, 100_000) + + expect(Process).to receive(:waitpid2).with(child_pid, Process::WNOHANG).and_return(nil) + expect(Process).to receive(:kill).with(:KILL, child_pid).and_raise(Errno::ESRCH) + expect(Process).to receive(:waitpid2).with(child_pid).and_return([child_pid, status]) + allow(Puppet).to receive(:err) + + expect(@agent.run_in_fork { 0 }).to eq(3) + end + end + describe "on Windows", :if => Puppet::Util::Platform.windows? do it "should never fork" do agent = Puppet::Agent.new(AgentTestClient, true)