Reject secret mv onto the same secret under another name (closes #78)

On a case-insensitive filesystem (the macOS default) "Foo" and "foo" name
one secret, so `secret mv --force Foo foo` removed the destination, which
was the source, and lost the secret with every version. Between vaults the
copy replaced the source, and removing the source then removed the copy.

Both kinds of move now compare the two secret directories with
os.SameFile before changing anything and reject the move if they are one,
with or without --force. The tests give one secret two names with
symbolic links on the real filesystem.

Model: opus-5-5

Dockerfile

@@ -9,7 +9,8 @@ RUN go mod download
 COPY . .
 
 RUN make fmt-check
-RUN make lint
+# Not make lint: script/lint is a docker build, which cannot run in here.
+RUN golangci-lint run --config .golangci.yml ./...
 
 # Build stage — tests and compilation
 # golang 1.24.13-alpine (2026-03-10)

Dockerfile.lint

@@ -0,0 +1,19 @@
+# Lint image, built by script/lint: golangci-lint runs as a build step, so a
+# successful build is a clean lint. Works where the docker daemon is remote
+# and bind mounts are impossible.
+
+# golangci/golangci-lint:v2.12.2 (Debian-based), 2026-08-07
+FROM golangci/golangci-lint:v2.12.2@sha256:5cceeef04e53efe1470638d4b4b4f5ceefd574955ab3941b2d9a68a8c9ad5240 AS deps
+
+WORKDIR /src
+
+COPY go.mod go.sum ./
+RUN go mod download
+
+# script/lint rebuilds this stage on every run, by this name; the module
+# download above stays cached.
+FROM deps AS lint
+
+COPY . .
+
+RUN golangci-lint run --config .golangci.yml ./...

README.md

@@ -139,6 +139,9 @@ matching.
 
 Moves or renames a secret within the current vault.
 - Fails if the destination already exists
+- Fails if the destination is the source under another name, such as `foo`
+  for `Foo` on a case-insensitive filesystem (the macOS default); there, to
+  change only the case of a name, move the secret to a third name first
 - Preserves all versions and metadata
 
 ### Version Management
@@ -496,15 +499,18 @@ standard: normalized scripts in `script/` are the entrypoints for the
 development workflow, and the Makefile targets are thin shims that call
 them. We provide:
 
-- `script/bootstrap` — install all dependencies (Go, golangci-lint, Go
-  module download), idempotently
+- `script/bootstrap` — install all dependencies (Go, Go module
+  download), idempotently; golangci-lint is not installed, it runs in
+  docker
 - `script/setup` — make a fresh clone ready for development: runs
   `script/bootstrap`, then `script/install-precommit`
 - `script/projectname` — output the project name (`secret`); used by
   other scripts such as `script/docker`
 - `script/test` — run `go vet` and the test suite (verbose rerun on
   failure)
-- `script/lint` — run `golangci-lint`
+- `script/lint` — run `golangci-lint` in docker only: builds
+  `Dockerfile.lint`, where the linter is a build step that runs on every
+  call, also on an unchanged tree
 - `script/fmt` — format all Go code (writes)
 - `script/fmt-check` — check formatting without writing
 - `script/check` — run `script/test`, `script/lint`, and

TODO.md

@@ -25,6 +25,63 @@ Bring the repo into policy compliance in one commit:
 
 # Completed Steps
 
+- 2026-10-04: `secret mv` rejects a move whose destination is the source
+  under another name, such as `foo` for `Foo` on a case-insensitive
+  filesystem (the macOS default) or a name reached through a symbolic
+  link, before changing anything, with or without `--force`, within a
+  vault and between vaults; before, `--force` removed the destination and
+  so deleted the secret. A rename that changes only letter case works on a
+  case-sensitive filesystem as before.
+- 2026-10-04: Lint runs only in docker: `script/lint` builds
+  `Dockerfile.lint`, where golangci-lint is a build step rebuilt on
+  every run (`--no-cache-filter`), so an unchanged tree is linted too;
+  the module download stays cached. `script/bootstrap` no longer
+  installs golangci-lint, and the `Dockerfile` lint stage calls it
+  directly instead of `make lint`. `golangci-lint config verify` is not
+  run: it fetches its schema live over unpinned HTTPS.
+- 2026-10-04: A PGP unlocker whose metadata has no usable GPG key ID
+  no longer panics: `GetID()` warns with the unlocker's directory and
+  returns `pgp-unknown`. `ListUnlockers` skips, with a warning, an
+  unlocker whose metadata file cannot be checked for, read or parsed
+  instead of failing, so `secret unlocker list` still lists the others;
+  the listing's ID lookup no longer warns about that directory again.
+- 2026-10-03: `secret mv` rejects a move whose destination is the
+  source (`mv --force x x`, `mv --force work:x work:`, or an empty
+  destination, which defaults to the source name) before changing
+  anything; before, `--force` removed the destination first and so
+  deleted the secret. Every vault name given with `vault:` must be one
+  of the existing vaults by exact name, so `work:x work/:x` is rejected
+  instead of being taken for a move between two vaults. A move within a
+  named vault no longer makes that vault the current one, whether it
+  succeeds or fails.
+- 2026-10-03: Commands that change the state directory hold one lock
+  (`flock` on `lock` in the state directory; a mutex on the in-memory
+  test filesystem), so concurrent commands no longer lose versions or
+  race on the current pointers. Every file is written through
+  `secret.WriteFileAtomic` (temporary file, sync, rename), so no file
+  is ever half-written and `current`, `currentvault` and
+  `current-unlocker` never go missing. New versions, new secrets and
+  cross-vault copies are built in a temporary directory and renamed
+  into place, and removals rename out of the way first, so a version
+  or secret is never half-added and never half-removed. An
+  interrupted command can still leave:
+    - a broken unlocker, when it was replacing one: an unlocker added
+      under the directory name of an existing one is rewritten file by
+      file. That happens to a passphrase unlocker added to a vault that
+      has one, and to a PGP, keychain or Secure Enclave unlocker added
+      on the same host and day as another of its type
+      (https://git.eeqj.de/sneak/secret/issues/71);
+    - from `vault create` stopped at the passphrase prompt, a new vault
+      with no unlocker that is already the current vault; from `init`
+      stopped there, the default vault with no unlocker;
+    - from an unlocker add stopped before its metadata is written, a
+      directory that `unlocker list` warns about and `unlocker rm`
+      cannot remove;
+    - data under a `.tmp-` name in the state directory: a secret or
+      version being added, or the secret, version, unlocker or vault
+      being removed, encrypted keys included. Nothing deletes it; it
+      must be deleted by hand
+      (https://git.eeqj.de/sneak/secret/issues/75).
 - 2026-10-03: `version rm`, `version promote` and `get --version`
   accept a version only if it is one of the versions `version list`
   lists for that secret, compared as typed before any path is built
@@ -119,14 +176,10 @@ Bring the repo into policy compliance in one commit:
       version.go:155); age secret key held in a plain string in
       cli/crypto.go:86,91,113; private keys exposed via buffer.Bytes()
       to GPGEncryptFunc and EncryptWithPassphrase.
-    - Race conditions: no file locking in vault/secrets.go:142-176;
-      non-atomic writes can leave the vault inconsistent.
     - Input validation: no maximum secret size (DoS).
     - Timing attacks: bytes.Equal passphrase compare (cli/init.go:
       209-216); non-constant-time public key compare (vault.go:95-100).
 - High priority:
-    - Return errors instead of panicking on corrupted metadata
-      (pgpunlocker.go:116, keychainunlocker.go:141).
     - Secure temporary file handling and cleanup.
     - Print cobra usage only for argument errors, not internal
       failures.

internal/cli/crypto.go

@@ -70,9 +70,44 @@ func newDecryptCmd() *cobra.Command {
 	)
 }
 
+// storeNewEncryptionKey generates an age secret key and stores it as the
+// named secret, holding the state directory lock while it does. It fails
+// with vault.ErrSecretExists if another command stored the secret first.
+// The caller must destroy the returned buffer.
+func (cli *Instance) storeNewEncryptionKey(
+	vlt *vault.Vault, secretName string,
+) (*memguard.LockedBuffer, error) {
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return nil, err
+	}
+	defer release()
+
+	identity, err := age.GenerateX25519Identity()
+	if err != nil {
+		return nil, fmt.Errorf("failed to generate age key: %w", err)
+	}
+
+	// Store the generated key directly in a secure buffer
+	secureBuffer := memguard.NewBufferFromBytes([]byte(identity.String()))
+
+	err = vlt.AddSecret(secretName, secureBuffer, false)
+	if err != nil {
+		secureBuffer.Destroy()
+
+		return nil, fmt.Errorf("failed to store age key: %w", err)
+	}
+
+	return secureBuffer, nil
+}
+
 // resolveEncryptionKey returns a secure buffer holding the age secret key
 // for the named secret, generating and storing a new key if the secret
-// does not exist. The caller must destroy the returned buffer.
+// does not exist. The caller must destroy the returned buffer. Only storing
+// a new key takes the state directory lock, so that reading an existing key
+// works on a read-only state directory and keeps no other command waiting
+// at the passphrase prompt, and Encrypt streams its input and output
+// unlocked.
 func (cli *Instance) resolveEncryptionKey(
 	vlt *vault.Vault, secretName string,
 ) (*memguard.LockedBuffer, error) {
@@ -85,23 +120,11 @@ func (cli *Instance) resolveEncryptionKey(
 	}
 
 	if !exists {
-		// Secret doesn't exist, generate new age key and store it
-		identity, err := age.GenerateX25519Identity()
-		if err != nil {
-			return nil, fmt.Errorf("failed to generate age key: %w", err)
+		key, err := cli.storeNewEncryptionKey(vlt, secretName)
+		if !errors.Is(err, vault.ErrSecretExists) {
+			return key, err
 		}
-
-		// Store the generated key directly in a secure buffer
-		secureBuffer := memguard.NewBufferFromBytes([]byte(identity.String()))
-
-		err = vlt.AddSecret(secretName, secureBuffer, false)
-		if err != nil {
-			secureBuffer.Destroy()
-
-			return nil, fmt.Errorf("failed to store age key: %w", err)
-		}
-
-		return secureBuffer, nil
+		// Another command stored the key since the check above: read it
 	}
 
 	// Secret exists, get the age secret key from it

internal/cli/generate.go

@@ -155,6 +155,12 @@ func (cli *Instance) GenerateSecret(
 		return fmt.Errorf("failed to generate random secret: %w", err)
 	}
 
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
 	// Store the secret in the vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {

internal/cli/init.go

@@ -103,8 +103,21 @@ func (cli *Instance) setupDefaultVault(
 	return vlt, ltIdentity, nil
 }
 
-// Init initializes the secret manager
+// Init initializes the secret manager, holding the state directory lock
+// while initialize runs
 func (cli *Instance) Init(cmd *cobra.Command) error {
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
+	return cli.initialize(cmd)
+}
+
+// initialize creates the state directory, the default vault and its first
+// unlocker
+func (cli *Instance) initialize(cmd *cobra.Command) error {
 	secret.Debug("Starting secret manager initialization")
 
 	// Create state directory

internal/cli/lock_test.go

@@ -0,0 +1,529 @@
+//nolint:testpackage // sets the unexported fields of Instance
+package cli
+
+import (
+	"io"
+	"os"
+	"path/filepath"
+	"strconv"
+	"strings"
+	"sync"
+	"testing"
+	"time"
+
+	"git.eeqj.de/sneak/secret/internal/secret"
+	"git.eeqj.de/sneak/secret/internal/vault"
+	"github.com/awnumar/memguard"
+	"github.com/spf13/afero"
+	"github.com/spf13/cobra"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+const (
+	// lockWait is how long a test waits for something that must happen
+	// once the lock is free.
+	lockWait = 10 * time.Second
+
+	// heldWait is how long a test watches a command that must wait for the
+	// lock. A command that takes no lock changes the state directory well
+	// within it.
+	heldWait = 100 * time.Millisecond
+
+	// testPassphrase protects the passphrase unlockers the tests create.
+	testPassphrase = "test-passphrase"
+
+	// testInput is a file outside the state directory that commands read.
+	testInput = "/input"
+)
+
+// lockInBackground starts taking the state directory lock and returns a
+// channel that delivers the function releasing it once it has been taken.
+func lockInBackground(t *testing.T, fs afero.Fs) <-chan func() {
+	t.Helper()
+
+	taken := make(chan func(), 1)
+
+	go func() {
+		release, err := vault.LockStateDir(fs, testStateDir)
+		if assert.NoError(t, err) {
+			taken <- release
+		}
+	}()
+
+	return taken
+}
+
+// addAtOnce runs one add of the secret name per value, all at once, and
+// returns their errors.
+func addAtOnce(
+	fs afero.Fs, stateDir, name string, force bool, values []string,
+) []error {
+	errs := make(chan error, len(values))
+
+	for _, value := range values {
+		go func() {
+			cli := NewCLIInstanceWithStateDir(fs, stateDir)
+			cli.cmd = &cobra.Command{}
+			cli.cmd.SetIn(strings.NewReader(value))
+
+			errs <- cli.AddSecret(name, force)
+		}()
+	}
+
+	results := make([]error, 0, len(values))
+	for range values {
+		results = append(results, <-errs)
+	}
+
+	return results
+}
+
+// numbered returns count distinct values starting with prefix.
+func numbered(prefix string, count int) []string {
+	values := make([]string, 0, count)
+	for i := range count {
+		values = append(values, prefix+"-"+strconv.Itoa(i))
+	}
+
+	return values
+}
+
+// TestConcurrentAddsKeepEveryVersion runs adds of one secret at once, on
+// the in-memory and on the real filesystem. Without the state directory
+// lock, adds of a new secret all find it absent and replace each other, and
+// forced adds read the same highest version number and overwrite each
+// other's version. With it they behave as if run one after another.
+//
+//nolint:paralleltest // t.Setenv forbids parallel subtests
+func TestConcurrentAddsKeepEveryVersion(t *testing.T) {
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+
+	const adds = 8
+
+	for _, tc := range []struct {
+		name     string
+		fs       afero.Fs
+		stateDir string
+	}{
+		{"memory", afero.NewMemMapFs(), testStateDir},
+		{"real", afero.NewOsFs(), t.TempDir()},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			_, err := vault.CreateVault(tc.fs, tc.stateDir, "default")
+			require.NoError(t, err)
+
+			// One add creates the secret; the others find that it exists
+			created := 0
+
+			for _, err := range addAtOnce(tc.fs, tc.stateDir, "shared", false,
+				numbered("create", adds)) {
+				if err == nil {
+					created++
+				} else {
+					require.ErrorIs(t, err, vault.ErrSecretExists)
+				}
+			}
+
+			require.Equal(t, 1, created, "exactly one add creates the secret")
+
+			// Every forced add stores a version of its own
+			for _, err := range addAtOnce(tc.fs, tc.stateDir, "shared", true,
+				numbered("force", adds)) {
+				require.NoError(t, err)
+			}
+
+			vlt, err := vault.GetCurrentVault(tc.fs, tc.stateDir)
+			require.NoError(t, err)
+
+			vaultDir, err := vlt.GetDirectory()
+			require.NoError(t, err)
+
+			versions, err := secret.ListVersions(tc.fs,
+				filepath.Join(vaultDir, "secrets.d", "shared"))
+			require.NoError(t, err)
+			require.Len(t, versions, adds+1, "one version per successful add")
+
+			values := make(map[string]bool, len(versions))
+
+			for _, version := range versions {
+				value, err := vlt.GetSecretVersion("shared", version)
+				require.NoError(t, err)
+
+				values[string(value)] = true
+			}
+
+			assert.Len(t, values, adds+1, "every add stored its own value")
+		})
+	}
+}
+
+// readNotifier passes reads through to Reader and closes reading at the
+// first one.
+type readNotifier struct {
+	io.Reader
+
+	reading chan struct{}
+	once    sync.Once
+}
+
+func (r *readNotifier) Read(p []byte) (int, error) {
+	r.once.Do(func() { close(r.reading) })
+
+	return r.Reader.Read(p)
+}
+
+// TestEncryptPipedIntoAdd runs `secret encrypt key | secret add name` in
+// one process, starting encrypt once add is reading its input. Had add
+// taken the state directory lock before reading, it would hold the lock
+// while waiting for encrypt's output, and encrypt would wait for the lock
+// to store its key: neither would finish.
+func TestEncryptPipedIntoAdd(t *testing.T) {
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+
+	fs := afero.NewMemMapFs()
+	_, err := vault.CreateVault(fs, testStateDir, "default")
+	require.NoError(t, err)
+	require.NoError(t, afero.WriteFile(fs, testInput, []byte("piped"), 0o600))
+
+	pipeReader, pipeWriter := io.Pipe()
+	// If the test gives up, this makes add's read fail, so that both
+	// commands return and release the lock the other tests use
+	t.Cleanup(func() { _ = pipeReader.Close() })
+
+	const commands = 2
+
+	input := &readNotifier{Reader: pipeReader, reading: make(chan struct{})}
+	results := make(chan error, commands)
+
+	go func() {
+		add := NewCLIInstanceWithStateDir(fs, testStateDir)
+		add.cmd = &cobra.Command{}
+		add.cmd.SetIn(input)
+
+		results <- add.AddSecret("encrypted", false)
+	}()
+
+	go func() {
+		<-input.reading
+
+		encrypt := NewCLIInstanceWithStateDir(fs, testStateDir)
+		encrypt.cmd = &cobra.Command{}
+		encrypt.cmd.SetOut(pipeWriter)
+
+		err := encrypt.Encrypt("key", testInput, "")
+		// Ends add's input, as the end of the pipe does
+		_ = pipeWriter.CloseWithError(err)
+
+		results <- err
+	}()
+
+	timeout := time.After(lockWait)
+
+	for range commands {
+		select {
+		case err := <-results:
+			require.NoError(t, err)
+		case <-timeout:
+			t.Fatal("secret encrypt piped into secret add never finished")
+		}
+	}
+}
+
+// TestFailedCommandReleasesLock checks that a command failing after it
+// took the state directory lock leaves the lock free for the next command.
+func TestFailedCommandReleasesLock(t *testing.T) {
+	t.Parallel()
+
+	fs := afero.NewMemMapFs()
+	cli := NewCLIInstanceWithStateDir(fs, testStateDir)
+
+	// Fails once it holds the lock: there is no current vault
+	err := cli.RemoveSecret(&cobra.Command{}, "missing", false)
+	require.Error(t, err)
+
+	select {
+	case release := <-lockInBackground(t, fs):
+		release()
+	case <-time.After(lockWait):
+		t.Fatal("the failed command left the state directory locked")
+	}
+}
+
+// stateDirModTimes returns the modification time of every file and
+// directory under the test state directory. Any change a command makes, even
+// rewriting a file with the same content, changes it.
+func stateDirModTimes(t *testing.T, fs afero.Fs) map[string]int64 {
+	t.Helper()
+
+	modTimes := make(map[string]int64)
+
+	err := afero.Walk(fs, testStateDir,
+		func(path string, info os.FileInfo, err error) error {
+			if err != nil {
+				return err
+			}
+
+			modTimes[path] = info.ModTime().UnixNano()
+
+			return nil
+		})
+	require.NoError(t, err)
+
+	return modTimes
+}
+
+// setupEveryCommand makes what each command in
+// TestChangingCommandsWaitForLock needs: the current vault "default" with
+// two versions of "test/secret", the vault "other" without a long-term key,
+// for vault import, and the file testInput. If withUnlocker is set, it also
+// gives "default" a passphrase unlocker, which is slow. It returns the older
+// version and the unlocker's ID.
+func setupEveryCommand(
+	t *testing.T, fs afero.Fs, withUnlocker bool,
+) (string, string) {
+	t.Helper()
+
+	other, err := vault.CreateVault(fs, testStateDir, "other")
+	require.NoError(t, err)
+
+	otherDir, err := other.GetDirectory()
+	require.NoError(t, err)
+	require.NoError(t, fs.Remove(filepath.Join(otherDir, "pub.age")))
+
+	vlt, err := vault.CreateVault(fs, testStateDir, "default")
+	require.NoError(t, err)
+
+	addTestSecret(t, vlt, []byte("older"), false)
+	addTestSecret(t, vlt, []byte("newer"), true)
+
+	vaultDir, err := vlt.GetDirectory()
+	require.NoError(t, err)
+
+	versions, err := secret.ListVersions(fs,
+		filepath.Join(vaultDir, "secrets.d", "test%secret"))
+	require.NoError(t, err)
+	require.Len(t, versions, 2)
+
+	unlockerID := ""
+
+	if withUnlocker {
+		passphrase := memguard.NewBufferFromBytes([]byte(testPassphrase))
+		defer passphrase.Destroy()
+
+		unlocker, err := vlt.CreatePassphraseUnlocker(passphrase)
+		require.NoError(t, err)
+
+		unlockerID = unlocker.GetID()
+	}
+
+	require.NoError(t, afero.WriteFile(fs, testInput, []byte("input"), 0o600))
+
+	// Newest first
+	return versions[1], unlockerID
+}
+
+// requireWaitsForLock runs a command, given what setupEveryCommand made,
+// while holding the state directory lock. The command must neither finish
+// nor change anything while the lock is held, and must succeed once it is
+// released.
+func requireWaitsForLock(
+	t *testing.T,
+	withUnlocker bool,
+	run func(cli *Instance, olderVersion, unlockerID string) error,
+) {
+	t.Helper()
+
+	fs := afero.NewMemMapFs()
+	olderVersion, unlockerID := setupEveryCommand(t, fs, withUnlocker)
+	before := stateDirModTimes(t, fs)
+
+	release, err := vault.LockStateDir(fs, testStateDir)
+	require.NoError(t, err)
+
+	// Released at most once, and also if the test fails while holding it,
+	// so that later tests can take it
+	release = sync.OnceFunc(release)
+	defer release()
+
+	cli := NewCLIInstanceWithStateDir(fs, testStateDir)
+	cli.cmd = &cobra.Command{}
+	cli.cmd.SetIn(strings.NewReader("value"))
+	cli.cmd.SetOut(io.Discard)
+
+	done := make(chan error, 1)
+
+	go func() { done <- run(cli, olderVersion, unlockerID) }()
+
+	select {
+	case err := <-done:
+		t.Fatalf("finished while the lock was held, with error %v", err)
+	case <-time.After(heldWait):
+	}
+
+	assert.Equal(t, before, stateDirModTimes(t, fs),
+		"changed the state directory while the lock was held")
+
+	release()
+
+	select {
+	case err := <-done:
+		require.NoError(t, err)
+	case <-time.After(lockWait):
+		t.Fatal("did not finish once the lock was released")
+	}
+}
+
+// TestChangingCommandsWaitForLock checks that each command that changes the
+// state directory waits for its lock.
+//
+//nolint:paralleltest // t.Setenv forbids parallel subtests
+func TestChangingCommandsWaitForLock(t *testing.T) {
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+	t.Setenv(secret.EnvUnlockPassphrase, testPassphrase)
+
+	for _, tc := range []struct {
+		name         string
+		withUnlocker bool
+		run          func(cli *Instance, olderVersion, unlockerID string) error
+	}{
+		{"add", false, func(cli *Instance, _, _ string) error {
+			return cli.AddSecret("added", false)
+		}},
+		{"import", false, func(cli *Instance, _, _ string) error {
+			return cli.ImportSecret(cli.cmd, "imported", testInput, false)
+		}},
+		{"generate secret", false, func(cli *Instance, _, _ string) error {
+			return cli.GenerateSecret(cli.cmd, "generated", 16, "base58", false)
+		}},
+		{"encrypt", false, func(cli *Instance, _, _ string) error {
+			return cli.Encrypt("key", testInput, "")
+		}},
+		{"rm", false, func(cli *Instance, _, _ string) error {
+			return cli.RemoveSecret(cli.cmd, "test/secret", false)
+		}},
+		{"move", false, func(cli *Instance, _, _ string) error {
+			return cli.MoveSecret(cli.cmd, "test/secret", "moved", false)
+		}},
+		{"version promote", false, func(cli *Instance, olderVersion, _ string) error {
+			return cli.PromoteVersion(cli.cmd, "test/secret", olderVersion)
+		}},
+		{"version rm", false, func(cli *Instance, olderVersion, _ string) error {
+			return cli.RemoveVersion(cli.cmd, "test/secret", olderVersion)
+		}},
+		{"vault create", false, func(cli *Instance, _, _ string) error {
+			return cli.CreateVault(cli.cmd, "created")
+		}},
+		{"vault select", false, func(cli *Instance, _, _ string) error {
+			return cli.SelectVault(cli.cmd, "other")
+		}},
+		{"vault import", false, func(cli *Instance, _, _ string) error {
+			return cli.VaultImport(cli.cmd, "other")
+		}},
+		{"vault rm", false, func(cli *Instance, _, _ string) error {
+			return cli.RemoveVault(cli.cmd, "other", false)
+		}},
+		{"unlocker add", false, func(cli *Instance, _, _ string) error {
+			return cli.UnlockersAdd("passphrase", cli.cmd)
+		}},
+		{"unlocker rm", true, func(cli *Instance, _, unlockerID string) error {
+			return cli.UnlockersRemove(unlockerID, true, cli.cmd)
+		}},
+		{"unlocker select", true, func(cli *Instance, _, unlockerID string) error {
+			return cli.UnlockerSelect(unlockerID)
+		}},
+		{"init", false, func(cli *Instance, _, _ string) error {
+			return cli.Init(cli.cmd)
+		}},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			requireWaitsForLock(t, tc.withUnlocker, tc.run)
+		})
+	}
+}
+
+// TestEncryptWithExistingKeyTakesNoLock checks that secret encrypt with a
+// key that already exists, which only reads the state directory, finishes
+// while another command holds the state directory lock.
+func TestEncryptWithExistingKeyTakesNoLock(t *testing.T) {
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+
+	fs := afero.NewMemMapFs()
+	_, err := vault.CreateVault(fs, testStateDir, "default")
+	require.NoError(t, err)
+	require.NoError(t, afero.WriteFile(fs, testInput, []byte("input"), 0o600))
+
+	encrypt := NewCLIInstanceWithStateDir(fs, testStateDir)
+	encrypt.cmd = &cobra.Command{}
+	encrypt.cmd.SetOut(io.Discard)
+
+	// Stores the key
+	require.NoError(t, encrypt.Encrypt("key", testInput, ""))
+
+	release, err := vault.LockStateDir(fs, testStateDir)
+	require.NoError(t, err)
+	// Also frees a waiting encrypt if the test fails, so that it releases
+	// the lock the other tests use
+	defer release()
+
+	done := make(chan error, 1)
+
+	go func() { done <- encrypt.Encrypt("key", testInput, "") }()
+
+	select {
+	case err := <-done:
+		require.NoError(t, err)
+	case <-time.After(lockWait):
+		t.Fatal("secret encrypt with an existing key waited for the lock")
+	}
+}
+
+// TestEncryptStreamsUnlocked checks that secret encrypt has released the
+// state directory lock by the time it writes its output. Holding it while
+// streaming would stall every other changing command for as long as the
+// stream lasts, and forever when the other end of the pipe is one of them.
+func TestEncryptStreamsUnlocked(t *testing.T) {
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+
+	fs := afero.NewMemMapFs()
+	_, err := vault.CreateVault(fs, testStateDir, "default")
+	require.NoError(t, err)
+	require.NoError(t, afero.WriteFile(fs, testInput, []byte("streamed"), 0o600))
+
+	outputReader, outputWriter := io.Pipe()
+	done := make(chan error, 1)
+
+	go func() {
+		encrypt := NewCLIInstanceWithStateDir(fs, testStateDir)
+		encrypt.cmd = &cobra.Command{}
+		encrypt.cmd.SetOut(outputWriter)
+
+		err := encrypt.Encrypt("key", testInput, "")
+		_ = outputWriter.CloseWithError(err)
+
+		done <- err
+	}()
+
+	// The first byte of output: encrypt is streaming now, and blocked
+	// writing until it is read
+	_, err = io.ReadFull(outputReader, make([]byte, 1))
+	require.NoError(t, err)
+
+	taken := lockInBackground(t, fs)
+
+	select {
+	case release := <-taken:
+		release()
+	case <-time.After(lockWait):
+		// Let encrypt finish, so that it releases the lock, then free it
+		// again for the tests that follow
+		_, _ = io.Copy(io.Discard, outputReader)
+
+		(<-taken)()
+		t.Fatal("secret encrypt held the lock while streaming")
+	}
+
+	_, err = io.Copy(io.Discard, outputReader)
+	require.NoError(t, err)
+	require.NoError(t, <-done)
+}

internal/cli/move_test.go

@@ -0,0 +1,229 @@
+package cli_test
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"git.eeqj.de/sneak/secret/internal/cli"
+	"git.eeqj.de/sneak/secret/internal/secret"
+	"git.eeqj.de/sneak/secret/internal/vault"
+	"github.com/awnumar/memguard"
+	"github.com/spf13/afero"
+	"github.com/spf13/cobra"
+	"github.com/stretchr/testify/require"
+)
+
+// TestRejectedMoveWithinVaultLeavesStateUnchanged is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/73, where a forced move of a secret
+// onto itself deleted it, also when "work" was spelled two ways, and a failed
+// move within "work" left "work" the current vault. "default" is the current
+// vault in every case, and each case runs on its own copy of the state
+// directory.
+//
+//nolint:paralleltest // newTwoVaultFs uses t.Setenv
+func TestRejectedMoveWithinVaultLeavesStateUnchanged(t *testing.T) {
+	before := snapshotStateDir(t, newTwoVaultFs(t))
+	require.Equal(t, "default", before[testStateDir+"/currentvault"])
+
+	const (
+		ontoItself = "secret 'x' cannot be moved onto itself"
+		workX      = "work:x"
+	)
+
+	tests := []struct {
+		command      string
+		source, dest string
+		force        bool
+		wantErr      string
+	}{
+		{"mv x x", "x", "x", false, ontoItself},
+		{"mv --force x x", "x", "x", true, ontoItself},
+		{"mv --force work:x work:", workX, "work:", true, ontoItself},
+		// An empty destination name defaults to the source name.
+		{`mv --force work:x ""`, workX, "", true, ontoItself},
+		// "work" is a vault name, so the destination is work:x.
+		{"mv --force work:x work", workX, "work", true, ontoItself},
+		{
+			"mv work:nosuch work:y", "work:nosuch", "work:y", false,
+			"secret 'nosuch' not found",
+		},
+		// Only an existing vault is used, so ".." cannot reach the state
+		// directory itself.
+		{
+			"mv --force ..:x ..:y", "..:x", "..:y", true,
+			"vault '..' does not exist",
+		},
+		// Each of these spells "work" a second way. The spelling is not an
+		// existing vault name, so the move is not taken for a move between
+		// two vaults, which would delete the destination, here the source.
+		{
+			"mv --force work:x work/:x", workX, "work/:x", true,
+			"vault 'work/' does not exist",
+		},
+		{
+			"mv --force work/:x work:", "work/:x", "work:", true,
+			"vault 'work/' does not exist",
+		},
+		{
+			"mv --force work:x ./work:x", workX, "./work:x", true,
+			"vault './work' does not exist",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.command, func(t *testing.T) {
+			fs := newFsFromSnapshot(t, before)
+			c := cli.NewCLIInstanceWithStateDir(fs, testStateDir)
+
+			err := c.MoveSecret(&cobra.Command{}, tt.source, tt.dest, tt.force)
+
+			require.Equal(t, before, snapshotStateDir(t, fs))
+			require.EqualError(t, err, tt.wantErr)
+		})
+	}
+}
+
+// TestMoveWithinOtherVaultKeepsCurrentVault checks that `secret mv work:x
+// work:y`, with "default" the current vault, renames "x" to "y" in "work" and
+// leaves "default" the current vault.
+//
+//nolint:paralleltest // newTwoVaultFs uses t.Setenv
+func TestMoveWithinOtherVaultKeepsCurrentVault(t *testing.T) {
+	fs := newTwoVaultFs(t)
+	c := cli.NewCLIInstanceWithStateDir(fs, testStateDir)
+
+	err := c.MoveSecret(&cobra.Command{}, "work:x", "work:y", false)
+	require.NoError(t, err)
+
+	after := snapshotStateDir(t, fs)
+	workSecrets := testStateDir + "/vaults.d/work/secrets.d/"
+
+	require.Equal(t, "default", after[testStateDir+"/currentvault"])
+	require.Contains(t, after, workSecrets+"y/")
+	require.NotContains(t, after, workSecrets+"x/")
+}
+
+// TestMoveOntoSameSecretUnderAnotherNameIsRejected is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/78: on a case-insensitive
+// filesystem "Foo" and "foo" are one secret, and `secret mv --force Foo foo`
+// removed the destination, which was the source. Symbolic links on the real
+// filesystem give one secret two names here: in "default", "y" is a link to
+// the secret "x", and the secrets.d of "other" is a link to that of
+// "default", so other:x is default:x. Each move must be rejected and leave
+// the secret and the links as they were.
+//
+//nolint:paralleltest // t.Setenv
+func TestMoveOntoSameSecretUnderAnotherNameIsRejected(t *testing.T) {
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+
+	const isSame = "is the same secret on this filesystem"
+
+	tests := []struct {
+		command      string
+		source, dest string
+		force        bool
+		wantErr      string
+	}{
+		{
+			"mv --force y x", "y", "x", true,
+			"secret 'y' cannot be moved onto itself: 'x' " + isSame,
+		},
+		{
+			"mv --force x y", "x", "y", true,
+			"secret 'x' cannot be moved onto itself: 'y' " + isSame,
+		},
+		{
+			"mv x y", "x", "y", false,
+			"secret 'x' cannot be moved onto itself: 'y' " + isSame,
+		},
+		{
+			"mv --force default:x other:x", "default:x", "other:x", true,
+			"secret 'default:x' cannot be moved onto itself: 'other:x' " +
+				isSame,
+		},
+		{
+			"mv default:x other", "default:x", "other", false,
+			"secret 'default:x' cannot be moved onto itself: 'other:x' " +
+				isSame,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.command, func(t *testing.T) {
+			fs := afero.NewOsFs()
+			stateDir := t.TempDir()
+			vaultsDir := filepath.Join(stateDir, "vaults.d")
+
+			// "default" is created last, so it is the current vault.
+			_, err := vault.CreateVault(fs, stateDir, "other")
+			require.NoError(t, err)
+
+			vlt, err := vault.CreateVault(fs, stateDir, "default")
+			require.NoError(t, err)
+
+			err = vlt.AddSecret("x", memguard.NewBufferFromBytes([]byte("value")), false)
+			require.NoError(t, err)
+
+			defaultSecrets := filepath.Join(vaultsDir, "default", "secrets.d")
+			otherSecrets := filepath.Join(vaultsDir, "other", "secrets.d")
+			link := filepath.Join(defaultSecrets, "y")
+
+			require.NoError(t, os.Symlink("x", link))
+			require.NoError(t, os.Remove(otherSecrets))
+			require.NoError(t, os.Symlink(defaultSecrets, otherSecrets))
+
+			c := cli.NewCLIInstanceWithStateDir(fs, stateDir)
+			moveErr := c.MoveSecret(&cobra.Command{}, tt.source, tt.dest, tt.force)
+
+			value, err := vlt.GetSecret("x")
+			require.NoError(t, err)
+			require.Equal(t, "value", string(value))
+
+			target, err := os.Readlink(link)
+			require.NoError(t, err)
+			require.Equal(t, "x", target)
+
+			target, err = os.Readlink(otherSecrets)
+			require.NoError(t, err)
+			require.Equal(t, defaultSecrets, target)
+
+			require.EqualError(t, moveErr, tt.wantErr)
+		})
+	}
+}
+
+// TestForcedCaseOnlyMoveOnCaseSensitiveFilesystem checks that where "Foo"
+// and "foo" are two secrets, `secret mv --force Foo foo` still replaces "foo"
+// with "Foo".
+func TestForcedCaseOnlyMoveOnCaseSensitiveFilesystem(t *testing.T) {
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+
+	fs := afero.NewOsFs()
+	stateDir := t.TempDir()
+
+	vlt, err := vault.CreateVault(fs, stateDir, "default")
+	require.NoError(t, err)
+
+	err = vlt.AddSecret("Foo", memguard.NewBufferFromBytes([]byte("upper")), false)
+	require.NoError(t, err)
+
+	_, err = os.Stat(filepath.Join(stateDir, "vaults.d", "default", "secrets.d", "foo"))
+	if err == nil {
+		t.Skip("the temporary directory is on a case-insensitive filesystem")
+	}
+
+	err = vlt.AddSecret("foo", memguard.NewBufferFromBytes([]byte("lower")), false)
+	require.NoError(t, err)
+
+	c := cli.NewCLIInstanceWithStateDir(fs, stateDir)
+	err = c.MoveSecret(&cobra.Command{}, "Foo", "foo", true)
+	require.NoError(t, err)
+
+	value, err := vlt.GetSecret("foo")
+	require.NoError(t, err)
+	require.Equal(t, "upper", string(value))
+
+	_, err = vlt.GetSecret("Foo")
+	require.ErrorIs(t, err, vault.ErrSecretNotFound)
+}

internal/cli/secrets.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"io"
 	"log"
+	"os"
 	"path/filepath"
 	"slices"
 	"strings"
@@ -40,6 +41,7 @@ var (
 	errVaultDoesNotExist           = errors.New("does not exist")
 	errCrossVaultSourceUnqualified = errors.New(
 		"source must specify vault (e.g., vault:secret) for cross-vault move")
+	errMoveOntoItself = errors.New("cannot be moved onto itself")
 )
 
 // bufferInfo tracks a protected buffer and the number of bytes used in it
@@ -383,6 +385,15 @@ func (cli *Instance) AddSecret(secretName string, force bool) error {
 	valueBuffer := combineBuffers(buffers, totalSize)
 	defer valueBuffer.Destroy()
 
+	// Locked only now that stdin has been read: in `secret encrypt key |
+	// secret add name`, holding the lock while reading would leave each
+	// command waiting for the other.
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
 	// Add the secret to the vault
 	secret.Debug("Calling vault.AddSecret", "secret_name", secretName,
 		"value_length", valueBuffer.Size(), "force", force)
@@ -660,6 +671,14 @@ func (cli *Instance) ImportSecret(
 	valueBuffer := combineBuffers(buffers, totalSize)
 	defer valueBuffer.Destroy()
 
+	// Locked only now that the file has been read, as in AddSecret: the
+	// file may be a pipe written by another secret command.
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
 	// Store the secret in the vault
 	err = vlt.AddSecret(secretName, valueBuffer, force)
 	if err != nil {
@@ -679,6 +698,12 @@ func (cli *Instance) RemoveSecret(cmd *cobra.Command, secretName string, _ bool)
 		return err
 	}
 
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
 	// Get current vault
 	currentVlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
@@ -713,7 +738,7 @@ func (cli *Instance) RemoveSecret(cmd *cobra.Command, secretName string, _ bool)
 	}
 
 	// Remove the secret directory
-	err = cli.fs.RemoveAll(secretDir)
+	err = secret.RemoveDirAtomic(cli.fs, secretDir)
 	if err != nil {
 		return fmt.Errorf("failed to remove secret: %w", err)
 	}
@@ -724,9 +749,24 @@ func (cli *Instance) RemoveSecret(cmd *cobra.Command, secretName string, _ bool)
 	return nil
 }
 
-// MoveSecret moves or renames a secret (within or across vaults)
+// MoveSecret moves or renames a secret (within or across vaults), holding
+// the state directory lock.
 func (cli *Instance) MoveSecret(
 	cmd *cobra.Command, source, dest string, force bool,
+) error {
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
+	return cli.moveSecret(cmd, source, dest, force)
+}
+
+// moveSecret does the work of MoveSecret. The caller holds the state
+// directory lock.
+func (cli *Instance) moveSecret(
+	cmd *cobra.Command, source, dest string, force bool,
 ) error {
 	// Parse source and destination
 	srcVaultName, srcSecretName, srcQualified := ParseVaultSecretRef(source)
@@ -743,8 +783,8 @@ func (cli *Instance) MoveSecret(
 	// with a new name
 	if srcQualified && !destQualified {
 		// Check if dest is actually a vault name
-		vaults, err := vault.ListVaults(cli.fs, cli.stateDir)
-		if err == nil && slices.Contains(vaults, dest) {
+		_, err := cli.existingVault(dest)
+		if err == nil {
 			// dest is a vault name, use source secret name
 			destVaultName = dest
 			destSecretName = srcSecretName
@@ -760,8 +800,8 @@ func (cli *Instance) MoveSecret(
 		destSecretName = srcSecretName
 	}
 
-	// Check both names, for every form of the move, before selecting a vault
-	// below, so that a rejected move leaves the current vault as it was.
+	// Check both names, for every form of the move, before building any path
+	// from them.
 	err := vault.ValidateSecretName(srcSecretName)
 	if err != nil {
 		return err
@@ -772,38 +812,66 @@ func (cli *Instance) MoveSecret(
 		return err
 	}
 
-	// If neither is qualified, this is a simple within-vault rename
-	if !srcQualified && !destQualified {
-		return cli.moveSecretWithinVault(cmd, srcSecretName, destSecretName, force)
-	}
-
-	// Same vault? Use simple rename if possible (optimization)
-	if srcVaultName == destVaultName {
-		// Select the vault and do a simple move
-		err = vault.SelectVault(cli.fs, cli.stateDir, srcVaultName)
+	// Neither name is qualified: a rename within the current vault.
+	if !srcQualified {
+		vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 		if err != nil {
-			return fmt.Errorf("failed to select vault '%s': %w", srcVaultName, err)
+			return err
 		}
 
-		return cli.moveSecretWithinVault(cmd, srcSecretName, destSecretName, force)
+		return cli.moveSecretWithinVault(
+			cmd, vlt, srcSecretName, destSecretName, force)
 	}
 
-	// Cross-vault move
-	return cli.moveSecretCrossVault(
-		cmd, srcVaultName, srcSecretName, destVaultName, destSecretName, force)
-}
-
-// moveSecretWithinVault handles rename within the current vault. Its caller,
-// MoveSecret, has already checked both secret names.
-func (cli *Instance) moveSecretWithinVault(
-	cmd *cobra.Command, source, dest string, force bool,
-) error {
-	currentVlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
+	// Both vaults must be existing vaults by exact name, so that two
+	// spellings of one vault, such as "work" and "work/", are never taken for
+	// two vaults. A named vault does not become the current vault.
+	srcVault, err := cli.existingVault(srcVaultName)
 	if err != nil {
 		return err
 	}
 
-	vaultDir, err := currentVlt.GetDirectory()
+	destVault, err := cli.existingVault(destVaultName)
+	if err != nil {
+		return err
+	}
+
+	if srcVaultName == destVaultName {
+		return cli.moveSecretWithinVault(
+			cmd, srcVault, srcSecretName, destSecretName, force)
+	}
+
+	return cli.moveSecretCrossVault(
+		cmd, srcVault, srcSecretName, destVault, destSecretName, force)
+}
+
+// existingVault returns the vault with the given name, or an error if there
+// is none. Unlike vault.SelectVault, it leaves the current vault as it is.
+func (cli *Instance) existingVault(name string) (*vault.Vault, error) {
+	vaults, err := vault.ListVaults(cli.fs, cli.stateDir)
+	if err != nil {
+		return nil, fmt.Errorf("failed to list vaults: %w", err)
+	}
+
+	if !slices.Contains(vaults, name) {
+		return nil, fmt.Errorf("vault '%s' %w", name, errVaultDoesNotExist)
+	}
+
+	return vault.NewVault(cli.fs, cli.stateDir, name), nil
+}
+
+// moveSecretWithinVault renames a secret within the vault vlt. Its caller,
+// MoveSecret, has already checked both secret names.
+func (cli *Instance) moveSecretWithinVault(
+	cmd *cobra.Command, vlt *vault.Vault, source, dest string, force bool,
+) error {
+	// With --force the destination is removed before the source is renamed
+	// onto it, which would delete the secret.
+	if source == dest {
+		return fmt.Errorf("secret '%s' %w", source, errMoveOntoItself)
+	}
+
+	vaultDir, err := vlt.GetDirectory()
 	if err != nil {
 		return err
 	}
@@ -823,6 +891,18 @@ func (cli *Instance) moveSecretWithinVault(
 	destEncoded := strings.ReplaceAll(dest, "/", "%")
 	destDir := filepath.Join(vaultDir, "secrets.d", destEncoded)
 
+	// Removing a destination that is the source under another name, such as
+	// "foo" for "Foo" on a case-insensitive filesystem, would delete it too.
+	same, err := cli.sameDirectory(sourceDir, destDir)
+	if err != nil {
+		return err
+	}
+
+	if same {
+		return fmt.Errorf("secret '%s' %w: '%s' is the same secret on "+
+			"this filesystem", source, errMoveOntoItself, dest)
+	}
+
 	exists, err = afero.DirExists(cli.fs, destDir)
 	if err != nil {
 		return fmt.Errorf("failed to check if destination secret exists: %w", err)
@@ -833,7 +913,7 @@ func (cli *Instance) moveSecretWithinVault(
 			return fmt.Errorf("secret '%s' %w", dest, errSecretExistsNoForce)
 		}
 
-		err = cli.fs.RemoveAll(destDir)
+		err = secret.RemoveDirAtomic(cli.fs, destDir)
 		if err != nil {
 			return fmt.Errorf("failed to remove existing destination: %w", err)
 		}
@@ -849,57 +929,80 @@ func (cli *Instance) moveSecretWithinVault(
 	return nil
 }
 
-// moveSecretCrossVault handles moving between different vaults. Its caller,
-// MoveSecret, has already checked both secret names.
+// sameDirectory reports whether the existing directory dir and the path
+// other are one directory under two names, as secrets.d/Foo and
+// secrets.d/foo are on a case-insensitive filesystem, or a directory and a
+// symbolic link to it. Removing other to make room for dir would then delete
+// dir. It is false if other does not exist, and always false on the
+// in-memory filesystem, which has no such aliasing and whose files
+// os.SameFile does not compare.
+func (cli *Instance) sameDirectory(dir, other string) (bool, error) {
+	dirInfo, err := cli.fs.Stat(dir)
+	if err != nil {
+		return false, fmt.Errorf("failed to check %s: %w", dir, err)
+	}
+
+	otherInfo, err := cli.fs.Stat(other)
+	if errors.Is(err, os.ErrNotExist) {
+		return false, nil
+	}
+
+	if err != nil {
+		return false, fmt.Errorf("failed to check %s: %w", other, err)
+	}
+
+	return os.SameFile(dirInfo, otherInfo), nil
+}
+
+// moveSecretCrossVault handles moving between two different vaults. Its
+// caller, MoveSecret, has already checked both secret names and that both
+// vaults exist.
 func (cli *Instance) moveSecretCrossVault(
 	cmd *cobra.Command,
-	srcVaultName, srcSecretName,
-	destVaultName, destSecretName string,
+	srcVault *vault.Vault, srcSecretName string,
+	destVault *vault.Vault, destSecretName string,
 	force bool,
 ) error {
-	// Get source vault
-	srcVault := vault.NewVault(cli.fs, cli.stateDir, srcVaultName)
-
 	srcVaultDir, err := srcVault.GetDirectory()
 	if err != nil {
 		return fmt.Errorf("failed to get source vault directory: %w", err)
 	}
 
-	// Verify source vault exists
-	exists, err := afero.DirExists(cli.fs, srcVaultDir)
-	if err != nil || !exists {
-		return fmt.Errorf("source vault '%s' %w", srcVaultName, errVaultDoesNotExist)
-	}
-
 	// Verify source secret exists
 	srcStorageName := strings.ReplaceAll(srcSecretName, "/", "%")
 	srcSecretDir := filepath.Join(srcVaultDir, "secrets.d", srcStorageName)
 
-	exists, err = afero.DirExists(cli.fs, srcSecretDir)
+	exists, err := afero.DirExists(cli.fs, srcSecretDir)
 	if err != nil || !exists {
 		return fmt.Errorf("secret '%s' %w in vault '%s'",
-			srcSecretName, errSecretNotFound, srcVaultName)
+			srcSecretName, errSecretNotFound, srcVault.Name)
 	}
 
-	// Get destination vault
-	destVault := vault.NewVault(cli.fs, cli.stateDir, destVaultName)
-
+	// The source is removed after the copy, so a destination that is the
+	// source under another name would be lost with it.
 	destVaultDir, err := destVault.GetDirectory()
 	if err != nil {
 		return fmt.Errorf("failed to get destination vault directory: %w", err)
 	}
 
-	// Verify destination vault exists
-	exists, err = afero.DirExists(cli.fs, destVaultDir)
-	if err != nil || !exists {
-		return fmt.Errorf("destination vault '%s' %w",
-			destVaultName, errVaultDoesNotExist)
+	destStorageName := strings.ReplaceAll(destSecretName, "/", "%")
+	destSecretDir := filepath.Join(destVaultDir, "secrets.d", destStorageName)
+
+	same, err := cli.sameDirectory(srcSecretDir, destSecretDir)
+	if err != nil {
+		return err
+	}
+
+	if same {
+		return fmt.Errorf("secret '%s:%s' %w: '%s:%s' is the same secret on "+
+			"this filesystem", srcVault.Name, srcSecretName, errMoveOntoItself,
+			destVault.Name, destSecretName)
 	}
 
 	// Unlock destination vault (will fail if neither mnemonic nor unlocker available)
 	_, err = destVault.GetOrDeriveLongTermKey()
 	if err != nil {
-		return fmt.Errorf("failed to unlock destination vault '%s': %w", destVaultName, err)
+		return fmt.Errorf("failed to unlock destination vault '%s': %w", destVault.Name, err)
 	}
 
 	// Count versions for user feedback
@@ -914,18 +1017,18 @@ func (cli *Instance) moveSecretCrossVault(
 	}
 
 	// Delete source secret
-	err = cli.fs.RemoveAll(srcSecretDir)
+	err = secret.RemoveDirAtomic(cli.fs, srcSecretDir)
 	if err != nil {
 		// Copy succeeded but delete failed - warn but don't fail
 		cmd.Printf("Warning: copied secret but failed to remove source: %v\n", err)
 		cmd.Printf("Moved secret '%s:%s' to '%s:%s' (%d version(s))\n",
-			srcVaultName, srcSecretName, destVaultName, destSecretName, versionCount)
+			srcVault.Name, srcSecretName, destVault.Name, destSecretName, versionCount)
 
 		return nil
 	}
 
 	cmd.Printf("Moved secret '%s:%s' to '%s:%s' (%d version(s))\n",
-		srcVaultName, srcSecretName, destVaultName, destSecretName, versionCount)
+		srcVault.Name, srcSecretName, destVault.Name, destSecretName, versionCount)
 
 	return nil
 }

internal/cli/unlockers.go

@@ -349,6 +349,10 @@ func unlockerIDFromDir(
 // itself cannot be read. Callers must distinguish the two: an unreadable
 // directory means the unlocker's real ID is unknowable, so the entry has
 // to be skipped rather than reported under a synthesized ID.
+//
+// A metadata file that cannot be read or parsed is skipped without a
+// warning: every caller gets metadata from vault.ListUnlockers first,
+// which has already warned about that directory.
 func findUnlockerIDByMetadata(
 	fs afero.Fs, unlockersDir string, metadata secret.UnlockerMetadata,
 	includeSecureEnclave bool,
@@ -371,9 +375,6 @@ func findUnlockerIDByMetadata(
 		// Check if this is the right unlocker by comparing metadata
 		metadataBytes, err := afero.ReadFile(fs, metadataPath)
 		if err != nil {
-			secret.Warn("Could not read unlocker metadata file",
-				"path", metadataPath, "error", err)
-
 			continue
 		}
 
@@ -381,9 +382,6 @@ func findUnlockerIDByMetadata(
 
 		err = json.Unmarshal(metadataBytes, &diskMetadata)
 		if err != nil {
-			secret.Warn("Could not parse unlocker metadata file",
-				"path", metadataPath, "error", err)
-
 			continue
 		}
 
@@ -534,6 +532,12 @@ func (cli *Instance) printUnlockersTable(unlockers []UnlockerInfo) error {
 
 // UnlockersAdd adds a new unlocker
 func (cli *Instance) UnlockersAdd(unlockerType string, cmd *cobra.Command) error {
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
 	switch unlockerType {
 	case unlockerTypePassphrase:
 		return cli.addPassphraseUnlocker(cmd)
@@ -714,6 +718,12 @@ func (cli *Instance) addPGPUnlocker(cmd *cobra.Command) error {
 func (cli *Instance) UnlockersRemove(
 	unlockerID string, force bool, cmd *cobra.Command,
 ) error {
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
@@ -763,6 +773,12 @@ func (cli *Instance) UnlockersRemove(
 
 // UnlockerSelect selects an unlocker as current
 func (cli *Instance) UnlockerSelect(unlockerID string) error {
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {

internal/cli/unlockers_list_test.go

@@ -1,13 +1,19 @@
 // Unlocker List Tests
 //
-// Tests for `secret unlocker list` behavior when the unlockers.d directory
-// cannot be read while the listing is being rendered:
+// Tests for `secret unlocker list` behavior when the unlockers.d directory,
+// or an unlocker's metadata in it, cannot be read while the listing is
+// being rendered:
 //
 //   - TestUnlockersListSkipsUnreadableUnlockersDir: an unreadable
 //     unlockers.d yields no rows rather than rows bearing synthesized IDs.
 //   - TestUnlockersListSkipsOnlyUnreadableEntries: a readable entry is
 //     still listed, with its real ID and its current-unlocker marker,
 //     when a later entry's scan fails.
+//   - TestUnlockersListToleratesCorruptMetadata: one unlocker's corrupt
+//     metadata does not stop the others from being listed.
+//   - TestUnlockersListSkipsUnreadableMetadata: an unlocker whose metadata
+//     file cannot be checked for or read is left out, and the other is
+//     still listed.
 //
 // The listing resolves each unlocker's real ID by rescanning unlockers.d
 // after the vault has already enumerated it. If that rescan fails the ID
@@ -22,6 +28,7 @@ import (
 	"bytes"
 	"encoding/json"
 	"errors"
+	"os"
 	"path/filepath"
 	"testing"
 	"time"
@@ -92,6 +99,49 @@ func (f *unlockersDirFailFs) Open(name string) (afero.File, error) {
 	return f.Fs.Open(name)
 }
 
+// errMetadataUnreadable is returned by the test filesystem in place of a
+// successful open of one unlocker's metadata file.
+var errMetadataUnreadable = errors.New("input/output error")
+
+// metadataReadFailFs fails every open of the file at unreadablePath. The
+// file still exists, so checking for it succeeds and only reading it fails.
+type metadataReadFailFs struct {
+	afero.Fs
+
+	unreadablePath string
+}
+
+//nolint:ireturn // afero.File is the interface required by afero.Fs
+func (f *metadataReadFailFs) Open(name string) (afero.File, error) {
+	if name == f.unreadablePath {
+		return nil, errMetadataUnreadable
+	}
+
+	//nolint:wrapcheck // test double must return the wrapped Fs error as-is
+	return f.Fs.Open(name)
+}
+
+// errMetadataUncheckable is returned by the test filesystem in place of a
+// successful check for one unlocker's metadata file.
+var errMetadataUncheckable = errors.New("permission denied")
+
+// metadataStatFailFs fails every check for whether the file at
+// uncheckablePath exists, as when its unlocker directory cannot be entered.
+type metadataStatFailFs struct {
+	afero.Fs
+
+	uncheckablePath string
+}
+
+func (f *metadataStatFailFs) Stat(name string) (os.FileInfo, error) {
+	if name == f.uncheckablePath {
+		return nil, errMetadataUncheckable
+	}
+
+	//nolint:wrapcheck // test double must return the wrapped Fs error as-is
+	return f.Fs.Stat(name)
+}
+
 // writePGPUnlocker writes a PGP unlocker directory with metadata that
 // yields the real ID "pgp-<keyID>".
 func writePGPUnlocker(
@@ -227,3 +277,102 @@ func TestUnlockersListReadableEntriesAreListed(t *testing.T) {
 	assert.True(t, unlockers[0].IsCurrent)
 	assert.False(t, unlockers[1].IsCurrent)
 }
+
+// TestUnlockersListToleratesCorruptMetadata asserts that one unlocker with
+// corrupt metadata does not stop the listing. Metadata that is not JSON
+// leaves that unlocker out; PGP metadata without a usable GPG key ID lists
+// it as "pgp-unknown". The healthy unlocker is listed with its real ID.
+func TestUnlockersListToleratesCorruptMetadata(t *testing.T) {
+	t.Parallel()
+
+	healthyID := "pgp-" + listTestGPGKeyID + "A"
+
+	tests := []struct {
+		name     string
+		metadata string
+		wantIDs  []string
+	}{
+		{
+			name:     "not JSON",
+			metadata: "not json",
+			wantIDs:  []string{healthyID},
+		},
+		{
+			name:     "GPG key ID of the wrong type",
+			metadata: `{"type": "pgp", "gpgKeyId": 42}`,
+			wantIDs:  []string{healthyID, "pgp-unknown"},
+		},
+		{
+			name:     "GPG key ID missing",
+			metadata: `{"type": "pgp"}`,
+			wantIDs:  []string{healthyID, "pgp-unknown"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			fs := newListTestVault(t, 2)
+			metadataPath := filepath.Join(listTestStateDir, "vaults.d",
+				listTestVaultName, listTestUnlockersDirName,
+				listTestUnlockerDirTwo, listTestMetadataFileName)
+			require.NoError(t, afero.WriteFile(
+				fs, metadataPath, []byte(tt.metadata), listTestFilePerm,
+			))
+
+			unlockers := listUnlockersJSON(t, fs)
+			require.Len(t, unlockers, len(tt.wantIDs))
+
+			for i, wantID := range tt.wantIDs {
+				assert.Equal(t, wantID, unlockers[i].ID)
+			}
+		})
+	}
+}
+
+// TestUnlockersListSkipsUnreadableMetadata asserts that an unlocker whose
+// metadata file cannot be checked for or cannot be read is left out of the
+// listing, and the other unlocker is still listed with its real ID. The
+// failing one sorts first, so finding the other's ID has to step past it
+// as well.
+func TestUnlockersListSkipsUnreadableMetadata(t *testing.T) {
+	t.Parallel()
+
+	failingPath := filepath.Join(listTestStateDir, "vaults.d",
+		listTestVaultName, listTestUnlockersDirName,
+		listTestUnlockerDirOne, listTestMetadataFileName)
+
+	tests := []struct {
+		name string
+		wrap func(base afero.Fs) afero.Fs
+	}{
+		{
+			name: "checking for the file fails",
+			wrap: func(base afero.Fs) afero.Fs {
+				return &metadataStatFailFs{Fs: base, uncheckablePath: failingPath}
+			},
+		},
+		{
+			name: "reading the file fails",
+			wrap: func(base afero.Fs) afero.Fs {
+				return &metadataReadFailFs{Fs: base, unreadablePath: failingPath}
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			fs := tt.wrap(newListTestVault(t, 2))
+
+			unlockers := listUnlockersJSON(t, fs)
+
+			require.Len(t, unlockers, 1,
+				"only the unlocker with usable metadata may be listed")
+			assert.Equal(t, "pgp-"+listTestGPGKeyID+"B", unlockers[0].ID,
+				"the listed row must carry the real unlocker ID")
+		})
+	}
+}

internal/cli/vault.go

@@ -267,6 +267,12 @@ func resolvePassphrase() (*memguard.LockedBuffer, error) {
 func (cli *Instance) CreateVault(cmd *cobra.Command, name string) error {
 	secret.Debug("Creating new vault", "name", name, "state_dir", cli.stateDir)
 
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
 	// Get or prompt for mnemonic
 	var mnemonicStr string
 
@@ -354,7 +360,13 @@ func (cli *Instance) CreateVault(cmd *cobra.Command, name string) error {
 
 // SelectVault selects a vault as the current one
 func (cli *Instance) SelectVault(cmd *cobra.Command, name string) error {
-	err := vault.SelectVault(cli.fs, cli.stateDir, name)
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
+	err = vault.SelectVault(cli.fs, cli.stateDir, name)
 	if err != nil {
 		return err
 	}
@@ -442,8 +454,21 @@ func updateVaultImportMetadata(
 	return nil
 }
 
-// VaultImport imports a mnemonic into a specific vault
+// VaultImport imports a mnemonic into a specific vault, holding the state
+// directory lock while importMnemonic runs
 func (cli *Instance) VaultImport(cmd *cobra.Command, vaultName string) error {
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
+	return cli.importMnemonic(cmd, vaultName)
+}
+
+// importMnemonic gives the vault a long-term key derived from the mnemonic
+// and a passphrase unlocker
+func (cli *Instance) importMnemonic(cmd *cobra.Command, vaultName string) error {
 	secret.Debug("Importing mnemonic into vault",
 		"vault_name", vaultName, "state_dir", cli.stateDir)
 
@@ -478,7 +503,7 @@ func (cli *Instance) VaultImport(cmd *cobra.Command, vaultName string) error {
 	secret.Debug("Storing long-term public key",
 		"pubkey", ltPublicKey, "vault_dir", vaultDir)
 
-	err = afero.WriteFile(cli.fs, pubKeyPath, []byte(ltPublicKey), secret.FilePerms)
+	err = secret.WriteFileAtomic(cli.fs, pubKeyPath, []byte(ltPublicKey))
 	if err != nil {
 		return fmt.Errorf("failed to store long-term public key: %w", err)
 	}
@@ -577,6 +602,12 @@ func (cli *Instance) switchAwayFromVault(
 
 // RemoveVault removes a vault with safety checks
 func (cli *Instance) RemoveVault(cmd *cobra.Command, name string, force bool) error {
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
 	// Get list of all vaults
 	vaults, err := vault.ListVaults(cli.fs, cli.stateDir)
 	if err != nil {
@@ -626,7 +657,7 @@ func (cli *Instance) RemoveVault(cmd *cobra.Command, name string, force bool) er
 	}
 
 	// Remove the vault directory
-	err = cli.fs.RemoveAll(vaultDir)
+	err = secret.RemoveDirAtomic(cli.fs, vaultDir)
 	if err != nil {
 		return fmt.Errorf("failed to remove vault directory: %w", err)
 	}

internal/cli/version.go

@@ -249,6 +249,12 @@ func (cli *Instance) PromoteVersion(
 		return err
 	}
 
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
@@ -295,6 +301,12 @@ func (cli *Instance) RemoveVersion(
 		return err
 	}
 
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
@@ -346,7 +358,7 @@ func (cli *Instance) RemoveVersion(
 	// Remove the version directory
 	versionDir := filepath.Join(secretDir, "versions", version)
 
-	err = cli.fs.RemoveAll(versionDir)
+	err = secret.RemoveDirAtomic(cli.fs, versionDir)
 	if err != nil {
 		return fmt.Errorf("failed to remove version: %w", err)
 	}

internal/secret/atomic.go

@@ -0,0 +1,86 @@
+package secret
+
+import (
+	"fmt"
+	"path/filepath"
+
+	"github.com/spf13/afero"
+)
+
+// WriteFileAtomic replaces the file at path with data so that a reader, or
+// a crash at any moment, finds either the old content or the new, never a
+// partial file. The data goes into a temporary file that afero.TempFile
+// creates with mode 0600 in the same directory (a rename is only atomic
+// within one filesystem), is synced to disk, and is renamed over path. The
+// temporary file is removed if any step fails.
+func WriteFileAtomic(fs afero.Fs, path string, data []byte) error {
+	tmp, err := afero.TempFile(fs, filepath.Dir(path),
+		"."+filepath.Base(path)+".tmp-*")
+	if err != nil {
+		return fmt.Errorf("failed to create temporary file for %s: %w", path, err)
+	}
+
+	_, err = tmp.Write(data)
+	if err == nil {
+		err = tmp.Sync()
+	}
+
+	closeErr := tmp.Close()
+	if err == nil {
+		err = closeErr
+	}
+
+	if err == nil {
+		err = fs.Rename(tmp.Name(), path)
+	}
+
+	if err != nil {
+		_ = fs.Remove(tmp.Name())
+
+		return fmt.Errorf("failed to write %s: %w", path, err)
+	}
+
+	return nil
+}
+
+// TempDirFor creates an empty temporary directory in which to build the
+// directory target before renaming it into place, or into which to move
+// target before deleting it. It is made in target's grandparent: on the
+// same filesystem, so the rename is atomic, and outside target's parent,
+// the directory that is listed to find vaults, secrets, versions and
+// unlockers, so one left behind by a crash is never taken for one of them.
+// Its name leaves out target's, which may already be as long as a file name
+// can be.
+func TempDirFor(fs afero.Fs, target string) (string, error) {
+	dir, err := afero.TempDir(fs, filepath.Dir(filepath.Dir(target)), ".tmp-")
+	if err != nil {
+		return "", fmt.Errorf(
+			"failed to create temporary directory for %s: %w", target, err)
+	}
+
+	return dir, nil
+}
+
+// RemoveDirAtomic deletes the directory dir so that it disappears in one
+// rename: dir is moved into a new directory from TempDirFor, which is then
+// deleted. A crash part-way leaves only that temporary directory behind.
+func RemoveDirAtomic(fs afero.Fs, dir string) error {
+	tmp, err := TempDirFor(fs, dir)
+	if err != nil {
+		return err
+	}
+
+	err = fs.Rename(dir, filepath.Join(tmp, filepath.Base(dir)))
+	if err != nil {
+		_ = fs.Remove(tmp)
+
+		return fmt.Errorf("failed to remove %s: %w", dir, err)
+	}
+
+	err = fs.RemoveAll(tmp)
+	if err != nil {
+		return fmt.Errorf("failed to remove %s: %w", dir, err)
+	}
+
+	return nil
+}

internal/secret/atomic_test.go

@@ -0,0 +1,672 @@
+package secret_test
+
+import (
+	"errors"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"filippo.io/age"
+	"git.eeqj.de/sneak/secret/internal/secret"
+	"git.eeqj.de/sneak/secret/internal/vault"
+	"github.com/awnumar/memguard"
+	"github.com/spf13/afero"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+var errInjected = errors.New("injected failure")
+
+// The kinds of change hookFs passes to before.
+const (
+	opCreate = "create"
+	opOpen   = "open"
+	opSync   = "sync"
+	opMkdir  = "mkdir"
+	opRemove = "remove"
+	opRename = "rename"
+)
+
+// currentFile is the file in a secret's directory that names its current
+// version.
+const currentFile = "current"
+
+// unlockerMetadataFile is the file a new unlocker writes last.
+const unlockerMetadataFile = "unlocker-metadata.json"
+
+// unlockerPassphrase protects the passphrase unlockers the tests create.
+//
+//nolint:gosec // G101: test data, not a real credential
+const unlockerPassphrase = "unlocker passphrase"
+
+// hookFs passes every call through to Fs, but first calls before for each
+// call that changes the filesystem, and for each Sync of a file opened
+// through it, with the path it changes (the new path, for Rename). A test
+// uses before to inspect the tree at every point where a crash could stop
+// the code under test, or returns an error from it to make that call fail.
+// If opened is set, OpenFile also tells it the mode it opens each file with.
+type hookFs struct {
+	afero.Fs
+
+	before func(op, path string) error
+	opened func(path string, perm os.FileMode)
+}
+
+// hookFile is a file opened through hookFs.
+type hookFile struct {
+	afero.File
+
+	before func(op, path string) error
+}
+
+func (f hookFile) Sync() error {
+	err := f.before(opSync, f.Name())
+	if err != nil {
+		return err
+	}
+
+	return f.File.Sync()
+}
+
+//nolint:ireturn // implements afero.Fs
+func (h hookFs) Create(name string) (afero.File, error) {
+	err := h.before(opCreate, name)
+	if err != nil {
+		return nil, err
+	}
+
+	file, err := h.Fs.Create(name)
+	if err != nil {
+		return nil, err
+	}
+
+	return hookFile{File: file, before: h.before}, nil
+}
+
+//nolint:ireturn // implements afero.Fs
+func (h hookFs) OpenFile(
+	name string, flag int, perm os.FileMode,
+) (afero.File, error) {
+	err := h.before(opOpen, name)
+	if err != nil {
+		return nil, err
+	}
+
+	if h.opened != nil {
+		h.opened(name, perm)
+	}
+
+	file, err := h.Fs.OpenFile(name, flag, perm)
+	if err != nil {
+		return nil, err
+	}
+
+	return hookFile{File: file, before: h.before}, nil
+}
+
+func (h hookFs) Mkdir(name string, perm os.FileMode) error {
+	err := h.before(opMkdir, name)
+	if err != nil {
+		return err
+	}
+
+	return h.Fs.Mkdir(name, perm)
+}
+
+func (h hookFs) MkdirAll(path string, perm os.FileMode) error {
+	err := h.before(opMkdir, path)
+	if err != nil {
+		return err
+	}
+
+	return h.Fs.MkdirAll(path, perm)
+}
+
+func (h hookFs) Remove(name string) error {
+	err := h.before(opRemove, name)
+	if err != nil {
+		return err
+	}
+
+	return h.Fs.Remove(name)
+}
+
+func (h hookFs) RemoveAll(path string) error {
+	err := h.before(opRemove, path)
+	if err != nil {
+		return err
+	}
+
+	return h.Fs.RemoveAll(path)
+}
+
+func (h hookFs) Rename(oldname, newname string) error {
+	err := h.before(opRename, newname)
+	if err != nil {
+		return err
+	}
+
+	return h.Fs.Rename(oldname, newname)
+}
+
+// testFilesystem is a filesystem to run a test on, with a directory in it
+// to work in.
+type testFilesystem struct {
+	name string
+	open func(t *testing.T) (afero.Fs, string)
+}
+
+// testFilesystems are the in-memory filesystem that most tests use and the
+// real one: every rename-based guarantee is checked on both.
+//
+//nolint:gochecknoglobals // read-only table shared by the tests below
+var testFilesystems = []testFilesystem{
+	{"memory", func(*testing.T) (afero.Fs, string) {
+		return afero.NewMemMapFs(), "/test"
+	}},
+	{"real", func(t *testing.T) (afero.Fs, string) {
+		t.Helper()
+
+		return afero.NewOsFs(), t.TempDir()
+	}},
+}
+
+// dirNames lists the names in dir.
+func dirNames(t *testing.T, fs afero.Fs, dir string) []string {
+	t.Helper()
+
+	entries, err := afero.ReadDir(fs, dir)
+	require.NoError(t, err)
+
+	names := make([]string, 0, len(entries))
+	for _, entry := range entries {
+		names = append(names, entry.Name())
+	}
+
+	return names
+}
+
+// writeLongTermKey gives the test vault under stateDir a new long-term key
+// and returns it.
+func writeLongTermKey(
+	t *testing.T, fs afero.Fs, stateDir string,
+) *age.X25519Identity {
+	t.Helper()
+
+	vault := &MockVersionVault{Name: testVaultName, fs: fs, stateDir: stateDir}
+
+	vaultDir, err := vault.GetDirectory()
+	require.NoError(t, err)
+	require.NoError(t, fs.MkdirAll(vaultDir, 0o700))
+
+	ltIdentity, err := age.GenerateX25519Identity()
+	require.NoError(t, err)
+	require.NoError(t, afero.WriteFile(fs, filepath.Join(vaultDir, "pub.age"),
+		[]byte(ltIdentity.Recipient().String()), 0o600))
+
+	return ltIdentity
+}
+
+// newVaultWithSecret creates the vault name under stateDir from the test
+// mnemonic, with a secret "shared" in it that holds value.
+func newVaultWithSecret(
+	t *testing.T, fs afero.Fs, stateDir, name, value string,
+) *vault.Vault {
+	t.Helper()
+
+	vlt, err := vault.CreateVault(fs, stateDir, name)
+	require.NoError(t, err)
+
+	buffer := memguard.NewBufferFromBytes([]byte(value))
+	defer buffer.Destroy()
+
+	require.NoError(t, vlt.AddSecret("shared", buffer, false))
+
+	return vlt
+}
+
+func TestWriteFileAtomicReplacesFile(t *testing.T) {
+	t.Parallel()
+
+	for _, tfs := range testFilesystems {
+		t.Run(tfs.name, func(t *testing.T) {
+			t.Parallel()
+
+			fs, dir := tfs.open(t)
+			path := filepath.Join(dir, currentFile)
+
+			require.NoError(t, secret.WriteFileAtomic(fs, path, []byte("old")))
+			require.NoError(t, secret.WriteFileAtomic(fs, path, []byte("new")))
+
+			data, err := afero.ReadFile(fs, path)
+			require.NoError(t, err)
+			assert.Equal(t, "new", string(data))
+
+			info, err := fs.Stat(path)
+			require.NoError(t, err)
+			assert.Equal(t, secret.FilePerms, info.Mode().Perm())
+
+			// No temporary file is left next to it
+			assert.Equal(t, []string{currentFile}, dirNames(t, fs, dir))
+		})
+	}
+}
+
+func TestWriteFileAtomicFailureKeepsOldFile(t *testing.T) {
+	t.Parallel()
+
+	for _, tfs := range testFilesystems {
+		t.Run(tfs.name, func(t *testing.T) {
+			t.Parallel()
+
+			base, dir := tfs.open(t)
+			path := filepath.Join(dir, currentFile)
+			require.NoError(t, secret.WriteFileAtomic(base, path, []byte("old")))
+
+			fs := hookFs{Fs: base, before: func(op, _ string) error {
+				if op == opRename {
+					return errInjected
+				}
+
+				return nil
+			}}
+
+			err := secret.WriteFileAtomic(fs, path, []byte("new"))
+			require.ErrorIs(t, err, errInjected)
+
+			data, err := afero.ReadFile(base, path)
+			require.NoError(t, err)
+			assert.Equal(t, "old", string(data))
+
+			// The temporary file is removed again
+			assert.Equal(t, []string{currentFile}, dirNames(t, base, dir))
+		})
+	}
+}
+
+// TestRemoveDirAtomic checks that RemoveDirAtomic deletes nothing where the
+// directory stands, which a crash could stop half-way, and that it leaves
+// nothing behind.
+func TestRemoveDirAtomic(t *testing.T) {
+	t.Parallel()
+
+	for _, tfs := range testFilesystems {
+		t.Run(tfs.name, func(t *testing.T) {
+			t.Parallel()
+
+			base, dir := tfs.open(t)
+			listed := filepath.Join(dir, "secrets.d")
+			target := filepath.Join(listed, "doomed")
+
+			require.NoError(t, base.MkdirAll(filepath.Join(target, "versions"), 0o700))
+			require.NoError(t, secret.WriteFileAtomic(base,
+				filepath.Join(target, currentFile), []byte("20231216.001")))
+
+			fs := hookFs{Fs: base, before: func(op, path string) error {
+				if op == opRemove && strings.HasPrefix(path, target) {
+					t.Errorf("deleted %s where it stands", path)
+				}
+
+				return nil
+			}}
+
+			require.NoError(t, secret.RemoveDirAtomic(fs, target))
+
+			// Gone, and no temporary directory is left in the directory
+			// that is listed or in the one above it
+			assert.Empty(t, dirNames(t, base, listed))
+			assert.Equal(t, []string{"secrets.d"}, dirNames(t, base, dir))
+		})
+	}
+}
+
+// TestLongestNames adds a secret to a vault and removes the vault, both
+// named with 255 bytes, the most a file name may have, on the real
+// filesystem: the temporary directories they use must fit that limit too.
+func TestLongestNames(t *testing.T) {
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+
+	const longestName = 255
+
+	fs := afero.NewOsFs()
+	name := strings.Repeat("a", longestName)
+
+	vlt, err := vault.CreateVault(fs, t.TempDir(), name)
+	require.NoError(t, err)
+
+	value := memguard.NewBufferFromBytes([]byte("long"))
+	defer value.Destroy()
+
+	require.NoError(t, vlt.AddSecret(name, value, false))
+
+	got, err := vlt.GetSecret(name)
+	require.NoError(t, err)
+	assert.Equal(t, "long", string(got))
+
+	vaultDir, err := vlt.GetDirectory()
+	require.NoError(t, err)
+	require.NoError(t, secret.RemoveDirAtomic(fs, vaultDir))
+	assert.NoDirExists(t, vaultDir)
+}
+
+// TestForcedCopyKeepsDestinationUntilReplaced copies a secret over one in
+// another vault, as a forced move between vaults does, and makes the last
+// step that completes the copy fail. The secret it was to replace must
+// still be there unchanged: it may go only once its replacement is whole.
+//
+//nolint:paralleltest // t.Setenv forbids t.Parallel
+func TestForcedCopyKeepsDestinationUntilReplaced(t *testing.T) {
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+
+	for _, tfs := range testFilesystems {
+		t.Run(tfs.name, func(t *testing.T) {
+			base, stateDir := tfs.open(t)
+			src := newVaultWithSecret(t, base, stateDir, "source", "new")
+			dest := newVaultWithSecret(t, base, stateDir, "dest", "old")
+
+			// The copy is complete once its current file is written
+			fs := hookFs{Fs: base, before: func(op, path string) error {
+				if op == opRename && filepath.Base(path) == currentFile {
+					return errInjected
+				}
+
+				return nil
+			}}
+
+			err := vault.NewVault(fs, stateDir, "dest").
+				CopySecretAllVersions(src, "shared", "shared", true)
+			require.ErrorIs(t, err, errInjected)
+
+			value, err := dest.GetSecret("shared")
+			require.NoError(t, err)
+			assert.Equal(t, "old", string(value))
+		})
+	}
+}
+
+// TestTempDirsStayOutOfListings adds a version, adds a secret, copies a
+// secret over another and removes one, and checks that none of them makes a
+// directory directly in secrets.d or in a versions directory. Those are
+// listed to find secrets and versions, so a temporary directory made there
+// would be listed while half-built, and one left by a crash would stay.
+//
+//nolint:paralleltest // t.Setenv forbids t.Parallel
+func TestTempDirsStayOutOfListings(t *testing.T) {
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+
+	for _, tfs := range testFilesystems {
+		t.Run(tfs.name, func(t *testing.T) {
+			base, stateDir := tfs.open(t)
+			newVaultWithSecret(t, base, stateDir, "default", "first")
+
+			fs := hookFs{Fs: base, before: func(op, path string) error {
+				parent := filepath.Base(filepath.Dir(path))
+				if op == opMkdir && (parent == "secrets.d" || parent == "versions") {
+					t.Errorf("made %s where it is listed", path)
+				}
+
+				return nil
+			}}
+			vlt := vault.NewVault(fs, stateDir, "default")
+
+			value := memguard.NewBufferFromBytes([]byte("second"))
+			defer value.Destroy()
+
+			require.NoError(t, vlt.AddSecret("shared", value, true))
+			require.NoError(t, vlt.AddSecret("other", value, false))
+			require.NoError(t, vlt.CopySecretAllVersions(vlt, "shared", "other", true))
+
+			vaultDir, err := vlt.GetDirectory()
+			require.NoError(t, err)
+			require.NoError(t, secret.RemoveDirAtomic(fs,
+				filepath.Join(vaultDir, "secrets.d", "shared")))
+		})
+	}
+}
+
+// TestVersionSaveIsWholeOrAbsent checks, before every change Save makes and
+// once after it returns, that the version directory either does not exist
+// or holds all of its files: a crash at any point leaves no version that
+// cannot be decrypted.
+func TestVersionSaveIsWholeOrAbsent(t *testing.T) {
+	t.Parallel()
+
+	for _, tfs := range testFilesystems {
+		t.Run(tfs.name, func(t *testing.T) {
+			t.Parallel()
+
+			base, stateDir := tfs.open(t)
+			ltIdentity := writeLongTermKey(t, base, stateDir)
+
+			var versionDir string
+
+			checkVersionDir := func(string, string) error {
+				exists, err := afero.DirExists(base, versionDir)
+				require.NoError(t, err)
+
+				if exists {
+					assert.ElementsMatch(t,
+						[]string{"pub.age", "value.age", "priv.age", "metadata.age"},
+						dirNames(t, base, versionDir),
+						"version directory visible before it was complete")
+				}
+
+				return nil
+			}
+
+			fs := hookFs{Fs: base, before: checkVersionDir}
+			vault := &MockVersionVault{Name: testVaultName, fs: fs, stateDir: stateDir}
+			sv := secret.NewVersion(vault, "test/secret", "20231215.001")
+			versionDir = sv.Directory
+
+			value := memguard.NewBufferFromBytes([]byte("whole or nothing"))
+			defer value.Destroy()
+
+			require.NoError(t, sv.Save(value))
+			require.NoError(t, checkVersionDir("", ""))
+
+			got, err := sv.GetValue(ltIdentity)
+			require.NoError(t, err)
+
+			defer got.Destroy()
+
+			assert.Equal(t, "whole or nothing", got.String())
+		})
+	}
+}
+
+// TestVersionSaveFailureLeavesNothing makes the write of the encrypted
+// private key fail, after the value has been written, and checks that
+// neither the version nor its temporary directory is left behind.
+func TestVersionSaveFailureLeavesNothing(t *testing.T) {
+	t.Parallel()
+
+	for _, tfs := range testFilesystems {
+		t.Run(tfs.name, func(t *testing.T) {
+			t.Parallel()
+
+			base, stateDir := tfs.open(t)
+			writeLongTermKey(t, base, stateDir)
+
+			fs := hookFs{Fs: base, before: func(op, path string) error {
+				if op == opRename && filepath.Base(path) == "priv.age" {
+					return errInjected
+				}
+
+				return nil
+			}}
+			vault := &MockVersionVault{Name: testVaultName, fs: fs, stateDir: stateDir}
+			sv := secret.NewVersion(vault, "test/secret", "20231215.001")
+
+			value := memguard.NewBufferFromBytes([]byte("never stored"))
+			defer value.Destroy()
+
+			require.ErrorIs(t, sv.Save(value), errInjected)
+
+			// The secret directory holds only the empty versions directory
+			versionsDir := filepath.Dir(sv.Directory)
+			assert.Equal(t, []string{"versions"},
+				dirNames(t, base, filepath.Dir(versionsDir)))
+			assert.Empty(t, dirNames(t, base, versionsDir))
+		})
+	}
+}
+
+// TestCurrentFilesNeverMissing selects the current version, vault and
+// unlocker again and checks, before each change this makes, that the file
+// naming the current one exists: a reader or a crash never finds it
+// missing.
+//
+//nolint:paralleltest // t.Setenv forbids t.Parallel
+func TestCurrentFilesNeverMissing(t *testing.T) {
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+
+	for _, tfs := range testFilesystems {
+		t.Run(tfs.name, func(t *testing.T) {
+			base, stateDir := tfs.open(t)
+			vlt := newVaultWithSecret(t, base, stateDir, testVaultName, "value")
+
+			passphrase := memguard.NewBufferFromBytes([]byte(unlockerPassphrase))
+			defer passphrase.Destroy()
+
+			// Created as the current unlocker
+			unlocker, err := vlt.CreatePassphraseUnlocker(passphrase)
+			require.NoError(t, err)
+
+			vaultDir, err := vlt.GetDirectory()
+			require.NoError(t, err)
+
+			secretDir := filepath.Join(vaultDir, "secrets.d", "shared")
+			version, err := secret.GetCurrentVersion(base, secretDir)
+			require.NoError(t, err)
+
+			for _, tc := range []struct {
+				path     string
+				reselect func(fs afero.Fs) error
+			}{
+				{filepath.Join(secretDir, currentFile), func(fs afero.Fs) error {
+					return secret.SetCurrentVersion(fs, secretDir, version)
+				}},
+				{filepath.Join(stateDir, "currentvault"), func(fs afero.Fs) error {
+					return vault.SelectVault(fs, stateDir, testVaultName)
+				}},
+				{filepath.Join(vaultDir, "current-unlocker"), func(fs afero.Fs) error {
+					return vault.NewVault(fs, stateDir, testVaultName).
+						SelectUnlocker(unlocker.GetID())
+				}},
+			} {
+				fs := hookFs{Fs: base, before: func(string, string) error {
+					exists, err := afero.Exists(base, tc.path)
+					require.NoError(t, err)
+					assert.True(t, exists, "%s is missing", filepath.Base(tc.path))
+
+					return nil
+				}}
+
+				require.NoError(t, tc.reselect(fs))
+			}
+		})
+	}
+}
+
+// TestWriteFileAtomicTempFile checks that WriteFileAtomic creates its
+// temporary file with mode 0600, rather than wider and narrowed later, so
+// that no other user can ever read it, and syncs it before renaming it into
+// place, so that a crash cannot leave the file named but its data lost.
+func TestWriteFileAtomicTempFile(t *testing.T) {
+	t.Parallel()
+
+	for _, tfs := range testFilesystems {
+		t.Run(tfs.name, func(t *testing.T) {
+			t.Parallel()
+
+			base, dir := tfs.open(t)
+
+			var modes []os.FileMode
+
+			synced := false
+			fs := hookFs{
+				Fs: base,
+				before: func(op, _ string) error {
+					switch op {
+					case opSync:
+						synced = true
+					case opRename:
+						assert.True(t, synced, "renamed before syncing")
+					}
+
+					return nil
+				},
+				opened: func(_ string, perm os.FileMode) {
+					modes = append(modes, perm)
+				},
+			}
+
+			require.NoError(t, secret.WriteFileAtomic(fs,
+				filepath.Join(dir, currentFile), []byte("new")))
+			assert.Equal(t, []os.FileMode{secret.FilePerms}, modes)
+		})
+	}
+}
+
+// TestPassphraseUnlockerGetsKeyFirst creates a passphrase unlocker in a
+// vault whose long-term key cannot be had: it must fail without writing
+// anything, so that it never leaves a partial unlocker, nor breaks the one
+// it would replace.
+func TestPassphraseUnlockerGetsKeyFirst(t *testing.T) {
+	// No mnemonic, and no current unlocker to get the key from
+	t.Setenv(secret.EnvMnemonic, "")
+
+	base := afero.NewMemMapFs()
+	_, err := vault.CreateVault(base, testVaultStateDir, testVaultName)
+	require.NoError(t, err)
+
+	fs := hookFs{Fs: base, before: func(_, path string) error {
+		t.Errorf("changed %s before getting the long-term key", path)
+
+		return nil
+	}}
+
+	passphrase := memguard.NewBufferFromBytes([]byte(unlockerPassphrase))
+	defer passphrase.Destroy()
+
+	_, err = vault.NewVault(fs, testVaultStateDir, testVaultName).
+		CreatePassphraseUnlocker(passphrase)
+	require.Error(t, err)
+}
+
+// TestPassphraseUnlockerWritesMetadataLast checks that the last file a new
+// passphrase unlocker writes in its directory is its metadata: an unlocker
+// directory without metadata is never used, so one interrupted earlier
+// cannot be.
+func TestPassphraseUnlockerWritesMetadataLast(t *testing.T) {
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+
+	base := afero.NewMemMapFs()
+	vlt, err := vault.CreateVault(base, testVaultStateDir, testVaultName)
+	require.NoError(t, err)
+
+	vaultDir, err := vlt.GetDirectory()
+	require.NoError(t, err)
+
+	unlockerDir := filepath.Join(vaultDir, "unlockers.d", "passphrase")
+
+	var last string
+
+	fs := hookFs{Fs: base, before: func(_, path string) error {
+		if filepath.Dir(path) == unlockerDir {
+			last = filepath.Base(path)
+		}
+
+		return nil
+	}}
+
+	passphrase := memguard.NewBufferFromBytes([]byte(unlockerPassphrase))
+	defer passphrase.Destroy()
+
+	_, err = vault.NewVault(fs, testVaultStateDir, testVaultName).
+		CreatePassphraseUnlocker(passphrase)
+	require.NoError(t, err)
+	assert.Equal(t, unlockerMetadataFile, last)
+}

internal/secret/keychainunlocker.go

@@ -189,7 +189,7 @@ func (k *KeychainUnlocker) Remove() error {
 
 	// Step 3: Remove directory
 	Debug("Removing keychain unlocker directory", "directory", k.Directory)
-	if err := k.fs.RemoveAll(k.Directory); err != nil {
+	if err := RemoveDirAtomic(k.fs, k.Directory); err != nil {
 		Debug("Failed to remove keychain unlocker directory", "error", err, "directory", k.Directory)
 
 		return fmt.Errorf("failed to remove keychain unlocker directory: %w", err)
@@ -368,7 +368,7 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er
 	// Step 3: Store age recipient as plaintext
 	ageRecipient := ageIdentity.Recipient().String()
 	recipientPath := filepath.Join(unlockerDir, "pub.txt")
-	if err := afero.WriteFile(fs, recipientPath, []byte(ageRecipient), FilePerms); err != nil {
+	if err := WriteFileAtomic(fs, recipientPath, []byte(ageRecipient)); err != nil {
 		return nil, fmt.Errorf("failed to write age recipient: %w", err)
 	}
 
@@ -384,7 +384,7 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er
 	}
 
 	agePrivKeyPath := filepath.Join(unlockerDir, "priv.age")
-	if err := afero.WriteFile(fs, agePrivKeyPath, encryptedAgePrivKey, FilePerms); err != nil {
+	if err := WriteFileAtomic(fs, agePrivKeyPath, encryptedAgePrivKey); err != nil {
 		return nil, fmt.Errorf("failed to write encrypted age private key: %w", err)
 	}
 
@@ -403,7 +403,7 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er
 
 	// Write encrypted long-term private key
 	ltPrivKeyPath := filepath.Join(unlockerDir, "longterm.age")
-	if err := afero.WriteFile(fs, ltPrivKeyPath, encryptedLtPrivKeyToAge, FilePerms); err != nil {
+	if err := WriteFileAtomic(fs, ltPrivKeyPath, encryptedLtPrivKeyToAge); err != nil {
 		return nil, fmt.Errorf("failed to write encrypted long-term private key: %w", err)
 	}
 
@@ -440,9 +440,9 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er
 		return nil, fmt.Errorf("failed to marshal unlocker metadata: %w", err)
 	}
 
-	if err := afero.WriteFile(fs,
+	if err := WriteFileAtomic(fs,
 		filepath.Join(unlockerDir, "unlocker-metadata.json"),
-		metadataBytes, FilePerms); err != nil {
+		metadataBytes); err != nil {
 		return nil, fmt.Errorf("failed to write unlocker metadata: %w", err)
 	}
 

internal/secret/passphraseunlocker.go

@@ -127,7 +127,7 @@ func (p *PassphraseUnlocker) Remove() error {
 
 	// For passphrase unlockers, we just need to remove the directory
 	// No external resources (like keychain items) to clean up
-	err := p.fs.RemoveAll(p.Directory)
+	err := RemoveDirAtomic(p.fs, p.Directory)
 	if err != nil {
 		return fmt.Errorf("failed to remove passphrase unlocker directory: %w", err)
 	}

internal/secret/pgpunlocker.go

@@ -155,14 +155,18 @@ func (p *PGPUnlocker) GetDirectory() string {
 	return p.Directory
 }
 
-// GetID implements Unlocker interface - generates ID from GPG key ID
+// GetID implements Unlocker interface - generates ID from GPG key ID.
+// If the metadata has no usable GPG key ID, it warns with the unlocker's
+// directory and returns "pgp-unknown", so listing the other unlockers
+// still works.
 func (p *PGPUnlocker) GetID() string {
 	// Generate ID using GPG key ID: pgp-<keyid>
 	gpgKeyID, err := p.GetGPGKeyID()
 	if err != nil {
-		// The vault metadata is corrupt - this is a fatal error
-		// We cannot continue with a fallback ID as that would mask data corruption
-		panic(fmt.Sprintf("PGP unlocker metadata is corrupt or missing GPG key ID: %v", err))
+		Warn("PGP unlocker metadata is corrupt or missing its GPG key ID",
+			"directory", p.Directory, "error", err)
+
+		return "pgp-unknown"
 	}
 
 	return "pgp-" + gpgKeyID
@@ -172,7 +176,7 @@ func (p *PGPUnlocker) GetID() string {
 func (p *PGPUnlocker) Remove() error {
 	// For PGP unlockers, we just need to remove the directory
 	// No external resources (like keychain items) to clean up
-	err := p.fs.RemoveAll(p.Directory)
+	err := RemoveDirAtomic(p.fs, p.Directory)
 	if err != nil {
 		return fmt.Errorf("failed to remove PGP unlocker directory: %w", err)
 	}
@@ -197,6 +201,10 @@ func (p *PGPUnlocker) GetGPGKeyID() (string, error) {
 		return "", fmt.Errorf("failed to parse PGP metadata: %w", err)
 	}
 
+	if pgpMetadata.GPGKeyID == "" {
+		return "", fmt.Errorf("PGP metadata: %w", errGPGKeyIDEmpty)
+	}
+
 	return pgpMetadata.GPGKeyID, nil
 }
 
@@ -275,7 +283,7 @@ func CreatePGPUnlocker(
 	ageRecipient := ageIdentity.Recipient().String()
 	recipientPath := filepath.Join(unlockerDir, "pub.txt")
 
-	err = afero.WriteFile(fs, recipientPath, []byte(ageRecipient), FilePerms)
+	err = WriteFileAtomic(fs, recipientPath, []byte(ageRecipient))
 	if err != nil {
 		return nil, fmt.Errorf("failed to write age recipient: %w", err)
 	}
@@ -298,7 +306,7 @@ func CreatePGPUnlocker(
 	// Write encrypted long-term private key
 	ltPrivKeyPath := filepath.Join(unlockerDir, "longterm.age")
 
-	err = afero.WriteFile(fs, ltPrivKeyPath, encryptedLtPrivKeyToAge, FilePerms)
+	err = WriteFileAtomic(fs, ltPrivKeyPath, encryptedLtPrivKeyToAge)
 	if err != nil {
 		return nil, fmt.Errorf("failed to write encrypted long-term private key: %w", err)
 	}
@@ -315,7 +323,7 @@ func CreatePGPUnlocker(
 
 	agePrivKeyPath := filepath.Join(unlockerDir, "priv.age.gpg")
 
-	err = afero.WriteFile(fs, agePrivKeyPath, encryptedAgePrivKey, FilePerms)
+	err = WriteFileAtomic(fs, agePrivKeyPath, encryptedAgePrivKey)
 	if err != nil {
 		return nil, fmt.Errorf("failed to write encrypted age private key: %w", err)
 	}
@@ -357,9 +365,8 @@ func writePGPUnlockerMetadata(
 		return nil, fmt.Errorf("failed to marshal unlocker metadata: %w", err)
 	}
 
-	err = afero.WriteFile(fs,
-		filepath.Join(unlockerDir, "unlocker-metadata.json"),
-		metadataBytes, FilePerms)
+	err = WriteFileAtomic(fs,
+		filepath.Join(unlockerDir, "unlocker-metadata.json"), metadataBytes)
 	if err != nil {
 		return nil, fmt.Errorf("failed to write unlocker metadata: %w", err)
 	}

internal/secret/seunlocker_darwin.go

@@ -148,7 +148,7 @@ func (s *SecureEnclaveUnlocker) Remove() error {
 	}
 
 	Debug("Removing SE unlocker directory", "directory", s.Directory)
-	if err := s.fs.RemoveAll(s.Directory); err != nil {
+	if err := RemoveDirAtomic(s.fs, s.Directory); err != nil {
 		return fmt.Errorf("failed to remove SE unlocker directory: %w", err)
 	}
 
@@ -271,7 +271,7 @@ func CreateSecureEnclaveUnlocker(
 
 	// Write SE-encrypted long-term key
 	ltKeyPath := filepath.Join(unlockerDir, seLongtermFilename)
-	if err := afero.WriteFile(fs, ltKeyPath, encryptedLtKey, FilePerms); err != nil {
+	if err := WriteFileAtomic(fs, ltKeyPath, encryptedLtKey); err != nil {
 		return nil, fmt.Errorf(
 			"failed to write SE-encrypted long-term key: %w",
 			err,
@@ -295,7 +295,7 @@ func CreateSecureEnclaveUnlocker(
 	}
 
 	metadataPath := filepath.Join(unlockerDir, "unlocker-metadata.json")
-	if err := afero.WriteFile(fs, metadataPath, metadataBytes, FilePerms); err != nil {
+	if err := WriteFileAtomic(fs, metadataPath, metadataBytes); err != nil {
 		return nil, fmt.Errorf("failed to write metadata: %w", err)
 	}
 

internal/secret/version.go

@@ -132,7 +132,10 @@ func GenerateVersionName(fs afero.Fs, secretDir string) (string, error) {
 	return fmt.Sprintf("%s.%03d", today, newSerial), nil
 }
 
-// Save saves the version metadata and value
+// Save saves the version metadata and value. The files are written into a
+// temporary directory that is renamed to sv.Directory once all of them are
+// complete, so the version directory is either whole or absent, even if the
+// process dies part-way.
 func (sv *Version) Save(value *memguard.LockedBuffer) error {
 	if value == nil {
 		return errNilValueBuffer
@@ -146,14 +149,22 @@ func (sv *Version) Save(value *memguard.LockedBuffer) error {
 
 	fs := sv.vault.GetFilesystem()
 
-	// Create version directory
-	err := fs.MkdirAll(sv.Directory, DirPerms)
+	// Create the versions directory the finished version is renamed into
+	err := fs.MkdirAll(filepath.Dir(sv.Directory), DirPerms)
 	if err != nil {
-		Debug("Failed to create version directory", "error", err, "dir", sv.Directory)
+		Debug("Failed to create versions directory", "error", err, "dir", sv.Directory)
 
-		return fmt.Errorf("failed to create version directory: %w", err)
+		return fmt.Errorf("failed to create versions directory: %w", err)
 	}
 
+	tmpDir, err := TempDirFor(fs, sv.Directory)
+	if err != nil {
+		return err
+	}
+
+	// Once the rename below has moved it into place, this finds nothing.
+	defer func() { _ = fs.RemoveAll(tmpDir) }()
+
 	// Generate a new keypair for this version
 	Debug("Generating version-specific keypair", "version", sv.Version)
 
@@ -174,21 +185,28 @@ func (sv *Version) Save(value *memguard.LockedBuffer) error {
 		slog.String("public_key", versionIdentity.Recipient().String()),
 	)
 
-	err = sv.writePublicKeyAndValue(fs, versionIdentity, value)
+	err = sv.writePublicKeyAndValue(fs, tmpDir, versionIdentity, value)
 	if err != nil {
 		return err
 	}
 
-	err = sv.writeEncryptedPrivateKey(fs, versionPrivateKeyBuffer)
+	err = sv.writeEncryptedPrivateKey(fs, tmpDir, versionPrivateKeyBuffer)
 	if err != nil {
 		return err
 	}
 
-	err = sv.writeEncryptedMetadata(fs, versionIdentity)
+	err = sv.writeEncryptedMetadata(fs, tmpDir, versionIdentity)
 	if err != nil {
 		return err
 	}
 
+	err = fs.Rename(tmpDir, sv.Directory)
+	if err != nil {
+		Debug("Failed to move version into place", "error", err, "dir", sv.Directory)
+
+		return fmt.Errorf("failed to move version into place: %w", err)
+	}
+
 	Debug("Successfully saved secret version",
 		"version", sv.Version, "secret_name", sv.SecretName)
 
@@ -359,17 +377,18 @@ func (sv *Version) GetValue(
 }
 
 // writePublicKeyAndValue stores the version's public key and the value
-// encrypted to it.
+// encrypted to it in dir.
 func (sv *Version) writePublicKeyAndValue(
 	fs afero.Fs,
+	dir string,
 	versionIdentity *age.X25519Identity,
 	value *memguard.LockedBuffer,
 ) error {
 	versionPublicKey := versionIdentity.Recipient().String()
-	pubKeyPath := filepath.Join(sv.Directory, "pub.age")
+	pubKeyPath := filepath.Join(dir, "pub.age")
 	Debug("Writing version public key", "path", pubKeyPath)
 
-	err := afero.WriteFile(fs, pubKeyPath, []byte(versionPublicKey), FilePerms)
+	err := WriteFileAtomic(fs, pubKeyPath, []byte(versionPublicKey))
 	if err != nil {
 		Debug("Failed to write version public key", "error", err, "path", pubKeyPath)
 
@@ -386,10 +405,10 @@ func (sv *Version) writePublicKeyAndValue(
 		return fmt.Errorf("failed to encrypt version value: %w", err)
 	}
 
-	valuePath := filepath.Join(sv.Directory, "value.age")
+	valuePath := filepath.Join(dir, "value.age")
 	Debug("Writing encrypted version value", "path", valuePath)
 
-	err = afero.WriteFile(fs, valuePath, encryptedValue, FilePerms)
+	err = WriteFileAtomic(fs, valuePath, encryptedValue)
 	if err != nil {
 		Debug("Failed to write encrypted version value", "error", err, "path", valuePath)
 
@@ -400,9 +419,10 @@ func (sv *Version) writePublicKeyAndValue(
 }
 
 // writeEncryptedPrivateKey encrypts the version's private key to the
-// vault's long-term public key and stores it.
+// vault's long-term public key and stores it in dir.
 func (sv *Version) writeEncryptedPrivateKey(
 	fs afero.Fs,
+	dir string,
 	versionPrivateKeyBuffer *memguard.LockedBuffer,
 ) error {
 	vaultDir, _ := sv.vault.GetDirectory()
@@ -436,10 +456,10 @@ func (sv *Version) writeEncryptedPrivateKey(
 		return fmt.Errorf("failed to encrypt version private key: %w", err)
 	}
 
-	privKeyPath := filepath.Join(sv.Directory, "priv.age")
+	privKeyPath := filepath.Join(dir, "priv.age")
 	Debug("Writing encrypted version private key", "path", privKeyPath)
 
-	err = afero.WriteFile(fs, privKeyPath, encryptedPrivKey, FilePerms)
+	err = WriteFileAtomic(fs, privKeyPath, encryptedPrivKey)
 	if err != nil {
 		Debug("Failed to write encrypted version private key",
 			"error", err, "path", privKeyPath)
@@ -451,9 +471,10 @@ func (sv *Version) writeEncryptedPrivateKey(
 }
 
 // writeEncryptedMetadata encrypts the version metadata to the version's
-// public key and stores it.
+// public key and stores it in dir.
 func (sv *Version) writeEncryptedMetadata(
 	fs afero.Fs,
+	dir string,
 	versionIdentity *age.X25519Identity,
 ) error {
 	Debug("Encrypting version metadata", "version", sv.Version)
@@ -477,10 +498,10 @@ func (sv *Version) writeEncryptedMetadata(
 		return fmt.Errorf("failed to encrypt version metadata: %w", err)
 	}
 
-	metadataPath := filepath.Join(sv.Directory, "metadata.age")
+	metadataPath := filepath.Join(dir, "metadata.age")
 	Debug("Writing encrypted version metadata", "path", metadataPath)
 
-	err = afero.WriteFile(fs, metadataPath, encryptedMetadata, FilePerms)
+	err = WriteFileAtomic(fs, metadataPath, encryptedMetadata)
 	if err != nil {
 		Debug("Failed to write encrypted version metadata",
 			"error", err, "path", metadataPath)
@@ -553,15 +574,12 @@ func GetCurrentVersion(fs afero.Fs, secretDir string) (string, error) {
 }
 
 // SetCurrentVersion updates the "current" file to point to a specific version
-// The file contains just the version name (e.g., "20231215.001")
+// The file contains just the version name (e.g., "20231215.001"). It is
+// replaced in one rename, so once written it always exists.
 func SetCurrentVersion(fs afero.Fs, secretDir string, version string) error {
 	currentPath := filepath.Join(secretDir, "current")
 
-	// Remove existing file if it exists
-	_ = fs.Remove(currentPath)
-
-	// Write just the version name to the file
-	err := afero.WriteFile(fs, currentPath, []byte(version), FilePerms)
+	err := WriteFileAtomic(fs, currentPath, []byte(version))
 	if err != nil {
 		return fmt.Errorf("failed to create current version file: %w", err)
 	}

internal/vault/errors.go

@@ -63,4 +63,10 @@ var (
 	// ErrUnlockerNotFound indicates no unlocker with the given ID exists.
 	// Composed as "unlocker with ID <id> not found".
 	ErrUnlockerNotFound = errors.New("not found")
+
+	// ErrNoLockForFilesystem indicates LockStateDir was given a filesystem
+	// it cannot lock. Composed as "cannot lock the state directory on
+	// filesystem <type>".
+	ErrNoLockForFilesystem = errors.New(
+		"cannot lock the state directory on filesystem")
 )

internal/vault/lock.go

@@ -0,0 +1,73 @@
+package vault
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"sync"
+	"syscall"
+
+	"git.eeqj.de/sneak/secret/internal/secret"
+	"github.com/spf13/afero"
+)
+
+// lockFileName is the file in the state directory that LockStateDir locks.
+const lockFileName = "lock"
+
+// memFsLock stands in for the lock file on the in-memory filesystem, which
+// has no file locks. Every in-memory filesystem in the process shares it.
+//
+//nolint:gochecknoglobals // must outlive the call that takes it
+var memFsLock sync.Mutex
+
+// LockStateDir takes the lock that a command changing anything under
+// stateDir holds until it returns, and returns the function that releases
+// it. While one command holds it, the next one waits here. Reads take no
+// lock: each file or directory a command changes is replaced in a single
+// rename, so a reader finds it as it was before or after, never half-made.
+//
+// On the real filesystem the lock is flock(2) on the file "lock" in
+// stateDir, which the kernel releases when the process dies, so a killed
+// command never leaves the tool locked. The in-memory filesystem the tests
+// use has no file locks, so a process-wide mutex stands in for flock there.
+// Any other filesystem is refused rather than left unlocked.
+func LockStateDir(fs afero.Fs, stateDir string) (func(), error) {
+	switch fs.(type) {
+	case *afero.OsFs:
+		return flockStateDir(stateDir)
+	case *afero.MemMapFs:
+		memFsLock.Lock()
+
+		return memFsLock.Unlock, nil
+	default:
+		return nil, fmt.Errorf("%w %T", ErrNoLockForFilesystem, fs)
+	}
+}
+
+// flockStateDir takes flock(2) on the lock file in stateDir, creating the
+// directory and the file if needed. Go opens files close-on-exec, so
+// programs the command runs, such as gpg, do not inherit the lock.
+func flockStateDir(stateDir string) (func(), error) {
+	err := os.MkdirAll(stateDir, secret.DirPerms)
+	if err != nil {
+		return nil, fmt.Errorf("failed to create state directory: %w", err)
+	}
+
+	lockPath := filepath.Join(stateDir, lockFileName)
+
+	//nolint:gosec // G304: the path is the lock file in the state directory
+	file, err := os.OpenFile(lockPath, os.O_RDWR|os.O_CREATE, secret.FilePerms)
+	if err != nil {
+		return nil, fmt.Errorf("failed to open lock file: %w", err)
+	}
+
+	err = syscall.Flock(int(file.Fd()), syscall.LOCK_EX)
+	if err != nil {
+		_ = file.Close()
+
+		return nil, fmt.Errorf("failed to lock %s: %w", lockPath, err)
+	}
+
+	// Closing the file releases the lock.
+	return func() { _ = file.Close() }, nil
+}

internal/vault/lock_test.go

@@ -0,0 +1,134 @@
+package vault_test
+
+import (
+	"testing"
+	"time"
+
+	"git.eeqj.de/sneak/secret/internal/vault"
+	"github.com/spf13/afero"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+const (
+	// lockWait is how long a test waits for the lock before deciding it
+	// will never come free.
+	lockWait = 10 * time.Second
+
+	// heldWait is how long a test watches a second holder fail to take a
+	// lock that is held. Broken exclusion lets it in at once.
+	heldWait = 100 * time.Millisecond
+)
+
+// lockFilesystem is a filesystem LockStateDir can lock, with a state
+// directory on it.
+type lockFilesystem struct {
+	name     string
+	fs       afero.Fs
+	stateDir string
+}
+
+// lockFilesystems returns the real filesystem, locked with flock, and the
+// in-memory one, locked with a mutex.
+func lockFilesystems(t *testing.T) []lockFilesystem {
+	t.Helper()
+
+	return []lockFilesystem{
+		{"memory", afero.NewMemMapFs(), testStateDir},
+		{"real", afero.NewOsFs(), t.TempDir()},
+	}
+}
+
+// lockInBackground starts taking the lock and returns a channel that
+// delivers the function releasing it once it has been taken.
+func lockInBackground(
+	t *testing.T, fs afero.Fs, stateDir string,
+) <-chan func() {
+	t.Helper()
+
+	taken := make(chan func(), 1)
+
+	go func() {
+		release, err := vault.LockStateDir(fs, stateDir)
+		if assert.NoError(t, err) {
+			taken <- release
+		}
+	}()
+
+	return taken
+}
+
+// TestLockStateDirExcludes checks that while the lock is held a second
+// holder, with its own open lock file on the real filesystem, waits, and
+// that it gets the lock once the first releases it.
+func TestLockStateDirExcludes(t *testing.T) {
+	t.Parallel()
+
+	for _, lfs := range lockFilesystems(t) {
+		t.Run(lfs.name, func(t *testing.T) {
+			t.Parallel()
+
+			release, err := vault.LockStateDir(lfs.fs, lfs.stateDir)
+			require.NoError(t, err)
+
+			taken := lockInBackground(t, lfs.fs, lfs.stateDir)
+
+			select {
+			case second := <-taken:
+				second()
+				release()
+				t.Fatal("a second holder took the lock while it was held")
+			case <-time.After(heldWait):
+			}
+
+			release()
+
+			select {
+			case second := <-taken:
+				second()
+			case <-time.After(lockWait):
+				t.Fatal("the second holder never got the lock")
+			}
+		})
+	}
+}
+
+// TestLockStateDirFreeAfterPanic checks that a holder that panics, and
+// releases the lock with defer as every command does, leaves it free.
+func TestLockStateDirFreeAfterPanic(t *testing.T) {
+	t.Parallel()
+
+	for _, lfs := range lockFilesystems(t) {
+		t.Run(lfs.name, func(t *testing.T) {
+			t.Parallel()
+
+			assert.Panics(t, func() {
+				release, err := vault.LockStateDir(lfs.fs, lfs.stateDir)
+				require.NoError(t, err)
+
+				defer release()
+
+				panic("the command failed")
+			})
+
+			select {
+			case release := <-lockInBackground(t, lfs.fs, lfs.stateDir):
+				release()
+			case <-time.After(lockWait):
+				t.Fatal("the lock was still held after its holder panicked")
+			}
+		})
+	}
+}
+
+// TestLockStateDirRefusesOtherFilesystems checks that a filesystem with no
+// lock implementation is refused instead of being used unlocked.
+func TestLockStateDirRefusesOtherFilesystems(t *testing.T) {
+	t.Parallel()
+
+	fs := afero.NewReadOnlyFs(afero.NewMemMapFs())
+
+	release, err := vault.LockStateDir(fs, testStateDir)
+	require.ErrorIs(t, err, vault.ErrNoLockForFilesystem)
+	assert.Nil(t, release)
+}

internal/vault/management.go

@@ -169,7 +169,7 @@ func processMnemonicForVault(
 
 	ltPubKeyPath := filepath.Join(vaultDir, "pub.age")
 
-	err = afero.WriteFile(fs, ltPubKeyPath, []byte(ltPubKey), secret.FilePerms)
+	err = secret.WriteFileAtomic(fs, ltPubKeyPath, []byte(ltPubKey))
 	if err != nil {
 		return 0, "", "", fmt.Errorf("failed to write long-term public key: %w", err)
 	}
@@ -295,21 +295,13 @@ func SelectVault(fs afero.Fs, stateDir string, name string) error {
 		return fmt.Errorf("vault %s %w", name, ErrVaultNotFound)
 	}
 
-	// Create or update the currentvault file with just the vault name
+	// Create or replace the currentvault file with just the vault name. It
+	// is replaced in one rename, so it never goes missing.
 	currentVaultPath := filepath.Join(stateDir, "currentvault")
 
-	// Remove existing file if it exists
-	_, err = fs.Stat(currentVaultPath)
-	if err == nil {
-		secret.Debug("Removing existing currentvault file", "path", currentVaultPath)
-
-		_ = fs.Remove(currentVaultPath)
-	}
-
-	// Write just the vault name to the file
 	secret.Debug("Writing currentvault file", "vault_name", name)
 
-	err = afero.WriteFile(fs, currentVaultPath, []byte(name), secret.FilePerms)
+	err = secret.WriteFileAtomic(fs, currentVaultPath, []byte(name))
 	if err != nil {
 		return fmt.Errorf("failed to select vault: %w", err)
 	}

internal/vault/metadata.go

@@ -113,7 +113,7 @@ func SaveVaultMetadata(fs afero.Fs, vaultDir string, metadata *Metadata) error {
 		return fmt.Errorf("failed to marshal vault metadata: %w", err)
 	}
 
-	err = afero.WriteFile(fs, metadataPath, metadataBytes, secret.FilePerms)
+	err = secret.WriteFileAtomic(fs, metadataPath, metadataBytes)
 	if err != nil {
 		return fmt.Errorf("failed to write vault metadata: %w", err)
 	}

internal/vault/secrets.go

@@ -171,17 +171,59 @@ func (v *Vault) AddSecret(name string, value *memguard.LockedBuffer, force bool)
 		slog.String("secret_dir", secretDir),
 	)
 
-	// Check for an existing secret and prepare its directory
-	exists, previousVersion, err := v.prepareSecretDir(name, secretDir, force)
+	// Check for an existing secret and the version the new one supersedes
+	exists, previousVersion, err := v.checkExistingSecret(name, secretDir, force)
 	if err != nil {
 		return err
 	}
 
+	if exists {
+		return v.addVersion(name, secretDir, value, previousVersion)
+	}
+
+	return v.addNewSecret(name, secretDir, value)
+}
+
+// addNewSecret creates a secret by assembling its first version and current
+// pointer in a temporary directory, then renaming that directory to
+// secretDir, so an interrupted add leaves no half-made secret behind.
+func (v *Vault) addNewSecret(
+	name, secretDir string, value *memguard.LockedBuffer,
+) error {
+	buildDir, err := secret.TempDirFor(v.fs, secretDir)
+	if err != nil {
+		return err
+	}
+
+	// Once the rename below has moved it into place, this finds nothing.
+	defer func() { _ = v.fs.RemoveAll(buildDir) }()
+
+	err = v.addVersion(name, buildDir, value, nil)
+	if err != nil {
+		return err
+	}
+
+	err = v.fs.Rename(buildDir, secretDir)
+	if err != nil {
+		return fmt.Errorf("failed to move new secret into place: %w", err)
+	}
+
+	return nil
+}
+
+// addVersion saves value as a new version under secretDir, sets the
+// notAfter timestamp of the version it supersedes, if any, and then points
+// current at the new version. Until that last step, current still names the
+// previous version, which stays readable.
+func (v *Vault) addVersion(
+	name, secretDir string, value *memguard.LockedBuffer,
+	previousVersion *secret.Version,
+) error {
 	now := time.Now()
 
 	// Create the new version and save the encrypted value
 	versionName, err := v.createAndSaveVersion(
-		name, secretDir, value, previousVersion, &now, exists)
+		name, secretDir, value, previousVersion, &now)
 	if err != nil {
 		return err
 	}
@@ -251,7 +293,7 @@ func updateVersionMetadata(
 	// Write encrypted metadata
 	metadataPath := filepath.Join(version.Directory, "metadata.age")
 
-	err = afero.WriteFile(fs, metadataPath, encryptedMetadata, secret.FilePerms)
+	err = secret.WriteFileAtomic(fs, metadataPath, encryptedMetadata)
 	if err != nil {
 		return fmt.Errorf("failed to write encrypted version metadata: %w", err)
 	}
@@ -423,12 +465,14 @@ func (v *Vault) GetSecretObject(name string) (*secret.Secret, error) {
 	return secretObj, nil
 }
 
-// CopySecretVersion copies a single version from source to this vault
-// It decrypts the value using srcIdentity and re-encrypts for this vault
+// CopySecretVersion copies a single version from source into destSecretDir
+// in this vault. It decrypts the value using srcIdentity and re-encrypts
+// for this vault.
 func (v *Vault) CopySecretVersion(
 	srcVersion *secret.Version,
 	srcIdentity *age.X25519Identity,
 	destSecretName string,
+	destSecretDir string,
 	destVersionName string,
 ) error {
 	secret.DebugWith("Copying secret version to vault",
@@ -454,6 +498,7 @@ func (v *Vault) CopySecretVersion(
 
 	// Create destination version with same name
 	destVersion := secret.NewVersion(v, destSecretName, destVersionName)
+	destVersion.Directory = filepath.Join(destSecretDir, "versions", destVersionName)
 
 	// Copy metadata (preserve original timestamps)
 	destVersion.Metadata = srcVersion.Metadata
@@ -494,11 +539,11 @@ func (v *Vault) CopySecretAllVersions(
 		return fmt.Errorf("failed to get destination vault directory: %w", err)
 	}
 
-	// Check if destination secret already exists and clear it if forced
+	// Refuse to replace an existing destination secret unless forced
 	destStorageName := strings.ReplaceAll(destSecretName, "/", "%")
 	destSecretDir := filepath.Join(destVaultDir, "secrets.d", destStorageName)
 
-	err = v.prepareCopyDestination(destSecretDir, destSecretName, force)
+	err = v.checkCopyDestination(destSecretDir, destSecretName, force)
 	if err != nil {
 		return err
 	}
@@ -534,14 +579,8 @@ func (v *Vault) CopySecretAllVersions(
 		return fmt.Errorf("failed to get current version: %w", err)
 	}
 
-	// Create destination secret directory
-	err = v.fs.MkdirAll(destSecretDir, secret.DirPerms)
-	if err != nil {
-		return fmt.Errorf("failed to create destination secret directory: %w", err)
-	}
-
-	// Copy each version and set the current pointer, rolling back on error
-	err = v.copyVersionsWithRollback(srcVault, srcIdentity,
+	// Copy each version and the current pointer, then move the copy into place
+	err = v.copyVersions(srcVault, srcIdentity,
 		srcSecretName, destSecretName, destSecretDir, versions, currentVersion)
 	if err != nil {
 		return err
@@ -556,10 +595,10 @@ func (v *Vault) CopySecretAllVersions(
 	return nil
 }
 
-// prepareSecretDir checks for an existing secret directory and prepares it
-// for a new version. It returns whether the secret already existed and the
-// current version to be superseded, if any.
-func (v *Vault) prepareSecretDir(
+// checkExistingSecret reports whether the secret already exists, refuses to
+// overwrite it unless force is set, and returns its current version, which
+// the new version supersedes, if any.
+func (v *Vault) checkExistingSecret(
 	name, secretDir string, force bool,
 ) (bool, *secret.Version, error) {
 	// Check if secret already exists
@@ -576,19 +615,6 @@ func (v *Vault) prepareSecretDir(
 	secret.Debug("Secret existence check complete", "exists", exists)
 
 	if !exists {
-		// Create secret directory for new secret
-		secret.Debug("Creating secret directory", "secret_dir", secretDir)
-
-		err = v.fs.MkdirAll(secretDir, secret.DirPerms)
-		if err != nil {
-			secret.Debug("Failed to create secret directory",
-				"error", err, "secret_dir", secretDir)
-
-			return false, nil, fmt.Errorf("failed to create secret directory: %w", err)
-		}
-
-		secret.Debug("Created secret directory successfully")
-
 		return false, nil, nil
 	}
 
@@ -708,11 +734,11 @@ func (v *Vault) checkSecretVersion(name, version string) error {
 }
 
 // createAndSaveVersion generates a new version name, sets the version
-// timestamps, and saves the encrypted value. When saving fails for a newly
-// created secret, the secret directory is removed again.
+// timestamps, and saves the encrypted value under secretDir, which is a
+// temporary directory while a new secret is being assembled.
 func (v *Vault) createAndSaveVersion(
 	name, secretDir string, value *memguard.LockedBuffer,
-	previousVersion *secret.Version, now *time.Time, exists bool,
+	previousVersion *secret.Version, now *time.Time,
 ) (string, error) {
 	// Generate new version name
 	versionName, err := secret.GenerateVersionName(v.fs, secretDir)
@@ -726,6 +752,7 @@ func (v *Vault) createAndSaveVersion(
 
 	// Create new version
 	newVersion := secret.NewVersion(v, name, versionName)
+	newVersion.Directory = filepath.Join(secretDir, "versions", versionName)
 
 	// Set version timestamps
 	if previousVersion == nil {
@@ -745,57 +772,73 @@ func (v *Vault) createAndSaveVersion(
 	if err != nil {
 		secret.Debug("Failed to save new version", "error", err, "version", versionName)
 
-		// Clean up the secret directory if this was a new secret
-		if !exists {
-			secret.Debug("Cleaning up secret directory due to save failure",
-				"secret_dir", secretDir)
-
-			_ = v.fs.RemoveAll(secretDir)
-		}
-
 		return "", fmt.Errorf("failed to save version: %w", err)
 	}
 
 	return versionName, nil
 }
 
-// copyVersionsWithRollback copies each version of the source secret into the
-// destination directory and sets the current version pointer, removing the
-// partial copy when any step fails.
-func (v *Vault) copyVersionsWithRollback(
+// copyVersions copies each version of the source secret and its current
+// pointer into a temporary directory, then moves that directory to
+// destSecretDir, replacing a secret already there. Nothing in this vault
+// changes until the copy is complete, so an interrupted copy leaves only a
+// temporary directory behind.
+func (v *Vault) copyVersions(
 	srcVault *Vault, srcIdentity *age.X25519Identity,
 	srcSecretName, destSecretName, destSecretDir string,
 	versions []string, currentVersion string,
 ) error {
-	// Copy each version
+	buildDir, err := secret.TempDirFor(v.fs, destSecretDir)
+	if err != nil {
+		return err
+	}
+
+	// Once the rename below has moved it into place, this finds nothing.
+	defer func() { _ = v.fs.RemoveAll(buildDir) }()
+
 	for _, versionName := range versions {
 		srcVersion := secret.NewVersion(srcVault, srcSecretName, versionName)
 
-		err := v.CopySecretVersion(srcVersion, srcIdentity, destSecretName, versionName)
+		err = v.CopySecretVersion(
+			srcVersion, srcIdentity, destSecretName, buildDir, versionName)
 		if err != nil {
-			// Rollback: remove partial copy
-			secret.Debug("Rolling back partial copy due to error", "error", err)
-
-			_ = v.fs.RemoveAll(destSecretDir)
-
 			return fmt.Errorf("failed to copy version %s: %w", versionName, err)
 		}
 	}
 
-	// Set current version
-	err := secret.SetCurrentVersion(v.fs, destSecretDir, currentVersion)
+	err = secret.SetCurrentVersion(v.fs, buildDir, currentVersion)
 	if err != nil {
-		_ = v.fs.RemoveAll(destSecretDir)
-
 		return fmt.Errorf("failed to set current version: %w", err)
 	}
 
+	// With --force, the secret being replaced goes only now that its
+	// replacement is complete
+	exists, err := afero.DirExists(v.fs, destSecretDir)
+	if err != nil {
+		return fmt.Errorf("failed to check destination: %w", err)
+	}
+
+	if exists {
+		secret.Debug("Removing existing destination secret", "path", destSecretDir)
+
+		err = secret.RemoveDirAtomic(v.fs, destSecretDir)
+		if err != nil {
+			return fmt.Errorf("failed to remove existing destination secret: %w", err)
+		}
+	}
+
+	err = v.fs.Rename(buildDir, destSecretDir)
+	if err != nil {
+		return fmt.Errorf("failed to move copied secret into place: %w", err)
+	}
+
 	return nil
 }
 
-// prepareCopyDestination ensures the destination secret directory can be
-// created, removing an existing secret when force is set.
-func (v *Vault) prepareCopyDestination(
+// checkCopyDestination refuses to copy over an existing secret unless force
+// is set. A secret being replaced is removed by copyVersions, once its
+// replacement is complete.
+func (v *Vault) checkCopyDestination(
 	destSecretDir, destSecretName string, force bool,
 ) error {
 	exists, err := afero.DirExists(v.fs, destSecretDir)
@@ -810,15 +853,5 @@ func (v *Vault) prepareCopyDestination(
 		)
 	}
 
-	if exists && force {
-		// Remove existing secret
-		secret.Debug("Removing existing destination secret", "path", destSecretDir)
-
-		err = v.fs.RemoveAll(destSecretDir)
-		if err != nil {
-			return fmt.Errorf("failed to remove existing destination secret: %w", err)
-		}
-	}
-
 	return nil
 }

internal/vault/unlockers.go

@@ -233,9 +233,10 @@ func (v *Vault) ListUnlockers() ([]UnlockerMetadata, error) {
 
 			exists, err := afero.Exists(v.fs, metadataPath)
 			if err != nil {
-				return nil, fmt.Errorf(
-					"failed to check if metadata exists for unlocker %s: %w",
-					file.Name(), err)
+				secret.Warn("Skipping unlocker directory whose metadata file cannot be checked",
+					"directory", file.Name(), "error", err)
+
+				continue
 			}
 
 			if !exists {
@@ -247,16 +248,20 @@ func (v *Vault) ListUnlockers() ([]UnlockerMetadata, error) {
 
 			metadataBytes, err := afero.ReadFile(v.fs, metadataPath)
 			if err != nil {
-				return nil, fmt.Errorf(
-					"failed to read metadata for unlocker %s: %w", file.Name(), err)
+				secret.Warn("Skipping unlocker directory with unreadable metadata file",
+					"directory", file.Name(), "error", err)
+
+				continue
 			}
 
 			var metadata UnlockerMetadata
 
 			err = json.Unmarshal(metadataBytes, &metadata)
 			if err != nil {
-				return nil, fmt.Errorf(
-					"failed to parse metadata for unlocker %s: %w", file.Name(), err)
+				secret.Warn("Skipping unlocker directory with corrupt metadata file",
+					"directory", file.Name(), "error", err)
+
+				continue
 			}
 
 			unlockers = append(unlockers, metadata)
@@ -310,30 +315,16 @@ func (v *Vault) SelectUnlocker(unlockerID string) error {
 		return fmt.Errorf("unlocker with ID %s %w", unlockerID, ErrUnlockerNotFound)
 	}
 
-	// Create/update current-unlocker file with just the unlocker name
+	// Create or replace the current-unlocker file with just the unlocker
+	// name. It is replaced in one rename, so it never goes missing.
 	currentUnlockerPath := filepath.Join(vaultDir, "current-unlocker")
 
-	// Remove existing file if it exists
-	exists, err := afero.Exists(v.fs, currentUnlockerPath)
-	if err != nil {
-		return fmt.Errorf("failed to check if current-unlocker file exists: %w", err)
-	}
-
-	if exists {
-		err = v.fs.Remove(currentUnlockerPath)
-		if err != nil {
-			return fmt.Errorf("failed to remove existing current-unlocker file: %w", err)
-		}
-	}
-
 	// Get just the unlocker name (basename of the directory)
 	unlockerName := filepath.Base(targetUnlockerDir)
 
-	// Write just the unlocker name to the file
 	secret.Debug("Writing current-unlocker file", "unlocker_name", unlockerName)
 
-	err = afero.WriteFile(v.fs, currentUnlockerPath, []byte(unlockerName),
-		secret.FilePerms)
+	err = secret.WriteFileAtomic(v.fs, currentUnlockerPath, []byte(unlockerName))
 	if err != nil {
 		return fmt.Errorf("failed to create current-unlocker file: %w", err)
 	}
@@ -351,6 +342,14 @@ func (v *Vault) CreatePassphraseUnlocker(
 		return nil, fmt.Errorf("failed to get vault directory: %w", err)
 	}
 
+	// We need to get the long-term key (either from memory if unlocked, or
+	// derive it). Getting it before anything is written means failing to
+	// get it changes nothing, even when replacing the current unlocker.
+	ltIdentity, err := v.GetOrDeriveLongTermKey()
+	if err != nil {
+		return nil, fmt.Errorf("failed to get long-term key: %w", err)
+	}
+
 	// Create unlocker directory
 	unlockerDir := filepath.Join(vaultDir, "unlockers.d", unlockerTypePassphrase)
 
@@ -371,33 +370,7 @@ func (v *Vault) CreatePassphraseUnlocker(
 		return nil, err
 	}
 
-	// Create metadata
-	metadata := UnlockerMetadata{
-		Type:      unlockerTypePassphrase,
-		CreatedAt: time.Now(),
-		Flags:     []string{},
-	}
-
-	// Write metadata
-	metadataBytes, err := json.MarshalIndent(metadata, "", "  ")
-	if err != nil {
-		return nil, fmt.Errorf("failed to marshal metadata: %w", err)
-	}
-
-	metadataPath := filepath.Join(unlockerDir, "unlocker-metadata.json")
-
-	err = afero.WriteFile(v.fs, metadataPath, metadataBytes, secret.FilePerms)
-	if err != nil {
-		return nil, fmt.Errorf("failed to write unlocker metadata: %w", err)
-	}
-
 	// Encrypt long-term private key to this unlocker
-	// We need to get the long-term key (either from memory if unlocked, or derive it)
-	ltIdentity, err := v.GetOrDeriveLongTermKey()
-	if err != nil {
-		return nil, fmt.Errorf("failed to get long-term key: %w", err)
-	}
-
 	ltPrivKeyBuffer := memguard.NewBufferFromBytes([]byte(ltIdentity.String()))
 	defer ltPrivKeyBuffer.Destroy()
 
@@ -409,11 +382,31 @@ func (v *Vault) CreatePassphraseUnlocker(
 
 	ltPrivKeyPath := filepath.Join(unlockerDir, "longterm.age")
 
-	err = afero.WriteFile(v.fs, ltPrivKeyPath, encryptedLtPrivKey, secret.FilePerms)
+	err = secret.WriteFileAtomic(v.fs, ltPrivKeyPath, encryptedLtPrivKey)
 	if err != nil {
 		return nil, fmt.Errorf("failed to write encrypted long-term private key: %w", err)
 	}
 
+	// Write the metadata last: readers skip an unlocker directory without
+	// it, so an unlocker interrupted before this point is never used.
+	metadata := UnlockerMetadata{
+		Type:      unlockerTypePassphrase,
+		CreatedAt: time.Now(),
+		Flags:     []string{},
+	}
+
+	metadataBytes, err := json.MarshalIndent(metadata, "", "  ")
+	if err != nil {
+		return nil, fmt.Errorf("failed to marshal metadata: %w", err)
+	}
+
+	metadataPath := filepath.Join(unlockerDir, "unlocker-metadata.json")
+
+	err = secret.WriteFileAtomic(v.fs, metadataPath, metadataBytes)
+	if err != nil {
+		return nil, fmt.Errorf("failed to write unlocker metadata: %w", err)
+	}
+
 	// Create the unlocker instance
 	unlocker := secret.NewPassphraseUnlocker(v.fs, unlockerDir, metadata)
 
@@ -467,9 +460,8 @@ func (v *Vault) writeUnlockerKeypair(
 	// Write public key
 	pubKeyPath := filepath.Join(unlockerDir, "pub.age")
 
-	err := afero.WriteFile(v.fs, pubKeyPath,
-		[]byte(unlockerIdentity.Recipient().String()),
-		secret.FilePerms)
+	err := secret.WriteFileAtomic(v.fs, pubKeyPath,
+		[]byte(unlockerIdentity.Recipient().String()))
 	if err != nil {
 		return fmt.Errorf("failed to write unlocker public key: %w", err)
 	}
@@ -488,7 +480,7 @@ func (v *Vault) writeUnlockerKeypair(
 	// Write encrypted private key
 	privKeyPath := filepath.Join(unlockerDir, "priv.age")
 
-	err = afero.WriteFile(v.fs, privKeyPath, encryptedPrivKey, secret.FilePerms)
+	err = secret.WriteFileAtomic(v.fs, privKeyPath, encryptedPrivKey)
 	if err != nil {
 		return fmt.Errorf("failed to write encrypted unlocker private key: %w", err)
 	}

internal/vault/vault_error_test.go

@@ -90,4 +90,10 @@ func TestAddSecretCleansUpOnFailure(t *testing.T) {
 	secretDir := filepath.Join(vaultDir, "secrets.d", testSecretName)
 	exists, _ := afero.DirExists(fs, secretDir)
 	assert.False(t, exists, "Secret directory should not exist after failed AddSecret")
+
+	// Nor is the temporary directory the secret was assembled in left behind
+	entries, err := afero.ReadDir(fs, vaultDir)
+	require.NoError(t, err)
+	require.Len(t, entries, 1)
+	assert.Equal(t, "pub.age", entries[0].Name())
 }

script/bootstrap

@@ -6,6 +6,7 @@
 # make, node, yarn, go, or python). Node is used directly if installed;
 # otherwise a pinned version is installed via nvm (installing nvm
 # itself first, from a hash-verified release archive, never curl | sh).
+# golangci-lint is never installed: script/lint runs it in docker.
 #
 # Uncomment the language sections in main() that apply to this repo.
 set -eu
@@ -136,12 +137,6 @@ main() {
 
     # ---- Go repos ----
     if missing go; then pkg_install go golang go go; fi
-    # golangci-lint: packaged in nix, brew, and apk. On apt there is no
-    # package: download a specific release archive from GitHub and
-    # verify its hash (verify_sha256), never curl | sh.
-    if missing golangci-lint; then
-        pkg_install golangci-lint golangci-lint golangci-lint golangci-lint
-    fi
     go mod download
 
     # ---- Python repos ----

script/lint

@@ -1,14 +1,24 @@
 #!/bin/sh
-# script/lint: run the linter.
+# script/lint: run the linter, in docker only. Builds Dockerfile.lint,
+# where golangci-lint runs as a build step.
+#
+# A cached build lints nothing, so --no-cache-filter rebuilds the lint
+# stage on every run, an unchanged tree included. It ignores a stage name
+# that does not exist, so --target names the same stage: a rename then
+# fails the build instead of serving the lint from cache. cacheonly keeps
+# no image; only the build's success matters.
 set -eu
 
 ROOT="$(cd "$(dirname "$0")/.." && pwd -P)"
 
 main() {
     cd "$ROOT"
-    # CGO is required (Makefile exports this too)
-    export CGO_ENABLED=1
-    golangci-lint run --timeout 5m
+    docker build \
+        --progress=plain \
+        --target lint \
+        --no-cache-filter=lint \
+        --output=type=cacheonly \
+        -f Dockerfile.lint .
 }
 
 main "$@"