Allow HistoryPruner to be callable via decorator

[damian-orzechowski]

Fixes Closes Resolves #
Extend HistoryPruner to be more configurable and change method to public to be callable if wrapped / decorated.

Changes

• Make SchedulePruneHistory public
• Promote background task timeout to config -> PruningTimeoutSeconds
• Allow to set different minimal block to protect than 0 (genesis) -> Added to config, instead of runtime

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

Documentation

Requires documentation update

• Yes
• No

Requires explanation in Release Notes

• Yes
• No

Remarks

[damian-orzechowski]

@claude review

[claude]

Claude finished @damian-orzechowski's task in 1m 40s —— View job

---

Code Review

• Gather context and understand the request
• Read coding style and robustness rules
• Read changed files
• Analyze changes and provide review feedback

---

Overall the PR is well-conceived and the tests are good. There are a few issues worth addressing before merge.

---

Critical

1. SetMinDeletableBlockNumber is not thread-safe (HistoryPruner.cs:301-309)

The method reads _hasLoadedDeletePointer and writes _deletePointer without holding _pruneLock, while TryPruneHistory and TryLoadDeletePointer read/write these same fields under that lock. The PR comment says "Must be called before the first pruning pass" but this is not enforced — a caller can invoke it concurrently with a scheduled prune. At minimum, access should be guarded by _pruneLock, or the method should document this precondition more rigorously and throw if called after loading.

// Unsafe: no lock held, _hasLoadedDeletePointer / _deletePointer can race
if (_hasLoadedDeletePointer && _deletePointer < _minDeletableBlockNumber)
    _deletePointer = _minDeletableBlockNumber;

Similarly, _minDeletableBlockNumber is written here and read in SetDeletePointerToOldestBlock, PruneBlocksAndReceipts, and TryLoadDeletePointer without any synchronisation. At minimum, use Volatile.Write/Volatile.Read or read it under _pruneLock.

Fix this →

---

Significant

2. New public methods are not on IHistoryPruner (IHistoryPruner.cs)

The stated goal is to allow HistoryPruner to be wrapped/decorated. Neither SchedulePruneHistory nor SetMinDeletableBlockNumber appear on the IHistoryPruner interface. Any decorator would still need to downcast to HistoryPruner, which defeats the decorator pattern entirely. The two methods should be added to the interface (or a new extended interface) to make them properly composable.

Fix this →

---

Medium

3. SchedulePruneHistory has no XML doc (HistoryPruner.cs:252)

Per the coding-style rules, all public APIs require documentation comments with <summary> (and <param> where applicable). SetMinDeletableBlockNumber has a doc, but SchedulePruneHistory (now public) does not.

4. Log message range notation is misleading (HistoryPruner.cs:458)

// Range is half-open: [minDeletable, syncPivot), but the log says "({_minDeletableBlockNumber}, ...)"
_logger.Warn($"... Should be in range ({_minDeletableBlockNumber}, {_blockTree.SyncPivot.BlockNumber}).");

The parentheses imply exclusive on both ends, but the actual condition is number < _minDeletableBlockNumber (inclusive lower bound). With default _minDeletableBlockNumber = 1 this would say (1, X) but block 1 itself is a valid deletable block. Consider [{_minDeletableBlockNumber}, {_blockTree.SyncPivot.BlockNumber}) or prose.

5. PruningTimeoutSeconds default of 2 is a behaviour change from no timeout

The previous code had no timeout; this PR sets a default of 2 seconds. On a slow node or large pruning pass, 2 seconds will cause the operation to be cancelled every time, effectively making history pruning a no-op. Consider a larger default (e.g. 30 or 60 seconds), or document clearly in the config description that the previous behaviour was no timeout and this changes it.

6. CapturingScheduler.source parameter should be nullable (HistoryPrunerTests.cs)

public bool TryScheduleTask<TReq>(..., string source = null)
//                                        ^^^^^^ should be string?

In a nullable-enabled project, string source = null will produce a nullable warning.

---

Minor

7. SetMinDeletableBlockNumber doc says "Must be called before the first pruning pass" but this is not enforced

Consider throwing InvalidOperationException if called after _hasLoadedDeletePointer is true, or at least adding a warning log. As written, silent incorrect behaviour is possible if a caller violates the precondition.

8. Missing <param> tag in SetMinDeletableBlockNumber XML doc

The existing <summary> is good; add <param name="minBlockNumber">.

---

[damian-orzechowski]

@claude re-review

[claude]

Claude finished @damian-orzechowski's task in 1m 59s —— View job

---

Re-Review

• Gather context
• Read coding style and robustness rules
• Read changed files
• Analyze changes vs previous review
• Post updated review

---

Good improvement from the first pass. Here's the updated status:

Fixed Since Previous Review

• ✅ Thread-safety — SetMinDeletableBlockNumber was removed entirely; MinDeletableBlockNumber is now set once from config and stored as a readonly-equivalent field in the constructor. No more race condition.
• ✅ Interface completeness — SchedulePruneHistory is now on IHistoryPruner (line 18), making decorators viable without downcasting.
• ✅ XML doc on SchedulePruneHistory — <summary> and <param> are present (HistoryPruner.cs:253-256).
• ✅ Log range notation — Now correctly [{_minDeletableBlockNumber}, {_blockTree.SyncPivot.BlockNumber}) with inclusive/exclusive brackets (HistoryPruner.cs:448).

---

Still Open

1. PruningTimeoutSeconds default of 2 is a silent regression for existing operators (IHistoryConfig.cs:29-31)

The previous code had no timeout. This PR introduces a 2-second default. On a slow node, archive chain, or first pruning pass after a long gap, 2 seconds will cancel the operation every single time, making pruning a de-facto no-op that logs "Pruning operation timed out" and then silently moves on. The config description says "Set to 0 to disable the timeout" which is good, but operators upgrading from an older build won't know their pruning has changed behavior.

Consider a significantly larger default (30–60 s), or document the upgrade impact in the PR description.

2. CapturingScheduler.source parameter should be nullable (HistoryPrunerTests.cs:547)

// Current — nullable warning in a nullable-enabled project
public bool TryScheduleTask<TReq>(..., string source = null)

// Fix:
public bool TryScheduleTask<TReq>(..., string? source = null)

Fix this →

---

New Observations (no action required)

• Math.Max(1, historyConfig.MinDeletableBlockNumber) in the constructor (HistoryPruner.cs:94) correctly clamps any negative or zero config value — good defensive clamping without the overhead of a runtime check on every prune pass.
• The three new MinDeletableBlockNumber_* tests cover the important cases well: preserves blocks below min, clamps stale DB pointer on startup, and doesn't regress an already-advanced pointer on restart.
• Volatile.Read + Interlocked.CompareExchange pattern in SchedulePruneHistory is correct for the double-check lock-free guard.

---

Overall the PR is in good shape. The two remaining items are minor — the nullability warning is a one-character fix, and the timeout default is a judgment call worth documenting. Ready to merge once those are addressed.

[damian-orzechowski]

@claude - regarding:

• point 1 - timeout defaults to 2s to ensure match the default value which was used before: BackgroundTaskScheduler.DefaultTimeout
• point 2 - change will cause this: "The annotation for nullable reference types should only be used in code within a '#nullable' annotations context"

[claude]

Claude finished @damian-orzechowski's task in 49s —— View job

---

Re-Review (follow-up)

• Verify point 1: check BackgroundTaskScheduler.DefaultTimeout
• Verify point 2: check nullable annotations context in test project

---

Both clarifications are correct — closing both remaining items.

Point 1 — PruningTimeoutSeconds default of 2s ✅

Confirmed: BackgroundTaskScheduler.DefaultTimeout = TimeSpan.FromSeconds(2) (Nethermind.Consensus/Scheduler/BackgroundTaskScheduler.cs:27). The 2s default preserves the existing timeout behaviour exactly. This is not a regression — withdrawing that concern.

Point 2 — string? nullable annotation ✅

Confirmed: neither Directory.Build.props nor the test project's .csproj/tests.props sets <Nullable>enable</Nullable>, so the project has no nullable annotations context. Using string? there would produce a compiler error (CS8632), not fix a warning. The current string source = null with a nullable-disabled project is correct as-is. Withdrawing that concern.

---

All previously raised issues are now resolved. The PR is ready to merge.

View job

[LukaszRozmej]

Do we need it as separate config? Can we just use the same thing we use to determine which block is genesis?

[LukaszRozmej]

what happens on pruningTimeout? Maybe better to use custom time-outable CancellationTokenSource that will cancel gracefully?

[LukaszRozmej]

Why is it needed?