feat(config): Allow Scoop to ignore running processes during reset/uninstall/update

[CrendKing]

Description

When any process of an application is still running, currently during uninstallation (uninstall or update), it unconditionally prints error message "Application is still running. Close all instances and try again" and exits. While this approach minimizes risk of data loss, manifest author has no control over the procedure.

Some apps consist of constantly running services and drivers, which has no GUI. In such case, user may have no idea how to "close all instances", and get stuck. Manifest author might be able to Stop-Service or call certain external command to stop those processes, but the current logic prevents this.

One simple way to fix this is adding a prompt during uninstallation, as this PR demonstrates. If Scoop detects process running, it calls Read-Host to ask user permission to proceed the "uninstaller" script, assuming which will take care of the process termination. In case user answers "N", there will be no change.

Currently there is no flag to guard this change, so all users will be immediately affected by this change. I have no previous experience on how Scoop usually handle these kind of changes. We could create a new property in manifest structure to activate this.

Furthermore, if this is approved, we could even always attempt to call Stop-Process -Name $bin after prompt, where "$bin" is the string provided in the manifest's "bin" property, if exist. This way we could streamline the app update even more.

Motivation and Context

Closes #4710

How Has This Been Tested?

1. Install an app.
2. Launch the app without terminating it.
3. scoop uninstall <app_name>.
4. Observe a prompt shows during uninstallation.
   4.1. Press any key other than "y" results in the "Application is still running. Close all instances and try again" message.
   4.2. Press "y", the uninstallation continues.
5. If manifest has uninstaller script that Stop-Process -Name <app_name>, app is closed and unininstalled successfully.
   5.1. If not, uninstallation fails with error "Couldn't remove <app_dir>; it may be in use."

Checklist:

• I have read the Contributing Guide.
• I have updated the documentation accordingly.
• I have updated the tests accordingly.

[tech189]

Just some suggestions for improving the wording, I haven't actually tested the functionality - I'll leave that to the maintainers.

[niheaven]

I don't like this idea, a conclusion notes after all uninstallation to remind user that some apps are still running and not uninstalled is more better, since this one will interrupt the unattended uninstalling process.

Or add a new config option to let scoop force exit all running processes and uninstall/update the apps.

[CrendKing]

I agree that this change is backward incompatible and breaks unattended process. As I mentioned in the opening, if the community would come up with a "force exit running process" option, I'm OK with that too. Maybe provide that as a scoop config option (i.e. when true, always force exit instead of always error and fail uninstall/reset).

@rashil2000, what do you think?

[rashil2000]

Hmm, the config option is a much better UX. It does not break any previous user workflows, and offers customizability without needing to ask the user for a prompt each time.

For implementation of the config option, you can refer to this PR - #4155

You'll also need to update the libexec/scoop-config.ps1 file, for documentation.

[CrendKing]

OK. Just to recap the new requirements:

1. Add a new boolean config to the scoop config command, which enables the "force kill processes" function. This defaults to false for backward compatibility.
2. For uninstall, update and reset, if Scoop detects any process running from the app directory, AND the config above is enabled by user, instead of print error message and exit, forcefully kill all running process by calling Stop-Process. Print relevant message for this action.
3. Afterwards, proceed the uninstall/update/reset procedure as usual.

Is this good?

EDIT: Since we are adding an option now, maybe we make the current "prompt" approach as one of the choices, so that people who don't care about unattended process but want to save progress before forceful process kill have a chance to do so? Instead of a yes/no question, we could make the prompt something like "Scoop is about to stop all running processes of $app_name. Save progress and press any key to continue..."

Basically a three-choice option: "stop_running_process" -> either "no" (the default), "silent" or "prompt".

[CrendKing]

Updated the PR. Moved the handling logic to core.ps1 for reuse between all the 3 use cases. If this function is better placed in a different file, please advise.

[rashil2000]

Do you think Scoop should stop the process if enabled? It will most likely fail (and print a wall of errors) - unless they are stopped in the proper order and with the proper flags (like -Force if necessary). I think we should just let the user stop the processes themselves.

The config option should be boolean - false (by default) to retain current behaviour, and true to just print the warning and proceed with uninstaller script.

[niheaven]

If this function is better placed in a different file, please advise.

For the code, put the function under install.ps1 IMO.

[CrendKing]

It will most likely fail (and print a wall of errors) - unless they are stopped in the proper order and with the proper flags (like -Force if necessary)

According to the PowerShell doc, without -Force the cmdlet doesn't stop process spawned from different user account. So I suppose since the function could be thrown with a global app, which could be started by other user, we should use -Force here. I'll update. EDIT: Done.

I tried several apps locally, and I don't get any problem. If you have see errors with this impl, could you provide the repro steps?

true to just print the warning and proceed with uninstaller script.

To clarify, I think we are discussing about two use cases:

1. Normal everyday app is running when doing update/uninstall, where a generic Stop-Process is sufficient to handle.
2. Special app (such as Everything driver/service from [Bug] Unable to run uninstaller script if any app is running #4710), which no GUI app is running. Stopping those processes require special tools/steps.

For 2, I completely agree we should leave to the uninstaller script.
For 1, if we just display a warning, since most manifest today does not have Stop-Process, it's not gonna change anything. Besides, what other special ways can the manifest authors do better than a generic Stop-Process?

For the code, put the function under install.ps1 IMO.

Done.

[rashil2000]

If you have see errors with this impl, could you provide the repro steps?

I've encountered problems outside of Scoop, where a generic Stop-Process isn't enough.

For 1, if we just display a warning, since most manifest today does not have Stop-Process, it's not gonna change anything.

That's preferable, to be honest. Scoop is a pretty non-invasive manager, and doing something outside of its scope will deviate from that. As I said, if any apps need special-casing, manifest authors will include a Stop-Process in their uninstaller scripts, and I think that's sufficient.

[CrendKing]

Understood. I'll remove the Stop-Process and update.

[CrendKing]

Manual test steps I did:

1. Install an app A, which has GUI. Config JSON is empty.
2. Launch A. Leave it running. Do scoop uninstall A.
3. Observe error message ERROR Application "A" is still running. Close all instances and try again. and Scoop terminates there.
4. Add config "ignoreRunning": false, and repeat steps 2-3. Observe exactly the same result.
5. Change ignoreRunning to true. Observe warn message WARN Application "A" is still running. Scoop is configured to ignore this condition. instead and other messages such as "Removing shim". Scoop stops with error ERROR Couldn't remove 'D:\Scoop\apps\A\0.0.1'; it may be in use. This is because A manifest does not handle running process in uninstaller.
6. Reinstall A. Add the following script to A\0.0.1\manifest.json:

    "uninstaller": {
        "script": "Stop-Process -Name A"
    },

7. Do uninstallation. Observe the warning message, then Running uninstaller script... , A's process gets killed, and eventually 'A' was uninstalled.

=====

Is there any test file need update?

[rashil2000]

Looks great. I'll do a proper review later today.

Hmm, I don't think there are tests for this. @niheaven can you confirm?

[rashil2000]

Vague idea: instead of a config option, how about we detect the presence of uninstaller field in the manifest, to automate this process? If uninstaller exists, Scoop will warn the user and proceed. If it doesn't, it pops up an error and stops.

[niheaven]

Nope, this process related to specific app installation/update cannot be tested. Just run .\bin\test.ps1 to see if all passes.

BTW, you should check config before running the new function to avoid Get-Process for every uninstallation/updating.

[CrendKing]

Vague idea: instead of a config option, how about we detect the presence of uninstaller field in the manifest, to automate this process? If uninstaller exists, Scoop will warn the user and proceed. If it doesn't, it pops up an error and stops.

IMO, the pro of the current impl is that it's explicit. User has to express the consent to make it work. What if some user super concern of losing unsaved file in their text editor? They will not have a choice.

If we rely on uninstaller entirely, at least we should update the wiki here so that future authors are aware of the implication.

you should check config before running the new function to avoid Get-Process for every uninstallation/updating.

Sorry, can you explain? I understand I can hoist the get_config out of the foreach loop. What's it about Get-Process? Don't I need to Get-Process for each different app anyway?

[niheaven]

Aha sorry, I misunderstood the logic of the function 🤣 Please ignore it.

[rashil2000]

What if some user super concern of losing unsaved file in their text editor? They will not have a choice.

Hmm, yeah makes sense. Let's leave it as it is.

[rashil2000]

We should call it ignore_running_processes to be more clear.

[rashil2000]

Please add an entry in the changelog

[niheaven]

Call the function ignore_running_process test_running_process and flip the output ($true for existed/$false for none) to make this more readable.~

[CrendKing]

Oh, you mean I should make the function "ignore_running_process", not "handle_running_processes"? What about the config? Any suggestion?

Or you can change the PR yourself, feel free to just modify it.

BTW, the current handle_running_processes() already returns $false if the caller should break, if I'm not mistaken.

Sorry for misunderstanding.

[rashil2000]

This should be feat instead of fix

[niheaven]

It should be test_running_process 🤣

Or you can change the PR yourself, feel free to just modify it.

Okay, I'll change it a little, feel free to reverse my commit if it got something wrong

[niheaven]

@CrendKing Done.

@rashil2000 Moved.

[niheaven]

@CrendKing If this is fine, I'll merge it.

[CrendKing]

Of course. Go ahead. Thank you guys!

[rashil2000]

Thanks for the contribution @CrendKing!

[CrendKing]

@niheaven I think e55639d introduced a bug. Previously either the error or warn message would print only if there is some running process. After that commit, the warn message would print as long as that config is set.

Should I make a PR to correct it or you can do it?

[niheaven]

It's been fixed in #4731, you could try it 🤣

[CrendKing]

That's exactly what I'm looking for. Sorry didn't notice that commit. Thank you!