fix(escrow): release a session when the user closes it for good

CHANGELOG.md

@@ -7,6 +7,7 @@ Programa is a fork of [cmux](https://github.com/manaflow-ai/cmux); for history p
 ## [Unreleased]
 
 ### Fixed
+- Closing a terminal tab or a workspace now actually ends the session. Anything running in it, an agent included, was being kept alive in the background after the tab disappeared: invisible, still using memory, and unable to talk back to the app. Quitting still preserves your sessions so they come back on the next launch.
 - The app no longer freezes on the first launch after an update while it is restoring your terminals. Restoring a session with a long transcript could wedge the whole app: no window, no input, and force-quitting was the only way out, which lost every session you had open. Long transcripts also come back more smoothly now, instead of stalling the window until they finish.
 - `programa` commands typed inside a restored terminal work again after an app update. They were being refused with "Access denied", which silently cut off any agent running in that pane until you opened a fresh one.
 - Opening a second window no longer blanks the terminals in your existing window until you resize it; clicking back into the window now redraws it immediately.

Sources/AppDelegate.swift

@@ -1469,6 +1469,7 @@ final class AppDelegate: NSObject, NSApplicationDelegate, @preconcurrency UNUser
 
     func applicationShouldTerminate(_ sender: NSApplication) -> NSApplication.TerminateReply {
         isTerminatingApp = true
+        SessionMachineryGate.isApplicationTerminating = true
         _ = saveSessionSnapshot(includeScrollback: true, removeWhenEmpty: false, cleanShutdown: true)
 
         // Tagged DEV builds are ephemeral, skip quit confirmation entirely.
@@ -1511,6 +1512,10 @@ final class AppDelegate: NSObject, NSApplicationDelegate, @preconcurrency UNUser
             } else {
                 // Reset so that the next quit attempt can show the dialog again.
                 self.isTerminatingApp = false
+                // Must be reset in lockstep, or a cancelled quit would leave
+                // the session machinery treating every later close as
+                // termination and never releasing escrowed sessions.
+                SessionMachineryGate.isApplicationTerminating = false
             }
             NSApp.reply(toApplicationShouldTerminate: shouldQuit)
         }
@@ -1519,6 +1524,7 @@ final class AppDelegate: NSObject, NSApplicationDelegate, @preconcurrency UNUser
 
     func applicationWillTerminate(_ notification: Notification) {
         isTerminatingApp = true
+        SessionMachineryGate.isApplicationTerminating = true
         _ = saveSessionSnapshot(includeScrollback: true, removeWhenEmpty: false, cleanShutdown: true)
         // Finalize any terminal closes still sitting in their undo grace period so a staged close
         // doesn't quietly leak instead of tearing down cleanly on quit.

Sources/SessionEscrow.swift

@@ -260,6 +260,24 @@ enum EscrowWireFormat {
     /// otherwise by this type). Carries the live master fd as `SCM_RIGHTS`
     /// ancillary data iff granted.
     static let retrieveResponseType: UInt8 = 0x04
+    /// App -> holder: this session was closed by the user for real, so drop
+    /// it instead of holding its pty master open. Session id + token, no
+    /// ancillary fd, no response.
+    ///
+    /// Without this the holder has no way to learn that a session ended: it
+    /// holds a dup of the pty master, so freeing the app-side surface never
+    /// hangs up the terminal and the shell (with whatever agent is running
+    /// in it) survives. A session closed while the app keeps running is not
+    /// even covered by `unclaimedSessionTTL`, which only applies once a
+    /// session is draining -- so it would be held until the holder exits.
+    ///
+    /// Only sent past the close-undo grace period (see
+    /// `ClosedTerminalUndoStore`), never on app quit: quitting must keep
+    /// escrowing so the next launch can reattach.
+    ///
+    /// Holders that predate this frame fall into `serve`'s `default` branch,
+    /// which logs and skips, degrading to the previous behavior.
+    static let releaseType: UInt8 = 0x05
 
     /// Why the holder denied a retrieve. Carried in the FIRST byte of the
     /// response frame's otherwise-unused 32-byte token padding
@@ -320,6 +338,16 @@ enum EscrowWireFormat {
         return data
     }
 
+    static func encodeReleaseFrame(sessionId: String, token: [UInt8]) -> Data? {
+        guard sessionId.utf8.count == sessionIdSize, token.count == tokenSize else { return nil }
+        var data = Data(capacity: frameSize)
+        data.append(releaseType)
+        data.append(contentsOf: Array(sessionId.utf8))
+        data.append(contentsOf: token)
+        data.append(Data(count: childPIDSize)) // unused padding for this type
+        return data
+    }
+
     static func encodeRetrieveResponseFrame(
         sessionId: String,
         granted: Bool,
@@ -341,7 +369,7 @@ enum EscrowWireFormat {
         let bytes = [UInt8](data)
         let type = bytes[0]
         switch type {
-        case escrowType, retrieveRequestType:
+        case escrowType, retrieveRequestType, releaseType:
             var offset = 1
             let sessionIdBytes = Array(bytes[offset..<(offset + sessionIdSize)])
             offset += sessionIdSize
@@ -640,6 +668,49 @@ final class SessionEscrowClient {
         }
     }
 
+    /// Tells the holder a session is genuinely closed so it stops holding the
+    /// pty master open. Fire-and-forget: the holder sends no response, and a
+    /// failure here is not worth surfacing -- the worst case is the previous
+    /// behavior, where the session lingers until the holder drops it.
+    ///
+    /// Callers must only use this once the close is final (past the undo
+    /// grace period) and never during app termination.
+    func release(surfaceId: String, tokenHex: String) {
+        guard !SessionMachineryGate.isUnitTesting else { return }
+        queue.async { [weak self] in
+            guard let self else { return }
+            // Nothing to release if we never escrowed it, and dropping the id
+            // here keeps a later re-escrow of the same surface id working.
+            guard self.escrowedSurfaceIds.contains(surfaceId) else { return }
+            guard let token = Self.tokenBytes(fromHex: tokenHex),
+                  let frame = EscrowWireFormat.encodeReleaseFrame(sessionId: surfaceId, token: token) else {
+                return
+            }
+            // Deliberately does NOT open a connection: if we have none, the
+            // holder either never got this session or is already gone.
+            guard let fd = self.connectionFD else { return }
+            if UnixDomainFDPassing.send(fd: nil, payload: frame, over: fd) {
+                self.escrowedSurfaceIds.remove(surfaceId)
+            } else {
+                self.teardownConnection()
+            }
+        }
+    }
+
+    private static func tokenBytes(fromHex hex: String) -> [UInt8]? {
+        let characters = Array(hex)
+        guard characters.count == EscrowWireFormat.tokenSize * 2 else { return nil }
+        var bytes = [UInt8]()
+        bytes.reserveCapacity(EscrowWireFormat.tokenSize)
+        var index = 0
+        while index < characters.count {
+            guard let byte = UInt8(String(characters[index...(index + 1)]), radix: 16) else { return nil }
+            bytes.append(byte)
+            index += 2
+        }
+        return bytes
+    }
+
     /// `queue`-confined. Returns the existing connection if live, otherwise
     /// attempts to connect, spawning the holder once (on the first failed
     /// attempt only) if nothing answers. Bounded by
@@ -1344,6 +1415,16 @@ enum SessionEscrowHolder {
                     if let fd { close(fd) }
                     guard let sessionId = decoded.sessionId, let token = decoded.token else { continue }
                     handleRetrieveRequest(connectionFD: connectionFD, sessionId: sessionId, token: token)
+                case EscrowWireFormat.releaseType:
+                    // Like a retrieve request, this never carries an fd.
+                    if let fd { close(fd) }
+                    guard let sessionId = decoded.sessionId, let token = decoded.token else { continue }
+                    if releaseSession(sessionId: sessionId, token: token) {
+                        // Drop it from this connection's set too, so the
+                        // connection's death does not later try to drain a
+                        // session that is already gone.
+                        registeredSessionIds.remove(sessionId)
+                    }
                 default:
                     if let fd {
                         // Stray/unexpected ancillary fd on a frame type
@@ -1387,6 +1468,42 @@ enum SessionEscrowHolder {
         registryLock.unlock()
     }
 
+    /// Drops a session the app has told us is genuinely closed, closing the
+    /// pty master we hold so the shell finally sees SIGHUP.
+    ///
+    /// Refuses a session that is already draining: draining means the owning
+    /// app died and a successor may still retrieve this session, which is
+    /// exactly the case escrow exists for. A release only ever arrives from
+    /// a live app over its own connection, so that combination should not
+    /// occur -- declining is the conservative branch either way, since the
+    /// TTL still bounds a draining session.
+    ///
+    /// Returns whether the session was actually dropped.
+    @discardableResult
+    private static func releaseSession(sessionId: String, token: [UInt8]) -> Bool {
+        registryLock.lock()
+        guard let session = registry[sessionId] else {
+            registryLock.unlock()
+            dilog("escrow.release", "session=\(sessionId.prefix(8)) outcome=unknown_session")
+            return false
+        }
+        guard constantTimeTokensEqual(session.token, token) else {
+            registryLock.unlock()
+            dilog("escrow.release", "session=\(sessionId.prefix(8)) outcome=token_mismatch")
+            return false
+        }
+        guard !session.isDraining else {
+            registryLock.unlock()
+            dilog("escrow.release", "session=\(sessionId.prefix(8)) outcome=declined_draining")
+            return false
+        }
+        registry.removeValue(forKey: sessionId)
+        registryLock.unlock()
+        session.markClosedIfNeeded()
+        dilog("escrow.release", "session=\(sessionId.prefix(8)) outcome=released")
+        return true
+    }
+
     /// Starts one drain thread per id that's registered and not already
     /// draining. Called once a connection's read loop ends (see `serve`);
     /// a no-op for any id already being drained by a prior death on a

Sources/SessionMachineryGate.swift

@@ -14,4 +14,16 @@ enum SessionMachineryGate {
         if env["DYLD_INSERT_LIBRARIES"]?.contains("libXCTest") == true { return true }
         return false
     }()
+
+    /// True once the app has begun terminating. Mirrors `AppDelegate`'s
+    /// private `isTerminatingApp` for the session machinery, which has to
+    /// tell a surface closed by the user apart from one that merely stopped
+    /// existing because the process is going away.
+    ///
+    /// The distinction matters to escrow: a user-closed session must be
+    /// released so the holder stops keeping its pty master open, while a
+    /// session that is open at quit must stay escrowed so the next launch can
+    /// reattach it. Set on the main thread during termination and read from
+    /// surface teardown, which also runs on main.
+    nonisolated(unsafe) static var isApplicationTerminating = false
 }

Sources/TerminalSurface.swift

@@ -224,6 +224,11 @@ final class TerminalSurface: Identifiable, ObservableObject {
     /// synchronously (main actor) the moment the attempt starts, before the
     /// async send even begins.
     private var hasAttemptedSessionEscrow = false
+
+    /// Token the holder issued for this surface's escrowed session, kept so a
+    /// genuine close can authenticate its release frame. Nil until escrow
+    /// succeeds, and for surfaces that were never escrowed.
+    private var escrowTokenHex: String?
     /// Issue #182 slice 2: set from `init`, consumed (cleared) the moment
     /// `createSurface` copies it into `surfaceConfig` -- see
     /// `TerminalSurfaceReviveDescriptor`'s doc comment.
@@ -893,6 +898,7 @@ final class TerminalSurface: Identifiable, ObservableObject {
             TerminalController.unregisterRevivedRoot(authorizedRoot)
             authorizedRevivedRootPID = nil
         }
+        releaseEscrowedSessionIfClosedForGood(reason: reason)
         markPortalLifecycleClosed(reason: reason)
 
         let callbackContext = surfaceCallbackContext
@@ -1734,6 +1740,44 @@ final class TerminalSurface: Identifiable, ObservableObject {
     /// ghostty-owned fd itself: `ghostty_surface_pty_master_fd` does not
     /// dup or transfer ownership, so `dup()` here is required before the
     /// fd can safely outlive this surface.
+    /// Tells the escrow holder to drop this session, but only when the
+    /// surface is going away because the user closed it for good.
+    ///
+    /// The holder keeps a dup of the pty master, so freeing the app-side
+    /// surface is not enough to hang up the terminal: without this the shell
+    /// and whatever agent is running in it stay alive, invisible to the app,
+    /// until the holder exits. A session closed while the app keeps running
+    /// is not even covered by `unclaimedSessionTTL`, which only starts once a
+    /// session is draining.
+    ///
+    /// Two conditions, both required:
+    ///
+    /// - `reason == "teardown"`, i.e. `teardownSurface()`. That is the path
+    ///   `ClosedTerminalUndoStore`'s `finalize` runs, so the close-undo grace
+    ///   period has already elapsed and nothing can restore this surface.
+    ///   `deinit` is deliberately excluded: a surface can be deallocated for
+    ///   reasons that are not a user-visible close.
+    /// - the app is not terminating. Sessions open at quit MUST stay
+    ///   escrowed -- reattaching them on the next launch is the entire point
+    ///   of escrow, and releasing here would silently kill every running
+    ///   agent on every app update.
+    nonisolated static func shouldReleaseEscrowOnTeardown(
+        reason: String,
+        isApplicationTerminating: Bool
+    ) -> Bool {
+        reason == "teardown" && !isApplicationTerminating
+    }
+
+    private func releaseEscrowedSessionIfClosedForGood(reason: String) {
+        guard Self.shouldReleaseEscrowOnTeardown(
+            reason: reason,
+            isApplicationTerminating: SessionMachineryGate.isApplicationTerminating
+        ) else { return }
+        guard let tokenHex = escrowTokenHex else { return }
+        escrowTokenHex = nil
+        SessionEscrowClient.shared.release(surfaceId: id.uuidString, tokenHex: tokenHex)
+    }
+
     private func attemptSessionEscrow(surface: ghostty_surface_t, surfaceId: String, childPID: Int32) {
         guard !SessionMachineryGate.isUnitTesting else { return }
         hasAttemptedSessionEscrow = true
@@ -1745,13 +1789,16 @@ final class TerminalSurface: Identifiable, ObservableObject {
             surfaceId: surfaceId,
             dupedMasterFD: dupedFD,
             childPID: childPID
-        ) { result in
+        ) { [weak self] result in
             guard let result else { return }
             SessionWALStore.shared.markEscrowed(
                 surfaceId: surfaceId,
                 socketPath: result.socketPath,
                 token: result.tokenHex
             )
+            // Kept in memory so a genuine close can authenticate the release
+            // frame without going back to the WAL for the token.
+            DispatchQueue.main.async { self?.escrowTokenHex = result.tokenHex }
         }
     }
 

programaTests/SessionPersistenceTests.swift

@@ -2145,4 +2145,59 @@ final class SessionEscrowReattachRegressionTests: XCTestCase {
             "the revive-success path consumed the claim (holder registry entry is gone) and must be able to clean up despite live-looking escrow fields"
         )
     }
+
+    // MARK: - Escrow release on genuine close
+
+    func testReleaseFrameRoundTripsThroughDecode() throws {
+        let sessionId = UUID().uuidString
+        let token = (0..<EscrowWireFormat.tokenSize).map { UInt8($0 % 251) }
+
+        let frame = try XCTUnwrap(
+            EscrowWireFormat.encodeReleaseFrame(sessionId: sessionId, token: token)
+        )
+        XCTAssertEqual(
+            frame.count,
+            EscrowWireFormat.frameSize,
+            "release must use the same fixed frame size as every other type, or the read side desynchronizes"
+        )
+
+        let decoded = try XCTUnwrap(EscrowWireFormat.decode(frame))
+        XCTAssertEqual(decoded.type, EscrowWireFormat.releaseType)
+        XCTAssertEqual(decoded.sessionId, sessionId)
+        XCTAssertEqual(decoded.token, token, "the holder authenticates a release by token; it must survive the round trip")
+        XCTAssertNil(decoded.childPID, "release carries no child pid")
+    }
+
+    func testReleaseFrameRejectsMalformedInput() {
+        let token = [UInt8](repeating: 0x11, count: EscrowWireFormat.tokenSize)
+        XCTAssertNil(
+            EscrowWireFormat.encodeReleaseFrame(sessionId: "too-short", token: token),
+            "a session id that is not a UUID string would shift every later field"
+        )
+        XCTAssertNil(
+            EscrowWireFormat.encodeReleaseFrame(sessionId: UUID().uuidString, token: [0x01, 0x02]),
+            "a short token would leave the frame undersized"
+        )
+    }
+
+    /// A session open when the app quits MUST stay escrowed — reattaching it
+    /// on the next launch is what escrow is for, and releasing on quit would
+    /// kill every running agent on every app update.
+    func testEscrowIsReleasedOnlyForAUserCloseWhileTheAppIsRunning() {
+        XCTAssertTrue(
+            TerminalSurface.shouldReleaseEscrowOnTeardown(reason: "teardown", isApplicationTerminating: false),
+            "a finalized user close is the one case that must release"
+        )
+        XCTAssertFalse(
+            TerminalSurface.shouldReleaseEscrowOnTeardown(reason: "teardown", isApplicationTerminating: true),
+            "quitting must leave sessions escrowed for the next launch to reattach"
+        )
+        XCTAssertFalse(
+            TerminalSurface.shouldReleaseEscrowOnTeardown(reason: "deinit", isApplicationTerminating: false),
+            "deallocation is not necessarily a user-visible close"
+        )
+        XCTAssertFalse(
+            TerminalSurface.shouldReleaseEscrowOnTeardown(reason: "deinit", isApplicationTerminating: true)
+        )
+    }
 }