[stable-35.0] Make change delivery acknowledgements safe against late and concurrent requests

shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Documentation.docc/ChangeEnumeration.md

@@ -93,10 +93,14 @@ limits it to 4,000.
 3. Uses the extra item to determine `moreComing`.
 4. Stores a pending acknowledgement boundary without advancing the committed cursor.
 
+These steps run in one database transaction. If a batch is already pending, the same batch is returned again instead.
+
 After `finishEnumeratingChanges(upTo:moreComing:)` returns, the enumerator acknowledges the batch and
 advances the committed cursor. If delivery is interrupted before that acknowledgement, the same batch is
 replayed from its original anchor.
 
+File Provider may request the next batch before that acknowledgement runs, also on a new enumerator. A request with the continuation anchor of a batch means that File Provider accepted that batch, so such a request acknowledges it first. Each acknowledgement names its batch by session and end position and is ignored when that batch is no longer pending, so a late acknowledgement never commits a later batch.
+
 Updates and deletions share the same limit. For a limit of three, two updates
 and one deletion consume the whole batch.
 
@@ -128,6 +132,8 @@ validation. When the saved list is found, the enumerator reads the next batch
 without contacting the server again. The final batch returns the container's
 final sync anchor instead of a continuation anchor.
 
+The saved list is found by its committed anchor or by the anchor of its pending batch, read in one query so a concurrent acknowledgement cannot hide it.
+
 The framework may invalidate the current `Enumerator` after an intermediate
 batch and create another one for the next request. The extension process may
 also be restarted. Pending changes must therefore be stored outside the
@@ -191,5 +197,7 @@ The change-enumeration tests are in
 - `testContainerChangesDeliveredInBatches` checks regular-directory changes.
 - `testTrashChangesDeliveredInBatches` checks batched trash deletions.
 
+`Tests/NextcloudFileProviderKitTests/ChangeDeliveryBufferTests.swift` checks that a late acknowledgement from another enumerator or an earlier session never commits a later batch. The change-delivery cases in `FilesDatabaseManagerTests.swift` cover preparation, replay, acknowledgement by session and end position, and removal of sessions.
+
 These tests exercise the delivery logic in the package. A signed macOS build
 is still required to verify the interaction with the real `fileproviderd`.

shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Enumeration/ChangeDeliveryBuffer.swift

@@ -41,49 +41,26 @@ final class ChangeDeliveryBuffer: @unchecked Sendable {
         logger = FileProviderLogger(category: "ChangeDeliveryBuffer", log: log)
     }
 
+    ///
     /// Whether an active durable session is positioned at the given anchor.
+    ///
+    /// The framework asks for a continuation anchor only after it accepted the batch which ended there, so a batch
+    /// which was reported up to `key` but not acknowledged yet is acknowledged here.
+    ///
     func isPrimed(forKey key: String) -> Bool {
         lock.lock()
         defer { lock.unlock() }
 
-        if let sessionId {
-            guard let session = dbManager.changeDeliverySession(sessionId: sessionId) else {
-                self.sessionId = nil
-                return false
-            }
-
-            if session.currentAnchorKey == key {
-                return true
-            }
-
-            guard session.pendingReported,
-                  session.pendingAnchorKey == key
-            else {
-                return false
-            }
+        guard let session = dbManager.changeDeliverySession(forContinuationKey: key, containerKey: containerKey) else {
+            sessionId = nil
+            return false
+        }
 
-            let deletedOcIds = dbManager.pendingChangeDeliveryDeletedOcIds(sessionId: sessionId)
-            guard dbManager.acknowledgeChangeDeliveryBatch(sessionId: sessionId, deletedOcIds: deletedOcIds) else {
+        if session.currentAnchorKey != key {
+            guard dbManager.acknowledgeChangeDeliveryBatch(sessionId: session.sessionId, endSequence: session.pendingEndSequence) else {
                 logger.error("Could not acknowledge the previously reported change delivery batch.")
                 return false
             }
-
-            return true
-        }
-
-        if let session = dbManager.changeDeliverySession(forAnchorKey: key, containerKey: containerKey) {
-            sessionId = session.sessionId
-            return true
-        }
-
-        guard let session = dbManager.changeDeliverySession(forPendingAnchorKey: key, containerKey: containerKey) else {
-            return false
-        }
-
-        let deletedOcIds = dbManager.pendingChangeDeliveryDeletedOcIds(sessionId: session.sessionId)
-        guard dbManager.acknowledgeChangeDeliveryBatch(sessionId: session.sessionId, deletedOcIds: deletedOcIds) else {
-            logger.error("Could not acknowledge the previously reported change delivery batch.")
-            return false
         }
 
         sessionId = session.sessionId
@@ -146,42 +123,50 @@ final class ChangeDeliveryBuffer: @unchecked Sendable {
         logger.debug("Persisted change delivery session.")
     }
 
+    ///
     /// Prepare the next update/delete batch without advancing the committed cursor.
-    /// Returns a continuation anchor for intermediate batches and the session's final anchor for the last.
+    /// Returns a continuation anchor for intermediate batches and the session's final anchor for the last, and
+    /// the identity of the batch to pass to ``acknowledgeBatch(_:)``.
+    ///
     func prepareChangeDeliveryBatch(
         maxItems: Int
     ) -> (
         updated: [SendableItemMetadata],
         deleted: [SendableItemMetadata],
         moreComing: Bool,
         continuationAnchorRawValue: Data?,
-        finalAnchorRawValue: Data?
+        finalAnchorRawValue: Data?,
+        acknowledgement: (sessionId: String, endSequence: Int)?
     ) {
         lock.lock()
         defer { lock.unlock() }
 
-        let budget = max(1, maxItems)
         guard let sessionId,
               let session = dbManager.changeDeliverySession(sessionId: sessionId)
         else {
-            return ([], [], false, nil, nil)
+            return ([], [], false, nil, nil, nil)
         }
 
-        let storedItems = dbManager.changeDeliveryItems(
+        guard let batch = dbManager.prepareChangeDeliveryBatch(
             sessionId: sessionId,
-            fromSequence: session.nextSequence,
-            limit: budget + 1
-        )
-        let batchItems = Array(storedItems.prefix(budget))
-        let moreComing = storedItems.count > budget
+            maxItems: max(1, maxItems),
+            continuationAnchorKey: { "\(Self.continuationPrefix)\(sessionId)|\($0)" }
+        ) else {
+            logger.error("Could not prepare change delivery batch.")
+            return ([], [], false, nil, nil, nil)
+        }
+
         let decoder = JSONDecoder()
         var updated = [SendableItemMetadata]()
         var deleted = [SendableItemMetadata]()
 
-        for item in batchItems {
+        for item in batch.items {
             guard let metadata = try? decoder.decode(SendableItemMetadata.self, from: item.metadataData) else {
+                // The batch is not delivered, so do not leave it reported.
                 logger.error("Could not decode change delivery item.")
-                return ([], [], false, nil, session.finalAnchorRawValue)
+                dbManager.removeChangeDeliverySessions(containerKey: containerKey)
+                self.sessionId = nil
+                return ([], [], false, nil, session.finalAnchorRawValue, nil)
             }
 
             if item.deleted {
@@ -191,54 +176,36 @@ final class ChangeDeliveryBuffer: @unchecked Sendable {
             }
         }
 
-        let nextSequence = batchItems.last.map { $0.sequence + 1 } ?? session.nextSequence
-        let continuationAnchor: Data?
-        let nextAnchorKey: String?
-        if moreComing {
-            let key = "\(Self.continuationPrefix)\(sessionId)|\(nextSequence)"
-            continuationAnchor = Data(key.utf8)
-            nextAnchorKey = key
-        } else {
-            continuationAnchor = nil
-            nextAnchorKey = nil
-        }
-
-        guard dbManager.prepareChangeDeliveryBatch(
-            sessionId: sessionId,
-            endSequence: nextSequence,
-            nextAnchorKey: nextAnchorKey,
-            moreComing: moreComing
-        ) else {
-            logger.error("Could not prepare change delivery batch.")
-            return ([], [], false, nil, nil)
-        }
-
         logger.debug("Prepared change delivery batch.")
         return (
             updated,
             deleted,
-            moreComing,
-            continuationAnchor,
-            session.finalAnchorRawValue
+            batch.moreComing,
+            batch.nextAnchorKey.map { Data($0.utf8) },
+            session.finalAnchorRawValue,
+            (sessionId, batch.endSequence)
         )
     }
 
-    /// Commit the prepared batch after `finishEnumeratingChanges` returns.
-    func acknowledgeBatch(deletedOcIds: [String]) {
-        lock.lock()
-        defer { lock.unlock() }
-
-        guard let sessionId else {
+    ///
+    /// Commit a batch prepared by ``prepareChangeDeliveryBatch(maxItems:)`` after `finishEnumeratingChanges`
+    /// returns. Does nothing for `nil`, which stands for a batch that was not prepared.
+    ///
+    func acknowledgeBatch(_ batch: (sessionId: String, endSequence: Int)?) {
+        guard let batch else {
             return
         }
 
-        guard dbManager.acknowledgeChangeDeliveryBatch(sessionId: sessionId, deletedOcIds: deletedOcIds) else {
+        lock.lock()
+        defer { lock.unlock() }
+
+        guard dbManager.acknowledgeChangeDeliveryBatch(sessionId: batch.sessionId, endSequence: batch.endSequence) else {
             logger.error("Could not acknowledge change delivery batch.")
             return
         }
 
-        if dbManager.changeDeliverySession(sessionId: sessionId) == nil {
-            self.sessionId = nil
+        if sessionId == batch.sessionId, dbManager.changeDeliverySession(sessionId: batch.sessionId) == nil {
+            sessionId = nil
         }
     }
 

shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Enumeration/Enumerator+ChangeEnumeration.swift

@@ -83,6 +83,7 @@ extension Enumerator {
                         deleted: [itemMetadata],
                         anchor: anchor,
                         moreComing: false,
+                        acknowledgement: nil,
                         account: account,
                         remoteInterface: remoteInterface,
                         dbManager: dbManager

shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Enumeration/Enumerator+ObserverReporting.swift

@@ -105,6 +105,7 @@ extension Enumerator {
             deleted: batch.deleted,
             anchor: reportedAnchor,
             moreComing: batch.moreComing,
+            acknowledgement: batch.acknowledgement,
             account: account,
             remoteInterface: remoteInterface,
             dbManager: dbManager
@@ -113,13 +114,15 @@ extension Enumerator {
 
     /// Report one batch of changes and acknowledge it after the observer finishes.
     /// The caller keeps the batch within the framework's size limit and orders updates parents-first.
-    /// Deletions remain available for replay until acknowledgement.
+    /// Deletions remain available for replay until acknowledgement. A `nil` acknowledgement means the batch was not
+    /// prepared from the change buffer and has nothing to acknowledge.
     func completeChangesBatch(
         _ observer: NSFileProviderChangeObserver,
         updated: [SendableItemMetadata],
         deleted: [SendableItemMetadata],
         anchor: NSFileProviderSyncAnchor,
         moreComing: Bool,
+        acknowledgement: (sessionId: String, endSequence: Int)?,
         account: Account,
         remoteInterface: RemoteInterface,
         dbManager: FilesDatabaseManager
@@ -144,9 +147,9 @@ extension Enumerator {
         reportBatchUpdates(
             observer,
             updated: updated,
-            deletedToRemove: deleted,
             anchor: anchor,
             moreComing: moreComing,
+            acknowledgement: acknowledgement,
             account: account,
             remoteInterface: remoteInterface,
             dbManager: dbManager
@@ -160,15 +163,15 @@ extension Enumerator {
     private func reportBatchUpdates(
         _ observer: NSFileProviderChangeObserver,
         updated: [SendableItemMetadata],
-        deletedToRemove: [SendableItemMetadata],
         anchor: NSFileProviderSyncAnchor,
         moreComing: Bool,
+        acknowledgement: (sessionId: String, endSequence: Int)?,
         account: Account,
         remoteInterface: RemoteInterface,
         dbManager: FilesDatabaseManager,
         handleInvalidParent: Bool = true
     ) {
-        Task { [updated, deletedToRemove] in
+        Task { [updated] in
             do {
                 let updatedItems = try await updated.toFileProviderItems(account: account, remoteInterface: remoteInterface, dbManager: dbManager, log: self.logger.log)
 
@@ -178,7 +181,7 @@ extension Enumerator {
                     }
 
                     observer.finishEnumeratingChanges(upTo: anchor, moreComing: moreComing)
-                    changeBuffer.acknowledgeBatch(deletedOcIds: deletedToRemove.map(\.ocId))
+                    changeBuffer.acknowledgeBatch(acknowledgement)
                 }
             } catch let error as NSError { // This error can only mean a missing parent item identifier
                 guard handleInvalidParent else {
@@ -202,9 +205,9 @@ extension Enumerator {
                     reportBatchUpdates(
                         observer,
                         updated: [metadata] + updated,
-                        deletedToRemove: deletedToRemove,
                         anchor: anchor,
                         moreComing: moreComing,
+                        acknowledgement: acknowledgement,
                         account: account,
                         remoteInterface: remoteInterface,
                         dbManager: dbManager,

shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Enumeration/Enumerator+Trash.swift

@@ -35,7 +35,7 @@ extension Enumerator {
         }
 
         observer.finishEnumeratingChanges(upTo: reportedAnchor, moreComing: batch.moreComing)
-        changeBuffer.acknowledgeBatch(deletedOcIds: batch.deleted.map(\.ocId))
+        changeBuffer.acknowledgeBatch(batch.acknowledgement)
 
         logger.debug("Reported trash deletion batch. deleted: \(batch.deleted.count), moreComing: \(batch.moreComing)")
     }