fix: guard inherited terminal config against stale surfaces - #2101
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
To use Codex here, create a Codex account and connect to github. |
📝 WalkthroughWalkthroughAdds a runtime surface ownership registry and a MainActor guarded-access API that validates wrapper ownership and native-pointer liveness before exposing Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller (e.g., newTerminalSplit)
participant TSurface as TerminalSurface
participant Registry as TerminalSurfaceRegistry
participant Malloc as malloc_zone/_size
participant Ghostty as Ghostty C API
Caller->>TSurface: liveSurfaceForGhosttyAccess(reason)
TSurface->>Registry: runtimeSurfaceOwnerId(ptr)?
Registry-->>TSurface: ownerId matches? / ownership info
alt ownership missing or mismatch
TSurface->>TSurface: Quarantine (clear callbacks, unregister, nil surface, seal lifecycle)
TSurface-->>Caller: nil (access denied)
else ownership matches
TSurface->>Malloc: cmuxSurfacePointerAppearsLive(ptr)?
Malloc-->>TSurface: appears live?
alt pointer appears freed
TSurface->>TSurface: Quarantine (clear callbacks, unregister, nil surface, seal lifecycle)
TSurface-->>Caller: nil (quarantined)
else pointer live
TSurface->>Ghostty: safe Ghostty C API call with ptr
Ghostty-->>TSurface: result
TSurface-->>Caller: ghostty_surface_t
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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. Comment |
Contributor
Greptile SummaryThis PR fixes a crash (#2024) where stale Key changes:
Minor issues found:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller as Caller (Workspace / TabManager)
participant TS as TerminalSurface
participant Reg as TerminalSurfaceRegistry
participant Ghostty as Ghostty C API
Note over TS,Reg: Surface creation (createSurface)
TS->>Ghostty: ghostty_surface_new(...)
Ghostty-->>TS: ghostty_surface_t (createdSurface)
TS->>Reg: registerRuntimeSurface(createdSurface, ownerId: id)
Note over Caller,Ghostty: Config inheritance (normal path)
Caller->>TS: liveSurfaceForGhosttyAccess(reason:)
TS->>TS: guard hasLiveSurface
TS->>Reg: runtimeSurfaceOwnerId(surface)
Reg-->>TS: ownerId (== self.id)
TS->>Ghostty: cmuxSurfacePointerAppearsLive(surface)
Ghostty-->>TS: true
TS-->>Caller: ghostty_surface_t ✓
Caller->>Ghostty: ghostty_surface_inherited_config(...)
Note over Caller,Ghostty: Config inheritance (stale pointer path)
Caller->>TS: liveSurfaceForGhosttyAccess(reason:)
TS->>TS: guard hasLiveSurface
TS->>Reg: runtimeSurfaceOwnerId(surface)
Reg-->>TS: nil (already unregistered / reused)
TS->>Reg: unregisterRuntimeSurface(surface)
TS->>TS: self.surface = nil (quarantine)
TS->>TS: markPortalLifecycleClosed
TS-->>Caller: nil — skip inherited config
Note over TS,Reg: Surface teardown
TS->>Reg: unregisterRuntimeSurface(surface)
TS->>Ghostty: ghostty_surface_free(surface)
Reviews (1): Last reviewed commit: "fix: guard inherited terminal config aga..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+2671
to
+2675
| func ownsRuntimeSurface(_ surface: ghostty_surface_t, ownerId: UUID) -> Bool { | ||
| lock.lock() | ||
| defer { lock.unlock() } | ||
| return runtimeSurfaceOwners[UInt(bitPattern: surface)] == ownerId | ||
| } |
Contributor
There was a problem hiding this comment.
ownsRuntimeSurface is dead code
ownsRuntimeSurface is defined here but never called anywhere in the codebase. It adds unnecessary public API surface to TerminalSurfaceRegistry and will confuse future readers about whether it is actually used.
Suggested change
| func ownsRuntimeSurface(_ surface: ghostty_surface_t, ownerId: UUID) -> Bool { | |
| lock.lock() | |
| defer { lock.unlock() } | |
| return runtimeSurfaceOwners[UInt(bitPattern: surface)] == ownerId | |
| } |
| window.makeKeyAndOrderFront(nil) | ||
| window.displayIfNeeded() | ||
| contentView.layoutSubtreeIfNeeded() | ||
| RunLoop.current.run(until: Date().addingTimeInterval(0.05)) |
Contributor
There was a problem hiding this comment.
RunLoop.current.run(until: Date().addingTimeInterval(0.05)) is a timed wait that relies on the Ghostty runtime surface being created within 50 ms. Surface creation can be slower on loaded CI machines or under Rosetta 2, making the subsequent XCTAssertNotNil(sourcePanel.surface.surface, ...) assertion spuriously fail and turn both new regression tests into false negatives.
Consider polling with a tight retry loop (e.g. XCTAssertTrue(waitForCondition({ sourcePanel.surface.surface != nil }, timeout: 2))) or using XCTestExpectation / fulfillment(of:timeout:) to wait for the surface to materialise, rather than a fixed sleep.
| guard hasLiveSurface, let surface else { return nil } | ||
| let registry = TerminalSurfaceRegistry.shared | ||
| let registeredOwnerId = registry.runtimeSurfaceOwnerId(surface) | ||
| let registeredOwnerToken = registeredOwnerId.map { String($0.uuidString.prefix(5)) } ?? "nil" |
Contributor
There was a problem hiding this comment.
registeredOwnerToken unused in Release builds
registeredOwnerToken is computed unconditionally on every call path, but it is only consumed inside the #if DEBUG block a few lines below. In Release builds Swift will emit an "immutable value 'registeredOwnerToken' was never used" warning because the #if DEBUG section is entirely excluded from compilation.
Move the let registeredOwnerToken = ... declaration to inside the #if DEBUG block (just before the dlog(...) call) so it is only compiled when it is actually referenced.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 2665-2669: The unregisterRuntimeSurface currently removes the
entry by pointer only; change its signature to accept the expected ownerId (e.g.
func unregisterRuntimeSurface(_ surface: ghostty_surface_t, ownerId: UInt) ) and
implement a compare-and-remove: compute key = UInt(bitPattern: surface), look up
runtimeSurfaceOwners[key], and only remove if the stored ownerId equals the
passed ownerId; otherwise do nothing. Update all teardown/quarantine callers to
pass the wrapper's id when calling unregisterRuntimeSurface so we never erase an
entry belonging to a new owner.
- Around line 2958-2984: The code currently exposes the raw surface pointer and
bypasses quarantine; change direct uses of the surface property so all C calls
go through a guarded accessor that invokes liveSurfaceForGhosttyAccess(reason:),
make the raw surface storage private (e.g. rename surface to _surface) and add
two explicit APIs: a safeSurfaceForGhosttyAccess(reason:) that returns
ghostty_surface_t? (calls liveSurfaceForGhosttyAccess) for all attachToView,
updateSize, forceRefresh, setFocus, performBindingAction, and
GhosttyNSView.surface call sites, and a separate identityOnlySurfaceToken() (or
runtimeSurfaceOwnerId()) that returns an identity token used only for
comparisons and tests (used by replaceSurfaceWithFreedPointerForTesting). Update
callers to use the safe accessor for C interactions and the identity-only API
for equality checks so tests can simulate freed-pointer wrappers without risking
raw pointer dereference.
In `@Sources/Workspace.swift`:
- Around line 6970-6972: Before calling
terminalPanel.surface.liveSurfaceForGhosttyAccess, read and stash the
candidate’s rooted font from
terminalInheritanceFontPointsByPanelId[terminalPanel.id] into a local variable;
if the guard fails (surface is quarantined) use that stashed value as the
fallback override for zoom/font inheritance instead of simply continuing and
dropping the map entry. In practice: capture the existing rooted font before the
guard, and when liveSurfaceForGhosttyAccess returns nil, propagate or reassign
that captured value as the fallback donor for downstream inheritance logic
rather than losing terminalInheritanceFontPointsByPanelId[terminalPanel.id].
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fdc5d343-e611-4359-854d-8458f704a45d
📒 Files selected for processing (4)
Sources/GhosttyTerminalView.swiftSources/TabManager.swiftSources/Workspace.swiftcmuxTests/WorkspaceUnitTests.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/GhosttyTerminalView.swift">
<violation number="1" location="Sources/GhosttyTerminalView.swift:2665">
P1: `unregisterRuntimeSurface` unconditionally removes the pointer from the registry, which will corrupt tracking if a stale wrapper unregisters a pointer that has been reallocated to a new surface. It must require an `ownerId` and verify it before removal.</violation>
</file>
<file name="cmuxTests/WorkspaceUnitTests.swift">
<violation number="1" location="cmuxTests/WorkspaceUnitTests.swift:813">
P3: Replace the fixed 50ms RunLoop sleep with a condition-based wait (polling or XCTest expectation) to avoid flaky failures when surface creation takes longer under CI load.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Sources/GhosttyTerminalView.swift (1)
2847-2857:⚠️ Potential issue | 🟠 MajorGuarded access is still opt-in.
Line 2847 still exposes
surfacemodule-wide, so this contract is easy to bypass. The file still has raw Ghostty call paths (attachToView,updateSize,forceRefresh,setFocus,performBindingAction,GhosttyNSView.surface), and the providedSources/AppDelegate.swiftsnippets still gate readiness offterminalPanel.surface.surface != nil. That means a stale wrapper can still hit freed memory on redraw/input/sendText even though inherited-config callers now use the guarded accessor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 2847 - 2857, The surface property is still externally accessible (private(set) var surface) so callers can bypass the hasLiveSurface contract and dereference freed pointers; make the stored surface fully private and force all external code paths to go through the guarded accessor pattern (liveSurfaceForGhosttyAccess(reason:)) or a new safe helper (e.g., withLiveSurface(_ closure: (ghostty_surface_t) -> Void) that returns false/throws if no live surface) and update all call sites that currently reference GhosttyTerminalView.surface or GhosttyNSView.surface — including attachToView, updateSize, forceRefresh, setFocus, performBindingAction and the AppDelegate readiness checks — to use the guarded API so raw Ghostty C calls never see a possibly-freed pointer.
🤖 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 3100-3126: The quarantined/closed TerminalSurface can be
immediately resurrected because liveSurfaceForGhosttyAccess clears surface but
attachSurface(_:)/TerminalSurface.attachToView(_:) only checks surface == nil
before calling createSurface(for:), allowing re-registration into a permanently
closed wrapper; update the attach/create flow to either (A) prevent re-creation
when the wrapper is in a quarantined/closed state by checking hasLiveSurface or
a new isQuarantined flag before calling createSurface(for:) (symbols:
GhosttyNSView.attachSurface(_:), TerminalSurface.attachToView(_:),
createSurface(for:), hasLiveSurface, canAcceptPortalBinding, surface) or (B)
explicitly reopen the portal lifecycle before re-registering the new native
surface by clearing the quarantined state and calling the lifecycle open path
(symbols: markPortalLifecycleClosed(reason:), recordTeardownRequest(reason:),
surfaceCallbackContext) so that re-registration is only allowed on a
legitimately open TerminalSurface; choose one approach and apply it at the
attach/create entry to eliminate the rogue re-registration.
---
Duplicate comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 2847-2857: The surface property is still externally accessible
(private(set) var surface) so callers can bypass the hasLiveSurface contract and
dereference freed pointers; make the stored surface fully private and force all
external code paths to go through the guarded accessor pattern
(liveSurfaceForGhosttyAccess(reason:)) or a new safe helper (e.g.,
withLiveSurface(_ closure: (ghostty_surface_t) -> Void) that returns
false/throws if no live surface) and update all call sites that currently
reference GhosttyTerminalView.surface or GhosttyNSView.surface — including
attachToView, updateSize, forceRefresh, setFocus, performBindingAction and the
AppDelegate readiness checks — to use the guarded API so raw Ghostty C calls
never see a possibly-freed pointer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 28b468cd-ec51-4c2b-90f0-854f99bcd202
📒 Files selected for processing (4)
Sources/GhosttyTerminalView.swiftSources/TabManager.swiftSources/Workspace.swiftcmuxTests/WorkspaceUnitTests.swift
✅ Files skipped from review due to trivial changes (1)
- cmuxTests/WorkspaceUnitTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/Workspace.swift
Comment on lines
+3100
to
+3126
| @MainActor | ||
| func liveSurfaceForGhosttyAccess(reason: String) -> ghostty_surface_t? { | ||
| guard hasLiveSurface, let surface else { return nil } | ||
| let registry = TerminalSurfaceRegistry.shared | ||
| let registeredOwnerId = registry.runtimeSurfaceOwnerId(surface) | ||
| guard registeredOwnerId == id, | ||
| cmuxSurfacePointerAppearsLive(surface) else { | ||
| let callbackContext = surfaceCallbackContext | ||
| surfaceCallbackContext = nil | ||
| registry.unregisterRuntimeSurface(surface, ownerId: id) | ||
| self.surface = nil | ||
| activePortalHostLease = nil | ||
| recordTeardownRequest(reason: reason) | ||
| markPortalLifecycleClosed(reason: reason) | ||
| #if DEBUG | ||
| let registeredOwnerToken = registeredOwnerId.map { String($0.uuidString.prefix(5)) } ?? "nil" | ||
| dlog( | ||
| "surface.lifecycle.stale surface=\(id.uuidString.prefix(5)) " + | ||
| "workspace=\(tabId.uuidString.prefix(5)) reason=\(reason) " + | ||
| "registryOwner=\(registeredOwnerToken)" | ||
| ) | ||
| #endif | ||
| callbackContext?.release() | ||
| return nil | ||
| } | ||
| return surface | ||
| } |
There was a problem hiding this comment.
Quarantined wrappers can immediately resurrect a new native surface.
This path seals the wrapper as .closed and clears surface, but the existing attach flow only checks surface == nil. On the next GhosttyNSView.attachSurface(_:) / TerminalSurface.attachToView(_:), createSurface(for:) can run again and Line 3714 re-registers a fresh pointer on a permanently closed TerminalSurface. After that, hasLiveSurface / canAcceptPortalBinding stay false while raw callers can still drive the new surface. Either block recreation after quarantine or explicitly reopen the lifecycle before re-registering.
Also applies to: 3714-3715
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/GhosttyTerminalView.swift` around lines 3100 - 3126, The
quarantined/closed TerminalSurface can be immediately resurrected because
liveSurfaceForGhosttyAccess clears surface but
attachSurface(_:)/TerminalSurface.attachToView(_:) only checks surface == nil
before calling createSurface(for:), allowing re-registration into a permanently
closed wrapper; update the attach/create flow to either (A) prevent re-creation
when the wrapper is in a quarantined/closed state by checking hasLiveSurface or
a new isQuarantined flag before calling createSurface(for:) (symbols:
GhosttyNSView.attachSurface(_:), TerminalSurface.attachToView(_:),
createSurface(for:), hasLiveSurface, canAcceptPortalBinding, surface) or (B)
explicitly reopen the portal lifecycle before re-registering the new native
surface by clearing the quarantined state and calling the lifecycle open path
(symbols: markPortalLifecycleClosed(reason:), recordTeardownRequest(reason:),
surfaceCallbackContext) so that re-registration is only allowed on a
legitimately open TerminalSurface; choose one approach and apply it at the
attach/create entry to eliminate the rogue re-registration.
This was referenced Mar 30, 2026
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
…-ai#2101) * test: add stale inherited surface regression * fix: guard inherited terminal config against stale surfaces * fix: address stale surface review feedback
This was referenced Apr 9, 2026
This was referenced May 3, 2026
This was referenced May 14, 2026
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
./scripts/setup.sh./scripts/reload.sh --tag issue-2024-config-crashDemo Video
For UI or behavior changes, include a short demo video (GitHub upload, Loom, or other direct link).
Review Trigger (Copy/Paste as PR comment)
Checklist
Summary by cubic
Fixes #2024 by guarding inherited terminal config against stale Ghostty surfaces and adding safe fallbacks. Prevents crashes when creating splits or new terminals by validating surface ownership/liveness and using recorded font size when the source is stale.
TerminalSurfaceRegistrywith pointer→owner UUID; register on create and unregister on teardown/free/deinit.liveSurfaceForGhosttyAccess(reason:)to verify registry ownership and liveness; quarantine stale/mis-owned pointers, close the portal, and release callbacks.TabManager), and font-size probes.cmuxSurfacePointerAppearsLive/cmuxPointerAppearsLivechecks for surfaces and unretained QuickLook font pointers; verify the surface before font probes.Written for commit eaf4856. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests