Fix response body leak in labs GitHub client#5519
Merged
Merged
Conversation
Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Collaborator
|
Commit: c04093c
22 interesting tests: 15 SKIP, 7 KNOWN
Top 28 slowest tests (at least 2 minutes):
|
asnare
reviewed
Jun 10, 2026
asnare
left a comment
asnare
left a comment
Collaborator
There was a problem hiding this comment.
The fix looks right to me. (Issue analysis is correct: we hit GitHub errors quite regularly due to per-IP rate limits.)
There's a small issue with the test: it mutates global state (the process-wide default HTTP client) which is fragile and a bit of a smell. Personally I'd avoid that by widening the scope of the PR to inject a fresh client into getPagedBytes() but that's a judgement call. (This could mean introducing WithHTTPClient to match the pattern of WithApiOverride and WithUserContentOverride, used by other tests in this package.)
asnare
approved these changes
Jun 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Found during a full-repo review of the CLI. In
cmd/labs/github/github.go,getPagedBytesregistersdefer res.Body.Close()only after the early returns for 404 and other 4xx/5xx responses. On those paths the response body is never closed, which leaks the connection. The labs update check polls the GitHub API unauthenticated, where 403 rate-limit responses are common, so these error paths are hit in practice.Changes
Before, the response body was only closed on successful responses; now it is closed on every path. The fix moves the
defer res.Body.Close()to immediately after thehttp.DefaultClient.Doerror check, before the status-code checks.Also adds a unit test that stubs the HTTP transport and asserts the body is closed when the request returns 404 or 500. The test fails on the previous code.
Test plan
TestGetPagedBytesClosesBodyOnHTTPErrorpasses, and was verified to fail against the unfixed codego test ./cmd/labs/...passes./task fmt-q,./task lint-q, and./task checkspassThis pull request and its description were written by Isaac.