fix(macOS): keep the GUI responsive when many files are added to a sync folder

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

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<int, QStringList> _selectiveSyncListCache;
+
     /* Storing etags to these folders, or their parent folders, is filtered out.
      *
      * When schedulePathForRemoteDiscovery() is called some etags to _invalid_ in the

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);

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.

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);

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<QString, int> _pendingPathes;
 

src/gui/folderwatcher_mac.cpp

@@ -13,11 +13,22 @@
 
 #include <cerrno>
 #include <QDirIterator>
+#include <QSet>
 #include <QStringList>
 
 
 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<QString> 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);
 }
 
 

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;
 };
 }
 

src/libsync/syncengine.cpp

@@ -30,6 +30,7 @@
 #include <unistd.h>
 #endif
 
+#include <algorithm>
 #include <climits>
 #include <cassert>
 #include <chrono>
@@ -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;
     }