fix(snap-sync): drive FlatSnapTrieFactory init/finalize from SnapSyncRunner.Run

[LukaszRozmej]

Summary

Ports the snap-sync flake fix from #11443 onto @asdacap's snap+state dispatcher refactor (#11102), in the cleaner shape he suggested: drive FlatSnapTrieFactory.Clear() from SnapSyncRunner.Run boundaries instead of a lazy EnsureDatabaseCleared lock.

This PR is stacked on top of amirul/simple-sync-dispatcher so it can land alongside #11102.

Background

#11443 root-caused a Windows-only E2ESyncTests.SnapSync flake to a race in FlatSnapTrieFactory.EnsureDatabaseCleared:

if (_initialized) return;          // unlocked fast path
using (_lock.EnterScope())
{
    if (_initialized) return;
    _initialized = true;            // ← published BEFORE Clear() ran
    persistence.Clear();             // ← still iterating GetAllKeys / queueing Removes
}

Thread T2 could read _initialized = true via the unlocked fast path while T1 was still inside Clear(). T2 would then call CreateStateTree and start writing accounts. When T1's Clear batch finally disposed, every queued Remove (including the keys T2 had just committed) would land on the live DB, and the flat DB ended up missing entries the trie still referenced — surfacing as "Account in trie not found in flat" in the verifier.

#11443 fixed it by publishing _initialized after Clear() and marking the field volatile. Validated as 0/300 stress runs vs. 8–15/60 before.

What this PR does

@asdacap pointed out that on top of #11102's runner refactor, the lazy-init pattern can go away entirely — the snap-sync run has a single sequential entry/exit point in SnapSyncRunner.Run, so init/cleanup can be hooked there:

• Add two default-no-op methods to ISnapTrieFactory:
  • EnsureInitialize() — called once before any tree is created.
  • FinalizeSync() — called once after the dispatcher has drained.
• FlatSnapTrieFactory implements them with persistence.Clear() and persistence.Flush() respectively. The lock and the _initialized flag are deleted.
• SnapSyncRunner.Run brackets dispatcher.Run(token) with EnsureInitialize (before) and FinalizeSync (in finally).
• PatriciaSnapTrieFactory, TestSnapTrieFactory, and the other test factories pick up the empty default implementations and don't need changes.

Because SimpleDispatcher.Run (introduced by #11102) only returns once every batch handler — and therefore every ISnapTree.Dispose — has completed, neither hook needs internal locking. This also obviates the in-flight tree drain that #11443 needed on master (commit 12a66f1e1b).

Stress reproducer

The same SnapSync_StressRepro test from #11443 (cherry-picked here) runs 30 iterations × 2 Flat fixtures = 60 cases, no [Retry], [Explicit]. Local Windows results on this branch:

Run	Verification failed	Test failures
Run 1	0	0
Run 2	0	0
Run 3	0	0
Total	0/180	0/180

Plus full Flat test suite (292 tests, 4 skipped) and RecreateState* (20 tests) and StateSyncFeed* (834 tests) all pass.

Test plan

• Solution builds clean (0 warnings, 0 errors).
• Nethermind.State.Flat.Test passes (292 tests).
• RecreateStateFromAccountRanges / RecreateStateFromStorageRanges pass (20 tests).
• StateSyncFeed* passes (834 tests).
• SnapSync_StressRepro: 0 failures across 3 stress runs (180 invocations).

[claude]

Claude finished @LukaszRozmej's task in 3m 53s —— View job

---

PR Review

• Load repo rules and context
• Read changed files
• Analyze correctness and edge cases
• Check security and performance
• Post findings

---

Summary

The fix is correct and well-designed. The core reasoning holds up: since ISnapTree instances are always created and disposed within using blocks in SnapProviderHelper.AddAccountRange/AddStorageRange, they are fully disposed before HandleResponse returns, and SimpleDispatcher.Run waits for all in-flight Task.Run workers via the semaphore drain before returning. This means FinalizeSync is guaranteed to run after every write batch has been committed and disposed.

The lazy-init pattern (_initialized + lock) is cleanly replaced by lifecycle hooks at the single sequential entry/exit point — a meaningfully simpler and safer design.

Critical: 0 | High: 0 | Medium: 0 | Low: 2

---

Findings

Low — FinalizeSync now also runs on every cancellation/exception path

SnapSyncRunner.Run calls FinalizeSync() in a finally, so persistence.Flush() is called on every exit including early cancellation (e.g. shutdown, pivot change). The old code never flushed on these paths. In practice this is harmless — flushing a partial or empty WAL is idempotent and fast — but it is a behavioural change worth noting for reviewers who benchmark startup/shutdown times.

If a deliberate no-flush-on-cancel behaviour is ever needed, a flag on FinalizeSync can be added later; the current choice (always flush) is the safer default for durability.

Low — No targeted unit test for SnapSyncRunner lifecycle ordering

The E2E stress test covers the concrete race via 180 invocations, but there is no lightweight unit test that asserts EnsureInitialize is called before any CreateStateTree/CreateStorageTree, and FinalizeSync after. A mock ISnapTrieFactory + mock SimpleDispatcher could capture call order cheaply. Optional, given the stress coverage, but would document the contract as executable spec.

---

Code review notes

SimpleDispatcher.Run drain correctness (lines 80-81):

for (int i = 0; i < maxThreads; i++)
    await semaphore.WaitAsync(CancellationToken.None);

Each started Task.Run holds exactly one semaphore slot and releases it in finally. When semaphore.WaitAsync(token) at line 64 is cancelled the slot is not acquired so no release is needed. The drain therefore correctly accounts for all and only started tasks — confirming dispatcher.Run only returns after every HandleResponse (and therefore every ISnapTree.Dispose) has completed.

FlatSnapStateTree.Dispose disposes _writeBatch (line 77) before control returns to SnapProviderHelper, which returns before HandleResponse returns, which returns before the semaphore is released, which happens before the drain unblocks dispatcher.Run. So persistence.Flush() in FinalizeSync is always called after every write batch is closed — no partially-open batches can exist.

DI: the new ISnapTrieFactory constructor parameter in SnapSyncRunner is already registered as a singleton in both Synchronizer.cs (via PatriciaSnapTrieFactory) and FlatWorldStateModule.cs (via FlatSnapTrieFactory), so no DI wiring changes are needed.

The [Explicit] stress reproducer won't run in CI unless manually triggered — correct for a load test. The _ = iteration; discard keeps the parameter in the NUnit test name (e.g. SnapSync_StressRepro(5)) while suppressing the unused-parameter warning; a comment explains the intent.

[LukaszRozmej]

Both Low findings addressed in 0a94bfd:

• Lifecycle ordering test: added SnapSyncRunnerTests with 3 cases (happy path, dispatcher throws, dispatcher cancelled) — all assert EnsureInitialize → dispatcher → FinalizeSync ordering. Required exposing an internal Func<CancellationToken, Task> ctor on SnapSyncRunner so the runner can be exercised without standing up a real SimpleDispatcher. InternalsVisibleTo("Nethermind.Synchronization.Test") already in scope.
• FinalizeSync on cancel/throw: kept the always-flush behaviour (idempotent, durability-safer default as you noted) and added a one-line comment in the finally block making the intent explicit. Cancel and throw paths are both covered by the new tests.

Also trimmed the multi-line XML doc comments on the new ISnapTrieFactory hooks to single-line comments per the repo's coding-style rule.