Repository navigation
test(split): #28436 was the test window, not the key path: type after the hold, and cover typing during it - #28810
Conversation
…(P0 new terminal) Fleet repro on v2.2.9-opt (cmux17, cmux20, cmux21): Cmd-T then Ctrl-`. The new-tab missed the client's 5 s start deadline behind the New Tab page's new-conversation-tab, and the app took the Ctrl-` stand-in back. Red on the base: newTab throws terminalStartTimedOut at the client deadline.
…in back on a client clock (P0 new terminal) v2.2.9-opt P0 (Lawrence): at times Ctrl-` made no new terminal. Fleet repro (4/4 on cmux17, cmux20, cmux21): Cmd-T, then Ctrl-`. The New Tab page's new-conversation-tab got no reply for seconds; the new-tab queued behind it on the app's connection missed the 5 s spawn deadline; DaemonService rejected the intent and the stand-in vanished, while cmux-tui still made the tab. The keys typed for the new terminal went to the old one. A new-tab with terminal-placement-env-v1 carries the stand-in's client-chosen terminal id, and cmux-tui does not replay a new-tab sent again (splits re-send with their client keys; new-tab cannot). So it now waits for its reply or the connection's end (LineTransport fails pending requests on close): the daemon's outcome decides, no client deadline does (principled-audit R5, R-L5). The shell integration arguments are added through one helper for both paths (R1). Regression test: NewTabLateStartTests (red on the base at 7340d85).
…n a client deadline Ctrl-` and Cmd-D must always give a terminal that takes keys (principled-audit R3, P0 2026-10-30). The one rollback point of every intent (DaemonService.intendNowReporting) now stops DEBUG builds when it takes a terminal stand-in (a New Terminal Tab's provisional tab, a terminal split's pane) back for a client deadline: only the daemon's answer or the connection's end may do that. TerminalStandInRollbackInvariantTests pins the judgement.
…mmand), not an app-internal failure type
… back (P0 review)
…its like a split, the late-start wait has no give-up Review of the P0 fix (FIX at 9dc00b5): the DEBUG R3 check would stop the app on two paths that still rolled a terminal stand-in back on a client clock: - New Row (row-client-keys-v1) sent new-row with the 5 s start deadline: it now waits through a late start under its client keys, as a split does. - The keyed late-start wait (splits, columns, panes, create-terminal) gave up after a fixed 120 s budget and on cancellation threw the timeout: it now ends on the reply, the daemon's refusal, the connection's end, or CancellationError (principled-audit R5, R-L5). The daemon replays each keyed pass, so a re-send changes nothing. Also: the newTab doc moved above its attribute. Regression test: NewRowLateStartTests (red at the commit before this one).
…d for, never closed (P0 R3)
…lies; R3 covers every new terminal Lead decision on the P0: Cmd-N is a new terminal too. Its create-workspace asked again only until a fixed 10 s budget, and its first create-terminal was sent once with the 5 s start deadline; either failure closed the workspace the person already saw (WorkspaceCreation.withTerminal) while the daemon still made it. Both steps are keyed (mutation id; terminal id + mutation id) and the daemon replays them, so both now wait through late replies until the reply, a refusal, the connection's end or cancellation (R5, R-L5): - DaemonConnection.createWorkspaceWaitingForReply, createTerminalWaitingForStart(_:) and one shared loop, waitingThroughLateReplies. - WorkspaceCreation.create (every workspace with a terminal) uses them; its rollback is checked in DEBUG (DaemonService.checkTerminalRollback). - The R3 predicate covers .createWorkspace with a terminal tab. Regression test: WorkspaceWithTerminalLateCreateTests (red at the commit before).
…al; R3 checked where the wait ends (review FIX at 70e92ae) Review: WorkspaceCreation.create's DEBUG check judged every error of the caller's terminal closure, but four callers still made their first terminal with the 5 s start deadline (Open Folder / createAndShow and its callers, agent fork, agent teams, incognito window), and a later step's 2 s timeout (configure, incognito tab) would also have tripped it: DEBUG builds would stop there. - Those callers make their first terminal with createTerminalWaitingForStart (keyed, replayed by the daemon), as Cmd-N does. - The R3 check moves to the two waits themselves (requestWaitingForStart, createWorkspaceWaitingForReply): DEBUG stops if either ever ends on a late start, which their loop never does. A caller's own later step is not judged. - WorkspaceWithTerminalLateCreateTests also makes the first terminal start late (the workspace part's red is ceac05e).
…e first-terminal path for every new workspace Lead decisions on the P0 (R-L4, R1): - waitingThroughLateReplies keeps waiting with no end, but a re-send (it only repairs a lost request; the daemon replays keyed creates) is spaced by full-jitter exponential backoff from 1 s, capped at 30 s (Backoff.fullJitter, the shared retry type), not at the fixed 2 s / 5 s deadline: no retry storm on a hung daemon. Cancellation ends it during the backoff wait. - WorkspaceCreation.firstTerminal is the one way the first terminal of a new workspace is made (Open Folder, fork, teams, incognito); the test uses it. - Tests: the scripted daemons of NewRowLateStartTests and WorkspaceWithTerminalLateCreateTests end their start once and replay later asks at once, as cmux-tui does (before, each ask was answered a fixed delay after it arrived, which no deadline shorter than that delay could ever see). LateReplyBackoffTests pins the spacing.
…ate-reply backoff)
…ane_id_exists), as cmux-tui does (third review)
…s not replay new-row (third review FIX at e4e6774) The third review found that a keyed new-row sent again after a late start is refused pane_id_exists by cmux-tui (mux/rows.rs commits it unkeyed; resource_topology/client_ids.rs refuses a held pane id), so the re-send made the app take the row back on that refusal. New Row now goes the way new-tab goes: sent once, answered by its reply or the connection's end (requestAwaitingStart, R5, R-L5). requestPaneCreation is again split, new-pane, new-pane-right only. Regression test: NewRowLateStartTests (its daemon now refuses a second ask; red at the commit before this one).
…pane (P0; bench-arms 02dcba2e: 12/12 made none)
…e, not its stand-in Lead P0 evidence (bench-arms 02dcba2e1c9e447c7cad0594): in a quick pair of splits the second Cmd-D / Cmd-Shift-D made no pane within 5 s, 12 of 12. Cause: the second split targets the focused pane, which is the first split's stand-in (ProvisionalPane), and PaneSplitCommand sent that stand-in's handle, which the daemon does not know; the refusal took the second pane back. New Column and New Row already resolve the target at send time and run in the workspace's create order; the split did neither. - PaneSplitCommand resolves its target at send time (DaemonStore.daemonHandle(showing:)) and waits until the store has applied its own reply, as NewColumnCommand / NewRowCommand do. - PaneHandlers.split's optimistic path joins the workspace's create chain (WorkspaceContentController.pendingCreate), as createInKeysTurn does: it goes out once the create before it is in the store (R1, one path). The wait is the earlier request's own outcome, no timer (R5). Regression test: SplitOfPendingSplitTests (red at the commit before).
… hop): pendingCreate is an ActionWork
…d-D and checks it runs in that pane's shell (the P0 invariant: a terminal that takes keys)
…aceWithTerminalLateCreateTests imports it)
…eads main-actor pane state)
…-keys-v1 as cmux-tui does (the app sends a row's client keys under it)
…o stand-in; Ctrl-` on a pending split's pane opens there and takes keys (P0 rules a, b)
…es for the daemon (P0 rules a, b)
cmux-ad's two rules for the P0, both in this PR:
(a) A refusal is never silent. A person's optimistic create (Cmd-D split,
Ctrl-` New Terminal Tab, New Row, New Column) the daemon refuses rolls its
stand-in back AND shows the refusal where refusals show (refusal HUD) with a
log line: ActionRegistry.trackRollingBack, the legacy handlers' form of the
ActionEffect.applied failure path (remoteFailureObserver). A create the
person closed first (PaneSplitCancelled) is not a failure.
(b) A stand-in id never leaves for the daemon. Every create that targets a
pane resolves it at send time with DaemonStore.daemonTarget(_:) (the daemon's
pane that took the stand-in over; PendingPaneNotCreated when its create was
never made) and runs in the workspace's create chain (pendingCreate), so it
goes out after the create that makes its target: split, New Column, New Row
and now New Terminal Tab. The New Terminal Tab overlay on a stand-in follows
it to the daemon's pane (ProvisionalSplit.rememberStandInTarget/shownPane).
DEBUG builds stop at a typed request that names a stand-in pane or surface
(DaemonRequestExecutor.standInReference).
Tests: RefusedCreateIsNeverSilentTests, NewTerminalOnPendingSplitTests (red at
the commit before), StandInReferenceTests.
…PendingPaneNotCreated, 21 locales); the English text stays for logs Lead decision: the refusal HUD is this PR's visible error; its text reaches the person, so PendingPaneNotCreated maps to MiscHandlerStrings.pendingPaneNotCreated (MiscHandlers.xcstrings: en and ja translated, the other 19 locales needs_review as the table's other keys) in ActionWorkFailure.create, the one failure builder of Cmd-D, Ctrl-`, New Row and New Column. Test: CreateFailureTextTests.
… main-actor strings; every caller is main-actor)
…@testable import (as other app tests do)
…eation hold ended (a test window is never key, as the journeys' outwait_hold) and says where a lost line went Green 87ad5a04 at 98b377d: 10 of 11 suites passed; this one failed at the typed line (wait-for timeout), every earlier check holding (two terminals in the confirmed pane, the new one selected, focus and responder in the pane). Feat's creation hold replays held keys with NSApp.sendEvent, which reaches only the key window. CreationInputCoordinator.isHolding(in:) (as perf has it) lets the test type after the hold; on a miss it records each terminal's screen, the responder and the hold.
…Reached | focusLeft | windowClosed) and logs a limit end; NewTerminalOnPendingSplitTests asserts focusLanded Lead decision (R-L5: a hold's end is a signal): waiting in the test for the hold to end could hide keys a person feels up to 2 s late. The hold now records its end per window (CreationInputCoordinator.lastHoldEnd(in:)): unforced means every creation replied and the focus it asked for holds with a responder that takes keys (focusLanded); the two bound timers are limitReached and log a notice (window, held key count, last action) so a person's log shows it; a key leaving the content is focusLeft. The test asserts the Ctrl-` on a pending split's pane ended its hold with focusLanded before it types.
…hat resolve their target; other verbs log a fault (review #4 B1); a failed create's hold ends as creationFailed (N1) Review #4 (FIX at 670131c), B1: DaemonRequestExecutor stopped DEBUG builds on every request naming a stand-in, but only the terminal creates (new-tab, split, new-pane, new-pane-right, new-row: TerminalSpawningRequest) resolve their target with DaemonStore.daemonTarget; zoom, rename, swap and a New Tab page on a pending pane still send the stand-in, which the daemon refuses ("unknown pane") as before. The assertion now covers those creates only; other requests log a fault. A seam that resolves every pane verb is a follow-up; rule (b) in the PR body is scoped to the creates. N1: a hold whose creation failed ended as focusLanded; it is creationFailed now. daemonTarget's doc says which stand-ins it covers (N3).
… opening' notice, sends nothing, zooms nothing (P0 rule b, lead decision)
…never key (#28436) The diagnostic runs found the cause of the missed line. At 9adddb3 (job eae2fd94), 6 of 8 runs typed while the window's creation hold was still open: the hold took all 24 keys, ended with focusLanded, and replayed them through NSApp.sendEvent, which reaches only the key window. The test window is never key and the test host's app is a plain NSApplication, so no terminal got a byte (no input attach record, both screens blank). The product replays into the key window, the window the person types in, so this is the rig, not a key path race. NewTerminalOnPendingSplitTests already waits for the hold for the same reason. The test now waits for the hold to end, expects it ended because the focus landed, and keeps the line-runs-in-its-pane assertion and the miss report.
…Cmd-D runs in the second pane (#28436) The person's path: the keys are typed at once after the second Cmd-D, while the hold of both splits waits. The test expects some keys held, the line to run in the second pane's shell, and the hold to end on focusLanded. CreationInputCoordinator.replay is the hold's replay, NSApp.sendEvent as before (to the key window); the test gives the window it types into, because a test window is never key. isHolding and hasPendingCreation become one-line expressions (same result) to pay for its lines (SW14). Correction to a87ac4f's message and comment: 5 of 8 runs (not 6) at 9adddb3 typed during the hold (job eae2fd94: 3 passed, 5 failed).
GitHub Actions
Error
…plit-keys # Conflicts: # Packages/macOS/CmuxNext/Tests/CmuxNextAppTests/SplitOfPendingSplitTests.swift
GitHub Actions
Error
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
|
Review (independent review subagent, Claude Opus 5.5): SHIP at 16a5309 I read the merge-base diff (3 files), the PR body, Findings
Checked / not checkedChecked: the diff, the call sites, the production key path, the test lifecycle, the journal's global state, and the counts in the run-3 log. Not checked: I did not compile or run anything. I did not check the fleet job results in the PR's table, the god-file size (SW14), or the behavior in a real key window (GUI). I did not check the pre-existing edge cases that this PR does not change, for example a Chromium child window of the same owner that is key at flush time. |
|
cmux status (one comment, edited in place; each line is updated by the job that owns it) Review App of this head (app.zip, kept 1 day)
red: cmux-next checks (god files, concurrency, crash safety, l10n); NEW: Shipped in:
|
… passes in, with a red form of the during-hold test (#28436) cmux-ad item 884. CreationInputCoordinator.replay was a settable property; it is now HeldKeyReplay, a closure passed at init (PATTERNS P1): AppDelegate passes NSApp.sendEvent to AppServices, AppServices to KeyRouter, KeyRouter to the coordinator. The app's behavior is the same: each held key runs through NSApp.sendEvent, to the key window, inside the same replaying flag and re-entrancy checks. Tests: the test AppServices init and PinnedPageTests.withApp take the replay (the app's by default); a KeyRouter(registry:) test init passes the app's. The during-hold body is one function run twice: - aLineTypedDuringTheHoldRunsInTheSecondPane: the replay runs each key as the app does (key router, then window) into the test's window, which is never key; the line runs. - aReplayThatDropsTheHeldKeysFailsTheCheck: the red form, a replay that drops the keys (the shape of #28436). The check must fail on its own assertion (the line missing from the second pane); withKnownIssue matches only that assertion, and fails if it passes. CreationInputCoordinator.HoldEnd moves to CmuxNextFocusUI as CreationHoldEnd (same cases), so the App does not grow (SW14). The during-hold body also checks the first split was still pending at the second Cmd-D and that no split was refused (review nit at 16a5309).
GitHub Actions
Error
GitHub Actions
Inactive
|
Found 9 test failures on Blacksmith runners: Failures
|
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
….sendEvent); test-target compile fixes 4f78e2c broke the test target: every KeyRouter(registry:) test call needed the new parameter, and its test convenience init did not import ActionRegistry. KeyRouter's heldKeyReplay now defaults to the app's replay, so those callers stay as they were; only the composition root (AppServices, from AppDelegate) and the hold tests pass one. The convenience init is gone. The red-form test is async throws (withKnownIssue rethrows).
GitHub Actions
Inactive
GitHub Actions
Inactive
|
Review (independent review subagent, Claude Opus 5.5): SHIP at 435d465 I did a read-only review of FindingsNo blocking findings. Correctness is good: the app path is the same as before. Low items, all non-blocking:
What I checked
Not checked
|
lawrencecchen
added
the
ready-to-land
Summary
#28436 ("a line typed after a quick double Cmd-D can miss the new pane") was a test rig fault. It was not a product race.
SplitOfPendingSplitTeststyped while the window's creation hold was still open. The hold kept the keys, and when the focus landed it replayed them throughNSApp.sendEvent.NSApp.sendEventdelivers only to the key window, and a test window is never key, so no terminal got a byte. When a person types, the replay goes to the window the person types in, because that window is the key window.The test now waits for the hold to end before it types (as
NewTerminalOnPendingSplitTestsalready does), and it expects the hold to end onfocusLanded.A new test,
aLineTypedDuringTheHoldRunsInTheSecondPane, follows the person's path: it types at once after the second Cmd-D, during the hold. It expects these:focusLanded.Its replay runs each key the way the app does (the key router first, then the window) into the test's window, because a test window is never key.
Its red form,
aReplayThatDropsTheHeldKeysFailsTheCheck, runs the same body with a replay that drops the held keys. That is the shape of P0: a line typed after a quick double Cmd-D can miss the new pane (rare race, after #28331) #28436:NSApp.sendEventwith no key window delivers the keys nowhere. The test wraps the body inwithKnownIssue { ... } matching: { the comment contains "did not run in the second pane" }, with noisIntermittent. The red form is strict for two reasons:withKnownIssueblock records no issue, Swift Testing records a failure. So if the dropping replay ever lets the line through, this test fails.The hold's replay is a seam passed at init (PATTERNS P1, cmux-ad item 884). The closure type is
HeldKeyReplay.AppDelegatepasses{ event, _, _ in NSApp.sendEvent(event) }toAppServices,AppServicespasses it toKeyRouter, andKeyRouterpasses it toCreationInputCoordinator(private let replay).KeyRouter's parameter defaults to the sameNSApp.sendEventcall, so its existing test callers do not change. There is no settable property, no static and no global. The app's behavior is the same: each held key still goes throughNSApp.sendEventto the key window, with the samereplayingflag and re-entrancy checks.CreationInputCoordinator.HoldEndmoves toCmuxNextFocusUIasCreationHoldEnd, with the same cases. With that move, and withisHoldingandhasPendingCreationnow one-line expressions with the same results, CmuxNextApp does not grow (SW14).When the typed line does not show, the test now records where it went: every pane's screen, how the hold ended, each terminal view's gate and keyboard, each key's path, and the input journal.
Evidence
Diagnostic run 3, job
eae2fd9426eb367df8bf1803(head 9adddb3, cmux18-perf), had 8 attempts: 3 passed and 5 failed. Each failed attempt reported:focusLanded, so the keys were replayed.inputattach record for any terminal. The replay reached no window, becausewindow key false.Diagnostic run 2, job
cd8cdc7cb8cb5af49528587f(head bee8e76, cmux23-perf), had 5 attempts: 3 passed and 2 failed. The report did not record the key path yet. Its 2 failed attempts show the same state:lastHoldEndwasfocusLanded, the second pane had the keyboard, both screens showed onlysh-3.2$, and the journal had noinputrecord. Run 1, job4bb1096c16feda23e4637ccf, had 1 attempt, and it passed.The person's path: a person's window is the key window. In product code,
CmuxApplication.sendEventpasses each key toKeyRouter.interceptKeyDown, andCreationInputCoordinator.captureholds the key. When every creation of the hold has replied and the focus landed with a responder that takes keys,flush(in:)replays the held keys in order throughNSApp.sendEvent, which delivers them to the key window.aLineTypedDuringTheHoldRunsInTheSecondPanecovers this path. It replays into the test's window, which the rig cannot make key.Testing
Each count is per attempt. A FLAKY result counts as a failure.
aSecondSplitOnAPendingSplitsPaneMakesASecondPaneaLineTypedDuringTheHoldRunsInTheSecondPanee55adfd211649348f4325973NewTerminalOnPendingSplitTests1 pass)138dbc39987611790c6570c42416056e7ad4b150b552c02253258233ceea53bea7212460fe72b9b00893f7be77443e7ca26206d636f911feb6e69be27b4d9383bc2d1e7e2c0bb6920d24d13186268d3cc6e82155745851a36dfa78263b76f1fc8cc154b06ceee4f05c504618Red form
aReplayThatDropsTheHeldKeysFailsTheCheckat 435d465: in each of the 25 attempts (1 + 12 + 12), the check failed on its own assertion, as the red form requires:Line 89 is the
Issue.record("the line typed during the hold ... did not run in the second pane ...")of the shared body. In each job, the same process ran the red form and the real form with the same keys and the same hold; only the replay was different.At 4f78e2c, jobs
3a80f141fbe1d98d28777b40andecaf648482fc6decab4e2afadid not compile the test target (the new init parameter had no default yet), so no test ran. Those two jobs count as neither a pass nor a fail. Release app compile8acde2072db8f9a0885cdd56at 4f78e2c passed.Two jobs at e431de4 (
04f59f971280f46394aa4429,27608ab5cdbd03c17ee2ae19) stopped before any test ran. They hit a base red that is not from this PR: "generated file is stale ... Native/OptChat/optchat-chief/inspector/index.html". e20297d fixed it on feat-cmux-next, and 16a5309 merges that fix in. Those two jobs count as neither a pass nor a fail.The rewritten test passed 96 of 96 attempts. Before this change, with the #28331 fix, it failed 5 of 20 (and 7 of 14 in the diagnostic runs above). The during-hold test passed 73 of 73, and in each attempt it expects that at least one key was typed during the hold.
Each command:
cmux-ci run --class light --pool perf-lead --script scripts/ci/package-test-lane.sh --ref <head> --arg=suite --arg=Packages/macOS/CmuxNext --arg=SplitOfPendingSplitTests ...(the suite named once per attempt).Observed / Not run
Observed: the fleet jobs above, and the prepush gate (no new violations) on each push.
Not run:
scripts/cmux-next/typeahead-gate.sh ... split). I read the product's replay path, but this PR does not run it in a key window.NSApp.sendEventpath. The red form drops them in the injected replay instead, which is the same effect that run 3 recorded: the replay reaches no window.Follow-ups (from the review at 435d465, non-blocking)
KeyRouter'sheldKeyReplaydefault (NSApp.sendEvent) is a fallback in production source (P1). The only production construction (AppServices) passes the root's replay. Follow-up: remove the default and give the tests a test-only init. Also makeheldKeyReplayprivate.AppServices+TestKeyWindow.swift:9,CreationInputCoordinator.swift:18).Changelog
none
Closes #28436.