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

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

Ubuntu's sole failure is unrelated SpectreResultsPrinterTests.LocalOutput_DoesNotAddModuleResultsGroup, contaminated by concurrent process-global AnsiConsole.Console use. The isolation fix is in PR #3929; this timeout PR does not touch Spectre output. Triggering this exact head's single allowed failed-job rerun for run 31384731910; no code change.

[thomhurst]

Fixed Claude's current-head command-derived-token timeout gap in fd7bd85. Command now normalizes its framework-owned linked cancellation back to the caller token before rethrowing, so TimeoutHelper attributes it to the module deadline while continuing to propagate genuinely unrelated cancellable tokens. Deterministic command-process regression red then green; ModuleTimeoutTests 16/16; focused command regression 1/1; guarded core Release build 0 warnings/errors. All review threads resolved.\n\n@codex review exact head fd7bd85\n\n@claude review exact head fd7bd85 and submit approval if clean.

[chatgpt-codex-connector]

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

Reviewed commit: fd7bd85a80

ℹ️ 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-review at fd7bd85a8088, current head)

Reviewed the latest three commits (fd7bd85 preserve caller cancellation, 66c9806 classify tokenless cancellation, e030dd9 attribute timeout cancellation by token) against the retry/timeout re-entrancy concerns raised in earlier rounds. The caller-cancellation-preservation fix in Command.cs correctly closes the token-identity gap that a previous review flagged in TimeoutHelper.cs (comparing exception.CancellationToken == timeoutCts.Token now works because Command rewraps to the caller's token before rethrowing). A few things remain worth addressing:

🟡 Command.cs:467-479 — rewrap loses the concrete exception type

if (e is OperationCanceledException cancellationException
    && cancellationException.CancellationToken != callerCancellationToken)
{
    throw new OperationCanceledException(
        cancellationException.Message,
        cancellationException,
        callerCancellationToken);
}

throw;

This constructs a brand-new base OperationCanceledException rather than rethrowing the original. The underlying cancellation from CliWrap/Task machinery commonly surfaces as TaskCanceledException, tied to the internal linked executionCancellationToken (which will generally differ from callerCancellationToken since it's created via a linked CTS) — so this branch is the common path, not the exceptional one. Any module/tool code doing catch (TaskCanceledException) around ExecuteCommandLineToolAsync will stop matching once this rewrap happens, where previously throw; alone preserved the exact runtime type. Consider preserving the original type — e.g. only rewrap when a same-type constructor is available, or use ExceptionDispatchInfo to capture/rethrow while patching just the CancellationToken property expectations callers rely on — rather than always downgrading to the base type.

🟡 ModuleExecutionPipeline.cs:478 — unconditional CTS allocation on the no-retry path (raised previously, still open)

using var retryCancellationTokenSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);
...
result = retryPolicy != null
    ? await retryPolicy.ExecuteAsync(ExecuteModuleAttempt, retryCancellationTokenSource.Token).ConfigureAwait(false)
    : await ExecuteModuleAttempt(cancellationToken).ConfigureAwait(false);

This was flagged in the 11:55 review as a minor hot-path cost and doesn't look addressed yet. retryCancellationTokenSource is created (and disposed) for every module execution, but on the no-retry branch (retryPolicy == null, the default for any module without DefaultRetryCount/an explicit retry policy) it's passed to ExecuteModuleAttempt only as a capture inside ExecuteModuleAttemptAsync for the abandoned-timeout Cancel() call — the attempt itself runs on the raw cancellationToken, not retryCancellationTokenSource.Token. That's harmless correctness-wise (the Cancel() call in ExecuteModuleAttemptAsync still fires and is observed by the catch at line 504), but it means every non-retrying module pays a linked-CTS allocation/registration/disposal for a mechanism it can't otherwise benefit from. Worth gating behind retryPolicy is not null if that's easy without restructuring the surrounding try/finally.

🟢 ModuleExecutionPipeline.cs:507 / :518 — non-bare rethrow of the stored timeout exception resets its stack trace

catch (OperationCanceledException) when (policyExecutionState.AbandonedAttemptTimeout is not null && ...)
{
    throw policyExecutionState.AbandonedAttemptTimeout;   // line 507
}
...
if (policyExecutionState.AbandonedAttemptTimeout is { } abandonedAttemptTimeout)
{
    throw abandonedAttemptTimeout;                          // line 518
}

Both are throw <expr>;, not bare throw;, so the stack trace is reset to this rethrow site rather than preserving where ModuleTimeoutException was originally thrown inside ExecuteModuleAttemptAsync. For a module-timeout diagnostic path specifically built to give better visibility into what happened, losing the original throw location in the trace is a bit self-defeating. ExceptionDispatchInfo.Capture(...).Throw() at the point AbandonedAttemptTimeout is assigned (or storing an ExceptionDispatchInfo instead of the raw exception) would preserve it.

🔵 Design note: the abandoned-attempt guard depends on the retry policy cooperating with cancellation

retryCancellationTokenSource.Cancel() (line 576) is the only signal preventing a wrapped Polly IAsyncPolicy from re-invoking ExecuteModuleAttempt after an attempt is abandoned mid-timeout. That's fine for the built-in retry policies, but ModuleConfigurationBuilder.Advanced.WithRetryPolicy(...) is a public, documented extension point — a custom policy that retries without checking the passed token before invoking the delegate again would silently defeat this safety mechanism and run the same module instance concurrently with its still-live abandoned attempt, which is exactly the bug this feature exists to prevent. Not something to fix in this PR, but might be worth a doc comment on WithRetryPolicy (or an assert-once-per-attempt guard in ExecuteModuleAttemptAsync itself) so the invariant doesn't rely purely on well-behaved external policies.

---

Nothing here blocks merge — the core timeout/retry re-arming logic and the caller-cancellation fix look correct for the paths exercised by the built-in retry policies. The Command.cs exception-type rewrap is the one I'd most want a second look at before merge, since it changes observable exception types for existing callers.

[thomhurst]

Updated exact head 35f8b4b5e0d93b95c7fda7d13aa06881e41733ee:

• extracted caller-cancellation handling from Command.Of, removing the CodeFactor complexity regression
• preserves TaskCanceledException while rekeying cancellation to the caller token
• avoids linked retry CTS allocation when no retry policy exists
• blocks module re-entry after an abandoned attempt even for a custom policy that ignores cancellation
• preserves the original timeout exception stack when surfacing abandoned attempts

Validation:

• RetryTests: 10/10 passed
• CommandLoggerTests: 52/52 passed
• ModularPipelines.slnx Release build: 0 warnings, 0 errors
• touched-file whitespace verification: clean
• solution-wide format verification only reported pre-existing unrelated whitespace/info diagnostics

@codex review exact head 35f8b4b5e0d93b95c7fda7d13aa06881e41733ee
@claude review exact head 35f8b4b5e0d93b95c7fda7d13aa06881e41733ee

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 35f8b4b5e0

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

[thomhurst]

Addressed linked attempt-token cancellation in 4a93600. Validation: ModuleTimeoutTests 17/17 passed; ModularPipelines.slnx Release build passed with 0 warnings and 0 errors.

[thomhurst]

@codex review commit 4a93600

[thomhurst]

@claude review commit 4a93600

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Keep them coming!

Reviewed commit: 4a93600f78

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

[thomhurst]

Fixed the Ubuntu timeout-completion race in 78e7bed. Execution completion is now classified synchronously against deadline state, so cancellation-handler faults after timeout count as cooperation while pre-deadline faults still propagate. ModuleTimeoutTests 18/18; core Release build clean.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 78e7bed55d

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

[thomhurst]

Fixed in c3a5c2f. Timeout and external-cancellation signals now sample execution-task completion directly before cancellation propagates through a separate linked attempt token; no deferred continuation determines ordering. Added a RunContinuationsAsynchronously pre-deadline regression. ModuleTimeoutTests pass 19/19; core Release build is clean.

[claude]

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

This PR has been through an unusually long review cycle already (20+ automated rounds from Claude/Codex plus author iterations). I re-traced the full current diff independently rather than trusting prior summaries.

Previously flagged issues — confirmed fixed

1. Command.cs rewrap losing exception type (flagged in the 14:55 review): ThrowCallerCancellationIfRequired (Command.cs:536-567) now preserves TaskCanceledException specifically when rewrapping to the caller's token, and uses ExceptionDispatchInfo.Capture(...).Throw() for the same-token path instead of a bare throw; (necessary now that this logic lives in an extracted method with no enclosing catch). This closes the gap where catch (TaskCanceledException) callers in module/tool code would previously stop matching.
2. Unconditional retry CancellationTokenSource allocation on the no-retry path (flagged in the 11:55/14:55 reviews): ModuleExecutionPipeline.cs:167-169 now only allocates retryCancellationTokenSource when retryPolicy is not null, avoiding the per-execution allocation for the common no-retry case.

Both are correctly addressed in the current head — nothing further to raise on either thread.

New scan of the latest commits (fd7bd85, 66c9806, 4a93600, 78e7bed, c3a5c2f)

I traced the abandoned-attempt-timeout flow end to end: ExecuteModuleAttemptAsync sets PolicyExecutionState.AbandonedAttemptTimeout and cancels retryCancellationTokenSource on a non-cooperative timeout; the guard at the top of the same method blocks re-entry on any subsequent Polly-driven call; the outer catch (OperationCanceledException) when (AbandonedAttemptTimeout is not null && !cancellationToken.IsCancellationRequested) reclassifies the resulting Polly-backoff cancellation back into the original ModuleTimeoutException via ThrowPreservingStack. This is consistent and the guard condition correctly excludes genuine pipeline cancellation (falls through to IsPipelineCancelled/PipelineTerminated instead), matching the new When_Cancelled_During_Later_Attempt_Then_Report_PipelineTerminated test. I didn't find a correctness gap in this path.

🟡 Architectural note: TimeoutHelper's race resolution leans on unspecified CancellationTokenSource.Register ordering

TimeoutHelper.cs:447-457 (and the accompanying comment) explicitly relies on CancellationTokenSource invoking registered callbacks in LIFO order to determine whether the execution task had already completed before the deadline/external-cancellation signal propagated to it:

// Register after linking the attempt token. Cancellation callbacks run in
// LIFO order, so these observers record whether execution had already
// completed before cancellation propagates to the task.

LIFO is the current CoreCLR implementation detail, not a documented contract of CancellationTokenSource.Register/CreateLinkedTokenSource — the BCL docs don't guarantee callback ordering. That makes a core correctness property of the timeout/retry engine (whether an attempt counts as "abandoned" vs "cooperative") implicitly dependent on runtime internals that could change between .NET versions without it showing up as a compile or obvious runtime error — it would silently flip WasCancellationTokenRespected classification under race conditions that are inherently hard to catch in CI.

Why this matters more than a typical implementation-detail nitpick: this is exactly the class of bug this PR spent 20+ rounds hunting down (order-dependent races in timeout classification). Baking in a new dependency on unspecified ordering re-introduces the same risk category one layer down, just moved from "application code race" to "framework callback race."

Suggested alternative: replace the "read IsCompleted inside the cancellation callback and hope it observes state established by un-fired-yet linked cancellation" pattern with an explicit, order-independent state machine — e.g. an Interlocked.CompareExchange on a single shared "outcome" field that's set exactly once from three places (task continuation, deadline callback, external-cancellation callback), where the first writer wins regardless of callback firing order. That gives the same "was it already done" semantics without depending on which of two independently-registered callbacks the CLR happens to invoke first. Given how many rounds this exact class of race has already cost, it's worth the extra explicitness so the next maintainer doesn't have to re-derive why LIFO ordering matters here.

Minor: redundant pattern in IsPipelineCancelled

private bool IsPipelineCancelled(Exception exception)
{
    return exception is TaskCanceledException or OperationCanceledException or ModuleTimeoutException
           && _engineCancellationToken.IsCancelled;
}

TaskCanceledException already derives from OperationCanceledException, so TaskCanceledException or OperationCanceledException is redundant — is OperationCanceledException alone covers both. Harmless, but it reads as if the two are handled differently, which they aren't. Not worth a fix-up commit on its own, but worth folding in if this file gets touched again.

Docs

docs/docs/how-to/retry-policy.md and timeouts.md additions accurately describe the implemented per-attempt-timeout/backoff-exclusion/no-retry-on-abandonment semantics — no discrepancies found against the code.

Overall

The core correctness concerns from earlier rounds are resolved. The one substantive item I'd want addressed before merge is the LIFO-ordering dependency in TimeoutHelper — not because I found a concrete failure case, but because this exact bug shape has repeatedly bitten this PR, and the fix as written trades an application-level race for a CLR-implementation-level one rather than eliminating the class of bug.

[thomhurst]

Addressed the latest architectural race at exact head b0d45fc. The attempt CancellationTokenSource is now independent: deadline/external observers record whether execution already completed, then explicitly propagate cancellation. Correctness no longer depends on CancellationTokenSource callback ordering or a deferred task continuation. Also removed the redundant TaskCanceledException pattern. Validation: ModuleTimeoutTests 19/19, RetryTests 10/10, ModularPipelines.slnx Release build 0 warnings/errors. @codex review exact head b0d45fc. @claude review exact head b0d45fc.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b0d45fc939

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