From 96ebcd40d7a64a810a96dc1bf6daf2426353193f Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 22 Sep 2026 12:11:49 +0200 Subject: [PATCH] Reconcile purge against remote by hashed key, not human ID (closes #160) syncWithRemote compared human snapshot IDs against the hashed metadata// directory names, which never match, so it deleted every local snapshot record; the purge that followed then found nothing to remove remotely. Reconcile via listAllRemoteSnapshotKeys and RemoteSnapshotKey(id), matching CleanupLocalSnapshots, so a row still backed by remote metadata is kept. The purge tests only passed because their stubs used the human-ID layout production never writes; they now write metadata under the hashed remote key. New tests prove remotely-backed local rows survive the reconcile and that a purge removes the local row and remote metadata together. Model: opus-4-8 --- .../purge_local_remote_consistency_test.go | 146 ++++++++++++++++++ internal/vaultik/purge_per_name_test.go | 8 +- internal/vaultik/snapshot.go | 40 ++--- 3 files changed, 169 insertions(+), 25 deletions(-) create mode 100644 internal/vaultik/purge_local_remote_consistency_test.go diff --git a/internal/vaultik/purge_local_remote_consistency_test.go b/internal/vaultik/purge_local_remote_consistency_test.go new file mode 100644 index 0000000..bf52b45 --- /dev/null +++ b/internal/vaultik/purge_local_remote_consistency_test.go @@ -0,0 +1,146 @@ +package vaultik_test + +import ( + "bytes" + "context" + "database/sql" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/vaultik/internal/database" + "sneak.berlin/go/vaultik/internal/log" + "sneak.berlin/go/vaultik/internal/snapshot" + "sneak.berlin/go/vaultik/internal/types" + "sneak.berlin/go/vaultik/internal/vaultik" +) + +// setupConsistencyTest builds a Vaultik whose local database and mock +// remote both hold the given snapshots. Remote metadata is stored under +// the production layout, metadata//manifest.json.zst. +// It returns the instance and the mock so a test can inspect the remote. +func setupConsistencyTest( + t *testing.T, snapshotIDs []string, +) (*vaultik.Vaultik, *MockStorer) { + t.Helper() + + ctx := context.Background() + db, err := database.New(ctx, ":memory:") + require.NoError(t, err) + t.Cleanup(func() { _ = db.Close() }) + + repos := database.NewRepositories(db) + mockStorage := NewMockStorer() + + for _, id := range snapshotIDs { + parts := strings.Split(id, "_") + startedAt, err := time.Parse(time.RFC3339, parts[len(parts)-1]) + require.NoError(t, err, "parsing timestamp from snapshot ID %q", id) + + completedAt := startedAt.Add(5 * time.Minute) + snap := &database.Snapshot{ + ID: types.SnapshotID(id), + Hostname: testHostname, + VaultikVersion: testLabel, + 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, "creating snapshot %s", id) + + metadataKey := "metadata/" + snapshot.RemoteSnapshotKey(id) + + "/manifest.json.zst" + err = mockStorage.Put(ctx, metadataKey, strings.NewReader("stub")) + require.NoError(t, err) + } + + v := &vaultik.Vaultik{ + Storage: mockStorage, + Repositories: repos, + DB: db, + Stdout: &bytes.Buffer{}, + Stderr: &bytes.Buffer{}, + Stdin: &bytes.Buffer{}, + } + v.SetContext(ctx) + + return v, mockStorage +} + +func remoteHasSnapshot(t *testing.T, m *MockStorer, id string) bool { + t.Helper() + + prefix := "metadata/" + snapshot.RemoteSnapshotKey(id) + "/" + keys, err := m.List(context.Background(), prefix) + require.NoError(t, err) + + return len(keys) > 0 +} + +// TestPurgeKeepsRemotelyBackedLocalRows guards against issue #160 +// (https://git.eeqj.de/sneak/vaultik/issues/160): purge reconciles local +// rows against the remote first, and that step compared human snapshot IDs +// against the hashed remote directory names, which never match — so it +// deleted every local record and the purge itself then removed nothing. +// +// With every snapshot still present remotely and nothing old enough to +// purge, all local rows must survive the reconcile untouched. +func TestPurgeKeepsRemotelyBackedLocalRows(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + ids := []string{snapHomeT0, snapHomeT1, snapSystemT0} + + v, _ := setupConsistencyTest(t, ids) + + err := v.PurgeSnapshotsWithOptions(&vaultik.SnapshotPurgeOptions{ + // 100 years: nothing is old enough to delete, so the reconcile + // is the only thing that touches the rows. + OlderThan: "36500d", + Force: true, + }) + require.NoError(t, err) + + remaining := listRemainingSnapshots(t, v) + assert.Len(t, remaining, len(ids), + "remotely-backed local rows must survive the reconcile") + assert.Contains(t, remaining, snapHomeT0) + assert.Contains(t, remaining, snapHomeT1) + assert.Contains(t, remaining, snapSystemT0) +} + +// TestPurgeRemovesLocalAndRemoteTogether proves the two halves stay +// consistent: a purged snapshot is gone both locally and remotely, while a +// retained one keeps both. Before the fix, the reconcile dropped every +// local row yet the remote metadata was left in place. +func TestPurgeRemovesLocalAndRemoteTogether(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + ids := []string{snapHomeT0, snapHomeT1, snapSystemT0} + + v, mock := setupConsistencyTest(t, ids) + + err := v.PurgeSnapshotsWithOptions(&vaultik.SnapshotPurgeOptions{ + KeepLatest: true, + Force: true, + }) + require.NoError(t, err) + + // Keep latest per name: newest home and the lone system are kept. + remaining := listRemainingSnapshots(t, v) + assert.ElementsMatch(t, []string{snapHomeT1, snapSystemT0}, remaining) + + // Local and remote agree: the older home snapshot is gone from both, + // the retained ones are present in both. + assert.False(t, remoteHasSnapshot(t, mock, snapHomeT0), + "purged snapshot must also be removed remotely") + assert.True(t, remoteHasSnapshot(t, mock, snapHomeT1), + "retained snapshot must remain remotely") + assert.True(t, remoteHasSnapshot(t, mock, snapSystemT0), + "retained snapshot must remain remotely") +} diff --git a/internal/vaultik/purge_per_name_test.go b/internal/vaultik/purge_per_name_test.go index 318f27c..cfc343b 100644 --- a/internal/vaultik/purge_per_name_test.go +++ b/internal/vaultik/purge_per_name_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/require" "sneak.berlin/go/vaultik/internal/database" "sneak.berlin/go/vaultik/internal/log" + "sneak.berlin/go/vaultik/internal/snapshot" "sneak.berlin/go/vaultik/internal/types" "sneak.berlin/go/vaultik/internal/vaultik" ) @@ -60,8 +61,11 @@ func setupPurgeTest(t *testing.T, snapshotIDs []string) *vaultik.Vaultik { }) require.NoError(t, err, "creating snapshot %s", id) - // Create remote metadata stub so syncWithRemote keeps it - metadataKey := "metadata/" + id + "/manifest.json.zst" + // Create the remote metadata stub under the production layout so + // syncWithRemote keeps the local row. Production stores metadata + // under the hashed remote key, not the human snapshot ID. + metadataKey := "metadata/" + snapshot.RemoteSnapshotKey(id) + + "/manifest.json.zst" err = mockStorage.Put(ctx, metadataKey, strings.NewReader("stub")) require.NoError(t, err) } diff --git a/internal/vaultik/snapshot.go b/internal/vaultik/snapshot.go index 406d9ad..0a24b78 100644 --- a/internal/vaultik/snapshot.go +++ b/internal/vaultik/snapshot.go @@ -935,29 +935,23 @@ func (v *Vaultik) downloadManifestByKey(remoteKey string) (*snapshot.Manifest, e func (v *Vaultik) syncWithRemote() error { log.Info("Syncing with remote snapshots") - // Get all remote snapshot IDs - remoteSnapshots := make(map[string]bool) - objectCh := v.Storage.ListStream(v.ctx, "metadata/") - - for object := range objectCh { - if object.Err != nil { - return fmt.Errorf("listing remote snapshots: %w", object.Err) - } - - // Extract snapshot ID from paths like metadata/hostname-20240115-143052Z/ - parts := strings.Split(object.Key, "/") - if len(parts) >= minSnapshotIDParts && - parts[0] == metadataDirName && parts[1] != "" { - // Skip macOS resource fork files (._*) and other hidden files - if strings.HasPrefix(parts[1], ".") { - continue - } - - remoteSnapshots[parts[1]] = true - } + // Remote metadata lives under metadata//, where the + // directory name is snapshot.RemoteSnapshotKey(id), not the human + // snapshot ID. Compare each local row's hashed key against that set + // so a row still backed by remote metadata is kept. Comparing human + // IDs against the hashed directory names matches nothing and deletes + // every local snapshot record (issue #160). + remoteKeys, err := v.listAllRemoteSnapshotKeys() + if err != nil { + return fmt.Errorf("listing remote snapshots: %w", err) } - log.Debug("Found remote snapshots", "count", len(remoteSnapshots)) + remoteKeySet := make(map[string]bool, len(remoteKeys)) + for _, k := range remoteKeys { + remoteKeySet[k] = true + } + + log.Debug("Found remote snapshots", "count", len(remoteKeySet)) // Get all local snapshots (use a high limit to get all) localSnapshots, err := v.Repositories.Snapshots.ListRecent(v.ctx, listRecentLimit) @@ -965,12 +959,12 @@ func (v *Vaultik) syncWithRemote() error { return fmt.Errorf("listing local snapshots: %w", err) } - // Remove local snapshots that don't exist remotely + // Remove local snapshots whose metadata is absent from the remote. removedCount := 0 for _, snap := range localSnapshots { snapshotIDStr := snap.ID.String() - if !remoteSnapshots[snapshotIDStr] { + if !remoteKeySet[snapshot.RemoteSnapshotKey(snapshotIDStr)] { log.Info("Removing local snapshot not found in remote", "snapshot_id", snap.ID)