From c3d6e6d0127e97a8b342a6aef7ed0a0a18665512 Mon Sep 17 00:00:00 2001 From: Camila Ayres Date: Mon, 6 Jul 2026 13:41:50 +0200 Subject: [PATCH 1/3] fix: prevent data loss when parent folder moves during sync. Items with pending uploads and local origin lock files are protected from deletion in the recursive directory delete path, so a moved or renamed parent no longer wipes unsynced children. Pending upload items are also skipped in the enumerator deletion pass, and reported changes are sorted so parents come before children to avoid duplicate folders on rename. Backport of nextcloud/desktop#10134, adapted to the enumerator architecture of the 3.2.x line. The 404 child deferral and move reconciliation logic from master do not apply here. Assisted-by: Claude:claude-opus-4-8 Signed-off-by: Camila Ayres --- .../FilesDatabaseManager+Directories.swift | 13 + .../Enumeration/Enumerator.swift | 11 + .../Item/Item+Create.swift | 6 + .../MoveSafeDeletionTests.swift | 264 ++++++++++++++++++ 4 files changed, 294 insertions(+) create mode 100644 Tests/NextcloudFileProviderKitTests/MoveSafeDeletionTests.swift diff --git a/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager+Directories.swift b/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager+Directories.swift index 1e18e44a..f4ba3cc8 100644 --- a/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager+Directories.swift +++ b/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager+Directories.swift @@ -74,7 +74,20 @@ public extension FilesDatabaseManager { $0.account == directoryAccount && $0.serverUrl.starts(with: directoryUrlPath) } + // Protect items whose local data is not yet on the server from deletion when their + // parent is removed. Otherwise a moved or renamed parent would wipe unsynced children. + // TODO: the parent directory itself is still deleted here, so a protected child is + // orphaned once its upload completes. Follow up by deferring parent deletion or + // reparenting the child after the upload finishes. for result in results { + if result.status >= Status.inUpload.rawValue { + logger.info("Skipping deletion of child with pending upload.", [.item: result.ocId]) + continue + } + if result.isLockFileOfLocalOrigin { + logger.info("Skipping deletion of local origin lock file during directory delete.", [.item: result.ocId, .name: result.fileName]) + continue + } let inactiveItemMetadata = SendableItemMetadata(value: result) do { try database.write { result.deleted = true } diff --git a/Sources/NextcloudFileProviderKit/Enumeration/Enumerator.swift b/Sources/NextcloudFileProviderKit/Enumeration/Enumerator.swift index e8820c97..e93a0c7a 100644 --- a/Sources/NextcloudFileProviderKit/Enumeration/Enumerator.swift +++ b/Sources/NextcloudFileProviderKit/Enumeration/Enumerator.swift @@ -492,6 +492,17 @@ public final class Enumerator: NSObject, NSFileProviderEnumerator, Sendable { allDeletedMetadatas = deletedMetadatas } + // Protect items with pending uploads: their local data is not yet on the server. + allDeletedMetadatas.removeAll { deletedMetadata in + guard deletedMetadata.status >= Status.inUpload.rawValue else { return false } + logger.info("Skipping deletion of item with pending upload.", [.item: deletedMetadata.ocId]) + return true + } + + // Report parents before children so macOS creates a moved directory before placing its + // contents, preventing both the old and new folder name from appearing during a rename. + allUpdatedMetadatas.sort { $0.remotePath().count < $1.remotePath().count } + let allFpItemDeletionsIdentifiers = Array( allDeletedMetadatas.map { NSFileProviderItemIdentifier($0.ocId) }) if !allFpItemDeletionsIdentifiers.isEmpty { diff --git a/Sources/NextcloudFileProviderKit/Item/Item+Create.swift b/Sources/NextcloudFileProviderKit/Item/Item+Create.swift index 6c457ee9..1ac59b15 100644 --- a/Sources/NextcloudFileProviderKit/Item/Item+Create.swift +++ b/Sources/NextcloudFileProviderKit/Item/Item+Create.swift @@ -23,6 +23,12 @@ public extension Item { ) async -> (Item?, Error?) { let logger = FileProviderLogger(category: "Item", log: log) + // Note: when a parent folder is renamed on another client and an editor has a file open + // inside it, the editor may recreate the old folder here, producing a server side duplicate. + // NSFilePresenter callbacks are only delivered when the writer uses NSFileCoordinator. + // It is not confirmed whether the File Provider daemon already does this internally when + // processing didUpdate renames. If it does not, wrapping rename propagation in an + // NSFileCoordinator coordinated write would notify registered presenters. let (_, _, _, createError) = await remoteInterface.createFolder( remotePath: remotePath, account: account, options: .init(), taskHandler: { task in if let domain, let itemTemplate { diff --git a/Tests/NextcloudFileProviderKitTests/MoveSafeDeletionTests.swift b/Tests/NextcloudFileProviderKitTests/MoveSafeDeletionTests.swift new file mode 100644 index 00000000..265eef7a --- /dev/null +++ b/Tests/NextcloudFileProviderKitTests/MoveSafeDeletionTests.swift @@ -0,0 +1,264 @@ +// SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors +// SPDX-License-Identifier: LGPL-3.0-or-later + +@preconcurrency import FileProvider +import Foundation +@testable import NextcloudFileProviderKit +import NextcloudFileProviderKitMocks +import RealmSwift +import TestInterface +import XCTest + +// MARK: - Move-safe deletion + +final class MoveSafeDeletionTests: NextcloudFileProviderKitTestCase { + static let account = Account( + user: "testUser", id: "testUserId", serverUrl: "https://mock.nc.com", password: "abcd" + ) + + static let dbManager = FilesDatabaseManager( + account: account, + databaseDirectory: makeDatabaseDirectory(), + fileProviderDomainIdentifier: NSFileProviderDomainIdentifier("test"), + log: FileProviderLogMock() + ) + + override func setUp() { + super.setUp() + Realm.Configuration.defaultConfiguration.inMemoryIdentifier = name + } + + func testDeleteDirectorySkipsChildrenWithPendingUpload() throws { + let dir = RealmItemMetadata() + dir.ocId = "upload-dir" + dir.account = "TestAccount" + dir.serverUrl = "https://cloud.example.com/files" + dir.fileName = "uploads" + dir.directory = true + + let normalChild = RealmItemMetadata() + normalChild.ocId = "normal-child" + normalChild.account = "TestAccount" + normalChild.serverUrl = "https://cloud.example.com/files/uploads" + normalChild.fileName = "synced.txt" + normalChild.status = Status.normal.rawValue + + let uploadingChild = RealmItemMetadata() + uploadingChild.ocId = "uploading-child" + uploadingChild.account = "TestAccount" + uploadingChild.serverUrl = "https://cloud.example.com/files/uploads" + uploadingChild.fileName = "uploading.txt" + uploadingChild.status = Status.uploading.rawValue + + let inUploadChild = RealmItemMetadata() + inUploadChild.ocId = "inupload-child" + inUploadChild.account = "TestAccount" + inUploadChild.serverUrl = "https://cloud.example.com/files/uploads" + inUploadChild.fileName = "queued.txt" + inUploadChild.status = Status.inUpload.rawValue + + let realm = Self.dbManager.ncDatabase() + try realm.write { + realm.add(dir) + realm.add(normalChild) + realm.add(uploadingChild) + realm.add(inUploadChild) + } + + let deleted = Self.dbManager.deleteDirectoryAndSubdirectoriesMetadata( + ocId: "upload-dir" + ) + + XCTAssertNotNil(deleted) + let deletedOcIds = deleted?.map(\.ocId) ?? [] + XCTAssertTrue(deletedOcIds.contains("upload-dir"), "Directory itself should be deleted") + XCTAssertTrue(deletedOcIds.contains("normal-child"), "Normal child should be deleted") + XCTAssertFalse( + deletedOcIds.contains("uploading-child"), + "Child with uploading status should be skipped" + ) + XCTAssertFalse( + deletedOcIds.contains("inupload-child"), + "Child with inUpload status should be skipped" + ) + + let uploadingItem = Self.dbManager.itemMetadata(ocId: "uploading-child") + XCTAssertNotNil(uploadingItem) + XCTAssertFalse( + uploadingItem?.deleted ?? true, + "Uploading child should not be marked as deleted" + ) + } + + /// `uploadError (4)` satisfies `status >= inUpload (2)`, so items that failed + /// to upload are preserved just like items that are actively uploading. + /// This keeps the upload error state visible to the user rather than losing it silently. + func testDeleteDirectorySkipsChildrenWithUploadError() throws { + let dir = RealmItemMetadata() + dir.ocId = "uperr-dir" + dir.account = "TestAccount" + dir.serverUrl = "https://cloud.example.com/files" + dir.fileName = "work" + dir.directory = true + + let uploadErrorChild = RealmItemMetadata() + uploadErrorChild.ocId = "uperr-child" + uploadErrorChild.account = "TestAccount" + uploadErrorChild.serverUrl = "https://cloud.example.com/files/work" + uploadErrorChild.fileName = "failed.txt" + uploadErrorChild.status = Status.uploadError.rawValue + + let realm = Self.dbManager.ncDatabase() + try realm.write { + realm.add(dir) + realm.add(uploadErrorChild) + } + + let deleted = Self.dbManager.deleteDirectoryAndSubdirectoriesMetadata(ocId: "uperr-dir") + + XCTAssertNotNil(deleted) + let deletedOcIds = deleted?.map(\.ocId) ?? [] + XCTAssertFalse( + deletedOcIds.contains("uperr-child"), + "Child with uploadError status must be skipped since status >= inUpload protects it." + ) + + let survivingChild = Self.dbManager.itemMetadata(ocId: "uperr-child") + XCTAssertNotNil(survivingChild) + XCTAssertFalse(survivingChild?.deleted ?? true) + } + + func testDeleteDirectoryDeletesChildrenWithDownloadError() throws { + let dir = RealmItemMetadata() + dir.ocId = "dl-err-dir" + dir.account = "TestAccount" + dir.serverUrl = "https://cloud.example.com/files" + dir.fileName = "errors" + dir.directory = true + + let dlErrorChild = RealmItemMetadata() + dlErrorChild.ocId = "dl-err-child" + dlErrorChild.account = "TestAccount" + dlErrorChild.serverUrl = "https://cloud.example.com/files/errors" + dlErrorChild.fileName = "broken.pdf" + dlErrorChild.status = Status.downloadError.rawValue + + let realm = Self.dbManager.ncDatabase() + try realm.write { + realm.add(dir) + realm.add(dlErrorChild) + } + + let deleted = Self.dbManager.deleteDirectoryAndSubdirectoriesMetadata( + ocId: "dl-err-dir" + ) + + XCTAssertNotNil(deleted) + let deletedOcIds = deleted?.map(\.ocId) ?? [] + XCTAssertTrue( + deletedOcIds.contains("dl-err-child"), + "Download error items should still be deleted; only upload status items are protected." + ) + } + + func testDeleteDirectorySkipsLocalOriginLockFile() throws { + let dir = RealmItemMetadata() + dir.ocId = "lock-del-dir" + dir.account = "TestAccount" + dir.serverUrl = "https://cloud.example.com/files" + dir.fileName = "work" + dir.directory = true + + let normalChild = RealmItemMetadata() + normalChild.ocId = "lock-del-normal" + normalChild.account = "TestAccount" + normalChild.serverUrl = "https://cloud.example.com/files/work" + normalChild.fileName = "report.docx" + normalChild.status = Status.normal.rawValue + + let lockFile = RealmItemMetadata() + lockFile.ocId = "lock-del-lockfile" + lockFile.account = "TestAccount" + lockFile.serverUrl = "https://cloud.example.com/files/work" + lockFile.fileName = ".~lock.report.docx#" + lockFile.isLockFileOfLocalOrigin = true + + let realm = Self.dbManager.ncDatabase() + try realm.write { + realm.add(dir) + realm.add(normalChild) + realm.add(lockFile) + } + + let deleted = Self.dbManager.deleteDirectoryAndSubdirectoriesMetadata(ocId: "lock-del-dir") + + XCTAssertNotNil(deleted) + let deletedOcIds = deleted?.map(\.ocId) ?? [] + XCTAssertTrue(deletedOcIds.contains("lock-del-dir"), "Directory itself must be deleted.") + XCTAssertTrue(deletedOcIds.contains("lock-del-normal"), "Normal child must be deleted.") + XCTAssertFalse( + deletedOcIds.contains("lock-del-lockfile"), + "Local origin lock file must be preserved since the editor still has it open." + ) + + let survivingLock = Self.dbManager.itemMetadata(ocId: "lock-del-lockfile") + XCTAssertNotNil(survivingLock) + XCTAssertFalse(survivingLock?.deleted ?? true) + } + + /// When a folder rename and its children are both pending in the working set change + /// list, macOS processes items in the order the extension reports them. If a child + /// arrives before its parent's rename, macOS creates the destination folder to house + /// the child, then cannot complete the parent rename because the destination already + /// exists, leaving both the old and new folder name visible on disk simultaneously. + /// + /// Regression test: verify that pendingWorkingSetChanges output, when sorted by the + /// fix applied in completeChangesObserver, places parent directories before children. + func testPendingChangesAreSortedParentBeforeChild() throws { + let now = Date() + let recentSync = now.addingTimeInterval(-60) + let anchorDate = now.addingTimeInterval(-300) + + // Parent directory and child file both have syncTime newer than the anchor so + // they appear in pendingWorkingSetChanges. + var dirMeta = SendableItemMetadata( + ocId: "sort-dir", fileName: "2026-renamed", account: Self.account + ) + dirMeta.serverUrl = Self.account.davFilesUrl + "/container" + dirMeta.directory = true + dirMeta.visitedDirectory = true + dirMeta.etag = "V2" + dirMeta.uploaded = true + dirMeta.syncTime = recentSync + Self.dbManager.addItemMetadata(dirMeta) + + var fileMeta = SendableItemMetadata( + ocId: "sort-file", fileName: "Spreadsheet.xlsx", account: Self.account + ) + fileMeta.serverUrl = Self.account.davFilesUrl + "/container/2026-renamed" + fileMeta.downloaded = true + fileMeta.uploaded = true + fileMeta.etag = "V2" + fileMeta.syncTime = recentSync + Self.dbManager.addItemMetadata(fileMeta) + + let pending = Self.dbManager.pendingWorkingSetChanges( + account: Self.account, since: anchorDate + ) + + // Both items must be in the pending list. + XCTAssertTrue(pending.updated.contains(where: { $0.ocId == "sort-dir" })) + XCTAssertTrue(pending.updated.contains(where: { $0.ocId == "sort-file" })) + + // Apply the same sort that completeChangesObserver uses before reporting to macOS. + let sorted = pending.updated.sorted { $0.remotePath().count < $1.remotePath().count } + + let dirIndex = try XCTUnwrap(sorted.firstIndex(where: { $0.ocId == "sort-dir" })) + let fileIndex = try XCTUnwrap(sorted.firstIndex(where: { $0.ocId == "sort-file" })) + + XCTAssertLessThan( + dirIndex, fileIndex, + "Parent directory must sort before its children to prevent duplicate folders on rename." + ) + } +} From 6e93c1f8c841f3b7021d29d0138633d5dc52ff69 Mon Sep 17 00:00:00 2001 From: Camila Ayres Date: Tue, 7 Jul 2026 22:31:02 +0200 Subject: [PATCH 2/3] fix(interface): adapt NextcloudKit extension to 7.3.x API. NextcloudKit 7.3.x changed several signatures: upload and download now report a raw response, uploadChunk was replaced by uploadChunkAsync, and NKFile.tags became [NKTag]. Parse the metadata callers expect from the response headers, provide a conforming downloadAsync wrapper, and switch chunked upload to uploadChunkAsync, keeping the RemoteInterface protocol and all call sites unchanged. This makes the 3.2.x line build against NextcloudKit 7.3.x, which CI resolves. Assisted-by: Claude:claude-opus-4-8 Signed-off-by: Camila Ayres --- .../Extensions/NKFile+Extensions.swift | 2 +- .../NextcloudKit+RemoteInterface.swift | 97 ++++++++++++++++--- 2 files changed, 83 insertions(+), 16 deletions(-) diff --git a/Sources/NextcloudFileProviderKit/Extensions/NKFile+Extensions.swift b/Sources/NextcloudFileProviderKit/Extensions/NKFile+Extensions.swift index cf0fc4fc..e1b46edc 100644 --- a/Sources/NextcloudFileProviderKit/Extensions/NKFile+Extensions.swift +++ b/Sources/NextcloudFileProviderKit/Extensions/NKFile+Extensions.swift @@ -80,7 +80,7 @@ extension NKFile { sharePermissionsCloudMesh: sharePermissionsCloudMesh, shareType: shareType, size: size, - tags: tags, + tags: tags.map(\.name), uploaded: uploaded, trashbinFileName: trashbinFileName, trashbinOriginalLocation: trashbinOriginalLocation, diff --git a/Sources/NextcloudFileProviderKit/Interface/NextcloudKit+RemoteInterface.swift b/Sources/NextcloudFileProviderKit/Interface/NextcloudKit+RemoteInterface.swift index 6e89fa53..0477e625 100644 --- a/Sources/NextcloudFileProviderKit/Interface/NextcloudKit+RemoteInterface.swift +++ b/Sources/NextcloudFileProviderKit/Interface/NextcloudKit+RemoteInterface.swift @@ -60,7 +60,17 @@ extension NextcloudKit: RemoteInterface { requestHandler: requestHandler, taskHandler: taskHandler, progressHandler: progressHandler - ) { account, ocId, etag, date, size, response, nkError in + ) { account, response, nkError in + let allHeaderFields = response?.response?.allHeaderFields + let ocId = self.nkCommonInstance.findHeader("oc-fileid", allHeaderFields: allHeaderFields) + let etag = self.nkCommonInstance.normalizedETag(self.nkCommonInstance.findHeader("oc-etag", allHeaderFields: allHeaderFields)) + let date = self.nkCommonInstance.findHeader("date", allHeaderFields: allHeaderFields)?.parsedDate(using: "EEE, dd MMM y HH:mm:ss zzz") + var size: Int64 = 0 + + if let value = allHeaderFields?["Content-Length"] as? String { + size = Int64(value) ?? 0 + } + continuation.resume(returning: ( account, ocId, @@ -88,7 +98,7 @@ extension NextcloudKit: RemoteInterface { chunkCounter: @escaping (_ counter: Int) -> Void = { _ in }, log: any FileProviderLogging, chunkUploadStartHandler: @escaping (_ filesChunk: [RemoteFileChunk]) -> Void = { _ in }, - requestHandler: @escaping (_ request: UploadRequest) -> Void = { _ in }, + requestHandler _: @escaping (_ request: UploadRequest) -> Void = { _ in }, taskHandler: @escaping (_ task: URLSessionTask) -> Void = { _ in }, progressHandler: @escaping (Progress) -> Void = { _ in }, chunkUploadCompleteHandler: @escaping (_ fileChunk: RemoteFileChunk) -> Void = { _ in } @@ -154,8 +164,10 @@ extension NextcloudKit: RemoteInterface { """ ) - return await withCheckedContinuation { continuation in - uploadChunk( + var startedChunks: [RemoteFileChunk] = [] + + do { + let (uploadAccount, file) = try await uploadChunkAsync( directory: directory, fileChunksOutputDirectory: fileChunksOutputDirectory, fileName: fileName, @@ -168,17 +180,19 @@ extension NextcloudKit: RemoteInterface { chunkSize: chunkSize, account: account.ncKitAccount, options: options, - numChunks: currentNumChunksUpdateHandler, - counterChunk: chunkCounter, - start: { processedChunks in + chunkProgressHandler: { total, counter in + currentNumChunksUpdateHandler(total) + chunkCounter(counter) + }, + uploadStart: { processedChunks in let chunks = RemoteFileChunk.fromNcKitChunks( processedChunks, remoteChunkStoreFolderName: remoteChunkStoreFolderName ) + startedChunks = chunks chunkUploadStartHandler(chunks) }, - requestHandler: requestHandler, - taskHandler: taskHandler, - progressHandler: { totalBytesExpected, totalBytes, _ in + uploadTaskHandler: taskHandler, + uploadProgressHandler: { totalBytesExpected, totalBytes, _ in let currentProgress = Progress(totalUnitCount: totalBytesExpected) currentProgress.completedUnitCount = totalBytes progressHandler(currentProgress) @@ -190,11 +204,64 @@ extension NextcloudKit: RemoteInterface { ) chunkUploadCompleteHandler(chunk) } - ) { account, receivedChunks, file, error in - let chunks = RemoteFileChunk.fromNcKitChunks( - receivedChunks ?? [], remoteChunkStoreFolderName: remoteChunkStoreFolderName - ) - continuation.resume(returning: (account, chunks, file, error)) + ) + + return (uploadAccount, startedChunks, file, .success) + } catch let nkError as NKError { + return (account.ncKitAccount, nil, nil, nkError) + } catch { + return (account.ncKitAccount, nil, nil, .urlError) + } + } + + /// NextcloudKit 7.3.x changed its own downloadAsync to return the raw AFDownloadResponse, + /// which no longer matches RemoteInterface. Provide a conforming wrapper that parses the + /// metadata the callers expect from the response headers. + public func downloadAsync( + serverUrlFileName: Any, + fileNameLocalPath: String, + account: String, + options: NKRequestOptions = .init(), + requestHandler: @escaping (_ request: DownloadRequest) -> Void = { _ in }, + taskHandler: @Sendable @escaping (_ task: URLSessionTask) -> Void = { _ in }, + progressHandler: @escaping (_ progress: Progress) -> Void = { _ in } + ) async -> ( + account: String, + etag: String?, + date: Date?, + length: Int64, + headers: [AnyHashable: any Sendable]?, + afError: AFError?, + nkError: NKError + ) { + await withCheckedContinuation { continuation in + download( + serverUrlFileName: serverUrlFileName, + fileNameLocalPath: fileNameLocalPath, + account: account, + options: options, + requestHandler: requestHandler, + taskHandler: taskHandler, + progressHandler: progressHandler + ) { account, response, nkError in + let allHeaderFields = response?.response?.allHeaderFields + let etag = self.nkCommonInstance.normalizedETag(self.nkCommonInstance.findHeader("oc-etag", allHeaderFields: allHeaderFields)) + let date = self.nkCommonInstance.findHeader("date", allHeaderFields: allHeaderFields)?.parsedDate(using: "EEE, dd MMM y HH:mm:ss zzz") + var length: Int64 = 0 + + if let value = allHeaderFields?["Content-Length"] as? String { + length = Int64(value) ?? 0 + } + + continuation.resume(returning: ( + account, + etag, + date, + length, + allHeaderFields as? [AnyHashable: any Sendable], + response?.error, + nkError + )) } } } From ac1f5717169f6c2ddfecbc0f49945951b4e935cc Mon Sep 17 00:00:00 2001 From: Camila Ayres Date: Tue, 7 Jul 2026 22:51:36 +0200 Subject: [PATCH 3/3] fix(test): add version to capability mocks NextcloudCapabilitiesKit 2.5.x requires ocs.data.version, so the inline capability JSONs without it made Capabilities(data:) return nil and the force unwrap in capabilitiesFromMockJSON crashed the test run. Add a minimal version block to each mock. Assisted-by: Claude:claude-opus-4-8 Signed-off-by: Camila Ayres --- .../RemoteInterfaceTests.swift | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/Tests/NextcloudFileProviderKitTests/RemoteInterfaceTests.swift b/Tests/NextcloudFileProviderKitTests/RemoteInterfaceTests.swift index d73ddef5..82676c5d 100644 --- a/Tests/NextcloudFileProviderKitTests/RemoteInterfaceTests.swift +++ b/Tests/NextcloudFileProviderKitTests/RemoteInterfaceTests.swift @@ -81,6 +81,11 @@ struct RemoteInterfaceExtensionTests { "message": "OK" }, "data": { + "version": { + "major": 28, + "minor": 0, + "micro": 4 + }, "capabilities": { "files": { "undelete": false @@ -187,6 +192,11 @@ struct RemoteInterfaceExtensionTests { "message": "OK" }, "data": { + "version": { + "major": 28, + "minor": 0, + "micro": 4 + }, "capabilities": { "files": { "undelete": false @@ -242,6 +252,11 @@ struct RemoteInterfaceExtensionTests { "message": "OK" }, "data": { + "version": { + "major": 28, + "minor": 0, + "micro": 4 + }, "capabilities": { "core": { "pollinterval": 60