Skip to content

fix(ws): surface HTTP status/body on relay dial failure - #134

Open
PrashantBtkl wants to merge 1 commit into
mainfrom
fix/ws-dial-error-diagnostics
Open

fix(ws): surface HTTP status/body on relay dial failure#134
PrashantBtkl wants to merge 1 commit into
mainfrom
fix/ws-dial-error-diagnostics

Conversation

@PrashantBtkl

Copy link
Copy Markdown
Contributor

Summary

  • On an EC2 host, forager logged repeated dial failed: websocket: bad handshake for ~30 minutes during a relay-side connectivity issue, with no way to tell whether the relay was down, rejecting auth, or rate limiting.
  • gorilla/websocket returns the HTTP response alongside ErrBadHandshake, but forager discarded it (conn, _, err := ...).
  • Now captures and logs the response status code and body (truncated to 1KB) so future incidents show e.g. status=502 vs status=401 vs status=429 instead of an opaque error.

Test plan

  • go build ./...
  • go vet ./pkg/ws/...
  • go test ./pkg/ws/... ./cmd/...

Bad-handshake errors from the relay were logged as an opaque
"websocket: bad handshake" with no indication of why (relay down,
auth rejected, rate limited, etc.). gorilla/websocket returns the
HTTP response alongside ErrBadHandshake; capture and log its status
code and body instead of discarding it.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request enhances error reporting during WebSocket connection failures by capturing and reading the HTTP response body and status code. The reviewer identified a potential nil pointer dereference panic if resp.Body is nil and provided a code suggestion to safely check resp.Body before reading or closing it.

Comment thread pkg/ws/client.go
Comment on lines +148 to +152
if resp != nil {
body, _ := io.ReadAll(io.LimitReader(resp.Body, 1024))
resp.Body.Close() // nolint:errcheck
return fmt.Errorf("dial failed: %w (status=%d body=%q)", err, resp.StatusCode, body)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To prevent potential nil pointer dereference panics, we should check if resp.Body is nil before attempting to read from or close it. Even if resp.Body is nil, we can still safely report the HTTP status code.

		if resp != nil {
			var body []byte
			if resp.Body != nil {
				body, _ = io.ReadAll(io.LimitReader(resp.Body, 1024))
				resp.Body.Close() // nolint:errcheck
			}
			return fmt.Errorf("dial failed: %w (status=%d body=%q)", err, resp.StatusCode, body)
		}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant