From 9c73c97c920c81b6b72bb1f3e02d203cd6e81161 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Tue, 22 Apr 2025 16:02:12 +0200 Subject: [PATCH 1/5] perf(discovery): optimize empty strings Signed-off-by: Matthieu Gallien --- src/libsync/discovery.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp index b3fedf6da4c35..8ef2238ae8481 100644 --- a/src/libsync/discovery.cpp +++ b/src/libsync/discovery.cpp @@ -573,8 +573,8 @@ void ProcessDirectoryJob::processFile(PathTuple path, const auto hasLocal = localEntry.isValid() ? "true" : _queryLocal == ParentNotChanged ? "db" : "false"; const auto serverFileIsLocked = (serverEntry.isValid() ? (serverEntry.locked == SyncFileItem::LockStatus::LockedItem ? "locked" : "not locked") : ""); const auto localFileIsLocked = dbEntry._lockstate._locked ? "locked" : "not locked"; - const auto serverFileLockType = serverEntry.isValid() ? QString::number(static_cast(serverEntry.lockOwnerType)) : QStringLiteral(""); - const auto localFileLockType = dbEntry._lockstate._locked ? QString::number(static_cast(dbEntry._lockstate._lockOwnerType)) : QStringLiteral(""); + const auto serverFileLockType = serverEntry.isValid() ? QString::number(static_cast(serverEntry.lockOwnerType)) : QString{}; + const auto localFileLockType = dbEntry._lockstate._locked ? QString::number(static_cast(dbEntry._lockstate._lockOwnerType)) : QString{}; QString processingLog; QDebug deleteLogger{&processingLog}; From ed48808fd9171d00ca65f12edaa0335640f188a9 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Tue, 22 Apr 2025 16:03:05 +0200 Subject: [PATCH 2/5] feat(discovery): add missing inode info for discovered files Signed-off-by: Matthieu Gallien --- src/libsync/discovery.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp index 8ef2238ae8481..ec86e7f4e3363 100644 --- a/src/libsync/discovery.cpp +++ b/src/libsync/discovery.cpp @@ -587,6 +587,7 @@ void ProcessDirectoryJob::processFile(PathTuple path, << " | checksum: " << dbEntry._checksumHeader << "//" << serverEntry.checksumHeader << " | perm: " << dbEntry._remotePerm << "//" << serverEntry.remotePerm << " | fileid: " << dbEntry._fileId << "//" << serverEntry.fileId + << " | inode: " << dbEntry._inode << "/" << localEntry.inode << "/" << " | type: " << dbEntry._type << "/" << localEntry.type << "/" << (serverEntry.isDirectory ? ItemTypeDirectory : ItemTypeFile) << " | e2ee: " << dbEntry.isE2eEncrypted() << "/" << serverEntry.isE2eEncrypted() << " | e2eeMangledName: " << dbEntry.e2eMangledName() << "/" << serverEntry.e2eMangledName From 5bd6a25bb80dc7bacef454a15a4f8a61d41e3889 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Tue, 22 Apr 2025 16:03:29 +0200 Subject: [PATCH 3/5] feat(discovery): add more info for renamed items add original name add renamed name add target name should make it easier to analyze rename operations Signed-off-by: Matthieu Gallien --- src/libsync/syncfilestatustracker.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/libsync/syncfilestatustracker.cpp b/src/libsync/syncfilestatustracker.cpp index d1ee8a06f3a96..d671389414dab 100644 --- a/src/libsync/syncfilestatustracker.cpp +++ b/src/libsync/syncfilestatustracker.cpp @@ -236,7 +236,11 @@ void SyncFileStatusTracker::slotAboutToPropagate(SyncFileItemVector &items) std::swap(_syncProblems, oldProblems); for (const auto &item : std::as_const(items)) { - qCInfo(lcStatusTracker) << "Investigating" << item->destination() << item->_status << item->_instruction << item->_direction; + if (item->_instruction == CSyncEnums::CSYNC_INSTRUCTION_RENAME) { + qCInfo(lcStatusTracker) << "Investigating" << item->destination() << item->_status << item->_instruction << item->_direction << item->_file << item->_originalFile << item->_renameTarget; + } else { + qCInfo(lcStatusTracker) << "Investigating" << item->destination() << item->_status << item->_instruction << item->_direction; + } _dirtyPaths.remove(item->destination()); if (hasErrorStatus(*item)) { From 47b3a5c8ca5a4c50dcf48b73275555db790342e3 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Wed, 23 Apr 2025 11:54:34 +0200 Subject: [PATCH 4/5] fix(rename): handle complex rename/move scenario ensure we do not leak records and properly update them in client database Signed-off-by: Matthieu Gallien --- src/libsync/propagatorjobs.cpp | 28 ++++++++++------- test/testsyncmove.cpp | 55 ++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 11 deletions(-) diff --git a/src/libsync/propagatorjobs.cpp b/src/libsync/propagatorjobs.cpp index 002480c82705d..59b7ec9c98dfa 100644 --- a/src/libsync/propagatorjobs.cpp +++ b/src/libsync/propagatorjobs.cpp @@ -302,8 +302,9 @@ void PropagateLocalRename::start() const auto previousNameInDb = propagator()->adjustRenamedPath(_item->_file); const auto existingFile = propagator()->fullLocalPath(previousNameInDb); const auto targetFile = propagator()->fullLocalPath(_item->_renameTarget); + const auto originalFile = propagator()->fullLocalPath(_item->_originalFile); - const auto fileAlreadyMoved = !FileSystem::fileExists(propagator()->fullLocalPath(_item->_originalFile)) && FileSystem::fileExists(existingFile); + const auto fileAlreadyMoved = (!FileSystem::fileExists(originalFile) || !FileSystem::fileExists(existingFile))&& FileSystem::fileExists(targetFile); auto pinState = OCC::PinState::Unspecified; if (!fileAlreadyMoved) { auto pinStateResult = vfs->pinState(propagator()->adjustRenamedPath(_item->_file)); @@ -315,6 +316,8 @@ 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"); + qCDebug(lcPropagateLocalRename()) << (FileSystem::fileExists(originalFile) ? "original file exists" : "orignal file is missing") << originalFile << _item->_originalFile; + qCDebug(lcPropagateLocalRename()) << (FileSystem::fileExists(existingFile) ? "existing file exists" : "existing file is missing") << existingFile << previousNameInDb; Q_ASSERT(FileSystem::fileExists(propagator()->fullLocalPath(_item->_originalFile)) || FileSystem::fileExists(existingFile)); if (_item->_file != _item->_renameTarget) { propagator()->reportProgress(*_item, 0); @@ -444,7 +447,7 @@ void PropagateLocalRename::start() if (fileAlreadyMoved && !deleteOldDbRecord(previousNameInDb)) { return; - } else if (!deleteOldDbRecord(_item->_originalFile)) { + } else if (!deleteOldDbRecord(previousNameInDb)) { qCWarning(lcPropagateLocalRename) << "Could not delete file from local DB" << _item->_originalFile; return; } @@ -470,25 +473,28 @@ void PropagateLocalRename::start() done(SyncFileItem::SoftError, tr("The file %1 is currently in use").arg(newItem._file), ErrorCategory::GenericError); return; } - } else { - const auto dbQueryResult = propagator()->_journal->getFilesBelowPath(oldFile.toUtf8(), [oldFile, this] (const SyncJournalFileRecord &record) -> void { - const auto oldFileName = record._path; - const auto oldFileNameString = QString::fromUtf8(oldFileName); + } else if (!fileAlreadyMoved) { + qCDebug(lcPropagateLocalRename) << "propagate child items after move from" << existingFile << "to" << targetFile; + const auto dbQueryResult = propagator()->_journal->getFilesBelowPath(previousNameInDb.toUtf8(), [previousNameInDb, this] (const SyncJournalFileRecord &record) -> void { + const auto oldFileNameString = propagator()->adjustRenamedPath(QString::fromUtf8(record._path)); auto newFileNameString = oldFileNameString; - newFileNameString.replace(0, oldFile.length(), _item->_renameTarget); + newFileNameString.replace(0, previousNameInDb.length(), _item->_renameTarget); + + qCDebug(lcPropagateLocalRename) << "child rename from" << oldFileNameString << "to" << newFileNameString; if (oldFileNameString == newFileNameString) { + Q_ASSERT(false); return; } SyncJournalFileRecord oldRecord; - if (!propagator()->_journal->getFileRecord(oldFileName, &oldRecord)) { - qCWarning(lcPropagateLocalRename) << "Could not get file from local DB" << oldFileName; + if (!propagator()->_journal->getFileRecord(oldFileNameString, &oldRecord)) { + qCWarning(lcPropagateLocalRename) << "Could not get file from local DB" << oldFileNameString; done(SyncFileItem::NormalError, tr("Could not get file %1 from local DB").arg(oldFileNameString), OCC::ErrorCategory::GenericError); return; } - if (!propagator()->_journal->deleteFileRecord(oldFileName)) { - qCWarning(lcPropagateLocalRename) << "could not delete file from local DB" << oldFileName; + if (!propagator()->_journal->deleteFileRecord(oldFileNameString)) { + qCWarning(lcPropagateLocalRename) << "could not delete file from local DB" << oldFileNameString; done(SyncFileItem::NormalError, tr("Could not delete file record %1 from local DB").arg(oldFileNameString), OCC::ErrorCategory::GenericError); return; } diff --git a/test/testsyncmove.cpp b/test/testsyncmove.cpp index 4f775c7fc9fd6..246d34882f787 100644 --- a/test/testsyncmove.cpp +++ b/test/testsyncmove.cpp @@ -1317,6 +1317,61 @@ private slots: QCOMPARE(counter.nMKCOL, 0); QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); } + + void testRenameComplexScenarioNoRecordLeak() + { + FakeFolder fakeFolder{FileInfo{}}; + + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + + fakeFolder.remoteModifier().mkdir("0"); + fakeFolder.remoteModifier().mkdir("0/00 without file"); + fakeFolder.remoteModifier().mkdir("0/00 without file/project"); + fakeFolder.remoteModifier().mkdir("0/00 without file/project/a"); + fakeFolder.remoteModifier().mkdir("0/00 without file/project/a/a with file"); + fakeFolder.remoteModifier().mkdir("0/00 without file/project/a/a without file"); + fakeFolder.remoteModifier().mkdir("0/00 without file/project/a/aa with file"); + fakeFolder.remoteModifier().mkdir("0/00 without file/project/00 with file"); + fakeFolder.remoteModifier().mkdir("0/00 without file/project/new without file"); + fakeFolder.remoteModifier().insert("0/00 without file/project/00 with file/test.md"); + fakeFolder.remoteModifier().insert("0/00 without file/project/a/a with file/test.md"); + fakeFolder.remoteModifier().insert("0/00 without file/project/a/aa with file/test.md"); + + QVERIFY(fakeFolder.syncOnce()); + + auto itemsCounter = 0; + auto dbResult = fakeFolder.syncJournal().getFilesBelowPath("", [&itemsCounter] (const SyncJournalFileRecord&) -> void { ++itemsCounter; }); + + QVERIFY(dbResult); + QCOMPARE(itemsCounter, 12); + + fakeFolder.remoteModifier().rename("0/00 without file/project", "project tests"); + fakeFolder.remoteModifier().rename("project tests/a", "project tests/a empty"); + fakeFolder.remoteModifier().rename("project tests/a empty/a with file", "project tests/a with file"); + fakeFolder.remoteModifier().rename("project tests/a empty/a without file", "project tests/a without file"); + fakeFolder.remoteModifier().rename("project tests/a empty/aa with file", "project tests/aa with file"); + fakeFolder.remoteModifier().rename("project tests/new without file", "project tests/new without file"); + fakeFolder.remoteModifier().rename("0/00 without file", "project tests/00 without file"); + fakeFolder.remoteModifier().rename("0", "project tests/z 0 empty"); + + connect(&fakeFolder.syncEngine(), &OCC::SyncEngine::itemCompleted, this, [&fakeFolder] () { + auto itemsCounter = 0; + auto dbResult = fakeFolder.syncJournal().getFilesBelowPath("", [&itemsCounter] (const SyncJournalFileRecord&) -> void { ++itemsCounter; }); + + QVERIFY(dbResult); + if (itemsCounter > 12) { + QVERIFY(itemsCounter <= 12); + } + }); + + QVERIFY(fakeFolder.syncOnce()); + + itemsCounter = 0; + dbResult = fakeFolder.syncJournal().getFilesBelowPath("", [&itemsCounter] (const SyncJournalFileRecord&) -> void { ++itemsCounter; }); + + QVERIFY(dbResult); + QCOMPARE(itemsCounter, 12); + } }; QTEST_GUILESS_MAIN(TestSyncMove) From 22862556353b10bf6541c4e39b580ea141157a9d Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Wed, 23 Apr 2025 18:26:54 +0200 Subject: [PATCH 5/5] fix(rename): avoid hitting runtime assert when renaming virtual files when propagating a rename to the child items, we must not make child items be done from propagator point of view they will be part of their own propagator item and that will create issues Signed-off-by: Matthieu Gallien --- src/libsync/propagatorjobs.cpp | 3 --- test/testsynccfapi.cpp | 29 +++++++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/src/libsync/propagatorjobs.cpp b/src/libsync/propagatorjobs.cpp index 59b7ec9c98dfa..6711685a31a3f 100644 --- a/src/libsync/propagatorjobs.cpp +++ b/src/libsync/propagatorjobs.cpp @@ -490,12 +490,10 @@ void PropagateLocalRename::start() SyncJournalFileRecord oldRecord; if (!propagator()->_journal->getFileRecord(oldFileNameString, &oldRecord)) { qCWarning(lcPropagateLocalRename) << "Could not get file from local DB" << oldFileNameString; - done(SyncFileItem::NormalError, tr("Could not get file %1 from local DB").arg(oldFileNameString), OCC::ErrorCategory::GenericError); return; } if (!propagator()->_journal->deleteFileRecord(oldFileNameString)) { qCWarning(lcPropagateLocalRename) << "could not delete file from local DB" << oldFileNameString; - done(SyncFileItem::NormalError, tr("Could not delete file record %1 from local DB").arg(oldFileNameString), OCC::ErrorCategory::GenericError); return; } @@ -503,7 +501,6 @@ void PropagateLocalRename::start() newItem->_file = newFileNameString; const auto result = propagator()->updateMetadata(*newItem); if (!result) { - done(SyncFileItem::FatalError, tr("Error updating metadata: %1").arg(result.error()), OCC::ErrorCategory::GenericError); return; } }); diff --git a/test/testsynccfapi.cpp b/test/testsynccfapi.cpp index 20f509cb64aa9..b34b480c0c14d 100644 --- a/test/testsynccfapi.cpp +++ b/test/testsynccfapi.cpp @@ -1486,6 +1486,35 @@ private slots: QVERIFY(itemInstruction(completeSpy, odtFile, CSYNC_INSTRUCTION_UPDATE_METADATA)); QCOMPARE(*vfs->pinState(odtFile), PinState::Unspecified); } + + void renameOnBothSides() + { + FakeFolder fakeFolder { FileInfo::A12_B12_C12_S12() }; + auto vfs = setupVfs(fakeFolder); + + // Test that renaming a file within a directory that was renamed on the other side actually do a rename. + + // 1) move the folder alphabetically before + fakeFolder.remoteModifier().rename("A/a1", "A/a1m"); + fakeFolder.localModifier().rename("A", "_A"); + fakeFolder.localModifier().rename("B/b1", "B/b1m"); + fakeFolder.remoteModifier().rename("B", "_B"); + + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentRemoteState(), fakeFolder.currentRemoteState()); + QVERIFY(fakeFolder.currentRemoteState().find("_A/a1m")); + QVERIFY(fakeFolder.currentRemoteState().find("_B/b1m")); + + // 2) move alphabetically after + fakeFolder.remoteModifier().rename("_A/a2", "_A/a2m"); + fakeFolder.localModifier().rename("_B/b2", "_B/b2m"); + fakeFolder.localModifier().rename("_A", "S/A"); + fakeFolder.remoteModifier().rename("_B", "S/B"); + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentRemoteState(), fakeFolder.currentRemoteState()); + QVERIFY(fakeFolder.currentRemoteState().find("S/A/a2m")); + QVERIFY(fakeFolder.currentRemoteState().find("S/B/b2m")); + } }; QTEST_GUILESS_MAIN(TestSyncCfApi)