test: separate cache overlap deadline from executor shutdown

[thomhurst]

Problem

The Linux run for #5181 failed Cache_Lookups_For_Ready_Modules_Run_Concurrently after both lookups started and the held lookup resumed within 13.4 ms. The failure was awaiting executor completion, after the overlap assertion had passed. The existing five-second test timeout also covered worker and scheduler shutdown, including thread-pool work queued by CancellationTokenSource.CancelAsync.

Change

Keep a separate five-second cancellation deadline for observing both lookups before releasing the first. Give executor completion an explicit 30-second deadline after overlap succeeds, with a 70-second outer guard covering five seconds for overlap, 30 seconds for completion, and 30 seconds for cleanup plus margin. The overlap clock starts after fixture setup. Keep all 50 repetitions and both restored-cache completion assertions. Record module completion, scheduler cancellation, thread-pool state on failure, and cleanup completion so any recurrence identifies the stalled phase. This does not change production scheduling or claim that a production deadlock has been reproduced.

Validation

• All 254 distributed tests pass with coverage, including the 50 concurrency repetitions.
• Negative control: temporarily serialized PublishReadyModulesAsync; all 50 repetitions failed at the retained five-second overlap deadline. Restored production source before final validation.
• Scoped formatting of ModularPipelines.Tests.slnf completed; its existing F# project is unsupported by dotnet format.
• Await full Linux CI before treating the recurrence as resolved.

Part of #5101. Unblocks #5181.

Failure evidence: https://github.com/thomhurst/ModularPipelines/actions/runs/35101343839/job/104813028062

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-16T14:33:51.146765Z	b9448fd	New commits

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[coderabbitai]

Warning

Review limit reached

Next included review available in 22 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 10a12315-2cc5-4a21-8f68-fe5203e51a8e

📥 Commits

Reviewing files that changed from the base of the PR and between c7b9a87 and b9448fd.

📒 Files selected for processing (1)

• test/ModularPipelines.Distributed.UnitTests/Master/DistributedModuleExecutorTests.cs

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 932a1256-0596-4377-9c42-0959fd291212

📥 Commits

Reviewing files that changed from the base of the PR and between a6d1b84 and c7b9a87.

📒 Files selected for processing (1)

• test/ModularPipelines.Distributed.UnitTests/Master/DistributedModuleExecutorTests.cs

Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review.

---

📝 Walkthrough

Walkthrough

The cache-concurrency test now separates lookup-overlap timing from execution shutdown. It tracks scheduler progress, supports cancellation, and records thread-pool and cleanup diagnostics.

Changes

Distributed cache concurrency test

Layer / File(s)	Summary
Concurrency timing and cleanup diagnostics test/ModularPipelines.Distributed.UnitTests/Master/DistributedModuleExecutorTests.cs	The test uses a 30-second overall timeout and a separate five-second lookup-overlap deadline. The scheduler records completion and cancellation, returns a cancellable task, and reports execution and thread-pool status on failure. Cleanup records execution-task draining.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Other

Merge Risk: ⚪ Minimal · up to c7b9a

This test-only change separates overlap detection from shutdown timing and adds diagnostics without changing production behavior, so it is ready to merge.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: separating the cache-overlap deadline from executor shutdown handling in the concurrency test.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch issue-5101-cache-completion

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit checks the cache in flight
Two lookups overlap just right
The scheduler signals done
Cancellation leaves none
Threads report their work with care
Cleanup finds an empty lair}

_{Comment @coderabbitai help to get the list of available commands.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c7b9a874cb

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

@coderabbitai The docstring-coverage warning concerns a test method and its local diagnostics function. This follows the surrounding test style; the ordering and shutdown rationale are documented beside the assertions. Treating this metric as advisory, consistent with your disposition on #5181.

[github-actions]

Review

Scope: this is a test-only change to Cache_Lookups_For_Ready_Modules_Run_Concurrently in DistributedModuleExecutorTests.cs, splitting the single 5s timeout into a dedicated 5s overlap-detection deadline plus the standard 30s budget for execution shutdown, and adding progress/diagnostic recording on failure. No production code paths are touched. The overall approach (separate the fast assertion window from the slower drain/shutdown window, and record diagnostics on the failure path) is a sound fix for the flakiness this addresses.

Finding: outer [Timeout(40_000)] leaves too little margin over the inner budgets it's supposed to defer to

The method-level [Timeout(40_000)] races independently against the test body. The try block's own explicit budgets already sum to 35s (5s overlap deadline + 30s execution.WaitAsync(TestHostSettings.DefaultTestTimeout, ...)), leaving only ~5s of slack. If the catch block's diagnostics fire (i.e. execution genuinely times out) the finally block adds another execution.WaitAsync(TestHostSettings.DefaultTestTimeout, CancellationToken.None) — up to 30 more seconds — but that wait uses CancellationToken.None, so it doesn't observe the outer attribute's cancellation at all.

Under the CI parallel load this PR exists to survive ([Repeat(49)], 50 concurrent repetitions per the linked #5181 failure), if the overlap phase and the shutdown/drain phase both run close to their budgets, total elapsed time can exceed 40s before the intended inner TimeoutException is thrown and caught. In that case TUnit's outer attribute-level timeout wins the race instead, cancelling the test via cancellationToken before the catch block's diagnostic Record(...) (thread-pool/execution status) ever runs — which defeats the specific goal of this PR (getting diagnostic detail on which phase stalled).

Suggested fix: give the outer attribute enough margin to always be the last resort, e.g. [Timeout(70_000)] (comfortably above 5s + 30s + the finally's potential extra 30s), or restructure so the finally's cleanup wait also has a bounded ceiling that's accounted for in the outer timeout. The important invariant is: the outer coarse timeout should never race the inner explicit ones — it should only trip if the test hangs somewhere not already covered by an explicit await, otherwise you lose the very diagnostics this PR adds.

Minor/secondary note: the new overlapDeadline (CancelAfter(TimeSpan.FromSeconds(5)), line 753) is armed before fixture setup (mock/executor construction) runs, so setup latency is drawn from the same 5s window meant for detecting concurrent overlap. This mirrors the previous single-timeout behavior, so it isn't a regression, but worth keeping in mind if overlap-detection flakiness resurfaces.

No issues found with the Moq setup for MarkModuleCompleted/RunSchedulerAsync, the cancellation-aware scheduler stub, or the additional diagnostic Record calls — these are correct and match IModuleScheduler's current signature.

[github-actions]

Review

Scope: test-only change to Cache_Lookups_For_Ready_Modules_Run_Concurrently in DistributedModuleExecutorTests.cs. No production code is touched.

Previous finding addressed

A prior review on this PR flagged that the method-level [Timeout(40_000)] left too little margin over the test body's own explicit budgets (5s overlap + 30s completion + up to 30s finally-block cleanup ≈ 65s worst case), risking the outer attribute-level timeout winning the race and suppressing the new diagnostic Record(...) calls this PR exists to add. The current head fixes this exactly as suggested: [Timeout(70_000)] (line 743), with a comment documenting the 5s/30s/30s budget breakdown. The prior review's secondary/minor note — that overlapDeadline.CancelAfter(...) was armed before fixture setup, drawing setup latency from the 5s overlap window — is also resolved: CancelAfter(TimeSpan.FromSeconds(5)) is now called immediately before executor.ExecuteAsync(...), after all fixture/mock construction, matching the PR description ("the overlap clock starts after fixture setup").

Current state

• overlapDeadline (linked to the test's cancellationToken) correctly bounds only the firstStarted/secondStarted overlap-detection waits to 5s, preserving the original fast-fail behavior for the ordering assertion.
• execution.WaitAsync(TestHostSettings.DefaultTestTimeout, cancellationToken) gives the post-overlap completion its own explicit 30s budget, matching TestHostSettings.DefaultTestTimeout.
• The added MarkModuleCompleted/RunSchedulerAsync Moq setups correctly override CreateMockScheduler's defaults for this test only, layering in Record(...) diagnostics (module completion, scheduler cancellation) without changing behavior — signatures match IModuleScheduler.
• The catch-block diagnostic (execution.Status, ThreadPool.ThreadCount, ThreadPool.PendingWorkItemCount) and the new "cleanup completed" record give a clear phase-by-phase trail if this recurs, which is the stated goal.
• No production scheduling code is touched, consistent with the PR's stated scope.

No actionable issues found. This is a solid, well-targeted fix to test flakiness with good diagnostic instrumentation for any recurrence.

[thomhurst]

Addressed the blocking finding in #5182 (comment) with b9448fd, confirmed in the remote PR head. The outer guard is now 70 seconds, covering the five-second overlap deadline, 30-second completion deadline, and 30-second cleanup deadline plus margin. The overlap clock now starts after fixture setup. All 50 focused repetitions pass; scoped formatting passes. Please re-review the updated head.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@thomhurst Thanks for the clarification. The test method and its local diagnostics function follow the surrounding test style. The nearby assertions document the ordering and shutdown rationale. I will treat docstring coverage for these test-local symbols as advisory here.

---

✏️ Learnings added

Learnt from: thomhurst
URL: https://github.com/thomhurst/ModularPipelines/pull/5182

Timestamp: 2026-09-16T14:36:59.341Z
Learning: In `test/ModularPipelines.Distributed.UnitTests`, test methods and test-local diagnostic functions can follow the surrounding style without XML docstrings when nearby assertions document the required ordering and shutdown rationale. Treat docstring-coverage warnings for these test-local symbols as advisory.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

You are interacting with an AI system.