fix: wait for background goroutines during shutdown#1756
Conversation
The UI server ignored the shutdown signal and had no way to stop: it used the package-level http.ListenAndServe, which offers no Shutdown. Build a dedicated http.Server on a local mux, watch the context, and drain requests within the grace period. The local mux also keeps routes registered by other packages off the UI port. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The dist folder is gitignored except for a placeholder, so in CI the embedded assets are empty and ServeUI exits before listening. Skip the shutdown test in that case, matching the condition ServeUI itself checks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Coverage Report for CI Build 29364880379Coverage decreased (-0.02%) to 44.843%Details
Uncovered Changes
Coverage Regressions89 previously-covered lines in 3 files lost coverage.
Coverage Stats
💛 - Coveralls |
rohilsurana
left a comment
There was a problem hiding this comment.
Verified the whole chain: the listener returns ctx.Err() on cancel, #1747's ServeUI returns on every path, and ServeConnect's drain is bounded, so g.Wait() cannot hang for more than the grace period. One suggestion inline about signal handling during that window.
| // Wait for every server to finish draining before the deferred cleanup | ||
| // above closes the database and other dependencies under them. | ||
| return g.Wait() |
There was a problem hiding this comment.
Now that StartServer actually waits out the drain, a second Ctrl-C during that window does nothing. ctx comes from signal.NotifyContext (line 124), and its stop function only runs via the defer when StartServer returns — so while the drain runs (up to ~10s), further SIGINT/SIGTERM are swallowed by the registration. Before this PR the process exited almost immediately, so it never mattered.
Unregistering the handler once shutdown starts restores default signal handling, giving operators "first signal drains, second signal quits now":
| // Wait for every server to finish draining before the deferred cleanup | |
| // above closes the database and other dependencies under them. | |
| return g.Wait() | |
| // Once shutdown starts, unregister the signal handler so a second | |
| // SIGINT/SIGTERM gets default handling and can kill a stuck drain | |
| // instead of being swallowed. | |
| go func() { | |
| <-gctx.Done() | |
| cancelFunc() | |
| }() | |
| // Wait for every server to finish draining before the deferred cleanup | |
| // above closes the database and other dependencies under them. | |
| return g.Wait() |
cancelFunc doubles as the stop function, and by the time gctx is done the context is already cancelled (either by a signal or by a member error), so calling it here only unregisters the handler.
There was a problem hiding this comment.
Good catch — the swallow-window only became real once the drain actually waits. Applied as suggested in 9a90116: the handler unregisters when shutdown starts, so the first signal drains and a second one quits immediately.
A second SIGINT/SIGTERM during the drain was swallowed by the NotifyContext registration, leaving no way to force-quit a stuck shutdown. Unregister the handler when shutdown begins so the second signal gets default handling. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Based on #1747 — merge that first; this base retargets to main after.
Summary
StartServernow waits for the UI server and the audit log listener tofinish shutting down before running deferred cleanup, instead of only
waiting for the connect server.
Changes
errgroup;StartServerreturnsg.Wait(), so deferred cleanup(database and service
Close()calls) runs only after all threefinish.
failure also winds down the UI server and listener.
normal shutdown.
Technical Details
errgroup.WithContextcancels the group context when any memberreturns an error. Only
ServeConnectreturns a non-nil error, sog.Wait()returns exactly whatStartServerreturned before.Test Plan
go build,go vet,golangci-lintclean"cleaning up" line, with no listener warning
StartServerneeds live Postgres and SpiceDB beforereaching this code; the composed pieces are tested in fix: wait for HTTP server drain during graceful shutdown #1745 and fix: add graceful shutdown to UI server #1747
🤖 Generated with Claude Code