Fix lstk start waits forever when the container fails during startup#390
Conversation
af37498 to
dd5f249
Compare
lstk start waits forever when the container fails during startup
tiurin
left a comment
There was a problem hiding this comment.
Will push a bit of a refactoring shortly.
|
|
||
| healthURL := fmt.Sprintf("http://localhost:%s%s", c.Port, c.HealthPath) | ||
| err = awaitStartup(ctx, rt, sink, containerID, "LocalStack", healthURL, startupLogs) | ||
| err = awaitStartup(ctx, rt, sink, containerID, "LocalStack", healthURL, exitCh, startupTimeout, interactive) |
There was a problem hiding this comment.
I'm not too happy with the function parameters count growing so much in awaitStatus and handleStartupTimeout/Faillure. Will refactor this to put dependencies that are constant for the whole Start invocation to a struct: rt, sink, tel, timeout, interactive. And then keep only per-call data as function agruments: containerID, healthURL, exitCh, c, err, logs
6ffb089 to
5092d07
Compare
anisaoshafi
left a comment
There was a problem hiding this comment.
Thank you for your contribution, Misha 👏🏼
small issue: before if CTRL+C called while awaiting it still did emit and send a telemetry event which is a sort of regression in this PR. Not a big deal, but if there's a reason for it, would be nice to assert it in some test so we recall the decision, or otherwise just try to track that event.
An interrupted start (Ctrl+C while awaiting readiness) deliberately renders no styled error, but the early return also dropped the start_error lifecycle telemetry event that was emitted before startup monitoring was introduced. Restore the emit (error code start_failed, matching the previous behavior) and add an integration test that interrupts a start mid-wait and asserts the event is delivered, guarding the decision from the PR #390 review. Co-Authored-By: Claude <noreply@anthropic.com>
lstk start could wait indefinitely at "Starting LocalStack..." when the container failed during startup (DEVX-758), and never reported the container's exit code. - Register a next-exit ContainerWait between create and start, so even an instantly-exiting container's exit code is captured before AutoRemove removes it; Runtime.Start now returns the exit channel. - Bound the readiness wait with a startup deadline: 20s interactive (shows a recoverable keep-waiting/stop prompt that re-arms on "keep waiting"), 60s non-interactive (fails, leaving the container running for inspection). LSTK_STARTUP_TIMEOUT overrides both. - Surface both failure modes as styled ErrorEvents with the exit code and the last container log lines, read after the log-tail drain so the tail is complete; add a start_timeout telemetry error code. Co-Authored-By: Claude <noreply@anthropic.com>
…ults Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
awaitStartup, handleStartupTimeout, and handleStartupFailure had grown to 6-9 parameters. Move the collaborators and mode that are constant across a Start invocation (rt, sink, tel, timeout, interactive) onto a startupMonitor receiver, keeping only per-call data as method arguments. Also drop the unused name parameter of awaitStartup. Co-Authored-By: Claude <noreply@anthropic.com>
An interrupted start (Ctrl+C while awaiting readiness) deliberately renders no styled error, but the early return also dropped the start_error lifecycle telemetry event that was emitted before startup monitoring was introduced. Restore the emit (error code start_failed, matching the previous behavior) and add an integration test that interrupts a start mid-wait and asserts the event is delivered, guarding the decision from the PR #390 review. Co-Authored-By: Claude <noreply@anthropic.com>
4178173 to
79db60e
Compare
Thanks for spotting this @anisaoshafi! Restored the telemetry event in 79db60e |
Motivation
lstk startkept waiting at "Starting LocalStack..." indefinitely when the container failed during startup (e.g. in offline/DNS-broken environments), and never surfaced the container's exit code or logs.Changes
Before: ∞ "Starting LocalStack..." without any actionable information.
After: If after 20 seconds LocalStack hasn't started lstk prompts to keep waiting or stop, suggesting to look at
lstk logsas well. In non-interactive mode the timeout is 60 seconds upon which lstk reports a failure and leaves the container running for inspection.Note: 3 second timeout is used for the video for the sake of brevity. 20 seconds felt like a good default for me, LocalStack should generally start faster as a local tool. Happy to adjust if there is any better idea about the default.
devx-758.mov
Changes details
next-exitcontainer wait between create and start, so the exit code is captured even for instantly-exiting containers before--rmremoves them;Runtime.Startnow returns the exit channel andawaitStartupconsumes it alongside the existing health/liveness pollLSTK_STARTUP_TIMEOUTKeep waiting [W](re-arms the deadline) /Stop LocalStack and exit [S](stops with progress feedback); non-interactive fails and leaves the container running for inspectionstart_timeouttelemetry error codeTests
Closes DEVX-758