From fdce0d860b71c4b6be0debf120b49cadc5694300 Mon Sep 17 00:00:00 2001 From: Frode Hus Date: Fri, 18 Sep 2026 23:10:08 +0200 Subject: [PATCH 1/2] Stop the app-model tests racing the model's own callbacks AppModel captures SynchronizationContext.Current and marshals callbacks from the activation coordinator back onto it, because in the app that is the one UI thread every member is required to run on. The context xUnit installs is not a thread at all: AsyncTestSyncContext.Post hands the callback to the thread pool. So an activation's progress callback could mutate Progress and Active while the authoritative loop at the end of ActivateCoreAsync was writing the same dictionaries, and the suite failed about once in twenty runs with "Operations that change non-concurrent collections must have exclusive access" out of Dictionary.set_Item. Nothing is wrong with the app: there the context is the dispatcher, so the callback and the loop are serialised on one thread, and the comment on that callback already allows it to land on either side of the loop. Only the harness broke the precondition. Construct the test model with no ambient context, so Post runs inline. The coordinator raises progress from inside the call the model is awaiting, so the callback now lands before that await returns - one of the two orderings the UI thread already produces, and the deterministic one. QuickActivateUsesTheRememberedReasonOrAsksForTheDialog is where it surfaced, having failed twice in twenty-five runs before; it passed forty consecutive runs after, and the whole suite five. Co-Authored-By: Claude Opus 5 --- .../Elevate.App.Tests/Support/TestModel.cs | 23 +++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/windows/tests/Elevate.App.Tests/Support/TestModel.cs b/windows/tests/Elevate.App.Tests/Support/TestModel.cs index cb25fb29..a042709a 100644 --- a/windows/tests/Elevate.App.Tests/Support/TestModel.cs +++ b/windows/tests/Elevate.App.Tests/Support/TestModel.cs @@ -126,8 +126,27 @@ public TestModel( Pinned = pinning ? new FakePinnedProviders(Tokens) : null; Notifier = notifier ?? new RecordingNotifier(); HotKeys = new NoopHotKeyCenter(); - Model = new AppModel(Tokens, Http, Store, Notifier, new FixedNetworkMonitor(online), Settings, FirstParty, - ownApp, ownAppFactory, HotKeys, Pinned); + // AppModel captures SynchronizationContext.Current and marshals callbacks from the + // coordinator back onto it, because in the app that is the one UI thread it requires. The + // context xUnit installs is not a thread at all: its Post hands the callback to the thread + // pool, so an activation's progress callback could mutate Progress and Active while the + // authoritative loop in ActivateCoreAsync was writing the same dictionaries — a genuine + // data race, and an intermittent "non-concurrent collections must have exclusive access". + // Constructing the model with no ambient context makes Post run inline instead: the + // coordinator raises progress from inside the call the model is awaiting, so the callback + // lands before that await returns. That is one of the two orderings the UI thread already + // produces, and it is the deterministic one. + var ambient = SynchronizationContext.Current; + SynchronizationContext.SetSynchronizationContext(null); + try + { + Model = new AppModel(Tokens, Http, Store, Notifier, new FixedNetworkMonitor(online), Settings, FirstParty, + ownApp, ownAppFactory, HotKeys, Pinned); + } + finally + { + SynchronizationContext.SetSynchronizationContext(ambient); + } } public RecordingNotifier Notifier { get; } From 1b7aaab518b66d5a9338cb7d7bb5e2269beee5d1 Mon Sep 17 00:00:00 2001 From: Frode Hus Date: Fri, 18 Sep 2026 23:25:56 +0200 Subject: [PATCH 2/2] Fix two propagation tests that the inline Post exposed Running the model's callbacks inline rather than through xUnit's thread-pool context made two tests from #181 fail reliably in CI, having passed locally by luck of scheduling. ARowIsPropagatingTheMomentItIsActivated asserts the state WatchPropagation sets before any probe runs, but the helper paced probes a millisecond apart, so the probe could settle first and the row was already done. It now holds the probe off entirely; the test is about what happens before one, so no probe should be able to race it. AConfirmedProbeClearsTheRowAndNotifies waited for the row to settle and then asserted on the notification, which the model raises without awaiting - so it landed just after. It now waits for the notification too. Its sibling, which proves a role ready at once is NOT announced, gained a short delay so the absence is evidence rather than impatience. RecordingNotifier is guarded while there: a probe reports from a background task, so the list was appended from one thread and read from another. Twelve consecutive runs of the app suite, clean. Co-Authored-By: Claude Opus 5 --- .../AppModelPropagationTests.cs | 43 ++++++++++++++++--- .../Elevate.App.Tests/Support/TestModel.cs | 19 ++++++-- 2 files changed, 52 insertions(+), 10 deletions(-) diff --git a/windows/tests/Elevate.App.Tests/AppModelPropagationTests.cs b/windows/tests/Elevate.App.Tests/AppModelPropagationTests.cs index 020c6ea7..5398c384 100644 --- a/windows/tests/Elevate.App.Tests/AppModelPropagationTests.cs +++ b/windows/tests/Elevate.App.Tests/AppModelPropagationTests.cs @@ -24,16 +24,41 @@ private static ActivationOutcome Activated(RoleKey key, DateTimeOffset? started new(key, new ActivationResult.Activated(new ActiveAssignment( key, "a1", started ?? DateTimeOffset.UtcNow, DateTimeOffset.UtcNow.AddHours(1), AssignmentStatus.Active))); - /// A model whose probe answers from 's claims, paced for a test. - private static async Task ModelAsync(string? token, RecordingNotifier? notifier = null) + /// + /// A model whose probe answers from 's claims. The pacing is a + /// millisecond so a test that waits for an answer gets one at once; pass + /// to hold the probe off instead, for a test about the state + /// before any probe has run. + /// + private static async Task ModelAsync( + string? token, RecordingNotifier? notifier = null, TimeSpan? probeAfter = null) { var tokens = new FakeTokenProvider { RefreshedToken = token }; var test = await TestModel.BootstrappedAsync(State(), online: true, tokens: tokens, notifier: notifier); - test.Model.PropagationFirstInterval = TimeSpan.FromMilliseconds(1); - test.Model.PropagationMaxInterval = TimeSpan.FromMilliseconds(1); + test.Model.PropagationFirstInterval = probeAfter ?? TimeSpan.FromMilliseconds(1); + test.Model.PropagationMaxInterval = probeAfter ?? TimeSpan.FromMilliseconds(1); return test; } + /// + /// Waits for rather than for a length of time. The model reports a + /// probe from a background task, so anything it triggers — a row changing, a notification — + /// lands a moment after the call that started it returns. + /// + private static async Task EventuallyAsync(Func condition, string what) + { + var deadline = DateTimeOffset.UtcNow.AddSeconds(5); + while (!condition()) + { + if (DateTimeOffset.UtcNow > deadline) + { + throw new TimeoutException($"Timed out waiting for {what}."); + } + + await Task.Delay(5); + } + } + /// Waits for the watch on to settle, rather than for a fixed time. private static async Task SettledAsync(AppModel model, RoleKey key) { @@ -52,8 +77,8 @@ private static async Task SettledAsync(AppModel model, RoleKey key) [Fact] public async Task ARowIsPropagatingTheMomentItIsActivated() { - // No token to read, so the probe cannot confirm — but the row must say so before it asks. - using var test = await ModelAsync(token: null); + // The probe is held off, so this is the state WatchPropagation sets before asking anything. + using var test = await ModelAsync(token: null, probeAfter: TimeSpan.FromMinutes(5)); var key = EntraKey; test.Model.Active[key] = new ActiveAssignment(key, "a1", DateTimeOffset.UtcNow, null, AssignmentStatus.Active); @@ -79,6 +104,8 @@ public async Task AConfirmedProbeClearsTheRowAndNotifies() test.Model.Propagation.Should().NotContainKey(key); test.Model.PropagationNote(key).Should().BeNull(); + // The notification is raised without being awaited, so it lands just after the state does. + await EventuallyAsync(() => notifier.Posted.Count > 0, "the ready notification"); notifier.Posted.Should().ContainSingle().Which.Title.Should().Contain("is ready"); } @@ -92,6 +119,8 @@ public async Task ARoleReadyStraightAwayIsNotWorthANotification() test.Model.WatchPropagation([Activated(key)]); await SettledAsync(test.Model, key); + // Nothing to wait for here, so give the notification every chance to appear and prove it does not. + await Task.Delay(100); test.Model.Propagation.Should().NotContainKey(key); notifier.Posted.Should().BeEmpty(); @@ -115,7 +144,7 @@ public async Task AnUnobservableRoleGoesBackToAPlainActiveRow() [Fact] public async Task DeactivatingWhileItPropagatesDropsTheWatch() { - using var test = await ModelAsync(token: null); + using var test = await ModelAsync(token: null, probeAfter: TimeSpan.FromMinutes(5)); var key = EntraKey; test.Model.Active[key] = new ActiveAssignment(key, "a1", DateTimeOffset.UtcNow, null, AssignmentStatus.Active); test.Model.WatchPropagation([Activated(key)]); diff --git a/windows/tests/Elevate.App.Tests/Support/TestModel.cs b/windows/tests/Elevate.App.Tests/Support/TestModel.cs index a042709a..540b1871 100644 --- a/windows/tests/Elevate.App.Tests/Support/TestModel.cs +++ b/windows/tests/Elevate.App.Tests/Support/TestModel.cs @@ -58,10 +58,19 @@ public Task AcquireInteractivelyAsync(Identity identity, string tenantId Inner.AcquireInteractivelyAsync(identity, tenantId, scopes, claims, ct); } -/// Records every notification the model posts. +/// +/// Records every notification the model posts. Guarded: a propagation probe reports from a +/// background task, so the list is appended from one thread while a test reads it from another. +/// public sealed class RecordingNotifier : IExpiryNotifier { - public List<(string Title, string Body)> Posted { get; } = []; + private readonly List<(string Title, string Body)> _posted = []; + private readonly Lock _gate = new(); + + public IReadOnlyList<(string Title, string Body)> Posted + { + get { lock (_gate) { return [.. _posted]; } } + } public Task RescheduleAsync( IReadOnlyList assignments, @@ -79,7 +88,11 @@ public Task SetPackageExpiriesAsync(IReadOnlyList expiries) public Task NotifyAsync(string title, string body) { - Posted.Add((title, body)); + lock (_gate) + { + _posted.Add((title, body)); + } + return Task.CompletedTask; } }