Skip to content

fix(utilities): make WpfDispatcherYield dispatcher lookup arrangeable (#508) - #521

Merged
drmoisan merged 6 commits into
mainfrom
bug/wpf-dispatcher-yield-test-order-dependent-508
Aug 8, 2026
Merged

fix(utilities): make WpfDispatcherYield dispatcher lookup arrangeable (#508)#521
drmoisan merged 6 commits into
mainfrom
bug/wpf-dispatcher-yield-test-order-dependent-508

Conversation

@drmoisan

@drmoisan drmoisan commented Aug 8, 2026

Copy link
Copy Markdown
Owner

Fix order-dependent WpfDispatcherYield test by making the dispatcher lookup arrangeable (#508)

Summary

  • Makes WpfDispatcherYieldTests.YieldAsync_WithoutDispatcher_RemainsStrict deterministic by giving WpfDispatcherYield an injectable-delegate seam, so the "no dispatcher" precondition is arranged by the test rather than inherited from ambient thread and process state.
  • Removes [ExcludeFromCodeCoverage] from WpfDispatcherYield: the class is now genuinely unit-testable, so the exemption is no longer defensible.
  • Expands the suite from 2 tests to 4, pinning all three dispatcher-resolution branches plus the pre-yield cancellation guard, and asserting resolution order rather than only outcome.
  • Public API surface is unchanged; neither existing call site was modified.
  • Repo-wide coverage moves 85.82% -> 85.83% line and 79.21% -> 79.23% branch. No regression.
  • Two production files changed: 1 source (+37/-4), 1 test (+164/-2). Everything else is evidence and planning documentation.

Why

The test asserted that YieldAsync throws InvalidOperationException when no WPF Dispatcher is available, but never arranged that precondition. The production resolution was:

Dispatcher dispatcher =
    Dispatcher.FromThread(Thread.CurrentThread) ?? UtilitiesCS.UiThread.Dispatcher;

Both operands are ambient state the test did not control:

  1. Dispatcher.FromThread(Thread.CurrentThread) is non-null on any pooled worker where an earlier test touched Dispatcher.CurrentDispatcher, which creates and caches a dispatcher for the calling thread on first access. At least nine classes in UtilitiesCS.Test do exactly that.
  2. UiThread.Dispatcher is process-global, set-once static state that stays non-null for the rest of the process once any test triggers UiThread.Initialize().

Under [assembly: Parallelize(Workers = 0, Scope = ExecutionScope.ClassLevel)] the thread this test lands on is undetermined, so the precondition silently evaporated and the assertion failed intermittently. This violated .claude/rules/general-unit-test.md Core Principles 1 (Independence) and 4 (Determinism).

An unreliable baseline is corrosive: it prevents anyone from distinguishing "my change broke a test" from "the suite is flaky," and it trains reviewers and agents to re-run until green.

What Changed

Core logic — UtilitiesCS/OutlookObjects/Folder/WpfDispatcherYield.cs (+37/-4)

  • Added two readonly Func<Dispatcher?> fields and an internal seam constructor.
  • Retained an explicit public WpfDispatcherYield() : this(null, null) { }. This is load-bearing: declaring any constructor removes C#'s implicit parameterless one, which would have broken both existing call sites.
  • Null seam arguments fall back to the verbatim prior expressions, so runtime behavior is unchanged.
  • The ?? stays in the same position with the same operand order inside YieldAsync, so short-circuiting is preserved and the tests still verify the ordering rather than replacing it. The default lambda evaluates Thread.CurrentThread when invoked, not at construction, so it still observes the calling thread.
  • Resolved local changed from Dispatcher to Dispatcher?; removed [ExcludeFromCodeCoverage] and the now-unused using System.Diagnostics.CodeAnalysis;.

The seam constructor is internal, reached from tests through the pre-existing [assembly: InternalsVisibleTo("UtilitiesCS.Test")] at UtilitiesCS/Properties/AssemblyInfo.cs:19 (present at the merge base; that file is not in this diff). The assembly's public API is unchanged in signature terms.

Tests — UtilitiesCS.Test/OutlookObjects/Folder/WpfDispatcherYieldTests.cs (+164/-2)

Test Arranged precondition Asserts
YieldAsync_CanceledToken_ThrowsBeforeDispatcherYield both lookups counting OperationCanceledException; neither lookup consulted
YieldAsync_ThreadAffinitizedDispatcherPresent_YieldsWithoutFallback thread lookup returns an owned dispatcher no throw; fallback not consulted
YieldAsync_ThreadDispatcherAbsent_FallsBackToProcessGlobalDispatcher thread lookup null, fallback owned no throw; each consulted exactly once
YieldAsync_WithoutDispatcher_RemainsStrict both lookups null InvalidOperationException

CountingDispatcherProvider records invocation counts so the tests pin resolution order, not just outcome. StaDispatcherHost owns a pumping STA thread (Dispatcher.Run()), sets IsBackground = true, and shuts down deterministically via BeginInvokeShutdown(DispatcherPriority.Send) + Join().

Note this is System.Windows.Threading.Dispatcher.Run(), not Application.Run — no Window, Form, or Control is constructed, so it cannot display a window during a headless run.

Documentation

The remaining files are the minor-audit feature folder for #508: issue.md (the sole acceptance-criteria source), the 3-phase atomic plan, three review artifacts, and 40 evidence artifacts.

Architecture / How It Fits Together

WpfDispatcherYield implements IDispatcherYield and is constructed once in production at TaskMaster/AppGlobals/AppOlObjects.FolderTreeService.cs:365, then injected into the folder-tree service so long traversals can yield to the UI. Resolution policy stays in the class; only the two lookups are externalized. Consumers see no change: they still call new WpfDispatcherYield() and get identical behavior.

Verification

Completed

Fail-before — a genuine failing run, not an assertion. With the pre-change code, marshalling the unchanged call onto a pumping STA thread produced:

Failed YieldAsync_WithoutDispatcher_RemainsStrict [235 ms]
  Expected a <System.InvalidOperationException> to be thrown, but no exception was thrown.

The test assembly was rebuilt before the probe (DLL mtime 16:18:36 -> 16:24:18), ruling out a stale-assembly false pass.

Repeated runs — a single green run does not prove an intermittent fix. Three full parallel UtilitiesCS.Test runs recorded as evidence, plus three further independent runs during merge verification:

Run EXIT Total Passed Failed
1 0 4667 4667 0
2 0 4667 4667 0
3 0 4667 4667 0
4-5 (independent) 0 4667 4667 0
6 (post-merge, after clean rebuild) 0 4688 4688 0

Identical counts every time; per-test durations varied, so scheduling genuinely differed while outcomes did not.

Full toolchain, single clean pass, no intervening file change:

Step Command EXIT Result
1 csharpier format . 0 1488 processed, 0 rewritten
2 msbuild TaskMaster.sln … /p:EnableNETAnalyzers=true /p:EnforceCodeStyleInBuild=true 0 6 warnings, 0 errors
3 msbuild TaskMaster.sln … /p:Nullable=enable /p:TreatWarningsAsErrors=true 0 5 warnings, 0 errors
4 Invoke-MSTestWithCoverage.ps1 0 6295 / 6295 / 0

Re-run after merging current main: csharpier check on both files exit 0; a full msbuild /t:Rebuild with analyzers exit 0; full suite 6397 / 6397 / 0.

CSharpier 1.3.0 requires the format / check subcommands — bare csharpier . is not a valid invocation in 1.x, and dotnet tool run csharpier is unavailable in this checkout. That is a CLI-surface difference from the string quoted in CLAUDE.md, not a deviation from the policy.

Coverage.

Scope Baseline Post-change
Repo-wide line 85.82% (95274/111021) 85.83% (95325/111059)
Repo-wide branch 79.21% 79.23%
WpfDispatcherYield excluded from measurement 96.43% line, 100% branch

The denominator grew by 38 lines because [ExcludeFromCodeCoverage] was removed; 45 newly covered lines more than offset it, so the exemption was retired honestly rather than by hiding the lines. One line remains uncovered: the default fallback lambda () => UtilitiesCS.UiThread.Dispatcher, reachable only via the parameterless constructor when the thread lookup returns null — arranging that through the parameterless constructor would reintroduce exactly the process-global ambient dependency this change removes.

Review. feature-review returned ready to merge, 0 blocking findings (0 FAIL, 0 blocking PARTIAL). AC1 through AC9 all PASS, each check-off independently re-verified. Six advisory items recorded; none holds the merge.

Recommended

pwsh -NoProfile -File scripts/vscode/Invoke-MSTestWithCoverage.ps1 -Configuration Debug

Backward Compatibility / Migration Notes

No breaking changes and no migration required. The public API surface is unchanged, the seam constructor is internal, and the defaults reproduce the prior expressions exactly. Both existing call sites — TaskMaster/AppGlobals/AppOlObjects.FolderTreeService.cs:365 and UtilitiesCS.Test/OutlookObjects/Folder/OutlookFolderTreeServiceConcurrencyTests.cs:55 — are absent from this diff and still bind to a public parameterless constructor.

Risks and Mitigations

Risk Mitigation
Seam defaults drift from production behavior Defaults are the verbatim prior expressions; resolution order stays inside YieldAsync and is asserted by two separate tests via invocation counts
StaDispatcherHost hangs or leaks a thread Real pumping dispatcher via Dispatcher.Run(), IsBackground = true, deterministic BeginInvokeShutdown + Join in Dispose. Advisory noted by review: the Join() is unbounded, so a shutdown failure would hang rather than fail; considered acceptable because shutdown is unconditional and the host is fully owned by the test
Removing [ExcludeFromCodeCoverage] drops repo-wide coverage Measured: coverage went up; both floors still met
The +0.0166 pp coverage delta is below measurement noise Review confirmed the non-regression verdict rests on the absolute figures clearing their floors by 0.83 pp and 4.23 pp, not on the delta's sign

Review Guide

  1. UtilitiesCS/OutlookObjects/Folder/WpfDispatcherYield.cs — the seam, about 35 lines.
  2. UtilitiesCS.Test/OutlookObjects/Folder/WpfDispatcherYieldTests.cs — four tests and two helpers.
  3. evidence/regression-testing/fail-before.*.md — proof the defect was real.
  4. evidence/qa-gates/repeat-run-*.md — proof it is gone.
  5. code-review.*.md, policy-audit.*.md, feature-audit.*.md — the audit verdicts.

The remaining ~50 files are evidence and planning artifacts containing no executable code.

Two notes so they are not misread as defects:

  • Coverage evidence is committed as compact package-level JaCoCo summaries rather than raw Cobertura, following the convention set by d0955dc4 for Bug: ribbon-engine-readiness-guard #503. The substitution was made before pushing, so roughly 20 MB and 378,000 lines never entered history. evidence/qa-gates/coverage-artifact-substitution.*.md records the projection and shows the derived counts reproduce the Cobertura root attributes exactly.
  • Every git gate in the plan is pathspec-scoped to '*.cs' '*.csproj' '*.sln', because .claude/agent-memory/** is tracked, already modified at branch head, and its prose contains tokens such as DoNotParallelize that would otherwise trip the prohibited-fix grep.

This branch merges current main (44440736). The only conflicts were three shared .claude/agent-memory index files, resolved by union with no entry dropped.

Follow-ups

GitHub Auto-close

drmoisan and others added 6 commits August 8, 2026 19:02
…#508)

`WpfDispatcherYieldTests.YieldAsync_WithoutDispatcher_RemainsStrict` was
order-dependent and failed intermittently under class-level parallelization.
Both operands of the production `??` were ambient state the test never
arranged: `Dispatcher.FromThread(Thread.CurrentThread)` is non-null on any
pooled worker where an earlier test touched `Dispatcher.CurrentDispatcher`,
and `UiThread.Dispatcher` is process-global set-once static state.

Add an injectable-delegate seam so the dispatcher-free precondition is
arranged rather than inherited. The resolution order stays inside
`YieldAsync`, so the tests still verify the ordering rather than replacing
it. The seam constructor is `internal` (reached via the existing
`InternalsVisibleTo("UtilitiesCS.Test")`) and the `public` parameterless
constructor is retained explicitly, so the public API surface is unchanged
and no call site needs modification.

Remove `[ExcludeFromCodeCoverage]`: the class is now genuinely unit-testable,
and `.claude/rules/general-unit-test.md` does not permit a coverage exemption
whose justification has been removed.

Tests pin all three resolution branches (thread dispatcher present, thread
dispatcher absent with fallback present, both absent) plus the pre-yield
cancellation guard, using counting providers to assert resolution order and
an owned pumping STA dispatcher thread that the test shuts down.

Fail-before evidence: with the pre-change code, marshalling the unchanged
call onto a pumping STA thread makes the assertion fail with "Expected a
<System.InvalidOperationException> to be thrown, but no exception was thrown".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011vjbBgWNjMZLxBexMQMwLd
Adds the minor-audit feature folder for issue #508: the populated issue.md
(sole acceptance-criteria source, AC1-AC9 all delivered), the 3-phase atomic
plan at revision 1.2, and 40 evidence artifacts under evidence/.

Notable evidence:
- regression-testing/fail-before.*.md - genuine failing run before the fix,
  with a rebuild-timestamp proof that rules out a stale-assembly false pass.
- regression-testing/preexisting-failure-attribution.*.md - controlled
  four-run experiment showing the two QuickFiler.Test pump-host failures are
  pre-existing at merge-base 003c571 and not caused by this change.
- qa-gates/repeat-run-{1,2,3}.*.md - three full parallel UtilitiesCS.Test
  runs, 4667/4667 each, demonstrating the flake is gone.
- qa-gates/coverage-changed-lines.*.md - aggregated changed-class coverage
  across the async state machine and lambda display classes.

Coverage evidence is committed as compact package-level JaCoCo summaries
rather than raw Cobertura, per the convention established by d0955dc for
issue #503. The substitution was made before pushing, so the ~20 MB of raw
reports never enters history; qa-gates/coverage-artifact-substitution.*.md
records the projection and shows the derived counts reproduce the Cobertura
root attributes exactly.

Also records agent-memory entries for three traps hit during this work:
tracked .claude/agent-memory breaking unscoped git gates, an agent worktree
root making a "\.claude\" path exclusion unsatisfiable, and async state
machines splitting coverage across Cobertura class elements.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011vjbBgWNjMZLxBexMQMwLd
Adds the three reduced-audit artifacts produced by feature-review:

- policy-audit.2026-08-08T17-45.md - all policy gates PASS, with an
  explicit coverage-scoped PASS verdict for CSharp (repo-wide 85.83% line
  against an 85% floor, 79.23% branch against a 75% floor).
- code-review.2026-08-08T17-45.md - Approved. Confirms the seam addresses
  the root cause for BOTH ambient operands, preserves the public API and
  runtime behavior exactly, and adds no prohibited construct.
- feature-audit.2026-08-08T17-45.md - AC1 through AC9 all PASS; all nine
  check-offs verified as earned and retained.

Blocking findings: 0 (0 FAIL, 0 blocking PARTIAL). Six advisory items are
recorded as follow-up candidates; none holds the merge.

AC4 carries one merge-time obligation: its "justified in the PR body" clause
is unsatisfiable until the PR exists, so it is discharged by the PR body
rather than by the branch content.

Also mirrors the issue-update posted to existing issue #511 documenting the
pre-existing QuickFiler pump-host flake found while delivering this fix. A
comment was added to #511 rather than opening a duplicate issue, because
#511 already tracks that defect and explicitly asked for the failure capture
this work produced.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011vjbBgWNjMZLxBexMQMwLd
- JaCoCo package-level summaries are valid coverage evidence (the hook
  parses JaCoCo counters and cannot read Cobertura), so a jacoco.xml in a
  feature folder is not a missing-evidence gap.
- Corrects the pr-context-misclassification note: the summary can report a
  stale head SHA and file both changed .cs files under docs with
  "Core logic changes: 0 files", which would make the coverage hook skip
  C# enforcement entirely. Regenerate and verify before trusting it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011vjbBgWNjMZLxBexMQMwLd
Found while verifying the #508 fix against a tree merged with current main.
`PrintTree_WritesIndentedTreeToConsole` failed once in a full instrumented
suite run (6397/6396/1) and passed on an immediate re-run of the same
assembly with no code change (4688/4688).

Root cause: the method name is defined twice, in two mirrored classes in
different namespaces (`UtilitiesCS.Test/OutlookObjects/DASLFilterParser_Tests.cs:95`
and `UtilitiesCS.Test/OutlookObjects/Filter DASL/DASLFilterParserTests.cs:95`),
and both bodies redirect the process-global `Console.Out`. Neither class is
`[DoNotParallelize]`, so under class-level parallelization one class's
restore detaches the other's StringWriter and its content assertion sees
empty output.

The hazard is assembly-wide, not specific to these two classes: 29 files in
`UtilitiesCS.Test` call `Console.SetOut` and most are not serialized.

Both DASL files predate this work (present at merge-base 003c571) and
neither is in the #508 diff, so the defect is pre-existing and out of scope
for #508. This is the fourth distinct nondeterminism defect in this test
assembly family, after #508, #511, and #516.

Refs: #520

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011vjbBgWNjMZLxBexMQMwLd
- completion-gate-receipt-shapes: the exact field shapes the MCP
  require_complete gate demands (delegation_receipts as a LIST with eight
  keys, skill_receipts needing `required: true`, MCP receipts under
  `mcp_call_receipts` with `ok: true` + `evidence`). Guessing these cost
  three validate cycles; the authoritative source is
  orchestrator-state-routing.ts in drm-copilot.
- jacoco-not-cobertura-for-evidence: convert raw Cobertura to package-level
  JaCoCo before pushing, per the maintainer's d0955dc decision, and derive
  artifacts/csharp/coverage.xml from it only once the figure clears the 85%
  floor the coverage hook enforces.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011vjbBgWNjMZLxBexMQMwLd
@drmoisan
drmoisan force-pushed the bug/wpf-dispatcher-yield-test-order-dependent-508 branch from e0cc646 to b465646 Compare August 8, 2026 23:05
@drmoisan
drmoisan merged commit f910ff2 into main Aug 8, 2026
2 checks passed
@drmoisan
drmoisan deleted the bug/wpf-dispatcher-yield-test-order-dependent-508 branch August 10, 2026 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: wpf-dispatcher-yield-test-order-dependent

1 participant