From 5b1f3dccecd6f441a54f6456d2c8914e57cb2345 Mon Sep 17 00:00:00 2001 From: Iliya Date: Mon, 1 Jun 2026 15:40:05 -0500 Subject: [PATCH] Fix three scan-correctness bugs: Trash FDA, duplicate rows, inflated estimate Root cause shared by two of these: TargetedScanner swallowed permission denials and never de-duplicated overlapping targets. 1. "Trash is empty" on a full Trash. ~/.Trash is TCC-protected; without Full Disk Access the recursive enumerator silently yields nothing and fileExists() still returns true, so the scan looked empty. The scanner now probes each target root with open(O_RDONLY|O_DIRECTORY) and reports EPERM/EACCES via the new ScanOutcome.permissionDenied. TrashBinsView surfaces a "Full Disk Access needed" empty-state (Open Settings / Rescan) instead of a false "Trash is empty." 2. Large & Old Files listed the same file twice. That module scans ~ recursively AND ~/Downloads recursively, so Downloads files were enumerated by both targets. Scan results are now de-duplicated by URL. 3. Cleanup freed less than the estimate (~2 GB estimated, 934 MB freed). Same duplicates: the pre-clean estimate summed each duplicate FileItem, while Clean trashes each URL once (the second copy hits a benign "already gone" skip). selectedSize() now counts each URL once, so the "X will be freed" preview matches reality even when a file appears in multiple categories. Bump 1.5.2 -> 1.5.3. --- .../Core/Scanner/TargetedScanner.swift | 58 +++++++++++++--- .../Modules/TrashBins/TrashBinsModule.swift | 26 +++++-- .../Views/Cleanup/TrashBinsView.swift | 14 ++-- .../Views/Shared/ModuleContainerView.swift | 64 +++++++++++++++-- Sources/MacCleanKit/CleanFilter.swift | 18 +++++ Sources/MacCleanKit/Constants.swift | 2 +- Sources/MacCleanKit/ScanOutcome.swift | 22 ++++++ Tests/MacCleanKitTests/CleanFilterTests.swift | 29 ++++++++ .../MacCleanTests/TargetedScannerTests.swift | 69 +++++++++++++++++++ VERSION | 2 +- 10 files changed, 278 insertions(+), 26 deletions(-) create mode 100644 Sources/MacCleanKit/ScanOutcome.swift diff --git a/Sources/MacClean/Core/Scanner/TargetedScanner.swift b/Sources/MacClean/Core/Scanner/TargetedScanner.swift index ffc65f2..d5b24ed 100644 --- a/Sources/MacClean/Core/Scanner/TargetedScanner.swift +++ b/Sources/MacClean/Core/Scanner/TargetedScanner.swift @@ -1,4 +1,5 @@ import Foundation +import Darwin import MacCleanKit // `ScanTarget` moved to MacCleanKit for testability — see MacCleanKit/ScanTarget.swift. @@ -15,8 +16,21 @@ public actor TargetedScanner { public init() {} public func scan(targets: [ScanTarget]) async -> [FileItem] { + await scanReportingPermissions(targets: targets).items + } + + /// Like `scan(targets:)` but also reports which target roots were + /// unreadable due to a permission/TCC denial. Use this when the caller + /// needs to tell "found nothing" apart from "blocked by missing Full + /// Disk Access" (e.g. the Trash module — see `ScanOutcome`). + /// + /// Items are de-duplicated by URL across targets: overlapping targets + /// (e.g. `~` recursive *and* `~/Downloads` recursive) otherwise emit the + /// same file twice, which surfaced as duplicate UI rows and a + /// double-counted size estimate. + public func scanReportingPermissions(targets: [ScanTarget]) async -> ScanOutcome { let keys = resourceKeys - return await withTaskGroup(of: [FileItem].self) { group in + return await withTaskGroup(of: (items: [FileItem], deniedPath: URL?).self) { group in for target in targets { group.addTask { Self.scanTarget(target, keys: keys) @@ -24,18 +38,44 @@ public actor TargetedScanner { } var allItems: [FileItem] = [] - for await items in group { - allItems.append(contentsOf: items) + var seenURLs = Set() + var deniedPaths: [URL] = [] + for await result in group { + for item in result.items where seenURLs.insert(item.url).inserted { + allItems.append(item) + } + if let denied = result.deniedPath { + deniedPaths.append(denied) + } } - return allItems + return ScanOutcome(items: allItems, permissionDeniedPaths: deniedPaths) } } - private static func scanTarget(_ target: ScanTarget, keys: [URLResourceKey]) -> [FileItem] { + /// True if `url` exists as a directory whose contents can't be read + /// because of a permission/TCC denial (EPERM/EACCES). `open` faithfully + /// reproduces what enumeration would hit — and unlike enumeration, it + /// surfaces the errno instead of silently yielding nothing. O(1): no + /// directory listing. Returns false for readable dirs and for paths + /// that don't exist / aren't directories. + private static func isPermissionDenied(_ url: URL) -> Bool { + let fd = open(url.path(percentEncoded: false), O_RDONLY | O_DIRECTORY) + if fd >= 0 { close(fd); return false } + return errno == EPERM || errno == EACCES + } + + private static func scanTarget(_ target: ScanTarget, keys: [URLResourceKey]) -> (items: [FileItem], deniedPath: URL?) { let fm = FileManager.default guard fm.fileExists(atPath: target.path.path(percentEncoded: false)) else { - return [] + return ([], nil) + } + + // The enumerator/`contentsOfDirectory` below swallow EPERM and just + // yield nothing, so probe readability up front to distinguish a + // permission denial from a genuinely empty directory. + if isPermissionDenied(target.path) { + return ([], target.path) } var results: [FileItem] = [] @@ -45,7 +85,7 @@ public actor TargetedScanner { at: target.path, includingPropertiesForKeys: keys, options: [.skipsPackageDescendants] - ) else { return [] } + ) else { return ([], nil) } while let obj = enumerator.nextObject() { if Task.isCancelled { break } @@ -81,7 +121,7 @@ public actor TargetedScanner { guard let contents = try? fm.contentsOfDirectory( at: target.path, includingPropertiesForKeys: keys - ) else { return [] } + ) else { return ([], nil) } for fileURL in contents { if Task.isCancelled { break } @@ -92,7 +132,7 @@ public actor TargetedScanner { } } - return results + return (results, nil) } private static func matchesExcludePattern(url: URL, target: ScanTarget) -> Bool { diff --git a/Sources/MacClean/Modules/TrashBins/TrashBinsModule.swift b/Sources/MacClean/Modules/TrashBins/TrashBinsModule.swift index a30876a..45603eb 100644 --- a/Sources/MacClean/Modules/TrashBins/TrashBinsModule.swift +++ b/Sources/MacClean/Modules/TrashBins/TrashBinsModule.swift @@ -11,6 +11,25 @@ public struct TrashBinsModule: ScanModule { public init() {} public func scan() async -> [ScanResult] { + await scanReportingPermissions().results + } + + /// Same scan as `scan()`, but also reports whether the Trash came back + /// empty because Full Disk Access is missing (macOS blocks reading + /// `~/.Trash` without it, and the enumerator silently yields nothing). + /// The Trash view uses this to show a "Grant Full Disk Access" prompt + /// instead of a false "Trash is empty." + public func scanReportingPermissions() async -> (results: [ScanResult], permissionDenied: Bool) { + let outcome = await scanner.scanReportingPermissions(targets: Self.targets()) + guard !outcome.items.isEmpty else { + return ([], outcome.permissionDenied) + } + let results = [ScanResult(category: .trashBins, items: outcome.items)] + .filteringUncleanable() + return (results, outcome.permissionDenied) + } + + private static func targets() -> [ScanTarget] { var targets: [ScanTarget] = [ // Main user trash ScanTarget(path: MCConstants.userTrash, recursive: true), @@ -28,11 +47,6 @@ public struct TrashBinsModule: ScanModule { } } } - - let items = await scanner.scan(targets: targets) - guard !items.isEmpty else { return [] } - - return [ScanResult(category: .trashBins, items: items)] - .filteringUncleanable() + return targets } } diff --git a/Sources/MacClean/Views/Cleanup/TrashBinsView.swift b/Sources/MacClean/Views/Cleanup/TrashBinsView.swift index 6b4477f..4f49c4d 100644 --- a/Sources/MacClean/Views/Cleanup/TrashBinsView.swift +++ b/Sources/MacClean/Views/Cleanup/TrashBinsView.swift @@ -7,6 +7,7 @@ struct TrashBinsView: View { @State private var selectedItems: Set = [] @State private var isScanning = false @State private var scanComplete = false + @State private var permissionDenied = false @State private var completion: CleanSummary? @State private var cleaning: CleaningEngine.Progress? @State private var cleanTask: Task? @@ -27,10 +28,12 @@ struct TrashBinsView: View { scanComplete: scanComplete, completion: completion, cleaning: cleaning, + permissionDenied: permissionDenied, onScan: scan, onClean: clean, onCancelClean: { cleanTask?.cancel() }, - onReset: reset + onReset: reset, + onGrantAccess: { PermissionManager.shared.openFullDiskAccessSettings() } ) } @@ -40,6 +43,7 @@ struct TrashBinsView: View { private func scan() { isScanning = true scanComplete = false + permissionDenied = false scanProgress = 0 Task { let scanStart = Date() @@ -51,8 +55,10 @@ struct TrashBinsView: View { scanPhase = "Checking external drives..." scanProgress = 0.6 - async let scanTask = module.scan() - results = await scanTask + async let scanTask = module.scanReportingPermissions() + let outcome = await scanTask + results = outcome.results + permissionDenied = outcome.permissionDenied scanPhase = "Calculating sizes..." scanProgress = 0.9 @@ -97,6 +103,6 @@ struct TrashBinsView: View { } private func reset() { - results = []; selectedItems = []; completion = nil; cleaning = nil; cleanTask = nil; scanComplete = false + results = []; selectedItems = []; completion = nil; cleaning = nil; cleanTask = nil; scanComplete = false; permissionDenied = false } } diff --git a/Sources/MacClean/Views/Shared/ModuleContainerView.swift b/Sources/MacClean/Views/Shared/ModuleContainerView.swift index 130ed6d..6d140b1 100644 --- a/Sources/MacClean/Views/Shared/ModuleContainerView.swift +++ b/Sources/MacClean/Views/Shared/ModuleContainerView.swift @@ -21,10 +21,18 @@ struct ModuleContainerView: View { /// this window. nil means "no clean in flight" — either before any /// clean or after one finishes (then `completion` takes over). let cleaning: CleaningEngine.Progress? + /// True when the scan came back empty because a target couldn't be read + /// (Full Disk Access not granted) rather than because there was nothing + /// to clean. Drives the "Grant Full Disk Access" empty-state instead of + /// the misleading "nothing to clean up" one. + let permissionDenied: Bool let onScan: () -> Void let onClean: () -> Void let onCancelClean: (() -> Void)? let onReset: () -> Void + /// Opens System Settings → Full Disk Access. Required when + /// `permissionDenied` can be true; the empty-state button calls it. + let onGrantAccess: (() -> Void)? init( title: String, @@ -39,10 +47,12 @@ struct ModuleContainerView: View { scanComplete: Bool = false, completion: CleanSummary? = nil, cleaning: CleaningEngine.Progress? = nil, + permissionDenied: Bool = false, onScan: @escaping () -> Void, onClean: @escaping () -> Void, onCancelClean: (() -> Void)? = nil, - onReset: @escaping () -> Void + onReset: @escaping () -> Void, + onGrantAccess: (() -> Void)? = nil ) { self.title = title self.subtitle = subtitle @@ -56,19 +66,22 @@ struct ModuleContainerView: View { self.scanComplete = scanComplete self.completion = completion self.cleaning = cleaning + self.permissionDenied = permissionDenied self.onScan = onScan self.onClean = onClean self.onCancelClean = onCancelClean self.onReset = onReset + self.onGrantAccess = onGrantAccess } @State private var showLargeSelectionConfirm = false @State private var showActivityLog = false private var totalSelected: UInt64 { - results.flatMap(\.items) - .filter { selectedItems.contains($0.url) } - .reduce(0) { $0 + $1.size } + // Dedupe by URL: a file can appear in two categories (large + old) + // and Clean trashes it once, so summing per-item would over-report + // versus what actually gets freed. + results.selectedSize(selectedItems) } private var selectedCount: Int { @@ -90,7 +103,11 @@ struct ModuleContainerView: View { } else if isScanning { scanningView } else if scanComplete { - emptyResultsView + if permissionDenied { + permissionDeniedView + } else { + emptyResultsView + } } else { idleView } @@ -219,6 +236,43 @@ struct ModuleContainerView: View { } } + /// Shown when the scan came back empty only because macOS blocked the + /// read (Full Disk Access not granted). Without this the user saw + /// "nothing to clean up" over a Trash that was actually full. + private var permissionDeniedView: some View { + VStack(spacing: 18) { + Spacer() + Image(systemName: "lock.fill") + .font(.system(size: 52)) + .foregroundStyle(.white.opacity(0.9)) + Text("Full Disk Access needed") + .font(.system(size: 20, weight: .semibold)) + .foregroundStyle(.white) + Text("macOS is blocking access to this location. Grant Mac Clean Full Disk Access, then scan again.") + .font(.system(size: 13)) + .foregroundStyle(.white.opacity(0.7)) + .multilineTextAlignment(.center) + .frame(maxWidth: 360) + HStack(spacing: 10) { + if let onGrantAccess { + Button("Open Settings") { onGrantAccess() } + .buttonStyle(.borderedProminent) + .tint(.white) + .controlSize(.large) + } + Button("Rescan") { onScan() } + .buttonStyle(.bordered) + .tint(.white) + .controlSize(.large) + Button("Done") { onReset() } + .buttonStyle(.bordered) + .tint(.white) + .controlSize(.large) + } + Spacer() + } + } + private var resultsView: some View { VStack(spacing: 0) { HStack { diff --git a/Sources/MacCleanKit/CleanFilter.swift b/Sources/MacCleanKit/CleanFilter.swift index 8f33165..174a37f 100644 --- a/Sources/MacCleanKit/CleanFilter.swift +++ b/Sources/MacCleanKit/CleanFilter.swift @@ -71,4 +71,22 @@ public extension Array where Element == ScanResult { ) } } + + /// On-disk size of the user's current selection, counting each URL + /// exactly once. The same file can appear in more than one category + /// (a file that's both "large" and "old") — and Clean trashes each + /// path only once — so summing per-item would over-report the estimate + /// versus what actually gets freed. Dedupe by URL to keep the + /// "X will be freed" preview honest. + func selectedSize(_ selected: Set) -> UInt64 { + var counted = Set() + var total: UInt64 = 0 + for result in self { + for item in result.items + where selected.contains(item.url) && counted.insert(item.url).inserted { + total += item.size + } + } + return total + } } diff --git a/Sources/MacCleanKit/Constants.swift b/Sources/MacCleanKit/Constants.swift index 0436f62..4fc502b 100644 --- a/Sources/MacCleanKit/Constants.swift +++ b/Sources/MacCleanKit/Constants.swift @@ -159,5 +159,5 @@ public enum MCConstants { // plugin was tried (commit history) but doesn't work under multi-arch // `swift build --arch arm64 --arch x86_64` because xcbuild doesn't // execute plugins. - public static let appVersion = "1.5.2" + public static let appVersion = "1.5.3" } diff --git a/Sources/MacCleanKit/ScanOutcome.swift b/Sources/MacCleanKit/ScanOutcome.swift new file mode 100644 index 0000000..ee5a72f --- /dev/null +++ b/Sources/MacCleanKit/ScanOutcome.swift @@ -0,0 +1,22 @@ +import Foundation + +/// Result of a `TargetedScanner` run that, unlike a bare `[FileItem]`, +/// distinguishes "found nothing" from "couldn't read because access was +/// denied." `~/.Trash` without Full Disk Access enumerates as empty — +/// the EPERM is swallowed by `NSDirectoryEnumerator` — so a plain item +/// count can't tell the two apart. Carrying the denied target paths lets +/// the UI show a "Grant Full Disk Access" prompt instead of a misleading +/// "Trash is empty." +public struct ScanOutcome: Sendable { + public let items: [FileItem] + /// Target roots that exist but whose contents this process is not + /// permitted to read (EPERM/EACCES — TCC denial or POSIX permissions). + public let permissionDeniedPaths: [URL] + + public init(items: [FileItem], permissionDeniedPaths: [URL] = []) { + self.items = items + self.permissionDeniedPaths = permissionDeniedPaths + } + + public var permissionDenied: Bool { !permissionDeniedPaths.isEmpty } +} diff --git a/Tests/MacCleanKitTests/CleanFilterTests.swift b/Tests/MacCleanKitTests/CleanFilterTests.swift index d4851dc..ab0c734 100644 --- a/Tests/MacCleanKitTests/CleanFilterTests.swift +++ b/Tests/MacCleanKitTests/CleanFilterTests.swift @@ -109,6 +109,35 @@ final class CleanFilterTests: XCTestCase { [.posixPermissions: 0o755], ofItemAtPath: locked.path) } + // MARK: Selected-size estimate (must match what Clean actually frees) + + func testSelectedSizeCountsEachURLOnce() { + // The same file can surface in two categories (large AND old) or — + // before scanner dedup — from overlapping scan targets. Clean + // trashes each path once, so the pre-clean estimate must dedupe by + // URL too; otherwise it over-reports ("2 GB will be freed" but only + // 934 MB actually freed). + let url = URL(filePath: "/tmp/big.dmg") + let item = FileItem(url: url, name: "big.dmg", size: 1000, allocatedSize: 1000, isDirectory: false) + let results: [ScanResult] = [ + ScanResult(category: .largeFiles, items: [item]), + ScanResult(category: .oldFiles, items: [item]), + ] + + XCTAssertEqual(results.selectedSize([url]), 1000, + "a URL present in two results must count once") + } + + func testSelectedSizeSumsDistinctSelectedItems() { + let a = FileItem(url: URL(filePath: "/tmp/a.bin"), name: "a", size: 100, allocatedSize: 100, isDirectory: false) + let b = FileItem(url: URL(filePath: "/tmp/b.bin"), name: "b", size: 250, allocatedSize: 250, isDirectory: false) + let c = FileItem(url: URL(filePath: "/tmp/c.bin"), name: "c", size: 999, allocatedSize: 999, isDirectory: false) + let results = [ScanResult(category: .largeFiles, items: [a, b, c])] + + // Only a + b are selected. + XCTAssertEqual(results.selectedSize([a.url, b.url]), 350) + } + func testFilesInsideUnwritableDirectoryAreNotCleanable() throws { // Even individual files inside an unwritable parent should be // dropped — this is what catches every leaf inside a root-owned diff --git a/Tests/MacCleanTests/TargetedScannerTests.swift b/Tests/MacCleanTests/TargetedScannerTests.swift index e745a90..f10eee2 100644 --- a/Tests/MacCleanTests/TargetedScannerTests.swift +++ b/Tests/MacCleanTests/TargetedScannerTests.swift @@ -1,5 +1,6 @@ import XCTest import Foundation +import Darwin @testable import MacClean import MacCleanKit import MacCleanTestSupport @@ -97,4 +98,72 @@ final class TargetedScannerTests: XCTestCase { XCTAssertEqual(items.count, 2) } } + + // MARK: - Overlapping targets must not double-count (duplicate-row bug) + + /// Mirrors the real Large & Old Files bug: that module scans `~` + /// recursively *and* `~/Downloads` recursively, so every Downloads file + /// was enumerated twice and surfaced as two identical rows (and a + /// double-counted size estimate). Overlapping targets must emit each + /// URL exactly once. + func testOverlappingTargetsDeduplicateByURL() async throws { + try await TestFixtures.withTempDir { dir in + let downloads = dir.appending(path: "Downloads") + try TestFixtures.writeFile(at: downloads.appending(path: "big.dmg"), size: 100) + + let items = await TargetedScanner().scan(targets: [ + ScanTarget(path: dir, recursive: true), // parent sweep + ScanTarget(path: downloads, recursive: true), // explicit child — overlaps + ]) + + let dmgs = items.filter { $0.name == "big.dmg" } + XCTAssertEqual(dmgs.count, 1, + "overlapping targets must not emit the same file twice") + } + } + + func testSameTargetListedTwiceDeduplicates() async throws { + try await TestFixtures.withTempDir { dir in + try TestFixtures.writeFile(at: dir.appending(path: "a.log"), size: 1) + try TestFixtures.writeFile(at: dir.appending(path: "b.log"), size: 1) + + let target = ScanTarget(path: dir, recursive: false) + let items = await TargetedScanner().scan(targets: [target, target]) + XCTAssertEqual(items.count, 2) + } + } + + // MARK: - Permission denial is surfaced, not swallowed (#1) + + /// `~/.Trash` without Full Disk Access reads as "empty" because the + /// enumerator silently swallows EPERM. A chmod-000 directory reproduces + /// the same EACCES denial: the scan must report it as `permissionDenied` + /// rather than an indistinguishable empty result. + func testPermissionDeniedDirectoryIsReported() async throws { + try await TestFixtures.withTempDir { dir in + let locked = dir.appending(path: "locked") + try TestFixtures.writeFile(at: locked.appending(path: "secret.bin"), size: 10) + _ = chmod(locked.path(percentEncoded: false), 0o000) + defer { _ = chmod(locked.path(percentEncoded: false), 0o755) } // so cleanup can remove it + + let outcome = await TargetedScanner().scanReportingPermissions( + targets: [ScanTarget(path: locked, recursive: true)] + ) + + XCTAssertTrue(outcome.items.isEmpty) + XCTAssertTrue(outcome.permissionDenied, + "an unreadable directory must surface as permissionDenied") + XCTAssertEqual(outcome.permissionDeniedPaths, [locked]) + } + } + + func testReadableEmptyDirectoryIsNotPermissionDenied() async throws { + try await TestFixtures.withTempDir { dir in + let outcome = await TargetedScanner().scanReportingPermissions( + targets: [ScanTarget(path: dir, recursive: true)] + ) + XCTAssertFalse(outcome.permissionDenied, + "a readable but empty dir is not a permission problem") + } + } } diff --git a/VERSION b/VERSION index 4cda8f1..8af85be 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.5.2 +1.5.3