Public Access
Merge branch 'claude/distracted-ritchie-53fc70'
Bounds the renderer wait, so a WebView2 that never initialises reports itself instead of hanging Connect with the busy flag stuck. Conflict resolution, all of it in the App test suite, which main had changed under the branch when sleepy-chebyshev landed: - The workspace fixture keeps main's fake SSH factory and its FakeRenderer-aware page, and takes the branch's RendererTimeout on top. One second rather than the branch's 250 ms, because the timeout now also bounds FakeRenderer's own wait for the attach it just made. - FakeRenderer arrived on main after the branch was cut and still called the no-argument WaitForRendererAsync. Both sides merged cleanly and left the build broken; it now passes its own token. - ConnectingWithNoRenderer's remark claimed the suite never starts the workspace and never attaches a renderer. Both are false here, so it now says what is true of the test: it is the one connect test that attaches no renderer.
This commit is contained in:
+13
-4
@@ -90,10 +90,19 @@ the case it was attached to. A process-level check cannot verify a rendering cla
|
|||||||
screenshot, and this defect shipped because one was never taken.
|
screenshot, and this defect shipped because one was never taken.
|
||||||
|
|
||||||
**What the first connection after unlocking actually depends on** is the `await
|
**What the first connection after unlocking actually depends on** is the `await
|
||||||
workspace.WaitForRendererAsync()` in `VaultViewModel.ConnectAsync`, because `TerminalDataPlane.SendAsync`
|
workspace.WaitForRendererAsync(cancellationToken)` in `VaultViewModel.ConnectAsync`, because
|
||||||
drops frames when no renderer is attached rather than queueing them. That await is the invariant; the
|
`TerminalDataPlane.SendAsync` drops frames when no renderer is attached rather than queueing them. That
|
||||||
control's visibility is not. It currently has no timeout, so a WebView2 that fails to initialise hangs
|
await is the invariant; the control's visibility is not.
|
||||||
Connect with the busy flag stuck — worth fixing on its own merits.
|
|
||||||
|
It is now bounded — `TerminalWorkspaceOptions.RendererTimeout`, 15 s, plus the command's own token —
|
||||||
|
because whether the renderer attaches at all depends on a runtime this application does not install. A
|
||||||
|
missing or policy-blocked Evergreen runtime, or an AppContainer that cannot reach loopback, previously
|
||||||
|
left Connect waiting forever with `IsBusy` stuck and nothing on screen to explain it. The gate is
|
||||||
|
unchanged; only the wait is. Why 15 s and not less: attaching is near-instant in the normal case (the page
|
||||||
|
attaches while the unlock screen is still up), but a cold WebView2 profile creates a user-data directory
|
||||||
|
and starts its process tree first, and reporting a broken runtime to someone whose runtime was merely slow
|
||||||
|
is the worse error. The timeout is caught in `VaultViewModel` and reported as a message naming WebView2,
|
||||||
|
because `TimeoutException.Message` is "The operation has timed out" and names nothing.
|
||||||
|
|
||||||
**Hiding the WebView does not pause it.** With the holder window hidden, the page keeps
|
**Hiding the WebView does not pause it.** With the holder window hidden, the page keeps
|
||||||
`visibilityState: "visible"` and `requestAnimationFrame` keeps firing at roughly 115/s — Chromium does not
|
`visibilityState: "visible"` and `requestAnimationFrame` keeps firing at roughly 115/s — Chromium does not
|
||||||
|
|||||||
@@ -604,13 +604,14 @@ internal sealed partial class VaultViewModel(
|
|||||||
/// <remarks>
|
/// <remarks>
|
||||||
/// The renderer has to be attached before a session opens: the transport drops frames when nothing is
|
/// The renderer has to be attached before a session opens: the transport drops frames when nothing is
|
||||||
/// connected, so a session opened earlier would lose its <c>SessionOpened</c> frame and then stream
|
/// connected, so a session opened earlier would lose its <c>SessionOpened</c> frame and then stream
|
||||||
/// output at a terminal that was never created.
|
/// output at a terminal that was never created. That wait is bounded and takes this command's token, so
|
||||||
|
/// a renderer that never arrives ends as a message rather than as a window stuck on "Connecting…".
|
||||||
/// </remarks>
|
/// </remarks>
|
||||||
private async Task OpenSessionAsync(HostRowViewModel row, CancellationToken cancellationToken)
|
private async Task OpenSessionAsync(HostRowViewModel row, CancellationToken cancellationToken)
|
||||||
{
|
{
|
||||||
try
|
try
|
||||||
{
|
{
|
||||||
await workspace.WaitForRendererAsync().ConfigureAwait(true);
|
await workspace.WaitForRendererAsync(cancellationToken).ConfigureAwait(true);
|
||||||
|
|
||||||
var request = new SshConnectionRequest(
|
var request = new SshConnectionRequest(
|
||||||
row.Host.Hostname,
|
row.Host.Hostname,
|
||||||
@@ -630,6 +631,14 @@ internal sealed partial class VaultViewModel(
|
|||||||
// UI instead of the remote shell.
|
// UI instead of the remote shell.
|
||||||
SessionOpened?.Invoke(this, EventArgs.Empty);
|
SessionOpened?.Invoke(this, EventArgs.Empty);
|
||||||
}
|
}
|
||||||
|
catch (TimeoutException)
|
||||||
|
{
|
||||||
|
// The renderer never attached, so nothing was connected. Reported here rather than left to
|
||||||
|
// RunAsync's generic handler because TimeoutException says only "The operation has timed out",
|
||||||
|
// and the one thing worth saying is where to look: a runtime this application does not install.
|
||||||
|
Status = "The terminal did not start, so nothing was connected. The Microsoft Edge WebView2 "
|
||||||
|
+ "runtime is probably missing or blocked; install it and try again.";
|
||||||
|
}
|
||||||
catch (SshHostKeyUnknownException exception)
|
catch (SshHostKeyUnknownException exception)
|
||||||
{
|
{
|
||||||
// First contact. The user has to decide, and they need the fingerprint to do it.
|
// First contact. The user has to decide, and they need the fingerprint to do it.
|
||||||
|
|||||||
@@ -2,6 +2,31 @@ using DodoSSH.Client.Ssh;
|
|||||||
|
|
||||||
namespace DodoSSH.Client.Terminal;
|
namespace DodoSSH.Client.Terminal;
|
||||||
|
|
||||||
|
/// <summary>Tuning for the workspace.</summary>
|
||||||
|
public sealed class TerminalWorkspaceOptions
|
||||||
|
{
|
||||||
|
/// <summary>
|
||||||
|
/// How long <see cref="TerminalWorkspace.WaitForRendererAsync"/> waits for the renderer's socket
|
||||||
|
/// before giving up.
|
||||||
|
/// </summary>
|
||||||
|
/// <remarks>
|
||||||
|
/// <para>
|
||||||
|
/// The value has to separate two cases. Attaching is normally near-instant: WebView2 starts with the
|
||||||
|
/// window and the page has usually attached while the user was still typing a passphrase. But a first
|
||||||
|
/// run on a cold profile creates a user-data directory and starts a process tree of some thirty-five
|
||||||
|
/// processes, and on a slow or loaded machine that is seconds rather than milliseconds. A renderer
|
||||||
|
/// that will never attach — no Evergreen runtime, an install blocked by policy, an AppContainer that
|
||||||
|
/// cannot reach loopback — will not attach however long the wait is.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// So being generous costs only how long a genuinely broken WebView2 takes to say so, while being
|
||||||
|
/// tight costs telling someone their runtime is broken when it was merely slow. Fifteen seconds is
|
||||||
|
/// well clear of any cold start observed here and is still an answer rather than a hang.
|
||||||
|
/// </para>
|
||||||
|
/// </remarks>
|
||||||
|
public TimeSpan RendererTimeout { get; init; } = TimeSpan.FromSeconds(15);
|
||||||
|
}
|
||||||
|
|
||||||
/// <summary>
|
/// <summary>
|
||||||
/// Owns the loopback data plane and every live terminal session.
|
/// Owns the loopback data plane and every live terminal session.
|
||||||
/// </summary>
|
/// </summary>
|
||||||
@@ -16,6 +41,7 @@ public sealed class TerminalWorkspace : IAsyncDisposable
|
|||||||
private readonly TerminalDataPlane dataPlane;
|
private readonly TerminalDataPlane dataPlane;
|
||||||
private readonly ISshConnectionFactory connections;
|
private readonly ISshConnectionFactory connections;
|
||||||
private readonly TimeProvider clock;
|
private readonly TimeProvider clock;
|
||||||
|
private readonly TerminalWorkspaceOptions options;
|
||||||
private readonly Dictionary<uint, LiveSession> sessions = [];
|
private readonly Dictionary<uint, LiveSession> sessions = [];
|
||||||
private readonly CancellationTokenSource lifetime = new();
|
private readonly CancellationTokenSource lifetime = new();
|
||||||
|
|
||||||
@@ -23,13 +49,19 @@ public sealed class TerminalWorkspace : IAsyncDisposable
|
|||||||
private Task? server;
|
private Task? server;
|
||||||
private int disposed;
|
private int disposed;
|
||||||
|
|
||||||
|
/// <param name="assets">Where the renderer's files come from.</param>
|
||||||
|
/// <param name="connections">How SSH connections are made.</param>
|
||||||
|
/// <param name="clock">Time source, so the pumps' flush interval is testable.</param>
|
||||||
|
/// <param name="options">Tuning, or null for the defaults.</param>
|
||||||
public TerminalWorkspace(
|
public TerminalWorkspace(
|
||||||
ITerminalAssetProvider assets,
|
ITerminalAssetProvider assets,
|
||||||
ISshConnectionFactory connections,
|
ISshConnectionFactory connections,
|
||||||
TimeProvider clock)
|
TimeProvider clock,
|
||||||
|
TerminalWorkspaceOptions? options = null)
|
||||||
{
|
{
|
||||||
this.connections = connections;
|
this.connections = connections;
|
||||||
this.clock = clock;
|
this.clock = clock;
|
||||||
|
this.options = options ?? new TerminalWorkspaceOptions();
|
||||||
|
|
||||||
dataPlane = new TerminalDataPlane(assets);
|
dataPlane = new TerminalDataPlane(assets);
|
||||||
}
|
}
|
||||||
@@ -44,11 +76,25 @@ public sealed class TerminalWorkspace : IAsyncDisposable
|
|||||||
/// Waits until the renderer page has attached its socket.
|
/// Waits until the renderer page has attached its socket.
|
||||||
/// </summary>
|
/// </summary>
|
||||||
/// <remarks>
|
/// <remarks>
|
||||||
/// A session opened before the renderer attaches would have its <c>SessionOpened</c> frame
|
/// <para>
|
||||||
/// dropped — the transport discards frames when nothing is connected — leaving output arriving
|
/// A session opened before the renderer attaches would have its <c>SessionOpened</c> frame dropped —
|
||||||
/// for a terminal that was never created.
|
/// the transport discards frames when nothing is connected — leaving output arriving for a terminal
|
||||||
|
/// that was never created. The gate is the invariant and stays.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// Bounded, because whether the renderer attaches at all depends on a WebView2 runtime this process
|
||||||
|
/// does not install. An unbounded wait turned a missing runtime into a Connect that never returned,
|
||||||
|
/// with the caller's busy state never cleared and nothing on screen to explain it. Callers are
|
||||||
|
/// expected to translate the timeout into something that names the runtime, because
|
||||||
|
/// <see cref="TimeoutException"/>'s own message names nothing.
|
||||||
|
/// </para>
|
||||||
/// </remarks>
|
/// </remarks>
|
||||||
public Task WaitForRendererAsync() => dataPlane.RendererAttached;
|
/// <param name="cancellationToken">Abandons the wait.</param>
|
||||||
|
/// <exception cref="TimeoutException">
|
||||||
|
/// No renderer attached within <see cref="TerminalWorkspaceOptions.RendererTimeout"/>.
|
||||||
|
/// </exception>
|
||||||
|
public Task WaitForRendererAsync(CancellationToken cancellationToken) =>
|
||||||
|
dataPlane.RendererAttached.WaitAsync(options.RendererTimeout, cancellationToken);
|
||||||
|
|
||||||
/// <summary>Connects to a host and starts a terminal for it.</summary>
|
/// <summary>Connects to a host and starts a terminal for it.</summary>
|
||||||
/// <returns>The session id, which identifies this terminal in the renderer.</returns>
|
/// <returns>The session id, which identifies this terminal in the renderer.</returns>
|
||||||
|
|||||||
@@ -66,7 +66,7 @@ internal sealed partial class FakeRenderer : IAsyncDisposable
|
|||||||
|
|
||||||
var renderer = new FakeRenderer(attached);
|
var renderer = new FakeRenderer(attached);
|
||||||
|
|
||||||
await workspace.WaitForRendererAsync().ConfigureAwait(false);
|
await workspace.WaitForRendererAsync(cancellationToken).ConfigureAwait(false);
|
||||||
|
|
||||||
return renderer;
|
return renderer;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -61,6 +61,12 @@ public sealed class ShellFlowTests : IAsyncLifetime
|
|||||||
//
|
//
|
||||||
// The page carries the same two placeholders the real one does, because FakeRenderer attaches by
|
// The page carries the same two placeholders the real one does, because FakeRenderer attaches by
|
||||||
// reading them back out of the served page rather than by being handed the token.
|
// reading them back out of the served page rather than by being handed the token.
|
||||||
|
//
|
||||||
|
// The renderer timeout is cut right down for the same reason: the tests that want a renderer attach
|
||||||
|
// one themselves, so a connect in a test that does not would wait the shipped fifteen seconds out
|
||||||
|
// in full, and that is fifteen seconds of a suite sitting still. A second rather than milliseconds
|
||||||
|
// because this bounds FakeRenderer's own wait too — an in-process loopback handshake that has
|
||||||
|
// already returned, so the margin is enormous, but not one worth making a loaded machine race for.
|
||||||
workspace = new TerminalWorkspace(
|
workspace = new TerminalWorkspace(
|
||||||
new InMemoryTerminalAssetProvider(
|
new InMemoryTerminalAssetProvider(
|
||||||
new Dictionary<string, TerminalAsset>(StringComparer.Ordinal)
|
new Dictionary<string, TerminalAsset>(StringComparer.Ordinal)
|
||||||
@@ -72,7 +78,8 @@ public sealed class ShellFlowTests : IAsyncLifetime
|
|||||||
+ $"data-socket=\"{TerminalDataPlane.SocketUrlPlaceholder}\"></div>")),
|
+ $"data-socket=\"{TerminalDataPlane.SocketUrlPlaceholder}\"></div>")),
|
||||||
}),
|
}),
|
||||||
ssh,
|
ssh,
|
||||||
TimeProvider.System);
|
TimeProvider.System,
|
||||||
|
new TerminalWorkspaceOptions { RendererTimeout = TimeSpan.FromSeconds(1) });
|
||||||
|
|
||||||
// Started, as the application does immediately after composing it. Without the accept loop the
|
// Started, as the application does immediately after composing it. Without the accept loop the
|
||||||
// page is never served, so nothing could attach a renderer.
|
// page is never served, so nothing could attach a renderer.
|
||||||
@@ -597,6 +604,30 @@ public sealed class ShellFlowTests : IAsyncLifetime
|
|||||||
server.PushCount.ShouldBe(0);
|
server.PushCount.ShouldBe(0);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// <remarks>
|
||||||
|
/// The one connect test that deliberately attaches no <see cref="FakeRenderer"/>: the listener is up
|
||||||
|
/// and nothing ever connects to it, which from the view model's side is indistinguishable from a
|
||||||
|
/// WebView2 that failed to initialise on a user's machine. The interesting assertion is the second
|
||||||
|
/// one: while the wait was unbounded this hung with the busy flag set, so the window stayed disabled
|
||||||
|
/// and said "Connecting…" for the rest of the session.
|
||||||
|
/// </remarks>
|
||||||
|
[Fact]
|
||||||
|
public async Task ConnectingWithNoRenderer_ExplainsItselfAndReleasesTheWindow()
|
||||||
|
{
|
||||||
|
await UnlockedAsync();
|
||||||
|
var vault = shell.Vault!;
|
||||||
|
|
||||||
|
await AddHostAsync(vault, "prod-db");
|
||||||
|
vault.SelectedHost = vault.Hosts[0];
|
||||||
|
|
||||||
|
await vault.ConnectCommand.ExecuteAsync(null);
|
||||||
|
|
||||||
|
// Naming the runtime is the whole point: a bare "The operation has timed out" sends someone
|
||||||
|
// looking at their network or their host.
|
||||||
|
vault.Status.ShouldContain("WebView2");
|
||||||
|
vault.IsBusy.ShouldBeFalse("a connect that gave up must not leave the window disabled");
|
||||||
|
}
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
public async Task LockingForgetsTheVault()
|
public async Task LockingForgetsTheVault()
|
||||||
{
|
{
|
||||||
|
|||||||
@@ -57,7 +57,7 @@ public sealed class TerminalEndToEndTests(SshServerFixture fixture)
|
|||||||
var token = await ReadTokenAsync(workspace.PageUrl);
|
var token = await ReadTokenAsync(workspace.PageUrl);
|
||||||
|
|
||||||
using var renderer = await AttachAsync(workspace.PageUrl, token);
|
using var renderer = await AttachAsync(workspace.PageUrl, token);
|
||||||
await workspace.WaitForRendererAsync();
|
await workspace.WaitForRendererAsync(TestContext.Current.CancellationToken);
|
||||||
|
|
||||||
var sessionId = await OpenTrustedSessionAsync(workspace, knownHosts);
|
var sessionId = await OpenTrustedSessionAsync(workspace, knownHosts);
|
||||||
|
|
||||||
|
|||||||
@@ -0,0 +1,66 @@
|
|||||||
|
using System.Text;
|
||||||
|
using DodoSSH.Client.Ssh;
|
||||||
|
using NSubstitute;
|
||||||
|
|
||||||
|
namespace DodoSSH.Client.Terminal.Tests;
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// The renderer gate: the one place the workspace waits on something outside the process.
|
||||||
|
/// </summary>
|
||||||
|
/// <remarks>
|
||||||
|
/// The gate itself is not in question. <see cref="TerminalDataPlane.SendAsync"/> drops frames when nothing
|
||||||
|
/// is attached, so a session opened before the renderer arrives loses its <c>SessionOpened</c> frame and
|
||||||
|
/// streams output at a terminal that was never created — and that it opens when a renderer does attach is
|
||||||
|
/// covered by <see cref="TerminalDataPlaneTests.TheRenderer_Attaches"/>. What is worth a test here is the
|
||||||
|
/// half that used to be missing: waiting for a renderer that never arrives has to end.
|
||||||
|
/// </remarks>
|
||||||
|
public sealed class TerminalWorkspaceTests
|
||||||
|
{
|
||||||
|
[Fact]
|
||||||
|
public async Task WaitingForARendererThatNeverAttaches_GivesUp()
|
||||||
|
{
|
||||||
|
// The shipped failure this stands in for is a WebView2 that never initialises — no Evergreen
|
||||||
|
// runtime, an install blocked by policy, an AppContainer that cannot reach loopback. From this
|
||||||
|
// side they are identical and all look like the listener being up with nothing ever connecting
|
||||||
|
// to it. Before the wait was bounded this test would have hung instead of failing.
|
||||||
|
await using var workspace = CreateWorkspace(TimeSpan.FromMilliseconds(250));
|
||||||
|
workspace.Start();
|
||||||
|
|
||||||
|
await Should.ThrowAsync<TimeoutException>(async () =>
|
||||||
|
await workspace.WaitForRendererAsync(TestContext.Current.CancellationToken));
|
||||||
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public async Task WaitingForARenderer_ObeysItsCancellationToken()
|
||||||
|
{
|
||||||
|
// The timeout is the backstop; the caller's token is what makes a Connect the user gave up on
|
||||||
|
// return at once rather than sitting out the rest of the wait. The timeout here is long enough
|
||||||
|
// that only cancellation can end this.
|
||||||
|
await using var workspace = CreateWorkspace(TimeSpan.FromMinutes(5));
|
||||||
|
workspace.Start();
|
||||||
|
|
||||||
|
using var cancellation = new CancellationTokenSource();
|
||||||
|
var wait = workspace.WaitForRendererAsync(cancellation.Token);
|
||||||
|
|
||||||
|
await cancellation.CancelAsync();
|
||||||
|
|
||||||
|
await Should.ThrowAsync<OperationCanceledException>(async () => await wait);
|
||||||
|
}
|
||||||
|
|
||||||
|
/// <remarks>
|
||||||
|
/// The connection factory is never reached: every test here stops at the gate, and reaching a real
|
||||||
|
/// host would make this a network test.
|
||||||
|
/// </remarks>
|
||||||
|
private static TerminalWorkspace CreateWorkspace(TimeSpan rendererTimeout) =>
|
||||||
|
new(
|
||||||
|
new InMemoryTerminalAssetProvider(
|
||||||
|
new Dictionary<string, TerminalAsset>(StringComparer.Ordinal)
|
||||||
|
{
|
||||||
|
[TerminalDataPlane.PagePath] = new(
|
||||||
|
"text/html; charset=utf-8",
|
||||||
|
Encoding.UTF8.GetBytes("<html><body></body></html>")),
|
||||||
|
}),
|
||||||
|
Substitute.For<ISshConnectionFactory>(),
|
||||||
|
TimeProvider.System,
|
||||||
|
new TerminalWorkspaceOptions { RendererTimeout = rendererTimeout });
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user