Repository navigation
fix(app): held keys for one terminal go out through one ordered send queue (P0 1193) - #29919
Merged
lawrencecchen merged 5 commits intoNov 1, 2026
Merged
Conversation
… they were typed (P0 1193) Re-review of #29693 at bc84781 (lead 00:2xZ: same input-order class as the P0): each held .type run for a terminal with no view is its own daemon send (a Task), and a run of several requests ('ab', then Return) can let a later run of the same terminal ('cd') in between them. Red on #29693's head bc84781.
…dered send queue per terminal (P0 1193)
Each held .type run for a terminal whose view is gone was its own DaemonService.send (its own Task). A run of several
requests ('ab', then Return) awaits a reply between them, and the connection actor takes new work meanwhile, so a later
run for the same terminal ('cd') could run between them: the shell got 'ab', 'cd', Return. DaemonService.sendInOrder
chains every ordered send to one surface after the one before it (a per-surface queue; the ticket opens at the call,
a failure is logged and the next send still runs; the entry clears after the last send). The held-key daemon path uses
it. It lives in CmuxNextDaemonService, so CmuxNextApp does not grow.
Test: SecondCreationKeysTests.twoHeldRunsForAnEvictedTerminalKeepTheirOrder (red on fd0d7c3).
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Packages/macOS/CmuxNext/Sources/CmuxNextDaemonService/DaemonService.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Sources/CmuxNextDaemonService/DaemonService.swift:40">
P1: `orderedSends` survives daemon shutdown, allowing queued held-key tasks to cross connection sessions. Cancel or invalidate this per-connection queue during shutdown so an old task cannot send to a replacement daemon or block new input.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| /// Set while an administrator turned this machine's feature off: no endpoint, no re-attach. | ||
| public let policyBlock = PolicyBlock() | ||
| /// The last ordered send to each terminal surface (``sendInOrder(_:to:_:)``): the next one waits for it. | ||
| @ObservationIgnored var orderedSends: [SurfaceID: (serial: UInt64, work: ActionWork)] = [:] |
There was a problem hiding this comment.
P1: orderedSends survives daemon shutdown, allowing queued held-key tasks to cross connection sessions. Cancel or invalidate this per-connection queue during shutdown so an old task cannot send to a replacement daemon or block new input.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Sources/CmuxNextDaemonService/DaemonService.swift, line 40:
<comment>`orderedSends` survives daemon shutdown, allowing queued held-key tasks to cross connection sessions. Cancel or invalidate this per-connection queue during shutdown so an old task cannot send to a replacement daemon or block new input.</comment>
<file context>
@@ -36,6 +36,9 @@ public final class DaemonService {
/// Set while an administrator turned this machine's feature off: no endpoint, no re-attach.
public let policyBlock = PolicyBlock()
+ /// The last ordered send to each terminal surface (``sendInOrder(_:to:_:)``): the next one waits for it.
+ @ObservationIgnored var orderedSends: [SurfaceID: (serial: UInt64, work: ActionWork)] = [:]
+ @ObservationIgnored var orderedSendSerial: UInt64 = 0
/// Retry Now and Restart cmux-tui while the local daemon cannot be reached (cx-jniyz.1).
</file context>
Contributor
Author
|
Review (independent review subagent): FIX at a51d0a8 Verdict: FIX (1 defect, one-line fix)The design is correct and the test proves the named cause. One path in the new primitive drops the input, reports success and leaves the command ticket open. Fix that path, then this can ship. Defect (must fix)
Checked and correct
Not blocking: other writers to the same terminalThese writers stay unordered against this queue. They were also unordered with the old
All of them need a live view. The remaining window: a held run goes through the daemon because the view is gone, the view comes back, and the person types (or a later held run takes the view path) before the daemon queue drains (about one round trip, more for a long queue). This is the same bug class (input order to one terminal) but a narrower case, and the PR says that it covers only held keys. I recommend a follow-up: a held run takes the view path only after that surface's ordered queue is empty, or live input waits for it. Test:
|
…es and a lost send is reported (P0 1193) Review of #29919 at a51d0a8 (FIX, REVIEW.md rows 9 and 10): sendInOrder's Task captured the DaemonService weakly and returned nil (success) when it was gone at its turn: the held keys were lost with no report, and the ticket opened at the call never closed, so an action scope waited for its control timeout. Remote services are released (Cloud, SSH, Server sessions) and held keys can go to remote terminals. Now the Task keeps the service, as send(_:_:) does: failure(...) closes the ticket and reports 'not connected' when the connection is gone. No red test: it needs a DaemonService released while a send waits in its queue, which the app test harness cannot make deterministically; the change is the same capture send(_:_:) has used.
lawrencecchen
temporarily deployed
to
ci-routing-agent
November 1, 2026 01:38 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
lawrencecchen
temporarily deployed
to
ci-routing-agent
November 1, 2026 01:47 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
…Work (compile 456a0d92)
lawrencecchen
temporarily deployed
to
ci-routing-agent
November 1, 2026 01:59 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
Author
|
Review (independent review subagent): SHIP at ded9410 Verdict: SHIPThis is a re-review after my FIX at a51d0a8 (weak What changed since a51d0a8
Checked
Test evidence (read-only: cmux-ci status/log)
Not blocking (same as before)Other writers to the same terminal (the view's Not run by me: no build or test run. |
lawrencecchen
added
risk:normal
Base automatically changed from
fix/p0-1193-followup-v2
to
fix/p0-1193-key-lanes
November 1, 2026 03:41
lawrencecchen
removed
the
ready-to-land
Contributor
Author
lawrencecchen
added a commit
that referenced
this pull request
Nov 1, 2026
…0 1193, Cmd-D twice) (#29301) * test(app): keys typed for the second of two pending creations never reach the first terminal, and back (P0 1193) Tip watcher smoke, feat 5097a27 on cmux18, step 23: Cmd-D, a line at once, Cmd-D, a line at once: both lines ran in the FIRST new terminal and the second got nothing. SecondCreationKeysTests drives two creation holds on two real terminals: keys typed after the second creation began must wait for the second terminal; keys held for the first creation must reach the first terminal, also when the second one has the keyboard first. Red on this base. * fix(app): each held key goes only to the terminal it was typed for (P0 1193) After Cmd-D, a line, Cmd-D, a line typed at once, both lines ran in the first new terminal (tip watcher smoke on feat 5097a27, 2 of 3 rounds on cmux18). CreationInputBuffer.takesKeysEarly(tab:) was true for the early tab of ANY pending creation: until the second creation's focus landed, focus still named the first one's early view and the first creation was still pending (its reply is slower under load, and the second create waits for it), so CreationInputCoordinator.capture let the second line's keys, and the keys held before them, go to the first terminal. Each held key now carries a lane: the creation whose tab the focus named, or was on its way to, when it was typed (FocusState.typingTab). An early view takes only its own lane's keys, and only when its terminal view is the window's first responder; a key typed for another creation waits for that one. When the hold ends, a lane whose terminal is not the focused one gets its plain text by that terminal's input (TerminalSession.sendBytes, by the terminal id its create chose); the focused lane, keys of no lane and chords run again as typed, as before. The perf line's rule (050651b, "only the newest pending creation's early tab") is not ported as is: it sends keys held for the first creation to the second terminal when the second gets the keyboard first. CreationHoldEnd and CreationHoldLog move to CmuxNextFocus (as on the perf line), so CmuxNextApp gains no lines. Test: SecondCreationKeysTests (red on 0383621). * test(app): a held Ctrl-C and arrow key reach only their own terminal, as 03 and cursor-up (P0 1193) Review of ad8c348 (coordinator): held keys that are not plain text (Ctrl chords, arrows, Return, Option-as-Alt) for a terminal that is not focused still ran in the focused pane; a held Ctrl-C there stops another process. Byte readers in both terminals show which one gets the held Ctrl-C and Up. * fix(app): a held key for a terminal that is not focused is encoded by that terminal's own surface (P0 1193) Review of ad8c348 (coordinator): only plain-text held keys went to their own terminal; Ctrl chords, arrows, Return and Option-as-Alt keys still ran in the focused pane. Now every held key of a lane whose terminal is not focused goes to that terminal through TerminalSurfaceView.typeHeldKey: its Ghostty key encoder (modifiers, Option-as-Alt setting, cursor and keypad modes, Kitty protocol), written to that terminal as a typed key, with no input method. It never falls back to the focused pane; keys of a terminal whose view is gone are dropped with it (logged), as a closed creation's are. Command chords are app actions: they run again as typed, in the focus at the hold's end, as before. Test: SecondCreationKeysTests.heldControlAndArrowKeysReachOnlyTheirOwnTerminal. * test(app): Cmd-D, ls, Up, Ctrl-C, Cmd-D, a line, held: each key runs at its place in its own terminal (P0 1193) cmux-ad 11:15Z review of #29301: a lane holding a non-text key (an arrow, Ctrl-C) or a Command chord went back to the focused terminal as a whole. The new test needs held keys that run again to be visible in a test window, which is never key: CreationInputCoordinator.runKey is the seam (NSApplication.sendEvent by default, unchanged). * fix(app): every held key runs at its place, for its own terminal; only Command chords go through app dispatch (P0 1193) cmux-ad 11:15Z review: take(focused:writer:) sent a lane with any key that is not plain text back to "the rest", which ran in the focused terminal, and ran Command chords after every typed key. Now the hold's end is one ordered list (CreationInputBuffer.HeldKey): a key of a lane whose terminal is not focused is typed by that terminal's own Ghostty encoder (TerminalSurfaceView.typeHeldKey: arrows, Ctrl chords, Return, Option-as-Alt, keypad and cursor modes); a Command chord, the focused lane's keys and keys of no lane run again as typed (runKey), each at its typing position. A Command chord that starts another creation still takes the keys after it into that creation's hold. TerminalSession .hasKeyboard is the one "this terminal has the keyboard" check. Test: SecondCreationKeysTests.eachHeldKeyRunsAtItsPlaceInItsOwnTerminal (red at the seam commit). * test(app): SecondCreationKeysTests: no #require nested in #require (compile 2c7852b3) * test(app): a held app shortcut acts on the pane it was typed for; held keys survive a workspace switch; Cmd-W closes only its pending split (P0 1193) cmux-ad 11:2xZ review of #29301: a held Command chord ran in the focus at the hold's end (a held Cmd-D split the focused pane, not the first terminal's), and keys of a terminal whose view was gone were dropped. Cmd-W is never held (a close key acts at once, CreationInputBuffer.holds); its test guards that rule in the reported sequence. * fix(app): a held app shortcut runs on the terminal it was typed for; a hidden or far terminal still gets its held keys (P0 1193) cmux-ad 11:2xZ review of #29301: - A held Command chord of a lane whose terminal is not focused ran in the focus at the hold's end. Now an app shortcut runs with that terminal's tab as its target (RegistryKeyBindings.run target:) and that terminal's key context, as a key press there; handlers resolve a tab target to its pane (AppActionContext.paneController, daemonPane). A Command chord that is no app shortcut is that terminal's key (a TUI's Cmd keys). - Keys of a terminal whose view is gone were dropped. A hidden terminal's view (another workspace, an unselected tab) still exists in the content cache and types them; with no view (evicted, another machine's), the daemon types them with that terminal's own modes (send-key / send, HeldKeyInput), on the machine its creation was on. Only a closed terminal takes its keys with it. DecidedKeyEvents moves to CmuxNextFocusUI (pure AppKit, the key router's), so CmuxNextApp shrinks. Tests: SecondCreationKeysTests.eachHeldKeyRunsAtItsPlaceInItsOwnTerminal (split position), heldKeysReachTheirTerminalAfterItsWorkspaceWasSwitchedAway, cmdWTypedForAPendingSplitClosesThatSplitOnly. * test seam(debug): debug.creation.hold / debug.creation.release hold a creation's daemon reply until released (P0 1193) cmux-ad 11:25Z: the 1193 journey controls the race instead of chasing it with load or timestamps. debug.creation.hold {n} arms the n-th creation (split, New Column, New Row, new terminal tab) that starts after it: the daemon makes the pane, and the app applies the reply only at debug.creation.release {n} (no n: all, disarmed); a held reply nobody releases goes on after 30 s. DEBUG and bench builds only (a release router drops debug.* methods); no product timing. Registered on the existing debug.daemon line, so CmuxNextApp does not grow. * fix(app): a key held for a terminal with no view honours macos-option-as-alt, as its view's encoder does (P0 1193) cmux-ad 11:3xZ: on the daemon path an Option key sent what macOS composed even when Ghostty's macos-option-as-alt (true, left, right) makes Option Alt, so another byte reached the terminal than a key press in its view sends. HeldKeyInput reads the same setting (GhosttyRuntime.optionAsAltValue) and sends alt+<base key> (ESC and the key). Test: HeldKeyInputTests (the mapping per setting and side, the config read, the order of text, arrows, Ctrl-C and Return). It needs the new API, so it is committed with the fix; it has no red commit of its own. * test seam(debug): debug.creation.hold builds its reply one key per line in the control socket's JSON type (P0 1193) pa-d's Debug builds 8a86e9ae and c1cfa1dc: "the compiler is unable to type-check this expression in reasonable time" at the reply's one-expression literal, which also named a JSONValue that CmuxNextDaemon defines too. Build it as debug.daemon does: the call's own params type, one key per line. * test seam(debug): debug.creation.release answers "resumed", the creations whose waiting reply it let go (P0 1193) pa-d's journey review: "released" lists every number a release named, so it cannot prove that creation 1 waited. "resumed" lists only the creations this call found waiting and resumed. * test seam(debug): debug.creation.hold {stand_in: [n]} keeps creation n's stand-in out of the store until its release (P0 1193) pa-d's rounds (cmux18, no hog): with only creation 1's reply held, the red ref PASSED: creation 2's stand-in landed the focus within a frame, before d2's first key. The 1193 state needs creation 2's focus asked for (in its key's turn) and not landed while d2 is typed, as when the main thread ran the keys before it laid the new pane out (the hog). stand_in makes that state on purpose: the intent waits, the reply waits too, and the release shows the stand-in first ("shown" in its reply). DEBUG and bench builds only. * test(app): SecondCreationKeysTests make both terminals by splits, and every wait says what it waited for and what it saw (P0 1193) Red step 5e829f0c5c0ff1f0d9702c6a (red ref 25b488f) was no valid red: 5 tests stopped in setup at line 50 ("timed out: panes [3: conversation 2, 6: pty 5]"): the test app's first pane is an agent chat, not a terminal, so the wait for two terminal panes never held. The Cmd-W guard read the new pane's currentTabKey in the split's own turn (nil: the pane shows its stand-in in a later frame). Now two splits make both terminals, each focused with the keyboard before the test body; the Cmd-W test starts from a terminal and reads the stand-in's tab from its model. * test(app): SecondCreationKeysTests red on the outcome the person sees, not on the hold mechanism (P0 1193) Red step f78bf3a645e937c0a98ffb98 (red ref 656454a), coordinator's reading: - Four tests failed on "were the keys held": the setup took the keyboard away (makeFirstResponder(nil)) and opened an early tab, but the app's focus gives the terminal the keyboard back, so on the base the keys went straight to it. Now lane 1's creation has no early view and the keyboard stays: the keys are held on any build, and the builds differ only in where the hold delivers them. Every test checks first which terminal got which line, then how. - wait-for's timeout threw (line 141); a timeout is now the answer "the line did not run there". - The Cmd-W guard required a pane from the second Cmd-D; on the base that Cmd-D made none (the focus stayed on the closed stand-in, a defect of its own, reported apart). The guard now checks only what it guards: the closed split is gone, the pane it was typed from stays, and no key reached another terminal. * test(app): a held Control shortcut runs on its own terminal's pane; a replayed creation keeps the other terminal's keys (P0 1193 follow-up) Review findings 1 and 2 of #29301 (receipt at 201b1e4): only Command chords of a non-focused lane went through app dispatch, so a held Ctrl-` was typed into that terminal as a byte; and when a replayed key opened a new creation, prepend gave every later key the new creation's lane, so keys held for another terminal ran in the new split. Red on 201b1e4. * fix(app): a held chord of any kind runs on the terminal it was typed for; a replay keeps each key's own terminal (P0 1193 follow-up) Review findings of #29301 (receipt at 201b1e4): 1. A held Control chord (Ctrl-`, Ctrl-Tab, Ctrl-1..9) of a lane whose terminal is not focused was typed into that terminal; only Command chords went through app dispatch. Now every chord (Command or Control) goes through it with that terminal's tab as the target and its key context; a chord that is no app shortcut (Ctrl-C) is still that terminal's key. 2. When a replayed key opened a new creation, prepend gave every later key the new creation's lane. Now each key keeps the terminal it was typed for (HeldKey carries its lane, also .run); only the keys of the issuing key's own lane move to the new creation (typed after that lane's Cmd-D). Keys of another hold keep their terminal in the new hold (adopted lanes). 3. A split's held keys record the split pane's machine (PaneHandlers passes daemon.machineID), not the window's, so the daemon path reaches a remote split whose view was evicted. Tests: SecondCreationKeysTests.aHeldControlShortcutRunsOnTheTerminalItWasTypedFor, aReplayedCreationKeepsTheOtherTerminalsKeys. * test(app): held keys never run into another view that has the keyboard when the hold ends (P0 1193 follow-up) Coordinator 17:2xZ, from #29530's review: the claim 'the 2 s bound counts from the hold's start' does not hold (a pending creation keeps its keys with no bound; the bound counts from the reply). The real gap: at the hold's end the focused lane's keys run again as typed into whatever has the keyboard, so a text view that took the keyboard while the focus names the new terminal gets the line. Red on 307d8e1. * fix(app): at a hold's end a key runs as typed only into its terminal when that terminal has the keyboard (P0 1193 follow-up) Coordinator 17:2xZ (from #29530's review): the focused lane's keys ran again through NSApplication.sendEvent into whatever had the keyboard, so a text view that took the keyboard while the focus named the new terminal (or a hold that ended at its bound after the reply, the focus not landed) got the keys typed for the terminal. Now the hold's end names a focused terminal only when its shown view has the keyboard (keyboardTerminal, the check the early view already used); otherwise every key of that lane is typed by the terminal's own encoder. Keys typed for no creation run as before. Test: SecondCreationKeysTests.heldKeysNeverRunIntoAnotherViewWhenTheHoldEnds (red on 923890f). * test(app): the hold-end case is a remount, not a text field; the replay test reports what the held Cmd-D did (P0 1193 follow-up) Red step 5eaa419c578b1eb69e40b3ce (923890f): heldKeysNeverRunIntoAnotherViewWhenTheHoldEnds could not set up its state (a text view that takes the keyboard moves the focus model to it: 'saw focus textField'), so the keys already went to the terminal. Its case is a terminal view out of the window (a remount) when the hold ends at its bound: heldKeysReachTheirTerminalWhenItsViewIsOffAtTheHoldsEnd. aReplayedCreationKeepsTheOtherTerminalsKeys failed with the finding-2 fix too ('the held Cmd-D made no pane', the line not in the second terminal); its report now names the router's last action, the refusals, the hold and the panes, to find why. * test(app): heldKeysReachTheirTerminalWhenItsViewIsOffAtTheHoldsEnd records the issue, then returns (compile 03dd1fc3) * test(app): SecondCreationKeysTests imports CmuxNextTerminal for TerminalSurfaceView (compile 7ac95c03) * test(app): SecondCreationKeysTests start from a quiet create chain; the replay test reports whether the held split's work ended (P0 1193 follow-up) Green af9e85983193c69b0d21b551 (afe87dd): aReplayedCreationKeepsTheOtherTerminalsKeys still fails: after 20 s a hold is open, no refusal, and the held Cmd-D's split stand-in (handle >= ProvisionalPane.firstHandle) is not confirmed, so that split never got its reply; the line typed for the second terminal waits in its hold (a pending creation has no bound). A split's request first waits for the workspace's create chain. The setup now waits (10 s at most) for that chain to end and says so if it does not; the replay report says whether the held split's work ended. * test(app): the replay test waits for the held split from the app before it reads a screen (P0 1193 follow-up) Green 9e51a7799b581c49a599666e (bbf38e5): the create chain was quiet, and at the end the held Cmd-D's split pane was confirmed with no hold open: the split DID end, but more than 20 s late. The daemon runs a connection's requests in order on one dispatcher thread (ConnectionSurfaceScheduler.dispatch / next_runnable_index), and the test's 20 s wait-for for the second terminal's line ran on the app's own connection: it held the split's request back, while the line waited for that split. A rig cycle, not a product stall. The test now waits for the split's pane and the end of its hold from app state first. * test(app): keys held for an evicted terminal arrive in the order they were typed (P0 1193 follow-up) Review of #29693 at bd1112a (defect 1, receipt FIX): a Control chord that is no app shortcut (Ctrl-C) became its own .command and its own daemon send, which can pass the send before it when the terminal's view is gone: "sleep 5", Return, Ctrl-C could arrive as "sleep 5", Ctrl-C, Return. The test evicts the first terminal's hidden view, so only the daemon types the held 'ab', Return, Ctrl-C, and reads the bytes the shell got. Red on bd1112a. * fix(app): a held chord that is no app shortcut stays in its terminal's typing run, so its keys keep their order (P0 1193 follow-up) Review of #29693 at bd1112a (defect 1): take(focused:) made every Control chord a .command; one that resolved no binding then went out by runCommand -> type as a separate daemon send, which can interleave with the send before it (DaemonService.send starts a Task per call; the connection takes new work while a request waits). Now take(focused:) asks the coordinator whether a chord runs an app shortcut in its terminal's context (shortcut(_:tab:in:), the lookup runCommand already made); only those become .command, and every other key of the lane stays in its .type run (one send per run). Also the comments the review named: HeldKey cases, runCommand, the bound-end log. Test: SecondCreationKeysTests.heldKeysForAnEvictedTerminalKeepTheirOrder (red on 149a88c). * test(app): the evicted-terminal order test hibernates the hidden workspace, the product path that frees its views (P0 1193 follow-up) Red d9560494d32c52f5f9b93ab0 (149a88c) stopped in setup: evictHiddenTerminal leaves a view a pane still presents, and the hidden workspace's pane still presented it. AppServices.hibernateWorkspace unmounts the workspace first, then frees its surfaces. * test(app): the evicted-terminal order test reads the screen without a blocking wait (P0 1193 follow-up) Red 6317fcf34f4610341c20d846 (19685b4) and green 9f791d638a2329763877196c (1df5e5c) failed alike: the byte reader got no byte in 20 s ('R42x...' and no result line). The same rig cycle as test 8: resolve flushes in a later turn, so the test's 20 s wait-for went out first on the app's connection, whose requests run in order, and the held keys' send-key waited behind it. The test now waits for the hold to end from app state, then polls read-screen (which returns at once and runs after the sends before it). * test(app): the byte-reader checks join the screen's lines before they match (P0 1193 follow-up) Green 83cae9e44f1555ab3c44ab80 (c8bac42) failed eachHeldKeyRunsAtItsPlaceInItsOwnTerminal at :304 with the right bytes on screen: 'K42x4027896c731b' / '5b4103K', the result line wrapped in a narrow pane, so the daemon's wait-for regex did not match across the line break. The three byte-reader checks (tests 3, 4, 10) now poll read-screen and match on the joined lines (readerGot). * test(app): closing a replay-opened split keeps the line carried for another terminal (P0 1193) Re-review of #29693 at bc84781 (lower finding 1, which I wrongly closed as unreachable in #29758): a held Cmd-D that runs again at a hold's end opens a new hold with only that split's creation (its awaited set is empty) plus the keys carried for other terminals; a Cmd-W on that split before its reply dropped every key, also the carried line. Red on #29693's head bc84781. * fix(app): closing a split drops only the keys with no terminal of their own, never a line carried for another terminal (P0 1193) CreationInputBuffer.cancel(tab:) dropped every held key once nothing waited (514770e, from before keys had lanes). A hold that a replayed Cmd-D opened has an empty awaited set, so a Cmd-W on that split before its reply also dropped the lines carried into it for other terminals. Now, with nothing waiting, only keys with no lane or a lane with no tab go with the close (they have no terminal of their own and must not run into whatever has the focus); a key whose lane names a terminal stays and goes to it when the hold ends (the coordinator flushes after a cancel). This differs from the closed #29758, which also kept tab-less lanes. Test: CreationInputHoldTests.closingAReplayOpenedSplitKeepsTheLineCarriedForAnotherTerminal (red on c313b63). * test(app): two held runs for one evicted terminal arrive in the order they were typed (P0 1193) Re-review of #29693 at bc84781 (lead 00:2xZ: same input-order class as the P0): each held .type run for a terminal with no view is its own daemon send (a Task), and a run of several requests ('ab', then Return) can let a later run of the same terminal ('cd') in between them. Red on #29693's head bc84781. * fix(app): held keys for a terminal with no view go out through one ordered send queue per terminal (P0 1193) Each held .type run for a terminal whose view is gone was its own DaemonService.send (its own Task). A run of several requests ('ab', then Return) awaits a reply between them, and the connection actor takes new work meanwhile, so a later run for the same terminal ('cd') could run between them: the shell got 'ab', 'cd', Return. DaemonService.sendInOrder chains every ordered send to one surface after the one before it (a per-surface queue; the ticket opens at the call, a failure is logged and the next send still runs; the entry clears after the last send). The held-key daemon path uses it. It lives in CmuxNextDaemonService, so CmuxNextApp does not grow. Test: SecondCreationKeysTests.twoHeldRunsForAnEvictedTerminalKeepTheirOrder (red on fd0d7c3). * fix(app): an ordered send keeps its service alive, so its ticket closes and a lost send is reported (P0 1193) Review of #29919 at a51d0a8 (FIX, REVIEW.md rows 9 and 10): sendInOrder's Task captured the DaemonService weakly and returned nil (success) when it was gone at its turn: the held keys were lost with no report, and the ticket opened at the call never closed, so an action scope waited for its control timeout. Remote services are released (Cloud, SSH, Server sessions) and held keys can go to remote terminals. Now the Task keeps the service, as send(_:_:) does: failure(...) closes the ticket and reports 'not connected' when the connection is gone. No red test: it needs a DaemonService released while a send waits in its queue, which the app test harness cannot make deterministically; the change is the same capture send(_:_:) has used. * test(app): an ordered send whose service is gone fails and closes its ticket (P0 1193) The same test as the red ref fix/p0-1193-followup-ordered-release-red (25a36d3, a51d0a8 + the test only), on the head that already has the fix (a6f62ea, pushed before the test). Red on 25a36d3. * test(app): OrderedSendReleaseTests imports CmuxNextActions for ActionWork (compile 456a0d92) --------- Co-authored-by: cmux-bot[bot] <348531757+cmux-bot[bot]@users.noreply.github.com>
This branch was previously deployed
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
P0 1193 family (lead 00:2xZ, from the re-review of #29693 at bc84781): two typing runs held for one terminal whose view is gone could reach that terminal out of order. Each held
.typerun for a terminal with no view is its own daemon send. One run can need several requests (text, then a key such as Return), and between them it awaits a reply; the connection takes other work meanwhile, so a later run for the same terminal ran in between. With "ab" + Return held, then a key for another terminal, then "cd", the shell got "ab", "cd", Return.Root fix: one ordered send queue per terminal.
DaemonService.sendInOrder(_:to:_:)(CmuxNextDaemonService) runs each send to a surface after the one before it. The ticket opens at the call, assend(_:_:)'s does; a failed send is logged and the next send still runs; the queue entry clears after the last send. The held-key daemon path (CreationInputCoordinator.type) uses it. The queued task keeps its service alive, assend(_:_:)does, so its ticket always closes and a send whose connection is gone is reported as "not connected" (review FIX at a51d0a8). CmuxNextApp: -10 lines.Base: stacked on #29693's branch
fix/p0-1193-followup-v2; after #29693 merges, the base moves tofeat-cmux-next(no rebase).Testing
Red-first:
SecondCreationKeysTests.twoHeldRunsForAnEvictedTerminalKeepTheirOrderfor the order fix (two creations with no early view; "ab", Return for terminal A, "x" for B, "cd" for A; A's workspace hibernates so only the daemon can type; A's shell must get 61 62 0d 63 64).OrderedSendReleaseTests.anOrderedSendWhoseServiceIsGoneFailsAndClosesItsTicketfor the review FIX (two sends queued in aDaemonCommandScope, the service released before their turn; each must fail and close its ticket).HeldKeyInputTests3 of 3OrderedSendReleaseTestsonly (review FIX red)[nil, nil]), the scope stayed busy (ticket open), 0 failures recorded (want 2)OrderedSendReleaseTests1 of 1;SecondCreationKeysTestsran 3 times, 11 of 11 each;HeldKeyInputTests3 of 3Display steps (console session,
CMUX_TEST_REQUIRE_GUI=1), each execution counted in the log. Compiles: head bb40d2709adffee001fb9294, red 4f7623c4b97910fc88db196b. Review-FIX compiles: head 74988f2532c6851464f29e56, red 33aaa7536438d8d83cb87c54.Observed / Not run
Observed: the runs above. Not run: Other writers to the same terminal (its view's attachment path,
TerminalSession.sendBytes, thesend-keyinTerminalHooks) are not in this queue; they need a live view, and were unordered with the oldsendtoo (follow-up). Other callers ofDaemonService.sendthat write to one surface (they keep their old path; only held keys use the queue now).Performance
No perf numbers: a correctness fix. A send to a surface now waits for the earlier send to the same surface (one reply round trip); sends to other surfaces do not wait. No work moves onto the main thread. perf claim: none.
Changelog
Fixed: Keys typed for a pane while new panes open arrive in the order they were typed, also when that pane is hidden
Proof
No capture: key order inside the daemon path; the evidence is the fleet tests above.