fix(terminal): backport responsive teardown to 0.64.22 - #1
Conversation
Backport the bounded, surface-scoped regression from upstream cmux PR manaflow-ai#9358. Fails on v0.64.22 before the production fix.
(cherry picked from commit 77ab34b)
vadimzak
left a comment
There was a problem hiding this comment.
Review — full · head 00f2a1d
claude-7db4eeaa-f8ab-413f-a876-2b20b3fedd5e@vz-kooply-mac-3 · claude / claude-fable-5-1 · effort high
Scope inspected: head 00f2a1d1ce vs base ddd4a01bc5 (stable/0.64.22). Production delta is 9+/8− in TerminalSurface+RuntimeLifecycle.swift; the rest is test-only (Swift test + C stub gate).
Production change (verified sound):
teardownSurface()no longer runsghostty_surface_freeinsideTask { @MainActor in … }. It now hands the pointer plus callback/manual-IO/tee userdata toruntimeTeardown.enqueueRuntimeTeardown(...), identical to the existingdeinitpath inTerminalSurface.swift:714and to upstream commit77ab34b3(manaflow-ai#9358, merged).- Coordinator at this base (
TerminalSurfaceRuntimeTeardownCoordinator.swift:163-229) frees on aTask.detached(priority: .utility)worker and releases userdata on the main actor only after the free returns. Prior release-ordering property is preserved. - Both production callers (
Sources/Workspace.swift:8316respawn,Sources/Panels/TerminalPanel.swift:674close) have no dependency on the free landing on the next main-actor turn; the old surface is already unregistered before the call. - Remaining direct
ghostty_surface_freecalls inTerminalSurface+Debug.swiftare...ForTestingmethods, not production. - Upstream's second commit (two-slot bounded close pool + lane rename + one comment in
GhosttyTerminalView.swift) is deliberately not backported; PR body names this as a non-goal. The captured deadlock cycle (main joins IO → IO joins reader → reader waits on main mailbox) is broken by moving off main alone, so this is a scope choice, not an absence.
Tests:
teardownSurfaceKeepsMainActorResponsiveWhileNativeFreeIsBlockedexercises the real production path (noruntimeSurfaceFreeOverrideForTesting, which pre-fix already went through the coordinator and therefore could not catch the bug). Pre-fix: the gated stub blocks the main actor, the probeTask { @MainActor }cannot run until the 5s gate timeout expires, at which pointblocking_is_active()is false → assertion fails. Post-fix: passes. The C gate is therefore necessary; an override-based test would not fail on the base.- Gate pointer
0x7541is unique to this.serializedsuite (other suites use0x7540), so no cross-suite interception. - Ran
swift test --package-path Packages/macOS/CmuxTerminal --filter 'TerminalSurfaceTeardownCallbackLifetimeTests|TerminalSurfaceRuntimeTeardownCoordinatorTests'at head: 16/16 passed.
Findings
SIMPLIFY
- F1 ·
Packages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/TerminalSurfaceTeardownCallbackLifetimeTests.swift:31·surfaceFreeGateDoesNotInterceptAnotherRuntimeSurfaceasserts the C test stub's own pointer scoping (arm gate for0x7541, free0x7542, checkdid_startis false). It exercises fixture representation, not cmux behaviour, and can only fail if the stub itself regresses. Smallest fix: delete the test, and with itcmux_test_ghostty_surface_free_blocking_did_startinGhosttyRuntimeTestStubs.c:87and.h:98, which then have no consumer.
No BLOCKER, no other IMPORTANT, no SCOPE, no DOCS findings.
RESULT=IMPORTANT n=1
vadimzak
left a comment
There was a problem hiding this comment.
Review — disposition · head 3d7b7d4
claude-1767e186-786b-4d6d-a99f-6da2b444c7d8@vz-kooply-mac-3 · claude / claude-fable-5-1 · effort high
Disposition review — head 3d7b7d44cf (prior 00f2a1d1ce, base ddd4a01bc5)
Delta inspected: 00f2a1d1ce..3d7b7d44cf is one commit, 25 deletions, 0 insertions, across three test-only files. No production files changed since the prior head. Full PR delta vs base is unchanged in production: TerminalSurface+RuntimeLifecycle.swift 9+/8−.
F1 · HOLDS. The delta removes exactly what the finding named:
TerminalSurfaceTeardownCallbackLifetimeTests.swift:31-46(prior numbering):surfaceFreeGateDoesNotInterceptAnotherRuntimeSurfacedeleted in full.GhosttyRuntimeTestStubs.c:87-92:cmux_test_ghostty_surface_free_blocking_did_startimplementation deleted.include/GhosttyRuntimeTestStubs.h:97: matching declaration deleted.
Repo-wide grep for blocking_did_start and the test name returns nothing outside the ghostty submodule, so no dangling consumer. The remaining gate primitives (blocking_begin, wait_until_started, blocking_is_active, release, blocking_reset) are still used by teardownSurfaceKeepsMainActorResponsiveWhileNativeFreeIsBlocked, the test that carries the actual regression coverage.
Verification: ran the two teardown suites at head in Packages/macOS/CmuxTerminal:
✔ Test run with 15 tests in 2 suites passed after 0.058 seconds.
The regression test teardownSurfaceKeepsMainActorResponsiveWhileNativeFreeIsBlocked passes and is intact.
New hunks since prior head: deletion-only; no new logic, state, type, module, or surface introduced. No BLOCKER or IMPORTANT introduced. Not SCOPE-EXPANDED.
RESULT=FIXES-VERIFIED
Terminal close on cmux 0.64.22 keeps the main thread available while native workers shut down.
Why: M deadlocked: main joined I/O, I/O joined its reader, and the reader waited for the full main-thread mailbox.
Risk: med — native cleanup moves off main; callback userdata remains retained until it finishes.
Needs you: Release signing setup before installing this backport.
Unverified: Release packaging is blocked: neither P nor M has a code-signing identity.
Changed
Checks
Review contract
Request: "do it" — approved stable backport, isolated validation, then M/P rollout.
Non-goals / Removed: blocking native free on main; unrelated upstream changes excluded.
Depends on: unchanged pinned GhosttyKit 6143bac; existing teardown coordinator.
Surface: test-only C gate functions; no production API/config changes.
Size: code +9/−8 · tests +130/−0
Rollout
Certainty
Problem 98 · Plan 95 · Impl 98 — fixes the captured cycle; Release signing remains outstanding.
Session
codex-2b59230d-7f2b-4db8-821d-447a05c87abc@vz-kooply-mac-3