Repository navigation
Wire GitLab gtest jobs into the test quarantine mechanism [PROF-15902] - #974
Conversation
covers() reduces gtest's decorated classnames (<inst>/<Suite>, <Suite>/<idx>) to the bare suite so a class-wide entry covers parameterized variants; the synthetic cell universe gains the jdk-less glibc-<config>-<arch> shape. flake_report.py stops counting gtest status=notrun records as observed. quarantine.txt documents the gtest id and cell shapes.
Replace the first-failure shell loop with run-gtest-quarantine.sh: gtest JUnit XML per binary, all binaries per attempt, the shared retry/quarantine wrapper with cell glibc-<config>-<arch>, and signal-deaths (rc > 128) outranking plain failures so a crashed binary is never read as a quarantined pass. TSan moves GTEST_DEATH_TEST_STYLE to job variables.
test_quarantine.sh gains gtest id-shape matching, 3-token cell validation, and end-to-end flake_report/runner cases; new hermetic test_run_gtest_quarantine.sh pins the attempt aggregation (signal rc wins, find failure vs clean zero match) and the asan-only retry flag, wired into both the GitHub shell checks and GitLab shell-unit-tests.
…d/cell construction The gtest synthetic cells list the same configs as the Java axis; reusing the constant removes a duplicate that could drift. test_run_gtest_quarantine.sh gains T7/T8 pinning the exact cell string passed to the retry wrapper and the unset-variable guard.
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Scan-Build Report
Bug Summary
Filter Results by FileReports
|
|||||||||||||||||||||||||||||||||||||||||||||||||
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfa63d7300
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
gtest suite ln (nativeThreadNameRace_ut) races thread death in the name scanner and fails ~daily on quiet CI runners. Quarantined everywhere: the failure is runner-load dependent, not config- or arch-specific. The entry uses the bare suite id the XML carries, not the Jira title.
![datadog-prod-us1-3[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
Quarantined assertion failures can mask an independent ASan or TSan failure, turning a sanitizer job green despite memory errors or data races. The aggregate signal check does not protect against ordinary sanitizer exit codes.
🤖 Bits Code Review · Commit bfa63d7
There was a problem hiding this comment.
The PR routes GitLab gtests through retry/quarantine handling and adds a thread-name race quarantine, but its new ln suite ID never matches the actual test. The earlier defect remains unfixed: quarantined assertions can still mask independent ASan/TSan failures and turn sanitizer jobs green.
🤖 Bits Code Review · Commit 46c4353
CI Test ResultsRun: #43033415602 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-11-11 17:41:58 UTC |
…the gating The quarantine entry named the ticket title's suite; the declaration is TEST(NativeThreadNameRaceTest, ...), so the entry matched nothing -- pinned by a test deriving the id from the source. A gtest binary aborted by a sanitizer diagnostic (ASan 1, TSan 66) leaves no XML and no signal exit; the runner now prints an ABORTED marker that flake_report.py reads as a cut-short run, so another binary's quarantined failures cannot excuse it. Tier 1 doc and AGENTS.md updated for the quarantine-gated contract.
There was a problem hiding this comment.
The PR adds retry/quarantine handling for GitLab gtests; the incorrect quarantine ID and handling of sanitizer exits without XML are fixed. One issue remains: TSan can write XML before exiting 66, allowing quarantined assertions in another binary to mask a real data race and turn the job green.
🤖 Bits Code Review · Commit 7fd66d6
…as a pass TSan's defaults (halt_on_error=0, exitcode=66) let gtest finish and write a passing JUnit XML before exiting 66 at shutdown -- a data race then bypasses both the ABORTED marker and the >128 check. The tsan cell now runs with halt_on_error=1: the first race kills the binary before the XML is written and the ABORTED path gates it. Config-specific env setup hoisted above the --attempt branch so the re-executed attempt sets it too.
… reject decorated suite entries A leftover TEST-*.xml from a reused workspace would satisfy the -s check and suppress the ABORTED marker; the attempt now clears the results dir first. The worst case is build + two full passes, so the job timeout moves to 45m. validate() rejects a decorated gtest suite name in a class-wide entry -- covers() reduces ids to the bare suite, so such an entry matches nothing.
…TSAN_OPTIONS doc claim
The bare substring grep swept a release_helper binary parked in an asan_* directory into the release cell and silently dropped an unprefixed binary from the right one. The hermetic fakes now use the real bin/gtest/<config>_<target>/<binary> layout.
…nt to what it pins
There was a problem hiding this comment.
More details
GitLab gtests now use shared retry/quarantine handling with per-binary XML. The quarantine ID is corrected, and TSan halts on the first race to route failures through abort detection; both previously reported defects are resolved, with no open findings.
🤖 Bits Code Review · Commit 0962e93
|
🔗 Commit SHA: 5bad328 | Docs | View more details | Give us feedback! |
…s budget On optimized builds the passes run fast enough that the abort windows cover nearly every pass read (measured 19/50 failures on release, 0/30 on debug): the root-enum retry is starved for the whole kMaxPasses budget and no search reaches a terminal outcome. Generalize the documented TSan skip to that case, gated on abort domination (aborted_passes >= passes/2) so a genuine never-terminates bug still gates. Failure asserts now carry the counters.
…ntry complaint; pin the covers boundary gtest still runs and can exit 0 when it cannot write its XML, so an unchecked mkdir yields a green-looking empty-evidence run (exit 3, like the rm -rf). validate() claimed a decorated class-wide entry matches nothing - covers() matches it directly and it is the narrower form, so both forms stay valid. Pins: 'a.B.*' must not cover the nested id a.B.c.d.
…cover their typed variants A binary that exits 0 without flushing its JUnit XML left no evidence and no signal: the ABORTED path now fires on a missing XML regardless of the exit code, and a zero exit is hardened to non-zero so the cut-short gate fires. covers() now matches a decorated class-wide entry against its own instantiation's typed variants (MyInst/WobblySuite.* covers MyInst/WobblySuite/0.case). The hermetic fakes honor --gtest_output, pinning the runner's XML-path and argument against renames.
The cwd-based lookup misses under the GitLab runner (repo-root cwd) and the exit(0) skip path bypassed gtest XML output, tripping the runner's exit-0-without-XML evidence-loss guard on every sanitizer cell.
…al caches loadSymbolTable aliased mapped bytes through an ElfSymbol*; a crafted sh_offset puts entries at addresses violating Elf64_Sym's 8-byte alignment, which UBSan halts on (exposed by the elfparser_ut tests this PR's runner made reachable). Copy each entry instead. The test's throwaway CodeCacheArrays leaked every parsed library; drain them explicitly.
What does this PR do?:
Wires the five GitLab sanitizer gtest jobs (
gtest-{asan,tsan,release}-{amd64,arm64}) into the flaky-test quarantine mechanism that already gates the Java cells and the GitLab el7 functional job. Each binary now emits JUnit XML (TEST-<binary>.xmlunderddprof-lib/build/test-results/gtest/<config>/, uploaded as artifacts), every attempt runs all binaries instead of exiting at the first failure, and the job verdict comes from the sharedrun_tests_with_retry.sh→flake_report.py→ddprof-test/quarantine.txtpath with cell namesglibc-<config>-<arch>. A failing gtest only excuses the job when it is on the quarantine list for that cell.To make gtest ids matchable,
quarantine.covers()reduces gtest's decorated classnames (<inst>/<Suite>,<Suite>/<type-idx>) to the bare suite, so a class-wideSuite.*entry covers plain, value-parameterized, and typed variants; a decorated entry additionally covers its own instantiation's typed variants (a.B/0.*⊇a.B/0.c/1), whilea.B.*stays narrower thana.B.c.*. The synthetic cell universe gains the jdk-less three-token gtest shape;flake_report.pystops counting gteststatus="notrun"records as observed.Motivation:
The gtest jobs failed hard on any test failure — no retry, no flake classification, no quarantine — so a flaky native test turned every branch-push pipeline red with no way to mute it, while the same failure in a Java cell only gated if it was not quarantined. Flaky gtests are a known recurring cost (see
PROF-16577,PROF-16566).Two design points worth calling out:
rc > 128) takes priority:flake_report.pyreads>128as a run cut short, so a crashed binary's missing tests can never be excused behind another binary's quarantined failures.findfailure vs clean zero match. A failedfindexits 2 with its own message; an empty match stays exit 1. Previously both collapsed to one path, and the zero-match case exited 0 (green with zero tests).Fixes surfaced by the new runner (found because the runner now refuses to pass without evidence):
exit(0), or a results-directory creation failure) prints anABORTED:marker and fails the attempt, so a green run with empty evidence is impossible. This is what exposedelfparser_ut.elfparser_utnow resolves its test resources from the executable location. The oldgetcwd-based lookup missed under the GitLab runner (repo-root cwd) and theexit(0)skip path bypassed gtest XML output;Elf.readSymTableand theElfReladynsuite have never actually run on CI. With/proc/self/exe-relative resolution andGTEST_SKIP, all 31 tests run and pass in the Linux container from both the repo root and/.GTEST_ARCHandSANITIZER_LCare enum-validated (amd64|aarch64,asan|tsan|release): a runner-tag spelling (arm64) or a typo fails the job loudly instead of building a cell string that matches no quarantine entry and mutes nothing silently.ReferenceChainsLifecycleTest.ChurnDuringConcurrentPassesStaysConsistentskips under abort-window starvation on optimized builds. Measured onglibc-release-amd64: 19 of 50 runs failed before the change, 0 after (35 pass / 15 skip). On fast builds the abort thread's duty cycle starves the root-enum retry (2s floor) inside the 2000-pass budget, sorestarts == 0and abort windows dominate pass reads — the test's own budget accounting, not the search under test. The skip mirrors the test's documented TSan skip and is gated on abort domination (aborted_passes >= total_passes/2), so a genuine low-abort never-terminates bug still gates. Accepted cost: a never-terminates defect whose churn also aborts at least half the passes would be skipped on every run rather than gated — disclosed for reviewer acknowledgment.Additional Notes:
GTEST_DEATH_TEST_STYLE=threadsafe, moved to job variables; the asan cell exportsRETRY_ON_NO_TEST_FAILURES=1(same ASan-init/ASLR abort as the GitHub asan matrix,google/sanitizers#856).How to test the change?:
.github/scripts/tests/test_quarantine.sh(78 assertions) — gtest id-shape matching, three-token cell validation (arm64spelling rejected), overlapping/disjoint gtest entries, decorated-variant coverage in both directions,notrunhandling, and end-to-endflake_report.py/run_tests_with_retry.shcases on gtest XML..gitlab/scripts/tests/test_run_gtest_quarantine.sh(42 assertions, hermetic) — signal-vs-plain exit precedence,findfailure vs zero match, exit-0-without-XML evidence loss, arch/config enum validation, asan-only retry flag, exact cell string, unset-variable guard; wired into both the GitHubci.ymlshell checks (incl. shellcheck) and GitLabshell-unit-tests.bash .gitlab/scripts/tests/test_run_gtest_quarantine.shand.github/scripts/tests/test_quarantine.sh.For Datadog employees:
credentials of any kind, I've requested a security review (run the
dd:platform-security-reviewskill, or file a request via the PSEC review form).
bewairealso runs automatically on every PR.