Fix dedupe.

Author: milesj

CHANGELOG.md

@@ -1,5 +1,13 @@
 # Changelog
 
+## Unreleased
+
+#### 🐞 Fixes
+
+- Fixed an issue where HTTP remote cache would deserialize manifests into the wrong shape, resulting
+  in failed caching.
+- Fixed an issue where dependency deduping would run on fresh/initial installs.
+
 ## 2.4.4
 
 #### 🐞 Fixes

crates/actions/src/actions/install_dependencies.rs

@@ -142,6 +142,11 @@ pub async fn install_dependencies(
         None => None,
     };
 
+    // Determine this before any commands run, as the install itself
+    // will create the vendor directory
+    let first_install = toolchain.metadata.vendor_dir_name.is_some()
+        && !has_vendor_installed_dependencies(&toolchain, &deps_root);
+
     // Create a lock if we haven't run before
     let Some(mut lock) = create_hash_and_return_lock_if_changed(
         action,
@@ -155,10 +160,7 @@ pub async fn install_dependencies(
             &input,
         )
         .await?,
-        || {
-            toolchain.metadata.vendor_dir_name.is_some()
-                && !has_vendor_installed_dependencies(&toolchain, &deps_root)
-        },
+        || first_install,
     )?
     else {
         debug!(
@@ -210,18 +212,30 @@ pub async fn install_dependencies(
     if !is_ci()
         && let Some(mut dedupe) = output.dedupe_command
     {
-        debug!(
-            root = node.root.as_str(),
-            toolchain_id = node.toolchain_id.as_str(),
-            "Deduping {} dependencies",
-            toolchain.metadata.name
-        );
-
-        dedupe.cache = None; // Disable
-        dedupe.command.stream = !hide_output;
-        action
-            .operations
-            .extend(exec_plugin_command(app_context, &dedupe, &options).await?);
+        // On the very first install there are no existing dependencies to
+        // dedupe against, and worse, deduping would re-resolve the graph and
+        // rewrite a pristine lockfile, since a cold cache always hashes as
+        // "changed". Only dedupe once dependencies exist on disk.
+        if first_install {
+            debug!(
+                root = node.root.as_str(),
+                toolchain_id = node.toolchain_id.as_str(),
+                "Dependencies were installed for the first time, skipping dedupe"
+            );
+        } else {
+            debug!(
+                root = node.root.as_str(),
+                toolchain_id = node.toolchain_id.as_str(),
+                "Deduping {} dependencies",
+                toolchain.metadata.name
+            );
+
+            dedupe.cache = None; // Disable
+            dedupe.command.stream = !hide_output;
+            action
+                .operations
+                .extend(exec_plugin_command(app_context, &dedupe, &options).await?);
+        }
     }
 
     finalize_action_operations(action, &toolchain, setup_op, output.operations, vec![])?;

crates/actions/tests/install_dependencies_test.rs

@@ -0,0 +1,136 @@
+use moon_action::{Action, ActionStatus, InstallDependenciesNode};
+use moon_action_context::ActionContext;
+use moon_actions::actions::install_dependencies;
+use moon_common::{Id, is_ci, path::WorkspaceRelativePathBuf};
+use moon_test_utils::WorkspaceMocker;
+use starbase_sandbox::{Sandbox, create_empty_sandbox};
+use starbase_utils::json::JsonValue;
+
+fn create_workspace() -> (Sandbox, WorkspaceMocker) {
+    let sandbox = create_empty_sandbox();
+
+    // The tier1 toolchain (inherited by tc-tier2) registers `tc.lock`
+    // as its lockfile and `vendor` as its vendor directory
+    sandbox.create_file("tc.lock", "v1");
+
+    let mocker = WorkspaceMocker::new(sandbox.path())
+        .with_default_projects()
+        .with_test_toolchains()
+        // Opt-in to the install/dedupe commands returned by tc-tier2
+        .update_toolchains_config(|config| {
+            config
+                .plugins
+                .get_mut(&Id::raw("tc-tier2"))
+                .unwrap()
+                .config
+                .insert("testInstallCommands".into(), JsonValue::Bool(true));
+        })
+        // Hash with the cache engine instead of the VCS,
+        // as the sandbox is not a git repository
+        .update_workspace_config(|config| {
+            config.experiments.native_file_hashing = true;
+        });
+
+    (sandbox, mocker)
+}
+
+async fn run_action(ws: &WorkspaceMocker) -> (Action, ActionStatus) {
+    let mut action = Action::default();
+    let node = InstallDependenciesNode {
+        members: None,
+        project_id: None,
+        root: WorkspaceRelativePathBuf::default(),
+        toolchain_id: Id::raw("tc-tier2"),
+    };
+
+    let status = install_dependencies(
+        &mut action,
+        ActionContext::default().into(),
+        ws.mock_app_context().into(),
+        ws.mock_workspace_graph().await.into(),
+        &node,
+    )
+    .await
+    .unwrap();
+
+    (action, status)
+}
+
+fn count_execs(action: &Action) -> usize {
+    action
+        .operations
+        .iter()
+        .filter(|op| op.meta.is_process_execution())
+        .count()
+}
+
+mod install_dependencies {
+    use super::*;
+
+    #[serial_test::serial]
+    #[tokio::test(flavor = "multi_thread")]
+    async fn does_not_dedupe_on_first_install() {
+        let (_sandbox, ws) = create_workspace();
+
+        // No vendor directory exists yet, so only the install command must
+        // run. Deduping would rewrite a pristine lockfile, since a cold
+        // cache always hashes as "changed"
+        let (action, status) = run_action(&ws).await;
+
+        assert_eq!(status, ActionStatus::Passed);
+        assert_eq!(count_execs(&action), 1);
+    }
+
+    #[serial_test::serial]
+    #[tokio::test(flavor = "multi_thread")]
+    async fn dedupes_when_dependencies_already_installed() {
+        let (sandbox, ws) = create_workspace();
+        sandbox.create_file("vendor/dep/manifest", "");
+
+        let (action, status) = run_action(&ws).await;
+
+        if is_ci() {
+            // In CI, an existing vendor directory skips the action entirely
+            assert_eq!(status, ActionStatus::Skipped);
+            assert_eq!(count_execs(&action), 0);
+        } else {
+            // Install + dedupe
+            assert_eq!(status, ActionStatus::Passed);
+            assert_eq!(count_execs(&action), 2);
+        }
+    }
+
+    #[serial_test::serial]
+    #[tokio::test(flavor = "multi_thread")]
+    async fn dedupes_on_lockfile_change_after_first_install() {
+        let (sandbox, ws) = create_workspace();
+
+        // Cold cache: install only, no dedupe
+        let (action, status) = run_action(&ws).await;
+
+        assert_eq!(status, ActionStatus::Passed);
+        assert_eq!(count_execs(&action), 1);
+
+        // Simulate the install having populated the vendor directory
+        sandbox.create_file("vendor/dep/manifest", "");
+
+        // Nothing changed: nothing to install or dedupe
+        let (action, status) = run_action(&ws).await;
+
+        assert_eq!(status, ActionStatus::Skipped);
+        assert_eq!(count_execs(&action), 0);
+
+        // Lockfile changed: install and dedupe
+        sandbox.create_file("tc.lock", "v2");
+
+        let (action, status) = run_action(&ws).await;
+
+        if is_ci() {
+            assert_eq!(status, ActionStatus::Skipped);
+            assert_eq!(count_execs(&action), 0);
+        } else {
+            assert_eq!(status, ActionStatus::Passed);
+            assert_eq!(count_execs(&action), 2);
+        }
+    }
+}

wasm/tc-tier2/src/lib.rs

@@ -55,9 +55,25 @@ pub fn locate_dependencies_root(
 
 #[plugin_fn]
 pub fn install_dependencies(
-    Json(_): Json<InstallDependenciesInput>,
+    Json(input): Json<InstallDependenciesInput>,
 ) -> FnResult<Json<InstallDependenciesOutput>> {
-    Ok(Json(InstallDependenciesOutput::default()))
+    let mut output = InstallDependenciesOutput::default();
+
+    // Only return commands when a test opts in via toolchain config,
+    // otherwise unrelated tests would execute real processes
+    if input
+        .toolchain_config
+        .get("testInstallCommands")
+        .and_then(|value| value.as_bool())
+        .unwrap_or_default()
+    {
+        output.install_command =
+            Some(ExecCommand::new(ExecCommandInput::pipe("git", ["--version"])).label("install"));
+        output.dedupe_command =
+            Some(ExecCommand::new(ExecCommandInput::pipe("git", ["--version"])).label("dedupe"));
+    }
+
+    Ok(Json(output))
 }
 
 #[plugin_fn]