Only restart runtimes on RuntimeExitReason.Error

[DavisVaughan]

Addresses #628

I realized that when q() is called in R, we do actually know that the exit code was 0, and we treat this as an Unknown exit reason (by definition, Unknown also means an exit code of 0, otherwise it would have been upgraded to an Error).

I think pretty much all of the expected cleanup is already happening on the Positron side when q() is called, the only weird thing is that we are treating an Unknown exit like a crash and automatically restarting.

It turns out that we can fix that pretty easily by only auto-restarting when the exit reason is Error, where previously we restarted if the exit reason is Error OR Unknown.

I think we should treat Unknown as more of an "expected" case where the shutdown was initiated by the kernel. I would be in favor of possibly changing the name of Unknown to BackendShutdown or something, and then change Shutdown to FrontendShutdown? i.e. here:

positron/src/vs/workbench/services/languageRuntime/common/languageRuntimeService.ts

Lines 312 to 338 in 7b71e16

	* Possible reasons a language runtime could exit.
	*/
	export enum RuntimeExitReason {
	/** The runtime exited because it could not start correctly. */
	StartupFailed = 'startupFailed',
	/** The runtime is shutting down at the request of the user. */
	Shutdown = 'shutdown',
	/** The runtime exited because it was forced to quit. */
	ForcedQuit = 'forcedQuit',
	/** The runtime is exiting in order to restart. */
	Restart = 'restart',
	/** The runtime is exiting in order to switch to a new runtime. */
	SwitchRuntime = 'switchRuntime',
	/** The runtime exited because of an error, most often a crash. */
	Error = 'error',
	/**
	* The runtime exited for an unknown reason. This typically means that
	* it exited unexpectedly but with a normal exit code (0).
	*/
	Unknown = 'unknown',
	}

Screen.Recording.2024-05-06.at.6.32.09.PM.mov

[DavisVaughan]

This would rely heavily on correctly reading the error code off the process, i.e. this code

positron/extensions/jupyter-adapter/src/JupyterKernel.ts

Lines 1449 to 1478 in 7b71e16

	let exitCode = 0;
	try {
	if (fs.existsSync(state.logFile)) {
	const lines = fs.readFileSync(state.logFile, 'utf8').split('\n');
	const lastLine = lines[lines.length - 2];
	if (lastLine) {
	try {
	const match = lastLine.match(/exit code (\d+)/);
	if (match) {
	exitCode = parseInt(match![1]);
	} else {
	this.log(`Last line of log file ${state.logFile} ` +
	`does't name an exit code: ${lastLine}`);
	}
	if (isNaN(exitCode)) {
	this.log(`Could not parse exit code in last line of log file ` +
	`${state.logFile}: ${lastLine}`);
	}
	} catch (e) {
	this.log(`Could not find exit code in last line of log file ` +
	`${state.logFile}: ${lastLine} (${e}))`);
	}
	}
	} else {
	this.log(`Not reading exit code from process ${state.processId}; ` +
	`log file ${state.logFile} does not exist`);
	}
	} catch (e) {
	this.log(`Error reading exit code from log file ${state.logFile}: ${e}`);
	}

[DavisVaughan]

@jmcphers with 1663456, the toast notifications that appear at second 18 in the video that occasionally popped up should no longer appear - instead they are only logged to our LSP output channel. That should ensure that q() with an exit code of 0 behaves pretty nicely and never bombards the user with toast notifications. The LSP Client will detect that its server is no longer connected, and will log an error but never show that to the user or try to restart itself.

It had to do with this case in the middleware:
https://github.com/microsoft/vscode-languageserver-node/blob/4b5f9cf622963dcfbc6129cdc1a570e2bb9f66a4/client/src/common/client.ts#L1637-L1642

If we don't tell the middleware that we are "handling" the error, then it sets "force" which forces a toast notification.

It was essentially the same issue as #2880, but coming at it from a different angle.

---

It is worth noting that this idea of "indicating shutdown from the kernel side" was also discussed in the jupyter-client repo, and they also came up with the idea of using an exit code of 0 as indication that the kernel "exited normally", making me feel a bit more confident about this approach.
jupyter/jupyter_client#237 (comment)

[DavisVaughan]

I now believe this is enough to "address" #628 and have updated it to reflect that

In theory an even better long term solution was if our LSP server ran in a separate process that communicates with the R process to get information like global env completions but would not fall down if R crashes or quits. That way, only the Client would be in charge of shutting down the LSP server, and in that case if the LSP server shut down early, then that would mean something is truly quite wrong (i.e. it is not just a reflection of R quitting).