terminal: ignore stale env file refreshes

Make env-file change tests await the refresh directly instead of sleeping, and prevent an older asynchronous refresh from overwriting newer terminal variables.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: eleanorjboyd

src/features/terminal/terminalEnvVarInjector.ts

@@ -29,6 +29,7 @@ export class TerminalEnvVarInjector implements Disposable {
     private disposables: Disposable[] = [];
     // Track which .env variables we've set for each workspace to avoid clearing shell activation variables
     private envVarKeys: Map<string, Set<string>> = new Map();
+    private refreshGenerations: Map<string, number> = new Map();
     private envFileNotificationShown = false;
 
     constructor(
@@ -49,19 +50,17 @@ export class TerminalEnvVarInjector implements Disposable {
             this.envVarManager.onDidChangeEnvironmentVariables((args) => {
                 if (!args.uri) {
                     // No specific URI, reload all workspaces
-                    this.updateEnvironmentVariables().catch((error) => {
+                    return this.updateEnvironmentVariables().catch((error) => {
                         traceError('Failed to update environment variables:', error);
                     });
-                    return;
                 }
 
                 const affectedWorkspace = getWorkspaceFolder(args.uri);
                 if (!affectedWorkspace) {
                     // No workspace folder found for this URI, reloading all workspaces
-                    this.updateEnvironmentVariables().catch((error) => {
+                    return this.updateEnvironmentVariables().catch((error) => {
                         traceError('Failed to update environment variables:', error);
                     });
-                    return;
                 }
 
                 // Check if env file injection is enabled when variables change
@@ -76,7 +75,7 @@ export class TerminalEnvVarInjector implements Disposable {
                     });
                 }
 
-                this.updateEnvironmentVariables(affectedWorkspace).catch((error) => {
+                return this.updateEnvironmentVariables(affectedWorkspace).catch((error) => {
                     traceError('Failed to update environment variables:', error);
                 });
             }),
@@ -105,12 +104,17 @@ export class TerminalEnvVarInjector implements Disposable {
      */
     private async updateEnvironmentVariables(workspaceFolder?: WorkspaceFolder): Promise<void> {
         try {
+            let workspaceUpdates: { folder: WorkspaceFolder; generation: number }[];
             if (workspaceFolder) {
-                // Update only the specified workspace
+                workspaceUpdates = [
+                    {
+                        folder: workspaceFolder,
+                        generation: this.beginWorkspaceRefresh(workspaceFolder),
+                    },
+                ];
                 traceVerbose(
                     `TerminalEnvVarInjector: Updating environment variables for workspace: ${workspaceFolder.uri.fsPath}`,
                 );
-                await this.injectEnvironmentVariablesForWorkspace(workspaceFolder);
             } else {
                 // No provided workspace - update all workspaces
 
@@ -121,9 +125,14 @@ export class TerminalEnvVarInjector implements Disposable {
                 }
 
                 traceVerbose('TerminalEnvVarInjector: Updating environment variables for all workspaces');
-                for (const folder of workspaceFolders) {
-                    await this.injectEnvironmentVariablesForWorkspace(folder);
-                }
+                workspaceUpdates = workspaceFolders.map((folder) => ({
+                    folder,
+                    generation: this.beginWorkspaceRefresh(folder),
+                }));
+            }
+
+            for (const { folder, generation } of workspaceUpdates) {
+                await this.injectEnvironmentVariablesForWorkspace(folder, generation);
             }
 
             traceVerbose('TerminalEnvVarInjector: Environment variable injection completed');
@@ -135,7 +144,10 @@ export class TerminalEnvVarInjector implements Disposable {
     /**
      * Inject environment variables for a specific workspace.
      */
-    private async injectEnvironmentVariablesForWorkspace(workspaceFolder: WorkspaceFolder): Promise<void> {
+    private async injectEnvironmentVariablesForWorkspace(
+        workspaceFolder: WorkspaceFolder,
+        generation: number,
+    ): Promise<void> {
         const workspaceUri = workspaceFolder.uri;
         const workspaceKey = workspaceUri.fsPath;
 
@@ -154,6 +166,9 @@ export class TerminalEnvVarInjector implements Disposable {
                 traceVerbose(
                     `TerminalEnvVarInjector: Env file injection disabled for workspace: ${workspaceUri.fsPath}`,
                 );
+                if (!this.isCurrentWorkspaceRefresh(workspaceFolder, generation)) {
+                    return;
+                }
                 // Clear only the .env variables we previously set, not shell activation variables
                 this.clearTrackedEnvVariables(envVarScope, workspaceKey);
                 return;
@@ -171,6 +186,9 @@ export class TerminalEnvVarInjector implements Disposable {
                     : (await fse.pathExists(defaultEnvFilePath))
                       ? defaultEnvFilePath
                       : undefined;
+            if (!this.isCurrentWorkspaceRefresh(workspaceFolder, generation)) {
+                return;
+            }
             if (activeEnvFilePath) {
                 traceVerbose(`TerminalEnvVarInjector: Using env file: ${activeEnvFilePath}`);
             } else {
@@ -214,6 +232,17 @@ export class TerminalEnvVarInjector implements Disposable {
         }
     }
 
+    private beginWorkspaceRefresh(workspaceFolder: WorkspaceFolder): number {
+        const workspaceKey = workspaceFolder.uri.fsPath;
+        const generation = (this.refreshGenerations.get(workspaceKey) ?? 0) + 1;
+        this.refreshGenerations.set(workspaceKey, generation);
+        return generation;
+    }
+
+    private isCurrentWorkspaceRefresh(workspaceFolder: WorkspaceFolder, generation: number): boolean {
+        return this.refreshGenerations.get(workspaceFolder.uri.fsPath) === generation;
+    }
+
     /**
      * Offer to enable env file injection for the affected workspace folder or suppress future reminders.
      */

src/test/features/terminalEnvVarInjector.unit.test.ts

@@ -19,6 +19,7 @@ import {
 import { ActivationStrings, Common } from '../../common/localize';
 import * as logging from '../../common/logging';
 import * as persistentState from '../../common/persistentState';
+import { createDeferred } from '../../common/utils/deferred';
 import * as windowApis from '../../common/window.apis';
 import * as workspaceApis from '../../common/workspace.apis';
 import { EnvVarManager } from '../../features/execution/envVariableManager';
@@ -153,16 +154,21 @@ suite('TerminalEnvVarInjector', () => {
         });
 
         suite('env file changes', () => {
-            let envChangeCallback: ((args: { uri?: Uri; changeType: number }) => void) | undefined;
+            let envChangeCallback: ((args: { uri?: Uri; changeType: number }) => Promise<void>) | undefined;
             let variables: Record<string, string>;
+            let variableReads: Promise<Record<string, string>>[];
             let existingFiles: Set<string>;
             const defaultEnvFile = path.join(testWorkspaceFolder.uri.fsPath, '.env');
             const configuredEnvFile = path.join(testWorkspaceFolder.uri.fsPath, 'configured.env');
 
             setup(() => {
                 variables = {};
+                variableReads = [];
                 existingFiles = new Set();
-                sinon.stub(fse, 'pathExists').callsFake(async (filePath) => existingFiles.has(path.resolve(filePath.toString())));
+                workspaceFoldersValue = undefined;
+                sinon
+                    .stub(fse, 'pathExists')
+                    .callsFake(async (filePath) => existingFiles.has(path.resolve(filePath.toString())));
                 sinon.stub(workspaceApis, 'getWorkspaceFolder').returns(testWorkspaceFolder);
                 envVarManager.reset();
                 envVarManager.setup((m) => m.onDidChangeEnvironmentVariables).returns(
@@ -173,13 +179,12 @@ suite('TerminalEnvVarInjector', () => {
                 );
                 envVarManager
                     .setup((m) => m.getEnvironmentVariables(typeMoq.It.isAny()))
-                    .returns(() => Promise.resolve({ ...variables }));
+                    .returns(() => variableReads.shift() ?? Promise.resolve({ ...variables }));
             });
 
             async function fireChange(changeType: number, filePath = defaultEnvFile): Promise<void> {
                 assert.ok(envChangeCallback);
-                envChangeCallback({ uri: Uri.file(filePath), changeType });
-                await new Promise((resolve) => setTimeout(resolve, 20));
+                await envChangeCallback({ uri: Uri.file(filePath), changeType });
             }
 
             test('creating an env file with injection disabled preserves shell activation variables', async () => {
@@ -195,7 +200,6 @@ suite('TerminalEnvVarInjector', () => {
             test('creating, editing, and deleting an env file updates only its injected variables', async () => {
                 getConfigurationStub.returns(createMockConfig({ useEnvFile: true }) as WorkspaceConfiguration);
                 injector = new TerminalEnvVarInjector(envVarCollection.object, envVarManager.object);
-                await new Promise((resolve) => setTimeout(resolve, 20));
 
                 existingFiles.add(defaultEnvFile);
                 variables = { TERMINAL_PROBE_VALUE: 'created' };
@@ -222,7 +226,7 @@ suite('TerminalEnvVarInjector', () => {
                 existingFiles.add(configuredEnvFile);
                 variables = { CONFIGURED_VALUE: 'configured', PROJECT_VALUE: 'project' };
                 injector = new TerminalEnvVarInjector(envVarCollection.object, envVarManager.object);
-                await new Promise((resolve) => setTimeout(resolve, 20));
+                await fireChange(2, configuredEnvFile);
 
                 existingFiles.delete(configuredEnvFile);
                 variables = { PROJECT_VALUE: 'project' };
@@ -241,7 +245,7 @@ suite('TerminalEnvVarInjector', () => {
                 existingFiles.add(configuredEnvFile);
                 variables = { CONFIGURED_VALUE: 'configured', PROJECT_VALUE: 'project' };
                 injector = new TerminalEnvVarInjector(envVarCollection.object, envVarManager.object);
-                await new Promise((resolve) => setTimeout(resolve, 20));
+                await fireChange(2);
 
                 existingFiles.delete(defaultEnvFile);
                 variables = { CONFIGURED_VALUE: 'configured' };
@@ -251,6 +255,27 @@ suite('TerminalEnvVarInjector', () => {
                 sinon.assert.neverCalledWith(mockScopedCollection.delete, 'CONFIGURED_VALUE');
                 sinon.assert.notCalled(mockScopedCollection.clear);
             });
+
+            test('does not let an older env file refresh overwrite newer variables', async () => {
+                getConfigurationStub.returns(createMockConfig({ useEnvFile: true }) as WorkspaceConfiguration);
+                existingFiles.add(defaultEnvFile);
+                const olderRefresh = createDeferred<Record<string, string>>();
+                const newerRefresh = createDeferred<Record<string, string>>();
+                variableReads.push(olderRefresh.promise, newerRefresh.promise);
+                injector = new TerminalEnvVarInjector(envVarCollection.object, envVarManager.object);
+
+                assert.ok(envChangeCallback);
+                const firstChange = envChangeCallback({ uri: Uri.file(defaultEnvFile), changeType: 3 });
+                const secondChange = envChangeCallback({ uri: Uri.file(defaultEnvFile), changeType: 2 });
+
+                newerRefresh.resolve({ NEW_VALUE: 'new' });
+                await secondChange;
+                olderRefresh.resolve({ OLD_VALUE: 'old' });
+                await firstChange;
+
+                sinon.assert.calledOnceWithExactly(mockScopedCollection.replace, 'NEW_VALUE', 'new');
+                sinon.assert.neverCalledWith(mockScopedCollection.replace, 'OLD_VALUE', 'old');
+            });
         });
 
         test('should NOT inject when useEnvFile is false even with python.envFile configured', async () => {