transfer/indexer: say which file blocks a download and what to do (#200)

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>

Author: 0g-peterzhb

indexer/client.go

@@ -414,6 +414,11 @@ func (c *Client) downloadPlainFragments(ctx context.Context, roots []string, fil
 			return err
 		}
 		err = downloader.Download(ctx, root, tempFile, withProof)
+		if errors.Is(err, transfer.ErrFileAlreadyExists) {
+			// A complete fragment file from an earlier attempt blocks this one. Say so and
+			// name it: the caller never created it and has no way to guess it is the obstacle.
+			return transfer.FragmentLeftBehindError(err)
+		}
 		if err != nil {
 			return errors.WithMessage(err, "Failed to download file")
 		}
@@ -460,6 +465,9 @@ func (c *Client) downloadEncryptedFragments(ctx context.Context, roots []string,
 		}
 		err = downloader.Download(ctx, root, tempFile, withProof)
 		if err != nil {
+			if errors.Is(err, transfer.ErrFileAlreadyExists) {
+				return transfer.FragmentLeftBehindError(err)
+			}
 			return errors.WithMessage(err, fmt.Sprintf("Failed to download fragment %d", i))
 		}
 

transfer/downloader.go

@@ -62,6 +62,21 @@ var createOutputFile = func(name string) (io.WriteCloser, error) {
 	return file, nil
 }
 
+// FragmentLeftBehindError reports that a fragment temp file from an earlier attempt is
+// blocking this download, and says what to do about it.
+//
+// The advice is specific to fragment temp files. checkFileExistence reports the same
+// ErrFileAlreadyExists for the caller's own destination, where "remove it" would be bad
+// advice - they may well want to keep the file they already have. It is exported because
+// indexer.Client runs the same fragment loops.
+//
+// err is wrapped rather than replaced, so it keeps carrying both ErrFileAlreadyExists
+// for errors.Is and the path that checkFileExistence added; the message deliberately
+// does not repeat that path.
+func FragmentLeftBehindError(err error) error {
+	return errors.WithMessage(err, "a previous attempt left this fragment file behind; remove it and retry")
+}
+
 type IDownloader interface {
 	Download(ctx context.Context, root, filename string, withProof bool) error
 	DownloadFragments(ctx context.Context, roots []string, filename string, withProof bool) error
@@ -132,6 +147,11 @@ func (downloader *Downloader) downloadPlainFragments(ctx context.Context, roots
 	for _, root := range roots {
 		tempFile := fmt.Sprintf("%v.temp", root)
 		err := downloader.Download(ctx, root, tempFile, withProof)
+		if errors.Is(err, ErrFileAlreadyExists) {
+			// A complete fragment file from an earlier attempt blocks this one. Say so and
+			// name it: the caller never created it and has no way to guess it is the obstacle.
+			return FragmentLeftBehindError(err)
+		}
 		if err != nil {
 			return errors.WithMessage(err, "Failed to download file")
 		}
@@ -173,6 +193,9 @@ func (downloader *Downloader) downloadEncryptedFragments(ctx context.Context, ro
 
 		// Download raw (without decryption)
 		if err := downloader.downloadAndValidate(ctx, root, tempFile, withProof); err != nil {
+			if errors.Is(err, ErrFileAlreadyExists) {
+				return FragmentLeftBehindError(err)
+			}
 			return errors.WithMessage(err, fmt.Sprintf("Failed to download fragment %d", i))
 		}
 
@@ -288,11 +311,16 @@ func checkFileExistence(filename string, hash common.Hash) error {
 		return errors.WithMessage(err, "Failed to create file merkle tree")
 	}
 
+	// Name the file. Both of these are reported for a fragment's temp file as well as for
+	// the caller's own destination, and without the path the message sends the reader to
+	// the wrong place: they see "File already exists", inspect the output file they asked
+	// for, find nothing wrong with it, and never learn the obstacle is a <root>.temp they
+	// did not create.
 	if tree.Root() == hash {
-		return ErrFileAlreadyExists
+		return errors.WithMessagef(ErrFileAlreadyExists, "%v", filename)
 	}
 
-	return errors.New("File already exists with different hash")
+	return errors.Errorf("File already exists with different hash: %v", filename)
 }
 
 func (downloader *Downloader) downloadFile(ctx context.Context, filename string, root common.Hash, info *node.FileInfo, withProof bool) error {

transfer/downloader_existence_test.go

@@ -0,0 +1,41 @@
+package transfer
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/0gfoundation/0g-storage-client/core"
+	"github.com/ethereum/go-ethereum/common"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// Both existence errors are reported for a fragment's temp file as well as for the
+// caller's own destination. Without the path, someone hitting the fragment case reads
+// "File already exists", inspects the output file they asked for, finds nothing wrong
+// with it, and never learns the obstacle is a <root>.temp they never created.
+func TestCheckFileExistence_NamesTheFile(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "0x1111.temp")
+	require.NoError(t, os.WriteFile(path, []byte("a completed fragment"), 0644))
+
+	root, err := core.MerkleRoot(path)
+	require.NoError(t, err)
+
+	matching := checkFileExistence(path, root)
+	require.Error(t, matching)
+	assert.ErrorIs(t, matching, ErrFileAlreadyExists,
+		"download_dir matches on this sentinel, so it must stay wrapped rather than replaced")
+	assert.Contains(t, matching.Error(), path, "the message must say which file")
+
+	mismatched := checkFileExistence(path, common.HexToHash("0xdead"))
+	require.Error(t, mismatched)
+	assert.NotErrorIs(t, mismatched, ErrFileAlreadyExists, "a different hash is a distinct, fatal error")
+	assert.Contains(t, mismatched.Error(), "different hash")
+	assert.Contains(t, mismatched.Error(), path, "the message must say which file")
+}
+
+// A path that does not exist is not an error at all.
+func TestCheckFileExistence_MissingFileIsNotAnError(t *testing.T) {
+	assert.NoError(t, checkFileExistence(filepath.Join(t.TempDir(), "absent.dat"), common.HexToHash("0xdead")))
+}

transfer/hot_downloader.go

@@ -166,6 +166,9 @@ func (d *HotDownloader) downloadPlainFragments(ctx context.Context, roots []stri
 				d.logger.WithField("fragment", i).Info("Fragment not in hot storage, falling back")
 			}
 			if err := d.fallback.Download(ctx, root, tempFile, withProof); err != nil {
+				if errors.Is(err, ErrFileAlreadyExists) {
+					return FragmentLeftBehindError(err)
+				}
 				return errors.WithMessage(err, fmt.Sprintf("failed to download fragment %d", i))
 			}
 		}
@@ -241,6 +244,9 @@ func (d *HotDownloader) downloadFragmentData(ctx context.Context, root string, i
 		}
 		tempFile := fmt.Sprintf("%v.temp", root)
 		if err := d.fallback.Download(ctx, root, tempFile, withProof); err != nil {
+			if errors.Is(err, ErrFileAlreadyExists) {
+				return nil, FragmentLeftBehindError(err)
+			}
 			return nil, errors.WithMessage(err, fmt.Sprintf("failed to download fragment %d", index))
 		}
 		fragmentData, err := os.ReadFile(tempFile)

transfer/hot_downloader_test.go

@@ -752,3 +752,30 @@ func mustNotBeContacted(t *testing.T, what string) *httptest.Server {
 
 	return server
 }
+
+// A complete fragment file from an earlier attempt blocks the next one. The caller
+// never created it and cannot guess it is the obstacle, so the failure has to name it
+// and say what to do about it.
+func TestHotDownloader_DownloadFragments_LeftoverFragmentTellsUserToRemoveIt(t *testing.T) {
+	chdirTemp(t)
+
+	router := newTestRouter(t, "") // 404 = cache miss, so the fallback is consulted
+	defer router.Close()
+
+	downloader := NewHotDownloader(
+		node.NewHotRouterClient(router.URL, testChainID),
+		testKey(t),
+		&mockFallbackDownloader{downloadFunc: func(_ context.Context, _, filename string, _ bool) error {
+			// Wrapped as checkFileExistence reports it once the file is named.
+			return errors.WithMessagef(ErrFileAlreadyExists, "%v", filename)
+		}},
+	)
+
+	err := downloader.DownloadFragments(context.Background(), []string{"0x1111"}, "output.dat", false)
+
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "0x1111.temp", "the message must name the leftover fragment file")
+	assert.Contains(t, err.Error(), "remove it and retry", "the message must say what to do")
+	assert.Equal(t, 1, strings.Count(err.Error(), "0x1111.temp"),
+		"the path comes from checkFileExistence, so the hint must not repeat it")
+}