diff --git a/src/libsync/propagateupload.cpp b/src/libsync/propagateupload.cpp index 5011eb1084fe5..92d9177547dd8 100644 --- a/src/libsync/propagateupload.cpp +++ b/src/libsync/propagateupload.cpp @@ -434,13 +434,29 @@ void PropagateUploadFileCommon::slotStartUpload(const QByteArray &transmissionCh return slotOnErrorStartFolderUnlock(SyncFileItem::SoftError, tr("Local file changed during syncing. It will be resumed.")); } + // for new uploads also ensure the file sizes stays the same, relying on the mtime alone is not always reliable + const auto prevFileToUploadSize = _fileToUpload._size; + const auto prevItemSize = _item->_size; _fileToUpload._size = FileSystem::getSize(fullFilePath); _item->_size = FileSystem::getSize(originalFilePath); + auto fileSizesChangedForNewItem = _item->_instruction == CSYNC_INSTRUCTION_NEW + && !(prevItemSize == 0 && prevFileToUploadSize == 0) // file conflict items created during propagation may not have a file size, ignore those + && !(prevFileToUploadSize == _fileToUpload._size && prevItemSize == _item->_size); + if (fileSizesChangedForNewItem) { + qCWarning(lcPropagateUpload).nospace() << "File sizes changed between discovery and propagation phase" + << " fileToUpload.path=" << _fileToUpload._path + << " fileToUpload.size=" << _fileToUpload._size + << " prevFileToUploadSize=" << prevFileToUploadSize + << " item.file=" << _item->_file + << " item.size=" << _item->_size + << " prevItemSize=" << prevItemSize; + } + // But skip the file if the mtime is too close to 'now'! // That usually indicates a file that is still being changed // or not yet fully copied to the destination. - if (fileIsStillChanging(*_item)) { + if (fileIsStillChanging(*_item) || fileSizesChangedForNewItem) { propagator()->_anotherSyncNeeded = true; return slotOnErrorStartFolderUnlock(SyncFileItem::SoftError, tr("Local file changed during sync.")); } diff --git a/test/testsyncengine.cpp b/test/testsyncengine.cpp index 503454e4d785e..e815e2a6046bc 100644 --- a/test/testsyncengine.cpp +++ b/test/testsyncengine.cpp @@ -17,6 +17,7 @@ #include "configfile.h" #include "propagatorjobs.h" #include "syncengine.h" +#include "syncoptions.h" #include #include @@ -24,6 +25,7 @@ #include using namespace OCC; +using namespace Qt::StringLiterals; namespace { @@ -88,6 +90,10 @@ int itemSuccessfullyCompletedGetRank(const ItemCompletedSpy &spy, const QString return -1; } +constexpr quint64 operator""_MiB(quint64 value) { + return value * 1024LL * 1024LL; +} + } class TestSyncEngine : public QObject @@ -2461,6 +2467,103 @@ private slots: const auto directoryItem = fakeFolder.remoteModifier().find("directory"); QCOMPARE(directoryItem, nullptr); } + + void testUploadWhileFileIsChanging() + { + // While files are still being copied/saved it is possible that the sync + // engine discovers a new file while it's still being written to. + // With a small enough file size during discovery the propagation will + // be handled by a PropagateUploadFileV1 job. + // This is fine as long as it stays at a small file size, otherwise its + // implementation of the old chunking algorithm will be used. That was + // still available until Nextcloud 30, since then any chunked uploads + // performed with the now-removed API will be finished right after the + // first chunk was uploaded: with the old V1 chunking system the server + // did not respond with an ETag until the final chunk was uploaded. + // Since the removal the ETag is always present in the response, which + // lead the client to believe that all chunks finished uploading, and + // considers the upload to be done. + + FakeFolder fakeFolder{FileInfo{}}; + fakeFolder.syncEngine().account()->setCapabilities({ { "dav", QVariantMap{ {"chunking", "1.0"} } } }); + QVERIFY(fakeFolder.syncOnce()); + + const auto changemePath = fakeFolder.localPath().append("/changeme"); + QFile changemeFile{changemePath}; + QVERIFY(changemeFile.open(QIODevice::WriteOnly | QIODevice::Unbuffered)); + QVERIFY(fakeFolder.localModifier().find("changeme").exists()); + QCOMPARE(changemeFile.write("AA"), 2); + + auto syncOptions = fakeFolder.syncEngine().syncOptions(); + syncOptions._initialChunkSize = 5_MiB; + fakeFolder.syncEngine().setSyncOptions(syncOptions); + + // the file should be discovered with an initial size of 2 bytes + // --> propagation is done by PropagateUploadFileV1 + QSignalSpy itemDiscoveredSpy(&fakeFolder.syncEngine(), &OCC::SyncEngine::itemDiscovered); + fakeFolder.scheduleSync(); + fakeFolder.execUntilBeforePropagation(); + QCOMPARE(itemDiscoveredSpy.size(), 1); + { + const auto discoveredItem = itemDiscoveredSpy.takeFirst().takeFirst().value(); + QCOMPARE(discoveredItem->_size, 2); + } + + // just before propagation starts, the file size changed! + // the PropagateUploadFileV1 job would then try to upload the file in + // chunks as it's now large enough to be chunked (25MiB). + const QByteArray oneMegabyteOfScreaming = "A"_ba.repeated(1_MiB); + for (auto i = 0; i < 25; i++) { + changemeFile.write(oneMegabyteOfScreaming); + } + + // finish the sync: the file should be reuploaded on the next sync run, + // and nothing should have been uploaded + ItemCompletedSpy itemCompleteSpy(fakeFolder); + QVERIFY(!fakeFolder.execUntilFinished()); + QVERIFY(!itemDidCompleteSuccessfully(itemCompleteSpy, "changeme")); + QCOMPARE(itemCompleteSpy.findItem("changeme")->_errorString, "Local file changed during sync."); + QVERIFY(fakeFolder.syncEngine().isAnotherSyncNeeded()); + QCOMPARE(fakeFolder.currentRemoteState().children.size(), 0); + + // retry the sync again -- the file is still changing + // this time as the file size is large enough the correct chunking + // system from PropagateUploadFileNG would be used + itemDiscoveredSpy.clear(); + itemCompleteSpy.clear(); + fakeFolder.scheduleSync(); + fakeFolder.execUntilBeforePropagation(); + QCOMPARE(itemDiscoveredSpy.size(), 1); + { + const auto discoveredItem = itemDiscoveredSpy.takeFirst().takeFirst().value(); + QCOMPARE(discoveredItem->_size, 25_MiB + 2); + } + for (auto i = 0; i < 25; i++) { + changemeFile.write(oneMegabyteOfScreaming); + } + // and we fail again! + QVERIFY(!fakeFolder.execUntilFinished()); + QVERIFY(!itemDidCompleteSuccessfully(itemCompleteSpy, "changeme")); + QCOMPARE(itemCompleteSpy.findItem("changeme")->_errorString, "Local file changed during sync."); + QVERIFY(fakeFolder.syncEngine().isAnotherSyncNeeded()); + QCOMPARE(fakeFolder.currentRemoteState().children.size(), 0); + + // the file is now complete, the next sync should work and result in the + // same remote and local states. again, still using the chunking from + // the PropagateUploadFileNG job + changemeFile.close(); + itemDiscoveredSpy.clear(); + itemCompleteSpy.clear(); + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(itemDiscoveredSpy.size(), 1); + { + const auto discoveredItem = itemDiscoveredSpy.takeFirst().takeFirst().value(); + QCOMPARE(discoveredItem->_size, 50_MiB + 2); + } + QVERIFY(itemDidCompleteSuccessfully(itemCompleteSpy, "changeme")); + QVERIFY(!fakeFolder.syncEngine().isAnotherSyncNeeded()); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + } }; QTEST_GUILESS_MAIN(TestSyncEngine)