Propagation: Fix delete-before-rename bug #7441

By introducing a PropagateRootDirectory job that explicitly
separates the directory deletion jobs from all the other jobs.

Note that this means that if there are errors in subJobs the
dirDeletionJobs won't get executed.

diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp
index 856b2e25fa162aa5dee8622dbea81f6e70752112..3fbbd8f035ad8532e963160386a15b8483060944 100644 (file)
--- a/src/libsync/owncloudpropagator.cpp
+++ b/src/libsync/owncloudpropagator.cpp
@@ -373,7 +373,7 @@ void OwncloudPropagator::start(const SyncFileItemVector &items)
      * In order to do that we loop over the items. (which are sorted by destination)
      * When we enter a directory, we can create the directory job and push it on the stack. */
 
-    _rootJob.reset(new PropagateDirectory(this));
+    _rootJob.reset(new PropagateRootDirectory(this));
     QStack<QPair<QString /* directory name */, PropagateDirectory * /* job */>> directories;
     directories.push(qMakePair(QString(), _rootJob.data()));
     QVector<PropagatorJob *> directoriesToRemove;
@@ -482,7 +482,7 @@ void OwncloudPropagator::start(const SyncFileItemVector &items)
     }
 
     foreach (PropagatorJob *it, directoriesToRemove) {
-        _rootJob->appendJob(it);
+        _rootJob->_dirDeletionJobs.appendJob(it);
     }
 
     connect(_rootJob.data(), &PropagatorJob::finished, this, &OwncloudPropagator::emitFinished);
@@ -993,6 +993,91 @@ void PropagateDirectory::slotSubJobsFinished(SyncFileItem::Status status)
     emit finished(status);
 }
 
+PropagateRootDirectory::PropagateRootDirectory(OwncloudPropagator *propagator)
+    : PropagateDirectory(propagator, SyncFileItemPtr(new SyncFileItem))
+    , _dirDeletionJobs(propagator)
+{
+    connect(&_dirDeletionJobs, &PropagatorJob::finished, this, &PropagateRootDirectory::slotDirDeletionJobsFinished);
+}
+
+PropagatorJob::JobParallelism PropagateRootDirectory::parallelism()
+{
+    // the root directory parallelism isn't important
+    return WaitForFinished;
+}
+
+void PropagateRootDirectory::abort(PropagatorJob::AbortType abortType)
+{
+    if (_firstJob)
+        // Force first job to abort synchronously
+        // even if caller allows async abort (asyncAbort)
+        _firstJob->abort(AbortType::Synchronous);
+
+    if (abortType == AbortType::Asynchronous) {
+        struct AbortsFinished {
+            bool subJobsFinished = false;
+            bool dirDeletionFinished = false;
+        };
+        auto abortStatus = QSharedPointer<AbortsFinished>(new AbortsFinished);
+
+        connect(&_subJobs, &PropagatorCompositeJob::abortFinished, this, [this, abortStatus]() {
+            abortStatus->subJobsFinished = true;
+            if (abortStatus->subJobsFinished && abortStatus->dirDeletionFinished)
+                emit abortFinished();
+        });
+        connect(&_dirDeletionJobs, &PropagatorCompositeJob::abortFinished, this, [this, abortStatus]() {
+            abortStatus->dirDeletionFinished = true;
+            if (abortStatus->subJobsFinished && abortStatus->dirDeletionFinished)
+                emit abortFinished();
+        });
+    }
+    _subJobs.abort(abortType);
+    _dirDeletionJobs.abort(abortType);
+}
+
+qint64 PropagateRootDirectory::committedDiskSpace() const
+{
+    return _subJobs.committedDiskSpace() + _dirDeletionJobs.committedDiskSpace();
+}
+
+bool PropagateRootDirectory::scheduleSelfOrChild()
+{
+    if (_state == Finished)
+        return false;
+
+    if (PropagateDirectory::scheduleSelfOrChild())
+        return true;
+
+    // Important: Finish _subJobs before scheduling any deletes.
+    if (_subJobs._state != Finished)
+        return false;
+
+    return _dirDeletionJobs.scheduleSelfOrChild();
+}
+
+void PropagateRootDirectory::slotSubJobsFinished(SyncFileItem::Status status)
+{
+    if (status != SyncFileItem::Success
+        && status != SyncFileItem::Restoration
+        && status != SyncFileItem::Conflict) {
+        if (_state != Finished) {
+            // Synchronously abort
+            abort(AbortType::Synchronous);
+            _state = Finished;
+            emit finished(status);
+        }
+        return;
+    }
+
+    propagator()->scheduleNextJob();
+}
+
+void PropagateRootDirectory::slotDirDeletionJobsFinished(SyncFileItem::Status status)
+{
+    _state = Finished;
+    emit finished(status);
+}
+
 // ================================================================================
 
 CleanupPollsJob::~CleanupPollsJob() = default;

diff --git a/src/libsync/owncloudpropagator.h b/src/libsync/owncloudpropagator.h
index c5431f051f6538ac4e68734284223591c260a8ea..8c874a019d1b91f3a630fa7a62841fdbfa73097e 100644 (file)
--- a/src/libsync/owncloudpropagator.h
+++ b/src/libsync/owncloudpropagator.h
@@ -289,7 +289,7 @@ public:
 
     PropagatorCompositeJob _subJobs;
 
-    explicit PropagateDirectory(OwncloudPropagator *propagator, const SyncFileItemPtr &item = SyncFileItemPtr(new SyncFileItem));
+    explicit PropagateDirectory(OwncloudPropagator *propagator, const SyncFileItemPtr &item);
 
     void appendJob(PropagatorJob *job)
     {
@@ -330,10 +330,35 @@ public:
 private slots:
 
     void slotFirstJobFinished(SyncFileItem::Status status);
-    void slotSubJobsFinished(SyncFileItem::Status status);
+    virtual void slotSubJobsFinished(SyncFileItem::Status status);
 
 };
 
+/**
+ * @brief Propagate the root directory, and all its sub entries.
+ * @ingroup libsync
+ *
+ * Primary difference to PropagateDirectory is that it keeps track of directory
+ * deletions that must happen at the very end.
+ */
+class OWNCLOUDSYNC_EXPORT PropagateRootDirectory : public PropagateDirectory
+{
+    Q_OBJECT
+public:
+    PropagatorCompositeJob _dirDeletionJobs;
+
+    explicit PropagateRootDirectory(OwncloudPropagator *propagator);
+
+    bool scheduleSelfOrChild() override;
+    JobParallelism parallelism() override;
+    void abort(PropagatorJob::AbortType abortType) override;
+
+    qint64 committedDiskSpace() const override;
+
+private slots:
+    void slotSubJobsFinished(SyncFileItem::Status status) override;
+    void slotDirDeletionJobsFinished(SyncFileItem::Status status);
+};
 
 /**
  * @brief Dummy job that just mark it as completed and ignored
@@ -565,7 +590,7 @@ signals:
 
 private:
     AccountPtr _account;
-    QScopedPointer<PropagateDirectory> _rootJob;
+    QScopedPointer<PropagateRootDirectory> _rootJob;
     SyncOptions _syncOptions;
     bool _jobScheduled = false;
 };

diff --git a/test/syncenginetestutils.h b/test/syncenginetestutils.h
index 8f349adc872d113e28eab31dbcce9e2806771743..276e3443525467a633b5383494bccd8aaf4febff 100644 (file)
--- a/test/syncenginetestutils.h
+++ b/test/syncenginetestutils.h
@@ -1190,20 +1190,26 @@ namespace OCC {
 inline void addFiles(QStringList &dest, const FileInfo &fi)
 {
     if (fi.isDir) {
-        dest += QString("%1 - dir").arg(fi.name);
+        dest += QString("%1 - dir").arg(fi.path());
         foreach (const FileInfo &fi, fi.children)
             addFiles(dest, fi);
     } else {
-        dest += QString("%1 - %2 %3-bytes").arg(fi.name).arg(fi.size).arg(fi.contentChar);
+        dest += QString("%1 - %2 %3-bytes").arg(fi.path()).arg(fi.size).arg(fi.contentChar);
     }
 }
 
-inline char *toString(const FileInfo &fi)
+inline QString toStringNoElide(const FileInfo &fi)
 {
     QStringList files;
     foreach (const FileInfo &fi, fi.children)
         addFiles(files, fi);
-    return QTest::toString(QString("FileInfo with %1 files(%2)").arg(files.size()).arg(files.join(", ")));
+    files.sort();
+    return QString("FileInfo with %1 files(\n\t%2\n)").arg(files.size()).arg(files.join("\n\t"));
+}
+
+inline char *toString(const FileInfo &fi)
+{
+    return QTest::toString(toStringNoElide(fi));
 }
 
 inline void addFilesDbData(QStringList &dest, const FileInfo &fi)

diff --git a/test/testpermissions.cpp b/test/testpermissions.cpp
index 7cab633594bad3df2e0f70b4f3d8dfce117b962f..ad02ca0a9bb8b34d4d638be36f8b2b1ec9007ee0 100644 (file)
--- a/test/testpermissions.cpp
+++ b/test/testpermissions.cpp
@@ -243,10 +243,13 @@ private slots:
         //2.
         // old removed
         QVERIFY(!currentLocalState.find("normalDirectory_PERM_CKDNV_/subdir_PERM_CKDNV_"));
+        // but still on the server: the rename causing an error meant the deletes didn't execute
+        QVERIFY(fakeFolder.currentRemoteState().find("normalDirectory_PERM_CKDNV_/subdir_PERM_CKDNV_"));
         // new still there
         QVERIFY(currentLocalState.find("readonlyDirectory_PERM_M_/moved_PERM_CK_/subsubdir_PERM_CKDNV_/normalFile_PERM_WVND_.data" ));
         //but not on server
         fakeFolder.localModifier().remove("readonlyDirectory_PERM_M_/moved_PERM_CK_");
+        fakeFolder.remoteModifier().remove("normalDirectory_PERM_CKDNV_/subdir_PERM_CKDNV_");
 
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
 

diff --git a/test/testsyncmove.cpp b/test/testsyncmove.cpp
index 390dd01041641f37eec14ef3c3ce015b25e7daaf..43206d96fcaa159fcd3c3bb9372a1514d52eb1e4 100644 (file)
--- a/test/testsyncmove.cpp
+++ b/test/testsyncmove.cpp
@@ -816,6 +816,23 @@ private slots:
         QCOMPARE(counter.nPUT, 0);
         QCOMPARE(counter.nMOVE, 2);
     }
+
+    // Test that deletes don't run before renames
+    void testRenameParallelism()
+    {
+        FakeFolder fakeFolder{ FileInfo{} };
+        fakeFolder.remoteModifier().mkdir("A");
+        fakeFolder.remoteModifier().insert("A/file");
+        QVERIFY(fakeFolder.syncOnce());
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+
+        fakeFolder.localModifier().mkdir("B");
+        fakeFolder.localModifier().rename("A/file", "B/file");
+        fakeFolder.localModifier().remove("A");
+
+        QVERIFY(fakeFolder.syncOnce());
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+    }
 };
 
 QTEST_GUILESS_MAIN(TestSyncMove)