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 <ftumonis@gmail.com>

Author: ftapps

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.