Refactor kernel management to derive status from runtime session and … #10001
Conversation
…update language handling accordingly
|
E2E Tests 🚀 |
There was a problem hiding this comment.
Pull Request Overview
Refactor kernel status derivation to use the runtime session state so the UI accurately reflects connection status, especially when attaching to an already-running kernel.
- Replace kernel-based status derivation with a RuntimeState → KernelStatus mapping.
- Update observables to react to runtime state changes and derive language from runtime metadata.
- Adjust getEditorViewState to read the currently selected/suggested kernel directly from the kernel service.
| })); | ||
| // Also update the observable when runtime state changes | ||
| // This ensures derived observables like kernelStatus stay in sync | ||
| this._register(session.onDidChangeRuntimeState(() => { |
There was a problem hiding this comment.
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.
| this._register(session.onDidChangeRuntimeState(() => { | |
| d.add(session.onDidChangeRuntimeState(() => { |
Copilot uses AI. Check for mistakes.
| 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; | ||
| }); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Code changes LGTM! Feels much safer to rely on language runtimes than notebook kernels where possible.
There's a possibility that the notebook's selected kernel becomes out of sync with the notebook's language runtime, but I'm not sure how likely that actually is.
| // Also update the observable when runtime state changes | ||
| // This ensures derived observables like kernelStatus stay in sync | ||
| this._register(session.onDidChangeRuntimeState(() => { | ||
| this.runtimeSession.set(session, undefined); |
There was a problem hiding this comment.
Might be better to directly kernelStatus.set here, rather than mapping from runtimeSession and triggering updates here when the session hasn't actually changed.
2 participants
Addresses #9874.
This PR fixes an issue where notebooks couldn't connect to an already-running kernel of the same language version. The kernel was (most of the time) connected but because the kernel status badge was derived from the selected notebook kernel rather than the actual runtime session state the connection was never reflected in an updated status badge.
The fix refactors the kernel management logic to derive the status directly from the runtime session's state, ensuring the UI correctly reflects when a kernel is already connected and available.
Release Notes
New Features
Bug Fixes
QA Notes
@:positron-notebooks
It's hard to get into the exact state mentioned in the first issue but this is another path to test in addition