From 3dff4b9feaabf80aa8648837a9af5b72b8266bf0 Mon Sep 17 00:00:00 2001 From: sneak Date: Tue, 13 Oct 2026 03:43:58 +0000 Subject: [PATCH] Check --keep-newer-than before the backup starts (closes #307) snapshot create read --keep-newer-than only in the post-backup prune, so a value the duration parser rejects failed the run after the snapshot was uploaded: a nightly cron line added a snapshot each night, reported it as failed, and never deleted any. Without --prune the flag was ignored. CreateSnapshot now checks it first and fails, before anything is backed up, on an invalid duration or a missing --prune. A failed post-backup prune is now wrapped in ErrPostBackupPruneFailed and reported as "Backup completed, but post-backup prune failed: ..." rather than as a failed snapshot creation. The exit status stays 1. Model: opus-5-5 --- README.md | 4 +- .../cli/entry_snapshot_create_prune_test.go | 102 ++++++++++++++++++ internal/cli/snapshot.go | 7 ++ internal/vaultik/snapshot.go | 22 +++- 4 files changed, 133 insertions(+), 2 deletions(-) create mode 100644 internal/cli/entry_snapshot_create_prune_test.go diff --git a/README.md b/README.md index 1523495..f9a22df 100644 --- a/README.md +++ b/README.md @@ -286,7 +286,9 @@ needed. remove orphaned blobs from remote storage. By default keeps only the latest snapshot per name; use `--keep-newer-than` for a rolling window. * `--keep-newer-than `: With `--prune`, keep snapshots newer than - this duration instead of only the latest (e.g. `4w`, `30d`, `6mo`, `1y`) + this duration instead of only the latest (e.g. `4w`, `30d`, `6mo`, `1y`). + Without `--prune`, or with a value that is not a valid duration, the + command fails before the backup starts. **`snapshot list`**: Show every snapshot known to this host — the union of the local index and the backup destination store — with timestamps diff --git a/internal/cli/entry_snapshot_create_prune_test.go b/internal/cli/entry_snapshot_create_prune_test.go new file mode 100644 index 0000000..c5448cc --- /dev/null +++ b/internal/cli/entry_snapshot_create_prune_test.go @@ -0,0 +1,102 @@ +package cli //nolint:testpackage // shares runEntry and the hermetic config helpers + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestEntrySnapshotCreateRejectsKeepNewerThanBeforeBackup checks that a +// --keep-newer-than the post-backup prune could not use stops `snapshot +// create` before the backup: the command fails and the destination +// store stays empty. It used to be read only by the prune, after the +// snapshot was uploaded, and without --prune it was ignored. +// +//nolint:paralleltest // replaces os.Args, os.Stdout, os.Stderr and the xdg globals +func TestEntrySnapshotCreateRejectsKeepNewerThanBeforeBackup(t *testing.T) { + for _, testCase := range []struct { + name string + args []string + }{ + { + name: "decimal in a day count", + args: []string{"--prune", "--keep-newer-than", "1.5d"}, + }, + { + name: "unknown unit", + args: []string{"--prune", "--keep-newer-than", "4x"}, + }, + { + name: "without --prune", + args: []string{"--keep-newer-than", "4w"}, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + configPath, storeDir := writeBackupConfig(t) + + code, _, _ := runEntry(t, append( + []string{flagConfig, configPath, cmdSnapshot, cmdCreate}, + testCase.args...)...) + + assert.Equal(t, 1, code) + + stored, err := os.ReadDir(storeDir) + require.NoError(t, err) + assert.Empty(t, stored, "nothing may be uploaded") + }) + } +} + +// TestEntrySnapshotCreateReportsPostBackupPruneFailure makes the +// post-backup prune fail after a good backup, by planting a manifest +// that cannot be decoded, and checks that the failure is reported as the +// prune's and not as a failed snapshot creation. +// +//nolint:paralleltest // replaces os.Args, os.Stdout, os.Stderr and the xdg globals +func TestEntrySnapshotCreateReportsPostBackupPruneFailure(t *testing.T) { + configPath, storeDir := writeBackupConfig(t) + + // A remote key is 64 lowercase hex characters. The prune reads every + // manifest on the store and fails on one it cannot decode. + plantedDir := filepath.Join(storeDir, "metadata", strings.Repeat("a", 64)) + require.NoError(t, os.MkdirAll(plantedDir, 0o750)) + require.NoError(t, os.WriteFile( + filepath.Join(plantedDir, "manifest.json.zst"), + []byte("not a valid manifest"), 0o600)) + + code, _, stderr := runEntry(t, + flagConfig, configPath, cmdSnapshot, cmdCreate, "--prune") + + assert.Equal(t, 1, code) + assert.Contains(t, stderr, "Backup completed, but post-backup prune failed") + assert.NotContains(t, stderr, "Snapshot creation failed") + + snapshots, err := os.ReadDir(filepath.Join(storeDir, "metadata")) + require.NoError(t, err) + assert.Len(t, snapshots, 2, + "the new snapshot is on the store beside the planted one") +} + +// writeBackupConfig builds a hermetic config with one small file in its +// source directory and an empty destination store directory. Returns the +// config path and the store directory. +func writeBackupConfig(t *testing.T) (string, string) { + t.Helper() + + configPath := writeHermeticPruneConfig(t, false) + + // writeHermeticPruneConfig puts the source and store directories + // beside the config file. + dir := filepath.Dir(configPath) + sourceDir := filepath.Join(dir, "source") + + require.NoError(t, os.Mkdir(sourceDir, 0o750)) + require.NoError(t, os.WriteFile( + filepath.Join(sourceDir, "file.txt"), []byte("hello"), 0o600)) + + return configPath, filepath.Join(dir, "store") +} diff --git a/internal/cli/snapshot.go b/internal/cli/snapshot.go index 58b32db..7ea6730 100644 --- a/internal/cli/snapshot.go +++ b/internal/cli/snapshot.go @@ -97,6 +97,13 @@ on Linux), then /etc/vaultik/config.yml.`, }, func(v *vaultik.Vaultik) error { return v.CreateSnapshot(opts) }, func(err error) { + if errors.Is(err, vaultik.ErrPostBackupPruneFailed) { + log.Error("Post-backup prune failed", "error", err) + ReportErrorf("Backup completed, but %v", err) + + return + } + log.Error("Snapshot creation failed", "error", err) ReportErrorf("Snapshot creation failed: %v", err) }) diff --git a/internal/vaultik/snapshot.go b/internal/vaultik/snapshot.go index 97ecfdb..2b77d8b 100644 --- a/internal/vaultik/snapshot.go +++ b/internal/vaultik/snapshot.go @@ -27,8 +27,14 @@ var ( errNoAgeRecipients = errors.New( "creating a snapshot needs at least one public key in " + "age_recipients (generate a keypair with: age-keygen)") + errKeepNewerThanNeedsPrune = errors.New("--keep-newer-than requires --prune") ) +// ErrPostBackupPruneFailed wraps a failure of the post-backup prune, +// which runs only after every snapshot was created and uploaded, so the +// caller can report it apart from a failed backup. +var ErrPostBackupPruneFailed = errors.New("post-backup prune failed") + // listRecentLimit caps how many snapshot rows are fetched from the // local index for listing and purge operations. const listRecentLimit = 10000 @@ -52,6 +58,20 @@ func (v *Vaultik) CreateSnapshot(opts *SnapshotCreateOptions) error { return errNoAgeRecipients } + // Only the post-backup prune reads --keep-newer-than, after the + // backup; check it here so a bad value stops the run before anything + // is backed up. + if opts.KeepNewerThan != "" { + if !opts.Prune { + return errKeepNewerThanNeedsPrune + } + + _, err := parseDuration(opts.KeepNewerThan) + if err != nil { + return fmt.Errorf("--keep-newer-than: %w", err) + } + } + overallStartTime := time.Now() log.Info("Starting snapshot creation", @@ -122,7 +142,7 @@ func (v *Vaultik) CreateSnapshot(opts *SnapshotCreateOptions) error { if opts.Prune { err = v.runPostBackupPrune(snapshotNames, opts.KeepNewerThan) if err != nil { - return fmt.Errorf("post-backup prune: %w", err) + return fmt.Errorf("%w: %w", ErrPostBackupPruneFailed, err) } }