Fix cleanup of R sessions

.vscode/launch.json

@@ -185,6 +185,7 @@
 				"order": 4
 			}
 		},
+		// --- Start Positron
 		{
 			// Note this uses the version of Code/Positron you launch it from.
 			// i.e., launch from dev Positron if your tests need dev Positron.
@@ -205,6 +206,7 @@
 				"order": 5
 			}
 		},
+		// --- End Positron
 		{
 			"type": "extensionHost",
 			"request": "launch",

extensions/positron-r/.zed/settings.json

@@ -0,0 +1,19 @@
+// Folder-specific settings
+//
+// For a full list of overridable settings, and general information on folder-specific settings,
+// see the documentation: https://zed.dev/docs/configuring-zed#settings-files
+{
+  "languages": {
+    "TypeScript": {
+      "tab_size": 2,
+      "hard_tabs": true,
+      "ensure_final_newline_on_save": true,
+      "remove_trailing_whitespace_on_save": true,
+      "format_on_save": "on",
+      "formatter": "language_server",
+      "code_actions_on_format": {
+        "source.fixAll.eslint": false
+      }
+    }
+  }
+}

extensions/positron-r/src/commands.ts

@@ -76,7 +76,7 @@ export async function registerCommands(context: vscode.ExtensionContext, runtime
 			if (!isInstalled) {
 				return;
 			}
-			const session = RSessionManager.instance.getConsoleSession();
+			const session = await RSessionManager.instance.getConsoleSession();
 			if (!session) {
 				return;
 			}
@@ -169,7 +169,7 @@ export async function registerCommands(context: vscode.ExtensionContext, runtime
 		}),
 
 		vscode.commands.registerCommand('r.scriptPath', async () => {
-			const session = RSessionManager.instance.getConsoleSession();
+			const session = await RSessionManager.instance.getConsoleSession();
 			if (!session) {
 				throw new Error(`Cannot get Rscript path; no R session available`);
 			}

extensions/positron-r/src/llm-tools.ts

@@ -15,7 +15,7 @@ export function registerRLanguageModelTools(context: vscode.ExtensionContext): v
 	const rListPackageHelpTopicsTool = vscode.lm.registerTool<{ sessionIdentifier: string; packageName: string }>('listPackageHelpTopics', {
 		invoke: async (options, token) => {
 			const manager = RSessionManager.instance;
-			const session = manager.getSessionById(options.input.sessionIdentifier);
+			const session = await manager.getSessionById(options.input.sessionIdentifier);
 			if (!session) {
 				return new vscode.LanguageModelToolResult([
 					new vscode.LanguageModelTextPart(`No active R session with identifier ${options.input.sessionIdentifier}`),
@@ -44,7 +44,7 @@ export function registerRLanguageModelTools(context: vscode.ExtensionContext): v
 	const rListAvailableVignettesTool = vscode.lm.registerTool<{ sessionIdentifier: string; packageName: string }>('listAvailableVignettes', {
 		invoke: async (options, token) => {
 			const manager = RSessionManager.instance;
-			const session = manager.getSessionById(options.input.sessionIdentifier);
+			const session = await manager.getSessionById(options.input.sessionIdentifier);
 			if (!session) {
 				return new vscode.LanguageModelToolResult([
 					new vscode.LanguageModelTextPart(`No active R session with identifier ${options.input.sessionIdentifier}`),
@@ -73,7 +73,7 @@ export function registerRLanguageModelTools(context: vscode.ExtensionContext): v
 	const rGetPackageVignetteTool = vscode.lm.registerTool<{ sessionIdentifier: string; packageName: string; vignetteName: string }>('getPackageVignette', {
 		invoke: async (options, token) => {
 			const manager = RSessionManager.instance;
-			const session = manager.getSessionById(options.input.sessionIdentifier);
+			const session = await manager.getSessionById(options.input.sessionIdentifier);
 			if (!session) {
 				return new vscode.LanguageModelToolResult([
 					new vscode.LanguageModelTextPart(`No active R session with identifier ${options.input.sessionIdentifier}`),
@@ -103,7 +103,7 @@ export function registerRLanguageModelTools(context: vscode.ExtensionContext): v
 	const rGetHelpPageTool = vscode.lm.registerTool<{ sessionIdentifier: string; packageName?: string; helpTopic: string }>('getHelpPage', {
 		invoke: async (options, token) => {
 			const manager = RSessionManager.instance;
-			const session = manager.getSessionById(options.input.sessionIdentifier);
+			const session = await manager.getSessionById(options.input.sessionIdentifier);
 			if (!session) {
 				return new vscode.LanguageModelToolResult([
 					new vscode.LanguageModelTextPart(`No active R session with identifier ${options.input.sessionIdentifier}`),

extensions/positron-r/src/session-manager.ts

@@ -5,12 +5,10 @@
 
 import * as positron from 'positron';
 import * as vscode from 'vscode';
-import { RSession } from './session';
+import { RSession, getActiveRSessions } from './session';
 
 /**
- * Manages all the R sessions. We keep our own references to each session in a
- * singleton instance of this class so that we can invoke methods/check status
- * directly, without going through Positron's API.
+ * Manages all the R sessions.
  */
 export class RSessionManager implements vscode.Disposable {
 	/// Singleton instance
@@ -21,9 +19,6 @@ export class RSessionManager implements vscode.Disposable {
 	/// but we may improve on this in the future so it is good practice to track them.
 	private readonly _disposables: vscode.Disposable[] = [];
 
-	/// Map of session IDs to RSession instances
-	private _sessions: Map<string, RSession> = new Map();
-
 	/// The most recent foreground R session (foreground implies it is a console session)
 	private _lastForegroundSessionId: string | null = null;
 
@@ -50,18 +45,12 @@ export class RSessionManager implements vscode.Disposable {
 	}
 
 	/**
-	 * Registers a runtime with the manager. Throws an error if a runtime with
-	 * the same ID is already registered.
+	 * Registers a runtime with the manager.
 	 *
-	 * @param id The runtime's ID
-	 * @param runtime The runtime.
+	 * @param session The session.
 	 */
-	setSession(sessionId: string, session: RSession): void {
-		if (this._sessions.has(sessionId)) {
-			throw new Error(`Session ${sessionId} already registered.`);
-		}
-		this._sessions.set(sessionId, session);
-		this._disposables.push(
+	setSession(session: RSession): void {
+		session.register(
 			session.onDidChangeRuntimeState(async (state) => {
 				await this.didChangeSessionRuntimeState(session, state);
 			})
@@ -99,18 +88,20 @@ export class RSessionManager implements vscode.Disposable {
 			return;
 		}
 
-		// TODO: Switch to `getActiveRSessions()` built on `positron.runtime.getActiveSessions()`
-		// and remove `this._sessions` entirely.
-		const session = this._sessions.get(sessionId);
+		const sessions = await getActiveRSessions();
+		const session = sessions.find(s => s.metadata.sessionId === sessionId);
 		if (!session) {
-			// The foreground session is for another language.
+			// The foreground session is for another language or was deactivated in the meantime
 			return;
 		}
 
 		if (session.metadata.sessionMode === positron.LanguageRuntimeSessionMode.Background) {
 			throw Error(`Foreground session with ID ${sessionId} must not be a background session.`);
 		}
 
+		// Multiple `activateConsoleSession()` might run concurrently if the
+		// `didChangeForegroundSession` event fires rapidly. We might want to queue
+		// the handling.
 		this._lastForegroundSessionId = session.metadata.sessionId;
 		await this.activateConsoleSession(session, 'foreground session changed');
 	}
@@ -120,7 +111,8 @@ export class RSessionManager implements vscode.Disposable {
 	 */
 	private async activateConsoleSession(session: RSession, reason: string): Promise<void> {
 		// Deactivate other console session servers first
-		await Promise.all(Array.from(this._sessions.values())
+		const sessions = await getActiveRSessions();
+		await Promise.all(sessions
 			.filter(s => {
 				return s.metadata.sessionId !== session.metadata.sessionId &&
 					s.metadata.sessionMode === positron.LanguageRuntimeSessionMode.Console;
@@ -152,9 +144,10 @@ export class RSessionManager implements vscode.Disposable {
 	 *
 	 * @returns The R console session, or undefined if there isn't one.
 	 */
-	getConsoleSession(): RSession | undefined {
+	async getConsoleSession(): Promise<RSession | undefined> {
+		const sessions = await getActiveRSessions();
+
 		// Sort the sessions by creation time (descending)
-		const sessions = Array.from(this._sessions.values());
 		sessions.sort((a, b) => b.created - a.created);
 
 		// Remove any sessions that aren't console sessions and have either
@@ -186,8 +179,9 @@ export class RSessionManager implements vscode.Disposable {
 	 * @param sessionId The session identifier
 	 * @returns The R session, or undefined if not found
 	 */
-	getSessionById(sessionId: string): RSession | undefined {
-		return this._sessions.get(sessionId);
+	async getSessionById(sessionId: string): Promise<RSession | undefined> {
+		const sessions = await getActiveRSessions();
+		return sessions.find(s => s.metadata.sessionId === sessionId);
 	}
 
 	/**

extensions/positron-r/src/session.ts

@@ -99,6 +99,9 @@ export class RSession implements positron.LanguageRuntimeSession, vscode.Disposa
 	/** Cache of installed packages and associated version info */
 	private _packageCache: Map<string, RPackageInstallation> = new Map();
 
+	/** Disposables. Disposed of after main resources (LSP, kernel, etc) */
+	private _disposables: vscode.Disposable[] = [];
+
 	/** The current dynamic runtime state */
 	public dynState: positron.LanguageRuntimeDynState;
 
@@ -126,7 +129,7 @@ export class RSession implements positron.LanguageRuntimeSession, vscode.Disposa
 		this._created = Date.now();
 
 		// Register this session with the session manager
-		RSessionManager.instance.setSession(metadata.sessionId, this);
+		RSessionManager.instance.setSession(this);
 
 		this.onDidChangeRuntimeState(async (state) => {
 			await this.onStateChange(state);
@@ -408,6 +411,15 @@ export class RSession implements positron.LanguageRuntimeSession, vscode.Disposa
 		if (this._kernel) {
 			await this._kernel.dispose();
 		}
+
+		// LIFO clean up of external resources
+		while (this._disposables.length > 0) {
+			this._disposables.pop()?.dispose();
+		}
+	}
+
+	async register(disposable: vscode.Disposable) {
+		this._disposables.push(disposable);
 	}
 
 	/**
@@ -882,7 +894,7 @@ export class RSession implements positron.LanguageRuntimeSession, vscode.Disposa
 				LOGGER.info(`Unknown DAP message: ${message.method}`);
 
 				if (message.kind === 'request') {
-					message.handle(() => { throw new Error(`Unknown request '${message.method}' for DAP comm`) });
+					message.handle(() => { throw new Error(`Unknown request '${message.method}' for DAP comm`); });
 				}
 			}
 		}
@@ -976,25 +988,31 @@ export function createJupyterKernelExtra(): JupyterKernelExtra {
 export async function checkInstalled(pkgName: string,
 	pkgVersion?: string,
 	session?: RSession): Promise<boolean> {
-	session = session || RSessionManager.instance.getConsoleSession();
+	session = session || await RSessionManager.instance.getConsoleSession();
 	if (session) {
 		return session.checkInstalled(pkgName, pkgVersion);
 	}
 	throw new Error(`Cannot check install status of ${pkgName}; no R session available`);
 }
 
 export async function getLocale(session?: RSession): Promise<Locale> {
-	session = session || RSessionManager.instance.getConsoleSession();
+	session = session || await RSessionManager.instance.getConsoleSession();
 	if (session) {
 		return session.getLocale();
 	}
 	throw new Error(`Cannot get locale information; no R session available`);
 }
 
 export async function getEnvVars(envVars: string[], session?: RSession): Promise<EnvVar[]> {
-	session = session || RSessionManager.instance.getConsoleSession();
+	session = session || await RSessionManager.instance.getConsoleSession();
 	if (session) {
 		return session.getEnvVars(envVars);
 	}
 	throw new Error(`Cannot get env var information; no R session available`);
 }
+
+/** Get the active R language runtime sessions. */
+export async function getActiveRSessions(): Promise<RSession[]> {
+	const sessions = await positron.runtime.getActiveSessions();
+	return sessions.filter((session) => session instanceof RSession) as RSession[];
+}

extensions/positron-r/src/test/ark-comm.test.ts

@@ -3,7 +3,7 @@
  *  Licensed under the Elastic License 2.0. See LICENSE.txt for license information.
  *--------------------------------------------------------------------------------------------*/
 
-import './mocha-setup'
+import './mocha-setup';
 
 import * as assert from 'assert';
 import * as vscode from 'vscode';
@@ -49,12 +49,12 @@ suite('ArkComm', () => {
 					i: -10
 				}
 			}
-		)
+		);
 	});
 
 	test('Can send request', async () => {
 		const requestReply = await assertRequest(comm, 'test_request', { i: 11 });
-		assert.deepStrictEqual(requestReply, { i: -11 })
+		assert.deepStrictEqual(requestReply, { i: -11 });
 	});
 
 	test('Invalid method sends error', async () => {
@@ -86,7 +86,7 @@ async function assertNextMessage(comm: Comm): Promise<CommBackendMessage> {
 		whenTimeout(5000, () => assert.fail(`Timeout while expecting comm message on ${comm.id}`)),
 	]) as any;
 
-	assert.strictEqual(result.done, false)
+	assert.strictEqual(result.done, false);
 	return result.value;
 }
 

extensions/positron-r/src/test/debugger.test.ts

@@ -3,7 +3,7 @@
  *  Licensed under the Elastic License 2.0. See LICENSE.txt for license information.
  *--------------------------------------------------------------------------------------------*/
 
-import './mocha-setup'
+import './mocha-setup';
 
 import * as vscode from 'vscode';
 import * as assert from 'assert';
@@ -55,7 +55,7 @@ suite('Debugger', () => {
 					new RegExp('Virtual namespace of package graphics'),
 					`Unexpected editor contents for ${ed.document.uri.fsPath}: Expected graphics namespace`
 				);
-			})
+			});
 		});
 	});
 
@@ -86,7 +86,7 @@ suite('Debugger', () => {
 					new RegExp('f <- function'),
 					`Unexpected editor contents for ${ed.document.uri.fsPath}`
 				);
-			})
+			});
 		});
 	});
 });

extensions/positron-r/src/test/discovery.test.ts

@@ -3,7 +3,7 @@
  *  Licensed under the Elastic License 2.0. See LICENSE.txt for license information.
  *--------------------------------------------------------------------------------------------*/
 
-import './mocha-setup'
+import './mocha-setup';
 
 import * as assert from 'assert';
 import * as Fs from "fs";

extensions/positron-r/src/test/hyperlink.test.ts

@@ -3,7 +3,7 @@
  *  Licensed under the Elastic License 2.0. See LICENSE.txt for license information.
  *--------------------------------------------------------------------------------------------*/
 
-import './mocha-setup'
+import './mocha-setup';
 
 import * as assert from 'assert';
 import { matchRunnable } from '../hyperlink';

extensions/positron-r/src/test/indentation.test.ts

@@ -3,7 +3,7 @@
  *  Licensed under the Elastic License 2.0. See LICENSE.txt for license information.
  *--------------------------------------------------------------------------------------------*/
 
-import './mocha-setup'
+import './mocha-setup';
 
 import * as vscode from 'vscode';
 import * as positron from 'positron';

extensions/positron-r/src/test/lsp.unit.test.ts

@@ -3,7 +3,7 @@
  *  Licensed under the Elastic License 2.0. See LICENSE.txt for license information.
  *--------------------------------------------------------------------------------------------*/
 
-import './mocha-setup'
+import './mocha-setup';
 
 import * as assert from 'assert';
 import * as testKit from './kit';
@@ -45,7 +45,7 @@ suite('Session manager', () => {
 			// The LSP of the first session eventually goes back online
 			testKit.pollForSuccess(() => {
 				assert.strictEqual(ses1Lsp.state, ArkLspState.Running);
-			})
+			});
 
 			// We would expect the following but currently we start the LSP client
 			// anew on each activation, so the event handler is no longer active.

extensions/positron-r/src/test/mocha-setup.ts

@@ -9,8 +9,10 @@ import * as testKit from './kit';
 export let currentTestName: string | undefined;
 
 suiteSetup(async () => {
-	// Set global Positron log level to trace for easier debugging
-	await vscode.commands.executeCommand('_extensionTests.setLogLevel', 'trace');
+	// Set global Positron log level to trace on CI for easier debugging
+	if (process.env.CI) {
+		await vscode.commands.executeCommand('_extensionTests.setLogLevel', 'trace');
+	}
 
 	// Set Ark kernel process log level to trace
 	await vscode.workspace.getConfiguration().update('positron.r.kernel.logLevel', 'trace', vscode.ConfigurationTarget.Global);