Fix intermittent "The term 'Get-Command' is not recognized" failures during recursive analysis - #2206
Jesse Houwing (jessehouwing) wants to merge 47 commits into
Conversation
Co-authored-by: jessehouwing <4173387+jessehouwing@users.noreply.github.com>
Co-authored-by: jessehouwing <4173387+jessehouwing@users.noreply.github.com>
Co-authored-by: jessehouwing <4173387+jessehouwing@users.noreply.github.com>
Co-authored-by: jessehouwing <4173387+jessehouwing@users.noreply.github.com>
Co-authored-by: jessehouwing <4173387+jessehouwing@users.noreply.github.com>
Co-authored-by: jessehouwing <4173387+jessehouwing@users.noreply.github.com>
Co-authored-by: jessehouwing <4173387+jessehouwing@users.noreply.github.com>
… path Co-authored-by: jessehouwing <4173387+jessehouwing@users.noreply.github.com>
…issue-2205 Fix intermittent "The term 'Get-Command' is not recognized" failures during recursive analysis
There was a problem hiding this comment.
Pull request overview
This PR hardens PSScriptAnalyzer’s command-resolution path to avoid intermittent PowerShell engine/runspace-affinity failures (notably Get-Command resolution and CommandInfo.Parameters access) from causing a cascading, process-long failure during recursive/parallel analysis.
Changes:
- Adds retry logic for transient
Get-Commandresolution failures and avoids permanently poisoning theCommandInfoCachewhen a cachedLazy<CommandInfo>faults. - Makes
UseCorrectCasingresilient toInvalidOperationException/NullReferenceExceptionfromCommandInfo.Parameters, retrying via a fresh lookup and skipping only parameter-casing when parameters can’t be determined. - Adds a Linux-only regression test exercising recursive analysis with the reported settings under
-ErrorAction Stop.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Tests/Rules/Issue2205/PSScriptAnalyzerSettings.psd1 | Adds the settings file used by the regression test scenario. |
| Tests/Rules/Issue2205.tests.ps1 | Adds a regression test for recursive analysis under -ErrorAction Stop (Linux). |
| Rules/UseCorrectCasing.cs | Adds a retry-and-skip path for parameter casing when parameter metadata can’t be reliably retrieved. |
| Engine/CommandInfoCache.cs | Adds retry logic for transient Get-Command resolution failures and evicts faulted cached entries to prevent permanent poisoning. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| It "does not fail the analysis when a command lookup hits the runspace affinity problem" -Skip:(-not $IsLinux) { | ||
| $settingsPath = Join-Path $PSScriptRoot 'Issue2205/PSScriptAnalyzerSettings.psd1' | ||
| # $PSScriptRoot is <repo>/Tests/Rules, so two levels up is the repository root. | ||
| $repositoryRoot = (Resolve-Path (Join-Path $PSScriptRoot '..' '..')).Path | ||
|
|
||
| Invoke-ScriptAnalyzer -Path $repositoryRoot -Recurse -Settings $settingsPath -ErrorAction Stop | Out-Null |
| @@ -0,0 +1,12 @@ | |||
| # Copyright (c) Microsoft Corporation. All rights reserved. | |||
Co-authored-by: jessehouwing <4173387+jessehouwing@users.noreply.github.com>
Co-authored-by: jessehouwing <4173387+jessehouwing@users.noreply.github.com>
Co-authored-by: jessehouwing <4173387+jessehouwing@users.noreply.github.com>
|
A more permanent solution may en to serialize access to the runspace. At the repo sizes I work at this causes minimal perf overhead and stabilizes the output from the script analyzer |
…lls-to-sequential Serialize CommandInfo lookups onto a single dedicated runspace
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (3)
Engine/CommandInfoCache.cs:99
- GetOrAdd(key, value) eagerly allocates a new Lazy on every lookup, even when the key is already cached. Since cache hits are expected to dominate (and are intentionally lock-free), this adds avoidable allocations/GC pressure in the hot path. Use the valueFactory overload so the Lazy is only created on cache misses.
var lazyCommandInfo = _commandInfoCache.GetOrAdd(key, new Lazy<CommandInfo>(() => GetCommandInfoInternal(commandName, commandTypes)));
Engine/CommandInfoCache.cs:62
- This comment refers to a “finalizer path”, but CommandInfoCache does not define a finalizer; Dispose(bool) is only called from Dispose() unless a derived type adds a finalizer. The wording is misleading for readers trying to reason about disposal semantics.
// Always take the lock, also on the finalizer path, so that 'disposed' is never
// published without the runspace being disposed along with it and so that the runspace
// cannot be disposed while a lookup is in flight.
Tests/Engine/CommandInfoCacheConcurrency.tests.ps1:35
- Task.WaitAll(tasks) has no timeout; if a regression causes a deadlock/hang, the test run can stall indefinitely. Add a bounded wait and fail fast on timeout to keep CI reliable.
Task.WaitAll(tasks);
|
Jesse Houwing (@jessehouwing) Thanks for your initiative to take this on. There have been a few issues with CommandIssuecache concurrency where PSSA errored. As far as I could track it down it's even root caused in PowerShell engine internals itself not being thread safe. I attempted a fix in PowerShell, which is a good read on related PowerShell and PSSA issues. |
Co-authored-by: jessehouwing <4173387+jessehouwing@users.noreply.github.com>
Co-authored-by: jessehouwing <4173387+jessehouwing@users.noreply.github.com>
Co-authored-by: jessehouwing <4173387+jessehouwing@users.noreply.github.com>
Co-authored-by: jessehouwing <4173387+jessehouwing@users.noreply.github.com>
…-workflow Add isolated cross-platform PSScriptAnalyzer performance benchmarks
Co-authored-by: jessehouwing <4173387+jessehouwing@users.noreply.github.com>
…mver Prevent runspace races during analyzer command metadata access
|
Christoph Bergmeister (@bergmeister) Added a perftest, found 2 more bugs while exercising the code under test. https://github.com/jessehouwing/PSScriptAnalyzer/actions/runs/34888380569 Had to re-run the "upstream" (1.25@main) several times until I got correct results. The fork is slower (in my eyes acceptable, but I'm not running against 10000s of powershell files), but consistently succeeds. I added the perf test as a workflow file to this branch, so you can easily run it yourself. |
Co-authored-by: jessehouwing <4173387+jessehouwing@users.noreply.github.com>
|
Jesse Houwing (@jessehouwing) Interesting results that cold runs are 3x slower but warm runs 36% faster. If this genuinely improves reliability, then maybe the better compromise would be to make this an optional command switch? I've seen many sporadic errors reported (and seen some myself as well) so although rare, definitely not super rare and worth providing a solution for. Thoughts Andy Jordan (@andyleejordan) ? |
Co-authored-by: jessehouwing <4173387+jessehouwing@users.noreply.github.com>
Co-authored-by: jessehouwing <4173387+jessehouwing@users.noreply.github.com>
Co-authored-by: jessehouwing <4173387+jessehouwing@users.noreply.github.com>
…cs-gathering Disable benchmark metrics and consolidate performance comparisons
Performance across all combinationsOptional lock/cache metrics are disabled. Times are medians of fresh-process cold/warm samples; lower is better.
Where :
|
|
In my tests so far I haven't been able to get clean results once from v1.25. And there are multiple bugs happening all at once. I may need to go over all the changes to clean some stuff up. I've been iterating over it using copilot and it introduced a few "retry loops" that may no longer be needed. And I added in a telemetry thingy, that may need to be ripped out again. It's now also faster when running warm. And got the cold perf to 2x on windows, still 3x on ubuntu. The issues are much easier to reproduce when using |
|
Here's al log where the issues reproduce very consistently: Somehow Ubuntu-latest seems to more consistently hit the problem. |
Co-authored-by: jessehouwing <4173387+jessehouwing@users.noreply.github.com>
Co-authored-by: jessehouwing <4173387+jessehouwing@users.noreply.github.com>
Co-authored-by: jessehouwing <4173387+jessehouwing@users.noreply.github.com>
Co-authored-by: jessehouwing <4173387+jessehouwing@users.noreply.github.com>
Make engine retries verifiable and centralize metadata recovery
Co-authored-by: jessehouwing <4173387+jessehouwing@users.noreply.github.com>
Expand perf workflow to benchmark `perf` with and without `DISABLE_ENGINE_RETRIES`
Co-authored-by: jessehouwing <4173387+jessehouwing@users.noreply.github.com>
…isons Restore benchmark comparisons and explicit retry variants
- Drop retry loops and DISABLE_ENGINE_RETRIES flag from CommandInfoCache; failed lookups evict the cache entry and return null gracefully - Delete PerformanceTelemetry and all metrics collection from the engine, benchmark harness, workflows and tests - Refactor GetCommandMetadata to Func<CommandInfo, T> - Add lookup-count and module-qualified resolution tests - Rework Issue2205 repro into generic ParallelRuleExecution test with a bounded synthetic workload, no Linux filter Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Benchmark now compares upstream@main against fork@main only, in preparation for merging the perf branch to main. Also makes source a proper matrix dimension. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
I think this is as much as I can squeeze out of this today:
The numbers are so close now, that I'd propose evaluating these changes as the new baseline. |
Windows PowerShell 5.1 compiles Add-Type definitions with the legacy C# 5 compiler, which rejects the null-conditional operator and expression-bodied members used by the new test fixtures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
While testing I've observed the following intermittent/flakey errors in 1.25, which no longer appear after applying this PR: These issues are easy to reproduce, especially when using See for example: https://github.com/jessehouwing/PSScriptAnalyzer/commit/b8472db30f8bf1d534c952aa6535d1ae6607b382/checks/104292570111/logs |
This pull request makes significant improvements to the thread safety, reliability, and performance of the
CommandInfoCacheand related PowerShell command metadata retrieval in the ScriptAnalyzer engine. The main focus is on serializing access to PowerShell runspaces, improving exception handling and cache eviction, and adding new APIs for efficient and safe parameter metadata access. Additionally, a newCommandParameterSnapshotclass is introduced to provide detached, thread-safe parameter information, which is leveraged in the analysis logic.Thread safety and runspace management improvements:
RunspacePoolwith a singleRunspace, and serializes all access to it with a re-entrant lock (_runspaceLock) to avoid concurrency issues in the PowerShell engine. All command lookups and metadata queries are now thread-safe and cannot run concurrently, addressing known engine bugs. ([1], [2])Caching and exception handling enhancements:
Lazy<CommandInfo>throws) so that transient errors don't poison the cache. Only successful lookups are retained. (Engine/CommandInfoCache.csL73-R140)New APIs and parameter metadata snapshotting:
GetCommandParameters,GetCommandParameterSets, etc.) that always operate under the runspace lock for safety. (Engine/CommandInfoCache.csL120-R374)CommandParameterSnapshotclass that provides a detached, immutable view of parameter metadata, safe for use outside the runspace. (Engine/CommandParameterSnapshot.csR1-R24)Refactoring and improved analysis logic:
Helper.GetExportedFunctionto use the new parameter snapshot API, eliminating re-entrance into the runspace and improving correctness and performance when analyzing exported module members. ([1], [2])GetModuleManifestForAnalysismethod that uses a shared cache for module manifest validation during a single analysis, reducing redundant work. (Engine/Helper.csR333-R341)Other improvements:
Errors observed while testing and now corrected
While testing I've observed the following intermittent/flakey errors in 1.25, which no longer appear after applying this PR:
These issues are easy to reproduce, especially when using
-recurse.See for example: https://github.com/jessehouwing/PSScriptAnalyzer/commit/b8472db30f8bf1d534c952aa6535d1ae6607b382/checks/104292570111/logs
Most recent performance
The numbers are so close now, that I'd propose evaluating these changes as the new baseline.
Linked issues
Fixes: #2205
Fixes: #1708
Likely also resolves: #1351