fix(lifecycle): finish shutdown before the process exits - #596
Conversation
Ctrl+C left the gateway with an unexplained "failed to shut down server within given timeout" and a bogus "make: *** [run] Error 1". Three separate causes: Run returned as soon as Start returned, while the goroutine doing the teardown was still working. The process exits the moment Run returns, so every closer the teardown had not reached yet — buffered usage and audit records, the database handle — was dropped. "application shutdown complete" was never reached on any interrupt. Run now waits for that goroutine, and the teardown also runs when the server stops without a signal. The graceful drain window was Echo's implicit 10s default, and the cutoff surfaced through Echo's own logger as a bare ERROR with no context. The gateway now sets the window itself, documented against the shutdown budget that has to contain it, and reports the cutoff as the routine event it is: Ctrl+C during a streamed response cuts the stream, because no drain window covers model traffic. `make run` used `go run`, which exits 1 when interrupted even after the program it supervises exits cleanly — verified against a twelve-line program that handles SIGINT and returns 0. Building and exec'ing puts the gateway directly under make's signal handling. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe application now coordinates startup and graceful shutdown through ChangesShutdown lifecycle
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Signal
participant Run
participant Application
participant Shutdown
Signal->>Run: cancellation
Run->>Application: start and monitor
Run->>Shutdown: invoke shutdown with timeout
Shutdown->>Application: drain and teardown
Application-->>Shutdown: teardown complete
Shutdown-->>Run: return
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
run/run.go (1)
1-1: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDouble shutdown invocation on the failed-Start path is unfixed and untested. When
application.Startfails,startApplicationshuts down the application itself; once it returns,serveUntilShutdown's goroutine shuts it down again afterserverReturnedcloses — relying entirely on an unverifiedApp.Shutdownidempotency contract, and doubling the worst-case shutdown latency budget.
run/run.go#L211-244: remove the innershutdownApplicationcall fromstartApplication's error branch (Line 263-272) and let this goroutine be the sole, unconditional caller of shutdown.run/lifecycle_test.go#L230-237: once fixed, assertapp.shutdownCallCount() == 1inTestServeUntilShutdown_ReturnsStartFailureto lock in single-invocation behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@run/run.go` at line 1, Remove the shutdownApplication call from startApplication’s application.Start error branch, allowing the serveUntilShutdown goroutine to remain the sole unconditional shutdown caller after serverReturned closes. Update TestServeUntilShutdown_ReturnsStartFailure to assert app.shutdownCallCount() equals 1, covering the failed-start path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/server/http.go`:
- Around line 48-59: Add an automated assertion covering the relationship
between internal/server/http.go's gracefulDrainTimeout and the run package's
shutdownTimeout, ensuring gracefulDrainTimeout remains strictly shorter. Keep
the existing timeout values and shutdown behavior unchanged, and place the check
where both symbols can be referenced safely.
In `@Makefile`:
- Line 38: Update the gomodel execution command to quote the Make-expanded
LOG_LEVEL and SWAGGER_ENABLED environment assignments, ensuring whitespace and
shell metacharacters remain data while preserving the existing exec behavior.
In `@run/lifecycle_test.go`:
- Around line 230-237: Extend TestServeUntilShutdown_ReturnsStartFailure to
assert the stubLifecycleApp shutdown call count after the start error is
returned, matching the teardown assertion used by
TestServeUntilShutdown_TearsDownWhenServerStopsOnItsOwn and verifying the
expected count for this path.
In `@run/run.go`:
- Around line 211-244: Update serveUntilShutdown and the startup/shutdown
coordination so a failed start does not trigger shutdownApplication twice: track
whether startApplication already performed teardown, and have the shutdown
goroutine skip its call when that path occurred. Preserve shutdown for signal
cancellation and servers that stop normally, and continue waiting for the
selected shutdown to complete before returning.
---
Outside diff comments:
In `@run/run.go`:
- Line 1: Remove the shutdownApplication call from startApplication’s
application.Start error branch, allowing the serveUntilShutdown goroutine to
remain the sole unconditional shutdown caller after serverReturned closes.
Update TestServeUntilShutdown_ReturnsStartFailure to assert
app.shutdownCallCount() equals 1, covering the failed-start path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cb58d8e1-011e-408f-9dd9-8465e91059e5
📒 Files selected for processing (5)
Makefileinternal/server/http.gointernal/server/http_start_test.gorun/lifecycle_test.gorun/run.go
|
|
||
| // gracefulDrainTimeout bounds how long the HTTP server waits for in-flight | ||
| // requests to finish once shutdown begins. Streamed model responses run far | ||
| // longer than any drain window worth waiting for, so this is deliberately a | ||
| // cutoff rather than a promise: past it the remaining connections are cut | ||
| // and shutdown moves on to flushing usage and audit records. It is sized | ||
| // for the short requests that can actually finish — a dashboard fetch, a | ||
| // health check — not for model traffic. | ||
| // | ||
| // It must stay below run.shutdownTimeout, which covers this drain plus | ||
| // those flushes and the database close that follow it. | ||
| gracefulDrainTimeout = 10 * time.Second |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Cross-package timeout invariant isn't enforced anywhere.
The comment states gracefulDrainTimeout "must stay below run.shutdownTimeout," but nothing in the code enforces this — both constants are unexported and defined in separate packages, so a future edit to either one could silently violate the invariant with no compiler or test failure. Consider adding a small test (in either package, or a shared lifecycle-config test) that asserts this relationship, or centralizing both timeouts in one place that both packages reference.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/server/http.go` around lines 48 - 59, Add an automated assertion
covering the relationship between internal/server/http.go's gracefulDrainTimeout
and the run package's shutdownTimeout, ensuring gracefulDrainTimeout remains
strictly shorter. Keep the existing timeout values and shutdown behavior
unchanged, and place the check where both symbols can be referenced safely.
Confidence Score: 2/5This PR should not merge until shutdown-before-start ordering, missing run-output directory creation, and teardown-error propagation are fixed. An early cancellation can permanently miss server shutdown and hang the process, fresh-checkout make run fails before startup, and completed teardown failures still produce a successful process exit. Files Needing Attention: run/run.go and Makefile
 T-Rex LogsWhat T-Rex did
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| go func() { | ||
| select { | ||
| case <-ctx.Done(): | ||
| case <-serverReturned: | ||
| } | ||
| shutdownCtx, cancel := context.WithTimeout(context.Background(), shutdownTimeout) | ||
| defer cancel() | ||
| shutdownDone <- shutdownApplication(application, shutdownCtx) |
There was a problem hiding this comment.
When the parent context is already canceled or a signal arrives before App.Start registers its cancellation function, this goroutine completes the idempotent shutdown against nil server state. App.Start then starts the server with no remaining shutdown call to cancel it, causing serveUntilShutdown to block indefinitely.
Artifacts
- Evidence file captured while the check ran.
Repro: verbose test output with timestamps showing early Shutdown and bounded hangs
- The full command output behind this check.
| # handling instead of behind a supervisor. | ||
| run: | ||
| LOG_LEVEL=$(LOG_LEVEL) SWAGGER_ENABLED=$(SWAGGER_ENABLED) go run -tags=swagger ./cmd/gomodel | ||
| go build -tags=swagger -ldflags '$(LDFLAGS)' -o bin/gomodel ./cmd/gomodel |
There was a problem hiding this comment.
When make run is invoked from a fresh checkout or after make clean, this build writes to bin/gomodel without creating the ignored bin directory, causing the command to fail before the gateway starts.
| if err := <-shutdownDone; err != nil { | ||
| slog.Error("application shutdown error", "error", err) | ||
| } | ||
| return startErr |
There was a problem hiding this comment.
When draining, flushing buffered usage or audit records, closing storage, or another teardown step fails, this code logs the shutdown error but returns the successful start result. The process therefore exits zero despite incomplete teardown, preventing supervisors and scripts from detecting the failure.
Review follow-ups. A failed Start used to shut down twice: startApplication tore the application down itself, then serveUntilShutdown's goroutine did it again once the server returned. That leaned on an App.Shutdown idempotency contract nothing in this package verified, and gave the failed-start path two shutdownTimeout budgets to burn through. serveUntilShutdown is now the only caller, so every exit — signal, server stopping on its own, or a Start that never got off the ground — runs teardown exactly once on one budget, and startApplication is gone. A teardown that also fails is now logged rather than wrapped around the start error, since the start error is what the operator needs and what sets the exit code. The comment claiming the drain window must stay below the shutdown budget was load-bearing but unchecked, and the two constants live in different packages. GracefulDrainTimeout is exported so a test can assert both the ordering and that enough of the budget is left for the flushes that follow the drain; verified it fails at 30s and at 26s. Quotes the Make-expanded values in the run recipe so whitespace or shell metacharacters in LOG_LEVEL or SWAGGER_ENABLED stay data. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
All four addressed. Double shutdown on the failed-Start path — agreed, and taken further than "track whether teardown already ran": One behavior change worth calling out: a teardown that also fails is now logged rather than joined onto the returned error. The start error is what the operator needs to see and what sets the exit code, and the shutdown error still reaches the log. Cross-package invariant — fair; the comment was load-bearing and unchecked. Makefile quoting — applied (and to the pro repo's equivalent recipe). Shutdown-count assertion on the start-failure test — folded into the first item. Re-verified end to end after the refactor, with a request deliberately held in flight: |
Ctrl+C on
make runended in an unexplainedfailed to shut down server within given timeoutfollowed bymake: *** [run] Error 1. Three independent causes, and the one that mattered most was not the one in the error message.1. The teardown never finished
Runreturned as soon asStartreturned, while the goroutine doing the teardown was still working. The process exits the momentRunreturns, so every closer the teardown had not reached yet — buffered usage records, buffered audit records, the database handle — was dropped.application shutdown completewas never reached on any interrupt, and this happened on every Ctrl+C, not only the ones that logged an error.Runnow waits for that goroutine. The teardown also runs when the server stops without a signal, so a server that dies on its own still releases its resources;App.Shutdownis idempotent, which keeps that harmless on the failed-start path.2. The scary error was Echo's, not ours
Nothing configured
GracefulTimeout, so Echo used its implicit 10s default and reported the cutoff through its own logger as a barecontext deadline exceeded. It fires whenever any request is in flight — one open stream is enough.The window is now set explicitly and documented against the shutdown budget that has to contain it, and the cutoff is reported as the routine event it is. The value stays at 10s deliberately: no drain window covers a streamed model response, so lengthening it would only make Ctrl+C slower without changing the outcome.
3.
make: Error 1isgo rungo runexits 1 when interrupted no matter how cleanly the child exits. Verified against a twelve-line program that catches SIGINT and returns 0:make runnow builds andexecs, which also puts the gateway directly under make's signal handling instead of behind a supervisor.Verification
Reproduced with a request deliberately held in flight, before and after:
make run+ SIGINT now exits 0. Both new lifecycle tests fail against the old code (serveUntilShutdown returned mid-teardown,shutdownCalls = 0, want 1) and pass with the fix.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores