Keep the doorbell listening between callers, and stop a hung test taking CI with it

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>

.github/workflows/ci.yml

@@ -29,6 +29,18 @@ jobs:
   build:
     name: build and test
     runs-on: [linux]
+    # ◆ A CEILING, BECAUSE THIS FILE HAD NONE AND A HUNG TEST RAN UNTIL SOMEBODY NOTICED.
+    #
+    # One test in DodoSSH.Client.App.Tests waited on a named pipe that, on Linux, nothing was ever going
+    # to answer — and it waited on a token with no timeout, so the job did not fail, it sat there. It was
+    # killed by hand after twenty minutes, having reported nothing but the eighteen suites that had
+    # already passed.
+    #
+    # Twenty-five minutes against a job that ordinarily takes three. It is not a performance budget and
+    # must not be read as one: a build that legitimately grows past this should have the number raised.
+    # What it buys is that the failure mode is a red job with a named step rather than a runner somebody
+    # has to go and stop.
+    timeout-minutes: 25
     steps:
       # act_runner runs every `uses:` action with node inside the job container, and this
       # runner's image has none — the run died on the first line of actions/checkout with
@@ -220,7 +232,12 @@ jobs:
       - name: build
         run: dotnet build DodoSSH.slnx --no-restore --configuration Release
 
+      # Tighter than the job's own ceiling, and deliberately so: a hang here is the likeliest of the
+      # lot — it is the only step running code written for this repository rather than a tool — and a
+      # step-level timeout names the step in the failure instead of leaving a reader to work out which
+      # of fifteen was still running. Ten minutes against a suite that takes two.
       - name: test
+        timeout-minutes: 10
         run: dotnet test DodoSSH.slnx --no-build --configuration Release
 
       # The one build shape nothing else here exercises: a self-contained RID-specific publish. Its
@@ -556,6 +573,8 @@ jobs:
           exit 0
   android:
     name: android head
+    # See the build job. The docker pull and the toolchain image make this one legitimately slower.
+    timeout-minutes: 40
     runs-on: [linux]
     # Writes the nightly release at the end of the job; see that step for what the capability is and why
     # it is acceptable here and nowhere else. Job-scoped, so nothing else in this file gains it.
@@ -929,6 +948,8 @@ jobs:
 
   desktop:
     name: desktop nightly
+    # See the build job. Two publishes, a pack and an upload of about 120 MB.
+    timeout-minutes: 40
     # Gated on the tests, like the image job and for a stronger version of its reason: this one is
     # installed by people and replaces itself afterwards. Nothing anybody runs should come out of a
     # commit whose suite was red.
@@ -1297,6 +1318,8 @@ jobs:
 
   image:
     name: api image
+    # See the build job. A cold layer cache is the slow case here.
+    timeout-minutes: 40
     # Gated on the tests rather than parallel with them, which costs a few minutes of wall
     # clock on every main commit and buys the thing worth having: no image reaches the
     # registry from a commit whose tests were red. An image is not a build artefact anyone

src/DodoSSH.Client.App/Platform/SingleInstance.cs

@@ -249,9 +249,30 @@ internal static class SingleInstance
 
     /// <summary>The accept loop that answers refused launches.</summary>
     /// <remarks>
-    /// One server instance at a time and a fresh stream per connection, which is the shape
-    /// <see cref="NamedPipeServerStream"/> wants: a stream that has served a client cannot serve another.
-    /// The loop is what makes a third and fourth launch work as well as the second.
+    /// <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
     {
@@ -287,27 +308,31 @@ internal static class SingleInstance
 
         private async Task RunAsync(CancellationToken cancellationToken)
         {
+            using var server = new NamedPipeServerStream(
+                name,
+                PipeDirection.In,
+                maxNumberOfServerInstances: 1,
+                PipeTransmissionMode.Byte,
+                PipeOptions.Asynchronous);
+
             while (!cancellationToken.IsCancellationRequested)
             {
                 try
                 {
-                    using var server = new NamedPipeServerStream(
-                        name,
-                        PipeDirection.In,
-                        maxNumberOfServerInstances: 1,
-                        PipeTransmissionMode.Byte,
-                        PipeOptions.Asynchronous);
-
                     await server.WaitForConnectionAsync(cancellationToken).ConfigureAwait(false);
 
-                    using var reader = new StreamReader(server, Encoding.UTF8);
-
-                    var said = await reader.ReadLineAsync(cancellationToken).ConfigureAwait(false);
-
-                    if (string.Equals(said, ComeForward, StringComparison.Ordinal))
+                    // leaveOpen, because this server outlives the conversation. See the remark above.
+                    using (var reader = new StreamReader(server, Encoding.UTF8, false, 1024, leaveOpen: true))
                     {
-                        comeForward();
+                        var said = await reader.ReadLineAsync(cancellationToken).ConfigureAwait(false);
+
+                        if (string.Equals(said, ComeForward, StringComparison.Ordinal))
+                        {
+                            comeForward();
+                        }
                     }
+
+                    HangUp(server);
                 }
                 catch (OperationCanceledException)
                 {
@@ -315,11 +340,33 @@ internal static class SingleInstance
                 }
                 catch (IOException)
                 {
-                    // A client that connected and hung up, or a name the platform took away. 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.
+                    // 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.
+            }
+        }
     }
 }

tests/DodoSSH.Client.App.Tests/SingleInstanceTests.cs

@@ -42,6 +42,19 @@ public sealed class SingleInstanceTests : IDisposable
     /// </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()
     {
@@ -106,16 +119,22 @@ public sealed class SingleInstanceTests : IDisposable
 
         SingleInstance.Claim(paths, Impatient).ShouldBeNull("the profile is taken");
 
-        // Awaited rather than polled: the callback comes off a thread pool thread, and the test's own
-        // cancellation is what bounds a doorbell that never rings.
-        await asked.Task.WaitAsync(TestContext.Current.CancellationToken);
+        // 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>
-    /// A third and a fourth launch work as well as the second, which is what the accept loop is for: a
-    /// <c>NamedPipeServerStream</c> that has served one client cannot serve another, so a doorbell that
-    /// forgot to go round again would answer exactly one refused launch and then leave every later one to
-    /// exit in silence.
+    /// <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()
@@ -139,7 +158,7 @@ public sealed class SingleInstanceTests : IDisposable
         SingleInstance.AskTheRunningOneToComeForward(paths).ShouldBeTrue();
         SingleInstance.AskTheRunningOneToComeForward(paths).ShouldBeTrue();
 
-        await twice.Task.WaitAsync(TestContext.Current.CancellationToken);
+        await twice.Task.WaitAsync(LongEnoughForAPipe, TestContext.Current.CancellationToken);
     }
 
     /// <remarks>