test: deterministic prewarmer env-pool eviction test

[svlachakis]

Changes

• PreWarmCaches_WhenPoolEvicts_EvictedEnvsAreDisposed asserted created.Count >= 2, which requires the two sender-group warmup iterations to run on two workers concurrently. That is scheduler-dependent: on a constrained runner one worker can warm both groups before the second pool thread wakes, so only one env is created and the test fails (seen on CI: Expected: greater than or equal to 2 / But was: 1).
• The eviction-disposal contract ("an env returned to a full pool is disposed immediately") is now exercised deterministically by EnvPool_ReturnedBeyondCapacity_IsDisposedImmediately: it builds a capacity-1 DisposableObjectPool from the same framework provider and env policy, then does two overlapping rentals and two returns single-threaded — no scheduling dependence.
• Production code is unchanged (BlockCachePreWarmer is byte-identical to master; no internals exposed for tests). That the real prewarmer wires up a disposing pool is already covered through the public Dispose() API by Dispose_WhenCalled_DisposesRetainedEnvsInPool, so there is no coverage gap.

Types of changes

What types of changes does your code introduce?

• Other: test de-flaking

[claude]

Claude finished @svlachakis's task in 1m 31s —— View job

---

Review: test de-flake for prewarmer env-pool eviction

I traced the pool semantics against the actual wiring and this is a solid, well-reasoned de-flake. ✅

Why the old test was flaky (confirmed): created.Count >= 2 required both sender-group warmup iterations to land on two workers concurrently. On a constrained runner one worker can warm both groups before the second pool thread wakes → only one env created → Expected: >= 2 / But was: 1. Genuine scheduler dependence.

Why the new test is deterministic (verified):

• IReadOnlyTxProcessorSource : IDisposable (Nethermind.Blockchain/IReadOnlyTxProcessorSource.cs:14), so DefaultObjectPoolProvider.Create yields a DisposableObjectPool<T> — the eviction-on-return-disposes contract is exactly what's under test.
• With MaximumRetained = 1, the pool retains a single slot (_firstItem). Single-threaded trace:
  • Get() ×2 on an empty pool → two fresh envs (created.Count == 2) ✅
  • Return(first) fills the one slot; Return(second) can't be retained → DisposableObjectPool disposes it → disposed.Count == 1, disposed contains second ✅
• No threads, no SpinUntil, no timing — same pool + same policy the prewarmer builds, so coverage of the contract is preserved.

Repo-rule adherence:

• internal EnvPool + existing [assembly: InternalsVisibleTo("Nethermind.Consensus.Test")] (Processing/BlockExtensions.cs:13) — minimal, test-only surface. Acceptable.
• No production behavior changed; regression contract is now exercised more directly.

Findings

Low — stale doc comment (not introduced by this PR). BuildTwoSenderBlock (line ~634) still reads "guaranteeing concurrent pool usage", which was the premise of the removed test. It's still used by other tests for its two-sender-group structure, so the claim is now slightly overstated. Optional to trim; per repo rules on not touching unrelated code, leaving it is also fine.

Low — preWarmer not disposed on assertion failure. preWarmer.Dispose() (line 106) runs only after the Assert.EnterMultipleScope() block; if an assertion throws, the scope disposal propagates before Dispose(), leaking the prewarmer's child scopes for that test run. This matches the pattern of the sibling tests (which don't dispose the tracking prewarmer at all), so it's consistent — a using/finally would be marginally cleaner but is not required.

Neither finding blocks merge. No Critical/High/Medium issues.

• Read PR diff and changed files
• Analyze pool eviction semantics for determinism
• Check design / repo-rule adherence
• Post review

[claude]

Claude finished @svlachakis's task in 1m 6s —— View job

---

Review: deterministic prewarmer env-pool eviction test

Traced the pool semantics against the actual wiring end-to-end. This is a correct, well-scoped de-flake. ✅

Root cause of the old flake (confirmed): Assert.That(created.Count, Is.GreaterThanOrEqualTo(2)) required both sender-group warmup iterations to run on two pool workers concurrently. On a constrained runner one worker can warm both groups before the second thread wakes → one env created → Expected: >= 2 / But was: 1. Genuine scheduler dependence, not a product bug.

New test is deterministic (verified line-by-line):

• _envPool is built by DefaultObjectPoolProvider { MaximumRetained = maxPoolSize }.Create(poolPolicy) (BlockCachePreWarmer.cs:86). Since IReadOnlyTxProcessorSource : IDisposable, Create returns a DisposableObjectPool<T> — so the "return-beyond-capacity disposes immediately" contract is exactly the code under test.
• Single-threaded trace with maxPoolSize: 1 and DisposalTrackingPolicy (BlockCachePreWarmerTests.cs:695, whose Return returns true):
  • Get() ×2 on an empty pool → both miss the single retained slot → two Create() calls → created.Count == 2 ✅ (matches the new precondition assert at line 93)
  • Return(first) fills the one slot; Return(second) — policy returns true but the slot is taken and the retained array is empty (MaximumRetained = 1) → DisposableObjectPool disposes it → disposed.Count == 1, disposed contains second ✅
• No threads, SpinUntil, or timing. Same pool + same policy path the prewarmer uses in production, so contract coverage is preserved, not weakened.

Production change: private readonly → internal readonly on _envPool (BlockCachePreWarmer.cs:37), consumed only via the existing [InternalsVisibleTo("Nethermind.Consensus.Test")]. Minimal, test-only surface, no behavior change. Acceptable per repo rules.

Findings

Low — stale doc comment (pre-existing, not introduced here). BuildTwoSenderBlock (BlockCachePreWarmerTests.cs:631-634) still says "guaranteeing concurrent pool usage" — the premise of the now-removed test. The helper is still used by other tests for its two-sender structure, so the "guaranteeing concurrent" claim is now overstated. Optional to trim; per repo rules on not touching unrelated code, leaving it verbatim is equally fine.

Low — preWarmer.Dispose() skipped on assertion failure. Dispose() at line 104 runs only after the Assert.EnterMultipleScope() block; if an assertion in that scope fails, the throw propagates before Dispose(), leaking the prewarmer's child scopes for that one test run. Consistent with the sibling tests (which don't dispose their tracking prewarmer either), so not a regression — a try/finally or using would be marginally cleaner but isn't required.

Neither finding blocks merge. No Critical/High/Medium issues — mergeable.

• Read PR diff and changed files
• Trace pool eviction semantics for determinism
• Verify DisposableObjectPool + policy wiring
• Check repo-rule / design adherence
• Post review