[CI Visibility] Fix coverage resolver assembly file locks#8666
Merged
Conversation
BenchmarksBenchmark execution time: 2026-05-21 15:23:15 Comparing candidate commit 05d895c in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 71 metrics, 0 unstable metrics, 60 known flaky benchmarks, 66 flaky benchmarks without significant changes.
|
This comment has been minimized.
This comment has been minimized.
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8666) and master. ✅ No regressions detected - check the details below Full Metrics ComparisonFakeDbCommand
HttpMessageHandler
Comparison explanationExecution-time benchmarks measure the whole time it takes to execute a program, and are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are highlighted in **red**. The following thresholds were used for comparing the execution times:
Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard. Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph). Duration chartsFakeDbCommand (.NET Framework 4.8)gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8666) - mean (73ms) : 70, 77
master - mean (73ms) : 70, 76
section Bailout
This PR (8666) - mean (77ms) : 75, 80
master - mean (77ms) : 75, 79
section CallTarget+Inlining+NGEN
This PR (8666) - mean (1,119ms) : 1051, 1186
master - mean (1,117ms) : 1055, 1179
FakeDbCommand (.NET Core 3.1)gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8666) - mean (115ms) : 107, 124
master - mean (114ms) : 109, 119
section Bailout
This PR (8666) - mean (119ms) : 113, 126
master - mean (118ms) : 113, 124
section CallTarget+Inlining+NGEN
This PR (8666) - mean (790ms) : 758, 823
master - mean (787ms) : 764, 810
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8666) - mean (106ms) : 100, 111
master - mean (105ms) : 100, 110
section Bailout
This PR (8666) - mean (105ms) : 99, 111
master - mean (103ms) : 100, 105
section CallTarget+Inlining+NGEN
This PR (8666) - mean (953ms) : 914, 992
master - mean (945ms) : 909, 982
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8666) - mean (100ms) : 96, 103
master - mean (100ms) : 95, 105
section Bailout
This PR (8666) - mean (105ms) : 99, 111
master - mean (104ms) : 99, 108
section CallTarget+Inlining+NGEN
This PR (8666) - mean (825ms) : 784, 865
master - mean (821ms) : 787, 854
HttpMessageHandler (.NET Framework 4.8)gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8666) - mean (202ms) : 197, 208
master - mean (202ms) : 197, 207
section Bailout
This PR (8666) - mean (206ms) : 202, 210
master - mean (206ms) : 202, 209
section CallTarget+Inlining+NGEN
This PR (8666) - mean (1,206ms) : 1158, 1255
master - mean (1,206ms) : 1160, 1253
HttpMessageHandler (.NET Core 3.1)gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8666) - mean (291ms) : 282, 300
master - mean (291ms) : 284, 298
section Bailout
This PR (8666) - mean (291ms) : 283, 299
master - mean (291ms) : 284, 297
section CallTarget+Inlining+NGEN
This PR (8666) - mean (975ms) : 947, 1002
master - mean (969ms) : 950, 988
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8666) - mean (281ms) : 274, 288
master - mean (283ms) : 277, 290
section Bailout
This PR (8666) - mean (282ms) : 275, 288
master - mean (283ms) : 277, 290
section CallTarget+Inlining+NGEN
This PR (8666) - mean (1,166ms) : 1122, 1209
master - mean (1,164ms) : 1119, 1208
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8666) - mean (279ms) : 271, 287
master - mean (281ms) : 274, 287
section Bailout
This PR (8666) - mean (279ms) : 272, 285
master - mean (282ms) : 276, 288
section CallTarget+Inlining+NGEN
This PR (8666) - mean (1,045ms) : 1006, 1084
master - mean (1,040ms) : 997, 1083
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
tonyredondo
marked this pull request as ready for review
May 20, 2026 19:52
![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: 6310ea5eb2
ℹ️ 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".
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses Windows file-locking failures in the CI Visibility coverage collector when multiple test assemblies are rewritten in parallel. It introduces an owned Cecil resolver that reads assemblies in-memory and coordinates per-assembly read/write access to prevent dependency reads from racing with target assembly writes (fixing issue #8592).
Changes:
- Introduce
CoverageAssemblyResolverthat caches/ownsAssemblyDefinitioninstances, reads dependencies in-memory, and disposes assemblies to avoid lingering file handles. - Add
CoverageAssemblyPathLockto provide per-assembly-path read/write locking and integrate it into target read/write and dependency resolution paths. - Refactor
CoverageCollectorretry logic and add focused resolver/locking unit tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tracer/src/Datadog.Trace.Coverage.collector/AssemblyProcessor.cs | Switch to the new owned resolver and add locked in-memory read + locked write helpers for the target assembly. |
| tracer/src/Datadog.Trace.Coverage.collector/CoverageAssemblyResolver.cs | New resolver that reads dependencies in-memory under per-path read locks and caches/disposes Cecil assemblies. |
| tracer/src/Datadog.Trace.Coverage.collector/CoverageAssemblyPathLock.cs | New per-path ReaderWriterLockSlim registry used to coordinate dependency reads vs. target writes. |
| tracer/src/Datadog.Trace.Coverage.collector/CoverageCollector.cs | Replace goto retry with a bounded attempt loop and avoid logging stale IO failures after success/skip. |
| tracer/test/Datadog.Trace.Tools.Runner.Tests/CoverageResolverTests.cs | New tests covering resolver caching/disposal and the per-path locking behavior (including Windows-only exclusive handle assertions). |
| tracer/test/Datadog.Trace.Tools.Runner.Tests/Datadog.Trace.Tools.Runner.Tests.csproj | Add Mono.Cecil reference needed by the new resolver tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
andrewlock
reviewed
May 21, 2026
andrewlock
left a comment
andrewlock
left a comment
Member
There was a problem hiding this comment.
LGTM as best as I can tell 😅
In the prod code, I've entirely just flagged stylistict and perf things, but I'm not sure how much of a concern the perf/allocation side is, so feel free to ignore.
In the tests, I have significant concerns about the use of timeouts and delays - these look practically guaranteed to flake in our overloaded CI. We should bump up "timeouts" that we expect to never fail to huge values, and ideally don't rely on timings at all, to keep the steps deterministic
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
Reason for change
When the coverage collector processes test assemblies in parallel, one assembly can resolve a sibling DLL as a dependency while another worker tries to rewrite that same DLL. Cecil can keep those resolved dependency handles open inside the same
dotnetprocess, so the rewrite worker repeatedly fails to open the DLL for read/write access.Fixes #8592.
Implementation details
AssemblyProcessorreads the target assembly into memory under a short per-path read lock, then releases that lock before metadata processing and dependency resolution continue.AssemblyProcessoracquires the per-path write lock only while writing the rewritten target assembly back to disk.CoverageAssemblyResolverreads output-folder dependencies through a per-path read lock, usesInMemory = true, caches ownedAssemblyDefinitioninstances, and disposes them with the resolver.CoverageCollectorkeeps bounded IOException retry handling and avoids logging stale IO failures after later success or skip paths.Test coverage
dotnet build tracer/src/Datadog.Trace.Coverage.collector/Datadog.Trace.Coverage.collector.csprojdotnet test tracer/test/Datadog.Trace.Tools.Runner.Tests/Datadog.Trace.Tools.Runner.Tests.csproj -f net10.0 --filter "FullyQualifiedName~CoverageResolverTests"DOTNET_ROOT=/usr/local/share/dotnet/x64 /usr/local/share/dotnet/x64/dotnet test tracer/test/Datadog.Trace.Tools.Runner.Tests/Datadog.Trace.Tools.Runner.Tests.csproj -f net8.0 --filter "FullyQualifiedName~CoverageResolverTests"git diff --check master...HEADOther details
The Windows-only exclusive-handle assertions are included in
CoverageResolverTestsand are skipped on non-Windows platforms.