Public Access
Merge pull request 'Stop the SSH suite's server refusing connections at random' (#4) from claude/ssh-fixture-hup-race into main
Reviewed-on: #4
This commit was merged in pull request #4.
This commit is contained in:
@@ -388,6 +388,19 @@ agent of our own plus ProxyJump covers the real use cases.
|
|||||||
**The SSH suite pulls `linuxserver/openssh-server` from Docker Hub**, which is rate-limited for
|
**The SSH suite pulls `linuxserver/openssh-server` from Docker Hub**, which is rate-limited for
|
||||||
unauthenticated pulls. If CI starts failing on image pulls rather than on tests, that is why.
|
unauthenticated pulls. If CI starts failing on image pulls rather than on tests, that is why.
|
||||||
|
|
||||||
|
**That suite has an intermittent `The connection was closed by the remote host`**, on whichever test
|
||||||
|
connects first, within tens of milliseconds. Seen in CI and reproducible locally. *Mitigated, not
|
||||||
|
solved:* `SshServerFixture` now raises sshd's `MaxStartups` from its compiled-in `10:30:100`, which
|
||||||
|
refuses connections at random past ten unauthenticated ones in flight — reachable because xUnit runs
|
||||||
|
test classes in parallel and most of them connect. The fixture comment carries the full argument and
|
||||||
|
is explicit that the cure is unproven.
|
||||||
|
|
||||||
|
**And the reason it is unproven is a measurement trap worth not falling into twice.** Docker
|
||||||
|
throughput on the Windows development machine swings enough to swamp the effect: the identical
|
||||||
|
unmodified suite ran 85/85 clean and, an hour later, failed 13 runs out of 15. Any before/after flake
|
||||||
|
comparison taken there is noise. Measure this class of thing in CI, or make the server say why —
|
||||||
|
raise sshd's `LogLevel`, disable Ryuk so the container outlives the run, and read `docker logs`.
|
||||||
|
|
||||||
**MSIX packaging is ruled out, not merely deprioritised.** A packaged app runs WebView2 in an
|
**MSIX packaging is ruled out, not merely deprioritised.** A packaged app runs WebView2 in an
|
||||||
AppContainer where loopback connections are blocked without a `CheckNetIsolation` exemption. The
|
AppContainer where loopback connections are blocked without a `CheckNetIsolation` exemption. The
|
||||||
terminal data plane *is* a loopback WebSocket, so MSIX would break the product outright. Velopack
|
terminal data plane *is* a loopback WebSocket, so MSIX would break the product outright. Velopack
|
||||||
|
|||||||
@@ -112,6 +112,61 @@ public sealed class SshServerFixture : IAsyncLifetime
|
|||||||
/// this image as hardening, not as a behaviour worth reproducing: nothing else here opens a channel of
|
/// this image as hardening, not as a behaviour worth reproducing: nothing else here opens a channel of
|
||||||
/// any kind, so allowing it changes what exactly one suite can do and what none of the others see.
|
/// any kind, so allowing it changes what exactly one suite can do and what none of the others see.
|
||||||
/// </para>
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// ◆ <b><c>MaxStartups</c> is raised here too, against a flake this suite has and that this change is
|
||||||
|
/// a mitigation for rather than a proven cure.</b> The distinction is stated because the evidence
|
||||||
|
/// stops short of the claim, and a later reader deserves to know which.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// What is established: sshd's compiled-in default is <c>10:30:100</c> — past ten
|
||||||
|
/// <em>unauthenticated</em> connections in flight it refuses new ones at random, thirty percent of the
|
||||||
|
/// time, rising to always at a hundred — and the image ships the line commented out, so that default
|
||||||
|
/// was what ran. xUnit runs test classes in parallel and most classes here open a connection, so ten
|
||||||
|
/// in flight is reachable in the opening seconds. A refused connection presents to the client as
|
||||||
|
/// <c>SshConnectionException: The connection was closed by the remote host</c> within tens of
|
||||||
|
/// milliseconds, on whichever test connects at the wrong moment — which is exactly the observed
|
||||||
|
/// failure, seen in CI and reproduced locally.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// What is <em>not</em> established is that this limit is the only cause, because the flake rate could
|
||||||
|
/// not be measured reliably. On the development machine the identical unmodified suite ran 85/85 clean
|
||||||
|
/// and, an hour later, failed 13 runs out of 15 — Docker throughput on that host swings far enough to
|
||||||
|
/// swamp the effect being measured. Any before/after comparison taken there is noise, and two were,
|
||||||
|
/// before that was noticed.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// It is committed anyway, on the narrower argument that it is right regardless: a connection throttle
|
||||||
|
/// is hardening this suite has no interest in reproducing. It exists to test an SSH client, not to
|
||||||
|
/// survive a rate limit, and a test server that drops connections at random is a bad test server
|
||||||
|
/// whether or not it is the cause of this particular flake.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// <b>Not fixed by serialising the suite</b>, which would have hidden it and cost the parallelism, and
|
||||||
|
/// not by retrying the connect, which would have made the client's own reconnect behaviour untestable
|
||||||
|
/// by burying it in the fixture. The limit is a property of a hardened server that this suite has no
|
||||||
|
/// interest in reproducing — it exists to test an SSH client, not to survive a throttle.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// Replaced in place rather than appended, because sshd_config takes the <em>first</em> value it finds
|
||||||
|
/// for a keyword: an appended line would be dead the day the image ships an uncommented one of its own.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// ◆ <b>The reload window is the other candidate, and it is deliberately not guarded against.</b>
|
||||||
|
/// <c>SIGHUP</c> makes sshd close its listeners and re-execute itself, and <c>pkill</c> returns when
|
||||||
|
/// the signal is delivered rather than when that has finished — so in principle a connection made
|
||||||
|
/// immediately afterwards is refused, producing this same exception. A wait that opened connections
|
||||||
|
/// until the server answered with its banner three times running was written, and then removed: it
|
||||||
|
/// could not be shown to change anything either, and a fixture carrying two unproven fixes for one
|
||||||
|
/// symptom is worse than one, because the next person has to disprove both.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// If this flake returns, that is the next thing to try. Two things to know before trying it: the two
|
||||||
|
/// causes are indistinguishable from the client, so a fix can only be judged by a repeat run and never
|
||||||
|
/// by whether the next run passes — and the repeat run has to happen somewhere with stable Docker
|
||||||
|
/// throughput, which the development machine is not. Better still, make sshd say why: raise its
|
||||||
|
/// <c>LogLevel</c> here, disable Ryuk so the container outlives the run, and read
|
||||||
|
/// <c>docker logs</c>. A <c>MaxStartups</c> refusal names itself there; a reload does not.
|
||||||
|
/// </para>
|
||||||
/// </remarks>
|
/// </remarks>
|
||||||
private async Task AllowTcpForwardingAsync()
|
private async Task AllowTcpForwardingAsync()
|
||||||
{
|
{
|
||||||
@@ -119,14 +174,16 @@ public sealed class SshServerFixture : IAsyncLifetime
|
|||||||
"sh",
|
"sh",
|
||||||
"-c",
|
"-c",
|
||||||
"sed -i 's/^AllowTcpForwarding no/AllowTcpForwarding yes/' /config/sshd/sshd_config"
|
"sed -i 's/^AllowTcpForwarding no/AllowTcpForwarding yes/' /config/sshd/sshd_config"
|
||||||
|
+ " && sed -i 's/^#*MaxStartups .*/MaxStartups 200/' /config/sshd/sshd_config"
|
||||||
+ " && pkill -HUP sshd",
|
+ " && pkill -HUP sshd",
|
||||||
]);
|
]);
|
||||||
|
|
||||||
if (result.ExitCode != 0)
|
if (result.ExitCode != 0)
|
||||||
{
|
{
|
||||||
throw new InvalidOperationException(
|
throw new InvalidOperationException(
|
||||||
$"Could not enable TCP forwarding on the test server: {result.Stderr}");
|
$"Could not reconfigure the test server: {result.Stderr}");
|
||||||
}
|
}
|
||||||
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/// <summary>
|
/// <summary>
|
||||||
|
|||||||
Reference in New Issue
Block a user