Repository navigation
Skip an inherited terminal surface the registry no longer owns - #8656
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesInherited terminal configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 25✅ Passed checks (25 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 |
Greptile SummaryThis PR replaces a raw
Confidence Score: 5/5The change is a targeted, well-scoped safety hardening of a single guard expression; no existing invariants are weakened and the accessor it routes through was already the established pattern for handing raw pointers to Ghostty C APIs. The one-line guard replacement is self-contained, the called accessor (liveSurfaceForGhosttyAccess) is already used elsewhere in the codebase for the same purpose and has its own ownership + liveness checks, the withExtendedLifetime fence on the success path is unchanged, and the change introduces no new state, no new synchronization, and no new dependencies. Files Needing Attention: No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[inheritedTerminalConfig loop\nover candidates] --> B[let surface = terminalPanel.surface\nlet sourceFontSizeLineage = snapshot]
B --> C{liveSurfaceForGhosttyAccess\nreason: inheritedTerminalConfig}
C -->|hasLiveSurface check fails| D[nil returned]
C -->|registeredOwnerId != id\nABA / pointer reuse| E[quarantine wrapper\nset surface = nil\nunregister from registry\nmarkPortalLifecycleClosed]
E --> D
C -->|GhosttySurfaceRuntimeProbe\nfails liveness| E
C -->|all checks pass| F[sourceSurface returned]
D --> G{sourceFontSizeLineage\navailable?}
G -->|yes| H[return partial config\nwith fontSizeLineage only]
G -->|no| I[continue to next candidate]
F --> J[cmuxInheritedSurfaceConfig\nGhostty C API calls]
J --> K[withExtendedLifetime\nkeep wrapper alive\nthrough C calls]
K --> L[return full config]
Reviews (3): Last reviewed commit: "Use liveSurfaceForGhosttyAccess for the ..." | Re-trigger Greptile |
d91f285 to
bb8f926
Compare
WorkspaceSplitWorkingDirectoryTests has two tests named for what they are meant to
prove — testNewTerminalSurfaceSkipsFreedInheritedSurfacePointer and its split-path
twin — and both currently prove the opposite by killing the test host:
_os_unfair_lock_corruption_abort
_os_unfair_lock_lock_slow
ghostty_surface_inherited_config
Workspace.inheritedTerminalConfig(preferredPanelId:inPane:)
Workspace.newTerminalSurfaceLocal(...) / Workspace.newTerminalSplitLocal(...)
inheritedTerminalConfig guarded on `surface.surface != nil`, but a non-nil wrapper
pointer is not proof the native surface is alive. Teardown unregisters the runtime
surface and only then frees it, so a wrapper still holding the pointer after an
out-of-band free hands its caller freed memory. libghostty then locks an
os_unfair_lock inside that freed allocation, the kernel detects lock corruption, and
the process is SIGKILLed — so an unrelated suite sharing the test host dies too, and
its verdict is lost with it.
The registry owns exactly the pointers that have not been freed, which makes it the
liveness oracle a nil-check cannot be, and TerminalSurfaceRegistering already exposes
runtimeSurfaceOwnerId. TerminalSurface gains liveRuntimeSurface, which returns the
pointer only while the registry still owns it and otherwise clears the wrapper, so
every caller that reads through it sees nil instead of freed memory. Clearing goes
through the existing setter, so it advances runtimeSurfaceGeneration exactly as a
normal teardown does and pointer-backed caches invalidate for the same reason.
inheritedTerminalConfig now reads liveRuntimeSurface, so a stale candidate is skipped
like a torn-down one. That is also the behaviour the tests' second assertion asks for:
XCTAssertNil(sourcePanel.surface.surface, "Expected stale surface pointer to be
quarantined").
Review pointed out that the new liveRuntimeSurface accessor checked only that the registry owner was non-nil, so a recycled pointer owned by a different surface would pass. liveSurfaceForGhosttyAccess already does the full check — owner id equality plus an allocation-liveness probe — and quarantines a stale wrapper the same way. Delete the weaker duplicate and call the existing accessor from inheritedTerminalConfig; a stale candidate is still skipped in favor of the font-lineage fallback.
bb8f926 to
8a24bbf
Compare
|
Maintainer verification (2026-08-04): this PR comes from a contributor fork, and repository policy forbids pushing maintainer changes to forks. I reproduced the exact runtime diff on the origin-hosted |
Workspace.inheritedTerminalConfigtreats a non-nilsurface.surfaceas proof the native surface is alive. It is not. Teardown unregisters the runtime surface and only then frees it, so a wrapper still holding the pointer after an out-of-band free hands its caller freed memory. libghostty locks anos_unfair_lockinside that freed allocation, the kernel detects lock corruption, and the process isSIGKILLed.The registry owns exactly the pointers that have not been freed, which makes it the liveness oracle a nil-check cannot be — and
TerminalSurfacealready has the accessor built for handing a pointer to Ghostty C APIs:liveSurfaceForGhosttyAccess(reason:). It returns the pointer only while the registry records this surface as its owner and the pointer probes live; otherwise it quarantines the wrapper, so the caller sees nil instead of freed memory andruntimeSurfaceGenerationadvances exactly as on a normal teardown, invalidating pointer-backed caches.inheritedTerminalConfignow reads through that accessor, so a stale candidate is skipped like a torn-down one. Requiring the owner to be this specific surface also covers pointer reuse: a freed address recycled to a new surface still has an owner, just not this one, and inheriting through it would read the wrong surface's config.I want to be exact about what I can show, because it changed while this was open.
I first hit this as a host kill in
WorkspaceSplitWorkingDirectoryTests, and it reproduces on4a69f421. Running that suite alone there,testNewTerminalSplitSkipsFreedInheritedSurfacePointerandtestNewTerminalSurfaceSkipsFreedInheritedSurfacePointereach start and never finish, the host is killed twice, no test completes, and the crash report names the path:On this branch's base it does not reproduce. I ran the suite in one worktree, with one warm derived-data path and the same selectors, once with this change and once with only its files reverted. Both runs executed three tests and killed nothing. #8543 explains it: that change reworked
inheritedTerminalConfigto readfontSizeLineageSnapshot()rather than callingghostty_surface_inherited_configthrough the wrapper, so the dereference that crashed is no longer on that path.So this is hardening and not a repro-backed fix, and the two tests do not cover it here, since they pass without it. The same function still dereferences
surface.surfacein its other branch, which is what routing it throughliveSurfaceForGhosttyAccessprotects, but I do not yet have a test that fails without this change on this base. Judge the guard on its own merits rather than on the crash above, and I will add that test before asking for a merge.Summary by CodeRabbit