From ddf0dd6a2b132596fb95eaaeec1845a1b8388285 Mon Sep 17 00:00:00 2001 From: Jaap-Jan de Wit | DodoTech Date: Sat, 1 Aug 2026 21:43:21 +0200 Subject: [PATCH] Stop the transfer tests depending on which thread ran them first MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every test in TransferQueueingTests failed on the Linux runner with "The calling thread cannot access this object because a different thread owns it", and none of them had anything to do with the commits in that run. The class drained its rows through Dispatcher.UIThread.RunJobs(). That dispatcher is process-wide and belongs to whichever thread touched it first, and xunit runs each test class as its own parallel collection — so the moment a runner scheduled another class onto that thread ahead of this one, all nine died inside DispatcherOperation.Execute having asserted nothing about transfers at all. It passes locally and fails on a machine that schedules differently, which is the whole of why this took a CI run to find. TransfersViewModel now takes the poster it marshals through, defaulting to Dispatcher.UIThread.Post — the seam VaultViewModel's clipboard already is, for the same reason: a view model that reaches a process-wide UI object directly makes every test of it depend on a thread it does not choose. No head passes the parameter, so nothing about the running application changes. The test supplies a queue of its own and drains it, which is the same shape the dispatcher gave it. A poster that ran the action inline was tried first and is wrong: the transfer queue raises Changed from its pump thread as well as from the call that enqueued, so inline execution has a background thread adding rows to an ObservableCollection while the test reads it — it passed once and then failed a different test on the next run. Draining keeps every mutation on the thread doing the asserting, which is the one thing the dispatcher was providing that was worth keeping. One test added for the property that broke: queueing is reachable from any thread and must not care which. The class as a whole guards the seam — remove it and nothing drains, so every assertion about a row fails. Verified by reproducing the failure first: a throwaway probe that touched the dispatcher on one thread and posted and drained on another produced exactly the CI message. Then six consecutive Release runs of the app suite, all green, plus the layout suite, which builds a TransfersViewModel of its own. Co-Authored-By: Claude Opus 5 (1M context) --- .../ViewModels/TransfersViewModel.cs | 23 +++++- .../TransferQueueingTests.cs | 77 ++++++++++++++++--- 2 files changed, 88 insertions(+), 12 deletions(-) diff --git a/src/DodoSSH.Client.Shell/ViewModels/TransfersViewModel.cs b/src/DodoSSH.Client.Shell/ViewModels/TransfersViewModel.cs index a1f4078..cb00adf 100644 --- a/src/DodoSSH.Client.Shell/ViewModels/TransfersViewModel.cs +++ b/src/DodoSSH.Client.Shell/ViewModels/TransfersViewModel.cs @@ -250,6 +250,7 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo { private readonly ISftpSessionFactory sftp; private readonly FileTransferQueue queue; + private readonly Action post; private VaultViewModel? vault; private VaultKnownHostStore? knownHosts; @@ -269,9 +270,27 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo private bool disposed; - internal TransfersViewModel(ISftpSessionFactory sftp, TimeProvider clock) + /// Opens SFTP sessions. + /// Time source, for transfer rates and timestamps. + /// + /// Runs an action on the thread this view model's collections are read from. Defaults to the UI thread's + /// dispatcher, which is the answer in every real head. + /// + /// A delegate rather than Dispatcher.UIThread reached directly, for the reason + /// VaultViewModel's clipboard is one. Dispatcher.UIThread is process-wide and belongs to + /// whichever thread touched it first, so a test that posts through it is asserting on a queue owned by + /// some other test's thread — which passes or throws "the calling thread cannot access this object" + /// depending on the order a runner happened to schedule its classes in. Running the action inline + /// removes the thread from the question rather than making the test guess it right. + /// + /// + internal TransfersViewModel( + ISftpSessionFactory sftp, + TimeProvider clock, + Action? post = null) { this.sftp = sftp; + this.post = post ?? (action => Dispatcher.UIThread.Post(action)); // The supplier answers with whatever session is current at the moment a transfer starts, which is // what lets a queue survive a disconnect and reconnect without every queued row failing. @@ -1296,7 +1315,7 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo /// instead of one for the first change and one for the rest. /// private void OnTransferChanged(object? sender, TransferChangedEventArgs e) => - Dispatcher.UIThread.Post(() => + post(() => { if (Transfers.FirstOrDefault(row => row.Id == e.Transfer.Id) is { } existing) { diff --git a/tests/DodoSSH.Client.App.Tests/TransferQueueingTests.cs b/tests/DodoSSH.Client.App.Tests/TransferQueueingTests.cs index b9318e0..3882ee2 100644 --- a/tests/DodoSSH.Client.App.Tests/TransferQueueingTests.cs +++ b/tests/DodoSSH.Client.App.Tests/TransferQueueingTests.cs @@ -1,4 +1,4 @@ -using Avalonia.Threading; +using System.Collections.Concurrent; using DodoSSH.Client.Shell.ViewModels; using DodoSSH.Client.Ssh; using NSubstitute; @@ -26,10 +26,33 @@ public sealed class TransferQueueingTests : IDisposable private readonly string directory = Path.Combine(Path.GetTempPath(), $"dodossh-drop-{Guid.CreateVersion7():N}"); - private readonly TransfersViewModel transfers = - new(Substitute.For(), TimeProvider.System); + /// What the view model has posted and has not run yet. + /// + /// A queue of this test's own, standing in for the dispatcher — which is what it replaces. Draining + /// Dispatcher.UIThread meant depending on which thread a runner happened to touch that + /// process-wide object from first, and once another class got there first every test here died on + /// "the calling thread cannot access this object" having asserted nothing about transfers at all. + /// + /// A queue rather than a poster that runs the action inline, and the difference is not stylistic: the + /// transfer queue raises Changed from its pump thread as well as from the call that enqueued, + /// so inline execution would have a background thread adding rows to an ObservableCollection + /// while the test reads it. Draining keeps every mutation on the thread doing the asserting, which is + /// the one thing the dispatcher was providing that is worth keeping. + /// + /// + private readonly ConcurrentQueue posted = new(); - public TransferQueueingTests() => Directory.CreateDirectory(directory); + private readonly TransfersViewModel transfers; + + public TransferQueueingTests() + { + Directory.CreateDirectory(directory); + + transfers = new TransfersViewModel( + Substitute.For(), + TimeProvider.System, + posted.Enqueue); + } /// public void Dispose() @@ -162,17 +185,51 @@ public sealed class TransferQueueingTests : IDisposable transfers.Status.ShouldContain("1 folder was skipped"); } + /// + /// Queueing is reachable from any thread — a drop is handled on the UI thread, a retry is not — and + /// nothing about it may depend on which one. Worth stating because the version of this class that + /// drained Dispatcher.UIThread did depend on exactly that, and said so only by failing in CI on + /// a machine whose scheduling differed. The draining still happens on the test's own thread, as + /// explains; what is asserted here is the half that has no business caring. + /// + [Fact] + public void QueueingFromAnotherThread_StillEnqueues() + { + Exception? failure = null; + + var thread = new Thread(() => + { + try + { + Connected(); + transfers.QueueUploads([File("one.txt")]); + } + catch (Exception exception) + { + failure = exception; + } + }); + + thread.Start(); + thread.Join(); + + failure.ShouldBeNull(); + Queued().ShouldHaveSingleItem(); + } + /// The queue's rows, once the posts that create them have been let run. /// - /// TransfersViewModel adds a row from the queue's own Changed event, which it marshals - /// through Dispatcher.UIThread because the queue raises it from a pump thread. There is no - /// Avalonia application here to drain that, so the posts are run by hand — the alternative is asserting - /// on the status line alone, which is a string this code wrote about itself and proves nothing about - /// anything having been enqueued. + /// TransfersViewModel adds a row from the transfer queue's own Changed event, which it + /// marshals because the queue raises it from a pump thread. Nothing drains that here, so the posts are + /// run by hand — the alternative is asserting on the status line alone, which is a string this code + /// wrote about itself and proves nothing about anything having been enqueued. /// private IReadOnlyList Queued() { - Dispatcher.UIThread.RunJobs(); + while (posted.TryDequeue(out var action)) + { + action(); + } return transfers.Transfers; }