Skip to content

fix: drain app mailbox while freeing terminal surfaces - #819

Merged
umputun merged 1 commit into
umputun:masterfrom
bot-rogerthat:fix/surface-mailbox-shutdown
Oct 27, 2026
Merged

umputun merged 1 commit into
umputun:masterfrom
bot-rogerthat:fix/surface-mailbox-shutdown

Conversation

@bot-rogerthat

@bot-rogerthat bot-rogerthat commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Closing a busy terminal surface can freeze every agterm window. The app thread joins the surface's workers while one of those workers is blocked posting into the full 64-slot app mailbox, which only the app thread drains. The captured 0.37.0 hang was traced to processExitCommon posting .child_exited with .forever. One- and eight-producer title-spam cases also hang on the installed Release; the eight-idle-surface control passes.

Backport the teardown drain from ghostty-org/ghostty#14444 to the pinned 683d8db libghostty. A temporary consumer keeps the mailbox moving until the closing surface's workers and in-flight Metal frames have finished, preserving other surfaces' messages for the next app tick. This also lets a frame completion report a health change before releasing its swap-chain permit. The backport retains the pin's redraw messages, removes stale deferred messages across consecutive closes, frees owned payloads when discarding messages, and bounds the final drain so continuing output cannot keep it alive indefinitely.

setup.sh and the GhosttyKit cache include the patch digest. The patch README describes how to run the Zig teardown regressions when updating the patch; CI closes a surface in isolated Release instances with one title producer, eight title producers, and eight idle surfaces. Remove the backport once the pin includes an equivalent upstream fix.

The full CI run passed for the final revision, including the Release build, all three surface-close scenarios, unit tests, strict SwiftLint, 45 session-host fixtures and 1,435 hosted tests with eight skips. The Zig teardown tests, including a simulated frame completion blocked on the mailbox, passed in a separate validation run against the identical patch. That run reached its 30-minute job limit during the later hosted tests; the final run passed after removing the temporary Zig invocation from setup.sh.

Related to ghostty-org/ghostty#14245

@bot-rogerthat
bot-rogerthat force-pushed the fix/surface-mailbox-shutdown branch 2 times, most recently from 0fef913 to d7dbcfe Compare October 26, 2026 15:19
@bot-rogerthat
bot-rogerthat marked this pull request as ready for review October 26, 2026 15:49

@umputun umputun left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one ordering issue left in the patch, plus a few small setup/doc things.

  • scripts/ghostty-patches/0001-drain-mailbox-during-surface-teardown.patch:324 - drain.finish() runs before self.renderer.deinit(), but up to 3 Metal frames can still be in flight after the renderer thread joins. On a health change frameCompleted (renderer/generic.zig:1915) pushes renderer_health into the app mailbox with .forever before releaseFrame, and SwapChain.deinit waits for those permits on the main thread, so a GPU error near a close while other surfaces refill the mailbox freezes every window again. Fix: move drain.finish() to right after self.renderer.deinit() and widen the README wording to cover in-flight frames. Upstream 14444 has the same ordering, so it is worth noting there too.
  • scripts/ghostty-patches/README.md:13 - "remove the patch" as written breaks setup: with no *.patch left, setup.sh:36 fails on the unmatched glob under set -euo pipefail. Pls spell out the removal: either keep GHOSTTY_STAMP="$GHOSTTY_REV" or restore both the stamp comparison (setup.sh:69) and the write (setup.sh:181), and drop the digest line and the apply loop. Also say that 14444 only counts as equivalent once its drain also outlives renderer.deinit().
  • scripts/setup.sh:160 - the Zig teardown test build adds about 4 minutes to every cold libghostty build, on every machine and every CI cache miss. Pls move it to the README regeneration steps next to git apply --check and zig fmt --check, the same way scripts/zmx-patches/README.md handles zig build test, and drop the README line saying the tests run during the source build. The Release teardown harness in CI stays as is.
  • ARCHITECTURE.md:62 and the setup.sh:12-15 header still describe an unpatched build that only a revision change rebuilds. Pls mention the patch step and the patch digest in the stamp.
  • docs/backlog/surface-free-deadlock-waits-on-a-ghostty-pin-bump.md - with the backport in, the item still reads as an open freeze. Pls retitle it to the remaining work: drop the backport once a pin carries an equivalent fix, with the three CI cases as the re-test.

@bot-rogerthat
bot-rogerthat marked this pull request as draft October 26, 2026 20:31
@bot-rogerthat
bot-rogerthat force-pushed the fix/surface-mailbox-shutdown branch from d7dbcfe to 78dce42 Compare October 26, 2026 20:31
@bot-rogerthat
bot-rogerthat force-pushed the fix/surface-mailbox-shutdown branch from 78dce42 to 63bd2da Compare October 26, 2026 21:06
@bot-rogerthat
bot-rogerthat marked this pull request as ready for review October 26, 2026 21:31

@umputun umputun left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@umputun
umputun merged commit fbb03e5 into umputun:master Oct 27, 2026
7 checks passed
umputun added a commit that referenced this pull request Oct 27, 2026
The symlink and build-stamp lines extended in #819 ran to about 120
columns and left a stray fragment, against the file's semantic-line
width. Rewrapped with no wording change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants