Prioritize pipeline cancellation classification

[thomhurst]

Summary

• classify cancellation-shaped exceptions against pipeline cancellation before timeout inference
• preserve independent failures while removing redundant timeout exception tests
• cover late module timeouts and elapsed cancellation races deterministically

Validation

• ModuleExecutionPipelineTests: 6/6
• ModuleTimeoutTests: 12/12
• EngineCancellationTokenTests: 15/15
• core Release build: 0 warnings, 0 errors

Closes #3791

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 590c20cd66

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Preserve AlwaysRun timeouts after cancellation

When an AlwaysRun cleanup module times out (or otherwise throws OperationCanceledException) after an earlier module has cancelled the engine, this branch returns PipelineTerminated solely because the global engine token is cancelled. SetupCancellation deliberately does not link AlwaysRun modules to engine cancellation, so their own timeout should still be reported as TimedOut; otherwise reports/telemetry hide the cleanup module's actual failure. Consider excluding config.AlwaysRun from this early pipeline-termination classification or classifying timeouts first for AlwaysRun modules.

Useful? React with 👍 / 👎.

[thomhurst]

Fixed in 952eaea. Pipeline cancellation classification now excludes AlwaysRun modules, matching their independent cancellation token setup. Added regressions for both ModuleTimeoutException and elapsed OperationCanceledException after prior engine cancellation; they retain TimedOut. Focused results: ModuleExecutionPipelineTests 8/8, ModuleTimeoutTests 12/12, EngineCancellationTokenTests 15/15; core Release build 0 warnings/errors.

[claude]

Review: Prioritize pipeline cancellation classification (#3876)

Intent matches the issue. #3791 asked for the classification chain to be reworked into pipeline-cancelled → own-timeout → failure, with the dead ModuleTimeoutException or ... arm in the old IsPipelineCancelled removed. ClassifyException (ModuleExecutionPipeline.cs:743-757) implements exactly that ordering, and the two new tests (ExecuteAsync_ClassifiesLateTimeoutAsPipelineTerminated, ExecuteAsync_ClassifiesElapsedCancellationAsPipelineTerminated) correctly demonstrate the previously-buggy race: a module whose own elapsed time exceeds its timeout, but which is only cancelled because the pipeline already terminated, now reports PipelineTerminated instead of TimedOut. The removal of the redundant ModuleTimeoutException/TaskCanceledException checks from IsTimeout is safe since TaskCanceledException : OperationCanceledException and the ModuleTimeoutException case is now handled by the explicit OR in ClassifyException.

Unresolved: AlwaysRun modules will be misclassified (raised by Codex, not yet addressed)

ClassifyException reads the global _engineCancellationToken.IsCancelled flag to decide pipeline-termination, but SetupCancellation (ModuleExecutionPipeline.cs:337-360) deliberately does not link an AlwaysRun module's token to _engineCancellationToken — that's the whole point of AlwaysRun (cleanup/reporting modules keep executing after another module has already failed and cancelled the engine).

Concretely: module A fails → _engineCancellationToken.CancelWithException(...) → IsCancelled becomes true pipeline-wide. Module B is AlwaysRun with its own 30s timeout, unaffected by A's cancellation. If B's own timeout genuinely elapses and its execution throws ModuleTimeoutException/OperationCanceledException after A already cancelled the engine (a very plausible ordering, since AlwaysRun modules typically run during/after teardown), ClassifyException will report Status.PipelineTerminated purely because some other, unrelated module cancelled the pipeline — not because B was cancelled. B's genuine timeout is now silently swallowed as "pipeline terminated" instead of surfacing as TimedOut, which is exactly the telemetry-hiding problem this PR is trying to fix, just for a different module category.

This was already flagged in an inline review comment on this same commit (590c20c, ModuleExecutionPipeline.cs:751) and hasn't been addressed in the current diff — ClassifyException still doesn't consult config.AlwaysRun anywhere.

Suggested fix: exclude AlwaysRun modules from the pipeline-cancelled branch, since their cancellation token is intentionally decoupled from _engineCancellationToken:

private Status ClassifyException(
    ModuleConfiguration config,
    ModuleExecutionContext executionContext,
    Exception exception)
{
    if (!config.AlwaysRun
        && _engineCancellationToken.IsCancelled
        && exception is OperationCanceledException or ModuleTimeoutException)
    {
        return Status.PipelineTerminated;
    }

    return exception is ModuleTimeoutException || IsTimeout(config, executionContext, exception)
        ? Status.TimedOut
        : Status.Failed;
}

This keeps the new pipeline-cancelled-first precedence for regular modules (the actual bug in #3791) while preserving genuine timeout/failure reporting for AlwaysRun cleanup modules, consistent with how SetupCancellation already treats them as independent of engine-wide cancellation. Worth adding a regression test alongside the two new ones (AlwaysRun module + already-cancelled engine + own timeout elapsed → expect TimedOut, not PipelineTerminated).

Minor

• ExecuteAsync_ClassifiesElapsedCancellationAsPipelineTerminated relies on a real Task.Delay(25ms) against a 5ms configured timeout to force isTimeoutExceeded. This is timing-based and could be flaky under CI load (a slow scheduler tick could shrink the observed gap). Not blocking, but consider driving executionContext.Stopwatch more deterministically if the test starts flaking, e.g. via a fake/injectable clock rather than a real delay.

Otherwise the refactor is a clean simplification of a previously order-dependent branch tangle into an explicit decision table, and the core fix for the issue is correct and well-tested.

[thomhurst]

Review fix pushed in 952eaeae42.

@claude review
@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Already looking forward to the next diff.

Reviewed commit: 952eaeae42

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Review: Prioritize pipeline cancellation classification (#3876)

Verified locally: checked out pull/3876/merge, built ModularPipelines.Tests.slnf (0 errors), and ran ModuleExecutionPipelineTests directly — all 8 tests pass, including the 4 new ones added in this PR.

The previously flagged blocker is now fixed. Both the earlier Codex review and my prior review on this PR flagged that ClassifyException read the global _engineCancellationToken.IsCancelled flag without excluding AlwaysRun modules, which would misclassify a genuine AlwaysRun cleanup-module timeout as PipelineTerminated just because some unrelated module had already cancelled the engine (since SetupCancellation at ModuleExecutionPipeline.cs:337-360 deliberately does not link AlwaysRun modules' tokens to _engineCancellationToken). The current diff now has:

if (!config.AlwaysRun
    && _engineCancellationToken.IsCancelled
    && exception is OperationCanceledException or ModuleTimeoutException)
{
    return Status.PipelineTerminated;
}

This matches the suggested fix exactly and is architecturally consistent with SetupCancellation's own treatment of AlwaysRun as independent of engine-wide cancellation. The two new regression tests (ExecuteAsync_ClassifiesAlwaysRunTimeoutIndependentlyOfPipelineCancellation, ExecuteAsync_ClassifiesAlwaysRunElapsedCancellationAsTimeout) correctly pin this: an AlwaysRun module that times out after the engine is already cancelled still reports TimedOut, not PipelineTerminated.

Core fix is sound. ClassifyException (ModuleExecutionPipeline.cs:743-758) now checks pipeline-cancellation before timeout inference for non-AlwaysRun modules, which correctly reflects reality: since a non-AlwaysRun module's cancellation token is linked to _engineCancellationToken (SetupCancellation, line 353), a cancellation observed after the engine is already cancelled is genuinely attributable to the pipeline shutting the module down — not a coincidental independent timeout — even if the module's own stopwatch happens to have separately exceeded its configured timeout. Removing the dead ModuleTimeoutException/TaskCanceledException arms from IsTimeout is safe since TaskCanceledException : OperationCanceledException and the ModuleTimeoutException case is handled by the explicit || in ClassifyException.

Minor (not blocking, carried over from the last round)

• ExecuteAsync_ClassifiesElapsedCancellationAsPipelineTerminated and the new ExecuteAsync_ClassifiesAlwaysRunElapsedCancellationAsTimeout both still rely on a real Task.Delay(25ms) against a 5ms configured timeout to force isTimeoutExceeded — now duplicated across two tests instead of one. The 5x margin makes flakiness unlikely, but if either starts flaking under CI load, consider driving executionContext.Stopwatch deterministically (e.g. via a fake/injectable clock) instead of a real delay.
• The new ExecuteAfterPipelineCancellation test helper duplicates the same ModuleExecutionPipeline-construction boilerplate that's repeated inline in several other tests in this file (e.g. around lines 207, 274, 333, 385). That duplication predates this PR and isn't something this diff needs to fix, but since this PR already extracted one reusable helper, a follow-up that consolidates the other four call sites onto it would reduce the file's overall duplication meaningfully.

Nothing else stood out. The refactor is a clean simplification of a previously order-dependent branch tangle into an explicit decision table, the fix for #3791 is correct, and test coverage for the race conditions (including the AlwaysRun edge case) is solid. Approving.