diff --git a/src/managers/conda/condaEnvManager.ts b/src/managers/conda/condaEnvManager.ts index 413034d9e..81cb52cc0 100644 --- a/src/managers/conda/condaEnvManager.ts +++ b/src/managers/conda/condaEnvManager.ts @@ -84,6 +84,7 @@ export class CondaEnvManager implements EnvironmentManager, Disposable { private collection: PythonEnvironment[] = []; private fsPathToEnv: Map = new Map(); private globalEnv: PythonEnvironment | undefined; + private inventoryOperations: Promise = Promise.resolve(); private readonly _onDidChangeEnvironment = new EventEmitter(); public readonly onDidChangeEnvironment = this._onDidChangeEnvironment.event; @@ -283,16 +284,18 @@ export class CondaEnvManager implements EnvironmentManager, Disposable { } const discovery = (this.discovery = createDeferred()); try { - await withProgress( - { location: ProgressLocation.Window, title: CondaStrings.condaDiscovering }, - async () => { - this.collection = await this.discoverEnvironments(false); - await this.loadEnvMap(); - this._onDidChangeEnvironments.fire( - this.collection.map((environment) => ({ environment, kind: EnvironmentChangeKind.add })), - ); - }, - ); + await this.enqueueInventoryOperation(async () => { + await withProgress( + { location: ProgressLocation.Window, title: CondaStrings.condaDiscovering }, + async () => { + this.collection = await this.discoverEnvironments(false); + await this.loadEnvMap(); + this._onDidChangeEnvironments.fire( + this.collection.map((environment) => ({ environment, kind: EnvironmentChangeKind.add })), + ); + }, + ); + }); } catch (error) { this.initializationError = error; this.discovery = undefined; @@ -302,6 +305,15 @@ export class CondaEnvManager implements EnvironmentManager, Disposable { } } + private enqueueInventoryOperation(operation: () => Promise): Promise { + const next = this.inventoryOperations.then(operation); + this.inventoryOperations = next.then( + () => undefined, + () => undefined, + ); + return next; + } + private async discoverEnvironments(hardRefresh: boolean): Promise { try { let discoveryError: unknown; @@ -454,26 +466,28 @@ export class CondaEnvManager implements EnvironmentManager, Disposable { } async refresh(context: RefreshEnvironmentsScope): Promise { if (context === undefined) { - await withProgress( - { - location: ProgressLocation.Window, - title: CondaStrings.condaRefreshingEnvs, - }, - async () => { - this.log.info('Refreshing Conda Environments'); - const discard = this.collection.map((c) => c); - this.collection = await this.discoverEnvironments(true); + await this.enqueueInventoryOperation(async () => { + await withProgress( + { + location: ProgressLocation.Window, + title: CondaStrings.condaRefreshingEnvs, + }, + async () => { + this.log.info('Refreshing Conda Environments'); + const discard = this.collection.map((c) => c); + this.collection = await this.discoverEnvironments(true); - await this.loadEnvMap(); + await this.loadEnvMap(); - const args = [ - ...discard.map((env) => ({ kind: EnvironmentChangeKind.remove, environment: env })), - ...this.collection.map((env) => ({ kind: EnvironmentChangeKind.add, environment: env })), - ]; + const args = [ + ...discard.map((env) => ({ kind: EnvironmentChangeKind.remove, environment: env })), + ...this.collection.map((env) => ({ kind: EnvironmentChangeKind.add, environment: env })), + ]; - this._onDidChangeEnvironments.fire(args); - }, - ); + this._onDidChangeEnvironments.fire(args); + }, + ); + }); } } async get(scope: GetEnvironmentScope, toolExecution = false): Promise { @@ -649,12 +663,8 @@ export class CondaEnvManager implements EnvironmentManager, Disposable { // If the environment is not found, resolve the fsPath. Could be portable conda. if (!this.globalEnv) { - this.globalEnv = await resolveCondaPath(fsPath, this.nativeFinder, this.api, this.log, this); - - // If the environment is resolved, add it to the collection - if (this.globalEnv) { - this.collection.push(this.globalEnv); - } + const resolved = await resolveCondaPath(fsPath, this.nativeFinder, this.api, this.log, this); + this.globalEnv = resolved ? this.trackResolvedEnvironment(resolved) : undefined; } } @@ -694,9 +704,7 @@ export class CondaEnvManager implements EnvironmentManager, Disposable { const resolved = await resolveCondaPath(env, this.nativeFinder, this.api, this.log, this); if (resolved) { - // If resolved add it to the collection - this.fsPathToEnv.set(normalizedPath, resolved); - this.collection.push(resolved); + this.fsPathToEnv.set(normalizedPath, this.trackResolvedEnvironment(resolved)); } else { this.log.error(`Failed to resolve conda environment: ${env}`); } @@ -756,6 +764,23 @@ export class CondaEnvManager implements EnvironmentManager, Disposable { }); } + /** + * Returns the canonical collection entry for a resolved Conda prefix, adding it only when the + * normalized environment path is not already present. Persisted paths can be resolved concurrently, + * so callers must perform this check after resolution rather than relying on an earlier lookup. + */ + private trackResolvedEnvironment(resolved: PythonEnvironment): PythonEnvironment { + const key = normalizePath(resolved.environmentPath.fsPath); + const existing = this.collection.find((environment) => + normalizePath(environment.environmentPath.fsPath) === key, + ); + if (existing) { + return existing; + } + this.collection.push(resolved); + return resolved; + } + private findEnvironmentByName(name: string): PythonEnvironment | undefined { return this.collection.find((e) => { return e.name === name; diff --git a/src/test/managers/conda/condaEnvManager.refreshRace.unit.test.ts b/src/test/managers/conda/condaEnvManager.refreshRace.unit.test.ts new file mode 100644 index 000000000..b43c02211 --- /dev/null +++ b/src/test/managers/conda/condaEnvManager.refreshRace.unit.test.ts @@ -0,0 +1,277 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import * as assert from 'assert'; +import * as fs from 'fs'; +import * as path from 'path'; +import * as sinon from 'sinon'; +import { CancellationTokenSource, LogOutputChannel, Uri } from 'vscode'; +import { + DidChangeEnvironmentsEventArgs, + EnvironmentChangeKind, + PythonEnvironment, + PythonEnvironmentApi, + PythonProject, +} from '../../../api'; +import { createDeferred, Deferred } from '../../../common/utils/deferred'; +import { isSameOrParentPath, normalizePath } from '../../../common/utils/pathUtils'; +import * as windowApis from '../../../common/window.apis'; +import { NativePythonFinder } from '../../../managers/common/nativePythonFinder'; +import { CondaEnvManager } from '../../../managers/conda/condaEnvManager'; +import * as condaUtils from '../../../managers/conda/condaUtils'; +import { createMockPythonEnvironment } from '../../mocks/pythonEnvironment'; + +type PersistedSelectionScope = 'global' | 'project'; + +interface CondaManagerInternals { + _initialized: Deferred | undefined; + loadEnvMap(): Promise; +} + +function nextTurn(): Promise { + return new Promise((resolve) => setImmediate(resolve)); +} + +function createProject(scope: PersistedSelectionScope): PythonProject { + return { + name: `${scope}-project`, + uri: Uri.file(path.resolve('conda-refresh-race', scope, 'project')), + }; +} + +function createApi(project: PythonProject): PythonEnvironmentApi { + return { + getPythonProjects: () => [project], + getPythonProject: (uri: Uri) => + isSameOrParentPath(project.uri.fsPath, uri.fsPath) ? project : undefined, + } as unknown as PythonEnvironmentApi; +} + +function createLog(): LogOutputChannel { + return { + info: sinon.stub(), + error: sinon.stub(), + warn: sinon.stub(), + } as unknown as LogOutputChannel; +} + +function createEnvironment(name: string, envPath: string, version = '3.12.0'): PythonEnvironment { + return createMockPythonEnvironment({ + name, + envPath, + version, + id: `${name}-id`, + }); +} + +suite('CondaEnvManager - concurrent persisted selection restoration', () => { + let sandbox: sinon.SinonSandbox; + let cancellation: CancellationTokenSource; + + setup(() => { + sandbox = sinon.createSandbox(); + cancellation = new CancellationTokenSource(); + sandbox.stub(fs.promises, 'access').resolves(); + sandbox.stub(windowApis, 'withProgress').callsFake(async (_options, task) => + task({ report: () => undefined }, cancellation.token), + ); + }); + + teardown(() => { + sandbox.restore(); + cancellation.dispose(); + }); + + for (const selectionScope of ['global', 'project'] as const) { + test(`deduplicates concurrent ${selectionScope} persisted-path resolution and keeps its mapping canonical`, async () => { + const project = createProject(selectionScope); + const targetPath = Uri.file(path.resolve('conda-refresh-race', selectionScope, 'external-prefix')).fsPath; + const persistedPath = path.join( + targetPath, + process.platform === 'win32' ? 'python.exe' : path.join('bin', 'python'), + ); + const alternateTargetPath = + path.sep === '\\' ? targetPath.replace(/\\/g, '/') : targetPath.replace(/\//g, '\\'); + assert.notStrictEqual(alternateTargetPath, targetPath, 'The raw path spellings must differ'); + assert.strictEqual( + normalizePath(alternateTargetPath), + normalizePath(targetPath), + 'The competing path spellings must have the same collection identity', + ); + const firstResolved = createEnvironment(`${selectionScope}-first`, targetPath); + const secondResolved: PythonEnvironment = { + ...createEnvironment(`${selectionScope}-second`, targetPath), + environmentPath: { fsPath: alternateTargetPath } as Uri, + }; + sandbox + .stub(condaUtils, 'getCondaForGlobal') + .resolves(selectionScope === 'global' ? persistedPath : undefined); + sandbox.stub(condaUtils, 'getCondaForWorkspace').callsFake(async (fsPath) => + selectionScope === 'project' && fsPath === project.uri.fsPath ? persistedPath : undefined, + ); + + const firstResolutionStarted = createDeferred(); + const releaseFirstResolution = createDeferred(); + let resolutionCount = 0; + let activeResolutions = 0; + let maxActiveResolutions = 0; + const resolveCondaPath = sandbox.stub(condaUtils, 'resolveCondaPath').callsFake(async () => { + const currentResolution = ++resolutionCount; + activeResolutions += 1; + maxActiveResolutions = Math.max(maxActiveResolutions, activeResolutions); + try { + if (currentResolution === 1) { + firstResolutionStarted.resolve(); + await releaseFirstResolution.promise; + return firstResolved; + } + return secondResolved; + } finally { + activeResolutions -= 1; + } + }); + + const manager = new CondaEnvManager( + {} as NativePythonFinder, + createApi(project), + createLog(), + ); + const internals = manager as unknown as CondaManagerInternals; + const initialized = createDeferred(); + initialized.resolve(); + internals._initialized = initialized; + + const firstLoad = internals.loadEnvMap(); + await firstResolutionStarted.promise; + const secondLoad = internals.loadEnvMap(); + await nextTurn(); + releaseFirstResolution.resolve(); + await Promise.all([firstLoad, secondLoad]); + + const environments = await manager.getEnvironments('all'); + const selected = await manager.get(selectionScope === 'global' ? undefined : project.uri); + assert.strictEqual(maxActiveResolutions, 2, 'The harness must force both stale checks before either append'); + assert.ok( + resolveCondaPath.getCalls().every((call) => call.args[0] === persistedPath), + 'Each persisted interpreter alias must resolve to its canonical prefix', + ); + assert.strictEqual(environments.length, 1, 'Only one object may represent the external prefix'); + assert.strictEqual(environments[0], secondResolved, 'The first completed resolution becomes canonical'); + assert.strictEqual(selected, environments[0], 'The persisted selection must reference the collection object'); + }); + + test(`serializes startup and explicit refresh for a ${selectionScope} persisted selection`, async () => { + const project = createProject(selectionScope); + const targetPath = Uri.file(path.resolve('conda-refresh-race', selectionScope, 'external-prefix')).fsPath; + const fastPathEnvironment = createEnvironment(`${selectionScope}-fast`, targetPath); + const backgroundEnvironment = createEnvironment(`${selectionScope}-background`, targetPath); + const refreshedEnvironment = createEnvironment(`${selectionScope}-refreshed`, targetPath); + const noPythonEnvironment = createEnvironment(`${selectionScope}-no-python`, targetPath, 'no-python'); + + sandbox + .stub(condaUtils, 'getCondaForGlobal') + .resolves(selectionScope === 'global' ? targetPath : undefined); + sandbox.stub(condaUtils, 'getCondaForWorkspace').callsFake(async (fsPath) => + selectionScope === 'project' && fsPath === project.uri.fsPath ? targetPath : undefined, + ); + + const releaseInitialDiscovery = createDeferred(); + let discoveryCount = 0; + const refreshCondaEnvs = sandbox.stub(condaUtils, 'refreshCondaEnvs').callsFake(async () => { + discoveryCount += 1; + if (discoveryCount === 1) { + return releaseInitialDiscovery.promise; + } + if (discoveryCount === 2) { + return []; + } + return [noPythonEnvironment]; + }); + + const firstInventoryResolutionStarted = createDeferred(); + const releaseFirstInventoryResolution = createDeferred(); + let resolutionCount = 0; + let activeResolutions = 0; + let maxActiveResolutions = 0; + const resolveCondaPath = sandbox.stub(condaUtils, 'resolveCondaPath').callsFake(async () => { + const currentResolution = ++resolutionCount; + activeResolutions += 1; + maxActiveResolutions = Math.max(maxActiveResolutions, activeResolutions); + try { + if (selectionScope === 'project' && currentResolution === 1) { + return fastPathEnvironment; + } + const inventoryResolution = currentResolution - (selectionScope === 'project' ? 1 : 0); + if (inventoryResolution === 1) { + firstInventoryResolutionStarted.resolve(); + await releaseFirstInventoryResolution.promise; + return backgroundEnvironment; + } + return refreshedEnvironment; + } finally { + activeResolutions -= 1; + } + }); + + const manager = new CondaEnvManager( + {} as NativePythonFinder, + createApi(project), + createLog(), + ); + const events: DidChangeEnvironmentsEventArgs[] = []; + const subscription = manager.onDidChangeEnvironments((event) => events.push(event)); + + try { + const startupGet = manager.get(project.uri); + if (selectionScope === 'project') { + assert.strictEqual(await startupGet, fastPathEnvironment, 'The fast path must remain immediate'); + } + releaseInitialDiscovery.resolve([]); + await firstInventoryResolutionStarted.promise; + + const explicitRefresh = manager.refresh(undefined); + await nextTurn(); + releaseFirstInventoryResolution.resolve(); + await Promise.all([startupGet, explicitRefresh, manager.initialize()]); + + let environments = await manager.getEnvironments('all'); + let selected = await manager.get(selectionScope === 'global' ? undefined : project.uri); + assert.strictEqual(maxActiveResolutions, 1, 'Inventory rebuilds must not overlap persisted resolution'); + assert.strictEqual(refreshCondaEnvs.callCount, 2, 'Both startup and explicit discovery must still run'); + assert.deepStrictEqual(environments, [refreshedEnvironment]); + assert.strictEqual(selected, refreshedEnvironment, 'The selection must use the final collection object'); + assert.deepStrictEqual( + events.map((event) => event.map((change) => change.kind)), + [ + [EnvironmentChangeKind.add], + [EnvironmentChangeKind.remove, EnvironmentChangeKind.add], + ], + 'Startup and refresh must publish ordered, balanced inventory changes', + ); + assert.strictEqual(events[0][0].environment, backgroundEnvironment); + assert.strictEqual(events[1][0].environment, backgroundEnvironment); + assert.strictEqual(events[1][1].environment, refreshedEnvironment); + + await manager.refresh(undefined); + + environments = await manager.getEnvironments('all'); + selected = await manager.get(selectionScope === 'global' ? undefined : project.uri); + assert.deepStrictEqual(environments, [noPythonEnvironment]); + assert.strictEqual(selected, noPythonEnvironment, 'A later no-Python update must not be suppressed'); + assert.strictEqual(resolveCondaPath.callCount, selectionScope === 'project' ? 3 : 2); + assert.deepStrictEqual( + events[2].map((change) => change.kind), + [EnvironmentChangeKind.remove, EnvironmentChangeKind.add], + 'A stable explicit refresh must retain normal remove/add event behavior', + ); + assert.strictEqual(events[2][0].environment, refreshedEnvironment); + assert.strictEqual(events[2][1].environment, noPythonEnvironment); + } finally { + releaseInitialDiscovery.resolve([]); + releaseFirstInventoryResolution.resolve(); + subscription.dispose(); + manager.dispose(); + } + }); + } +});