Make config set create a list for an index under a missing key (closes #308)

`config set age_recipients.0 age1...` on a file without `age_recipients`
wrote a map keyed `0`, after which no command could load the config. A
missing key is now created as a list when the next path element is a
number.

`config set` checks its result with the config loader before writing and
refuses a failure the change brings in: a config that loaded must still
load, and one still being filled in (failing validation until complete)
must still decode into the config type. The new `config.Parse`, split out
of `config.Load`, decodes config data, filling in `${...}` references,
without validating.

Rule suppressed: gosec G304 where `config.Load` and the new tests read a
file.

Model: opus-5-5

README.md

@@ -274,11 +274,13 @@ existing file. Created with mode `0600` since it will contain credentials.
 formatting in the file are preserved. Missing keys along the path are
 created: as a list when the next path element is a number (so
 `vaultik config set age_recipients.0 age1...` works on a file without
-`age_recipients`), otherwise as a map. A change that gives a setting the
-wrong type, such as a single value where a list belongs or a word where
-a number belongs, is refused and the file is left unchanged. Whether the
-config is complete and its values are in range is checked when a command
-loads it, not by `config set`, so settings can be set one at a time.
+`age_recipients`), otherwise as a map. If the config loads, a change
+after which it would not, such as `vaultik config set compression_level 99`,
+is refused and the file is left unchanged. A config that does not load
+yet, such as the one `config init` writes before `storage_url` is set,
+can be filled in one setting at a time: only a change that gives a
+setting the wrong type, such as a single value where a list belongs or a
+word where a number belongs, is refused.
 
 **`snapshot create`**: Perform incremental backup of configured snapshots.
 * Optional snapshot names argument to create specific snapshots (default: all)

internal/cli/config.go

@@ -45,7 +45,8 @@ var (
 	errIndexOutOfRange  = errors.New("index out of range")
 	errNotMapOrList     = errors.New("key is not a map or list")
 	errWouldNotLoad     = errors.New(
-		"the config would no longer load, so the file was not changed")
+		"vaultik would not load the config after this change, " +
+			"so the file was not changed")
 )
 
 const defaultConfigTemplate = `# vaultik configuration
@@ -395,9 +396,13 @@ func newConfigSetCommand() *cobra.Command {
 		Long: `Sets a scalar config value addressed by dotted YAML path and writes
 the file back, preserving comments and formatting. Missing keys along
 the path are created: as a list when the next path element is a
-number, otherwise as a map. A change that gives a setting the wrong
-type, such as a single value where a list belongs, is refused and the
-file is left unchanged.
+number, otherwise as a map.
+
+If the config loads, a change after which it would not, such as
+compression_level 99, is refused and the file is left unchanged. A
+config that does not load yet, such as the one config init writes
+before storage_url is set, can be filled in one setting at a time:
+only a change that gives a setting the wrong type is refused.
 
 Examples:
   vaultik config set storage_url "file:///mnt/backups"
@@ -427,6 +432,11 @@ func writeConfigSet(out *ui.Writer, path, key, value string) error {
 		return err
 	}
 
+	oldData, err := marshalConfigYAML(root)
+	if err != nil {
+		return fmt.Errorf("marshaling config: %w", err)
+	}
+
 	err = yamlPathSet(root, strings.Split(key, "."), value)
 	if err != nil {
 		return err
@@ -437,10 +447,7 @@ func writeConfigSet(out *ui.Writer, path, key, value string) error {
 		return fmt.Errorf("marshaling config: %w", err)
 	}
 
-	// Refuse a result that config.Load could not decode. Load also
-	// validates the config; that is left out here because settings are set
-	// one at a time, so a config being filled in is incomplete in between.
-	err = yaml.Unmarshal(data, &config.Config{})
+	err = checkConfigChange(oldData, data)
 	if err != nil {
 		return fmt.Errorf("%w: %w", errWouldNotLoad, err)
 	}
@@ -466,6 +473,28 @@ func writeConfigSet(out *ui.Writer, path, key, value string) error {
 	return nil
 }
 
+// checkConfigChange returns the reason vaultik would not load newData, the
+// config after a change, if the change brings that failure in. If oldData,
+// the config before it, decodes into the config type, the new one must
+// too; if the old one also passes validation, the new one must pass it. A
+// config still being filled in fails validation until it is complete, so
+// only decoding is checked for it.
+func checkConfigChange(oldData, newData []byte) error {
+	oldCfg, oldErr := config.Parse(oldData)
+	if oldErr == nil {
+		newCfg, err := config.Parse(newData)
+		if err != nil {
+			return err
+		}
+
+		if oldCfg.Validate() == nil {
+			return newCfg.Validate()
+		}
+	}
+
+	return nil
+}
+
 // marshalConfigYAML renders a config document tree with 2-space indentation,
 // matching defaultConfigTemplate. yaml.Marshal defaults to 4 spaces, which
 // would reindent the whole file on the first `config set` despite the promise

internal/cli/config_test.go

@@ -64,6 +64,7 @@ const testRecipient = "age1278m9q7dp3chsh2dcy82qk27v047zywyvtxwnj4cvt0z65jw6a7q5
 // TestConfigSetRecipientOnFreshConfig follows the README quickstart: on the
 // file `config init` writes, `config set age_recipients.0` and
 // `config set storage_url` give a config that loads with that recipient.
+// Neither is refused, although the config does not load until both are set.
 func TestConfigSetRecipientOnFreshConfig(t *testing.T) {
 	t.Parallel()
 
@@ -152,38 +153,55 @@ func TestConfigSetIndexUnderMissingKey(t *testing.T) {
 }
 
 // TestConfigSetKeepsFileThatWouldNotLoad checks that config set refuses a
-// change after which the config would not load, and leaves the file as it
-// was.
+// change after which the config would not load, with an error giving the
+// reason, and leaves the file as it was. On the file `config init` writes,
+// which does not load until storage_url is set, only a value of the wrong
+// type is refused.
 func TestConfigSetKeepsFileThatWouldNotLoad(t *testing.T) {
 	t.Parallel()
 
 	tests := []struct {
-		key   string
-		value string
+		config string
+		key    string
+		value  string
+		reason string
 	}{
 		// Index 1 of the empty list the missing key gives.
-		{"age_recipients.1", testRecipient},
+		{configWithoutRecipients, "age_recipients.1", testRecipient,
+			"index out of range"},
 		// A list set to a single value.
-		{"snapshots.home.paths", "/srv"},
-		{"compression_level", "fast"},
+		{configWithoutRecipients, "snapshots.home.paths", "/srv",
+			"cannot unmarshal"},
+		{configWithoutRecipients, "compression_level", "fast",
+			"cannot unmarshal"},
+		// Values of the right type that fail validation.
+		{configWithoutRecipients, "compression_level", "99",
+			"compression_level must be between 1 and 19"},
+		{configWithoutRecipients, "chunk_size", "1KB",
+			"chunk_size must be at least 1MB"},
+		{configWithoutRecipients, "age_recipients.0", "notakey",
+			"not a valid recipient"},
+		// The type check on a config that does not load yet.
+		{defaultConfigTemplate, "snapshots.apps.paths", "/srv",
+			"cannot unmarshal"},
 	}
 
 	for _, tt := range tests {
-		t.Run(tt.key, func(t *testing.T) {
+		t.Run(tt.key+" "+tt.value, func(t *testing.T) {
 			t.Parallel()
 
 			path := filepath.Join(t.TempDir(), "config.yml")
 
-			err := os.WriteFile(path, []byte(configWithoutRecipients), configFileMode)
+			err := os.WriteFile(path, []byte(tt.config), configFileMode)
 			if err != nil {
 				t.Fatalf("write config: %v", err)
 			}
 
 			err = writeConfigSet(ui.NewWithColor(&bytes.Buffer{}, false),
 				path, tt.key, tt.value)
-			if err == nil {
-				t.Errorf("config set %s %s succeeded, want an error",
-					tt.key, tt.value)
+			if err == nil || !strings.Contains(err.Error(), tt.reason) {
+				t.Errorf("config set %s %s gives error %v, want one saying %q",
+					tt.key, tt.value, err, tt.reason)
 			}
 
 			data, err := os.ReadFile(path) //nolint:gosec // G304: test-controlled path
@@ -191,7 +209,7 @@ func TestConfigSetKeepsFileThatWouldNotLoad(t *testing.T) {
 				t.Fatalf("read config: %v", err)
 			}
 
-			if string(data) != configWithoutRecipients {
+			if string(data) != tt.config {
 				t.Errorf("config set %s changed the file to:\n%s",
 					tt.key, data)
 			}
@@ -199,6 +217,55 @@ func TestConfigSetKeepsFileThatWouldNotLoad(t *testing.T) {
 	}
 }
 
+// TestConfigSetRefusesOnlyItsOwnFailure checks that setting another key
+// works on a config with a ${...} reference on compression_level, which
+// loads because loading fills the reference in, and on one whose
+// compression_level already fails to load.
+func TestConfigSetRefusesOnlyItsOwnFailure(t *testing.T) {
+	t.Parallel()
+
+	levelPath := filepath.Join(t.TempDir(), "level")
+
+	err := os.WriteFile(levelPath, []byte("9"), configFileMode)
+	if err != nil {
+		t.Fatalf("write level: %v", err)
+	}
+
+	configs := map[string]string{
+		"reference": configWithoutRecipients +
+			"compression_level: ${FILE:" + levelPath + "}\n",
+		"failing": configWithoutRecipients + "compression_level: fast\n",
+	}
+
+	for name, configYAML := range configs {
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			path := filepath.Join(t.TempDir(), "config.yml")
+
+			err := os.WriteFile(path, []byte(configYAML), configFileMode)
+			if err != nil {
+				t.Fatalf("write config: %v", err)
+			}
+
+			err = writeConfigSet(ui.NewWithColor(&bytes.Buffer{}, false),
+				path, "hostname", "myhost")
+			if err != nil {
+				t.Fatalf("config set hostname: %v", err)
+			}
+
+			data, err := os.ReadFile(path) //nolint:gosec // G304: test-controlled path
+			if err != nil {
+				t.Fatalf("read config: %v", err)
+			}
+
+			if !strings.Contains(string(data), "hostname: myhost") {
+				t.Errorf("config set hostname wrote:\n%s", data)
+			}
+		})
+	}
+}
+
 // TestConfigSetStringLooksLikeNumber sets string settings to values that
 // YAML reads as numbers or booleans when they are unquoted, and checks that
 // config.Load returns each one unchanged.

internal/config/config.go

@@ -4,6 +4,7 @@
 package config
 
 import (
+	"bytes"
 	"errors"
 	"fmt"
 	"os"
@@ -233,14 +234,48 @@ func New(path Path) (*Config, error) {
 	return cfg, nil
 }
 
-// Load reads and parses the configuration file from the specified path.
-// It applies default values for optional fields, performs environment variable
-// substitution using smartconfig, and validates the configuration.
-// The configuration file should be in YAML format. Returns an error if the file
-// cannot be read, parsed, or if validation fails.
+// Load reads the configuration file at path, parses it with Parse, warns
+// when others can read the file, and validates the configuration.
+// Returns an error if the file cannot be read, parsed, or if validation
+// fails.
 func Load(path string) (*Config, error) {
+	data, err := os.ReadFile(path) //nolint:gosec // G304: config path is operator-supplied
+	if err != nil {
+		return nil, fmt.Errorf("failed to load config file: %w", err)
+	}
+
+	cfg, err := Parse(data)
+	if err != nil {
+		return nil, err
+	}
+
+	// Check config file permissions (warn if world or group readable)
+	//nolint:gosec // G703: config path is operator-supplied by design
+	info, statErr := os.Stat(path)
+	if statErr == nil {
+		mode := info.Mode().Perm()
+		if mode&0044 != 0 { // group or world readable
+			log.Warn(cfg.readableByOthersWarning(),
+				"path", path,
+				"mode", fmt.Sprintf("%04o", mode),
+				"recommendation", "chmod 600 "+path)
+		}
+	}
+
+	err = cfg.Validate()
+	if err != nil {
+		return nil, fmt.Errorf("invalid config: %w", err)
+	}
+
+	return cfg, nil
+}
+
+// Parse reads YAML config data as Load does, without validating it. It
+// applies default values for optional fields, fills in ${...} references
+// using smartconfig, and expands ~ in paths.
+func Parse(data []byte) (*Config, error) {
 	// Load config using smartconfig for interpolation
-	sc, err := smartconfig.NewFromConfigPath(path)
+	sc, err := smartconfig.NewFromReader(bytes.NewReader(data))
 	if err != nil {
 		return nil, fmt.Errorf("failed to load config file: %w", err)
 	}
@@ -302,24 +337,6 @@ func Load(path string) (*Config, error) {
 		cfg.S3.Region = "us-east-1"
 	}
 
-	// Check config file permissions (warn if world or group readable)
-	//nolint:gosec // G703: config path is operator-supplied by design
-	info, statErr := os.Stat(path)
-	if statErr == nil {
-		mode := info.Mode().Perm()
-		if mode&0044 != 0 { // group or world readable
-			log.Warn(cfg.readableByOthersWarning(),
-				"path", path,
-				"mode", fmt.Sprintf("%04o", mode),
-				"recommendation", "chmod 600 "+path)
-		}
-	}
-
-	err = cfg.Validate()
-	if err != nil {
-		return nil, fmt.Errorf("invalid config: %w", err)
-	}
-
 	return cfg, nil
 }
 

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")
+		})
+	}
+}