Cap Retry-After waits and redact URLs in the GitHub attachments downloader
Everyone can contribute. Help move this issue forward while earning points, leveling up and collecting rewards.
Why
lib/gitlab/github_import/attachments_downloader.rb is used by every GitHub import, one-time or continuous-sync, not just the connected-sync feature. The GitHub continuous-sync proof of concept made two small hardening changes to it that benefit all GitHub imports today. This is its own standalone MR, independent of the GitHub continuous-sync epic, and should not wait for it.
What to do
Port two changes from the proof of concept:
- When GitHub returns a rate-limit response with a
Retry-Aftervalue, the downloader currently waits exactly what GitHub said, falling back to a 120-second default only when GitHub didn't return a usable value. Add a cap:RATE_LIMIT_MAX_RESET_IN = 1.hour.to_i. An unexpectedly large or malformedRetry-Aftervalue can no longer stall an import job for an unbounded amount of time. - Error messages for a failed attachment download currently include the raw download URL ("Error downloading file from #{url}. Error code: #{chunk.code}"), which can carry an embedded, short-lived GitHub-signed token. Change the message to a fixed "Error downloading attachment. Error code: #{chunk.code}" with no URL, for both the retriable and non-retriable error paths.
Code to read
lib/gitlab/github_import/attachments_downloader.rb, diffed against origin/master, and its existing spec file for what needs updating.
Acceptance criteria
- A
Retry-Afterwait longer than one hour is capped at one hour. - A download failure's error message no longer contains the source URL.
- Existing rate-limit and error-code specs are updated to match.
- No behavior change for a
Retry-Aftervalue already under one hour, or for the existing 120-second default when the value is missing or zero.
Out of scope
Any other GitHub importer hardening. LFS-specific hardening is tracked separately as #628601. This issue is scoped to the two changes in attachments_downloader.rb only.
Notes for the reviewer
This is a small, low-risk, generically beneficial fix. Check that the one-hour cap doesn't undercut GitHub's own documented rate-limit guidance for any known scenario.