From 3dab671fa7ed97b47d890939e68c50da8d1707c7 Mon Sep 17 00:00:00 2001 From: chrip Date: Wed, 26 Aug 2026 14:28:46 +0200 Subject: [PATCH] fix(updater): remove leftover installer during config version migration Application::configVersionMigration() clears the Updater/* keys on the first start of a newly installed version, before NSISUpdater::handleStartup() gets a chance to see them and clean up the downloaded installer. The installer and msi.log are then orphaned in the config folder with nothing left to find them by, repeating on every update. ConfigFile::cleanUpdaterConfiguration() now deletes the installer and msi.log before dropping the keys, so both the migration path and the updater's own cleanup remove the file. NSISUpdater::wipeUpdateData() delegates to it instead of duplicating the removal logic. Reported by @tgebler as still reproducing after #8980: https://github.com/nextcloud/desktop/pull/8980#issuecomment-5368390981 Closes #7009. Assisted-by: ClaudeCode:claude-sonnet-5 Signed-off-by: chrip --- src/gui/updater/ocupdater.cpp | 32 +++++------------------- src/libsync/configfile.cpp | 28 +++++++++++++++++++++ src/libsync/configfile.h | 2 ++ test/testupdater.cpp | 47 +++++++++++++++++++++++++++++++++++ 4 files changed, 83 insertions(+), 26 deletions(-) diff --git a/src/gui/updater/ocupdater.cpp b/src/gui/updater/ocupdater.cpp index 27507a7c24e85..918d7877cb47f 100644 --- a/src/gui/updater/ocupdater.cpp +++ b/src/gui/updater/ocupdater.cpp @@ -26,7 +26,6 @@ const auto updateAvailableC = QStringLiteral("Updater/updateAvailable"); const auto updateTargetVersionC = QStringLiteral("Updater/updateTargetVersion"); const auto updateTargetVersionStringC = QStringLiteral("Updater/updateTargetVersionString"); const auto autoUpdateAttemptedC = QStringLiteral("Updater/autoUpdateAttempted"); -const auto msiLogFileNameC = QStringLiteral("msi.log"); } UpdaterScheduler::UpdaterScheduler(QObject *parent) @@ -223,7 +222,7 @@ void OCUpdater::slotStartInstaller() return QDir::toNativeSeparators(path); }; - QString msiLogFile = cfg.configPath() + msiLogFileNameC; + QString msiLogFile = cfg.msiLogFilePath(); QString command = QStringLiteral("&{msiexec /i '%1' /L*V '%2'| Out-Null ; &'%3'}") .arg(preparePathForPowershell(updateFile)) .arg(preparePathForPowershell(msiLogFile)) @@ -305,30 +304,11 @@ void NSISUpdater::slotWriteFile() void NSISUpdater::wipeUpdateData() { - ConfigFile cfg; - QSettings settings(cfg.configFile(), QSettings::IniFormat); - QString updateFileName = settings.value(updateAvailableC).toString(); - if (!updateFileName.isEmpty()) { - if (QFile::remove(updateFileName)) { - qCInfo(lcUpdater) << "Removed updater file:" << updateFileName; - } else { - qCWarning(lcUpdater) << "Failed to remove updater file:" << updateFileName; - } - } - // Also try to remove the msi log file (created when running msiexec) - const auto msiLogFileName = QString{cfg.configPath() + msiLogFileNameC}; - if (QFile::exists(msiLogFileName)) { - if (QFile::remove(msiLogFileName)) { - qCInfo(lcUpdater) << "Removed msi log file:" << msiLogFileName; - } else { - qCWarning(lcUpdater) << "Failed to remove msi log file:" << msiLogFileName; - } - } - - settings.remove(updateAvailableC); - settings.remove(updateTargetVersionC); - settings.remove(updateTargetVersionStringC); - settings.remove(autoUpdateAttemptedC); + // Deliberately delegated: ConfigFile::cleanUpdaterConfiguration() is also + // reached from Application::configVersionMigration() on the first start of + // a newly installed version, and both paths must remove the installer, not + // just the keys that point at it. + ConfigFile().cleanUpdaterConfiguration(); } void NSISUpdater::slotDownloadFinished() diff --git a/src/libsync/configfile.cpp b/src/libsync/configfile.cpp index 920ff4aa9ef20..27cdbe6f6fd7f 100644 --- a/src/libsync/configfile.cpp +++ b/src/libsync/configfile.cpp @@ -434,10 +434,38 @@ QString ConfigFile::excludeFileFromSystem() return fi.absoluteFilePath(); } +namespace { +void removeUpdaterArtifact(const QString &path) +{ + if (path.isEmpty() || !QFile::exists(path)) { + return; + } + + if (QFile::remove(path)) { + qCInfo(lcConfigFile) << "Removed leftover updater file:" << path; + } else { + qCWarning(lcConfigFile) << "Failed to remove leftover updater file:" << path; + } +} +} + +QString ConfigFile::msiLogFilePath() const +{ + return configPath() + QStringLiteral("msi.log"); +} + void OCC::ConfigFile::cleanUpdaterConfiguration() { QSettings settings(configFile(), QSettings::IniFormat); settings.beginGroup("Updater"); + + // The config is the only record of where the downloaded installer lives. + // Delete it before dropping the keys, or every version change orphans + // another installer in the config folder with nothing left to find it by. + // See https://github.com/nextcloud/desktop/issues/7009 + removeUpdaterArtifact(settings.value("updateAvailable").toString()); + removeUpdaterArtifact(msiLogFilePath()); + settings.remove("autoUpdateAttempted"); settings.remove("updateTargetVersion"); settings.remove("updateTargetVersionString"); diff --git a/src/libsync/configfile.h b/src/libsync/configfile.h index 552273b2ef387..d6d44fafbea9d 100644 --- a/src/libsync/configfile.h +++ b/src/libsync/configfile.h @@ -42,6 +42,8 @@ class OWNCLOUDSYNC_EXPORT ConfigFile static QString excludeFileFromSystem(); // doesn't access config dir void cleanUpdaterConfiguration(); + /** Path of the log msiexec writes while installing an update (Windows). */ + [[nodiscard]] QString msiLogFilePath() const; void cleanupGlobalNetworkConfiguration(); /** diff --git a/test/testupdater.cpp b/test/testupdater.cpp index bf8968114675d..d91212328e81b 100644 --- a/test/testupdater.cpp +++ b/test/testupdater.cpp @@ -51,6 +51,53 @@ private Q_SLOTS: QVERIFY(currVersion < highVersion); } + // Reproduces #7009: on the first start of a newly installed version the + // leftover installer must be gone. Application::configVersionMigration() + // (src/gui/application.cpp:161-164) drops the Updater/* keys before + // main() calls handleStartup() (src/gui/main.cpp:104 vs 142), so the + // cleanup in NSISUpdater::handleStartup() never sees the artifact. + void testLeftoverInstallerIsRemovedAfterVersionChange() + { + QTemporaryDir tempDir; + QVERIFY(tempDir.isValid()); + QVERIFY(ConfigFile::setConfDir(tempDir.path())); + + ConfigFile cfg; + const auto writeFile = [](const QString &path, const QByteArray &contents) { + QFile f(path); + return f.open(QIODevice::WriteOnly) && f.write(contents) == contents.size(); + }; + + // a previous run downloaded an installer and msiexec left its log behind + const auto installer = FileSystem::joinPath(cfg.configPath(), "Nextcloud-1.0.0-x64.msi"_L1); + const auto msiLog = FileSystem::joinPath(cfg.configPath(), "msi.log"_L1); + QVERIFY(writeFile(installer, "this would be the installer"_ba)); + QVERIFY(writeFile(msiLog, "this would be the msiexec log"_ba)); + + { + QSettings settings(cfg.configFile(), QSettings::IniFormat); + settings.setValue("Updater/updateAvailable"_L1, installer); + settings.setValue("Updater/updateTargetVersion"_L1, "1.0.0"_L1); + settings.setValue("Updater/updateTargetVersionString"_L1, "1.0.0"_L1); + settings.setValue("Updater/autoUpdateAttempted"_L1, true); + settings.sync(); + } + + // the update was installed: the running binary is newer than the target + QVERIFY(Updater::Helper::currentVersionToInt() > Updater::Helper::stringVersionToInt("1.0.0"_L1)); + + // what Application::configVersionMigration() does on that very first + // start, before the updater gets a chance to look at its own state + cfg.setClientVersionString("1.0.0"_L1); + cfg.cleanUpdaterConfiguration(); + + NSISUpdater updater(QUrl("http://localhost:1/updateinfo.xml"_L1)); + updater.handleStartup(); + + QVERIFY(!QFileInfo::exists(installer)); + QVERIFY(!QFileInfo::exists(msiLog)); + } + #ifdef HAVE_QHTTPSERVER void testUpdaterDownloadRedirect() {