Improve server process cleanup and readiness checks (#199)

* Improve server process cleanup and readiness checks

- 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

Author: justin808

lib/cypress_on_rails/configuration.rb

@@ -22,6 +22,12 @@ 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
     def cypress_folder
@@ -62,6 +68,8 @@ 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', '/')
+      self.server_readiness_timeout = ENV.fetch('CYPRESS_RAILS_READINESS_TIMEOUT', '5').to_i
     end
 
     def tagged_logged

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
@@ -9,13 +10,16 @@ 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)
+      # Process management: track PID and process group for proper cleanup
+      @server_pid = nil
+      @server_pgid = nil
     end
 
     def open
@@ -105,34 +109,91 @@ 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)
+      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
+      @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?
+      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: timeout, read_timeout: timeout) do |http|
+        http.get(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
+      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(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
 
+    def send_term_signal(pid)
+      if @server_pgid && process_exists?(pid)
+        Process.kill('TERM', -@server_pgid)
+      else
+        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', 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, Errno::EPERM
+      # Process already terminated or permission denied
+    end
+
     def base_url
       "http://#{host}:#{port}"
     end

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,12 +24,16 @@
       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')
     expect(CypressOnRails.configuration.use_middleware?).to eq(false)
     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