fix(ci-visibility): avoid Go FailNow teardown#4717
fix(ci-visibility): avoid Go FailNow teardown#4717gh-worker-dd-mergequeue-cf854d[bot] merged 2 commits into
Conversation
🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: 2c6f3bb | Docs | Datadog PR Page | Give us feedback! |
ef16981 to
12fa701
Compare
BenchmarksBenchmark execution time: 2026-05-05 14:48:01 Comparing candidate commit 2c6f3bb in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 261 metrics, 8 unstable metrics.
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ 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". |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
🚀 New features to boost your workflow:
|
51e0cfb
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?
Prevents Go CI Visibility from shutting down the tracer when a test or benchmark calls
FailNow.testing.T.FailNowandtesting.B.FailNowstop the current test goroutine viaruntime.Goexit; they do not end the whole test process. The previous CI Visibility wrappers and Orchestrion advice calledExitCiVisibility()from those paths, which could close the span pipeline mid-package and drop later test spans, retry attempts, and cleanup-created spans.This PR removes the premature teardown from:
gotesting.T.FailNowgotesting.B.FailNowtesting.common.FailNowThe real process teardown paths are left unchanged:
M.Run, signal handling, panic/re-panic paths, benchmark panic finalization, retry re-panic, and close actions still callExitCiVisibility().It also adds regression coverage for direct wrappers and Orchestrion-instrumented tests, including fatal assertions, retries, benchmarks, and cleanup spans created after
FailNow. No new dependency is introduced; module files are unchanged.Motivation
A customer reported that when any Go test called
FailNow,Fatal,Fatalf, orrequire.*, subsequent tests in the package were silently missing from CI Visibility and ATR retry spans were not reported.The root cause was that CI Visibility treated
FailNowlike a process exit, but Go only stops the current test or benchmark. Closing CI Visibility there loses spans that are created later in the same process.Testing
go test ./internal/civisibility/integrations/gotesting/failnow -count=1go test ./internal/civisibility/integrations/gotesting/... -count=1cd internal/orchestrion/_integration && go run github.com/DataDog/orchestrion go test ./civisibility -count=1cd internal/orchestrion/_integration && go run github.com/DataDog/orchestrion go test ./civisibility_failnow -count=1cd internal/orchestrion/_integration && go run github.com/DataDog/orchestrion go test ./civisibility_failnow_retry -count=1cd internal/orchestrion/_integration && go run github.com/DataDog/orchestrion go test ./civisibility_failnow_benchmark -run '^$' -bench . -benchtime=1x -count=1go test ./internal/orchestrion/_integration/civisibilitytestgit diff --checkgo.modandgo.sumare unchanged in both the root module and Orchestrion integration module.