fix(809): reject non-STA UiThread.Init callers, allow init retry, and relax the awaiter predicate - #814
Merged
drmoisan merged 20 commits intoSep 8, 2026
Conversation
Research establishes the AC1/AC2 interaction, a 56-site live-read census, the testability seams, and the awaiter predicate. It also shows the issue's own note misattributed a bare thread-id predicate to #781; BreadcrumbUiDispatcher records the opposite rule and two WinFormsPumpHost tests would fail under it.
Fills the seeded spec template from the research: corrected root cause for #784, the three concrete changes, the AC2 anti-regression argument, and the test strategy. Acceptance criteria are AC1-AC4 verbatim from issue.md plus AC5 (the required MTA construction measurement) and AC6 (coverage uplift). Evidence paths normalized to the canonical evidence/qa-gates/ location.
Seven phases, 71 tasks, all unchecked. Phase 0 carries the fresh-worktree bootstrap (repo SDK, NuGet restore, tool restore, analyzer parity) plus the AC5 MTA construction measurement. Toolchain command strings and success-case output shapes are taken from recorded #782 runs rather than composed. Passes the MCP plan validator with no warnings.
… test files Preflight round 1 established three unprotected parallel-bucket writers of the UiThread process-global statics. Marking only the classes this delivery adds does not serialize them, because the serial and parallel buckets overlap under ClassLevel parallelization. Each of the three gains one attribute line.
All eighteen defects applied. Phase 6 grows to 16 tasks, total 72. Adds three standing conventions (skipped-count derivation, TRX selection under a re-run, results-directory creation) and re-anchors the issue.md diff to HEAD. Also corrects the runsettings citation in the spec Write Set amendment: the MSTest parallelization uses the element form, not an attribute form.
Five defects and two observations applied. Replaces the unsatisfiable repository-wide results-file gate with a base-anchored delivery-added count, closes a producer/consumer gap on the ThreadMonitorField accessor, and corrects three cross-references. Task count unchanged at 72 across 7 phases.
Separates the pre-existing 500-line overrun in FolderPredictorTests.cs from delivery-introduced overruns, so the file-size gate measures what this change causes rather than what it inherits. Corrects the P3-T3 removal attribution and replaces two unfounded uncovered-line predictions with a deferral to the recorded baseline artifact. Task count unchanged at 72 across 7 phases.
Pins the physical-line counting idiom and rejects Measure-Object -Line by name, which counted non-blank lines and could report zero overruns on a file sixty lines over the 500-line cap. Adds base-inertness verification, stops appended Phase 5 pass sections duplicating schema fields, and completes the residual-throw enumeration. The orchestrator added the porcelain companion span to P0-T3 to clear the G8b validator warning its own base-inertness request introduced.
Records the pre-809-base anchor, SDK and package bootstrap, analyzer parity, baseline csharpier/analyzer/nullable/vstest/coverage gates, per-file UiThread.cs coverage, the decision-D5 MTA measurement and the five design preconditions.
…et scope Introduces IUiCaptureSource, UiThread.SyncContextFormFactory, UiThread.ResetForTesting, the NonStaInitMessagePrefix constant and UiThreadStateScope. No observable behaviour of Init, Initialize or IsCompleted changes in this phase.
Seventeen new MSTest cases covering the apartment precondition, retry after a failed Initialize, and the awaiter predicate. Six fail against current behaviour, which is the recorded fail-before evidence. Four classes that write UiThread statics gain DoNotParallelize.
…d awaiter AC1 adds an apartment precondition as the first statement of Init(). AC2 replaces the single-shot latch with a success-recorded flag under a serializing lock. AC3 replaces the reference-equality IsCompleted predicate. Two regression tests are re-authored to drive a dedicated MTA thread because the shared serial worker was measured to be STA.
…atcher QfcHomeControllerRunAsyncTests no longer calls UiThread.Init(false). It installs a WinFormsPumpHost dispatcher through the existing UiThreadDispatcherFixture transaction, which removes the order-dependency rather than only satisfying the new apartment precondition.
Format, format-check, analyzer build, nullable build and the full nine-assembly coverage run all pass in a single clean loop. 7137 of 7137 tests pass, the baseline 7120 plus exactly seventeen added tests.
…and closure UiThread.cs per-file line coverage rises from 76.83 to 96.03 percent. Changed-line coverage is 95.83 percent and every new member with executable lines is at or above 90 percent. Aggregate first-party coverage is comparable and rises on both line and branch. All six acceptance criteria are checked off in spec.md, with AC1 through AC4 mirrored into issue.md.
Records completion of every task through P6-T15. The P6-T16 mark is written after this commit, because no task can commit the mark that records its own completion.
…partment claim Feature review returned 0 blocking findings across policy-audit, code-review and feature-audit. Two acceptance criteria were graded PARTIAL. AC5 is unchecked in spec.md. Its measurement clause is not established: the [P0-T15] probe never read the executing thread's apartment state, and this same delivery falsified the research premise the MTA label rested on. A correction section is added to the two evidence artifacts that drew conclusions from that label, recording that the status of the #782 mechanism narrative reverts to UNKNOWN. The delivered code and tests are unaffected, because the AC2 design argument was written to stand independently of the measured value and the review verified it structurally against the head tree. AC6 remains checked. Every measurable clause is met and was recomputed by the reviewer from the raw Cobertura documents. The two unmet clauses are literal wording: the collector route and the storage location of a gitignored 18 MB report. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Section 6 of the closure summary attributed the observed STA execution to a plain [TestClass] [DoNotParallelize] class sharing the serial bucket with an [STATestClass] [DoNotParallelize] class. That stated cause is not established. Two facts verified directly against the tree support a simpler explanation: UtilitiesCS.Test/Properties/AssemblyInfo.cs:18 carries the only assembly-level Parallelize attribute in the repository, and no runsettings file sets ExecutionThreadApartmentState. An assembly invoked without a settings file therefore does not parallelize, and its tests inherit the vstest main execution thread apartment, which is STA on .NET Framework. The operational rule the section exists to convey is unchanged and is confirmed: a test that needs a caller of a known apartment must create a dedicated thread and set the apartment explicitly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
5 tasks
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
1 participant
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.
Fix three UiThread init-contract defects: non-STA acceptance, unretryable latch, and over-strict awaiter
Summary
UiThread.Init()now rejects any caller whose apartment is not STA, throwing a namedInvalidOperationExceptionas the first statement of the method, ahead of the four process-global monitoring assignments and ahead of the initialization latch (Bug: uithread-init-accepts-non-sta-callers #787).UiThreadonly, by a success-recorded flag guarded by a serializing lock. The flag is set afterInitialize()returns rather than before it runs, so a failed first attempt no longer permanently consumes the latch and a later call retries (Bug: uithread-init-latch-not-rearmed-after-failed-initialize #788).SynchronizationContextAwaiter.IsCompletedno longer forces a post for a context captured inside a WPF dispatcher operation. It keeps reference equality as its fast path and additionally admits exactly two demonstrably UI-owned cases while the caller stands on the owning UI thread (Bug: uithread-synccontext-awaiter-always-posts-for-dispatcher-built-viewers #784).IUiCaptureSourceseam plus aUiThreadStateScopesnapshot/restore helper make initialization drivable, and failable, in tests without a live Outlook host.UiThread.csline coverage rises from 76.83% to 96.03%.Why
Three findings on a single file were filed separately after the #781 and #782 reviews and are shipped together because they touch the same initialization and awaiter code and share one test suite.
The ordering between them is load-bearing rather than cosmetic. The safety argument for relaxing the latch depends on the apartment precondition already being present and being the first statement of
Init(): with the precondition in place, the expensive and potentially-throwing body ofInitialize()is unreachable from any non-STA caller, so no retry storm can originate on a thread-pool thread. The two changes must land together.What changed
Core logic (
UtilitiesCS/)Threading/UiThread.cs— apartment precondition and its message constant;ThreadSafeSingleShotGuardusage replaced bylock (InitLock)plus a_initializedflag set after success;IsCompletedrewritten;SyncContextFormFactoryandResetForTesting()test seams added. The field is now typedIUiCaptureSource?.Threading/IUiCaptureSource.cs(new) — the capture-object contractInitialize()reads UI values from.Threading/SyncContextForm.cs— implements the new interface.UtilitiesCS.csproj— one compile item for the new file.Tests
UtilitiesCS.Test/Threading/UiThreadInitContract_Tests.cs(new) — apartment rejection, latch re-arm after throw, and the anti-retry-storm case.UtilitiesCS.Test/TestHelpers/UiThreadStateScope.cs(new) — snapshot/restore of every process-global staticUiThreadowns.UtilitiesCS.Test/Threading/UiThread_Tests.cs— awaiter inline-versus-post cases, including the guards that pin the false paths.QuickFiler.Test/Controllers/QfcHomeControllerRunAsyncTests.cs— the MTAUiThread.Init(false)caller is removed and replaced with a pumping dispatcher scoped to the test, which makes the test order-independent rather than merely making it pass.[DoNotParallelize]attribute line each, because they mutateUiThreadstatics.UtilitiesCS.Test.csproj— two compile items.Docs and evidence — spec, plan, research, 44 evidence artifacts, and the three review artifacts.
Architecture / how it fits together
Init()is the single entry point that captures UI state into process-global statics. Its four reachable callers are the two directInit()calls and the two lazy accessors that route through it;UiThread.Dispatcherremains deliberately non-lazy. Placing the apartment check as the first statement means every one of those paths fails fast on a non-STA thread at one enum read and a throw, before any global is written.IsCompletedis a field-free predicate, deliberately so.GetAwaiter()runs at theawaitsite on the awaiting thread rather than on the thread that captured the context, so a construction-time capture of the owning thread id would be unsound.default(SynchronizationContextAwaiter)behaviour is therefore unchanged.Thread identity is used as a necessary guard and never as a sufficient one: each new true-branch additionally requires a reference match against an object captured at
Init()time. This preserves the rule recorded atQuickFiler/Viewers/BreadcrumbUiDispatcher.cs:263-272, which forbids substituting bare owning-thread identity for a captured-context reference match, because a continuation resumed afterConfigureAwait(false)can be scheduled onto a recycled thread-pool thread whose managed thread id equals the owner's.A null ambient context continues to return
false, which keeps the twoTaskScheduler.FromCurrentSynchronizationContext()call sites working.Verification
Completed (all recorded as evidence artifacts under the feature folder)
dotnet tool run csharpier check .Checked 1611 files(baseline 1608 + 3 new)/t:Rebuildwith analyzers0 Warning(s),0 Error(s), 18 projects/t:RebuildwithTreatWarningsAsErrors0 Warning(s),0 Error(s)UiThread.csline coverageFail-before / pass-after evidence exists for every regression test. Six tests failed against the pre-fix tree with assertion messages mapping to the specific defects, and pass after.
Feature review returned 0 blocking findings across policy audit, code review and feature audit.
Recommended — manual verification on a live Outlook host (QuickFiler launch, item load, breadcrumb open), confirming no change in observable UI behaviour and no keyboard-focus regression. The automated suite cannot close this residual: the awaiter change alters execution ordering at eleven production await sites and no existing test asserts ordering at any of them.
Known gap
AC5 is unchecked and this PR does not claim it. It required a recorded measurement of whether
new SyncContextForm(); Show();throws on an MTA thread. The probe that was run never readThread.CurrentThread.GetApartmentState(); it inferred the apartment from a research premise that this same delivery then falsified by direct measurement. Two facts verified against the tree —UtilitiesCS.Test/Properties/AssemblyInfo.cs:18carries the repository's only assembly-levelParallelizeattribute, and no runsettings file setsExecutionThreadApartmentState— indicate the probe most likely ran STA, in which case no MTA measurement was taken.The claim has been withdrawn in the evidence artifacts rather than left standing, and the status of the #782 mechanism narrative reverts to UNKNOWN.
This does not affect the delivered code. The design argument was written from the outset to stand independently of the measured value, and the review verified it structurally against the head tree: the apartment precondition makes the potentially-throwing path unreachable from any non-STA caller whatever the answer turns out to be.
Backward compatibility
UiThread.Init()gains a precondition, so a non-STA caller that previously succeeded now throws. Exactly one such caller existed in the repository, in test code, and it is corrected here. The production caller runs on the Outlook main STA thread and is unaffected. No public API is removed or renamed,ThreadSafeSingleShotGuardis retained as a type, noInternalsVisibleTogrant is added, and the reflected field names_uiSyncContextand_dispatcherare preserved for four existing test helpers.Risks and mitigations
WinFormsPumpHostTestsmarshal tests; the live-host check above is the residual.ResetForTesting(),UiThreadStateScope, and[DoNotParallelize]on every class that mutates aUiThreadstatic.UiThread.cs:177-178, the body of one predicate branch. The condition is covered and asserted false; only thereturn true;arm is unreached. TheIsCompletedmember sits at exactly 90.00%.Review guide
UtilitiesCS/Threading/UiThread.cs— the whole change is here; readIsCompletedfirst, thenInit().UtilitiesCS/Threading/IUiCaptureSource.csandSyncContextForm.cs— the seam.UtilitiesCS.Test/Threading/UiThreadInitContract_Tests.csand the additions toUiThread_Tests.cs.QuickFiler.Test/Controllers/QfcHomeControllerRunAsyncTests.cs— the largest test diff, mostly the dispatcher fixture.[DoNotParallelize]additions are mechanical.The 51 documentation and evidence files are generated audit trail and can be read last. Note that the PR-context tool classified this change as documentation-only with zero core-logic files; that classification is wrong, and the twelve source paths above are the real change.
Follow-ups
ReferenceEquals(_context, _uiSyncContext)branch ofIsCompletedwith a companion dispatcher-affinity check, and add the awaiter test that closes theUiThread.cs:177-178coverage residual. These belong together because the hardening changes what the test must assert.CLAUDE.mdCUT3 step 4 with the collection route the repository actually uses.CLAUDE.mdand.claude/rules/general-unit-test.md.UtilitiesCS.Test/OutlookObjects/Folder/FolderPredictorTests.cs(1066 lines before this change, 1067 after the one attribute line).GitHub auto-close
GitHub validation was unavailable when the PR context was collected, so no
Closesbullet is emitted. The author-asserted list produced by the tool harvested every issue number mentioned anywhere in the feature documents, including several this change does not resolve, so it is not a safe source. #809 should be closed manually on merge.