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

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,