diff --git a/README.md b/README.md index 70f5eb4..72097c3 100644 --- a/README.md +++ b/README.md @@ -352,8 +352,9 @@ may hold snapshots this host doesn't know about), which is what prune` invocation to run as a follow-up. Local row cleanup (files, chunks, blobs the snapshot was the last referrer for) runs automatically. If the destination store is unreachable, the local-DB -removal still completes and a warning is emitted; rerun `vaultik prune` -once the store is reachable to finish remote cleanup. To wipe everything +removal still completes and a warning is emitted; run `vaultik snapshot +remove ` again once the store is reachable to remove the +snapshot's metadata from it (`vaultik prune` does not). To wipe everything on the destination in one go, use `vaultik remote nuke --force`. * `--local-only`: Skip remote cleanup; only touch the local index * `--dry-run`: Show what would be deleted without deleting diff --git a/TODO.md b/TODO.md index ec17c26..f280f3d 100644 --- a/TODO.md +++ b/TODO.md @@ -22,6 +22,17 @@ the tag exists and is exercised; what is left is merging `next` to # Completed Steps +- 2026-10-06: Made `snapshot remove --json` write only its document to + stdout when the destination store cannot be reached + ([issue #251](https://git.eeqj.de/sneak/vaultik/issues/251)). Its + warning that the snapshot's metadata was left on the destination store + went to stdout ahead of the document, breaking `| jq` on a command that + exited 0. Under `--json` the warning now reaches stderr only, through + the logger. The warning, the README and the command's help said + `vaultik prune` would finish the cleanup, but `prune` never removes + snapshot metadata; they now say to run `vaultik snapshot remove` for the + snapshot again once the destination store is reachable. + - 2026-10-06: Made the backup summary and the `snapshots` row count each file, byte and upload once ([issue #225](https://git.eeqj.de/sneak/vaultik/issues/225)). The diff --git a/internal/cli/entry_stdout_stderr_test.go b/internal/cli/entry_stdout_stderr_test.go index 43a6e42..1eda0d4 100644 --- a/internal/cli/entry_stdout_stderr_test.go +++ b/internal/cli/entry_stdout_stderr_test.go @@ -2,7 +2,9 @@ package cli //nolint:testpackage // shares hermeticConfig and the capture helper import ( "context" + "encoding/json" "fmt" + "log/slog" "os" "path/filepath" "strings" @@ -87,12 +89,45 @@ func TestEntryJSONFailureIsReportedOnStderr(t *testing.T) { } } -// writeUnusableDestinationConfig builds a config whose destination -// directory does not exist, which fails `remote info`, and whose local -// index is bound to another destination, which fails `prune` and -// `snapshot remove` (a missing destination alone only makes `snapshot -// remove` warn). Returns the config path. -func writeUnusableDestinationConfig(t *testing.T) string { +// TestEntrySnapshotRemoveJSONWarningIsOnStderr runs `snapshot remove +// --json` on a snapshot in the local index, against a destination +// directory that does not exist. The command removes the snapshot from +// the local index and still exits 0. Its stdout must hold the document +// alone, with the warning about the destination store on stderr: the +// command to run again once it is reachable, and the snapshot's ID in +// the record's snapshot_id field. +// +//nolint:paralleltest // replaces os.Args, os.Stdout, os.Stderr and the xdg globals +func TestEntrySnapshotRemoveJSONWarningIsOnStderr(t *testing.T) { + configPath, indexPath := writeMissingDestinationConfig(t) + seedStaleSnapshotRecord(t, indexPath) + + code, stdout, stderr := runEntry(t, flagConfig, configPath, + cmdSnapshot, cmdRemove, stalePruneSnapshotID, flagJSON) + + require.Equal(t, 0, code) + requireExactlyOneJSONDocument(t, stdout) + + // stderr is a pipe here, so the logger writes one JSON record a line. + var warning map[string]any + + for line := range strings.Lines(stderr) { + if strings.Contains(line, + "Could not remove snapshot metadata from remote storage") { + require.NoError(t, json.Unmarshal([]byte(line), &warning)) + } + } + + require.NotNil(t, warning, "the warning must reach stderr") + assert.Contains(t, warning[slog.MessageKey], + "run 'vaultik snapshot remove' with the snapshot's ID again") + assert.Equal(t, stalePruneSnapshotID, warning["snapshot_id"]) +} + +// writeMissingDestinationConfig builds a config whose destination +// directory does not exist. Returns the config path and the path of +// its local index, which is not created here. +func writeMissingDestinationConfig(t *testing.T) (string, string) { t.Helper() dir := t.TempDir() @@ -114,6 +149,19 @@ func writeUnusableDestinationConfig(t *testing.T) string { xdg.Reload() t.Cleanup(xdg.Reload) + return configPath, indexPath +} + +// writeUnusableDestinationConfig builds a config whose destination +// directory does not exist, which fails `remote info`, and whose local +// index is bound to another destination, which fails `prune` and +// `snapshot remove` (a missing destination alone only makes `snapshot +// remove` warn). Returns the config path. +func writeUnusableDestinationConfig(t *testing.T) string { + t.Helper() + + configPath, indexPath := writeMissingDestinationConfig(t) + ctx := context.Background() db, err := database.New(ctx, indexPath) @@ -122,7 +170,7 @@ func writeUnusableDestinationConfig(t *testing.T) string { defer func() { require.NoError(t, db.Close()) }() require.NoError(t, database.NewRepositories(db).LocalMeta.Set(ctx, - database.LocalMetaKeyStorageURL, "file://"+filepath.Join(dir, "other"))) + database.LocalMetaKeyStorageURL, "file://"+t.TempDir())) return configPath } diff --git a/internal/cli/snapshot.go b/internal/cli/snapshot.go index 4a0e095..fb2dc4d 100644 --- a/internal/cli/snapshot.go +++ b/internal/cli/snapshot.go @@ -258,8 +258,9 @@ Use --local-only to skip the remote half (e.g. when you want to forget a snapshot locally without touching the destination store). If the remote is unreachable, the local-database removal still completes -and a warning is emitted; rerun 'vaultik prune' once the destination store -is reachable to finish remote cleanup. +and a warning is emitted; run 'vaultik snapshot remove ' again +once the destination store is reachable to remove the snapshot's metadata +from it ('vaultik prune' does not). To wipe the entire destination store and start over, use 'vaultik remote nuke --force' — it is the single supported entry point for that.`, diff --git a/internal/vaultik/snapshot.go b/internal/vaultik/snapshot.go index 82a116e..7071bd4 100644 --- a/internal/vaultik/snapshot.go +++ b/internal/vaultik/snapshot.go @@ -1110,6 +1110,12 @@ type RemoveResult struct { // just-removed snapshot left behind on the destination store. const pruneCommandHint = "vaultik prune" +// snapshotRemoveCommandHint is the command suggested, with the +// snapshot's ID, when a remove could not reach the destination store: +// running it again removes the snapshot's metadata there, which +// `vaultik prune` never does. +const snapshotRemoveCommandHint = "vaultik snapshot remove" + // RemoveSnapshot removes a snapshot from the local index database and, // unless LocalOnly is set, also strips the snapshot's metadata from the // destination store. Blobs are NOT touched: removing a snapshot's @@ -1146,7 +1152,7 @@ func (v *Vaultik) RemoveSnapshot( } if !opts.LocalOnly { - result.RemoteRemoved = v.removeSnapshotRemote(snapshotID) + result.RemoteRemoved = v.removeSnapshotRemote(snapshotID, opts) } if v.SnapshotManager != nil { @@ -1229,9 +1235,11 @@ func (v *Vaultik) confirmRemoveSnapshot(snapshotID string, opts *RemoveOptions) // removeSnapshotRemote strips the snapshot's metadata from the // destination store, warning and proceeding on failure: the local-DB // removal has already happened, so the user is told the remote half -// didn't finish and can retry with `vaultik prune` once the destination -// store is reachable. Returns true when the remote removal succeeded. -func (v *Vaultik) removeSnapshotRemote(snapshotID string) bool { +// didn't finish and to run `vaultik snapshot remove` for the snapshot +// again once the destination store is reachable (`vaultik prune` never +// removes snapshot metadata). Returns true when the remote removal +// succeeded. +func (v *Vaultik) removeSnapshotRemote(snapshotID string, opts *RemoveOptions) bool { log.Info("Removing snapshot metadata from remote storage", "snapshot_id", snapshotID) @@ -1239,13 +1247,17 @@ func (v *Vaultik) removeSnapshotRemote(snapshotID string) bool { err := v.deleteRemoteSnapshotByKey(remoteKey) if err != nil { - log.Warn("Could not remove snapshot metadata from remote storage", - "error", err) + log.Warn("Could not remove snapshot metadata from remote storage; "+ + "run '"+snapshotRemoveCommandHint+"' with the snapshot's ID "+ + "again once the remote is reachable", + "snapshot_id", snapshotID, "error", err) - if v.UI != nil { + // The UI writes to stdout, which under --json holds only the + // document; the log record above is the warning on stderr. + if v.UI != nil && !opts.JSON { v.UI.Warningf("Could not remove snapshot metadata from remote: "+ - "%v. Run '%s' once the remote is reachable to finish cleanup.", - err, pruneCommandHint) + "%v. Run '%s %s' again once the remote is reachable.", + err, snapshotRemoveCommandHint, snapshotID) } return false