Keep unreferenced blobs newer than prune_grace_period (closes #387)

A backup uploads its blobs before the manifest that lists them, and the
process lock is per user per machine, so a prune from another host or
user sharing the destination deleted a running backup's blobs; that
backup still reported success and could not be restored.

Prune now deletes an unreferenced blob only once it is older than the new
config key prune_grace_period (default 24h), and reports how many it kept
(blobs_too_new under --json). The storage listing now carries each
object's modification time. remote nuke is unchanged. The README says what
the lock covers and that a shared destination relies on the grace period
or needs one prefix per host.

Model: opus-5-5

README.md

@@ -99,10 +99,8 @@ vaultik snapshot restore <snapshot-id> ~/vaultik-restore
 On a destination shared between hosts or users, the prune in that cron line
 relies on `prune_grace_period` (default `24h`) to leave the blobs of a backup
 another one is still running, so it must be longer than the longest backup
-run. It does not protect a blob that backup reuses from an earlier run that
-stopped before its manifest: the prune can delete such a blob, and the new
-snapshot then cannot be restored. To not depend on the grace period, give each
-host its own prefix or directory. See [locking](#locking).
+run; otherwise give each host its own prefix or directory. See
+[locking](#locking).
 
 ## restoring on another machine
 
@@ -208,14 +206,9 @@ time. A backup uploads its blobs before the manifest that lists them, so a
 prune from elsewhere at that moment finds blobs no snapshot lists. It
 deletes such a blob only once it is older than `prune_grace_period`
 (default `24h`), which therefore must be longer than the longest backup run
-of any host or user sharing the destination. The grace period does not
-protect a blob a backup reuses from an earlier run that stopped before its
-manifest: a backup skips the upload of a blob already on the destination, so
-that blob keeps the time of its first upload, and a prune from elsewhere
-during the backup can delete it; the new snapshot then cannot be restored.
-`remote nuke` deletes everything regardless. To not depend on the grace
-period, give each host or user its own prefix (or directory) on the
-destination.
+of any host or user sharing the destination. `remote nuke` deletes
+everything regardless. To not depend on the grace period, give each host
+or user its own prefix (or directory) on the destination.
 
 Read-only commands — `info`, `snapshot list`, `snapshot verify`, and
 `remote info` — do not take the lock and are never blocked, so they run
@@ -450,11 +443,8 @@ manifest on the destination store to compute the still-referenced blob
 set and delete any blob not in that set that is older than
 `prune_grace_period` (default `24h`). A newer one is kept and counted: it
 may belong to a backup still running, on this host or another sharing the
-destination store, that has not yet uploaded its manifest. The grace period
-does not protect a blob such a backup reuses from an earlier run that stopped
-before its manifest: that blob keeps the time of its first upload, so it can
-be deleted. Step (3) reads all remote manifests — network cost scales with
-the number of snapshots.
+destination store, that has not yet uploaded its manifest. Step (3) reads
+all remote manifests — network cost scales with the number of snapshots.
 `snapshot create --prune` runs the same cleanup automatically; this is the
 manual entry point for the same work. If the destination store cannot
 be listed, or the local index holds snapshots and the destination has
@@ -466,8 +456,7 @@ exits non-zero.
 * `--json`: Output stats as JSON. Also skips the confirmation prompt, as
   `--force` does. A blob that cannot be deleted is counted in
   `blobs_failed`, and the document is printed before the non-zero exit.
-  A blob kept for being newer than `prune_grace_period`, or because the
-  listing of the destination gives no time for it, is counted in
+  A blob kept for being newer than `prune_grace_period` is counted in
   `blobs_too_new` and not in `blobs_found`.
 
 **`info`**: Display system configuration, storage settings, encryption
@@ -546,9 +535,7 @@ and can be deleted.
 A destination shared between hosts or users, each with its own local index,
 relies on `prune_grace_period` to keep one's prune from deleting the blobs of
 another's backup still running, so it must be longer than the longest backup
-run. It does not protect a blob that backup reuses from an earlier run that
-stopped before its manifest, which the prune can delete. To not depend on the
-grace period, give each host its own prefix (or directory). See
+run; otherwise give each host its own prefix (or directory). See
 [locking](#locking).
 
 Legacy S3 configuration via `s3.*` fields (endpoint, bucket, prefix, etc.) is
@@ -683,7 +670,7 @@ complete annotated example also lives in
 | `chunk_size` | `10MB` | Average chunk size for content-defined chunking |
 | `blob_size_limit` | `10GB` | Maximum blob size before splitting. Must be at least four times `chunk_size` (the largest chunk the chunker can emit), otherwise a single-chunk blob could exceed the limit. A backup needs free temporary space, because each blob is written whole to a temporary file before it is uploaded (up to about `blob_size_limit`; an rclone destination that cannot stream uploads needs about twice that) and the metadata export writes copies of the local index. Temporary files go to `$TMPDIR` (default `/tmp`); with `TMPDIR` unset, SQLite writes one of those copies to `/var/tmp` |
 | `compression_level` | `3` | zstd compression level (1-19) |
-| `prune_grace_period` | `24h` | How old a blob that no snapshot lists must be before `prune` deletes it. A backup uploads its blobs before the manifest that lists them, so on a destination shared between hosts or users this must be longer than the longest backup run (see [locking](#locking)). It does not protect a blob a backup reuses from an earlier run that stopped before its manifest: that blob keeps the time of its first upload, so a prune can delete it. A duration such as `36h` or `90m`; `0s` deletes such blobs at once |
+| `prune_grace_period` | `24h` | How old a blob that no snapshot lists must be before `prune` deletes it. A backup uploads its blobs before the manifest that lists them, so on a destination shared between hosts or users this must be longer than the longest backup run (see [locking](#locking)). A duration such as `36h` or `90m`; `0s` deletes such blobs at once |
 | `hostname` | system hostname | Hostname used in snapshot IDs |
 | `index_path` | platform data dir | Local SQLite index path |
 

config.example.yml

@@ -333,9 +333,6 @@ compression_level: 5
 # A backup uploads its blobs before the manifest that lists them, so on a
 # destination shared between hosts or users this must be longer than the
 # longest backup run
-# It does not protect a blob a backup reuses from an earlier run that stopped
-# before its manifest: that blob keeps the time of its first upload, so a
-# prune can delete it
 # Supports: 36h, 90m, etc.; 0s deletes them at once
 # Default: 24h
 #prune_grace_period: 24h

docs/REPOSTRUCTURE.md

@@ -166,7 +166,7 @@ The prune operation is safe because:
 1. It keeps every blob listed in any snapshot's manifest and deletes only blobs that no manifest references
 2. Manifests are unencrypted and can be read without keys
 3. If any manifest cannot be downloaded or decoded, prune deletes nothing and exits with an error, rather than treating that snapshot's blobs as unreferenced
-4. It deletes an unreferenced blob only once the blob is older than `prune_grace_period` (default `24h`). A backup uploads its blobs before the manifest that lists them, so a prune running meanwhile, on this host or another sharing the destination, leaves them as long as no backup run takes longer than the grace period. The grace period does not protect a blob a backup reuses from an earlier run that stopped before its manifest: the backup skips the upload of a blob already on the destination, so that blob keeps the time of its first upload, and a prune before the backup's manifest is written can delete it; the new snapshot then cannot be restored. `remote nuke` has no such check
+4. It deletes an unreferenced blob only once the blob is older than `prune_grace_period` (default `24h`). A backup uploads its blobs before the manifest that lists them, so a prune running meanwhile, on this host or another sharing the destination, leaves them as long as no backup run takes longer than the grace period. `remote nuke` has no such check
 
 ## Restoration Requirements
 

internal/cli/config.go

@@ -242,10 +242,7 @@ storage_url: ""
 # How old a blob that no snapshot lists must be before prune deletes it.
 # A backup uploads its blobs before the manifest that lists them, so on a
 # destination shared between hosts or users this must be longer than the
-# longest backup run. It does not protect a blob a backup reuses from an
-# earlier run that stopped before its manifest: that blob keeps the time of
-# its first upload, so a prune can delete it.
-# Accepts: 36h, 90m, etc. 0s deletes them at once.
+# longest backup run. Accepts: 36h, 90m, etc. 0s deletes them at once.
 # Default: 24h
 # prune_grace_period: 24h
 

internal/cli/prune.go

@@ -23,9 +23,7 @@ set of still-referenced blob hashes, and deletes any blob not in that
 set that is older than prune_grace_period (default 24h). A newer one
 is kept: it may belong to a backup still running, on this host or
 another sharing the destination store, that has not yet uploaded its
-manifest. The grace period does not protect a blob such a backup
-reuses from an earlier run that stopped before its manifest: that
-blob keeps the time of its first upload, so it can be deleted.
+manifest.
 
 Snapshot create --prune runs the same cleanup automatically; this
 command is the manual entry point for the same work (e.g. after a

internal/config/config.go

@@ -69,8 +69,6 @@ var (
 	errFileURLNotAbsolute = errors.New(
 		"a file:// storage_url needs an absolute path, " +
 			"as in file:///path/to/backup")
-	errNegativePruneGracePeriod = errors.New(
-		"prune_grace_period must not be negative")
 	errStorageNotConfigured = errors.New(
 		"storage not configured; set storage_url or provide s3.endpoint + " +
 			"s3.bucket + credentials")
@@ -368,7 +366,6 @@ func Parse(data []byte) (*Config, error) {
 //     exceeds the configured limit
 //   - Compression level must be between 1 and 19
 //   - S3 part size must be between 5MiB and 5GiB, the part sizes S3 accepts
-//   - Prune grace period must not be negative
 //
 // Returns an error describing the first validation failure encountered.
 func (c *Config) Validate() error {
@@ -418,10 +415,6 @@ func (c *Config) Validate() error {
 		return errBadS3PartSize
 	}
 
-	if c.PruneGracePeriod < 0 {
-		return errNegativePruneGracePeriod
-	}
-
 	return nil
 }
 

internal/config/config_test.go

@@ -6,7 +6,6 @@ import (
 	"path/filepath"
 	"strings"
 	"testing"
-	"time"
 
 	"sneak.berlin/go/vaultik/internal/chunker"
 	"sneak.berlin/go/vaultik/internal/log"
@@ -413,73 +412,6 @@ func TestDocumentedSizeExamples(t *testing.T) {
 	}
 }
 
-// TestValidatePruneGracePeriod checks that a negative prune_grace_period
-// is refused rather than acting like 0s, which deletes unreferenced blobs
-// at once, and that 0s and the documented examples are accepted.
-func TestValidatePruneGracePeriod(t *testing.T) {
-	t.Parallel()
-
-	const withoutGracePeriod = "snapshots:\n" +
-		"  test:\n" +
-		"    paths: [/tmp/vaultik-test-source]\n" +
-		"storage_url: file:///tmp/vaultik-test-storage\n"
-
-	tests := []struct {
-		setting string
-		wantErr error
-	}{
-		{"prune_grace_period: 0s", nil},
-		{"prune_grace_period: 90m", nil},
-		{"prune_grace_period: 36h", nil},
-		{"prune_grace_period: -1h", errNegativePruneGracePeriod},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.setting, func(t *testing.T) {
-			t.Parallel()
-
-			cfg, err := Parse([]byte(withoutGracePeriod + tt.setting + "\n"))
-			if err != nil {
-				t.Fatalf("Parse() unexpected error: %v", err)
-			}
-
-			err = cfg.Validate()
-			if !errors.Is(err, tt.wantErr) {
-				t.Fatalf("Validate() error = %v, want %v", err, tt.wantErr)
-			}
-		})
-	}
-}
-
-// TestLoadPruneGracePeriod checks that a config file without
-// prune_grace_period loads with the documented 24h. The value is written out
-// here rather than taken from defaultPruneGracePeriod, so that a change of the
-// default fails this test.
-func TestLoadPruneGracePeriod(t *testing.T) {
-	t.Parallel()
-
-	const withoutGracePeriod = "snapshots:\n" +
-		"  test:\n" +
-		"    paths: [/tmp/vaultik-test-source]\n" +
-		"storage_url: file:///tmp/vaultik-test-storage\n"
-
-	path := filepath.Join(t.TempDir(), "config.yml")
-
-	err := os.WriteFile(path, []byte(withoutGracePeriod), 0o600)
-	if err != nil {
-		t.Fatalf("write config: %v", err)
-	}
-
-	cfg, err := Load(path)
-	if err != nil {
-		t.Fatalf("Load() unexpected error: %v", err)
-	}
-
-	if cfg.PruneGracePeriod != 24*time.Hour {
-		t.Errorf("prune_grace_period = %s, want 24h", cfg.PruneGracePeriod)
-	}
-}
-
 // TestLoadRejectsRelativeFileURL checks that a file:// storage_url with a
 // relative path fails at load. Accepted, it would resolve against the
 // working directory, so a backup run from another directory would write

internal/storage/rclone.go

@@ -53,14 +53,6 @@ func NewRcloneStorer(ctx context.Context, remote, path string) (*RcloneStorer, e
 		rclonePath += path
 	}
 
-	// On S3, Swift and some other remotes rclone keeps a time of its own in
-	// each object's metadata, which costs one request per object to read.
-	// rclone's use_server_modtime setting makes ModTime the time the
-	// listing gives instead: when the object was stored, which is what
-	// prune compares with prune_grace_period.
-	ctx, ci := fs.AddConfig(ctx)
-	ci.UseServerModTime = true
-
 	// Create the rclone filesystem
 	fsys, err := fs.NewFs(ctx, rclonePath)
 	if err != nil {

internal/storage/rclone_test.go

@@ -5,17 +5,11 @@ import (
 	"context"
 	"errors"
 	"io"
-	"net/http"
-	"net/http/httptest"
 	"os"
 	"path/filepath"
 	"strings"
-	"sync/atomic"
 	"testing"
-	"time"
 
-	"github.com/johannesboyne/gofakes3"
-	"github.com/johannesboyne/gofakes3/backend/s3mem"
 	"github.com/rclone/rclone/fs"
 	"github.com/rclone/rclone/fs/config"
 	"github.com/rclone/rclone/fs/config/configmap"
@@ -435,72 +429,3 @@ func TestNewRcloneStorerReadsRcloneConfigEnv(t *testing.T) {
 		t.Errorf("object is not in the remote's directory: %v", err)
 	}
 }
-
-// TestRcloneListStreamOnS3MakesNoRequestPerObject checks that listing an
-// rclone S3 remote takes each object's time from the listing itself. By
-// default rclone reads the time it keeps in each object's metadata, one
-// request per object, and every command that lists the destination store
-// would slow down as it grows.
-//
-//nolint:paralleltest // NewRcloneStorer installs the process-global rclone config
-func TestRcloneListStreamOnS3MakesNoRequestPerObject(t *testing.T) {
-	backend := s3mem.New()
-
-	err := backend.CreateBucket(s3TestBucket)
-	if err != nil {
-		t.Fatalf("create bucket: %v", err)
-	}
-
-	const body = "blob-bytes"
-
-	_, err = backend.PutObject(s3TestBucket, testBlobKey, nil,
-		strings.NewReader(body), int64(len(body)))
-	if err != nil {
-		t.Fatalf("seed object: %v", err)
-	}
-
-	var headRequests atomic.Int64
-
-	fakeS3 := gofakes3.New(backend).Server()
-	srv := httptest.NewServer(http.HandlerFunc(
-		func(w http.ResponseWriter, r *http.Request) {
-			if r.Method == http.MethodHead {
-				headRequests.Add(1)
-			}
-
-			fakeS3.ServeHTTP(w, r)
-		}))
-	t.Cleanup(srv.Close)
-
-	remote := ":s3,provider=Other" +
-		",access_key_id=" + s3TestAccessKeyID +
-		",secret_access_key=" + s3TestSecretAccessKey +
-		",endpoint='" + srv.URL + "'"
-	ctx := context.Background()
-
-	s, err := storage.NewRcloneStorer(ctx, remote, s3TestBucket)
-	if err != nil {
-		t.Fatalf("NewRcloneStorer: %v", err)
-	}
-
-	headRequests.Store(0)
-
-	var listed []storage.ObjectInfo
-
-	for info := range s.ListStream(ctx, "blobs/") {
-		listed = append(listed, info)
-	}
-
-	if len(listed) != 1 || listed[0].Err != nil {
-		t.Fatalf("ListStream(\"blobs/\") = %v, want the one object", listed)
-	}
-
-	if time.Since(listed[0].ModTime).Abs() > time.Minute {
-		t.Errorf("ListStream ModTime = %v, want within a minute of now",
-			listed[0].ModTime)
-	}
-
-	if n := headRequests.Load(); n != 0 {
-		t.Errorf("ListStream made %d HEAD request(s), want none", n)
-	}
-}

internal/vaultik/prune.go

@@ -166,8 +166,7 @@ type PruneBlobsResult struct {
 	BlobsFailed  int   `json:"blobs_failed,omitempty"`
 	BytesFreed   int64 `json:"bytes_freed"`
 	// BlobsTooNew counts the unreferenced blobs kept because they are
-	// newer than the prune grace period, or of unknown age. BlobsFound
-	// does not include them.
+	// newer than the prune grace period. BlobsFound does not include them.
 	BlobsTooNew int `json:"blobs_too_new"`
 }
 
@@ -396,8 +395,7 @@ func (v *Vaultik) listAllRemoteBlobs() (map[string]storage.ObjectInfo, error) {
 // findUnreferencedBlobs returns the hashes of the blobs no manifest
 // references that are older than the prune grace period, and their total
 // size. It also returns how many unreferenced blobs it left out for
-// being newer than that, or of unknown age because the listing gave no
-// time for them.
+// being newer than that.
 func (v *Vaultik) findUnreferencedBlobs(
 	allBlobs map[string]storage.ObjectInfo, referenced map[string]bool,
 ) ([]string, int64, int) {
@@ -414,7 +412,7 @@ func (v *Vaultik) findUnreferencedBlobs(
 			continue
 		}
 
-		if blob.ModTime.IsZero() || blob.ModTime.After(cutoff) {
+		if blob.ModTime.After(cutoff) {
 			tooNew++
 
 			continue
@@ -428,16 +426,15 @@ func (v *Vaultik) findUnreferencedBlobs(
 }
 
 // reportBlobsTooNew says how many unreferenced blobs prune kept for being
-// newer than the grace period or of unknown age. Under --json the
-// document carries the count instead.
+// newer than the grace period. Under --json the document carries the
+// count instead.
 func (v *Vaultik) reportBlobsTooNew(tooNew int, opts *PruneOptions) {
 	if tooNew == 0 || opts.JSON {
 		return
 	}
 
 	v.UI.Infof("Kept %d unreferenced blob(s) newer than "+
-		"prune_grace_period (%s), or of unknown age: a backup still "+
-		"running may need them.",
+		"prune_grace_period (%s): a backup still running may need them.",
 		tooNew, v.Config.PruneGracePeriod)
 }
 

internal/vaultik/prune_grace_period_test.go

@@ -84,34 +84,6 @@ func TestPruneDuringBackupKeepsItsBlobs(t *testing.T) {
 	assertRestoredTree(t, fs, restoreDir, testFiles)
 }
 
-// TestPruneKeepsUnreferencedBlobWithNoTime checks that a blob no
-// manifest lists, whose listing gives no modification time (as an S3
-// listing entry without LastModified does), is kept and counted with
-// the blobs too new to delete: its age is unknown.
-func TestPruneKeepsUnreferencedBlobWithNoTime(t *testing.T) {
-	log.Initialize(log.Config{})
-	t.Parallel()
-
-	env := newListEnv(t)
-
-	// Written into the mock store's map directly rather than through
-	// Put, so its listing gives no time.
-	blobKey := "blobs/" + testBlobHashA[:2] + "/" + testBlobHashA[2:4] +
-		"/" + testBlobHashA
-	env.store.data[blobKey] = []byte("blob")
-
-	require.NoError(t, env.v.PruneBlobs(&vaultik.PruneOptions{JSON: true}))
-
-	assert.True(t, env.store.hasKey(blobKey),
-		"a blob of unknown age must be kept")
-
-	var result vaultik.PruneBlobsResult
-
-	require.NoError(t, json.Unmarshal(env.stdout.Bytes(), &result))
-	assert.Equal(t, 0, result.BlobsDeleted)
-	assert.Equal(t, 1, result.BlobsTooNew)
-}
-
 // TestPruneDeletesUnreferencedBlobOlderThanGracePeriod checks that the
 // grace period only delays deletion: a blob no manifest lists is deleted
 // once it is older than the grace period, and kept and counted until

internal/vaultik/remove_snapshot_test.go

@@ -26,14 +26,11 @@ import (
 type testStorer struct {
 	mu   sync.Mutex
 	data map[string][]byte
-	// putTimes holds when Put stored each object; ListStream reports it.
-	putTimes map[string]time.Time
 }
 
 func newTestStorer() *testStorer {
 	return &testStorer{
-		data:     make(map[string][]byte),
-		putTimes: make(map[string]time.Time),
+		data: make(map[string][]byte),
 	}
 }
 
@@ -47,7 +44,6 @@ func (s *testStorer) Put(_ context.Context, key string, reader io.Reader) error
 	}
 
 	s.data[key] = data
-	s.putTimes[key] = time.Now().UTC()
 
 	return nil
 }
@@ -91,7 +87,6 @@ func (s *testStorer) Delete(_ context.Context, key string) error {
 	defer s.mu.Unlock()
 
 	delete(s.data, key)
-	delete(s.putTimes, key)
 
 	return nil
 }
@@ -125,9 +120,8 @@ func (s *testStorer) ListStream(
 		for key, data := range s.data {
 			if prefix == "" || strings.HasPrefix(key, prefix) {
 				ch <- storage.ObjectInfo{
-					Key:     key,
-					Size:    int64(len(data)),
-					ModTime: s.putTimes[key],
+					Key:  key,
+					Size: int64(len(data)),
 				}
 			}
 		}