From acd845077223e2a58fd4938b06a326bda620c522 Mon Sep 17 00:00:00 2001 From: Udi Feldman Date: Tue, 26 May 2026 09:11:27 -0400 Subject: [PATCH] Drop per-fork shutdown deadline in Hybrid#run `Hybrid#run`'s ensure block called `container.stop` (no positional argument) on the inner Threaded sub-container. `Generic#stop(timeout = true)` then forwarded the literal `true` to `Group#stop`, which expanded it to the hardcoded `DEFAULT_GRACEFUL_TIMEOUT = 10.0`. That capped the per-fork drain budget at 10s regardless of the outer `Container::Controller#@graceful_stop` value, so a configured `graceful_stop: 30` produced an effective 10s drain on every Hybrid deployment. The parent `Container::Controller#stop(graceful)` already imposes the authoritative wall-clock deadline on fork exit via `Group#stop(graceful)` -> `Forked::Child.wait(timeout)` -> `kill!` escalation. A second, independent deadline inside each fork's ensure block adds nothing useful and actively races the parent's budget when both layers are configured equally (the most natural configuration). Both `false` (force-kill) and `true`/finite values are wrong here: `false` skips the graceful drain that the ensure path is meant to provide, and any finite value reintroduces the same race for some choice of outer budget. Pass `Float::INFINITY` so the inner threads exit at their own pace and the parent's escalation remains the single source of shutdown timing. The `rescue Async::Container::Terminate` branch's explicit `stop(false)` is unchanged -- Terminate is the force-kill path and bypasses the ensure-block drain. Test coverage: a new unit test in `test/async/container/hybrid.rb` pins the contract directly. It overrides `#spawn` so the run block executes in-process, stubs `Threaded.new` to capture the ensure-block `#stop` argument, and asserts `[Float::INFINITY]`. The existing `it_behaves_like Async::Container::AContainer` shared spec continues to exercise the real fork + SIGINT + drain path end-to-end. --- lib/async/container/hybrid.rb | 13 ++++++-- test/async/container/hybrid.rb | 59 ++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 3 deletions(-) diff --git a/lib/async/container/hybrid.rb b/lib/async/container/hybrid.rb index 736429b..b4b819b 100644 --- a/lib/async/container/hybrid.rb +++ b/lib/async/container/hybrid.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true # Released under the MIT License. -# Copyright, 2019-2025, by Samuel Williams. +# Copyright, 2019-2026, by Samuel Williams. # Copyright, 2022, by Anton Sozontov. require_relative "forked" @@ -37,8 +37,15 @@ def run(count: nil, forks: nil, threads: nil, health_check_timeout: nil, **optio container.stop(false) raise ensure - # Stop it gracefully (also code path for Interrupt): - container.stop + # Graceful drain on Interrupt. The inner per-fork container imposes + # no wall-clock deadline of its own — the parent's `Group#stop(graceful)` + # is the authoritative timer, via `Forked::Child.wait(timeout)` followed + # by `kill!` escalation if the fork doesn't exit within budget. A finite + # value here would race the parent's budget (most visibly when they're + # equal, e.g. configured at 30s on both layers); `Float::INFINITY` lets + # the inner threads exit at their own pace so the parent's escalation + # remains the single source of shutdown timing. + container.stop(Float::INFINITY) end end diff --git a/test/async/container/hybrid.rb b/test/async/container/hybrid.rb index ce9f615..2f5111e 100644 --- a/test/async/container/hybrid.rb +++ b/test/async/container/hybrid.rb @@ -14,3 +14,62 @@ expect(subject).to be(:multiprocess?) end end if Async::Container.fork? + +describe Async::Container::Hybrid do + # Pin the inner per-fork ensure-block contract without actually forking: override + # `#spawn` so the block runs in-process and stub `Threaded.new` so the inner + # `#stop` call can be observed. The parent `Group#stop(graceful)` is the + # authoritative deadline for fork exit (`Forked::Child.wait(timeout)` then + # `kill!`); the inner per-fork shutdown must therefore impose no deadline of + # its own, otherwise the two budgets race when configured equally. + with "inner per-fork shutdown timing" do + let(:inner_stop_calls) {[]} + + let(:threaded_double) do + double = Object.new + stops = inner_stop_calls + double.define_singleton_method(:run){|**|} + double.define_singleton_method(:wait_until_ready){} + double.define_singleton_method(:wait){raise Interrupt} + double.define_singleton_method(:stop){|arg = :__no_arg__| stops << arg} + double + end + + let(:fake_instance) do + instance = Object.new + instance.define_singleton_method(:ready!){} + instance + end + + def run_hybrid_inline(hybrid, instance:) + hybrid.define_singleton_method(:spawn) do |**options, &block| + begin + block.call(instance) + rescue Interrupt + # Swallow: simulates the forked process exiting on Interrupt. + end + end + hybrid.run(forks: 1, threads: 1, count: 1){} + end + + def with_threaded_stubbed(double) + original = Async::Container::Threaded.method(:new) + Async::Container::Threaded.singleton_class.send(:define_method, :new){|*, **, &| double} + begin + yield + ensure + Async::Container::Threaded.singleton_class.send(:remove_method, :new) + Async::Container::Threaded.singleton_class.send(:define_method, :new, original) + end + end + + it "drains the inner Threaded container without imposing its own deadline" do + with_threaded_stubbed(threaded_double) do + hybrid = Async::Container::Hybrid.new + run_hybrid_inline(hybrid, instance: fake_instance) + end + + expect(inner_stop_calls).to be == [Float::INFINITY] + end + end +end if Async::Container.fork?