From dabc691c3530442fe474e06e99a8a15a8e8ab054 Mon Sep 17 00:00:00 2001 From: Laurens Bezemer | DodoTech Date: Wed, 26 Aug 2026 15:42:41 +0200 Subject: [PATCH] Keep the doorbell listening between callers, and stop a hung test taking CI with it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .github/workflows/ci.yml | 23 +++++ .../Platform/SingleInstance.cs | 85 ++++++++++++++----- .../SingleInstanceTests.cs | 35 ++++++-- 3 files changed, 116 insertions(+), 27 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7ccc0a9..862f7ce 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/src/DodoSSH.Client.App/Platform/SingleInstance.cs b/src/DodoSSH.Client.App/Platform/SingleInstance.cs index 4ee2019..89935ce 100644 --- a/src/DodoSSH.Client.App/Platform/SingleInstance.cs +++ b/src/DodoSSH.Client.App/Platform/SingleInstance.cs @@ -249,9 +249,30 @@ internal static class SingleInstance /// The accept loop that answers refused launches. /// - /// One server instance at a time and a fresh stream per connection, which is the shape - /// 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. + /// + /// 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. + /// + /// + /// ◆ THE ONE INSTANCE IS THE WHOLE CORRECTNESS OF THIS CLASS ON LINUX, AND A FRESH ONE PER CALLER + /// LOSES LAUNCHES. This was written the obvious way — a using 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 listening 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 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. + /// + /// + /// 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. Disconnect ends the conversation and keeps the socket, so + /// there is never a moment with nothing listening. + /// + /// + /// Which makes leaveOpen on the reader load-bearing as well. A 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. + /// /// 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); } } } + + /// Ends one conversation without ending the doorbell. + /// + /// Guarded, because Disconnect throws on a server that is not connected — which is the state + /// an 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. + /// + private static void HangUp(NamedPipeServerStream server) + { + try + { + if (server.IsConnected) + { + server.Disconnect(); + } + } + catch (InvalidOperationException) + { + // Already disconnected. Nothing to end. + } + } } } diff --git a/tests/DodoSSH.Client.App.Tests/SingleInstanceTests.cs b/tests/DodoSSH.Client.App.Tests/SingleInstanceTests.cs index 9f0ebb1..6fa25a6 100644 --- a/tests/DodoSSH.Client.App.Tests/SingleInstanceTests.cs +++ b/tests/DodoSSH.Client.App.Tests/SingleInstanceTests.cs @@ -42,6 +42,19 @@ public sealed class SingleInstanceTests : IDisposable /// private static readonly TimeSpan Impatient = TimeSpan.Zero; + /// + /// How long a doorbell is given to ring before the test says it never will. + /// + /// + /// ◆ A bound at all, which is the lesson rather than the number. These two awaits were written + /// against TestContext.Current.CancellationToken alone, which carries no timeout — so the run + /// where a ring was genuinely lost did not fail, it hung, 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. + /// + private static readonly TimeSpan LongEnoughForAPipe = TimeSpan.FromSeconds(20); + /// 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); } /// - /// A third and a fourth launch work as well as the second, which is what the accept loop is for: a - /// NamedPipeServerStream 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. + /// + /// 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 NamedPipeServerStream 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 SingleInstance.Doorbell. + /// + /// + /// 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. + /// /// [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); } ///