perf: stop invisible windows from rendering, cap the escrow holder leak - #237
Merged
Conversation
Previously ghostty only learned about intra-app pane visibility (setVisibleInUI), never whole-window visibility, so a minimized or fully-covered window kept its renderer and CVDisplayLink running as if on screen. Adds window-level observers (didChangeOcclusionState, didMiniaturize/didDeminiaturize) that track window visibility separately from pane visibility and push their AND to ghostty_surface_set_occlusion whenever either input changes, so neither can clobber the other.
Escrow holder processes never exited: after the app died, a draining session stayed in the registry forever waiting for a retrieval that might never come (crash, force quit, deleted tagged build, an update that reclaims nothing), and the holder's accept loop had no exit path at all even once nothing was left to hold. Adds a reaper thread, started before the accept loop so it can never race the first connection: it sweeps every reaperInterval to close out sessions that have been draining past unclaimedSessionTTL (mirroring handleRetrieveRequest's drain/retrieve coordination so an expiry can never race a drain thread's in-flight read), and exits the holder once its registry has been empty with no live connections for idleExitGrace. Never exits with a non-empty registry or a live connection, matching the same safety precondition retrieval already relies on.
Two defects found in the escrow reaper added by the previous commit: - retireExpiredSessions() discarded the drain-stop wait result and closed the fd unconditionally, even on a timeout where the drain thread might still be mid-read on it. Now mirrors handleRetrieveRequest: on timeout, leave the fd untouched and let HeldSession.deinit's safety net close it once the drain thread (which holds its own strong reference for as long as it runs) actually returns. - reaperTick() could decide to exit based on a stale snapshot of registry/activeConnectionCount taken before releasing the lock, leaving a window where the accept loop could register a new session right before exit() destroyed it. Now the exit decision re-checks the precondition and sets a shuttingDown flag in the same lock acquisition the accept loop checks before registering a connection, so the two can never race.
unclaimedSessionTTL: 600s -> 3600s. Elapsed time alone can't prove a session is abandoned -- a machine that sleeps mid-relaunch, or a user who quits and reopens much later, can legitimately exceed 10 minutes, and a false expiry silently destroys a session someone wanted (SIGHUP to the shell, only scrollback survives). One hour keeps the leak bounded while making false retirement implausible for real relaunch flows; explicit claim/renew reconciliation is the durable fix and is tracked separately. retrieveRecvTimeoutSeconds (5s) -> retrieveRecvTimeout (500ms). SessionEscrowClient.retrieve runs synchronously on the main thread during session restore, once per escrowed panel, serially -- a present-but-unresponsive holder stalled launch by 5s per panel. The holder is a local same-machine AF_UNIX socket that answers in microseconds when healthy, so 500ms is already ~1000x a healthy round trip, and the cost of a miss is just the scrollback-replay fallback, not data loss. The timeval is now built by splitting whole seconds from the fractional remainder instead of truncating to tv_sec, which would have silently zeroed the budget.
The remote daemon is Go at daemon/remote, built via scripts/build_remote_daemon_release_assets.sh. The documented 'cd programad && zig build' referenced a directory that does not exist, failing every agent session that followed it.
This was referenced Aug 3, 2026
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.
What this does
Two costs that never showed up in normal use, found by this morning's perf audit:
Also fixes CLAUDE.md pointing at a
programadZig build that has not existed for a while (the daemon is Go, atdaemon/remote).Review order
Sources/SessionEscrow.swiftacross the three escrow commits, in order. The middle one closes two races the first one introduced (an fd close after a timed-out drain wait, and an exit/accept race). The safety rule throughout: never exit while a session is held, never close an fd a drain thread might still be reading.Sources/GhosttySurfaceScrollView.swift- occlusion is the AND of pane visibility and window visibility, tracked separately so neither can clobber the other.CLAUDE.md- doc fix only.Behavior change to know about
Escrowed sessions nobody reclaims are now dropped after one hour, closing their pty (the shell gets SIGHUP; scrollback still restores from disk on next launch). Before, they were held forever. One hour is deliberately far above any real relaunch, but it is a judgment call - flag if you think it should differ. Explicit claim/renew on relaunch is the durable design and should get an issue.
Test plan
top, wait, restore - confirm it resumes cleanly andtopkept runningps | grep session-escrow-holder)Verified so far
Occlusion wiring confirmed live in a tagged build (debug log shows
effective=0when covered,effective=1on expose). Idle CPU with a visible window measured at ~6%; the occluded-window delta is not yet measured. One earlier claim corrected during verification: the "CVDisplayLink = 69% of activity" figure from the audit was a misread ofsampleoutput (it counts thread presence, not CPU) - the fix stands on the unwired-occlusion finding, not that number.