Reconnect the terminal view when somebody comes back to it #14
Merged
jaap-jan
merged 2 commits from 2026-08-14 14:13:29 +00:00
claude/terminal-reconnection-stuck-4e57ff into main
2
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 jaap-jan
|
ef12e8cc99 |
Stop the data plane's tests racing the socket they just connected
CI went red on the run that added the renderer tests, and not on anything they assert: TerminalDataPlaneTests.Output_ReachesTheRenderer read the output frame where it expected the session's opening one, having lost a race that has been in the helper since it was written. Connected and attached are two different moments. ClientWebSocket.ConnectAsync completes on the 101, which UpgradeAsync writes before it has a WebSocket to attach — it builds one from the stream and swaps it in a few instructions later, on the accept thread. SendAsync drops anything sent in between, which is the transport's documented contract rather than a bug: there is nowhere to put a frame for a renderer that is not there, and queueing it is the unbounded growth the credit window exists to prevent. So a helper that returned on the handshake and let its caller send immediately was betting on thread scheduling, every run, on every test in the file. It only started losing now because that assembly grew nine tests and a JavaScript engine to run them in, which is more work in parallel with a window measured in instructions. The race is older than the branch that exposed it. ConnectAsync now waits for the plane's own SocketAttached, subscribed before the connection because the event can be over before ConnectAsync returns, and bounded so a socket that never attaches fails the helper instead of hanging the suite in a later receive. TerminalWorkspaceTests.ConnectRendererAsync has the same exposure through OpenSessionAsync's opening frame and now waits on WaitForRendererAsync, with a note on why that is enough for the reattach tests and what would stop being enough. ◆ NOTHING IN src CHANGED, AND THAT IS THE CONCLUSION RATHER THAN THE SHORTCUT. Production waits for exactly this moment already — every path that opens a session goes through TerminalWorkspace.WaitForRendererAsync, which resolves from the same few lines that raise the event this helper now waits on. Only the tests skipped the gate the application does not. Verified by widening the window rather than by hunting the flake: a 100ms delay inserted between the 101 and the attach, in a throwaway tree, hangs the old helper outright — both frames dropped, the test blocked in receive — and passes 89/89 with this one. Green three times over on the CI platform besides (Alpine, musl, Release, dotnet/sdk:10.0-alpine). |
||
|
jaap-jan
|
963cb7f670 |
Reconnect the terminal view when somebody comes back to it
Going back to a terminal left alone for a while found it stuck on "Reconnecting the terminal view…", and stuck is the right word: the banner stayed and nothing behind it was reconnecting. The page's whole recovery story was a setTimeout chain, and a chain is exactly what a WebView is entitled to stop running. Chromium throttles timers in a page nobody is looking at — down to once a minute once it has been hidden five minutes — and a renderer that is frozen, or reclaimed and not yet reloaded, runs none of them. So the socket drops while nobody is watching, the banner goes up, the retry is scheduled, and the retry is then the one thing not running. Three defects, each of which leaves that banner up for the rest of the page's life. ◆ NOTHING LISTENED FOR THE PAGE COMING BACK. The only thing that could clear the banner was a timer that may never fire. terminal.js now reconnects on visibilitychange, focus and online — the events that mean somebody is looking again, and the ones that cannot be throttled — cancelling the pending timer and resetting the backoff. Over a healthy socket all three do nothing, which is what makes them safe to fire as often as clicking a window does. ◆ A HANDSHAKE THAT NEVER FINISHED WAS INVISIBLE. Every retry was scheduled by a close or an error, so an attempt parked in CONNECTING — which is what a suspended renderer leaves behind — scheduled nothing at all, ever. There is now a five-second watchdog on the handshake. ◆ STALE SOCKETS SCHEDULED RETRIES, AND THAT ONE IS A LOOP RATHER THAN A STALL. connect() never detached the old socket's handlers, and the host aborts the displaced socket on takeover — TerminalDataPlane.UpgradeAsync, doing exactly what it should. That close read as a fresh failure and scheduled a retry against the socket that had just succeeded, whose own close scheduled the next: no fixed point, reconnecting every second forever with the banner up for most of it. Every handler now asks whether it is still the page's own attempt, and connect() closes what it abandons. ◆ WHICH OF THE PLATFORM BEHAVIOURS ACTUALLY BIT IS NOT ESTABLISHED, and the fix does not depend on knowing. Throttled timers, a frozen renderer and a reclaimed one all end at the same dead timer; guessing between them would have produced a narrower fix for one of the three. THE TEST RUNS terminal.js ITSELF, in a fake browser, inside dotnet test. RendererPage loads the file the shell project ships — not a transcription of its logic into C#, which would be a copy that stays correct while the page rots — into a Jint engine, one per test, over a harness that fakes a WebSocket and a clock and nothing else. Jint rather than a node script because CI would run the node one and nobody's inner loop would; the cost is that Jint is not Chromium, so this proves the page's logic and nothing about how a WebView behaves. That line is drawn in RendererPage's remark and picked up by two new manual checks, 1.10 for the desktop and 11.12a for the phone, which own the platform half. Four of the nine tests fail against the page as it stood — the stale close, the parked handshake, and the two wake-ups. Two more assert that a wake-up over a healthy socket does nothing, and pass against either version on purpose: they are what stops the cure being worse. Left alone deliberately: a socket that is open and dead shows no banner at all, because readyState still reads OPEN. That looks like a terminal that swallows what is typed, needs a liveness probe rather than a faster retry, and is written down at the end of 11.12a rather than quietly bundled in here. |