Merge pull request #1625 from entireio/soph/keyring-interrupt-race-followup

fix(cli): close keyring-interrupt race in Ctrl-C signal-abort

Author: toothbrush

cmd/entire/main.go

@@ -8,13 +8,13 @@ import (
 	"os/signal"
 	"runtime"
 	"strings"
-	"sync/atomic"
 	"syscall"
 	"time"
 
 	"github.com/entireio/cli/cmd/entire/cli"
 	"github.com/entireio/cli/cmd/entire/cli/api"
 	"github.com/entireio/cli/cmd/entire/cli/versioninfo"
+	"github.com/entireio/cli/internal/procsignal"
 	"github.com/spf13/cobra"
 )
 
@@ -45,7 +45,7 @@ func main() {
 		// re-raises that same signal — a SIGTERM (from a supervisor /
 		// container stop) must exit 143, not masquerade as a SIGINT 130.
 		sig := <-sigChan
-		caughtSignal.Store(sig)
+		procsignal.Store(sig)
 		if sig == os.Interrupt {
 			fmt.Fprintln(os.Stderr, "\nInterrupting… press Ctrl-C again to force quit.")
 		} else {
@@ -89,20 +89,25 @@ func main() {
 		var silent *cli.SilentError
 
 		switch {
-		case errors.Is(err, context.Canceled) && caughtSignal.Load() != nil:
-			// A signal cancelled the root context (our handler fired). Don't
-			// dump the raw transport/keyring cancellation string ("...:
-			// context canceled", "read access token: signal: interrupt") as
-			// if it were a failure — die quietly by re-raising the signal
-			// that triggered it (see dieFromSignal) so an enclosing
-			// `while ...; do entire; done` loop actually breaks on a single
-			// Ctrl-C, and a SIGTERM shutdown still exits 143.
+		case errors.Is(err, context.Canceled) && procsignal.Load() != nil:
+			// A signal cancelled the root context (our handler fired) or a
+			// keyring read was aborted by Ctrl-C. Don't dump the raw
+			// transport/keyring cancellation string ("...: context canceled",
+			// "read access token: signal: interrupt") as if it were a failure
+			// — die quietly by re-raising the signal that triggered it (see
+			// dieFromSignal) so an enclosing `while ...; do entire; done` loop
+			// actually breaks on a single Ctrl-C, and a SIGTERM shutdown still
+			// exits 143.
 			//
-			// We gate on the handler having fired rather than on the error
-			// type alone: a context.Canceled that arose without a signal
+			// We gate on a signal having been recorded rather than on the
+			// error type alone: a context.Canceled that arose without a signal
 			// (e.g. an internally-cancelled sub-context) is a genuine error
 			// and must fall through to normal reporting, not masquerade as a
 			// user abort (which would also wrongly break an enclosing loop).
+			// procsignal is the shared source of truth written both by the
+			// handler above and by the keyring interrupt path; the latter
+			// records the signal on this same goroutine before returning, so
+			// this Load never races that write.
 			cancel()
 			dieFromSignal(terminatingSignal())
 		case errors.As(err, &silent):
@@ -130,19 +135,14 @@ func main() {
 	cancel() // Cleanup on successful exit
 }
 
-// caughtSignal records the terminating signal (SIGINT or SIGTERM) the handler
-// observed, so a later cancellation-driven exit can re-raise the *same* signal
-// rather than always SIGINT. Read via terminatingSignal.
-var caughtSignal atomic.Value // stores os.Signal
-
 // terminatingSignal returns the signal that cancelled the root context,
-// defaulting to SIGINT when the cancellation came from something other than
-// our signal handler (so a stray context.Canceled still exits 130).
+// defaulting to SIGINT when the cancellation came from something other than a
+// recorded terminating signal (so a stray context.Canceled still exits 130).
+// The recorded signal lives in the shared procsignal package, written by the
+// handler goroutine (SIGINT/SIGTERM) and by the keyring interrupt path (SIGINT).
 func terminatingSignal() os.Signal {
-	if v := caughtSignal.Load(); v != nil {
-		if s, ok := v.(os.Signal); ok {
-			return s
-		}
+	if s := procsignal.Load(); s != nil {
+		return s
 	}
 	return os.Interrupt
 }

cmd/entire/main_test.go

@@ -1,18 +1,93 @@
 package main
 
 import (
+	"errors"
 	"os"
+	"os/exec"
+	"runtime"
 	"syscall"
 	"testing"
 )
 
+// dieFromSignalEnvVar, when set on a re-executed test binary, makes TestMain
+// invoke dieFromSignal with the named signal instead of running the test suite.
+// The parent process then inspects how the child died. This is how
+// TestDieFromSignal_TerminatesBySignal exercises real process death without
+// killing the test runner itself.
+const dieFromSignalEnvVar = "ENTIRE_TEST_DIE_FROM_SIGNAL"
+
+func TestMain(m *testing.M) {
+	// Child mode: exercise dieFromSignal for the named signal and let it
+	// terminate this process. dieFromSignal only returns if the re-raise
+	// couldn't be delivered, so the os.Exit fallback mirrors it.
+	if name := os.Getenv(dieFromSignalEnvVar); name != "" {
+		sig := os.Interrupt
+		if name == "TERM" {
+			sig = syscall.SIGTERM
+		}
+		dieFromSignal(sig)
+		os.Exit(exitCodeForSignal(sig))
+	}
+	os.Exit(m.Run())
+}
+
 // nonNumericSignal is an os.Signal that isn't a syscall.Signal, exercising
 // exitCodeForSignal's fallback branch.
 type nonNumericSignal struct{}
 
 func (nonNumericSignal) String() string { return "non-numeric" }
 func (nonNumericSignal) Signal()        {}
 
+// TestDieFromSignal_TerminatesBySignal is the regression guard for the headline
+// behavior: an enclosing `while true; do entire …; done` loop only breaks when
+// the process is *killed by* the signal (WIFSIGNALED), not when it exits
+// normally with code 130. A "simplification" of dieFromSignal back to a plain
+// os.Exit(130) would leave the exit code looking right while silently breaking
+// loop-escape — this test catches exactly that by re-executing the test binary
+// in child mode and asserting it died by the signal.
+func TestDieFromSignal_TerminatesBySignal(t *testing.T) {
+	t.Parallel()
+	if runtime.GOOS == "windows" {
+		t.Skip("signal-to-self / WIFSIGNALED semantics do not apply on Windows")
+	}
+
+	tests := []struct {
+		name string
+		env  string
+		want syscall.Signal
+	}{
+		{"SIGINT", "INT", syscall.SIGINT},
+		{"SIGTERM", "TERM", syscall.SIGTERM},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			// -test.run=^$ matches no test; TestMain's child branch runs before
+			// m.Run() and terminates the process, so no test actually executes.
+			cmd := exec.CommandContext(t.Context(), os.Args[0], "-test.run=^$")
+			cmd.Env = append(os.Environ(), dieFromSignalEnvVar+"="+tc.env)
+
+			err := cmd.Run()
+
+			var exitErr *exec.ExitError
+			if !errors.As(err, &exitErr) {
+				t.Fatalf("child did not exit with an error status; err=%v (expected death by signal)", err)
+			}
+			ws, ok := exitErr.Sys().(syscall.WaitStatus)
+			if !ok {
+				t.Fatalf("no syscall.WaitStatus available: %T", exitErr.Sys())
+			}
+			if !ws.Signaled() {
+				t.Fatalf("child exited normally with code %d; want death by signal %v — dieFromSignal must re-raise, not os.Exit", ws.ExitStatus(), tc.want)
+			}
+			if ws.Signal() != tc.want {
+				t.Fatalf("child killed by %v, want %v", ws.Signal(), tc.want)
+			}
+		})
+	}
+}
+
 // TestExitCodeForSignal locks the conventional 128+signum mapping the
 // Ctrl-C/SIGTERM fix relies on, so a future "simplification" back to a
 // hardcoded 130 can't silently regress SIGTERM's 143.

internal/entireclient/tokenstore/keyring_timeout.go

@@ -2,11 +2,14 @@ package tokenstore
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"os"
 	"os/signal"
 	"runtime"
 	"time"
+
+	"github.com/entireio/cli/internal/procsignal"
 )
 
 // defaultKeyringTimeout caps how long every OS keyring call may take.
@@ -57,7 +60,21 @@ func callKeyringWithTimeout(op string, fn func() (string, error)) (string, error
 	sigCh := make(chan os.Signal, 1)
 	signal.Notify(sigCh, os.Interrupt)
 	defer signal.Stop(sigCh)
-	return callKeyringWithInterrupt(op, keyringTimeout(), fn, sigCh)
+	return recordInterruptSignal(callKeyringWithInterrupt(op, keyringTimeout(), fn, sigCh))
+}
+
+// recordInterruptSignal records the shared "we were signalled" marker when the
+// keyring call was aborted by a Ctrl-C (a wrapped context.Canceled from the
+// interrupt branch below). It runs on the goroutine that unwinds to the CLI's
+// top-level signal-abort gate, so the store is ordered before that gate reads
+// procsignal — closing the race against the asynchronous signal handler that
+// also received the SIGINT. A timeout wraps context.DeadlineExceeded, not
+// Canceled, so it is left untouched.
+func recordInterruptSignal(val string, err error) (string, error) {
+	if errors.Is(err, context.Canceled) {
+		procsignal.Store(os.Interrupt)
+	}
+	return val, err
 }
 
 // callKeyringWithInterrupt is the testable core of callKeyringWithTimeout:

internal/entireclient/tokenstore/keyring_timeout_test.go

@@ -3,12 +3,19 @@ package tokenstore
 import (
 	"context"
 	"errors"
+	"fmt"
 	"os"
 	"strings"
 	"testing"
 	"time"
+
+	"github.com/entireio/cli/internal/procsignal"
 )
 
+// notReturnedSentinel is the value fn returns when the test expects the
+// interrupt/timeout branch to win the select, so this value must never surface.
+const notReturnedSentinel = "should not be returned"
+
 func TestCallKeyringWithTimeout_ReturnsValueWhenFast(t *testing.T) {
 	t.Parallel()
 
@@ -55,7 +62,7 @@ func TestCallKeyringWithTimeout_DeadlineExceeded(t *testing.T) {
 	start := time.Now()
 	_, err := callKeyringWithTimeout("get", func() (string, error) {
 		time.Sleep(5 * time.Second)
-		return "should not be returned", nil
+		return notReturnedSentinel, nil
 	})
 	elapsed := time.Since(start)
 
@@ -94,7 +101,7 @@ func TestCallKeyringWithInterrupt_AbortsOnSignal(t *testing.T) {
 	_, err := callKeyringWithInterrupt("get", 10*time.Second, func() (string, error) {
 		close(started)
 		time.Sleep(10 * time.Second) // never completes within the test
-		return "should not be returned", nil
+		return notReturnedSentinel, nil
 	}, interrupt)
 	elapsed := time.Since(start)
 
@@ -109,6 +116,66 @@ func TestCallKeyringWithInterrupt_AbortsOnSignal(t *testing.T) {
 	}
 }
 
+// recordInterruptSignal must record a shared SIGINT marker for a Ctrl-C abort
+// (wrapped context.Canceled) so the CLI's signal-abort gate recognizes it
+// without racing the async top-level handler — but must leave the marker
+// untouched for a timeout or any non-abort error. This test mutates the
+// process-global procsignal state, so it can't run in parallel.
+func TestRecordInterruptSignal(t *testing.T) {
+	t.Run("records SIGINT on interrupt abort", func(t *testing.T) {
+		procsignal.Reset()
+		t.Cleanup(procsignal.Reset)
+
+		val, err := recordInterruptSignal(callKeyringWithInterruptResult())
+		if val != "" || !errors.Is(err, context.Canceled) {
+			t.Fatalf("passthrough changed value/err: val=%q err=%v", val, err)
+		}
+		if got := procsignal.Load(); got != os.Interrupt {
+			t.Fatalf("procsignal.Load() = %v, want SIGINT", got)
+		}
+	})
+
+	t.Run("leaves marker unset on timeout", func(t *testing.T) {
+		procsignal.Reset()
+		t.Cleanup(procsignal.Reset)
+
+		timeoutErr := fmt.Errorf("get timed out: %w", context.DeadlineExceeded)
+		if _, err := recordInterruptSignal("", timeoutErr); !errors.Is(err, context.DeadlineExceeded) {
+			t.Fatalf("passthrough changed err: %v", err)
+		}
+		if got := procsignal.Load(); got != nil {
+			t.Fatalf("procsignal.Load() = %v, want nil (timeout is not a signal abort)", got)
+		}
+	})
+
+	t.Run("leaves marker unset on success", func(t *testing.T) {
+		procsignal.Reset()
+		t.Cleanup(procsignal.Reset)
+
+		if _, err := recordInterruptSignal("token", nil); err != nil {
+			t.Fatalf("passthrough changed err: %v", err)
+		}
+		if got := procsignal.Load(); got != nil {
+			t.Fatalf("procsignal.Load() = %v, want nil", got)
+		}
+	})
+}
+
+// callKeyringWithInterruptResult produces the exact (val, err) shape the
+// interrupt branch returns, so the test exercises recordInterruptSignal against
+// the real wrapped error rather than a hand-rolled one.
+func callKeyringWithInterruptResult() (string, error) {
+	interrupt := make(chan os.Signal, 1)
+	interrupt <- os.Interrupt
+	return callKeyringWithInterrupt("get", time.Second, func() (string, error) {
+		// The pre-loaded interrupt wins the select immediately; this brief
+		// sleep just keeps fn from racing it, then the goroutine exits into
+		// the buffered result channel (no leak).
+		time.Sleep(50 * time.Millisecond)
+		return notReturnedSentinel, nil
+	}, interrupt)
+}
+
 func TestKeyringTimeout_DefaultWhenUnset(t *testing.T) {
 	t.Setenv(keyringTimeoutEnvVar, "")
 

internal/procsignal/procsignal.go

@@ -0,0 +1,58 @@
+// Package procsignal records the OS signal, if any, that initiated process
+// shutdown. It gives the CLI a single source of truth for "were we signalled?"
+// shared by the two places that can observe a terminating signal:
+//
+//   - the top-level signal handler in cmd/entire, which cancels the root
+//     context on SIGINT/SIGTERM, and
+//   - the keyring interrupt path in internal/entireclient/tokenstore, which
+//     detects Ctrl-C via its own signal.Notify listener so a stuck keyring
+//     read unblocks immediately.
+//
+// Before this package the two mechanisms were uncoordinated: the keyring path
+// returned a context.Canceled error while the top-level handler stored the
+// caught signal asynchronously on a different goroutine, so the CLI's
+// signal-abort gate could read the store before it was set and misreport a
+// user abort as a failure (and fail to break an enclosing shell loop).
+// Recording the signal here — on the same goroutine that unwinds to the gate —
+// removes that race.
+//
+// Tech debt: this shared global exists only because auth-go's Store interface
+// (LoadTokens/SaveTokens) carries no context.Context, so the keyring path can't
+// ride the root context and instead detects Ctrl-C via its own signal listener.
+// Once that interface accepts a context, keyring cancellation can flow from the
+// root context like everything else, the tokenstore signal listener can go
+// away, and this package with it.
+package procsignal
+
+import (
+	"os"
+	"sync/atomic"
+)
+
+// holder wraps the stored signal so atomic.Value always observes one concrete
+// type. Storing differing concrete types (or nil) into an atomic.Value panics;
+// wrapping avoids both.
+type holder struct{ sig os.Signal }
+
+var caught atomic.Value // stores holder
+
+// Store records sig as the signal that initiated shutdown. Safe for concurrent
+// use; last writer wins, which is fine because every caller stores a genuine
+// terminating signal.
+func Store(sig os.Signal) {
+	caught.Store(holder{sig: sig})
+}
+
+// Load returns the recorded terminating signal, or nil if none was recorded.
+func Load() os.Signal {
+	if h, ok := caught.Load().(holder); ok {
+		return h.sig
+	}
+	return nil
+}
+
+// Reset clears the recorded signal. It exists for tests that need a clean
+// slate; production code never clears it (the process is on its way out).
+func Reset() {
+	caught.Store(holder{})
+}