Avoid Range Not Satisfiable errors during normal request flow

3 files changed, 80 insertions, 3 deletions

diff --git a/lib/bundler/compact_index_client/updater.rb b/lib/bundler/compact_index_client/updater.rb
index 5aa480fdcc..dc26095040 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..d95941eae6
--- /dev/null
+++ b/spec/support/artifice/compact_index_partial_update.rb
@@ -0,0 +1,38 @@
+# 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
+    versions = 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(versions).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(versions).bytesize - 1}-"
+      raise("Range header should be present, and start from the index of the final byte of the cache.")
+    end
+
+    etag_response do
+      file = tmp("versions.list")
+      file.delete if file.file?
+      file = CompactIndex::VersionsFile.new(file.to_s)
+      file.create(gems)
+      file.contents
+    end
+  end
+end
+
+Artifice.activate_with(CompactIndexPartialUpdate)