-
Notifications
You must be signed in to change notification settings - Fork 529
feat: add HTTP timeout, retries, and concurrency env var support #5581
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+266
−5
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
db694de
feat: support UV_HTTP_TIMEOUT env var for configuring uv HTTP timeouts
claude 931997a
feat: support UV_HTTP_RETRIES and UV_CONCURRENT_* env vars
claude efe4b06
refactor: consolidate uv env var tests
claude 1042a51
run fmt
wolfv df7233d
Merge branch 'main' into claude/fix-uv-http-timeout-f50ek
ruben-arts f8ad7de
chore: cleanup tracing calls
ruben-arts 005a97d
lint
ruben-arts File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| use std::sync::Arc; | ||
| use std::time::Duration; | ||
|
|
||
| use fs_err::create_dir_all; | ||
| use miette::{Context, IntoDiagnostic}; | ||
|
|
@@ -8,7 +9,6 @@ use pixi_utils::reqwest::{ | |
| LazyReqwestClient, should_use_builtin_certs_uv, should_use_native_tls_for_uv, uv_middlewares, | ||
| }; | ||
| use pixi_uv_conversions::{ConversionError, to_uv_trusted_host}; | ||
| use tracing::debug; | ||
| use uv_cache::Cache; | ||
| use uv_client::{ | ||
| BaseClientBuilder, Connectivity, ExtraMiddleware, RegistryClient, RegistryClientBuilder, | ||
|
|
@@ -48,6 +48,108 @@ pub struct UvResolutionContext { | |
| pub extra_build_variables: ExtraBuildVariables, | ||
| pub preview: Preview, | ||
| pub workspace_cache: WorkspaceCache, | ||
| /// HTTP timeout for uv operations, read from UV_HTTP_TIMEOUT, | ||
| /// UV_REQUEST_TIMEOUT, or HTTP_TIMEOUT environment variables. | ||
| pub http_timeout: Option<Duration>, | ||
| /// HTTP retry count for uv operations, read from UV_HTTP_RETRIES. | ||
| pub http_retries: Option<u32>, | ||
| } | ||
|
|
||
| /// Read a `usize` from an environment variable, logging on success or invalid | ||
| /// values. | ||
| fn read_usize_env(var: &str) -> Option<usize> { | ||
| let val = std::env::var(var).ok()?; | ||
| match val.parse::<usize>() { | ||
| Ok(n) if n > 0 => { | ||
| tracing::debug!("using {var}={n}"); | ||
| Some(n) | ||
| } | ||
| _ => { | ||
| tracing::warn!( | ||
| "ignoring invalid value for {var}: {val:?} (expected a positive integer)" | ||
| ); | ||
| None | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Read the HTTP timeout from environment variables. | ||
| /// | ||
| /// Checks `UV_HTTP_TIMEOUT`, `UV_REQUEST_TIMEOUT`, and `HTTP_TIMEOUT` | ||
| /// (in that order of precedence), matching the behavior of the `uv` CLI. | ||
| /// The value should be a number of seconds (e.g., `300` for 5 minutes). | ||
| fn read_http_timeout_from_env() -> Option<Duration> { | ||
| let env_vars = ["UV_HTTP_TIMEOUT", "UV_REQUEST_TIMEOUT", "HTTP_TIMEOUT"]; | ||
| for var in env_vars { | ||
| if let Ok(val) = std::env::var(var) { | ||
| match val.parse::<u64>() { | ||
| Ok(secs) => { | ||
| tracing::debug!("using {var}={secs}s for HTTP timeout"); | ||
| return Some(Duration::from_secs(secs)); | ||
| } | ||
| Err(_) => { | ||
| // Also try parsing as float for values like "30.5" | ||
| match val.parse::<f64>() { | ||
| Ok(secs) if secs >= 0.0 => { | ||
| tracing::debug!("using {var}={secs}s for HTTP timeout"); | ||
| return Some(Duration::from_secs_f64(secs)); | ||
| } | ||
| _ => { | ||
| tracing::warn!( | ||
| "ignoring invalid value for {var}: {val:?} (expected a number of seconds)" | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| None | ||
| } | ||
|
|
||
| /// Read `UV_HTTP_RETRIES` from the environment. | ||
| /// | ||
| /// The value should be a non-negative integer (e.g., `5`). The default in uv | ||
| /// is 3. | ||
| fn read_http_retries_from_env() -> Option<u32> { | ||
| let val = std::env::var("UV_HTTP_RETRIES").ok()?; | ||
| match val.parse::<u32>() { | ||
| Ok(n) => { | ||
| tracing::debug!("using UV_HTTP_RETRIES={n}"); | ||
| Some(n) | ||
| } | ||
| Err(_) => { | ||
| tracing::warn!( | ||
| "ignoring invalid value for UV_HTTP_RETRIES: {val:?} (expected a non-negative integer)" | ||
| ); | ||
| None | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Build a [`Concurrency`] from pixi config and UV environment variables. | ||
| /// | ||
| /// Precedence (highest wins): | ||
| /// 1. `UV_CONCURRENT_DOWNLOADS` / `UV_CONCURRENT_BUILDS` / | ||
| /// `UV_CONCURRENT_INSTALLS` environment variables | ||
| /// 2. Pixi `concurrency.downloads` config value | ||
| /// 3. uv defaults (50 downloads, system threads for builds/installs) | ||
| fn build_concurrency(config: &Config) -> Concurrency { | ||
| let defaults = Concurrency::default(); | ||
|
|
||
| // Start with pixi config for downloads (it defaults to 50, same as uv) | ||
| let downloads = config.max_concurrent_downloads(); | ||
|
|
||
| // Apply UV_ env var overrides | ||
| let downloads = read_usize_env("UV_CONCURRENT_DOWNLOADS").unwrap_or(downloads); | ||
| let builds = read_usize_env("UV_CONCURRENT_BUILDS").unwrap_or(defaults.builds); | ||
| let installs = read_usize_env("UV_CONCURRENT_INSTALLS").unwrap_or(defaults.installs); | ||
|
|
||
| Concurrency { | ||
| downloads, | ||
| builds, | ||
| installs, | ||
| } | ||
| } | ||
|
|
||
| impl UvResolutionContext { | ||
|
|
@@ -63,11 +165,11 @@ impl UvResolutionContext { | |
|
|
||
| let keyring_provider = match config.pypi_config.use_keyring() { | ||
| pixi_config::KeyringProvider::Subprocess => { | ||
| debug!("using uv keyring (subprocess) provider"); | ||
| tracing::debug!("using uv keyring (subprocess) provider"); | ||
| uv_configuration::KeyringProviderType::Subprocess | ||
| } | ||
| pixi_config::KeyringProvider::Disabled => { | ||
| debug!("uv keyring provider is disabled"); | ||
| tracing::debug!("uv keyring provider is disabled"); | ||
| uv_configuration::KeyringProviderType::Disabled | ||
| } | ||
| }; | ||
|
|
@@ -86,12 +188,16 @@ impl UvResolutionContext { | |
| ) | ||
| .into_diagnostic() | ||
| .context("failed to parse trusted host")?; | ||
| let http_timeout = read_http_timeout_from_env(); | ||
| let http_retries = read_http_retries_from_env(); | ||
| let concurrency = build_concurrency(config); | ||
|
|
||
| Ok(Self { | ||
| cache, | ||
| in_flight: InFlight::default(), | ||
| hash_strategy: HashStrategy::None, | ||
| keyring_provider, | ||
| concurrency: Concurrency::default(), | ||
| concurrency, | ||
| source_strategy: SourceStrategy::Enabled, | ||
| capabilities: IndexCapabilities::default(), | ||
| allow_insecure_host, | ||
|
|
@@ -106,6 +212,8 @@ impl UvResolutionContext { | |
| extra_build_variables: ExtraBuildVariables::default(), | ||
| preview: Preview::default(), | ||
| workspace_cache: WorkspaceCache::default(), | ||
| http_timeout, | ||
| http_retries, | ||
| }) | ||
| } | ||
|
|
||
|
|
@@ -143,6 +251,14 @@ impl UvResolutionContext { | |
| .built_in_root_certs(self.use_builtin_certs) | ||
| .extra_middleware(self.extra_middleware.clone()); | ||
|
|
||
| if let Some(timeout) = self.http_timeout { | ||
| base_client_builder = base_client_builder.timeout(timeout); | ||
| } | ||
|
|
||
| if let Some(retries) = self.http_retries { | ||
| base_client_builder = base_client_builder.retries(retries); | ||
| } | ||
|
|
||
| if let Some(markers) = markers { | ||
| base_client_builder = base_client_builder.markers(markers); | ||
| } | ||
|
|
@@ -159,3 +275,148 @@ impl UvResolutionContext { | |
| Arc::new(uv_client_builder.build()) | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use std::sync::Mutex; | ||
|
|
||
| static ENV_MUTEX: Mutex<()> = Mutex::new(()); | ||
|
|
||
| fn with_env_vars<F, R>(vars: &[(&str, Option<&str>)], f: F) -> R | ||
| where | ||
| F: FnOnce() -> R, | ||
| { | ||
| let _lock = ENV_MUTEX.lock().unwrap(); | ||
| let originals: Vec<_> = vars | ||
| .iter() | ||
| .map(|(k, _)| (*k, std::env::var(k).ok())) | ||
| .collect(); | ||
|
|
||
| // SAFETY: We hold ENV_MUTEX to ensure no concurrent env var access. | ||
| unsafe { | ||
| for (k, v) in vars { | ||
| match v { | ||
| Some(val) => std::env::set_var(k, val), | ||
| None => std::env::remove_var(k), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let result = f(); | ||
|
|
||
| unsafe { | ||
| for (k, v) in &originals { | ||
| match v { | ||
| Some(val) => std::env::set_var(k, val), | ||
| None => std::env::remove_var(k), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| result | ||
| } | ||
|
Comment on lines
+286
to
+318
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we had a crate that basically already did this. |
||
|
|
||
| /// Clear all timeout-related env vars for a clean test. | ||
| const TIMEOUT_VARS: [(&str, Option<&str>); 3] = [ | ||
| ("UV_HTTP_TIMEOUT", None), | ||
| ("UV_REQUEST_TIMEOUT", None), | ||
| ("HTTP_TIMEOUT", None), | ||
| ]; | ||
|
|
||
| fn timeout_vars_with<'a>( | ||
| overrides: &'a [(&'a str, &'a str)], | ||
| ) -> Vec<(&'a str, Option<&'a str>)> { | ||
| TIMEOUT_VARS | ||
| .iter() | ||
| .map(|&(k, _)| { | ||
| let val = overrides.iter().find(|(ok, _)| *ok == k).map(|(_, v)| *v); | ||
| (k, val) | ||
| }) | ||
| .collect() | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_http_timeout_precedence_and_parsing() { | ||
| // No env vars → None | ||
| with_env_vars(&TIMEOUT_VARS, || { | ||
| assert!(read_http_timeout_from_env().is_none()); | ||
| }); | ||
|
|
||
| // UV_HTTP_TIMEOUT takes precedence over the others | ||
| with_env_vars( | ||
| &[ | ||
| ("UV_HTTP_TIMEOUT", Some("100")), | ||
| ("UV_REQUEST_TIMEOUT", Some("200")), | ||
| ("HTTP_TIMEOUT", Some("300")), | ||
| ], | ||
| || { | ||
| assert_eq!(read_http_timeout_from_env(), Some(Duration::from_secs(100))); | ||
| }, | ||
| ); | ||
|
|
||
| // Falls through to UV_REQUEST_TIMEOUT, then HTTP_TIMEOUT | ||
| with_env_vars(&timeout_vars_with(&[("UV_REQUEST_TIMEOUT", "200")]), || { | ||
| assert_eq!(read_http_timeout_from_env(), Some(Duration::from_secs(200))); | ||
| }); | ||
| with_env_vars(&timeout_vars_with(&[("HTTP_TIMEOUT", "300")]), || { | ||
| assert_eq!(read_http_timeout_from_env(), Some(Duration::from_secs(300))); | ||
| }); | ||
|
|
||
| // Invalid value is skipped, falls through to next | ||
| with_env_vars( | ||
| &[ | ||
| ("UV_HTTP_TIMEOUT", Some("nope")), | ||
| ("UV_REQUEST_TIMEOUT", Some("200")), | ||
| ("HTTP_TIMEOUT", None), | ||
| ], | ||
| || { | ||
| assert_eq!(read_http_timeout_from_env(), Some(Duration::from_secs(200))); | ||
| }, | ||
| ); | ||
|
|
||
| // Float seconds work | ||
| with_env_vars(&timeout_vars_with(&[("UV_HTTP_TIMEOUT", "30.5")]), || { | ||
| assert_eq!( | ||
| read_http_timeout_from_env(), | ||
| Some(Duration::from_secs_f64(30.5)) | ||
| ); | ||
| }); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_http_retries() { | ||
| with_env_vars(&[("UV_HTTP_RETRIES", None)], || { | ||
| assert!(read_http_retries_from_env().is_none()); | ||
| }); | ||
| with_env_vars(&[("UV_HTTP_RETRIES", Some("5"))], || { | ||
| assert_eq!(read_http_retries_from_env(), Some(5)); | ||
| }); | ||
| with_env_vars(&[("UV_HTTP_RETRIES", Some("0"))], || { | ||
| assert_eq!(read_http_retries_from_env(), Some(0)); | ||
| }); | ||
| with_env_vars(&[("UV_HTTP_RETRIES", Some("abc"))], || { | ||
| assert!(read_http_retries_from_env().is_none()); | ||
| }); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_read_usize_env() { | ||
| with_env_vars(&[("UV_CONCURRENT_DOWNLOADS", Some("10"))], || { | ||
| assert_eq!(read_usize_env("UV_CONCURRENT_DOWNLOADS"), Some(10)); | ||
| }); | ||
| // Zero and invalid values are rejected | ||
| for bad in ["0", "-1", "abc"] { | ||
| with_env_vars(&[("UV_CONCURRENT_DOWNLOADS", Some(bad))], || { | ||
| assert!( | ||
| read_usize_env("UV_CONCURRENT_DOWNLOADS").is_none(), | ||
| "expected None for {bad:?}" | ||
| ); | ||
| }); | ||
| } | ||
| // Unset → None | ||
| with_env_vars(&[("UV_CONCURRENT_DOWNLOADS", None)], || { | ||
| assert!(read_usize_env("UV_CONCURRENT_DOWNLOADS").is_none()); | ||
| }); | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should double check if we should use
read_timeouthere?timeoutis a "global" timeout (ie. deadline for the entire request) but read timeout refreshes after each "chunk" that was read.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The uv client builder uses
read_timeoutfor the timeout: