From 13392605935f1d873391f87e405cc1ea0e4aab57 Mon Sep 17 00:00:00 2001 From: Rabindra Dhakal Date: Sun, 1 Nov 2026 18:22:07 +0545 Subject: [PATCH 1/5] feat(dl): segmented multi-connection downloads --- crates/soar-cli/src/cli.rs | 5 + crates/soar-cli/src/download.rs | 14 +- crates/soar-cli/src/main.rs | 6 + crates/soar-config/src/config.rs | 22 +- crates/soar-core/src/package/install.rs | 21 +- crates/soar-dl/src/download.rs | 240 ++++- crates/soar-dl/src/error.rs | 9 + crates/soar-dl/src/http.rs | 130 ++- crates/soar-dl/src/lib.rs | 1 + crates/soar-dl/src/oci.rs | 6 +- crates/soar-dl/src/segmented.rs | 1058 +++++++++++++++++++++++ crates/soar-dl/src/test_server.rs | 133 ++- crates/soar-dl/src/types.rs | 47 + crates/soar-dl/src/utils.rs | 20 + crates/soar-dl/src/xattr.rs | 2 + docs/configuration.md | 1 + docs/download.md | 1 + 17 files changed, 1671 insertions(+), 45 deletions(-) create mode 100644 crates/soar-dl/src/segmented.rs diff --git a/crates/soar-cli/src/cli.rs b/crates/soar-cli/src/cli.rs index fe91ad51..0e7e3349 100644 --- a/crates/soar-cli/src/cli.rs +++ b/crates/soar-cli/src/cli.rs @@ -456,6 +456,11 @@ pub enum Commands { /// Overwrite existing download with same file #[arg(required = false, long)] force_overwrite: bool, + + /// Number of connections used per download; more than one fetches a + /// range-capable server's file in parallel segments + #[arg(required = false, long)] + connections: Option, }, /// Health check diff --git a/crates/soar-cli/src/download.rs b/crates/soar-cli/src/download.rs index 8462e8fb..87b455db 100644 --- a/crates/soar-cli/src/download.rs +++ b/crates/soar-cli/src/download.rs @@ -35,6 +35,9 @@ pub struct DownloadContext { pub extract_dir: Option, pub skip_existing: bool, pub force_overwrite: bool, + /// Connections each download may open; more than one downloads a + /// range-capable server's file in parallel segments. + pub connections: usize, } impl DownloadContext { @@ -124,7 +127,8 @@ async fn download_link(ctx: &DownloadContext, link: &str) -> SoarResult<()> { let mut dl = Download::new(url) .overwrite(ctx.get_overwrite_mode()) - .extract(ctx.extract); + .extract(ctx.extract) + .connections(ctx.connections); if let Some(ref out) = ctx.output { dl = dl.output(out); @@ -266,8 +270,9 @@ async fn download_link(ctx: &DownloadContext, link: &str) -> SoarResult<()> { dl.execute()?; } else { - let mut dl = - Download::new(&package.download_url).overwrite(ctx.get_overwrite_mode()); + let mut dl = Download::new(&package.download_url) + .overwrite(ctx.get_overwrite_mode()) + .connections(ctx.connections); if let Some(ref out) = ctx.output { dl = dl.output(out); @@ -384,7 +389,8 @@ fn handle_forge_release( let mut dl = Download::new(selected_asset.url()) .overwrite(ctx.get_overwrite_mode()) - .extract(ctx.extract); + .extract(ctx.extract) + .connections(ctx.connections); if let Some(ref out) = ctx.output { dl = dl.output(out); diff --git a/crates/soar-cli/src/main.rs b/crates/soar-cli/src/main.rs index 9c1e9e53..04336064 100644 --- a/crates/soar-cli/src/main.rs +++ b/crates/soar-cli/src/main.rs @@ -582,6 +582,7 @@ async fn handle_cli() -> SoarResult<()> { extract_dir, skip_existing, force_overwrite, + connections, } => { let progress_callback: Arc = { let pb: Arc>> = Arc::new(Mutex::new(None)); @@ -596,6 +597,10 @@ async fn handle_cli() -> SoarResult<()> { let match_keywords = match_keywords.unwrap_or_default(); let exclude_keywords = exclude_keywords.unwrap_or_default(); + let connections = connections + .or(get_config().download_concurrency) + .unwrap_or(8) + .max(1); let context = DownloadContext { regexes, globs, @@ -609,6 +614,7 @@ async fn handle_cli() -> SoarResult<()> { extract_dir, skip_existing, force_overwrite, + connections, }; download(context, links, github, gitlab, codeberg, gitea, ghcr).await?; diff --git a/crates/soar-config/src/config.rs b/crates/soar-config/src/config.rs index bd74f3c7..59b563a7 100644 --- a/crates/soar-config/src/config.rs +++ b/crates/soar-config/src/config.rs @@ -75,6 +75,11 @@ pub struct Config { /// Default: 8 pub ghcr_concurrency: Option, + /// Number of connections each download may open. More than one splits a + /// range-capable server's file into parallel segments. + /// Default: 8 + pub download_concurrency: Option, + /// Limits the number of results returned by a search. /// Default: 20 pub search_limit: Option, @@ -373,6 +378,7 @@ impl Config { parallel_limit: Some(4), search_limit: Some(20), ghcr_concurrency: Some(8), + download_concurrency: Some(8), cross_repo_updates: Some(false), install_patterns: Some(default_install_patterns()), completions: None, @@ -463,6 +469,7 @@ impl Config { parallel_limit: Some(4), search_limit: Some(20), ghcr_concurrency: Some(8), + download_concurrency: Some(8), cross_repo_updates: Some(false), install_patterns: Some(default_install_patterns()), completions: None, @@ -530,6 +537,7 @@ impl Config { } self.ghcr_concurrency.get_or_insert(8); + self.download_concurrency.get_or_insert(8); self.search_limit.get_or_insert(20); self.cross_repo_updates.get_or_insert(false); @@ -539,6 +547,9 @@ impl Config { if self.ghcr_concurrency == Some(0) { return Err(ConfigError::ZeroLimit("ghcr_concurrency")); } + if self.download_concurrency == Some(0) { + return Err(ConfigError::ZeroLimit("download_concurrency")); + } if self.search_limit == Some(0) { return Err(ConfigError::ZeroLimit("search_limit")); } @@ -839,6 +850,7 @@ mod tests { assert_eq!(config.parallel_limit, Some(4)); assert_eq!(config.search_limit, Some(20)); assert_eq!(config.ghcr_concurrency, Some(8)); + assert_eq!(config.download_concurrency, Some(8)); assert_eq!(config.cross_repo_updates, Some(false)); } @@ -955,11 +967,17 @@ mod tests { #[test] fn test_config_resolve_rejects_zero_limits() { - for field in ["parallel_limit", "ghcr_concurrency", "search_limit"] { + for field in [ + "parallel_limit", + "ghcr_concurrency", + "download_concurrency", + "search_limit", + ] { let mut config = Config::default_config::<&str>(&[]); match field { "parallel_limit" => config.parallel_limit = Some(0), "ghcr_concurrency" => config.ghcr_concurrency = Some(0), + "download_concurrency" => config.download_concurrency = Some(0), _ => config.search_limit = Some(0), } assert!( @@ -1049,6 +1067,7 @@ mod tests { fn test_config_resolve_sets_defaults() { let mut config = Config::default_config::<&str>(&[]); config.ghcr_concurrency = None; + config.download_concurrency = None; config.search_limit = None; config.cross_repo_updates = None; config.install_patterns = None; @@ -1056,6 +1075,7 @@ mod tests { config.resolve().unwrap(); assert_eq!(config.ghcr_concurrency, Some(8)); + assert_eq!(config.download_concurrency, Some(8)); assert_eq!(config.search_limit, Some(20)); assert_eq!(config.cross_repo_updates, Some(false)); assert!(config.install_patterns.is_some()); diff --git a/crates/soar-core/src/package/install.rs b/crates/soar-core/src/package/install.rs index 14559055..26342bf6 100644 --- a/crates/soar-core/src/package/install.rs +++ b/crates/soar-core/src/package/install.rs @@ -52,7 +52,11 @@ use crate::{ /// the artifact. A licence publishes none, because it is served from a branch /// and is documentation rather than something that runs: pinning it would turn /// an upstream copyright-year edit into a failed download. -async fn install_extras(package: &Package, install_dir: &Path) -> SoarResult<()> { +async fn install_extras( + package: &Package, + install_dir: &Path, + connections: usize, +) -> SoarResult<()> { let Some(extras) = &package.extra else { return Ok(()); }; @@ -85,7 +89,8 @@ async fn install_extras(package: &Package, install_dir: &Path) -> SoarResult<()> } let mut dl = Download::new(&e.url) .output(dest.to_string_lossy()) - .overwrite(OverwriteMode::Skip); + .overwrite(OverwriteMode::Skip) + .connections(connections); if let Some(sum) = e.blake3.as_ref() { dl = dl.checksum(sum); } @@ -1205,7 +1210,8 @@ impl PackageInstaller { .output(output_path.to_string_lossy()) .overwrite(OverwriteMode::Skip) .extract(should_extract) - .extract_to(&extract_dir); + .extract_to(&extract_dir) + .connections(self.config.download_concurrency.unwrap_or(8)); if let Some(ref bsum) = self.package.bsum { dl = dl.checksum(bsum); @@ -1353,7 +1359,12 @@ impl PackageInstaller { mark_elfs_executable(&self.install_dir); } - install_extras(&self.package, &self.install_dir).await?; + install_extras( + &self.package, + &self.install_dir, + self.config.download_concurrency.unwrap_or(8), + ) + .await?; // Handle nested_extract: extract an archive within the package if let Some(ref nested_archive) = self.nested_extract { @@ -1619,7 +1630,7 @@ mod tests { /// Runs `install_extras`, which never waits on anything, to completion. fn install_side_files(package: &Package, dir: &Path) { - let future = std::pin::pin!(install_extras(package, dir)); + let future = std::pin::pin!(install_extras(package, dir, 8)); let mut cx = std::task::Context::from_waker(std::task::Waker::noop()); match std::future::Future::poll(future, &mut cx) { std::task::Poll::Ready(result) => result.unwrap(), diff --git a/crates/soar-dl/src/download.rs b/crates/soar-dl/src/download.rs index 5ea00283..bb897386 100644 --- a/crates/soar-dl/src/download.rs +++ b/crates/soar-dl/src/download.rs @@ -18,12 +18,22 @@ use ureq::{ use crate::{ error::DownloadError, - http::Http, - types::{OverwriteMode, Progress, ResumeInfo}, - utils::{filename_from_header, filename_from_url, open_output, resolve_output_path}, + http::{Http, RemoteInfo}, + segmented, + types::{OverwriteMode, Progress, ResumeInfo, SegmentProgress}, + utils::{ + filename_from_header, filename_from_url, open_existing, open_output, resolve_output_path, + }, xattr::{read_usable_resume, remove_resume, write_resume}, }; +/// Connections one download uses by default. Where one stream is throttled +/// below the path's capacity, several in parallel add up. +const DEFAULT_CONNECTIONS: usize = 8; + +/// Bytes a single-stream read loop takes from the network per read. +const READ_BUFFER: usize = 256 * 1024; + #[derive(Clone)] pub struct Download { pub url: String, @@ -34,6 +44,10 @@ pub struct Download { pub on_progress: Option>, pub ghcr_blob: bool, pub expected_checksum: Option, + /// Connections this download may open at once. One streams without + /// ranges; more than one splits a range-capable server's file between + /// them. + pub connections: usize, } impl Download { @@ -65,9 +79,28 @@ impl Download { on_progress: None, ghcr_blob: false, expected_checksum: None, + connections: DEFAULT_CONNECTIONS, } } + /// Sets how many connections this download may open at once. + /// + /// More than one lets a range-capable server's file be fetched in + /// parallel segments; one streams a single response body. Zero is treated + /// as one. + /// + /// # Examples + /// + /// ``` + /// use soar_dl::download::Download; + /// + /// let dl = Download::new("https://example.com/archive.tar.gz").connections(16); + /// ``` + pub fn connections(mut self, connections: usize) -> Self { + self.connections = connections.max(1); + self + } + /// Sets the expected blake3 checksum (hex) to verify the downloaded file /// against before it is made executable or extracted. /// @@ -254,10 +287,13 @@ impl Download { }; let mut head_size = None; + let mut remote: Option = None; let (header_filename, url_filename) = if needs_head { trace!("performing HEAD request for filename"); let resp = Http::head(&self.url)?; - head_size = content_length(&resp); + let info = RemoteInfo::from_head(&resp); + head_size = info.size; + remote = Some(info); ( resp.headers() .get(CONTENT_DISPOSITION) @@ -316,7 +352,7 @@ impl Download { std::fs::create_dir_all(parent)?; } - let file = self.download_to_file(&output_path, resume_info)?; + let file = self.download_to_file(&output_path, resume_info, remote)?; if let Err(e) = self.verify_checksum(&output_path) { fs::remove_file(&output_path).ok(); @@ -443,6 +479,187 @@ impl Download { &self, path: &Path, resume_info: Option, + remote: Option, + ) -> Result { + // A segmented download continues segment by segment; its file is + // pre-allocated, so appending to it as a single stream would splice + // the ranges out of place. + if let Some(info) = &resume_info + && let Some(segments) = &info.segments + { + // A size the server just reported that disagrees with the state + // means the resource changed since it was written; the offsets + // no longer describe it and resuming would splice two versions. + let size_changed = remote + .as_ref() + .and_then(|remote| remote.size) + .is_some_and(|size| size != info.total); + return match segmented::plan_from_resume(info.total, segments) { + Some(resumed) if !size_changed => { + debug!(offset = resumed.current, "resuming segmented download"); + self.download_segmented( + path, + info.total, + resumed.ranges, + resumed.written, + info.etag.as_deref(), + ) + } + _ => { + warn!( + "segmented resume state does not describe this resource, restarting download" + ); + self.download_to_file(path, None, remote) + } + }; + } + + if self.connections > 1 { + // The HEAD only picks a strategy, so a server that refuses it + // still gets the download, in one stream. A HEAD that is silent + // about ranges gets a one-byte range probe, since many servers + // honor Range on GET without ever saying so on HEAD. + let mut remote = match remote { + Some(remote) => remote, + None => { + match Http::head(&self.url) { + Ok(resp) => RemoteInfo::from_head(&resp), + Err(err) => { + debug!(%err, "size probe by HEAD failed, trying a range probe"); + RemoteInfo::default() + } + } + } + }; + if (remote.size.is_none() || !remote.ranges_ok) + && let Some(probed) = Http::probe_range(&self.url, self.ghcr_blob) + { + remote = probed; + } + if let Some(total) = remote.size + && remote.ranges_ok + && total >= segmented::MIN_SEGMENTED_TOTAL + { + let ranges = segmented::plan(total, self.connections); + let written = vec![0; ranges.len()]; + return self.download_segmented( + path, + total, + ranges, + written, + remote.etag.as_deref(), + ); + } + } + + self.download_single(path, resume_info) + } + + /// Fetches a range-capable server's file in parallel segments. + /// + /// The output is allocated to its full size up front and each segment is + /// written at its own offset. The resume state is written before the first + /// byte, since a half-filled allocation is indistinguishable by size from + /// a finished download; a filesystem that cannot hold that state falls + /// back to a single stream. + fn download_segmented( + &self, + path: &Path, + total: u64, + ranges: Vec<(u64, u64)>, + written: Vec, + etag: Option<&str>, + ) -> Result { + let current: u64 = written.iter().sum(); + let file = if current == 0 { + trace!(path = %path.display(), total = total, "allocating segmented download"); + let file = open_output(path, None)?; + file.set_len(total)?; + file + } else { + let file = open_existing(path)?; + if file.metadata()?.len() != total { + warn!( + path = %path.display(), + "file size does not match the resource, restarting download" + ); + let ranges = segmented::plan(total, self.connections); + let written = vec![0; ranges.len()]; + return self.download_segmented(path, total, ranges, written, etag); + } + file + }; + + let info = ResumeInfo { + downloaded: current, + total, + etag: etag.map(String::from), + last_modified: None, + segments: Some( + ranges + .iter() + .zip(&written) + .map(|(&(start, end), &written)| { + SegmentProgress { + start, + end, + written, + } + }) + .collect(), + ), + }; + if let Err(err) = write_resume(path, &info) { + warn!(%err, "filesystem cannot store resume state, falling back to single stream"); + return self.download_single(path, None); + } + + let result = segmented::run(segmented::Job { + url: &self.url, + ghcr_blob: self.ghcr_blob, + etag, + path, + file: &file, + total, + connections: self.connections, + ranges, + written, + on_progress: self.on_progress.as_ref(), + }); + + match result { + Ok(()) => { + if let Some(ref cb) = self.on_progress { + cb(Progress::Chunk { + current: total, + total, + }); + cb(Progress::Complete { + total, + }); + } + Ok(file) + } + // The server turned out not to honor ranges after all; a single + // stream needs none. + Err( + err @ DownloadError::RangeUnsupported { + .. + }, + ) => { + warn!(%err, "falling back to single stream"); + self.download_single(path, None) + } + Err(err) => Err(err), + } + } + + /// Streams one HTTP response body into the file at `path`, resuming from + /// `resume_info` when it describes the same resource. + fn download_single( + &self, + path: &Path, + resume_info: Option, ) -> Result { let (resume_from, etag) = resume_info .as_ref() @@ -458,7 +675,7 @@ impl Download { status: 416, .. }) if resume_from.is_some() => { warn!("server rejected the resume range, restarting download"); - return self.download_to_file(path, None); + return self.download_single(path, None); } resp => resp?, }; @@ -472,11 +689,11 @@ impl Download { "server doesn't support resume (status {}), restarting download", status ); - return self.download_to_file(path, None); + return self.download_single(path, None); } if let Err(err) = check_partial_response(&resp, info) { warn!(%err, "partial response does not continue the file, restarting download"); - return self.download_to_file(path, None); + return self.download_single(path, None); } } @@ -491,7 +708,7 @@ impl Download { let mut file = match open_output(path, resume_from) { Err(DownloadError::ResumeMismatch) => { warn!(path = %path.display(), "file shrank below its resume offset, restarting download"); - return self.download_to_file(path, None); + return self.download_single(path, None); } file => file?, }; @@ -513,7 +730,7 @@ impl Download { } let mut reader = resp.into_body().into_reader(); - let mut buffer = [0u8; 8192]; + let mut buffer = vec![0u8; READ_BUFFER]; let mut downloaded = resume_from.unwrap_or(0); let mut last_checkpoint = downloaded / (1024 * 1024); @@ -538,6 +755,7 @@ impl Download { total, etag: new_etag.clone(), last_modified: None, + segments: None, }, ) { trace!(%err, "failed to save resume checkpoint"); @@ -698,6 +916,7 @@ mod tests { total: body().len() as u64, etag: None, last_modified: None, + segments: None, }; if write_resume(&out, &stale).is_err() { eprintln!("skipping: filesystem lacks user xattrs"); @@ -723,6 +942,7 @@ mod tests { total: body().len() as u64, etag: None, last_modified: None, + segments: None, }; if write_resume(&out, &info).is_err() { eprintln!("skipping: filesystem lacks user xattrs"); diff --git a/crates/soar-dl/src/error.rs b/crates/soar-dl/src/error.rs index 4d36452a..38633273 100644 --- a/crates/soar-dl/src/error.rs +++ b/crates/soar-dl/src/error.rs @@ -86,6 +86,10 @@ pub enum DownloadError { #[diagnostic(code(soar_dl::resume_mismatch))] ResumeMismatch, + #[error("Server does not support segmented downloads: {url}")] + #[diagnostic(code(soar_dl::range_unsupported))] + RangeUnsupported { url: String }, + /// Several downloads failed. `transient` holds when every one of them /// failed in a way a retry may get past. #[error("{}", .errors.join("; "))] @@ -169,6 +173,11 @@ impl DownloadError { Self::Multiple { transient, .. } => *transient, + // Retrying cannot help: the server ignored the range, so the + // caller falls back to a single-stream download instead. + Self::RangeUnsupported { + .. + } => false, _ => false, } } diff --git a/crates/soar-dl/src/http.rs b/crates/soar-dl/src/http.rs index 67846dc6..18ccbc5e 100644 --- a/crates/soar-dl/src/http.rs +++ b/crates/soar-dl/src/http.rs @@ -3,7 +3,7 @@ use ureq::{ Body, http::{ Response, - header::{CONTENT_LENGTH, CONTENT_TYPE, LOCATION}, + header::{ACCEPT_RANGES, CONTENT_LENGTH, CONTENT_RANGE, CONTENT_TYPE, ETAG, LOCATION}, }, }; @@ -11,6 +11,40 @@ use crate::{error::DownloadError, http_client::SHARED_AGENT}; pub struct Http; +/// What a HEAD response says about the resource a segmented download would +/// target: its size, whether it can be fetched in byte ranges, and the ETag a +/// later ranged GET can pin itself to. +#[derive(Debug, Clone, Default)] +pub(crate) struct RemoteInfo { + pub size: Option, + pub ranges_ok: bool, + pub etag: Option, +} + +impl RemoteInfo { + pub(crate) fn from_head(resp: &Response) -> Self { + let headers = resp.headers(); + Self { + size: content_length_header(headers), + ranges_ok: headers + .get(ACCEPT_RANGES) + .and_then(|h| h.to_str().ok()) + .is_some_and(|v| v.split(',').any(|u| u.trim().eq_ignore_ascii_case("bytes"))), + etag: headers + .get(ETAG) + .and_then(|h| h.to_str().ok()) + .map(String::from), + } + } +} + +fn content_length_header(headers: &ureq::http::HeaderMap) -> Option { + headers + .get(CONTENT_LENGTH) + .and_then(|h| h.to_str().ok()) + .and_then(|len| len.parse().ok()) +} + impl Http { pub fn head(url: &str) -> Result, DownloadError> { trace!(url = url, "sending HEAD request"); @@ -106,6 +140,100 @@ impl Http { result } + /// Fetches a closed byte range of the resource at `url`, optionally pinning + /// the request to the resource version an ETag names. + /// + /// The request carries `Range: bytes={start}-{end}` with both ends + /// inclusive. A server that answers `200` ignored the range; callers + /// surface that as [`DownloadError::RangeUnsupported`] so the download can + /// fall back to a single stream. + pub(crate) fn fetch_range( + url: &str, + start: u64, + end: u64, + etag: Option<&str>, + ghcr_blob: bool, + ) -> Result, DownloadError> { + debug!("GET {url} (Range: bytes={start}-{end})"); + trace!(etag = ?etag, ghcr_blob = ghcr_blob, "range request details"); + let mut req = SHARED_AGENT.get(url); + + if ghcr_blob { + trace!("adding GHCR authorization header"); + req = req.header("Authorization", "Bearer QQ=="); + } + + req = req.header("Range", &format!("bytes={start}-{end}")); + if let Some(tag) = etag { + trace!(etag = tag, "adding If-Range header"); + req = req.header("If-Range", tag); + } + + let result = req.call().map_err(|err| DownloadError::request(url, err)); + if let Ok(ref resp) = result { + Self::log_response_headers(resp, "GET"); + } + result + } + + /// Asks the server directly whether it serves byte ranges, with a + /// one-byte range GET. + /// + /// Some servers refuse HEAD outright and others answer it without + /// `Accept-Ranges` while still honoring `Range` on GET, so the probe is + /// what a segmented download falls back to when HEAD says nothing. The + /// `206`'s `Content-Range` names the resource's full size and its ETag + /// pins later range requests to this version. `None` when the server + /// gives no usable answer; the caller then downloads in one stream. + pub(crate) fn probe_range(url: &str, ghcr_blob: bool) -> Option { + debug!("probing range support: GET {url} (Range: bytes=0-0)"); + let mut req = SHARED_AGENT.get(url); + if ghcr_blob { + req = req.header("Authorization", "Bearer QQ=="); + } + req = req.header("Range", "bytes=0-0"); + + let resp = match req.call() { + Ok(resp) => resp, + Err(err) => { + debug!(%err, "range probe request failed"); + return None; + } + }; + if resp.status() != 206 { + debug!( + status = resp.status().as_u16(), + "range probe was not answered with a partial response" + ); + return None; + } + let total = resp + .headers() + .get(CONTENT_RANGE) + .and_then(|h| h.to_str().ok()) + .and_then(|range| range.rsplit_once('/')) + .and_then(|(_, total)| total.trim().parse::().ok()); + match total { + Some(total) => { + let etag = resp + .headers() + .get(ETAG) + .and_then(|h| h.to_str().ok()) + .map(String::from); + debug!(total, "range probe confirms byte ranges"); + Some(RemoteInfo { + size: Some(total), + ranges_ok: true, + etag, + }) + } + None => { + debug!("range probe's Content-Range names no total size"); + None + } + } + } + /// Fetches JSON from the given URL and deserializes it into `T`. /// /// Performs an HTTP GET request to `url` and deserializes the response body into the requested type. diff --git a/crates/soar-dl/src/lib.rs b/crates/soar-dl/src/lib.rs index 10583319..3897b4d4 100644 --- a/crates/soar-dl/src/lib.rs +++ b/crates/soar-dl/src/lib.rs @@ -9,6 +9,7 @@ pub mod http_client; pub mod oci; pub mod platform; pub mod release; +mod segmented; #[cfg(test)] mod test_server; pub mod types; diff --git a/crates/soar-dl/src/oci.rs b/crates/soar-dl/src/oci.rs index 9e97dc15..cafcd6b9 100644 --- a/crates/soar-dl/src/oci.rs +++ b/crates/soar-dl/src/oci.rs @@ -1041,6 +1041,7 @@ fn download_layer_impl( total: layer.size, etag: new_etag.clone(), last_modified: None, + segments: None, }, ) { trace!(%err, "failed to save resume checkpoint"); @@ -1459,6 +1460,7 @@ mod tests { total: bytes.len() as u64, etag: None, last_modified: None, + segments: None, }; if write_resume(&path, &info).is_err() { eprintln!("skipping: filesystem lacks user xattrs"); @@ -1557,7 +1559,9 @@ mod tests { match err { DownloadError::InvalidJson { reason, .. - } => assert!(reason.contains("EOF"), "{reason}"), + } => { + assert!(reason.contains("EOF"), "{reason}") + } other => panic!("expected InvalidJson, got {other:?}"), } } diff --git a/crates/soar-dl/src/segmented.rs b/crates/soar-dl/src/segmented.rs new file mode 100644 index 00000000..40cfb8d8 --- /dev/null +++ b/crates/soar-dl/src/segmented.rs @@ -0,0 +1,1058 @@ +//! Segmented, multi-connection downloads. +//! +//! A file the server serves in byte ranges is cut into chunks smaller than a +//! connection's share and fetched in parallel, each written at its offset in +//! the output file. Where one connection is throttled well below the path's +//! capacity, several in parallel add up; where connections run at uneven +//! speeds, the queue keeps every worker fed so a fast one picks up a slow +//! one's share instead of the whole download waiting on it. + +use std::{ + fs::File, + io::Read as _, + os::unix::fs::FileExt as _, + path::Path, + sync::{ + Arc, Mutex, + atomic::{AtomicBool, AtomicU64, AtomicUsize, Ordering}, + }, + thread, +}; + +use tracing::{debug, trace}; + +use crate::{ + error::DownloadError, + http::Http, + types::{Progress, ResumeInfo, SegmentProgress}, + xattr::write_resume, +}; + +/// The smallest chunk a download is cut into. Below this, another connection +/// adds handshake overhead without adding throughput. +pub(crate) const MIN_SEGMENT_SIZE: u64 = 4 * 1024 * 1024; + +/// A file smaller than this is never segmented, whatever the connection count +/// allows. +pub(crate) const MIN_SEGMENTED_TOTAL: u64 = 2 * MIN_SEGMENT_SIZE; + +/// Chunks queued per connection. More chunks than workers keeps a worker +/// whose chunk ran out from going idle while others still have work. +const CHUNKS_PER_CONNECTION: usize = 4; + +/// Upper bound on chunks per download, keeping the resume state small. +const MAX_CHUNKS: usize = 128; + +/// Bytes one worker reads from the network before writing them out. +const CHUNK_BUFFER: usize = 256 * 1024; + +/// Aggregate bytes between two resume checkpoints and progress reports. +const REPORT_INTERVAL: u64 = 1024 * 1024; + +/// Times one chunk may fail on the network before the download gives up and +/// leaves its resume state for the next run. +const MAX_ATTEMPTS: u32 = 3; + +/// Cuts `total` bytes into contiguous, inclusive chunks: a queue of +/// [`CHUNKS_PER_CONNECTION`] per connection, capped by the per-chunk minimum +/// and [`MAX_CHUNKS`]. The last chunk absorbs the remainder. +pub(crate) fn plan(total: u64, connections: usize) -> Vec<(u64, u64)> { + let max_chunks = (total / MIN_SEGMENT_SIZE).max(1) as usize; + let parts = connections + .max(1) + .saturating_mul(CHUNKS_PER_CONNECTION) + .min(max_chunks) + .min(MAX_CHUNKS); + let chunk = total / parts as u64; + let mut ranges = Vec::with_capacity(parts); + let mut start = 0; + for i in 0..parts { + let len = if i == parts - 1 { total - start } else { chunk }; + ranges.push((start, start + len - 1)); + start += len; + } + ranges +} + +/// The chunk plan and progress a usable resume xattr carries. +pub(crate) struct ResumedSegments { + /// Inclusive byte range of every chunk. + pub ranges: Vec<(u64, u64)>, + /// Bytes of each chunk already on disk. + pub written: Vec, + /// Bytes on disk across all chunks. + pub current: u64, +} + +/// Validates the chunk state a resume xattr carries against the resource it +/// describes. `None` when the state cannot describe this resource. +pub(crate) fn plan_from_resume( + total: u64, + segments: &[SegmentProgress], +) -> Option { + if segments.is_empty() { + return None; + } + let mut resumed = ResumedSegments { + ranges: Vec::with_capacity(segments.len()), + written: Vec::with_capacity(segments.len()), + current: 0, + }; + // The chunks must tile the resource exactly: sorted, contiguous from + // zero, through the last byte. Anything else would leave holes that no + // request ever fills while the download still reports success. + let mut expected_start = 0; + for seg in segments { + if seg.start != expected_start || seg.end < seg.start || seg.end >= total { + return None; + } + if seg.written > seg.end - seg.start + 1 { + return None; + } + resumed.ranges.push((seg.start, seg.end)); + resumed.written.push(seg.written); + resumed.current += seg.written; + expected_start = seg.end + 1; + } + if expected_start != total { + return None; + } + Some(resumed) +} + +/// One segmented transfer over an open, pre-allocated output file. +pub(crate) struct Job<'a> { + pub url: &'a str, + pub ghcr_blob: bool, + pub etag: Option<&'a str>, + pub path: &'a Path, + pub file: &'a File, + pub total: u64, + /// How many workers to run; each pulls chunks from the shared queue. + pub connections: usize, + /// Inclusive byte range per chunk. + pub ranges: Vec<(u64, u64)>, + /// Bytes of each chunk already on disk, from a resume state. + pub written: Vec, + pub on_progress: Option<&'a Arc>, +} + +enum Outcome { + Done, + Cancelled, + Failed(DownloadError), +} + +struct Shared { + cancel: AtomicBool, + /// Bytes written across all chunks. + aggregate: AtomicU64, + /// The aggregate the last checkpoint and progress report carried. + reported: AtomicU64, + /// Bytes written per chunk, counted from the chunk's start. + chunk_written: Box<[AtomicU64]>, + /// The full chunk plan, for resume checkpoints. + ranges: Arc>, + /// The next chunk index a free worker may claim. + cursor: AtomicUsize, + /// Chunks an interrupted run left half-written. A worker finishes one of + /// these before reaching for the queue, so the bytes already on disk for + /// it are kept rather than refetched. + partial: Mutex>, + /// One slot per chunk: a worker claimed it and no other may. + claimed: Mutex>, +} + +impl Shared { + fn chunk(&self, index: usize) -> (u64, u64, u64) { + let (start, end) = self.ranges[index]; + ( + start, + end, + self.chunk_written[index].load(Ordering::Relaxed), + ) + } + + /// Marks a chunk as this worker's, unless another worker already has it. + fn claim(&self, index: usize) -> bool { + let mut claimed = self.claimed.lock().unwrap(); + if claimed[index] { + return false; + } + claimed[index] = true; + true + } +} + +/// Runs the chunk workers to completion. +/// +/// Workers share the output file, each writing only inside chunks it claimed +/// from the queue. One worker's failure cancels the rest; a server that +/// ignores a range request surfaces as [`DownloadError::RangeUnsupported`] so +/// the caller can fall back to a single stream. +pub(crate) fn run(job: Job) -> Result<(), DownloadError> { + let initial: u64 = job.written.iter().sum(); + let ranges = Arc::new(job.ranges.clone()); + let partial = (0..job.ranges.len()) + .filter(|&i| { + let (start, end) = job.ranges[i]; + let written = job.written[i]; + written > 0 && written < end - start + 1 + }) + .collect(); + let shared = Arc::new(Shared { + cancel: AtomicBool::new(false), + aggregate: AtomicU64::new(initial), + reported: AtomicU64::new(initial), + chunk_written: job + .written + .iter() + .map(|w| AtomicU64::new(*w)) + .collect::>() + .into_boxed_slice(), + ranges: ranges.clone(), + cursor: AtomicUsize::new(0), + partial: Mutex::new(partial), + claimed: Mutex::new(vec![false; job.ranges.len()]), + }); + + if let Some(cb) = job.on_progress { + if initial > 0 { + cb(Progress::Resuming { + current: initial, + total: job.total, + }); + } else { + cb(Progress::Starting { + total: job.total, + }); + } + } + + debug!( + workers = job.connections.min(job.ranges.len()).max(1), + chunks = job.ranges.len(), + total = job.total, + "segmented download starting" + ); + + let workers = job.connections.min(job.ranges.len()).max(1); + let mut outcomes = Vec::with_capacity(workers); + + thread::scope(|scope| { + let mut handles = Vec::with_capacity(workers); + for _ in 0..workers { + let worker = Worker { + shared: shared.clone(), + url: job.url, + ghcr_blob: job.ghcr_blob, + etag: job.etag, + path: job.path, + total: job.total, + file: job.file.try_clone().expect("cloning the output file"), + progress: job.on_progress.cloned(), + }; + handles.push(scope.spawn(move || worker.run())); + } + + for handle in handles { + match handle.join() { + Ok(outcome) => outcomes.push(outcome), + Err(_) => { + outcomes.push(Outcome::Failed(DownloadError::Io(std::io::Error::other( + "chunk worker panicked", + )))) + } + } + } + }); + + // A server that ignored one range request is treated as ignoring them all; + // the caller falls back to a single stream, which needs no ranges. + if let Some(Outcome::Failed(err)) = outcomes + .iter() + .find(|o| matches!(o, Outcome::Failed(DownloadError::RangeUnsupported { .. }))) + { + let url = match err { + DownloadError::RangeUnsupported { + url, + } => url.clone(), + _ => unreachable!(), + }; + return Err(DownloadError::RangeUnsupported { + url, + }); + } + + for outcome in outcomes { + match outcome { + Outcome::Done | Outcome::Cancelled => {} + Outcome::Failed(err) => return Err(err), + } + } + + if let Some(cb) = job.on_progress { + cb(Progress::Chunk { + current: shared.aggregate.load(Ordering::Relaxed), + total: job.total, + }); + } + + Ok(()) +} + +struct Worker<'a> { + shared: Arc, + url: &'a str, + ghcr_blob: bool, + etag: Option<&'a str>, + path: &'a Path, + total: u64, + file: File, + progress: Option>, +} + +impl Worker<'_> { + fn run(self) -> Outcome { + let mut worker = self; + loop { + if worker.shared.cancel.load(Ordering::Relaxed) { + return Outcome::Cancelled; + } + match worker.next_chunk() { + None => return Outcome::Done, + Some((index, start, end, written)) => { + if let Err(err) = worker.download_chunk(index, start, end, written) { + // Cancellation is fallout from another worker's + // failure; that worker's error is the one worth + // reporting. + if worker.shared.cancel.load(Ordering::Relaxed) { + return Outcome::Cancelled; + } + worker.shared.cancel.store(true, Ordering::Relaxed); + return Outcome::Failed(err); + } + } + } + } + } + + /// Claims this worker's next chunk: half-written ones first, then the + /// queue, skipping chunks a previous run already completed or another + /// worker owns. `None` when no work is left. + fn next_chunk(&self) -> Option<(usize, u64, u64, u64)> { + let mut partial = self.shared.partial.lock().unwrap(); + while let Some(index) = partial.pop() { + let (start, end, written) = self.shared.chunk(index); + if written < end - start + 1 && self.shared.claim(index) { + return Some((index, start, end, written)); + } + } + drop(partial); + loop { + let index = self.shared.cursor.fetch_add(1, Ordering::Relaxed); + if index >= self.shared.ranges.len() { + return None; + } + let (start, end, written) = self.shared.chunk(index); + if written == end - start + 1 { + continue; + } + if self.shared.claim(index) { + return Some((index, start, end, written)); + } + } + } + + fn download_chunk( + &mut self, + index: usize, + start: u64, + end: u64, + written: u64, + ) -> Result<(), DownloadError> { + let mut attempts = 0; + let mut cursor = start + written; + loop { + if cursor > end { + return Ok(()); + } + attempts += 1; + match self.fetch_and_write(index, start, end, &mut cursor) { + Ok(()) => continue, + Err(err) => { + if self.shared.cancel.load(Ordering::Relaxed) { + return Err(err); + } + if err.is_transient() && attempts < MAX_ATTEMPTS { + debug!(chunk = index, attempt = attempts, %err, "chunk failed, retrying"); + continue; + } + return Err(err); + } + } + } + } + + fn fetch_and_write( + &mut self, + index: usize, + start: u64, + end: u64, + cursor: &mut u64, + ) -> Result<(), DownloadError> { + let resp = Http::fetch_range(self.url, *cursor, end, self.etag, self.ghcr_blob)?; + + if resp.status() != 206 { + let status = resp.status().as_u16(); + if status == 200 { + trace!(url = self.url, "server ignored the range request"); + return Err(DownloadError::RangeUnsupported { + url: self.url.to_string(), + }); + } + return Err(DownloadError::HttpError { + status, + url: self.url.to_string(), + }); + } + + // A range that does not start where it was asked to start, or that + // describes a different resource than the one being downloaded, + // means the server's idea of the file has diverged from ours; refuse + // it entirely rather than splice bytes out of place. + let content_range = resp + .headers() + .get(ureq::http::header::CONTENT_RANGE) + .and_then(|h| h.to_str().ok()) + .and_then(|range| range.trim().strip_prefix("bytes ")) + .and_then(|range| range.split_once('/')); + let Some((span, total)) = content_range else { + return Err(DownloadError::RangeUnsupported { + url: self.url.to_string(), + }); + }; + let matches = span + .split_once('-') + .and_then(|(start, _)| start.trim().parse::().ok()) + .is_some_and(|start| start == *cursor) + && total.trim().parse::() == Ok(self.total); + if !matches { + return Err(DownloadError::RangeUnsupported { + url: self.url.to_string(), + }); + } + + let mut reader = resp.into_body().into_reader(); + let mut buffer = vec![0u8; CHUNK_BUFFER]; + loop { + if *cursor > end { + return Ok(()); + } + // Never read past the chunk's last byte, whatever the server + // sends; a body that overshoots its Content-Range must not leak + // into the neighboring chunks. + let want = (end + 1 - *cursor).min(buffer.len() as u64) as usize; + let n = reader + .read(&mut buffer[..want]) + .map_err(DownloadError::Io)?; + if n == 0 { + // The connection ended before this chunk's last byte. + return Err(DownloadError::Io(std::io::Error::new( + std::io::ErrorKind::UnexpectedEof, + "chunk closed before its range was complete", + ))); + } + if self.shared.cancel.load(Ordering::Relaxed) { + return Err(DownloadError::Io(std::io::Error::new( + std::io::ErrorKind::Interrupted, + "cancelled", + ))); + } + self.file.write_all_at(&buffer[..n], *cursor)?; + *cursor += n as u64; + self.shared.chunk_written[index].store(*cursor - start, Ordering::Relaxed); + let aggregate = self.shared.aggregate.fetch_add(n as u64, Ordering::Relaxed) + n as u64; + self.maybe_report(aggregate); + } + } + + /// Checkpoints resume state and reports progress once per + /// [`REPORT_INTERVAL`] aggregate bytes; one worker wins that right per + /// interval, so the xattr and the bar advance in step. + fn maybe_report(&self, aggregate: u64) { + let reported = self.shared.reported.load(Ordering::Relaxed); + if aggregate.saturating_sub(reported) < REPORT_INTERVAL { + return; + } + if self + .shared + .reported + .compare_exchange(reported, aggregate, Ordering::Relaxed, Ordering::Relaxed) + .is_err() + { + return; + } + + if let Some(cb) = &self.progress { + cb(Progress::Chunk { + current: aggregate, + total: self.total, + }); + } + + let segments = self + .shared + .ranges + .iter() + .enumerate() + .map(|(index, &(start, end))| { + SegmentProgress { + start, + end, + written: self.shared.chunk_written[index].load(Ordering::Relaxed), + } + }) + .collect(); + let info = ResumeInfo { + downloaded: aggregate, + total: self.total, + etag: self.etag.map(String::from), + last_modified: None, + segments: Some(segments), + }; + if let Err(err) = write_resume(self.path, &info) { + trace!(%err, "failed to save resume checkpoint"); + } + } +} + +#[cfg(test)] +mod tests { + use std::sync::Mutex; + + use super::*; + use crate::{ + download::Download, + test_server::{self, ServeOptions}, + types::OverwriteMode, + }; + + fn body(mib: usize) -> Vec { + ((0..mib * 1024 * 1024).map(|i| (i % 251) as u8)).collect() + } + + fn ranges_are_contiguous(ranges: &[(u64, u64)], total: u64) -> bool { + let mut next = 0; + for &(start, end) in ranges { + if start != next || end < start { + return false; + } + next = end + 1; + } + next == total + } + + #[test] + fn plan_queues_chunks_per_connection() { + assert_eq!(plan(1024 * 1024 * 1024, 8).len(), 32); + assert_eq!(plan(4 * 1024 * 1024 * 1024, 32).len(), 128); + assert_eq!(plan(2 * 1024 * 1024 * 1024, 1).len(), 4); + } + + #[test] + fn plan_splits_evenly_and_covers_the_file() { + let total = 100 * 1024 * 1024; + let ranges = plan(total, 8); + assert_eq!(ranges.len(), 25); + assert!(ranges_are_contiguous(&ranges, total)); + } + + #[test] + fn plan_respects_the_segment_minimum() { + let total = 8 * 1024 * 1024; + let ranges = plan(total, 8); + assert_eq!(ranges.len(), 2); + assert!(ranges_are_contiguous(&ranges, total)); + } + + #[test] + fn plan_of_a_tiny_file_is_one_segment() { + let ranges = plan(1024, 8); + assert_eq!(ranges, vec![(0, 1023)]); + } + + #[test] + fn resume_state_that_cannot_describe_the_resource_is_refused() { + let seg = |start, end, written| { + SegmentProgress { + start, + end, + written, + } + }; + let total = 100; + + assert!(plan_from_resume(total, &[]).is_none()); + assert!(plan_from_resume(total, &[seg(0, 100, 50)]).is_none()); + assert!(plan_from_resume(total, &[seg(0, 99, 101)]).is_none()); + assert!(plan_from_resume(total, &[seg(60, 10, 5)]).is_none()); + + let resumed = plan_from_resume(total, &[seg(0, 49, 10), seg(50, 99, 50)]).unwrap(); + assert_eq!(resumed.ranges, vec![(0, 49), (50, 99)]); + assert_eq!(resumed.written, vec![10, 50]); + assert_eq!(resumed.current, 60); + } + + #[test] + fn a_large_download_is_fetched_in_segments() { + let content = body(16); + let base = test_server::serve(vec![("/file".into(), content.clone())]); + let dir = tempfile::tempdir().unwrap(); + let out = dir.path().join("file"); + + Download::new(format!("{base}/file")) + .output(out.to_string_lossy()) + .overwrite(OverwriteMode::Force) + .execute() + .unwrap(); + + assert_eq!(std::fs::read(&out).unwrap(), content); + assert!(read_resume_segments(&out).is_none(), "resume state kept"); + } + + #[test] + fn a_multi_chunk_download_reassembles() { + let content = body(96); + let (base, requests) = test_server::serve_with( + vec![("/file".into(), content.clone())], + ServeOptions::default(), + ); + let dir = tempfile::tempdir().unwrap(); + let out = dir.path().join("file"); + + Download::new(format!("{base}/file")) + .output(out.to_string_lossy()) + .overwrite(OverwriteMode::Force) + .connections(8) + .execute() + .unwrap(); + + assert_eq!(std::fs::read(&out).unwrap(), content); + let requests = requests.lock().unwrap(); + let gets = requests.iter().filter(|r| r.starts_with("GET")).count(); + assert_eq!(gets, 24, "96 MiB over 24 queued chunks: {requests:?}"); + } + + #[test] + fn the_planned_number_of_connections_is_used() { + let content = body(16); + let (base, requests) = test_server::serve_with( + vec![("/file".into(), content.clone())], + ServeOptions::default(), + ); + let dir = tempfile::tempdir().unwrap(); + let out = dir.path().join("file"); + + Download::new(format!("{base}/file")) + .output(out.to_string_lossy()) + .overwrite(OverwriteMode::Force) + .connections(8) + .execute() + .unwrap(); + + assert_eq!(std::fs::read(&out).unwrap(), content); + let requests = requests.lock().unwrap(); + let gets: Vec<_> = requests.iter().filter(|r| r.starts_with("GET")).collect(); + assert_eq!(gets.len(), 4, "16 MiB over 4 MiB chunks: {requests:?}"); + let ranged = gets.iter().filter(|r| r.contains("Some")).count(); + assert_eq!(ranged, 4, "every chunk on its own range: {requests:?}"); + } + + #[test] + fn a_server_without_ranges_downloads_in_one_stream() { + let content = body(16); + let (base, requests) = test_server::serve_with( + vec![("/file".into(), content.clone())], + ServeOptions { + ignore_ranges: true, + ..Default::default() + }, + ); + let dir = tempfile::tempdir().unwrap(); + let out = dir.path().join("file"); + + Download::new(format!("{base}/file")) + .output(out.to_string_lossy()) + .overwrite(OverwriteMode::Force) + .execute() + .unwrap(); + + assert_eq!(std::fs::read(&out).unwrap(), content); + let requests = requests.lock().unwrap(); + let probed = |r: &String| r.contains("range=Some((0, Some(0)))"); + assert!( + requests + .iter() + .filter(|r| r.starts_with("GET")) + .all(|r| probed(r) || r.ends_with("range=None")), + "only the one-byte probe asked for a range: {requests:?}" + ); + } + + #[test] + fn a_server_that_lies_about_ranges_falls_back_to_one_stream() { + let content = body(16); + let (base, requests) = test_server::serve_with( + vec![("/file".into(), content.clone())], + ServeOptions { + lie_about_ranges: true, + ..Default::default() + }, + ); + let dir = tempfile::tempdir().unwrap(); + let out = dir.path().join("file"); + + Download::new(format!("{base}/file")) + .output(out.to_string_lossy()) + .overwrite(OverwriteMode::Force) + .execute() + .unwrap(); + + assert_eq!(std::fs::read(&out).unwrap(), content); + let requests = requests.lock().unwrap(); + assert!( + requests + .iter() + .any(|r| r.starts_with("GET") && r.ends_with("range=None")), + "the fallback fetched without a range: {requests:?}" + ); + } + + #[test] + fn a_server_that_refuses_head_still_downloads_in_segments() { + let content = body(16); + let (base, requests) = test_server::serve_with( + vec![("/file".into(), content.clone())], + ServeOptions { + refuse_head: true, + ..Default::default() + }, + ); + let dir = tempfile::tempdir().unwrap(); + let out = dir.path().join("file"); + + Download::new(format!("{base}/file")) + .output(out.to_string_lossy()) + .overwrite(OverwriteMode::Force) + .execute() + .unwrap(); + + assert_eq!(std::fs::read(&out).unwrap(), content); + let requests = requests.lock().unwrap(); + let gets: Vec<_> = requests.iter().filter(|r| r.starts_with("GET")).collect(); + assert_eq!(gets.len(), 5, "the probe plus 4 chunks: {requests:?}"); + } + + #[test] + fn a_server_that_refuses_head_and_ranges_downloads_in_one_stream() { + let content = body(16); + let (base, requests) = test_server::serve_with( + vec![("/file".into(), content.clone())], + ServeOptions { + refuse_head: true, + ignore_ranges: true, + ..Default::default() + }, + ); + let dir = tempfile::tempdir().unwrap(); + let out = dir.path().join("file"); + + Download::new(format!("{base}/file")) + .output(out.to_string_lossy()) + .overwrite(OverwriteMode::Force) + .execute() + .unwrap(); + + assert_eq!(std::fs::read(&out).unwrap(), content); + let requests = requests.lock().unwrap(); + let probed = |r: &String| r.contains("range=Some((0, Some(0)))"); + assert!( + requests + .iter() + .filter(|r| r.starts_with("GET")) + .all(|r| probed(r) || r.ends_with("range=None")), + "only the one-byte probe asked for a range: {requests:?}" + ); + } + + #[test] + fn an_interrupted_segmented_download_resumes() { + let content = body(16); + let interrupted = test_server::serve_with( + vec![("/file".into(), content.clone())], + ServeOptions { + close_after_bytes: Some(1024 * 1024), + ..Default::default() + }, + ) + .0; + let dir = tempfile::tempdir().unwrap(); + let out = dir.path().join("file"); + let target = format!("{interrupted}/file"); + + let first = Download::new(&target) + .output(out.to_string_lossy()) + .overwrite(OverwriteMode::Skip) + .execute(); + assert!(first.is_err(), "the interrupted transfer must fail"); + + let segments = read_resume_segments(&out).expect("resume state kept after failure"); + assert_eq!(segments.len(), 4); + assert!( + segments.iter().any(|s| s.written > 0), + "the interruption left progress behind: {segments:?}" + ); + + let healthy = test_server::serve(vec![("/file".into(), content.clone())]); + let target = format!("{healthy}/file"); + Download::new(&target) + .output(out.to_string_lossy()) + .overwrite(OverwriteMode::Skip) + .execute() + .unwrap(); + assert_eq!(std::fs::read(&out).unwrap(), content); + } + + #[test] + fn a_mixed_state_resume_fetches_only_what_is_missing() { + let content = body(32); + let dir = tempfile::tempdir().unwrap(); + let out = dir.path().join("file"); + + // Plant a half-finished 8-chunk download: two chunks complete, three + // half written, three untouched. + let ranges = plan(content.len() as u64, 8); + assert_eq!(ranges.len(), 8); + let file = std::fs::File::create(&out).unwrap(); + file.set_len(content.len() as u64).unwrap(); + let segments: Vec = ranges + .iter() + .enumerate() + .map(|(i, &(start, end))| { + let len = end - start + 1; + let written = match i { + 0 | 1 => len, + 2..=4 => len / 2, + _ => 0, + }; + if written > 0 { + file.write_all_at(&content[start as usize..(start + written) as usize], start) + .unwrap(); + } + SegmentProgress { + start, + end, + written, + } + }) + .collect(); + let current: u64 = segments.iter().map(|s| s.written).sum(); + crate::xattr::write_resume( + &out, + &ResumeInfo { + downloaded: current, + total: content.len() as u64, + etag: None, + last_modified: None, + segments: Some(segments), + }, + ) + .unwrap(); + drop(file); + + let (base, requests) = test_server::serve_with( + vec![("/file".into(), content.clone())], + ServeOptions::default(), + ); + let seen = Arc::new(Mutex::new(Vec::new())); + let record = seen.clone(); + Download::new(format!("{base}/file")) + .output(out.to_string_lossy()) + .overwrite(OverwriteMode::Skip) + .progress(move |p| record.lock().unwrap().push(p)) + .execute() + .unwrap(); + + assert_eq!(std::fs::read(&out).unwrap(), content); + let requests = requests.lock().unwrap(); + let gets = requests.iter().filter(|r| r.starts_with("GET")).count(); + assert_eq!( + gets, 6, + "three partial and three untouched chunks: {requests:?}" + ); + let seen = seen.lock().unwrap(); + assert!( + seen.iter().any( + |p| matches!(p, Progress::Resuming { current, .. } if *current == 14 * 1024 * 1024) + ), + "the resume point was not reported: {seen:?}" + ); + } + + #[test] + fn plan_tolerates_a_huge_connection_count() { + assert_eq!(plan(1024, usize::MAX).len(), 1); + } + + #[test] + fn resume_state_with_gaps_or_disorder_is_refused() { + let seg = |start, end, written| { + SegmentProgress { + start, + end, + written, + } + }; + let total = 100; + + // A gap no request would ever fill. + assert!(plan_from_resume(total, &[seg(0, 49, 10), seg(60, 99, 0)]).is_none()); + // Out of order. + assert!(plan_from_resume(total, &[seg(50, 99, 0), seg(0, 49, 10)]).is_none()); + // Coverage short of the resource. + assert!(plan_from_resume(total, &[seg(0, 49, 10)]).is_none()); + // An exact tiling is still accepted. + assert!(plan_from_resume(total, &[seg(0, 49, 10), seg(50, 99, 50)]).is_some()); + } + + #[test] + fn a_chunk_interrupted_twice_completes_on_its_final_attempt() { + let content = body(16); + let (base, requests) = test_server::serve_with( + vec![("/file".into(), content.clone())], + ServeOptions { + // Chunks are 4 MiB here; a GET starting within the first + // 1 MiB of a chunk is cut, so every chunk's first two + // attempts (offsets +0, +1 MiB) drop and only its third + // (+2 MiB) carries it home, whatever the worker schedule. + truncate_periodic: Some((4 * 1024 * 1024, 1024 * 1024)), + ..Default::default() + }, + ); + let dir = tempfile::tempdir().unwrap(); + let out = dir.path().join("file"); + + let result = Download::new(format!("{base}/file")) + .output(out.to_string_lossy()) + .overwrite(OverwriteMode::Force) + .connections(8) + .execute(); + if let Err(err) = &result { + eprintln!("ERR {err} REQUESTS {:?}", requests.lock().unwrap()); + } + result.unwrap(); + + assert_eq!(std::fs::read(&out).unwrap(), content); + let requests = requests.lock().unwrap(); + let gets = requests.iter().filter(|r| r.starts_with("GET")).count(); + assert_eq!( + gets, 12, + "every chunk interrupted twice, whole on its third attempt: {requests:?}" + ); + } + + #[test] + fn an_oversending_server_yields_the_exact_resource() { + let content = body(16); + let (base, _requests) = test_server::serve_with( + vec![("/file".into(), content.clone())], + ServeOptions { + oversend: Some(8 * 1024 * 1024), + ..Default::default() + }, + ); + let dir = tempfile::tempdir().unwrap(); + let out = dir.path().join("file"); + + Download::new(format!("{base}/file")) + .output(out.to_string_lossy()) + .overwrite(OverwriteMode::Force) + .execute() + .unwrap(); + + let downloaded = std::fs::read(&out).unwrap(); + assert_eq!( + downloaded.len(), + content.len(), + "no overshoot bytes on disk" + ); + assert_eq!(downloaded, content); + } + + #[test] + fn a_resume_state_from_a_changed_resource_is_discarded() { + let stale = body(16); + let dir = tempfile::tempdir().unwrap(); + let out = dir.path().join("file"); + + // Plant a half-finished 4-chunk download of a 16 MiB resource. + let ranges = plan(stale.len() as u64, 8); + assert_eq!(ranges.len(), 4); + let plant = || { + let file = std::fs::File::create(&out).unwrap(); + file.set_len(stale.len() as u64).unwrap(); + file.write_all_at(&stale[..ranges[0].1 as usize + 1], 0) + .unwrap(); + let segments: Vec = ranges + .iter() + .enumerate() + .map(|(i, &(start, end))| { + SegmentProgress { + start, + end, + written: if i == 0 { end - start + 1 } else { 0 }, + } + }) + .collect(); + crate::xattr::write_resume( + &out, + &ResumeInfo { + downloaded: segments[0].written, + total: stale.len() as u64, + etag: None, + last_modified: None, + segments: Some(segments), + }, + ) + .unwrap(); + drop(file); + }; + + // The resource grew since the state was written. + let grew: Vec = body(32).into_iter().map(|b| !b).collect(); + plant(); + let base = test_server::serve(vec![("/file".into(), grew.clone())]); + Download::new(format!("{base}/file")) + .output(out.to_string_lossy()) + .overwrite(OverwriteMode::Skip) + .execute() + .unwrap(); + assert_eq!(std::fs::read(&out).unwrap(), grew); + + // The resource shrank since the state was written. + let shrank: Vec = body(8).into_iter().map(|b| !b).collect(); + plant(); + let base = test_server::serve(vec![("/file".into(), shrank.clone())]); + Download::new(format!("{base}/file")) + .output(out.to_string_lossy()) + .overwrite(OverwriteMode::Skip) + .execute() + .unwrap(); + assert_eq!(std::fs::read(&out).unwrap(), shrank); + } + + fn read_resume_segments(path: &std::path::Path) -> Option> { + crate::xattr::read_resume(path).and_then(|info| info.segments) + } +} diff --git a/crates/soar-dl/src/test_server.rs b/crates/soar-dl/src/test_server.rs index 61063f3e..e302a4d5 100644 --- a/crates/soar-dl/src/test_server.rs +++ b/crates/soar-dl/src/test_server.rs @@ -4,24 +4,66 @@ use std::{ collections::HashMap, io::{BufRead as _, BufReader, Write as _}, net::{TcpListener, TcpStream}, + sync::{Arc, Mutex}, thread, }; +/// How the server behaves where a download cares. +#[derive(Default, Clone)] +pub struct ServeOptions { + /// Send no `Accept-Ranges` header and answer range requests with the + /// whole body, as a server without range support would. + pub ignore_ranges: bool, + /// Send `Accept-Ranges: bytes` but still answer range requests with the + /// whole body, as a misbehaving server would. + pub lie_about_ranges: bool, + /// Close the connection mid-body once this many bytes of it are out, as + /// an interrupted transfer would. + pub close_after_bytes: Option, + /// Close any range request whose start offset lies at or within the + /// second field's bytes after a multiple of the first, then serve the + /// rest whole, as a link that repeatedly cuts the first stretch of every + /// chunk's transfer would. Deterministic under any worker interleaving. + pub truncate_periodic: Option<(usize, usize)>, + /// Send this many extra bytes past the requested range on 206 bodies, + /// as a server that overshoots its Content-Range would. + pub oversend: Option, + /// Drop every HEAD connection without a response, as HEAD-refusing + /// servers do. + pub refuse_head: bool, +} + /// Serves each body in `routes` at its path until the test ends, and returns -/// the base URL. `Range: bytes=N-` is honored with a 206, other paths are 404. +/// the base URL. `Range` requests are honored with a 206, other paths are 404. pub fn serve(routes: Vec<(String, Vec)>) -> String { + serve_with(routes, ServeOptions::default()).0 +} + +/// Serves under `options` and reports every request with its `Range` header, +/// in arrival order. +pub fn serve_with( + routes: Vec<(String, Vec)>, + options: ServeOptions, +) -> (String, Arc>>) { let listener = TcpListener::bind("127.0.0.1:0").unwrap(); let addr = listener.local_addr().unwrap(); let routes: HashMap<_, _> = routes.into_iter().collect(); + let requests: Arc>> = Arc::new(Mutex::new(Vec::new())); + let recorded = requests.clone(); thread::spawn(move || { for stream in listener.incoming().flatten() { - respond(stream, &routes); + respond(stream, &routes, &options, &recorded); } }); - format!("http://{addr}") + (format!("http://{addr}"), requests) } -fn respond(mut stream: TcpStream, routes: &HashMap>) { +fn respond( + mut stream: TcpStream, + routes: &HashMap>, + options: &ServeOptions, + requests: &Arc>>, +) { let mut reader = BufReader::new(stream.try_clone().unwrap()); let mut request_line = String::new(); if reader.read_line(&mut request_line).is_err() { @@ -31,7 +73,7 @@ fn respond(mut stream: TcpStream, routes: &HashMap>) { let method = parts.next().unwrap_or_default().to_string(); let path = parts.next().unwrap_or_default().to_string(); - let mut range_start = None; + let mut range: Option<(usize, Option)> = None; loop { let mut line = String::new(); if reader.read_line(&mut line).is_err() || line.trim().is_empty() { @@ -40,27 +82,30 @@ fn respond(mut stream: TcpStream, routes: &HashMap>) { if let Some((name, value)) = line.split_once(':') && name.eq_ignore_ascii_case("range") { - range_start = value - .trim() - .strip_prefix("bytes=") - .and_then(|r| r.strip_suffix('-')) - .and_then(|r| r.parse::().ok()); + range = value.trim().strip_prefix("bytes=").and_then(parse_range); } } + requests + .lock() + .unwrap() + .push(format!("{method} {path} range={range:?}")); + + if options.refuse_head && method == "HEAD" { + // Dropping the stream without a response is the refusal. + return; + } + let range_honored = !options.ignore_ranges && !options.lie_about_ranges; let (status, extra, body) = match routes.get(&path) { None => ("404 Not Found", String::new(), &b""[..]), Some(body) => { - match range_start { - Some(start) if start < body.len() => { + match range.filter(|_| range_honored) { + Some((start, end)) if start < body.len() => { + let end = end.map_or(body.len() - 1, |end| end.min(body.len() - 1)); ( "206 Partial Content", - format!( - "Content-Range: bytes {start}-{}/{}\r\n", - body.len() - 1, - body.len() - ), - &body[start..], + format!("Content-Range: bytes {start}-{end}/{}\r\n", body.len()), + &body[start..=end], ) } _ => ("200 OK", String::new(), &body[..]), @@ -68,12 +113,54 @@ fn respond(mut stream: TcpStream, routes: &HashMap>) { } }; - let head = format!( - "HTTP/1.1 {status}\r\nContent-Length: {}\r\n{extra}Connection: close\r\n\r\n", - body.len() + // Only whole 206 responses overshoot, simulating a server whose body + // does not stop where its Content-Range promised. + let overshoot = if status == "206 Partial Content" { + options.oversend.unwrap_or(0) + } else { + 0 + }; + let truncated = options + .truncate_periodic + .filter(|_| method == "GET") + .and_then(|(period, within)| { + let (start, _) = range?; + (start % period <= within).then_some(within) + }); + + let mut head = format!( + "HTTP/1.1 {status}\r\nContent-Length: {}\r\n{extra}", + body.len() + overshoot ); + if !options.ignore_ranges || options.lie_about_ranges { + head.push_str("Accept-Ranges: bytes\r\n"); + } + head.push_str("Connection: close\r\n\r\n"); let _ = stream.write_all(head.as_bytes()); - if method != "HEAD" { - let _ = stream.write_all(body); + if method == "HEAD" { + return; + } + if let Some(limit) = truncated { + let _ = stream.write_all(&body[..limit.min(body.len())]); + // Dropping the stream here is the interruption. + return; } + match options.close_after_bytes { + Some(limit) if limit < body.len() => { + let _ = stream.write_all(&body[..limit]); + // Dropping the stream here is the interruption. + } + _ => { + let _ = stream.write_all(body); + if overshoot > 0 { + let _ = stream.write_all(&vec![0u8; overshoot]); + } + } + } +} + +/// Parses a `bytes=N-M` or `bytes=N-` range value. +fn parse_range(value: &str) -> Option<(usize, Option)> { + let (start, end) = value.trim().split_once('-')?; + Some((start.trim().parse().ok()?, end.trim().parse().ok())) } diff --git a/crates/soar-dl/src/types.rs b/crates/soar-dl/src/types.rs index 4bcebf16..a4be58de 100644 --- a/crates/soar-dl/src/types.rs +++ b/crates/soar-dl/src/types.rs @@ -24,6 +24,17 @@ pub enum OverwriteMode { Prompt, } +/// Where one segment of a segmented download stands. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct SegmentProgress { + /// First byte of the segment, absolute in the file. + pub start: u64, + /// Last byte of the segment (inclusive), absolute in the file. + pub end: u64, + /// Bytes of this segment already on disk, counted from `start`. + pub written: u64, +} + /// Resume information stored in xattrs #[derive(Debug, Clone, Serialize, Deserialize)] pub struct ResumeInfo { @@ -31,6 +42,10 @@ pub struct ResumeInfo { pub total: u64, pub etag: Option, pub last_modified: Option, + /// Per-segment state of a segmented download. `None` for the + /// single-stream layout, where `downloaded` is a prefix length. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub segments: Option>, } #[cfg(test)] @@ -127,6 +142,7 @@ mod tests { total: 1024, etag: Some("\"abc123\"".to_string()), last_modified: None, + segments: None, }; assert_eq!(info.downloaded, 512); @@ -142,6 +158,7 @@ mod tests { total: 1024, etag: None, last_modified: Some("Wed, 21 Oct 2015 07:28:00 GMT".to_string()), + segments: None, }; assert_eq!(info.downloaded, 256); @@ -158,6 +175,7 @@ mod tests { total: 2048, etag: Some("etag-value".to_string()), last_modified: Some("date".to_string()), + segments: None, }; let json = serde_json::to_string(&info).unwrap(); @@ -167,6 +185,34 @@ mod tests { assert_eq!(deserialized.total, info.total); assert_eq!(deserialized.etag, info.etag); assert_eq!(deserialized.last_modified, info.last_modified); + assert!(deserialized.segments.is_none()); + } + + #[test] + fn resume_info_segments_round_trip() { + let info = ResumeInfo { + downloaded: 100, + total: 200, + etag: None, + last_modified: None, + segments: Some(vec![SegmentProgress { + start: 0, + end: 99, + written: 100, + }]), + }; + + let json = serde_json::to_string(&info).unwrap(); + let deserialized: ResumeInfo = serde_json::from_str(&json).unwrap(); + assert_eq!(deserialized.segments.as_ref().unwrap()[0].written, 100); + } + + #[test] + fn resume_info_without_segments_parses_from_old_json() { + let old = r#"{"downloaded":10,"total":20,"etag":null,"last_modified":null}"#; + let info: ResumeInfo = serde_json::from_str(old).unwrap(); + assert_eq!(info.downloaded, 10); + assert!(info.segments.is_none()); } #[test] @@ -176,6 +222,7 @@ mod tests { total: 200, etag: Some("tag".to_string()), last_modified: None, + segments: None, }; let info2 = info1.clone(); diff --git a/crates/soar-dl/src/utils.rs b/crates/soar-dl/src/utils.rs index 37bfb18f..bcd26492 100644 --- a/crates/soar-dl/src/utils.rs +++ b/crates/soar-dl/src/utils.rs @@ -122,6 +122,26 @@ pub(crate) fn open_output(path: &Path, resume_pos: Option) -> Result Result { + let mut options = OpenOptions::new(); + options.write(true).custom_flags(libc::O_NOFOLLOW); + + let file = options.open(path)?; + let metadata = file.metadata()?; + if !metadata.is_file() { + return Err( + std::io::Error::other(format!("{} is not a regular file", path.display())).into(), + ); + } + Ok(file) +} + /// Compute the final file system path for a download based on an explicit output and inferred filenames. /// /// The `output` argument can be: diff --git a/crates/soar-dl/src/xattr.rs b/crates/soar-dl/src/xattr.rs index 17d05209..dbe4fd92 100644 --- a/crates/soar-dl/src/xattr.rs +++ b/crates/soar-dl/src/xattr.rs @@ -79,6 +79,7 @@ pub fn read_usable_resume(path: &Path) -> Option { /// total: 10240, /// etag: Some("etag-value".into()), /// last_modified: None, +/// segments: None, /// }; /// let path = std::path::Path::new("/tmp/download.partial"); /// write_resume(path, &info).unwrap(); @@ -117,6 +118,7 @@ mod tests { total, etag: etag.map(String::from), last_modified: None, + segments: None, } } diff --git a/docs/configuration.md b/docs/configuration.md index 918aac7a..83312725 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -34,6 +34,7 @@ Control where soar stores its data. | `parallel` | Boolean | `true` | Enable parallel downloads | | `parallel_limit` | Integer | `4` | Max parallel downloads (1-16) | | `ghcr_concurrency` | Integer | `8` | Max GHCR concurrent requests (1-32) | +| `download_concurrency` | Integer | `8` | Connections per download; more than one fetches a range-capable server's file in parallel segments | | `search_limit` | Integer | `20` | Max search results (5-100) | | `cross_repo_updates` | Boolean | `false` | Allow cross-repo updates (not implemented) | diff --git a/docs/download.md b/docs/download.md index d4a8cde9..97163e89 100644 --- a/docs/download.md +++ b/docs/download.md @@ -108,6 +108,7 @@ Filters narrow down the available assets. If multiple assets remain after filter |--------|-------------| | `--skip-existing` | Skip download if the file already exists | | `--force-overwrite` | Overwrite existing files without prompting | +| `--connections` | Number of connections used per download (default: 8). More than one fetches a range-capable server's file in parallel segments, which lifts a server's per-connection speed cap; `1` streams a single response | ## Download Sources From 57c2234b4dcecaad17bf6743af36d0cf6d07b18f Mon Sep 17 00:00:00 2001 From: Rabindra Dhakal Date: Wed, 4 Nov 2026 19:41:35 +0545 Subject: [PATCH 2/5] fix(dl): harden segmented downloads and resume --- crates/soar-cli/src/cli.rs | 46 ++++- crates/soar-cli/src/download.rs | 5 +- crates/soar-cli/src/main.rs | 3 +- crates/soar-cli/src/plugin_manifest.rs | 13 ++ crates/soar-cli/src/self_actions.rs | 2 + crates/soar-dl/src/download.rs | 241 ++++++++++++++++++++--- crates/soar-dl/src/http.rs | 37 +++- crates/soar-dl/src/oci.rs | 106 ++++++++++- crates/soar-dl/src/segmented.rs | 252 +++++++++++++++++++------ crates/soar-dl/src/test_server.rs | 74 +++++++- crates/soar-operations/src/run.rs | 4 + docs/configuration.md | 8 +- docs/download.md | 2 +- 13 files changed, 682 insertions(+), 111 deletions(-) diff --git a/crates/soar-cli/src/cli.rs b/crates/soar-cli/src/cli.rs index 0e7e3349..32d311a8 100644 --- a/crates/soar-cli/src/cli.rs +++ b/crates/soar-cli/src/cli.rs @@ -3,6 +3,13 @@ use clap_complete::Shell; use crate::utils::parse_default_repos_arg; +fn parse_connections(value: &str) -> Result { + value + .parse::() + .map(std::num::NonZeroUsize::get) + .map_err(|_| "connections must be a positive integer".to_string()) +} + #[derive(Parser)] #[command( author, @@ -457,9 +464,9 @@ pub enum Commands { #[arg(required = false, long)] force_overwrite: bool, - /// Number of connections used per download; more than one fetches a - /// range-capable server's file in parallel segments - #[arg(required = false, long)] + /// Connections per HTTP or OCI blob download (must be positive). + /// OCI manifest layers use separate single-stream downloads. + #[arg(required = false, long, value_parser = parse_connections)] connections: Option, }, @@ -595,3 +602,36 @@ pub enum Commands { repo: Option, }, } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn download_connections_must_be_positive() { + for value in ["0", "-1", "invalid"] { + assert!( + Args::try_parse_from([ + "soar", + "download", + "--connections", + value, + "https://example.com/file" + ]) + .is_err() + ); + } + for value in ["1", "8"] { + assert!( + Args::try_parse_from([ + "soar", + "download", + "--connections", + value, + "https://example.com/file" + ]) + .is_ok() + ); + } + } +} diff --git a/crates/soar-cli/src/download.rs b/crates/soar-cli/src/download.rs index 87b455db..00434c85 100644 --- a/crates/soar-cli/src/download.rs +++ b/crates/soar-cli/src/download.rs @@ -257,7 +257,9 @@ async fn download_link(ctx: &DownloadContext, link: &str) -> SoarResult<()> { info!("Downloading package: {}", package.pkg_name); if let Some(ref url) = package.ghcr_blob { - let mut dl = OciDownload::new(url.as_str()).overwrite(ctx.get_overwrite_mode()); + let mut dl = OciDownload::new(url.as_str()) + .connections(ctx.connections) + .overwrite(ctx.get_overwrite_mode()); if let Some(ref out) = ctx.output { dl = dl.output(out); @@ -294,6 +296,7 @@ async fn handle_oci_download(ctx: &DownloadContext, reference: &str) -> SoarResu info!("Downloading using OCI reference: {}", reference); let mut dl = OciDownload::new(reference) + .connections(ctx.connections) .filter(ctx.create_filter()) .parallel(get_config().ghcr_concurrency.unwrap_or(8)) .overwrite(ctx.get_overwrite_mode()); diff --git a/crates/soar-cli/src/main.rs b/crates/soar-cli/src/main.rs index 04336064..2d779f94 100644 --- a/crates/soar-cli/src/main.rs +++ b/crates/soar-cli/src/main.rs @@ -599,8 +599,7 @@ async fn handle_cli() -> SoarResult<()> { let connections = connections .or(get_config().download_concurrency) - .unwrap_or(8) - .max(1); + .unwrap_or(8); let context = DownloadContext { regexes, globs, diff --git a/crates/soar-cli/src/plugin_manifest.rs b/crates/soar-cli/src/plugin_manifest.rs index 35094516..abe368e6 100644 --- a/crates/soar-cli/src/plugin_manifest.rs +++ b/crates/soar-cli/src/plugin_manifest.rs @@ -47,6 +47,12 @@ label = "Parallel limit" type = "number" default = 4 +[[config]] +key = "download_concurrency" +label = "Connections per download" +type = "number" +default = 8 + [[config]] key = "search_limit" label = "Search result limit" @@ -260,6 +266,13 @@ mod tests { toml::from_str(&manifest(&["default".into()])).expect("manifest must parse"); assert_eq!(parsed["schema_version"].as_integer(), Some(1)); assert_eq!(parsed["id"].as_str(), Some("soar")); + let connections = parsed["config"] + .as_array() + .unwrap() + .iter() + .find(|entry| entry["key"].as_str() == Some("download_concurrency")) + .expect("download concurrency setting"); + assert_eq!(connections["default"].as_integer(), Some(8)); } #[test] diff --git a/crates/soar-cli/src/self_actions.rs b/crates/soar-cli/src/self_actions.rs index fad04eb9..2b066200 100644 --- a/crates/soar-cli/src/self_actions.rs +++ b/crates/soar-cli/src/self_actions.rs @@ -4,6 +4,7 @@ use std::{ }; use semver::Version; +use soar_config::config::get_config; use soar_core::{ SoarResult, error::{ErrorContext, SoarError}, @@ -163,6 +164,7 @@ pub async fn process_self_action(action: &SelfAction) -> SoarResult<()> { })?; let dl = Download::new(asset.url()) + .connections(get_config().download_concurrency.unwrap_or(8)) .output(staging.to_string_lossy()) .overwrite(OverwriteMode::Force) .checksum(expected_checksum) diff --git a/crates/soar-dl/src/download.rs b/crates/soar-dl/src/download.rs index bb897386..8d9cdbb6 100644 --- a/crates/soar-dl/src/download.rs +++ b/crates/soar-dl/src/download.rs @@ -18,7 +18,7 @@ use ureq::{ use crate::{ error::DownloadError, - http::{Http, RemoteInfo}, + http::{Http, RemoteInfo, strong_etag}, segmented, types::{OverwriteMode, Progress, ResumeInfo, SegmentProgress}, utils::{ @@ -481,12 +481,29 @@ impl Download { resume_info: Option, remote: Option, ) -> Result { + if resume_info + .as_ref() + .is_some_and(|info| info.etag.as_deref().and_then(strong_etag).is_none()) + { + warn!("resume has no strong ETag, restarting download"); + return self.download_to_file(path, None, remote); + } + // A segmented download continues segment by segment; its file is // pre-allocated, so appending to it as a single stream would splice // the ranges out of place. if let Some(info) = &resume_info && let Some(segments) = &info.segments { + let remote = remote.or_else(|| { + Http::head(&self.url) + .ok() + .map(|resp| RemoteInfo::from_head(&resp)) + }); + let etag_changed = remote + .as_ref() + .and_then(|r| r.etag.as_deref()) + .is_some_and(|tag| Some(tag) != info.etag.as_deref()); // A size the server just reported that disagrees with the state // means the resource changed since it was written; the offsets // no longer describe it and resuming would splice two versions. @@ -495,7 +512,13 @@ impl Download { .and_then(|remote| remote.size) .is_some_and(|size| size != info.total); return match segmented::plan_from_resume(info.total, segments) { - Some(resumed) if !size_changed => { + Some(resumed) + if !size_changed + && !etag_changed + && (resumed.current < info.total + || remote.as_ref().and_then(|r| r.etag.as_deref()) + == info.etag.as_deref()) => + { debug!(offset = resumed.current, "resuming segmented download"); self.download_segmented( path, @@ -514,6 +537,10 @@ impl Download { }; } + if resume_info.is_some() { + return self.download_single(path, resume_info); + } + if self.connections > 1 { // The HEAD only picks a strategy, so a server that refuses it // still gets the download, in one stream. A HEAD that is silent @@ -538,6 +565,7 @@ impl Download { } if let Some(total) = remote.size && remote.ranges_ok + && remote.etag.as_deref().and_then(strong_etag).is_some() && total >= segmented::MIN_SEGMENTED_TOTAL { let ranges = segmented::plan(total, self.connections); @@ -571,25 +599,6 @@ impl Download { etag: Option<&str>, ) -> Result { let current: u64 = written.iter().sum(); - let file = if current == 0 { - trace!(path = %path.display(), total = total, "allocating segmented download"); - let file = open_output(path, None)?; - file.set_len(total)?; - file - } else { - let file = open_existing(path)?; - if file.metadata()?.len() != total { - warn!( - path = %path.display(), - "file size does not match the resource, restarting download" - ); - let ranges = segmented::plan(total, self.connections); - let written = vec![0; ranges.len()]; - return self.download_segmented(path, total, ranges, written, etag); - } - file - }; - let info = ResumeInfo { downloaded: current, total, @@ -609,6 +618,35 @@ impl Download { .collect(), ), }; + let file = if current == 0 { + if path.exists() { + // Replace old offsets before truncating an existing allocation. + if let Err(err) = write_resume(path, &info) { + if read_usable_resume(path).is_some() { + return Err(err.into()); + } + warn!(%err, "filesystem cannot store resume state, falling back to single stream"); + return self.download_single(path, None); + } + } + let file = open_output(path, None)?; + if let Err(err) = write_resume(path, &info) { + warn!(%err, "filesystem cannot store resume state, falling back to single stream"); + return self.download_single(path, None); + } + file.set_len(total)?; + file + } else { + let file = open_existing(path)?; + if file.metadata()?.len() != total { + warn!(path = %path.display(), "file size does not match the resource, restarting download"); + let ranges = segmented::plan(total, self.connections); + let written = vec![0; ranges.len()]; + return self.download_segmented(path, total, ranges, written, etag); + } + file + }; + if let Err(err) = write_resume(path, &info) { warn!(%err, "filesystem cannot store resume state, falling back to single stream"); return self.download_single(path, None); @@ -702,9 +740,14 @@ impl Download { .headers() .get(ETAG) .and_then(|h| h.to_str().ok()) - .map(String::from); + .map(String::from) + .or_else(|| etag.map(String::from)); trace!(total = total, etag = ?new_etag, "parsed response headers"); + if resume_from.is_none() && path.is_file() && read_usable_resume(path).is_some() { + fs::remove_file(path)?; + } + let mut file = match open_output(path, resume_from) { Err(DownloadError::ResumeMismatch) => { warn!(path = %path.display(), "file shrank below its resume offset, restarting download"); @@ -820,7 +863,12 @@ pub(crate) fn check_partial_response( Some(total) => info.total == 0 || info.total == total, None => true, }; - if start == Some(info.downloaded) && same_size { + let same_etag = resp + .headers() + .get(ETAG) + .and_then(|header| header.to_str().ok()) + .is_none_or(|tag| info.etag.as_deref() == Some(tag)); + if start == Some(info.downloaded) && same_size && same_etag { Ok(()) } else { Err(DownloadError::ResumeMismatch) @@ -940,7 +988,7 @@ mod tests { let info = ResumeInfo { downloaded: 8_000, total: body().len() as u64, - etag: None, + etag: Some("\"test-etag\"".into()), last_modified: None, segments: None, }; @@ -968,6 +1016,151 @@ mod tests { assert_eq!(std::fs::read(&out).unwrap(), body()); } + #[test] + fn stale_segment_validators_restart_without_splicing() { + let base = crate::test_server::serve(vec![("/file".into(), body())]); + for (stored, observed) in [ + (Some("W/\"old\""), Some("W/\"old\"")), + (None, Some("\"new\"")), + (Some("\"old\""), Some("\"new\"")), + ] { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("file"); + std::fs::write(&path, vec![42; body().len()]).unwrap(); + let info = ResumeInfo { + downloaded: 8000, + total: body().len() as u64, + etag: stored.map(String::from), + last_modified: None, + segments: Some(vec![SegmentProgress { + start: 0, + end: body().len() as u64 - 1, + written: 8000, + }]), + }; + if write_resume(&path, &info).is_err() { + return; + } + let remote = RemoteInfo { + size: Some(body().len() as u64), + ranges_ok: true, + etag: observed.map(String::from), + }; + Download::new(format!("{base}/file")) + .connections(1) + .download_to_file(&path, Some(info), Some(remote)) + .unwrap(); + assert_eq!(std::fs::read(path).unwrap(), body()); + } + } + + #[test] + fn weak_single_resume_restarts() { + let base = crate::test_server::serve(vec![("/file".into(), body())]); + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("file"); + std::fs::write(&path, vec![42; 8000]).unwrap(); + let info = ResumeInfo { + downloaded: 8000, + total: body().len() as u64, + etag: Some("W/\"old\"".into()), + last_modified: None, + segments: None, + }; + if write_resume(&path, &info).is_err() { + return; + } + Download::new(format!("{base}/file")) + .connections(1) + .download_to_file(&path, Some(info), None) + .unwrap(); + assert_eq!(std::fs::read(path).unwrap(), body()); + } + + #[test] + fn single_resume_takes_precedence_over_segmenting() { + let data = vec![7; segmented::MIN_SEGMENTED_TOTAL as usize]; + let (base, requests) = crate::test_server::serve_with( + vec![("/file".into(), data.clone())], + Default::default(), + ); + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("file"); + std::fs::write(&path, &data[..8000]).unwrap(); + let info = ResumeInfo { + downloaded: 8000, + total: data.len() as u64, + etag: Some("\"test-etag\"".into()), + last_modified: None, + segments: None, + }; + let remote = RemoteInfo { + size: Some(data.len() as u64), + ranges_ok: true, + etag: Some("\"test-etag\"".into()), + }; + Download::new(format!("{base}/file")) + .download_to_file(&path, Some(info), Some(remote)) + .unwrap(); + assert_eq!(std::fs::read(path).unwrap(), data); + assert!( + requests + .lock() + .unwrap() + .iter() + .any(|r| r.contains("range=Some((8000, None))")) + ); + } + + #[test] + fn failed_segmented_restart_keeps_zeroed_state_for_new_allocation() { + let data = vec![7; segmented::MIN_SEGMENTED_TOTAL as usize]; + let (base, _) = crate::test_server::serve_with( + vec![("/file".into(), data.clone())], + crate::test_server::ServeOptions { + close_after_bytes: Some(0), + ..Default::default() + }, + ); + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("file"); + std::fs::write(&path, vec![42; 1000]).unwrap(); + let total = data.len() as u64; + let ranges = segmented::plan(total, 2); + let old = ResumeInfo { + downloaded: 1000, + total, + etag: Some("\"test-etag\"".into()), + last_modified: None, + segments: Some( + ranges + .iter() + .enumerate() + .map(|(i, &(start, end))| { + SegmentProgress { + start, + end, + written: if i == 0 { 1000 } else { 0 }, + } + }) + .collect(), + ), + }; + if write_resume(&path, &old).is_err() { + return; + } + let dl = Download::new(format!("{base}/file")).connections(2); + let written = vec![1000, 0]; + assert!( + dl.download_segmented(&path, total, ranges, written, Some("\"test-etag\"")) + .is_err() + ); + assert_eq!(std::fs::metadata(&path).unwrap().len(), total); + let saved = crate::xattr::read_resume(&path).unwrap(); + assert_eq!(saved.downloaded, 0); + assert!(saved.segments.unwrap().iter().all(|s| s.written == 0)); + } + #[test] fn a_symlink_planted_at_the_output_is_not_followed() { let base = crate::test_server::serve(vec![("/file".into(), body())]); diff --git a/crates/soar-dl/src/http.rs b/crates/soar-dl/src/http.rs index 18ccbc5e..8d5d3d30 100644 --- a/crates/soar-dl/src/http.rs +++ b/crates/soar-dl/src/http.rs @@ -38,6 +38,22 @@ impl RemoteInfo { } } +/// Returns a valid strong entity tag suitable for an If-Range request. +pub(crate) fn strong_etag(tag: &str) -> Option<&str> { + let bytes = tag.as_bytes(); + if bytes.len() >= 2 + && bytes[0] == b'"' + && bytes[bytes.len() - 1] == b'"' + && bytes[1..bytes.len() - 1] + .iter() + .all(|&byte| byte == 0x21 || (0x23..=0x7e).contains(&byte) || byte >= 0x80) + { + Some(tag) + } else { + None + } +} + fn content_length_header(headers: &ureq::http::HeaderMap) -> Option { headers .get(CONTENT_LENGTH) @@ -90,7 +106,7 @@ impl Http { /// Fetches a GET response for the given URL, optionally requesting a byte range and using an ETag for conditional requests. /// /// If `resume_from` is `Some(pos)`, the request includes a `Range: bytes={pos}-` header. If `etag` is `Some(tag)` and a range is requested, - /// the request also includes an `If-Range: {tag}` header. + /// the request also includes an `If-Range: {tag}` header when the tag is a valid strong ETag. /// /// # Returns /// @@ -127,7 +143,7 @@ impl Http { if let Some(pos) = resume_from { debug!(" Range: bytes={}-", pos); req = req.header("Range", &format!("bytes={}-", pos)); - if let Some(tag) = etag { + if let Some(tag) = etag.and_then(strong_etag) { trace!(etag = tag, "adding If-Range header"); req = req.header("If-Range", tag); } @@ -164,7 +180,7 @@ impl Http { } req = req.header("Range", &format!("bytes={start}-{end}")); - if let Some(tag) = etag { + if let Some(tag) = etag.and_then(strong_etag) { trace!(etag = tag, "adding If-Range header"); req = req.header("If-Range", tag); } @@ -266,3 +282,18 @@ impl Http { result } } + +#[cfg(test)] +mod tests { + use super::strong_etag; + + #[test] + fn only_strong_entity_tags_are_usable_for_if_range() { + for tag in ["\"\"", "\"abc!#$%\""] { + assert_eq!(strong_etag(tag), Some(tag)); + } + for tag in ["W/\"abc\"", "abc", "\"a b\"", "\"a\"b\"", "\"a\n\""] { + assert_eq!(strong_etag(tag), None); + } + } +} diff --git a/crates/soar-dl/src/oci.rs b/crates/soar-dl/src/oci.rs index cafcd6b9..963f06f7 100644 --- a/crates/soar-dl/src/oci.rs +++ b/crates/soar-dl/src/oci.rs @@ -21,6 +21,7 @@ use crate::{ download::{Download, check_partial_response}, error::DownloadError, filter::Filter, + http::strong_etag, http_client::SHARED_AGENT, types::{OverwriteMode, Progress, ResumeInfo}, utils::open_output, @@ -252,6 +253,7 @@ pub struct OciDownload { extract: bool, extract_to: Option, parallel: usize, + connections: Option, on_progress: Option>, } @@ -265,6 +267,7 @@ impl OciDownload { /// - `overwrite` = `OverwriteMode::Prompt` /// - `extract` = `false` /// - `parallel` = 1 (sequential downloads) + /// - direct blob downloads use the default connection count from `Download` /// - no progress callback /// /// # Examples @@ -287,6 +290,7 @@ impl OciDownload { extract: false, extract_to: None, parallel: 1, + connections: None, on_progress: None, } } @@ -413,6 +417,16 @@ impl OciDownload { self } + /// Sets the maximum connections for a direct digest blob download. + /// + /// Range-capable servers can use parallel segments. Zero is treated as one. + /// Manifest layers use a separate single-stream downloader; use `parallel` + /// to control how many layers can download concurrently. + pub fn connections(mut self, connections: usize) -> Self { + self.connections = Some(connections.max(1)); + self + } + /// Registers a progress callback to receive download `Progress` events. /// /// The provided callback will be invoked with progress updates (e.g., Starting, Chunk, Complete) @@ -845,6 +859,12 @@ impl OciDownload { .overwrite(self.overwrite) .ghcr_blob(); + let dl = if let Some(connections) = self.connections { + dl.connections(connections) + } else { + dl + }; + let dl = if let Some(ref cb) = self.on_progress { let cb = cb.clone(); dl.progress(move |p| cb(p)) @@ -947,7 +967,8 @@ fn download_layer_impl( "downloading layer" ); - let mut resume_info = read_usable_resume(path); + let mut resume_info = read_usable_resume(path) + .filter(|info| info.etag.as_deref().and_then(strong_etag).is_some()); let resp = loop { let mut req = SHARED_AGENT.get(&url).header(AUTHORIZATION, "Bearer QQ=="); if let Some(info) = &resume_info { @@ -956,7 +977,7 @@ fn download_layer_impl( "attempting to resume download" ); req = req.header(RANGE, &format!("bytes={}-", info.downloaded)); - if let Some(tag) = &info.etag { + if let Some(tag) = info.etag.as_deref().and_then(strong_etag) { req = req.header(IF_RANGE, tag); } } @@ -1458,7 +1479,7 @@ mod tests { let info = ResumeInfo { downloaded: 20_000, total: bytes.len() as u64, - etag: None, + etag: Some("\"test-etag\"".into()), last_modified: None, segments: None, }; @@ -1487,6 +1508,85 @@ mod tests { assert!(crate::xattr::read_resume(&path).is_none()); } + #[test] + fn unvalidated_layer_checkpoint_restarts_without_a_range() { + let bytes = vec![7u8; 50_000]; + let layer = layer("one", &bytes); + for etag in [None, Some("W/\"test-etag\"".to_string())] { + let (base, requests) = crate::test_server::serve_with( + vec![( + format!("/v2/org/repo/blobs/{}", layer.digest), + bytes.clone(), + )], + Default::default(), + ); + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("one"); + std::fs::write(&path, &bytes[..20_000]).unwrap(); + if write_resume( + &path, + &ResumeInfo { + downloaded: 20_000, + total: bytes.len() as u64, + etag, + last_modified: None, + segments: None, + }, + ) + .is_err() + { + eprintln!("skipping: filesystem lacks user xattrs"); + return; + } + let dl = OciDownload::new("org/repo:tag").api(format!("{base}/v2")); + dl.download_layer(&layer, &path, &mut 0, layer.size) + .unwrap(); + assert_eq!(std::fs::read(path).unwrap(), bytes); + assert!( + requests + .lock() + .unwrap() + .iter() + .all(|r| r.contains("range=None")) + ); + } + } + + #[test] + fn direct_blob_connections_control_segmentation() { + let bytes = vec![5u8; 8 * 1024 * 1024]; + let digest = format!("sha256:{}", sha256_hex(&bytes)); + for connections in [1, 2] { + let (base, requests) = crate::test_server::serve_with( + vec![(format!("/v2/org/repo/blobs/{digest}"), bytes.clone())], + Default::default(), + ); + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("blob"); + OciDownload::new(format!("org/repo@{digest}").as_str()) + .api(format!("{base}/v2")) + .output(path.to_string_lossy()) + .connections(connections) + .execute() + .unwrap(); + assert_eq!(std::fs::read(path).unwrap(), bytes); + let requests = requests.lock().unwrap(); + let ranged = requests + .iter() + .filter(|r| r.starts_with("GET ") && r.contains("range=Some")) + .count(); + if connections == 1 { + assert_eq!(ranged, 0, "{requests:?}"); + } else { + assert!(ranged >= 2, "{requests:?}"); + } + } + assert_eq!( + OciDownload::new("org/repo:tag").connections(0).connections, + Some(1) + ); + } + #[test] fn a_blob_lands_in_an_output_directory_under_the_package_name() { let bytes = vec![5u8; 25_000]; diff --git a/crates/soar-dl/src/segmented.rs b/crates/soar-dl/src/segmented.rs index 40cfb8d8..008d6885 100644 --- a/crates/soar-dl/src/segmented.rs +++ b/crates/soar-dl/src/segmented.rs @@ -49,7 +49,7 @@ const CHUNK_BUFFER: usize = 256 * 1024; /// Aggregate bytes between two resume checkpoints and progress reports. const REPORT_INTERVAL: u64 = 1024 * 1024; -/// Times one chunk may fail on the network before the download gives up and +/// Consecutive attempts without progress before the download gives up and /// leaves its resume state for the next run. const MAX_ATTEMPTS: u32 = 3; @@ -57,7 +57,10 @@ const MAX_ATTEMPTS: u32 = 3; /// [`CHUNKS_PER_CONNECTION`] per connection, capped by the per-chunk minimum /// and [`MAX_CHUNKS`]. The last chunk absorbs the remainder. pub(crate) fn plan(total: u64, connections: usize) -> Vec<(u64, u64)> { - let max_chunks = (total / MIN_SEGMENT_SIZE).max(1) as usize; + if total == 0 { + return Vec::new(); + } + let max_chunks = (total / MIN_SEGMENT_SIZE).max(1).min(MAX_CHUNKS as u64) as usize; let parts = connections .max(1) .saturating_mul(CHUNKS_PER_CONNECTION) @@ -148,7 +151,7 @@ struct Shared { /// Bytes written across all chunks. aggregate: AtomicU64, /// The aggregate the last checkpoint and progress report carried. - reported: AtomicU64, + reported: Mutex, /// Bytes written per chunk, counted from the chunk's start. chunk_written: Box<[AtomicU64]>, /// The full chunk plan, for resume checkpoints. @@ -203,7 +206,7 @@ pub(crate) fn run(job: Job) -> Result<(), DownloadError> { let shared = Arc::new(Shared { cancel: AtomicBool::new(false), aggregate: AtomicU64::new(initial), - reported: AtomicU64::new(initial), + reported: Mutex::new(initial), chunk_written: job .written .iter() @@ -249,7 +252,7 @@ pub(crate) fn run(job: Job) -> Result<(), DownloadError> { etag: job.etag, path: job.path, total: job.total, - file: job.file.try_clone().expect("cloning the output file"), + file: job.file, progress: job.on_progress.cloned(), }; handles.push(scope.spawn(move || worker.run())); @@ -269,16 +272,14 @@ pub(crate) fn run(job: Job) -> Result<(), DownloadError> { // A server that ignored one range request is treated as ignoring them all; // the caller falls back to a single stream, which needs no ranges. - if let Some(Outcome::Failed(err)) = outcomes - .iter() - .find(|o| matches!(o, Outcome::Failed(DownloadError::RangeUnsupported { .. }))) - { - let url = match err { - DownloadError::RangeUnsupported { + if let Some(url) = outcomes.iter().find_map(|outcome| { + match outcome { + Outcome::Failed(DownloadError::RangeUnsupported { url, - } => url.clone(), - _ => unreachable!(), - }; + }) => Some(url.clone()), + _ => None, + } + }) { return Err(DownloadError::RangeUnsupported { url, }); @@ -291,13 +292,6 @@ pub(crate) fn run(job: Job) -> Result<(), DownloadError> { } } - if let Some(cb) = job.on_progress { - cb(Progress::Chunk { - current: shared.aggregate.load(Ordering::Relaxed), - total: job.total, - }); - } - Ok(()) } @@ -308,7 +302,7 @@ struct Worker<'a> { etag: Option<&'a str>, path: &'a Path, total: u64, - file: File, + file: &'a File, progress: Option>, } @@ -378,12 +372,16 @@ impl Worker<'_> { return Ok(()); } attempts += 1; + let previous_cursor = cursor; match self.fetch_and_write(index, start, end, &mut cursor) { Ok(()) => continue, Err(err) => { if self.shared.cancel.load(Ordering::Relaxed) { return Err(err); } + if cursor > previous_cursor { + attempts = 0; + } if err.is_transient() && attempts < MAX_ATTEMPTS { debug!(chunk = index, attempt = attempts, %err, "chunk failed, retrying"); continue; @@ -401,11 +399,24 @@ impl Worker<'_> { end: u64, cursor: &mut u64, ) -> Result<(), DownloadError> { - let resp = Http::fetch_range(self.url, *cursor, end, self.etag, self.ghcr_blob)?; + let resp = Http::fetch_range(self.url, *cursor, end, self.etag, self.ghcr_blob).map_err( + |err| { + match err { + DownloadError::HttpError { + status: 416, .. + } => { + DownloadError::RangeUnsupported { + url: self.url.to_string(), + } + } + err => err, + } + }, + )?; if resp.status() != 206 { let status = resp.status().as_u16(); - if status == 200 { + if status == 200 || status == 416 { trace!(url = self.url, "server ignored the range request"); return Err(DownloadError::RangeUnsupported { url: self.url.to_string(), @@ -432,10 +443,12 @@ impl Worker<'_> { url: self.url.to_string(), }); }; - let matches = span - .split_once('-') - .and_then(|(start, _)| start.trim().parse::().ok()) - .is_some_and(|start| start == *cursor) + let matches = span.split_once('-').and_then(|(start, finish)| { + Some(( + start.trim().parse::().ok()?, + finish.trim().parse::().ok()?, + )) + }) == Some((*cursor, end)) && total.trim().parse::() == Ok(self.total); if !matches { return Err(DownloadError::RangeUnsupported { @@ -481,27 +494,26 @@ impl Worker<'_> { /// [`REPORT_INTERVAL`] aggregate bytes; one worker wins that right per /// interval, so the xattr and the bar advance in step. fn maybe_report(&self, aggregate: u64) { - let reported = self.shared.reported.load(Ordering::Relaxed); - if aggregate.saturating_sub(reported) < REPORT_INTERVAL { - return; - } - if self - .shared - .reported - .compare_exchange(reported, aggregate, Ordering::Relaxed, Ordering::Relaxed) - .is_err() - { + let mut reported = self.shared.reported.lock().unwrap(); + // Another worker may have reported more recent progress while this + // worker was waiting for the reporting lock. + let aggregate = self.shared.aggregate.load(Ordering::Relaxed).max(aggregate); + if aggregate.saturating_sub(*reported) < REPORT_INTERVAL { return; } + *reported = aggregate; - if let Some(cb) = &self.progress { + // The caller emits the final Chunk event after all workers finish. + if aggregate < self.total + && let Some(cb) = &self.progress + { cb(Progress::Chunk { current: aggregate, total: self.total, }); } - let segments = self + let segments: Vec = self .shared .ranges .iter() @@ -515,7 +527,7 @@ impl Worker<'_> { }) .collect(); let info = ResumeInfo { - downloaded: aggregate, + downloaded: segments.iter().map(|segment| segment.written).sum(), total: self.total, etag: self.etag.map(String::from), last_modified: None, @@ -538,6 +550,15 @@ mod tests { types::OverwriteMode, }; + fn supports_xattrs() -> bool { + let file = tempfile::NamedTempFile::new().unwrap(); + match xattr::set(file.path(), "user.soar.test", b"test") { + Ok(()) => true, + Err(err) if err.kind() == std::io::ErrorKind::Unsupported => false, + Err(err) => panic!("failed to test xattr support: {err}"), + } + } + fn body(mib: usize) -> Vec { ((0..mib * 1024 * 1024).map(|i| (i % 251) as u8)).collect() } @@ -553,6 +574,86 @@ mod tests { next == total } + #[test] + fn unsatisfiable_range_triggers_fallback() { + let base = test_server::serve(vec![("/file".into(), b"small".to_vec())]); + let file = tempfile::NamedTempFile::new().unwrap(); + let url = format!("{base}/file"); + let result = run(Job { + url: &url, + ghcr_blob: false, + etag: None, + path: file.path(), + file: file.as_file(), + total: 20, + connections: 1, + ranges: vec![(10, 19)], + written: vec![0], + on_progress: None, + }); + assert!(matches!( + result, + Err(DownloadError::RangeUnsupported { .. }) + )); + } + + #[test] + fn retries_without_progress_are_bounded() { + let (base, requests) = test_server::serve_with( + vec![("/file".into(), b"hello".to_vec())], + ServeOptions { + close_after_bytes: Some(0), + ..Default::default() + }, + ); + let file = tempfile::NamedTempFile::new().unwrap(); + let url = format!("{base}/file"); + let result = run(Job { + url: &url, + ghcr_blob: false, + etag: None, + path: file.path(), + file: file.as_file(), + total: 5, + connections: 1, + ranges: vec![(0, 4)], + written: vec![0], + on_progress: None, + }); + assert!(result.is_err()); + assert_eq!(requests.lock().unwrap().len(), MAX_ATTEMPTS as usize); + } + + #[test] + fn incorrect_content_range_end_triggers_fallback() { + use std::io::{Read, Write}; + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let url = format!("http://{}", listener.local_addr().unwrap()); + std::thread::spawn(move || { + let (mut stream, _) = listener.accept().unwrap(); + let mut request = [0; 4096]; + let _ = stream.read(&mut request); + let _ = stream.write_all(b"HTTP/1.1 206 Partial Content\r\nContent-Range: bytes 0-3/5\r\nContent-Length: 5\r\nConnection: close\r\n\r\nabcde"); + }); + let file = tempfile::NamedTempFile::new().unwrap(); + let result = run(Job { + url: &url, + ghcr_blob: false, + etag: None, + path: file.path(), + file: file.as_file(), + total: 5, + connections: 1, + ranges: vec![(0, 4)], + written: vec![0], + on_progress: None, + }); + assert!(matches!( + result, + Err(DownloadError::RangeUnsupported { .. }) + )); + } + #[test] fn plan_queues_chunks_per_connection() { assert_eq!(plan(1024 * 1024 * 1024, 8).len(), 32); @@ -576,6 +677,11 @@ mod tests { assert!(ranges_are_contiguous(&ranges, total)); } + #[test] + fn plan_of_an_empty_file_has_no_segments() { + assert!(plan(0, 8).is_empty()); + } + #[test] fn plan_of_a_tiny_file_is_one_segment() { let ranges = plan(1024, 8); @@ -606,6 +712,9 @@ mod tests { #[test] fn a_large_download_is_fetched_in_segments() { + if !supports_xattrs() { + return; + } let content = body(16); let base = test_server::serve(vec![("/file".into(), content.clone())]); let dir = tempfile::tempdir().unwrap(); @@ -623,6 +732,9 @@ mod tests { #[test] fn a_multi_chunk_download_reassembles() { + if !supports_xattrs() { + return; + } let content = body(96); let (base, requests) = test_server::serve_with( vec![("/file".into(), content.clone())], @@ -631,13 +743,30 @@ mod tests { let dir = tempfile::tempdir().unwrap(); let out = dir.path().join("file"); + let seen = Arc::new(Mutex::new(Vec::new())); + let record = seen.clone(); Download::new(format!("{base}/file")) .output(out.to_string_lossy()) .overwrite(OverwriteMode::Force) .connections(8) + .progress(move |p| record.lock().unwrap().push(p)) .execute() .unwrap(); + let seen = seen.lock().unwrap(); + let chunks: Vec<_> = seen + .iter() + .filter_map(|p| { + match p { + Progress::Chunk { + current, .. + } => Some(*current), + _ => None, + } + }) + .collect(); + assert!(chunks.windows(2).all(|w| w[0] < w[1]), "{chunks:?}"); + assert_eq!(chunks.last(), Some(&(content.len() as u64))); assert_eq!(std::fs::read(&out).unwrap(), content); let requests = requests.lock().unwrap(); let gets = requests.iter().filter(|r| r.starts_with("GET")).count(); @@ -646,6 +775,9 @@ mod tests { #[test] fn the_planned_number_of_connections_is_used() { + if !supports_xattrs() { + return; + } let content = body(16); let (base, requests) = test_server::serve_with( vec![("/file".into(), content.clone())], @@ -731,6 +863,9 @@ mod tests { #[test] fn a_server_that_refuses_head_still_downloads_in_segments() { + if !supports_xattrs() { + return; + } let content = body(16); let (base, requests) = test_server::serve_with( vec![("/file".into(), content.clone())], @@ -787,7 +922,10 @@ mod tests { } #[test] - fn an_interrupted_segmented_download_resumes() { + fn repeated_interruptions_with_progress_complete() { + if !supports_xattrs() { + return; + } let content = body(16); let interrupted = test_server::serve_with( vec![("/file".into(), content.clone())], @@ -805,27 +943,15 @@ mod tests { .output(out.to_string_lossy()) .overwrite(OverwriteMode::Skip) .execute(); - assert!(first.is_err(), "the interrupted transfer must fail"); - - let segments = read_resume_segments(&out).expect("resume state kept after failure"); - assert_eq!(segments.len(), 4); - assert!( - segments.iter().any(|s| s.written > 0), - "the interruption left progress behind: {segments:?}" - ); - - let healthy = test_server::serve(vec![("/file".into(), content.clone())]); - let target = format!("{healthy}/file"); - Download::new(&target) - .output(out.to_string_lossy()) - .overwrite(OverwriteMode::Skip) - .execute() - .unwrap(); + first.unwrap(); assert_eq!(std::fs::read(&out).unwrap(), content); } #[test] fn a_mixed_state_resume_fetches_only_what_is_missing() { + if !supports_xattrs() { + return; + } let content = body(32); let dir = tempfile::tempdir().unwrap(); let out = dir.path().join("file"); @@ -863,7 +989,7 @@ mod tests { &ResumeInfo { downloaded: current, total: content.len() as u64, - etag: None, + etag: Some("\"test-etag\"".into()), last_modified: None, segments: Some(segments), }, @@ -928,6 +1054,9 @@ mod tests { #[test] fn a_chunk_interrupted_twice_completes_on_its_final_attempt() { + if !supports_xattrs() { + return; + } let content = body(16); let (base, requests) = test_server::serve_with( vec![("/file".into(), content.clone())], @@ -992,6 +1121,9 @@ mod tests { #[test] fn a_resume_state_from_a_changed_resource_is_discarded() { + if !supports_xattrs() { + return; + } let stale = body(16); let dir = tempfile::tempdir().unwrap(); let out = dir.path().join("file"); @@ -1020,7 +1152,7 @@ mod tests { &ResumeInfo { downloaded: segments[0].written, total: stale.len() as u64, - etag: None, + etag: Some("\"test-etag\"".into()), last_modified: None, segments: Some(segments), }, diff --git a/crates/soar-dl/src/test_server.rs b/crates/soar-dl/src/test_server.rs index e302a4d5..46587f61 100644 --- a/crates/soar-dl/src/test_server.rs +++ b/crates/soar-dl/src/test_server.rs @@ -9,8 +9,10 @@ use std::{ }; /// How the server behaves where a download cares. -#[derive(Default, Clone)] +#[derive(Clone)] pub struct ServeOptions { + /// ETag returned with responses. `None` omits the header. + pub etag: Option, /// Send no `Accept-Ranges` header and answer range requests with the /// whole body, as a server without range support would. pub ignore_ranges: bool, @@ -33,6 +35,20 @@ pub struct ServeOptions { pub refuse_head: bool, } +impl Default for ServeOptions { + fn default() -> Self { + Self { + etag: Some("\"test-etag\"".into()), + ignore_ranges: false, + lie_about_ranges: false, + close_after_bytes: None, + truncate_periodic: None, + oversend: None, + refuse_head: false, + } + } +} + /// Serves each body in `routes` at its path until the test ends, and returns /// the base URL. `Range` requests are honored with a 206, other paths are 404. pub fn serve(routes: Vec<(String, Vec)>) -> String { @@ -40,19 +56,23 @@ pub fn serve(routes: Vec<(String, Vec)>) -> String { } /// Serves under `options` and reports every request with its `Range` header, -/// in arrival order. +/// in the order their headers are read. pub fn serve_with( routes: Vec<(String, Vec)>, options: ServeOptions, ) -> (String, Arc>>) { let listener = TcpListener::bind("127.0.0.1:0").unwrap(); let addr = listener.local_addr().unwrap(); - let routes: HashMap<_, _> = routes.into_iter().collect(); + let routes: Arc> = Arc::new(routes.into_iter().collect()); + let options = Arc::new(options); let requests: Arc>> = Arc::new(Mutex::new(Vec::new())); let recorded = requests.clone(); thread::spawn(move || { for stream in listener.incoming().flatten() { - respond(stream, &routes, &options, &recorded); + let routes = routes.clone(); + let options = options.clone(); + let recorded = recorded.clone(); + thread::spawn(move || respond(stream, &routes, &options, &recorded)); } }); (format!("http://{addr}"), requests) @@ -74,15 +94,18 @@ fn respond( let path = parts.next().unwrap_or_default().to_string(); let mut range: Option<(usize, Option)> = None; + let mut if_range = None; loop { let mut line = String::new(); if reader.read_line(&mut line).is_err() || line.trim().is_empty() { break; } - if let Some((name, value)) = line.split_once(':') - && name.eq_ignore_ascii_case("range") - { - range = value.trim().strip_prefix("bytes=").and_then(parse_range); + if let Some((name, value)) = line.split_once(':') { + if name.eq_ignore_ascii_case("range") { + range = value.trim().strip_prefix("bytes=").and_then(parse_range); + } else if name.eq_ignore_ascii_case("if-range") { + if_range = Some(value.trim().to_string()); + } } } requests @@ -95,7 +118,11 @@ fn respond( return; } - let range_honored = !options.ignore_ranges && !options.lie_about_ranges; + let range_honored = !options.ignore_ranges + && !options.lie_about_ranges + && if_range + .as_deref() + .is_none_or(|tag| options.etag.as_deref() == Some(tag) && !tag.starts_with("W/")); let (status, extra, body) = match routes.get(&path) { None => ("404 Not Found", String::new(), &b""[..]), Some(body) => { @@ -125,13 +152,16 @@ fn respond( .filter(|_| method == "GET") .and_then(|(period, within)| { let (start, _) = range?; - (start % period <= within).then_some(within) + (period != 0 && start % period <= within).then_some(within) }); let mut head = format!( "HTTP/1.1 {status}\r\nContent-Length: {}\r\n{extra}", body.len() + overshoot ); + if let Some(etag) = &options.etag { + head.push_str(&format!("ETag: {etag}\r\n")); + } if !options.ignore_ranges || options.lie_about_ranges { head.push_str("Accept-Ranges: bytes\r\n"); } @@ -164,3 +194,27 @@ fn parse_range(value: &str) -> Option<(usize, Option)> { let (start, end) = value.trim().split_once('-')?; Some((start.trim().parse().ok()?, end.trim().parse().ok())) } + +#[cfg(test)] +mod tests { + use super::*; + use crate::http::Http; + + #[test] + fn if_range_requires_a_matching_strong_etag() { + let base = serve(vec![("/file".into(), b"abcdef".to_vec())]); + let url = format!("{base}/file"); + assert_eq!( + Http::fetch_range(&url, 0, 2, Some("\"test-etag\""), false) + .unwrap() + .status(), + 206 + ); + assert_eq!( + Http::fetch_range(&url, 0, 2, Some("\"other\""), false) + .unwrap() + .status(), + 200 + ); + } +} diff --git a/crates/soar-operations/src/run.rs b/crates/soar-operations/src/run.rs index a95751ee..58be20b2 100644 --- a/crates/soar-operations/src/run.rs +++ b/crates/soar-operations/src/run.rs @@ -253,6 +253,7 @@ pub async fn prepare_run( &output_path, &cache_dir, no_verify, + config.download_concurrency.unwrap_or(8), progress_callback, )?; @@ -298,11 +299,13 @@ fn download_to_cache( output_path: &Path, cache_bin: &Path, no_verify: bool, + connections: usize, progress_callback: Arc, ) -> SoarResult<()> { if let Some(ref url) = package.ghcr_blob { let cb = progress_callback.clone(); let mut dl = OciDownload::new(url.as_str()) + .connections(connections) .output(output_path.to_string_lossy()) .overwrite(OverwriteMode::Force); dl = dl.progress(move |p| { @@ -313,6 +316,7 @@ fn download_to_cache( let extract_dir = get_extract_dir(cache_bin); let cb = progress_callback.clone(); let mut dl = Download::new(&package.download_url) + .connections(connections) .output(output_path.to_string_lossy()) .overwrite(OverwriteMode::Force) .extract(true) diff --git a/docs/configuration.md b/docs/configuration.md index 83312725..2e32c79c 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -32,10 +32,10 @@ Control where soar stores its data. | Configuration Option | Type | Default | Description | |---------------------|------|---------|-------------| | `parallel` | Boolean | `true` | Enable parallel downloads | -| `parallel_limit` | Integer | `4` | Max parallel downloads (1-16) | -| `ghcr_concurrency` | Integer | `8` | Max GHCR concurrent requests (1-32) | -| `download_concurrency` | Integer | `8` | Connections per download; more than one fetches a range-capable server's file in parallel segments | -| `search_limit` | Integer | `20` | Max search results (5-100) | +| `parallel_limit` | Integer | `4` | Max parallel downloads; must be positive | +| `ghcr_concurrency` | Integer | `8` | Max parallel GHCR layer downloads; must be positive | +| `download_concurrency` | Integer | `8` | Connections per HTTP or OCI blob download; must be positive. More than one fetches parallel segments when the server supports ranges and supplies a strong ETag. OCI manifest layers use separate single-stream downloads | +| `search_limit` | Integer | `20` | Max search results; must be positive | | `cross_repo_updates` | Boolean | `false` | Allow cross-repo updates (not implemented) | ### Package Installation diff --git a/docs/download.md b/docs/download.md index 97163e89..eeff5dfb 100644 --- a/docs/download.md +++ b/docs/download.md @@ -108,7 +108,7 @@ Filters narrow down the available assets. If multiple assets remain after filter |--------|-------------| | `--skip-existing` | Skip download if the file already exists | | `--force-overwrite` | Overwrite existing files without prompting | -| `--connections` | Number of connections used per download (default: 8). More than one fetches a range-capable server's file in parallel segments, which lifts a server's per-connection speed cap; `1` streams a single response | +| `--connections` | Positive number of connections per HTTP or OCI blob download (defaults to `download_concurrency`, or 8). More than one fetches parallel segments when the server supports ranges and supplies a strong ETag; `1` streams a single response. Resuming also requires a strong ETag. OCI manifest layers use separate single-stream downloads | ## Download Sources From 3e223455d495625384e69f8055b7bd64874e69b9 Mon Sep 17 00:00:00 2001 From: Rabindra Dhakal Date: Wed, 4 Nov 2026 19:59:14 +0545 Subject: [PATCH 3/5] fix(dl): validate resume plan before probing --- crates/soar-dl/src/download.rs | 71 ++++++++++++++++++++++------------ 1 file changed, 47 insertions(+), 24 deletions(-) diff --git a/crates/soar-dl/src/download.rs b/crates/soar-dl/src/download.rs index 8d9cdbb6..9cfe7d74 100644 --- a/crates/soar-dl/src/download.rs +++ b/crates/soar-dl/src/download.rs @@ -495,6 +495,10 @@ impl Download { if let Some(info) = &resume_info && let Some(segments) = &info.segments { + let Some(resumed) = segmented::plan_from_resume(info.total, segments) else { + warn!("segmented resume plan is invalid, restarting download"); + return self.download_to_file(path, None, remote); + }; let remote = remote.or_else(|| { Http::head(&self.url) .ok() @@ -511,30 +515,22 @@ impl Download { .as_ref() .and_then(|remote| remote.size) .is_some_and(|size| size != info.total); - return match segmented::plan_from_resume(info.total, segments) { - Some(resumed) - if !size_changed - && !etag_changed - && (resumed.current < info.total - || remote.as_ref().and_then(|r| r.etag.as_deref()) - == info.etag.as_deref()) => - { - debug!(offset = resumed.current, "resuming segmented download"); - self.download_segmented( - path, - info.total, - resumed.ranges, - resumed.written, - info.etag.as_deref(), - ) - } - _ => { - warn!( - "segmented resume state does not describe this resource, restarting download" - ); - self.download_to_file(path, None, remote) - } - }; + if !size_changed + && !etag_changed + && (resumed.current < info.total + || remote.as_ref().and_then(|r| r.etag.as_deref()) == info.etag.as_deref()) + { + debug!(offset = resumed.current, "resuming segmented download"); + return self.download_segmented( + path, + info.total, + resumed.ranges, + resumed.written, + info.etag.as_deref(), + ); + } + warn!("remote resource changed or could not be verified, restarting download"); + return self.download_to_file(path, None, remote); } if resume_info.is_some() { @@ -1054,6 +1050,33 @@ mod tests { } } + #[test] + fn invalid_segment_plan_restarts_without_a_head_request() { + let (base, requests) = + crate::test_server::serve_with(vec![("/file".into(), body())], Default::default()); + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("file"); + std::fs::write(&path, vec![42; body().len()]).unwrap(); + let info = ResumeInfo { + downloaded: 8000, + total: body().len() as u64, + etag: Some("\"test-etag\"".into()), + last_modified: None, + segments: Some(vec![SegmentProgress { + start: 1, + end: body().len() as u64 - 1, + written: 8000, + }]), + }; + Download::new(format!("{base}/file")) + .connections(1) + .download_to_file(&path, Some(info), None) + .unwrap(); + assert_eq!(std::fs::read(path).unwrap(), body()); + let requests = requests.lock().unwrap(); + assert_eq!(requests.as_slice(), ["GET /file range=None"]); + } + #[test] fn weak_single_resume_restarts() { let base = crate::test_server::serve(vec![("/file".into(), body())]); From e415cf3898dd3a3440b41a5ae7875781d6bba2fe Mon Sep 17 00:00:00 2001 From: Rabindra Dhakal Date: Wed, 4 Nov 2026 20:25:28 +0545 Subject: [PATCH 4/5] fix(dl): address segmented download review findings --- Cargo.lock | 7 + crates/soar-cli/src/cli.rs | 27 +- crates/soar-core/src/package/install.rs | 1 + crates/soar-dl/Cargo.toml | 1 + crates/soar-dl/src/download.rs | 172 ++++++++- crates/soar-dl/src/http.rs | 10 +- crates/soar-dl/src/oci.rs | 136 ++++++- crates/soar-dl/src/segmented.rs | 490 +++++++++++++++++++++++- crates/soar-dl/src/utils.rs | 16 + crates/soar-dl/src/xattr.rs | 43 ++- 10 files changed, 848 insertions(+), 55 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 51110719..2297fb18 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -989,6 +989,12 @@ version = "1.10.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6dbf3de79e51f3d586ab4cb9d5c3e2c14aa28ed23d180cf89b4df0454a69cc87" +[[package]] +name = "httpdate" +version = "1.0.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "df3b46402a9d5adb4c86a0cf463f42e19994e3ee891101b1841f30a545cb49a9" + [[package]] name = "hybrid-array" version = "0.4.15" @@ -2289,6 +2295,7 @@ version = "0.13.0" dependencies = [ "compak", "fast-glob", + "httpdate", "miette", "nix", "percent-encoding", diff --git a/crates/soar-cli/src/cli.rs b/crates/soar-cli/src/cli.rs index 32d311a8..c7e0feb1 100644 --- a/crates/soar-cli/src/cli.rs +++ b/crates/soar-cli/src/cli.rs @@ -621,17 +621,22 @@ mod tests { .is_err() ); } - for value in ["1", "8"] { - assert!( - Args::try_parse_from([ - "soar", - "download", - "--connections", - value, - "https://example.com/file" - ]) - .is_ok() - ); + for (value, expected) in [("1", 1), ("8", 8)] { + let args = Args::try_parse_from([ + "soar", + "download", + "--connections", + value, + "https://example.com/file", + ]) + .unwrap(); + let Commands::Download { + connections, .. + } = args.command + else { + panic!("expected download command"); + }; + assert_eq!(connections, Some(expected)); } } } diff --git a/crates/soar-core/src/package/install.rs b/crates/soar-core/src/package/install.rs index 26342bf6..5491b1ef 100644 --- a/crates/soar-core/src/package/install.rs +++ b/crates/soar-core/src/package/install.rs @@ -1090,6 +1090,7 @@ impl PackageInstaller { let mut dl = OciDownload::new(url.as_str()) .output(output_path.to_string_lossy()) .parallel(self.config.ghcr_concurrency.unwrap_or(8)) + .connections(self.config.download_concurrency.unwrap_or(8)) .overwrite(OverwriteMode::Skip); if let Some(ref cb) = self.progress_callback { diff --git a/crates/soar-dl/Cargo.toml b/crates/soar-dl/Cargo.toml index 294ce107..8893852c 100644 --- a/crates/soar-dl/Cargo.toml +++ b/crates/soar-dl/Cargo.toml @@ -13,6 +13,7 @@ categories.workspace = true [dependencies] compak = { workspace = true } fast-glob = { workspace = true } +httpdate = "1" miette = { workspace = true } nix = { workspace = true } percent-encoding = { workspace = true } diff --git a/crates/soar-dl/src/download.rs b/crates/soar-dl/src/download.rs index 9cfe7d74..90cffc49 100644 --- a/crates/soar-dl/src/download.rs +++ b/crates/soar-dl/src/download.rs @@ -22,9 +22,10 @@ use crate::{ segmented, types::{OverwriteMode, Progress, ResumeInfo, SegmentProgress}, utils::{ - filename_from_header, filename_from_url, open_existing, open_output, resolve_output_path, + filename_from_header, filename_from_url, open_existing, open_output, open_untruncated, + resolve_output_path, }, - xattr::{read_usable_resume, remove_resume, write_resume}, + xattr::{finish_resume, read_usable_resume, resume_bytes, write_resume, write_resume_file}, }; /// Connections one download uses by default. Where one stream is throttled @@ -364,9 +365,7 @@ impl Download { file.set_permissions(Permissions::from_mode(0o755))?; } - if let Err(err) = remove_resume(&output_path) { - warn!(path = %output_path.display(), %err, "failed to clear resume state"); - } + finish_resume(&file)?; // Extraction is driven by what the file actually is, not by what the // caller guessed it would be. compak detects by magic number, so an @@ -499,6 +498,10 @@ impl Download { warn!("segmented resume plan is invalid, restarting download"); return self.download_to_file(path, None, remote); }; + if self.connections == 1 { + debug!("restarting segmented download in single-stream mode"); + return self.download_single(path, None); + } let remote = remote.or_else(|| { Http::head(&self.url) .ok() @@ -615,21 +618,19 @@ impl Download { ), }; let file = if current == 0 { - if path.exists() { - // Replace old offsets before truncating an existing allocation. - if let Err(err) = write_resume(path, &info) { - if read_usable_resume(path).is_some() { - return Err(err.into()); - } - warn!(%err, "filesystem cannot store resume state, falling back to single stream"); - return self.download_single(path, None); + let file = open_untruncated(path)?; + if let Err(err) = write_resume_file(&file, &info) { + // Never truncate a file whose old offsets may still be stored. + match resume_bytes(&file) { + Ok(None) => {} + Err(read_err) if read_err.kind() == std::io::ErrorKind::Unsupported => {} + _ => return Err(err.into()), } - } - let file = open_output(path, None)?; - if let Err(err) = write_resume(path, &info) { warn!(%err, "filesystem cannot store resume state, falling back to single stream"); return self.download_single(path, None); } + file.sync_all()?; + file.set_len(0)?; file.set_len(total)?; file } else { @@ -643,11 +644,6 @@ impl Download { file }; - if let Err(err) = write_resume(path, &info) { - warn!(%err, "filesystem cannot store resume state, falling back to single stream"); - return self.download_single(path, None); - } - let result = segmented::run(segmented::Job { url: &self.url, ghcr_blob: self.ghcr_blob, @@ -1050,6 +1046,40 @@ mod tests { } } + #[test] + fn single_connection_restarts_segmented_resume_as_one_stream() { + let (base, requests) = + crate::test_server::serve_with(vec![("/file".into(), body())], Default::default()); + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("file"); + std::fs::write(&path, vec![42; body().len()]).unwrap(); + let info = ResumeInfo { + downloaded: 8000, + total: body().len() as u64, + etag: Some("\"test-etag\"".into()), + last_modified: None, + segments: Some(vec![SegmentProgress { + start: 0, + end: body().len() as u64 - 1, + written: 8000, + }]), + }; + if write_resume(&path, &info).is_err() { + eprintln!("skipping: filesystem lacks user xattrs"); + return; + } + Download::new(format!("{base}/file")) + .connections(1) + .download_to_file(&path, Some(info), None) + .unwrap(); + assert_eq!(std::fs::read(&path).unwrap(), body()); + assert_eq!( + requests.lock().unwrap().as_slice(), + ["GET /file range=None"] + ); + assert!(read_usable_resume(&path).is_none()); + } + #[test] fn invalid_segment_plan_restarts_without_a_head_request() { let (base, requests) = @@ -1077,6 +1107,54 @@ mod tests { assert_eq!(requests.as_slice(), ["GET /file range=None"]); } + #[test] + fn partial_response_rejects_changed_etag() { + let info = ResumeInfo { + downloaded: 8, + total: 20, + etag: Some("\"old\"".into()), + last_modified: None, + segments: None, + }; + let (base, _) = crate::test_server::serve_with( + vec![("/file".into(), vec![0; 20])], + crate::test_server::ServeOptions { + etag: Some("\"new\"".into()), + ..Default::default() + }, + ); + let response = Http::fetch_range(&format!("{base}/file"), 8, 19, None, false).unwrap(); + assert!(matches!( + check_partial_response(&response, &info), + Err(DownloadError::ResumeMismatch) + )); + } + + #[test] + fn fresh_download_without_strong_etag_uses_one_stream() { + let data = vec![7; segmented::MIN_SEGMENTED_TOTAL as usize]; + for etag in [None, Some("W/\"weak\"".to_string())] { + let (base, requests) = crate::test_server::serve_with( + vec![("/file".into(), data.clone())], + crate::test_server::ServeOptions { + etag, + ..Default::default() + }, + ); + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("file"); + Download::new(format!("{base}/file")) + .connections(2) + .download_to_file(&path, None, None) + .unwrap(); + assert_eq!(std::fs::read(path).unwrap(), data); + assert_eq!( + requests.lock().unwrap().as_slice(), + ["HEAD /file range=None", "GET /file range=None"] + ); + } + } + #[test] fn weak_single_resume_restarts() { let base = crate::test_server::serve(vec![("/file".into(), body())]); @@ -1184,6 +1262,58 @@ mod tests { assert!(saved.segments.unwrap().iter().all(|s| s.written == 0)); } + #[test] + fn failed_segmented_allocation_keeps_zero_marker() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("output"); + std::fs::write(&path, b"old data").unwrap(); + let old = ResumeInfo { + downloaded: 1, + total: 8, + etag: Some("\"old\"".into()), + last_modified: None, + segments: Some(vec![SegmentProgress { + start: 0, + end: 7, + written: 1, + }]), + }; + if write_resume(&path, &old).is_err() { + eprintln!("skipping: filesystem lacks user xattrs"); + return; + } + let dl = Download::new("http://127.0.0.1/unused").connections(2); + assert!( + dl.download_segmented( + &path, + u64::MAX, + vec![(0, u64::MAX - 1)], + vec![0], + Some("\"new\"") + ) + .is_err() + ); + let saved = crate::xattr::read_resume(&path).unwrap(); + assert_eq!(saved.downloaded, 0); + assert_eq!(saved.segments.unwrap()[0].written, 0); + assert_eq!(std::fs::metadata(&path).unwrap().len(), 0); + } + + #[test] + fn segmented_initialization_refuses_symlink_without_touching_target() { + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("target"); + let path = dir.path().join("output"); + std::fs::write(&target, b"keep me").unwrap(); + std::os::unix::fs::symlink(&target, &path).unwrap(); + let dl = Download::new("http://127.0.0.1/unused").connections(2); + assert!( + dl.download_segmented(&path, 16, vec![(0, 15)], vec![0], Some("\"tag\"")) + .is_err() + ); + assert_eq!(std::fs::read(&target).unwrap(), b"keep me"); + } + #[test] fn a_symlink_planted_at_the_output_is_not_followed() { let base = crate::test_server::serve(vec![("/file".into(), body())]); diff --git a/crates/soar-dl/src/http.rs b/crates/soar-dl/src/http.rs index 8d5d3d30..9ee3b493 100644 --- a/crates/soar-dl/src/http.rs +++ b/crates/soar-dl/src/http.rs @@ -162,7 +162,8 @@ impl Http { /// The request carries `Range: bytes={start}-{end}` with both ends /// inclusive. A server that answers `200` ignored the range; callers /// surface that as [`DownloadError::RangeUnsupported`] so the download can - /// fall back to a single stream. + /// fall back to a single stream. HTTP error statuses are returned as + /// responses so callers can inspect retry headers. pub(crate) fn fetch_range( url: &str, start: u64, @@ -185,7 +186,12 @@ impl Http { req = req.header("If-Range", tag); } - let result = req.call().map_err(|err| DownloadError::request(url, err)); + let result = req + .config() + .http_status_as_error(false) + .build() + .call() + .map_err(|err| DownloadError::request(url, err)); if let Ok(ref resp) = result { Self::log_response_headers(resp, "GET"); } diff --git a/crates/soar-dl/src/oci.rs b/crates/soar-dl/src/oci.rs index 963f06f7..d38b2100 100644 --- a/crates/soar-dl/src/oci.rs +++ b/crates/soar-dl/src/oci.rs @@ -967,8 +967,9 @@ fn download_layer_impl( "downloading layer" ); - let mut resume_info = read_usable_resume(path) - .filter(|info| info.etag.as_deref().and_then(strong_etag).is_some()); + let mut resume_info = read_usable_resume(path).filter(|info| { + info.segments.is_none() && info.etag.as_deref().and_then(strong_etag).is_some() + }); let resp = loop { let mut req = SHARED_AGENT.get(&url).header(AUTHORIZATION, "Bearer QQ=="); if let Some(info) = &resume_info { @@ -1019,6 +1020,10 @@ fn download_layer_impl( }; let resume_from = resume_info.as_ref().map(|info| info.downloaded); + if resume_from.is_none() { + // Invalidate the old checkpoint before truncating or writing the file. + remove_resume(path)?; + } let mut file = open_output(path, resume_from)?; if let Some(resume_offset) = resume_from { @@ -1035,7 +1040,8 @@ fn download_layer_impl( .headers() .get(ETAG) .and_then(|h| h.to_str().ok()) - .map(String::from); + .map(String::from) + .or_else(|| resume_info.as_ref().and_then(|info| info.etag.clone())); let mut reader = resp.into_body().into_reader(); let mut buffer = [0u8; 8192]; let mut layer_downloaded = resume_from.unwrap_or(0); @@ -1552,8 +1558,132 @@ mod tests { } } + #[test] + fn segmented_layer_checkpoint_restarts_and_clears_stale_state() { + let bytes = vec![7u8; 50_000]; + let layer = layer("one", &bytes); + let (base, requests) = crate::test_server::serve_with( + vec![( + format!("/v2/org/repo/blobs/{}", layer.digest), + bytes.clone(), + )], + crate::test_server::ServeOptions { + close_after_bytes: Some(10_000), + ..Default::default() + }, + ); + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("one"); + std::fs::write(&path, vec![0u8; bytes.len()]).unwrap(); + let info = ResumeInfo { + downloaded: 20_000, + total: bytes.len() as u64, + etag: Some("\"test-etag\"".into()), + last_modified: None, + segments: Some(vec![crate::types::SegmentProgress { + start: 25_000, + end: 49_999, + written: 20_000, + }]), + }; + if write_resume(&path, &info).is_err() { + eprintln!("skipping: filesystem lacks user xattrs"); + return; + } + let dl = OciDownload::new("org/repo:tag").api(format!("{base}/v2")); + assert!( + dl.download_layer(&layer, &path, &mut 0, layer.size) + .is_err() + ); + assert!(crate::xattr::read_resume(&path).is_none()); + assert!( + requests + .lock() + .unwrap() + .iter() + .all(|r| r.contains("range=None")) + ); + } + + #[test] + fn resumed_layer_checkpoint_keeps_etag_when_response_omits_it() { + use std::{ + io::{BufRead, BufReader}, + net::TcpListener, + }; + + let bytes = vec![3u8; 3 * 1024 * 1024]; + let layer = layer("one", &bytes); + let offset = 100_000; + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("one"); + std::fs::write(&path, &bytes[..offset]).unwrap(); + let info = ResumeInfo { + downloaded: offset as u64, + total: bytes.len() as u64, + etag: Some("\"test-etag\"".into()), + last_modified: None, + segments: None, + }; + if write_resume(&path, &info).is_err() { + eprintln!("skipping: filesystem lacks user xattrs"); + return; + } + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let addr = listener.local_addr().unwrap(); + let total = bytes.len(); + let server = thread::spawn(move || { + let (mut stream, _) = listener.accept().unwrap(); + let mut reader = BufReader::new(stream.try_clone().unwrap()); + let mut headers = String::new(); + loop { + let mut line = String::new(); + reader.read_line(&mut line).unwrap(); + if line == "\r\n" || line.is_empty() { + break; + } + headers.push_str(&line.to_ascii_lowercase()); + } + assert!( + headers.contains(&format!("range: bytes={offset}-")), + "{headers}" + ); + assert!(headers.contains("if-range: \"test-etag\""), "{headers}"); + write!(stream, "HTTP/1.1 206 Partial Content\r\nContent-Range: bytes {offset}-{}/{total}\r\nContent-Length: {}\r\nConnection: close\r\n\r\n", total - 1, total - offset).unwrap(); + stream + .write_all(&bytes[offset..offset + 1_200_000]) + .unwrap(); + }); + let dl = OciDownload::new("org/repo:tag").api(format!("http://{addr}/v2")); + assert!( + dl.download_layer(&layer, &path, &mut 0, layer.size) + .is_err() + ); + server.join().unwrap(); + let checkpoint = crate::xattr::read_resume(&path).unwrap(); + assert!(checkpoint.downloaded >= 1024 * 1024); + assert_eq!(checkpoint.etag.as_deref(), Some("\"test-etag\"")); + assert!(checkpoint.segments.is_none()); + } + #[test] fn direct_blob_connections_control_segmentation() { + let probe = tempfile::NamedTempFile::new().unwrap(); + if write_resume( + probe.path(), + &ResumeInfo { + downloaded: 0, + total: 0, + etag: None, + last_modified: None, + segments: None, + }, + ) + .is_err() + { + eprintln!("skipping: filesystem lacks user xattrs"); + return; + } let bytes = vec![5u8; 8 * 1024 * 1024]; let digest = format!("sha256:{}", sha256_hex(&bytes)); for connections in [1, 2] { diff --git a/crates/soar-dl/src/segmented.rs b/crates/soar-dl/src/segmented.rs index 008d6885..13bc46eb 100644 --- a/crates/soar-dl/src/segmented.rs +++ b/crates/soar-dl/src/segmented.rs @@ -17,6 +17,7 @@ use std::{ atomic::{AtomicBool, AtomicU64, AtomicUsize, Ordering}, }, thread, + time::{Duration, Instant, SystemTime}, }; use tracing::{debug, trace}; @@ -25,7 +26,7 @@ use crate::{ error::DownloadError, http::Http, types::{Progress, ResumeInfo, SegmentProgress}, - xattr::write_resume, + xattr::write_resume_file, }; /// The smallest chunk a download is cut into. Below this, another connection @@ -53,6 +54,43 @@ const REPORT_INTERVAL: u64 = 1024 * 1024; /// leaves its resume state for the next run. const MAX_ATTEMPTS: u32 = 3; +/// A chunk also has a total failure limit, even when every attempt writes bytes. +const MAX_CHUNK_FAILURES: u32 = 32; + +const RETRY_BASE_DELAY: Duration = Duration::from_millis(100); +const RETRY_MAX_DELAY: Duration = Duration::from_secs(2); +const MAX_RETRY_AFTER: Duration = Duration::from_secs(60); + +struct AttemptError { + error: DownloadError, + retry_after: Option, +} + +impl From for AttemptError { + fn from(error: DownloadError) -> Self { + Self { + error, + retry_after: None, + } + } +} + +impl From for AttemptError { + fn from(error: std::io::Error) -> Self { + DownloadError::Io(error).into() + } +} + +fn retry_after(value: &str, now: SystemTime) -> Option { + let value = value.trim(); + if !value.is_empty() && value.bytes().all(|b| b.is_ascii_digit()) { + return Some(Duration::from_secs(value.parse().unwrap_or(u64::MAX))); + } + httpdate::parse_http_date(value) + .ok() + .map(|date| date.duration_since(now).unwrap_or_default()) +} + /// Cuts `total` bytes into contiguous, inclusive chunks: a queue of /// [`CHUNKS_PER_CONNECTION`] per connection, capped by the per-chunk minimum /// and [`MAX_CHUNKS`]. The last chunk absorbs the remainder. @@ -366,6 +404,7 @@ impl Worker<'_> { written: u64, ) -> Result<(), DownloadError> { let mut attempts = 0; + let mut failures = 0; let mut cursor = start + written; loop { if cursor > end { @@ -375,15 +414,33 @@ impl Worker<'_> { let previous_cursor = cursor; match self.fetch_and_write(index, start, end, &mut cursor) { Ok(()) => continue, - Err(err) => { + Err(attempt_error) => { + let AttemptError { + error: err, + retry_after, + } = attempt_error; if self.shared.cancel.load(Ordering::Relaxed) { return Err(err); } + failures += 1; if cursor > previous_cursor { attempts = 0; } - if err.is_transient() && attempts < MAX_ATTEMPTS { - debug!(chunk = index, attempt = attempts, %err, "chunk failed, retrying"); + if err.is_transient() + && attempts < MAX_ATTEMPTS + && failures < MAX_CHUNK_FAILURES + { + if retry_after.is_some_and(|delay| delay > MAX_RETRY_AFTER) { + return Err(err); + } + let delay = RETRY_BASE_DELAY + .saturating_mul(1 << attempts.saturating_sub(1)) + .min(RETRY_MAX_DELAY) + .max(retry_after.unwrap_or_default()); + debug!(chunk = index, failures, ?delay, %err, "chunk failed, retrying"); + if !self.wait_for_retry(delay) { + return Err(err); + } continue; } return Err(err); @@ -392,13 +449,27 @@ impl Worker<'_> { } } + fn wait_for_retry(&self, delay: Duration) -> bool { + let deadline = Instant::now() + delay; + loop { + if self.shared.cancel.load(Ordering::Relaxed) { + return false; + } + let remaining = deadline.saturating_duration_since(Instant::now()); + if remaining.is_zero() { + return true; + } + thread::sleep(remaining.min(Duration::from_millis(50))); + } + } + fn fetch_and_write( &mut self, index: usize, start: u64, end: u64, cursor: &mut u64, - ) -> Result<(), DownloadError> { + ) -> Result<(), AttemptError> { let resp = Http::fetch_range(self.url, *cursor, end, self.etag, self.ghcr_blob).map_err( |err| { match err { @@ -420,14 +491,33 @@ impl Worker<'_> { trace!(url = self.url, "server ignored the range request"); return Err(DownloadError::RangeUnsupported { url: self.url.to_string(), - }); + } + .into()); } - return Err(DownloadError::HttpError { - status, - url: self.url.to_string(), + let retry_after = resp + .headers() + .get(ureq::http::header::RETRY_AFTER) + .and_then(|value| value.to_str().ok()) + .and_then(|value| retry_after(value, SystemTime::now())); + return Err(AttemptError { + error: DownloadError::HttpError { + status, + url: self.url.to_string(), + }, + retry_after, }); } + if let Some(pinned) = self.etag.and_then(crate::http::strong_etag) + && let Some(actual) = resp.headers().get(ureq::http::header::ETAG) + && actual.as_bytes() != pinned.as_bytes() + { + return Err(DownloadError::RangeUnsupported { + url: self.url.to_string(), + } + .into()); + } + // A range that does not start where it was asked to start, or that // describes a different resource than the one being downloaded, // means the server's idea of the file has diverged from ours; refuse @@ -441,7 +531,8 @@ impl Worker<'_> { let Some((span, total)) = content_range else { return Err(DownloadError::RangeUnsupported { url: self.url.to_string(), - }); + } + .into()); }; let matches = span.split_once('-').and_then(|(start, finish)| { Some(( @@ -453,7 +544,8 @@ impl Worker<'_> { if !matches { return Err(DownloadError::RangeUnsupported { url: self.url.to_string(), - }); + } + .into()); } let mut reader = resp.into_body().into_reader(); @@ -474,17 +566,19 @@ impl Worker<'_> { return Err(DownloadError::Io(std::io::Error::new( std::io::ErrorKind::UnexpectedEof, "chunk closed before its range was complete", - ))); + )) + .into()); } if self.shared.cancel.load(Ordering::Relaxed) { return Err(DownloadError::Io(std::io::Error::new( std::io::ErrorKind::Interrupted, "cancelled", - ))); + )) + .into()); } self.file.write_all_at(&buffer[..n], *cursor)?; *cursor += n as u64; - self.shared.chunk_written[index].store(*cursor - start, Ordering::Relaxed); + self.shared.chunk_written[index].store(*cursor - start, Ordering::Release); let aggregate = self.shared.aggregate.fetch_add(n as u64, Ordering::Relaxed) + n as u64; self.maybe_report(aggregate); } @@ -522,7 +616,7 @@ impl Worker<'_> { SegmentProgress { start, end, - written: self.shared.chunk_written[index].load(Ordering::Relaxed), + written: self.shared.chunk_written[index].load(Ordering::Acquire), } }) .collect(); @@ -533,8 +627,14 @@ impl Worker<'_> { last_modified: None, segments: Some(segments), }; - if let Err(err) = write_resume(self.path, &info) { - trace!(%err, "failed to save resume checkpoint"); + // Snapshot the counters first, then flush their corresponding writes. + // Writes concurrent with the flush can be refetched on resume. + if let Err(err) = self.file.sync_data() { + trace!(%err, "failed to sync data for resume checkpoint"); + return; + } + if let Err(err) = write_resume_file(&self.file, &info) { + trace!(path = %self.path.display(), %err, "failed to save resume checkpoint"); } } } @@ -624,6 +724,294 @@ mod tests { assert_eq!(requests.lock().unwrap().len(), MAX_ATTEMPTS as usize); } + #[test] + fn retries_with_tiny_progress_are_bounded() { + let (base, requests) = test_server::serve_with( + vec![("/file".into(), vec![42; 100])], + ServeOptions { + close_after_bytes: Some(1), + ..Default::default() + }, + ); + let file = tempfile::NamedTempFile::new().unwrap(); + let url = format!("{base}/file"); + let result = run(Job { + url: &url, + ghcr_blob: false, + etag: None, + path: file.path(), + file: file.as_file(), + total: 100, + connections: 1, + ranges: vec![(0, 99)], + written: vec![0], + on_progress: None, + }); + assert!(result.is_err()); + assert_eq!(requests.lock().unwrap().len(), MAX_CHUNK_FAILURES as usize); + assert_eq!( + std::fs::read(file.path()).unwrap().len(), + MAX_CHUNK_FAILURES as usize + ); + } + + #[test] + fn contradictory_etag_is_rejected_before_writing() { + use std::io::{Read, Write}; + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let url = format!("http://{}", listener.local_addr().unwrap()); + std::thread::spawn(move || { + let (mut stream, _) = listener.accept().unwrap(); + let mut request = [0; 4096]; + let _ = stream.read(&mut request); + let _ = stream.write_all(b"HTTP/1.1 206 Partial Content\r\nETag: \"different\"\r\nContent-Range: bytes 0-4/5\r\nContent-Length: 5\r\nConnection: close\r\n\r\nabcde"); + }); + let file = tempfile::NamedTempFile::new().unwrap(); + let result = run(Job { + url: &url, + ghcr_blob: false, + etag: Some("\"pinned\""), + path: file.path(), + file: file.as_file(), + total: 5, + connections: 1, + ranges: vec![(0, 4)], + written: vec![0], + on_progress: None, + }); + assert!(matches!( + result, + Err(DownloadError::RangeUnsupported { .. }) + )); + assert_eq!(file.as_file().metadata().unwrap().len(), 0); + } + + #[test] + fn failed_data_sync_does_not_advance_checkpoint() { + if !supports_xattrs() { + return; + } + let path = tempfile::NamedTempFile::new().unwrap(); + let unsyncable = File::open("/dev/null").unwrap(); + assert!(unsyncable.sync_data().is_err()); + let shared = Arc::new(Shared { + cancel: AtomicBool::new(false), + aggregate: AtomicU64::new(REPORT_INTERVAL), + reported: Mutex::new(0), + chunk_written: vec![AtomicU64::new(REPORT_INTERVAL)].into_boxed_slice(), + ranges: Arc::new(vec![(0, REPORT_INTERVAL * 2 - 1)]), + cursor: AtomicUsize::new(0), + partial: Mutex::new(Vec::new()), + claimed: Mutex::new(vec![true]), + }); + let worker = Worker { + shared, + url: "", + ghcr_blob: false, + etag: None, + path: path.path(), + total: REPORT_INTERVAL * 2, + file: &unsyncable, + progress: None, + }; + worker.maybe_report(REPORT_INTERVAL); + assert!(crate::xattr::read_resume(path.path()).is_none()); + } + + #[test] + fn retry_wait_observes_cancellation() { + let shared = Arc::new(Shared { + cancel: AtomicBool::new(false), + aggregate: AtomicU64::new(0), + reported: Mutex::new(0), + chunk_written: Box::new([]), + ranges: Arc::new(Vec::new()), + cursor: AtomicUsize::new(0), + partial: Mutex::new(Vec::new()), + claimed: Mutex::new(Vec::new()), + }); + let other = shared.clone(); + let file = tempfile::NamedTempFile::new().unwrap(); + let worker = Worker { + shared, + url: "", + ghcr_blob: false, + etag: None, + path: file.path(), + total: 0, + file: file.as_file(), + progress: None, + }; + let thread = std::thread::spawn(move || { + std::thread::sleep(Duration::from_millis(50)); + other.cancel.store(true, Ordering::Relaxed); + }); + let started = Instant::now(); + assert!(!worker.wait_for_retry(Duration::from_secs(2))); + assert!(started.elapsed() < Duration::from_secs(1)); + thread.join().unwrap(); + } + + #[test] + fn rate_limit_retries_are_delayed() { + use std::io::{Read, Write}; + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let url = format!("http://{}", listener.local_addr().unwrap()); + let server = std::thread::spawn(move || { + let mut times = Vec::new(); + for _ in 0..MAX_ATTEMPTS { + let (mut stream, _) = listener.accept().unwrap(); + let mut request = [0; 4096]; + let _ = stream.read(&mut request); + times.push(Instant::now()); + stream.write_all(b"HTTP/1.1 429 Too Many Requests\r\nContent-Length: 0\r\nConnection: close\r\n\r\n").unwrap(); + } + times + }); + let file = tempfile::NamedTempFile::new().unwrap(); + let result = run(Job { + url: &url, + ghcr_blob: false, + etag: None, + path: file.path(), + file: file.as_file(), + total: 5, + connections: 1, + ranges: vec![(0, 4)], + written: vec![0], + on_progress: None, + }); + assert!(matches!( + result, + Err(DownloadError::HttpError { + status: 429, + .. + }) + )); + let times = server.join().unwrap(); + assert!(times[1].duration_since(times[0]) >= Duration::from_millis(90)); + assert!(times[2].duration_since(times[1]) >= Duration::from_millis(190)); + } + + #[test] + fn retry_after_parses_seconds_and_dates() { + let now = SystemTime::UNIX_EPOCH + Duration::from_secs(1_700_000_000); + assert_eq!(retry_after(" 12 ", now), Some(Duration::from_secs(12))); + let future = httpdate::fmt_http_date(now + Duration::from_secs(42)); + assert_eq!(retry_after(&future, now), Some(Duration::from_secs(42))); + let past = httpdate::fmt_http_date(now - Duration::from_secs(1)); + assert_eq!(retry_after(&past, now), Some(Duration::ZERO)); + assert_eq!(retry_after("invalid", now), None); + assert!(retry_after("9999999999999999999999999999", now).unwrap() > MAX_RETRY_AFTER); + } + + #[test] + fn server_retry_after_delays_next_range_request() { + use std::io::{Read, Write}; + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let url = format!("http://{}", listener.local_addr().unwrap()); + let server = std::thread::spawn(move || { + let mut times = Vec::new(); + for response in [ + "HTTP/1.1 429 Too Many Requests\r\nRetry-After: 1\r\nContent-Length: 0\r\nConnection: close\r\n\r\n", + "HTTP/1.1 206 Partial Content\r\nContent-Range: bytes 0-4/5\r\nContent-Length: 5\r\nConnection: close\r\n\r\nhello", + ] { + let (mut stream, _) = listener.accept().unwrap(); + let mut request = [0; 4096]; + stream.read(&mut request).unwrap(); + times.push(Instant::now()); + stream.write_all(response.as_bytes()).unwrap(); + } + times + }); + let file = tempfile::NamedTempFile::new().unwrap(); + run(Job { + url: &url, + ghcr_blob: false, + etag: None, + path: file.path(), + file: file.as_file(), + total: 5, + connections: 1, + ranges: vec![(0, 4)], + written: vec![0], + on_progress: None, + }) + .unwrap(); + let times = server.join().unwrap(); + assert!(times[1].duration_since(times[0]) >= Duration::from_secs(1)); + assert_eq!(std::fs::read(file.path()).unwrap(), b"hello"); + } + + #[test] + fn excessive_server_hint_returns_error_without_retry() { + use std::io::{Read, Write}; + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let url = format!("http://{}", listener.local_addr().unwrap()); + let server = std::thread::spawn(move || { + let (mut stream, _) = listener.accept().unwrap(); + let mut request = [0; 4096]; + stream.read(&mut request).unwrap(); + stream.write_all(b"HTTP/1.1 503 Service Unavailable\r\nRetry-After: 61\r\nContent-Length: 0\r\nConnection: close\r\n\r\n").unwrap(); + listener.set_nonblocking(true).unwrap(); + std::thread::sleep(Duration::from_millis(400)); + assert!( + matches!(listener.accept(), Err(e) if e.kind() == std::io::ErrorKind::WouldBlock) + ); + }); + let file = tempfile::NamedTempFile::new().unwrap(); + let result = run(Job { + url: &url, + ghcr_blob: false, + etag: None, + path: file.path(), + file: file.as_file(), + total: 5, + connections: 1, + ranges: vec![(0, 4)], + written: vec![0], + on_progress: None, + }); + assert!(matches!( + result, + Err(DownloadError::HttpError { + status: 503, + .. + }) + )); + server.join().unwrap(); + } + + #[test] + fn missing_response_remains_a_request_error() { + use std::io::Read; + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let url = format!("http://{}", listener.local_addr().unwrap()); + let server = std::thread::spawn(move || { + // Accept all retries and close without an HTTP response. + for _ in 0..MAX_ATTEMPTS { + let (mut stream, _) = listener.accept().unwrap(); + let mut request = [0; 4096]; + let _ = stream.read(&mut request); + } + }); + let file = tempfile::NamedTempFile::new().unwrap(); + let result = run(Job { + url: &url, + ghcr_blob: false, + etag: None, + path: file.path(), + file: file.as_file(), + total: 5, + connections: 1, + ranges: vec![(0, 4)], + written: vec![0], + on_progress: None, + }); + assert!(result.is_err()); + server.join().unwrap(); + } + #[test] fn incorrect_content_range_end_triggers_fallback() { use std::io::{Read, Write}; @@ -773,6 +1161,74 @@ mod tests { assert_eq!(gets, 24, "96 MiB over 24 queued chunks: {requests:?}"); } + #[test] + fn workers_issue_overlapping_range_requests() { + use std::{ + io::{BufRead, BufReader, Write}, + net::TcpListener, + }; + + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + listener.set_nonblocking(true).unwrap(); + let url = format!("http://{}", listener.local_addr().unwrap()); + let server = std::thread::spawn(move || { + let deadline = Instant::now() + Duration::from_secs(3); + let mut pending = Vec::new(); + while pending.len() < 2 && Instant::now() < deadline { + match listener.accept() { + Ok((stream, _)) => { + stream + .set_read_timeout(Some(Duration::from_secs(2))) + .unwrap(); + let mut reader = BufReader::new(stream.try_clone().unwrap()); + let mut range = String::new(); + loop { + let mut line = String::new(); + if reader.read_line(&mut line).unwrap() == 0 || line.trim().is_empty() { + break; + } + if line.to_ascii_lowercase().starts_with("range:") { + range = line; + } + } + pending.push((stream, range)); + } + Err(err) if err.kind() == std::io::ErrorKind::WouldBlock => { + std::thread::sleep(Duration::from_millis(5)); + } + Err(err) => panic!("accept failed: {err}"), + } + } + let overlapping = pending.len(); + for (mut stream, range) in pending { + let (span, content) = if range.contains("bytes=0-4") { + ("0-4", "abcde") + } else { + assert!(range.contains("bytes=5-9"), "unexpected range: {range}"); + ("5-9", "fghij") + }; + write!(stream, "HTTP/1.1 206 Partial Content\r\nContent-Range: bytes {span}/10\r\nContent-Length: 5\r\nConnection: close\r\n\r\n{content}").unwrap(); + } + overlapping + }); + let file = tempfile::NamedTempFile::new().unwrap(); + let result = run(Job { + url: &url, + ghcr_blob: false, + etag: None, + path: file.path(), + file: file.as_file(), + total: 10, + connections: 2, + ranges: vec![(0, 4), (5, 9)], + written: vec![0, 0], + on_progress: None, + }); + assert_eq!(server.join().unwrap(), 2, "requests did not overlap"); + result.unwrap(); + assert_eq!(std::fs::read(file.path()).unwrap(), b"abcdefghij"); + } + #[test] fn the_planned_number_of_connections_is_used() { if !supports_xattrs() { diff --git a/crates/soar-dl/src/utils.rs b/crates/soar-dl/src/utils.rs index bcd26492..56fc9861 100644 --- a/crates/soar-dl/src/utils.rs +++ b/crates/soar-dl/src/utils.rs @@ -122,6 +122,22 @@ pub(crate) fn open_output(path: &Path, resume_pos: Option) -> Result Result { + let file = OpenOptions::new() + .write(true) + .create(true) + .custom_flags(libc::O_NOFOLLOW) + .open(path)?; + if !file.metadata()?.is_file() { + return Err( + std::io::Error::other(format!("{} is not a regular file", path.display())).into(), + ); + } + Ok(file) +} + /// Opens an existing download output at `path` for in-place writes, refusing /// to follow a symlink planted there or to write into anything but a regular /// file. diff --git a/crates/soar-dl/src/xattr.rs b/crates/soar-dl/src/xattr.rs index dbe4fd92..836e9dbb 100644 --- a/crates/soar-dl/src/xattr.rs +++ b/crates/soar-dl/src/xattr.rs @@ -1,12 +1,33 @@ -use std::path::Path; +use std::{fs::File, path::Path}; use tracing::warn; use ureq::http::HeaderValue; +use xattr::FileExt as _; use crate::types::ResumeInfo; const XATTR_RESUME_KEY: &str = "user.soar.resume"; +pub(crate) fn resume_bytes(file: &File) -> std::io::Result>> { + file.get_xattr(XATTR_RESUME_KEY) +} + +pub(crate) fn write_resume_file(file: &File, info: &ResumeInfo) -> std::io::Result<()> { + file.set_xattr(XATTR_RESUME_KEY, &serde_json::to_vec(info)?) +} + +pub(crate) fn finish_resume(file: &File) -> std::io::Result<()> { + match resume_bytes(file) { + Ok(Some(_)) => { + file.sync_all()?; + file.remove_xattr(XATTR_RESUME_KEY) + } + Ok(None) => Ok(()), + Err(err) if err.kind() == std::io::ErrorKind::Unsupported => Ok(()), + Err(err) => Err(err), + } +} + /// Reads the `user.soar.resume` extended attribute from the given path and deserializes it into a `ResumeInfo`. /// /// Returns `Some(ResumeInfo)` if the attribute exists and contains valid JSON; returns `None` if the attribute is missing, cannot be read, or fails to deserialize. @@ -122,6 +143,26 @@ mod tests { } } + #[test] + fn descriptor_marker_follows_open_file_and_is_cleared_on_finish() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("partial"); + let moved = dir.path().join("moved"); + std::fs::write(&path, b"data").unwrap(); + let file = std::fs::OpenOptions::new().write(true).open(&path).unwrap(); + if write_resume_file(&file, &resume(0, 4, None)).is_err() { + eprintln!("skipping: filesystem lacks user xattrs"); + return; + } + std::fs::rename(&path, &moved).unwrap(); + std::fs::write(&path, b"replacement").unwrap(); + finish_resume(&file).unwrap(); + assert!(read_resume(&moved).is_none()); + assert_eq!(std::fs::read(&path).unwrap(), b"replacement"); + assert_eq!(std::fs::read(&moved).unwrap(), b"data"); + finish_resume(&file).unwrap(); + } + #[test] fn stale_resume_state_is_discarded() { let dir = tempfile::tempdir().unwrap(); From e355881e042aefe990cbdf518879175bb24861b1 Mon Sep 17 00:00:00 2001 From: Rabindra Dhakal Date: Wed, 4 Nov 2026 22:29:16 +0545 Subject: [PATCH 5/5] fix(dl): respect server cooldown across retries --- crates/soar-dl/src/error.rs | 15 +- crates/soar-dl/src/segmented.rs | 242 ++++++++++++++++++++++++++++---- crates/soar-dl/src/xattr.rs | 7 +- 3 files changed, 234 insertions(+), 30 deletions(-) diff --git a/crates/soar-dl/src/error.rs b/crates/soar-dl/src/error.rs index 38633273..636cb522 100644 --- a/crates/soar-dl/src/error.rs +++ b/crates/soar-dl/src/error.rs @@ -26,6 +26,18 @@ pub enum DownloadError { #[diagnostic(code(soar_dl::http_error))] HttpError { status: u16, url: String }, + /// The server requested a cooldown that this download could not honor. + /// Callers must not automatically retry without honoring the delay. + #[error( + "HTTP {status}: {url}: server requested a retry after {delay:?}; automatic retry stopped" + )] + #[diagnostic(code(soar_dl::retry_after))] + RetryAfter { + status: u16, + url: String, + delay: std::time::Duration, + }, + #[error(transparent)] #[diagnostic(code(soar_dl::io))] Io(#[from] std::io::Error), @@ -150,7 +162,8 @@ impl DownloadError { /// /// A body read that fails surfaces as `Io`. ureq hands its own errors /// there wrapped in an `io::Error`, the stall timeout among them, so a - /// wrapped ureq error counts as a network failure. + /// wrapped ureq error counts as a network failure. `RetryAfter` is not + /// transient for automatic retries because its cooldown remains outstanding. pub fn is_transient(&self) -> bool { match self { Self::Network { diff --git a/crates/soar-dl/src/segmented.rs b/crates/soar-dl/src/segmented.rs index 13bc46eb..c507b74f 100644 --- a/crates/soar-dl/src/segmented.rs +++ b/crates/soar-dl/src/segmented.rs @@ -186,6 +186,7 @@ enum Outcome { struct Shared { cancel: AtomicBool, + retry_deadline: Mutex>, /// Bytes written across all chunks. aggregate: AtomicU64, /// The aggregate the last checkpoint and progress report carried. @@ -243,6 +244,7 @@ pub(crate) fn run(job: Job) -> Result<(), DownloadError> { .collect(); let shared = Arc::new(Shared { cancel: AtomicBool::new(false), + retry_deadline: Mutex::new(None), aggregate: AtomicU64::new(initial), reported: Mutex::new(initial), chunk_written: job @@ -308,6 +310,15 @@ pub(crate) fn run(job: Job) -> Result<(), DownloadError> { } }); + if let Some(index) = outcomes + .iter() + .position(|outcome| matches!(outcome, Outcome::Failed(DownloadError::RetryAfter { .. }))) + { + if let Outcome::Failed(err) = outcomes.swap_remove(index) { + return Err(err); + } + } + // A server that ignored one range request is treated as ignoring them all; // the caller falls back to a single stream, which needs no ranges. if let Some(url) = outcomes.iter().find_map(|outcome| { @@ -358,7 +369,9 @@ impl Worker<'_> { // Cancellation is fallout from another worker's // failure; that worker's error is the one worth // reporting. - if worker.shared.cancel.load(Ordering::Relaxed) { + if worker.shared.cancel.load(Ordering::Relaxed) + && !matches!(err, DownloadError::RetryAfter { .. }) + { return Outcome::Cancelled; } worker.shared.cancel.store(true, Ordering::Relaxed); @@ -410,6 +423,9 @@ impl Worker<'_> { if cursor > end { return Ok(()); } + if !self.wait_for_shared_retry() { + return Ok(()); + } attempts += 1; let previous_cursor = cursor; match self.fetch_and_write(index, start, end, &mut cursor) { @@ -419,26 +435,56 @@ impl Worker<'_> { error: err, retry_after, } = attempt_error; - if self.shared.cancel.load(Ordering::Relaxed) { - return Err(err); - } failures += 1; if cursor > previous_cursor { attempts = 0; } - if err.is_transient() + let can_retry = err.is_transient() && attempts < MAX_ATTEMPTS && failures < MAX_CHUNK_FAILURES + && !self.shared.cancel.load(Ordering::Relaxed); + if let ( + DownloadError::HttpError { + status, + url, + }, + Some(delay), + ) = (&err, retry_after) + && !delay.is_zero() { - if retry_after.is_some_and(|delay| delay > MAX_RETRY_AFTER) { - return Err(err); + if delay > MAX_RETRY_AFTER || !can_retry { + return Err(DownloadError::RetryAfter { + status: *status, + url: url.clone(), + delay, + }); } + let deadline = Instant::now() + delay; + let mut shared = self.shared.retry_deadline.lock().unwrap(); + *shared = Some(shared.map_or(deadline, |current| current.max(deadline))); + } + if can_retry { let delay = RETRY_BASE_DELAY .saturating_mul(1 << attempts.saturating_sub(1)) .min(RETRY_MAX_DELAY) .max(retry_after.unwrap_or_default()); debug!(chunk = index, failures, ?delay, %err, "chunk failed, retrying"); if !self.wait_for_retry(delay) { + if let ( + DownloadError::HttpError { + status, + url, + }, + Some(delay), + ) = (&err, retry_after) + && !delay.is_zero() + { + return Err(DownloadError::RetryAfter { + status: *status, + url: url.clone(), + delay, + }); + } return Err(err); } continue; @@ -449,6 +495,20 @@ impl Worker<'_> { } } + fn wait_for_shared_retry(&self) -> bool { + loop { + let deadline = *self.shared.retry_deadline.lock().unwrap(); + match deadline { + Some(deadline) if deadline > Instant::now() => { + if !self.wait_for_retry(deadline.saturating_duration_since(Instant::now())) { + return false; + } + } + _ => return !self.shared.cancel.load(Ordering::Relaxed), + } + } + } + fn wait_for_retry(&self, delay: Duration) -> bool { let deadline = Instant::now() + delay; loop { @@ -588,9 +648,12 @@ impl Worker<'_> { /// [`REPORT_INTERVAL`] aggregate bytes; one worker wins that right per /// interval, so the xattr and the bar advance in step. fn maybe_report(&self, aggregate: u64) { - let mut reported = self.shared.reported.lock().unwrap(); - // Another worker may have reported more recent progress while this - // worker was waiting for the reporting lock. + let mut reported = match self.shared.reported.try_lock() { + Ok(reported) => reported, + Err(std::sync::TryLockError::WouldBlock) => return, + Err(std::sync::TryLockError::Poisoned(err)) => panic!("{err}"), + }; + // A previous reporter may have recorded more recent progress. let aggregate = self.shared.aggregate.load(Ordering::Relaxed).max(aggregate); if aggregate.saturating_sub(*reported) < REPORT_INTERVAL { return; @@ -796,6 +859,7 @@ mod tests { assert!(unsyncable.sync_data().is_err()); let shared = Arc::new(Shared { cancel: AtomicBool::new(false), + retry_deadline: Mutex::new(None), aggregate: AtomicU64::new(REPORT_INTERVAL), reported: Mutex::new(0), chunk_written: vec![AtomicU64::new(REPORT_INTERVAL)].into_boxed_slice(), @@ -818,10 +882,48 @@ mod tests { assert!(crate::xattr::read_resume(path.path()).is_none()); } + #[test] + fn reporting_does_not_block_other_workers() { + let shared = Arc::new(Shared { + cancel: AtomicBool::new(false), + retry_deadline: Mutex::new(None), + aggregate: AtomicU64::new(REPORT_INTERVAL), + reported: Mutex::new(0), + chunk_written: Box::new([]), + ranges: Arc::new(Vec::new()), + cursor: AtomicUsize::new(0), + partial: Mutex::new(Vec::new()), + claimed: Mutex::new(Vec::new()), + }); + let guard = shared.reported.lock().unwrap(); + let other = shared.clone(); + let (tx, rx) = std::sync::mpsc::channel(); + let thread = std::thread::spawn(move || { + let file = tempfile::NamedTempFile::new().unwrap(); + let worker = Worker { + shared: other, + url: "", + ghcr_blob: false, + etag: None, + path: file.path(), + total: REPORT_INTERVAL * 2, + file: file.as_file(), + progress: None, + }; + worker.maybe_report(REPORT_INTERVAL); + tx.send(()).unwrap(); + }); + let result = rx.recv_timeout(Duration::from_secs(1)); + drop(guard); + thread.join().unwrap(); + assert!(result.is_ok(), "reporter blocked on checkpoint lock"); + } + #[test] fn retry_wait_observes_cancellation() { let shared = Arc::new(Shared { cancel: AtomicBool::new(false), + retry_deadline: Mutex::new(None), aggregate: AtomicU64::new(0), reported: Mutex::new(0), chunk_written: Box::new([]), @@ -945,43 +1047,127 @@ mod tests { #[test] fn excessive_server_hint_returns_error_without_retry() { + use std::io::{Read, Write}; + for status in [429, 503] { + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let url = format!("http://{}", listener.local_addr().unwrap()); + let server = std::thread::spawn(move || { + let (mut stream, _) = listener.accept().unwrap(); + let mut request = [0; 4096]; + stream.read(&mut request).unwrap(); + write!(stream, "HTTP/1.1 {status} Error\r\nRetry-After: 61\r\nContent-Length: 0\r\nConnection: close\r\n\r\n").unwrap(); + listener.set_nonblocking(true).unwrap(); + std::thread::sleep(Duration::from_millis(400)); + assert!( + matches!(listener.accept(), Err(e) if e.kind() == std::io::ErrorKind::WouldBlock) + ); + }); + let file = tempfile::NamedTempFile::new().unwrap(); + let err = run(Job { + url: &url, + ghcr_blob: false, + etag: None, + path: file.path(), + file: file.as_file(), + total: 5, + connections: 1, + ranges: vec![(0, 4)], + written: vec![0], + on_progress: None, + }) + .unwrap_err(); + assert!( + matches!(err, DownloadError::RetryAfter { status: actual, delay, .. } if actual == status && delay == Duration::from_secs(61)) + ); + assert!(!err.is_transient()); + server.join().unwrap(); + } + } + + #[test] + fn cancellation_preserves_another_workers_cooldown() { use std::io::{Read, Write}; let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); let url = format!("http://{}", listener.local_addr().unwrap()); let server = std::thread::spawn(move || { - let (mut stream, _) = listener.accept().unwrap(); + let (mut first, _) = listener.accept().unwrap(); + let (mut second, _) = listener.accept().unwrap(); let mut request = [0; 4096]; - stream.read(&mut request).unwrap(); - stream.write_all(b"HTTP/1.1 503 Service Unavailable\r\nRetry-After: 61\r\nContent-Length: 0\r\nConnection: close\r\n\r\n").unwrap(); - listener.set_nonblocking(true).unwrap(); - std::thread::sleep(Duration::from_millis(400)); - assert!( - matches!(listener.accept(), Err(e) if e.kind() == std::io::ErrorKind::WouldBlock) - ); + first.read(&mut request).unwrap(); + second.read(&mut request).unwrap(); + first.write_all(b"HTTP/1.1 429 Error\r\nRetry-After: 10\r\nContent-Length: 0\r\nConnection: close\r\n\r\n").unwrap(); + std::thread::sleep(Duration::from_millis(200)); + second + .write_all(b"HTTP/1.1 416 Error\r\nContent-Length: 0\r\nConnection: close\r\n\r\n") + .unwrap(); }); let file = tempfile::NamedTempFile::new().unwrap(); - let result = run(Job { + let started = Instant::now(); + let err = run(Job { url: &url, ghcr_blob: false, etag: None, path: file.path(), file: file.as_file(), - total: 5, - connections: 1, - ranges: vec![(0, 4)], - written: vec![0], + total: 10, + connections: 2, + ranges: vec![(0, 4), (5, 9)], + written: vec![0, 0], on_progress: None, - }); + }) + .unwrap_err(); assert!(matches!( - result, - Err(DownloadError::HttpError { - status: 503, + err, + DownloadError::RetryAfter { + status: 429, .. - }) + } )); + assert!(!err.is_transient()); + assert!(started.elapsed() < Duration::from_secs(3)); server.join().unwrap(); } + #[test] + fn final_attempt_preserves_outstanding_cooldown() { + use std::io::{Read, Write}; + for delay in [30, 61] { + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let url = format!("http://{}", listener.local_addr().unwrap()); + let server = std::thread::spawn(move || { + for attempt in 0..MAX_ATTEMPTS { + let (mut stream, _) = listener.accept().unwrap(); + let mut request = [0; 4096]; + stream.read(&mut request).unwrap(); + if attempt + 1 == MAX_ATTEMPTS { + write!(stream, "HTTP/1.1 429 Error\r\nRetry-After: {delay}\r\nContent-Length: 0\r\nConnection: close\r\n\r\n").unwrap(); + } else { + stream.write_all(b"HTTP/1.1 429 Error\r\nContent-Length: 0\r\nConnection: close\r\n\r\n").unwrap(); + } + } + }); + let file = tempfile::NamedTempFile::new().unwrap(); + let err = run(Job { + url: &url, + ghcr_blob: false, + etag: None, + path: file.path(), + file: file.as_file(), + total: 5, + connections: 1, + ranges: vec![(0, 4)], + written: vec![0], + on_progress: None, + }) + .unwrap_err(); + assert!( + matches!(err, DownloadError::RetryAfter { status: 429, delay: d, .. } if d == Duration::from_secs(delay)) + ); + assert!(!err.is_transient()); + server.join().unwrap(); + } + } + #[test] fn missing_response_remains_a_request_error() { use std::io::Read; diff --git a/crates/soar-dl/src/xattr.rs b/crates/soar-dl/src/xattr.rs index 836e9dbb..9f5d1786 100644 --- a/crates/soar-dl/src/xattr.rs +++ b/crates/soar-dl/src/xattr.rs @@ -150,7 +150,12 @@ mod tests { let moved = dir.path().join("moved"); std::fs::write(&path, b"data").unwrap(); let file = std::fs::OpenOptions::new().write(true).open(&path).unwrap(); - if write_resume_file(&file, &resume(0, 4, None)).is_err() { + if let Err(err) = write_resume_file(&file, &resume(0, 4, None)) { + assert_eq!( + err.kind(), + std::io::ErrorKind::Unsupported, + "descriptor resume write failed: {err}" + ); eprintln!("skipping: filesystem lacks user xattrs"); return; }