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..