Public Access
The test step ran for twenty minutes and had to be stopped by hand. Eighteen suites
had passed; DodoSSH.Client.App.Tests never reported.
◆ ON UNIX, A FRESH SERVER PER CONNECTION LOSES THE NEXT ONE.
The doorbell's accept loop built a NamedPipeServerStream inside the loop, served one
caller and disposed it. That is fine on Windows and quietly wrong on Linux, where a
named pipe is a Unix domain socket: disposing the server closes the *listening*
socket and unlinks its path, so a second launch that connected while the first was
being served has its connection dropped. Its Connect returned, its write succeeded
into a buffer, and nobody ever read it — the launch was told it had rung the
doorbell and the window never came forward.
Reproduced in a container rather than reasoned about, because the two platforms
disagree and this machine is only one of them. Three rings back to back:
one instance, Disconnect between callers -> 3 of 3 answered
a fresh instance per connection -> 1 of 3 answered
So there is one server for the life of the doorbell now, Disconnect()ed between
conversations, and the socket is never not listening. Which makes leaveOpen on the
reader load-bearing too: a StreamReader disposes the stream it was given, and that
stream now has to survive into the next iteration.
Two things I had wrong about this, both worth recording. Cancelling
WaitForConnectionAsync *does* unblock a pending accept on Linux — that was my first
theory and the container disproved it, so DisposeAsync awaiting the loop was never
the problem. And the app itself never hung: the lost ring is a launch that is
ignored, not a process that stops.
◆ AND THE REASON IT COST TWENTY MINUTES RATHER THAN TWENTY SECONDS.
await twice.Task.WaitAsync(TestContext.Current.CancellationToken);
That token carries no timeout. A ring that never arrives is then not a failing test,
it is a job that sits there — which is exactly what happened, and it is the more
expensive of the two mistakes because it turns any future bug of this shape into a
runner somebody has to go and stop. Both awaits are bounded now.
TheDoorbellAnswersMoreThanOnce keeps its shape deliberately: two rings back to back
rather than one and a pause, because the gap between accepts is the whole defect. A
test that waited for the first to be answered would pass against the broken version.
ci.yml had no timeout-minutes anywhere, on any job. Every job has a ceiling now, and
the test step has a tighter one of its own — ten minutes against a suite that takes
two — because a hang there is the likeliest of the lot and a step-level timeout names
the step instead of leaving a reader to work out which of fifteen was still running.
None of them is a performance budget; a build that legitimately grows past one should
have the number raised.
Verified both ways on Linux, in a container, against the committed tree and this one:
31c836f SingleInstanceTests -> hung in TheDoorbellAnswersMoreThanOnce,
still going when killed at 3 minutes
this SingleInstanceTests -> 7 of 7 in 1.3s
this the whole suite -> 570 tests, 0 failed, 88s
And on Windows, where none of this was ever broken: 570 tests, 0 failed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
202 lines
8.3 KiB
C#
202 lines
8.3 KiB
C#
using DodoSSH.Client.App.Platform;
|
|
using DodoSSH.Client.Session;
|
|
|
|
namespace DodoSSH.Client.App.Tests;
|
|
|
|
/// <summary>
|
|
/// One desktop instance per profile, and what the second launch does instead of starting.
|
|
/// </summary>
|
|
/// <remarks>
|
|
/// <para>
|
|
/// Both halves are real here: a real lock file in a real temporary profile, and a real named pipe. Neither
|
|
/// is worth faking — the whole claim being made is about what the operating system does with an exclusive
|
|
/// handle and a pipe name, and a fake of either would be a test of the fake.
|
|
/// </para>
|
|
/// <para>
|
|
/// What is <em>not</em> reachable from here is a second process. Every test below runs in one, so the lock
|
|
/// is proved against a second <em>handle</em> rather than a second executable, and the window that comes
|
|
/// forward is a delegate rather than a window. <c>docs/manual-checks.md</c> 16.11 is the other side of that
|
|
/// line.
|
|
/// </para>
|
|
/// </remarks>
|
|
public sealed class SingleInstanceTests : IDisposable
|
|
{
|
|
/// <summary>
|
|
/// A profile of its own per test, which is what makes these safe to run beside a real installation.
|
|
/// </summary>
|
|
/// <remarks>
|
|
/// The pipe name is derived from this directory — see <c>SingleInstance.PipeNameFor</c> — so a unique
|
|
/// directory is also a unique pipe. Without that, a test here would ring the doorbell of whatever
|
|
/// DodoSSH the developer happens to have running.
|
|
/// </remarks>
|
|
private readonly ClientPaths paths =
|
|
new(Path.Combine(Path.GetTempPath(), $"dodossh-instance-{Guid.CreateVersion7():N}"));
|
|
|
|
/// <summary>
|
|
/// No patience at all, which is what every test here wants.
|
|
/// </summary>
|
|
/// <remarks>
|
|
/// The shipped three seconds exist for a lock held by a process on its way out — an update relaunching
|
|
/// itself, above all — and nothing here is on its way anywhere. Passing zero takes the give-up decision
|
|
/// on the first attempt, so a refusal costs a millisecond rather than three seconds of the suite.
|
|
/// </remarks>
|
|
private static readonly TimeSpan Impatient = TimeSpan.Zero;
|
|
|
|
/// <summary>
|
|
/// How long a doorbell is given to ring before the test says it never will.
|
|
/// </summary>
|
|
/// <remarks>
|
|
/// ◆ <b>A bound at all, which is the lesson rather than the number.</b> These two awaits were written
|
|
/// against <c>TestContext.Current.CancellationToken</c> alone, which carries no timeout — so the run
|
|
/// where a ring was genuinely lost did not fail, it <em>hung</em>, and took the whole CI job with it for
|
|
/// as long as anybody let it. Twenty seconds is a hundred times what a local pipe needs and a fifth of
|
|
/// what it costs to notice; a bound that is never reached costs nothing and a missing one costs the
|
|
/// build.
|
|
/// </remarks>
|
|
private static readonly TimeSpan LongEnoughForAPipe = TimeSpan.FromSeconds(20);
|
|
|
|
/// <inheritdoc />
|
|
public void Dispose()
|
|
{
|
|
if (Directory.Exists(paths.DataDirectory))
|
|
{
|
|
Directory.Delete(paths.DataDirectory, recursive: true);
|
|
}
|
|
}
|
|
|
|
[Fact]
|
|
public void TheFirstLaunchClaimsTheProfile()
|
|
{
|
|
using var claim = SingleInstance.Claim(paths, Impatient);
|
|
|
|
claim.ShouldNotBeNull();
|
|
File.Exists(paths.InstanceLockFile).ShouldBeTrue("the claim is a handle on a real file");
|
|
}
|
|
|
|
/// <remarks>
|
|
/// The point of the whole exercise. Nobody is listening in this test, so the second claim takes the
|
|
/// give-up path rather than the ask-and-leave one — which is why it is handed no patience at all.
|
|
/// </remarks>
|
|
[Fact]
|
|
public void ASecondLaunchIsRefused()
|
|
{
|
|
using var first = SingleInstance.Claim(paths, Impatient);
|
|
first.ShouldNotBeNull();
|
|
|
|
SingleInstance.Claim(paths, Impatient).ShouldBeNull();
|
|
}
|
|
|
|
/// <remarks>
|
|
/// ◆ There is no such thing as a stale lock here, and this is what says so. The handle is closed by the
|
|
/// runtime when it is disposed and by the kernel when the process ends however it ends — which is the
|
|
/// reason this is a file rather than a named mutex, whose abandonment is a state to be handled.
|
|
/// </remarks>
|
|
[Fact]
|
|
public void TheProfileIsClaimableAgainOnceTheFirstInstanceLetsGo()
|
|
{
|
|
var first = SingleInstance.Claim(paths, Impatient);
|
|
first.ShouldNotBeNull();
|
|
first.Dispose();
|
|
|
|
using var second = SingleInstance.Claim(paths, Impatient);
|
|
|
|
second.ShouldNotBeNull("the lock went with the handle");
|
|
}
|
|
|
|
/// <remarks>
|
|
/// The other half: a refused launch is not a silent one. Somebody who double-clicks the icon and sees
|
|
/// nothing has been told the application is broken, so the running instance is asked to come forward.
|
|
/// </remarks>
|
|
[Fact]
|
|
public async Task ARefusedLaunchAsksTheRunningInstanceToComeForward()
|
|
{
|
|
using var first = SingleInstance.Claim(paths, Impatient);
|
|
first.ShouldNotBeNull();
|
|
|
|
var asked = new TaskCompletionSource();
|
|
|
|
await using var doorbell = SingleInstance.Listen(paths, () => asked.TrySetResult());
|
|
|
|
SingleInstance.Claim(paths, Impatient).ShouldBeNull("the profile is taken");
|
|
|
|
// Awaited rather than polled: the callback comes off a thread pool thread. Bounded explicitly,
|
|
// because the test's own token is not — see LongEnoughForAPipe.
|
|
await asked.Task.WaitAsync(LongEnoughForAPipe, TestContext.Current.CancellationToken);
|
|
}
|
|
|
|
/// <remarks>
|
|
/// <para>
|
|
/// A third and a fourth launch work as well as the second, and this is the test that caught the doorbell
|
|
/// getting that wrong on Linux and only on Linux. A fresh <c>NamedPipeServerStream</c> per connection
|
|
/// closes the listening socket between callers there, so the second ring was accepted by nobody while
|
|
/// the launch that made it was told it had succeeded. See <c>SingleInstance.Doorbell</c>.
|
|
/// </para>
|
|
/// <para>
|
|
/// Two rings back to back rather than one and a pause, because the gap is the whole defect: a test that
|
|
/// waited for the first to be answered before making the second would pass against the broken version.
|
|
/// </para>
|
|
/// </remarks>
|
|
[Fact]
|
|
public async Task TheDoorbellAnswersMoreThanOnce()
|
|
{
|
|
using var first = SingleInstance.Claim(paths, Impatient);
|
|
first.ShouldNotBeNull();
|
|
|
|
var rings = 0;
|
|
var twice = new TaskCompletionSource();
|
|
|
|
await using var doorbell = SingleInstance.Listen(
|
|
paths,
|
|
() =>
|
|
{
|
|
if (Interlocked.Increment(ref rings) == 2)
|
|
{
|
|
twice.TrySetResult();
|
|
}
|
|
});
|
|
|
|
SingleInstance.AskTheRunningOneToComeForward(paths).ShouldBeTrue();
|
|
SingleInstance.AskTheRunningOneToComeForward(paths).ShouldBeTrue();
|
|
|
|
await twice.Task.WaitAsync(LongEnoughForAPipe, TestContext.Current.CancellationToken);
|
|
}
|
|
|
|
/// <remarks>
|
|
/// What the give-up path turns on. A lock held by something that is not listening — a process still
|
|
/// starting, or one on its way out — answers no, and that is what makes <c>Claim</c> wait and try the
|
|
/// lock again rather than concluding anything from the silence.
|
|
/// </remarks>
|
|
[Fact]
|
|
public void AskingWithNobodyListening_SaysSo()
|
|
{
|
|
SingleInstance.AskTheRunningOneToComeForward(paths).ShouldBeFalse();
|
|
}
|
|
|
|
/// <remarks>
|
|
/// Two profiles, because a nightly and a release build are installed side by side on purpose and must
|
|
/// not exclude each other — ADR 0013. The lock file is inside the profile and the pipe name is derived
|
|
/// from it, so both halves are scoped by the one decision <c>ClientPaths.ForChannel</c> already made.
|
|
/// </remarks>
|
|
[Fact]
|
|
public void TwoProfilesDoNotExcludeEachOther()
|
|
{
|
|
var other = new ClientPaths(paths.DataDirectory + ".Nightly");
|
|
|
|
try
|
|
{
|
|
using var release = SingleInstance.Claim(paths, Impatient);
|
|
using var nightly = SingleInstance.Claim(other, Impatient);
|
|
|
|
release.ShouldNotBeNull();
|
|
nightly.ShouldNotBeNull("a second profile is a second application");
|
|
}
|
|
finally
|
|
{
|
|
if (Directory.Exists(other.DataDirectory))
|
|
{
|
|
Directory.Delete(other.DataDirectory, recursive: true);
|
|
}
|
|
}
|
|
}
|
|
}
|