TODO.md

@@ -25,6 +25,21 @@ Bring the repo into policy compliance in one commit:
 
 # Completed Steps
 
+- 2026-10-04: What a command killed part-way left under a `.tmp-` name
+  (https://git.eeqj.de/sneak/secret/issues/75), the temporary directories
+  of `secret.TempDirFor` and the temporary files of
+  `secret.WriteFileAtomic`, encrypted keys included, is deleted by the next
+  command that takes the state directory lock. Before, it stayed until
+  deleted by hand. A command writes `finished` into the lock file just
+  before it releases the lock; the next one to take the lock searches only
+  when it does not find that, so after a command that finished nothing is
+  searched, however many secrets and versions there are. The search looks
+  in the state directory, each vault, each secret and each version, the
+  only directories those helpers make them in. A command that only reads
+  takes no lock and deletes nothing. A failure to delete is warned about
+  and the command goes on. An unlocker directory with no metadata file was
+  already removed by `secret unlocker remove` given its directory name; a
+  test now shows it.
 - 2026-10-04: An age identity's private key goes into a locked buffer
   through `secret.IdentityToLockedBuffer` everywhere
   (https://git.eeqj.de/sneak/secret/issues/38): the vault's long-term key
@@ -235,15 +250,10 @@ Bring the repo into policy compliance in one commit:
   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:
-    - from `init` or `vault create` killed after the passphrase prompt
-      but before the unlocker is written, a vault with no unlocker,
-      which `vault create` has already made the current vault;
-    - data under a `.tmp-` name in the state directory: a secret,
-      version or unlocker 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).
+  interrupted command can still leave, from `init` or `vault create`
+  killed after the passphrase prompt but before the unlocker is
+  written, a vault with no unlocker, which `vault create` has already
+  made the current vault.
 - 2026-10-03: The checks run before changing a vault now stop with an
   error naming the path and cause when they cannot read what they
   inspect, instead of reading the failure as "nothing there": the

internal/cli/leftovers_test.go

@@ -0,0 +1,74 @@
+package cli_test
+
+import (
+	"io"
+	"testing"
+
+	"git.eeqj.de/sneak/secret/internal/cli"
+	"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/require"
+)
+
+// TestLeftoversRemovedByNextChangingCommand is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/75. It plants what a command
+// killed part-way leaves in each directory where secret.TempDirFor and
+// secret.WriteFileAtomic make temporary entries: a temporary directory
+// holding a vault, secret, unlocker or version being added or removed, and
+// a temporary file beside a file being replaced. `secret list` must leave
+// them all, and the next command that takes the state directory lock, here
+// `secret vault select` of the vault already current, must delete exactly
+// them: a vault named like a temporary directory stays. The copy has no
+// lock file yet, so that command, as after a killed one, finds no mark that
+// the last holder of the lock finished.
+func TestLeftoversRemovedByNextChangingCommand(t *testing.T) {
+	t.Parallel()
+
+	fs := newTwoVaultFs(t)
+
+	_, err := vault.CreateVault(fs, testStateDir, ".tmp-0", nil)
+	require.NoError(t, err)
+	require.NoError(t, vault.SelectVault(fs, testStateDir, "default"))
+
+	before := snapshotStateDir(t, fs)
+
+	vaultDir := testStateDir + "/vaults.d/default"
+	secretDir := vaultDir + "/secrets.d/x"
+
+	versions, err := secret.ListVersions(fs, secretDir)
+	require.NoError(t, err)
+	require.Len(t, versions, 1)
+
+	for _, dir := range []string{
+		testStateDir + "/.tmp-1/default",
+		vaultDir + "/.tmp-2/x",
+		secretDir + "/.tmp-3/" + testVersion,
+	} {
+		require.NoError(t, fs.MkdirAll(dir, secret.DirPerms))
+		require.NoError(t, afero.WriteFile(fs, dir+"/value.age",
+			[]byte("encrypted"), secret.FilePerms))
+	}
+
+	for _, file := range []string{
+		testStateDir + "/.currentvault.tmp-4",
+		vaultDir + "/.current-unlocker.tmp-5",
+		secretDir + "/.current.tmp-6",
+		secretDir + "/versions/" + versions[0] + "/.metadata.age.tmp-7",
+	} {
+		require.NoError(t, afero.WriteFile(fs, file,
+			[]byte("partial"), secret.FilePerms))
+	}
+
+	planted := snapshotStateDir(t, fs)
+	c := cli.NewCLIInstanceWithStateDir(fs, testStateDir)
+	cmd := &cobra.Command{}
+	cmd.SetOut(io.Discard)
+
+	require.NoError(t, c.ListSecrets(cmd, false, false, ""))
+	require.Equal(t, planted, snapshotStateDir(t, fs))
+
+	require.NoError(t, c.SelectVault(cmd, "default"))
+	require.Equal(t, before, snapshotStateDir(t, fs))
+}

internal/cli/lock_test.go

@@ -362,7 +362,6 @@ func requireWaitsForLock(
 
 	fs := afero.NewMemMapFs()
 	olderVersion, unlockerID := setupEveryCommand(t, fs, withUnlocker)
-	before := stateDirModTimes(t, fs)
 
 	release, err := vault.LockStateDir(fs, testStateDir)
 	require.NoError(t, err)
@@ -372,6 +371,9 @@ func requireWaitsForLock(
 	release = sync.OnceFunc(release)
 	defer release()
 
+	// Taken only now, since taking the lock writes the lock file.
+	before := stateDirModTimes(t, fs)
+
 	unlockPassphrase := memguard.NewBufferFromBytes([]byte(testPassphrase))
 	defer unlockPassphrase.Destroy()
 

internal/cli/path_traversal_test.go

@@ -90,6 +90,8 @@ func newTwoVaultFs(t *testing.T) afero.Fs {
 // snapshotStateDir maps every file under the state directory to its
 // contents, and every directory, written with a trailing "/", to "". Two
 // snapshots are equal only if nothing in it was added, removed or changed.
+// The lock file, which every command that takes the lock writes, is left
+// out.
 func snapshotStateDir(t *testing.T, fs afero.Fs) map[string]string {
 	t.Helper()
 
@@ -102,6 +104,10 @@ func snapshotStateDir(t *testing.T, fs afero.Fs) map[string]string {
 			return err
 		}
 
+		if path == testStateDir+"/lock" {
+			return nil
+		}
+
 		if info.IsDir() {
 			tree[path+"/"] = ""
 

internal/cli/unlockers_corrupt_test.go

@@ -4,9 +4,10 @@
 // by its ID. These tests give the first unlocker, which sorts before the
 // one the commands act on, metadata that is not JSON, and check that the
 // commands step past it, and that it can itself be removed by its
-// directory name, which `secret unlocker list` names in its warning. A
-// last test checks that an unlocker whose metadata file cannot be read
-// counts as the last unlocker when it is removed by its directory name.
+// directory name, which `secret unlocker list` names in its warning, as can
+// one with no metadata file. A last test checks that an unlocker whose
+// metadata file cannot be read counts as the last unlocker when it is
+// removed by its directory name.
 
 //nolint:testpackage // white-box test of unexported internals
 package cli
@@ -106,6 +107,34 @@ func TestUnlockerRemoveWithCorruptUnlocker(t *testing.T) {
 	}
 }
 
+// TestUnlockerRemoveWithoutMetadata asserts that a partial unlocker
+// directory, one with no metadata file, removed by its directory name from
+// a vault with secrets, does not count as the vault's last unlocker, since
+// it cannot unlock the vault, so the question says it is not. It is
+// removed once the user confirms.
+func TestUnlockerRemoveWithoutMetadata(t *testing.T) {
+	t.Parallel()
+
+	fs := newListTestVault(t, 2)
+	vaultDir := testVaultDir(listTestVaultName)
+	unlockersDir := filepath.Join(vaultDir, listTestUnlockersDirName)
+
+	require.NoError(t, fs.Remove(filepath.Join(
+		unlockersDir, listTestUnlockerDirOne, listTestMetadataFileName)))
+	writeTestSecret(t, fs, vaultDir)
+
+	instance, cmd := newTestInstance(fs)
+
+	found, err := instance.findUnlockerToRemove(listTestUnlockerDirOne)
+	require.NoError(t, err)
+	assert.False(t, found.last)
+	assert.Contains(t, found.question, "not the vault's last unlocker")
+
+	instance.terminal = strings.NewReader("y\n")
+	require.NoError(t, instance.UnlockersRemove(listTestUnlockerDirOne, false, cmd))
+	assertDirEntries(t, fs, unlockersDir, listTestUnlockerDirTwo)
+}
+
 // TestUnlockerRemoveWithUnreadableMetadata asserts that the only unlocker
 // of a vault with secrets, removed by its directory name when its metadata
 // file cannot be checked for or read, counts as the vault's last unlocker,

internal/secret/atomic.go

@@ -5,10 +5,15 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"strings"
 
 	"github.com/spf13/afero"
 )
 
+// tempNamePart is in the name of every temporary file WriteFileAtomic makes,
+// ".NAME.tmp-123", and every temporary directory TempDirFor makes, ".tmp-123".
+const tempNamePart = ".tmp-"
+
 // 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
@@ -17,7 +22,7 @@ import (
 // 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-*")
+		"."+filepath.Base(path)+tempNamePart+"*")
 	if err != nil {
 		return fmt.Errorf("failed to create temporary file for %s: %w", path, err)
 	}
@@ -54,7 +59,7 @@ func WriteFileAtomic(fs afero.Fs, path string, data []byte) error {
 // 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-")
+	dir, err := afero.TempDir(fs, filepath.Dir(filepath.Dir(target)), tempNamePart)
 	if err != nil {
 		return "", fmt.Errorf(
 			"failed to create temporary directory for %s: %w", target, err)
@@ -63,6 +68,40 @@ func TempDirFor(fs afero.Fs, target string) (string, error) {
 	return dir, nil
 }
 
+// RemoveLeftovers deletes from dir the temporary files of WriteFileAtomic
+// and the temporary directories of TempDirFor that a command killed
+// part-way left there: each entry whose name starts with "." and holds
+// tempNamePart. The caller must hold the state directory lock, so that no
+// running command is still using one. A dir that does not exist holds none.
+func RemoveLeftovers(fs afero.Fs, dir string) error {
+	entries, err := afero.ReadDir(fs, dir)
+	if errors.Is(err, os.ErrNotExist) {
+		return nil
+	}
+
+	if err != nil {
+		return fmt.Errorf("failed to read %s: %w", dir, err)
+	}
+
+	for _, entry := range entries {
+		name := entry.Name()
+		if !strings.HasPrefix(name, ".") || !strings.Contains(name, tempNamePart) {
+			continue
+		}
+
+		path := filepath.Join(dir, name)
+
+		err = fs.RemoveAll(path)
+		if err != nil {
+			return fmt.Errorf("failed to remove %s: %w", path, err)
+		}
+
+		Debug("Removed what an interrupted command left", "path", path)
+	}
+
+	return nil
+}
+
 // WriteDir calls write to write the files of the new directory dir into a
 // temporary directory from TempDirFor, which is then renamed to dir, so that
 // neither a failure nor a crash leaves dir half-written; on a failure the

internal/vault/lock.go

@@ -1,6 +1,7 @@
 package vault
 
 import (
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -14,6 +15,11 @@ import (
 // lockFileName is the file in the state directory that LockStateDir locks.
 const lockFileName = "lock"
 
+// finishedMark is what the lock file holds once the command that last held
+// the lock has released it. A command killed while holding it leaves the
+// file empty.
+const finishedMark = "finished\n"
+
 // 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.
 //
@@ -25,6 +31,12 @@ var memFsLock sync.Mutex
 // 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.
+// Once it holds the lock, it empties the lock file, and the function it
+// returns writes finishedMark there just before releasing the lock, so a
+// command killed while holding the lock leaves the mark missing. Finding it
+// missing, LockStateDir first deletes the temporary files and directories
+// such a command may have left, since no command still using them can be
+// running. After a command that finished, it searches nothing.
 //
 // 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
@@ -32,16 +44,97 @@ var memFsLock sync.Mutex
 // 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) {
+	var release func()
+
 	switch fs.(type) {
 	case *afero.OsFs:
-		return flockStateDir(stateDir)
+		var err error
+
+		release, err = flockStateDir(stateDir)
+		if err != nil {
+			return nil, err
+		}
 	case *afero.MemMapFs:
 		memFsLock.Lock()
 
-		return memFsLock.Unlock, nil
+		release = memFsLock.Unlock
 	default:
 		return nil, fmt.Errorf("%w %T", ErrNoLockForFilesystem, fs)
 	}
+
+	// The lock file is written in place, never replaced: a command waiting
+	// for flock on the old file would then take a lock nobody else checks.
+	lockPath := filepath.Join(stateDir, lockFileName)
+
+	mark, err := afero.ReadFile(fs, lockPath)
+	if err != nil || string(mark) != finishedMark {
+		removeLeftovers(fs, stateDir)
+	}
+
+	err = afero.WriteFile(fs, lockPath, nil, secret.FilePerms)
+	if err != nil {
+		release()
+
+		return nil, fmt.Errorf("failed to empty lock file %s: %w", lockPath, err)
+	}
+
+	return func() {
+		// If this fails, the next command searches when it need not.
+		_ = afero.WriteFile(fs, lockPath, []byte(finishedMark), secret.FilePerms)
+
+		release()
+	}, nil
+}
+
+// removeLeftovers deletes the temporary files and directories that commands
+// killed part-way left in each directory where secret.WriteFileAtomic and
+// secret.TempDirFor make them: the state directory, each vault, each secret
+// and each version. Unlocker directories are written whole by
+// secret.WriteDir and never changed after, so they hold none. A failure is
+// only warned about, and the command goes on.
+func removeLeftovers(fs afero.Fs, stateDir string) {
+	dirs := []string{stateDir}
+
+	for _, vaultDir := range subdirs(fs, filepath.Join(stateDir, "vaults.d")) {
+		dirs = append(dirs, vaultDir)
+
+		for _, secretDir := range subdirs(fs, filepath.Join(vaultDir, "secrets.d")) {
+			dirs = append(dirs, secretDir)
+			dirs = append(dirs, subdirs(fs, filepath.Join(secretDir, "versions"))...)
+		}
+	}
+
+	for _, dir := range dirs {
+		err := secret.RemoveLeftovers(fs, dir)
+		if err != nil {
+			secret.Warn("Failed to remove what an interrupted command left",
+				"error", err)
+		}
+	}
+}
+
+// subdirs returns the directories in dir: none if dir does not exist, and
+// none, with a warning, if it cannot be read.
+func subdirs(fs afero.Fs, dir string) []string {
+	entries, err := afero.ReadDir(fs, dir)
+	if err != nil {
+		if !errors.Is(err, os.ErrNotExist) {
+			secret.Warn("Failed to look for what an interrupted command left",
+				"directory", dir, "error", err)
+		}
+
+		return nil
+	}
+
+	var dirs []string
+
+	for _, entry := range entries {
+		if entry.IsDir() {
+			dirs = append(dirs, filepath.Join(dir, entry.Name()))
+		}
+	}
+
+	return dirs
 }
 
 // flockStateDir takes flock(2) on the lock file in stateDir, creating the

internal/vault/lock_test.go

@@ -1,9 +1,11 @@
 package vault_test
 
 import (
+	"path/filepath"
 	"testing"
 	"time"
 
+	"git.eeqj.de/sneak/secret/internal/secret"
 	"git.eeqj.de/sneak/secret/internal/vault"
 	"github.com/spf13/afero"
 	"github.com/stretchr/testify/assert"
@@ -121,6 +123,51 @@ func TestLockStateDirFreeAfterPanic(t *testing.T) {
 	}
 }
 
+// TestLockStateDirRemovesLeftoversOnlyAfterKill checks that taking the lock
+// deletes a temporary directory a killed command left only when the last
+// holder of the lock did not release it. A holder killed while it holds the
+// lock leaves the lock file as it is at that moment.
+func TestLockStateDirRemovesLeftoversOnlyAfterKill(t *testing.T) {
+	t.Parallel()
+
+	for _, lfs := range lockFilesystems(t) {
+		t.Run(lfs.name, func(t *testing.T) {
+			t.Parallel()
+
+			lockFile := filepath.Join(lfs.stateDir, "lock")
+			leftover := filepath.Join(lfs.stateDir, ".tmp-1")
+
+			release, err := vault.LockStateDir(lfs.fs, lfs.stateDir)
+			require.NoError(t, err)
+
+			whileHeld, err := afero.ReadFile(lfs.fs, lockFile)
+			require.NoError(t, err)
+			release()
+
+			require.NoError(t, lfs.fs.MkdirAll(leftover, secret.DirPerms))
+
+			release, err = vault.LockStateDir(lfs.fs, lfs.stateDir)
+			require.NoError(t, err)
+			release()
+
+			exists, err := afero.DirExists(lfs.fs, leftover)
+			require.NoError(t, err)
+			assert.True(t, exists, "searched after a holder that finished")
+
+			require.NoError(t, afero.WriteFile(lfs.fs, lockFile, whileHeld,
+				secret.FilePerms))
+
+			release, err = vault.LockStateDir(lfs.fs, lfs.stateDir)
+			require.NoError(t, err)
+			release()
+
+			exists, err = afero.DirExists(lfs.fs, leftover)
+			require.NoError(t, err)
+			assert.False(t, exists, "not searched after a holder that was killed")
+		})
+	}
+}
+
 // TestLockStateDirRefusesOtherFilesystems checks that a filesystem with no
 // lock implementation is refused instead of being used unlocked.
 func TestLockStateDirRefusesOtherFilesystems(t *testing.T) {