Repository navigation
fix(sidecar)!: make Windows RC notifications unload-safe - #2569
Conversation
cataphract
changed the title
Clippy Allow Annotation ReportTracked Clippy
By file and crateBy file
By crate
About This ReportThis report tracks Clippy allow annotations for specific rules, showing how they've changed in this PR. Decreasing the number of these annotations generally improves code quality. Panic-inducing macros in particular should be avoided. In the future, this report may become a PR-blocking quality gate. |
![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: 609a80ddbd
ℹ️ 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".
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 63de94e | Docs | View more details | Give us feedback! |
BenchmarksComparisonBenchmark execution time: 2026-09-29 15:56:29 Comparing candidate commit 63de94e in PR branch Found 4 performance improvements and 2 performance regressions! Performance is the same for 171 metrics, 0 unstable metrics.
|
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
bwoebi
left a comment
bwoebi
left a comment
There was a problem hiding this comment.
I haven't tested this out, but seeing CI working, and the fundamental idea being in line with the expectations I have towards the notification system, this gets a thumbs up from me!
Thank you Gustavo!
The sidecar previously called CreateRemoteThread in the client process with an entry point inside the extension DLL. A notification racing module shutdown could therefore execute code after the DLL was unloaded, causing an access violation in Unloaded_php_ddtrace.dll. Replace the remote thread with an auto-reset event transferred to the sidecar. The sidecar signals the event, while a client-owned thread-pool wait invokes the callback. Dropping the registration disarms the wait, cancels callbacks that have not started, and waits for any callback already running before releasing its state. It does not wait for sidecar work during shutdown. A stable ID lets the sidecar deduplicate repeated transfers of one notification. Each transfer may produce a different sidecar-local HANDLE value even though all duplicates refer to the same event, so the HANDLE value alone is not a stable identity. This removes the unload race and avoids creating a remote thread for every notification.
609a80d to
6117b96
Compare
|
/merge |
|
View all feedbacks in Devflow UI.
It will be processed automatically as soon as GitHub reports it as mergeable. View in MergeQueue UI.
The expected merge time in
This PR is rejected because it was updated |
|
/merge -c Still missing an approval |
|
View all feedbacks in Devflow UI.
Still missing an approval`
Still missing an approval`* Failed to generate input: unknown flag: - If you need support, contact us on Slack #devflow. with those details! |
winapi-rs is effectively unmaintained and has not received a commit since November 2021. Migrate direct consumers to the actively maintained windows-sys crate. The existing page_size and sysinfo dependencies are retained, so winapi remains in the lockfile transitively through both crates. See the upstream supersession discussion: retep998/winapi-rs#1055
bantonsson
left a comment
bantonsson
left a comment
There was a problem hiding this comment.
Solid fix. Only minor doc comment since the doc threw me off a bit.
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: rejected
and removed
mergequeue-status: in_progress
labels
|
/merge |
|
View all feedbacks in Devflow UI.
It will be processed automatically as soon as GitHub reports it as mergeable. View in MergeQueue UI.
The expected merge time in
|
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: waiting
and removed
mergequeue-status: rejected
labels
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
4 participants
What does this PR do?
Replace the Windows remote-config notification mechanism that called
CreateRemoteThreadin the client process with a client-owned auto-reset eventand Windows thread-pool wait.
The event handle is transferred to the sidecar over IPC. The sidecar signals
the event when configuration may have changed, and the client callback runs on
the process thread pool. Dropping the registration disarms the wait, cancels
callbacks that have not started, and drains any callback already running.
A stable notification ID lets the sidecar deduplicate repeated transfers of
the same event even when they have different sidecar-local
HANDLEvalues.Motivation
The previous sidecar implementation passed an extension DLL entry point to
CreateRemoteThread. A notification racing PHP module shutdown could executeafter the DLL was unloaded. The Windows VM Red scenario reproduced this as an
immediate
c0000005access violation in<Unloaded_php_ddtrace.dll>.The new client-owned registration makes module shutdown independent of sidecar
work while ensuring that callback code and state are no longer in use before
the DLL is unloaded. It also avoids creating a remote thread for every
notification.
Additional Notes
This PR is stacked on #2567 and uses its optional-handle IPC support. Review the
single commit on top of
glopes/ipc-changes. It should be retargeted tomainafter #2567 merges.
How to test the change?
Windows Red/Green integration validation
I prepared an adopting dd-trace-php checkout which:
ddog_sidecar_session_set_configfor each sidecarconnection; and
unloaded.
I built that extension on the Windows Server 2019 VM with MSVC. Each stress
iteration launches a short-lived PHP CLI process which starts tracing, asks a
test agent to publish an APM_TRACING configuration update, and waits for the
sidecar's next
/v0.7/configpoll. The test agent records that poll, and the PHPscript prints the
client.client_tracer.process_tagssent in its request body.Adding the configuration gives the sidecar an update to notify the extension
about; terminating the process immediately afterwards exercises that
notification against extension shutdown and DLL unload.
The harness runs those PHP processes under CDB with a subprocess sidecar and a
100 ms remote-config polling interval. Every iteration must both exit
successfully and print
runtime.sapi:cliamong the recorded process tags. Thetag assertion proves that the sidecar made the remote-config request; a clean
process exit without exercising remote config would not pass. CDB was
configured to catch access violations and failures in unloaded module code.
on its first iteration with
c0000005in<Unloaded_php_ddtrace.dll>+0x1118030. This reproduces the shutdown race: asidecar-created remote thread was still executing an extension entry point
after Windows had unloaded the extension DLL.
successfully and produced the expected tag. CDB reported no access violation,
execution in
<Unloaded_php_ddtrace.dll>, or other captured exception.Finally, I ran one process with CDB breakpoints on dd-trace-php's remote-config
MSHUTDOWN function and on
ddog_sidecar_remote_config_notification_drop. Both breakpoints fired, in thatorder, before the extension unloaded, and shutdown completed without an
exception. This verifies that the adopting caller invokes the draining drop
path at the point whose lifetime the fix is intended to protect.