Repository navigation
P0: Ctrl-` right after Cmd-N shows its terminal in the daemon's pane, not an empty pane (display 0e91fb59 at 88341f28932) - #36215
Conversation
… over shows the terminal the stand-in showed (P0 STATUS 13438, red) Cmd-N, then Ctrl-`: the stand-in pane selects and shows its stand-in terminal; then the daemon's pane takes the stand-in over under another layout id and the window makes it a new pane controller. The test holds the Ctrl-`'s create behind a gate until that controller mounted, so the order is fixed on every run: no daemon terminal and no focus expectation can show the terminal there first. It checks that the daemon's pane selects and shows the person's terminal from the turn it mounts, then that the daemon's terminal shows there when the create goes on, and that the stand-in's controller, out of the layout, shows nothing more.
…over selects and shows what the stand-in showed (P0 STATUS 13438) Cmd-N's stand-in pane and the daemon's pane that takes it over have different layout ids, so the window makes the daemon's pane a new PaneController, and nothing moved the stand-in's view state to it: it started from the daemon's default tab (the New Tab page) while the person had selected the Ctrl-`'s terminal on the stand-in. The terminal then showed only if the focus expectation landed later, and the stand-in's controller, already out of the layout, still ran the create's end (selectWhenReported) and showed its own New Tab page, taking that page's view from the daemon's pane: the terminal tab selected over an empty pane for 20 s. The hand-over itself now carries the state, with no retry, timer or second show: - DaemonStore.daemonPane(takingOver:) names the daemon pane that took a stand-in over (the store already knew it for daemonTarget). - The daemon pane's controller, made while the stand-in's is still mounted (a new screen takes its panes before the old one goes), takes the stand-in's selected tab (the daemon's tab in its place once listed) or a select still waiting there as its own pending select, so its first apply selects and shows it. - A create made on the stand-in that lands after the hand-over selects in the daemon pane's controller (successor), never in the stand-in's, and the Ctrl-`'s end reads that controller's strip.
…the other creates on a stand-in read the daemon pane's strip Review of 7a26c7a: - The daemon pane takes the stand-in's waiting surface select, else its waiting tab select or selected tab, never both: apply uses the surface first and would leave a tab select set for a later apply. - A new stand-in pane skips the walk of the workspace's panes (it takes nothing over). - The browser tab opener and a remote chat's create, which now forward their select to the daemon pane after Cmd-N's hand-over, read that pane's strip and waiting select first, as the Ctrl-` create does: a tab the person picked there after the hand-over keeps the selection.
…wn selection state; reviewed)
|
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)
Shipped in:
|
…n hand-over are a store value type (PendingTabSelect); no app_lines exception Lead on #36215: meet app_lines by moving the logic to its package. PaneController's two waiting selects (pendingSelectSurface, pendingSelectTab), the apply rule that takes them, and the hand-over carry are now CmuxNextDaemon.PendingTabSelect (take(shown:record:) and takeOver(_:selected:replacement:)), with unit tests (PendingTabSelectTests). PaneController keeps only the calls; CmuxNextApp is 1 line below the base, so the arch exception goes. No behavior change from 2c93f4b. The remote chat's successor read (AgentTabs+Lifecycle) goes back to the base: Cmd-N stand-ins exist only on the local daemon, so a remote create never has a successor there (review of 2c93f4b).
…n hand-over are a store value type (PendingTabSelect); no app_lines exception Lead on #36215: meet app_lines by moving the logic to its package. PaneController's two waiting selects (pendingSelectSurface, pendingSelectTab), the apply rule that takes them, and the hand-over carry are now CmuxNextDaemon.PendingTabSelect (take(shown:record:) and takeOver(_:selected:replacement:)), with unit tests (PendingTabSelectTests). PaneController keeps only the calls; CmuxNextApp is 1 line below the base, so the arch exception goes. No behavior change from 2c93f4b. The remote chat's successor read (AgentTabs+Lifecycle) goes back to the base: Cmd-N stand-ins exist only on the local daemon, so a remote create never has a successor there (review of 2c93f4b). (cherry picked from commit 88341f2)
|
Found 1 test failure on Blacksmith runners: Failure
|
|
Review (independent review subagent): SHIP at 88341f2 Read-only correctness review in three rounds: 7a26c7a (SHIP), 2c93f4b plus the arch exception at 464eea6 (FIX for one false phrase in the exception's
|
|
Base finding, not this PR: |
|
ready-to-land at 88341f2: Review (independent review subagent): SHIP at 88341f2 (receipt above); compile 45685209 and test-target compile a590e2c1 passed; green b642d397 (14 suites, 88 tests, 0 skipped), 10-round green 0c1c735e, display run 0e91fb59 (4 of 4, none skipped); red at 6c2716b (efa15d48) and 10 of 10 red rounds (88ed2f4f). Non-green checks: |
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
Summary
P0: Ctrl-
pressed right after Cmd-N could leave the new terminal tab selected over an empty pane. Two P0 journeys on cmux23 failed this way:r3-after-switch-traced(build cd6a94aa, feat 29efa5800072, STATUS 13438) andr3-e2-reattach` (build 742dc06b, feat 7e6f2ab, STATUS 13556). Both waited 20 s for a focused terminal. Their last frames show "New Tab" and the selected terminal tab "~" in the strip over a blank pane. With this change the daemon's pane shows the person's terminal from the turn it takes Cmd-N's stand-in over.Cause, from the traced journey's input journal: Cmd-N shows a stand-in workspace and pane at once, and the Ctrl-
selects and shows its stand-in terminal there. When the daemon's workspace arrives, its pane takes the stand-in over under another layout id, so the window makes it a newPaneController. Nothing moved the stand-in's selection or pending select to that controller, so it started on the daemon's default tab, the New Tab page. The terminal showed there only if the focus expectation landed later. Also, the stand-in's controller had already left the layout, but it still ran the create's end (selectWhenReported). It then showed its own New Tab page, which took that page's view from the daemon's pane (journal seq 134:show pane=pane:6917… from=- pending_tab_21f0…).r3-e2-reattach` recorded no trail, but its frames match the traced failure, so this fix covers both journeys.The fix makes the hand-over carry the state, with no retry, timer or second show:
DaemonStore.daemonPane(takingOver:)names the daemon pane that took a stand-in over. It uses the same lookup asdaemonTarget.CmuxNextDaemon.PendingTabSelect(it wasPaneController.pendingSelectSurfaceandpendingSelectTab).take(shown:record:)is the rulePaneController.applyused for them.takeOver(_:selected:replacement:)is the hand-over: the stand-in's waiting surface select, else its waiting tab select, else its selected tab (as the daemon's tab in its place, once listed), as one value. A second value would select later what nobody chose then.takeOverin its init, so its first apply selects and shows what the stand-in showed. A new stand-in pane skips this lookup.successor), not in the stand-in's. The Ctrl-` create and the browser tab opener read that controller's strip.The split, New Column and New Row stand-ins do not change. Their daemon pane keeps the same layout id and controller (the record is updated in place), and
daemonPane(takingOver:)is nil for a daemon handle.Testing
CmdNThenNewTerminalTabTests.theDaemonsPaneThatTakesCmdNsStandInOverShowsTheTerminalTheStandInShowedfixes the order the trail shows (PendingTabSelectTestscovers the value type's rules):. A gate on the workspace'spendingCreateholds the Ctrl-'s create.tab_9392…, not the stand-in terminalpending_tab_53ec…(lines 128 and 130); the suite's other 3 tests passedpackage-test-lane.sh compile-tests)The same build of the failed journey (cd6a94aa) passed
r3-after-switch-traced15 of 15 times on cmux22 and cmux23 with the P0 runner's kit and flags. A 20-round soak of the old, unordered test passed 20 of 20. So a soak does not show this race; the ordered test above does.Observed / Not run
r3-after-switch-tracedrun (/work/wt/chrome-parity-design/p0-journey-runner/runs/cd6a94aa/)., 12 runs per phase): a same-host A/B on cmux24-perf in the order base, head, base, head (base 94b22d2354d: d0b1ae51, a6a93662; head 88341f28932: 184618ee and the fourth round). Every key-turn figure of the head is inside the spread of the two base rounds: idle key p95 Cmd-N 16.4 / 15.7 ms base, 16.0 / 15.7 head; Ctrl-13.2 / 12.8 base, 13.1 / 13.1 head; under load Cmd-N 20.4 / 19.2 base, 17.6 / 15.9 head; Ctrl-` 8.4 / 9.1 base, 6.5 / 9.4 head. Visible-frame p95 swings more between two rounds of the same arm (Cmd-N idle 96.2 / 69.6 ms base) than between the arms. Cmd-N's key turn is over the 10 ms budget on the base too; that is the CMD-N FIRST FRAME owner's work (its view reuse follows this PR). The first pair, on two hosts, compares nothing.Changelog
Fixed: Pressing New Terminal Tab right after New Workspace shows the new terminal in the new workspace, not an empty pane.
Proof
No capture: the change is which tab the daemon pane's controller selects at the hand-over. A still image cannot show the race; the ordered test and the two journeys check it.