From 6aa0078fce83dd187f2d6f76a9cf406604334784 Mon Sep 17 00:00:00 2001 From: Alexey Karimov Date: Wed, 7 Oct 2026 15:17:46 +0500 Subject: [PATCH 01/11] feat(debug-files): --type elf accepts directories of .so files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Walk directories recursively for .so/.so.dbg/.so.sym (+ --extension), reading libraries in place instead of requiring a pre-built zip. Multiple paths (dirs and/or zips) are collected first so the one-per-build-id preference spans all of them. Each zip gets its own extraction dir; an unreadable directory root fails instead of exiting 0; a missing path is exit 10. Closes #69 πŸ€– Generated with [Claude Code](https://claude.com/claude-code) --- README.md | 2 +- src/cli/debug_files.rs | 178 ++++++++++++++++++++-------------------- src/symbols/elf.rs | 160 ++++++++++++++++++++++++++++++++++-- src/symbols/mod.rs | 4 +- tests/elf_upload.rs | 180 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 425 insertions(+), 99 deletions(-) diff --git a/README.md b/README.md index 0980d3e..495165f 100644 --- a/README.md +++ b/README.md @@ -65,7 +65,7 @@ The upload flow itself. ProGuard, Rust, ELF, dSYM, PDB, sourcemap, and Unity IL2 #### Android NDK β€” `--type elf` -Takes AGP's pre-built `native-debug-symbols.zip` (a directory is rejected; the Gradle plugin zips the intermediates folder first). Every library inside is uploaded as its own symbol, keyed by its GNU build-id β€” `--uuid` (the SDK's `BUILD_UUID`) is still required but only correlates logs. Built-in entry names are `.so`, `.so.dbg` (`ndk.debugSymbolLevel = 'FULL'`) and `.so.sym` (`'SYMBOL_TABLE'`, function names only β€” no `file:line`). A library without a build-id is warned about and skipped; two entries with the same build-id upload once, preferring the one with DWARF. The server dedups on the build-id, so switching a library from `SYMBOL_TABLE` to `FULL` needs `--force` to replace the stored symbols. +Takes one or more paths: AGP's pre-built `native-debug-symbols.zip`, and/or directories, which are walked recursively for libraries (e.g. `build/intermediates/merged_native_libs/`) and read in place β€” nothing is re-zipped. Every library found is uploaded as its own symbol, keyed by its GNU build-id β€” `--uuid` (the SDK's `BUILD_UUID`) is still required but only correlates logs. Built-in entry names are `.so`, `.so.dbg` (`ndk.debugSymbolLevel = 'FULL'`) and `.so.sym` (`'SYMBOL_TABLE'`, function names only β€” no `file:line`). A library without a build-id is warned about and skipped; two entries with the same build-id upload once (across all the given paths), preferring the one with DWARF. The server dedups on the build-id, so switching a library from `SYMBOL_TABLE` to `FULL` needs `--force` to replace the stored symbols. #### Unity IL2CPP β€” `--type il2cpp-linemap` diff --git a/src/cli/debug_files.rs b/src/cli/debug_files.rs index 4d9dd29..3fc10fd 100644 --- a/src/cli/debug_files.rs +++ b/src/cli/debug_files.rs @@ -17,7 +17,9 @@ const DEFAULT_ENDPOINT: &str = "https://api.bugsee.com"; pub enum DebugFilesCommand { /// Discover, package, and upload debug information files from one or more paths. Upload { - /// One or more directories or files to scan. + /// One or more directories or files to scan. For `--type elf`, a directory + /// is walked recursively for native libraries (e.g. AGP's + /// `merged_native_libs`), and a file is a `native-debug-symbols.zip`. #[arg(required = true)] paths: Vec, @@ -141,8 +143,9 @@ pub enum DebugFilesCommand { /// such as `.so.sym` work. It only widens the NAME match: each type's content check /// still applies to what it picks up. /// - /// Per type: `elf` β€” entries in the native-debug-symbols zip (built in: `.so`, - /// `.so.dbg`, `.so.sym`; each still needs a GNU build-id). `proguard` β€” files in a + /// Per type: `elf` β€” files in a scanned directory or entries in the + /// native-debug-symbols zip (built in: `.so`, `.so.dbg`, `.so.sym`; each still + /// needs a GNU build-id). `proguard` β€” files in a /// scanned directory (built in: `mapping*.txt`). `sourcemaps` β€” files in a scanned /// directory (built in: `.map`). `pdb` β€” files in a scanned directory, compared /// case-insensitively (built in: `.pdb`; each still needs the MSF container @@ -1078,15 +1081,9 @@ pub(crate) fn discover_dsyms(paths: &[PathBuf], extra: &ExtraSuffixes) -> Vec/out` directory -/// before invoking the CLI). +/// Upload native libraries from one or more inputs: AGP's +/// `native-debug-symbols.zip` and/or directories walked recursively. Each +/// library becomes its own symbol document keyed by its GNU build-id. #[allow(clippy::too_many_arguments)] /// Max concurrent per-`.so` register+upload pipelines. A native archive can hold /// dozens of libraries across ABIs; uploading them serially would dominate CI @@ -1151,18 +1148,6 @@ async fn run_elf_upload( if paths.is_empty() { return Err(input_not_found("no input paths supplied")); } - for p in paths { - if !p.is_file() { - // Directory-walk + re-pack is out of scope β€” the Gradle plugin - // pre-zips the intermediate-folder case before invoking the CLI. - return Err(input_invalid(format!( - "--type elf expects a pre-built zip file (typically AGP's \ - native-debug-symbols.zip); got {}", - p.display() - ))); - } - } - // `build_uuid` (the SDK's runtime BUILD_UUID) is NO LONGER the native // symbol identity β€” each `.so` is keyed by its OWN GNU build-id (one file β†’ // one symbol document β†’ one S3 object). The BUILD_UUID belongs to the @@ -1194,10 +1179,42 @@ async fn run_elf_upload( let mut full_already_existed = 0u32; let mut skipped_no_build_id = 0u32; - for archive in paths { - tracing::info!(path = %archive.display(), "processing native-debug-symbols archive"); - let work_dir = tempfile::tempdir()?; - let libs = elf::extract_libs(archive, work_dir.path(), extra)?; + // Collect from EVERY input first so the one-per-build-id preference spans + // paths (e.g. the same library under two variant folders), then upload once. + let work_dir = tempfile::tempdir()?; + let mut libs: Vec = Vec::new(); + for (n, input) in paths.iter().enumerate() { + tracing::info!(path = %input.display(), "scanning for native libraries"); + // Own extraction dir per input: zip entries are staged as `_`, + // and indices restart at 0 in every archive. + let input_dir = work_dir.path().join(n.to_string()); + std::fs::create_dir_all(&input_dir)?; + let found = elf::collect_libs(input, &input_dir, extra).map_err(|e| { + if e.kind() == std::io::ErrorKind::NotFound { + input_not_found(format!("{} does not exist", input.display())) + } else if input.is_dir() { + input_invalid(format!("cannot scan {}: {e}", input.display())) + } else { + input_invalid(format!( + "--type elf expects a directory or a native-debug-symbols zip; \ + cannot read {}: {e}", + input.display() + )) + } + })?; + if found.is_empty() { + // Nothing even matched by name β€” the one case `--extension` fixes. + // Ahead of the dry-run exit: a dry run is how a caller checks this. + tracing::warn!( + path = %input.display(), + "no files named .so / .so.dbg / .so.sym (or an --extension suffix) β€” \ + nothing to upload. If your toolchain emits another suffix, pass it \ + with --extension" + ); + } + libs.extend(found); + } + { let libs = elf::keep_richest_per_build_id(libs); let total = libs.len(); @@ -1220,61 +1237,45 @@ async fn run_elf_upload( tracing::info!( libraries = total, uploadable = uploadable.len(), - "extracted native libraries" + "collected native libraries" ); if total == 0 { - // Nothing even matched by name β€” the one case `--extension` fixes. - // Ahead of the dry-run exit: a dry run is how a caller checks this. - tracing::warn!( - archive = %archive.display(), - "no entries named .so / .so.dbg / .so.sym (or an --extension suffix) β€” \ - nothing to upload. If your toolchain emits another suffix, pass it \ - with --extension" - ); - continue; - } - if dry_run { + // Already warned per input above. + } else if dry_run { tracing::info!( - "dry-run: would register + upload {} libraries from {}", - uploadable.len(), - archive.display() + "dry-run: would register + upload {} libraries", + uploadable.len() ); - continue; - } - if uploadable.is_empty() { - tracing::warn!( - archive = %archive.display(), - "no native libraries with a GNU build-id β€” nothing to upload" - ); - continue; - } - - let client = client.as_ref().expect("client constructed when !dry_run"); - - // Run each `.so`'s pack β†’ register β†’ dedup-or-PUT pipeline concurrently. - let outcomes: Vec<(bool, anyhow::Result)> = - futures_util::stream::iter(uploadable) - .map(|lib| { - let client = client.clone(); - let work = work_dir.path().to_path_buf(); - async move { - let symbol_table_only = lib.name.ends_with(".so.sym"); - let outcome = upload_one_so(&client, ctx, &lib, &work).await; - (symbol_table_only, outcome) - } - }) - .buffer_unordered(ELF_UPLOAD_CONCURRENCY) - .collect() - .await; - - for (symbol_table_only, outcome) in outcomes { - match outcome? { - presigned::Outcome::Uploaded => uploaded += 1, - presigned::Outcome::AlreadyExists => { - already_existed += 1; - if !symbol_table_only { - full_already_existed += 1; + } else if uploadable.is_empty() { + tracing::warn!("no native libraries with a GNU build-id β€” nothing to upload"); + } else { + let client = client.as_ref().expect("client constructed when !dry_run"); + + // Run each `.so`'s pack β†’ register β†’ dedup-or-PUT pipeline concurrently. + let outcomes: Vec<(bool, anyhow::Result)> = + futures_util::stream::iter(uploadable.into_iter().enumerate()) + .map(|(i, lib)| { + let client = client.clone(); + let work = work_dir.path().to_path_buf(); + async move { + let symbol_table_only = lib.name.ends_with(".so.sym"); + let outcome = upload_one_so(&client, ctx, &lib, i, &work).await; + (symbol_table_only, outcome) + } + }) + .buffer_unordered(ELF_UPLOAD_CONCURRENCY) + .collect() + .await; + + for (symbol_table_only, outcome) in outcomes { + match outcome? { + presigned::Outcome::Uploaded => uploaded += 1, + presigned::Outcome::AlreadyExists => { + already_existed += 1; + if !symbol_table_only { + full_already_existed += 1; + } } } } @@ -1314,6 +1315,7 @@ async fn upload_one_so( client: &reqwest::Client, ctx: ElfUploadCtx<'_>, lib: &elf::ElfLib, + index: usize, work_dir: &Path, ) -> anyhow::Result { let build_id = lib @@ -1325,18 +1327,14 @@ async fn upload_one_so( .and_then(|n| n.to_str()) .unwrap_or("lib.so"); - // Name the staging ZIP after the extracted `.so`'s on-disk filename, which - // `elf::extract_libs` already made unique with an index prefix - // (`_`). Keying it on `build_id` instead would collide under - // the concurrent `buffer_unordered` upload if two libraries shared a - // build-id β€” two tasks would `File::create` the same path and interleave - // writes into a corrupt ZIP with the wrong SHA-1. - let zip_stem = lib - .path - .file_name() - .and_then(|n| n.to_str()) - .unwrap_or("lib.so"); - let zip_path = work_dir.join(format!("{zip_stem}.zip")); + // Name the staging ZIP by the caller's per-library index, unique within the + // run. Keying it on `build_id` would collide under the concurrent + // `buffer_unordered` upload if two libraries shared a build-id, and the + // basename alone collides across ABIs (a directory scan reads libraries in + // place, so `lib/arm64-v8a/libfoo.so` and `lib/x86_64/libfoo.so` both end + // in `libfoo.so`) β€” two tasks would `File::create` the same path and + // interleave writes into a corrupt ZIP with the wrong SHA-1. + let zip_path = work_dir.join(format!("{index}_{entry_name}.zip")); compress::pack_single_entry(&lib.path, entry_name, &zip_path, ctx.strategy)?; let hash = elf::sha1_hex_of_file(&zip_path)?; diff --git a/src/symbols/elf.rs b/src/symbols/elf.rs index 7a3150c..0b05c70 100644 --- a/src/symbols/elf.rs +++ b/src/symbols/elf.rs @@ -2,7 +2,7 @@ //! //! The caller hands the CLI an already-packaged `native-debug-symbols.zip` //! (the artifact AGP writes under `build/outputs/native-debug-symbols/ -//! /`). We open it and, for EACH `.so` (or the `.so.dbg` / `.so.sym` +//! /`) or a directory of libraries. We open/walk it and, for EACH `.so` (or the `.so.dbg` / `.so.sym` //! variants, depending on `ndk.debugSymbolLevel`), read the GNU build-id //! (`code_id`) via `symbolic-debuginfo` β€” the SAME crate family (major 13) the //! worker uses, so the identifier is byte-identical producer↔consumer. @@ -21,9 +21,9 @@ //! matched at crash time, so it is warned about and skipped β€” never faked with //! the BUILD_UUID. //! -//! Out of scope: walking a loose directory of unstripped `.so`s (AGP's -//! intermediate-folder case) β€” the Gradle plugin pre-zips that before invoking -//! the CLI. +//! The input is either that zip or a directory, walked recursively for the same +//! entry names (AGP's `merged_native_libs` intermediates folder) β€” libraries +//! are read in place, so nothing is re-compressed just to be unpacked again. use sha1::{Digest as _, Sha1}; use std::io::Read; @@ -42,7 +42,7 @@ pub fn sha1_hex_of_file(path: &Path) -> std::io::Result { Ok(sha1_hex(&std::fs::read(path)?)) } -/// One native library extracted from an AGP `native-debug-symbols.zip`. +/// One native library found in an AGP `native-debug-symbols.zip` or a scanned directory. /// /// Each `.so` is its OWN symbol: one file β†’ one symbol document β†’ one S3 /// object, keyed by its own GNU build-id. (`images[]` within a single document @@ -59,7 +59,8 @@ pub struct ElfLib { /// Object architecture (e.g. `arm64`); diagnostic only β€” the worker /// reconciles the canonical arch from the ELF during processing. pub arch: String, - /// Temp file holding the extracted `.so` bytes, packed into its own upload. + /// File holding the library bytes, packed into its own upload: a temp copy + /// extracted from a zip, or the original file when scanned from a directory. pub path: PathBuf, /// How much this file can symbolicate: (DWARF debug info, symbol table, /// byte size), compared in that order by [`keep_richest_per_build_id`]. @@ -122,6 +123,79 @@ pub fn extract_libs( Ok(libs) } +/// Collect native libraries from `input`: a directory is walked recursively +/// (see [`scan_dir`]), anything else is read as a `native-debug-symbols.zip` +/// and extracted into `out_dir` (see [`extract_libs`]). +pub fn collect_libs( + input: &Path, + out_dir: &Path, + extra: &ExtraSuffixes, +) -> std::io::Result> { + if input.is_dir() { + scan_dir(input, extra) + } else { + extract_libs(input, out_dir, extra) + } +} + +/// Walk `dir` recursively for native libraries (same name rules as the zip +/// entries), reading each one's identity in place. `name` is the path relative +/// to `dir`. Symlinks are followed (loops are skipped); unreadable entries are +/// skipped with a warning rather than failing the whole scan. Sorted by path so +/// the "first entry wins" tie-break in [`keep_richest_per_build_id`] is stable. +pub fn scan_dir(dir: &Path, extra: &ExtraSuffixes) -> std::io::Result> { + let mut libs = Vec::new(); + for entry in walkdir::WalkDir::new(dir) + .follow_links(true) + .sort_by_file_name() + { + let entry = match entry { + Ok(e) => e, + // An unreadable root means nothing was scanned: fail, never exit 0 with + // a misleading "pass --extension" hint. + Err(e) if e.depth() == 0 => return Err(e.into()), + Err(e) => { + tracing::warn!(error = %e, "skipping unreadable path while scanning for native libraries"); + continue; + } + }; + if !entry.file_type().is_file() { + continue; + } + let file_name = entry.file_name().to_string_lossy(); + if !(is_native_lib_entry(&file_name) || extra.matches(&file_name)) { + continue; + } + let bytes = match std::fs::read(entry.path()) { + Ok(b) => b, + Err(e) => { + tracing::warn!(path = %entry.path().display(), error = %e, "skipping unreadable native library"); + continue; + } + }; + let ElfIdentity { + build_id, + arch, + has_debug_info, + has_symbols, + } = parse_elf_identity(&bytes); + let name = entry + .path() + .strip_prefix(dir) + .unwrap_or(entry.path()) + .to_string_lossy() + .replace('\\', "/"); + libs.push(ElfLib { + name, + build_id, + arch, + path: entry.path().to_path_buf(), + richness: (has_debug_info, has_symbols, bytes.len() as u64), + }); + } + Ok(libs) +} + /// Whether a `native-debug-symbols.zip` entry is a native library to upload. /// The suffix depends on AGP's `ndk.debugSymbolLevel`: `FULL` packs unstripped /// `.so` (occasionally `.so.dbg`), while `SYMBOL_TABLE` packs `.so.sym` β€” an @@ -359,6 +433,80 @@ mod tests { assert_eq!(std::fs::read(&libs[0].path).unwrap(), b"not a real elf"); } + fn fixture_bytes() -> Vec { + std::fs::read( + Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures/elf/libsymbol1.so"), + ) + .unwrap() + } + + #[test] + fn scan_dir_walks_recursively_in_place_and_skips_other_files() { + let dir = tempfile::tempdir().unwrap(); + let bytes = fixture_bytes(); + for rel in [ + "lib/arm64-v8a/libsymbol1.so", + "lib/x86_64/libsymbol1.so.sym", + "lib/x86_64/readme.txt", + ] { + let p = dir.path().join(rel); + std::fs::create_dir_all(p.parent().unwrap()).unwrap(); + std::fs::write(p, &bytes).unwrap(); + } + std::fs::write(dir.path().join("lib/x86_64/libbare.so"), b"not elf").unwrap(); + + let libs = scan_dir(dir.path(), &ExtraSuffixes::default()).unwrap(); + assert_eq!( + names(&libs), + [ + "lib/arm64-v8a/libsymbol1.so", + "lib/x86_64/libbare.so", + "lib/x86_64/libsymbol1.so.sym" + ] + ); + assert_eq!( + libs[0].build_id.as_deref(), + Some("bca64abfec40dbb631bb8f1c37414472") + ); + assert_eq!(libs[1].build_id, None, "non-ELF yields no build-id"); + assert_eq!( + libs[0].path, + dir.path().join("lib/arm64-v8a/libsymbol1.so"), + "read in place, not copied" + ); + + let extra = ExtraSuffixes::parse(&["txt".to_string()]).unwrap(); + assert_eq!(scan_dir(dir.path(), &extra).unwrap().len(), 4); + } + + #[test] + fn collect_libs_dispatches_on_directory_vs_zip() { + let dir = tempfile::tempdir().unwrap(); + let bytes = fixture_bytes(); + let libdir = dir.path().join("libs"); + std::fs::create_dir_all(&libdir).unwrap(); + std::fs::write(libdir.join("libsymbol1.so"), &bytes).unwrap(); + let zip_path = dir.path().join("n.zip"); + { + let mut zw = zip::ZipWriter::new(std::fs::File::create(&zip_path).unwrap()); + zw.start_file("arm64-v8a/libsymbol1.so", SimpleFileOptions::default()) + .unwrap(); + zw.write_all(&bytes).unwrap(); + zw.finish().unwrap(); + } + let out = dir.path().join("out"); + std::fs::create_dir_all(&out).unwrap(); + let x = ExtraSuffixes::default(); + assert_eq!( + names(&collect_libs(&libdir, &out, &x).unwrap()), + ["libsymbol1.so"] + ); + assert_eq!( + names(&collect_libs(&zip_path, &out, &x).unwrap()), + ["arm64-v8a/libsymbol1.so"] + ); + } + fn lib(name: &str, build_id: Option<&str>, richness: (bool, bool, u64)) -> ElfLib { ElfLib { name: name.to_string(), diff --git a/src/symbols/mod.rs b/src/symbols/mod.rs index 75838a9..d30ef4c 100644 --- a/src/symbols/mod.rs +++ b/src/symbols/mod.rs @@ -2,8 +2,8 @@ //! //! Submodules handle format-specific concerns: //! - `proguard` β€” R8 / ProGuard mapping files (v0.1) -//! - `elf` β€” NDK native-debug-symbols zip pass-through -//! (v0.1 pass-through; per-`.so` build-id extraction TBD) +//! - `elf` β€” NDK native libraries (a native-debug-symbols zip or a +//! directory), per-`.so` build-id keyed //! - `dsym` β€” Apple Mach-O dSYM bundles (v0.1: single bundle; //! multi-bundle directory walk + BCSymbolMap support TBD) //! - `pdb` β€” Windows PDB (MSF container); identity is the debug id diff --git a/tests/elf_upload.rs b/tests/elf_upload.rs index 7791c98..168429e 100644 --- a/tests/elf_upload.rs +++ b/tests/elf_upload.rs @@ -325,3 +325,183 @@ fn elf_dry_run_with_no_matching_entries_suggests_extension() { .success() .stderr(predicates::str::contains("pass it with --extension")); } + +fn elf_args<'a>(endpoint: &'a str, inputs: &'a [String]) -> Vec<&'a str> { + let mut a = vec![ + "--endpoint", + endpoint, + "--app-token", + "TKN", + "debug-files", + "upload", + "--type", + "elf", + "--version", + "1.0", + "--build", + "1", + "--uuid", + "00000000-0000-0000-0000-000000000000", + ]; + a.extend(inputs.iter().map(String::as_str)); + a +} + +/// A directory (AGP's `merged_native_libs` layout) is walked recursively. The +/// same library under two ABI folders, plus a zip of it, shares ONE build-id, +/// so exactly one POST + PUT go out; the non-library file is ignored. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn elf_upload_walks_a_directory_and_dedups_by_build_id_across_paths() { + let server = MockServer::start().await; + let tmp = tempfile::tempdir().unwrap(); + let fixture = Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures/elf/libsymbol1.so"); + let root = tmp.path().join("merged_native_libs/release/out/lib"); + for abi in ["arm64-v8a", "x86_64"] { + std::fs::create_dir_all(root.join(abi)).unwrap(); + std::fs::copy(&fixture, root.join(abi).join("libsymbol1.so")).unwrap(); + } + std::fs::write(root.join("x86_64/notes.txt"), b"ignored").unwrap(); + let zip_path = pack_native_zip(tmp.path(), "arm64-v8a/libsymbol1.so"); + + let put_url = format!("{}/put/sym", server.uri()); + Mock::given(method("POST")) + .and(wm_path("/apps/TKN/symbols")) + .and(body_string_contains(FIXTURE_BUILD_ID)) + .respond_with( + ResponseTemplate::new(200).set_body_json(json!({"code": 0, "endpoint": put_url})), + ) + .expect(1) + .mount(&server) + .await; + Mock::given(method("PUT")) + .respond_with(ResponseTemplate::new(200)) + .expect(1) + .mount(&server) + .await; + + let endpoint = server.uri(); + let inputs = vec![ + tmp.path() + .join("merged_native_libs") + .to_string_lossy() + .into_owned(), + zip_path.to_string_lossy().into_owned(), + ]; + tokio::task::spawn_blocking(move || { + common::cli() + .args(elf_args(&endpoint, &inputs)) + .assert() + .success(); + }) + .await + .unwrap(); +} + +/// An input path that exists as neither a directory nor a readable zip still +/// fails (it is not silently skipped), with a message naming it. +#[test] +fn elf_upload_rejects_a_missing_path() { + let tmp = tempfile::tempdir().unwrap(); + let missing = tmp.path().join("nope").to_string_lossy().into_owned(); + let inputs = vec![missing]; + common::cli() + .args(elf_args("http://127.0.0.1:1", &inputs)) + .assert() + .code(10) + .stderr(predicates::str::contains("nope")); +} + +/// Two zips whose entries share a basename and archive index but carry DIFFERENT +/// build-ids must each upload their own bytes (extraction must not collide). +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn elf_upload_keeps_same_named_entries_of_different_zips_apart() { + let server = MockServer::start().await; + let tmp = tempfile::tempdir().unwrap(); + let fixture = Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures/elf/libsymbol1.so"); + let a = pack_native_zip(tmp.path(), "arm64-v8a/libsymbol1.so"); + let a = { + let d = tmp.path().join("a"); + std::fs::create_dir_all(&d).unwrap(); + let t = d.join("a.zip"); + std::fs::rename(a, &t).unwrap(); + t + }; + // Same fixture with the build-id's first byte flipped (found by value). + let mut other = std::fs::read(&fixture).unwrap(); + let id = hex::decode(FIXTURE_BUILD_ID).unwrap(); + let pos = other.windows(id.len()).position(|w| w == id).unwrap(); + other[pos] ^= 0xff; + let other_id = hex::encode(&other[pos..pos + id.len()]); + let b = tmp.path().join("b.zip"); + { + let mut zw = zip::ZipWriter::new(std::fs::File::create(&b).unwrap()); + zw.start_file( + "x86_64/libsymbol1.so", + zip::write::SimpleFileOptions::default(), + ) + .unwrap(); + zw.write_all(&other).unwrap(); + zw.finish().unwrap(); + } + + let put_url = format!("{}/put/sym", server.uri()); + Mock::given(method("POST")) + .and(wm_path("/apps/TKN/symbols")) + .respond_with( + ResponseTemplate::new(200).set_body_json(json!({"code": 0, "endpoint": put_url})), + ) + .expect(2) + .mount(&server) + .await; + Mock::given(method("PUT")) + .respond_with(ResponseTemplate::new(200)) + .expect(2) + .mount(&server) + .await; + + let endpoint = server.uri(); + let inputs = vec![ + a.to_string_lossy().into_owned(), + b.to_string_lossy().into_owned(), + ]; + tokio::task::spawn_blocking(move || { + common::cli() + .args(elf_args(&endpoint, &inputs)) + .assert() + .success(); + }) + .await + .unwrap(); + + let reqs = server.received_requests().await.unwrap(); + let posts: Vec = reqs + .iter() + .filter(|r| r.method.as_str() == "POST") + .map(|r| String::from_utf8_lossy(&r.body).into_owned()) + .collect(); + assert!(posts.iter().any(|p| p.contains(FIXTURE_BUILD_ID))); + assert!(posts.iter().any(|p| p.contains(&other_id))); + let puts = reqs.iter().filter(|r| r.method.as_str() == "PUT").count(); + assert_eq!(puts, 2); +} + +/// An unreadable/nonexistent directory root must not exit 0. +#[cfg(unix)] +#[test] +fn elf_upload_fails_on_an_unreadable_directory() { + use std::os::unix::fs::PermissionsExt; + let tmp = tempfile::tempdir().unwrap(); + let locked = tmp.path().join("locked"); + std::fs::create_dir_all(&locked).unwrap(); + std::fs::set_permissions(&locked, std::fs::Permissions::from_mode(0o000)).unwrap(); + // Root bypasses permission checks; nothing to assert then. + let readable = std::fs::read_dir(&locked).is_ok(); + let inputs = vec![locked.to_string_lossy().into_owned()]; + let assert = common::cli() + .args(elf_args("http://127.0.0.1:1", &inputs)) + .assert(); + std::fs::set_permissions(&locked, std::fs::Permissions::from_mode(0o755)).unwrap(); + if !readable { + assert.failure(); + } +} From 7ace775f9131a51fb48ead06464f1b0b91ab8142 Mon Sep 17 00:00:00 2001 From: Alexey Karimov Date: Wed, 7 Oct 2026 15:28:44 +0500 Subject: [PATCH 02/11] fix(debug-files): empty ELF directory exits 10; don't descend dir symlinks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review on #70: a directory with no matching libraries is a miswired path, not success (exit 10; empty zip still warns). The walk no longer follows directory symlinks (file symlinks are still read). Add changelog entry and tests. πŸ€– Generated with [Claude Code](https://claude.com/claude-code) --- CHANGELOG.md | 7 +++++++ src/cli/debug_files.rs | 10 ++++++++++ src/symbols/elf.rs | 9 ++++++--- tests/elf_upload.rs | 38 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 61 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 68f73ab..1a585bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,13 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Added +- **`debug-files upload --type elf` accepts directories.** One or more directories (and/or + `native-debug-symbols.zip` files) are scanned; directories are walked recursively for `.so` / + `.so.dbg` / `.so.sym` (+ `--extension`) and read in place, so a Gradle build can point it at + `merged_native_libs/` without re-zipping. One upload per GNU build-id across all + paths. A directory with no matching libraries exits 10; an empty zip still only warns. See #69. + ## [0.7.13] - 2026-10-07 ### Added diff --git a/src/cli/debug_files.rs b/src/cli/debug_files.rs index 3fc10fd..dacb029 100644 --- a/src/cli/debug_files.rs +++ b/src/cli/debug_files.rs @@ -1202,6 +1202,16 @@ async fn run_elf_upload( )) } })?; + if found.is_empty() && input.is_dir() { + // A directory is the build's own output: empty usually means a + // miswired path or a task that ran before the libraries were merged, + // and exit 0 would let the build go green with no native symbols. + // (An empty ZIP stays a warning, as it always was.) + return Err(input_not_found(format!( + "no .so / .so.dbg / .so.sym files (or --extension suffix) under {}", + input.display() + ))); + } if found.is_empty() { // Nothing even matched by name β€” the one case `--extension` fixes. // Ahead of the dry-run exit: a dry run is how a caller checks this. diff --git a/src/symbols/elf.rs b/src/symbols/elf.rs index 0b05c70..5e9b809 100644 --- a/src/symbols/elf.rs +++ b/src/symbols/elf.rs @@ -140,13 +140,13 @@ pub fn collect_libs( /// Walk `dir` recursively for native libraries (same name rules as the zip /// entries), reading each one's identity in place. `name` is the path relative -/// to `dir`. Symlinks are followed (loops are skipped); unreadable entries are +/// to `dir`. Directory symlinks are not descended, file symlinks are read; unreadable entries are /// skipped with a warning rather than failing the whole scan. Sorted by path so /// the "first entry wins" tie-break in [`keep_richest_per_build_id`] is stable. pub fn scan_dir(dir: &Path, extra: &ExtraSuffixes) -> std::io::Result> { let mut libs = Vec::new(); for entry in walkdir::WalkDir::new(dir) - .follow_links(true) + .follow_links(false) .sort_by_file_name() { let entry = match entry { @@ -159,7 +159,10 @@ pub fn scan_dir(dir: &Path, extra: &ExtraSuffixes) -> std::io::Result Date: Wed, 7 Oct 2026 16:04:41 +0500 Subject: [PATCH 03/11] fix(debug-files): fail ELF dir scan on any I/O error; harden tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address adversarial review on #70: - any I/O error while scanning a directory (unreadable subdir/library, dangling link) now fails with exit 11 instead of silently uploading a partial set - the symlink test used same-build-id copies so dedup masked it; the escaped library now has its own id (verified to fail if links are followed) - pin exit 11 for unreadable inputs; hints for a zip-in-directory and a bare .so; NotFound only maps to exit 10 when the input itself is missing - e2e harness gets a directory flow; changelog/help/README updated; remove a leftover block scope πŸ€– Generated with [Claude Code](https://claude.com/claude-code) --- CHANGELOG.md | 10 ++- README.md | 2 +- scripts/e2e_flows.py | 17 +++++ src/cli/debug_files.rs | 138 +++++++++++++++++++++++------------------ src/symbols/elf.rs | 30 +++------ tests/elf_upload.rs | 108 +++++++++++++++++++++++++++----- 6 files changed, 207 insertions(+), 98 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a585bd..a517bc0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,15 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). `native-debug-symbols.zip` files) are scanned; directories are walked recursively for `.so` / `.so.dbg` / `.so.sym` (+ `--extension`) and read in place, so a Gradle build can point it at `merged_native_libs/` without re-zipping. One upload per GNU build-id across all - paths. A directory with no matching libraries exits 10; an empty zip still only warns. See #69. + paths. A directory with no matching libraries exits 10; an empty zip still only warns. Directory + symlinks are not descended (file symlinks are read), and any I/O error while scanning (an + unreadable subdirectory or library, a dangling link) fails with exit 11 rather than uploading a + partial set. See #69. + +### Changed +- **`--type elf` collects every input before uploading anything.** A corrupt second zip now aborts + (exit 11) before the first zip uploads, instead of after. A path that does not exist now exits + 10 (input not found) rather than 11; both are in the no-fallback range. ## [0.7.13] - 2026-10-07 diff --git a/README.md b/README.md index 495165f..2c36897 100644 --- a/README.md +++ b/README.md @@ -65,7 +65,7 @@ The upload flow itself. ProGuard, Rust, ELF, dSYM, PDB, sourcemap, and Unity IL2 #### Android NDK β€” `--type elf` -Takes one or more paths: AGP's pre-built `native-debug-symbols.zip`, and/or directories, which are walked recursively for libraries (e.g. `build/intermediates/merged_native_libs/`) and read in place β€” nothing is re-zipped. Every library found is uploaded as its own symbol, keyed by its GNU build-id β€” `--uuid` (the SDK's `BUILD_UUID`) is still required but only correlates logs. Built-in entry names are `.so`, `.so.dbg` (`ndk.debugSymbolLevel = 'FULL'`) and `.so.sym` (`'SYMBOL_TABLE'`, function names only β€” no `file:line`). A library without a build-id is warned about and skipped; two entries with the same build-id upload once (across all the given paths), preferring the one with DWARF. The server dedups on the build-id, so switching a library from `SYMBOL_TABLE` to `FULL` needs `--force` to replace the stored symbols. +Takes one or more paths: AGP's pre-built `native-debug-symbols.zip`, and/or directories, which are walked recursively for libraries (e.g. `build/intermediates/merged_native_libs/`) and read in place β€” nothing is re-zipped. Directory symlinks are not descended; an unreadable entry fails the run (exit 11) and a directory with no libraries exits 10. Every library found is uploaded as its own symbol, keyed by its GNU build-id β€” `--uuid` (the SDK's `BUILD_UUID`) is still required but only correlates logs. Built-in entry names are `.so`, `.so.dbg` (`ndk.debugSymbolLevel = 'FULL'`) and `.so.sym` (`'SYMBOL_TABLE'`, function names only β€” no `file:line`). A library without a build-id is warned about and skipped; two entries with the same build-id upload once (across all the given paths), preferring the one with DWARF. The server dedups on the build-id, so switching a library from `SYMBOL_TABLE` to `FULL` needs `--force` to replace the stored symbols. #### Unity IL2CPP β€” `--type il2cpp-linemap` diff --git a/scripts/e2e_flows.py b/scripts/e2e_flows.py index 2e586e0..4cd6e83 100644 --- a/scripts/e2e_flows.py +++ b/scripts/e2e_flows.py @@ -241,6 +241,12 @@ def make_fixtures(fix): import zipfile as zf with zf.ZipFile(os.path.join(fix, "native-debug-symbols.zip"), "w") as z: z.writestr("arm64-v8a/libfoo.so", b"\x7fELF" + b"\x02\x01\x01\x00" + b"\x00" * 256) + # AGP's merged_native_libs layout: a directory of unstripped libraries (a REAL + # ELF with a GNU build-id, so the flow registers + uploads one symbol). + libdir = os.path.join(fix, "native_libs", "lib", "arm64-v8a") + os.makedirs(libdir, exist_ok=True) + shutil.copy(os.path.join(os.path.dirname(os.path.abspath(__file__)), "..", + "tests", "fixtures", "elf", "libsymbol1.so"), libdir) # A Rust MSVC build drops the PDB into target// next to unrelated # files, so the flow points at a directory, not the file. tgt = os.path.join(fix, "target", "release") @@ -488,6 +494,17 @@ def main(): results["elf"] = run(binpath, "elf", ["debug-files", "upload", "--type", "elf", os.path.join(fix, "native-debug-symbols.zip"), "--uuid", "11111111-2222-3333-4444-555555555555"] + v) + results["elf_dir"] = run(binpath, "elf_dir", ["debug-files", "upload", "--type", "elf", + os.path.join(fix, "native_libs"), + "--uuid", "11111111-2222-3333-4444-555555555555"] + v) + try: + posted = json.load(open(cappath("elf_dir__symbols_post.json"))) + ok = (posted.get("uuid") == "bca64abfec40dbb631bb8f1c37414472" + and STATE["puts"].get("elf_dir") == 1) + except Exception as e: + print(" [FAIL] could not verify the elf_dir flow:", e) + ok = False + results["elf_dir_keyed_by_build_id"] = ok results["pdb"] = run(binpath, "pdb", ["debug-files", "upload", "--type", "pdb", os.path.join(fix, "target", "release")] + v) if os.path.isdir(os.path.join(fix, "App.dSYM")): diff --git a/src/cli/debug_files.rs b/src/cli/debug_files.rs index dacb029..f5ebf75 100644 --- a/src/cli/debug_files.rs +++ b/src/cli/debug_files.rs @@ -19,7 +19,9 @@ pub enum DebugFilesCommand { Upload { /// One or more directories or files to scan. For `--type elf`, a directory /// is walked recursively for native libraries (e.g. AGP's - /// `merged_native_libs`), and a file is a `native-debug-symbols.zip`. + /// `merged_native_libs`; directory symlinks are not descended, an I/O error + /// fails the run, and no match exits 10), and a file is a + /// `native-debug-symbols.zip`. #[arg(required = true)] paths: Vec, @@ -1190,14 +1192,19 @@ async fn run_elf_upload( let input_dir = work_dir.path().join(n.to_string()); std::fs::create_dir_all(&input_dir)?; let found = elf::collect_libs(input, &input_dir, extra).map_err(|e| { - if e.kind() == std::io::ErrorKind::NotFound { + if e.kind() == std::io::ErrorKind::NotFound && !input.exists() { input_not_found(format!("{} does not exist", input.display())) } else if input.is_dir() { input_invalid(format!("cannot scan {}: {e}", input.display())) } else { + let hint = if input.extension().is_some_and(|x| x == "so") { + " (a single library is not accepted: pass its directory)" + } else { + "" + }; input_invalid(format!( "--type elf expects a directory or a native-debug-symbols zip; \ - cannot read {}: {e}", + cannot read {}: {e}{hint}", input.display() )) } @@ -1207,8 +1214,16 @@ async fn run_elf_upload( // miswired path or a task that ran before the libraries were merged, // and exit 0 would let the build go green with no native symbols. // (An empty ZIP stays a warning, as it always was.) + let has_zip = std::fs::read_dir(input).is_ok_and(|mut d| { + d.any(|e| e.is_ok_and(|e| e.path().extension().is_some_and(|x| x == "zip"))) + }); + let hint = if has_zip { + " (it contains a .zip: zips inside a directory are not opened; pass the zip itself)" + } else { + "" + }; return Err(input_not_found(format!( - "no .so / .so.dbg / .so.sym files (or --extension suffix) under {}", + "no .so / .so.dbg / .so.sym files (or --extension suffix) under {}{hint}", input.display() ))); } @@ -1224,68 +1239,67 @@ async fn run_elf_upload( } libs.extend(found); } - { - let libs = elf::keep_richest_per_build_id(libs); - let total = libs.len(); - - // A `.so` with no GNU build-id can never be matched at crash time, so - // warn + skip rather than fake an identity (never the build UUID). - let mut uploadable: Vec = Vec::new(); - for lib in libs { - if lib.build_id.is_some() { - uploadable.push(lib); - } else { - skipped_no_build_id += 1; - tracing::warn!( - lib = %lib.name, - arch = %lib.arch, - "native library has no GNU build-id; skipping β€” it cannot be \ - symbolicated. Build the library with -Wl,--build-id." - ); - } + + let libs = elf::keep_richest_per_build_id(libs); + let total = libs.len(); + + // A `.so` with no GNU build-id can never be matched at crash time, so + // warn + skip rather than fake an identity (never the build UUID). + let mut uploadable: Vec = Vec::new(); + for lib in libs { + if lib.build_id.is_some() { + uploadable.push(lib); + } else { + skipped_no_build_id += 1; + tracing::warn!( + lib = %lib.name, + arch = %lib.arch, + "native library has no GNU build-id; skipping β€” it cannot be \ + symbolicated. Build the library with -Wl,--build-id." + ); } + } + tracing::info!( + libraries = total, + uploadable = uploadable.len(), + "collected native libraries" + ); + + if total == 0 { + // Already warned per input above. + } else if dry_run { tracing::info!( - libraries = total, - uploadable = uploadable.len(), - "collected native libraries" + "dry-run: would register + upload {} libraries", + uploadable.len() ); + } else if uploadable.is_empty() { + tracing::warn!("no native libraries with a GNU build-id β€” nothing to upload"); + } else { + let client = client.as_ref().expect("client constructed when !dry_run"); - if total == 0 { - // Already warned per input above. - } else if dry_run { - tracing::info!( - "dry-run: would register + upload {} libraries", - uploadable.len() - ); - } else if uploadable.is_empty() { - tracing::warn!("no native libraries with a GNU build-id β€” nothing to upload"); - } else { - let client = client.as_ref().expect("client constructed when !dry_run"); - - // Run each `.so`'s pack β†’ register β†’ dedup-or-PUT pipeline concurrently. - let outcomes: Vec<(bool, anyhow::Result)> = - futures_util::stream::iter(uploadable.into_iter().enumerate()) - .map(|(i, lib)| { - let client = client.clone(); - let work = work_dir.path().to_path_buf(); - async move { - let symbol_table_only = lib.name.ends_with(".so.sym"); - let outcome = upload_one_so(&client, ctx, &lib, i, &work).await; - (symbol_table_only, outcome) - } - }) - .buffer_unordered(ELF_UPLOAD_CONCURRENCY) - .collect() - .await; - - for (symbol_table_only, outcome) in outcomes { - match outcome? { - presigned::Outcome::Uploaded => uploaded += 1, - presigned::Outcome::AlreadyExists => { - already_existed += 1; - if !symbol_table_only { - full_already_existed += 1; - } + // Run each `.so`'s pack β†’ register β†’ dedup-or-PUT pipeline concurrently. + let outcomes: Vec<(bool, anyhow::Result)> = + futures_util::stream::iter(uploadable.into_iter().enumerate()) + .map(|(i, lib)| { + let client = client.clone(); + let work = work_dir.path().to_path_buf(); + async move { + let symbol_table_only = lib.name.ends_with(".so.sym"); + let outcome = upload_one_so(&client, ctx, &lib, i, &work).await; + (symbol_table_only, outcome) + } + }) + .buffer_unordered(ELF_UPLOAD_CONCURRENCY) + .collect() + .await; + + for (symbol_table_only, outcome) in outcomes { + match outcome? { + presigned::Outcome::Uploaded => uploaded += 1, + presigned::Outcome::AlreadyExists => { + already_existed += 1; + if !symbol_table_only { + full_already_existed += 1; } } } diff --git a/src/symbols/elf.rs b/src/symbols/elf.rs index 5e9b809..57a3dec 100644 --- a/src/symbols/elf.rs +++ b/src/symbols/elf.rs @@ -140,25 +140,19 @@ pub fn collect_libs( /// Walk `dir` recursively for native libraries (same name rules as the zip /// entries), reading each one's identity in place. `name` is the path relative -/// to `dir`. Directory symlinks are not descended, file symlinks are read; unreadable entries are -/// skipped with a warning rather than failing the whole scan. Sorted by path so -/// the "first entry wins" tie-break in [`keep_richest_per_build_id`] is stable. +/// to `dir`. Directory symlinks are not descended, file symlinks are read. Any +/// I/O error (unreadable subdirectory, unreadable library, dangling link) fails +/// the scan: skipping would upload a partial set (a whole ABI missing) and let +/// the build go green, the same way the zip path fails on a read error. Sorted +/// by path so the "first entry wins" tie-break in [`keep_richest_per_build_id`] +/// is stable. pub fn scan_dir(dir: &Path, extra: &ExtraSuffixes) -> std::io::Result> { let mut libs = Vec::new(); for entry in walkdir::WalkDir::new(dir) .follow_links(false) .sort_by_file_name() { - let entry = match entry { - Ok(e) => e, - // An unreadable root means nothing was scanned: fail, never exit 0 with - // a misleading "pass --extension" hint. - Err(e) if e.depth() == 0 => return Err(e.into()), - Err(e) => { - tracing::warn!(error = %e, "skipping unreadable path while scanning for native libraries"); - continue; - } - }; + let entry = entry?; // Directory symlinks are never descended (a walk must stay under the // root); a symlink to a FILE is a candidate and `read` follows it. let ft = entry.file_type(); @@ -169,13 +163,9 @@ pub fn scan_dir(dir: &Path, extra: &ExtraSuffixes) -> std::io::Result b, - Err(e) => { - tracing::warn!(path = %entry.path().display(), error = %e, "skipping unreadable native library"); - continue; - } - }; + let bytes = std::fs::read(entry.path()).map_err(|e| { + std::io::Error::new(e.kind(), format!("{}: {e}", entry.path().display())) + })?; let ElfIdentity { build_id, arch, diff --git a/tests/elf_upload.rs b/tests/elf_upload.rs index 54a1628..bbe7954 100644 --- a/tests/elf_upload.rs +++ b/tests/elf_upload.rs @@ -486,24 +486,87 @@ async fn elf_upload_keeps_same_named_entries_of_different_zips_apart() { } /// An unreadable/nonexistent directory root must not exit 0. +/// Any I/O error while scanning a directory fails the run (exit 11): skipping +/// would upload a partial set β€” a whole ABI missing β€” behind a green build. #[cfg(unix)] #[test] -fn elf_upload_fails_on_an_unreadable_directory() { +fn elf_upload_fails_on_unreadable_directory_content() { use std::os::unix::fs::PermissionsExt; + let fixture = Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures/elf/libsymbol1.so"); + let lock = |p: &Path| { + std::fs::set_permissions(p, std::fs::Permissions::from_mode(0o000)).unwrap(); + // Root bypasses permission checks; nothing to assert then. + std::fs::read_dir(p).is_err() && std::fs::read(p).is_err() + }; + let unlock = |p: &Path| { + std::fs::set_permissions(p, std::fs::Permissions::from_mode(0o755)).unwrap(); + }; + let run = |root: &Path| { + let inputs = vec![root.to_string_lossy().into_owned()]; + common::cli() + .args(elf_args("http://127.0.0.1:1", &inputs)) + .assert() + .code(11) + }; + + // unreadable root let tmp = tempfile::tempdir().unwrap(); - let locked = tmp.path().join("locked"); - std::fs::create_dir_all(&locked).unwrap(); - std::fs::set_permissions(&locked, std::fs::Permissions::from_mode(0o000)).unwrap(); - // Root bypasses permission checks; nothing to assert then. - let readable = std::fs::read_dir(&locked).is_ok(); - let inputs = vec![locked.to_string_lossy().into_owned()]; - let assert = common::cli() - .args(elf_args("http://127.0.0.1:1", &inputs)) - .assert(); - std::fs::set_permissions(&locked, std::fs::Permissions::from_mode(0o755)).unwrap(); - if !readable { - assert.failure(); + let root = tmp.path().join("locked"); + std::fs::create_dir_all(&root).unwrap(); + if lock(&root) { + run(&root); } + unlock(&root); + + // a good library plus an unreadable ABI subdirectory + let root = tmp.path().join("r2"); + std::fs::create_dir_all(root.join("x86_64")).unwrap(); + std::fs::copy(&fixture, root.join("libok.so")).unwrap(); + if lock(&root.join("x86_64")) { + run(&root); + } + unlock(&root.join("x86_64")); + + // a good library plus an unreadable library file + let root = tmp.path().join("r3"); + std::fs::create_dir_all(&root).unwrap(); + std::fs::copy(&fixture, root.join("libok.so")).unwrap(); + std::fs::copy(&fixture, root.join("libbad.so")).unwrap(); + if lock(&root.join("libbad.so")) { + run(&root); + } + unlock(&root.join("libbad.so")); + + // a dangling symlink named like a library + let root = tmp.path().join("r4"); + std::fs::create_dir_all(&root).unwrap(); + std::fs::copy(&fixture, root.join("libok.so")).unwrap(); + std::os::unix::fs::symlink(tmp.path().join("gone"), root.join("libdangling.so")).unwrap(); + run(&root); +} + +/// A directory holding only a zip, and a bare `.so`, get pointed at the right input. +#[test] +fn elf_upload_hints_for_a_zip_in_a_directory_and_a_bare_so() { + let tmp = tempfile::tempdir().unwrap(); + let dir = tmp.path().join("native-debug-symbols"); + std::fs::create_dir_all(&dir).unwrap(); + pack_native_zip(&dir, "arm64-v8a/libsymbol1.so"); + let inputs = vec![dir.to_string_lossy().into_owned()]; + common::cli() + .args(elf_args("http://127.0.0.1:1", &inputs)) + .assert() + .code(10) + .stderr(predicates::str::contains("pass the zip itself")); + + let so = tmp.path().join("libx.so"); + std::fs::write(&so, b"x").unwrap(); + let inputs = vec![so.to_string_lossy().into_owned()]; + common::cli() + .args(elf_args("http://127.0.0.1:1", &inputs)) + .assert() + .code(11) + .stderr(predicates::str::contains("pass its directory")); } /// A directory that exists but holds no libraries is a miswired path / too-early @@ -519,7 +582,20 @@ fn elf_upload_empty_directory_is_input_not_found() { .stderr(predicates::str::contains("no .so")); } +/// The fixture with its GNU build-id's first byte flipped: same ELF, different identity. +fn fixture_with_other_build_id() -> (Vec, String) { + let fixture = Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures/elf/libsymbol1.so"); + let mut bytes = std::fs::read(&fixture).unwrap(); + let id = hex::decode(FIXTURE_BUILD_ID).unwrap(); + let pos = bytes.windows(id.len()).position(|w| w == id).unwrap(); + bytes[pos] ^= 0xff; + let other = hex::encode(&bytes[pos..pos + id.len()]); + (bytes, other) +} + /// Directory symlinks are not descended; a symlink to a library file is read. +/// The escaped library has its OWN build-id, so following the link would make it +/// 2 libraries (same-id copies would be collapsed by dedup and prove nothing). #[cfg(unix)] #[test] fn elf_dry_run_does_not_descend_directory_symlinks_but_reads_file_symlinks() { @@ -527,7 +603,11 @@ fn elf_dry_run_does_not_descend_directory_symlinks_but_reads_file_symlinks() { let fixture = Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures/elf/libsymbol1.so"); let outside = tmp.path().join("outside"); std::fs::create_dir_all(&outside).unwrap(); - std::fs::copy(&fixture, outside.join("libescaped.so")).unwrap(); + std::fs::write( + outside.join("libescaped.so"), + fixture_with_other_build_id().0, + ) + .unwrap(); let root = tmp.path().join("root"); std::fs::create_dir_all(&root).unwrap(); std::os::unix::fs::symlink(&outside, root.join("linkdir")).unwrap(); From d7655c556ec00a8f1786c0bac1e0748ed8b3b36e Mon Sep 17 00:00:00 2001 From: Alexey Karimov Date: Wed, 7 Oct 2026 16:35:37 +0500 Subject: [PATCH 04/11] perf(upload): stream the presigned PUT body and the SHA-1 instead of reading whole files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit put_payload read the packed archive into memory and cloned it for every retry attempt, and sha1_hex_of_file read it whole again; with up to 6 uploads in flight that held ~3x the archive size each. Both now stream in 64 KiB chunks (the PUT keeps an explicit Content-Length, which a presigned S3 PUT requires, and re-opens the file per attempt). Peak RSS, 4 x 100 MB libraries: 564 MB -> 143 MB. Shared by every presigned upload (elf, dsym, pdb, rust). πŸ€– Generated with [Claude Code](https://claude.com/claude-code) --- src/symbols/elf.rs | 42 +++++++++--- src/upload/presigned.rs | 142 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 173 insertions(+), 11 deletions(-) diff --git a/src/symbols/elf.rs b/src/symbols/elf.rs index 57a3dec..ca6e630 100644 --- a/src/symbols/elf.rs +++ b/src/symbols/elf.rs @@ -32,14 +32,21 @@ use symbolic_debuginfo::Archive; use super::suffix::ExtraSuffixes; -fn sha1_hex(bytes: &[u8]) -> String { - let digest: [u8; 20] = Sha1::digest(bytes).into(); - hex::encode(digest) -} - /// SHA-1 hex of a file's bytes β€” the wire `hash` for a per-`.so` upload. pub fn sha1_hex_of_file(path: &Path) -> std::io::Result { - Ok(sha1_hex(&std::fs::read(path)?)) + // Streamed: the hashed file is the packed archive, which can be tens of MB. + let mut file = std::fs::File::open(path)?; + let mut hasher = Sha1::new(); + let mut buf = [0u8; 64 * 1024]; + loop { + let n = file.read(&mut buf)?; + if n == 0 { + break; + } + hasher.update(&buf[..n]); + } + let digest: [u8; 20] = hasher.finalize().into(); + Ok(hex::encode(digest)) } /// One native library found in an AGP `native-debug-symbols.zip` or a scanned directory. @@ -163,9 +170,10 @@ pub fn scan_dir(dir: &Path, extra: &ExtraSuffixes) -> std::io::Result = (0..300_001u32).map(|i| (i % 251) as u8).collect(); + std::fs::write(&path, &data).unwrap(); + let expected: [u8; 20] = Sha1::digest(&data).into(); + assert_eq!(sha1_hex_of_file(&path).unwrap(), hex::encode(expected)); + std::fs::write(&path, b"").unwrap(); + assert_eq!( + sha1_hex_of_file(&path).unwrap(), + "da39a3ee5e6b4b0d3255bfef95601890afd80709", + "SHA-1 of the empty input" + ); + } + #[test] fn parse_elf_identity_on_non_elf_returns_none_unknown() { let id = parse_elf_identity(b"this is not an ELF file"); diff --git a/src/upload/presigned.rs b/src/upload/presigned.rs index 848dcb2..5a2c119 100644 --- a/src/upload/presigned.rs +++ b/src/upload/presigned.rs @@ -228,11 +228,20 @@ pub async fn put_payload( payload: &Path, ) -> Result<()> { tracing::debug!(url = %http::redact_url(presigned_url), "PUT payload"); - let payload_bytes = tokio::fs::read(payload).await?; + // Stream the payload from disk: reading it whole (and cloning it per retry + // 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. + // 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).body(payload_bytes.clone()) + client + .put(presigned_url) + .header(reqwest::header::CONTENT_LENGTH, len) + .body(reqwest::Body::wrap_stream(file_chunks( + payload.to_path_buf(), + ))) }) .await?; @@ -248,6 +257,38 @@ 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 @@ -535,6 +576,101 @@ mod tests { assert!(matches!(reg, Registration::AlreadyExists), "got {reg:?}"); } + /// A payload bigger than the 64 KiB stream chunk and not a multiple of it. + fn patterned_payload(len: usize) -> (tempfile::NamedTempFile, Vec) { + use std::io::Write; + let data: Vec = (0..len).map(|i| (i % 251) as u8).collect(); + let mut f = tempfile::NamedTempFile::new().unwrap(); + f.write_all(&data).unwrap(); + (f, data) + } + + /// The PUT streams the file but must still go out with an exact + /// `Content-Length` (a presigned S3 PUT rejects a chunked body) and every byte. + #[tokio::test] + async fn put_payload_streams_the_whole_file_with_an_exact_content_length() { + let server = MockServer::start().await; + Mock::given(method("PUT")) + .and(path("/put")) + .respond_with(ResponseTemplate::new(200)) + .expect(1) + .mount(&server) + .await; + let (payload, data) = patterned_payload(300_001); + let client = http::build_client().unwrap(); + put_payload( + &client, + RetryPolicy::none(), + &format!("{}/put", server.uri()), + payload.path(), + ) + .await + .unwrap(); + + let reqs = server.received_requests().await.unwrap(); + assert_eq!(reqs.len(), 1); + assert_eq!( + reqs[0] + .headers + .get("content-length") + .map(|v| v.to_str().unwrap().to_owned()), + Some(data.len().to_string()) + ); + assert!( + reqs[0].headers.get("transfer-encoding").is_none(), + "must not be chunked" + ); + assert_eq!(reqs[0].body, data); + } + + /// Each retry re-opens the file: the second attempt must carry the full body, + /// not whatever a shared cursor had left over. + #[tokio::test] + async fn put_payload_retry_resends_the_complete_body() { + let server = MockServer::start().await; + Mock::given(method("PUT")) + .respond_with(ResponseTemplate::new(503)) + .up_to_n_times(1) + .mount(&server) + .await; + Mock::given(method("PUT")) + .respond_with(ResponseTemplate::new(200)) + .mount(&server) + .await; + let (payload, data) = patterned_payload(200_000); + let client = http::build_client().unwrap(); + put_payload( + &client, + RetryPolicy::fast(3), + &format!("{}/put", server.uri()), + payload.path(), + ) + .await + .unwrap(); + + let reqs = server.received_requests().await.unwrap(); + assert_eq!(reqs.len(), 2, "one 503 then one success"); + for r in &reqs { + assert_eq!(r.body, data, "every attempt sends the whole payload"); + } + } + + /// A payload that vanished is a reported error, not a panic or a hang. + #[tokio::test] + async fn put_payload_reports_a_missing_payload_file() { + let server = MockServer::start().await; + let client = http::build_client().unwrap(); + let err = put_payload( + &client, + RetryPolicy::none(), + &format!("{}/put", server.uri()), + Path::new("/nonexistent/payload.zip"), + ) + .await + .unwrap_err(); + assert!(!err.to_string().is_empty()); + } + #[tokio::test] async fn upload_skips_the_put_when_the_server_reports_the_nested_duplicate() { let server = MockServer::start().await; From 20a7eb35b45a765810aa2d37b560b591d820009c Mon Sep 17 00:00:00 2001 From: Alexey Karimov Date: Wed, 7 Oct 2026 16:35:51 +0500 Subject: [PATCH 05/11] perf(elf): memory-map libraries for the identity scan instead of reading them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit scan_dir read every library whole just to extract its build-id, arch and richness, all of which live in the headers/notes. Map the file read-only instead (memmap2 was already in the tree via symbolic-debuginfo): the largest-file transient disappears. An empty file, which cannot be mapped, is reported as having no build-id rather than failing the scan. Peak RSS, 4 x 100 MB libraries: 143 MB -> 36 MB; --dry-run 112 MB -> 5 MB. Separate commit because it is the only step needing `unsafe`. πŸ€– Generated with [Claude Code](https://claude.com/claude-code) --- Cargo.lock | 1 + Cargo.toml | 1 + src/symbols/elf.rs | 31 ++++++++++++++++++++++++++++--- 3 files changed, 30 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7bb9b0a..8b2ff33 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -215,6 +215,7 @@ dependencies = [ "hex", "libc", "md-5 0.11.0", + "memmap2", "plist", "predicates", "regex", diff --git a/Cargo.toml b/Cargo.toml index c5d7a7d..fc15b36 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -153,6 +153,7 @@ sha1 = "0.11" sha2 = "0.11" md-5 = "0.11" hex = "0.4" +memmap2 = "0.9" # File discovery walkdir = "2" diff --git a/src/symbols/elf.rs b/src/symbols/elf.rs index ca6e630..2c3a316 100644 --- a/src/symbols/elf.rs +++ b/src/symbols/elf.rs @@ -173,13 +173,27 @@ pub fn scan_dir(dir: &Path, extra: &ExtraSuffixes) -> std::io::Result std::io::Result Date: Wed, 7 Oct 2026 16:36:05 +0500 Subject: [PATCH 06/11] docs(changelog): note the streaming/mmap memory reduction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit πŸ€– Generated with [Claude Code](https://claude.com/claude-code) --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a517bc0..3eea103 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,10 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - **`--type elf` collects every input before uploading anything.** A corrupt second zip now aborts (exit 11) before the first zip uploads, instead of after. A path that does not exist now exits 10 (input not found) rather than 11; both are in the no-fallback range. +- **Presigned symbol uploads no longer hold the whole archive in memory.** The PUT body and the + SHA-1 are streamed from disk in 64 KiB chunks (the PUT keeps an exact `Content-Length` and + re-opens the file per retry), and the ELF identity scan memory-maps libraries instead of reading + them. Peak RSS for four 100 MB libraries dropped from 564 MB to 36 MB. Wire format unchanged. ## [0.7.13] - 2026-10-07 From 7b454ac55836488c3feb8db556d9dcedbcda2519 Mon Sep 17 00:00:00 2001 From: Alexey Karimov Date: Wed, 7 Oct 2026 16:41:19 +0500 Subject: [PATCH 07/11] perf: stop loading whole files into memory across the upload/identify paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit of every std::fs::read / read_to_string / tokio::fs::read outside tests: - upload build artefact PUT (IPA/APK/AAB, hundreds of MB) and the build-info PUT now stream from disk via a shared http::file_body (+ exact Content-Length), instead of reading whole and cloning per retry - ProGuard identify: MD5 + SHA-1 in one streamed pass (was whole-file read) - dSYM / PDB / Rust ELF / `xcode` IPA Mach-O / `dsym` identity reads use a shared symbols::mapped::map_file (the single unsafe, empty-file safe) instead of copying 100+ MB binaries to read a UUID from the headers - ELF zip input: stream entries to disk, then map for identity - source maps: streamed SHA-1/size; read_debug_id maps the file (non-UTF-8 still an I/O error, exit 10) Peak RSS: upload build (300 MB artefact + 200 MB mapping) 661 -> 32 MB; 200 MB ProGuard mapping 241 -> 32 MB; ELF zip of 4 x 100 MB 463 -> 36 MB; 89 MB source map 274 -> 183 MB (JSON tree remains). πŸ€– Generated with [Claude Code](https://claude.com/claude-code) --- CHANGELOG.md | 8 +++++ src/cli/debug_files.rs | 2 +- src/cli/dsym.rs | 2 +- src/cli/xcode_ipa.rs | 2 +- src/inject/mod.rs | 9 +++-- src/symbols/dsym.rs | 2 +- src/symbols/elf.rs | 49 ++++++++++++-------------- src/symbols/mapped.rs | 52 ++++++++++++++++++++++++++++ src/symbols/mod.rs | 1 + src/symbols/pdb.rs | 2 +- src/symbols/proguard.rs | 51 +++++++++++++++++++++++---- src/symbols/rust.rs | 2 +- src/symbols/sourcemap.rs | 55 ++++++++++++++++++++++++----- src/upload/build.rs | 74 ++++++++++++++++++++++++++++++++++++++-- src/upload/build_info.rs | 10 +++--- src/upload/http.rs | 46 +++++++++++++++++++++++++ src/upload/presigned.rs | 38 ++------------------- 17 files changed, 312 insertions(+), 93 deletions(-) create mode 100644 src/symbols/mapped.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 3eea103..3a1065e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,14 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). SHA-1 are streamed from disk in 64 KiB chunks (the PUT keeps an exact `Content-Length` and re-opens the file per retry), and the ELF identity scan memory-maps libraries instead of reading them. Peak RSS for four 100 MB libraries dropped from 564 MB to 36 MB. Wire format unchanged. +- **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 f5ebf75..b1d91a2 100644 --- a/src/cli/debug_files.rs +++ b/src/cli/debug_files.rs @@ -1952,7 +1952,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..7152667 100644 --- a/src/cli/dsym.rs +++ b/src/cli/dsym.rs @@ -155,7 +155,7 @@ fn push_slices_from_macho(path: &Path, out: &mut Vec) { return; } } - let data = match std::fs::read(path) { + let data = match crate::symbols::mapped::map_file(path) { Ok(d) => d, Err(_) => return, }; 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 2c3a316..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,27 +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 only hazard is another process + // truncating the file while it is being parsed, which the build output + // and symbol files scanned here do not do. + 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..483fc83 100644 --- a/src/upload/build.rs +++ b/src/upload/build.rs @@ -184,13 +184,16 @@ 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"); + // 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::send_with_retry(policy, "artefact PUT", true, || { client .put(®.artifact_endpoint) .header(reqwest::header::CONTENT_TYPE, "application/octet-stream") - .body(body.clone()) + .header(reqwest::header::CONTENT_LENGTH, body_len) + .body(http::file_body(&zip_path)) }) .await?; if !put.status().is_success() { @@ -472,6 +475,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..8deee9e 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. // @@ -153,7 +152,8 @@ pub async fn run(params: Params<'_>, policy: RetryPolicy) -> Result { client .put(&presigned) .header(reqwest::header::CONTENT_TYPE, "application/octet-stream") - .body(body.clone()) + .header(reqwest::header::CONTENT_LENGTH, body_len) + .body(http::file_body(&zip_path)) }) .await?; diff --git a/src/upload/http.rs b/src/upload/http.rs index 56712e8..4bef074 100644 --- a/src/upload/http.rs +++ b/src/upload/http.rs @@ -215,6 +215,52 @@ 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()) +} + #[cfg(test)] mod tests { use super::*; diff --git a/src/upload/presigned.rs b/src/upload/presigned.rs index 5a2c119..63724d4 100644 --- a/src/upload/presigned.rs +++ b/src/upload/presigned.rs @@ -232,16 +232,14 @@ 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(); + let len = http::file_len(payload).await?; // 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(), - ))) + .body(http::file_body(payload)) }) .await?; @@ -257,38 +255,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 From 01e17d9c20b57cd5b8b8181776eea14f87821b60 Mon Sep 17 00:00:00 2001 From: Alexey Karimov Date: Wed, 7 Oct 2026 17:26:04 +0500 Subject: [PATCH 08/11] test(elf): gate the unix-only fixture helper (dead code is an error on Windows CI) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The helper's only user is a #[cfg(unix)] test, so the Windows test jobs failed to compile under -D warnings. πŸ€– Generated with [Claude Code](https://claude.com/claude-code) --- tests/elf_upload.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/elf_upload.rs b/tests/elf_upload.rs index bbe7954..62eeebd 100644 --- a/tests/elf_upload.rs +++ b/tests/elf_upload.rs @@ -583,6 +583,9 @@ fn elf_upload_empty_directory_is_input_not_found() { } /// The fixture with its GNU build-id's first byte flipped: same ELF, different identity. +/// (Unix-only because its sole user is: an unused helper is a hard error under CI's +/// `-D warnings` on Windows.) +#[cfg(unix)] fn fixture_with_other_build_id() -> (Vec, String) { let fixture = Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures/elf/libsymbol1.so"); let mut bytes = std::fs::read(&fixture).unwrap(); From 1ab176ab556b0f582243a22efcde2fcf23c62989 Mon Sep 17 00:00:00 2001 From: Alexey Karimov Date: Wed, 7 Oct 2026 17:28:54 +0500 Subject: [PATCH 09/11] docs(elf): state the mmap contract - directories must not be written while scanned MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review (Cursor, #72): a library truncated or rewritten under its mapping is undefined behaviour (SIGBUS in practice). That is acceptable for build-finalized outputs, but the constraint is now stated where integrators read it: the SAFETY note, `--help` for `paths`, and the changelog. πŸ€– Generated with [Claude Code](https://claude.com/claude-code) --- CHANGELOG.md | 3 +++ src/cli/debug_files.rs | 4 +++- src/symbols/elf.rs | 8 +++++--- 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3eea103..b0d127d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,9 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). SHA-1 are streamed from disk in 64 KiB chunks (the PUT keeps an exact `Content-Length` and re-opens the file per retry), and the ELF identity scan memory-maps libraries instead of reading them. Peak RSS for four 100 MB libraries dropped from 564 MB to 36 MB. Wire format unchanged. + 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. ## [0.7.13] - 2026-10-07 diff --git a/src/cli/debug_files.rs b/src/cli/debug_files.rs index f5ebf75..c931c20 100644 --- a/src/cli/debug_files.rs +++ b/src/cli/debug_files.rs @@ -21,7 +21,9 @@ pub enum DebugFilesCommand { /// is walked recursively for native libraries (e.g. AGP's /// `merged_native_libs`; directory symlinks are not descended, an I/O error /// fails the run, and no match exits 10), and a file is a - /// `native-debug-symbols.zip`. + /// `native-debug-symbols.zip`. The libraries are memory-mapped while scanned, so + /// they must be finished build outputs: do not run this while a linker is still + /// writing into the directory. #[arg(required = true)] paths: Vec, diff --git a/src/symbols/elf.rs b/src/symbols/elf.rs index 2c3a316..a445573 100644 --- a/src/symbols/elf.rs +++ b/src/symbols/elf.rs @@ -179,9 +179,11 @@ pub fn scan_dir(dir: &Path, extra: &ExtraSuffixes) -> std::io::Result Date: Wed, 7 Oct 2026 17:39:06 +0500 Subject: [PATCH 10/11] fix(upload): follow 307/308 on streamed PUTs ourselves; correct stale docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review (Fable): reqwest follows a 307/308 only when it can replay the body, and a streamed body cannot be replayed, so any redirect on a symbol / artefact / build-info PUT became a hard failure (exit 30) where it used to be followed transparently. S3 answers a freshly created bucket's presigned URL with exactly such a temporary 307. http::put_file now owns the streaming PUT for all three call sites: retries like send_with_retry, re-issues the PUT (new stream from the file, same Content-Length and Content-Type) to Location on 307/308, up to 5 hops, and passes an unusable redirect through for the caller to report. Also: send_with_retry's doc no longer claims callers clone the body, and the mapped.rs note no longer calls its unsafe the crate's only one. πŸ€– Generated with [Claude Code](https://claude.com/claude-code) --- src/symbols/mapped.rs | 5 +- src/upload/build.rs | 15 ++-- src/upload/build_info.rs | 15 ++-- src/upload/http.rs | 165 ++++++++++++++++++++++++++++++++++++++- src/upload/presigned.rs | 44 +++++++++-- 5 files changed, 218 insertions(+), 26 deletions(-) diff --git a/src/symbols/mapped.rs b/src/symbols/mapped.rs index 35ace55..0709ed8 100644 --- a/src/symbols/mapped.rs +++ b/src/symbols/mapped.rs @@ -3,8 +3,9 @@ //! Identifying a debug file (ELF build-id, Mach-O UUID, PDB GUID) touches only //! its headers, yet the files are routinely 100+ MB and `std::fs::read` copies //! every byte onto the heap. Mapping instead keeps the cost to the pages the -//! parser actually reads. This is the ONLY place the crate maps a file, so the -//! `unsafe` lives here. +//! parser actually reads. This is the ONLY place the crate memory-maps a file, so +//! that `unsafe` lives here (the crate has two other, unrelated `unsafe` uses: the +//! `fork` in `daemon.rs` and `gethostname` in `build_env.rs`). use std::ops::Deref; use std::path::Path; diff --git a/src/upload/build.rs b/src/upload/build.rs index 483fc83..b6f3ddb 100644 --- a/src/upload/build.rs +++ b/src/upload/build.rs @@ -188,13 +188,14 @@ pub async fn run(params: Params<'_>, policy: RetryPolicy) -> Result { // 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::send_with_retry(policy, "artefact PUT", true, || { - client - .put(®.artifact_endpoint) - .header(reqwest::header::CONTENT_TYPE, "application/octet-stream") - .header(reqwest::header::CONTENT_LENGTH, body_len) - .body(http::file_body(&zip_path)) - }) + 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(); diff --git a/src/upload/build_info.rs b/src/upload/build_info.rs index 8deee9e..31a884f 100644 --- a/src/upload/build_info.rs +++ b/src/upload/build_info.rs @@ -148,13 +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") - .header(reqwest::header::CONTENT_LENGTH, body_len) - .body(http::file_body(&zip_path)) - }) + 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 4bef074..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. @@ -261,12 +261,173 @@ 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 63724d4..99711fb 100644 --- a/src/upload/presigned.rs +++ b/src/upload/presigned.rs @@ -232,16 +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 = http::file_len(payload).await?; // 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(http::file_body(payload)) - }) - .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() { @@ -622,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; From 308d4d0b8a3126baefaa94dbc73ec991f769ae2b Mon Sep 17 00:00:00 2001 From: Alexey Karimov Date: Wed, 7 Oct 2026 19:00:58 +0500 Subject: [PATCH 11/11] Remove file_chunks re-introduced by the squash merge of #72 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #73 replaced it with the shared http::file_body; the squash of #72 on main still carried the old copy, which the merge brought back as dead code. πŸ€– Generated with [Claude Code](https://claude.com/claude-code) --- src/upload/presigned.rs | 32 -------------------------------- 1 file changed, 32 deletions(-) diff --git a/src/upload/presigned.rs b/src/upload/presigned.rs index 8ddae01..99711fb 100644 --- a/src/upload/presigned.rs +++ b/src/upload/presigned.rs @@ -249,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