Write rclone uploads under a temporary name, re-upload short blobs (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 a remote where rclone says a file can be seen while it is still being
written, an object is now written under a name ending in `.partial` and
moved onto its key with the remote's server-side move, as rclone's own
copy does; listings skip such names. A backup also uploads a blob again
when the stored object's size differs from the blob's.

Judgement call: the temporary name is used only where rclone sets its
PartialUploads feature; other remotes already show an object only once
complete.

Model: opus-5-5

README.md

@@ -431,12 +431,13 @@ 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 with a server-side move (local and sftp among them) are written under a
+remotes that show a file while it is still being written (local, sftp, ftp and
-temporary name ending in `.partial` and moved into place. Rclone remotes without
+smb among them) are written under a temporary name ending in `.partial` and
-one are written in place: where such a remote shows a file while it is still
+moved into place where the remote has a server-side move; without one they are
-being written, a killed upload can leave a truncated object under its name,
+written in place. Other rclone remotes show an object only once its upload has
-which a later backup takes for complete. A leftover `.partial` file is ignored
+completed. A leftover `.partial` file is ignored and can be deleted. A backup
-and can be deleted.
+that finds a blob stored at a size other than the one it packed uploads the
+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,16 +22,17 @@ the tag exists and is exercised; what is left is merging `next` to
 
 # Completed Steps
 
-- 2026-10-07: Stopped a killed rclone upload from leaving a truncated
+- 2026-10-07: Stopped a backup from trusting a blob object left short by
-  object under its key
+  a killed upload
   ([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 with a server-side
+  a snapshot that could not be restored. On a remote where rclone says a
-  move, an object is now written under a temporary name ending in
+  file can be seen while it is being written, an object is now written
-  `.partial` and moved into place, and listings skip such names. Remotes
+  under a temporary name ending in `.partial` and moved into place, and
-  without one are still written in place.
+  listings skip such names. A backup also uploads a blob again when the
+  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 if it doesn't already
+// uploadBlobIfNeeded uploads the blob to storage unless an object of the
-// exist, returns whether it existed
+// blob's size is already there, and returns whether it was there
 func (s *Scanner) uploadBlobIfNeeded(
 	ctx context.Context,
 	blobPath string,
@@ -1546,11 +1546,13 @@ func (s *Scanner) uploadBlobIfNeeded(
 ) (bool, error) {
 	finishedBlob := blobWithReader.FinishedBlob
 
-	// Check if blob already exists (deduplication after restart)
+	// Check if blob already exists (deduplication after restart). An
+	// object of another size was left by an upload cut off part-way and
+	// is uploaded again.
 	destination := s.storage.Info().Location
 
-	_, err := s.storage.Stat(ctx, blobPath)
+	stored, err := s.storage.Stat(ctx, blobPath)
-	if err == nil {
+	if err == nil && stored.Size == finishedBlob.Compressed {
 		log.Info("Blob already exists in storage, skipping upload",
 			"hash", finishedBlob.Hash,
 			"size", humanize.Bytes(safeUint64(finishedBlob.Compressed)))
@@ -1561,6 +1563,11 @@ 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,10 +50,9 @@ 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 with a
+// blob. The rclone backend's upload does the same on remotes that need it.
-// server-side move. List and ListStream skip these files, so a leftover from
+// List and ListStream skip these files, so a leftover from an interrupted
-// an interrupted write is never listed or trusted as a blob; it is otherwise
+// write is never listed or trusted as a blob; it is otherwise harmless.
-// harmless.
 const tempSuffix = ".partial"
 
 // Put stores data at the specified key.

internal/storage/rclone.go

@@ -226,18 +226,15 @@ func (r *RcloneStorer) Info() Info {
 	}
 }
 
-// upload writes data to key. Where the remote has a server-side move, it
+// upload writes data to key. On a remote where rclone says a file can be
-// writes under a temporary name ending in tempSuffix and moves the object
+// seen while it is still being written (its PartialUploads feature: local,
-// onto key once it is complete, so a killed upload cannot leave a truncated
+// sftp, ftp, smb and others), it writes under a temporary name ending in
-// object at key; a remote without one is written in place. List and
+// tempSuffix and moves the object onto key once it is complete, as
-// ListStream skip a temporary object left behind.
+// rclone's own copy does, so a killed upload cannot leave a truncated
-//
+// 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.Move == nil {
+	if !features.PartialUploads || 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
-// with a server-side move, such as local, nothing is at the key until the
+// where a file can be seen while it is still being written, such as local,
-// upload has finished, so a killed upload cannot leave a truncated object
+// nothing is at the key until the upload has finished, so a killed upload
-// there.
+// cannot leave a truncated object there.
 //
 //nolint:paralleltest // NewRcloneStorer installs the process-global rclone config
 func TestRcloneStorerObjectAppearsOnlyWhenComplete(t *testing.T) {

internal/vaultik/fault_injection_test.go

@@ -5,6 +5,7 @@ import (
 	"errors"
 	"io"
 	"os"
+	"path"
 	"path/filepath"
 	"strings"
 	"testing"
@@ -414,6 +415,69 @@ 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