Add error reporting + Make temp dir erase safer

assignment-client/src/assets/AssetServer.cpp

@@ -1314,27 +1314,57 @@ void AssetServer::handleFailedBake(QString originalAssetHash, QString assetPath,
 
 void AssetServer::handleCompletedBake(QString originalAssetHash, QString originalAssetPath,
                                       QString bakedTempOutputDir) {
+    auto reportCompletion = [this, originalAssetPath, originalAssetHash](bool errorCompletingBake,
+                                                                         QString errorReason,
+                                                                         QString redirectTarget) {
+        auto type = assetTypeForFilename(originalAssetPath);
+        auto currentTypeVersion = currentBakeVersionForAssetType(type);
+
+        AssetMeta meta;
+        meta.bakeVersion = currentTypeVersion;
+        meta.failedLastBake = errorCompletingBake;
+        meta.redirectTarget = redirectTarget;
+
+        if (errorCompletingBake) {
+            qWarning() << "Could not complete bake for" << originalAssetHash;
+            meta.lastBakeErrors = errorReason;
+        }
+
+        writeMetaFile(originalAssetHash, meta);
+
+        _pendingBakes.remove(originalAssetHash);
+    };
+
     bool errorCompletingBake { false };
     QString errorReason;
+    QString redirectTarget;
 
     qDebug() << "Completing bake for " << originalAssetHash;
 
-
+    // Find the directory containing the baked content
-
     QDir outputDir(bakedTempOutputDir);
+    QString outputDirName = outputDir.dirName();
     auto directories = outputDir.entryList(QDir::Dirs | QDir::NoDotAndDotDot);
-    assert(directories.size() == 1);
     QString bakedDirectoryPath;
     for (const auto& dirName : directories) {
         outputDir.cd(dirName);
         if (outputDir.exists("baked") && outputDir.exists("original")) {
             bakedDirectoryPath = outputDir.filePath("baked");
+            break;
         }
         outputDir.cdUp();
     }
+    if (bakedDirectoryPath.isEmpty()) {
+        errorCompletingBake = true;
+        errorReason = "Failed to find baking output";
 
-    assert(!bakedDirectoryPath.isEmpty());
+        // Cleanup temporary output directory
+        PathUtils::deleteMyTemporaryDir(outputDirName);
+        reportCompletion(errorCompletingBake, errorReason, redirectTarget);
+        return;
+    }
 
+    // Compile list of all the baked files
     QDirIterator it(bakedDirectoryPath, QDirIterator::Subdirectories);
     QVector<QString> bakedFilePaths;
     while (it.hasNext()) {
@@ -1343,9 +1373,17 @@ void AssetServer::handleCompletedBake(QString originalAssetHash, QString origina
             bakedFilePaths.push_back(it.filePath());
         }
     }
+    if (bakedFilePaths.isEmpty()) {
+        errorCompletingBake = true;
+        errorReason = "Baking output has no files";
+
+        // Cleanup temporary output directory
+        PathUtils::deleteMyTemporaryDir(outputDirName);
+        reportCompletion(errorCompletingBake, errorReason, redirectTarget);
+        return;
+    }
 
     QDir bakedDirectory(bakedDirectoryPath);
-    QString redirectTarget;
 
     for (auto& filePath : bakedFilePaths) {
         // figure out the hash for the contents of this file
@@ -1359,7 +1397,7 @@ void AssetServer::handleCompletedBake(QString originalAssetHash, QString origina
             qDebug() << "Failed to open baked file: " << filePath;
             // stop handling this bake, we couldn't open one of the files for reading
             errorCompletingBake = true;
-            errorReason = "Failed to finalize bake";
+            errorReason = "Could not open baked file " + file.fileName();
             break;
         }
 
@@ -1368,7 +1406,7 @@ void AssetServer::handleCompletedBake(QString originalAssetHash, QString origina
         if (!hasher.addData(&file)) {
             // stop handling this bake, couldn't hash the contents of the file
             errorCompletingBake = true;
-            errorReason = "Failed to finalize bake";
+            errorReason = "Could not hash data for " + file.fileName();
             break;
         }
 
@@ -1388,13 +1426,15 @@ void AssetServer::handleCompletedBake(QString originalAssetHash, QString origina
 
         // setup the mapping for this bake file
         auto relativeFilePath = bakedDirectory.relativeFilePath(filePath);
-        qDebug() << "Relative file path is: " << relativeFilePath;
 
         QString bakeMapping = getBakeMapping(originalAssetHash, relativeFilePath);
 
         // Check if this is the file we should redirect to when someone asks for the original asset
         if ((relativeFilePath.endsWith(".baked.fst", Qt::CaseInsensitive) && originalAssetPath.endsWith(".fbx")) ||
             (relativeFilePath.endsWith(".texmeta.json", Qt::CaseInsensitive) && !originalAssetPath.endsWith(".fbx"))) {
+            if (!redirectTarget.isEmpty()) {
+                qWarning() << "Found multiple baked redirect target for" << originalAssetPath;
+            }
             redirectTarget = bakeMapping;
         }
 
@@ -1403,41 +1443,22 @@ void AssetServer::handleCompletedBake(QString originalAssetHash, QString origina
             qDebug() << "Failed to set mapping";
             // stop handling this bake, couldn't add a mapping for this bake file
             errorCompletingBake = true;
-            errorReason = "Failed to finalize bake";
+            errorReason = "Failed to set mapping for baked file " + file.fileName();
             break;
         }
 
         qDebug() << "Added" << bakeMapping << "for bake file" << bakedFileHash << "from bake of" << originalAssetHash;
     }
 
-    for (auto& filePath : bakedFilePaths) {
+
-        QFile file(filePath);
+    if (redirectTarget.isEmpty()) {
-        if (!file.remove()) {
+        errorCompletingBake = true;
-            qWarning() << "Failed to remove temporary file:" << filePath;
+        errorReason = "Could not find root file for baked output";
-        }
-    }
-    if (!QDir(bakedTempOutputDir).rmdir(".")) {
-        qWarning() << "Failed to remove temporary directory:" << bakedTempOutputDir;
     }
 
-    auto type = assetTypeForFilename(originalAssetPath);
+    // Cleanup temporary output directory
-    auto currentTypeVersion = currentBakeVersionForAssetType(type);
+    PathUtils::deleteMyTemporaryDir(outputDirName);
-
+    reportCompletion(errorCompletingBake, errorReason, redirectTarget);
-    assert(!redirectTarget.isEmpty());
-
-    AssetMeta meta;
-    meta.bakeVersion = currentTypeVersion;
-    meta.failedLastBake = errorCompletingBake;
-    meta.redirectTarget = redirectTarget;
-
-    if (errorCompletingBake) {
-        qWarning() << "Could not complete bake for" << originalAssetHash;
-        meta.lastBakeErrors = errorReason;
-    }
-
-    writeMetaFile(originalAssetHash, meta);
-
-    _pendingBakes.remove(originalAssetHash);
 }
 
 void AssetServer::handleAbortedBake(QString originalAssetHash, QString assetPath) {

assignment-client/src/assets/BakeAssetTask.cpp

@@ -36,21 +36,6 @@ BakeAssetTask::BakeAssetTask(const AssetUtils::AssetHash& assetHash, const Asset
     });
 }
 
-void cleanupTempFiles(QString tempOutputDir, std::vector<QString> files) {
-    for (const auto& filename : files) {
-        QFile f { filename };
-        if (!f.remove()) {
-            qDebug() << "Failed to remove:" << filename;
-        }
-    }
-    if (!tempOutputDir.isEmpty()) {
-        QDir dir { tempOutputDir };
-        if (!dir.rmdir(".")) {
-            qDebug() << "Failed to remove temporary directory:" << tempOutputDir;
-        }
-    }
-};
-
 void BakeAssetTask::run() {
     if (_isBaking.exchange(true)) {
         qWarning() << "Tried to start bake asset task while already baking";
@@ -59,11 +44,24 @@ void BakeAssetTask::run() {
 
     // Make a new temporary directory for the Oven to work in
     QString tempOutputDir = PathUtils::generateTemporaryDir();
+    QString tempOutputDirName = QDir(tempOutputDir).dirName();
+    if (tempOutputDir.isEmpty()) {
+        QString errors = "Could not create temporary working directory";
+        emit bakeFailed(_assetHash, _assetPath, errors);
+        PathUtils::deleteMyTemporaryDir(tempOutputDirName);
+        return;
+    }
 
     // Copy file to bake the temporary dir and give a name the oven can work with
     auto assetName = _assetPath.split("/").last();
     auto tempAssetPath = tempOutputDir + "/" + assetName;
-    QFile::copy(_filePath, tempAssetPath);
+    auto sucess = QFile::copy(_filePath, tempAssetPath);
+    if (!sucess) {
+        QString errors = "Couldn't copy file to bake to temporary directory";
+        emit bakeFailed(_assetHash, _assetPath, errors);
+        PathUtils::deleteMyTemporaryDir(tempOutputDirName);
+        return;
+    }
 
     auto base = QFileInfo(QCoreApplication::applicationFilePath()).absoluteDir();
     QString path = base.absolutePath() + "/oven";
@@ -79,30 +77,23 @@ void BakeAssetTask::run() {
     QEventLoop loop;
 
     connect(_ovenProcess.get(), static_cast<void(QProcess::*)(int, QProcess::ExitStatus)>(&QProcess::finished),
-            this, [&loop, this, tempOutputDir, tempAssetPath](int exitCode, QProcess::ExitStatus exitStatus) {
+            this, [&loop, this, tempOutputDir, tempAssetPath, tempOutputDirName](int exitCode, QProcess::ExitStatus exitStatus) {
         qDebug() << "Baking process finished: " << exitCode << exitStatus;
 
         if (exitStatus == QProcess::CrashExit) {
+            PathUtils::deleteMyTemporaryDir(tempOutputDirName);
             if (_wasAborted) {
                 emit bakeAborted(_assetHash, _assetPath);
             } else {
                 QString errors = "Fatal error occurred while baking";
                 emit bakeFailed(_assetHash, _assetPath, errors);
             }
-            if (!QDir(tempOutputDir).rmdir(".")) {
-                qWarning() << "Failed to remove temporary directory:" << tempOutputDir;
-            }
         } else if (exitCode == OVEN_STATUS_CODE_SUCCESS) {
-            // Remove temp copy of the original asset
-            QFile::remove(tempAssetPath);
-
             emit bakeComplete(_assetHash, _assetPath, tempOutputDir);
         } else if (exitStatus == QProcess::NormalExit && exitCode == OVEN_STATUS_CODE_ABORT) {
             _wasAborted.store(true);
+            PathUtils::deleteMyTemporaryDir(tempOutputDirName);
             emit bakeAborted(_assetHash, _assetPath);
-            if (!QDir(tempOutputDir).rmdir(".")) {
-                qWarning() << "Failed to remove temporary directory:" << tempOutputDir;
-            }
         } else {
             QString errors;
             if (exitCode == OVEN_STATUS_CODE_FAIL) {
@@ -116,10 +107,8 @@ void BakeAssetTask::run() {
                     errors = "Unknown error occurred while baking";
                 }
             }
+            PathUtils::deleteMyTemporaryDir(tempOutputDirName);
             emit bakeFailed(_assetHash, _assetPath, errors);
-            if (!QDir(tempOutputDir).rmdir(".")) {
-                qWarning() << "Failed to remove temporary directory:" << tempOutputDir;
-            }
         }
 
         loop.quit();
@@ -128,12 +117,11 @@ void BakeAssetTask::run() {
     qDebug() << "Starting oven for " << _assetPath;
     _ovenProcess->start(path, args, QIODevice::ReadOnly);
     qDebug() << "Running:" << path << args;
-    if (!_ovenProcess->waitForStarted(-1)) {
+    if (!_ovenProcess->waitForStarted()) {
+        PathUtils::deleteMyTemporaryDir(tempOutputDirName);
+
         QString errors = "Oven process failed to start";
         emit bakeFailed(_assetHash, _assetPath, errors);
-        if (!QDir(tempOutputDir).rmdir(".")) {
-            qWarning() << "Failed to remove temporary directory:" << tempOutputDir;
-        }
         return;
     }
 

libraries/shared/src/PathUtils.cpp

@@ -185,6 +185,30 @@ QString PathUtils::generateTemporaryDir() {
     return "";
 }
 
+bool PathUtils::deleteMyTemporaryDir(QString dirName) {
+    QDir rootTempDir = QDir::tempPath();
+
+    QString appName = qApp->applicationName();
+    QRegularExpression re { "^" + QRegularExpression::escape(appName) + "\\-(?<pid>\\d+)\\-(?<timestamp>\\d+)$" };
+
+    auto match = re.match(dirName);
+    auto pid = match.capturedRef("pid").toLongLong();
+
+    if (match.hasMatch() && rootTempDir.exists(dirName) && pid == qApp->applicationPid()) {
+        auto absoluteDirPath = QDir(rootTempDir.absoluteFilePath(dirName));
+
+        bool success = absoluteDirPath.removeRecursively();
+        if (success) {
+            qDebug() << "  Removing temporary directory: " << absoluteDirPath.absolutePath();
+        } else {
+            qDebug() << "  Failed to remove temporary directory: " << absoluteDirPath.absolutePath();
+        }
+        return success;
+    }
+
+    return false;
+}
+
 // Delete all temporary directories for an application
 int PathUtils::removeTemporaryApplicationDirs(QString appName) {
     if (appName.isNull()) {

libraries/shared/src/PathUtils.h

@@ -56,6 +56,7 @@ public:
     static QString getAppLocalDataFilePath(const QString& filename);
 
     static QString generateTemporaryDir();
+    static bool deleteMyTemporaryDir(QString dirName);
 
     static int removeTemporaryApplicationDirs(QString appName = QString::null);