fix: memory leak in dropping uninserted guard#120
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to fix a memory leak / slot exhaustion scenario where dropping an uninserted PlaceholderGuard leaves the placeholder allocated in CacheShard::entries, preventing reuse of the freed slab slot for subsequent placeholders.
Changes:
- Add a regression test (
test_guard_leak) that asserts dropping an uninserted guard frees and reuses the same placeholder index. - Add a test-only accessor on
PlaceholderGuardto expose its underlying shared placeholder (for test assertions). - Update
CacheShard::remove_placeholderto also remove the placeholder fromentries(in addition tomap).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/sync.rs |
Adds a regression unit test to verify placeholder slot reuse after dropping an uninserted guard. |
src/sync_placeholder.rs |
Adds a #[cfg(test)] accessor to expose the shared placeholder for assertions in unit tests. |
src/shard.rs |
Attempts to fix the leak by removing placeholder slots from CacheShard::entries when removing placeholders. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hey @arthurprs, please let me know if there are any outstanding issues with this fix. I'd appreciate a patch release once this is accepted and merged. I have a service leaking ~10GiB RAM in ~10h due to this bug. Thanks! |
Cover the two documented cases where a placeholder is removed/replaced while a guard is still outstanding (overwrite insert, and remove + slot reuse). Both panicked under the original unconditional entries.remove and pass with the fix that only frees the slot when the placeholder is still present.
|
@hanabi1224 release coming shortly! |
|
Thank you for the fix @hanabi1224 , expect a release within the hour. |
This PR attempts to fix a potential memory leak when dropping uninserted
PlaceholderGuardinstances. Basically, an uninsertedPlaceholderGuardis not removed fromCacheShard::entriesand cannot be reused for the same key.