fix: Improve pipeline context isolation and thread safety (#1417)

This PR addresses issue #1417 by implementing proper module-scoped isolation:

**Breaking Changes:**
- `IEnvironmentContext.ContentDirectory` and `WorkingDirectory` are now readonly
  (changed from `{ get; set; }` to `{ get; }`)

**Improvements:**
- Add AsyncLocal cleanup in ModuleRunner to prevent potential context leaks
- Replace `List<T>` with `ConcurrentBag<T>` in ModuleLoggerContainer for lock-free
  thread-safe operations during high-concurrency module execution
- Add documentation explaining PipelineContext scoped lifetime and logger caching safety

**Files Changed:**
- ModuleRunner.cs: Added try/finally to clear AsyncLocal logger context
- IEnvironmentContext.cs: Made ContentDirectory/WorkingDirectory readonly with docs
- EnvironmentContext.cs: Implementation matches interface changes
- PipelineContext.cs: Added XML documentation for scoped DI behavior
- ModuleLoggerContainer.cs: Replaced List+locks with ConcurrentBag+Interlocked

Closes #1417

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

Author: thomhurst

src/ModularPipelines/Context/EnvironmentContext.cs

@@ -27,9 +27,9 @@ public EnvironmentContext(IHostEnvironment hostEnvironment,
 
     public Folder AppDomainDirectory { get; } = AppDomain.CurrentDomain.BaseDirectory!;
 
-    public Folder ContentDirectory { get; set; }
+    public Folder ContentDirectory { get; }
 
-    public Folder WorkingDirectory { get; set; } = Environment.CurrentDirectory!;
+    public Folder WorkingDirectory { get; } = Environment.CurrentDirectory!;
 
     public IEnvironmentVariables EnvironmentVariables { get; }
 

src/ModularPipelines/Context/IEnvironmentContext.cs

@@ -30,10 +30,20 @@ public interface IEnvironmentContext
     public Folder AppDomainDirectory { get; }
 
     /// <inheritdoc cref="IHostEnvironment.ContentRootPath"/>
-    public Folder ContentDirectory { get; set; }
+    /// <remarks>
+    /// This property is immutable after pipeline initialization.
+    /// If you need to change the working directory for command execution,
+    /// use command options or <see cref="System.Environment.CurrentDirectory"/> directly.
+    /// </remarks>
+    public Folder ContentDirectory { get; }
 
     /// <inheritdoc cref="Environment.CurrentDirectory"/>
-    public Folder WorkingDirectory { get; set; }
+    /// <remarks>
+    /// This property captures the working directory at pipeline initialization time and is immutable.
+    /// If you need to change the working directory for command execution,
+    /// use command options or <see cref="System.Environment.CurrentDirectory"/> directly.
+    /// </remarks>
+    public Folder WorkingDirectory { get; }
 
     /// <inheritdoc cref="Environment.GetFolderPath(System.Environment.SpecialFolder)"/>
     public Folder? GetFolder(Environment.SpecialFolder specialFolder);

src/ModularPipelines/Context/PipelineContext.cs

@@ -10,11 +10,31 @@
 
 namespace ModularPipelines.Context;
 
+/// <summary>
+/// Provides context and services for module execution.
+/// </summary>
+/// <remarks>
+/// This class is registered as Scoped in the DI container, meaning each module execution
+/// gets its own instance. This ensures proper isolation between concurrent module executions.
+/// </remarks>
 internal class PipelineContext : IPipelineContext
 {
     private readonly IModuleLoggerProvider _moduleLoggerProvider;
+
+    /// <summary>
+    /// Cached logger instance for this context.
+    /// </summary>
+    /// <remarks>
+    /// This caching is safe because PipelineContext is Scoped (one instance per module execution).
+    /// Each module gets a fresh PipelineContext, so the cached logger is inherently module-scoped.
+    /// </remarks>
     private IModuleLogger? _logger;
 
+    /// <inheritdoc />
+    /// <remarks>
+    /// Logger is lazily initialized and cached for the lifetime of this context.
+    /// Since PipelineContext is Scoped, this provides a module-specific logger.
+    /// </remarks>
     public IModuleLogger Logger => _logger ??= _moduleLoggerProvider.GetLogger();
 
     public IServiceProvider ServiceProvider { get; }

src/ModularPipelines/Engine/Execution/ModuleRunner.cs

@@ -147,9 +147,31 @@ private async Task ExecuteModuleWithPipeline(ModuleState moduleState, IServicePr
         var logger = GetOrCreateLogger(moduleType, scopedServiceProvider);
         var moduleContext = new ModuleContext(pipelineContext, module, executionContext, logger);
 
-        // Set up logging
+        // Set up logging - use try/finally to ensure cleanup of AsyncLocal context
         ModuleLogger.Values.Value = logger;
 
+        try
+        {
+            await ExecuteModuleLifecycle(moduleState, scopedServiceProvider, pipelineContext, executionContext, moduleContext, cancellationToken).ConfigureAwait(false);
+        }
+        finally
+        {
+            // Clear AsyncLocal to prevent potential leaks in edge cases (thread pool reuse, long-running contexts)
+            ModuleLogger.Values.Value = null;
+        }
+    }
+
+    private async Task ExecuteModuleLifecycle(
+        ModuleState moduleState,
+        IServiceProvider scopedServiceProvider,
+        IPipelineContext pipelineContext,
+        ModuleExecutionContext executionContext,
+        IModuleContext moduleContext,
+        CancellationToken cancellationToken)
+    {
+        var module = moduleState.Module;
+        var moduleType = moduleState.ModuleType;
+
         // Before module hooks
         await _pipelineSetupExecutor.OnBeforeModuleStartAsync(moduleState).ConfigureAwait(false);
 

src/ModularPipelines/Logging/ModuleLoggerContainer.cs

@@ -1,3 +1,5 @@
+using System.Collections.Concurrent;
+
 namespace ModularPipelines.Logging;
 
 /// <summary>
@@ -8,29 +10,24 @@ namespace ModularPipelines.Logging;
 /// This container tracks all module loggers created during pipeline execution
 /// and ensures they are properly flushed and disposed in the correct order
 /// (ordered by last log written time) to maintain logical output ordering.
+/// Uses ConcurrentBag for lock-free thread-safe operations during high-concurrency
+/// module execution.
 /// </remarks>
 internal class ModuleLoggerContainer : IModuleLoggerContainer, IDisposable
 {
-    private readonly List<ModuleLogger> _loggers = new();
-    private bool _isDisposed;
+    private readonly ConcurrentBag<ModuleLogger> _loggers = new();
+    private int _isDisposed;
 
     public void FlushAndDisposeAll()
     {
-        lock (_loggers)
+        // Use Interlocked to ensure only one thread disposes
+        if (Interlocked.Exchange(ref _isDisposed, 1) == 1)
         {
-            if (_isDisposed)
-            {
-                return;
-            }
-
-            _isDisposed = true;
+            return;
         }
 
-        List<ModuleLogger> snapshot;
-        lock (_loggers)
-        {
-            snapshot = _loggers.ToList();
-        }
+        // ToArray() is thread-safe on ConcurrentBag
+        var snapshot = _loggers.ToArray();
 
         foreach (var logger in snapshot.Where(x => x != null).OrderBy(x => x.LastLogWritten))
         {
@@ -40,22 +37,18 @@ public void FlushAndDisposeAll()
 
     public IModuleLogger? GetLogger(Type type)
     {
-        lock (_loggers)
-        {
-            return _loggers.FirstOrDefault(logger => logger.GetType() == type);
-        }
+        // ToArray() creates a snapshot for safe enumeration
+        return _loggers.ToArray().FirstOrDefault(logger => logger.GetType() == type);
     }
 
     public void AddLogger(ModuleLogger logger)
     {
-        lock (_loggers)
-        {
-            _loggers.Add(logger);
-        }
+        // ConcurrentBag.Add is lock-free and thread-safe
+        _loggers.Add(logger);
     }
 
     public void Dispose()
     {
         FlushAndDisposeAll();
     }
-}
+}