AppLifecycle Restart API

[hamza-usmani]

Bringing in Restart API spec into main. This spec addresses the gaps in restart today and how we can expose new functions to manage restart across all app types.

[riverar]

Please review spec authoring/merge guidance at https://github.com/microsoft/WindowsAppSDK/tree/main/specs.

Also I don't recall this ever going through review?

[Samuel12321]

Issue #111

Does this support restarting with identity

[DrusTheAxe]

Why all the HTML? Markdown supports tables (and with much simpler syntax)

[DrusTheAxe]

I think your TOC is out of sync with your actual content

SUGGESTION: Use VSCode with the Markdown All in One extension, then issue commands via Ctrl-P

1. Markdown All in One: Add/Update section numbers
2. Put cursor at top of file
3. Markdown All in One: Create Table of Contents

and if/when you make edits the pair of ...Add/Update section numbers + ...Update Table of Contents keeps everything correct and current

[DrusTheAxe]

Please use specs/spec_template.md

You can find existing specs @ specs for reference examples. For example a really good simple example see DeploymentAPI. For more extensive specs AppLifecycle Activation and [Dynamic Dependencies]https://github.com/microsoft/WindowsAppSDK/blob/main/specs/dynamicdependencies/DynamicDependencies.md) are good examples.

[DrusTheAxe]

I can't tell what kind of API this is. WinRT? But it's just a function w/o mention where it lives (no runtimeclass or interface mentioned). Flat-C? But then why 'static', and the data types imply otherwise.

If you're intending WinRT I'd expect to see a MIDL3 .idl (or at least the relevant subset of one)

Please use the standard template and see other existing specs for representative examples how to express your API.

[aeloros]

RestartPending is only returned if another restart is in progress. Since that is more likely for the async variation in the platform, this should only ever happen if the API is called twice by the same process.

[hamza-usmani]

Updated with this information.

[DrusTheAxe]

NIT: Indent

            Telemetry::WriteLine("RegisterForRestart: Restart Context: " + RestartContext::Crash.ToString() 
                + ". Result: " + result ? "success" : "fail");

[DrusTheAxe]

Why the backslashes? Are they needed in a code block (blah)?

[DrusTheAxe]

NIT: WCHAR buffer[1024]{};

ES.23: Prefer the {}-initializer syntax

[DrusTheAxe]

Magic constant. And why 1024? Why not 256 or 2K or...?

Also, buffer is assigned in some cases but not used?
If it's needed, why not std::wstring?
But it's unclear if this is needed at all

[DrusTheAxe]

NIT: {} initialization

AppRestartFailureReason result{ AppLifecycle::RequestRestartNow("args for new instance") };

[jonwis]

Apologies for the delay here. API reviewers, please leave your comments on the document. We'll go through them at 1pm PST on January 11th, 2022. We'll specifically be reviewing https://github.com/microsoft/WindowsAppSDK/pull/1134/files/c7c31991d80601c06ee52e8ec7c5e3929cfe7d28 in case additional items get pushed.

[riverar]

⚠️ AppName could contain PII. Do not recommend using as-is. #ByDesign

[oldnewthing]

App names have generally been considered non-PII. App paths contain PII, but not the executable name itself.

[riverar]

Darn, I better rename RafsIceCreamConeMaker.exe.

(I see there's established precedent for this so OK.)

[oldnewthing]

In practice, software developers do not ship a custom executable name for each customer. (ContosoDeluxe-BobSmithFrom Denver.exe, ContosoDeluxe-JaneJohnsonFromChicago.exe, ContosoDeluxe-PatWilsonFromLosAngeles.exe.) The executable name is the same for all customers: ContosoDeluxe.exe.

RafsIceCreamConeMaker.exe is presumably a piece of software that is marketed as "Check out this cool program 'Raf's Ice Cream Cone Maker', in which a cartoon character name Raf helps you design the perfect ice cream cone." It doesn't tell you who bought the program. Raf is the program name, not the customer name. (That's the theory at least.)

[riverar]

One of my favorites, PowerGrep, ships SetupPowerGREP522RafaelRivera.exe. (The executable is generated server-side for licensing purposes.) Hex Rays (maker of IDA) also ships custom executables with uniquely identifying details (e.g. idapronw_hexx86w_hexarmw_orderId_userSpecificId). (Not relevant in this context, given they're one-shot installers.) But I was also thinking of scenarios where I fire up Visual Studio and just hack on a project and don't necessarily want information about it going back to the mothership. All outliers in the grand scheme, perhaps!

[riverar]

The Agent will then terminate the application [...]

Am concerned about cleanup here, if the Agent handles app termination.

An app may only cleanup unmanaged resources (e.g. GDI handles) when terminating cleanly. Is the thinking here that the developer needs to redesign their application to have a cleanup-first-then-call-RequestRestartNow code path? Perhaps this can be redesigned so that the Agent sends back an ACK to the caller, which can then attempt to exit gracefully from within?

[oldnewthing]

Apps already have to deal with abnormal termination in the case of termination-on-suspension, app crash, or the user terminating from Task Manager. Resources like kernel handles and GDI resources are automatically cleaned up at process termination. Other resources like temporary files will have to be cleaned up by the restarted application, just like any other launch after termination. So no new recovery scenarios are introduced here.

[riverar]

I was thinking more along the lines of shared GDI resources like icons and possible impact to Desktop Bridge middleware, but I concede this all falls under abnormal termination handling.

[riverar]

It's not clear what the app developer will do these reasons. InvalidUser leaks out some obscure internal details (observe I never specified a user), RestartPending means I'm about to die and any sort of handling here is not guaranteed to run, and Other is useless.

Perhaps this can be replaced with a Boolean indicating if the request was accepted or not. (Duplicate requests can be dropped/ignored/merged quietly.)

[riverar]

What happens if the restart takes too long and the user starts the app manually? Does the Agent still start the app? What is the value for PreviousExecutionState? Or will multiple instances start, with varying states?

Perhaps an opportunity for the Agent to provide progress UI?

[jonwis]

DISCUSSION: Similar to clicking the tile twice, the API-launched and user-launched instances will come up. Both get the "previous state = terminated" value. The user-launched one gets whatever the tile would have passed to launch the app, the API-launched one gets the command line. The single-instancing APIs - also on this type, AppInstance - can help developers resolve this into a single target instance.

[riverar]

Nits:

Suggested change

	| AppType | UWP App or Centennial App or Win32 app | String |
	| AppType | UWP app or Desktop Bridge app or Win32 app | String |

[jonwis]

RECOMMEND: Fix. s/Centennial/Desktop Bridge/gi

[hamza-usmani]

Fixed

[andrewleader]

The class that the method is in looks like it should be AppInstance based on Scott's code pull request (AppLifecycle is the namespace), so should be AppInstance.RequestRestartNow("args") (and prolly needs updating in multiple places.

[jonwis]

RECOMMEND: Yes, AppInstance (when you update the IDL it'll be clear.)

[hamza-usmani]

Fixed

[jeffstall]

If this is real C#, then please use "." instead of "::".

[jonwis]

RECOMMEND: Fix

[hamza-usmani]

Fixed

[jeffstall]

How would I cancel the previous restart and "upgrade" it with new information? It would be helpful if the sample showed this.

[jonwis]

DISCUSSION: Once a request has been started there's no cancelling it.

[jeffstall]

Can the sample show how the dialog box would get the correct message text to show the end user?

[jonwis]

SAMPLE: It will likely say "something went wrong". There's not much to say other than "you will need to manually fix it, user."

[hamza-usmani]

Updated

[johnstep]

How is the helper process lifetime managed? What happens if the app is running elevated or as a different user? If the helper is not started by the app, it can handle ERROR_ELEVATION_REQUIRED from CreateProcess by trying again with ShellExecute, but that only works if the executable requires admin, and does not guarantee the same user.

[jonwis]

DISCUSSION: The elevation state of the app is maintained in the restarted process.

RECOMMEND: Ensure the behaviors of elevation levels is documented. Also document how much of the source environment is flowed to the target.

DISCUSSION: If the source exe is marked delete-on-close, the agent could terminate that instance then call CreateProcess and get back a "file not found", etc. The caller gets back an "other" error. Ultimately the worst possible case is the app terminates and cannot be restarted, and the user goes "huh" then clicks the tile again.

[hamza-usmani]

Added

[jonwis]

Please fix the two things, and run a word-wrapper (check out the docs/coding-guidelines.md for suggested tooling.) Looks fine otherwise.

[jonwis]

Consider putting a "this is the new one!" marker here. Also since this is a new API in 1.1, you'll need to add the proper contract information here, like:

[contractversion(1.1)] apicontract AppLifecycleContract{}

[contract(AppLifecycleContract, 1)]
runtimeclass AppInstance
{

    // On the new thing
    [contract(AppLifecycleContract, 1.1)]
    {
        static Windows.ApplicationModel.Core.AppRestartFailureReason Restart(String arguments);
    }
}

[hamza-usmani]

Added a marker for this.

On the note on contract: Is there a template for how/where the contract information belongs? Do I just drop it into the IDL as you've commented above?

[jonwis]

Markdown nit: Put a blank line before & after number/bullet lists.

[hamza-usmani]

Done for all lists!

Changes made post-API review address issues

[hamza-usmani]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).