Unschedule previously scheduled sync runs for locked files if it is found the sync run is no longer needed

Signed-off-by: Claudio Cambra <claudio.cambra@gmail.com>

4 files changed, 124 insertions, 60 deletions

diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp
index 00494ee52..4583380b1 100644
--- a/src/libsync/discovery.cpp
+++ b/src/libsync/discovery.cpp
@@ -407,6 +407,14 @@ void ProcessDirectoryJob::processFile(PathTuple path,
 
         _discoveryData->_anotherSyncNeeded = true;
         _discoveryData->_filesNeedingScheduledSync.insert(path._original, lockExpirationTimeout);
+
+    } else if (serverEntry.locked == SyncFileItem::LockStatus::UnlockedItem && dbEntry._lockstate._locked) {
+        // We have received data that this file has been unlocked remotely, so let's notify the sync engine
+        // that we no longer need a scheduled sync run for this file
+        qCInfo(lcDisco) << "File:" << path._original << "is unlocked and a scheduled sync is no longer needed."
+                        << "Will remove scheduled sync if there is one.";
+
+        _discoveryData->_filesUnscheduleSync.append(path._original);
     }
 
     // VFS suffixed files on the server are ignored
@@ -520,7 +528,7 @@ void ProcessDirectoryJob::processFileAnalyzeRemoteInfo(
         const bool isVirtualE2EePlaceholder = isDbEntryAnE2EePlaceholder && serverEntry.size >= Constants::e2EeTagSize;
         const qint64 sizeOnServer = isVirtualE2EePlaceholder ? serverEntry.size - Constants::e2EeTagSize : serverEntry.size;
         const bool metaDataSizeNeedsUpdateForE2EeFilePlaceholder = isVirtualE2EePlaceholder && dbEntry._fileSize == serverEntry.size;
-        const bool serverEntryLockedAsBool = serverEntry.locked == SyncFileItem::LockStatus::LockedItem;
+        const bool isServerEntryLocked = serverEntry.locked == SyncFileItem::LockStatus::LockedItem;
 
         if (serverEntry.isDirectory != dbEntry.isDirectory()) {
             // If the type of the entity changed, it's like NEW, but
@@ -567,7 +575,7 @@ void ProcessDirectoryJob::processFileAnalyzeRemoteInfo(
             }
             item->_instruction = CSYNC_INSTRUCTION_UPDATE_METADATA;
             item->_direction = SyncFileItem::Down;
-        } else if(serverEntryLockedAsBool != dbEntry._lockstate._locked) {
+        } else if(isServerEntryLocked != dbEntry._lockstate._locked) {
             item->_instruction = CSYNC_INSTRUCTION_UPDATE_METADATA;
         } else {
             // if (is virtual mode enabled and folder is encrypted - check if the size is the same as on the server and then - trigger server query
diff --git a/src/libsync/discoveryphase.h b/src/libsync/discoveryphase.h
index 21226b444..0acb862d2 100644
--- a/src/libsync/discoveryphase.h
+++ b/src/libsync/discoveryphase.h
@@ -284,6 +284,7 @@ public:
     QByteArray _dataFingerprint;
     bool _anotherSyncNeeded = false;
     QHash<QString, long long> _filesNeedingScheduledSync;
+    QVector<QString> _filesUnscheduleSync;
 
 signals:
     void fatalError(const QString &errorString);
diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp
index 0b885e86f..c39d63989 100644
--- a/src/libsync/syncengine.cpp
+++ b/src/libsync/syncengine.cpp
@@ -704,6 +704,10 @@ void SyncEngine::slotDiscoveryFinished()
             _anotherSyncNeeded = ImmediateFollowUp;
         }
 
+        if (!_discoveryPhase->_filesUnscheduleSync.empty()) {
+            slotUnscheduleFilesDelayedSync();
+        }
+
         Q_ASSERT(std::is_sorted(_syncItems.begin(), _syncItems.end()));
 
         qCInfo(lcEngine) << "#### Reconcile (aboutToPropagate) #################################################### " << _stopWatch.addLapTime(QStringLiteral("Reconcile (aboutToPropagate)")) << "ms";
@@ -1138,7 +1142,7 @@ void SyncEngine::slotScheduleFilesDelayedSync()
     // balance between updating the needed file in a timely manner while also syncing late enough
     // to cover all the files in the interval bucket.
 
-    static constexpr long long intervalSecs = 60;
+    static constexpr qint64 intervalSecs = 60;
     const auto scheduledSyncBuckets = groupNeededScheduledSyncRuns(intervalSecs);
 
     qCDebug(lcEngine) << "Active scheduled sync run timers:" << _scheduledSyncTimers.count();
@@ -1149,27 +1153,24 @@ void SyncEngine::slotScheduleFilesDelayedSync()
         const auto scheduledSyncTimerTime = QDateTime::fromSecsSinceEpoch(currentSecsSinceEpoch + scheduledSyncTimerSecs);
         const auto scheduledSyncTimerMsecs = std::chrono::milliseconds(scheduledSyncTimerSecs * 1000);
 
+        const auto addFilesToTimerAndScheduledHash = [this, &files = filesAffected] (const QSharedPointer<ScheduledSyncTimer> &timer) {
+            for (const auto &file : files) {
+                timer->files.insert(file);
+                _filesScheduledForLaterSync.insert(file, timer);
+            }
+        };
+
         // We want to make sure that this bucket won't schedule a sync near a pre-existing sync run,
         // as we often get, for example, locked file notifications one by one as the user interacts
         // through the web.
 
-        if (nearbyScheduledSyncTimerUsable(scheduledSyncTimerSecs, intervalSecs)) {
-            qCInfo(lcEngine) << "Already have a nearby scheduled sync run at:" << scheduledSyncTimerTime
-                             << "which will be used for files:" << filesAffected;
-
-            // We still want to remove these files as files scheduled for sync. We schedule another timer
-            // for the same time the nearby scheduled sync run completes, remocing these files from our
-            // hash of files already scheduled for later sync.
-
-            QTimer::singleShot(scheduledSyncTimerMsecs, this, [this, filesAffected = filesAffected] {
-                qCInfo(lcEngine) << "Following files:" << filesAffected
-                                 << "were scheduled for a sync run that already had a nearby run scheduled.";
-
-                for (const auto &file : filesAffected) {
-                    _filesScheduledForLaterSync.remove(file);
-                }
-            });
+        const auto nearbyTimer = nearbyScheduledSyncTimer(scheduledSyncTimerSecs, intervalSecs);
 
+        if (nearbyTimer) {
+            addFilesToTimerAndScheduledHash(nearbyTimer);
+            qCInfo(lcEngine) << "Using a nearby scheduled sync run at:" << scheduledSyncTimerTime
+                             << "for files:" << filesAffected
+                             << "this timer is now resoponsible for files:" << nearbyTimer->files;
             continue;
         }
 
@@ -1177,17 +1178,12 @@ void SyncEngine::slotScheduleFilesDelayedSync()
                          << "seconds, at" << scheduledSyncTimerTime
                          << "for files:" << filesAffected;
 
-        QSharedPointer<QTimer> newTimer(new QTimer);
+        QSharedPointer<ScheduledSyncTimer> newTimer(new ScheduledSyncTimer);
         newTimer->setSingleShot(true);
+        newTimer->callOnTimeout(this, [this, newTimer] {
+            qCInfo(lcEngine) << "Rescanning now that delayed sync run is scheduled for:" << newTimer->files;
 
-        // In C++17 structured bindings (used above) cannot be captured, so we need an init capture.
-        // This is because structured bindings are never names of variables, thus uncapturable.
-        // This has been changed in C++20 -- feel free to change once we use it
-
-        newTimer->callOnTimeout(this, [this, filesAffected = filesAffected] {
-            qCInfo(lcEngine) << "Rescanning now that delayed sync run is scheduled for:" << filesAffected;
-
-            for (const auto &file : filesAffected) {
+            for (const auto &file : newTimer->files) {
                 this->_filesScheduledForLaterSync.remove(file);
             }
 
@@ -1195,14 +1191,19 @@ void SyncEngine::slotScheduleFilesDelayedSync()
             this->slotCleanupScheduledSyncTimers();
         });
 
+        addFilesToTimerAndScheduledHash(newTimer);
         newTimer->start(scheduledSyncTimerMsecs);
         _scheduledSyncTimers.append(newTimer);
     }
 }
 
-QHash<long long, SyncEngine::ScheduledSyncBucket> SyncEngine::groupNeededScheduledSyncRuns(const int interval)
+QHash<qint64, SyncEngine::ScheduledSyncBucket> SyncEngine::groupNeededScheduledSyncRuns(const qint64 interval) const
 {
-    QHash<long long, ScheduledSyncBucket> intervalSyncBuckets;
+    if (!_discoveryPhase || _discoveryPhase->_filesNeedingScheduledSync.empty()) {
+        return {};
+    }
+
+    QHash<qint64, ScheduledSyncBucket> intervalSyncBuckets;
 
     for (auto it = _discoveryPhase->_filesNeedingScheduledSync.cbegin();
          it != _discoveryPhase->_filesNeedingScheduledSync.cend();
@@ -1213,16 +1214,20 @@ QHash<long long, SyncEngine::ScheduledSyncBucket> SyncEngine::groupNeededSchedul
 
         // We don't want to schedule syncs again for files we have already discovered needing a
         // scheduled sync, unless the files have been re-locked or had their lock expire time
-        // extended
+        // extended. So we check the time-out of the already set timer with the time-out we
+        // receive from the server entry
+        //
+        // Since the division here is both of ints, we receive a "floor" of the division, so we
+        // are safe from a possible situation where the timer's interval is lower than we need
+        // for the file we are possibly scheduling a sync run for
 
         if (_filesScheduledForLaterSync.contains(file) &&
-            _filesScheduledForLaterSync.value(file) > syncScheduledSecs) {
+            _filesScheduledForLaterSync.value(file)->interval() / 1000 >= syncScheduledSecs) {
 
             continue;
         }
 
-        _filesScheduledForLaterSync.insert(file, syncScheduledSecs);
-        // Both long long so division results in floor-ed result
+        // Both qint64 so division results in floor-ed result
         const auto intervalBucketKey = syncScheduledSecs / interval;
 
         if (!intervalSyncBuckets.contains(intervalBucketKey)) {
@@ -1239,39 +1244,36 @@ QHash<long long, SyncEngine::ScheduledSyncBucket> SyncEngine::groupNeededSchedul
     return intervalSyncBuckets;
 }
 
-bool SyncEngine::nearbyScheduledSyncTimerUsable(const long long scheduledSyncTimerSecs,
-                                                const long long intervalSecs) const
+QSharedPointer<SyncEngine::ScheduledSyncTimer> SyncEngine::nearbyScheduledSyncTimer(const qint64 scheduledSyncTimerSecs,
+                                                                                    const qint64 intervalSecs) const
 {
     const auto scheduledSyncTimerMsecs = scheduledSyncTimerSecs * 1000;
     const auto halfIntervalMsecs = (intervalSecs * 1000) / 2;
-    auto nearbyScheduledSync = false;
 
     for (const auto &scheduledTimer : _scheduledSyncTimers) {
 
         const auto timerRemainingMsecs = scheduledTimer->remainingTime();
         const auto differenceMsecs = timerRemainingMsecs - scheduledSyncTimerMsecs;
+        const auto nearbyScheduledSync = differenceMsecs > -halfIntervalMsecs &&
+                                         differenceMsecs < halfIntervalMsecs;
 
-        nearbyScheduledSync = differenceMsecs > -halfIntervalMsecs && differenceMsecs < halfIntervalMsecs;
-
-        // Iterated timer is going to fire slightly before we need it to for the parameter timer,
-        // delay it.
+        // Iterated timer is going to fire slightly before we need it to for the parameter timer, delay it.
         if (differenceMsecs > -halfIntervalMsecs && differenceMsecs < 0) {
-            const auto scheduledSyncTimerTimeoutMsecs = std::chrono::milliseconds(scheduledSyncTimerMsecs);
 
-            nearbyScheduledSync = true;
+            const auto scheduledSyncTimerTimeoutMsecs = std::chrono::milliseconds(scheduledSyncTimerMsecs);
             scheduledTimer->start(scheduledSyncTimerTimeoutMsecs);
 
-            qCInfo(lcEngine) << "Delayed sync timer with remaining time" << timerRemainingMsecs / 1000
-                             << "by" << (differenceMsecs * -1) / 1000
-                             << "seconds due to nearby new sync run needed.";
+            qCDebug(lcEngine) << "Delayed sync timer with remaining time" << timerRemainingMsecs / 1000
+                              << "by" << (differenceMsecs * -1) / 1000
+                              << "seconds due to nearby new sync run needed.";
         }
 
-        if (nearbyScheduledSync) {
-            break;
+        if(nearbyScheduledSync) {
+            return scheduledTimer;
         }
     }
 
-    return nearbyScheduledSync;
+    return {};
 }
 
 void SyncEngine::slotCleanupScheduledSyncTimers()
@@ -1281,10 +1283,19 @@ void SyncEngine::slotCleanupScheduledSyncTimers()
     auto it = _scheduledSyncTimers.begin();
 
     while(it != _scheduledSyncTimers.end()) {
-        const auto &timerPtr = *it;
+        const auto &timer = *it;
+        auto eraseTimer = false;
+
+        if(timer && (timer->files.empty() || !timer->isActive())) {
+            qCInfo(lcEngine) << "Stopping and erasing an expired/empty scheduled sync run timer.";
+            timer->stop();
+            eraseTimer = true;
+        } else if (!timer) {
+            qCInfo(lcEngine) << "Erasing a null scheduled sync run timer.";
+            eraseTimer = true;
+        }
 
-        if(timerPtr.isNull() || !timerPtr->isActive()) {
-            qCDebug(lcEngine) << "Erasing an expired scheduled sync timer.";
+        if(eraseTimer) {
             it = _scheduledSyncTimers.erase(it);
         } else {
             ++it;
@@ -1292,4 +1303,28 @@ void SyncEngine::slotCleanupScheduledSyncTimers()
     }
 }
 
+void SyncEngine::slotUnscheduleFilesDelayedSync()
+{
+    if (!_discoveryPhase || _discoveryPhase->_filesUnscheduleSync.empty()) {
+        return;
+    }
+
+    for (const auto &file : _discoveryPhase->_filesUnscheduleSync) {
+        const auto fileSyncRunTimer = _filesScheduledForLaterSync.value(file);
+
+        if (fileSyncRunTimer) {
+            fileSyncRunTimer->files.remove(file);
+
+            // Below is only needed for logging
+            const auto currentMSecsSinceEpoch = QDateTime::currentMSecsSinceEpoch();
+            const auto scheduledSyncTimerMSecs = fileSyncRunTimer->remainingTime();
+            const auto timerExpireDate = QDateTime::fromMSecsSinceEpoch(currentMSecsSinceEpoch + scheduledSyncTimerMSecs);
+            qCInfo(lcEngine) << "Removed" << file << "from sync run timer elapsing at" << timerExpireDate
+                             << "this timer is still running for files:" << fileSyncRunTimer->files;
+        }
+    }
+
+    slotCleanupScheduledSyncTimers();
+}
+
 } // namespace OCC
diff --git a/src/libsync/syncengine.h b/src/libsync/syncengine.h
index e35087693..3ee0b10f2 100644
--- a/src/libsync/syncengine.h
+++ b/src/libsync/syncengine.h
@@ -214,6 +214,7 @@ private slots:
     void slotInsufficientRemoteStorage();
 
     void slotScheduleFilesDelayedSync();
+    void slotUnscheduleFilesDelayedSync();
     void slotCleanupScheduledSyncTimers();
 
 private:
@@ -238,10 +239,24 @@ private:
     // See SyncEngine::groupNeededScheduledSyncRuns and
     // SyncEngine::slotScheduleFilesDelayedSync for usage.
     struct ScheduledSyncBucket {
-        long long scheduledSyncTimerSecs;
+        qint64 scheduledSyncTimerSecs;
         QVector<QString> files;
     };
 
+    // Sometimes we schedule a timer for, say, 10 files. But we receive updated
+    // data from an earlier sync run and we no longer need a scheduled sync.
+    //
+    // E.g. we had a scheduled sync timer going for a file with a lock state
+    // scheduled to expire, but someone already unlocked the file on the web UI
+    //
+    // By keeping a counter of the files depending on this timer we can
+    // perform "garbage collection", by killing the timer if there are no
+    // longer any files depending on the scheduled sync run.
+    class ScheduledSyncTimer : public QTimer {
+    public:
+        QSet<QString> files;
+    };
+
     bool checkErrorBlacklisting(SyncFileItem &item);
 
     // Cleans up unnecessary downloadinfo entries in the journal as well
@@ -265,7 +280,7 @@ private:
     //
     // Bucket classification is done by simply dividing the seconds until
     // scheduled sync time by the interval (note -- integer division!)
-    QHash<long long, ScheduledSyncBucket> groupNeededScheduledSyncRuns(const int interval);
+    QHash<qint64, ScheduledSyncBucket> groupNeededScheduledSyncRuns(const qint64 interval) const;
 
     // Checks if there is already a scheduled sync run timer active near the
     // time provided as the parameter.
@@ -275,8 +290,8 @@ private:
     //
     // If this expiration occurs before the scheduled sync run provided as the
     // parameter, it is rescheduled to expire at the time of the parameter.
-    bool nearbyScheduledSyncTimerUsable(const long long scheduledSyncTimerSecs,
-                                        const long long intervalSecs) const;
+    QSharedPointer<SyncEngine::ScheduledSyncTimer> nearbyScheduledSyncTimer(const qint64 scheduledSyncTimerSecs,
+                                                                            const qint64 intervalSecs) const;
 
     static bool s_anySyncRunning; //true when one sync is running somewhere (for debugging)
 
@@ -350,10 +365,15 @@ private:
 
     QStringList _leadingAndTrailingSpacesFilesAllowed;
 
-    // Hash of files we have scheduled for later sync runs, along with their lock expire times
-    // NOTE: not necessarily the time at which their sync run will take place
-    QHash<QString, long long> _filesScheduledForLaterSync;
-    QVector<QSharedPointer<QTimer>> _scheduledSyncTimers;
+    // Hash of files we have scheduled for later sync runs, along with a
+    // pointer to the timer which will trigger the sync run for it.
+    //
+    // NOTE: these sync timers are not unique and will likely be shared
+    // between several files
+    QHash<QString, QSharedPointer<ScheduledSyncTimer>> _filesScheduledForLaterSync;
+
+    // A vector of all the (unique) scheduled sync timers
+    QVector<QSharedPointer<ScheduledSyncTimer>> _scheduledSyncTimers;
 };
 }