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>
373 lines
16 KiB
C#
373 lines
16 KiB
C#
using System.IO.Pipes;
|
|
using System.Security.Cryptography;
|
|
using System.Text;
|
|
using DodoSSH.Client.Session;
|
|
|
|
namespace DodoSSH.Client.App.Platform;
|
|
|
|
/// <summary>
|
|
/// One desktop instance per profile, and a way for a second launch to raise the first.
|
|
/// </summary>
|
|
/// <remarks>
|
|
/// <para>
|
|
/// <b>Per profile rather than per machine, which decides two things at once.</b> The lock lives in
|
|
/// <see cref="ClientPaths.InstanceLockFile"/>, so a nightly and a release build never exclude each other —
|
|
/// they are separate applications by design, ADR 0013 — and two people signed in to one machine each get
|
|
/// their own instance. A global mutex would have had to re-derive both of those from somewhere.
|
|
/// </para>
|
|
/// <para>
|
|
/// <b>Why one instance at all.</b> The cache is a SQLite file with an outbox in it, and the whole design
|
|
/// assumes one writer: <see cref="ClientPaths"/> says so about two machines sharing a profile through a
|
|
/// cloud sync client, and two processes on one machine is the same hazard with a shorter path to it. The
|
|
/// terminal's loopback listener is the visible half — the second process fails to bind, gets a port of its
|
|
/// own or refuses to start a workspace at all — and the cache is the half that would quietly corrupt.
|
|
/// </para>
|
|
/// <para>
|
|
/// <b>A file handle rather than a named mutex</b>, because the operating system releases it when the
|
|
/// process ends <em>however</em> it ends. A named mutex abandoned by a killed process is a state both
|
|
/// platforms model differently and neither models simply; an exclusive handle is closed by the kernel on
|
|
/// Windows and by the file descriptor going away on Unix, so there is no such thing as a stale lock here.
|
|
/// </para>
|
|
/// <para>
|
|
/// <b>And a named pipe for the other half, because exiting silently is not an answer.</b> Somebody who
|
|
/// double-clicks the icon and sees nothing happen has been told the application is broken. The second
|
|
/// process asks the first to come forward and then leaves, which is what every desktop application does and
|
|
/// what makes the exclusion invisible rather than mysterious.
|
|
/// </para>
|
|
/// </remarks>
|
|
internal static class SingleInstance
|
|
{
|
|
/// <summary>The one thing a second launch has to say.</summary>
|
|
/// <remarks>
|
|
/// Compared against a constant rather than parsed, for the reason <c>MainWindow</c>'s release-focus
|
|
/// message is: one message exists and treating anything else as a command would be a wider door than
|
|
/// this needs. Anything may connect to a per-user pipe — it is not a security boundary — so what arrives
|
|
/// is checked rather than trusted.
|
|
/// </remarks>
|
|
private const string ComeForward = "dodossh.come-forward";
|
|
|
|
/// <summary>How long a second launch waits for the first to answer its pipe.</summary>
|
|
/// <remarks>
|
|
/// Short, because the pipe is local and the process on the other end is either listening or not. It is
|
|
/// not a bound on anything slow; it is how long to wait before concluding that whoever holds the lock
|
|
/// cannot be talked to.
|
|
/// </remarks>
|
|
private const int AnswerTimeoutMilliseconds = 500;
|
|
|
|
/// <summary>How long to keep trying for a lock that is held by nobody who answers.</summary>
|
|
/// <remarks>
|
|
/// <para>
|
|
/// ◆ <b>This is the window an update relaunch lands in, and without it an update would look like a
|
|
/// crash.</b> Velopack's apply ends this process and its updater then starts the new build; the two are
|
|
/// ordered — the updater waits on the old process id — but the whole sequence is one where a lock held a
|
|
/// moment too long would make the relaunched application exit on the spot, having apparently uninstalled
|
|
/// itself. The same window covers an ordinary double-double-click, where the first process holds the file
|
|
/// and has not reached its pipe yet.
|
|
/// </para>
|
|
/// <para>
|
|
/// Only reached when nobody answers the pipe. A live instance replies in milliseconds and the second
|
|
/// launch is gone before any of this matters.
|
|
/// </para>
|
|
/// </remarks>
|
|
private static readonly TimeSpan DefaultPatience = TimeSpan.FromSeconds(3);
|
|
|
|
/// <summary>How long to wait between attempts within that window.</summary>
|
|
private static readonly TimeSpan RetryDelay = TimeSpan.FromMilliseconds(100);
|
|
|
|
/// <summary>
|
|
/// Claims this profile for this process, or asks whoever holds it to come forward.
|
|
/// </summary>
|
|
/// <returns>
|
|
/// A handle to hold for the life of the process, or <see langword="null"/> when another instance has it —
|
|
/// in which case that instance has already been asked to raise its window and this process should end.
|
|
/// </returns>
|
|
/// <param name="paths">The profile to claim. Its directory is created if it is not there.</param>
|
|
/// <param name="patience">
|
|
/// How long to keep trying a lock nobody answers for, or null for <see cref="DefaultPatience"/>. Named
|
|
/// by a test so the give-up path does not cost the suite three seconds; the application never passes it.
|
|
/// </param>
|
|
internal static IDisposable? Claim(ClientPaths paths, TimeSpan? patience = null)
|
|
{
|
|
ArgumentNullException.ThrowIfNull(paths);
|
|
|
|
try
|
|
{
|
|
paths.EnsureCreated();
|
|
}
|
|
catch (Exception exception) when (exception is IOException or UnauthorizedAccessException)
|
|
{
|
|
// No profile directory means no lock file, and every build before this one started anyway. What
|
|
// follows will fail on its own terms and say something better than this could.
|
|
return NoClaim.Instance;
|
|
}
|
|
|
|
// Counted rather than clocked. Thread.Sleep is not a clock and reading one here would be pretending
|
|
// otherwise — what this bounds is the number of attempts, and the wall time is whatever the platform
|
|
// makes of them.
|
|
var attemptsLeft = (int)Math.Ceiling(
|
|
(patience ?? DefaultPatience).TotalMilliseconds / RetryDelay.TotalMilliseconds);
|
|
|
|
while (true)
|
|
{
|
|
if (TryTake(paths.InstanceLockFile) is { } taken)
|
|
{
|
|
return taken;
|
|
}
|
|
|
|
// Asked before waiting, so the ordinary case — an instance that is up and running — costs one
|
|
// round trip on a local pipe rather than the whole window below.
|
|
if (AskTheRunningOneToComeForward(paths))
|
|
{
|
|
return null;
|
|
}
|
|
|
|
if (attemptsLeft-- <= 0)
|
|
{
|
|
// Held by something that will not talk. Giving up rather than starting anyway, because what
|
|
// is being protected is a database with one writer, and "probably nobody" is not an answer
|
|
// worth corrupting it for.
|
|
return null;
|
|
}
|
|
|
|
Thread.Sleep(RetryDelay);
|
|
}
|
|
}
|
|
|
|
/// <summary>
|
|
/// Answers the launches that were refused, for as long as the returned handle is held.
|
|
/// </summary>
|
|
/// <remarks>
|
|
/// <paramref name="comeForward"/> is invoked on a thread pool thread, because that is where a pipe
|
|
/// accept completes. A caller that touches a window has to marshal.
|
|
/// </remarks>
|
|
/// <param name="paths">The profile this instance claimed.</param>
|
|
/// <param name="comeForward">What to do when another launch asks for the window.</param>
|
|
internal static IAsyncDisposable Listen(ClientPaths paths, Action comeForward)
|
|
{
|
|
ArgumentNullException.ThrowIfNull(paths);
|
|
ArgumentNullException.ThrowIfNull(comeForward);
|
|
|
|
var listener = new Doorbell(PipeNameFor(paths), comeForward);
|
|
|
|
listener.Start();
|
|
|
|
return listener;
|
|
}
|
|
|
|
/// <summary>Tells the instance that holds this profile to raise its window.</summary>
|
|
/// <returns>Whether anything was listening.</returns>
|
|
/// <remarks>
|
|
/// Internal rather than private because it is the half a test can drive: the exclusion is a file handle
|
|
/// and proves itself, and this is the part with a protocol in it.
|
|
/// </remarks>
|
|
internal static bool AskTheRunningOneToComeForward(ClientPaths paths)
|
|
{
|
|
ArgumentNullException.ThrowIfNull(paths);
|
|
|
|
try
|
|
{
|
|
using var pipe = new NamedPipeClientStream(
|
|
serverName: ".",
|
|
PipeNameFor(paths),
|
|
PipeDirection.Out);
|
|
|
|
pipe.Connect(AnswerTimeoutMilliseconds);
|
|
|
|
using var writer = new StreamWriter(pipe, Encoding.UTF8) { AutoFlush = true };
|
|
|
|
writer.WriteLine(ComeForward);
|
|
|
|
return true;
|
|
}
|
|
catch (Exception exception)
|
|
when (exception is TimeoutException or IOException or UnauthorizedAccessException)
|
|
{
|
|
// Nothing there, or nothing willing. The caller decides what that means; here it is only ever
|
|
// "no".
|
|
return false;
|
|
}
|
|
}
|
|
|
|
/// <summary>Takes the lock, or answers null if somebody else has it.</summary>
|
|
/// <remarks>
|
|
/// The file is left in place rather than deleted on close. <c>FileOptions.DeleteOnClose</c> looks
|
|
/// tidier and is worse: it makes the file's existence part of the protocol, so a second process opening
|
|
/// it while the first is closing gets a handle to something on its way out. An empty file that is always
|
|
/// there and sometimes locked has one fewer state.
|
|
/// </remarks>
|
|
private static IDisposable? TryTake(string lockFile)
|
|
{
|
|
try
|
|
{
|
|
return new FileStream(
|
|
lockFile,
|
|
FileMode.OpenOrCreate,
|
|
FileAccess.ReadWrite,
|
|
FileShare.None,
|
|
bufferSize: 1,
|
|
FileOptions.None);
|
|
}
|
|
catch (IOException)
|
|
{
|
|
// Somebody else holds it. The one expected failure, and the whole point.
|
|
return null;
|
|
}
|
|
catch (UnauthorizedAccessException)
|
|
{
|
|
// A profile this user cannot write to, which is a broken installation rather than a second
|
|
// instance. Answered as a claim so the application starts and fails on something legible.
|
|
return NoClaim.Instance;
|
|
}
|
|
}
|
|
|
|
/// <summary>
|
|
/// The pipe both sides name, derived from the profile so that it is scoped exactly as the lock is.
|
|
/// </summary>
|
|
/// <remarks>
|
|
/// A digest of the path rather than the path itself: a pipe name may not contain a directory separator
|
|
/// on Windows, and on Unix the name becomes a file in a temporary directory whose length is bounded.
|
|
/// Eight bytes of SHA-256, which is not doing security work — it is making one string out of another
|
|
/// deterministically, and <c>string.GetHashCode</c> cannot because it is randomised per process.
|
|
/// </remarks>
|
|
private static string PipeNameFor(ClientPaths paths)
|
|
{
|
|
var digest = SHA256.HashData(Encoding.UTF8.GetBytes(paths.DataDirectory));
|
|
|
|
return "dodossh-" + Convert.ToHexStringLower(digest.AsSpan(0, 8));
|
|
}
|
|
|
|
/// <summary>A claim over nothing, for the cases where there was nothing to claim.</summary>
|
|
private sealed class NoClaim : IDisposable
|
|
{
|
|
internal static NoClaim Instance { get; } = new();
|
|
|
|
public void Dispose()
|
|
{
|
|
// Nothing was taken.
|
|
}
|
|
}
|
|
|
|
/// <summary>The accept loop that answers refused launches.</summary>
|
|
/// <remarks>
|
|
/// <para>
|
|
/// One server instance for the life of the doorbell, disconnected between callers rather than disposed
|
|
/// and rebuilt. The loop is what makes a third and fourth launch work as well as the second.
|
|
/// </para>
|
|
/// <para>
|
|
/// ◆ <b>THE ONE INSTANCE IS THE WHOLE CORRECTNESS OF THIS CLASS ON LINUX, AND A FRESH ONE PER CALLER
|
|
/// LOSES LAUNCHES.</b> This was written the obvious way — a <c>using</c> inside the loop, a new stream
|
|
/// per connection — which is fine on Windows and quietly wrong on Unix, where a named pipe is a Unix
|
|
/// domain socket. Disposing the server there closes the <em>listening</em> socket and unlinks its path,
|
|
/// so a second launch that connected while the first was being served has its connection dropped on the
|
|
/// floor: its <c>Connect</c> 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.
|
|
/// </para>
|
|
/// <para>
|
|
/// Measured rather than reasoned about, in a container, because the two platforms disagree and this
|
|
/// machine is only one of them: three rings back to back are answered three times with one instance and
|
|
/// once with a fresh one per connection. <c>Disconnect</c> ends the conversation and keeps the socket, so
|
|
/// there is never a moment with nothing listening.
|
|
/// </para>
|
|
/// <para>
|
|
/// Which makes <c>leaveOpen</c> on the reader load-bearing as well. A <see cref="StreamReader"/> disposes
|
|
/// the stream it was given, and the stream is now the server that has to survive into the next
|
|
/// iteration — so the reader that was harmless per-connection would close the doorbell after one ring.
|
|
/// </para>
|
|
/// </remarks>
|
|
private sealed class Doorbell : IAsyncDisposable
|
|
{
|
|
private readonly string name;
|
|
private readonly Action comeForward;
|
|
private readonly CancellationTokenSource lifetime = new();
|
|
|
|
private Task? loop;
|
|
|
|
internal Doorbell(string name, Action comeForward)
|
|
{
|
|
this.name = name;
|
|
this.comeForward = comeForward;
|
|
}
|
|
|
|
internal void Start() => loop = RunAsync(lifetime.Token);
|
|
|
|
/// <inheritdoc />
|
|
public async ValueTask DisposeAsync()
|
|
{
|
|
await lifetime.CancelAsync().ConfigureAwait(false);
|
|
|
|
if (loop is { } running)
|
|
{
|
|
// Awaited rather than abandoned, so the pipe name is free by the time this returns — which
|
|
// matters for exactly one caller: an update that is about to relaunch this application and
|
|
// would otherwise race its own successor for the name.
|
|
await running.ConfigureAwait(false);
|
|
}
|
|
|
|
lifetime.Dispose();
|
|
}
|
|
|
|
private async Task RunAsync(CancellationToken cancellationToken)
|
|
{
|
|
using var server = new NamedPipeServerStream(
|
|
name,
|
|
PipeDirection.In,
|
|
maxNumberOfServerInstances: 1,
|
|
PipeTransmissionMode.Byte,
|
|
PipeOptions.Asynchronous);
|
|
|
|
while (!cancellationToken.IsCancellationRequested)
|
|
{
|
|
try
|
|
{
|
|
await server.WaitForConnectionAsync(cancellationToken).ConfigureAwait(false);
|
|
|
|
// leaveOpen, because this server outlives the conversation. See the remark above.
|
|
using (var reader = new StreamReader(server, Encoding.UTF8, false, 1024, leaveOpen: true))
|
|
{
|
|
var said = await reader.ReadLineAsync(cancellationToken).ConfigureAwait(false);
|
|
|
|
if (string.Equals(said, ComeForward, StringComparison.Ordinal))
|
|
{
|
|
comeForward();
|
|
}
|
|
}
|
|
|
|
HangUp(server);
|
|
}
|
|
catch (OperationCanceledException)
|
|
{
|
|
return;
|
|
}
|
|
catch (IOException)
|
|
{
|
|
// A client that connected and hung up mid-sentence. Round again: giving up here would
|
|
// leave every later launch with nothing to talk to, which is the silent-exit behaviour
|
|
// this class exists to avoid.
|
|
HangUp(server);
|
|
}
|
|
}
|
|
}
|
|
|
|
/// <summary>Ends one conversation without ending the doorbell.</summary>
|
|
/// <remarks>
|
|
/// Guarded, because <c>Disconnect</c> throws on a server that is not connected — which is the state
|
|
/// an <see cref="IOException"/> above may already have left it in, and the state a second call would
|
|
/// find. Throwing out of the error path would take the loop with it.
|
|
/// </remarks>
|
|
private static void HangUp(NamedPipeServerStream server)
|
|
{
|
|
try
|
|
{
|
|
if (server.IsConnected)
|
|
{
|
|
server.Disconnect();
|
|
}
|
|
}
|
|
catch (InvalidOperationException)
|
|
{
|
|
// Already disconnected. Nothing to end.
|
|
}
|
|
}
|
|
}
|
|
}
|