Skip to content

Refactor kernel management to derive status from runtime session and … #10001

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Refactor kernel management to derive status from runtime session and … #10001

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9da7d62
Refactor kernel management to derive status from runtime session and …
nstrayer Oct 16, 2025
fb44178
Extract mapping to status to a map object
nstrayer Oct 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 52 additions & 24 deletions src/vs/workbench/contrib/positronNotebook/browser/PositronNotebookInstance.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,9 @@ import { IPositronNotebookInstance, KernelStatus } from './IPositronNotebookInst
import { NotebookCellTextModel } from '../../notebook/common/model/notebookCellTextModel.js';
import { ICommandService } from '../../../../platform/commands/common/commands.js';
import { SELECT_KERNEL_ID_POSITRON, SelectPositronNotebookKernelContext } from './SelectPositronNotebookKernelAction.js';
import { INotebookKernel, INotebookKernelService } from '../../notebook/common/notebookKernelService.js';
import { INotebookKernelService } from '../../notebook/common/notebookKernelService.js';
import { IRuntimeSessionService } from '../../../services/runtimeSession/common/runtimeSessionService.js';
import { RuntimeState } from '../../../services/languageRuntime/common/languageRuntimeService.js';
import { isEqual } from '../../../../base/common/resources.js';
import { IPositronWebviewPreloadService } from '../../../services/positronWebviewPreloads/browser/positronWebviewPreloadService.js';
import { autorun, observableValue } from '../../../../base/common/observable.js';
Expand All @@ -44,6 +45,36 @@ interface IPositronNotebookInstanceRequiredTextModel extends IPositronNotebookIn
textModel: NotebookTextModel;
}

/**
* Maps runtime states to their corresponding kernel status values. This mapping
* defines how the notebook UI interprets runtime session states and displays
* them to the user as kernel connection status. As we expand the states we
* report this map will evolve.
*
* States are grouped by their semantic meaning:
* - Connecting: Runtime is initializing or starting
* - Connected: Runtime is operational and ready to execute code
* - Disconnected: Runtime has ended or is shutting down
*/
const RUNTIME_STATE_TO_KERNEL_STATUS: Partial<Record<RuntimeState, KernelStatus>> = {
// Runtime is starting up
[RuntimeState.Uninitialized]: KernelStatus.Connecting,
[RuntimeState.Initializing]: KernelStatus.Connecting,
[RuntimeState.Starting]: KernelStatus.Connecting,
[RuntimeState.Restarting]: KernelStatus.Connecting,

// Runtime is operational
[RuntimeState.Ready]: KernelStatus.Connected,
[RuntimeState.Idle]: KernelStatus.Connected,
[RuntimeState.Busy]: KernelStatus.Connected,
[RuntimeState.Interrupting]: KernelStatus.Connected,

// Runtime is shutting down or ended
[RuntimeState.Exiting]: KernelStatus.Disconnected,
[RuntimeState.Exited]: KernelStatus.Disconnected,
[RuntimeState.Offline]: KernelStatus.Disconnected,
} as const;

/**
* Implementation of IPositronNotebookInstance that handles the core notebook functionality
* and state management. This class serves as the bridge between the UI and the underlying
Expand Down Expand Up @@ -214,11 +245,6 @@ export class PositronNotebookInstance extends Disposable implements IPositronNot
selectionStateMachine;
contextManager;

/**
* Selected kernel for the notebook.
*/
kernel;

/**
* Status of kernel for the notebook.
*/
Expand Down Expand Up @@ -337,29 +363,29 @@ export class PositronNotebookInstance extends Disposable implements IPositronNot
d.dispose();
this.runtimeSession.set(undefined, undefined);
}));
// Also update the observable when runtime state changes
// This ensures derived observables like kernelStatus stay in sync
this._register(session.onDidChangeRuntimeState(() => {
Copy link

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This subscription is registered on the instance instead of the per-session DisposableStore, so it won't be disposed when the session changes. This can lead to stale callbacks updating runtimeSession from an old session and a potential leak. Add this listener to the same per-session DisposableStore (d) used above.

Suggested change
this._register(session.onDidChangeRuntimeState(() => {
d.add(session.onDidChangeRuntimeState(() => {

Copilot uses AI. Check for mistakes.

this.runtimeSession.set(session, undefined);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be better to directly kernelStatus.set here, rather than mapping from runtimeSession and triggering updates here when the session hasn't actually changed.

}));
}
}));

// Track the current selected kernel for this notebook
this.kernel = observableValue<INotebookKernel | undefined>('positronNotebookKernel', undefined);
this._register(this.notebookKernelService.onDidChangeSelectedNotebooks(e => {
if (e && this._isThisNotebook(e.notebook) && this.textModel) {
const matching = this.notebookKernelService.getMatchingKernel(this.textModel);
const kernel = matching.all.find(k => k.id === e.newKernel);
this.kernel.set(kernel, undefined);
// Derive the kernel connection status from the runtime session state
// Uses the RUNTIME_STATE_TO_KERNEL_STATUS map for the conversion
this.kernelStatus = this.runtimeSession.map(session => {
if (!session) {
return KernelStatus.Uninitialized;
}
}));

// Derive the kernel connection status
this.kernelStatus = this.kernel.map(
this,
kernel => /** @description positronNotebookKernelStatus */ kernel ? KernelStatus.Connected : KernelStatus.Disconnected
);
const runtimeState = session.getRuntimeState();
// Return the mapped status, or Errored if the runtime state is unknown
return RUNTIME_STATE_TO_KERNEL_STATUS[runtimeState] ?? KernelStatus.Errored;
});
Comment on lines +376 to +384
Copy link

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously the observable mapping included a @description annotation (e.g., /** @description positronNotebookKernelStatus */) which helps with reactive tracing/debugging. Please retain that description on this derived observable.

Copilot uses AI. Check for mistakes.


// Derive the notebook language from the selected kernel
this._language = this.kernel.map(
this,
kernel => /** @description positronNotebookLanguage */ kernel?.supportedLanguages[0] ?? 'plaintext'
// Derive the notebook language from the runtime session
this._language = this.runtimeSession.map(
session => /** @description positronNotebookLanguage */ session?.runtimeMetadata.languageId ?? 'plaintext'
);

this.contextManager = this._register(
Expand Down Expand Up @@ -800,13 +826,15 @@ export class PositronNotebookInstance extends Disposable implements IPositronNot
* fully determine the view we see.
*/
getEditorViewState(): INotebookEditorViewState {
this._assertTextModel();
const selectedKernel = this.notebookKernelService.getSelectedOrSuggestedKernel(this.textModel);
return {
editingCells: {},
cellLineNumberStates: {},
editorViewStates: {},
collapsedInputCells: {},
collapsedOutputCells: {},
selectedKernelId: this.kernel.get()?.id,
selectedKernelId: selectedKernel?.id,
};
}

Expand Down