feature: additional signal handling #2244
Conversation
Signed-off-by: Valery Piashchynski <[email protected]>
rustatian
added
the
C-enhancement
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (1)
WalkthroughAdds SIGQUIT and SIGUSR2 handling and an exec-based restart flow to the serve command (platform-specific implementations for non-Windows and Windows added/updated), removes Changes
Sequence Diagram(s)sequenceDiagram
participant OS as Operating System
participant RR as RoadRunner (serve)
participant Container as Application Container
rect rgb(235,248,255)
note right of OS: Restart (SIGUSR2)
OS->>RR: SIGUSR2
activate RR
RR->>Container: stop container (graceful)
RR->>RR: resolve executable path
RR->>RR: exec replacement (os.Exec) -> new process
deactivate RR
end
rect rgb(245,255,235)
note right of OS: Graceful stop (SIGQUIT/SIGTERM/SIGINT)
OS->>RR: SIGQUIT / SIGTERM / SIGINT
activate RR
RR->>Container: stop container with grace timeout
RR->>RR: notify sdnotify (READY/STOPPING), teardown watchdog
RR->>OS: exit gracefully
deactivate RR
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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 |
Signed-off-by: Valery Piashchynski <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2244 +/- ##
==========================================
- Coverage 36.26% 35.61% -0.66%
==========================================
Files 18 18
Lines 1092 1112 +20
==========================================
Hits 396 396
- Misses 657 677 +20
Partials 39 39 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
.gitignore(0 hunks).vscode/launch.json(1 hunks)go.mod(7 hunks)internal/cli/serve/command.go(4 hunks)
💤 Files with no reviewable changes (1)
- .gitignore
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-02T16:44:02.549Z
Learnt from: CR
PR: roadrunner-server/roadrunner#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-02T16:44:02.549Z
Learning: Applies to /go.mod : Set the module path to github.com/roadrunner-server/roadrunner/v2025 in go.mod
Applied to files:
go.mod
🧬 Code graph analysis (1)
internal/cli/serve/command.go (2)
internal/meta/meta.go (2)
Version(12-20)BuildTime(23-23)internal/sdnotify/sdnotify.go (3)
SdNotify(61-87)Ready(37-37)StartWatchdog(89-114)
🪛 Biome (2.1.2)
.vscode/launch.json
[error] 18-18: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 30-30: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 42-42: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 43-43: Expected an array, an object, or a literal but instead found ']'.
Expected an array, an object, or a literal here.
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Golang-CI (lint)
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
internal/cli/serve/command.go (4)
19-24: LGTM!The
loghelper function is a clean refactoring that ensures all output respects the silent flag.
85-89: LGTM! Signal handling expanded as intended.The changes correctly add SIGQUIT for graceful shutdown and introduce a dedicated channel for SIGUSR2 restart signals, aligning with the PR objectives.
106-126: LGTM!The startup and sdnotify integration correctly uses the new
loghelper for consistent output handling.
132-139: LGTM!The stop signal handling correctly logs the shutdown and stops the container gracefully.
go.mod (2)
3-3: Go 1.25.0 is a valid, released version. No action required.Go 1.25 was released and as of October 21, 2025, the latest stable version is Go 1.25.3 (released October 13, 2025). The version specified in
go.modis correct.Likely an incorrect or invalid review comment.
65-65: Remove the broken replace directive or ensure the sibling directory exists.The replace directive points to
../plugins/roadrunner-temporal, which doesn't exist outside the repository. This path will breakgo modcommands for all developers and CI/CD workflows:
- Local:
go mod tidy,go build, etc. will fail- CI: No workflows include steps to clone or fetch the sibling directory
Either remove the replace directive if it's not needed, or ensure the plugins directory is properly set up and documented (e.g., as a git submodule or separate checkout step).
⛔ Skipped due to learnings
Learnt from: CR PR: roadrunner-server/roadrunner#0 File: CLAUDE.md:0-0 Timestamp: 2025-10-02T16:44:02.549Z Learning: Applies to /go.mod : Set the module path to github.com/roadrunner-server/roadrunner/v2025 in go.mod
Signed-off-by: Valery Piashchynski <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/cli/serve/command.go (1)
143-165: Fix restart flow: preserve symlink semantics, notify systemd, and remove unreachable return.
- Use argv[0] + exec.LookPath to preserve symlink re-resolution for deployments switching a “current” symlink; os.Executable() resolves the link and prevents #929 behavior.
- Send sdnotify.Stopping before stopping the container on restart.
- Remove the dead
return nilaftersyscall.Exec(Exec never returns on success).- Preflight the target path before stopping to avoid downtime if Exec fails.
Apply within the shown range:
- case <-restartCh: - log("restart signal [SIGUSR2] received", *silent) - executable, err := os.Executable() - if err != nil { - log(fmt.Sprintf("restart failed: %s", err), *silent) - return errors.E("failed to restart") - } - args := os.Args - env := os.Environ() - - if err := cont.Stop(); err != nil { - log(fmt.Sprintf("restart failed: %s", err), *silent) - return errors.E("failed to restart") - } - - err = syscall.Exec(executable, args, env) - if err != nil { - log(fmt.Sprintf("restart failed: %s", err), *silent) - return errors.E("failed to restart") - } - - return nil + case <-restartCh: + log("restart signal [SIGUSR2] received", *silent) + args := os.Args + env := os.Environ() + + // Preserve symlink semantics (e.g., capistrano-style `current`): + // resolve what would run given argv[0] instead of resolving the real executable path. + resolvedPath, err := exec.LookPath(args[0]) + if err != nil { + log(fmt.Sprintf("restart preflight failed (resolve target): %s", err), *silent) + return errors.E(op, err) + } + + // Notify systemd that we are stopping due to restart. + _, _ = sdnotify.SdNotify(sdnotify.Stopping) + + if err := cont.Stop(); err != nil { + log(fmt.Sprintf("restart failed (stop container): %s", err), *silent) + return errors.E(op, err) + } + + // Replace the current process. On success, this never returns. + if err := syscall.Exec(resolvedPath, args, env); err != nil { + log(fmt.Sprintf("restart failed (exec): %s", err), *silent) + return errors.E(op, err) + }Add imports outside this range:
import ( "fmt" "os" "os/signal" "syscall" + "os/exec" )Verification scripts:
#!/bin/bash # 1) Confirm no dead code remains after Exec rg -n 'syscall\.Exec' -n internal/cli/serve/command.go -A3 # 2) Ensure we reference exec.LookPath and import os/exec rg -n 'exec\.LookPath|os/exec' internal/cli/serve/command.go # 3) Manual symlink check suggestion (run locally): # ln -s /path/to/rr-new /usr/local/bin/rr ; send SIGUSR2 and ensure new binary runs.
🧹 Nitpick comments (1)
internal/cli/serve/command.go (1)
87-92: Minor: consider signal.NotifyContext and dedup signals.You can simplify by using signal.NotifyContext and dropping duplicate SIGINT (os.Interrupt already covers it). Not required.
Example (for reference only):
ctx, stopFn := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM, syscall.SIGABRT, syscall.SIGQUIT) defer stopFn()Also applies to: 130-141
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/cli/serve/command.go(5 hunks)internal/cli/serve/command_windows.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/cli/serve/command_windows.go (4)
internal/cli/serve/command.go (1)
NewCommand(28-169)container/config.go (2)
NewConfig(27-52)ParseLogLevel(54-67)internal/meta/meta.go (2)
Version(12-20)BuildTime(23-23)internal/sdnotify/sdnotify.go (3)
SdNotify(61-87)Stopping(41-41)StartWatchdog(89-114)
internal/cli/serve/command.go (2)
internal/meta/meta.go (2)
Version(12-20)BuildTime(23-23)internal/sdnotify/sdnotify.go (3)
SdNotify(61-87)Ready(37-37)StartWatchdog(89-114)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Unit tests
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
internal/cli/serve/command.go (1)
87-92: SIGQUIT now mapped to graceful stop — LGTM.SIGQUIT is included in Notify and will follow the same graceful path as SIGTERM, satisfying #2243 expectations.
Optional: consider documenting in the help/CHANGELOG that SIGQUIT acts as graceful stop (honors grace timeout).
internal/cli/serve/command_windows.go (1)
105-126: Windows flow parity — LGTM.Startup, sdnotify ready/watchdog, and graceful stop mirror non‑Windows behavior appropriately; absence of restart handling aligns with platform notes.
Also applies to: 127-139
Signed-off-by: Valery Piashchynski <[email protected]>
Reason for This PR
closes: #2243
closes: #929
Description of Changes
License Acceptance
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit
New Features
Chores