## Why are we doing this work GitLab's GitHub importer assumes all GitHub Enterprise Server (GHES) instances do not have rate limiting enabled. In reality, rate limiting is only disabled by default on GHES, but can be enabled and configured by site admins. I.e., `Gitlab::GithubImport::Client#with_rate_limit` skips the logic to handle rate limit errors differently from other API errors when `rate_limiting_enabled?` is false, and `rate_limiting_enabled?` simply checks `api_endpoint.include?('.github.com')`. The import workers (`ReschedulingMethods`, `StageMethods`) only reschedule on `RateLimitError`. Anything else counts against Sidekiq retries or is tracked as an import failure, so a rate-limited GHES import loses data instead of waiting and resuming. A customer attempting to import 50+ repositories from a GHES instance encountered this, resulting in missing data. ## Proposed fix * Refactor `Gitlab::GithubImport::Client#with_rate_limit` so that the importer rate limiting logic is skipped only when `octokit.rate_limit` is attempted rather than checking the API endpoint host. * Update `Gitlab::GithubImport::Client#requests_remaining?` so that the request thresholds are dynamic and set relative to the actual requests limit. Currently, the GitHub client pauses importing records just before the rate limit is reached (50 requests before exhausting the limit for non-search requests, and 3 for search requests) so that parallel processes aren't likely to reach the request limit at the same time. When importing from GHES, limits may be configured low enough to where a 50 request buffer is much more than "just before" the rate limit is reached, significantly slowing down the import. Therefore, `requests_remaining?` must have dynamic thresholds for remaining requests rather than the current implementation that relies on constants. **Other caveats to consider:** * Ensure `octokit.rate_limit` isn't repeatedly attempted when it raises an error * GHES rate limits can technically change during an import, though not a likely scenario and it may be wise to document that this should be avoided if it gives us any potential efficiencies ## Relevant links - Review of the original proposal: https://gitlab.com/gitlab-org/gitlab/-/work_items/621582#note_3730112037 - #697 — the 2016 bug that introduced the `.github.com` assumption - #581751 — same class of problem for attachment downloads, fixed by !213727 (18.6) - !214563 — made 4XX non-retriable for attachments (18.7) - [Octokit error mapping](https://github.com/octokit/octokit.rb/blob/main/lib/octokit/error.rb) (`Octokit::Error.from_response`) ## Non-functional requirements - [ ] Documentation: not expected; behaviour change only affects error recovery - [ ] Feature flag: none (agreed in https://gitlab.com/gitlab-org/gitlab/-/work_items/621582#note_3730112037) ## Original proposal <details> <summary>Original proposed code</summary> (see https://gitlab.com/gitlab-org/gitlab/-/work_items/621582#note_3730112037 for why it doesn't work as written) Handle 429 responses on for GHES behind a feature flag.Feature flag: ```yaml --- name: github_import_ghes_rate_limit_handling description: Handle HTTP 429 rate limit responses from GitHub Enterprise Server during project imports by rescheduling jobs instead of failing permanently. feature_issue_url: '' introduced_by_url: '' rollout_issue_url: '' milestone: '19.4' group: group::import and integrate type: gitlab_com_derisk default_enabled: false ``` `lib/gitlab/github_import/client.rb`: ```ruby def with_rate_limit unless rate_limiting_enabled? if Feature.enabled?(:github_import_ghes_rate_limit_handling, type: :gitlab_com_derisk) return with_retry do begin yield rescue ::Octokit::TooManyRequests => e raise_or_wait_for_rate_limit(e.response_body) retry end end end return with_retry { yield } end request_count_counter.increment # ... existing SaaS path unchanged end ``` `spec/lib/gitlab/github_import/client_spec.rb`: ```ruby context 'when rate_limiting_enabled is false and TooManyRequests is raised' do let(:parallel) { true } before do allow(client).to receive(:rate_limiting_enabled?).and_return(false) stub_feature_flags(github_import_ghes_rate_limit_handling: true) end it 'raises RateLimitError in parallel mode' do limited_block = -> { raise(Octokit::TooManyRequests.new(body: 'rate limit exceeded')) } expect { client.with_rate_limit(&limited_block) } .to raise_error(Gitlab::GithubImport::RateLimitError, 'rate limit exceeded') end context 'when running in sequential mode' do let(:parallel) { false } it 'sleeps and retries the request' do retries = 0 expect(client).to receive(:rate_limit_resets_in).and_return(5) expect(client).to receive(:sleep).with(5) client.with_rate_limit do if retries == 0 retries += 1 raise(Octokit::TooManyRequests.new(body: 'rate limit exceeded')) end 'success' end expect(retries).to eq(1) end end it 'still retries connection errors via Retriable' do call_count = 0 allow(client.octokit).to receive(:pull_request) do call_count += 1 raise(Faraday::ConnectionFailed, 'execution expired') if call_count == 1 {} end block = -> { client.octokit.pull_request('foo/bar', 1) } expect(Gitlab::GithubImport::Logger).to receive(:info).with( hash_including('error.class': Faraday::ConnectionFailed) ).once expect(client.with_rate_limit(&block)).to eq({}) end end context 'when rate_limiting_enabled is false and feature flag is disabled' do let(:parallel) { true } before do allow(client).to receive(:rate_limiting_enabled?).and_return(false) stub_feature_flags(github_import_ghes_rate_limit_handling: false) end it 'does not catch TooManyRequests' do limited_block = -> { raise(Octokit::TooManyRequests.new(body: 'rate limit exceeded')) } expect { client.with_rate_limit(&limited_block) } .to raise_error(Octokit::TooManyRequests) end end ``` </details>