Bound AlwaysRun scheduler progress waits

[thomhurst]

Closes #3794

Summary

• add a dedicated AlwaysRunProgressTimeout with a 30-second default
• keep AlwaysRun teardown bounded when DefaultModuleTimeout is disabled
• validate negative values and document independent configuration

Validation

• AlwaysRunHandlerTests: 8/8 passed
• new option validation/default tests: 2/2 passed
• lightweight core Release build: 0 warnings, 0 errors

[claude]

Code review

Nice, tightly-scoped fix. Decoupling the AlwaysRun scheduler-progress watchdog from DefaultModuleTimeout into its own AlwaysRunProgressTimeout (default 30s) directly addresses #3794 — a disabled DefaultModuleTimeout no longer leaves deferred AlwaysRun modules waiting forever. Validation, XML docs, and the timeouts guide were all updated consistently with the existing DefaultModuleTimeout pattern.

One thing worth tightening:

Test no longer exercises what its name/setup claims — AlwaysRunHandlerTests.cs#L360-L365

WaitForAlwaysRunModulesAsync_TimesOutWhenModuleTimeoutsAreDisabled was renamed from ...WhenSchedulerCannotMakeProgress and now passes defaultModuleTimeout: TimeSpan.Zero via the CreateHandler helper (CreateHandler, L461-L482). But AlwaysRunHandler no longer reads DefaultModuleTimeout at all — it only reads AlwaysRunProgressTimeout (AlwaysRunHandler.cs line 24). So defaultModuleTimeout is dead weight here: the test would time out identically whether that value is Zero or TimeSpan.FromMinutes(30), since it's the default AlwaysRunProgressTimeout (30s, unset in this call) driving the timeout, not DefaultModuleTimeout.

The rename makes it read as "this verifies the watchdog still fires when module timeouts are disabled," but the code path being asserted has already been fully decoupled from that setting by this same PR. That's a bit self-contradicting and could mislead a future reader into thinking DefaultModuleTimeout still matters to AlwaysRunHandler.

Suggest either:

• Drop the now-unused defaultModuleTimeout parameter from CreateHandler and this call site (nothing else in the diff needs it), or
• If you want a regression test that documents "disabling DefaultModuleTimeout no longer causes indefinite AlwaysRun waits," write it at a level that actually wires both options together (e.g. through PipelineOptions/builder) so the assertion is coupled to real behavior rather than an inert constructor argument.

Everything else looks solid — thanks for the focused change and good test coverage on the new option's validation/default.

[thomhurst]

Review feedback addressed at e6b3cf9. The helper now accepts the complete PipelineOptions object, so the regression explicitly wires DefaultModuleTimeout = TimeSpan.Zero while leaving the dedicated 30-second AlwaysRunProgressTimeout at its real default. The focused regression passes. Please re-review current head.

[claude]

Code review

Re-reviewed at e6b3cf9d. The previous feedback is fully addressed: CreateHandler now takes a full PipelineOptions object instead of a single TimeSpan?, and WaitForAlwaysRunModulesAsync_UsesDedicatedProgressTimeoutWhenModuleTimeoutsAreDisabled (renamed from the earlier ...ModuleTimeoutsAreDisabled variant) now sets DefaultModuleTimeout = TimeSpan.Zero explicitly while relying on the real default AlwaysRunProgressTimeout (30s) and advancing the fake time provider by the matching 30s. The assertion is now coupled to actual behavior rather than an inert constructor parameter — exactly what was suggested.

No other references to the old DefaultModuleTimeout-driven watchdog remain (checked AlwaysRunHandler.cs, PipelineOptions.cs, OptionsValidator.cs, docs, and all test files) — the decoupling is consistent throughout.

No further issues found. Nice, focused fix.

[thomhurst]

Refreshed onto current main after the prior Ubuntu full-suite run ended without a failing-test summary (test host exit 2, workflow exit 134). Focused validation on 4e66447: AlwaysRunHandlerTests 8/8, ModuleTimeoutTests 13/13, ValidationTests 40/40, and ModularPipelines.slnx Release build succeeded with 0 warnings/errors.

[claude]

Code review

Re-reviewed at 4e664478 (a merge from main, no changes to the touched files since e6b3cf9d). Diffed e6b3cf9d → 4e664478 for every file in this PR and confirmed it's a no-op merge — same content already reviewed and approved.

Recap of the fix: AlwaysRunHandler now reads a dedicated PipelineOptions.AlwaysRunProgressTimeout (default 30s) instead of DefaultModuleTimeout for its scheduler-progress watchdog, so setting DefaultModuleTimeout = TimeSpan.Zero to disable module timeouts no longer silently disables the AlwaysRun teardown watchdog too — directly fixes #3794. The new option follows the existing DefaultModuleTimeout convention exactly: same XML-doc phrasing, same TimeSpan.Zero-to-disable semantics, same negative-value validation in OptionsValidator, and the docs page was updated to match.

The earlier round's feedback (test WaitForAlwaysRunModulesAsync_UsesDedicatedProgressTimeoutWhenModuleTimeoutsAreDisabled asserting against an option the code no longer reads) was fixed by passing the full PipelineOptions into CreateHandler and explicitly setting DefaultModuleTimeout = TimeSpan.Zero while relying on the real AlwaysRunProgressTimeout default — the regression test is now coupled to actual behavior.

No further issues found — this is a small, well-scoped decoupling with matching test, validation, and doc coverage.

[thomhurst]

Refreshed onto current main in 68627a5 after required Ubuntu CI again hit the known core-test host failure (no failed-test summary; exit 2/134 after 15m51s). Local validation: AlwaysRunHandlerTests 8/8, ModuleTimeoutTests 13/13, ValidationTests 40/40; core Release build 0 warnings/errors.

[claude]

Code review

No new issues found. This PR has already been reviewed across three prior rounds (472eeb1 → e6b3cf9 → 4e66447), with each round of feedback fully addressed:

• The dedicated AlwaysRunProgressTimeout (default 30s) correctly decouples the AlwaysRun scheduler-progress watchdog from DefaultModuleTimeout, fixing Engine: AlwaysRunHandler reuses DefaultModuleTimeout as its scheduler-progress timeout — waits forever when the default timeout is disabled #3794 without affecting existing DefaultModuleTimeout semantics.
• Validation (negative-value rejection), XML docs, and the timeouts guide all follow the existing DefaultModuleTimeout convention consistently.
• The earlier test-coupling concern (CreateHandler taking an inert TimeSpan? instead of full PipelineOptions) was resolved — the regression test now explicitly sets DefaultModuleTimeout = TimeSpan.Zero while relying on the real AlwaysRunProgressTimeout default, so the assertion is coupled to actual behavior.

The current head (68627a5) only merges in an unrelated upstream commit (#3870) — I diffed 4e66447 → 68627a5 across every file this PR touches and confirmed zero changes. No further action needed here.