Skip to content

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

Merged
Soph merged 4 commits into
mainfrom
serialized-plotting-thompson
Jul 3, 2026
Merged

fix(cli): Ctrl-C escapes shell loops and aborts cleanly#1604
Soph merged 4 commits into
mainfrom
serialized-plotting-thompson

Conversation

@toothbrush

@toothbrush toothbrush commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

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

while true; do entire mirror create …; done couldn't be stopped with Ctrl-C, and interrupts spewed raw transport/keyring errors.

Cause: entire trapped SIGINT and exited normally. A shell only breaks a loop when the child is killed by SIGINT, so each Ctrl-C just ended one iteration and the loop respawned entire instantly.

Fixes:

  • On abort, re-raise SIGINT to self → the process dies by the signal → enclosing loops break on a single Ctrl-C.
  • Second Ctrl-C force-quits (escape hatch for a stuck shutdown).
  • context.Canceled exits silently (130) instead of printing do request: Post ".../mirrors": … context canceled.
  • Keyring reads now abort on Ctrl-C instead of waiting out the 5s timeout (or hanging on a daemon-less box).

Try it: while true; do entire repo mirror create …; done, then Ctrl-C once — the loop now exits.

Verified with a PTY harness driving interactive zsh: one Ctrl-C breaks the loop (0 further iterations) vs. the old behavior (loop kept running). mise run test:ci green.

🤖 Generated with Claude Code


Note

Medium Risk
Changes global signal handling and exit semantics for all CLI invocations; incorrect behavior could affect shutdown, loop scripts, or interrupt during credential access.

Overview
Fixes Ctrl-C so it reliably stops the CLI and breaks enclosing shell loops (e.g. while true; do entire …; done), instead of respawning each iteration or printing noisy cancellation/keyring errors.

Process shutdown (main): The first SIGINT cancels the root context and prints a short “Interrupting…” hint; a second Ctrl-C force-quits. User aborts that surface as context.Canceled no longer print as failures—they go through dieFromInterrupt, which resets SIGINT and re-raises it to the process so the shell sees a signal-killed child (not a plain exit 130), which is what breaks while loops.

Keyring reads: Wrapped OS keyring calls now also listen for SIGINT for the duration of the call, returning immediately with an error wrapping context.Canceled (same silent abort path) instead of waiting for the full timeout when the backend is stuck.

Adds a unit test for the injectable interrupt path on keyring calls.

Reviewed by Cursor Bugbot for commit b3d0b68. Configure here.

toothbrush and others added 2 commits July 2, 2026 14:36
An interactive shell only aborts a `while true; do entire ...; done`
loop when the child is killed *by* SIGINT (WIFSIGNALED). entire trapped
SIGINT and exited normally, so every Ctrl-C just ended one iteration and
the loop instantly respawned entire — the user could never break out.

- On abort, re-raise SIGINT to self (dieFromInterrupt) so the process
  dies by the signal and enclosing loops break on a single Ctrl-C.
- Second Ctrl-C force-quits, so a genuinely stuck shutdown still has an
  escape hatch (the first signal now only cancels + warns).
- Treat context.Canceled as a silent 130 exit instead of dumping the raw
  transport/keyring cancellation string as if it were a failure.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: be700a8a175c
The OS keyring call ran on context.Background(), so a Ctrl-C during a
slow/blocked read (Keychain subprocess, or a headless box with no keyring
daemon) left the user waiting out the full 5s timeout — or hanging.

Listen for SIGINT for the duration of the call and return early wrapping
context.Canceled (which flows into the CLI's silent user-abort exit).
signal.Notify fans out to every channel, so the process's own handler
still cancels the root context. A per-request context can't be threaded
here: the store is reached via auth-go's Store interface, which carries
no context.Context.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 32928d28c989
Copilot AI review requested due to automatic review settings July 2, 2026 05:07
@toothbrush
toothbrush requested a review from a team as a code owner July 2, 2026 05:07

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 improves interrupt (Ctrl-C) behavior across the Entire CLI so cancellations abort cleanly, break enclosing shell loops, and avoid noisy “context canceled” / keyring transport errors.

Changes:

  • Updates process-level signal handling so a canceled root context exits via a SIGINT-style termination (vs. a normal exit) to escape shell loops.
  • Makes OS keyring operations abort promptly on Ctrl-C (instead of waiting for the keyring timeout), surfacing as context.Canceled.
  • Adds a unit test to exercise the keyring interrupt path via an injected interrupt channel.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
cmd/entire/main.go Adjusts global signal handling and cancellation exit semantics to better match Ctrl-C expectations in shells.
internal/entireclient/tokenstore/keyring_timeout.go Adds SIGINT listening to keyring calls so Ctrl-C unblocks stuck keyring reads before the timeout.
internal/entireclient/tokenstore/keyring_timeout_test.go Adds a unit test validating the interrupt branch returns promptly and wraps context.Canceled.

Comment thread cmd/entire/main.go Outdated
Comment thread cmd/entire/main.go Outdated
Addresses PR review: the handler also catches SIGTERM on non-Windows, but
the exit path unconditionally re-raised SIGINT and printed a "press Ctrl-C
again" hint. A SIGTERM shutdown (supervisor / container stop) would exit
130 instead of the conventional 143 and show a misleading message.

Capture which signal fired, re-raise that same signal (SIGINT→130,
SIGTERM→143 via 128+signum), and tailor the first-signal message.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 3ef3536ec8b3
Soph
Soph previously approved these changes Jul 2, 2026
Addresses two trail findings:

- Matching context.Canceled at the top level was too broad: an
  internally-cancelled sub-context would have silently re-raised SIGINT
  (exit 130) and broken an enclosing loop even without a Ctrl-C. Gate the
  abort path on caughtSignal being set, so a signal-less context.Canceled
  falls through to normal error reporting.
- Add a table test for exitCodeForSignal locking SIGINT→130, SIGTERM→143
  (and the non-numeric fallback), guarding the mapping the fix relies on.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: e087cbfd44e3
@Soph
Soph merged commit 6a41271 into main Jul 3, 2026
10 checks passed
@Soph
Soph deleted the serialized-plotting-thompson branch July 3, 2026 11:15
huangyingting pushed a commit to repomesh/cli that referenced this pull request Jul 4, 2026
PR entireio#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
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