Repository navigation
Release only what the program was sent when a terminal surface is torn down - #689
Merged
Merged
Conversation
…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
force-pushed
the
coneilen-investigate-mouse-teardown-stuck-button
branch
from
October 10, 2026 17:20
4b198e3 to
21b21cf
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_buttonsis set when a report is queued, so teardown guessed withinputPending(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.inputWorkerMainand by the tests (takeInputLocked,deliverInput,pumpInput), updatesSurface.delivered_buttonsanddelivered_pointeronly 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.Surface.mouse_generationis unique per attach (a workspace counter, so no two surfaces ever share one even when slots are swapped) and is replaced underinput_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 inwaitInputIdle), 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 underinput_mutex.TerminalVt.State.feedsplits the buffer after each sequence that can switch tracking off (a private-mode reset ending inl, orESC c), compares the tracking state after each part, and counts every on-to-off transition inmouse_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.feedTerminalOutputcallsnoteMouseTrackingResetsafter 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).destroySurfacereleasesdelivered_buttonsatdelivered_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.waitInputIdlereturns 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.attach_nonblockingis false (the teardown write could block the shell), or when the attach already exited (handleAttachExitends it before the surface, so there is no session to write to). The recreate path with a live session does write the release.release_owedsemantics 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).flushOwedReleasesqueues the owed release again atdelivered_pointer;polldoes that for every button, andreportMouseToProgramdoes 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 indelivered_buttonsfor 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_hookreplaces the attach pipe for queued writes in tests;LiveKeyboard.drainInputnow runspumpInputthrough it instead of dequeuing by hand.inputPendingsuppressed the release.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.libfollowed by--test-filteroptions (the same listTools\windows\Tests\WindowsShell.Tests.ps1uses for "App shell executable tests"). Final head, after rebasing on #687 and #688: filtersmouseandVT countstogether run 37 tests (18live terminal mouse, 10mouse ledger/mouse teardown/VT counts, the rest existing mouse tests) and all pass; no filter runs 1096 tests and all pass; the CI standalone commandszig 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) andzig 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
feedTerminalOutputand thendestroySurfacewith 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 usereportMouseToProgramon 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 callsnoteMouseTrackingResets), theVT counts every time the program switched mouse tracking offtest (needsmouse_tracking_resets), and the earlier unit testsonly 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...andno release goes to an exited attach...(they needpumpInput,input_write_hookand 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
feedTerminalOutputare the two live tests (real surface, no mouse event after the output). The in-flight variant usesnoteMouseTrackingResets, the functionfeedTerminalOutputcalls, 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
releaseDeliveredMouserather thandestroySurface, since the native surface teardown cannot link there; the live tests exercisedestroySurfaceitself. The test attach process goes throughZmxSession.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
takeInputLockedanddeliverInputsteps on the test thread through a write hook, and the fake zmx never reads its input), so the real cancel race (CancelIoExagainst a write in progress) and the 500 ms timeout against a genuinely blocked pipe are exercised only by simulating an in-flight item;PIPE_NOWAITbehavior 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
git commit -s) per the DCOmake test)make check)make testandmake checkare macOS targets and were not run; the change is Windows-only.