fix(ipc)!: fix windows bugs; support optional handles - #2567
Conversation
try_send used non-overlapped WriteFile on a pipe in PIPE_WAIT. When a busy sidecar filled the pipe, the call could block the instrumented process instead of reporting backpressure. Temporarily selecting PIPE_NOWAIT avoided that block, but a full message pipe may report a successful write with zero bytes and silently lose the request. Keep the pipe in PIPE_WAIT and open the client endpoint for overlapped I/O, matching the server endpoint. Route every ReadFile and WriteFile operation, including the PID handshake, through one helper. Blocking calls wait for completion. Nonblocking calls cancel a pending request and wait only for its final status. Completion that wins the cancellation race remains successful; confirmed cancellation becomes WouldBlock. Waiting for a final status also keeps the OVERLAPPED structure and borrowed buffers valid until Windows has released them. Require the complete byte count so success is authoritative about whether the message entered the pipe. Add a regression test that fills a pipe and verifies try_send returns WouldBlock instead of hanging.
Windows handle transfer duplicates each source handle into the receiver before writing its numeric value into the pipe message. If a later duplication or the write fails, the receiver never sees the message and cannot close those duplicates. Track receiver-owned duplicates until the complete message write succeeds. On failure, move each duplicate back with DUPLICATE_CLOSE_SOURCE and close the local copy. On success, commit the guard so ownership remains with the receiver. Keep truncating the temporary handle suffix on every return so an unsent request can be retried from its original bytes, and reject handle counts that do not fit in the wire format. Also take ownership of a connected pipe before fallible setup and report failures to select message read mode instead of leaking the raw handle or continuing with an invalid connection.
The service macro treated every SerializedHandle parameter as a concrete value convertible into one PlatformHandle. That prevents a Windows-only notification target from being optional in the shared service protocol. Delegate handle copying and receiving to TransferHandles on the complete marked value. Option forwards those operations for Some and consumes no handles for None, while wrapper types can delegate to their contained handle-bearing value. Preserve cfg attributes in the generated match fields and transfer statements so a platform-specific handle parameter is omitted together with its field on other targets. Require marked parameter types to implement TransferHandles explicitly.
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. |
BenchmarksComparisonBenchmark execution time: 2026-09-28 15:44:12 Comparing candidate commit aaf3c80 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 2 metrics, 0 unstable metrics.
|
| cpu_model | git_commit_sha | git_commit_date | git_branch |
|---|---|---|---|
| Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz | aaf3c80 | 1790609694 | glopes/ipc-changes |
| scenario | metric | min | mean ± sd | median ± mad | p75 | p95 | p99 | max | peak_to_median_ratio | skewness | kurtosis | cv | sem | runs | sample_size |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| two way interface | execution_time | 15.228µs | 15.533µs ± 0.266µs | 15.501µs ± 0.078µs | 15.577µs | 15.787µs | 16.076µs | 18.571µs | 19.81% | 7.769 | 83.767 | 1.71% | 0.019µs | 1 | 200 |
| scenario | metric | 95% CI mean | Shapiro-Wilk pvalue | Ljung-Box pvalue (lag=1) | Dip test pvalue |
|---|---|---|---|---|---|
| two way interface | execution_time | [15.496µs; 15.570µs] or [-0.237%; +0.237%] | None | None | None |
Group 2
| cpu_model | git_commit_sha | git_commit_date | git_branch |
|---|---|---|---|
| Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz | aaf3c80 | 1790609694 | glopes/ipc-changes |
| scenario | metric | min | mean ± sd | median ± mad | p75 | p95 | p99 | max | peak_to_median_ratio | skewness | kurtosis | cv | sem | runs | sample_size |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| write only interface | execution_time | 1.066µs | 1.079µs ± 0.005µs | 1.079µs ± 0.002µs | 1.081µs | 1.085µs | 1.089µs | 1.098µs | 1.75% | -0.130 | 2.048 | 0.42% | 0.000µs | 1 | 200 |
| scenario | metric | 95% CI mean | Shapiro-Wilk pvalue | Ljung-Box pvalue (lag=1) | Dip test pvalue |
|---|---|---|---|---|---|
| write only interface | execution_time | [1.078µs; 1.079µs] or [-0.059%; +0.059%] | None | None | None |
Baseline
Baseline benchmark details
Group 1
| cpu_model | git_commit_sha | git_commit_date | git_branch |
|---|---|---|---|
| Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz | 7028079 | 1790605004 | main |
| scenario | metric | min | mean ± sd | median ± mad | p75 | p95 | p99 | max | peak_to_median_ratio | skewness | kurtosis | cv | sem | runs | sample_size |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| two way interface | execution_time | 15.402µs | 15.797µs ± 0.248µs | 15.770µs ± 0.101µs | 15.870µs | 16.127µs | 16.256µs | 18.346µs | 16.33% | 5.618 | 53.939 | 1.57% | 0.018µs | 1 | 200 |
| scenario | metric | 95% CI mean | Shapiro-Wilk pvalue | Ljung-Box pvalue (lag=1) | Dip test pvalue |
|---|---|---|---|---|---|
| two way interface | execution_time | [15.762µs; 15.831µs] or [-0.218%; +0.218%] | None | None | None |
Group 2
| cpu_model | git_commit_sha | git_commit_date | git_branch |
|---|---|---|---|
| Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz | 7028079 | 1790605004 | main |
| scenario | metric | min | mean ± sd | median ± mad | p75 | p95 | p99 | max | peak_to_median_ratio | skewness | kurtosis | cv | sem | runs | sample_size |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| write only interface | execution_time | 1.066µs | 1.084µs ± 0.005µs | 1.085µs ± 0.003µs | 1.088µs | 1.091µs | 1.096µs | 1.097µs | 1.05% | -0.670 | 0.730 | 0.48% | 0.000µs | 1 | 200 |
| scenario | metric | 95% CI mean | Shapiro-Wilk pvalue | Ljung-Box pvalue (lag=1) | Dip test pvalue |
|---|---|---|---|---|---|
| write only interface | execution_time | [1.084µs; 1.085µs] or [-0.067%; +0.067%] | None | None | None |
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. |
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: aaf3c80 | Docs | View more details | Give us feedback! |
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'll trust you that this is correct.
It seems to still work, but I'm not an expert to the degree here that I can tell that the specific write operations are correct.
The important invariant is that it's still non-blocking and the overlapped IO guarantees that.
|
The only questionable thing here, as I signaled in the description, is the i/o cancellation. I still need to run stress tests on wine and windows. |
|
@bwoebi Unfortunately, while this works fine on Wine, the same is not true for windows. It's possible, even though unlikely (in my test it happened 8 times in 400k writes), that:
I'll looking for alternatives EDIT, further test:
so around 0.001% of the "aborts" actually completed |
|
@bwoebi OK, these my tentative interim conclusions, focused on the client code (the server operations have their own problems, like parking one thread per idle client, but let's set that aside):
|
CancelIoEx cannot safely turn a pending named-pipe write into Unix-style WouldBlock. NPFS may expose the complete buffer to the peer before cancellation claims completion. It can then report ERROR_OPERATION_ABORTED even though the message was delivered. Retrying can duplicate messages and transferred handles. Give each Windows connection one thread-pool I/O-backed write slot. Keep the encoded buffer, OVERLAPPED state, and handle-transfer state alive until terminal completion. Retire immediate writes inline and latch asynchronous errors. Blocking senders wait for the slot or completion; nonblocking senders reject before submission. Teardown cancels pending I/O and quiesces callbacks before closing the pipe. Move encoded Vec ownership through the connection APIs so the pending slot can retain the caller allocation. Generated IPC encoding reserves the exact Windows handle suffix, avoiding allocation when metadata is appended. Keep read cancellation separate because NPFS read cancellation is rollback-safe. Cover repeated backpressure, ordered delivery, concurrent blocked reads and writes, cancellation, peer closure, allocation ownership, and teardown on native Windows.
d787170 to
186c31b
Compare
Creating and closing an event for every overlapped read adds avoidable kernel-handle churn, especially under Wine. Keep one completed event per connection and claim or return it through a pointer-sized atomic slot. Concurrent readers use independent events instead of sharing or serializing (there's not much use for concurrent blocking readers, but kept for correctness/completeness -- in practice, the cached event will always be used). Refactor the reader to a dedicated module.
|
Keep using This effectively enlarges the pipe for writes, allowing one more message beyond its capacity. The other path I considered, the undocumented
|
|
/merge |
|
View all feedbacks in Devflow UI.
It will be processed automatically as soon as GitHub reports it as mergeable. View in MergeQueue UI.
gustavo.lopes@datadoghq.com unqueued this merge request |
|
/merge -c pending review |
|
View all feedbacks in Devflow UI.
|
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: removed
and removed
mergequeue-status: waiting
labels
yannham
left a comment
yannham
left a comment
There was a problem hiding this comment.
A lot of this is out of my expertise tbh, so I've only done a bird eye's review. I'll trust sidecar people for the technical details.
a838932 to
aaf3c80
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
|
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
# What does this PR do? Replace the Windows remote-config notification mechanism that called `CreateRemoteThread` in the client process with a client-owned auto-reset event and 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 `HANDLE` values. # Motivation The previous sidecar implementation passed an extension DLL entry point to `CreateRemoteThread`. A notification racing PHP module shutdown could execute after the DLL was unloaded. The Windows VM Red scenario reproduced this as an immediate `c0000005` access 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 to `main` after #2567 merges. # How to test the change? ## Windows Red/Green integration validation I prepared an adopting [dd-trace-php checkout](DataDog/dd-trace-php@3df08d9) which: - creates one process-wide notification registration during PHP MINIT; - gives the registration to `ddog_sidecar_session_set_config` for each sidecar connection; and - drops the registration during PHP MSHUTDOWN, before the extension DLL is 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/config` poll. The test agent records that poll, and the PHP script prints the `client.client_tracer.process_tags` sent 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:cli` among the recorded process tags. The tag 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. - **Red:** with the pre-fix libdatadog implementation, the same workload failed on its first iteration with `c0000005` in `<Unloaded_php_ddtrace.dll>+0x1118030`. This reproduces the shutdown race: a sidecar-created remote thread was still executing an extension entry point after Windows had unloaded the extension DLL. - **Green:** with this PR and the adopting caller, all 321/321 iterations exited 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 that order, 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. Co-authored-by: gustavo.lopes <gustavo.lopes@datadoghq.com>
4 participants
What does this PR do?
PIPE_NOWAITmode to simulate unix-style nonblocking I/O to (unsuccessfully) try to achieve this. Use insteadPIPE_WAITand overlapped I/O (the way the pipe is already opened) and, in a somewhat hacky fashion, to simulate Unix behavior, cancel pending requests when the operation returnsERROR_IO_PENDING.Motivation
See DataDog/dd-trace-php#4219