Skip to content

Fix: dispatch interactive .kql queries to currently selected controller#142

Open
pjanowski wants to merge 1 commit into
microsoft:mainfrom
pjanowski:fix/interactive-uses-selected-controller
Open

Fix: dispatch interactive .kql queries to currently selected controller#142
pjanowski wants to merge 1 commit into
microsoft:mainfrom
pjanowski:fix/interactive-uses-selected-controller

Conversation

@pjanowski
Copy link
Copy Markdown

Fix: dispatch interactive .kql queries to currently selected controller

Fixes #141

Summary

kusto.executeSelectedQuery (run from a .kql file via Shift+Enter) currently dispatches to the controller cached at the time the interactive window was first opened. Changing the kernel via the interactive window's picker has no effect on subsequent runs from the source .kql — they continue to execute against the original connection.

There are actually two cache layers that need to know about the kernel switch; fixing only one is not enough. This PR addresses both.

What changed

1. src/extension/kernel/provider.ts — track selected controller per notebook.

  • Added module-level selectedControllerByNotebook: WeakMap<NotebookDocument, KernelPerConnection> and an exported getSelectedController(notebook) lookup.
  • Extended the existing onDidChangeSelectedNotebooks handler in KernelPerConnection to set/clear the map. Clearing is guarded so an A→B switch where B's select fires before A's deselect doesn't accidentally drop B's entry.

2. src/extension/interactive/interactive.ts — use the selected controller at execute time.

  • At execute time, prefer getSelectedController(notebook) over the cached controller; fall back to the cached one if the selection event hasn't landed yet.

3. src/extension/kusto/client.ts — drop stale client cache when connection changes.

  • Client.create previously returned the cached Client for the document regardless of the connectionInfo argument. After (1) + (2), executeCell correctly calls Client.create(textDocument, B's connection) — but the cached client (built with A's connection) was still returned.
  • Now: when the caller passes a connectionInfo that doesn't match the cached client's connectionInfo (lodash isEqual), drop the stale entry and recreate. Caching is preserved when nothing changed.

Total: 3 files, ~30 insertions, ~3 deletions. No persistence-format changes, one new public export (getSelectedController).

Repro (before this PR)

  1. Open any .kql file. Bind it to cluster A / db A via the $(database) icon.
  2. Shift+Enter on a query block — interactive window opens executing against A.
  3. In the interactive window kernel dropdown, pick cluster B / db B.
  4. Back in the .kql file, Shift+Enter again on the same block.

Before this PR: query still runs against A (the kernel chip says B; execution silently routes to A).
After this PR: query runs against B as expected.

Manual verification

Built as a .vsix, installed alongside the marketplace 0.5.4 build, validated against a real Kusto cluster (mspva01.westus2.kusto.windows.net / pvananalytics) and a deliberately-wrong cluster (fdislandsus.centralus.kusto.windows.net / CAPAnalytics, which lacks the queried function).

  • ✅ Switch kernel from correct → wrong cluster in the interactive window picker: next Shift+Enter from the .kql fails with the expected "Unknown function" error against the wrong cluster (no longer silently routing to the original cluster).
  • ✅ Switch kernel from wrong → correct: query succeeds.
  • ✅ Single-controller happy path (no kernel switch): unchanged.
  • .knb notebook path: unchanged — the notebook still binds its kernel via its embedded metadata.

Why this is safe

  • onDidChangeSelectedNotebooks already fires on the initial controller assignment when interactive.open is called with a specific controller id, so the WeakMap is populated by the time the first executeInteractive would run.
  • The ?? cachedController fallback in interactive.ts preserves current behavior in the pathological case where the selection event has not fired yet.
  • The deselect handler only clears if the entry still points to this, which handles both A→B and B→A switch orderings VS Code may produce.
  • The Client.create change only changes behavior when the caller explicitly passes a connectionInfo that differs from the cached client's — when no connectionInfo is passed (the ensureDocumentHasConnectionInfo fallback path), behavior is unchanged.
  • WeakMap keys are notebook documents / text documents, so closed-but-cached entries are GC'd without leaking.

Notes for reviewers

  • The pre-existing connection reference (no this.) in the last line of the onDidChangeSelectedNotebooks handler was left as-is to minimize the diff.
  • Connection equality uses lodash isEqual to handle the discriminated IConnectionInfo union (azAuth vs appInsights) without special-casing fields. lodash is already a transitive dep.
  • A related but separate scenario — changing the .kql per-file connection via the editor title-bar $(database) icon (not the kernel picker) — also doesn't fully take effect on an already-open interactive window. The cached controller in interactive.ts's documentInteractiveDocuments is unchanged by onDidChangeConnection. This PR does not address that; it's a follow-up.
  • No new tests added: the existing test suite doesn't cover this code path, and adding one would require mocking VS Code's notebook controller selection events. Open to suggestions if there's an existing pattern in the repo for testing this.
  • Build note: npm run compile doesn't include the server bundle (src/server/); npm run compile:webpack does. Mentioning in case a CI script needs updating.

When a query is launched from a .kql file via kusto.executeSelectedQuery,
the controller used to execute it is currently the one cached at the time
the interactive window was first opened. Changing the kernel via the
interactive window's picker updates the UI and globalState but does not
affect subsequent runs from the source .kql -- they keep routing through
the cached (now stale) controller, AND the per-document Kusto client cache
in Client.create also returns a stale client built for the original
connection, even if the caller now passes a different connection.

Two-part fix:

* Track the currently-selected controller per notebook via the existing
  onDidChangeSelectedNotebooks signal, expose getSelectedController(),
  and prefer it over the cached controller in executeSelectedQuery.

* In Client.create, when the caller passes a connectionInfo that doesn't
  match the cached client's connectionInfo, drop the stale entry and
  recreate. Preserves caching when nothing changed.
@pjanowski
Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree company="Microsoft"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Interactive window kernel picker has no effect on queries re-run from a .kql file

1 participant