From 788262502d5a0209c15c5c3ebda46ad620403cd5 Mon Sep 17 00:00:00 2001 From: Robin Kutesa Date: Mon, 18 May 2026 19:33:26 +0300 Subject: [PATCH] fix: detect SMB Object Name Collision as already-safe, not failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SMBClient throws ErrorResponse with NTStatus.objectNameCollision (0xC0000035) when a file already exists on the NAS. BackupError.uploadFailed wraps this, hiding the real cause from the retry/failure logic. Add isAlreadyExistsError() that unwraps BackupError.uploadFailed and checks the underlying error for "object name collision", "already exist", or POSIX EEXIST (code 17). When detected inside the retry catch block, the asset is immediately marked as skipped and added to manifestEntries — no 3-retry penalty (saves up to 6s per already-existing file), no false failure count. The post-retry fileExists fallback is retained for edge cases where the file lands on NAS but the error wasn't a collision (e.g. connection reset after partial upload). Also switch BackupQueueItem.UploadError to store the underlying transport error's domain and description instead of the BackupError wrapper text, so the SyncView failure list shows the real reason (e.g. "Access Denied", "IO Timeout") rather than the generic "Failed to upload ". Co-Authored-By: Kutesir Co-Authored-By: Sentry --- Services/BackupEngine.swift | 69 ++++++++++++++++++++++++++++++++----- 1 file changed, 61 insertions(+), 8 deletions(-) diff --git a/Services/BackupEngine.swift b/Services/BackupEngine.swift index dad2ba3..32b3616 100644 --- a/Services/BackupEngine.swift +++ b/Services/BackupEngine.swift @@ -298,17 +298,38 @@ final class BackupEngine: ObservableObject { } } } catch { + // "Object Name Collision" (NTStatus 0xC0000035) means the file is already + // on the NAS. Retrying won't help — treat it as safe immediately. + if isAlreadyExistsError(error) { + job.fileCompleted(skipped: true) + skipped += 1 + if assetIdx < queueItems.count { + queueItems[assetIdx].status = .skipped + queueItems[assetIdx].completedAt = Date() + } + manifestEntries.append(ManifestEntry( + localIdentifier: asset.localIdentifier, + filename: asset.filename, + creationDate: asset.creationDate, + fileSize: 0, + remotePath: remotePath, + uploadedAt: Date() + )) + logger.info("Collision — file already on NAS, marking safe: \(asset.filename, privacy: .public)") + uploadSucceeded = true + break + } lastUploadError = error - logger.warning("Upload attempt \(attempt + 1)/\(maxRetries) failed for \(asset.filename, privacy: .public): \(error.localizedDescription, privacy: .public)") + logger.warning("Upload attempt \(attempt + 1)/\(maxRetries) failed for \(asset.filename, privacy: .public): \(underlyingDescription(error), privacy: .public)") } if uploadSucceeded { break } } if !uploadSucceeded, let error = lastUploadError { - // Before recording a failure, verify the file isn't already on NAS. - // fileExists() may have returned nil earlier due to a transient SMB error; - // if it's actually there, the asset is safe — count it as skipped, not failed. + // Last-resort check: maybe the file landed on NAS despite the errors + // (e.g. fileExists threw transiently on the pre-check and the upload + // actually succeeded before the connection reset). if (try? await transfer.fileExists(at: remotePath)) == true { job.fileCompleted(skipped: true) skipped += 1 @@ -324,17 +345,18 @@ final class BackupEngine: ObservableObject { remotePath: remotePath, uploadedAt: Date() )) - logger.info("Upload failed but file confirmed on NAS — marking safe: \(asset.filename, privacy: .public)") + logger.info("Post-retry check — file on NAS, marking safe: \(asset.filename, privacy: .public)") } else { - logger.error("Upload permanently failed for \(asset.filename, privacy: .public): \(error.localizedDescription, privacy: .public)") + let reason = underlyingDescription(error) + logger.error("Upload permanently failed for \(asset.filename, privacy: .public): \(reason, privacy: .public)") job.fileFailed() failed += 1 if assetIdx < queueItems.count { queueItems[assetIdx].status = .failed queueItems[assetIdx].completedAt = Date() queueItems[assetIdx].error = BackupQueueItem.UploadError( - code: (error as NSError).domain, - message: error.localizedDescription, + code: underlyingDomain(error), + message: reason, timestamp: Date() ) } @@ -403,6 +425,37 @@ final class BackupEngine: ObservableObject { } +// MARK: — Error classification helpers + +/// True when the error indicates the remote file already exists. +/// SMBClient surfaces this as ErrorResponse with NTStatus.objectNameCollision (0xC0000035), +/// whose localizedDescription is "Object Name Collision". +/// SFTP and POSIX use EEXIST (code 17) or the string "already exists". +private func isAlreadyExistsError(_ error: Error) -> Bool { + let base = unwrapUploadError(error) + let desc = base.localizedDescription.lowercased() + return desc.contains("object name collision") || + desc.contains("already exist") || + desc.contains("file exist") || + (base as NSError).code == 17 // POSIX EEXIST +} + +/// Returns the underlying transport error, unwrapping BackupError.uploadFailed if present. +private func unwrapUploadError(_ error: Error) -> Error { + if case let BackupError.uploadFailed(_, underlying) = error { return underlying } + return error +} + +/// Human-readable description from the underlying transport error, not the wrapper. +private func underlyingDescription(_ error: Error) -> String { + unwrapUploadError(error).localizedDescription +} + +/// NSError domain from the underlying transport error. +private func underlyingDomain(_ error: Error) -> String { + (unwrapUploadError(error) as NSError).domain +} + // MARK: — SpeedTracker private struct SpeedTracker {