Write rclone uploads under a temporary name and move them into place (closes #266)

The rclone backend wrote each object straight to its key, so killing an
upload to a local or sftp remote left a truncated object there. The next
backup found the key, skipped the upload and recorded a snapshot that
could not be restored.

On every remote with a server-side move, an object is now written under
a name ending in `.partial` and moved onto its key; listings skip such
names. Rclone's own copy also requires the remote's PartialUploads flag;
this does not, because hdfs shows a file while it is written without
setting it. Remotes without a move are written in place, as the README
now says.

Model: opus-5-5

README.md

@@ -431,13 +431,12 @@ providers. Requires rclone to be configured separately (`rclone config`).
 An upload cut off part-way leaves nothing under the object's name on S3, which
 shows an object only once its upload has completed, and on the local filesystem
 backend, which writes a temporary file and renames it into place. Rclone
-remotes that show a file while it is still being written (local, sftp, ftp and
+remotes with a server-side move (local and sftp among them) are written under a
-smb among them) are written under a temporary name ending in `.partial` and
+temporary name ending in `.partial` and moved into place. Rclone remotes without
-moved into place where the remote has a server-side move; without one they are
+one are written in place: where such a remote shows a file while it is still
-written in place. Other rclone remotes show an object only once its upload has
+being written, a killed upload can leave a truncated object under its name,
-completed. A leftover `.partial` file is ignored and can be deleted. A backup
+which a later backup takes for complete. A leftover `.partial` file is ignored
-that finds a blob stored at a size other than the one it packed uploads the
+and can be deleted.
-blob again.
 
 Legacy S3 configuration via `s3.*` fields (endpoint, bucket, prefix, etc.) is
 still supported for backward compatibility. `storage_url` takes precedence if

TODO.md

@@ -22,17 +22,16 @@ the tag exists and is exercised; what is left is merging `next` to
 
 # Completed Steps
 
-- 2026-10-07: Stopped a backup from trusting a blob object left short by
+- 2026-10-07: Stopped a killed rclone upload from leaving a truncated
-  a killed upload
+  object under its key
   ([issue #266](https://git.eeqj.de/sneak/vaultik/issues/266)). The
   rclone backend wrote each object straight to its key, so killing an
   upload to a local or sftp remote left a truncated object there; the
   next backup found the key with `Stat`, skipped the upload and recorded
-  a snapshot that could not be restored. On a remote where rclone says a
+  a snapshot that could not be restored. On a remote with a server-side
-  file can be seen while it is being written, an object is now written
+  move, an object is now written under a temporary name ending in
-  under a temporary name ending in `.partial` and moved into place, and
+  `.partial` and moved into place, and listings skip such names. Remotes
-  listings skip such names. A backup also uploads a blob again when the
+  without one are still written in place.
-  stored object's size differs from the blob's.
 
 - 2026-10-07: Corrected documentation, help text and comments that were
   false about the code

internal/snapshot/scanner.go

@@ -1535,8 +1535,8 @@ func (s *Scanner) handleBlobReady(
 	return nil
 }
 
-// uploadBlobIfNeeded uploads the blob to storage unless an object of the
+// uploadBlobIfNeeded uploads the blob to storage if it doesn't already
-// blob's size is already there, and returns whether it was there
+// exist, returns whether it existed
 func (s *Scanner) uploadBlobIfNeeded(
 	ctx context.Context,
 	blobPath string,
@@ -1546,13 +1546,11 @@ func (s *Scanner) uploadBlobIfNeeded(
 ) (bool, error) {
 	finishedBlob := blobWithReader.FinishedBlob
 
-	// Check if blob already exists (deduplication after restart). An
+	// Check if blob already exists (deduplication after restart)
-	// object of another size was left by an upload cut off part-way and
-	// is uploaded again.
 	destination := s.storage.Info().Location
 
-	stored, err := s.storage.Stat(ctx, blobPath)
+	_, err := s.storage.Stat(ctx, blobPath)
-	if err == nil && stored.Size == finishedBlob.Compressed {
+	if err == nil {
 		log.Info("Blob already exists in storage, skipping upload",
 			"hash", finishedBlob.Hash,
 			"size", humanize.Bytes(safeUint64(finishedBlob.Compressed)))
@@ -1563,11 +1561,6 @@ func (s *Scanner) uploadBlobIfNeeded(
 		return true, nil
 	}
 
-	if err == nil {
-		log.Warn("Blob in storage has the wrong size, uploading it again",
-			"hash", finishedBlob.Hash, "stored_size", stored.Size)
-	}
-
 	s.ui.Beginf("Uploading blob %s (%s) to %s.",
 		s.ui.Hex(finishedBlob.Hash), s.ui.Size(finishedBlob.Compressed),
 		s.ui.Path(destination))

internal/storage/file.go

@@ -50,9 +50,10 @@ const storageDirPerm = 0o755
 // temp file carrying this suffix and only renames it onto the real key once
 // the whole object is on disk, so an interrupted write can never leave a
 // truncated object at the key a later run would Stat and trust as a complete
-// blob. The rclone backend's upload does the same on remotes that need it.
+// blob. The rclone backend's upload does the same on remotes with a
-// List and ListStream skip these files, so a leftover from an interrupted
+// server-side move. List and ListStream skip these files, so a leftover from
-// write is never listed or trusted as a blob; it is otherwise harmless.
+// an interrupted write is never listed or trusted as a blob; it is otherwise
+// harmless.
 const tempSuffix = ".partial"
 
 // Put stores data at the specified key.

internal/storage/rclone.go

@@ -226,15 +226,18 @@ func (r *RcloneStorer) Info() Info {
 	}
 }
 
-// upload writes data to key. On a remote where rclone says a file can be
+// upload writes data to key. Where the remote has a server-side move, it
-// seen while it is still being written (its PartialUploads feature: local,
+// writes under a temporary name ending in tempSuffix and moves the object
-// sftp, ftp, smb and others), it writes under a temporary name ending in
+// onto key once it is complete, so a killed upload cannot leave a truncated
-// tempSuffix and moves the object onto key once it is complete, as
+// object at key; a remote without one is written in place. List and
-// rclone's own copy does, so a killed upload cannot leave a truncated
+// ListStream skip a temporary object left behind.
-// object at key. List and ListStream skip a temporary object left behind.
+//
+// rclone's own copy does this only where the remote also sets
+// PartialUploads. That flag is not checked here: hdfs, for one, shows a
+// file while it is written without setting it.
 func (r *RcloneStorer) upload(ctx context.Context, key string, data io.Reader) error {
 	features := r.fsys.Features()
-	if !features.PartialUploads || features.Move == nil {
+	if features.Move == nil {
 		_, err := operations.Rcat(ctx, r.fsys, key, io.NopCloser(data), time.Now(), nil)
 		if err != nil {
 			return fmt.Errorf("uploading object: %w", err)

internal/storage/rclone_test.go

@@ -62,9 +62,9 @@ func TestNewRcloneStorerConstruction(t *testing.T) {
 }
 
 // TestRcloneStorerObjectAppearsOnlyWhenComplete checks that on a remote
-// where a file can be seen while it is still being written, such as local,
+// with a server-side move, such as local, nothing is at the key until the
-// nothing is at the key until the upload has finished, so a killed upload
+// upload has finished, so a killed upload cannot leave a truncated object
-// cannot leave a truncated object there.
+// there.
 //
 //nolint:paralleltest // NewRcloneStorer installs the process-global rclone config
 func TestRcloneStorerObjectAppearsOnlyWhenComplete(t *testing.T) {

internal/vaultik/fault_injection_test.go

@@ -5,7 +5,6 @@ import (
 	"errors"
 	"io"
 	"os"
-	"path"
 	"path/filepath"
 	"strings"
 	"testing"
@@ -415,69 +414,6 @@ func TestBackupRetryAfterInterruptedUploadIsRestorable(t *testing.T) {
 	assertRestoredTree(t, fs, restoreDir, testFiles)
 }
 
-// Scenario 1c: a killed upload left a short object at a blob's key, as an
-// rclone remote that writes in place can. A later backup that packs the
-// same blob must upload it again rather than trust the short object. See
-// https://git.eeqj.de/sneak/vaultik/issues/266.
-func TestBackupReplacesShortBlobObject(t *testing.T) {
-	log.Initialize(log.Config{})
-	t.Parallel()
-
-	fs := afero.NewOsFs()
-	tempDir := t.TempDir()
-	dataDir := filepath.Join(tempDir, "src")
-	restoreDir := filepath.Join(tempDir, "restored")
-	firstDBPath := filepath.Join(tempDir, "first.sqlite")
-	retryDBPath := filepath.Join(tempDir, "retry.sqlite")
-
-	ctx := context.Background()
-	cfg := faultTestConfig()
-	testFiles := writeFaultSourceTree(t, fs, dataDir)
-
-	store, err := storage.NewFileStorer(filepath.Join(tempDir, "remote"))
-	require.NoError(t, err)
-
-	firstDB, err := database.New(ctx, firstDBPath)
-	require.NoError(t, err)
-	fullFaultBackup(ctx, t, fs, store, cfg, database.NewRepositories(firstDB),
-		dataDir, firstDBPath, "first")
-	require.NoError(t, firstDB.Close())
-
-	blobKeys, err := store.List(ctx, "blobs/")
-	require.NoError(t, err)
-	require.NotEmpty(t, blobKeys)
-
-	shortKey := blobKeys[0]
-	require.NoError(t, store.Put(ctx, shortKey, strings.NewReader("cut off")))
-
-	// A backup with a new local index packs the same blobs again.
-	retryDB, err := database.New(ctx, retryDBPath)
-	require.NoError(t, err)
-
-	retryRepos := database.NewRepositories(retryDB)
-	id := fullFaultBackup(ctx, t, fs, store, cfg, retryRepos,
-		dataDir, retryDBPath, "retry")
-
-	blob, err := retryRepos.Blobs.GetByHash(ctx, path.Base(shortKey))
-	require.NoError(t, err)
-	require.NotNil(t, blob, "the retry must pack the blob that was cut short")
-	require.NoError(t, retryDB.Close())
-
-	info, err := store.Stat(ctx, shortKey)
-	require.NoError(t, err)
-	assert.Equal(t, blob.CompressedSize, info.Size,
-		"the short object must be replaced by the whole blob")
-
-	reader := newReaderVaultik(ctx, cfg, store, nil, fs)
-	require.NoError(t, reader.Restore(&vaultik.RestoreOptions{
-		SnapshotID: id,
-		TargetDir:  restoreDir,
-		Verify:     true,
-	}), "the retry's snapshot must be restorable")
-
-	assertRestoredTree(t, fs, restoreDir, testFiles)
-}
-
 // Scenario 2: the process dies during the metadata export, after the
 // database is uploaded but before the manifest. The destination is left
 // with blobs and a database but no manifest. verify and snapshot list