Merge pull request #10387 from nextcloud/backport/10377/stable-34.0

[stable-34.0] Improved upload chunk integrity

Author: nilsding

shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift

@@ -463,9 +463,6 @@ public final class FilesDatabaseManager: Sendable {
                     result.downloaded = false
                 } else if result.isUpload {
                     result.uploaded = false
-                    result.chunkUploadId = UUID().uuidString
-                } else if status == .normal, metadata.isUpload {
-                    result.chunkUploadId = nil
                 }
 
                 logger.debug("Updated status for item metadata.", [

shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Item/Item+Create.swift

@@ -137,15 +137,13 @@ public extension Item {
         log: any FileProviderLogging
     ) async -> (Item?, Error?) {
         let logger = FileProviderLogger(category: "Item", log: log)
-        let chunkUploadId =
-            itemTemplate.itemIdentifier.rawValue.replacingOccurrences(of: "/", with: "")
         let (ocId, etag, date, size, error) = await upload(
             fileLocatedAt: localPath,
             toRemotePath: remotePath,
             usingRemoteInterface: remoteInterface,
             withAccount: account,
             inChunksSized: forcedChunkSize,
-            usingChunkUploadId: chunkUploadId,
+            forItemWithIdentifier: itemTemplate.itemIdentifier.rawValue,
             dbManager: dbManager,
             creationDate: itemTemplate.creationDate as? Date,
             modificationDate: itemTemplate.contentModificationDate as? Date,

shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Item/Item+Modify.swift

@@ -139,7 +139,7 @@ public extension Item {
             usingRemoteInterface: remoteInterface,
             withAccount: account,
             inChunksSized: forcedChunkSize,
-            usingChunkUploadId: metadata.chunkUploadId,
+            forItemWithIdentifier: ocId,
             dbManager: dbManager,
             creationDate: newCreationDate,
             modificationDate: newContentModificationDate,

shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Utilities/Upload.swift

@@ -9,13 +9,44 @@ import RealmSwift
 
 let defaultFileChunkSize = 104_857_600 // 100 MiB
 
+/// The per-item prefix shared by every chunked-upload identifier for a given item, so stale chunk
+/// bookkeeping from earlier versions of the same item can be swept.
+func chunkUploadIdentifierPrefix(forItemWithIdentifier itemIdentifier: String) -> String {
+    itemIdentifier.replacingOccurrences(of: "/", with: "") + "_"
+}
+
+/// Derives a stable, *content-scoped* identifier for a chunked upload's server folder and its local
+/// `RemoteFileChunk` bookkeeping.
+///
+/// The identity is `(item, size, modificationDate)`: the same content re-uploads under the same id,
+/// so an interrupted transfer resumes and reuses the chunks already stored on the server. Any content
+/// change yields a different id, so chunks from a previous version are never spliced into a different
+/// one (see F3). The value is prefixed with the item id so stale sets can be swept by prefix.
+///
+/// When no modification date is available the content can't be bound to an id, so a per-attempt unique
+/// id is used: this forgoes resume for that upload but never risks a bad splice. In the File Provider
+/// model a real content change always bumps `contentModificationDate` (that is how the change was
+/// detected), so equal `(size, modificationDate)` reliably means identical content.
+func chunkUploadIdentifier(
+    forItemWithIdentifier itemIdentifier: String, fileSize: Int64, modificationDate: Date?
+) -> String {
+    let prefix = chunkUploadIdentifierPrefix(forItemWithIdentifier: itemIdentifier)
+
+    guard let modificationDate else {
+        return prefix + UUID().uuidString
+    }
+
+    let mtimeSeconds = Int64(modificationDate.timeIntervalSince1970.rounded())
+    return "\(prefix)\(fileSize)_\(mtimeSeconds)"
+}
+
 func upload(
     fileLocatedAt localFilePath: String,
     toRemotePath remotePath: String,
     usingRemoteInterface remoteInterface: RemoteInterface,
     withAccount account: Account,
     inChunksSized chunkSize: Int? = nil,
-    usingChunkUploadId chunkUploadId: String? = UUID().uuidString,
+    forItemWithIdentifier itemIdentifier: String,
     dbManager: FilesDatabaseManager,
     creationDate: Date? = nil,
     modificationDate: Date? = nil,
@@ -106,7 +137,9 @@ func upload(
         return (ocId, etag, date as? Date, size, remoteError)
     }
 
-    let chunkUploadId = chunkUploadId ?? UUID().uuidString
+    let chunkUploadId = chunkUploadIdentifier(
+        forItemWithIdentifier: itemIdentifier, fileSize: fileSize, modificationDate: modificationDate
+    )
 
     uploadLogger.info(
         """
@@ -117,6 +150,26 @@ func upload(
         """
     )
 
+    // Content-scoped resume (F3): drop any chunk bookkeeping left over from a *different* version of
+    // this same item — a prior interrupted upload whose content has since changed derives a different
+    // id. Keeping those rows would risk resuming against server-side chunks that belong to the old
+    // content and splicing them into the new file. Rows under the current id (a genuine resume of
+    // identical content) share the id and are preserved.
+    let staleChunkPrefix = chunkUploadIdentifierPrefix(forItemWithIdentifier: itemIdentifier)
+    do {
+        let db = dbManager.ncDatabase()
+        let staleChunks = db.objects(RemoteFileChunk.self).where {
+            $0.remoteChunkStoreFolderName.starts(with: staleChunkPrefix)
+                && $0.remoteChunkStoreFolderName != chunkUploadId
+        }
+
+        if !staleChunks.isEmpty {
+            try db.write { db.delete(staleChunks) }
+        }
+    } catch {
+        uploadLogger.error("Could not clear stale chunk bookkeeping for item.", [.error: error])
+    }
+
     let remainingChunks = dbManager
         .ncDatabase()
         .objects(RemoteFileChunk.self)

shell_integration/MacOSX/NextcloudFileProviderKit/Tests/NextcloudFileProviderKitTests/ItemCreateTests.swift

@@ -697,13 +697,31 @@ final class ItemCreateTests: NextcloudFileProviderKitTestCase {
 
     func testCreateFileChunkedResumed() async throws {
         let chunkSize = 2
-        let expectedChunkUploadId = UUID().uuidString // Check if illegal characters are stripped
-        let illegalChunkUploadId = expectedChunkUploadId + "/" // Check if illegal characters are stripped
+        let expectedChunkUploadIdBase = UUID().uuidString // Check that illegal characters are stripped.
+        let illegalChunkUploadId = expectedChunkUploadIdBase + "/" // Check that illegal characters are stripped.
+
+        let tempUrl = FileManager.default.temporaryDirectory.appendingPathComponent("file")
+        let tempData = Data(repeating: 1, count: chunkSize * 3)
+        try tempData.write(to: tempUrl)
+
+        // New-item creation has no persisted ItemMetadata (the OS supplies the template), so the chunk
+        // id is derived from the template's itemIdentifier plus the content's (size, modificationDate),
+        // with illegal path characters stripped. Seed the prior interrupted attempt under exactly that
+        // derived id so the resume path recognises identical content.
+        let modificationDate = Date(timeIntervalSince1970: 1_700_000_000)
+        let chunkUploadId = chunkUploadIdentifier(
+            forItemWithIdentifier: illegalChunkUploadId,
+            fileSize: Int64(tempData.count),
+            modificationDate: modificationDate
+        )
+        XCTAssertTrue(chunkUploadId.hasPrefix(expectedChunkUploadIdBase + "_"))
+        XCTAssertFalse(chunkUploadId.contains("/"))
+
         let previousUploadedChunkNum = 1
         let preexistingChunk = RemoteFileChunk(
             fileName: String(previousUploadedChunkNum),
             size: Int64(chunkSize),
-            remoteChunkStoreFolderName: expectedChunkUploadId
+            remoteChunkStoreFolderName: chunkUploadId
         )
 
         let db = Self.dbManager.ncDatabase()
@@ -712,37 +730,24 @@ final class ItemCreateTests: NextcloudFileProviderKitTestCase {
                 RemoteFileChunk(
                     fileName: String(previousUploadedChunkNum + 1),
                     size: Int64(chunkSize),
-                    remoteChunkStoreFolderName: expectedChunkUploadId
+                    remoteChunkStoreFolderName: chunkUploadId
                 ),
                 RemoteFileChunk(
                     fileName: String(previousUploadedChunkNum + 2),
                     size: Int64(chunkSize),
-                    remoteChunkStoreFolderName: expectedChunkUploadId
+                    remoteChunkStoreFolderName: chunkUploadId
                 )
             ])
         }
 
         let remoteInterface = MockRemoteInterface(account: Self.account, rootItem: rootItem)
-        remoteInterface.currentChunks = [expectedChunkUploadId: [preexistingChunk]]
-
-        // With real new item uploads we do not have an associated ItemMetadata as the template is
-        // passed onto us by the OS. We cannot rely on the chunkUploadId property we usually use
-        // during modified item uploads.
-        //
-        // We therefore can only use the system-provided item template's itemIdentifier as the
-        // chunked upload identifier during new item creation.
-        //
-        // To test this situation we set the ocId of the metadata used to construct the item
-        // template to the chunk upload id.
+        remoteInterface.currentChunks = [chunkUploadId: [preexistingChunk]]
+
         var fileItemMetadata = SendableItemMetadata(
             ocId: illegalChunkUploadId, fileName: "file", account: Self.account
         )
-        fileItemMetadata.ocId = illegalChunkUploadId
         fileItemMetadata.classFile = NKTypeClassFile.document.rawValue
-
-        let tempUrl = FileManager.default.temporaryDirectory.appendingPathComponent("file")
-        let tempData = Data(repeating: 1, count: chunkSize * 3)
-        try tempData.write(to: tempUrl)
+        fileItemMetadata.date = modificationDate
 
         let fileItemTemplate = Item(
             metadata: fileItemMetadata,
@@ -776,7 +781,7 @@ final class ItemCreateTests: NextcloudFileProviderKitTestCase {
         XCTAssertEqual(remoteItem.directory, fileItemMetadata.directory)
         XCTAssertEqual(remoteItem.data, tempData)
         XCTAssertEqual(
-            remoteInterface.completedChunkTransferSize[expectedChunkUploadId],
+            remoteInterface.completedChunkTransferSize[chunkUploadId],
             Int64(tempData.count) - preexistingChunk.size
         )
 

shell_integration/MacOSX/NextcloudFileProviderKit/Tests/NextcloudFileProviderKitTests/ItemModifyTests.swift

@@ -1292,13 +1292,31 @@ final class ItemModifyTests: NextcloudFileProviderKitTestCase {
 
     func testModifyFileContentsChunkedResumed() async throws {
         let chunkSize = 2
-        let chunkUploadId = UUID().uuidString
+        let newContents = Data(repeating: 1, count: chunkSize * 3)
+        let newContentsUrl = FileManager.default.temporaryDirectory.appendingPathComponent("test")
+        try newContents.write(to: newContentsUrl)
+
+        let remoteInterface = MockRemoteInterface(account: Self.account, rootItem: rootItem)
+
+        let itemMetadata = remoteItem.toItemMetadata(account: Self.account)
+        Self.dbManager.addItemMetadata(itemMetadata)
+
+        // The chunk id is derived from (item, size, modificationDate); seed the prior interrupted
+        // attempt under exactly that derived id so the resume path recognises identical content.
+        let modificationDate = Date(timeIntervalSince1970: 1_700_000_000)
+        let chunkUploadId = chunkUploadIdentifier(
+            forItemWithIdentifier: itemMetadata.ocId,
+            fileSize: Int64(newContents.count),
+            modificationDate: modificationDate
+        )
+
         let previousUploadedChunkNum = 1
         let preexistingChunk = RemoteFileChunk(
             fileName: String(previousUploadedChunkNum),
             size: Int64(chunkSize),
             remoteChunkStoreFolderName: chunkUploadId
         )
+        remoteInterface.currentChunks = [chunkUploadId: [preexistingChunk]]
 
         let db = Self.dbManager.ncDatabase()
         try db.write {
@@ -1316,19 +1334,8 @@ final class ItemModifyTests: NextcloudFileProviderKitTestCase {
             ])
         }
 
-        let remoteInterface = MockRemoteInterface(account: Self.account, rootItem: rootItem)
-        remoteInterface.currentChunks = [chunkUploadId: [preexistingChunk]]
-
-        var itemMetadata = remoteItem.toItemMetadata(account: Self.account)
-        itemMetadata.chunkUploadId = chunkUploadId
-        Self.dbManager.addItemMetadata(itemMetadata)
-
-        let newContents = Data(repeating: 1, count: chunkSize * 3)
-        let newContentsUrl = FileManager.default.temporaryDirectory.appendingPathComponent("test")
-        try newContents.write(to: newContentsUrl)
-
         var targetItemMetadata = SendableItemMetadata(value: itemMetadata)
-        targetItemMetadata.date = .init()
+        targetItemMetadata.date = modificationDate
         targetItemMetadata.size = Int64(newContents.count)
 
         let item = Item(
@@ -1366,9 +1373,6 @@ final class ItemModifyTests: NextcloudFileProviderKitTestCase {
             remoteInterface.completedChunkTransferSize[chunkUploadId],
             Int64(newContents.count) - preexistingChunk.size
         )
-
-        let dbItem = try XCTUnwrap(Self.dbManager.itemMetadata(ocId: itemMetadata.ocId))
-        XCTAssertNil(dbItem.chunkUploadId)
     }
 
     func testModifyDoesNotPropagateIgnoredFile() async {