fix: address scoped package manager lifecycle feedback

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: edvilme

src/features/envManagers.ts

@@ -175,7 +175,7 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
     private _projectPackageManagers: Map<string, InternalPackageManager> = new Map();
     private readonly _packageManagerEventSubscriptions = new Map<
         string,
-        Map<Event<DidChangePackagesEventArgs>, Disposable>
+        Map<Event<DidChangePackagesEventArgs>, { disposable: Disposable; references: number }>
     >();
     private readonly subscriptions: Disposable[] = [];
 
@@ -235,6 +235,15 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
         private readonly pm: PythonProjectManager,
         private readonly inlineScriptRouting?: InlineScriptRoutingRegistry,
     ) {
+        const projectChanges = this.pm.onDidChangeProjects;
+        if (projectChanges) {
+            const subscription = projectChanges((projects) =>
+                this.evictRemovedProjectPackageManagers(projects ?? this.pm.getProjects()),
+            );
+            if (subscription) {
+                this.subscriptions.push(subscription);
+            }
+        }
         if (this.inlineScriptRouting) {
             this.subscriptions.push(
                 this.inlineScriptRouting.onDidChangeRouteability((e) => {
@@ -328,6 +337,7 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
             for (const [key, scopedManager] of this._projectPackageManagers) {
                 if (scopedManager.id === managerId) {
                     this._projectPackageManagers.delete(key);
+                    this.unsubscribeFromPackageManagerEvents(scopedManager);
                 }
             }
             this.disposePackageManagerEventSubscriptions(managerId);
@@ -485,32 +495,72 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
             subscriptions = new Map();
             this._packageManagerEventSubscriptions.set(provider.id, subscriptions);
         }
-        if (subscriptions.has(event)) {
+        const existing = subscriptions.get(event);
+        if (existing) {
+            existing.references += 1;
             return;
         }
 
         subscriptions.set(
             event,
-            event((e) => {
-                this._onDidChangePackageProviderPackages.fire(e);
-                const eventManager =
-                    Array.from(this._projectPackageManagers.values()).find(
-                        (candidate) => candidate.id === provider.id && candidate.wraps(e.manager),
-                    ) ?? provider;
-                setImmediate(() =>
-                    this._onDidChangePackages.fire({
-                        environment: e.environment,
-                        manager: eventManager,
-                        changes: e.changes,
-                    }),
-                );
-            }),
+            {
+                references: 1,
+                disposable: event((e) => {
+                    this._onDidChangePackageProviderPackages.fire(e);
+                    const eventManager =
+                        Array.from(this._projectPackageManagers.values()).find(
+                            (candidate) => candidate.id === provider.id && candidate.wraps(e.manager),
+                        ) ?? provider;
+                    setImmediate(() =>
+                        this._onDidChangePackages.fire({
+                            environment: e.environment,
+                            manager: eventManager,
+                            changes: e.changes,
+                        }),
+                    );
+                }),
+            },
         );
     }
 
+    private unsubscribeFromPackageManagerEvents(manager: InternalPackageManager): void {
+        const event = manager.packageChangeEvent;
+        if (!event) {
+            return;
+        }
+
+        const subscriptions = this._packageManagerEventSubscriptions.get(manager.id);
+        const subscription = subscriptions?.get(event);
+        if (!subscription) {
+            return;
+        }
+
+        subscription.references -= 1;
+        if (subscription.references === 0) {
+            subscription.disposable.dispose();
+            subscriptions?.delete(event);
+            if (subscriptions?.size === 0) {
+                this._packageManagerEventSubscriptions.delete(manager.id);
+            }
+        }
+    }
+
+    private evictRemovedProjectPackageManagers(projects: readonly PythonProject[]): void {
+        const projectsByPath = new Map(
+            projects.map((project) => [normalizePath(project.uri.fsPath), project] as const),
+        );
+        for (const [key, manager] of this._projectPackageManagers) {
+            const projectPath = manager.project && normalizePath(manager.project.uri.fsPath);
+            if (!projectPath || projectsByPath.get(projectPath) !== manager.project) {
+                this._projectPackageManagers.delete(key);
+                this.unsubscribeFromPackageManagerEvents(manager);
+            }
+        }
+    }
+
     private disposePackageManagerEventSubscriptions(managerId: string): void {
         const subscriptions = this._packageManagerEventSubscriptions.get(managerId);
-        subscriptions?.forEach((subscription) => subscription.dispose());
+        subscriptions?.forEach(({ disposable }) => disposable.dispose());
         this._packageManagerEventSubscriptions.delete(managerId);
     }
 

src/managers/common/packageWatcher.ts

@@ -188,7 +188,22 @@ export function registerPackageWatchers(
     const terminalActivationDisposable = terminalActivation.onDidChangeTerminalActivationState((changes) => {
         if (changes.activated) {
             if (!closedTerminals.has(changes.terminal)) {
-                watchEnvironment(changes.terminal, changes.environment, changes.environment);
+                const projectScope = Array.from(activeEnvironmentByScope.values()).find(
+                    ({ scope, environment }) =>
+                        scope &&
+                        environment.envId.id === changes.environment.envId.id &&
+                        environment.envId.managerId === changes.environment.envId.managerId,
+                )?.scope;
+                const managerContext = projectScope ?? changes.environment;
+                const packageManager = envManagers.getPackageManager(managerContext);
+                if (!projectScope && packageManager?.supportsProjectBinding) {
+                    releaseConsumer(changes.terminal);
+                    log.debug(
+                        `Skipping unscoped package watcher for project-aware manager ${packageManager.id}`,
+                    );
+                    return;
+                }
+                watchEnvironment(changes.terminal, managerContext, changes.environment);
             }
         } else {
             releaseConsumer(changes.terminal);

src/managers/common/registeredManagers.ts

@@ -291,6 +291,10 @@ export class InternalPackageManager implements PackageManager {
         return this.manager.onDidChangePackages;
     }
 
+    get supportsProjectBinding(): boolean {
+        return this.manager.createForProject !== undefined;
+    }
+
     wraps(other: PackageManager): boolean {
         return this.manager === other;
     }

src/managers/poetry/poetryPackageManager.ts

@@ -78,6 +78,7 @@ export class PoetryPackageManager implements PackageManager, Disposable {
     }
 
     async manage(environment: PythonEnvironment, options: PackageManagementOptions): Promise<void> {
+        const cwd = await this.getProjectCwd();
         let toInstall: string[] = [...(options.install ?? [])];
         let toUninstall: string[] = [...(options.uninstall ?? [])];
 
@@ -104,7 +105,6 @@ export class PoetryPackageManager implements PackageManager, Disposable {
             }
         }
 
-        const cwd = await this.getProjectCwd();
         const execute = async (token?: CancellationToken): Promise<void> => {
             try {
                 await this.runPoetryManage({ install: toInstall, uninstall: toUninstall }, cwd, token);

src/test/features/packageManager.api.unit.test.ts

@@ -48,6 +48,7 @@ suite('PythonPackageManagerApi Tests', () => {
     let environment: typeMoq.IMock<PythonEnvironment>;
     let packageManager: typeMoq.IMock<PackageManager>;
     let onDidChangePackagesEmitter: EventEmitter<DidChangePackagesEventArgs>;
+    let projectChangesEmitter: EventEmitter<PythonProject[] | undefined>;
     let getExtensionStub: sinon.SinonStub;
 
     setup(() => {
@@ -71,6 +72,9 @@ suite('PythonPackageManagerApi Tests', () => {
 
         // Mock project manager
         projectManager = typeMoq.Mock.ofType<PythonProjectManager>();
+        projectChangesEmitter = new EventEmitter<PythonProject[] | undefined>();
+        projectManager.setup((pm) => pm.getProjects()).returns(() => []);
+        projectManager.setup((pm) => pm.onDidChangeProjects).returns(() => projectChangesEmitter.event);
         setupNonThenable(projectManager);
 
         // Create environment managers instance
@@ -96,6 +100,7 @@ suite('PythonPackageManagerApi Tests', () => {
         sinon.restore();
         envManagers.dispose();
         onDidChangePackagesEmitter.dispose();
+        projectChangesEmitter.dispose();
     });
 
     /**
@@ -806,5 +811,65 @@ suite('PythonPackageManagerApi Tests', () => {
 
             assert.strictEqual(envManagers.getPackageManager(project.uri), registeredManager);
         });
+
+        test('Should evict scoped package managers when their project is removed', async () => {
+            disposable.dispose();
+            const projectUri = Uri.file(path.join(process.cwd(), 'removed-project'));
+            let currentProject = { name: 'original', uri: projectUri } as PythonProject;
+            projectManager.setup((pm) => pm.get(projectUri)).returns(() => currentProject);
+            const scopedManagers: PackageManager[] = [];
+            const scopedEmitters: EventEmitter<DidChangePackagesEventArgs>[] = [];
+            const provider: PackageManager = {
+                name: 'project-pkg-mgr',
+                manage: async () => undefined,
+                refresh: async () => undefined,
+                getPackages: async () => [],
+                createForProject: () => {
+                    const emitter = new EventEmitter<DidChangePackagesEventArgs>();
+                    const scopedManager: PackageManager = {
+                        name: 'project-pkg-mgr',
+                        manage: async () => undefined,
+                        refresh: async () => undefined,
+                        getPackages: async () => [],
+                        onDidChangePackages: emitter.event,
+                    };
+                    scopedEmitters.push(emitter);
+                    scopedManagers.push(scopedManager);
+                    return scopedManager;
+                },
+            };
+            disposable = envManagers.registerPackageManager(provider);
+            const registeredManager = envManagers.packageManagers[0];
+            sinon.stub(workspaceApis, 'getConfiguration').returns({
+                get: (section: string, defaultValue?: unknown) =>
+                    section === 'defaultPackageManager' ? registeredManager.id : defaultValue,
+            } as WorkspaceConfiguration);
+            const events: unknown[] = [];
+            const eventDisposable = envManagers.onDidChangePackages((event) => events.push(event));
+
+            const original = envManagers.getPackageManager(projectUri);
+            currentProject = { name: 'replacement', uri: projectUri } as PythonProject;
+            projectChangesEmitter.fire([currentProject]);
+            const replacement = envManagers.getPackageManager(projectUri);
+
+            assert.notStrictEqual(original, replacement);
+            assert.strictEqual(scopedManagers.length, 2);
+            assert.strictEqual(replacement?.project, currentProject);
+
+            const packageChange = {
+                environment: environment.object,
+                changes: [],
+            };
+            scopedEmitters[0].fire({ ...packageChange, manager: scopedManagers[0] });
+            await new Promise<void>((resolve) => setImmediate(resolve));
+            assert.strictEqual(events.length, 0);
+
+            scopedEmitters[1].fire({ ...packageChange, manager: scopedManagers[1] });
+            await new Promise<void>((resolve) => setImmediate(resolve));
+            assert.strictEqual(events.length, 1);
+
+            eventDisposable.dispose();
+            scopedEmitters.forEach((emitter) => emitter.dispose());
+        });
     });
 });

src/test/managers/common/packageWatcher.unit.test.ts

@@ -538,6 +538,61 @@ suite('Package Watcher', () => {
             assert.strictEqual(createFileSystemWatcherStub.callCount, 1);
         });
 
+        test('should reuse a project-scoped watcher for terminal activation', () => {
+            createFileSystemWatcherStub.returns(createMockWatcher());
+            const environmentChanges = new EventEmitter<DidChangeEnvironmentEventArgs>();
+            const scope = Uri.file('workspace');
+            const project = { name: 'project', uri: scope } as PythonProject;
+            const rootManager = new InternalPackageManager('poetry', {
+                ...createMockPackageManager(),
+                createForProject: () => createMockPackageManager() as PackageManager,
+            } as PackageManager);
+            const scopedManager = new InternalPackageManager(
+                'poetry',
+                createMockPackageManager() as PackageManager,
+                project,
+            );
+            const env = createMockEnvironment();
+            const terminal = { name: 'terminal' } as Terminal;
+            const envManagers = {
+                onDidChangeActiveEnvironment: environmentChanges.event,
+                getPackageManager: sandbox
+                    .stub()
+                    .callsFake((context) => (context instanceof Uri ? scopedManager : rootManager)),
+            } as unknown as EnvironmentManagers;
+
+            registerPackageWatchers(envManagers, mockTerminalActivation, mockLogOutputChannel as LogOutputChannel);
+            environmentChanges.fire({ uri: scope, new: env, old: undefined });
+            terminalActivationChanges.fire({ terminal, environment: env, activated: true });
+
+            assert.strictEqual(createFileSystemWatcherStub.callCount, 1);
+            assert.ok((envManagers.getPackageManager as sinon.SinonStub).calledWith(scope));
+        });
+
+        test('should skip terminal-only watchers for project-aware package managers', () => {
+            const environmentChanges = new EventEmitter<DidChangeEnvironmentEventArgs>();
+            const packageManager = new InternalPackageManager('poetry', {
+                ...createMockPackageManager(),
+                createForProject: () => createMockPackageManager() as PackageManager,
+            } as PackageManager);
+            const env = createMockEnvironment();
+            const terminal = { name: 'terminal' } as Terminal;
+            const envManagers = {
+                onDidChangeActiveEnvironment: environmentChanges.event,
+                getPackageManager: sandbox.stub().returns(packageManager),
+            } as unknown as EnvironmentManagers;
+
+            registerPackageWatchers(envManagers, mockTerminalActivation, mockLogOutputChannel as LogOutputChannel);
+            terminalActivationChanges.fire({ terminal, environment: env, activated: true });
+
+            assert.strictEqual(createFileSystemWatcherStub.callCount, 0);
+            assert.ok(
+                (mockLogOutputChannel.debug as sinon.SinonStub).calledWith(
+                    'Skipping unscoped package watcher for project-aware manager poetry',
+                ),
+            );
+        });
+
         test('should release a terminal environment watcher when the terminal closes', () => {
             const mockWatcher = createMockWatcher();
             createFileSystemWatcherStub.returns(mockWatcher);

src/test/managers/poetry/poetryPackageManager.unit.test.ts

@@ -97,6 +97,19 @@ suite('PoetryPackageManager', () => {
         assert.strictEqual(runPoetryStub.callCount, 0);
     });
 
+    test('unbound package management rejects before no-op exits or prompts', async () => {
+        const showInputBox = sinon.stub(windowApis, 'showInputBox');
+
+        await assert.rejects(
+            manager.manage(environment, { install: [], runHeadless: true }),
+            /require a Python project/,
+        );
+        await assert.rejects(manager.manage(environment, { install: [] }), /require a Python project/);
+
+        assert.ok(showInputBox.notCalled);
+        assert.strictEqual(runPoetryStub.callCount, 0);
+    });
+
     test('refresh rejects operations without a project', async () => {
         await assert.rejects(manager.refresh(environment), /require a Python project/);
         assert.strictEqual(runPoetryStub.callCount, 0);