fix: Improve pipeline context isolation and thread safety

[thomhurst]

Summary

This PR addresses issue #1417 by implementing proper module-scoped isolation for pipeline context objects.

Changes

• AsyncLocal cleanup: Added try/finally block in ModuleRunner.cs to explicitly clear ModuleLogger.Values.Value after module execution, preventing potential context leaks in edge cases (thread pool reuse, long-running async contexts)

• Immutable EnvironmentContext properties: Changed ContentDirectory and WorkingDirectory from { get; set; } to { get; } to prevent modules from accidentally affecting each other's working directory state

• Thread-safe ModuleLoggerContainer: Replaced List<ModuleLogger> with ConcurrentBag<ModuleLogger> and lock-based synchronization with Interlocked for lock-free thread-safe operations during high-concurrency module execution

• Documentation: Added XML documentation to PipelineContext explaining the scoped DI lifetime and why logger caching is safe

Breaking Changes

⚠️ IEnvironmentContext.ContentDirectory and IEnvironmentContext.WorkingDirectory are now readonly. Code that was setting these properties will need to be updated to use command options or System.Environment.CurrentDirectory directly.

Test plan

• Build succeeds with no new errors
• Unit tests pass (389 passed, 0 failed, 4 skipped)
• Manual verification of concurrent module execution isolation

Closes #1417

🤖 Generated with Claude Code

[github-actions]

Summary

Improves pipeline context isolation and thread safety by adding AsyncLocal cleanup, making environment properties immutable, and using thread-safe collections for module logger container.

Critical Issues

1. AsyncLocal Set Outside Try Block (src/ModularPipelines/Engine/Execution/ModuleRunner.cs:151)

Problem: The ModuleLogger.Values.Value = logger; assignment happens BEFORE the try block (line 151), but the cleanup ModuleLogger.Values.Value = null; is in the finally block. If an exception occurs between line 151 and the try block, the AsyncLocal value won't be cleared, causing the exact leak this PR aims to prevent.

Fix: Move the AsyncLocal assignment INSIDE the try block:

try
{
    // Set up logging - MOVED INSIDE try
    ModuleLogger.Values.Value = logger;
    
    await ExecuteModuleLifecycle(moduleState, scopedServiceProvider, pipelineContext, executionContext, moduleContext, cancellationToken).ConfigureAwait(false);
}
finally
{
    // Clear AsyncLocal to prevent potential leaks
    ModuleLogger.Values.Value = null;
}

This ensures the cleanup always matches the setup.

---

Suggestions

1. Optimize ConcurrentBag Enumeration (src/ModularPipelines/Logging/ModuleLoggerContainer.cs)

The GetLogger method calls ToArray() on every lookup, creating unnecessary allocations during concurrent module execution:

public IModuleLogger? GetLogger(Type type)
{
    // ToArray() creates a new array on every call
    return _loggers.ToArray().FirstOrDefault(logger => logger.GetType() == type);
}

Suggestion: Since loggers are rarely removed and lookups happen frequently, consider caching or using a ConcurrentDictionary<Type, ModuleLogger> for O(1) lookups instead of O(n) enumeration. However, if the current approach is intentional (loggers looked up once at module start), document why to prevent future "optimization" attempts.

2. Memory Barrier for Interlocked Flag (src/ModularPipelines/Logging/ModuleLoggerContainer.cs:13)

The _isDisposed field is accessed via Interlocked.Exchange but declared as a plain int. While Interlocked provides atomicity, consider making it explicit:

private volatile int _isDisposed;

Or document that Interlocked provides sufficient guarantees (which it does). This is a minor clarity improvement.

---

Previous Review Status

No previous comments found.

---

Verdict

⚠️ REQUEST CHANGES - Critical issue with AsyncLocal cleanup scope must be fixed

[github-actions]

Summary

Improves pipeline context isolation and thread safety by fixing AsyncLocal cleanup scope and optimizing logger container lookups.

Critical Issues

None found ✅

Suggestions

None - the code changes properly address all issues identified in the previous review.

Previous Review Status

All critical issues from the bot's previous review have been addressed:

• ✅ AsyncLocal cleanup fixed: The assignment is now inside the try block, ensuring proper cleanup even if an exception occurs immediately after assignment (src/ModularPipelines/Engine/Execution/ModuleRunner.cs:153-154)
• ✅ ConcurrentDictionary optimization implemented: Switched from ConcurrentBag to ConcurrentDictionary for O(1) lookups instead of O(n) (src/ModularPipelines/Logging/ModuleLoggerContainer.cs:18)
• ✅ Interlocked documentation: Added clear comment explaining that Interlocked.Exchange provides both atomicity and full memory barrier (src/ModularPipelines/Logging/ModuleLoggerContainer.cs:19-20)

Verdict

✅ APPROVE - No critical issues, all previous feedback properly addressed