From 2cf4f47daefd4190ed1e9fe5896b32fd70a5ea92 Mon Sep 17 00:00:00 2001
From: Doug Stull <dstull@gitlab.com>
Date: Sun, 21 Nov 2021 19:19:34 -0500
Subject: [PATCH] Add retries to github importer on client errors

- fix transient github errors.

Changelog: fixed
---
 lib/gitlab/github_import/client.rb           | 26 ++++++-
 spec/lib/gitlab/github_import/client_spec.rb | 71 ++++++++++++++++++++
 2 files changed, 94 insertions(+), 3 deletions(-)

diff --git a/lib/gitlab/github_import/client.rb b/lib/gitlab/github_import/client.rb
index efa816c5eb0233..d2495b328005ee 100644
--- a/lib/gitlab/github_import/client.rb
+++ b/lib/gitlab/github_import/client.rb
@@ -21,6 +21,7 @@ class Client
       SEARCH_MAX_REQUESTS_PER_MINUTE = 30
       DEFAULT_PER_PAGE = 100
       LOWER_PER_PAGE = 50
+      CLIENT_CONNECTION_ERROR = ::Faraday::ConnectionFailed # used/set in sawyer agent which octokit uses
 
       # A single page of data and the corresponding page number.
       Page = Struct.new(:objects, :number)
@@ -148,14 +149,14 @@ def each_object(method, *args, &block)
       # whether we are running in parallel mode or not. For more information see
       # `#rate_or_wait_for_rate_limit`.
       def with_rate_limit
-        return yield unless rate_limiting_enabled?
+        return with_retry { yield } unless rate_limiting_enabled?
 
         request_count_counter.increment
 
         raise_or_wait_for_rate_limit unless requests_remaining?
 
         begin
-          yield
+          with_retry { yield }
         rescue ::Octokit::TooManyRequests
           raise_or_wait_for_rate_limit
 
@@ -166,7 +167,7 @@ def with_rate_limit
       end
 
       def search_repos_by_name(name, options = {})
-        octokit.search_repositories(search_query(str: name, type: :name), options)
+        with_retry { octokit.search_repositories(search_query(str: name, type: :name), options) }
       end
 
       def search_query(str:, type:, include_collaborations: true, include_orgs: true)
@@ -270,6 +271,25 @@ def organizations_subquery
           .map { |org| "org:#{org.login}" }
           .join(' ')
       end
+
+      def with_retry
+        Retriable.retriable(on: CLIENT_CONNECTION_ERROR, on_retry: on_retry) do
+          yield
+        end
+      end
+
+      def on_retry
+        proc do |exception, try, elapsed_time, next_interval|
+          Gitlab::Import::Logger.info(
+            message: "GitHub connection retry triggered",
+            'error.class': exception.class,
+            'error.message': exception.message,
+            try_count: try,
+            elapsed_time_s: elapsed_time,
+            wait_to_retry_s: next_interval
+          )
+        end
+      end
     end
   end
 end
diff --git a/spec/lib/gitlab/github_import/client_spec.rb b/spec/lib/gitlab/github_import/client_spec.rb
index 194dfb228ee464..c4d05e9263350c 100644
--- a/spec/lib/gitlab/github_import/client_spec.rb
+++ b/spec/lib/gitlab/github_import/client_spec.rb
@@ -221,6 +221,50 @@
 
       expect(client.with_rate_limit { 10 }).to eq(10)
     end
+
+    context 'when Faraday error received from octokit', :aggregate_failures do
+      let(:error_class) { described_class::CLIENT_CONNECTION_ERROR }
+      let(:info_params) { { 'error.class': error_class } }
+      let(:block_to_rate_limit) { -> { client.pull_request('foo/bar', 999) } }
+
+      context 'when rate_limiting_enabled is true' do
+        it 'retries on error and succeeds' do
+          allow_retry
+
+          expect(client).to receive(:requests_remaining?).twice.and_return(true)
+          expect(Gitlab::Import::Logger).to receive(:info).with(hash_including(info_params)).once
+
+          expect(client.with_rate_limit(&block_to_rate_limit)).to be(true)
+        end
+
+        it 'retries and does not succeed' do
+          allow(client).to receive(:requests_remaining?).and_return(true)
+          allow(client.octokit).to receive(:pull_request).and_raise(error_class, 'execution expired')
+
+          expect { client.with_rate_limit(&block_to_rate_limit) }.to raise_error(error_class, 'execution expired')
+        end
+      end
+
+      context 'when rate_limiting_enabled is false' do
+        before do
+          allow(client).to receive(:rate_limiting_enabled?).and_return(false)
+        end
+
+        it 'retries on error and succeeds' do
+          allow_retry
+
+          expect(Gitlab::Import::Logger).to receive(:info).with(hash_including(info_params)).once
+
+          expect(client.with_rate_limit(&block_to_rate_limit)).to be(true)
+        end
+
+        it 'retries and does not succeed' do
+          allow(client.octokit).to receive(:pull_request).and_raise(error_class, 'execution expired')
+
+          expect { client.with_rate_limit(&block_to_rate_limit) }.to raise_error(error_class, 'execution expired')
+        end
+      end
+    end
   end
 
   describe '#requests_remaining?' do
@@ -505,6 +549,25 @@
 
         client.search_repos_by_name('test')
       end
+
+      context 'when Faraday error received from octokit', :aggregate_failures do
+        let(:error_class) { described_class::CLIENT_CONNECTION_ERROR }
+        let(:info_params) { { 'error.class': error_class } }
+
+        it 'retries on error and succeeds' do
+          allow_retry(:search_repositories)
+
+          expect(Gitlab::Import::Logger).to receive(:info).with(hash_including(info_params)).once
+
+          expect(client.search_repos_by_name('test')).to be(true)
+        end
+
+        it 'retries and does not succeed' do
+          allow(client.octokit).to receive(:search_repositories).and_raise(error_class, 'execution expired')
+
+          expect { client.search_repos_by_name('test') }.to raise_error(error_class, 'execution expired')
+        end
+      end
     end
 
     describe '#search_query' do
@@ -531,4 +594,12 @@
       end
     end
   end
+
+  def allow_retry(method = :pull_request)
+    call_count = 0
+    allow(client.octokit).to receive(method) do
+      call_count += 1
+      call_count > 1 ? true : raise(described_class::CLIENT_CONNECTION_ERROR, 'execution expired')
+    end
+  end
 end
-- 
GitLab