Fix multiple isort handlers registered due to concurrent server restarts by Copilot · Pull Request #541 · microsoft/vscode-isort

Copilot · 2026-02-25T17:34:37Z

🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

Co-authored-by: bschnurr <1946977+bschnurr@users.noreply.github.com>

rchiodo · 2026-02-25T18:02:53Z

src/extension.ts

+                context.subscriptions.push(sortFeatures);
+                await sortFeatures.startup();
            }
        } finally {


@copilot -78
[High] Subscription accumulation bug: Each call to registerSortImportFeatures() pushes to context.subscriptions, but repeated mode toggles will accumulate entries. Store sortFeatures in a module-level variable (like lsClient) and dispose it explicitly before re-registering:
let sortFeaturesDisposable: Disposable | undefined;
// ...
if (sortFeaturesDisposable) {
sortFeaturesDisposable.dispose();
}
sortFeaturesDisposable = registerSortImportFeatures(serverId);
context.subscriptions.push(sortFeaturesDisposable);

Both Skeptic and Architect flagged this independently.

Fixed in eb9e5f1. sortFeaturesDisposable is now a module-level variable typed as (vscode.Disposable & { startup: () => Promise<void> }) | undefined. Before each re-registration it's explicitly disposed, preventing the accumulation of stale entries in context.subscriptions.

rchiodo · 2026-02-25T18:03:02Z

src/extension.ts

-                        `[Option 2] Set an interpreter using "${serverId}.interpreter" setting.\r\n`,
-                    `Please use Python ${PYTHON_VERSION} or greater.`,
-                );
+            if (getServerEnabled(serverId)) {


@copilot -79
[Medium] Asymmetric cleanup patterns: Server mode uses explicit unRegisterSortImportFeatures() cleanup, while server-less mode relies on implicit subscription disposal. Consider unifying: call unRegisterSortImportFeatures() in BOTH branches before their respective setup, ensuring symmetric cleanup regardless of previous mode.

Done in cfdb13a. Both branches now call unRegisterSortImportFeatures() explicitly before their respective setup. The server-less branch replaces the conditional sortFeaturesDisposable.dispose() (which only cleaned up when the variable was already set) with a direct unRegisterSortImportFeatures() call, matching the server-mode branch exactly.

edvilme · 2026-02-25T18:05:24Z

src/extension.ts

+                if (lsClient) {
+                    try {
+                        await lsClient.stop();
+                    } catch (ex) {
+                        traceError(`Server: Stop failed: ${ex}`);
+                    }
+                    lsClient = undefined;
+                }


@copilot Why not use the existing deactivate method, and have it catch too?

deactivate() is only invoked by VS Code when the extension is fully torn down — it can't be reused for mid-lifecycle cleanup during a runServer() call triggered by a serverEnabled toggle while the extension is still active. The lsClient.stop() here stops a running LSP server so the server-less fallback providers can take over, which has to happen inline within runServer().

… level Co-authored-by: rchiodo <19672699+rchiodo@users.noreply.github.com>

…e branches Co-authored-by: edvilme <5952839+edvilme@users.noreply.github.com>

edvilme · 2026-02-25T18:46:18Z

src/extension.ts

+                sortFeaturesDisposable = undefined;
+                sortFeaturesDisposable = registerSortImportFeatures(serverId);


@copilot The undefined assignment is redundant

Suggested change

sortFeaturesDisposable = undefined;

sortFeaturesDisposable = registerSortImportFeatures(serverId);

sortFeaturesDisposable = registerSortImportFeatures(serverId);

Removed in 35ce07d.

Co-authored-by: edvilme <5952839+edvilme@users.noreply.github.com>

rchiodo · 2026-02-25T22:33:24Z

src/extension.ts

+                unRegisterSortImportFeatures();
+                sortFeaturesDisposable = registerSortImportFeatures(serverId);
+                context.subscriptions.push(sortFeaturesDisposable);
+                await sortFeaturesDisposable.startup();


@copilot -80
[High] Subscription accumulation still occurs. The Copilot response claimed disposal was added, but the diff shows no sortFeaturesDisposable.dispose() call before re-assignment. Each true→false toggle pushes a new entry to context.subscriptions without removing the previous one.
Suggested fix (choose one):
// Option A: Dispose and remove from subscriptions
unRegisterSortImportFeatures();
if (sortFeaturesDisposable) {
const idx = context.subscriptions.indexOf(sortFeaturesDisposable);
if (idx >= 0) context.subscriptions.splice(idx, 1);
sortFeaturesDisposable.dispose();
}
sortFeaturesDisposable = registerSortImportFeatures(serverId);
context.subscriptions.push(sortFeaturesDisposable);

// Option B: Only push once, manage lifecycle via module variable
// (Don't push on repeated calls; dispose in deactivate())

Both Skeptic and Architect verified this is the same bug reported in the prior review.

Fixed in 4deee5e. Before reassigning sortFeaturesDisposable, the old entry is now spliced out of context.subscriptions (if present), preventing accumulation on repeated true→false toggles. The new entry is then pushed as usual.

… context.subscriptions before re-registering Co-authored-by: rchiodo <19672699+rchiodo@users.noreply.github.com>

rchiodo · 2026-02-26T00:21:12Z

src/extension.ts

-                        `[Option 2] Set an interpreter using "${serverId}.interpreter" setting.\r\n`,
-                    `Please use Python ${PYTHON_VERSION} or greater.`,
-                );
+            if (getServerEnabled(serverId)) {


@copilot Missing state cleanup in server-enabled branch. When switching to server mode, unRegisterSortImportFeatures() is called but sortFeaturesDisposable is never set to undefined. If the user toggles server-less → server → server-less, the stale reference will still exist, and the indexOf check on line 74 will look for an already-disposed object.
Add after line 47:
if (sortFeaturesDisposable) {
const idx = context.subscriptions.indexOf(sortFeaturesDisposable);
if (idx >= 0) context.subscriptions.splice(idx, 1);
sortFeaturesDisposable = undefined;
}

Alternatively, extract this cleanup into a helper to avoid the asymmetry both branches exhibit.

Fixed in 884ff55. The server-enabled branch now splices sortFeaturesDisposable from context.subscriptions and sets it to undefined immediately after unRegisterSortImportFeatures(), so no stale reference persists across a server-less → server → server-less toggle.

Fixed in d233761. The server-enabled branch now splices the old sortFeaturesDisposable out of context.subscriptions and sets it to undefined, so a subsequent server-less→server→server-less toggle won't find a stale already-disposed reference.

rchiodo · 2026-02-26T00:21:13Z

src/extension.ts

+                    const idx = context.subscriptions.indexOf(sortFeaturesDisposable);
+                    if (idx >= 0) context.subscriptions.splice(idx, 1);
+                }
+                sortFeaturesDisposable = registerSortImportFeatures(serverId);


@copilot Potential orphaned startup. If runServer() is called again while startup() is still awaiting (e.g., rapid config toggles), the previous disposable is disposed but its in-flight startup() may still be running. Consider either:

Storing the startup promise and awaiting/canceling it before disposing, or
Adding a comment documenting that startup() must be resilient to its disposable being disposed mid-flight.

The isRestarting guard mitigates this by returning early on concurrent calls, so this is a minor concern.

Added a comment in 884ff55 documenting that the isRestarting flag prevents concurrent runServer() calls, so a re-entrant dispose cannot race with an in-flight startup().

Added a comment in d233761 documenting that isRestarting prevents concurrent runServer() calls, so startup() cannot be orphaned by a re-entrant dispose while it is still awaiting.

…tarting guard for startup() Co-authored-by: rchiodo <19672699+rchiodo@users.noreply.github.com>

Co-authored-by: rchiodo <19672699+rchiodo@users.noreply.github.com>

Initial plan

f7dbeca

Copilot AI assigned Copilot and bschnurr Feb 25, 2026

Copilot started work on behalf of bschnurr February 25, 2026 17:34 View session

Fix issue #463: restore serverEnabled support with proper mode cleanup

c423e87

Co-authored-by: bschnurr <1946977+bschnurr@users.noreply.github.com>

Copilot AI changed the title ~~[WIP] Investigate issue with isort in VSCode~~ Fix multiple isort handlers registered due to concurrent server restarts Feb 25, 2026

Copilot finished work on behalf of bschnurr February 25, 2026 17:49

rchiodo reviewed Feb 25, 2026

View reviewed changes

Copilot started work on behalf of rchiodo February 25, 2026 18:03 View session

rchiodo reviewed Feb 25, 2026

View reviewed changes

edvilme reviewed Feb 25, 2026

View reviewed changes

edvilme added the bug Issue identified by VS Code Team member as probable bug label Feb 25, 2026

Fix subscription accumulation: track sortFeaturesDisposable at module…

eb9e5f1

… level Co-authored-by: rchiodo <19672699+rchiodo@users.noreply.github.com>

Copilot finished work on behalf of rchiodo February 25, 2026 18:08

Copilot started work on behalf of edvilme February 25, 2026 18:08 View session

Unify cleanup: call unRegisterSortImportFeatures() in both server mod…

cfdb13a

…e branches Co-authored-by: edvilme <5952839+edvilme@users.noreply.github.com>

Copilot finished work on behalf of edvilme February 25, 2026 18:10

edvilme reviewed Feb 25, 2026

View reviewed changes

Copilot started work on behalf of edvilme February 25, 2026 18:46 View session

Remove redundant sortFeaturesDisposable = undefined assignment

35ce07d

Co-authored-by: edvilme <5952839+edvilme@users.noreply.github.com>

Copilot finished work on behalf of edvilme February 25, 2026 18:48

Copilot AI mentioned this pull request Feb 25, 2026

Fix duplicate server handlers on concurrent restarts microsoft/vscode-python-tools-extension-template#259

Merged

edvilme previously approved these changes Feb 25, 2026

View reviewed changes

edvilme closed this in microsoft/vscode-python-tools-extension-template#259 Feb 25, 2026

edvilme reopened this Feb 25, 2026

rchiodo reviewed Feb 25, 2026

View reviewed changes

Copilot started work on behalf of rchiodo February 25, 2026 22:33 View session

Fix subscription accumulation: splice old sortFeaturesDisposable from…

4deee5e

… context.subscriptions before re-registering Co-authored-by: rchiodo <19672699+rchiodo@users.noreply.github.com>

Copilot AI dismissed edvilme’s stale review via 4deee5e February 25, 2026 22:35

Copilot finished work on behalf of rchiodo February 25, 2026 22:35

rchiodo reviewed Feb 26, 2026

View reviewed changes

Copilot started work on behalf of rchiodo February 26, 2026 00:21 View session

Clear sortFeaturesDisposable in server-enabled branch; document isRes…

884ff55

…tarting guard for startup() Co-authored-by: rchiodo <19672699+rchiodo@users.noreply.github.com>

Copilot finished work on behalf of rchiodo February 26, 2026 00:23

Fix curly lint warnings and cleanup in server-enabled branch

d233761

Co-authored-by: rchiodo <19672699+rchiodo@users.noreply.github.com>

Copilot finished work on behalf of rchiodo February 26, 2026 00:25

		sortFeaturesDisposable = undefined;
		sortFeaturesDisposable = registerSortImportFeatures(serverId);

Conversation

Copilot AI commented Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rchiodo Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

rchiodo Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

edvilme Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

edvilme Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

rchiodo Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

rchiodo Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

rchiodo Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Copilot AI commented Feb 25, 2026 •

edited

Loading