Repository navigation
fix: reclaim entries when duplicate key destructors panic - #409
Merged
Merged
Conversation
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
No open issues 🎉
What changed in this PR
Fixes potential leaks in OnceMap and singleflight::Group when dropping a duplicate key triggers a panic, by establishing cleanup ownership before dropping the key outside internal locks, and adds regression tests plus a changelog entry.
Changes:
- Adjust
OnceMapandsingleflight::Groupinternals to create cleanup guards before explicitly dropping duplicate keys outside the lock. - Add
KeyWithDroptest helper to deterministically trigger panicking key destructors. - Add regression tests covering pending computations/work and cached lookups, and record the fix in the changelog.
| File | Description |
|---|---|
| asyncband/src/test_support.rs | Adds KeyWithDrop helper to trigger controlled panics during key drop. |
| asyncband/src/singleflight/tests.rs | Adds regression test for duplicate-key destructor panic cleanup in Group. |
| asyncband/src/singleflight/mod.rs | Refactors duplicate-key path to establish WorkCleanupGuard ownership before dropping the key. |
| asyncband/src/once/once_map/tests.rs | Adds regression tests for pending and cached scenarios involving duplicate-key destructor panics. |
| asyncband/src/once/once_map/mod.rs | Refactors lookup path to return a pending cleanup guard and drop duplicate keys only after ownership is established. |
| CHANGELOG.md | Documents the bug fix. |
Review effort: Lite
Copilot stopped reviewing on behalf of
tisonkun due to an error
November 5, 2026 13:32
orthur2
approved these changes
Nov 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Establish cleanup ownership before explicitly dropping duplicate keys outside the table locks in
OnceMapandsingleflight::Group. A panicking key destructor now releases pending entries and cached value clones, including when another caller cancels during that destructor, while the panic continues to propagate.Add deterministic regression coverage for
compute,try_compute,work,try_work, and cached lookups, and record the fix in the changelog.Fixes #388.
Validation: all three new regression tests fail before the fix and pass afterward;
cargo x testpasses on stable and Rust 1.86.0 (688 tests each),cargo x checkandcargo x lintpass, and the three regression tests pass under Miri with strict provenance and symbolic alignment checks.