From 7fc7657ee4f1b72bf23584280dced8b955f2513b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20I=C3=B1igo=20Blasco?= Date: Thu, 26 Mar 2026 10:22:58 +0100 Subject: [PATCH] fix: defer uninstall to next restart when DLL is locked on Windows On Windows, uninstalling a plugin while it is loaded by the host process causes QDir::removeRecursively() to fail silently. This change aligns the uninstall flow with the existing install staging pattern: - ExtensionManager: new signal `uninstallPendingRestart`, new public method `applyPendingUninstalls()`, private helper `schedulePendingUninstall()` - MarketplaceWindow: subscribes to `uninstallPendingRestart` and shows the appropriate "restart required" feedback to the user When removeRecursively() fails on Windows, uninstall() now: 1. Writes a `.pj_pending_uninstall` marker inside the extension directory 2. Removes the extension from the in-memory registry immediately 3. Emits the new `uninstallPendingRestart(id)` signal instead of an error At the next application startup, `applyPendingUninstalls()` scans extensions_dir for directories carrying the marker and deletes them. --- .../pj_marketplace/extension_manager.hpp | 13 +++++++ pj_marketplace/src/core/ExtensionManager.cpp | 32 ++++++++++++--- pj_marketplace/src/ui/marketplace_window.cpp | 11 +++++- .../tests/extension_manager_test.cpp | 39 +++++++++++++++++++ 4 files changed, 89 insertions(+), 6 deletions(-) diff --git a/pj_marketplace/include/pj_marketplace/extension_manager.hpp b/pj_marketplace/include/pj_marketplace/extension_manager.hpp index b3b2784f..5381fcd7 100644 --- a/pj_marketplace/include/pj_marketplace/extension_manager.hpp +++ b/pj_marketplace/include/pj_marketplace/extension_manager.hpp @@ -21,7 +21,10 @@ class DownloadManager; // DownloadManager, then registers the extension immediately // - On Windows: extracts to a staging directory (.pending/) because in-use DLLs // cannot be overwritten; the extension becomes active after the next restart +// - On Windows uninstall: if the directory cannot be removed (DLL still mapped), +// schedules it for deletion at the next startup via applyPendingUninstalls() // - At startup: applies any pending staged installs via applyPendingInstalls() +// and deletes any directories deferred from a previous uninstall via applyPendingUninstalls() // - Discovers installed extensions by scanning extensions_dir and reading manifest.json // // All constructor dependencies are injected, so tests can pass a DownloadManager stub @@ -65,6 +68,11 @@ class ExtensionManager : public QObject { // because staging is never used, but it is safe to call on any platform. void applyPendingInstalls(); + // Deletes any extension directories that could not be removed during a previous + // uninstall() because their DLL was still loaded (Windows only). + // Should be called once at application startup. Safe to call on any platform. + void applyPendingUninstalls(); + bool isInstalled(const QString& id) const; // Compares the registry version against the installed one using QVersionNumber, @@ -86,12 +94,17 @@ class ExtensionManager : public QObject { void uninstallFinished(const QString& id, bool success); void uninstallError(const QString& id, const QString& error_message); + // Emitted on Windows when the extension is deregistered but its directory could not + // be removed (DLL still loaded). The directory will be deleted on the next startup + // via applyPendingUninstalls(). + void uninstallPendingRestart(const QString& id); private: void loadState(); void saveState(); void disconnectDlConns(); void savePendingMeta(const Extension& ext); + void schedulePendingUninstall(const QString& path); DownloadManager* downloader_; QString extensions_dir_; diff --git a/pj_marketplace/src/core/ExtensionManager.cpp b/pj_marketplace/src/core/ExtensionManager.cpp index 4331df16..519c9db0 100644 --- a/pj_marketplace/src/core/ExtensionManager.cpp +++ b/pj_marketplace/src/core/ExtensionManager.cpp @@ -27,6 +27,7 @@ ExtensionManager::ExtensionManager(DownloadManager* downloader, const QString& e } static constexpr const char* kManifestFileName = "manifest.json"; +static constexpr const char* kPendingUninstallMarker = ".pj_pending_uninstall"; // --------------------------------------------------------------------------- // Public interface @@ -174,11 +175,17 @@ void ExtensionManager::uninstall(const QString& extension_id) { const QString dir_path = installed_[extension_id].path; if (!QDir(dir_path).removeRecursively()) { - // On Windows the DLL may still be mapped by the host process (F-14, staging deferred to April+). - // Report the error rather than corrupting the state file with a phantom entry. - emit uninstallError( - extension_id, QString("Could not remove directory \"%1\" — the plugin may still be loaded").arg(dir_path)); - emit uninstallFinished(extension_id, false); + if (PlatformUtils::isWindows()) { + // The DLL is still mapped by the host process. Deregister the extension immediately + // and mark the directory for deletion at the next startup. + schedulePendingUninstall(dir_path); + installed_.remove(extension_id); + emit uninstallPendingRestart(extension_id); + } else { + emit uninstallError( + extension_id, QString("Could not remove directory \"%1\" — the plugin may still be loaded").arg(dir_path)); + emit uninstallFinished(extension_id, false); + } return; } @@ -269,6 +276,16 @@ void ExtensionManager::applyPendingInstalls() { } } +void ExtensionManager::applyPendingUninstalls() { + const QDir dir(extensions_dir_); + for (const QFileInfo& entry : dir.entryInfoList(QDir::Dirs | QDir::NoDotAndDotDot)) { + if (!QFile::exists(entry.absoluteFilePath() + "/" + kPendingUninstallMarker)) { + continue; + } + QDir(entry.absoluteFilePath()).removeRecursively(); + } +} + bool ExtensionManager::isInstalled(const QString& id) const { return installed_.contains(id); } @@ -300,6 +317,11 @@ void ExtensionManager::disconnectDlConns() { disconnect(dl_cancelled_conn_); } +void ExtensionManager::schedulePendingUninstall(const QString& path) { + QFile marker(path + "/" + kPendingUninstallMarker); + marker.open(QIODevice::WriteOnly); // content irrelevant; existence is the signal +} + void ExtensionManager::savePendingMeta(const Extension& ext) { QJsonObject obj; obj["id"] = ext.id; diff --git a/pj_marketplace/src/ui/marketplace_window.cpp b/pj_marketplace/src/ui/marketplace_window.cpp index 4190bc89..a1a7cfce 100644 --- a/pj_marketplace/src/ui/marketplace_window.cpp +++ b/pj_marketplace/src/ui/marketplace_window.cpp @@ -42,6 +42,7 @@ MarketplaceWindow::MarketplaceWindow(const QUrl& registry_url, QWidget* parent) setupUi(); setupSignals(); ext_mgr_->applyPendingInstalls(); + ext_mgr_->applyPendingUninstalls(); registry_mgr_->fetchRegistry(registry_url_); // extensions_ is now populated via the fetchFinished signal above. } @@ -100,7 +101,15 @@ void MarketplaceWindow::setupSignals() { ui_->progress_bar_->setVisible(false); populateCards(); setStatus("Extension staged — will be active after restart"); - }); + }); + + connect(ext_mgr_, &ExtensionManager::uninstallPendingRestart, this, + [this](const QString& id) { + pending_restart_ids_.insert(id); + ui_->progress_bar_->setVisible(false); + populateCards(); + setStatus("Extension staged — will be uninstalled after restart"); + }); connect(registry_mgr_, &RegistryManager::fetchError, this, [this](const QString& error) { setStatus("Registry error: " + error, true); }); diff --git a/pj_marketplace/tests/extension_manager_test.cpp b/pj_marketplace/tests/extension_manager_test.cpp index df5fb24a..bf5145bb 100644 --- a/pj_marketplace/tests/extension_manager_test.cpp +++ b/pj_marketplace/tests/extension_manager_test.cpp @@ -9,6 +9,7 @@ // [6] applyPendingInstalls: simulates the Windows post-restart staging path // [7] State persistence: installed state derived from disk across manager restarts // [8] Platform detection: currentPlatform() format and registry key resolution +// [9] applyPendingUninstalls: deferred directory cleanup via marker file #include @@ -665,6 +666,44 @@ TEST_F(ExtensionManagerTest, FreshManagerHasNoInstalledExtensions) { EXPECT_TRUE(mgr_->installedExtensions().isEmpty()); } +// --------------------------------------------------------------------------- +// [9] applyPendingUninstalls — deferred directory cleanup simulation +// +// On Windows, uninstall() writes a .pj_pending_uninstall marker inside the +// extension directory when removeRecursively() fails. On the next startup, +// applyPendingUninstalls() removes every extension directory that carries that +// marker. These tests create that state manually so the cleanup logic can be +// verified on any platform. +// --------------------------------------------------------------------------- + +// A directory containing the marker is removed by applyPendingUninstalls(). +TEST_F(ExtensionManagerTest, ApplyPendingUninstallsRemovesMarkedDirectory) { + const QString ext_path = ext_dir_.path() + "/csv-loader"; + ASSERT_TRUE(QDir().mkpath(ext_path)); + QFile marker(ext_path + "/.pj_pending_uninstall"); + ASSERT_TRUE(marker.open(QIODevice::WriteOnly)); + marker.close(); + + mgr_->applyPendingUninstalls(); + + EXPECT_FALSE(QDir(ext_path).exists()); +} + +// A directory without the marker is left untouched. +TEST_F(ExtensionManagerTest, ApplyPendingUninstallsIgnoresUnmarkedDirectory) { + const QString ext_path = ext_dir_.path() + "/csv-loader"; + ASSERT_TRUE(QDir().mkpath(ext_path)); + + mgr_->applyPendingUninstalls(); + + EXPECT_TRUE(QDir(ext_path).exists()); +} + +// applyPendingUninstalls() is a no-op when extensions_dir contains no sub-directories. +TEST_F(ExtensionManagerTest, ApplyPendingUninstallsIsNoOpForEmptyDirectory) { + mgr_->applyPendingUninstalls(); // must not crash +} + // --------------------------------------------------------------------------- // [8] Platform detection // ---------------------------------------------------------------------------