From f74b41d8188caec4b85b434da8f25c723e995a59 Mon Sep 17 00:00:00 2001 From: Gord Pearson Date: Fri, 14 Aug 2026 14:03:05 -0400 Subject: [PATCH] Stop opening a null-device handle per Capture and spinner task Capture#initialize and SpinGroup#add both defaulted duplicate_output_to to a freshly opened File::NULL handle. Every capture, including one per spinner task, therefore opened an OS handle solely to discard writes. Ruby closes these handles when their File objects are finalized, but they remain open while captures or queued task closures are live. Default duplicate_output_to to nil and skip the duplicate write when unset. Type the option as io_like? so StringIO remains supported, and ignore IOError from caller-owned duplicate streams to match the module-level duplicate writer. The regression tests compare process descriptor sets while captures and pending spinner tasks remain live, asserting only on additions so unrelated File finalization cannot make the tests flaky. They also cover explicit StringIO duplication through Capture and SpinGroup, merged stderr, and a duplicate stream closed during capture. Co-authored-by: River Co-authored-by: Claude Fable 5 Assisted-By: devx/cceb8550-0aa0-4aa5-b0d0-bd55a129e07c --- lib/cli/ui/spinner/spin_group.rb | 6 +-- lib/cli/ui/stdout_router.rb | 10 +++-- test/cli/ui/spinner/spin_group_test.rb | 44 +++++++++++++++++++++ test/cli/ui/stdout_router_test.rb | 53 ++++++++++++++++++++++++++ test/test_helper.rb | 10 +++++ 5 files changed, 117 insertions(+), 6 deletions(-) diff --git a/lib/cli/ui/spinner/spin_group.rb b/lib/cli/ui/spinner/spin_group.rb index ab459921..898027d1 100644 --- a/lib/cli/ui/spinner/spin_group.rb +++ b/lib/cli/ui/spinner/spin_group.rb @@ -100,7 +100,7 @@ class Task # * +title+ - Title of the task # * +block+ - Block for the task, will be provided with an instance of the spinner # - #: (String title, final_glyph: ^(bool success) -> (Glyph | String), merged_output: bool, duplicate_output_to: IO, work_queue: WorkQueue) { (Task task) -> untyped } -> void + #: (String title, final_glyph: ^(bool success) -> (Glyph | String), merged_output: bool, duplicate_output_to: io_like?, work_queue: WorkQueue) { (Task task) -> untyped } -> void def initialize(title, final_glyph:, merged_output:, duplicate_output_to:, work_queue:, &block) @title = title @final_glyph = final_glyph @@ -300,12 +300,12 @@ def inset_width # spin_group.add('Title') { |spinner| sleep 1.0 } # spin_group.wait # - #: (String title, ?final_glyph: ^(bool success) -> (Glyph | String), ?merged_output: bool, ?duplicate_output_to: IO) { (Task task) -> void } -> void + #: (String title, ?final_glyph: ^(bool success) -> (Glyph | String), ?merged_output: bool, ?duplicate_output_to: io_like?) { (Task task) -> void } -> void def add( title, final_glyph: DEFAULT_FINAL_GLYPH, merged_output: false, - duplicate_output_to: File.new(File::NULL, 'w'), + duplicate_output_to: nil, &block ) @m.synchronize do diff --git a/lib/cli/ui/stdout_router.rb b/lib/cli/ui/stdout_router.rb index 20f8144b..97677d1d 100644 --- a/lib/cli/ui/stdout_router.rb +++ b/lib/cli/ui/stdout_router.rb @@ -184,11 +184,11 @@ def outermost_uncaptured? end end - #: (?with_frame_inset: bool, ?merged_output: bool, ?duplicate_output_to: IO) { -> void } -> void + #: (?with_frame_inset: bool, ?merged_output: bool, ?duplicate_output_to: io_like?) { -> void } -> void def initialize( with_frame_inset: true, merged_output: false, - duplicate_output_to: File.open(File::NULL, 'w'), + duplicate_output_to: nil, &block ) @with_frame_inset = with_frame_inset @@ -225,7 +225,11 @@ def run case stream when :stdout @out.write(data) - @duplicate_output_to.write(data) + begin + @duplicate_output_to&.write(data) + rescue IOError + # Ignore + end when :stderr @err.write(data) else raise diff --git a/test/cli/ui/spinner/spin_group_test.rb b/test/cli/ui/spinner/spin_group_test.rb index 4f262568..411b04d1 100644 --- a/test/cli/ui/spinner/spin_group_test.rb +++ b/test/cli/ui/spinner/spin_group_test.rb @@ -36,6 +36,50 @@ def test_spin_group_auto_debrief_false assert_equal('', err) end + def test_spin_group_tasks_open_no_descriptors + skip('/dev/fd unavailable') unless open_fds + + capture_io do + CLI::UI::StdoutRouter.ensure_activated + + warmup = SpinGroup.new(auto_debrief: false) + warmup.add('warmup') { true } + assert(warmup.wait) + + before = open_fds + release = Queue.new + sg = SpinGroup.new(auto_debrief: false, max_concurrent: 1) + 20.times { |i| sg.add("task #{i}") { release.pop } } + + while_running = open_fds + 20.times { release.push(true) } + assert(sg.wait) + after = open_fds + + assert_empty(while_running - before) # pending tasks hold no handles + assert_empty(after - before) + end + end + + def test_spin_group_task_duplicates_output_to_stream + dup = StringIO.new + + capture_io do + CLI::UI::StdoutRouter.ensure_activated + + sg = SpinGroup.new(auto_debrief: false) + sg.add('task', duplicate_output_to: dup) do + print('task output') + true + end + + assert(sg.wait) + end + + assert_equal('task output', dup.string) + refute_predicate(dup, :closed?) # the task doesn't own the handle + end + def test_spin_group_success_debrief capture_io do CLI::UI::StdoutRouter.ensure_activated diff --git a/test/cli/ui/stdout_router_test.rb b/test/cli/ui/stdout_router_test.rb index 23be5aa5..e73e88a2 100644 --- a/test/cli/ui/stdout_router_test.rb +++ b/test/cli/ui/stdout_router_test.rb @@ -24,6 +24,59 @@ def test_current_id end end + def test_capture_opens_no_descriptors + skip('/dev/fd unavailable') unless open_fds + + StdoutRouter::Capture.new {} # warm up any lazily-required files + before = open_fds + captures = Array.new(10) { StdoutRouter::Capture.new {} } + + assert_empty(open_fds - before) + refute_empty(captures) # keep them reachable so nothing is finalized early + end + + def test_capture_duplicate_output_to + capture_io do + StdoutRouter.with_enabled do + dup = StringIO.new + cap = StdoutRouter::Capture.new(duplicate_output_to: dup) { print('hello') } + cap.run + assert_equal('hello', cap.stdout) + assert_equal('hello', dup.string) + refute_predicate(dup, :closed?) # the capture doesn't own the handle + end + end + end + + def test_capture_duplicate_output_to_receives_merged_stderr + capture_io do + StdoutRouter.with_enabled do + dup = StringIO.new + cap = StdoutRouter::Capture.new(merged_output: true, duplicate_output_to: dup) do + $stderr.print('oops') + end + cap.run + assert_equal('oops', dup.string) + end + end + end + + def test_capture_ignores_closed_duplicate_output + capture_io do + StdoutRouter.with_enabled do + dup = StringIO.new + cap = StdoutRouter::Capture.new(duplicate_output_to: dup) do + dup.close + print('hello') + end + + cap.run + + assert_equal('hello', cap.stdout) + end + end + end + def test_frame_can_autoload_after_router_is_enabled script = <<~RUBY require 'stringio' diff --git a/test/test_helper.rb b/test/test_helper.rb index e3cdb17c..5695a192 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -56,6 +56,16 @@ def with_os_mock_and_reload(os, class_names = [], files = []) reset.call end +# The file descriptors this process currently holds open. Both macOS and Linux expose +# them as /dev/fd; returns nil elsewhere so callers can skip. Compare sets and assert +# only on additions: unrelated handles left by earlier tests get finalized at arbitrary +# points, which changes the count without anything having leaked. +def open_fds + Dir.children('/dev/fd') +rescue SystemCallError + nil +end + require 'fileutils' require 'tmpdir' require 'tempfile'