fix(net): report the status code when the response body cannot be read

[JspIIV]

What

GetCurl's doc comment promises:

check status code first. in some cases response can be different, so unmarshalling will fail
status code return as is even in unmarshalling error

The json.Unmarshal failure path in executeRequest honours that and returns resp.StatusCode. The io.ReadAll failure path five lines above returns 0 instead.

b, err := io.ReadAll(resp.Body)
if err != nil {
    return res, 0, fmt.Errorf("failed to read response body: %w", err)   // <- 0
}
var result T
if strings.TrimSpace(...) != "" {
    if err = json.Unmarshal(b, &result); err != nil {
        return res, resp.StatusCode, fmt.Errorf(...)                     // <- real status
    }
}

A body read fails on a truncated response, a mid-body connection reset, or a client timeout that expires while the body is streaming — all of which happen against a server that did reply. In those cases the caller sees 0, which is the same value it gets when the request never reached the server, so it cannot tell the two apart or decide whether the status is worth retrying.

This already shows up inside the package. getIPViaTraceURL wraps the result as:

return "", fmt.Errorf("request failed, code: %d, err: %w", code, err)

with a 10s client timeout, so a slow trace endpoint that answered 200 and then stalled mid-body is logged as code: 0.

Fix

Return resp.StatusCode on that path too, matching the sibling error return and the documented contract.

Test

TestCurlReportsStatusWhenBodyReadFails uses a stub RoundTripper that returns a 503 with a body that fails on first Read, so it is deterministic and needs no network.

Against main it fails:

Error:      Not equal:
            expected: 503
            actual  : 0
Messages:   status code should survive a body read failure

With the fix it passes.

Note: pkg/net has three pre-existing failures on Windows unrelated to this change — TestGetCurl/TestPostCurl/TestPatchCurl assert the Linux dial error string connect: connection refused, while Windows reports connectex: No connection could be made.... They fail on untouched main too. Happy to open a separate issue if that is useful.

Summary by CodeRabbit

• Bug Fixes

  • Preserved the HTTP status code when a response body cannot be read.
  • Improved error responses for failed body reads, including service-unavailable responses.

• Tests

  • Added coverage to verify that read failures return the correct status code and no response data.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: getoptimum/coderabbit/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: e20fa3c2-a65b-45cf-894f-fa2358e968d0

📥 Commits

Reviewing files that changed from the base of the PR and between 4674354 and 75a7ab4.

📒 Files selected for processing (2)

• pkg/net/request.go
• pkg/net/request_test.go

Included review availability: 4 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour.

---

📝 Walkthrough

Walkthrough

executeRequest now returns the HTTP response status code when reading the response body fails. Tests add an unreadable response body and verify that GetCurl returns the read error, status code 503, and no response value.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Merge Risk: 🟡 Moderate · up to 75a7a

The new regression test currently contains a duplicate type declaration, so the test package will not compile and the PR is not merge-ready until that declaration is removed.

Suggested reviewers: abergasov, hpsing, swarna1101

🚥 Pre-merge checks | ✅ 8 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (8 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title follows the required format, uses the valid type and package domain, states the fix clearly, is 70 characters long, and has no trailing punctuation.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Scope Discipline	✅ Passed	The pull request changes only pkg/net/request.go and its focused test file, pkg/net/request_test.go. The implementation changes one return value in the response-body read error path. The added `Ro…
Behavior Safety	✅ Passed	The one-line change preserves resp.StatusCode only after client.Do has returned a response and io.ReadAll fails. Transport/request failures still return 0, while the existing JSON-unmarshal an…
Over-Engineering	✅ Passed	PASS. The production change is a single return-value correction and adds no cache, helper layer, or signature churn. The test uses small local transport/body stubs only to create a deterministic read …
Security	✅ Passed	PASS — The pull request changes only the returned HTTP status code on response-body read failure and adds a deterministic test. The diff introduces no injection handling, credentials, cryptographic co…

Full details: Scope Discipline

Explanation

The pull request changes only pkg/net/request.go and its focused test file, pkg/net/request_test.go. The implementation changes one return value in the response-body read error path. The added RoundTripper and failing ReadCloser test helpers support the stated deterministic regression test. No unrelated files, dependencies, refactors, or unexplained scope expansion were found.

Full details: Behavior Safety

Explanation

The one-line change preserves resp.StatusCode only after client.Do has returned a response and io.ReadAll fails. Transport/request failures still return 0, while the existing JSON-unmarshal and decoder paths already return the response status. The focused test uses a deterministic 503 response with a failing body and checks the error, status, and nil result. Downstream callers treat the status as diagnostic metadata and no invariant is broken.

Full details: Over-Engineering

Explanation

PASS. The production change is a single return-value correction and adds no cache, helper layer, or signature churn. The test uses small local transport/body stubs only to create a deterministic read failure, and it checks observable results: the error, HTTP status, and nil response. It does not assert internal implementation details.

Full details: Security

Explanation

PASS — The pull request changes only the returned HTTP status code on response-body read failure and adds a deterministic test. The diff introduces no injection handling, credentials, cryptographic code, sensitive logging, or secrets in manifests. The test uses only the literal status 503 and the error text "connection reset".

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}