Skip to content

Fix suggest model triggering on disposed inline completions model#320077

Merged
ulugbekna merged 1 commit into
mainfrom
ulugbekna/fix-suggest-disposed-inline-completions-wait
Jun 5, 2026
Merged

Fix suggest model triggering on disposed inline completions model#320077
ulugbekna merged 1 commit into
mainfrom
ulugbekna/fix-suggest-disposed-inline-completions-wait

Conversation

@ulugbekna
Copy link
Copy Markdown
Contributor

Problem

The wait inside SuggestModel._waitForInlineCompletionsAndTrigger snapshotted inlineController.model.get() and then subscribed long-term to that snapshot's state / status derived observables.

Those derived observables are owned only for debug — they are not registered to the InlineCompletionsModel._store. The model itself is produced by a derivedDisposable in InlineCompletionsController and is disposed and recreated whenever its dependencies change:

  • the editor's text model is replaced (peek opens, diff editor mode switch, programmatic editor.setModel(...), etc.)
  • readOnly is toggled
  • the controller is disposed (editor disposal)

After the model is disposed, its state / status derived keep observing live external dependencies (_editorObs.versionId, config observables, …) and continue to fire. SuggestModel does not call cancel() on onDidChangeConfiguration, so a readOnly toggle (concrete reproducer) leaves the wait running against a dead model:

  • the unconditional 750 ms timeout calls inlineModel.stop('automatic') on a disposed instance (mutates _isActive on a dead model);
  • the autorun fires triggerAndCleanUp(true) and can spuriously call this.trigger({ auto: true }).

Fix

Read inlineController.model as the first dependency of a single autorun. If the value differs from the snapshot we got at the start of the wait, the controller has swapped or disposed the model — bail out before any read on state / status.

A single autorun (rather than a nested outer/inner pair) is deliberate: independent autoruns share no run-ordering guarantee, so an inner state watcher could otherwise run on a disposed model before the outer model watcher cleaned it up.

Test

Added a regression test that:

  1. registers a hanging inline completions provider that only resolves on cancellation,
  2. types a character so the wait is set up,
  3. toggles readOnly: true mid-wait (disposes the model),
  4. advances past the 750 ms timeout and asserts that quick suggest is not fired.

Verified the test fails on the original code and passes on the fix.

The wait inside \`_waitForInlineCompletionsAndTrigger\` snapshotted
\`inlineController.model.get()\` and then subscribed long-term to the
snapshot's \`state\`/\`status\` derived observables. Those derived are
owned only for debug — they are not registered to the model's
\`_store\`, so after the controller swaps or disposes the model
(text model replaced, readOnly toggled, controller disposed) they
keep observing live external dependencies and continue to fire.
That could cause the 750ms timeout to call \`stop()\` on a disposed
model and the autorun to trigger quick suggest spuriously.

Read \`inlineController.model\` as the first dependency of a single
autorun; if it differs from the snapshot, bail out before touching
\`state\`/\`status\`. A single autorun avoids the nested-autorun
ordering hazard (no defined run order between independent autoruns
that share a dependency).
Copilot AI review requested due to automatic review settings June 5, 2026 11:10
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.

Pull request overview

Fixes a race in SuggestModel._waitForInlineCompletionsAndTrigger where the wait could keep observing InlineCompletionsModel-owned derived observables after the inline model had been disposed/recreated (e.g. due to readOnly toggling), potentially leading to stop()/auto-trigger being applied to a dead inline model.

Changes:

  • Bind the inline-completions wait to the current inline model by reading inlineController.model first within a single autorun, and bailing out when the model instance changes.
  • Add a regression test that disposes the inline model mid-wait by toggling readOnly, then verifies quick suggest is not triggered after the 750ms timeout window.
Show a summary per file
File Description
src/vs/editor/contrib/suggest/browser/suggestModel.ts Ensures the inline-completions wait observes the controller’s current model instance and cleans up when it changes, avoiding actions on disposed models.
src/vs/editor/contrib/suggest/test/browser/suggestModel.test.ts Adds coverage for the mid-wait inline model disposal scenario (via readOnly toggle) to prevent regressions.

Copilot's findings

  • Files reviewed: 2/2 changed files
  • Comments generated: 0

@ulugbekna ulugbekna merged commit 95b4def into main Jun 5, 2026
26 checks passed
@ulugbekna ulugbekna deleted the ulugbekna/fix-suggest-disposed-inline-completions-wait branch June 5, 2026 11:52
@vs-code-engineering vs-code-engineering Bot added this to the 1.124.0 milestone Jun 5, 2026
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.

3 participants