Repository navigation
fix(#3593): a failed drain schedule must not latch the pump, and the verdict must measure its mechanism - #3798
Conversation
…verdict must measure its mechanism The pump verdict #3615 added still asserted the one thing that mattered: "the drain flag is latched (a drain IS scheduled on this hub's TaskScheduler)". Nothing measured that parenthesis. drainsInFlight == 0 says no drain body is RUNNING; it does not say one is queued. Two states read identically and have opposite owners: M1 the scheduler accepted a drain and has not run it → the scheduler M3 nothing is outstanding at all → this file M3 was REACHABLE. `draining = true` is set inside the turn gate in KickDrain and the schedule happens outside it; Terminal() re-schedules with the latch still held. A throw from Task.Factory.StartNew at either site left the flag set with nothing outstanding — and because KickDrain returns immediately whenever the flag is set, the pump was then frozen FOR THE LIFE OF THE HUB, silently, producing exactly #3593's fingerprint (queue non-empty, draining=true, drainsInFlight=0, nothing ever dequeued) while the verdict blamed a scheduler that had never been asked. Real schedulers throw there: a completed ConcurrentExclusiveSchedulerPair, or an Orleans activation torn down underneath the hub. ScheduleDrainOne now releases the latch and logs an Error naming the scheduler and the queue depth, so "draining ⇒ a drain is running or queued" holds by construction. 🚨 It does NOT fall back to another scheduler: the turn scheduler is the hub's serialisation guarantee (on a root grain hub it IS the activation scheduler), so running a turn elsewhere would break the actor model to keep a queue moving. drainsScheduled/drainsStarted are counted at the two ends of the hand-off and the verdict picks its MECHANISM: clause from them instead of asserting one. 🚨 This does NOT claim the production wedge is fixed, and the doc says so: StartNew on TaskScheduler.Default does not throw, and a hosted hub is built from a FRESH MessageHubConfiguration (CreateMessageHub) that inherits no scheduler — which is what WithTaskScheduler's own contract asks for. So the sync/* hubs in that incident run on the default pool scheduler, where M3 is unreachable and a dead activation scheduler cannot reach them either. That narrows the incident to M1-by-pool-starvation or M2 and leaves #3593 open; what changes is that the next occurrence prints which. Falsified: with the latch release removed, ASchedulerThatRefusesADrain_DoesNotLeaveThePumpLatched fails — the post after the scheduler recovers is never dequeued. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Approval recommended
The behavioral changes are covered by targeted tests and documentation, with only a minor in-code comment correction suggested.
Pull request overview
This PR addresses a hub message-pump edge case where a failed drain scheduling attempt could leave the pump latched (draining=true) with no drain running or queued, permanently preventing future dequeues; it also strengthens the disposal-stall verdict so it reports the measured mechanism (scheduler accepted-but-didn’t-run vs. pump latched-with-nothing-outstanding).
Changes:
- Make
MessageService.ScheduleDrainOnerelease the drain latch (and log) whenTask.Factory.StartNew(..., turnScheduler)throws, preventing a permanent pump freeze. - Add drain hand-off counters (
drainsScheduled,drainsStarted) and plumbdrainsAwaitingSchedulerthrough snapshots soMessageHubcan select a “MECHANISM:” clause based on measurements. - Extend/introduce tests and documentation to cover the new mechanism discrimination and the latch-release behavior.
File summaries
| File | Description |
|---|---|
| test/MeshWeaver.Messaging.Hub.Test/DisposalStallNamesThePumpTest.cs | Extends the existing pump-verdict assertions and adds a new regression test ensuring a refused schedule does not leave the pump latched. |
| src/MeshWeaver.Messaging.Hub/MessageService.cs | Adds drain hand-off counters and makes ScheduleDrainOne release the latch + log when scheduling throws. |
| src/MeshWeaver.Messaging.Hub/MessageHub.cs | Updates disposal stall verdict wording and mechanism selection to use drainsAwaitingScheduler as a measurement. |
| src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-09-a-latched-pump-that-scheduled-nothing.md | Adds a What’s New entry describing the latch-leak fix and improved mechanism reporting. |
| src/MeshWeaver.Documentation/Data/Architecture/DisposalStallVerdicts.md | Updates the architecture doc to include the new M3 mechanism and the new measured discriminator fields. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // grain's), the second would be a defect in THIS file. Monotonic; the difference is the number | ||
| // of drains queued on the turn scheduler that have not begun. |
Test Results (shard 1)496 tests 496 ✅ 47s ⏱️ Results for commit 07c5ace. |
Test Results (shard 0)254 tests 254 ✅ 2m 15s ⏱️ Results for commit 07c5ace. |
Test Results (shard 3)389 tests 389 ✅ 50s ⏱️ Results for commit 07c5ace. |
Test Results (shard 4)1 861 tests 1 861 ✅ 2m 37s ⏱️ Results for commit 07c5ace. |
Test Results (shard 2)1 142 tests 1 140 ✅ 3m 36s ⏱️ Results for commit 07c5ace. |
Test Results (shard 5) 6 files 6 suites 4m 38s ⏱️ Results for commit 07c5ace. |
Test Results 17 files 17 suites 14m 44s ⏱️ Results for commit 07c5ace. |
Refs #3593. 🚨 This does not claim the production wedge is fixed — see What this is not at the
end. It closes a third mechanism nobody had listed, and makes the remaining two tell themselves
apart.
The verdict asserted the one thing that mattered
#3615 made the stall report honest about three fields that were not measurements. One assertion
survived it. For the pump branch the line reads:
Nothing measured the parenthesis.
drainsInFlight == 0says no drain body is running; it doesnot say one is queued. Two states read identically and have opposite owners:
drainsInFlightdrainsScheduled − drainsStartedTaskSchedulerMessageService— this fileM3 was reachable, and it froze a hub for its whole life
draining = trueis set inside the turn gate inKickDrain; the schedule happens outside it,and
Terminal()re-schedules with the latch still held. A throw fromTask.Factory.StartNewateither site left the flag set with nothing outstanding — and since
KickDrainreturns immediatelywhenever the flag is set, the pump was then frozen for the life of the hub, silently, producing
exactly this issue's fingerprint (queue non-empty,
draining=true,drainsInFlight=0, nothing everdequeued) with the verdict blaming a scheduler that had never been asked.
A real scheduler throws there: a completed
ConcurrentExclusiveSchedulerPair, or an Orleansactivation torn down underneath the hub.
ScheduleDrainOnenow releases the latch and logs an Error naming the scheduler and the queuedepth, so the invariant "
draining⇒ a drain is running or queued" holds by construction andevery verdict that reads it is entitled to.
🚨 It does not fall back to another scheduler. The turn scheduler is the hub's serialisation
guarantee — on a root grain hub it is the activation scheduler — so running a turn elsewhere would
break the actor model to keep a queue moving. Releasing the latch is the honest recovery: the next
post tries again and reports again, instead of the pump going dark.
And the verdict now measures its mechanism
drainsScheduled/drainsStartedare counted at the two ends of the hand-off; the snapshotcarries the difference; the line picks its
MECHANISM:clause from the counters instead ofasserting one:
drainsInFlight > 0→ M2: a drain body is running and blocked before the dequeue.drainsAwaitingScheduler > 0→ M1: the scheduler accepted work and is not running it. OnTaskScheduler.Defaultthat is pool starvation or a local-LIFO stall; on a root grain hub itis a wedged or deactivated
ActivationTaskScheduler.ScheduleDrainOne, which the change above makes unreachable.Tests
DisposalStallNamesThePumpTest— the suite that already builds this exact state:drainsAwaitingScheduler=1, thethe turn scheduler ACCEPTEDmechanism clause, and
NotContain("defect in MessageService.ScheduleDrainOne")— the pump didits job there, and blaming it for a scheduler that will not run work is the same misattribution as
blaming a child;
ASchedulerThatRefusesADrain_DoesNotLeaveThePumpLatched(new) — a scheduler that throws onqueue, then accepts again; the hub must answer the next request. It asserts the refusal actually
happened first, so it cannot pass over a scheduler that quietly worked.
Falsified: with the latch release removed, the new test fails — the post after recovery is never
dequeued and the request times out. Both pass with it. No
SemaphoreSlim, noTask.Delay, nobridge: both fake schedulers are plain
TaskSchedulersubclasses over aLock.Verification
dotnet build -c Release -warnaserror, one project per invocation:MeshWeaver.Messaging.Hub,MeshWeaver.Documentation,MeshWeaver.Messaging.Hub.Test,MeshWeaver.Documentation.Test—0 Warning(s), 0 Error(s) each. Tests below.
Docs
Doc/Architecture/DisposalStallVerdicts— M3 added to the mechanism table, the sample verdictupdated, and the Orleans clause corrected (see below).
Category: Fix).🚨 What this is not
It is almost certainly not what the 47 production reports were, and the doc says so.
Task.Factory.StartNewonTaskScheduler.Defaultdoes not throw, and a hosted hub is built from afresh
MessageHubConfiguration(IServiceProvider.CreateMessageHub) — nothing copies aparent's
TaskScheduler, which is exactly whatWithTaskScheduler's own contract asks for ("usethe grain's scheduler ONLY for the root grain hub"). So the
sync/*hubs in that incident run onthe default pool scheduler, where M3 is unreachable and a dead activation scheduler cannot reach
them either.
That narrows the candidate set for the incident to M1-by-pool-starvation or M2 and leaves #3593
open — the same call #3615 made. What changes is that the next occurrence prints which one it is,
and that a real latch leak with the identical fingerprint can no longer be one of them.