api: revalidate JSON API caches with ETag, not the bumped mtime

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,16 +93,33 @@ 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")
           end
           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

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)

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

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

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

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)

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|