Fix PATH caching in CommandDiscovery code - #27809
Fix PATH caching in CommandDiscovery code#27809Dongbo Wang (daxian-dbw) wants to merge 3 commits into
CommandDiscovery code#27809Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR fixes an engine-level PATH caching bug in CommandDiscovery where cached lookup paths could be mutated during relative-path resolution, causing subsequent command discovery to resolve against an old working directory. It also adds a regression test to validate that Get-Command resolves relative PATH entries based on the current $PWD.
Changes:
- Updated
CommandDiscovery.GetLookupDirectoryPaths()to return a freshLookupPathCollectioncopy each time, preventing cache mutation during relative path resolution. - Simplified PATH cache storage by caching tokenized entries as a
List<string>and rebuilding when PATH changes/unsets. - Added a Pester test covering relative PATH resolution across directory changes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/System.Management.Automation/engine/CommandDiscovery.cs | Fixes PATH lookup caching by returning a copy per lookup to avoid cache mutation when resolving relative entries. |
| test/powershell/Modules/Microsoft.PowerShell.Core/Get-Command.Tests.ps1 | Adds regression coverage for resolving relative PATH entries based on the current working directory. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Ilya (iSazonov)
left a comment
There was a problem hiding this comment.
LGTM with minor comments.
| _pathCacheKey = path; | ||
|
|
||
| if (_pathCacheKey != null) | ||
| if (path is null) |
There was a problem hiding this comment.
For empty path we get the same empty collection _cachedLookupPaths = new List<string>()
| if (path is null) | |
| if (string.IsNullorEmpty(path)) |
And line 1210: _pathCacheKey = path;
There was a problem hiding this comment.
Good point. Updated as suggested, please take another look.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (3)
test/powershell/Modules/Microsoft.PowerShell.Core/Get-Command.Tests.ps1:311
- This test prepends the relative entry to the existing PATH (
...${pathSep}$env:PATH). That meansbye.ps1could be found later in the original PATH on some dev machines/CI images, making theShould -BeNullOrEmptyassertion flaky. Consider isolating PATH to just the relative entry for the duration of this test.
$dirSep = [System.IO.Path]::DirectorySeparatorChar
$pathSep = [System.IO.Path]::PathSeparator
Set-Location $subDir1
$env:PATH = ".${dirSep}tools${pathSep}$env:PATH"
src/System.Management.Automation/engine/CommandDiscovery.cs:1215
- The comment says this branch is for when PATH is null, but the condition is
string.IsNullOrEmpty(path)(null or empty). Please update the comment so it matches behavior.
// Cache an empty collection when PATH is null (unset).
src/System.Management.Automation/engine/CommandDiscovery.cs:1251
GetLookupDirectoryPaths()now allocates a newLookupPathCollectionon every call, andLookupPathCollection(IEnumerable<string>)adds items viaAdd(), which does an O(n)Contains()check each time. That makes copying O(n^2) per command lookup (even when PATH is unchanged), which may regress command discovery performance for large PATH values. Consider adding a fast-copy path (e.g., an internal ctor/Clone that copies the backing list without duplicate checks, since the cached list can already be de-duplicated when built).
// The returned instance will be mutated in 'CommandPathSearch.ResolveCurrentDirectoryInLookupPaths' when resolving relative paths,
// which depends on user's current working directory. So, we need to return a copy of the lookup paths to keep the cache intact.
return new LookupPathCollection(_cachedLookupPaths);
PR Summary
The method
GetLookupDirectoryPathsalways returns the cached instance of path collection. However, the returned collection gets mutated inCommandPathSearch.ResolveCurrentDirectoryInLookupPathsto resolve the relative paths such as.\toolsbased on the user's current working directory$PWD, so for example,.\toolsgets replaced withcwd-1\toolsin the cached instance. Then, when the user changes to a different working directorycwd-2, command discovery won't find the executable or ps1 script undercwd-2/toolsas expected, but those executables undercwd-1\toolswill always be discoverable no matter what the$PWDis. That behavior is incorrect.This pull request improves the caching logic of
GetLookupDirectoryPathsand makes it return a copy of the path collection, so the mutation happens to the returned copy, and the cached instance is kept intact. So, for every command search, theCommandPathSearchwill resolve relative paths against the$PWDthat the user is located at that time.Core engine improvements:
GetLookupDirectoryPathsinCommandDiscovery.csto ensure that relative paths inPATHare resolved based on the current working directory by returning a copy of the cached lookup paths, preventing mutation of the cache during command resolution.PATHcache, removing redundant fields and ensuring that the cache is invalidated and rebuilt properly when the environment variable changes.Testing enhancements:
Get-Command.Tests.ps1that verifiesGet-Commandcorrectly resolves relative paths inPATHdepending on the current working directory, covering scenarios where scripts exist in different directories.PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright header