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.
Serializing changes across vaults costs a command-line tool nothing.

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.

Model: opus-5-5

internal/cli/crypto.go

@@ -72,10 +72,18 @@ func newDecryptCmd() *cobra.Command {
 
 // 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. It holds the
+// state directory lock itself, so that Encrypt streams its input and output
+// unlocked and cannot block a secret command at the other end of a pipe.
 func (cli *Instance) resolveEncryptionKey(
 	vlt *vault.Vault, secretName string,
 ) (*memguard.LockedBuffer, error) {
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return nil, err
+	}
+	defer release()
+
 	// Check if secret exists
 	secretObj := secret.NewSecret(vlt, secretName)
 

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,150 @@
+//nolint:testpackage // sets the unexported fields of Instance
+package cli
+
+import (
+	"path/filepath"
+	"strconv"
+	"strings"
+	"testing"
+	"time"
+
+	"git.eeqj.de/sneak/secret/internal/secret"
+	"git.eeqj.de/sneak/secret/internal/vault"
+	"github.com/spf13/afero"
+	"github.com/spf13/cobra"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// 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")
+		})
+	}
+}
+
+// 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)
+
+	taken := make(chan func(), 1)
+
+	go func() {
+		release, err := vault.LockStateDir(fs, testStateDir)
+		if assert.NoError(t, err) {
+			taken <- release
+		}
+	}()
+
+	select {
+	case release := <-taken:
+		release()
+	case <-time.After(10 * time.Second):
+		t.Fatal("the failed command left the state directory locked")
+	}
+}

internal/cli/secrets.go

@@ -377,6 +377,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)
@@ -635,6 +644,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 {
@@ -649,6 +666,12 @@ func (cli *Instance) ImportSecret(
 
 // RemoveSecret removes a secret from the vault
 func (cli *Instance) RemoveSecret(cmd *cobra.Command, secretName string, _ bool) error {
+	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 {
@@ -683,7 +706,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)
 	}
@@ -698,6 +721,12 @@ func (cli *Instance) RemoveSecret(cmd *cobra.Command, secretName string, _ bool)
 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()
+
 	// Parse source and destination
 	srcVaultName, srcSecretName, srcQualified := ParseVaultSecretRef(source)
 	destVaultName, destSecretName, destQualified := ParseVaultSecretRef(dest)
@@ -792,7 +821,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)
 		}
@@ -872,7 +901,7 @@ 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)

internal/cli/unlockers.go

@@ -534,6 +534,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 +720,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 +775,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/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

@@ -239,6 +239,12 @@ func formatVersionTime(t *time.Time) string {
 func (cli *Instance) PromoteVersion(
 	cmd *cobra.Command, secretName string, version 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 {
@@ -282,6 +288,12 @@ func (cli *Instance) PromoteVersion(
 func (cli *Instance) RemoveVersion(
 	cmd *cobra.Command, secretName string, version 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 {
@@ -333,7 +345,7 @@ func (cli *Instance) RemoveVersion(
 	}
 
 	// Remove the version directory
-	err = cli.fs.RemoveAll(versionDir)
+	err = secret.RemoveDirAtomic(cli.fs, versionDir)
 	if err != nil {
 		return fmt.Errorf("failed to remove version: %w", err)
 	}