GH-4562: grandfather a group-affinity candidate per member, not per partition

[erdtsieck]

Closes #4562.

Problem

In DistributeByGroupAffinity, a node became a candidate for a whole capability partition as soon as it ran one member (|| members.Any(m => m.OriginalNode == n)). The sibling preference then picks that node, because it already hosts the database's other partition.

During a blue/green rollout, blue runs the agents of a tenant provisioned after blue started without declaring them. Only green declares those agents, so they share a partition with the new version's agents. Blue then took that whole partition, new version included. It cannot build those agents, so they never started, and the leader re-sent them to blue on every evaluation.

Fix

The grandfathering moves inside All: a node qualifies when every member is either declared by it or already running on it. A node that runs the whole partition, the stale-snapshot case the grandfathering exists for, still qualifies. A node that runs only part of it no longer does.

Tests

a_node_running_part_of_a_partition_is_not_a_candidate_for_members_it_cannot_run reproduces this. On main it fails because the bumped agent lands on the blue node.

Everything else in CoreTests.Runtime.Agents is still green (387 tests), including a_node_already_running_a_groups_agents_stays_a_candidate_despite_a_stale_capability_snapshot, an_incumbent_node_keeps_its_group_up_to_the_ceiling and the blue/green split tests. I did not run the MartenTests blue/green integration tests locally.

I also replayed a real 6-node production grid snapshot through DistributeByGroupAffinity. The affected partitions were exactly the three databases holding such a tenant.

[jeremydmiller]

Thanks for the detailed writeup and the production replay — the diagnosis is right, and the fix is right. I pushed one commit to this branch (fd80264) rather than asking you to round-trip it. Here is what it does and why.

The extra commit

Moving the grandfathering inside All withdraws something the old Any(m => m.OriginalNode == n) was also doing: holding together a partition that no node declares at all. That is the GH-3341 stale-snapshot case — a shard database provisioned after every surviving node captured its capability snapshot.

The GH-3341 rescue below the candidate filter did not cover it, because it also required every member to be unassigned:

if (members.All(m => m.AssignedNode == null && m.CandidateNodes.Count == 0))

While a single running member grandfathered its node into the candidates, that extra clause never mattered — the candidate filter got there first. Per-member grandfathering removes that cover, so a database with two tenants placed by an earlier evaluation and a third provisioned since now falls through to the per-member fallback and scatters one agent per node. That opens a connection pool per node to that database, which is the cost DistributeByGroupAffinity exists to avoid (JasperFx/marten#4806).

Concretely, two nodes with divergent snapshots and neither declaring db1, node 1 running t1/t2, t3 newly provisioned:

main:     t1 -> node 1, t2 -> node 1, t3 -> node 1   (whole)
this PR:  t1 -> node 1, t2 -> node 1, t3 -> node 2   (split)

The fix drops the unassigned clause:

if (members.All(m => m.CandidateNodes.Count == 0))

That is safe because every member of a partition shares one capabilityKey — the sorted ids of its candidate nodes — so a partition is either wholly undeclared or wholly declared. The unassigned clause was always redundant with the capability test.

Worth recording for whoever reads this next: that same capabilityKey property bounds the blast radius of your change. When a partition's candidate set is non-empty it always contains the declaring nodes, so candidates can only go empty for a wholly-undeclared partition. The undeclared case above is therefore the only behaviour the per-member tightening can alter, which is why one added test closes it.

Tests

• a_partially_running_undeclared_group_still_stays_whole — the scenario above. Red with the unassigned clause restored, green without.
• a_node_running_part_of_a_partition_is_not_a_candidate_for_members_it_cannot_run — your test, extended from three placements to all five. The unchanged agent sharing a capability set with the bumped ones moves off blue with them; that is the price of an indivisible partition, and it is better asserted than left as an untested side effect.
• the_blue_green_partial_grandfather_split_settles_on_the_next_evaluation — replays the result as the next evaluation's grid. This one is a churn guard, not a proof of the fix: it passes on the old predicate too, because the grid cannot model a node failing to build what it was handed. Noted in the test so it is not mistaken for coverage it does not provide.

Verification

364 tests green in CoreTests.Runtime.Agents, 3035 in CoreTests, and wolverine.slnx builds clean in Release on net9.0 with no warnings. The MartenTests blue/green tests you mentioned not running locally are covered by CI (CIMartenTenancy, CIMartenDistribution) and were green on your commit.

One note on those: blue_green_version_bump_assignment cannot catch this class of defect. Its BuildGrid() has blue running only what it advertises, so the post-snapshot-tenant shape — a node running an agent it does not declare — never occurs end to end. The placement logic is covered by the unit tests instead.

Merging once CI is green. Thanks again for chasing this down to the grid snapshot.