ci: split valgrind extension tests and stop retrying real failures - #4097
Draft
bengl wants to merge 3 commits into
Draft
ci: split valgrind extension tests and stop retrying real failures#4097bengl wants to merge 3 commits into
bengl wants to merge 3 commits into
Conversation
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.
|
brettlangdon
reviewed
Aug 6, 2026
| ARCH: "<?= $arch ?>" | ||
| artifacts: true | ||
| retry: 2 | ||
| # No `retry:` override: inherit the repo-wide `default.retry` from |
Member
There was a problem hiding this comment.
this comment probably isn't needed long term / only really applies as context to the PR.
| - api_failure | ||
| - stuck_or_timeout_failure | ||
| - job_execution_timeout | ||
| # No `retry:` override: inherit the repo-wide `default.retry` from |
| foreach ($all_minor_major_targets as $major_minor): | ||
| ?> | ||
| <?php /* | ||
| * The extension .phpt suite runs as two jobs, not two passes of one job. The |
Member
There was a problem hiding this comment.
Same here, I think this comment can be simplified as saying to parallelize them separately since valgrind takes a long time.
we don't need all the context you have here
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.
brettlangdon
approved these changes
Aug 6, 2026
| foreach ($all_minor_major_targets as $major_minor): | ||
| ?> | ||
| <?php /* Normal and valgrind passes run as separate jobs: valgrind is far | ||
| slower, so pairing them in one job blew its timeout. */ ?> |
Member
There was a problem hiding this comment.
Suggested change
| slower, so pairing them in one job blew its timeout. */ ?> | |
| slower, so run in parallel. */ ?> |
Applies review suggestion. Comment only.
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.
Description
Three CI cost/latency fixes in the tracer pipeline, found while looking at
this repo's CI times in CI Visibility.
1.
test_extension_ciran the extension.phptsuite twice in one job.make test_extension_cidid a normal pass and then a valgrind leak-checkpass sequentially in a single job. The valgrind pass is roughly an order of
magnitude slower, and the pair routinely blew the job's 120m timeout —
throwing away the normal pass's results along with it. Over 14 days this job
class recorded 2440 failures against 3403 successes, with
[8.0](481errors / 301 successes) and
[7.4](321 / 260) worst affected; a large shareended in
Job failed: execution took longer than 2h0m0s.Now split into
test_extension_ci_normal/test_extension_ci_valgrindmaketargets, run as two CI jobs.
2.
MAX_TEST_PARALLELISMdidn't match the CPU request. The job requested12 CPUs but ran 4 test workers. On PHP >= 7.4 the normal job now uses 12.
3. Two jobs overrode
default.retryto re-addscript_failure.ASAN test_cused the bareretry: 2shorthand (retries every failurereason) and
PHP Language Testsspelled out awhen:list includingscript_failure. Both re-ran genuine test failures up to 3x. Neither job canblock a merge — both are in
flaky-jobs.txt— so the retries bought nosignal. Both now inherit the repo-wide infrastructure-only
default.retryfrom
generate-common.php.Timeouts are version-conditional on purpose
RUN_TESTS_IS_PARALLELgates-jon PHP >= 7.4, so 7.0–7.3 run the suiteserially and
MAX_TEST_PARALLELISMis dead config there. Measured durationsfor successful runs over 14 days (both passes combined):
So
>= 7.4normal jobs get45m+ parallelism 12;< 7.4keeps120mwithno parallelism override. Cutting the pre-7.4 budget without per-pass timing
would risk timeouts that
default.retrythen pays for three times over.Behaviour explicitly preserved
test_extension_ci:prefix, so they still match the
test_extension_ci:*glob inflaky-jobs.txt. Simulatedmerge-gate.sh's bash glob matching over allgenerated job names: 245 gating jobs before and after, and all 22
test_extension_cijobs non-gating.make test_extension_cistill runs both passes, via recursive$(MAKE)rather than prerequisites, somake -jNcan't run themconcurrently over the same
.phptsandbox.TEST_PHP_OUTPUT,DD_SPAWN_WORKER_STABLE_TRAMPOLINE,DD_TRACE_AGENT_TIMEOUT, thetests/ext/valgrindPATHshim and thetrailing
! grep -e '^LEAKED TEST SUMMARY'check verbatim.Known tradeoffs
duplicated
after_script. Net wall clock still improves.set -xepreviously skipped it. That's the cost of getting normal-pass results back
promptly on a broken branch.
script_failureisn'tretried away. That's intended; they were already non-gating.
>= 7.4timing sample is successful runs only, so the tail that hit theold 120m budget is censored. Margin looks large, but worth watching the
first week.
Verification
php -lclean on all.gitlab/generate-*.php; all five generators run.to master; only the tracer pipeline changed.
jobs changed (14
ASAN test_cretry-only, 11PHP Language Testsretry-only, 11
test_extension_ci). Zero real- script_failureentriesremain.
timeoutemits at job level in bothconditional branches.
budget is unmeasured — that's why it was left at 120m.
Reviewer checklist