Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] Uninstallation should be reverted if subsequent installation in same changeset fails #3604

Closed
Ictiv opened this issue Jul 17, 2022 · 5 comments · Fixed by #3635
Closed
Labels
GUI Issues affecting the interactive GUI

Comments

@Ictiv
Copy link

Ictiv commented Jul 17, 2022

Background

  • Operating System: Windows 10
  • CKAN Version: 1.31.0
  • KSP Version: 1.12.3.3173

Have you made any manual changes to your GameData folder (i.e., not via CKAN)?
Yes, however not to any of the mods involved.

Problem

Describe the bug
Steps to reproduce
(Combining these two as it frankly makes more sense in this case.)
I had thought to replace ScienceAlert Relarted with [x]Science!, and while at it, try out Parallax.
CKAN correctly uninstalled Science Alert Realerted, then proceeded to the download step of the installation half of my changes.
Due to dodgy internet, the sizable download of Parallax got interrupted, resulting in a download error.
Upon choosing "Retry", CKAN alerted me that ScienceAlert Realerted isn't installed, no doubt since it was still trying to use the same changelist as when I started out, when I asked it to uninstall the mod.

Expected behavior
Either CKAN undoing the entire process and restoring the pre-uninstallation state, or not worrying about the uninstallation issue, either because it's unimportant, or due to the program handling uninstallations and installations as two entirely separate processes with their independent checklists. (If such a thing happens to someone who's assembling a large list of changes, it could be quite annoying, as backing out of the error appears to have made CKAN clear the change list.)

Screenshots (if applicable)
CKAN error code (if applicable):
Sadly, while switching back to the main tab to double check which folders ScienceAlert touches so I can be sure it was indeed uninstalled fully, I lost the tab with the error so I can't share it. However, I expect "[X Mod] isn't installed" to identify it well enough.

PLACE CODE HERE !

Overall, just a minor inconvenience for me, but it might be worth taking a look at it to avoid others' headaches. Thank you for your great work!

@HebaruSan HebaruSan transferred this issue from KSP-CKAN/NetKAN Jul 17, 2022
@HebaruSan HebaruSan added the GUI Issues affecting the interactive GUI label Jul 17, 2022
@HebaruSan
Copy link
Member

HebaruSan commented Jul 17, 2022

I have some changes in progress to improve installation error handling related to #3588, and this could make sense to address there. Probably each full changeset should be atomic, so a download failure should revert any previous uninstallations.

@HebaruSan HebaruSan changed the title [Bug] Failure to log mod removal during application of changes in case of error [Bug] Uninstallation should be reverted if subsequent installation in same changeset fails Jul 17, 2022
@HebaruSan
Copy link
Member

HebaruSan commented Aug 4, 2022

But then there's #873, which wants almost the opposite recovery strategy: if a download fails, try to process as much of the rest of the changeset as can be done without leaving dependencies unsatisifed.

Both requests are understandable. Maybe we could prompt the user?

+---------------------------------------------------+
|                 Download Failed                   |
+---------------------------------------------------+
| The download of Mod 1.0 has failed.               |
| What should we do?                                |
|                                                   |
| [Retry download]                                  |
| [Abort whole changeset]                           |
| [Skip Mod 1.0 and proceed with rest of changeset] |
+---------------------------------------------------+

This would relieve us of the burden of choosing whether to prioritize changeset atomicity (when things will break if we don't complete all the tasks) or partial recovery (when we just want as many tasks done as possible, even if others fail) at development time.

@Ictiv
Copy link
Author

Ictiv commented Aug 4, 2022

This pop-up option would be ideal, I think, however as it might take time to implement, I'd recommend either starting with Abort or Skip option as the only possibility, assuming there's an intermediary release before full implementation of the pop-up.
Retrying in general would be best, but it could also run into other issues if download becomes impossible for whatever reason. Because of that, I don't think the Retry option should exist, unless the pop-up system is implemented, and subsequent retries can be aborted through it.

@HebaruSan
Copy link
Member

Looking at this now, rolling back the uninstall is suprisingly easy, we just need to add a transaction around the whole uninstall/install/upgrade block:

while (!resolvedAllProvidedMods)
{
try
{
e.Result = new KeyValuePair<bool, ModChanges>(false, opts.Key);
if (toUninstall.Count > 0)
{
processSuccessful = false;
if (!installCanceled)
{
installer.UninstallList(toUninstall.Select(m => m.identifier),
ref possibleConfigOnlyDirs, registry_manager, false, toInstall);
toUninstall.Clear();
processSuccessful = true;
}
}
if (toInstall.Count > 0)
{
processSuccessful = false;
if (!installCanceled)
{
installer.InstallList(toInstall, opts.Value, registry_manager, ref possibleConfigOnlyDirs, downloader, false);
toInstall.Clear();
processSuccessful = true;
}
}
if (toUpgrade.Count > 0)
{
processSuccessful = false;
if (!installCanceled)
{
installer.Upgrade(toUpgrade, downloader, ref possibleConfigOnlyDirs, registry_manager, true, true, false);
toUpgrade.Clear();
processSuccessful = true;
}
}
HandlePossibleConfigOnlyDirs(registry, possibleConfigOnlyDirs);
e.Result = new KeyValuePair<bool, ModChanges>(processSuccessful, opts.Key);
if (installCanceled)
{
return;
}
resolvedAllProvidedMods = true;
}
catch (TooManyModsProvideKraken k)
{
// Prompt user to choose which mod to use
tabController.ShowTab("ChooseProvidedModsTabPage", 3);
ChooseProvidedMods.LoadProviders(k.Message, k.modules, Manager.Cache);
tabController.SetTabLock(true);
CkanModule chosen = ChooseProvidedMods.Wait();
// Close the selection prompt
tabController.SetTabLock(false);
tabController.HideTab("ChooseProvidedModsTabPage");
if (chosen != null)
{
// User picked a mod, queue it up for installation
toInstall.Add(chosen);
// DON'T return so we can loop around and try the above InstallList call again
tabController.ShowTab("WaitTabPage");
}
else
{
// User cancelled, get out
tabController.ShowTab("ManageModsTabPage");
Util.Invoke(this, () => Enabled = true);
Util.Invoke(menuStrip1, () => menuStrip1.Enabled = true);
e.Result = new KeyValuePair<bool, ModChanges>(false, opts.Key);
return;
}
}
}

Since ModuleInstaller already uses transaction operations for everything it does, this means that even cancelling out of selecting a virtual dependency brings back anything previously removed in that pass, both on disk and in the registry.

@HebaruSan
Copy link
Member

HebaruSan commented Aug 16, 2022

+---------------------------------------------------+
|                 Download Failed                   |
+---------------------------------------------------+
| The download of Mod 1.0 has failed.               |
| What should we do?                                |
|                                                   |
| [Retry download]                                  |
| [Abort whole changeset]                           |
| [Skip Mod 1.0 and proceed with rest of changeset] |
+---------------------------------------------------+

This UI needs to be more significantly complicated; if the user doesn't choose abort, then they should get to choose between retry and skip for each mod that failed, which could be several. At the moment I'm thinking about a table listing each failed mod with checkbox columns at the left, which I don't really like much:

+--------------------------------------------------------+
|                   Download Failed                      |
+--------------------------------------------------------+
| The following mods failed to download:                 |
|                                                        |
|  +--------+-------+-----------------+---------------+  |
|  | Retry? | Skip? | Mod             | Error         |  |
|  +--------+-------+-----------------+---------------+  |
|  |   X    |       | CoolMod 1.0     | Timed out     |  |
|  |   X    |       | AwesomeMod 1.0  | 404 Not Found |  |
|  |        |   X   | MediocreMod 1.0 | 403 Forbidden |  |
|  +--------+-------+-----------------+---------------+  |
|                                                        |
| What should we do?                                     |
|                                                        |
| [Retry without "Skip" mods]                            |
| [Abort whole changeset]                                |
+--------------------------------------------------------+

... but that's the structure of the decisions to be made. The default choice should probably be retry, since otherwise there's a chance of removing a mod from the changeset by accident.

There's also the question of how to alert the user that skipping a mod will also skip any mods that depend on it. A "Dependencies" column might be servicable, but at that point the grid is getting pretty unwieldy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GUI Issues affecting the interactive GUI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants