Skip to content

fix(terminal): promote the options token only once its metrics are measured - #143

Merged
AThraen merged 1 commit into
mainfrom
fix/options-token-settle
Sep 30, 2026
Merged

AThraen merged 1 commit into
mainfrom
fix/options-token-settle

Conversation

@mortenaslo

Copy link
Copy Markdown
Contributor

Note

@AThraen: I have not tested this manually in the running app. This is page-side JavaScript, and none of the 597 unit tests touch it; node --check and a clean build are the only automated checks here. The manual checks under Test plan are all still open. Could you run through them, or tell me if you'd rather I do it before review?

Summary

Follow-up to #141. A review pass over the merged size handshake found two holes in terminal-init.js. One could still release the host's size wait on a measurement taken with the old font; the other could make it wait out the full timeout.

Why

1. The page adopted the options token too early. The message handler copied msg.token into optionsToken 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 (its own comment says so), but it reported that size tagged with the new token. So did any fit or focus message landing before the next frame. The host's NoteOptionsToken then saw an echo new enough, and WaitForInitialSizeAsync let the launch continue on a pre-font measurement. A session with a profile font override could therefore still get its ConPTY at the default font's column count, which is exactly what the token exists to prevent.

2. The acknowledgement waited on a rendered frame. Both the forced refit and the optionsApplied ack lived only inside requestAnimationFrame, and WebView2 pauses rAF whenever the control isn't rendering: the window is minimized, or the pane was detached by RefreshTerminalLayout's TerminalGrid.Children.Clear() while a launch was still waiting. Every other way the wait can end is also blocked in that state: doFit skips a pane it can't measure, the ResizeObserver needs a size change, and the 50ms/250ms timers fired long ago at page load. So nothing acknowledged the token, and each affected launch waited the full 1.5s. That's roughly +37s across a 25-session restore.

Implementation notes

  • The token goes into a per-message newToken and is only adopted inside settle(), just before the forced refit. So a size report can only carry the new token once the new font is what's being measured.
  • It's adopted with max() rather than plain assignment, so two setOptions in flight (ApplyFontSettings then ApplyProfileOverrides) can't move it backwards.
  • settle() runs from requestAnimationFrame or a 250ms setTimeout, whichever comes first, and a flag stops it running twice. rAF still wins whenever frames are running, so normal behavior is unchanged; the timer only matters when rendering is paused.
  • A correction to the review that prompted this: it said fit/focus messages carried the token. They don't; only setOptions does. They reported the token that had already been adopted too early. It's the same bug one step removed, and this change fixes it too.
  • CLAUDE.md's "gated on an options token" section is updated, since it described adopting the token on arrival as if that were correct.

Two findings from the same review are not fixed here:

  • TerminalBridge.cs: optionsApplied can end the wait before any size was ever measured, and the placeholder is now (220, 50). So a pane that can't be measured yet gets a ConPTY that is too wide rather than too narrow.
  • terminal-init.js: term.onResize(postSize) passes xterm's event object as force, which bypasses the duplicate-size check the other two callback sites were wrapped to keep. It's a one-line fix.

Test plan

  • node --check src/CodeShellManager/Assets/terminal-init.js
  • dotnet build passes with 0 warnings
  • dotnet test tests/CodeShellManager.Tests/ passes 597/597 (none of these cover this code)
  • With DebugTerminalTrace on, launch a session with a profile font override (different family/size from the global font). The RESIZE … token=N released=True line carries the column count for the override font, and the program fills the pane.
  • Same, with an override whose column count happens to match the default font. It still releases promptly (look for OPTIONS-APPLIED) instead of timing out.
  • Minimize the window during a multi-session restore. Launches don't each stall for 1.5s: look for released=True within about 250ms rather than the timeout.
  • Switch layout while sessions are launching (detaches the panes). Same check as above.
  • Transparent profile (terminal-transparent.html shares this file): same checks, no regression.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NsYm9iLc2aRnRDfV2uZRQX

…asured

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NsYm9iLc2aRnRDfV2uZRQX
@mortenaslo
mortenaslo requested a review from AThraen September 30, 2026 11:02
@AThraen
AThraen merged commit 936fb41 into main Sep 30, 2026
1 check passed
AThraen added a commit that referenced this pull request Sep 30, 2026
`term.onResize(postSize)` passed xterm's `{cols, rows}` event object straight
into postSize's `force` parameter, where it is truthy. That is the same latent
bug #141 wrapped the ResizeObserver, requestAnimationFrame and
document.fonts.ready sites for, and this one call site was missed — both #143's
review and a review of the merged #141 found it independently.

Effect is small: a resize originating inside the terminal (CSI 8 t) bypassed the
duplicate-size check and posted even when the size already matched what the host
had been told. `force` exists to acknowledge an options token when the column
count happens not to change; it is not meant to be on for ordinary resizes.

Every doFit/postSize call site is now either explicit or wrapped; the only
`force: true` left is settle()'s options ack, which is deliberate.

Verified with `node --check`; build clean at 0 warnings, 597/597 tests (none
cover page-side JS — this is reasoned, not exercised).


Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants