Handle dotnet/runtime#127957 in integration tests#8665
Merged
Conversation
NachoEchevarria
changed the title
BenchmarksBenchmark execution time: 2026-05-29 15:00:50 Comparing candidate commit 0abfb46 in PR branch Some scenarios are present only in baseline or only in candidate runs. If you didn't create or remove some scenarios in your branch, this maybe a sign of crashed benchmarks 💥💥💥 Scenarios present only in baseline:
Found 8 performance improvements and 11 performance regressions! Performance is the same for 44 metrics, 9 unstable metrics, 90 known flaky benchmarks, 36 flaky benchmarks without significant changes.
|
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8665) 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 (8665) - mean (73ms) : 71, 76
master - mean (75ms) : 70, 79
section Bailout
This PR (8665) - mean (77ms) : 75, 80
master - mean (77ms) : 75, 79
section CallTarget+Inlining+NGEN
This PR (8665) - mean (1,110ms) : 1060, 1159
master - mean (1,111ms) : 1045, 1177
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 (8665) - mean (114ms) : 110, 118
master - mean (115ms) : 109, 120
section Bailout
This PR (8665) - mean (119ms) : 114, 124
master - mean (116ms) : 113, 119
section CallTarget+Inlining+NGEN
This PR (8665) - mean (794ms) : 776, 812
master - mean (799ms) : 766, 832
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8665) - mean (102ms) : 98, 105
master - mean (102ms) : 98, 106
section Bailout
This PR (8665) - mean (106ms) : 101, 111
master - mean (103ms) : 100, 107
section CallTarget+Inlining+NGEN
This PR (8665) - mean (960ms) : 925, 995
master - mean (955ms) : 919, 991
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8665) - mean (99ms) : 96, 103
master - mean (104ms) : 99, 108
section Bailout
This PR (8665) - mean (101ms) : 99, 103
master - mean (101ms) : 98, 105
section CallTarget+Inlining+NGEN
This PR (8665) - mean (828ms) : 793, 863
master - mean (827ms) : 792, 862
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 (8665) - mean (198ms) : 192, 204
master - mean (199ms) : 195, 204
section Bailout
This PR (8665) - mean (202ms) : 198, 206
master - mean (202ms) : 197, 207
section CallTarget+Inlining+NGEN
This PR (8665) - mean (1,199ms) : 1155, 1244
master - mean (1,202ms) : 1159, 1246
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 (8665) - mean (287ms) : 280, 294
master - mean (287ms) : 280, 294
section Bailout
This PR (8665) - mean (289ms) : 283, 295
master - mean (288ms) : 282, 294
section CallTarget+Inlining+NGEN
This PR (8665) - mean (968ms) : 950, 985
master - mean (966ms) : 950, 983
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8665) - mean (279ms) : 274, 284
master - mean (278ms) : 270, 285
section Bailout
This PR (8665) - mean (277ms) : 271, 283
master - mean (278ms) : 273, 283
section CallTarget+Inlining+NGEN
This PR (8665) - mean (1,157ms) : 1127, 1188
master - mean (1,157ms) : 1120, 1193
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8665) - mean (277ms) : 272, 281
master - mean (277ms) : 271, 283
section Bailout
This PR (8665) - mean (277ms) : 272, 282
master - mean (278ms) : 273, 283
section CallTarget+Inlining+NGEN
This PR (8665) - mean (1,040ms) : 998, 1082
master - mean (1,041ms) : 999, 1083
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
NachoEchevarria
changed the title
NachoEchevarria
changed the title
NachoEchevarria
changed the title
NachoEchevarria
marked this pull request as ready for review
May 22, 2026 08:26
andrewlock
approved these changes
May 29, 2026
andrewlock
left a comment
andrewlock
left a comment
Member
There was a problem hiding this comment.
Nice, thanks! Looks good to me as a generic way to handle these types of unavoidable errors, and we can remove them (or constrain them to only earlier runtimes) in the future once the fix makes it into the runtime 🎉
| // Windows — CLR FailFast with HRESULT 0x80131506 (COR_E_EXECUTIONENGINE) on the | ||
| // threadpool gate thread. Exit code is unreliable here (we've seen 0 in CI even though | ||
| // the runtime died), so we don't gate on it. | ||
| if (standardError.Contains("Fatal error. Internal CLR error. (0x80131506)") |
Member
There was a problem hiding this comment.
Hmmm, this feels quite broad, but let's see how it goes
|
|
||
| public static bool IsRuntime127957Race(int exitCode, string standardError) | ||
| { | ||
| if (standardError is null) |
Member
There was a problem hiding this comment.
Should we preemptively check if we're running against a "known good" value. e.g. this is fixed in .NET 11, and will be in .NET 8.0.x (we don't know the x yet!). Should we bailout of this check if we know that the race shouldn't exist?
Collaborator
Author
There was a problem hiding this comment.
Yes, I think that we should do it when the patch is released and out ci have it. Still, we will need this code to handle < net 8 errors
Member
There was a problem hiding this comment.
Does ASM use this fixture too, or do we need something similar there as well?
Collaborator
Author
There was a problem hiding this comment.
ASM relies on this class ultimatelly, so we are covered
| @@ -168,54 +168,38 @@ public async Task<Process> StartSample(MockTracerAgent agent, string arguments, | |||
|
|
|||
| public async Task<ProcessResult> RunSampleAndWaitForExit(MockTracerAgent agent, string arguments = null, string packageVersion = "", string framework = "", int aspNetCorePort = 5000, bool usePublishWithRID = false, string dotnetRuntimeArgs = null) | |||
Member
There was a problem hiding this comment.
Is this method called by all of the different "Run sample" methods in here? I think it probably is, but I know CI Visibility uses different methods etc, so not sure if this will catch those too?
Collaborator
Author
There was a problem hiding this comment.
You are right, we need to cover StartDotnetTestSample for ci vis too, thanks!
NachoEchevarria
added a commit
that referenced
this pull request
Jul 2, 2026
…nt (#8729) ## Summary of changes Add a fourth `dotnet/runtime#127957` fingerprint to `ErrorHelpers.IsRuntime127957Race`: Linux/SIGABRT (exit 134) + `System.BadImageFormatException` + "The metadata is corrupt". ## Reason for change Follow-up to #8665. We hit a new manifestation of the same metadata-race in CI on arm64/net9.0 — the test failed with "Unable to determine port application is listening on" because the sample aborted at startup: ``` [webserver][stderr] Unhandled exception. System.BadImageFormatException: The metadata is corrupt. at ...PhysicalFilesWatcher.GetOrAddFilePathChangeToken(String) ... at Microsoft.Extensions.Hosting.HostBuilder.Build() at Samples.Security.AspNetCore5.Program.Main(String[]) ``` Failing build log: https://dev.azure.com/datadoghq/a51c4863-3eb4-4c5d-878a-58b41a049e4e/_apis/build/builds/202607/logs/6762 ## Implementation details The existing three fingerprints (#8665) surface the race as downstream `TypeLoadException` / `MissingMethodException` / Windows FailFast symptoms. This one is the metadata reader throwing the corruption directly. Crash-dump analysis confirms the fingerprint: - `BadImageFormatException`, HRESULT `0x8007000b` (`COR_E_BADIMAGEFORMAT`) - Throw originates inside the JIT/metadata reader while compiling `GetOrAddFilePathChangeToken` during the first `HostBuilder.Build()` — earliest startup, peak instrumentation window. - Datadog native profiler/tracer threads are attached and active in the process. - Transient (passes on retry), on a nightly arm64 build — the exact profile of the issue. Like the others, "The metadata is corrupt" is unreachable through normal product/user code, so the gate stays narrow. The retry/metrics harness and `AspNetCoreTestFixture.TryStartApp` `port == null` path are unchanged — only the detection is extended. ## Test coverage Existing suite. Retry/fail only fires on the narrow fingerprint. ## Other details Revisit when dotnet/runtime#127957 is fixed upstream.
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
3 participants
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
Retry sample launches that crash with the dotnet/runtime#127957 fingerprint (a race in
MDInternalRWbetween unlocked metadata readers and the profiler emitting tokens). Up to 2 retries; if the fingerprint persists across all 3 attempts, throw a clear exception to fail the test — at that point it's not a flake.Reason for change
Integration tests intermittently crash during runtime startup with one of three platform-specific shapes — all documented manifestations of the issue:
TypeLoadException: [Undefined resource string ID:0x...]fromPortableThreadPool.GateThreadMissingMethodException: Method not found: 'Boolean ConcurrentDictionary'2.TryGetValue(...)'duringHostBuilder.Build()Fatal error. Internal CLR error. (0x80131506)(COR_E_EXECUTIONENGINE FailFast) onPortableThreadPool.GateThread. The Windows variant is more annoying — the host can report exit code 0 even though the runtime died, and the sample can emit partial output before dying, so downstream test assertions fail with misleading messages.All three fingerprints are structurally unique to corrupted-metadata throws — unreachable through normal product or user code.
Implementation details
ErrorHelpers.HandleRuntimeSkippableErrorsAsync— central decision helper. Returnstrueto retry,falseto proceed normally, throws when the fingerprint persists past the retry budget (exit code can't be trusted to surface the failure on Windows). Wired into the three "run sample" entry points:TestHelper.RunSampleAndWaitForExit,TestHelper.RunDotnetTestSampleAndWaitForExit(CI Visibility / VSTest path), andAspNetCoreTestFixture.TryStartApp.runtime_issue:127957:dd_trace_dotnet.ci.tests.retried_due_to_runtime_metadata_race(recovered via retry)dd_trace_dotnet.ci.tests.persistent_runtime_metadata_race(failed after all retries)EnsureServerStartedhealth-check failure) — the Windows manifestation often binds Kestrel before the gate thread dies, so the crash surfaces during health-check rather than at port-bind. Both detection paths share aCheckRaceAndCleanupForRetryAsynchelper.createdumpdoesn't slip past detection.Test coverage
Existing suite. Retry/fail only fires on the narrow fingerprints, so other failures surface normally.
Other details
Revisit when dotnet/runtime#127957 is fixed upstream (currently open).