Skip to content

Commit

Permalink
Merge branch 'KDESKTOP-1468-Fix-BackError-in-tests' of https://github…
Browse files Browse the repository at this point in the history 
….com/Infomaniak/desktop-kDrive into KDESKTOP-1468-Fix-BackError-in-tests
  • Loading branch information
@herve-er
herve-er committed Jan 8, 2025
2 parents 14e75c7 + 5d39daf commit e2e0ba7
Show file tree
Hide file tree
Showing 7 changed files with 190 additions and 102 deletions.
3 changes: 3 additions & 0 deletions .github/workflows/macos.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ jobs:
runs-on: [ self-hosted, macOS, desktop-kdrive ]

steps:
- name: Clean-up XCode DerivedData
run : rm -rf /Users/runner/Library/Developer/Xcode/DerivedData

- name: Checkout the code
uses: actions/[email protected]
with:
Expand Down
3 changes: 2 additions & 1 deletion CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,8 @@ elseif(BUILD_CLIENT)
else ()
# Default sync exclude list
if (CMAKE_BUILD_TYPE STREQUAL "Debug")
file(COPY sync-exclude-linux.lst DESTINATION ${CMAKE_INSTALL_PREFIX}/sync-exclude.lst)
file(COPY sync-exclude-linux.lst DESTINATION ${CMAKE_BINARY_DIR}/bin)
file(RENAME ${CMAKE_BINARY_DIR}/bin/sync-exclude-linux.lst ${CMAKE_BINARY_DIR}/bin/sync-exclude.lst)
else()
install(FILES sync-exclude-linux.lst DESTINATION ${SYSCONFDIR}/${APPLICATION_SHORTNAME} RENAME sync-exclude.lst)
endif ()
Expand Down
13 changes: 10 additions & 3 deletions src/libcommonserver/utility/utility.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -246,14 +246,21 @@ std::wstring Utility::formatStdError(const SyncPath &path, const std::error_code
return ss.str();
}

std::wstring Utility::formatIoError(const SyncPath &path, IoError ioError) {
std::wstring Utility::formatIoError(const IoError ioError) {
std::wstringstream ss;
ss << L"path='" << Path2WStr(path) << L"', err='" << s2ws(IoHelper::ioError2StdString(ioError)) << L"'";
ss << s2ws(IoHelper::ioError2StdString(ioError));

return ss.str();
}

std::wstring Utility::formatIoError(const QString &path, IoError ioError) {
std::wstring Utility::formatIoError(const SyncPath &path, const IoError ioError) {
std::wstringstream ss;
ss << L"path='" << Path2WStr(path) << L"', err='" << formatIoError(ioError) << L"'";

return ss.str();
}

std::wstring Utility::formatIoError(const QString &path, const IoError ioError) {
return formatIoError(QStr2Path(path), ioError);
}

Expand Down
1 change: 1 addition & 0 deletions src/libcommonserver/utility/utility.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ struct COMMONSERVER_EXPORT Utility {

static std::wstring formatStdError(const std::error_code &ec);
static std::wstring formatStdError(const SyncPath &path, const std::error_code &ec);
static std::wstring formatIoError(IoError ioError);
static std::wstring formatIoError(const SyncPath &path, IoError ioError);
static std::wstring formatIoError(const QString &path, IoError ioError);
static std::wstring formatSyncName(const SyncName &name);
Expand Down
168 changes: 92 additions & 76 deletions src/libsyncengine/jobs/network/API_v2/downloadjob.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,13 @@ DownloadJob::DownloadJob(int driveDbId, const NodeId &remoteFileId, const SyncPa
}

DownloadJob::~DownloadJob() {
// Remove tmp file
// For a remote CREATE operation, the tmp file should no longer exist, but if an error occurred in handleResponse, it must be
// deleted
if (!removeTmpFile() && !_isCreate) {
LOGW_WARN(_logger, L"Failed to remove tmp file: " << Utility::formatSyncPath(_tmpPath));
}

if (_responseHandlingCanceled) {
if (_vfsSetPinState) {
if (!_vfsSetPinState(_localpath, PinState::OnlineOnly)) {
Expand Down Expand Up @@ -208,7 +215,6 @@ bool DownloadJob::handleResponse(std::istream &is) {
return false;
}

SyncPath tmpPath;
std::ofstream output;
do {
// Create/fetch normal file
Expand All @@ -218,13 +224,13 @@ bool DownloadJob::handleResponse(std::istream &is) {
const std::string tmpFileName = "kdrive_" + CommonUtility::generateRandomStringAlphaNum();
#endif

tmpPath = tmpDirectoryPath / tmpFileName;
_tmpPath = tmpDirectoryPath / tmpFileName;

output.open(tmpPath.native().c_str(), std::ofstream::out | std::ofstream::binary);
output.open(_tmpPath.native().c_str(), std::ofstream::out | std::ofstream::binary);
if (!output.is_open()) {
LOGW_WARN(_logger, L"Failed to open tmp file: " << Utility::formatSyncPath(tmpPath));
LOGW_WARN(_logger, L"Failed to open tmp file: " << Utility::formatSyncPath(_tmpPath));
_exitCode = ExitCode::SystemError;
_exitCause = Utility::enoughSpace(tmpPath) ? ExitCause::FileAccessError : ExitCause::NotEnoughDiskSpace;
_exitCause = Utility::enoughSpace(_tmpPath) ? ExitCause::FileAccessError : ExitCause::NotEnoughDiskSpace;
return false;
}

Expand Down Expand Up @@ -308,7 +314,7 @@ bool DownloadJob::handleResponse(std::istream &is) {
std::chrono::duration<double> elapsed_seconds = std::chrono::steady_clock::now() - fileProgressTimer;
if (elapsed_seconds.count() > NOTIFICATION_DELAY || done) {
// Update fetch status
if (!_vfsUpdateFetchStatus(tmpPath, _localpath, getProgress(), fetchCanceled, fetchFinished)) {
if (!_vfsUpdateFetchStatus(_tmpPath, _localpath, getProgress(), fetchCanceled, fetchFinished)) {
LOGW_WARN(_logger, L"Error in vfsUpdateFetchStatus: " << Utility::formatSyncPath(_localpath));
fetchError = true;
break;
Expand All @@ -326,8 +332,9 @@ bool DownloadJob::handleResponse(std::istream &is) {
// Unfortunately, the file hash is not available, so we check only its size
output.flush();
output.seekp(0, std::ios_base::end);
if (expectedSize != output.tellp()) {
if (expectedSize != Poco::Net::HTTPMessage::UNKNOWN_CONTENT_LENGTH && output.tellp() != expectedSize) {
LOG_WARN(_logger, "Request " << jobId() << ": tmp file has been corrupted by another process");
sentry::Handler::captureMessage(sentry::Level::Error, "DownloadJob::handleResponse", "Tmp file is corrupted");
writeError = true;
}

Expand All @@ -344,7 +351,7 @@ bool DownloadJob::handleResponse(std::istream &is) {
if (!_responseHandlingCanceled) {
if (_vfsUpdateFetchStatus && !fetchFinished) {
// Update fetch status
if (!_vfsUpdateFetchStatus(tmpPath, _localpath, getProgress(), fetchCanceled, fetchFinished)) {
if (!_vfsUpdateFetchStatus(_tmpPath, _localpath, getProgress(), fetchCanceled, fetchFinished)) {
LOGW_WARN(_logger, L"Error in vfsUpdateFetchStatus: " << Utility::formatSyncPath(_localpath));
fetchError = true;
} else if (fetchCanceled) {
Expand All @@ -357,8 +364,8 @@ bool DownloadJob::handleResponse(std::istream &is) {
} else if (!_vfsUpdateFetchStatus) {
// Replace file by tmp one
bool replaceError = false;
if (!moveTmpFile(tmpPath, restartSync)) {
LOGW_WARN(_logger, L"Failed to replace file by tmp one: " << Utility::formatSyncPath(tmpPath));
if (!moveTmpFile(restartSync)) {
LOGW_WARN(_logger, L"Failed to replace file by tmp one: " << Utility::formatSyncPath(_tmpPath));
replaceError = true;
}

Expand All @@ -368,12 +375,6 @@ bool DownloadJob::handleResponse(std::istream &is) {

if (_responseHandlingCanceled) {
// NB: VFS reset is done in the destructor

// Remove tmp file
if (!removeTmpFile(tmpPath)) {
LOGW_WARN(_logger, L"Failed to remove tmp file: " << Utility::formatSyncPath(tmpPath));
}

if (isAborted() || fetchCanceled) {
// Download aborted or canceled by the user
_exitCode = ExitCode::Ok;
Expand Down Expand Up @@ -505,95 +506,110 @@ bool DownloadJob::createLink(const std::string &mimeType, const std::string &dat
return true;
}

bool DownloadJob::removeTmpFile(const SyncPath &path) {
if (std::error_code ec; !std::filesystem::remove_all(path, ec)) {
LOGW_WARN(_logger, L"Failed to remove all: " << Utility::formatStdError(path, ec));
bool DownloadJob::removeTmpFile() {
if (_tmpPath.empty()) return true;

if (std::error_code ec; !std::filesystem::remove_all(_tmpPath, ec)) {
LOGW_WARN(_logger, L"Failed to remove a downloaded temporary file: " << Utility::formatStdError(_tmpPath, ec));
return false;
}

return true;
}

bool DownloadJob::moveTmpFile(const SyncPath &tmpPath, bool &restartSync) {
bool DownloadJob::moveTmpFile(bool &restartSync) {
restartSync = false;

// Move downloaded file from tmp directory to sync directory
std::error_code ec;
#ifdef _WIN32
bool retry = true;
int counter = 50;
while (retry) {
retry = false;
#endif

IoError ioError = IoError::Success;
IoHelper::moveItem(tmpPath, _localpath, ioError);
const bool crossDeviceLink = ioError == IoError::CrossDeviceLink;
if (ioError != IoError::Success && !crossDeviceLink) {
LOGW_WARN(_logger, L"Failed to move: " << Utility::formatIoError(tmpPath, ioError) << L" to "
<< Utility::formatSyncPath(_localpath));
return false;
bool error = false;
bool accessDeniedError = false;
bool crossDeviceLinkError = false;
#ifdef _WIN32
bool sharingViolationError = false;
#endif
if (_isCreate) {
// Move file
IoError ioError = IoError::Success;
IoHelper::moveItem(_tmpPath, _localpath, ioError);
crossDeviceLinkError = ioError == IoError::CrossDeviceLink; // Unable to move between 2 distinct file systems
if (ioError != IoError::Success && !crossDeviceLinkError) {
LOGW_WARN(_logger, L"Failed to move downloaded file " << Utility::formatSyncPath(_tmpPath) << L" to "
<< Utility::formatSyncPath(_localpath) << L", err='"
<< Utility::formatIoError(ioError) << L"'");
error = true;
accessDeniedError = ioError == IoError::AccessDenied;
// NB: On Windows, ec.value() == ERROR_SHARING_VIOLATION is translated as IoError::AccessDenied
}
}

if (crossDeviceLink) {
// The sync might be on a different file system than tmp folder.
// In that case, try to copy the file instead.
ec.clear();
std::filesystem::copy(tmpPath, _localpath, std::filesystem::copy_options::overwrite_existing, ec);
bool removed = removeTmpFile(tmpPath);
if (!_isCreate || crossDeviceLinkError) {
// Copy file content (i.e. when the target exists, do not change its node id).
std::error_code ec;
std::filesystem::copy(_tmpPath, _localpath, std::filesystem::copy_options::overwrite_existing, ec);
if (ec) {
LOGW_WARN(_logger,
L"Failed to copy to " << Utility::formatSyncPath(_localpath) << L", " << Utility::formatStdError(ec));
return false;
}
if (!removed) {
LOGW_WARN(_logger, L"Failed to remove " << Utility::formatSyncPath(tmpPath));
return false;
LOGW_WARN(_logger, L"Failed to copy downloaded file " << Utility::formatSyncPath(_tmpPath) << L" to "
<< Utility::formatSyncPath(_localpath) << L", err='"
<< Utility::formatStdError(ec) << L"'");
error = true;
accessDeniedError = IoHelper::stdError2ioError(ec.value()) == IoError::AccessDenied;
#ifdef _WIN32
sharingViolationError = ec.value() == ERROR_SHARING_VIOLATION; // In this case, we will try again
#endif
}
}

if (error) {
#ifdef _WIN32
else if (ec.value() == ERROR_SHARING_VIOLATION) {
if (counter) {
// Retry
retry = true;
Utility::msleep(10);
LOGW_DEBUG(_logger, L"Retrying to move downloaded file: " << Utility::formatSyncPath(_localpath));
counter--;
} else {
LOGW_WARN(_logger, L"Failed to rename: " << Utility::formatStdError(_localpath, ec));
return false;
if (sharingViolationError) {
if (counter) {
// Retry
retry = true;
Utility::msleep(10);
LOGW_DEBUG(_logger, L"Retrying to copy downloaded file: " << Utility::formatSyncPath(_localpath));
counter--;
continue;
} else {
return false;
}
}
}
#endif
else if (IoHelper::stdError2ioError(ec.value()) == IoError::AccessDenied) {
_exitCode = ExitCode::SystemError;
_exitCause = ExitCause::FileAccessError;
return false;
} else if (ec) {
bool exists = false;
IoError ioError = IoError::Success;
if (!IoHelper::checkIfPathExists(_localpath.parent_path(), exists, ioError)) {
LOGW_WARN(_logger,
L"Error in IoHelper::checkIfPathExists: " << Utility::formatIoError(_localpath.parent_path(), ioError));
_exitCode = ExitCode::SystemError;
_exitCause = ExitCause::Unknown;
return false;
}
if (ioError == IoError::AccessDenied) {
LOGW_WARN(_logger, L"Access denied to " << Utility::formatSyncPath(_localpath.parent_path()));

if (accessDeniedError) {
_exitCode = ExitCode::SystemError;
_exitCause = ExitCause::FileAccessError;
return false;
}
} else {
bool exists = false;
IoError ioError = IoError::Success;
if (!IoHelper::checkIfPathExists(_localpath.parent_path(), exists, ioError)) {
LOGW_WARN(_logger,
L"Error in IoHelper::checkIfPathExists: " << Utility::formatIoError(_localpath.parent_path(), ioError));
_exitCode = ExitCode::SystemError;
_exitCause = ExitCause::Unknown;
return false;
}
if (ioError == IoError::AccessDenied) {
LOGW_WARN(_logger, L"Access denied to item " << Utility::formatSyncPath(_localpath.parent_path()));
_exitCode = ExitCode::SystemError;
_exitCause = ExitCause::FileAccessError;
return false;
}

if (!exists) {
LOGW_INFO(_logger, L"Parent of item does not exist anymore: " << Utility::formatStdError(_localpath, ec));
restartSync = true;
return true;
}
if (!exists) {
LOGW_INFO(_logger, L"Parent of item does not exist anymore " << Utility::formatSyncPath(_localpath));
restartSync = true;
return true;
}

LOGW_WARN(_logger, L"Failed to rename: " << Utility::formatStdError(_localpath, ec));
return false;
return false;
}
}
#ifdef _WIN32
}
Expand Down
5 changes: 3 additions & 2 deletions src/libsyncengine/jobs/network/API_v2/downloadjob.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,12 @@ class DownloadJob : public AbstractTokenNetworkJob {
virtual bool handleResponse(std::istream &is) override;

bool createLink(const std::string &mimeType, const std::string &data);
bool removeTmpFile(const SyncPath &path);
bool moveTmpFile(const SyncPath &path, bool &restartSync);
bool removeTmpFile();
bool moveTmpFile(bool &restartSync);

NodeId _remoteFileId;
SyncPath _localpath;
SyncPath _tmpPath;
int64_t _expectedSize = -1;
SyncTime _creationTime = 0;
SyncTime _modtimeIn = 0;
Expand Down
Loading

0 comments on commit e2e0ba7

Please sign in to comment.