Conversation
…tive free The ghostty PTY tee callback fires on the io-reader thread for every output chunk until ghostty_surface_free joins that thread, and the MANUAL-mode io_write_cb fires on the io thread the same way. The retained callback userdata must outlive the native free. These tests fail today: every teardown path that defers the free to the runtime teardown coordinator (deinit, and the override-free paths of teardownSurface and agent-hibernation suspend) releases the tee lease and manual IO context immediately, leaving a window where the io-reader thread dereferences freed userdata.
…native surface free ghostty's io-reader thread calls the PTY tee callback for every output chunk, and the io thread calls the MANUAL-mode io_write_cb, right up until ghostty_surface_free joins those threads. The teardown paths that defer the free to the runtime teardown coordinator (deinit, and the override-free paths of teardownSurface and agent-hibernation suspend) released the tee lease and manual IO context immediately, so until the coordinator's worker ran the free — seconds later under load — the reader thread could fire the tee callback into freed userdata. That use-after-free killed unit-test app hosts mid-suite and can take down the app on surface close. Transport the tee lease and manual IO context through the teardown request, exactly like the surface callback context, and release all three on the main actor only after freeSurface returns. The free is the happens-before edge that joins the IO threads, so a callback can never observe released userdata.
|
@ejc3 is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthrough
ChangesRuntime teardown callback lifetime
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant TerminalSurface
participant TerminalSurfaceRuntimeTeardownCoordinator
participant ghostty_surface_free
participant CallbackResources
TerminalSurface->>TerminalSurfaceRuntimeTeardownCoordinator: enqueueRuntimeTeardown(manualIOContext, byteTeeLease)
TerminalSurfaceRuntimeTeardownCoordinator->>ghostty_surface_free: freeSurface()
ghostty_surface_free-->>TerminalSurfaceRuntimeTeardownCoordinator: native free completes
TerminalSurfaceRuntimeTeardownCoordinator->>CallbackResources: release retained resources
Suggested reviewers: 🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 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 |
ejc3
marked this pull request as ready for review
July 14, 2026 08:15
Contributor
Greptile SummaryThis PR keeps terminal callback userdata alive until native surface teardown completes. The main changes are:
Confidence Score: 5/5This looks safe to merge after tightening the deinit ordering test.
Packages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/TerminalSurfaceTeardownCallbackLifetimeTests.swift Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant S as TerminalSurface
participant C as TeardownCoordinator
participant G as Ghostty Surface
participant IO as I/O Threads
participant M as MainActor
S->>C: Enqueue surface and callback userdata
C->>G: ghostty_surface_free(surface)
G->>IO: Stop and join threads
IO-->>G: Callbacks complete
G-->>C: Free returns
C->>M: Release callback contexts and tee lease
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant S as TerminalSurface
participant C as TeardownCoordinator
participant G as Ghostty Surface
participant IO as I/O Threads
participant M as MainActor
S->>C: Enqueue surface and callback userdata
C->>G: ghostty_surface_free(surface)
G->>IO: Stop and join threads
IO-->>G: Callbacks complete
G-->>C: Free returns
C->>M: Release callback contexts and tee lease
Reviews (1): Last reviewed commit: "terminal: release tee and manual-IO call..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+76
to
+77
| await recorder.waitForEventCount(1) | ||
| #expect(recorder.events == [.teeLeaseRelease]) |
Contributor
There was a problem hiding this comment.
This test records only the tee release because deinit does not use runtimeSurfaceFreeOverrideForTesting. It would still pass if the coordinator released the lease before the native free, so the deinit path’s claimed ordering regression is not covered.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Contributor
Author
There was a problem hiding this comment.
Right — as written this test never observed the native free, so it could not catch the wrong ordering. The version of this change that merged to main via #7996 fixes exactly that: it installs the free override and asserts [.nativeFree, .teeLeaseRelease]. Closing this PR since main now carries the full change.
Contributor
Author
|
Superseded: this change merged to main inside #7996 (3d508bc, 2026-07-15) — TerminalSurfaceRuntimeTeardownRequest and the coordinator's free-then-release ordering are byte-identical on main, and main's deinit test is stronger than the one here (it records the native free and asserts the two-event order). Nothing in this branch is missing from main. |
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.
Terminal surfaces could crash the process during teardown: the ghostty io-reader thread delivers every PTY output chunk through a retained C callback (the byte tee, and in MANUAL mode the io_write box), and that thread only stops when
ghostty_surface_freejoins it. Three teardown paths defer the free to the teardown coordinator's background worker but released the callback userdata immediately — so for the window until the worker ran (seconds on a loaded machine), the reader thread invoked the tee callback through a danglingUnmanagedpointer. Under load with surfaces churning (heavy test runs, many panes closing), that is a use-after-free crash; a run of unit-test gates showed nine test-host deaths from this in one evening.The fix makes the ordering structural instead of hopeful: the teardown request now carries the callback context, the manual-IO context, and the tee lease, and the coordinator releases all three only after
freeSurfacereturns. The free is the happens-before edge that joins the io threads, so no callback can observe released userdata — no null checks, no narrowing of the race.Tests pin the invariant deterministically rather than by stress: a recording tee lease and an order recorder assert the release happens strictly after the native free on every teardown path (production deinit, the DEBUG override-free branch, and agent hibernation). On the unfixed code they fail every run with the observed order reversed.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes a use-after-free during terminal teardown by keeping IO callback userdata alive until the native surface free completes. Prevents crashes under load by releasing the callback context, MANUAL I/O write box, and PTY byte-tee lease only after
ghostty_surface_freejoins the IO threads.manualIOContextand PTY byte-tee lease throughTerminalSurfaceRuntimeTeardownRequest; coordinator releases callback context → manual IO context → tee lease on the main actor only afterfreeSurfacereturns, eliminating danglingUnmanagedpointers for tee andio_write_cb.teardownSurface, agent hibernation).Written for commit 5f54f2b. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests