fix(propagateupload): ensure file size stays the same for new uploads

src/libsync/propagateupload.cpp

@@ -1,10 +1,10 @@
 /*
  * SPDX-FileCopyrightText: 2017 Nextcloud GmbH and Nextcloud contributors
  * SPDX-FileCopyrightText: 2014 ownCloud GmbH
  * SPDX-License-Identifier: GPL-2.0-or-later
  */
 
 #include "config.h"
 #include "propagateupload.h"
 #include "propagateuploadencrypted.h"
 #include "owncloudpropagator_p.h"
@@ -71,7 +71,7 @@
         qCWarning(lcPutJob) << " Network error: " << reply()->errorString();
     }
 
     connect(reply(), &QNetworkReply::uploadProgress, this, [requestID] (qint64 bytesSent, qint64 bytesTotal) {
         qCDebug(lcPutJob()) << requestID << "upload progress" << bytesSent << bytesTotal;
     });
 
@@ -114,7 +114,7 @@
         _item->_requestId = requestId();
         _item->_status = classifyError(err, _item->_httpErrorCode);
         _item->_errorString = errorString();
         const auto exceptionParsed = getExceptionFromReply(reply());
         _item->_errorExceptionName = exceptionParsed.first;
         _item->_errorExceptionMessage = exceptionParsed.second;
 
@@ -159,7 +159,7 @@
         _item->_fileId = json["fileId"].toString().toUtf8();
 
         if (SyncJournalFileRecord oldRecord; _journal->getFileRecord(_item->destination(), &oldRecord) && oldRecord.isValid()) {
             if (oldRecord._etag != _item->_etag) {
                 _item->updateLockStateFromDbRecord(oldRecord);
             }
         }
@@ -182,9 +182,9 @@
 
 PropagateUploadFileCommon::PropagateUploadFileCommon(OwncloudPropagator *propagator, const SyncFileItemPtr &item)
     : PropagateItemJob(propagator, item)
     , _finished(false)
     , _deleteExisting(false)
     , _aborting(false)
 {
     const auto path = _item->_file;
     const auto slashPosition = path.lastIndexOf('/');
@@ -434,13 +434,29 @@
         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."));
     }
@@ -741,7 +757,7 @@
 
     job->setTimeout(qBound(
         // Calculate 3 minutes for each gigabyte of data
         qMin(thirtyMinutes - 1, qRound64(threeMinutes * fileSize / 1e9)),
         job->timeoutMsec(),
         // Maximum of 30 minutes
         thirtyMinutes));
@@ -749,7 +765,7 @@
 
 void PropagateUploadFileCommon::slotJobDestroyed(QObject *job)
 {
     _jobs.erase(std::remove(_jobs.begin(), _jobs.end(), job), _jobs.end());
 }
 
 // This function is used whenever there is an error occurring and jobs might be in progress

test/testsyncengine.cpp

@@ -1,3 +1,3 @@
 /*
  * SPDX-FileCopyrightText: 2021 Nextcloud GmbH and Nextcloud contributors
  * SPDX-FileCopyrightText: 2016 ownCloud, Inc.
@@ -8,7 +8,7 @@
  * any purpose.
  */
 
 #include <QtTest>
 #include <QTextCodec>
 
 #include "syncenginetestutils.h"
@@ -17,13 +17,15 @@
 #include "configfile.h"
 #include "propagatorjobs.h"
 #include "syncengine.h"
+#include "syncoptions.h"
 
 #include <QFile>
 #include <QtTest>
 
 #include <filesystem>
 
 using namespace OCC;
+using namespace Qt::StringLiterals;
 
 namespace {
 
@@ -88,6 +90,10 @@
     return -1;
 }
 
+constexpr quint64 operator""_MiB(quint64 value) {
+    return value * 1024LL * 1024LL;
+}
+
 }
 
 class TestSyncEngine : public QObject
@@ -2461,6 +2467,103 @@
         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<OCC::SyncFileItemPtr>();
+            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<OCC::SyncFileItemPtr>();
+            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<OCC::SyncFileItemPtr>();
+            QCOMPARE(discoveredItem->_size, 50_MiB + 2);
+        }
+        QVERIFY(itemDidCompleteSuccessfully(itemCompleteSpy, "changeme"));
+        QVERIFY(!fakeFolder.syncEngine().isAnotherSyncNeeded());
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+    }
 };
 
 QTEST_GUILESS_MAIN(TestSyncEngine)