Refactor: Redesign ProgressPrinter with clean architecture

[thomhurst]

Problem

The ProgressPrinter (373 lines) has accumulated significant technical debt with thread safety issues, redundant state tracking, fire-and-forget tasks, and untestable design.

Location: src/ModularPipelines/Helpers/ProgressPrinter.cs

---

Current Issues

1. Redundant State Tracking (3 Dictionaries)

private readonly ConcurrentDictionary<IModule, ProgressTask> _progressTasks = new();
private readonly ConcurrentDictionary<ModuleState, ProgressTask> _moduleStateProgressTasks = new();
private readonly ConcurrentDictionary<SubModuleBase, ProgressTask> _subModuleProgressTasks = new();

Same ProgressTask objects stored in 3 dictionaries with different keys - hard to keep in sync.

2. Fire-and-Forget Background Tasks (Lines 95-113)

_ = Task.Run(async () =>
{
    while (progressTask is { IsFinished: false, Value: < 95 })
    {
        await Task.Delay(TimeSpan.FromSeconds(1), CancellationToken.None);
        progressTask.Increment(ticksPerSecond);
    }
    catch { }  // Silent exception swallowing
}, CancellationToken.None);  // Cannot be cancelled

• No tracking or cleanup of background tasks
• Uses CancellationToken.None - cannot be cancelled
• Silently swallows all exceptions

3. Thread Safety Inconsistency

• Uses ConcurrentDictionary but also manual locks
• Non-atomic counter increments (_completedModuleCount++) causes race conditions

4. Polling-Based Refresh

Refreshes every 1 second regardless of changes - wasteful CPU wake-ups.

5. Incomplete PrintResults (Lines 288-295)

Shows placeholder "-" instead of actual module Duration, Status, Start, End times.

6. Not Testable

• [ExcludeFromCodeCoverage] attribute
• Direct Spectre.Console dependency
• No abstraction over rendering

---

Proposed Clean Design

Architecture Overview

+-------------------------------------------------------------------+
|                      IProgressRenderer                             |
|  (Abstraction over Spectre.Console - testable)                    |
+-------------------------------------------------------------------+
|                      ProgressSnapshot                              |
|  (Single source of truth - immutable state)                       |
+-------------------------------------------------------------------+
|                   ProgressCoordinator                              |
|  (Handles events, updates state, triggers renders)                |
+-------------------------------------------------------------------+
|               IProgressEstimator                                   |
|  (Isolated estimation logic - injectable/testable)                |
+-------------------------------------------------------------------+

1. Immutable Progress State

public sealed record ProgressSnapshot
{
    public required ImmutableDictionary<Type, ModuleProgress> Modules { get; init; }
    public required int TotalModules { get; init; }
    public required int CompletedModules { get; init; }
    public required double OverallProgress { get; init; }
    public required bool IsComplete { get; init; }
}

public sealed record ModuleProgress
{
    public required Type ModuleType { get; init; }
    public required string DisplayName { get; init; }
    public required ModuleProgressStatus Status { get; init; }
    public required double ProgressPercent { get; init; }
    public required TimeSpan ElapsedTime { get; init; }
    public required ImmutableList<SubModuleProgress> SubModules { get; init; }
    public string? ErrorMessage { get; init; }
}

public enum ModuleProgressStatus { Pending, Running, Completed, Skipped, Failed }

2. Progress Renderer Abstraction

public interface IProgressRenderer : IAsyncDisposable
{
    Task InitializeAsync(int totalModules, CancellationToken cancellationToken);
    void Render(ProgressSnapshot snapshot);
    Task RenderResultsAsync(PipelineSummary summary, CancellationToken cancellationToken);
}

// Real implementation
internal sealed class SpectreProgressRenderer : IProgressRenderer { }

// Test double
internal sealed class TestProgressRenderer : IProgressRenderer
{
    public List<ProgressSnapshot> RecordedSnapshots { get; } = new();
}

3. Progress Estimator (Isolated Logic)

public interface IProgressEstimator
{
    double CalculateProgress(TimeSpan elapsed, TimeSpan? estimated);
}

internal sealed class DefaultProgressEstimator : IProgressEstimator
{
    private const double MaxAutoProgress = 95.0;

    public double CalculateProgress(TimeSpan elapsed, TimeSpan? estimated)
    {
        if (estimated is null || estimated.Value <= TimeSpan.Zero)
        {
            // Logarithmic curve approaching but never reaching 95%
            return MaxAutoProgress * (1 - Math.Exp(-elapsed.TotalSeconds / 30));
        }

        var progress = (elapsed.TotalSeconds / estimated.Value.TotalSeconds) * 100;
        return Math.Min(progress, MaxAutoProgress);
    }
}

4. Progress Coordinator

internal sealed class ProgressCoordinator :
    INotificationHandler<ModuleStartedNotification>,
    INotificationHandler<ModuleCompletedNotification>,
    IAsyncDisposable
{
    private readonly IProgressRenderer _renderer;
    private readonly IProgressEstimator _estimator;
    private readonly TimeProvider _timeProvider;

    // Single source of truth with atomic updates
    private ImmutableDictionary<Type, ModuleProgressState> _states =
        ImmutableDictionary<Type, ModuleProgressState>.Empty;
    private readonly object _stateLock = new();

    // Managed, cancellable update task
    private readonly CancellationTokenSource _cts = new();
    private Task? _updateTask;

    private async Task RunUpdateLoopAsync(CancellationToken cancellationToken)
    {
        using var timer = new PeriodicTimer(TimeSpan.FromMilliseconds(100));

        while (await timer.WaitForNextTickAsync(cancellationToken))
        {
            UpdateProgressEstimates();
            _renderer.Render(CreateSnapshot());
        }
    }

    public async ValueTask DisposeAsync()
    {
        _cts.Cancel();
        if (_updateTask is not null)
            await _updateTask;
        _cts.Dispose();
    }
}

5. Complete Results Display

public async Task RenderResultsAsync(PipelineSummary summary, CancellationToken ct)
{
    var table = new Table()
        .AddColumn("Module")
        .AddColumn("Duration")
        .AddColumn("Status")
        .AddColumn("Start")
        .AddColumn("End");

    foreach (var module in summary.Modules.OrderBy(m => m.ModuleStart))
    {
        var status = module.Status switch
        {
            Status.Successful => "[green]Success[/]",
            Status.Skipped => "[yellow]Skipped[/]",
            Status.Failed => "[red]Failed[/]",
            _ => "[grey]Unknown[/]"
        };

        table.AddRow(
            module.ModuleName,
            TimeSpanFormatter.Format(module.ModuleDuration),
            status,
            module.ModuleStart.ToString("HH:mm:ss"),
            module.ModuleEnd.ToString("HH:mm:ss"));
    }

    _console.Write(table);

    // Summary stats
    _console.MarkupLine($"Total: {summary.TotalDuration:hh\\:mm\\:ss}");
    _console.MarkupLine($"Passed: {summary.Modules.Count(m => m.Status == Status.Successful)}");
    _console.MarkupLine($"Skipped: {summary.Modules.Count(m => m.Status == Status.Skipped)}");
    _console.MarkupLine($"Failed: {summary.Modules.Count(m => m.Status == Status.Failed)}");
}

---

Comparison

Current	Proposed
3 dictionaries tracking same data	Single immutable state
Fire-and-forget untracked tasks	Managed cancellable task
Mixed sync primitives	Consistent locking
1-second polling	100ms event-driven timer
Empty catch blocks	Logged exceptions
Not testable	Fully testable via abstractions
Incomplete results table	Complete module details

---

Migration Path

1. Create new abstractions alongside existing code
2. Implement ProgressCoordinator with new design
3. Create SpectreProgressRenderer implementation
4. Add comprehensive tests using TestProgressRenderer
5. Switch DI registration to new implementation
6. Remove old ProgressPrinter

Files to Create/Update

• src/ModularPipelines/Progress/IProgressRenderer.cs (new)
• src/ModularPipelines/Progress/IProgressEstimator.cs (new)
• src/ModularPipelines/Progress/ProgressSnapshot.cs (new)
• src/ModularPipelines/Progress/ProgressCoordinator.cs (new)
• src/ModularPipelines/Progress/SpectreProgressRenderer.cs (new)
• test/ModularPipelines.UnitTests/Progress/ (new test folder)
• Remove: src/ModularPipelines/Helpers/ProgressPrinter.cs

Impact

• Significantly improves reliability and maintainability
• Enables testing of progress display logic
• Better UX with complete results table
• Eliminates race conditions and resource leaks