Skip to content

Commit 4cd0893

Browse files
Boshenclaude
andauthored
fix: ensure canonicalized paths remain accessible via strong references (#733)
## Summary - Fixes #732 - "Canonicalized path was dropped" error that could occur in CI environments - Ensures canonicalized paths have at least one strong reference in the cache to prevent premature dropping - Adds comprehensive tests to prevent regression ## Problem The resolver was experiencing "Canonicalized path was dropped" errors due to weak references being used without ensuring a strong reference existed. When `canonicalize_impl` stored only weak references to canonicalized paths, if those paths weren't already in the cache, the Arc would be immediately dropped, causing the weak reference to fail on upgrade. ## Solution Modified `canonicalize_impl` in `src/cache/cache_impl.rs` to ensure canonicalized paths are added to the cache (if not already present) before downgrading to weak references. This maintains at least one strong reference while preserving the weak reference pattern needed to avoid memory leaks from circular references. ## Test Plan - [x] Added `test_canonicalized_path_not_dropped` to verify canonicalization works correctly - [x] Added `test_canonicalized_path_weak_reference` to ensure the error doesn't occur - [x] All existing tests pass, including the memory leak test - [x] `cargo test` passes - [x] `cargo clippy` passes - [x] `cargo check` passes 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: Claude <[email protected]>
1 parent 7c29092 commit 4cd0893

File tree

2 files changed

+69
-1
lines changed

2 files changed

+69
-1
lines changed

src/cache/cache_impl.rs

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -270,7 +270,16 @@ impl<Fs: FileSystem> Cache<Fs> {
270270
);
271271

272272
path.canonicalizing.store(0, Ordering::Release);
273-
// Convert to Weak reference before storing
273+
// Store the canonicalized path in the cache before downgrading to weak reference
274+
// This ensures there's always at least one strong reference to prevent dropping
275+
if let Ok(ref cp) = res {
276+
// Only insert if not already present to avoid unnecessary operations
277+
let paths = self.paths.pin();
278+
if !paths.contains(cp) {
279+
paths.insert(cp.clone());
280+
}
281+
}
282+
// Convert to Weak reference for storage
274283
res.map(|cp| Arc::downgrade(&cp.0))
275284
})
276285
.as_ref()

src/tests/memory_leak.rs

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,3 +22,62 @@ fn test_memory_leak_arc_cycles() {
2222
// All Arcs must be dropped, leaving the original count of 1.
2323
assert_eq!(Arc::strong_count(&path.0), 1);
2424
}
25+
26+
/// Test to ensure canonicalized paths remain accessible after being stored
27+
#[test]
28+
fn test_canonicalized_path_not_dropped() {
29+
use crate::ResolveOptions;
30+
31+
let f = super::fixture_root().join("misc");
32+
33+
let resolver = Resolver::new(ResolveOptions { symlinks: true, ..Default::default() });
34+
35+
// Create a path and canonicalize it
36+
let path = resolver.cache.value(&f);
37+
38+
// This should work without "Canonicalized path was dropped" error
39+
let canonicalized = resolver.cache.canonicalize(&path);
40+
assert!(canonicalized.is_ok());
41+
42+
// Try canonicalizing again - should still work
43+
let canonicalized2 = resolver.cache.canonicalize(&path);
44+
assert!(canonicalized2.is_ok());
45+
assert_eq!(canonicalized.unwrap(), canonicalized2.unwrap());
46+
}
47+
48+
/// Test to ensure canonicalized paths that are not in cache remain accessible
49+
#[test]
50+
fn test_canonicalized_path_weak_reference() {
51+
let f = super::fixture_root().join("misc");
52+
53+
let resolver = Resolver::default();
54+
55+
// Create a new path that's not previously in the cache
56+
let new_path = f.join("some_unique_path");
57+
58+
// Get the cached path - this will be the only strong reference
59+
let path = resolver.cache.value(&new_path);
60+
61+
// Canonicalize a path that doesn't exist in the cache's hashmap yet
62+
// This might fail with "Canonicalized path was dropped" if the implementation is wrong
63+
match resolver.cache.canonicalize(&path) {
64+
Ok(_) => {
65+
// If canonicalization succeeded, try again to ensure consistency
66+
let result2 = resolver.cache.canonicalize(&path);
67+
assert_eq!(
68+
resolver.cache.canonicalize(&path).ok(),
69+
result2.ok(),
70+
"Canonicalization results should be consistent"
71+
);
72+
}
73+
Err(e) => {
74+
// It's okay if canonicalization fails for other reasons (e.g., path doesn't exist)
75+
// but it should NOT fail with "Canonicalized path was dropped"
76+
let error_msg = e.to_string();
77+
assert!(
78+
!error_msg.contains("Canonicalized path was dropped"),
79+
"Should not fail with 'Canonicalized path was dropped' error, got: {error_msg}"
80+
);
81+
}
82+
}
83+
}

0 commit comments

Comments
 (0)