Cancel parallel limiter waits

[thomhurst]

Closes #3792

Summary

• pass engine cancellation into custom and execution-type semaphore waits
• acquire slots before MarkModuleStarted so queued wait time is not execution time
• keep dependency readiness work outside scarce limiter slots

Validation

• ParallelLimitHandlerTests: 3/3 passed
• lightweight core Release build: 0 warnings, 0 errors

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6f4f90a772

ℹ️ About Codex in GitHub

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

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

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

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

[chatgpt-codex-connector]

Preserve AlwaysRun modules while waiting on limiters

When StopOnFirstException cancels the worker pool, this cancellationToken is canceled for every dequeued module. An AlwaysRun module that has been dequeued but is still waiting for a custom or execution-type slot will now throw here before ModuleExecutionPipeline.SetupCancellation can apply the AlwaysRun exemption; ModuleRunner then marks it terminated, so teardown modules behind a limiter no longer run after another module fails. Use an uncanceled/module-specific token for AlwaysRun limiter waits, or move the AlwaysRun cancellation setup before these waits.

Useful? React with 👍 / 👎.

[thomhurst]

Fixed at 0e8ea91. AlwaysRun modules now use their module-owned uncancelled token for both limiter waits, matching ModuleExecutionPipeline's cancellation exemption; ordinary modules still use engine cancellation. Added a regression test covering both limiter tokens with an already-cancelled engine token (ParallelLimitHandlerTests 4/4). Core Release build passes with 0 warnings/errors.

[claude]

Code review

Summary: this PR fixes #3792 by threading the engine's CancellationToken into AcquireParallelLimitAsync/AcquireExecutionTypeLimitAsync (so a pipeline cancellation actually interrupts a module stuck waiting on a [ParallelLimiter]/execution-type semaphore instead of hanging forever), and moves the semaphore acquisition in ModuleRunner.ExecuteCore to happen before scheduler.MarkModuleStarted(...) so that time spent queued for a scarce slot is no longer counted as module execution time.

Both changes look correct and are the right fix for the underlying issue:

• ParallelLimitHandler.cs now passes the token straight into the semaphore WaitAsync, which is the standard SemaphoreSlim-style cancellation contract — a canceled wait won't consume a permit.
• ModuleRunner.MarkModuleCompleted derives its state transition from the module's actual current state (Queued/Pending, not an assumed Executing), so a cancellation that throws before MarkModuleStarted is ever reached still completes/fails the module cleanly without corrupting the scheduler's state counters.
• The new ParallelLimitHandlerTests directly cover the two things that matter here: cancellation actually unblocks a queued waiter, and the limiter slot is acquired before MarkModuleStarted (verified via a scheduler mock).

One architectural point worth considering as a follow-up (not a blocker):

ModuleRunner.cs#L153-L166 now acquires the ParallelLimiter/execution-type semaphore before re-checking MarkModuleStarted. MarkModuleStarted also enforces scheduling constraints like [NotInParallel] via CanStartExecution, and returns the module to Pending (to be redispatched later) when a constraint blocks it. With the new ordering, a module that's blocked purely by such a constraint will still wait for/acquire a scarce limiter slot first, then immediately release it once the constraint check fails — on every redispatch attempt. Under contention (e.g. a [NotInParallel] module that also shares a [ParallelLimiter]/CPU-intensive execution-type pool with other modules), this means a module that can't run yet competes for the same limited slots as modules that are actually ready to execute, which can transiently starve the ready ones.

This is a real side effect of fixing the execution-time-accounting bug, not a new correctness bug — the module never leaks the slot (it's released via using) and eventually gets rescheduled correctly. But it could be avoided by splitting the constraint check into a cheap, side-effect-free "peek" (using the existing CanStartExecution/executing-snapshot check) that runs before acquiring the semaphore, while keeping today's MarkModuleStarted as the authoritative re-check-and-transition immediately after acquiring the slot (to close the race where the executing set changes while waiting). That preserves the accurate execution-time reporting this PR is going for, without letting constraint-blocked modules contend for slots they can't actually use yet.

Validation noted in the PR description (targeted unit tests + core build) matches what's appropriate for this change per this repo's agent build guidance.

[thomhurst]

@codex review

[thomhurst]

@claude review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Keep it up!

Reviewed commit: 0e8ea91ce1

ℹ️ 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 new 0e8ea91 commit ("fix: preserve AlwaysRun limiter waits"), which was pushed after my previous review of 6f4f90a.

What changed: ModuleRunner.ExecuteCore now picks a different cancellation token for the limiter/execution-type semaphore wait depending on module.Configuration.AlwaysRun — non-AlwaysRun modules still use the pipeline's cancellationToken (so the previous fix for #3792 still works), while AlwaysRun modules use executionContext.ModuleCancellationTokenSource.Token so a pipeline-wide cancellation (including Ctrl+C / process exit, per EngineCancellationToken) can't strand an AlwaysRun cleanup module mid-wait. This closes the regression the first commit introduced: before this change, an AlwaysRun module queued on a [ParallelLimiter]/execution-type semaphore would have been cancelled by engine cancellation, contradicting the documented guarantee in ModuleExecutionPipeline.SetupCancellation ("AlwaysRun modules don't get cancelled when the engine cancels") and the same exemption already applied elsewhere (e.g. ModuleStateTracker's pending-module cancellation, ModuleResultRegistrar). The new ModuleRunner_PreservesAlwaysRunDuringCancelledLimiterWaits test correctly exercises this.

One architectural point worth considering (not a blocker):

ModuleRunner.cs#L153-L161 re-derives the "should this module observe engine cancellation?" decision from module.Configuration.AlwaysRun directly, duplicating the logic that already lives in ModuleExecutionPipeline.SetupCancellation (ModuleRunner.cs#L153-L161). This works today only because of an implicit ordering: SetupCancellation runs later, inside ExecuteModuleWithPipeline → _executionPipeline.ExecuteAsync, after the limiter wait — so for AlwaysRun modules the code is really just reading the fresh, still-unlinked CancellationTokenSource created in ModuleExecutionContext's constructor, not anything SetupCancellation actually produced. Nothing in the types enforces that ordering, so if a future change ever ran SetupCancellation earlier (e.g. to support a "hard-kill after grace period" timeout even for AlwaysRun modules, which is a plausible future ask for CI systems), ModuleRunner would keep using the stale unlinked token and silently diverge from whatever cancellation policy SetupCancellation is meant to be the single source of truth for.

A more maintainable shape: have ModuleRunner call the same cancellation-setup step (or a shared helper it delegates to) before acquiring the limiter/execution-type semaphores, then just use executionContext.ModuleCancellationTokenSource.Token unconditionally for the limiter wait. That keeps "what cancellation applies to an AlwaysRun module" defined in exactly one place — the class whose doc comment already claims ownership of "AlwaysRun behavior" — instead of two classes needing to independently agree on it via call-order timing.

Separately, the earlier architectural note from my previous review (constraint-blocked modules, e.g. [NotInParallel], now contend for scarce limiter/execution-type slots before MarkModuleStarted's constraint check runs, since it happens after semaphore acquisition) is unchanged by this commit and remains an optional follow-up rather than something this PR needs to fix.

Validation: ParallelLimitHandlerTests now has 4 tests (the PR description's "3/3 passed" predates this commit's added ModuleRunner_PreservesAlwaysRunDuringCancelledLimiterWaits test) — worth a quick re-run to confirm all 4 still pass, but this is a documentation nit, not a code issue.

[thomhurst]

Refreshed onto current main in 047373c after required Ubuntu CI hit the known core-test host failure (no failed-test summary; exit 2/134 after 13m04s). Local validation: ParallelLimitHandlerTests 4/4, ModuleTimeoutTests 12/12; core Release build 0 warnings/errors.

[claude]

Code review

Re-reviewed at 047373cd2 (a no-op merge of main into this branch — git diff 0e8ea91ce1..047373cd2 -- src/ModularPipelines/Engine/Execution/{ModuleRunner,ParallelLimitHandler,IParallelLimitHandler}.cs is empty, so nothing in this PR's actual fix changed since my last review).

Previous feedback check: Codex's P1 ("Preserve AlwaysRun modules while waiting on limiters") was fixed in 0e8ea91ce1 — AlwaysRun modules now use executionContext.ModuleCancellationTokenSource.Token (a fresh, not-yet-linked token) for the limiter wait instead of the engine's cancellationToken, so a StopOnFirstException cancellation can no longer strand a teardown module behind a semaphore. This is covered by ModuleRunner_PreservesAlwaysRunDuringCancelledLimiterWaits, and I traced it end-to-end: ModuleExecutionContext's ModuleCancellationTokenSource is a brand-new CancellationTokenSource at the point ExecuteCore reads it (it's only linked to anything later, inside ExecuteModuleWithPipeline → SetupCancellation), so for AlwaysRun modules this is effectively CancellationToken.None during the limiter wait — matching the documented "AlwaysRun doesn't get cancelled by the engine" guarantee.

I also independently re-derived the core fix and it's sound:

• ParallelLimitHandler.AcquireParallelLimitAsync/AcquireExecutionTypeLimitAsync now forward the token straight into the underlying AsyncSemaphore.WaitAsync, which won't consume a permit on cancellation — no leaked slots.
• Moving the semaphore acquisition in ModuleRunner.ExecuteCore to before scheduler.MarkModuleStarted means a module can now throw (dependency-wait failure, or a cancelled limiter wait) while still in Queued/Pending state rather than Executing. I checked ModuleStateTracker.MarkModuleCompleted/ModuleStateCounters.Transition and this is handled correctly — Transition decrements whatever the actual source bucket is, _executingModules.Remove is a no-op if the module was never added, and ExecutionStartTime is already nullable and null-checked downstream (ModuleStateTracker.cs:232, PipelineSetupExecutor.cs:89). So no corrupted counters or crashes, and it's arguably more accurate than before (a module that never truly started no longer gets a bogus near-zero execution duration).
• The second, now-removed acquisition point (previously inside ExecuteModuleLifecycle) is fully replaced by the new single acquisition point — no double-acquire.

Both architectural follow-ups from my previous review are still open and still non-blocking, since the code hasn't changed:

1. Constraint-blocked modules (e.g. [NotInParallel]) now acquire a scarce [ParallelLimiter]/execution-type slot, immediately release it when MarkModuleStarted's constraint re-check fails, and repeat this on every redispatch — so they transiently compete with actually-ready modules for the same limited slots. A cheap side-effect-free "peek" via the existing CanStartExecution snapshot before acquiring the semaphore (keeping today's MarkModuleStarted as the authoritative post-acquire re-check) would avoid this without giving up the accurate execution-time accounting this PR is going for.
2. ModuleRunner.cs's module.Configuration.AlwaysRun check duplicates the cancellation-exemption decision that ModuleExecutionPipeline.SetupCancellation already owns; it only works today because SetupCancellation happens to run later. Nothing in the types enforces that ordering, so a future change to SetupCancellation (e.g. a grace-period timeout that still applies to AlwaysRun modules) could silently diverge from what ModuleRunner uses for the limiter wait. Worth consolidating into one place long-term.

Neither blocks this PR — they're pre-existing shapes this fix inherits rather than regressions it introduces, and the module never leaks a slot (using) or gets stuck incorrectly.

Test coverage: the new ParallelLimitHandlerTests (4/4) cover the important cases directly: cancellation unblocks a queued waiter for both limiter types, MarkModuleStarted isn't called until after the slot is acquired, and AlwaysRun modules ignore an already-cancelled engine token. One (optional) gap: there's no test exercising a real ModuleStateTracker/ModuleScheduler (rather than mocks) through a dependency-wait failure or limiter-cancellation prior to MarkModuleStarted, to lock in that the state-counter transition from Queued→Completed (skipping Executing) stays correct as the scheduler evolves.

Validation reported (local ParallelLimitHandlerTests 4/4, ModuleTimeoutTests 12/12, core Release build 0 warnings/errors) is appropriate for this change per the repo's agent build guidance. No new issues found — approving.