Conversation
KingPin
commented
Mar 10, 2026
- fix(sendRequest): handle nil GetBody, close response on retry, and constant-format logging
- test: use closed localhost port for deterministic network-error test
- docs: add copilot instructions and docker-only + atomic commits guidance
- refactor: pre-read request body when GetBody missing; improve retry detection (net/url errors)
…nstant-format logging
…etection (net/url errors)
There was a problem hiding this comment.
Pull request overview
This PR improves request retry robustness in the FanOut Go service by handling GetBody being nil, tightening retryable-network-error detection, and making CI/tests more deterministic, with accompanying documentation updates.
Changes:
- Update
multiplex/sendRequestto better support retries whenGetBodyis nil (pre-read body) and to close responses on retry. - Improve retryability detection using typed
net.Error/url.Errorchecks and adjust logging calls to use constant formats. - Add/adjust tests for deterministic network errors and
GetBody-nil scenarios; update Docker CI steps and add repo Copilot instructions.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
fanout.go |
Adds body pre-read fallback when GetBody is nil; improves retry error detection and retry cleanup; adjusts logging usage. |
fanout_test.go |
Makes the network-error test deterministic by using a closed localhost port. |
fanout_additional_test.go |
Adds tests for multiplex handling without GetBody and for retryable net.Error. |
.github/workflows/docker-image.yml |
Runs formatting/vet, gosec, and tests inside Docker in CI. |
.github/copilot-instructions.md |
Documents repo-specific build/test conventions and workflow preferences. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fanout.go
Outdated
| @@ -527,7 +556,7 @@ func sendRequest(ctx context.Context, client *http.Client, target string, origin | |||
| resp.Status = http.StatusInternalServerError | |||
| resp.Error = fmt.Sprintf("Failed to create request: %v", err) | |||
| bodyReader.Close() | |||
There was a problem hiding this comment.
bodyReader can be nil when getBody == nil, but on http.NewRequestWithContext error you unconditionally call bodyReader.Close(), which will panic. Guard the close with a nil check (or avoid closing here and rely on Transport when Do is called).
| bodyReader.Close() | |
| if bodyReader != nil { | |
| bodyReader.Close() | |
| } |
| run: | | ||
| go mod download | ||
| go test -v ./... | ||
| docker run --rm -v ${{ github.workspace }}:/src -w /src golang:1.24 sh -c "gofmt -l . || true; go vet ./... || true" |
There was a problem hiding this comment.
This step masks failures: go vet ./... || true will always succeed even when vet finds issues, and gofmt -l . won’t fail the job when files are unformatted. Make the step fail on vet/gofmt findings (e.g., remove || true for vet and explicitly error when gofmt -l outputs any files).
| docker run --rm -v ${{ github.workspace }}:/src -w /src golang:1.24 sh -c "gofmt -l . || true; go vet ./... || true" | |
| docker run --rm -v ${{ github.workspace }}:/src -w /src golang:1.24 sh -c 'set -e | |
| fmt_output=$(gofmt -l .) | |
| if [ -n "$fmt_output" ]; then | |
| echo "gofmt found unformatted files:" | |
| echo "$fmt_output" | |
| exit 1 | |
| fi | |
| go vet ./...' |
| // Create a request WITHOUT GetBody (http.NewRequest leaves GetBody nil) | ||
| body := []byte("hello") | ||
| req, err := http.NewRequest("POST", "/fanout", bytes.NewReader(body)) | ||
| if err != nil { | ||
| t.Fatalf("Failed to create request: %v", err) | ||
| } |
There was a problem hiding this comment.
This test claims http.NewRequest leaves GetBody nil, but for common in-memory body types like *bytes.Reader Go sets GetBody automatically. As written, this may not exercise the getBody == nil branch in multiplex; consider explicitly clearing req.GetBody or using a body type that doesn’t trigger automatic GetBody generation.
| // Create a request WITHOUT GetBody (http.NewRequest leaves GetBody nil) | |
| body := []byte("hello") | |
| req, err := http.NewRequest("POST", "/fanout", bytes.NewReader(body)) | |
| if err != nil { | |
| t.Fatalf("Failed to create request: %v", err) | |
| } | |
| // Create a request and explicitly clear GetBody to simulate a request without it | |
| body := []byte("hello") | |
| req, err := http.NewRequest("POST", "/fanout", bytes.NewReader(body)) | |
| if err != nil { | |
| t.Fatalf("Failed to create request: %v", err) | |
| } | |
| req.GetBody = nil // ensure GetBody is nil so multiplex exercises the no-GetBody path |
…r with timeouts to satisfy G114