Conversation
…them CI could not tell a flaky test from a broken one, and its one retry erased the evidence either way. - The retry existed only for ASan. A test that failed on attempt 1 and passed on attempt 2 left a green job and a ::warning:: in a log, naming nothing. - Test reports upload only `if: failure()`, so the run that recovered on a retry -- the one worth studying -- produced no artifact at all. - generate-test-summary.sh already downloaded `(test-reports)*` and grepped TEST-*.xml for failed test names, but prepare_reports.sh copies build/reports/tests (the HTML) and never build/test-results (the XML), so it searched artifacts containing no XML and every failed job rendered "No detailed failure information available". A `**` glob with no `shopt -s globstar` would have stopped it recursing even had the files been there. Retrying until green would only have made the tolerance official. Instead the retry now buys a label and nothing else, and an explicit list decides what may fail: flaky failed one attempt, passed another broken failed every attempt gating not on the quarantine list -- red, whichever of the above it is So a flake fails the build until somebody quarantines it against a PROF ticket. ddprof-test/quarantine.txt is a plain "|"-separated table, one entry per line, chosen over JSON/YAML because it is edited by hand far more than by machine: real comments, one-line diffs, clean git blame, and no parser beyond str.split (it must also load inside the Alpine containers, where PyYAML is not a given). Every entry carries a ticket and a review_by date, and validate-quarantine fails CI once that date passes -- otherwise the list only grows and quarantine becomes a permanent mute rather than tracked debt. Quarantined tests still run and still report; only the gating is suspended, so the pass rate keeps saying whether the test is recovering or has quietly become permanently broken. To keep the honest path the cheap one, the PR comment prints a filled-in entry to paste, with a `cells` glob narrowed to the axis that actually failed. The ticket and the judgement stay with a person; the typing does not. Reporting is grouped by test rather than by cell -- one flaky test reddens a dozen cells and so do a dozen unrelated breakages -- and per-cell outcomes now upload whether the cell passed or failed, since a cell that failed only on its first attempt produces no failure artifact. Failing to classify is itself a failure: if flake_report.py cannot run, the job goes red rather than inheriting a pass nothing examined. An earlier draft had `|| true` there and turned a real flake green in testing. test_quarantine.sh covers the gating decisions against fixtures, including that an un-quarantined flake stays red, a quarantined one does not, a build error is never excused by the list, and an unreadable list cannot yield green. It runs in the validate-quarantine job. The retry path only executes once something has failed, so CI would otherwise never exercise it. Deferred: auto-filing PROF tickets (needs dedupe and an Atlassian credential for CI) and the GitLab dd-trace integration matrix, which still gets one shot per config.
Scan-Build Report
Bug Summary
Reports
|
||||||||||||||||||||||||||||||||||||
CI Test ResultsRun: #34965861903 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-09-15 12:13:19 UTC |
Two defects that would have mattered: Quarantine excused too much. When every named test failure was on the list, the runner forced a green exit -- including when the same invocation had also failed a native or verification task, which the list has no business excusing. The runner now scans the attempt's log for `Execution failed for task` naming anything outside the test task and refuses to zero the exit code. The documented cell glob could never match. Cells are named <libc>-<jdk>-<config>-<arch> with arch amd64 or aarch64, so the `*arm64*` in quarantine.txt's example and in flake_summary.py's axis list matched nothing: the narrowing they advertised silently quarantined everywhere. Both use aarch64 now, and `validate` rejects a glob naming an architecture CI never builds. Also: - flaky now means failed once and observed passing on another attempt, not merely absent from it. An attempt that aborted early no longer turns every earlier failure into a flake with a paste-ready entry. - the runner clears its own evidence directory, so a reused workspace cannot contribute a previous run's attempts to this run's gating. - the counter and the gating read are checked rather than defaulted to zero, matching the fail-loud policy already applied to the classifier. - Docker-written results are made readable before snapshotting, and the snapshot warns instead of discarding errors; musl-aarch64 was losing flake classification silently. - pipes in failure messages are escaped, flaky tests appear in the per-job details, and an unparseable outcome report is visible rather than rendering as a clean non-test failure. - validating a missing list fails instead of reporting zero problems; duplicate detection keys on the cell globs, so narrowing by cell is actually usable. - one first-match helper shared by both selection paths. The regression test for the classifier guard did not exercise it -- a malformed line is skipped, not fatal, so the flake was the reason for the red. It now points --list at a directory to make the classifier genuinely fail. Each new guard was mutation-checked: reverting it turns the corresponding test red. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The exit code the quarantine list overrides comes from the final attempt, but the failures it was weighed against were aggregated across all of them. A final attempt that failed without naming a test -- a docker failure in the musl-aarch64 job, a Gradle configuration error, an OOM-killed daemon, the ASan init abort this retry exists for -- was excused as soon as one entry matched a failure from an earlier attempt. flake_report.py now reports the final attempt's own standing and the runner refuses to zero the exit code unless that attempt produced results with every one of its own named failures quarantined; the non-test-task grep stays as a second line of defence rather than the only one. Alongside it: - validate rejects overlapping cell globs, not just byte-identical ones, and reports an out-of-range review_by as an annotated problem instead of an uncaught ValueError that loses every other annotation in the file - the dead `propose` subcommand goes; flake_summary.py already renders the paste-ready entry CI actually uses - test ids and failure messages are sanitised before they reach the PR comment, so a test's own output cannot break out of the fenced quarantine proposal a reviewer is invited to copy - a summary with no readable outcome reports says so rather than looking like a clean run - the cell label carries the slow/regular axis, so nightly's two invocations of the same config stop colliding in ci-outcome/<cell>.json - testcase elements with no name are skipped instead of being counted and proposed for quarantine as "." - make_results_readable probes per-file ownership rather than the top of the tree, and covers the parent so the pre-attempt rm -rf can unlink it - flake-evidence/ and ci-outcome/ are gitignored Tests: the clean-pass path and the final-attempt-named-no-test regression are now covered (27 assertions), and a new test_generate_test_summary.sh pins generate-test-summary.sh's jq failure branch -- verified by removing the `!` and watching it go red. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A final attempt that crashes part-way through still writes JUnit XML for the tests it reached, and those passed -- so it names no failure of its own while every aggregated failure is quarantined. Gradle attributes the abort to the test task itself, so the non-test-task check cannot see it either. The final attempt's own exit code is the only thing that tells this apart from an ordinary flaky-then-passed run, so the runner now hands it to the classifier, which gates a non-zero exit that named nothing. Two evidence-integrity holes alongside it: - snapshot() takes read access again before copying. The XML is written by the command that just ran, after the loop-top call, and under Docker it lands root-owned; without this the copy fails and the cell loses flake classification silently. - make_results_readable() reports failure instead of warning and returning success, and its callers turn that into EVIDENCE_SUSPECT. A snapshot missing root-owned files is indistinguishable from an attempt whose missing tests all passed, which is exactly what the quarantine list must not be allowed to excuse. Three assertions cover these; each was mutation-checked individually. One of them asserts the ordinary quarantined-flake-recovers case stays green, so the new gate cannot be satisfied by reddening everything. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
musl produced no JUnit XML, so every mechanism here read nothing for it: the retry never fired, no failure could be classified, and a passing attempt was indistinguishable from one that never ran. ProfilerTestRunner now registers a LegacyXmlReportGeneratingListener when -Dtest.reportsDir is set, and ProfilerTestPlugin points it at build/test-results/<taskName> -- the same layout Gradle's own Test task uses, so the TEST-*.xml scan finds musl's results exactly as it finds every other cell's. Adds junit-platform-reporting to the testing bundle. Four gating holes: - A non-zero final attempt gates regardless of how many of its own failures were named and quarantined. A JVM can abort part-way through *after* naming a real quarantined failure, leaving the tests it never reached absent from the XML rather than passing; requiring zero named failures let one quarantined name paper over the crash. - quarantine.py enforces review_by at match time, not only in the validate subcommand. validate-quarantine runs from ci.yml alone, while find_entry is reached from every workflow that reuses run_tests_with_retry.sh, so an expired entry could keep excusing failures in nightly and release-validated. - cells_glob listed its axes out of cell-name order. Cell names read <libc>-<jdk>-<config>-<arch>[-slow] and the glob is built by `*`-joining, so an axis order of aarch64 before musl produced `*aarch64*musl*` -- a proposal that fnmatch can never match against the cells it was derived from. A test now asserts every generated glob matches its own cells. - A failed snapshot sets EVIDENCE_SUSPECT rather than only warning, so results that could not be copied reach the classifier as suspect evidence instead of looking like an attempt whose missing tests all passed. All 35 quarantine tests and 3 summary tests pass; :ddprof-test:compileTestJava succeeds with the new dependency. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A quarantined test that fails makes Gradle exit non-zero by itself, so gating
on any non-zero final-attempt exit code gated the ordinary case the list
exists to excuse. Only "failed on an earlier attempt, then passed outright on
the final one" could ever be excused, and on slow suites -- MAX_ATTEMPTS=1, so
no earlier attempt exists -- the list had no effect at all. That contradicts
quarantine.txt's first line.
A non-zero exit now gates only with positive evidence that the attempt was cut
short rather than run to completion:
- Gradle reporting a dead test JVM in the attempt log ("finished with non-zero
exit value"), or the JVM's own fatal-error banner
- the final attempt having reached fewer tests than another attempt managed,
which needs more than one attempt and so does not help slow suites
Both are reported in the outcome JSON. A missing log yields no evidence and
does not gate, matching how non_test_task_failures() already treats it.
No test passed --final-attempt-exit-code, so it defaulted to None, took the
(None, 0) exemption, and the whole guard went unexercised. Three fixtures now
cover it: a quarantined failure with exit 1 and an ordinary test-task failure
in the log must not gate; the same with a dead-JVM log must gate; and a final
attempt that reached fewer tests than an earlier one must gate. 38 assertions
pass, and each new guard was mutation-checked.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Gradle records a JUnit 5 @test method as name="method()", so the id built from the report is Class.method(), while quarantine.txt documents -- and its own worked example uses -- Class.method. covers() compares by exact equality, so an entry written exactly as documented matched nothing: the failure still gated, validate still reported the list as valid, and the only symptom was a quarantine that quietly excused nothing. Confirmed against this repo's own reports, which carry both name="attributeOverflowReturnsFalse()" and name="[1]". normalise_test_id() strips the trailing parentheses, and covers() applies it. That keeps the normalisation in the one function every caller's match goes through, and leaves reports and annotations showing the id JUnit produced rather than a rewritten one. Two shapes can never match, so validate now rejects them instead of passing them as valid: - a test field carrying JUnit's parentheses, which the normalisation above makes unnecessary - one whose last segment is an invocation index. @ParameterizedTest and @RetryingTest invocations appear as "[1]", "[2]" with no method name at all, so the index identifies neither the method nor a stable case; the class-wide "<class>.*" form is the only thing that can cover them, and quarantine.txt now says so. The paste-ready proposal keeps a real method name for the first shape. sanitize_quarantine_test_pattern() widened anything containing punctuation to the whole class, and "()" is punctuation, so every per-method proposal came out as <class>.* -- muting far more than the evidence supported. The parens are now stripped before that test, while an indexed invocation still proposes the class, which is correct for it. 42 assertions pass. Each guard was mutation-checked: dropping the normalisation, either validation, or the precise proposal turns its own assertion red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every item here shares one shape: something the machinery could not read, or could not express, was treated as permission to proceed rather than as a reason to stop. Each now has a fixture, and each fixture was mutation-checked against its own guard. quarantine.py - An unreadable review_by (blank, or not a calendar date -- parse() accepts both) made an entry never expire. Since validate() only runs in PR CI while find_entry() is reached from nightly and release-validated too, the one entry nobody can review outlived every entry that could. It now counts as expired. - A trailing ".*" was an unbounded prefix match, so "com.datadoghq.profiler.*" suspended the merge gate for the whole repository from a single validate-clean line. It now covers one class's methods, as quarantine.txt has always claimed, and validate rejects a pattern that reads as a package. - A bare ".*" was validated against the empty string and accepted, then matched nothing. - The synthetic cell universe listed a non-existent 8-graal and omitted every -j9/-ibm/-orcl variant plus 21-graal and 25-graal. cells_overlap() fails closed for a glob matching nothing synthetic, so two genuinely disjoint entries (say *17-j9* and *21-graal*) were rejected as duplicates. The JDK axis is now the cross-product of the bases and the suffixes CI builds. flake_report.py - --evidence-suspect gated unconditionally, before establishing there was any failure to excuse: a suite that passed while its results tree could not be cleared was turned from exit 0 into a red job. Suspect evidence is a reason to distrust a quarantine excuse, so it now applies only where an excuse is being weighed. - A single attempt cannot distinguish flaky from broken -- passed_in is empty for every failure because nothing re-ran -- yet every slow-suite failure was reported as "persistent" and offered no quarantine entry. Those now land in an "unclassified" bucket which the summary renders with proposals and says plainly that flakiness was not measured. flake_summary.py - cells_glob() matched a hardcoded token list, omitting the JDK and the regular/slow axes -- the two the matrix varies along most. A flake seen only on glibc-8-j9-debug-amd64 proposed *glibc*debug*amd64*, quarantining it on all 13 JDKs and on the slow suite too. The axes are now read positionally out of the cell grammar, so an axis nobody thought of cannot be dropped. - A proposal widened to the whole class (the only form covers() can express for an invocation index) said nothing about it. It now carries a WIDENED line naming the id that could not be expressed exactly. run_tests_with_retry.sh - An empty command list degraded to a bare redirection, exited 0, recorded no attempt, and passed the cell without running a test. - A destination that could not be cleared left cp -r merging this attempt's evidence with an earlier one's, so attempt_results() reported tests as observed-and-passed that this attempt never ran -- which is how a persistent failure acquires a flaky label and a paste-ready entry that buries a real defect. Both that and the run-start clear now record suspect evidence instead of discarding the status. prepare_reports.sh - The fallback that ships build/test-results keyed off the attempt-* directory, which snapshot() creates before the copy that may fail. An empty or partial snapshot therefore uploaded an artifact with no JUnit XML in it. It now tests for actual XML, and has its own hermetic suite, wired into the validate job. ProfilerTestRunner - The musl XML path discarded mkdirs()'s result, and the listener reports write failures to a PrintWriter rather than throwing, so a musl run could finish with no TEST-*.xml and look like a cell with nothing to report. It now fails loudly when reports were requested and cannot be written. Also: the non-test-task guard was asserted on stdout the fixture echoed itself, so deleting it left the test green; it is now asserted on the recorded gate_reason with the fixture exiting 0 to isolate it from the exit-code guard. And the job-name regex is checked against test_workflow.yml's own name: expressions, so a rename fails in the validate job rather than silently rendering a summary that reports zero test jobs. 56 quarantine assertions, 4 summary, 2 prepare-reports. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Running the quarantine scripts or their tests locally leaves .github/scripts/__pycache__/, which showed up as untracked in every worktree that had run them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
A normal musl test failure makes Gradle print the same text that the classifier treats as a dead test JVM. This makes every quarantined musl failure keep the CI job red.
🤖 Datadog Autotest · Commit 5f01c39 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| # itself cannot separate "the quarantined test failed" from "the JVM died with | ||
| # tests still unrun" -- but Gradle names the latter explicitly. | ||
| _CUT_SHORT_RE = re.compile( | ||
| r"finished with non-zero exit value" |
There was a problem hiding this comment.
Normal musl failures look like test JVM crashes
Quarantine cannot suspend CI gating for musl test failures.
Assertion details
- Input: A musl test cell completes with a quarantined test failure.
- Expected:
A complete musl run with only quarantined failures must not gate the CI job. - Actual:
ProfilerTestRunner exits with code 1 for a normal assertion failure. Gradle Exec then prints this marker. The classifier treats the run as cut short and keeps the job red.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest · Open Bits AI session
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f01c39cc0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # itself cannot separate "the quarantined test failed" from "the JVM died with | ||
| # tests still unrun" -- but Gradle names the latter explicitly. | ||
| _CUT_SHORT_RE = re.compile( | ||
| r"finished with non-zero exit value" |
There was a problem hiding this comment.
Distinguish ordinary musl Exec failures from JVM crashes
When a quarantined test fails on a musl cell, ProfilerTestRunner exits 1 and the Gradle Exec task reports that its process “finished with non-zero exit value”; this regex therefore marks every ordinary persistent or single-attempt musl test failure as a cut-short run. The gating branch then overrides the quarantine hit and leaves both musl workflows red, so quarantine cannot work for slow musl suites or for a failure that reproduces on retry. Restrict this marker to native Test-executor crashes or otherwise account for musl's Exec task semantics.
AGENTS.md reference: AGENTS.md:L221-L225
Useful? React with 👍 / 👎.
| widths = {len(f) for f in fields} | ||
| if len(widths) != 1: | ||
| return None |
There was a problem hiding this comment.
Keep proposals narrow across mixed JDK label widths
When the same failure occurs in cells such as glibc-8-j9-debug-amd64 and glibc-17-debug-amd64—both forms occur in the workflow matrix—the split arrays have different widths and this returns None. render_proposals() converts that to an empty cells list, which format_entry() renders as -, so the paste-ready proposal quarantines the test on every libc, configuration, architecture, and slow suite rather than just the observed cells. Parse the fixed axes from the ends or fall back to listing the observed cells instead of proposing a global quarantine.
Useful? React with 👍 / 👎.
What does this PR do?:
Makes flaky tests a tracked, expiring debt instead of something CI quietly retries past.
A retry now buys a label, not a pass:
A flake fails the build until it is quarantined against a PROF ticket in
ddprof-test/quarantine.txt. Entries carry areview_bydate and expire: once it passes, the entry stops excusing anything and CI goes red again. Quarantined tests still run and report — only the gating is suspended, so the pass rate still says whether a test is recovering or has quietly become permanently broken. The PR comment prints a paste-ready entry for each flake, withcellsnarrowed to the axes that actually failed.Only the final attempt's own named failures are weighed against the list, and
flakyrequires observing a pass rather than mere absence from a later attempt. The list cannot excuse:Conversely, suspect evidence with no failure recorded holds no opinion: it is a reason to distrust an excuse, not a test failure, so a suite that passed stays green.
Motivation:
CI could not tell a flaky test from a broken one, and its single retry erased the evidence either way:
::warning::naming nothingif: failure(), so the recovered run — the one worth studying — produced no artifactgenerate-test-summary.shgreppedTEST-*.xmlout of artifacts that only ever heldbuild/reports/tests(HTML), neverbuild/test-results(XML), so every failed job rendered "No detailed failure information available"Answering "is this flaky?" therefore meant hand-comparing PRs by eye. Retrying until green would only have made the tolerance official.
Additional Notes:
musl coverage.
ProfilerTestRunnerruns musl's tests through the JUnit launcher directly and registered no XML listener. It now attaches aLegacyXmlReportGeneratingListenerwhen-Dtest.reportsDiris set, andProfilerTestPluginpoints that atbuild/test-results/<taskName>— the layout Gradle's ownTesttask uses — so theTEST-*.xmlscan finds musl's results the same way it finds every other cell's. It fails loudly if reports were requested and cannot be written, since producing none silently is indistinguishable from a cell with nothing to report.Patterns match the ids JUnit writes. Gradle records a
@Testmethod asname="method()", so ids arrive asClass.method()while entries are writtenClass.method;covers()normalises that.@ParameterizedTest/@RetryingTestinvocations appear as[1],[2]with no method name at all, so they can only be quarantined class-wide. A trailing.*now covers one class's methods, as the format has always documented, rather than being an unbounded prefix — so one validate-clean line can no longer suspend the merge gate repo-wide.validaterejects all three unmatchable shapes, and a proposal widened to the class says so with aWIDENEDline.Expiry is enforced where matching happens, not only in the
validatesubcommand:validate-quarantineruns fromci.ymlalone, whilefind_entryis reached from every workflow reusingrun_tests_with_retry.sh. An unreadablereview_bycounts as expired rather than as never expiring.Proposals narrow on every axis.
cells_globreads the shared axes positionally out of the<libc>-<jdk>-<config>-<arch>[-slow]grammar rather than matching a token list, so the JDK and the regular/slow axes cannot be silently dropped — a flake seen on one cell no longer proposes muting it across 13 JDKs and the slow suite.Behaviour changes. Retry now covers all four platform jobs, not just ASan, so a flake-shaped failure costs that cell a second run; slow suites are capped at one attempt to stay clear of the 180-minute timeout; ASan keeps retry-on-abort via
RETRY_ON_NO_TEST_FAILURES, naming no test when it dies at init.Status check names change. The four matrix jobs gain an explicit
name:, so their checks go fromtest-linux-glibc-amd64 (17, debug)totest-linux-glibc-amd64 (17, debug, regular). The suffix is required: this workflow is called twice in one run (nightly, release-validated) with overlapping configs, and without it two different jobs collapse onto the same cell. No ruleset on this repo requires status checks by name, so nothing should block on the old strings — but anything outside GitHub that matches on them would need the new form.How to test the change?:
Three hermetic suites, all run by the new
validate-quarantinejob:test_quarantine.sh(56 assertions),test_generate_test_summary.sh(4),test_prepare_reports.sh(2). Between them they cover every validation rejection, glob matching in both directions (a proposal matches the cells it came from and excludes those differing only in JDK or in the slow suffix), each thing the list cannot excuse, the classification edges (absent-from-retry, single-attempt, unnamed testcase), an empty command list, an artifact still shipping XML when a snapshot came out empty, and the rendered output. The summary suite also checks the job-name regex againsttest_workflow.yml's ownname:lines, so a rename fails here rather than rendering a summary that reports zero test jobs.Every guard was mutation-checked: reverting it individually turns its own assertion red.
The retry and gating paths only run once something has failed, so nothing in CI has exercised them end-to-end — they are covered by fixtures instead. Worth knowing when reading the risk of this PR.
Known gaps, deliberately not in this PR:
ProfilerTestRunnerto confirm the shape matches whatflake_report.pyparses. A mismatch there makes the whole list a no-op while every assertion stays green — which is exactly what theClass.method()bug above was. The highest-value follow-up.python3 -cand a two-line stdout protocol, rather thanflake_report.pycommunicating its verdict directly.For Datadog employees: