Skip to content

feat: forward SIGINT/SIGTERM to wrapped tools for graceful shutdown#391

Open
gtsiolis wants to merge 2 commits into
mainfrom
lav-973-forward-sigintsigterm-to-wrapped-tools-before-c871
Open

feat: forward SIGINT/SIGTERM to wrapped tools for graceful shutdown#391
gtsiolis wants to merge 2 commits into
mainfrom
lav-973-forward-sigintsigterm-to-wrapped-tools-before-c871

Conversation

@gtsiolis

@gtsiolis gtsiolis commented Jul 16, 2026

Copy link
Copy Markdown
Member

Pressing Ctrl-C during a wrapped-tool run (lstk terraform apply, aws, cdk, sam, az, extensions) killed the child immediately instead of letting it shut down gracefully — so terraform never got to release its state lock. The tools run via exec.CommandContext with lstk's root context, which is cancelled on SIGINT/SIGTERM; exec.CommandContext's default Cancel then SIGKILLs the child right away.

All wrapped-tool passthroughs now run through a new proc.Run helper that:

  • disarms the SIGKILL-on-cancel (its Cancel returns os.ErrProcessDone), so the tool terminates from the signal it received and lstk waits for its own shutdown — preserving the real exit code (Ctrl-C on terraform now exits 130 after full cleanup instead of signal: killed/255);
  • forwards SIGTERM unconditionally: a terminal never generates it, so without forwarding kill <lstk-pid>, timeout 30 lstk …, or an IDE stop button would never reach the tool;
  • forwards SIGINT only when none of stdin/stdout/stderr is a terminal: an attached terminal already delivers Ctrl-C to the child via the foreground process group, and a second near-simultaneous SIGINT makes tools like terraform abort without cleaning up. Checking all three streams matters — with only stdin redirected (yes | lstk terraform apply) the process group still delivers;
  • registers its signal handler before Start, so a signal landing during startup is queued rather than lost.

(npm/launcher.js forwards unconditionally instead — safe there because its child is lstk itself, which tolerates duplicate signals; wrapped tools don't.)

Short internal captured-output execs (version checks, schema discovery, backend provisioning) are unchanged. No configurable grace period yet, per the issue scope.

Tests: internal/proc unit tests cover the disarm/exit-code semantics and the forwarding policy; test/integration/signal_forwarding_test.go covers the behavior end to end through a new signal-wait mode of the reference extension (exits 40 + number of signals received) — non-interactive SIGINT/SIGTERM forwarding, exactly-one SIGINT on Ctrl-C in a PTY, the piped-stdin double-signal regression, and SIGTERM reaching the tool from inside a terminal.

Fixes DEVX-1000

Wrapped tools (aws, terraform, cdk, sam, az, extensions) were run via
exec.CommandContext with lstk's root context, which is cancelled on
SIGINT/SIGTERM. exec.CommandContext's default Cancel then SIGKILLs the
child immediately, so Ctrl-C during e.g. `lstk terraform apply` killed
terraform before it could clean up and release its state lock.

Route these execs through a new proc.Run helper that disarms the
SIGKILL-on-cancel (preserving the tool's real exit code) and forwards
SIGINT/SIGTERM to the child in the non-interactive case, letting the
tool shut down gracefully. Interactive Ctrl-C already reaches the child
via the controlling terminal's process group, so forwarding is
suppressed there to avoid a double signal. Mirrors npm/launcher.js.

Generated with [Linear](https://linear.app/localstack/issue/LAV-973/forward-sigintsigterm-to-wrapped-tools-before-terminating-them#agent-session-efa68cad)

Co-authored-by: linear-code[bot] <222613912+linear-code[bot]@users.noreply.github.com>
@gtsiolis gtsiolis added semver: patch docs: skip Pull request does not require documentation changes labels Jul 16, 2026
…nal stream

Two gaps in the TTY heuristic, both proven by failing integration tests
before the fix:

- Gating suppression on stdin alone re-armed forwarding for piped-stdin
  interactive runs (`yes | lstk terraform apply`): the terminal's
  foreground process group already delivers Ctrl-C, so the wrapped tool
  received a second SIGINT and aborted without cleanup - the exact
  stale-lock bug this branch fixes. SIGINT suppression now triggers when
  any of stdin/stdout/stderr is a terminal.

- SIGTERM is never terminal-generated, so suppressing it interactively
  meant `kill <lstk-pid>`, `timeout`, or an IDE stop button never
  reached the tool while lstk swallowed the signal and waited. SIGTERM
  now forwards unconditionally.

Also registers the signal handler before Start so a signal landing
during startup is queued instead of lost, adds end-to-end signal tests
via the reference extension's new signal-wait mode, skips sh-based unit
tests on Windows, and corrects the npm/launcher.js comparison (the
launcher forwards unconditionally; its child tolerates duplicates).

Co-Authored-By: Claude <noreply@anthropic.com>
@gtsiolis
gtsiolis marked this pull request as ready for review July 16, 2026 17:50
@gtsiolis
gtsiolis requested a review from a team as a code owner July 16, 2026 17:50
@gtsiolis gtsiolis self-assigned this Jul 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs: skip Pull request does not require documentation changes semver: patch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant