Delete .tmp- leftovers of a killed command when the lock is next taken (closes #75)

A command killed part-way could leave a temporary file or directory of
secret.WriteFileAtomic or secret.TempDirFor, encrypted keys included,
for good. LockStateDir now empties the lock file once it holds the lock
and writes "finished" there just before releasing it. A holder that
does not find that deletes such leftovers from the state directory,
each vault, each secret and each version, the only places those helpers
make them, matching names that start with "." and hold ".tmp-". After a
command that finished nothing is searched, so the added time does not
grow with the number of secrets and versions. A test shows that
`unlocker remove` removes an unlocker directory with no metadata file.

Model: opus-5-5

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:
+  interrupted command can still leave, from `init` or `vault create`
-    - from `init` or `vault create` killed after the passphrase prompt
+  killed after the passphrase prompt but before the unlocker is
-      but before the unlocker is written, a vault with no unlocker,
+  written, a vault with no unlocker, which `vault create` has already
-      which `vault create` has already made the current vault;
+  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).
 - 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
+// directory name, which `secret unlocker list` names in its warning, as can
-// last test checks that an unlocker whose metadata file cannot be read
+// one with no metadata file. A last test checks that an unlocker whose
-// counts as the last unlocker when it is removed by its directory name.
+// 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) {