Delete .tmp- leftovers when the next command takes the lock (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 deletes them once it holds the lock, looking
only in the directories those helpers make them in (the state
directory, each vault, each secret, each version) and only at names
that start with "." and hold ".tmp-". A vault may be named ".tmp-1", so
vaults.d and the other listed directories are not searched. Commands
that only read take no lock and delete nothing. A test shows that
`unlocker remove` removes an unlocker directory with no metadata file.

Model: opus-5-5

internal/cli/leftovers_test.go

@@ -0,0 +1,72 @@
+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.
+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/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 is
-// removed by its directory name only as the last unlocker is.
+// 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 is removed by its directory name only as the
+// last unlocker is.
 
 //nolint:testpackage // white-box test of unexported internals
 package cli
@@ -113,6 +114,27 @@ func TestUnlockerRemoveWithCorruptUnlocker(t *testing.T) {
 	}
 }
 
+// TestUnlockerRemoveWithoutMetadata asserts that a partial unlocker
+// directory, one with no metadata file, is removed by its directory name
+// without --force from a vault with secrets: it cannot unlock the vault, so
+// removing it never removes the last unlocker.
+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)
+
+	require.NoError(t, instance.UnlockersRemove(listTestUnlockerDirOne, false, cmd))
+	assertDirEntries(t, fs, unlockersDir, listTestUnlockerDirTwo)
+}
+
 // TestUnlockerRemoveWithUnreadableMetadata asserts that removing the only
 // unlocker of a vault with secrets by its directory name, when its
 // metadata file cannot be checked for or read, is refused without --force:

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"
@@ -25,6 +26,9 @@ 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 deletes the temporary files and directories
+// that commands killed part-way left behind, since no command still using
+// them can be running.
 //
 // 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 +36,78 @@ 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)
 	}
+
+	removeLeftovers(fs, stateDir)
+
+	return 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; the command goes on, and the next one tries again.
+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