Apply module timeout per retry attempt

[thomhurst]

Summary

• enforce module timeout independently for every retry attempt
• exclude retry backoff from timeout and cancellation-cooperation accounting
• document timeout/retry composition across API docs and guides

Validation

• RetryTests: 8 passed
• ModuleTimeoutTests: 12 passed
• TelemetryIntegrationTests: 18 passed
• ModularPipelines.slnx Release build: 0 warnings, 0 errors

Closes #3790

[claude]

Code review

Summary: Moves the module WithTimeout wrapper from around the whole retry chain (all attempts + backoff) to inside each individual retry attempt, so each attempt gets a fresh timeout budget and backoff delays no longer count against it (closes #3790). Also drops the moduleAttemptRespondedToCancellation aggregate flag in favor of per-attempt TimeoutExecutionResult.WasCancellationTokenRespected.

CLAUDE.md compliance: Two independent passes found no violations. Nothing in the diff touches generated options classes, build scripts, or .slnx/CI files that CLAUDE.md's rules govern.

Bugs: Two independent passes (one confirmed by an actual dotnet build of the core project) found no compile errors and no "wrong regardless of input" logic errors in the diff itself.

Architecture / design concerns — the semantic shift from "timeout wraps the chain" to "timeout wraps one attempt" has a few ripple effects elsewhere in ExecuteWithPolicies/HandleException that are worth resolving before merge rather than as follow-ups, since they weaken guarantees the timeout feature exists to provide:

1. Non-cooperative modules can now overlap themselves across retries

ModularPipelines/src/ModularPipelines/Engine/ModuleExecutionPipeline.cs

Lines 491 to 514 in c7bc225

	// Keep timeout enforcement inside the retry policy so each attempt gets a fresh budget
	// and policy-owned backoff delays are not mistaken for unresponsive module execution.
	async Task<T> ExecuteModuleAttempt(CancellationToken ct)
	{
	Interlocked.Increment(ref moduleAttemptCount);
	var timeoutResult = await TimeoutHelper.ExecuteWithTimeoutAndDetailsAsync(
	attemptToken => module.ExecuteAsync(moduleContext, attemptToken),
	timeout == TimeSpan.Zero ? null : timeout,
	ct,
	$"Module {executionContext.ModuleType.Name} timed out after {timeout}").ConfigureAwait(false);
	if (timeoutResult.TimedOut)
	{
	throw new ModuleTimeoutException(
	executionContext.ModuleType,
	timeout,
	timeoutResult.ElapsedTime,
	timeoutResult.WasCancellationTokenRespected);
	}
	return timeoutResult.Value!;
	}

TimeoutHelper.ExecuteWithTimeoutAndDetailsAsync abandons (doesn't await/cancel) the underlying task after a 1s grace period if the module ignores its CancellationToken. Previously the outer timeout wrapped the whole retry chain, so attempts were strictly sequential and at most one abandoned execution could ever exist. Now that the timeout is scoped per-attempt, Polly re-invokes ExecuteModuleAttempt (starting a new module.ExecuteAsync on the same module instance) as soon as the current attempt throws ModuleTimeoutException — without waiting for the previous abandoned task to actually finish. For a non-cooperative module with RetryCount = N, up to N + 1 overlapping executions of the same instance can be alive concurrently, racing over shared instance fields, the shared IModuleContext, working directory, and artifact paths.

Suggested approach: since TimeoutExecutionResult already reports whether the attempt's task actually completed (WasCancellationTokenRespected / grace-period outcome), treat "timed out and abandoned" as non-retryable — surface it through the retry policy's Handle predicate (or throw a distinct exception type for that case) so the policy fails fast instead of stacking new executions on top of an orphaned one. This keeps the per-attempt timeout behavior for the cooperative-module case the PR targets, without introducing self-racing modules for the non-cooperative case.

2. Per-attempt timeout removes the only overall wall-clock ceiling

ModularPipelines/src/ModularPipelines/Engine/ModuleExecutionPipeline.cs

Lines 515 to 520 in c7bc225

	try
	{
	return retryPolicy != null
	? await retryPolicy.ExecuteAsync(ExecuteModuleAttempt, cancellationToken).ConfigureAwait(false)
	: await ExecuteModuleAttempt(cancellationToken).ConfigureAwait(false);
	}

ModuleRetryPolicyFactory.Create defaults to Policy.Handle<Exception>() when no ShouldRetry filter is configured, so the ModuleTimeoutException thrown per attempt is retryable by default. Before this PR, the single outer timeout was a hard ceiling on total module time. After this PR, a hung module can occupy a pipeline slot for timeout * (RetryCount + 1) plus backoff, and there's no config knob left that expresses "this module must never take longer than X overall" — something CI schedulers/orchestrators typically rely on.

Suggested approach: keep per-attempt as the primary knob, but restore an overall deadline via a linked token — CancellationTokenSource.CreateLinkedTokenSource(cancellationToken) with CancelAfter(overallTimeout) created once in ExecuteWithPolicies and passed to retryPolicy.ExecuteAsync — which bounds attempts and backoff delays (Polly's delay awaits honor the token) without reintroducing the "backoff counts against timeout" problem this PR is fixing for the per-attempt case.

3. IsTimeout's cumulative-elapsed heuristic is now comparing against the wrong scale

ModularPipelines/src/ModularPipelines/Engine/ModuleExecutionPipeline.cs

Lines 731 to 743 in c7bc225

	private bool IsTimeout(ModuleConfiguration config, ModuleExecutionContext executionContext, Exception exception)
	{
	var timeout = GetTimeout(config);
	if (timeout == TimeSpan.Zero)
	{
	return false;
	}
	var isTimeoutExceeded = executionContext.Stopwatch.Elapsed >= timeout;
	return isTimeoutExceeded && exception is ModuleTimeoutException or TaskCanceledException or OperationCanceledException;
	}

(used at

ModularPipelines/src/ModularPipelines/Engine/ModuleExecutionPipeline.cs

Lines 657 to 681 in c7bc225

	executionContext.Exception = exception;
	// Check for timeout - use the enhanced exception type for detailed logging
	if (exception is ModuleTimeoutException timeoutException)
	{
	executionContext.Status = Status.TimedOut;
	// Log additional timeout details
	if (!timeoutException.WasCancellationTokenRespected)
	{
	logger.LogWarning(
	"Module {ModuleName} did not complete within the cancellation grace period; timeout enforcement stopped waiting after {ElapsedTime}",
	executionContext.ModuleType.Name,
	timeoutException.ElapsedTime.ToDisplayString());
	}
	}
	else if (IsTimeout(config, executionContext, exception))
	{
	executionContext.Status = Status.TimedOut;
	}
	// Check for pipeline cancellation
	else if (IsPipelineCancelled(exception))
	{
	executionContext.Status = Status.PipelineTerminated;

)

This method isn't touched by the diff, but its correctness silently depends on what GetTimeout(config) means — and this PR redefines that from "budget for the whole chain" to "budget for one attempt," without updating the method. IsTimeout only matters for exceptions that reach HandleException as OperationCanceledException/TaskCanceledException (i.e., genuine external/pipeline cancellation — direct timeouts already short-circuit via the exception is ModuleTimeoutException branch above it). It classifies those as Status.TimedOut whenever cumulative executionContext.Stopwatch.Elapsed >= timeout. Since Stopwatch.Elapsed spans the whole retry chain (all attempts + backoff) while timeout is now just one attempt's budget, this condition becomes true almost as soon as a single retry has occurred — so cancelling a pipeline (Ctrl+C, CI cancellation) while a module happens to be on its 2nd+ attempt will likely be misreported as Status.TimedOut instead of falling through to IsPipelineCancelled → Status.PipelineTerminated, giving misleading status/telemetry for a cancelled build.

Suggested approach: check IsPipelineCancelled before IsTimeout (cheap reordering), or better — delete IsTimeout entirely. With this PR's change, every genuine per-attempt timeout is now always surfaced as a typed ModuleTimeoutException from ExecuteModuleAttempt, so the duration-based heuristic in IsTimeout is redundant for its original purpose and only remains as a source of misclassification for real cancellations.

Nice cleanup on the moduleAttemptRespondedToCancellation removal — sourcing cooperation directly from the failing attempt's TimeoutExecutionResult is more accurate than the old chain-aggregated flag.

[thomhurst]

Addressed the review at ab212bcf5b:

• Non-cooperative timed-out attempts now bypass even broad custom Polly retry policies, preventing overlapping executions of the same module instance.
• Removed the cumulative stopwatch timeout heuristic; external cancellation during a later attempt is now PipelineTerminated, not TimedOut.
• Added focused regressions for both behaviors and clarified the no-retry rule in both timeout/retry docs.

The suggested overall wall-clock deadline was not added: #3790 intentionally defines WithTimeout as a per-attempt budget excluding backoff, and the docs explicitly state total time can be (attempts × timeout) + backoff. A separate total-time API would be a distinct public contract.

Validation: RetryTests 10/10; ModuleTimeoutTests 12/12; core Release build 0 warnings/errors.

[claude]

Code review (re-review at ab212bcf5b)

Scope: This re-reviews the PR after the second commit (ab212bcf5b), which responds to the prior automated review posted against the first commit (c7bc22599a). I re-read the full current diff and the surrounding, unchanged code it depends on (TimeoutHelper.ExecuteWithTimeoutAndDetailsAsync, ModuleRetryPolicyFactory, HandleException) rather than trusting the summary in the PR/issue comment.

Verdict: the three concerns from the prior review are all genuinely fixed, not just claimed fixed.

1. Non-cooperative overlap — fixed correctly

ExecuteModuleAttempt now returns (T? Value, ModuleTimeoutException? NonRetryableTimeout) instead of throwing directly. When WasCancellationTokenRespected is false (grace period elapsed, module ignored the token, task abandoned), it returns the exception in the tuple rather than throwing it. Because Polly's retry policies here are exception-based only (ModuleRetryPolicyFactory.Create uses Policy.Handle<Exception>(...), and AdvancedRetryPolicyFactory is typed as Func<IModuleContext, IAsyncPolicy> — non-generic, so no Policy<T>.OrResult path exists anywhere in this codebase), a returned-not-thrown result is invisible to the retry predicate. The policy sees a "successful" execution, doesn't retry, and the outer code re-throws NonRetryableTimeout once control returns from retryPolicy.ExecuteAsync. That's a clean way to keep timeout-as-exception for the cooperative case while making the non-cooperative case genuinely non-retryable, without needing a new exception type or touching the Handle predicate surface. moduleAttemptCount also stays correct (1 attempt, 0 retries recorded) since Polly never re-invokes the delegate. The new When_Timed_Out_Attempt_Remains_Active_Then_Do_Not_Retry test exercises exactly this path.

2. Overall wall-clock ceiling — intentionally not restored, and that's defensible

The PR does not add a total-duration deadline; the issue comment explains this was deliberate because #3790 defines WithTimeout as a per-attempt budget and the docs now say total time can be (attempts × timeout) + backoff. I agree this is reasonable to leave for a follow-up rather than block on: bundling a new "overall deadline" API into a bug-fix PR would be scope creep, and the behavior is now clearly documented (timeouts.md: "a module configured with a five-minute timeout and three retries can therefore spend up to five minutes in each of its four attempts, plus retry delays"). One suggestion for a fast-follow, not this PR: since orchestrators (CI schedulers) do rely on an outer ceiling, it'd be worth a short GitHub issue capturing the CancellationTokenSource.CreateLinkedTokenSource + CancelAfter approach floated in the earlier review, so the idea isn't lost.

3. IsTimeout cumulative-elapsed heuristic — removed, and verified against TimeoutHelper

IsTimeout and its call site are deleted rather than patched, which is the right call — I traced TimeoutHelper.ExecuteWithTimeoutAndDetailsAsync directly to confirm the replacement is sound: when the external cancellationToken fires (not the internal per-attempt timeout token), it throws OperationCanceledException(cancellationToken) directly rather than producing a TimeoutExecutionResult.TimedOut. That exception is not a ModuleTimeoutException, so in HandleException it skips the exception is ModuleTimeoutException branch and correctly lands in IsPipelineCancelled → Status.PipelineTerminated. Genuine per-attempt timeouts, by contrast, always surface as a typed ModuleTimeoutException, so the duration-heuristic really was redundant for its original purpose and only a source of misclassification, as the prior review suspected. When_Cancelled_During_Later_Attempt_Then_Report_PipelineTerminated directly covers the scenario the old heuristic got wrong (cancellation while a module is into its 2nd+ attempt).

Minor observations (non-blocking)

• The unrelated-looking diff to ModuleConfiguration.cs/ModuleConfigurationBuilder.cs (removal of CacheAssemblyVersionKey) that shows up when diffing against current main is just merge-base drift — feat(cache): support stable assembly keys (#3830) landed on main after this branch forked. It's not part of this PR's actual diff (confirmed via git diff <merge-base> <head> and gh pr diff), so nothing to act on here; it'll disappear on rebase/merge.
• Docs updates (timeouts.md, retry-policy.md) accurately describe the new per-attempt/grace-period/non-retryable semantics and match the code.

CLAUDE.md compliance: No violations — no generated options classes, build scripts, or .slnx/CI files touched.

Bugs: None found in this pass. The tuple-return approach for the non-retryable path is a little unusual stylistically (returning an exception instead of throwing it), but it's well-contained to one local function and is what makes the fix correct given Polly's exception-only retry model here — a comment above ExecuteModuleAttempt explaining why the non-retryable path returns instead of throws (to bypass the exception-based retry predicate) would help the next reader who might otherwise "simplify" it back to a throw.

Nice iteration — both structural issues from the first pass (self-overlapping retries, misclassified cancellations) are addressed with targeted regression tests rather than broad rewrites.

[claude]

Code review (re-review at c5747fd3, the current merge commit)

Scope: Diffed the current PR head against the commit reviewed last time (ab212bcf) restricted to the files that carry this PR's actual logic (ModuleExecutionPipeline.cs, Configuration/*, RetryTests.cs, timeouts.md, retry-policy.md). The only changes since the last review are unrelated main-branch drift pulled in by the merge commit (the WithCacheAssemblyVersionKey / MVID cache-fingerprint feature from #3830 and its docs) — none of it touches timeout/retry behavior. I independently re-verified the three structural concerns raised in the first review rather than trusting the prior "addressed" comment at face value:

1. Non-cooperative overlap — ExecuteModuleAttempt returns (T?, ModuleTimeoutException?) and only returns (never throws) when WasCancellationTokenRespected is false, so Polly's exception-based retry predicate never sees it as retryable and no second attempt races the abandoned one. Confirmed ModuleRetryPolicyFactory and the advanced-policy path (AdvancedRetryPolicyFactory is Func<IModuleContext, IAsyncPolicy>, non-generic) have no OrResult<T> predicate anywhere that could observe/retry the tuple's "success" — so this holds for both the built-in and custom-policy paths, not just the default one.
2. Overall wall-clock ceiling — still intentionally not restored; the PR/issue comment's rationale (per-attempt budget is the documented contract per #3790, total time is now (attempts × timeout) + backoff and that's spelled out in timeouts.md) is reasonable to leave as a documented follow-up rather than scope-creep into this fix.
3. IsTimeout cumulative-elapsed heuristic — confirmed removed along with its only call site, and that external cancellation still correctly reaches IsPipelineCancelled → Status.PipelineTerminated via the direct OperationCanceledException(cancellationToken) thrown by TimeoutHelper (not a ModuleTimeoutException), rather than being misclassified as TimedOut.

Also spot-checked moduleAttemptCount bookkeeping in the finally block: for the non-retryable-timeout path the count stays at 1 (Polly never re-invokes the delegate since it saw a "successful" tuple), so ModuleActivityTracing.RecordModuleRetries correctly reports 0 retries rather than double-counting.

Tests: When_Retry_Backoff_Exceeds_Timeout_Then_All_Attempts_Run, When_Retry_Timeouts_During_Module_Then_Report_Token_Respected, When_Timed_Out_Attempt_Remains_Active_Then_Do_Not_Retry, and When_Cancelled_During_Later_Attempt_Then_Report_PipelineTerminated each target exactly one of the behaviors above rather than re-asserting the same path, and the new CancelledDuringRetryModule/NonCancellableModuleWithTimeout fixtures are minimal and purpose-built.

CLAUDE.md compliance: No violations — no generated options classes, build scripts, or .slnx/CI files touched by this PR's actual diff.

No new issues found. This has now had two structural passes plus this confirmation pass with all raised concerns fixed and regression-tested — approving.

[claude]

Code review

Summary: Follow-up to the earlier automated review on this PR. That review raised three concerns about moving the timeout from wrapping the whole retry chain to wrapping each attempt: (1) non-cooperative modules could overlap themselves across retries, (2) per-attempt timeout removes the only overall wall-clock ceiling, (3) the cumulative-elapsed IsTimeout heuristic would misclassify cancellation as timeout once scoped per-attempt. I re-read the current diff (c7bc225 → 6c95fae) against those points.

#1 (self-overlap) — fixed. ExecuteModuleAttempt now returns (T? Value, ModuleTimeoutException? NonRetryableTimeout) instead of throwing when WasCancellationTokenRespected is false (ModuleExecutionPipeline.cs:503-517). Returning normally rather than throwing means Polly's Handle<Exception>-based policy (the only kind ModuleRetryPolicyFactory/AdvancedRetryPolicyFactory support — both are non-generic IAsyncPolicy, so there's no OrResult path that could still act on this sentinel) never sees an exception to retry on, so a hung/non-cooperative attempt is never retried while its abandoned execution is still live. The exception is only re-thrown once outside the retry policy (ExecuteWithPolicies line 528-531). This is exercised by the new When_Timed_Out_Attempt_Remains_Active_Then_Do_Not_Retry test, and the behavior is documented in retry-policy.md/timeouts.md.

#3 (IsTimeout heuristic) — fixed by removal. The cumulative-stopwatch IsTimeout method and its call site are deleted entirely. Genuine per-attempt timeouts now always arrive at HandleException as a typed ModuleTimeoutException, and IsPipelineCancelled already includes ModuleTimeoutException in its exception-type check, so a timeout that races with external/pipeline cancellation correctly resolves to Status.PipelineTerminated rather than Status.TimedOut. The new When_Cancelled_During_Later_Attempt_Then_Report_PipelineTerminated test covers exactly this race (cancel while a module is on a later attempt) and asserts PipelineTerminated.

#2 (overall wall-clock ceiling) — knowingly not addressed, and I think that's the right call. The PR comment explains this was deliberately left out: WithTimeout is being defined as a per-attempt budget by design (closes #3790), and both docs now spell out that total time can be (attempts × timeout) + backoff. Adding an overall deadline on top would be a separate, additive API (e.g. a linked CancelAfter token) rather than a fix folded into this change, so deferring it as a distinct feature/issue is reasonable rather than a gap in this PR.

Verification of the fix's mechanics: I traced TimeoutHelper.ExecuteWithTimeoutAndDetailsAsync to confirm the two paths line up with the new tests — a non-cooperative attempt (task still running after the 1s grace period) yields TimeoutWithTokenIgnored, which is the path that now returns instead of throws; a cooperative attempt or one that observes real pipeline cancellation still throws (either ModuleTimeoutException with WasCancellationTokenRespected = true, or OperationCanceledException when cancellationToken.IsCancellationRequested), and Polly itself won't retry once the passed-in CancellationToken is already cancelled, so that case can't loop either. This all matches the four scenarios the new RetryTests cases assert.

Minor note, not a blocker: suppressing the exception for the non-cooperative-timeout case means a caller who supplies a custom ShouldRetry/AdvancedRetryPolicyFactory that explicitly wants to retry on ModuleTimeoutException regardless of cooperation can no longer do so for that specific case — the engine now overrides that choice for safety (preventing concurrent executions of the same module instance). That's a reasonable and clearly documented tradeoff, just worth being aware of as a behavioral edge case for anyone with a custom policy keyed off exception type.

Build/tests: Did not re-run the full suite locally (local dotnet guard script hit an unrelated argument-parsing issue with response-file switches, not something introduced by this PR). Relying on the PR's own reported validation (RetryTests 10/10, ModuleTimeoutTests 12/12, TelemetryIntegrationTests 18/18, core Release build clean) combined with the code trace above, which is consistent with that result.

Overall: this addresses the two real correctness issues from the prior review with targeted tests and clear docs, and gives a reasoned justification for not folding in the third (opt-in) suggestion. No further blocking issues found.

[thomhurst]

Refreshed onto current main at c06b680. Revalidated the timeout/retry behavior: RetryTests 10/10, ModuleTimeoutTests 12/12, TelemetryIntegrationTests 18/18, core Release build 0 warnings/errors. @claude review

[claude]

Code review

Scope: Independent re-review of the current head (c06b680f, merge of 6f4a3027 into issue-3790-attempt-timeout). This PR has already been through four automated review rounds that raised and resolved three structural concerns (non-cooperative self-overlap on retry, loss of an overall wall-clock ceiling, and a stale cumulative-elapsed IsTimeout heuristic). I re-verified those are genuinely fixed in the current diff rather than re-litigating them:

• ExecuteModuleAttempt returns (T?, ModuleTimeoutException?) and only returns (never throws) when WasCancellationTokenRespected is false. Since ModuleRetryPolicyFactory/AdvancedRetryPolicyFactory are exception-only (Policy.Handle<Exception>, non-generic IAsyncPolicy — no OrResult<T> predicate anywhere), this correctly makes a non-cooperative timed-out attempt non-retryable without throwing a fresh exception type. Confirmed.
• IsTimeout and its cumulative-Stopwatch.Elapsed heuristic are deleted outright rather than patched. Traced TimeoutHelper.ExecuteWithTimeoutAndDetailsAsync: genuine per-attempt timeouts always surface as typed ModuleTimeoutException, and true external cancellation throws a bare OperationCanceledException(cancellationToken) that correctly falls through to IsPipelineCancelled → Status.PipelineTerminated. Confirmed.
• The overall-wall-clock-ceiling tradeoff is clearly documented in timeouts.md/retry-policy.md ((attempts × timeout) + backoff) and reasonably deferred as a separate feature rather than folded into this bug fix.

New finding: When_Cancelled_During_Later_Attempt_Then_Report_PipelineTerminated has a real race window

test/ModularPipelines.UnitTests/Execution/RetryTests.cs (the CancelledDuringRetryModule fixture and its test, ~lines 307-333 and 414-437):

.WithTimeout(TimeSpan.FromMilliseconds(50))
.WithRetry(1, TimeSpan.FromMilliseconds(250))
...
_secondAttemptStarted.TrySetResult();
await Task.Delay(Timeout.InfiniteTimeSpan, cancellationToken);

await module.SecondAttemptStarted.WaitAsync(TimeSpan.FromSeconds(5));
cancellationTokenSource.Cancel();

The per-attempt timeout (50ms) is now scoped to this specific attempt — that's the whole point of the PR. The 50ms CancelAfter clock starts the instant ExecuteModuleAttempt begins attempt #2, essentially before ExecuteAsync runs its first line. The test's ability to react to _secondAttemptStarted and call Cancel() depends on a RunContinuationsAsynchronously TCS continuation being scheduled and run on the thread pool, then the test method resuming and calling Cancel() — all of which has to complete inside that same 50ms window for the test to reliably exercise "external cancellation wins." Under CI load (thread-pool contention, GC pauses, parallel test execution), it's entirely plausible for the internal 50ms attempt timeout to fire first, in which case the assertion changes from Status.PipelineTerminated to Status.TimedOut and the test fails — not because the fix regressed, but because the test's own timing margin is too tight against its own scenario.

Worth noting this module's own file elsewhere establishes 250ms as the working per-attempt timeout for timing-sensitive tests (ModuleTimeoutMs = 250, used by CancellableModuleWithTimeout/FailedModuleWithTimeout) — this new fixture uses 50ms instead, 5x tighter, for the one test that also has an external signal to race against.

Suggested fix: widen the per-attempt timeout for CancelledDuringRetryModule to something like 2-5 seconds (well beyond TCS-continuation + Cancel() scheduling latency), so the internal timeout can't plausibly preempt the external cancellation under load, while SecondAttemptStarted.WaitAsync(TimeSpan.FromSeconds(5)) already tolerates the extra wall-clock time. That removes the coin-flip and keeps the test asserting exactly the scenario it's named for (cancellation arriving during a later attempt, not a timeout race).

This is a test-only concern — it doesn't affect the production fix, which looks correct — but as-is it's a plausible source of CI flakiness for exactly the behavior this PR is trying to pin down with regression coverage, so worth tightening before merge.

No other issues found. The engine change itself (ModuleExecutionPipeline.cs) is a clean, well-contained refactor: timeout enforcement moved inside the retry-attempt closure, the non-retryable-timeout path is handled without a new exception type, and the docs accurately describe the new per-attempt/backoff-excluded semantics.

[thomhurst]

Addressed the latest review: widened only CancelledDuringRetryModule's per-attempt timeout from 50 ms to 2 s, removing the scheduling race while preserving the external-cancellation scenario. Validation: RetryTests 10/10, ModuleTimeoutTests 12/12, TelemetryIntegrationTests 18/18, core Release build 0 warnings/errors. Current head f9d1c24.

[thomhurst]

@claude review

[claude]

Code review

Reviewed the diff, the current ExecuteWithPolicies<T> implementation in ModuleExecutionPipeline.cs, TimeoutHelper.cs, and ModuleRetryPolicyFactory.cs, plus the earlier review rounds on this PR.

CLAUDE.md compliance: No violations. The changed files are core framework/config types (not generated options classes), stay within ModularPipelines.slnx scope, and the added tests follow the documented Module<T> / AddModule<T>() pattern.

Design is sound overall — moving timeout enforcement inside the retry-policy delegate so each attempt gets its own budget, and excluding backoff from that budget, is the right fix for the bug described in #3790. Two things worth a look before/after merge:

1. Non-cooperative timeouts are reported to the wrapped Polly policy as a success, not just "don't retry"

ModularPipelines/src/ModularPipelines/Engine/ModuleExecutionPipeline.cs

Lines 512 to 519 in f9d1c24

	if (!timeoutResult.WasCancellationTokenRespected)
	{
	return (default, timeoutException);
	}
	throw timeoutException;
	}

if (!timeoutResult.WasCancellationTokenRespected)
{
    return (default, timeoutException);   // returned normally, no throw
}
throw timeoutException;

ExecuteModuleAttempt is passed straight into retryPolicy.ExecuteAsync(...), and retryPolicy can be any IAsyncPolicy supplied via .Advanced.WithRetryPolicy(...) — a circuit breaker, a fallback, a policy wrap with onRetry/onBreak callbacks, etc. Because the delegate returns without throwing, every one of those policies sees this as a successful execution: a circuit breaker's failure count won't increment (a module that hangs and ignores cancellation on every call can never trip the breaker), FallbackAsync won't fire, and onRetry/onBreak telemetry is silently skipped. The actual failure only surfaces afterwards, outside the policy, via the NonRetryableTimeout check at line 530.

This looks like an accidental side effect of the mechanism that was added for the earlier "bypass retries for non-cooperative timeouts" fix, rather than an intended behavior — the docs describe "the engine bypasses retries," not "the engine reports success to the policy chain." A cleaner way to get the same non-re-entrancy guarantee without corrupting arbitrary user policies: keep a per-invocation flag/field for "this module instance has an abandoned attempt still running," and have ExecuteModuleAttempt check it up front on each call — if set, rethrow the stored ModuleTimeoutException immediately without invoking module.ExecuteAsync again. That way the delegate still reports a genuine failure to whatever policy wraps it (so breakers/fallbacks/telemetry behave correctly), while the module itself is guaranteed never to be re-entered concurrently.

2. Grace-period TimeoutException handling can misclassify a task that actually did complete

ModularPipelines/src/ModularPipelines/Helpers/TimeoutHelper.cs

Lines 162 to 169 in f9d1c24

	}
	catch (TimeoutException)
	{
	// Task still didn't complete - definitely not respecting the token
	taskRespondedDuringGrace = false;
	TaskObservation.ObserveFault(executionTask);
	}
	catch (OperationCanceledException)

catch (TimeoutException)
{
    // Task still didn't complete - definitely not respecting the token
    taskRespondedDuringGrace = false;
    TaskObservation.ObserveFault(executionTask);
}

Task.WaitAsync(GracePeriod, CancellationToken.None) throws System.TimeoutException both when the grace period itself elapses and when the awaited task faults with its own unrelated TimeoutException during that window (e.g. a module whose own ExecuteAsync awaits a command with ExecutionTimeout — this repo's own Command.cs CreateTimeoutException throws a bare System.TimeoutException, so this isn't just a hypothetical). Both cases land in the same catch and are indistinguishable, so a module that did complete (with its own timeout fault) inside the grace period gets taskRespondedDuringGrace = false.

That ambiguity already existed, but this PR raises its stakes: WasCancellationTokenRespected used to be informational/logging only, and now it's a hard retry-gate (finding above) — so a false false here permanently suppresses retries for an attempt that actually did respond. Disambiguating is simple: check executionTask.IsCompleted in the catch (or after it) instead of relying solely on the exception type — if the task has completed, it responded, regardless of what it faulted with.

Nice test coverage for the new per-attempt/non-retryable/external-cancellation scenarios (CancelledDuringRetryModule, NonCancellableModuleWithTimeout, etc.) — they exercise the three cases this PR changes behavior for.

[thomhurst]

Required pipeline failed only on timing-sensitive Dispose_WaitsForInProgressLogAdmission under full CI load; rerunning failed jobs once on the unchanged head.

[thomhurst]

Addressed the current-head review in 99cc37c: abandoned non-cooperative timeouts now throw through the wrapped Polly policy while subsequent invocations short-circuit before module re-entry; grace-period TimeoutException handling now checks task completion. Validation: RetryTests 10/10 and ModuleTimeoutTests 13/13 passed.

[claude]

Code review (re-review at 99cc37c733, current head)

Scope: This PR has already been through several automated review rounds. The most recent one (at f9d1c24ccd) raised two concerns about the non-cooperative-timeout handling introduced to fix self-overlapping retries. The author's latest comment claims both are addressed at 99cc37c733. I independently re-traced the current diff against those claims rather than trusting the summary.

Verified: both prior findings are genuinely fixed

1. Non-cooperative timeouts reported as a fake "success" to the wrapped policy — fixed correctly.
   

   ModularPipelines/src/ModularPipelines/Engine/ModuleExecutionPipeline.cs

   Lines 98 to 140 in 99cc37c

   	var skippedResult = await HandleSkipped(
   	module,
   	executionContext,
   	moduleContext,
   	skipDecision,
   	logger)
   	.ConfigureAwait(false);
   	await finalizer.FinalizeAsync(skippedResult).ConfigureAwait(false);
   	executionContext.SetTypedResult(skippedResult);
   	finalizer.Complete(skippedResult);
   	return skippedResult;
   	}
   	// Check for cancellation after skip check
   	executionContext.ModuleCancellationTokenSource.Token.ThrowIfCancellationRequested();
   	if (prepareExecutionAsync is not null)
   	{
   	await prepareExecutionAsync(executionContext.ModuleCancellationTokenSource.Token)
   	.ConfigureAwait(false);
   	}
   	var cachedResult = await TryUseCachedResultAsync(
   	module,
   	config,
   	executionContext,
   	moduleContext,
   	logger)
   	.ConfigureAwait(false);
   	if (cachedResult is not null)
   	{
   	await finalizer.FinalizeAsync(cachedResult).ConfigureAwait(false);
   	executionContext.SetTypedResult(cachedResult);
   	finalizer.Complete(cachedResult);
   	return cachedResult;
   	}
   	// Execute direct before hook first (virtual override)
   	await _directHookInvoker.InvokeBeforeExecuteAsync(module, moduleContext, executionContext.ModuleCancellationTokenSource.Token).ConfigureAwait(false);
   	// Track that the before hook executed (for OnAfterExecuteAsync in finally)
   	beforeHooksExecuted = true;

   
   ExecuteModuleAttempt now throws abandonedAttemptTimeout on every call once an attempt is abandoned (the if (abandonedAttemptTimeout is not null) throw ...; guard at the top), instead of the previous approach of returning it as a normal value. This means any wrapped Polly policy (circuit breaker, fallback, onRetry/onBreak telemetry) sees a genuine failure on every subsequent "retry" — but because the guard fires before module.ExecuteAsync is ever called again, the module instance itself is still never re-entered concurrently with its abandoned attempt. The new When_Timed_Out_Attempt_Remains_Active_Then_Do_Not_Retry test confirms both halves: ExecutionCount == 1 (no re-entrancy) and RetryCallbackCount == DefaultRetryCount (the policy's onRetry still fires normally). Good fix — it resolves the correctness/telemetry issue without reopening the original self-overlap bug.

2. Grace-period TimeoutException misclassifying a task that actually completed — fixed correctly.
   https://github.com/thomhurst/ModularPipelines/blob/99cc37c7335a3ea7539f9b3c974c686e72d70b8a/src/ModularPipelines/Helpers/TimeoutHelper.cs#L235-L249
   The catch block now checks executionTask.IsCompleted instead of assuming any TimeoutException means the grace period expired, exactly as suggested. The new Timeout_Fault_During_Grace_Period_Counts_As_Response test exercises the ambiguous case (task completes with its own TimeoutException fault during the grace window) and asserts WasCancellationTokenRespected == true. This matters because that flag is now a hard retry-gate (per fix Feature/initial work #1 above), so this fix prevents a module that actually did respond from being permanently marked non-retryable.

New observation (non-blocking): exponential backoff still fully elapses after the outcome is already known

ModularPipelines/src/ModularPipelines/Engine/ModuleExecutionPipeline.cs

Lines 98 to 140 in 99cc37c

	var skippedResult = await HandleSkipped(
	module,
	executionContext,
	moduleContext,
	skipDecision,
	logger)
	.ConfigureAwait(false);
	await finalizer.FinalizeAsync(skippedResult).ConfigureAwait(false);
	executionContext.SetTypedResult(skippedResult);
	finalizer.Complete(skippedResult);
	return skippedResult;
	}
	// Check for cancellation after skip check
	executionContext.ModuleCancellationTokenSource.Token.ThrowIfCancellationRequested();
	if (prepareExecutionAsync is not null)
	{
	await prepareExecutionAsync(executionContext.ModuleCancellationTokenSource.Token)
	.ConfigureAwait(false);
	}
	var cachedResult = await TryUseCachedResultAsync(
	module,
	config,
	executionContext,
	moduleContext,
	logger)
	.ConfigureAwait(false);
	if (cachedResult is not null)
	{
	await finalizer.FinalizeAsync(cachedResult).ConfigureAwait(false);
	executionContext.SetTypedResult(cachedResult);
	finalizer.Complete(cachedResult);
	return cachedResult;
	}
	// Execute direct before hook first (virtual override)
	await _directHookInvoker.InvokeBeforeExecuteAsync(module, moduleContext, executionContext.ModuleCancellationTokenSource.Token).ConfigureAwait(false);
	// Track that the before hook executed (for OnAfterExecuteAsync in finally)
	beforeHooksExecuted = true;

combined with ModuleRetryPolicyFactory.Create's WaitAndRetryAsync

Once an attempt is abandoned (abandonedAttemptTimeout set), every subsequent "retry" is a guaranteed no-op — the module is never invoked again, only the stored exception is rethrown. But that rethrow still happens after Polly's WaitAndRetryAsync sleep for that retry attempt, because the sleep-duration provider (CalculateDelay, exponential with jitter) has no visibility into the abandoned state and computes a normal growing delay regardless. So for a module configured with, say, .WithRetry(5, TimeSpan.FromSeconds(10)) (a reasonable choice for a module wrapping a flaky external process), a single non-cooperative timeout now costs roughly the full exponential backoff schedule (10s, 20s, 40s, 80s, 160s ≈ 5 minutes) sitting idle before the pipeline finally reports failure — occupying a pipeline slot the whole time — even though the very first abandoned attempt already made the outcome certain. With the built-in DefaultBaseDelay (100ms) this is negligible, but it scales directly with whatever baseDelay/count the module author configures, and this is a build/CI pipeline tool where modules commonly wrap long-running external processes that are exactly the kind of thing that might not honor cancellation cleanly.

This is a legitimate tradeoff, not a bug: the alternative (skip the delay too) would need the engine's default ModuleRetryPolicyFactory-created policy to be delay-aware of abandonedAttemptTimeout, which isn't achievable at all for a fully custom AdvancedRetryPolicyFactory policy — the engine can't reach into an arbitrary user-supplied IAsyncPolicy's internal delay logic. So there may be no clean fix that preserves both "policy sees real failures" and "no wasted backoff" for the general case.

Given that, I'd suggest as a fast-follow rather than a blocker: for the engine-owned default policy path (ModuleRetryPolicyFactory.Create), thread the abandoned-state through the sleepDurationProvider so it returns TimeSpan.Zero once an attempt has been abandoned — this covers the common case (.WithTimeout() + .WithRetry(), no custom Advanced policy) without touching the custom-policy path. Separately, it's worth a one-line addition to timeouts.md/retry-policy.md noting that a non-cooperative timeout still consumes the full configured backoff schedule before failing, since the current docs ("An attempt that remains active after the grace period is never retried") could be read as "fails immediately," which isn't quite accurate now that retries still occur at the policy level even though the module isn't re-invoked.

Overall

The core engine change (ModuleExecutionPipeline.cs) is a clean, well-contained fix for the original per-attempt-timeout bug (#3790), and this round correctly resolves both issues from the last review with targeted regression tests rather than reopening earlier ones. No blocking issues found — the backoff-waste point above is worth a follow-up but shouldn't hold up this PR.

[thomhurst]

Fixed the remaining CodeFactor failure in 1bbd752 by extracting no-timeout execution and grace-period timeout classification from ExecuteWithTimeoutAndDetailsAsync; behavior is unchanged. Validation: ModuleTimeoutTests 13/13, RetryTests 10/10, targeted formatter and git diff --check pass. The three platform build failures were the unrelated shared RunReportTests.cs CS0117 covered by #3864.

[thomhurst]

@codex review

[thomhurst]

@claude review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1bbd75256e

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

Fail immediately after an abandoned timeout

When an attempt times out and ignores cancellation, abandonedAttemptTimeout is rethrown from inside the retry delegate, so policies such as the default WithRetry/Policy.Handle<Exception>() still handle it and execute every configured retry callback/backoff before the failure escapes. In a module with a short timeout but long retry delays, the original non-cancellable attempt is already abandoned, yet the pipeline can sit through minutes of retry delays without starting any real retry; surface this condition outside the retry policy or otherwise make it unhandled by the retry policy.

Useful? React with 👍 / 👎.

[thomhurst]

Fixed in e488ac2. An attempt that remains active exits through the retry policy as a successful sentinel result, then its saved ModuleTimeoutException is thrown immediately outside the policy. The regression now requires one execution and zero retry callbacks; RetryTests pass 10/10, ModuleTimeoutTests 13/13, and the core build is clean.

[claude]

Overview

This PR fixes #3790: previously WithTimeout wrapped the entire Polly retry chain (retryPolicy.ExecuteAsync was itself wrapped in TimeoutHelper), so a 5-minute timeout with 3 retries gave 5 minutes total instead of 5 minutes per attempt, and a timeout firing during backoff sleep was misreported as "module ignored cancellation." The fix moves timeout enforcement inside the per-attempt delegate (ExecuteModuleAttempt) so each retry gets a fresh timeout budget that excludes backoff delay, and it adds an abandonedAttemptTimeout latch so a still-running (non-cooperative) attempt is never re-entered concurrently by a subsequent retry. Docs (timeouts.md, retry-policy.md) and XML doc comments are updated to describe the new per-attempt semantics. TimeoutHelper.ExecuteWithTimeoutAndDetailsAsync is also refactored into smaller private methods, and along the way fixes a real bug: if the module's own delegate happens to throw a TimeoutException right as the grace-period wait would also throw TimeoutException, the old code always treated that as "did not respond," even when the task had actually completed. The new code disambiguates via executionTask.IsCompleted, and it's covered by the new Timeout_Fault_During_Grace_Period_Counts_As_Response test. Good fix.

Strengths

• Correctly identifies and fixes the root cause from the linked issue rather than patching symptoms.
• DidTaskRespondDuringGracePeriodAsync's IsCompleted check is a genuine, well-tested bug fix for a subtle race (module throwing TimeoutException vs. the grace-period wait throwing TimeoutException).
• Docs are updated in the same PR, including the concrete "5 min timeout × 4 attempts + backoff" example — this is a behavior change users need to know about, and it's called out clearly.
• New tests (When_Retry_Backoff_Exceeds_Timeout_Then_All_Attempts_Run, When_Timed_Out_Attempt_Remains_Active_Then_Do_Not_Retry, When_Cancelled_During_Later_Attempt_Then_Report_PipelineTerminated) exercise the new per-attempt/abandonment/cancellation paths directly rather than just re-asserting old behavior.
• IsTimeout/the redundant else if branch in HandleException is correctly removed now that every timeout path always produces a ModuleTimeoutException.

Main concern: "bypasses retries" doesn't actually bypass the backoff wait

In ModuleExecutionPipeline.cs, once an attempt is abandoned (didn't respect cancellation within the grace period), abandonedAttemptTimeout is set and every subsequent call to ExecuteModuleAttempt immediately rethrows the cached exception instead of re-invoking the module:

if (abandonedAttemptTimeout is not null)
{
    throw abandonedAttemptTimeout;
}

But this short-circuit only skips running the module — it does not stop the Polly retry policy itself. Since GetRetryPolicy defaults to ShouldRetry: null ("retries every exception handled by the retry engine"), and ModuleTimeoutException is a normal handled exception, Polly will still treat each rethrow as a retryable failure and wait out its full backoff delay before calling ExecuteModuleAttempt again. So with a real (non-zero) backoff — the normal/documented case per the new timeouts.md example — once the first attempt is abandoned, the pipeline still sits through every remaining backoff interval before finally failing, even though we already know for certain no further retry will ever re-execute the module.

This contradicts both the code comment ("the engine bypasses retries to avoid running the same module instance concurrently") and the docs ("the engine bypasses retries") — retries in the sense of re-execution are bypassed, but retries in the sense of wall-clock cost are not. It also inflates RecordModuleRetries telemetry (moduleAttemptCount - 1) with attempts that never actually ran the module, which will skew retry-count dashboards/metrics.

This isn't caught by the new test (NonCancellableModuleWithTimeout in RetryTests.cs uses WaitAndRetryAsync(DefaultRetryCount, _ => TimeSpan.Zero, ...) — zero delay), so the wasted-time behavior is currently invisible to the test suite.

Suggested approach: once abandonment is detected, actually short-circuit the retry loop instead of letting it run its course with a cached exception. For example, cancel a small dedicated CancellationTokenSource at the same point abandonedAttemptTimeout is set, and pass its token into retryPolicy.ExecuteAsync(...) (linked with the outer cancellationToken). Polly's WaitAndRetryAsync delay is cancellation-aware, so the pending backoff Task.Delay will unwind immediately instead of running to completion. Catch the resulting OperationCanceledException around the retryPolicy.ExecuteAsync call and rethrow abandonedAttemptTimeout in its place. That makes the "bypass" real (fails fast, matching the doc's claim) while preserving the existing concurrency-safety guarantee, and it would also stop moduleAttemptCount from being inflated by phantom attempts.

Minor nits

• abandonedAttemptTimeout is a plain (non-volatile) field read/written across what may be different continuations, whereas the sibling moduleAttemptCount in the same closure uses Interlocked/Volatile. In practice this is likely safe because of the happens-before relationship async/await continuations establish, but the inconsistency reads oddly next to code that's otherwise careful about this — worth a comment explaining why it's safe without Volatile, or just using Volatile.Read/Write for consistency.
• Rethrowing the same cached ModuleTimeoutException instance (throw abandonedAttemptTimeout;) on every subsequent bypassed attempt resets its stack trace to the throw site inside ExecuteModuleAttempt each time, discarding the trace from the original timeout. Not a functional bug (the exception surfaces from HandleException fine either way), but slightly degrades debuggability if someone is staring at a stack trace trying to find where the original timeout occurred.

Test coverage

Good — the new/rewritten tests target the exact scenarios this PR changes (per-attempt timeout with backoff, abandonment-prevents-concurrency, and cancellation mid-retry). As noted above, I'd add one more case with non-zero backoff between bypassed retries to lock in (or catch the absence of) the fast-fail behavior suggested above.

Risk assessment

Low risk of regression to existing single-attempt (no-retry) timeout behavior — that path is essentially unchanged. The behavior change for timeout+retry combinations is real and intentional (that's the point of the PR) and is documented. The backoff-waste issue above is a correctness/perf gap in the new abandonment-handling path specifically, not a regression from main.

[thomhurst]

Refreshed onto current main at c9be84e. Validation: RetryTests 10/10, ModuleTimeoutTests 13/13, core Release build 0 warnings / 0 errors. Please re-review the current head.

[claude]

Code review (re-review at c9be84e18a, current head)

Scope: Since the last review round (approved at 99cc37c733), the only functional change is 1bbd75256e ("fix: reduce timeout helper complexity"), a pure refactor of TimeoutHelper.ExecuteWithTimeoutAndDetailsAsync extracting ExecuteWithoutTimeoutAsync, CreateTimeoutResultAsync, and DidTaskRespondDuringGracePeriodAsync. c9be84e18a on top is just a merge from main with no further code changes. ModuleExecutionPipeline.cs (the core per-attempt-timeout/retry logic reviewed previously) is byte-identical to the version already approved.

Verified: the complexity-reduction refactor is behavior-preserving

I traced both extracted branches line-by-line against the pre-refactor version rather than trusting the commit message:

• ExecuteWithoutTimeoutAsync — identical control flow to the old inline fast path (direct await when the token can't be cancelled, Task.WhenAny race against a TaskCompletionSource otherwise). Only local variable names changed.
• CreateTimeoutResultAsync / DidTaskRespondDuringGracePeriodAsync — the external-cancellation short-circuit, the grace-period WaitAsync call, and all four catch clauses (TimeoutException checking executionTask.IsCompleted, OperationCanceledException, generic Exception, success) are preserved exactly, including the TaskObservation.ObserveFault call on the still-unresponsive path. Splitting the grace-period check into its own Task-typed helper (rather than Task<T>) is safe since only WaitAsync/IsCompleted are used, not the result value.

This is a reasonable way to address a cyclomatic-complexity lint (CodeFactor, per the commit message) without touching semantics, and the existing Timeout_Fault_During_Grace_Period_Counts_As_Response test (added in a prior round) still exercises the extracted grace-period-fault path end-to-end through the public API, so the refactor has regression coverage rather than relying on manual tracing alone.

Prior review findings — still correctly resolved

The two correctness issues raised in the previous round (non-cooperative timeouts previously reported as a fake "success" to the wrapped Polly policy, and the grace-period TimeoutException misclassification) remain fixed in ModuleExecutionPipeline.cs and TimeoutHelper.cs respectively — no regression from this round's refactor.

Still open (non-blocking, previously flagged)

The backoff-waste observation from the last review — a non-cooperative timeout still burns through the full exponential backoff schedule after the outcome is already certain, since ModuleRetryPolicyFactory's sleepDurationProvider has no visibility into abandonedAttemptTimeout — is unaddressed. That's consistent with the author's earlier response that it's an intentional fast-follow rather than a blocker, and I agree it's reasonable to ship without it.

Overall

No new issues found. The only change since the last approved round is a verified-equivalent refactor with existing test coverage. Good to merge from my perspective.

[thomhurst]

Refreshed again onto current main after Ubuntu's core test host ran 17m7s and exited without a failing-test summary. Validation on 8699bc6: RetryTests 10/10, ModuleTimeoutTests 13/13, and ModularPipelines.slnx Release build 0 warnings/errors.

[thomhurst]

@claude review

[thomhurst]

@codex review

[claude]

Code review (re-review at e488ac2969, current head)

Scope: Since the last full review (approved at c9be84e18a, "No new issues found"), two commits landed: 8699bc653 (a no-op merge from main, confirmed by the author's comment and matching ModuleExecutionPipeline.cs being otherwise unchanged) and e488ac2969 ("fix: bypass retries after abandoned timeout"), which is new logic not yet reviewed. I focused on that commit.

This commit reintroduces a previously-identified-and-fixed bug

e488ac2969 was written to address the last round's non-blocking observation that an abandoned (non-cooperative) timeout still burns through the full exponential backoff schedule even though the outcome is already certain. To fix that, it changes the abandoned-timeout path to return default! instead of throwing, so the wrapped Polly retry policy sees the attempt as successful and exits immediately instead of sleeping and retrying:

ModularPipelines/src/ModularPipelines/Engine/ModuleExecutionPipeline.cs

Lines 508 to 522 in e488ac2

	var timeoutException = new ModuleTimeoutException(
	executionContext.ModuleType,
	timeout,
	timeoutResult.ElapsedTime,
	timeoutResult.WasCancellationTokenRespected);
	if (!timeoutResult.WasCancellationTokenRespected)
	{
	abandonedAttemptTimeout = timeoutException;
	// Exit the retry policy successfully, then surface the timeout below.
	return default!;
	}
	throw timeoutException;
	}

if (!timeoutResult.WasCancellationTokenRespected)
{
    abandonedAttemptTimeout = timeoutException;
    // Exit the retry policy successfully, then surface the timeout below.
    return default!;
}

The real exception is only re-thrown after retryPolicy.ExecuteAsync returns, outside the policy:

ModularPipelines/src/ModularPipelines/Engine/ModuleExecutionPipeline.cs

Lines 539 to 543 in e488ac2

	}
	if (abandonedAttemptTimeout is not null)
	{
	throw abandonedAttemptTimeout;

This is exactly the "non-cooperative timeouts reported as a fake success to the wrapped Polly policy" issue that an earlier review round on this same PR flagged and that was subsequently fixed (by making ExecuteModuleAttempt throw on the abandoned path so any wrapped policy — circuit breaker, fallback, onRetry/onBreak telemetry — sees a genuine failure). That fix is now undone: since the delegate returns normally, a circuit breaker's failure count won't increment for a module that hangs and ignores cancellation, FallbackAsync won't fire, and onRetry/onBreak callbacks are silently skipped.

The test change in this same commit is direct self-evidence of the regression — When_Timed_Out_Attempt_Remains_Active_Then_Do_Not_Retry previously asserted RetryCallbackCount == DefaultRetryCount (proving the wrapped policy's onRetry still fired on the genuine failure); it's now changed to assert RetryCallbackCount == 0:

ModularPipelines/test/ModularPipelines.UnitTests/Execution/RetryTests.cs

Lines 405 to 412 in e488ac2

	{
	var moduleFailedException = await Assert.ThrowsAsync<ModuleFailedException>(() => host.RunAsync());
	var timeoutException = moduleFailedException?.InnerException as ModuleTimeoutException;
	using (Assert.Multiple())
	{
	await Assert.That(module.ExecutionCount).IsEqualTo(ExpectedSingleExecutionCount);
	await Assert.That(module.RetryCallbackCount).IsEqualTo(0);

i.e. the test was updated to lock in the exact behavior the earlier review round required removing.

Suggested approach: keep throwing abandonedAttemptTimeout from ExecuteModuleAttempt (so wrapped policies still observe a genuine failure once), but stop retrying on it without needing a return default! success path. Since abandonedAttemptTimeout is already a captured local, the retry predicate can consult it directly — e.g. in ModuleRetryPolicyFactory, build the policy with a Handle predicate like .Handle<Exception>(_ => abandonedAttemptTimeout is null) (or equivalent for the advanced-policy path), so Polly still sees the exception as a failure but declines to retry once the attempt has been abandoned, skipping the backoff wait instead of sleeping through it. That preserves both properties the two review rounds independently required: genuine failure signal to arbitrary wrapped policies, and no wasted backoff after the outcome is certain.

Everything else in this PR (per-attempt timeout scoping, IsTimeout heuristic removal, grace-period TimeoutException/IsCompleted disambiguation, docs) was already independently verified correct across the prior review rounds and is unchanged by e488ac2969.

CLAUDE.md compliance: No violations — the changed files are core framework code within ModularPipelines.slnx scope; no generated options classes, build scripts, or .slnx/CI files touched.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Hooray!

Reviewed commit: e488ac2969

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