utils/curl: request headers when body is unused - #23528
Conversation
There was a problem hiding this comment.
Pull request overview
This PR optimizes curl_check_http_content to avoid downloading full response bodies when they are not needed (notably for HTTPS reachability checks), reducing unnecessary network transfer during audits while preserving existing comparison behavior for HTTP→HTTPS checks.
Changes:
- Switch HTTPS-only reachability checks to request headers (
HEAD) instead of downloading bodies. - Add a fallback to retry with
GETwhen a server rejectsHEAD. - Add unit tests covering the new
head_onlybehavior and retry logic.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
Library/Homebrew/utils/curl.rb |
Adds head_only support to avoid body downloads on HTTPS checks and introduces retry behavior. |
Library/Homebrew/test/utils/curl_spec.rb |
Adds coverage for head_only curl args and HEAD→GET retry behavior. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
p-linnane
left a comment
p-linnane
left a comment
There was a problem hiding this comment.
Thanks, nice win. The HEAD→GET fallback on any failure status is the right call (servers reject HEAD with all sorts of codes, 403 from WAFs especially, and retrying broadly means we still report the GET's status like before).
One change I'd like: a server that drops HEAD without ever sending a status line (curl exit 52/56/28) never gets the GET fallback. It just re-HEADs through the retry loop and reports "not reachable". The "failure was below HTTP" comment in the test doesn't quite hold there; the server has already seen the method by the time it hangs up. Could you extend the fallback to also flip to GET when there's no status code and the exit status is 28/52/56? DNS/connect failures (6/7) can stay as-is since those really are method-independent.
On the Copilot comments: I'd skip both. Narrowing the retry to 405/501 would turn HEAD-hostile 403s into false audit failures, and the unused Tempfile on the HEAD path is negligible next to the network round trip.
8d43118 to
fe19a01
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (4)
Library/Homebrew/utils/curl.rb:609
curl_http_content_headers_and_checksumalways creates aTempfile, even whenhead_onlyis true and the body is never written/read. This adds avoidable filesystem work on the new fast-path. Consider only allocating the Tempfile when!head_onlyand making theensurecleanup conditional (e.g.file&.unlink).
file = Tempfile.new.tap(&:close)
Library/Homebrew/utils/curl.rb:496
head_rejectedcurrently treats any non-1xx/2xxstatus_codefrom theHEADresponse as “HEAD rejected”, which causes an unconditional retry as aGET(and potentially a full download) for genuine failures like 404/500. This adds an extra request without improving the outcome. Restrict theHEAD→GETfallback to status codes that indicate the method is not supported (e.g. 405/501), keeping the existing exit-status-based fallback for cases where no HTTP status is available.
head_rejected = if (status_code = details[:status_code])
!http_status_ok?(status_code)
else
CURL_REQUEST_SENT_EXIT_CODES.include?(details[:exit_status])
end
Library/Homebrew/utils/curl.rb:491
- This comment is incomplete (trailing comma) and doesn’t clearly explain the
elsebranch; it reads like it was meant to continue. Please reword so it’s a complete sentence.
# Some servers reject `HEAD` but serve `GET`.
# DNS and connection failures happen before the request,
Library/Homebrew/test/utils/curl_spec.rb:732
- The new
HEAD→GETfallback behavior is only tested for 405. Add coverage for a “normal” HTTP failure (e.g. 404) to ensure we don’t do an unnecessaryGETretry whenHEADreturns a valid non-2xx response.
it "retries as a `GET` when the server rejects `HEAD`" do
recorded = record_head_only(response.merge(status_code: "405"), response)
curl_check_http_content("https://brew.sh/", "homepage URL")
expect(recorded).to eq([true, false])
end
fe19a01 to
d1b1cdf
Compare
Today,
curl_check_http_contentverifies that a URL is reachable, but does so with a full GET: it passes --output and downloads the entire resource, then discards it. For an HTTPS URL the body is never read, so this asks for the headers instead.brewcommands to reproduce the bug?brew lgtm(style, typechecking and tests) locally?Opus 5 with several manual review cycles.
I was investigating Cask audit and seeing where I could speed them up, and started exploring.
brew audit --online currently transfers every cask artifact twice: once by
curl_check_http_contentinto a Tempfile that is unlinked on return, and once byaudit_downloadinto the cache. For Firefox (as an example) that is 147 MB downloaded and thrown away on every audit. Because online implies download, both always happen in the same run.Across a sampling of Casks, speedup was anywhere from 1.5x to 45x faster, network speeds will affect individual results.
Formula were a bit different, realizing slight speedups but much less notable, and on a few smaller downloads took a small amount longer, but that could be network jitter with how small the variances are.
Details
Hyperfine statistics
Casks —
hyperfine --runs 3Roughly 172.9 s saved per pass.
Formulae —
hyperfine --runs 5Roughly 81.5 s saved per pass.
Times may vary based upon network
Individual sampling results
Casks
insomniainkscapediscordobspostmanzoomtableplusvisual-studio-codevlcslackfirefoxspotifygoogle-chromeiterm2gimpcyberduckkekasequel-aceaudacitymaccyhandbrake-appstatstransmissionrectangleappcleanerFormulae
Formulae
cmakegopython@3.13gitnodepostgresql@16imagemagickbatneovimredispandocprotobufjqnginxfdlibgit2openssl@3htopsqlitezstdtmuxcurlffmpegripgrep