From 57ff283642aa73e2e408bd7e76a19c81f285ac77 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 22 Oct 2026 12:31:45 -0600 Subject: [PATCH 1/3] test(secretstore): cover invocation unlock caching Pin reuse of resolved passphrase material, retry after failed prompt, empty Material before resolution, and single prompt under concurrent callers. --- cmd/internal/secretstore/options_test.go | 130 +++++++++++++++++++++++ 1 file changed, 130 insertions(+) create mode 100644 cmd/internal/secretstore/options_test.go diff --git a/cmd/internal/secretstore/options_test.go b/cmd/internal/secretstore/options_test.go new file mode 100644 index 0000000000..d22d357bed --- /dev/null +++ b/cmd/internal/secretstore/options_test.go @@ -0,0 +1,130 @@ +package secretstore + +import ( + "context" + "errors" + "sync" + "sync/atomic" + "testing" + + "github.com/devsy-org/devsy/pkg/secrets" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// newTestResolver mirrors the base resolver Options() builds, with the process +// environment and remembered-passphrase store pinned so the host cannot leak in. +func newTestResolver( + prompt func(context.Context, secrets.UnlockRequest) (string, error), +) *invocationResolver { + return &invocationResolver{ + base: secrets.DefaultUnlockResolver{ + LookupEnv: func(string) string { return "" }, + ReadRemembered: func() (string, error) { return "", nil }, + Prompt: prompt, + }, + } +} + +func TestInvocationResolverReusesResolvedMaterial(t *testing.T) { + var prompts atomic.Int32 + resolver := newTestResolver(func(context.Context, secrets.UnlockRequest) (string, error) { + prompts.Add(1) + return "typed-once", nil + }) + + first, err := resolver.ResolvePassphrase( + context.Background(), + secrets.UnlockRequest{AllowPrompt: true, Purpose: "unlock"}, + ) + require.NoError(t, err) + // A later caller that may not prompt must still receive the cached material. + second, err := resolver.ResolvePassphrase( + context.Background(), + secrets.UnlockRequest{Purpose: "unlock"}, + ) + require.NoError(t, err) + + assert.Equal( + t, + secrets.UnlockMaterial{Passphrase: "typed-once", Source: secrets.PassphrasePrompt}, + first, + ) + assert.Equal(t, first, second) + assert.Equal(t, int32(1), prompts.Load()) +} + +func TestInvocationResolverRetriesAfterFailedPrompt(t *testing.T) { + promptErr := errors.New("prompt cancelled") + prompts := 0 + resolver := newTestResolver(func(context.Context, secrets.UnlockRequest) (string, error) { + prompts++ + if prompts == 1 { + return "", promptErr + } + return "retry-value", nil + }) + request := secrets.UnlockRequest{AllowPrompt: true, Purpose: "unlock"} + + _, err := resolver.ResolvePassphrase(context.Background(), request) + require.ErrorIs(t, err, promptErr) + _, ok := resolver.Material() + assert.False(t, ok, "failed resolution must not be reported as material") + + material, err := resolver.ResolvePassphrase(context.Background(), request) + require.NoError(t, err) + assert.Equal(t, "retry-value", material.Passphrase) + assert.Equal(t, 2, prompts) +} + +func TestInvocationResolverMaterialIsEmptyUntilResolved(t *testing.T) { + prompts := 0 + resolver := newTestResolver(func(context.Context, secrets.UnlockRequest) (string, error) { + prompts++ + return "typed", nil + }) + + material, ok := resolver.Material() + assert.False(t, ok) + assert.Empty(t, material.Passphrase) + assert.Equal(t, 0, prompts, "reading material must never prompt") + + _, err := resolver.ResolvePassphrase( + context.Background(), + secrets.UnlockRequest{AllowPrompt: true, Purpose: "unlock"}, + ) + require.NoError(t, err) + + material, ok = resolver.Material() + assert.True(t, ok) + assert.Equal(t, "typed", material.Passphrase) + assert.Equal(t, secrets.PassphrasePrompt, material.Source) +} + +func TestInvocationResolverPromptsOnceForConcurrentCallers(t *testing.T) { + var prompts atomic.Int32 + resolver := newTestResolver(func(context.Context, secrets.UnlockRequest) (string, error) { + prompts.Add(1) + return "shared", nil + }) + request := secrets.UnlockRequest{AllowPrompt: true, Purpose: "unlock"} + + const callers = 16 + var wg sync.WaitGroup + results := make([]secrets.UnlockMaterial, callers) + errs := make([]error, callers) + for i := range callers { + wg.Add(1) + go func() { + defer wg.Done() + results[i], errs[i] = resolver.ResolvePassphrase(context.Background(), request) + }() + } + wg.Wait() + + for i := range callers { + require.NoError(t, errs[i]) + assert.Equal(t, "shared", results[i].Passphrase) + } + assert.Equal(t, int32(1), prompts.Load()) +} From 4b4048ec57ede15268e14f0b8fd9478dba90adcd Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 22 Oct 2026 12:43:29 -0600 Subject: [PATCH 2/3] test(secretstore): drop redundant comment and satisfy lint Remove the inline comment that restated the reuse test. Replace the repeated unlock purpose with a constant (goconst) and use WaitGroup.Go in the concurrency test (modernize). --- cmd/internal/secretstore/options_test.go | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/cmd/internal/secretstore/options_test.go b/cmd/internal/secretstore/options_test.go index d22d357bed..a4a417d108 100644 --- a/cmd/internal/secretstore/options_test.go +++ b/cmd/internal/secretstore/options_test.go @@ -12,6 +12,8 @@ import ( "github.com/stretchr/testify/require" ) +const testPurpose = "unlock" + // newTestResolver mirrors the base resolver Options() builds, with the process // environment and remembered-passphrase store pinned so the host cannot leak in. func newTestResolver( @@ -35,13 +37,12 @@ func TestInvocationResolverReusesResolvedMaterial(t *testing.T) { first, err := resolver.ResolvePassphrase( context.Background(), - secrets.UnlockRequest{AllowPrompt: true, Purpose: "unlock"}, + secrets.UnlockRequest{AllowPrompt: true, Purpose: testPurpose}, ) require.NoError(t, err) - // A later caller that may not prompt must still receive the cached material. second, err := resolver.ResolvePassphrase( context.Background(), - secrets.UnlockRequest{Purpose: "unlock"}, + secrets.UnlockRequest{Purpose: testPurpose}, ) require.NoError(t, err) @@ -64,7 +65,7 @@ func TestInvocationResolverRetriesAfterFailedPrompt(t *testing.T) { } return "retry-value", nil }) - request := secrets.UnlockRequest{AllowPrompt: true, Purpose: "unlock"} + request := secrets.UnlockRequest{AllowPrompt: true, Purpose: testPurpose} _, err := resolver.ResolvePassphrase(context.Background(), request) require.ErrorIs(t, err, promptErr) @@ -91,7 +92,7 @@ func TestInvocationResolverMaterialIsEmptyUntilResolved(t *testing.T) { _, err := resolver.ResolvePassphrase( context.Background(), - secrets.UnlockRequest{AllowPrompt: true, Purpose: "unlock"}, + secrets.UnlockRequest{AllowPrompt: true, Purpose: testPurpose}, ) require.NoError(t, err) @@ -107,18 +108,16 @@ func TestInvocationResolverPromptsOnceForConcurrentCallers(t *testing.T) { prompts.Add(1) return "shared", nil }) - request := secrets.UnlockRequest{AllowPrompt: true, Purpose: "unlock"} + request := secrets.UnlockRequest{AllowPrompt: true, Purpose: testPurpose} const callers = 16 var wg sync.WaitGroup results := make([]secrets.UnlockMaterial, callers) errs := make([]error, callers) for i := range callers { - wg.Add(1) - go func() { - defer wg.Done() + wg.Go(func() { results[i], errs[i] = resolver.ResolvePassphrase(context.Background(), request) - }() + }) } wg.Wait() From bfcca591ae04f274cfc835a98e440e158adf2c12 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Fri, 23 Oct 2026 02:21:10 -0600 Subject: [PATCH 3/3] test(secretstore): prove concurrent callers contend before prompt returns --- cmd/internal/secretstore/options_test.go | 44 +++++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/cmd/internal/secretstore/options_test.go b/cmd/internal/secretstore/options_test.go index a4a417d108..0400ca4358 100644 --- a/cmd/internal/secretstore/options_test.go +++ b/cmd/internal/secretstore/options_test.go @@ -3,9 +3,12 @@ package secretstore import ( "context" "errors" + "runtime" + "strings" "sync" "sync/atomic" "testing" + "time" "github.com/devsy-org/devsy/pkg/secrets" "github.com/stretchr/testify/assert" @@ -102,15 +105,53 @@ func TestInvocationResolverMaterialIsEmptyUntilResolved(t *testing.T) { assert.Equal(t, secrets.PassphrasePrompt, material.Source) } +// parkedCallers counts goroutines blocked on the resolver mutex inside +// ResolvePassphrase. Callers cannot be observed entering the method directly, so +// their goroutine stacks are inspected instead. +func parkedCallers() int { + buf := make([]byte, 1<<20) + for { + n := runtime.Stack(buf, true) + if n < len(buf) { + buf = buf[:n] + break + } + buf = make([]byte, 2*len(buf)) + } + count := 0 + for stack := range strings.SplitSeq(string(buf), "\n\n") { + if strings.Contains(stack, "(*invocationResolver).ResolvePassphrase") && + strings.Contains(stack, "Mutex).Lock") { + count++ + } + } + return count +} + +func waitForParkedCallers(want int, timeout time.Duration) bool { + deadline := time.Now().Add(timeout) + for parkedCallers() < want { + if time.Now().After(deadline) { + return false + } + time.Sleep(time.Millisecond) + } + return true +} + func TestInvocationResolverPromptsOnceForConcurrentCallers(t *testing.T) { + const callers = 16 var prompts atomic.Int32 + var contended atomic.Bool resolver := newTestResolver(func(context.Context, secrets.UnlockRequest) (string, error) { prompts.Add(1) + // Hold the prompt until every other caller is parked on the mutex, so the + // first resolution cannot finish before all callers have contended. + contended.Store(waitForParkedCallers(callers-1, 5*time.Second)) return "shared", nil }) request := secrets.UnlockRequest{AllowPrompt: true, Purpose: testPurpose} - const callers = 16 var wg sync.WaitGroup results := make([]secrets.UnlockMaterial, callers) errs := make([]error, callers) @@ -121,6 +162,7 @@ func TestInvocationResolverPromptsOnceForConcurrentCallers(t *testing.T) { } wg.Wait() + require.True(t, contended.Load(), "callers did not all contend on the resolver") for i := range callers { require.NoError(t, errs[i]) assert.Equal(t, "shared", results[i].Passphrase)