Stop the transfer tests depending on which thread ran them first

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) <noreply@anthropic.com>
This commit is contained in:
2026-08-01 21:43:21 +02:00
co-authored by Claude Opus 5
parent 093f3904c1
commit ddf0dd6a2b
2 changed files with 88 additions and 12 deletions
@@ -250,6 +250,7 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo
{ {
private readonly ISftpSessionFactory sftp; private readonly ISftpSessionFactory sftp;
private readonly FileTransferQueue queue; private readonly FileTransferQueue queue;
private readonly Action<Action> post;
private VaultViewModel? vault; private VaultViewModel? vault;
private VaultKnownHostStore? knownHosts; private VaultKnownHostStore? knownHosts;
@@ -269,9 +270,27 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo
private bool disposed; private bool disposed;
internal TransfersViewModel(ISftpSessionFactory sftp, TimeProvider clock) /// <param name="sftp">Opens SFTP sessions.</param>
/// <param name="clock">Time source, for transfer rates and timestamps.</param>
/// <param name="post">
/// 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.
/// <para>
/// A delegate rather than <c>Dispatcher.UIThread</c> reached directly, for the reason
/// <c>VaultViewModel</c>'s clipboard is one. <c>Dispatcher.UIThread</c> 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.
/// </para>
/// </param>
internal TransfersViewModel(
ISftpSessionFactory sftp,
TimeProvider clock,
Action<Action>? post = null)
{ {
this.sftp = sftp; 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 // 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. // 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. /// instead of one for the first change and one for the rest.
/// </remarks> /// </remarks>
private void OnTransferChanged(object? sender, TransferChangedEventArgs e) => private void OnTransferChanged(object? sender, TransferChangedEventArgs e) =>
Dispatcher.UIThread.Post(() => post(() =>
{ {
if (Transfers.FirstOrDefault(row => row.Id == e.Transfer.Id) is { } existing) if (Transfers.FirstOrDefault(row => row.Id == e.Transfer.Id) is { } existing)
{ {
@@ -1,4 +1,4 @@
using Avalonia.Threading; using System.Collections.Concurrent;
using DodoSSH.Client.Shell.ViewModels; using DodoSSH.Client.Shell.ViewModels;
using DodoSSH.Client.Ssh; using DodoSSH.Client.Ssh;
using NSubstitute; using NSubstitute;
@@ -26,10 +26,33 @@ public sealed class TransferQueueingTests : IDisposable
private readonly string directory = private readonly string directory =
Path.Combine(Path.GetTempPath(), $"dodossh-drop-{Guid.CreateVersion7():N}"); Path.Combine(Path.GetTempPath(), $"dodossh-drop-{Guid.CreateVersion7():N}");
private readonly TransfersViewModel transfers = /// <summary>What the view model has posted and <see cref="Queued"/> has not run yet.</summary>
new(Substitute.For<ISftpSessionFactory>(), TimeProvider.System); /// <remarks>
/// A queue of this test's own, standing in for the dispatcher — which is what it replaces. Draining
/// <c>Dispatcher.UIThread</c> 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.
/// <para>
/// A queue rather than a poster that runs the action inline, and the difference is not stylistic: the
/// transfer queue raises <c>Changed</c> from its pump thread as well as from the call that enqueued,
/// so inline execution would have a background thread adding rows to an <c>ObservableCollection</c>
/// 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.
/// </para>
/// </remarks>
private readonly ConcurrentQueue<Action> posted = new();
public TransferQueueingTests() => Directory.CreateDirectory(directory); private readonly TransfersViewModel transfers;
public TransferQueueingTests()
{
Directory.CreateDirectory(directory);
transfers = new TransfersViewModel(
Substitute.For<ISftpSessionFactory>(),
TimeProvider.System,
posted.Enqueue);
}
/// <inheritdoc /> /// <inheritdoc />
public void Dispose() public void Dispose()
@@ -162,17 +185,51 @@ public sealed class TransferQueueingTests : IDisposable
transfers.Status.ShouldContain("1 folder was skipped"); transfers.Status.ShouldContain("1 folder was skipped");
} }
/// <remarks>
/// 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 <c>Dispatcher.UIThread</c> 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
/// <see cref="Queued"/> explains; what is asserted here is the half that has no business caring.
/// </remarks>
[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();
}
/// <summary>The queue's rows, once the posts that create them have been let run.</summary> /// <summary>The queue's rows, once the posts that create them have been let run.</summary>
/// <remarks> /// <remarks>
/// <c>TransfersViewModel</c> adds a row from the queue's own <c>Changed</c> event, which it marshals /// <c>TransfersViewModel</c> adds a row from the transfer queue's own <c>Changed</c> event, which it
/// through <c>Dispatcher.UIThread</c> because the queue raises it from a pump thread. There is no /// marshals because the queue raises it from a pump thread. Nothing drains that here, so the posts are
/// Avalonia application here to drain that, so the posts are run by hand — the alternative is asserting /// run by hand — the alternative is asserting on the status line alone, which is a string this code
/// on the status line alone, which is a string this code wrote about itself and proves nothing about /// wrote about itself and proves nothing about anything having been enqueued.
/// anything having been enqueued.
/// </remarks> /// </remarks>
private IReadOnlyList<TransferRowViewModel> Queued() private IReadOnlyList<TransferRowViewModel> Queued()
{ {
Dispatcher.UIThread.RunJobs(); while (posted.TryDequeue(out var action))
{
action();
}
return transfers.Transfers; return transfers.Transfers;
} }