Skip to content

Commit 5a1867d

Browse files
justin808claude
andcommitted
Fix critical issues in server process management
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 <[email protected]>
1 parent 5db98c3 commit 5a1867d

File tree

1 file changed

+13
-7
lines changed

1 file changed

+13
-7
lines changed

lib/cypress_on_rails/server.rb

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,8 @@ def spawn_server
113113
begin
114114
@server_pgid = Process.getpgid(@server_pid)
115115
rescue Errno::ESRCH => e
116+
# Edge case: process terminated before we could get pgid
117+
# This is OK - send_term_signal will fall back to single-process kill
116118
CypressOnRails.configuration.logger.warn("Process #{@server_pid} terminated immediately after spawn: #{e.message}")
117119
@server_pgid = nil
118120
end
@@ -133,14 +135,18 @@ def wait_for_server(timeout = 30)
133135
def server_responding?
134136
config = CypressOnRails.configuration
135137
readiness_path = config.server_readiness_path || '/'
136-
timeout = config.server_readiness_timeout || 5
138+
# Use shorter timeout for individual readiness checks since we poll in a loop
139+
# The configured timeout is for slow CI, but each check should be quick
140+
timeout = [config.server_readiness_timeout || 5, 2].min
137141
uri = URI("http://#{host}:#{port}#{readiness_path}")
138142

139143
response = Net::HTTP.start(uri.host, uri.port, open_timeout: timeout, read_timeout: timeout) do |http|
140-
http.get(uri.path)
144+
# Ensure path is never empty - default to '/'
145+
http.get(uri.path.empty? ? '/' : uri.path)
141146
end
142147

143148
# Accept 200-399 (success and redirects), reject 404 and 5xx
149+
# 3xx redirects are considered "ready" because the server is responding correctly
144150
(200..399).cover?(response.code.to_i)
145151
rescue Errno::ECONNREFUSED, Errno::EADDRNOTAVAIL, Errno::ETIMEDOUT, SocketError,
146152
Net::OpenTimeout, Net::ReadTimeout, Net::HTTPBadResponse
@@ -151,21 +157,21 @@ def stop_server(pid)
151157
return unless pid
152158

153159
puts "Stopping Rails server (PID: #{pid})"
154-
send_term_signal
160+
send_term_signal(pid)
155161
Process.wait(pid)
156162
rescue Errno::ESRCH
157163
# Process already terminated
158164
end
159165

160-
def send_term_signal
161-
if @server_pgid && process_exists?(@server_pid)
166+
def send_term_signal(pid)
167+
if @server_pgid && process_exists?(pid)
162168
Process.kill('TERM', -@server_pgid)
163169
else
164-
safe_kill_process('TERM', @server_pid)
170+
safe_kill_process('TERM', pid)
165171
end
166172
rescue Errno::ESRCH, Errno::EPERM => e
167173
CypressOnRails.configuration.logger.warn("Failed to kill process group #{@server_pgid}: #{e.message}, trying single process")
168-
safe_kill_process('TERM', @server_pid)
174+
safe_kill_process('TERM', pid)
169175
end
170176

171177
def process_exists?(pid)

0 commit comments

Comments
 (0)