fix: implement graceful shutdown respecting OS signals#505
Conversation
WalkthroughThe ChangesApplication Lifecycle & Graceful Shutdown
Sequence DiagramsequenceDiagram
participant Main as Application
participant SigHandler as Signal Handler
participant Metrics as Metrics
participant Server as HTTP Server
participant OS as OS Signals
Main->>SigHandler: SetupSignalHandler()
SigHandler->>OS: Subscribe to SIGTERM/SIGINT
SigHandler-->>Main: Return context
Main->>Metrics: ConfigureMetrics(ctx)
Metrics->>Metrics: Initialize with signal context
Main->>Server: Start server in goroutine
Server-->>Main: Report errors via channel
alt Signal or Server Error
OS->>SigHandler: Signal received
SigHandler->>Main: ctx.Done() fires
else
Server->>Main: Error sent to channel
end
Main->>Server: Shutdown(10s timeout)
Server->>Server: Graceful drain & close
Server-->>Main: Shutdown complete
Main->>Main: Exit
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
|
@Ajpantuso: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/main.go (1)
125-131: ⚡ Quick winDefensively filter
http.ErrServerClosedin the server error case.Structurally,
http.ErrServerClosedcannot land inerrChwhile theselectis still active today —server.Shutdown()is only called afterctx.Done()exits theselect. However, without an explicit guard, a future change that callsShutdown()earlier (e.g., from another goroutine) would silently misclassify a clean shutdown as a fatal error and exit with code 1. Adding the check costs one line and self-documents the intended invariant.🛡️ Proposed defensive filter
case err := <-errCh: - log.Error(err, "Server failed") - os.Exit(1) + if !errors.Is(err, http.ErrServerClosed) { + log.Error(err, "Server failed") + os.Exit(1) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/main.go` around lines 125 - 131, When receiving an error from errCh inside the select, defensively ignore the benign http.ErrServerClosed instead of treating it as a fatal error: replace the current case handling (case err := <-errCh:) with a conditional that checks if err == http.ErrServerClosed (or errors.Is(err, http.ErrServerClosed)) and, if so, log it at Info/debug level and return without calling os.Exit(1); otherwise call log.Error(err, "Server failed") and os.Exit(1). This touches the select branch that reads from errCh and uses ctx.Done(), and references errCh, ctx.Done(), http.ErrServerClosed, log.Error and os.Exit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/main.go`:
- Around line 125-131: When receiving an error from errCh inside the select,
defensively ignore the benign http.ErrServerClosed instead of treating it as a
fatal error: replace the current case handling (case err := <-errCh:) with a
conditional that checks if err == http.ErrServerClosed (or errors.Is(err,
http.ErrServerClosed)) and, if so, log it at Info/debug level and return without
calling os.Exit(1); otherwise call log.Error(err, "Server failed") and
os.Exit(1). This touches the select branch that reads from errCh and uses
ctx.Done(), and references errCh, ctx.Done(), http.ErrServerClosed, log.Error
and os.Exit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1ca11990-11ec-498d-964a-645d5effc8d3
📒 Files selected for processing (1)
cmd/main.go
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Ajpantuso, tnierman The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
2 participants
Summary
Implements graceful shutdown when
SIGTERMorSIGINTis received to prevent blocking node drains.Summary by CodeRabbit