diff --git a/CLAUDE.md b/CLAUDE.md index a1c56867..ff82b89e 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. 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 `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..db19a5ba --- /dev/null +++ b/internal/proc/run.go @@ -0,0 +1,95 @@ +// 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} + +// 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 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 +// 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. +// +// 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). + if cmd.Cancel != nil { + 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 + } + + 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 + } + } + }() + + return cmd.Wait() +} diff --git a/internal/proc/run_test.go b/internal/proc/run_test.go new file mode 100644 index 00000000..7891d132 --- /dev/null +++ b/internal/proc/run_test.go @@ -0,0 +1,108 @@ +package proc + +import ( + "context" + "errors" + "os" + "os/exec" + "runtime" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "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) + + 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() + requireUnixShell(t) + + 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() + requireUnixShell(t) + + 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() + requireUnixShell(t) + + 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)) +} 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 {