From dcb2b5435a80ea1d827d7b5cabe62d68e98ce988 Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Wed, 1 Apr 2026 11:08:31 +0200 Subject: [PATCH 1/2] fix(discovery): check quota before upload (during discovery) Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- src/libsync/discovery.cpp | 67 ++++++++-- src/libsync/discovery.h | 5 + src/libsync/propagateupload.cpp | 39 +++++- src/libsync/syncengine.cpp | 27 +++- test/testlocaldiscovery.cpp | 79 ++++------- test/testsyncengine.cpp | 224 ++++++++++++++++++++++++++++++++ 6 files changed, 374 insertions(+), 67 deletions(-) diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp index 103518e8a7f68..dbaebd1ddab49 100644 --- a/src/libsync/discovery.cpp +++ b/src/libsync/discovery.cpp @@ -31,7 +31,6 @@ namespace constexpr const char *editorNamesForDelayedUpload[] = {"PowerPDF"}; constexpr const char *fileExtensionsToCheckIfOpenForSigning[] = {".pdf"}; constexpr auto delayIntervalForSyncRetryForOpenedForSigningFilesSeconds = 60; -constexpr auto delayIntervalForSyncRetryForFilesExceedQuotaSeconds = 60; } namespace OCC { @@ -1114,11 +1113,26 @@ int64_t ProcessDirectoryJob::folderBytesAvailable(const SyncFileItemPtr &item, c return unlimitedFreeSpace; } + // Helper: subtract _quotaBytesReserved from a known quota value (>= 0), + // clamping to 0. Negative sentinels (-1 = new unscanned folder, + // -2 = unknown, -3 = unlimited) are passed through unchanged so the + // caller's "folderQuota > -1" guard correctly skips them. + auto adjustForReserved = [this](const int64_t raw) { + return raw >= 0 ? std::max(0, raw - _quotaBytesReserved) : raw; + }; + if (serverEntry == FolderQuota::ServerEntry::Valid) { qCDebug(lcDisco) << "Returning cached _folderQuota.bytesAvailable for item quota."; - return _folderQuota.bytesAvailable; + return adjustForReserved(_folderQuota.bytesAvailable); } + // serverEntry == Invalid: the file is new locally and has no server-side + // counterpart yet. Fall through to the DB / _dirItem fallback so the + // quota value stored during the previous PROPFIND cycle is used. If quota + // remains unknown there too, folderBytesAvailable() returns unlimitedFreeSpace + // and the upload is allowed; the propagator's reactive HTTP-507 path catches + // the failure and blacklists the item when quota is unavailable upfront. + if (!_dirItem) { qCDebug(lcDisco) << "Returning unlimited free space (-3) for item quota with no _dirItem."; return unlimitedFreeSpace; @@ -1126,15 +1140,28 @@ int64_t ProcessDirectoryJob::folderBytesAvailable(const SyncFileItemPtr &item, c qCDebug(lcDisco) << "_dirItem->_folderQuota.bytesAvailable:" << _dirItem->_folderQuota.bytesAvailable; + // Priority 1: fresh value from the current PROPFIND cycle stored in _dirItem. + // _dirItem is the SyncFileItem for the parent folder (e.g. "A"), whose + // _folderQuota was populated by processFileAnalyzeRemoteInfo() when "A" + // was itself processed — this happens before any child of "A" is finalized. + if (_dirItem->_folderQuota.bytesAvailable != -1) { + qCDebug(lcDisco) << "Returning _dirItem->_folderQuota.bytesAvailable for item quota (fresh PROPFIND value)."; + return adjustForReserved(_dirItem->_folderQuota.bytesAvailable); + } + + // Priority 2: value persisted from the previous sync cycle in the journal DB. SyncJournalFileRecord dirItemDbRecord; if (_discoveryData->_statedb->getFileRecord(_dirItem->_file, &dirItemDbRecord) && dirItemDbRecord.isValid()) { const auto dirDbBytesAvailable = dirItemDbRecord._folderQuota.bytesAvailable; - qCDebug(lcDisco) << "Returning for item quota db value dirItemDbRecord._folderQuota.bytesAvailable" << dirDbBytesAvailable; - return dirDbBytesAvailable; + if (dirDbBytesAvailable != -1) { + qCDebug(lcDisco) << "Returning for item quota db value dirItemDbRecord._folderQuota.bytesAvailable" << dirDbBytesAvailable; + return adjustForReserved(dirDbBytesAvailable); + } } - qCDebug(lcDisco) << "Returning _dirItem->_folderQuota.bytesAvailable for item quota."; - return _dirItem->_folderQuota.bytesAvailable; + // Priority 3: quota unknown, allow the upload; reactive HTTP-507 will catch it. + qCDebug(lcDisco) << "Returning unlimited free space (-3) for item quota: quota unknown from both dirItem and DB."; + return unlimitedFreeSpace; } void ProcessDirectoryJob::processFileAnalyzeLocalInfo( @@ -1220,9 +1247,22 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo( _currentFolder._server); } - item->_status = SyncFileItem::Status::NormalError; - _discoveryData->_anotherSyncNeeded = true; - _discoveryData->_filesNeedingScheduledSync.insert(path._original, delayIntervalForSyncRetryForFilesExceedQuotaSeconds); + // Use DetailError so the error appears in the issues tab without a + // prominent pop-up, matching the reactive HTTP 507 path. + // Do NOT set _anotherSyncNeeded or insert into _filesNeedingScheduledSync: + // those were the source of the 60-second retry loop. The discovery check + // re-fires on every normal sync cycle, so the upload remains blocked + // without wasting any network bandwidth. + item->_status = SyncFileItem::Status::DetailError; + } else if (item->_direction == SyncFileItem::Up + && !item->isDirectory() + && (item->_instruction == CSYNC_INSTRUCTION_NEW + || item->_instruction == CSYNC_INSTRUCTION_SYNC)) { + // Upload approved: reserve these bytes so subsequent files processed + // in the same discovery pass see the reduced available quota. Without + // this, two files that individually fit but together exceed the quota + // would both be approved. + _quotaBytesReserved += item->_size; } if (item->_type != CSyncEnums::ItemTypeVirtualFile) { @@ -1931,6 +1971,15 @@ void ProcessDirectoryJob::processFileFinalize( auto job = new ProcessDirectoryJob(path, item, recurseQueryLocal, recurseQueryServer, _lastSyncTimestamp, this); job->setInsideEncryptedTree(isInsideEncryptedTree() || item->isEncrypted()); + // Propagate the parent folder's quota into the child job so that + // new-local-only files (serverEntry invalid, no DB record yet) can + // read a valid quota via _folderQuota in folderBytesAvailable(). + if (item->_folderQuota.bytesAvailable != -1) { + OCC::FolderQuota folderQuota; + folderQuota.bytesUsed = item->_folderQuota.bytesUsed; + folderQuota.bytesAvailable = item->_folderQuota.bytesAvailable; + job->setFolderQuota(folderQuota); + } if (removed) { job->setParent(_discoveryData); _discoveryData->_deletedItem[path._original] = item; diff --git a/src/libsync/discovery.h b/src/libsync/discovery.h index f59cf75250863..1b64c1c2ef202 100644 --- a/src/libsync/discovery.h +++ b/src/libsync/discovery.h @@ -300,6 +300,11 @@ class ProcessDirectoryJob : public QObject bool _isInsideEncryptedTree = false; // this directory is encrypted or is within the tree of directories with root directory encrypted FolderQuota _folderQuota; + // Running total of bytes already reserved for approved uploads in this + // directory during the current discovery pass. Subtracted from the quota + // returned by folderBytesAvailable() so that two files which individually + // fit but together exceed quota cannot both be approved in one pass. + qint64 _quotaBytesReserved = 0; int64_t folderBytesAvailable(const SyncFileItemPtr &item, const FolderQuota::ServerEntry serverEntry) const; diff --git a/src/libsync/propagateupload.cpp b/src/libsync/propagateupload.cpp index 5011eb1084fe5..da85aa0bc09cc 100644 --- a/src/libsync/propagateupload.cpp +++ b/src/libsync/propagateupload.cpp @@ -281,14 +281,40 @@ void PropagateUploadFileCommon::startUploadFile() { return; } - // Check if we believe that the upload will fail due to remote quota limits - const qint64 quotaGuess = propagator()->_folderQuota.value( - QFileInfo(_fileToUpload._file).path(), std::numeric_limits::max()); + // Check if we believe that the upload will fail due to remote quota limits. + // _folderQuota is seeded at the start of each sync cycle from the quota data + // returned by PROPFIND during discovery (see SyncEngine::finishSync). + // It is also tightened reactively whenever the server returns HTTP 507. + // + // A PROPFIND quota entry applies to a directory and all its descendants. + // Walk up the path hierarchy to find the nearest quota entry, so that a + // quota set on "A" also guards uploads into "A/B/C/". + qint64 quotaGuess = std::numeric_limits::max(); + QString lookupPath = QFileInfo(_fileToUpload._file).path(); + while (!lookupPath.isEmpty()) { + if (propagator()->_folderQuota.contains(lookupPath)) { + quotaGuess = propagator()->_folderQuota.value(lookupPath); + break; + } + if (lookupPath == QLatin1String(".")) + break; // reached sync root with no quota entry + const int slash = lookupPath.lastIndexOf(QLatin1Char('/')); + lookupPath = slash >= 0 ? lookupPath.left(slash) : QStringLiteral("."); + } if (_fileToUpload._size > quotaGuess) { - // Necessary for blacklisting logic + // quotaGuess is never std::numeric_limits::max() here: reaching + // this branch requires _size > quotaGuess, which is impossible when + // quotaGuess == max(). So Utility::octetsToString(quotaGuess) in the + // message below always formats a real quota value, never "max". + // + // Set httpErrorCode so blacklistUpdate creates an InsufficientRemoteStorage + // entry, which suppresses automatic retries until quota becomes sufficient. _item->_httpErrorCode = 507; emit propagator()->insufficientRemoteStorage(); - done(SyncFileItem::DetailError, tr("Upload of %1 exceeds the quota for the folder").arg(Utility::octetsToString(_fileToUpload._size))); + done(SyncFileItem::DetailError, + tr("Upload of %1 exceeds %2 of remaining storage quota for this folder") + .arg(Utility::octetsToString(_fileToUpload._size), + Utility::octetsToString(quotaGuess))); return; } @@ -720,7 +746,8 @@ void PropagateUploadFileCommon::commonErrorHandling(AbstractNetworkJob *job) // Set up the error status = SyncFileItem::DetailError; - errorString = tr("Upload of %1 exceeds the quota for the folder").arg(Utility::octetsToString(_fileToUpload._size)); + errorString = tr("Upload of %1 exceeds the remaining storage quota for this folder") + .arg(Utility::octetsToString(_fileToUpload._size)); emit propagator()->insufficientRemoteStorage(); } else if (_item->_httpErrorCode == 400) { const auto exception = job->errorStringParsingBodyException(replyContent); diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp index 1afd7cff4031b..380d2781f42e0 100644 --- a/src/libsync/syncengine.cpp +++ b/src/libsync/syncengine.cpp @@ -1093,6 +1093,30 @@ void SyncEngine::finishSync() _propagator = QSharedPointer( new OwncloudPropagator(_account, _localPath, _remotePath, _journal, _bulkUploadBlackList)); _propagator->setSyncOptions(_syncOptions); + + // Seed the propagator's per-folder quota cache from the quota data gathered + // during the discovery phase (PROPFIND responses). The propagator is recreated + // fresh every sync cycle, so without this seeding the pre-upload quota check in + // PropagateUploadFileCommon::startUploadFile() would always pass on the first + // attempt of a new cycle, causing a full upload that the server then rejects with + // HTTP 507 and wasting bandwidth. By initialising the cache here, the pre-upload + // check can block oversized files from the very first attempt of each cycle. + // + // NOTE: This loop must remain before _propagator->start(std::move(_syncItems)) + // below, because start() transfers ownership of _syncItems and leaves it empty. + for (const auto &syncItem : std::as_const(_syncItems)) { + if (syncItem->isDirectory() && syncItem->_folderQuota.bytesAvailable >= 0) { + // OwncloudPropagator keys _folderQuota by QFileInfo::path() of the file + // being uploaded, which yields "." for items at the root of the sync + // folder. Normalise the empty-string root path accordingly. + const auto key = syncItem->_file.isEmpty() ? QStringLiteral(".") : syncItem->_file; + // Only insert when there is no tighter bound already present (e.g. set + // from a prior HTTP 507 reply within the same cycle). + if (!_propagator->_folderQuota.contains(key)) { + _propagator->_folderQuota.insert(key, syncItem->_folderQuota.bytesAvailable); + } + } + } connect(_propagator.data(), &OwncloudPropagator::itemCompleted, this, &SyncEngine::slotItemCompleted); connect(_propagator.data(), &OwncloudPropagator::progress, @@ -1485,7 +1509,8 @@ void SyncEngine::slotInsufficientLocalStorage() void SyncEngine::slotInsufficientRemoteStorage() { - auto msg = tr("There is insufficient space available on the server for some uploads."); + auto msg = tr("Upload paused: one or more files exceed your remaining Nextcloud storage quota. " + "Free up server space or contact your administrator to increase your quota."); if (_uniqueErrors.contains(msg)) { return; } diff --git a/test/testlocaldiscovery.cpp b/test/testlocaldiscovery.cpp index e5e2a943dc982..84382ea15336a 100644 --- a/test/testlocaldiscovery.cpp +++ b/test/testlocaldiscovery.cpp @@ -870,106 +870,83 @@ private slots: void testDiscoveryUsesCorrectQuotaSource() { - //setup sync folder + // Verifies that the discovery-phase quota check uses the FRESH + // PROPFIND value (from the current sync cycle) rather than the + // stale value persisted in the journal DB from a previous cycle. + FakeFolder fakeFolder{FileInfo{}}; - // create folder const QString folderA("A"); fakeFolder.localModifier().mkdir(folderA); fakeFolder.remoteModifier().mkdir(folderA); fakeFolder.remoteModifier().setFolderQuota(folderA, {0, 500}); - // sync folderA + // Initial sync — folder A is created, DB stores quota = 500 ItemCompletedSpy syncSpy(fakeFolder); QVERIFY(fakeFolder.syncOnce()); QCOMPARE(syncSpy.findItem(folderA)->_status, SyncFileItem::Status::NoStatus); - // check db quota for folderA - bytesAvailable is 500 SyncJournalFileRecord recordFolderA; QVERIFY(fakeFolder.syncJournal().getFileRecord(folderA, &recordFolderA)); QCOMPARE(recordFolderA._folderQuota.bytesAvailable, 500); - // add fileNameA to folderA - size < quota in db + // ── Case 1: upload succeeds when size < fresh PROPFIND quota ── const QString fileNameA("A/A.data"); fakeFolder.localModifier().insert(fileNameA, 200); - // set different quota for folderA - remote change does not change etag yet - fakeFolder.remoteModifier().setFolderQuota(folderA, {0, 0}); - - // sync filenameA - size == quota => success syncSpy.clear(); QVERIFY(fakeFolder.syncOnce()); QCOMPARE(syncSpy.findItem(fileNameA)->_status, SyncFileItem::Status::Success); - // add smallFile to folderA - size < quota in db - const QString smallFile("A/smallFile.data"); - fakeFolder.localModifier().insert(smallFile, 100); + // ── Case 2: fresh PROPFIND value (0) wins over stale DB (500) ── + // Reduce quota to 0 without invalidating the etag. The DB still + // holds 500, but the root PROPFIND now returns 0 for folder A. + // A new file must be blocked proactively. + fakeFolder.remoteModifier().setFolderQuota(folderA, {0, 0}); + + const QString fileBlocked("A/blocked.data"); + fakeFolder.localModifier().insert(fileBlocked, 100); - // sync smallFile - size < quota in db => success => update quota in db syncSpy.clear(); - QVERIFY(fakeFolder.syncOnce()); - QCOMPARE(syncSpy.findItem(smallFile)->_status, SyncFileItem::Status::Success); + QVERIFY(!fakeFolder.syncOnce()); + QCOMPARE(syncSpy.findItem(fileBlocked)->_status, SyncFileItem::Status::DetailError); - // create remoteFileA - size > bytes available + // ── Case 3: downloads are not affected by upload quota ── const QString remoteFileA("A/remoteA.data"); fakeFolder.remoteModifier().insert(remoteFileA, 200); - // sync remoteFile - it is a download syncSpy.clear(); - QVERIFY(fakeFolder.syncOnce()); + // Sync still fails overall (blocked.data remains blocked), but the + // download of remoteFileA must succeed independently. + QVERIFY(!fakeFolder.syncOnce()); QCOMPARE(syncSpy.findItem(remoteFileA)->_status, SyncFileItem::Status::Success); + QCOMPARE(syncSpy.findItem(fileBlocked)->_status, SyncFileItem::Status::DetailError); - // check db quota for folderA - bytesAvailable have changed to 0 due to new PROPFIND - QVERIFY(fakeFolder.syncJournal().getFileRecord(folderA, &recordFolderA)); - QCOMPARE(recordFolderA._folderQuota.bytesAvailable, 0); - - // create local fileNameB - size < quota in db - const QString fileNameB("A/B.data"); - fakeFolder.localModifier().insert(fileNameB, 0); - - // set different quota for folderA - remote change does not change etag yet + // ── Case 4: after quota increase, previously blocked file uploads ── fakeFolder.remoteModifier().setFolderQuota(folderA, {500, 600}); - // sync fileNameB - size < quota in db => success syncSpy.clear(); QVERIFY(fakeFolder.syncOnce()); - QCOMPARE(syncSpy.findItem(fileNameB)->_status, SyncFileItem::Status::Success); - - // create remoteFileB - it is a download - const QString remoteFileB("A/remoteB.data"); - fakeFolder.remoteModifier().insert(remoteFileA, 100); + QCOMPARE(syncSpy.findItem(fileBlocked)->_status, SyncFileItem::Status::Success); - // create local fileNameC - size < quota in db - const QString fileNameC("A/C.data"); - fakeFolder.localModifier().insert(fileNameC, 0); - - // sync filenameC - size < quota in db => success - syncSpy.clear(); - QVERIFY(fakeFolder.syncOnce()); - QCOMPARE(syncSpy.findItem(fileNameC)->_status, SyncFileItem::Status::Success); - - // check db quota for folderA - bytesAvailable have changed to 600 due to new PROPFIND + // DB now reflects the fresh PROPFIND value (600) QVERIFY(fakeFolder.syncJournal().getFileRecord(folderA, &recordFolderA)); QCOMPARE(recordFolderA._folderQuota.bytesAvailable, 600); - QCOMPARE(syncSpy.findItem(remoteFileB)->_status, SyncFileItem::Status::NoStatus); - - // create local fileNameD - size > quota in db + // ── Case 5: file exceeding fresh quota is blocked ── const QString fileNameD("A/D.data"); fakeFolder.localModifier().insert(fileNameD, 700); - // sync fileNameD - size > quota in db => error syncSpy.clear(); QVERIFY(!fakeFolder.syncOnce()); - QCOMPARE(syncSpy.findItem(fileNameD)->_status, SyncFileItem::Status::NormalError); + QCOMPARE(syncSpy.findItem(fileNameD)->_status, SyncFileItem::Status::DetailError); - // create local fileNameE - size < quota in db + // ── Case 6: file within quota succeeds even while another is blocked ── const QString fileNameE("A/E.data"); fakeFolder.localModifier().insert(fileNameE, 400); - // sync fileNameE - size < quota in db => success syncSpy.clear(); - QVERIFY(!fakeFolder.syncOnce()); + QVERIFY(!fakeFolder.syncOnce()); // fileNameD still blocked QCOMPARE(syncSpy.findItem(fileNameE)->_status, SyncFileItem::Status::Success); } diff --git a/test/testsyncengine.cpp b/test/testsyncengine.cpp index 503454e4d785e..0d30deda73cf6 100644 --- a/test/testsyncengine.cpp +++ b/test/testsyncengine.cpp @@ -651,6 +651,230 @@ private slots: QCOMPARE(n507, 3); } + /** + * When the server reports folder quota via PROPFIND and a local file exceeds it, + * the client must: + * 1. Not start any upload (zero PUTs) — quota is detected during discovery, + * before any data is transmitted. + * 2. Not start an automatic retry loop — _anotherSyncNeeded and + * _filesNeedingScheduledSync must NOT be set for quota errors. + * 3. On the next manually-triggered sync (same quota), still not upload. + * 4. Upload successfully once quota is increased above the file size. + */ + void testQuotaExceededBlocksUploadAndSuppressesRetry() + { + FakeFolder fakeFolder{FileInfo::A12_B12_C12_S12()}; + + // Give folder "A" a tight quota: only 500 bytes of free space. + fakeFolder.remoteModifier().setFolderQuota("A", {0, 500}); + + int nPUT = 0; + fakeFolder.setServerOverride([&](QNetworkAccessManager::Operation op, + const QNetworkRequest &, + QIODevice *) -> QNetworkReply * { + if (op == QNetworkAccessManager::PutOperation) + ++nPUT; + return nullptr; + }); + + // Insert a file whose size exceeds the available quota. + fakeFolder.localModifier().insert("A/toobig", 700); + + // The sync must fail: quota is exceeded. + QVERIFY(!fakeFolder.syncOnce()); + + // No HTTP PUT must have been issued — the discovery-phase quota check must + // have blocked the upload before any data was transmitted. + QCOMPARE(nPUT, 0); + + // Trigger another sync immediately (simulates the retry loop that existed + // before the fix). With the old code _anotherSyncNeeded / _filesNeedingScheduledSync + // would cause an upload attempt; with the fix the discovery check re-fires and + // again blocks the upload without touching the network. + nPUT = 0; + QVERIFY(!fakeFolder.syncOnce()); + QCOMPARE(nPUT, 0); + + // Now increase quota above the file size; the upload must succeed. + fakeFolder.remoteModifier().setFolderQuota("A", {0, 1000}); + nPUT = 0; + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(nPUT, 1); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + } + + /** + * When quota is unknown (bytesAvailable == -1, server did not report it), + * the upload must not be blocked — the client has no basis to refuse it. + * If the server later rejects with HTTP 507, the reactive path handles it. + * + * Two cases are verified indirectly (folderBytesAvailable() is private): + * + * Case 1 — _quotaBytesReserved == 0 at check time: + * A single new file is inserted. When folderBytesAvailable() is called, + * _quotaBytesReserved is still 0. adjustForReserved(-1) must return -1, + * not 0. Proof: nPUT == 1 (upload was not blocked). + * + * Case 2 — _quotaBytesReserved == 700 at check time: + * A second new file is inserted in the same pass. "A/first" (700 B) is + * processed first (alphabetical order), gets approved, and increments + * _quotaBytesReserved to 700. When "A/second" (600 B) is evaluated, + * adjustForReserved(-1) must still return -1 — not 0 or −701 — so the + * upload is not blocked. Proof: nPUT == 2 (both files uploaded). + * + * If the clamping were wrong (raw < 0 → max(0, raw - reserved) = 0), + * "A/second" would be blocked by the "size > 0 && 0 > -1" guard, giving + * nPUT == 1 and a sync failure — which is the regression this catches. + */ + void testQuotaUnknownDoesNotBlockUpload() + { + FakeFolder fakeFolder{FileInfo::A12_B12_C12_S12()}; + + // Quota is unknown for folder "A" (bytesAvailable = -1). + fakeFolder.remoteModifier().setFolderQuota("A", {0, -1}); + + int nPUT = 0; + fakeFolder.setServerOverride([&](QNetworkAccessManager::Operation op, + const QNetworkRequest &, + QIODevice *) -> QNetworkReply * { + if (op == QNetworkAccessManager::PutOperation) + ++nPUT; + return nullptr; + }); + + // Case 1: _quotaBytesReserved == 0 when "A/newfile" is evaluated. + // adjustForReserved(-1) must return -1 → upload allowed. + fakeFolder.localModifier().insert("A/newfile", 700); + + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(nPUT, 1); // upload was not blocked + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + + // Case 2: _quotaBytesReserved == 700 when the second file is evaluated. + // "A/first" (700 B) sorts before "A/second" (600 B), so it is processed + // first, approved, and _quotaBytesReserved becomes 700. When "A/second" + // is evaluated, adjustForReserved(-1) must still return -1 — not 0 — so + // the upload is not blocked despite the non-zero reservation. + nPUT = 0; + fakeFolder.localModifier().insert("A/first", 700); + fakeFolder.localModifier().insert("A/second", 600); + + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(nPUT, 2); // both files uploaded; neither was blocked + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + } + + /** + * Chunked uploads (PropagateUploadFileNG) go through the same + * PropagateUploadFileCommon::startUploadFile() pre-upload quota check. + * Verify that a file larger than the reported quota is blocked before + * any chunk is transmitted, regardless of chunking being active. + */ + void testQuotaExceededBlocksChunkedUpload() + { + FakeFolder fakeFolder{FileInfo::A12_B12_C12_S12()}; + + fakeFolder.syncEngine().account()->setCapabilities( + {{"dav", QVariantMap{{"chunking", "1.0"}}}}); + // Confirm capability wiring before proceeding: if chunkingNg() is false, + // PropagateUploadFileV1 is used instead of NG, MKCOL is never issued, and + // the post-quota-increase assertions would fail for the wrong reason. + QVERIFY(fakeFolder.syncEngine().account()->capabilities().chunkingNg()); + + // Force chunked mode: set initialChunkSize to 100 bytes so even our + // small test file is chunked. + SyncOptions syncOptions; + syncOptions._initialChunkSize = 100; + syncOptions.setMinChunkSize(100); + syncOptions.setMaxChunkSize(100); + fakeFolder.syncEngine().setSyncOptions(syncOptions); + + // Quota: 500 bytes available. + fakeFolder.remoteModifier().setFolderQuota("A", {0, 500}); + + int nPUT = 0; + int nMKCOL = 0; + fakeFolder.setServerOverride([&](QNetworkAccessManager::Operation op, + const QNetworkRequest &request, + QIODevice *) -> QNetworkReply * { + if (op == QNetworkAccessManager::PutOperation) + ++nPUT; + if (op == QNetworkAccessManager::CustomOperation + && request.attribute(QNetworkRequest::CustomVerbAttribute).toByteArray() == QByteArrayLiteral("MKCOL")) + ++nMKCOL; + return nullptr; + }); + + // File exceeds quota. + fakeFolder.localModifier().insert("A/bigchunked", 700); + + QVERIFY(!fakeFolder.syncOnce()); + + // Neither a PUT (chunk) nor a MKCOL (chunk folder creation) must have + // been issued — the quota check fires before the chunked upload starts. + QCOMPARE(nPUT, 0); + QCOMPARE(nMKCOL, 0); + + // After quota increase the chunked upload must succeed and must actually + // transmit data — confirm MKCOL (chunk folder) and PUT (chunk data) happened. + fakeFolder.remoteModifier().setFolderQuota("A", {0, 1000}); + nPUT = 0; + nMKCOL = 0; + QVERIFY(fakeFolder.syncOnce()); + QVERIFY(nMKCOL > 0); // chunk folder was created + QVERIFY(nPUT > 0); // at least one chunk was uploaded + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + } + + /** + * Two files that individually fit the available quota but together exceed + * it must not both be approved in the same discovery pass. + * + * The first file processed reserves its bytes; the second then sees the + * reduced available quota and is blocked before any data is transmitted. + * This ensures the propagator-level 507 guard is never the first line of + * defence against combined over-quota uploads. + * + * File names are chosen so that alphabetical order (the iteration order of + * the std::map used internally) is the same as the intended + * processing order: "afile" sorts before "bfile", so "afile" is always first. + * This makes the ordering a stated property of the names, not a hidden + * assumption about the container implementation. + */ + void testQuotaTwoFilesExceedingCombinedQuota() + { + FakeFolder fakeFolder{FileInfo::A12_B12_C12_S12()}; + + // 1000 bytes free. "afile" (700 B) and "bfile" (600 B) each individually + // fit (700 < 1000, 600 < 1000) but their combined size (1300 B) exceeds + // the quota. + fakeFolder.remoteModifier().setFolderQuota("A", {0, 1000}); + + int nPUT = 0; + fakeFolder.setServerOverride([&](QNetworkAccessManager::Operation op, + const QNetworkRequest &, + QIODevice *) -> QNetworkReply * { + if (op == QNetworkAccessManager::PutOperation) + ++nPUT; + return nullptr; + }); + + // "afile" sorts before "bfile": "afile" is always processed first, + // its 700 B are reserved, and only 300 B remain when "bfile" is + // evaluated — which is then blocked (600 B > 300 B). + fakeFolder.localModifier().insert("A/afile", 700); + fakeFolder.localModifier().insert("A/bfile", 600); + + // Sync must fail: "bfile" is blocked by the combined quota check. + QVERIFY(!fakeFolder.syncOnce()); + + // Exactly one PUT: "afile" uploaded, "bfile" blocked. Without + // _quotaBytesReserved tracking both would be approved individually + // and together overflow the quota. + QCOMPARE(nPUT, 1); + QVERIFY(fakeFolder.currentLocalState() != fakeFolder.currentRemoteState()); + } + // Checks whether downloads with bad checksums are accepted void testChecksumValidation() { From c72a6c3350fb642cc44ccdb370a310a8d4cdc286 Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Fri, 3 Apr 2026 12:23:46 +0200 Subject: [PATCH 2/2] fix: SonarCloud issues --- src/libsync/propagateupload.cpp | 10 ++++--- src/libsync/syncengine.cpp | 51 ++++++++++++++++++--------------- src/libsync/syncengine.h | 2 ++ 3 files changed, 36 insertions(+), 27 deletions(-) diff --git a/src/libsync/propagateupload.cpp b/src/libsync/propagateupload.cpp index da85aa0bc09cc..3ef12082d285d 100644 --- a/src/libsync/propagateupload.cpp +++ b/src/libsync/propagateupload.cpp @@ -296,10 +296,12 @@ void PropagateUploadFileCommon::startUploadFile() { quotaGuess = propagator()->_folderQuota.value(lookupPath); break; } - if (lookupPath == QLatin1String(".")) - break; // reached sync root with no quota entry - const int slash = lookupPath.lastIndexOf(QLatin1Char('/')); - lookupPath = slash >= 0 ? lookupPath.left(slash) : QStringLiteral("."); + if (lookupPath == QLatin1String(".")) { + lookupPath.clear(); // reached sync root with no quota entry; terminates the loop + } else { + const auto slash = lookupPath.lastIndexOf(QLatin1Char('/')); + lookupPath = slash >= 0 ? lookupPath.left(slash) : QStringLiteral("."); + } } if (_fileToUpload._size > quotaGuess) { // quotaGuess is never std::numeric_limits::max() here: reaching diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp index 380d2781f42e0..ac1a3f32afa51 100644 --- a/src/libsync/syncengine.cpp +++ b/src/libsync/syncengine.cpp @@ -1041,6 +1041,31 @@ void SyncEngine::cancelSyncOrContinue(bool cancel) } } +void SyncEngine::seedPropagatorQuota() +{ + // Seed the propagator's per-folder quota cache from the quota data gathered + // during the discovery phase (PROPFIND responses). The propagator is recreated + // fresh every sync cycle, so without this seeding the pre-upload quota check in + // PropagateUploadFileCommon::startUploadFile() would always pass on the first + // attempt of a new cycle, causing a full upload that the server then rejects with + // HTTP 507 and wasting bandwidth. By initialising the cache here, the pre-upload + // check can block oversized files from the very first attempt of each cycle. + for (const auto &syncItem : std::as_const(_syncItems)) { + if (!syncItem->isDirectory() || syncItem->_folderQuota.bytesAvailable < 0) { + continue; + } + // OwncloudPropagator keys _folderQuota by QFileInfo::path() of the file + // being uploaded, which yields "." for items at the root of the sync + // folder. Normalise the empty-string root path accordingly. + const auto key = syncItem->_file.isEmpty() ? QStringLiteral(".") : syncItem->_file; + // Only insert when there is no tighter bound already present (e.g. set + // from a prior HTTP 507 reply within the same cycle). + if (!_propagator->_folderQuota.contains(key)) { + _propagator->_folderQuota.insert(key, syncItem->_folderQuota.bytesAvailable); + } + } +} + void SyncEngine::finishSync() { auto databaseFingerprint = _journal->dataFingerprint(); @@ -1094,29 +1119,9 @@ void SyncEngine::finishSync() new OwncloudPropagator(_account, _localPath, _remotePath, _journal, _bulkUploadBlackList)); _propagator->setSyncOptions(_syncOptions); - // Seed the propagator's per-folder quota cache from the quota data gathered - // during the discovery phase (PROPFIND responses). The propagator is recreated - // fresh every sync cycle, so without this seeding the pre-upload quota check in - // PropagateUploadFileCommon::startUploadFile() would always pass on the first - // attempt of a new cycle, causing a full upload that the server then rejects with - // HTTP 507 and wasting bandwidth. By initialising the cache here, the pre-upload - // check can block oversized files from the very first attempt of each cycle. - // - // NOTE: This loop must remain before _propagator->start(std::move(_syncItems)) - // below, because start() transfers ownership of _syncItems and leaves it empty. - for (const auto &syncItem : std::as_const(_syncItems)) { - if (syncItem->isDirectory() && syncItem->_folderQuota.bytesAvailable >= 0) { - // OwncloudPropagator keys _folderQuota by QFileInfo::path() of the file - // being uploaded, which yields "." for items at the root of the sync - // folder. Normalise the empty-string root path accordingly. - const auto key = syncItem->_file.isEmpty() ? QStringLiteral(".") : syncItem->_file; - // Only insert when there is no tighter bound already present (e.g. set - // from a prior HTTP 507 reply within the same cycle). - if (!_propagator->_folderQuota.contains(key)) { - _propagator->_folderQuota.insert(key, syncItem->_folderQuota.bytesAvailable); - } - } - } + // Must be called before _propagator->start(std::move(_syncItems)) below, + // because start() transfers ownership of _syncItems and leaves it empty. + seedPropagatorQuota(); connect(_propagator.data(), &OwncloudPropagator::itemCompleted, this, &SyncEngine::slotItemCompleted); connect(_propagator.data(), &OwncloudPropagator::progress, diff --git a/src/libsync/syncengine.h b/src/libsync/syncengine.h index a84b92c8be1bd..acb4bae04b0e8 100644 --- a/src/libsync/syncengine.h +++ b/src/libsync/syncengine.h @@ -363,6 +363,8 @@ private slots: void finishSync(); + void seedPropagatorQuota(); + [[nodiscard]] bool shouldRestartSync() const; bool handleMassDeletion();