Repository navigation
Guard performKeyEquivalent keyDown force-dispatches against replay loops - #5891
Conversation
NSWindow.cmux_performKeyEquivalent force-dispatches certain key events straight into the focused responder's keyDown. When the responder does not consume the key, AppKit routes the same event back into performKeyEquivalent while the first dispatch is still on the stack (WebKit replays unhandled keys, and on macOS 26 -[NSWindow keyDown:] re-enters performKeyEquivalent). The printable-Option-text bypass has no re-entry guard, so the event ping-pongs between the swizzle and the responder until the main-thread stack overflows. The test drives the real chokepoint: a window whose first responder re-invokes performKeyEquivalent with the same event from keyDown, bounded so the pre-fix failure is a clean assertion instead of a crash. It asserts the force-dispatch happens exactly once per event, that distinct events (autorepeat) still each dispatch, and that the same event may dispatch again once the prior dispatch has unwound (WebKit's legitimate replay). Repro of #5887: Option+A with a browser pane focused on non-editable content crashes with "Thread stack size exceeded due to excessive recursion" (incident C9470E41-11A7-4A04-874F-C8AE5DF1CA06 on a debug build, mirroring incident 96E09E5C-19CC-49D4-B068-7A666CE784A9 from cmux NIGHTLY 0.64.14). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…y loops Replace the seven per-branch forwarding-depth counters in NSWindow.cmux_performKeyEquivalent with one shared chokepoint, cmuxForceDispatchKeyDownOnce. The helper tracks the identity (window number, event type, keyCode, modifiers, timestamp) of every key event whose force-dispatch is currently on the stack and refuses to dispatch the same event a second time, returning false so the caller falls through to default AppKit handling. This closes the unguarded printable-Option-text bypass that crashed cmux NIGHTLY 0.64.14 (#5887): WebKit replays an unhandled key through the responder chain, macOS 26 -[NSWindow keyDown:] re-enters performKeyEquivalent, and the bypass force-dispatched the same event back into CmuxWebView.keyDown forever until the main-thread stack overflowed. It also guards the previously unguarded ghostty zoom, stale-menu-bypass, and menu-miss keyDown dispatches, and protects against cross-branch ping-pong that per-branch counters cannot see (the first responder can change while a dispatch is in flight). The guard is stack-scoped (insert before keyDown, remove via defer), so WebKit's legitimate single replay of an unhandled key, which arrives after the original dispatch has unwound, still force-dispatches normally. Key autorepeat produces distinct events with fresh timestamps, so repeat typing is never throttled, and the dispatching window's number is part of the identity so multiple windows cannot suppress each other. Fixes #5887 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a unified in‑flight forced‑dispatch identity guard in AppDelegate to prevent re‑entrant keyDown replay loops, updates multiple forwarding sites to use the guard, and adds regression tests plus Xcode project wiring verifying exactly‑once forced dispatch per in‑flight event, independent dispatch for distinct events, and re‑dispatch after unwind. ChangesKey-event replay guard and tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 20 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (20 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryFixes a main-thread stack overflow (issue #5887) caused by an infinite loop between
Confidence Score: 5/5Safe to merge. The guard is principled and covers all 13 force-dispatch sites; the stack-overflow crash path is closed and no regression to WebKit's legitimate post-unwind replay. The unified chokepoint correctly replaces seven per-branch depth counters that could not see cross-branch ping-pong. Field-tuple identity (window, type, keyCode, modifiers, timestamp) is stable across event copies and correctly distinguishes autorepeat. The defer-based remove preserves WebKit's legitimate replay once the original dispatch unwinds. Fallthrough behavior when the guard declines — either return false or delegating to cmux_performKeyEquivalent — matches the prior per-branch semantics in each affected path. Three regression tests pin the invariant deterministically. No files require special attention. Important Files Changed
Reviews (5): Last reviewed commit: "Merge branch 'main' of https://github.co..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| import AppKit | ||
| import XCTest | ||
|
|
||
| #if canImport(cmux_DEV) | ||
| @testable import cmux_DEV |
There was a problem hiding this comment.
The cmuxForceDispatchKeyDownOnce guard described in the PR is not present in any committed file. AppDelegate.swift line 17252 still calls textInputTarget.keyDown(with: event) with no replay guard in the shortcutRoutingShouldBypassForPrintableOptionText branch, and the seven per-branch depth counters remain unchanged. As submitted, testPrintableOptionTextKeyDownIsForceDispatchedExactlyOncePerEvent will trip on the real crash path (bounded to 6 dispatches by replaysRemaining = 5), failing CI with keyDownEvents.count == 6 != 1. The fix described in the PR description — a unified cmuxForceDispatchKeyDownOnce wrapping all keyDown dispatch sites — needs to land in the same merge commit (or as the follow-on squash before merge) for this PR to accomplish what it claims.
There was a problem hiding this comment.
Correct at review time, and thank you for catching it: the branch head you reviewed (b655fed) was the intentionally failing first commit of the two-commit regression structure, and the fix commit had not been pushed yet due to an orchestration slip. The fix is now on the branch as c4f61dd (cmuxForceDispatchKeyDownOnce, routing every force-dispatch site).
Verified on a dev build of the fix: with web content focused on https://example.com, Option+A x3, Return, and Down all leave the app alive. The identical Option+A setup crashed an unguarded build (current main) in under a second this morning with the issue's exact signature (incident C9470E41-11A7-4A04-874F-C8AE5DF1CA06).
One correction to the prediction that the test would trip CI: the tests job at b655fed stayed green because the app-host suite crashed and hit its 900s timeout before reaching the W* classes, and the job still passed. That CI gap is now tracked in #5903. The deterministic red/green proof ran on a macOS 26 builder instead: WindowKeyDownReplayGuardTests fails at b655fed and passes at c4f61dd.
There was a problem hiding this comment.
Thanks for the full context — the two-commit structure makes sense now, and the orchestration slip explains the timing. My original comment was accurate for the branch head I reviewed; it's moot now.
The fix at c4f61dd looks correct. A few observations from reading it:
Identity struct is sound. Using timestamp: TimeInterval (a Double) for equality is safe here because the comparison is always of the same-object timestamp against itself, not a computed value — same bits, exact equality holds.
@autoclosure on reason: is a nice touch; the string interpolation never runs in release builds.
Previously-unguarded paths are all covered. The stale-menu bypass and browser-surface-shortcut-to-terminal paths were genuinely unguarded in the old code and are now routed through cmuxForceDispatchKeyDownOnce. That's the most important correctness gain beyond the nominal crash path.
One subtle difference to be aware of in the browser-arrow omnibar-restore sub-path: In the old code the depth check short-circuited before the makeFirstResponder(focusedOmnibarField) call. In the new code, if cmuxForceDispatchKeyDownOnce returns false on re-entry, makeFirstResponder may have already been called for that call frame. In practice this is benign — on re-entry the field editor is already first responder so the currentEditorResponder == nil || firstResponder !== currentEditorResponder condition evaluates false and makeFirstResponder is skipped — but it's a semantics difference worth knowing.
The CI gap (test-suite crash swallowing the test failure) being tracked in #5903 is the right call; the macOS 26 builder red/green proof is sufficient evidence for this PR.
LGTM from my side. The invariant is right, the chokepoint is clean, and the three regression tests cover the cases that matter (guard fires, guard is per-event not global, guard is stack-scoped).
| ) -> NSEvent? { | ||
| NSEvent.keyEvent( | ||
| with: .keyDown, | ||
| location: .zero, | ||
| modifierFlags: [.option], | ||
| timestamp: timestamp, | ||
| windowNumber: windowNumber, | ||
| context: nil, | ||
| characters: "å", | ||
| charactersIgnoringModifiers: "a", | ||
| isARepeat: false, | ||
| keyCode: 0 | ||
| ) | ||
| } | ||
|
|
||
| func testPrintableOptionTextKeyDownIsForceDispatchedExactlyOncePerEvent() { | ||
| _ = NSApplication.shared | ||
| AppDelegate.installWindowResponderSwizzlesForTesting() | ||
|
|
||
| let (window, responder) = makeWindowWithReplayingResponder() | ||
| guard let event = makeOptionTextKeyDownEvent(windowNumber: window.windowNumber) else { | ||
| XCTFail("Failed to construct Option+A key event") | ||
| return | ||
| } | ||
|
|
||
| XCTAssertTrue(window.performKeyEquivalent(with: event)) | ||
| XCTAssertEqual( | ||
| responder.keyDownEvents.count, | ||
| 1, | ||
| "The same in-flight key event must not be force-dispatched into keyDown again " + |
There was a problem hiding this comment.
keyDown dispatch sites in cmux_performKeyEquivalent are not covered
The test exercises the printable-option-text path but two additional keyDown dispatch sites in cmux_performKeyEquivalent also have no replay guard in the current code: the Ghostty font-zoom path (ghosttyView.keyDown(with: event) at line 17317) and the shouldForwardBrowserSurfaceShortcutToTerminal menu-miss path (firstResponderGhosttyView.keyDown(with: event) at line 17597). If commit 2 routes those calls through the same cmuxForceDispatchKeyDownOnce chokepoint, adding test variants for a GhosttyNSView-as-first-responder scenario (zoom shortcut) would confirm the guard is wired there too and prevent silent regression if a future branch adds another unguarded dispatch.
There was a problem hiding this comment.
Addressed in c4f61dd (now pushed): the Ghostty font-zoom and menu-miss dispatches, plus the stale-menu bypass, command palette arrows, text-box input, editable-text-view, omnibar, and browser Return paths, all route through the single cmuxForceDispatchKeyDownOnce chokepoint. There are 13 call sites and zero direct keyDown(with:) dispatches left in cmux_performKeyEquivalent; the only one remaining in that region is inside the helper itself.
On the suggested GhosttyNSView-as-first-responder test variant: a real GhosttyNSView needs the embedded terminal runtime, which the headless unit host cannot bring up, and a stub standing in for it would re-test the same helper the existing three tests already cover (dispatch-once, distinct events each dispatch, same event again after unwind). Since every branch now shares that helper, per-branch variants would duplicate the chokepoint coverage rather than add protection, so we skipped them per the repo's low-value-test guidance.
…file length budget No behavior change. The guard helper, identity struct, and in-flight set move from Sources/AppDelegate.swift (which the fix had pushed 32 lines over its 18057-line budget) into Sources/App/WindowKeyDownReplayGuard.swift, leaving AppDelegate.swift 35 lines under budget. The new file stays below the 500-line tracking threshold. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-5887-keydown-replay-loop # Conflicts: # Sources/AppDelegate.swift
…ops (manaflow-ai#5891) * Add failing regression test for the keyDown replay loop NSWindow.cmux_performKeyEquivalent force-dispatches certain key events straight into the focused responder's keyDown. When the responder does not consume the key, AppKit routes the same event back into performKeyEquivalent while the first dispatch is still on the stack (WebKit replays unhandled keys, and on macOS 26 -[NSWindow keyDown:] re-enters performKeyEquivalent). The printable-Option-text bypass has no re-entry guard, so the event ping-pongs between the swizzle and the responder until the main-thread stack overflows. The test drives the real chokepoint: a window whose first responder re-invokes performKeyEquivalent with the same event from keyDown, bounded so the pre-fix failure is a clean assertion instead of a crash. It asserts the force-dispatch happens exactly once per event, that distinct events (autorepeat) still each dispatch, and that the same event may dispatch again once the prior dispatch has unwound (WebKit's legitimate replay). Repro of manaflow-ai#5887: Option+A with a browser pane focused on non-editable content crashes with "Thread stack size exceeded due to excessive recursion" (incident C9470E41-11A7-4A04-874F-C8AE5DF1CA06 on a debug build, mirroring incident 96E09E5C-19CC-49D4-B068-7A666CE784A9 from cmux NIGHTLY 0.64.14). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Guard every performKeyEquivalent keyDown force-dispatch against replay loops Replace the seven per-branch forwarding-depth counters in NSWindow.cmux_performKeyEquivalent with one shared chokepoint, cmuxForceDispatchKeyDownOnce. The helper tracks the identity (window number, event type, keyCode, modifiers, timestamp) of every key event whose force-dispatch is currently on the stack and refuses to dispatch the same event a second time, returning false so the caller falls through to default AppKit handling. This closes the unguarded printable-Option-text bypass that crashed cmux NIGHTLY 0.64.14 (manaflow-ai#5887): WebKit replays an unhandled key through the responder chain, macOS 26 -[NSWindow keyDown:] re-enters performKeyEquivalent, and the bypass force-dispatched the same event back into CmuxWebView.keyDown forever until the main-thread stack overflowed. It also guards the previously unguarded ghostty zoom, stale-menu-bypass, and menu-miss keyDown dispatches, and protects against cross-branch ping-pong that per-branch counters cannot see (the first responder can change while a dispatch is in flight). The guard is stack-scoped (insert before keyDown, remove via defer), so WebKit's legitimate single replay of an unhandled key, which arrives after the original dispatch has unwound, still force-dispatches normally. Key autorepeat produces distinct events with fresh timestamps, so repeat typing is never throttled, and the dispatching window's number is part of the identity so multiple windows cannot suppress each other. Fixes manaflow-ai#5887 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Move the keyDown replay guard into its own file to satisfy the Swift file length budget No behavior change. The guard helper, identity struct, and in-flight set move from Sources/AppDelegate.swift (which the fix had pushed 32 lines over its 18057-line budget) into Sources/App/WindowKeyDownReplayGuard.swift, leaving AppDelegate.swift 35 lines under budget. The new file stays below the 500-line tracking threshold. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This branch was successfully deployed
1 participant
Crash
cmux NIGHTLY 0.64.14 on macOS 26.4.1 crashed with "Thread stack size exceeded due to excessive recursion": 68,812 main-thread frames (incident 96E09E5C-19CC-49D4-B068-7A666CE784A9). A key pressed while a browser pane was focused on non-editable content looped forever between
NSWindow.cmux_performKeyEquivalentandCmuxWebView.keyDown: WebKit replays the unhandled key through the responder chain, macOS 26-[NSWindow keyDown:]re-entersperformKeyEquivalent, and the swizzle force-dispatched the same event back into the web view with no replay guard.Fixes #5887
Repro evidence
Reproduced on a debug build (r5886, current main) via the debug socket: focus a browser pane on https://example.com (web content first responder, non-editable), send Option+A. The printable-Option-text bypass force-dispatches with no guard; the app logged 1,150
performKeyEquiv: Opt+'a'(0) fr=CmuxWebViewlines in 30 ms and died with the same "Thread stack size exceeded" signature (incident C9470E41-11A7-4A04-874F-C8AE5DF1CA06). Plain Return survives on main only because that one branch had a hand-rolled depth counter; the Option-text bypass, ghostty zoom, stale-menu bypass, and menu-miss dispatches had none.Fix
One shared chokepoint,
cmuxForceDispatchKeyDownOnce, now performs every directkeyDown(with:)force-dispatch incmux_performKeyEquivalent. It tracks the identity of each event whose dispatch is on the stack (window number, type, keyCode, modifiers, timestamp; inserted beforekeyDown, removed viadefer) and declines to dispatch the same event twice, so the caller falls through to default AppKit handling. The seven per-branch forwarding-depth counters are replaced by this one mechanism, which also covers cross-branch ping-pong they could not see (the first responder can change while a dispatch is in flight).Composition with the parallel fix on main
While this PR was in review, #5899 landed on main and fixes the webview path of the same crash:
CmuxWebViewwraps WebKit keyDown dispatch in an is-active flag, and three browser branches ofcmux_performKeyEquivalentreturn early when a webview is first responder during that dispatch. This branch merges main and keeps those guards intact.What this PR still adds: those early-returns only fire for webview first responders, so the same-event replay loop through any other responder, and the ghostty font-zoom, stale-menu, menu-miss, command palette, text-box, omnibar, and editable-text-view dispatch sites, remained unguarded. All 13 force-dispatch sites now route through
cmuxForceDispatchKeyDownOnce. Proof of the residual hole:WindowKeyDownReplayGuardTestsfails on current main even with #5899 (run at 37df40d, which is main plus only the test), and passes on this branch's merge head dbae247. Both runs on a macOS 26 builder.Principled rather than a branch patch: the invariant "never force-dispatch the same in-flight event twice" is enforced at the single point where force-dispatches happen, for present and future branches. Stack-scoping preserves WebKit's legitimate single replay of unhandled keys (it arrives after the original dispatch unwinds), key autorepeat produces distinct timestamps so repeat typing is never throttled, and the window number keeps multiple windows independent.
Test
WindowKeyDownReplayGuardTestsdrives the real chokepoint: a window whose first responder re-invokesperformKeyEquivalentwith the same event fromkeyDown(bounded, so the pre-fix failure is a clean assertion rather than a stack overflow). Commit 1 adds the failing test only, commit 2 adds the fix. Note on CI: the GitHub tests job stayed green at the test-only commit because the app-host unit suite crashed and hit its 900s timeout before reaching this class and the job still passed (tracked in #5903). The deterministic red/green proof ran on a macOS 26 builder: WindowKeyDownReplayGuardTests fails at b655fed and passes at c4f61dd. Two companion tests pin the non-regression behavior: distinct events each dispatch (autorepeat), and the same event dispatches again once the prior dispatch has unwound (the legitimate WebKit replay).🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Stops a stack-overflow crash on macOS 26 by guarding
performKeyEquivalentkeyDown force-dispatches so the same event isn’t re-dispatched while in flight. Fixes #5887 and adds regression tests.Bug Fixes
keyDown(with:)force-dispatches throughNSWindow.cmuxForceDispatchKeyDownOnce.WindowKeyDownReplayGuardTestsfor once-per-event dispatch, distinct events (autorepeat), and replay-after-unwind.Refactors
Sources/App/WindowKeyDownReplayGuard.swiftand wire into the project; no behavior change.Written for commit dbae247. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests