From 47b1586f03b9ae63e870f500ee254834b0186c35 Mon Sep 17 00:00:00 2001 From: George Tsiolis <120486+gtsiolis@users.noreply.github.com> Date: Mon, 29 Jun 2026 21:46:14 +0000 Subject: [PATCH 1/3] fix: run containers with AutoRemove so they do not linger after exit Containers were created without --rm (AutoRemove=false), leaving stopped containers in Exited state indefinitely on mixed hosts. Set AutoRemove on the HostConfig to match the Python CLI lifecycle. Generated with [Linear](https://linear.app/localstack/issue/DEVX-943/run-cli-containers-with-rm-to-avoid-lingering-exited-containers#agent-session-0a1c1991) Co-authored-by: linear-code[bot] <222613912+linear-code[bot]@users.noreply.github.com> --- internal/runtime/docker.go | 1 + test/integration/start_test.go | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/internal/runtime/docker.go b/internal/runtime/docker.go index b0c96571..7fe2f337 100644 --- a/internal/runtime/docker.go +++ b/internal/runtime/docker.go @@ -278,6 +278,7 @@ func (d *DockerRuntime) Start(ctx context.Context, config ContainerConfig) (stri HostConfig: &container.HostConfig{ PortBindings: portBindings, Binds: binds, + AutoRemove: true, }, Name: config.Name, }) diff --git a/test/integration/start_test.go b/test/integration/start_test.go index c4056399..c07b66e4 100644 --- a/test/integration/start_test.go +++ b/test/integration/start_test.go @@ -481,6 +481,11 @@ func TestStartCommandSetsUpContainerCorrectly(t *testing.T) { "expected volume bind mount to /var/lib/localstack, got: %v", inspect.Container.HostConfig.Binds) }) + t.Run("auto remove", func(t *testing.T) { + assert.True(t, inspect.Container.HostConfig.AutoRemove, + "expected container to be created with AutoRemove (--rm) so it does not linger after exit") + }) + t.Run("http health endpoint", func(t *testing.T) { resp, err := http.Get("http://localhost.localstack.cloud:4566/_localstack/health") require.NoError(t, err) From 0bfa4ab64329392a4d0ae58c29385e6c1c8de639 Mon Sep 17 00:00:00 2001 From: George Tsiolis Date: Tue, 30 Jun 2026 12:41:38 +0300 Subject: [PATCH 2/3] fix: handle AutoRemove container-removal race and preserve startup logs Running containers with --rm (AutoRemove) introduced two regressions that broke `lstk restart` / start-over-existing and erased crash diagnostics: 1. Removal race. With --rm, stopping a container kicks off an asynchronous removal. The start flow's "remove existing container before create" (pullImages -> rt.Remove) then hit "removal of container ... is already in progress" (a conflict, not not-found) and failed; even when tolerated, the name could still be taken when ContainerCreate ran next. DockerRuntime.Remove now tolerates the conflict and waits until the container is actually gone before returning, so the subsequent create does not race the in-flight removal. This fixes TestRestartCommandSucceeds, TestRestartCommandPersistFlagSetsPersistenceEnv, and TestSAME2EDeployCustomAccount. 2. Lost startup logs. awaitStartup fetched the container's logs to explain a crash, but --rm removed the container before the poll detected the exit, so the fetch returned nothing. The start flow now follows the container's logs into a bounded buffer while it runs, so the tail survives auto-removal and still explains a failed start. Also make the custom-image integration test's post-hoc container inspect best-effort, since the stub container is auto-removed the instant it exits; the "Using local image" output remains the authoritative signal. --- internal/container/logtail.go | 37 ++++++++++++++++++++ internal/container/logtail_test.go | 54 ++++++++++++++++++++++++++++++ internal/container/start.go | 49 +++++++++++++++++++++++---- internal/container/start_test.go | 2 ++ internal/runtime/docker.go | 32 +++++++++++++++++- test/integration/start_test.go | 12 ++++--- 6 files changed, 174 insertions(+), 12 deletions(-) create mode 100644 internal/container/logtail.go create mode 100644 internal/container/logtail_test.go diff --git a/internal/container/logtail.go b/internal/container/logtail.go new file mode 100644 index 00000000..d436c1b4 --- /dev/null +++ b/internal/container/logtail.go @@ -0,0 +1,37 @@ +package container + +import "sync" + +// logTail is a thread-safe, byte-bounded buffer that retains the most recent +// maxBytes written to it. It captures a container's startup logs while the +// container runs so they survive the container being auto-removed (--rm) the +// instant it exits — otherwise a crash during startup would leave no diagnostics. +type logTail struct { + mu sync.Mutex + buf []byte + maxBytes int +} + +func newLogTail(maxBytes int) *logTail { + return &logTail{maxBytes: maxBytes} +} + +// Write appends p, keeping only the most recent maxBytes. It never errors, so a +// follow-logs goroutine writing into it cannot fail the capture. +func (l *logTail) Write(p []byte) (int, error) { + l.mu.Lock() + defer l.mu.Unlock() + l.buf = append(l.buf, p...) + if len(l.buf) > l.maxBytes { + trimmed := make([]byte, l.maxBytes) + copy(trimmed, l.buf[len(l.buf)-l.maxBytes:]) + l.buf = trimmed + } + return len(p), nil +} + +func (l *logTail) String() string { + l.mu.Lock() + defer l.mu.Unlock() + return string(l.buf) +} diff --git a/internal/container/logtail_test.go b/internal/container/logtail_test.go new file mode 100644 index 00000000..da1bc27c --- /dev/null +++ b/internal/container/logtail_test.go @@ -0,0 +1,54 @@ +package container + +import ( + "strings" + "sync" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestLogTailRetainsContentUnderLimit(t *testing.T) { + lt := newLogTail(1024) + n, err := lt.Write([]byte("hello ")) + assert.NoError(t, err) + assert.Equal(t, 6, n) + _, _ = lt.Write([]byte("world")) + assert.Equal(t, "hello world", lt.String()) +} + +func TestLogTailKeepsOnlyMostRecentBytes(t *testing.T) { + lt := newLogTail(5) + _, _ = lt.Write([]byte("abc")) + _, _ = lt.Write([]byte("defgh")) + // Only the last 5 bytes are retained. + assert.Equal(t, "defgh", lt.String()) + + _, _ = lt.Write([]byte("XY")) + assert.Equal(t, "fghXY", lt.String()) +} + +func TestLogTailWriteReportsFullLength(t *testing.T) { + // Write must report len(p) even when the tail is trimmed, so an io.Writer + // consumer (stdcopy) does not treat it as a short write. + lt := newLogTail(4) + n, err := lt.Write([]byte("abcdefgh")) + assert.NoError(t, err) + assert.Equal(t, 8, n) + assert.Equal(t, "efgh", lt.String()) +} + +func TestLogTailConcurrentWritesAreSafe(t *testing.T) { + lt := newLogTail(64) + var wg sync.WaitGroup + for i := 0; i < 50; i++ { + wg.Add(1) + go func() { + defer wg.Done() + _, _ = lt.Write([]byte(strings.Repeat("x", 8))) + _ = lt.String() + }() + } + wg.Wait() + assert.LessOrEqual(t, len(lt.String()), 64) +} diff --git a/internal/container/start.go b/internal/container/start.go index fb66d687..9c044176 100644 --- a/internal/container/start.go +++ b/internal/container/start.go @@ -350,8 +350,9 @@ func tipsForType(t config.EmulatorType) []string { func pullImages(ctx context.Context, rt runtime.Runtime, sink output.Sink, tel *telemetry.Client, containers []runtime.ContainerConfig, interactive bool) (map[string]bool, error) { pulled := make(map[string]bool, len(containers)) for _, c := range containers { - // Remove any existing stopped container with the same name - if err := rt.Remove(ctx, c.Name); err != nil && !errdefs.IsNotFound(err) { + // Remove any existing container with the same name. rt.Remove tolerates the + // container being absent or mid auto-removal (--rm) and waits until it is gone. + if err := rt.Remove(ctx, c.Name); err != nil { return nil, fmt.Errorf("failed to remove existing container %s: %w", c.Name, err) } @@ -529,8 +530,28 @@ func startContainers(ctx context.Context, rt runtime.Runtime, sink output.Sink, return fmt.Errorf("failed to start LocalStack: %w", err) } + // Follow the container's logs into a bounded buffer from the moment it + // starts. With AutoRemove (--rm) the container is removed the instant it + // exits, so a post-hoc log fetch would race the removal; buffering as it + // runs keeps the startup logs available to explain a crash. + startupLogs := newLogTail(maxStartupLogBytes) + logCtx, stopLogTail := context.WithCancel(ctx) + logDone := make(chan struct{}) + go func() { + defer close(logDone) + _ = rt.StreamLogs(logCtx, containerID, startupLogs, true) + }() + healthURL := fmt.Sprintf("http://localhost:%s%s", c.Port, c.HealthPath) - if err := awaitStartup(ctx, rt, sink, containerID, "LocalStack", healthURL); err != nil { + err = awaitStartup(ctx, rt, sink, containerID, "LocalStack", healthURL, startupLogs) + // Stop following and let the goroutine return before continuing, so it does + // not outlive the start. Bounded so a slow stream teardown can't hang start. + stopLogTail() + select { + case <-logDone: + case <-time.After(2 * time.Second): + } + if err != nil { sink.Emit(output.SpinnerStop()) errCode := telemetry.ErrCodeStartFailed var licErr *licenseNotCoveredError @@ -783,12 +804,19 @@ func (e *licenseNotCoveredError) Error() string { return "license does not include this emulator" } +// maxStartupLogBytes bounds how much of a failing container's log tail is buffered +// to explain a crash during startup. +const maxStartupLogBytes = 64 * 1024 + // awaitStartup polls until one of two outcomes: // - Success: health endpoint returns 200 (license is valid, LocalStack is ready) // - Failure: container stops running (e.g., license activation failed), returns error with container logs // +// startupLogs holds the container's logs streamed while it ran, so they survive +// the container being auto-removed (--rm) on exit. +// // TODO: move to Runtime interface if other runtimes (k8s?) need native readiness probes -func awaitStartup(ctx context.Context, rt runtime.Runtime, sink output.Sink, containerID, name, healthURL string) error { +func awaitStartup(ctx context.Context, rt runtime.Runtime, sink output.Sink, containerID, name, healthURL string, startupLogs *logTail) error { client := &http.Client{Timeout: 2 * time.Second} for { @@ -797,11 +825,18 @@ func awaitStartup(ctx context.Context, rt runtime.Runtime, sink output.Sink, con return fmt.Errorf("failed to check container status: %w", err) } if !running { - logs, logsErr := rt.Logs(ctx, containerID, 20) - if logsErr == nil && strings.Contains(logs, "not covered by your license") { + // Prefer the logs streamed while the container ran: with --rm the + // container is already gone, so a direct fetch would return nothing. + logs := startupLogs.String() + if logs == "" { + if direct, derr := rt.Logs(ctx, containerID, 20); derr == nil { + logs = direct + } + } + if strings.Contains(logs, "not covered by your license") { return &licenseNotCoveredError{} } - if logsErr != nil || logs == "" { + if logs == "" { return fmt.Errorf("%s exited unexpectedly", name) } return fmt.Errorf("%s exited unexpectedly:\n%s", name, logs) diff --git a/internal/container/start_test.go b/internal/container/start_test.go index 33228e90..c862014d 100644 --- a/internal/container/start_test.go +++ b/internal/container/start_test.go @@ -586,6 +586,7 @@ func TestStartContainers_SnowflakeLicenseError(t *testing.T) { const containerID = "abc123" licenseLog := "⚠️ The Snowflake emulator is currently not covered by your license. ❄️" mockRT.EXPECT().Start(gomock.Any(), c).Return(containerID, nil) + mockRT.EXPECT().StreamLogs(gomock.Any(), containerID, gomock.Any(), true).Return(nil) mockRT.EXPECT().IsRunning(gomock.Any(), containerID).Return(false, nil) mockRT.EXPECT().Logs(gomock.Any(), containerID, 20).Return(licenseLog, nil) @@ -632,6 +633,7 @@ func TestStartContainers_AzureLicenseError(t *testing.T) { const containerID = "abc123" licenseLog := "The Azure emulator is currently not covered by your license." mockRT.EXPECT().Start(gomock.Any(), c).Return(containerID, nil) + mockRT.EXPECT().StreamLogs(gomock.Any(), containerID, gomock.Any(), true).Return(nil) mockRT.EXPECT().IsRunning(gomock.Any(), containerID).Return(false, nil) mockRT.EXPECT().Logs(gomock.Any(), containerID, 20).Return(licenseLog, nil) diff --git a/internal/runtime/docker.go b/internal/runtime/docker.go index 7fe2f337..0bd08c20 100644 --- a/internal/runtime/docker.go +++ b/internal/runtime/docker.go @@ -305,9 +305,39 @@ func (d *DockerRuntime) Stop(ctx context.Context, containerName string) error { return nil } +const ( + containerRemovalTimeout = 10 * time.Second + containerRemovalPollInterval = 100 * time.Millisecond +) + func (d *DockerRuntime) Remove(ctx context.Context, containerName string) error { _, err := d.client.ContainerRemove(ctx, containerName, client.ContainerRemoveOptions{}) - return err + // With AutoRemove (--rm) Docker may already be removing the container, so + // ContainerRemove can report it is already gone (not-found) or that removal is + // in progress (conflict). Both mean the container is on its way out. + if err != nil && !errdefs.IsNotFound(err) && !errdefs.IsConflict(err) { + return err + } + // Wait until the container is actually gone, so a subsequent create reusing the + // same name does not race the in-flight auto-removal ("name already in use"). + return d.waitContainerGone(ctx, containerName) +} + +// waitContainerGone blocks until no container named containerName exists, the +// context is cancelled, or containerRemovalTimeout elapses. +func (d *DockerRuntime) waitContainerGone(ctx context.Context, containerName string) error { + ctx, cancel := context.WithTimeout(ctx, containerRemovalTimeout) + defer cancel() + for { + if _, err := d.client.ContainerInspect(ctx, containerName, client.ContainerInspectOptions{}); errdefs.IsNotFound(err) { + return nil + } + select { + case <-ctx.Done(): + return fmt.Errorf("timed out waiting for container %s to be removed", containerName) + case <-time.After(containerRemovalPollInterval): + } + } } func (d *DockerRuntime) IsRunning(ctx context.Context, containerID string) (bool, error) { diff --git a/test/integration/start_test.go b/test/integration/start_test.go index c07b66e4..4dbaf2c7 100644 --- a/test/integration/start_test.go +++ b/test/integration/start_test.go @@ -1096,10 +1096,14 @@ image = %q "lstk must not run a pre-flight license check for a local image") // Started from the configured local image: lstk created the container using it. - inspect, err := dockerClient.ContainerInspect(ctx, wantContainer, client.ContainerInspectOptions{}) - require.NoError(t, err, "lstk should have created a container from the custom image") - assert.Equal(t, fullRef, inspect.Container.Config.Image, - "the container should be created from the configured custom image") + // With --rm the stub image's container is auto-removed the instant it exits, so + // it may already be gone — the "Using local image" output above is the + // authoritative signal. When the container is still present, additionally + // confirm it was created from the configured image. + if inspect, err := dockerClient.ContainerInspect(ctx, wantContainer, client.ContainerInspectOptions{}); err == nil { + assert.Equal(t, fullRef, inspect.Container.Config.Image, + "the container should be created from the configured custom image") + } } func cleanup() { From a37e5a4e453df100d8703d81e326a4bca71d94c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cristian=20Pallar=C3=A9s?= Date: Wed, 8 Jul 2026 11:59:16 +0200 Subject: [PATCH 3/3] fix: bound logTail.Write peak allocation to maxBytes append-then-trim let a single oversized write grow buf to len(existing)+len(p) before trimming back down, spiking peak memory well past maxBytes. Trim first instead, so buf never exceeds maxBytes. Co-Authored-By: Claude Sonnet 5 --- internal/container/logtail.go | 17 ++++++---- internal/container/logtail_test.go | 54 ++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 6 deletions(-) diff --git a/internal/container/logtail.go b/internal/container/logtail.go index d436c1b4..e233285e 100644 --- a/internal/container/logtail.go +++ b/internal/container/logtail.go @@ -17,16 +17,21 @@ func newLogTail(maxBytes int) *logTail { } // Write appends p, keeping only the most recent maxBytes. It never errors, so a -// follow-logs goroutine writing into it cannot fail the capture. +// follow-logs goroutine writing into it cannot fail the capture. It trims +// before appending so a single oversized write never grows buf past maxBytes. func (l *logTail) Write(p []byte) (int, error) { l.mu.Lock() defer l.mu.Unlock() - l.buf = append(l.buf, p...) - if len(l.buf) > l.maxBytes { - trimmed := make([]byte, l.maxBytes) - copy(trimmed, l.buf[len(l.buf)-l.maxBytes:]) - l.buf = trimmed + + if len(p) >= l.maxBytes { + l.buf = append(l.buf[:0], p[len(p)-l.maxBytes:]...) + return len(p), nil } + + if keep := l.maxBytes - len(p); len(l.buf) > keep { + l.buf = l.buf[len(l.buf)-keep:] + } + l.buf = append(l.buf, p...) return len(p), nil } diff --git a/internal/container/logtail_test.go b/internal/container/logtail_test.go index da1bc27c..e701d403 100644 --- a/internal/container/logtail_test.go +++ b/internal/container/logtail_test.go @@ -1,6 +1,7 @@ package container import ( + "runtime" "strings" "sync" "testing" @@ -38,6 +39,59 @@ func TestLogTailWriteReportsFullLength(t *testing.T) { assert.Equal(t, "efgh", lt.String()) } +func TestLogTailWritePeakAllocationBoundedByMaxBytes(t *testing.T) { + // Write must not allocate proportionally to len(p) on an oversized single + // write — the old "append-then-trim" implementation grows buf to + // len(existing)+len(p) before trimming back down, spiking peak memory well + // past maxBytes for one large write. Bytes allocated per call should stay + // close to maxBytes regardless of how large p is. + const maxBytes = 1024 + small := make([]byte, maxBytes/2) + huge := make([]byte, 200*maxBytes) + + allocsFor := func(p []byte) float64 { + lt := newLogTail(maxBytes) + return testing.AllocsPerRun(5, func() { + _, _ = lt.Write(p) + }) + } + + bytesAllocFor := func(p []byte) uint64 { + lt := newLogTail(maxBytes) + var before, after runtime.MemStats + runtime.ReadMemStats(&before) + _, _ = lt.Write(p) + runtime.ReadMemStats(&after) + return after.TotalAlloc - before.TotalAlloc + } + + t.Logf("allocs/run small=%v huge=%v", allocsFor(small), allocsFor(huge)) + + smallBytes := bytesAllocFor(small) + hugeBytes := bytesAllocFor(huge) + t.Logf("bytes allocated small=%d huge=%d (len(huge)=%d)", smallBytes, hugeBytes, len(huge)) + + // A single write must never need to allocate anywhere near len(p) bytes; + // it should stay within a small constant multiple of maxBytes. + assert.Lessf(t, hugeBytes, uint64(10*maxBytes), + "Write allocated %d bytes for a %d-byte input with maxBytes=%d — peak memory scales with input size instead of being bounded", + hugeBytes, len(huge), maxBytes) +} + +func TestLogTailHandlesRepeatedOversizedWrites(t *testing.T) { + lt := newLogTail(5) + + n, err := lt.Write([]byte("abcdefgh")) + assert.NoError(t, err) + assert.Equal(t, 8, n) + assert.Equal(t, "defgh", lt.String()) + + n, err = lt.Write([]byte("123456789")) + assert.NoError(t, err) + assert.Equal(t, 9, n) + assert.Equal(t, "56789", lt.String()) +} + func TestLogTailConcurrentWritesAreSafe(t *testing.T) { lt := newLogTail(64) var wg sync.WaitGroup