Skip to content

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

Merged
toothbrush merged 2 commits into
mainfrom
soph/keyring-interrupt-race-followup
Jul 4, 2026
Merged

fix(cli): close keyring-interrupt race in Ctrl-C signal-abort#1625
toothbrush merged 2 commits into
mainfrom
soph/keyring-interrupt-race-followup

Conversation

@Soph

@Soph Soph commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

https://entire.io/gh/entireio/cli/trails/749

Follow-up to #1604.

Problem

#1604 made Ctrl-C break enclosing shell loops by re-raising the caught signal, gating the silent-abort path on a caughtSignal atomic set by the top-level signal-handler goroutine. A Ctrl-C during a blocked keyring read could still slip through:

  • The keyring's own signal listener returns a wrapped context.Canceled independently of the root-context cancellation.
  • So the main flow can reach the abort gate (errors.Is(err, context.Canceled) && caughtSignal.Load() != nil) and read caughtSignal before the handler goroutine stores it — there's no happens-before between the keyring's return and that store.

When the race lost, the raw "...: context canceled" string printed as a failure and the process exited 1 (not signal-killed), so the loop kept respawning — the exact symptom #1604 set out to fix, on the very path (a stuck keyring read) it targets.

Fix

Unify "were we signalled?" behind a single shared source of truth instead of two uncoordinated mechanisms:

  • internal/procsignal — a tiny package holding the caught terminating signal, importable by both cmd/entire (top-level handler) and internal/entireclient/tokenstore (keyring path) without an import cycle.
  • cmd/entire/main.go — replace the local caughtSignal atomic.Value with procsignal; the handler and the abort gate both go through it.
  • keyring_timeout.go — on the interrupt branch, record the signal via procsignal on the same goroutine that unwinds to main's gate (recordInterruptSignal). This turns the cross-goroutine race into a same-goroutine happens-before, so the gate can never read the flag before it's set. Timeouts (DeadlineExceeded) are deliberately left untouched.

This also addresses the altitude critique from the review: signal-abort detection is now one shared value written by both writers, not two flags racing.

Tests

  • procsignal store/load/reset unit tests (incl. mixed concrete signal types).
  • TestRecordInterruptSignal — a Ctrl-C abort records SIGINT; a timeout/success does not.
  • TestDieFromSignal_TerminatesBySignal — deterministic regression guard for the headline behavior: re-execs the test binary in a child mode that calls dieFromSignal, then asserts the child died by the signal (WaitStatus.Signaled() + correct SIGINT/SIGTERM), not a normal exit. This locks the WIFSIGNALED property that makes loops break — a "simplify back to os.Exit(130)" would fail it immediately.

Note on a PTY loop test

I went with the deterministic subprocess guard rather than a PTY-driven shell-loop test. The loop breaks because the process is WIFSIGNALED, which the subprocess test locks deterministically in normal CI. A real PTY-loop test can't be reliably exercised on macOS (reaching the die-by-signal path needs entire blocked in a hung keyring read, which only actually hangs on a headless/no-Secret-Service box), so it would just skip locally. Happy to add a build-tagged, self-skipping PTY variant for headless-Linux CI if desired.

Verification

mise run fmt, mise run lint, -race on the three affected packages, and the full mise run test:ci all green.

🤖 Generated with Claude Code


Note

Medium Risk
Touches process shutdown, signal handling, and keyring interrupt paths—behavioral changes to Ctrl-C/SIGTERM exit semantics, though scoped and well-tested.

Overview
Fixes a race where Ctrl-C during a stuck keyring read could still print a failure and exit 1 instead of the quiet signal re-raise that breaks enclosing shell loops.

A new internal/procsignal package is the shared “were we signalled?” store. cmd/entire drops its local caughtSignal atomic and uses procsignal in the top-level handler and the context.Canceled abort gate. tokenstore wraps keyring calls with recordInterruptSignal, which records SIGINT on the same goroutine that returns to main when the interrupt path yields context.Canceled, so the gate no longer loses to the async handler goroutine. Timeouts stay unchanged.

Tests add procsignal unit coverage, TestRecordInterruptSignal, and TestDieFromSignal_TerminatesBySignal (re-exec child asserts WIFSIGNALED, not a normal exit with code 130).

Reviewed by Cursor Bugbot for commit b1c063c. Configure here.

PR #1604 made Ctrl-C break enclosing shell loops by re-raising the caught
signal, gating the silent-abort path on a caughtSignal atomic set by the
top-level signal handler goroutine. A Ctrl-C during a blocked keyring read
could still slip through: the keyring's own signal listener returns a wrapped
context.Canceled independently of the root-context cancellation, so the main
flow could reach the abort gate and read caughtSignal before the handler
goroutine stored it. When that happened the raw "...: context canceled" error
printed as a failure and the process exited 1 (not signal-killed), so the
loop kept respawning — the exact symptom the PR fixed.

Unify "were we signalled?" behind a single shared source of truth:

- Add internal/procsignal: a tiny package holding the caught terminating
  signal, importable by both cmd/entire and the tokenstore keyring path
  (no import cycle).
- cmd/entire/main.go: replace the local caughtSignal atomic with procsignal;
  the handler and the abort gate both go through it.
- tokenstore keyring: on the interrupt branch, record the signal via
  procsignal on the *same goroutine* that unwinds to main's gate
  (recordInterruptSignal), turning the cross-goroutine race into a
  same-goroutine happens-before. Timeouts (DeadlineExceeded) are untouched.

Tests:
- procsignal store/load/reset unit tests.
- TestRecordInterruptSignal: a Ctrl-C abort records SIGINT; timeout/success
  do not.
- TestDieFromSignal_TerminatesBySignal: deterministic regression guard that
  re-execs the test binary and asserts it dies *by* the signal (WIFSIGNALED,
  SIGINT/SIGTERM) rather than exiting normally — the WIFSIGNALED property a
  "simplify back to os.Exit(130)" would silently regress.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Entire-Checkpoint: 72e0907000ba
Copilot AI review requested due to automatic review settings July 3, 2026 13:02
@Soph Soph requested a review from a team as a code owner July 3, 2026 13:02

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a Ctrl-C race during blocked OS keyring operations by introducing a shared, process-wide “caught terminating signal” store. It ensures the top-level context.Canceled abort gate reliably re-raises the terminating signal (so enclosing shell loops break) even when cancellation originates from the keyring interrupt path before the main signal-handler goroutine records the signal.

Changes:

  • Added internal/procsignal as a shared signal store (Store/Load/Reset) used across cmd/entire and the keyring tokenstore code.
  • Updated cmd/entire/main.go to use procsignal for both signal capture and the context.Canceled silent-abort gate.
  • Updated keyring timeout handling to record SIGINT on the same goroutine that returns the wrapped context.Canceled, with unit tests including a subprocess “dies by signal” regression guard.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
internal/procsignal/procsignal.go Adds a small atomic-backed shared store for the terminating signal.
internal/procsignal/procsignal_test.go Unit tests for Store/Load/Reset and mixed concrete signal types.
internal/entireclient/tokenstore/keyring_timeout.go Records SIGINT in procsignal when the keyring call returns a wrapped context.Canceled from the interrupt branch.
internal/entireclient/tokenstore/keyring_timeout_test.go Adds tests for recordInterruptSignal behavior and refactors a sentinel return value.
cmd/entire/main.go Replaces local caughtSignal with procsignal for consistent signal-abort detection and termination semantics.
cmd/entire/main_test.go Adds a deterministic subprocess test asserting dieFromSignal results in WIFSIGNALED termination (SIGINT/SIGTERM).

Post-review cleanup (no behavior change):
- main_test.go: collapse the duplicated INT/TERM cases in TestMain's
  child-mode switch into one signal-selection path.
- procsignal.go: document why the shared global exists (auth-go's Store
  interface carries no context) and the path to removing it.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Entire-Checkpoint: 0fa8a28fa29e

@toothbrush toothbrush left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks 👌

@toothbrush toothbrush merged commit 875d597 into main Jul 4, 2026
10 checks passed
@toothbrush toothbrush deleted the soph/keyring-interrupt-race-followup branch July 4, 2026 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants