feat: defer module output until pipeline end when live progress enabled

[thomhurst]

Summary

• When live progress is enabled, module output is now deferred until the pipeline ends instead of flushing immediately after each module completes
• Output is grouped by module and displayed in completion order after progress stops
• Prevents console flickering caused by repeated progress pause/resume cycles during module output flushing

Changes

• Added SetProgressActive, OnModuleCompletedAsync, and FlushDeferredAsync methods to IOutputCoordinator
• OutputCoordinator stores deferred buffers with timestamps and flushes them in order at pipeline end
• ModuleLogger now calls OnModuleCompletedAsync which decides whether to defer or flush immediately
• PipelineOutputCoordinator calls FlushDeferredAsync after progress ends, before results are printed
• Added IAsyncDisposable safety net to ensure deferred output is never lost

Test Plan

• Unit tests for deferred flush behavior pass
• Full test suite passes (900+ tests)
• Build succeeds with no errors

[thomhurst]

Summary

Defers module output until pipeline end when live progress is enabled to prevent console flickering from pause/resume cycles.

Critical Issues

1. Race Condition in Progress Active State (ConsoleCoordinator.cs:232-233)

// Wire up the progress controller for output coordination
_outputCoordinator.SetProgressController(session);
_outputCoordinator.SetProgressActive(true);  // Added in PR

Issue: SetProgressActive(true) is called outside the _phaseLock (lock released at line 222), while _isProgressActive = true is set inside the lock (line 221). This creates a race condition where:

• ConsoleCoordinator thinks progress is active
• OutputCoordinator thinks progress is NOT active yet
• Module output could be flushed immediately instead of deferred

Fix: Move _outputCoordinator.SetProgressActive(true) inside the lock before line 222:

lock (_phaseLock)
{
    if (_isProgressActive) { ... }
    _isProgressActive = true;
    _outputCoordinator.SetProgressActive(true);  // Move inside lock
}

2. Missing SetProgressActive(false) in ConsoleCoordinator.cs:249

The EndProgressPhase() method sets progress to inactive, but the code shows:

_outputCoordinator.SetProgressController(NoOpProgressController.Instance);
_outputCoordinator.SetProgressActive(false);

However, this is inside the _phaseLock, which is correct. But there's an asymmetry issue: if BeginProgressAsync returns a NoOpProgressSession when progress is disabled (line 210), the OutputCoordinator's progress state is never set. This means if progress was previously active, then disabled, then re-enabled, the state could be stale.

Recommendation: Also call _outputCoordinator.SetProgressActive(false) when creating NoOpProgressSession (around line 210).

3. Memory Leak - Unbounded Deferred Output Growth (OutputCoordinator.cs:76-80)

private readonly List<DeferredModuleOutput> _deferredOutputs = new();

Issue: If a pipeline runs for a very long time with many modules and progress enabled, _deferredOutputs will grow without bound until FlushDeferredAsync() is called at the very end. For pipelines with thousands of modules, this could consume significant memory.

Severity: Medium - unlikely in most scenarios but possible in very large pipelines.

Recommendation: Consider a maximum size or periodic flushing strategy for extremely long-running pipelines.

4. PipelineOutputCoordinator Missing Parameter in Constructor (PipelineOutputCoordinator.cs:30-42)

The PR diff shows IOutputCoordinator is added as a parameter, but the original file (read at line 30-42) doesn't have it. Looking at the diff again:

+    private readonly IOutputCoordinator _outputCoordinator;
+        IOutputCoordinator outputCoordinator)
+        _outputCoordinator = outputCoordinator;

This means the PR correctly adds the dependency. However, I notice the original file I read doesn't have this - this is expected since I read the current main branch, not the PR branch. This is correct in the PR.

5. Timeout Message Change is Misleading (ModuleLogger.cs:173-183)

The diff changes the timeout warning message from:

-"Module output flush timed out after 30 seconds for {ModuleType}. Some output may be lost."
+"Module output handling timed out after 30 seconds for {ModuleType}."

Issue: The new message removes "Some output may be lost", which is critical information for debugging. When output is deferred and the timeout occurs, output will be lost since it won't be flushed.

Recommendation: Keep the "Some output may be lost" warning or change to "Output may be deferred until pipeline end."

Suggestions

1. Test Coverage for Race Conditions

The new tests (OutputCoordinatorDeferredFlushTests.cs) only test happy paths. Consider adding tests for:

• Concurrent module completion during progress state changes
• FlushDeferredAsync called while modules are still completing
• Disposal race conditions

2. Documentation Clarity

The comment in OutputCoordinator.cs:76 says "defer output until pipeline end", but this assumes progress is enabled. Consider clarifying: "defer output until progress ends (if progress is active)"

Previous Review Status

No previous review comments found (permission issues prevented reading comments API).

Verdict

⚠️ REQUEST CHANGES - Critical race condition (#1) must be fixed before merge. Issues #2 and #5 should also be addressed.

[thomhurst]

Summary

This PR defers module output until the pipeline ends when live progress is enabled, preventing console flickering from repeated progress pause/resume cycles.

Critical Issues

1. Missing disposal registration for IAsyncDisposable OutputCoordinator

The PR adds IAsyncDisposable to OutputCoordinator as a safety net to flush deferred output on disposal (line 258 in OutputCoordinator.cs). However, OutputCoordinator is registered as a singleton service and is never explicitly disposed.

In ModularPipelines, services are managed by the Microsoft.Extensions.DependencyInjection container. While the container does dispose IDisposable services when the ServiceProvider is disposed, it does NOT automatically dispose IAsyncDisposable services.

Impact: The safety net DisposeAsync() method will never be called, meaning if FlushDeferredAsync() is not called elsewhere (e.g., due to a bug or exception), deferred output would be permanently lost.

Recommendation: Either:

1. Remove the IAsyncDisposable implementation since it won't be called automatically, OR
2. Ensure the DI container properly disposes async disposables (requires special handling in the host shutdown), OR
3. Document that this is purely a defensive measure and the primary flush happens in PipelineOutputCoordinator.DisposeAsync()

Reference: src/ModularPipelines/DependencyInjection/DependencyInjectionSetup.cs:240-241 shows singleton registration

---

2. Potential memory leak if pipeline is cancelled before completion

In OutputCoordinator.OnModuleCompletedAsync() (lines 62-84), when progress is active, module buffers are added to _deferredOutputs but only cleared in FlushDeferredAsync().

If the pipeline is cancelled/aborted before reaching PipelineOutputCoordinator.DisposeAsync(), the deferred outputs remain in memory for the lifetime of the singleton OutputCoordinator.

Impact: In long-running applications that execute multiple pipelines (e.g., a web service), this could accumulate significant memory.

Recommendation: Clear _deferredOutputs in more scenarios:

• On cancellation
• When SetProgressActive(false) is called
• Add a check in the safety net DisposeAsync() to log if deferred outputs exist

---

3. Lock contention in OnModuleCompletedAsync

Line 74 in OutputCoordinator.OnModuleCompletedAsync():

lock (_queueLock)
{
    _deferredOutputs.Add(new DeferredModuleOutput(...));
}

The _queueLock is also used in:

• EnqueueAndFlushAsync (line 59) - frequently called when progress is disabled
• FlushDeferredAsync (line 96) - called at pipeline end
• ProcessQueueAsync (lines 88, 90) - called during immediate flush mode

Impact: When modules complete rapidly in parallel, this could cause lock contention between deferred storage and immediate flush operations.

Recommendation: Consider using a separate lock for _deferredOutputs or use a ConcurrentBag<T> instead of List<T> to avoid locking on adds.

Suggestions

1. Missing unit test coverage for error scenarios

The new test file OutputCoordinatorDeferredFlushTests.cs only tests happy path scenarios. Consider adding tests for:

• Pipeline cancellation before flush
• Multiple concurrent module completions (stress test)
• FlushDeferredAsync timeout/failure scenarios
• The IAsyncDisposable safety net

2. Race condition comment could be clearer

In ConsoleCoordinator.BeginProgressAsync() line 228 (in the diff):

// CRITICAL: Set OutputCoordinator's progress state inside the lock
// to prevent race conditions where a module completes between
// _isProgressActive = true and OutputCoordinator being notified

This is excellent defensive programming, but the comment could specify what would happen without the lock (modules would defer output when they shouldn't, or vice versa).

3. Consider documenting the ordering guarantee

The PR description mentions "Output is grouped by module and displayed in completion order" - this is implemented via OrderBy(x => x.CompletedAt) at line 103 in OutputCoordinator.cs. Consider adding a comment explaining why completion order matters (vs. module dependency order or start order).

Previous Review Status

No previous review comments found.

Verdict

⚠️ REQUEST CHANGES - Critical issue #1 (disposal) and #2 (memory leak) should be addressed before merging.

[thomhurst]

Summary

Defers module output until pipeline end when live progress is enabled to prevent console flickering from repeated pause/resume cycles.

Critical Issues

1. IAsyncDisposable Won't Be Called - Safety Net is Ineffective

OutputCoordinator implements IAsyncDisposable (OutputCoordinator.cs:11) as a safety net to flush deferred output on disposal. However, OutputCoordinator is registered as a singleton service, and the .NET DI container does not automatically dispose IAsyncDisposable services.

Impact: The safety net DisposeAsync() method at line 290 will never be called, making it dead code. This creates a false sense of safety - if FlushDeferredAsync() is not called in PipelineOutputCoordinator.DisposeAsync(), deferred output will be permanently lost.

Recommendation: Remove the IAsyncDisposable implementation entirely. The extensive documentation (lines 272-289) acknowledges this is a "defensive measure" but also states the primary flush happens in PipelineOutputCoordinator. Since the disposal path never executes, keeping it is misleading.

---

2. Memory Leak on Pipeline Cancellation

When _isProgressActive is true, OnModuleCompletedAsync() (line 84) adds buffers to _deferredOutputs. These are only cleared in two places:

1. FlushDeferredAsync() - called at pipeline end
2. SetProgressActive(true) - clears stale outputs from previous runs (line 70)

Issue: If a pipeline is cancelled mid-execution (before reaching PipelineOutputCoordinator.DisposeAsync()), the deferred outputs remain in memory for the lifetime of the singleton OutputCoordinator.

Impact: In long-running applications that execute multiple pipelines (e.g., a web service running CI/CD pipelines), this accumulates unbounded memory.

Example scenario:

1. Pipeline starts with progress enabled
2. 100 modules complete → 100 buffers added to _deferredOutputs
3. User cancels pipeline (Ctrl+C)
4. PipelineOutputCoordinator.DisposeAsync() might not be called
5. Next pipeline starts → previous 100 buffers still in memory
6. Repeat over days → memory leak

Recommendation: Clear _deferredOutputs in SetProgressActive(false) at line 258 (in ConsoleCoordinator.EndProgressPhase). This ensures cleanup happens whenever progress ends, regardless of cancellation:

public void SetProgressActive(bool isActive)
{
    if (!isActive)
    {
        // Progress ending - clean up any remaining deferred outputs
        lock (_deferredLock)
        {
            if (_deferredOutputs.Count > 0)
            {
                _logger.LogWarning(
                    "Progress ended with {Count} unflushed deferred outputs. " +
                    "This indicates FlushDeferredAsync was not called. Clearing to prevent memory leak.",
                    _deferredOutputs.Count);
                _deferredOutputs.Clear();
            }
        }
    }
    else
    {
        // ... existing stale output check ...
    }
    
    _isProgressActive = isActive;
}

Suggestions

Test Coverage for Error Scenarios

The new test file OutputCoordinatorDeferredFlushTests.cs only tests happy paths with progress disabled. Consider adding tests for:

• Pipeline cancellation before flush
• Multiple concurrent module completions (race condition stress test)
• Verifying deferred outputs are cleared when progress ends without flush

Previous Review Status

First Review Issues:

• ✅ Race condition in SetProgressActive: FIXED - now inside lock
• ✅ Missing SetProgressActive(false) for NoOpProgressSession: FIXED
• ⚠️ Unbounded memory growth: PARTIALLY ADDRESSED - see Critical Issue Feedback/Ideas for Improvement #2
• ✅ Timeout message: FIXED - retains "Some output may be lost"

Second Review Issues:

• ❌ IAsyncDisposable won't be called: NOT FIXED - see Critical Issue Feature/initial work #1
• ⚠️ Memory leak on cancellation: PARTIALLY ADDRESSED - see Critical Issue Feedback/Ideas for Improvement #2
• ✅ Lock contention: FIXED - separate _deferredLock added

Verdict

⚠️ REQUEST CHANGES - Two critical issues must be addressed:

1. Remove ineffective IAsyncDisposable implementation (misleading dead code)
2. Clear _deferredOutputs when progress ends to prevent memory leaks on cancellation

[thomhurst]

Summary

Defers module output until pipeline completion when live progress is enabled to prevent console flickering from repeated progress pause/resume cycles.

Critical Issues

None found ✅

Suggestions

1. Consider logging level adjustment for cancellation scenarios (OutputCoordinator.cs:71-77)

The warning at line 72-76 treats cancellation-related unflushed outputs the same as abnormal termination. Since cancellation is a normal operational scenario, consider using LogDebug or LogInformation instead of LogWarning for this specific case, or differentiate between abnormal crashes vs expected cancellation.

2. Test coverage could be expanded (OutputCoordinatorDeferredFlushTests.cs)

The new tests only verify basic pipeline completion with progress disabled. Consider adding tests for:

• Deferred flush behavior when progress IS enabled (the main feature being added)
• Multiple modules completing in different orders
• Cancellation scenarios
• Exception handling during flush

These are optional improvements - the current implementation appears solid.

Verdict

✅ APPROVE - No critical issues

The race condition fix (moving SetProgressActive inside the lock) is well-reasoned and the architecture for deferred output looks sound. The use of separate locks (_deferredLock vs _queueLock) to reduce contention is good design.