From 095292545374db480584117cd779633ba8c1bdbe Mon Sep 17 00:00:00 2001 From: sneak Date: Sun, 9 Aug 2026 03:15:36 +0000 Subject: [PATCH 1/3] Route every remote manifest read through downloadManifestByKey internal/vaultik/verify.go and internal/vaultik/info.go each built the metadata//manifest.json.zst path and decoded the manifest inline, duplicating downloadManifestByKey. Both now call the helper. The manifest is currently stored compressed but unencrypted, which is what will let `snapshot list` enumerate the destination store on a host holding no private key. Whether to encrypt it is still open (#81), and a single reader means that decision has one call site to change rather than four. Refs #81 --- internal/vaultik/info.go | 19 +++++-------------- internal/vaultik/verify.go | 17 ++++------------- 2 files changed, 9 insertions(+), 27 deletions(-) diff --git a/internal/vaultik/info.go b/internal/vaultik/info.go index 59d41fc..0d9476b 100644 --- a/internal/vaultik/info.go +++ b/internal/vaultik/info.go @@ -9,7 +9,6 @@ import ( "github.com/dustin/go-humanize" "sneak.berlin/go/vaultik/internal/log" - "sneak.berlin/go/vaultik/internal/snapshot" ) // ShowInfo displays system and configuration information @@ -312,20 +311,12 @@ func (v *Vaultik) collectReferencedBlobsFromManifests( referencedBlobs := make(map[string]int64) for _, snapshotID := range snapshotIDs { - manifestKey := fmt.Sprintf("metadata/%s/manifest.json.zst", snapshotID) - - reader, err := v.Storage.Get(v.ctx, manifestKey) + // snapshotIDs here are remote keys, taken straight from the + // metadata/ listing. downloadManifestByKey is the single reader + // for remote manifests; see its doc comment. + manifest, err := v.downloadManifestByKey(snapshotID) if err != nil { - log.Warn("Failed to get manifest", "snapshot", snapshotID, "error", err) - - continue - } - - manifest, err := snapshot.DecodeManifest(reader) - _ = reader.Close() - - if err != nil { - log.Warn("Failed to decode manifest", "snapshot", snapshotID, "error", err) + log.Warn("Failed to read manifest", "snapshot", snapshotID, "error", err) continue } diff --git a/internal/vaultik/verify.go b/internal/vaultik/verify.go index 07d1e01..38be849 100644 --- a/internal/vaultik/verify.go +++ b/internal/vaultik/verify.go @@ -141,30 +141,21 @@ func (v *Vaultik) loadVerificationData( // All remote paths use the hashed key derived from the human ID. remoteKey := snapshot.RemoteSnapshotKey(snapshotID) - // Download manifest - manifestPath := fmt.Sprintf("metadata/%s/manifest.json.zst", remoteKey) - log.Info("Downloading manifest", "path", manifestPath) + // Download manifest. downloadManifestByKey is the single reader for + // remote manifests; see its doc comment. + log.Info("Downloading manifest", "remote_key", remoteKey) if !opts.JSON { v.stdoutf("Downloading manifest...\n") } - manifestReader, err := v.Storage.Get(v.ctx, manifestPath) + manifest, err := v.downloadManifestByKey(remoteKey) if err != nil { return nil, nil, nil, v.deepVerifyFailure(result, opts, fmt.Sprintf("failed to download manifest: %v", err), fmt.Errorf("failed to download manifest: %w", err)) } - defer func() { _ = manifestReader.Close() }() - - manifest, err := snapshot.DecodeManifest(manifestReader) - if err != nil { - return nil, nil, nil, v.deepVerifyFailure(result, opts, - fmt.Sprintf("failed to decode manifest: %v", err), - fmt.Errorf("failed to decode manifest: %w", err)) - } - log.Info("Manifest loaded", "manifest_blob_count", manifest.BlobCount, "manifest_total_size", ubytes(manifest.TotalCompressedSize)) -- 2.49.1 From 0e2929d75de414d44658dd74aa6d3cffc1a20c2f Mon Sep 17 00:00:00 2001 From: sneak Date: Sun, 9 Aug 2026 03:15:52 +0000 Subject: [PATCH 2/3] List remote snapshots without requiring the private key (closes #64) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ListSnapshots built its table entirely from the local SQLite index. The only remote access, reportRemoteDrift, was gated on AgeSecretKey being non-empty — so on a correctly configured host, which by design holds no private key, `snapshot list` never contacted the destination store at all. A user who lost their local index could not see their own backups, and the "" cell the README documents was unreachable dead code. The listing is now the union of the local index and the destination store, with no age_secret_key gate. The manifest is unencrypted, so a host holding only the public key can enumerate what it has backed up: one streamed listing of the metadata/ prefix, then a manifest read per remote key the local index does not already account for, bounded by maxRemoteOnlyRows and run with bounded concurrency. A remote-only snapshot's hostname and name are deliberately NOT recovered. RemoteSnapshotKey is one-way and the manifest stores the hash rather than the human ID, so they are recoverable only from the encrypted per-snapshot database; making them readable from remote storage would undo a deliberate privacy property (#81). Such rows are labelled ">", carry the real timestamp and compressed size from the manifest, and show "" in the two columns that genuinely require the local index. --json carries the full 64-character key in remote_key, and remote_present distinguishes seen (true), missing (false) and not-listable (null). Local records with no counterpart on the destination store are still surfaced as drift, and the remediation hint now names `vaultik prune`, which exists, rather than `vaultik snapshot cleanup`, which does not: CleanupLocalSnapshots is already wired as prune's first pass, and re-adding a second entry point would undo the CLI consolidation. reportRemoteDrift collapses. Its remote-only half is subsumed by the table — those snapshots are rows now, not a footnote count — and its local-only half reads the merge ListSnapshots already computed, so the command lists the destination exactly once per invocation. An unreachable destination stays a warning plus local-only output and a zero exit code, as the doc comment always claimed. In --json mode that warning goes to stderr, because the logger and the UI writer both emit on stdout and would otherwise corrupt the document. Tests cover remote-only rendering, the no-private-key property (a storer that counts prefix listings and records fetched keys, asserting the destination is read and nothing encrypted is touched), graceful degradation on an unreachable destination in both output modes, local-only drift, and an unreadable manifest not hiding other snapshots. --- README.md | 40 +- TODO.md | 19 + internal/vaultik/helpers.go | 24 +- internal/vaultik/snapshot.go | 247 +----------- internal/vaultik/snapshot_list.go | 486 ++++++++++++++++++++++ internal/vaultik/snapshot_list_test.go | 533 +++++++++++++++++++++++++ 6 files changed, 1101 insertions(+), 248 deletions(-) create mode 100644 internal/vaultik/snapshot_list.go create mode 100644 internal/vaultik/snapshot_list_test.go diff --git a/README.md b/README.md index a65786b..319f01d 100644 --- a/README.md +++ b/README.md @@ -175,12 +175,40 @@ needed. * `--keep-newer-than `: With `--prune`, keep snapshots newer than this duration instead of only the latest (e.g. `4w`, `30d`, `6mo`, `1y`) -**`snapshot list`**: Show every snapshot known to the destination -store with timestamps and three sizes per snapshot (compressed -remote size; total uncompressed chunk size; size of chunks newly -referenced by that snapshot). The uncompressed and "new chunk" -columns show `` for snapshots not in the local index. -* `--json`: Output in JSON format +**`snapshot list`**: Show every snapshot known to this host — the union +of the local index and the backup destination store — with timestamps +and three sizes per snapshot (compressed remote size; total +uncompressed chunk size; size of chunks newly referenced by that +snapshot). + +Listing the destination store does **not** require the age secret key, +so it works in vaultik's intended configuration, where the backed-up +host holds only the public key. A host that has lost its local index +can still see what it has backed up. + +What that host cannot see is a remote-only snapshot's name. The +snapshot ID is hashed at the storage boundary and the manifest records +only the hash, so hostname and snapshot name exist solely in the local +index and in the encrypted per-snapshot database. Snapshots found only +on the destination store are therefore listed as +`>` and show `` in +the uncompressed and "new chunk" columns, which can only be computed +from the local index. Their timestamp and compressed size are real, +read from the manifest. + +Snapshots in the local index with no counterpart on the destination +store are reported below the table as drift, with the `vaultik prune` +invocation that reconciles them. + +If the destination store cannot be listed (unmounted volume, +permission denied, network down), the command warns, falls back to the +local index alone, and still exits zero. +* `--json`: Output in JSON format. Each entry carries `locally_tracked` + (whether the snapshot is in the local index), `remote_key` (the full + 64-character storage key), and `remote_present` (whether it was seen + on the destination store, or `null` if the destination could not be + listed). The warning about an unlistable destination goes to stderr + so stdout stays a single parseable document. **`snapshot verify`**: Verify snapshot integrity. * Default (shallow): checks that all blobs referenced in the manifest exist in storage diff --git a/TODO.md b/TODO.md index 723fa4d..3a362c5 100644 --- a/TODO.md +++ b/TODO.md @@ -19,6 +19,25 @@ or delete the branch. # Completed Steps +- 2026-08-09: Made `snapshot list` list the destination store without + the private key (issue #64). The listing is now the union of the + local index and a single streamed listing of the `metadata/` prefix, + with no `age_secret_key` gate — the manifest is unencrypted, so a + host holding only the public key can enumerate its own backups and a + host that lost its local index can still see them. A remote-only + snapshot's hostname and name are deliberately not recovered (they are + not recoverable without the private key, and making them so would + undo the privacy property tracked in issue #81); such rows are + labelled by an abbreviation of their remote key and carry the real + timestamp and compressed size from the manifest, with `` + in the two columns that require the local index. Local-only snapshots + are reported as drift, and the hint now names `vaultik prune`, which + exists, instead of `vaultik snapshot cleanup`, which does not. + `reportRemoteDrift` collapsed into the merged view. Every remote + manifest read in the codebase now goes through + `downloadManifestByKey`, so issue #81 has one call site to change. + Verified with `script/cibuild` and end to end against a `file://` + destination with no secret key present. - 2026-08-09: Closed the gap between `make lint` and CI (issue #78). `script/lint` now runs the digest-pinned `golangci-lint` image taken from the `Dockerfile` lint stage, which is the single source of truth diff --git a/internal/vaultik/helpers.go b/internal/vaultik/helpers.go index cc57832..00ebc20 100644 --- a/internal/vaultik/helpers.go +++ b/internal/vaultik/helpers.go @@ -53,18 +53,36 @@ const ( ) // SnapshotInfo contains information about a snapshot. -// UncompressedSize and NewChunkSize are populated only when the snapshot -// is present in the local database; LocallyTracked indicates whether -// those values are meaningful. +// +// LocallyTracked says which of the two sources this row came from, and +// therefore which fields are meaningful: +// +// - true: the snapshot is in the local index. ID is its human +// snapshot ID and UncompressedSize/NewChunkSize are populated. +// - false: the snapshot was found only on the destination store. ID +// is empty, because the human ID cannot be recovered from remote +// storage without the age secret key (see RemoteKey), and +// UncompressedSize/NewChunkSize are zero because they are computed +// from local index rows that do not exist. +// +// RemoteKey is always populated: for a locally tracked snapshot it is +// the key the snapshot would occupy on the destination store, and for a +// remote-only snapshot it is the only identifier available. +// +// RemotePresent reports whether the snapshot's metadata was seen on the +// destination store. It is nil when the destination could not be +// listed, so "absent" and "unknown" stay distinguishable. // //nolint:tagliatelle // snake_case is the established output format type SnapshotInfo struct { ID types.SnapshotID `json:"id"` + RemoteKey string `json:"remote_key"` Timestamp time.Time `json:"timestamp"` CompressedSize int64 `json:"compressed_size"` UncompressedSize int64 `json:"uncompressed_size,omitempty"` NewChunkSize int64 `json:"new_chunk_size,omitempty"` LocallyTracked bool `json:"locally_tracked"` + RemotePresent *bool `json:"remote_present"` } // formatBytes formats bytes in a human-readable format diff --git a/internal/vaultik/snapshot.go b/internal/vaultik/snapshot.go index 80efb3a..32424b6 100644 --- a/internal/vaultik/snapshot.go +++ b/internal/vaultik/snapshot.go @@ -9,10 +9,8 @@ import ( "regexp" "sort" "strings" - "text/tabwriter" "time" - "sneak.berlin/go/vaultik/internal/database" "sneak.berlin/go/vaultik/internal/log" "sneak.berlin/go/vaultik/internal/snapshot" ) @@ -447,243 +445,6 @@ func (v *Vaultik) getSnapshotBlobSizes(snapshotID string) (int64, int64) { return compressed, uncompressed } -// ListSnapshots prints the table of snapshots, plus any reconciliation -// warnings/notes between the local index and the backup destination -// store. -// -// The local index database is always the primary source for the -// table — it has the human snapshot IDs, timestamps, and per-snapshot -// stats. -// -// If an age secret key is configured AND remote listing succeeds, we -// cross-reference: any local snapshot whose hashed key isn't visible -// remotely gets a "local-only" cleanup hint, and any remote key that -// doesn't correspond to a known local snapshot gets reported in a -// NOTE. -// -// If no age key is set the local machine is assumed write-only -// (backup-only), so we skip remote listing entirely — there's no -// value showing keys the user couldn't restore anyway. -// -// If remote listing fails (unmounted volume, permission denied, -// network), we degrade to local-only with a warning. List never -// fails just because the destination is unreachable. -func (v *Vaultik) ListSnapshots(jsonOutput bool) error { - log.Info("Listing snapshots") - - localSnaps, err := v.Repositories.Snapshots.ListRecent(v.ctx, listRecentLimit) - if err != nil { - return fmt.Errorf("listing local snapshots: %w", err) - } - - snapshots := make([]SnapshotInfo, 0, len(localSnaps)) - for _, ls := range localSnaps { - if ls.CompletedAt == nil { - continue - } - - snapshots = append(snapshots, v.snapshotInfoFromLocal(ls)) - } - - sort.Slice(snapshots, func(i, j int) bool { - return snapshots[i].Timestamp.After(snapshots[j].Timestamp) - }) - - if jsonOutput { - encoder := json.NewEncoder(v.Stdout) - encoder.SetIndent("", " ") - - return encoder.Encode(snapshots) - } - - err = v.printSnapshotTable(snapshots) - if err != nil { - return err - } - - if v.Config.AgeSecretKey == "" { - return nil - } - - v.reportRemoteDrift(localSnaps) - - return nil -} - -// reportRemoteDrift cross-references local snapshot records against the -// remote metadata keys and reports local-only records and unknown -// remote keys. Never fails: remote listing errors degrade to a warning. -func (v *Vaultik) reportRemoteDrift(localSnaps []*database.Snapshot) { - remoteKeys, err := v.listAllRemoteSnapshotKeys() - if err != nil { - v.UI.Warningf("Could not list backup destination store: %v.", err) - - return - } - - localKeys := make(map[string]string, len(localSnaps)) - for _, ls := range localSnaps { - if ls.CompletedAt == nil { - continue - } - - localKeys[snapshot.RemoteSnapshotKey(ls.ID.String())] = ls.ID.String() - } - - remoteSet := make(map[string]bool, len(remoteKeys)) - for _, k := range remoteKeys { - remoteSet[k] = true - } - - var localOnly []string - - for key, humanID := range localKeys { - if !remoteSet[key] { - localOnly = append(localOnly, humanID) - } - } - - var remoteOnlyCount int - - for key := range remoteSet { - if _, ok := localKeys[key]; !ok { - remoteOnlyCount++ - } - } - - if len(localOnly) > 0 { - v.UI.Warningf("%d local snapshot record(s) not found in backup "+ - "destination store:", len(localOnly)) - - for _, id := range localOnly { - v.UI.Infof("%s", v.UI.Snapshot(id)) - } - - v.UI.Infof("Run 'vaultik snapshot cleanup' to remove stale local records.") - } - - if remoteOnlyCount > 0 { - v.UI.Noticef("NOTE: %d remote snapshot(s) found in backup destination "+ - "store but not in local database.", remoteOnlyCount) - } -} - -// snapshotInfoFromLocal builds a SnapshotInfo row from a local snapshot -// record. Failures from any per-snapshot stat query degrade that -// column to its snapshot-row fallback but never fail the listing. -func (v *Vaultik) snapshotInfoFromLocal(ls *database.Snapshot) SnapshotInfo { - idStr := ls.ID.String() - - totalSize, err := v.Repositories.Snapshots.GetSnapshotTotalCompressedSize( - v.ctx, idStr) - if err != nil { - log.Warn("Failed to get total compressed size", "id", idStr, "error", err) - - totalSize = ls.BlobSize - } - - uncompressedSize, err := v.Repositories.Snapshots.GetSnapshotUncompressedChunkSize( - v.ctx, idStr) - if err != nil { - log.Warn("Failed to get uncompressed chunk size", "id", idStr, "error", err) - } - - newChunkSize, err := v.Repositories.Snapshots.GetSnapshotNewChunkSize(v.ctx, idStr) - if err != nil { - log.Warn("Failed to get new chunk size", "id", idStr, "error", err) - } - - return SnapshotInfo{ - ID: ls.ID, - Timestamp: ls.StartedAt, - CompressedSize: totalSize, - UncompressedSize: uncompressedSize, - NewChunkSize: newChunkSize, - LocallyTracked: true, - } -} - -// tabPadding is the tabwriter cell padding for the snapshot table. -const tabPadding = 3 - -// printSnapshotTable renders the snapshot list as a formatted table -func (v *Vaultik) printSnapshotTable(snapshots []SnapshotInfo) error { - w := tabwriter.NewWriter(v.Stdout, 0, 0, tabPadding, ' ', 0) - - _, err := fmt.Fprintln(w, "CONFIGURED SNAPSHOTS:") - if err != nil { - return err - } - - _, err = fmt.Fprintln(w, "NAME\tPATHS") - if err != nil { - return err - } - - _, err = fmt.Fprintln(w, "────\t─────") - if err != nil { - return err - } - - for _, name := range v.Config.SnapshotNames() { - snap := v.Config.Snapshots[name] - - paths := strings.Join(snap.Paths, ", ") - - _, err = fmt.Fprintf(w, "%s\t%s\n", name, paths) - if err != nil { - return err - } - } - - _, err = fmt.Fprintln(w) - if err != nil { - return err - } - - _, err = fmt.Fprintln(w, "REMOTE SNAPSHOTS:") - if err != nil { - return err - } - - _, err = fmt.Fprintln(w, - "SNAPSHOT ID\tTIMESTAMP\tCOMPRESSED SIZE\t"+ - "UNCOMPRESSED SIZE\tNEW CHUNK SIZE") - if err != nil { - return err - } - - _, err = fmt.Fprintln(w, - "───────────\t─────────\t───────────────\t"+ - "─────────────────\t──────────────") - if err != nil { - return err - } - - const remoteOnlyCell = "" - for _, snap := range snapshots { - uncompressed := remoteOnlyCell - newChunks := remoteOnlyCell - - if snap.LocallyTracked { - uncompressed = formatBytes(snap.UncompressedSize) - newChunks = formatBytes(snap.NewChunkSize) - } - - _, err = fmt.Fprintf(w, "%s\t%s\t%s\t%s\t%s\n", - snap.ID, - snap.Timestamp.Format("2006-01-02 15:04:05"), - formatBytes(snap.CompressedSize), - uncompressed, - newChunks) - if err != nil { - return err - } - } - - return w.Flush() -} - // SnapshotPurgeOptions contains options for the snapshot purge command. type SnapshotPurgeOptions struct { KeepLatest bool // Keep only the most recent snapshot per name @@ -1123,6 +884,14 @@ func (v *Vaultik) CleanupLocalSnapshots() error { // metadata//manifest.json.zst. The remoteKey is the double- // SHA256 derivation produced by snapshot.RemoteSnapshotKey, not the // human snapshot ID. Callers that have a human ID must hash first. +// +// This is the only place vaultik reads a manifest off the destination +// store, deliberately: the manifest is currently stored compressed but +// unencrypted, which is what lets `snapshot list` enumerate the +// destination on a host holding no private key. Whether to encrypt it +// is open (issue #81), and routing every read through here means that +// decision has exactly one call site to change. Keep it that way — do +// not open metadata//manifest.json.zst directly elsewhere. func (v *Vaultik) downloadManifestByKey(remoteKey string) (*snapshot.Manifest, error) { manifestPath := fmt.Sprintf("metadata/%s/manifest.json.zst", remoteKey) diff --git a/internal/vaultik/snapshot_list.go b/internal/vaultik/snapshot_list.go new file mode 100644 index 0000000..70d228f --- /dev/null +++ b/internal/vaultik/snapshot_list.go @@ -0,0 +1,486 @@ +package vaultik + +import ( + "encoding/json" + "fmt" + "sort" + "strings" + "text/tabwriter" + "time" + + "golang.org/x/sync/errgroup" + "sneak.berlin/go/vaultik/internal/database" + "sneak.berlin/go/vaultik/internal/log" + "sneak.berlin/go/vaultik/internal/snapshot" +) + +// remoteOnlyCell fills the table columns that can only be derived from +// the local index. A snapshot present only on the destination store has +// no local rows to derive them from. +const remoteOnlyCell = "" + +// remoteKeyDisplayLen is how many hex characters of a remote key are +// shown in the identifier column for a remote-only snapshot. Twelve +// matches the abbreviation length used elsewhere in the UI and is far +// past the point of ambiguity for a SHA256 digest. +const remoteKeyDisplayLen = 12 + +// maxRemoteOnlyRows caps how many remote-only snapshots a single +// `snapshot list` will describe. Each one costs a manifest read, so an +// uncapped listing against a destination holding many thousands of +// unknown snapshots would be both slow and unbounded in memory. Beyond +// the cap the table is truncated and the count of omitted snapshots is +// reported. +const maxRemoteOnlyRows = 1000 + +// remoteManifestFetchConcurrency bounds how many manifest reads are in +// flight at once while describing remote-only snapshots. The listing +// itself is a single streamed prefix request; only the per-snapshot +// manifest reads need throttling. +const remoteManifestFetchConcurrency = 8 + +// tabPadding is the tabwriter cell padding for the snapshot table. +const tabPadding = 3 + +// ListSnapshots prints the table of snapshots known to this host: the +// union of the local index database and the backup destination store. +// +// Remote listing needs no age secret key. A snapshot's manifest +// (metadata//manifest.json.zst) is compressed but not +// encrypted, so a host holding only the public key — the configuration +// vaultik is designed for — can still enumerate what it has backed up +// and see each snapshot's timestamp and compressed size. +// +// What that host cannot see is a remote-only snapshot's human ID. +// snapshot.RemoteSnapshotKey is one-way and the manifest stores the +// hashed key rather than the ID, so hostname and snapshot name live +// only in the local index and in the encrypted db.zst.age. Remote-only +// rows are therefore identified by an abbreviation of their remote key, +// and the two columns that genuinely require the local index +// (uncompressed size, new chunk size) render as "". No +// attempt is made to recover or fabricate the human ID. +// +// Snapshots in the local index with no counterpart on the destination +// store are reported as drift below the table. +// +// If remote listing fails (unmounted volume, permission denied, +// network), we degrade to local-only with a warning. List never fails +// just because the destination is unreachable. +func (v *Vaultik) ListSnapshots(jsonOutput bool) error { + log.Info("Listing snapshots") + + localSnaps, err := v.Repositories.Snapshots.ListRecent(v.ctx, listRecentLimit) + if err != nil { + return fmt.Errorf("listing local snapshots: %w", err) + } + + snapshots := make([]SnapshotInfo, 0, len(localSnaps)) + localKeys := make(map[string]bool, len(localSnaps)) + + for _, ls := range localSnaps { + if ls.CompletedAt == nil { + continue + } + + info := v.snapshotInfoFromLocal(ls) + localKeys[info.RemoteKey] = true + snapshots = append(snapshots, info) + } + + listing, remoteErr := v.collectRemoteSnapshots(localKeys) + if remoteErr != nil { + v.warnRemoteListingFailed(remoteErr, jsonOutput) + } else { + snapshots = append(snapshots, listing.remoteOnly...) + markRemotePresence(snapshots, listing.keys) + } + + sort.Slice(snapshots, func(i, j int) bool { + return snapshots[i].Timestamp.After(snapshots[j].Timestamp) + }) + + if jsonOutput { + encoder := json.NewEncoder(v.Stdout) + encoder.SetIndent("", " ") + + return encoder.Encode(snapshots) + } + + err = v.printSnapshotTable(snapshots) + if err != nil { + return err + } + + if remoteErr == nil { + v.reportListDrift(snapshots, listing) + } + + return nil +} + +// warnRemoteListingFailed reports an unreachable or unreadable +// destination store without failing the command: the local index is +// still worth printing, and `snapshot list` exiting non-zero because a +// volume is unmounted would be worse than useless. +// +// In --json mode the warning goes to stderr rather than through the +// logger or the UI writer, both of which emit on stdout — the JSON +// document has to be the only thing on stdout for `snapshot list --json +// | jq` to work. The failure is also representable in the document +// itself: every row's remote_present is null when the destination could +// not be listed. +func (v *Vaultik) warnRemoteListingFailed(err error, jsonOutput bool) { + if jsonOutput { + _, _ = fmt.Fprintf(v.Stderr, + "Warning: could not list backup destination store: %v. "+ + "Showing snapshots from the local index only.\n", err) + + return + } + + log.Warn("Could not list backup destination store", "error", err) + v.UI.Warningf("Could not list backup destination store: %v.", err) + v.UI.Infof("Showing snapshots from the local index only.") +} + +// remoteSnapshotListing is the result of one pass over the destination +// store's metadata/ prefix. +type remoteSnapshotListing struct { + // keys holds every remote snapshot key present on the destination + // store, whether or not it is known locally. + keys map[string]bool + + // remoteOnly holds one row per remote key with no local + // counterpart, built from that snapshot's manifest. + remoteOnly []SnapshotInfo + + // omitted counts remote-only keys dropped because describing them + // all would have exceeded maxRemoteOnlyRows. + omitted int + + // unreadable counts remote-only keys whose manifest could not be + // read or decoded. + unreadable int +} + +// collectRemoteSnapshots enumerates the destination store and describes +// every snapshot on it that localKeys does not already account for. +// +// The enumeration is a single streamed listing of the metadata/ prefix, +// so the request count does not scale with the number of snapshots. +// Manifest reads scale only with the number of snapshots the local +// index does not already know about, and are capped at +// maxRemoteOnlyRows. +func (v *Vaultik) collectRemoteSnapshots( + localKeys map[string]bool, +) (*remoteSnapshotListing, error) { + keys, err := v.listAllRemoteSnapshotKeys() + if err != nil { + return nil, err + } + + listing := &remoteSnapshotListing{ + keys: make(map[string]bool, len(keys)), + } + + unknown := make([]string, 0, len(keys)) + + for _, key := range keys { + listing.keys[key] = true + + if !localKeys[key] { + unknown = append(unknown, key) + } + } + + // Sorted so both the truncation point and the fetch order are + // deterministic run to run. + sort.Strings(unknown) + + if len(unknown) > maxRemoteOnlyRows { + listing.omitted = len(unknown) - maxRemoteOnlyRows + unknown = unknown[:maxRemoteOnlyRows] + } + + listing.remoteOnly, listing.unreadable = v.describeRemoteOnlySnapshots(unknown) + + return listing, nil +} + +// describeRemoteOnlySnapshots reads the manifest for each supplied +// remote key and turns it into a table row, returning the rows and the +// number of keys whose manifest could not be read. +// +// A key whose manifest is missing or corrupt is skipped rather than +// failing the listing: one bad snapshot directory must not hide every +// other snapshot the user has. +func (v *Vaultik) describeRemoteOnlySnapshots(keys []string) ([]SnapshotInfo, int) { + found := make([]SnapshotInfo, len(keys)) + ok := make([]bool, len(keys)) + + var group errgroup.Group + + group.SetLimit(remoteManifestFetchConcurrency) + + for i, key := range keys { + group.Go(func() error { + info, err := v.remoteSnapshotInfo(key) + if err != nil { + log.Warn("Could not describe remote snapshot", + "remote_key", key, "error", err) + + return nil + } + + found[i] = info + ok[i] = true + + return nil + }) + } + + // No goroutine above ever returns an error; failures are recorded + // in ok and reported as a count. + _ = group.Wait() + + infos := make([]SnapshotInfo, 0, len(keys)) + unreadable := 0 + + for i := range keys { + if !ok[i] { + unreadable++ + + continue + } + + infos = append(infos, found[i]) + } + + return infos, unreadable +} + +// remoteSnapshotInfo builds a table row for a snapshot that exists on +// the destination store but not in the local index, from the only +// source available without the private key: the unencrypted manifest. +// +// ID is deliberately left zero. Recovering it would mean inverting +// snapshot.RemoteSnapshotKey, which is not possible, or writing the +// human ID somewhere unencrypted on the destination, which would undo +// the privacy property that hashing the key exists to provide (see +// issue #81). The renderer marks the row as unnamed rather than +// guessing. +func (v *Vaultik) remoteSnapshotInfo(remoteKey string) (SnapshotInfo, error) { + manifest, err := v.downloadManifestByKey(remoteKey) + if err != nil { + return SnapshotInfo{}, err + } + + timestamp, err := time.Parse(time.RFC3339, manifest.Timestamp) + if err != nil { + // The snapshot is really there; an unparseable timestamp is not + // reason enough to hide it. It sorts to the bottom as the zero + // time. + log.Warn("Remote manifest has an unparseable timestamp", + "remote_key", remoteKey, "timestamp", manifest.Timestamp, "error", err) + + timestamp = time.Time{} + } + + return SnapshotInfo{ + RemoteKey: remoteKey, + Timestamp: timestamp.UTC(), + CompressedSize: manifest.TotalCompressedSize, + LocallyTracked: false, + }, nil +} + +// markRemotePresence records, for every row, whether its remote key was +// seen on the destination store during this listing. Only called when +// the listing succeeded: when it did not, presence stays nil ("not +// known") rather than being reported as absence. +func markRemotePresence(snapshots []SnapshotInfo, remoteKeys map[string]bool) { + for i := range snapshots { + present := remoteKeys[snapshots[i].RemoteKey] + snapshots[i].RemotePresent = &present + } +} + +// snapshotInfoFromLocal builds a SnapshotInfo row from a local snapshot +// record. Failures from any per-snapshot stat query degrade that +// column to its snapshot-row fallback but never fail the listing. +func (v *Vaultik) snapshotInfoFromLocal(ls *database.Snapshot) SnapshotInfo { + idStr := ls.ID.String() + + totalSize, err := v.Repositories.Snapshots.GetSnapshotTotalCompressedSize( + v.ctx, idStr) + if err != nil { + log.Warn("Failed to get total compressed size", "id", idStr, "error", err) + + totalSize = ls.BlobSize + } + + uncompressedSize, err := v.Repositories.Snapshots.GetSnapshotUncompressedChunkSize( + v.ctx, idStr) + if err != nil { + log.Warn("Failed to get uncompressed chunk size", "id", idStr, "error", err) + } + + newChunkSize, err := v.Repositories.Snapshots.GetSnapshotNewChunkSize(v.ctx, idStr) + if err != nil { + log.Warn("Failed to get new chunk size", "id", idStr, "error", err) + } + + return SnapshotInfo{ + ID: ls.ID, + RemoteKey: snapshot.RemoteSnapshotKey(idStr), + Timestamp: ls.StartedAt, + CompressedSize: totalSize, + UncompressedSize: uncompressedSize, + NewChunkSize: newChunkSize, + LocallyTracked: true, + } +} + +// reportListDrift prints the reconciliation notes the merged table +// cannot express on its own: local records with no counterpart on the +// destination store, plus counts of remote snapshots that were +// unreadable or omitted. +// +// This is what remains of the old reportRemoteDrift, and it no longer +// touches the destination store. Its remote-only half collapsed into +// the table — those snapshots are rows now, not a footnote count — and +// its local-only half reads the merge ListSnapshots already computed, +// so `snapshot list` lists the destination exactly once per invocation. +func (v *Vaultik) reportListDrift( + snapshots []SnapshotInfo, listing *remoteSnapshotListing, +) { + var localOnly []string + + for _, snap := range snapshots { + if snap.LocallyTracked && !listing.keys[snap.RemoteKey] { + localOnly = append(localOnly, snap.ID.String()) + } + } + + if len(localOnly) > 0 { + v.UI.Warningf("%d local snapshot record(s) not found in backup "+ + "destination store:", len(localOnly)) + + for _, id := range localOnly { + v.UI.Infof("%s", v.UI.Snapshot(id)) + } + + v.UI.Infof("Run '%s' to remove stale local records.", pruneCommandHint) + } + + if len(listing.remoteOnly) > 0 { + v.UI.Noticef("NOTE: %d snapshot(s) on the backup destination store "+ + "are not in the local index. Their hostname and snapshot name "+ + "cannot be recovered without the age secret key, so they are "+ + "listed by remote key.", len(listing.remoteOnly)) + } + + if listing.unreadable > 0 { + v.UI.Warningf("%d remote snapshot(s) could not be described: "+ + "manifest missing or unreadable.", listing.unreadable) + } + + if listing.omitted > 0 { + v.UI.Warningf("%d further remote-only snapshot(s) not shown "+ + "(limit %d per listing).", listing.omitted, maxRemoteOnlyRows) + } +} + +// formatRemoteOnlyID renders the identifier cell for a snapshot absent +// from the local index. Its human ID cannot be recovered without the +// private key, so the cell shows an abbreviation of the remote key +// instead. The angle brackets make it obvious this is not a snapshot +// name, which matters more than compactness: a bare hex string would +// read as a name the user simply doesn't recognize. +func formatRemoteOnlyID(remoteKey string) string { + short := remoteKey + if len(short) > remoteKeyDisplayLen { + short = short[:remoteKeyDisplayLen] + } + + return "" +} + +// printSnapshotTable renders the snapshot list as a formatted table +func (v *Vaultik) printSnapshotTable(snapshots []SnapshotInfo) error { + w := tabwriter.NewWriter(v.Stdout, 0, 0, tabPadding, ' ', 0) + + _, err := fmt.Fprintln(w, "CONFIGURED SNAPSHOTS:") + if err != nil { + return err + } + + _, err = fmt.Fprintln(w, "NAME\tPATHS") + if err != nil { + return err + } + + _, err = fmt.Fprintln(w, "────\t─────") + if err != nil { + return err + } + + for _, name := range v.Config.SnapshotNames() { + snap := v.Config.Snapshots[name] + + paths := strings.Join(snap.Paths, ", ") + + _, err = fmt.Fprintf(w, "%s\t%s\n", name, paths) + if err != nil { + return err + } + } + + _, err = fmt.Fprintln(w) + if err != nil { + return err + } + + _, err = fmt.Fprintln(w, "REMOTE SNAPSHOTS:") + if err != nil { + return err + } + + _, err = fmt.Fprintln(w, + "SNAPSHOT ID\tTIMESTAMP\tCOMPRESSED SIZE\t"+ + "UNCOMPRESSED SIZE\tNEW CHUNK SIZE") + if err != nil { + return err + } + + _, err = fmt.Fprintln(w, + "───────────\t─────────\t───────────────\t"+ + "─────────────────\t──────────────") + if err != nil { + return err + } + + for _, snap := range snapshots { + id := formatRemoteOnlyID(snap.RemoteKey) + uncompressed := remoteOnlyCell + newChunks := remoteOnlyCell + + if snap.LocallyTracked { + id = snap.ID.String() + uncompressed = formatBytes(snap.UncompressedSize) + newChunks = formatBytes(snap.NewChunkSize) + } + + _, err = fmt.Fprintf(w, "%s\t%s\t%s\t%s\t%s\n", + id, + snap.Timestamp.Format("2006-01-02 15:04:05"), + formatBytes(snap.CompressedSize), + uncompressed, + newChunks) + if err != nil { + return err + } + } + + return w.Flush() +} diff --git a/internal/vaultik/snapshot_list_test.go b/internal/vaultik/snapshot_list_test.go new file mode 100644 index 0000000..b5280ed --- /dev/null +++ b/internal/vaultik/snapshot_list_test.go @@ -0,0 +1,533 @@ +package vaultik_test + +import ( + "bytes" + "context" + "database/sql" + "encoding/json" + "errors" + "io" + "strings" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/vaultik/internal/config" + "sneak.berlin/go/vaultik/internal/database" + "sneak.berlin/go/vaultik/internal/log" + "sneak.berlin/go/vaultik/internal/snapshot" + "sneak.berlin/go/vaultik/internal/storage" + "sneak.berlin/go/vaultik/internal/types" + "sneak.berlin/go/vaultik/internal/ui" + "sneak.berlin/go/vaultik/internal/vaultik" +) + +// errRemoteUnreachable stands in for the real-world reasons a +// destination store cannot be listed: unmounted volume, permission +// denied, network down. +var errRemoteUnreachable = errors.New("permission denied") + +// observingStorer wraps testStorer to record how the destination store +// was used: how many prefix listings were issued (the merged listing +// must not scale requests with snapshot count) and which object keys +// were fetched (nothing encrypted may be fetched during a listing). +// Setting listErr makes every listing fail, simulating an unreachable +// destination. +type observingStorer struct { + *testStorer + + mu sync.Mutex + listCalls int + fetched []string + listErr error +} + +func newObservingStorer() *observingStorer { + return &observingStorer{testStorer: newTestStorer()} +} + +func (s *observingStorer) ListStream( + ctx context.Context, prefix string, +) <-chan storage.ObjectInfo { + s.mu.Lock() + s.listCalls++ + failure := s.listErr + s.mu.Unlock() + + if failure != nil { + ch := make(chan storage.ObjectInfo, 1) + ch <- storage.ObjectInfo{Err: failure} + + close(ch) + + return ch + } + + return s.testStorer.ListStream(ctx, prefix) +} + +func (s *observingStorer) Get( + ctx context.Context, key string, +) (io.ReadCloser, error) { + s.mu.Lock() + s.fetched = append(s.fetched, key) + s.mu.Unlock() + + return s.testStorer.Get(ctx, key) +} + +// listStreamCalls returns how many prefix listings were issued. +func (s *observingStorer) listStreamCalls() int { + s.mu.Lock() + defer s.mu.Unlock() + + return s.listCalls +} + +// fetchedKeys returns a copy of every object key that was read. +func (s *observingStorer) fetchedKeys() []string { + s.mu.Lock() + defer s.mu.Unlock() + + return append([]string(nil), s.fetched...) +} + +// listEnv is a Vaultik wired for exercising ListSnapshots: an in-memory +// index database, an observable in-memory destination store, and +// captured output. +// +// The configuration deliberately has no age secret key. That is the +// production configuration vaultik is designed for — the backed-up host +// holds only the public key — and every assertion in this file has to +// hold in it. +type listEnv struct { + v *vaultik.Vaultik + store *observingStorer + stdout *bytes.Buffer + stderr *bytes.Buffer +} + +func newListEnv(t *testing.T) *listEnv { + t.Helper() + + ctx := context.Background() + + db, err := database.New(ctx, ":memory:") + require.NoError(t, err) + t.Cleanup(func() { _ = db.Close() }) + + store := newObservingStorer() + stdout := &bytes.Buffer{} + stderr := &bytes.Buffer{} + + v := &vaultik.Vaultik{ + Config: &config.Config{ + AgeSecretKey: "", + Snapshots: map[string]config.SnapshotConfig{ + listConfiguredName: {Paths: []string{"/" + listConfiguredName}}, + }, + }, + Storage: store, + Repositories: database.NewRepositories(db), + DB: db, + Stdout: stdout, + Stderr: stderr, + Stdin: &bytes.Buffer{}, + UI: ui.NewWithColor(stdout, false), + } + v.SetContext(ctx) + + return &listEnv{v: v, store: store, stdout: stdout, stderr: stderr} +} + +// addLocal inserts a completed snapshot into the local index. +func (e *listEnv) addLocal(t *testing.T, id string, startedAt time.Time) { + t.Helper() + + completedAt := startedAt.Add(time.Minute) + snap := &database.Snapshot{ + ID: types.SnapshotID(id), + Hostname: "testhost", + VaultikVersion: testLabel, + StartedAt: startedAt, + CompletedAt: &completedAt, + } + + ctx := context.Background() + err := e.v.Repositories.WithTx(ctx, func(ctx context.Context, tx *sql.Tx) error { + return e.v.Repositories.Snapshots.Create(ctx, tx, snap) + }) + require.NoError(t, err, "creating local snapshot %s", id) +} + +// addRemote writes a manifest to the destination store at the hashed +// path the production code uses, exactly as a real backup would. Every +// fixture snapshot has the same compressed size (fiveMegabytes); the +// tests care about which columns are populated, not about size variety. +func (e *listEnv) addRemote( + t *testing.T, snapshotID string, timestamp time.Time, +) string { + t.Helper() + + remoteKey := snapshot.RemoteSnapshotKey(snapshotID) + manifest := &snapshot.Manifest{ + // Note: the hashed key, never the human ID. That is precisely + // why a remote-only snapshot cannot be named. + SnapshotID: remoteKey, + Timestamp: timestamp.UTC().Format(time.RFC3339), + BlobCount: 1, + TotalCompressedSize: fiveMegabytes, + Blobs: []snapshot.BlobInfo{ + {Hash: testBlobHashA, CompressedSize: fiveMegabytes}, + }, + } + + data, err := snapshot.EncodeManifest(manifest, 3) + require.NoError(t, err) + + err = e.store.Put(context.Background(), + "metadata/"+remoteKey+"/manifest.json.zst", bytes.NewReader(data)) + require.NoError(t, err) + + return remoteKey +} + +// Fixtures shared across the listing tests. +const ( + // listConfiguredName is the one snapshot name in the test config. + listConfiguredName = "home" + listLocalID = "testhost_home_2026-03-01T10:00:00Z" + listRemoteID = "otherhost_media_2026-03-02T11:22:33Z" + // fiveMegabytes formats as "5.0 MB" through formatBytes. + fiveMegabytes = 5 * 1024 * 1024 +) + +// TestListSnapshots_RemoteWithoutSecretKey is the regression guard for +// issue #64: `snapshot list` must read the destination store on a host +// that holds no private key. If the remote listing is ever gated on +// age_secret_key again, this fails. +func TestListSnapshots_RemoteWithoutSecretKey(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + env := newListEnv(t) + require.Empty(t, env.v.Config.AgeSecretKey, + "this test is meaningless unless the host has no private key") + + timestamp := time.Date(2026, 3, 2, 11, 22, 33, 0, time.UTC) + remoteKey := env.addRemote(t, listRemoteID, timestamp) + + err := env.v.ListSnapshots(false) + require.NoError(t, err) + + // The destination store was actually read, with a single prefix + // listing rather than one request per snapshot. + assert.Equal(t, 1, env.store.listStreamCalls(), + "expected exactly one prefix listing of the destination store") + + // Nothing encrypted was touched: enumerating snapshots must never + // need the age secret key. + for _, key := range env.store.fetchedKeys() { + assert.NotContains(t, key, ".age", + "listing must not read encrypted objects") + } + + out := env.stdout.String() + + // The snapshot is identified by an abbreviation of its remote key. + assert.Contains(t, out, "") + + // Its human ID is not recoverable and must not be invented. + assert.NotContains(t, out, "otherhost") + assert.NotContains(t, out, "media") + + // Manifest-derived columns carry real values. + assert.Contains(t, out, "2026-03-02 11:22:33") + assert.Contains(t, out, "5.0 MB") + + // The two columns that require the local index are marked, not + // blank and not zero. ("" cells and the +// LocallyTracked == false branch are verified rather than assumed. +func TestListSnapshots_RemoteOnlyRowRendering(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + env := newListEnv(t) + + timestamp := time.Date(2026, 3, 2, 11, 22, 33, 0, time.UTC) + remoteKey := env.addRemote(t, listRemoteID, timestamp) + + err := env.v.ListSnapshots(false) + require.NoError(t, err) + + label := "" + row := findTableRow(t, env.stdout.String(), label) + + // Identifier column: the abbreviated remote key, never blank and + // visibly not a snapshot name. + assert.True(t, strings.HasPrefix(row, label), + "identifier column must lead the row: %q", row) + + // Manifest-derived columns: real values, not placeholders. + assert.Contains(t, row, "2026-03-02 11:22:33") + assert.Contains(t, row, "5.0 MB") + + // Exactly the two local-index-derived columns are marked. + assert.Equal(t, 2, strings.Count(row, remoteOnlyCellText), + "uncompressed and new-chunk cells must both be marked: %q", row) + + // And the note explaining why the row has no name. + assert.Contains(t, env.stdout.String(), + "are not in the local index") +} + +// findTableRow returns the single output line containing needle. +func findTableRow(t *testing.T, out, needle string) string { + t.Helper() + + var found []string + + for line := range strings.SplitSeq(out, "\n") { + if strings.Contains(line, needle) { + found = append(found, line) + } + } + + require.Len(t, found, 1, "expected exactly one line containing %q", needle) + + return found[0] +} + +// TestListSnapshots_MergesLocalAndRemote checks that both sources land +// in one table and that a locally tracked snapshot keeps its human ID +// and its local-index-derived columns. +func TestListSnapshots_MergesLocalAndRemote(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + env := newListEnv(t) + + localStart := time.Date(2026, 3, 1, 10, 0, 0, 0, time.UTC) + env.addLocal(t, listLocalID, localStart) + env.addRemote(t, listLocalID, localStart) + + remoteKey := env.addRemote(t, listRemoteID, + time.Date(2026, 3, 2, 11, 22, 33, 0, time.UTC)) + + err := env.v.ListSnapshots(false) + require.NoError(t, err) + + out := env.stdout.String() + + assert.Contains(t, out, listLocalID) + assert.Contains(t, out, "") + + // The locally tracked row is not marked as remote-only anywhere. + localRow := findTableRow(t, out, listLocalID) + assert.NotContains(t, localRow, "") + + // The local snapshot is present remotely, so no drift is reported. + assert.NotContains(t, out, "not found in backup destination store") +} + +// TestListSnapshots_LocalOnlyReportedAsDrift covers a snapshot in the +// local index with no counterpart on the destination store, and checks +// the remediation hint names a command that actually exists. +func TestListSnapshots_LocalOnlyReportedAsDrift(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + env := newListEnv(t) + env.addLocal(t, listLocalID, time.Date(2026, 3, 1, 10, 0, 0, 0, time.UTC)) + + err := env.v.ListSnapshots(false) + require.NoError(t, err) + + out := env.stdout.String() + + assert.Contains(t, out, listLocalID) + assert.Contains(t, out, "1 local snapshot record(s) not found in backup") + assert.Contains(t, out, "vaultik prune") + + // There is no `vaultik snapshot cleanup` command; the hint must not + // name one. + assert.NotContains(t, out, "snapshot cleanup") +} + +// TestListSnapshots_UnreachableRemoteDegrades covers the promise in the +// doc comment: an unreachable destination is a warning plus local-only +// output, never a failure. +func TestListSnapshots_UnreachableRemoteDegrades(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + env := newListEnv(t) + env.addLocal(t, listLocalID, time.Date(2026, 3, 1, 10, 0, 0, 0, time.UTC)) + env.store.listErr = errRemoteUnreachable + + // Zero exit code: the CLI turns a nil return into exit 0. + err := env.v.ListSnapshots(false) + require.NoError(t, err) + + out := env.stdout.String() + + assert.Contains(t, out, "Could not list backup destination store") + assert.Contains(t, out, "permission denied") + assert.Contains(t, out, "Showing snapshots from the local index only.") + + // The local index is still shown. + assert.Contains(t, out, listLocalID) + + // With no remote listing there is no basis for a drift claim, so + // none must be made. + assert.NotContains(t, out, "not found in backup destination store") +} + +// TestListSnapshots_UnreadableManifestDoesNotHideOthers checks that one +// corrupt remote snapshot directory cannot suppress every other +// snapshot on the destination store. +func TestListSnapshots_UnreadableManifestDoesNotHideOthers(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + env := newListEnv(t) + + goodKey := env.addRemote(t, listRemoteID, + time.Date(2026, 3, 2, 11, 22, 33, 0, time.UTC)) + + badKey := snapshot.RemoteSnapshotKey("testhost_broken_2026-03-03T00:00:00Z") + err := env.store.Put(context.Background(), + "metadata/"+badKey+"/manifest.json.zst", + strings.NewReader("this is not a zstd stream")) + require.NoError(t, err) + + err = env.v.ListSnapshots(false) + require.NoError(t, err) + + out := env.stdout.String() + + assert.Contains(t, out, "") + assert.NotContains(t, out, "") + assert.Contains(t, out, "1 remote snapshot(s) could not be described") +} + +// listJSONRow mirrors the JSON shape ListSnapshots emits, so the test +// asserts against the wire format rather than the Go struct. +// +//nolint:tagliatelle // snake_case is the established output format +type listJSONRow struct { + ID string `json:"id"` + RemoteKey string `json:"remote_key"` + CompressedSize int64 `json:"compressed_size"` + LocallyTracked bool `json:"locally_tracked"` + RemotePresent *bool `json:"remote_present"` +} + +// decodeListJSON parses the command's stdout, which must contain +// nothing but the JSON document. +func decodeListJSON(t *testing.T, out string) []listJSONRow { + t.Helper() + + var rows []listJSONRow + + err := json.Unmarshal([]byte(out), &rows) + require.NoError(t, err, "stdout must be parseable JSON: %q", out) + + return rows +} + +// TestListSnapshots_JSONMergedView covers the --json view of all three +// cases at once: tracked-and-present, tracked-but-missing remotely, and +// remote-only. +func TestListSnapshots_JSONMergedView(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + env := newListEnv(t) + + syncedStart := time.Date(2026, 3, 1, 10, 0, 0, 0, time.UTC) + env.addLocal(t, listLocalID, syncedStart) + env.addRemote(t, listLocalID, syncedStart) + + driftedID := "testhost_home_2026-02-01T10:00:00Z" + env.addLocal(t, driftedID, time.Date(2026, 2, 1, 10, 0, 0, 0, time.UTC)) + + remoteKey := env.addRemote(t, listRemoteID, + time.Date(2026, 3, 2, 11, 22, 33, 0, time.UTC)) + + err := env.v.ListSnapshots(true) + require.NoError(t, err) + + rows := decodeListJSON(t, env.stdout.String()) + require.Len(t, rows, 3) + + byKey := make(map[string]listJSONRow, len(rows)) + for _, row := range rows { + byKey[row.RemoteKey] = row + } + + synced := byKey[snapshot.RemoteSnapshotKey(listLocalID)] + assert.Equal(t, listLocalID, synced.ID) + assert.True(t, synced.LocallyTracked) + require.NotNil(t, synced.RemotePresent) + assert.True(t, *synced.RemotePresent) + + drifted := byKey[snapshot.RemoteSnapshotKey(driftedID)] + assert.Equal(t, driftedID, drifted.ID) + assert.True(t, drifted.LocallyTracked) + require.NotNil(t, drifted.RemotePresent) + assert.False(t, *drifted.RemotePresent, + "a local-only snapshot must be visible as drift in --json too") + + remoteOnly := byKey[remoteKey] + assert.False(t, remoteOnly.LocallyTracked) + assert.Empty(t, remoteOnly.ID, + "the human ID is unrecoverable and must not be fabricated") + assert.Len(t, remoteOnly.RemoteKey, 64, + "--json carries the full remote key, not the truncated form") + assert.Equal(t, int64(fiveMegabytes), remoteOnly.CompressedSize) + require.NotNil(t, remoteOnly.RemotePresent) + assert.True(t, *remoteOnly.RemotePresent) +} + +// TestListSnapshots_JSONUnreachableRemote checks that a failed listing +// does not corrupt the JSON document with warning text, and that +// "unknown" is reported as null rather than as absence. +func TestListSnapshots_JSONUnreachableRemote(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + env := newListEnv(t) + env.addLocal(t, listLocalID, time.Date(2026, 3, 1, 10, 0, 0, 0, time.UTC)) + env.store.listErr = errRemoteUnreachable + + err := env.v.ListSnapshots(true) + require.NoError(t, err) + + // stdout must be nothing but the JSON document, so the warning has + // to go to stderr. + rows := decodeListJSON(t, env.stdout.String()) + require.Len(t, rows, 1) + + assert.Equal(t, listLocalID, rows[0].ID) + assert.True(t, rows[0].LocallyTracked) + assert.Nil(t, rows[0].RemotePresent, + "remote state is unknown when the destination cannot be listed") + + assert.Contains(t, env.stderr.String(), + "could not list backup destination store") + assert.Contains(t, env.stderr.String(), "permission denied") +} -- 2.49.1 From 9a45221b791aed2e0dfd0a713be13e6b164c6765 Mon Sep 17 00:00:00 2001 From: sneak Date: Sun, 9 Aug 2026 05:21:43 +0000 Subject: [PATCH 3/3] Fix timezone drift and --json truncation in snapshot list (closes #64) Three defects in the merged listing path, all found in review. 1. The TIMESTAMP column mixed two timezones. Remote-only rows render timestamp.UTC(); local rows come from ListRecent, whose scanner decoded Unix seconds in the host's local zone, unlike the two other snapshot scanners in that file. Both render through the same zone-less format string, so on a non-UTC host the same snapshot showed one time when locally tracked and another when remote-only, in the same column, with nothing to indicate why. Normalized at the point the timestamp enters the domain rather than at the display layer: scanSnapshotRows now decodes in UTC like its siblings, and GetIncompleteByHostname's copy of that loop was folded onto the shared scanner so the three call sites cannot drift apart again. 2. --json silently truncated. The early return skipped reportListDrift, so neither the 1000-row cap nor the unreadable-manifest count reached a machine consumer: past the cap the document was short with no signal at all. Both counts now go to stderr, where the unreachable-destination warning already goes. The document's shape is deliberately unchanged, so existing consumers keep parsing. 3. The --json stderr workaround was half-applied. Two per-snapshot log.Warn calls on the same new path were left unguarded, and the logger writes to stdout at default level, so one corrupt manifest put a log line ahead of the document and broke `| jq`. Both now route through the same JSON-aware writer. They are also collected during the concurrent manifest reads and emitted afterwards in key order, since that writer is not safe for concurrent use. Still a local workaround; the logger itself is issue #82. Also from review, non-blocking: the destination-listing failure is no longer printed twice in table mode, the identifier cell is no longer computed and discarded for locally tracked rows, and the merge sort is stable so that rows sharing the zero-timestamp fallback keep a deterministic order. Tests: each fix has a test that fails without it. The timezone tests pin time.Local to a non-UTC zone and assert Location identity, so they would have caught this on the UTC host where it was missed. The JSON stdout test redirects the process's own stdout to a pipe and rebuilds the logger over it, so it can actually observe a log line landing on stdout ahead of the document rather than asserting on an injected buffer the log never reaches. --- TODO.md | 16 +- internal/database/snapshots.go | 51 ++--- internal/database/snapshots_test.go | 113 +++++++++++ internal/vaultik/snapshot_list.go | 158 +++++++++++++-- internal/vaultik/snapshot_list_test.go | 270 ++++++++++++++++++++++++- 5 files changed, 548 insertions(+), 60 deletions(-) diff --git a/TODO.md b/TODO.md index 3a362c5..1bb83f7 100644 --- a/TODO.md +++ b/TODO.md @@ -36,8 +36,20 @@ or delete the branch. `reportRemoteDrift` collapsed into the merged view. Every remote manifest read in the codebase now goes through `downloadManifestByKey`, so issue #81 has one call site to change. - Verified with `script/cibuild` and end to end against a `file://` - destination with no secret key present. + Review rework: snapshot timestamps now normalize to UTC in + `scanSnapshotRows`, the one place they enter the domain, so the merged + TIMESTAMP column cannot show local time for a locally tracked row and + UTC for a remote-only row on a non-UTC host; `GetIncompleteByHostname` + was folded onto that same scanner. `--json` now reports the + unreadable-manifest count and the 1000-row truncation on stderr + instead of returning a silently short document (the document's shape + is unchanged). The two per-snapshot `log.Warn` calls on the listing + path now route through the same JSON-aware writer as the existing + workaround, so one corrupt manifest can no longer put a log line on + stdout ahead of the document and break `| jq` — still a local + workaround pending issue #82. Verified with `script/cibuild` and with + an uncached `make check` (`0 issues.`, no cached test packages), plus + end to end against a `file://` destination with no secret key present. - 2026-08-09: Closed the gap between `make lint` and CI (issue #78). `script/lint` now runs the digest-pinned `golangci-lint` image taken from the `Dockerfile` lint stage, which is the single source of truth diff --git a/internal/database/snapshots.go b/internal/database/snapshots.go index 715ab78..97bef66 100644 --- a/internal/database/snapshots.go +++ b/internal/database/snapshots.go @@ -609,43 +609,9 @@ func (r *SnapshotRepository) GetIncompleteByHostname( } }() - var snapshots []*Snapshot - - for rows.Next() { - var ( - snapshot Snapshot - startedAtUnix int64 - completedAtUnix *int64 - ) - - err := rows.Scan( - &snapshot.ID, - &snapshot.Hostname, - &snapshot.VaultikVersion, - &snapshot.VaultikGitRevision, - &startedAtUnix, - &completedAtUnix, - &snapshot.FileCount, - &snapshot.ChunkCount, - &snapshot.BlobCount, - &snapshot.TotalSize, - &snapshot.BlobSize, - &snapshot.CompressionRatio, - ) - if err != nil { - return nil, fmt.Errorf("scanning snapshot: %w", err) - } - - snapshot.StartedAt = time.Unix(startedAtUnix, 0).UTC() - if completedAtUnix != nil { - t := time.Unix(*completedAtUnix, 0).UTC() - snapshot.CompletedAt = &t - } - - snapshots = append(snapshots, &snapshot) - } - - return snapshots, rows.Err() + // Same column set as every other multi-row snapshot query, so the + // shared scanner applies — including its timestamp normalization. + return r.scanSnapshotRows(rows) } // Delete removes a snapshot record @@ -764,9 +730,16 @@ func (r *SnapshotRepository) scanSnapshotRows(rows *sql.Rows) ([]*Snapshot, erro return nil, fmt.Errorf("scanning snapshot: %w", err) } - snapshot.StartedAt = time.Unix(startedAtUnix, 0) + // UTC, matching every other snapshot scanner in this file. The + // column holds a bare Unix second, so the zone is a decode + // choice rather than stored data, and callers render these + // timestamps through zone-less format strings alongside + // timestamps read from remote manifests. Decoding in the host's + // local zone here would put two different wall clocks in one + // column. + snapshot.StartedAt = time.Unix(startedAtUnix, 0).UTC() if completedAtUnix != nil { - t := time.Unix(*completedAtUnix, 0) + t := time.Unix(*completedAtUnix, 0).UTC() snapshot.CompletedAt = &t } diff --git a/internal/database/snapshots_test.go b/internal/database/snapshots_test.go index 5204a61..9d07c8e 100644 --- a/internal/database/snapshots_test.go +++ b/internal/database/snapshots_test.go @@ -191,6 +191,119 @@ func TestSnapshotRepositoryListRecent(t *testing.T) { } } +// TestSnapshotTimestampsDecodeAsUTC pins the zone every snapshot reader +// returns. started_at and completed_at are stored as bare Unix seconds, +// so the zone is a decode choice, and callers (notably `snapshot list`) +// render these timestamps through zone-less format strings in the same +// column as timestamps read from remote manifests, which are always +// UTC. If one reader decodes in the host's local zone, that column +// silently shows two different wall clocks for the same instant. +// +// The assertions compare *time.Location pointers, so this fails on a +// UTC host too: time.Unix returns time.Local, which is never the same +// Location value as time.UTC no matter what the host's offset is. +func TestSnapshotTimestampsDecodeAsUTC(t *testing.T) { + t.Parallel() + + db, cleanup := setupTestDB(t) + defer cleanup() + + ctx := context.Background() + repo := database.NewSnapshotRepository(db) + + startedAt := time.Date(2026, 3, 1, 10, 0, 0, 0, time.UTC) + completedAt := startedAt.Add(time.Minute) + + completed := &database.Snapshot{ + ID: types.SnapshotID("testhost_home_2026-03-01T10:00:00Z"), + Hostname: testHostname, + VaultikVersion: testVersion, + StartedAt: startedAt, + CompletedAt: &completedAt, + } + + err := repo.Create(ctx, nil, completed) + if err != nil { + t.Fatalf("failed to create completed snapshot: %v", err) + } + + // An incomplete row as well, so the scanner shared by the two + // GetIncomplete* readers is covered with a nil completed_at too. + incomplete := &database.Snapshot{ + ID: types.SnapshotID("testhost_home_2026-03-02T10:00:00Z"), + Hostname: testHostname, + VaultikVersion: testVersion, + StartedAt: startedAt.Add(time.Hour), + CompletedAt: nil, + } + + err = repo.Create(ctx, nil, incomplete) + if err != nil { + t.Fatalf("failed to create incomplete snapshot: %v", err) + } + + byID, err := repo.GetByID(ctx, completed.ID.String()) + if err != nil { + t.Fatalf("failed to get snapshot by id: %v", err) + } + + recent, err := repo.ListRecent(ctx, 10) + if err != nil { + t.Fatalf("failed to list recent snapshots: %v", err) + } + + incompletes, err := repo.GetIncompleteSnapshots(ctx) + if err != nil { + t.Fatalf("failed to list incomplete snapshots: %v", err) + } + + byHost, err := repo.GetIncompleteByHostname(ctx, testHostname) + if err != nil { + t.Fatalf("failed to list incomplete snapshots by hostname: %v", err) + } + + read := make([]*database.Snapshot, 0, + 1+len(recent)+len(incompletes)+len(byHost)) + read = append(read, byID) + read = append(read, recent...) + read = append(read, incompletes...) + read = append(read, byHost...) + + if len(read) < 5 { + t.Fatalf("expected every reader to return rows, got %d", len(read)) + } + + assertTimestampsAreUTC(t, read) + + // And the wall clock is the UTC one, not the host's rendering of it. + rendered := byID.StartedAt.Format("2006-01-02 15:04:05") + if rendered != "2026-03-01 10:00:00" { + t.Errorf("started_at rendered as %q, want the UTC wall clock", rendered) + } +} + +// assertTimestampsAreUTC fails for any snapshot whose timestamps did not +// decode in UTC. It compares *time.Location pointers rather than +// offsets, so it is equally strict on a host whose local zone happens to +// be UTC: time.Unix returns time.Local, which is never the same Location +// value as time.UTC. +func assertTimestampsAreUTC(t *testing.T, snapshots []*database.Snapshot) { + t.Helper() + + for _, snapshot := range snapshots { + if snapshot.StartedAt.Location() != time.UTC { + t.Errorf("snapshot %s: started_at decoded in %s, want UTC", + snapshot.ID, snapshot.StartedAt.Location()) + } + + if snapshot.CompletedAt != nil && + snapshot.CompletedAt.Location() != time.UTC { + t.Errorf("snapshot %s: completed_at decoded in %s, want UTC", + snapshot.ID, snapshot.CompletedAt.Location()) + } + } +} + func TestSnapshotRepositoryNotFound(t *testing.T) { t.Parallel() diff --git a/internal/vaultik/snapshot_list.go b/internal/vaultik/snapshot_list.go index 70d228f..f4fcc8c 100644 --- a/internal/vaultik/snapshot_list.go +++ b/internal/vaultik/snapshot_list.go @@ -87,7 +87,7 @@ func (v *Vaultik) ListSnapshots(jsonOutput bool) error { snapshots = append(snapshots, info) } - listing, remoteErr := v.collectRemoteSnapshots(localKeys) + listing, remoteErr := v.collectRemoteSnapshots(localKeys, jsonOutput) if remoteErr != nil { v.warnRemoteListingFailed(remoteErr, jsonOutput) } else { @@ -95,11 +95,19 @@ func (v *Vaultik) ListSnapshots(jsonOutput bool) error { markRemotePresence(snapshots, listing.keys) } - sort.Slice(snapshots, func(i, j int) bool { + // Stable so that rows sharing a timestamp keep the order they were + // merged in, rather than depending on the sort's pivot choices. The + // unparseable-timestamp fallback in remoteSnapshotInfo makes ties + // realistic: every such row carries the zero time. + sort.SliceStable(snapshots, func(i, j int) bool { return snapshots[i].Timestamp.After(snapshots[j].Timestamp) }) if jsonOutput { + if remoteErr == nil { + v.reportJSONListingLimits(listing) + } + encoder := json.NewEncoder(v.Stdout) encoder.SetIndent("", " ") @@ -138,11 +146,79 @@ func (v *Vaultik) warnRemoteListingFailed(err error, jsonOutput bool) { return } - log.Warn("Could not list backup destination store", "error", err) + // Once only: the logger also writes to stdout, so emitting through + // both it and the UI would print the same sentence to the user twice. v.UI.Warningf("Could not list backup destination store: %v.", err) v.UI.Infof("Showing snapshots from the local index only.") } +// reportJSONListingLimits tells a --json consumer that the document it +// is about to read is incomplete: manifests that could not be read, and +// remote-only snapshots dropped by the maxRemoteOnlyRows cap. +// +// Table mode reports both below the table (see reportListDrift). In +// --json mode they cannot go on stdout — the document has to be the +// only thing there for `snapshot list --json | jq` to work — and the +// document's shape is deliberately left alone so existing consumers +// keep parsing. So they go to stderr, the same place the +// unreachable-destination warning already goes. A consumer that must +// react to truncation can treat any output on this stream as "this +// listing is not the whole picture"; silent truncation of a listing +// whose whole purpose is disaster recovery is the worse failure. +func (v *Vaultik) reportJSONListingLimits(listing *remoteSnapshotListing) { + if listing.unreadable > 0 { + _, _ = fmt.Fprintf(v.Stderr, + "Warning: %d remote snapshot(s) could not be described: "+ + "manifest missing or unreadable. They are missing from "+ + "this listing.\n", listing.unreadable) + } + + if listing.omitted > 0 { + _, _ = fmt.Fprintf(v.Stderr, + "Warning: listing truncated: %d further remote-only "+ + "snapshot(s) not shown (limit %d per listing).\n", + listing.omitted, maxRemoteOnlyRows) + } +} + +// kvPairSize is the number of variadic arguments that make up one +// structured logging key/value pair. +const kvPairSize = 2 + +// warnWhileListing reports a per-snapshot problem found while +// describing the destination store, through a writer that is safe for +// the current output mode. +// +// In --json mode it writes to v.Stderr rather than calling log.Warn, +// for the same reason warnRemoteListingFailed does: internal/log builds +// its logger over os.Stdout and defaults to level Warn, so one warning +// there would put a log line on stdout ahead of the JSON document and +// break `snapshot list --json | jq`. A single corrupt manifest is +// precisely the degradation this listing is built to survive, so it +// must not be the thing that corrupts the output. +// +// This is a local workaround. Remove it, and the branch in +// warnRemoteListingFailed, once issue #82 makes the logger's sink +// configurable. +func (v *Vaultik) warnWhileListing(jsonOutput bool, msg string, args ...any) { + if !jsonOutput { + log.Warn(msg, args...) + + return + } + + var line strings.Builder + + _, _ = fmt.Fprintf(&line, "Warning: %s", msg) + + for i := 0; i+kvPairSize <= len(args); i += kvPairSize { + pair := args[i : i+kvPairSize] + _, _ = fmt.Fprintf(&line, " %v=%v", pair[0], pair[1]) + } + + _, _ = fmt.Fprintln(v.Stderr, line.String()) +} + // remoteSnapshotListing is the result of one pass over the destination // store's metadata/ prefix. type remoteSnapshotListing struct { @@ -171,8 +247,11 @@ type remoteSnapshotListing struct { // Manifest reads scale only with the number of snapshots the local // index does not already know about, and are capped at // maxRemoteOnlyRows. +// +// jsonOutput only selects where per-snapshot warnings are written; see +// warnWhileListing. func (v *Vaultik) collectRemoteSnapshots( - localKeys map[string]bool, + localKeys map[string]bool, jsonOutput bool, ) (*remoteSnapshotListing, error) { keys, err := v.listAllRemoteSnapshotKeys() if err != nil { @@ -202,11 +281,23 @@ func (v *Vaultik) collectRemoteSnapshots( unknown = unknown[:maxRemoteOnlyRows] } - listing.remoteOnly, listing.unreadable = v.describeRemoteOnlySnapshots(unknown) + listing.remoteOnly, listing.unreadable = v.describeRemoteOnlySnapshots( + unknown, jsonOutput) return listing, nil } +// listingWarning is a problem found with one remote snapshot, recorded +// rather than emitted on the spot. Manifest reads run concurrently and +// the writer chosen by warnWhileListing is not guaranteed to be safe +// for concurrent use, so warnings are held until every read has +// finished and then emitted in key order from a single goroutine. That +// also makes the warning order deterministic run to run. +type listingWarning struct { + msg string + args []any +} + // describeRemoteOnlySnapshots reads the manifest for each supplied // remote key and turns it into a table row, returning the rows and the // number of keys whose manifest could not be read. @@ -214,9 +305,12 @@ func (v *Vaultik) collectRemoteSnapshots( // A key whose manifest is missing or corrupt is skipped rather than // failing the listing: one bad snapshot directory must not hide every // other snapshot the user has. -func (v *Vaultik) describeRemoteOnlySnapshots(keys []string) ([]SnapshotInfo, int) { +func (v *Vaultik) describeRemoteOnlySnapshots( + keys []string, jsonOutput bool, +) ([]SnapshotInfo, int) { found := make([]SnapshotInfo, len(keys)) ok := make([]bool, len(keys)) + warnings := make([]*listingWarning, len(keys)) var group errgroup.Group @@ -224,15 +318,22 @@ func (v *Vaultik) describeRemoteOnlySnapshots(keys []string) ([]SnapshotInfo, in for i, key := range keys { group.Go(func() error { - info, err := v.remoteSnapshotInfo(key) + info, warning, err := v.remoteSnapshotInfo(key) if err != nil { - log.Warn("Could not describe remote snapshot", - "remote_key", key, "error", err) + warnings[i] = &listingWarning{ + msg: "Could not describe remote snapshot", + args: []any{"remote_key", key, "error", err}, + } - return nil + // Deliberately not returned: the failure is carried in + // warnings/ok and reported as a count. Returning it + // would cancel the group and let one bad snapshot + // directory hide every other snapshot the user has. + return nil //nolint:nilerr // see above } found[i] = info + warnings[i] = warning ok[i] = true return nil @@ -247,6 +348,10 @@ func (v *Vaultik) describeRemoteOnlySnapshots(keys []string) ([]SnapshotInfo, in unreadable := 0 for i := range keys { + if warnings[i] != nil { + v.warnWhileListing(jsonOutput, warnings[i].msg, warnings[i].args...) + } + if !ok[i] { unreadable++ @@ -269,19 +374,34 @@ func (v *Vaultik) describeRemoteOnlySnapshots(keys []string) ([]SnapshotInfo, in // the privacy property that hashing the key exists to provide (see // issue #81). The renderer marks the row as unnamed rather than // guessing. -func (v *Vaultik) remoteSnapshotInfo(remoteKey string) (SnapshotInfo, error) { +// +// The returned warning, when non-nil, is a problem worth telling the +// user about that was not bad enough to drop the row. It is returned +// rather than logged because this runs on a worker goroutine; see +// listingWarning. +func (v *Vaultik) remoteSnapshotInfo( + remoteKey string, +) (SnapshotInfo, *listingWarning, error) { manifest, err := v.downloadManifestByKey(remoteKey) if err != nil { - return SnapshotInfo{}, err + return SnapshotInfo{}, nil, err } + var warning *listingWarning + timestamp, err := time.Parse(time.RFC3339, manifest.Timestamp) if err != nil { // The snapshot is really there; an unparseable timestamp is not // reason enough to hide it. It sorts to the bottom as the zero // time. - log.Warn("Remote manifest has an unparseable timestamp", - "remote_key", remoteKey, "timestamp", manifest.Timestamp, "error", err) + warning = &listingWarning{ + msg: "Remote manifest has an unparseable timestamp", + args: []any{ + "remote_key", remoteKey, + "timestamp", manifest.Timestamp, + "error", err, + }, + } timestamp = time.Time{} } @@ -291,7 +411,7 @@ func (v *Vaultik) remoteSnapshotInfo(remoteKey string) (SnapshotInfo, error) { Timestamp: timestamp.UTC(), CompressedSize: manifest.TotalCompressedSize, LocallyTracked: false, - }, nil + }, warning, nil } // markRemotePresence records, for every row, whether its remote key was @@ -461,14 +581,16 @@ func (v *Vaultik) printSnapshotTable(snapshots []SnapshotInfo) error { } for _, snap := range snapshots { - id := formatRemoteOnlyID(snap.RemoteKey) - uncompressed := remoteOnlyCell - newChunks := remoteOnlyCell + var id, uncompressed, newChunks string if snap.LocallyTracked { id = snap.ID.String() uncompressed = formatBytes(snap.UncompressedSize) newChunks = formatBytes(snap.NewChunkSize) + } else { + id = formatRemoteOnlyID(snap.RemoteKey) + uncompressed = remoteOnlyCell + newChunks = remoteOnlyCell } _, err = fmt.Fprintf(w, "%s\t%s\t%s\t%s\t%s\n", diff --git a/internal/vaultik/snapshot_list_test.go b/internal/vaultik/snapshot_list_test.go index b5280ed..3547e77 100644 --- a/internal/vaultik/snapshot_list_test.go +++ b/internal/vaultik/snapshot_list_test.go @@ -6,7 +6,9 @@ import ( "database/sql" "encoding/json" "errors" + "fmt" "io" + "os" "strings" "sync" "testing" @@ -171,12 +173,24 @@ func (e *listEnv) addRemote( ) string { t.Helper() + return e.addRemoteRawTimestamp(t, snapshotID, + timestamp.UTC().Format(time.RFC3339)) +} + +// addRemoteRawTimestamp is addRemote with the manifest's timestamp field +// written verbatim, so the unparseable-timestamp path can be exercised +// with a value no time.Parse will accept. +func (e *listEnv) addRemoteRawTimestamp( + t *testing.T, snapshotID, timestamp string, +) string { + t.Helper() + remoteKey := snapshot.RemoteSnapshotKey(snapshotID) manifest := &snapshot.Manifest{ // Note: the hashed key, never the human ID. That is precisely // why a remote-only snapshot cannot be named. SnapshotID: remoteKey, - Timestamp: timestamp.UTC().Format(time.RFC3339), + Timestamp: timestamp, BlobCount: 1, TotalCompressedSize: fiveMegabytes, Blobs: []snapshot.BlobInfo{ @@ -431,6 +445,7 @@ func TestListSnapshots_UnreadableManifestDoesNotHideOthers(t *testing.T) { type listJSONRow struct { ID string `json:"id"` RemoteKey string `json:"remote_key"` + Timestamp string `json:"timestamp"` CompressedSize int64 `json:"compressed_size"` LocallyTracked bool `json:"locally_tracked"` RemotePresent *bool `json:"remote_present"` @@ -531,3 +546,256 @@ func TestListSnapshots_JSONUnreachableRemote(t *testing.T) { "could not list backup destination store") assert.Contains(t, env.stderr.String(), "permission denied") } + +// useNonUTCLocalZone points time.Local at a fixed non-UTC zone for the +// duration of the test. +// +// Snapshot timestamps are stored as bare Unix seconds, so the zone a +// reader decodes them in is a decode choice rather than stored data — +// and on a UTC host a wrong choice is invisible. This makes it visible: +// with time.Local at +07:13, a row decoded in local time renders 7h13m +// away from the same instant decoded in UTC. +// +// time.Local is process-global, so a test using this must not call +// t.Parallel. Go runs every non-parallel test to completion before +// resuming any parallel one, so the mutation is not observable from +// another test. +// +//nolint:gosmopolitan // pinning time.Local is the entire point here +func useNonUTCLocalZone(t *testing.T) { + t.Helper() + + const offsetSeconds = 7*60*60 + 13*60 + + previous := time.Local + time.Local = time.FixedZone("VaultikTest", offsetSeconds) + + t.Cleanup(func() { time.Local = previous }) +} + +// TestListSnapshots_TimestampsAreUTCOnNonUTCHost is the regression guard +// for the merged TIMESTAMP column. Local rows come from the index +// database and remote-only rows come from a manifest; both render +// through the same zone-less format string, so both have to be in the +// same zone or the column silently shows two different wall clocks for +// the same instant. +// +// This test fails on any host if either source stops normalizing to UTC, +// because it pins time.Local to a zone that is not UTC. +// +//nolint:paralleltest // pins process-global time.Local; see useNonUTCLocalZone +func TestListSnapshots_TimestampsAreUTCOnNonUTCHost(t *testing.T) { + log.Initialize(log.Config{}) + useNonUTCLocalZone(t) + + // One instant, rendered twice: once through a locally tracked + // snapshot and once through a snapshot only the destination store + // knows about. + instant := time.Date(2026, 3, 1, 10, 0, 0, 0, time.UTC) + + const wallClock = "2026-03-01 10:00:00" + + env := newListEnv(t) + env.addLocal(t, listLocalID, instant) + env.addRemote(t, listLocalID, instant) + remoteKey := env.addRemote(t, listRemoteID, instant) + + err := env.v.ListSnapshots(false) + require.NoError(t, err) + + out := env.stdout.String() + + assert.Contains(t, findTableRow(t, out, listLocalID), wallClock, + "a locally tracked row must render in UTC like every other row") + assert.Contains(t, + findTableRow(t, out, ""), wallClock) + + // The --json timestamp carries its zone explicitly, so rows from the + // two sources must be string-comparable as well. + jsonEnv := newListEnv(t) + jsonEnv.addLocal(t, listLocalID, instant) + jsonEnv.addRemote(t, listLocalID, instant) + jsonEnv.addRemote(t, listRemoteID, instant) + + err = jsonEnv.v.ListSnapshots(true) + require.NoError(t, err) + + rows := decodeListJSON(t, jsonEnv.stdout.String()) + require.Len(t, rows, 2) + + for _, row := range rows { + assert.Equal(t, "2026-03-01T10:00:00Z", row.Timestamp, + "--json timestamps must be comparable between row types") + } +} + +// TestListSnapshots_JSONReportsUnreadableManifests checks that a +// snapshot missing from the JSON document because its manifest could not +// be read is still announced. Table mode says so below the table; a +// machine consumer would otherwise see no difference between "that +// snapshot is not on the destination" and "that snapshot could not be +// read". +func TestListSnapshots_JSONReportsUnreadableManifests(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + env := newListEnv(t) + + goodKey := env.addRemote(t, listRemoteID, + time.Date(2026, 3, 2, 11, 22, 33, 0, time.UTC)) + + badKey := snapshot.RemoteSnapshotKey("testhost_broken_2026-03-03T00:00:00Z") + err := env.store.Put(context.Background(), + "metadata/"+badKey+"/manifest.json.zst", + strings.NewReader("this is not a zstd stream")) + require.NoError(t, err) + + err = env.v.ListSnapshots(true) + require.NoError(t, err) + + rows := decodeListJSON(t, env.stdout.String()) + require.Len(t, rows, 1) + assert.Equal(t, goodKey, rows[0].RemoteKey) + + assert.Contains(t, env.stderr.String(), + "1 remote snapshot(s) could not be described", + "a row dropped from the JSON document must be announced somewhere") +} + +// maxRemoteOnlyRowsForTest mirrors the maxRemoteOnlyRows cap in the +// package under test, which is unexported. +const maxRemoteOnlyRowsForTest = 1000 + +// TestListSnapshots_JSONReportsTruncation covers the row cap in --json +// mode. Past the cap the document is a partial listing, and silent +// truncation of a listing whose whole purpose is disaster recovery is +// the wrong failure mode: the consumer least able to notice is exactly +// the one reading JSON. +func TestListSnapshots_JSONReportsTruncation(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + env := newListEnv(t) + + timestamp := time.Date(2026, 3, 2, 11, 22, 33, 0, time.UTC) + + // One past the cap, so exactly one snapshot is omitted. + for i := range maxRemoteOnlyRowsForTest + 1 { + env.addRemote(t, fmt.Sprintf("otherhost_bulk_%04d", i), timestamp) + } + + err := env.v.ListSnapshots(true) + require.NoError(t, err) + + rows := decodeListJSON(t, env.stdout.String()) + assert.Len(t, rows, maxRemoteOnlyRowsForTest) + + assert.Contains(t, env.stderr.String(), "listing truncated") + assert.Contains(t, env.stderr.String(), "1 further remote-only") +} + +// captureProcessStdout redirects the process's own stdout to a pipe, +// rebuilds the global logger over it, runs fn, and returns everything +// written. +// +// internal/log builds its logger over os.Stdout at construction time and +// offers no injectable sink (issue #82), so a warning logged during a +// --json listing lands on the process's real stdout, not on any writer a +// test can inject. Capturing the file descriptor is therefore the only +// way a test can see what `snapshot list --json | jq` would see. +// +// Not parallel-safe: os.Stdout and the logger are process-global. +func captureProcessStdout(t *testing.T, fn func(stdout io.Writer)) string { + t.Helper() + + reader, writer, err := os.Pipe() + require.NoError(t, err) + + previous := os.Stdout + os.Stdout = writer + + // Rebuild the logger so it writes to the pipe rather than to the + // real stdout the test process was started with. + log.Initialize(log.Config{}) + + drained := make(chan string, 1) + + go func() { + var buf bytes.Buffer + + _, _ = io.Copy(&buf, reader) + + drained <- buf.String() + }() + + fn(writer) + + os.Stdout = previous + + require.NoError(t, writer.Close()) + + captured := <-drained + + require.NoError(t, reader.Close()) + + // Put the logger back on the restored stdout. + log.Initialize(log.Config{}) + + return captured +} + +// TestListSnapshots_JSONStdoutIsOnlyTheDocument is the regression guard +// for `snapshot list --json | jq` surviving a damaged destination store. +// +// Every stdout writer the command has — the JSON encoder, the UI, and +// the global logger — is pointed at one pipe here, exactly as they are +// pointed at one file descriptor in production. A single log line about +// a corrupt manifest ahead of the array is enough to break the parse, +// and that is what this asserts cannot happen. +// +//nolint:paralleltest // replaces os.Stdout and the global logger +func TestListSnapshots_JSONStdoutIsOnlyTheDocument(t *testing.T) { + env := newListEnv(t) + + goodKey := env.addRemote(t, listRemoteID, + time.Date(2026, 3, 2, 11, 22, 33, 0, time.UTC)) + + // A manifest that is not even a zstd stream. + badKey := snapshot.RemoteSnapshotKey("testhost_broken_2026-03-03T00:00:00Z") + err := env.store.Put(context.Background(), + "metadata/"+badKey+"/manifest.json.zst", + strings.NewReader("this is not a zstd stream")) + require.NoError(t, err) + + // And a manifest that decodes but carries a timestamp no parser will + // accept: the second warning on this path. + oddKey := env.addRemoteRawTimestamp(t, + "testhost_odd_2026-03-04T00:00:00Z", "the day before yesterday") + + captured := captureProcessStdout(t, func(stdout io.Writer) { + env.v.Stdout = stdout + env.v.UI = ui.NewWithColor(stdout, false) + + require.NoError(t, env.v.ListSnapshots(true)) + }) + + rows := decodeListJSON(t, captured) + require.Len(t, rows, 2, "the readable snapshots must both be listed") + + byKey := make(map[string]listJSONRow, len(rows)) + for _, row := range rows { + byKey[row.RemoteKey] = row + } + + assert.Contains(t, byKey, goodKey) + assert.Contains(t, byKey, oddKey, + "an unparseable timestamp must not hide the snapshot itself") + assert.NotContains(t, byKey, badKey) + + // Both warnings were emitted, on the stream that cannot corrupt the + // document. + stderr := env.stderr.String() + assert.Contains(t, stderr, "Could not describe remote snapshot") + assert.Contains(t, stderr, "Remote manifest has an unparseable timestamp") + assert.Contains(t, stderr, "1 remote snapshot(s) could not be described") +} -- 2.49.1