Fix a response body leak and remove Clone in Workhorse httprs
What does this MR do and why?
HTTPReadSeeker in workhorse/internal/httprs/httprs.go is the reader gitlab-zip-cat and gitlab-zip-metadata use to read a remote zip artifact with range requests (open_archive.go#L90). This fixes a response body leak in it and removes its unused Clone method, one commit each.
The body of a rejected range response is never closed. When a range request is answered with 416 Requested Range Not Satisfiable, or with a 200 OK that cannot serve the requested range (the reader has moved past the start, or the ETag changed), rangeRequest returns ErrInvalidRange or ErrContentHasChanged and drops the response without closing its body, which keeps the response open until the request's context ends (over HTTP/1.1, together with the connection behind it, which cannot serve another request in the meantime). Any other unexpected status already closes the body before returning ErrRangeRequestsNotSupported (httprs.go#L217). The first commit closes the body on these two paths as well. Both commands are short-lived, so today such a response stays open until the command finishes; the fix matters for correctness and for any longer-lived caller. Successful reads behave as before.
Clone() is removed. Nothing in Workhorse calls Clone: open_archive.go is the only user of the package and calls only NewHTTPReadSeeker. Clone copies the request and the response but not the context that NewHTTPReadSeeker stores (httprs.go#L67), and has missed it since the package was vendored in gitlab-workhorse!550 (merged): the first Read on a clone builds its range request with r.req.WithContext(r.ctx) and panics with nil context. The second commit removes it rather than fix a method nothing calls (an earlier version of this MR copied the context into it instead). Clone was the only importer of github.com/mitchellh/copystructure, so that module and github.com/mitchellh/reflectwalk, which only copystructure required, leave go.mod and go.sum, and copystructure leaves the depguard allowlist in workhorse/.golangci.yml. httprs is an internal package, so only this module could call Clone, and its build and go vet ./..., which also compiles the tests, pass without it.
Test:
TestRangeRequestClosesRejectedResponseBody: a stub transport answers the range request with416, with200after a seek, with200and a changedETag, and with500. Each case checks the error returned and that the response body was closed. The500case already passes onmasterand keeps the existing path covered.
References
Closes #633775 (closed)
Screenshots or screen recordings
Not applicable: no UI change.
How to set up and validate locally
- In
workhorse/, rungo test ./internal/httprs/ -run TestRangeRequestClosesRejectedResponseBody -count=1 -v. The test passes. - Run
git show HEAD~1 -- internal/httprs/httprs.go | git apply -R, which takes out the first commit's fix, and the same command again. The test fails for the416case and both200cases with "the body of the rejected range response was not closed". git checkout -- internal/httprs/httprs.gorestores the branch's file.go build ./...passes,go mod tidyleavesgo.modandgo.sumunchanged, andgit grep -n 'copystructure\|reflectwalk' -- .finds nothing.
What I ran (base 52872bb5)
The commands of the Workhorse CI jobs on the Go versions they use, in golang images with exiftool added, with config.toml built from config.toml.example as the jobs build it, and a Redis 7.2 service beside it:
make verifyon Go 1.26.10 (theworkhorse:verifyjob): lint (golangci-lint 2.11.4,depguardincluded, against the empty baseline), vet,detect-context,detect-external-tests,check-formatting,staticcheck2026.1, andgo mod tidyleavinggo.modandgo.sumunchanged.make test-raceon Go 1.26.10, andmake teston Go 1.25.14, Go 1.26.10 and Go 1.27.3: every package passes. Gitaly was not running, so the Gitaly integration tests did not run.- The branch merged with
masterat 660726b1, which changedgo.mod,go.sumand.golangci.ymlsince the base: the merge is clean and passes the samemake verify, andgo testandgo vetoninternal/httprsandinternal/zipartifacts. - On Go 1.25.12 (
.tool-versions): the leak test fails against the code before its fix and passes with it, on the first commit and at the head; each commit on its own passesgo build ./..., andgo testandgo vetoninternal/httprsandinternal/zipartifacts; the leak test passes with-race -count=50;internal/httprs,internal/zipartifactsand the two zip commands pass with-race;go mod tidychanges nothing there or on Go 1.27.3, and nogithub.com/mitchellhpackage is left in the build or test graph. - Two mutants (either new
Closecall removed) each make the leak test fail. - Both commit messages pass
scripts/lint/commit_linter.rbwithgitlab-dangerfiles4.13.0.
The new test on master's httprs.go:
Messages: the body of the rejected range response was not closed
--- FAIL: TestRangeRequestClosesRejectedResponseBody (0.00s)
--- FAIL: TestRangeRequestClosesRejectedResponseBody/416_Requested_Range_Not_Satisfiable (0.00s)
--- FAIL: TestRangeRequestClosesRejectedResponseBody/200_OK_after_a_seek (0.00s)
--- FAIL: TestRangeRequestClosesRejectedResponseBody/200_OK_with_a_changed_ETag (0.00s)
--- PASS: TestRangeRequestClosesRejectedResponseBody/unexpected_status (0.00s)MR acceptance checklist
Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.