Stop a stalled blob read when restore or deep verify is interrupted (closes #325)

Restore and deep verify looked for a cancel only between blobs, and the
file backend's reader ignores the context. A blob read stalled on a
slow USB stick or network share outlasted the 30-second shutdown
timeout, so the process exited before removing the decrypted snapshot
database and blob cache from the temp directory.

FetchBlob, through which both read every blob, now closes the storage
reader as soon as the context is cancelled, ending a blocked read on
any backend; Read then returns the context's error, not the closed
reader's.

Limitation: a read stuck in the kernel that closing cannot interrupt,
such as one on an unresponsive hard NFS mount, still cannot be stopped.

Model: opus-5-5

README.md

@@ -433,7 +433,10 @@ object store. Credentials are read from `s3.access_key_id` and
 `s3.secret_access_key` in the config file.
 
 **Local filesystem** (`file:///path/to/backup`): Stores blobs and metadata on
-a local or mounted filesystem. Useful for testing or backing up to a NAS.
+a local or mounted filesystem. Useful for testing or backing up to a NAS. The
+path must be absolute; a relative one is refused when the config loads.
+Percent-escapes in the path are decoded, so `my%20stick` and `my stick` name
+the same directory.
 
 **Rclone** (`rclone://remote/path`): Uses rclone's 70+ supported cloud
 providers. Requires rclone to be configured separately (`rclone config`).

internal/config/config.go

@@ -61,6 +61,9 @@ var (
 	errBadS3PartSize    = errors.New("s3.part_size must be between 5MiB and 5GiB")
 	errBadStorageScheme = errors.New(
 		"storage_url must start with s3://, file://, or rclone://")
+	errFileURLNotAbsolute = errors.New(
+		"a file:// storage_url needs an absolute path, " +
+			"as in file:///path/to/backup")
 	errStorageNotConfigured = errors.New(
 		"storage not configured; set storage_url or provide s3.endpoint + " +
 			"s3.bucket + credentials")
@@ -461,11 +464,18 @@ func (c *Config) validateStorage() error {
 
 // validateStorageURL validates URL-based storage configuration. File and
 // rclone URLs need no credentials; S3 URLs require the legacy s3.*
-// credential fields.
+// credential fields. A file URL must name an absolute path: a relative one
+// would resolve against the working directory, so the destination would
+// move with it.
 func (c *Config) validateStorageURL() error {
 	switch {
 	case strings.HasPrefix(c.StorageURL, "file://"):
-		// File storage doesn't need S3 credentials
+		// storage.ParseStorageURL applies the same rule, but config
+		// cannot import storage, which imports config.
+		if !strings.HasPrefix(c.StorageURL, "file:///") {
+			return errFileURLNotAbsolute
+		}
+
 		return nil
 	case strings.HasPrefix(c.StorageURL, "rclone://"):
 		// Rclone storage uses rclone's own config

internal/config/config_test.go

@@ -337,6 +337,35 @@ func TestLoadS3PartSize(t *testing.T) {
 	})
 }
 
+// TestLoadRejectsRelativeFileURL checks that a file:// storage_url with a
+// relative path fails at load. Accepted, it would resolve against the
+// working directory, so a backup run from another directory would write
+// to a different destination and still succeed.
+func TestLoadRejectsRelativeFileURL(t *testing.T) {
+	t.Parallel()
+
+	const relativeConfig = "snapshots:\n" +
+		"  test:\n" +
+		"    paths: [/tmp/vaultik-test-source]\n" +
+		"storage_url: file://dest\n"
+
+	path := filepath.Join(t.TempDir(), "config.yml")
+
+	err := os.WriteFile(path, []byte(relativeConfig), 0o600)
+	if err != nil {
+		t.Fatalf("write config: %v", err)
+	}
+
+	_, err = Load(path)
+	if !errors.Is(err, errFileURLNotAbsolute) {
+		t.Fatalf("Load() error = %v, want errFileURLNotAbsolute", err)
+	}
+
+	if !strings.Contains(err.Error(), "absolute path") {
+		t.Errorf("error %q does not say to use an absolute path", err.Error())
+	}
+}
+
 // TestValidateAgeRecipients checks that recipients are parsed at config load
 // (a bad entry fails immediately, not mid-backup) and that no invalid entry —
 // least of all a pasted secret key — is echoed in the error. An empty list

internal/storage/url.go

@@ -17,8 +17,10 @@ const (
 
 // Sentinel errors for storage URL parsing.
 var (
-	ErrEmptyStorageURL   = errors.New("storage URL is empty")
-	ErrEmptyFilePath     = errors.New("file URL path is empty")
+	ErrEmptyStorageURL  = errors.New("storage URL is empty")
+	ErrEmptyFilePath    = errors.New("file URL path is empty")
+	ErrRelativeFilePath = errors.New(
+		"file URL needs an absolute path, as in file:///path/to/backup")
 	ErrMissingBucket     = errors.New("s3 URL missing bucket name")
 	ErrMissingRemote     = errors.New("rclone URL missing remote name")
 	ErrUnsupportedScheme = errors.New(
@@ -51,17 +53,8 @@ func ParseStorageURL(rawURL string) (*URL, error) {
 		return nil, ErrEmptyStorageURL
 	}
 
-	// Handle file:// URLs
 	if after, ok := strings.CutPrefix(rawURL, "file://"); ok {
-		path := after
-		if path == "" {
-			return nil, ErrEmptyFilePath
-		}
-
-		return &URL{
-			Scheme: schemeFile,
-			Prefix: path,
-		}, nil
+		return parseFileURL(after)
 	}
 
 	if strings.HasPrefix(rawURL, "s3://") {
@@ -75,6 +68,27 @@ func ParseStorageURL(rawURL string) (*URL, error) {
 	return nil, ErrUnsupportedScheme
 }
 
+// parseFileURL parses the path that follows file://. The path must be
+// absolute: a relative one would resolve against the working directory,
+// so the destination would move with it. Percent-escapes are decoded, so
+// my%20stick names the directory "my stick"; a literal space is kept.
+func parseFileURL(rawPath string) (*URL, error) {
+	if rawPath == "" {
+		return nil, ErrEmptyFilePath
+	}
+
+	if !strings.HasPrefix(rawPath, "/") {
+		return nil, ErrRelativeFilePath
+	}
+
+	path, err := url.PathUnescape(rawPath)
+	if err != nil {
+		return nil, fmt.Errorf("invalid file URL path: %w", err)
+	}
+
+	return &URL{Scheme: schemeFile, Prefix: path}, nil
+}
+
 // parseS3URL parses an s3://bucket/prefix URL. It rejects credentials in
 // the userinfo and any query parameter other than endpoint, region and
 // ssl, so a credential-bearing URL is never stored or echoed.

internal/storage/url_parse_test.go

@@ -14,7 +14,10 @@ import (
 func TestParseStorageURLValid(t *testing.T) {
 	t.Parallel()
 
-	const bucket = "mybucket"
+	const (
+		bucket     = "mybucket"
+		fileScheme = "file"
+	)
 
 	cases := []struct {
 		name string
@@ -24,7 +27,17 @@ func TestParseStorageURLValid(t *testing.T) {
 		{
 			name: "file absolute path",
 			raw:  "file:///var/backups/vaultik",
-			want: &storage.URL{Scheme: "file", Prefix: "/var/backups/vaultik"},
+			want: &storage.URL{Scheme: fileScheme, Prefix: "/var/backups/vaultik"},
+		},
+		{
+			name: "file percent-escaped path is decoded",
+			raw:  "file:///Volumes/my%20stick/backup",
+			want: &storage.URL{Scheme: fileScheme, Prefix: "/Volumes/my stick/backup"},
+		},
+		{
+			name: "file path with a literal space",
+			raw:  "file:///Volumes/my stick/backup",
+			want: &storage.URL{Scheme: fileScheme, Prefix: "/Volumes/my stick/backup"},
 		},
 		{
 			name: "s3 bucket and prefix, ssl defaults on",
@@ -90,6 +103,7 @@ func TestParseStorageURLErrors(t *testing.T) {
 	}{
 		{"empty url", "", storage.ErrEmptyStorageURL},
 		{"file empty path", "file://", storage.ErrEmptyFilePath},
+		{"file relative path", "file://dest", storage.ErrRelativeFilePath},
 		{"s3 missing bucket", "s3://", storage.ErrMissingBucket},
 		{"s3 missing bucket with path", "s3:///justprefix", storage.ErrMissingBucket},
 		{"rclone missing remote", "rclone://", storage.ErrMissingRemote},

internal/vaultik/blob_fetch.go

@@ -103,6 +103,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.
 func (v *Vaultik) FetchBlob(
 	ctx context.Context, blobHash string,
 ) (io.ReadCloser, error) {
@@ -120,5 +122,47 @@ func (v *Vaultik) FetchBlob(
 		return nil, fmt.Errorf("downloading blob %s: %w", shortHash(blobHash), err)
 	}
 
-	return rc, nil
+	return newCloseOnCancelReader(ctx, rc), nil
+}
+
+// closeOnCancelReader closes the reader it wraps when ctx is cancelled.
+// Not every backend's reader stops on cancel by itself (the file
+// backend's does not), and a read stalled on a slow or hung source would
+// otherwise outlast the shutdown timeout, so the process would exit
+// before restore or deep verify removed its decrypted temporary files.
+type closeOnCancelReader struct {
+	io.ReadCloser
+
+	ctx  context.Context //nolint:containedctx // Read reports its error
+	stop func() bool
+}
+
+func newCloseOnCancelReader(
+	ctx context.Context, rc io.ReadCloser,
+) *closeOnCancelReader {
+	return &closeOnCancelReader{
+		ReadCloser: rc,
+		ctx:        ctx,
+		stop:       context.AfterFunc(ctx, func() { _ = rc.Close() }),
+	}
+}
+
+// Read returns the context's error, not the closed reader's, once ctx
+// has been cancelled.
+func (r *closeOnCancelReader) Read(p []byte) (int, error) {
+	n, err := r.ReadCloser.Read(p)
+	if err != nil && r.ctx.Err() != nil {
+		return n, r.ctx.Err()
+	}
+
+	return n, err
+}
+
+// Close closes the wrapped reader, unless the cancel already closed it.
+func (r *closeOnCancelReader) Close() error {
+	if !r.stop() {
+		return nil
+	}
+
+	return r.ReadCloser.Close()
 }

internal/vaultik/restore_interrupt_test.go

@@ -142,14 +142,14 @@ func TestRestoreCleansTempDirOnInterrupt(t *testing.T) {
 }
 
 // scratchEntries returns the vaultik blob-cache and snapshot-database
-// scratch entries currently present in dir.
+// scratch entries (restore's and deep verify's) currently present in dir.
 func scratchEntries(t *testing.T, dir string) []string {
 	t.Helper()
 
 	var matches []string
 
 	for _, pattern := range []string{
-		"vaultik-blobcache-*", "vaultik-restore-*",
+		"vaultik-blobcache-*", "vaultik-restore-*", "vaultik-verify-*",
 	} {
 		found, err := filepath.Glob(filepath.Join(dir, pattern))
 		require.NoError(t, err)
@@ -206,3 +206,119 @@ func TestRestoreSkipErrorsCancelDuringBlobDownload(t *testing.T) {
 	assert.NotContains(t, out.String(), srcPath,
 		"the cancel was reported as a failed file")
 }
+
+// stallingBlobStorer wraps a Storer so that every blob download hands
+// over the first half of the blob and then stalls, ignoring the context,
+// as a file:// blob on a hung USB stick or network share does. Only
+// closing the returned reader ends the stall.
+type stallingBlobStorer struct {
+	storage.Storer
+
+	once    sync.Once
+	stalled chan struct{}
+}
+
+func newStallingBlobStorer(inner storage.Storer) *stallingBlobStorer {
+	return &stallingBlobStorer{Storer: inner, stalled: make(chan struct{})}
+}
+
+func (s *stallingBlobStorer) Get(
+	ctx context.Context, key string,
+) (io.ReadCloser, error) {
+	rc, err := s.Storer.Get(ctx, key)
+	if err != nil || !strings.HasPrefix(key, "blobs/") {
+		return rc, err
+	}
+
+	blob, err := io.ReadAll(rc)
+	_ = rc.Close()
+
+	if err != nil {
+		return nil, err
+	}
+
+	// The writer is never closed, so once the first half has been read
+	// the next Read blocks until the reader is closed.
+	pr, pw := io.Pipe()
+
+	go func() {
+		_, _ = pw.Write(blob[:len(blob)/2])
+
+		s.once.Do(func() { close(s.stalled) })
+	}()
+
+	return pr, nil
+}
+
+// TestInterruptDuringStalledBlobReadCleansTempDir stops a restore and a
+// deep verify while a blob read is stalled on a source that ignores the
+// context. Stopping must end that read, so the operation unwinds and
+// removes its decrypted scratch files well within the shutdown timeout
+// (issue #325).
+//
+// Not parallel: it points TMPDIR at a private directory (via t.Setenv)
+// so it can assert on exactly the scratch files the operation created.
+func TestInterruptDuringStalledBlobReadCleansTempDir(t *testing.T) {
+	log.Initialize(log.Config{})
+
+	operations := map[string]func(v *Vaultik, snapshotID, targetDir string) error{
+		"restore": func(v *Vaultik, snapshotID, targetDir string) error {
+			return v.Restore(&RestoreOptions{
+				SnapshotID: snapshotID,
+				TargetDir:  targetDir,
+			})
+		},
+		"deep verify": func(v *Vaultik, snapshotID, _ string) error {
+			return v.RunDeepVerify(snapshotID, &VerifyOptions{Deep: true})
+		},
+	}
+
+	for name, op := range operations {
+		t.Run(name, func(t *testing.T) {
+			fs := afero.NewOsFs()
+			root := t.TempDir()
+
+			// Random content, so the blob stays large enough to stall
+			// part-way through its compressed, encrypted body.
+			cfg, storer, snapshotID, _ := backupOneFile(context.Background(),
+				t, fs, root, "a.bin", randomBytes(t, 256*1024), 0o644)
+
+			scratch := filepath.Join(root, "scratch")
+			require.NoError(t, fs.MkdirAll(scratch, 0o755))
+			t.Setenv("TMPDIR", scratch)
+
+			stalling := newStallingBlobStorer(storer)
+
+			ctx, cancel := context.WithCancel(context.Background())
+			defer cancel()
+
+			v := newRestoreVaultik(ctx, cfg, stalling, fs)
+			v.cancel = cancel
+
+			var opErr error
+
+			stop := v.StartOperation(func() {
+				opErr = op(v, snapshotID, filepath.Join(root, "restored"))
+			})
+
+			select {
+			case <-stalling.stalled:
+			case <-time.After(30 * time.Second):
+				t.Fatal("the operation never reached a blob read")
+			}
+
+			require.NotEmpty(t, scratchEntries(t, scratch),
+				"expected decrypted scratch files to exist mid-read")
+
+			stopCtx, stopCancel := context.WithTimeout(
+				context.Background(), 10*time.Second)
+			defer stopCancel()
+
+			require.True(t, stop(stopCtx),
+				"stop timed out; the stalled blob read was not interrupted")
+			require.ErrorIs(t, opErr, context.Canceled)
+			require.Empty(t, scratchEntries(t, scratch),
+				"decrypted scratch files remained after the interrupt")
+		})
+	}
+}