fix(internal/civisibility): fix orchestrion deadlock with parallel subtests#4554
Conversation
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: 0fea100 | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback! |
BenchmarksBenchmark execution time: 2026-03-17 16:41:24 Comparing candidate commit 0fea100 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 156 metrics, 8 unstable metrics.
|
b689627 to
0fea100
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
🚀 New features to boost your workflow:
|
d224c35
into
main
…btests (#4554) ### What does this PR do? This fixes a CI Visibility deadlock when Orchestrion instruments tests that call `t.Parallel()` on a parent test and then also call `t.Parallel()` in subtests. The fix changes the `gotesting` Orchestrion advice for `testing.T.Parallel()` so we ask `instrumentTestingParallel` to forward the call before the stdlib `Parallel()` body runs, and return early when Datadog has already handled the handoff on the original wrapped test. That prevents the cloned wrapper test from blocking in its own local `Parallel()` path while still preserving real Go parallel scheduling. This PR also adds a real Orchestrion civisibility integration regression that exercises a parallel parent test with parallel subtests, so the YAML-driven `toolexec` path is covered directly. Validation run locally: - `go test ./internal/civisibility/integrations/gotesting -count=1` - `GOWORK=off go test -shuffle=on -toolexec='orchestrion toolexec' ./civisibility` ### Motivation This came from the Slack report. The customer reported that with `dd-trace-go v2.6.0` and `orchestrion v1.6.0`, tests using a parent `t.Parallel()` plus parallel subtests could hang indefinitely under Orchestrion. They were able to reproduce it with a minimal test case and `go test -toolexec='orchestrion toolexec' -timeout=30s -v ./...`. The failure mode matched our additional-feature wrapper path in `gotesting`: the wrapper executes the test body on a cloned `*testing.T`, waits for that cloned execution to finish, and the old Orchestrion `Parallel()` advice deferred the Datadog forwarding hook until after the local stdlib `Parallel()` call. Once the cloned test parked in `testing.(*T).Parallel()`, the forwarding hook never ran soon enough, which created the deadlock. This PR fixes that timing bug and adds coverage for the exact Orchestrion civisibility path that was reported. ### Reviewer's Checklist - [x] 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. - [x] 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. - [x] 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: kemal.akkoyun <[email protected]>
What does this PR do?
This fixes a CI Visibility deadlock when Orchestrion instruments tests that call
t.Parallel()on a parent test and then also callt.Parallel()in subtests.The fix changes the
gotestingOrchestrion advice fortesting.T.Parallel()so we askinstrumentTestingParallelto forward the call before the stdlibParallel()body runs, and return early when Datadog has already handled the handoff on the original wrapped test. That prevents the cloned wrapper test from blocking in its own localParallel()path while still preserving real Go parallel scheduling.This PR also adds a real Orchestrion civisibility integration regression that exercises a parallel parent test with parallel subtests, so the YAML-driven
toolexecpath is covered directly.Validation run locally:
go test ./internal/civisibility/integrations/gotesting -count=1GOWORK=off go test -shuffle=on -toolexec='orchestrion toolexec' ./civisibilityMotivation
This came from the Slack report. The customer reported that with
dd-trace-go v2.6.0andorchestrion v1.6.0, tests using a parentt.Parallel()plus parallel subtests could hang indefinitely under Orchestrion. They were able to reproduce it with a minimal test case andgo test -toolexec='orchestrion toolexec' -timeout=30s -v ./....The failure mode matched our additional-feature wrapper path in
gotesting: the wrapper executes the test body on a cloned*testing.T, waits for that cloned execution to finish, and the old OrchestrionParallel()advice deferred the Datadog forwarding hook until after the local stdlibParallel()call. Once the cloned test parked intesting.(*T).Parallel(), the forwarding hook never ran soon enough, which created the deadlock.This PR fixes that timing bug and adds coverage for the exact Orchestrion civisibility path that was reported.
Reviewer's Checklist
make lintlocally.make testlocally.make generatelocally.make fix-moduleslocally.