Merge pull request #1604 from entireio/serialized-plotting-thompson

fix(cli): Ctrl-C escapes shell loops and aborts cleanly

Author: Soph

cmd/entire/main.go

@@ -8,7 +8,9 @@ import (
 	"os/signal"
 	"runtime"
 	"strings"
+	"sync/atomic"
 	"syscall"
+	"time"
 
 	"github.com/entireio/cli/cmd/entire/cli"
 	"github.com/entireio/cli/cmd/entire/cli/api"
@@ -31,8 +33,27 @@ func main() {
 	}
 	signal.Notify(sigChan, signals...)
 	go func() {
-		<-sigChan
+		// First signal: cancel the context so in-flight work unwinds
+		// cleanly. signal.Notify has disabled Go's default "signal
+		// terminates" behavior, so without the second read below a user
+		// who Ctrl-C's again during a slow/stuck shutdown (e.g. a keyring
+		// read blocked in a subprocess we can't cancel) would find every
+		// further Ctrl-C swallowed. The second read restores an escape
+		// hatch: signal again to force-exit.
+		//
+		// We remember which signal fired so the eventual termination
+		// 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)
+		if sig == os.Interrupt {
+			fmt.Fprintln(os.Stderr, "\nInterrupting… press Ctrl-C again to force quit.")
+		} else {
+			fmt.Fprintln(os.Stderr, "\nReceived termination signal, shutting down… signal again to force quit.")
+		}
 		cancel()
+		<-sigChan
+		dieFromSignal(sig)
 	}()
 
 	// Create and execute root command
@@ -68,6 +89,22 @@ 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.
+			//
+			// We gate on the handler having fired 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).
+			cancel()
+			dieFromSignal(terminatingSignal())
 		case errors.As(err, &silent):
 			// Command already printed the error
 		case strings.Contains(err.Error(), "unknown command") || strings.Contains(err.Error(), "unknown flag"):
@@ -93,6 +130,53 @@ 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).
+func terminatingSignal() os.Signal {
+	if v := caughtSignal.Load(); v != nil {
+		if s, ok := v.(os.Signal); ok {
+			return s
+		}
+	}
+	return os.Interrupt
+}
+
+// dieFromSignal terminates the process as if it had been killed by sig, rather
+// than exiting normally. The distinction matters to an interactive shell: it
+// only aborts a `while true; do entire ...; done` loop when the child is
+// *killed by* SIGINT (WIFSIGNALED). A plain os.Exit(130) is an ordinary exit,
+// so the loop keeps respawning entire and Ctrl-C never escapes it. Re-raising
+// the actual signal also keeps a SIGTERM shutdown reporting the conventional
+// 143 (not 130). We reset sig to its default disposition, re-raise it to
+// ourselves, and briefly wait for delivery; if the re-raise can't be delivered
+// (e.g. Windows, where signal-to-self is unsupported) we fall back to a
+// conventional 128+signal exit so we never hang.
+func dieFromSignal(sig os.Signal) {
+	signal.Reset(sig)
+	if p, err := os.FindProcess(os.Getpid()); err == nil {
+		if err := p.Signal(sig); err == nil {
+			time.Sleep(500 * time.Millisecond) // signal delivery ends the process well before this elapses
+		}
+	}
+	os.Exit(exitCodeForSignal(sig))
+}
+
+// exitCodeForSignal maps a signal to the conventional 128+signum exit code
+// (130 for SIGINT, 143 for SIGTERM), falling back to 130 for a signal that
+// doesn't carry a numeric value on this platform.
+func exitCodeForSignal(sig os.Signal) int {
+	if s, ok := sig.(syscall.Signal); ok {
+		return 128 + int(s)
+	}
+	return 130
+}
+
 // isPositionalArgError reports whether err looks like a cobra positional-
 // arg validator failure. cobra's stock validators (ExactArgs, NoArgs,
 // MinimumNArgs, MaximumNArgs, RangeArgs) all surface error strings

cmd/entire/main_test.go

@@ -0,0 +1,39 @@
+package main
+
+import (
+	"os"
+	"syscall"
+	"testing"
+)
+
+// 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()        {}
+
+// 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.
+func TestExitCodeForSignal(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name string
+		sig  os.Signal
+		want int
+	}{
+		{"SIGINT", os.Interrupt, 130},
+		{"SIGTERM", syscall.SIGTERM, 143},
+		{"non-numeric signal falls back to 130", nonNumericSignal{}, 130},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			if got := exitCodeForSignal(tc.sig); got != tc.want {
+				t.Errorf("exitCodeForSignal(%v) = %d, want %d", tc.sig, got, tc.want)
+			}
+		})
+	}
+}

internal/entireclient/tokenstore/keyring_timeout.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"os"
+	"os/signal"
 	"runtime"
 	"time"
 )
@@ -36,15 +37,34 @@ func keyringTimeout() time.Duration {
 }
 
 // callKeyringWithTimeout runs fn in a goroutine and returns its result,
-// or a descriptive error if the configured keyring timeout elapses
-// first. The goroutine continues running — a blocked D-Bus syscall
-// can't be cancelled from Go — and its eventual result is discarded.
-// The buffered result channel keeps the goroutine from leaking forever
-// waiting to publish into a receiver that's already gone. fn's own
-// error (including ErrNotFound) propagates unchanged on the fast path;
-// only the timeout branch wraps.
+// or a descriptive error if the configured keyring timeout elapses or the
+// user interrupts (Ctrl-C) first. The goroutine continues running — a
+// blocked D-Bus syscall / Keychain subprocess can't be cancelled from Go —
+// and its eventual result is discarded. The buffered result channel keeps
+// the goroutine from leaking forever waiting to publish into a receiver
+// that's already gone. fn's own error (including ErrNotFound) propagates
+// unchanged on the fast path; only the timeout and interrupt branches wrap.
+//
+// It listens for SIGINT for the duration of the call so a Ctrl-C unblocks a
+// stuck keyring read *now* rather than after the full timeout. This is the
+// only cancellation lever available here: the credential store is reached
+// through auth-go's Store interface (LoadTokens/SaveTokens), which carries
+// no context.Context, so a per-request context can't be threaded down to
+// this point. signal.Notify fans a signal out to every registered channel,
+// so the process's own handler (which cancels the root context) still runs
+// — this is an additional listener scoped to the keyring call.
 func callKeyringWithTimeout(op string, fn func() (string, error)) (string, error) {
-	ctx, cancel := context.WithTimeout(context.Background(), keyringTimeout())
+	sigCh := make(chan os.Signal, 1)
+	signal.Notify(sigCh, os.Interrupt)
+	defer signal.Stop(sigCh)
+	return callKeyringWithInterrupt(op, keyringTimeout(), fn, sigCh)
+}
+
+// callKeyringWithInterrupt is the testable core of callKeyringWithTimeout:
+// the interrupt source is injected so tests can exercise the Ctrl-C branch
+// without sending real signals to the test process.
+func callKeyringWithInterrupt(op string, timeout time.Duration, fn func() (string, error), interrupt <-chan os.Signal) (string, error) {
+	ctx, cancel := context.WithTimeout(context.Background(), timeout)
 	defer cancel()
 
 	type result struct {
@@ -59,6 +79,10 @@ func callKeyringWithTimeout(op string, fn func() (string, error)) (string, error
 	select {
 	case r := <-ch:
 		return r.val, r.err
+	case <-interrupt:
+		// Wrap context.Canceled so the abort flows into the CLI's silent
+		// "user aborted" exit path rather than printing as a keyring failure.
+		return "", fmt.Errorf("%s interrupted: %w", op, context.Canceled)
 	case <-ctx.Done():
 		return "", fmt.Errorf(
 			"%s timed out: OS keyring (%s) appears unavailable; set %s to a longer duration to wait further: %w",

internal/entireclient/tokenstore/keyring_timeout_test.go

@@ -3,6 +3,7 @@ package tokenstore
 import (
 	"context"
 	"errors"
+	"os"
 	"strings"
 	"testing"
 	"time"
@@ -76,6 +77,38 @@ func TestCallKeyringWithTimeout_DeadlineExceeded(t *testing.T) {
 	}
 }
 
+// A Ctrl-C must unblock a stuck keyring call immediately — well before the
+// timeout — and surface as a context.Canceled so the CLI treats it as a user
+// abort rather than a keyring failure.
+func TestCallKeyringWithInterrupt_AbortsOnSignal(t *testing.T) {
+	t.Parallel()
+
+	interrupt := make(chan os.Signal, 1)
+	started := make(chan struct{})
+	start := time.Now()
+	go func() {
+		<-started
+		interrupt <- os.Interrupt
+	}()
+
+	_, 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
+	}, interrupt)
+	elapsed := time.Since(start)
+
+	if !errors.Is(err, context.Canceled) {
+		t.Fatalf("want context.Canceled wrapped, got %v", err)
+	}
+	if elapsed > 2*time.Second {
+		t.Fatalf("interrupt did not return promptly: elapsed=%s", elapsed)
+	}
+	if !strings.Contains(err.Error(), "interrupted") {
+		t.Errorf("error %q should mention it was interrupted", err.Error())
+	}
+}
+
 func TestKeyringTimeout_DefaultWhenUnset(t *testing.T) {
 	t.Setenv(keyringTimeoutEnvVar, "")