Repository navigation
fix: prevent recursive os_unfair_lock crash on cmd-clicked .md viewer route + drop fragment/query gate - #3559
Conversation
Fixes manaflow-ai#3370 (and reduces manaflow-ai#1283's gating): - Defer Workspace.openOrFocusMarkdownSplit to the next runloop tick. Ghostty's Surface.openUrl holds an internal lock when it dispatches into the Swift open-URL handler; opening a new panel synchronously triggers Workspace.applyTabSelectionNow -> Surface.encodeKey, which tries to acquire the same lock and aborts with recursive_os_unfair_lock (BUG IN CLIENT OF LIBPLATFORM, Abort Cause 259). Running the split creation on DispatchQueue.main.async lets Surface.openUrl return and release the lock first. - Drop the fragment == nil / query == nil gate. Tools like Claude Code emit markdown links with line-anchor fragments (foo.md#L42); under the previous gate those URLs were routed to NSWorkspace and opened in the system editor instead of the cmux markdown viewer. The viewer only consumes URL.path (which is already fragment/query-free), so stripping is automatic and the panel-state contract is preserved. - Add an async fallback so a click is never silently lost: if the deferred split creation fails (e.g. the source pane was closed in between), the URL still surfaces through NSWorkspace. Crash backtrace from a stable 0.64.1 reproduction: 0 _os_unfair_lock_recursive_abort 1 _os_unfair_lock_lock_slow 2 Surface.encodeKey 3 Workspace.applyTabSelectionNow 4 Workspace.focusPanel 5 Workspace.newMarkdownSplit 6 Workspace.openOrFocusMarkdownSplit 7 GhosttyApp.handleAction (closure manaflow-ai#33) ... performOnMain (sync, MainActor.assumeIsolated) 13 GhosttyApp.handleAction 14 Surface.openUrl <-- holds the lock that frame 2 re-enters 15 GhosttyNSView.mouseUp
|
@psh4607 is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
To use Codex here, create an environment for this repo. |
|
@codex review |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCmd-click handling for local ChangesMarkdown URL Routing and Execution
Sequence Diagram(s)sequenceDiagram
participant TerminalView
participant Resolver as WorkspaceResolver
participant Workspace
participant System as NSWorkspace
TerminalView->>Resolver: cmd-click -> parse URL & shouldRoute?
alt shouldRoute == true
TerminalView-->>TerminalView: return true (immediate)
TerminalView->>System: schedule DispatchQueue.main.async
Note right of TerminalView: async block runs later
TerminalView->>Resolver: re-resolve workspace & re-check shouldRoute
alt re-check == true
Resolver->>Workspace: openOrFocusMarkdownSplit(fileURL)
alt split created/focused
Workspace->>Workspace: focus/open split
else split creation failed
Workspace->>System: NSWorkspace.shared.open(fileURL)
end
else re-check == false
TerminalView->>System: NSWorkspace.shared.open(fileURL)
end
else shouldRoute == false
TerminalView->>System: NSWorkspace.shared.open(fileURL) (immediate)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 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 |
@psh4607 I have started the AI code review. It will take a few minutes to complete. |
|
Tip For best results, initiate chat on the files or code changes.
[review] |
Greptile SummaryThis PR fixes a recursive
Confidence Score: 5/5Safe to merge — well-scoped single-file deferral with documented fallback for every failure mode. The diff is small (one file, +28/-11) and all substantive concerns raised in prior review rounds were resolved in follow-up commits. The async closure is written defensively with workspace re-resolution, remote-surface re-check, TOCTOU readability guard, and system-opener fallback on every failure path. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["GHOSTTY_ACTION_OPEN_URL callback\nSurface lock held"] --> B{isEnabled?}
B -- No --> Z["Fall through to BrowserLinkOpen / NSWorkspace"]
B -- Yes --> C{isFileURL + local host + .md path?}
C -- No --> Z
C -- Yes --> D["performOnMain sync\nResolve workspace\nRemote-surface guard\nshouldRoute check"]
D -- Guard fails --> E["return false"]
E --> Z
D -- Guards pass --> F["Capture workspaceId + surfaceId\nDispatchQueue.main.async\nreturn true"]
F --> G["Surface lock released same runloop cycle"]
G --> H["async block: re-resolve workspace\nremote-surface re-check"]
H -- Remote surface --> I["NSWorkspace.shared.open fileURL"]
H -- Local --> J["TOCTOU shouldRoute re-check"]
J -- File gone --> I
J -- File OK --> K["openOrFocusMarkdownSplit"]
K -- non-nil success --> L["Panel opens - done"]
K -- nil pane gone --> I
Reviews (4): Last reviewed commit: "review: re-apply remote-surface guard in..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR updates terminal URL handling in GhosttyTerminalView so cmd-clicked local markdown links open in cmux’s markdown viewer without triggering the recursive os_unfair_lock crash described in #3370, while also covering file:// markdown URLs that include fragments or queries as requested in #1283.
Changes:
- Expands markdown viewer routing to local
file://markdown URLs even when they include#fragmentor?query. - Defers markdown split creation to the next main-queue turn so the Ghostty
open_urlcallback can unwind before focus-changing UI work runs. - Adds an async
NSWorkspace.shared.openfallback if the deferred markdown split cannot be created.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…low-ai#3560 Addresses Greptile P2 review feedback on manaflow-ai#3559: the DispatchQueue.main.async deferral is a timing-based mitigation that relies on the Ghostty C side releasing its surface lock within the same runloop cycle that dispatches this Swift callback — an undocumented invariant. Strengthens the inline comment to spell out: - the dependency is on Ghostty implementation detail, not API contract - future Ghostty changes that move the unlock can silently regress every Swift caller in this performOnMain window, not just this one - the structural fix lives on the C side (async dispatch of the OPEN_URL callback) and is tracked in manaflow-ai#3560 - do NOT remove this DispatchQueue.main.async until manaflow-ai#3560 lands No code-behavior change; comment-only update so future readers cannot 'clean up' the deferral without reading why it exists.
Addresses Copilot review feedback on manaflow-ai#3559: - Re-resolve workspace at dispatch time via AppDelegate.shared?.workspaceContainingPanel(panelId:preferredWorkspaceId:), mirroring the deferred browser path. The user can move a tab to a different workspace between the synchronous gate and the runloop tick; using the captured workspace would route into the wrong one and incorrectly fall through to NSWorkspace even though the source pane still lives. - Re-validate CmdClickMarkdownRouteSettings.shouldRoute inside the async closure. The file may have been deleted/renamed in the gap; if so, fall through to NSWorkspace instead of materializing a MarkdownPanel onto an unavailable file (the panel would render its 'file unavailable' state, which is worse UX than letting the system opener report the missing path).
…coalesce AppDelegate.shared?.workspaceContainingPanel(panelId:preferredWorkspaceId:) returns (workspace: Workspace, tabManager: TabManager)?, not Workspace?, so the previous '?? workspace' fallback would not compile (Swift rejects the operands as incompatible types). Extract .workspace from the tuple before coalescing, matching the existing usage pattern at Sources/AppDelegate.swift:13250. Caught by Greptile P1 review on manaflow-ai#3559. Compile-only check is still blocked locally on the GhosttyKit.xcframework / zig dependency, so this slipped through SourceKit-LSP's partial type resolution; the suggestion patch from Greptile is a direct match for the in-tree caller convention.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 3824-3841: After re-resolving the workspace into
resolvedWorkspace, re-apply the same remote-vs-local guard used earlier so we
don't open an in-app markdown split for a remote surface: call the same
remote-surface check (the one used in the synchronous gate) against the
panel/surface or resolvedWorkspace and if it indicates the surface is remote,
fall back to NSWorkspace.shared.open(fileURL) and return; otherwise continue to
use CmdClickMarkdownRouteSettings.shouldRoute(path:) and
resolvedWorkspace.openOrFocusMarkdownSplit(from:fileId:filePath:) as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a7a1c0bf-fbca-46a0-9f44-b4c036fe0d27
📒 Files selected for processing (1)
Sources/GhosttyTerminalView.swift
The synchronous gate at line 3796-3799 forbids routing markdown opens for remote terminal surfaces. Inside the deferred async closure that re-resolves the workspace (because the panel can migrate before the next runloop tick), the same remote/local guard must be re-applied — otherwise a panel that moved into a remote workspace during the gap would still receive an in-app split, bypassing the sync rule. Mirror the sync gate via resolvedWorkspace.isRemoteTerminalSurface(...) and fall back to NSWorkspace.shared.open on remote. Caught by CoderabbitAI (🟠 Major) on manaflow-ai#3559.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/GhosttyTerminalView.swift (1)
3828-3845:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-apply the remote-workspace guard after the async re-resolve.
Line 3828 re-resolves the workspace because the panel can move before the async block runs, but this path no longer re-checks whether the surface is remote before Line 3838 opens the in-app markdown split. If the surface moves into a remote workspace during that gap, this bypasses the earlier remote/local routing guard. Fall back to
NSWorkspace.shared.open(fileURL)here beforeshouldRoute(path:)/openOrFocusMarkdownSplit(...).Suggested fix
let resolvedWorkspace = AppDelegate.shared?.workspaceContainingPanel( panelId: surfaceId, preferredWorkspaceId: preferredWorkspaceId )?.workspace ?? workspace + guard !resolvedWorkspace.isRemoteTerminalSurface(surfaceId) else { + NSWorkspace.shared.open(fileURL) + return + } // TOCTOU re-check: file may have been removed/renamed // since the synchronous gate. Fall through if so. guard CmdClickMarkdownRouteSettings.shouldRoute(path: fileURL.path) else { NSWorkspace.shared.open(fileURL) return🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/GhosttyTerminalView.swift` around lines 3828 - 3845, After re-resolving the workspace into resolvedWorkspace, re-apply the remote-workspace guard before calling CmdClickMarkdownRouteSettings.shouldRoute(...) / resolvedWorkspace.openOrFocusMarkdownSplit(...): if the resolvedWorkspace has become remote (e.g., check resolvedWorkspace.isRemote or equivalent API on the workspace object), call NSWorkspace.shared.open(fileURL) and return so we don’t bypass remote routing; then proceed to shouldRoute(...) and openOrFocusMarkdownSplit(...) as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 3828-3845: After re-resolving the workspace into
resolvedWorkspace, re-apply the remote-workspace guard before calling
CmdClickMarkdownRouteSettings.shouldRoute(...) /
resolvedWorkspace.openOrFocusMarkdownSplit(...): if the resolvedWorkspace has
become remote (e.g., check resolvedWorkspace.isRemote or equivalent API on the
workspace object), call NSWorkspace.shared.open(fileURL) and return so we don’t
bypass remote routing; then proceed to shouldRoute(...) and
openOrFocusMarkdownSplit(...) as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ec7f356f-92f0-4f83-adc1-b550a38975ab
📒 Files selected for processing (1)
Sources/GhosttyTerminalView.swift
|
Hey @lawrencecchen @austinywang — when one of you has a moment, could you take a look at #3558 and #3559? Both are external-fork PRs, so the GitHub Actions workflows (2 on #3558, 8 on #3559) and the Vercel deploys are stuck on "Approve and run". A one-click approval would unblock CI. Review status:
Happy to revise anything once CI runs. Thanks! |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Closes #3370.
Extends partial coverage of #1283 (fragmented
.mdURLs now route through the viewer).Summary
Sources/GhosttyTerminalView.swift'sGHOSTTY_ACTION_OPEN_URLhandler, the cmd-click.mdviewer route (a) defersWorkspace.openOrFocusMarkdownSplittoDispatchQueue.main.asyncinstead of running synchronously insideperformOnMain, and (b) no longer gates routing onfragment == nil/query == nil. An asyncNSWorkspace.shared.openfallback covers the rare case where deferred split creation fails.openMarkdownInCmuxViewer: true, cmd-clicking a real.mdlink inside a terminal pane crashes cmux on stable 0.64.1 (reproduced) and on nightly per the original report.Surface.openUrl(Ghostty side) is holding an internal lock when it dispatches into the Swift handler; the synchronousperformOnMainchain runsopenOrFocusMarkdownSplit -> newMarkdownSplit -> focusPanel -> applyTabSelectionNow -> Surface.encodeKey, andSurface.encodeKeytries to re-acquire that same lock — `BUG IN CLIENT OF LIBPLATFORM: Trying to recursively lock an os_unfair_lock`, Abort Cause 259. Running the split creation on the next runloop tick lets `Surface.openUrl` return and release its lock first.Net diff: 1 file, +28/-11 lines.
Crash backtrace (cmux 0.64.1 stable, build 81, macOS 26.4.1, Apple Silicon)
```
0 _os_unfair_lock_recursive_abort
1 _os_unfair_lock_lock_slow
2 Surface.encodeKey
3 Workspace.applyTabSelectionNow
4 Workspace.focusPanel
5 Workspace.newMarkdownSplit
6 Workspace.openOrFocusMarkdownSplit
7 closure #33 in GhosttyApp.handleAction
... performOnMain (synchronous, MainActor.assumeIsolated)
13 GhosttyApp.handleAction
14 @objc closure #2 in GhosttyApp.initializeGhostty
15 Surface.openUrl ← holds the lock that frame 2 tries to re-acquire
16 @objc GhosttyNSView.mouseUp
... AppKit event delivery
```
`Surface.openUrl` (Ghostty submodule) acquires the surface lock for the duration of the URL action callback. The Swift handler runs synchronously inside that window via `performOnMain` (`@MainActor () -> T`), so any panel/focus mutation that ends up in `Surface.encodeKey` deadlocks on the same `os_unfair_lock`. Deferring the panel mutation by one runloop tick is the smallest correct fix; it does not require Ghostty submodule changes.
Why this also resolves the original #3370 stable report
The `addisonlynch` repro on 0.63.2 stable used a placeholder path (`/any/path/to/file.md`) that does not exist; `CmdClickMarkdownRouteSettings.shouldRoute(path:)` rejects unreadable paths, so on stable they hit the editor-fall-through branch and concluded "the setting is ignored." With a real `.md` file the same stable build hits the recursive-lock crash documented above (verified locally on 0.64.1 → identical commit as 0.63.2's mainline routing path). One bug, two reports.
Scope (what this does NOT do)
Testing
Demo Video
For UI or behavior changes, include a short demo video.
Review Trigger (Copy/Paste as PR comment)
```text
@codex review
@coderabbitai review
@greptile-apps review
@cubic-dev-ai review
```
Checklist
Summary by CodeRabbit