From 1672356a1107442b8afa896c51e85b854651e35f Mon Sep 17 00:00:00 2001 From: Justin Gordon Date: Sun, 5 Oct 2025 13:48:02 -1000 Subject: [PATCH 1/7] Improve server process cleanup and readiness checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add process group management for better child process cleanup - Add warning logging when process group kill fails with EPERM - Improve server readiness check to accept only 200-399 HTTP status codes - Reject 404 and 5xx responses as "not ready" for more reliable health checks These improvements provide better debugging visibility and reduce flaky tests by ensuring the server is truly ready before running tests. Fixes #198 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- lib/cypress_on_rails/server.rb | 47 ++++++++++++++++++++++++++-------- 1 file changed, 36 insertions(+), 11 deletions(-) diff --git a/lib/cypress_on_rails/server.rb b/lib/cypress_on_rails/server.rb index 00168ef..5e393c1 100644 --- a/lib/cypress_on_rails/server.rb +++ b/lib/cypress_on_rails/server.rb @@ -1,6 +1,7 @@ require 'socket' require 'timeout' require 'fileutils' +require 'net/http' require 'cypress_on_rails/configuration' module CypressOnRails @@ -105,30 +106,54 @@ def spawn_server puts "Starting Rails server: #{server_args.join(' ')}" - spawn(*server_args, out: $stdout, err: $stderr) + @server_pid = spawn(*server_args, out: $stdout, err: $stderr, pgroup: true) + @server_pgid = Process.getpgid(@server_pid) + @server_pid end def wait_for_server(timeout = 30) Timeout.timeout(timeout) do loop do - begin - TCPSocket.new(host, port).close - break - rescue Errno::ECONNREFUSED, Errno::EHOSTUNREACH - sleep 0.1 - end + break if server_responding? + sleep 0.1 end end rescue Timeout::Error raise "Rails server failed to start on #{host}:#{port} after #{timeout} seconds" end + def server_responding? + url = "http://#{host}:#{port}" + response = Net::HTTP.get_response(URI(url)) + # Accept 200-399 (success and redirects), reject 404 and 5xx + (200..399).cover?(response.code.to_i) + rescue Errno::ECONNREFUSED, Errno::EADDRNOTAVAIL, SocketError + false + end + def stop_server(pid) - if pid - puts "Stopping Rails server (PID: #{pid})" - Process.kill('TERM', pid) - Process.wait(pid) + return unless pid + + puts "Stopping Rails server (PID: #{pid})" + send_term_signal + Process.wait(pid) + rescue Errno::ESRCH + # Process already terminated + end + + def send_term_signal + if @server_pgid + Process.kill('TERM', -@server_pgid) + else + safe_kill_process('TERM', @server_pid) end + rescue Errno::ESRCH, Errno::EPERM => e + puts "Warning: Failed to kill process group #{@server_pgid}: #{e.message}, trying single process" + safe_kill_process('TERM', @server_pid) + end + + def safe_kill_process(signal, pid) + Process.kill(signal, pid) if pid rescue Errno::ESRCH # Process already terminated end From 12aa71aaa9f70e6d9a937608e4336a9b9b704ab3 Mon Sep 17 00:00:00 2001 From: Justin Gordon Date: Sun, 5 Oct 2025 14:08:41 -1000 Subject: [PATCH 2/7] Fix race conditions and improve error handling in server management MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Initialize @server_pid and @server_pgid in constructor to avoid nil issues - Add error handling for Process.getpgid race condition when process terminates immediately - Add comprehensive exception handling in server_responding? (ETIMEDOUT, Net::OpenTimeout, Net::ReadTimeout, Net::HTTPBadResponse) - Add configurable server_readiness_path with ENV var support (CYPRESS_RAILS_READINESS_PATH) - Add explicit HTTP timeouts (1s open_timeout, 1s read_timeout) to prevent hangs - Improve URI handling to support custom readiness check paths These fixes address potential edge cases and race conditions in server lifecycle management. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- lib/cypress_on_rails/configuration.rb | 2 ++ lib/cypress_on_rails/server.rb | 24 +++++++++++++++++++----- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/lib/cypress_on_rails/configuration.rb b/lib/cypress_on_rails/configuration.rb index a128052..f6887f3 100644 --- a/lib/cypress_on_rails/configuration.rb +++ b/lib/cypress_on_rails/configuration.rb @@ -22,6 +22,7 @@ class Configuration attr_accessor :server_host attr_accessor :server_port attr_accessor :transactional_server + attr_accessor :server_readiness_path # Attributes for backwards compatibility def cypress_folder @@ -62,6 +63,7 @@ def reset self.server_host = ENV.fetch('CYPRESS_RAILS_HOST', 'localhost') self.server_port = ENV.fetch('CYPRESS_RAILS_PORT', nil) self.transactional_server = true + self.server_readiness_path = ENV.fetch('CYPRESS_RAILS_READINESS_PATH', '/') end def tagged_logged diff --git a/lib/cypress_on_rails/server.rb b/lib/cypress_on_rails/server.rb index 5e393c1..25ad995 100644 --- a/lib/cypress_on_rails/server.rb +++ b/lib/cypress_on_rails/server.rb @@ -10,13 +10,15 @@ class Server def initialize(options = {}) config = CypressOnRails.configuration - + @framework = options[:framework] || :cypress @host = options[:host] || config.server_host @port = options[:port] || config.server_port || find_available_port @port = @port.to_i if @port @install_folder = options[:install_folder] || config.install_folder || detect_install_folder @transactional = options.fetch(:transactional, config.transactional_server) + @server_pid = nil + @server_pgid = nil end def open @@ -107,7 +109,12 @@ def spawn_server puts "Starting Rails server: #{server_args.join(' ')}" @server_pid = spawn(*server_args, out: $stdout, err: $stderr, pgroup: true) - @server_pgid = Process.getpgid(@server_pid) + begin + @server_pgid = Process.getpgid(@server_pid) + rescue Errno::ESRCH => e + puts "Warning: Process #{@server_pid} terminated immediately after spawn: #{e.message}" + @server_pgid = nil + end @server_pid end @@ -123,11 +130,18 @@ def wait_for_server(timeout = 30) end def server_responding? - url = "http://#{host}:#{port}" - response = Net::HTTP.get_response(URI(url)) + config = CypressOnRails.configuration + readiness_path = config.server_readiness_path || '/' + uri = URI("http://#{host}:#{port}#{readiness_path}") + + response = Net::HTTP.start(uri.host, uri.port, open_timeout: 1, read_timeout: 1) do |http| + http.get(uri.path.empty? ? '/' : uri.path) + end + # Accept 200-399 (success and redirects), reject 404 and 5xx (200..399).cover?(response.code.to_i) - rescue Errno::ECONNREFUSED, Errno::EADDRNOTAVAIL, SocketError + rescue Errno::ECONNREFUSED, Errno::EADDRNOTAVAIL, Errno::ETIMEDOUT, SocketError, + Net::OpenTimeout, Net::ReadTimeout, Net::HTTPBadResponse false end From 5db98c3ba14a602abdad30c53b92550b84dc954b Mon Sep 17 00:00:00 2001 From: Justin Gordon Date: Sun, 5 Oct 2025 14:34:20 -1000 Subject: [PATCH 3/7] Improve robustness and configurability of server management MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code quality improvements: - Add documentation comment for process management instance variables - Replace puts with logger.warn for consistency with gem conventions - Simplify URI path handling (remove redundant empty check) - Add process_exists? helper to validate PID before kill operations Error handling improvements: - Add EPERM to safe_kill_process rescue clause for complete error coverage - Validate process existence before attempting process group kill to avoid stale PGID issues - Prevent attempts to kill recycled PIDs Configuration improvements: - Add configurable server_readiness_timeout (default: 5 seconds) - Support CYPRESS_RAILS_READINESS_TIMEOUT environment variable - Addresses potential timeout issues on slow CI systems 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- lib/cypress_on_rails/configuration.rb | 2 ++ lib/cypress_on_rails/server.rb | 24 +++++++++++++++++------- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/lib/cypress_on_rails/configuration.rb b/lib/cypress_on_rails/configuration.rb index f6887f3..1e4e157 100644 --- a/lib/cypress_on_rails/configuration.rb +++ b/lib/cypress_on_rails/configuration.rb @@ -23,6 +23,7 @@ class Configuration attr_accessor :server_port attr_accessor :transactional_server attr_accessor :server_readiness_path + attr_accessor :server_readiness_timeout # Attributes for backwards compatibility def cypress_folder @@ -64,6 +65,7 @@ def reset self.server_port = ENV.fetch('CYPRESS_RAILS_PORT', nil) self.transactional_server = true self.server_readiness_path = ENV.fetch('CYPRESS_RAILS_READINESS_PATH', '/') + self.server_readiness_timeout = ENV.fetch('CYPRESS_RAILS_READINESS_TIMEOUT', '5').to_i end def tagged_logged diff --git a/lib/cypress_on_rails/server.rb b/lib/cypress_on_rails/server.rb index 25ad995..301cdd6 100644 --- a/lib/cypress_on_rails/server.rb +++ b/lib/cypress_on_rails/server.rb @@ -17,6 +17,7 @@ def initialize(options = {}) @port = @port.to_i if @port @install_folder = options[:install_folder] || config.install_folder || detect_install_folder @transactional = options.fetch(:transactional, config.transactional_server) + # Process management: track PID and process group for proper cleanup @server_pid = nil @server_pgid = nil end @@ -112,7 +113,7 @@ def spawn_server begin @server_pgid = Process.getpgid(@server_pid) rescue Errno::ESRCH => e - puts "Warning: Process #{@server_pid} terminated immediately after spawn: #{e.message}" + CypressOnRails.configuration.logger.warn("Process #{@server_pid} terminated immediately after spawn: #{e.message}") @server_pgid = nil end @server_pid @@ -132,10 +133,11 @@ def wait_for_server(timeout = 30) def server_responding? config = CypressOnRails.configuration readiness_path = config.server_readiness_path || '/' + timeout = config.server_readiness_timeout || 5 uri = URI("http://#{host}:#{port}#{readiness_path}") - response = Net::HTTP.start(uri.host, uri.port, open_timeout: 1, read_timeout: 1) do |http| - http.get(uri.path.empty? ? '/' : uri.path) + response = Net::HTTP.start(uri.host, uri.port, open_timeout: timeout, read_timeout: timeout) do |http| + http.get(uri.path) end # Accept 200-399 (success and redirects), reject 404 and 5xx @@ -156,20 +158,28 @@ def stop_server(pid) end def send_term_signal - if @server_pgid + if @server_pgid && process_exists?(@server_pid) Process.kill('TERM', -@server_pgid) else safe_kill_process('TERM', @server_pid) end rescue Errno::ESRCH, Errno::EPERM => e - puts "Warning: Failed to kill process group #{@server_pgid}: #{e.message}, trying single process" + CypressOnRails.configuration.logger.warn("Failed to kill process group #{@server_pgid}: #{e.message}, trying single process") safe_kill_process('TERM', @server_pid) end + def process_exists?(pid) + return false unless pid + Process.kill(0, pid) + true + rescue Errno::ESRCH, Errno::EPERM + false + end + def safe_kill_process(signal, pid) Process.kill(signal, pid) if pid - rescue Errno::ESRCH - # Process already terminated + rescue Errno::ESRCH, Errno::EPERM + # Process already terminated or permission denied end def base_url From 5a1867d93ed07a34976c46cb6b923817afee71be Mon Sep 17 00:00:00 2001 From: Justin Gordon Date: Sun, 5 Oct 2025 20:01:27 -1000 Subject: [PATCH 4/7] Fix critical issues in server process management MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Critical fixes: 1. Fix PID parameter passing in stop_server/send_term_signal - Previously stop_server(pid) received parameter but used instance variable - Now consistently passes pid parameter through the call chain - Prevents potential bugs if pid values diverge 2. Fix HTTP timeout in readiness polling loop - Cap individual check timeout at 2 seconds max - Prevents 5s timeouts from blocking the 0.1s polling loop - Maintains responsiveness while supporting slow CI via configuration 3. Fix URI path handling edge case - Ensure GET path defaults to '/' if uri.path is empty - Prevents invalid HTTP requests with empty path Documentation improvements: - Document that @server_pgid may be nil in edge cases - Clarify that 3xx redirects are considered valid "ready" state - Add comments explaining timeout behavior in polling context 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- lib/cypress_on_rails/server.rb | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/lib/cypress_on_rails/server.rb b/lib/cypress_on_rails/server.rb index 301cdd6..aae596b 100644 --- a/lib/cypress_on_rails/server.rb +++ b/lib/cypress_on_rails/server.rb @@ -113,6 +113,8 @@ def spawn_server begin @server_pgid = Process.getpgid(@server_pid) rescue Errno::ESRCH => e + # Edge case: process terminated before we could get pgid + # This is OK - send_term_signal will fall back to single-process kill CypressOnRails.configuration.logger.warn("Process #{@server_pid} terminated immediately after spawn: #{e.message}") @server_pgid = nil end @@ -133,14 +135,18 @@ def wait_for_server(timeout = 30) def server_responding? config = CypressOnRails.configuration readiness_path = config.server_readiness_path || '/' - timeout = config.server_readiness_timeout || 5 + # Use shorter timeout for individual readiness checks since we poll in a loop + # The configured timeout is for slow CI, but each check should be quick + timeout = [config.server_readiness_timeout || 5, 2].min uri = URI("http://#{host}:#{port}#{readiness_path}") response = Net::HTTP.start(uri.host, uri.port, open_timeout: timeout, read_timeout: timeout) do |http| - http.get(uri.path) + # Ensure path is never empty - default to '/' + http.get(uri.path.empty? ? '/' : uri.path) end # Accept 200-399 (success and redirects), reject 404 and 5xx + # 3xx redirects are considered "ready" because the server is responding correctly (200..399).cover?(response.code.to_i) rescue Errno::ECONNREFUSED, Errno::EADDRNOTAVAIL, Errno::ETIMEDOUT, SocketError, Net::OpenTimeout, Net::ReadTimeout, Net::HTTPBadResponse @@ -151,21 +157,21 @@ def stop_server(pid) return unless pid puts "Stopping Rails server (PID: #{pid})" - send_term_signal + send_term_signal(pid) Process.wait(pid) rescue Errno::ESRCH # Process already terminated end - def send_term_signal - if @server_pgid && process_exists?(@server_pid) + def send_term_signal(pid) + if @server_pgid && process_exists?(pid) Process.kill('TERM', -@server_pgid) else - safe_kill_process('TERM', @server_pid) + safe_kill_process('TERM', pid) end rescue Errno::ESRCH, Errno::EPERM => e CypressOnRails.configuration.logger.warn("Failed to kill process group #{@server_pgid}: #{e.message}, trying single process") - safe_kill_process('TERM', @server_pid) + safe_kill_process('TERM', pid) end def process_exists?(pid) From 8e2b94424fe38e92532d8ddf2ac044bd74e1fc2d Mon Sep 17 00:00:00 2001 From: Justin Gordon Date: Sun, 5 Oct 2025 20:18:35 -1000 Subject: [PATCH 5/7] Respect user-configured readiness timeout and simplify URI handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove timeout cap that prevented honoring user-specified timeouts - Previously capped at 2 seconds with [config.server_readiness_timeout || 5, 2].min - Now respects full configured timeout value (default: 5 seconds) - Remove redundant uri.path.empty? check since readiness_path defaults to '/' This allows users to configure longer timeouts for slow environments while maintaining clean, simple code. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- lib/cypress_on_rails/server.rb | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/lib/cypress_on_rails/server.rb b/lib/cypress_on_rails/server.rb index aae596b..a9c8a8e 100644 --- a/lib/cypress_on_rails/server.rb +++ b/lib/cypress_on_rails/server.rb @@ -135,14 +135,11 @@ def wait_for_server(timeout = 30) def server_responding? config = CypressOnRails.configuration readiness_path = config.server_readiness_path || '/' - # Use shorter timeout for individual readiness checks since we poll in a loop - # The configured timeout is for slow CI, but each check should be quick - timeout = [config.server_readiness_timeout || 5, 2].min + timeout = config.server_readiness_timeout || 5 uri = URI("http://#{host}:#{port}#{readiness_path}") response = Net::HTTP.start(uri.host, uri.port, open_timeout: timeout, read_timeout: timeout) do |http| - # Ensure path is never empty - default to '/' - http.get(uri.path.empty? ? '/' : uri.path) + http.get(uri.path) end # Accept 200-399 (success and redirects), reject 404 and 5xx From bda37554e1833101f09884e99fa32a814abe201f Mon Sep 17 00:00:00 2001 From: Justin Gordon Date: Sun, 5 Oct 2025 20:19:42 -1000 Subject: [PATCH 6/7] Add timeout and KILL fallback to server shutdown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add 10-second timeout to Process.wait to prevent indefinite hangs - Send KILL signal if server doesn't respond to TERM within timeout - Prevents test runs from hanging if server doesn't shut down gracefully - Logs warning when escalating from TERM to KILL This ensures robust cleanup even when servers are unresponsive to graceful shutdown signals. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- lib/cypress_on_rails/server.rb | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/lib/cypress_on_rails/server.rb b/lib/cypress_on_rails/server.rb index a9c8a8e..15390f0 100644 --- a/lib/cypress_on_rails/server.rb +++ b/lib/cypress_on_rails/server.rb @@ -155,7 +155,16 @@ def stop_server(pid) puts "Stopping Rails server (PID: #{pid})" send_term_signal(pid) - Process.wait(pid) + + begin + Timeout.timeout(10) do + Process.wait(pid) + end + rescue Timeout::Error + CypressOnRails.configuration.logger.warn("Server did not terminate after TERM signal, sending KILL") + safe_kill_process('KILL', pid) + Process.wait(pid) rescue Errno::ESRCH + end rescue Errno::ESRCH # Process already terminated end From 501b0b47e5298d4663b9cf7bdca60815b1dd2fbc Mon Sep 17 00:00:00 2001 From: Justin Gordon Date: Sun, 5 Oct 2025 20:42:28 -1000 Subject: [PATCH 7/7] Add documentation and tests for new configuration options MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add inline documentation for server_readiness_path and server_readiness_timeout - Document default values and environment variable names - Add test coverage for default values - Add test coverage for custom configuration This improves discoverability and ensures configuration options are properly tested. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- lib/cypress_on_rails/configuration.rb | 4 ++++ spec/cypress_on_rails/configuration_spec.rb | 6 ++++++ 2 files changed, 10 insertions(+) diff --git a/lib/cypress_on_rails/configuration.rb b/lib/cypress_on_rails/configuration.rb index 1e4e157..9c112ed 100644 --- a/lib/cypress_on_rails/configuration.rb +++ b/lib/cypress_on_rails/configuration.rb @@ -22,7 +22,11 @@ class Configuration attr_accessor :server_host attr_accessor :server_port attr_accessor :transactional_server + # HTTP path to check for server readiness (default: '/') + # Can be set via CYPRESS_RAILS_READINESS_PATH environment variable attr_accessor :server_readiness_path + # Timeout in seconds for individual HTTP readiness checks (default: 5) + # Can be set via CYPRESS_RAILS_READINESS_TIMEOUT environment variable attr_accessor :server_readiness_timeout # Attributes for backwards compatibility diff --git a/spec/cypress_on_rails/configuration_spec.rb b/spec/cypress_on_rails/configuration_spec.rb index f790abf..3652e8d 100644 --- a/spec/cypress_on_rails/configuration_spec.rb +++ b/spec/cypress_on_rails/configuration_spec.rb @@ -10,6 +10,8 @@ expect(CypressOnRails.configuration.logger).to_not be_nil expect(CypressOnRails.configuration.before_request).to_not be_nil expect(CypressOnRails.configuration.vcr_options).to eq({}) + expect(CypressOnRails.configuration.server_readiness_path).to eq('/') + expect(CypressOnRails.configuration.server_readiness_timeout).to eq(5) end it 'can be configured' do @@ -22,6 +24,8 @@ config.logger = my_logger config.before_request = before_request_lambda config.vcr_options = { hook_into: :webmock } + config.server_readiness_path = '/health' + config.server_readiness_timeout = 10 end expect(CypressOnRails.configuration.api_prefix).to eq('/api') expect(CypressOnRails.configuration.install_folder).to eq('my/path') @@ -29,5 +33,7 @@ expect(CypressOnRails.configuration.logger).to eq(my_logger) expect(CypressOnRails.configuration.before_request).to eq(before_request_lambda) expect(CypressOnRails.configuration.vcr_options).to eq(hook_into: :webmock) + expect(CypressOnRails.configuration.server_readiness_path).to eq('/health') + expect(CypressOnRails.configuration.server_readiness_timeout).to eq(10) end end