fix(shutdown): graceful shutdown on SIGTERM (#1324)#1420
Open
sansyrox wants to merge 2 commits into
Open
Conversation
Single-process mode never installed a Python SIGTERM handler, so the process blocked in the Rust event loop and ignored SIGTERM (multiprocessing terminate(), `docker stop`, Kubernetes) -- it only stopped on SIGKILL, and shutdown handlers never ran. spawn_process() now registers a SIGTERM handler that raises KeyboardInterrupt (POSIX only), reusing the working SIGINT path so the Rust runtime runs registered shutdown handlers and exits cleanly. The multi-process parent handler now stops workers gracefully (terminate -> join with a timeout -> force-kill stragglers) instead of an immediate SIGKILL, so worker shutdown handlers run too. The timeout is configurable via ROBYN_GRACEFUL_SHUTDOWN_TIMEOUT (default 10s). Adds an integration test that spawns the server, sends SIGTERM, and asserts a clean exit (returncode 0) and that the shutdown handler ran. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughRobyn now handles SIGTERM through a graceful shutdown path, waits for workers before force-killing survivors, and adds an integration app plus test that verify shutdown handlers run and the process exits cleanly. ChangesGraceful shutdown flow
Sequence Diagram(s)sequenceDiagram
participant "integration_tests/test_graceful_shutdown.py"
participant "integration_tests/graceful_shutdown_app.py"
participant "robyn.processpool.py"
"integration_tests/test_graceful_shutdown.py"->>"integration_tests/graceful_shutdown_app.py": start subprocess on a free port
"integration_tests/test_graceful_shutdown.py"->>"integration_tests/graceful_shutdown_app.py": proc.terminate() sends SIGTERM
"integration_tests/graceful_shutdown_app.py"->>"robyn.processpool.py": SIGTERM raises KeyboardInterrupt in the main thread
"robyn.processpool.py"->>"integration_tests/graceful_shutdown_app.py": run shutdown handlers and stop workers
"integration_tests/graceful_shutdown_app.py"->>"integration_tests/test_graceful_shutdown.py": exit code 0 after sentinel file is written
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@integration_tests/test_graceful_shutdown.py`:
- Line 35: The sentinel path setup in the graceful shutdown test currently uses
tempfile.mktemp, which creates a race-prone path only; replace it with a
pre-created tempfile.TemporaryDirectory-based location so the subprocess uses an
already created, uniquely scoped directory. Update the sentinel setup in
integration_tests/test_graceful_shutdown.py and keep the rest of the test logic
pointing at that directory through the existing sentinel variable.
In `@robyn/processpool.py`:
- Around line 72-76: The second-signal path in terminating_signal_handler is
currently just returning, which keeps the custom handler active instead of
forcing termination. Update terminating_signal_handler in processpool.py so that
when shutting_down is already true it restores the default handler for the
received signal and re-raises that same signal, ensuring a repeated SIGINT or
SIGTERM hard-stops the process instead of being swallowed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7cafb3d4-3128-41a5-9822-78abf32c0166
📒 Files selected for processing (3)
integration_tests/graceful_shutdown_app.pyintegration_tests/test_graceful_shutdown.pyrobyn/processpool.py
- Reject non-finite ROBYN_GRACEFUL_SHUTDOWN_TIMEOUT (inf/nan) so the force-kill fallback stays reachable; fall back to the 10s default. - Second SIGINT/SIGTERM during shutdown now restores the default disposition and re-sends the signal, so users can force-stop a slow shutdown instead of having it silently swallowed. - Test: use tempfile.TemporaryDirectory for the sentinel instead of the race-prone tempfile.mktemp. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@integration_tests/test_graceful_shutdown.py`:
- Around line 37-40: The startup in test_graceful_shutdown uses _free_port() and
then passes the port into subprocess.Popen, which can race with other processes
before the child binds. Update the test setup so the server process itself
chooses or reserves the listening port in a way that cannot be stolen between
_free_port() and _wait_until_serving(), and keep the verification logic in
_wait_until_serving(proc, port) aligned with the new startup flow.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: dcb4193d-dd4d-4ffe-8873-0fb205e48ca9
📒 Files selected for processing (2)
integration_tests/test_graceful_shutdown.pyrobyn/processpool.py
🚧 Files skipped from review as they are similar to previous changes (1)
- robyn/processpool.py
Comment on lines
+37
to
+40
| port = _free_port() | ||
| proc = subprocess.Popen([sys.executable, APP, sentinel, str(port)]) | ||
| try: | ||
| assert _wait_until_serving(proc, port), "server failed to start" |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Avoid the free-port race during startup.
_free_port() only proves the port was free before Popen; another process can claim it before the child binds, which makes this integration test intermittently fail under parallel CI.
Suggested direction
- port = _free_port()
- proc = subprocess.Popen([sys.executable, APP, sentinel, str(port)])
- try:
- assert _wait_until_serving(proc, port), "server failed to start"
+ for _ in range(3):
+ port = _free_port()
+ proc = subprocess.Popen([sys.executable, APP, sentinel, str(port)])
+ if _wait_until_serving(proc, port):
+ break
+ if proc.poll() is None:
+ proc.kill()
+ proc.wait(timeout=5)
+ else:
+ pytest.fail("server failed to start")
+ try:📝 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.
Suggested change
| port = _free_port() | |
| proc = subprocess.Popen([sys.executable, APP, sentinel, str(port)]) | |
| try: | |
| assert _wait_until_serving(proc, port), "server failed to start" | |
| for _ in range(3): | |
| port = _free_port() | |
| proc = subprocess.Popen([sys.executable, APP, sentinel, str(port)]) | |
| if _wait_until_serving(proc, port): | |
| break | |
| if proc.poll() is None: | |
| proc.kill() | |
| proc.wait(timeout=5) | |
| else: | |
| pytest.fail("server failed to start") | |
| try: |
🧰 Tools
🪛 ast-grep (0.44.0)
[error] 37-37: Command coming from incoming request
Context: subprocess.Popen([sys.executable, APP, sentinel, str(port)])
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🤖 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 `@integration_tests/test_graceful_shutdown.py` around lines 37 - 40, The
startup in test_graceful_shutdown uses _free_port() and then passes the port
into subprocess.Popen, which can race with other processes before the child
binds. Update the test setup so the server process itself chooses or reserves
the listening port in a way that cannot be stolen between _free_port() and
_wait_until_serving(), and keep the verification logic in
_wait_until_serving(proc, port) aligned with the new startup flow.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Closes #1324 — Robyn now shuts down gracefully on SIGTERM. Previously, sending SIGTERM (e.g.
multiprocessing.Process.terminate(),docker stop, a Kubernetes pod stop) to a single-process server hung until SIGKILL, and registeredshutdown_handlers never ran. The reporter saw a pytest fixture'sproc.terminate()leave the process running forever.Root cause
server.start()is a blocking call into the Rust runtime (run_forever). SIGINT works only because CPython installs a C-level SIGINT handler that raisesKeyboardInterrupt, which propagates out of that blocking call → the Rust runtime runs the shutdown handler →exit(0).But in single-process mode (
processes == 1, the default — and always on Windows) Robyn installed no Python SIGTERM handler: the parent's signal handlers were only registered in the multi-process path. So SIGTERM was left at its default and the Python main thread, blocked inside Rust, was never interrupted → hang.Fix (Python-only, no Rust changes)
spawn_process(): register a SIGTERM handler that raisesKeyboardInterrupt(POSIX only), before the blockingserver.start(). SIGTERM now follows the exact same working path as Ctrl+C → shutdown handlers run → clean exit. This fixes the reported single-process hang and also makes every worker exit gracefully.process.kill()(SIGKILL) with graceful terminate → join(timeout) → force-kill stragglers, so worker shutdown handlers run on multi-process shutdown too. Includes a re-entry guard for a second signal during shutdown.ROBYN_GRACEFUL_SHUTDOWN_TIMEOUT(default10s).I verified actix-web's own signal handling does not clobber the Python handler (it chains via
signal_hook_registry), so no Rust.disable_signals()change is needed.Verification
Manually reproduced the issue and confirmed the fix against the current build: a single-process app sent SIGTERM now exits rc=0 immediately and the shutdown handler runs (writes its sentinel), instead of hanging.
Tests
integration_tests/test_graceful_shutdown.py(+graceful_shutdown_app.py): spawns the server on a free port, waits until it's serving, sends SIGTERM, and asserts:shutdown_handlerran.Skipped on Windows (no SIGTERM delivery).
Notes
This supersedes the stale PRs #1396 and #1349 with a clean implementation. #1349's single-process fix (
loop.add_signal_handler(SIGTERM, loop.stop)) was placed after the blockingserver.start(), so it never ran while serving and didn't fix the reported path.🤖 Generated with Claude Code
Summary by CodeRabbit
ROBYN_GRACEFUL_SHUTDOWN_TIMEOUT(with safe fallback behavior).