Skip to content

Commit 237318d

Browse files
StellaHuang95StellaHuang95
committed
Address inline persistence review feedback
Prevent stale central selection publication, align inline environment identity with executable paths, normalize stale cleanup paths, and extract named association types. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1a9f6ba1-9bd3-4664-bc25-a0d34d7a2e91
1 parent dae5e68 commit 237318d

4 files changed

Lines changed: 270 additions & 89 deletions

File tree

‎src/features/envManagers.ts‎

Lines changed: 129 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
6565
*/
6666
private readonly _activeSelection = new Map<string, PythonEnvironment | undefined>();
6767
private readonly _selectionRevisions = new Map<string, number>();
68+
private readonly _selectionOperationCounters = new Map<string, number>();
6869

6970
private _onDidChangeEnvironmentManager = new EventEmitter<DidChangeEnvironmentManagerEventArgs>();
7071
private _onDidChangePackageManager = new EventEmitter<DidChangePackageManagerEventArgs>();
@@ -117,7 +118,7 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
117118
);
118119
}),
119120
mgr.onDidChangeEnvironment((e: DidChangeEnvironmentEventArgs) => {
120-
if (e.old?.envId.id === e.new?.envId.id) {
121+
if (this.isSameEnvironment(e.old, e.new)) {
121122
return;
122123
}
123124

@@ -362,11 +363,12 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
362363
}
363364
const project = scope ? this.pm.get(scope) : undefined;
364365
const key = this.getActiveSelectionKey(scope, manager, project);
366+
const operation = this.beginSelectionOperation(key);
367+
const inlineClearOperation =
368+
scope instanceof Uri && manager.id !== INLINE_SCRIPT_MANAGER_ID
369+
? this.beginSelectionOperation(this.getInlineScriptSelectionKey(scope))
370+
: undefined;
365371
await manager.set(scope, environment);
366-
if (scope instanceof Uri) {
367-
this.clearInlineActiveSelection(scope, manager);
368-
}
369-
this.bumpSelectionRevision(key);
370372

371373
// Only persist to settings when explicitly requested
372374
if (shouldPersistSettings && scope) {
@@ -393,8 +395,14 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
393395
);
394396
}
395397

398+
if (scope instanceof Uri) {
399+
this.clearInlineActiveSelection(scope, manager, inlineClearOperation);
400+
}
401+
if (!this.commitSelectionOperation(key, operation)) {
402+
return;
403+
}
396404
const oldEnv = this._activeSelection.get(key);
397-
if (oldEnv?.envId.id !== environment?.envId.id) {
405+
if (!this.isSameEnvironment(oldEnv, environment)) {
398406
this._activeSelection.set(key, environment);
399407
await new Promise<void>((resolve, reject) => {
400408
setImmediate(() => {
@@ -443,42 +451,44 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
443451
const settings: EditAllManagerSettings[] = [];
444452
const events: DidChangeEnvironmentEventArgs[] = [];
445453
if (Array.isArray(scope) && scope.every((s) => s instanceof Uri)) {
454+
const selections = scope.map((uri) => this.beginPendingSelection(uri, manager));
446455
await manager.set(scope, environment);
447-
scope.forEach((uri) => {
448-
this.clearInlineActiveSelection(uri, manager);
449-
const project = this.pm.get(uri);
450-
this.bumpSelectionRevision(this.getActiveSelectionKey(uri, manager, project));
451-
});
452-
scope.forEach((uri) => {
453-
const m = this.getEnvironmentManager(uri);
454-
const project = this.pm.get(uri);
456+
selections.forEach((selection) => {
457+
const m = this.getEnvironmentManager(selection.scope);
455458
// Always add settings when persisting, OR when manager differs
456459
if (
457460
(shouldPersistSettings || manager.id !== m?.id) &&
458-
this.canPersistManagerSettingForScope(uri, manager, project)
461+
this.canPersistManagerSettingForScope(selection.scope, manager, selection.project)
459462
) {
460463
settings.push({
461-
project,
464+
project: selection.project,
462465
envManager: manager.id,
463466
packageManager: manager.preferredPackageManagerId,
464467
});
465468
}
466-
467-
const key = this.getActiveSelectionKey(uri, manager, project);
468-
const oldEnv = this._activeSelection.get(key);
469-
if (oldEnv?.envId.id !== environment?.envId.id) {
470-
this._activeSelection.set(key, environment);
469+
});
470+
if (shouldPersistSettings) {
471+
await setAllManagerSettings(settings);
472+
}
473+
selections.forEach((selection) => {
474+
this.clearInlineActiveSelection(selection.scope, manager, selection.inlineClearOperation);
475+
if (!this.commitSelectionOperation(selection.key, selection.operation)) {
476+
return;
477+
}
478+
const oldEnv = this._activeSelection.get(selection.key);
479+
if (!this.isSameEnvironment(oldEnv, environment)) {
480+
this._activeSelection.set(selection.key, environment);
471481
events.push({
472-
uri: this.getActiveSelectionUri(uri, manager, project),
482+
uri: this.getActiveSelectionUri(selection.scope, manager, selection.project),
473483
new: environment,
474484
old: oldEnv,
475485
});
476486
}
477487
});
478488
} else if (typeof scope === 'string' && scope === 'global') {
479489
const m = this.getEnvironmentManager(undefined);
490+
const operation = this.beginSelectionOperation('global');
480491
await manager.set(undefined, environment);
481-
this.bumpSelectionRevision('global');
482492
// Always add settings when persisting, OR when manager differs
483493
if (shouldPersistSettings || manager.id !== m?.id) {
484494
settings.push({
@@ -488,15 +498,16 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
488498
});
489499
}
490500

491-
const oldEnv = this._activeSelection.get('global');
492-
if (oldEnv?.envId.id !== environment?.envId.id) {
493-
this._activeSelection.set('global', environment);
494-
events.push({ uri: undefined, new: environment, old: oldEnv });
501+
if (shouldPersistSettings) {
502+
await setAllManagerSettings(settings);
503+
}
504+
if (this.commitSelectionOperation('global', operation)) {
505+
const oldEnv = this._activeSelection.get('global');
506+
if (!this.isSameEnvironment(oldEnv, environment)) {
507+
this._activeSelection.set('global', environment);
508+
events.push({ uri: undefined, new: environment, old: oldEnv });
509+
}
495510
}
496-
}
497-
// Only persist to settings when explicitly requested
498-
if (shouldPersistSettings) {
499-
await setAllManagerSettings(settings);
500511
}
501512
if (events.length > 0) {
502513
await new Promise<void>((resolve, reject) => {
@@ -521,21 +532,19 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
521532
}
522533
});
523534
for (const [manager, uris] of groupedScopes) {
535+
const selections = uris.map((uri) => this.beginPendingSelection(uri, manager));
524536
await manager.set(uris);
525-
uris.forEach((uri) => {
526-
const project = this.pm.get(uri);
527-
this.bumpSelectionRevision(this.getActiveSelectionKey(uri, manager, project));
528-
});
529537
await Promise.all(
530-
uris.map(async (uri) => {
531-
const project = this.pm.get(uri);
532-
const newEnv = await manager.get(uri);
533-
const key = this.getActiveSelectionKey(uri, manager, project);
534-
const oldEnv = this._activeSelection.get(key);
535-
if (oldEnv?.envId.id !== newEnv?.envId.id) {
536-
this._activeSelection.set(key, newEnv);
538+
selections.map(async (selection) => {
539+
const newEnv = await manager.get(selection.scope);
540+
if (!this.commitSelectionOperation(selection.key, selection.operation)) {
541+
return;
542+
}
543+
const oldEnv = this._activeSelection.get(selection.key);
544+
if (!this.isSameEnvironment(oldEnv, newEnv)) {
545+
this._activeSelection.set(selection.key, newEnv);
537546
events.push({
538-
uri: this.getActiveSelectionUri(uri, manager, project),
547+
uri: this.getActiveSelectionUri(selection.scope, manager, selection.project),
539548
new: newEnv,
540549
old: oldEnv,
541550
});
@@ -546,13 +555,15 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
546555
} else if (typeof scope === 'string' && scope === 'global') {
547556
const manager = this.getEnvironmentManager(undefined);
548557
if (manager) {
558+
const operation = this.beginSelectionOperation('global');
549559
await manager.set(undefined);
550-
this.bumpSelectionRevision('global');
551560
const newEnv = await manager.get(undefined);
552-
const oldEnv = this._activeSelection.get('global');
553-
if (oldEnv?.envId.id !== newEnv?.envId.id) {
554-
this._activeSelection.set('global', newEnv);
555-
events.push({ uri: undefined, new: newEnv, old: oldEnv });
561+
if (this.commitSelectionOperation('global', operation)) {
562+
const oldEnv = this._activeSelection.get('global');
563+
if (!this.isSameEnvironment(oldEnv, newEnv)) {
564+
this._activeSelection.set('global', newEnv);
565+
events.push({ uri: undefined, new: newEnv, old: oldEnv });
566+
}
556567
}
557568
}
558569
}
@@ -640,27 +651,24 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
640651

641652
const project = scope ? this.pm.get(scope) : undefined;
642653
const key = this.getActiveSelectionKey(scope, manager, project);
643-
const revision = (this._selectionRevisions.get(key) ?? 0) + 1;
654+
const operation = this.beginSelectionOperation(key);
644655
const newEnv = await manager.get(scope);
645-
if (
646-
this.getEnvironmentManager(scope) !== manager ||
647-
(this._selectionRevisions.get(key) ?? 0) >= revision
648-
) {
656+
if (this.getEnvironmentManager(scope) !== manager) {
649657
return;
650658
}
651-
this._selectionRevisions.set(key, revision);
652659

653660
const oldEnv = this._activeSelection.get(key);
654-
if (oldEnv?.envId.id !== newEnv?.envId.id) {
655-
this._activeSelection.set(key, newEnv);
656-
setImmediate(() =>
657-
this._onDidChangeActiveEnvironment.fire({
658-
uri: this.getActiveSelectionUri(scope, manager, project),
659-
new: newEnv,
660-
old: oldEnv,
661-
}),
662-
);
661+
if (this.isSameEnvironment(oldEnv, newEnv) || !this.commitSelectionOperation(key, operation)) {
662+
return;
663663
}
664+
this._activeSelection.set(key, newEnv);
665+
setImmediate(() =>
666+
this._onDidChangeActiveEnvironment.fire({
667+
uri: this.getActiveSelectionUri(scope, manager, project),
668+
new: newEnv,
669+
old: oldEnv,
670+
}),
671+
);
664672
}
665673

666674
getLastKnownEnvironment(scope: GetEnvironmentScope): PythonEnvironment | undefined {
@@ -694,13 +702,32 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
694702
return `inline-script:${normalizePath(scope.fsPath)}`;
695703
}
696704

697-
private clearInlineActiveSelection(scope: Uri, manager: InternalEnvironmentManager): void {
698-
if (manager.id === INLINE_SCRIPT_MANAGER_ID) {
705+
private beginPendingSelection(scope: Uri, manager: InternalEnvironmentManager): PendingEnvironmentSelection {
706+
const project = this.pm.get(scope);
707+
const key = this.getActiveSelectionKey(scope, manager, project);
708+
return {
709+
scope,
710+
project,
711+
key,
712+
operation: this.beginSelectionOperation(key),
713+
inlineClearOperation:
714+
manager.id === INLINE_SCRIPT_MANAGER_ID
715+
? undefined
716+
: this.beginSelectionOperation(this.getInlineScriptSelectionKey(scope)),
717+
};
718+
}
719+
720+
private clearInlineActiveSelection(
721+
scope: Uri,
722+
manager: InternalEnvironmentManager,
723+
operation: number | undefined,
724+
): void {
725+
if (manager.id === INLINE_SCRIPT_MANAGER_ID || operation === undefined) {
699726
return;
700727
}
701728
const key = this.getInlineScriptSelectionKey(scope);
702-
if (this._activeSelection.delete(key)) {
703-
this.bumpSelectionRevision(key);
729+
if (this.commitSelectionOperation(key, operation)) {
730+
this._activeSelection.delete(key);
704731
}
705732
}
706733

@@ -716,10 +743,33 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
716743
);
717744
}
718745

719-
private bumpSelectionRevision(key: string): number {
720-
const revision = (this._selectionRevisions.get(key) ?? 0) + 1;
721-
this._selectionRevisions.set(key, revision);
722-
return revision;
746+
private beginSelectionOperation(key: string): number {
747+
const operation = (this._selectionOperationCounters.get(key) ?? 0) + 1;
748+
this._selectionOperationCounters.set(key, operation);
749+
return operation;
750+
}
751+
752+
private commitSelectionOperation(key: string, operation: number): boolean {
753+
if ((this._selectionRevisions.get(key) ?? 0) > operation) {
754+
return false;
755+
}
756+
this._selectionRevisions.set(key, operation);
757+
return true;
758+
}
759+
760+
private isSameEnvironment(
761+
first: PythonEnvironment | undefined,
762+
second: PythonEnvironment | undefined,
763+
): boolean {
764+
if (first === second) {
765+
return true;
766+
}
767+
if (!first || !second || first.envId.managerId !== second.envId.managerId) {
768+
return false;
769+
}
770+
return first.envId.managerId === INLINE_SCRIPT_MANAGER_ID
771+
? normalizePath(first.environmentPath.fsPath) === normalizePath(second.environmentPath.fsPath)
772+
: first.envId.id === second.envId.id;
723773
}
724774

725775
getProjectEnvManagers(uris: Uri[]): InternalEnvironmentManager[] {
@@ -733,3 +783,11 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
733783
return projectEnvManagers;
734784
}
735785
}
786+
787+
interface PendingEnvironmentSelection {
788+
readonly scope: Uri;
789+
readonly project: PythonProject | undefined;
790+
readonly key: string;
791+
readonly operation: number;
792+
readonly inlineClearOperation: number | undefined;
793+
}

‎src/managers/builtin/inlineScript/envManager.ts‎

Lines changed: 24 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -65,14 +65,6 @@ const CACHED_ASSOCIATION_VALIDATION_INTERVAL_MS = 5_000;
6565
/** Workspace-state key for PEP 723 script path to environment executable associations. */
6666
export const INLINE_SCRIPT_ENVS_KEY = `${ENVS_EXTENSION_ID}:inline-script:SCRIPT_ENVIRONMENTS`;
6767

68-
type PersistedInlineScriptEnvironments = Record<string, string>;
69-
70-
interface PersistedAssociationChange {
71-
readonly scriptPath: string;
72-
readonly environmentPath?: string;
73-
readonly expectedEnvironmentPath?: string;
74-
}
75-
7668
interface SelectedBaseInterpreter {
7769
readonly environment: PythonEnvironment;
7870
readonly canonicalPath: string;
@@ -277,13 +269,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable {
277269
environmentPath = environment.environmentPath.fsPath;
278270
}
279271

280-
const updates: {
281-
readonly uri: Uri;
282-
readonly scriptPath: string;
283-
readonly before: PythonEnvironment | undefined;
284-
readonly needsPersistence: boolean;
285-
readonly shouldNotify: boolean;
286-
}[] = [];
272+
const updates: PendingScriptUpdate[] = [];
287273
for (const script of scripts) {
288274
const before = await this.getAssociationForMutation(script.scriptPath);
289275
const hadPersistedAssociation = this.fsPathToPersistedEnvPath.has(script.scriptPath);
@@ -371,7 +357,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable {
371357
: environment;
372358
}
373359

374-
private getScriptUris(scope: SetEnvironmentScope): { readonly uri: Uri; readonly scriptPath: string }[] {
360+
private getScriptUris(scope: SetEnvironmentScope): ScriptReference[] {
375361
const candidates = scope instanceof Uri ? [scope] : Array.isArray(scope) ? scope : undefined;
376362
if (
377363
!candidates ||
@@ -381,7 +367,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable {
381367
throw new Error('Inline-script environment selection requires one or more local file URIs.');
382368
}
383369

384-
const scripts: { readonly uri: Uri; readonly scriptPath: string }[] = [];
370+
const scripts: ScriptReference[] = [];
385371
const seen = new Set<string>();
386372
for (const candidate of candidates) {
387373
const scriptPath = normalizePath(candidate.fsPath);
@@ -679,7 +665,8 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable {
679665
try {
680666
await this.updatePersistedAssociations([{ scriptPath, expectedEnvironmentPath }]);
681667
if (
682-
this.fsPathToPersistedEnvPath.get(scriptPath) === expectedEnvironmentPath &&
668+
normalizePath(this.fsPathToPersistedEnvPath.get(scriptPath) ?? '') ===
669+
normalizePath(expectedEnvironmentPath) &&
683670
this.isCurrentAssociationRevision(scriptPath, revision)
684671
) {
685672
const old = this.fsPathToEnv.get(scriptPath);
@@ -1284,3 +1271,22 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable {
12841271
this._onDidChangeEnvironment.dispose();
12851272
}
12861273
}
1274+
1275+
type PersistedInlineScriptEnvironments = Record<string, string>;
1276+
1277+
interface PersistedAssociationChange {
1278+
readonly scriptPath: string;
1279+
readonly environmentPath?: string;
1280+
readonly expectedEnvironmentPath?: string;
1281+
}
1282+
1283+
interface ScriptReference {
1284+
readonly uri: Uri;
1285+
readonly scriptPath: string;
1286+
}
1287+
1288+
interface PendingScriptUpdate extends ScriptReference {
1289+
readonly before: PythonEnvironment | undefined;
1290+
readonly needsPersistence: boolean;
1291+
readonly shouldNotify: boolean;
1292+
}

0 commit comments

Comments
 (0)