From d029b25fe63269ad76827562f3fe4a8828a988dc Mon Sep 17 00:00:00 2001 From: Mitchell Spencer Date: Mon, 16 Feb 2026 17:58:20 -0500 Subject: [PATCH 1/4] feat(ampup): add token resolution chain and --jobs CLI flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add GitHub token resolution with fallback chain: explicit token → `gh auth token` → unauthenticated. Add `--jobs` / `-j` CLI flag to `install` and `update` commands for controlling concurrent downloads (default: 4). --- ampup/src/commands/init.rs | 1 + ampup/src/commands/install.rs | 10 ++++- ampup/src/main.rs | 26 ++++++++++-- ampup/src/tests/it_ampup.rs | 4 ++ ampup/src/token.rs | 75 +++++++++++++++++++++++++++++++++++ 5 files changed, 111 insertions(+), 5 deletions(-) create mode 100644 ampup/src/token.rs diff --git a/ampup/src/commands/init.rs b/ampup/src/commands/init.rs index 20ce048..7a1acad 100644 --- a/ampup/src/commands/init.rs +++ b/ampup/src/commands/init.rs @@ -98,6 +98,7 @@ pub async fn run( None, None, None, + None, ) .await?; } else { diff --git a/ampup/src/commands/install.rs b/ampup/src/commands/install.rs index 684c4bd..630a289 100644 --- a/ampup/src/commands/install.rs +++ b/ampup/src/commands/install.rs @@ -5,7 +5,7 @@ use crate::{ github::GitHubClient, install::Installer, platform::{Architecture, Platform}, - ui, + token, ui, version_manager::VersionManager, }; @@ -16,9 +16,15 @@ pub async fn run( version: Option, arch_override: Option, platform_override: Option, + jobs: Option, ) -> Result<()> { let config = Config::new(install_dir)?; - let github = GitHubClient::new(repo, github_token)?; + let _max_concurrent = jobs.unwrap_or(4); + + // Resolve token with fallback chain: explicit → gh auth token → unauthenticated + let resolved_token = token::resolve_github_token(github_token); + + let github = GitHubClient::new(repo, resolved_token)?; let version_manager = VersionManager::new(config); // Determine version to install diff --git a/ampup/src/main.rs b/ampup/src/main.rs index feceb6f..63687a1 100644 --- a/ampup/src/main.rs +++ b/ampup/src/main.rs @@ -57,6 +57,10 @@ enum Commands { /// Override platform detection (linux, darwin) #[arg(long)] platform: Option, + + /// Number of concurrent downloads (default: 4) + #[arg(short = 'j', long = "jobs")] + jobs: Option, }, /// List installed versions @@ -142,6 +146,10 @@ enum Commands { /// Override platform detection (linux, darwin) #[arg(long)] platform: Option, + + /// Number of concurrent downloads (default: 4) + #[arg(short = 'j', long = "jobs")] + jobs: Option, }, /// Manage the ampup executable @@ -198,9 +206,18 @@ async fn run() -> anyhow::Result<()> { github_token, arch, platform, + jobs, }) => { - commands::install::run(install_dir, repo, github_token, version, arch, platform) - .await?; + commands::install::run( + install_dir, + repo, + github_token, + version, + arch, + platform, + jobs, + ) + .await?; } Some(Commands::List { install_dir }) => { commands::list::run(install_dir)?; @@ -235,9 +252,11 @@ async fn run() -> anyhow::Result<()> { github_token, arch, platform, + jobs, }) => { // Install latest version (same as default behavior) - commands::install::run(install_dir, repo, github_token, None, arch, platform).await?; + commands::install::run(install_dir, repo, github_token, None, arch, platform, jobs) + .await?; } Some(Commands::SelfCmd { command }) => match command { SelfCommands::Update { repo, github_token } => { @@ -256,6 +275,7 @@ async fn run() -> anyhow::Result<()> { None, None, None, + None, ) .await?; } diff --git a/ampup/src/tests/it_ampup.rs b/ampup/src/tests/it_ampup.rs index eaaae4e..22603eb 100644 --- a/ampup/src/tests/it_ampup.rs +++ b/ampup/src/tests/it_ampup.rs @@ -180,6 +180,7 @@ async fn install_latest_version() -> Result<()> { None, None, None, + None, ) .await?; @@ -208,6 +209,7 @@ async fn install_specific_version() -> Result<()> { Some(version.to_string()), None, None, + None, ) .await?; @@ -234,6 +236,7 @@ async fn install_already_installed_version_switches_to_it() -> Result<()> { Some(version.to_string()), None, None, + None, ) .await?; @@ -245,6 +248,7 @@ async fn install_already_installed_version_switches_to_it() -> Result<()> { Some(version.to_string()), None, None, + None, ) .await?; diff --git a/ampup/src/token.rs b/ampup/src/token.rs new file mode 100644 index 0000000..44763c4 --- /dev/null +++ b/ampup/src/token.rs @@ -0,0 +1,75 @@ +use std::process::Command; + +/// Resolve a GitHub token using the following fallback chain: +/// +/// 1. Explicit token passed via `--github-token` flag or `GITHUB_TOKEN` env var +/// 2. Token from `gh auth token` (GitHub CLI) +/// 3. `None` (unauthenticated — lower rate limits) +/// +/// Note: `--github-token` values may be visible in process listings (`ps aux`). +/// Prefer `GITHUB_TOKEN` env var or `gh auth token` for sensitive environments. +pub fn resolve_github_token(explicit: Option) -> Option { + if explicit.is_some() { + return explicit; + } + + try_gh_auth_token() +} + +/// Attempt to retrieve a token from the GitHub CLI. +/// +/// Runs `gh auth token` as a subprocess. Returns `None` on any failure: +/// `gh` not installed, not logged in, timeout, etc. +fn try_gh_auth_token() -> Option { + let output = Command::new("gh") + .args(["auth", "token"]) + .stdin(std::process::Stdio::null()) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::null()) + .output() + .ok()?; + + if !output.status.success() { + return None; + } + + let token = String::from_utf8(output.stdout).ok()?.trim().to_string(); + + if token.is_empty() { + return None; + } + + Some(token) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn resolve_github_token_with_explicit_token_returns_explicit() { + //* Given + let explicit = Some("my-explicit-token".to_string()); + + //* When + let result = resolve_github_token(explicit); + + //* Then + assert_eq!( + result, + Some("my-explicit-token".to_string()), + "should return the explicit token without falling through to gh CLI" + ); + } + + #[test] + fn resolve_github_token_with_none_exercises_fallback_without_panicking() { + //* Given — no explicit token provided + + //* When — the result depends on whether `gh` is installed and + //* authenticated, so we only assert it doesn't panic. + let _result = resolve_github_token(None); + + //* Then — reaching this point means the fallback chain completed + } +} From 1f414378699a6754e3770a6dc6394b1a2d45e7f5 Mon Sep 17 00:00:00 2001 From: Mitchell Spencer Date: Mon, 16 Feb 2026 17:58:31 -0500 Subject: [PATCH 2/4] feat(ampup): add rate limiter with 429/403 handling and one-retry logic Add GitHubRateLimiter that parses X-RateLimit-Remaining, X-RateLimit-Reset, and Retry-After headers. On 429 or rate-limit 403, sets a global pause shared across all concurrent downloads and retries once. Preemptively pauses when remaining hits 0. Integrates into GitHubClient via send_with_rate_limit, which wraps all HTTP methods with rate-limit awareness. --- ampup/src/github.rs | 102 +++++++-- ampup/src/lib.rs | 2 + ampup/src/rate_limiter.rs | 422 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 510 insertions(+), 16 deletions(-) create mode 100644 ampup/src/rate_limiter.rs diff --git a/ampup/src/github.rs b/ampup/src/github.rs index 0192d98..3a4c67c 100644 --- a/ampup/src/github.rs +++ b/ampup/src/github.rs @@ -1,8 +1,12 @@ +use std::sync::Arc; + use anyhow::{Context, Result}; use futures::StreamExt; use indicatif::{ProgressBar, ProgressStyle}; use serde::Deserialize; +use crate::rate_limiter::GitHubRateLimiter; + const AMPUP_API_URL: &str = "https://ampup.sh/api"; const GITHUB_API_URL: &str = "https://api.github.com"; @@ -37,6 +41,10 @@ pub enum GitHubError { url: String, body: String, }, + RateLimited { + retry_after_secs: u64, + has_token: bool, + }, } impl std::fmt::Display for GitHubError { @@ -144,6 +152,18 @@ impl std::fmt::Display for GitHubError { writeln!(f, " Response: {}", body)?; } } + Self::RateLimited { + retry_after_secs, + has_token, + } => { + writeln!(f, "GitHub API rate limit exceeded")?; + writeln!(f, " Retry after: {} seconds", retry_after_secs)?; + writeln!(f)?; + if !*has_token { + writeln!(f, " Unauthenticated requests have lower rate limits.")?; + writeln!(f, " Try: export GITHUB_TOKEN=$(gh auth token)")?; + } + } } Ok(()) } @@ -166,12 +186,16 @@ struct Asset { url: String, } +/// Clone is cheap: `reqwest::Client` and `rate_limiter` are both `Arc`-backed. +/// Needed so `DownloadManager` can move a handle into each spawned download task. +#[derive(Clone)] pub struct GitHubClient { client: reqwest::Client, repo: String, token: Option, /// Base URL for API requests (either custom API or GitHub API) api: String, + rate_limiter: Arc, } impl GitHubClient { @@ -203,11 +227,14 @@ impl GitHubClient { format!("{}/repos/{}/releases", GITHUB_API_URL, repo) }; + let rate_limiter = Arc::new(GitHubRateLimiter::new(github_token.is_some())); + Ok(Self { client, repo, token: github_token, api, + rate_limiter, }) } @@ -227,16 +254,59 @@ impl GitHubClient { self.get_release(&format!("tags/{}", version)).await } + /// Send a request with rate-limit awareness and one retry on 429. + async fn send_with_rate_limit( + &self, + build_request: impl Fn() -> reqwest::RequestBuilder, + context_msg: &str, + ) -> Result { + self.rate_limiter.wait_if_paused().await; + + let response = build_request() + .send() + .await + .context(context_msg.to_string())?; + + if let Some(retry_after) = self.rate_limiter.update_from_response(&response).await { + crate::ui::warn!( + "Rate limited by GitHub API, retrying in {} seconds...", + retry_after + ); + self.rate_limiter.wait_if_paused().await; + + let response = build_request() + .send() + .await + .context(context_msg.to_string())?; + + if let Some(retry_after) = self.rate_limiter.update_from_response(&response).await { + return Err(GitHubError::RateLimited { + retry_after_secs: retry_after, + has_token: self.token.is_some(), + } + .into()); + } + + return Ok(response); + } + + // Warn if rate limit is exhausted (preemptive pause applies to next request) + if self.rate_limiter.remaining().await == Some(0) { + crate::ui::warn!( + "GitHub API rate limit exhausted, subsequent requests will be paused until reset" + ); + } + + Ok(response) + } + /// Fetch release from GitHub API async fn get_release(&self, path: &str) -> Result { let url = format!("{}/{}", self.api, path); let response = self - .client - .get(&url) - .send() - .await - .context("Failed to fetch release")?; + .send_with_rate_limit(|| self.client.get(&url), "Failed to fetch release") + .await?; if !response.status().is_success() { let status = response.status(); @@ -312,12 +382,15 @@ impl GitHubClient { ); let response = self - .client - .get(&url) - .header(reqwest::header::ACCEPT, "application/octet-stream") - .send() - .await - .context("Failed to download asset")?; + .send_with_rate_limit( + || { + self.client + .get(&url) + .header(reqwest::header::ACCEPT, "application/octet-stream") + }, + "Failed to download asset", + ) + .await?; self.download_with_progress(response, &url, asset_name) .await @@ -326,11 +399,8 @@ impl GitHubClient { /// Download asset directly (for public repos) async fn download_asset_direct(&self, url: &str, asset_name: &str) -> Result> { let response = self - .client - .get(url) - .send() - .await - .context("Failed to download asset")?; + .send_with_rate_limit(|| self.client.get(url), "Failed to download asset") + .await?; self.download_with_progress(response, url, asset_name).await } diff --git a/ampup/src/lib.rs b/ampup/src/lib.rs index c6b953f..8484923 100644 --- a/ampup/src/lib.rs +++ b/ampup/src/lib.rs @@ -4,7 +4,9 @@ pub mod config; pub mod github; pub mod install; pub mod platform; +pub mod rate_limiter; pub mod shell; +pub mod token; pub mod updater; pub mod version_manager; diff --git a/ampup/src/rate_limiter.rs b/ampup/src/rate_limiter.rs new file mode 100644 index 0000000..d48f4b9 --- /dev/null +++ b/ampup/src/rate_limiter.rs @@ -0,0 +1,422 @@ +use std::time::{Duration, Instant, SystemTime}; + +use tokio::sync::Mutex; + +/// Shared rate limiter that respects GitHub API rate-limit headers. +/// +/// All concurrent downloads share one `GitHubRateLimiter` so that a 429 +/// response pauses every in-flight request, not just the one that triggered it. +pub struct GitHubRateLimiter { + inner: Mutex, + has_token: bool, +} + +struct RateLimiterState { + paused_until: Option, + remaining: Option, +} + +impl GitHubRateLimiter { + /// Create a new rate limiter. + pub fn new(has_token: bool) -> Self { + Self { + inner: Mutex::new(RateLimiterState { + paused_until: None, + remaining: None, + }), + has_token, + } + } + + /// Whether the client has an authentication token. + pub fn has_token(&self) -> bool { + self.has_token + } + + /// Block until any active rate-limit pause has expired. + pub async fn wait_if_paused(&self) { + let wait_duration = { + let state = self.inner.lock().await; + state.paused_until.and_then(|until| { + let now = Instant::now(); + if until > now { Some(until - now) } else { None } + }) + }; + + if let Some(duration) = wait_duration { + tokio::time::sleep(duration).await; + } + } + + /// Inspect a response and update rate-limit state. + /// + /// Parses `X-RateLimit-Remaining`, `X-RateLimit-Reset`, and `Retry-After` + /// headers. On HTTP 429, sets a global pause and returns + /// `Some(retry_after_secs)`. When remaining hits 0, preemptively pauses + /// until the reset timestamp. Returns `None` for non-429 responses. + /// + /// Header names per GitHub REST API docs: + /// https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api + pub async fn update_from_response(&self, response: &reqwest::Response) -> Option { + let status = response.status(); + let remaining = Self::parse_header_u64(response, "x-ratelimit-remaining"); + let reset_at = Self::parse_header_u64(response, "x-ratelimit-reset"); + let retry_after = Self::parse_header_u64(response, "retry-after"); + + self.update_state(status, remaining, reset_at, retry_after) + .await + } + + /// Core rate-limit state machine, separated for testability. + async fn update_state( + &self, + status: reqwest::StatusCode, + remaining: Option, + reset_at: Option, + retry_after: Option, + ) -> Option { + let mut state = self.inner.lock().await; + + if let Some(rem) = remaining { + state.remaining = Some(rem); + } + + // GitHub returns 429 or 403 for rate limiting. Treat 403 as rate-limited + // only when there's a clear signal (retry-after present or remaining is 0) + // to avoid confusing it with a permissions error. + let is_rate_limited = status == reqwest::StatusCode::TOO_MANY_REQUESTS + || (status == reqwest::StatusCode::FORBIDDEN + && (retry_after.is_some() || remaining == Some(0))); + + if is_rate_limited { + // Retry-After is not guaranteed to be present on rate-limit responses. + // GitHub docs recommend waiting at least one minute when absent. + let secs = retry_after.unwrap_or(60); + let pause_until = Instant::now() + Duration::from_secs(secs); + Self::extend_pause(&mut state, pause_until); + return Some(secs); + } + + // Preemptive pause when remaining hits 0 + if remaining == Some(0) + && let Some(reset) = reset_at + { + let now_unix = SystemTime::now() + .duration_since(SystemTime::UNIX_EPOCH) + .unwrap_or_default() + .as_secs(); + if reset > now_unix { + let pause_until = Instant::now() + Duration::from_secs(reset - now_unix); + Self::extend_pause(&mut state, pause_until); + } + } + + None + } + + /// Extend the pause window, never shortening an existing one. + fn extend_pause(state: &mut RateLimiterState, pause_until: Instant) { + match state.paused_until { + Some(existing) if existing > pause_until => {} + _ => state.paused_until = Some(pause_until), + } + } + + /// Current remaining API calls, if known. + pub async fn remaining(&self) -> Option { + self.inner.lock().await.remaining + } + + fn parse_header_u64(response: &reqwest::Response, name: &str) -> Option { + response + .headers() + .get(name) + .and_then(|v| v.to_str().ok()) + .and_then(|v| v.parse().ok()) + } +} + +#[cfg(test)] +mod tests { + //! Tests are organized into nested modules by the method under test, + //! following the project convention for modules with 10+ tests (see + //! docs/code/test-files.md "Module Structure Within cfg(test)"). + + use tokio::io::{AsyncReadExt, AsyncWriteExt}; + + use super::*; + + /// Spawn a one-shot TCP server that returns a raw HTTP response. + /// Accepts one connection, drains the request, writes `response_bytes`, then closes. + async fn mock_http_response(response_bytes: Vec) -> std::net::SocketAddr { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0") + .await + .expect("should bind to a random port"); + let addr = listener.local_addr().expect("should have a local address"); + + tokio::spawn(async move { + let (mut stream, _) = listener.accept().await.expect("should accept a connection"); + let mut buf = [0u8; 1024]; + let _ = stream.read(&mut buf).await; + stream + .write_all(&response_bytes) + .await + .expect("should write response"); + }); + + addr + } + + /// Tests for the blocking gate that callers use before making HTTP requests. + mod wait_if_paused { + use super::*; + + #[tokio::test] + async fn with_no_active_pause_returns_immediately() { + //* Given + let limiter = GitHubRateLimiter::new(true); + + //* When + let start = Instant::now(); + limiter.wait_if_paused().await; + + //* Then + assert!( + start.elapsed() < Duration::from_millis(50), + "should return immediately when no pause is set" + ); + } + + #[tokio::test] + async fn with_expired_pause_returns_immediately() { + //* Given + let limiter = GitHubRateLimiter::new(false); + { + let mut state = limiter.inner.lock().await; + state.paused_until = Some(Instant::now() - Duration::from_secs(1)); + } + + //* When + let start = Instant::now(); + limiter.wait_if_paused().await; + + //* Then + assert!( + start.elapsed() < Duration::from_millis(50), + "should return immediately when pause has already expired" + ); + } + + #[tokio::test] + async fn with_active_pause_blocks_until_expiry() { + //* Given + let limiter = GitHubRateLimiter::new(true); + { + let mut state = limiter.inner.lock().await; + state.paused_until = Some(Instant::now() + Duration::from_millis(100)); + } + + //* When + let start = Instant::now(); + limiter.wait_if_paused().await; + + //* Then + assert!( + start.elapsed() >= Duration::from_millis(90), + "should block for approximately the pause duration" + ); + } + } + + /// Tests for the core state machine using direct values (no HTTP involved). + /// Covers edge cases like missing headers, 403 vs 429 disambiguation, and + /// expired reset timestamps. + mod update_state { + use super::*; + + #[tokio::test] + async fn with_429_and_no_retry_after_defaults_to_60s() { + //* Given + let limiter = GitHubRateLimiter::new(false); + + //* When + let result = limiter + .update_state(reqwest::StatusCode::TOO_MANY_REQUESTS, None, None, None) + .await; + + //* Then + assert_eq!( + result, + Some(60), + "should default to 60 seconds when Retry-After header is absent" + ); + } + + #[tokio::test] + async fn with_403_and_remaining_zero_treats_as_rate_limited() { + //* Given + let limiter = GitHubRateLimiter::new(true); + + //* When + let result = limiter + .update_state(reqwest::StatusCode::FORBIDDEN, Some(0), None, None) + .await; + + //* Then + assert_eq!( + result, + Some(60), + "should treat 403 with remaining=0 as rate-limited" + ); + } + + #[tokio::test] + async fn with_403_and_no_rate_limit_signal_ignores() { + //* Given + let limiter = GitHubRateLimiter::new(true); + + //* When — 403 without retry-after or remaining=0 is a permissions error, not rate limiting + let result = limiter + .update_state(reqwest::StatusCode::FORBIDDEN, None, None, None) + .await; + + //* Then + assert_eq!(result, None, "should not treat a plain 403 as rate-limited"); + } + + #[tokio::test] + async fn with_remaining_zero_and_past_reset_skips_pause() { + //* Given + let limiter = GitHubRateLimiter::new(true); + let past_reset = SystemTime::now() + .duration_since(SystemTime::UNIX_EPOCH) + .expect("system time should be after epoch") + .as_secs() + - 60; + + //* When + limiter + .update_state(reqwest::StatusCode::OK, Some(0), Some(past_reset), None) + .await; + + //* Then + let state = limiter.inner.lock().await; + assert!( + state.paused_until.is_none(), + "should not pause when the reset timestamp is already in the past" + ); + } + } + + /// End-to-end tests that send real HTTP through reqwest to verify that the + /// header names (`X-RateLimit-Remaining`, `X-RateLimit-Reset`, `Retry-After`) + /// are parsed correctly from actual HTTP responses. + mod update_from_response { + use super::*; + + #[tokio::test] + async fn with_ok_status_parses_remaining_header() { + //* Given + let addr = mock_http_response( + b"HTTP/1.1 200 OK\r\n\ + X-RateLimit-Remaining: 42\r\n\ + X-RateLimit-Reset: 1700000000\r\n\ + Content-Length: 0\r\n\ + \r\n" + .to_vec(), + ) + .await; + + let limiter = GitHubRateLimiter::new(true); + let client = reqwest::Client::new(); + let response = client + .get(format!("http://{}", addr)) + .send() + .await + .expect("request to mock server should succeed"); + + //* When + let result = limiter.update_from_response(&response).await; + + //* Then + assert_eq!(result, None, "should not signal retry for 200 OK"); + assert_eq!( + limiter.remaining().await, + Some(42), + "should parse X-RateLimit-Remaining header" + ); + } + + #[tokio::test] + async fn with_429_status_returns_retry_after() { + //* Given + let addr = mock_http_response( + b"HTTP/1.1 429 Too Many Requests\r\n\ + Retry-After: 30\r\n\ + X-RateLimit-Remaining: 0\r\n\ + Content-Length: 0\r\n\ + \r\n" + .to_vec(), + ) + .await; + + let limiter = GitHubRateLimiter::new(true); + let client = reqwest::Client::new(); + let response = client + .get(format!("http://{}", addr)) + .send() + .await + .expect("request to mock server should succeed"); + + //* When + let result = limiter.update_from_response(&response).await; + + //* Then + assert_eq!(result, Some(30), "should return Retry-After value on 429"); + assert_eq!( + limiter.remaining().await, + Some(0), + "should parse X-RateLimit-Remaining from 429 response" + ); + } + + #[tokio::test] + async fn with_remaining_zero_sets_preemptive_pause() { + //* Given + let future_reset = SystemTime::now() + .duration_since(SystemTime::UNIX_EPOCH) + .expect("system time should be after epoch") + .as_secs() + + 120; + let response_str = format!( + "HTTP/1.1 200 OK\r\n\ + X-RateLimit-Remaining: 0\r\n\ + X-RateLimit-Reset: {}\r\n\ + Content-Length: 0\r\n\ + \r\n", + future_reset + ); + + let addr = mock_http_response(response_str.into_bytes()).await; + + let limiter = GitHubRateLimiter::new(true); + let client = reqwest::Client::new(); + let response = client + .get(format!("http://{}", addr)) + .send() + .await + .expect("request to mock server should succeed"); + + //* When + let result = limiter.update_from_response(&response).await; + + //* Then + assert_eq!(result, None, "should not signal retry for 200 OK"); + let state = limiter.inner.lock().await; + assert!( + state.paused_until.is_some(), + "should set preemptive pause when remaining is 0 with future reset" + ); + } + } +} From 25c7b77e82f6d7af99dbd6a9dc8dc65a743ee1c2 Mon Sep 17 00:00:00 2001 From: Mitchell Spencer Date: Tue, 17 Feb 2026 10:08:40 -0500 Subject: [PATCH 3/4] refactor(ampup): minor review polish for PR 1 Add comment explaining deferred _max_concurrent usage and use lazy with_context() for error string allocation in send_with_rate_limit. --- ampup/src/commands/install.rs | 1 + ampup/src/github.rs | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/ampup/src/commands/install.rs b/ampup/src/commands/install.rs index 630a289..8575d1b 100644 --- a/ampup/src/commands/install.rs +++ b/ampup/src/commands/install.rs @@ -19,6 +19,7 @@ pub async fn run( jobs: Option, ) -> Result<()> { let config = Config::new(install_dir)?; + // Will be passed to DownloadManager for bounded-concurrent downloads let _max_concurrent = jobs.unwrap_or(4); // Resolve token with fallback chain: explicit → gh auth token → unauthenticated diff --git a/ampup/src/github.rs b/ampup/src/github.rs index 3a4c67c..3c8331b 100644 --- a/ampup/src/github.rs +++ b/ampup/src/github.rs @@ -265,7 +265,7 @@ impl GitHubClient { let response = build_request() .send() .await - .context(context_msg.to_string())?; + .with_context(|| context_msg.to_string())?; if let Some(retry_after) = self.rate_limiter.update_from_response(&response).await { crate::ui::warn!( @@ -277,7 +277,7 @@ impl GitHubClient { let response = build_request() .send() .await - .context(context_msg.to_string())?; + .with_context(|| context_msg.to_string())?; if let Some(retry_after) = self.rate_limiter.update_from_response(&response).await { return Err(GitHubError::RateLimited { From 005c1d68e2765a89b1e05b945baa8f8d3a74a06c Mon Sep 17 00:00:00 2001 From: Mitchell Spencer Date: Wed, 18 Feb 2026 14:13:39 -0500 Subject: [PATCH 4/4] fix(ampup): fail fast on long rate-limit pauses instead of silently blocking Pauses exceeding 60s now return an actionable error prompting users to set GITHUB_TOKEN, instead of sleeping for up to an hour with no output. --- ampup/src/github.rs | 16 +++++++++++-- ampup/src/rate_limiter.rs | 48 +++++++++++++++++++++++++++++++++++---- 2 files changed, 58 insertions(+), 6 deletions(-) diff --git a/ampup/src/github.rs b/ampup/src/github.rs index 3c8331b..1da2fad 100644 --- a/ampup/src/github.rs +++ b/ampup/src/github.rs @@ -254,13 +254,25 @@ impl GitHubClient { self.get_release(&format!("tags/{}", version)).await } + /// Wait for any active rate-limit pause, or fail if the wait is too long. + async fn check_rate_limit_pause(&self) -> Result<()> { + if let Err(duration) = self.rate_limiter.wait_if_paused().await { + return Err(GitHubError::RateLimited { + retry_after_secs: duration.as_secs(), + has_token: self.token.is_some(), + } + .into()); + } + Ok(()) + } + /// Send a request with rate-limit awareness and one retry on 429. async fn send_with_rate_limit( &self, build_request: impl Fn() -> reqwest::RequestBuilder, context_msg: &str, ) -> Result { - self.rate_limiter.wait_if_paused().await; + self.check_rate_limit_pause().await?; let response = build_request() .send() @@ -272,7 +284,7 @@ impl GitHubClient { "Rate limited by GitHub API, retrying in {} seconds...", retry_after ); - self.rate_limiter.wait_if_paused().await; + self.check_rate_limit_pause().await?; let response = build_request() .send() diff --git a/ampup/src/rate_limiter.rs b/ampup/src/rate_limiter.rs index d48f4b9..4d6cb15 100644 --- a/ampup/src/rate_limiter.rs +++ b/ampup/src/rate_limiter.rs @@ -34,7 +34,12 @@ impl GitHubRateLimiter { } /// Block until any active rate-limit pause has expired. - pub async fn wait_if_paused(&self) { + /// + /// Returns `Err(remaining_duration)` if the pause exceeds 60 seconds, + /// so the caller can fail immediately with an actionable error instead of + /// silently blocking for a long time (e.g., unauthenticated rate-limit + /// resets can be up to ~60 minutes). + pub async fn wait_if_paused(&self) -> Result<(), Duration> { let wait_duration = { let state = self.inner.lock().await; state.paused_until.and_then(|until| { @@ -44,8 +49,13 @@ impl GitHubRateLimiter { }; if let Some(duration) = wait_duration { + if duration > Duration::from_secs(60) { + return Err(duration); + } tokio::time::sleep(duration).await; } + + Ok(()) } /// Inspect a response and update rate-limit state. @@ -178,9 +188,10 @@ mod tests { //* When let start = Instant::now(); - limiter.wait_if_paused().await; + let result = limiter.wait_if_paused().await; //* Then + assert!(result.is_ok(), "should succeed when no pause is set"); assert!( start.elapsed() < Duration::from_millis(50), "should return immediately when no pause is set" @@ -198,9 +209,10 @@ mod tests { //* When let start = Instant::now(); - limiter.wait_if_paused().await; + let result = limiter.wait_if_paused().await; //* Then + assert!(result.is_ok(), "should succeed when pause has expired"); assert!( start.elapsed() < Duration::from_millis(50), "should return immediately when pause has already expired" @@ -218,14 +230,42 @@ mod tests { //* When let start = Instant::now(); - limiter.wait_if_paused().await; + let result = limiter.wait_if_paused().await; //* Then + assert!(result.is_ok(), "should succeed for short pauses"); assert!( start.elapsed() >= Duration::from_millis(90), "should block for approximately the pause duration" ); } + + #[tokio::test] + async fn with_pause_exceeding_max_fails_immediately() { + //* Given + let limiter = GitHubRateLimiter::new(false); + { + let mut state = limiter.inner.lock().await; + state.paused_until = + Some(Instant::now() + Duration::from_secs(60 + 1)); + } + + //* When + let start = Instant::now(); + let result = limiter.wait_if_paused().await; + + //* Then + assert!(result.is_err(), "should fail when pause exceeds 60"); + assert!( + start.elapsed() < Duration::from_millis(50), + "should fail immediately without sleeping" + ); + let duration = result.unwrap_err(); + assert!( + duration > Duration::from_secs(60), + "should return the remaining pause duration" + ); + } } /// Tests for the core state machine using direct values (no HTTP involved).