ChunkingNG: remove stale files when resuming

src/libsync/propagateremotedelete.cpp

@@ -22,11 +22,14 @@ DeleteJob::DeleteJob(AccountPtr account, const QString& path, QObject* parent)
     : AbstractNetworkJob(account, path, parent)
 { }
 
+DeleteJob::DeleteJob(AccountPtr account, const QUrl& url, QObject* parent)
+    : AbstractNetworkJob(account, QString(), parent), _url(url)
+{ }
 
 void DeleteJob::start()
 {
     QNetworkRequest req;
-    setReply(davRequest("DELETE", path(), req));
+    setReply(_url.isValid() ? davRequest("DELETE", _url, req) : davRequest("DELETE", path(), req));
     setupConnections(reply());
 
     if( reply()->error() != QNetworkReply::NoError ) {

src/libsync/propagateremotedelete.h

@@ -24,8 +24,10 @@ namespace OCC {
  */
 class DeleteJob : public AbstractNetworkJob {
     Q_OBJECT
+    QUrl _url; // Only used if the constructor taking a url is taken.
 public:
     explicit DeleteJob(AccountPtr account, const QString& path, QObject* parent = 0);
+    explicit DeleteJob(AccountPtr account, const QUrl& url, QObject* parent = 0);
 
     void start() Q_DECL_OVERRIDE;
     bool finished() Q_DECL_OVERRIDE;

src/libsync/propagateupload.h

@@ -289,6 +289,7 @@ private:
     quint64 _sent; /// amount of data (bytes) that was already sent
     uint _transferId; /// transfer id (part of the url)
     int _currentChunk; /// Id of the next chunk that will be sent
+    bool _removeJobError; /// If not null, there was an error removing the job
 
     // Map chunk number with its size  from the PROPFIND on resume.
     // (Only used from slotPropfindIterate/slotPropfindFinished because the LsColJob use signals to report data.)
@@ -313,6 +314,7 @@ private slots:
     void slotPropfindFinished();
     void slotPropfindFinishedWithError();
     void slotPropfindIterate(const QString &name, const QMap<QString,QString> &properties);
+    void slotDeleteJobFinished();
     void slotMkColFinished(QNetworkReply::NetworkError);
     void slotPutFinished();
     void slotMoveJobFinished();

src/libsync/propagateuploadng.cpp

@@ -24,6 +24,8 @@
 #include "propagatorjobs.h"
 #include "syncengine.h"
 #include "propagateremotemove.h"
+#include "propagateremotedelete.h"
+
 
 #include <QNetworkAccessManager>
 #include <QFileInfo>
@@ -56,10 +58,13 @@ QUrl PropagateUploadFileNG::chunkUrl(int chunk)
        startNewUpload() <-+        +----------------------------\
           |               |        |                             \
          MKCOL            + slotPropfindFinishedWithError()     slotPropfindFinished()
-          |                                                       |
-      slotMkColFinished()                                         |
-          |                                                       |
-    +-----+-------------------------------------------------------+
+          |                                                       Is there stale files to remove?
+      slotMkColFinished()                                         |                      |
+          |                                                       no                    yes
+          |                                                       |                      |
+          |                                                       |                  DeleteJob
+          |                                                       |                      |
+    +-----+<------------------------------------------------------+<---  slotDeleteJobFinished()
     |
     +---->  startNextChunk()  ---finished?  --+
                   ^               |          |
@@ -119,12 +124,29 @@ void PropagateUploadFileNG::slotPropfindFinished()
     _sent = 0;
     while (_serverChunks.contains(_currentChunk)) {
         _sent += _serverChunks[_currentChunk];
+        _serverChunks.remove(_currentChunk);
         ++_currentChunk;
     }
-    // FIXME: we should make sure that if there is a "hole" and then a few more chunks, on the server
-    // we should remove the later chunks. Otherwise when we do dynamic chunk sizing, we may end up
-    // with corruptions if there are too many chunks, or if we abort and there are still stale chunks.
+
     qDebug() << "Resuming "<< _item->_file << " from chunk " << _currentChunk << "; sent ="<< _sent;
+
+    if (!_serverChunks.isEmpty()) {
+        qDebug() << "To Delete" << _serverChunks;
+        _propagator->_activeJobList.append(this);
+        _removeJobError = false;
+
+        // Make sure that if there is a "hole" and then a few more chunks, on the server
+        // we should remove the later chunks. Otherwise when we do dynamic chunk sizing, we may end up
+        // with corruptions if there are too many chunks, or if we abort and there are still stale chunks.
+        for (auto it = _serverChunks.begin(); it != _serverChunks.end(); ++it) {
+            auto job = new DeleteJob(_propagator->account(), Utility::concatUrlPath(chunkUrl(), QString::number(it.key())), this);
+            QObject::connect(job, SIGNAL(finishedSignal()), this, SLOT(slotDeleteJobFinished()));
+            _jobs.append(job);
+            job->start();
+        }
+        return;
+    }
+
     startNextChunk();
 }
 
@@ -144,6 +166,38 @@ void PropagateUploadFileNG::slotPropfindFinishedWithError()
     startNewUpload();
 }
 
+void PropagateUploadFileNG::slotDeleteJobFinished()
+{
+    auto job = qobject_cast<DeleteJob *>(sender());
+    Q_ASSERT(job);
+    _jobs.remove(_jobs.indexOf(job));
+
+    QNetworkReply::NetworkError err = job->reply()->error();
+    if (err != QNetworkReply::NoError && err != QNetworkReply::ContentNotFoundError) {
+        const int httpStatus = job->reply()->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt();
+        SyncFileItem::Status status = classifyError(err, httpStatus);
+        if (status == SyncFileItem::FatalError) {
+            abortWithError(status, job->errorString());
+            return;
+        } else {
+            qWarning() << "DeleteJob errored out" << job->errorString() << job->reply()->url();
+            _removeJobError = true;
+            // Let the other jobs finish
+        }
+    }
+
+    if (_jobs.isEmpty()) {
+        _propagator->_activeJobList.removeOne(this);
+        if (_removeJobError) {
+            // There was an error removing some files, just start over
+            startNewUpload();
+        } else {
+            startNextChunk();
+        }
+    }
+}
+
+
 
 void PropagateUploadFileNG::startNewUpload()
 {

test/syncenginetestutils.h

@@ -297,6 +297,12 @@ public:
         setOperation(op);
         open(QIODevice::ReadOnly);
 
+        QString fileName = getFilePathFromUrl(request.url());
+        Q_ASSERT(!fileName.isNull()); // for root, it should be empty
+        const FileInfo *fileInfo = remoteRootFileInfo.find(fileName);
+        Q_ASSERT(fileInfo);
+        QString prefix = request.url().path().left(request.url().path().size() - fileName.size());
+
         // Don't care about the request and just return a full propfind
         const QString davUri{QStringLiteral("DAV:")};
         const QString ocUri{QStringLiteral("http://owncloud.org/ns")};
@@ -310,7 +316,7 @@ public:
         auto writeFileResponse = [&](const FileInfo &fileInfo) {
             xml.writeStartElement(davUri, QStringLiteral("response"));
 
-            xml.writeTextElement(davUri, QStringLiteral("href"), "/owncloud/remote.php/webdav/" + fileInfo.path());
+            xml.writeTextElement(davUri, QStringLiteral("href"), prefix + fileInfo.path());
             xml.writeStartElement(davUri, QStringLiteral("propstat"));
             xml.writeStartElement(davUri, QStringLiteral("prop"));
 
@@ -334,11 +340,6 @@ public:
             xml.writeEndElement(); // response
         };
 
-        QString fileName = getFilePathFromUrl(request.url());
-        Q_ASSERT(!fileName.isNull()); // for root, it should be empty
-        const FileInfo *fileInfo = remoteRootFileInfo.find(fileName);
-        Q_ASSERT(fileInfo);
-
         writeFileResponse(*fileInfo);
         foreach(const FileInfo &childFileInfo, fileInfo->children)
            writeFileResponse(childFileInfo);
@@ -400,6 +401,7 @@ public:
     }
 
     Q_INVOKABLE void respond() {
+        emit uploadProgress(fileInfo->size, fileInfo->size);
         setRawHeader("OC-ETag", fileInfo->etag.toLatin1());
         setRawHeader("ETag", fileInfo->etag.toLatin1());
         setRawHeader("X-OC-MTime", "accepted"); // Prevents Q_ASSERT(!_runningNow) since we'll call PropagateItemJob::done twice in that case.
@@ -583,6 +585,7 @@ public:
         } while(true);
 
         Q_ASSERT(count > 1); // There should be at least two chunks, otherwise why would we use chunking?
+        QCOMPARE(sourceFolder->children.count(), count); // There should not be holes or extra files
 
         QString fileName = getFilePathFromUrl(QUrl::fromEncoded(request.rawHeader("Destination")));
         Q_ASSERT(!fileName.isEmpty());
@@ -750,7 +753,7 @@ public:
     }
 
     FileInfo currentRemoteState() { return _fakeQnam->currentRemoteState(); }
-    FileInfo uploadState() { return _fakeQnam->uploadState(); }
+    FileInfo &uploadState() { return _fakeQnam->uploadState(); }
 
     QStringList &serverErrorPaths() { return _fakeQnam->errorPaths(); }
 

test/testchunkingng.cpp

@@ -25,7 +25,7 @@ private slots:
         QVERIFY(fakeFolder.syncOnce());
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
         QCOMPARE(fakeFolder.uploadState().children.count(), 1); // the transfer was done with chunking
-        QCOMPARE(fakeFolder.currentLocalState().find("A/a0")->size, size);
+        QCOMPARE(fakeFolder.currentRemoteState().find("A/a0")->size, size);
 
         // Check that another upload of the same file also work.
         fakeFolder.localModifier().appendByte("A/a0");
@@ -33,6 +33,45 @@ private slots:
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
         QCOMPARE(fakeFolder.uploadState().children.count(), 2); // the transfer was done with chunking
     }
+
+
+    void testResume () {
+
+        FakeFolder fakeFolder{FileInfo::A12_B12_C12_S12()};
+        fakeFolder.syncEngine().account()->setCapabilities({ { "dav", QVariantMap{ {"chunking", "1.0"} } } });
+        const int size = 300 * 1000 * 1000; // 300 MB
+        fakeFolder.localModifier().insert("A/a0", size);
+
+        // Abort when the upload is at 1/3
+        int sizeWhenAbort = -1;
+        auto con = QObject::connect(&fakeFolder.syncEngine(),  &SyncEngine::transmissionProgress,
+                                    [&](const ProgressInfo &progress) {
+                if (progress.completedSize() > (progress.totalSize() /3 )) {
+                    sizeWhenAbort = progress.completedSize();
+                    fakeFolder.syncEngine().abort();
+                }
+        });
+
+        QVERIFY(!fakeFolder.syncOnce()); // there should have been an error
+        QObject::disconnect(con);
+        QVERIFY(sizeWhenAbort > 0);
+        QVERIFY(sizeWhenAbort < size);
+        QCOMPARE(fakeFolder.uploadState().children.count(), 1); // the transfer was done with chunking
+        auto upStateChildren = fakeFolder.uploadState().children.first().children;
+        QCOMPARE(sizeWhenAbort, std::accumulate(upStateChildren.cbegin(), upStateChildren.cend(), 0,
+                                               [](int s, const FileInfo &i) { return s + i.size; }));
+
+
+        // Add a fake file to make sure it gets deleted
+        fakeFolder.uploadState().children.first().insert("10000", size);
+        QVERIFY(fakeFolder.syncOnce());
+
+
+
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+        QCOMPARE(fakeFolder.uploadState().children.count(), 1); // The same chunk id was re-used
+        QCOMPARE(fakeFolder.currentRemoteState().find("A/a0")->size, size);
+    }
 };
 
 QTEST_GUILESS_MAIN(TestChunkingNG)