Stop snapshot purge when a snapshot cannot leave the local index (closes #337)
check / check (push) Successful in 13m40s
check / check (push) Successful in 13m40s
snapshot purge, and the purge step of snapshot create --prune, logged a failed local index delete, then deleted that snapshot's metadata from the destination store anyway and exited 0, leaving a local record whose metadata was gone. It now stops at that snapshot with an error, before touching its metadata, so the command exits non-zero; snapshots already purged stay purged. This matches how the same loop already handles a failed metadata delete. Model: opus-5-5
This commit is contained in:
@@ -339,7 +339,9 @@ local index alone, and still exits zero.
|
|||||||
|
|
||||||
**`snapshot purge`**: Remove old snapshots based on criteria. Retention is
|
**`snapshot purge`**: Remove old snapshots based on criteria. Retention is
|
||||||
per-snapshot-name (`--keep-latest` keeps the latest of each name, not the
|
per-snapshot-name (`--keep-latest` keeps the latest of each name, not the
|
||||||
latest globally).
|
latest globally). If a snapshot cannot be removed from the local index, the
|
||||||
|
command stops there, leaves that snapshot's metadata on the destination store
|
||||||
|
and exits non-zero; `snapshot create --prune` fails the same way.
|
||||||
* `--keep-latest`: Keep only the most recent snapshot of each name
|
* `--keep-latest`: Keep only the most recent snapshot of each name
|
||||||
* `--older-than <duration>`: Remove snapshots older than duration (e.g. `30d`,
|
* `--older-than <duration>`: Remove snapshots older than duration (e.g. `30d`,
|
||||||
`4w`, `6mo`, `1y`; `m` is minutes, `mo` is months)
|
`4w`, `6mo`, `1y`; `m` is minutes, `mo` is months)
|
||||||
|
|||||||
@@ -0,0 +1,90 @@
|
|||||||
|
package cli //nolint:testpackage // shares runEntry and the hermetic config helpers
|
||||||
|
|
||||||
|
import (
|
||||||
|
"context"
|
||||||
|
"database/sql"
|
||||||
|
"os"
|
||||||
|
"path/filepath"
|
||||||
|
"testing"
|
||||||
|
"time"
|
||||||
|
|
||||||
|
"github.com/stretchr/testify/assert"
|
||||||
|
"github.com/stretchr/testify/require"
|
||||||
|
"sneak.berlin/go/vaultik/internal/database"
|
||||||
|
"sneak.berlin/go/vaultik/internal/snapshot"
|
||||||
|
"sneak.berlin/go/vaultik/internal/types"
|
||||||
|
)
|
||||||
|
|
||||||
|
// The `snapshot purge` command word and flags used below.
|
||||||
|
const (
|
||||||
|
cmdPurge = "purge"
|
||||||
|
flagKeepLatest = "--keep-latest"
|
||||||
|
flagForce = "--force"
|
||||||
|
)
|
||||||
|
|
||||||
|
// TestEntrySnapshotPurgeKeepsMetadataWhenLocalIndexDeleteFails makes
|
||||||
|
// removing a snapshot from the local index fail and checks that
|
||||||
|
// `snapshot purge` exits non-zero and leaves that snapshot's metadata on
|
||||||
|
// the destination store. It used to log the error, delete the metadata
|
||||||
|
// anyway and exit 0.
|
||||||
|
//
|
||||||
|
//nolint:paralleltest // replaces os.Args, os.Stdout, os.Stderr and the xdg globals
|
||||||
|
func TestEntrySnapshotPurgeKeepsMetadataWhenLocalIndexDeleteFails(t *testing.T) {
|
||||||
|
ctx := context.Background()
|
||||||
|
configPath := writeHermeticPruneConfig(t, false)
|
||||||
|
|
||||||
|
// writeHermeticPruneConfig puts the index and the store directory
|
||||||
|
// beside the config file.
|
||||||
|
dir := filepath.Dir(configPath)
|
||||||
|
|
||||||
|
// Two snapshots of the same name: --keep-latest purges the older.
|
||||||
|
olderStart := time.Date(2026, 5, 1, 9, 0, 0, 0, time.UTC)
|
||||||
|
newerStart := olderStart.Add(time.Hour)
|
||||||
|
olderID := "test-host_test_" + olderStart.Format(time.RFC3339)
|
||||||
|
newerID := "test-host_test_" + newerStart.Format(time.RFC3339)
|
||||||
|
|
||||||
|
db, err := database.New(ctx, filepath.Join(dir, "index.sqlite"))
|
||||||
|
require.NoError(t, err)
|
||||||
|
|
||||||
|
repos := database.NewRepositories(db)
|
||||||
|
|
||||||
|
manifestPaths := make(map[string]string)
|
||||||
|
|
||||||
|
for id, startedAt := range map[string]time.Time{
|
||||||
|
olderID: olderStart,
|
||||||
|
newerID: newerStart,
|
||||||
|
} {
|
||||||
|
completedAt := startedAt.Add(time.Minute)
|
||||||
|
snap := &database.Snapshot{
|
||||||
|
ID: types.SnapshotID(id),
|
||||||
|
Hostname: "test-host",
|
||||||
|
VaultikVersion: "test",
|
||||||
|
StartedAt: startedAt,
|
||||||
|
CompletedAt: &completedAt,
|
||||||
|
}
|
||||||
|
|
||||||
|
err = repos.WithTx(ctx, func(ctx context.Context, tx *sql.Tx) error {
|
||||||
|
return repos.Snapshots.Create(ctx, tx, snap)
|
||||||
|
})
|
||||||
|
require.NoError(t, err)
|
||||||
|
|
||||||
|
manifestPaths[id] = filepath.Join(dir, "store", "metadata",
|
||||||
|
snapshot.RemoteSnapshotKey(id), "manifest.json.zst")
|
||||||
|
require.NoError(t, os.MkdirAll(filepath.Dir(manifestPaths[id]), 0o750))
|
||||||
|
require.NoError(t, os.WriteFile(manifestPaths[id], []byte("stub"), 0o600))
|
||||||
|
}
|
||||||
|
|
||||||
|
_, err = db.Conn().ExecContext(ctx, `
|
||||||
|
CREATE TRIGGER refuse_snapshot_delete BEFORE DELETE ON snapshots
|
||||||
|
BEGIN SELECT RAISE(ABORT, 'simulated index error'); END`)
|
||||||
|
require.NoError(t, err)
|
||||||
|
require.NoError(t, db.Close())
|
||||||
|
|
||||||
|
code, _, stderr := runEntry(t,
|
||||||
|
flagConfig, configPath, cmdSnapshot, cmdPurge, flagKeepLatest, flagForce)
|
||||||
|
|
||||||
|
assert.Equal(t, 1, code)
|
||||||
|
assert.Contains(t, stderr, "simulated index error")
|
||||||
|
assert.FileExists(t, manifestPaths[olderID],
|
||||||
|
"a snapshot still in the local index keeps its metadata")
|
||||||
|
}
|
||||||
@@ -627,7 +627,8 @@ func selectSnapshotsToPurge(
|
|||||||
}
|
}
|
||||||
|
|
||||||
// confirmAndExecutePurge shows deletion candidates, confirms with the
|
// confirmAndExecutePurge shows deletion candidates, confirms with the
|
||||||
// user, and deletes snapshots.
|
// user, and deletes snapshots. It stops with an error at the first
|
||||||
|
// snapshot it cannot delete.
|
||||||
func (v *Vaultik) confirmAndExecutePurge(
|
func (v *Vaultik) confirmAndExecutePurge(
|
||||||
toDelete []SnapshotInfo, force, quiet bool,
|
toDelete []SnapshotInfo, force, quiet bool,
|
||||||
) error {
|
) error {
|
||||||
@@ -669,10 +670,11 @@ func (v *Vaultik) confirmAndExecutePurge(
|
|||||||
snapshotID := snap.ID.String()
|
snapshotID := snap.ID.String()
|
||||||
log.Info("Deleting snapshot", "id", snapshotID)
|
log.Info("Deleting snapshot", "id", snapshotID)
|
||||||
|
|
||||||
|
// Stop before the remote delete: a snapshot still in the local
|
||||||
|
// index must keep its metadata on the destination store.
|
||||||
err := v.deleteSnapshotFromLocalDB(snapshotID)
|
err := v.deleteSnapshotFromLocalDB(snapshotID)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
log.Error("Failed to delete from local database",
|
return fmt.Errorf("deleting snapshot %s from local index: %w", snapshotID, err)
|
||||||
"snapshot_id", snapshotID, "error", err)
|
|
||||||
}
|
}
|
||||||
|
|
||||||
_, err = v.deleteRemoteSnapshotByKey(snapshot.RemoteSnapshotKey(snapshotID))
|
_, err = v.deleteRemoteSnapshotByKey(snapshot.RemoteSnapshotKey(snapshotID))
|
||||||
|
|||||||
Reference in New Issue
Block a user