From 145382971b87d269fe145a95c1af92ebc5251027 Mon Sep 17 00:00:00 2001 From: Justin Gordon Date: Wed, 8 Oct 2025 18:43:30 -1000 Subject: [PATCH 1/2] Improve bin/dev kill error handling for process termination MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Enhance the terminate_processes method to distinguish between ESRCH (process already stopped) and EPERM (permission denied) errors, providing clearer feedback to users when process termination fails. Key improvements: - Separate handling for ESRCH vs EPERM exceptions - User warning when permission is denied for a process - Clearer indication of what actually happened during process termination This change aligns with the pattern implemented in react_on_rails-demos PR #42 for better process management. Fixes #1858 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- lib/react_on_rails/dev/server_manager.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/react_on_rails/dev/server_manager.rb b/lib/react_on_rails/dev/server_manager.rb index 08587dbb2..0ac73b7e0 100644 --- a/lib/react_on_rails/dev/server_manager.rb +++ b/lib/react_on_rails/dev/server_manager.rb @@ -70,8 +70,12 @@ def find_process_pids(pattern) def terminate_processes(pids) pids.each do |pid| Process.kill("TERM", pid) - rescue StandardError + rescue Errno::ESRCH + # Process already stopped - this is fine nil + rescue Errno::EPERM + # Permission denied - warn the user + puts " ⚠️ Process #{pid} - permission denied (process owned by another user)" end end From dd852448ea96a94aacf4510293dee981e66b62df Mon Sep 17 00:00:00 2001 From: Justin Gordon Date: Wed, 8 Oct 2025 22:10:50 -1000 Subject: [PATCH 2/2] Add comprehensive test coverage and edge case handling for terminate_processes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Improvements based on PR feedback: 1. Test Coverage: - Add 7 new test cases for terminate_processes method - Cover ESRCH, EPERM, ArgumentError, RangeError scenarios - Test mixed success/error cases 2. Edge Case Handling: - Handle ArgumentError (invalid signal) - Handle RangeError (invalid PID) - Consistent with file_manager.rb patterns 3. Return Value Consistency: - All rescue branches now explicitly return nil - Consistent behavior across all error types 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- lib/react_on_rails/dev/server_manager.rb | 5 +- .../react_on_rails/dev/server_manager_spec.rb | 60 +++++++++++++++++++ 2 files changed, 63 insertions(+), 2 deletions(-) diff --git a/lib/react_on_rails/dev/server_manager.rb b/lib/react_on_rails/dev/server_manager.rb index 0ac73b7e0..29169e857 100644 --- a/lib/react_on_rails/dev/server_manager.rb +++ b/lib/react_on_rails/dev/server_manager.rb @@ -70,12 +70,13 @@ def find_process_pids(pattern) def terminate_processes(pids) pids.each do |pid| Process.kill("TERM", pid) - rescue Errno::ESRCH - # Process already stopped - this is fine + rescue Errno::ESRCH, ArgumentError, RangeError + # Process already stopped, or invalid signal/PID - silently skip nil rescue Errno::EPERM # Permission denied - warn the user puts " ⚠️ Process #{pid} - permission denied (process owned by another user)" + nil end end diff --git a/spec/react_on_rails/dev/server_manager_spec.rb b/spec/react_on_rails/dev/server_manager_spec.rb index 54226f642..fd1c6d184 100644 --- a/spec/react_on_rails/dev/server_manager_spec.rb +++ b/spec/react_on_rails/dev/server_manager_spec.rb @@ -203,6 +203,66 @@ def mock_system_calls end end + describe ".terminate_processes" do + it "successfully kills processes" do + pids = [1234, 5678] + expect(Process).to receive(:kill).with("TERM", 1234) + expect(Process).to receive(:kill).with("TERM", 5678) + + described_class.terminate_processes(pids) + end + + it "handles ESRCH (process not found) silently" do + pids = [1234] + allow(Process).to receive(:kill).with("TERM", 1234).and_raise(Errno::ESRCH) + + # Should not raise an error and should not output anything + expect { described_class.terminate_processes(pids) }.not_to raise_error + end + + it "handles EPERM (permission denied) with warning" do + pids = [1234] + allow(Process).to receive(:kill).with("TERM", 1234).and_raise(Errno::EPERM) + + # Should not raise an error but should output a warning + expect { described_class.terminate_processes(pids) }.to output(/permission denied/).to_stdout_from_any_process + end + + it "handles mixed success and ESRCH" do + pids = [1234, 5678] + expect(Process).to receive(:kill).with("TERM", 1234) + allow(Process).to receive(:kill).with("TERM", 5678).and_raise(Errno::ESRCH) + + expect { described_class.terminate_processes(pids) }.not_to raise_error + end + + it "handles mixed success and EPERM" do + pids = [1234, 5678] + expect(Process).to receive(:kill).with("TERM", 1234) + allow(Process).to receive(:kill).with("TERM", 5678).and_raise(Errno::EPERM) + + expect do + described_class.terminate_processes(pids) + end.to output(/5678.*permission denied/).to_stdout_from_any_process + end + + it "handles ArgumentError (invalid signal)" do + pids = [1234] + allow(Process).to receive(:kill).with("TERM", 1234).and_raise(ArgumentError) + + # Should not raise an error + expect { described_class.terminate_processes(pids) }.not_to raise_error + end + + it "handles RangeError (invalid PID)" do + pids = [999_999_999_999] + allow(Process).to receive(:kill).with("TERM", 999_999_999_999).and_raise(RangeError) + + # Should not raise an error + expect { described_class.terminate_processes(pids) }.not_to raise_error + end + end + describe ".show_help" do it "displays help information" do expect { described_class.show_help }.to output(%r{Usage: bin/dev \[command\]}).to_stdout_from_any_process