Repository navigation
fix(profiling): release the profile lock before the upload request [backport 3.19] - #20771
Draft
bvolpato-dd wants to merge 11 commits into
Draft
bvolpato-dd wants to merge 11 commits into
bvolpato-dd wants to merge 11 commits into
Conversation
…ackport 3.19] Manual backport of 441fc1c from DataDog#15302 to 3.19. ## Description A slow or unresponsive Datadog Agent could stop every Python thread of the application during a profile upload. `ddup_upload` kept the profile lock during the complete call of `Uploader::upload`, which includes the HTTP request (`ddog_prof_Exporter_send`). Each thread that adds a sample needs the same lock in `Profile::collect`. The lock profiler and the memory profiler add samples while they hold the GIL. When the Agent answered late, one of these threads waited for the lock with the GIL, and all other Python threads waited for the GIL. The stop lasted until the Agent answered or the upload timed out (`DD_PROFILING_API_TIMEOUT_MS`, 10 seconds by default). `UploaderBuilder::build` now serializes the profile, which also resets it, and holds the profile lock only for that operation. The `Uploader` owns the encoded profile, and `Uploader::upload` sends the request without the profile lock. The upload sequence number, the cancellation of an in-flight upload, the fork handlers and the upload timeout do not change. Differences from the commit on main: - The 3.19 branch has no `ProfilerStats` and no `ProfileBorrow`, so the port does not include these parts. It uses `Sample::profile_borrow()` and `Sample::profile_release()`. - `ddup_upload` continues to return `true` after an upload attempt, as before on 3.19. - `Uploader::upload` continues to free the encoded profile immediately after `ddog_prof_Exporter_Request_build`, as before on 3.19. - If serialization fails, `ddup_upload` now reports the error at most one time as "Failed to create uploader". Before, `Uploader::upload` printed "Error serializing pprof" for each failure. The original commit has no release note. This backport adds one. ## Testing `test_upload_does_not_block_sample_flush` starts a local HTTP server that keeps the profile request open. The test adds a sample while the upload waits for the response. Without the fix, `flush_sample()` waited 2.9 seconds, which is the upload timeout of the test. With the fix, the test passes in less than 1 second. Measurement on macOS arm64 with Python 3.13: a thread heartbeat and an asyncio heartbeat of 10 ms, a thread that uses one `threading.Lock`, an upload interval of 10 seconds, and two runs of 45 seconds for each case. The table shows the largest heartbeat gap and the number of gaps of 1 second or more. | Agent behavior | 3.19.8 | This change | |---|---|---| | Reads the request, never answers | 9937 ms and 9897 ms, 2 stops in each run | 50 ms and 21 ms, no stop | | Answers after 3 seconds | 2991 ms and 2960 ms, 3 stops in each run | 46 ms and 15 ms, no stop | | Answers immediately | 49 ms and 17 ms, no stop | 51 ms and 21 ms, no stop | When the Agent does not answer, the upload still fails after the timeout and prints `Error uploading (ddog_prof_Exporter_send failed: operation timed out)`. When the Agent answers, it receives the same number of profile requests as with 3.19.8 (4 in each run), with bodies of 5.1 kB to 8.4 kB. ## Risks The scheduler thread still waits for the upload. Only the profile lock is released earlier. Samples that arrive during the request now go to the next profile. (cherry picked from commit 441fc1c, ported by hand) Co-Authored-By: Claude Code <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
KowalskiThomas
added
the
Backport
The repository policy rejects each action that is not pinned to a full commit SHA. The policy also applies to the actions that a composite action calls. Two actions of this branch call another action by tag, so their jobs failed in "Set up job": - actions-rust-lang/setup-rust-toolchain v1.12.0 calls Swatinem/rust-cache@v2. Version v1.17.0 pins rust-cache to a commit SHA. - pypa/cibuildwheel v3.1.4 calls actions/setup-python@v5. The build job now installs cibuildwheel 3.1.4 with pip, as the build-wheels-matrix job already does. The cibuildwheel version does not change. Co-Authored-By: Claude Code <noreply@anthropic.com>
…a fork The token of a pull request from a fork is read-only. The "Comment PR" step then fails with "Resource not accessible by integration", and the "Report codeowners" check fails. The job still prints the resolved owners in the "List owners of all changed files" step. Co-Authored-By: Claude Code <noreply@anthropic.com>
…nutes A valgrind job of this workflow usually takes 7.5 to 8.5 minutes, and the limit was 9 minutes. One job exceeded the limit after all its tests passed, because "Install Valgrind" took 3 minutes 44 seconds instead of about 11 seconds. Co-Authored-By: Claude Code <noreply@anthropic.com>
…-local data
A uWSGI worker could abort with SIGABRT during a graceful shutdown:
The Tokio context thread-local variable has been destroyed.
panic in a function that cannot unwind
uWSGI finalizes Python in an atexit handler. The profiler stops there and uploads the last profile. At that time, exit() has already run the thread-local destructors of the calling thread. libdatadog keeps the state of its HTTP client in thread-local data, and `ddog_prof_Exporter_send` panics when that state is destroyed.
The abort is not new. A uWSGI worker with `ddtrace==3.19.8` aborted in 6 of 6 graceful shutdowns in a local test. If a periodic upload occurred before the shutdown, the last upload instead failed earlier with `Error building request (ddog_prof_Exporter_Request_build failed: failed to create compressor: size hint shouldn't be larger than max capacity)`, because it read the code provenance string after exit() destroyed it. This is why `system-tests (uwsgi-poc, other)` failed only in some runs.
`ddup_upload` now returns without an upload when the thread-local destructors of the calling thread have run. A thread-local sentinel object records this. The thread that calls `ddup_start` first and each thread that calls `ddup_upload` have a sentinel. An upload during a normal Python exit does not change, because Python stops the profiler before exit().
`test_upload_during_exit_does_not_abort` calls `ddup.upload()` from a handler of `__cxa_atexit`. Without the fix, the test process aborts.
Co-Authored-By: Claude Code <noreply@anthropic.com>
The `parametric` job failed on each pull request to 3.19 with 3 failures of `TestRemoteConfigApplyEndpoint`:
remote config apply failed: AttributeError("'RemoteConfigClient' object has no attribute '_global_subscriber'")
The parametric server of the pinned system-tests calls `RemoteConfigClient._global_subscriber`, which does not exist on the 3.19 branch. The system-tests manifest has no entry for this test class, so the tests run for all versions.
The `Run` step now deselects this test class. The job stays `continue-on-error`, as before.
Co-Authored-By: Claude Code <noreply@anthropic.com>
A child process could wait forever in its first profile upload. This is the cause of the `valgrind` jobs of `Profiling Native Tests with Sanitizers` that hang in `ForkDeathTest.*` until the time limit. `ddup_upload` took the upload lock only for the upload request. The prefork handler takes the same lock, so fork() could occur while another thread was in `UploaderBuilder::build`. If that thread was in `ddog_prof_Exporter_new` and initialized the connector of libdatadog for the first time, the child process received this state as "initialization in progress". No thread of the child completes it, so the next `ddog_prof_Exporter_new` of the child waited forever. The child also received a copy of the exporter that it could not free. `ddup_upload` now takes the upload lock before it builds the uploader, and releases the lock after it destroys the uploader. `Uploader::upload` does not take the lock, because the caller holds it. This is the lock change of c674c8b (DataDog#16153) on main, without the test changes of that commit. A fork() now also waits for `UploaderBuilder::build`. Before, it waited only for the upload request. Local test with valgrind, 8 runs in parallel, `ForkDeathTest.SampleInThreadsAndForkNormal`: 9 of 16 runs hung without this change, and 0 of 16 with it. `ForkDeathTest.*` with this change: 63 of 64 runs passed. One run failed on a leak report for thread-local data of the upload thread of the parent, which is a different problem: the fork occurred while that thread ended. Co-Authored-By: Claude Code <noreply@anthropic.com>
The `thread` jobs of `Profiling Native Tests with Sanitizers` failed in `ForkDeathTest.*` after the upload lock change:
WARNING: ThreadSanitizer: thread leak
Thread T1 (finished) created by main thread at:
DataDog#1 sample_in_threads_and_fork(unsigned int, unsigned int) test_forking.cpp:97
`ddup_upload` now holds the upload lock while it builds the uploader, so fork() waits until the upload thread of the test completes its upload. That thread has then usually ended when the fork occurs. The child process has no such thread, but it has the ThreadSanitizer record of a thread that ended and that nobody joined.
The test now detaches the upload thread and does not join it. This is the test change of c674c8b (DataDog#16153) on main.
Local test with ThreadSanitizer and a delay of 400 ms before the fork, so that the upload thread ends first: `ForkDeathTest.*` failed in 12 of 12 runs without this change and passed in 12 of 12 runs with it.
Co-Authored-By: Claude Code <noreply@anthropic.com>
Author
|
Merged Local validation passed: workflow parsing, actionlint, formatting, and a build of the merged C++ profiling wrapper on macOS arm64 with Python 3.13. The existing slow-upload regression test passed with that rebuilt wrapper installed over the 3.19.8 wheel. The earlier CI failure report refers to |
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
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.
Manual backport of #15302 to 3.19, with the fork-safety guard from #16153.
A slow Agent could hold the profile lock for the HTTP request. A thread waiting for that lock while holding the GIL could stop the application. This change serializes and resets the profile under the lock, then releases it before the request. Samples collected during the request go into the next profile.
The upload lock also covers exporter construction and destruction, so a fork cannot copy a partially initialized exporter. Fork handlers also protect initialization and code provenance before the profiler starts. Code provenance uses a protected snapshot and remains available to late exit callbacks. A watched thread skips upload after its thread-local state starts to shut down.
Validation
Limits and CI
A fork can still wait for an ongoing upload or its timeout. The exit guard cannot detect teardown on a thread that the profiler has never watched.
In the native fork tests, leak-checking builds wait for the upload thread before forking, because the child inherits thread-local allocations that leak checkers report as lost. ThreadSanitizer and unsanitized builds still allow the fork and upload to overlap.
The branch retains the 3.19 CI workarounds: skip the Codeowners comment for forked pull requests, extend the native-test timeout, and deselect incompatible remote-configuration cases in the non-gating parametric job.