From e56d8c1a1cd1e6ad33098e190c0937c6f52907f4 Mon Sep 17 00:00:00 2001 From: arzafran Date: Tue, 11 Aug 2026 17:27:32 -0300 Subject: [PATCH] fix(escrow): release a session when the user closes it for good Closing a tab or a workspace left its shell running. The escrow holder keeps a dup of the pty master, so freeing the app-side surface never hangs up the terminal: the shell, and whatever agent was running in it, survived with nothing referencing it. Measured live: three orphaned Claude Code sessions, one of them 24 hours old, still polling the app's socket and being refused because they no longer belong to any window. The protocol had no way to say this. Its three frame types cover handing a session over, asking for it back, and the reply -- nothing means "this one is finished". `unclaimedSessionTTL` does not cover it either: that only starts once a session is draining, which happens when the owning app dies. A session closed while the app keeps running was bounded by nothing at all. Adds frame 0x05, sent from the app when a surface tears down for good; the holder authenticates it by token, closes its pty master, and drops the session, so the shell finally sees SIGHUP. Gated on two conditions, because getting this wrong destroys live work: - only from `teardownSurface()`, which is the path `ClosedTerminalUndoStore` runs on `finalize` -- so the close-undo grace period has already elapsed and nothing can restore the surface. `deinit` is excluded. - never while the app is terminating. Sessions open at quit must stay escrowed; releasing them there would kill every running agent on every update, which is the exact regression escrow exists to prevent. The second condition needed a flag the session machinery can read, so `AppDelegate`'s private termination state is mirrored onto `SessionMachineryGate` and reset in lockstep when a quit is cancelled. Holders predating this frame land in `serve`'s `default` branch, which logs and skips, degrading to the previous behavior. --- CHANGELOG.md | 1 + Sources/AppDelegate.swift | 6 + Sources/SessionEscrow.swift | 119 +++++++++++++++++++- Sources/SessionMachineryGate.swift | 12 ++ Sources/TerminalSurface.swift | 49 +++++++- programaTests/SessionPersistenceTests.swift | 55 +++++++++ 6 files changed, 240 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c950506..d2d15e3b 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/Sources/AppDelegate.swift b/Sources/AppDelegate.swift index c4f6ef8c..b5473207 100644 --- a/Sources/AppDelegate.swift +++ b/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. diff --git a/Sources/SessionEscrow.swift b/Sources/SessionEscrow.swift index 86979e94..96e748f0 100644 --- a/Sources/SessionEscrow.swift +++ b/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 diff --git a/Sources/SessionMachineryGate.swift b/Sources/SessionMachineryGate.swift index 09532ee6..5e33390e 100644 --- a/Sources/SessionMachineryGate.swift +++ b/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 } diff --git a/Sources/TerminalSurface.swift b/Sources/TerminalSurface.swift index e130e8f8..fe59f45c 100644 --- a/Sources/TerminalSurface.swift +++ b/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 } } } diff --git a/programaTests/SessionPersistenceTests.swift b/programaTests/SessionPersistenceTests.swift index 7ac279bd..13f33176 100644 --- a/programaTests/SessionPersistenceTests.swift +++ b/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..