From 340f128e646ff28fa49f40f8620a6f2843d733ff Mon Sep 17 00:00:00 2001 From: Felipe Tumonis Date: Wed, 15 Jul 2026 15:55:25 -0300 Subject: [PATCH 1/5] fix(macOS): handle file system events off the GUI thread The FSEvents stream was scheduled on the main dispatch queue, so every event batch was handled on the GUI thread. Dropping a large tree into the sync folder made that work unbounded: addCoalescedPaths() walked each reported directory's whole subtree with QDirIterator and deduplicated it with a linear QStringList::contains(), and on kFSEventStreamEventFlagMustScanSubDirs or a kernel drop -- exactly what a mass copy triggers -- notifyAll() enumerated the entire sync folder and reported every path one by one. Each reported path then costs a journal lookup and several stats in Folder::slotWatchedPathChanged(), so the window stayed frozen until the whole batch was processed. Deliver events on a private serial queue instead, as the Windows backend already does with its own WatcherThread, and hand each batch over to the FolderWatcher's thread with a queued invocation. Deduplicate the expansion with a QSet rather than a linear scan, and stop expanding past maxCoalescedPaths: nothing is lost by stopping, because the directory being expanded was itself reported and SyncEngine::shouldDiscoverLocally() descends into a touched directory's entire subtree. notifyAll() no longer walks the tree either: a full local discovery re-reads it anyway, so it just emits lostChanges(). It reports no path deliberately -- the watched root would reach the local discovery tracker as an empty relative path, which matches only the root itself and not the tree below it. Signed-off-by: Felipe Tumonis --- src/gui/folderwatcher.cpp | 11 ++++ src/gui/folderwatcher.h | 8 +++ src/gui/folderwatcher_mac.cpp | 99 ++++++++++++++++++++++++++--------- src/gui/folderwatcher_mac.h | 14 ++++- 4 files changed, 105 insertions(+), 27 deletions(-) diff --git a/src/gui/folderwatcher.cpp b/src/gui/folderwatcher.cpp index 2f522ee8fad3f..1cc2b03f71ae9 100644 --- a/src/gui/folderwatcher.cpp +++ b/src/gui/folderwatcher.cpp @@ -213,6 +213,17 @@ int FolderWatcher::lockChangeDebouncingTimout() const return _lockChangeDebouncingTimer.interval(); } +void FolderWatcher::changesLost(const QString &root) +{ + qCWarning(lcFolderWatcher) << "Lost track of individual changes in" << root + << "- the next sync will run a full local discovery"; + + // Makes the next sync rediscover the tree from the filesystem instead of the database. No path + // is reported: the root would reach the local discovery tracker as an empty relative path, + // which matches only the root itself and not the tree below it. + emit lostChanges(); +} + void FolderWatcher::changeDetected(const QString &path) { QStringList paths(path); diff --git a/src/gui/folderwatcher.h b/src/gui/folderwatcher.h index 1e13a8b29c13d..544273b236920 100644 --- a/src/gui/folderwatcher.h +++ b/src/gui/folderwatcher.h @@ -129,6 +129,14 @@ private slots: void startNotificationTestWhenReady(); void lockChangeDebouncingTimerTimedOut(); +private: + /** Report that the backend could not keep track of individual changes under @a root. + * + * Emits lostChanges() so that the next sync runs a full local discovery. Reports no path, so + * whoever wants a sync scheduled has to react to lostChanges() as well. + */ + void changesLost(const QString &root); + protected: QHash _pendingPathes; diff --git a/src/gui/folderwatcher_mac.cpp b/src/gui/folderwatcher_mac.cpp index b4105aac8d954..f6787e57ac6f1 100644 --- a/src/gui/folderwatcher_mac.cpp +++ b/src/gui/folderwatcher_mac.cpp @@ -13,11 +13,22 @@ #include #include +#include #include namespace OCC { +namespace { +/* Upper bound on the paths one event batch may be expanded into by addCoalescedPaths(). + * + * Every reported path costs a journal lookup and a few stats on the GUI thread later on, so for a + * large tree it is both faster and gentler on the UI to stop enumerating and let the next sync + * rediscover the tree instead. + */ +constexpr auto maxCoalescedPaths = 1000; +} + FolderWatcherPrivate::FolderWatcherPrivate(FolderWatcher *p, const QString &path) : _parent(p) , _folder(path) @@ -27,9 +38,20 @@ FolderWatcherPrivate::FolderWatcherPrivate(FolderWatcher *p, const QString &path FolderWatcherPrivate::~FolderWatcherPrivate() { - FSEventStreamStop(_stream); - FSEventStreamInvalidate(_stream); - FSEventStreamRelease(_stream); + if (_stream) { + FSEventStreamStop(_stream); + FSEventStreamInvalidate(_stream); + FSEventStreamRelease(_stream); + _stream = nullptr; + } + + if (_queue) { + // A callback block may already be running on the queue and still touching this object, + // so let it drain before the members it reads are destroyed. + dispatch_sync(_queue, ^{}); + dispatch_release(_queue); + _queue = nullptr; + } } static void callback( @@ -108,44 +130,71 @@ void FolderWatcherPrivate::startWatching() CFRelease(pathsToWatch); CFRelease(folderCF); - FSEventStreamSetDispatchQueue(_stream, dispatch_get_main_queue()); + + // Deliver events on a private queue rather than the main one: dropping a large tree into the + // sync folder produces event batches big enough that handling them on the main queue blocks + // the GUI. This mirrors what the Windows watcher does with its own WatcherThread. + _queue = dispatch_queue_create("com.nextcloud.desktopclient.folderwatcher", DISPATCH_QUEUE_SERIAL); + FSEventStreamSetDispatchQueue(_stream, _queue); FSEventStreamStart(_stream); } -QStringList FolderWatcherPrivate::addCoalescedPaths(const QStringList &paths) const +void FolderWatcherPrivate::addCoalescedPaths(const QStringList &paths, QStringList &coalesced) const { - QStringList coalescedPaths; + // Only the expansion below is bounded, not `paths` itself: those are events FSEvents actually + // reported, they are capped by the kernel's own buffer, and reporting them is what drives + // things a rediscovery cannot redo, such as releasing office file locks. + // + // FSEvents reports a renamed or moved directory, but not the items below it, so the tree is + // walked to report those too. + coalesced = paths; + QSet seen{paths.begin(), paths.end()}; + for (const auto &eventPath : paths) { - if (QDir(eventPath).exists()) { - QDirIterator it(eventPath, QDir::AllDirs | QDir::NoDotAndDotDot | QDir::Files, QDirIterator::Subdirectories); - while (it.hasNext()) { - const auto path = it.next(); - if (!paths.contains(path)) { - coalescedPaths.append(path); - } + if (!QDir(eventPath).exists()) { + continue; + } + QDirIterator it(eventPath, QDir::AllDirs | QDir::NoDotAndDotDot | QDir::Files, QDirIterator::Subdirectories); + while (it.hasNext()) { + const auto path = it.next(); + if (coalesced.size() >= maxCoalescedPaths) { + // Give up on expanding this batch rather than report a whole tree path by path. + // Nothing is lost by stopping: the directory that is being expanded was itself + // reported, and SyncEngine::shouldDiscoverLocally() descends into a touched + // directory's entire subtree, so the entries below it are discovered anyway. + return; + } + if (!seen.contains(path)) { + seen.insert(path); + coalesced.append(path); } } } - return (paths + coalescedPaths); } void FolderWatcherPrivate::doNotifyParent(const QStringList &paths) { - const QStringList totalPaths = addCoalescedPaths(paths); - _parent->changeDetected(totalPaths); + if (paths.isEmpty()) { + return; + } + + QStringList totalPaths; + addCoalescedPaths(paths, totalPaths); + + // We are on the private dispatch queue here, but everything FolderWatcher touches from + // changeDetected() (its timers, the lock-file sets, the pathChanged() receivers) may only be + // used on the thread it lives on, so hand the batch over instead of processing it here. + QMetaObject::invokeMethod( + _parent, [parent = _parent, totalPaths] { parent->changeDetected(totalPaths); }, Qt::QueuedConnection); } void FolderWatcherPrivate::notifyAll() { - QDirIterator dirIterator(_folder, QDirIterator::Subdirectories); - QStringList allPaths; - - while(dirIterator.hasNext()) { - const auto dirEntry = dirIterator.next(); - allPaths.append(dirEntry); - } - - _parent->changeDetected(allPaths); + // FSEvents either dropped events or asked for a full rescan. Enumerating the whole tree to + // report every path would take minutes on a large folder, and a full local discovery re-reads + // it anyway, so ask for one of those instead of reporting anything. + QMetaObject::invokeMethod( + _parent, [parent = _parent, folder = _folder] { parent->changesLost(folder); }, Qt::QueuedConnection); } diff --git a/src/gui/folderwatcher_mac.h b/src/gui/folderwatcher_mac.h index 723227bebb2d8..092ff274309ee 100644 --- a/src/gui/folderwatcher_mac.h +++ b/src/gui/folderwatcher_mac.h @@ -26,7 +26,14 @@ class FolderWatcherPrivate ~FolderWatcherPrivate(); void startWatching(); - QStringList addCoalescedPaths(const QStringList &) const; + + /** Expand @a paths with the contents of any directory among them, into @a coalesced. + * + * The expansion stops at maxCoalescedPaths; the sync engine descends into a reported directory + * on its own, so the entries left out are still discovered. @a paths itself is always passed + * through, however large it is. + */ + void addCoalescedPaths(const QStringList &paths, QStringList &coalesced) const; void doNotifyParent(const QStringList &); void notifyAll(); @@ -38,7 +45,10 @@ class FolderWatcherPrivate QString _folder; - FSEventStreamRef _stream; + FSEventStreamRef _stream = nullptr; + + /// Events are delivered here instead of on the main queue, to keep them off the GUI thread. + dispatch_queue_t _queue = nullptr; }; } From b385cf522f1b3e857222696b4207016deb49deaa Mon Sep 17 00:00:00 2001 From: Felipe Tumonis Date: Wed, 15 Jul 2026 15:55:39 -0300 Subject: [PATCH 2/5] fix: keep a full local discovery request across a running sync slotNextSyncFullLocalDiscovery() is documented to ensure the next sync performs a full local discovery, but it only invalidated _timeSinceLastFullLocalDiscovery. A sync that was already running restarts that timer when it finishes, so a request arriving mid-sync -- the common case, since the watcher emits lostChanges() precisely while changes are pouring in -- was silently dropped and the next sync read from the database instead. The missed changes then waited for the periodic full local discovery, an hour by default, or forever where that interval is configured negative. Track whether a request arrived after the running sync fixed its discovery style, and only mark a full local discovery as done when none did. While here, check the selective sync list in warnOnNewExcludedItem() before stat()ing the file: the function runs once per added file and the list is empty unless the user configured selective sync. Signed-off-by: Felipe Tumonis --- src/gui/folder.cpp | 36 ++++++++++++++++++++++++++---------- src/gui/folder.h | 8 ++++++++ 2 files changed, 34 insertions(+), 10 deletions(-) diff --git a/src/gui/folder.cpp b/src/gui/folder.cpp index 074c87ee42895..63ea4070f45bd 100644 --- a/src/gui/folder.cpp +++ b/src/gui/folder.cpp @@ -1203,6 +1203,10 @@ void Folder::startSync(const QStringList &pathList) fullLocalDiscoveryInterval.count() >= 0 // negative means we don't require periodic full runs && _timeSinceLastFullLocalDiscovery.hasExpired(fullLocalDiscoveryInterval.count()); + // From here on the sync's discovery style is fixed, so a request arriving later is about a + // state this sync will not see and must not be marked as covered by it. + _fullLocalDiscoveryRequestedDuringSync = false; + if (singleItemDiscoveryOptions.isValid() && singleItemDiscoveryOptions.discoveryPath != QStringLiteral("/")) { qCInfo(lcFolder) << "Going to sync just one file"; _engine->setLocalDiscoveryOptions(LocalDiscoveryStyle::DatabaseAndFilesystem, {singleItemDiscoveryOptions.discoveryPath}); @@ -1369,7 +1373,8 @@ void Folder::slotSyncFinished(bool success) if ((_syncResult.status() == SyncResult::Success || _syncResult.status() == SyncResult::Problem) && success) { - if (_engine->lastLocalDiscoveryStyle() == LocalDiscoveryStyle::FilesystemOnly) { + if (_engine->lastLocalDiscoveryStyle() == LocalDiscoveryStyle::FilesystemOnly + && !_fullLocalDiscoveryRequestedDuringSync) { _timeSinceLastFullLocalDiscovery.start(); } } @@ -1588,6 +1593,10 @@ void Folder::slotScheduleThisFolder() void Folder::slotNextSyncFullLocalDiscovery() { _timeSinceLastFullLocalDiscovery.invalidate(); + + // Invalidating on its own is not enough: a sync that is already running restarts the timer when + // it finishes (see slotSyncFinished), which would drop this request. + _fullLocalDiscoveryRequestedDuringSync = true; } void Folder::setSilenceErrorsUntilNextSync(bool silenceErrors) @@ -1618,15 +1627,8 @@ void Folder::warnOnNewExcludedItem(const SyncJournalFileRecord &record, const QS return; } - // Don't warn for items that no longer exist. - // Note: This assumes we're getting file watcher notifications - // for folders only on creation and deletion - if we got a notification - // on content change that would create spurious warnings. - const auto fullPath = QString{_canonicalLocalPath + path}; - if (!FileSystem::fileExists(fullPath)) { - return; - } - + // Checked before touching the filesystem below: this runs once per added file, and the list is + // empty unless the user configured selective sync. bool ok = false; const auto selectiveSyncList = _journal.getSelectiveSyncList(SyncJournalDb::SelectiveSyncBlackList, &ok); if (!ok) { @@ -1636,6 +1638,15 @@ void Folder::warnOnNewExcludedItem(const SyncJournalFileRecord &record, const QS return; } + // Don't warn for items that no longer exist. + // Note: This assumes we're getting file watcher notifications + // for folders only on creation and deletion - if we got a notification + // on content change that would create spurious warnings. + const auto fullPath = QString{_canonicalLocalPath + path}; + if (!FileSystem::fileExists(fullPath)) { + return; + } + QFileInfo excludeItemFileInfo(fullPath); const auto excludeItemFilePath = excludeItemFileInfo.filePath(); const auto message = FileSystem::isDir(fullPath) @@ -1749,6 +1760,10 @@ void Folder::registerFolderWatcher() this, [this](const QString &path) { slotWatchedPathChanged(path, Folder::ChangeReason::Other); }); connect(_folderWatcher.data(), &FolderWatcher::lostChanges, this, &Folder::slotNextSyncFullLocalDiscovery); + // The watcher reports no paths along with lostChanges(), so nothing else would trigger the + // sync that is supposed to pick the missed changes up. + connect(_folderWatcher.data(), &FolderWatcher::lostChanges, + this, &Folder::scheduleThisFolderSoon); connect(_folderWatcher.data(), &FolderWatcher::becameUnreliable, this, &Folder::slotWatcherUnreliable); if (_accountState->account()->capabilities().filesLockAvailable()) { @@ -1769,6 +1784,7 @@ void Folder::disconnectFolderWatcher() } disconnect(_folderWatcher.data(), &FolderWatcher::pathChanged, nullptr, nullptr); disconnect(_folderWatcher.data(), &FolderWatcher::lostChanges, this, &Folder::slotNextSyncFullLocalDiscovery); + disconnect(_folderWatcher.data(), &FolderWatcher::lostChanges, this, &Folder::scheduleThisFolderSoon); disconnect(_folderWatcher.data(), &FolderWatcher::becameUnreliable, this, &Folder::slotWatcherUnreliable); if (_accountState->account()->capabilities().filesLockAvailable()) { disconnect(_folderWatcher.data(), &FolderWatcher::filesLockReleased, this, &Folder::slotFilesLockReleased); diff --git a/src/gui/folder.h b/src/gui/folder.h index 94ce4abdef259..bd642c81366db 100644 --- a/src/gui/folder.h +++ b/src/gui/folder.h @@ -580,6 +580,14 @@ private slots: QElapsedTimer _timeSinceLastSyncDone; QElapsedTimer _timeSinceLastSyncStart; QElapsedTimer _timeSinceLastFullLocalDiscovery; + + /** Whether a full local discovery was requested after the running sync picked its discovery + * style, and is therefore not covered by it. + * + * Without this, slotSyncFinished() would restart _timeSinceLastFullLocalDiscovery and the + * request would be lost. + */ + bool _fullLocalDiscoveryRequestedDuringSync = false; std::chrono::milliseconds _lastSyncDuration; /// The number of syncs that failed in a row. From 30af2717b9eb73b29fe71a815c2b0aa486a1f122 Mon Sep 17 00:00:00 2001 From: Felipe Tumonis Date: Wed, 15 Jul 2026 15:55:39 -0300 Subject: [PATCH 3/5] perf: sort discovered sync items once instead of inserting in order slotItemDiscovered() kept _syncItems sorted by inserting each item at its lower_bound. Inserting into the middle of a contiguous vector memmoves the tail, so discovering n items costs O(n^2): for a first sync of a large tree that is billions of shared-pointer moves on the GUI thread, all of it before the first file is transferred. Nothing reads _syncItems before discovery ends -- the first consumer is handleMassDeletion(), from slotDiscoveryFinished() -- so append and sort once there. Stable, so that items comparing equal keep the order they were discovered in. Signed-off-by: Felipe Tumonis --- src/libsync/syncengine.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp index 8c754d3eac718..cb571759413b4 100644 --- a/src/libsync/syncengine.cpp +++ b/src/libsync/syncengine.cpp @@ -30,6 +30,7 @@ #include #endif +#include #include #include #include @@ -473,9 +474,10 @@ void OCC::SyncEngine::slotItemDiscovered(const OCC::SyncFileItemPtr &item) checkErrorBlacklisting(*item); _needsUpdate = true; - // Insert sorted - auto it = std::lower_bound( _syncItems.begin(), _syncItems.end(), item ); // the _syncItems is sorted - _syncItems.insert( it, item ); + // Consumers need _syncItems sorted, but inserting in order here would memmove the tail of the + // vector once per item and cost O(n^2) overall. Nothing reads _syncItems before discovery ends, + // so it is sorted in one pass in slotDiscoveryFinished() instead. + _syncItems.append(item); slotNewItem(item); @@ -847,6 +849,10 @@ void SyncEngine::slotDiscoveryFinished() _restartedSyncAfterDiscovery = false; } + // Restores the ordering slotItemDiscovered() deliberately did not maintain per item. Stable so + // that items comparing equal keep the order they were discovered in. + std::stable_sort(_syncItems.begin(), _syncItems.end()); + if (handleMassDeletion()) { return; } From 0de37dcd13e1d69da3171f8f493d416c29d79c0a Mon Sep 17 00:00:00 2001 From: Felipe Tumonis Date: Wed, 15 Jul 2026 15:55:39 -0300 Subject: [PATCH 4/5] perf: cache the selective sync list in the journal getSelectiveSyncList() runs a full query and materializes the whole list on every call, and Folder::warnOnNewExcludedItem() calls it once per file added to the sync folder. The list only changes when the user edits the selective sync configuration, so keep it in memory and drop it in setSelectiveSyncList(), the only writer of the table, and on close(). Signed-off-by: Felipe Tumonis --- src/common/syncjournaldb.cpp | 13 +++++++++++++ src/common/syncjournaldb.h | 9 +++++++++ 2 files changed, 22 insertions(+) diff --git a/src/common/syncjournaldb.cpp b/src/common/syncjournaldb.cpp index ab99460a0f2a6..603d46ee06196 100644 --- a/src/common/syncjournaldb.cpp +++ b/src/common/syncjournaldb.cpp @@ -703,6 +703,7 @@ void SyncJournalDb::close() _db.close(); clearEtagStorageFilter(); + _selectiveSyncListCache.clear(); _metadataTableIsEmpty = false; } @@ -2380,6 +2381,12 @@ QStringList SyncJournalDb::getSelectiveSyncList(SyncJournalDb::SelectiveSyncList return result; } + const auto cached = _selectiveSyncListCache.constFind(int(type)); + if (cached != _selectiveSyncListCache.constEnd()) { + *ok = true; + return *cached; + } + const auto query = _queryManager.get(PreparedSqlQueryManager::GetSelectiveSyncListQuery, QByteArrayLiteral("SELECT path FROM selectivesync WHERE type=?1"), _db); if (!query) { qCWarning(lcDb) << "database error:" << query->error(); @@ -2408,6 +2415,8 @@ QStringList SyncJournalDb::getSelectiveSyncList(SyncJournalDb::SelectiveSyncList } *ok = true; + _selectiveSyncListCache.insert(int(type), result); + return result; } @@ -2418,6 +2427,10 @@ void SyncJournalDb::setSelectiveSyncList(SyncJournalDb::SelectiveSyncListType ty return; } + // Dropped rather than replaced with `list`: getSelectiveSyncList() hands out entries with a + // trailing slash, which is added on read, not on write. + _selectiveSyncListCache.remove(int(type)); + startTransaction(); //first, delete all entries of this type diff --git a/src/common/syncjournaldb.h b/src/common/syncjournaldb.h index 8cd868e1f0b71..84cd10dc69a3a 100644 --- a/src/common/syncjournaldb.h +++ b/src/common/syncjournaldb.h @@ -431,6 +431,15 @@ public slots: int _transaction = 0; bool _metadataTableIsEmpty = false; + /* Cache for getSelectiveSyncList(), keyed by SelectiveSyncListType. + * + * The list is read once per file in some hot paths (see Folder::warnOnNewExcludedItem) while + * changing only when the user edits the selective sync configuration, so it is kept here + * rather than re-queried. Invalidated by setSelectiveSyncList() -- the only writer of the + * selectivesync table -- and on close(). + */ + QHash _selectiveSyncListCache; + /* Storing etags to these folders, or their parent folders, is filtered out. * * When schedulePathForRemoteDiscovery() is called some etags to _invalid_ in the From c08e79d7347502de250e31cd6fe77693809630c1 Mon Sep 17 00:00:00 2001 From: Felipe Tumonis Date: Thu, 16 Jul 2026 13:07:38 -0300 Subject: [PATCH 5/5] test: pin that reporting a directory discovers its whole subtree The macOS folder watcher stops expanding an FSEvents batch at its coalescing limit and reports only the directory it was expanding, relying on the sync engine discovering that directory's whole subtree from the single entry. If SyncEngine::shouldDiscoverLocally() ever stopped descending into a reported directory, the watcher would silently miss every file below it -- a data-loss regression with no visible error. Add a TestLocalDiscovery case that pins the guarantee: with only "A" in the local discovery set, entries arbitrarily deep under it must still be discovered, while unrelated siblings must not. Verified to fail if the subtree branch of shouldDiscoverLocally() is broken. Signed-off-by: Felipe Tumonis --- test/testlocaldiscovery.cpp | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/test/testlocaldiscovery.cpp b/test/testlocaldiscovery.cpp index e5e2a943dc982..a15168ff5f60b 100644 --- a/test/testlocaldiscovery.cpp +++ b/test/testlocaldiscovery.cpp @@ -204,6 +204,29 @@ private slots: QVERIFY(!engine.shouldDiscoverLocally("")); } + // The macOS folder watcher stops expanding an FSEvents batch once it reaches its coalescing + // limit and reports only the directory it was expanding, relying on the fact that reporting a + // directory makes the sync engine discover its whole subtree. If that guarantee ever broke, + // the watcher would silently miss files below such a directory, so pin it here. + void testReportingDirectoryDiscoversWholeSubtree() + { + FakeFolder fakeFolder{FileInfo::A12_B12_C12_S12()}; + auto &engine = fakeFolder.syncEngine(); + + // Only the directory "A" was reported (the deep entries below it were coalesced away). + engine.setLocalDiscoveryOptions(LocalDiscoveryStyle::DatabaseAndFilesystem, {u"A"_s}); + + // Parents are traversed to reach it, the directory itself and everything below it is + // discovered, and unrelated siblings are left out. + QVERIFY(engine.shouldDiscoverLocally("")); + QVERIFY(engine.shouldDiscoverLocally("A")); + QVERIFY(engine.shouldDiscoverLocally("A/X")); + QVERIFY(engine.shouldDiscoverLocally("A/X/deep")); + QVERIFY(engine.shouldDiscoverLocally("A/X/deep/deeper/deepest")); + QVERIFY(!engine.shouldDiscoverLocally("B")); + QVERIFY(!engine.shouldDiscoverLocally("Ax")); + } + // Check whether item success and item failure adjusts the // tracker correctly. void testTrackerItemCompletion()