fix: coalesce locator cache in-flight lookups#419
Conversation
Performance Report (Linux) ✅
Legend
|
Test Coverage Report (Linux)
Coverage increased! Great work! |
Performance Report (macOS)
Legend
|
Performance Report (Windows) ➖
Legend
|
Test Coverage Report (Windows)
Coverage increased! Great work! |
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #399 by eliminating the TOCTOU window in LocatorCache::get_or_insert_with, ensuring concurrent callers for the same key share a single in-flight computation (preventing duplicate side effects such as repeated process spawns in locators).
Changes:
- Added per-key in-flight coordination to
LocatorCache::get_or_insert_withusing a mutex-protected in-flight map plus condvar waiters. - Ensured
Noneresults are shared with concurrent waiters but are not persisted in the cache (allowing later retries). - Added concurrency + panic-cleanup tests and updated the conda locator comment to reflect the new caching behavior.
Show a summary per file
| File | Description |
|---|---|
| crates/pet-core/src/cache.rs | Implements per-key in-flight coalescing for get_or_insert_with and adds tests for concurrency, None sharing, and panic cleanup. |
| crates/pet-conda/src/lib.rs | Updates the mamba-manager caching comment to reflect the new in-flight coalescing behavior. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 0
There was a problem hiding this comment.
Pull request overview
This PR updates LocatorCache::get_or_insert_with to coalesce concurrent lookups per key, preventing duplicate closure execution (and side effects) while still allowing later retries when a computation returns None (by not caching None results).
Changes:
- Add per-key “in-flight” coordination to
LocatorCache::get_or_insert_withso concurrent callers share the first computation result. - Share
Noneresults with current waiters but avoid cachingNone, enabling subsequent retries. - Add concurrency/panic-cleanup tests and update the conda mamba-manager cache comment to reflect the new behavior.
Show a summary per file
| File | Description |
|---|---|
crates/pet-core/src/cache.rs |
Implements per-key in-flight coordination and adds tests for coalescing, None sharing, and panic cleanup. |
crates/pet-conda/src/lib.rs |
Updates comment describing mamba-manager lookup behavior now that the cache coalesces concurrent lookups. |
Copilot's findings
Comments suppressed due to low confidence (1)
crates/pet-core/src/cache.rs:80
Dropcallspublish_result(None)and that function currently usesexpect(...)on mutex locks. If a panic is already in flight (e.g., the closure panicked) and a lock is poisoned, panicking again fromDropcan cause an abort (double panic) and can also strand waiters.Dropcleanup should be best-effort and avoid panicking; recover from poison and still remove the key + notify.
impl<K: Eq + Hash, V> Drop for InFlightOwnerGuard<'_, K, V> {
fn drop(&mut self) {
if self.key.is_some() {
self.publish_result(None);
}
- Files reviewed: 2/2 changed files
- Comments generated: 1
There was a problem hiding this comment.
Pull request overview
This PR updates pet-core’s LocatorCache::get_or_insert_with to coordinate concurrent lookups per key, preventing duplicate closure execution (and side effects) under contention, and aligns Conda’s mamba-manager discovery commentary with the new behavior.
Changes:
- Add per-key in-flight tracking (Mutex + Condvar) to coalesce concurrent
get_or_insert_withcalls for the same key. - Share an in-flight
Noneresult with current waiters while still not cachingNonefor future retries. - Add new concurrency/panic-cleanup tests and update Conda’s cache comment for mamba discovery.
Show a summary per file
| File | Description |
|---|---|
| crates/pet-core/src/cache.rs | Implements per-key in-flight coordination for get_or_insert_with and adds concurrency/panic/poisoning tests. |
| crates/pet-conda/src/lib.rs | Updates comment to reflect that mamba discovery/reporting is coalesced per in-flight cache key. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 2
There was a problem hiding this comment.
Pull request overview
This PR fixes a TOCTOU window in pet-core’s LocatorCache::get_or_insert_with by adding per-key in-flight coordination so concurrent callers share a single computation (and its side effects), addressing #399. It also updates conda’s mamba-manager caching comment to reflect the new behavior and adds concurrency + panic-cleanup tests to validate the coordination logic.
Changes:
- Add per-key “in-flight” tracking to coalesce concurrent
get_or_insert_withcalls for the same key. - Ensure
Noneresults are shared with current waiters but not cached (allowing future retries), and ensure owner panics wake waiters with a panic. - Add multi-threaded and panic/poison recovery tests; update related conda comment.
Show a summary per file
| File | Description |
|---|---|
crates/pet-core/src/cache.rs |
Implements per-key in-flight lookup coordination for get_or_insert_with and adds concurrency/panic robustness tests. |
crates/pet-conda/src/lib.rs |
Updates the mamba-manager cache comment to match the new coalescing semantics. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
Summary:
LocatorCache::get_or_insert_withso concurrent callers share the first closure result.Someand in-flightNoneresults with current waiters while keepingNoneuncached for later retries.Validation:
Fixes #399