-
Notifications
You must be signed in to change notification settings - Fork 949
fix(quota): protect local files from deletion when quota blocked upload. #10001
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
d20d4e9
d6508c3
88e3fbf
a990043
a02e0ce
bc93973
52d1af6
a431894
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,10 @@ | ||
| /* | ||
| * SPDX-FileCopyrightText: 2020 Nextcloud GmbH and Nextcloud contributors | ||
| * SPDX-FileCopyrightText: 2014 ownCloud GmbH | ||
| * SPDX-License-Identifier: LGPL-2.1-or-later | ||
| */ | ||
|
|
||
| #include <QCryptographicHash> | ||
| #include <QFile> | ||
| #include <QJsonArray> | ||
| #include <QJsonDocument> | ||
|
|
@@ -219,7 +219,7 @@ | |
|
|
||
| // Since the list is sorted, we can do a binary search. | ||
| // If the path is a prefix of another item or right after in the lexical order. | ||
| auto it = std::lower_bound(list.cbegin(), list.cend(), pathSlash); | ||
|
Check warning on line 222 in src/common/syncjournaldb.cpp
|
||
|
|
||
| if (it != list.cend() && *it == pathSlash) { | ||
| return true; | ||
|
|
@@ -1061,7 +1061,7 @@ | |
| return tr("Failed to connect database."); // checkConnect failed. | ||
| } | ||
|
|
||
| int plen = record._path.length(); | ||
|
Check warning on line 1064 in src/common/syncjournaldb.cpp
|
||
|
|
||
| QByteArray etag(record._etag); | ||
| if (etag.isEmpty()) { | ||
|
|
@@ -1169,7 +1169,7 @@ | |
| return true; | ||
| } | ||
|
|
||
| bool SyncJournalDb::listAllE2eeFoldersWithEncryptionStatusLessThan(const int status, const std::function<void(const SyncJournalFileRecord &)> &rowCallback) | ||
|
Check warning on line 1172 in src/common/syncjournaldb.cpp
|
||
| { | ||
| QMutexLocker locker(&_mutex); | ||
|
|
||
|
|
@@ -1487,7 +1487,7 @@ | |
|
|
||
| bool SyncJournalDb::getFileRecordsByFileId(const QByteArray &fileId, const std::function<void(const SyncJournalFileRecord &)> &rowCallback) | ||
| { | ||
| QMutexLocker locker(&_mutex); | ||
|
Check warning on line 1490 in src/common/syncjournaldb.cpp
|
||
|
|
||
| if (fileId.isEmpty() || _metadataTableIsEmpty) { | ||
| return true; // no error, yet nothing found (rec->isValid() == false) | ||
|
|
@@ -2171,6 +2171,36 @@ | |
| return entry; | ||
| } | ||
|
|
||
| bool SyncJournalDb::renameErrorBlacklistPaths(const QString &from, const QString &to) | ||
| { | ||
| QMutexLocker locker(&_mutex); | ||
| if (!checkConnect()) { | ||
| return false; | ||
| } | ||
|
|
||
| SqlQuery countQuery(_db); | ||
| countQuery.prepare("SELECT COUNT(*) FROM blacklist " | ||
| "WHERE errorCategory = ?1 " | ||
| "AND (path = ?2 OR (path > (?2 || '/') AND path < (?2 || '0')))"); | ||
| countQuery.bindValue(1, SyncJournalErrorBlacklistRecord::InsufficientRemoteStorage); | ||
| countQuery.bindValue(2, from); | ||
| const bool hasQuotaEntries = countQuery.exec() && countQuery.next().hasData && countQuery.intValue(0) > 0; | ||
|
|
||
| // Update the exact folder entry and all entries whose path starts with "from/". | ||
| // Uses the same range trick as IS_PREFIX_PATH_OR_EQUAL: '/' + 1 == '0'. | ||
| SqlQuery query(_db); | ||
| query.prepare("UPDATE blacklist " | ||
| "SET path = ?2 || substr(path, length(?1) + 1) " | ||
| "WHERE path == ?1 OR (path > (?1 || '/') AND path < (?1 || '0'))"); | ||
| query.bindValue(1, from); | ||
| query.bindValue(2, to); | ||
| if (!query.exec()) { | ||
| sqlFail(QStringLiteral("renameErrorBlacklistPaths"), query); | ||
| } | ||
|
Comment on lines
+2189
to
+2199
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does this need to be run if there aren't any quota entries? |
||
|
|
||
| return hasQuotaEntries; | ||
| } | ||
|
|
||
| bool SyncJournalDb::deleteStaleErrorBlacklistEntries(const QSet<QString> &keep) | ||
| { | ||
| QMutexLocker locker(&_mutex); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -7,7 +7,7 @@ | |||||
| #ifndef SYNCJOURNALDB_H | ||||||
| #define SYNCJOURNALDB_H | ||||||
|
|
||||||
| #include <QObject> | ||||||
| #include <QDateTime> | ||||||
| #include <QHash> | ||||||
| #include <QMutex> | ||||||
|
|
@@ -145,6 +145,7 @@ | |||||
|
|
||||||
| SyncJournalErrorBlacklistRecord errorBlacklistEntry(const QString &); | ||||||
| [[nodiscard]] bool deleteStaleErrorBlacklistEntries(const QSet<QString> &keep); | ||||||
| bool renameErrorBlacklistPaths(const QString &from, const QString &to); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
|
||||||
| /// Delete flags table entries that have no metadata correspondent | ||||||
| void deleteStaleFlagsEntries(); | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,3 @@ | ||
| /* | ||
| * SPDX-FileCopyrightText: 2021 Nextcloud GmbH and Nextcloud contributors | ||
| * SPDX-FileCopyrightText: 2018 ownCloud GmbH | ||
|
|
@@ -29,7 +29,7 @@ | |
| namespace | ||
| { | ||
| constexpr const char *editorNamesForDelayedUpload[] = {"PowerPDF"}; | ||
| constexpr const char *fileExtensionsToCheckIfOpenForSigning[] = {".pdf"}; | ||
|
Check warning on line 32 in src/libsync/discovery.cpp
|
||
| constexpr auto delayIntervalForSyncRetryForOpenedForSigningFilesSeconds = 60; | ||
| constexpr auto delayIntervalForSyncRetryForFilesExceedQuotaSeconds = 60; | ||
| } | ||
|
|
@@ -61,7 +61,7 @@ | |
| computePinState(parent->_pinState); | ||
| } | ||
|
|
||
| ProcessDirectoryJob::ProcessDirectoryJob(DiscoveryPhase *data, PinState basePinState, const PathTuple &path, const SyncFileItemPtr &dirItem, const SyncFileItemPtr &parentDirItem, QueryMode queryLocal, qint64 lastSyncTimestamp, QObject *parent) | ||
|
Check warning on line 64 in src/libsync/discovery.cpp
|
||
| : QObject(parent) | ||
| , _dirItem(dirItem) | ||
| , _dirParentItem(parentDirItem) | ||
|
|
@@ -252,7 +252,7 @@ | |
| QTimer::singleShot(0, _discoveryData, &DiscoveryPhase::scheduleMoreJobs); | ||
| } | ||
|
|
||
| bool ProcessDirectoryJob::handleExcluded(const QString &path, const Entries &entries, const std::map<QString, Entries> &allEntries, const bool isHidden, const bool isBlacklisted) | ||
|
Check failure on line 255 in src/libsync/discovery.cpp
|
||
| { | ||
| const auto isDirectory = entries.localEntry.isDirectory || entries.serverEntry.isDirectory; | ||
|
|
||
|
|
@@ -276,7 +276,7 @@ | |
| || excluded == CSYNC_FILE_EXCLUDE_TRAILING_SPACE | ||
| || excluded == CSYNC_FILE_EXCLUDE_LEADING_AND_TRAILING_SPACE; | ||
|
|
||
| const auto leadingAndTrailingSpacesFilesAllowed = !_discoveryData->_shouldEnforceWindowsFileNameCompatibility || _discoveryData->_leadingAndTrailingSpacesFilesAllowed.contains(_discoveryData->_localDir + path); | ||
|
Check warning on line 279 in src/libsync/discovery.cpp
|
||
| #if defined Q_OS_WINDOWS | ||
| if (hasLeadingOrTrailingSpaces && leadingAndTrailingSpacesFilesAllowed) { | ||
| #else | ||
|
|
@@ -403,7 +403,7 @@ | |
| } else { | ||
| char invalid = '\0'; | ||
| constexpr QByteArrayView invalidChars("\\:?*\"<>|"); | ||
| for (const auto x : invalidChars) { | ||
|
Check failure on line 406 in src/libsync/discovery.cpp
|
||
| if (item->_file.contains(x)) { | ||
| invalid = x; | ||
| break; | ||
|
|
@@ -425,7 +425,7 @@ | |
| } | ||
| item->_status = SyncFileItem::FileNameInvalid; | ||
| break; | ||
| case CSYNC_FILE_EXCLUDE_TRAILING_SPACE: | ||
|
Check warning on line 428 in src/libsync/discovery.cpp
|
||
| item->_errorString = tr("Filename contains trailing spaces."); | ||
| item->_status = SyncFileItem::FileNameInvalid; | ||
| if (isLocal && !maybeRenameForWindowsCompatibility(_discoveryData->_localDir + item->_file, excluded)) { | ||
|
|
@@ -1059,9 +1059,14 @@ | |
| } else { | ||
| // we need to make a request to the server to know that the original file is deleted on the server | ||
| _pendingAsyncJobs++; | ||
| // Mark this path as a pending rename check so that children blocked from upload | ||
| // because of quota errors inside a queued deleted-directory job do not cancel the | ||
| // parent's REMOVE instruction before rename detection can claim it. | ||
| _discoveryData->_pendingRenameSourcePaths.insert(originalPath); | ||
| const auto job = new RequestEtagJob(_discoveryData->_account, _discoveryData->_remoteFolder + originalPath, this); | ||
| connect(job, &RequestEtagJob::finishedWithResult, this, [=, this](const HttpResult<QByteArray> &etag) mutable { | ||
|
Check failure on line 1067 in src/libsync/discovery.cpp
|
||
| _pendingAsyncJobs--; | ||
| _discoveryData->_pendingRenameSourcePaths.remove(originalPath); | ||
| QTimer::singleShot(0, _discoveryData, &DiscoveryPhase::scheduleMoreJobs); | ||
| if (etag || etag.error().code != 404 || | ||
| // Somehow another item claimed this original path, consider as if it existed | ||
|
|
@@ -1231,6 +1236,11 @@ | |
| } | ||
|
|
||
| item->_status = SyncFileItem::Status::NormalError; | ||
| // Mark as a quota error so blacklistUpdate writes an InsufficientRemoteStorage entry. | ||
| // Without this, _httpErrorCode would be 0 and blacklistUpdate would not create an | ||
| // entry, leaving the file unprotected by checkNewDeleteConflict if the remote parent | ||
| // folder is deleted before the quota situation is resolved. | ||
| item->_httpErrorCode = 507; | ||
|
Comment on lines
+1239
to
+1243
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this seems like a hack to me, we probably shouldn't fake a returned HTTP error code looking at |
||
| _discoveryData->_anotherSyncNeeded = true; | ||
| _discoveryData->_filesNeedingScheduledSync.insert(path._original, delayIntervalForSyncRetryForFilesExceedQuotaSeconds); | ||
| } | ||
|
|
@@ -1467,7 +1477,7 @@ | |
| return; | ||
| } | ||
|
|
||
| if (checkNewDeleteConflict(item)) { | ||
| if (checkNewDeleteConflict(item, localEntry.size)) { | ||
| return; | ||
| } | ||
|
|
||
|
|
@@ -1570,7 +1580,7 @@ | |
| const auto isE2eeMove = isMove && (base.isE2eEncrypted() || isInsideEncryptedTree()); | ||
| const auto isCfApiVfsMode = _discoveryData->_syncOptions._vfs && _discoveryData->_syncOptions._vfs->mode() == Vfs::WindowsCfApi; | ||
| const bool isOnlineOnlyItem = isCfApiVfsMode && (localEntry.isDirectory || _discoveryData->_syncOptions._vfs->isDehydratedPlaceholder(_discoveryData->_localDir + path._local)); | ||
| const auto isE2eeMoveOnlineOnlyItemWithCfApi = isE2eeMove && isOnlineOnlyItem; | ||
|
Check warning on line 1583 in src/libsync/discovery.cpp
|
||
|
|
||
| if (isE2eeMove) { | ||
| qCDebug(lcDisco) << "requesting permanent deletion for" << originalPath; | ||
|
|
@@ -1650,7 +1660,7 @@ | |
| return; | ||
| } | ||
|
|
||
| auto wasDeletedOnClient = _discoveryData->findAndCancelDeletedJob(originalPath); | ||
|
Check warning on line 1663 in src/libsync/discovery.cpp
|
||
|
|
||
| auto processRename = [item, originalPath, base, this](PathTuple &path) { | ||
| auto adjustedOriginalPath = _discoveryData->adjustRenamedPath(originalPath, SyncFileItem::Down); | ||
|
|
@@ -1694,7 +1704,7 @@ | |
| if (base.isVirtualFile() && isVfsWithSuffix()) | ||
| chopVirtualFileSuffix(serverOriginalPath); | ||
| auto job = new RequestEtagJob(_discoveryData->_account, serverOriginalPath, this); | ||
| connect(job, &RequestEtagJob::finishedWithResult, this, [=, this](const HttpResult<QByteArray> &etag) mutable { | ||
|
Check failure on line 1707 in src/libsync/discovery.cpp
|
||
|
|
||
|
|
||
| if (!etag || (etag.get() != base._etag && !item->isDirectory()) || _discoveryData->isRenamed(originalPath) | ||
|
|
@@ -1740,7 +1750,7 @@ | |
| // If there's no content hash, use heuristics | ||
| if (serverEntry.checksumHeader.isEmpty()) { | ||
| // If the size or mtime is different, it's definitely a conflict. | ||
| bool isConflict = (serverEntry.size != localEntry.size) || (serverEntry.modtime != localEntry.modtime) || (dbEntry.isValid() && dbEntry._modtime != localEntry.modtime && serverEntry.modtime == localEntry.modtime); | ||
|
Check warning on line 1753 in src/libsync/discovery.cpp
|
||
|
|
||
| // It could be a conflict even if size and mtime match! | ||
| // | ||
|
|
@@ -1896,7 +1906,7 @@ | |
| item->_direction = _dirItem->_direction; | ||
| } | ||
|
|
||
| { | ||
|
Check warning on line 1909 in src/libsync/discovery.cpp
|
||
| const auto isImportantInstruction = item->_instruction != CSYNC_INSTRUCTION_NONE; | ||
| if (isImportantInstruction) { | ||
| qCInfo(lcDisco).noquote() << item->_discoveryResult; | ||
|
|
@@ -2117,7 +2127,7 @@ | |
| } | ||
|
|
||
| const auto isMatchingFileExtension = std::find_if(std::cbegin(fileExtensionsToCheckIfOpenForSigning), std::cend(fileExtensionsToCheckIfOpenForSigning), | ||
| [path](const auto &matchingExtension) { | ||
|
Check warning on line 2130 in src/libsync/discovery.cpp
|
||
| return path._local.endsWith(matchingExtension, Qt::CaseInsensitive); | ||
| }) != std::cend(fileExtensionsToCheckIfOpenForSigning); | ||
|
|
||
|
|
@@ -2130,7 +2140,7 @@ | |
|
|
||
| for (const auto &detectedEditorName : editorsKeepingFileBusy) { | ||
| const auto isMatchingEditorFound = std::find_if(std::cbegin(editorNamesForDelayedUpload), std::cend(editorNamesForDelayedUpload), | ||
| [detectedEditorName](const auto &matchingEditorName) { | ||
|
Check warning on line 2143 in src/libsync/discovery.cpp
|
||
| return detectedEditorName.processName.startsWith(matchingEditorName, Qt::CaseInsensitive); | ||
| }) != std::cend(editorNamesForDelayedUpload); | ||
| if (isMatchingEditorFound) { | ||
|
|
@@ -2213,6 +2223,13 @@ | |
| // Do not remove a directory that has ignored files | ||
| qCInfo(lcDisco) << "Child ignored for a folder to remove" << _dirItem->_file << "direction" << _dirItem->_direction; | ||
| _dirItem->_instruction = CSYNC_INSTRUCTION_NONE; | ||
| // Invalidate the parent directory's etag so the next sync queries it from | ||
| // the server instead of using a ParentNotChanged cache hit. Without this, a | ||
| // parent whose etag has not changed since the server side deletion uses its DB | ||
| // record as a proxy for server state and keeps issuing NONE for this folder. | ||
| const auto slashPos = _dirItem->_file.lastIndexOf(QLatin1Char('/')); | ||
| const auto parentPath = slashPos >= 0 ? _dirItem->_file.left(slashPos) : QString(); | ||
| _discoveryData->_statedb->schedulePathForRemoteDiscovery(parentPath.toUtf8()); | ||
| } | ||
| } | ||
| emit finished(); | ||
|
|
@@ -2483,7 +2500,7 @@ | |
| case CSYNC_FILE_EXCLUDE_LEADING_SPACE: | ||
| case CSYNC_FILE_EXCLUDE_TRAILING_SPACE: | ||
| { | ||
| const auto removeTrailingSpaces = [] (QString string) -> QString { | ||
|
Check warning on line 2503 in src/libsync/discovery.cpp
|
||
| for (int n = string.size() - 1; n >= 0; -- n) { | ||
| if (!string.at(n).isSpace()) { | ||
| string.truncate(n + 1); | ||
|
|
@@ -2504,18 +2521,67 @@ | |
| return result; | ||
| } | ||
|
|
||
| bool ProcessDirectoryJob::checkNewDeleteConflict(const SyncFileItemPtr &item) const | ||
| bool ProcessDirectoryJob::checkNewDeleteConflict(const SyncFileItemPtr &item, int64_t localFileSize) | ||
|
Check warning on line 2524 in src/libsync/discovery.cpp
|
||
| { | ||
| if (_discoveryData->recursiveCheckForDeletedParents(item->_file)) { | ||
| qCWarning(lcDisco) << "Removing local file inside a remotely deleted folder" << item->_file; | ||
| item->_instruction = CSYNC_INSTRUCTION_REMOVE; | ||
| item->_direction = SyncFileItem::Down; | ||
| item->_wantsSpecificActions = SyncFileItem::SynchronizationOptions::MoveToClientTrashBin; | ||
| if (!_discoveryData->recursiveCheckForDeletedParents(item->_file)) { | ||
| return false; | ||
| } | ||
|
|
||
| // Deleting the local copy could result in permanent data loss if the file was never in the | ||
| // server and blocked from being uploaded by a quota error. | ||
| // Protect it instead and let the user resolve the storage situation first. | ||
| if (const auto blacklistEntry = _discoveryData->_statedb->errorBlacklistEntry(item->_file); | ||
| blacklistEntry.isValid() | ||
| && blacklistEntry._errorCategory == SyncJournalErrorBlacklistRecord::InsufficientRemoteStorage) { | ||
| qCWarning(lcDisco) << "Not removing local file inside a remotely deleted folder: " | ||
| "file was never uploaded due to storage quota —" | ||
| << item->_file; | ||
| item->_instruction = CSYNC_INSTRUCTION_ERROR; | ||
| item->_status = SyncFileItem::SoftError; | ||
| // Keep _httpErrorCode at 507 so blacklistUpdate recreates the InsufficientRemoteStorage | ||
| // entry if the ignore duration later expires. | ||
| item->_httpErrorCode = 507; | ||
| item->_errorString = tr("\"%1\" was not deleted because its latest changes were not synced " | ||
| "and your server quota was exceeded. " | ||
| "Please manage your storage and try syncing again.") | ||
| .arg(item->_file); | ||
| // Prevent the parent directory from being deleted while a file with an error exists. | ||
| _childIgnored = true; | ||
| emit _discoveryData->itemDiscovered(item); | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| // No prior blacklist entry. For files blocked from upload due to a quota error in the same | ||
| // sync as the folder deletion, check the parent folder's last known quota from the DB. | ||
| if (!item->isDirectory() && localFileSize > 0 && _dirItem) { | ||
| SyncJournalFileRecord dirItemDbRecord; | ||
| if (_discoveryData->_statedb->getFileRecord(_dirItem->_file, &dirItemDbRecord) | ||
| && dirItemDbRecord.isValid()) { | ||
| const auto bytesAvailable = dirItemDbRecord._folderQuota.bytesAvailable; | ||
| if (bytesAvailable >= 0 && localFileSize > bytesAvailable) { | ||
| qCWarning(lcDisco) << "Not removing local file inside a remotely deleted folder: " | ||
| "file would exceed last known parent folder quota —" | ||
| << item->_file; | ||
| item->_instruction = CSYNC_INSTRUCTION_ERROR; | ||
| item->_status = SyncFileItem::SoftError; | ||
| item->_httpErrorCode = 507; | ||
| item->_errorString = tr("\"%1\" was not deleted because its latest changes were not synced " | ||
| "and your server quota was exceeded. " | ||
| "Please manage your storage and try syncing again.") | ||
| .arg(item->_file); | ||
| _childIgnored = true; | ||
| emit _discoveryData->itemDiscovered(item); | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| qCWarning(lcDisco) << "Removing local file inside a remotely deleted folder" << item->_file; | ||
| item->_instruction = CSYNC_INSTRUCTION_REMOVE; | ||
| item->_direction = SyncFileItem::Down; | ||
| item->_wantsSpecificActions = SyncFileItem::SynchronizationOptions::MoveToClientTrashBin; | ||
| emit _discoveryData->itemDiscovered(item); | ||
| return true; | ||
| } | ||
|
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use prepared SQL statements in this method (similar to how it's done in e.g. getFileRecordsByFileId)