diff --git a/CHANGELOG.md b/CHANGELOG.md index b0d127d..96298e9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,13 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). The one new constraint: a directory passed to `--type elf` must not be written to while it is scanned (a library truncated under its mapping is undefined behaviour, in practice SIGBUS), so run it after the native build task has finished, as the Gradle task graph does. +- **The rest of the whole-file reads are gone too.** The build artefact PUT (`upload build`, an + IPA/APK/AAB) and the build-info PUT stream from disk; ProGuard identification hashes in one + streamed pass; dSYM, PDB, Rust and `xcode` identity reads memory-map the file instead of copying + it; zip-input extraction streams entries to disk; source-map identification streams its hash. + Peak RSS: `upload build` with a 300 MB artefact + 200 MB mapping 661 MB -> 32 MB, a 200 MB + ProGuard mapping 241 MB -> 32 MB, an ELF zip of four 100 MB libraries 463 MB -> 36 MB. Wire + format and exit codes unchanged. ## [0.7.13] - 2026-10-07 diff --git a/src/cli/debug_files.rs b/src/cli/debug_files.rs index c931c20..8449c61 100644 --- a/src/cli/debug_files.rs +++ b/src/cli/debug_files.rs @@ -1954,7 +1954,7 @@ fn stripped_copy_without_sources( // Typed, not a bare `?` into anyhow: an I/O failure on the map must classify the way // `sourcemap::identify` classifies one on the SAME file moments earlier — exit 10, which // integrators are documented not to fall back on — rather than exit 1 ("unexpected"). - let bytes = std::fs::read(map_path).map_err(crate::error::Error::Io)?; + let bytes = crate::symbols::mapped::map_file(map_path).map_err(crate::error::Error::Io)?; let Ok(serde_json::Value::Object(mut map)) = serde_json::from_slice(&bytes) else { return Ok(None); }; diff --git a/src/cli/dsym.rs b/src/cli/dsym.rs index 336fc33..dfbc9bf 100644 --- a/src/cli/dsym.rs +++ b/src/cli/dsym.rs @@ -137,25 +137,12 @@ pub fn extract_slices(path: &Path) -> Vec { out } -/// Hard cap on the size of a single Mach-O file we will read into -/// memory. Real production dSYMs (large iOS apps + Swift stdlib + -/// extensions) typically sit between 200 MB and 1 GB per slice. The -/// cap is a generous-but-bounded safety net against a corrupt or -/// adversarial input (a "dSYM" that is actually a 10 GB random file -/// would otherwise drive the build host into swap). Skip rather than -/// fail — the empty-fallback contract still applies. -const MAX_MACHO_FILE_BYTES: u64 = 1024 * 1024 * 1024; // 1 GiB - fn push_slices_from_macho(path: &Path, out: &mut Vec) { - // Stat the file first so a runaway-size input is rejected without - // allocating gigabytes. `std::fs::read` would otherwise sequentially - // grow the Vec until the kernel says no. - if let Ok(md) = std::fs::metadata(path) { - if md.len() > MAX_MACHO_FILE_BYTES { - return; - } - } - let data = match std::fs::read(path) { + // No size cap: the file is memory-mapped and only its headers are read, so a + // 2 GB DWARF slice costs the same few pages as a small one. (A cap used to guard + // `std::fs::read` growing a multi-GB Vec; it also made `dsym uuid` report `[]`, + // silently, for a legitimate slice over 1 GiB that `debug-files upload` accepted.) + let data = match crate::symbols::mapped::map_file(path) { Ok(d) => d, Err(_) => return, }; @@ -193,6 +180,32 @@ mod tests { use super::*; use tempfile::TempDir; + /// A DWARF slice over 1 GiB (large games) is identified like any other: there is no + /// size cap, since only the headers of the mapping are read. The file is sparse, so + /// this costs no disk and the parse touches a few pages. + #[cfg(unix)] + #[test] + fn extract_uuids_has_no_size_cap() { + use std::io::Write; + let tmp = TempDir::new().unwrap(); + let p = tmp.path().join("huge"); + let mut f = std::fs::File::create(&p).unwrap(); + f.write_all(&crate::symbols::test_macho::thin_macho( + 0x0100_000c, + 0, + [4; 16], + )) + .unwrap(); + f.set_len(1024 * 1024 * 1024 + 4096).unwrap(); // just over 1 GiB, sparse + drop(f); + assert_eq!( + extract_uuids(&p), + [format_uuid(DebugId::from_uuid(uuid::Uuid::from_bytes( + [4; 16] + )))] + ); + } + #[test] fn extract_uuids_returns_empty_for_nonexistent_path() { let out = extract_uuids(Path::new("/no/such/file")); diff --git a/src/cli/xcode_ipa.rs b/src/cli/xcode_ipa.rs index 2d004b9..435a5a5 100644 --- a/src/cli/xcode_ipa.rs +++ b/src/cli/xcode_ipa.rs @@ -232,7 +232,7 @@ pub fn main_executable_uuid(app_path: &Path) -> Option { return None; } - let data = std::fs::read(&binary_path).ok()?; + let data = crate::symbols::mapped::map_file(&binary_path).ok()?; let archive = Archive::parse(&data).ok()?; let mut slices: Vec<(String, Uuid)> = Vec::new(); for obj in archive.objects() { diff --git a/src/inject/mod.rs b/src/inject/mod.rs index 1486ad6..1cd40c9 100644 --- a/src/inject/mod.rs +++ b/src/inject/mod.rs @@ -564,8 +564,13 @@ fn write_map_debug_id( /// / legacy `uuid`, in that precedence. Mirrors the worker's /// `symbolfiles/sourcemap.py:parse`. pub fn read_debug_id(map_path: &Path) -> Result> { - let raw = std::fs::read_to_string(map_path)?; - let value: serde_json::Value = serde_json::from_str(&raw).map_err(|e| { + // Mapped rather than read into a String: the JSON tree below is the only + // copy this needs. UTF-8 is checked up front so a non-UTF-8 map still fails + // as an I/O `InvalidData` error, exactly as `read_to_string` did. + let bytes = crate::symbols::mapped::map_file(map_path)?; + let raw = std::str::from_utf8(&bytes) + .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e))?; + let value: serde_json::Value = serde_json::from_str(raw).map_err(|e| { Error::InputInvalid(format!( "sourcemap is not valid JSON: {e} ({})", map_path.display() diff --git a/src/symbols/dsym.rs b/src/symbols/dsym.rs index f2abf5d..3f073f3 100644 --- a/src/symbols/dsym.rs +++ b/src/symbols/dsym.rs @@ -79,7 +79,7 @@ pub fn identify(dsym_path: &Path) -> Result { continue; } let bin_path = entry.path(); - let data = std::fs::read(&bin_path)?; + let data = super::mapped::map_file(&bin_path)?; let archive = Archive::parse(&data).map_err(|e| { Error::InputInvalid(format!( "failed to parse Mach-O at {}: {}", diff --git a/src/symbols/elf.rs b/src/symbols/elf.rs index a445573..f4fd182 100644 --- a/src/symbols/elf.rs +++ b/src/symbols/elf.rs @@ -101,23 +101,29 @@ pub fn extract_libs( continue; } - let mut bytes = Vec::new(); - entry.read_to_end(&mut bytes)?; - let ElfIdentity { - build_id, - arch, - has_debug_info, - has_symbols, - } = parse_elf_identity(&bytes); - let richness = (has_debug_info, has_symbols, bytes.len() as u64); - let base = Path::new(&name) .file_name() .and_then(|n| n.to_str()) .unwrap_or("lib.so"); // Entries across ABIs can share a basename — prefix with the index. let out_path = out_dir.join(format!("{i}_{base}")); - std::fs::write(&out_path, &bytes)?; + // Stream the entry to disk, then read its identity from the file: an + // inflated unstripped library is 100+ MB and was held whole in memory. + { + let mut out = std::fs::File::create(&out_path)?; + std::io::copy(&mut entry, &mut out)?; + } + let ElfIdentity { + build_id, + arch, + has_debug_info, + has_symbols, + } = parse_elf_identity(&super::mapped::map_file(&out_path)?); + let richness = ( + has_debug_info, + has_symbols, + std::fs::metadata(&out_path)?.len(), + ); libs.push(ElfLib { name, @@ -173,29 +179,16 @@ pub fn scan_dir(dir: &Path, extra: &ExtraSuffixes) -> std::io::Result); + +impl Deref for Mapped { + type Target = [u8]; + fn deref(&self) -> &[u8] { + self.0.as_deref().unwrap_or(&[]) + } +} + +/// Map `path` read-only. +/// +/// Keep the result scoped to the parse and drop it before the file is +/// rewritten or removed (Windows refuses to replace a mapped file). +pub fn map_file(path: &Path) -> std::io::Result { + let file = std::fs::File::open(path)?; + if file.metadata()?.len() == 0 { + return Ok(Mapped(None)); + } + // SAFETY: the map is read-only. The contract (memmap2's): nothing may truncate + // or rewrite the file while it is mapped (SIGBUS / undefined behaviour + // otherwise). It holds for finished build outputs and symbol files, which is all + // this reads; a file still being written by a linker is not a supported input. + Ok(Mapped(Some(unsafe { memmap2::Mmap::map(&file) }?))) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn maps_file_contents_and_handles_an_empty_file() { + let dir = tempfile::tempdir().unwrap(); + let p = dir.path().join("f"); + std::fs::write(&p, b"abc").unwrap(); + assert_eq!(&*map_file(&p).unwrap(), b"abc"); + std::fs::write(&p, b"").unwrap(); + assert!(map_file(&p).unwrap().is_empty()); + assert!(map_file(&dir.path().join("missing")).is_err()); + } +} diff --git a/src/symbols/mod.rs b/src/symbols/mod.rs index d30ef4c..a6919a5 100644 --- a/src/symbols/mod.rs +++ b/src/symbols/mod.rs @@ -26,6 +26,7 @@ pub mod dsym; pub mod elf; pub mod il2cpp_linemap; +pub mod mapped; pub mod pdb; pub mod proguard; pub mod rust; diff --git a/src/symbols/pdb.rs b/src/symbols/pdb.rs index 8f00f67..3afa6f3 100644 --- a/src/symbols/pdb.rs +++ b/src/symbols/pdb.rs @@ -81,7 +81,7 @@ pub fn identify(pdb_path: &Path) -> Result { ))); } - let data = std::fs::read(pdb_path)?; + let data = super::mapped::map_file(pdb_path)?; let archive = Archive::parse(&data).map_err(|e| { Error::InputInvalid(format!( "failed to parse PDB at {}: {}", diff --git a/src/symbols/proguard.rs b/src/symbols/proguard.rs index c9fb78b..67f3ff5 100644 --- a/src/symbols/proguard.rs +++ b/src/symbols/proguard.rs @@ -25,15 +25,33 @@ pub fn looks_like_mapping_filename(name: &str) -> bool { /// Read a file from disk and compute (debug-id, sha1 hex). /// -/// Streams the SHA1 chunk-at-a-time to keep memory flat for large mappings -/// (multi-megabyte mappings are common for big apps). MD5 still needs the -/// whole buffer because Java's `nameUUIDFromBytes` operates on contiguous -/// bytes; mappings rarely exceed 50 MB so this is acceptable. +/// Both digests are computed in ONE streamed pass (64 KiB buffer), so memory +/// stays flat however large the mapping is: Java's `nameUUIDFromBytes` is just +/// an MD5 of the bytes with the version/variant bits set, and MD5 is +/// incremental like SHA-1. pub fn identify(path: &Path) -> std::io::Result { - let bytes = std::fs::read(path)?; - Ok(identify_bytes(&bytes)) + use std::io::Read as _; + let mut file = std::fs::File::open(path)?; + let mut md5 = Md5::new(); + let mut sha1 = Sha1::new(); + let mut buf = [0u8; 64 * 1024]; + loop { + let n = file.read(&mut buf)?; + if n == 0 { + break; + } + md5.update(&buf[..n]); + sha1.update(&buf[..n]); + } + let md5_bytes: [u8; 16] = md5.finalize().into(); + let sha1_bytes: [u8; 20] = sha1.finalize().into(); + Ok(MappingIdentity { + debug_id: Builder::from_md5_bytes(md5_bytes).into_uuid(), + content_sha1_hex: hex::encode(sha1_bytes), + }) } +#[cfg(test)] pub fn identify_bytes(bytes: &[u8]) -> MappingIdentity { MappingIdentity { debug_id: java_name_uuid_from_bytes(bytes), @@ -51,6 +69,7 @@ pub struct MappingIdentity { pub content_sha1_hex: String, } +#[cfg(test)] /// Replicates Java `UUID.nameUUIDFromBytes(name)`: /// 1. MD5 of input bytes. /// 2. Set the version nibble of byte 6 to 3. @@ -62,6 +81,7 @@ pub fn java_name_uuid_from_bytes(name: &[u8]) -> Uuid { Builder::from_md5_bytes(md5_bytes).into_uuid() } +#[cfg(test)] fn sha1_hex(bytes: &[u8]) -> String { let digest: [u8; 20] = Sha1::digest(bytes).into(); hex::encode(digest) @@ -88,6 +108,25 @@ mod tests { ); } + #[test] + fn streamed_identify_equals_in_memory_identify_across_buffer_boundaries() { + // Bigger than the 64 KiB buffer and not a multiple of it: a chunking bug + // would change either digest relative to the one-shot computation. + let dir = tempfile::tempdir().unwrap(); + let p = dir.path().join("mapping.txt"); + let data: Vec = (0..300_001u32).map(|i| (i % 251) as u8).collect(); + std::fs::write(&p, &data).unwrap(); + let streamed = identify(&p).unwrap(); + let whole = identify_bytes(&data); + assert_eq!(streamed.debug_id, whole.debug_id); + assert_eq!(streamed.content_sha1_hex, whole.content_sha1_hex); + std::fs::write(&p, b"").unwrap(); + assert_eq!( + identify(&p).unwrap().debug_id.to_string(), + "d41d8cd9-8f00-3204-a980-0998ecf8427e" + ); + } + #[test] fn sha1_hex_matches_reference() { assert_eq!(sha1_hex(b""), "da39a3ee5e6b4b0d3255bfef95601890afd80709"); diff --git a/src/symbols/rust.rs b/src/symbols/rust.rs index 39f0c3a..d5e740a 100644 --- a/src/symbols/rust.rs +++ b/src/symbols/rust.rs @@ -153,7 +153,7 @@ fn is_bundle_like(p: &Path, extra: &ExtraSuffixes) -> bool { /// on — and to the `code_id` the Rust SDK reports for that module at crash /// time. All three agreeing is what makes symbolication resolve. fn parse_elf(path: &Path) -> Option<(Option, String, bool)> { - let bytes = std::fs::read(path).ok()?; + let bytes = super::mapped::map_file(path).ok()?; let archive = Archive::parse(&bytes).ok()?; let obj = archive.objects().next()?.ok()?; Some(( diff --git a/src/symbols/sourcemap.rs b/src/symbols/sourcemap.rs index 99e7885..5f3337d 100644 --- a/src/symbols/sourcemap.rs +++ b/src/symbols/sourcemap.rs @@ -17,12 +17,28 @@ use crate::error::Result; use crate::inject; /// Read a `.map` file and derive its upload identity. +/// +/// The hash is streamed (64 KiB buffer) and the size comes from the file +/// itself, so identifying a map no longer copies it onto the heap. pub fn identify(path: &Path) -> Result { - let bytes = std::fs::read(path)?; + use std::io::Read as _; + let mut file = std::fs::File::open(path)?; + let mut hasher = Sha1::new(); + let mut buf = [0u8; 64 * 1024]; + let mut size_bytes = 0u64; + loop { + let n = file.read(&mut buf)?; + if n == 0 { + break; + } + hasher.update(&buf[..n]); + size_bytes += n as u64; + } + let digest: [u8; 20] = hasher.finalize().into(); Ok(SourcemapIdentity { debug_id: inject::read_debug_id(path)?, - content_sha1_hex: sha1_hex(&bytes), - size_bytes: bytes.len() as u64, + content_sha1_hex: hex::encode(digest), + size_bytes, }) } @@ -39,15 +55,38 @@ pub struct SourcemapIdentity { pub size_bytes: u64, } -fn sha1_hex(bytes: &[u8]) -> String { - let digest: [u8; 20] = Sha1::digest(bytes).into(); - hex::encode(digest) -} - #[cfg(test)] mod tests { use super::*; + #[test] + fn identify_streams_the_hash_and_size_across_buffer_boundaries() { + // Bigger than the 64 KiB read buffer and not a multiple of it. + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("big.js.map"); + let body = format!( + r#"{{"version":3,"debug_id":"did-big","mappings":"{}"}}"#, + "A".repeat(300_001) + ); + std::fs::write(&path, &body).unwrap(); + let id = identify(&path).unwrap(); + let expected: [u8; 20] = Sha1::digest(body.as_bytes()).into(); + assert_eq!(id.content_sha1_hex, hex::encode(expected)); + assert_eq!(id.size_bytes, body.len() as u64); + assert_eq!(id.debug_id.as_deref(), Some("did-big")); + } + + #[test] + fn identify_still_reports_a_non_utf8_map_as_an_io_error() { + // `read_debug_id` used `read_to_string`, so invalid UTF-8 was an I/O + // `InvalidData` error (exit 10), not a JSON error (exit 11). The mapped + // read must keep that classification. + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("bad.js.map"); + std::fs::write(&path, b"{\"debug_id\":\"\xff\xfe\"}").unwrap(); + assert!(matches!(identify(&path), Err(crate::error::Error::Io(_)))); + } + #[test] fn identify_reads_debug_id_and_hashes_bytes() { let dir = tempfile::tempdir().unwrap(); diff --git a/src/upload/build.rs b/src/upload/build.rs index f90d4f3..b6f3ddb 100644 --- a/src/upload/build.rs +++ b/src/upload/build.rs @@ -184,14 +184,18 @@ pub async fn run(params: Params<'_>, policy: RetryPolicy) -> Result { } // PUT the artefact ZIP. Idempotent overwrite of the same S3 key, so a // retriable 5xx is safe to retry. - let body = tokio::fs::read(&zip_path).await?; - tracing::debug!(endpoint = %http::redact_url(®.artifact_endpoint), body_len = body.len(), "PUT artefact"); - let put = http::send_with_retry(policy, "artefact PUT", true, || { - client - .put(®.artifact_endpoint) - .header(reqwest::header::CONTENT_TYPE, "application/octet-stream") - .body(body.clone()) - }) + // Streamed from disk: the artefact is an IPA/APK/AAB (hundreds of MB), and + // reading it whole and cloning it per attempt held two copies in memory. + let body_len = http::file_len(&zip_path).await?; + tracing::debug!(endpoint = %http::redact_url(®.artifact_endpoint), body_len, "PUT artefact"); + let put = http::put_file( + &client, + policy, + "artefact PUT", + ®.artifact_endpoint, + &zip_path, + Some("application/octet-stream"), + ) .await?; if !put.status().is_success() { let s = put.status().as_u16(); @@ -472,6 +476,71 @@ mod tests { assert_eq!(got, b"x -> y\n".repeat(50)); } + /// The artefact PUT streams the ZIP from disk: a large artefact (bigger than + /// the 64 KiB stream chunk) must arrive whole with an exact `Content-Length` + /// (no chunked body, which a presigned S3 PUT rejects), and a retried attempt + /// must re-send the complete body. + #[tokio::test] + async fn artefact_put_streams_with_exact_length_and_retries_whole() { + let server = MockServer::start().await; + let tmp = tempfile::tempdir().unwrap(); + let data: Vec = (0..300_001u32).map(|i| (i % 251) as u8).collect(); + let artifact = write(tmp.path(), "app.aab", &data); + let payload = write(tmp.path(), "p.json", br#"{"uuid":"abc","version":"1.0"}"#); + let put_url = format!("{}/artefact-put", server.uri()); + + Mock::given(method("POST")) + .and(path("/v2/apps/TKN/builds")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "ok": true, "result": { "build_id": "b1", "endpoint": put_url } + }))) + .mount(&server) + .await; + Mock::given(method("PUT")) + .and(path("/artefact-put")) + .respond_with(ResponseTemplate::new(503)) + .up_to_n_times(1) + .mount(&server) + .await; + Mock::given(method("PUT")) + .and(path("/artefact-put")) + .respond_with(ResponseTemplate::new(200)) + .mount(&server) + .await; + + let uri = server.uri(); + let params = base_params(&uri, &payload, &artifact); + run(params, RetryPolicy::fast(3)).await.unwrap(); + + let received = server.received_requests().await.unwrap(); + let puts: Vec<_> = received + .iter() + .filter(|r| r.url.path() == "/artefact-put") + .collect(); + assert_eq!(puts.len(), 2, "one 503 then one success"); + for put in &puts { + assert_eq!( + put.headers + .get("content-length") + .map(|v| v.to_str().unwrap().to_owned()), + Some(put.body.len().to_string()) + ); + assert!(put.headers.get("transfer-encoding").is_none()); + assert_eq!( + put.headers.get("content-type").unwrap(), + "application/octet-stream" + ); + let mut zip = ZipArchive::new(std::io::Cursor::new(put.body.clone())).unwrap(); + let mut got = Vec::new(); + zip.by_name("app.aab") + .unwrap() + .read_to_end(&mut got) + .unwrap(); + assert_eq!(got, data, "the artefact survives the stream intact"); + } + assert_eq!(puts[0].body, puts[1].body, "retry re-sends the same bytes"); + } + #[tokio::test] async fn uploads_build_info_bundle_from_same_registration() { let server = MockServer::start().await; diff --git a/src/upload/build_info.rs b/src/upload/build_info.rs index afe4e82..31a884f 100644 --- a/src/upload/build_info.rs +++ b/src/upload/build_info.rs @@ -136,10 +136,9 @@ pub async fn run(params: Params<'_>, policy: RetryPolicy) -> Result { } }; - // Read the bundle into memory so retries can re-issue the body. The - // build-info bundle is small (typically < 5 MB compressed). - let body = tokio::fs::read(&zip_path).await?; - tracing::debug!(url = %http::redact_url(&presigned), body_len = body.len(), "PUT build-info bundle"); + // Streamed from disk like the other presigned PUTs (each retry re-opens it). + let body_len = http::file_len(&zip_path).await?; + tracing::debug!(url = %http::redact_url(&presigned), body_len, "PUT build-info bundle"); // The PUT to the presigned S3 URL is idempotent (an overwrite of the same // key), so retrying on a retriable 5xx is safe. // @@ -149,12 +148,14 @@ pub async fn run(params: Params<'_>, policy: RetryPolicy) -> Result { // Omitting it makes S3 recompute a different signature → 403 // SignatureDoesNotMatch. The artefact (`build.rs`) and chunk (`chunked.rs`) // PUTs set the same header for the same reason. - let put = http::send_with_retry(policy, "build-info PUT", true, || { - client - .put(&presigned) - .header(reqwest::header::CONTENT_TYPE, "application/octet-stream") - .body(body.clone()) - }) + let put = http::put_file( + &client, + policy, + "build-info PUT", + &presigned, + &zip_path, + Some("application/octet-stream"), + ) .await?; let put_status = put.status(); diff --git a/src/upload/http.rs b/src/upload/http.rs index 56712e8..807d6f6 100644 --- a/src/upload/http.rs +++ b/src/upload/http.rs @@ -100,8 +100,8 @@ pub fn is_retriable_status(status: reqwest::StatusCode) -> bool { /// statuses. /// /// `make` is invoked once per attempt and MUST build an equivalent request each -/// time (callers clone the body — fine for the small JSON POSTs and in-memory -/// bundle PUTs this module serves). Returns the final response (which the caller +/// time (small JSON POSTs clone their body; a file PUT builds a fresh +/// [`file_body`] stream per attempt — see [`put_file`]). Returns the final response (which the caller /// inspects for success / parses) once it is a success, is non-retriable, or the /// attempts are exhausted; returns `UploadTransport` only when every attempt /// failed at the transport layer. @@ -215,12 +215,219 @@ pub fn truncate_for_log(s: &str, max: usize) -> String { format!("{}…", &s[..end]) } +/// A request body that streams `path` from disk in 64 KiB chunks. +/// +/// Reading the file whole (and cloning it for each retry attempt) holds one or +/// two full copies of an artefact in memory per upload. The file is opened +/// lazily on first poll so an open error surfaces as a failed (retriable) +/// request rather than a panic, and every attempt re-opens it from the start. +/// +/// A streamed body has no known length and would go out chunked, which a +/// presigned S3 PUT rejects: the caller MUST also set `Content-Length` +/// (see [`file_len`]). +pub fn file_body(path: &std::path::Path) -> reqwest::Body { + use tokio::io::AsyncReadExt; + enum St { + Unopened(std::path::PathBuf), + Open(tokio::fs::File), + Done, + } + reqwest::Body::wrap_stream(futures_util::stream::unfold( + St::Unopened(path.to_path_buf()), + |st| async move { + let mut file = match st { + St::Unopened(p) => match tokio::fs::File::open(p).await { + Ok(f) => f, + Err(e) => return Some((Err(e), St::Done)), + }, + St::Open(f) => f, + St::Done => return None, + }; + let mut buf = vec![0u8; 64 * 1024]; + match file.read(&mut buf).await { + Ok(0) => None, + Ok(n) => { + buf.truncate(n); + Some((Ok::<_, std::io::Error>(buf), St::Open(file))) + } + Err(e) => Some((Err(e), St::Done)), + } + }, + )) +} + +/// The byte length to send as `Content-Length` alongside [`file_body`]. +pub async fn file_len(path: &std::path::Path) -> std::io::Result { + Ok(tokio::fs::metadata(path).await?.len()) +} + +/// How many 307/308 hops [`put_file`] follows before giving up. +const MAX_PUT_REDIRECTS: usize = 5; + +/// PUT `path` to `url`, streaming it, retrying like [`send_with_retry`], and following +/// 307/308 redirects ITSELF. +/// +/// reqwest follows a 307/308 only when it can replay the request body, and a streamed +/// body cannot be replayed: it hands the 3xx back instead, which would fail the upload +/// (S3 answers a freshly created bucket's presigned URL with a temporary 307 to the +/// regional endpoint). Re-issuing the PUT to `Location` with a new stream from the +/// file is exactly what following it means. `content_type`, when set, is sent on every hop +/// (a presigned URL's signature may cover it). Returns the final response for the caller +/// to judge (success or not). +pub async fn put_file( + client: &reqwest::Client, + policy: RetryPolicy, + what: &str, + url: &str, + path: &std::path::Path, + content_type: Option<&str>, +) -> Result { + let len = file_len(path).await?; + let mut target = reqwest::Url::parse(url) + .map_err(|e| Error::UploadTransport(format!("{what}: invalid URL: {e}")))?; + for _ in 0..=MAX_PUT_REDIRECTS { + let resp = send_with_retry(policy, what, true, || { + let mut req = client + .put(target.clone()) + .header(reqwest::header::CONTENT_LENGTH, len) + .body(file_body(path)); + if let Some(ct) = content_type { + req = req.header(reqwest::header::CONTENT_TYPE, ct); + } + req + }) + .await?; + if !matches!(resp.status().as_u16(), 307 | 308) { + return Ok(resp); + } + let Some(next) = resp + .headers() + .get(reqwest::header::LOCATION) + .and_then(|v| v.to_str().ok()) + .and_then(|loc| target.join(loc).ok()) + else { + return Ok(resp); // a redirect with no usable Location: let the caller report it + }; + tracing::debug!( + from = %redact_url(target.as_str()), + to = %redact_url(next.as_str()), + status = resp.status().as_u16(), + "{what}: following redirect" + ); + target = next; + } + Err(Error::UploadTransport(format!( + "{what}: more than {MAX_PUT_REDIRECTS} redirects" + ))) +} + #[cfg(test)] mod tests { use super::*; use wiremock::matchers::method; use wiremock::{Mock, MockServer, ResponseTemplate}; + // ── put_file: streaming PUT that follows 307/308 itself ────────────────── + + fn payload(len: usize) -> (tempfile::TempDir, std::path::PathBuf, Vec) { + let dir = tempfile::tempdir().unwrap(); + let p = dir.path().join("payload.zip"); + let data: Vec = (0..len).map(|i| (i % 251) as u8).collect(); + std::fs::write(&p, &data).unwrap(); + (dir, p, data) + } + + /// reqwest cannot replay a streamed body, so it hands a 307/308 back instead of + /// following it. `put_file` must follow it itself — with the WHOLE body again, the same + /// Content-Type, and a relative `Location` resolved against the current URL. + #[tokio::test] + async fn put_file_follows_307_and_308_resending_the_whole_body() { + for status in [307u16, 308] { + let server = MockServer::start().await; + Mock::given(method("PUT")) + .and(wiremock::matchers::path("/first")) + .respond_with( + ResponseTemplate::new(status).insert_header("Location", "/regional/second"), + ) + .mount(&server) + .await; + Mock::given(method("PUT")) + .and(wiremock::matchers::path("/regional/second")) + .respond_with(ResponseTemplate::new(200)) + .mount(&server) + .await; + let (_d, p, data) = payload(200_000); + let client = build_client().unwrap(); + let resp = put_file( + &client, + RetryPolicy::none(), + "test PUT", + &format!("{}/first", server.uri()), + &p, + Some("application/octet-stream"), + ) + .await + .unwrap(); + assert!(resp.status().is_success(), "{status}"); + let reqs = server.received_requests().await.unwrap(); + assert_eq!(reqs.len(), 2, "{status}: one redirect hop"); + for r in &reqs { + assert_eq!(r.body, data, "{status}: every hop carries the full body"); + assert_eq!( + r.headers.get("content-type").unwrap(), + "application/octet-stream" + ); + assert_eq!( + r.headers.get("content-length").unwrap().to_str().unwrap(), + data.len().to_string() + ); + } + } + } + + #[tokio::test] + async fn put_file_gives_up_on_a_redirect_loop_and_passes_through_an_unusable_redirect() { + // A loop: every hop redirects back. + let server = MockServer::start().await; + Mock::given(method("PUT")) + .respond_with(ResponseTemplate::new(307).insert_header("Location", "/again")) + .mount(&server) + .await; + let (_d, p, _) = payload(10); + let client = build_client().unwrap(); + let err = put_file( + &client, + RetryPolicy::none(), + "loop PUT", + &format!("{}/x", server.uri()), + &p, + None, + ) + .await + .unwrap_err(); + assert!(err.to_string().contains("redirects"), "{err}"); + assert!(server.received_requests().await.unwrap().len() <= MAX_PUT_REDIRECTS + 1); + + // A 307 with no Location is returned for the caller to report, not looped on. + let server = MockServer::start().await; + Mock::given(method("PUT")) + .respond_with(ResponseTemplate::new(307)) + .mount(&server) + .await; + let resp = put_file( + &client, + RetryPolicy::none(), + "bare PUT", + &format!("{}/x", server.uri()), + &p, + None, + ) + .await + .unwrap(); + assert_eq!(resp.status().as_u16(), 307); + assert_eq!(server.received_requests().await.unwrap().len(), 1); + } + #[test] fn backoff_doubles_and_caps() { let p = RetryPolicy { diff --git a/src/upload/presigned.rs b/src/upload/presigned.rs index 5a2c119..99711fb 100644 --- a/src/upload/presigned.rs +++ b/src/upload/presigned.rs @@ -232,18 +232,10 @@ pub async fn put_payload( // attempt) held two-plus copies of the archive in memory per upload. The // length is sent explicitly because a streamed body would otherwise go out // chunked, which a presigned S3 PUT rejects. - let len = tokio::fs::metadata(payload).await?.len(); // The PUT is idempotent (overwrite of the same key) — retry on transport // AND retriable status. Each attempt re-opens the file from the start. - let put_resp = http::send_with_retry(policy, "symbol PUT", true, || { - client - .put(presigned_url) - .header(reqwest::header::CONTENT_LENGTH, len) - .body(reqwest::Body::wrap_stream(file_chunks( - payload.to_path_buf(), - ))) - }) - .await?; + let put_resp = + http::put_file(client, policy, "symbol PUT", presigned_url, payload, None).await?; let put_status = put_resp.status(); if !put_status.is_success() { @@ -257,38 +249,6 @@ pub async fn put_payload( Ok(()) } -/// The file's bytes in 64 KiB chunks. The file is opened lazily on first poll so -/// an open error surfaces as a failed (retriable) request, not a panic. -fn file_chunks( - path: std::path::PathBuf, -) -> impl futures_util::Stream>> + Send + 'static { - use tokio::io::AsyncReadExt; - enum St { - Unopened(std::path::PathBuf), - Open(tokio::fs::File), - Done, - } - futures_util::stream::unfold(St::Unopened(path), |st| async move { - let mut file = match st { - St::Unopened(p) => match tokio::fs::File::open(p).await { - Ok(f) => f, - Err(e) => return Some((Err(e), St::Done)), - }, - St::Open(f) => f, - St::Done => return None, - }; - let mut buf = vec![0u8; 64 * 1024]; - match file.read(&mut buf).await { - Ok(0) => None, - Ok(n) => { - buf.truncate(n); - Some((Ok(buf), St::Open(file))) - } - Err(e) => Some((Err(e), St::Done)), - } - }) -} - /// Run the two-stage presigned upload for a single symbol artifact /// ([`register`] then [`put_payload`]). The payload is always produced up /// front; callers that want to skip producing it when the server already has @@ -656,6 +616,40 @@ mod tests { } /// A payload that vanished is a reported error, not a panic or a hang. + /// A freshly created S3 bucket answers the presigned URL with a temporary 307 to the + /// regional endpoint; the symbol PUT must follow it (a streamed body is not replayed by + /// reqwest on its own). + #[tokio::test] + async fn put_payload_follows_a_temporary_redirect() { + let server = MockServer::start().await; + Mock::given(method("PUT")) + .and(path("/bucket/key")) + .respond_with( + ResponseTemplate::new(307) + .insert_header("Location", format!("{}/regional/key", server.uri())), + ) + .mount(&server) + .await; + Mock::given(method("PUT")) + .and(path("/regional/key")) + .respond_with(ResponseTemplate::new(200)) + .mount(&server) + .await; + let (payload, data) = patterned_payload(150_001); + let client = http::build_client().unwrap(); + put_payload( + &client, + RetryPolicy::none(), + &format!("{}/bucket/key", server.uri()), + payload.path(), + ) + .await + .unwrap(); + let reqs = server.received_requests().await.unwrap(); + assert_eq!(reqs.len(), 2); + assert_eq!(reqs[1].body, data); + } + #[tokio::test] async fn put_payload_reports_a_missing_payload_file() { let server = MockServer::start().await;