From a3c77d92c4b4954156e83d4089369f87a3e6bc62 Mon Sep 17 00:00:00 2001 From: Bryan English Date: Thu, 6 Aug 2026 14:17:28 -0400 Subject: [PATCH 1/3] ci: split valgrind extension tests and stop retrying real failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `test_extension_ci` job ran the extension .phpt suite twice in one job: once normally, then again under valgrind for leak checking. The valgrind pass is roughly an order of magnitude slower, and the pair routinely exceeded the job's 120m timeout — discarding the normal pass's results along with it. Over the last two weeks this job class failed 2440 times against 3403 successes, with `[8.0]` and `[7.4]` worst affected. Split the two passes into `test_extension_ci_normal` and `test_extension_ci_valgrind` make targets, and run them as two jobs. Both job names keep the `test_extension_ci:` prefix so they still match the `test_extension_ci:*` glob in flaky-jobs.txt; merge-gate behaviour is unchanged. `make test_extension_ci` still runs both, via recursive `$(MAKE)` rather than prerequisites so that `make -jN` cannot run the two passes concurrently over the same .phpt sandbox. On PHP >= 7.4 the normal job now sets MAX_TEST_PARALLELISM to 12 to match its CPU request, and its timeout drops to 45m. Both passes together have a pc95 of ~30m and a worst case of ~39m on those versions, so the normal pass alone has headroom. Below 7.4 the timeout stays at 120m and no parallelism override is set: the Makefile only passes -j to run-tests.php when RUN_TESTS_IS_PARALLEL is set, which is gated on PHP >= 7.4, so those versions run serially. The inherited value of 8 from .base_test is unused there. Both passes together currently take a pc95 of 82-89m and a worst case of 91-99m on those versions; cutting that budget without per-pass timing would risk timeouts that `default.retry` then pays for three times over. Note the split means the valgrind pass now runs even when the normal pass fails, where previously `set -xe` skipped it. That is the cost of getting the normal pass's results back promptly on a broken branch. Separately, drop two `retry:` overrides that re-added `script_failure` on top of the repo-wide infrastructure-only `default.retry`: - `ASAN test_c` used the bare `retry: 2` shorthand, which retries every failure reason. Inheriting the default also restores the `exit_codes: [75, 128]` retry that the shorthand had replaced. - `PHP Language Tests` spelled out a `when:` list including `script_failure`. Both re-ran genuine test failures up to three times, tripling the compute and wall clock of an already-failing pipeline. Neither job can block a merge — both are listed in flaky-jobs.txt — so the retries bought no signal. Expect ASAN failures to become more visible as a result. --- .claude/ci/tracer-unit-tests.md | 29 ++++++++---- .gitlab/generate-tracer.php | 82 +++++++++++++++++++++++++++------ Makefile | 28 +++++++++-- 3 files changed, 113 insertions(+), 26 deletions(-) diff --git a/.claude/ci/tracer-unit-tests.md b/.claude/ci/tracer-unit-tests.md index 97a1a5974ff..75ee86e125d 100644 --- a/.claude/ci/tracer-unit-tests.md +++ b/.claude/ci/tracer-unit-tests.md @@ -8,7 +8,7 @@ - `.gitlab/compile_extension.sh` — compiles ddtrace.so (used by the `compile extension: debug` prerequisite) - `Makefile` — defines the `test_c`, `test_unit`, `test_opcache`, - `test_extension_ci`, etc. targets + `test_extension_ci_normal`, `test_extension_ci_valgrind`, etc. targets | CI Job | Image | What it does | |--------|-------|-------------| @@ -16,7 +16,8 @@ | `compile extension: debug-zts-asan` | same | Compiles ddtrace.so with ASAN+ZTS; used by ASAN test jobs | | `Unit tests: [{ver}]` | `dd-trace-ci:php-{ver}_bookworm-6` | Runs PHPUnit `--testsuite=unit` | | `API unit tests: [{ver}]` | same | Runs PHPUnit API unit tests | -| `test_extension_ci: [{ver}]` | same | Runs .phpt extension tests + valgrind wrapper, with test-agent | +| `test_extension_ci: [{ver}]` | same | Runs .phpt extension tests (normal pass), with test-agent | +| `test_extension_ci: [{ver}, valgrind]` | same | Same suite under valgrind for leak checking; much slower, so it is a separate job | | `PHP Language Tests: [{ver}]` | same | Runs the upstream PHP test suite with ddtrace loaded; uses an xfail list | | `Opcache tests: [{ver}]` | same | Runs .phpt tests in `tests/opcache/` with opcache.so loaded | | `xDebug tests: [{ver}, {xdebug_ver}]` | same | Runs xdebug-specific .phpt tests + unit tests with xdebug loaded | @@ -549,14 +550,22 @@ make test_opcache Each new `dockerh` invocation must re-run `make install_all` even when the compiled artifacts are cached. -- **`test_extension_ci` uses a valgrind wrapper.** The Makefile - prepends `tests/ext/valgrind` to `$PATH`, which intercepts `php` - calls to run them under valgrind. This makes the job significantly - slower and is specific to CI. - -- **`PHP Language Tests` has retry:2 in CI.** These tests are - inherently flaky due to timing-sensitive PHP runtime tests. The CI - job retries up to 2 times on script failure. +- **The valgrind pass is a separate job.** `make test_extension_ci_normal` + runs the suite normally; `make test_extension_ci_valgrind` re-runs it + under valgrind (`run-tests.php -m`) for leak checking. Both prepend + `tests/ext/valgrind` to `$PATH` — the shim there intercepts `valgrind` + invocations to add the suppressions file. The valgrind pass is roughly + an order of magnitude slower, which is why it no longer shares a job + with the normal pass. `make test_extension_ci` still runs both + serially for local use. + +- **Retries are infrastructure-only.** `test_extension_ci`, + `ASAN test_c`, and `PHP Language Tests` inherit `default.retry` from + `.gitlab/generate-common.php`, which retries runner/API/timeout + failures but *not* `script_failure`. A genuine test failure is not + retried — retrying it tripled compute without changing the outcome. + These job classes are listed in `flaky-jobs.txt`, so their failures + are already non-gating. - **`test_integration` talks to test-agent on port 9126** and mongodb. `test_composer`, `test_auto_instrumentation`, and diff --git a/.gitlab/generate-tracer.php b/.gitlab/generate-tracer.php index 05aea7c9b62..0abb0d4d20d 100644 --- a/.gitlab/generate-tracer.php +++ b/.gitlab/generate-tracer.php @@ -237,7 +237,11 @@ function before_script_steps($with_docker_auth = false) { - PHP_MAJOR_MINOR: "" ARCH: "" artifacts: true - retry: 2 + # No `retry:` override: inherit the repo-wide `default.retry` from + # generate-common.php. The bare `retry: 2` shorthand retried *every* failure + # reason, including `script_failure`, so a genuine ASAN regression burned 3x + # the compute. These failures are already non-gating (`ASAN test_c:*` is in + # flaky-jobs.txt), so the retries bought no merge signal. variables: WAIT_FOR: test-agent:9126 KUBERNETES_CPU_REQUEST: 6 @@ -355,6 +359,16 @@ function before_script_steps($with_docker_auth = false) { + "test_extension_ci: []": extends: .debug_test services: @@ -369,13 +383,62 @@ function before_script_steps($with_docker_auth = false) { variables: WAIT_FOR: test-agent:9126 KUBERNETES_CPU_REQUEST: 12 +=")): ?> + # Match parallelism to the reserved CPU count. The previous value of 4 left + # 8 of the 12 requested cores idle for the whole job; the low value was only + # ever needed by the valgrind pass, which now runs as its own job below. + MAX_TEST_PARALLELISM: 12 + + # No MAX_TEST_PARALLELISM override below 7.4: run-tests.php is only invoked + # with -j when RUN_TESTS_IS_PARALLEL is set, and the Makefile gates that on + # PHP >= 7.4 (see RUN_TESTS_IS_PARALLEL in the Makefile). These versions run + # the suite serially, so any value here would be dead config. + + PHP_MAJOR_MINOR: "" + ARCH: "amd64" + KUBERNETES_POD_ANNOTATIONS_1: "ci.ddbuild.io/enforce-static-cpus=true" +=")): ?> + # Both passes together have a pc95 of ~30m and a worst case of ~39m on these + # versions, so the normal pass alone has ample headroom here. + timeout: 45m + + # Left at the pre-split budget deliberately. These versions run serially and + # both passes together already take 82-99m, so the normal pass alone has not + # been measured in isolation. Tighten once there is per-pass timing; cutting + # this blind risks timeouts that `default.retry` would then pay for 3x via + # job_execution_timeout. + timeout: 120m + + script: + - make test_extension_ci_normal + + +"test_extension_ci: [, valgrind]": + extends: .debug_test + services: + + needs: + - job: "compile extension: debug" + parallel: + matrix: + - PHP_MAJOR_MINOR: "" + ARCH: "amd64" + artifacts: true + variables: + WAIT_FOR: test-agent:9126 + KUBERNETES_CPU_REQUEST: 12 + # Deliberately below the reserved CPU count: each worker spawns a valgrind + # process with its own memory and CPU overhead, so the pre-split pairing of + # 12 CPUs to 4 workers is preserved here rather than guessed at. Worth + # measuring separately. (Below 7.4 this is dead config -- see the note on + # the normal job above.) MAX_TEST_PARALLELISM: 4 PHP_MAJOR_MINOR: "" ARCH: "amd64" KUBERNETES_POD_ANNOTATIONS_1: "ci.ddbuild.io/enforce-static-cpus=true" timeout: 120m script: - - make test_extension_ci + - make test_extension_ci_valgrind "Unit tests: []": @@ -534,17 +597,10 @@ function before_script_steps($with_docker_auth = false) { DD_INSTRUMENTATION_TELEMETRY_ENABLED: 0 timeout: 40m - retry: - max: 2 - when: - - script_failure - - unknown_failure - - data_integrity_failure - - runner_system_failure - - scheduler_failure - - api_failure - - stuck_or_timeout_failure - - job_execution_timeout + # No `retry:` override: inherit the repo-wide `default.retry` from + # generate-common.php, which retries infrastructure failures only. Retrying + # `script_failure` re-ran genuine test failures up to 3x, tripling both the + # compute and the wall clock of an already-failing pipeline. script: - make install_all - export XFAIL_LIST="dockerfiles/ci/xfail_tests/${PHP_MAJOR_MINOR}.list" diff --git a/Makefile b/Makefile index aa1ba9dad6f..5a54dbe3b48 100644 --- a/Makefile +++ b/Makefile @@ -202,18 +202,40 @@ test_c2php: $(SO_FILE) $(INIT_HOOK_TEST_FILES) $(BUILD_DIR)/run-tests.php test_with_init_hook: $(SO_FILE) $(INIT_HOOK_TEST_FILES) $(BUILD_DIR)/run-tests.php $(if $(ASAN), USE_ZEND_ALLOC=0 USE_TRACKED_ALLOC=1) $(RUN_TESTS_CMD) -d extension=$(SO_FILE) $(TRACER_SOURCES_INI) $(INIT_HOOK_TEST_FILES); -test_extension_ci: $(SO_FILE) $(TEST_FILES) $(TEST_STUB_FILES) $(BUILD_DIR)/run-tests.php +# The extension .phpt suite is run twice: once normally, and once under valgrind +# for leak checking. The valgrind pass is an order of magnitude slower, so the +# two passes are separate targets and separate CI jobs -- running them serially +# in one job pushed `test_extension_ci` into its 2h timeout. +# +# `tests/ext/valgrind` is prepended to $PATH in both passes: the shim there +# intercepts `valgrind` invocations to add the suppressions file. Only the `-m` +# pass invokes valgrind, but the export is kept in both to preserve the +# pre-split environment exactly. +test_extension_ci_normal: $(SO_FILE) $(TEST_FILES) $(TEST_STUB_FILES) $(BUILD_DIR)/run-tests.php ( \ set -xe; \ export PATH="$(PROJECT_ROOT)/tests/ext/valgrind:$$PATH"; \ export TEST_PHP_JUNIT=$(JUNIT_RESULTS_DIR)/normal-extension-test.xml; \ $(ALL_TEST_ENV_OVERRIDE) $(RUN_TESTS_CMD) -d extension=$(SO_FILE) $(BUILD_DIR)/$(TESTS); \ - \ + ) + +test_extension_ci_valgrind: $(SO_FILE) $(TEST_FILES) $(TEST_STUB_FILES) $(BUILD_DIR)/run-tests.php + ( \ + set -xe; \ + export PATH="$(PROJECT_ROOT)/tests/ext/valgrind:$$PATH"; \ export TEST_PHP_JUNIT=$(JUNIT_RESULTS_DIR)/valgrind-extension-test.xml; \ export TEST_PHP_OUTPUT=$(JUNIT_RESULTS_DIR)/valgrind-run-tests.out; \ DD_SPAWN_WORKER_STABLE_TRAMPOLINE=1 $(ALL_TEST_ENV_OVERRIDE) DD_TRACE_AGENT_TIMEOUT=5000 $(RUN_TESTS_CMD) -d extension=$(SO_FILE) -m -s $$TEST_PHP_OUTPUT $(BUILD_DIR)/$(TESTS) && ! grep -e '^LEAKED TEST SUMMARY' $$TEST_PHP_OUTPUT; \ ) +# Back-compat composite for local use: both passes, serially, as before. +# Recursive $(MAKE) rather than prerequisites, because prerequisites would run +# concurrently under `make -jN` and both passes share the same .phpt sandbox +# (.out/.diff/.mem files), which would interleave and corrupt results. +test_extension_ci: + $(MAKE) test_extension_ci_normal + $(MAKE) test_extension_ci_valgrind + build_tea: TEA_BUILD_TESTS=ON build_tea: TEA_PREFIX_PATH=/opt/catch2 build_tea: build_tea_common @@ -1637,5 +1659,5 @@ test_internal_api_randomized: $(SO_FILE) composer.lock: composer.json $(call run_composer_with_retry,,) -.PHONY: dev dist_clean clean cores all clang_format_check clang_format_fix install sudo_install test_c test_c_mem test_extension_ci test_zai test_zai_asan test install_ini install_all \ +.PHONY: dev dist_clean clean cores all clang_format_check clang_format_fix install sudo_install test_c test_c_mem test_extension_ci test_extension_ci_normal test_extension_ci_valgrind test_zai test_zai_asan test install_ini install_all \ .apk .rpm .deb .tar.gz sudo debug prod strict run-tests.php verify_pecl_file_definitions verify_package_xml cbindgen cbindgen_binary From 13156a4c6269160d4b396f54cc61ae0fb59c5a1f Mon Sep 17 00:00:00 2001 From: Bryan English Date: Thu, 6 Aug 2026 14:54:47 -0400 Subject: [PATCH 2/3] ci: trim generator comments to the load-bearing bits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Most of the comments added in the previous commit explained the change rather than the code, which is PR context and goes stale. Drop the two `retry:` notes entirely — the absence of an override is not itself noteworthy — and cut the rest to a line or two. Two are kept because they document non-obvious constraints a future change could silently break: that run-tests.php only gets -j on PHP >= 7.4, which is why the pre-7.4 timeout is higher, and that the composite `test_extension_ci` target uses recursive $(MAKE) so `make -jN` cannot run both passes over the same .phpt sandbox. Comments only; the generated YAML is unchanged apart from the comment lines it emits. --- .gitlab/generate-tracer.php | 45 +++++-------------------------------- Makefile | 18 +++++---------- 2 files changed, 11 insertions(+), 52 deletions(-) diff --git a/.gitlab/generate-tracer.php b/.gitlab/generate-tracer.php index 0abb0d4d20d..b6768d4cc53 100644 --- a/.gitlab/generate-tracer.php +++ b/.gitlab/generate-tracer.php @@ -237,11 +237,6 @@ function before_script_steps($with_docker_auth = false) { - PHP_MAJOR_MINOR: "" ARCH: "" artifacts: true - # No `retry:` override: inherit the repo-wide `default.retry` from - # generate-common.php. The bare `retry: 2` shorthand retried *every* failure - # reason, including `script_failure`, so a genuine ASAN regression burned 3x - # the compute. These failures are already non-gating (`ASAN test_c:*` is in - # flaky-jobs.txt), so the retries bought no merge signal. variables: WAIT_FOR: test-agent:9126 KUBERNETES_CPU_REQUEST: 6 @@ -359,16 +354,8 @@ function before_script_steps($with_docker_auth = false) { - + "test_extension_ci: []": extends: .debug_test services: @@ -384,29 +371,17 @@ function before_script_steps($with_docker_auth = false) { WAIT_FOR: test-agent:9126 KUBERNETES_CPU_REQUEST: 12 =")): ?> - # Match parallelism to the reserved CPU count. The previous value of 4 left - # 8 of the 12 requested cores idle for the whole job; the low value was only - # ever needed by the valgrind pass, which now runs as its own job below. + # Match the CPU request. MAX_TEST_PARALLELISM: 12 - - # No MAX_TEST_PARALLELISM override below 7.4: run-tests.php is only invoked - # with -j when RUN_TESTS_IS_PARALLEL is set, and the Makefile gates that on - # PHP >= 7.4 (see RUN_TESTS_IS_PARALLEL in the Makefile). These versions run - # the suite serially, so any value here would be dead config. PHP_MAJOR_MINOR: "" ARCH: "amd64" KUBERNETES_POD_ANNOTATIONS_1: "ci.ddbuild.io/enforce-static-cpus=true" =")): ?> - # Both passes together have a pc95 of ~30m and a worst case of ~39m on these - # versions, so the normal pass alone has ample headroom here. timeout: 45m - # Left at the pre-split budget deliberately. These versions run serially and - # both passes together already take 82-99m, so the normal pass alone has not - # been measured in isolation. Tighten once there is per-pass timing; cutting - # this blind risks timeouts that `default.retry` would then pay for 3x via - # job_execution_timeout. + # run-tests.php only gets -j on PHP >= 7.4 (RUN_TESTS_IS_PARALLEL in the + # Makefile), so these versions run serially and need the larger budget. timeout: 120m script: @@ -427,11 +402,7 @@ function before_script_steps($with_docker_auth = false) { variables: WAIT_FOR: test-agent:9126 KUBERNETES_CPU_REQUEST: 12 - # Deliberately below the reserved CPU count: each worker spawns a valgrind - # process with its own memory and CPU overhead, so the pre-split pairing of - # 12 CPUs to 4 workers is preserved here rather than guessed at. Worth - # measuring separately. (Below 7.4 this is dead config -- see the note on - # the normal job above.) + # Below the CPU request: each worker spawns its own valgrind process. MAX_TEST_PARALLELISM: 4 PHP_MAJOR_MINOR: "" ARCH: "amd64" @@ -597,10 +568,6 @@ function before_script_steps($with_docker_auth = false) { DD_INSTRUMENTATION_TELEMETRY_ENABLED: 0 timeout: 40m - # No `retry:` override: inherit the repo-wide `default.retry` from - # generate-common.php, which retries infrastructure failures only. Retrying - # `script_failure` re-ran genuine test failures up to 3x, tripling both the - # compute and the wall clock of an already-failing pipeline. script: - make install_all - export XFAIL_LIST="dockerfiles/ci/xfail_tests/${PHP_MAJOR_MINOR}.list" diff --git a/Makefile b/Makefile index 5a54dbe3b48..c26c6acba62 100644 --- a/Makefile +++ b/Makefile @@ -202,15 +202,9 @@ test_c2php: $(SO_FILE) $(INIT_HOOK_TEST_FILES) $(BUILD_DIR)/run-tests.php test_with_init_hook: $(SO_FILE) $(INIT_HOOK_TEST_FILES) $(BUILD_DIR)/run-tests.php $(if $(ASAN), USE_ZEND_ALLOC=0 USE_TRACKED_ALLOC=1) $(RUN_TESTS_CMD) -d extension=$(SO_FILE) $(TRACER_SOURCES_INI) $(INIT_HOOK_TEST_FILES); -# The extension .phpt suite is run twice: once normally, and once under valgrind -# for leak checking. The valgrind pass is an order of magnitude slower, so the -# two passes are separate targets and separate CI jobs -- running them serially -# in one job pushed `test_extension_ci` into its 2h timeout. -# -# `tests/ext/valgrind` is prepended to $PATH in both passes: the shim there -# intercepts `valgrind` invocations to add the suppressions file. Only the `-m` -# pass invokes valgrind, but the export is kept in both to preserve the -# pre-split environment exactly. +# The .phpt suite runs twice: normally, and under valgrind for leak checking. +# Separate targets so CI can parallelize them -- valgrind is far slower. +# The PATH shim in tests/ext/valgrind adds the suppressions file. test_extension_ci_normal: $(SO_FILE) $(TEST_FILES) $(TEST_STUB_FILES) $(BUILD_DIR)/run-tests.php ( \ set -xe; \ @@ -228,10 +222,8 @@ test_extension_ci_valgrind: $(SO_FILE) $(TEST_FILES) $(TEST_STUB_FILES) $(BUILD_ DD_SPAWN_WORKER_STABLE_TRAMPOLINE=1 $(ALL_TEST_ENV_OVERRIDE) DD_TRACE_AGENT_TIMEOUT=5000 $(RUN_TESTS_CMD) -d extension=$(SO_FILE) -m -s $$TEST_PHP_OUTPUT $(BUILD_DIR)/$(TESTS) && ! grep -e '^LEAKED TEST SUMMARY' $$TEST_PHP_OUTPUT; \ ) -# Back-compat composite for local use: both passes, serially, as before. -# Recursive $(MAKE) rather than prerequisites, because prerequisites would run -# concurrently under `make -jN` and both passes share the same .phpt sandbox -# (.out/.diff/.mem files), which would interleave and corrupt results. +# Recursive $(MAKE), not prerequisites: under `make -jN` prerequisites would run +# concurrently and both passes share the same .phpt sandbox (.out/.diff/.mem). test_extension_ci: $(MAKE) test_extension_ci_normal $(MAKE) test_extension_ci_valgrind From 24e25e8e234584819303028d660a8ccbc090d34b Mon Sep 17 00:00:00 2001 From: Bryan English Date: Fri, 7 Aug 2026 09:08:50 -0400 Subject: [PATCH 3/3] ci: shorten the valgrind split comment Applies review suggestion. Comment only. --- .gitlab/generate-tracer.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitlab/generate-tracer.php b/.gitlab/generate-tracer.php index b6768d4cc53..2d463293c7a 100644 --- a/.gitlab/generate-tracer.php +++ b/.gitlab/generate-tracer.php @@ -355,7 +355,7 @@ function before_script_steps($with_docker_auth = false) { foreach ($all_minor_major_targets as $major_minor): ?> + slower, so run in parallel. */ ?> "test_extension_ci: []": extends: .debug_test services: