fix(logging): Address additional code review issues

Fix issues identified in second review:

1. AfterPipelineLogger: Use boolean flag for cache validity
   - Added _isCacheValid field instead of relying on StringBuilder.Length
   - Prevents unnecessary rebuilds when GetOutput() called multiple times
   - Clear StringBuilder on rebuild, set flag to true after build

2. ModuleLoggerProvider: Separate caching concerns
   - GetLogger(Type type) no longer modifies _moduleLogger cache
   - Removes lock from GetLogger(Type type) - doesn't need shared cache
   - Renamed MakeLogger to CreateLogger for clarity
   - Parameterless GetLogger() properly caches with lock protection
   - Avoids potential deadlock by not holding lock during DI resolution
     in GetLogger(Type type)

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

Co-Authored-By: Claude Opus 4.5 <[email protected]>

Author: thomhurst

src/ModularPipelines/Logging/AfterPipelineLogger.cs

@@ -13,6 +13,7 @@ internal class AfterPipelineLogger : IAfterPipelineLogger
     private readonly StringBuilder _stringBuilder = new();
     private readonly List<string> _values = [];
     private readonly object _lock = new();
+    private bool _isCacheValid;
 
     public AfterPipelineLogger(ILogger<AfterPipelineLogger> logger)
     {
@@ -27,8 +28,7 @@ public void LogOnPipelineEnd(string value)
         lock (_lock)
         {
             _values.Add(value);
-            // Clear cached output so GetOutput() rebuilds it with the new value
-            _stringBuilder.Clear();
+            _isCacheValid = false;
         }
     }
 
@@ -39,17 +39,18 @@ public string GetOutput()
     {
         lock (_lock)
         {
-            if (_stringBuilder.Length > 0)
+            if (_isCacheValid)
             {
                 return _stringBuilder.ToString();
             }
 
-            // Build once and cache
+            _stringBuilder.Clear();
             foreach (var value in _values)
             {
                 _stringBuilder.AppendLine(value);
             }
 
+            _isCacheValid = true;
             return _stringBuilder.ToString();
         }
     }

src/ModularPipelines/Logging/ModuleLoggerProvider.cs

@@ -32,12 +32,16 @@ public ModuleLoggerProvider(
         _stackTraceDetector = stackTraceDetector;
     }
 
+    /// <summary>
+    /// Gets a logger for a specific module type.
+    /// Does not cache to _moduleLogger to avoid conflicts with parameterless GetLogger().
+    /// </summary>
     public IModuleLogger GetLogger(Type type)
     {
-        lock (_lock)
-        {
-            return MakeLogger(type);
-        }
+        var loggerType = typeof(ModuleLogger<>).MakeGenericType(type);
+
+        return _moduleLoggerContainer.GetLogger(loggerType)
+            ?? (IModuleLogger)_serviceProvider.GetRequiredService(loggerType);
     }
 
     public IModuleLogger GetLogger()
@@ -59,7 +63,7 @@ public IModuleLogger GetLogger()
             var moduleType = ModuleLogger.CurrentModuleType.Value;
             if (moduleType != null)
             {
-                return MakeLogger(moduleType);
+                return _moduleLogger = CreateLogger(moduleType);
             }
 
             // Fallback: use stack trace inspection (for edge cases where AsyncLocal context is lost)
@@ -70,7 +74,7 @@ public IModuleLogger GetLogger()
                 throw new InvalidOperationException("Could not detect module type from call stack.");
             }
 
-            return MakeLogger(detectedType);
+            return _moduleLogger = CreateLogger(detectedType);
         }
     }
 
@@ -79,11 +83,11 @@ public void Dispose()
         _moduleLogger?.Dispose();
     }
 
-    private IModuleLogger MakeLogger(Type module)
+    private IModuleLogger CreateLogger(Type module)
     {
         var loggerType = typeof(ModuleLogger<>).MakeGenericType(module);
 
-        return _moduleLogger ??= _moduleLoggerContainer.GetLogger(loggerType)
-            ?? (IModuleLogger) _serviceProvider.GetRequiredService(loggerType);
+        return _moduleLoggerContainer.GetLogger(loggerType)
+            ?? (IModuleLogger)_serviceProvider.GetRequiredService(loggerType);
     }
 }