You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Capture Windows loader ownership and thread CPU times - #7603
The Windows reflection session still hangs after #7594. In the fresh capture analyzed by run 41888472009, thread 36 waits in KERNELBASE!GetConsoleLangId during DLL initialization; threads 37–38 wait in LdrpDrainWorkQueue, as do three exiting executor threads. Existing !locks output only says it scanned 93 critical sections, leaving the loader ownership unclear.
Record verbose critical sections, explicit loader-lock ownership, Windows module versions, and per-thread user/kernel/elapsed times. These help distinguish spinning starters from blocked loader initialization. Remove the hard-coded native Thread +0x110 read: its values disagree with clrthreads in this capture. Continue correlating target addresses with clrthreads instead.
Investigates #7554. This diagnostic change does not close the issue or claim to fix the hang. No executor behavior or timeout is changed. This PR also refreshes the four stale STAThreadTests generator snapshots after #7594 reduced Repeat(100) to Repeat(10) and shifted source locations five lines. Their accidental UTF-8 BOMs are removed to match the existing snapshot format.
Validation: actionlint 1.7.7, PowerShell parser validation of the edited native-analysis script, and git diff --check passed. CDB execution requires the hosted Windows diagnostic run. No engine tests ran locally.
Debugger commands follow Microsoft critical-section guidance and !runaway documentation. CPU times may be unavailable if the captured dump lacks thread timing data; the remaining commands still run and the raw debugger output is retained.
Summary by CodeRabbit
Diagnostics
Windows dump analysis now includes additional system module versions, loader-lock ownership and details, and per-thread CPU usage alongside existing runtime and thread-stack information.
Starter-target state collection now records four raw 32-bit values at each target address and no longer reads a 64-bit value at offset 0x110.
Windows dump reports provide additional details about system modules, thread activity, and target state.
Snapshot review fix validation (afbb5e4d14): removed only the three leading BOM bytes from each STA snapshot. The focused net10.0 STAThreadTests generator snapshot test passed (1/1), demonstrating that the generated text still matches; git diff --check passed. Other target frameworks remain for CI.
No actionable comments were generated in the recent review.
Walkthrough
The AOT workflow expands Windows dump diagnostics and changes starter-target state reads. Four STAThread generated-test snapshots now use repeat counts of 10 and updated source line metadata. The .NET Framework 4.7 snapshot also removes its UTF-8 BOM.
Changes: Windows dump diagnostics
Layer / File(s)
Summary
Expand Windows dump and starter-state diagnostics .github/workflows/aot.yml
The native debugger command adds ntdll and kernelbase module details, loader-lock inspection, verbose lock information, and per-thread CPU usage. Starter-state collection reads four raw 32-bit words at each target address and removes the 64-bit read at offset 0x110.
STAThread test snapshots
Layer / File(s)
Summary
Update generated STAThread test metadata tests/TUnit.Core.SourceGenerator.Tests/STAThreadTests.Test.*.verified.txt
The .NET 8, .NET 9, .NET 10, and .NET Framework 4.7 snapshots change repeat counts from 100 to 10 and update source line numbers. The .NET Framework 4.7 snapshot removes its UTF-8 BOM.
thomhurst/TUnit#7584: Adds the AOT workflow dump-analysis job that this PR updates.
thomhurst/TUnit#7565: Captures the Windows reflection-mode dump analyzed by this workflow.
thomhurst/TUnit#7594: Reduces STAThread test repetitions and is reflected in the updated snapshots.
Merge Risk: Minimal · up to afbb5
No actionable issue is established at the reviewed head. The diagnostic workflow can proceed through normal validation.
Pre-merge checks: 4
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title "Capture Windows loader ownership and thread CPU times" matches the main change. The PR adds Windows diagnostic output for loader-lock ownership, verbose critical sections, module versions, …
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Title check
Explanation
The title "Capture Windows loader ownership and thread CPU times" matches the main change. The PR adds Windows diagnostic output for loader-lock ownership, verbose critical sections, module versions, and per-thread user, kernel, and elapsed times. The title does not mention the STAThreadTests snapshot refresh, but that is a secondary change and the title does not need to cover every detail.
Finishing Touches
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks the dump at night, New module clues come into sight. Four little words replace the read, Ten test repeats now meet the need. The snapshots shift their lines in place, Then hop away at a gentler pace.
Recent review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: e82f4438-01d2-453e-aadc-419dc54aaa62
Commits
Reviewing files that changed from the base of the PR and between 8db6c98 and afbb5e4.
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 2 remain after this review.
Diagnostic-only change to the CDB command line in aot.yml. It adds lmvm ntdll/kernelbase, !runaway 7, !critsec ntdll!LdrpLoaderLock and !locks -v, and drops the hard-coded +0x110 Thread read. I found no correctness problems, and the change doesn't touch executor behaviour. Removing the runtime-specific offset is the right call, since it was giving values that disagreed with clrthreads.
Things to check when the Windows run executes:
Symbol names are unverified.dt ntdll!_PEB @$peb LoaderLock and !critsec ntdll!LdrpLoaderLock rely on symbols that may be missing from public ntdll PDBs. If a symbol is missing, CDB prints an error and carries on, and the $LASTEXITCODE check will not catch it. Look for the actual output in native-stacks.txt rather than assuming the run succeeded. !locks -v is the fallback, because it should show the loader lock owner anyway.
Output size.!locks -v across about 93 critical sections, plus ~* kn, can make a large artifact. It is already uploaded raw, so this is fine as long as the artifact size limits hold.
Maintainability. The long -c string is getting hard to read. It would be cleaner to build it from a PowerShell array joined with ; , as the $stateCommands block below it already does. Each command could then carry a short comment. This is optional.
Assigned #7569: the earlier wait for #7603 AOT run 41900939879 is cleared. That run completed successfully, including Windows engine tests. Released pr-7569 and acquired pr-7603 to work the blocker first.
Re-read all PR conversation comments, REST reviews and inline comments, and GraphQL review threads. Claude reports no blocking issues; Codex explicitly records completed/no findings for current head afbb5e4. CodeRabbit and Greptile have no actionable findings. No review threads exist. The snapshot BOM finding is fixed and confirmed by the later review; optional debugger formatting remains a nonblocking suggestion. Assert-PrGreen.ps1 -ForAutoMerge passes, and squash auto-merge is already armed on this unchanged head.
Remaining CI: modularpipeline (macos-latest) in run 41900939897 and Apple smoke tests in run 41900939938 are in progress, with no failed checks. This is a wait for newer CI, not a deferral on the completed AOT run. No unchanged-head rerun, on-hold label or local engine-test execution was used.
Resolve routes checked: #7194 remains open and #7569 retains Closes #7194; neither main nor #7603 replaces that generator cleanup. #7554 remains open; a passing intermittent engine run does not prove its hang fixed, and this diagnostic PR does not claim to close it. No finished-parent, stale acceptance criteria, superseded PR, reversible decision, human, upstream or credentials route removes the remaining CI gate. No code changes were necessary. All held locks are released at unit completion.
Review: Diagnostic-only change to the Windows CDB dump analysis in aot.yml. No executor/test behavior is touched, which matches the PR description.
Adding lmvm ntdll/kernelbase, !runaway 7, the loader-lock dt and !critsec, and !locks -v is reasonable for the loader-lock investigation.
Dropping the hard-coded +0x110 read is the right call, since that offset is runtime-version specific.
One risk: cdb -c commands run in sequence, and dt ntdll!_PEB @$peb LoaderLock or !critsec ntdll!LdrpLoaderLock can fail without ntdll private symbols (they are generally available from the symbol server). A failed command only prints an error and the script continues, so this is non-fatal. It would still be worth confirming on the first hosted run that native-stacks.txt contains the output. If it does not, consider moving the new commands after ~* kn in a separate cdb invocation, so a failure can't hide the existing data.
The output is redirected to the same file, so a very large !locks -v could bloat the artifact. That is acceptable for a diagnostic.
The earlier wait for #7603 run 41895687235 is over: that run failed its Windows engine job with ExpectedStateTests.Pass producing no TRX after the eight-minute dump deadline. The .NET Ubuntu and Windows jobs independently failed STAThreadTests generator snapshots introduced by #7594.
Pushed 8db6c98 to fix that deterministic failure. Reused the four reviewed snapshot corrections already validated on #7593: Repeat(100) becomes Repeat(10), and source locations advance five lines. Reviewed the differences against the fixture. Targeted net10.0 STAThreadTests snapshot test passed (1/1); git diff --check passed. Other framework snapshots require CI. No engine tests ran locally. Existing restore/analyzer warnings are tracked by #7412/#7410.
Read reviews, inline comments, GraphQL threads and conversation comments: previous-head Claude and Codex verdicts were positive, with no threads or blocking findings. Disabled auto-merge before pushing; new-head automatic reviews and CI must complete before re-arming. GitHub head now matches the pushed commit.
Resolve routes: #7593 remains open and its dependency update is not superseded or already on main. #7603 remains an independent diagnostic change, not a fix closing #7554. No finished-parent, stale acceptance criteria, owner decision, upstream release or credentials gate applies. Fixed the actionable snapshot failure rather than repeating the old deferral or rerunning an undiagnosed hang. The Windows hang still needs investigation under #7554; a passing intermittent run alone cannot establish a fix. This unit reports pushed fixes to #7603, not a deferral or a wait on the completed run.
Workflow change (aot.yml) looks good. The extra CDB commands are sensible for the loader-lock investigation: lmvm ntdll/kernelbase, !runaway 7, dt ntdll!_PEB @$peb LoaderLock, !critsec ntdll!LdrpLoaderLock and !locks -v. Dropping the hard-coded +0x110 read is the right call, since that offset is runtime-specific. The script stays diagnostic-only and keeps the $LASTEXITCODE check. One caveat: if any of the new commands fails to resolve (for example LdrpLoaderLock without private symbols), CDB prints an error and carries on, so the run still succeeds. That is fine for diagnostics.
Concern: unrelated snapshot changes. The PR is described as a diagnostic-only change with no test changes. It also rewrites the four STAThreadTests.Test.*.verified.txt files:
Repeat(100) becomes Repeat(10) and the line numbers shift. tests/TUnit.TestProject/STAThreadTests.cs already has [Repeat(10)] on main, so this looks like a legitimate refresh of stale snapshots. It should still be called out in the description, because it is unrelated to the debugger work.
A UTF-8 BOM (EF BB BF) is added before // <auto-generated/> on line 1. Other verified files such as BasicTests.Test.DotNet10_0.verified.txt have no BOM. This looks like an accidental artifact of how the snapshots were regenerated, for example a Windows editor or PowerShell redirect. Please strip the BOM so these files match the other snapshots and don't churn later. Also check the generator output itself, because the snapshot test compares text and might not tolerate it.
Suggestion: move the snapshot refresh into its own small PR, or at least say in the description that it is a stale-snapshot fix. That keeps this diagnostics PR easy to revert or bisect.
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
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.
GitHub Actions
Error
The Windows reflection session still hangs after #7594. In the fresh capture analyzed by run 41888472009, thread 36 waits in KERNELBASE!GetConsoleLangId during DLL initialization; threads 37–38 wait in LdrpDrainWorkQueue, as do three exiting executor threads. Existing !locks output only says it scanned 93 critical sections, leaving the loader ownership unclear.
Record verbose critical sections, explicit loader-lock ownership, Windows module versions, and per-thread user/kernel/elapsed times. These help distinguish spinning starters from blocked loader initialization. Remove the hard-coded native Thread +0x110 read: its values disagree with clrthreads in this capture. Continue correlating target addresses with clrthreads instead.
Investigates #7554. This diagnostic change does not close the issue or claim to fix the hang. No executor behavior or timeout is changed. This PR also refreshes the four stale STAThreadTests generator snapshots after #7594 reduced Repeat(100) to Repeat(10) and shifted source locations five lines. Their accidental UTF-8 BOMs are removed to match the existing snapshot format.
Validation: actionlint 1.7.7, PowerShell parser validation of the edited native-analysis script, and git diff --check passed. CDB execution requires the hosted Windows diagnostic run. No engine tests ran locally.
Debugger commands follow Microsoft critical-section guidance and !runaway documentation. CPU times may be unavailable if the captured dump lacks thread timing data; the remaining commands still run and the raw debugger output is retained.
Summary by CodeRabbit
0x110.Snapshot review fix validation (
afbb5e4d14): removed only the three leading BOM bytes from each STA snapshot. The focused net10.0 STAThreadTests generator snapshot test passed (1/1), demonstrating that the generated text still matches; git diff --check passed. Other target frameworks remain for CI.