From e9793f91060bd20af800545a0a7f369d9c968d7b Mon Sep 17 00:00:00 2001 From: Dustin Hilgaertner Date: Fri, 8 May 2026 13:42:43 -0500 Subject: [PATCH] Run session deletion off the main actor so the UI stays responsive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Worktree teardown shells out to git and rm via process.waitUntilExit and synchronous FileManager calls. Doing that on the @MainActor SessionService froze the app — single deletes for a few seconds, batch deletes proportionally longer because BulkDeleteSessionsAlert iterated serially. Push the slow disk work into a Task.detached, expose a per-session isDeletingSession flag for an inline spinner / dimmed row, surface failures via sessionDeletionError so the row keeps a red triangle (and the session itself) until the user retries, and fan batch deletes out as concurrent Tasks so N deletes finish in roughly one delete's time. The CLI delete-session RPC inherits the same path. 🤖 Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude Crow-Session: EAD0ACF1-C213-4DE8-8E46-7282960F5694 --- .../CrowCore/Sources/CrowCore/AppState.swift | 9 + .../Sources/CrowUI/DeleteSessionAlert.swift | 23 +- .../Sources/CrowUI/SessionDetailView.swift | 1 + .../Sources/CrowUI/SessionListView.swift | 32 ++- Sources/Crow/App/SessionService.swift | 205 ++++++++++++------ 5 files changed, 197 insertions(+), 73 deletions(-) diff --git a/Packages/CrowCore/Sources/CrowCore/AppState.swift b/Packages/CrowCore/Sources/CrowCore/AppState.swift index 7a94e95..ecd1a5e 100644 --- a/Packages/CrowCore/Sources/CrowCore/AppState.swift +++ b/Packages/CrowCore/Sources/CrowCore/AppState.swift @@ -254,6 +254,15 @@ public final class AppState { /// Must be cleaned up when a session is deleted (see `SessionService.deleteSession`). public var isMarkingInReview: [UUID: Bool] = [:] + /// Sessions whose async deletion (worktree teardown, branch removal, persistence) + /// is currently in progress. Set on the main actor at the start of + /// `SessionService.deleteSession` and cleared when the session is fully removed. + public var isDeletingSession: [UUID: Bool] = [:] + + /// Most recent delete-cleanup error for a session, surfaced inline on the row + /// so failures aren't silent. Auto-cleared after a short delay or on retry. + public var sessionDeletionError: [UUID: String] = [:] + /// Called to open a session's primary worktree in VS Code. public var onOpenInVSCode: ((UUID) -> Void)? diff --git a/Packages/CrowUI/Sources/CrowUI/DeleteSessionAlert.swift b/Packages/CrowUI/Sources/CrowUI/DeleteSessionAlert.swift index e8228a4..3fde92a 100644 --- a/Packages/CrowUI/Sources/CrowUI/DeleteSessionAlert.swift +++ b/Packages/CrowUI/Sources/CrowUI/DeleteSessionAlert.swift @@ -134,7 +134,8 @@ enum DeleteSessionMessageBuilder { // MARK: - Bulk Delete Sessions Alert /// View modifier that attaches a bulk delete-sessions confirmation alert. -/// Iterates `selectedIDs` serially through `appState.onDeleteSession`. +/// Fans out `selectedIDs` concurrently through `appState.onDeleteSession` so +/// the UI doesn't wait N×single-delete to repaint. struct BulkDeleteSessionsAlert: ViewModifier { @Binding var isPresented: Bool let selectedIDs: Set @@ -147,14 +148,22 @@ struct BulkDeleteSessionsAlert: ViewModifier { Button(buttonLabel, role: .destructive) { let snapshot = sortedSnapshot Task { + // Fan each delete out as its own Task so they run concurrently. + // SessionService.deleteSession yields the main actor while its + // disk/git cleanup runs on a detached task, so all selected + // sessions clean up in parallel rather than serially. + var tasks: [Task] = [] for id in snapshot { - do { - try await appState.onDeleteSession?(id) - } catch { - NSLog("Failed to delete session \(id): \(error)") - } + tasks.append(Task { + do { + try await appState.onDeleteSession?(id) + } catch { + NSLog("Failed to delete session \(id): \(error)") + } + }) } - await MainActor.run { onCompletion() } + for t in tasks { await t.value } + onCompletion() } } } message: { diff --git a/Packages/CrowUI/Sources/CrowUI/SessionDetailView.swift b/Packages/CrowUI/Sources/CrowUI/SessionDetailView.swift index b280fe0..8e9e441 100644 --- a/Packages/CrowUI/Sources/CrowUI/SessionDetailView.swift +++ b/Packages/CrowUI/Sources/CrowUI/SessionDetailView.swift @@ -198,6 +198,7 @@ public struct SessionDetailView: View { } .buttonStyle(.bordered) .controlSize(.small) + .disabled(appState.isDeletingSession[session.id] == true) } } .padding(.horizontal, 16) diff --git a/Packages/CrowUI/Sources/CrowUI/SessionListView.swift b/Packages/CrowUI/Sources/CrowUI/SessionListView.swift index d9bb4db..47994fd 100644 --- a/Packages/CrowUI/Sources/CrowUI/SessionListView.swift +++ b/Packages/CrowUI/Sources/CrowUI/SessionListView.swift @@ -132,6 +132,7 @@ public struct SessionListView: View { } label: { Label("Delete", systemImage: "trash") } + .disabled(appState.isDeletingSession[session.id] == true) } } } @@ -255,6 +256,8 @@ public struct SessionListView: View { @ViewBuilder private func sessionContextMenu(_ session: Session) -> some View { + let deleting = appState.isDeletingSession[session.id] == true + if session.status == .active, session.ticketURL != nil, session.provider == .github { @@ -263,7 +266,7 @@ public struct SessionListView: View { } label: { Label("Mark as In Review", systemImage: "eye.circle") } - .disabled(appState.isMarkingInReview[session.id] == true) + .disabled(appState.isMarkingInReview[session.id] == true || deleting) } if session.status == .active || session.status == .inReview { @@ -272,6 +275,7 @@ public struct SessionListView: View { } label: { Label("Mark as Completed", systemImage: "checkmark.circle") } + .disabled(deleting) } Button(role: .destructive) { @@ -279,6 +283,7 @@ public struct SessionListView: View { } label: { Label("Delete", systemImage: "trash") } + .disabled(deleting) } private func filteredSessions(_ sessions: [Session]) -> [Session] { @@ -461,6 +466,14 @@ struct SessionRow: View { appState.terminals(for: session.id).contains { $0.backend == .tmux } } + private var isDeleting: Bool { + appState.isDeletingSession[session.id] == true + } + + private var deletionError: String? { + appState.sessionDeletionError[session.id] + } + var body: some View { HStack(spacing: 8) { if isSelectionMode { @@ -471,9 +484,11 @@ struct SessionRow: View { } .buttonStyle(.plain) .accessibilityLabel(isChecked ? "Deselect \(session.name)" : "Select \(session.name)") + .disabled(isDeleting) } rowContent + .opacity(isDeleting ? 0.55 : 1.0) } .padding(.horizontal, 8) .padding(.vertical, 6) @@ -506,6 +521,7 @@ struct SessionRow: View { } .animation(.easeInOut(duration: 1.0).repeatForever(autoreverses: true), value: needsAttention) .animation(.easeInOut(duration: 0.2), value: appState.hideSessionDetails) + .animation(.easeInOut(duration: 0.2), value: isDeleting) .padding(.vertical, 1) } @@ -521,7 +537,19 @@ struct SessionRow: View { RemoteControlBadge(compact: true) } Spacer() - statusIndicator + if isDeleting { + ProgressView() + .controlSize(.small) + .accessibilityLabel("Deleting session") + } else if let deletionError { + Image(systemName: "exclamationmark.triangle.fill") + .foregroundStyle(.red) + .font(.caption) + .help("Delete failed: \(deletionError)") + .accessibilityLabel("Delete failed: \(deletionError)") + } else { + statusIndicator + } } // Row 2: Ticket title (if any) diff --git a/Sources/Crow/App/SessionService.swift b/Sources/Crow/App/SessionService.swift index 345df73..9a6c22c 100644 --- a/Sources/Crow/App/SessionService.swift +++ b/Sources/Crow/App/SessionService.swift @@ -474,88 +474,70 @@ final class SessionService { // MARK: - Delete Session + /// Snapshot of one worktree's cleanup work, captured on the MainActor and + /// passed by value into a detached task so disk/git operations don't block UI. + struct WorktreeCleanupItem: Sendable { + let repoPath: String + let worktreePath: String + let branch: String + let isMainCheckout: Bool + } + /// Delete a session and clean up all associated resources. /// /// Performs a full cascade: destroys terminal surfaces, removes worktrees from disk /// (with branch deletion for non-protected branches), removes hook configs, and cleans /// up all in-memory state (sessions, worktrees, links, terminals, hook state, PR status). /// The manager session cannot be deleted. + /// + /// The slow filesystem/git work runs in a detached task so the main thread stays + /// responsive. While cleanup is in flight, `appState.isDeletingSession[id]` is `true` + /// so the UI can show a spinner. On failure the session is left in place with + /// `appState.sessionDeletionError[id]` set, allowing the user to retry. func deleteSession(id: UUID) async { guard id != AppState.managerSessionID else { return } + guard appState.isDeletingSession[id] != true else { return } let session = appState.sessions.first(where: { $0.id == id }) let wts = appState.worktrees(for: id) let terminals = appState.terminals(for: id) - - // Destroy live terminal surfaces (routes per-backend so .tmux windows are killed too) - for terminal in terminals { - TerminalRouter.destroy(terminal) + let isReview = session?.kind == .review + let items = wts.map { + WorktreeCleanupItem( + repoPath: $0.repoPath, + worktreePath: $0.worktreePath, + branch: $0.branch, + isMainCheckout: $0.isMainRepoCheckout + ) } - if session?.kind == .review { - // For review sessions, clean up the clone directory - for wt in wts { - try? FileManager.default.removeItem(atPath: wt.worktreePath) - NSLog("[SessionService] Cleaned up review clone: \(wt.worktreePath)") + appState.isDeletingSession[id] = true + appState.sessionDeletionError.removeValue(forKey: id) + + // Slow git + filesystem work runs on a background thread so the main actor + // stays free to render the spinner and respond to other input. + let cleanupError: String? = await Task.detached(priority: .utility) { + Self.performDiskCleanup(items: items, isReview: isReview) + }.value + + if let cleanupError { + // Leave session, terminals, and persisted state intact so the user can + // retry. Surface the failure inline; auto-clear after a short delay so + // the row returns to its normal appearance. + appState.sessionDeletionError[id] = cleanupError + appState.isDeletingSession.removeValue(forKey: id) + Task { [weak appState] in + try? await Task.sleep(nanoseconds: 6_000_000_000) + _ = await MainActor.run { appState?.sessionDeletionError.removeValue(forKey: id) } } - } else { - // Remove worktrees from disk: git worktree remove + branch delete - // Skip cleanup for worktrees that point at the main repo checkout (not a real worktree) - for wt in wts { - let isMainCheckout = wt.isMainRepoCheckout - - if isMainCheckout { - NSLog("Skipping worktree cleanup for main checkout: \(wt.worktreePath) (branch: \(wt.branch))") - continue - } - - // Remove our hook config from settings.local.json before deleting the worktree - HookConfigGenerator.removeHookConfig(worktreePath: wt.worktreePath) - - do { - // Remove the worktree - let removeResult = try await shell("git", "-C", wt.repoPath, "worktree", "remove", "--force", wt.worktreePath) - NSLog("Removed worktree: \(wt.worktreePath) \(removeResult)") - - // Delete the local branch (only if not a protected branch) - if !SessionWorktree.isProtectedBranch(wt.branch) { - do { - _ = try await shell("git", "-C", wt.repoPath, "branch", "-D", wt.branch) - } catch { - NSLog("[SessionService] Failed to delete branch \(wt.branch): \(error)") - } - } - - // Prune worktree metadata - do { - _ = try await shell("git", "-C", wt.repoPath, "worktree", "prune") - } catch { - NSLog("[SessionService] Failed to prune worktree metadata: \(error)") - } + return + } - // Remove the directory if it still exists - if FileManager.default.fileExists(atPath: wt.worktreePath) { - do { - try FileManager.default.removeItem(atPath: wt.worktreePath) - } catch { - NSLog("[SessionService] Failed to remove directory \(wt.worktreePath): \(error)") - } - } - } catch { - NSLog("[SessionService] Failed to remove worktree \(wt.worktreePath): \(error)") - // Still try to remove the directory (but not if it's the main repo) - if FileManager.default.fileExists(atPath: wt.worktreePath) { - do { - try FileManager.default.removeItem(atPath: wt.worktreePath) - } catch { - NSLog("[SessionService] Failed to remove directory \(wt.worktreePath): \(error)") - } - } - } - } + // Cleanup succeeded — destroy live terminal surfaces and tear down state. + for terminal in terminals { + TerminalRouter.destroy(terminal) } - // Remove from state and persistence appState.sessions.removeAll { $0.id == id } appState.worktrees.removeValue(forKey: id) appState.links.removeValue(forKey: id) @@ -568,6 +550,7 @@ final class SessionService { appState.removeHookState(for: id) appState.prStatus.removeValue(forKey: id) appState.isMarkingInReview.removeValue(forKey: id) + appState.isDeletingSession.removeValue(forKey: id) store.mutate { data in data.sessions.removeAll { $0.id == id } @@ -581,6 +564,100 @@ final class SessionService { } } + /// Run the on-disk portion of session deletion. Safe to call from any thread — + /// touches no MainActor state. Returns `nil` on success, or a short error + /// string describing the first fatal failure (a worktree that could be removed + /// neither by `git worktree remove` nor by direct directory removal). + /// Soft failures (branch delete, prune) only get NSLog'd. + nonisolated static func performDiskCleanup( + items: [WorktreeCleanupItem], + isReview: Bool + ) -> String? { + var firstFatalError: String? = nil + + for item in items { + if item.isMainCheckout { + NSLog("Skipping worktree cleanup for main checkout: \(item.worktreePath) (branch: \(item.branch))") + continue + } + + if isReview { + guard FileManager.default.fileExists(atPath: item.worktreePath) else { continue } + do { + try FileManager.default.removeItem(atPath: item.worktreePath) + NSLog("[SessionService] Cleaned up review clone: \(item.worktreePath)") + } catch { + let msg = "Failed to remove review clone: \(error.localizedDescription)" + NSLog("[SessionService] \(msg) (\(item.worktreePath))") + if firstFatalError == nil { firstFatalError = msg } + } + continue + } + + // Remove our hook config from settings.local.json before deleting the worktree + HookConfigGenerator.removeHookConfig(worktreePath: item.worktreePath) + + var gitRemoveFailed = false + do { + let removeResult = try runShellSync(["git", "-C", item.repoPath, "worktree", "remove", "--force", item.worktreePath]) + NSLog("Removed worktree: \(item.worktreePath) \(removeResult)") + + if !SessionWorktree.isProtectedBranch(item.branch) { + do { + _ = try runShellSync(["git", "-C", item.repoPath, "branch", "-D", item.branch]) + } catch { + NSLog("[SessionService] Failed to delete branch \(item.branch): \(error)") + } + } + + do { + _ = try runShellSync(["git", "-C", item.repoPath, "worktree", "prune"]) + } catch { + NSLog("[SessionService] Failed to prune worktree metadata: \(error)") + } + } catch { + gitRemoveFailed = true + NSLog("[SessionService] Failed to remove worktree \(item.worktreePath): \(error)") + } + + // Either way, ensure the directory is gone. + if FileManager.default.fileExists(atPath: item.worktreePath) { + do { + try FileManager.default.removeItem(atPath: item.worktreePath) + } catch { + NSLog("[SessionService] Failed to remove directory \(item.worktreePath): \(error)") + if gitRemoveFailed && firstFatalError == nil { + firstFatalError = "Could not remove worktree at \(item.worktreePath): \(error.localizedDescription)" + } + } + } + } + + return firstFatalError + } + + /// Synchronous shell helper safe to call from any thread. Used by + /// `performDiskCleanup` while running on a detached task. + nonisolated static func runShellSync(_ args: [String]) throws -> String { + let process = Process() + let stdoutPipe = Pipe() + let stderrPipe = Pipe() + process.executableURL = URL(fileURLWithPath: "/usr/bin/env") + process.arguments = args + process.environment = ShellEnvironment.shared.env + process.standardOutput = stdoutPipe + process.standardError = stderrPipe + try process.run() + process.waitUntilExit() + let stdout = String(data: stdoutPipe.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8) ?? "" + let stderr = String(data: stderrPipe.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8) ?? "" + guard process.terminationStatus == 0 else { + throw NSError(domain: "SessionService", code: Int(process.terminationStatus), + userInfo: [NSLocalizedDescriptionKey: stderr.isEmpty ? stdout : stderr]) + } + return stdout + } + // MARK: - Worktree Safety Checks // Protected branch and main-checkout detection are centralized on SessionWorktree in CrowCore.