Merge branch 'master' into fix/2394-non-utf8-output

Author: milesj

CHANGELOG.md

@@ -28,6 +28,13 @@
   codepage output from Python) was discarded entirely, resulting in empty `stdout.log`/`stderr.log`
   state files, cache hits replaying no output, and missing output in run reports. Invalid bytes are
   now replaced with `�` instead.
+- Fixed an issue where a task's dependents would not run when requested with a downstream scope
+  (`moon ci`, `--dependents`, `--downstream`), if the task was first added to the action graph as a
+  dependency of another target. For example, `moon run app:build lib:build --dependents` would skip
+  the dependents of `lib:build` when `app:build` depends on it.
+- Fixed tasks that emit read-only outputs (e.g. `0444` files) failing on every cache hit with a
+  "Permission denied" error when the `casOutputsCache` experiment is enabled. Caches that already
+  contain read-only objects are healed automatically.
 
 ## 2.4.2
 

crates/action-graph/src/action_graph_builder.rs

@@ -133,6 +133,10 @@ pub struct ActionGraphBuilder<'query> {
 
     // Target tracking
     ignored_dependencies: FxHashMap<Target, FxHashSet<Target>>,
+    // Tasks whose dependents were out of scope when their node was created.
+    // Consumed when the task is revisited with dependents in scope, since the
+    // node-exists early return would otherwise skip the expansion entirely.
+    ignored_dependents: FxHashSet<Target>,
     passthrough_targets: FxHashSet<Target>,
     primary_targets: FxHashSet<Target>,
 
@@ -159,6 +163,7 @@ impl<'query> ActionGraphBuilder<'query> {
             nodes: FxHashMap::default(),
             options,
             ignored_dependencies: FxHashMap::default(),
+            ignored_dependents: FxHashSet::default(),
             passthrough_targets: FxHashSet::default(),
             primary_targets: FxHashSet::default(),
             serial_edges: FxHashSet::default(),
@@ -998,6 +1003,12 @@ impl<'query> ActionGraphBuilder<'query> {
             false
         };
 
+        let had_ignored_dependents = if should_run_dependents {
+            self.ignored_dependents.remove(&task.target)
+        } else {
+            false
+        };
+
         // Check if the node exists to avoid all the overhead below
         if let Some(index) = self.get_index_from_node(&node) {
             if had_ignored_dependencies && !task.deps.is_empty() {
@@ -1008,6 +1019,12 @@ impl<'query> ActionGraphBuilder<'query> {
                 self.link_optional_requirements(index, edges)?;
             }
 
+            if had_ignored_dependents {
+                child_reqs.skip_affected = false;
+
+                Box::pin(self.run_task_dependents(task, &child_reqs, state)).await?;
+            }
+
             return Ok(Some(index));
         }
 
@@ -1047,6 +1064,8 @@ impl<'query> ActionGraphBuilder<'query> {
             child_reqs.skip_affected = false;
 
             Box::pin(self.run_task_dependents(task, &child_reqs, state)).await?;
+        } else {
+            self.ignored_dependents.insert(task.target.clone());
         }
 
         Ok(Some(index))

crates/action-graph/tests/action_graph_builder_test.rs

@@ -2049,6 +2049,54 @@ mod action_graph_builder {
 
                 assert_snapshot!(graph.to_dot());
             }
+
+            #[tokio::test(flavor = "multi_thread")]
+            async fn expands_skipped_dependents_when_task_is_revisited_in_scope() {
+                let sandbox = create_sandbox("tasks");
+                let mut container = ActionGraphContainer::new(sandbox.path());
+
+                let wg = container.create_workspace_graph().await;
+                let mut builder = container.create_builder(wg.clone()).await;
+
+                let parent = wg.get_task_from_project("deps", "parent1").unwrap();
+                let task = wg.get_task_from_project("deps", "base").unwrap();
+
+                // First insert the task as a dependency of another target,
+                // with dependents out of scope
+                builder
+                    .run_task(
+                        &parent,
+                        &RunRequirements {
+                            dependencies: UpstreamScope::Deep,
+                            dependents: DownstreamScope::None,
+                            ..RunRequirements::default()
+                        },
+                    )
+                    .await
+                    .unwrap();
+
+                // Then run it as an explicit target with dependents in scope
+                builder
+                    .run_task(
+                        &task,
+                        &RunRequirements {
+                            dependencies: UpstreamScope::Deep,
+                            dependents: DownstreamScope::Direct,
+                            ..RunRequirements::default()
+                        },
+                    )
+                    .await
+                    .unwrap();
+
+                let (_, graph) = builder.build();
+
+                assert_snapshot!(graph.to_dot());
+
+                assert!(topo(graph).into_iter().any(|node| matches!(
+                    node,
+                    ActionNode::RunTask(inner) if inner.target == Target::parse("deps:parent2").unwrap()
+                )));
+            }
         }
     }
 

crates/action-graph/tests/snapshots/action_graph_builder_test__action_graph_builder__run_task__dependents__expands_skipped_dependents_when_task_is_revisited_in_scope.snap

@@ -0,0 +1,23 @@
+---
+source: crates/action-graph/tests/action_graph_builder_test.rs
+expression: graph.to_dot()
+---
+digraph {
+    0 [ label="SyncWorkspace"]
+    1 [ label="SyncProject(deps)"]
+    2 [ label="RunTask(deps:parent1)"]
+    3 [ label="RunTask(deps:base)"]
+    4 [ label="RunTask(deps:parent2)"]
+    5 [ label="SyncProject(deps-external)"]
+    6 [ label="RunTask(deps-external:external)"]
+    1 -> 0 [ label="required"]
+    3 -> 1 [ label="required"]
+    2 -> 1 [ label="required"]
+    2 -> 3 [ label="required"]
+    4 -> 1 [ label="required"]
+    4 -> 3 [ label="required"]
+    5 -> 0 [ label="required"]
+    5 -> 1 [ label="required"]
+    6 -> 5 [ label="required"]
+    6 -> 3 [ label="required"]
+}

crates/cache/src/lib.rs

@@ -6,7 +6,7 @@ pub use cache_engine::*;
 pub use hash_engine::*;
 pub use moon_cache_item::*;
 pub use moon_cache_storage::*;
-pub use moon_cas::CasStore;
+pub use moon_cas::{CasStore, grant_owner_write_access};
 pub use moon_hash::*;
 pub use state_engine::*;
 

crates/cas/src/cas.rs

@@ -3,7 +3,7 @@ use moon_blob::{Blob, BlobCleanStats};
 use moon_config::CacheCasConfig;
 use moon_hash::{ContentHash, Digest};
 use rustc_hash::FxHashSet;
-use starbase_utils::fs;
+use starbase_utils::fs::{self, FsError};
 use starbase_utils::hash::{
     self, hex,
     sha256::native::{Digest as ShaDigest, Sha256},
@@ -94,6 +94,12 @@ impl CasStore {
 
         fs::reflink_file(source, &guard.path)?;
 
+        // The reflink also clones the source's permissions. Grant the owner
+        // write access so a read-only source (e.g. a task emitting read-only
+        // outputs, #2608) doesn't produce a read-only object, which would fail
+        // later store mutations like `touch`
+        grant_owner_write_access(&guard.path)?;
+
         // No fsync: see `write` for rationale.
         self.commit_temp_file(hash, &mut guard)?;
 
@@ -382,3 +388,29 @@ impl CasStore {
         Ok(())
     }
 }
+
+/// Grant the owner write permission on the file, leaving all other bits intact.
+/// Reflinks clone the source's permissions, so both storing and hydrating a
+/// read-only file must restore writability on the clone.
+pub fn grant_owner_write_access(path: &Path) -> miette::Result<()> {
+    let mut perms = fs::metadata(path)?.permissions();
+
+    #[cfg(unix)]
+    {
+        use std::os::unix::fs::PermissionsExt;
+
+        perms.set_mode(perms.mode() | 0o200);
+    }
+
+    // The readonly attribute is the only permission Windows has
+    #[cfg(not(unix))]
+    #[allow(clippy::permissions_set_readonly_false)]
+    perms.set_readonly(false);
+
+    std::fs::set_permissions(path, perms).map_err(|error| FsError::Perms {
+        path: path.to_path_buf(),
+        error: Box::new(error),
+    })?;
+
+    Ok(())
+}

crates/cas/src/lib.rs

@@ -2,5 +2,5 @@ mod cas;
 mod cas_error;
 mod gc;
 
-pub use cas::CasStore;
+pub use cas::{CasStore, grant_owner_write_access};
 pub use cas_error::CasError;

crates/cas/tests/cas_test.rs

@@ -231,6 +231,40 @@ mod cas {
             assert_eq!(count_temp_files(&store), 0);
         }
 
+        #[test]
+        fn normalizes_read_only_source_permissions() {
+            // A reflink clones the source's permissions, so a read-only output
+            // must not produce a read-only object — later store mutations and
+            // hydration clones would fail on it (#2608).
+            let sandbox = create_empty_sandbox();
+            let store = create_store(&sandbox);
+
+            let source = sandbox.path().join("input.txt");
+            std::fs::write(&source, b"read only").unwrap();
+
+            let mut perms = std::fs::metadata(&source).unwrap().permissions();
+            perms.set_readonly(true);
+            std::fs::set_permissions(&source, perms).unwrap();
+
+            let digest = store.store_file(&source).unwrap();
+            let object_perms = std::fs::metadata(store.object_path(&digest.hash))
+                .unwrap()
+                .permissions();
+
+            // Restore the source so the sandbox can clean up on Windows,
+            // where read-only files block directory removal.
+            #[cfg(windows)]
+            #[allow(clippy::permissions_set_readonly_false)]
+            {
+                let mut perms = std::fs::metadata(&source).unwrap().permissions();
+                perms.set_readonly(false);
+                std::fs::set_permissions(&source, perms).unwrap();
+            }
+
+            assert!(!object_perms.readonly());
+            assert_eq!(store.read(&digest.hash).unwrap(), b"read only");
+        }
+
         #[test]
         fn warm_cache_creates_no_temp_file() {
             // Regression: a file already present in the store must short-circuit

crates/task-runner/src/output_hydrater.rs

@@ -2,7 +2,9 @@ use crate::run_state::TaskRunState;
 use crate::task_runner_error::TaskRunnerError;
 use miette::IntoDiagnostic;
 use moon_app_context::AppContext;
-use moon_cache::{Manifest, ManifestFile, ManifestSource, ManifestSymlink, StorageOptions};
+use moon_cache::{
+    Manifest, ManifestFile, ManifestSource, StorageOptions, grant_owner_write_access,
+};
 use moon_common::{
     color,
     path::{WorkspaceRelativePath, clean_components},
@@ -157,7 +159,6 @@ impl OutputHydrater<'_> {
                     }
                 })?,
                 output_path,
-                link,
             )?;
         }
 
@@ -257,6 +258,12 @@ impl OutputHydrater<'_> {
         let fd = if let Some(source) = &file.source_path {
             fs::reflink_file(source, &output_path)?;
 
+            // The reflink clones the source's permissions, which may lack the
+            // write bit (stores populated before objects were normalized may
+            // contain read-only blobs), so restore it before opening a handle
+            // to apply the mtime/mode below
+            grant_owner_write_access(&output_path)?;
+
             fs::open_file_for_writing(&output_path)?
         }
         // Otherwise write the bytes from the manifest
@@ -284,14 +291,10 @@ impl OutputHydrater<'_> {
         Ok(())
     }
 
-    // Windows lint!
-    #[allow(unused_variables)]
-    fn link_output_file(
-        &self,
-        from_path: PathBuf,
-        to_path: PathBuf,
-        link: &ManifestSymlink,
-    ) -> miette::Result<()> {
+    // The manifest's unix mode is deliberately not applied: it records the
+    // followed target's mode (which the target's own manifest entry restores),
+    // and a chmod through the link would modify the target, not the link
+    fn link_output_file(&self, from_path: PathBuf, to_path: PathBuf) -> miette::Result<()> {
         if let Some(parent) = to_path.parent() {
             fs::create_dir_all(parent)?;
         }
@@ -314,19 +317,9 @@ impl OutputHydrater<'_> {
 
         #[cfg(unix)]
         {
-            use std::os::unix::fs::{PermissionsExt, symlink};
+            use std::os::unix::fs::symlink;
 
             symlink(&from_path, &to_path).map_err(map_error)?;
-
-            if let Some(mode) = &link.unix_mode {
-                let fd = fs::open_file_for_writing(&to_path)?;
-
-                fd.set_permissions(std::fs::Permissions::from_mode(*mode))
-                    .map_err(|error| FsError::Write {
-                        path: to_path.clone(),
-                        error: Box::new(error),
-                    })?;
-            }
         }
 
         Ok(())

crates/task-runner/tests/output_hydrater_test.rs

@@ -213,6 +213,56 @@ mod output_hydrater {
             );
         }
 
+        #[cfg(unix)]
+        #[tokio::test(flavor = "multi_thread")]
+        async fn hydrates_read_only_output_file_from_cas() {
+            use std::os::unix::fs::PermissionsExt;
+
+            // A task can emit read-only outputs (#2608). Hydration must replace
+            // the existing read-only file and re-apply the recorded mode, even
+            // when the blob itself is read-only (stores populated before
+            // objects were normalized).
+            let container = TaskRunnerContainer::new("archive", "file-outputs").await;
+            container
+                .sandbox
+                .create_file("project/file.txt", "read only");
+
+            let file_path = container.sandbox.path().join("project/file.txt");
+
+            fs::set_permissions(&file_path, fs::Permissions::from_mode(0o444)).unwrap();
+
+            let mut state = container.create_state();
+            setup_cas_state(&mut state);
+
+            let source = archive_and_load(&container, &state).await;
+
+            let blob_digest = Digest::from_bytes(b"read only").unwrap();
+            let blob_path = container
+                .sandbox
+                .path()
+                .join(".moon/cache/blobs")
+                .join(blob_digest.hash.prefix())
+                .join(blob_digest.hash.suffix());
+
+            fs::set_permissions(&blob_path, fs::Permissions::from_mode(0o444)).unwrap();
+
+            // Hydrate over the still-existing read-only output, as a
+            // subsequent cache-hit run would
+            assert_hydrated(
+                container
+                    .create_hydrator()
+                    .hydrate(HydrateFrom::Storage(Box::new(source)), "hash123", &state)
+                    .await
+                    .unwrap(),
+            );
+
+            assert_eq!(fs::read_to_string(&file_path).unwrap(), "read only");
+            assert_eq!(
+                fs::metadata(&file_path).unwrap().permissions().mode() & 0o777,
+                0o444
+            );
+        }
+
         #[tokio::test(flavor = "multi_thread")]
         async fn doesnt_hydrate_if_cache_disabled() {
             let container = TaskRunnerContainer::new("archive", "file-outputs").await;