Fix buffer leak tests to use PooledBufferLeakDetector

[newmanifold]

Changes

• Refactor discovery message serializer leak tests to use PooledBufferLeakDetector instead of manual ref-count assertions - They don't assert any leakage, just the refcounts
• Replace manual SafeRelease() calls with using DisposableByteBuffer pattern across all serializer tests
• Extract PooledBufferLeakDetector.CreateAllocator() factory so a single allocator can be shared across a test fixture, avoiding per-test arena allocation overhead
• Track initial allocation count in PooledBufferLeakDetector so it works correctly with shared allocators that may already have outstanding allocations
• Removed Redundant tests; instead added leak detector in original tests
• Removed AssertNoLeaks method, it was error prone with IDisposable

Types of changes

What types of changes does your code introduce?

• Bugfix (a non-breaking change that fixes an issue)
• New feature (a non-breaking change that adds functionality)
• Breaking change (a change that causes existing functionality not to work as expected)
• Optimization
• Refactoring
• Documentation update
• Build-related changes
• Other: Description

Testing

Requires testing

• Yes
• No

If yes, did you write tests?

• Yes
• No

Notes on testing

The leak detection tests themselves are the tests — they verify that serializer round-trips don't leak pooled DotNetty buffers. The PooledBufferLeakDetector asserts zero net allocations on dispose.

Documentation

Requires documentation update

• Yes
• No

Requires explanation in Release Notes

• Yes
• No

[newmanifold]

@claude review please

[claude]

Claude finished @newmanifold's task in 2m 0s —— View job

---

Code Review

• Gather context
• Read changed files
• Review changes
• Post feedback

---

Overall this is a clean, well-motivated refactor. The use of using DisposableByteBuffer is idiomatic and the _initialAlloc baseline approach is correct for shared allocators. A few things worth addressing:

---

Issues

1. _leakDetectionAllocator is never disposed (moderate)

DiscoveryMessageSerializerTests.cs:36 — PooledByteBufferAllocator allocates native arena memory. The shared _leakDetectionAllocator has no teardown path (no [OneTimeTearDown] / IDisposable on the fixture). This leaks a native arena for every test run. Consider adding a teardown:

[OneTimeTearDown]
public void TearDown() => _leakDetectionAllocator.Dispose();

Fix this →

---

2. Shared allocator can mask previous-test leaks (low, by design but worth noting)

PooledBufferLeakDetector.cs:65 — The _initialAlloc snapshot is taken at construction time. If a prior test leaked a buffer (and its own detector caught the failure), that leaked buffer is baked into the next test's _initialAlloc baseline, hiding it from subsequent assertions. This means a leaked buffer from test N-1 will never surface in test N's leak check. In practice each failing test is fixed in isolation, so this shouldn't matter, but it's worth being aware of when debugging flaky scenarios.

---

3. Leak-only tests discard deserialization results (minor)

DiscoveryMessageSerializerTests.cs:192, 205, 218, 232 — Tests like Ping_deserialize_does_not_leak_buffer call Deserialize but throw away the result:

_messageSerializationService.Deserialize<PingMsg>(serialized); // result unused

This verifies no leak, but doesn't verify the deserialized message is valid. Not a blocker, but combining both (like Enr_response_deserialize_does_not_leak_buffer_on_success does) improves test value.

---

4. Missing blank line between field and constructor (style)

DiscoveryMessageSerializerTests.cs:36-37:

private readonly PooledByteBufferAllocator _leakDetectionAllocator = PooledBufferLeakDetector.CreateAllocator();
public DiscoveryMessageSerializerTests()  // ← missing blank line

---

5. CreateAllocator() XML doc is slightly misleading (minor)

PooledBufferLeakDetector.cs:52-55 — The doc says "share a single allocator across multiple PooledBufferLeakDetector instances" but in practice each test creates exactly one PooledBufferLeakDetector and the sharing is across test methods in the same fixture. Consider rephrasing to "share a single allocator across multiple tests in a fixture".

---

Positive observations

• The _initialAlloc baseline pattern is the correct approach for shared allocators — it properly scopes each test's leak assertion to that test's allocations.
• Removing manual SafeRelease() calls in favour of using DisposableByteBuffer is safer (exception-safe) and more idiomatic.
• [Parallelizable(ParallelScope.Self)] on the fixture ensures tests within it run sequentially, which is required for the shared allocator approach to be correct.
• The comment explaining why cache sizes must be zero is clear and valuable.

---

The resource leak (#1) is the only item I'd flag as worth fixing before merge. The rest are minor.

[newmanifold]

@claude review again

1. there is no Release method for Allocator and
2. For test file in the PR we literally uses shared allocator instead of creating new one.

Can you please focus on proper findings this time

[claude]

Claude finished @newmanifold's task in 1m 53s —— View job

---

Code Review (Corrected)

• Gather context
• Read changed files
• Review changes
• Post feedback

---

Apologies for the inaccurate findings in the first pass. Correcting them now:

On finding #1 (retracted): PooledByteBufferAllocator does not implement IDisposable in DotNetty — there is no Dispose()/Release() to call. The allocator is intentionally left to GC. That finding was wrong.

On finding #5 (retracted): The XML doc says "share a single allocator across multiple PooledBufferLeakDetector instances" — that is literally what happens. Each test method constructs a new PooledBufferLeakDetector(_leakDetectionAllocator), so the shared allocator IS passed to multiple detector instances across tests. The wording is accurate. That finding was wrong.

---

Actual findings

1. Missing blank line between field and constructor (style, DiscoveryMessageSerializerTests.cs:36-37)

private readonly PooledByteBufferAllocator _leakDetectionAllocator = PooledBufferLeakDetector.CreateAllocator();
public DiscoveryMessageSerializerTests()   // ← no blank line

Per C# style convention, a blank line should separate member declarations from the constructor.

Fix this →

---

2. Leaked buffer from a failing test is baked into the next test's _initialAlloc baseline (design note, PooledBufferLeakDetector.cs:65)

_initialAlloc is snapshotted at construction time:

_initialAlloc = Allocator.Metric.HeapArenas().Sum(a => a.NumActiveAllocations);

If test N leaks a buffer and its own detector catches it (test fails), that buffer remains outstanding. Test N+1's detector then includes that leaked buffer in its baseline, so it will only catch additional leaks introduced by test N+1 — not the one carried forward. In practice this self-resolves once test N is fixed, but can make debugging confusing when multiple tests fail together.

This is a design trade-off of using a shared allocator. The [Parallelizable(ParallelScope.Self)] attribute correctly prevents concurrent interference, but sequential leakage across test failures is still possible. Not a blocker.

---

3. Dispose() asserting can produce double-failure output (minor, PooledBufferLeakDetector.cs:78-81)

public void Dispose() => AssertNoLeaks();

The public AssertNoLeaks() XML doc correctly warns: "Call at the end of the test body to get a clear failure without risk of masking the original exception (as Dispose would from a finally block)." — but Dispose() itself always calls AssertNoLeaks() unconditionally. If the test body throws before releasing a buffer, the using block's implicit finally will fire Dispose() which then also throws, and NUnit may surface only one of the two exceptions, hiding the original error.

The cleanest fix would be to guard in Dispose():

public void Dispose()
{
    if (!TestContext.CurrentContext.Result.FailCount > 0)
        AssertNoLeaks();
}

or simply document clearly that AssertNoLeaks() should be preferred over relying on Dispose() for the assertion. The existing XML comment already points this out, so it's low priority.

---

Positive observations

• Retracting finding Patricia alt 2 #5: the doc comment is accurate and the shared-allocator approach is correct.
• Retracting finding Feature/jsonrpc #1: PooledByteBufferAllocator has no disposable API, nothing to teardown.
• The _initialAlloc baseline pattern is the right solution for shared allocators.
• using DisposableByteBuffer replacing manual SafeRelease() is exception-safe and idiomatic.
• [Parallelizable(ParallelScope.Self)] correctly scopes sequential execution within the fixture, making the shared allocator pattern safe.
• The zero-cache-size explanation in the XML doc is clear and valuable.

---

Only finding #1 (blank line) is worth a trivial fix before merge. Findings #2 and #3 are design notes that don't affect correctness.

[newmanifold]

@claude Who is going to point about test redundancy? Also don't suggest the approaches that you are just speculating upon.
This time use our review skill in a sub agent and aggregate results, if you don't have permissions let me know about that.

[claude]

Claude finished @newmanifold's task in 2m 57s —— View job

---

Code Review

• Gather context
• Read changed files
• Run review skill
• Aggregate and post findings

---

Issue 1 — Test Redundancy: Four new tests duplicate existing round-trip tests (critical)

DiscoveryMessageSerializerTests.cs lines 179–233

New test	Duplicates
Ping_deserialize_does_not_leak_buffer (L180)	PingMessageTest (L49)
Pong_deserialize_does_not_leak_buffer (L196)	PongMessageTest (L86)
FindNode_deserialize_does_not_leak_buffer (L209)	FindNodeMessageTest (L269)
Neighbors_deserialize_does_not_leak_buffer (L221)	NeighborsMessageTest (L300)

All four new tests call the same ZeroSerialize + Deserialize over the same message types as the existing round-trip tests, exercising identical code paths. The only addition is the PooledBufferLeakDetector wrapper. Per the project rule at test-infrastructure.md:72 — "Do not duplicate test methods that differ only in parameters" — the correct fix is to fold the leak detector into the existing tests, not add parallel ones. Adding using PooledBufferLeakDetector detector = new(_leakDetectionAllocator) and switching the ZeroSerialize call to use detector.Allocator would convert each existing test into both a round-trip and leak test with zero duplication.

---

Issue 2 — Duplicate Port-Zero Rejection Test (critical)

DiscoveryMessageSerializerTests.cs L236 vs L341

Neighbors_deserialize_does_not_leak_buffer_on_port_zero_rejection and NeighborsMessage_Rejects_Port_Zero use identical inputs and expect the same NetworkingException. The only difference is the detector wrapper. Same fix applies: add the detector to NeighborsMessage_Rejects_Port_Zero directly and remove the duplicate.

---

Issue 3 — Inconsistent Exception-Path Leak Coverage (moderate)

PingMessage_Rejects_Port_Zero (L73), FindNodeMessage_Rejects_Oversized_Node_Id (L288), and NeighborsMessage_Rejects_Too_Many_Nodes (L355) all exercise exception paths — exactly where buffer leaks are most likely. Neighbors gets a leak-detection variant but Ping and FindNode do not. Either cover all exception paths with the detector or none; the current state is inconsistent.

---

Issue 4 — Missing blank line between field and constructor (style)

DiscoveryMessageSerializerTests.cs L36–37:

private readonly PooledByteBufferAllocator _leakDetectionAllocator = PooledBufferLeakDetector.CreateAllocator();
public DiscoveryMessageSerializerTests()   // ← no blank line

Fix this →

---

Issue 5 — test-infrastructure.md rule omits the Dispose-masks-exception caveat (minor)

.agents/rules/test-infrastructure.md L75–78 — The new rule says "use PooledBufferLeakDetector" but does not mention the key nuance documented in the class itself: that Dispose() can mask the original exception when called from a finally block (e.g., by a using statement), and that explicit AssertNoLeaks() at the end of the test body is safer. Future contributors following only the rule file will unknowingly fall into the masking trap.

---

Positive observations

• _initialAlloc snapshot pattern is the correct approach for shared allocators — properly scopes each test's assertion.
• Zero cache-size configuration (tinyCacheSize/smallCacheSize/normalCacheSize: 0) is correctly reasoned and well documented.
• [Parallelizable(ParallelScope.Self)] on the fixture ensures sequential execution within the fixture, which is required for the shared allocator to be correct.
• using DisposableByteBuffer replacing manual SafeRelease() calls is exception-safe and idiomatic.
• Enr_response_deserialize_does_not_leak_buffer_on_invalid_signature is a genuinely new test with no prior coverage — this one is not redundant.

---

Issues 1 and 2 are the blockers — they directly violate the test-deduplication rule the PR itself added to test-infrastructure.md.

[newmanifold]

@claude review again and use our review skill in a sub agent and aggregate results.

[claude]

Claude finished @newmanifold's task in 2m 51s —— View job

---

Code Review

• Gather context
• Read changed files
• Run review skill in sub-agent
• Aggregate findings and post feedback

---

The PR's goal — removing standalone leak tests and inlining detection into existing round-trip tests — is directionally correct and the implementation is solid. A few gaps follow.

---

Issue 1 — Incomplete leak coverage: six tests skip the detector (moderate)

DiscoveryMessageSerializerTests.cs lines 76, 108, 118, 128, 210, 279

The following tests call ZeroSerialize(message) with no allocator argument, so their buffers come from the default DotNetty pool — invisible to _leakDetectionAllocator:

Test	Line
PingMessage_Rejects_Port_Zero	76
Ping_with_enr_there_and_back	108
Enr_request_there_and_back	118
Enr_request_contains_hash	128
FindNodeMessage_Rejects_Oversized_Node_Id	210
NeighborsMessage_Rejects_Too_Many_Nodes	279

These include exception-path tests (PingMessage_Rejects_Port_Zero, FindNodeMessage_Rejects_Oversized_Node_Id, NeighborsMessage_Rejects_Too_Many_Nodes) — exactly the paths most prone to leaks. Passing _leakDetectionAllocator to ZeroSerialize in each would complete the coverage.

Fix this →

---

Issue 2 — Stale class-level doc comment (minor)

PooledBufferLeakDetector.cs lines 12–13, 33–35

The class summary still says:

On Dispose, asserts that the arena has zero active (unreleased) allocations.

and the <code> example block says:

// detector.Dispose() asserts no unreleased buffers remain

With the _initialAlloc baseline this is only true when the allocator is fresh. The accurate description is "asserts no net allocations since construction". The doc should be updated to reflect the actual semantic.

---

Issue 3 — Missing blank line between field and constructor (style)

DiscoveryMessageSerializerTests.cs line 36–38:

private readonly PooledByteBufferAllocator _leakDetectionAllocator = PooledBufferLeakDetector.CreateAllocator();
public DiscoveryMessageSerializerTests()

A blank line is missing between the field declaration and the constructor.

---

Positive observations

• Inlining detection into existing tests (removing the 8 standalone *_does_not_leak_buffer methods) is the right approach per the test-deduplication rule — each round-trip test now validates both correctness and absence of leaks with zero duplication.
• _initialAlloc snapshot pattern is correct for a shared allocator — scopes each test's assertion to its own allocations.
• [Parallelizable(ParallelScope.Self)] ensures sequential execution within the fixture, making the shared allocator safe.
• using DisposableByteBuffer replacing SafeRelease() is exception-safe and idiomatic.
• NeighborsMessage_Rejects_Port_Zero correctly gets the detector since exception-path leaks are the important ones — just needs to be applied uniformly.
• Enr_response_deserialize_does_not_leak_buffer_on_invalid_signature correctly retained — it covers the one previously-confirmed leak path (mismatched ENR signature).

---

Issue 1 is the only substantive gap. Issues 2 and 3 are minor polish items.