Concurrent Working Set Scan Reads

[juliusvaart]

Summary

The working set scan issued its PROPFINDs strictly sequentially: 669 reads of roughly 100 ms each, so 76 seconds of almost pure network waiting to surface zero changes. Reads now go out six at a time, in waves grouped by remote path depth. One measured scan went from 76 s to 51 s with the read count essentially unchanged, 669 to 652 — that second number is the check that matters, because it shows coverage held on live data rather than the walk simply doing less.

Depth is what makes the concurrency safe. The only ordering the walk depends on is that a directory is read before the items it covers, and a depth-1 read records its unchanged direct children in scannedItemIds, which are always exactly one level deeper. Processing shallowest first preserves every coverage decision, and items within one depth can never cover one another. Results are reordered before merging and the merge stays single threaded, so the accumulators and the deletion reconciliation evolve exactly as before.

Reading concurrently also means persisting concurrently, and that cannot be done from the tasks themselves. Every Realm access blocks: a refresh waits on the coordinator mutex, and a commit walks the reachable object graph before it returns. Blocking inside a Task parks a thread of the Swift cooperative pool, which the runtime cannot reclaim, and six at once exhausted it — after which nothing else in the extension ran at all, including the enumeration for a folder the user had just opened. The symptom was a folder spinning forever while the extension sat at full CPU, and it left no trace, because the starved task never reached its own first log line. A sample of a stalled extension showed twenty threads inside depth1ReadUpdateItemMetadatas, every one in __psynch_mutexwait, on a sixteen core machine.

So FilesDatabaseManager gets a serial queue of its own and a perform(_:) that hops onto it, and the three ingestion points in the read path go through it. Callers suspend instead of blocking. The queue is serial because Realm serializes commits internally anyway, and contending for that lock is what made each stall long enough to drain the pool. Only the persistence moved; the reads stay concurrent, which is the point of the change.

Tests

DatabaseQueueTests asserts the two properties of the hop that are invisible when broken — that the work leaves the calling executor, and that concurrent callers never overlap — using counters rather than timers, so a slow runner cannot flake them. RemoteChangePropagationTests gains a case pinning the coverage rule, which the suite did not previously check: collapsing the waves makes it read three paths instead of one.

Verified against a deliberately broken build: replacing the queue hop with a direct call fails all three DatabaseQueueTests, the first with "Database work must run on the database queue, not on the cooperative pool thread that asked for it."

Full package suite green, 374 XCTest (1 skipped) and 78 swift-testing.

Assisted-by: Claude Code:claude-opus-5

Checklist

• Sign-off message is added to all commits.
• The commit messages are following The Conventional Commits specification.
• The commit history is clean with no merge commits.
• Uploaded screenshots from before and after for UI changes.
• Test(s) added to branch, with before/after results included in PR description, for performance improvements where applicable.
• Documentation has been updated or is not required.
• Backports requested where applicable (critical bugfixes).
• Labels added where applicable (bug/enhancement).
• Milestone added for target version.

AI (if applicable)

• The content of this PR was partly or fully generated using AI
  • I have read the guidelines for AI-assisted contributions.
  • I have read Nextcloud's AI Contribution Policy.

[i2h3]

This needs a thorough review. There is conceptual catch that must not be broken: We need to fetch all remote changes first before deriving any conclusions to apply on the locally cached state. Otherwise we will not discover item moves correctly.

[i2h3]

Upfront, there is a different doubt I have in addition, too: this potentially hammers a server with sixfold peak load which is a real problem in large deployments with thousands of connected clients. The assumption of so much headroom in resources may be too much or does not work in general. 😕 "Been there, done that" 😅

[juliusvaart]

Thanks, both are fair things to pin down. Taking them in turn.

Fetching everything before concluding anything

Agreed that this must not break, and it does not: the ordering you describe is exactly the rule the scan already relied on, and this change does not move it.

Nothing is concluded inside the walk. Deletions are only accumulated while reading; the reconciliation that protects moves runs after the whole walk has drained, and it is unchanged by this commit:

// Catches moves across directories: items found at a new location (updated or new)
// should not be marked deleted at the old location.
let survivingOcIds = Set(accumulatedUpdates.map(\.ocId))
    .union(accumulatedCreations.map(\.ocId))

accumulatedDeletions.removeAll { survivingOcIds.contains($0.ocId) }

Only after that filter does anything get written as deleted (dbManager.addItemMetadata(deleteMarked)), and only after that does scanMaterialisedItemsForRemoteChanges() return, which is the first point any of it reaches the change observer. So an item that moved is found at its new location during the same pass and drops out of the deletion set before it can be reported or persisted.

I diffed that whole region — from the survivor filter down to the return — between the commit and its parent, and it is byte-identical. The pre-existing comment stating the rule is still there too:

Unlike when enumerating items we can't progressively enumerate items as we need to wait to see which items are truly deleted and which have just been moved elsewhere.

What the commit changes is only how the reads are issued. Results are reordered back into the original order before merging, and the merge loop stays single-threaded, so scannedItemIds, the accumulators and the deletion reconciliation evolve in exactly the same sequence they did when the reads were serial. The depth grouping exists for the one ordering the walk does depend on: a directory is read before the items it covers.

Server load

This is the right thing to be nervous about, so here are the numbers rather than an assurance. Same working set, before and after, from the scan quoted in the commit message:

	reads	wall clock	requests/s	mean in flight (at ~100 ms/read)
sequential	669	76 s	8.8	0.9
concurrent	652	51 s	12.8	1.3

Two things in there matter more than the speedup:

The total number of requests does not go up. It went slightly down (669 to 652), which was the check I cared about most, because it is also the check that coverage held: the same folders got read, just not one after another.

Six is a ceiling, not a rate. Measured request rate rose about 1.45x, not 6x. The reads are issued in waves grouped by remote-path depth, and most depths hold only a handful of items, so the six slots are only filled in the widest wave — mean concurrency over the whole scan came out around 1.3. A sixfold sustained load would require a working set that is uniformly wide at every depth, which is not what a materialised set looks like.

I also did not pick 6 arbitrarily. It is this client's own existing limit for concurrent PROPFINDs during remote discovery:

• src/libsync/syncoptions.h:59 — int _parallelNetworkJobs = 6;
• src/libsync/discoveryphase.cpp:315 — DiscoveryPhase::scheduleMoreJobs() schedules up to that many at once

So a regular non-VFS client against the same server already discovers at this concurrency. The File Provider scan was the outlier by being strictly sequential, and this brings it to parity rather than past it.

That said, "the default is defensible" is not the same as "an admin can do nothing about it", and for a large deployment the second one is what counts. If you want a lever, the natural place is FileProviderDomainDefaults, following the debugLoggingEnabled precedent that is already documented as process-global specifically so administrators can set it with one defaults write per extension bundle. A workingSetScanConcurrency key there would let a deployment cap or disable the concurrency via MDM without changing what everyone else gets. It is a private let today. Happy to add that here if you would rather not merge a fixed value, or to lower the default outright if you think 6 is too optimistic for this path.

Worth noting for context on total load: this PR makes the scan faster, not smaller. Making it smaller is the next series in the queue, where a push is answered by reading only the containers it names rather than walking the working set. That is the change that actually reduces what the server sees, and it depends on this one.