fix(install/clean): infinite loops on circular dependencies (#211)

* fix(package-manager): avoid infinite loops when clean install circular dependencies

* chore: use dashset instread of memo-map for resolved_packages cache

* chore: fmt

* docs: make it appears nicer

---------

Co-authored-by: Khải <[email protected]>

crates/cli/src/cli_args/add.rs

@@ -85,7 +85,8 @@ impl AddArgs {
     pub async fn run(self, mut state: State) -> miette::Result<()> {
         // TODO: if a package already exists in another dependency group, don't remove the existing entry.
 
-        let State { tarball_mem_cache, http_client, config, manifest, lockfile } = &mut state;
+        let State { tarball_mem_cache, http_client, config, manifest, lockfile, resolved_packages } =
+            &mut state;
 
         Add {
             tarball_mem_cache,
@@ -96,6 +97,7 @@ impl AddArgs {
             list_dependency_groups: || self.dependency_options.dependency_groups(),
             package_name: &self.package_name,
             save_exact: self.save_exact,
+            resolved_packages,
         }
         .run()
         .await

crates/cli/src/cli_args/install.rs

@@ -49,7 +49,8 @@ pub struct InstallArgs {
 
 impl InstallArgs {
     pub async fn run(self, state: State) -> miette::Result<()> {
-        let State { tarball_mem_cache, http_client, config, manifest, lockfile } = &state;
+        let State { tarball_mem_cache, http_client, config, manifest, lockfile, resolved_packages } =
+            &state;
         let InstallArgs { dependency_options, frozen_lockfile } = self;
 
         Install {
@@ -60,6 +61,7 @@ impl InstallArgs {
             lockfile: lockfile.as_ref(),
             dependency_groups: dependency_options.dependency_groups(),
             frozen_lockfile,
+            resolved_packages,
         }
         .run()
         .await;

crates/cli/src/state.rs

@@ -3,6 +3,7 @@ use miette::Diagnostic;
 use pacquet_lockfile::{LoadLockfileError, Lockfile};
 use pacquet_network::ThrottledClient;
 use pacquet_npmrc::Npmrc;
+use pacquet_package_manager::ResolvedPackages;
 use pacquet_package_manifest::{PackageManifest, PackageManifestError};
 use pacquet_tarball::MemCache;
 use pipe_trait::Pipe;
@@ -20,6 +21,8 @@ pub struct State {
     pub manifest: PackageManifest,
     /// Data from the `pnpm-lock.yaml` file.
     pub lockfile: Option<Lockfile>,
+    /// In-memory cache for packages that have started resolving dependencies.
+    pub resolved_packages: ResolvedPackages,
 }
 
 /// Error type of [`State::init`].
@@ -45,6 +48,7 @@ impl State {
                 .map_err(InitStateError::LoadLockfile)?,
             http_client: ThrottledClient::new_from_cpu_count(),
             tarball_mem_cache: MemCache::new(),
+            resolved_packages: ResolvedPackages::new(),
         })
     }
 }

crates/cli/tests/install.rs

@@ -12,6 +12,7 @@ use pipe_trait::Pipe;
 use std::{
     fs::{self, OpenOptions},
     io::Write,
+    path::Path,
 };
 
 #[test]
@@ -164,6 +165,29 @@ fn frozen_lockfile_should_be_able_to_handle_big_lockfile() {
 
     eprintln!("Executing command...");
     pacquet.with_args(["install", "--frozen-lockfile"]).assert().success();
+}
+
+#[test]
+fn should_install_circular_dependencies() {
+    let CommandTempCwd { pacquet, root, workspace, npmrc_info, .. } =
+        CommandTempCwd::init().add_mocked_registry();
+    let AddMockedRegistry { mock_instance, .. } = npmrc_info;
+
+    eprintln!("Creating package.json...");
+    let manifest_path = workspace.join("package.json");
+    let package_json_content = serde_json::json!({
+        "dependencies": {
+            "@pnpm.e2e/circular-deps-1-of-2": "1.0.2",
+        },
+    });
+    fs::write(manifest_path, package_json_content.to_string()).expect("write to package.json");
+
+    eprintln!("Executing command...");
+    pacquet.with_arg("install").assert().success();
+
+    assert!(workspace.join("./node_modules/@pnpm.e2e/circular-deps-1-of-2").exists());
+    assert!(workspace.join("./node_modules/.pnpm/@[email protected]").exists());
+    assert!(workspace.join("./node_modules/.pnpm/@[email protected]").exists());
 
     drop((root, mock_instance)); // cleanup
 }

crates/package-manager/Cargo.toml

@@ -20,6 +20,7 @@ pacquet-registry         = { workspace = true }
 pacquet-tarball          = { workspace = true }
 
 async-recursion = { workspace = true }
+dashmap         = { workspace = true }
 derive_more     = { workspace = true }
 futures-util    = { workspace = true }
 node-semver     = { workspace = true }

crates/package-manager/src/add.rs

@@ -1,4 +1,4 @@
-use crate::Install;
+use crate::{Install, ResolvedPackages};
 use derive_more::{Display, Error};
 use miette::Diagnostic;
 use pacquet_lockfile::Lockfile;
@@ -17,6 +17,7 @@ where
     DependencyGroupList: IntoIterator<Item = DependencyGroup>,
 {
     pub tarball_mem_cache: &'a MemCache,
+    pub resolved_packages: &'a ResolvedPackages,
     pub http_client: &'a ThrottledClient,
     pub config: &'static Npmrc,
     pub manifest: &'a mut PackageManifest,
@@ -51,6 +52,7 @@ where
             list_dependency_groups,
             package_name,
             save_exact,
+            resolved_packages,
         } = self;
 
         let latest_version = PackageVersion::fetch_from_registry(
@@ -77,6 +79,7 @@ where
             lockfile,
             dependency_groups: list_dependency_groups(),
             frozen_lockfile: false,
+            resolved_packages,
         }
         .run()
         .await;

crates/package-manager/src/install.rs

@@ -1,4 +1,4 @@
-use crate::{InstallFrozenLockfile, InstallWithoutLockfile};
+use crate::{InstallFrozenLockfile, InstallWithoutLockfile, ResolvedPackages};
 use pacquet_lockfile::Lockfile;
 use pacquet_network::ThrottledClient;
 use pacquet_npmrc::Npmrc;
@@ -12,6 +12,7 @@ where
     DependencyGroupList: IntoIterator<Item = DependencyGroup>,
 {
     pub tarball_mem_cache: &'a MemCache,
+    pub resolved_packages: &'a ResolvedPackages,
     pub http_client: &'a ThrottledClient,
     pub config: &'static Npmrc,
     pub manifest: &'a PackageManifest,
@@ -28,6 +29,7 @@ where
     pub async fn run(self) {
         let Install {
             tarball_mem_cache,
+            resolved_packages,
             http_client,
             config,
             manifest,
@@ -42,6 +44,7 @@ where
             (false, _, _) => {
                 InstallWithoutLockfile {
                     tarball_mem_cache,
+                    resolved_packages,
                     http_client,
                     config,
                     manifest,
@@ -122,6 +125,7 @@ mod tests {
                 DependencyGroup::Optional,
             ],
             frozen_lockfile: false,
+            resolved_packages: &Default::default(),
         }
         .run()
         .await;

crates/package-manager/src/install_without_lockfile.rs

@@ -1,5 +1,6 @@
 use crate::InstallPackageFromRegistry;
 use async_recursion::async_recursion;
+use dashmap::DashSet;
 use futures_util::future;
 use node_semver::Version;
 use pacquet_network::ThrottledClient;
@@ -9,6 +10,12 @@ use pacquet_registry::PackageVersion;
 use pacquet_tarball::MemCache;
 use pipe_trait::Pipe;
 
+/// In-memory cache for packages that have started resolving dependencies.
+///
+/// The contents of set is the package's virtual_store_name.
+/// e.g. `@pnpm.e2e/[email protected]` →  `@[email protected]`
+pub type ResolvedPackages = DashSet<String>;
+
 /// This subroutine install packages from a `package.json` without reading or writing a lockfile.
 ///
 /// **Brief overview for each package:**
@@ -21,6 +28,7 @@ use pipe_trait::Pipe;
 #[must_use]
 pub struct InstallWithoutLockfile<'a, DependencyGroupList> {
     pub tarball_mem_cache: &'a MemCache,
+    pub resolved_packages: &'a ResolvedPackages,
     pub http_client: &'a ThrottledClient,
     pub config: &'static Npmrc,
     pub manifest: &'a PackageManifest,
@@ -39,6 +47,7 @@ impl<'a, DependencyGroupList> InstallWithoutLockfile<'a, DependencyGroupList> {
             config,
             manifest,
             dependency_groups,
+            resolved_packages,
         } = self;
 
         let _: Vec<()> = manifest
@@ -62,6 +71,7 @@ impl<'a, DependencyGroupList> InstallWithoutLockfile<'a, DependencyGroupList> {
                     config,
                     manifest,
                     dependency_groups: (),
+                    resolved_packages,
                 }
                 .install_dependencies_from_registry(&dependency)
                 .await;
@@ -75,7 +85,19 @@ impl<'a> InstallWithoutLockfile<'a, ()> {
     /// Install dependencies of a dependency.
     #[async_recursion]
     async fn install_dependencies_from_registry(&self, package: &PackageVersion) {
-        let InstallWithoutLockfile { tarball_mem_cache, http_client, config, .. } = self;
+        let InstallWithoutLockfile {
+            tarball_mem_cache,
+            http_client,
+            config,
+            resolved_packages,
+            ..
+        } = self;
+
+        // This package has already resolved, there is no need to reinstall again.
+        if !resolved_packages.insert(package.to_virtual_store_name()) {
+            tracing::info!(target: "pacquet::install", package = ?package.to_virtual_store_name(), "Skip subset");
+            return;
+        }
 
         let node_modules_path = self
             .config