diff --git a/src/csharp/Pester/VerbsPatcher.cs b/src/csharp/Pester/VerbsPatcher.cs index b56473175..91d04c11c 100644 --- a/src/csharp/Pester/VerbsPatcher.cs +++ b/src/csharp/Pester/VerbsPatcher.cs @@ -15,6 +15,14 @@ public static class VerbsPatcher // Concurrent bag in case we start this multiple times, and god forbid in parallel. private static ConcurrentBag s_tasks = new ConcurrentBag(); + // s_validVerbs is PowerShell's own process-global static Dictionary (one instance per + // process, shared by every runspace). Dictionary is not thread-safe, so when + // Run.Parallel imports Pester from several ForEach-Object -Parallel runspaces at once, the + // concurrent writes can tear the internal bucket array and throw IndexOutOfRangeException. + // VerbsPatcher lives in Pester.dll, which is loaded once per process, so this static lock + // object is shared across all runspaces and serializes every mutation of that dictionary. + private static readonly object s_lock = new object(); + public static void AllowShouldVerb(int powershellVersion) { var should = "Should"; @@ -25,17 +33,41 @@ public static void AllowShouldVerb(int powershellVersion) // private static readonly Dictionary s_validVerbs; Dictionary validVerbs = (Dictionary)verbsField.GetValue(null); - // Overwrite when we call this multiple times. - validVerbs[should] = true; // The bool does not matter. + // Only the first import structurally mutates the shared dictionary; later parallel + // workers see the key already present and become no-ops. The lock prevents concurrent + // writes from corrupting the dictionary. Do not use TryAdd, it is not available on the + // net462 target. + // + // We deliberately do NOT double-check with an unlocked ContainsKey before taking the + // lock. That would be an optimization to skip the lock once patched, but reading a + // plain Dictionary outside the lock while another runspace is inserting inside the lock + // is itself a data race: the insert can trigger an internal bucket-array resize, and a + // concurrent unlocked read can observe the torn array and throw IndexOutOfRange, the + // exact failure this lock exists to prevent. AllowShouldVerb runs once per runspace at + // import time and the lock only holds a ContainsKey plus at most one insert, so there + // is no contention worth optimizing here. A correct fast path would need a separate + // volatile flag rather than an unlocked read of this non-thread-safe dictionary. + lock (s_lock) + { + if (!validVerbs.ContainsKey(should)) + { + validVerbs[should] = true; // The bool does not matter. + } + } s_tasks.Add(Task.Run(async () => { await Task.Delay(5_000); try { - if (validVerbs.ContainsKey(should)) + // Serialize the removal behind the same process-global lock so it cannot race + // with a concurrent insert from another runspace. No await inside the lock. + lock (s_lock) { - validVerbs.Remove(should); + if (validVerbs.ContainsKey(should)) + { + validVerbs.Remove(should); + } } } catch { }