Fix intermittent "The term 'Get-Command' is not recognized" failures during recursive analysis

Engine/CommandInfoCache.cs

@@ -3,6 +3,7 @@
 
 using System;
 using System.Collections.Concurrent;
+using System.Collections.Generic;
 using System.Management.Automation;
 using System.Linq;
 using System.Management.Automation.Runspaces;
@@ -14,8 +15,25 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer
     /// </summary>
     internal class CommandInfoCache : IDisposable
     {
+        /// <summary>
+        /// Number of times a command lookup is attempted before giving up.
+        /// Command lookups can fail transiently because the PowerShell engine is not thread safe,
+        /// see https://github.com/PowerShell/PowerShell/issues/4003
+        /// </summary>
+        private const int MaxLookupAttempts = 3;
+
         private readonly ConcurrentDictionary<CommandLookupKey, Lazy<CommandInfo>> _commandInfoCache;
-        private readonly RunspacePool _runspacePool;
+
+        /// <summary>
+        /// Guards all access to <see cref="_runspace"/> so that only one thread at a time drives the
+        /// PowerShell engine. The engine is not thread safe, so concurrent lookups can fail transiently,
+        /// see https://github.com/PowerShell/PowerShell/issues/4003.
+        /// A monitor is used rather than a semaphore because it is re-entrant, which avoids a deadlock
+        /// should a lookup ever end up calling back into the cache on the same thread.
+        /// </summary>
+        private readonly object _runspaceLock = new object();
+
+        private readonly Runspace _runspace;
         private bool disposed = false;
 
         /// <summary>
@@ -24,11 +42,13 @@ internal class CommandInfoCache : IDisposable
         public CommandInfoCache()
         {
             _commandInfoCache = new ConcurrentDictionary<CommandLookupKey, Lazy<CommandInfo>>();
-            _runspacePool = RunspaceFactory.CreateRunspacePool(1, 10);
-            _runspacePool.Open();
+            // A single runspace rather than a pool: all lookups are serialized on it, so that the
+            // PowerShell engine is never driven concurrently.
+            _runspace = RunspaceFactory.CreateRunspace();
+            _runspace.Open();
         }
 
-        /// <summary>Dispose the runspace pool</summary>
+        /// <summary>Dispose the runspace</summary>
         public void Dispose()
         {
             Dispose(true);
@@ -37,17 +57,23 @@ public void Dispose()
 
         protected virtual void Dispose(bool disposing)
         {
-            if ( disposed )
+            // 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.
+            lock (_runspaceLock)
             {
-                return;
-            }
+                if ( disposed )
+                {
+                    return;
+                }
 
-            if ( disposing )
-            {
-                _runspacePool.Dispose();
-            }
+                disposed = true;
 
-            disposed = true;
+                if ( disposing )
+                {
+                    _runspace.Dispose();
+                }
+            }
         }
 
         /// <summary>
@@ -70,7 +96,21 @@ public CommandInfo GetCommandInfo(string commandName, CommandTypes? commandTypes
                 return GetCommandInfoInternal(commandName, commandTypes);
             }
             // Atomically either use PowerShell to query a command info object, or fetch it from the cache
-            return _commandInfoCache.GetOrAdd(key, new Lazy<CommandInfo>(() => GetCommandInfoInternal(commandName, commandTypes))).Value;
+            var lazyCommandInfo = _commandInfoCache.GetOrAdd(key, new Lazy<CommandInfo>(() => GetCommandInfoInternal(commandName, commandTypes)));
+            try
+            {
+                return lazyCommandInfo.Value;
+            }
+            catch
+            {
+                // Lazy<T> caches exceptions forever, which would make every subsequent lookup of this
+                // command fail for the lifetime of the process. Evict the entry so that the next lookup
+                // can try again. Only remove the faulted instance so that a replacement that another
+                // thread may already have added is left alone.
+                ((ICollection<KeyValuePair<CommandLookupKey, Lazy<CommandInfo>>>)_commandInfoCache)
+                    .Remove(new KeyValuePair<CommandLookupKey, Lazy<CommandInfo>>(key, lazyCommandInfo));
+                throw;
+            }
         }
 
 
@@ -99,26 +139,56 @@ private CommandInfo GetCommandInfoInternal(string cmdName, CommandTypes? command
             // For more details see https://github.com/PowerShell/PowerShell/issues/9308
             actualCmdName = WildcardPattern.Escape(actualCmdName);
 
-            using (var ps = System.Management.Automation.PowerShell.Create())
+            for (int attempt = 1; ; attempt++)
             {
-                ps.RunspacePool = _runspacePool;
-
-                ps.AddCommand("Get-Command")
-                    .AddParameter("Name", actualCmdName)
-                    .AddParameter("ErrorAction", "SilentlyContinue");
-
-                if (commandType != null)
-                {
-                    ps.AddParameter("CommandType", commandType);
-                }
-
-                if (!string.IsNullOrEmpty(moduleName))
+                // Serialize all use of the PowerShell engine. Only cache misses reach this point;
+                // lookups that are already cached are served without taking the lock.
+                lock (_runspaceLock)
                 {
-                    ps.AddParameter("Module", moduleName);
+                    if (disposed)
+                    {
+                        return null;
+                    }
+
+                    using (var ps = System.Management.Automation.PowerShell.Create())
+                    {
+                        ps.Runspace = _runspace;
+
+                        ps.AddCommand("Get-Command")
+                            .AddParameter("Name", actualCmdName)
+                            .AddParameter("ErrorAction", "SilentlyContinue");
+
+                        if (commandType != null)
+                        {
+                            ps.AddParameter("CommandType", commandType);
+                        }
+
+                        if (!string.IsNullOrEmpty(moduleName))
+                        {
+                            ps.AddParameter("Module", moduleName);
+                        }
+
+                        try
+                        {
+                            return ps.Invoke<CommandInfo>()
+                                .FirstOrDefault();
+                        }
+                        // 'Get-Command' is invoked with 'SilentlyContinue', so a CommandNotFoundException can only
+                        // mean that the engine failed to resolve 'Get-Command' itself in the runspace.
+                        // That happened intermittently when lookups ran concurrently because the PowerShell engine
+                        // is not thread safe, see https://github.com/PowerShell/PowerShell/issues/4003 and
+                        // https://github.com/PowerShell/PSScriptAnalyzer/issues/2205
+                        // Lookups are serialized now, so this should no longer occur, but the retry is kept as a
+                        // safety net for hosts that drive the engine from other threads at the same time.
+                        catch (CommandNotFoundException)
+                        {
+                            if (attempt >= MaxLookupAttempts)
+                            {
+                                return null;
+                            }
+                        }
+                    }
                 }
-
-                return ps.Invoke<CommandInfo>()
-                    .FirstOrDefault();
             }
         }
 

Rules/UseCorrectCasing.cs

@@ -128,10 +128,17 @@ public override IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string file
                     // It's a known issue that objects from PowerShell can have a runspace affinity,
                     // therefore if that happens, we query a fresh object instead of using the cache.
                     // https://github.com/PowerShell/PowerShell/issues/4003
-                    catch (InvalidOperationException)
+                    // The affinity problem surfaces as an InvalidOperationException or as a
+                    // NullReferenceException, see https://github.com/PowerShell/PSScriptAnalyzer/issues/1708
+                    catch (Exception exception) when (exception is InvalidOperationException || exception is NullReferenceException)
                     {
-                        commandInfo = Helper.Instance.GetCommandInfo(commandName, bypassCache: true);
-                        availableParameters = commandInfo.Parameters;
+                        availableParameters = GetParametersFromFreshCommandInfo(commandName);
+                    }
+                    if (availableParameters is null)
+                    {
+                        // The parameters of this command cannot be determined reliably,
+                        // so skip the parameter casing check instead of failing the analysis.
+                        continue;
                     }
                     foreach (var commandParameterAst in commandParameterAsts)
                     {
@@ -161,6 +168,22 @@ public override IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string file
             }
         }
 
+        /// <summary>
+        /// Queries a fresh <see cref="CommandInfo"/> object to work around the runspace affinity problem
+        /// of the PowerShell engine and returns its parameters, or null if they cannot be determined.
+        /// </summary>
+        private Dictionary<string, ParameterMetadata> GetParametersFromFreshCommandInfo(string commandName)
+        {
+            try
+            {
+                return Helper.Instance.GetCommandInfo(commandName, bypassCache: true)?.Parameters;
+            }
+            catch (Exception exception) when (exception is InvalidOperationException || exception is NullReferenceException)
+            {
+                return null;
+            }
+        }
+
         /// <summary>
         /// For a command like "gci -path c:", returns the extent of "gci" in the command
         /// </summary>

Tests/Engine/CommandInfoCacheConcurrency.tests.ps1

@@ -0,0 +1,64 @@
+# Copyright (c) Microsoft Corporation. All rights reserved.
+# Licensed under the MIT License.
+
+Describe "Concurrent command lookups" {
+    BeforeAll {
+        # Run the analyzer once so that the singleton Helper is created by the cmdlet. Touching
+        # Helper.Instance before that would install a helper without a command invocation context,
+        # which breaks every later analysis in this process.
+        $null = Invoke-ScriptAnalyzer -ScriptDefinition 'Get-Item -Path .'
+
+        # The concurrency driver is written in C# so that the lookups really do run on separate
+        # threads. Invoking a PowerShell script block on a thread pool thread would introduce
+        # runspace affinity problems of its own and would not test the command info cache.
+        $analyzerAssembly = [Microsoft.Windows.PowerShell.ScriptAnalyzer.Helper].Assembly.Location
+        Add-Type -IgnoreWarnings -WarningAction SilentlyContinue -ReferencedAssemblies $analyzerAssembly, ([System.Management.Automation.PSObject].Assembly.Location) -TypeDefinition @'
+using System.Threading.Tasks;
+using Microsoft.Windows.PowerShell.ScriptAnalyzer;
+
+public static class ConcurrentCommandLookup
+{
+    public static string[] Lookup(string[] commandNames)
+    {
+        var helper = Helper.Instance;
+        var tasks = new Task<string>[commandNames.Length];
+        for (int i = 0; i < commandNames.Length; i++)
+        {
+            string name = commandNames[i];
+            tasks[i] = Task.Run(() =>
+            {
+                var commandInfo = helper.GetCommandInfo(name);
+                return commandInfo == null ? null : commandInfo.Name;
+            });
+        }
+
+        Task.WaitAll(tasks);
+
+        var results = new string[tasks.Length];
+        for (int i = 0; i < tasks.Length; i++)
+        {
+            results[i] = tasks[i].Result;
+        }
+
+        return results;
+    }
+}
+'@
+    }
+
+    It "resolves commands from several threads without failing" {
+        $commandNames = @(
+            'Get-ChildItem', 'Where-Object', 'ForEach-Object', 'Get-Content', 'Write-Output',
+            'Test-Path', 'Get-Command', 'Select-Object', 'Sort-Object', 'Measure-Object'
+        ) * 4
+
+        # A lookup that hits the thread safety problem throws, which fails the test.
+        $results = [ConcurrentCommandLookup]::Lookup($commandNames)
+
+        $results.Count | Should -Be $commandNames.Count
+        # A failed lookup returns null, so every entry must name the command that was requested.
+        for ($i = 0; $i -lt $commandNames.Count; $i++) {
+            $results[$i] | Should -BeExactly $commandNames[$i]
+        }
+    }
+}

Tests/Rules/Issue2205.tests.ps1

@@ -0,0 +1,12 @@
+# Copyright (c) Microsoft Corporation. All rights reserved.
+# Licensed under the MIT License.
+
+Describe 'Issue 2205' {
+    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
+    }
+}

Tests/Rules/Issue2205/PSScriptAnalyzerSettings.psd1

@@ -0,0 +1,53 @@
+@{
+    Severity = @('Error', 'Warning', 'Information')
+    IncludeRules = @(
+        'PSAvoidUsingCmdletAliases', 'PSAvoidDefaultValueForMandatoryParameter',
+        'PSAvoidDefaultValueSwitchParameter', 'PSAvoidGlobalAliases',
+        'PSAvoidGlobalFunctions', 'PSAvoidGlobalVars', 'PSAvoidInvokingEmptyMembers',
+        'PSAvoidNullOrEmptyHelpMessageAttribute', 'PSAvoidShouldContinueWithoutForce',
+        'PSAvoidUsingComputerNameHardcoded', 'PSAvoidUsingConvertToSecureStringWithPlainText',
+        'PSAvoidUsingDeprecatedManifestFields', 'PSAvoidUsingEmptyCatchBlock',
+        'PSAvoidUsingInvokeExpression', 'PSAvoidUsingPlainTextForPassword',
+        'PSAvoidUsingPositionalParameters', 'PSAvoidUsingUsernameAndPasswordParams',
+        'PSAvoidUsingWMICmdlet', 'PSAvoidUsingWriteHost', 'PSMisleadingBacktick',
+        'PSMissingModuleManifestField', 'PSPossibleIncorrectComparisonWithNull',
+        'PSPossibleIncorrectUsageOfAssignmentOperator', 'PSPossibleIncorrectUsageOfRedirectionOperator',
+        'PSProvideCommentHelp', 'PSReservedCmdletChar', 'PSReservedParams',
+        'PSUseApprovedVerbs', 'PSUseBOMForUnicodeEncodedFile', 'PSUseCmdletCorrectly',
+        'PSUseConsistentIndentation', 'PSUseConsistentWhitespace', 'PSUseCorrectCasing',
+        'PSUseDeclaredVarsMoreThanAssignments', 'PSUseLiteralInitializerForHashtable',
+        'PSUseOutputTypeCorrectly', 'PSUsePSCredentialType', 'PSUseSingularNouns',
+        'PSUseToExportFieldsInManifest', 'PSUseUTF8EncodingForHelpFile'
+    )
+    ExcludeRules = @(
+        'PSAvoidUsingWriteHost', 'PSAvoidUsingPositionalParameters', 'PSUseApprovedVerbs',
+        'PSProvideCommentHelp', 'PSAvoidGlobalVars', 'PSAvoidGlobalFunctions',
+        'PSUseSingularNouns', 'PSUseOutputTypeCorrectly'
+    )
+    Rules = @{
+        PSUseConsistentIndentation = @{
+            Enable = $true
+            IndentationSize = 4
+            PipelineIndentation = 'IncreaseIndentationForFirstPipeline'
+            Kind = 'space'
+        }
+        PSUseConsistentWhitespace = @{
+            Enable = $true
+            CheckInnerBrace = $true
+            CheckOpenBrace = $true
+            CheckOpenParen = $true
+            CheckOperator = $true
+            CheckPipe = $true
+            CheckPipeForRedundantWhitespace = $false
+            CheckSeparator = $true
+            CheckParameter = $false
+            IgnoreAssignmentOperatorInsideHashTable = $true
+        }
+        PSUseCompatibleCmdlets = @{ Enable = $false }
+        PSUseCorrectCasing = @{ Enable = $true }
+        PSAvoidUsingCmdletAliases = @{ Enable = $true; allowlist = @() }
+        PSAlignAssignmentStatement = @{ Enable = $false; CheckHashtable = $false }
+        PSPlaceOpenBrace = @{ Enable = $true; OnSameLine = $true; NewLineAfter = $true; IgnoreOneLineBlock = $true }
+        PSPlaceCloseBrace = @{ Enable = $true; NewLineAfter = $true; IgnoreOneLineBlock = $true; NoEmptyLineBefore = $false }
+    }
+}