From 4a339cfc38119437c152e687ea913b885b39dcb9 Mon Sep 17 00:00:00 2001 From: Alex Potanin Date: Thu, 27 Aug 2026 09:16:11 +1000 Subject: [PATCH] fix(file-provider): make materialized item reconciliation converge MaterializedEnumerationObserver treats every item in the database's materialized set that is absent from the system's enumeration of materialized items as freshly evicted. Since a81b5dab7, an evicted directory keeps visitedDirectory == true so it stays subscribed to remote change scanning. That same flag is what keeps an item in materialisedItemMetadatas(), so every previously visited directory without local content reappeared as an eviction candidate on every reconciliation pass. Each pass rewrote its metadata row, logged "Updating item state to dataless." and reported it to the completion handler again, so reconciliation never reached a quiescent state. On accounts with many previously browsed directories this produced a continuous re-marking loop scaling with the size of the materialized set (issue #10558 reports 81,563 dataless marks covering 17,473 distinct items within 87 minutes in a single extension process lifetime), with sustained CPU load, database growth and starvation of enumeration, fetching and upload work in the extension. Persist and report only actual state transitions: an eviction candidate whose stored flags already match the dataless target state is now skipped entirely. Directories keep their visitedDirectory subscription, preserving the shared mount root behaviour introduced by a81b5dab7, and a directory carrying a stale downloaded flag is still cleared and reported, but only once. The updated expectation in testMaterialisedObserverWithMixedState encodes the new contract: a visited directory in steady state is not reported as evicted again. The new regression test drives two reconciliation passes over a dataless visited directory, a stale downloaded directory and an evicted file, asserting that the second pass reports no transitions at all. It fails before this change and passes with it. Full package test suite: 347 XCTest and 59 Swift Testing tests pass. The analysis, fix and tests were authored by Claude Fable 5 running in Claude Code, at the request of and operated by the contributor. Closes #10558. Assisted-by: ClaudeCode:claude-fable-5 Co-Authored-By: Claude Fable 5 Signed-off-by: Alex Potanin --- .../MaterializedEnumerationObserver.swift | 30 +++-- ...MaterialisedEnumerationObserverTests.swift | 125 +++++++++++++++++- 2 files changed, 144 insertions(+), 11 deletions(-) diff --git a/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Enumeration/MaterializedEnumerationObserver.swift b/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Enumeration/MaterializedEnumerationObserver.swift index 45e8bbc1a0ce9..3c0517c279d37 100644 --- a/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Enumeration/MaterializedEnumerationObserver.swift +++ b/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Enumeration/MaterializedEnumerationObserver.swift @@ -46,18 +46,19 @@ public class MaterializedEnumerationObserver: NSObject, NSFileProviderEnumeratio func handleEnumeratedItems(_ identifiers: Set, account: Account, dbManager: FilesDatabaseManager, completionHandler: @escaping (_ materialized: Set, _ evicted: Set) -> Void) { let metadataForMaterializedItems = dbManager.materialisedItemMetadatas(account: account.ncKitAccount) var metadataForMaterializedItemsByIdentifier = [NSFileProviderItemIdentifier: SendableItemMetadata]() + var evictionCandidates = Set() var evictedItems = Set() var stillMaterializedItems = Set() for metadata in metadataForMaterializedItems { let identifier = NSFileProviderItemIdentifier(metadata.ocId) metadataForMaterializedItemsByIdentifier[identifier] = metadata - evictedItems.insert(identifier) // Assume the item related to the metadata object was evicted until proven otherwise below. + evictionCandidates.insert(identifier) // Assume the item related to the metadata object was evicted until proven otherwise below. } for enumeratedIdentifier in identifiers { - if evictedItems.contains(enumeratedIdentifier) { - evictedItems.remove(enumeratedIdentifier) // The enumerated item cannot be assumed as evicted any longer. + if evictionCandidates.contains(enumeratedIdentifier) { + evictionCandidates.remove(enumeratedIdentifier) // The enumerated item cannot be assumed as evicted any longer. } else { stillMaterializedItems.insert(enumeratedIdentifier) @@ -88,14 +89,13 @@ public class MaterializedEnumerationObserver: NSObject, NSFileProviderEnumeratio } } - for evictedItemIdentifier in evictedItems { - guard var metadata = metadataForMaterializedItemsByIdentifier[evictedItemIdentifier] else { - logger.error("No metadata found for apparently evicted identifier.", [.item: evictedItemIdentifier]) + for candidateIdentifier in evictionCandidates { + guard let materializedMetadata = metadataForMaterializedItemsByIdentifier[candidateIdentifier] else { + logger.error("No metadata found for apparently evicted identifier.", [.item: candidateIdentifier]) continue } - logger.info("Updating item state to dataless.", [.name: metadata.fileName, .item: evictedItemIdentifier]) - + var metadata = materializedMetadata metadata.downloaded = false // Being absent from enumeratorForMaterializedItems only means the item has no @@ -107,7 +107,21 @@ public class MaterializedEnumerationObserver: NSObject, NSFileProviderEnumeratio metadata.visitedDirectory = false } + // Because visitedDirectory is preserved above, a visited directory without local + // content remains in the database's materialized set and thus reappears as an + // eviction candidate on every reconciliation pass. Persist and report only actual + // state transitions so reconciliation converges instead of re-marking the same + // items dataless on every pass (#10558). + guard metadata.downloaded != materializedMetadata.downloaded + || metadata.visitedDirectory != materializedMetadata.visitedDirectory + else { + continue + } + + logger.info("Updating item state to dataless.", [.name: metadata.fileName, .item: candidateIdentifier]) + dbManager.addItemMetadata(metadata) + evictedItems.insert(candidateIdentifier) } completionHandler(stillMaterializedItems, evictedItems) diff --git a/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/NextcloudFileProviderKitTests/MaterialisedEnumerationObserverTests.swift b/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/NextcloudFileProviderKitTests/MaterialisedEnumerationObserverTests.swift index e400d833d932a..f7073ea9ea565 100644 --- a/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/NextcloudFileProviderKitTests/MaterialisedEnumerationObserverTests.swift +++ b/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/NextcloudFileProviderKitTests/MaterialisedEnumerationObserverTests.swift @@ -102,12 +102,13 @@ final class MaterialisedEnumerationObserverTests: NextcloudFileProviderKitTestCa let enumeratorItemsToReturn = [itemB, itemC] let observer = MaterializedEnumerationObserver(account: Self.account, dbManager: dbManager, log: FileProviderLogMock()) { newlyMaterialisedIds, unmaterialisedIds in - // Unmaterialised: itemA and dirD were materialized but not in the latest enumeration. + // Unmaterialised: itemA was materialized but not in the latest enumeration. dirD is + // also absent from the enumeration, but keeps its visitedDirectory subscription and + // has no other state to clear, so it must not be reported as a state transition. XCTAssertEqual( - unmaterialisedIds.count, 2, "itemA and dirD should be reported as unmaterialised." + unmaterialisedIds.count, 1, "Only itemA should be reported as unmaterialised." ) XCTAssertTrue(unmaterialisedIds.contains(NSFileProviderItemIdentifier("itemA"))) - XCTAssertTrue(unmaterialisedIds.contains(NSFileProviderItemIdentifier("dirD"))) // Newly Materialised: itemB was NOT materialized but WAS in the latest enumeration. XCTAssertEqual( @@ -145,4 +146,122 @@ final class MaterialisedEnumerationObserverTests: NextcloudFileProviderKitTestCa await fulfillment(of: [expect], timeout: 1) } + + /// + /// Regression test for the non-converging reconciliation loop reported in + /// [#10558](https://github.com/nextcloud/desktop/issues/10558). + /// + /// A directory which was browsed before keeps `visitedDirectory == true` even when the system + /// reports it as dataless, because that flag subscribes it to remote change scanning. That + /// also keeps it in the database's materialized item set, so it reappears as an eviction + /// candidate on every reconciliation pass. The observer therefore must only persist and + /// report actual state transitions — otherwise it re-marks the same directories as dataless + /// on every pass and reconciliation never reaches a quiescent state. + /// + func testMaterialisedObserverConvergesForDatalessVisitedDirectories() async { + // A directory which was browsed before but holds no materialized content any more. + var visitedDir = SendableItemMetadata(ocId: "visitedDir", fileName: "visitedDir", account: Self.account) + visitedDir.directory = true + visitedDir.visitedDirectory = true + visitedDir.downloaded = false + + // A directory which was browsed before and still carries a stale downloaded flag. + var staleDir = SendableItemMetadata(ocId: "staleDir", fileName: "staleDir", account: Self.account) + staleDir.directory = true + staleDir.visitedDirectory = true + staleDir.downloaded = true + + // A downloaded file about to be evicted. + var file = SendableItemMetadata(ocId: "file", fileName: "file.txt", account: Self.account) + file.downloaded = true + + let dbManager = FilesDatabaseManager(account: Self.account, databaseDirectory: makeDatabaseDirectory(), fileProviderDomainIdentifier: NSFileProviderDomainIdentifier("test"), log: FileProviderLogMock()) + dbManager.addItemMetadata(visitedDir) + dbManager.addItemMetadata(staleDir) + dbManager.addItemMetadata(file) + + let remoteInterface = MockRemoteInterface(account: Self.account) + let firstPass = XCTestExpectation(description: "First pass completion handler called") + + // First pass: the system reports no materialized items at all. + let firstObserver = MaterializedEnumerationObserver(account: Self.account, dbManager: dbManager, log: FileProviderLogMock()) { newlyMaterialisedIds, unmaterialisedIds in + XCTAssertTrue( + newlyMaterialisedIds.isEmpty, + "Nothing was enumerated, so nothing should be newly materialised." + ) + + // Only actual state transitions should be reported. + XCTAssertEqual( + unmaterialisedIds.count, 2, "The file and the stale directory should be reported as evicted." + ) + XCTAssertTrue(unmaterialisedIds.contains(NSFileProviderItemIdentifier("file"))) + XCTAssertTrue(unmaterialisedIds.contains(NSFileProviderItemIdentifier("staleDir"))) + XCTAssertFalse( + unmaterialisedIds.contains(NSFileProviderItemIdentifier("visitedDir")), + "A visited directory in steady state must not be reported as a fresh eviction." + ) + + let persistedFile = dbManager.itemMetadata(ocId: "file") + XCTAssertFalse(persistedFile?.downloaded ?? true, "The file should be marked as not downloaded.") + + let persistedStaleDir = dbManager.itemMetadata(ocId: "staleDir") + XCTAssertFalse(persistedStaleDir?.downloaded ?? true, "The stale directory should lose its downloaded flag.") + XCTAssertTrue( + persistedStaleDir?.visitedDirectory ?? false, + "The stale directory should keep its visitedDirectory subscription." + ) + + let persistedVisitedDir = dbManager.itemMetadata(ocId: "visitedDir") + XCTAssertTrue( + persistedVisitedDir?.visitedDirectory ?? false, + "The visited directory should keep its visitedDirectory subscription." + ) + + firstPass.fulfill() + } + + let firstEnumerator = MockEnumerator( + account: Self.account, dbManager: dbManager, remoteInterface: remoteInterface + ) + firstEnumerator.enumeratorItems = [] + firstEnumerator.enumerateItems(for: firstObserver, startingAt: NSFileProviderPage(Data(count: 1))) + + await fulfillment(of: [firstPass], timeout: 1) + + // Second pass under unchanged conditions: reconciliation must have converged. + let secondPass = XCTestExpectation(description: "Second pass completion handler called") + + let secondObserver = MaterializedEnumerationObserver(account: Self.account, dbManager: dbManager, log: FileProviderLogMock()) { newlyMaterialisedIds, unmaterialisedIds in + XCTAssertTrue( + newlyMaterialisedIds.isEmpty, + "Nothing was enumerated, so nothing should be newly materialised." + ) + XCTAssertTrue( + unmaterialisedIds.isEmpty, + "A repeated pass without changes must not report any evictions again." + ) + + let persistedVisitedDir = dbManager.itemMetadata(ocId: "visitedDir") + XCTAssertTrue( + persistedVisitedDir?.visitedDirectory ?? false, + "The visited directory should still keep its visitedDirectory subscription." + ) + + let persistedStaleDir = dbManager.itemMetadata(ocId: "staleDir") + XCTAssertTrue( + persistedStaleDir?.visitedDirectory ?? false, + "The stale directory should still keep its visitedDirectory subscription." + ) + + secondPass.fulfill() + } + + let secondEnumerator = MockEnumerator( + account: Self.account, dbManager: dbManager, remoteInterface: remoteInterface + ) + secondEnumerator.enumeratorItems = [] + secondEnumerator.enumerateItems(for: secondObserver, startingAt: NSFileProviderPage(Data(count: 1))) + + await fulfillment(of: [secondPass], timeout: 1) + } }