Repository navigation
renderer: an off-main present never commits while it holds the Core Animation lock (embedder-mode deadlock from #70) - #71
Conversation
…the Core Animation lock (red) cmux FRAME OWNER REFACTOR S4 films, 2026-11-03 (build ada386be, pin 635c6c1): with GHOSTTY_HOST_PRESENT_EMBEDDER on and the app's frame recorder running, the main thread hung for good at the first terminal resize. Sample of the hung app: main in CA::Transaction::commit > commit_transaction > _os_unfair_lock_lock_slow; com.Metal.CompletionQueueDispatch in Frame.bufferCompleted > Metal.present > IOSurfaceLayer.setSurfaceOffMain (the embedder branch); every renderer thread waiting for a swap-chain frame. The embedder branch took [CATransaction lock] before begin and committed while it held it (Zig runs defers last-first: commit, then unlock). The base branch, which the same app ran without a hang under the same recorder load (the switch-OFF films), unlocks before it commits. The test states the invariant directly and needs no timing and no GPU: +[CATransaction lock], +unlock and +commit are swapped for versions that count, per thread, commits made at a lock depth above 0; one off-main present of a frame at the shown size runs on its own thread. It must commit once, at depth 0. Red in the embedder mode on 635c6c1 (1 commit under the lock); the renderer mode is the control (0 on both).
…it commits (embedder mode deadlock) The embedder branch of IOSurfaceLayer.setSurfaceOffMain (PR #70, third review) took [CATransaction lock] before begin and committed under it (Zig runs defers last-first: commit, then unlock). In cmux's S4 films that deadlocked against the main thread's commit: main in CA::Transaction::commit > commit_transaction waiting for a lock, the Metal completion thread in this present, every renderer thread waiting for a swap-chain frame the completion handler never returned. The branch is gone: every off-main present takes the base order (begin, lock, checks, unlock, then commit), which the same app ran without a hang under the same load (the switch-OFF films), with the main_pending gate. The invariant is written at the function: no thread but main commits a transaction while it holds [CATransaction lock]. The removed branch also kept an off-main present from running its checks or setting its frame between an embedder's present and its bounds change. That stays the embedder's call-site rule, now said plainly in ghostty.h: present and give the view its bounds while holding [CATransaction lock] (or inside a Core Animation layout or display pass of the commit), which off-main presents never hold while they commit (Codex review of the first version of this fix). The default mode does not change.
…stty.h says to unlock before a commit, a flush or ghostty_surface_draw Claude review of the first version: main is not exempt from the lock order (a commit or flush under the lock on main deadlocks the same way against an off-main commit), and ghostty_surface_draw under the lock can wait for a frame whose completion waits for the lock. Comments and ghostty.h only.
…ceOffMain (no thread commits under the Core Animation lock)
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
|
Codex review (gpt via No blocking findings in the supplied code.
|
|
Independent review (Claude subagent, read-only, no build) at 3163f4b: SHIP. Findings and what I do with them:
Not verified (Core Animation and Metal internals): whether layout or display passes hold the CA lock, and whether |
… explicit CATransaction lock span with no draw in it docs: ghostty_surface_host_frame's present needs an explicit CATransaction lock span with no draw in it (#71 follow-up)
Summary
GHOSTTY_HOST_PRESENT_EMBEDDERmode,IOSurfaceLayer.setSurfaceOffMain(the GPU completion thread) took[CATransaction lock]beforebegin. Zig runs defers last-first, so the thread committed its transaction while it held the lock.CA::Transaction::commit>commit_transaction, waiting for a lock. The Metal completion thread hung inbufferCompleted>setSurfaceOffMain, in its commit, while it held the CA lock. The renderer threads waited inSwapChain.nextFrame. The sample without the recorder did not hang. The default mode (GHOSTTY_HOST_PRESENT_RENDERER) never hung under the same load: it already didbegin, lock, checks, unlock,commit.main_pendingclear). Otherwise it goes through main (setSurface).setSurfaceOffMain,ghostty.handNEXT.md): no thread commits a Core Animation transaction, or flushes, while it holds[CATransaction lock].ghostty.hcall-site rule forghostty_surface_host_frame(present: true): present and set the bounds in one lock span, or in one Core Animation layout or display pass of the commit. An open transaction alone is not enough. Unlock before any commit or flush. Never callghostty_surface_drawunder the lock.fresh.mutexinpublishFreshand releases it before it takes the CA lock. Under the CA lock it does only the shown-size and sequence checks and the contents set. Main takes the CA lock, thenfresh.mutex(takeFresh).fresh.mutexis a leaf on both threads, so no cycle.ghostty_surface_host_framenever waits. Main does wait for a free frame, and so for the completion thread, in a first-frame display pass and inghostty_surface_draw, as before this PR (ghostty.h says so). cmux on macOS never callsghostty_surface_draw.commit_transactionwaiting for that lock, but one frame does not prove that it holds the lock for all of its layer work. The S4 films judge it (two terminals in one span, with echo in a third). The gate (main_pending) and the mode are still read under the lock, so the checks for this surface stay correct.Testing
host present: an off-main present never commits while it holds the Core Animation lock (embedder), and the same for(renderer)as a control. It replaces+[CATransaction lock],+unlockand+commitwith counting versions on the metaclass. It runs onesetSurfaceOffMainon a spawned thread and expects one commit with no commit under the lock. It uses no GPU and no timing.b0c7e291a, run https://github.com/manaflow-ai/ghostty-next/actions/runs/41694823112 onscratch/off-main-lock-red-2, which only narrows the filter tohost present:. The embedder test fails withexpected 0, found 1. The renderer control passes (102 of 103).6a7472263, run https://github.com/manaflow-ai/ghostty-next/actions/runs/41694827381 onscratch/off-main-lock-green-2: ghostty-test 241/241, lib-vt 475/475.4338c404eand3163f4b73change only comments,ghostty.handNEXT.md.Observed / Not run
3163f4b73: Codex SHIP and Claude SHIP (comments on this PR). The header text fix from the Claude review goes in a follow-up fork PR.TerminalFrameOwnerMainThreadTests(main must keep turning under the recorder load) on a cmux build pinned to this fix. It goes in the cmux pin PR (#33956), with its red step on the old pin.Changelog
none