fix(macos): resolve final quality review findings

Author: danielkov

.github/workflows/ci.yml

@@ -35,6 +35,7 @@ jobs:
       - name: Check generated Xcode project drift
         shell: bash
         run: |
+          PYTHONDONTWRITEBYTECODE=1 python3 -m unittest scripts/tests/test_generate_acp_swift.py
           scripts/generate-acp-swift.py --check
           PATH="$RUNNER_TEMP/bin:$PATH" scripts/generate-macos-project.sh
           git diff --exit-code -- macos/Config/Version.xcconfig macos/KitDesktop/Generated macos/KitDesktop.xcodeproj

fixtures/mock-acp-v2.py

@@ -135,28 +135,101 @@ final class AppModel: ObservableObject {
         guard allowPersistenceMutation() else { return }
         var merged = state.conversations
         var indexBySessionID: [String: Int] = [:]
+        var reservedIndexBySessionID: [String: Int] = [:]
         for (index, conversation) in merged.enumerated() where conversation.workspaceID == workspaceID {
-            if let sessionID = conversation.sessionID { indexBySessionID[sessionID] = index }
+            if let sessionID = conversation.sessionID, indexBySessionID[sessionID] == nil {
+                indexBySessionID[sessionID] = index
+            }
+            if let controller = controllers[conversation.id] {
+                reservedIndexBySessionID[controller.reservedSessionID] = index
+            }
         }
+
+        var preferredIndexBySessionID: [String: Int] = [:]
         for session in catalog {
-            if let index = indexBySessionID[session.sessionID] {
-                if let title = session.title { merged[index].title = title }
-                if let updatedAt = session.updatedAt { merged[index].updatedAt = updatedAt }
+            let index: Int
+            if let reserved = reservedIndexBySessionID[session.sessionID] {
+                index = reserved
+                merged[index].sessionID = session.sessionID
+            } else if let existing = indexBySessionID[session.sessionID] {
+                index = existing
             } else {
                 let timestamp = session.updatedAt ?? Date()
                 let conversation = Conversation(
                     workspaceID: workspaceID, title: session.title ?? "Kit session",
                     sessionID: session.sessionID, createdAt: timestamp, updatedAt: timestamp
                 )
-                indexBySessionID[session.sessionID] = merged.count
+                index = merged.count
                 merged.append(conversation)
             }
+            preferredIndexBySessionID[session.sessionID] = index
+            if let title = session.title { merged[index].title = title }
+            if let updatedAt = session.updatedAt { merged[index].updatedAt = updatedAt }
         }
+
+        let catalogIDs = Set(catalog.map(\.sessionID))
+        let preferredIDBySessionID = preferredIndexBySessionID.mapValues { merged[$0].id }
+        var removedIDs: Set<UUID> = []
+        var replacements: [UUID: UUID] = [:]
+        merged = merged.compactMap { conversation in
+            guard conversation.workspaceID == workspaceID, let sessionID = conversation.sessionID else {
+                return conversation
+            }
+            guard catalogIDs.contains(sessionID), let preferredID = preferredIDBySessionID[sessionID] else {
+                // A live controller is itself a recovery path and may briefly outrun an
+                // eventually consistent catalog. Only prune omitted dormant sessions.
+                guard controllers[conversation.id] == nil else { return conversation }
+                removedIDs.insert(conversation.id)
+                return nil
+            }
+            guard preferredID == conversation.id else {
+                removedIDs.insert(conversation.id)
+                replacements[conversation.id] = preferredID
+                return nil
+            }
+            return conversation
+        }
+        removeControllersAndRepairSelection(removedIDs, replacements: replacements)
         guard merged != state.conversations else { return }
         state.conversations = merged
         save()
     }
 
+    func sessionBecameReady(conversationID: UUID, sessionID: String) {
+        guard allowPersistenceMutation(),
+              let preferred = state.conversations.first(where: { $0.id == conversationID }) else { return }
+        let duplicates = state.conversations.filter {
+            $0.id != conversationID && $0.workspaceID == preferred.workspaceID && $0.sessionID == sessionID
+        }
+        let duplicateIDs = Set(duplicates.map(\.id))
+        let replacements = Dictionary(uniqueKeysWithValues: duplicates.map { ($0.id, conversationID) })
+        let catalogMetadata = duplicates.max { $0.updatedAt < $1.updatedAt }
+        state.conversations.removeAll { duplicateIDs.contains($0.id) }
+        removeControllersAndRepairSelection(duplicateIDs, replacements: replacements)
+        guard let index = state.conversations.firstIndex(where: { $0.id == conversationID }) else { return }
+        state.conversations[index].sessionID = sessionID
+        if let catalogMetadata {
+            state.conversations[index].title = catalogMetadata.title
+            state.conversations[index].createdAt = min(state.conversations[index].createdAt, catalogMetadata.createdAt)
+        }
+        state.conversations[index].updatedAt = Date()
+        save()
+    }
+
+    private func removeControllersAndRepairSelection(_ removedIDs: Set<UUID>, replacements: [UUID: UUID]) {
+        guard !removedIDs.isEmpty else { return }
+        for id in removedIDs {
+            controllers.removeValue(forKey: id)?.close()
+            activity.removeValue(forKey: id)
+        }
+        if let selectedConversationID, removedIDs.contains(selectedConversationID) {
+            self.selectedConversationID = replacements[selectedConversationID]
+        }
+        if let pendingConversationID, removedIDs.contains(pendingConversationID) {
+            self.pendingConversationID = replacements[pendingConversationID]
+        }
+    }
+
     private func refreshSessionCatalog(for workspaceID: UUID) {
         guard let catalogLoader, !persistenceIsReadOnly, !isClosing,
               let workspace = state.workspaces.first(where: { $0.id == workspaceID }) else { return }
@@ -188,7 +261,7 @@ final class AppModel: ObservableObject {
         let controller = controllerFactory(conversation, workspace.path)
         controller.onSessionReady = { [weak self] sessionID, _ in
             guard let self else { return }
-            self.updateConversation(id) { item in item.sessionID = sessionID; item.updatedAt = Date() }
+            self.sessionBecameReady(conversationID: id, sessionID: sessionID)
             if self.pendingConversationID == id || self.selectedConversationID == id {
                 self.pendingConversationID = nil
                 self.commitSelection(id)

fixtures/mock-acp.py

@@ -78,6 +78,7 @@ final class ConversationController: ObservableObject {
     private var composerTransaction: ComposerTransaction?
     private var shuttingDown = false
     private var retrying = false
+    private var clientGeneration: UInt64 = 0
     private var startupUpdates: [DesktopUpdate]?
     private var expectedRuntimeSessionID: String?
     private var rosterPruneWorkItem: DispatchWorkItem?
@@ -97,37 +98,40 @@ final class ConversationController: ObservableObject {
         self.client = client
     }
 
+    var reservedSessionID: String { conversation.sessionID ?? launchSessionID }
+
     func start() { start(force: false) }
 
     func retryIfNeeded() {
-        guard isRetryable, !retrying else { return }
+        guard isRetryable, !retrying, !shuttingDown else { return }
         retrying = true
         isRetryable = false
         status = "Reconnecting…"
-        let previous = client
-        previous.close(activeTurn: false) { [weak self] in
-            guard let self else { return }
-            self.client = previous.replacement()
-            self.retrying = false
-            self.start(force: false)
-        }
+        replaceClientAfterClosing(force: false)
     }
 
     private func start(force: Bool) {
+        guard !shuttingDown else { return }
         status = force ? "Recovering stale session lock…" : "Connecting…"
         let persisted = conversation.sessionID
+        let activeClient = client
+        let generation = clientGeneration
         startupUpdates = persisted == nil ? nil : []
-        client.onUpdate = { [weak self] update in
-            guard let self else { return }
+        activeClient.onUpdate = { [weak self, weak activeClient] update in
+            guard let self, let activeClient, self.isCurrentClient(activeClient, generation: generation), !self.shuttingDown else { return }
             if self.startupUpdates != nil { self.startupUpdates?.append(update) }
             else { self.apply(update) }
         }
-        client.onRuntimeEvent = { [weak self] event in self?.applyRuntime(event) }
-        client.onDiagnostic = { [weak self] text in
-            self?.recordDiagnostic(text, updateStatus: !text.hasPrefix("Ignored unsupported desktop update:") && !text.hasPrefix("Ignored malformed desktop update:"))
+        activeClient.onRuntimeEvent = { [weak self, weak activeClient] event in
+            guard let self, let activeClient, self.isCurrentClient(activeClient, generation: generation), !self.shuttingDown else { return }
+            self.applyRuntime(event)
         }
-        client.onExit = { [weak self] code in
-            guard let self else { return }
+        activeClient.onDiagnostic = { [weak self, weak activeClient] text in
+            guard let self, let activeClient, self.isCurrentClient(activeClient, generation: generation), !self.shuttingDown else { return }
+            self.recordDiagnostic(text, updateStatus: !text.hasPrefix("Ignored unsupported desktop update:") && !text.hasPrefix("Ignored malformed desktop update:"))
+        }
+        activeClient.onExit = { [weak self, weak activeClient] code in
+            guard let self, let activeClient, self.isCurrentClient(activeClient, generation: generation) else { return }
             self.reduceSettlement(.processExit(code))
             var roster = self.agentRoster
             roster.retireActive(at: Self.nowMilliseconds())
@@ -145,8 +149,8 @@ final class ConversationController: ObservableObject {
             reasoningEffort: inheritsConfig ? nil : conversation.reasoningEffort,
             force: force
         )
-        client.start(options: options, loading: persisted != nil) { [weak self] result in
-            guard let self else { return }
+        activeClient.start(options: options, loading: persisted != nil) { [weak self, weak activeClient] result in
+            guard let self, let activeClient, self.isCurrentClient(activeClient, generation: generation), !self.shuttingDown else { return }
             switch result {
             case .failure(let error):
                 self.startupUpdates = nil
@@ -732,17 +736,28 @@ final class ConversationController: ObservableObject {
     }
 
     private func retryAfterStaleLock() {
-        guard !retrying else { return }
+        guard !retrying, !shuttingDown else { return }
         retrying = true
+        replaceClientAfterClosing(force: true)
+    }
+
+    private func replaceClientAfterClosing(force: Bool) {
         let previous = client
-        previous.close(activeTurn: false) { [weak self] in
-            guard let self else { return }
+        let generation = clientGeneration
+        previous.close(activeTurn: false) { [weak self, weak previous] in
+            guard let self, let previous, !self.shuttingDown,
+                  self.isCurrentClient(previous, generation: generation) else { return }
+            self.clientGeneration &+= 1
             self.client = previous.replacement()
             self.retrying = false
-            self.start(force: true)
+            self.start(force: force)
         }
     }
 
+    private func isCurrentClient(_ candidate: ACPClient, generation: UInt64) -> Bool {
+        client === candidate && clientGeneration == generation
+    }
+
     private static func isStaleLockError(_ error: Error) -> Bool {
         guard case ACPClientError.remote(_, let message) = error else { return false }
         return message.contains("use --force to override a stale lock")

macos/KitDesktop/Models/AppModel.swift

@@ -20,6 +20,10 @@ struct ACPSessionInfo: Equatable {
 final class ACPClient {
     typealias Dictionary = [String: Any]
     static let protocolVersion = 2
+    private static let activeTurnCancelGrace: TimeInterval = 1
+    private static let closeRequestTimeout: TimeInterval = 2
+    private static let terminationGrace: TimeInterval = 1
+    private static let inheritedPipeDrainTimeout: TimeInterval = 2
 
     struct LaunchOverride {
         let executable: URL
@@ -134,7 +138,7 @@ final class ACPClient {
             self.closing = true
             if activeTurn, let sessionID = self.sessionID {
                 self.notifyLocked(method: "session/cancel", params: ["sessionId": sessionID])
-                self.queue.asyncAfter(deadline: .now() + 3) { self.closeSessionLocked() }
+                self.queue.asyncAfter(deadline: .now() + Self.activeTurnCancelGrace) { self.closeSessionLocked() }
             } else { self.closeSessionLocked() }
         }
     }
@@ -520,14 +524,22 @@ final class ACPClient {
     private func closeSessionLocked() {
         guard isProcessRunning else { if let exitStatus { processDidExitLocked(exitStatus) }; return }
         guard let sessionID else { terminateLocked(); return }
-        requestLocked(method: "session/close", params: ["sessionId": sessionID], timeout: 10) { _ in self.queue.async { self.terminateLocked() } }
+        requestLocked(method: "session/close", params: ["sessionId": sessionID], timeout: Self.closeRequestTimeout) { _ in
+            self.queue.async { self.terminateLocked() }
+        }
+        // Request completions are delivered on the main queue. Keep the hard stop on
+        // the transport queue so a busy UI cannot push process teardown past app exit.
+        queue.asyncAfter(deadline: .now() + Self.closeRequestTimeout) {
+            guard self.closing, !self.exited else { return }
+            self.terminateLocked()
+        }
     }
 
     private func terminateLocked() {
         try? inputPipe.fileHandleForWriting.close()
         if let group = processGroup { _ = Darwin.kill(-group, SIGTERM) }
         else if let pid = processPID, isProcessRunning { _ = Darwin.kill(pid, SIGTERM) }
-        queue.asyncAfter(deadline: .now() + 2) {
+        queue.asyncAfter(deadline: .now() + Self.terminationGrace) {
             if let group = self.processGroup {
                 if Darwin.kill(-group, 0) == 0 { _ = Darwin.kill(-group, SIGKILL) }
             } else if self.isProcessRunning, let pid = self.processPID { _ = Darwin.kill(pid, SIGKILL) }
@@ -568,8 +580,12 @@ final class ACPClient {
     private func processDidExitLocked(_ status: Int32) {
         guard exitStatus == nil else { return }
         exitStatus = status
+        // The process leader can crash while descendants keep inherited stdio pipes open.
+        // Signal the whole group even after the leader has been reaped so those descendants
+        // cannot keep shutdown or transport completion alive indefinitely.
+        terminateLocked()
         if stdoutClosed && stderrClosed { finalizeExitLocked(status); return }
-        queue.asyncAfter(deadline: .now() + 5) {
+        queue.asyncAfter(deadline: .now() + Self.inheritedPipeDrainTimeout) {
             guard !self.exited, self.exitStatus != nil else { return }
             self.outputPipe.fileHandleForReading.readabilityHandler = nil
             self.errorPipe.fileHandleForReading.readabilityHandler = nil

macos/KitDesktop/Models/ConversationController.swift

@@ -27,7 +27,10 @@ final class PersistenceStore {
     }
 
     func load() throws -> PersistedAppState {
-        guard FileManager.default.fileExists(atPath: fileURL.path) else { return PersistedAppState() }
+        guard FileManager.default.fileExists(atPath: fileURL.path) else {
+            guard FileManager.default.fileExists(atPath: backupURL.path) else { return PersistedAppState() }
+            return try decode(Data(contentsOf: backupURL))
+        }
         do { return try decode(Data(contentsOf: fileURL)) }
         catch let error as PersistenceError {
             if case .unsupportedSchema = error { throw error }
@@ -72,7 +75,12 @@ final class PersistenceStore {
         try? FileManager.default.copyItem(at: fileURL, to: quarantine)
         if FileManager.default.fileExists(atPath: backupURL.path) {
             do { return try decode(Data(contentsOf: backupURL)) }
-            catch { throw PersistenceError.unreadableState("\(original.localizedDescription); backup: \(error.localizedDescription)") }
+            catch let error as PersistenceError {
+                if case .unsupportedSchema = error { throw error }
+                throw PersistenceError.unreadableState("\(original.localizedDescription); backup: \(error.localizedDescription)")
+            } catch {
+                throw PersistenceError.unreadableState("\(original.localizedDescription); backup: \(error.localizedDescription)")
+            }
         }
         throw PersistenceError.unreadableState(original.localizedDescription)
     }