fix: typing/CLI race on terminal surfaces (#3339) - #3341
austinywang wants to merge 2 commits into
Conversation
The repro shows local typed input losing characters while read-screen, tree, and sibling send commands pressure the same workspace. This regression adds a simulated terminal surface that models stale CLI refresh state overwriting a concurrently typed line, with the operation gate intentionally still a pass-through in this commit. Constraint: Local tests are not run per task and repo instructions; this commit is the failing-test half of the required two-commit structure. Confidence: medium Scope-risk: narrow Tested: Not run locally by instruction Not-tested: XCTest execution; final verification is deferred until after the fix/build path
Local keyboard input, socket send/read-screen, refresh, focus, geometry, and teardown all cross the same native Ghostty surface boundary. The repro shows a stale CLI-side surface operation can overlap a typed line and drop rendered characters, so this adds a recursive operation gate and routes the relevant surface API calls through it. Constraint: Do not use sleeps/retries; preserve socket command behavior and keep the change inside the native surface access boundary. Rejected: Debounce or slow cmux read-screen/tree/send loops | hides the race and changes automation responsiveness Rejected: Only lock cmux send | read-screen/refresh and local typing still share the native surface state Confidence: medium Scope-risk: moderate Tested: git diff --check Not-tested: Local XCTest by instruction; final tagged reload build still pending
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughIntroduces Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 1/8 review remaining, refill in 50 minutes and 36 seconds.Comment |
Greptile SummaryFixes the typing/CLI race (#3339) by introducing Two design concerns stand out:
Confidence Score: 4/5Safe to merge as a correctness fix; the global-lock scope and keystroke-path latency are design trade-offs worth revisiting but not blockers. All findings are P2. The gate correctly eliminates the reported data corruption. The global lock scope and typing-path latency are real concerns in multi-surface workloads and under heavy CLI load, but neither is an immediate correctness regression. The missing two-commit structure is a process violation only. Sources/GhosttyTerminalView.swift — global lock scope and keystroke-path lock acquisition deserve follow-up before this pattern is extended further. Important Files Changed
Sequence DiagramsequenceDiagram
participant KB as Local Keyboard (Main Thread)
participant Gate as GhosttySurfaceOperationGate
participant CLI as CLI Thread (cmux read-screen)
participant GS as Ghostty Surface C API
CLI->>Gate: sync { ghostty_surface_read_text }
Gate-->>CLI: lock acquired
CLI->>GS: ghostty_surface_read_text(surface, ...)
Note over CLI,GS: CLI holds lock for duration of read
KB->>Gate: sendGhosttyKey → sync { ghostty_surface_key }
Note over KB,Gate: ⚠️ Blocked — waits for CLI to release
CLI->>GS: ghostty_surface_free_text(surface, ...)
Gate-->>CLI: lock released
KB->>Gate: sync { ghostty_surface_key }
Gate-->>KB: lock acquired
KB->>GS: ghostty_surface_key(surface, keyEvent)
Gate-->>KB: lock released
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| #if DEBUG | ||
| Self.debugGhosttySurfaceKeyEventObserver?(keyEvent) | ||
| #endif | ||
| return ghostty_surface_key(surface, keyEvent) | ||
| return GhosttySurfaceOperationGate.sync { | ||
| ghostty_surface_key(surface, keyEvent) | ||
| } |
There was a problem hiding this comment.
sendGhosttyKey — and by extension the entire keyDown / keyUp pipeline — now acquires GhosttySurfaceOperationGate on every keystroke. Per CLAUDE.md, this path is explicitly flagged as latency-sensitive: "Do not add allocations, file I/O, or formatting here." Under the exact high-frequency-CLI scenario described in the PR, a cmux read-screen holding the lock while reading a large buffer will block every pending keystroke for the full duration of that read.
The PR's own test demonstrates the CLI hold time can reach 100 ms (Thread.sleep(forTimeInterval: 0.10)). With a global lock this directly translates to 100 ms of input stall per read cycle.
![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 the current code and only fix it if needed.
Inline comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 21-33: The current gate allows callers to capture self.surface
before locking, risking use-after-free when teardownSurface/deinit nils and
frees the pointer; add a new helper on GhosttySurfaceOperationGate (e.g.
withLockedSurface<T>(_ body: (OpaquePointer) throws -> T) rethrows -> T?) that
calls sync and, inside the lock, reads self.surface, validates it's non-nil and
not freed, then passes the safe snapshot into the body; update call sites that
currently do guard let surface = surface before locking (sendText, sendNamedKey,
updateSize, setFocus and the GhosttyNSView send/read helpers) to use
withLockedSurface so the surface is captured and validated inside
GhosttySurfaceOperationGate.sync, and ensure teardownSurface/free paths still
nil and free the pointer 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4f65287c-e27b-45c6-aca6-de953efa8c24
📒 Files selected for processing (3)
Sources/GhosttyTerminalView.swiftSources/TerminalController.swiftcmuxTests/TerminalAndGhosttyTests.swift
| enum GhosttySurfaceOperationGate { | ||
| private static let lock: NSRecursiveLock = { | ||
| let lock = NSRecursiveLock() | ||
| lock.name = "com.cmux.ghostty-surface-operation-gate" | ||
| return lock | ||
| }() | ||
|
|
||
| static func sync<T>(_ operation: () throws -> T) rethrows -> T { | ||
| lock.lock() | ||
| defer { lock.unlock() } | ||
| return try operation() | ||
| } | ||
| } |
There was a problem hiding this comment.
Serialize surface lookup with the operation, not just the FFI call.
The new gate still lets callers capture self.surface before locking. That leaves a use-after-free window: teardownSurface()/deinit nil out self.surface, then free the old pointer under this gate later; a background sender can grab the old pointer first, block behind ghostty_surface_free, and then call ghostty_surface_text/ghostty_surface_key on freed memory once the lock opens. Please add a helper that snapshots and validates the live surface inside GhosttySurfaceOperationGate.sync and route the send/size/focus paths through it.
Suggested direction
enum GhosttySurfaceOperationGate {
private static let lock: NSRecursiveLock = {
let lock = NSRecursiveLock()
lock.name = "com.cmux.ghostty-surface-operation-gate"
return lock
}()
static func sync<T>(_ operation: () throws -> T) rethrows -> T {
lock.lock()
defer { lock.unlock() }
return try operation()
}
}
+
+extension TerminalSurface {
+ private func withLockedSurface<T>(_ body: (ghostty_surface_t) throws -> T) rethrows -> T? {
+ try GhosttySurfaceOperationGate.sync {
+ guard let surface = self.surface else { return nil }
+ return try body(surface)
+ }
+ }
+}Then convert paths that currently do guard let surface = surface before locking, e.g. sendText, sendNamedKey, updateSize, setFocus, and the corresponding GhosttyNSView send/read helpers, to use withLockedSurface(...).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/GhosttyTerminalView.swift` around lines 21 - 33, The current gate
allows callers to capture self.surface before locking, risking use-after-free
when teardownSurface/deinit nils and frees the pointer; add a new helper on
GhosttySurfaceOperationGate (e.g. withLockedSurface<T>(_ body: (OpaquePointer)
throws -> T) rethrows -> T?) that calls sync and, inside the lock, reads
self.surface, validates it's non-nil and not freed, then passes the safe
snapshot into the body; update call sites that currently do guard let surface =
surface before locking (sendText, sendNamedKey, updateSize, setFocus and the
GhosttyNSView send/read helpers) to use withLockedSurface so the surface is
captured and validated inside GhosttySurfaceOperationGate.sync, and ensure
teardownSurface/free paths still nil and free the pointer as before.
This branch was successfully deployed
1 participant
Summary
Fixes #3339 by serializing Ghostty surface operations that can be reached from both local terminal typing and cmux CLI commands.
Reproduction
Before changing code, I reproduced the corruption against an existing tagged DEBUG build using the debug socket and
/tmp/cmux-cli:workspace:13surface:13surface:14caton the typing surface.cmux read-screen,cmux tree, and siblingcmux sendcommands ran against the same workspace:Observed corrupted rendered line under CLI pressure:
Root Cause
The local keyboard path and socket/CLI paths both reached Ghostty surface APIs without a single serialization boundary. Main-queue scheduling ordered some Swift work, but Ghostty surface reads, writes, refreshes, focus/geometry changes, and teardown could still interleave through separate call sites, allowing CLI-side stale refresh/read activity to race with local input echo/rendering.
Fix
GhosttySurfaceOperationGatebacked byNSRecursiveLock.Verification
git diff --check./scripts/reload.sh --tag issue-3339-typing-cli-race --launchLocal XCTest runs were intentionally not run because repository instructions for this task said not to run local tests.
Note
Medium Risk
Touches many call sites around the Ghostty surface C-API and changes their synchronization behavior; bugs could manifest as deadlocks or latency regressions on input/render paths if the lock is misused.
Overview
Prevents a typing corruption race by introducing
GhosttySurfaceOperationGate(anNSRecursiveLock) and routing Ghostty surface operations through it.This wraps surface lifecycle (create/free), geometry/display updates, focus/occlusion changes, text/selection reads, refreshes, and key/text input calls in both
GhosttyTerminalViewandTerminalControllerso CLI/socket commands can’t interleave with local typing on the same runtime surface.Adds a regression test (
TerminalSurfaceTypingCLIRaceTests) that simulates concurrent typing and a CLI refresh and asserts the final rendered line remains intact.Reviewed by Cursor Bugbot for commit 1feab55. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Serialize Ghostty surface operations to prevent typed input from being corrupted when CLI commands (read-screen/tree/send) run at the same time. Fixes #3339.
GhosttySurfaceOperationGateusingNSRecursiveLockto serialize nativeghostty_surface_*reads/writes/refresh/focus/geometry/free.GhosttyTerminalView,GhosttyNSView, andTerminalControllerthrough the gate.Written for commit 1feab55. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Release Notes