Repository navigation
Fix inverted HEAD response and fd leak in index server - #338
Merged
Merged
Conversation
HTTPIndexHandler.head had inverted status logic: it returned 200 OK when GetIndexReader failed and 404 Not Found when it succeeded. A client doing HEAD-before-PUT to avoid clobbering would see 404 for an existing index and overwrite it. It also discarded the io.ReadCloser returned by GetIndexReader without closing it. For LocalIndexStore that reader is the *os.File from os.Open, so every HEAD to an existing index leaked a file descriptor until GC finalization, allowing a sustained HEAD burst to exhaust the process fd limit. Correct the status mapping to mirror the sibling GET handler (os.IsNotExist -> 404, other error -> stderr log + 400, success -> 200) and close the reader deterministically with defer. Add a test covering the existing-index and missing-index HEAD paths.
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
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.
Problem
HTTPIndexHandler.head(httpindexhandler.go) had two defects:200 OKwhenGetIndexReaderfailed and404 Not Foundwhen it succeeded. A client that does HEAD-before-PUT to avoid clobbering an index would see404for an index that exists and overwrite it.io.ReadCloserreturned byGetIndexReaderwas discarded into_and never closed. ForLocalIndexStorethis reader is the*os.Filefromos.Open, so every HEAD to an existing index leaked one fd until GC finalization. A sustained HEAD burst against a known index name could exhaust the process fd limit (EMFILE), breakingAccept()and legitimate GET/PUT traffic.Fix
gethandler in the same file:os.IsNotExist(err)→404, other error → stderr log +400, success →200. This keeps HEAD and GET consistent for the same name and correctly detects the missing-file case forLocalIndexStore(the index-server default backing).defer rdr.Close(), matching thedefer obj.Close()pattern used by everyGetIndeximplementation, eliminating the fd leak.TestIndexServerHeadcovering the existing-index (→ 200) and missing-index (→ 404) paths, which the existing suite never exercised.Verification
go build ./cmd/desync— OKgo test ./cmd/desync -run TestIndexServer -v— all pass, including the newTestIndexServerHeadgo test ./...— full suite green, no regressionsgofmt -l— clean