Handle local errors with the ignore list

Fixes: #9208, #9133
owncloud · Dec 9, 2021 · 64da8ec · 64da8ec
1 parent 108fcad
commit 64da8ec
Show file tree

Hide file tree

Showing 12 changed files with 59 additions and 47 deletions.
diff --git a/changelog/unreleased/9208 b/changelog/unreleased/9208
@@ -0,0 +1,7 @@
+Enhancement: Also ignore local reapeating errors for a period of time
+
+If an error occurs on the server (a url is not reachable) we try a couple of times, then we ignore that file for a period of time.
+We now do the same with erros that occure locally.
+
+https://github.com/owncloud/client/issues/9208
+https://github.com/owncloud/client/issues/9133
diff --git a/src/common/syncjournaldb.cpp b/src/common/syncjournaldb.cpp
@@ -1607,18 +1607,19 @@ int SyncJournalDb::wipeErrorBlacklist()
     return -1;
 }
 
-void SyncJournalDb::wipeErrorBlacklistEntry(const QString &file)
+void SyncJournalDb::wipeErrorBlacklistEntry(const QString &relativeFile)
 {
-    if (file.isEmpty()) {
+    if (relativeFile.isEmpty()) {
         return;
     }
+    Q_ASSERT(QFileInfo(relativeFile).isRelative());
 
     QMutexLocker locker(&_mutex);
     if (checkConnect()) {
         SqlQuery query(_db);
 
         query.prepare("DELETE FROM blacklist WHERE path=?1");
-        query.bindValue(1, file);
+        query.bindValue(1, relativeFile);
         if (!query.exec()) {
             sqlFail(QStringLiteral("Deletion of blacklist item failed."), query);
         }

diff --git a/src/common/syncjournaldb.h b/src/common/syncjournaldb.h
@@ -87,7 +87,7 @@ class OCSYNC_EXPORT SyncJournalDb : public QObject
     static qint64 getPHash(const QByteArray &);
 
     void setErrorBlacklistEntry(const SyncJournalErrorBlacklistRecord &item);
-    void wipeErrorBlacklistEntry(const QString &file);
+    void wipeErrorBlacklistEntry(const QString &relativeFile);
     void wipeErrorBlacklistCategory(SyncJournalErrorBlacklistRecord::Category category);
     int wipeErrorBlacklist();
     int errorBlackListEntryCount();

diff --git a/src/common/syncjournalfilerecord.h b/src/common/syncjournalfilerecord.h
@@ -67,13 +67,16 @@ operator==(const SyncJournalFileRecord &lhs,
 
 class OCSYNC_EXPORT SyncJournalErrorBlacklistRecord
 {
+    Q_GADGET
 public:
-    enum Category {
+    enum class Category {
         /// Normal errors have no special behavior
         Normal = 0,
         /// These get a special summary message
-        InsufficientRemoteStorage
+        InsufficientRemoteStorage,
+        LocalSoftError
     };
+    Q_ENUM(Category)
 
     SyncJournalErrorBlacklistRecord()
         : _retryCount(0)

diff --git a/src/gui/folder.cpp b/src/gui/folder.cpp
@@ -95,6 +95,9 @@ Folder::Folder(const FolderDefinition &definition,
 
     _syncResult.setFolder(_definition.alias);
 
+    // those errors should not persist over sessions
+    _journal.wipeErrorBlacklistCategory(SyncJournalErrorBlacklistRecord::Category::LocalSoftError);
+
     _engine.reset(new SyncEngine(_accountState->account(), path(), remotePath(), &_journal));
     // pass the setting if hidden files are to be ignored, will be read in csync_update
     _engine->setIgnoreHiddenFiles(_definition.ignoreHiddenFiles);
@@ -599,7 +602,10 @@ void Folder::slotWatchedPathChanged(const QString &path, ChangeReason reason)
         return;
     }
 
-    auto relativePath = path.midRef(this->path().size());
+    auto relativePath = path.mid(this->path().size());
+    if (reason == ChangeReason::UnLock) {
+        journalDb()->wipeErrorBlacklistEntry(relativePath);
+    }
 
     // Add to list of locally modified paths
     //
@@ -634,7 +640,7 @@ void Folder::slotWatchedPathChanged(const QString &path, ChangeReason reason)
             && !FileSystem::fileChanged(path, record._fileSize, record._modtime)) {
             spurious = true;
 
-            if (auto pinState = _vfs->pinState(relativePath.toString())) {
+            if (auto pinState = _vfs->pinState(relativePath)) {
                 if (*pinState == PinState::AlwaysLocal && record.isVirtualFile())
                     spurious = false;
                 if (*pinState == PinState::OnlineOnly && record.isFile())
@@ -1155,7 +1161,7 @@ void Folder::slotFolderConflicts(const QString &folder, const QStringList &confl
         r.setNumOldConflictItems(conflictPaths.size() - r.numNewConflictItems());
 }
 
-void Folder::warnOnNewExcludedItem(const SyncJournalFileRecord &record, const QStringRef &path)
+void Folder::warnOnNewExcludedItem(const SyncJournalFileRecord &record, QStringView path)
 {
     // Never warn for items in the database
     if (record.isValid())

diff --git a/src/gui/folder.h b/src/gui/folder.h
@@ -410,7 +410,7 @@ private slots:
     void slotFolderConflicts(const QString &folder, const QStringList &conflictPaths);
 
     /** Warn users if they create a file or folder that is selective-sync excluded */
-    void warnOnNewExcludedItem(const SyncJournalFileRecord &record, const QStringRef &path);
+    void warnOnNewExcludedItem(const SyncJournalFileRecord &record, QStringView path);
 
     /** Warn users about an unreliable folder watcher */
     void slotWatcherUnreliable(const QString &message);

diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp
@@ -173,13 +173,11 @@ static SyncJournalErrorBlacklistRecord createBlacklistEntry(
 
     entry._ignoreDuration = qBound(minBlacklistTime, entry._ignoreDuration, maxBlacklistTime);
 
-    if (item._status == SyncFileItem::SoftError) {
-        // Track these errors, but don't actively suppress them.
-        entry._ignoreDuration = 0;
-    }
-
     if (item._httpErrorCode == 507) {
-        entry._errorCategory = SyncJournalErrorBlacklistRecord::InsufficientRemoteStorage;
+        entry._errorCategory = SyncJournalErrorBlacklistRecord::Category::InsufficientRemoteStorage;
+    } else if (item._httpErrorCode == 0 && item._status == SyncFileItem::SoftError) {
+        // assume a local error
+        entry._errorCategory = SyncJournalErrorBlacklistRecord::Category::LocalSoftError;
     }
 
     return entry;
@@ -193,11 +191,9 @@ static void blacklistUpdate(SyncJournalDb *journal, SyncFileItem &item)
 {
     SyncJournalErrorBlacklistRecord oldEntry = journal->errorBlacklistEntry(item._file);
 
-    const bool mayBlacklist = (item._status == SyncFileItem::NormalError)
-        || ((item._status == SyncFileItem::SoftError
-                || item._status == SyncFileItem::DetailError)
-            && item._httpErrorCode != 0 // or non-local error
-        );
+    const bool mayBlacklist = item._status == SyncFileItem::NormalError
+        || item._status == SyncFileItem::SoftError
+        || item._status == SyncFileItem::DetailError;
 
     // No new entry? Possibly remove the old one, then done.
     if (!mayBlacklist) {
@@ -213,24 +209,13 @@ static void blacklistUpdate(SyncJournalDb *journal, SyncFileItem &item)
     // Suppress the error if it was and continues to be blacklisted.
     // An ignoreDuration of 0 mean we're tracking the error, but not actively
     // suppressing it.
-    if (item._hasBlacklistEntry && newEntry._ignoreDuration > 0) {
+    if (item._status != SyncFileItem::SoftError && item._hasBlacklistEntry && newEntry._ignoreDuration > 0) {
         item._status = SyncFileItem::BlacklistedError;
-
-        qCInfo(lcPropagator) << "blacklisting " << item._file
-                             << " for " << newEntry._ignoreDuration
-                             << ", retry count " << newEntry._retryCount;
-
-        return;
     }
 
-    // Some soft errors might become louder on repeat occurrence
-    if (item._status == SyncFileItem::SoftError
-        && newEntry._retryCount > 1) {
-        qCWarning(lcPropagator) << "escalating soft error on " << item._file
-                                << " to normal error, " << item._httpErrorCode;
-        item._status = SyncFileItem::NormalError;
-        return;
-    }
+    qCInfo(lcPropagator) << "blacklisting " << item._file
+                         << " for " << newEntry._ignoreDuration
+                         << ", retry count " << newEntry._retryCount;
 }
 
 void PropagateItemJob::done(SyncFileItem::Status statusArg, const QString &errorString)

diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp
@@ -173,21 +173,24 @@ bool SyncEngine::checkErrorBlacklisting(SyncFileItem &item)
     qCInfo(lcEngine) << "Item is on blacklist: " << entry._file
                      << "retries:" << entry._retryCount
                      << "for another" << waitSeconds << "s";
-
     // We need to indicate that we skip this file due to blacklisting
     // for reporting and for making sure we don't update the blacklist
     // entry yet.
     // Classification is this _instruction and _status
     item._instruction = CSYNC_INSTRUCTION_IGNORE;
-    item._status = SyncFileItem::BlacklistedError;
+    if (entry._errorCategory == SyncJournalErrorBlacklistRecord::Category::LocalSoftError) {
+        item._status = SyncFileItem::SoftError;
+        item._errorString = entry._errorString;
+    } else {
+        item._status = SyncFileItem::BlacklistedError;
 
-    auto waitSecondsStr = Utility::durationToDescriptiveString1(1000 * waitSeconds);
-    item._errorString = tr("%1 (skipped due to earlier error, trying again in %2)").arg(entry._errorString, waitSecondsStr);
+        auto waitSecondsStr = Utility::durationToDescriptiveString1(1000 * waitSeconds);
+        item._errorString = tr("%1 (skipped due to earlier error, trying again in %2)").arg(entry._errorString, waitSecondsStr);
 
-    if (entry._errorCategory == SyncJournalErrorBlacklistRecord::InsufficientRemoteStorage) {
-        slotInsufficientRemoteStorage();
+        if (entry._errorCategory == SyncJournalErrorBlacklistRecord::Category::InsufficientRemoteStorage) {
+            slotInsufficientRemoteStorage();
+        }
     }
-
     return true;
 }
 

diff --git a/test/testblacklist.cpp b/test/testblacklist.cpp
@@ -73,7 +73,7 @@ private slots:
 
             auto entry = fakeFolder.syncJournal().errorBlacklistEntry(testFileName);
             QVERIFY(entry.isValid());
-            QCOMPARE(entry._errorCategory, SyncJournalErrorBlacklistRecord::Normal);
+            QCOMPARE(entry._errorCategory, SyncJournalErrorBlacklistRecord::Category::Normal);
             QCOMPARE(entry._retryCount, 1);
             QCOMPARE(counter, 1);
             QVERIFY(entry._ignoreDuration > 0);
@@ -94,7 +94,7 @@ private slots:
 
             auto entry = fakeFolder.syncJournal().errorBlacklistEntry(testFileName);
             QVERIFY(entry.isValid());
-            QCOMPARE(entry._errorCategory, SyncJournalErrorBlacklistRecord::Normal);
+            QCOMPARE(entry._errorCategory, SyncJournalErrorBlacklistRecord::Category::Normal);
             QCOMPARE(entry._retryCount, 1);
             QCOMPARE(counter, 1);
             QVERIFY(entry._ignoreDuration > 0);
@@ -121,7 +121,7 @@ private slots:
 
             auto entry = fakeFolder.syncJournal().errorBlacklistEntry(testFileName);
             QVERIFY(entry.isValid());
-            QCOMPARE(entry._errorCategory, SyncJournalErrorBlacklistRecord::Normal);
+            QCOMPARE(entry._errorCategory, SyncJournalErrorBlacklistRecord::Category::Normal);
             QCOMPARE(entry._retryCount, 2);
             QCOMPARE(counter, 2);
             QVERIFY(entry._ignoreDuration > 0);
@@ -143,7 +143,7 @@ private slots:
 
             auto entry = fakeFolder.syncJournal().errorBlacklistEntry(testFileName);
             QVERIFY(entry.isValid());
-            QCOMPARE(entry._errorCategory, SyncJournalErrorBlacklistRecord::Normal);
+            QCOMPARE(entry._errorCategory, SyncJournalErrorBlacklistRecord::Category::Normal);
             QCOMPARE(entry._retryCount, 3);
             QCOMPARE(counter, 3);
             QVERIFY(entry._ignoreDuration > 0);

diff --git a/test/testdownload.cpp b/test/testdownload.cpp
@@ -86,6 +86,7 @@ private slots:
             }
             return nullptr;
         });
+        fakeFolder.syncJournal().wipeErrorBlacklist();
         QVERIFY(fakeFolder.syncOnce()); // now this succeeds
         QCOMPARE(ranges, QByteArray("bytes=" + QByteArray::number(stopAfter) + "-"));
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());

diff --git a/test/testlockedfiles.cpp b/test/testlockedfiles.cpp
@@ -136,6 +136,7 @@ private slots:
 
         fakeFolder.syncEngine().setLocalDiscoveryOptions(LocalDiscoveryStyle::DatabaseAndFilesystem, tracker.localDiscoveryPaths());
         tracker.startSyncPartialDiscovery();
+        fakeFolder.syncJournal().wipeErrorBlacklist();
         QVERIFY(fakeFolder.syncOnce());
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
 
@@ -159,6 +160,7 @@ private slots:
 
         fakeFolder.syncEngine().setLocalDiscoveryOptions(LocalDiscoveryStyle::DatabaseAndFilesystem, tracker.localDiscoveryPaths());
         tracker.startSyncPartialDiscovery();
+        fakeFolder.syncJournal().wipeErrorBlacklist();
         QVERIFY(fakeFolder.syncOnce());
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
     }

diff --git a/test/testsyncengine.cpp b/test/testsyncengine.cpp
@@ -524,6 +524,7 @@ private slots:
 
         // Good OC-Checksum
         checksumValue = "SHA1:19b1928d58a2030d08023f3d7054516dbc186f20"; // printf 'A%.0s' {1..16} | sha1sum -
+        fakeFolder.syncJournal().wipeErrorBlacklist();
         QVERIFY(fakeFolder.syncOnce());
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
         checksumValue = QByteArray();
@@ -535,6 +536,7 @@ private slots:
 
         // Good Content-MD5
         contentMd5Value = "d8a73157ce10cd94a91c2079fc9a92c8"; // printf 'A%.0s' {1..16} | md5sum -
+        fakeFolder.syncJournal().wipeErrorBlacklist();
         QVERIFY(fakeFolder.syncOnce());
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
 
@@ -547,6 +549,7 @@ private slots:
         fakeFolder.remoteModifier().create("A/a7", 16, 'A');
         QVERIFY(!fakeFolder.syncOnce());
         contentMd5Value.clear();
+        fakeFolder.syncJournal().wipeErrorBlacklist();
         QVERIFY(fakeFolder.syncOnce());
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
 
@@ -555,6 +558,7 @@ private slots:
         fakeFolder.remoteModifier().create("A/a8", 16, 'A');
         QVERIFY(!fakeFolder.syncOnce()); // Since the supported SHA1 checksum is invalid, no download
         checksumValue =  "Unsupported:XXXX SHA1:19b1928d58a2030d08023f3d7054516dbc186f20 Invalid:XxX";
+        fakeFolder.syncJournal().wipeErrorBlacklist();
         QVERIFY(fakeFolder.syncOnce()); // The supported SHA1 checksum is valid now, so the file are downloaded
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
     }