From 6aa0078fce83dd187f2d6f76a9cf406604334784 Mon Sep 17 00:00:00 2001 From: Alexey Karimov Date: Wed, 7 Oct 2026 15:17:46 +0500 Subject: [PATCH 01/14] 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/14] 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/14] 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/14] 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/14] 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/14] 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/14] 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 530ba3cdc7626e06e9fab0dd1b741e96c2008fe2 Mon Sep 17 00:00:00 2001 From: Alexey Karimov Date: Wed, 7 Oct 2026 16:56:22 +0500 Subject: [PATCH 08/14] perf(sourcemaps): stream map JSON with struson instead of a serde_json::Value tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A source map is tens of MB (a huge `mappings` string plus whole sources in `sourcesContent`), and parsing it into a Value held ~2x the file just to read or change a few top-level keys. New inject::mapjson walks the document once with struson (syntax still validated end to end, trailing garbage rejected): - read_debug_id / restamp_id carry-check: top-level string keys only - --strip-sources-content: filtered copy, drops sourcesContent at the top level and inside nested sections[].map, everything else copied verbatim - inject's map rewrite: two passes (read ids, then stream-copy with the ids set) into a sibling temp file, copied over the original so permissions / symlinks survive - the inject debug-id (UUIDv5 over len||bundle||map) is hashed incrementally with the map streamed from disk; pinned bit-for-bit to Uuid::new_v5 by a test across buffer boundaries Peak RSS, 89 MB map: upload 183 -> 32 MB, --strip-sources-content 262 -> 32 MB, sourcemaps inject 298 -> 6 MB. πŸ€– Generated with [Claude Code](https://claude.com/claude-code) --- CHANGELOG.md | 9 ++ Cargo.lock | 56 ++++++++ Cargo.toml | 4 + src/cli/debug_files.rs | 91 +++++------- src/inject/mapjson.rs | 313 +++++++++++++++++++++++++++++++++++++++++ src/inject/mod.rs | 174 +++++++++++++++-------- 6 files changed, 533 insertions(+), 114 deletions(-) create mode 100644 src/inject/mapjson.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a1065e..8899121 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,15 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). 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. +- **Source maps are streamed instead of parsed into a JSON tree.** Reading a map's debug-id, + stripping `sourcesContent` (`--strip-sources-content`, including nested indexed-map sections) and + `sourcemaps inject` rewriting a map's ids now walk the document with `struson`, and the inject + debug-id hash is computed incrementally instead of over a concatenated copy. Peak RSS for an + 89 MB map: upload 183 MB -> 32 MB, `--strip-sources-content` 262 MB -> 32 MB, `inject` 298 MB -> + 6 MB. Debug-ids are unchanged. Two byte-level differences: a stripped copy keeps the map's + original key order (it was alphabetical), and `inject` writes `debug_id` / `debugId` last. Both + are still valid maps; an invalid-UTF-8 map outside a string value now reports as invalid JSON + (exit 11) rather than I/O (exit 10). ## [0.7.13] - 2026-10-07 diff --git a/Cargo.lock b/Cargo.lock index 8b2ff33..3471bf1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -225,6 +225,7 @@ dependencies = [ "serde_json", "sha1", "sha2", + "struson", "symbolic-common", "symbolic-debuginfo", "tempfile", @@ -464,6 +465,17 @@ dependencies = [ "syn 2.0.117", ] +[[package]] +name = "duplicate" +version = "2.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8e92f10a49176cbffacaedabfaa11d51db1ea0f80a83c26e1873b43cd1742c24" +dependencies = [ + "heck", + "proc-macro2", + "proc-macro2-diagnostics", +] + [[package]] name = "elementtree" version = "1.2.3" @@ -1482,6 +1494,18 @@ dependencies = [ "unicode-ident", ] +[[package]] +name = "proc-macro2-diagnostics" +version = "0.10.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "af066a9c399a26e020ada66a034357a868728e72cd426f3adcd35f80d88d88c8" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.117", + "version_check", +] + [[package]] name = "quick-xml" version = "0.42.0" @@ -2000,6 +2024,38 @@ version = "0.11.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7da8b5736845d9f2fcb837ea5d9e2628564b3b043a70948a3f0b778838c5fb4f" +[[package]] +name = "strum" +version = "0.27.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "af23d6f6c1a224baef9d3f61e287d2761385a5b88fdab4eb4c6f11aeb54c4bcf" +dependencies = [ + "strum_macros", +] + +[[package]] +name = "strum_macros" +version = "0.27.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7695ce3845ea4b33927c055a39dc438a45b059f7c1b3d91d38d10355fb8cbca7" +dependencies = [ + "heck", + "proc-macro2", + "quote", + "syn 2.0.117", +] + +[[package]] +name = "struson" +version = "0.7.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9a8eaaf563e9de627dadbe66ff8b7ef8f065fc69844c90ed6acdc90bdc9f0571" +dependencies = [ + "duplicate", + "strum", + "thiserror 2.0.21", +] + [[package]] name = "subtle" version = "2.6.1" diff --git a/Cargo.toml b/Cargo.toml index fc15b36..079fd2b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -172,6 +172,10 @@ tempfile = "3" # [dependencies] table (NOT a cfg(target) table). self-replace = "1" +# Streaming JSON reader/writer: lets the source-map paths read or rewrite a few +# top-level keys of a tens-of-MB map without building a serde_json::Value tree. +struson = "0.7" + # Unix double-fork for `xcode post-action`'s background (daemon) mode. This is a # TARGET-specific table, so it MUST come AFTER the main [dependencies] table β€” # placed mid-table, TOML would capture every following key into the unix-only diff --git a/src/cli/debug_files.rs b/src/cli/debug_files.rs index b1d91a2..feb10a8 100644 --- a/src/cli/debug_files.rs +++ b/src/cli/debug_files.rs @@ -1917,23 +1917,6 @@ fn is_stylesheet_or_declaration_map(path: &Path, extra: &ExtraSuffixes) -> bool }) } -/// Remove every `sourcesContent` a source map can carry; `true` when one was there. -/// -/// Not just the top-level key: an INDEXED map (spec Β§Index-Map) keeps its source inside -/// `sections[i].map`, and those sections nest. Stripping only the top level left the source in the -/// upload while reporting success β€” the flag's whole promise, failing silently. -fn remove_sources_content(map: &mut serde_json::Map) -> bool { - let mut removed = map.remove("sourcesContent").is_some(); - if let Some(serde_json::Value::Array(sections)) = map.get_mut("sections") { - for section in sections { - if let Some(serde_json::Value::Object(inner)) = section.get_mut("map") { - removed |= remove_sources_content(inner); - } - } - } - removed -} - /// A copy of `map_path` with `sourcesContent` removed, plus its recomputed identity β€” or `None` /// when nothing needs stripping (flag off, no `sourcesContent`, or the map is not the JSON object we /// expect, which is a privacy preference's business to ignore rather than to fail over). @@ -1952,40 +1935,42 @@ 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 = 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); - }; - if !remove_sources_content(&mut map) { - return Ok(None); - } - let stripped = serde_json::to_vec(&map).map_err(|e| { - input_invalid(format!( - "{}: could not re-serialize the map without its sources: {e}", - map_path.display() - )) - })?; let name = map_path .file_name() .map(Path::new) .unwrap_or_else(|| Path::new("bundle.js.map")); let out = tmpdir.join(name); - std::fs::write(&out, &stripped).map_err(crate::error::Error::Io)?; + // Streamed input -> output: only the dropped `sourcesContent` values (the bulk of a + // map) are never held, and nothing else is either, so memory is flat however big the map. + let before_bytes = std::fs::metadata(map_path) + .map_err(crate::error::Error::Io)? + .len(); + let input = + std::io::BufReader::new(std::fs::File::open(map_path).map_err(crate::error::Error::Io)?); + let output = + std::io::BufWriter::new(std::fs::File::create(&out).map_err(crate::error::Error::Io)?); + match crate::inject::mapjson::strip_sources_content(input, output) { + Ok(true) => {} + // Nothing to strip, or not the JSON object we expect: the copy is moot. + Ok(false) | Err(crate::inject::mapjson::MapJsonError::Json(_)) => { + let _ = std::fs::remove_file(&out); + return Ok(None); + } + Err(crate::inject::mapjson::MapJsonError::Io(e)) => { + let _ = std::fs::remove_file(&out); + return Err(crate::error::Error::Io(e).into()); + } + } + // The identity describes what is UPLOADED (the stripped copy): its streamed hash and size. + // The debug-id is carried over untouched: it is the key, and re-deriving it here would + // break the pair with the bundle that `sourcemaps inject` stamped. + let identity = sourcemap::identify(&out)?; tracing::info!( path = %map_path.display(), - before_bytes = bytes.len(), - after_bytes = stripped.len(), + before_bytes, + after_bytes = identity.size_bytes, "stripped sourcesContent from the uploaded copy" ); - let identity = sourcemap::SourcemapIdentity { - debug_id: crate::inject::read_debug_id(&out)?, - content_sha1_hex: { - use sha1::Digest; - let digest: [u8; 20] = sha1::Sha1::digest(&stripped).into(); - hex::encode(digest) - }, - size_bytes: stripped.len() as u64, - }; Ok(Some((out, identity))) } @@ -3399,18 +3384,7 @@ mod sourcemap_upload_tests { } async fn uploaded_map_json(server: &MockServer) -> serde_json::Value { - let requests = server.received_requests().await.unwrap(); - let put = requests - .iter() - .find(|r| r.method.as_str() == "PUT") - .expect("no PUT"); - let mut archive = - zip::ZipArchive::new(std::io::Cursor::new(put.body.clone())).expect("not a zip"); - assert_eq!(archive.len(), 1, "one entry: just the map"); - let mut entry = archive.by_index(0).unwrap(); - let mut body = String::new(); - std::io::Read::read_to_string(&mut entry, &mut body).unwrap(); - serde_json::from_str(&body).expect("uploaded map is not JSON") + serde_json::from_slice(&uploaded_map_bytes(server).await).expect("uploaded map is not JSON") } /// `sourcesContent` embeds the ORIGINAL SOURCE in the map, which is how symbolication shows @@ -3495,14 +3469,17 @@ mod sourcemap_upload_tests { .body, ) .unwrap(); - let uploaded = uploaded_map_json(&server).await; + let uploaded = uploaded_map_bytes(&server).await; let expected = { use sha1::Digest; - let digest: [u8; 20] = - sha1::Sha1::digest(serde_json::to_vec(&uploaded).unwrap()).into(); + let digest: [u8; 20] = sha1::Sha1::digest(&uploaded).into(); hex::encode(digest) }; assert_eq!(post["hash"].as_str().unwrap(), expected); + assert!( + !String::from_utf8_lossy(&uploaded).contains("sourcesContent"), + "and the hash is of the STRIPPED bytes" + ); } /// Without the flag, the source rides along exactly as before β€” that is what makes the diff --git a/src/inject/mapjson.rs b/src/inject/mapjson.rs new file mode 100644 index 0000000..7d74c77 --- /dev/null +++ b/src/inject/mapjson.rs @@ -0,0 +1,313 @@ +//! Streaming reads and rewrites of source-map JSON. +//! +//! A source map is routinely tens of MB, mostly one huge `mappings` string and +//! an array of whole source files (`sourcesContent`). Parsing it into a +//! `serde_json::Value` held ~2x the file on the heap just to look at, or change, +//! a handful of top-level keys. Everything here walks the document once with +//! [`struson`] instead, so memory stays at the reader's small buffer plus the +//! one member name / id string being looked at β€” whatever the map's size. +//! +//! Syntax is still validated end to end: a skipped value is checked like a read +//! one, and trailing garbage after the top-level value is an error. + +use std::io::{Read, Write}; + +use struson::reader::{JsonReader, JsonStreamReader, ReaderError, ValueType}; +use struson::writer::{JsonStreamWriter, JsonWriter}; + +/// Why a streaming pass over a map failed. +#[derive(Debug)] +pub enum MapJsonError { + /// The input is not (this shape of) valid JSON. + Json(String), + /// Writing the rewritten copy failed. + Io(std::io::Error), +} + +impl std::fmt::Display for MapJsonError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + MapJsonError::Json(m) => f.write_str(m), + MapJsonError::Io(e) => write!(f, "{e}"), + } + } +} + +impl From for MapJsonError { + fn from(e: ReaderError) -> Self { + match e { + // Invalid UTF-8 inside a string surfaces as an I/O `InvalidData` error; + // keep it one (as `read_to_string` reported it) rather than "bad JSON". + ReaderError::IoError { error, .. } => MapJsonError::Io(error), + other => MapJsonError::Json(other.to_string()), + } + } +} + +impl From for MapJsonError { + fn from(e: struson::reader::TransferError) -> Self { + match e { + struson::reader::TransferError::ReaderError(r) => r.into(), + struson::reader::TransferError::WriterError(w) => MapJsonError::Io(w), + } + } +} + +impl From for MapJsonError { + fn from(e: std::io::Error) -> Self { + MapJsonError::Io(e) + } +} + +type Res = std::result::Result; + +/// What [`top_level_strings`] found. +pub struct TopLevel { + /// Whether the document's top-level value is an object. + pub is_object: bool, + /// One entry per requested key, in order: its value when it is a string, + /// else `None` (absent, or any other JSON type). With a duplicated key the + /// LAST occurrence wins, as it does in a parsed `serde_json::Value`. + pub values: Vec>, +} + +/// Read the string values of the given top-level keys, validating the whole +/// document. A top-level value that is not an object yields `is_object: false` +/// and no values (it is still syntax-checked). +pub fn top_level_strings(input: R, keys: &[&str]) -> Res { + let mut r = JsonStreamReader::new(input); + let mut values: Vec> = vec![None; keys.len()]; + let is_object = r.peek()? == ValueType::Object; + if is_object { + r.begin_object()?; + while r.has_next()? { + let idx = { + let name = r.next_name()?; + keys.iter().position(|k| *k == name) + }; + match idx { + Some(i) if r.peek()? == ValueType::String => values[i] = Some(r.next_string()?), + Some(i) => { + values[i] = None; + r.skip_value()?; + } + None => r.skip_value()?, + } + } + r.end_object()?; + } else { + r.skip_value()?; + } + r.consume_trailing_whitespace()?; + Ok(TopLevel { is_object, values }) +} + +/// Copy `input` to `output`, dropping every `sourcesContent` β€” at the top level +/// AND inside the (nestable) `sections[i].map` of an indexed map. Returns +/// whether anything was dropped. Everything else is copied verbatim, in its +/// original order, without being held in memory. +/// +/// Errors with [`MapJsonError::Json`] when the input is not a JSON object. +pub fn strip_sources_content(input: R, output: W) -> Res { + let mut r = JsonStreamReader::new(input); + let mut w = JsonStreamWriter::new(output); + if r.peek()? != ValueType::Object { + return Err(MapJsonError::Json("not a JSON object".into())); + } + let removed = strip_object(&mut r, &mut w)?; + r.consume_trailing_whitespace()?; + w.finish_document()?; + Ok(removed) +} + +fn strip_object(r: &mut R, w: &mut W) -> Res { + let mut removed = false; + r.begin_object()?; + w.begin_object()?; + while r.has_next()? { + let name = r.next_name()?.to_owned(); + if name == "sourcesContent" { + r.skip_value()?; + removed = true; + continue; + } + w.name(&name)?; + if name == "sections" && r.peek()? == ValueType::Array { + r.begin_array()?; + w.begin_array()?; + while r.has_next()? { + if r.peek()? == ValueType::Object { + removed |= strip_section(r, w)?; + } else { + r.transfer_to(w)?; + } + } + r.end_array()?; + w.end_array()?; + } else { + r.transfer_to(w)?; + } + } + r.end_object()?; + w.end_object()?; + Ok(removed) +} + +/// One `sections[i]` object: only its `map` (when an object) is searched. +fn strip_section(r: &mut R, w: &mut W) -> Res { + let mut removed = false; + r.begin_object()?; + w.begin_object()?; + while r.has_next()? { + let name = r.next_name()?.to_owned(); + w.name(&name)?; + if name == "map" && r.peek()? == ValueType::Object { + removed |= strip_object(r, w)?; + } else { + r.transfer_to(w)?; + } + } + r.end_object()?; + w.end_object()?; + Ok(removed) +} + +/// Copy `input` to `output` with the top-level `debug_id` and `debugId` set to +/// `debug_id` (any existing ones are dropped and the pair is written last). +/// Everything else is copied verbatim and in order. +/// +/// Errors with [`MapJsonError::Json`] when the input is not a JSON object. +pub fn set_debug_ids(input: R, output: W, debug_id: &str) -> Res<()> { + let mut r = JsonStreamReader::new(input); + let mut w = JsonStreamWriter::new(output); + if r.peek()? != ValueType::Object { + return Err(MapJsonError::Json("not a JSON object".into())); + } + r.begin_object()?; + w.begin_object()?; + while r.has_next()? { + let name = r.next_name()?.to_owned(); + if name == "debug_id" || name == "debugId" { + r.skip_value()?; + continue; + } + w.name(&name)?; + r.transfer_to(&mut w)?; + } + r.end_object()?; + w.name("debug_id")?; + w.string_value(debug_id)?; + w.name("debugId")?; + w.string_value(debug_id)?; + w.end_object()?; + r.consume_trailing_whitespace()?; + w.finish_document()?; + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn strip(s: &str) -> Res<(bool, String)> { + let mut out = Vec::new(); + let removed = strip_sources_content(s.as_bytes(), &mut out)?; + Ok((removed, String::from_utf8(out).unwrap())) + } + + #[test] + fn top_level_strings_picks_string_values_last_wins_and_ignores_other_types() { + let t = top_level_strings( + br#"{"debug_id": 5, "debugId": "camel", "uuid": "a", "uuid": "b", "x": {"debug_id": "nested"}}"#.as_slice(), + &["debug_id", "debugId", "uuid"], + ) + .unwrap(); + assert!(t.is_object); + assert_eq!( + t.values, + [None, Some("camel".into()), Some("b".into())], + "non-string -> None, duplicates -> last, nested keys ignored" + ); + // A later non-string occurrence replaces an earlier string, as in a Value. + let t = top_level_strings(br#"{"uuid": "a", "uuid": 1}"#.as_slice(), &["uuid"]).unwrap(); + assert_eq!(t.values, [None]); + } + + #[test] + fn top_level_strings_non_object_and_invalid_json() { + for ok in ["[]", "3", "\"s\"", "null"] { + let t = top_level_strings(ok.as_bytes(), &["uuid"]).unwrap(); + assert!(!t.is_object, "{ok}"); + assert_eq!(t.values, [None]); + } + for bad in [ + "", + "{", + r#"{"a":}"#, + r#"{"a":1} x"#, + r#"{"a":1}{"b":2}"#, + "[1,]", + ] { + assert!( + matches!( + top_level_strings(bad.as_bytes(), &["a"]), + Err(MapJsonError::Json(_)) + ), + "{bad:?} must be rejected" + ); + } + } + + #[test] + fn strip_removes_top_level_and_nested_section_sources_in_order() { + let (removed, out) = strip( + r#"{"version":3,"sourcesContent":["a"],"sections":[{"offset":{"line":0,"column":0},"map":{"sources":["x"],"sourcesContent":["secret"],"sections":[{"map":{"sourcesContent":["deep"],"names":[]}}]}},7],"mappings":"AAAA"}"#, + ) + .unwrap(); + assert!(removed); + assert_eq!( + out, + r#"{"version":3,"sections":[{"offset":{"line":0,"column":0},"map":{"sources":["x"],"sections":[{"map":{"names":[]}}]}},7],"mappings":"AAAA"}"# + ); + } + + #[test] + fn strip_reports_nothing_removed_and_ignores_non_object_sections() { + let (removed, out) = + strip(r#"{"sections":"nope","map":{"sourcesContent":[1]},"a":1.50}"#).unwrap(); + assert!(!removed, "only sections[].map is searched"); + assert!( + out.contains(r#""a":1.50"#), + "numbers keep their text: {out}" + ); + } + + #[test] + fn strip_rejects_non_objects_and_bad_json() { + assert!(matches!(strip("[]"), Err(MapJsonError::Json(_)))); + assert!(matches!(strip("{"), Err(MapJsonError::Json(_)))); + assert!(matches!( + strip(r#"{"a":1} junk"#), + Err(MapJsonError::Json(_)) + )); + } + + #[test] + fn set_debug_ids_replaces_existing_pair_and_keeps_the_rest() { + let mut out = Vec::new(); + set_debug_ids( + br#"{"debug_id":"old","version":3,"debugId":"older","mappings":"AA"}"#.as_slice(), + &mut out, + "new-id", + ) + .unwrap(); + assert_eq!( + String::from_utf8(out).unwrap(), + r#"{"version":3,"mappings":"AA","debug_id":"new-id","debugId":"new-id"}"# + ); + assert!(matches!( + set_debug_ids(b"[]".as_slice(), Vec::new(), "x"), + Err(MapJsonError::Json(_)) + )); + } +} diff --git a/src/inject/mod.rs b/src/inject/mod.rs index 1cd40c9..a028762 100644 --- a/src/inject/mod.rs +++ b/src/inject/mod.rs @@ -23,6 +23,7 @@ use uuid::Uuid; use crate::error::{Error, Result}; +pub mod mapjson; pub mod sri; /// Fixed namespace for Bugsee sourcemap debug-ids β€” keeps UUIDv5 generation @@ -54,15 +55,48 @@ pub fn compute_debug_id(content: &[u8]) -> Uuid { /// server while the upload reported the new one as already there. The bundle's /// length is hashed ahead of it, so the boundary between the two inputs cannot /// shift without changing the id. +#[cfg(test)] pub fn compute_debug_id_with_map(bundle: &[u8], map: Option<&[u8]>) -> Uuid { let Some(map) = map else { return compute_debug_id(bundle); }; - let mut input = Vec::with_capacity(8 + bundle.len() + map.len()); - input.extend_from_slice(&(bundle.len() as u64).to_le_bytes()); - input.extend_from_slice(bundle); - input.extend_from_slice(map); - Uuid::new_v5(&DEBUG_ID_NAMESPACE, &input) + // Reading a slice cannot fail. + hash_with_map(bundle, map).expect("reading from a slice cannot fail") +} + +/// [`compute_debug_id_with_map`] with the map STREAMED from `map_path` instead of +/// held in memory: a map is tens of MB, and concatenating it with the bundle +/// into one buffer just to hash it was a second full copy. +pub fn compute_debug_id_with_map_file(bundle: &[u8], map_path: &Path) -> std::io::Result { + hash_with_map( + bundle, + std::io::BufReader::new(std::fs::File::open(map_path)?), + ) +} + +/// UUIDv5 over `len(bundle) as u64 LE || bundle || map`, computed incrementally. +/// +/// A v5 UUID is the first 16 bytes of SHA-1(namespace || name) with the +/// version/variant bits set, so feeding the pieces to the hasher one at a time +/// gives the same id as `Uuid::new_v5` over their concatenation. +fn hash_with_map(bundle: &[u8], mut map: impl std::io::Read) -> std::io::Result { + use sha1::{Digest as _, Sha1}; + let mut hasher = Sha1::new(); + hasher.update(DEBUG_ID_NAMESPACE.as_bytes()); + hasher.update((bundle.len() as u64).to_le_bytes()); + hasher.update(bundle); + let mut buf = [0u8; 64 * 1024]; + loop { + let n = map.read(&mut buf)?; + if n == 0 { + break; + } + hasher.update(&buf[..n]); + } + let digest = hasher.finalize(); + let mut first16 = [0u8; 16]; + first16.copy_from_slice(&digest[..16]); + Ok(uuid::Builder::from_sha1_bytes(first16).into_uuid()) } /// The runtime stub appended to each JS bundle. On load it registers @@ -352,8 +386,10 @@ fn inject_one(js_path: &Path, dry_run: bool, stats: &mut InjectStats) -> Result< } }, None => { - let map_bytes = map_path.as_deref().map(std::fs::read).transpose()?; - let id = compute_debug_id_with_map(content.as_bytes(), map_bytes.as_deref()); + let id = match map_path.as_deref() { + Some(m) => compute_debug_id_with_map_file(content.as_bytes(), m)?, + None => compute_debug_id(content.as_bytes()), + }; if !dry_run { std::fs::write(js_path, format!("{content}{}", runtime_stub(&id)))?; } @@ -392,19 +428,18 @@ fn restamp_id<'a>( let Some(original) = content.strip_suffix(runtime_stub(&id).as_str()) else { return Ok(None); }; - let map_bytes = std::fs::read(map_path)?; + let file = std::fs::File::open(map_path)?; // A map that is not JSON is not re-keyed here: writing the id into it fails // next, and it must fail BEFORE the bundle is rewritten, as it always did. - let Ok(map) = serde_json::from_slice::(&map_bytes) else { + let Ok(top) = + mapjson::top_level_strings(std::io::BufReader::new(file), &["debug_id", "debugId"]) + else { return Ok(None); }; - let carries_id = ["debug_id", "debugId"] - .iter() - .any(|k| map.get(*k).and_then(serde_json::Value::as_str).is_some()); - if carries_id { + if top.values.iter().any(Option::is_some) { return Ok(None); } - let fresh = compute_debug_id_with_map(original.as_bytes(), Some(&map_bytes)); + let fresh = compute_debug_id_with_map_file(original.as_bytes(), map_path)?; Ok((fresh.to_string() != id).then_some((original, fresh))) } @@ -500,25 +535,15 @@ fn write_map_debug_id( freshly_computed: bool, dry_run: bool, ) -> Result { - let raw = std::fs::read_to_string(map_path)?; - let mut value: serde_json::Value = serde_json::from_str(&raw).map_err(|e| { - Error::InputInvalid(format!( - "sourcemap is not valid JSON: {e} ({})", - map_path.display() - )) - })?; - let obj = value.as_object_mut().ok_or_else(|| { - Error::InputInvalid(format!( + // Pass 1 (streamed, also validates the whole file): what ids does it carry? + let top = map_top_level(map_path, &["debug_id", "debugId"])?; + if !top.is_object { + return Err(Error::InputInvalid(format!( "sourcemap is not a JSON object: {}", map_path.display() - )) - })?; - let current = |k: &str| { - obj.get(k) - .and_then(serde_json::Value::as_str) - .map(str::to_string) - }; - let (snake, camel) = (current("debug_id"), current("debugId")); + ))); + } + let (snake, camel) = (top.values[0].clone(), top.values[1].clone()); match map_id_action( snake.as_deref(), camel.as_deref(), @@ -544,18 +569,28 @@ fn write_map_debug_id( return Ok(false); } } - obj.insert( - "debug_id".into(), - serde_json::Value::String(debug_id.to_string()), - ); - obj.insert( - "debugId".into(), - serde_json::Value::String(debug_id.to_string()), - ); if !dry_run { - let serialized = serde_json::to_string(&value) - .map_err(|e| Error::InputInvalid(format!("failed to serialize sourcemap JSON: {e}")))?; - std::fs::write(map_path, serialized)?; + // Pass 2: stream the map into a sibling temp file with the ids set, then + // copy it over the original (not rename: that would swap the inode and + // drop the permissions / symlink / hard-link the build gave the map). + let dir = map_path.parent().filter(|p| !p.as_os_str().is_empty()); + let tmp = match dir { + Some(d) => tempfile::NamedTempFile::new_in(d)?, + None => tempfile::NamedTempFile::new_in(".")?, + }; + { + let input = std::io::BufReader::new(std::fs::File::open(map_path)?); + let mut out = std::io::BufWriter::new(tmp.as_file()); + mapjson::set_debug_ids(input, &mut out, debug_id).map_err(|e| match e { + mapjson::MapJsonError::Io(e) => Error::Io(e), + mapjson::MapJsonError::Json(m) => Error::InputInvalid(format!( + "failed to rewrite sourcemap JSON: {m} ({})", + map_path.display() + )), + })?; + std::io::Write::flush(&mut out)?; + } + std::fs::copy(tmp.path(), map_path)?; } Ok(true) } @@ -564,22 +599,24 @@ 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> { - // 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} ({})", + // Walked as a stream: no copy of the map and no JSON tree, so a 100 MB map + // costs a few KB. Invalid UTF-8 inside a string stays an I/O `InvalidData` + // error, as it was with `read_to_string`. + let top = map_top_level(map_path, &["debug_id", "debugId", "uuid"])?; + Ok(top.values.into_iter().flatten().next()) +} + +/// Stream-read the string values of top-level `keys` of the map at `map_path`. +/// Not-JSON becomes `InputInvalid` (naming the file); an I/O failure stays `Io`. +fn map_top_level(map_path: &Path, keys: &[&str]) -> Result { + let file = std::io::BufReader::new(std::fs::File::open(map_path)?); + mapjson::top_level_strings(file, keys).map_err(|e| match e { + mapjson::MapJsonError::Io(e) => Error::Io(e), + mapjson::MapJsonError::Json(m) => Error::InputInvalid(format!( + "sourcemap is not valid JSON: {m} ({})", map_path.display() - )) - })?; - Ok(["debug_id", "debugId", "uuid"] - .iter() - .find_map(|k| value.get(*k).and_then(serde_json::Value::as_str)) - .map(str::to_string)) + )), + }) } #[cfg(test)] @@ -590,6 +627,29 @@ mod tests { /// RELATIVE root have to take turns, or they change the directory out from under each other. static CWD_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + #[test] + fn streamed_map_hash_equals_the_concatenated_uuid_v5_formula() { + // The id is a public key (it is embedded in shipped bundles): the + // incremental hash must reproduce `Uuid::new_v5` over `len || bundle || + // map` bit for bit, including across the 64 KiB read-buffer boundary. + let dir = tempfile::tempdir().unwrap(); + for map_len in [0usize, 1, 65_535, 65_536, 65_537, 300_001] { + let bundle = b"console.log('x')"; + let map: Vec = (0..map_len).map(|i| (i % 251) as u8).collect(); + let mut input = (bundle.len() as u64).to_le_bytes().to_vec(); + input.extend_from_slice(bundle); + input.extend_from_slice(&map); + let expected = Uuid::new_v5(&DEBUG_ID_NAMESPACE, &input); + let p = dir.path().join("m.map"); + std::fs::write(&p, &map).unwrap(); + assert_eq!( + compute_debug_id_with_map_file(bundle, &p).unwrap(), + expected + ); + assert_eq!(compute_debug_id_with_map(bundle, Some(&map)), expected); + } + } + #[test] fn is_contained_relative_rejects_traversal_and_absolute() { assert!(is_contained_relative("bundle.js.map")); From 01e17d9c20b57cd5b8b8181776eea14f87821b60 Mon Sep 17 00:00:00 2001 From: Alexey Karimov Date: Wed, 7 Oct 2026 17:26:04 +0500 Subject: [PATCH 09/14] 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 10/14] 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:31:27 +0500 Subject: [PATCH 11/14] fix(inject): rewrite a map's content in place instead of fs::copy from a 0600 temp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review (Cursor, #74): fs::copy carries the source file's metadata onto the destination, so every rewritten map was tightened to the temp file's 0o600 (and on Windows given FILE_ATTRIBUTE_TEMPORARY): unreadable by a deploy user afterwards. Write the temp file's content into the original through a truncating open instead, which keeps mode, inode, symlink and hard links. Pinned with a 0o644 map that is also hard-linked (verified to fail with fs::copy back). πŸ€– Generated with [Claude Code](https://claude.com/claude-code) --- src/inject/mod.rs | 48 +++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 44 insertions(+), 4 deletions(-) diff --git a/src/inject/mod.rs b/src/inject/mod.rs index a028762..d0b4d3f 100644 --- a/src/inject/mod.rs +++ b/src/inject/mod.rs @@ -570,9 +570,10 @@ fn write_map_debug_id( } } if !dry_run { - // Pass 2: stream the map into a sibling temp file with the ids set, then - // copy it over the original (not rename: that would swap the inode and - // drop the permissions / symlink / hard-link the build gave the map). + // Pass 2: stream the map into a sibling temp file with the ids set, then write + // its CONTENT into the original (not rename: that would swap the inode and + // drop the symlink / hard link the build gave the map; and not `fs::copy`: + // it also copies the temp file's metadata, i.e. its 0o600 mode, onto the map). let dir = map_path.parent().filter(|p| !p.as_os_str().is_empty()); let tmp = match dir { Some(d) => tempfile::NamedTempFile::new_in(d)?, @@ -590,7 +591,11 @@ fn write_map_debug_id( })?; std::io::Write::flush(&mut out)?; } - std::fs::copy(tmp.path(), map_path)?; + let mut dest = std::fs::OpenOptions::new() + .write(true) + .truncate(true) + .open(map_path)?; + std::io::copy(&mut std::fs::File::open(tmp.path())?, &mut dest)?; } Ok(true) } @@ -650,6 +655,41 @@ mod tests { } } + /// The rewrite must leave the map's mode, inode and links as the bundler made them. A + /// sibling temp file is 0o600, so any step that carries ITS metadata over (as + /// `fs::copy` does) silently tightens a 0o644 map to owner-only. Distinct modes on + /// the map and the bundle make that visible. + #[cfg(unix)] + #[test] + fn rewriting_a_map_keeps_its_mode_inode_and_hard_links() { + use std::os::unix::fs::{MetadataExt, PermissionsExt}; + let dir = tempfile::tempdir().unwrap(); + let js = dir.path().join("a.js"); + let map = dir.path().join("a.js.map"); + let link = dir.path().join("a.js.map.link"); + std::fs::write(&js, "a()\n").unwrap(); + std::fs::write(&map, r#"{"version":3,"mappings":"AAAA"}"#).unwrap(); + std::fs::hard_link(&map, &link).unwrap(); + std::fs::set_permissions(&map, std::fs::Permissions::from_mode(0o644)).unwrap(); + let ino = std::fs::metadata(&map).unwrap().ino(); + + inject_paths(&[dir.path().to_path_buf()], &[], true, false).unwrap(); + + let after = std::fs::metadata(&map).unwrap(); + assert_eq!( + after.permissions().mode() & 0o777, + 0o644, + "the mode must not be tightened" + ); + assert_eq!(after.ino(), ino, "same inode: rewritten in place"); + assert!(std::fs::read_to_string(&map).unwrap().contains("debug_id")); + assert_eq!( + std::fs::read(&link).unwrap(), + std::fs::read(&map).unwrap(), + "a hard link sees the new content" + ); + } + #[test] fn is_contained_relative_rejects_traversal_and_absolute() { assert!(is_contained_relative("bundle.js.map")); From 0f388c27aae832e7073f43ffd84d547dddf7d9bf Mon Sep 17 00:00:00 2001 From: Alexey Karimov Date: Wed, 7 Oct 2026 17:39:06 +0500 Subject: [PATCH 12/14] 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 3aa901345f5328c223ac6488cd1897fa0fb48f7d Mon Sep 17 00:00:00 2001 From: Alexey Karimov Date: Wed, 7 Oct 2026 17:40:14 +0500 Subject: [PATCH 13/14] fix(mapjson): copy any valid JSON number; anonymous scratch file; warn when strip is skipped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review (Fable): - struson's default reader rejects exponents beyond +-99 and numbers over 100 chars (a guard for code that PARSES numbers). We only skip or copy their text, but transfer_to enforced it: a map with `1e100` made --strip-sources-content give up and upload the UNSTRIPPED map (sources included), exit 0, silently; inject exited 11. Reader now built with restrict_number_values = false (verified: the new test fails with it back on). - The strip fallback for an unusable map now logs a warning saying the upload is UNSTRIPPED instead of passing silently. - The map rewrite's scratch file is tempfile::tempfile() (anonymous, OS temp dir) instead of a sibling NamedTempFile: no write access to the map's directory needed (fs::write never needed it), and a killed run leaves no stray .tmpXXXX. πŸ€– Generated with [Claude Code](https://claude.com/claude-code) --- src/cli/debug_files.rs | 15 ++++++++++++-- src/inject/mapjson.rs | 45 ++++++++++++++++++++++++++++++++++++++---- src/inject/mod.rs | 22 ++++++++++----------- 3 files changed, 65 insertions(+), 17 deletions(-) diff --git a/src/cli/debug_files.rs b/src/cli/debug_files.rs index 16b7f1d..f7da7df 100644 --- a/src/cli/debug_files.rs +++ b/src/cli/debug_files.rs @@ -1953,8 +1953,19 @@ fn stripped_copy_without_sources( std::io::BufWriter::new(std::fs::File::create(&out).map_err(crate::error::Error::Io)?); match crate::inject::mapjson::strip_sources_content(input, output) { Ok(true) => {} - // Nothing to strip, or not the JSON object we expect: the copy is moot. - Ok(false) | Err(crate::inject::mapjson::MapJsonError::Json(_)) => { + // Nothing to strip: the copy is moot. + Ok(false) => { + let _ = std::fs::remove_file(&out); + return Ok(None); + } + // Not the JSON object we expect. A privacy preference is not a reason to fail the + // upload, but it must not fail SILENTLY either: say that the sources are NOT stripped. + Err(crate::inject::mapjson::MapJsonError::Json(why)) => { + tracing::warn!( + path = %map_path.display(), + reason = %why, + "--strip-sources-content could not process this map; uploading it UNSTRIPPED" + ); let _ = std::fs::remove_file(&out); return Ok(None); } diff --git a/src/inject/mapjson.rs b/src/inject/mapjson.rs index 7d74c77..ca8e50a 100644 --- a/src/inject/mapjson.rs +++ b/src/inject/mapjson.rs @@ -12,7 +12,7 @@ use std::io::{Read, Write}; -use struson::reader::{JsonReader, JsonStreamReader, ReaderError, ValueType}; +use struson::reader::{JsonReader, JsonStreamReader, ReaderError, ReaderSettings, ValueType}; use struson::writer::{JsonStreamWriter, JsonWriter}; /// Why a streaming pass over a map failed. @@ -61,6 +61,22 @@ impl From for MapJsonError { type Res = std::result::Result; +/// A reader that does NOT apply struson's default "restrict number values" (it rejects +/// an exponent beyond Β±99 and any number over 100 characters). That guard protects code +/// that PARSES numbers into big integers; this module never parses one, it only skips or +/// copies the number's text. Left on, valid JSON such as `1e100` made a copy fail, and +/// `--strip-sources-content` then fell back to uploading the unstripped map: the very +/// source the flag exists to keep off the wire. +fn reader(input: R) -> JsonStreamReader { + JsonStreamReader::new_custom( + input, + ReaderSettings { + restrict_number_values: false, + ..Default::default() + }, + ) +} + /// What [`top_level_strings`] found. pub struct TopLevel { /// Whether the document's top-level value is an object. @@ -75,7 +91,7 @@ pub struct TopLevel { /// document. A top-level value that is not an object yields `is_object: false` /// and no values (it is still syntax-checked). pub fn top_level_strings(input: R, keys: &[&str]) -> Res { - let mut r = JsonStreamReader::new(input); + let mut r = reader(input); let mut values: Vec> = vec![None; keys.len()]; let is_object = r.peek()? == ValueType::Object; if is_object { @@ -109,7 +125,7 @@ pub fn top_level_strings(input: R, keys: &[&str]) -> Res { /// /// Errors with [`MapJsonError::Json`] when the input is not a JSON object. pub fn strip_sources_content(input: R, output: W) -> Res { - let mut r = JsonStreamReader::new(input); + let mut r = reader(input); let mut w = JsonStreamWriter::new(output); if r.peek()? != ValueType::Object { return Err(MapJsonError::Json("not a JSON object".into())); @@ -178,7 +194,7 @@ fn strip_section(r: &mut R, w: &mut W) -> Res(input: R, output: W, debug_id: &str) -> Res<()> { - let mut r = JsonStreamReader::new(input); + let mut r = reader(input); let mut w = JsonStreamWriter::new(output); if r.peek()? != ValueType::Object { return Err(MapJsonError::Json("not a JSON object".into())); @@ -282,6 +298,27 @@ mod tests { ); } + /// Numbers are copied as TEXT, however unusual: a large exponent or a very long integer + /// is valid JSON, and refusing to copy it must never make a strip give up (which would + /// upload the sources it was asked to drop). + #[test] + fn unusual_numbers_are_copied_verbatim_and_sources_are_still_stripped() { + let long_int = "9".repeat(101); + for n in ["1e100", "1e-100", "-2.5E+150", "1e999999", long_int.as_str(), "0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001"] { + let doc = format!(r#"{{"n":{n},"sourcesContent":["secret"],"mappings":"AA"}}"#); + let (removed, out) = strip(&doc).unwrap_or_else(|e| panic!("{n}: {e}")); + assert!(removed, "{n}: the sources must still be stripped"); + assert_eq!(out, format!(r#"{{"n":{n},"mappings":"AA"}}"#), "{n}"); + let mut ids = Vec::new(); + set_debug_ids(doc.as_bytes(), &mut ids, "id").unwrap_or_else(|e| panic!("{n}: {e}")); + assert!(String::from_utf8(ids).unwrap().contains(&format!(r#""n":{n}"#)), "{n}"); + assert_eq!( + top_level_strings(doc.as_bytes(), &["mappings"]).unwrap().values, + [Some("AA".to_string())] + ); + } + } + #[test] fn strip_rejects_non_objects_and_bad_json() { assert!(matches!(strip("[]"), Err(MapJsonError::Json(_)))); diff --git a/src/inject/mod.rs b/src/inject/mod.rs index d0b4d3f..426dc67 100644 --- a/src/inject/mod.rs +++ b/src/inject/mod.rs @@ -570,18 +570,17 @@ fn write_map_debug_id( } } if !dry_run { - // Pass 2: stream the map into a sibling temp file with the ids set, then write - // its CONTENT into the original (not rename: that would swap the inode and - // drop the symlink / hard link the build gave the map; and not `fs::copy`: - // it also copies the temp file's metadata, i.e. its 0o600 mode, onto the map). - let dir = map_path.parent().filter(|p| !p.as_os_str().is_empty()); - let tmp = match dir { - Some(d) => tempfile::NamedTempFile::new_in(d)?, - None => tempfile::NamedTempFile::new_in(".")?, - }; + // Pass 2: stream the map into a scratch file with the ids set, then write its + // CONTENT into the original (not rename: that would swap the inode and drop the + // symlink / hard link the build gave the map; and not `fs::copy`: it also copies + // the scratch file's metadata, i.e. its 0o600 mode, onto the map). + // The scratch file is anonymous and lives in the OS temp dir, so rewriting a map + // needs no write access to the map's directory and a killed run leaves no stray + // `.tmpXXXX` beside it. + let mut scratch = tempfile::tempfile()?; { let input = std::io::BufReader::new(std::fs::File::open(map_path)?); - let mut out = std::io::BufWriter::new(tmp.as_file()); + let mut out = std::io::BufWriter::new(&scratch); mapjson::set_debug_ids(input, &mut out, debug_id).map_err(|e| match e { mapjson::MapJsonError::Io(e) => Error::Io(e), mapjson::MapJsonError::Json(m) => Error::InputInvalid(format!( @@ -595,7 +594,8 @@ fn write_map_debug_id( .write(true) .truncate(true) .open(map_path)?; - std::io::copy(&mut std::fs::File::open(tmp.path())?, &mut dest)?; + std::io::Seek::seek(&mut scratch, std::io::SeekFrom::Start(0))?; + std::io::copy(&mut scratch, &mut dest)?; } Ok(true) } From 0d99de4b2e3fded7a8aa95ce72e099aa2cc715e9 Mon Sep 17 00:00:00 2001 From: Alexey Karimov Date: Wed, 7 Oct 2026 17:53:52 +0500 Subject: [PATCH 14/14] fix(inject): keep the map's scratch file on the map's own volume MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review (Cursor, #74): the anonymous scratch lived in $TMPDIR, so rewriting an 89 MB map needed 89 MB free there; in CI containers /tmp is often a small tmpfs and inject failed (exit 10) although dist/ had room. tempfile_in() keeps it on the volume that already holds the map, falling back to the OS temp dir only when the directory refuses a file (read-only dir). πŸ€– Generated with [Claude Code](https://claude.com/claude-code) --- src/inject/mod.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/inject/mod.rs b/src/inject/mod.rs index 426dc67..862c233 100644 --- a/src/inject/mod.rs +++ b/src/inject/mod.rs @@ -574,10 +574,16 @@ fn write_map_debug_id( // CONTENT into the original (not rename: that would swap the inode and drop the // symlink / hard link the build gave the map; and not `fs::copy`: it also copies // the scratch file's metadata, i.e. its 0o600 mode, onto the map). - // The scratch file is anonymous and lives in the OS temp dir, so rewriting a map - // needs no write access to the map's directory and a killed run leaves no stray - // `.tmpXXXX` beside it. - let mut scratch = tempfile::tempfile()?; + // The scratch file is anonymous (a killed run leaves no stray `.tmpXXXX`). It lives + // beside the map, on the volume that already holds one copy of it: a tens-of-MB map + // must not need that much free space in `$TMPDIR`, which in CI containers is often a + // small tmpfs. Only when the directory refuses a file (it is read-only: editing the + // map in place never needed it writable) does it fall back to the OS temp dir. + let dir = map_path + .parent() + .filter(|p| !p.as_os_str().is_empty()) + .unwrap_or_else(|| Path::new(".")); + let mut scratch = tempfile::tempfile_in(dir).or_else(|_| tempfile::tempfile())?; { let input = std::io::BufReader::new(std::fs::File::open(map_path)?); let mut out = std::io::BufWriter::new(&scratch);