fix: stale notification blocks + unified surface teardown (mechanism 2)

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,22 +990,25 @@ 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(
             forName: NSWindow.didMiniaturizeNotification,
             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)

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")
     }
 }
 

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")