Ensure we properly cancel hydration on server errors

Signed-off-by: Kevin Ottens <kevin.ottens@nextcloud.com>

src/libsync/vfs/cfapi/cfapiwrapper.cpp

@@ -15,6 +15,7 @@
 #include "cfapiwrapper.h"
 
 #include "common/utility.h"
+#include "hydrationjob.h"
 #include "vfs_cfapi.h"
 
 #include <QDir>
@@ -106,6 +107,7 @@ void CALLBACK cfApiFetchDataCallback(const CF_CALLBACK_INFO *callbackInfo, const
         }
     });
     loop.exec();
+    QObject::disconnect(vfs, nullptr, &loop, nullptr);
     qCInfo(lcCfApiWrapper) << "VFS replied for hydration of" << path << requestId << "status was:" << hydrationRequestResult;
 
     if (!hydrationRequestResult) {
@@ -123,18 +125,31 @@ void CALLBACK cfApiFetchDataCallback(const CF_CALLBACK_INFO *callbackInfo, const
     }
 
     qint64 offset = 0;
-    while (socket.waitForReadyRead()) {
+
+    QObject::connect(&socket, &QLocalSocket::readyRead, &loop, [&] {
         auto data = socket.readAll();
         if (data.isEmpty()) {
             qCWarning(lcCfApiWrapper) << "Unexpected empty data received" << requestId;
             sendTransferError();
-            break;
+            loop.quit();
+            return;
         }
         sendTransferInfo(data, offset);
         offset += data.length();
-    }
+    });
 
-    qCInfo(lcCfApiWrapper) << "Hydration done for" << path << requestId;
+    QObject::connect(vfs, &OCC::VfsCfApi::hydrationRequestFinished, &loop, [&](const QString &id, int s) {
+        if (requestId == id) {
+            const auto status = static_cast<OCC::HydrationJob::Status>(s);
+            qCInfo(lcCfApiWrapper) << "Hydration done for" << path << requestId << status;
+            if (status != OCC::HydrationJob::Success) {
+                sendTransferError();
+            }
+            loop.quit();
+        }
+    });
+
+    loop.exec();
 }
 
 CF_CALLBACK_REGISTRATION cfApiCallbacks[] = {

src/libsync/vfs/cfapi/hydrationjob.cpp

@@ -88,6 +88,11 @@ void OCC::HydrationJob::setFolderPath(const QString &folderPath)
     _folderPath = folderPath;
 }
 
+OCC::HydrationJob::Status OCC::HydrationJob::status() const
+{
+    return _status;
+}
+
 void OCC::HydrationJob::start()
 {
     Q_ASSERT(_account);
@@ -103,7 +108,7 @@ void OCC::HydrationJob::start()
     const auto listenResult = _server->listen(_requestId);
     if (!listenResult) {
         qCCritical(lcHydration) << "Couldn't get server to listen" << _requestId << _localPath << _folderPath;
-        emitFinished();
+        emitFinished(Error);
         return;
     }
 
@@ -111,13 +116,19 @@ void OCC::HydrationJob::start()
     connect(_server, &QLocalServer::newConnection, this, &HydrationJob::onNewConnection);
 }
 
-void OCC::HydrationJob::emitFinished()
+void OCC::HydrationJob::emitFinished(Status status)
 {
-    _socket->disconnectFromServer();
-    connect(_socket, &QLocalSocket::disconnected, this, [=]{
+    _status = status;
+    if (status == Success) {
+        _socket->disconnectFromServer();
+        connect(_socket, &QLocalSocket::disconnected, this, [=]{
+            _socket->close();
+            emit finished(this);
+        });
+    } else {
         _socket->close();
         emit finished(this);
-    });
+    }
 }
 
 void OCC::HydrationJob::onNewConnection()
@@ -134,10 +145,10 @@ void OCC::HydrationJob::onNewConnection()
 
 void OCC::HydrationJob::onGetFinished()
 {
-    qCInfo(lcHydration) << "GETFileJob finished" << _requestId << _folderPath << _job->errorStatus() << _job->errorString();
+    qCInfo(lcHydration) << "GETFileJob finished" << _requestId << _folderPath << _job->reply()->error();
 
-    if (_job->errorStatus() != SyncFileItem::NoStatus && _job->errorStatus() != SyncFileItem::Success) {
-        emitFinished();
+    if (_job->reply()->error()) {
+        emitFinished(Error);
         return;
     }
 
@@ -146,11 +157,11 @@ void OCC::HydrationJob::onGetFinished()
     Q_ASSERT(record.isValid());
     if (!record.isValid()) {
         qCWarning(lcHydration) << "Couldn't find record to update after hydration" << _requestId << _folderPath;
-        emitFinished();
+        emitFinished(Error);
         return;
     }
 
     record._type = ItemTypeFile;
     _journal->setFileRecord(record);
-    emitFinished();
+    emitFinished(Success);
 }

src/libsync/vfs/cfapi/hydrationjob.h

@@ -28,6 +28,12 @@ class OWNCLOUDSYNC_EXPORT HydrationJob : public QObject
 {
     Q_OBJECT
 public:
+    enum Status {
+        Success = 0,
+        Error,
+    };
+    Q_ENUM(Status)
+
     explicit HydrationJob(QObject *parent = nullptr);
 
     AccountPtr account() const;
@@ -48,13 +54,15 @@ public:
     QString folderPath() const;
     void setFolderPath(const QString &folderPath);
 
+    Status status() const;
+
     void start();
 
 signals:
     void finished(HydrationJob *job);
 
 private:
-    void emitFinished();
+    void emitFinished(Status status);
 
     void onNewConnection();
     void onGetFinished();
@@ -70,6 +78,7 @@ private:
     QLocalServer *_server = nullptr;
     QLocalSocket *_socket = nullptr;
     GETFileJob *_job = nullptr;
+    Status _status = Success;
 };
 
 } // namespace OCC

src/libsync/vfs/cfapi/vfs_cfapi.cpp

@@ -331,6 +331,8 @@ void VfsCfApi::scheduleHydrationJob(const QString &requestId, const QString &fol
 void VfsCfApi::onHydrationJobFinished(HydrationJob *job)
 {
     Q_ASSERT(d->hydrationJobs.contains(job));
+    qCInfo(lcCfApi) << "Hydration job finished" << job->requestId() << job->folderPath() << job->status();
+    emit hydrationRequestFinished(job->requestId(), job->status());
     d->hydrationJobs.removeAll(job);
     if (d->hydrationJobs.isEmpty()) {
         emit doneHydrating();

src/libsync/vfs/cfapi/vfs_cfapi.h

@@ -60,6 +60,7 @@ public slots:
 signals:
     void hydrationRequestReady(const QString &requestId);
     void hydrationRequestFailed(const QString &requestId);
+    void hydrationRequestFinished(const QString &requestId, int status);
 
 protected:
     void startImpl(const VfsSetupParams &params) override;

test/testsynccfapi.cpp

@@ -35,6 +35,12 @@ using namespace OCC::CfApiWrapper;
 
 using namespace OCC;
 
+enum ErrorKind : int {
+    NoError = 0,
+    // Lower code are corresponding to HTTP error code
+    Timeout = 1000,
+};
+
 void setPinState(const QString &path, PinState state, cfapi::SetPinRecurseMode mode)
 {
     Q_ASSERT(mode == cfapi::Recurse || mode == cfapi::NoRecurse);
@@ -1098,8 +1104,23 @@ private slots:
         CFVERIFY_VIRTUAL(fakeFolder, "local/file1");
     }
 
+    void testOpeningOnlineFileTriggersDownload_data()
+    {
+        QTest::addColumn<int>("errorKind");
+        QTest::newRow("no error") << static_cast<int>(NoError);
+        QTest::newRow("400") << 400;
+        QTest::newRow("401") << 401;
+        QTest::newRow("403") << 403;
+        QTest::newRow("404") << 404;
+        QTest::newRow("500") << 500;
+        QTest::newRow("503") << 503;
+        QTest::newRow("Timeout") << static_cast<int>(Timeout);
+    }
+
     void testOpeningOnlineFileTriggersDownload()
     {
+        QFETCH(int, errorKind);
+
         FakeFolder fakeFolder{ FileInfo() };
         setupVfs(fakeFolder);
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
@@ -1116,6 +1137,21 @@ private slots:
 
         CFVERIFY_VIRTUAL(fakeFolder, "online/sub/file1");
 
+        // Setup error case if needed
+        if (errorKind == Timeout) {
+            fakeFolder.setServerOverride([&](QNetworkAccessManager::Operation op, const QNetworkRequest &req, QIODevice *) -> QNetworkReply * {
+                if (req.url().path().endsWith("online/sub/file1")) {
+                    return new FakeHangingReply(op, req, this);
+                }
+                return nullptr;
+            });
+        } else if (errorKind != NoError) {
+            fakeFolder.serverErrorPaths().append("online/sub/file1", errorKind);
+        }
+
+        // So the test that test timeout finishes fast
+        QScopedValueRollback<int> setHttpTimeout(AbstractNetworkJob::httpTimeout, errorKind == Timeout ? 1 : 10000);
+
         // Simulate another process requesting the open
         QEventLoop loop;
         bool openResult = false;
@@ -1130,14 +1166,22 @@ private slots:
         loop.exec();
         t.join();
 
-        CFVERIFY_NONVIRTUAL(fakeFolder, "online/sub/file1");
+        if (errorKind == NoError) {
+            CFVERIFY_NONVIRTUAL(fakeFolder, "online/sub/file1");
+        } else {
+            CFVERIFY_VIRTUAL(fakeFolder, "online/sub/file1");
+        }
 
         // Nothing should change
         ItemCompletedSpy completeSpy(fakeFolder);
         QVERIFY(fakeFolder.syncOnce());
         QVERIFY(completeSpy.isEmpty());
 
-        CFVERIFY_NONVIRTUAL(fakeFolder, "online/sub/file1");
+        if (errorKind == NoError) {
+            CFVERIFY_NONVIRTUAL(fakeFolder, "online/sub/file1");
+        } else {
+            CFVERIFY_VIRTUAL(fakeFolder, "online/sub/file1");
+        }
     }
 };