Skip to content

fix(threading): harden the captured-UI-context exit of IsCompleted and settle AC5 - #890

Merged
drmoisan merged 14 commits into
mainfrom
bug/uithread-iscompleted-branch2-residual-and-ac5-apartment-measurement-816
Sep 14, 2026
Merged

drmoisan merged 14 commits into
mainfrom
bug/uithread-iscompleted-branch2-residual-and-ac5-apartment-measurement-816

Conversation

@drmoisan

Copy link
Copy Markdown
Owner

Suggested title

fix(threading): harden the captured-UI-context exit of IsCompleted and settle issue #809's AC5

Summary

  • Hardens the _uiSyncContext exit of SynchronizationContextAwaiter.IsCompleted in UtilitiesCS/Threading/UiThread.cs so it returns true only when the captured context matches and a non-null captured UI dispatcher is reference-equal to the executing thread's dispatcher.
  • Adds two negative regression tests (recycled-id and null-dispatcher cases) and one positive twin proving the change does not alter the WPF-dispatcher-operation leg.
  • Settles the outstanding clause of issue Bug: uithread-init-contract-residuals-784-787-788 #809's AC5 with a fresh runtime measurement: constructing and showing QuickFiler.Viewers.SyncContextForm on an MTA thread was measured to complete without throwing on this host.
  • Records the issue Refactor: pr-778-post-merge-review-residuals #782 findings (retry-after-failed-Initialize() is already correctly implemented in production; only the test's apartment premise and message discrimination needed tightening).
  • Raises UtilitiesCS/Threading/UiThread.cs line coverage from 96.03% (121/126) to 97.74% (130/133); the newly-hardened return true; line moves from uncovered to covered.
  • Files issue Bug: uithread-dispatcher-exit-null-dispatcher-referenceequals #889 for a related, out-of-scope residual found during review: the sibling dispatcher exit has the same null-reference-equals-null shape one exit below the one hardened here.

Why

Issue #809 hardened three related defects in UtilitiesCS/Threading/UiThread.cs but left two residuals: (1) the _uiSyncContext exit of IsCompleted still admitted a thread with no captured dispatcher because a null-to-null ReferenceEquals comparison passes, and (2) AC5 of #809's own specification — a runtime measurement of whether constructing SyncContextForm on an MTA thread throws — was never actually measured, only asserted from the issue #782 narrative. This PR closes both residuals under issue #816.

What Changed

Core fix (UtilitiesCS/Threading/UiThread.cs, +15/-2, confined to the _uiSyncContext exit and its comment; the other four exits of the five-exit accessor are byte-identical):

// A non-null captured UI dispatcher is required in addition to context identity, because a
// null captured dispatcher would otherwise satisfy a null-to-null reference match on a thread
// that owns no dispatcher of its own.
if (ReferenceEquals(_context, _uiSyncContext) && _dispatcher is not null
    && ReferenceEquals(System.Windows.Threading.Dispatcher.FromThread(Thread.CurrentThread), _dispatcher))
{
    return true;
}

Tests (new file UtilitiesCS.Test/Threading/UiThreadApartmentMeasurement_Tests.cs, plus one addition to UtilitiesCS.Test/Threading/UiThread_Tests.cs and two assertion tightenings in UtilitiesCS.Test/Threading/UiThreadInitContract_Tests.cs):

  • UiThreadPredicateHardening_Tests — two negative cases: a recycled thread-id with a foreign dispatcher, and a null captured dispatcher with none on the executing thread. Both are red against the unmodified predicate and green after.
  • UiThreadApartmentMeasurement_Tests.SyncContextFormShow_OnAThreadMeasuredAsMta_RecordsTheOutcome — the runtime measurement for issue Bug: uithread-init-contract-residuals-784-787-788 #809's AC5, run on a dedicated MTA thread with the form disposed in a finally and hidden from the taskbar/minimized before Show(), so no window is displayed in an unattended run.
  • A positive twin proving the dispatcher-owning-caller case is unaffected.
  • The existing retry test gained an explicit ApartmentState.STA assertion and a .WithMessage(...) constraint so it can no longer pass for the wrong reason.

Docs: this feature folder's spec.md/issue.md/user-story.md acceptance criteria checked off (AC1-AC14), plus issue #809's own spec.md AC5 checkbox, plus the AC5 measurement artifact mirrored into issue #809's evidence folder.

Architecture / How It Fits Together

No new type or seam. The change is a single added conjunct plus a null guard on one existing exit of a five-exit accessor inside UtilitiesCS.Threading.UiThread. UiThread.Init()/Initialize() are unchanged (verified by an anchored diff against a pre-change ref showing zero lines removed from the initializer).

Verification

Completed (see docs/features/active/2026-09-08-uithread-iscompleted-branch2-residual-and-ac5-apartment-measurement-816/evidence/ and the policy-audit/code-review/feature-audit artifacts dated 2026-09-14T00-40 in the same folder):

  • dotnet tool run csharpier format . / check . — clean, no file rewritten on the final pass.
  • msbuild /t:Rebuild ... /p:EnableNETAnalyzers=true /p:EnforceCodeStyleInBuild=true — 0 errors.
  • msbuild /t:Rebuild ... /p:TreatWarningsAsErrors=true (nullable) — 0 errors, no /p:Nullable=enable.
  • vstest.console.exe over UtilitiesCS.Test + QuickFiler.Test with coverage — 6336/6336 passed, run three times.
  • Per-file coverage of UtilitiesCS/Threading/UiThread.cs: 96.03% → 97.74% (CLAUDE.md floor is 80%); repo-wide first-party line coverage 81.51% → 81.52%.
  • Independent re-derivation by the feature-review agent from the raw Cobertura documents (not merely cited from the executor), confirming the get_IsCompleted accessor's line-rate moved from 0.9 to 1.0 and branch-rate from 0.917 to 1.0.

Recommended: none beyond the above; this is a self-contained hardening with full regression coverage.

Backward Compatibility / Migration Notes

No breaking change. No public API changed. The accessor's five-exit structure and source order are unchanged; only the _uiSyncContext exit's condition is stricter, which can only turn a prior true into false for callers that previously matched context identity with a null captured dispatcher — a state issue #809 already establishes as a defect rather than an intended caller path.

Risks and Mitigations

  • Risk: the added dispatcher check could reject a legitimately UI-owned thread if _dispatcher is ever null on a fully-initialized instance. Mitigation: Init()/Initialize() are unchanged by this PR (verified by diff), so _dispatcher is populated on every successful initialization exactly as before; the new conjunct only changes behavior for the previously-defective null/foreign-thread case.
  • Risk: apartment-state test flakiness. Mitigation: every new test creates its own thread with an explicit requested apartment and joins it before returning; none relies on the ambient test-worker apartment.

Review Guide

  1. UtilitiesCS/Threading/UiThread.cs — the one-exit hardening (smallest, highest-value read).
  2. UtilitiesCS.Test/Threading/UiThreadApartmentMeasurement_Tests.cs — new file, both hardening-negative tests and the AC5 measurement test.
  3. UtilitiesCS.Test/Threading/UiThread_Tests.cs and UiThreadInitContract_Tests.cs — the positive twin and the two assertion tightenings.
  4. docs/features/active/2026-09-08-.../spec.md and docs/features/active/2026-09-07-.../spec.md — acceptance-criteria check-offs, for traceability only.

Follow-ups

GitHub Auto-close

# Conflicts:
#	.claude/agent-memory/atomic-executor/MEMORY.md
#	.claude/agent-memory/atomic-planner/MEMORY.md
#	.claude/agent-memory/orchestrator/MEMORY.md
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_012YZxqEe1udErQiYYt6Bb2d
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_012YZxqEe1udErQiYYt6Bb2d
…etry assertions

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_012YZxqEe1udErQiYYt6Bb2d
…dings

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_012YZxqEe1udErQiYYt6Bb2d
…dence

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_012YZxqEe1udErQiYYt6Bb2d
…d settle issue 809 AC5 (#816)

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_012YZxqEe1udErQiYYt6Bb2d
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_012YZxqEe1udErQiYYt6Bb2d
@drmoisan
drmoisan merged commit 1e32500 into main Sep 14, 2026
5 checks passed
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: uithread-iscompleted-branch2-residual-and-ac5-apartment-measurement

1 participant