[lawrencecchen]

Summary

• Follow-up to renderer: an off-main present never commits while it holds the Core Animation lock (embedder-mode deadlock from #70) #71 (the Claude review, should-fix 3). The header rule for ghostty_surface_host_frame(present: true) said "in one [CATransaction lock] span (or one Core Animation layout or display pass of the commit)". It is not known whether Core Animation holds the lock in a layout or display pass. Also, a display pass of a surface with nothing shown draws on main: drawFrame(true) waits for a free frame, and the GPU completion thread frees that frame only after it took the lock.
• The rule now allows only an explicit lock span. In that span: no commit, no flush, and no draw (ghostty_surface_draw, or a first-frame display pass).
• The example call site is now a main run-loop observer just before Core Animation's commit (what cmux's TerminalFramePresenter does), not a layer that Core Animation displays.
• The comment in the commit-order test now says that the test counts commits only. The off-main present calls no flush.
• Comments only. No code changes, no test changes and no workflow changes. cmux does not need to move its pin for this change (it pins 7d43bb68b, the renderer: an off-main present never commits while it holds the Core Animation lock (embedder-mode deadlock from #70) #71 merge).

Testing

• Comment-only diff: git diff 7d43bb68b...db8627d95 -- src/ changes only // lines.
• PR CI of this branch.

Observed / Not run

• Observed: the diff above.
• Not run: no test run is necessary for a comment change.

Changelog

none

---

^{Open in [code]smith · Need help on this PR? Tag @codesmith-bot with what you need.}

---

Summary by cubic

Follow-up to #71's review: tightens the locking rule for ghostty_surface_host_frame(present: true). The rule previously accepted either an explicit [CATransaction lock] span or a Core Animation layout or display pass; only the explicit lock span is valid now, since it's unknown whether Core Animation holds the lock in those passes and a display pass of a surface with nothing shown draws on main and can deadlock. The docs now also forbid drawing in the span, the example call site is a main run-loop observer just before Core Animation's commit (what cmux's TerminalFramePresenter does) instead of a displayed layer, and the commit-order test's comment now says it counts commits only. Comments only: no code, test, or workflow changes, so cmux does not need to move its pin.

^{Written for commit db8627d. Summary will update on new commits.}

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: e252d330-8134-4da9-8d3a-8ce8d051b0d4

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• Trigger review

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

• X
• Mastodon
• Reddit
• LinkedIn

CodeRabbit Verify

• Run CodeRabbit Verify to test the changes in this pull request.

[lawrencecchen]

Review (codex): SHIP at db8627d

Codex review (codex exec -s read-only, code inline: the diff, displayCallback, present, displayWhileEmbedderPresents, bufferCompleted, setSurfaceOffMain, drawFrame). Findings:

No blocking findings.

• Header accuracy — include/ghostty.h:1578: A visible, host-sized surface with nothing shown makes displayWhileEmbedderPresents return false, so displayCallback calls drawFrame(true) on main. A healthy asynchronous completion can enter setSurfaceOffMain, acquire CATransaction.lock, and commit before returning to frameCompleted. Holding that lock while waiting for a free frame can therefore deadlock.
• Verification limit — src/renderer/generic.zig:2093: The supplied excerpt shows the nextFrame() call but omits its implementation and frameCompleted’s body. It does not independently establish that frame acquisition waits or precisely where the frame becomes free. The header’s explanation is consistent with the stated behavior.
• Call-site rule — include/ghostty.h:1574: Requiring an explicit lock span closes the previous layout/display-pass exception. cmux’s described main run-loop observer complies: it locks around the present and bounds change, then unlocks before Core Animation commits. Any synchronous display triggered inside that span must also be avoided, as the new text requires.
• Diff scope — both files: The supplied diff changes only // comments; no declarations, executable code, or test assertions change.