diff --git a/CLAUDE.md b/CLAUDE.md index 955e58fe..dd2526b4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -159,6 +159,8 @@ lstk supports Git-style extensions: when `lstk ` is not a built-in command 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. +When lstk's stdout and stderr are both terminals, `lstk aws` runs the child via `proc.RunInPTY` (in `internal/proc/pty.go`) instead: the spinner path wraps the child's output in an `io.Writer`, which makes os/exec hand the child a pipe, and the frozen Python aws CLI then block-buffers stdout (8 KB, ignoring `PYTHONUNBUFFERED`) — streaming commands like `aws logs tail --follow` showed nothing until exit (DEVX-1026). The PTY makes the child see a terminal (line-buffered, colored output; stdout/stderr merged, no new session so Ctrl-C still reaches it via the process group) while the master side is copied through the spinner's `StopOnWriteWriter`. Falls back to plain `proc.Run` when no PTY can be allocated (Windows), and stays on pipes whenever stdout is redirected so pipelines never receive colors/CRLF. `lstk az` has the same wrapper pattern and needs the same treatment (pending follow-up). + # Snapshots diff --git a/cmd/aws.go b/cmd/aws.go index 299bcc5b..628bef1a 100644 --- a/cmd/aws.go +++ b/cmd/aws.go @@ -69,7 +69,7 @@ Examples: // --help/-h never contacts LocalStack, so it runs directly without // requiring Docker or a running emulator (DEVX-1002). if awscli.IsHelp(passthrough) { - return awscli.Exec(cmd.Context(), "", false, os.Stdout, os.Stderr, passthrough) + return awscli.Exec(cmd.Context(), "", false, false, os.Stdout, os.Stderr, passthrough) } rt, err := runtime.NewDockerRuntime(cfg.DockerHost) @@ -119,7 +119,13 @@ Examples: stderr = &terminal.StopOnWriteWriter{W: os.Stderr, Spinner: s} } - return awscli.Exec(cmd.Context(), "http://"+host, profileExists, stdout, stderr, passthrough) + // Only hand the aws CLI a PTY when both of lstk's output streams + // are the terminal: with stdout piped (`lstk aws ... | grep`) the + // child must keep seeing a pipe, or it would emit colors and CRLF + // into the pipeline. + usePTY := !cfg.NonInteractive && terminal.IsTerminal(os.Stdout) && terminal.IsTerminal(os.Stderr) + + return awscli.Exec(cmd.Context(), "http://"+host, profileExists, usePTY, stdout, stderr, passthrough) }, } } diff --git a/go.mod b/go.mod index 1a539f5c..4371bc20 100644 --- a/go.mod +++ b/go.mod @@ -11,6 +11,7 @@ require ( github.com/charmbracelet/lipgloss v1.1.0 github.com/charmbracelet/x/exp/teatest v0.0.0-20260216111343-536eb63c1f4c github.com/containerd/errdefs v1.0.0 + github.com/creack/pty v1.1.24 github.com/docker/go-units v0.5.0 github.com/google/uuid v1.6.0 github.com/hashicorp/hcl/v2 v2.24.0 diff --git a/go.sum b/go.sum index 852155ff..40700d9f 100644 --- a/go.sum +++ b/go.sum @@ -44,6 +44,8 @@ github.com/containerd/errdefs/pkg v0.3.0 h1:9IKJ06FvyNlexW690DXuQNx2KA2cUJXx151X github.com/containerd/errdefs/pkg v0.3.0/go.mod h1:NJw6s9HwNuRhnjJhM7pylWwMyAkmCQvQ4GpJHEqRLVk= github.com/cpuguy83/go-md2man/v2 v2.0.6 h1:XJtiaUW6dEEqVuZiMTn1ldk455QWwEIsMIJlo5vtkx0= github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g= +github.com/creack/pty v1.1.24 h1:bJrF4RRfyJnbTJqzRLHzcGaZK1NeM5kTC9jGgovnR1s= +github.com/creack/pty v1.1.24/go.mod h1:08sCNb52WyoAwi2QDyzUCTgcvVFhUzewun7wtTfvcwE= github.com/danieljoos/wincred v1.2.3 h1:v7dZC2x32Ut3nEfRH+vhoZGvN72+dQ/snVXo/vMFLdQ= github.com/danieljoos/wincred v1.2.3/go.mod h1:6qqX0WNrS4RzPZ1tnroDzq9kY3fu1KwE7MRLQK4X0bs= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= diff --git a/internal/awscli/exec.go b/internal/awscli/exec.go index 10f441a0..5deec687 100644 --- a/internal/awscli/exec.go +++ b/internal/awscli/exec.go @@ -44,7 +44,11 @@ func IsHelp(args []string) bool { return false } -func Exec(ctx context.Context, endpointURL string, useProfile bool, stdout, stderr io.Writer, args []string) error { +// Exec runs `aws ` against endpointURL. When usePTY is true (lstk's +// stdout and stderr are both terminals), the child's output goes through a +// pseudo-terminal merged into stdout — see proc.RunInPTY for why; otherwise +// stdout/stderr are wired as given. +func Exec(ctx context.Context, endpointURL string, useProfile, usePTY bool, stdout, stderr io.Writer, args []string) error { ctx, span := otel.Tracer("github.com/localstack/lstk/internal/awscli").Start(ctx, "aws cli") defer span.End() @@ -75,13 +79,22 @@ func Exec(ctx context.Context, endpointURL string, useProfile bool, stdout, stde cmd := exec.CommandContext(ctx, awsBin, cmdArgs...) cmd.Stdin = os.Stdin - cmd.Stdout = stdout - cmd.Stderr = stderr - if !useProfile { - cmd.Env = BuildEnv(os.Environ()) + cmd.Env = execEnv(os.Environ(), useProfile) + + var runErr error + started := false + if usePTY { + started, runErr = proc.RunInPTY(cmd, stdout) + } + if !started { + // No PTY requested or none could be allocated (e.g. on Windows): plain + // writer wiring, as before. + cmd.Stdout = stdout + cmd.Stderr = stderr + runErr = proc.Run(cmd) } - if err := proc.Run(cmd); err != nil { + if err := runErr; err != nil { var exitErr *exec.ExitError if errors.As(err, &exitErr) { span.SetAttributes(attribute.Int("aws.exit_code", exitErr.ExitCode())) @@ -95,6 +108,23 @@ func Exec(ctx context.Context, endpointURL string, useProfile bool, stdout, stde return nil } +// execEnv builds the child environment for the aws CLI. PYTHONUNBUFFERED stops +// a pip-installed (non-frozen) aws CLI from block-buffering stdout when it gets +// a pipe instead of a terminal (DEVX-1026); the official frozen v2 binary +// ignores it — that build's buffering is what the PTY in Exec fixes. (Python +// treats any non-empty value as enabled, so a user-set value is left alone.) +func execEnv(base []string, useProfile bool) []string { + var env []string + if useProfile { + env = make([]string, len(base), len(base)+1) + copy(env, base) + } else { + env = BuildEnv(base) + } + setIfAbsent(&env, "PYTHONUNBUFFERED", "1") + return env +} + func BuildEnv(base []string) []string { env := make([]string, len(base), len(base)+3) copy(env, base) diff --git a/internal/awscli/exec_test.go b/internal/awscli/exec_test.go index e6133f39..d8a9fc7c 100644 --- a/internal/awscli/exec_test.go +++ b/internal/awscli/exec_test.go @@ -63,3 +63,34 @@ func TestBuildEnvPartialOverride(t *testing.T) { assert.Contains(t, env, "AWS_SECRET_ACCESS_KEY=test") assert.Contains(t, env, "AWS_DEFAULT_REGION=us-east-1") } + +func TestExecEnvForcesUnbufferedPython(t *testing.T) { + base := []string{"PATH=/usr/bin"} + + withProfile := execEnv(base, true) + assert.Contains(t, withProfile, "PYTHONUNBUFFERED=1") + assert.NotContains(t, withProfile, "AWS_ACCESS_KEY_ID=test") + + withoutProfile := execEnv(base, false) + assert.Contains(t, withoutProfile, "PYTHONUNBUFFERED=1") + assert.Contains(t, withoutProfile, "AWS_ACCESS_KEY_ID=test") +} + +func TestExecEnvPreservesUserPythonUnbuffered(t *testing.T) { + base := []string{"PYTHONUNBUFFERED=x"} + env := execEnv(base, true) + + assert.Contains(t, env, "PYTHONUNBUFFERED=x") + assert.NotContains(t, env, "PYTHONUNBUFFERED=1") +} + +func TestExecEnvDoesNotMutateInput(t *testing.T) { + base := []string{"PATH=/usr/bin"} + original := make([]string, len(base)) + copy(original, base) + + execEnv(base, true) + execEnv(base, false) + + assert.Equal(t, original, base) +} diff --git a/internal/proc/pty.go b/internal/proc/pty.go new file mode 100644 index 00000000..56f680e0 --- /dev/null +++ b/internal/proc/pty.go @@ -0,0 +1,46 @@ +package proc + +import ( + "io" + "os" + "os/exec" + + "github.com/creack/pty" +) + +// RunInPTY runs cmd like Run, but gives the child a pseudo-terminal instead of +// a plain pipe for stdout/stderr, copying its output to out. Over a pipe, +// e.g. the Python aws CLI block-buffers stdout (ignoring PYTHONUNBUFFERED), +// holding back streaming commands like `logs tail --follow` until exit; a PTY +// makes it detect a terminal and stay line-buffered (DEVX-1026). +// +// No new session is created, so the child stays in lstk's process group and +// still gets Ctrl-C directly. started is false if no PTY could be allocated +// (e.g. Windows); the caller should fall back to Run. +func RunInPTY(cmd *exec.Cmd, out io.Writer) (started bool, err error) { + ptmx, tty, err := pty.Open() + if err != nil { + return false, err + } + defer func() { _ = ptmx.Close() }() + + if size, sizeErr := pty.GetsizeFull(os.Stdout); sizeErr == nil { + _ = pty.Setsize(ptmx, size) + } + + cmd.Stdout = tty + cmd.Stderr = tty + + copied := make(chan struct{}) + go func() { + _, _ = io.Copy(out, ptmx) + close(copied) + }() + + runErr := Run(cmd) + + // Wait for the copy goroutine to drain remaining output and hit EOF/EIO. + _ = tty.Close() + <-copied + return true, runErr +} diff --git a/internal/proc/pty_test.go b/internal/proc/pty_test.go new file mode 100644 index 00000000..06fbb2e3 --- /dev/null +++ b/internal/proc/pty_test.go @@ -0,0 +1,123 @@ +package proc + +import ( + "os/exec" + "runtime" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func skipWithoutPTY(t *testing.T) { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("PTY not supported on Windows") + } +} + +// syncWriter collects writes with timestamps so tests can assert on when +// output arrived, not just what arrived. +type syncWriter struct { + mu chan struct{} + chunks []timedChunk +} + +type timedChunk struct { + at time.Time + data string +} + +func newSyncWriter() *syncWriter { + w := &syncWriter{mu: make(chan struct{}, 1)} + w.mu <- struct{}{} + return w +} + +func (w *syncWriter) Write(p []byte) (int, error) { + <-w.mu + defer func() { w.mu <- struct{}{} }() + w.chunks = append(w.chunks, timedChunk{at: time.Now(), data: string(p)}) + return len(p), nil +} + +func (w *syncWriter) String() string { + <-w.mu + defer func() { w.mu <- struct{}{} }() + var b strings.Builder + for _, c := range w.chunks { + b.WriteString(c.data) + } + return b.String() +} + +func (w *syncWriter) firstWriteContaining(s string) (time.Time, bool) { + <-w.mu + defer func() { w.mu <- struct{}{} }() + var seen strings.Builder + for _, c := range w.chunks { + seen.WriteString(c.data) + if strings.Contains(seen.String(), s) { + return c.at, true + } + } + return time.Time{}, false +} + +func TestRunInPTYChildSeesTerminal(t *testing.T) { + skipWithoutPTY(t) + out := newSyncWriter() + + cmd := exec.Command("sh", "-c", "test -t 1 && test -t 2 && echo is-a-tty") + started, err := RunInPTY(cmd, out) + + require.True(t, started) + require.NoError(t, err) + assert.Contains(t, out.String(), "is-a-tty") +} + +func TestRunInPTYStreamsBeforeExit(t *testing.T) { + skipWithoutPTY(t) + out := newSyncWriter() + + start := time.Now() + cmd := exec.Command("sh", "-c", "echo early-line; sleep 1") + started, err := RunInPTY(cmd, out) + elapsed := time.Since(start) + + require.True(t, started) + require.NoError(t, err) + require.GreaterOrEqual(t, elapsed, time.Second, "sanity: child slept after writing") + at, ok := out.firstWriteContaining("early-line") + require.True(t, ok, "output missing early-line: %q", out.String()) + assert.Less(t, at.Sub(start), 900*time.Millisecond, + "output written before exit should reach the writer while the child still runs") +} + +func TestRunInPTYPropagatesExitCode(t *testing.T) { + skipWithoutPTY(t) + out := newSyncWriter() + + cmd := exec.Command("sh", "-c", "exit 7") + started, err := RunInPTY(cmd, out) + + require.True(t, started) + var exitErr *exec.ExitError + require.ErrorAs(t, err, &exitErr) + assert.Equal(t, 7, exitErr.ExitCode()) +} + +func TestRunInPTYMergesStderrIntoOut(t *testing.T) { + skipWithoutPTY(t) + out := newSyncWriter() + + cmd := exec.Command("sh", "-c", "echo to-stdout; echo to-stderr >&2") + started, err := RunInPTY(cmd, out) + + require.True(t, started) + require.NoError(t, err) + assert.Contains(t, out.String(), "to-stdout") + assert.Contains(t, out.String(), "to-stderr") +} diff --git a/test/integration/aws_e2e_test.go b/test/integration/aws_e2e_test.go new file mode 100644 index 00000000..b2c0b527 --- /dev/null +++ b/test/integration/aws_e2e_test.go @@ -0,0 +1,100 @@ +package integration_test + +import ( + "fmt" + "io" + "os/exec" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/creack/pty" + "github.com/localstack/lstk/test/integration/env" + "github.com/stretchr/testify/require" +) + +// End-to-end tests for `lstk aws` that exercise the real aws CLI against a real +// LocalStack container (see localstack_test.go for the shared bring-up helpers) +// — unlike aws_cmd_test.go, which uses a fake aws binary and an alpine stand-in. +// They are gated on Docker + an aws binary + an auth token (CI installs the +// binaries and provides the token; otherwise they skip). + +func requireRealAWSCLI(t *testing.T) { + t.Helper() + if _, err := exec.LookPath("aws"); err != nil { + t.Skip("aws binary not found on PATH") + } +} + +// TestAWSLogsTailFollowStreamsInPTY reproduces DEVX-1026: `lstk aws logs tail +// --follow` run on a terminal must stream log events as they are read, not sit +// silent until the process exits. lstk's interactive path wraps the child's +// stdout in a spinner-stopping io.Writer, which makes os/exec hand the aws CLI +// a pipe instead of the TTY; the Python-based aws CLI then block-buffers stdout +// (8 KB), so tail output only ever appeared on exit. The PTY here makes lstk +// take that interactive path; the test asserts an already-written event shows +// up while the follow process is still running. +func TestAWSLogsTailFollowStreamsInPTY(t *testing.T) { + requireDocker(t) + requireRealAWSCLI(t) + token := requireAuthToken(t) + cleanup() + t.Cleanup(cleanup) + ctx := testContext(t) + startRealLocalStack(t, ctx, token) + + e := env.With(env.DisableEvents, "1").With(env.Home, t.TempDir()) + + const logGroup = "/lstk-e2e/tail-follow" + const marker = "hello-tail-follow" + + _, stderr, err := runLstk(t, ctx, "", e, "aws", "logs", "create-log-group", "--log-group-name", logGroup) + require.NoError(t, err, "create-log-group failed: %s", stderr) + _, stderr, err = runLstk(t, ctx, "", e, "aws", "logs", "create-log-stream", "--log-group-name", logGroup, "--log-stream-name", "s1") + require.NoError(t, err, "create-log-stream failed: %s", stderr) + events := fmt.Sprintf("timestamp=%d,message=%s", time.Now().UnixMilli(), marker) + _, stderr, err = runLstk(t, ctx, "", e, "aws", "logs", "put-log-events", "--log-group-name", logGroup, "--log-stream-name", "s1", "--log-events", events) + require.NoError(t, err, "put-log-events failed: %s", stderr) + + binPath, err := filepath.Abs(binaryPath()) + require.NoError(t, err) + cmd := exec.CommandContext(ctx, binPath, "aws", "logs", "tail", logGroup, "--follow", "--since", "1h") + cmd.Env = e + + ptmx, err := pty.Start(cmd) + require.NoError(t, err, "failed to start command in PTY") + + out := &syncBuffer{} + go func() { _, _ = io.Copy(out, ptmx) }() + + // The event already exists, so a streaming tail prints it on its first + // poll — within a couple of seconds. Poll well past that so a slow first + // CloudWatch call can't flake the test; the buggy behavior holds the line + // in the aws CLI's stdout buffer until exit, which this deadline catches. + deadline := time.Now().Add(30 * time.Second) + seen := false + for time.Now().Before(deadline) { + if strings.Contains(out.String(), marker) { + seen = true + break + } + time.Sleep(200 * time.Millisecond) + } + + // Ctrl-C via the PTY reaches the whole foreground process group (lstk and + // the aws child), matching what a user does to stop a follow. + _, _ = ptmx.Write([]byte{3}) + waitErr := make(chan error, 1) + go func() { waitErr <- cmd.Wait() }() + select { + case <-waitErr: + case <-time.After(10 * time.Second): + _ = cmd.Process.Kill() + <-waitErr + } + _ = ptmx.Close() + + require.True(t, seen, + "tail --follow produced no output while running; event appeared only after exit (DEVX-1026). Output after exit:\n%s", out.String()) +}