GH-3959 follow-ups: require a real load monitor, stop the shed draining the cluster, hold pins across every distribution path - #4596
Merged
Conversation
…reachable shed, and pins that hold Three follow-ups to the capacity-aware agent assignment merged in #4297. GH-4589 — the default load monitor measured the wrong thing. MemoryPressureLoadMonitor divided process RSS by GCMemoryInfo.TotalAvailableMemoryBytes, which is the GC's budget rather than a memory limit. On a host with no cgroup limit that denominator is the whole machine's RAM: a 116 MB service on a 128 GB box read 0.09, so the feature was inert, and inert silently. Inside a cgroup the denominator is ~75% of the limit while the numerator is still full RSS, so a container at 60% of its limit read 80 -- already at the receive line. One formula, wrong in opposite directions on the two shapes we actually deploy. There is now no default at all: CapacityAwareAssignment requires an explicit INodeLoadMonitor and the runtime refuses to start without one. What "load" means is specific to what an application does, and a built-in guess looks authoritative while being wrong for most deployments. Silently falling back was the worst option available -- a node advertising nothing is treated as having unlimited headroom, so the fallback would turn the feature on and then quietly make that node the cluster's preferred target. MemoryPressureLoadMonitor stays, opt-in, measuring against the cgroup limit it actually runs under and returning null when there is no limit to measure against. GH-4590 — shedding drained an overloaded cluster to zero. The shed pass ran before the "nobody has headroom" early return, so when every node was over the line each evaluation detached a batch and placed none of it: 3 -> 2 -> 1 -> 0, one batch per tick, taking the durability agents and daemon shards with it. On a single-node cluster it was unconditional. Shedding is only ever a move, so it now happens only once a destination is known to exist. Also: a node advertising no load sorted into the LOWEST band, making a node mid-rolling-upgrade -- or one whose sampler was throwing -- the preferred placement target precisely when least was known about it. It sorts mid-range now. Still eligible, no longer privileged. GH-4591 — pinned agents were detached by the ceiling pass. Restrictions are applied before the families distribute, so a pass that detached whatever sat above the line undid an operator's pin, and ApplyRestrictions put it back next evaluation, and the pass took it off again: a churn loop that never converges. #4297 fixed this for DistributeEvenly; the blue/green and affinity paths still had the bare Skip(maximum). All three now share Node.ExtrasAboveCeiling, which counts pins toward the ceiling without ever detaching one. The capability detach in the affinity path deliberately still ignores pins -- a pin to a node that cannot run the agent is an instruction that cannot be carried out, and honoring it would park the agent somewhere that only throws. Each new test was confirmed red against the pre-fix code before being kept. The pin tests in particular needed two corrections to bite: the pin has to sit at the TAIL of the node's agent list (where TryAssign appends it) and has to have somewhere else it could go, or the detach is invisible in the final assignment. Full wolverine.slnx Release build on net9.0 clean; CoreTests 3062 green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Checks out on the tools I used to reproduce it. Sorry for the oversight on the lost assignment. I added hooks to detect agents that were both unassigned and not running for a later experiment for testing ungraceful shutdown on nodes but didn't then apply that check to the overload thing I used for testing #4297. |
This was referenced Sep 23, 2026
Closed
Merged
This was referenced Sep 24, 2026
This was referenced Sep 28, 2026
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.

Immediate follow-ups to #4297, closing #4589, #4590 and #4591.
#4589 — the default load monitor measured the wrong thing
MemoryPressureLoadMonitordivided process RSS byGCMemoryInfo.TotalAvailableMemoryBytes— the GC's budget, not a memory limit. The mismatch broke the reading in opposite directions on the two shapes we actually deploy:So: inert on one, trigger-happy on the other, and inert silently — you set the flag, nothing happens, nothing says why.
There is now no default.
CapacityAwareAssignmentrequires an explicitINodeLoadMonitor, and the runtime refuses to start without one. What "load" means is specific to what an application does, and a built-in guess looks authoritative while being wrong for most deployments. Falling back silently was the worst option on the table: a node advertising nothing is treated as having unlimited headroom, so the fallback would turn the feature on and then quietly make that node the cluster's preferred placement target.MemoryPressureLoadMonitorstays as an opt-in class, now measuring against the cgroup limit the process actually runs under (v2memory.max, then v1, then a configured GC hard limit) and returningnullwhen there is no limit to measure against. A monitor that cannot see a ceiling says so instead of inventing one.#4590 — shedding drained an overloaded cluster to zero
The shed pass ran before the "nobody has headroom" early return, so with every node over the line each evaluation detached a batch and placed none of it:
One batch per tick, taking the durability agents and daemon shards with it, and never restarting while the pressure held. On a single-node cluster it was unconditional. Shedding is only ever a move, so it now happens only once a destination is known to exist — an overloaded node still running its work beats an idle one.
Also in this area: a node advertising no load sorted into the lowest band, which made a node mid-rolling-upgrade — or one whose sampler was throwing — the preferred target for every placement precisely when least was known about it. It sorts mid-range now: still eligible, no longer privileged.
#4591 — pinned agents were detached by the ceiling pass
ApplyRestrictionsruns before the families distribute, so a ceiling pass that detached whatever sat above the line undid an operator's pin — andApplyRestrictionsput it back next evaluation, and the pass took it off again. A churn loop that emits commands forever and never converges.#4297 fixed this for
DistributeEvenly.DistributeEvenlyWithBlueGreenSemanticsandDistributeEvenlyWithAffinitystill had the bareSkip(maximum). All three now shareNode.ExtrasAboveCeiling, which counts pins toward the ceiling without ever detaching one — a node carrying pins gives up more of its unpinned agents instead.The capability detach in the affinity path deliberately still ignores pins. A pin to a node that cannot run the agent is an instruction that cannot be carried out, and honoring it would park the agent somewhere that only throws "Unrecognized agent scheme".
Notes on the tests
Every new test was confirmed red against the pre-fix code before being kept. Two needed correcting before they bit:
ApplyRestrictionsactually leaves it, sinceTryAssignappends — and has somewhere else it could go. A pinned agent whose only capable node is the one it is pinned to gets detached and re-placed on that same node within the pass, so the final assignment looks untouched even though the evaluation churned.TryAssignonly succeeds for an agent the node declares, so a grid built without capabilities silently never applies the pin at all. My first version of the fixed-point test was green for that reason rather than for the right one.Environment.WorkingSet, which passed alone and failed inside the full suite.MemoryPressureLoadMonitornow has aprotected virtual CurrentWorkingSetseam so the arithmetic and the smoothing are driven deterministically.The drain regressions need consecutive evaluations to fail — a single-tick assertion passed throughout the bug's life.
Verification
dotnet build wolverine.slnx -c Release -f net9.0— 0 warnings, 0 errorsCoreTests— 3062 passed, 0 failed (+16 new)PostgresqlTests.Agents.capacity_aware_column_is_opt_in— 3 passed against the docker PostgresRemaining follow-ups from the #4297 review are tracked in #4592 (group-affinity / blue-green coverage — the multi-database shape this feature was asked for), #4593 (the other message stores), #4594 (docs), JasperFx/CritterWatch#1343 (alert category) and JasperFx/ai-skills#233.
🤖 Generated with Claude Code