-
-
Notifications
You must be signed in to change notification settings - Fork 32
fix: ensure canonicalized paths remain accessible via strong references #733
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
How to use the Graphite Merge QueueAdd the label merge to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #733 +/- ##
==========================================
- Coverage 94.64% 94.58% -0.06%
==========================================
Files 15 15
Lines 2914 2920 +6
==========================================
+ Hits 2758 2762 +4
- Misses 156 158 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
395834e
to
b70e409
Compare
Fixes #732 - "Canonicalized path was dropped" error that could occur in CI environments. The issue was that canonicalized paths were only stored as weak references without ensuring a strong reference existed in the cache. This could cause the Arc to be immediately dropped, leading to the weak reference failing to upgrade. The fix ensures canonicalized paths are added to the cache (if not already present) before downgrading to weak references, maintaining at least one strong reference while preserving the weak reference pattern needed to avoid memory leaks. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
b70e409
to
9e971fa
Compare
Merge activity
|
CodSpeed Performance ReportMerging #733 will not alter performanceComparing Summary
Footnotes
|
…es (#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)
Summary
Canonicalized path was dropped
error #732 - "Canonicalized path was dropped" error that could occur in CI environmentsProblem
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
insrc/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
test_canonicalized_path_not_dropped
to verify canonicalization works correctlytest_canonicalized_path_weak_reference
to ensure the error doesn't occurcargo test
passescargo clippy
passescargo check
passes🤖 Generated with Claude Code