From 94c7be539d8301603627c06004ab1a3b0eea2ad7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Morten=20Aslo-=C3=98stergaard?= Date: Wed, 30 Sep 2026 12:27:59 +0200 Subject: [PATCH] fix(terminal): promote the options token only once its metrics are measured Follow-up to #141. Review of the size handshake found that the page handed the mechanism back the bug it exists to prevent, plus a way for it to stall. 1. The token was promoted on arrival (terminal-init.js). The message handler stamped optionsToken = msg.token at the very top, before the new options had taken measurable effect. The doFit() that runs straight after the option assignments still measures the OLD metrics (the code's own comment says so), yet it posted that size under the NEW token. Any 'fit' or 'focus' message landing before the next frame did the same. The host's NoteOptionsToken then saw an echo at least as new as it was waiting for, and WaitForInitialSizeAsync released on a pre-font measurement, so a session with a profile font override could still get its ConPTY at the default font's column count. The token now lands in a per-message newToken and is promoted inside settle(), immediately before the forced refit, so a report can only carry the new token once the new metrics are the ones measured. Promotion is max() rather than assignment, so two setOptions in flight (ApplyFontSettings then ApplyProfileOverrides) can never walk it backwards. 2. The ack depended on a frame being rendered. Both the forced refit and the optionsApplied ack lived only inside requestAnimationFrame, which WebView2 suspends while the control isn't rendering: window minimized, or the wrapper detached by RefreshTerminalLayout's TerminalGrid.Children.Clear() while a launch sits in its await. Every other release path is gated off in that state (doFit declines an unmeasurable pane, the ResizeObserver needs a size change, and the 50ms/250ms one-shots fired long ago at page load), so nothing acked and each affected launch burned the full 1.5s, roughly +37s across a 25-session restore. settle() now runs from requestAnimationFrame or a 250ms timer, whichever comes first, guarded so it runs once. rAF still wins whenever frames are running, so the measured-metrics path is unchanged in the normal case. Not exercised by the unit tests (it is page-side). Check by launching a session with a profile font override and looking for RESIZE ... token=N released=True under DebugTerminalTrace. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01NsYm9iLc2aRnRDfV2uZRQX --- CLAUDE.md | 17 ++++++++ src/CodeShellManager/Assets/terminal-init.js | 42 +++++++++++++++++--- 2 files changed, 53 insertions(+), 6 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index cda4cd7..2fc74a4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -353,6 +353,23 @@ So each `setOptions` carries an incrementing token (stamped **synchronously**, b size report, and the wait resolves only once an echo is at least as new as the last token stamped. That is a happens-after relationship rather than a timing guess. +**The page promotes that token late, in `settle()`, not when the message arrives.** Stamping +it on arrival is the same bug wearing the fix's clothes: the `doFit()` that runs immediately +after the option assignments still measures the *old* metrics, and any `fit`/`focus` message +landing before the next frame does too — so those reports carry the new token, `NoteOptionsToken` +sees an echo new enough, and the wait releases on a pre-font measurement. Only a report made +after the new metrics are in effect may carry the new token. + +**And `settle()` must not depend on a frame.** It runs from `requestAnimationFrame` *or* a +250ms timer, whichever comes first, because WebView2 suspends rAF whenever the control isn't +rendering — window minimized, or the wrapper detached by `RefreshTerminalLayout`'s +`TerminalGrid.Children.Clear()` while a launch sits in its `await`. In that state every other +release path is already gated off (`doFit` declines an unmeasurable pane, the `ResizeObserver` +needs a size *change*, and the 50ms/250ms one-shots fired long ago at page load), so nothing +acked the token and each affected launch burned the full 1.5s — roughly +37s across a +25-session restore. rAF still wins whenever frames are running, so the measured-metrics path +is unchanged in the normal case. + The page also posts a separate `optionsApplied` ack, and that split is load-bearing in both directions: `doFit()` may legitimately decline to report an unmeasurable pane, and without an ack of its own a launch waiting on that token would burn the full 1.5s timeout per session. diff --git a/src/CodeShellManager/Assets/terminal-init.js b/src/CodeShellManager/Assets/terminal-init.js index 7fe358d..d383f90 100644 --- a/src/CodeShellManager/Assets/terminal-init.js +++ b/src/CodeShellManager/Assets/terminal-init.js @@ -58,6 +58,9 @@ // Mirrors the token the host stamps on each setOptions message, and is echoed back on // every size report. It is how the host can tell "the size measured with the font you // just asked for" from "the size measured before it" — see WaitForInitialSizeAsync. + // Promoted in settle() once the new metrics are the ones being measured, NOT when the + // message arrives: stamping it on arrival makes every report in between claim a font it + // was not measured with, which is the whole failure the token exists to catch. var optionsToken = 0; // force: report even when the size is unchanged. Needed to ACK an options token, since @@ -213,7 +216,6 @@ window.chrome.webview.addEventListener('message', e => { try { const msg = JSON.parse(e.data); - if (typeof msg.token === 'number') optionsToken = msg.token; if (msg.type === 'output') diagWrite(msg.data); else if (msg.type === 'setDiag') diagOn = !!msg.on; else if (msg.type === 'clear') term.clear(); @@ -244,22 +246,50 @@ // OS-installed and there are no such rules, so it resolves on the next microtask // having matched nothing. requestAnimationFrame is the honest signal: it fires // after the style change has been applied and measured. - // Unconditional now, and forced, so a font change that does not happen to alter - // the column count is still reported rather than silently deduped away. + // Unconditional, and forced, so a font change that does not happen to alter the + // column count is still reported rather than silently deduped away. // // The ack is posted separately and never skipped. doFit() declines to report an // unmeasurable pane (0x0 container, cell metrics not computed yet), and a host // waiting on this token would then have nothing to wait for but its own timeout — // 1.5s of dead launch per session. Splitting them keeps both properties: the host // only ever adopts a size it actually measured, and the wait always ends promptly. - requestAnimationFrame(function () { + // + // **The token is promoted HERE, not on arrival.** It used to be stamped at the top + // of this handler, which handed the whole mechanism back its own bug: the doFit() + // above — and any 'fit'/'focus' message landing before the frame — would post a + // size measured with the OLD font carrying the NEW token, the host's + // NoteOptionsToken would see an echo at least as new as it was waiting for, and + // WaitForInitialSizeAsync would release on a pre-font measurement. That is the + // exact failure the token exists to prevent. Promoting inside settle() means a + // report can only carry the new token once the new metrics are the ones measured. + // max() rather than assignment so two setOptions in flight (ApplyFontSettings then + // ApplyProfileOverrides) can never walk the token backwards. + // + // **settle() must not depend on a frame.** WebView2 suspends rAF whenever the + // control is not rendering — window minimized, or the wrapper detached by + // RefreshTerminalLayout's TerminalGrid.Children.Clear() while a launch sits in its + // await. Every other release path is gated off in that state (doFit declines an + // unmeasurable pane, the ResizeObserver needs a size CHANGE, and the 50ms/250ms + // one-shots were scheduled at page load and have long since fired), so nothing + // acked and every affected launch burned the full 1.5s — ~37s across a 25-session + // restore. The timer is the backstop; rAF still wins whenever frames are running, + // so the measured-metrics path is unchanged in the normal case. + const newToken = (typeof msg.token === 'number') ? msg.token : optionsToken; + let settled = false; + const settle = function () { + if (settled) return; + settled = true; + if (newToken > optionsToken) optionsToken = newToken; doFit(true); try { window.chrome.webview.postMessage(JSON.stringify({ - type: 'optionsApplied', token: optionsToken + type: 'optionsApplied', token: newToken })); } catch (e) {} - }); + }; + requestAnimationFrame(settle); + setTimeout(settle, 250); } else if (msg.type === 'dropOverlayClear') overlay.classList.remove('active'); else if (msg.type === 'setBootState') {