From be786fe25e59e7e6be72926224fe9648af3cb4b3 Mon Sep 17 00:00:00 2001 From: sneak Date: Sun, 9 Aug 2026 17:33:06 +0000 Subject: [PATCH] Gate prune's local-cleanup prose on --json and give make build a rule (closes #108) CleanupLocalSnapshots wrote three prose lines to stdout with no --json awareness, and they covered every branch of the function, so no input avoided them: `vaultik prune --json | jq` failed even after the banner fix. -q never helped either, because printlnStdout and stdoutf write straight to Vaultik.Stdout and never consult Vaultik.UI, which is what SetQuiet affects. The issue offered three fixes and asked for a decision. Taken: thread *PruneOptions into the function and gate each write on !opts.JSON, matching PruneBlobs -- its sibling phase, which already takes the same struct -- along with RemoveSnapshot and remote info, so the package has one pattern rather than two. Rejected: moving the lines to log.Info, because the logger's default level is slog.LevelWarn, so that would not relocate them to stderr, it would delete them from a plain `vaultik prune`, and the removal of rows from the local index is not something to narrate only under --verbose. Also rejected: putting the stale-record count into PruneBlobsResult, whose every field is blob-scoped and which is produced by the later phase; a prune document covering both phases is a reasonable thing to want, but that is a schema design question, not a stream-hygiene fix. The two events are duplicated as log.Info records, which PruneBlobs already does alongside its own prints, so they survive on stderr under --verbose. Also closes #110. `make build` printed "Nothing to be done for 'build'" and exited 0 without producing a binary: build was listed in .PHONY with no build: rule anywhere, and declaring a name phony is exactly what converts make's "No rule to make target" error into a silent success. Fixed with `build: vaultik`, keeping vaultik: as the file rule. Audited all 19 .PHONY names: build was the only one without a rule, and vaultik is correctly absent from .PHONY, being a real file target. Tests, each verified to fail with the fix reverted rather than assumed to. CleanupLocalSnapshots leaves stdout untouched under --json in all three branches (stale records, none, empty index) and still emits every line without it, so the guard cannot be satisfied by deleting the output. prune --json runs end to end through Entry, cobra and fx over the process's real stdout descriptor against a file:// store, asserting exactly one JSON document, in both the stale and non-stale branches. And a parse of the Makefile asserts every .PHONY name has a rule and that build reaches the rule producing the binary, which keeps the audit true for names added later; it is a parse rather than an invocation of make because `make test` is what runs it, so shelling back into `make build` would nest a build inside the test run. The property a parse cannot establish -- that the recipe still fails when the build fails -- was verified by hand against a deliberately broken tree: make build exits 2 and produces nothing. cmd/vaultik gains its first test file, so `make test` now reports 16 packages ok where it reported 15. flagConfig and programName constants are extracted in the CLI tests because the new argument vector pushed "--config" and "vaultik" over goconst's threshold. README's stdout/stderr section described the banner as "the other thing that writes to stdout", which this defect contradicted; it now states the contract that holds, which is that stdout under --json is the document and nothing else, for prune as well as for the other four. --- Makefile | 13 +- README.md | 11 +- TODO.md | 56 ++++++++ cmd/vaultik/makefile_test.go | 169 +++++++++++++++++++++++++ internal/cli/entry_banner_test.go | 13 +- internal/cli/entry_prune_json_test.go | 165 ++++++++++++++++++++++++ internal/vaultik/prune.go | 2 +- internal/vaultik/prune_cleanup_test.go | 134 ++++++++++++++++++++ internal/vaultik/snapshot.go | 23 +++- 9 files changed, 574 insertions(+), 12 deletions(-) create mode 100644 cmd/vaultik/makefile_test.go create mode 100644 internal/cli/entry_prune_json_test.go create mode 100644 internal/vaultik/prune_cleanup_test.go diff --git a/Makefile b/Makefile index b753313..45e4b5c 100644 --- a/Makefile +++ b/Makefile @@ -66,7 +66,18 @@ lint: lint-fix: @script/lint-fix -# Build binary. +# Build binary. `build` is the name the org convention reaches for and +# the one a caller checks the exit code of; `vaultik` is the file rule +# that does the work, so an unchanged tree still short-circuits. +# +# This alias is not decorative. `build` was listed in .PHONY with no +# rule, and a phony target with no prerequisites and no recipe is +# already satisfied: `make build` printed "Nothing to be done" and +# exited 0 without producing a binary (issue #110). Every name in +# .PHONY needs a rule for that reason; TestPhonyTargetsAllHaveRules in +# cmd/vaultik keeps it that way. +build: vaultik + vaultik: internal/*/*.go cmd/vaultik/*.go go build -ldflags "$(LDFLAGS)" -o $@ ./cmd/vaultik diff --git a/README.md b/README.md index 0f49813..cb7edda 100644 --- a/README.md +++ b/README.md @@ -139,10 +139,13 @@ Format follows the stream: when stderr is a terminal the records are colorized one-liners, and when it is redirected or piped they are JSON, one object per line. -The startup banner is the other thing that writes to stdout, and -`--json` suppresses it, as `--quiet` and `--cron` do. So -`vaultik snapshot list --json | jq .` works on its own, with no -additional flag to quiet the banner first. +Under `--json`, stdout holds the document and nothing else. The startup +banner is suppressed, as `--quiet` and `--cron` suppress it, and the +progress narration a command would otherwise print — such as the stale +local records `prune` reconciles away — is suppressed too, so it cannot +land ahead of the document. Every `--json` command therefore pipes on +its own, with no additional flag: `vaultik snapshot list --json | jq .` +and `vaultik prune --json | jq .` both work as written. ### environment variables diff --git a/TODO.md b/TODO.md index be8d2e4..85aad68 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,62 @@ release" is exactly the contradiction # Completed Steps +- 2026-08-09: Finished the `--json` stdout contract and gave `make build` + a rule ([issue #108](https://git.eeqj.de/sneak/vaultik/issues/108), + [issue #110](https://git.eeqj.de/sneak/vaultik/issues/110)). Two + unrelated defects of the same shape — a command reporting something it + did not do — landed together because both are small. + + `CleanupLocalSnapshots` wrote three prose lines to stdout with no + `--json` awareness, covering every branch of the function, so no input + avoided them and `vaultik prune --json | jq` failed even after + [issue #106](https://git.eeqj.de/sneak/vaultik/issues/106) removed the + banner. `-q` never helped either: `printlnStdout` and `stdoutf` write + straight to `Vaultik.Stdout` and never consult `Vaultik.UI`, which is + what `SetQuiet` affects. The issue offered three fixes and asked for a + decision. Taken: thread `*PruneOptions` into the function and gate each + write on `!opts.JSON`, matching `PruneBlobs` (its sibling phase, which + already takes the same struct), `RemoveSnapshot` and `remote info`, so + the package has one pattern rather than two. Rejected: moving the lines + to `log.Info`, because the logger's default level is `slog.LevelWarn`, + so that would not relocate them to stderr — it would delete them from a + plain `vaultik prune`, and the removal of rows from the local index is + not something to narrate only under `--verbose`. Also rejected: putting + the stale-record count into `PruneBlobsResult`, whose every field is + blob-scoped and which is produced by the later phase; a prune document + covering both phases is a reasonable thing to want, but it is a schema + design question and not a stream-hygiene fix. The narration is + duplicated as `log.Info` records, which `PruneBlobs` already does + alongside its own prints, so the events survive on stderr for anyone + running `--verbose`. + + `make build` printed "Nothing to be done for 'build'" and exited 0 + without producing a binary: `build` was listed in `.PHONY` with no + `build:` rule anywhere, and declaring a name phony is exactly what + converts make's "No rule to make target" error into a silent success. + Fixed with `build: vaultik`, keeping `vaultik:` as the file rule. The + audit the issue asked for covers all 19 `.PHONY` names; `build` was the + only one without a rule, and `vaultik` is correctly absent from + `.PHONY`, being a real file target. + + Tests, each verified to fail with the fix reverted rather than assumed + to: `CleanupLocalSnapshots` leaves stdout untouched under `--json` in + all three branches (stale records, none, empty index) and still emits + every line without it, so the guard cannot be satisfied by deleting the + output; `prune --json` run end to end through `Entry`, cobra and fx + over the process's real stdout descriptor against a `file://` store, + asserting exactly one JSON document, in both the stale and non-stale + branches; and a parse of the `Makefile` asserting every `.PHONY` name + has a rule and that `build` reaches the rule that produces the binary, + which keeps the audit true for names added later. That last one is a + parse rather than an invocation of `make`, since `make test` is what + runs it and shelling back into `make build` would nest a build inside + the test run. The property a parse cannot establish — that the recipe + still fails when the build fails — was verified by hand against a + deliberately broken tree: `make build` exits 2 and produces nothing. + `cmd/vaultik` gains its first test file, so `make test` now reports 16 + packages `ok` where it reported 15. + - 2026-08-09: Stopped the startup banner from contaminating `--json` documents ([issue #106](https://git.eeqj.de/sneak/vaultik/issues/106)). `Entry` writes the banner to stdout before cobra parses anything, and diff --git a/cmd/vaultik/makefile_test.go b/cmd/vaultik/makefile_test.go new file mode 100644 index 0000000..a577a27 --- /dev/null +++ b/cmd/vaultik/makefile_test.go @@ -0,0 +1,169 @@ +package main_test + +import ( + "os" + "path/filepath" + "regexp" + "slices" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// This file guards the Makefile that builds this program, which is why +// it lives beside it rather than in a package of its own. +// +// Issue #110: `build` was listed in .PHONY with no `build:` rule +// anywhere in the file. That combination is silently successful — make +// considers a phony target with no prerequisites and no recipe already +// satisfied, so `rm -f vaultik && make build` printed "Nothing to be +// done for 'build'" and exited 0 with no binary produced. Declaring the +// name phony is precisely what converts the "No rule to make target" +// error into a green. +// +// The guard is a parse of the Makefile rather than an invocation of +// make. `make test` is what runs these tests, so shelling back into +// `make build` here would nest a build inside the test run and drop a +// binary into the tree as a side effect of testing. The one property a +// parse cannot establish — that the recipe still fails when the build +// fails — is not testable from inside the build either; it is verified +// by hand against a deliberately broken tree. + +// phonyDirective introduces the list of phony target names. +const phonyDirective = ".PHONY:" + +// ruleLine matches a rule's target list: a target starts in column +// zero, so recipe lines (tab-indented) and the continuation lines of a +// variable assignment (space-indented) are excluded by construction. +// +// The trailing (?:[^=]|$) rejects `:=` assignments such as +// `VERSION := $(shell script/version)`, which are not rules. Directives +// and function calls (`.PHONY:`, `ifeq`, `$(error ...)`) do not match +// because a target here must begin with a letter, digit or underscore. +var ruleLine = regexp.MustCompile(`^([A-Za-z0-9_][A-Za-z0-9_./ -]*):(?:[^=]|$)`) + +// TestPhonyTargetsAllHaveRules fails on any name in .PHONY that has no +// rule in the Makefile. Such a name is not a build target at all: it is +// a command that reports success without doing anything, which is worse +// than one that does not exist, because a caller checking the exit code +// cannot tell the difference. +func TestPhonyTargetsAllHaveRules(t *testing.T) { + t.Parallel() + + makefile := readMakefile(t) + + phony := phonyTargets(makefile) + require.NotEmpty(t, phony, "no .PHONY names found; the parser is broken") + + rules := declaredRules(makefile) + + // Sanity check on the rule parser before trusting its verdict: a + // parser that found nothing would pass this test by accident. + require.Contains(t, rules, "vaultik", + "the file rule that builds the binary must be recognized") + + for _, target := range phony { + assert.Contains(t, rules, target, + "`.PHONY` lists %q but the Makefile declares no %q rule, so "+ + "`make %s` exits 0 without doing anything", target, target, target) + } +} + +// TestBuildTargetBuildsTheBinary pins the specific shape of issue #110: +// `make build` has to reach the rule that produces the binary. The test +// above would also pass if `build:` were given an empty recipe of its +// own, which would be the same silent success under a different +// spelling. +func TestBuildTargetBuildsTheBinary(t *testing.T) { + t.Parallel() + + prerequisites := rulePrerequisites(readMakefile(t), "build") + require.NotNil(t, prerequisites, "the Makefile declares no `build` rule") + + assert.Contains(t, prerequisites, "vaultik", + "`make build` must depend on the rule that builds the binary") +} + +// readMakefile returns the contents of the repository's Makefile. The +// test binary runs with its package directory as the working directory, +// so the root is found by walking up until the Makefile appears. +func readMakefile(t *testing.T) string { + t.Helper() + + dir, err := os.Getwd() + require.NoError(t, err) + + for { + //nolint:gosec // G304: the path is this test's own directory walk + contents, err := os.ReadFile(filepath.Join(dir, "Makefile")) + if err == nil { + return string(contents) + } + + parent := filepath.Dir(dir) + require.NotEqual(t, dir, parent, + "walked to the filesystem root without finding a Makefile") + + dir = parent + } +} + +// phonyTargets returns every name declared phony, across all .PHONY +// lines. +func phonyTargets(makefile string) []string { + var targets []string + + for line := range strings.SplitSeq(makefile, "\n") { + if !strings.HasPrefix(line, phonyDirective) { + continue + } + + targets = append(targets, + strings.Fields(strings.TrimPrefix(line, phonyDirective))...) + } + + return targets +} + +// declaredRules returns the set of target names that have a rule. +func declaredRules(makefile string) map[string]bool { + rules := make(map[string]bool) + + for line := range strings.SplitSeq(makefile, "\n") { + match := ruleLine.FindStringSubmatch(line) + if match == nil { + continue + } + + // One rule may name several targets: `a b: prereq`. + for target := range strings.FieldsSeq(match[1]) { + rules[target] = true + } + } + + return rules +} + +// rulePrerequisites returns the prerequisites of the named rule, or nil +// if no such rule exists. A rule with none returns an empty slice, so +// "declared with nothing to do" is distinguishable from "not declared". +func rulePrerequisites(makefile, target string) []string { + for line := range strings.SplitSeq(makefile, "\n") { + match := ruleLine.FindStringSubmatch(line) + if match == nil { + continue + } + + if !slices.Contains(strings.Fields(match[1]), target) { + continue + } + + _, after, _ := strings.Cut(line, ":") + + return append([]string{}, strings.Fields(after)...) + } + + return nil +} diff --git a/internal/cli/entry_banner_test.go b/internal/cli/entry_banner_test.go index fd1ab6e..1ffad8a 100644 --- a/internal/cli/entry_banner_test.go +++ b/internal/cli/entry_banner_test.go @@ -28,8 +28,13 @@ const ( cmdRemote = "remote" cmdInfo = "info" - flagJSON = "--json" - flagQuiet = "--quiet" + flagJSON = "--json" + flagQuiet = "--quiet" + flagConfig = "--config" + + // programName is argv[0] as the real process receives it. Entry + // strips it before scanning, so it has to be present. + programName = "vaultik" // someSnapshotID is any snapshot identifier: these tests never run // the command, so it only has to occupy the positional argument. @@ -65,7 +70,7 @@ var jsonArgumentVectors = map[string][]string{ // A --json invocation that also carries a flag with a value, so the // scan cannot be fooled by an argument that consumes the next one. "json with config": { - "--config", "/nonexistent/vaultik.yml", cmdSnapshot, cmdList, flagJSON, + flagConfig, "/nonexistent/vaultik.yml", cmdSnapshot, cmdList, flagJSON, }, } @@ -222,7 +227,7 @@ func TestEntryJSONStdoutIsExactlyOneDocument(t *testing.T) { }) os.Args = []string{ - "vaultik", "--config", configPath, cmdSnapshot, cmdList, flagJSON, + programName, flagConfig, configPath, cmdSnapshot, cmdList, flagJSON, } stdout := captureProcessStdout(t, Entry) diff --git a/internal/cli/entry_prune_json_test.go b/internal/cli/entry_prune_json_test.go new file mode 100644 index 0000000..15b9cf8 --- /dev/null +++ b/internal/cli/entry_prune_json_test.go @@ -0,0 +1,165 @@ +package cli //nolint:testpackage // shares hermeticConfig and the capture helpers + +import ( + "context" + "database/sql" + "encoding/json" + "fmt" + "os" + "path/filepath" + "testing" + "time" + + "github.com/adrg/xdg" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/vaultik/internal/database" + "sneak.berlin/go/vaultik/internal/types" +) + +// pruneJSONDocument is the shape `prune --json` writes: the +// PruneBlobsResult document, and nothing else. +// +//nolint:tagliatelle // snake_case is the established JSON output format +type pruneJSONDocument struct { + BlobsFound int `json:"blobs_found"` + BlobsDeleted int `json:"blobs_deleted"` + BytesFreed int64 `json:"bytes_freed"` +} + +// stalePruneSnapshotID is seeded into the local index with no manifest +// on the destination store, which is exactly what makes it stale. +const stalePruneSnapshotID = "test-host_test_2026-04-01T09:00:00Z" + +// TestEntryPruneJSONStdoutIsExactlyOneDocument is the end-to-end +// regression guard for issue #108: `vaultik prune --json | jq .` must +// work with no other flags. +// +// It runs Entry over the process's real stdout descriptor, through +// cobra and the fx graph, against a hermetic file:// destination store +// — the same construction TestEntryJSONStdoutIsExactlyOneDocument uses +// for `snapshot list`, with the pipe to jq replaced by a decoder. +// +// Both branches of the local-snapshot reconciliation are exercised +// because the three stdout writes that broke this covered all of them: +// one line per stale record and a summary when there were any, and a +// "No stale local snapshots found." line when there were none. No input +// avoided the contamination, so no single branch demonstrates the fix. +// +// Not parallel: it replaces os.Args, os.Stdout and the xdg globals. +// +//nolint:paralleltest // replaces os.Args, os.Stdout and the xdg globals +func TestEntryPruneJSONStdoutIsExactlyOneDocument(t *testing.T) { + for _, testCase := range []struct { + name string + seedStale bool + description string + }{ + { + name: "no stale local records", + seedStale: false, + description: "the empty-index branch used to print a 'No stale' line", + }, + { + name: "stale local records present", + seedStale: true, + description: "the removal branch used to print a line per record " + + "plus a summary", + }, + } { + t.Run(testCase.name, func(t *testing.T) { + configPath := writeHermeticPruneConfig(t, testCase.seedStale) + + previousArgs := os.Args + + t.Cleanup(func() { + os.Args = previousArgs + rootFlags = RootFlags{} + }) + + os.Args = []string{ + programName, flagConfig, configPath, cmdPrune, flagJSON, + } + + stdout := captureProcessStdout(t, Entry) + + requireExactlyOneJSONDocument(t, stdout) + + var document pruneJSONDocument + + require.NoError(t, json.Unmarshal([]byte(stdout), &document), + testCase.description) + + // A destination store with no blobs has none to prune. The + // assertion that matters is the one above; this one keeps the + // test honest about which document it decoded. + assert.Equal(t, 0, document.BlobsFound) + }) + } +} + +// writeHermeticPruneConfig builds a config over a temp directory and, if +// seedStale is set, creates the index database up front with one +// snapshot record that has no counterpart on the destination store. +// Returns the config path. +func writeHermeticPruneConfig(t *testing.T, seedStale bool) string { + t.Helper() + + dir := t.TempDir() + configPath := filepath.Join(dir, "config.yml") + indexPath := filepath.Join(dir, "index.sqlite") + + contents := fmt.Sprintf(hermeticConfig, + filepath.Join(dir, "source"), + filepath.Join(dir, "store"), + indexPath) + + require.NoError(t, + os.WriteFile(configPath, []byte(contents), configFileMode)) + + // The PID lock lives under xdg.DataHome, which xdg resolves at + // package init; point it at the temp dir so the test neither + // touches nor collides with the real one. + t.Setenv("XDG_DATA_HOME", filepath.Join(dir, "data")) + xdg.Reload() + t.Cleanup(xdg.Reload) + + if seedStale { + seedStaleSnapshotRecord(t, indexPath) + } + + return configPath +} + +// seedStaleSnapshotRecord creates the index database at path and +// inserts one completed snapshot into it. Nothing is written to the +// destination store, so `prune` finds the record stale and removes it — +// the branch that printed a line per record. +func seedStaleSnapshotRecord(t *testing.T, path string) { + t.Helper() + + ctx := context.Background() + + db, err := database.New(ctx, path) + require.NoError(t, err) + + defer func() { require.NoError(t, db.Close()) }() + + startedAt := time.Date(2026, 4, 1, 9, 0, 0, 0, time.UTC) + completedAt := startedAt.Add(time.Minute) + + snap := &database.Snapshot{ + ID: types.SnapshotID(stalePruneSnapshotID), + Hostname: "test-host", + VaultikVersion: "test", + StartedAt: startedAt, + CompletedAt: &completedAt, + } + + repos := database.NewRepositories(db) + + err = repos.WithTx(ctx, func(ctx context.Context, tx *sql.Tx) error { + return repos.Snapshots.Create(ctx, tx, snap) + }) + require.NoError(t, err) +} diff --git a/internal/vaultik/prune.go b/internal/vaultik/prune.go index 328b0b2..490f7dd 100644 --- a/internal/vaultik/prune.go +++ b/internal/vaultik/prune.go @@ -79,7 +79,7 @@ func (v *Vaultik) Prune(opts *PruneOptions) error { // store is treated as gone. This used to be the separate 'snapshot // cleanup' command and is now folded in so a single 'vaultik prune' // gets the local index fully back in sync with the destination. - err = v.CleanupLocalSnapshots() + err = v.CleanupLocalSnapshots(opts) if err != nil { return fmt.Errorf("reconciling local snapshots with remote: %w", err) } diff --git a/internal/vaultik/prune_cleanup_test.go b/internal/vaultik/prune_cleanup_test.go new file mode 100644 index 0000000..593f738 --- /dev/null +++ b/internal/vaultik/prune_cleanup_test.go @@ -0,0 +1,134 @@ +package vaultik_test + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/vaultik/internal/log" + "sneak.berlin/go/vaultik/internal/vaultik" +) + +// cleanupStaleID is a local snapshot record with no remote manifest — +// the record CleanupLocalSnapshots exists to remove. +const cleanupStaleID = "testhost_home_2026-04-01T09:00:00Z" + +// remainingSnapshotLimit bounds the post-cleanup listing. ListRecent +// takes a SQL LIMIT, so it must be positive; the fixtures never exceed +// a handful of rows. +const remainingSnapshotLimit = 100 + +// cleanupStart is the fixture snapshot's start time. Its exact value is +// irrelevant; only presence in the index matters here. +// +//nolint:gochecknoglobals // read-only fixture shared by the tests below +var cleanupStart = time.Date(2026, 4, 1, 9, 0, 0, 0, time.UTC) + +// TestCleanupLocalSnapshots_JSONWritesNothingToStdout is the regression +// guard for issue #108: `vaultik prune --json | jq` failed because this +// function wrote prose to stdout on every branch, ahead of the +// PruneBlobsResult document, with no --json awareness at all. +// +// Both branches are covered because the three writes between them left +// no input that avoided the contamination: with stale records there was +// a line per record plus a summary, and with none there was still the +// "No stale local snapshots found." line. +func TestCleanupLocalSnapshots_JSONWritesNothingToStdout(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + for name, seed := range map[string]func(*listEnv){ + "no stale records": func(env *listEnv) { + // A snapshot present both locally and remotely: nothing to + // remove, which used to print the "No stale" line. + env.addLocal(t, listLocalID, cleanupStart) + env.addRemote(t, listLocalID, cleanupStart) + }, + "stale records present": func(env *listEnv) { + env.addLocal(t, cleanupStaleID, cleanupStart) + }, + "nothing at all": func(_ *listEnv) {}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + + env := newListEnv(t) + seed(env) + + err := env.v.CleanupLocalSnapshots(&vaultik.PruneOptions{JSON: true}) + require.NoError(t, err) + + assert.Empty(t, env.stdout.String(), + "stdout carries the --json document and nothing else") + }) + } +} + +// TestCleanupLocalSnapshots_HumanOutputRetained pins the other half of +// the contract. Without it the test above would be satisfied by +// deleting the three lines outright, and a `vaultik prune` with no +// flags must still say that it removed records from the local index — +// that is the deletion of local state, not decoration. +func TestCleanupLocalSnapshots_HumanOutputRetained(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + t.Run("stale records present", func(t *testing.T) { + t.Parallel() + + env := newListEnv(t) + env.addLocal(t, cleanupStaleID, cleanupStart) + + err := env.v.CleanupLocalSnapshots(&vaultik.PruneOptions{}) + require.NoError(t, err) + + out := env.stdout.String() + + assert.Contains(t, out, "Removing stale local record: "+cleanupStaleID) + assert.Contains(t, out, "Removed 1 stale local snapshot record(s).") + }) + + t.Run("no stale records", func(t *testing.T) { + t.Parallel() + + env := newListEnv(t) + env.addLocal(t, listLocalID, cleanupStart) + env.addRemote(t, listLocalID, cleanupStart) + + err := env.v.CleanupLocalSnapshots(&vaultik.PruneOptions{}) + require.NoError(t, err) + + assert.Contains(t, env.stdout.String(), + "No stale local snapshots found.") + }) +} + +// TestCleanupLocalSnapshots_RemovesOnlyStaleRecords checks that the +// --json gate did not change what the function does, only what it +// says: the stale record is gone from the index and the one with a +// remote manifest is untouched. +func TestCleanupLocalSnapshots_RemovesOnlyStaleRecords(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + env := newListEnv(t) + env.addLocal(t, listLocalID, cleanupStart) + env.addRemote(t, listLocalID, cleanupStart) + env.addLocal(t, cleanupStaleID, cleanupStart) + + err := env.v.CleanupLocalSnapshots(&vaultik.PruneOptions{JSON: true}) + require.NoError(t, err) + + remaining, err := env.v.Repositories.Snapshots.ListRecent( + env.v.Context(), remainingSnapshotLimit) + require.NoError(t, err) + + ids := make([]string, 0, len(remaining)) + for _, snap := range remaining { + ids = append(ids, snap.ID.String()) + } + + assert.Equal(t, []string{listLocalID}, ids, + "only the record with no remote manifest may be removed") +} diff --git a/internal/vaultik/snapshot.go b/internal/vaultik/snapshot.go index 32424b6..16bba39 100644 --- a/internal/vaultik/snapshot.go +++ b/internal/vaultik/snapshot.go @@ -829,7 +829,15 @@ func (v *Vaultik) outputVerifyJSON(result *VerifyResult) error { // behind by incomplete or interrupted backups. Each local snapshot's // human ID is hashed via RemoteSnapshotKey and compared against the // remote listing. -func (v *Vaultik) CleanupLocalSnapshots() error { +// +// It takes the whole *PruneOptions, symmetric with PruneBlobs, because +// it is the other half of one command: Prune runs this phase and then +// that one. Only JSON is read here. Under --json every write below is +// suppressed, because stdout carries the PruneBlobsResult document and +// nothing else — prose ahead of it is what made `vaultik prune --json | +// jq` fail (issue #108). The narration is duplicated as log records, +// which go to stderr and so cannot corrupt the document. +func (v *Vaultik) CleanupLocalSnapshots(opts *PruneOptions) error { err := v.EnsureStorageBinding() if err != nil { return err @@ -855,7 +863,11 @@ func (v *Vaultik) CleanupLocalSnapshots() error { for _, snap := range localSnapshots { id := snap.ID.String() if !remoteSet[snapshot.RemoteSnapshotKey(id)] { - v.stdoutf("Removing stale local record: %s\n", id) + log.Info("Removing stale local snapshot record", "snapshot_id", id) + + if !opts.JSON { + v.stdoutf("Removing stale local record: %s\n", id) + } err = v.deleteSnapshotFromLocalDB(id) if err != nil { @@ -869,6 +881,13 @@ func (v *Vaultik) CleanupLocalSnapshots() error { } } + log.Info("Reconciled local snapshot records against remote metadata", + "removed", removed, "examined", len(localSnapshots)) + + if opts.JSON { + return nil + } + if removed == 0 { v.printlnStdout("No stale local snapshots found.") } else { -- 2.49.1