Stabilize running the tracer on macos - #4187
Open
bwoebi wants to merge 25 commits into
Open
Conversation
Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
macOS invalidates the code signature when the .so is modified, causing SIGKILL (exit 137) when PHP processes try to load it. Re-sign with an ad-hoc signature after each build. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
The libdatadog submodule bump crosses de67b226e (renamed datadog-live-debugger -> libdd-live-debugger for publishing), so the old crate name/path no longer resolves. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
macos:sonoma-arm64 is now a project-runner tag reserved for a fixed allowlist of repos (datadog-ios, datadog-agent, etc); dd-trace-php isn't on it, so the job had no matching runner. Switch to the default arm64 macOS pool: tag macos:tart with an image pointing at a Tart VM OCI image in ECR.
Now that the sidecar stays up for the whole test_c run (previously it kept crash-looping, incidentally resetting the shared-memory rate limiter each time), the default 100 traces/sec limit becomes reachable for real: 717 tests across 4 concurrent workers can legitimately spike past it within the run, causing the local rate limiter to kick in and tag spans with a computed _dd.p.ksr rate instead of the deterministic _dd.p.dm=-0 that most tests' EXPECTF fixtures assume. Other non-deterministic sampling sources are already neutralized for tests (DD_TRACE_IGNORE_AGENT_SAMPLING_RATES, DD_TRACE_GIT_METADATA_ENABLED), but this one wasn't, since it never mattered before. Pin it to an effectively-unlimited value alongside the other overrides, and update config.phpt's expected telemetry dump for the new env-origin entry this introduces. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… test determinism DD_TRACE_IGNORE_AGENT_SAMPLING_RATES=1 was only added to ALL_TEST_ENV_OVERRIDE when DD_TRACE_DOCKER_DEBUG was set -- which is true inside the Linux dev/CI Docker images but never on bare-metal runners (macOS Tart VMs, Windows). With the sidecar now staying up for the whole test_c run instead of crash-looping, its remote-config channel can actually deliver an agent-provided rate-by-service on these platforms, tagging spans with a computed _dd.p.ksr instead of the deterministic _dd.p.dm=-0 most .phpt EXPECTF fixtures assume. Make the flag unconditional so every platform gets the same deterministic sampling behavior, and set it (plus the same DD_TRACE_RATE_LIMIT override used elsewhere) explicitly for the Windows test_c job, which invokes run-tests.php directly rather than through make test_c. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… dump Temporary, to be reverted once the source of the agent-provided default_env on the macOS CI runner is identified.
…OS runner The macOS Tart VM runner (bare-metal, unlike containerized Linux jobs) has DD_AGENT_HOST=192.168.64.1 and DD_TRACE_AGENT_PORT=8126 injected into its environment, pointing at a real agent on the host (presumably for the runner's own infra monitoring). Confirmed via debug instrumentation: the sidecar's agent-info fetch was reaching that real agent and caching a genuine (if generic) "default_env": "none" in its shared session state, which then got applied to DD_ENV at runtime for every subsequent test process sharing that daemon -- since these env vars take precedence over the test harness's -d datadog.agent_host=... -d datadog.trace.agent_port=1 INI overrides, which are meant to guarantee no agent is reachable during tests. Unset them alongside the other DD_* vars GitLab already auto-injects for infra purposes (see unset_dd_runner_env_vars in generate-common.php), which this job's custom before_script wasn't calling. Reverts the temporary debug instrumentation and CI env dump added to diagnose this. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Unset DD_DOGSTATSD_PORT alongside the other GitLab-injected DD_* env vars for the macOS test_c job, closing the last leak that let a real agent config reach the sidecar and break test determinism. - Downgrade the "INI not found" crashtracker setup log from DEBUG to TRACE: it fires on every setup when opcache/JIT aren't compiled in (as on the macOS CI build) and was polluting DD_TRACE_DEBUG=1 test diffs. - Skip force_flush_traces.phpt when the pcntl extension (and thus SIGKILL) isn't available. - Bump libdatadog to 025012a39, picking up the fix for a macOS SHM mapping panic when the size slot hasn't been committed yet. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
bwoebi
requested review from
btthomas and
typotter
and removed request for
a team
September 11, 2026 20:55
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. |
|
… break Two prior macOS test-determinism fixes had been applied unconditionally in the shared Makefile, breaking Linux CI: - ALL_TEST_ENV_OVERRIDE forced DD_TRACE_IGNORE_AGENT_SAMPLING_RATES=1 on every platform, breaking tests/ext/background-sender/agent_sampling_sidecar.phpt, which specifically verifies behavior when agent sampling rates are NOT ignored. - RUN_TESTS_CMD unconditionally appended "-d datadog.agent_host=127.0.0.1 -d datadog.trace.agent_port=1", clobbering tests (e.g. tests/ext/live-debugger/debugger_log_probe_capture_size_limit.phpt) that explicitly point at the request-replayer mock agent via --ENV--. The env leakage these overrides guard against is specific to macOS's bare-metal CI runners, which GitLab auto-injects DD_* vars into for infra monitoring; Linux CI runs in Docker with no such stray agent and never needed either override. Scoped both to `uname -s` == Darwin, matching the existing pattern used for the macOS codesign step just below. Also bumps libdatadog to pick up a clippy::never_loop fix in datadog-sidecar/src/entry.rs that was breaking clippy/helper-rust CI jobs across every PHP version on Linux. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
bwoebi
force-pushed
the
bob/macos-runner
branch
from
September 11, 2026 21:37
ee31a6e to
8679cbd
Compare
Benchmarks [ tracer ]Benchmark execution time: 2026-09-12 16:49:01 Comparing candidate commit dd1480e in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 192 metrics, 2 unstable metrics.
|
Comment on lines
-10
to
-11
| echo dd_trace_env_config("DD_TRACE_AGENT_PORT"); | ||
| echo PHP_EOL; |
Collaborator
There was a problem hiding this comment.
Why would we need to remove this?
Collaborator
Author
There was a problem hiding this comment.
Because the CI env is messing with it :-(
morrisonlevi
approved these changes
Sep 11, 2026
- Skip segfault_backtrace_disabled.phpt on macOS with an accurate reason: with the default config, SIGSEGV handling is delegated entirely to libdatadog's crashtracker (ext/signals.c's own handler only installs when crashtracking is disabled), and on macOS the crashtracker's fork+receiver-process handling doesn't leave the process to die by/report the signal the way run-tests.php's Termsig detection expects. - Revert the broken_pipe.phpt Darwin skip added in a prior commit -- per Codex review, it was hiding a real macOS IPC issue (a timeout reconnecting after a forced broken connection) rather than a mere wording difference. Investigating for real instead of skipping. - Add --enable-pcntl to the macOS PHP build so pcntl-dependent tests (force_flush_traces.phpt, etc.) actually run on macOS CI instead of being silently skipped. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Picks up fix/macos-recv-liveness-detection, which addresses the real bug Codex found in tests/ext/telemetry/broken_pipe.phpt on macOS: recv_raw_blocking never watched the liveness pipe, so a daemon disconnecting mid-wait (e.g. after send_garbage causes the IPC serve loop to fail decoding and close its connection) went undetected until the whole read_timeout elapsed, then surfaced as a non-reconnectable TimedOut instead of BrokenPipe. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Enabling --enable-pcntl on the macOS CI job (to fix issue #3 of the Codex review) exposed this test, which was previously skipped for lack of pcntl. It fails there because its re-exec trick reconstructs the process's full original argv (interpreter + its own -n/-d flags + script args) via Linux's /proc/<pid>/cmdline; macOS has no portable equivalent without ext-ffi + sysctl(KERN_PROCARGS2). This is a test-infrastructure limitation, not a ddtrace behavior gap -- this test already runs (and presumably passes) on Linux CI today, unaffected by this change. $argv alone (tried first) is insufficient since it doesn't carry the interpreter's own flags needed to keep ddtrace loaded after the re-exec. Verified locally on macOS: test now skips cleanly instead of failing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
bump libdatadog for the recv_raw_async liveness fix (root cause of the pcntl_fork_thread_mode_orphan.phpt hang: the server-side dispatch loop had no way to detect a disconnected macOS peer, so it waited indefinitely for the shutdown watchdog to see all connections close). Skip pcntl_fork_thread_mode_orphan.phpt on Darwin: with the hang fixed, the test now surfaces a separate, genuine macOS OS-level constraint -- kqueue fds don't survive fork(), so tokio's I/O driver panics with "Bad file descriptor" in the forked child. Thread mode is documented as not pcntl_fork()-safe; this only affects test coverage for the orphan-promotion scenario, not real usage (apps that fork already need subprocess mode). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The macOS Tart runner has no Docker service network like the Linux/ Windows jobs' "request-replayer" service container, so tests gated by tests/ext/includes/skipif_no_dev_env.inc (checking DATADOG_HAVE_DEV_ENV) were unconditionally skipped there. Run request-replayer as a native background process on loopback instead: alias its hostname via /etc/hosts (most call sites hardcode "request-replayer" rather than reading DD_AGENT_HOST, e.g. tests/Common/TracerTestTrait.php), install its composer dependency with brew's php (bundles curl + gmp -- both required, see dockerfiles/services/request-replayer/linux.Dockerfile and index.php's UnpackOptions::BIGINT_AS_GMP), and start it on port 80. Validated locally end-to-end (native php -S bound to a loopback alias, /etc/hosts entry, DATADOG_HAVE_DEV_ENV=1): a previously-unrunnable dev-env-gated test (debugger_log_probe_capture_size_limit.phpt) passes. The CI wiring itself (sudo for the privileged port + /etc/hosts edit, brew's php formula) is unverified in the actual Tart runner environment and may need iteration. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The CI job hung indefinitely after the test suite finished (confirmed via job trace: it stopped dead right after "make: *** [test_c] Error 1", with zero after_script output, for 2+ hours). Root cause: `sudo bash -c "... &"` backgrounds the php -S process inside the bash -c subshell, but non-interactive shells don't put background jobs in their own process group, so it stayed in sudo's process group. sudo with pty allocation (common on macOS) waits for the whole process group to exit before returning -- so the long-lived request-replayer server kept the whole job's script step from ever completing, even though its own stdout/stderr were redirected to a log file. Use `sudo -b`, sudo's own flag for backgrounding a command, so sudo itself returns immediately without waiting on the child. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Crashtracking isn't supported on macOS: the crash report never reaches the agent/request-replayer the way it does on Linux. Skip both crashtracker_segfault.phpt and crashtracker_segfault_disabled.phpt there (the latter also independently picks up macOS /bin/sh's own "Segmentation fault: 11" job-control notice on stderr, which isn't suppressed by the test's own "2>/dev/null"). rc_trace_enabled_rinit.phpt hits the same /proc/<pid>/cmdline re-exec constraint already fixed for extract_server_values.phpt -- apply the same Darwin skip. These three tests were previously silently skipped by skipif_no_dev_env.inc (no DATADOG_HAVE_DEV_ENV in CI) and only surfaced now that the macOS job runs request-replayer natively. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
sudo -b alone wasn't enough: the job hung again (confirmed via job trace -- test suite finished cleanly with 0 failures, but no after_script output followed, and the job sat idle for minutes). Whatever residual handle keeps the runner's output pipe open isn't fully understood, so rather than chase the exact detachment mechanism further, capture the server's PID when starting it and kill it explicitly in after_script, which runs regardless of script outcome. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Found the actual mechanism via the job trace: GitLab's own timeout fired on "step_script" (before_script + script combined) after 1h, with after_script never starting at all -- the test suite had finished cleanly (0 failures) 50 minutes earlier. So the hang is in step_script itself never returning, meaning the after_script-only kill added in fcaf2fe never got a chance to run. Most likely explanation: request-replayer runs as root (via sudo), and gitlab-runner's own process-tree cleanup, running as the unprivileged CI user, cannot signal it -- so whatever wait it performs before concluding step_script blocks forever on a process it can't kill, until GitLab's outer 1h job timeout intervenes. Kill it explicitly (with our own passwordless sudo) as the last line of script: itself, so nothing survives for the runner's own cleanup to wait on. Kept the after_script kill too as a backstop for a script: that fails before reaching that line. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
These were missing configure flags, not real platform gaps -- verified
locally that --with-curl/--enable-sockets/--with-ffi build cleanly and
all three extensions load and function (curl_init, socket_create, FFI
class all present). This unlocks the "curl extension required" (26),
"sockets extension is required" (5), and "ffi extension required" (2)
skips from the macOS test_c run.
Enabling ffi surfaces a fourth crashtracker test
(crashtracker_collect_all_threads.phpt, gated on both dev-env and ffi)
that was never exercised before; skip it on Darwin for the same reason
as the other three crashtracker tests.
Note: opcache.so is not produced by this build (libtool reports
"whether to build shared libraries... no" even with opcache otherwise
enabled and functional -- extension_loaded("Zend OPcache") is true),
so the 5 tests that check for the physical .so file's existence still
skip. Left uninvestigated for now.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
debugger_log_probe_capture_timeout.phpt (a dev-env-gated test, never
exercised on macOS until this session's CI work turned DATADOG_HAVE_DEV_ENV
on) crashed reproducibly on macOS with EXC_BAD_ACCESS in
ZEND_DO_UCALL_SPEC_OBSERVER_TAILCALL_HANDLER: execute_data->call was NULL
where the VM expected a valid pending call frame.
Traced it to a genuine PHP core bug, not a ddtrace bug: on
ZEND_VM_KIND_TAILCALL builds (used on macOS/ARM64, unlike Linux's
HYBRID VM), a VM interrupt handled right as a tailcall returns to its
caller didn't save the opline first, corrupting the caller's in-flight
call setup for its next statement. Confirmed via lldb: the crash
follows immediately after a dd_vm_interrupt-triggered
ddog_process_remote_configs() call lands on the top-level script frame
right between two statements.
Fixed upstream in PHP 8.5.8 ("Fix another vm_interrupt bug for
tailcall VM", GH-22265, commit 05c7b00d796). Verified locally:
reproduced the crash 3/3 times on 8.5.7, and it's gone 6/6 times after
rebuilding against 8.5.9.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
2 participants
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.
dd-trace-php has always more or less been running on a mac, but not necessary very stably.
This runs the test_c testsuite on a mac, ensuring that general tracer functionality definitely runs on the mac.