fix: detect SMB Object Name Collision as already-safe, not failure

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 <file>".

Co-Authored-By: Kutesir <kutesir@provoc.ug>
Co-Authored-By: Sentry <sentry@provoc.ug>
This commit is contained in:
Robin Kutesa
2026-05-18 19:33:26 +03:00
parent 8d8b6c4418
commit 788262502d

View File

@@ -298,17 +298,38 @@ final class BackupEngine: ObservableObject {
} }
} }
} catch { } 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 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 { break }
} }
if !uploadSucceeded, let error = lastUploadError { if !uploadSucceeded, let error = lastUploadError {
// Before recording a failure, verify the file isn't already on NAS. // Last-resort check: maybe the file landed on NAS despite the errors
// fileExists() may have returned nil earlier due to a transient SMB error; // (e.g. fileExists threw transiently on the pre-check and the upload
// if it's actually there, the asset is safe count it as skipped, not failed. // actually succeeded before the connection reset).
if (try? await transfer.fileExists(at: remotePath)) == true { if (try? await transfer.fileExists(at: remotePath)) == true {
job.fileCompleted(skipped: true) job.fileCompleted(skipped: true)
skipped += 1 skipped += 1
@@ -324,17 +345,18 @@ final class BackupEngine: ObservableObject {
remotePath: remotePath, remotePath: remotePath,
uploadedAt: Date() 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 { } 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() job.fileFailed()
failed += 1 failed += 1
if assetIdx < queueItems.count { if assetIdx < queueItems.count {
queueItems[assetIdx].status = .failed queueItems[assetIdx].status = .failed
queueItems[assetIdx].completedAt = Date() queueItems[assetIdx].completedAt = Date()
queueItems[assetIdx].error = BackupQueueItem.UploadError( queueItems[assetIdx].error = BackupQueueItem.UploadError(
code: (error as NSError).domain, code: underlyingDomain(error),
message: error.localizedDescription, message: reason,
timestamp: Date() 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 // MARK: SpeedTracker
private struct SpeedTracker { private struct SpeedTracker {