Merge pull request #10534 from Extra-Chill/fix-10290-unify-tree-hashers

fix(core): unify deploy and harvest tree walks behind one scanner

Author: chubes4

crates/homeboy-core/src/content_diff.rs

@@ -1,4 +1,13 @@
 //! Generic byte-level directory comparison and safe source-to-destination application.
+//!
+//! This module owns the one directory walk every tree comparison in the
+//! codebase is built on. Recovery (`harvest`) and deploy drift detection used
+//! to each carry a privately written walker, and the two could return
+//! contradictory answers for the same directory pair because their divergences
+//! were accidents of two implementations rather than decisions (#10290). The
+//! walk now lives in [`scan_tree`] and every remaining divergence is a named
+//! field on [`TreeScanOptions`], so a behavioural difference between call sites
+//! is reviewable instead of invisible.
 
 use std::collections::{BTreeMap, BTreeSet};
 use std::fs;
@@ -45,7 +54,7 @@ pub fn compare(
                 kind: ContentChangeKind::Deletion,
                 bytes: to.bytes,
             }),
-            (Some(from), Some(to)) if from.digest != to.digest => Some(ContentChange {
+            (Some(from), Some(to)) if !from.matches(to) => Some(ContentChange {
                 path,
                 kind: ContentChangeKind::Modification,
                 bytes: from.bytes,
@@ -76,13 +85,96 @@ pub fn apply(source: &Path, destination: &Path, changes: &[ContentChange]) -> cr
     Ok(())
 }
 
-#[derive(Debug)]
-struct Entry {
-    digest: String,
-    bytes: u64,
+/// What a tree scan records, and what it leaves out.
+///
+/// Recovery and deploy walk the same shape of directory but are answering
+/// different questions about it, so the facts each one needs differ. Every
+/// difference is a field here rather than a separate walker: two call sites can
+/// still behave differently, but only by declaring that they do.
+#[derive(Debug, Clone, Default)]
+pub struct TreeScanOptions {
+    /// Relative glob and directory patterns removed from the scan. `.git` is
+    /// always removed regardless of this set.
+    pub excludes: Vec<String>,
+    /// Record symlinks as entries carrying their link target. When false a
+    /// symlink is neither followed nor recorded, so link-only drift is
+    /// invisible to the caller.
+    pub record_symlinks: bool,
+    /// Record the executable bit as part of each file's identity.
+    pub record_executable_mode: bool,
+    /// Prune this product's own transport scratch files at any depth. They are
+    /// created by the tooling performing the comparison, never by the content
+    /// being compared, so no side of a comparison should report them as drift.
+    pub prune_runtime_artifacts: bool,
 }
 
-fn collect(root: &Path, excludes: &[String]) -> crate::Result<BTreeMap<String, Entry>> {
+/// Whether a scanned path is a regular file or a symlink.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum TreeEntryKind {
+    File,
+    Symlink,
+}
+
+impl TreeEntryKind {
+    /// Single-character tag used by manifest wire formats and digests.
+    pub fn tag(self) -> char {
+        match self {
+            TreeEntryKind::File => 'f',
+            TreeEntryKind::Symlink => 'l',
+        }
+    }
+}
+
+/// One scanned path and the facts recorded about it.
+#[derive(Debug, Clone)]
+pub struct TreeEntry {
+    pub kind: TreeEntryKind,
+    /// Executable bits in octal (see [`executable_mode_tag`]), or `"0"` when
+    /// the scan does not record mode, the entry is a symlink, or the platform
+    /// has no mode concept.
+    pub mode: String,
+    /// SHA-256 hex digest for files; the link target for symlinks.
+    pub value: String,
+    /// Filesystem byte length, or `0` when the entry did not come from a
+    /// filesystem walk. Deliberately excluded from [`TreeEntry::matches`]:
+    /// manifests parsed from a remote probe or an archive carry content
+    /// identity without a stat size, and comparing a missing size against a
+    /// real one would report drift on identical content.
+    pub bytes: u64,
+}
+
+impl TreeEntry {
+    /// Whether two entries describe the same content at the same path.
+    pub fn matches(&self, other: &Self) -> bool {
+        self.kind == other.kind && self.mode == other.mode && self.value == other.value
+    }
+}
+
+/// The mode semantic that survives a deploy.
+///
+/// Deploy normalizes ownership and group-write/setgid bits on the target, so
+/// only executability is stable enough to compare. Rendering it here — rather
+/// than in each manifest producer — is what keeps a locally walked tree, a
+/// remotely probed tree, and an archive expressing the same bits identically.
+pub fn executable_mode_tag(mode: u32) -> String {
+    format!("{:o}", mode & 0o111)
+}
+
+/// Whether a relative path is one of this product's own transport scratch
+/// files, at any depth.
+pub fn runtime_artifact(path: &str) -> bool {
+    let prefix = crate::product_identity::PRODUCT_IDENTITY.artifact_prefix;
+    path.split('/').any(|part| part.starts_with(prefix))
+}
+
+/// Walk `root` and record one entry per path, honouring `options`.
+///
+/// Entries are keyed by `/`-separated path relative to `root`, so the result is
+/// directly comparable with a manifest produced for a different root.
+pub fn scan_tree(
+    root: &Path,
+    options: &TreeScanOptions,
+) -> crate::Result<BTreeMap<String, TreeEntry>> {
     if !root.is_dir() {
         return Err(crate::Error::validation_invalid_argument(
             "path",
@@ -92,15 +184,26 @@ fn collect(root: &Path, excludes: &[String]) -> crate::Result<BTreeMap<String, E
         ));
     }
     let mut entries = BTreeMap::new();
-    visit(root, root, excludes, &mut entries)?;
+    scan_directory(root, root, options, &mut entries)?;
     Ok(entries)
 }
 
-fn visit(
+fn collect(root: &Path, excludes: &[String]) -> crate::Result<BTreeMap<String, TreeEntry>> {
+    scan_tree(
+        root,
+        &TreeScanOptions {
+            excludes: excludes.to_vec(),
+            prune_runtime_artifacts: true,
+            ..TreeScanOptions::default()
+        },
+    )
+}
+
+fn scan_directory(
     root: &Path,
     directory: &Path,
-    excludes: &[String],
-    entries: &mut BTreeMap<String, Entry>,
+    options: &TreeScanOptions,
+    entries: &mut BTreeMap<String, TreeEntry>,
 ) -> crate::Result<()> {
     for entry in fs::read_dir(directory).map_err(io_error)? {
         let path = entry.map_err(io_error)?.path();
@@ -109,26 +212,68 @@ fn visit(
             .map_err(io_error)?
             .to_string_lossy()
             .replace('\\', "/");
-        if excluded(&relative, excludes) {
+        if excluded(&relative, &options.excludes)
+            || (options.prune_runtime_artifacts && runtime_artifact(&relative))
+        {
             continue;
         }
         let metadata = fs::symlink_metadata(&path).map_err(io_error)?;
-        if metadata.is_dir() {
-            visit(root, &path, excludes, entries)?;
+        if metadata.file_type().is_symlink() {
+            if !options.record_symlinks {
+                continue;
+            }
+            let target = fs::read_link(&path)
+                .map_err(io_error)?
+                .to_string_lossy()
+                .to_string();
+            entries.insert(
+                relative,
+                TreeEntry {
+                    kind: TreeEntryKind::Symlink,
+                    mode: "0".to_string(),
+                    value: target,
+                    bytes: 0,
+                },
+            );
+        } else if metadata.is_dir() {
+            scan_directory(root, &path, options, entries)?;
         } else if metadata.is_file() {
-            let bytes = metadata.len();
             entries.insert(
                 relative,
-                Entry {
-                    digest: digest(&path)?,
-                    bytes,
+                TreeEntry {
+                    kind: TreeEntryKind::File,
+                    mode: entry_mode(&metadata, options),
+                    value: digest(&path)?,
+                    bytes: metadata.len(),
                 },
             );
         }
     }
     Ok(())
 }
 
+fn entry_mode(metadata: &fs::Metadata, options: &TreeScanOptions) -> String {
+    if options.record_executable_mode {
+        platform_executable_mode(metadata)
+    } else {
+        "0".to_string()
+    }
+}
+
+/// Executable bits as the platform reports them.
+#[cfg(unix)]
+fn platform_executable_mode(metadata: &fs::Metadata) -> String {
+    use std::os::unix::fs::PermissionsExt;
+    executable_mode_tag(metadata.permissions().mode())
+}
+
+/// Targets without a mode concept compare every entry as non-executable, which
+/// is the same answer the previous deploy walker gave there.
+#[cfg(not(unix))]
+fn platform_executable_mode(_metadata: &fs::Metadata) -> String {
+    "0".to_string()
+}
+
 pub fn excluded(path: &str, excludes: &[String]) -> bool {
     path == ".git"
         || path.starts_with(".git/")
@@ -212,4 +357,118 @@ mod tests {
             .expect("compare")
             .is_empty());
     }
+
+    /// #10290: deploy's manifest pruned this product's transport scratch files
+    /// and recovery's comparison did not, so the two disagreed about the same
+    /// directory pair. The scratch files belong to the tooling doing the
+    /// comparing; neither answer should ever have included them.
+    #[test]
+    fn recovery_comparison_prunes_this_products_transport_scratch_files() {
+        let temp = tempfile::tempdir().expect("temp");
+        let source = temp.path().join("source");
+        let destination = temp.path().join("destination");
+        fs::create_dir_all(source.join("nested")).expect("source");
+        fs::create_dir_all(&destination).expect("destination");
+        let scratch = format!(
+            "{}upload.tmp",
+            crate::product_identity::PRODUCT_IDENTITY.artifact_prefix
+        );
+        fs::write(source.join(&scratch), "remote").expect("scratch");
+        fs::write(source.join("nested").join(&scratch), "remote").expect("nested scratch");
+        fs::write(destination.join(&scratch), "local").expect("scratch");
+
+        assert!(compare(&source, &destination, &[])
+            .expect("compare")
+            .is_empty());
+        assert!(runtime_artifact(&scratch));
+        assert!(runtime_artifact(&format!("nested/{scratch}")));
+        assert!(!runtime_artifact("nested/payload.txt"));
+    }
+
+    /// The recorded facts are options, not accidents: the same tree scanned
+    /// with recovery's options and with deploy's options must differ only in
+    /// the ways those options declare.
+    #[test]
+    fn scan_options_decide_which_facts_a_walk_records() {
+        let temp = tempfile::tempdir().expect("temp");
+        let root = temp.path().join("tree");
+        fs::create_dir_all(&root).expect("root");
+        fs::write(root.join("file"), "bytes").expect("file");
+        #[cfg(unix)]
+        {
+            use std::os::unix::fs::PermissionsExt;
+            fs::set_permissions(root.join("file"), fs::Permissions::from_mode(0o755))
+                .expect("mode");
+            std::os::unix::fs::symlink("file", root.join("link")).expect("link");
+        }
+
+        let recovery = scan_tree(
+            &root,
+            &TreeScanOptions {
+                prune_runtime_artifacts: true,
+                ..TreeScanOptions::default()
+            },
+        )
+        .expect("recovery scan");
+        let deployed = scan_tree(
+            &root,
+            &TreeScanOptions {
+                record_symlinks: true,
+                record_executable_mode: true,
+                prune_runtime_artifacts: true,
+                ..TreeScanOptions::default()
+            },
+        )
+        .expect("deploy scan");
+
+        assert_eq!(recovery["file"].kind, TreeEntryKind::File);
+        assert_eq!(recovery["file"].bytes, "bytes".len() as u64);
+        assert_eq!(recovery["file"].mode, "0");
+        assert_eq!(recovery["file"].value, deployed["file"].value);
+
+        #[cfg(unix)]
+        {
+            assert_eq!(deployed["file"].mode, executable_mode_tag(0o755));
+            assert!(!recovery.contains_key("link"));
+            assert_eq!(deployed["link"].kind, TreeEntryKind::Symlink);
+            assert_eq!(deployed["link"].value, "file");
+            assert_eq!(deployed["link"].mode, "0");
+        }
+    }
+
+    /// Byte length is provenance, not identity. A manifest parsed from a remote
+    /// probe or an archive has no stat size, so folding size into the match
+    /// predicate would report drift on byte-identical content.
+    #[test]
+    fn entry_identity_excludes_filesystem_byte_length() {
+        let walked = TreeEntry {
+            kind: TreeEntryKind::File,
+            mode: executable_mode_tag(0o755),
+            value: "a".repeat(64),
+            bytes: 42,
+        };
+        let parsed = TreeEntry {
+            bytes: 0,
+            ..walked.clone()
+        };
+        assert!(walked.matches(&parsed));
+        assert!(!walked.matches(&TreeEntry {
+            mode: "0".to_string(),
+            ..walked.clone()
+        }));
+        assert!(!walked.matches(&TreeEntry {
+            kind: TreeEntryKind::Symlink,
+            ..walked.clone()
+        }));
+    }
+
+    #[test]
+    fn executable_mode_tag_keeps_only_bits_that_survive_a_deploy() {
+        // Deploy normalizes ownership and group-write bits, so 0644 and 0664
+        // are the same deployed file while 0755 is not.
+        assert_eq!(executable_mode_tag(0o644), "0");
+        assert_eq!(executable_mode_tag(0o664), "0");
+        assert_eq!(executable_mode_tag(0o755), "111");
+        assert_eq!(executable_mode_tag(0o775), "111");
+    }
 }

crates/homeboy-core/src/harvest.rs

@@ -297,7 +297,10 @@ fn materialize_remote(
                     Some("materialize harvest snapshot".to_string()),
                 )
             })?;
-        if content_diff::excluded(relative, excludes) {
+        // Runtime scratch files are created by deploy's own transport, so they
+        // are never remote content worth recovering. Skipping them here as well
+        // as in the comparison avoids downloading bytes only to discard them.
+        if content_diff::excluded(relative, excludes) || content_diff::runtime_artifact(relative) {
             continue;
         }
         let local = crate::resolve_contained_local_path(destination, relative, "remote path")?;