feat: forward SIGINT/SIGTERM to wrapped tools for graceful shutdown#391
Open
gtsiolis wants to merge 2 commits into
Open
feat: forward SIGINT/SIGTERM to wrapped tools for graceful shutdown#391gtsiolis wants to merge 2 commits into
gtsiolis wants to merge 2 commits into
Conversation
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>
…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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 viaexec.CommandContextwith lstk's root context, which is cancelled onSIGINT/SIGTERM;exec.CommandContext's defaultCancelthen SIGKILLs the child right away.All wrapped-tool passthroughs now run through a new
proc.Runhelper that:Cancelreturnsos.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 ofsignal: killed/255);SIGTERMunconditionally: a terminal never generates it, so without forwardingkill <lstk-pid>,timeout 30 lstk …, or an IDE stop button would never reach the tool;SIGINTonly 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;Start, so a signal landing during startup is queued rather than lost.(
npm/launcher.jsforwards 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/procunit tests cover the disarm/exit-code semantics and the forwarding policy;test/integration/signal_forwarding_test.gocovers the behavior end to end through a newsignal-waitmode 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