Lock the state directory and write vault files atomically (closes #34)

Each command that changes the state directory holds one lock: flock(2)
on `lock` in the state directory, dropped by the kernel if the process
dies, or a process-wide mutex on the in-memory test filesystem. It
covers the state directory, not each vault, because `currentvault`,
`vault create` and cross-vault moves span vaults, and a lock file in a
vault would be deleted by `vault remove` under a waiting command.

Files go through `secret.WriteFileAtomic`; versions, new secrets and
cross-vault copies are built in a temporary directory and renamed into
place; removals rename out of the way first. An unlocker added under
an existing unlocker's directory name is still rewritten file by file:
#71.

Model: opus-5-5

TODO.md

@@ -33,7 +33,13 @@ Bring the repo into policy compliance in one commit:
   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 an interrupted command leaves nothing
-  half-written.
+  half-written, with one exception: an unlocker added under the
+  directory name of an existing one is rewritten in place, file by
+  file, and a crash part-way leaves it unable to open the vault. 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).
 - 2026-10-02: A plain `docker build .` builds again: the size tests
   skip a case that needs more locked memory than the process can
   lock, and run every case under `script/cibuild`. The image stamps the

internal/cli/lock_test.go

@@ -2,9 +2,11 @@
 package cli
 
 import (
+	"io"
 	"path/filepath"
 	"strconv"
 	"strings"
+	"sync"
 	"testing"
 	"time"
 
@@ -120,6 +122,78 @@ func TestConcurrentAddsKeepEveryVersion(t *testing.T) {
 	}
 }
 
+// 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, "/plaintext", []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", "/plaintext", "")
+		// Ends add's input, as the end of the pipe does
+		_ = pipeWriter.CloseWithError(err)
+
+		results <- err
+	}()
+
+	timeout := time.After(10 * time.Second)
+
+	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) {

internal/secret/atomic.go

@@ -49,9 +49,10 @@ func WriteFileAtomic(fs afero.Fs, path string, data []byte) error {
 // 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)),
-		"."+filepath.Base(target)+".tmp-")
+	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)

internal/secret/atomic_test.go

@@ -4,10 +4,12 @@ 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"
@@ -16,6 +18,19 @@ import (
 
 var errInjected = errors.New("injected failure")
 
+// The kinds of change hookFs passes to before.
+const (
+	opCreate = "create"
+	opOpen   = "open"
+	opMkdir  = "mkdir"
+	opRemove = "remove"
+	opRename = "rename"
+)
+
+// currentFile is the file in a secret's directory that names its current
+// version.
+const currentFile = "current"
+
 // hookFs passes every call through to Fs, but first calls before for each
 // call that changes the filesystem, with the path it changes (the new path,
 // for Rename). A test uses before to inspect the tree at every point where
@@ -29,7 +44,7 @@ type hookFs struct {
 
 //nolint:ireturn // implements afero.Fs
 func (h hookFs) Create(name string) (afero.File, error) {
-	err := h.before("create", name)
+	err := h.before(opCreate, name)
 	if err != nil {
 		return nil, err
 	}
@@ -41,7 +56,7 @@ func (h hookFs) Create(name string) (afero.File, error) {
 func (h hookFs) OpenFile(
 	name string, flag int, perm os.FileMode,
 ) (afero.File, error) {
-	err := h.before("open", name)
+	err := h.before(opOpen, name)
 	if err != nil {
 		return nil, err
 	}
@@ -50,7 +65,7 @@ func (h hookFs) OpenFile(
 }
 
 func (h hookFs) Mkdir(name string, perm os.FileMode) error {
-	err := h.before("mkdir", name)
+	err := h.before(opMkdir, name)
 	if err != nil {
 		return err
 	}
@@ -59,7 +74,7 @@ func (h hookFs) Mkdir(name string, perm os.FileMode) error {
 }
 
 func (h hookFs) MkdirAll(path string, perm os.FileMode) error {
-	err := h.before("mkdir", path)
+	err := h.before(opMkdir, path)
 	if err != nil {
 		return err
 	}
@@ -68,7 +83,7 @@ func (h hookFs) MkdirAll(path string, perm os.FileMode) error {
 }
 
 func (h hookFs) Remove(name string) error {
-	err := h.before("remove", name)
+	err := h.before(opRemove, name)
 	if err != nil {
 		return err
 	}
@@ -77,7 +92,7 @@ func (h hookFs) Remove(name string) error {
 }
 
 func (h hookFs) RemoveAll(path string) error {
-	err := h.before("remove", path)
+	err := h.before(opRemove, path)
 	if err != nil {
 		return err
 	}
@@ -86,7 +101,7 @@ func (h hookFs) RemoveAll(path string) error {
 }
 
 func (h hookFs) Rename(oldname, newname string) error {
-	err := h.before("rename", newname)
+	err := h.before(opRename, newname)
 	if err != nil {
 		return err
 	}
@@ -152,6 +167,24 @@ func writeLongTermKey(
 	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()
 
@@ -160,7 +193,7 @@ func TestWriteFileAtomicReplacesFile(t *testing.T) {
 			t.Parallel()
 
 			fs, dir := tfs.open(t)
-			path := filepath.Join(dir, "current")
+			path := filepath.Join(dir, currentFile)
 
 			require.NoError(t, secret.WriteFileAtomic(fs, path, []byte("old")))
 			require.NoError(t, secret.WriteFileAtomic(fs, path, []byte("new")))
@@ -174,7 +207,7 @@ func TestWriteFileAtomicReplacesFile(t *testing.T) {
 			assert.Equal(t, secret.FilePerms, info.Mode().Perm())
 
 			// No temporary file is left next to it
-			assert.Equal(t, []string{"current"}, dirNames(t, fs, dir))
+			assert.Equal(t, []string{currentFile}, dirNames(t, fs, dir))
 		})
 	}
 }
@@ -187,11 +220,11 @@ func TestWriteFileAtomicFailureKeepsOldFile(t *testing.T) {
 			t.Parallel()
 
 			base, dir := tfs.open(t)
-			path := filepath.Join(dir, "current")
+			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 == "rename" {
+				if op == opRename {
 					return errInjected
 				}
 
@@ -206,11 +239,14 @@ func TestWriteFileAtomicFailureKeepsOldFile(t *testing.T) {
 			assert.Equal(t, "old", string(data))
 
 			// The temporary file is removed again
-			assert.Equal(t, []string{"current"}, dirNames(t, base, dir))
+			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()
 
@@ -218,20 +254,132 @@ func TestRemoveDirAtomic(t *testing.T) {
 		t.Run(tfs.name, func(t *testing.T) {
 			t.Parallel()
 
-			fs, dir := tfs.open(t)
+			base, dir := tfs.open(t)
 			listed := filepath.Join(dir, "secrets.d")
 			target := filepath.Join(listed, "doomed")
 
-			require.NoError(t, fs.MkdirAll(filepath.Join(target, "versions"), 0o700))
-			require.NoError(t, secret.WriteFileAtomic(fs,
-				filepath.Join(target, "current"), []byte("20231216.001")))
+			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, fs, listed))
-			assert.Equal(t, []string{"secrets.d"}, dirNames(t, fs, dir))
+			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")))
 		})
 	}
 }
@@ -301,7 +449,7 @@ func TestVersionSaveFailureLeavesNothing(t *testing.T) {
 			writeLongTermKey(t, base, stateDir)
 
 			fs := hookFs{Fs: base, before: func(op, path string) error {
-				if op == "rename" && filepath.Base(path) == "priv.age" {
+				if op == opRename && filepath.Base(path) == "priv.age" {
 					return errInjected
 				}
 
@@ -339,7 +487,7 @@ func TestSetCurrentVersionNeverMissing(t *testing.T) {
 			require.NoError(t, base.MkdirAll(secretDir, 0o700))
 			require.NoError(t, secret.SetCurrentVersion(base, secretDir, "20231216.001"))
 
-			currentPath := filepath.Join(secretDir, "current")
+			currentPath := filepath.Join(secretDir, currentFile)
 			fs := hookFs{Fs: base, before: func(string, string) error {
 				exists, err := afero.Exists(base, currentPath)
 				require.NoError(t, err)