Relaunch UniGetUI through /bin/sh instead of powershell.exe off Windows - #5418
awss (awss1i) wants to merge 2 commits into
Conversation
CoreTools.ScheduleRelaunchAfterExit started "powershell.exe -Command Wait-Process ...; Start-Process ..." on every platform. On macOS and Linux there is no powershell.exe, so accepting a restart (after a dependency install, from settings, or from the portable-import banner) threw a Win32Exception out of the shutdown coordinator: the app went away and never came back, and the exception was reported on the next start. Split the start-info construction into CreateRelaunchStartInfo. Windows keeps the same PowerShell command. Elsewhere a detached /bin/sh helper waits for the current pid with kill -0 (0.2 s polls, 30 s cap) and then starts the executable, or opens the .app bundle with "open -na" when the executable lives inside one on macOS. This is the shape the self-updater already uses for its post-exit swap; arguments are positional so no path is interpolated into the script. Tests pin the Windows command, the helper's arguments and script, the bundle detection, and run the helper for real against a stand-in process on Linux and macOS. Relates to Devolutions#5237
Gabriel Dufresne (GabrielDuf)
left a comment
Gabriel Dufresne (GabrielDuf)
left a comment
There was a problem hiding this comment.
I left four comments. Could you take a look when you have a chance?
| $"Wait-Process -Id {currentProcessId}; Start-Process -FilePath '{escapedExecutablePath}'"; | ||
| ProcessStartInfo startInfo = CreateRelaunchStartInfo(Environment.ProcessId, executablePath); | ||
| Logger.Debug($"Scheduling a relaunch of {executablePath} through {startInfo.FileName}"); | ||
| using var process = Process.Start(startInfo); |
There was a problem hiding this comment.
Process.Start is still unguarded here, so the failure mode this PR fixes can still occur.
ScheduleRelaunchAfterExit is passed as onAuthorized into ApplicationShutdownCoordinator.RequestAsync, which runs it inside try { onAuthorized?.Invoke(); } catch { Interlocked.Exchange(ref _isQuitting, 0); throw; }, and it is reached from AppRestartHelper.Restart() => _ = RestartAsync() — fire-and-forget.
If Process.Start throws for any reason other than "powershell.exe is missing" (no /bin/sh in a minimal container, a sandbox/posix_spawn denial, fork failure under resource pressure), the exception rolls _isQuitting back and propagates into an unobserved Task: the app neither restarts nor quits, and the exception surfaces on the next start — exactly the symptom the commit message describes.
The sibling helper AvaloniaAutoUpdater.TrySpawnSwapHelper already wraps the same Process.Start in try/catch and returns false. Worth doing the same here and reporting back to the caller, which can then keep the window open instead of silently disappearing.
There was a problem hiding this comment.
Addressed in 1342c47. ScheduleRelaunchAfterExit is now TryScheduleRelaunchAfterExit: it catches Win32Exception, InvalidOperationException and PlatformNotSupportedException from Process.Start, logs them and returns false, like TrySpawnSwapHelper. AppRestartHelper acts on the result. With a main window the callback throws a private marker exception, so ApplicationShutdownCoordinator rolls _isQuitting back and skips shutdown() as it already does, and RestartAsync catches it and logs instead of leaving it in the unobserved task. Without a main window it returns before Shutdown(). RelaunchProcess (no callers today) no longer reaches Environment.Exit when the helper did not start.
Re-verified on Fedora 44 with /bin/sh made unexecutable in a bubblewrap sandbox (bwrap --dev-bind / / --ro-bind /dev/null /usr/bin/sh) and a console harness calling the method: the previous head throws Win32Exception: An error occurred trying to start process '/bin/sh' ... Permission denied, this head returns False and the process keeps running. The new TryStartRelaunchHelper_ReturnsFalseWhenTheHelperCannotStart runs on every OS; narrowing the catch back to PlatformNotSupportedException fails it. What I could not run: the Restart click itself (synthetic input does not reach the window on my Wayland session) and UniGetUI.Tests, which only builds on Windows, so the window staying open rests on the coordinator's existing rollback.
| if [ -n "$bundle" ]; then | ||
| /usr/bin/open -na "$bundle" | ||
| else | ||
| "$exe" >/dev/null 2>&1 & | ||
| fi |
There was a problem hiding this comment.
The macOS branch has no fallback and swallows failure, which can leave no app running at all.
FindAppBundle matches any ancestor directory whose name ends in .app (case-insensitive) — it does not verify that the directory is a real bundle. If open refuses the path (a tarball extracted into a directory that merely ends in .app, a bundle LaunchServices rejects as damaged or quarantined, an unregistered ad-hoc-signed build), the helper exits non-zero, nobody reads that status, and UniGetUI has already terminated. The user accepts "restart" after a dependency install and the app is simply gone — the bug this PR set out to remove, moved to macOS.
Falling back to the direct-exec branch costs one token:
/usr/bin/open -na "$bundle" || "$exe" >/dev/null 2>&1 &There was a problem hiding this comment.
Addressed in 1342c47 with the line you suggested: /usr/bin/open -na "$bundle" || "$exe" >/dev/null 2>&1 &.
The new RelaunchHelper_StartsTheExecutableWhenOpenRejectsTheBundle runs the real helper with a bundle path that does not exist (on Linux there is no /usr/bin/open at all; on macOS open refuses the path) and asserts the executable starts only after the stand-in process exits. 5 consecutive runs green on Fedora 44; removing the || "$exe" fallback fails it and the script assertion. Still not run on a Mac.
| // the script text. The wait gives up after 30 seconds, like the self-updater's helper. | ||
| const string script = """ | ||
| pid="$1"; exe="$2"; bundle="$3" | ||
| i=0 | ||
| while kill -0 "$pid" 2>/dev/null && [ "$i" -lt 150 ]; do sleep 0.2; i=$((i+1)); done |
There was a problem hiding this comment.
The 30 s cap relaunches unconditionally, which diverges from the Windows branch.
The loop exits on either condition and nothing afterwards checks which one ended it. If the old process is still alive at 30 s — shutdown wedged past the 5 s IPC-stop cap in StopAndExitApplicationAsync, or the MainWindow.Instance is null path in AppRestartHelper.RestartAsync where Avalonia's Shutdown() can be cancelled by a handler — the helper launches a second instance. The single-instance guard (AvaloniaAppHost.TryRegisterSingleInstance, flock on macOS) then makes that second instance forward its args and exit; once the first process finally dies, no UniGetUI is running.
Wait-Process -Id on the Windows branch has no such cap. The self-updater this was copied from has a real reason for one (it must perform the swap regardless); a relaunch does not. Either wait longer, or skip the launch when the loop ended on timeout.
There was a problem hiding this comment.
Addressed in 1342c47: the cap is gone. The helper now waits with while kill -0 "$pid" 2>/dev/null; do sleep 0.2; done, like Wait-Process -Id.
Re-verified with a harness that schedules the relaunch and then stays alive for 40 s. Previous head: scheduled 16:59:18.299, the target started at 16:59:48.473 while the harness was still alive, harness exited 16:59:58.299. This head: same schedule and exit, the target started at 16:59:58.331, 32 ms after the exit. With the Debug UniGetUI as the target, the relaunched instance's start time equals the observed harness exit at the kernel clock's 10 ms resolution. There is no automated test for this one, since it would need a wait longer than 30 s.
| if (OperatingSystem.IsWindows()) | ||
| { | ||
| return; | ||
| } |
There was a problem hiding this comment.
The tests covering the new code path never run in CI and report as passed.
CreateRelaunchStartInfo_OffWindows_WaitsForThePidInAShellHelper, CreateRelaunchStartInfo_OnMacOs_PassesTheBundleToTheHelper and RelaunchHelper_StartsTheExecutableOnlyAfterTheProcessExits all open with a bare return guard, and .github/workflows/dotnet-test.yml runs on windows-latest only. All three exit immediately and xUnit reports green — the /bin/sh helper, bundle detection on a real macOS path, and the end-to-end relaunch have zero automated coverage, so a regression in the script text would ship without a red build. Only the FindAppBundle_* theories (string-only, separator-agnostic) actually execute.
Assert.Skip.If(...) / SkipUnless instead of a bare return would at least make the gap visible in the test report.
There was a problem hiding this comment.
Addressed in 1342c47. The test projects are on xunit 2.9.3, which has no Assert.Skip (that arrived in v3), so PlatformFacts.cs adds WindowsFact, UnixFact and MacOSFact: FactAttribute subclasses that set Skip when the test is not on its platform. The bare return guards are gone. On Linux, Core.Tools now reports Passed: 362, Failed: 4, Skipped: 2 (the 4 are the pre-existing Windows-only failures; the 2 skips are the Windows and macOS facts), and by the same mechanism the three UnixFact tests report as skipped on windows-latest instead of passed. That makes the gap visible in the report; it does not close it, since CI still runs the tests on Windows only.
Review follow-up for the off-Windows relaunch. TryScheduleRelaunchAfterExit (was ScheduleRelaunchAfterExit) catches the Process.Start failures (Win32Exception, InvalidOperationException, PlatformNotSupportedException), logs them and returns false, as AvaloniaAutoUpdater.TrySpawnSwapHelper does. AppRestartHelper then cancels the restart: with a main window it throws from the coordinator callback, which rolls the shutdown back and keeps the window open, and it logs instead of leaving the exception in the fire-and-forget task; without one it skips Shutdown(). RelaunchProcess no longer exits when the helper did not start. The /bin/sh helper waits for the pid with no cap, like Wait-Process on Windows, so it never starts a second instance while the first is still shutting down. On macOS it falls back to starting the executable when open rejects the bundle. Tests for one platform use WindowsFact, UnixFact or MacOSFact, so they report as skipped elsewhere instead of passing without running. New: TryStartRelaunchHelper returning false for a helper that cannot start, and the real helper falling back to the executable when the bundle cannot be opened. Relates to Devolutions#5237
This branch has not been deployed
2 participants
On macOS and Linux, accepting a restart (after a dependency install, from a setting that needs one, or from the portable-import banner) closes UniGetUI and never brings it back; the next start reports
Win32Exception: An error occurred trying to start process 'powershell.exe'. This PR relaunches through a detached/bin/shhelper off Windows, the same mechanism the macOS/Linux self-updater already uses, and leaves the Windows path as it is.Why it happened
CoreTools.ScheduleRelaunchAfterExitbuildspowershell.exe -NoProfile -WindowStyle Hidden -Command "Wait-Process -Id <pid>; Start-Process -FilePath '<exe>'"on every platform.AppRestartHelper.ResolveRestartExecutablePathalready returnsEnvironment.ProcessPathoff Windows, so the executable is right and only the launcher is wrong. The throw happens insideApplicationShutdownCoordinator.RequestAsync, which invokes the relaunch callback after authorizing the shutdown and rethrows beforeshutdown()runs;AppRestartHelper.Restart()is fire-and-forget, so the exception escapes and surfaces on the next start, which is what #5237 describes.What changed
Tools.cs:ScheduleRelaunchAfterExitis nowTryScheduleRelaunchAfterExit; it builds the start info withCreateRelaunchStartInfo(pid, path), logs one Debug line, and returns false (logging the error) whenProcess.Startfails, likeAvaloniaAutoUpdater.TrySpawnSwapHelper.powershell.exe, same arguments, same'escaping)./bin/sh -c <script> sh <pid> <exe> <bundle>. The script waits withkill -0 "$pid"(0.2 s polls, no cap, likeWait-Process) and then runs"$exe" >/dev/null 2>&1 &, or/usr/bin/open -na "$bundle"when the executable is inside a.appbundle on macOS, falling back to the executable whenopenrejects the bundle. Arguments are positional, so no path is interpolated into the script text. This isAvaloniaAutoUpdater.TrySpawnSwapHelper's shape.FindAppBundlewalks up the path for a*.appancestor; it is applied only on macOS.InternalsVisibleTo.csforUniGetUI.Core.Tools.Tests, like the other Core projects.AppRestartHelper: when the helper did not start, the restart is cancelled and UniGetUI stays open. With a main window the callback throws a private marker exception, soApplicationShutdownCoordinatorrolls back as it already does, andRestartAsynccatches and logs it; without one,Shutdown()is skipped. The self-updater is untouched.PlatformFacts.cs(tests):WindowsFact,UnixFact,MacOSFact, so a test for another platform reports as skipped instead of passing without running (xunit 2.9.3 has noAssert.Skip).Verification
Fedora 44, .NET SDK 10.0.401, no
powershell.exeorpwsh; Debug build ofUniGetUI.Avalonia.slnx.CoreTools.ScheduleRelaunchAfterExit(<own executable>)from a console harness (whatAppRestartHelper.Restart()reaches off Windows)Win32Exception: An error occurred trying to start process 'powershell.exe' with working directory '...'. No such file or directoryat Tools.cs:173mainand here;CreateRelaunchStartInfo_OnWindows_WaitsAndStartsThroughPowerShellpins the assembled string, including the'doubling, on Windows CIFindAppBundlehas 5 rows that run on every OS; theopen -nawiring is asserted by a macOS-only test and the script text by every non-Windows run. tromm, if you can try a restart from the dependency dialog on this build, that would confirm the bundle path.UniGetUI.Core.Tools.Tests353 to 362 passed and 2 skipped (11 new: 6 facts, 2 Theories with 5 rows; the skips are the Windows-only and macOS-only facts); the 4 failures on Linux are the pre-existing Windows-only ones (ShortcutFileRemoverTests,TestEnvVariable*), unchanged. The real-run test starts asleepchild as the exiting process and a marker script as the executable, and asserts the marker is absent while the child lives and present within 5 s after it exits; 5 consecutive runs, about 1 s each.kill -0wait line fails 2 tests (the marker appears while the child is alive); swapping the pid and executable slots fails the same 2; removing the|| "$exe"fallback fails 2; narrowing theProcess.Startcatch failsTryStartRelaunchHelper_ReturnsFalseWhenTheHelperCannotStart./bin/shunexecutable in a bubblewrap sandbox, the old code throwsWin32Exception ... Permission deniedand the new one returns false; with the process kept alive for 40 s, the old helper started the target at 30.2 s while it was alive and the new one 32 ms after it exited; the Debug UniGetUI relaunched through the new helper started at the observed exit (10 ms clock resolution).dotnet format whitespace src --folder --verify-no-changesanddotnet format style UniGetUI.Avalonia.slnx --verify-no-changesclean;dotnet restore+dotnet test UniGetUI.Avalonia.slnxfrom a fresh clone with an empty NuGet cache gives the same Core.Tools numbers.AppRestartHelperpasses and relaunches the same binary.Review guide
Tools.cs: the Windowsifblock is the old code; read the script (7 lines) and the sixArgumentList.Addcalls;FindAppBundleis a loop overPath.GetDirectoryName.RelaunchTests.cs: oneWindowsFact, oneMacOSFact, threeUnixFacts;TryStartRelaunchHelper_ReturnsFalseWhenTheHelperCannotStartand the fiveFindAppBundlerows run everywhere.Not in this PR
Closes #5237
Relates to #5238