Remove dead module awaiter cancellation

[thomhurst]

Closes #3795.

Removes the unused cancelModuleResultAwaiters branch, ModuleCompletionSourceCanceller reflection fallback, and generated-runtime cancellation hook. Scheduler completion is still cancelled; typed module results remain available for terminated-result registration.

Validation:

• focused cancellation tests: 7/7 passed
• lightweight core Release build: 0 warnings, 0 errors

[claude]

Review: Remove dead module awaiter cancellation

Solid, well-motivated cleanup — every production call site (DistributedModuleExecutor, ModuleExecutor x3) already passed cancelModuleResultAwaiters: false, so the true default and the ModuleCompletionSourceCanceller reflection fallback it drove were genuinely unreachable in real usage. Simplifying CancelPendingModules to a single behavior and deleting the reflection/expression-tree canceller is a good simplification — one fewer code path to reason about, no more IL3050/IL2075 AOT/trimming suppressions to maintain.

Blocking: compile break in ModularPipelines.Distributed.UnitTests

test/ModularPipelines.Distributed.UnitTests/Master/DistributedModuleExecutorTests.cs still calls the old bool-parameter overload in three places and was not updated by this PR:

• test/ModularPipelines.Distributed.UnitTests/Master/DistributedModuleExecutorTests.cs:195 — scheduler.Setup(s => s.CancelPendingModules(It.IsAny<bool>())).Returns([]);
• test/ModularPipelines.Distributed.UnitTests/Master/DistributedModuleExecutorTests.cs:901 — scheduler.Setup(s => s.CancelPendingModules(false))
• test/ModularPipelines.Distributed.UnitTests/Master/DistributedModuleExecutorTests.cs:957 — scheduler.Setup(s => s.CancelPendingModules(false))

Since IModuleScheduler.CancelPendingModules() (src/ModularPipelines/Engine/IModuleScheduler.cs:56) no longer accepts a bool, these Mock<IModuleScheduler> setups won't compile — there's no matching overload for the lambda to bind to. This project only builds as part of ModularPipelines.All.slnx, which explains why it wasn't caught by the PR's stated validation ("lightweight core Release build" + focused tests only cover ModularPipelines.UnitTests), but CI does build ModularPipelines.All.slnx and this will fail there.

Fix: drop the false / It.IsAny<bool>() argument in all three setups to match the new signature, same as the changes already made in test/ModularPipelines.UnitTests/Distributed/DistributedModuleExecutorTests.cs:33 and test/ModularPipelines.UnitTests/Engine/*.

Behavioral note (worth a sentence in test naming/PR description, not a blocker)

The rename of CancelPendingModules_CompletesPendingModuleAwaitable → CancelPendingModules_LeavesModuleResultForRegistrarCompletion (test/ModularPipelines.UnitTests/Engine/ModuleExecutorLoggingTests.cs:295) correctly reflects a real behavior change: previously the default (no-arg) call to CancelPendingModules would cancel the module's own typed CompletionSource immediately, completing ResultTask synchronously. Now ResultTask stays incomplete until RegisterTerminatedResultsForCancelledModules runs afterward. Since production code already always paired CancelPendingModules with a subsequent RegisterTerminatedResultsForCancelledModules call, this is safe — but it does mean any future direct caller of CancelPendingModules() who expects ResultTask to be settled immediately (matching the old default) will now get a module stuck pending until they remember to also call the registrar. Worth a one-line note on IModuleScheduler.CancelPendingModules (or IModuleStateTracker.CancelPendingModules) doc comments making that pairing an explicit contract, since the parameter that used to make this configurable is now gone.

Otherwise

Everything else in the diff is a mechanical signature simplification (removing the now-always-false parameter and its XML-doc <param> blocks) with matching test updates. No architectural concerns beyond the compile break above.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6368c53146

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

Update all scheduler mock call sites

Changing CancelPendingModules to take no arguments leaves the distributed unit-test project with calls to the removed overload: rg still finds CancelPendingModules(It.IsAny<bool>()) and CancelPendingModules(false) in test/ModularPipelines.Distributed.UnitTests/Master/DistributedModuleExecutorTests.cs. Because ModularPipelines.All.slnx includes that project, the full validation build will fail until those mocks are updated to the new signature.

Useful? React with 👍 / 👎.

[thomhurst]

Blocking compile finding fixed at b407153: all three distributed-test scheduler mocks now call the parameterless CancelPendingModules signature. Guarded Release build of ModularPipelines.Distributed.UnitTests succeeded with 0 warnings and 0 errors. Please re-review current head.

[claude]

Code review

Re-reviewed at head b407153 (branch issue-3795-remove-cancellation-dead-code).

The previously-flagged blocking compile break in test/ModularPipelines.Distributed.UnitTests/Master/DistributedModuleExecutorTests.cs (three Mock<IModuleScheduler> setups still calling the old bool-parameter CancelPendingModules(...) overload) has been fixed — all three now call the parameterless CancelPendingModules(), matching the new IModuleScheduler/IModuleStateTracker signature.

I re-ran a full independent pass (2 CLAUDE.md-compliance checks + 2 bug-focused checks) over the current diff:

• No CLAUDE.md violations. The diff stays within core-library scope (src/ModularPipelines/Engine, src/ModularPipelines/Distributed, and their matching unit-test projects), doesn't touch auto-generated options classes, and the PR's stated validation (lightweight core Release build + focused tests) matches the scoped build/test guidance rather than the full-solution build agents are told to avoid.
• No compile errors or logic bugs in the changed code. All interface declarations, implementations (including the WorkerModuleScheduler stub), call sites, XML doc comments, and Moq setups/verifies are consistently updated to the new zero-arg signature — no stale It.IsAny<bool>() or mismatched overloads remain. The removal is behavior-preserving in production since every real call site already passed cancelModuleResultAwaiters: false.
• The renamed test CancelPendingModules_LeavesModuleResultForRegistrarCompletion correctly reflects the behavior change: ResultTask now stays incomplete until RegisterTerminatedResultsForCancelledModules runs, instead of completing synchronously as before.

No issues found. Good cleanup — one fewer code path (and the IL3050/IL2075 AOT/trimming suppressions that came with it) to maintain.

[thomhurst]

Refreshed with current main at 7794402. Merge was clean. Post-refresh validation: changed cancellation regression 1/1 and ModularPipelines.Distributed.UnitTests Release build 0 warnings/errors. Please re-review current head.

[claude]

Code review

Re-checked at head 7794402b7c (branch issue-3795-remove-cancellation-dead-code).

This commit is a clean merge of main into the branch — git diff confirms it introduces no file changes beyond what was already reviewed at b407153976 (the fix for the previously-flagged blocking compile break in DistributedModuleExecutorTests.cs). No new commits, no conflict-resolution edits.

Since the prior review round already validated:

• No CLAUDE.md violations (diff stays within core-library scope, no auto-generated files touched).
• No compile errors or logic bugs — all CancelPendingModules call sites, interface signatures, and Moq setups consistently updated to the parameterless overload.
• The renamed test CancelPendingModules_LeavesModuleResultForRegistrarCompletion correctly reflects the intended behavior change.

No issues found. Nothing further to add since the last review — good to go from a code-review standpoint.

[thomhurst]

CI diagnosis: the sole Ubuntu failure is the shared core-test process leak, not this cancellation cleanup. The job ran RunCoreUnitTestsModule for 16m10s, exited code 2, and produced no failed assertion or hang dump; macOS and Windows passed. On exact head 7794402, RunHistoryOperationsTimeoutWhenStoreIgnoresCancellation alone hangs beyond 30s. Its pending mock tasks are cleaned up by the fix already pushed in #3829. Leaving this PR focused; refresh after that shared fix lands.

[thomhurst]

Refreshed onto current main in ad6aafd after required Ubuntu CI hit the known core-test host failure (no failed-test summary; exit 2/134 after 16m10s). Local validation: GeneratedModuleMetadataTests 8/8, ModuleExecutorLoggingTests 16/16, TaskCompletionSourceTests 9/9, distributed executor tests 27/27; core Release build 0 warnings/errors.

[claude]

Code review

Re-checked at current head ad6aafda (branch issue-3795-remove-cancellation-dead-code).

The two new commits since the last review are both Merge remote-tracking branch 'origin/main' merges (pulling in unrelated PRs #3870, #3868, #3865, #3853, #3840) — verified via git diff origin/main...<commit> --stat that this PR's own diff is byte-for-byte identical before and after these merges (same 14 files, +20/-115). No new changes to this PR's actual content.

Previous review cycle is still valid: the blocking compile break in DistributedModuleExecutorTests.cs (stale bool-parameter CancelPendingModules mock setups) was fixed and confirmed. No outstanding issues — the cleanup remains sound (dead cancelModuleResultAwaiters branch and reflection-based canceller removed, all call sites and tests consistently updated to the parameterless signature).

No issues found.