Refactor UserOutput to remove Arc<Mutex<>> pattern

[josecelano]

Overview

Remove the Arc<Mutex<UserOutput>> pattern throughout the codebase and replace it with direct ownership/borrowing to eliminate deadlock risks and simplify the architecture.

Specification

See detailed specification: docs/issues/164-refactor-useroutput-remove-mutex.md

📋 Background

This refactoring addresses a critical reentrancy deadlock issue discovered in CreateTemplateCommandController and implements the architectural decision documented in ADR: Remove UserOutput Mutex.

Current Problems

• Reentrancy Deadlock: Tests hang indefinitely when UserOutput mutex is acquired through nested calls
• Architectural Mismatch: Complex concurrency mechanisms for a sequential, single-user application
• Timeout Workarounds: Defensive programming patterns that shouldn't be necessary
• Complex Lock Management: Error-prone manual lock scoping throughout codebase

🏗️ Architecture Requirements

DDD Layer: Presentation (primary), Application (secondary)
Module Path: src/presentation/user_output/, command controllers, progress reporters
Pattern: Direct ownership model with mutable borrowing

Module Structure Requirements

• Follow DDD layer separation (see docs/codebase-architecture.md)
• Respect dependency flow rules (dependencies flow toward domain)
• Use appropriate module organization (see docs/contributing/module-organization.md)

Architectural Constraints

• No business logic in presentation layer
• Error handling follows project conventions (see docs/contributing/error-handling.md)
• Testing strategy aligns with layer responsibilities

Anti-Patterns to Avoid

• ❌ Mixing concerns across layers
• ❌ Domain layer depending on infrastructure
• ❌ Monolithic modules with multiple responsibilities

Implementation Plan

Phase 1: Core UserOutput Refactor

• Remove Arc<Mutex<>> wrapper from UserOutput
• Update ExecutionContext to own UserOutput directly
• Modify command signatures to accept &mut UserOutput
• Remove timeout mechanisms and deadlock prevention code

Phase 2: ProgressReporter Simplification

• Refactor ProgressReporter to use direct &mut UserOutput access
• Remove mutex-related error handling in progress reporting
• Simplify progress update methods

Phase 3: Command Controller Updates

• Update all command controllers to use &mut UserOutput
• Remove scoped lock management code
• Simplify error handling paths

Phase 4: Test Updates

• Remove mutex mocking in tests
• Update test fixtures to use direct UserOutput
• Verify no deadlock scenarios can occur in new architecture

Acceptance Criteria

Note for Contributors: These criteria define what the PR reviewer will check. Use this as your pre-review checklist before submitting the PR to minimize back-and-forth iterations.

Quality Checks:

• Pre-commit checks pass: ./scripts/pre-commit.sh

Architecture Validation:

• Zero Arc<Mutex<UserOutput>> usage in codebase
• All command handlers use &mut UserOutput or owned UserOutput
• No mutex-related timeout or deadlock prevention code
• ProgressReporter uses direct access patterns

Functional Validation:

• All existing functionality preserved (user experience unchanged)
• All tests pass with no mutex mocking
• E2E tests demonstrate no deadlock scenarios
• Performance improved (no lock contention overhead)

Documentation:

• ADR implementation section updated with results
• Code comments explain new ownership patterns
• Any breaking changes documented

Related Issues

• Root Cause: ProgressReporter deadlock investigation (resolved with this refactor)
• ADR: Remove UserOutput Mutex

References

• Rust Book: Ownership and Borrowing
• Rust Anti-patterns: Shared Mutability
• Development Principles - Observability and simplicity

[josecelano]

Analysis Update: Deadlock Root Cause and Solution Options

After deeper analysis, I want to clarify the deadlock issue and explain why our initial approach needs revision.

1. Why We Have Deadlock

The deadlock occurs due to reentrancy - the same thread trying to acquire the same mutex twice:

// PROBLEMATIC: Reentrancy deadlock pattern
impl CreateTemplateCommandController {
    fn display_success_and_guidance(&self) -> Result<(), Error> {
        // Thread acquires UserOutput mutex here
        let mut output = self.user_output.lock().unwrap();
        output.display_success("Template created successfully!");

        // Still holding mutex, calls progress.complete()
        self.progress.complete()?; // ← Tries to acquire SAME mutex = DEADLOCK
        Ok(())
    }
}

This isn't a concurrency issue between multiple threads - it's a single thread trying to lock the same resource twice, which std::sync::Mutex doesn't allow.

2. Why We Made UserOutput Singleton (Creating This Problem)

We centralized UserOutput in the Container for a valid architectural reason: ensuring consistent theming/formatting across the entire application.

Before this change, different parts of the application could create their own UserOutput instances with different configurations, leading to inconsistent user experience. By making it a singleton in the Container, we guarantee:

• ✅ Same verbosity level throughout the app
• ✅ Consistent output formatting/colors
• ✅ Single "sink" for all output operations
• ✅ Centralized configuration management

However, this introduced the shared mutable state problem - multiple services need mutable access to the same UserOutput instance.

3. Our Initial Idea (Remove Mutex) and Why It Doesn't Work

Our first approach was to remove Arc<Mutex<UserOutput>> entirely and use direct ownership:

// ATTEMPTED SOLUTION: Direct ownership
pub struct ExecutionContext {
    user_output: UserOutput,  // Direct ownership, no mutex
}

Why this doesn't work:

1. Ownership conflicts: Multiple services need references to UserOutput (Container, ProgressReporter, Controllers)
2. Lifetime management: These services have different lifetimes and can't all own UserOutput
3. Mutable sharing: We need multiple parts of the code to have mutable access to the same sink
4. Lost configuration consistency: Would force us back to multiple UserOutput instances

The "sink pattern" insight is key - the output destination must be shared and mutable, so we can't eliminate synchronization entirely.

4. Two Promising Alternatives We're Considering

Option A: ReentrantMutex (Recommended)

Use parking_lot::ReentrantMutex instead of std::sync::Mutex:

use parking_lot::ReentrantMutex;

pub struct Container {
    user_output: Arc<ReentrantMutex<UserOutput>>,
}

// This works - no deadlock:
let guard1 = container.user_output.lock(); // Thread gets lock
guard1.display_success("Step 1");
let guard2 = container.user_output.lock(); // Same thread gets lock again - OK!
guard2.display_progress("Step 2");

Benefits:

• ✅ Prevents deadlock: Same thread can acquire lock multiple times
• ✅ Minimal code changes: Drop-in replacement for std::sync::Mutex
• ✅ Preserves architecture: Keeps singleton pattern and shared configuration
• ✅ Developer-friendly: No special patterns to remember - just works
• ✅ Better performance: parking_lot is generally faster than std mutex

Option B: Restructure ProgressReporter (More Complex)

Remove UserOutput storage from ProgressReporter and pass it as parameters:

// BEFORE: ProgressReporter stores reference (deadlock risk)
pub struct ProgressReporter {
    output: Arc<Mutex<UserOutput>>, // ← Stores reference
}

// AFTER: ProgressReporter takes UserOutput as parameter
pub struct ProgressReporter {
    total_steps: usize,
}

impl ProgressReporter {
    pub fn complete(&self, output: &mut UserOutput, summary: &str) {
        output.success(summary); // ← Direct access, no reentrancy
    }
}

Benefits:

• ✅ No external dependency: Uses standard library only
• ✅ Eliminates reentrancy: No stored references to prevent double-locking

Drawbacks:

• ❌ Major refactor: All ProgressReporter usage must change
• ❌ No enforcement: Developers could reintroduce the pattern elsewhere
• ❌ Complex threading: Harder to extend if we ever need concurrent output

Recommendation

I recommend Option A (ReentrantMutex) because:

1. Solves the root cause - eliminates reentrancy deadlocks completely
2. Minimal disruption - almost no code changes required
3. Future-proof - works even if we add concurrent operations later
4. Developer ergonomics - no special patterns to remember

The change would involve:

1. Add parking_lot dependency
2. Replace std::sync::Mutex<UserOutput> with parking_lot::ReentrantMutex<UserOutput>
3. Remove timeout/deadlock prevention code from ProgressReporter
4. Update imports

What are your thoughts on this approach?

cc @da2ce7