Repository navigation
A WPF test hang now names its step and says whether the stranded body returned (#6632) - #6636
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| _ = finished.ContinueWith( | ||
| _ => Interlocked.Exchange(ref _stuckReturnedAfterMs, Math.Max(1, Environment.TickCount64 - began)), |
There was a problem hiding this comment.
Non-blocking nit on precision: _stuckReturnedAfterMs is stamped when finished (the InvokeAsync task) completes, which is the whole Job — body and drain (CloseLeftoverWindows + Dispatcher.Yield(ApplicationIdle)) — not the instant the body itself returns. The field's own doc comment at line 175-176 ("how long after it started the body ... did return") and the PR description ("records whether that body returns later, and how long after it started") both describe it as a body-return timestamp.
In the scenario this PR is chasing — a cold first WPF start under load — the drain right after a cold dispatcher's first-ever operations could itself take non-trivial time, so the reported "N ms" slightly overstates how long the body alone took. More importantly, if the drain itself ever got stuck (not the body), FailIfStuck() would keep reporting "has not returned yet" even though the body genuinely did return — the two are conflated.
The new test's own comment at line 147 ("The record is written when the host's job finishes, just after its drain") shows this is understood, just not surfaced in the public-facing doc/PR text. Might be worth a one-line clarification in the field comment and/or the refusal message, since the whole point of this PR is precise wording for whoever reads the next CI hang report.
There was a problem hiding this comment.
Verdict: LGTM
Claims
- VERIFIED — "All three files are test code. Nothing in
PerformanceMonitor.Uior any product changes." —gh pr diff --name-onlylists onlyDarling/Darling.Tests/{StaTestThread.cs,StaTestThreadTests.cs,StatementScrubViewerPlanWithheldTests.cs}. - VERIFIED — "the host's drain ... now names its own step" —
StaTestThread.cs:304-306,Step("the host's drain after the body: ...")added insideJobbeforeCloseLeftoverWindows(). - VERIFIED — "the host records whether that body returns later, and how long after it started" and the two refusal sentences —
StaTestThread.cs:175-177,255-266,328-339(_stuckReturnedAfterMs, theContinueWith,FailIfStuck's two message branches), matched exactly byStaTestThreadTests.cs:143,150("has not returned yet (#6632)", regexThat body did return, \d+ ms after it started). - VERIFIED — "No test asserted the old words [the removed 'stuck inside a body that never returned' wording]" — grepped the whole repo for that phrase and
STA test thread/WPF test body did not return; the only hits are the source file itself and no.yml/.mddepends on the old wording. - VERIFIED — "it compiles
StaTestThread.csthrough a link" (Lite.Tests) —Lite.Tests/Lite.Tests.csproj:69,<Compile Include="..\Darling\Darling.Tests\StaTestThread.cs" Link="StaTestThread.cs" />. This is also why no separate Lite.Tests change was needed for parity: the same file ships to both suites. - UNVERIFIED — the RED/GREEN result for the new test, the build/warning counts, and the
Darling.Tests/Lite.Testsrun totals ("Total 520, Failed 0" etc.) — this tree is Linux and read-only; the suites are WPF (Windows-only) and nothing here can build or run them. - UNVERIFIED — the CI forensics (job ids, runner image,
ProcessThread.WaitReasonsamples, CPU-ms figures) — these are claims about external CI logs, not about this diff's code.
Riskiest lines
Darling/Darling.Tests/StaTestThread.cs:264-266— the continuation that stamps_stuckReturnedAfterMsfires on completion of the wholeJob(body + drain), not on the body's own return, so the reported duration (and, in a drain-hangs scenario, the returned/not-returned verdict itself) folds in the drain — see inline comment. Covered byALaterRefusal_SaysWhetherTheStrandedBodyReturned_AndHowLongAfterItStarted, but that test's own in-code comment ("just after its drain") shows it's asserting the as-built, drain-inclusive behavior rather than a stricter body-only one — not a gap in the test, just a looser contract than the field's doc comment implies.Darling/Darling.Tests/StaTestThread.cs:333-338—FailIfStuck's sentinel logic (returnedAfter > 0vs. "not returned yet") is the only thing standing between a correct slow-vs-stuck verdict and a misleading one once the thread is permanently stranded. Covered by the same new test (asserts both branches' exact wording).Darling/Darling.Tests/StaTestThreadTests.cs:148— theSpinWait.SpinUntilpoll for the async stamp to appear is the one place a flaky false-negative could sneak in (CI trap: "Our wrong test is a bug we introduced, never a flake"). It polls up to 10s with no fixed sleep, which is the right shape; not a finding.
Traps near this diff
- "Lite and Darling ship in parity ... a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding" — does not apply here:
StaTestThread.csis the one file both suites compile (via theLink=include inLite.Tests.csproj:69), so this change reaches Lite.Tests automatically with no counterpart edit needed. - "Ruling (#5833): tests are load-bearing ... never answer a trap with a test that reads source text or asserts how code is spelled" — does not apply:
ALaterRefusal_SaysWhetherTheStrandedBodyReturned_AndHowLongAfterItStarted(StaTestThreadTests.cs:129-156) is a real behaviour test — it blocks a real body on aManualResetEventSlim, lets the real hang guard fire, releases it, and asserts on the live refusal text produced by that run, not on the source ofStaTestThread.cs. - "Our wrong test is a bug we introduced, never a flake" — does not apply: no new test here is failing; it's a new passing test plus pure diagnostic instrumentation (
Step(...)calls) added to an existing test, with no logic change to that test's assertions (StatementScrubViewerPlanWithheldTests.cs).
Divergence (blind read vs PR body)
The PR body describes the stranded-body record as tracking "whether that body returns later, and how long after it started" — phrased as a body-return timestamp. The implementation actually timestamps completion of the body's dispatcher operation as a whole, which includes the post-body drain (CloseLeftoverWindows + Dispatcher.Yield(ApplicationIdle)); the new test's own in-code comment acknowledges this ("The record is written when the host's job finishes, just after its drain") but the PR body does not mention it. Everything else in the body matches the diff on inspection; this is the only point where the author's description is less precise than the code.
Reviewer checklist
- Build and run
Darling.Tests/Lite.Testson a Windows WPF-capable machine to confirm the stated RED→GREEN result for the new test and the reported pass counts (520/0, 49/0), since this tree cannot build or execute WPF tests.
|
Follow-up pass (bacf84e..4774017): the only content in this range is the merge commit |
There was a problem hiding this comment.
Verdict: LGTM
Claims
No new factual claims to grade: this range (bacf84e..4774017) contains only the merge commit 477401708 ("Merge branch 'dev' into fix/6632-wpf-plan-withheld-hang") and the dev-branch commit 0a6121e1c it brings in. There is no PR-body text or commit message in this range making a claim about the diff's behavior.
Riskiest lines
No lines to assess — the only diff content (Darling/tools/provision-roles.sql) is base-branch (dev) content merged in via 0a6121e1c, not an author change on this PR, and no commit in this range interacts with it.
Traps near this diff
No traps apply — there is no author change in this range to check against the traps file.
Divergence (blind read vs PR body)
None — no new claims in this range to compare against the diff.
Reviewer checklist
- None beyond the prior pass's checklist (build/run
Darling.Tests/Lite.Testson Windows), since this range adds no new code to the WPF fix.
Fixes #6632.
Claude posting for Erik Darling.
This is the brief's fallback outcome. The wait is not named and the cause is not fixed. I could not name the call the STA thread blocks in from the code and the logs. This PR adds step names to the test body and a stranded-body record to the host, so the next CI hang says which call it stopped in, and whether it was slow or blocked for good. Keep #6632 open until that report comes in.
Why
StatementScrubViewerPlanWithheldTests.APlanWithheldWhole_ShowsTheWithheldSentenceInTheViewer_NotAParseErrorhit the 60 s hang guard in CI. The #6216 report said "the body named no step", one dispatcher operation still running, and the thread inWait (Executive).What the logs show:
DARLING_SCOPE: guard,-trait Stage=Guard, 4461 tests in 138-148 s): job 120412158679 (--enable-web and --enable-mcp print the address and the claude mcp add command (#6474) #6606) and job 120380656499 (The Darling installer waits until the service is ready and prints next steps (#6470) #6590). Both ran on runner image windows-2025-vs2026 20261021.278.Darling PG tests (4)) did not hang. Its only failure wasQueryStoreGroupMembersTests.TheResolver_KeepsServerExact_AndQueryHashUnknown_WithoutTouchingAStore(Assert.Equal, expected 1, actual 0). The issue's job id for Move the alert-rule, compose and custom-view code to Storage (part 1 of #6390) #6611 points at a different failure.ViewerPlanCorrectionsDataStartTests. In both hangs the second class ran later and failed naming this test. So the body was doing WPF's one-time process startup:InitializeComponent, theme and system resources, the native WPF DLL loads, and the firstTextBox.What the code and the evidence rule out:
SemaphoreSlim, a static constructor's class-init lock, a GC wait, or an idle dispatcher. All of these are user-mode waits, whichProcessThread.WaitReasonreports asUserRequest, and a GC suspension shows asSuspended.Executiveis KWAIT_REASON 0 or 7: a wait inside the kernel, such as a synchronous I/O completion, a file object's lock, or a driver.Dispatcher.Yieldwould leave the thread idle inGetMessage(UserRequest) with no operation running. The report showed one operation running.LoadPlanon the marker returns before anyawaitorTask.Run.Clear()sets element properties only, andCleanup()unsubscribes one static event.Configure-WindowsDefender.ps1setsDisableRealtimeMonitoringandDisableIOAVProtection, and excludesC:\andD:\.The leading hypothesis is unproven: WPF's first start in the process is I/O-bound and runs past 60 s under the Guard pass's load. The 47 ms of CPU fits that. The host can't confirm it, because once the guard fires it marks the thread stranded for good and never learns whether the body would have returned.
What changes
All three files are test code. Nothing in
PerformanceMonitor.Uior any product changes.StatementScrubViewerPlanWithheldTests.cs: the body names four steps withStaTestThread.Step(...). They are the constructor (the process's first WPF use), the withheld path ofLoadPlan, the reads of the result, andCleanup(). The next hang report names the step it stopped in.StaTestThread.cs, the drain step: the host's drain runs inside the body's own dispatcher operation. It now names its own step, so a stuck drain is no longer reported under the body's last step.StaTestThread.cs, the stranded-body record: when the guard gives up on a body, the host records whether that body returns later, and how long after it started. Every later refusal says either "That body did return, N ms after it started, so it was slow, not blocked for good" or "That body has not returned yet". The refusal's head now reads "was stranded by a body that did not return within its hang guard", because "never returned" would be false for a slow body. No test asserted the old words. The 60 s guard and the permanent refusal stay as they were (Tests share WPF threads, Postgres roles and runner CPU by design: give each test its own, and guard it #5602, A Settings naming test can leave the shared WPF test thread stuck, failing every later WPF test #6216, Lite test ScreenReaderRowNamesTests hung for 60 seconds on a dev run #6333).The next Guard-stage hang will then show:
ViewerPlanCorrectionsDataStartTestsfailures that follow in the same pass, whether the body was slow or blocked.Test plan
Darling.Testsbuilds with 0 errors. Its 6 warnings are inDarlingMcpNamedTokenTests.csandChangeWatchDetectionLiveTests.cs, which are already on dev and untouched here.Lite.Testsbuilds with 0 warnings; it compilesStaTestThread.csthrough a link.StaTestThreadTests.ALaterRefusal_SaysWhetherTheStrandedBodyReturned_AndHowLongAfterItStarted.origin/dev'sStaTestThread.csplanted:Assert.Contains() Failure: Sub-string not found(Total 1, Failed 1).StaTestThreadTests,StatementScrubViewerPlanWithheldTests,StaThreadCensusTests,ViewerPlanCorrectionsDataStartTestsand every*Census*class: Total 520, Failed 0, Skipped 1.*StaTestThread*,*PlanViewer*and*Census*: Total 49, Failed 0.pre-push.sh: not run, per the brief. CI decides.What the coordinator should double-check
CHANGELOG entry
None: a test-only PR.
Considered, not added
createdumpships with the runtime), uploaded as a CI artifact, would give the stuck thread's native stack. It is larger work: an artifact step inbuild.ymland a dump that can run to hundreds of MB. It is worth doing only if the step names and the return record still leave the wait unnamed.