From b7bd0fff8850f6c8b9283d4a6ee50e4d24bb86e1 Mon Sep 17 00:00:00 2001 From: Micah Alpern Date: Mon, 21 Sep 2026 07:21:21 -0700 Subject: [PATCH] Bound the duplicate-app Spotlight scan and take it off the main actor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Opening Settings → Advanced could freeze KeyPath behind a spinner indefinitely. detectDuplicateAppCopies() ran mdfind with an unbounded waitUntilExit(), bypassing SubprocessRunner, and every caller invoked it synchronously from main-actor UI code. When Spotlight stalled (observed with KeyPath.app copies indexed on an external drive), the whole window hung until the mdfind process was killed by hand. - detectDuplicateAppCopies() is async and runs mdfind through SubprocessRunner with a 2s timeout; timeout, failure, or an empty result falls back to the canonical install locations. - Settings, Advanced settings, and both wizard pages await it; the synchronous copyPublishedStatus() folds the result in from a follow-up task so its observer contract is unchanged. - Sparkle_generate_appcast/ staging copies are excluded from the duplicate report alongside the existing build directories. - Tests for the bounded scan and the timeout fallback; bug note in docs/bugs/. Co-Authored-By: Claude Opus 5 --- .../Managers/HelperMaintenance.swift | 84 ++++++++++++------- .../UI/Settings/AdvancedSettingsTabView.swift | 4 +- .../UI/Settings/SettingsView.swift | 23 ++++- .../WizardSystemStatusOverview.swift | 6 +- .../UI/Pages/WizardHelperPage.swift | 6 +- .../WizardServiceProtocols.swift | 2 +- .../Core/SubprocessRunnerTests.swift | 16 ++++ .../InstallerEngineEndToEndTests.swift | 2 +- .../Managers/HelperMaintenanceTests.swift | 44 +++++++++- ...26-09-21-duplicate-app-scan-mdfind-hang.md | 55 ++++++++++++ 10 files changed, 202 insertions(+), 40 deletions(-) create mode 100644 docs/bugs/2026-09-21-duplicate-app-scan-mdfind-hang.md diff --git a/Sources/KeyPathAppKit/Managers/HelperMaintenance.swift b/Sources/KeyPathAppKit/Managers/HelperMaintenance.swift index d1dd82f3a..f80b268fa 100644 --- a/Sources/KeyPathAppKit/Managers/HelperMaintenance.swift +++ b/Sources/KeyPathAppKit/Managers/HelperMaintenance.swift @@ -63,7 +63,7 @@ public final class HelperMaintenance { log("🔐 Helper install finished") } - let copies = detectDuplicateAppCopies() + let copies = await detectDuplicateAppCopies() if copies.filter({ !$0.hasPrefix("/Applications/KeyPath.app") }).count > 0 { log("⚠️ Multiple KeyPath.app copies detected:") for c in copies { @@ -115,7 +115,7 @@ public final class HelperMaintenance { } // Step 0: Duplicate app detection - let copies = detectDuplicateAppCopies() + let copies = await detectDuplicateAppCopies() if copies.filter({ !$0.hasPrefix("/Applications/KeyPath.app") }).count > 0 { log("⚠️ Multiple KeyPath.app copies detected:") for c in copies { @@ -205,47 +205,39 @@ public final class HelperMaintenance { return healthy } - // Find all KeyPath.app copies visible to Spotlight (fast, robust in practice). + // Find all KeyPath.app copies visible to Spotlight. // Results are sorted with `/Applications/KeyPath.app` first if present. - // Excludes build directories (dist/, .build/, build/) to avoid flagging build artifacts. + // Excludes build and Sparkle staging directories to avoid flagging artifacts. + // + // Async and time-boxed on purpose: Spotlight can stall for minutes (for example + // while it wakes a sleeping external volume that holds an indexed KeyPath.app), + // and this used to run `mdfind` with an unbounded `waitUntilExit()` on the main + // actor, freezing Settings and the wizard behind a spinner. A slow or failed + // scan now falls back to the canonical install locations instead. #if DEBUG nonisolated(unsafe) static var testDuplicateAppPathsOverride: (() -> [String]?)? + nonisolated(unsafe) static var testDuplicateScanRunner: (any SubprocessRunning)? #endif - public nonisolated func detectDuplicateAppCopies() -> [String] { - var paths: [String] = [] - let process = Process() - process.launchPath = "/usr/bin/mdfind" - process.arguments = ["kMDItemFSName == 'KeyPath.app'c"] - let out = Pipe() - process.standardOutput = out - process.standardError = Pipe() - do { try process.run() } catch { - return canonicalAppCandidates() - } + nonisolated static let duplicateScanTimeout: TimeInterval = 2 + nonisolated static let duplicateScanExcludedPathFragments = [ + "/dist/", "/.build/", "/build/", "/DerivedData/", "/Sparkle_generate_appcast/" + ] + + public nonisolated func detectDuplicateAppCopies() async -> [String] { + var paths: [String] #if DEBUG if let override = Self.testDuplicateAppPathsOverride?() { paths = override } else { - process.waitUntilExit() - let s = String(data: out.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8) ?? "" - paths = s.split(separator: "\n").map(String.init) - if paths.isEmpty { - paths = canonicalAppCandidates() - } + paths = await spotlightAppCopies() } #else - process.waitUntilExit() - let s = String(data: out.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8) ?? "" - paths = s.split(separator: "\n").map(String.init) - if paths.isEmpty { - paths = canonicalAppCandidates() - } + paths = await spotlightAppCopies() #endif - // Filter out build directories to avoid flagging build artifacts as duplicates - let buildDirPatterns = ["/dist/", "/.build/", "/build/", "/DerivedData/"] + // Filter out build/staging directories to avoid flagging artifacts as duplicates paths = paths.filter { path in - !buildDirPatterns.contains { pattern in path.contains(pattern) } + !Self.duplicateScanExcludedPathFragments.contains { path.contains($0) } } paths = Array(Set(paths)) // unique @@ -261,6 +253,34 @@ public final class HelperMaintenance { return paths } + private nonisolated func spotlightAppCopies() async -> [String] { + var runner: any SubprocessRunning = SubprocessRunner.shared + #if DEBUG + if let override = Self.testDuplicateScanRunner { + runner = override + } + #endif + do { + let result = try await runner.run( + "/usr/bin/mdfind", + args: ["kMDItemFSName == 'KeyPath.app'c"], + timeout: Self.duplicateScanTimeout + ) + let paths = result.stdout.split(separator: "\n").map(String.init) + if result.exitCode == 0, !paths.isEmpty { + return paths + } + AppLogger.shared.log( + "⚠️ [HelperMaintenance] Duplicate-app Spotlight scan exited \(result.exitCode) with \(paths.count) result(s); using canonical locations" + ) + } catch { + AppLogger.shared.log( + "⚠️ [HelperMaintenance] Duplicate-app Spotlight scan failed; using canonical locations: \(error)" + ) + } + return canonicalAppCandidates() + } + // MARK: - Private steps private func unregisterHelperIfPresent() async { @@ -427,7 +447,9 @@ public final class HelperMaintenance { for p in defaults where Foundation.FileManager().fileExists(atPath: p) { candidates.append(p) } - return candidates.isEmpty ? defaults : candidates + // Only locations that exist: returning the hardcoded defaults when none do + // reported phantom "multiple copies" whenever the Spotlight scan fell back. + return candidates } private nonisolated func legacyHelperArtifactPaths() -> (legacyBin: String, legacyPlist: String) { diff --git a/Sources/KeyPathAppKit/UI/Settings/AdvancedSettingsTabView.swift b/Sources/KeyPathAppKit/UI/Settings/AdvancedSettingsTabView.swift index dc41a1d93..48c92a4c8 100644 --- a/Sources/KeyPathAppKit/UI/Settings/AdvancedSettingsTabView.swift +++ b/Sources/KeyPathAppKit/UI/Settings/AdvancedSettingsTabView.swift @@ -183,7 +183,7 @@ struct AdvancedSettingsTabView: View { } .task { await refreshHelperStatus() - duplicateAppCopies = HelperMaintenance.shared.detectDuplicateAppCopies() + duplicateAppCopies = await HelperMaintenance.shared.detectDuplicateAppCopies() backups = kanataManager.underlyingManager.configBackupManager.getAvailableBackups() } .alert("Uninstall Privileged Helper?", isPresented: $showingHelperUninstallConfirm) { @@ -410,7 +410,7 @@ struct AdvancedSettingsTabView: View { } } - let refreshed = HelperMaintenance.shared.detectDuplicateAppCopies() + let refreshed = await HelperMaintenance.shared.detectDuplicateAppCopies() await MainActor.run { duplicateAppCopies = refreshed if removed > 0 { diff --git a/Sources/KeyPathAppKit/UI/Settings/SettingsView.swift b/Sources/KeyPathAppKit/UI/Settings/SettingsView.swift index 987edf932..cee7d15ae 100644 --- a/Sources/KeyPathAppKit/UI/Settings/SettingsView.swift +++ b/Sources/KeyPathAppKit/UI/Settings/SettingsView.swift @@ -19,6 +19,7 @@ struct StatusSettingsTabView: View { @State var wizardIssues: [WizardIssue] = [] @State var tcpConfigured: Bool? @State var duplicateAppCopies: [String] = [] + @State private var lastDuplicateScan: Date? @State private var settingsToastManager = WizardToastManager() @State var showingPermissionAlert = false @State private var localServiceRunning: Bool? // Optimistic local state for instant toggle feedback @@ -493,7 +494,7 @@ struct StatusSettingsTabView: View { let controller = MainAppStateController.shared let context = controller.lastValidatedSystemContext ?? .empty - let duplicates = HelperMaintenance.shared.detectDuplicateAppCopies() + let duplicates = duplicateAppCopies permissionSnapshot = context.permissions systemContext = context @@ -504,6 +505,26 @@ struct StatusSettingsTabView: View { || duplicates.count > 1 || (context.services.kanataRunning && controller.lastTCPConfigured == false) duplicateAppCopies = duplicates + refreshDuplicateAppCopies() + } + + /// Spotlight can stall, so the duplicate-app scan never runs inline on the main + /// actor: publish status immediately, then fold the scan result in when it lands. + /// Until the first scan lands (at most `duplicateScanTimeout`), a genuine duplicate + /// install is not yet reflected in the setup banner; that brief warm-up is intended. + /// Validation publishes roughly every minute, so rescans are throttled to match. + private func refreshDuplicateAppCopies() { + if let last = lastDuplicateScan, Date().timeIntervalSince(last) < 60 { return } + lastDuplicateScan = Date() + Task { @MainActor in + let duplicates = await HelperMaintenance.shared.detectDuplicateAppCopies() + guard duplicates != duplicateAppCopies else { return } + duplicateAppCopies = duplicates + let context = systemContext ?? .empty + showSetupBanner = !(context.permissions.isSystemReady && context.services.isHealthy) + || duplicates.count > 1 + || (context.services.kanataRunning && tcpConfigured == false) + } } private func startViaInstallerEngine() async { diff --git a/Sources/KeyPathInstallationWizard/UI/Components/WizardSystemStatusOverview.swift b/Sources/KeyPathInstallationWizard/UI/Components/WizardSystemStatusOverview.swift index d151e9d0a..bb0064296 100644 --- a/Sources/KeyPathInstallationWizard/UI/Components/WizardSystemStatusOverview.swift +++ b/Sources/KeyPathInstallationWizard/UI/Components/WizardSystemStatusOverview.swift @@ -120,9 +120,13 @@ public struct WizardSystemStatusOverview: View { } } .onAppear { - duplicateCopies = WizardDependencies.helperMaintenance?.detectDuplicateAppCopies() ?? [] updateNavSequence() } + .task { + // .task, not a Task in .onAppear: dismissing the page cancels the scan + // (and SubprocessRunner kills its mdfind) instead of leaving it running. + duplicateCopies = await WizardDependencies.helperMaintenance?.detectDuplicateAppCopies() ?? [] + } .onChange(of: showAllItems) { _, _ in updateNavSequence() } .onChange(of: issues.count) { _, _ in updateNavSequence() } .onChange(of: systemState) { _, _ in updateNavSequence() } diff --git a/Sources/KeyPathInstallationWizard/UI/Pages/WizardHelperPage.swift b/Sources/KeyPathInstallationWizard/UI/Pages/WizardHelperPage.swift index e7d7870ff..fa5953428 100644 --- a/Sources/KeyPathInstallationWizard/UI/Pages/WizardHelperPage.swift +++ b/Sources/KeyPathInstallationWizard/UI/Pages/WizardHelperPage.swift @@ -171,9 +171,13 @@ public struct WizardHelperPage: View { isLoadingHelperStatus = false } .onAppear { - duplicateCopies = WizardDependencies.helperMaintenance?.detectDuplicateAppCopies() ?? [] logLoginItemsDiagnostics() } + .task { + // .task, not a Task in .onAppear: dismissing the page cancels the scan + // (and SubprocessRunner kills its mdfind) instead of leaving it running. + duplicateCopies = await WizardDependencies.helperMaintenance?.detectDuplicateAppCopies() ?? [] + } .onDisappear { stopApprovalPolling() } diff --git a/Sources/KeyPathWizardCore/WizardServiceProtocols.swift b/Sources/KeyPathWizardCore/WizardServiceProtocols.swift index 63a74d400..79f51e285 100644 --- a/Sources/KeyPathWizardCore/WizardServiceProtocols.swift +++ b/Sources/KeyPathWizardCore/WizardServiceProtocols.swift @@ -30,7 +30,7 @@ public extension WizardSystemValidating { @MainActor public protocol WizardHelperMaintaining: AnyObject, Sendable { - func detectDuplicateAppCopies() -> [String] + func detectDuplicateAppCopies() async -> [String] func installOrRefresh() async -> Bool func runCleanupAndRepair(useAppleScriptFallback: Bool) async -> Bool func runCleanupAndRepair(useAppleScriptFallback: Bool, forceFullRepair: Bool) async -> Bool diff --git a/Tests/KeyPathTests/Core/SubprocessRunnerTests.swift b/Tests/KeyPathTests/Core/SubprocessRunnerTests.swift index f73967c12..17812c7a9 100644 --- a/Tests/KeyPathTests/Core/SubprocessRunnerTests.swift +++ b/Tests/KeyPathTests/Core/SubprocessRunnerTests.swift @@ -137,6 +137,22 @@ final class SubprocessRunnerTests: XCTestCase { // MARK: - Timeout Scenarios + func testRealRunnerTimeoutTerminatesLongRunningProcess() async { + // The fake above only proves callers handle a thrown timeout. This drives the + // real runner against a real process, which is the path that let an unbounded + // mdfind freeze Settings (docs/bugs/2026-09-21-duplicate-app-scan-mdfind-hang.md). + let start = Date() + do { + _ = try await SubprocessRunner.shared.run("/bin/sleep", args: ["10"], timeout: 0.5) + XCTFail("Expected timeout error") + } catch let SubprocessError.timeout(executable, _) { + XCTAssertEqual(executable, "/bin/sleep") + } catch { + XCTFail("Unexpected error: \(error)") + } + XCTAssertLessThan(Date().timeIntervalSince(start), 5, "Timeout must bound the wait") + } + func testTimeoutHandling() async { // Setup fake to simulate timeout await fakeRunner.setShouldTimeout(true) diff --git a/Tests/KeyPathTests/InstallationEngine/InstallerEngineEndToEndTests.swift b/Tests/KeyPathTests/InstallationEngine/InstallerEngineEndToEndTests.swift index ac7216c13..efb0e91ff 100644 --- a/Tests/KeyPathTests/InstallationEngine/InstallerEngineEndToEndTests.swift +++ b/Tests/KeyPathTests/InstallationEngine/InstallerEngineEndToEndTests.swift @@ -766,7 +766,7 @@ private final class StubHelperMaintenance: WizardHelperMaintaining { var logLines: [String] = [] var lastErrorLine: String? - func detectDuplicateAppCopies() -> [String] { + func detectDuplicateAppCopies() async -> [String] { ["/Applications/KeyPath.app"] } diff --git a/Tests/KeyPathTests/Managers/HelperMaintenanceTests.swift b/Tests/KeyPathTests/Managers/HelperMaintenanceTests.swift index 0b847ae11..3c70dad18 100644 --- a/Tests/KeyPathTests/Managers/HelperMaintenanceTests.swift +++ b/Tests/KeyPathTests/Managers/HelperMaintenanceTests.swift @@ -1,4 +1,5 @@ @testable import KeyPathAppKit +@testable import KeyPathCore @preconcurrency import XCTest @MainActor @@ -13,6 +14,7 @@ final class HelperMaintenanceTests: XCTestCase { override func tearDown() async throws { try await super.tearDown() HelperMaintenance.testDuplicateAppPathsOverride = nil + HelperMaintenance.testDuplicateScanRunner = nil HelperMaintenance.testLegacyHelperArtifactPathsOverride = nil HelperMaintenance.shared.applyTestHooks(nil) HelperManager.testHelperFunctionalityOverride = nil @@ -20,17 +22,18 @@ final class HelperMaintenanceTests: XCTestCase { AdminCommandExecutorHolder.shared = originalExecutor } - func testDetectDuplicateAppCopiesFiltersBuildPathsAndSortsApplicationsFirst() { + func testDetectDuplicateAppCopiesFiltersBuildPathsAndSortsApplicationsFirst() async { HelperMaintenance.testDuplicateAppPathsOverride = { [ "/Users/test/Downloads/KeyPath.app", "/dist/KeyPath.app", + "/Volumes/Cache/Sparkle_generate_appcast/abc123/KeyPath.app", "/Applications/KeyPath.app", "/Users/test/KeyPath.app" ] } - let copies = HelperMaintenance.shared.detectDuplicateAppCopies() + let copies = await HelperMaintenance.shared.detectDuplicateAppCopies() XCTAssertEqual(copies.first, "/Applications/KeyPath.app") let remaining = Set(copies.dropFirst()) XCTAssertEqual( @@ -42,6 +45,43 @@ final class HelperMaintenanceTests: XCTestCase { ) } + func testDetectDuplicateAppCopiesUsesBoundedSpotlightScan() async { + let runner = SubprocessRunnerFake.shared + await runner.reset() + await runner.configureRunResult { _, _ in + ProcessResult( + exitCode: 0, + stdout: "/Applications/KeyPath.app\n/Users/test/Downloads/KeyPath.app\n", + stderr: "", + duration: 0.01 + ) + } + HelperMaintenance.testDuplicateScanRunner = runner + + let copies = await HelperMaintenance.shared.detectDuplicateAppCopies() + + XCTAssertEqual(copies, ["/Applications/KeyPath.app", "/Users/test/Downloads/KeyPath.app"]) + let commands = await runner.executedCommands + XCTAssertEqual(commands.first?.executable, "/usr/bin/mdfind") + XCTAssertEqual(HelperMaintenance.duplicateScanTimeout, 2) + XCTAssertTrue(HelperMaintenance.duplicateScanExcludedPathFragments.contains("/Sparkle_generate_appcast/")) + } + + func testDetectDuplicateAppCopiesFallsBackWhenSpotlightTimesOut() async { + let runner = SubprocessRunnerFake.shared + await runner.reset() + await runner.setShouldTimeout(true) + HelperMaintenance.testDuplicateScanRunner = runner + + let copies = await HelperMaintenance.shared.detectDuplicateAppCopies() + + // A stalled Spotlight must degrade to the canonical locations, never hang, and + // never report a location that does not exist (which read as a phantom duplicate). + XCTAssertTrue(copies.allSatisfy { $0.hasSuffix("/KeyPath.app") }) + XCTAssertTrue(copies.allSatisfy { FileManager.default.fileExists(atPath: $0) }) + await runner.reset() + } + func testRunCleanupLogsWarningForDuplicateCopies() async { HelperMaintenance.testDuplicateAppPathsOverride = { [ diff --git a/docs/bugs/2026-09-21-duplicate-app-scan-mdfind-hang.md b/docs/bugs/2026-09-21-duplicate-app-scan-mdfind-hang.md new file mode 100644 index 000000000..9c55a7fce --- /dev/null +++ b/docs/bugs/2026-09-21-duplicate-app-scan-mdfind-hang.md @@ -0,0 +1,55 @@ +# Settings/Wizard Spinner: Unbounded Spotlight Scan on the Main Actor + +**Date:** 2026-09-21 +**Severity:** User-visible hang (window stuck on a spinner indefinitely) +**Status:** Fixed + +## Problem + +Opening Settings → Advanced froze KeyPath behind a spinner for over a minute. The +debug log stopped mid-validation, and `ps` showed a single child process that never +exited: + +``` +/usr/bin/mdfind kMDItemFSName == 'KeyPath.app'c (parent: KeyPath, 70s+) +``` + +Killing that `mdfind` unblocked the app instantly. Run by hand a moment later, the +same query returned in 0.18s. + +## Root Cause + +Two defects compounded: + +1. **No timeout.** `HelperMaintenance.detectDuplicateAppCopies()` launched `mdfind` + with `Process` and called `waitUntilExit()`, bypassing `SubprocessRunner`, which + every other subprocess call uses for its timeout. Spotlight can stall for a long + time — here the index included `KeyPath.app` copies on an external drive + (Sparkle `generate_appcast` staging directories), plausibly waiting on a sleeping + volume — and the call simply never returned. +2. **Called on the main actor.** The function was `nonisolated` but synchronous, and + every caller invoked it from main-actor UI code (`AdvancedSettingsTabView`'s + `.task`, `SettingsView.copyPublishedStatus()`, and the wizard pages' `.onAppear`). + A slow scan therefore froze the entire UI rather than one status row. + +A third, smaller issue: the build-artifact filter did not know Sparkle's +`Sparkle_generate_appcast/` staging directories, so those copies would have been +reported as duplicate installs. + +## Fix + +- `detectDuplicateAppCopies()` is now `async` and runs `mdfind` through + `SubprocessRunner` with a 2-second timeout (`duplicateScanTimeout`). A timeout, + launch failure, non-zero exit, or empty result falls back to the canonical install + locations. +- All callers `await` it off the synchronous path. `SettingsView.copyPublishedStatus()` + stays synchronous (its validation-date observer must only copy published state, see + `SettingsStatusValidationLintTests`) and folds the scan result in from a follow-up task. +- `/Sparkle_generate_appcast/` joins the excluded path fragments. +- Tests: `testDetectDuplicateAppCopiesUsesBoundedSpotlightScan`, + `testDetectDuplicateAppCopiesFallsBackWhenSpotlightTimesOut`. + +## Lesson + +Any subprocess reachable from UI code must go through `SubprocessRunner` with a +timeout. "Fast in practice" (the old comment on this function) is not a bound.