Skip to content

Stream snapshot download without storing the archive - #12862

Draft
svlachakis wants to merge 12 commits into
masterfrom
feature/snapshot-streaming-download
Draft

Stream snapshot download without storing the archive#12862
svlachakis wants to merge 12 commits into
masterfrom
feature/snapshot-streaming-download

Conversation

@svlachakis

@svlachakis svlachakis commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Changes

  • Adds Snapshot.Streaming (default false): the snapshot is downloaded and extracted in a single pass, without ever storing the archive on disk. Peak disk usage drops from archive plus extracted database to the extracted database alone (for the mainnet archive snapshot: ~2.4TB instead of ~4.4TB).
  • Snapshot.StreamingConnections (default 4) parallel connections fetch 64 MiB ranges and deliver them in order into a live zstd/gzip-to-tar extractor. Memory is bounded by the connection window; nothing is buffered to disk.
  • Interrupted connections resume with Range/If-Range from the exact byte, including partial resume inside a chunk. A 200 response to a range request is handled by skipping the already-delivered prefix instead of feeding duplicate bytes to the decompressor; a changed ETag or content length aborts and restarts the download cleanly (up to 3 attempts). Servers without range support fall back to a single connection that resumes by re-reading and skipping.
  • SHA-256 is computed incrementally on the stream and verified against Snapshot.Checksum after extraction. On a checksum mismatch or a corrupt archive the extracted database is deleted and the node continues running, matching the two-phase behavior.
  • Disk space is checked before streaming starts (1.5x the archive size), zip archives are rejected upfront (they cannot be extracted from a stream), and a stale archive left by a previous two-phase run is deleted since streaming never uses it.
  • The redirect-following HTTP request logic, skip helper, permanent-error classification, and the disk-space/checksum verification are shared between the two-phase and streaming paths.

Contains #12861

Types of changes

What types of changes does your code introduce?

  • New feature (a non-breaking change that adds functionality)

Testing

Requires testing

  • Yes

If yes, did you write tests?

  • Yes

Notes on testing

23 tests against a local HTTP server with injected faults: chunked and sequential delivery with exact byte and hash verification, every range dropped mid-response once, servers that ignore range requests (resume by skipping), source rotation detected via ETag and via content length when no ETag is present, permanent HTTP errors aborting instead of retrying, corrupt archives deleting the partial database without failing node startup, checksum mismatch, insufficient disk space, zip rejection, and end-to-end runs through InitDatabaseSnapshot with Streaming=true.

Documentation

Requires documentation update

  • Yes
  • No

Requires explanation in Release Notes

  • Yes

Snapshot.Streaming=true streams the snapshot directly into the database directory, removing the need to provision disk space for the archive on top of the extracted database.

The 2.5x guard ran at the extraction stage, after the tarball
was already on disk, so passing it effectively required ~3.5x
the snapshot in volume space. Probe the snapshot size upfront
and demand the remaining download bytes plus a 1.5x extraction
estimate before downloading; at extraction demand only the
1.5x estimate. The pre-check runs outside the retry loop so
low disk aborts instead of retrying forever.
@claude

claude Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Claude finished @svlachakis's task in 12m 46s —— View job


Review: streaming snapshot download

  • Gather context (diff vs origin/master, repo rules)
  • Review SnapshotHttpStream (chunk ordering, resume, concurrency, hashing)
  • Review StreamingSnapshotInitializer / InitDatabaseSnapshot wiring
  • Review SnapshotHttpClient, SnapshotExtractor, config
  • Review tests and CI wiring
  • Post findings (9 inline comments)

The core streaming machinery holds up. I worked through the producer/consumer protocol specifically looking for ordering and buffer-lifetime bugs and did not find one: chunk indices are handed out monotonically, Connections + 1 permits guarantee the chunk at _consumeIndex is always in flight or delivered (so the in-order consumer can't deadlock), a permit maps 1:1 to a 64 MiB buffer released only after full consumption, the sentinel is delivered exactly once, buffers are never handed to a producer while the consumer still holds them, and the incremental hash covers every byte exactly once across retries and the rangeless skip-to-resume path. EnsureSourceUnchanged runs before the status check on every response, so a rotated object can't be spliced in. SnapshotSourceChangedException deriving from IOException while being excluded from every retry/swallow filter is delicate but correct at each of the four sites. Nice work on the fault propagation: the GetOrAddPending fault re-check closes the lost-wakeup race against Fail.

Findings below are mostly about behaviour at the edges — what happens when the network stalls, when the source is hostile, and when the operator mis-sets a knob.

High

1. The 23 new tests never run in CI. Nethermind.Init.Snapshot.Test is added to Nethermind.slnx but not to the project matrix in .github/workflows/nethermind-tests.yml. Per .agents/rules/github-workflows.md, "the project list in nethermind-tests.yml is the source of truth" — without an entry this whole suite, including the entire test story for the base PR, is dead weight. It needs one line after Nethermind.History.Test. I can't modify .github/workflows/ myself (GitHub App permissions), so this one is yours.

Medium

2. No stall detection; timeouts aren't retryable. SnapshotHttpClient leaves HttpClient.Timeout at its default while using ResponseHeadersRead. Either a half-open connection hangs the synchronous Read (and node startup) indefinitely with no log output and nothing on disk to resume from, or the 100 s default fires and the resulting TaskCanceledException misses the IOException or HttpRequestException retry filters and hard-aborts the download. Details

3. DeleteStaleArchive can discard a complete, verified download. Deletion is unconditional, including at checkpoint Downloaded/Verified — flipping Streaming=true throws away a finished ~2 TB archive for no disk saving. Details

4. Verify-then-extract is inverted, and the extractor doesn't validate links. Streaming necessarily writes tar entries before the checksum is known, and ExtractTarEntries only does a lexical containment check before calling entry.ExtractToFile on any non-directory entry — so a symlink entry plus a follow-up write escapes dbPath before the mismatch is ever detected. The gap pre-exists; the base branch had the checksum in front of it. Details

5. StreamingConnections is unbounded. Peak buffers are (StreamingConnections + 1) × 64 MiB, so 64 means ~4 GB at startup; only < 1 is rejected, and the per-connection memory cost isn't in the ConfigItem description. Details

6. A thrice-rotated source aborts node startup and leaves the third attempt's partial DB behind, while corrupt archives and checksum mismatches are deliberately downgraded to "delete and keep running". Details

Low

7. A 200 to a range request is only prefix-skipped in the sequential path, not the parallel one — contrary to the PR description — and it eventually surfaces as "corrupt archive". Details

8. The decompressor is chosen from SnapshotFileName, which streaming never writes; the default snapshot.zip makes the flag fail hard at startup. Details

9. When the server reports no length, truncation is undetected and Checksum is the only remaining guard — but it's optional. Details

10. The Connections + 1 liveness invariant is load-bearing and uncommented; no test would catch a future tweak. Details

11. StartListener is duplicated verbatim across the two test servers. Details

12. Two smaller items I didn't file inline: Drain ignores its CancellationToken (it only gates Task.Run), so FinishAsync can't be interrupted once started; and ReadAsync isn't overridden, so an async consumer silently gets Stream's thread-blocking fallback — worth a <remarks> noting the stream is sync-only by design. Dispose(bool) blocking on Task.WhenAll(_producers) is the standard sync bridge, but it can rethrow a producer fault and mask the original extraction exception.

Notes

I could not build or run the test suite in this environment (dotnet execution isn't permitted here), so the findings come from reading the code — nothing above is a reproduced failure. I also couldn't empirically settle whether HttpClient.Timeout covers body reads under ResponseHeadersRead on this runtime, which is why finding 2 is framed to cover both outcomes; a five-minute local experiment would tell you which branch of it applies.
• branch feature/snapshot-streaming-download

Comment thread src/Nethermind/Nethermind.Init.Snapshot/SnapshotHttpClient.cs Outdated
Comment thread src/Nethermind/Nethermind.Init.Snapshot/SnapshotExtractor.cs
Comment thread src/Nethermind/Nethermind.Init.Snapshot/ISnapshotConfig.cs
Comment thread src/Nethermind/Nethermind.Init.Snapshot/StreamingSnapshotInitializer.cs Outdated
Comment thread src/Nethermind/Nethermind.Init.Snapshot/SnapshotHttpStream.cs
Comment thread src/Nethermind/Nethermind.Init.Snapshot/SnapshotHttpStream.cs
Comment thread src/Nethermind/Nethermind.Init.Snapshot/SnapshotHttpStream.cs
Comment thread src/Nethermind/Nethermind.Init.Snapshot.Test/InitDatabaseSnapshotTests.cs Outdated
Stall timeout on body reads with retry, reject tar link entries,
keep a fully downloaded archive and extract it two-phase, bound
StreamingConnections to 16, require a checksum when the size is
unknown, graceful stop after repeated source changes
@svlachakis

Copy link
Copy Markdown
Contributor Author

@claude review again, I adapted the comments that were valid.

@claude

claude Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

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


Re-review: streaming snapshot download

  • Fetch branch state and diff 91f6604 against the previous review
  • Verify each prior finding
  • Fresh pass over the hardened code for new defects
  • Post findings (4 inline comments)

Reviewed 91f6604 ("harden snapshot streaming per review"). Six of the eleven prior findings are properly closed, and the fixes are the right shape rather than minimal patches — the MaxSourceChangedRestarts loop now reads as a bounded loop with the give-up path after it, the two-phase fallback for a completed archive is gated on checkpoint.Read() >= Downloaded && File.Exists(...) rather than on a flag, and the tar link rejection came with a symlink-escape regression test. I re-derived the producer/consumer invariants after the IsRetryable change and they still hold: OperationCanceledException is now retryable only while _cts is un-cancelled, so shutdown and Task.Delay cancellation still fall through to Fail, and the Fail/GetOrAddPending ordering (write _fault → cancel _cts → drain _pending, versus GetOrAdd → re-read _fault) closes both interleavings. No new correctness bug found in the hardening commit.

Prior findings — status

# Finding Status
1 Test project missing from CI Open — see below
2 No stall detection; timeouts not retryable Partial — stall timeout added; HttpClient.Timeout still default
3 DeleteStaleArchive discards a complete download Fixed (InitDatabaseSnapshot.cs:86-99, with a test)
4 Verify-then-extract inverted; no link validation Fixed (SnapshotExtractor.cs:106-111, with a test)
5 StreamingConnections unbounded Fixed (capped at 16, memory cost documented, parameterized test)
6 Thrice-rotated source aborts startup Fixed (bounded loop, DeleteDatabase() on every attempt)
7 200 not prefix-skipped in the parallel path Open (Low, unchanged)
8 Format derived from SnapshotFileName Partial — message now names the config key
9 Silent truncation when length unknown Fixed (refuses to start without a checksum, with a test)
10 Connections + 1 invariant uncommented Open (Low, unchanged)
11 StartListener duplicated Fixed (TestHttpListener)

High

1. The test suite still never runs in CI. Nethermind.Init.Snapshot.Test is in Nethermind.slnx but absent from the project matrix in .github/workflows/nethermind-tests.yml (I checked the current list — it goes Nethermind.History.TestNethermind.Hive.Test). Per .agents/rules/github-workflows.md that matrix is the source of truth, so all 25 tests here — including the new symlink-rejection and connection-bound regressions — are dead weight on every PR. One line, alphabetically after Nethermind.History.Test:

          - Nethermind.Init.Snapshot.Test

I'm unable to modify files under .github/workflows/ — the GitHub App token has no workflows permission (FAQ). This one needs a human commit.

Medium

2. HttpClient.Timeout is still the 100 s default. The new per-read stall timeout closes the "hang forever" failure mode, but not the opposite bound: on .NET the client timeout is a deadline for the whole request including the body under ResponseHeadersRead. In the parallel path that caps a 64 MiB chunk at 100 s (~5.4 Mbit/s per connection) and reports the teardown as No data received for 120s; in the rangeless path it can livelock — each retry re-reads from 0 and skips, so once the skip exceeds one timeout's worth of transfer, no attempt ever finishes. Timeout = Timeout.InfiniteTimeSpan is now unambiguously safe given StallTimeout supplies the liveness guarantee. Details · Fix this →

3. Streaming silently loses all progress across a node restart. The checkpoint is never advanced below Completed and no archive is kept, so a restart mid-download hits InitDatabaseSnapshot.cs:69, deletes dbPath, and re-streams from byte 0 — whereas the two-phase path resumes the partial archive with Range. On the mainnet archive snapshot that is a multi-day transfer lost to an ordinary systemctl restart. The trade-off is defensible; the problem is that the config description currently says "Interrupted connections are resumed automatically", which reads as exactly the opposite. It should be stated here and in the release note. Details

Low

4. ReadWithStallTimeoutAsync allocates a linked CTS + timer per socket read — tens of millions over a full snapshot. TryReset() on a per-chunk source gets the same semantics. Details

5. The two behaviours the hardening commit introduced at the initializer level — exhausted source-change restarts returning gracefully, and the stall timeout being retried rather than fatal — are the two without tests. FlakySnapshotServer and the injectable StallTimeout already make both cheap to write. Details

6. Carried over unchanged, all still minor: the parallel path still doesn't prefix-skip a 200 (contrary to the PR description) and surfaces it as "corrupt archive"; the load-bearing Connections + 1 window invariant is still uncommented; Drain still ignores its CancellationToken; ReadAsync is still not overridden, so an async consumer gets Stream's blocking fallback; EnsureStreamableArchive still accepts a bare .gz/.zst that isn't a tar.

7. Neither path caps total retry time — an unreachable host retries forever at a 300 s ceiling and node startup never completes. Pre-existing in DownloadWithRetryAsync, inherited by FetchChunkAsync; worth an overall deadline eventually, not in this PR.

Notes

dotnet execution isn't permitted in this environment, so as before nothing here is a reproduced failure — the findings come from reading the code, and I flagged in the inline comment which part of finding 2 rests on ResponseHeadersRead semantics I couldn't test locally. The recommended change there is correct under either reading.

Nothing blocks merge on correctness grounds. Finding 1 is mechanical and finding 3 is a docs change; finding 2 is a one-line hardening I'd take before this ships to operators on slow links.

• branch feature/snapshot-streaming-download

Comment thread src/Nethermind/Nethermind.Init.Snapshot/SnapshotHttpClient.cs Outdated
Comment thread src/Nethermind/Nethermind.Init.Snapshot/ISnapshotConfig.cs Outdated
Comment thread src/Nethermind/Nethermind.Init.Snapshot/SnapshotHttpStream.cs Outdated
Explicit infinite HttpClient timeout with a 100s header guard so
body reads are governed only by the stall timeout, one stall CTS
per response instead of per read, document that a node restart
restarts the download, cover exhausted restarts and stalls
@svlachakis

Copy link
Copy Markdown
Contributor Author

@claude review again

@claude

claude Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Claude finished @svlachakis's task in 7m 35s —— View job


Re-review: streaming snapshot download

  • Diff 7dcf349 + f4aa509 against the previously reviewed 91f6604
  • Verify each open prior finding
  • Fresh pass over the hardened code
  • Post findings (3 inline comments)

Reviewed f4aa509. The two round-two commits close the CI gap and three of the four remaining findings, and the HttpClient change is better than what I suggested: rather than just setting Timeout.InfiniteTimeSpan, it pairs that with an explicit HeaderTimeout around SendAsync, converted to HttpRequestException so a header-phase hang lands in the existing retry filters instead of escaping as TaskCanceledException. The catch filter correctly excludes caller-initiated cancellation. I re-derived the reused-stallCts lifetime across both paths — created and disposed together with content in the sequential path, scoped per response in the parallel path, disposed on every exit including the non-retryable one — and it is sound; the only gap is the disarm race noted below.

Two new tests are the right ones: Read_ServerStallsOnce_DetectsStallAndResumes pins down that a stall is retried rather than fatal and that the pre-stall bytes are hashed exactly once, and InitializeAsync_SourceKeepsChanging_GivesUpWithoutThrowing pins the terminal branch that used to abort startup. The HangOnceAfterBytes hook can't be tripped by the probe (length > hangAfter excludes the 1-byte probe) and the accept loop dispatches via Task.Run, so the 30 s hold doesn't block sibling requests — no flakiness there that I can see.

Prior findings — status

# Finding Status
1 Test project missing from CI Fixed (7dcf349)
2 No stall detection; timeouts not retryable Fixed for streaming — see new Medium below for the two-phase path
3 Streaming loses progress across a restart, docs said otherwise Fixed (ISnapshotConfig.cs:28)
4 Per-read CTS allocation Fixed (one CTS per chunk/connection) — small race, below
5 No test for exhausted restarts / stall timeout Fixed (both added)
6 Carried-over Low items Open, unchanged
7 No overall retry deadline Open (pre-existing, out of scope)

Medium

1. The two-phase downloader shares this client and has no stall timeout of its own. SnapshotDownloader.CopyWithProgressAsync (SnapshotDownloader.cs:119) and SnapshotHttpClient.SkipAsync (line 104) read with only the caller's token — no per-read budget anywhere. A half-open connection blocks forever: no exception for DownloadWithRetryAsync to catch, no progress line, node startup hangs silently. Streaming=false is the default, so this is the path most operators run. Whether Timeout.InfiniteTimeSpan created this exposure or merely made a pre-existing one explicit still hinges on ResponseHeadersRead semantics I can't test here — but the fix is the same either way, and it's now a copy of machinery that already exists next door. Details · Fix this →

2. ProbeAsync is a single unretried request that gates node startup. Everything else in the streaming path retries or degrades gracefully — chunk fetches, sequential reads, source rotation, corrupt archives, checksum mismatches. StreamingSnapshotInitializer.cs:49 does not: one DNS blip, connection reset, or 503 and the exception escapes InitializeAsyncExecute → the node won't boot. The two-phase equivalent (CheckDiskSpaceBeforeDownloadAsync) catches and downgrades to a warning. Details

Low

3. Disarming the reused stall source with CancelAfter(Timeout.InfiniteTimeSpan) can't recall an already-queued timer callback, so a read that completes in the last tick of its budget leaves the source cancelled and the next read fails instantly as No data received for Ns. Converges, but on a rangeless server the retry re-reads and re-skips the whole prefix. Details

4. The rangeless path still sizes _window at Connections + 1 (SnapshotHttpStream.cs:65) even though it runs one producer, so StreamingConnections=16 against a server without range support buffers ~1.1 GiB behind a single connection. The total bound is unchanged and correct; it's the config text — "Each connection buffers up to 64 MiB" — that reads as if it scaled down with the connection count.

5. Carried over unchanged, all still minor: the parallel path doesn't prefix-skip a 200 (contrary to the PR description) and surfaces it as "corrupt archive"; the load-bearing Connections + 1 window invariant is still uncommented; Drain still ignores its CancellationToken (it only gates Task.Run, so FinishAsync can't be interrupted once started); ReadAsync is still not overridden, so an async consumer silently gets Stream's blocking fallback; EnsureStreamableArchive still accepts a bare .gz/.zst that isn't a tar.

Notes

dotnet isn't executable in this environment, so as in the previous rounds nothing here is a reproduced failure — the findings come from reading the code, and finding 1 explicitly flags which part of it rests on runtime semantics I couldn't verify locally. I did not re-derive the producer/consumer invariants from scratch this round; they're unchanged from 91f6604, which I verified.

Nothing here blocks merge on correctness. Finding 1 is the one I'd take before this ships, and it's a fix to the default path rather than to the new feature.
• branch feature/snapshot-streaming-download

Comment thread src/Nethermind/Nethermind.Init.Snapshot/SnapshotHttpClient.cs
Comment thread src/Nethermind/Nethermind.Init.Snapshot/StreamingSnapshotInitializer.cs Outdated
Comment thread src/Nethermind/Nethermind.Init.Snapshot/SnapshotHttpStream.cs Outdated
Shared StallGuardedReader (TryReset, no per-read CTS, no disarm
race) used by streaming, skip, and the two-phase downloader; probe
retried with backoff instead of failing startup on one blip
@svlachakis

Copy link
Copy Markdown
Contributor Author

@claude review again.

@claude

claude Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

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


Re-review: streaming snapshot download

  • Diff a5000a9 + 06f8ac4 against the previously reviewed f4aa509
  • Verify each open prior finding
  • Fresh pass over the changed code
  • Post findings (2 inline comments)

Reviewed a5000a9. All three open Medium findings are closed, and the fix that closes two of them at once — extracting StallGuardedReader — is a better answer than the two separate patches I suggested. It gives the two-phase path (CopyWithProgressAsync, SkipAsync) the same per-read budget the streaming path had, and it collapses the per-read CancellationTokenSource churn into one source per chunk/connection via TryReset(). Both paths are now bounded end to end: HeaderTimeout around SendAsync, StallTimeout around every body read.

I chased the one thing I'd flagged as a possible regression — whether TryReset() on a linked source drops the link and stops shutdown propagating. It doesn't: CreateLinkedTokenSource registers its cancel callback on the parent token and keeps the handle in the child (Linked1CancellationTokenSource._reg1), so TryReset only touches the child's own registration list. Recreate() disposes the old source, which disposes that parent registration, so nothing accumulates on _cts across stalls. The important correctness property also holds: when TryReset() fails after a successful read, the bytes are still returned before the source is recreated, so nothing is dropped from the hash.

One design detail I liked on re-reading: CancelAfter is armed only around the socket read, so backpressure (_window.WaitAsync) and disk writes (destination.WriteAsync) fall outside the budget. A slow consumer or a slow disk can't be misread as a dead connection. That's load-bearing and undocumented — see the inline note.

ProbeWithRetryAsync is the right shape too: IsPermanentHttpError still aborts (so a typo'd URL fails fast rather than looping), transient errors back off, and InitializeAsync_ProbeFailsTransientlyOnce_RetriesAndCompletes pins it with a deterministic 500 on request #1 — the probe is provably the first request, so no ordering flake.

Prior findings — status

# Finding Status
1 Two-phase path had no stall timeout Fixed (StallGuardedReader in CopyWithProgressAsync + SkipAsync; IOException lands in the existing DownloadWithRetryAsync filter)
2 ProbeAsync unretried, gated node startup Fixed (ProbeWithRetryAsync, with a test)
3 Stall-source disarm race Fixed (TryReset() + Recreate()); residual one-tick window now self-heals
4 Rangeless path buffers Connections + 1 chunks Open (Low) — config text still reads as if it scaled with connection count
5 Carried-over Low items Open, unchanged
6 No overall retry deadline Open (pre-existing) — now consistent, see below

Also confirmed from earlier rounds: HttpRequestException chains the original OperationCanceledException as inner, and SkipAsync's hand-rolled loop is equivalent to the ReadAtLeastAsync(throwOnEndOfStream: true) it replaced (EndOfStreamException derives from IOException, so it stays retryable in both paths).

Low

1. StallGuardedReader carries three contracts and documents none: the IOException type is what puts a stall inside every caller's retry filter; the mutable _stallCts makes one-instance-per-flow load-bearing; and arming only around the read is what keeps backpressure and disk latency out of the budget. Each is easy to break by accident. Details

2. The 2-minute stall budget now exists as two independent literals (SnapshotDownloader.cs:20 and SnapshotHttpStream.cs:18), only one of which is injectable. Details

3. Retry-forever is now uniform across probe, chunk fetch, sequential read, and the two-phase download — which is what I asked for, but it means an unreachable host makes node startup hang indefinitely (warning every ≤300 s) where the probe previously aborted it. That's the better default for a bootstrap-only step; still the strongest argument for the overall deadline in finding 6, and still out of scope here.

4. Carried over unchanged, all minor: the parallel path doesn't prefix-skip a 200 (contrary to the PR description) and surfaces it as "corrupt archive"; the Connections + 1 window invariant is still uncommented; Drain still ignores its CancellationToken; ReadAsync is still not overridden, so an async consumer gets Stream's blocking fallback; EnsureStreamableArchive still accepts a bare .gz/.zst that isn't a tar.

5. Read_ServerStallsOnce_DetectsStallAndResumes runs a 250 ms stall budget, so on a thread-pool-starved CI runner a healthy localhost read can trip it. That costs an extra retry, not a failure — the test asserts bytes and hash, both of which survive a retry, and it asserts no request count. Worth knowing if it ever shows up as slow rather than red.

Notes

dotnet still isn't executable in this environment, so as in every prior round nothing here is a reproduced failure — the findings come from reading the code. The TryReset-on-a-linked-source conclusion above rests on the linked-source implementation rather than an experiment; if you want it nailed down, a two-line test asserting that cancelling the parent still cancels the reader after a successful TryReset would settle it permanently and guard the property.

No Critical, High, or Medium findings remain. From my side this is ready.
• branch feature/snapshot-streaming-download

Comment thread src/Nethermind/Nethermind.Init.Snapshot/StallGuardedReader.cs
Comment thread src/Nethermind/Nethermind.Init.Snapshot/SnapshotDownloader.cs Outdated
An extraction that writes zero files (StripComponents mismatch)
now fails instead of completing, a completed checkpoint with a
missing or empty database directory reinitializes instead of
skipping, and a mounted-but-empty db directory (lost+found only)
no longer counts as an existing database
@svlachakis

Copy link
Copy Markdown
Contributor Author

@claude review again

@claude

claude Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Claude finished @svlachakis's task in 5m 14s —— View job


Re-review: streaming snapshot download

  • Diff 2d339d3 + 5f5c887 + d6c6497 against the previously reviewed a5000a9
  • Verify open prior findings
  • Fresh pass over the changed code
  • Post findings (3 inline comments)

Reviewed d6c6497. This round is small and mostly closes gaps I hadn't found: the stall-timeout literal now has a single source (SnapshotHttpClient.DefaultStallTimeout), and 5f5c887 goes after a class of bug I'd walked past three times — the snapshot step completing while leaving no database behind. Both new guards are real:

  • DatabaseExists replacing Path.Exists closes the case where a Completed checkpoint plus a missing or emptied database made the step return "already exists, skipping" and left the node to sync from genesis. SnapshotCheckpoint.Advance writes unconditionally rather than monotonically, so the rewind to Started actually takes effect — I checked, because a monotonic Advance would have made the whole branch a silent no-op.
  • The extractedFiles == 0 check in ExtractTarEntries closes the strip-components mismatch, which previously walked every entry, wrote nothing, and advanced to Completed.

I also re-derived the d6c6497 disposal change. Clearing _buffers / _current / _pending after Task.WhenAll(_producers) is safe — both producer bodies funnel every exception into Fail, so WhenAll cannot fault and skip the cleanup, and no consumer can be mid-Read at that point. It matters most on the SnapshotSourceChangedException restart path, where the previous attempt's ~320 MiB of 64 MiB LOH buffers would otherwise stay rooted through the next attempt. A read after dispose now surfaces as OperationCanceledException rather than an NRE (GetOrAddPending only touches IsCancellationRequested, which is legal post-dispose); nothing calls it, so that's an observation, not a finding.

One thing I verified rather than assumed: rewinding the checkpoint to Started with a complete archive still on disk does not break the two-phase resume — DownloadAsync re-requests Range: existingSize- and SnapshotDownloader.cs:56 handles the resulting 416 by returning cleanly. It costs a full re-hash, not a re-download. The streaming variant of that same state is a different story — first finding below.

Prior findings — status

# Finding Status
1 StallGuardedReader contracts undocumented Open (Low, unchanged)
2 Stall timeout duplicated as two literals Fixed (2d339d3) — still no note on why two minutes
3 No overall retry deadline Open (pre-existing, out of scope)
4 Carried-over Low items Open, unchanged

Low

1. The new checkpoint rewind can discard a complete archive under Streaming=true. The branch fires at checkpoint >= Extracted, and at exactly Extracted the archive is still on disk — it's deleted one line before Completed. Rewinding to Started makes InitDatabaseSnapshot.cs:93 match, which hands control to StreamingSnapshotInitializer, whose first act is DeleteStaleArchive(). That undoes the round-two guard for this one state. Rewinding to Downloaded when File.Exists(snapshotPath) keeps the archive and still re-verifies. Details

2. The emptiness guard is tar-only, and zip is now worse than before. ExtractZip has no equivalent, so a degenerate zip that extracts nothing used to advance to Completed and short-circuit on every later start — idempotent, if silent. With the rewind in place it re-downloads and re-extracts the entire snapshot on every restart, forever. A shared post-extraction check covers both formats, and also the dbPath/lost+found case where the tar counter and DatabaseExists disagree. Details

3. lost+found is load-bearing, unexplained, and the new predicate can throw. It quietly fixes a pre-existing trap (a dedicated ext4 mount looked like a database, sending an incomplete run into Directory.Delete on the mount point), which is exactly why it deserves a line saying so. Separately, EnumerateFileSystemEntries throws on an unreadable directory where Path.Exists returned false. Details

4. Carried over unchanged, all minor: StallGuardedReader's three implicit contracts (the IOException type is what keeps stalls inside every caller's retry filter; _stallCts makes one-instance-per-flow load-bearing; arming only around the read is what keeps backpressure and disk latency out of the budget); the two-minute value now has one home but no rationale; the parallel path doesn't prefix-skip a 200 (contrary to the PR description); the Connections + 1 window invariant is uncommented; Drain ignores its CancellationToken; ReadAsync isn't overridden, so an async consumer gets Stream's blocking fallback; EnsureStreamableArchive accepts a bare .gz/.zst that isn't a tar.

5. Test coverage for 5f5c887 is asymmetric: the streaming half of the empty-extraction guard has a test, the two-phase half — the one that aborts node startup — doesn't.

Notes

dotnet still isn't executable in this environment, so as in every prior round nothing here is a reproduced failure; the findings come from reading the code. Findings 1 and 2 both hinge on state machines I traced by hand (SnapshotStage transitions against what's on disk) rather than ran — the reachability arguments are spelled out inline so you can check my work.

No Critical, High, or Medium findings. All three new items are narrow edges of an otherwise good hardening commit.

• branch feature/snapshot-streaming-download

Comment thread src/Nethermind/Nethermind.Init.Snapshot/InitDatabaseSnapshot.cs Outdated
Comment thread src/Nethermind/Nethermind.Init.Snapshot/SnapshotExtractor.cs Outdated
Comment thread src/Nethermind/Nethermind.Init.Snapshot/InitDatabaseSnapshot.cs Outdated
Checkpoint rewind keeps a complete archive (Downloaded, not
Started), the empty-extraction guard covers zip and tar in one
place using the database-exists predicate, and DatabaseExists
tolerates unreadable directories
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant