Preserve IntelliSense results across slow binding operations - #2779
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves IntelliSense completion robustness by distinguishing “slow” binding operations from true timeouts, adding a completion-specific hard deadline, and preventing completions from returning stale or misleading fallback results when binding is blocked or times out.
Changes:
- Introduces separate slow-threshold vs hard-timeout behavior in the binding queue, including correlation IDs and richer timing logs.
- Updates completion and ParseAndBind flows to stop/return failure when required reparse/binding cannot complete (lock-wait timeout or hard timeout), preventing stale parse state usage.
- Adds/updates unit and integration tests to cover slow operations, hard timeouts, lock-wait timeouts, late results, and stale-parse scenarios.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Microsoft.SqlTools.ServiceLayer.UnitTests/LanguageServer/LanguageServiceTests.cs | Adds regression covering completion + reparse behavior when binding lock cannot be acquired (stale-parse protection). |
| test/Microsoft.SqlTools.ServiceLayer.UnitTests/LanguageServer/CompletionServiceTest.cs | Adds tests for slow vs hard completion timeout behavior and expected failure semantics. |
| test/Microsoft.SqlTools.ServiceLayer.IntegrationTests/LanguageServer/PeekDefinitionTests.cs | Updates mock signature to match new QueueBindingOperation parameter list. |
| test/Microsoft.SqlTools.LanguageService.UnitTests/LanguageServices/BindingQueueTests.cs | Adds deterministic tests for lock-wait timeout, slow-threshold completion, and hard-timeout lock retention behavior. |
| src/Microsoft.SqlTools.LanguageService/LanguageServices/TSqlLanguageService.cs | Stops completion when required reparse/bind fails and avoids exposing stale parse state as valid. |
| src/Microsoft.SqlTools.LanguageService/LanguageServices/QueueItem.cs | Adds queue item correlation ID, lifetime stopwatch, and execution/timeout state tracking. |
| src/Microsoft.SqlTools.LanguageService/LanguageServices/ConnectedBindingQueue.cs | Extends QueueBindingOperation API with optional hard timeout and clarifies BindingTimeout semantics. |
| src/Microsoft.SqlTools.LanguageService/LanguageServices/Completion/CompletionService.cs | Adds completion hard-timeout support and returns failure on queue timeouts instead of fallback “success”. |
| src/Microsoft.SqlTools.LanguageService/LanguageServices/BindingQueue.cs | Implements slow-threshold vs hard-timeout handling; retains binding lock across hard timeouts until the underlying operation actually exits. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (slowWaitInMs == bindTimeoutInMs) | ||
| { | ||
| Logger.Warning($"Binding queue item {queueItem.Id} exceeded the {bindTimeoutInMs} ms slow-operation threshold at {queueItem.Lifetime.ElapsedMilliseconds} ms"); | ||
| } |
| AutoCompletionResult result = completionService.CreateCompletions( | ||
| connectionInfo, | ||
| docInfo, | ||
| useLowerCaseSuggestions: true); | ||
|
|
||
| Assert.That(operationStarted.WaitOne(0), Is.True); | ||
| Assert.That(result, Is.Null, "A hard timeout must be returned as a failure, not a fallback success."); | ||
| Assert.That(operationFinished.WaitOne(0), Is.False, | ||
| "The caller should return before the non-cooperative parser operation finishes."); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/Microsoft.SqlTools.LanguageService/LanguageServices/BindingQueue.cs:437
- The warning about exceeding the "slow-operation threshold" is currently emitted whenever
slowWaitInMs == bindTimeoutInMs, which also includes the case wherehardTimeoutInMs == bindTimeoutInMs(i.e., there is no separate hard timeout). In that case this is not a slow-threshold crossing but the actual hard timeout, so the log is misleading and can confuse timeout investigations.
if (slowWaitInMs == bindTimeoutInMs)
{
Logger.Warning($"Binding queue item {queueItem.Id} exceeded the {bindTimeoutInMs} ms slow-operation threshold at {queueItem.Lifetime.ElapsedMilliseconds} ms");
}
src/Microsoft.SqlTools.LanguageService/LanguageServices/BindingQueue.cs:421
CancellationTokenSource cancelTokenis never disposed. With the new hard-timeout behavior the binding operation can keep running afterItemProcessedis signaled, so this CTS can live significantly longer and accumulate across requests, increasing memory/handle pressure. Consider disposing it once the binding task completes (regardless of success/timeout).
});
Task.Run(() =>
* Keep completion document locks short * Add completion lock timing logs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/Microsoft.SqlTools.LanguageService/LanguageServices/TSqlLanguageService.cs:552
HandleCompletionRequestcan throw when the request has a null/empty URI.RunLatestCompletionByUriAsynccalls the provided operation for whitespace URIs, but the operation immediately callsCurrentWorkspace.GetFile(uri), which validatesuriand throwsArgumentExceptionfor null/whitespace. This would fail the completion request instead of returningnullas intended.
string uri = textDocumentPosition?.TextDocument?.Uri;
CompletionItem[] completionItems = await RunLatestCompletionByUriAsync(
uri,
async cancellationToken =>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/Microsoft.SqlTools.LanguageService/LanguageServices/TSqlLanguageService.cs:558
- HandleCompletionRequest passes the raw Uri into CurrentWorkspace.GetFile(uri). Workspace.GetFile throws on null/empty/whitespace, and completion coordination keys should also normalize percent-encoded URIs (GetScriptParseInfo explicitly unescapes) to avoid treating
file:///c%3A/...andfile:///C:/...as different documents for cancellation/serialization.
string uri = textDocumentPosition?.TextDocument?.Uri;
CompletionItem[] completionItems = await RunLatestCompletionByUriAsync(
uri,
async cancellationToken =>
{
ScriptFile scriptFile = CurrentWorkspace.GetFile(uri);
if (scriptFile == null)
src/Microsoft.SqlTools.LanguageService/LanguageServices/BindingQueue.cs:485
- The per-item CancellationTokenSource is never disposed. Since the binding operation can hard-timeout and continue running, this can retain timer/registration resources longer than necessary. Consider disposing the CTS when bindTask completes (immediately if already completed, otherwise via a continuation).
if (lockTaken)
{
bindingContext.BindingLock.Set();
Logger.Verbose($"Binding queue item {queueItem.Id} released BindingLock after holding it for {bindingLockStopwatch.ElapsedMilliseconds} ms");
Description
Addresses microsoft/vscode-mssql#21930 and the verified binding-queue/SMO subcase of microsoft/vscode-mssql#22236.
Reproduction findings
We reproduced #21930 end to end using a SQL Server Docker database modeled on the reported scale: 57,885 tables, 783 views, and 470,826 columns. Generating approximately 36,900
dbosuggestions took 650–760 ms, so the queue selected its fallback at 500 ms and discarded the valid parser result produced shortly afterward. A smaller schema with 17,874 objects completed within 304–408 ms and succeeded.The result was unchanged after waiting for metadata initialization and with or without
VIEW DEFINITION. This identifies metadata volume crossing the fixed 500 ms boundary—notdbo, permissions, or rapid typing—as the reproduced cause.Scope caveat: the public #22236 report does not identify the original non-cooperative production SMO or parser call, so this PR should not by itself close the entire umbrella issue.
Validation performed:
Code Changes Checklist
dotnet test)Reviewers: Please read our reviewer guidelines