fix: dispose evicted envs from BlockCachePreWarmer pool

[newmanifold]

Changes

• Add IDisposable to IReadOnlyTxProcessorSource and IShareableTxProcessorSource so pool implementations can clean up retained items
• Replace new DefaultObjectPool<>() with DefaultObjectPoolProvider.Create() in BlockCachePreWarmer and ShareableTxProcessingSource — this selects the internal DisposableObjectPool<T> which calls Dispose() on evicted items when the pool is at capacity
• Add IDisposable to IBlockCachePreWarmer and implement Dispose() on BlockCachePreWarmer to release retained envs when the processing scope closes, rather than waiting for the parent Autofac scope to shut down
• Same fix applied to ShareableTxProcessingSource
• Rename private fields preBlockCaches/nodeStorageCache to _preBlockCaches/_nodeStorageCache for consistency with the rest of the class

Root cause

DefaultObjectPool<T> silently drops objects when the pool is at capacity without calling Dispose(). Each evicted AutoReadOnlyTxProcessingEnv held a child ILifetimeScope registered on the parent Autofac scope's disposal list — so those scopes (and the WorldState instances they own) stayed alive until the parent scope shut down, never released eagerly.

A secondary issue: even retained envs in the pool were never disposed when the processing scope closed, because neither BlockCachePreWarmer nor ShareableTxProcessingSource implemented IDisposable. Autofac skipped them on scope teardown, deferring cleanup to the parent scope.

Types of changes

• Bugfix (a non-breaking change that fixes an issue)

Testing

Requires testing

• Yes

If yes, did you write tests?

• Yes

Notes on testing

Two regression tests in BlockCachePreWarmerTests:

• PreWarmCaches_WhenPoolEvicts_EvictedEnvsAreDisposed — verifies evicted envs have Dispose() called immediately (pool capacity 1, two parallel sender groups)
• Dispose_WhenCalled_DisposesRetainedEnvsInPool — verifies all retained envs in the pool are disposed when the prewarmer itself is disposed

Documentation

Requires documentation update

• No

Requires explanation in Release Notes

• No

[newmanifold]

@claude review

[claude]

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

---

Code Review

The fix is correct and the approach is sound. DefaultObjectPoolProvider.Create<T>() selects DisposableObjectPool<T> when typeof(IDisposable).IsAssignableFrom(typeof(T)) — since IReadOnlyTxProcessorSource now extends IDisposable, this condition holds and evictions will call Dispose(). Here are the issues I found:

---

Bug / Correctness

1. Nullability mismatch in TrackingEnv.Build

src/Nethermind/Nethermind.Consensus.Test/BlockCachePreWarmerTests.cs:178

// Interface declares:
IReadOnlyTxProcessingScope Build(BlockHeader? baseBlock);

// TrackingEnv implements:
public IReadOnlyTxProcessingScope Build(BlockHeader baseBlock) =>

The parameter is BlockHeader (non-nullable) vs. BlockHeader? on the interface. This will produce a nullability annotation warning and is inconsistent with the interface contract. Fix this →

---

Test Reliability

2. Test can pass vacuously if no eviction occurs

src/Nethermind/Nethermind.Consensus.Test/BlockCachePreWarmerTests.cs:141-145

int evictedCount = created.Count - 1; // at most 1 retained in pool
disposed.Count.Should().BeGreaterThanOrEqualTo(evictedCount, ...);

If the thread scheduler runs both sender groups sequentially on the same thread, only 1 env is ever created. Then evictedCount = 0, disposed.Count = 0 >= 0 — the test passes but verifies nothing. The comment at line 124 says "at least one eviction must have occurred" but the assertion doesn't enforce it.

Consider adding an assertion that forces eviction to have been observable:

created.Count.Should().BeGreaterThanOrEqualTo(2, "two distinct senders must exercise two concurrent workers");

Fix this →

---

Minor Style

3. Redundant IDisposable on AutoReadOnlyTxProcessingEnv

src/Nethermind/Nethermind.Consensus/Processing/AutoReadOnlyTxProcessingEnvFactory.cs:29

public class AutoReadOnlyTxProcessingEnv(...) : IReadOnlyTxProcessorSource, IDisposable

Since IReadOnlyTxProcessorSource now inherits IDisposable, listing , IDisposable explicitly is redundant. Can be simplified to : IReadOnlyTxProcessorSource.

4. Odd field name _envPool2 in ScopeWrapper

src/Nethermind/Nethermind.Consensus/Processing/ShareableTxProcessingSource.cs:49

private readonly ObjectPool<IReadOnlyTxProcessorSource> _envPool2;

The 2 suffix serves no purpose. Should be _envPool.

---

Observation (not a bug, but worth noting)

5. BlockCachePreWarmer and ShareableTxProcessingSource don't dispose their pools

Both classes hold an ObjectPool<IReadOnlyTxProcessorSource> which, after this fix, is a DisposableObjectPool<T>. Neither class implements IDisposable, so when they are replaced/reconfigured, the pool's retained items won't have Dispose() called. Since both are long-lived singletons this isn't a practical problem today, but it's worth being aware of if lifetime management changes in the future.

---

Summary

The core fix is correct — the mechanism works and the AuRa test teardowns are a good catch. Items 1 and 2 are worth fixing before merge; items 3 and 4 are minor cleanup.

[newmanifold]

@claude review again

[claude]

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

---

Code Review

• Gathering context
• Reading changed files
• Analyzing and writing review

---

All four items from the previous review have been addressed. Here's the updated assessment:

Previously Flagged — Now Fixed

#	Issue	Status
1	Nullability mismatch TrackingEnv.Build(BlockHeader baseBlock)	✅ Fixed — now BlockHeader? baseBlock
2	Test could pass vacuously if no eviction occurs	✅ Fixed — created.Count >= 2 assertion added first
3	Redundant explicit IDisposable on AutoReadOnlyTxProcessingEnv	✅ Fixed — now only : IReadOnlyTxProcessorSource
4	Misleading field name _envPool2	✅ Fixed — renamed to _envPool

---

Remaining Minor Issue

Constructor parameter still named envPool2 in ScopeWrapper

src/Nethermind/Nethermind.Consensus/Processing/ShareableTxProcessingSource.cs:49

public ScopeWrapper(IReadOnlyTxProcessorSource source, ObjectPool<IReadOnlyTxProcessorSource> envPool2, IReadOnlyTxProcessingScope scope)

The field was correctly renamed to _envPool but the constructor parameter still carries the old envPool2 name. Cosmetic only — no functional impact.
Fix this →

---

Correctness Check

The core mechanism is sound: DefaultObjectPoolProvider.Create<T>() checks typeof(IDisposable).IsAssignableFrom(typeof(T)) at runtime. Since IReadOnlyTxProcessorSource now inherits IDisposable, this condition holds and DisposableObjectPool<T> is selected, which calls Dispose() on any item the pool cannot retain.

NSubstitute mocks in TxPermissionFilterTest and AuRaAdditionalBlockProcessorFactoryTests use Substitute.For<IReadOnlyTxProcessorSource>() — NSubstitute automatically implements Dispose() as a no-op, so those tests are unaffected.

The only two concrete implementations (AutoReadOnlyTxProcessingEnv and test TrackingEnv) both implement Dispose() correctly.

---

Standing Observation (non-blocking)

BlockCachePreWarmer and ShareableTxProcessingSource hold a DisposableObjectPool<T> (which is itself IDisposable) but neither class implements IDisposable. Retained items in the pool won't be disposed if the pool is ever replaced or the class goes out of scope without the pool being disposed. Since both are long-lived singletons in practice this isn't a leak today, but worth tracking if lifetime management changes.

---

Summary

The fix is correct and all substantive issues have been resolved. The envPool2 parameter name is the only remaining cosmetic nit. Ready to merge pending that optional cleanup.

[newmanifold]

@claude review please

[claude]

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

---

Code Review

• Gathering context
• Reading changed files
• Analyzing and writing review

---

This revision adds two meaningful improvements on top of the previous pass: BlockCachePreWarmer now implements IDisposable (and IBlockCachePreWarmer extends it), and a second test Dispose_WhenCalled_DisposesRetainedEnvsInPool validates that retained pool items are disposed on shutdown.

---

Still Open (from previous review)

1. Constructor parameter envPool2 still not renamed

src/Nethermind/Nethermind.Consensus/Processing/ShareableTxProcessingSource.cs:49

public ScopeWrapper(IReadOnlyTxProcessorSource source, ObjectPool<IReadOnlyTxProcessorSource> envPool2, IReadOnlyTxProcessingScope scope)

The field was correctly renamed to _envPool but the constructor parameter still reads envPool2. Cosmetic only, but was flagged in the last two passes.
Fix this →

---

New Issues in This Revision

2. Private fields missing _ prefix

src/Nethermind/Nethermind.Consensus/Processing/BlockCachePreWarmer.cs:33-34

private readonly PreBlockCaches preBlockCaches;
private readonly NodeStorageCache nodeStorageCache;

The other private fields in the same class (_concurrencyLevel, _envPool, _logger) use the _camelCase convention. These two were introduced in the refactor and break that consistency. Should be _preBlockCaches and _nodeStorageCache.
Fix this →

3. Fragile (IDisposable)_envPool cast in Dispose()

src/Nethermind/Nethermind.Consensus/Processing/BlockCachePreWarmer.cs:103

public void Dispose() => ((IDisposable)_envPool).Dispose();

This relies on DefaultObjectPoolProvider.Create<T>() returning a DisposableObjectPool<T> at runtime (which it does when T : IDisposable). But ObjectPool<T> doesn't expose IDisposable in its static type, so the cast will throw InvalidCastException if that implementation detail ever changes (e.g. if the policy is swapped out in a future refactor or test). A safer pattern is to capture the disposable at construction time:

// In the constructor:
ObjectPool<IReadOnlyTxProcessorSource> pool = new DefaultObjectPoolProvider { MaximumRetained = maxPoolSize }.Create(poolPolicy);
_envPool = pool;
_poolDisposable = pool as IDisposable; // or store IDisposable separately

// Dispose:
public void Dispose() => _poolDisposable?.Dispose();

Or if you want a hard guarantee, assert it in the constructor:

Debug.Assert(_envPool is IDisposable, "Pool must be disposable for env cleanup to work");

Fix this →

---

Previously Flagged — Now Fixed

#	Issue	Status
1	Nullability mismatch TrackingEnv.Build(BlockHeader baseBlock)	✅ Fixed
2	Test vacuous-pass if no eviction occurs	✅ Fixed
3	Redundant explicit IDisposable on AutoReadOnlyTxProcessingEnv	✅ Fixed
4	_envPool2 field name	✅ Fixed
5	BlockCachePreWarmer didn't dispose its pool (observation)	✅ Addressed — Dispose() implemented and IBlockCachePreWarmer extended

---

Minor Observation (non-blocking)

ShareableTxProcessingSource now creates a DisposableObjectPool<T> via DefaultObjectPoolProvider().Create(...) but the class still doesn't implement IDisposable, so retained items in its pool won't be disposed if the source is ever replaced or goes out of scope. This is consistent with the previous observation — not a regression here, just not fully symmetric with BlockCachePreWarmer.

---

Summary

Items 2 and 3 above are worth fixing before merge — item 2 is a style violation, item 3 is a latent correctness risk. Item 1 (the envPool2 rename) has been outstanding for three review rounds. The core fix remains correct.

[newmanifold]

@claude review again

[claude]

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

---

Code Review

• Gathering context
• Reading changed files
• Analyzing and writing review

---

All three issues flagged in the previous review have been addressed.

Previously Flagged — Now Fixed

#	Issue	Status
1	Constructor parameter envPool2 not renamed in ScopeWrapper	✅ Fixed — now envPool
2	Private fields missing _ prefix (preBlockCaches, nodeStorageCache)	✅ Fixed — now _preBlockCaches, _nodeStorageCache
3	Fragile hard cast ((IDisposable)_envPool).Dispose()	✅ Fixed — now (_envPool as IDisposable)?.Dispose()

---

Correctness Check

The core mechanism is correct throughout:

• IReadOnlyTxProcessorSource extends IDisposable → DefaultObjectPoolProvider.Create<T>() selects DisposableObjectPool<T> at runtime → evicted items get Dispose() called immediately.
• BlockCachePreWarmer.Dispose() safely disposes the pool via the as pattern, releasing retained items when the warmer shuts down.
• AutoReadOnlyTxProcessingEnv implements only : IReadOnlyTxProcessorSource (no redundant IDisposable).
• Both tests are well-structured: the eviction test correctly asserts created.Count >= 2 before checking the eviction count, preventing a vacuous pass.

---

Standing Observation (non-blocking)

ShareableTxProcessingSource creates a DisposableObjectPool<T> (via DefaultObjectPoolProvider().Create(...)) but the class itself still doesn't implement IDisposable, so retained items in its pool won't be disposed if the source is replaced or goes out of scope. Consistent with prior observations — not a regression introduced by this PR, and not a practical issue given its singleton lifetime.

---

Summary

The fix is correct, all substantive and cosmetic issues have been resolved, and the tests provide good regression coverage for both eviction and disposal on shutdown. Ready to merge.

[newmanifold]

@claude review please

[claude]

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

---

Code Review

• Gathering context
• Reading changed files
• Analyzing and writing review

---

This revision addresses all issues from the previous round and adds one more meaningful improvement: IShareableTxProcessorSource now extends IDisposable and ShareableTxProcessingSource implements Dispose(), fully closing the "standing observation" that had been carried across all four review passes.

---

Previously Flagged — Now Fixed

#	Issue	Status
1	Nullability mismatch TrackingEnv.Build(BlockHeader baseBlock)	✅ Fixed
2	Test could pass vacuously if no eviction occurs	✅ Fixed
3	Redundant explicit IDisposable on AutoReadOnlyTxProcessingEnv	✅ Fixed
4	_envPool2 field and constructor parameter	✅ Fixed
5	Private fields missing _ prefix	✅ Fixed
6	Fragile hard cast ((IDisposable)_envPool).Dispose()	✅ Fixed — as pattern used
7	ShareableTxProcessingSource didn't dispose its pool (standing observation)	✅ Fixed — IShareableTxProcessorSource extends IDisposable, Dispose() implemented

---

One Remaining Nit

_envPool field in ShareableTxProcessingSource missing private readonly

src/Nethermind/Nethermind.Consensus/Processing/ShareableTxProcessingSource.cs:21

ObjectPool<IReadOnlyTxProcessorSource> _envPool = new DefaultObjectPoolProvider().Create(new EnvPoolPolicy(envFactory));

The equivalent field in BlockCachePreWarmer is declared private readonly. This field is implicitly private (C# default for class members) and is never reassigned, so private readonly would match both the convention in the sibling class and make the intent explicit.

---

Correctness Check

The core mechanism is correct throughout:

• Both DefaultObjectPoolProvider.Create<T>() calls see T : IDisposable (since IReadOnlyTxProcessorSource : IDisposable) and select DisposableObjectPool<T>, which calls Dispose() immediately on any item the pool cannot retain.
• Both Dispose() implementations use (_envPool as IDisposable)?.Dispose(), which is safe and won't throw if the pool implementation changes.
• AutoReadOnlyTxProcessingEnv satisfies IReadOnlyTxProcessorSource with a proper Dispose() that tears down its child ILifetimeScope.
• Both tests cover the two disposal paths: eviction under pool pressure, and graceful shutdown via preWarmer.Dispose().

---

Summary

All substantive and cosmetic issues have been resolved. The only remaining item is a private readonly annotation nit on _envPool in ShareableTxProcessingSource. Ready to merge.