test(runtime): fail drain unit-test hangs instead of stalling PR CI

[sfmskywalker]

Purpose

Stop Elsa.Workflows.Runtime.UnitTests from hanging the Nuke pr workflow, and make any remaining hang fail in 10 minutes with a mini dump uploaded on failure.

Scope

Select one primary concern:

• Bug fix
• Refactor (no behavior change)
• Documentation update
• Formatting / code cleanup
• Dependency / build update
• New feature

This is test- and CI-only. No production behavior change.

Description

Problem

On release/3.9.0, Nuke ubuntu-latest hung twice in Elsa.Workflows.Runtime.UnitTests with no testhost output until cancel. Code Review reproduced it at 050e4458 and named the hung test:

BookmarkQueueAmbientTenantTests.RecurringTask_ExecuteAsync_SignalsCapturedTenant_WhenAmbientTenantIsMissing

TriggerBookmarkQueueRecurringTask.StartAsync started a live BookmarkQueueWorker that reads tenant-a's one-slot SingleReader channel. The test's await awaiter reads the same channel. ExecuteAsync writes exactly one signal, so when the worker wins the read the unbounded await waits forever (13/40 in CR's tight loop). Drain/execution-cycle frames were not in the hang dump.

Solution

• B1: Recurring-task test now uses a substitute IBookmarkQueueWorker so nothing else consumes the signal, and the await is bounded to 5s with a clear timeout. The notification-handler test in the same class is bounded the same way. BookmarkQueueTenantIsolationTests / distributed worker tests wait on processor call counts with a 3s WaitUntilAsync and do not have a competing test-side AwaitAsync.
• B2/B3: Shared CallbackGate records a gate timeout and AssertNotTimedOut() in finally, so a swallowed TryCancel exception still fails the test. One Timeout constant and shared ObserveCleanupAsync (TimeoutException + OCE).
• Mini hang dumps via Nuke --blame-hang-dump-type mini, uploaded from pr.yml / the Nuke GitHub Actions generator on failure.

Verification

Local loops at e423ca4d3:

• BookmarkQueueAmbientTenantTests: 40/40 passed (--blame-hang-timeout 2m)
• Full Elsa.Workflows.Runtime.UnitTests with coverlet + blame-hang: 10/10 passed, 435 tests, 15–16s each

Checklist

• The PR is focused on a single concern
• Commit messages follow the recommended convention
• Tests added or updated (if applicable)
• Documentation updated (if applicable)
• No unrelated cleanup included
• All tests pass

  

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 52dfb804-bfd6-4198-accf-7fee155dc5ba

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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

[sfmskywalker]

cursor review

[cursor]

Stale Bugbot comment from a previous run.

[sfmskywalker]

Elsa 3 Code Review: REQUEST_CHANGES + HIGH @ 050e445

Code Review, Round 1/4

Scope check: 3 files (+80/−39): build/Build.cs and two test files under Elsa.Workflows.Runtime.UnitTests/Quiescence. No production code changed. The branch is 1 commit ahead of release/3.9.0 and 2 behind; the 2 commits are a CI workflow-only change, and the branch is mergeable.

Summary

The blame-hang change is correct and valuable, and the bounded gates are an improvement. However, the hang still reproduces at this head, and it does not come from the tests this PR changes. As written, the PR would make the next hang fail after 10 minutes, but it would not stop it from happening.

Blocking

1. The real hang is in BookmarkQueueAmbientTenantTests, not in the drain/execution-cycle gates.

• I ran the full Elsa.Workflows.Runtime.UnitTests project 5 times at this head with --blame-hang --blame-hang-timeout 10m (net10.0, Release). Runs 1, 2, 4 and 5 passed (435 tests, about 20 s each). Run 3 hung: 434 tests passed, then blame fired after 10 minutes. It named Services.BookmarkQueueAmbientTenantTests.RecurringTask_ExecuteAsync_SignalsCapturedTenant_WhenAmbientTenantIsMissing.
• The hang dump agrees. The async stacks show that test suspended at await awaiter, its AwaitTenantAsync lambda waiting on signaler.AwaitAsync(), and BookmarkQueueWorker.AwaitSignalAsync waiting for its next signal. No Quiescence, Drain or ExecutionCycle frames appear, and no gate thread is blocked.
• Root cause (a race in the test):
  • TriggerBookmarkQueueRecurringTask.StartAsync calls worker.Start(), which runs the real AwaitSignalAsync loop under tenant-a.
  • The test's AwaitTenantAsync reads the same tenant-a channel. BookmarkQueueSignaler uses a bounded(1) SingleReader channel, and ExecuteAsync writes exactly one token.
  • When the worker wins that read, await awaiter never completes, because nothing bounds it.
• A tight-loop reproduction of the test body, with a 1 s bound on the awaiter, missed the signal 13 of 40 times.
• Suggested fix:
  • Don't let a live worker compete for the channel. Give TriggerBookmarkQueueRecurringTask a substitute IBookmarkQueueWorker (the test only asserts which tenant the trigger is written under), or record the tenant in a fake signaler's TriggerAsync.
  • Bound the await (await awaiter.WaitAsync(TimeSpan.FromSeconds(5))) so any future regression fails fast.
  • It would be worth a quick audit of the other AwaitTenantAsync call sites and of BookmarkQueueTenantIsolationTests for the same "worker plus test both read one tenant channel" pattern.
• Please also update the PR description. The claim that the only unbounded waits are the drain gates doesn't hold.

2. A gate timeout is swallowed, so on its own it cannot fail the test.

• WaitForRelease throws TimeoutException, but every cancelCallback gate runs inside ExecutionCycleHandle.TryCancel, which catches all non-fatal exceptions (catch (Exception ex) when (!ex.IsFatal())). The same applies to the inline copy in ExecutionCycleRegistryTests.
• The tests only fail on a missed release because their outer waits (5 s) are shorter than the gate's 10 s bound. I verified both directions with a deliberately broken gate:
  • release forgotten, existing 5 s outer wait: fails at 5 s, for both the registry test and the drain test ✓;
  • release forgotten, outer wait 20 s: passes after 10 s. The callback times out, TryCancel swallows the exception and returns false, and Assert.False(...) passes.
• That's latent today, but it's exactly the "timeout returns quietly" case. Make the gate record its own timeout and fail the test from the test thread. For example, give a small gate type a TimedOut flag (or a captured exception) and check it in the finally: Assert.False(gate.TimedOut, ...). Alternatively, document and centralise the invariant that the gate bound must exceed every outer wait.

3. Turn the duplicated gate code into one clear helper.

• WaitForRelease is a private helper in DrainOrchestratorWaitTests but is copied inline in ExecutionCycleRegistryTests.
• The cleanup observer is also duplicated, and the copies differ: ObserveCleanupAsync ignores TimeoutException and OperationCanceledException, while the inline copy ignores only TimeoutException.
• The 10 s and 5 s values are scattered magic numbers.
• A small shared test helper would fix this together with item 2, for example a CallbackGate with Entered, WaitForRelease(), Release() and AssertNotTimedOut(), plus a shared ObserveCleanupAsync and named timeouts. It would also make the try/finally blocks shorter and easier to read.

Verified OK

• (b) Nuke: build/_build.csproj pins Nuke.Components 10.1.0, which brings in Nuke.Common 10.1.0. That version has DotNetTestSettingsExtensions.EnableBlameHang<T>() and SetBlameHangTimeout<T>(string), and "10m" is a documented --blame-hang-timeout format. The CI log at this head shows --blame-hang --blame-hang-timeout 10m on every test project, and locally blame produced a hang dump and named the hung test exactly as intended.
• SnapshotQueueWaitDoesNotExcludeLaterFinds: completing hang in finally is good hygiene.
• (c) No production code changed.

Non-blocking

• The new comment on the snapshot test says the pending hang TCS can "keep testhost/coverlet alive". A pending task does not hold a thread, so consider rewording it to "so abandoned Finds complete deterministically".
• pr.yml doesn't upload test results, so a CI hang dump (about 480 MB, full dump) is discarded. The console line naming the hung test is the useful part. Consider .SetBlameHangDumpType("mini"), or uploading testresults/** on failure.
• The 10-minute per-test timeout applies to every test project, which seems fine for this repo.
• The PR template has "Bug fix (behavior change)" ticked, but this is a test and build-only change.

CI and bots @ head

• CI: ubuntu-latest (Nuke Compile + Test) ✅ green (03:54–04:07 CEST), with Elsa.Workflows.Runtime.UnitTests at 435 passed. GitGuardian and license/cla are green.
• Cursor Bugbot: no issues found at 050e4458.
• CodeRabbit: skipped because the PR is a draft. Copilot: no review. Greptile: no review or score yet.
• No review threads.

[cursor]

Addressing Code Review (round 1) at 050e445. Fixes are in e423ca4.

B1 — confirmed hang is BookmarkQueueAmbientTenantTests.RecurringTask_ExecuteAsync_SignalsCapturedTenant_WhenAmbientTenantIsMissing. Agreed. StartAsync was starting a live worker that raced the test for the bounded(1) SingleReader tenant-a channel. The test now passes a substitute IBookmarkQueueWorker so only the test awaiter reads the signal, and WaitForSignalAsync bounds that await to 5s with a timeout that names the tenant. The other AwaitTenantAsync site in this class (SignalBookmarkQueueWorker_CanReadAmbientTenant_WhenSaveNotificationIsHandled) has the same 5s bound; it never started a worker. BookmarkQueueWorker_CanReadAmbientTenant_WhenStarted only asserts CapturedTenantId and does not await a signal. BookmarkQueueTenantIsolationTests and DistributedBookmarkQueueWorkerTests start workers as the intended consumers and wait on processor call counts via a 3s WaitUntilAsync, so they do not have this “two readers, one write” hang.

B2 — gate timeout must fail the test by itself. CallbackGate.WaitForRelease still throws, but it also sets TimedOut. Each gated test calls AssertNotTimedOut() in finally, so a forgotten release fails even if TryCancel swallows the exception and an outer wait is longer than the gate.

B3 — deduplicate. WaitForRelease, ObserveCleanupAsync, the 5s/10s literals, and the recorded-timeout assert now live in test/unit/Elsa.Workflows.Runtime.UnitTests/Support/CallbackGate.cs (Timeout = 10s; observe handles both TimeoutException and OperationCanceledException).

Non-blocking

• Snapshot hang TCS comment now says abandoned Finds complete deterministically.
• Nuke uses --blame-hang-dump-type mini; pr.yml and Build.CI.GitHubActions.cs upload testresults/, *.dmp, and sequence XML on failure.
• PR body: “behavior change” unticked; description names the bookmark-queue test as the confirmed hang.

Verify: BookmarkQueueAmbientTenantTests 40/40 passed; full suite with coverlet + blame-hang 10/10 passed (435 tests, 15–16s).

[sfmskywalker]

cursor review

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit e423ca4. Configure here.}

[sfmskywalker]

Elsa 3 Code Review: APPROVE + HIGH @ e423ca4

Code Review, Round 2/4

All three blocking items from Round 1 are fixed, and I re-verified each one at this head. The scope is still test and build/CI only: 7 files (+218/−107) and no production code. The branch is 2 commits ahead of release/3.9.0 and 2 behind; the 2 commits only touch packages.yml, and the branch is mergeable.

Round 1 items

B1 — bookmark-queue signal race: fixed.

• RecurringTask_ExecuteAsync_SignalsCapturedTenant_WhenAmbientTenantIsMissing now passes Substitute.For<IBookmarkQueueWorker>(), so the test's awaiter is the only reader of the tenant-a channel. worker.Received(1).Start() keeps coverage that StartAsync starts the worker.
• Both AwaitTenantAsync sites now go through WaitForSignalAsync (5 s, with a timeout message that names the tenant).
• Real-worker tenant capture stays covered by BookmarkQueueWorker_CanReadAmbientTenant_WhenStarted.
• Verified: the BookmarkQueueAmbientTenantTests class passed 40/40 runs (each a separate dotnet test with blame-hang). The full project passed 10/10 runs with --blame-hang --blame-hang-timeout 10m --blame-hang-dump-type mini (435 tests, about 20 s each). Round 1 hung once in 5 runs.

B2 — gate timeout must fail the test by itself: fixed.

• CallbackGate.WaitForRelease records TimedOut before throwing, and every gated test calls AssertNotTimedOut() in finally after releasing and observing cleanup.
• Verified with a deliberately broken gate (a temporary local probe that wasn't committed). The callback runs under TryCancel, which swallows the exception:
  • release done (control): passes;
  • release forgotten, 10 s outer wait: fails with "Callback gate '…' timed out waiting for release.";
  • release forgotten, 20 s outer wait (passed silently in Round 1): now fails with the same message.

B3 — one clear helper: fixed.

• Support/CallbackGate.cs replaces both copies of the gate code. It has one Timeout constant, and ObserveCleanupAsync handles TimeoutException and OperationCanceledException the same way.
• All five gates in DrainOrchestratorWaitTests and ExecutionCycleRegistryTests use it, and each test's try/finally is now shorter and reads the same way.

Round 1 non-blocking items

• The snapshot-test comment is corrected.
• .SetBlameHangDumpType("mini") is in place. The CI log at head shows --blame-hang --blame-hang-timeout 10m --blame-hang-dump-type mini.
• Artifact upload:
  • pr.yml parses as valid YAML. The new step is if: failure(), actions/upload-artifact@v4, if-no-files-found: ignore.
  • On this green run, GitHub Actions accepted the step and skipped it, as intended.
  • I regenerated the workflow from Build.CI.GitHubActions.cs (--generate-configuration GitHubActions_pr) after stripping the step. The output is byte-identical to the committed pr.yml, so the generator and the YAML stay consistent.
  • The paths are right. Nuke passes --results-directory testresults, so blame dumps and Sequence XML land under testresults/<guid>/. The extra **/*.dmp and **/*Sequence*.xml globs are redundant but a harmless safety net.

Non-blocking notes (no change required to merge)

1. One name, two jobs. CallbackGate.Timeout is both how long a callback waits for release and the test-side wait for Entered and the drain/cancel tasks. That used to be 5 s and is now 10 s. It's safe now that AssertNotTimedOut exists. Splitting it into ReleaseTimeout and a test-side wait constant would make the intent obvious, and would avoid shadowing System.Threading.Timeout inside the class.

2. Repeated two-call pattern. Every callback body is gate.SignalEntered(); gate.WaitForRelease();. A single gate.Block() (or EnterAndWaitForRelease()) would remove that small repetition.

3. Visibility of TimedOut. It's written on the callback thread and read on the test thread. The read happens after ObserveCleanupAsync awaits the task, so in practice it's ordered. A Volatile.Write/Volatile.Read would make that explicit.

4. Follow-up idea: other unbounded test-side awaits. These remain in files this PR doesn't touch:

   • QuiescenceSignalPersistFailureTests:78 (await resumeTask);
   • WorkflowActivationGateTests:98/149 (evaluatorStarted, firstReady);
   • LocalWorkflowClientTests:348 (runnerStarted);
   • WorkflowCommitNotificationBufferTests:48/50.

   They wait on "started" signals that are set unconditionally, so they aren't races. They would only hang on a real regression, and blame-hang now caps that at 10 minutes with a named test. Adding .WaitAsync(...) would make them fail fast. The GetResult() at BookmarkQueueTenantIsolationTests:219 is a synchronous in-memory AddAsync, not a gate.

5. The PR is still a draft. Please mark it ready for review so CodeRabbit and Greptile can run.

CI and bots @ head

• CI: ubuntu-latest (Nuke Compile + Test) ✅ green (04:37–04:50 CEST), with Elsa.Workflows.Runtime.UnitTests at 435 passed. The upload step was skipped (success path). GitGuardian and license/cla are green.
• Cursor Bugbot: "no new issues" at e423ca4d.
• CodeRabbit: skipped because the PR is a draft. Greptile: no review or score yet. Copilot: no review.
• No review threads.

[greptile-apps]

Confidence Score: 4/5

[Low risk] Test infrastructure improvements for hang detection.

The missing test-results artifact is a non-blocking diagnostic gap; it does not make this change unsafe to merge.

Findings

1.  Ordinary failures lack results ▶

Fix with agent prompt

### Issue 1
.github/workflows/pr.yml:50-54
On an ordinary test failure, this upload can find no files: the PR test run does not request a TRX logger, and blame-hang produces no dump or sequence file when tests finish. With `if-no-files-found: ignore`, the failure remains in the job log but has no test-results artifact for later inspection. Configure result output for PR runs and keep the upload path in sync with the workflow generator. This is a non-blocking diagnostic gap.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

The PR bounds test synchronization waits and adds a failure-only upload for test results and hang dumps. An ordinary completed test failure can leave the upload with no matching files, so the job log records the failure but no test-results artifact is preserved.

_{Reviews (1) · Last reviewed commit: "test(runtime): stop bookmark-queue signa..."}

[greptile-apps]

Ordinary failures lack results

On an ordinary test failure, this upload can find no files: the PR test run does not request a TRX logger, and blame-hang produces no dump or sequence file when tests finish. With if-no-files-found: ignore, the failure remains in the job log but has no test-results artifact for later inspection. Configure result output for PR runs and keep the upload path in sync with the workflow generator. This is a non-blocking diagnostic gap.

Artifacts

Full source of the executed test-results reproduction

• The executed script creates one failing xUnit test and runs the workflow-like and TRX-enabled control commands, showing exactly how the local behavior was tested.

Workflow and build configuration inspection

• A captured Git diff and configuration search identifies the upload paths and conditional results-directory setting, showing the code paths behind the finding.

Successful restore of the temporary xUnit project

• The captured restore command exited 0 before either test run, showing the test project was ready to execute.

Ordinary failing test without uploadable result files

• The workflow-like command exited 1 on an assertion failure and found zero TRX, dump, or Sequence XML files, showing the missing-output case.

Same failing test with explicit TRX output

• The control command also exited 1 but wrote one TRX file under `testresults/`, showing that the configured upload path can contain the failure result.

Completed reproduction script invocation

• The captured script invocation exited 0 after verifying both expected test failures and the zero-versus-one result-file comparison.

Post-test cleanup and tracked-file status

• The captured cleanup and status check shows the control TRX was removed and no tracked files were changed.

_{Ran code and verified through T-Rex}

Prompt To Fix With AI

This is a comment left during a code review.
Path: .github/workflows/pr.yml
Line: 50-54

Comment:
**Ordinary failures lack results**

On an ordinary test failure, this upload can find no files: the PR test run does not request a TRX logger, and blame-hang produces no dump or sequence file when tests finish. With `if-no-files-found: ignore`, the failure remains in the job log but has no test-results artifact for later inspection. Configure result output for PR runs and keep the upload path in sync with the workflow generator. This is a non-blocking diagnostic gap.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!