fix(ci-visibility): harden Go retry edge cases#4731
Conversation
🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: f600ace | Docs | Datadog PR Page | Give us feedback! |
021fd12 to
1f3a7d5
Compare
BenchmarksBenchmark execution time: 2026-05-06 10:04:32 Comparing candidate commit f600ace in PR branch Found 1 performance improvements and 0 performance regressions! Performance is the same for 268 metrics, 9 unstable metrics.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f3a7d5160
ℹ️ 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".
1f3a7d5 to
5bdcd7e
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
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
panicis recorded on the failing attempt, cleanupFatal/FailNowno longer escapes the retry loop throughruntime.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:
addandflushtest.code_coverage.enabled=falsewhen ITR is enabled but coverage is not actually active.packfile is missingMotivation
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
go test ./internal/civisibility/integrations/gotesting/failnow -count=1go test ./internal/civisibility/integrations/gotesting -count=1go test ./internal/civisibility/integrations/gotesting/subtests -count=1go test ./internal/civisibility/utils -run 'TestCreatePackFiles|TestGit|TestPackFiles' -count=1go test -race ./internal/civisibility/integrations/gotesting/coverage -count=1Bypass=true go test -race ./internal/civisibility/integrations/gotesting -run TestGetFieldPointerFrom -count=1PATH="$(go env GOPATH)/bin:$PATH" GOFLAGS="-timeout=30m -tags=githubci,buildtag" go run github.com/DataDog/orchestrion go test -count=1 ./parallel_testing_retriesgit diff --check main..HEADReviewer's Checklist
make lintlocally.make testlocally.make generatelocally.make fix-moduleslocally.