NSIS install fails on terminal servers with multiple users already running the program

[TDola]

22.11.7

Which version of node are you using?
14.17.1

Which version of electron are you using?
8.5.5

Which version of electron-updater are you using (if applicable)?
4.3.9

For which target are you building for?

"win": {
            "icon": "resources/icons/win/icon.ico",
            "target": {
                "target": "nsis-web",
                "arch": [
                    "ia32",
                    "x64"
                ]
            },
            "artifactName": "${name}_setup_${version}.${ext}"
        },
        "nsisWeb": {
            "createDesktopShortcut": false,
            "createStartMenuShortcut": true,
            "menuCategory": true,
            "guid": "com.update-test",
            "perMachine": false,
            "allowElevation": true,
            "differentialPackage": true,
            "uninstallDisplayName": "${productName}",
            "runAfterFinish": true,
            "oneClick": true,
            "allowToChangeInstallationDirectory": false
        },

The issue I am having is the NSIS installer incorrectly determines the application is still running on terminal servers if any user has it open, even when using per user. This blocks the install most of the time, or worse yet, auto kills all instances on the terminal server for all users when one user wants to update their individual install. Both of these are serious bugs. The installer should not check processes for other users when it is installed to one user folder. And it absolutely should not be able to kill the process for other users.

To reproduce. Build an NSIS-web installer.
Connect to a terminal server with 2 different users. Install and run the application one one user. Then try to install on the other, it will fail.

On the right is the application running on a terminal server user. On the left is the installer trying to run on the same server but a different user

Is there a way to disable to is running check with a custom nsis script or anything? We encountered this bug during a large deployment which we have now paused.

[mmaietta]

Hmmm, you have multiple users accessing the same machine all using their own per-user installation of the same Electron app?
Can you try v22.11.5 and/or v22.10.5? Curious if this is a recent regression or has always been a long-standing issue.

It looks like you can actually use your own nsis macro customCheckAppRunning for detecting whether the app is running, albeit not documented I guess. Just need to provide it via https://www.electron.build/configuration/nsis#custom-nsis-script

electron-builder/packages/app-builder-lib/templates/nsis/include/allowOnlyOneInstallerInstance.nsh

Lines 30 to 36 in d307c21

	!macro CHECK_APP_RUNNING
	!ifmacrodef customCheckAppRunning
	!insertmacro customCheckAppRunning
	!else
	!insertmacro _CHECK_APP_RUNNING
	!endif
	!macroend

[TDola]

Same result on 22.10.5

I don't know much about nsis scripts :(.
Maybe electron-builder/packages/app-builder-lib/templates/nsis/include/getProcessInfo.nsh is returning too much and not restricted to the logged in user?

As a quick and dirty, this in installer.nsh seems to have fixed this issue

!macro customCheckAppRunning
    DetailPrint "User Mode Installer"
!macroend

I don't know what other issues it will cause. Will this cause any issues?

[mmaietta]

I'm not too familiar with nsis scripting either. Just picking it up while I go tbh.

I don't foresee that causing any issues, but then again, I also wasn't expecting this use case where multiple users are concurrently using different instances of the app on the same terminal server.
I'd give it a go though and see if it suits your purposes.

[TDola]

I'm not too familiar with nsis scripting either. Just picking it up while I go tbh.

I don't foresee that causing any issues, but then again, I also wasn't expecting this use case where multiple users are concurrently using different instances of the app on the same terminal server.
I'd give it a go though and see if it suits your purposes.

Well it change made everyone happy again so I guess that worked. Should I leave this issue open because the solution was just bypassing electron-builders nsis script and it should still be fixed?

[I-Otsuki]

I have seen similar problem but with different message on similar setup (terminal service environment).
What I see is a message added in #5902 (appCannotBeClosed).

I guess Windows allows non-previleged process seeing image names of processes in other users' sessions, but not seeing image paths (nor killing them).

Maybe we should not test if the app is running but test if the app executable is locked (opened), to evaluate the possibility of install/update?

[TDola]

Should be possible to check if the app is running and what user is running it. Only close the current users versions.

Something like this powershell command gives the user ID with the app

Get-WmiObject -Class Win32_Process | 
    Select-Object Name,Path, @{ 
        Name = 'Owner'
        Expression = { 
            $_.GetOwner().User 
        }
    }

[I-Otsuki]

Definitely. But not with nsProcess::_FindProcess...

[TDola]

Had to google it but if this is the plugin electron used https://nsis.sourceforge.io/NsProcess_plugin then it doesn't really work on terminal servers because it finds processes from all users.

And not much found for alternate solutions either. Would have to build something custom. Although just removing it and replacing with powershell scripts looks possible. According to the commit you referenced, it's using taskkill by the name anyways. If your just running command line, might as well go all the way.
I don't really know anyting about these NSIS scripts though. I am just sort of guessing based on what I see.

An example

Get-Process | ? {$_.SI -eq (Get-Process -PID $PID).SessionId} | ? {$_.ProcessName -eq "electron"}

Change electron to the app name. Kill the PIDs that pop up in this list which is filtered to the current user session

[Simolation]

I also have this problem with a larger deployment, where the initial installation as well as later updates fail, as long as other instances run on the same terminal server under a different user.
I don't really know anything about NSIS, but is disabling "customCheckAppRunning" not checking for a running instance at all? So, the current running instance would probably not be closed, or am I wrong?

[TDola]

@Simolation Sort of yeah. As long as in app you still run the quit and update function it still works fine.
It's working for me now. I have tested what happens if you leave it open, and it just errors trying to install. Not a great solution but if your head is under the gun too, it might be the best solution for now until the electron-builder team fixes the issue.

[Simolation]

I found now more or less a solution which is better than just skipping everything.

What I did is to take the script electron-builder uses normally, and I just removed the message boxes. So, the installer tries closing the application and when it does not work, it just installs it anyway. It is skipped, but it also tries to close it at least. This should prevent problems when the application is actually running.

I took this as a base:

electron-builder/packages/app-builder-lib/templates/nsis/include/allowOnlyOneInstallerInstance.nsh

Lines 39 to 96 in 41a59f9

	${GetProcessInfo} 0 $pid $1 $2 $3 $4
	${if} $3 != "${APP_EXECUTABLE_FILENAME}"
	${if} ${isUpdated}
	# allow app to exit without explicit kill
	Sleep 300
	${endIf}
	${nsProcess::FindProcess} "${APP_EXECUTABLE_FILENAME}" $R0
	${if} $R0 == 0
	${if} ${isUpdated}
	# allow app to exit without explicit kill
	Sleep 1000
	Goto doStopProcess
	${endIf}
	MessageBox MB_OKCANCEL|MB_ICONEXCLAMATION "$(appRunning)" /SD IDOK IDOK doStopProcess
	Quit
	doStopProcess:
	DetailPrint `Closing running "${PRODUCT_NAME}"...`
	# https://github.com/electron-userland/electron-builder/issues/2516#issuecomment-372009092
	nsExec::Exec `taskkill /im "${APP_EXECUTABLE_FILENAME}" /fi "PID ne $pid"` $R0
	# to ensure that files are not "in-use"
	Sleep 300
	# Retry counter
	StrCpy $R1 0
	loop:
	IntOp $R1 $R1 + 1
	${nsProcess::FindProcess} "${APP_EXECUTABLE_FILENAME}" $R0
	${if} $R0 == 0
	# wait to give a chance to exit gracefully
	Sleep 1000
	nsExec::Exec `taskkill /f /im "${APP_EXECUTABLE_FILENAME}" /fi "PID ne $pid"` $R0
	${If} $R0 != 0
	DetailPrint `Waiting for "${PRODUCT_NAME}" to close (taskkill exit code $R0).`
	Sleep 2000
	${else}
	Goto not_running
	${endIf}
	${else}
	Goto not_running
	${endIf}
	# App likely running with elevated permissions.
	# Ask user to close it manually
	${if} $R1 > 1
	MessageBox MB_RETRYCANCEL|MB_ICONEXCLAMATION "$(appCannotBeClosed)" /SD IDCANCEL IDRETRY loop
	Quit
	${else}
	Goto loop
	${endIf}
	not_running:
	${endIf}
	${endIf}

Here is the modified code of my installer.nsh:

!include "getProcessInfo.nsh"
Var pid

!macro customCheckAppRunning
  ${GetProcessInfo} 0 $pid $1 $2 $3 $4
    ${if} $3 != "${APP_EXECUTABLE_FILENAME}"
      ${if} ${isUpdated}
      # allow app to exit without explicit kill
      Sleep 300
      ${endIf}

      ${nsProcess::FindProcess} "${APP_EXECUTABLE_FILENAME}" $R0
      ${if} $R0 == 0
      ${if} ${isUpdated}
        # allow app to exit without explicit kill
        Sleep 1000
        Goto doStopProcess
      ${endIf}
      # Skip message box
      # MessageBox MB_OKCANCEL|MB_ICONEXCLAMATION "$(appRunning)" /SD IDOK IDOK doStopProcess
      # Quit

      doStopProcess:

      DetailPrint `Closing running "${PRODUCT_NAME}"...`

      # https://github.com/electron-userland/electron-builder/issues/2516#issuecomment-372009092
      nsExec::Exec `taskkill /im "${APP_EXECUTABLE_FILENAME}" /fi "PID ne $pid"` $R0
      # to ensure that files are not "in-use"
      Sleep 300

      # Retry counter
      StrCpy $R1 0

      loop:
        IntOp $R1 $R1 + 1

        ${nsProcess::FindProcess} "${APP_EXECUTABLE_FILENAME}" $R0
        ${if} $R0 == 0
        # wait to give a chance to exit gracefully
        Sleep 1000
        nsExec::Exec `taskkill /f /im "${APP_EXECUTABLE_FILENAME}" /fi "PID ne $pid"` $R0
        ${If} $R0 != 0
          DetailPrint `Waiting for "${PRODUCT_NAME}" to close (taskkill exit code $R0).`
          Sleep 2000
        ${endIf}
        ${else}
        Goto not_running
        ${endIf}

        # App likely running with elevated permissions.
        # Ask user to close it manually
        ${if} $R1 > 1
        # Just skip the message box and continue installing
        Goto not_running
        ${else}
        Goto loop
        ${endIf}
      not_running:
      ${endIf}
    ${endIf}
!macroend

I haven't tested it on a terminal server yet, this is going to happen on Monday 🤞

[TDola]

I tried something similar, but found it did in fact close the process randomly. Roughly half the tests I ran it successfully closed, other times it complained it failed to close it. But that could be an environment problem here.

[Simolation]

So far, everything has worked perfectly. I tried it on two different terminal server configurations, and it worked for ~25 devices without any issues.

[I-Otsuki]

Thinking of #5902, simply skipping dialog seems not the best way, as that would leave elevated process running and fail to install.

Elevated instances owned by current user are something current user should close. So I think @TDola's pwsh solution (or maybe tasklist /fi "USERNAME eq %USERNAME%" /fi "IMAGENAM eq ${APP_EXECUTABLE_FILENAME}" if you prefeer aligning with taskkill) is better.

Or, maybe just making MessageBox ignorable could be enough? So we leave them to users and forget about machine-wide install, elevated processes, terminal servers, processes owned by other users with runas...

Then these lines

electron-builder/packages/app-builder-lib/templates/nsis/include/allowOnlyOneInstallerInstance.nsh

Line 89 in f5c1e00

MessageBox MB_RETRYCANCEL|MB_ICONEXCLAMATION "$(appCannotBeClosed)" /SD IDCANCEL IDRETRY loop

should look like:

          MessageBox MB_ABORTRETRYIGNORE|MB_ICONEXCLAMATION "$(appCannotBeClosed)" /SD IDABORT IDRETRY loop IDIGNORE not_running

[TDola]

I can't really trust users not to click a button that will shut down the application for everyone else 😞
I think part of the problem I am having is users are admin on their own terminal sessions because of some really.... really old legacy programs. Like Windows 95 era. So they all accidentally have permissions to kill the process for everyone else.

[I-Otsuki]

Oh no. I misunderstood the problem.
The message box I mentioned above is shown after you click [OK] on your message box.
It's shown after it fails to nuke processes for everyone, As I'm not admin on the TS.
I'm really sorry for confusing and messing up.

Anyways, both of our situations should be resolved with replacing ${nsProcess::FindProcess} with one checking the owner of process.

Can you try replacing allowOnlyOneInstallerInstance.nsh with this?
https://github.com/I-Otsuki/electron-builder/blob/ccaed1de6c111338c726725e9d63a448897eeda7/packages/app-builder-lib/templates/nsis/include/allowOnlyOneInstallerInstance.nsh

[I-Otsuki]

Um, no, this isn't enough. If current user have the app running, this will kill other users instances too.
On per-user install, I have to add /FI "USERNAME eq %USERNAME%" to taskkill too. So it won't kill everyone's app.
I'll add some lines tomorrow.

[TDola]

That sounds perfect. I look forward to testing it

[I-Otsuki]

Hope this works.
https://github.com/I-Otsuki/electron-builder/blob/c4691563b6dfd98125265b71bdd2f921a2706012/packages/app-builder-lib/templates/nsis/include/allowOnlyOneInstallerInstance.nsh

[mmaietta]

Would love to have a PR submitted for this if possible! :)

[I-Otsuki]

Seems not possible yet.
While testing, I found that elevated instance of current user is detected, but not closed while non-elevated installer continues.
In this case, the installer should show message box asking user to close it manually.

The problem is that, taskkill returns 0 even it successfully killed only gpu process (1 of 4 elevated process).
Relying on such value is not a good idea.
https://github.com/I-Otsuki/electron-builder/blob/c4691563b6dfd98125265b71bdd2f921a2706012/packages/app-builder-lib/templates/nsis/include/allowOnlyOneInstallerInstance.nsh#L89-L101

I'll try modifying the code again to see tasklist instead and see if that works.,,

[I-Otsuki]

elevated instance of current user is detected, but not closed while non-elevated installer continues.

Found that said behaviour has been there from before I made changes.

Anyway, this could be a cause of what @TDola reported, I mean, random closing of processes.
I fixed this and opened a PR #6472

[et-hh]

elevated instance of current user is detected, but not closed while non-elevated installer continues.

Found that said behaviour has been there from before I made changes.

Anyway, this could be a cause of what @TDola reported, I mean, random closing of processes. I fixed this and opened a PR #6472

i still have this problem...., the code is right, with the PR code