1140130 - [mozinstall] Keyboard interrupt during installation should cause already installed files to be removed

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Hit this by accident today. When a keyboard interrupt is raised by e.g. pressing ctrl-c during an installation, the exception is not handled and the already copied files remain on disk.

Given that any aborted installation will be broken, mozinstall should remove the already installed files.

Not sure how the Windows Installer is handling that. It might do the cleanup itself.

Comment 1

[Julien Pagès (:parkouss)]

Attachment: Exception during installation should cause already installed files to be removed

It pass the unit tests with success locally.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8576623 [details] [diff] [review]
Exception during installation should cause already installed files to be removed

Review of attachment 8576623 [details] [diff] [review]:
-----------------------------------------------------------------

Julian, thanks for the patch. Please see my comment what may need an update. Also I would prefer if you could add an additional person for review. I would like to have a second pair of eyes on it. Maybe you can ask Andrew.

::: testing/mozbase/mozinstall/mozinstall/mozinstall.py
@@ +127,5 @@
> +            # try to uninstall this properly
> +            uninstall(dest)
> +        except:
> +            # uninstall may fail, so let's just clean the folder in this case
> +            mozfile.remove(dest)

I think that we should better use the already existent uninstall() method, and not re-implement everything here.

Comment 3

[Julien Pagès (:parkouss)]

Comment on attachment 8576623 [details] [diff] [review]
Exception during installation should cause already installed files to be removed

So as discussed on irc, the call to uninstall is done. (this is also visible in the last comment). The thing is that looking at the uninstall code, it can fail and not remove the install dir in that case.

So the logic is:

1. call uninstall
2. if that fails, call mozfile.remove

:ahal, what do you think of this patch ?

Comment 4

[Andrew Halberstadt [:ahal]]

Comment on attachment 8576623 [details] [diff] [review]
Exception during installation should cause already installed files to be removed

Review of attachment 8576623 [details] [diff] [review]:
-----------------------------------------------------------------

I see we already call mozfile.remove(dest) in the uninstall method, so this patch isn't the problem, but if dest already exists this could cause data loss! I think we should only remove dest if we created it in the first place here:
https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozinstall/mozinstall/mozinstall.py#108

If it already existed, we should either just leave it be, or at least be semi-smart and only remove files we think we created. We could even get a list of files before and after, and only remove the difference. It's a little unfair to r-, but I think the potential for data loss is serious enough that we should fix it at the same time.

Comment 5

[Julien Pagès (:parkouss)]

Ok I undesrtand the point. Reading at the install dest documentation, I thought that
it was OK to remove it, because there is a warning:

:param dest: Path to install to (to ensure we do not overwrite any existent
                  files the folder should not exist yet)

And I added in this documentation:

+    Note that in case of an exception during install, *dest* will be removed.

But yes, this is quite a possible loss if the function is already used
somewhere with a non-empty dest dir (and that the files in there are important).

I like the idea of uninstalling if we had an empty dest dir, and I can adapt
the patch for it. I wonder if supporting non empty dest dir is a good thing
at all (and if that could be changed ?).

Comment 6

[Andrew Halberstadt [:ahal]]

Yeah, I saw that message too, but I doubt most people will read the docs until it is too late. This is just one angry "I just deleted my homedir!" away from being our top priority.

Another alternative is to create a new folder in 'dest' and put everything in there, e.g:
path/to/dest/mozinstall-<app>

Actually, probably the best solution is to create a manifest of the files and only uninstall the files found in that manifest, e.g:
path/to/dest/.mozinstall

That way we can be sure never to lose data, even if the user calls "uninstall" at a later time. I guess these solutions are getting more and more complicated. For now I'd just focus on the case where install fails, set a flag if we created 'dest' and don't delete it if we didn't.

I'll file a separate bug for the manifest idea.

Comment 7

[Andrew Halberstadt [:ahal]]

Filed bug 1164443 for fixing the general uninstall case.

Comment 8

[Julien Pagès (:parkouss)]

Attachment: Exception during installation should cause already installed files to be removed

Ok,new attempt here - the dest dir is only removed if we created it.

Comment 9

[Andrew Halberstadt [:ahal]]

Comment on attachment 8606393 [details] [diff] [review]
Exception during installation should cause already installed files to be removed

Review of attachment 8606393 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, looks great! This can potentially leave files behind, but imo that's better than potentially deleting unrelated files. I don't think this will have much of an impact on our systems.

Comment 10

[Julien Pagès (:parkouss)]

Simple push to try here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e714c500f47

Comment 11

[Julien Pagès (:parkouss)]

Seems all good. :)

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7a14d34d112

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a7a14d34d112