Skip to content

[cherry-pick] Fix: cross-file inline edit list never disposed (handleListEndOfLifetime not fired)#318434

Merged
ulugbekna merged 1 commit into
release/1.122from
cherry-pick/318340
May 27, 2026
Merged

[cherry-pick] Fix: cross-file inline edit list never disposed (handleListEndOfLifetime not fired)#318434
ulugbekna merged 1 commit into
release/1.122from
cherry-pick/318340

Conversation

@vs-code-engineering
Copy link
Copy Markdown
Contributor

Cherry-pick of #318340 from main.

Fixes #318341

Summary

InlineSuggestionList for a cross-file inline edit was never disposed after the user accepted it, because InlineCompletionsModel.transplantCompletion held one extra ref it never released. As a side effect, the extension-host handleListEndOfLifetime callback 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 addRef and adds a cheap always-on invariant so a refcount that goes negative is surfaced via onUnexpectedError.

The bug

Cross-file accept flow:

  1. User accepts an inline edit whose uri points to a different document.
  2. InlineCompletionsModel.accept opens the target editor via ICodeEditorService.openCodeEditor.
  3. It calls the target model's transplantCompletion(item) to seed the item there.

transplantCompletion was:

public transplantCompletion(item: InlineSuggestionItem): void {
    item.addRef();                                  // ← unpaired
    transaction(tx => {
        this._source.seedWithCompletion(item, tx);  // already takes its own ref
        this._isActive.set(true, tx);
        this._inAcceptFlow.set(true, tx);
        this.dontRefetchSignal.trigger(tx);
    });
}

But seedWithCompletion already takes the necessary ref — it constructs a new InlineCompletionsState([item], …) whose ctor calls inlineCompletion.addRef() and pairs it with removeRef() in its dispose. The explicit item.addRef() had no matching removeRef() anywhere, leaking one ref on every cross-file accept.

InlineSuggestionList.removeRef only calls provider.disposeInlineCompletions when refCount reaches 0, so for cross-file edits it never did. On the extension-host side, MainThreadLanguageFeatures.disposeInlineCompletions is the trigger for extHostLanguageFeatures.disposeCompletions, which in turn calls provider.handleListEndOfLifetime — so all of that never ran for cross-file edits.

This was caught while investigating missing provideInlineEdit telemetry for cross-file NES in extensions/copilot, which depends on handleListEndOfLifetime to flush its per-suggestion builder.

Investigation summary

This was caught by adding temporary instrumentation to InlineSuggestionList:

  • per-instance id + alive-list registry
  • captured new Error().stack on every addRef
  • a globalThis.__inlineCompletionListDump() hook returning per-list { id, provider, requestUuid, refCount, ageMs, addRefStacks }
  • a setTimeout watchdog firing onUnexpectedError if a list remained alive past a threshold

Repro: edit math.ts to add an exported symbol used by example.ts, accept the resulting NES jump from math.ts to example.ts, then call __inlineCompletionListDump() from DevTools. Result: one leaked list per cross-file accept with refCount=1 indefinitely; the dump showed an unpaired addRef stack pointing at InlineCompletionsModel.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 unpaired item.addRef() in transplantCompletion. Add a short comment explaining who already owns the ref (seedWithCompletionnew InlineCompletionsState ctor).
  • provideInlineCompletions.ts: in InlineSuggestionList.removeRef, fire onUnexpectedError(new BugIndicatingError(...)) if the count goes negative. This catches the inverse class of mistake — a removeRef without a matching addRef — at the same cost as the existing === 0 check.

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

  • Open a file example.ts importing a non-existent symbol from ./math.
  • In math.ts, add the missing export so NES suggests a cross-file edit.
  • Accept the NES.
  • Trigger any additional inline edits to give the model a chance to dispose stale state.
  • Confirm extension's handleListEndOfLifetime fires (e.g. by logging in the extension or watching its telemetry).

Notes for reviewers

  • transplantCompletion was introduced in Implements https://github.com/microsoft/vscode/issues/271133 #290036 by @hediet (commit 37ac613). The item.addRef() predates this fix and has been the source of the leak since that commit.
  • I considered making InlineCompletionsState not take the ref so transplantCompletion's explicit addRef would become correct, but InlineCompletionsState is 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.
  • An end-to-end unit test that exercises transplantCompletion would be useful but requires additional test plumbing (second model + mocked ICodeEditorService.openCodeEditor); leaving that as a follow-up.

Copilot AI review requested due to automatic review settings May 26, 2026 20:59
@vs-code-engineering vs-code-engineering Bot added the cherry-pick-artifact Auto-generated cherry-pick PR label May 26, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@ulugbekna ulugbekna merged commit 50f4deb into release/1.122 May 27, 2026
25 checks passed
@ulugbekna ulugbekna deleted the cherry-pick/318340 branch May 27, 2026 07:50
@vs-code-engineering vs-code-engineering Bot added this to the 1.122.0 milestone May 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cherry-pick-artifact Auto-generated cherry-pick PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants