Skip to content

fix(split): the second quick Cmd-D moves the focus and holds its keys (smoke step 23) - #31885

Merged
cmux-bot[bot] merged 4 commits into
feat-cmux-nextfrom
fix/split-twice-stale-handle
Nov 2, 2026
Merged

cmux-bot[bot] merged 4 commits into
feat-cmux-nextfrom
fix/split-twice-stale-handle

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Smoke step 23, "Cmd-D twice fast: two new panes, and the line typed into each at once ran there": the second line sometimes ran in the FIRST new terminal, and the second new terminal got nothing.

An optimistic split's echo gives the provisional pane record the daemon's handle in place (DaemonStore.confirmedPane). The window's handle map follows on its next topology apply. A Cmd-D in that step asked AppServices.paneController(for:) for the focused pane's controller. It looked the record up by its new handle, found no controller, and returned nil. PaneHandlers.split then had no workspace content: it began no focus intent and opened no creation key hold. The split still happened, but the keys typed after it went to whatever had the keyboard: the first new terminal.

The fix: with no handle match, paneController(for:) returns the controller that holds this very record (an identity scan over the shown panes, only on a miss). The handle match, and its rule that a handle counts only for the same model (handles repeat across machines), are unchanged. CmuxNextApp lines: +0.

This is not the Oct 31 failure of the same step: all 7 failures of Oct 31 predate the P0-1193 fix (#29301). Since then the smoke passed the step 394 times and failed once (2026-11-02, fix build of #31837 on cmux24).

Testing

Regression test (committed red before the fix): SplitOnEchoedPaneTests (CmuxNextAppTests, live same-tree cmux-tui). It makes a confirmed terminal pane with a first Cmd-D, gives the pane record a new handle as the echo does, then runs a second Cmd-D in that step and checks that the focus generation rose and that a creation hold opened.

Fleet command (red at dbc839e, green at 089087d):
cmux-ci run --class light --script scripts/ci/package-test-lane.sh --ref SHA --arg=suite --arg=Packages/macOS/CmuxNext --arg=SplitOnEchoedPaneTests,SecondCreationKeysTests,CmdNThenCreateKeysTests,CloseTabKeepsPaneTests --env CMUX_CI_XCODE_APP=/Applications/Xcode_26.6.app

  • Red (step 40d179e1, aws-m4pro-9): paneController(for:) nil; generation 1 -> 1 (no focus intent); no key hold. The 3 other suites passed.
  • Green (step 90539886, aws-m4pro-3): all 4 suites passed.
  • Display run at this head 434b123 (cmux-ci run --display --label gui-session-v1, CMUX_TEST_REQUIRE_GUI=1, worker cmux-austin-mini-0; same 4 suites, all passed):
    25a92aef7a97afa7597e9eb4
  • Light suite green at 434b123 (step 3593377fd13b469c351276af) and at 089087d (step 90539886). 434b123 takes the review note: the fallback is the keyed WorkspaceContentController.controller(for:), not a scan.
  • scripts/cmux-next/prepush-gate.py: no new violations; arch ratchet: no count rises.

Observed / Not run

Observed:

  • Fleet diag journey smoke-split-diag (10 loops per round: Cmd-N, Ctrl-`, then Cmd-D, a line, Cmd-D, a line, typed at once; hog +15 GB; debug.key; mini scheduler):
    • Tip build 84b90c53 (b732874): 3 of 4 rounds failed (cmux23, cmux24, cmux18), each with both lines on one terminal. In each failing loop the focus generation after the loop rose by one less than in a passing loop.
    • Fix build 58a04e68 (089087d): 4 of 4 rounds passed (cmux24 twice, cmux18, cmux19), 40 of 40 loops.

Not run: real HID key rounds (the smoke's real-key rounds stop earlier at the New Column item, STATUS 1712/1724); a full 10-round smoke gate.

Changelog

Fixed: Two quick Cmd-Ds with typing in between put each typed line in its own new pane, not both in the first one.

Proof

No capture: no visible UI change; the evidence is the regression test and the fleet journey above.

…ves the focus and holds the keys (red)

Smoke step 23 (Cmd-D, a line, Cmd-D, a line, typed at once): the second line ran in the first new
terminal. Fleet diag on the tip b732874 (10 loops per round, hog +15 GB): 3 of 4 rounds
failed; in each failing loop the focus generation rose by one less than in a passing loop, so the
second split began no focus intent and opened no key hold. Red expected on the base:
AppServices.paneController(for:) finds no controller for a record whose handle changed in place.
…d's handle is new, so it moves the focus and holds the keys (green)

An optimistic split's echo updates the provisional pane record in place, with the daemon's handle
(DaemonStore.confirmedPane); the window's handle map follows on its next topology apply. In that
step AppServices.paneController(for:) looked the record up by its new handle, found nothing, and
returned nil: PaneHandlers.split then had no content, began no focus intent and opened no
creation hold, and the line typed after the second Cmd-D ran in the first new terminal. With no
handle match, paneController(for:) now returns the controller that holds this very record (an
identity scan over the shown panes, only on a miss). CmuxNextApp lines: +0.
@lawrencecchen lawrencecchen added the risk:normal Risk tier (docs/ci/risk-tiers.md) label Nov 2, 2026
@github-actions

github-actions Bot commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

cmux status (one comment, edited in place; each line is updated by the job that owns it)

Review 434b123: HQ feed card · gallery before/after runs · checks

App of this head (app.zip, kept 1 day)

gh workflow run reload-build.yml -R manaflow-ai/cmux --ref feat-cmux-next -f tag=pr31885 -f ref=fix/split-twice-stale-handle -f platform=macos -f backend_mode=local

Shipped in:

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (smoke-split reviewer): SHIP at 089087d

No blocking findings.

Checked:

  1. Cause: PaneModel.update sets handle = s.id in place (PaneModel.swift:57), and confirmedPane reuses the provisional object (DaemonStore+Intents.swift:193). WorkspaceContentController.pane(for:) reads handles.paneIDs, which changes only in adoptHandles on the next apply. A plain Cmd-D goes daemonPane -> paneController(invocation)?.pane -> services.paneController(for:) (PaneHandlers.swift:77). When that is nil, content is nil, so there is no beginFocusIntent, no creationInputCoordinator.begin, and also no pendingCreate chain and no applyCurrent in the key's turn. The cause is plausible, and the fix restores all four.
  2. Wrong controller: the fallback scans the same content.panes set as the handle path and requires ===. So it cannot pick another machine's pane or a mirror. panes is keyed by LayoutPaneID(pane.id), so one content holds at most one controller per model. The window order is the same as before. I found no caller that a hit during the echo step makes worse: the hit returns the controller that the next apply returns anyway.
  3. Cost: there is no perf-hot marker on this path. A miss is normal for every pane of a hidden workspace. ControlSnapshotPublisher.topology calls it per pane (selectedTab), and the uncached Search Tabs path and shownTab(of:) loops call it per pane too. Each miss now walks every shown pane, which gives O(hidden x shown). This is small in absolute terms, but REVIEW.md's "no quadratic lookups" applies.
  4. Test: it reaches the same app-side state (record handle != map handle), and it fails red for the right reason: lookup nil, generation unchanged, no hold. defer restores the handle. The second split then sends a fake handle to the daemon, which refuses it and rolls back inside the per-test app. That is harmless.
  5. Rules: CmuxNextApp net +0 lines, nothing blocking, red-then-green commits, fleet command named.

Non-blocking:

  • AppServices.swift:863: use the existing O(1) helper WorkspaceContentController.controller(for:) (InternalPages/WorkspaceContentController+PageTabPane.swift:32: panes[LayoutPaneID(pane.id)] with an identity check). For example, return windows.controllers.lazy.compactMap { $0.content?.controller(for: pane) }.first. This makes a miss cost O(windows), not O(shown panes), and it can replace the handle path entirely.
  • BrowserPageRequests.swift:329 (openPopup) uses the same handle-only lookup, with no identity check and the same stale step. Route it through paneController(for:).
  • The test does not reach the real echo through the daemon. A daemon-driven variant would be stronger, but the fleet journey covers that.

…, not by a scan (review note)

Review of #31885: the fallback walked every shown pane on each miss, and a miss is normal for a
hidden workspace's pane (topology publisher, Search Tabs), so it was O(hidden x shown).
WorkspaceContentController.controller(for:) is keyed by the pane's public id (which the echo does
not change) with the same identity check: O(windows) per miss. CmuxNextApp lines: +0.
@cursor

cursor Bot commented Nov 2, 2026

Copy link
Copy Markdown

Bugbot needs on-demand usage enabled

Bugbot uses usage-based billing for this team and requires on-demand usage to be enabled.

A team admin can enable on-demand usage in the Cursor dashboard.

@blacksmith-sh

blacksmith-sh Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

Found 6 test failures on Blacksmith runners:

Failures

Test View Logs
main.DeveloperIdAppTests.test_download_prints_the_active_profile_only/
test_download_prints_the_active_profile_only
View Logs
stubs: /System/Library/Frameworks/Security.framework/Versions/A/Security has no stub/
Security has no stub
View Logs
stubs: CoreFoundation.tbd: install-name /System/Library/Frameworks/
CoreFoundation.framework/Versions/A/CoreFoundation, missing 1: ['_CFRetain']/
CoreFoundation, missing 1: ['_CFRetain']
View Logs
stubs: CoreFoundation.tbd: install-name /System/Library/Frameworks/
CoreFoundation.framework/Versions/A/CoreFoundation, missing 1: ['_log2']/
CoreFoundation, missing 1: ['_log2']
View Logs
test_every_compile_job_names_its_warm_sources (main.ThisTree.test_every_compile_job
_names_its_warm_sources)/
test_every_compile_job_names_its_warm_sources (main.ThisTree.test_every_compile_job
_names_its_warm_sources)
View Logs
test_the_repository_highlights_validate (main.DigestTests.test_the_repository_highl
ights_validate)/
test_the_repository_highlights_validate (main.DigestTests.test_the_repository_highl
ights_validate)
View Logs

Fix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (smoke-followup reviewer): SHIP at 434b123

No blocking findings.

Checked:

  1. Cause: PaneModel.update changes handle in place (PaneModel.swift:57) on the provisional model that confirmedPane reuses (DaemonStore+Intents.swift:193). content.pane(for:) reads handles.paneIDs, and that map changes only in adoptHandles on the next apply. So the old lookup missed, and PaneHandlers.split (PaneHandlers.swift:77-78) got no content: no beginFocusIntent, no creation hold.
  2. The key is stable. PaneModel.id is a let. For a client-keyed split, the provisional model is created with provisional.paneID (pane_…, ProvisionalSplit.swift:267-272), and confirmedPane finds it again by identity(snapshot) (the same pane_… resource id). LayoutMapping.swift:23 keys content.panes with the same LayoutPaneID(pane.id), so the key stays the same through the echo while the handle changes. For an id without a resource (pane:<handle>), the id and the layout key are both fixed when the model is made, so they agree too.
  3. Collisions: a WorkspaceContentController shows the workspace of one daemon, so ids in one panes map cannot come from two machines. Across windows, controller(for:) checks $0.pane === pane, so a pane:<n> id that repeats on another machine is rejected. The handle path keeps its identity rule.
  4. Cost: one dictionary lookup per window, only on a miss. That is fine on the Cmd-D path. A hidden workspace's pane still gets nil.
  5. Test: it reaches the step (a confirmed pane, then the record's handle changed in place) and asserts the behavior: the focus generation rises and a hold opens. The red at dbc839e failed on exactly those asserts.

Non-blocking: the second split sends handle + 10_000, a pane the daemon does not know, so the daemon refuses that split after the asserts. The test proves the focus and the hold, not that a second pane lands. Acceptable here; a comment in the test would help.

@lawrencecchen lawrencecchen added the ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds label Nov 2, 2026
@cmux-bot
cmux-bot Bot temporarily deployed to ci-routing-agent November 2, 2026 07:04 Inactive
@cmux-bot
cmux-bot Bot temporarily deployed to ci-routing-agent November 2, 2026 07:04 Inactive
@cmux-bot
cmux-bot Bot temporarily deployed to ci-routing-agent November 2, 2026 07:06 Inactive
@cmux-bot
cmux-bot Bot merged commit e573965 into feat-cmux-next Nov 2, 2026
150 of 152 checks passed
@cmux-bot
cmux-bot Bot deleted the fix/split-twice-stale-handle branch November 2, 2026 07:26
@lawrencecchen lawrencecchen added ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds and removed ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds labels Nov 2, 2026

This branch was previously deployed

1 inactive deployment
ci-routing-agent — bc9be4ee Deployed Nov 2, 2026 by cmux-bot[bot] via wait for same-key tree publisher #99931
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk:normal Risk tier (docs/ci/risk-tiers.md)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant