ci: quarantine flaky tests against a ticket instead of retrying past them - #777
Draft
rkennke wants to merge 4 commits into
Draft
ci: quarantine flaky tests against a ticket instead of retrying past them#777rkennke wants to merge 4 commits into
rkennke wants to merge 4 commits into
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.
Contributor
Scan-Build Report
Bug Summary
Reports
|
||||||||||||||||||||||||||||||||||||
Contributor
CI Test ResultsRun: #33873197693 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-09-04 12:50:30 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>
Contributor
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>
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
1 participant
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.
What does this PR do?:
Makes flaky tests a tracked, expiring debt instead of something CI quietly retries past.
The retry now buys a label, not a pass:
A flake fails the build until someone quarantines it against a PROF ticket in
ddprof-test/quarantine.txt. Every entry carries areview_bydate and CI fails once it passes, so the list expires rather than accumulating. Quarantined tests still run and 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 ready-to-paste entry for each flaky test, with
cellspre-narrowed to the axis that actually failed.Motivation:
CI could not tell a flaky test from a broken one, and its one retry erased the evidence either way.
::warning::in a log, naming nothing.if: failure()— so the run that recovered on a retry, the one worth studying, produced no artifact at all.generate-test-summary.shalready downloaded(test-reports)*and greppedTEST-*.xmlfor failed test names, butprepare_reports.shcopiesbuild/reports/tests(the HTML) and neverbuild/test-results(the XML). It searched artifacts containing no XML, so every failed job rendered "No detailed failure information available". A**glob with noshopt -s globstarwould have stopped it recursing even had the files been there.Answering "is this flaky?" therefore meant hand-comparing PRs by eye. Retrying until green would only have made the tolerance official.
Additional Notes:
Why a text table and not JSON/YAML. The list is edited by hand far more often than by machine, so real comments, one-line diffs and clean
git blamematter more than a schema. It also has to parse inside the Alpine test containers, where PyYAML cannot be assumed — this needs nothing butstr.split.Failing to classify is itself a failure. If
flake_report.pycannot run, the job goes red rather than inheriting a pass nothing examined. An earlier draft had|| truethere and turned a real flake green during testing; there's a regression test for it now.Behaviour changes worth knowing: retry is enabled for all four platform jobs (previously ASan only), 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 its retry-on-abort via
RETRY_ON_NO_TEST_FAILURES, since it names no test when it dies at init.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.
Not exercised on real CI: the retry and gating paths only run once something has failed, which is why they are covered by fixtures instead.
Review fixes (second commit). Two defects worth naming. 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 — it now refuses to zero the exit code if the attempt log shows
Execution failed for taskoutside the test task. And the cell glob documented inquarantine.txtcould never match: cells end inamd64/aarch64, so the advertised*arm64*narrowing silently quarantined everywhere.validatenow rejects a glob naming an architecture CI never builds. Also:flakyrequires observing an actual pass rather than mere absence from a later attempt; the runner clears its own evidence directory so a reused workspace cannot import a previous run's attempts; Docker-written results are made readable before snapshotting, which musl-aarch64 was failing silently.Review fixes (third commit). The excuse was scoped to the wrong thing. The exit code quarantine overrides comes from the final attempt, but the failures weighed against it were aggregated across all of them — so a final attempt that failed without naming a test (a
docker runfailure on musl-aarch64, a Gradle configuration error, an OOM-killed daemon, or the very ASan init abort the retry exists for, which Gradle attributes to:ddprof-test:testasan) went green as soon as one entry matched an earlier attempt's failure.flake_report.pynow reports the final attempt's own standing and the runner requires that attempt to have produced results with all of its own named failures quarantined; the non-test-task grep stays as a second check rather than the only one.Also in that commit:
validaterejects overlapping cell globs rather than only byte-identical ones, and an out-of-rangereview_bybecomes an annotated problem instead of an uncaughtValueErrorthat loses every other annotation in the file; the deadproposesubcommand goes, leaving one renderer for the paste-ready entry; test ids and failure messages are sanitised before reaching the PR comment, so a test's own output cannot break out of the fenced proposal a reviewer is invited to copy; a summary with no readable reports says so instead of looking clean; the cell label carries the slow/regular axis so nightly's two invocations of one config stop colliding inci-outcome/<cell>.json; andmake_results_readableprobes per-file ownership instead of the top of the tree.How to test the change?:
.github/scripts/tests/test_quarantine.sh— 27 assertions, and.github/scripts/tests/test_generate_test_summary.sh— 2, both run by the newvalidate-quarantinejob:review_by, duplicate entry, malformed line, a missing list file, a cell glob naming an architecture CI never builds, two entries for one test on disjoint cells, and that the committed list is itself valid*aarch64*matches aarch64 cells and not amd64), class wildcards (a.B.*)attempt-*directory is ignored whileattempt-1is still read; atestcasewith no name is not countedEach gating guard was mutation-checked: reverting it individually turns the corresponding assertion red.
For Datadog employees: