fix(huntsman): Add SIGTERM listener for storage server, scheduler server, and execution manager (fixes #393). - #416
Conversation
WalkthroughUnix SIGTERM handlers are added to the execution manager, scheduler gRPC server, and storage gRPC server. Each integrates SIGTERM with its existing cancellation and graceful shutdown flow. ChangesSIGTERM shutdown handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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}
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 `@components/spider-execution-manager/src/bin/execution_manager.rs`:
- Around line 68-70: Register the SIGTERM handler before any startup work: in
components/spider-execution-manager/src/bin/execution_manager.rs lines 68-70,
move signal(SignalKind::terminate()) before runtime creation or spawning
runtime.run(); in components/spider-scheduler/src/bin/grpc_server.rs lines
63-65, move registration before config, storage, runtime, and listener setup;
and in components/spider-storage/src/bin/grpc_server.rs lines 58-60, move it
before config, runtime, listener, and server setup. Preserve the existing error
logging and handler behavior.
🪄 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: 9b135f09-cbf2-4568-95d7-659f54d56aa5
📒 Files selected for processing (3)
components/spider-execution-manager/src/bin/execution_manager.rscomponents/spider-scheduler/src/bin/grpc_server.rscomponents/spider-storage/src/bin/grpc_server.rs
| let mut sigterm = signal(SignalKind::terminate()).inspect_err( | ||
| |error| tracing::error!(em_id, error = % error, "Failed to register SIGTERM handler."), | ||
| )?; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repository files =="
git ls-files | rg '(^|/)execution_manager\.rs$|components/spider-(scheduler|storage)/src/bin/grpc_server\.rs$' || true
echo
echo "== relevant files =="
for f in \
components/spider-execution-manager/src/bin/execution_manager.rs \
components/spider-scheduler/src/bin/grpc_server.rs \
components/spider-storage/src/bin/grpc_server.rs
do
if [ -f "$f" ]; then
echo "--- $f ($(wc -l < "$f") lines) ---"
sed -n '1,140p' "$f" | nl -ba
fi
done
echo
echo "== signal crate/docs dependency annotations =="
for f in Cargo.toml cargo.lock components/*/Cargo.toml components/**/Cargo.lock; do
if [ -f "$f" ]; then
if rg -n 'tokio|tokio-signal|signal|ctrlc|async-signal' "$f" >/dev/null 2>&1; then
echo "--- $f ---"
rg -n 'tokio|signal|ctrlc|async-signal' "$f" || true
fi
fi
done
echo
echo "== signal registration occurrences =="
rg -n 'signal\(SignalKind::terminate\)|signal\(|SignalKind::terminate|tokio::signal' .Repository: y-scope/spider
Length of output: 487
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== lockfiles for tokio signal implementation snippets by package extraction =="
python3 - <<'PY'
from pathlib import Path
for path in Path('.').rglob('Cargo.lock'):
if any(part in {'.git', 'target'} for part in path.parts):
continue
text = path.read_text(errors='ignore')
idx = text.find('package = "tokio"')
if idx != -1:
print(f'--- {path} tokio package start: line={text[:idx].count(chr(10))+1} ---')
for line_no, line in enumerate(text.splitlines(), 1):
if 'version = ' in line or 'source = ' in line or 'dependencies' in line:
print(f'{line_no}: {line}')
PYRepository: y-scope/spider
Length of output: 231
🌐 Web query:
Tokio signal Unix install signal handler when created documentation
💡 Result:
In Tokio, signal handlers are installed lazily [1][2]. When you create a new tokio::signal::unix::Signal instance for a specific signal kind, the underlying OS signal handler is installed the first time that signal is registered for the process [3][1]. Key characteristics of this mechanism include: 1. Process-wide Scope: Once the signal handler is installed for a specific signal kind, it replaces the default platform behavior (SIG_DFL) for the duration of the entire process [3][1]. 2. Persistence: Dropping the Signal instance does not restore the original default signal handler [3][4]. Because signals are a global resource, Tokio's handler will continue to intercept that signal even after the listener is dropped [3][4]. If no active listeners are registered when a signal is subsequently received, the signal may be silently consumed by Tokio without the default system action (such as process termination) taking effect [4]. 3. Implementation Detail: Internally, Tokio uses a "self-pipe" pattern (typically using a UnixStream pair) to bridge the signal handler to the async reactor [5]. This allows the signal handler to remain minimal and safe while notifying the asynchronous runtime that a signal event has occurred [5][4]. Best practices for handling these behaviors include: - Keep Signal listeners active throughout the lifecycle of your application if you intend to catch those signals, or manage the signal state manually if you must drop them [4]. - For graceful shutdowns, you can use tokio::select! to monitor for signals alongside other application tasks [6][7]. - Be aware that if your application integrates with other libraries or languages (like Go) that also register signal handlers, conflicts may occur if the handlers are not configured compatibly [2]. For most use cases, creating a Signal instance via tokio::signal::unix::signal(SignalKind::...) is the standard approach to receiving asynchronous notifications for OS signals [8][9].
Citations:
- 1: https://github.com/tokio-rs/tokio/blob/c637f6e7/tokio/src/signal/unix.rs
- 2: Signals do not add SA_ONSTACK which may cause linked Go applications to crash. tokio-rs/tokio#3520
- 3: https://docs.rs/tokio/latest/tokio/signal/unix/struct.Signal.html
- 4: Unix signals: SIG_DFL never restored after Signal drop, second Ctrl-C silently swallowed tokio-rs/tokio#7905
- 5: https://deepwiki.com/tokio-rs/tokio/7.2-signal-handling
- 6: https://tokio.rs/tokio/topics/shutdown
- 7: https://stackoverflow.com/questions/77585473/rust-tokio-how-to-handle-more-signals-than-just-sigint-i-e-sigquit
- 8: https://docs.rs/tokio/latest/tokio/signal
- 9: https://docs.rs/tokio/latest/tokio/signal/
🌐 Web query:
docs Tokio tokio::signal::unix::Signal installs Unix handler only when created manual
💡 Result:
The statement that Tokio's Unix signal handler is only installed when created manually is correct in the sense that the registration process is triggered lazily upon the creation of a Signal listener [1][2]. Specifically, the behavior works as follows: 1. Lazy Installation: The underlying OS signal handler is not installed globally when the Tokio runtime starts. Instead, it is registered the first time a Signal instance (or a related helper like ctrl_c) is created for a specific SignalKind [1][3]. 2. Persistence: Once a signal handler has been registered for a particular signal kind, it remains installed for the duration of the entire process [1][4]. The underlying libc signal handler is never unregistered, even if the Signal instance that triggered the initial registration is dropped [1][5]. 3. Impact: By registering a handler, Tokio effectively takes over the handling of that signal, replacing the default platform behavior (e.g., terminating the process on SIGINT) with the capability to translate that signal into an asynchronous stream event [1][3]. Subsequent deliveries of that signal will continue to be captured by Tokio, and the process's default behavior will not be restored even if the original Signal listener is destroyed [1][3]. In summary, you must explicitly create a listener to "opt-in" to handling a specific Unix signal within your application, at which point the global handler for that signal becomes active for the remainder of the process's lifecycle [1][4].
Citations:
- 1: https://docs.rs/tokio/latest/tokio/signal/unix/struct.Signal.html
- 2: https://github.com/tokio-rs/tokio/blob/c637f6e7/tokio/src/signal/unix.rs
- 3: https://docs.rs/tokio/latest/src/tokio/signal/ctrl_c.rs.html
- 4: https://docs.rs/tokio/latest/tokio/signal/unix/fn.signal.html
- 5: https://www.chiark.greenend.org.uk/~ijackson/quicksand/d/doc/tokio/signal/unix/fn.signal.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
components/spider-execution-manager/src/bin/execution_manager.rs \
components/spider-scheduler/src/bin/grpc_server.rs \
components/spider-storage/src/bin/grpc_server.rs
do
echo "--- $f ($(wc -l < "$f") lines) ---"
awk '{printf "%5d\t%s\n", NR, $0}' "$f" | sed -n '1,160p'
done
echo
echo "== signal registration occurrences =="
rg -n 'signal\(SignalKind::terminate\)|signal\(|SignalKind::terminate|tokio::signal' .
echo
echo "== root Cargo.toml signal dependency excerpts =="
for f in Cargo.toml components/spider-execution-manager/Cargo.toml components/spider-scheduler/Cargo.toml components/spider-storage/Cargo.toml; do
if [ -f "$f" ]; then
echo "--- $f ---"
rg -n 'tokio|signal|ctrlc' "$f" || true
fi
doneRepository: y-scope/spider
Length of output: 17460
Install all SIGTERM handlers before startup work begins.
Tokio only installs the Unix handler when signal(...)/ctrl_c() is first registered, so before those calls SIGTERM keeps its default termination behaviour.
components/spider-execution-manager/src/bin/execution_manager.rs#L68-L70: register before creating the runtime or spawningruntime.run().components/spider-scheduler/src/bin/grpc_server.rs#L63-L65: register before config/storage/runtime/listener setup.components/spider-storage/src/bin/grpc_server.rs#L58-L60: register before config/runtime/listener/server setup.
📍 Affects 3 files
components/spider-execution-manager/src/bin/execution_manager.rs#L68-L70(this comment)components/spider-scheduler/src/bin/grpc_server.rs#L63-L65components/spider-storage/src/bin/grpc_server.rs#L58-L60
🤖 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 `@components/spider-execution-manager/src/bin/execution_manager.rs` around
lines 68 - 70, Register the SIGTERM handler before any startup work: in
components/spider-execution-manager/src/bin/execution_manager.rs lines 68-70,
move signal(SignalKind::terminate()) before runtime creation or spawning
runtime.run(); in components/spider-scheduler/src/bin/grpc_server.rs lines
63-65, move registration before config, storage, runtime, and listener setup;
and in components/spider-storage/src/bin/grpc_server.rs lines 58-60, move it
before config, runtime, listener, and server setup. Preserve the existing error
logging and handler behavior.
LinZhihao-723
changed the title
SIGTERM listener for servers. SIGTERM listener for storage server, scheduler server, and execution manager.
LinZhihao-723
changed the title
SIGTERM listener for storage server, scheduler server, and execution manager. SIGTERM listener for storage server, scheduler server, and execution manager (fixes #393).
LinZhihao-723
left a comment
LinZhihao-723
left a comment
There was a problem hiding this comment.
lgtm. Directly modified the PR title.
2 participants
Description
This PR resolves #393 by adding
SIGTERMsignal handler for storage server, scheduler and execution manager, so the k8s termination can be handled gracefully.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
SIGTERMsignals across execution management, scheduling, and storage services.