feat(network): use Retry-After header for HTTP 429 responses #15463
Conversation
rustbot
added
A-networking
src/cargo/util/network/retry.rs
Outdated
| @@ -47,6 +47,7 @@ use anyhow::Error; | |||
| use rand::Rng; | |||
| use std::cmp::min; | |||
| use std::time::Duration; | |||
| use time::OffsetDateTime; | |||
There was a problem hiding this comment.
I feel like we are on the way of dropping time? #15293
There was a problem hiding this comment.
Maybe using jiff isn't the right call?
In my testing, it can take 2 to 10 seconds for jiff to initialize.
I suspect part of the problem is that jiff is configured to use the system timezone data, and it's probably loading and initializing a bunch of junk. Unfortunately because gix-date enables the default features, we can't easily disable that AFAIK.
Can we maybe get gix-date to not use the default? I don't fully understand what all the jiff features do, or when or if they are necessary.
There was a problem hiding this comment.
It seems like jiff init is taking around that time in CI, which I agree is unacceptable. I'm not seeing the delay on Windows, but that's likely because jiff isn't using system timezone data there.
There was a problem hiding this comment.
I wonder if there's a way to do the parsing without loading the timezone data. The HTTP Date header is always GMT.
This only applies for things that use |
|
Would it be possible to add a test using our |
src/cargo/util/network/retry.rs
Outdated
| let expected = jiff::Zoned::now() | ||
| .until( | ||
| &jiff::civil::date(2100, 1, 1) | ||
| .at(0, 0, 0, 0) | ||
| .to_zoned(jiff::tz::TimeZone::UTC) | ||
| .unwrap(), | ||
| ) | ||
| .unwrap() | ||
| .total(jiff::Unit::Millisecond) | ||
| .unwrap() as u64; | ||
| let actual = Retry::parse_retry_after(&headers).unwrap(); | ||
| let diff = expected.abs_diff(actual.into()); | ||
| assert!((diff < 1000), "{} != {} ({})", expected, actual, diff); |
There was a problem hiding this comment.
Independent of the problem with jiff initialization, I would recommend moving the expected initialization to after the actual line. That should reduce the time variance (since computing expected is very efficient, whereas parse_retry_after is not).
There was a problem hiding this comment.
I'll change parse_retry_after to take in now as a parameter to simplify the testing and eliminate the variation.
There was a problem hiding this comment.
Correction, I think I had that backwards. It looks like jiff::Zoned::now() is what is taking all the time.
There was a problem hiding this comment.
Can you try again with latest push and see if it's fixed?
There was a problem hiding this comment.
I reported this upstream at BurntSushi/jiff#366.
Update cargo 25 commits in 7918c7eb59614c39f1c4e27e99d557720976bdd7..056f5f4f3c100cb36b5e9aed2d20b9ea70aae295 2025-04-27 09:44:23 +0000 to 2025-05-09 14:54:18 +0000 - Revert "doc: Mention `XDG_DATA_HOME`" (rust-lang/cargo#15512) - docs: update version notice for deprecation removal (rust-lang/cargo#15511) - doc: Update instructions on using native-completions (rust-lang/cargo#15480) - feat(network): use Retry-After header for HTTP 429 responses (rust-lang/cargo#15463) - CI: Require schema job to pass (rust-lang/cargo#15504) - chore(config): migrate renovate config (rust-lang/cargo#15501) - Make cargo script ignore workspaces (rust-lang/cargo#15496) - fix(rustc): Don't panic on unknown bins (rust-lang/cargo#15497) - test: Remove unused nightly requirements (rust-lang/cargo#15498) - Add support for `-Zembed-metadata` (rust-lang/cargo#15378) - Fix tracking issue template link (rust-lang/cargo#15494) - Refactor artifact deps in FeatureResolver::deps (rust-lang/cargo#15492) - Improved error message for versions prefixed with `v` (rust-lang/cargo#15484) - chore: fix some typos in comment (rust-lang/cargo#15485) - fix: default to all targets when using `--edition` and ` --edition-idioms` in cargo fix (rust-lang/cargo#15192) - Update fingerprint footnote (rust-lang/cargo#15478) - feat(add): suggest similarly named features (rust-lang/cargo#15438) - In package-workspace, keep dev-dependencies if they have a version (rust-lang/cargo#15470) - docs: fix a typo in DependencyUI (rust-lang/cargo#15472) - fix grammar, and remove confusing example (rust-lang/cargo#15457) - Added tracing spans for rustc invocations (rust-lang/cargo#15464) - Trivial tweaks to 'target_short_hash' (rust-lang/cargo#15461) - chore(deps): update msrv (3 versions) to v1.84 (rust-lang/cargo#15456) - feat(add/install): check if given crate argument would be valid with inserted @ symbol (rust-lang/cargo#15441) - chang 1 tries to 1 try (rust-lang/cargo#15328) r? ghost
What does this PR try to resolve?
Cargo registries that return HTTP 429 when the service is overloaded expect the client to retry the request automatically after a delay. Cargo currently does not retry for HTTP 429.
What changed?
In this implementation, the maximum delay is limited to Cargo's existing limit of 10 seconds. We could consider increasing that limit for this case, since the server is explicitly requesting the delay.