Decouple the assignment evaluation from the command drain, with pending assignment as grid state (GH-3698) - #3719
Merged
Merged
Conversation
… time (GH-3698) The leader's agent command drain executed one command at a time across the whole cluster. Combined with NodeAgentController.batchCommands, which groups a wave's AssignAgent commands by destination and appends each destination's chunks contiguously, that made the drain destination-at-a-time: every chunk for node A was awaited to completion before node B was sent its first. Cluster-wide agent start concurrency was therefore capped at a single node's MaxAgentStartParallelism no matter how many nodes were in the cluster, so adding nodes did nothing for convergence time. On a cluster of ~5,240 slow-starting Marten subscription agents that is hours of one node working while its peers sit idle. And because executeHealthChecks awaits the drain before the next EvaluateAssignmentsAsync, the leader also stopped writing AssignmentChanged records for the whole duration -- which is why the reported cluster looked stalled rather than merely slow, with no errors and every node healthy. IAgentCommand gains a `Guid? DestinationNodeId` default interface member, so existing and third-party commands stay source-compatible. The new AgentCommandDrain partitions each round into one lane per destination: serial within a lane (one chunk in flight per node, which is what the WO-5 pending-assignment ledger assumes), concurrent across lanes. Commands naming no single destination share one strictly-serial lane, exactly as before. Cascades are deferred to the next round rather than appended to the producing lane, so "a cascade runs after its producer" still holds while being re-keyed onto the right lane. A failing command no longer discards the rest of the wave -- previously any exception, most realistically a TimeoutException from an AssignAgents chunk that outran its reply window, unwound out of the drain and dropped every command still queued for every other node. Failures are returned rather than thrown so ApplyRestrictionsAsync, which answers to an operator, still surfaces them as an AggregateException once the whole wave has run instead of silently reporting success. Measured on the new SlowTests repro (900 agents, 3 nodes, 1s per agent start, shipped defaults): peak cluster-wide concurrent starts 10/30 -> 30/30, time to converge 125.5s -> 57.2s, and all three nodes now gain agents in lockstep instead of one at a time. Still open, tracked separately: the drain continues to block the assignment evaluation itself, so the leader's AssignmentChanged gap shrinks from 103s to 35s but does not go away. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
) Every batched agent command carries a Uri[], and the record default compares those arrays by reference -- so two commands naming exactly the same agents were never equal. StartAgents and StopAgents had hand-written SequenceEqual overrides for that reason, but both left GetHashCode returning the array's reference hash: a hash that disagrees with equality means the two values never land in the same bucket and are never compared at all, so those overrides did nothing in any hash-based lookup. AssignAgents, StopRemoteAgents, AgentsStarted and AgentsStopped had no override at all. All six now share AgentUriSet, which compares by the actual Uri values and is deliberately order-independent: nothing about a batch depends on the order its URIs happen to be in, and two assignment waves can chunk the same set of agents in a different order. It is a multiset comparison, so a repeated URI still has to be matched by the same number of repeats rather than collapsing away, and the hash is summed rather than order-sensitively combined so any ordering of the same agents hashes alike. This matters for anything that uses equality to recognise the same work twice -- notably the pending-command suppression in the agent command pump. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
GH-3698) NOT FINISHED -- SlowTests/Agents/agent_assignment_at_scale.cs currently FAILS on this commit. Pushed as part of a draft PR for review, see below. executeHealthChecks used to await DoHealthChecksAsync() and then the whole agent command drain in the same loop body, so for as long as a wave of agent starts was in flight, EvaluateAssignmentsAsync did not run at all -- and since the observer early-returns on an empty command list, the leader wrote ZERO AssignmentChanged records the entire time. That is why the reported cluster looked stalled and idle rather than merely slow. Same defect as GH-3604/D1, which decoupled the heartbeat; this decouples the other half. The health-check loop now writes commands to a channel and ticks on; a drainAgentCommands pump executes them. A _queuedCommands set suppresses enqueuing a command equal to one already queued or in flight, so a wave re-emitted after the pending-assignment ledger's TTL is not issued against itself. Measured on the repro: longest leader evaluation gap 103s -> 1.1s, and the leader keeps writing AssignmentChanged throughout a wave. Peak cluster-wide start concurrency still 30/30. WHY IT IS NOT DONE: convergence regressed 57.2s -> 83.6s. Once evaluation runs during a wave, the distribution pass re-decides placement for every agent that has been dispatched but has not started yet -- Agent.OriginalNode comes from persisted ActiveAgents, so an in-flight agent looks entirely unassigned. pinPendingAssignments holds such an agent on the node it was already dispatched to, which stopped the re-emission flood, but it runs AFTER the families distribute: the distribution balances all three nodes evenly and the pin then moves agents back, leaving one node over its share. The observed tail is one node finishing ~300 agents alone while the other two sit idle. Next step is to pin BEFORE the family distribution, which needs care in DistributeEvenly / DistributeEvenlyWithBlueGreenSemantics -- both open by detaching everything a node holds beyond `maximum`, which would undo a pin. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…a wave drains (GH-3698) Completes the previous commit, which is now green: agent_assignment_at_scale converges in 69.4s with a longest leader evaluation gap of 1.0s. Once the assignment evaluation no longer waits for the command drain, an evaluation lands every HealthCheckPollingTime while a wave of slow agent starts is still in flight -- and the distribution pass re-decides placement for every agent that has been dispatched but has not started yet, because Agent.OriginalNode comes from each node's PERSISTED ActiveAgents and the assignment row only appears once the agent is actually running. A re-targeted agent defeats the pending-assignment ledger, which is keyed on (agent -> node): the suppression stops working, every pass re-emits the whole agent universe, and the AssignmentChanged flood of GH-3604/D6 comes straight back. Worse, the agent ends up dispatched to two nodes at once. pinPendingAssignments holds such an agent on the node it was already dispatched to. It runs AFTER restrictions -- an operator's pause or explicit pin outranks a dispatch we have not managed to complete, and re-placing a paused agent here would resurrect the GH-3663 class of bug -- and BEFORE the family distributes, which is the part that matters: the distribution then sees those agents as already placed, counts them toward their node's share, and balances the genuinely unassigned remainder around them. Pinning after the fact instead leaves the distribution's own even split intact and moves agents on top of it, which overloads whichever node the pins land on (that cost 83.6s vs 69.4s). Deliberately does not touch Agent.OriginalNode: the ledger confirms entries by matching OriginalNode, so fabricating one here would make every pending entry instantly self-confirming. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ommand (GH-3698) Command-level suppression in the agent command pump was not enough. The pending-assignment ledger releases an agent after its TTL, the next evaluation re-emits the still-unstarted remainder, and batchCommands re-chunks it -- so the new chunks carry DIFFERENT membership than the ones the pump is still working through (900 agents chunk into 18 batches of 50; the 650 not yet started chunk into 13). No command-level comparison can recognise those as the same work, so agents were started a second time while their first start was still in flight. Suppress at the agent level instead: an incoming batch is narrowed to the agents nothing already has queued or in flight, and dropped outright if that leaves nothing. Deliberately only for first-time placements -- AssignAgent and AssignAgents. A ReassignAgent also stops the agent on its previous node, so suppressing one because a start is pending would drop that stop, and the stop commands are not starts at all. Measured on the repro, all four runs on the same freshly created Postgres container: before: peak cluster-wide concurrency 20 of 30, converged 89.3s / 90.4s after: peak cluster-wide concurrency 30 of 30, converged 71.3s / 75.2s / 76.3s The concurrency figure is the interesting one: redundant re-chunked work was crowding a destination out of the pump's batch, so a batch often covered only two of the three nodes. Also recalibrates the convergence budget in agent_assignment_at_scale from 75s to 100s. The old value was derived from start work alone (~52s) and ignored that one chunk is in flight per destination at a time, so each lane additionally pays a control-queue request/reply round-trip per chunk -- six per node here, over the POLLED dbcontrol transport. That is ~20s of latency rather than wasted work, and it shrinks with a larger AgentStartBatchSize, which is what the reporter observed helping. The pre-fix behaviour was 125s so the assertion still catches the defect, and the peak-concurrency and evaluation-gap assertions are the precise ones. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jeremydmiller
marked this pull request as ready for review
July 29, 2026 20:39
jeremydmiller
marked this pull request as draft
July 29, 2026 20:45
…ispatcher (GH-3698) NOT SUFFICIENT ON ITS OWN. The RavenDb leadership-election suite is still red on this branch; see below. Kept because the work and its tests are correct as far as they go, and the eventual fix builds on them. Replaces the one-wave-at-a-time pump with AgentCommandDispatcher: a long-lived lane (channel + worker) per destination node, serial within a lane, concurrent across lanes, cascades routed back through the dispatcher so a ReassignAgent's cascaded AssignAgent lands in the lane of the node it targets. This removes the head-of-line blocking the pump had -- a wave holding an AssignAgents aimed at a just-removed node blocked on its 30s+chunkSize reply window while the re-targets to surviving nodes queued behind it and never ran. Also: in-flight suppression is keyed on (agent uri -> destination) rather than the agent alone. A re-target is a different instruction, not a duplicate of the copy in flight, and keying on the agent alone stranded it for as long as the doomed copy took to time out. Both of those were real defects and both are fixed here. NEITHER was the cause of the regression, which was found by bisecting the RavenDb suite instead: clean origin/main 1 of 28 failures (the known flake) 37b45d8 fan-out 28 of 28 PASS 527260f decoupling 7 failures and by reading the failure dump rather than theorising. It shows the database holding a clean 3/3/3/3 while the runtimes disagree -- fake://six/ and fake://nine/ each RUNNING ON TWO NODES. Once the evaluation is decoupled from execution, a dispatched-but-unconfirmed agent looks entirely unassigned, because Agent.OriginalNode comes from persisted ActiveAgents and the assignment row only appears once the agent is actually running. A later pass therefore emits a plain AssignAgent to a different node with NO stop for the copy already starting elsewhere. Before the decoupling the wave always completed first, so a move was a ReassignAgent, which is stop-then-start. pinPendingAssignments only guards while the ledger entry lives, and that TTL is 2x CheckAssignmentPeriod -- two seconds in that suite, far shorter than confirmation takes. The fix is a design change, not another patch: pending assignment has to be a first-class grid state rather than a TTL'd suppression list. Either hold the ledger entry until confirmed-or-failed (the dispatcher now knows about failure), or convert an AssignAgent whose agent has a pending entry on a different node into a ReassignAgent from that node so a stop is always issued. Probably both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… tests GH-3702 reinstated the xUnit1051 analyzer on main after this branch was cut, and CI builds the merge commit, so these six WaitAsync calls and one Task.Delay became hard build errors the moment #3720 landed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013ojaHNzLcE8m5krwfTNPxw
Decoupling the leader's assignment evaluation from the command drain let the same agent run on two nodes at once. An agent dispatched but not yet confirmed running looks completely unplaced -- Agent.OriginalNode comes from each node's PERSISTED active agents, and the assignment row only appears once the agent is actually running -- so a later pass re-decided its placement and emitted a bare AssignAgent to a second node with no stop for the copy already starting on the first. Before the decoupling the wave always finished first, so a move was a ReassignAgent. The ledger that was meant to prevent this was a TTL'd suppression list applied to commands after the fact. Two seconds in the compliance suite, and keyed on (agent -> node), so a re-target defeated it entirely. Pending assignment is now a state the grid can see: - AssignmentGrid.Agent.PendingNode, projected from the ledger before each family distributes. TryBuildAssignmentCommand accounts for the possible copy in every arm: a stop aimed at the pending node when the agent is paused or detached, a stop-then-start when it moves, nothing when it is still going to the same place. Pausing a pending agent previously emitted nothing at all -- there was no OriginalNode to stop it on -- so the in-flight start came up and kept running. - ReassignAgent.StopInSourceLane runs the stop half in the source node's lane, so it queues behind the start it is cancelling instead of racing ahead of it, finding nothing to stop, and letting that node bring the agent up moments later. - The hold now lasts until the dispatch is confirmed or failed rather than for a fixed TTL. AgentCommandDispatcher already tracks a command from the moment it is queued until its lane is done with it; TryFindPendingDestination exposes that, and it -- not the clock -- decides when a dispatch is over. The TTL survives only as a backstop for the window between the wire completing and the assignment row turning up in a later LoadNodeAgentState. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013ojaHNzLcE8m5krwfTNPxw
#3723 landed on main as a squash, so this branch's own merge of `gh-3698/fanout` conflicted with the squashed copy. Three conflicts, all resolved in favour of this branch, which supersedes the fanout work rather than diverging from it: * `ReassignAgent.DestinationNodeId` — keep `StopInSourceLane`, so the stop half can be ordered in OriginalNode's lane and queue behind a still-pending start instead of racing ahead of it. main's version keys unconditionally to ActiveNode, which predates pending-dispatch state. * `WolverineRuntime.executeHealthChecks` — keep the `_dispatcher.Enqueue` hand-off. main still awaited `executeAgentCommandsAsync` inline, which is precisely the coupling this PR removes. `executeAgentCommandsAsync`/`buildDrain` stay for `ApplyRestrictionsAsync`, which still needs the synchronous drain to report failures back to the operator. * `agent_assignment_at_scale` — keep this branch's assertions, a superset: main's convergence budget plus `longestEvaluationGap`, the assertion that the leader keeps evaluating while starts are in flight. Budget stays at this branch's measured 100s rather than main's 75s because the decoupled drain pays a control-queue round-trip per chunk per lane. Verified: `dotnet build wolverine.slnx -c Release` clean with 0 warnings; CoreTests 2139 passed, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jeremydmiller
marked this pull request as ready for review
July 30, 2026 00:31
erdtsieck
pushed a commit
to erdtsieck/wolverine
that referenced
this pull request
Jul 31, 2026
…ments (JasperFxGH-3698) JasperFx#3719 changed what happens when an assignment is dispatched but not yet confirmed, and merged without this test — it was written alongside that work but never committed, so the two situations the pending-assignment state exists for had no compliance coverage on any persistence engine. `every_agent_survives_a_pause_and_losing_the_nodes_that_held_them` puts both into one scenario: an operator pause applied while assignments are in flight, and then the nodes holding the rest of them disappearing — the leader among them, so a fresh election has to carry the restriction forward too. Neither may lose an agent, and neither may leave one running on two nodes. Before JasperFx#3719, re-placing a dispatched-but-unconfirmed agent produced a bare start on the new node with no stop for the copy already coming up on the old one, and pausing such an agent produced no command at all — there was no OriginalNode to stop it on, so the in-flight start came up and ran straight through the pause. `expectExactlyOneCopyOfEachAsync` distinguishes the two failure directions rather than reporting a bare timeout: a MISSING agent means a stop was issued without a start reaching anyone, a DUPLICATED one means a start was issued without a stop. Those point at opposite halves of the fix, so the assertion says which. This runs on every engine that inherits LeadershipElectionCompliance. Verified locally: Postgres 5/5 green on the new test alone plus 15/15 for the full suite; SqlServer 15/15; `dotnet build wolverine.slnx -c Release` clean with 0 warnings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #3723 — merge that first.
This is the reporter's ask 2 from #3698:
executeHealthChecksawaitedDoHealthChecksAsync()and then the whole agent-command drain in the sameloop body, so for as long as a wave of agent starts was in flight,
EvaluateAssignmentsAsyncdid not run atall — and since the observer early-returns on an empty command list, the leader wrote zero
AssignmentChangedrecords the entire time. That is why the reported cluster looked stalled and idle ratherthan merely slow.
What was wrong with the first attempt
Decoupling alone let the same agent run on two nodes at once, and
RavenDbTests.LeaderElectionbisectedcleanly onto it: 1 failure of 28 on clean
origin/main, 28/28 on #3723 alone, 7 failures with thedecoupling.
An agent that has been dispatched but not yet confirmed running looks completely unplaced —
Agent.OriginalNodecomes from each node's persisted active agents, and the assignment row only appearsonce the agent is actually running. A later pass re-decided its placement and emitted a plain
AssignAgentto a second node with no stop for the copy already starting on the first. Before the decoupling the wave
always completed first, so a move was a
ReassignAgent, which is stop-then-start.The ledger meant to prevent this was a TTL'd suppression list applied to the command list after the fact:
2 × CheckAssignmentPeriod— two seconds in that suite — and keyed on(agent → node), so a re-targetdefeated it outright.
The fix — pending assignment is a grid state
AssignmentGrid.Agent.PendingNode, projected from the ledger before each family distributes.TryBuildAssignmentCommandnow accounts for the possible in-flight copy in every arm: a stop aimed at thepending node when the agent is paused or detached, a stop-then-start when it moves, nothing when it is
still going to the same place, and a re-drive to the same node once the dispatch has gone unconfirmed for
long enough.
ReassignAgent.StopInSourceLaneruns the stop half in the source node's lane, so it queues behindthe start it is cancelling instead of racing ahead of it, finding nothing to stop, and letting that node
bring the agent up moments later.
AgentCommandDispatcheralready tracks a command from the moment it is queued until its lane is done with it, whatever the
outcome;
TryFindPendingDestinationexposes that, and it — not the clock — decides when a dispatch isover. The TTL survives only as a backstop for the window between the wire completing and the assignment
row turning up in a later
LoadNodeAgentState.Found in passing, with coverage: pausing an agent mid-dispatch previously emitted nothing at all. There
was no
OriginalNodeto stop it on, so the in-flight start came up and kept running straight through thepause.
Verification
Three runs of
SlowTests/Agents/agent_assignment_at_scale, each against a freshly recreated Postgrescontainer, plus a same-session control run of #3723 alone:
Agent starts land 300/300/300 across the three nodes. So the convergence regression is fully recovered and
both of the reporter's asks now hold at once. The mechanism shows up in a unit test as well as the
stopwatch: with the hold in place a node joining mid-wave emits zero commands, because the distribution
balances the remainder around the in-flight agents instead of re-spreading everything and clawing it back.
That re-spreading was the duplicate work.
RavenDbTests.LeaderElection:origin/mainspin_up_several_nodes_take_away_original_nodeflake)Plus 10 new unit tests in
CoreTests/Runtime/Agents/pending_assignment_grid_state.csand the existingpending_assignment_ledgersuite, with all 230CoreTests.Runtime.Agentstests green.Also on this branch
Uri[]-carrying agent command (AgentUriSet). The recorddefault compares those arrays by reference;
StartAgentsandStopAgentshad hand-writtenSequenceEqualoverrides but both leftGetHashCodereturning the array's reference hash, so they didnothing in any hash-based lookup, and the other four types had no override at all.
AgentCommandDispatcher— a persistent lane (channel + long-lived worker) per destination, replacing aone-wave-at-a-time pump that had head-of-line blocking between waves.
instruction, not a duplicate of the copy in flight.
🤖 Generated with Claude Code
https://claude.ai/code/session_013ojaHNzLcE8m5krwfTNPxw