feat(pass): cancel run if the engine becomes unavailable mid-resolution

plugins/pass/commands/run.go

@@ -37,11 +37,10 @@ import (
 
 const sePrefix = "se://"
 
-// ExitCodeError is returned from RunCommand when the executed child process
-// terminated with a non-zero status. It carries the exit code the wrapper
-// should exit with. Returning this instead of calling os.Exit directly lets
-// the surrounding OTel span wrapper finish recording metrics and span data
-// before the process exits.
+const defaultPreflightPingTimeout = 3 * time.Second
+
+// ExitCodeError is returned when the child process exits non-zero, letting the
+// OTel span wrapper finish before the process exits.
 type ExitCodeError struct {
 	Code int
 }
@@ -57,21 +56,35 @@ var runExample string
 var runLong string
 
 type runOpts struct {
-	envFiles []string
-	timeout  *time.Duration
+	envFiles        []string
+	timeout         *time.Duration
+	responseTimeout *time.Duration
+	socketPath      string
 }
 
 type RunOption func(*runOpts)
 
-// WithTimeout sets the request timeout forwarded to the secrets-engine
-// client. A timeout of 0 disables the request timeout; when the option is
-// not provided, the client's default applies.
+// WithTimeout sets the client request timeout; 0 disables it.
 func WithTimeout(timeout time.Duration) RunOption {
 	return func(o *runOpts) {
 		o.timeout = &timeout
 	}
 }
 
+// WithResponseTimeout sets the client response header timeout; 0 disables it.
+func WithResponseTimeout(responseTimeout time.Duration) RunOption {
+	return func(o *runOpts) {
+		o.responseTimeout = &responseTimeout
+	}
+}
+
+// WithSocketPath overrides the engine socket path; empty means the default.
+func WithSocketPath(socketPath string) RunOption {
+	return func(o *runOpts) {
+		o.socketPath = socketPath
+	}
+}
+
 func RunCommand(options ...RunOption) *cobra.Command {
 	opts := runOpts{}
 	for _, o := range options {
@@ -89,36 +102,33 @@ func RunCommand(options ...RunOption) *cobra.Command {
 				return err
 			}
 
-			copts := []client.Option{client.WithSocketPath(api.DefaultSocketPath())}
-			if opts.timeout != nil {
-				copts = append(copts, client.WithTimeout(*opts.timeout))
-			}
-			c, err := client.New(copts...)
+			c, err := newRunClient(opts)
 			if err != nil {
 				return err
 			}
 
+			if opts.timeout == nil || *opts.timeout == 0 {
+				if err := preflightPing(cmd.Context(), c, defaultPreflightPingTimeout); err != nil {
+					return err
+				}
+			}
+
 			env, err := resolveEnv(cmd.Context(), c, merged)
 			if err != nil {
 				return err
 			}
 
-			// No CommandContext: the signal forwarder owns the child's
-			// lifecycle. Tying the child to cmd.Context() would let cobra's
-			// ctx cancellation SIGKILL the child out from under the forwarder.
+			// No CommandContext: cobra's ctx cancellation would SIGKILL the
+			// child out from under the signal forwarder.
 			child := exec.Command(args[0], args[1:]...)
 			child.Env = env
 			child.Stdin = os.Stdin
 			child.Stdout = os.Stdout
 			child.Stderr = os.Stderr
-			// Isolate the child in its own process group so that
-			// terminal-generated signals (Ctrl-C) are delivered to us alone;
-			// the forwarder is then the sole path that reaches the child.
-			configureChildProcGroup(child)
-
-			// Install the signal handler before Start so a signal arriving in
-			// the window between fork and the forwarder goroutine cannot kill
-			// the parent and orphan the child.
+			configureChildProcGroup(child) // own process group; Ctrl-C goes to us only
+
+			// Install before Start to avoid orphaning the child if a signal
+			// arrives between fork and the forwarder goroutine.
 			sigCh := make(chan os.Signal, 1)
 			signal.Notify(sigCh, forwardableSignals()...)
 			defer signal.Stop(sigCh)
@@ -157,9 +167,6 @@ func RunCommand(options ...RunOption) *cobra.Command {
 	return cmd
 }
 
-// mergeEnv folds the process environment and any --env-file inputs into a
-// single deterministic KEY=VALUE slice. Precedence: process env first, then
-// each file in order; later entries override earlier ones.
 func mergeEnv(processEnv, files []string) ([]string, error) {
 	merged := make(map[string]string, len(processEnv))
 	for _, kv := range processEnv {
@@ -182,6 +189,38 @@ func mergeEnv(processEnv, files []string) ([]string, error) {
 	return out, nil
 }
 
+func newRunClient(opts runOpts) (client.Client, error) {
+	socketPath := opts.socketPath
+	if socketPath == "" {
+		socketPath = api.DefaultSocketPath()
+	}
+	copts := []client.Option{client.WithSocketPath(socketPath)}
+	if opts.timeout != nil {
+		copts = append(copts, client.WithTimeout(*opts.timeout))
+	}
+	if opts.responseTimeout != nil {
+		copts = append(copts, client.WithResponseTimeout(*opts.responseTimeout))
+	}
+	return client.New(copts...)
+}
+
+// preflightPing fails fast when the engine is unreachable, instead of letting
+// an unbounded client block resolution indefinitely.
+//
+// The Docker CLI bounds its daemon connection ping the same way
+// (docker/cli#3722, fixing the unreachable-daemon hang in docker/cli#3652).
+func preflightPing(ctx context.Context, c client.Client, timeout time.Duration) error {
+	ctx, cancel := context.WithTimeout(ctx, timeout)
+	defer cancel()
+	if _, err := c.Version(ctx); err != nil {
+		if !errors.Is(err, client.ErrSecretsEngineNotAvailable) {
+			err = fmt.Errorf("%w: %w", client.ErrSecretsEngineNotAvailable, err)
+		}
+		return fmt.Errorf("preflight ping: %w", err)
+	}
+	return nil
+}
+
 func resolveEnv(ctx context.Context, r secrets.Resolver, env []string) ([]string, error) {
 	out := make([]string, 0, len(env))
 	for _, kv := range env {
@@ -201,8 +240,7 @@ func resolveEnv(ctx context.Context, r secrets.Resolver, env []string) ([]string
 
 func resolveRef(ctx context.Context, r secrets.Resolver, key, value string) (string, error) {
 	name := strings.TrimPrefix(value, sePrefix)
-	// Validate as an ID first so wildcards in the reference are rejected
-	// instead of silently broadening the lookup.
+	// ParseID rejects wildcards before ParsePattern broadens the lookup.
 	if _, err := secrets.ParseID(name); err != nil {
 		return "", fmt.Errorf("resolving %s: %w", key, err)
 	}

plugins/pass/commands/run_test.go

@@ -16,6 +16,7 @@ package commands
 
 import (
 	"bufio"
+	"bytes"
 	"context"
 	"errors"
 	"fmt"
@@ -32,6 +33,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"github.com/docker/secrets-engine/client"
 	"github.com/docker/secrets-engine/x/secrets"
 	"github.com/docker/secrets-engine/x/testhelper"
 )
@@ -46,6 +48,10 @@ const (
 	helperActiveEnv  = "GO_PASS_RUN_HELPER_ACTIVE"
 	helperExitEnv    = "GO_PASS_RUN_HELPER_EXIT"
 	helperSleepEnv   = "GO_PASS_RUN_HELPER_SLEEP"
+	// helperSocketEnv switches the wrapper to preflight mode: RunCommand is
+	// built with WithSocketPath(value) and no request timeout, so the
+	// preflight ping must run and fail against the dead socket.
+	helperSocketEnv = "GO_PASS_RUN_HELPER_SOCKET"
 )
 
 func TestMain(m *testing.M) {
@@ -83,7 +89,13 @@ func runAsWrapper() {
 	if err != nil {
 		os.Exit(2)
 	}
-	cmd := RunCommand()
+	// A bounded timeout skips the preflight ping, so these subprocess tests
+	// exercise child-process mechanics without needing a running engine.
+	ropts := []RunOption{WithTimeout(time.Second)}
+	if socket := os.Getenv(helperSocketEnv); socket != "" {
+		ropts = []RunOption{WithSocketPath(socket)}
+	}
+	cmd := RunCommand(ropts...)
 	cmd.SetArgs([]string{exe})
 	cmd.SetContext(context.Background())
 	cmd.SilenceUsage = true
@@ -94,6 +106,7 @@ func runAsWrapper() {
 		os.Exit(exitErr.Code)
 	}
 	if err != nil {
+		_, _ = fmt.Fprintln(os.Stderr, err)
 		os.Exit(2)
 	}
 	os.Exit(0)
@@ -270,6 +283,23 @@ func TestRunCommand(t *testing.T) {
 		assert.Equal(t, 42, exitErr.ExitCode())
 	})
 
+	t.Run("preflight ping fails fast on a dead socket", func(t *testing.T) {
+		sub := exec.CommandContext(t.Context(), exe)
+		sub.Env = append(os.Environ(),
+			helperWrapperEnv+"=1",
+			helperActiveEnv+"=1",
+			helperExitEnv+"=0",
+			helperSocketEnv+"="+filepath.Join(t.TempDir(), "dead.sock"),
+		)
+		var stderr bytes.Buffer
+		sub.Stderr = &stderr
+		err := sub.Run()
+		var exitErr *exec.ExitError
+		require.True(t, errors.As(err, &exitErr), "expected ExitError, got %v", err)
+		assert.Equal(t, 2, exitErr.ExitCode())
+		assert.Contains(t, stderr.String(), "preflight ping")
+	})
+
 	t.Run("forwards SIGINT and exits 130", func(t *testing.T) {
 		if runtime.GOOS == "windows" {
 			t.Skip("SIGINT cross-process semantics differ on Windows")
@@ -319,6 +349,64 @@ func waitForReady(t *testing.T, r io.Reader) {
 	go func() { _, _ = io.Copy(io.Discard, r) }()
 }
 
+// pingClient adapts a Version func to client.Client. The embedded
+// MockResolver supplies GetSecrets, so no hand-rolled resolver mock can drift
+// from the shared one.
+type pingClient struct {
+	testhelper.MockResolver
+	ping func(context.Context) (client.DaemonVersion, error)
+}
+
+func (p pingClient) Version(ctx context.Context) (client.DaemonVersion, error) {
+	return p.ping(ctx)
+}
+
+func TestPreflightPing(t *testing.T) {
+	t.Parallel()
+
+	t.Run("passes when the engine responds", func(t *testing.T) {
+		t.Parallel()
+		c := pingClient{ping: func(_ context.Context) (client.DaemonVersion, error) {
+			return client.DaemonVersion{}, nil
+		}}
+		require.NoError(t, preflightPing(t.Context(), c, time.Second))
+	})
+
+	t.Run("fails when the engine is unreachable", func(t *testing.T) {
+		t.Parallel()
+		engineErr := errors.New("connection refused")
+		c := pingClient{ping: func(_ context.Context) (client.DaemonVersion, error) {
+			return client.DaemonVersion{}, engineErr
+		}}
+		err := preflightPing(t.Context(), c, time.Second)
+		require.Error(t, err)
+		assert.ErrorContains(t, err, "preflight ping")
+		assert.ErrorIs(t, err, engineErr)
+		assert.ErrorIs(t, err, client.ErrSecretsEngineNotAvailable)
+	})
+
+	t.Run("gives up after the timeout when the engine hangs", func(t *testing.T) {
+		t.Parallel()
+		c := pingClient{ping: func(ctx context.Context) (client.DaemonVersion, error) {
+			<-ctx.Done()
+			return client.DaemonVersion{}, ctx.Err()
+		}}
+		// Watchdog parent: if preflightPing loses its own deadline, ping
+		// unblocks here and the elapsed assertion fails fast, instead of the
+		// package hanging until the go test panic. A parent deadline alone is
+		// not enough — the regressed path would still surface
+		// DeadlineExceeded, just later, and pass spuriously.
+		watchdogCtx, cancel := context.WithTimeout(t.Context(), 5*time.Second)
+		defer cancel()
+		start := time.Now()
+		err := preflightPing(watchdogCtx, c, 50*time.Millisecond)
+		require.Error(t, err)
+		require.Less(t, time.Since(start), 2*time.Second)
+		assert.ErrorIs(t, err, context.DeadlineExceeded)
+		assert.ErrorIs(t, err, client.ErrSecretsEngineNotAvailable)
+	})
+}
+
 // testWriter forwards cobra output to t.Log so it does not leak onto stderr.
 type testWriter struct{ t *testing.T }