fix(loader): avoid background dlclose at shutdown by cataphract · Pull Request #4229 · DataDog/dd-trace-php

cataphract · 2026-09-24T13:23:38Z

The telemetry reaper pins the loader with dlopen() so it can keep waiting after Zend shuts down. Its eventual dlclose() can overlap glibc's atexit handlers on another thread and race libdatadog teardown.

But if we don't issue dlopen(), then we don't need to call dlclose(), and the problem goes away.

So copy a small x86-64 or AArch64 reaper and its context into a separate executable mapping before fork. The detached thread waits for its child with EINTR retries, then tail-calls libc's munmap() to release the mapping and return directly to pthread cleanup. It no longer pins the loader or calls dlclose(), so Zend can unload the library while telemetry is still running.

Validation: built the loader and ran the restored functional test on native AArch64 and ubuntu-vm amd64. Reaper checks also covered EINTR, unload/reload, CET/BTI/GCS properties and BTI enforcement under QEMU.

Description

Reviewer checklist

Test coverage seems ok.
Appropriate labels assigned.

The telemetry reaper pins the loader with dlopen() so it can keep waiting after Zend shuts down. Its eventual dlclose() can overlap glibc's atexit handlers on another thread and race libdatadog teardown. Tail-calling dlclose protects the reaper's return address, but does not serialize these destructor paths. Joining the thread during module shutdown avoids that race but makes short-lived CLI processes wait for telemetry delivery. Keeping the loader permanently mapped would preserve stale state across Apache reloads. Copy a small x86-64 or AArch64 reaper and its context into a separate executable mapping before fork. The detached thread waits for its child with EINTR retries, then tail-calls libc's munmap() to release the mapping and return directly to pthread cleanup. It no longer pins the loader or calls dlclose(), so Zend can unload the library while telemetry is still running. Clean up preparations in the child and on fork failures. Skip telemetry if setup fails, reap synchronously if thread creation fails, and use _exit() if exec fails to avoid inherited exit handlers. Preserve ELF hardening properties and enable BTI on the copied page when the AArch64 kernel reports support. Restore the shutdown regression with a gated forwarder: PHP must exit before the test releases telemetry children. Validation: built the loader and ran the restored functional test on native AArch64 and ubuntu-vm amd64. Reaper checks also covered EINTR, unload/reload, setup failures, CET/BTI/GCS properties and BTI enforcement under QEMU.

chatgpt-codex-connector · 2026-09-24T13:23:55Z

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-24T13:30:25.686229Z	`78ce2d7`	PR opened
🔒 Security Review	✅ Completed 2026-09-24T13:30:13.706708Z	`78ce2d7`	PR opened

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

datadog-prod-us1-4 · 2026-09-24T13:29:07Z

✨ Unblock PR with BitsAI

❌ Errors

Your PR has failed checks. Please review the issues below and take necessary action before merging.

🚦 9 Pipeline jobs failed

DataDog/apm-reliability/dd-trace-php | test_web_drupal_101: [8.1, cli-server] — ❄️ 2 known flaky tests

View more details · View in GitLab

↳ ❄️ Known flaky: testScenarioGetToMissingRoute from tests/Integrations/Drupal/V10_1.DDTrace\Tests\Integrations\Drupal\V10_1\CommonScenariosTest

↳ ❄️ Known flaky: testScenarioGetWithView from tests/Integrations/Drupal/V10_1.DDTrace\Tests\Integrations\Drupal\V10_1\CommonScenariosTest

DataDog/apm-reliability/dd-trace-php | Extension Tea Tests: [7.0, nts] — 🔧 Needs a code fix, caused by this PR

View more details · View in GitLab

DataDog/apm-reliability/dd-trace-php | Extension Tea Tests: [7.4, debug] — 🔧 Needs a code fix, caused by this PR

View more details · View in GitLab

View all 9 failed jobs.

ℹ️ Info

No other issues found (see more)

❄️ No new flaky tests detected

🎯 Code Coverage (details)
• Patch Coverage: 100.00%
• Overall Coverage: 68.25% (+0.00%)

Useful? React with 👍 / 👎

_{This comment will be updated automatically if new data arrives.

🔗 Commit SHA: dfd83d0 | Docs | View more details | Give us feedback!}

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 78ce2d7201

ℹ️ 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".

bwoebi · 2026-09-24T15:08:00Z

I approve of the approach of that PR. I'll look at it more in depth later.

Avoiding the asm trampoline

bwoebi · 2026-09-25T15:31:54Z

@cataphract The idea is right, but the implementation far more complex than needed: https://github.com/DataDog/dd-trace-php/compare/bob/ssi-shutdown-crash?expand=1

If you like this, I'll cherry-pick that onto this PR.

cataphract · 2026-09-25T17:28:01Z

@cataphract The idea is right, but the implementation far more complex than needed: https://github.com/DataDog/dd-trace-php/compare/bob/ssi-shutdown-crash?expand=1

If you like this, I'll cherry-pick that onto this PR.

I'm OK with it.

bwoebi

Pushed my commit then and acking.

bwoebi · 2026-09-25T18:15:44Z

Thanks for setting up this patch :-)

pr-commenter · 2026-09-25T19:34:43Z

Benchmarks [ tracer ]

Benchmark execution time: 2026-09-25 19:32:39

Comparing candidate commit dfd83d0 in PR branch glopes/ssi-shutdown-crash with baseline commit 07fd89d in branch master.

📊 Benchmarking dashboard

Found 1 performance improvements and 0 performance regressions! Performance is the same for 192 metrics, 1 unstable metrics.

Explanation

This is an A/B test comparing a candidate commit's performance against that of a baseline commit. Performance changes are noted in the tables below as:

🟩 = significantly better candidate vs. baseline
🟥 = significantly worse candidate vs. baseline

We compute a confidence interval (CI) over the relative difference of means between metrics from the candidate and baseline commits, considering the baseline as the reference.

If the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD), the change is considered significant.

Feel free to reach out to #apm-benchmarking-platform on Slack if you have any questions.

More details about the CI and significant changes

You can imagine this CI as a range of values that is likely to contain the true difference of means between the candidate and baseline commits.

CIs of the difference of means are often centered around 0%, because often changes are not that big:

---------------------------------(------|---^--------)-------------------------------->
                              -0.6%    0%  0.3%     +1.2%
                                 |          |        |
         lower bound of the CI --'          |        |
sample mean (center of the CI) -------------'        |
         upper bound of the CI ----------------------'

As described above, a change is considered significant if the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD).

For instance, for an execution time metric, this confidence interval indicates a significantly worse performance:

----------------------------------------|---------|---(---------^---------)---------->
                                       0%        1%  1.3%      2.2%      3.1%
                                                  |   |         |         |
       significant impact threshold --------------'   |         |         |
                      lower bound of CI --------------'         |         |
       sample mean (center of the CI) --------------------------'         |
                      upper bound of CI ----------------------------------'

scenario:MessagePackSerializationBench/benchMessagePackSerialization-opcache

🟩 execution_time [-4.459µs; -2.681µs] or [-3.857%; -2.319%]

Unstable benchmarks

These benchmarks have a confidence interval too wide to call a change; treat them as noise rather than signal.

scenario:LaravelBench/benchLaravelDdprof-opcache

unstable execution_time [-979.084µs; +493.284µs] or [-7.411%; +3.734%]

cataphract requested review from a team as code owners September 24, 2026 13:23

cataphract mentioned this pull request Sep 24, 2026

Misc CI fixes #4219

Open

2 tasks

chatgpt-codex-connector Bot reviewed Sep 24, 2026

View reviewed changes

Comment thread loader/dd_library_loader.c

Comment thread loader/telemetry_reaper_asm.S Outdated

Comment thread loader/telemetry_reaper.c Outdated

cataphract added 2 commits September 24, 2026 14:47

fix(loader): support legacy assemblers

5d1cbeb

if reaper thread is not launched, kill&reap child immediately

41f9523

Simplify the reaping

dfd83d0

Avoiding the asm trampoline

bwoebi approved these changes Sep 25, 2026

View reviewed changes

cataphract merged commit 5fe256c into master Sep 25, 2026
2197 of 2205 checks passed

cataphract deleted the glopes/ssi-shutdown-crash branch September 25, 2026 23:52

github-actions Bot added this to the 1.26.0 milestone Sep 25, 2026

Navigation Menu

fix(loader): avoid background dlclose at shutdown - #4229

fix(loader): avoid background dlclose at shutdown#4229
cataphract merged 4 commits into
masterfrom
glopes/ssi-shutdown-crash

cataphract commented Sep 24, 2026

Uh oh!

chatgpt-codex-connector Bot commented Sep 24, 2026 •

edited

Loading

Uh oh!

datadog-prod-us1-4 Bot commented Sep 24, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bwoebi commented Sep 24, 2026

Uh oh!

bwoebi commented Sep 25, 2026

Uh oh!

cataphract commented Sep 25, 2026

Uh oh!

bwoebi left a comment

Uh oh!

bwoebi commented Sep 25, 2026

Uh oh!

pr-commenter Bot commented Sep 25, 2026

Explanation

More details about the CI and significant changes

Unstable benchmarks

scenario:LaravelBench/benchLaravelDdprof-opcache

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

cataphract commented Sep 24, 2026

Description

Reviewer checklist

Uh oh!

chatgpt-codex-connector Bot commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

datadog-prod-us1-4 Bot commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ Errors

ℹ️ Info

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bwoebi commented Sep 24, 2026

Uh oh!

bwoebi commented Sep 25, 2026

Uh oh!

cataphract commented Sep 25, 2026

Uh oh!

bwoebi left a comment

Choose a reason for hiding this comment

Uh oh!

bwoebi commented Sep 25, 2026

Uh oh!

pr-commenter Bot commented Sep 25, 2026

Benchmarks [ tracer ]

Explanation

More details about the CI and significant changes

scenario:MessagePackSerializationBench/benchMessagePackSerialization-opcache

Unstable benchmarks

scenario:LaravelBench/benchLaravelDdprof-opcache

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chatgpt-codex-connector Bot commented Sep 24, 2026 •

edited

Loading

datadog-prod-us1-4 Bot commented Sep 24, 2026 •

edited

Loading