ci: keep request replayer responsive under parallel tests - #4106
Merged
Conversation
bm1549
added
the
AI Generated
|
bwoebi
reviewed
Aug 14, 2026
bwoebi
reviewed
Aug 14, 2026
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.
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.
Applies review suggestion. Comment only.
Leiyks
force-pushed
the
brian.marks/fix-request-replayer-concurrency
branch
from
August 25, 2026 12:57
f9bd8de to
15fb2c2
Compare
Signed-off-by: Alexandre Rulleau <alexandre.rulleau@datadoghq.com>
The request-replayer service image was built by a manual `docker buildx --push` from a developer laptop to Docker Hub, then consumed from the digest-pinned `images/mirror/` namespace -- so shipping a source change also required a lockfile PR in DataDog/images, documented nowhere. Onboard `dockerfiles/services/` to generate-ci-images.php so the image is built, signed and pushed by CI into this repo's own registry namespace. Windows builds via the existing windows compose with an out-of-tree context; its Dockerfile now takes CI_REGISTRY_IMAGE as an ARG so it builds FROM the internal base, matching dockerfiles/ci/windows. No consumer is switched over yet: this only adds the ability to build. The `Services build` job is manual, like the other image builds.
Point the request-replayer service and the Windows docker run at registry.ddbuild.io/ci/dd-trace-php/dd-trace-ci:php-request-replayer-3.0, built and signed by the Services build job added in the previous commit. The image bakes in src/ (COPY src /var/www + composer install), so the runtime source-injection shim is no longer needed: - install-request-replayer-source.sh is removed. It waited on a $CI_PROJECT_DIR path with no timeout, so a missing checkout hung every request until the job timeout, and it touched its ready file after an unverified cp, so a partial checkout served a parse error for the rest of the job. - The service command keeps only the metrics-server supervisor, since index.php's own spawn races under PHP_CLI_SERVER_WORKERS. It now caps restarts and logs them instead of respawning silently forever. - test-request-replayer-concurrency.php is removed. It asserted the generated YAML still contained the router and installer strings, and ran in generate-templates ahead of the generators, so it would fail every pipeline. Requested by @bwoebi. - The laptop push targets for this image are dropped from dockerfiles/services/Makefile. Local-dev compose files still reference the Docker Hub 2.0 tag; they move once Services publish works.
The 3.0 image is now on Docker Hub, pushed by CI rather than from a laptop, so the local-dev compose files can move off 2.0.
Leiyks
marked this pull request as ready for review
August 25, 2026 14:51
Drop the obsolete top-level `version:` key and use the filename Compose looks for first, per review feedback. Teach generate-ci-images.php Compose's documented filename precedence rather than hardcoding docker-compose.yml, so the rename doesn't silently drop the Services jobs -- and make an unparseable compose file fail the generator instead of quietly emitting a pipeline with an OS missing. Existing compose files are left as they are.
Per review feedback, publish as request-replayer:3.0 rather than dd-trace-ci:php-request-replayer-3.0. The generator models the registry as one pipeline-wide variable, so override it for the Services jobs only instead of reshaping every matrix entry. No consumer is switched over yet: jobs keep pulling the existing dd-trace-ci tag until the new image has been built.
bwoebi
reviewed
Aug 25, 2026
Point the Linux service and the local dev compose files at request-replayer:3.0, now that the image is built, signed and published under its own repository. Windows still consumes dd-trace-ci:php-request-replayer-3.0_windows: its image is built from the shared windows compose, whose Dockerfiles resolve their FROM base through the same CI_REGISTRY_IMAGE variable, so moving it needs that base decoupled first.
Guard the UDP metrics server with flock() instead of a pid file, so the replayer looks after itself under PHP_CLI_SERVER_WORKERS rather than needing a shell supervisor in the CI service definition. metricsserver.php now holds an exclusive non-blocking lock for its whole lifetime, which the kernel releases when it dies. index.php trylocks the same file as a liveness probe and only spawns when it succeeds. Two workers racing is harmless: the loser's own LOCK_NB makes it exit before binding, so udp/80 can never be double-bound -- which the pid file could not prevent, since SO_REUSEADDR lets a second bind succeed and silently steal datagrams, and the pid file was written asynchronously by the wrapper shell after the spawn decision had already been made. Also stop re-raising warnings through trigger_error(), which is a ValueError on PHP 8 for any level outside E_USER_* and turned a failed lock open into an HTML fatal served as HTTP 200. The service command is not changed here: that follows once the image has been rebuilt.
The rebuilt image guards the UDP metrics server with flock(), so the shell supervisor in the service definition is redundant: restore the plain command and drop the flag that suppressed the in-image spawn. The replayer is now self-contained -- nothing about running it correctly under PHP_CLI_SERVER_WORKERS lives in this file any more.
bwoebi
approved these changes
Aug 25, 2026
bwoebi
left a comment
bwoebi
left a comment
Collaborator
There was a problem hiding this comment.
Looks good to me, and tests seem to work
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
5 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
High-level change and expected CI impact report
Stacked on #4097.
test_extension_ciruns as many as 12 PHPT workers against one single-threaded request-replayer. If its worker gets stuck processing a request,/replayalso blocks. The test process then waits on agent requests until the job reaches its two-hour timeout.Run request-replayer with 16 PHP CLI workers. Its file-backed queues use read-modify-write operations, so each session now has a file lock around state changes. MessagePack decoding stays outside the lock. Requests for different sessions can still run at the same time, including the un-tokened PHPT path.
The image is built and signed by CI (
Services buildin theci-imagespipeline) withsrc/baked in, so no runtime source injection is needed. UDP metrics runs under a restart loop. It publishes an immutable versioned tag (php-request-replayer-3.0), replacing the manual Docker Hub push and theDataDog/imagesmirror digest bump.This is separate from #4097 (now merged): that PR splits normal and valgrind passes and raises test parallelism; this PR keeps request-replayer responsive under that parallel workload.
Verification
generate-templatesimage with one CPU and 256 MiB. Child logs use files rather than undrained pipes and are printed if the test fails.git diff --checkis clean.Reviewer checklist