Improve runtime session service stability #5380

seeM · 2024-11-15T16:26:06Z

The aim of this PR is to improve the stability of the runtime session service, with a slight focus on notebooks.

This is another step toward #2671.

I've also added an extensive suite of unit tests for the runtime session service, for two reasons:

Some of the issues we're seeing with notebooks are intermittent and therefore tricky to reproduce without tests.
I wanted to ensure that I didn't break any existing behavior with these changes as well as more planned changes to add notebook-runtime methods to the extension API.

Since there are a bunch of changes to some core code, I've annotated them with comments.

…untime is already starting

…untime is already running

…uests

…exceptions

…tart queuing tests

…ame ID in the case of restores

…ession maps

seeM · 2024-11-26T11:34:12Z

extensions/positron-notebook-controllers/src/notebookController.ts

+
+			// Avoid unhandled rejections being logged to the console.
+			// The actual error-handling is in the catch block above.
+			currentMessagePromise.catch(() => { });


Cleaning up some log noise.

seeM · 2024-11-26T11:35:22Z

src/vs/workbench/contrib/positronPreview/browser/positronPreview.contribution.ts


 // The Positron preview view icon.
 const positronPreviewViewIcon = registerIcon('positron-preview-view-icon', Codicon.positronPreviewView, nls.localize('positronPreviewViewIcon', 'View icon of the Positron preview view.'));

-export const POSITRON_PREVIEW_PLOTS_IN_VIEWER = 'positron.viewer.interactivePlotsInViewer';


Needed to move this from browser to common to fix failing tests as well as a linting error that must've snuck through a while ago.

seeM · 2024-11-26T11:36:13Z

src/vs/workbench/services/runtimeSession/common/runtimeSession.ts

-			session.session.runtimeMetadata.runtimeId === runtimeId &&
-			session.session.metadata.sessionMode === LanguageRuntimeSessionMode.Console &&
-			session.state !== RuntimeState.Exited);
+		// It's possible that there are multiple consoles for the same runtime,


Fixes a bug where the old console is returned after a failed restore attempt.

Is there a repo case for that / is that filed? This mitigation is fine but I think we should also ensure we only create one console in that case.

seeM · 2024-11-26T11:36:50Z

src/vs/workbench/services/runtimeSession/common/runtimeSession.ts

@@ -367,22 +371,29 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession
 		session: ILanguageRuntimeSession, exitReason: RuntimeExitReason): Promise<void> {
 		// We wait for `onDidEndSession()` rather than `RuntimeState.Exited`, because the former
 		// generates some Console output that must finish before starting up a new runtime:
+		let disposable: IDisposable | undefined;


Fixes a leaked disposable if shutting down errors and the session never exits.

seeM · 2024-11-26T11:38:39Z

src/vs/workbench/services/runtimeSession/common/runtimeSession.ts

-						`for the ${metadata.languageName} language.`);
-				}
-			}
+		const runningSessionId = this.validateRuntimeSessionStart(sessionMode, languageRuntime, notebookUri, source);


We now also validate that only one runtime is ever running per notebook URI and returning an existing session if it matches.

seeM · 2024-11-26T11:43:29Z

src/vs/workbench/services/runtimeSession/common/runtimeSession.ts


-		// Create a promise that resolves when the runtime is ready to use.
-		const startPromise = new DeferredPromise<string>();


The start promise was previously always created here but now might be created by the caller, and we don't want to overwrite that.

seeM · 2024-11-26T11:43:51Z

src/vs/workbench/services/runtimeSession/common/runtimeSession.ts

@@ -814,8 +855,7 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession
 				`Creating session for language runtime ` +
 				`${formatLanguageRuntimeMetadata(runtimeMetadata)} failed. Reason: ${err}`);
 			startPromise.error(err);
-			this._startingSessionsBySessionMapKey.delete(sessionMapKey);
-			this._startingConsolesByLanguageId.delete(runtimeMetadata.languageId);
+			this.clearStartingSessionMaps(sessionMode, runtimeMetadata, notebookUri);


Fixes a bug where notebooks weren't cleared from the map, and where a starting notebook could affect a starting console.

seeM · 2024-11-26T11:44:44Z

src/vs/workbench/services/runtimeSession/common/runtimeSession.ts

-			// Remove the runtime from the starting runtimes.
-			this._startingConsolesByLanguageId.delete(session.runtimeMetadata.languageId);
-			this._startingSessionsBySessionMapKey.delete(sessionMapKey);
+			this.clearStartingSessionMaps(


Fixes a bug where notebooks weren't cleared from the map, and where a starting notebook could affect a starting console.

seeM · 2024-11-26T11:46:07Z

src/vs/workbench/services/runtimeSession/common/runtimeSession.ts

 					if (session.metadata.sessionMode === LanguageRuntimeSessionMode.Console &&
 						!this._consoleSessionsByLanguageId.has(session.runtimeMetadata.languageId)) {
 						this._consoleSessionsByLanguageId.set(session.runtimeMetadata.languageId,
 							session);
+					} else if (session.metadata.sessionMode === LanguageRuntimeSessionMode.Notebook &&


Fixes a bug where notebooks weren't added to the maps after a restart.

seeM · 2024-11-26T12:23:23Z

src/vs/workbench/services/runtimeSession/common/runtimeSession.ts

@@ -1079,6 +1240,8 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession
 			state === RuntimeState.Ready) {
 			// The runtime looks like it could handle a restart request, so send
 			// one over.
+			this.setStartingSessionMaps(


Fixes a bug where a second console/notebook could be created if done so while the runtime was in the exited state during a restart.

jmcphers

Compiled locally and confirmed tests passing, also spun out a few sessions in a built copy and confirmed that things looked stable.

This LGTM. Thanks so much for improving this and adding some tests.

Before you merge, please run the Full Test Suite on this branch; this code is pretty sensitive due to the amount of async state propagating around.

jmcphers · 2024-11-27T22:50:23Z

src/vs/workbench/services/runtimeSession/common/runtimeSession.ts

-			session.session.runtimeMetadata.runtimeId === runtimeId &&
-			session.session.metadata.sessionMode === LanguageRuntimeSessionMode.Console &&
-			session.state !== RuntimeState.Exited);
+		// It's possible that there are multiple consoles for the same runtime,


Is there a repo case for that / is that filed? This mitigation is fine but I think we should also ensure we only create one console in that case.

src/vs/workbench/services/runtimeSession/common/runtimeSession.ts

…vironment

seeM · 2024-11-28T13:43:42Z

E2E tests passed thrice. Unit tests failed in the first two attempts, fixed in 811cc2f and 25cea7f. All green on the third attempt, so we should be good to merge!

Attempt 1: https://github.com/posit-dev/positron/actions/runs/12067290897/attempts/1

Attempt 2: https://github.com/posit-dev/positron/actions/runs/12068410989/attempts/1

Attempt 3: https://github.com/posit-dev/positron/actions/runs/12069245433/attempts/1

seeM force-pushed the 2671-runtime-session-service-tests branch 2 times, most recently from b736859 to 4dc4cbb Compare November 22, 2024 07:34

seeM added 28 commits November 26, 2024 13:31

throw an error when starting a notebook session without a notebook uri

3d9d2e3

throw an error when starting a runtime for a notebook while another r…

3a354a3

…untime is already starting

throw an error when starting a runtime for a notebook while another r…

d58c28c

…untime is already running

update restoreRuntimeSession: add validation; coalesce concurrent req…

19368eb

…uests

fix leaked disposable

bb7df67

log uncaught exception

974aa05

update test session manager for tests

a7197bd

checkpoint tests

5f1a33f

checkpoint 2 tests

5883cd8

refactor to generate tests

7069706

add selectRuntime to start tests

ad4a962

log uncaught promise rejection

4187cf5

all current tests passing

fe6a5f9

add auto start and more select tests;

34a37c3

change error message when selecting a non-existing runtime

fe5a76e

throw when starting a notebook in an untrusted workspace

6487dea

coalesce concurrent auto start requests; validate before auto start

987af00

finalize auto start tests

d6eb11c

fix typo

fa91bf6

add restart tests

79cad9e

fix restarting notebooks

ed52e44

test encountering createSession/restoreSession error; catch uncaught …

6044bda

…exceptions

refine test session shutdown & restart behavior; fix related tests; s…

1b54978

…tart queuing tests

skip failing test for now

b1eb0c4

fix leaked disposable

0a89f1e

wip fixing restart queueing issue

d15995b

make console session for language/runtime checks explicit

8ae2a4a

return the correct session for selectRuntime test helper

0558d3d

seeM added 12 commits November 26, 2024 13:31

make activeSessions check explicit

84c37aa

test starting after an error was encountered previously

b67b6f9

test actual objects since there could be multiple sessions with the s…

4e7efaf

…ame ID in the case of restores

remove unneeded roundtrip

9acb4b0

we can actually use getConsoleSessionForRuntime after the fix

801d0d7

fix: return the latest matching console in getConsoleSessionForRuntime

275b611

fix: notebook sessions not being set/cleared from starting starting s…

c9077df

…ession maps

re-add restore fires onWillStartSession test

7b84fda

fix import across environment types

f9bee54

fix leaked disposable

688322d

stop notebook runtime error messages from showing up in the console

2d5092a

sort by createdTimestamp instead of insert order

586b7e1

seeM force-pushed the 2671-runtime-session-service-tests branch from 36827f0 to 586b7e1 Compare November 26, 2024 11:32

fix multiple sessions having the same createdTimestamp

0259c3d

seeM changed the title ~~Runtime session service unit tests; various fixes & improvements~~ Improve runtime session service stability Nov 26, 2024

seeM requested a review from jmcphers November 26, 2024 12:27

seeM commented Nov 26, 2024

View reviewed changes

seeM marked this pull request as ready for review November 26, 2024 12:28

fix restart while ready not clearing the starting maps

0e3cf4f

jmcphers previously approved these changes Nov 27, 2024

View reviewed changes

add onDidEndSession disposable to the session's disposables

065e6e7

seeM dismissed jmcphers’s stale review via 065e6e7 November 28, 2024 10:41

seeM added 2 commits November 28, 2024 13:53

fix uncaught error

811cc2f

dont use assert.strict since it doesnt seem to work in the browser en…

25cea7f

…vironment

seeM merged commit a36f4d7 into main Nov 28, 2024
10 checks passed

seeM deleted the 2671-runtime-session-service-tests branch November 28, 2024 13:44

github-actions bot locked and limited conversation to collaborators Nov 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve runtime session service stability #5380

Improve runtime session service stability #5380

seeM commented Nov 15, 2024 •

edited

Loading

seeM Nov 26, 2024

seeM Nov 26, 2024

seeM Nov 26, 2024

jmcphers Nov 27, 2024

seeM Nov 26, 2024

seeM Nov 26, 2024

seeM Nov 26, 2024

seeM Nov 26, 2024

seeM Nov 26, 2024

seeM Nov 26, 2024

seeM Nov 26, 2024

jmcphers left a comment

jmcphers Nov 27, 2024

seeM commented Nov 28, 2024


		// Create a promise that resolves when the runtime is ready to use.
		const startPromise = new DeferredPromise<string>();

Improve runtime session service stability #5380

Improve runtime session service stability #5380

Conversation

seeM commented Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmcphers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seeM commented Nov 28, 2024

seeM commented Nov 15, 2024 •

edited

Loading