fix(xcresult): read a copy of the bundle instead of migrating the caller's

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.

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"] }
 

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,13 +13,41 @@ 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,
     org_url_slug: String,
     repo_full_name: String,
     legacy_xcresult_tests: HashMap<String, XCResultTestLegacy>,
     test_run_started_at: Option<DateTime<Utc>>,
+    _bundle_copy: Arc<TempDir>,
 }
 
 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),
         })
     }
 

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<String> {
+        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(&current).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(&current).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"
+    );
+}