Skip to content

Release only what the program was sent when a terminal surface is torn down - #689

Merged
coneilen merged 3 commits into
mainfrom
coneilen-investigate-mouse-teardown-stuck-button
Oct 10, 2026
Merged

coneilen merged 3 commits into
mainfrom
coneilen-investigate-mouse-teardown-stuck-button

Conversation

@coneilen

@coneilen coneilen commented Oct 10, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

When a terminal surface is torn down (a recreate re-attaches the same session), a mouse button the program was told is down could stay down in the program, and a release could be sent for a press the program never saw. Teardown now releases exactly the buttons the input worker really wrote as down, from a ledger it updates only after a complete write, and the ledger is invalidated when the program switches mouse tracking off.

Changes

  • program_buttons is set when a report is queued, so teardown guessed with inputPending (any input queued for the surface). That was wrong both ways: a delivered press with a key, a drag or its own release still queued behind it got no release (stuck button), and a press whose enqueue failed got an orphan release.
  • Queued mouse reports carry a tag (press, release or pointer, buttons, position, modifier, generation). The worker, through one production function used by inputWorkerMain and by the tests (takeInputLocked, deliverInput, pumpInput), updates Surface.delivered_buttons and delivered_pointer only when the whole write succeeded and the generation still matches. A write cancelled after it completed still counts; a partial write that then fails does not.
  • Generation and tracking epoch: Surface.mouse_generation is unique per attach (a workspace counter, so no two surfaces ever share one even when slots are swapped) and is replaced under input_mutex (1) at teardown, (2) when the program stops tracking the mouse as seen by a mouse event (reconcileProgramGesture, cancelProgramGesture), and (3) after every buffer of program output in which the VT parser saw tracking go from on to off. A write that finishes late (after the 500 ms wait in waitInputIdle), or against a recreated surface in the same slot, or across a tracking switch-off, finds a different generation and changes nothing. All ledger reads, writes and resets are under input_mutex.
  • Tracking switch-off no longer needs a mouse event to be noticed. TerminalVt.State.feed splits the buffer after each sequence that can switch tracking off (a private-mode reset ending in l, or ESC c), compares the tracking state after each part, and counts every on-to-off transition in mouse_tracking_resets, including one switched straight back on within the same buffer or a sequence split across two buffers (the scan state persists between calls). Workspace.feedTerminalOutput calls noteMouseTrackingResets after each buffer, which replaces the generation and clears the ledger and owed releases. Switching from one tracking mode to another (for example 1002 to 1000) passes through off and counts, so it too makes the program forgotten. Not covered because the pinned VT parser does not turn tracking off for them: DECSTR (CSI ! p).
  • The worker writes a release, or a drag with a button held, only when that button's press was delivered in the same generation, so a press that could not be queued is not followed by the rest of its gesture. A press queued before a tracking switch-off is still written (as before this change) but is not recorded.
  • destroySurface releases delivered_buttons at delivered_pointer (where the program last heard from the pointer, not the last queued position) and drops the queue. The release step is its own function, releaseDeliveredMouse. The sink is now consulted only after the attach check, so the exited-attach case is observable.
  • Timeout outcome: waitInputIdle returns false when the worker is still busy with this surface after 500 ms (it also cancels the I/O). Then a write may still be using the pipe and a second write could interleave with it, so NO release is written and the buttons are forgotten. The generation is replaced under the mutex before anything else, so when that write finishes it is discarded and cannot touch the ledger of the recreated surface. The program may therefore keep a button down in this case; this PR does not claim otherwise.
  • No release is written either when attach_nonblocking is false (the teardown write could block the shell), or when the attach already exited (handleAttachExit ends it before the surface, so there is no session to write to). The recreate path with a live session does write the release.
  • release_owed semantics and ordering: a bit is set (under the mutex) only when a release report for a button could not be queued (queue full or allocation failure). flushOwedReleases queues the owed release again at delivered_pointer; poll does that for every button, and reportMouseToProgram does it for the button of a new press BEFORE that press is queued, so the release always precedes the next press of the same button in the queue (FIFO order is kept per button). If the owed release still cannot be queued, the new press is not queued either (it would overtake the release and the worker would clear the new press with the old release); the user's release of that press then closes the earlier press. The worker still writes an owed release only if the button is in delivered_buttons for the current generation, so a button whose press was never delivered, or a program that stopped tracking (which clears the owed bits and replaces the generation), is never sent one. If the surface is torn down first, the ledger still has the button, so teardown releases it.
  • input_write_hook replaces the attach pipe for queued writes in tests; LiveKeyboard.drainInput now runs pumpInput through it instead of dequeuing by hand.
  • Scope of the old orphan-release case (press enqueue failed): it only happened when nothing else for that surface was queued or in flight, since otherwise inputPending suppressed the release.
  • Rejected: flushing the queued bytes in order before the release (UI-thread writes of up to 64 items, races the worker, delivers half of a paste into a surface being torn down, and does not help a press that could not be queued).
  • Parity ledger row (Clipboard/selection) gains one sentence; it stays Partial.

Test plan

RED: zig test src\App.zig (pinned Zig 0.15.2, winghostty host lib 6286560, scratch cache) --test-filter "live terminal mouse" on origin/main plus the 3 first live tests -> 13 passed, 3 failed (delivered press with queued keys and drag, and with a queued release, send no teardown release: expected the release bytes, found end of string; a press that could not be queued gets an orphan release: expected 0, found 9); the same command on the first pushed head of this PR plus the 4 tests added in review (tracking off and on in one output buffer, the same split across two buffers, release owed ahead of the next press with and without room) -> 4 failed: expected 0, found 9 (twice, a release sent into a fresh tracking epoch), expected 0, found 1 (a press queued past an owed release), and press, release, press, release expected but press, press, release found
GREEN: same zig test src\App.zig command with the change, --test-filter mouse and --test-filter "VT counts" -> All 37 tests passed (18 live terminal mouse, 10 new TerminalSurface and TerminalVt tests among them)
REGRESSION: same zig test src\App.zig command with no filter, plus zig test src\TerminalSurface.zig and zig test src\TerminalVt.zig as CI runs them -> All 1096 tests passed, 259 of 259 and 27 of 27 passed

What the evidence is and is not:

Exact command (run from graphcode-windows with the pinned toolchain; scratch cache and profile directories; the host library is the pinned Winghostty build, the VT library is the pinned ghostty-vt-static.lib): zig test src\App.zig src\AccessibilityProvider.cpp src\FilePicker.c src\FolderPicker.c ...\winghostty\zig-out\lib\winghostty-win32-host.lib -DUNICODE -D_UNICODE -target x86_64-windows-msvc -lc -luser32 -lgdi32 -lgdiplus -lmsimg32 -lopengl32 -lkernel32 -limm32 -lole32 -loleaut32 -luiautomationcore -lshell32 -ladvapi32 -lwinhttp -I...\winghostty\include zig-out\lib\ghostty-vt-static.lib followed by --test-filter options (the same list Tools\windows\Tests\WindowsShell.Tests.ps1 uses for "App shell executable tests"). Final head, after rebasing on #687 and #688: filters mouse and VT counts together run 37 tests (18 live terminal mouse, 10 mouse ledger / mouse teardown / VT counts, the rest existing mouse tests) and all pass; no filter runs 1096 tests and all pass; the CI standalone commands zig test src\TerminalSurface.zig -target x86_64-windows-msvc -lc -luser32 -lgdi32 -ladvapi32 -I...\winghostty\include zig-out\lib\ghostty-vt-static.lib (259 of 259) and zig test src\TerminalVt.zig -target x86_64-windows-msvc -lc -I...\winghostty\include zig-out\lib\ghostty-vt-static.lib (27 of 27) pass.

Label of each new test against its RED baseline:

  • BEHAVIORAL (runtime assertion failure on unmodified code, baseline origin/main): the 3 first live tests (a delivered press is released at teardown though keys and motion are still queued behind it, a release still queued when the surface is torn down is not lost, a press that could not be queued is not released at teardown).

  • BEHAVIORAL (baseline: the first pushed head of this PR, where the review found the defects; each failed at a runtime assertion): the two App-fixture tests that reach the code through feedTerminalOutput and then destroySurface with a real surface and no mouse event in between (tracking switched off and on in one output buffer forgets a delivered press, ... by a sequence split across output buffers ...), and the two owed-release ordering unit tests (a release that could not be queued stays ahead of the next press of that button, a press is not queued ahead of a release that still cannot be), which use reportMouseToProgram on a workspace without a window and no new API.

  • COMPILE-LEVEL ONLY (the test needs the new seam, so on the older heads it does not compile and no behavior was observed): the in-flight reset unit test (tracking switched off and on within one buffer voids a press still in flight, with no mouse event, which calls noteMouseTrackingResets), the VT counts every time the program switched mouse tracking off test (needs mouse_tracking_resets), and the earlier unit tests only a complete write..., a release or drag is not written..., a release that could not be queued is queued again..., a program that stops and resumes tracking forgets..., only the buttons still down are released... and no release goes to an exited attach... (they need pumpInput, input_write_hook and the ledger). For these, the evidence that they test something is the mutation runs below, not a RED baseline.

  • The seven RED results above are runtime assertion failures on unmodified code. Tests that use the new seam (pumpInput, input_write_hook, noteMouseTrackingResets, mouse_tracking_resets) fail to compile on the older heads; that is a compile failure, not behavioral evidence. They cover full success, partial write then error, cancel after a complete write, generation change with a write in flight, a failed press followed by a release and a drag, a release that could not be queued, stopping and resuming tracking with a press in flight (by a mouse event, and through the parser's output path), the parser counting every on-to-off transition (one buffer, split sequence, mode switch, ESC c, text containing the letters l and h, off while already off), multi-button teardown at the delivered position, an exited attach, a blocking pipe and a worker that does not stop.

  • The tracking tests that reach the code through feedTerminalOutput are the two live tests (real surface, no mouse event after the output). The in-flight variant uses noteMouseTrackingResets, the function feedTerminalOutput calls, because a unit test has no native surface.

  • The unit tests also run in the standalone command CI uses for the input queue (no provider library). For that, the unit tests call releaseDeliveredMouse rather than destroySurface, since the native surface teardown cannot link there; the live tests exercise destroySurface itself. The test attach process goes through ZmxSession.child (aliased so the shell's spawn-site count contract is unchanged).

  • Deliberate mutations in a scratch copy (no forget on tracking off, release despite a write in flight, no worker gate, commit ignoring the generation, pointer from the queued position, sink before the attach check, no call of the tracking-reset check after output, no owed-release flush before a press, the parser only checking at the end of a buffer) each fail at least one of these tests.

  • Not tested: the real input worker thread writing to a real attach pipe (tests run the same takeInputLocked and deliverInput steps on the test thread through a write hook, and the fake zmx never reads its input), so the real cancel race (CancelIoEx against a write in progress) and the 500 ms timeout against a genuinely blocked pipe are exercised only by simulating an in-flight item; PIPE_NOWAIT behavior on the real pipe and whether the 50 ms bound of the teardown write applies; real conhost; a real mouse-aware program (vim, tmux) reacting to a release or a missing one; real program output interleaving with real mouse input; Dev Box. The ledger row stays Partial.

  • Known limits: a write that is cancelled mid-way (partial) may leave the program inside an escape sequence, the ledger counts it as not delivered and nothing repairs the program's parser; mouse reports queued before a tracking switch-off but written after it still reach the program (unchanged from before); a release for a mouse mode that sends no release bytes (DECSET 9) is never queued, so the ledger keeps that button until teardown, where the empty release writes nothing; a stream scan that mistakes a sequence inside a string for a reset only costs an extra split of the buffer.

Checklist

  • I have read the Contributing Guidelines
  • I have signed off my commits (git commit -s) per the DCO
  • Tests pass locally (make test)
  • Code follows the existing style (make check)
  • I added the test/contract before the implementation and observed the intended RED failure

make test and make check are macOS targets and were not run; the change is Windows-only.

coneilen and others added 3 commits October 10, 2026 10:16
…n down

A mouse button the program was told is down could stay down when its surface
was torn down (a recreate re-attaches the same session), and a release could
be sent for a press the program never saw. The teardown gate treated any
queued input for the surface as "undelivered", while the buttons were counted
when a report was queued.

Queued mouse reports now carry a tag, and the input worker keeps a ledger of
what the program was really sent, updated only after a complete write whose
generation still matches. Teardown releases that ledger at the last delivered
position; a release or drag is only written for a press delivered in the same
generation; a release that could not be queued is queued again.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Colin Neilens <coneilen@microsoft.com>
The input queue tests build TerminalSurface.zig alone, where destroySurface cannot link. Split the release out of it, spawn the test attach through ZmxSession.child, and call the split part from the unit tests.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Colin Neilens <coneilen@microsoft.com>
…wed releases ahead of the next press

A program that switches mouse tracking off and on again within one buffer (or one sequence split across two) left the ledger standing, so teardown released into a new tracking epoch and an in-flight press could still commit. The VT parser now counts every on-to-off transition, including one switched straight back on, and the workspace invalidates the ledger after each buffer of output. A release that could not be queued is now queued ahead of the next press of the same button (or that press is held back while it still cannot be), so the worker no longer clears the new press with the old release.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Colin Neilens <coneilen@microsoft.com>
@coneilen
coneilen force-pushed the coneilen-investigate-mouse-teardown-stuck-button branch from 4b198e3 to 21b21cf Compare October 10, 2026 17:20
@coneilen
coneilen merged commit df4d79f into main Oct 10, 2026
26 checks passed
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.

1 participant