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".

[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.

[coderabbitai]

Warning

Review limit reached

@thomhurst, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 27 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 748d30f5-18b4-454d-9d9d-264011080d2d

📥 Commits

Reviewing files that changed from the base of the PR and between 020a014 and b2f729c.

📒 Files selected for processing (2)

• src/ModularPipelines/Engine/ModuleExecutionPipeline.cs
• test/ModularPipelines.UnitTests/Engine/ModuleExecutionPipelineTests.cs

---

_{Comment @coderabbitai help to get the list of available commands.}

[thomhurst]

Fresh validation triggered in f1093a7ef0 after the prior jobs were externally cancelled. The rebased fix is byte-for-byte identical to the content approved at 952eaeae42; the fixed AlwaysRun thread is resolved.

Local validation on current head:

• ModuleExecutionPipelineTests: 8/8 passed
• ModularPipelines.slnx Release build: 0 warnings, 0 errors

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f1093a7ef0

ℹ️ 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]

Code review

Re-reviewed at head f1093a7ef0 (a rebase-only retrigger, per the PR description and thomhurst's comment).

Confirms previous round is fully addressed. The AlwaysRun misclassification issue raised in my earlier review is resolved: ClassifyException (ModuleExecutionPipeline.cs:743-757) now short-circuits the PipelineTerminated branch with !config.AlwaysRun, so a module configured with .WithAlwaysRun() whose own timeout genuinely elapses is no longer swallowed as "pipeline terminated" just because some unrelated module already cancelled the engine — it correctly reports Status.TimedOut instead. This lines up with SetupCancellation's existing design of decoupling AlwaysRun modules' cancellation tokens from _engineCancellationToken, so the fix is consistent with how the rest of the pipeline already treats that module category.

Test coverage for the fix is solid and directly exercises the previously-buggy paths:

• ExecuteAsync_ClassifiesLateTimeoutAsPipelineTerminated / ExecuteAsync_ClassifiesElapsedCancellationAsPipelineTerminated — regular modules correctly prioritize pipeline cancellation (the original Engine: HandleException status classification — dead ModuleTimeoutException branch and timeout-vs-cancellation ordering #3791 fix).
• ExecuteAsync_ClassifiesAlwaysRunTimeoutIndependentlyOfPipelineCancellation / ExecuteAsync_ClassifiesAlwaysRunElapsedCancellationAsTimeout — AlwaysRun modules correctly report TimedOut even when the engine is already cancelled (the fix from the previous review round).

IsTimeout was correctly narrowed to OperationCanceledException only, since TaskCanceledException : OperationCanceledException (still covered) and the ModuleTimeoutException arm is now handled explicitly in ClassifyException's ||. No behavior gap there.

I don't have anything new to add beyond the minor, non-blocking notes from the last round (the Task.Delay(25ms)-based timing in the two "elapsed cancellation" tests, and the pre-existing test-setup duplication in ExecuteAfterPipelineCancellation vs. other tests in the file) — neither blocks this PR.

No issues found. Checked for bugs and CLAUDE.md compliance.

[thomhurst]

Review regression fixed in a380aa4: the late-timeout test now starts the module before pipeline cancellation, then releases an actual ModuleTimeoutException through classification. Validation: ModuleExecutionPipelineTests 8/8; core Release build 0 warnings/errors; scoped formatting clean.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a380aa437e

ℹ️ 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]

Code review

Re-reviewed at head a380aa437e (post-a380aa437e0 fix). This head still matches what my last round approved for the AlwaysRun-specific classification, and I found one new issue in the current code that's worth a look before merge, plus a minor test-quality nit.

HandleException's new early-return silently bypasses IgnoreFailuresCondition and the timeout warning log for cascaded ModuleTimeoutExceptions

ModuleExecutionPipeline.cs:683-689 returns immediately when ClassifyException yields Status.PipelineTerminated, before reaching:

• the !timeoutException.WasCancellationTokenRespected grace-period warning log (lines 694-702), and
• the config.IgnoreFailuresCondition check (lines 706-725).

Before this PR, a ModuleTimeoutException was always classified TimedOut first (the "dead branch" issue #3791 flagged), so both of those ran unconditionally. Now, for a non-AlwaysRun module, if _engineCancellationToken.IsCancelled is already true when its own ModuleTimeoutException surfaces, ClassifyException reclassifies it as PipelineTerminated and the method returns before either block runs.

Concretely: a module configured with .WithIgnoreFailuresWhen(...) specifically to swallow its own timeouts will stop having that condition honored as soon as any other module has already triggered pipeline cancellation — the module now hard-fails as PipelineTerminated instead. That's a behavior change the linked issue (#3791) didn't ask for; the issue's fix was about classification order, not about skipping the ignore-condition/warning-log side effects once terminated. I didn't find any test in ModuleExecutionPipelineTests.cs (or elsewhere) that exercises IgnoreFailuresCondition together with pipeline cancellation, so this path is currently unverified.

Suggest either running the IgnoreFailuresCondition check (and the warning log, when applicable) before the PipelineTerminated short-circuit, or explicitly deciding — and documenting/testing — that cascaded failures are never eligible for IgnoreFailuresCondition.

Minor: dead Stopwatch setup in ExecuteAsync_ClassifiesElapsedCancellationAsPipelineTerminated

ModuleExecutionPipelineTests.cs:147-156 manually starts/delays/stops the stopwatch before asserting PipelineTerminated, but ElapsedCancellationModule throws a plain OperationCanceledException, which ClassifyException's !AlwaysRun && IsCancelled && exception is OperationCanceledException or ModuleTimeoutException branch matches unconditionally — the elapsed time is never consulted for this path since it short-circuits before IsTimeout. The test would pass identically without the Stopwatch/Task.Delay lines, so it doesn't actually cover an "elapsed cancellation race" the way its name suggests (that race is only meaningfully covered by the AlwaysRun counterpart, ExecuteAsync_ClassifiesAlwaysRunElapsedCancellationAsTimeout, where the elapsed check is load-bearing). Not blocking, just worth tightening the name/setup so it doesn't imply coverage it doesn't have.

Everything else from the previous rounds (the AlwaysRun misclassification fix, IsTimeout narrowing to OperationCanceledException) still looks correct and well covered.

[thomhurst]

Addressed the exact-head review regressions in \535e49d866:\n\n- timeout warnings and \IgnoreFailuresCondition\ now run before the \PipelineTerminated\ short-circuit, preserving prior behavior;\n- the timeout regression cancels the engine from \InvokeFailedAsync, after \ModuleTimeoutException\ is already caught but before classification;\n- added ignored-timeout-after-cancellation coverage and verified the non-cooperative warning;\n- removed dead elapsed-time setup from the plain cancellation test.\n\nValidation: \ModuleExecutionPipelineTests\ 9/9; core Release build 0 warnings/errors.\n\n@claude review\n@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 535e49d866

ℹ️ 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]

Code review

Re-reviewed at head 535e49d866 (fix "preserve cascaded timeout handling"), which addresses the one blocking issue from my last round.

The IgnoreFailuresCondition/warning-log bypass is fixed correctly. In HandleException (ModuleExecutionPipeline.cs), the Status.PipelineTerminated early-return block was moved down past both the !WasCancellationTokenRespected grace-period warning log and the config.IgnoreFailuresCondition check, instead of short-circuiting before them. That's exactly the fix I suggested: a module can still have its own timeout ignored via .WithIgnoreFailuresWhen(...) even after the engine has already been cancelled by another module, and the grace-period warning still logs regardless of classification. This restores the pre-#3791 side-effect ordering while keeping the classification-order fix that #3791 was actually about.

Test coverage now directly exercises this:

• ExecuteAsync_HonorsIgnoredTimeoutAfterPipelineCancellation — new test, asserts Status.IgnoredFailure for a module with .WithIgnoreFailures() whose timeout surfaces after the pipeline is already cancelled. This is the exact regression scenario from my last review.
• ExecuteAsync_ClassifiesLateTimeoutAsPipelineTerminated now also asserts the grace-period warning log actually fires via logger.Verify(...), closing the other half of the gap.

The minor test-quality nit is also resolved. ExecuteAsync_ClassifiesElapsedCancellationAsPipelineTerminated was renamed to ExecuteAsync_ClassifiesPipelineCancellationAsPipelineTerminated and the dead Stopwatch/Task.Delay setup that didn't actually influence the assertion was removed, so the test name and setup now match what's actually being verified.

I built ModularPipelines.slnx/ModularPipelines.Tests.slnf (Release) and ran ModuleExecutionPipelineTests locally: 9/9 passing, 0 build errors.

One forward-looking (non-blocking) observation: this round-trip (issue → fix → re-review) happened because HandleException packs classification, conditional logging, ignore-condition evaluation, and cancellation/result-finalization into one method where a step's position relative to the PipelineTerminated short-circuit silently changes its behavior. Now that the ordering is log warning → check IgnoreFailuresCondition → check PipelineTerminated → cancel & throw, it'd be worth pulling those into small, explicitly-named private steps (e.g. LogTimeoutGraceWarningIfNeeded, TryIgnoreFailure, FinalizeTerminatedResult) called in sequence from HandleException. That wouldn't change behavior, but it would make the ordering invariant self-documenting instead of relying on where a block physically sits in the method, which is exactly what caused this regression to slip through unnoticed the first time.

No blocking issues found. Approved from a correctness/CLAUDE.md-compliance standpoint.