fix(profiling): maintain fast-copy and foreign-takeover fallback across forks (PROF-14568) - #18947
fix(profiling): maintain fast-copy and foreign-takeover fallback across forks (PROF-14568)#18947vlad-scherbich wants to merge 1 commit into
Conversation
Codeowners resolved as |
Circular import analysis
|
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f149185bb9
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
This PR improves robustness of the native stack profiler’s “fast copy” (safe_memcpy) mode by separating persistent configuration intent from transient runtime state, and by tightening SIGSEGV/SIGBUS handler ownership checks across warmup, fork, and mid-cycle handler takeovers.
Changes:
- Introduces a persistent
fast_copy_desiredflag distinct fromfast_copy_active, and updates configuration/initialization to maintain intent across warmup/fallbacks/fork. - Updates SIGSEGV/SIGBUS handler uninstall/reinstall and warmup/upgrade logic to key off
fast_copy_desiredinstead of the transient active state. - Adds per-thread ownership re-checks right before sampling to narrow the crash window from mid-cycle foreign handler takeovers.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| releasenotes/notes/profiling-stack-sampler-foreign-handler-warmup-fork-2c9d4e7a1f6b0853.yaml | Release note describing improved handler-ownership robustness across warmup, fork, and mid-cycle swaps. |
| ddtrace/internal/datadog/profiling/stack/src/stack.cpp | Records persistent fast-copy intent; gates handler uninstall/reinstall on intent rather than transient active state. |
| ddtrace/internal/datadog/profiling/stack/src/sampler.cpp | Uses persistent intent for warmup and handler install; adds per-thread “ensure safe” checks to detect mid-cycle takeovers. |
| ddtrace/internal/datadog/profiling/stack/src/echion/vm.cc | Sets fast_copy_desired alongside fast_copy_active when safe memcpy initializes successfully. |
| ddtrace/internal/datadog/profiling/stack/echion/echion/vm.h | Declares fast_copy_desired as the persistent configuration state. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
4b02113 to
e677103
Compare
1d4a449 to
a986735
Compare
|
This change is marked for backport to 4.11 and it does not conflict with that branch. |
|
This change is marked for backport to 4.10, but it conflicts with that branch. The command used to test backporting was |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 12 comments.
Comments suppressed due to low confidence (1)
tests/profiling/collector/test_copy_memory_stats.py:85
- Add an availability guard for the stack v2 native extension (
_stack_ext.is_available) so this subprocess test is skipped cleanly when the extension isn't built.
@pytest.mark.skipif(sys.platform == "win32", reason="stack v2 profiler is not available on Windows")
@pytest.mark.subprocess(
env=dict(
DD_PROFILING_OUTPUT_PPROF="/tmp/test_fast_copy_memory_enabled",
DD_PROFILING_UPLOAD_INTERVAL="1",
_DD_PROFILING_STACK_FAST_COPY="1",
),
err=None,
)
There was a problem hiding this comment.
Is that PR ready for review?
I find it concerning that this follow-up is almost as large (in changes count) as the original PR
There was a problem hiding this comment.
Is that PR ready for review?
Yes, you can begin if you have the time. I was waiting to open it while going over the code one more time.
There was a problem hiding this comment.
I find it concerning that this follow-up is almost as large (in changes count) as the original PR
It is not small, but tests and release note take up almost 300 LoC (290ish to be precise). So the actual code changes are "only" ~330 LoC. The reason it's so large is we try to handle lots of different edge cases with forking that you raised in the parent PR. We also make the fault-handler ownership logic more robust, per the 3 Codex recommendations from the parent PR.
The good news is that this PR is not required to fix the customer crashes. It's complimentary future-proofing the code.
e4ec908 to
fcdda10
Compare
1ac266b to
6aea597
Compare
cb75c54 to
34501b0
Compare
e1cee95 to
68d3838
Compare
34501b0 to
0253d11
Compare
68d3838 to
fd5fd49
Compare
0253d11 to
d506b08
Compare
…EGV handlers (PROF-15342) (#18798) | [Next PR](#18947) User-reported crash: https://datadoghq.atlassian.net/browse/AIPTS-1715 ## Description `safe_memcpy`'s fault recovery only works while the profiler owns the `SIGSEGV`/`SIGBUS` handlers. 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](https://datadoghq.atlassian.net/browse/PROF-14568)). This PR makes the default-on state safe and removes the need for the `_DD_PROFILING_STACK_FAST_COPY=0` workaround. It handles the handlers we *cannot* wrap (torch/CUDA/abseil) via detect-and-fallback. ## Changes * Sampler **starts on the safe syscall copy** (`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). * After warmup it **upgrades to `safe_memcpy` only if we still own both `SIGSEGV` and `SIGBUS`** (`segv_handler_installed()`). * It **re-checks ownership every cycle** and permanently falls back to the syscall copy if a handler is taken over later (e.g. lazy CUDA init). If no safe fallback exists (`process_vm_readv` blocked), it **stops sampling** — we degrade to dropped samples, never a crash. * Warmup is a fixed 15s internal constant (not a user knob). Policy is auto-fallback, not reinstall-and-chain, so the foreign handler stays authoritative; `init_segv_catcher` stays `call_once` to avoid reintroducing handler-chaining races. ## Test plan * New unit tests * Manual repro (synthetic + `--torch`) on #18911): crashes on `main`, runs OK on this branch * Tested on another internal service in staging: `ai_gateway` ([profile link](https://ddstaging.datadoghq.com/profiling/explorer?query=service%3Aai_gateway%20env%3Astaging%20version%3Afaulthandling-1ac266be&my_code=disabled&profile_type=heap-live-size&refresh_mode=paused&viz=flame_graph&from_ts=1784830809012&to_ts=1784834409012&live=false)) * Py CPU <img width="1004" height="1024" alt="image" src="https://github.com/user-attachments/assets/5b4311bf-ee2b-4dd0-bb1f-b23f05ca567d" /> * eBPF CPU <img width="999" height="1024" alt="image" src="https://github.com/user-attachments/assets/ada757ba-8e35-48c0-ba97-e6972f69549b" /> * Live Heap <img width="1009" height="1024" alt="image" src="https://github.com/user-attachments/assets/c7161fad-e922-43d6-b7c8-5cfbc0706cd0" /> ### Testing in User Environment: * User deployed a binary with this and next PR (#18797) to their service; no crashes observed in 24 hours. (cc @askardog ) [PROF-14568]: https://datadoghq.atlassian.net/browse/PROF-14568?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ Co-authored-by: KowalskiThomas <thomas.kowalski@datadoghq.com> Co-authored-by: vlad.scherbich <vlad.scherbich@datadoghq.com>
9117001 to
e957ddf
Compare
…ss forks (PROF-14568) Follow-up to #18798: add persistent fast-copy intent flags, fork-safe warmup re-run, sticky foreign-takeover inheritance, prefork sampling pause, and per-cycle handler ownership checks. Publish fast-copy metadata via ProfilerState snapshots and extend fork/warmup coverage in tests.
e957ddf to
46812e9
Compare
< Prev PR
Summary
Follow-up to #18798 :
New behaviors:
prefork()adds a brief sampling pause aroundfork()in processes that use the stack profiler'spthread_atforkhook — same pattern as faulthandler handler swaps. Ifpause()times out (3s), fork proceeds without the pause guarantee (unchanged best-effort edge case).Changes
fast_copy_desired, distinct from the transientfast_copy_active. Set viaset_fast_copy()/ at constructor time; not touched by warmup or fallback; survivesfork().uninstall/reinstall_segv_handlergate onfast_copy_desired, so afaulthandlerswap during warmup is reclaimed instead of leaving a foreign handler in place.safe_memcpyinstead of inheritingfast_copy_active == falsefor life.fast_copy_foreign_takeoversurvivesfork(), so a child of a parent that already fell back stays on the syscall copy and cannot reclaim our handler or re-enable fast copy.prefork()pauses the sampling thread (same mechanism as faulthandler) sofork()does not span an in-flightsafe_memcpy;postfork_parent()resumes it. Child path resets viapostfork_child/restart_after_fork.thread.sample()while fast copy is active, narrowing the crash window for a lazily-installed foreign handler (CUDA/vLLM-style) from a whole sampling cycle to a single thread. Stickyfast_copy_foreign_takeoverblocks reclaim after fallback.Testing
[x] Tests passed locally (py3.12):
tests/profiling/test_main.py::test_stack_profiler_foreign_segv_handler_detectiontests/profiling/test_faulthandler.pytests/profiling/collector/test_copy_memory_stats.pytests/profiling/test_main.py