Avoid request-bloat in double checking logic

This avoids attempting to double-check in each source for gems that are _in the locally installed set of gems_, which could add hundreds or thousands of extra requests

6 files changed, 65 insertions, 40 deletions

diff --git a/lib/bundler/definition.rb b/lib/bundler/definition.rb
index ecb41f83c2..02ad967c2a 100644
--- a/lib/bundler/definition.rb
+++ b/lib/bundler/definition.rb
@@ -265,9 +265,8 @@ module Bundler
         dependency_names = @dependencies.map(&:name)
 
         sources.all_sources.each do |source|
-          source.dependency_names = dependency_names.dup
+          source.dependency_names = dependency_names - pinned_spec_names(source)
           idx.add_source source.specs
-          dependency_names -= pinned_spec_names(source.specs)
           dependency_names.concat(source.unmet_deps).uniq!
         end
 
@@ -281,21 +280,25 @@ module Bundler
     # of Foo specifically depends on a version of Bar that is only found in source B. This ensures that for
     # each spec we found, we add all possible versions from all sources to the index.
     def double_check_for_index(idx, dependency_names)
+      pinned_names = pinned_spec_names
       loop do
         idxcount = idx.size
+
+        names = :names # do this so we only have to traverse to get dependency_names from the index once
+        unmet_dependency_names = lambda do
+          return names unless names == :names
+          new_names = sources.all_sources.map(&:dependency_names_to_double_check)
+          return names = nil if new_names.compact!
+          names = new_names.flatten(1).concat(dependency_names)
+          names.uniq!
+          names -= pinned_names
+          names
+        end
+
         sources.all_sources.each do |source|
-          names = :names # do this so we only have to traverse to get dependency_names from the index once
-          unmet_dependency_names = proc do
-            break names unless names == :names
-            names = if idx.size > Source::Rubygems::API_REQUEST_LIMIT
-              new_names = idx.dependency_names_if_available
-              new_names && dependency_names.+(new_names).uniq
-            else
-              dependency_names.+(idx.dependency_names).uniq
-            end
-          end
           source.double_check_for(unmet_dependency_names, :override_dupes)
         end
+
         break if idxcount == idx.size
       end
     end
@@ -916,18 +919,15 @@ module Bundler
       source_requirements
     end
 
-    def pinned_spec_names(specs)
-      names = []
-      specs.each do |s|
-        # TODO: when two sources without blocks is an error, we can change
-        # this check to !s.source.is_a?(Source::LocalRubygems). For now,
-        # we need to ask every RubyGems for every gem name.
-        if s.source.is_a?(Source::Git) || s.source.is_a?(Source::Path)
-          names << s.name
-        end
+    def pinned_spec_names(skip = nil)
+      pinned_names = []
+      default = Bundler.feature_flag.lockfile_uses_separate_rubygems_sources? && sources.default_source
+      @dependencies.each do |dep|
+        dep_source = dep.source || default
+        next if dep_source == skip
+        pinned_names << dep.name
       end
-      names.uniq!
-      names
+      pinned_names
     end
 
     def requested_groups
diff --git a/lib/bundler/index.rb b/lib/bundler/index.rb
index ce4062f753..9166a92738 100644
--- a/lib/bundler/index.rb
+++ b/lib/bundler/index.rb
@@ -139,19 +139,6 @@ module Bundler
       names.uniq
     end
 
-    def dependency_names_if_available
-      reduce([]) do |names, spec|
-        case spec
-        when EndpointSpecification, Gem::Specification, LazySpecification, StubSpecification
-          names.concat(spec.dependencies)
-        when RemoteSpecification # from the full index
-          return nil
-        else
-          raise "unhandled spec type in #dependency_names_if_available (#{spec.inspect})"
-        end
-      end.tap {|n| n && n.map!(&:name) }
-    end
-
     def use(other, override_dupes = false)
       return unless other
       other.each do |s|
diff --git a/lib/bundler/source.rb b/lib/bundler/source.rb
index 956cf39d56..5a1f05098b 100644
--- a/lib/bundler/source.rb
+++ b/lib/bundler/source.rb
@@ -38,6 +38,10 @@ module Bundler
     # dependencies, looking for gems we don't have info on yet.
     def double_check_for(*); end
 
+    def dependency_names_to_double_check
+      specs.dependency_names
+    end
+
     def include?(other)
       other == self
     end
diff --git a/lib/bundler/source/rubygems.rb b/lib/bundler/source/rubygems.rb
index fa60bb0c84..02835a5485 100644
--- a/lib/bundler/source/rubygems.rb
+++ b/lib/bundler/source/rubygems.rb
@@ -259,13 +259,32 @@ module Bundler
         return unless api_fetchers.any?
 
         unmet_dependency_names = unmet_dependency_names.call
-        return if !unmet_dependency_names.nil? && unmet_dependency_names.empty?
+        unless unmet_dependency_names.nil?
+          unmet_dependency_names -= remote_specs.spec_names # avoid re-fetching things we've already gotten
+          return if unmet_dependency_names.empty?
+        end
 
         Bundler.ui.debug "Double checking for #{unmet_dependency_names || "all specs (due to the size of the request)"} in #{self}"
 
         fetch_names(api_fetchers, unmet_dependency_names, index, override_dupes)
       end
 
+      def dependency_names_to_double_check
+        names = []
+        remote_specs.each do |spec|
+          case spec
+          when EndpointSpecification, Gem::Specification, StubSpecification, LazySpecification
+            names.concat(spec.runtime_dependencies)
+          when RemoteSpecification # from the full index
+            return nil
+          else
+            raise "unhandled spec type (#{spec.inspect})"
+          end
+        end
+        names.map!(&:name) if names
+        names
+      end
+
     protected
 
       def credless_remotes
diff --git a/spec/install/gems/compact_index_spec.rb b/spec/install/gems/compact_index_spec.rb
index 273366c32f..f633004a3d 100644
--- a/spec/install/gems/compact_index_spec.rb
+++ b/spec/install/gems/compact_index_spec.rb
@@ -253,6 +253,22 @@ The checksum of /versions does not match the checksum provided by the server! So
     end
   end
 
+  it "does not double check for gems that are only installed locally" do
+    system_gems %w[rack-1.0.0 thin-1.0 net_a-1.0]
+    bundle! "config --local path.system true"
+    ENV["BUNDLER_SPEC_ALL_REQUESTS"] = strip_whitespace(<<-EOS).strip
+      #{source_uri}/versions
+      #{source_uri}/info/rack
+    EOS
+
+    install_gemfile! <<-G, :artifice => "compact_index", :verbose => true
+      source "#{source_uri}"
+      gem "rack"
+    G
+
+    expect(last_command.stdboth).not_to include "Double checking"
+  end
+
   it "fetches again when more dependencies are found in subsequent sources", :bundler => "< 2" do
     build_repo2 do
       build_gem "back_deps" do |s|
@@ -279,14 +295,13 @@ The checksum of /versions does not match the checksum provided by the server! So
       FileUtils.rm_rf Dir[gem_repo2("gems/foo-*.gem")]
     end
 
-    gemfile <<-G
+    install_gemfile! <<-G, :artifice => "compact_index_extra", :verbose => true
       source "#{source_uri}"
       source "#{source_uri}/extra" do
         gem "back_deps"
       end
     G
 
-    bundle! :install, :artifice => "compact_index_extra"
     expect(the_bundle).to include_gems "back_deps 1.0", "foo 1.0"
   end
 
diff --git a/spec/install/gems/dependency_api_spec.rb b/spec/install/gems/dependency_api_spec.rb
index 22c623ab40..85a7f07092 100644
--- a/spec/install/gems/dependency_api_spec.rb
+++ b/spec/install/gems/dependency_api_spec.rb
@@ -368,7 +368,7 @@ RSpec.describe "gemcutter's dependency API" do
 
     bundle :install, :artifice => "endpoint_extra"
 
-    expect(out).to include("Fetching gem metadata from http://localgemserver.test/..")
+    expect(out).to include("Fetching gem metadata from http://localgemserver.test/.")
     expect(out).to include("Fetching source index from http://localgemserver.test/extra")
   end