perf: Replace exclusive lock with ReaderWriterLockSlim in ModuleScheduler

[thomhurst]

Summary

• Refactor ModuleScheduler locking strategy to reduce contention in the hot execution path
• Replace object _stateLock with ReaderWriterLockSlim to allow concurrent reads
• Use read locks for query operations (GetStatistics, GetPendingModules, GetExecutingModules)
• Use write locks only for state mutations (MarkModuleStarted, MarkModuleCompleted, etc.)
• Move LINQ processing and logging operations outside of locks where possible
• Add proper disposal of the ReaderWriterLockSlim

This allows multiple threads to read scheduler state concurrently while maintaining exclusive access for writes, improving performance in scenarios with many parallel module queries.

Fixes #1458

Test plan

• Build succeeds
• Existing unit tests pass
• Run example pipelines to verify no deadlocks or race conditions
• Verify concurrent module execution still works correctly

🤖 Generated with Claude Code

[github-actions]

Summary

Replaces exclusive lock with ReaderWriterLockSlim in ModuleScheduler to reduce contention by allowing concurrent reads.

Critical Issues

1. Race condition in ShouldExitScheduler (lines 262-302)

The method takes a read lock to snapshot state, releases it, evaluates conditions outside the lock, then takes a write lock to update _schedulerCompleted. Between releasing the read lock and acquiring the write lock, another thread can modify _moduleStates, making the snapshot stale and potentially causing incorrect exit decisions.

Current flow:

_stateLock.EnterReadLock();
snapshot = ModuleStateSnapshot.Create(_moduleStates.Values);
_stateLock.ExitReadLock();
// <-- Race window: state can change here
var shouldExit = _exitConditions.ShouldExit(snapshot, queuedCount);
if (shouldExit) {
    _stateLock.EnterWriteLock();
    _schedulerCompleted = true;
    _stateLock.ExitWriteLock();
}

Fix: Keep read lock held while evaluating exit conditions, only upgrade to write lock when actually setting _schedulerCompleted:

_stateLock.EnterReadLock();
try {
    snapshot = ModuleStateSnapshot.Create(_moduleStates.Values);
    var shouldExit = _exitConditions.ShouldExit(snapshot, queuedCount);
    
    if (shouldExit) {
        _stateLock.ExitReadLock();
        _stateLock.EnterWriteLock();
        try {
            _schedulerCompleted = true;
        } finally {
            _stateLock.ExitWriteLock();
        }
        // Continue outside all locks
    } else {
        return false;
    }
} finally {
    if (_stateLock.IsReadLockHeld) _stateLock.ExitReadLock();
}

2. Copy-paste bug in CancelPendingModules (line 599)

The logging statement always logs ModuleExecutionState.Completed instead of the actual moduleState.State. This loses valuable debugging information about which state each module was in when cancelled.

Line 599:

_logger.LogDebug(
    "Cancelling pending module {ModuleName} (State={State})",
    MarkupFormatter.FormatModuleName(moduleState.ModuleType.Name),
    ModuleExecutionState.Completed);  // Bug: should be moduleState.State

Fix: Capture the state before modifying it:

var originalState = moduleState.State;
// ... modify state ...
// Then in logging section:
_logger.LogDebug(
    "Cancelling pending module {ModuleName} (State={State})",
    MarkupFormatter.FormatModuleName(moduleState.ModuleType.Name),
    originalState);

3. Potential race in AddModule (lines 179-197)

Dependencies are resolved outside the write lock (line 180), but then processed inside the lock. Between resolution and lock acquisition, the _moduleStates collection could change, potentially causing inconsistencies.

Fix: Move dependency resolution inside the write lock as it was in the original code, or document why external resolution is safe.

Suggestions

1. Consider upgradeability: The current LockRecursionPolicy.NoRecursion prevents lock upgrades. If you need read-to-write upgrades, use LockRecursionPolicy.SupportsRecursion, but be aware of potential deadlocks.

2. Performance verification: The issue mentions 10-30% improvement. Consider adding before/after benchmarks to verify the performance gain justifies the added complexity.

3. Documentation: Add XML comments noting that ReaderWriterLockSlim requires careful handling of lock/unlock pairs in all code paths (including exceptions).

Verdict

⚠️ REQUEST CHANGES - Critical race condition and copy-paste bug must be fixed

[github-actions]

Summary

This PR refactors ModuleScheduler to use ReaderWriterLockSlim instead of an exclusive lock, enabling concurrent reads while maintaining exclusive writes for better performance.

Critical Issues

None found ✅

All three critical issues from the previous review have been properly addressed in commit a004751:

1. Race condition in ShouldExitScheduler ✅ FIXED

   • Now keeps read lock held while evaluating exit conditions (lines 276-324)
   • Captures isDeadlocked under read lock before upgrading to write lock
   • Properly handles lock transitions with IsReadLockHeld check in finally block

2. Copy-paste bug in CancelPendingModules ✅ FIXED

   • Now captures originalState before modifying module state (line 602)
   • Logs the original state instead of hardcoded ModuleExecutionState.Completed (line 623)

3. Race condition in AddModule ✅ FIXED

   • Dependency resolution moved back inside write lock (line 186)
   • Unregistered dependencies tracked in a list and logged outside lock (lines 198-217)
   • Prevents race where _moduleStates changes between resolution and processing

Suggestions

1. Consider performance benchmarks: The PR description mentions this fixes issue MEDIUM: Scheduler lock contention in hot path could impact performance #1458 which likely documented performance problems. Consider adding benchmark results or performance metrics to validate the improvement.

2. Lock recursion policy trade-off: The current LockRecursionPolicy.NoRecursion is correct for this implementation but means lock upgrades aren't supported. The current code pattern (exit read lock, enter write lock) is appropriate. Document this if it becomes a maintenance concern.

3. Minor: .gitignore formatting (commit b00c9f1): The last line has nul.worktrees/ which should probably be two separate lines:

   nul
   .worktrees/
   

Previous Review Status

All critical issues from the previous review (race conditions and logging bug) have been properly addressed in commit a004751.

Verdict

✅ APPROVE - No critical issues, all previous concerns resolved