fix(internal/civisibility): make parallel tests safe across retries#4722
Conversation
🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: 27213f5 | Docs | Datadog PR Page | Give us feedback! |
Codecov Report❌ Patch coverage is Additional details and impacted files
🚀 New features to boost your workflow:
|
BenchmarksBenchmark execution time: 2026-05-05 17:37:30 Comparing candidate commit 27213f5 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 258 metrics, 8 unstable metrics.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bffedf440a
ℹ️ 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".
3e8a286
into
main
### What does this PR do? Fixes several Go CI Visibility edge cases found while reviewing retry, cleanup, coverage, and Git metadata handling after #4717 and #4722. This PR updates Go test retry orchestration so cleanup failures behave like normal test failures. A cleanup `panic` is recorded on the failing attempt, cleanup `Fatal` / `FailNow` no longer escapes the retry loop through `runtime.Goexit`, and cleanups now run after Datadog-managed parallel subtests have completed so parent cleanup order remains consistent with Go's testing lifecycle. It also fixes Orchestrion subtest panic handling when a Datadog retry wrapper owns the execution. In that case, subtest panics are recorded as failed attempts and returned to the retry owner, instead of immediately repanicking before retries can run. Other fixes included here: - stop automatic flaky retries when the global retry budget reaches zero - protect benchmark instrumentation map writes with the write lock - synchronize coverage writer payload rotation between `add` and `flush` - report `test.code_coverage.enabled=false` when ITR is enabled but coverage is not actually active - avoid an index panic when Git reports a pack hash but the expected `.pack` file is missing ### Motivation The previous CI Visibility retry fixes addressed known lifecycle bugs, but a follow-up review found more cases where retry attempts could be misclassified, skipped, or allowed to race with shared state. The highest-risk issues were in Go cleanup and Orchestrion subtest panic flows. Both are part of the real test result, so they need to pass through the same retry owner that handles failures in the test body. Without that, a cleanup failure could be hidden or abort retry orchestration, and a retryable subtest panic could fail the process before Datadog could schedule the next attempt. The smaller fixes remove incorrect state reporting and race-prone code paths found during the same review. ### Testing - [x] `go test ./internal/civisibility/integrations/gotesting/failnow -count=1` - [x] `go test ./internal/civisibility/integrations/gotesting -count=1` - [x] `go test ./internal/civisibility/integrations/gotesting/subtests -count=1` - [x] `go test ./internal/civisibility/utils -run 'TestCreatePackFiles|TestGit|TestPackFiles' -count=1` - [x] `go test -race ./internal/civisibility/integrations/gotesting/coverage -count=1` - [x] `Bypass=true go test -race ./internal/civisibility/integrations/gotesting -run TestGetFieldPointerFrom -count=1` - [x] `PATH="$(go env GOPATH)/bin:$PATH" GOFLAGS="-timeout=30m -tags=githubci,buildtag" go run github.com/DataDog/orchestrion go test -count=1 ./parallel_testing_retries` - [x] `git diff --check main..HEAD` ### Reviewer's Checklist <!-- * Authors can use this list as a reference to ensure that there are no problems during the review but the signing off is to be done by the reviewer(s). --> - [ ] Changed code has unit tests for its functionality at or near 100% coverage. - [ ] [System-Tests](https://github.com/DataDog/system-tests/) covering this feature have been added and enabled with the va.b.c-dev version tag. - [ ] There is a benchmark for any new code, or changes to existing code. - [ ] If this interacts with the agent in a new way, a system test has been added. - [ ] New code is free of linting errors. You can check this by running `make lint` locally. - [ ] New code doesn't break existing tests. You can check this by running `make test` locally. - [ ] Add an appropriate team label so this PR gets put in the right place for the release notes. - [ ] All generated files are up to date. You can check this by running `make generate` locally. - [ ] Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild. Make sure all nested modules are up to date by running `make fix-modules` locally. Co-authored-by: tony.redondo <[email protected]>
What does this PR do?
Fixes CI Visibility retries for Go tests that call
t.Parallel().When CI Visibility runs cloned executions of the same test for Early Flake Detection, automatic flaky retries, or attempt-to-fix, each clone can call
t.Parallel(). Before this change, every clone forwarded that call to the same original*testing.T, which made Go panic with:This PR adds retry-group coordination so only one cloned execution forwards
t.Parallel()to the original test. Duplicatet.Parallel()calls within the same execution are still forwarded deliberately, so Go keeps producing the standard duplicate-call panic for real user-code bugs.It also fixes a related retry lifecycle issue found while validating the auto-retry scenario:
FailNow/t.Fatalno longer shuts down CI Visibility while an additional-feature retry wrapper is still active, allowing later retry attempts to emit their test events correctly.The PR includes Orchestrion integration coverage for:
t.Parallel()and no retryt.Parallel()callst.Parallel()callsMotivation
A user reported that enabling Go automatic instrumentation with test retries caused tests that call
t.Parallel()once to fail withtesting: t.Parallel called multiple times. This happened because retry clones all pointed back to the same original*testing.Tand each clone forwarded its ownParallel()call.The fix keeps CI Visibility retry orchestration internal and preserves normal Go testing semantics: retry clones no longer create false duplicate-Parallel panics, while real duplicate
t.Parallel()calls still fail the same way they do without CI Visibility.Validation
go test -count=1 ./internal/civisibility/integrations/gotesting/...go test -race -count=1 ./internal/civisibility/integrations/gotesting/...go test -count=1 ./internal/orchestrion/_integration/civisibilitytestcd internal/orchestrion/_integration && go list ./... | grep -v /civisibility | grep '/parallel_testing_retries$'cd internal/orchestrion/_integration && PATH="$(go env GOPATH)/bin:$PATH" GOFLAGS="-timeout=30m -tags=githubci,buildtag" orchestrion go test -count=1 -shuffle=on ./parallel_testing_retries/...cd internal/orchestrion/_integration && PATH="$(go env GOPATH)/bin:$PATH" GOFLAGS="-timeout=30m -tags=githubci" go test -count=1 -shuffle=on -toolexec='orchestrion toolexec' ./parallel_testing_retries/...Reviewer's Checklist
make lintlocally.make testlocally.make generatelocally.make fix-moduleslocally.