From f357981c5261efdada2b10dcef1aa3d038e50597 Mon Sep 17 00:00:00 2001 From: sneak Date: Sun, 4 Oct 2026 04:36:57 +0000 Subject: [PATCH] Speed up the internal/cli lock and two-vault tests (closes #80) The test that each changing command waits for the state directory lock slept a fixed 100 ms per command. It now polls the goroutine stacks until the command is parked in vault.LockStateDir, checks the state directory is unchanged, and releases the lock; a command that takes no lock still fails by finishing first. newTwoVaultFs creates its two vaults, each with a passphrase unlocker, once, and returns a fresh copy of them on every call, so the six path and move tests no longer each pay for two passphrase key derivations. Model: opus-5-5 --- TODO.md | 7 +++++ internal/cli/lock_test.go | 45 +++++++++++++++++++++-------- internal/cli/path_traversal_test.go | 40 ++++++++++++++++++------- 3 files changed, 69 insertions(+), 23 deletions(-) diff --git a/TODO.md b/TODO.md index d78416a..b9b5eac 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,13 @@ Bring the repo into policy compliance in one commit: # Completed Steps +- 2026-10-04: The `internal/cli` tests are back to about their time + before the state directory lock + (https://git.eeqj.de/sneak/secret/issues/80). The test that each + changing command waits for the lock releases it as soon as it sees the + command waiting there, instead of after a fixed 100 ms. The two vaults + with passphrase unlockers that the path and move tests start from are + made once and copied for each test. - 2026-10-03: `secret mv` rejects a move whose destination is the source (`mv --force x x`, `mv --force work:x work:`, or an empty destination, which defaults to the source name) before changing diff --git a/internal/cli/lock_test.go b/internal/cli/lock_test.go index 15274df..06eaac0 100644 --- a/internal/cli/lock_test.go +++ b/internal/cli/lock_test.go @@ -2,9 +2,11 @@ package cli import ( + "bytes" "io" "os" "path/filepath" + "runtime" "strconv" "strings" "sync" @@ -25,11 +27,6 @@ const ( // once the lock is free. lockWait = 10 * time.Second - // heldWait is how long a test watches a command that must wait for the - // lock. A command that takes no lock changes the state directory well - // within it. - heldWait = 100 * time.Millisecond - // testPassphrase protects the passphrase unlockers the tests create. testPassphrase = "test-passphrase" @@ -323,10 +320,28 @@ func setupEveryCommand( return versions[1], unlockerID } +// waitingForLock reports whether a goroutine is stopped in +// vault.LockStateDir, waiting for the in-memory filesystem's lock. The +// stack trace of such a goroutine starts with the reason it waits, +// "[sync.Mutex.Lock]", and names LockStateDir. +func waitingForLock() bool { + stacks := make([]byte, 1<<20) + stacks = stacks[:runtime.Stack(stacks, true)] + + for goroutine := range bytes.SplitSeq(stacks, []byte("\n\n")) { + if bytes.Contains(goroutine, []byte("[sync.Mutex.Lock")) && + bytes.Contains(goroutine, []byte("vault.LockStateDir(")) { + return true + } + } + + return false +} + // requireWaitsForLock runs a command, given what setupEveryCommand made, // while holding the state directory lock. The command must neither finish -// nor change anything while the lock is held, and must succeed once it is -// released. +// nor change anything before it waits for the lock, and must succeed once +// the lock is released. func requireWaitsForLock( t *testing.T, withUnlocker bool, @@ -355,14 +370,20 @@ func requireWaitsForLock( go func() { done <- run(cli, olderVersion, unlockerID) }() - select { - case err := <-done: - t.Fatalf("finished while the lock was held, with error %v", err) - case <-time.After(heldWait): + timeout := time.After(lockWait) + + for !waitingForLock() { + select { + case err := <-done: + t.Fatalf("finished while the lock was held, with error %v", err) + case <-timeout: + t.Fatal("never waited for the lock") + case <-time.After(time.Millisecond): + } } assert.Equal(t, before, stateDirModTimes(t, fs), - "changed the state directory while the lock was held") + "changed the state directory before waiting for the lock") release() diff --git a/internal/cli/path_traversal_test.go b/internal/cli/path_traversal_test.go index 9e242ce..75dbc76 100644 --- a/internal/cli/path_traversal_test.go +++ b/internal/cli/path_traversal_test.go @@ -6,6 +6,7 @@ import ( "os" "slices" "strings" + "sync" "testing" "git.eeqj.de/sneak/secret/internal/cli" @@ -32,9 +33,20 @@ const ( missingFile = "/no/such/file" ) +// The state directory newTwoVaultFs copies, recorded by snapshotStateDir. +// Creating a passphrase unlocker is slow by design, so the vaults are made +// once, by the first test that needs them. +// +//nolint:gochecknoglobals // shared by the tests that use newTwoVaultFs +var ( + twoVaultsOnce sync.Once + twoVaults map[string]string +) + // newTwoVaultFs returns an in-memory filesystem holding the vaults "work" // and "default", the current one. Each holds the secret "x" and a // passphrase unlocker, so both secrets.d and unlockers.d have contents. +// Every call returns a new copy of the same vaults. // //nolint:ireturn // afero.Fs is the filesystem abstraction used throughout func newTwoVaultFs(t *testing.T) afero.Fs { @@ -42,21 +54,27 @@ func newTwoVaultFs(t *testing.T) afero.Fs { t.Setenv(secret.EnvMnemonic, testMnemonic) - fs := afero.NewMemMapFs() + twoVaultsOnce.Do(func() { + fs := afero.NewMemMapFs() - for _, name := range []string{"work", "default"} { - vlt, err := vault.CreateVault(fs, testStateDir, name) - require.NoError(t, err) + for _, name := range []string{"work", "default"} { + vlt, err := vault.CreateVault(fs, testStateDir, name) + require.NoError(t, err) - err = vlt.AddSecret("x", memguard.NewBufferFromBytes([]byte("value")), false) - require.NoError(t, err) + err = vlt.AddSecret("x", memguard.NewBufferFromBytes([]byte("value")), false) + require.NoError(t, err) - _, err = vlt.CreatePassphraseUnlocker( - memguard.NewBufferFromBytes([]byte(testPassphrase))) - require.NoError(t, err) - } + _, err = vlt.CreatePassphraseUnlocker( + memguard.NewBufferFromBytes([]byte(testPassphrase))) + require.NoError(t, err) + } - return fs + twoVaults = snapshotStateDir(t, fs) + }) + + require.NotNil(t, twoVaults, "making the vaults failed in an earlier test") + + return newFsFromSnapshot(t, twoVaults) } // snapshotStateDir maps every file under the state directory to its