fix: use Weak references for CachedPath to enable proper drop (#727)

Related:

* oxc-project/oxc#10627

The resolver was experiencing memory leaks due to circular strong references between `CachedPath` objects. Even after the resolver was dropped, `CachedPath` instances (and their associated `PackageJson` data) remained in memory due to reference cycles created by Arc (reference-counted) pointers.

The memory leak was caused by circular references in three fields of `CachedPathImpl`:
1. `parent: Option<CachedPath>` - Child paths held strong references to parent paths
2. `canonicalized: OnceLock<Result<CachedPath, ResolveError>>` - Paths held strong references to their canonicalized versions
3. `node_modules: OnceLock<Option<CachedPath>>` - Parent directories held strong references to their node_modules subdirectories

These created reference cycles preventing Arc reference counts from reaching zero.

Converted all fields that could create circular references to use `Weak` references:
- `parent: Option<Weak<CachedPathImpl>>`
- `canonicalized: OnceLock<Result<Weak<CachedPathImpl>, ResolveError>>`
- `node_modules: OnceLock<Option<Weak<CachedPathImpl>>>`

Updated all accessor methods to upgrade Weak references when needed:
- `parent()` - Upgrades weak parent reference
- `cached_node_modules()` - Stores and retrieves weak references
- `canonicalize_impl()` - Stores canonicalized paths as weak references

Passes `test_memory_leak_arc_cycles` that failed in the previous stack.

Author: Boshen

src/cache/cache_impl.rs

@@ -54,12 +54,13 @@ impl<Fs: FileSystem> Cache<Fs> {
         let is_node_modules = path.file_name().as_ref().is_some_and(|&name| name == "node_modules");
         let inside_node_modules =
             is_node_modules || parent.as_ref().is_some_and(|parent| parent.inside_node_modules);
+        let parent_weak = parent.as_ref().map(|p| Arc::downgrade(&p.0));
         let cached_path = CachedPath(Arc::new(CachedPathImpl::new(
             hash,
             path.to_path_buf().into_boxed_path(),
             is_node_modules,
             inside_node_modules,
-            parent,
+            parent_weak,
         )));
         paths.insert(cached_path.clone());
         cached_path
@@ -240,7 +241,7 @@ impl<Fs: FileSystem> Cache<Fs> {
                 let res = path.parent().map_or_else(
                     || Ok(path.normalize_root(self)),
                     |parent| {
-                        self.canonicalize_impl(parent).and_then(|parent_canonical| {
+                        self.canonicalize_impl(&parent).and_then(|parent_canonical| {
                             let normalized = parent_canonical.normalize_with(
                                 path.path().strip_prefix(parent.path()).unwrap(),
                                 self,
@@ -269,8 +270,15 @@ impl<Fs: FileSystem> Cache<Fs> {
                 );
 
                 path.canonicalizing.store(0, Ordering::Release);
-                res
+                // Convert to Weak reference before storing
+                res.map(|cp| Arc::downgrade(&cp.0))
+            })
+            .as_ref()
+            .map_err(Clone::clone)
+            .and_then(|weak| {
+                weak.upgrade().map(CachedPath).ok_or_else(|| {
+                    ResolveError::from(io::Error::other("Canonicalized path was dropped"))
+                })
             })
-            .clone()
     }
 }

src/cache/cached_path.rs

@@ -4,7 +4,7 @@ use std::{
     hash::{Hash, Hasher},
     ops::Deref,
     path::{Component, Path, PathBuf},
-    sync::{Arc, atomic::AtomicU64},
+    sync::{Arc, Weak, atomic::AtomicU64},
 };
 
 use cfg_if::cfg_if;
@@ -23,13 +23,13 @@ pub struct CachedPath(pub Arc<CachedPathImpl>);
 pub struct CachedPathImpl {
     pub hash: u64,
     pub path: Box<Path>,
-    pub parent: Option<CachedPath>,
+    pub parent: Option<Weak<CachedPathImpl>>,
     pub is_node_modules: bool,
     pub inside_node_modules: bool,
     pub meta: OnceLock<Option<FileMetadata>>,
-    pub canonicalized: OnceLock<Result<CachedPath, ResolveError>>,
+    pub canonicalized: OnceLock<Result<Weak<CachedPathImpl>, ResolveError>>,
     pub canonicalizing: AtomicU64,
-    pub node_modules: OnceLock<Option<CachedPath>>,
+    pub node_modules: OnceLock<Option<Weak<CachedPathImpl>>>,
     pub package_json: OnceLock<Option<Arc<PackageJson>>>,
 }
 
@@ -39,7 +39,7 @@ impl CachedPathImpl {
         path: Box<Path>,
         is_node_modules: bool,
         inside_node_modules: bool,
-        parent: Option<CachedPath>,
+        parent: Option<Weak<Self>>,
     ) -> Self {
         Self {
             hash,
@@ -73,8 +73,8 @@ impl CachedPath {
         self.path.to_path_buf()
     }
 
-    pub(crate) fn parent(&self) -> Option<&Self> {
-        self.0.parent.as_ref()
+    pub(crate) fn parent(&self) -> Option<Self> {
+        self.0.parent.as_ref().and_then(|weak| weak.upgrade().map(CachedPath))
     }
 
     pub(crate) fn is_node_modules(&self) -> bool {
@@ -100,7 +100,12 @@ impl CachedPath {
         cache: &Cache<Fs>,
         ctx: &mut Ctx,
     ) -> Option<Self> {
-        self.node_modules.get_or_init(|| self.module_directory("node_modules", cache, ctx)).clone()
+        self.node_modules
+            .get_or_init(|| {
+                self.module_directory("node_modules", cache, ctx).map(|cp| Arc::downgrade(&cp.0))
+            })
+            .as_ref()
+            .and_then(|weak| weak.upgrade().map(CachedPath))
     }
 
     /// Find package.json of a path by traversing parent directories.
@@ -114,21 +119,21 @@ impl CachedPath {
         cache: &Cache<Fs>,
         ctx: &mut Ctx,
     ) -> Result<Option<Arc<PackageJson>>, ResolveError> {
-        let mut cache_value = self;
+        let mut cache_value = self.clone();
         // Go up directories when the querying path is not a directory
-        while !cache.is_dir(cache_value, ctx) {
-            if let Some(cv) = &cache_value.parent {
+        while !cache.is_dir(&cache_value, ctx) {
+            if let Some(cv) = cache_value.parent() {
                 cache_value = cv;
             } else {
                 break;
             }
         }
         let mut cache_value = Some(cache_value);
         while let Some(cv) = cache_value {
-            if let Some(package_json) = cache.get_package_json(cv, options, ctx)? {
+            if let Some(package_json) = cache.get_package_json(&cv, options, ctx)? {
                 return Ok(Some(package_json));
             }
-            cache_value = cv.parent.as_ref();
+            cache_value = cv.parent();
         }
         Ok(None)
     }

src/lib.rs

@@ -304,13 +304,13 @@ impl<Fs: FileSystem> ResolverGeneric<Fs> {
         let inside_node_modules = cached_path.inside_node_modules();
         if inside_node_modules {
             let mut last = None;
-            for cp in iter::successors(Some(cached_path), |cp| cp.parent()) {
+            for cp in iter::successors(Some(cached_path.clone()), CachedPath::parent) {
                 if cp.is_node_modules() {
                     break;
                 }
-                if self.cache.is_dir(cp, ctx) {
+                if self.cache.is_dir(&cp, ctx) {
                     if let Some(package_json) =
-                        self.cache.get_package_json(cp, &self.options, ctx)?
+                        self.cache.get_package_json(&cp, &self.options, ctx)?
                     {
                         last = Some(package_json);
                     }
@@ -852,13 +852,14 @@ impl<Fs: FileSystem> ResolverGeneric<Fs> {
         // 1. let DIRS = NODE_MODULES_PATHS(START)
         // 2. for each DIR in DIRS:
         for module_name in &self.options.modules {
-            for cached_path in std::iter::successors(Some(cached_path), |p| p.parent()) {
+            for cached_path in std::iter::successors(Some(cached_path.clone()), CachedPath::parent)
+            {
                 // Skip if /path/to/node_modules does not exist
-                if !self.cache.is_dir(cached_path, ctx) {
+                if !self.cache.is_dir(&cached_path, ctx) {
                     continue;
                 }
 
-                let Some(cached_path) = self.get_module_directory(cached_path, module_name, ctx)
+                let Some(cached_path) = self.get_module_directory(&cached_path, module_name, ctx)
                 else {
                     continue;
                 };
@@ -884,7 +885,7 @@ impl<Fs: FileSystem> ResolverGeneric<Fs> {
                         // Skip if the directory lead to the scope package does not exist
                         // i.e. `foo/node_modules/@scope` is not a directory for `foo/node_modules/@scope/package`
                         if package_name.starts_with('@') {
-                            if let Some(path) = cached_path.parent() {
+                            if let Some(path) = cached_path.parent().as_ref() {
                                 if !self.cache.is_dir(path, ctx) {
                                     continue;
                                 }
@@ -1530,9 +1531,10 @@ impl<Fs: FileSystem> ResolverGeneric<Fs> {
 
         // 11. While parentURL is not the file system root,
         for module_name in &self.options.modules {
-            for cached_path in std::iter::successors(Some(cached_path), |p| p.parent()) {
+            for cached_path in std::iter::successors(Some(cached_path.clone()), CachedPath::parent)
+            {
                 // 1. Let packageURL be the URL resolution of "node_modules/" concatenated with packageSpecifier, relative to parentURL.
-                let Some(cached_path) = self.get_module_directory(cached_path, module_name, ctx)
+                let Some(cached_path) = self.get_module_directory(&cached_path, module_name, ctx)
                 else {
                     continue;
                 };

src/tests/memory_leak.rs

@@ -14,7 +14,7 @@ fn test_memory_leak_arc_cycles() {
     resolver.resolve(&f, "package-json-nested").unwrap();
 
     // Populated cache - path is now owned in multiple places.
-    assert_eq!(Arc::strong_count(&path.0), 4);
+    assert_eq!(Arc::strong_count(&path.0), 2);
 
     // Drop the resolver.
     drop(resolver);