perf: Add cancellation support and short-circuit to ModuleConditionHandler

[thomhurst]

Summary

Fixes #1571

• Added CancellationToken parameter to IModuleConditionHandler.ShouldIgnore for proper cancellation support
• Changed from parallel condition evaluation to sequential with short-circuit optimization:
  • Mandatory conditions short-circuit on first failure
  • Non-mandatory conditions short-circuit on first success
• Removed dependency on EnumerableAsyncProcessor for simpler, more efficient code
• Added ConfigureAwait(false) to async calls

Rationale

The previous implementation evaluated all conditions in parallel, which was wasteful when:

1. A mandatory condition fails early - no need to check remaining conditions
2. A non-mandatory condition succeeds - no need to check remaining conditions

Sequential evaluation with short-circuiting is both simpler and more efficient for this use case.

Test plan

• Build succeeds
• Existing tests pass
• Verify cancellation behavior in module condition evaluation

🤖 Generated with Claude Code

[thomhurst]

Summary

This PR adds cancellation token support and switches from parallel to sequential short-circuit evaluation in ModuleConditionHandler for better performance and proper cancellation handling.

Critical Issues

None found ✅

Suggestions

1. Consider passing CancellationToken at call site (ModuleRetriever.cs:55)

While the new parameter has a default value, for proper cancellation propagation you might want to update the call site in ModuleRetriever.GetInternal() to pass a cancellation token:

var (shouldIgnore, skipDecision) = await _moduleConditionHandler.ShouldIgnore(module, cancellationToken).ConfigureAwait(false);

This would require adding a CancellationToken parameter to GetOrganizedModules() and GetInternal(). This could be a follow-up improvement if you want full cancellation support throughout the module organization phase.

2. Condition evaluation remains non-cancellable

The RunConditionAttribute.Condition() method does not accept a CancellationToken, so individual condition evaluations cannot be cancelled mid-execution. The PR adds cancellationToken.ThrowIfCancellationRequested() between evaluations which is good, but if a single condition takes a long time, it will not respect cancellation until it completes. This is a broader architectural consideration beyond this PR scope.

Previous Review Status

No previous comments found.

Verdict

✅ APPROVE - No critical issues. The changes are well-reasoned: sequential evaluation with short-circuiting is both simpler and more efficient than parallel evaluation for this use case. The cancellation token support is a good addition, even if propagation could be improved in future work.

[Copilot]

Pull request overview

This PR enhances the ModuleConditionHandler with cancellation support and optimizes condition evaluation by replacing parallel processing with sequential short-circuit logic. The change simplifies the code by removing the dependency on EnumerableAsyncProcessor and improves efficiency by stopping evaluation as soon as the outcome is determined.

Key changes:

• Added CancellationToken parameter to IModuleConditionHandler.ShouldIgnore method
• Replaced parallel condition evaluation with sequential evaluation that short-circuits on first failure (mandatory) or first success (non-mandatory)
• Added ConfigureAwait(false) to async calls and removed the RunnableConditionMet record class

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/ModularPipelines/Engine/IModuleConditionHandler.cs	Added CancellationToken parameter to ShouldIgnore method signature with default value
src/ModularPipelines/Engine/ModuleConditionHandler.cs	Implemented cancellation support, replaced parallel evaluation with sequential short-circuit logic, removed dependency on EnumerableAsyncProcessor, and cleaned up unused code

[Copilot]

The cancellation token is being checked between evaluating conditions, but it's not being passed to the attr.Condition(pipelineContext) call on line 86. This means the actual condition evaluation cannot be cancelled, which limits the effectiveness of the cancellation support added in this PR. Consider updating the RunConditionAttribute.Condition method signature to accept a CancellationToken parameter.

[Copilot]

The cancellation token is being checked between evaluating conditions, but it's not being passed to the attr.Condition(pipelineContext) call on line 103. This means the actual condition evaluation cannot be cancelled, which limits the effectiveness of the cancellation support added in this PR. Consider updating the RunConditionAttribute.Condition method signature to accept a CancellationToken parameter.