NCBC-4279: Stop unit tests asserting exact sizes on pooled buffers - #158
Draft
davidkelly wants to merge 1 commit into
Draft
NCBC-4279: Stop unit tests asserting exact sizes on pooled buffers#158davidkelly wants to merge 1 commit into
davidkelly wants to merge 1 commit into
Conversation
Motivation ========== Five tests in SlicedMemoryOwnerTests fail intermittently on net48 only, most recently the 2026-08-08 nightly (run 31243388414, windows-2025). They call MemoryPool<byte>.Shared.Rent(32) and then assume exactly 32 bytes, but Rent promises only *at least* that many. Given 64, the expected lengths 32/22/1 come back as 64/54/33 and start=33 is genuinely in range, so the guard correctly does not throw. net48 alone because .NET Framework's pool falls back to a larger bucket when the requested one is exhausted, where Core's allocates the exact bucket size. That makes it load-dependent, which is why the per-TFM split in nightly-unit-tests.yml reduced it without fixing it: xUnit still parallelizes collections within the net48 process. Four EnsureCapacity tests in OperationBuilderTests share the mechanism. Capacity returns the length of a buffer rented from ArrayPool, so asserting it doubles or quadruples asserts the pool's bucket rounding - OperationBuilder has no growth logic, EnsureCapacity just rents. These have not yet failed, the 16KB+ buckets being far less contended than the 32-byte one, but the defect is the same. Modification ============ SlicedMemoryOwnerTests: replace every Rent(32) with a FakeMemoryOwner<byte> over a plain byte[], as the ..._SliceStart tests in the same file already do. Two tests that cannot fail today are converted too, so the pattern is not left behind to be copied. OperationBuilderTests: assert capacity is at least what was requested, which is what EnsureCapacity guarantees and what its own Debug.Assert states, and rename _Doubles/_Quadruples to _Grows to match. No production code changes. Results ======= All 84 tests in the two classes pass on net8.0 locally. net48 cannot be built on macOS, so this shows the rewrites are behaviour-preserving, not that the flake is gone; the case for that is structural, since neither class now reads a pooled buffer's length. CI covers net48. Other MemoryPool uses in tests go through RentAndSlice, which slices to the requested length, and are unaffected. GetBuffer is likewise sliced to the written length; no test asserts on the length GetMemory/GetSpan return. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
davidkelly
force-pushed
the
dk/ncbc-4279
branch
from
August 8, 2026 16:41
a011154 to
9b1a0c4
Compare
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.
Motivation
Five tests in
SlicedMemoryOwnerTestsfail intermittently on net48 only, most recently the 2026-08-08 nightly (run 31243388414), wherewindows-2025/net48went red withFailed: 5, Passed: 2769while the other 17 legs — includingwindows-2022/net48on the same commit — passed.ctor1_StartWithinRange_SliceUntilEnd(0, 32)ctor1_StartWithinRange_SliceUntilEnd(10, 22)ctor1_StartWithinRange_SliceUntilEnd(31, 1)ctor1_StartOutsideRange_ArgumentOutOfRangeException(33)ctor2_StartOutsideRange_ArgumentOutOfRangeException(32)The tests are wrong, not
SlicedMemoryOwner. Each callsMemoryPool<byte>.Shared.Rent(32)and then assumes it got exactly 32 bytes, butRent(minBufferSize)promises only at least that many. Given a 64-byte buffer,_length = memory.Length - startyields 64/54/33, andstart=33is genuinely in range so the guard correctly does not throw.SlicedMemoryOwner's own summary says as much — it exists because a pool "may return a larger array of memory than desired".net48 alone because .NET Framework's shared pool searches the next couple of buckets and hands back a 64-byte array when the 32 bucket is exhausted, where Core's allocates exactly the bucket size. That makes the failure load-dependent — hence one Windows image and not the other, and hence it survived the per-TFM job split in
nightly-unit-tests.yml: that split removed contention between TFMs, but xUnit still runs collections in parallel within the single net48 process.Sweeping the test tree for the same pattern turned up one other site. Four
EnsureCapacitytests inOperationBuilderTestsassertCapacitydoubles or quadruples, butCapacityreturns the length of a buffer rented fromArrayPool(OperationBuilder.cs:71,:521), so they assert the pool's bucket rounding.OperationBuilderimplements no growth logic at all —EnsureCapacityjust callsRent(capacity), and its ownDebug.Assert(_buffer.Length >= capacity)(:535) states the real contract. These have not failed yet, the 16KB+ buckets being far less contended than the 32-byte one, but the defect is identical.Modification
SlicedMemoryOwnerTests— replace everyMemoryPool<byte>.Shared.Rent(32)with a privateExactlySized(int)helper returning aFakeMemoryOwner<byte>over a plainbyte[], the same helper the..._SliceStarttests in this file already use. Two tests that cannot fail today are converted as well: leavingRentin the file invites the pattern into a test where it does matter.OperationBuilderTests— assert capacity is at least what was requested, and rename_Doubles/_Quadruplesto_Growsto match. The three_NoChangetests are untouched; they exercise the no-grow path and are already immune.No production code changes.
Results
All 84 tests across the two classes pass on net8.0 locally.
net48 cannot be built on macOS —
Directory.Build.propsadds it toSdkTestTargetsonly on Windows — so the local run shows the rewrites are behaviour-preserving but cannot show the flake is gone, net8.0 having never flaked. The argument is structural: neither class now reads a pooled buffer's length. Note the PR gate runs a singlewindows-latestnet48 leg, so a green run here is not proof either, the failure being load-dependent; the nightly'swindows-2025andwindows-2022net48 legs are the real signal over the next few days.The rest of the test tree is clean. Remaining
MemoryPooluses all go throughRentAndSlice, which slices to the exact requested length (MemoryPoolExtensionsTests.RentAndSlice_RequestLength_ReturnsExactlyThatLengthis the regression test for this hazard).GetBuffer()is likewise sliced to the written length.GetMemory()/GetSpan()do return a pool-dependent length, but every test uses them purely as a write target and no test asserts on it.Not addressed here: the 2026-08-06 nightly failure (run 31078666014,
ubuntu-24.04/net10.0) is unrelated — all 2844 tests passed and--blame-hangthen killed the host, blaming StellarCollectionTests.🤖 Generated with Claude Code