transfer/indexer: say which file blocks a download and what to do (#200) - #201
Merged
Conversation
This was referenced Aug 24, 2026
0g-peterzhb
force-pushed
the
name-file-in-existence-errors
branch
from
August 24, 2026 16:34
c206b6e to
4a5b714
Compare
checkFileExistence reported both outcomes without saying which file it meant, so what reached the user was "Failed to download file: Failed to check file existence: File already exists" with no path at all. For the caller's own destination that is merely terse. For a fragment temp file it misleads: fragments are downloaded to <root>.temp in the process working directory, and if one survives an attempt - the copy into the output failed, os.Remove failed, the process was killed mid-iteration - every later attempt reports "File already exists" for a file the caller never created. They inspect the output file they asked for, find nothing wrong, delete it, retry, and get the same error, with nothing indicating what is actually blocking them. Name the file in both errors, and at the fragment sites add what to do about it via a single shared FragmentLeftBehindError rather than six copies of the sentence. The hint does not repeat the path, since checkFileExistence now supplies it. It is deliberately fragment-specific: the same ErrFileAlreadyExists is reported for the caller's own destination, where "remove it" would be bad advice since they may want to keep the file they already have. Exported because indexer.Client runs the same fragment loops; the alternative was two unexported copies. ErrFileAlreadyExists stays wrapped rather than replaced, so the errors.Is checks that depend on it keep matching - download_dir relies on it in three places. Reusing the leftover instead was tried in #199 and dropped: checkFileExistence compares only the merkle root, never the size, and because the root covers zero-padded chunks a file short by under a chunk of trailing zeros has the same root - 16 bytes and 256 bytes of the same padded content produce identical roots. Telling the caller to remove it needs no such trust. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
0g-peterzhb
force-pushed
the
name-file-in-existence-errors
branch
from
August 24, 2026 19:27
4a5b714 to
206e5c7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #200. Supersedes #199, which took the other approach and was dropped.
The problem
checkFileExistencereported both outcomes anonymously, so the user saw:No path anywhere. These fire for two very different files.
For the caller's own destination it's merely terse — they passed
--file out.datand can guess.For a fragment temp file it misleads. Fragments download to
<root>.tempin the process working directory. If one survives an attempt — the copy into the output failed (a full disk, which I hit myself today),os.Removefailed, the process was killed mid-iteration — every later attempt reportsFile already existsfor a file the caller never created. So they inspect theout.datthey asked for, find nothing wrong with it, delete it, retry, and get the identical error, with nothing pointing at the real obstacle.Change
Name the file in both errors, and at the six fragment sites say what to do:
The hint deliberately does not repeat the path —
checkFileExistencesupplies it — and there's an assertion that it appears exactly once.The sentence lives in one place,
FragmentLeftBehindError, rather than being copied at each of the six fragment sites. It's exported becauseindexer.Clientruns the same loops; the alternative was two unexported copies. Sharing the constructor rather than a bare string constant means the wrapping can't drift either.The guidance is fragment-specific on purpose: for the caller's own destination, "remove it" would be bad advice, since they may well want to keep the file they already have.
ErrFileAlreadyExistsstays wrapped, not replaced, so theerrors.Ischecks that depend on it keep matching —download_dir.gorelies on that in three places. Asserted, not assumed.Why not reuse the leftover instead
That was #199, and I closed it. Reuse means trusting
checkFileExistenceas a completeness signal, and it compares only the Merkle root, never the size. Because the root covers zero-padded chunks, a file short by less than a chunk of trailing zeros has the same root:The fragment path never validates the assembled output, so a short temp would silently produce a short result. Telling the caller to remove the file requires no such trust.
Testing
TestCheckFileExistence_NamesTheFileerrors.Is(err, ErrFileAlreadyExists); a different hash stays a distinct, fatal errorTestCheckFileExistence_MissingFileIsNotAnError..._LeftoverFragmentTellsUserToRemoveIt0x1111.temp, says "remove it and retry", and mentions the path exactly onceThe last test asserts the literal words rather than referencing
FragmentLeftBehindError. A test that reads the same constant it verifies would pass for any message, including nonsense — the point is that the user sees actionable wording, so that belongs as a literal in the test.RED on main: "the message must say which file", twice.
go build,go vet,gofmtandgo test -count=1 ./...(14 packages) all pass.This change is