From 695180cd7c12047e8e90e0ade138823c2a4d7012 Mon Sep 17 00:00:00 2001 From: Dylan Frankland Date: Mon, 31 Aug 2026 14:25:56 -0700 Subject: [PATCH] fix(xcresult): read a copy of the bundle instead of migrating the caller's MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `xcresulttool` migrates a bundle that predates `database.sqlite3` in place the first time it is read. Two things follow, neither of them ours to do: - an upload writes into a build artifact it was only asked to read, and - the read fails outright when that directory is not writable: Error: "database.sqlite3" couldn't be moved because you don't have permission to access "test4.xcresult". which is `exit 64` and no JUnit at all, on the read-only artifact mounts CI systems hand out. It is also why two concurrent readers of one bundle race to create the same file. `XCResult::new` now copies the bundle into a `TempDir` and reads that, so the caller's directory is never written to and never needs to be writable. The copy is unconditional rather than keyed on whether a migration would happen: sniffing the format to save a copy trades a correctness guarantee for work we already do in well under a second on a 64 MB bundle. The temp directory is held behind an `Arc` because `XCResult` is `Clone` and `TempDir` is not — every clone shares the copy, and it is removed when the last one drops. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.lock | 1 + xcresult/CONTRIBUTING.md | 9 +++++ xcresult/Cargo.toml | 1 + xcresult/src/xcresult.rs | 33 ++++++++++++++++- xcresult/tests/xcresult.rs | 76 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 119 insertions(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index 75b576d9..dcb3992a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7050,6 +7050,7 @@ dependencies = [ "syn 2.0.110", "tar", "temp_testdir", + "tempfile", "tracing", "tracing-subscriber", "typify", diff --git a/xcresult/CONTRIBUTING.md b/xcresult/CONTRIBUTING.md index 42484e7a..a9c49715 100644 --- a/xcresult/CONTRIBUTING.md +++ b/xcresult/CONTRIBUTING.md @@ -10,6 +10,15 @@ This crate serves two main purposes: 2. **Conditional File Path Specification**: While there are other xcresult parses, this crate handles specifying file paths in the JUnit output, which are conditionally present based on whether a failure (not error) has occurred. File paths are only included in the JUnit output when a test case has failed, as they are extracted from failure summaries in the xcresult bundle. This also handles generating stable identfiers because, by default, one of the values we generate IDs from is the file path. Without this crate, we wouldn't be able to safely map files to tests nor have codeowners support for xcresult. +## Every read goes through a copy + +`xcresulttool` migrates a pre-`database.sqlite3` bundle **in place** the first time it is +read. That writes into a directory the uploader was only asked to read, fails outright with +`exit 64` when the directory is not writable — read-only artifact mounts are ordinary in CI — +and makes two concurrent readers of one bundle race to create the same file. `XCResult::new` +copies the bundle into a `TempDir` and reads that instead, so the caller's bundle is never +touched and never needs to be writable. + ## Running the Binary The crate provides a binary called `xcresult-to-junit` that can be used to convert xcresult bundles to JUnit XML. diff --git a/xcresult/Cargo.toml b/xcresult/Cargo.toml index 2cf90e27..028bcbd3 100644 --- a/xcresult/Cargo.toml +++ b/xcresult/Cargo.toml @@ -23,6 +23,7 @@ quick-junit = "0.5.0" regex = "1.11.0" serde = { version = "1.0.215", default-features = false } serde_json = "1.0.133" +tempfile = "3.2.0" tracing = "0.1.41" uuid = { version = "1.10.0", features = ["v5"] } diff --git a/xcresult/src/xcresult.rs b/xcresult/src/xcresult.rs index 18335f1e..027aa709 100644 --- a/xcresult/src/xcresult.rs +++ b/xcresult/src/xcresult.rs @@ -1,9 +1,10 @@ use std::collections::HashMap; use std::str; -use std::{fs, path::Path, time::Duration}; +use std::{fs, path::Path, path::PathBuf, sync::Arc, time::Duration}; use chrono::{DateTime, Utc}; use quick_junit::{NonSuccessKind, Report, TestCase, TestCaseStatus, TestRerun, TestSuite}; +use tempfile::TempDir; use crate::types::{ SWIFT_DEFAULT_TEST_SUITE_NAME, @@ -12,6 +13,33 @@ use crate::types::{ use crate::xcresult_legacy::XCResultTestLegacy; use crate::xcrun::{xcresulttool_get_object, xcresulttool_get_test_results_tests}; +/// `xcresulttool` migrates an older bundle in place on first read, writing into a directory +/// we were only asked to read and failing outright when it is not writable. +fn copy_bundle(path: &Path) -> anyhow::Result<(TempDir, PathBuf)> { + fn copy_dir(from: &Path, to: &Path) -> std::io::Result<()> { + fs::create_dir_all(to)?; + for entry in fs::read_dir(from)? { + let entry = entry?; + let destination = to.join(entry.file_name()); + if entry.file_type()?.is_dir() { + copy_dir(&entry.path(), &destination)?; + } else { + fs::copy(entry.path(), destination)?; + } + } + Ok(()) + } + + let temp_dir = TempDir::new()?; + let destination = temp_dir.path().join( + path.file_name() + .unwrap_or_else(|| std::ffi::OsStr::new("bundle.xcresult")), + ); + copy_dir(path, &destination) + .map_err(|e| anyhow::anyhow!("failed to copy {} for reading: {}", path.display(), e))?; + Ok((temp_dir, destination)) +} + #[derive(Debug, Clone)] pub struct XCResult { tests: Tests, @@ -19,6 +47,7 @@ pub struct XCResult { repo_full_name: String, legacy_xcresult_tests: HashMap, test_run_started_at: Option>, + _bundle_copy: Arc, } impl XCResult { @@ -35,6 +64,7 @@ impl XCResult { e ) })?; + let (bundle_copy, absolute_path) = copy_bundle(&absolute_path)?; // Call xcresulttool_get_object once and use it for both timestamp extraction and legacy tests let actions_invocation_record = xcresulttool_get_object(&absolute_path); @@ -99,6 +129,7 @@ impl XCResult { org_url_slug, repo_full_name, test_run_started_at, + _bundle_copy: Arc::new(bundle_copy), }) } diff --git a/xcresult/tests/xcresult.rs b/xcresult/tests/xcresult.rs index 35397b5d..b2eff8b6 100644 --- a/xcresult/tests/xcresult.rs +++ b/xcresult/tests/xcresult.rs @@ -639,3 +639,79 @@ fn test_xcresult_with_variant_id_generation() { ); } } + +// Reading used to migrate the bundle in place, which failed when it was not writable. +#[cfg(target_os = "macos")] +#[test] +fn test_reading_a_bundle_neither_writes_to_it_nor_needs_it_writable() { + fn entries(dir: &Path) -> Vec { + let mut found = Vec::new(); + let mut stack = vec![dir.to_path_buf()]; + while let Some(current) = stack.pop() { + for entry in std::fs::read_dir(¤t).unwrap() { + let path = entry.unwrap().path(); + found.push(path.strip_prefix(dir).unwrap().display().to_string()); + if path.is_dir() { + stack.push(path); + } + } + } + found.sort(); + found + } + + fn set_writable(dir: &Path, writable: bool) { + let mut stack = vec![dir.to_path_buf()]; + let mut all = vec![dir.to_path_buf()]; + while let Some(current) = stack.pop() { + for entry in std::fs::read_dir(¤t).unwrap() { + let path = entry.unwrap().path(); + if path.is_dir() { + stack.push(path.clone()); + } + all.push(path); + } + } + // Directories have to come last on the way down and first on the way back up. + all.sort(); + if !writable { + all.reverse(); + } + for path in all { + let mode = if writable { 0o755 } else { 0o555 }; + std::fs::set_permissions(&path, std::os::unix::fs::PermissionsExt::from_mode(mode)) + .unwrap(); + } + } + + let temp_dir = unpack_archive_to_temp_dir("tests/data/test4.xcresult.tar.gz"); + let bundle = temp_dir.as_ref().join("test4.xcresult"); + let before = entries(&bundle); + assert!( + !before + .iter() + .any(|entry| entry.contains("database.sqlite3")), + "the fixture must start un-migrated for this to prove anything" + ); + + set_writable(&bundle, false); + let xcresult = XCResult::new( + bundle.to_str().unwrap(), + ORG_URL_SLUG.clone(), + REPO_FULL_NAME.clone(), + false, + ); + let read_only_result = xcresult.map(|xcresult| xcresult.generate_junits().len()); + set_writable(&bundle, true); + + assert_eq!( + read_only_result.map_err(|e| e.to_string()), + Ok(1), + "a read-only bundle must still be readable" + ); + pretty_assertions::assert_eq!( + entries(&bundle), + before, + "reading the bundle changed it on disk" + ); +}