Skip to content

Commit

Permalink
Fix promise handling when no session manager manages a runtime (#5618)
Browse files Browse the repository at this point in the history 
Attempts to address #5615.

I noticed that in Positron 2024.12.0-96 (the last version **without**
this bug), the logs still show the "No session manager found" error the
first time the R runtime is started, and it only starts successfully the
second time:

<details>
<summary>Logs</summary>

```
2024-12-04 22:05:05.806 [debug] [Runtime startup] Phase changed to 'starting'
2024-12-04 22:05:05.807 [debug] [Runtime startup] Activating extension positron.positron-r for language ID r
2024-12-04 22:05:05.807 [debug] [Runtime startup] Activating extension ms-python.python for language ID python
2024-12-04 22:05:05.989 [info] Language runtime a4d614fd43bbc66862ddf04eb847956e (language: R name: R 4.3.2 version: 4.3.2) automatically starting. Source: Affiliated r runtime for workspace
2024-12-04 22:05:05.989 [info] Language runtime dc77b4b8d64602dadd49a9e5455e6925 (language: Python name: Python 3.9.18 (Venv: .venv) version: 3.9.18) automatically starting. Source: Affiliated python runtime for workspace
2024-12-04 22:05:05.989 [debug] [Runtime startup] Activating extension ms-python.python for language ID python
2024-12-04 22:05:05.989 [debug] [Runtime startup] Activating extension positron.positron-r for language ID r
2024-12-04 22:05:05.989 [debug] [Runtime startup] Activating extension vscode.positron-reticulate for language ID reticulate
2024-12-04 22:05:06.004 [debug] [Ext host 0] Runtime manager for 'R 4.3.2': false
2024-12-04 22:05:06.004 [debug] [Ext host 0] Runtime manager for 'Python 3.9.18 (Venv: .venv)': true
2024-12-04 22:05:06.004 [error] No session manager found for runtime a4d614fd43bbc66862ddf04eb847956e (language: R name: R 4.3.2 version: 4.3.2) (1 managers registered).: Error: No session manager found for runtime a4d614fd43bbc66862ddf04eb847956e (language: R name: R 4.3.2 version: 4.3.2) (1 managers registered).
    at $Opc.X (vscode-file://vscode-app/Applications/Positron.app/Contents/Resources/app/out/vs/workbench/workbench.desktop.main.js:675547:19)
    at async $Opc.S (vscode-file://vscode-app/Applications/Positron.app/Contents/Resources/app/out/vs/workbench/workbench.desktop.main.js:675369:36)
2024-12-04 22:05:06.005 [debug] [Runtime startup] All extensions contributing language runtimes have been activated.
2024-12-04 22:05:06.005 [debug] [Runtime startup] Phase changed to 'discovering'
2024-12-04 22:05:06.010 [debug] [Registering Log Channel] File does not exist. Waiting for 1s to retry. file:///Users/seem/Library/Application%20Support/Positron/logs/20241204T220501/window1/exthost/vscode.positron-notebook-controllers/Positron%20Notebook%20Controllers.log
2024-12-04 22:05:06.010 [debug] [Registering Log Channel] File does not exist. Waiting for 1s to retry. file:///Users/seem/Library/Application%20Support/Positron/logs/20241204T220501/window1/exthost/positron.positron-r/Positron%20R%20Extension.log
2024-12-04 22:05:06.010 [debug] [Registering Log Channel] File does not exist. Waiting for 1s to retry. file:///Users/seem/Library/Application%20Support/Positron/logs/20241204T220501/window1/exthost/vscode.positron-run-app/Positron%20Run%20App.log
2024-12-04 22:05:06.020 [debug] [Ext host 0] Runtime manager for 'Python 3.9.18 (Venv: .venv)': true
2024-12-04 22:05:06.034 [debug] [Registering Log Channel] File does not exist. Waiting for 1s to retry. file:///Users/seem/Library/Application%20Support/Positron/logs/20241204T220501/window1/exthost/vscode.jupyter-adapter/Jupyter%20Adapter.log
2024-12-04 22:05:06.034 [debug] [Registering Log Channel] File does not exist. Waiting for 1s to retry. file:///Users/seem/Library/Application%20Support/Positron/logs/20241204T220501/window1/exthost/vscodevim.vim/Vim.log
2024-12-04 22:05:06.034 [debug] [Registering Log Channel] File does not exist. Waiting for 1s to retry. file:///Users/seem/Library/Application%20Support/Positron/logs/20241204T220501/window1/exthost/vscode.positron-supervisor/Positron%20Kernel%20Supervisor.log
2024-12-04 22:05:06.076 [warning] Ignoring call to requestRefresh; client is not available.
2024-12-04 22:05:06.256 [info] [perf] Render performance baseline is 15ms
2024-12-04 22:05:06.509 [info] Starting session for language runtime a4d614fd43bbc66862ddf04eb847956e (language: R name: R 4.3.2 version: 4.3.2) (Source: Affiliated runtime for workspace)
2024-12-04 22:05:06.509 [debug] [Ext host 0] Runtime manager for 'R 4.3.2': true
```

</details>

This might suggest another upstream issue that could've been around for
a while.

However, #5615 added "batching" of multiple calls to `autoStartRuntime`,
but we weren't rejecting the underlying deferred promise and clearing it
from the session maps when the "No session manager found" error was
encountered. We actually also weren't handling this error case in other
start methods but those didn't seem to have caused any issues in
practice.

### QA Notes

I added a unit test for this issue. I'm also building a local release
since I couldn't repro this in a dev build.
  • Loading branch information
@seeM
seeM authored Dec 4, 2024
1 parent af4fafc commit 1b6be26
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 3 deletions.
34 changes: 31 additions & 3 deletions src/vs/workbench/services/runtimeSession/common/runtimeSession.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -485,11 +485,21 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession
throw new Error(`No session manager has been registered.`);
}

// Get the runtime's manager.
let sessionManager: ILanguageRuntimeSessionManager;
try {
sessionManager = await this.getManagerForRuntime(runtimeMetadata);
} catch (err) {
startPromise.error(err);
this.clearStartingSessionMaps(
sessionMetadata.sessionMode, runtimeMetadata, sessionMetadata.notebookUri);
throw err;
}

// Restore the session. This can take some time; it may involve waiting
// for the extension to finish activating and the network to attempt to
// reconnect, etc.
let session: ILanguageRuntimeSession;
const sessionManager = await this.getManagerForRuntime(runtimeMetadata);
try {
session = await sessionManager.restoreSession(runtimeMetadata, sessionMetadata);
} catch (err) {
Expand Down Expand Up @@ -719,11 +729,20 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession
// at the debug level since we still expect the error to be handled/logged elsewhere.
startPromise.p.catch(err => this._logService.debug(`Error starting runtime session: ${err}`));

// Get the runtime's manager.
let sessionManager: ILanguageRuntimeSessionManager;
try {
sessionManager = await this.getManagerForRuntime(metadata);
} catch (err) {
startPromise.error(err);
this.clearStartingSessionMaps(sessionMode, metadata, notebookUri);
throw err;
}

// Check to see if the runtime has already been registered with the
// language runtime service.
const languageRuntime =
this._languageRuntimeService.getRegisteredRuntime(metadata.runtimeId);
const sessionManager = await this.getManagerForRuntime(metadata);

// If it has not been registered, validate the metadata.
if (!languageRuntime) {
Expand Down Expand Up @@ -813,7 +832,16 @@ export class RuntimeSessionService extends Disposable implements IRuntimeSession
startPromise.p.catch(err => this._logService.debug(`Error starting runtime session: ${err}`));
}

const sessionManager = await this.getManagerForRuntime(runtimeMetadata);
// Get the runtime's manager.
let sessionManager: ILanguageRuntimeSessionManager;
try {
sessionManager = await this.getManagerForRuntime(runtimeMetadata);
} catch (err) {
startPromise.error(err);
this.clearStartingSessionMaps(sessionMode, runtimeMetadata, notebookUri);
throw err;
}

const sessionId = this.generateNewSessionId(runtimeMetadata);
const sessionMetadata: IRuntimeSessionMetadata = {
sessionId,
Expand Down
16 changes: 16 additions & 0 deletions src/vs/workbench/services/runtimeSession/test/common/runtimeSession.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -498,6 +498,22 @@ suite('Positron - RuntimeSessionService', () => {

assertSingleSessionIsStarting(result1);
});

if (mode === LanguageRuntimeSessionMode.Console) {
test(`${action} concurrently with no session manager for runtime (#5615)`, async () => {
sinon.stub(manager, 'managesRuntime').resolves(false);

// Start twice concurrently.
const promise1 = start();
const promise2 = start();

// Both promises should reject.
// This was not previously the case since the second call returns a deferred
// promise that does not necessarily resolve/reject with the first call.
await assert.rejects(promise1);
await assert.rejects(promise2);
});
}
}

if (startNotebook) {
Expand Down

0 comments on commit 1b6be26

Please sign in to comment.