From df031670bee6f15f6953028f5fb3da3e60671156 Mon Sep 17 00:00:00 2001 From: arzafran Date: Wed, 5 Aug 2026 11:07:57 -0300 Subject: [PATCH 1/3] fix: guard windowObservers against stale-generation teardown race NotificationCenter.removeObserver does not cancel a block already handed off to the main OperationQueue. During rapid window create/close, viewDidMoveToWindow can tear down and re-register windowObservers for a new window while a block registered against the previous window is still sitting in the main queue; when it runs, it now no-ops instead of acting on stale state. Each addObserver(queue: .main) block in viewDidMoveToWindow now captures the generation live at its own registration and checks it against the instance's current generation before doing anything. Live (current) blocks always match, so #241 occlusion-heal semantics on didBecomeKey / didChangeOcclusionState are unaffected. --- Sources/GhosttySurfaceScrollView.swift | 29 +++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/Sources/GhosttySurfaceScrollView.swift b/Sources/GhosttySurfaceScrollView.swift index 11b048f4..37cbdb23 100644 --- a/Sources/GhosttySurfaceScrollView.swift +++ b/Sources/GhosttySurfaceScrollView.swift @@ -151,6 +151,18 @@ final class GhosttySurfaceScrollView: NSView { #endif private var observers: [NSObjectProtocol] = [] private var windowObservers: [NSObjectProtocol] = [] + // Teardown-race guard (forensic evidence: 6/7 CI minidumps traced to a + // NotificationCenter.addObserver(queue: .main) block reached via + // _dispatch_call_block_and_release touching reused memory). NotificationCenter's + // removeObserver does NOT cancel a block already handed off to the main + // OperationQueue — during rapid window create/close, a block enqueued against + // the *previous* window/registration batch can still run after + // viewDidMoveToWindow has already torn down and re-registered observers for a + // new window. Every block captures the generation live at its own registration + // and no-ops if the instance has since moved on to a newer generation. Live + // (current) blocks always match, so #241 occlusion-heal semantics on + // didBecomeKey/didChangeOcclusionState are unaffected. + private var windowObserverGeneration: UInt64 = 0 private var isLiveScrolling = false private var lastSentRow: Int? /// Tracks whether the user has scrolled away from the bottom to review scrollback. @@ -696,6 +708,7 @@ final class GhosttySurfaceScrollView: NSView { #endif observers.forEach { NotificationCenter.default.removeObserver($0) } windowObservers.forEach { NotificationCenter.default.removeObserver($0) } + windowObserverGeneration &+= 1 deferredSearchOverlayMutationWorkItem?.cancel() imageTransferIndicatorShowWorkItem?.cancel() dropZoneOverlayView.removeFromSuperview() @@ -925,13 +938,15 @@ final class GhosttySurfaceScrollView: NSView { super.viewDidMoveToWindow() windowObservers.forEach { NotificationCenter.default.removeObserver($0) } windowObservers.removeAll() + windowObserverGeneration &+= 1 + let observerGeneration = windowObserverGeneration guard let window else { return } windowObservers.append(NotificationCenter.default.addObserver( forName: NSWindow.didBecomeKeyNotification, object: window, queue: .main ) { [weak self] _ in - guard let self else { return } + guard let self, self.windowObserverGeneration == observerGeneration else { return } let searchActive = self.surfaceView.terminalSurface?.searchState != nil #if DEBUG dlog("find.window.didBecomeKey surface=\(self.surfaceView.terminalSurface?.id.uuidString.prefix(5) ?? "nil") searchActive=\(searchActive) focusTarget=\(self.searchFocusTarget) firstResponder=\(String(describing: self.window?.firstResponder))") @@ -950,7 +965,8 @@ final class GhosttySurfaceScrollView: NSView { object: window, queue: .main ) { [weak self] _ in - guard let self, let window = self.window else { return } + guard let self, self.windowObserverGeneration == observerGeneration, + let window = self.window else { return } let searchActive = self.surfaceView.terminalSurface?.searchState != nil // Losing key window does not always trigger first-responder resignation, so force // the focused terminal view to yield responder to keep Ghostty cursor/focus state in sync. @@ -974,7 +990,8 @@ final class GhosttySurfaceScrollView: NSView { object: window, queue: .main ) { [weak self] _ in - guard let self, let window = self.window else { return } + guard let self, self.windowObserverGeneration == observerGeneration, + let window = self.window else { return } self.updateWindowVisibility(window.occlusionState.contains(.visible) || window.isKeyWindow) }) windowObservers.append(NotificationCenter.default.addObserver( @@ -982,14 +999,16 @@ final class GhosttySurfaceScrollView: NSView { object: window, queue: .main ) { [weak self] _ in - self?.updateWindowVisibility(false) + guard let self, self.windowObserverGeneration == observerGeneration else { return } + self.updateWindowVisibility(false) }) windowObservers.append(NotificationCenter.default.addObserver( forName: NSWindow.didDeminiaturizeNotification, object: window, queue: .main ) { [weak self] _ in - guard let self, let window = self.window else { return } + guard let self, self.windowObserverGeneration == observerGeneration, + let window = self.window else { return } self.updateWindowVisibility(window.occlusionState.contains(.visible) || window.isKeyWindow) }) updateWindowVisibility((window.occlusionState.contains(.visible) || window.isKeyWindow) && !window.isMiniaturized) From 21ad659a8e99af877815b35dbf4b01dc4a33453d Mon Sep 17 00:00:00 2001 From: arzafran Date: Wed, 5 Aug 2026 11:08:11 -0300 Subject: [PATCH 2/3] refactor: unify teardownSurface/deinit into one teardown path (N12) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit teardownSurface() and deinit each had their own copy of the snapshot-callback-context / snapshot-tap-context / nil-surface / free-on-deferred-Task logic. Unify them into a single performSurfaceTeardown(reason:) that snapshots and nils every piece of handoff state a second call could act on (surface, surfaceCallbackContext, outputTapContext, reviveDescriptor), so calling it from either site can never double-free or double-release: a second call sees surface == nil and no-ops. Also extends the reviveDescriptor fd leak-guard (previously deinit-only) to teardownSurface(), now nil-ing it after close() so a repeat call is safe. liveSurfaceForGhosttyAccess(reason:), releaseSurfaceForTesting(), and replaceSurfaceWithFreedPointerForTesting() are intentionally not folded in — each has real semantic differences (documented inline) that would be lost by unification: not freeing a possibly-reowned pointer, keeping portalLifecycleState live for a recreate-after-release test fixture, and deliberately leaving a dangling surface pointer to simulate an out-of-band free. --- Sources/TerminalSurface.swift | 175 ++++++++++++++++------------------ 1 file changed, 84 insertions(+), 91 deletions(-) diff --git a/Sources/TerminalSurface.swift b/Sources/TerminalSurface.swift index c0e01568..694d6664 100644 --- a/Sources/TerminalSurface.swift +++ b/Sources/TerminalSurface.swift @@ -837,17 +837,58 @@ final class TerminalSurface: Identifiable, ObservableObject { #endif } - /// Explicitly free the Ghostty runtime surface. Idempotent — safe to call - /// before deinit; deinit will skip the free if already torn down. - @MainActor - func teardownSurface() { - recordTeardownRequest(reason: "surface.teardown") - markPortalLifecycleClosed(reason: "teardown") + /// Single teardown path (audit finding N12) for the runtime surface plus its + /// callback/tap handoff state, shared by the two *real* close paths — + /// `teardownSurface()` and `deinit` — which is where the forensic evidence + /// (#250-era teardown-race audit) points: both can independently decide to + /// tear the same surface down, and before this unification each had its own + /// copy of the snapshot-and-free logic. Snapshots and nils every piece of + /// state a second call could act on (surface, surfaceCallbackContext, + /// outputTapContext, reviveDescriptor), so calling this from either site can + /// never double-free or double-release: a second call sees `surface == nil` + /// and no-ops at the `guard`. + /// + /// Deliberately NOT folded in here (real semantic differences that resist + /// unification): + /// - `liveSurfaceForGhosttyAccess(reason:)` — the pointer there may already + /// be reowned/recycled by another surface, so it must NOT call + /// `ghostty_surface_free` or touch the C surface at all; it only forgets + /// Swift-side bookkeeping. + /// - `releaseSurfaceForTesting()` — deliberately does NOT call + /// `markPortalLifecycleClosed`, so `portalLifecycleState` stays `.live` + /// and a subsequent `requestBackgroundSurfaceStartIfNeeded()` / + /// `createSurface()` can still recreate the runtime surface. This is + /// load-bearing for the "detach race" regression tests in + /// `TerminalAndGhosttyTests.swift`, which release the surface and then + /// assert it gets recreated on next keystroke. + /// - `replaceSurfaceWithFreedPointerForTesting()` — deliberately leaves + /// `self.surface` non-nil after freeing, to simulate a stale Swift + /// wrapper whose native surface was already freed out-of-band. Folding + /// it into the nil-as-idempotency-guard shape here would erase the exact + /// dangling-pointer scenario that fixture exists to test. + /// + /// Keeps the deferred `Task { @MainActor in }` free timing (#432) exactly + /// as both prior call sites had it — this PR does not make the free + /// synchronous. + /// + /// Deliberately not `@MainActor`: `deinit` is always nonisolated in Swift's + /// concurrency model and cannot call a main-actor-isolated method + /// synchronously, so this stays a plain nonisolated method callable from + /// both `deinit` and the `@MainActor`-isolated `teardownSurface()`. + private func performSurfaceTeardown(reason: String) { + if let leftoverReviveDescriptor = reviveDescriptor { + close(leftoverReviveDescriptor.masterFD) + reviveDescriptor = nil + } + markPortalLifecycleClosed(reason: reason) let callbackContext = surfaceCallbackContext surfaceCallbackContext = nil let tapContext = outputTapContext outputTapContext = nil + // Snapshot before any possible async hop so a deferred Task below + // never needs to capture `self` for logging/unregistration. + let surfaceIdForTap = id.uuidString let surfaceToFree = surface if let surfaceToFree { @@ -856,6 +897,12 @@ final class TerminalSurface: Identifiable, ObservableObject { surface = nil guard let surfaceToFree else { +#if DEBUG + dlog( + "surface.lifecycle.\(reason).skip surface=\(surfaceIdForTap.prefix(5)) " + + "workspace=\(tabId.uuidString.prefix(5))" + ) +#endif callbackContext?.release() tapContext?.release() return @@ -870,19 +917,40 @@ final class TerminalSurface: Identifiable, ObservableObject { } #endif +#if DEBUG + dlog( + "surface.lifecycle.\(reason).begin surface=\(surfaceIdForTap.prefix(5)) " + + "workspace=\(tabId.uuidString.prefix(5))" + ) +#endif + Task { @MainActor in - // Keep free behavior aligned with deinit: perform the runtime teardown on - // the next main-actor turn so SIGHUP delivery is deterministic but non-reentrant. - // Clear the output tap right before free, per the C API contract. - // This is the surface's genuine normal-close path, so delete its - // WAL directory now that it's torn down. - SessionWALStore.shared.unregister(surface: surfaceToFree, surfaceId: id.uuidString, deleteDirectory: true) + // Keep free behavior aligned across teardown sites: perform the runtime + // teardown on the next main-actor turn so SIGHUP delivery is + // deterministic but non-reentrant. Clear the output tap right before + // free, per the C API contract. Both call sites are the surface's + // genuine normal-close path, so delete its WAL directory now that + // it's torn down. + SessionWALStore.shared.unregister(surface: surfaceToFree, surfaceId: surfaceIdForTap, deleteDirectory: true) ghostty_surface_free(surfaceToFree) callbackContext?.release() tapContext?.release() +#if DEBUG + dlog( + "surface.lifecycle.\(reason).end surface=\(surfaceIdForTap.prefix(5)) freed=1" + ) +#endif } } + /// Explicitly free the Ghostty runtime surface. Idempotent — safe to call + /// before deinit; deinit will skip the free if already torn down. + @MainActor + func teardownSurface() { + recordTeardownRequest(reason: "surface.teardown") + performSurfaceTeardown(reason: "teardown") + } + #if DEBUG private static let surfaceLogPath = "/tmp/programa-ghostty-surface.log" private static let sizeLogPath = "/tmp/programa-ghostty-size.log" @@ -2346,87 +2414,12 @@ final class TerminalSurface: Identifiable, ObservableObject { #endif deinit { - // Issue #182 slice 2 safety net: if this surface's revive - // descriptor was never consumed by `createSurface` (e.g. the - // owning `newTerminalSurface` call's own tab/panel bookkeeping - // failed before the hosted view ever attached to a window and - // triggered surface creation), its fd would otherwise leak - // silently -- close it here. A no-op whenever `createSurface` did - // run: it always clears `reviveDescriptor` to nil the moment it - // reads it, whether creation itself then succeeded or failed. - if let leftoverReviveDescriptor = reviveDescriptor { - close(leftoverReviveDescriptor.masterFD) - } - markPortalLifecycleClosed(reason: "deinit") - - let callbackContext = surfaceCallbackContext - surfaceCallbackContext = nil - let tapContext = outputTapContext - outputTapContext = nil - // Deinit's Task closure below must not capture self, so snapshot the id - // string now for the WAL tap teardown call. - let surfaceIdForTap = id.uuidString - - // Nil out the surface pointer so any in-flight closures (e.g. geometry - // reconcile dispatched via DispatchQueue.main.async) that read self.surface - // before this object is fully deallocated will see nil and bail out, - // rather than passing a freed pointer to ghostty_surface_refresh (#432). - let surfaceToFree = surface - if let surfaceToFree { - TerminalSurfaceRegistry.shared.unregisterRuntimeSurface(surfaceToFree, ownerId: id) - } - surface = nil - - guard let surfaceToFree else { -#if DEBUG - dlog( - "surface.lifecycle.deinit.skip surface=\(id.uuidString.prefix(5)) " + - "workspace=\(tabId.uuidString.prefix(5)) reason=noRuntimeSurface" - ) -#endif - callbackContext?.release() - tapContext?.release() - return - } - -#if DEBUG - if runtimeSurfaceFreedOutOfBandForTesting { - runtimeSurfaceFreedOutOfBandForTesting = false - callbackContext?.release() - tapContext?.release() - return - } -#endif - -#if DEBUG - let surfaceToken = String(id.uuidString.prefix(5)) - let workspaceToken = String(tabId.uuidString.prefix(5)) - dlog( - "surface.lifecycle.deinit.begin surface=\(surfaceToken) " + - "workspace=\(workspaceToken) hasAttachedView=\(attachedView != nil ? 1 : 0) " + - "hostedInWindow=\(hostedView.window != nil ? 1 : 0)" - ) -#endif - // Keep teardown asynchronous to avoid re-entrant close/deinit loops, but retain // callback userdata until surface free completes so callbacks never dereference - // a deallocated view pointer. - Task { @MainActor in - // Clear the output tap right before free, per the C API contract. - // This is the surface's genuine normal-close path (when - // teardownSurface() didn't already run it), so delete its WAL - // directory now that it's torn down. - SessionWALStore.shared.unregister(surface: surfaceToFree, surfaceId: surfaceIdForTap, deleteDirectory: true) - ghostty_surface_free(surfaceToFree) - callbackContext?.release() - tapContext?.release() -#if DEBUG - dlog( - "surface.lifecycle.deinit.end surface=\(surfaceToken) " + - "workspace=\(workspaceToken) freed=1" - ) -#endif - } + // a deallocated view pointer. performSurfaceTeardown's revive-descriptor + // leak-guard covers issue #182 slice 2 (fd leak if `createSurface` never + // consumed `reviveDescriptor`) and is a no-op whenever `createSurface` did run. + performSurfaceTeardown(reason: "deinit") } } From 4216758e90a92900669430ac4f8e9e28fa59eea8 Mon Sep 17 00:00:00 2001 From: arzafran Date: Wed, 5 Aug 2026 11:08:21 -0300 Subject: [PATCH 3/3] test: extend rapid-window-teardown tripwire iterations 20 -> 28 Same rapid create/close shape covers the windowObserverGeneration guard in GhosttySurfaceScrollView (a second, distinct teardown-race mechanism from the callback-context race this test already targets). Making the exact NotificationCenter enqueue race deterministic would need a dedicated seam into GhosttySurfaceScrollView's private observer bookkeeping that doesn't exist yet, so per policy this bumps the existing tripwire's iteration count modestly instead of adding a new seam-dependent test. --- .../AppDelegateShortcutRoutingTests.swift | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/programaTests/AppDelegateShortcutRoutingTests.swift b/programaTests/AppDelegateShortcutRoutingTests.swift index 9114106f..99b1336e 100644 --- a/programaTests/AppDelegateShortcutRoutingTests.swift +++ b/programaTests/AppDelegateShortcutRoutingTests.swift @@ -4623,9 +4623,18 @@ final class AppDelegateShortcutRoutingTests: XCTestCase { /// (`PROGRAMA_FORCE_OCCLUDED=1`, honored by /// `GhosttySurfaceScrollView.applyEffectiveOcclusion()`) with a live shell in each /// window -- exactly the kind of concurrent IO-thread callback traffic vs. main-thread - /// teardown that used to race. Bounded to ~20 iterations: this is inherently - /// probabilistic, and a higher count would only buy marginal extra confidence at the - /// cost of runtime; it is not meant to chase local determinism. + /// teardown that used to race. This same rapid create/close shape is also the one the + /// `windowObserverGeneration` guard in `GhosttySurfaceScrollView.viewDidMoveToWindow` + /// targets (a second, distinct teardown-race mechanism: an + /// `NSNotificationCenter.addObserver(queue: .main)` block already handed off to the + /// main `OperationQueue` before `removeObserver` runs for it). Making that exact + /// enqueue race deterministic would require a dedicated seam into + /// `GhosttySurfaceScrollView`'s private observer bookkeeping that doesn't exist today, + /// so per the test-quality policy for cases where a targeted deterministic repro isn't + /// practical yet, this tripwire's iteration count is bumped modestly (20 -> 28) instead + /// of adding a new seam-dependent test. Bounded rather than large: this is inherently + /// probabilistic, and a much higher count would only buy marginal extra confidence at + /// the cost of runtime; it is not meant to chase local determinism. /// /// NOTE: against the CURRENT (reverted, pre-occluded-render) ghostty framework, the /// crash this guards against is masked by incidental renderer serialization, so this @@ -4646,7 +4655,7 @@ final class AppDelegateShortcutRoutingTests: XCTestCase { } } - for iteration in 0..<20 { + for iteration in 0..<28 { let windowId = appDelegate.createMainWindow() guard window(withId: windowId) != nil else { XCTFail("iteration \(iteration): expected test window")