ci: split valgrind extension tests and stop retrying real failures

.claude/ci/tracer-unit-tests.md

@@ -8,15 +8,16 @@
 - `.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 |
 |--------|-------|-------------|
 | `compile extension: debug` | `dd-trace-ci:php-{ver}_bookworm-6` | Compiles ddtrace.so in debug mode; produces artifact consumed by all test jobs below |
 | `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

.gitlab/generate-tracer.php

@@ -237,7 +237,11 @@ function before_script_steps($with_docker_auth = false) {
           - PHP_MAJOR_MINOR: "<?= $major_minor ?>"
             ARCH: "<?= $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) {
 <?php
 foreach ($all_minor_major_targets as $major_minor):
 ?>
+<?php /*
+ * The extension .phpt suite runs as two jobs, not two passes of one job. The
+ * valgrind leak-check pass is ~an order of magnitude slower than the normal
+ * pass; running both serially in a single job routinely hit the 120m timeout
+ * and discarded the normal pass's results along with it.
+ *
+ * Both job names keep the `test_extension_ci:` prefix so they continue to match
+ * the `test_extension_ci:*` glob in flaky-jobs.txt -- merge-gate behaviour is
+ * unchanged by the split.
+ */ ?>
 "test_extension_ci: [<?= $major_minor ?>]":
   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
+<?php if (version_compare($major_minor, "7.4", ">=")): ?>
+    # 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
+<?php else: ?>
+    # 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 endif; ?>
+    PHP_MAJOR_MINOR: "<?= $major_minor ?>"
+    ARCH: "amd64"
+    KUBERNETES_POD_ANNOTATIONS_1: "ci.ddbuild.io/enforce-static-cpus=true"
+<?php if (version_compare($major_minor, "7.4", ">=")): ?>
+  # 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
+<?php else: ?>
+  # 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
+<?php endif; ?>
+  script:
+    - make test_extension_ci_normal
+<?php after_script("tmp/build_extension", has_test_agent: true); ?>
+
+"test_extension_ci: [<?= $major_minor ?>, valgrind]":
+  extends: .debug_test
+  services:
+<?php agent_httpbin_service() ?>
+  needs:
+    - job: "compile extension: debug"
+      parallel:
+        matrix:
+          - PHP_MAJOR_MINOR: "<?= $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: "<?= $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
 <?php after_script("tmp/build_extension", has_test_agent: true); ?>
 
 "Unit tests: [<?= $major_minor ?>]":
@@ -534,17 +597,10 @@ function before_script_steps($with_docker_auth = false) {
     DD_INSTRUMENTATION_TELEMETRY_ENABLED: 0
 <?php endif; ?>
   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"

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