Fix SQL notebook IntelliSense loss on save and reconnect (#22124) - #22525
Conversation
Cell IntelliSense registrations now wait for the real connection/complete result instead of trusting the connect request acknowledgement, so failed cell binds are retried instead of silently memoized (saved notebooks previously ended up with keyword-only completions). Cell connects also use the live execution-connection credentials so they carry refreshed auth tokens and the actual database. Saving an untitled notebook replaces its NotebookDocument, which previously disposed the notebook's connection mid-save. The connection manager is now parked when a connected untitled notebook closes and adopted by the file-based notebook that replaces it (matched by cell content within the save-transition window, guarded by tab presence and open recency), covering close-then-open, open-then-close, and lingering-document event orderings. Adoption re-registers the new cell URIs with STS and refreshes UI state. Additional fixes from review: per-cell STS connections are released on notebook disconnect/close instead of leaking one server session per cell; stale cell connects that complete after a connection change are disconnected; completion expectations are identity-guarded against cross-cancellation between overlapping registrations; re-parking a reused untitled URI no longer leaks the superseded manager. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
This is a proposed fix for #22124 |
There was a problem hiding this comment.
Pull request overview
This PR hardens SQL notebook IntelliSense connection handling around save/reconnect flows by (1) waiting for real connection/complete outcomes for per-cell registrations, and (2) transferring/adopting notebook connection managers when untitled notebooks are saved and VS Code replaces the NotebookDocument.
Changes:
- Add untitled→file save adoption/transfer logic in
SqlNotebookController, including re-registering cell URIs and handling close/open event ordering. - Update
NotebookConnectionManagercell IntelliSense registration to awaitconnection/complete, release/disconnect per-cell STS connections, and handle stale completions across connection changes. - Add/extend unit tests covering save URI transfer, adoption flows, late cell document registration, and per-cell registration retry/release scenarios.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| extensions/mssql/test/unit/notebooks/sqlNotebookController.test.ts | Adds unit tests for save URI transfer/adoption and late cell document registration. |
| extensions/mssql/test/unit/notebooks/notebookConnectionManager.test.ts | Adds unit tests for connection/complete-based registration, retry on failure/timeout, and releaseCellRegistrations. |
| extensions/mssql/src/notebooks/sqlNotebookController.ts | Implements manager parking/adoption across untitled save replacement; re-registers cell URIs; registers opened cell docs for IntelliSense. |
| extensions/mssql/src/notebooks/notebookConnectionManager.ts | Awaits connection/complete, prefers live execution-connection credentials for cell binds, and disconnects released cell URIs from STS. |
| extensions/mssql/src/controllers/connectionManager.ts | Exposes expectConnectionComplete + cancellation to observe real connect outcomes for direct connection/connect callers. |
- Locate a cell's owning notebook via path-prefiltered lookup (verified membership, full-scan fallback) instead of scanning every cell of every notebook per cell-open event. - Build the notebook content signature incrementally so large notebooks no longer materialize their full concatenated cell text for a capped signature. - Best-effort disconnect a cell URI whose connect times out, so a late-completing STS connection doesn't linger untracked after the cell was deregistered. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PR Changes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #22525 +/- ##
===========================================
- Coverage 88.24% 75.89% -12.35%
===========================================
Files 415 407 -8
Lines 131028 131070 +42
Branches 8394 8447 +53
===========================================
- Hits 115622 99476 -16146
- Misses 15406 31594 +16188
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
This fixes the issue where saved notebooks don't get intellisense; but it doesn't resolve the issue where saving an untitled notebook loses the active connection. I'll fix that as a separate PR so that I can take this one, which fixes the core bug reported in the issue. The "auto-reconnect" issue can be worked-around by manually reconnecting after "save as". |
…2525) (#22553) * Fix SQL notebook IntelliSense loss on save and reconnect (#22124) Cell IntelliSense registrations now wait for the real connection/complete result instead of trusting the connect request acknowledgement, so failed cell binds are retried instead of silently memoized (saved notebooks previously ended up with keyword-only completions). Cell connects also use the live execution-connection credentials so they carry refreshed auth tokens and the actual database. Saving an untitled notebook replaces its NotebookDocument, which previously disposed the notebook's connection mid-save. The connection manager is now parked when a connected untitled notebook closes and adopted by the file-based notebook that replaces it (matched by cell content within the save-transition window, guarded by tab presence and open recency), covering close-then-open, open-then-close, and lingering-document event orderings. Adoption re-registers the new cell URIs with STS and refreshes UI state. Additional fixes from review: per-cell STS connections are released on notebook disconnect/close instead of leaking one server session per cell; stale cell connects that complete after a connection change are disconnected; completion expectations are identity-guarded against cross-cancellation between overlapping registrations; re-parking a reused untitled URI no longer leaks the superseded manager. * Address Copilot review feedback on notebook adoption paths - Locate a cell's owning notebook via path-prefiltered lookup (verified membership, full-scan fallback) instead of scanning every cell of every notebook per cell-open event. - Build the notebook content signature incrementally so large notebooks no longer materialize their full concatenated cell text for a capped signature. - Best-effort disconnect a cell URI whose connect times out, so a late-completing STS connection doesn't linger untracked after the cell was deregistered. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Cell IntelliSense registrations now wait for the real connection/complete result instead of trusting the connect request acknowledgement, so failed cell binds are retried instead of silently memoized (saved notebooks previously ended up with keyword-only completions). Cell connects also use the live execution-connection credentials so they carry refreshed auth tokens and the actual database.
Saving an untitled notebook replaces its NotebookDocument, which previously disposed the notebook's connection mid-save. The connection manager is now parked when a connected untitled notebook closes and adopted by the file-based notebook that replaces it (matched by cell content within the save-transition window, guarded by tab presence and open recency), covering close-then-open, open-then-close, and lingering-document event orderings. Adoption re-registers the new cell URIs with STS and refreshes UI state.
Additional fixes from review: per-cell STS connections are released on notebook disconnect/close instead of leaking one server session per cell; stale cell connects that complete after a connection change are disconnected; completion expectations are identity-guarded against cross-cancellation between overlapping registrations; re-parking a reused untitled URI no longer leaks the superseded manager.
Description
Provide a clear, concise summary of the changes in this PR. What problem does it solve? Why is it needed? Link any related issues using issue closing keywords.
Code Changes Checklist
npm run test)Reviewers: Please read our reviewer guidelines