corpus: real-world ArrayPool leak from Nethermind PatriciaTree (P-007/P-012 replay)#131
Conversation
…/P-012 replay) A new real-world replay case from a named P-007 target. NethermindEth/nethermind#9322 (fixed in v1.35.0): Nethermind.Trie/PatriciaTree.cs `Get`/`GetNodeByKey` Rent a byte[] from ArrayPool for long keys, call `GetNew` (which can throw TrieException), and Return the buffer only on the success path — so a thrown TrieException leaks the rented buffer. The fix moves the Return into a `finally`. - case.own: the OwnLang reduction (the throw path modeled as an early `return` arm that exits before `release`) trips OWN001 (owned resource not released on all paths). Verified: `corpus: 20/20 cases match`. - before.cs / after.cs: reduced, self-contained, compilable C# (leak vs finally-fix) for the dotnet-backed `corpus-benchmark` recall/specificity job. - notes.md: source (PR #9322 / v1.35.0), pattern, and the throw->early-return modeling caveat. Suite green (corpus 20/20); ruff + mypy clean. The C# before/after are scored by the SDK-backed corpus-benchmark CI job (not runnable locally). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KkpSWNx7ARLpQeAs13kkyA
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a buggy and fixed PatriciaTree ArrayPool example, the matching OwnLang ownership model, an expected OWN001 diagnostic, and notes describing the leak scenario. ChangesArrayPool leak corpus
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://cdn.github.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9d2c6f96d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| int nibblesCount = 2 * rawKey.Length; | ||
| byte[] array = ArrayPool<byte>.Shared.Rent(nibblesCount); // Rent (long key) | ||
| byte[] result = GetNew(array); // can throw TrieException |
There was a problem hiding this comment.
This GetNew(array) call is the only path that skips Return, but the corpus benchmark invokes scripts/own-check.sh without --body-throw-edges (scripts/benchmark.py passes only --format sarif, and own-check enables only --flow-locals). In the extractor, body-level may-throw calls outside an enclosing try do not get an exceptional exit unless --body-throw-edges is set, so this no-try fixture is analyzed as acquire → release → return and the new real-C# before.cs row is a benchmark miss despite the notes claiming it is caught.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 03c0004. You're right that a body-level may-throw call outside a try gets no exceptional exit under the benchmark's default --flow-locals (only --body-throw-edges would add those), so the no-try reduction was a benchmark miss.
before.cs now keeps the ArrayPool.Return inside the try after the may-throw GetNew — which is also the real structure PR #9322 fixed ("moved Return from the try to a finally"), so it's both faithful and what the --flow-locals throw-edge model (a throw exit before each may-throw leaf in a try) actually sees. notes.md was corrected to describe this mechanism (and stop claiming end-to-end catch via a path that wouldn't get a throw edge).
Generated by Claude Code
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@corpus/real-world/nethermind-patriciatree-arraypool-leak/notes.md`:
- Around line 20-23: The sample checker output is stale: the reported acquire
site for the OWN001 leak no longer matches the current case.own location.
Regenerate the documented checker snippet so it reflects the current output from
the case.own file, and update the example in notes.md to use the correct line
reference from the present checker run.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 333a3a5e-5197-429b-8d96-c5a18c20ed54
📒 Files selected for processing (5)
corpus/real-world/nethermind-patriciatree-arraypool-leak/after.cscorpus/real-world/nethermind-patriciatree-arraypool-leak/before.cscorpus/real-world/nethermind-patriciatree-arraypool-leak/case.owncorpus/real-world/nethermind-patriciatree-arraypool-leak/expected-diagnostics.txtcorpus/real-world/nethermind-patriciatree-arraypool-leak/notes.md
… (Codex/CodeRabbit #131) - before.cs: keep the ArrayPool.Return INSIDE the try after the may-throw GetNew (the real structure PR #9322 fixed). A body-level may-throw call outside a try gets no exceptional exit under the benchmark's default `--flow-locals` (only `--body-throw-edges` adds those), so the previous no-try reduction was a benchmark miss; the in-try form is what the throw-edge model actually catches (Codex). - notes.md: regenerate the sample output line (case.own:22, not :12) and state the throw-edge mechanism + the SDK-only benchmark scoring accurately (CodeRabbit). case.own unchanged -> still OWN001; suite green (corpus 20/20). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KkpSWNx7ARLpQeAs13kkyA
2 participants
What & why
"Real-world replay" — a regression case from a named P-007 replay target (Nethermind), reduced faithfully and verified to be caught.
Source: NethermindEth/nethermind#9322, fixed in v1.35.0.
Nethermind.Trie/PatriciaTree.csGet/GetNodeByKeyRentabyte[]fromArrayPool<byte>.Sharedfor keys longer than the 64-byte stackalloc minimum, then callGetNew(which can throwTrieException) andReturnthe buffer only on the success path — so a thrownTrieExceptionskips theReturnand leaks the buffer (memory pressure under sustained RPC load). The fix moves theReturninto afinally.The case (
corpus/real-world/nethermind-patriciatree-arraypool-leak/)case.own— OwnLang reduction; the uncaughtthrowis modeled as an earlyreturnarm that exits beforerelease, so the core trips OWN001 (owned resource not released on all paths). Verified locally:corpus: 20/20 cases match.before.cs/after.cs— reduced, self-contained, compilable C# (leak vsfinally-fix) for the SDK-backedcorpus-benchmarkrecall/specificity job.notes.md— source/provenance, the pattern, and thethrow → early-returnmodeling caveat.Testing
python tests/run_tests.py→ green (corpus: 20/20);scripts/benchmark.py --selftestOK;ruff+mypyclean.Honest caveat
The dotnet-backed
corpus-benchmarkCI job scansbefore.cs/after.csthrough the real Roslyn extractor — I can't run that locally (no SDK in this env). Recall there is a floor gate (a missed case doesn't fail CI), but specificity is absolute:after.cs(the fix) must be silent. The fix is a textbookReturn-in-finally, which the extractor's finally/exception-edge model should treat as clean — but I'm watching that job and will adjust the reduction if it reports anything onafter.cs.🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
New Features