fix(agent-data-plane): handle SIGTERM for graceful shutdown - #2322
Conversation
SIGTERM is the signal used by systemd, container runtimes, and Kubernetes to request shutdown, but ADP only listened for SIGINT, so it terminated immediately without draining the topology or honoring the shutdown timeout. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Binary Size Analysis (Agent Data Plane)Baseline: ca31d5f · Comparison: 75b8f23 · diff ✅ Binary size difference within thresholdChanges by Module
Detailed Symbol Changes |
Regression Detector (Agent Data Plane)Run ID: Optimization Goals: ✅ No significant changes detectedFine details of change detection per experiment (5)Experiments configured
Bounds Checks: ✅ Passed (5)
ExplanationA change is flagged as a regression when |Δ mean %| > 5.00% in the regressing direction for its optimization goal AND SMP marks the experiment as a regression ( |
…dows On Windows, ADP is managed as a subprocess by dd-procmgr, which requests a graceful stop by sending CTRL_BREAK_EVENT (not CTRL_C_EVENT) via GenerateConsoleCtrlEvent. The previous fallback only listened for tokio::signal::ctrl_c(), so this request went unnoticed and ADP would sit until dd-procmgr's stop timeout expired and force-killed it via its Job Object, reproducing the same "no graceful shutdown" behavior as SIGTERM on Unix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ulary The new SIGTERM/CTRL_BREAK doc comment on wait_for_shutdown_signal uses these terms, which Vale's spelling check doesn't recognize, failing check-docs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75b8f233af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| use tokio::signal::unix::{signal, SignalKind}; | ||
|
|
||
| let mut sigterm = signal(SignalKind::terminate()).expect("failed to install SIGTERM handler"); |
There was a problem hiding this comment.
When the service is stopped while ADP is still bootstrapping—for example, while handle_run_command is waiting for the initial Agent configuration—this future has not yet been polled, so the SIGTERM listener is not installed and the kernel terminates ADP immediately. Register the listener before the startup awaits and select the startup path against it so SIGTERM consistently enters graceful shutdown rather than only working after Agent Data Plane running. is logged.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Mmm, this is a good catch, but is a pre-existing bug. I'll open a stacked PR to address it to keep the changes isolated.
There was a problem hiding this comment.
Actually, thinking about this more, if the SIGTERM is received before the supervisor starts, I don't think there is a need to handle the signal for graceful shutdown. It's fine if the process just exits. cc/ @tobz for thoughts given the interaction with the supervisor.
There was a problem hiding this comment.
if the SIGTERM is received before the supervisor starts I don't think there is a need to handle the signal for graceful shutdown
Sounds right to me.
There was a problem hiding this comment.
Yeah, we only care about intercepting typical shutdown-indicating signals if we're at a point where we need to shutdown in an orderly fashion... so anything before running the "main loop" (run the root supervisor, etc) is generally fair game for abrupt shutdown.
![datadog-datadog-prod-us1-2[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
Direct and process-group SIGTERM reach graceful shutdown while preserving SIGINT behavior, and interrupted DogStatsD replay reaches cleanup. The Unix and Windows signals also match those emitted by the real process manager.
📊 Validated against 6 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit 75b8f23 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: queued
mergequeue-status: in_progress
and removed
mergequeue-status: queued
labels
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
## Human Summary Adds support for handling SIGTERM on *nix platforms, which we expect to receive when being managed by SystemD or s6, which ADP is packaged with. On Windows, we where we are managed by [procmgr](https://github.com/DataDog/datadog-agent/tree/main/pkg/procmgr) we expect to receive a CTRL_BREAK_EVENT ([ref](https://github.com/DataDog/datadog-agent/blob/eb013ebe547c7eb01868d377e266e47f420ee445/pkg/procmgr/rust/src/platform/windows.rs#L215)) so handle that instead. For `dogstatsd replay` we currently only support Linux so we only additionally handle SIGTERM. We expect to only be run interactively, but the process may still receive a SIGTERM from out-of-band. ## AI Summary ADP's production binaries only listened for `SIGINT` when deciding to enter graceful shutdown, but `SIGINT` is not the signal used in any of ADP's real deployment paths — systemd, container runtimes, and Kubernetes all send `SIGTERM` to request shutdown (during service stops/restarts, rollouts, evictions, node drains, and container termination). Since `SIGTERM` used its default disposition, ADP was killed outright instead of draining its topology, so buffered data could be lost and the shutdown-timeout/forceful-abort diagnostics never had a chance to fire. This wires `SIGTERM` into the same graceful-shutdown path already used for `SIGINT`, so the two behave identically. The `dogstatsd replay` debug subcommand gets the same treatment for consistency, since it also has state that should be flushed on interruption. `SIGPIPE` handling is being tracked separately and isn't part of this change. Verified on a systemd-managed host by installing the current Datadog Agent package and swapping in a build of `agent-data-plane` with this fix: - Before the fix: `systemctl stop datadog-agent-data-plane` (and `systemctl restart datadog-agent`, which cascades a stop via `BindsTo`) killed the process in single-digit milliseconds with `code=killed, signal=TERM` and no shutdown-path log lines at all. - After the fix: the same `systemctl stop` produces `Received SIGTERM, shutting down...`, a clean topology drain (listeners and HTTP servers stopping), `Agent Data Plane shut down successfully.`, and the process exits with `status=0/SUCCESS`. Closes #2319. ## Test plan - [x] Verified under real systemd: patched `agent-data-plane` binary running as the actual `datadog-agent-data-plane.service` unit now drains and exits cleanly on `systemctl stop` / `systemctl restart datadog-agent`, versus being hard-killed before the fix. - [x] Verified standalone: direct `SIGTERM` to the binary now triggers the same graceful-shutdown log sequence as `SIGINT`. Co-authored-by: jesse.szwedko <jesse.szwedko@datadoghq.com> d9a74ed
…`run` and `dogstatsd replay` Per discussion on #2322, a signal received before the supervisor starts doesn't need graceful handling, so this drops the earlier eager pre-registration approach and instead extracts the existing lazy signal-wait logic into a shared helper reused by both the `run` and `dogstatsd replay` commands, removing the duplication. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
## Human Summary Refactors the shutdown wait handler into a shared module that can be used where ever we need to wait to ensure we consistently rely on the same signals, cross-platform. ## AI Summary #2322 added SIGTERM/SIGINT handling for graceful shutdown, but the same signal-waiting logic was duplicated between the `run` command and `dogstatsd replay`'s cancellation setup. An earlier version of this PR tried to close a related gap -- a signal delivered before the supervisor starts falling through to the OS's default disposition -- by registering signal handlers eagerly at process start. Per [discussion on #2322](#2322 (comment)), that gap doesn't need graceful handling: an abrupt exit before the supervisor is running is acceptable. This PR instead does a pure refactor, extracting the existing lazy signal-wait logic into a shared `saluki_app::util::wait_for_shutdown_signal` helper used by both `run` and `dogstatsd replay`, removing the duplication and making it reusable by any binary built on the saluki crates, not just agent-data-plane. ## Test plan - [x] `cargo check --workspace`, `cargo clippy -p agent-data-plane -p saluki-app --no-deps` pass. - No new automated tests added; this is a non-behavioral refactor of existing signal-handling code. Co-authored-by: jesse.szwedko <jesse.szwedko@datadoghq.com>
## Human Summary Refactors the shutdown wait handler into a shared module that can be used where ever we need to wait to ensure we consistently rely on the same signals, cross-platform. ## AI Summary #2322 added SIGTERM/SIGINT handling for graceful shutdown, but the same signal-waiting logic was duplicated between the `run` command and `dogstatsd replay`'s cancellation setup. An earlier version of this PR tried to close a related gap -- a signal delivered before the supervisor starts falling through to the OS's default disposition -- by registering signal handlers eagerly at process start. Per [discussion on #2322](#2322 (comment)), that gap doesn't need graceful handling: an abrupt exit before the supervisor is running is acceptable. This PR instead does a pure refactor, extracting the existing lazy signal-wait logic into a shared `saluki_app::util::wait_for_shutdown_signal` helper used by both `run` and `dogstatsd replay`, removing the duplication and making it reusable by any binary built on the saluki crates, not just agent-data-plane. ## Test plan - [x] `cargo check --workspace`, `cargo clippy -p agent-data-plane -p saluki-app --no-deps` pass. - No new automated tests added; this is a non-behavioral refactor of existing signal-handling code. Co-authored-by: jesse.szwedko <jesse.szwedko@datadoghq.com> e760adf
3 participants
Human Summary
Adds support for handling SIGTERM on *nix platforms, which we expect to receive when being managed by SystemD or s6, which ADP is packaged with. On Windows, we where we are managed by procmgr we expect to receive a CTRL_BREAK_EVENT (ref) so handle that instead.
For
dogstatsd replaywe currently only support Linux so we only additionally handle SIGTERM. We expect to only be run interactively, but the process may still receive a SIGTERM from out-of-band.AI Summary
ADP's production binaries only listened for
SIGINTwhen deciding to enter graceful shutdown, butSIGINTis not the signal used in any of ADP's real deployment paths — systemd, container runtimes, and Kubernetes all sendSIGTERMto request shutdown (during service stops/restarts, rollouts, evictions, node drains, and container termination). SinceSIGTERMused its default disposition, ADP was killed outright instead of draining its topology, so buffered data could be lost and the shutdown-timeout/forceful-abort diagnostics never had a chance to fire. This wiresSIGTERMinto the same graceful-shutdown path already used forSIGINT, so the two behave identically. Thedogstatsd replaydebug subcommand gets the same treatment for consistency, since it also has state that should be flushed on interruption.SIGPIPEhandling is being tracked separately and isn't part of this change.Verified on a systemd-managed host by installing the current Datadog Agent package and swapping in a build of
agent-data-planewith this fix:systemctl stop datadog-agent-data-plane(andsystemctl restart datadog-agent, which cascades a stop viaBindsTo) killed the process in single-digit milliseconds withcode=killed, signal=TERMand no shutdown-path log lines at all.systemctl stopproducesReceived SIGTERM, shutting down..., a clean topology drain (listeners and HTTP servers stopping),Agent Data Plane shut down successfully., and the process exits withstatus=0/SUCCESS.Closes #2319.
Test plan
agent-data-planebinary running as the actualdatadog-agent-data-plane.serviceunit now drains and exits cleanly onsystemctl stop/systemctl restart datadog-agent, versus being hard-killed before the fix.SIGTERMto the binary now triggers the same graceful-shutdown log sequence asSIGINT.