Cap Retry-After waits and drop URLs from attachment errors
What does this MR do and why?
Gitlab::GithubImport::AttachmentsDownloader is used by every GitHub import, so these two small hardening changes benefit all of them.
The downloader waited for exactly whatever Retry-After GitHub returned, falling back to 120 seconds only when the value was missing or zero. An unexpectedly large or malformed value could therefore park an import job for as long as the header claimed. The wait is now capped by RATE_LIMIT_MAX_RESET_IN, one hour. The cap is applied after the default fallback, so a value already under an hour and the 120 second default behave exactly as before.
Download failures reported the raw download URL in their message. Those URLs can carry a short lived GitHub signed token, which then ends up in logs and error tracking. The message now names only the error code, on both the retriable and the non retriable path.
References
- Closes #628602
- Ported from the GitHub continuous sync proof of concept, but standalone: it does not depend on that epic
- LFS specific hardening is tracked separately in #628601
Differences
Before: a Retry-After of two hours meant a two hour wait, and there was no upper bound at all. A failed download raised "Error downloading file from https://..." with the full URL, token included.
After: any Retry-After above one hour waits one hour. A failed download raises "Error downloading attachment. Error code: 404" with no URL. Nothing else changes: values under an hour, the 120 second default, and the 403 without a header case all behave as they did.
One pre-existing edge worth naming so it is not mistaken for a regression: a negative Retry-After still passes through unchanged, as it did before. The cap uses min, which does not affect it. It is outside the two changes this issue scopes.
How to set up and validate locally
bundle exec rspec spec/lib/gitlab/github_import/attachments_downloader_spec.rbThe new context is "when retry-after header exceeds RATE_LIMIT_MAX_RESET_IN", which sends a two hour value and expects reset_in of one hour. The existing 60 second and missing header cases are unchanged and still pass, which is the "no behaviour change under one hour" criterion. The three error message expectations now assert the URL free form.