fix(updater): remove leftover installer during config version migration - #10680
Open
chrip wants to merge 1 commit into
Open
fix(updater): remove leftover installer during config version migration#10680chrip wants to merge 1 commit into
chrip wants to merge 1 commit into
Conversation
chrip
requested review from
camilasan,
claucambra,
i2h3,
mgallien and
nilsding
as code owners
August 26, 2026 13:59
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 nextcloud#8980: nextcloud#8980 (comment) Closes nextcloud#7009. Assisted-by: ClaudeCode:claude-sonnet-5 Signed-off-by: chrip <christoph.schaefer@nextcloud.com>
mgallien
force-pushed
the
fix/7009-installer-cleanup-on-migration
branch
from
August 28, 2026 11:21
4253651 to
3dab671
Compare
Contributor
|
Artifact containing the AppImage: nextcloud-appimage-pr-10680.zip Digest: To test this change/fix you can download the above artifact file, unzip it, and run it. Please make sure to quit your existing Nextcloud app and backup your data. |
mgallien
requested changes
Aug 28, 2026
mgallien
left a comment
Collaborator
There was a problem hiding this comment.
thanks
one small change only
| }; | ||
|
|
||
| QString msiLogFile = cfg.configPath() + msiLogFileNameC; | ||
| QString msiLogFile = cfg.msiLogFilePath(); |
Collaborator
There was a problem hiding this comment.
Suggested change
| QString msiLogFile = cfg.msiLogFilePath(); | |
| const auto msiLogFile = cfg.msiLogFilePath(); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves
#7009
Summary
Every update leaves its installer (~200 MB) and
msi.logbehind in%APPDATA%\Nextcloud, one file per release.#8980 did not take effect in practice. On the first start of a newly installed version,
Application::configVersionMigration()callsConfigFile::cleanUpdaterConfiguration()(application.cpp:161-164), which dropsUpdater/updateAvailable— the only record of where the installer is — beforemain()reachesNSISUpdater::handleStartup()(main.cpp:104vs:142). BothwipeUpdateData()call sites then sit behind a guard that can no longer be true, and the file is orphaned with nothing left to find it by.The trigger is the version number in
nextcloud.cfgversus the one compiled into the running binary. They differ on every real update, and are identical when the same version is reinstalled — which is why this passes local testing, and why CI cannot express the case at all (one binary, oneMIRALL_VERSION_STRING). It regressed in v3.16.0 with b3886ed (#7807); before that the main flow cleaned up correctly and only unusual paths leaked.ConfigFile::cleanUpdaterConfiguration()now deletes the installer andmsi.logbefore dropping the keys, andNSISUpdater::wipeUpdateData()delegates to it instead of duplicating the same four key removals. Startup ordering is untouched, so #7807's intent is preserved.Deliberately not included: installers that have already accumulated (no recorded path remains, so they need a directory sweep) and the download location itself.
Tests
testLeftoverInstallerIsRemovedAfterVersionChangefails on master and passes with the fix.nextcloud.exeVersion changed…logged, no removal, both files surviveRemoved leftover updater file:lines, both files gonectestctestAll seven paths the updater can take were driven against the real binary — the five dialog outcomes ("New update ready" → Cancel / OK; "Update Failed" → Ask again later / Restart and update / Update manually) plus the two non-interactive ones (version change, external install).
Deletion happens only in the two cases where the update is genuinely finished, and in none of the five where a decision is still pending — a pending installer is never removed.
Built and verified on Windows (Craft/MSVC 2022) and Linux (Qt 6.10.2). Not tested: the locked-file case (
QFile::removefailing under an antivirus or open handle), which logs a warning and leaves the file.Checklist
AI (if applicable)