Complete pending module awaiters on cancellation

[thomhurst]

Closes #3458

Summary

• complete every pending or queued Module<T> typed completion source during scheduler cancellation
• register rich PipelineTerminated results without racing bare task cancellation
• handle standalone, dynamic runtime-await, AlwaysRun, dependency-failure, and distributed cancellation paths
• use generated runtime metadata when available, with a cached reflection fallback for dynamic modules

Validation

• EngineCancellationTokenTests: 9/9 passed
• ModuleExecutorLoggingTests: 11/11 passed
• GeneratedModuleMetadataTests: 9/9 passed
• distributed executor tests: 22/22 passed
• distributed test project Release build: 0 errors
• ModularPipelines.sln Release build: 0 errors (existing warnings only)
• scoped whitespace verification: passed

The latest follow-up updates all distributed executor fixtures for the new required IModuleResultRegistrar dependency, fixing the cross-platform CI compilation failure.

Collector race follow-up

• Reserve the scheduler start transition before publishing an assignment, so terminal modules are not published and constraint-deferred modules are safely requeued without duplicate side effects.
• DistributedModuleExecutorTests: 24/24.

[thomhurst]

Fixed the failing changed-file analyzer gate in fce814119c:

• SA1642: use the repository-configured Initialises constructor summary
• SA1202: place public query members before private helpers

Validation:

• scoped CI analyzer verification: clean
• ModuleExecutorLoggingTests: 11/11
• EngineCancellationTokenTests: 7/7
• GeneratedModuleMetadataTests: 8/8
• ModularPipelines.sln Release build: 0 errors (225 existing warnings)

[claude]

Review

Thanks for tackling #3458 — the fix for the specific scenario in the issue (a module cancelled by ModuleStateTracker.CancelPendingModules while still Pending) is implemented cleanly:

• ModuleCompletionSourceCanceller mirrors the existing generated-metadata-with-reflection-fallback pattern used elsewhere (ModuleExecutionDelegateFactory, CompletionSourceSetterCache), so it stays AOT/trimming-safe.
• The new EngineCancellationTokenTests.CancelledBeforeStartModule_UnblocksRuntimeAwaiter test is a good end-to-end reproduction of the original hang (dynamic context.GetModule<T>() await on a module cancelled while pending), and it's paired with focused unit tests at the ModuleStateTracker/GeneratedModuleMetadata level.

One correctness gap: the fix doesn't cover the more common trigger for this same hang

The linked issue names two call sites that need to complete a module's typed Module<T>.CompletionSource: ModuleStateTracker.CancelPendingModules (fixed here) and ModuleResultRegistrar.RegisterTerminatedResult (src/ModularPipelines/Engine/Execution/ModuleResultRegistrar.cs:26-40, not touched by this PR). The second one is still broken, and it's reachable through a much more ordinary path than the one this PR fixes:

1. Module Dep fails.
2. Module Target has [DependsOn<Dep>]. In ModuleStateTracker.MarkModuleCompleted → ProcessDependentModules, a dependency is removed from UnresolvedDependencies on any completion of the dependency, success or failure (ModuleStateTracker.cs:340-353). ModuleState.IsReadyToExecute only checks UnresolvedDependencies.Count == 0, not success (ModuleState.cs:153). So Target becomes ready and gets scheduled to Executing even though Dep failed.
3. In ModuleRunner.ExecuteCore (ModuleRunner.cs:83-138), _dependencyWaiter.WaitForDependenciesAsync(...) runs before ExecuteModuleWithPipeline (i.e., before ModuleExecutionPipeline.ExecuteAsync is ever invoked). DependencyWaiter.WaitForDependenciesAsync awaits scheduler.GetModuleCompletionTask(dependencyType), which is already faulted for Dep, so it rethrows Dep's exception immediately — for a non-AlwaysRun module this isn't swallowed (DependencyWaiter.cs:36-52).
4. That exception lands in ModuleRunner.ExecuteCore's catch (Exception ex) (ModuleRunner.cs:122-136), which calls scheduler.MarkModuleCompleted(moduleType, false, ex) (only completes the scheduler-side ModuleState.CompletionSource, not Target's own typed one) and _resultRegistrar.RegisterTerminatedResult(module, moduleType, ex).
5. RegisterTerminatedResult only calls _resultRegistry.RegisterResult(...) — it never touches Target's Module<T>.CompletionSource. Since Target never reaches ModuleExecutionPipeline.ExecuteAsync (the only other place that completes it, at ModuleExecutionPipeline.cs:116/135/221/229), its CompletionSource is now stuck forever.
6. Anything doing a dynamic await context.GetModule<Target>() (not a static [DependsOn]) hangs exactly like the original issue — just via "my dependency failed" instead of "the scheduler cancelled me while pending."

This is essentially the same failure mode the PR sets out to fix, but through the everyday "a dependency failed" path rather than the pending-cancellation path, so it seems worth closing in the same PR rather than leaving a second, more common hang. IgnoredModuleResultRegistrar.RegisterIgnoredModuleResultAsync already establishes the right pattern for this (see its comment "Set the completion source so awaiting the module returns immediately... necessary... so dependent modules don't wait forever" at IgnoredModuleResultRegistrar.cs:141-149) — ModuleResultRegistrar.RegisterTerminatedResult could do the same via GeneratedModuleMetadata.TryGetRuntime(...).SetCompletionSource(module, result) (or the new ModuleCompletionSourceCanceller/an equivalent "set failure" fallback for non-generated modules).

Suggest adding a test analogous to CancelledBeforeStartModule_UnblocksRuntimeAwaiter but where the blocking module is a [DependsOn<T>] dependent of a module that fails (rather than being cancelled while Pending), with a third module dynamically awaiting the dependent — I'd expect that to currently hang/timeout.

Minor

• Unrelated churn: the CreateCounters method relocation and the Initializes → Initialises docstring wording change in ModuleStateTracker.cs aren't related to the fix — worth dropping to keep the diff focused (not blocking).

Nice, minimal approach otherwise — the generated-metadata + reflection-fallback split is consistent with how the rest of the engine handles AOT-vs-dynamic modules.

[thomhurst]

Addressed the requested correctness gap in d7a9376212.

• ModuleResultRegistrar.RegisterTerminatedResult now completes the typed Module<T>.CompletionSource after registering the PipelineTerminated result, using generated runtime metadata when available and the existing compiled fallback otherwise.
• Added a direct regression proving the registered terminated result is also returned through IModule.ResultTask.
• Added the requested end-to-end dependency-failure scenario: a third module dynamically awaits the dependent module while its declared dependency fails.

Validation:

• Generated_Runtime_Creates_Typed_Terminated_Result: 1/1 passed
• EngineCancellationTokenTests: 8/8 passed
• changed-file analyzer verification: passed
• Release core build: 0 errors (225 existing warnings)

The CreateCounters relocation and constructor wording remain because the changed-file analyzer gate explicitly reported SA1202 and SA1642 on those changed files; reverting them would restore a failing required check.

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d7a9376212

ℹ️ About Codex in GitHub

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

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

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

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

[claude]

Review

The correctness gap from the previous round is resolved. ModuleResultRegistrar.RegisterTerminatedResult (src/ModularPipelines/Engine/Execution/ModuleResultRegistrar.cs:26-49) now completes the module's typed Module<T>.CompletionSource after registering the PipelineTerminated result — via runtime.SetCompletionSource(...) when generated metadata exists, falling back to the existing CompletionSourceSetterCache expression-compiled setter otherwise. This mirrors the exact pattern IgnoredModuleResultRegistrar.RegisterIgnoredModuleResultAsync already uses, so the engine now has one consistent mechanism (generated-metadata-first, reflection-fallback) for every place a module can terminate without reaching ModuleExecutionPipeline.ExecuteAsync: skipped, ignored-via-history, cancelled-while-pending, and now dependency-failure-terminated.

I checked this by hand (dependency fails → dependent is scheduled per ModuleState.IsReadyToExecute only checking UnresolvedDependencies.Count == 0, not success → ModuleRunner.ExecuteCore catches the propagated exception before the pipeline ever runs → RegisterTerminatedResult is the only place left to unblock a dynamic await context.GetModule<T>()) and it now does. I also pulled the branch and ran the three relevant suites locally rather than trusting the PR description:

• EngineCancellationTokenTests: 8/8 passed, including the new TerminatedBeforeExecutionModule_UnblocksRuntimeAwaiter reproducing exactly the scenario from my last review.
• ModuleExecutorLoggingTests: 11/11 passed.
• GeneratedModuleMetadataTests: 8/8 passed.

One non-blocking simplification: ModuleCompletionSourceCanceller.GetResultType reflection walk is unnecessary

ModuleCompletionSourceCanceller.Cancel(IModule module, Type moduleType) (src/ModularPipelines/Engine/ModuleCompletionSourceCanceller.cs:84-111) falls back to GetResultType(moduleType), which walks the base-type chain looking for a closed Module<> to pull the generic argument out via reflection. But IModule already exposes ResultType (Module.cs:78, Type IModule.ResultType => typeof(T); — zero-cost, no reflection), and it's already used for exactly this purpose one file over in ModuleResultRegistrar.RegisterTerminatedResult (var resultType = module.ResultType;) and in IgnoredModuleResultRegistrar. Since Cancel is called with the IModule instance in hand (ModuleStateTracker.CancelPendingModules has moduleState.Module), it can just do:

public static void Cancel(IModule module, Type moduleType)
{
    if (GeneratedModuleMetadata.TryGetRuntime(moduleType, out var runtime))
    {
        runtime.CancelCompletionSource(module);
        return;
    }

    Cache.GetOrAdd(module.ResultType, CreateCanceller)(module);
}

This deletes the ~15-line GetResultType helper (and its per-call reflection loop over the base-type chain) entirely, and keeps this new class consistent with the resolution strategy every other completion-source helper in the engine already uses. Not a bug — just avoids reinventing a lookup the interface already provides for free.

Minor test-coverage note

The reflection-fallback branch of ModuleCompletionSourceCanceller.CreateCanceller (the "no generated runtime metadata" path) doesn't appear to be directly exercised by a test — the covered scenarios (LaterModule, GeneratedMetadataDependencyModule) all have generated runtime metadata, so Cancel always takes the runtime.CancelCompletionSource branch in tests. Not blocking, since the logic is a straight copy of the already-proven CompletionSourceSetterCache expression-tree pattern, but worth a follow-up test with a module type that forces the fallback (e.g. one with an open generic argument) if you want the new class fully covered.

Nice work closing this out — the fix now covers both hang triggers named in #3458, and the added tests reproduce both end-to-end.

[thomhurst]

Rebased onto current main at 8c247942fb. Patch equivalence preserved (06eb99dcce230266dc9af49f74c5b12d206f0c22 before and after). New head: a8f167a165.

@codex review

[claude]

Review

Solid fix for the original hang (#3458): both call sites named in the issue — ModuleStateTracker.CancelPendingModules and ModuleResultRegistrar.RegisterTerminatedResult — now complete a module's typed Module<T>.CompletionSource instead of leaving it stuck forever, and the generated-metadata-first/reflection-fallback split (ModuleCompletionSourceCanceller) is consistent with the rest of the engine's AOT/trimming story (ModuleExecutionDelegateFactory, CompletionSourceSetterCache). The new end-to-end tests reproduce the dynamic-await deadlock convincingly.

Unresolved P1 from prior review round: the two fixes race each other

This was flagged inline on the current head (a8f167a165, ModuleStateTracker.cs:286) and doesn't appear to have a follow-up commit addressing it, so re-raising it here since it's a real correctness gap, not a style nit:

ExecuteWithSchedulerAsync in ModuleExecutor.cs registers scheduler.CancelPendingModules on the cancellation token (ModuleExecutor.cs:191) and that fires before RegisterTerminatedResultsForCancelledModules runs later in the same method (ModuleExecutor.cs:178-181), once firstException != null (e.g. StopOnFirstException, still the common configuration).

With this PR's change, that means for every module cancelled-while-pending in that flow:

1. CancelPendingModules calls ModuleCompletionSourceCanceller.Cancel(...), which does TrySetCanceled() on the module's typed Module<T>.CompletionSource immediately.
2. RegisterTerminatedResultsForCancelledModules → RegisterTerminatedResult then tries runtime.SetCompletionSource(module, result) (→ TrySetResult(...)) for the same completion source — but it's already completed, so TrySetResult silently returns false.

The net effect: _resultRegistry.GetResult(moduleType) holds a proper ModuleResult<T> with Status.PipelineTerminated and the causing exception, but anything that does await context.GetModule<T>() or IModule.ResultTask on that same module observes a bare OperationCanceledException/TaskCanceledException instead — because ResultTask is CompletionSource.Task.ContinueWith(t => (IModuleResult) t.Result, ...) (Module.cs), and accessing .Result on a canceled antecedent throws rather than yielding the terminated result. So the two mechanisms this PR adds for the same scenario disagree with each other, and the richer, purpose-built RegisterTerminatedResult path (already proven correct for the "dependency failed" case earlier in this PR's history) is effectively dead code whenever a module goes through CancelPendingModules first.

The existing tests don't catch this because neither CancelledBeforeStartModule_UnblocksRuntimeAwaiter nor TerminatedBeforeExecutionModule_UnblocksRuntimeAwaiter routes through CancelPendingModules — both hit the direct ModuleRunner.ExecuteCore catch-block path instead, where RegisterTerminatedResult runs uncontested. A test that drives an actual StopOnFirstException cancellation of a still-pending module and asserts on the awaited ModuleResult's Status/Exception (not just that the pipeline exits) would reproduce this.

Given the issue itself says either fix is sufficient ("TrySetCanceled in CancelPendingModules ... or set the PipelineTerminated result in RegisterTerminatedResult(s)"), the cleanest resolution is probably to not have CancelPendingModules race the richer path at all — e.g. only eagerly cancel there when there's no exception context to build a proper terminated result from (true external cancellation, no firstException), and otherwise let RegisterTerminatedResultsForCancelledModules be the single source of truth for completing these modules' typed completion sources, mirroring how IgnoredModuleResultRegistrar already does this cleanly for the ignored-via-history case.

Minor (non-blocking, previously noted and still present)

• ModuleCompletionSourceCanceller.GetResultType (ModuleCompletionSourceCanceller.cs:35-47) walks the base-type chain via reflection to find the closed Module<> generic argument, but IModule.ResultType already exposes this for free (Type IModule.ResultType => typeof(T)), and is already used the same way in ModuleResultRegistrar.RegisterTerminatedResult and IgnoredModuleResultRegistrar. Since Cancel is always called with the IModule instance in hand, this could just be Cache.GetOrAdd(module.ResultType, CreateCanceller)(module), dropping the ~15-line reflection walk.
• The reflection-fallback branch of ModuleCompletionSourceCanceller.CreateCanceller (no generated runtime metadata) isn't exercised by any test — all covered scenarios have generated metadata, so Cancel always takes the runtime.CancelCompletionSource branch.

Nice, focused approach overall — recommend closing the completion-source race above before merge, since it reintroduces (in a different, more silent form) the same "awaiter gets the wrong outcome" class of bug this PR sets out to fix.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Chef's kiss.

Reviewed commit: a8f167a165

ℹ️ 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 blocking StopOnFirstException race in a941c73.

Standalone execution now cancels scheduler state without cancelling typed module result awaiters; ModuleResultRegistrar can therefore complete them with the registered PipelineTerminated result. Direct/distributed scheduler cancellation retains immediate typed-task cancellation. Also replaced fallback result-type reflection with IModule.ResultType.

TDD/validation:

• EngineCancellationTokenTests: 9/9 passed
• ModuleExecutorLoggingTests: 11/11 passed
• scoped formatting/analyzers: clean at error severity
• ModularPipelines.sln Release build: 0 errors (225 existing warnings)

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a941c7371c

ℹ️ About Codex in GitHub

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

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

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

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

[claude]

Review

The unresolved P1 from the previous round — the race between CancelPendingModules and RegisterTerminatedResultsForCancelledModules — is fixed in a941c737 (fix(engine): preserve terminated awaiters).

• ModuleExecutor.RegisterCancellationCallback now registers scheduler.CancelPendingModules(cancelModuleResultAwaiters: false) for the standalone execution path (

  ModularPipelines/src/ModularPipelines/Engine/ModuleExecutor.cs

  Lines 189 to 193 in a941c73

  	private void RegisterCancellationCallback(CancellationTokenSource cancellationTokenSource, IModuleScheduler scheduler)
  	{
  	cancellationTokenSource.Token.Register(
  	() => scheduler.CancelPendingModules(cancelModuleResultAwaiters: false));
  	}

  ). This defers completing a module's typed Module<T>.CompletionSource to RegisterTerminatedResultsForCancelledModules (

  ModularPipelines/src/ModularPipelines/Engine/Execution/ModuleResultRegistrar.cs

  Lines 53 to 71 in a941c73

  	/// <inheritdoc />
  	public void RegisterTerminatedResultsForCancelledModules(IReadOnlyList<IModule> modules, Exception exception)
  	{
  	foreach (var module in modules)
  	{
  	var moduleType = module.GetType();
  	// Check if a result was already registered for this module
  	if (_resultRegistry.GetResult(moduleType) != null)
  	{
  	continue;
  	}
  	_logger.LogDebug(
  	"Registering PipelineTerminated result for cancelled module {ModuleName}",
  	moduleType.Name);
  	RegisterTerminatedResult(module, moduleType, exception);
  	}

  ), which now runs uncontested — the eager ModuleCompletionSourceCanceller.Cancel call is skipped, so RegisterTerminatedResult's SetCompletionSource/TrySetResult is no longer racing an already-canceled completion source.
• I traced the ordering to make sure this can't reintroduce the original hang: the only place that calls CancelPendingModules(false) with modules actually pending is inside ModuleExecutor's StopOnFirstException catch handler, which sets firstException via Interlocked.CompareExchange before calling EnsureCancellation/Cancel() (

  ModularPipelines/src/ModularPipelines/Engine/ModuleExecutor.cs

  Lines 229 to 235 in a941c73

  	{
  	_secondaryExceptionContainer.RegisterException(ex);
  	Interlocked.CompareExchange(ref firstException, ex, null);
  	}
  	EnsureCancellation(cancellationTokenSource);
  	}

  ) — so by the time RegisterTerminatedResultsForCancelledModules(modules, firstException) runs afterward, firstException is guaranteed non-null and every module left pending gets a proper PipelineTerminated result instead of a bare cancellation.
• DistributedModuleExecutor/WorkerModuleScheduler still take the immediate-cancellation path (default cancelModuleResultAwaiters: true), which is correct — that flow has no later "register terminated result" step for it to race against.
• The other minor note from the last round — ModuleCompletionSourceCanceller.GetResultType's reflection walk over the base-type chain — is also gone; Cancel now uses module.ResultType directly (

  ModularPipelines/src/ModularPipelines/Engine/ModuleCompletionSourceCanceller.cs

  Lines 26 to 29 in a941c73

  	}
  	Cache.GetOrAdd(module.ResultType, CreateCanceller)(module);
  	}

  ), matching the pattern already used in ModuleResultRegistrar/IgnoredModuleResultRegistrar.
• New regression test StopOnFirstException_PendingModuleAwaiterReturnsTerminatedResult (

  ModularPipelines/test/ModularPipelines.UnitTests/Execution/EngineCancellationTokenTests.cs

  Lines 338 to 371 in a941c73

  	[Test]
  	public async Task StopOnFirstException_PendingModuleAwaiterReturnsTerminatedResult()
  	{
  	var builder = TestPipelineHostBuilder.Create()
  	.ConfigurePipelineOptions(options => options with
  	{
  	DefaultModuleTimeout = TimeSpan.Zero,
  	ThrowOnPipelineFailure = true,
  	Concurrency = options.Concurrency with
  	{
  	MaxParallelism = 1,
  	},
  	})
  	.AddModule<StopOnFirstFailingModule>()
  	.AddModule<StopOnFirstQueuedDependencyModule>()
  	.AddModule<StopOnFirstPendingModule>();
  	var host = await builder.BuildAsync();
  	var resultRegistry = host.Services.GetRequiredService<IModuleResultRegistry>();
  	var pendingModule = host.Services.GetServices<IModule>()
  	.OfType<StopOnFirstPendingModule>()
  	.Single();
  	var exception = await Assert.ThrowsAsync<ModuleFailedException>(
  	async () => await host.RunAsync().WaitAsync(TimeSpan.FromSeconds(5)));
  	var awaitedResult = await ((IModule)pendingModule).ResultTask
  	.WaitAsync(TimeSpan.FromSeconds(1));
  	var registeredResult = resultRegistry.GetResult(typeof(StopOnFirstPendingModule));
  	using (Assert.Multiple())
  	{
  	await Assert.That(awaitedResult).IsSameReferenceAs(registeredResult);
  	await Assert.That(awaitedResult.ModuleStatus).IsEqualTo(Status.PipelineTerminated);
  	await Assert.That(awaitedResult.ExceptionOrDefault)

  ) reproduces exactly the previously-broken scenario and asserts the awaited ResultTask is the same reference as the registered PipelineTerminated result.

I pulled the branch and verified rather than trusting the description alone: ModularPipelines.sln builds with 0 errors, and EngineCancellationTokenTests (9/9) and ModuleExecutorLoggingTests (11/11) pass locally.

Non-blocking observation

Threading a cancelModuleResultAwaiters bool through IModuleScheduler → ModuleScheduler → IModuleStateTracker → ModuleStateTracker (plus a no-op override in WorkerModuleScheduler) works and is well-tested, but it's a slightly indirect way to say "the caller promises to complete these typed completion sources itself, later." A version of this fix that made RegisterTerminatedResultsForCancelledModules/RegisterTerminatedResult the single universal mechanism for completing any not-yet-completed typed completion source — dropping ModuleCompletionSourceCanceller's direct-cancel path entirely, even for distributed/direct-scheduler callers — would remove the need for the flag and collapse two diverging completion paths into one. Not requesting a change here: the current fix is minimal, correctly scoped, defaults preserve existing distributed behavior, and it's backed by a test that reproduces the exact bug. Just flagging as a possible future simplification if a third caller with different completion requirements shows up.

Both correctness gaps raised in earlier review rounds are now closed and covered by tests. Approving.

[thomhurst]

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit: ba3f1518a3

ℹ️ About Codex in GitHub

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

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

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

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

[claude]

Review

Confirms previously-raised issues are fixed on this head (ba3f1518a3). Checked the diff against the earlier CHANGES_REQUESTED rounds in this thread:

• The "two fixes race each other" P1 (typed completion source cancelled by CancelPendingModules before RegisterTerminatedResultsForCancelledModules can set the real terminated result) is fixed — ModuleExecutor.RegisterCancellationCallback now passes cancelModuleResultAwaiters: false (ModuleExecutor.cs:191), so only RegisterTerminatedResultsForCancelledModules completes the typed source for the standalone path.
• The earlier "unnecessary reflection walk" nit (GetResultType over the base-type chain) is gone; ModuleCompletionSourceCanceller.Cancel now uses module.ResultType directly, matching ModuleResultRegistrar/IgnoredModuleResultRegistrar.

The newest commit adds a well-reasoned third fix. RegisterTerminatedResultsForCancelledModules now skips AlwaysRun modules (ModuleResultRegistrar.cs:58-63). Without this, bulk cancellation could stamp a synthetic PipelineTerminated result/completion onto an AlwaysRun module before AlwaysRunHandler.WaitForAlwaysRunModulesAsync (invoked later, from the outer catch in ModuleExecutor.ExecuteAsync) gets to run it for real — the module's genuine result would then silently lose the race, since TrySetResult/TrySetResult on an already-completed TaskCompletionSource is a no-op. Cancelled_Result_Registration_Defers_AlwaysRun_Completion verifies the deferral directly. Good catch.

Suggestion: unify the two completion-source reflection caches

ModuleCompletionSourceCanceller (new, src/ModularPipelines/Engine/ModuleCompletionSourceCanceller.cs) duplicates almost the entire body of the pre-existing CompletionSourceSetterCache (IgnoredModuleResultRegistrar.cs:187+): both cache ConcurrentDictionary<Type, Action<...>>, both do typeof(Module<>).MakeGenericType(resultType), both reflect the same non-public CompletionSource property with the same BindingFlags, and both compile near-identical Expression trees. The only real difference is the trailing method call (TrySetCanceled() vs TrySetResult(...)).

That's two independently-maintained reflection caches resolving the exact same "get Module<T>.CompletionSource" accessor. A shared helper — e.g. a cached Func<IModule, object> that resolves the boxed CompletionSource per result type — would let both CompletionSourceSetterCache.CreateSetter and ModuleCompletionSourceCanceller.CreateCanceller just compile their one differing method call against the shared accessor's output, instead of re-deriving moduleType/completionSourceProperty from scratch. Not a correctness issue, but this PR is exactly the kind of change (a new "do X to the completion source" operation) that the shared accessor would have turned into a one-line addition rather than a new 58-line file — and it heads off a fourth copy-pasted cache the next time someone needs TrySetException or similar.

Minor (non-blocking): distributed cancellation semantics are now intentionally asymmetric with standalone

DistributedModuleExecutor.cs:92 still calls scheduler.CancelPendingModules() with the default cancelModuleResultAwaiters: true, so a distributed pipeline's runtime await context.GetModule<T>() on a module cancelled-while-pending still resolves via a bare OperationCanceledException, whereas the standalone path now resolves the same scenario to a proper PipelineTerminated result. A prior review round already confirmed this is deliberate (the distributed flow has no later "register terminated result" step to race against), so I'm not reopening it as a blocker — just flagging that the two engines now diverge in what a cancelled runtime await actually returns (thrown exception vs. result object), which is worth a short note on IModuleScheduler.CancelPendingModules's doc comment so it reads as an intentional design choice rather than an oversight to the next person touching this code.

Overall: a solid, iteratively-hardened fix with targeted regression coverage for each race that was found along the way. No blocking issues from me.

[thomhurst]

Rebased onto current main and fixed the changed-file SA1629 failures reported by CI. Scoped SA1629 verification and the core Release build pass (0 errors). @codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 928ff3df8c

ℹ️ About Codex in GitHub

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

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

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

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

[claude]

Review: Complete pending module awaiters on cancellation (#3565)

I traced the full lifecycle: ModuleExecutor.RegisterCancellationCallback → IModuleScheduler.CancelPendingModules(cancelModuleResultAwaiters: false) → ModuleStateTracker.CancelPendingModules → (later) ModuleResultRegistrar.RegisterTerminatedResultsForCancelledModules, plus DependencyWaiter, ModuleRunner.ExecuteCore, and Module<T>.GetAwaiter()/CompletionSource. The two new EngineCancellationTokenTests (CancelledBeforeStartModule_UnblocksRuntimeAwaiter, TerminatedBeforeExecutionModule_UnblocksRuntimeAwaiter) pass locally, confirming the fix works for the case they cover.

Correctness: the fix appears to only close the deadlock for one sub-case

The two new regression tests both give the awaited module (CancelledBeforeStartModule / TerminatedBeforeExecutionModule) a [DependsOn<T>] on the same module that fails (CoordinatedFailingModule / CoordinatedDependencyFailureModule). Tracing that path: the awaited module does get dispatched (its dependency-wait in DependencyWaiter.WaitForDependenciesAsync awaits the failed module's untyped ModuleState.CompletionSource, which CancelPendingModules always completes immediately regardless of the new flag). That throws inside ModuleRunner.ExecuteCore's catch, which calls _resultRegistrar.RegisterTerminatedResult(...) directly and immediately — completing the typed Module<T>.CompletionSource right there. So these two tests are actually exercising the pre-existing "dependency-failure propagates a terminated result" path (ModuleRunner.cs:127-131), not the new deferred-cancellation path.

The deferred path this PR adds (ModuleResultRegistrar.RegisterTerminatedResultsForCancelledModules, ModuleResultRegistrar.cs:54-79) only runs in ModuleExecutor.ExecuteWithSchedulerAsync after ExecuteWorkerPoolAsync returns (ModuleExecutor.cs:156-181), and ExecuteWorkerPoolAsync's Parallel.ForEachAsync can't return until every dispatched worker body finishes.

Consider a module C that has no DependsOn relationship to the module that fails — it's simply still sitting Queued in the ready channel because MaxParallelism is exhausted (ModuleScheduler.FindReadyModules/GetCancellablePendingModules treats Queued-but-undispatched modules as cancellable pending modules, ModuleStateQueries.cs:73-79). If another module A is concurrently running and does a dynamic runtime await (await context.GetModule<C>(), no DependsOn<C> — this is a documented, PR-tested pattern, not a misuse), and a peer module B fails, triggering CancelPendingModules(cancelModuleResultAwaiters: false):

• C's untyped ModuleState.CompletionSource gets cancelled immediately (harmless, nothing awaits it via a declared dependency).
• C's typed Module<T>.CompletionSource — the one A's context.GetModule<C>() is blocked on — is not touched (deferred by design).
• C never runs (channel never drained further because the cancellation token is now set), so nothing else will ever complete its typed completion source.
• A's worker task therefore blocks forever inside Parallel.ForEachAsync, which means ExecuteWorkerPoolAsync never returns, which means RegisterTerminatedResultsForCancelledModules (the only thing that would unblock A) never runs.

That's a circular wait — the same deadlock class issue #3458 describes, just for peers that aren't DependsOn-linked to the failure. I wasn't able to add a throwaway test to confirm empirically in this sandbox (file-write permission wasn't granted), but the code path is unambiguous by inspection. Suggest adding a regression test shaped like the existing CancelledBeforeStartModule_UnblocksRuntimeAwaiter but without any DependsOn edge between the awaited module and the failing module (only tied together by MaxParallelism exhaustion), and confirming it doesn't hang.

If this is confirmed, a more robust fix than "defer and hope everything drains" might be to have CancelPendingModules complete the typed completion sources itself for genuinely-unreachable modules once it has enough information to build a PipelineTerminated result (i.e., do what RegisterTerminatedResultsForCancelledModules does, right there, instead of splitting the operation across a boolean flag and a second call site — see below).

Design: cancelModuleResultAwaiters boolean threaded through 4 layers

IModuleScheduler.CancelPendingModules(bool cancelModuleResultAwaiters = true) / IModuleStateTracker.CancelPendingModules(...) / ModuleScheduler / ModuleStateTracker / WorkerModuleScheduler all now carry this flag, and its false value is only meaningful if the caller also remembers to call RegisterTerminatedResultsForCancelledModules afterwards (as ModuleExecutor does, in a different method, at a different time). That's an implicit two-phase protocol enforced only by convention between two call sites in two different classes. A future caller of scheduler.CancelPendingModules(false) (e.g. a new distributed/worker code path) that forgets the second half will silently reintroduce hangs for dynamic runtime awaiters, with no compiler or analyzer signal.

Given the deadlock risk above, I'd suggest collapsing this into a single operation — e.g. IModuleResultRegistrar/IModuleScheduler exposes one method that atomically cancels pending state and resolves typed completion sources with the terminated result (using generated metadata/CompletionSourceSetterCache the way RegisterTerminatedResultsForCancelledModules already does), so there's no window where a "cancelled but not yet resolved" state can be observed by a concurrent awaiter, and no way to call one half without the other.

Minor: duplicated reflection/expression-tree caches for CompletionSource access

ModuleCompletionSourceCanceller (new file) builds a compiled Expression delegate that reflects on Module<T>.CompletionSource and calls TrySetCanceled(), caching by Type. CompletionSourceSetterCache (IgnoredModuleResultRegistrar.cs:187-241, pre-existing) does the same thing for TrySetResult(...). Both:

• reflect for the non-public CompletionSource property via MakeGenericType(typeof(Module<>), resultType),
• cache the compiled delegate in a ConcurrentDictionary<Type, ...>,
• carry the same AOT/trimming suppression attributes.

Worth factoring into one shared cache that resolves the CompletionSource property/expression once per resultType and exposes both the "set result" and "cancel" delegates (e.g. a small record/tuple of delegates), rather than maintaining two independent reflection paths that could drift (e.g. if the CompletionSource property is ever renamed, one cache could be updated and the other missed).

Minor: RegisterTerminatedResultsForCancelledModules now permanently excludes AlwaysRun

The new if (module.ModuleRunType == ModuleRunType.AlwaysRun) { continue; } (ModuleResultRegistrar.cs:58-63) means an AlwaysRun module's typed completion source is only ever completed by AlwaysRunHandler.WaitForAlwaysRunModulesAsync. That handler can bail out without completing every module (e.g. the "AlwaysRun modules could not make progress because their dependency graph has no ready modules" branch in AlwaysRunHandler.cs:41-47, or a scheduler-progress timeout). Previously, RegisterTerminatedResultsForCancelledModules was the unconditional fallback that guaranteed every cancelled module's typed completion source got resolved eventually. Now that guarantee no longer holds for AlwaysRun modules specifically in that failure branch — anything awaiting such a module via context.GetModule<T>() could still hang, even though the overall pipeline correctly fails via the AggregateException. This is a narrower/pre-existing edge case (I didn't find a test exercising it either way), but worth a deliberate look given it's a direct trade-off this PR is making.

What looks solid

• The core mechanism (defer typed-CS cancellation, then resolve with a real PipelineTerminated result) is the right shape for fixing the declared-dependency case, and the two new tests plus Cancelled_Result_Registration_Defers_AlwaysRun_Completion / Generated_Runtime_Cancels_Typed_Completion_Source verify it precisely.
• Keeping immediate cancellation (cancelModuleResultAwaiters: true) for the distributed/worker and direct scheduler callers (DistributedModuleExecutor.cs:92) while only deferring for the standalone in-process path is a sensible, narrowly-scoped choice.
• I ran the full EngineCancellationTokenTests suite locally (9/9 passed) to confirm the new tests as committed are green.

[thomhurst]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5571c29d72

ℹ️ 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 cleanly onto current main after #3571 merged. Publish-failure lifecycle regression remains green: DistributedModuleExecutorTests 26/26. New head ce77e1e. @codex review

[claude]

Review — head ce77e1e4

Followed up on the long review thread on this PR. All of the previously-flagged P1s I could find in the history are now resolved on this head:

• Standalone StopOnFirstException/AlwaysRun completion-source races (multiple earlier rounds) — fixed and covered by EngineCancellationTokenTests.
• Distributed "reserve before publish" / cancelled-queue race / constraint-deferral-vs-cancellation (MarkModuleStarted now called before the collector task is created, and rejects any state other than Pending/Queued at ModuleStateTracker.cs:102, so a Completed module can no longer be restarted into Executing) — fixed.
• The last blocking item (PublishAsync failure orphaning the collector task because publish happened outside any try/catch that resolves the module) — fixed by folding publish into PublishAndCollectDistributedResultAsync's single try/catch (Exception) (DistributedModuleExecutor.cs:409-446), and it's now directly tested by Publish_Failure_Completes_Module_Result.
• AlwaysRunHandler.CanLateStartAlwaysRunModule now covers both Pending and Queued (AlwaysRunHandler.cs:247-250), closing the "queued AlwaysRun module never late-started" gap.

This has clearly been an iteratively-hardened pass and the core mechanism (generated-metadata-first / reflection-fallback completion, deferred-vs-immediate cancellation split between standalone and distributed) is sound and well tested.

Remaining gap: the configured module/result timeout doesn't bound the publish call

PublishAndCollectDistributedResultAsync (src/ModularPipelines/Distributed/Master/DistributedModuleExecutor.cs:409-424):

using var timeoutCts = CreateResultTimeoutSource(module.Configuration.Timeout, cts.Token);

try
{
    ...
    await _publisher.PublishAsync(assignment, cts.Token);                          // <-- cts.Token, not the timeout token
    await CollectResultAsync(module, moduleType, scheduler, cts, timeoutCts?.Token ?? cts.Token);
}

timeoutCts is created up front but only ever handed to CollectResultAsync — the PublishAsync (→ EnqueueModuleAsync, a real broker/coordinator call) is only linked to cts.Token, i.e. pipeline cancellation, not the per-module/default result timeout. If the broker call itself stalls (dropped connection, backpressure, etc.) rather than throwing, a configured WithTimeout(...) or ModuleResultTimeoutSeconds will never fire for that module, because the timeout token was never passed to the operation it's supposed to bound. That reintroduces the exact "typed Module<T>.CompletionSource never resolves" hang this PR exists to close — just via a stuck publish instead of a stuck collect. I didn't find a test that simulates a hanging (rather than throwing) PublishAsync with a timeout configured — the existing timeout tests (Distributed_Result_Wait_Times_Out, Module_Timeout_Takes_Precedence_Over_Default_Result_Timeout) only exercise the collect-side wait.

Suggest passing timeoutCts?.Token ?? cts.Token to _publisher.PublishAsync as well, so the same token bounds the whole claim→publish→collect lifecycle rather than just its tail half.

Minor, non-blocking: inconsistent terminal Status on the publish/collector failure path

RegisterFailureResult (DistributedModuleExecutor.cs:491-506) builds new ModuleExecutionContext(module, moduleType) directly, which defaults Status to NotYetStarted (ModuleExecutionContext.cs:54) and never sets it — unlike ModuleResultRegistrar.RegisterTerminatedResult, which explicitly stamps Status.PipelineTerminated via ExecutionContextFactory. So a module that fails because publish/collection blew up (or because cancellation propagated into that path) ends up with a result whose Status reads "not yet started" rather than something terminal — misleading for anything inspecting IModuleResult.Status for diagnostics/reporting. Was flagged in an earlier review round as P2 and still applies; not worth blocking on, but a one-line executionContext.Status = Status.PipelineTerminated; (or Failed, depending on cause) before creating the result would make this consistent with the standalone path.

Design note (recurring across this thread, still true): three near-identical CompletionSource accessors

The engine now has CompletionSourceSetterCache (IgnoredModuleResultRegistrar.cs:187+, reflection+expression-tree, TrySetResult), ModuleCompletionSourceCanceller (ModuleCompletionSourceCanceller.cs, same reflection pattern, TrySetCanceled), and ModuleCompletionSourceApplicator (thin wrapper over IModule.TrySetDistributedResult, used by the distributed path added in this PR). The first two are literally the same reflection/expression-compilation logic differing only in the trailing call. This has been raised non-blocking in three earlier rounds and hasn't moved — I'm not asking for it here either, since it's cosmetic and each round has correctly prioritized closing actual hangs over refactoring. But given this PR is adding a third variant of "resolve a module's completion source from the outside," it's a good marker that the next time this area is touched, collapsing the two reflection caches into one accessor (returning the boxed CompletionSource, with each caller doing its own one-line terminal call) would prevent a fourth copy the next time someone needs e.g. TrySetException.

Verdict

Everything previously raised as blocking is fixed and tested. The publish-timeout gap above is the one item I'd want closed before merge, since it's in the same failure class (await context.GetModule<T>() hanging forever) that #3458 and this whole PR are about — everything else is a non-blocking follow-up.

[chatgpt-codex-connector]

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

Reviewed commit: ce77e1e485

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

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 69f403125e

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

Complete awaiters when the scheduler faults

When ReadyModules faults before a worker assigns firstException—for example, when ModuleScheduler detects a deadlock and completes the channel with DependencyCollisionException—ExecuteWorkerPoolAsync exits through its finally and invokes this callback. The callback transitions pending modules to completed but deliberately leaves their typed result completion sources unset, while the registrar at line 178 only runs when firstException is non-null. The outer catch can then start an AlwaysRun module that awaits one of those modules and hang indefinitely; non-worker failure paths must also register terminated results.

Useful? React with 👍 / 👎.

[claude]

Review — head 69f40312

This has been through many iterative rounds already (9 prior claude[bot] reviews), and I focused on: (a) whether the last blocking item is actually resolved on this head, and (b) scanning the full current diff independently for anything new/regressive.

Previously-blocking P1 is now fixed

The last review round (head ce77e1e4) flagged that PublishAndCollectDistributedResultAsync created a timeoutCts up front but only ever passed it to CollectResultAsync — the _publisher.PublishAsync(...) call (a real broker/coordinator call) was still bounded only by cts.Token (pipeline cancellation), not by the per-module/default result timeout. A broker call that stalls without throwing would never time out, reintroducing the exact "Module<T>.CompletionSource never resolves" hang class this PR exists to close.

The final commit (69f40312, "fix(distributed): time out stalled publish") fixes exactly this: both PublishAsync and CollectResultAsync now share a single lifecycleToken = timeoutCts?.Token ?? cts.Token (DistributedModuleExecutor.cs:416-425), and it's covered by a new regression test, Publish_Timeout_Completes_Module_Result, which simulates a PublishAsync that hangs indefinitely (Task.Delay(Timeout.InfiniteTimeSpan, token)) with a short module timeout configured, and asserts the module still completes with a TimeoutException rather than hanging forever. Good — this was the one item I'd have wanted closed before merge, and it now is.

Overall assessment of the mechanism

Having read through the full diff independently (not just diffing against the last review), the core design holds together:

• The cancelModuleResultAwaiters bool on CancelPendingModules/IModuleStateTracker cleanly separates "cancel the typed CompletionSource immediately" (safe default, used only by tests) from "defer to RegisterTerminatedResultsForCancelledModules, which completes it with a rich PipelineTerminated ModuleResult<T> instead of a bare OperationCanceledException" (the pattern every production call site actually uses). Defaulting the interface to the immediately-safe variant, with production code always passing false explicitly alongside a follow-up registrar call, is the right way to keep this safe against a future caller that forgets the follow-up call.
• MarkModuleStarted's new guard (ModuleStateTracker.cs:102-105, only proceed from Pending/Queued) correctly closes the claim race where a module cancelled concurrently with a scheduler dispatch could otherwise be restarted into Executing after already being marked Completed. DistributedModuleExecutor.ExecuteAsync now checks the MarkModuleStarted return value and skips publish/collect entirely on false (previously this return value was silently ignored) — verified by Rejected_Start_Does_Not_Publish_Or_Collect_Result and Deferred_Start_Publishes_Only_After_Scheduler_Requeues_Module.
• The AlwaysRun skip in RegisterTerminatedResultsForCancelledModules (ModuleResultRegistrar.cs:60-63) is correctly defensive even though GetCancellablePendingModules already excludes AlwaysRun — it's needed because ModuleExecutor.ExecuteWithSchedulerAsync calls this method with the full module list (not just the cancelled subset) as a catch-all, and an in-flight AlwaysRun module must not have its real, still-pending result clobbered by a synthetic terminated one. AlwaysRunModuleAwaitingTerminatedModule_UnblocksRuntimeAwaiter exercises this end-to-end (an AlwaysRun module awaiting a dependency that gets terminated still completes successfully afterward), which is a good test of a genuinely subtle interaction.
• I traced whether the standalone ModuleExecutor's RegisterTerminatedResultsForCancelledModules(modules, firstException) catch-all (ModuleExecutor.cs:178-181) could ever be skipped while a pipeline was actually cancelled but firstException stayed null (which would leave a context.GetModule<T>() awaiter hanging). Since ExecuteWithSchedulerAsync's cancellationTokenSource is a bare token only ever cancelled from within the StopOnFirstException catch block in ExecuteWorkerPoolAsync, cancellation and firstException != null are equivalent in this class today — so no gap there currently. Worth keeping in mind if a future change ever links that token to something external (e.g. IHostApplicationLifetime) without also routing through firstException.

Non-blocking, carried forward from earlier rounds (still true, not asking for changes here)

• RegisterFailureResult in DistributedModuleExecutor.cs (~line 492) still builds new ModuleExecutionContext(module, moduleType) without stamping Status, so it defaults to Status.NotYetStarted (ModuleExecutionContext.cs:54) — unlike the standalone path's RegisterTerminatedResult, which explicitly sets Status.PipelineTerminated. A module that fails via the publish/collect exception path ends up with a diagnostically misleading terminal status. One line (executionContext.Status = Status.PipelineTerminated;) would fix it; flagged as P2 in an earlier round and still minor/cosmetic.
• Three now-near-identical "reach into Module<T>'s private CompletionSource via reflection/expression-tree" accessors exist: CompletionSourceSetterCache (TrySetResult), the new ModuleCompletionSourceCanceller (TrySetCanceled), and ModuleCompletionSourceApplicator (thin wrapper over IModule.TrySetDistributedResult). The first two differ only in their trailing call. Not asking for a refactor in this PR — closing the actual hangs has correctly been prioritized every round — but this PR adds a third variant, so the next time this area is touched, extracting the shared "resolve the boxed CompletionSource for a resultType" lookup into one cache (with each caller doing its own one-line terminal call) would avoid a fourth copy.
• Minor, very low priority: in DistributedModuleExecutor.ExecuteAsync's reader loop (DistributedModuleExecutor.cs:107-112), _publisher.CreateAssignment(moduleState.Module) (which does reflection-based capability scanning and dependency-result gathering) runs before the MarkModuleStarted check, so a rejected start still pays for building (and discarding) the assignment. No functional impact — just avoidable work on the rejected-start path.

Verdict

The previously-identified blocking gap is resolved and directly regression-tested. I didn't find anything new or regressive in the rest of the diff. The remaining items above are cosmetic/follow-up material that multiple prior rounds have correctly deprioritized. CI was still running at review time (several jobs pending) — worth confirming it goes green before merge, but nothing in the diff itself blocks approval.

[claude]

Follow-up to my review above, after tracing the engine-cancellation path in more detail:

Correction/precision on my "cancellation and firstException != null are equivalent" note: more precisely, pure engine-level cancellation (Ctrl+C, EngineCancellationToken) never needs ModuleExecutor's CancelPendingModules/RegisterTerminatedResultsForCancelledModules mechanism at all — it doesn't touch ModuleExecutor's local cancellationTokenSource in that path. Each module independently observes the already-cancelled engine token via ModuleExecutionPipeline.SetupCancellation/HandleException/IsPipelineCancelled (ModuleExecutionPipeline.cs:191-211, 404-413, 458-462) and completes its own typed CompletionSource with a PipelineTerminated result directly, bypassing this PR's mechanism entirely. So the conclusion stands (no hang for pure engine cancellation), just via a different, pre-existing code path than the one I described.

Unrelated, pre-existing gap worth a follow-up issue (not blocking this PR): if ModuleScheduler's background scheduling loop throws DependencyCollisionException (deadlock detection, ModuleScheduler.cs:676-679), that exception propagates out of scheduler.ReadyModules.ReadAllAsync uncaught — not as OperationCanceledException — which means it isn't caught by ExecuteWorkerPoolAsync's catch (OperationCanceledException) when (firstException != null) and bypasses RegisterTerminatedResultsForCancelledModules entirely. Any modules still pending at that point would be left with an incomplete typed CompletionSource — the same hang class #3458/this PR is about, just triggered by a scheduler-detected deadlock instead of a module exception. Since this is orthogonal to the cancellation-completion changes here and predates this PR, I wouldn't block on it, but it seems worth a follow-up issue given how much care has gone into closing every other variant of this hang in this thread.

[thomhurst]

Rebased onto current main; cancellation, scheduler, metadata, and distributed executor suites pass (75/75). @codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. More of your lovely PRs please.

Reviewed commit: dd520a4e74

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

Follow-up review: #3565 — Complete pending module awaiters on cancellation

Re-reviewed the current head (dd520a4e74). The only change since the last approved commit (ce77e1e485, which Codex also cleared with no comments) is the new fix(distributed): time out stalled publish commit.

The new commit is correct

PublishAndCollectDistributedResultAsync previously called _publisher.PublishAsync(assignment, cts.Token) with the bare pipeline token, while only CollectResultAsync used the per-module timeoutCts.Token:

await _publisher.PublishAsync(assignment, cts.Token);
await CollectResultAsync(module, moduleType, scheduler, cts, timeoutCts?.Token ?? cts.Token);

That meant a stalled _publisher.PublishAsync → _coordinator.EnqueueModuleAsync call (broker hang, slow connection) was only bounded by full pipeline cancellation, not by the module's configured result timeout — the exact "typed completion source never resolves" failure mode this PR has been closing at every other call site. The fix unifies both calls onto one lifecycleToken:

var lifecycleToken = timeoutCts?.Token ?? cts.Token;
...
await _publisher.PublishAsync(assignment, lifecycleToken);
await CollectResultAsync(module, moduleType, scheduler, cts, lifecycleToken);

Since CreateResultTimeoutSource links timeoutCts to cts.Token and adds CancelAfter(timeout), this is a strict superset of the old cancellation coverage — pipeline cancellation still cancels lifecycleToken (falls into the un-guarded catch (OperationCanceledException) branch), and now a timeout during publish also correctly lands in the catch (OperationCanceledException) when (!cts.IsCancellationRequested) branch (since only the linked timer fired, not cts itself), registering a TimeoutException failure result exactly like a stalled collect does today. No behavioral regression for the non-timeout path since timeoutCts is null (and lifecycleToken falls back to cts.Token) when no timeout is configured.

Publish_Timeout_Completes_Module_Result (new) exercises this directly: it makes EnqueueModuleAsync hang via Task.Delay(Timeout.InfiniteTimeSpan, token) on a module with a short configured timeout, and asserts ResultTask resolves to TimeoutException, the registry gets the same result, WaitForResultAsync is never called (publish never actually completed), and MarkModuleCompleted(..., false, null, null) fires once — good coverage of the new path, distinct from the existing collect-side timeout test.

Everything else

All prior rounds on this thread (single-node engine fix, AlwaysRun deferral, queued-worker-claim rejection, distributed-master PipelineTerminated wiring, the collector race, claim-before-publish ordering, and the publish-failure lifecycle fix) are unchanged since my last approval and still hold up — this is purely an incremental hardening on top of an already-reviewed base.

Non-blocking (repeated from earlier rounds)

ModuleCompletionSourceCanceller and the pre-existing CompletionSourceSetterCache remain near-duplicate reflection/Expression-compiled caches over Module<T>.CompletionSource. Still a reasonable follow-up to collapse into one cache keyed by (resultType, operation) — not a blocker.

Verdict

No new issues. The publish-timeout gap is closed correctly and is well tested. Approving.