fix(profiling): avoid crashing stack sampler with active foreign SIGSEGV handlers (PROF-15342) - #18798
Conversation
Codeowners resolved as |
🎉 All green!🧪 All tests passed 🔗 Commit SHA: 98c1492 | Docs | Datadog PR Page | Give us feedback! |
d6649ac to
127a251
Compare
|
@codex review plz |
There was a problem hiding this comment.
Pull request overview
Hardens the stack v2 native sampler against crashes when another library takes over SIGSEGV (e.g., torch/CUDA, abseil), by preferring a syscall-based copy during startup and only using safe_memcpy when the profiler still owns the fault handlers.
Changes:
- Add a configurable warmup window to start with syscall-based memory copying and then conditionally upgrade to
safe_memcpy. - Re-check signal-handler ownership during sampling and permanently fall back to syscall copy if a foreign handler takes over.
- Add a regression test and a standalone repro script for the foreign-handler scenario; include a release note.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/profiling/test_main.py | Adds subprocess regression test ensuring sampler degrades under a foreign SIGSEGV handler. |
| scripts/profiling/repro_prof_14568_foreign_handler.py | Adds a manual repro script to validate behavior with synthetic or torch-installed handlers. |
| releasenotes/notes/profiling-stack-sampler-foreign-handler-fallback-3d8f1b6a0e25c947.yaml | Documents the crash hardening and new warmup behavior. |
| ddtrace/internal/datadog/profiling/stack/src/sampler.cpp | Implements warmup + runtime ownership checks and fallback behavior. |
| ddtrace/internal/datadog/profiling/stack/src/echion/danger.cc | Adds handler-ownership probe used by the sampler. |
| ddtrace/internal/datadog/profiling/stack/echion/echion/danger.h | Declares the new handler-ownership probe API. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 127a251509
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
4f3001a to
6a7790b
Compare
Circular import analysis
|
Address review feedback on PR #18798: - sampling_thread only (re)arms our SIGSEGV/SIGBUS handlers via init_segv_catcher when we still own them. If a foreign component (abseil via vLLM/gRPC, torch/CUDA) took over the dispositions before the sampling thread started, we no longer overwrite it - which had made segv_handler_installed() falsely report ownership and defeated the warmup/fallback. We now leave the foreign handler authoritative and stay on the safe syscall copy. - Reduce the fast-copy stat test deadline from 45s to 30s for faster CI feedback (warmup window is 15s).
Address review feedback on PR #18798: - sampling_thread only (re)arms our SIGSEGV/SIGBUS handlers via init_segv_catcher when we still own them. If a foreign component (abseil via vLLM/gRPC, torch/CUDA) took over the dispositions before the sampling thread started, we no longer overwrite it - which had made segv_handler_installed() falsely report ownership and defeated the warmup/fallback. We now leave the foreign handler authoritative and stay on the safe syscall copy. - Reduce the fast-copy stat test deadline from 45s to 30s for faster CI feedback (warmup window is 15s).
6506c75 to
4f65361
Compare
Address review feedback on PR #18798: - sampling_thread only (re)arms our SIGSEGV/SIGBUS handlers via init_segv_catcher when we still own them. If a foreign component (abseil via vLLM/gRPC, torch/CUDA) took over the dispositions before the sampling thread started, we no longer overwrite it - which had made segv_handler_installed() falsely report ownership and defeated the warmup/fallback. We now leave the foreign handler authoritative and stay on the safe syscall copy. - Reduce the fast-copy stat test deadline from 45s to 30s for faster CI feedback (warmup window is 15s).
- Note the intentional call_once on init_segv_catcher (auto-fallback policy, not reinstall-and-chain) so the handler-chaining races from PROF-14568 are not reintroduced. - Expand the release note to mention SIGBUS, the stop-sampling fallback, and the complementary faulthandler integration.
Address review feedback on PR #18798: - sampling_thread only (re)arms our SIGSEGV/SIGBUS handlers via init_segv_catcher when we still own them. If a foreign component (abseil via vLLM/gRPC, torch/CUDA) took over the dispositions before the sampling thread started, we no longer overwrite it - which had made segv_handler_installed() falsely report ownership and defeated the warmup/fallback. We now leave the foreign handler authoritative and stay on the safe syscall copy. - Reduce the fast-copy stat test deadline from 45s to 30s for faster CI feedback (warmup window is 15s).
The foreign-handler repro is kept for reference on the stacked branch vlad/prof-14568-repro-script; it is not part of the shipped fix.
Fixes the prechecks style step, which pins clang-format 18.1.5 and splits
`struct X{}` aggregate initializers onto their own line. The local pre-commit
hook used a different clang-format version (Apple 17), so the skew slipped
through locally.
Mirror the segv_handler_installed declaration into the native _stack extension stub so both stubs agree (addresses Copilot review on 18798).
Replace the metadata-file scraping in test_fast_copy_memory_enabled with an in-process check and a fast, deterministic warmup: - Add fast_copy_memory_active() native getter so tests can read the live copy mode directly instead of polling *.internal_metadata.json (which raced with mid-write files and depended on the upload cadence). Mirrors the existing segv_handler_installed()/is_safe_copy_failed() introspection. - Add a test-only _set_fast_copy_warmup_seconds() knob (underscore-prefixed, accessed via the _stack submodule) so the warmup->safe_memcpy upgrade can be observed in ~1s instead of waiting out the 15s production default. The default warmup is now a Sampler member (15s) rather than a file-local constant. The test now shrinks the warmup, confirms the sampler runs on the syscall copy during warmup, then confirms it upgrades to safe_memcpy, all via the getter.
…check The clang-tidy profiling job fails on clang-analyzer-optin.performance.Padding (warnings-as-errors): class Datadog::Sampler had 35 padding bytes where 3 is optimal after adding fast_copy_warmup_seconds. Reorder the data members into the alignment-descending order clang-tidy reports as optimal (pointers/atomics/8-byte scalars, then vectors, mutexes, condvars, then 4-byte and bool fields). Layout only; no behavior change (the constructor initializes only echion, which stays first, so no -Wreorder).
… unavailable Address review feedback on PR #18798: - _set_fast_copy_warmup_seconds now rejects non-finite (NaN/inf) and negative durations with a ValueError, instead of letting a nonsensical warmup deadline through this Python-exposed entry point. - test_stack_profiler_foreign_segv_handler_detection now skips when the native stack extension is unavailable (stack.is_available is False), avoiding an AttributeError on builds/platforms without it.
68d3838 to
fd5fd49
Compare
e083509
into
main
…port 4.12] (PROF-15342) (#18993) ## Summary 4.12 never received #18798 (#18971/#18972). This backport adds: 1. Foreign SIGSEGV/SIGBUS handler warmup/fallback (cherry-pick of #18972, conflicts resolved with 4.12 adaptive sampling) 2. Fast-copy fallback ProfilerStats metrics (d6ee9b5) ## Test plan - CI green on 4.12 profiling suites - Handler backport: merged fast_copy_warmup_seconds with 4.12 p_stable fields in sampler.hpp - Metrics cherry-pick applied cleanly on top ## Risks Low-medium: behavior change only when fast copy is enabled and a foreign handler is present.
| Next PR
User-reported crash: https://datadoghq.atlassian.net/browse/AIPTS-1715
Description
safe_memcpy's fault recovery only works while the profiler owns theSIGSEGV/SIGBUShandlers. Libraries like PyTorch/CUDA (and abseil via vLLM/gRPC) install their own
handler during startup; once a foreign handler owns those signals, a fault on a stale
read is no longer recovered and the process crashes (PROF-14568).
This PR makes the default-on state safe and removes the need for the
_DD_PROFILING_STACK_FAST_COPY=0workaround. It handles the handlers we cannot wrap (torch/CUDA/abseil) via detect-and-fallback.
Changes
process_vm_readv/mach_vm_read_overwrite)for a short warmup, so a fault during crash-prone startup can't crash the process (these
syscalls return an error instead of faulting).
safe_memcpyonly if we still own bothSIGSEGVandSIGBUS(segv_handler_installed()).a handler is taken over later (e.g. lazy CUDA init). If no safe fallback exists
(
process_vm_readvblocked), it stops sampling — we degrade to dropped samples,never a crash.
reinstall-and-chain, so the foreign handler stays authoritative;
init_segv_catcherstays
call_onceto avoid reintroducing handler-chaining races.Test plan
New unit tests
Manual repro (synthetic +
--torch) on Vlad/prof 14568 repro script #18911):crashes on
main, runs OK on this branchTested on another internal service in staging:
ai_gateway(profile link)Py CPU
Testing in User Environment: