Auto merge of #5826 - greysteil:handle-invalid-range-errors, r=indirect

Avoid Range Not Satisfiable errors during normal request flow

### What was the end-user problem that led to this PR?
Previously, Bundler was requesting partial response ranges for the Rubygems compact index that could be empty. Since Rubygems was [ignoring the `ETag` header](https://github.com/rubygems/rubygems.org/pull/1652) for these requests, empty ranges would occur whenever the versions index (for instance) hadn't been modified since the version Bundler currently had cached. When this happened, Rubygems would respond with a 416 (Range Not Satisfiable). Bundler would treat this as a `Bundler::HTTPError`, and fall back to using `Fetcher::Dependency` for dependency info. Sadly, that meant metadata about what Ruby version each dependency required was no-longer checked, and updates for gems which should be limited by the system Ruby version were failing.

Closes #5373.

### What was your diagnosis of the problem?
See above

### What is your fix for the problem, implemented in this PR?
This PR updates the range Bundler requests from Rubygems to ensure it's always satisfiable. It does that but requesting all bytes from (and including) the final byte in the Bundler cache, rather than all bytes after (and not including) it.

### Why did you choose this fix out of the possible options?
An alternative fix would be to catch the 416 responses and retry the index lookup in those cases, asking for a full response. That would mean an extra request in all of those cases, though - this method keeps the number of calls to Rubygems down.

3 files changed, 79 insertions, 3 deletions

diff --git a/lib/bundler/compact_index_client/updater.rb b/lib/bundler/compact_index_client/updater.rb
index adf013de04..b501fc194b 100644
--- a/lib/bundler/compact_index_client/updater.rb
+++ b/lib/bundler/compact_index_client/updater.rb
@@ -34,7 +34,14 @@ module Bundler
           if retrying.nil? && local_path.file?
             FileUtils.cp local_path, local_temp_path
             headers["If-None-Match"] = etag_for(local_temp_path)
-            headers["Range"] = "bytes=#{local_temp_path.size}-"
+            headers["Range"] =
+              if local_temp_path.size.nonzero?
+                # Subtract a byte to ensure the range won't be empty.
+                # Avoids 416 (Range Not Satisfiable) responses.
+                "bytes=#{local_temp_path.size - 1}-"
+              else
+                "bytes=#{local_temp_path.size}-"
+              end
           else
             # Fastly ignores Range when Accept-Encoding: gzip is set
             headers["Accept-Encoding"] = "gzip"
@@ -48,9 +55,12 @@ module Bundler
             content = Zlib::GzipReader.new(StringIO.new(content)).read
           end
 
-          mode = response.is_a?(Net::HTTPPartialContent) ? "a" : "w"
           SharedHelpers.filesystem_access(local_temp_path) do
-            local_temp_path.open(mode) {|f| f << content }
+            if response.is_a?(Net::HTTPPartialContent) && local_temp_path.size.nonzero?
+              local_temp_path.open("a") {|f| f << slice_body(content, 1..-1) }
+            else
+              local_temp_path.open("w") {|f| f << content }
+            end
           end
 
           response_etag = (response["ETag"] || "").gsub(%r{\AW/}, "")
@@ -74,6 +84,14 @@ module Bundler
         sum ? %("#{sum}") : nil
       end
 
+      def slice_body(body, range)
+        if body.respond_to?(:byteslice)
+          body.byteslice(range)
+        else # pre-1.9.3
+          body.unpack("@#{range.first}a#{range.end + 1}").first
+        end
+      end
+
       def checksum_for_file(path)
         return nil unless path.file?
         # This must use IO.read instead of Digest.file().hexdigest
diff --git a/spec/install/gems/compact_index_spec.rb b/spec/install/gems/compact_index_spec.rb
index fb5d97ce57..0f1a210433 100644
--- a/spec/install/gems/compact_index_spec.rb
+++ b/spec/install/gems/compact_index_spec.rb
@@ -768,6 +768,27 @@ The checksum of /versions does not match the checksum provided by the server! So
     end
   end
 
+  it "performs partial update with a non-empty range" do
+    gemfile <<-G
+      source "#{source_uri}"
+      gem 'rack', '0.9.1'
+    G
+
+    # Initial install creates the cached versions file
+    bundle! :install, :artifice => "compact_index"
+
+    # Update the Gemfile so we can check subsequent install was successful
+    gemfile <<-G
+      source "#{source_uri}"
+      gem 'rack', '1.0.0'
+    G
+
+    # Second install should make only a partial request to /versions
+    bundle! :install, :artifice => "compact_index_partial_update"
+
+    expect(the_bundle).to include_gems "rack 1.0.0"
+  end
+
   it "performs partial update while local cache is updated by another process" do
     gemfile <<-G
       source "#{source_uri}"
diff --git a/spec/support/artifice/compact_index_partial_update.rb b/spec/support/artifice/compact_index_partial_update.rb
new file mode 100644
index 0000000000..bf6feab877
--- /dev/null
+++ b/spec/support/artifice/compact_index_partial_update.rb
@@ -0,0 +1,37 @@
+# frozen_string_literal: true
+require File.expand_path("../compact_index", __FILE__)
+
+Artifice.deactivate
+
+class CompactIndexPartialUpdate < CompactIndexAPI
+  # Stub the server to never return 304s. This simulates the behaviour of
+  # Fastly / Rubygems ignoring ETag headers.
+  def not_modified?(_checksum)
+    false
+  end
+
+  get "/versions" do
+    cached_versions_path = File.join(
+      Bundler.rubygems.user_home, ".bundle", "cache", "compact_index",
+      "localgemserver.test.80.dd34752a738ee965a2a4298dc16db6c5", "versions"
+    )
+
+    # Verify a cached copy of the versions file exists
+    unless File.read(cached_versions_path).start_with?("created_at: ")
+      raise("Cached versions file should be present and have content")
+    end
+
+    # Verify that a partial request is made, starting from the index of the
+    # final byte of the cached file.
+    unless env["HTTP_RANGE"] == "bytes=#{File.read(cached_versions_path).bytesize - 1}-"
+      raise("Range header should be present, and start from the index of the final byte of the cache.")
+    end
+
+    etag_response do
+      # Return the exact contents of the cache.
+      File.read(cached_versions_path)
+    end
+  end
+end
+
+Artifice.activate_with(CompactIndexPartialUpdate)