From 0e65a548f26efa1974ff427a7ada32ec586f5130 Mon Sep 17 00:00:00 2001 From: Robin Kutesa Date: Mon, 18 May 2026 20:41:49 +0300 Subject: [PATCH] fix: NAS operation timeouts, fast-path warm-up, 30-min stale threshold MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three changes that together eliminate the "Checking…" stall and excessive NAS rescanning without requiring a database migration: NASManifestCache: extend stale threshold from 5 min to 30 min. directoryCount is now updated incrementally after every upload, and PHPhotoLibraryChangeObserver keeps LocalPhotoIndex current — a full NAS roundtrip every 5 min was redundant and forced the expensive reconcile path far more often than necessary. BackupStatusService: add withTimeout(_:work:) using a racing ThrowingTaskGroup so NAS operations (SMB connect, manifest download, directory listing) fail fast instead of hanging indefinitely when the NAS is slow or unreachable. Timeouts: 15s for connect, 12s for manifest download, 12s for directory listings. On timeout, BackupError.timeout is caught separately to preserve cached counts instead of marking connectionState = .offline. BackupStatusService: call LocalPhotoIndex.shared.loadOrBuild() at the start of performRefresh before checking localIndexCount. Without this, AppDelegate's concurrent loadOrBuild() task races with the first onActive() trigger; if it loses, localIndexCount == 0 and the fast path is bypassed, forcing a full NAS roundtrip on every cold launch. Co-Authored-By: Kutesir Co-Authored-By: Sentry --- Services/BackupStatusService.swift | 56 +++++++++++++++++++++++++----- Services/NASManifestCache.swift | 5 ++- 2 files changed, 52 insertions(+), 9 deletions(-) diff --git a/Services/BackupStatusService.swift b/Services/BackupStatusService.swift index 965a5d7..6178193 100644 --- a/Services/BackupStatusService.swift +++ b/Services/BackupStatusService.swift @@ -100,6 +100,11 @@ final class BackupStatusService: NSObject, ObservableObject { return } + // Ensure LocalPhotoIndex is loaded from disk before reconcile. + // loadOrBuild() is a fast no-op when already in memory; on cold launch it + // reads the cached JSON so the fast path (skip NAS roundtrip) can fire. + await LocalPhotoIndex.shared.loadOrBuild() + lastFullRefresh = Date() isRefreshing = true refreshError = nil @@ -110,6 +115,13 @@ final class BackupStatusService: NSObject, ObservableObject { log.info("Refresh completed — phone=\(self.snapshot.phoneTotal) safe=\(self.snapshot.alreadySafe)") } catch is CancellationError { log.info("Refresh cancelled") + } catch BackupError.timeout { + // NAS did not respond within the timeout window. + // Retain cached counts so the UI shows useful data instead of zeroes. + log.warning("Refresh timed out — retaining cached state") + refreshError = "NAS not responding — using last known state" + snapshot.lastCheckedAt = Date() + saveSnapshot() } catch { log.error("Refresh failed: \(error.localizedDescription)") refreshError = error.localizedDescription @@ -179,12 +191,19 @@ final class BackupStatusService: NSObject, ObservableObject { // ── Step 2: NAS connection ──────────────────────────────────────── let transfer: any NASTransferProtocol = conn.nasProtocol == .smb ? SMBService() : SFTPService() do { - try await transfer.connect( - to: conn.host, port: conn.port, - username: conn.username, password: conn.password - ) + try await Self.withTimeout(15) { + try await transfer.connect( + to: conn.host, port: conn.port, + username: conn.username, password: conn.password + ) + } } catch { - let reason = (error as? BackupError).flatMap { $0.errorDescription } ?? error.localizedDescription + let reason: String + if case BackupError.timeout = error { + reason = "NAS not responding — using last known state" + } else { + reason = (error as? BackupError).flatMap { $0.errorDescription } ?? error.localizedDescription + } log.warning("NAS connect failed: \(reason, privacy: .public) — retaining cached counts") refreshError = reason @@ -262,7 +281,7 @@ final class BackupStatusService: NSObject, ObservableObject { ) async -> ManifestIndex { log.info("buildManifestIndex: remotePath=\(conn.remotePath, privacy: .public) manifestFile=\(path, privacy: .public)") do { - let data = try await transfer.downloadData(at: path) + let data = try await Self.withTimeout(12) { try await transfer.downloadData(at: path) } log.info("buildManifestIndex: manifest downloaded — \(data.count) bytes") // Decode and build index off main actor — JSONDecoder is not cheap for large manifests. let decoded: (ManifestIndex, BackupManifest)? = await Task.detached(priority: .utility) { @@ -279,7 +298,7 @@ final class BackupStatusService: NSObject, ObservableObject { // created. Scan the directory to get the real file count for the NAS Archive // display — but do NOT add directory files as orphan manifest entries. if index.totalCount <= 1 { - let items = (try? await transfer.listDirectory(at: conn.remotePath)) ?? [] + let items = (try? await Self.withTimeout(12) { try await transfer.listDirectory(at: conn.remotePath) }) ?? [] let dirCount = items.filter { !$0.isDirectory && !$0.name.hasPrefix(".") && $0.name != BackupManifest.remoteFilename }.count @@ -297,7 +316,7 @@ final class BackupStatusService: NSObject, ObservableObject { } // Bootstrap: list NAS directory and build filename-only index in background. - let items = (try? await transfer.listDirectory(at: conn.remotePath)) ?? [] + let items = (try? await Self.withTimeout(12) { try await transfer.listDirectory(at: conn.remotePath) }) ?? [] log.info("buildManifestIndex: bootstrap from \(items.count) items in \(conn.remotePath, privacy: .public)") let bootstrapIndex = await Task.detached(priority: .utility) { ManifestIndex(nasListing: items) @@ -424,6 +443,27 @@ final class BackupStatusService: NSObject, ObservableObject { } } + // MARK: — Timeout helper + + /// Runs `work` and throws `BackupError.timeout` if it doesn't finish within `seconds`. + /// Uses a racing task group so the timeout fires even when the underlying SMB/SFTP + /// stack doesn't propagate Swift cooperative cancellation. + private nonisolated static func withTimeout( + _ seconds: TimeInterval, + work: @Sendable @escaping () async throws -> T + ) async throws -> T { + try await withThrowingTaskGroup(of: T.self) { group in + group.addTask(priority: .userInitiated) { try await work() } + group.addTask(priority: .utility) { + try await Task.sleep(nanoseconds: UInt64(seconds * 1_000_000_000)) + throw BackupError.timeout + } + defer { group.cancelAll() } + guard let result = try await group.next() else { throw BackupError.timeout } + return result + } + } + // MARK: — Cache private func loadCachedSnapshot() { diff --git a/Services/NASManifestCache.swift b/Services/NASManifestCache.swift index 65efbea..26c1b96 100644 --- a/Services/NASManifestCache.swift +++ b/Services/NASManifestCache.swift @@ -14,7 +14,10 @@ actor NASManifestCache { private var cached: CachedEntry? private let diskURL: URL - private static let staleInterval: TimeInterval = 5 * 60 + // 30-minute window: directoryCount is updated incrementally after every upload, + // and PHPhotoLibraryChangeObserver keeps LocalPhotoIndex current. A full NAS + // roundtrip every 5 minutes was unnecessary and caused "Checking…" stalls. + private static let staleInterval: TimeInterval = 30 * 60 private struct CachedEntry: Codable { var manifest: BackupManifest