[cherry-pick] Fix: cross-file inline edit list never disposed (handleListEndOfLifetime not fired)#318434
Merged
Merged
Conversation
…ListEndOfLifetime not fired)
benibenj
approved these changes
May 26, 2026
osortega
approved these changes
May 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cherry-pick of #318340 from
main.Fixes #318341
Summary
InlineSuggestionListfor a cross-file inline edit was never disposed after the user accepted it, becauseInlineCompletionsModel.transplantCompletionheld one extra ref it never released. As a side effect, the extension-hosthandleListEndOfLifetimecallback never fired on the producing provider, and any end-of-life telemetry the extension sends from that callback was silently dropped for every cross-file inline edit.This PR removes the unpaired
addRefand adds a cheap always-on invariant so a refcount that goes negative is surfaced viaonUnexpectedError.The bug
Cross-file accept flow:
uripoints to a different document.InlineCompletionsModel.acceptopens the target editor viaICodeEditorService.openCodeEditor.transplantCompletion(item)to seed the item there.transplantCompletionwas:But
seedWithCompletionalready takes the necessary ref — it constructs a newInlineCompletionsState([item], …)whose ctor callsinlineCompletion.addRef()and pairs it withremoveRef()in its dispose. The explicititem.addRef()had no matchingremoveRef()anywhere, leaking one ref on every cross-file accept.InlineSuggestionList.removeRefonly callsprovider.disposeInlineCompletionswhen refCount reaches 0, so for cross-file edits it never did. On the extension-host side,MainThreadLanguageFeatures.disposeInlineCompletionsis the trigger forextHostLanguageFeatures.disposeCompletions, which in turn callsprovider.handleListEndOfLifetime— so all of that never ran for cross-file edits.This was caught while investigating missing
provideInlineEdittelemetry for cross-file NES inextensions/copilot, which depends onhandleListEndOfLifetimeto flush its per-suggestion builder.Investigation summary
This was caught by adding temporary instrumentation to
InlineSuggestionList:new Error().stackon everyaddRefglobalThis.__inlineCompletionListDump()hook returning per-list{ id, provider, requestUuid, refCount, ageMs, addRefStacks }setTimeoutwatchdog firingonUnexpectedErrorif a list remained alive past a thresholdRepro: edit
math.tsto add an exported symbol used byexample.ts, accept the resulting NES jump frommath.tstoexample.ts, then call__inlineCompletionListDump()from DevTools. Result: one leaked list per cross-file accept withrefCount=1indefinitely; the dump showed an unpairedaddRefstack pointing atInlineCompletionsModel.transplantCompletion.After the fix, the same repro shows
__inlineCompletionListDump()returning[]after the accept, and the extension's end-of-life telemetry fires as expected.The instrumentation itself is not in this PR — only the cheap negative-refCount invariant is kept for ongoing safety.
Changes
inlineCompletionsModel.ts: remove the unpaireditem.addRef()intransplantCompletion. Add a short comment explaining who already owns the ref (seedWithCompletion→new InlineCompletionsStatector).provideInlineCompletions.ts: inInlineSuggestionList.removeRef, fireonUnexpectedError(new BugIndicatingError(...))if the count goes negative. This catches the inverse class of mistake — aremoveRefwithout a matchingaddRef— at the same cost as the existing=== 0check.Risk
Very low. The removed line was redundant: tested manually against the original repro, and the new negative-refCount assertion did not fire during normal use (same-document and cross-file).
Manual test
example.tsimporting a non-existent symbol from./math.math.ts, add the missing export so NES suggests a cross-file edit.handleListEndOfLifetimefires (e.g. by logging in the extension or watching its telemetry).Notes for reviewers
transplantCompletionwas introduced in Implements https://github.com/microsoft/vscode/issues/271133 #290036 by @hediet (commit37ac613). Theitem.addRef()predates this fix and has been the source of the leak since that commit.InlineCompletionsStatenot take the ref sotransplantCompletion's explicitaddRefwould become correct, butInlineCompletionsStateis used from many call sites and its current semantics — "the state owns refs on the items it holds" — is the cleaner invariant. The fix keeps that and drops the redundant ref in the one caller that double-counted.transplantCompletionwould be useful but requires additional test plumbing (second model + mockedICodeEditorService.openCodeEditor); leaving that as a follow-up.