From d1c0f1fb6e56b3f8594d7e93f5f9ecbf0af86996 Mon Sep 17 00:00:00 2001 From: Xavier Lange Date: Wed, 7 Oct 2026 15:50:23 -0400 Subject: [PATCH 1/3] api: revalidate JSON API caches with ETag, not the bumped mtime fetch_json_api_file touches the cache file to now after every successful check (it doubles as the staleness clock), then sends --time-cond next time. A copy fetched from a stale CDN edge therefore sends an If-Modified-Since newer than the real object's Last-Modified, gets 304 forever, and `brew upgrade` keeps reporting "already installed" until the next publish. Save the ETag beside the cache file and send it with --etag-compare when present; fall back to --time-cond otherwise (mirrors, first run after upgrading). curl blanks the ETag file on a 304, so save to a temp path and only replace the stored ETag on a non-empty result. --- Library/Homebrew/api.rb | 13 +++++++- Library/Homebrew/test/api_spec.rb | 50 +++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) diff --git a/Library/Homebrew/api.rb b/Library/Homebrew/api.rb index 9ab55cbca1270..43182545c8971 100644 --- a/Library/Homebrew/api.rb +++ b/Library/Homebrew/api.rb @@ -75,6 +75,8 @@ def self.fetch_json_api_file(endpoint, target: HOMEBREW_CACHE_API/endpoint, insecure_download = DevelopmentTools.ca_file_substitution_required? || DevelopmentTools.curl_substitution_required? skip_download = skip_download?(target:, stale_seconds:) + etag_path = Pathname("#{target}.etag") + new_etag_path = Pathname("#{target}.etag.new") if enqueue unless skip_download @@ -91,7 +93,15 @@ def self.fetch_json_api_file(endpoint, target: HOMEBREW_CACHE_API/endpoint, download_succeeded = T.let(false, T::Boolean) begin args = curl_args.dup - args.prepend("--time-cond", target.to_s) if target.exist? && !target.empty? + if target.exist? && !target.empty? + if etag_path.exist? && !etag_path.empty? + args.prepend("--etag-compare", etag_path.to_s) + else + args.prepend("--time-cond", target.to_s) + end + end + new_etag_path.unlink if new_etag_path.exist? + args.prepend("--etag-save", new_etag_path.to_s) if insecure_download opoo DevelopmentTools.insecure_download_warning(endpoint) args.append("--insecure") @@ -101,6 +111,7 @@ def self.fetch_json_api_file(endpoint, target: HOMEBREW_CACHE_API/endpoint, # Disable retries here, we handle them ourselves below. Utils::Curl.curl_download(*args, url, to: target, retries: 0, show_error: false) download_succeeded = true + FileUtils.mv(new_etag_path, etag_path) if new_etag_path.exist? && !new_etag_path.empty? end rescue ErrorDuringExecution if url == default_url diff --git a/Library/Homebrew/test/api_spec.rb b/Library/Homebrew/test/api_spec.rb index bdb93d813f61a..7de5dbb32533d 100644 --- a/Library/Homebrew/test/api_spec.rb +++ b/Library/Homebrew/test/api_spec.rb @@ -103,6 +103,56 @@ def mock_curl_download(stdout:) expect(target.mtime.to_i).to eq stale_mtime.to_i end + it "revalidates with the saved ETag instead of the bumped cache mtime" do + target = cache_dir/"bar.json" + target.write json + FileUtils.touch(target, mtime: Time.now - 7200) + etag_path = Pathname("#{target}.etag") + etag_path.write 'W/"old"' + + args = T.let(nil, T.untyped) + allow(Utils::Curl).to receive(:curl_download) do |*curl_args, **| + args = curl_args + save_to = curl_args[curl_args.index("--etag-save") + 1] + File.write(save_to, 'W/"new"') + end + + described_class.fetch_json_api_file("bar.json", target:, stale_seconds: 3600) + + expect(args).to include("--etag-compare", etag_path.to_s) + expect(args).not_to include("--time-cond") + expect(etag_path.read).to eq 'W/"new"' + end + + it "keeps the saved ETag when curl blanks the new one on a 304" do + target = cache_dir/"bar.json" + target.write json + FileUtils.touch(target, mtime: Time.now - 7200) + etag_path = Pathname("#{target}.etag") + etag_path.write 'W/"old"' + + allow(Utils::Curl).to receive(:curl_download) do |*curl_args, **| + File.write(curl_args[curl_args.index("--etag-save") + 1], "") + end + + described_class.fetch_json_api_file("bar.json", target:, stale_seconds: 3600) + + expect(etag_path.read).to eq 'W/"old"' + end + + it "falls back to --time-cond when no ETag has been saved" do + target = cache_dir/"bar.json" + target.write json + FileUtils.touch(target, mtime: Time.now - 7200) + + args = T.let(nil, T.untyped) + allow(Utils::Curl).to receive(:curl_download) { |*curl_args, **| args = curl_args } + + described_class.fetch_json_api_file("bar.json", target:, stale_seconds: 3600) + + expect(args).to include("--time-cond", target.to_s) + end + it "refreshes the cache mtime when a fallback to the default API domain succeeds" do target = cache_dir/"bar.json" target.write json From 0a3e2b2e8062c7ef773842f11305e7aeb1d95b13 Mon Sep 17 00:00:00 2001 From: Xavier Lange Date: Wed, 7 Oct 2026 16:16:35 -0400 Subject: [PATCH 2/3] api: drop the saved ETag after a 200 that carries none CodeRabbit on #24193: curl creates an empty --etag-save file for an untagged 200 as well as for a 304, so the old ETag survived a body it no longer describes. Tell them apart by the HTTP status already written to stderr. --- Library/Homebrew/api.rb | 8 ++++++-- Library/Homebrew/test/api_spec.rb | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/Library/Homebrew/api.rb b/Library/Homebrew/api.rb index 43182545c8971..5d0f230089bf4 100644 --- a/Library/Homebrew/api.rb +++ b/Library/Homebrew/api.rb @@ -109,9 +109,13 @@ def self.fetch_json_api_file(endpoint, target: HOMEBREW_CACHE_API/endpoint, unless skip_download ohai "Downloading #{url}" if $stdout.tty? && !Context.current.quiet? # Disable retries here, we handle them ourselves below. - Utils::Curl.curl_download(*args, url, to: target, retries: 0, show_error: false) + result = Utils::Curl.curl_download(*args, url, to: target, retries: 0, show_error: false) download_succeeded = true - FileUtils.mv(new_etag_path, etag_path) if new_etag_path.exist? && !new_etag_path.empty? + if new_etag_path.exist? && !new_etag_path.empty? + FileUtils.mv(new_etag_path, etag_path) + elsif result.respond_to?(:stderr) && result.stderr.exclude?("HTTP status: 304") + etag_path.unlink if etag_path.exist? + end end rescue ErrorDuringExecution if url == default_url diff --git a/Library/Homebrew/test/api_spec.rb b/Library/Homebrew/test/api_spec.rb index 7de5dbb32533d..9737db8fdd7af 100644 --- a/Library/Homebrew/test/api_spec.rb +++ b/Library/Homebrew/test/api_spec.rb @@ -115,6 +115,7 @@ def mock_curl_download(stdout:) args = curl_args save_to = curl_args[curl_args.index("--etag-save") + 1] File.write(save_to, 'W/"new"') + instance_double(SystemCommand::Result, stderr: "HTTP status: 200") end described_class.fetch_json_api_file("bar.json", target:, stale_seconds: 3600) @@ -133,6 +134,7 @@ def mock_curl_download(stdout:) allow(Utils::Curl).to receive(:curl_download) do |*curl_args, **| File.write(curl_args[curl_args.index("--etag-save") + 1], "") + instance_double(SystemCommand::Result, stderr: "HTTP status: 304") end described_class.fetch_json_api_file("bar.json", target:, stale_seconds: 3600) @@ -140,6 +142,23 @@ def mock_curl_download(stdout:) expect(etag_path.read).to eq 'W/"old"' end + it "removes the saved ETag when a 200 response carries none" do + target = cache_dir/"bar.json" + target.write json + FileUtils.touch(target, mtime: Time.now - 7200) + etag_path = Pathname("#{target}.etag") + etag_path.write 'W/"old"' + + allow(Utils::Curl).to receive(:curl_download) do |*curl_args, **| + File.write(curl_args[curl_args.index("--etag-save") + 1], "") + instance_double(SystemCommand::Result, stderr: "HTTP status: 200") + end + + described_class.fetch_json_api_file("bar.json", target:, stale_seconds: 3600) + + expect(etag_path).not_to exist + end + it "falls back to --time-cond when no ETag has been saved" do target = cache_dir/"bar.json" target.write json From 8f602940321011da0f91490c3eec167b8fc60d5d Mon Sep 17 00:00:00 2001 From: Xavier Lange Date: Wed, 7 Oct 2026 16:31:53 -0400 Subject: [PATCH 3/3] api: gate ETag revalidation on curl 7.68, keep the ETag when scrubbing --etag-save and --etag-compare need curl 7.68.0 while Homebrew only requires 7.41, so on older curl every API fetch failed. Add curl_supports_etag? and fall back to --time-cond when it is false. brew cleanup --scrub removed the .etag beside the current packages file, so the next fetch lost its validator; keep it. Unlink the empty .etag.new curl leaves after a 304, and rework the specs around a shared stale-cache context with real SystemCommand::Result stubs. --- Library/Homebrew/api.rb | 28 +++-- Library/Homebrew/cleanup.rb | 1 + Library/Homebrew/test/api_spec.rb | 135 ++++++++++++++--------- Library/Homebrew/test/cleanup_spec.rb | 3 +- Library/Homebrew/test/spec_helper.rb | 1 + Library/Homebrew/test/utils/curl_spec.rb | 12 ++ Library/Homebrew/utils/curl.rb | 8 ++ 7 files changed, 121 insertions(+), 67 deletions(-) diff --git a/Library/Homebrew/api.rb b/Library/Homebrew/api.rb index 5d0f230089bf4..4ae5318fa58b0 100644 --- a/Library/Homebrew/api.rb +++ b/Library/Homebrew/api.rb @@ -93,15 +93,16 @@ def self.fetch_json_api_file(endpoint, target: HOMEBREW_CACHE_API/endpoint, download_succeeded = T.let(false, T::Boolean) begin args = curl_args.dup - if target.exist? && !target.empty? - if etag_path.exist? && !etag_path.empty? - args.prepend("--etag-compare", etag_path.to_s) - else - args.prepend("--time-cond", target.to_s) - end + use_etag = Utils::Curl.curl_supports_etag? + if use_etag && etag_path.exist? && !etag_path.empty? && target.exist? && !target.empty? + args.prepend("--etag-compare", etag_path.to_s) + elsif target.exist? && !target.empty? + args.prepend("--time-cond", target.to_s) + end + if use_etag + new_etag_path.unlink if new_etag_path.exist? + args.prepend("--etag-save", new_etag_path.to_s) end - new_etag_path.unlink if new_etag_path.exist? - args.prepend("--etag-save", new_etag_path.to_s) if insecure_download opoo DevelopmentTools.insecure_download_warning(endpoint) args.append("--insecure") @@ -111,10 +112,13 @@ def self.fetch_json_api_file(endpoint, target: HOMEBREW_CACHE_API/endpoint, # Disable retries here, we handle them ourselves below. result = Utils::Curl.curl_download(*args, url, to: target, retries: 0, show_error: false) download_succeeded = true - if new_etag_path.exist? && !new_etag_path.empty? - FileUtils.mv(new_etag_path, etag_path) - elsif result.respond_to?(:stderr) && result.stderr.exclude?("HTTP status: 304") - etag_path.unlink if etag_path.exist? + if use_etag + if new_etag_path.exist? && !new_etag_path.empty? + FileUtils.mv(new_etag_path, etag_path) + elsif result.respond_to?(:stderr) && result.stderr.exclude?("HTTP status: 304") + etag_path.unlink if etag_path.exist? + end + new_etag_path.unlink if new_etag_path.exist? end end rescue ErrorDuringExecution diff --git a/Library/Homebrew/cleanup.rb b/Library/Homebrew/cleanup.rb index 06af8f62eb1be..7dfdc992a0372 100644 --- a/Library/Homebrew/cleanup.rb +++ b/Library/Homebrew/cleanup.rb @@ -670,6 +670,7 @@ def cache_files current_api_package_basename, "#{current_api_package_basename}.payload", "#{current_api_package_basename}.payload.index", + "#{current_api_package_basename}.etag", ] api_internal.glob("packages.*.jws.json*").reject do |path| kept_basenames.include?(path.basename.to_s) diff --git a/Library/Homebrew/test/api_spec.rb b/Library/Homebrew/test/api_spec.rb index 9737db8fdd7af..149734e661b87 100644 --- a/Library/Homebrew/test/api_spec.rb +++ b/Library/Homebrew/test/api_spec.rb @@ -103,73 +103,100 @@ def mock_curl_download(stdout:) expect(target.mtime.to_i).to eq stale_mtime.to_i end - it "revalidates with the saved ETag instead of the bumped cache mtime" do - target = cache_dir/"bar.json" - target.write json - FileUtils.touch(target, mtime: Time.now - 7200) - etag_path = Pathname("#{target}.etag") - etag_path.write 'W/"old"' - - args = T.let(nil, T.untyped) - allow(Utils::Curl).to receive(:curl_download) do |*curl_args, **| - args = curl_args - save_to = curl_args[curl_args.index("--etag-save") + 1] - File.write(save_to, 'W/"new"') - instance_double(SystemCommand::Result, stderr: "HTTP status: 200") + context "with a stale cached file" do + let(:target) { cache_dir/"bar.json" } + let(:etag_path) { Pathname("#{target}.etag") } + let(:curl_args) { [] } + + before do + target.write json + FileUtils.touch(target, mtime: Time.now - 7200) end - described_class.fetch_json_api_file("bar.json", target:, stale_seconds: 3600) - - expect(args).to include("--etag-compare", etag_path.to_s) - expect(args).not_to include("--time-cond") - expect(etag_path.read).to eq 'W/"new"' - end - - it "keeps the saved ETag when curl blanks the new one on a 304" do - target = cache_dir/"bar.json" - target.write json - FileUtils.touch(target, mtime: Time.now - 7200) - etag_path = Pathname("#{target}.etag") - etag_path.write 'W/"old"' - - allow(Utils::Curl).to receive(:curl_download) do |*curl_args, **| - File.write(curl_args[curl_args.index("--etag-save") + 1], "") - instance_double(SystemCommand::Result, stderr: "HTTP status: 304") + def stub_curl(status:, etag: "") + allow(Utils::Curl).to receive(:curl_download) do |*args, to:, **| + curl_args.replace(args) + File.write(args.fetch(args.index("--etag-save") + 1), etag) if etag + to.write json if status == 200 + SystemCommand::Result.new(["curl"], [[:stderr, "HTTP status: #{status}"]], + instance_double(Process::Status, success?: true, exitstatus: 0), + secrets: []) + end end - described_class.fetch_json_api_file("bar.json", target:, stale_seconds: 3600) - - expect(etag_path.read).to eq 'W/"old"' - end + def fetch + described_class.fetch_json_api_file("bar.json", target:, stale_seconds: 3600) + end - it "removes the saved ETag when a 200 response carries none" do - target = cache_dir/"bar.json" - target.write json - FileUtils.touch(target, mtime: Time.now - 7200) - etag_path = Pathname("#{target}.etag") - etag_path.write 'W/"old"' + it "sends the saved ETag instead of the cache mtime and stores the new one" do + etag_path.write 'W/"old"' + stub_curl(status: 200, etag: 'W/"new"') + fetch + expect(curl_args).to include("--etag-compare", etag_path.to_s) + expect(curl_args).not_to include("--time-cond") + expect(etag_path.read).to eq 'W/"new"' + end - allow(Utils::Curl).to receive(:curl_download) do |*curl_args, **| - File.write(curl_args[curl_args.index("--etag-save") + 1], "") - instance_double(SystemCommand::Result, stderr: "HTTP status: 200") + it "falls back to --time-cond and saves the ETag when none is saved" do + stub_curl(status: 200, etag: 'W/"new"') + fetch + expect(curl_args).to include("--time-cond", target.to_s) + expect(curl_args).not_to include("--etag-compare") + expect(etag_path.read).to eq 'W/"new"' end - described_class.fetch_json_api_file("bar.json", target:, stale_seconds: 3600) + it "falls back to --time-cond when the saved ETag is empty" do + etag_path.write "" + stub_curl(status: 200, etag: 'W/"new"') + fetch + expect(curl_args).to include("--time-cond", target.to_s) + expect(curl_args).not_to include("--etag-compare") + end - expect(etag_path).not_to exist - end + it "keeps the saved ETag, refreshes the mtime and leaves no temp file on a 304" do + etag_path.write 'W/"old"' + stub_curl(status: 304) + fetch + expect(etag_path.read).to eq 'W/"old"' + expect(Pathname("#{target}.etag.new")).not_to exist + expect(target.mtime).to be > Time.now - 60 + end - it "falls back to --time-cond when no ETag has been saved" do - target = cache_dir/"bar.json" - target.write json - FileUtils.touch(target, mtime: Time.now - 7200) + it "ignores a leftover temp ETag from an earlier run" do + etag_path.write 'W/"old"' + Pathname("#{target}.etag.new").write 'W/"leftover"' + stub_curl(status: 304, etag: nil) + fetch + expect(etag_path.read).to eq 'W/"old"' + end - args = T.let(nil, T.untyped) - allow(Utils::Curl).to receive(:curl_download) { |*curl_args, **| args = curl_args } + it "removes the saved ETag when a 200 carries none" do + etag_path.write 'W/"old"' + stub_curl(status: 200) + fetch + expect(etag_path).not_to exist + end - described_class.fetch_json_api_file("bar.json", target:, stale_seconds: 3600) + it "keeps the saved ETag when the download fails" do + etag_path.write 'W/"old"' + allow(Utils::Curl).to receive(:curl_download).and_raise(ErrorDuringExecution.new(["curl"], status: 1)) + expect { fetch }.to output(/falling back to cached version/).to_stderr + expect(etag_path.read).to eq 'W/"old"' + end - expect(args).to include("--time-cond", target.to_s) + it "uses only --time-cond and never touches ETags when curl is too old" do + allow(Utils::Curl).to receive(:curl_supports_etag?).and_return(false) + etag_path.write 'W/"old"' + stub_curl(status: 200, etag: nil) + allow(Utils::Curl).to receive(:curl_download) do |*args, to:, **| + curl_args.replace(args) + to.write json + end + fetch + expect(curl_args).to include("--time-cond", target.to_s) + expect(curl_args.grep(/etag/)).to be_empty + expect(etag_path.read).to eq 'W/"old"' + end end it "refreshes the cache mtime when a fallback to the default API domain succeeds" do diff --git a/Library/Homebrew/test/cleanup_spec.rb b/Library/Homebrew/test/cleanup_spec.rb index 3b09bf034503d..c645519fd6d46 100644 --- a/Library/Homebrew/test/cleanup_spec.rb +++ b/Library/Homebrew/test/cleanup_spec.rb @@ -874,6 +874,7 @@ api_internal/current_basename, api_internal/"#{current_basename}.payload", api_internal/"#{current_basename}.payload.index", + api_internal/"#{current_basename}.etag", ] scrubbed_files = [ api_internal/"packages.stale.jws.json.payload", @@ -887,7 +888,7 @@ described_class.new(scrub: true, cache:).cleanup_cache - expect((kept_files + scrubbed_files).map(&:exist?)).to eq([true, true, true, false, false, false]) + expect((kept_files + scrubbed_files).map(&:exist?)).to eq([true, true, true, true, false, false, false]) end it "cleans up API source files and symlinks at any depth without cleaning directories" do diff --git a/Library/Homebrew/test/spec_helper.rb b/Library/Homebrew/test/spec_helper.rb index d6883da4b8b8d..0819333ab2f27 100644 --- a/Library/Homebrew/test/spec_helper.rb +++ b/Library/Homebrew/test/spec_helper.rb @@ -231,6 +231,7 @@ def ensure_test_dependency!(available, message) allow(Utils::Curl).to receive(:curl_executable).and_raise(<<~ERROR) Unexpected call to Utils::Curl.curl_executable without setting :needs_network or :needs_utils_curl. ERROR + allow(Utils::Curl).to receive(:curl_supports_etag?).and_return(true) end config.before do diff --git a/Library/Homebrew/test/utils/curl_spec.rb b/Library/Homebrew/test/utils/curl_spec.rb index bb2696b19b855..ce8a2ffaeca14 100644 --- a/Library/Homebrew/test/utils/curl_spec.rb +++ b/Library/Homebrew/test/utils/curl_spec.rb @@ -595,6 +595,18 @@ end end + describe "::curl_supports_etag?" do + it "returns `true` if curl version is 7.68.0 or higher" do + allow_any_instance_of(Utils::Curl).to receive(:curl_version).and_return(Version.new("7.68.0")) + expect(curl_supports_etag?).to be(true) + end + + it "returns `false` if curl version is lower than 7.68.0" do + allow_any_instance_of(Utils::Curl).to receive(:curl_version).and_return(Version.new("7.64.1")) + expect(curl_supports_etag?).to be(false) + end + end + describe "::curl_supports_tls13?" do it "returns `true` if curl command is successful" do allow(SystemCommand).to receive(:quiet_system).and_return(true) diff --git a/Library/Homebrew/utils/curl.rb b/Library/Homebrew/utils/curl.rb index 8c24d6cdef59b..020dadc4a6cad 100644 --- a/Library/Homebrew/utils/curl.rb +++ b/Library/Homebrew/utils/curl.rb @@ -724,6 +724,14 @@ def curl_supports_fail_with_body? @curl_supports_fail_with_body[curl_path] end + sig { returns(T::Boolean) } + def curl_supports_etag? + @curl_supports_etag ||= T.let(Hash.new do |h, key| + h[key] = curl_version >= Version.new("7.68.0") + end, T.nilable(T::Hash[T.any(Pathname, String), T::Boolean])) + @curl_supports_etag[curl_path] + end + sig { returns(T::Boolean) } def curl_supports_tls13? @curl_supports_tls13 ||= T.let(Hash.new do |h, key|