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 with 416, with 200 after a seek, with 200 and a changed ETag, and with 500. Each case checks the error returned and that the response body was closed. The 500 case already passes on master and 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

  1. In workhorse/, run go test ./internal/httprs/ -run TestRangeRequestClosesRejectedResponseBody -count=1 -v. The test passes.
  2. 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 the 416 case and both 200 cases with "the body of the rejected range response was not closed".
  3. git checkout -- internal/httprs/httprs.go restores the branch's file.
  4. go build ./... passes, go mod tidy leaves go.mod and go.sum unchanged, and git 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 verify on Go 1.26.10 (the workhorse:verify job): lint (golangci-lint 2.11.4, depguard included, against the empty baseline), vet, detect-context, detect-external-tests, check-formatting, staticcheck 2026.1, and go mod tidy leaving go.mod and go.sum unchanged.
  • make test-race on Go 1.26.10, and make test on 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 master at 660726b1, which changed go.mod, go.sum and .golangci.yml since the base: the merge is clean and passes the same make verify, and go test and go vet on internal/httprs and internal/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 passes go build ./..., and go test and go vet on internal/httprs and internal/zipartifacts; the leak test passes with -race -count=50; internal/httprs, internal/zipartifacts and the two zip commands pass with -race; go mod tidy changes nothing there or on Go 1.27.3, and no github.com/mitchellh package is left in the build or test graph.
  • Two mutants (either new Close call removed) each make the leak test fail.
  • Both commit messages pass scripts/lint/commit_linter.rb with gitlab-dangerfiles 4.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.

Edited by José M. Requena Plens

Merge request reports

Loading
Loading