Late-start queued AlwaysRun cleanup modules

[thomhurst]

Summary

• late-start AlwaysRun modules left queued when fail-fast cancellation stops scheduler workers
• include both pending and queued states in deferred retry bookkeeping
• cover direct late-start, constraint retry, and the stopped-channel failure path

Test plan

• AlwaysRunHandlerTests (8/8)
• ModuleExecutorLoggingTests (11/11)
• core Release build (0 errors)
• scoped whitespace and changed-file analyzer verification
• full core unit project: 1292 passed, 4 skipped; one existing timing-sensitive cumulative-timeout test failed under full parallel load and passed on focused rerun (8/8)

Closes #3457

[claude]

Code Review: #3564 — Late-start queued AlwaysRun cleanup modules

Summary

This closes a real bug (#3457): AlwaysRunHandler previously only late-started AlwaysRun modules that were Pending. When a fail-fast (StopOnFirstException) cancellation fires, ModuleExecutor.ExecuteWorkerPoolAsync's Parallel.ForEachAsync(scheduler.ReadyModules.ReadAllAsync(token), ...) stops consuming the ready channel as soon as the token is cancelled — even if an AlwaysRun module was already dequeued out of FindReadyModules() and written to the channel (state Queued). Combined with ModuleStateTracker.CancelPendingModules deliberately excluding ModuleRunType.AlwaysRun from cancellation (GetCancellablePendingModules), a Queued AlwaysRun module was left stranded forever: WaitForSingleAlwaysRunModuleAsync's old else branch just logged "Skipping" and returned, silently never running the cleanup module and leaving its CompletionSource unresolved.

The fix extracts CanLateStartAlwaysRunModule (Pending or Queued) and uses it consistently at both call sites (WaitForAlwaysRunModulesAsync's deferred-retry check and WaitForSingleAlwaysRunModuleAsync's late-start check). Since ModuleExecutionState only has 4 values (Pending, Queued, Executing, Completed), this + the existing ShouldWaitForAlwaysRunModule (Executing/Completed) now exhaustively cover the state space — no state is silently skipped anymore.

Verification performed

• Traced the call path: AlwaysRunHandler.WaitForAlwaysRunModulesAsync is only invoked from ModuleExecutor.ExecuteAsync's outer catch, which only runs after both ExecuteWorkerPoolAsync's Parallel.ForEachAsync and the scheduler task have fully drained/completed. So the late-start path can't race with a live scheduler worker concurrently pulling the same ModuleState off the channel and calling MarkModuleStarted a second time — I looked for a double-execution hazard here since ModuleConstraintEvaluator.CanStartExecution doesn't itself check moduleState.State (nothing stops two concurrent MarkModuleStarted calls from both succeeding), but the call ordering rules it out in practice.
• Ran the two touched test classes locally (AlwaysRunHandlerTests: 8/8, ModuleExecutorLoggingTests: 11/11) via dotnet run --project test/ModularPipelines.UnitTests -- --treenode-filter ..., matching the PR's stated test plan.
• dotnet format --verify-no-changes on the three changed files: no diffs (only pre-existing CA1873 info-level noise consistent with the rest of the file).
• Confirmed ModuleStateCounters.Transition and MarkModuleStarted/MarkModuleCompleted are state-origin agnostic, so late-starting from Queued instead of Pending doesn't desync the scheduler's internal counters/_queuedModules bookkeeping.

Design notes

• Good instinct extracting CanLateStartAlwaysRunModule as a named predicate rather than repeating the is Pending or Queued check — it makes the exhaustiveness of the two-state-group split (late-startable vs. wait-worthy) self-documenting, and the new debug log messages ("Starting unexecuted AlwaysRun module... (State={State})") make it clear at runtime which of the two states triggered the late start, which will help debug any future scheduler edge cases in this area.
• Test coverage is well targeted: _ExecutesQueuedModule covers the direct case, _RetriesDeferredQueuedModule covers a Queued module blocked by a constraint (mirrors the existing Pending retry test), and StopOnFirstException_LateStartsQueuedAlwaysRunModule reproduces the actual end-to-end scenario from the issue (a real FaultingModule triggering StopOnFirstException while an AlwaysRun module sits Queued) rather than only unit-testing the handler in isolation.

No changes requested — this is a targeted, well-verified fix with good regression coverage.

[thomhurst]

Rebased onto current main after #3559 merged. PR patch remained byte-identical (28baa552022fdc84ea8877d44e0f7031a7fcb206).

[claude]

Code review

No new changes since the previous review — this PR was rebased onto current main and the patch is byte-identical (confirmed by the author). The prior review still stands: no changes requested, this is a targeted, well-verified fix with good regression coverage.

[thomhurst]

Rebased onto current main at 8c247942fb. Patch equivalence preserved (f90a444e4d13e47e3abcf5d637c89d0c9c8da943 before and after). New head: 40c2fda97f.

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. 🚀

Reviewed commit: 40c2fda97f

ℹ️ About Codex in GitHub

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

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

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

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

[claude]

Code review

Re-reviewed after the latest rebase (now at 40c2fda97f onto 8c247942fb). Independently confirmed the diff is unchanged from the previous two reviews:

• AlwaysRunHandler.CanLateStartAlwaysRunModule correctly treats Pending and Queued as late-startable, closing the gap where a Queued AlwaysRun module dequeued into the ready channel just before fail-fast cancellation stops the worker pool would otherwise be silently skipped forever (the old else branch just logged and returned, leaving CompletionSource unresolved).
• ModuleExecutionState only has 4 values (Pending, Queued, Executing, Completed); combined with ShouldWaitForAlwaysRunModule (Executing/Completed), the two predicates are now exhaustive over the enum — verified directly against the enum definition in ModuleState.cs.
• New tests (_ExecutesQueuedModule, _RetriesDeferredQueuedModule, StopOnFirstException_LateStartsQueuedAlwaysRunModule) cover the direct late-start case, the deferred-retry-while-blocked case, and the actual end-to-end cancellation scenario from Engine: AlwaysRun cleanup modules stuck in Queued state are silently skipped during failure teardown #3457.

No changes requested — this remains a targeted, well-verified fix.

[thomhurst]

Rebased cleanly onto current main; no code changes beyond the rebase. @codex review

[claude]

Code review

Re-checked after the latest rebase (now at 4c0f162f, previously 40c2fda97f/8c247942fb). Diffed the current PR content against the prior three reviews — no code changes beyond the rebase, confirming the author's note.

The fix stands as previously reviewed: AlwaysRunHandler.CanLateStartAlwaysRunModule correctly treats both Pending and Queued as late-startable, closing the gap where a Queued AlwaysRun module dequeued into the ready channel just before fail-fast cancellation stops the worker pool would otherwise be silently skipped forever. Combined with the existing ShouldWaitForAlwaysRunModule (Executing/Completed), the two predicates are exhaustive over ModuleExecutionState's four values — no state falls through. Test coverage (_ExecutesQueuedModule, _RetriesDeferredQueuedModule, StopOnFirstException_LateStartsQueuedAlwaysRunModule) remains well targeted, covering the direct case, the deferred-retry-while-blocked case, and the actual end-to-end cancellation scenario from #3457.

No changes requested — this remains a targeted, well-verified fix.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. You're on a roll.

Reviewed commit: 4c0f162f37

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

Rebased onto current main after #3567 merged. Patch equivalence preserved (f90a444e4d13e47e3abcf5d637c89d0c9c8da943 before and after). New head: c8117a1dc4.

@codex review

[chatgpt-codex-connector]

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

Reviewed commit: c8117a1dc4

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