From 5572cc5354efe73eca789b4cc3c23aa5997737c9 Mon Sep 17 00:00:00 2001 From: George Tsiolis <120486+gtsiolis@users.noreply.github.com> Date: Thu, 16 Jul 2026 14:45:54 +0000 Subject: [PATCH 1/2] feat: forward SIGINT/SIGTERM to wrapped tools for graceful shutdown 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> --- CLAUDE.md | 5 ++ internal/awscli/exec.go | 3 +- internal/azurecli/exec.go | 4 +- internal/extension/exec.go | 3 +- internal/iac/cdk/cli/exec.go | 3 +- internal/iac/sam/cli/exec.go | 3 +- internal/iac/terraform/cli/exec.go | 3 +- internal/proc/run.go | 70 +++++++++++++++++++++++++++ internal/proc/run_test.go | 78 ++++++++++++++++++++++++++++++ 9 files changed, 166 insertions(+), 6 deletions(-) create mode 100644 internal/proc/run.go create mode 100644 internal/proc/run_test.go diff --git a/CLAUDE.md b/CLAUDE.md index a1c56867..1e026ee9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -136,6 +136,11 @@ Scope: the first release **runs** extensions (PATH and bundled-dir resolution) a See [extensions-authoring.md](docs/extensions-authoring.md) for the author-facing contract. +# Signal Forwarding to Wrapped Tools + +Wrapped external tools (`aws`, `terraform`, `cdk`, `sam`, `az`, and extensions) are run through `proc.Run(cmd)` (in `internal/proc/run.go`) rather than `cmd.Run()`. These execs are created with `exec.CommandContext` using lstk's root context, which is cancelled on `SIGINT`/`SIGTERM`; `exec.CommandContext`'s default `Cancel` would then SIGKILL the child immediately, denying tools like `terraform apply` the chance to clean up (e.g. release the state lock). `proc.Run` disarms that (its `Cancel` returns `os.ErrProcessDone`, which both suppresses the kill and avoids injecting `context.Canceled` into the wait result, preserving the tool's real exit code) and instead lets the tool terminate from the signal it receives, waiting for it to finish its own shutdown. In an interactive terminal the child already receives the signal via the controlling terminal's foreground process group, so `proc.Run` forwards `SIGINT`/`SIGTERM` only when stdin is not a terminal (a second near-simultaneous signal would make tools like terraform abort immediately). This mirrors the model in `npm/launcher.js`: the TTY covers interactive use, forwarding covers the non-interactive case. Short internal captured-output execs (version checks, schema discovery, backend provisioning) still use `cmd.Run()` directly. + + # Snapshots `lstk snapshot` captures and restores the running emulator's state. For Snowflake and Azure, snapshot support is still maturing, so these commands surface a friendly heads-up that results may be incomplete. Domain logic lives in `internal/snapshot/`; `cmd/snapshot.go` is wiring + output-mode selection. diff --git a/internal/awscli/exec.go b/internal/awscli/exec.go index ea91d1ac..b4bfdf29 100644 --- a/internal/awscli/exec.go +++ b/internal/awscli/exec.go @@ -14,6 +14,7 @@ import ( "github.com/localstack/lstk/internal/awsconfig" "github.com/localstack/lstk/internal/output" + "github.com/localstack/lstk/internal/proc" ) const InstallURL = "https://docs.aws.amazon.com/cli/latest/userguide/getting-started-install.html" @@ -62,7 +63,7 @@ func Exec(ctx context.Context, endpointURL string, useProfile bool, stdout, stde cmd.Env = BuildEnv(os.Environ()) } - if err := cmd.Run(); err != nil { + if err := proc.Run(cmd); err != nil { var exitErr *exec.ExitError if errors.As(err, &exitErr) { span.SetAttributes(attribute.Int("aws.exit_code", exitErr.ExitCode())) diff --git a/internal/azurecli/exec.go b/internal/azurecli/exec.go index a887d2a4..99b76f1d 100644 --- a/internal/azurecli/exec.go +++ b/internal/azurecli/exec.go @@ -12,6 +12,8 @@ import ( "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" + + "github.com/localstack/lstk/internal/proc" ) const InstallURL = "https://learn.microsoft.com/en-us/cli/azure/" @@ -51,7 +53,7 @@ func Exec(ctx context.Context, extraEnv []string, stdin io.Reader, stdout, stder if len(extraEnv) > 0 { cmd.Env = append(os.Environ(), extraEnv...) } - if err := cmd.Run(); err != nil { + if err := proc.Run(cmd); err != nil { var exitErr *exec.ExitError if errors.As(err, &exitErr) { span.SetAttributes(attribute.Int("az.exit_code", exitErr.ExitCode())) diff --git a/internal/extension/exec.go b/internal/extension/exec.go index e2289ce9..66dc3c0e 100644 --- a/internal/extension/exec.go +++ b/internal/extension/exec.go @@ -11,6 +11,7 @@ import ( "go.opentelemetry.io/otel/codes" "github.com/localstack/lstk/internal/output" + "github.com/localstack/lstk/internal/proc" ) // Invoke executes the resolved extension with args forwarded unmodified, @@ -51,7 +52,7 @@ func Invoke(ctx context.Context, ext *Extension, args []string, runCtx Context) cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr - if err := cmd.Run(); err != nil { + if err := proc.Run(cmd); err != nil { var exitErr *exec.ExitError if errors.As(err, &exitErr) { span.SetAttributes(attribute.Int("extension.exit_code", exitErr.ExitCode())) diff --git a/internal/iac/cdk/cli/exec.go b/internal/iac/cdk/cli/exec.go index 95b6712d..23bae660 100644 --- a/internal/iac/cdk/cli/exec.go +++ b/internal/iac/cdk/cli/exec.go @@ -15,6 +15,7 @@ import ( "github.com/localstack/lstk/internal/endpoint" "github.com/localstack/lstk/internal/log" "github.com/localstack/lstk/internal/output" + "github.com/localstack/lstk/internal/proc" ) // Run proxies an AWS CDK invocation against LocalStack. It locates the cdk @@ -75,7 +76,7 @@ func Run(ctx context.Context, endpointURL, region string, sink output.Sink, logg cmd.Stderr = os.Stderr cmd.Env = BuildEnv(os.Environ(), effectiveEndpoint, s3Endpoint, region) - if err := cmd.Run(); err != nil { + if err := proc.Run(cmd); err != nil { var exitErr *exec.ExitError if errors.As(err, &exitErr) { span.SetAttributes(attribute.Int("cdk.exit_code", exitErr.ExitCode())) diff --git a/internal/iac/sam/cli/exec.go b/internal/iac/sam/cli/exec.go index 38bde0a2..053112a0 100644 --- a/internal/iac/sam/cli/exec.go +++ b/internal/iac/sam/cli/exec.go @@ -13,6 +13,7 @@ import ( "github.com/localstack/lstk/internal/log" "github.com/localstack/lstk/internal/output" + "github.com/localstack/lstk/internal/proc" ) const installDocsURL = "https://docs.aws.amazon.com/serverless-application-model/latest/developerguide/install-sam-cli.html" @@ -69,7 +70,7 @@ func Run(ctx context.Context, endpointURL, account, region string, sink output.S cmd.Stderr = os.Stderr cmd.Env = BuildEnv(os.Environ(), effectiveEndpoint, account, region) - if err := cmd.Run(); err != nil { + if err := proc.Run(cmd); err != nil { var exitErr *exec.ExitError if errors.As(err, &exitErr) { span.SetAttributes(attribute.Int("sam.exit_code", exitErr.ExitCode())) diff --git a/internal/iac/terraform/cli/exec.go b/internal/iac/terraform/cli/exec.go index 223d8e64..4da6da64 100644 --- a/internal/iac/terraform/cli/exec.go +++ b/internal/iac/terraform/cli/exec.go @@ -17,6 +17,7 @@ import ( "github.com/localstack/lstk/internal/endpoint" "github.com/localstack/lstk/internal/log" "github.com/localstack/lstk/internal/output" + "github.com/localstack/lstk/internal/proc" ) // Run proxies a terraform invocation against LocalStack. The path taken depends @@ -229,7 +230,7 @@ func runTerraform(ctx context.Context, span trace.Span, tfBin string, args []str cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr - if err := cmd.Run(); err != nil { + if err := proc.Run(cmd); err != nil { var exitErr *exec.ExitError if errors.As(err, &exitErr) { span.SetAttributes(attribute.Int("terraform.exit_code", exitErr.ExitCode())) diff --git a/internal/proc/run.go b/internal/proc/run.go new file mode 100644 index 00000000..01aa1134 --- /dev/null +++ b/internal/proc/run.go @@ -0,0 +1,70 @@ +// Package proc runs wrapped external tools (aws, terraform, cdk, sam, az, and +// extensions) so that termination signals reach the child gracefully instead of +// hard-killing it. +package proc + +import ( + "os" + "os/exec" + "os/signal" + "syscall" + + "github.com/localstack/lstk/internal/terminal" +) + +// forwardedSignals are relayed to the wrapped child so it can shut down +// gracefully (e.g. terraform releasing its state lock) rather than being killed +// abruptly. +var forwardedSignals = []os.Signal{os.Interrupt, syscall.SIGTERM} + +// Run starts cmd, waits for it to exit, and returns the same error cmd.Run +// would. It ensures SIGINT/SIGTERM reach the wrapped tool so it can clean up +// instead of being SIGKILLed the instant lstk's context is cancelled. +// +// When cmd was created with exec.CommandContext, its default Cancel hard-kills +// the child (SIGKILL) as soon as the context is done — which is exactly what +// happens on Ctrl-C, since lstk cancels its root context on SIGINT/SIGTERM. Run +// disarms that: a cancelled context no longer kills the child, so the tool +// terminates from the signal it receives and lstk waits for it to finish its own +// shutdown. Returning os.ErrProcessDone from Cancel both suppresses the kill and +// avoids injecting context.Canceled into the wait result, so the tool's real +// exit code is preserved. +// +// In an interactive terminal the child already receives the signal via the +// controlling terminal's foreground process group, so Run does not forward +// there: a second, near-simultaneous SIGINT makes tools like terraform abort +// immediately instead of cleaning up. Forwarding therefore covers only the +// non-interactive case (e.g. a supervisor sending kill to lstk alone), mirroring +// the model documented in npm/launcher.js. +func Run(cmd *exec.Cmd) error { + // Disarm exec.CommandContext's SIGKILL-on-cancel (no-op for a plain + // exec.Command, whose Cancel is nil). + if cmd.Cancel != nil { + cmd.Cancel = func() error { return os.ErrProcessDone } + } + + if err := cmd.Start(); err != nil { + return err + } + + if !terminal.IsTerminal(os.Stdin) { + sigCh := make(chan os.Signal, 1) + signal.Notify(sigCh, forwardedSignals...) + defer signal.Stop(sigCh) + + done := make(chan struct{}) + defer close(done) + go func() { + for { + select { + case sig := <-sigCh: + _ = cmd.Process.Signal(sig) + case <-done: + return + } + } + }() + } + + return cmd.Wait() +} diff --git a/internal/proc/run_test.go b/internal/proc/run_test.go new file mode 100644 index 00000000..005e7e34 --- /dev/null +++ b/internal/proc/run_test.go @@ -0,0 +1,78 @@ +package proc + +import ( + "context" + "errors" + "os/exec" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestRunPropagatesExitCode verifies a wrapped tool's non-zero exit surfaces as +// an *exec.ExitError, matching cmd.Run's behaviour. +func TestRunPropagatesExitCode(t *testing.T) { + t.Parallel() + + cmd := exec.Command("sh", "-c", "exit 7") + err := Run(cmd) + + var exitErr *exec.ExitError + require.ErrorAs(t, err, &exitErr) + assert.Equal(t, 7, exitErr.ExitCode()) +} + +// TestRunSucceeds verifies a zero-exit tool returns nil. +func TestRunSucceeds(t *testing.T) { + t.Parallel() + + require.NoError(t, Run(exec.Command("sh", "-c", "exit 0"))) +} + +// TestRunDoesNotKillOnContextCancel is the regression test for the reported bug: +// a cancelled context must no longer SIGKILL the wrapped tool. The child sleeps +// briefly then exits 0 on its own; if Run left exec.CommandContext's default +// Cancel in place, the cancellation would kill the child and Run would report a +// non-nil error (a signal-terminated ExitError or context.Canceled) instead of +// the clean exit. +func TestRunDoesNotKillOnContextCancel(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + cmd := exec.CommandContext(ctx, "sh", "-c", "sleep 0.3; exit 0") + + go func() { + time.Sleep(50 * time.Millisecond) + cancel() + }() + + err := Run(cmd) + require.NoError(t, err) + assert.True(t, cmd.ProcessState.Success(), "child should have exited on its own, not been killed") +} + +// TestRunPreservesExitCodeOnContextCancel ensures the tool's real exit code +// survives a mid-run cancellation rather than being clobbered by context.Canceled. +func TestRunPreservesExitCodeOnContextCancel(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + cmd := exec.CommandContext(ctx, "sh", "-c", "sleep 0.3; exit 5") + + go func() { + time.Sleep(50 * time.Millisecond) + cancel() + }() + + err := Run(cmd) + var exitErr *exec.ExitError + require.ErrorAs(t, err, &exitErr) + assert.Equal(t, 5, exitErr.ExitCode()) + assert.False(t, errors.Is(err, context.Canceled)) +} From c3b7b9c3a30bcd6ac9993832ac3129b7f346aa8c Mon Sep 17 00:00:00 2001 From: George Tsiolis Date: Thu, 16 Jul 2026 18:24:33 +0300 Subject: [PATCH 2/2] fix: forward SIGTERM unconditionally and suppress SIGINT on any terminal 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 `, `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 --- CLAUDE.md | 2 +- internal/proc/run.go | 71 ++++--- internal/proc/run_test.go | 30 +++ test/integration/signal_forwarding_test.go | 177 ++++++++++++++++++ .../test-samples/extensions/lstk-ref/main.go | 34 ++++ 5 files changed, 290 insertions(+), 24 deletions(-) create mode 100644 test/integration/signal_forwarding_test.go diff --git a/CLAUDE.md b/CLAUDE.md index 1e026ee9..ff82b89e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -138,7 +138,7 @@ See [extensions-authoring.md](docs/extensions-authoring.md) for the author-facin # Signal Forwarding to Wrapped Tools -Wrapped external tools (`aws`, `terraform`, `cdk`, `sam`, `az`, and extensions) are run through `proc.Run(cmd)` (in `internal/proc/run.go`) rather than `cmd.Run()`. These execs are created with `exec.CommandContext` using lstk's root context, which is cancelled on `SIGINT`/`SIGTERM`; `exec.CommandContext`'s default `Cancel` would then SIGKILL the child immediately, denying tools like `terraform apply` the chance to clean up (e.g. release the state lock). `proc.Run` disarms that (its `Cancel` returns `os.ErrProcessDone`, which both suppresses the kill and avoids injecting `context.Canceled` into the wait result, preserving the tool's real exit code) and instead lets the tool terminate from the signal it receives, waiting for it to finish its own shutdown. In an interactive terminal the child already receives the signal via the controlling terminal's foreground process group, so `proc.Run` forwards `SIGINT`/`SIGTERM` only when stdin is not a terminal (a second near-simultaneous signal would make tools like terraform abort immediately). This mirrors the model in `npm/launcher.js`: the TTY covers interactive use, forwarding covers the non-interactive case. Short internal captured-output execs (version checks, schema discovery, backend provisioning) still use `cmd.Run()` directly. +Wrapped external tools (`aws`, `terraform`, `cdk`, `sam`, `az`, and extensions) are run through `proc.Run(cmd)` (in `internal/proc/run.go`) rather than `cmd.Run()`. These execs are created with `exec.CommandContext` using lstk's root context, which is cancelled on `SIGINT`/`SIGTERM`; `exec.CommandContext`'s default `Cancel` would then SIGKILL the child immediately, denying tools like `terraform apply` the chance to clean up (e.g. release the state lock). `proc.Run` disarms that (its `Cancel` returns `os.ErrProcessDone`, which both suppresses the kill and avoids injecting `context.Canceled` into the wait result, preserving the tool's real exit code) and instead lets the tool terminate from the signal it receives, waiting for it to finish its own shutdown. Forwarding is per-signal: `SIGTERM` is always relayed to the child (a terminal never generates it, so `kill ` / `timeout` / an IDE stop button would otherwise never reach the tool), while `SIGINT` is relayed only when none of lstk's std streams 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 immediately instead of cleaning up. The any-stream check matters: with only stdin redirected (`yes | lstk terraform apply`) lstk still sits in the terminal's foreground process group. This differs from `npm/launcher.js`, which forwards unconditionally — safe there because its child is lstk itself, which tolerates duplicate signals; wrapped tools do not. Short internal captured-output execs (version checks, schema discovery, backend provisioning) still use `cmd.Run()` directly. End-to-end signal tests live in `test/integration/signal_forwarding_test.go`, backed by the reference extension's `signal-wait` mode. # Snapshots diff --git a/internal/proc/run.go b/internal/proc/run.go index 01aa1134..db19a5ba 100644 --- a/internal/proc/run.go +++ b/internal/proc/run.go @@ -17,9 +17,30 @@ import ( // abruptly. var forwardedSignals = []os.Signal{os.Interrupt, syscall.SIGTERM} +// shouldForward decides whether a received signal is relayed to the child. +// SIGTERM is never generated by a terminal, so it is always forwarded — without +// that, `kill ` or `timeout 30 lstk terraform apply` would never reach +// the wrapped tool. SIGINT is terminal-generated (Ctrl-C) and already delivered +// to the child via the controlling terminal's foreground process group, so +// forwarding it while attached to a terminal would double-signal the tool — and +// a second, near-simultaneous SIGINT makes tools like terraform abort +// immediately instead of cleaning up. +func shouldForward(sig os.Signal, attachedToTerminal bool) bool { + return sig != os.Interrupt || !attachedToTerminal +} + +// attachedToTerminal reports whether any of lstk's std streams is a terminal — +// i.e. whether a terminal's foreground process group will deliver Ctrl-C to the +// wrapped child directly. Checking stdin alone is not enough: with stdin +// redirected (`yes | lstk terraform apply`) lstk still runs in the terminal's +// foreground process group. +func attachedToTerminal() bool { + return terminal.IsTerminal(os.Stdin) || terminal.IsTerminal(os.Stdout) || terminal.IsTerminal(os.Stderr) +} + // Run starts cmd, waits for it to exit, and returns the same error cmd.Run -// would. It ensures SIGINT/SIGTERM reach the wrapped tool so it can clean up -// instead of being SIGKILLed the instant lstk's context is cancelled. +// would. It ensures SIGINT/SIGTERM reach the wrapped tool exactly once so it can +// clean up instead of being SIGKILLed the instant lstk's context is cancelled. // // When cmd was created with exec.CommandContext, its default Cancel hard-kills // the child (SIGKILL) as soon as the context is done — which is exactly what @@ -30,12 +51,11 @@ var forwardedSignals = []os.Signal{os.Interrupt, syscall.SIGTERM} // avoids injecting context.Canceled into the wait result, so the tool's real // exit code is preserved. // -// In an interactive terminal the child already receives the signal via the -// controlling terminal's foreground process group, so Run does not forward -// there: a second, near-simultaneous SIGINT makes tools like terraform abort -// immediately instead of cleaning up. Forwarding therefore covers only the -// non-interactive case (e.g. a supervisor sending kill to lstk alone), mirroring -// the model documented in npm/launcher.js. +// Received signals are relayed per shouldForward: SIGTERM always, SIGINT only +// when lstk is not attached to a terminal (an attached terminal already +// delivers Ctrl-C to the child via the foreground process group). npm/launcher.js +// forwards unconditionally instead — safe there because its child is lstk, +// which tolerates duplicate signals; wrapped tools like terraform do not. func Run(cmd *exec.Cmd) error { // Disarm exec.CommandContext's SIGKILL-on-cancel (no-op for a plain // exec.Command, whose Cancel is nil). @@ -43,28 +63,33 @@ func Run(cmd *exec.Cmd) error { cmd.Cancel = func() error { return os.ErrProcessDone } } + attached := attachedToTerminal() + + // Register before Start so a signal landing during startup is queued and + // relayed once the child is running, not silently swallowed by lstk's own + // root handler. + sigCh := make(chan os.Signal, len(forwardedSignals)) + signal.Notify(sigCh, forwardedSignals...) + defer signal.Stop(sigCh) + if err := cmd.Start(); err != nil { return err } - if !terminal.IsTerminal(os.Stdin) { - sigCh := make(chan os.Signal, 1) - signal.Notify(sigCh, forwardedSignals...) - defer signal.Stop(sigCh) - - done := make(chan struct{}) - defer close(done) - go func() { - for { - select { - case sig := <-sigCh: + done := make(chan struct{}) + defer close(done) + go func() { + for { + select { + case sig := <-sigCh: + if shouldForward(sig, attached) { _ = cmd.Process.Signal(sig) - case <-done: - return } + case <-done: + return } - }() - } + } + }() return cmd.Wait() } diff --git a/internal/proc/run_test.go b/internal/proc/run_test.go index 005e7e34..7891d132 100644 --- a/internal/proc/run_test.go +++ b/internal/proc/run_test.go @@ -3,7 +3,10 @@ package proc import ( "context" "errors" + "os" "os/exec" + "runtime" + "syscall" "testing" "time" @@ -11,10 +14,34 @@ import ( "github.com/stretchr/testify/require" ) +// requireUnixShell skips tests that exec `sh`, which is unavailable on Windows +// (CI runs unit tests on Linux only, but developers run them anywhere). +func requireUnixShell(t *testing.T) { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("test execs `sh`, unavailable on Windows") + } +} + +// TestShouldForward pins the forwarding policy: SIGTERM is never generated by a +// terminal, so it is always forwarded; SIGINT reaches the child via the +// controlling terminal's foreground process group when lstk is attached to a +// terminal, so forwarding there would double-signal the tool (terraform aborts +// on a second interrupt) and is suppressed. +func TestShouldForward(t *testing.T) { + t.Parallel() + + assert.True(t, shouldForward(syscall.SIGTERM, true), "SIGTERM must forward even when attached to a terminal") + assert.True(t, shouldForward(syscall.SIGTERM, false), "SIGTERM must forward when detached") + assert.False(t, shouldForward(os.Interrupt, true), "SIGINT must not forward when attached to a terminal") + assert.True(t, shouldForward(os.Interrupt, false), "SIGINT must forward when detached") +} + // TestRunPropagatesExitCode verifies a wrapped tool's non-zero exit surfaces as // an *exec.ExitError, matching cmd.Run's behaviour. func TestRunPropagatesExitCode(t *testing.T) { t.Parallel() + requireUnixShell(t) cmd := exec.Command("sh", "-c", "exit 7") err := Run(cmd) @@ -27,6 +54,7 @@ func TestRunPropagatesExitCode(t *testing.T) { // TestRunSucceeds verifies a zero-exit tool returns nil. func TestRunSucceeds(t *testing.T) { t.Parallel() + requireUnixShell(t) require.NoError(t, Run(exec.Command("sh", "-c", "exit 0"))) } @@ -39,6 +67,7 @@ func TestRunSucceeds(t *testing.T) { // the clean exit. func TestRunDoesNotKillOnContextCancel(t *testing.T) { t.Parallel() + requireUnixShell(t) ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -59,6 +88,7 @@ func TestRunDoesNotKillOnContextCancel(t *testing.T) { // survives a mid-run cancellation rather than being clobbered by context.Canceled. func TestRunPreservesExitCodeOnContextCancel(t *testing.T) { t.Parallel() + requireUnixShell(t) ctx, cancel := context.WithCancel(context.Background()) defer cancel() diff --git a/test/integration/signal_forwarding_test.go b/test/integration/signal_forwarding_test.go new file mode 100644 index 00000000..c641024d --- /dev/null +++ b/test/integration/signal_forwarding_test.go @@ -0,0 +1,177 @@ +//go:build !windows + +package integration_test + +import ( + "io" + "os" + "os/exec" + "path/filepath" + "strings" + "syscall" + "testing" + "time" + + "github.com/creack/pty" + "github.com/stretchr/testify/require" +) + +// These tests cover proc.Run's signal handling for wrapped tools end to end, +// using the reference extension's `signal-wait` mode: it exits 40 + the number +// of SIGINT/SIGTERM it received, so 41 proves exactly-once delivery, 40 proves +// the signal never arrived, and 42 proves a double signal (the terraform +// "two interrupts = abort immediately" hazard). + +// startSignalWait starts `lstk sig signal-wait` with an isolated env and the +// reference extension on PATH, wiring stdio as configured by wire before start. +func startSignalWait(t *testing.T, wire func(*exec.Cmd)) *exec.Cmd { + t.Helper() + extDir := t.TempDir() + installExtension(t, extDir, "sig") + + binPath, err := filepath.Abs(binaryPath()) + require.NoError(t, err) + + cmd := exec.CommandContext(testContext(t), binPath, "sig", "signal-wait") + cmd.Dir = t.TempDir() + cmd.Env = envWithPath(t.TempDir(), extDir) + wire(cmd) + return cmd +} + +// waitForMarker polls out until the extension's readiness marker appears, so a +// signal is never sent before lstk has started the child (and armed its +// forwarding). +func waitForMarker(t *testing.T, out *syncBuffer) { + t.Helper() + deadline := time.Now().Add(30 * time.Second) + for time.Now().Before(deadline) { + if strings.Contains(out.String(), "SIGNAL_WAIT_READY") { + return + } + time.Sleep(10 * time.Millisecond) + } + t.Fatalf("extension never reported readiness; output so far:\n%s", out.String()) +} + +// drainPTY copies ptmx into a syncBuffer until the child exits (EIO on Linux / +// EOF on macOS), returning the buffer and a channel closed when copying ends. +func drainPTY(ptmx *os.File) (*syncBuffer, chan struct{}) { + out := &syncBuffer{} + done := make(chan struct{}) + go func() { + _, _ = io.Copy(out, ptmx) + close(done) + }() + return out, done +} + +// TestWrappedToolReceivesForwardedSignalNonInteractive is the core DEVX-1000 +// scenario: with no terminal attached (CI, supervisors), a SIGINT/SIGTERM sent +// to the lstk process alone must be forwarded to the wrapped tool — lstk itself +// swallows the signal (root context cancel) and, without forwarding, the tool +// would never learn it should shut down. +func TestWrappedToolReceivesForwardedSignalNonInteractive(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + sig os.Signal + }{ + {name: "SIGTERM", sig: syscall.SIGTERM}, + {name: "SIGINT", sig: os.Interrupt}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + out := &syncBuffer{} + cmd := startSignalWait(t, func(c *exec.Cmd) { + c.Stdout = out + c.Stderr = out + }) + require.NoError(t, cmd.Start()) + + waitForMarker(t, out) + require.NoError(t, cmd.Process.Signal(tc.sig)) + + err := cmd.Wait() + requireExitCode(t, 41, err) + }) + } +} + +// TestWrappedToolReceivesForwardedSIGTERMInTerminal: SIGTERM is never generated +// by a terminal, so even when lstk runs interactively a `kill ` (IDE +// stop button, `timeout 30 lstk ...`) must still be forwarded — the terminal's +// process group will not deliver it for us. +func TestWrappedToolReceivesForwardedSIGTERMInTerminal(t *testing.T) { + t.Parallel() + var ptmx *os.File + cmd := startSignalWait(t, func(c *exec.Cmd) {}) + ptmx, err := pty.Start(cmd) + require.NoError(t, err) + defer func() { _ = ptmx.Close() }() + + out, copied := drainPTY(ptmx) + waitForMarker(t, out) + require.NoError(t, cmd.Process.Signal(syscall.SIGTERM)) + + err = cmd.Wait() + <-copied + requireExitCode(t, 41, err) +} + +// TestWrappedToolSingleSIGINTOnCtrlCInTerminal: interactively, Ctrl-C reaches +// the wrapped tool via the terminal's foreground process group; lstk must NOT +// forward a duplicate — a second, near-simultaneous SIGINT makes tools like +// terraform abort immediately instead of cleaning up. +func TestWrappedToolSingleSIGINTOnCtrlCInTerminal(t *testing.T) { + t.Parallel() + cmd := startSignalWait(t, func(c *exec.Cmd) {}) + ptmx, err := pty.Start(cmd) + require.NoError(t, err) + defer func() { _ = ptmx.Close() }() + + out, copied := drainPTY(ptmx) + waitForMarker(t, out) + _, err = ptmx.Write([]byte{0x03}) // Ctrl-C → SIGINT to the foreground process group + require.NoError(t, err) + + err = cmd.Wait() + <-copied + requireExitCode(t, 41, err) +} + +// TestWrappedToolSingleSIGINTOnCtrlCWithRedirectedStdin guards the piped-stdin +// regression: `yes | lstk terraform apply` (stdin redirected, stdout/stderr +// still the terminal) keeps lstk in the terminal's foreground process group, so +// Ctrl-C already reaches the wrapped tool via the group. Gating suppression on +// stdin alone would arm forwarding here and double-signal the tool. +func TestWrappedToolSingleSIGINTOnCtrlCWithRedirectedStdin(t *testing.T) { + t.Parallel() + ptmx, tty, err := pty.Open() + require.NoError(t, err) + defer func() { _ = ptmx.Close() }() + + devNull, err := os.Open(os.DevNull) + require.NoError(t, err) + defer func() { _ = devNull.Close() }() + + cmd := startSignalWait(t, func(c *exec.Cmd) { + c.Stdin = devNull // stdin redirected: not a terminal + c.Stdout = tty // stdout/stderr: the terminal + c.Stderr = tty + // Make the PTY the controlling terminal so Ctrl-C on it signals the + // foreground process group, exactly like a real interactive session. + c.SysProcAttr = &syscall.SysProcAttr{Setsid: true, Setctty: true, Ctty: 1} + }) + require.NoError(t, cmd.Start()) + _ = tty.Close() // child holds its own copy of the slave side + + out, copied := drainPTY(ptmx) + waitForMarker(t, out) + _, err = ptmx.Write([]byte{0x03}) // Ctrl-C → SIGINT to the foreground process group + require.NoError(t, err) + + err = cmd.Wait() + <-copied + requireExitCode(t, 41, err) +} diff --git a/test/integration/test-samples/extensions/lstk-ref/main.go b/test/integration/test-samples/extensions/lstk-ref/main.go index 421daff9..8822346c 100644 --- a/test/integration/test-samples/extensions/lstk-ref/main.go +++ b/test/integration/test-samples/extensions/lstk-ref/main.go @@ -16,6 +16,9 @@ // (exit 13). A real extension would verify the token server-side // against the LocalStack platform — authorization must never rely on // lstk, which is open source and rebuildable. +// signal-wait Print a readiness marker, then exit 40 + the number of +// SIGINT/SIGTERM received (40 = none after a deadline, 41 = exactly +// one, 42 = a double signal). Backs lstk's signal-forwarding tests. // // The extension also self-enforces contract compatibility: it requires // LSTK_EXT_API_VERSION >= minAPIVersion and refuses to run otherwise, rather @@ -26,7 +29,10 @@ import ( "encoding/json" "fmt" "os" + "os/signal" "strconv" + "syscall" + "time" ) // minAPIVersion is the lowest contract version this extension supports. lstk @@ -88,11 +94,39 @@ func run(args []string) int { } fmt.Println("lstk-ref: authorized") return 0 + case "signal-wait": + return signalWait() default: return 0 } } +// signalWait reports signal delivery for lstk's signal-forwarding tests. It +// prints a readiness marker, then exits 40 + the number of SIGINT/SIGTERM +// received: 40 = none arrived before the deadline, 41 = exactly one, 42 = a +// double signal. After the first signal it lingers briefly so a +// near-simultaneous duplicate (the double-signal bug) is counted, not missed. +func signalWait() int { + sigCh := make(chan os.Signal, 4) + signal.Notify(sigCh, os.Interrupt, syscall.SIGTERM) + fmt.Println("SIGNAL_WAIT_READY") + + count := 0 + deadline := time.After(3 * time.Second) + for { + select { + case <-sigCh: + count++ + if count >= 2 { + return 40 + count + } + deadline = time.After(500 * time.Millisecond) + case <-deadline: + return 40 + count + } + } +} + // decodeContext decodes LSTK_EXT_CONTEXT, reporting a malformed payload but still // returning the zero context so tests can observe the absence of fields. func decodeContext() extContext {