fix: graceful shutdown on SIGTERM, improve deploy script - #245
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughThe pull request reorganizes the deployment and shutdown workflows across the Makefile, Go application, and deployment script. The Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 31 minutes and 41 seconds.Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Adding
os.Exit(0)inside adeferinmainwill force a zero exit code even on unexpected failures or panics; consider wiring a specific SIGTERM handler or propagating a status code tomain’s return path instead so only the intended shutdown path returns 0. - The deploy script’s port check hardcodes
:9021inssoutput; if the port becomes configurable or the listener binds differently (e.g., IPv6, different interface or format), this check may silently fail—consider reading the port from config or making the check more robust. - The deploy script now immediately
pkill -9’s the process, which removes any chance for a graceful shutdown; you might want to send a regular SIGTERM first and only escalate to SIGKILL if the process fails to exit within the port-release retry loop.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Adding `os.Exit(0)` inside a `defer` in `main` will force a zero exit code even on unexpected failures or panics; consider wiring a specific SIGTERM handler or propagating a status code to `main`’s return path instead so only the intended shutdown path returns 0.
- The deploy script’s port check hardcodes `:9021` in `ss` output; if the port becomes configurable or the listener binds differently (e.g., IPv6, different interface or format), this check may silently fail—consider reading the port from config or making the check more robust.
- The deploy script now immediately `pkill -9`’s the process, which removes any chance for a graceful shutdown; you might want to send a regular SIGTERM first and only escalate to SIGKILL if the process fails to exit within the port-release retry loop.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmd/app/main.go">
<violation number="1" location="cmd/app/main.go:336">
P2: `os.Exit(0)` bypasses the deferred cleanup in `main`, so shutdown can skip DB closes and other flushes.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| fmt.Fprintf(os.Stderr, "Dashboard stop error: %v\n", err) | ||
| } | ||
| log.Println("shutdown complete") | ||
| os.Exit(0) |
There was a problem hiding this comment.
P2: os.Exit(0) bypasses the deferred cleanup in main, so shutdown can skip DB closes and other flushes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmd/app/main.go, line 336:
<comment>`os.Exit(0)` bypasses the deferred cleanup in `main`, so shutdown can skip DB closes and other flushes.</comment>
<file context>
@@ -333,6 +333,7 @@ func main() {
fmt.Fprintf(os.Stderr, "Dashboard stop error: %v\n", err)
}
log.Println("shutdown complete")
+ os.Exit(0)
}()
</file context>
| os.Exit(0) | |
| return |
0f569a7 to
6c1397e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Makefile (1)
111-118: 💤 Low valueMinor redundancy:
installrestarts service thatdeployalready started.The
deploytarget (viadeploy.sh) already starts the service withsystemctl start dbkrab. Theninstalldoessystemctl restart dbkrab, which works but is redundant. Consider whetherinstallshould skip the restart if the goal is just to set up systemd, or ifdeploy.shshould skip starting the service when called frominstall.This is not a bug since restart on an already-running service is safe, but it adds a few seconds of unnecessary downtime.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 111 - 118, The install Makefile target redundantly restarts the service that deploy (via deploy.sh) already starts; update the install target (or deploy.sh) so they don't both restart/start the service: either remove or conditionalize the "systemctl restart dbkrab" in the install target (referencing the install target and its "systemctl restart dbkrab" line) or modify deploy.sh to skip starting when invoked from install (referencing deploy.sh and its "systemctl start dbkrab" call); ensure the chosen change still copies scripts/dbkrab.service and runs "systemctl daemon-reload" and "systemctl enable dbkrab" so the unit is installed without forcing an extra restart.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/app/main.go`:
- Around line 328-329: Replace the plain stdlib log.Printf call used after
receiving a signal with a structured slog.Info call: when you read sig from
sigCh (sig := <-sigCh) call slog.Info("shutting down", "signal", sig) instead of
log.Printf, and ensure the package imports use log/slog so structured logging is
used everywhere; update any import list to include slog and remove the
log.Printf usage in the shutdown path.
In `@scripts/deploy.sh`:
- Line 115: The deploy.sh echo references /opt/dbkrab/logs/dbkrab.log which is
inaccessible under the service's ProtectSystem settings; update the deploy
message in deploy.sh to point to the allowed path /var/log/dbkrab/dbkrab.log (or
alternatively modify the systemd unit to add ReadWritePaths=/opt/dbkrab/logs/),
and ensure this matches the logging config in internal/config/config.go (which
currently resolves to ./logs/dbkrab.log) so logs are written/read from the same
permitted location.
---
Nitpick comments:
In `@Makefile`:
- Around line 111-118: The install Makefile target redundantly restarts the
service that deploy (via deploy.sh) already starts; update the install target
(or deploy.sh) so they don't both restart/start the service: either remove or
conditionalize the "systemctl restart dbkrab" in the install target (referencing
the install target and its "systemctl restart dbkrab" line) or modify deploy.sh
to skip starting when invoked from install (referencing deploy.sh and its
"systemctl start dbkrab" call); ensure the chosen change still copies
scripts/dbkrab.service and runs "systemctl daemon-reload" and "systemctl enable
dbkrab" so the unit is installed without forcing an extra restart.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a356b55-2cd8-4801-b9b7-c82565ad1617
📒 Files selected for processing (3)
Makefilecmd/app/main.goscripts/deploy.sh
| sig := <-sigCh | ||
| log.Printf("Received %v, shutting down...", sig) |
There was a problem hiding this comment.
Use slog.Info instead of log.Printf for shutdown logging.
The signal capture is a good addition, but log.Printf violates the coding guidelines. Use structured logging with slog.
Proposed fix
- sig := <-sigCh
- log.Printf("Received %v, shutting down...", sig)
+ sig := <-sigCh
+ slog.Info("Received signal, shutting down...", "signal", sig)As per coding guidelines: "Use log/slog for structured logging" and "Prohibit using log.Print and log.Printf for logging".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sig := <-sigCh | |
| log.Printf("Received %v, shutting down...", sig) | |
| sig := <-sigCh | |
| slog.Info("Received signal, shutting down...", "signal", sig) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/app/main.go` around lines 328 - 329, Replace the plain stdlib log.Printf
call used after receiving a signal with a structured slog.Info call: when you
read sig from sigCh (sig := <-sigCh) call slog.Info("shutting down", "signal",
sig) instead of log.Printf, and ensure the package imports use log/slog so
structured logging is used everywhere; update any import list to include slog
and remove the log.Printf usage in the shutdown path.
- Signal handler uses context cancellation for graceful shutdown - Simplify systemd service: remove security restrictions causing issues - Add HOME env var for config file discovery - Simplify Makefile: stop/start/restart use systemd directly - reset target uses make stop/start for consistency - Fix log file creation with proper permissions
6c1397e to
4a64de7
Compare
2 participants
Summary
Test plan
make deployworks without stale process issuessystemctl stop dbkrabexits cleanly (check: echo $? -> 0)