Don't load folder if we encounter a db error

Fixes: #9147

4 files changed, 173 insertions, 151 deletions

diff --git a/src/common/ownsql.cpp b/src/common/ownsql.cpp
index 0877b2787..2e970cece 100644
--- a/src/common/ownsql.cpp
+++ b/src/common/ownsql.cpp
@@ -17,26 +17,35 @@
  */
 
 #include <QDateTime>
-#include <QLoggingCategory>
-#include <QString>
+#include <QDir>
 #include <QFile>
 #include <QFileInfo>
-#include <QDir>
+#include <QLoggingCategory>
+#include <QString>
 
-#include "ownsql.h"
-#include "common/utility.h"
 #include "common/asserts.h"
+#include "common/utility.h"
+#include "ownsql.h"
+
 #include <sqlite3.h>
 
-#define SQLITE_SLEEP_TIME_USEC 100000
-#define SQLITE_REPEAT_COUNT 20
+#include <chrono>
+#include <thread>
+
+using namespace std::chrono_literals;
 
-#define SQLITE_DO(A)                                         \
-    if (1) {                                                 \
-        _errId = (A);                                        \
-        if (_errId != SQLITE_OK && _errId != SQLITE_DONE && _errId != SQLITE_ROW) {  \
-            _error = QString::fromUtf8(sqlite3_errmsg(_db)); \
-        }                                                    \
+namespace {
+constexpr auto SQLITE_SLEEP_TIME = 500ms;
+constexpr int SQLITE_REPEAT_COUNT = 20;
+
+}
+
+#define SQLITE_DO(A)                                                                \
+    if (1) {                                                                        \
+        _errId = (A);                                                               \
+        if (_errId != SQLITE_OK && _errId != SQLITE_DONE && _errId != SQLITE_ROW) { \
+            _error = QString::fromUtf8(sqlite3_errmsg(_db));                        \
+        }                                                                           \
     }
 
 namespace OCC {
@@ -132,28 +141,29 @@ bool SqlDatabase::openOrCreateReadWrite(const QString &filename)
 
     auto checkResult = checkDb();
     if (checkResult != CheckDbResult::Ok) {
+        close();
         if (checkResult == CheckDbResult::CantPrepare) {
             // When disk space is low, preparing may fail even though the db is fine.
             // Typically CANTOPEN or IOERR.
             qint64 freeSpace = Utility::freeDiskSpace(QFileInfo(filename).dir().absolutePath());
             if (freeSpace != -1 && freeSpace < 1000000) {
                 qCWarning(lcSql) << "Can't prepare consistency check and disk space is low:" << freeSpace;
-                close();
-                return false;
-            }
-
-            // Even when there's enough disk space, it might very well be that the
-            // file is on a read-only filesystem and can't be opened because of that.
-            if (_errId == SQLITE_CANTOPEN) {
+            } else if (_errId == SQLITE_CANTOPEN) {
+                // Even when there's enough disk space, it might very well be that the
+                // file is on a read-only filesystem and can't be opened because of that.
                 qCWarning(lcSql) << "Can't open db to prepare consistency check, aborting";
-                close();
-                return false;
+            } else if (_errId == SQLITE_LOCKED || _errId == SQLITE_BUSY) {
+                qCWarning(lcSql) << "Can't open db to prepare consistency check, the db is locked aborting" << _errId << _error;
             }
+            return false;
         }
 
         qCCritical(lcSql) << "Consistency check failed, removing broken db" << filename;
-        close();
-        QFile::remove(filename);
+        QFile fileToRemove(filename);
+        if (!fileToRemove.remove()) {
+            qCCritical(lcSql) << "Failed to remove broken db" << filename << ":" << fileToRemove.errorString();
+            return false;
+        }
 
         return openHelper(filename, SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE);
     }
@@ -254,20 +264,24 @@ int SqlQuery::prepare(const QByteArray &sql, bool allow_failure)
         finish();
     }
     if (!_sql.isEmpty()) {
-        int n = 0;
-        int rc;
-        do {
+        int rc = {};
+        for (int n = 0; n < SQLITE_REPEAT_COUNT; ++n) {
+            qCDebug(lcSql) << "SQL prepare" << _sql << "Try:" << n;
             rc = sqlite3_prepare_v2(_db, _sql.constData(), -1, &_stmt, nullptr);
-            if ((rc == SQLITE_BUSY) || (rc == SQLITE_LOCKED)) {
-                n++;
-                OCC::Utility::usleep(SQLITE_SLEEP_TIME_USEC);
+            if (rc != SQLITE_OK) {
+                qCWarning(lcSql) << "SQL prepare failed" << _sql << QString::fromUtf8(sqlite3_errmsg(_db));
+                if ((rc == SQLITE_BUSY) || (rc == SQLITE_LOCKED)) {
+                    std::this_thread::sleep_for(SQLITE_SLEEP_TIME);
+                    continue;
+                }
             }
-        } while ((n < SQLITE_REPEAT_COUNT) && ((rc == SQLITE_BUSY) || (rc == SQLITE_LOCKED)));
+            break;
+        }
         _errId = rc;
 
         if (_errId != SQLITE_OK) {
             _error = QString::fromUtf8(sqlite3_errmsg(_db));
-            qCWarning(lcSql) << "Sqlite prepare statement error:" << _error << "in" << _sql;
+            qCWarning(lcSql) << "Sqlite prepare statement error:" << _errId << _error << "in" << _sql;
             OC_ENFORCE_X(allow_failure, "SQLITE Prepare error");
         } else {
             OC_ASSERT(_stmt);
@@ -298,8 +312,6 @@ bool SqlQuery::isPragma()
 
 bool SqlQuery::exec()
 {
-    qCDebug(lcSql) << "SQL exec" << _sql;
-
     if (!_stmt) {
         qCWarning(lcSql) << "Can't exec query, statement unprepared.";
         return false;
@@ -307,18 +319,19 @@ bool SqlQuery::exec()
 
     // Don't do anything for selects, that is how we use the lib :-|
     if (!isSelect() && !isPragma()) {
-        int rc, n = 0;
-        do {
+        int rc = 0;
+        for (int n = 0; n < SQLITE_REPEAT_COUNT; ++n) {
+            qCDebug(lcSql) << "SQL exec" << _sql << "Try:" << n;
             rc = sqlite3_step(_stmt);
-            if (rc == SQLITE_LOCKED) {
-                rc = sqlite3_reset(_stmt); /* This will also return SQLITE_LOCKED */
-                n++;
-                OCC::Utility::usleep(SQLITE_SLEEP_TIME_USEC);
-            } else if (rc == SQLITE_BUSY) {
-                OCC::Utility::usleep(SQLITE_SLEEP_TIME_USEC);
-                n++;
+            if (rc != SQLITE_DONE || rc != SQLITE_ROW) {
+                qCWarning(lcSql) << "SQL exec failed" << _sql << QString::fromUtf8(sqlite3_errmsg(_db));
+                if (rc == SQLITE_LOCKED || rc == SQLITE_BUSY) {
+                    std::this_thread::sleep_for(SQLITE_SLEEP_TIME);
+                    continue;
+                }
+                break;
             }
-        } while ((n < SQLITE_REPEAT_COUNT) && ((rc == SQLITE_BUSY) || (rc == SQLITE_LOCKED)));
+        }
         _errId = rc;
 
         if (_errId != SQLITE_DONE && _errId != SQLITE_ROW) {
@@ -343,13 +356,10 @@ auto SqlQuery::next() -> NextResult
 {
     const bool firstStep = !sqlite3_stmt_busy(_stmt);
 
-    int n = 0;
-    forever {
+    for (int n = 0; n < SQLITE_REPEAT_COUNT; ++n) {
         _errId = sqlite3_step(_stmt);
-        if (n < SQLITE_REPEAT_COUNT && firstStep && (_errId == SQLITE_LOCKED || _errId == SQLITE_BUSY)) {
-            sqlite3_reset(_stmt); // not necessary after sqlite version 3.6.23.1
-            n++;
-            OCC::Utility::usleep(SQLITE_SLEEP_TIME_USEC);
+        if (firstStep && (_errId == SQLITE_LOCKED || _errId == SQLITE_BUSY)) {
+            std::this_thread::sleep_for(SQLITE_SLEEP_TIME);
         } else {
             break;
         }
diff --git a/src/gui/folder.cpp b/src/gui/folder.cpp
index 3e42d50c8..26d86568a 100644
--- a/src/gui/folder.cpp
+++ b/src/gui/folder.cpp
@@ -90,75 +90,71 @@ Folder::Folder(const FolderDefinition &definition,
     }
     _syncResult.setStatus(status);
 
-    // check if the local path exists
-    const auto folderOk = checkLocalPath();
-
     _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);
-
-    ConfigFile::setupDefaultExcludeFilePaths(_engine->excludedFiles());
-    if (!reloadExcludes())
-        qCWarning(lcFolder, "Could not read system exclude file");
-
-    connect(_accountState.data(), &AccountState::isConnectedChanged, this, &Folder::canSyncChanged);
-    connect(_engine.data(), &SyncEngine::rootEtag, this, &Folder::etagRetrievedFromSyncEngine);
-
-    connect(_engine.data(), &SyncEngine::started, this, &Folder::slotSyncStarted, Qt::QueuedConnection);
-    connect(_engine.data(), &SyncEngine::finished, this, &Folder::slotSyncFinished, Qt::QueuedConnection);
-
-    connect(_engine.data(), &SyncEngine::aboutToRemoveAllFiles,
-        this, &Folder::slotAboutToRemoveAllFiles);
-    connect(_engine.data(), &SyncEngine::transmissionProgress, this, &Folder::slotTransmissionProgress);
-    connect(_engine.data(), &SyncEngine::itemCompleted,
-        this, &Folder::slotItemCompleted);
-    connect(_engine.data(), &SyncEngine::newBigFolder,
-        this, &Folder::slotNewBigFolderDiscovered);
-    connect(_engine.data(), &SyncEngine::seenLockedFile, FolderMan::instance(), &FolderMan::slotSyncOnceFileUnlocks);
-    connect(_engine.data(), &SyncEngine::aboutToPropagate,
-        this, &Folder::slotLogPropagationStart);
-    connect(_engine.data(), &SyncEngine::syncError, this, &Folder::slotSyncError);
-
-    _scheduleSelfTimer.setSingleShot(true);
-    _scheduleSelfTimer.setInterval(SyncEngine::minimumFileAgeForUpload);
-    connect(&_scheduleSelfTimer, &QTimer::timeout,
-        this, &Folder::slotScheduleThisFolder);
-
-    connect(ProgressDispatcher::instance(), &ProgressDispatcher::folderConflicts,
-        this, &Folder::slotFolderConflicts);
-    connect(_engine.data(), &SyncEngine::excluded, this, [this](const QString &path, CSYNC_EXCLUDE_TYPE reason) {
-        Q_EMIT ProgressDispatcher::instance()->excluded(this, path, reason);
-    });
-
-    _localDiscoveryTracker.reset(new LocalDiscoveryTracker);
-    connect(_engine.data(), &SyncEngine::finished,
-        _localDiscoveryTracker.data(), &LocalDiscoveryTracker::slotSyncFinished);
-    connect(_engine.data(), &SyncEngine::itemCompleted,
-        _localDiscoveryTracker.data(), &LocalDiscoveryTracker::slotItemCompleted);
-
-    // Potentially upgrade suffix vfs to windows vfs
-    OC_ENFORCE(_vfs);
-    if (_definition.virtualFilesMode == Vfs::WithSuffix
-        && _definition.upgradeVfsMode) {
-        if (isVfsPluginAvailable(Vfs::WindowsCfApi)) {
-            if (auto winvfs = createVfsFromPlugin(Vfs::WindowsCfApi)) {
-                // Wipe the existing suffix files from fs and journal
-                SyncEngine::wipeVirtualFiles(path(), _journal, *_vfs);
-
-                // Then switch to winvfs mode
-                _vfs.reset(winvfs.release());
-                _definition.virtualFilesMode = Vfs::WindowsCfApi;
+    // check if the local path exists
+    if (checkLocalPath()) {
+        // 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);
+
+        ConfigFile::setupDefaultExcludeFilePaths(_engine->excludedFiles());
+        if (!reloadExcludes())
+            qCWarning(lcFolder, "Could not read system exclude file");
+
+        connect(_accountState.data(), &AccountState::isConnectedChanged, this, &Folder::canSyncChanged);
+        connect(_engine.data(), &SyncEngine::rootEtag, this, &Folder::etagRetrievedFromSyncEngine);
+
+        connect(_engine.data(), &SyncEngine::started, this, &Folder::slotSyncStarted, Qt::QueuedConnection);
+        connect(_engine.data(), &SyncEngine::finished, this, &Folder::slotSyncFinished, Qt::QueuedConnection);
+
+        connect(_engine.data(), &SyncEngine::aboutToRemoveAllFiles,
+            this, &Folder::slotAboutToRemoveAllFiles);
+        connect(_engine.data(), &SyncEngine::transmissionProgress, this, &Folder::slotTransmissionProgress);
+        connect(_engine.data(), &SyncEngine::itemCompleted,
+            this, &Folder::slotItemCompleted);
+        connect(_engine.data(), &SyncEngine::newBigFolder,
+            this, &Folder::slotNewBigFolderDiscovered);
+        connect(_engine.data(), &SyncEngine::seenLockedFile, FolderMan::instance(), &FolderMan::slotSyncOnceFileUnlocks);
+        connect(_engine.data(), &SyncEngine::aboutToPropagate,
+            this, &Folder::slotLogPropagationStart);
+        connect(_engine.data(), &SyncEngine::syncError, this, &Folder::slotSyncError);
+
+        _scheduleSelfTimer.setSingleShot(true);
+        _scheduleSelfTimer.setInterval(SyncEngine::minimumFileAgeForUpload);
+        connect(&_scheduleSelfTimer, &QTimer::timeout,
+            this, &Folder::slotScheduleThisFolder);
+
+        connect(ProgressDispatcher::instance(), &ProgressDispatcher::folderConflicts,
+            this, &Folder::slotFolderConflicts);
+        connect(_engine.data(), &SyncEngine::excluded, this, [this](const QString &path, CSYNC_EXCLUDE_TYPE reason) {
+            Q_EMIT ProgressDispatcher::instance()->excluded(this, path, reason);
+        });
+
+        _localDiscoveryTracker.reset(new LocalDiscoveryTracker);
+        connect(_engine.data(), &SyncEngine::finished,
+            _localDiscoveryTracker.data(), &LocalDiscoveryTracker::slotSyncFinished);
+        connect(_engine.data(), &SyncEngine::itemCompleted,
+            _localDiscoveryTracker.data(), &LocalDiscoveryTracker::slotItemCompleted);
+
+        // Potentially upgrade suffix vfs to windows vfs
+        OC_ENFORCE(_vfs);
+        if (_definition.virtualFilesMode == Vfs::WithSuffix
+            && _definition.upgradeVfsMode) {
+            if (isVfsPluginAvailable(Vfs::WindowsCfApi)) {
+                if (auto winvfs = createVfsFromPlugin(Vfs::WindowsCfApi)) {
+                    // Wipe the existing suffix files from fs and journal
+                    SyncEngine::wipeVirtualFiles(path(), _journal, *_vfs);
+
+                    // Then switch to winvfs mode
+                    _vfs.reset(winvfs.release());
+                    _definition.virtualFilesMode = Vfs::WindowsCfApi;
+                }
             }
+            saveToSettings();
         }
-        saveToSettings();
-    }
-
-    if (folderOk) {
         // Initialize the vfs plugin
         startVfs();
     }
@@ -192,10 +188,13 @@ bool Folder::checkLocalPath()
         _canonicalLocalPath.append('/');
     }
 
+    QString error;
     if (fi.isDir() && fi.isReadable() && fi.isWritable()) {
         qCDebug(lcFolder) << "Checked local path ok";
+        if (!_journal.open()) {
+            error = tr("%1 failed to open the database.").arg(_definition.localPath);
+        }
     } else {
-        QString error;
         // Check directory again
         if (!FileSystem::fileExists(_definition.localPath, fi)) {
             error = tr("Local folder %1 does not exist.").arg(_definition.localPath);
@@ -206,11 +205,12 @@ bool Folder::checkLocalPath()
         } else if (!fi.isWritable()) {
             error = tr("%1 is not writable.").arg(_definition.localPath);
         }
-        if (!error.isEmpty()) {
-            _syncResult.appendErrorString(error);
-            _syncResult.setStatus(SyncResult::SetupError);
-            return false;
-        }
+    }
+    qCWarning(lcFolder) << error;
+    if (!error.isEmpty()) {
+        _syncResult.appendErrorString(error);
+        _syncResult.setStatus(SyncResult::SetupError);
+        return false;
     }
     return true;
 }
@@ -278,7 +278,7 @@ QString Folder::cleanPath() const
 
 bool Folder::isSyncRunning() const
 {
-    return _engine->isSyncRunning() || _vfs->isHydrating();
+    return !hasSetupError() && (_engine->isSyncRunning() || _vfs->isHydrating());
 }
 
 QString Folder::remotePath() const
@@ -544,14 +544,13 @@ void Folder::startVfs()
     connect(_vfs.data(), &Vfs::doneHydrating, this, &Folder::slotHydrationDone);
 
     connect(&_engine->syncFileStatusTracker(), &SyncFileStatusTracker::fileStatusChanged,
-            _vfs.data(), &Vfs::fileStatusChanged);
+        _vfs.data(), &Vfs::fileStatusChanged);
 
 
     connect(_vfs.data(), &Vfs::started, this, [this] {
         // Immediately mark the sqlite temporaries as excluded. They get recreated
         // on db-open and need to get marked again every time.
         QString stateDbFile = _journal.databaseFilePath();
-        _journal.open();
         _vfs->fileStatusChanged(stateDbFile + QStringLiteral("-wal"), SyncFileStatus::StatusExcluded);
         _vfs->fileStatusChanged(stateDbFile + QStringLiteral("-shm"), SyncFileStatus::StatusExcluded);
         _vfsIsReady = true;
@@ -828,6 +827,10 @@ void Folder::slotTerminateSync()
 
 void Folder::wipeForRemoval()
 {
+    // we can't acces those variables
+    if (hasSetupError()) {
+        return;
+    }
     // prevent interaction with the db etc
     _vfsIsReady = false;
 
diff --git a/src/gui/folder.h b/src/gui/folder.h
index f4ad30588..e130dc741 100644
--- a/src/gui/folder.h
+++ b/src/gui/folder.h
@@ -188,6 +188,11 @@ public:
      */
     bool isReady() const;
 
+    bool hasSetupError() const
+    {
+        return _syncResult.status() == SyncResult::SetupError;
+    }
+
     /**
      *  Returns true if the folder needs sync poll interval wise, and can
      *  sync due to its internal state
diff --git a/src/gui/folderman.cpp b/src/gui/folderman.cpp
index 2d9ae5731..7597d2637 100644
--- a/src/gui/folderman.cpp
+++ b/src/gui/folderman.cpp
@@ -193,24 +193,26 @@ void FolderMan::unloadFolder(Folder *f)
 {
     Q_ASSERT(f);
 
+    _folderMap.remove(f->alias());
     _socketApi->slotUnregisterPath(f->alias());
 
-    _folderMap.remove(f->alias());
 
-    disconnect(f, &Folder::syncStarted,
-        this, &FolderMan::slotFolderSyncStarted);
-    disconnect(f, &Folder::syncFinished,
-        this, &FolderMan::slotFolderSyncFinished);
-    disconnect(f, &Folder::syncStateChange,
-        this, &FolderMan::slotForwardFolderSyncStateChange);
-    disconnect(f, &Folder::syncPausedChanged,
-        this, &FolderMan::slotFolderSyncPaused);
-    disconnect(&f->syncEngine().syncFileStatusTracker(), &SyncFileStatusTracker::fileStatusChanged,
-        _socketApi.data(), &SocketApi::broadcastStatusPushMessage);
-    disconnect(f, &Folder::watchedFileChangedExternally,
-        &f->syncEngine().syncFileStatusTracker(), &SyncFileStatusTracker::slotPathTouched);
+    if (!f->hasSetupError()) {
+        disconnect(f, &Folder::syncStarted,
+            this, &FolderMan::slotFolderSyncStarted);
+        disconnect(f, &Folder::syncFinished,
+            this, &FolderMan::slotFolderSyncFinished);
+        disconnect(f, &Folder::syncStateChange,
+            this, &FolderMan::slotForwardFolderSyncStateChange);
+        disconnect(f, &Folder::syncPausedChanged,
+            this, &FolderMan::slotFolderSyncPaused);
+        disconnect(&f->syncEngine().syncFileStatusTracker(), &SyncFileStatusTracker::fileStatusChanged,
+            _socketApi.data(), &SocketApi::broadcastStatusPushMessage);
+        disconnect(f, &Folder::watchedFileChangedExternally,
+            &f->syncEngine().syncFileStatusTracker(), &SyncFileStatusTracker::slotPathTouched);
 
-    f->syncEngine().disconnect(f);
+        f->syncEngine().disconnect(f);
+    }
 }
 
 void FolderMan::unloadAndDeleteAllFolders()
@@ -1123,18 +1125,20 @@ Folder *FolderMan::addFolderInternal(
     }
 
     // See matching disconnects in unloadFolder().
-    connect(folder, &Folder::syncStarted, this, &FolderMan::slotFolderSyncStarted);
-    connect(folder, &Folder::syncFinished, this, &FolderMan::slotFolderSyncFinished);
-    connect(folder, &Folder::syncStateChange, this, &FolderMan::slotForwardFolderSyncStateChange);
-    connect(folder, &Folder::syncPausedChanged, this, &FolderMan::slotFolderSyncPaused);
-    connect(folder, &Folder::canSyncChanged, this, &FolderMan::slotFolderCanSyncChanged);
-    connect(&folder->syncEngine().syncFileStatusTracker(), &SyncFileStatusTracker::fileStatusChanged,
-        _socketApi.data(), &SocketApi::broadcastStatusPushMessage);
-    connect(folder, &Folder::watchedFileChangedExternally,
-        &folder->syncEngine().syncFileStatusTracker(), &SyncFileStatusTracker::slotPathTouched);
-
-    folder->registerFolderWatcher();
-    registerFolderWithSocketApi(folder);
+    if (!folder->hasSetupError()) {
+        connect(folder, &Folder::syncStarted, this, &FolderMan::slotFolderSyncStarted);
+        connect(folder, &Folder::syncFinished, this, &FolderMan::slotFolderSyncFinished);
+        connect(folder, &Folder::syncStateChange, this, &FolderMan::slotForwardFolderSyncStateChange);
+        connect(folder, &Folder::syncPausedChanged, this, &FolderMan::slotFolderSyncPaused);
+        connect(folder, &Folder::canSyncChanged, this, &FolderMan::slotFolderCanSyncChanged);
+        connect(&folder->syncEngine().syncFileStatusTracker(), &SyncFileStatusTracker::fileStatusChanged,
+            _socketApi.data(), &SocketApi::broadcastStatusPushMessage);
+        connect(folder, &Folder::watchedFileChangedExternally,
+            &folder->syncEngine().syncFileStatusTracker(), &SyncFileStatusTracker::slotPathTouched);
+
+        folder->registerFolderWatcher();
+        registerFolderWithSocketApi(folder);
+    }
     return folder;
 }