Skip to content

db/downloader: bound kept-snapshot seeding and rebuild malformed metainfo - #23459

Draft
AskAlexSharov wants to merge 7 commits into
mainfrom
alex/downloader_seed_kept_followups_37
Draft

db/downloader: bound kept-snapshot seeding and rebuild malformed metainfo#23459
AskAlexSharov wants to merge 7 commits into
mainfrom
alex/downloader_seed_kept_followups_37

Conversation

@AskAlexSharov

@AskAlexSharov AskAlexSharov commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Follow-up to the review on #23446; lands on main since that cherry-pick is
faithful. The seeding bound and cancellation went to #23472, the
AddNewSeedableFile upload fix to #23653. What is left:

problem: on a datadir with snapshot data but no .torrent files (rsync'd or
restored), BuildTorrentIfNeed only checks that the path exists. A corrupt
.torrent suppressed the derivation the kept data would supply, and the snapshot
stayed out of torrentsByName — hence out of allActiveSnapshots and
PublishLocalChainToml — for the life of the process.

Solution: remove and rebuild confirmed-malformed metainfo; keep a file that merely
could not be read. TestKeptLocalSnapshotWithUnreadableMetainfoIsSeeded verified red.

Also: three README/docstring statements that were the opposite of the code.

…info

Three fixes on the kept-local path, all reachable on a datadir that has snapshot
data but no .torrent files (rsync'd or restored), where every preverified item
takes the kept-local branch.

Unbounded fan-out: addDownload spawned one goroutine per item, each running
BuildFromFilePath, which SHA-1s a whole data file with a 2 MiB piece buffer.
Thousands of those at once exhaust descriptors and thrash the disk. Bound it
like BuildTorrentFilesIfNeed, and let the wait observe the downloader shutting
down so a stop drains queued tasks instead of hashing every file first.

Note the bound is scoped to d.ctx, not the batch: wait()'s defer runs abandon()
on every path including success, and abandon cancels unconditionally, so a
batch-scoped wait would drop seeding for a download that succeeded.

Malformed metainfo: BuildTorrentIfNeed only checks that the .torrent path
exists, so a corrupt one suppressed the derivation the kept data would supply.
addCompleteTorrent then failed on the same bytes and the snapshot stayed out of
torrentsByName -- absent from allActiveSnapshots and PublishLocalChainToml -- for
the life of the process. Remove metainfo confirmed malformed so it is rebuilt
from the data, keeping it when the file merely could not be read.

Also: README described the complete path as dropping a mismatched .torrent and
as never deriving one, both the opposite of what the code does; the
maybeLoadMetainfoFromDisk docstring still promised a removal dropped in #18056;
and the name assertions in the local-snapshot tests could not fail, since an
earlier warn had already put the name in the buffer.
@AskAlexSharov

Copy link
Copy Markdown
Collaborator Author

Overlap check against #23472, which was opened for the same defect.

#23472 bounds the fan-out with a downloader-wide semaphore.Weighted, which is stronger than the per-batch seedSlots channel here — N concurrent batches get N x the limit with mine. It also fixes something this PR does not: the uninterruptible wait. It gives seeding its own seedCtx cancelled from the caller's context via context.AfterFunc, and makes wait() surface context.Cause(ctx) for an all-kept-local batch, which never samples ctx because me.torrents is empty.

What is only here: removeMalformedMetainfo (so BuildTorrentIfNeed's bare path-exists check stops suppressing the derivation the kept data would supply), maybeLoadMetainfoFromDisk's docstring, the seedKeptSnapshot re-entrancy comment, and the two README claims that contradict the code.

Suggested split: take the bound and the cancellation from #23472, and reduce this PR to the metainfo repair plus the docs. #23446 is blocked on both landing.

@AskAlexSharov

Copy link
Copy Markdown
Collaborator Author

Reviewed #23446 and QA'd it on n0; three of the findings land here. Checked each against this branch at 47b7552, so two of them are already yours — noting them only to say they're confirmed fixed.

Already covered here. removeMalformedMetainfo fixes the corrupt-.torrent case (kept snapshot never seeded, never repaired, for the life of the process). seedSlots bounds the fan-out. For a number on that one: I ran downloader --seedbox on n0 against a real mainnet preverified.toml materialised as 14,432 kept files and saw a peak of 13,620 goroutines, one per kept file, each hashing a whole data file. With 64KB files that finished in seconds; at real snapshot sizes it would not. The bound earns its place.

Still live

1. Kept data with no local .torrent gets no validation at all, then is seeded as authoritative.

localMetainfoUnbacked is only computed inside the if localMetainfo.Ok branch. With preverified.toml present, the data file present, and no .torrent on disk, prepareLocalDataForDownload falls straight through to keeping local snapshot, skipping preverified download with no size check and no content check. seedKeptSnapshot then derives a .torrent from those bytes, so addCompleteTorrentFromMetainfo always agrees the file is complete, and the node advertises it to peers while erigon opens it. A truncated or half-copied file is adopted as canonical and there is no later path that catches it.

That is exactly the datadir this PR names in its own description — snapshot data with no .torrent files, rsync'd or restored. Before #23350 invalidateData moved such a file aside and the correct one was re-fetched.

I don't think there's a free fix: with no local .torrent and no size in the preverified TOML, there is nothing local to validate against, so a real check means fetching the preverified metainfo from webseeds for kept files. Worth deciding explicitly rather than leaving it implicit — either do that fetch, or say in the README that a bare kept data-file is trusted unverified.

2. abandon() still awaits the seeding, so Download blocks on full hashing with logging already dead.

logDownload runs on batch.ctx and is registered on batch.all. wait() does defer abandon(); abandon() cancels batch.ctx — killing logDownload — and only then calls me.all.Wait(), which includes the seeding goroutines. So the gRPC Download call, and therefore erigon's snapshot stage, returns only after every kept file has been hashed, and emits nothing at all for that entire window. Bounding the concurrency reduced the peak but not the total, so the wall time is unchanged; it just looks hung more slowly. Correctly scoping the seeding to d.ctx is what makes this visible — the work now outlives the batch, but the batch still waits for it.

If seeding is meant to outlive the batch, it probably shouldn't be on me.all.

Question, not a defect

The README now says a stale-but-well-formed .torrent "is left on disk" and the kept snapshot is seeded "under whatever infohash its local .torrent carries" — accurate, since removeMalformedMetainfo only removes unparseable metainfo. That means PublishLocalChainToml advertises a hash that disagrees with the official manifest, and every later Download for that name hits the "already loaded with different infohash" branch permanently. Documented rather than fixed is a fine call; flagging it so it's a call and not an oversight.

AddNewSeedableFile stopped at addCompleteTorrent, so a kept snapshot kept
DisallowDataUpload and carried no trackers.
# Conflicts:
#	db/downloader/download-batch.go
#	db/downloader/downloader.go
#	db/downloader/downloader_test.go
@AskAlexSharov

Copy link
Copy Markdown
Collaborator Author

Rebased on main and reduced again: the AddNewSeedableFile upload fix and its test are dropped here, because #23653 now owns that one and does it better — it guards on isNew from addCompleteTorrent, which this branch did not.

What is left is the malformed-metainfo repair, the README/docstring corrections, and requireLoggedForName. 59 lines over main. Head is d3a6bc735b.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant