diff --git a/Library/Homebrew/api.rb b/Library/Homebrew/api.rb index 9ab55cbca1270..4ae5318fa58b0 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,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 - args.prepend("--time-cond", target.to_s) if target.exist? && !target.empty? + 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 if insecure_download opoo DevelopmentTools.insecure_download_warning(endpoint) args.append("--insecure") @@ -99,8 +110,16 @@ 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 + 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 if url == default_url 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 bdb93d813f61a..149734e661b87 100644 --- a/Library/Homebrew/test/api_spec.rb +++ b/Library/Homebrew/test/api_spec.rb @@ -103,6 +103,102 @@ def mock_curl_download(stdout:) expect(target.mtime.to_i).to eq stale_mtime.to_i end + 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 + + 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 + + def fetch + described_class.fetch_json_api_file("bar.json", target:, stale_seconds: 3600) + end + + 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 + + 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 + + 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 + + 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 "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 + + 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 + + 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 + + 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 target = cache_dir/"bar.json" target.write json 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|