Compare commits

..
Author SHA1 Message Date
jaap-jan 8e61fc72b4 Stop an attach's own replay landing in the middle of a test's frames
ci / build and test (pull_request) Successful in 7m43s
ci / desktop nightly (pull_request) Skipped
ci / api image (pull_request) Failing after 11s
ci / android head (pull_request) Successful in 8m7s
TheClipboard_ReachesThePaneThatAskedForIt failed in CI on an opcode of 2
where it wanted 8: a second SessionOpened frame where the clipboard's
should have been. Only one thing sends a session a second SessionOpened,
and that is ReplayAfterAttachAsync, the replay a reattaching page gets.

It arrives there because the replay is fired and forgotten from the
socket-accept thread immediately after the attach is signalled, and the
test helper returned on that signal. Deschedule the accept thread for a
timeslice between the two -- which a loaded CI box does -- and the test
opens its session first, so the replay finds that session live and sends
it a replay-flagged SessionOpened after its real one.

Nothing is wrong with the workspace. The page treats a replay onto a pane
it still has as idempotent, and ReplayAfterAttachAsync's own remarks
already say racing a fresh page's first frames is harmless. What was
wrong is a helper that waited for the attach and then let its callers
count frames as though the attach were finished, so ConnectRendererAsync
now waits for RendererReattached as well -- raised as the last thing the
replay does. The remark there argued a replay could not be sent early; it
missed that it can be sent late, and now says so.

The same late replay could reset the credit window out from under
ANewRenderer_ReplaysTheLiveSessionAndResetsItsCredits, which shares the
helper, so that latent flake goes with it.

Verified by delaying the replay's snapshot by a millisecond: the old test
fails under that, and the fixed class passes it eight runs running.
2026-09-15 14:29:08 +02:00
jaap-jan 8a4058ce3a Merge pull request 'Stop a reconnect dialling a different machine of the same name' (#17) from claude/reconnect-wrong-server-9cccfb into main
ci / desktop nightly (push) Skipped
ci / api image (push) Skipped
ci / android head (push) Successful in 3m24s
ci / build and test (push) Failing after 2m20s
Reviewed-on: #17
2026-09-14 11:31:04 +00:00
@@ -27,6 +27,12 @@ public sealed class TerminalWorkspaceTests
/// <summary>How long <see cref="WaitUntilAsync"/> polls before calling it a failure.</summary>
private static readonly TimeSpan PollTimeout = TimeSpan.FromSeconds(10);
/// <summary>
/// How long <see cref="ConnectRendererAsync"/> waits for the attach's replay before calling it a
/// failure. Generous, because it bounds a hang rather than timing anything.
/// </summary>
private static readonly TimeSpan ReplayTimeout = TimeSpan.FromSeconds(10);
[Fact]
public async Task WaitingForARendererThatNeverAttaches_GivesUp()
{
@@ -773,11 +779,20 @@ public sealed class TerminalWorkspaceTests
/// </para>
/// <para>
/// <c>WaitForRendererAsync</c> answers only for the first renderer ever to attach, so a second call
/// returns immediately without proving anything about the second socket. That is enough here and is
/// not luck: the only frames a second socket is given are the replay, and the replay is *caused* by the
/// attach — <see cref="TerminalWorkspace.RendererReattached"/> and the frames before it cannot be sent
/// early. The day a test sends something else down a reattached socket, this needs the data plane's own
/// <c>SocketAttached</c>, which the workspace does not forward today.
/// returns immediately without proving anything about the second socket — and even for the first it
/// answers too early for what the callers here need. It completes when the socket is attached, which
/// is before <see cref="TerminalWorkspace.ReplayAfterAttachAsync"/> has run: that is fired and
/// forgotten from the accept thread, and a thread-pool that deschedules it for a timeslice lets the
/// test open a session in the meantime. The replay then finds that session live and sends it a second,
/// replay-flagged <c>SessionOpened</c> — harmless to the page, which treats it as idempotent, but it
/// lands in the middle of the frames the test is counting on.
/// </para>
/// <para>
/// ◆ SO WAIT FOR THE REPLAY TO BE OVER, NOT ONLY FOR THE ATTACH. <c>RendererReattached</c> is raised
/// as the last thing the replay does, so a socket this returns has had every frame the attach itself
/// causes, and nothing but what the test does next can put another on it. Subscribed before the
/// upgrade, because the replay for a socket that attaches quickly can be over before this method gets
/// to look.
/// </para>
/// </remarks>
private static async Task<ClientWebSocket> ConnectRendererAsync(TerminalWorkspace workspace)
@@ -795,17 +810,29 @@ public sealed class TerminalWorkspaceTests
"Origin",
string.Create(CultureInfo.InvariantCulture, $"http://127.0.0.1:{workspace.PageUrl.Port}"));
var replayed = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
void OnReattached(object? sender, EventArgs e) => replayed.TrySetResult();
workspace.RendererReattached += OnReattached;
try
{
await client.ConnectAsync(new Uri(socketUrl), TestContext.Current.CancellationToken);
await workspace.WaitForRendererAsync(TestContext.Current.CancellationToken);
await replayed.Task.WaitAsync(ReplayTimeout, TestContext.Current.CancellationToken);
}
catch
{
client.Dispose();
throw;
}
finally
{
workspace.RendererReattached -= OnReattached;
}
return client;
}