From 5fd9d8e07b2231ed281af54a2f0575dc03148946 Mon Sep 17 00:00:00 2001 From: sneak Date: Tue, 13 Oct 2026 22:07:36 +0000 Subject: [PATCH] Name each prefix once in restore, verify and purge errors (closes #327) A missing blob during restore, a failed shallow verify and an invalid purge duration each printed their prefix twice. FetchBlob now returns the storage error as is, since restore and deep verify name the blob themselves; purge returns the duration parser's error unchanged; and a failed shallow verify returns only what failed, because the command prints "Verification failed:" before it. Deep verify words a storage error other than not found as a blob it could not check, and its log line no longer says S3. Already fixed for https://git.eeqj.de/sneak/vaultik/issues/314: the deep verify size message naming S3 and other errors called missing; tests now check both. Judgement call: the shallow verify error is a small error type, since the linter refuses an error built from a plain string. Not tested: the log line. Model: opus-5-5 --- internal/vaultik/blob_fetch.go | 5 +++-- internal/vaultik/deep_verify_test.go | 11 +++++++++-- internal/vaultik/purge_per_name_test.go | 3 ++- internal/vaultik/restore_skip_errors_test.go | 5 ++++- internal/vaultik/shallow_verify_test.go | 7 ++++++- internal/vaultik/snapshot.go | 11 +++++++++-- internal/vaultik/verify.go | 4 ++-- 7 files changed, 35 insertions(+), 11 deletions(-) diff --git a/internal/vaultik/blob_fetch.go b/internal/vaultik/blob_fetch.go index aa00243..4672921 100644 --- a/internal/vaultik/blob_fetch.go +++ b/internal/vaultik/blob_fetch.go @@ -104,7 +104,8 @@ func (v *Vaultik) FetchAndDecryptBlob( // FetchBlob downloads a blob and returns a reader for the encrypted data. // The reader is closed as soon as ctx is cancelled, which ends a read -// blocked inside it. +// blocked inside it. A storage error is returned as is: restore and deep +// verify name the blob in their own errors. func (v *Vaultik) FetchBlob( ctx context.Context, blobHash string, ) (io.ReadCloser, error) { @@ -119,7 +120,7 @@ func (v *Vaultik) FetchBlob( rc, err := v.Storage.Get(ctx, blobPath) if err != nil { - return nil, fmt.Errorf("downloading blob %s: %w", shortHash(blobHash), err) + return nil, err } return newCloseOnCancelReader(ctx, rc), nil diff --git a/internal/vaultik/deep_verify_test.go b/internal/vaultik/deep_verify_test.go index 0592935..0651bf2 100644 --- a/internal/vaultik/deep_verify_test.go +++ b/internal/vaultik/deep_verify_test.go @@ -6,6 +6,7 @@ import ( "crypto/rand" "io" "os" + "path" "path/filepath" "testing" @@ -142,7 +143,8 @@ func TestDeepVerifyJSONCountsBlobs(t *testing.T) { // TestDeepVerifyJSONCountsWrongSizeBlob checks that under --deep --json a // blob of the wrong size is counted in mismatched and fails the run, and -// that the check goes on past it to count a missing blob after it. +// that the check goes on past it to count a missing blob after it. The +// store is on disk, so the message must not call it S3. func TestDeepVerifyJSONCountsWrongSizeBlob(t *testing.T) { log.Initialize(log.Config{}) t.Parallel() @@ -168,11 +170,14 @@ func TestDeepVerifyJSONCountsWrongSizeBlob(t *testing.T) { require.Equal(t, 1, got.Mismatched) require.Equal(t, 1, got.Missing) require.Zero(t, got.Verified) + require.Contains(t, got.ErrorMessage, "with the wrong size") + require.NotContains(t, got.ErrorMessage, "S3") } // TestDeepVerifyStopsOnBlobThatCannotBeChecked checks that a blob that is // present in storage but cannot be checked is not counted as missing: -// deep verify stops and fails with the storage error. +// deep verify stops and fails, saying it could not check the blob, with +// the storage error. func TestDeepVerifyStopsOnBlobThatCannotBeChecked(t *testing.T) { log.Initialize(log.Config{}) t.Parallel() @@ -187,6 +192,8 @@ func TestDeepVerifyStopsOnBlobThatCannotBeChecked(t *testing.T) { require.Error(t, err) require.Equal(t, "failed", got.Status) require.Zero(t, got.Missing) + require.Contains(t, got.ErrorMessage, + "could not check blob "+path.Base(keys[0])) require.Contains(t, got.ErrorMessage, errStatDenied.Error()) } diff --git a/internal/vaultik/purge_per_name_test.go b/internal/vaultik/purge_per_name_test.go index 8b537b8..700bf43 100644 --- a/internal/vaultik/purge_per_name_test.go +++ b/internal/vaultik/purge_per_name_test.go @@ -281,7 +281,8 @@ func TestPurgeOlderThan_TooLargeDeletesNothing(t *testing.T) { OlderThan: "1000y", Force: true, }) - require.Error(t, err) + require.EqualError(t, err, + `invalid duration: "1000y" is longer than about 292 years`) err = v.PurgeSnapshotsWithOptions(&vaultik.SnapshotPurgeOptions{ OlderThan: "292y", diff --git a/internal/vaultik/restore_skip_errors_test.go b/internal/vaultik/restore_skip_errors_test.go index 376c833..311d21d 100644 --- a/internal/vaultik/restore_skip_errors_test.go +++ b/internal/vaultik/restore_skip_errors_test.go @@ -231,7 +231,8 @@ func (f trailingByteFile) Close() error { } // TestRestoreMissingBlobAbortsWithoutSkipErrors checks that a deleted blob -// still ends the restore with an error when SkipErrors is not set. +// still ends the restore with an error when SkipErrors is not set, and +// that the error names the blob once. func TestRestoreMissingBlobAbortsWithoutSkipErrors(t *testing.T) { log.Initialize(log.Config{}) t.Parallel() @@ -247,6 +248,8 @@ func TestRestoreMissingBlobAbortsWithoutSkipErrors(t *testing.T) { }) require.ErrorIs(t, err, storage.ErrNotFound) + assert.Equal(t, 1, strings.Count(err.Error(), "downloading blob"), + err.Error()) } // TestRestoreChunkInNoBlobAbortsWithoutSkipErrors checks that a chunk held diff --git a/internal/vaultik/shallow_verify_test.go b/internal/vaultik/shallow_verify_test.go index 3633cf6..0f9c08e 100644 --- a/internal/vaultik/shallow_verify_test.go +++ b/internal/vaultik/shallow_verify_test.go @@ -27,7 +27,7 @@ import ( // then grows one stored blob so its size no longer matches the manifest. // Shallow verify must then fail, count the grown blob as a size mismatch, // and drop it from the verified count rather than continuing to report it -// as checked. +// as checked, and its error must not repeat the cli layer's prefix. func TestShallowVerifyDetectsWrongBlobSize(t *testing.T) { log.Initialize(log.Config{}) t.Parallel() @@ -96,6 +96,11 @@ func TestShallowVerifyDetectsWrongBlobSize(t *testing.T) { require.Equal(t, 1, bad.Mismatched) require.Equal(t, healthy.BlobCount-1, bad.Verified, "the wrong-sized blob must not be counted as verified") + + // The cli layer prints this error after its own "Verification failed:". + err = newVerifier(io.Discard).VerifySnapshotWithOptions( + snapshotID, &vaultik.VerifyOptions{}) + require.EqualError(t, err, "1 blobs have the wrong size") } // TestShallowVerifyDetectsMissingDatabase backs up a real snapshot, diff --git a/internal/vaultik/snapshot.go b/internal/vaultik/snapshot.go index 63a9809..61e4a91 100644 --- a/internal/vaultik/snapshot.go +++ b/internal/vaultik/snapshot.go @@ -612,7 +612,7 @@ func selectSnapshotsToPurge( case opts.OlderThan != "": duration, err := parseDuration(opts.OlderThan) if err != nil { - return nil, fmt.Errorf("invalid duration: %w", err) + return nil, err } cutoff := time.Now().UTC().Add(-duration) @@ -907,6 +907,13 @@ func (v *Vaultik) verifyManifestBlobs( return verified, missing, mismatched, missingSize, unchecked, nil } +// shallowVerifyError is the error a failed shallow verify returns. Its +// text is only what failed, because the cli layer prints it after its own +// "Verification failed:". +type shallowVerifyError string + +func (e shallowVerifyError) Error() string { return string(e) } + // formatVerifyResult outputs the final verification results as JSON or // human-readable text. func (v *Vaultik) formatVerifyResult( @@ -942,7 +949,7 @@ func (v *Vaultik) formatVerifyResult( } if failure != "" { - return fmt.Errorf("%w: %s", errSnapshotVerifyFailed, failure) + return shallowVerifyError(failure) } return nil diff --git a/internal/vaultik/verify.go b/internal/vaultik/verify.go index 1fd298c..6c88d93 100644 --- a/internal/vaultik/verify.go +++ b/internal/vaultik/verify.go @@ -680,7 +680,7 @@ func (v *Vaultik) verifyManifestAgainstDatabase( func (v *Vaultik) verifyBlobExistenceFromDB( blobs []snapshot.BlobInfo, result *VerifyResult, ) error { - log.Info("Verifying blob existence in S3", "blob_count", len(blobs)) + log.Info("Verifying blob existence in storage", "blob_count", len(blobs)) for i, blob := range blobs { // The hash is read from the snapshot database, which is not @@ -700,7 +700,7 @@ func (v *Vaultik) verifyBlobExistenceFromDB( result.Missing++ result.MissingSize += blob.CompressedSize case err != nil: - return fmt.Errorf("checking blob %s in storage: %w", + return fmt.Errorf("could not check blob %s in storage: %w", blob.Hash, err) case stat.Size != blob.CompressedSize: log.Warn("Blob has the wrong size in storage",