From 976dbd6df6099f12d8f8cd6d1b8b8190351dad1c Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Mon, 19 Feb 2024 14:46:52 +0100 Subject: [PATCH 1/3] when moving a file, checks that it exists at origin or destination current automated tests are never breaking the rule that a file that should be moved on client side (after a move was done server side) exists either at teh original path or the destination path. in practice it may happen (and I manually tested it) thet the sync engine decides to move a single file in two distinct places that decision will violate this rule and a debug build would then run into the assert Close #6462 Signed-off-by: Matthieu Gallien --- src/libsync/propagatorjobs.cpp | 5 +++-- test/testsyncmove.cpp | 23 ++++++++++++++++++++++- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/libsync/propagatorjobs.cpp b/src/libsync/propagatorjobs.cpp index 9749f5a1d0b79..82167f5b9c817 100644 --- a/src/libsync/propagatorjobs.cpp +++ b/src/libsync/propagatorjobs.cpp @@ -224,10 +224,10 @@ void PropagateLocalRename::start() auto &vfs = propagator()->syncOptions()._vfs; const auto previousNameInDb = propagator()->adjustRenamedPath(_item->_file); - const auto existingFile = propagator()->fullLocalPath(propagator()->adjustRenamedPath(_item->_file)); + const auto existingFile = propagator()->fullLocalPath(previousNameInDb); const auto targetFile = propagator()->fullLocalPath(_item->_renameTarget); - const auto fileAlreadyMoved = !QFileInfo::exists(propagator()->fullLocalPath(_item->_originalFile)); + const auto fileAlreadyMoved = !QFileInfo::exists(propagator()->fullLocalPath(_item->_originalFile)) && QFileInfo::exists(existingFile); auto pinState = OCC::PinState::Unspecified; if (!fileAlreadyMoved) { auto pinStateResult = vfs->pinState(propagator()->adjustRenamedPath(_item->_file)); @@ -239,6 +239,7 @@ void PropagateLocalRename::start() // if the file is a file underneath a moved dir, the _item->file is equal // to _item->renameTarget and the file is not moved as a result. qCDebug(lcPropagateLocalRename) << _item->_file << _item->_renameTarget << _item->_originalFile << previousNameInDb << (fileAlreadyMoved ? "original file has already moved" : "original file is still there"); + Q_ASSERT(QFileInfo::exists(propagator()->fullLocalPath(_item->_originalFile)) || QFileInfo::exists(existingFile)); if (_item->_file != _item->_renameTarget) { propagator()->reportProgress(*_item, 0); qCDebug(lcPropagateLocalRename) << "MOVE " << existingFile << " => " << targetFile; diff --git a/test/testsyncmove.cpp b/test/testsyncmove.cpp index 04f29f8bebf96..8ad19815524ca 100644 --- a/test/testsyncmove.cpp +++ b/test/testsyncmove.cpp @@ -84,6 +84,12 @@ class TestSyncMove : public QObject Q_OBJECT private slots: + void initTestCase() + { + Logger::instance()->setLogFlush(true); + Logger::instance()->setLogDebug(true); + } + void testMoveCustomRemoteRoot() { FileInfo subFolder(QStringLiteral("AS"), { { QStringLiteral("f1"), 4 } }); @@ -140,7 +146,22 @@ private slots: void testSelectiveSyncMovedFolder() { // issue #5224 - FakeFolder fakeFolder{ FileInfo{ QString(), { FileInfo{ QStringLiteral("parentFolder"), { FileInfo{ QStringLiteral("subFolderA"), { { QStringLiteral("fileA.txt"), 400 } } }, FileInfo{ QStringLiteral("subFolderB"), { { QStringLiteral("fileB.txt"), 400 } } } } } } } }; + FakeFolder fakeFolder{ + FileInfo{QString(), { + FileInfo{QStringLiteral("parentFolder"), { + FileInfo{QStringLiteral("subFolderA"), { + {QStringLiteral("fileA.txt"), 400} + } + }, + FileInfo{QStringLiteral("subFolderB"), { + {QStringLiteral("fileB.txt"), 400} + } + } + } + } + } + } + }; QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); auto expectedServerState = fakeFolder.currentRemoteState(); From b35a26091b3db06db52b51b90cc9b8b3fcc7b3ba Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Tue, 5 Mar 2024 08:55:13 +0100 Subject: [PATCH 2/3] add new test to simulate the data loss scenario this new test trigger the assert that a file is either in the old place or the new place when we execute a MOVE instruction for a local file in the test one file is discovered as in need of a local MOVE but will be missing from the old and new places when running the propagation due to a bug Signed-off-by: Matthieu Gallien --- test/testsyncmove.cpp | 60 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/test/testsyncmove.cpp b/test/testsyncmove.cpp index 8ad19815524ca..f4926722f681f 100644 --- a/test/testsyncmove.cpp +++ b/test/testsyncmove.cpp @@ -1063,6 +1063,66 @@ private slots: QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); } + + void testRenameSameFileInMultiplePaths() + { + FakeFolder fakeFolder{FileInfo{}}; + + fakeFolder.remoteModifier().mkdir("FolderA"); + fakeFolder.remoteModifier().mkdir("FolderA/folderParent"); + fakeFolder.remoteModifier().mkdir("FolderB"); + fakeFolder.remoteModifier().mkdir("FolderB/folderChild"); + fakeFolder.remoteModifier().insert("FolderB/folderChild/FileA.txt"); + fakeFolder.remoteModifier().mkdir("FolderC"); + + const auto folderParentFileInfo = fakeFolder.remoteModifier().find("FolderA/folderParent"); + const auto folderParentSharedFolderFileId = folderParentFileInfo->fileId; + const auto folderParentSharedFolderEtag = folderParentFileInfo->etag; + const auto folderChildFileInfo = fakeFolder.remoteModifier().find("FolderB/folderChild"); + const auto folderChildInFolderAFolderFileId = folderChildFileInfo->fileId; + const auto folderChildInFolderAEtag = folderChildFileInfo->etag; + const auto fileAFileInfo = fakeFolder.remoteModifier().find("FolderB/folderChild/FileA.txt"); + const auto fileAInFolderAFolderFileId = fileAFileInfo->fileId; + const auto fileAInFolderAEtag = fileAFileInfo->etag; + + auto folderCFileInfo = fakeFolder.remoteModifier().find("FolderC"); + folderCFileInfo->fileId = folderParentSharedFolderFileId; + folderCFileInfo->etag = folderParentSharedFolderEtag; + + QVERIFY(fakeFolder.syncOnce()); + + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + + fakeFolder.remoteModifier().remove("FolderB/folderChild"); + fakeFolder.remoteModifier().mkdir("FolderA/folderParent/folderChild"); + fakeFolder.remoteModifier().insert("FolderA/folderParent/folderChild/FileA.txt"); + fakeFolder.remoteModifier().mkdir("FolderC/folderChild"); + fakeFolder.remoteModifier().insert("FolderC/folderChild/FileA.txt"); + + auto folderChildInFolderParentFileInfo = fakeFolder.remoteModifier().find("FolderA/folderParent/folderChild"); + folderChildInFolderParentFileInfo->fileId = folderChildInFolderAFolderFileId; + folderChildInFolderParentFileInfo->etag = folderChildInFolderAEtag; + + auto fileAInFolderParentFileInfo = fakeFolder.remoteModifier().find("FolderA/folderParent/folderChild/FileA.txt"); + fileAInFolderParentFileInfo->fileId = fileAInFolderAFolderFileId; + fileAInFolderParentFileInfo->etag = fileAInFolderAEtag; + + auto folderChildInFolderCFileInfo = fakeFolder.remoteModifier().find("FolderC/folderChild"); + folderChildInFolderCFileInfo->fileId = folderChildInFolderAFolderFileId; + folderChildInFolderCFileInfo->etag = folderChildInFolderAEtag; + + auto fileAInFolderCFileInfo = fakeFolder.remoteModifier().find("FolderC/folderChild/FileA.txt"); + fileAInFolderCFileInfo->fileId = fileAInFolderAFolderFileId; + fileAInFolderCFileInfo->etag = fileAInFolderAEtag; + + fakeFolder.syncEngine().setLocalDiscoveryOptions(OCC::LocalDiscoveryStyle::FilesystemOnly); + QVERIFY(fakeFolder.syncOnce()); + + fakeFolder.syncEngine().setLocalDiscoveryOptions(OCC::LocalDiscoveryStyle::FilesystemOnly); + QVERIFY(fakeFolder.syncOnce()); + + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + } }; QTEST_GUILESS_MAIN(TestSyncMove) From b7c1a95d1cb3ba7800e79f8272d065eb674e9f54 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Tue, 5 Mar 2024 12:09:45 +0100 Subject: [PATCH 3/3] fix missing tracking for some item rename operations will fix mishandling of rename of a single file to multiple places during discovery Signed-off-by: Matthieu Gallien --- src/libsync/discovery.cpp | 3 +++ src/libsync/discoveryphase.cpp | 5 +++++ src/libsync/discoveryphase.h | 2 +- 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp index 4326295c93930..a9fb2d63b2f14 100644 --- a/src/libsync/discovery.cpp +++ b/src/libsync/discovery.cpp @@ -1666,6 +1666,9 @@ void ProcessDirectoryJob::processFileFinalize( ASSERT(_dirItem && _dirItem->_instruction == CSYNC_INSTRUCTION_RENAME); // This is because otherwise subitems are not updated! (ideally renaming a directory could // update the database for all items! See PropagateDirectory::slotSubJobsFinished) + const auto adjustedOriginalPath = _discoveryData->adjustRenamedPath(path._original, SyncFileItem::Down); + Q_UNUSED(adjustedOriginalPath) + _discoveryData->_renamedItemsLocal.insert(path._original, path._target); item->_instruction = CSYNC_INSTRUCTION_RENAME; item->_renameTarget = path._target; item->_direction = _dirItem->_direction; diff --git a/src/libsync/discoveryphase.cpp b/src/libsync/discoveryphase.cpp index 7ae2022841803..ce492d9726299 100644 --- a/src/libsync/discoveryphase.cpp +++ b/src/libsync/discoveryphase.cpp @@ -261,6 +261,11 @@ void DiscoveryPhase::setSelectiveSyncWhiteList(const QStringList &list) _selectiveSyncWhiteList.sort(); } +bool DiscoveryPhase::isRenamed(const QString &p) const +{ + return _renamedItemsLocal.contains(p) || _renamedItemsRemote.contains(p); +} + void DiscoveryPhase::scheduleMoreJobs() { auto limit = qMax(1, _syncOptions._parallelNetworkJobs); diff --git a/src/libsync/discoveryphase.h b/src/libsync/discoveryphase.h index ccad6bb92a361..4d6dcea40644f 100644 --- a/src/libsync/discoveryphase.h +++ b/src/libsync/discoveryphase.h @@ -256,7 +256,7 @@ class DiscoveryPhase : public QObject * Useful for avoiding processing of items that have already been claimed in * a rename (would otherwise be discovered as deletions). */ - [[nodiscard]] bool isRenamed(const QString &p) const { return _renamedItemsLocal.contains(p) || _renamedItemsRemote.contains(p); } + [[nodiscard]] bool isRenamed(const QString &p) const; int _currentlyActiveJobs = 0;