CB-524: make worker placement order deterministic across JVM runs
The weighted policy breaks an exact-weight tie on candidate list order (WeightedRoundRobinPolicy picks the first candidate with a strictly greater score), and that list comes from CompositePeerLauncher.candidates(), which iterates profileConfigs. Both that map and BridgedConfig.workerProfiles() were built with Map.copyOf, whose iteration order is salted per JVM run — so the "in definition order" contract candidates() documents was not held. Two consequences. In production, a config with equal weights (ollama 0.5 / gx10 0.5) placed its first worker on a profile chosen at random on every daemon restart. In the suite, CompositePeerLauncherTest.failoverRetriesNextCandidate- WhenProfileIsUnreachable failed roughly one run in four, because whether profile "a" was tried first depended on the salt. Preserve definition order at every layer: unmodifiable LinkedHashMap for workerProfiles(), profileConfigs, and byProfile (which also feeds the user-visible bridge_profiles listing). The tests build profile maps with an ordered helper rather than Map.of, which is salted for the same reason. Guarded by a pair of tests declaring the same two profiles in opposite order and asserting opposite first attempts, so any order-scrambling implementation must fail one of them. Verified by mutation: reverting profileConfigs to Map.copyOf fails 8/8 runs (6 caught by the original test, 2 only by the new reversed-order one); with the fix, 10/10 fresh JVMs pass, 388 tests green.
This commit is contained in:
@@ -79,6 +79,11 @@ class BridgedConfigTest {
|
||||
|
||||
BridgedConfig cfg = BridgedConfig.load(f);
|
||||
assertEquals(Set.of("gx10", "ollama"), cfg.workerProfiles().keySet());
|
||||
// Order, not just membership: placement breaks an exact-weight tie on definition order, so a
|
||||
// hash-ordered map here would make equal-weight placement differ from one restart to the next.
|
||||
assertEquals(java.util.List.of("gx10", "ollama"),
|
||||
java.util.List.copyOf(cfg.workerProfiles().keySet()),
|
||||
"workerProfiles must preserve YAML definition order");
|
||||
assertEquals("gx10", cfg.defaultProfile());
|
||||
assertEquals("ollama", cfg.workerProfiles().get("ollama").profile(), "profile defaults to its map key");
|
||||
assertEquals("http://gx10.gw:8000", cfg.workerProfiles().get("gx10").baseUrl());
|
||||
|
||||
@@ -18,6 +18,7 @@ import org.junit.jupiter.api.Test;
|
||||
import java.util.EnumSet;
|
||||
import java.util.HashMap;
|
||||
import java.util.HashSet;
|
||||
import java.util.LinkedHashMap;
|
||||
import java.util.List;
|
||||
import java.util.Map;
|
||||
import java.util.Set;
|
||||
@@ -122,6 +123,20 @@ class CompositePeerLauncherTest {
|
||||
weight, maxLoad);
|
||||
}
|
||||
|
||||
/**
|
||||
* An <em>order-preserving</em> profile map. Never {@code Map.of} here: its iteration order is
|
||||
* salted per JVM run, and the weighted policy breaks an exact-weight tie on candidate order —
|
||||
* so a {@code Map.of} would make "which profile is tried first" a coin flip per run and any
|
||||
* assertion about the first attempt intermittently false.
|
||||
*/
|
||||
private static Map<String, BridgedConfig.Worker> ordered(String first, BridgedConfig.Worker a,
|
||||
String second, BridgedConfig.Worker b) {
|
||||
Map<String, BridgedConfig.Worker> m = new LinkedHashMap<>();
|
||||
m.put(first, a);
|
||||
m.put(second, b);
|
||||
return m;
|
||||
}
|
||||
|
||||
@SuppressWarnings("unchecked")
|
||||
private static String startedName(FakeHerdr herdr) {
|
||||
return (String) ((Map<String, Object>) herdr.lastCall("agent.start").params()).get("name");
|
||||
@@ -249,7 +264,7 @@ class CompositePeerLauncherTest {
|
||||
@Test
|
||||
void weightedPolicyGatesProfileAtMaxLoad() {
|
||||
FakeHerdr herdr = new FakeHerdr();
|
||||
Map<String, BridgedConfig.Worker> profiles = Map.of(
|
||||
Map<String, BridgedConfig.Worker> profiles = ordered(
|
||||
"a", stubWorker("a", 1.0f, 1),
|
||||
"b", stubWorker("b", 1.0f, null));
|
||||
StubLauncher adapter = new StubLauncher("claude", herdr, profiles, "a", Set.of());
|
||||
@@ -265,7 +280,7 @@ class CompositePeerLauncherTest {
|
||||
@Test
|
||||
void weightedPolicyDistributesAccordingToWeightRatio() {
|
||||
FakeHerdr herdr = new FakeHerdr();
|
||||
Map<String, BridgedConfig.Worker> profiles = Map.of(
|
||||
Map<String, BridgedConfig.Worker> profiles = ordered(
|
||||
"a", stubWorker("a", 0.75f, null),
|
||||
"b", stubWorker("b", 0.25f, null));
|
||||
StubLauncher adapter = new StubLauncher("claude", herdr, profiles, "a", Set.of());
|
||||
@@ -285,7 +300,7 @@ class CompositePeerLauncherTest {
|
||||
@Test
|
||||
void failoverRetriesNextCandidateWhenProfileIsUnreachable() {
|
||||
FakeHerdr herdr = new FakeHerdr();
|
||||
Map<String, BridgedConfig.Worker> profiles = Map.of(
|
||||
Map<String, BridgedConfig.Worker> profiles = ordered(
|
||||
"a", stubWorker("a"),
|
||||
"b", stubWorker("b"));
|
||||
StubLauncher adapter = new StubLauncher("claude", herdr, profiles, "a", Set.of("a"));
|
||||
@@ -298,10 +313,30 @@ class CompositePeerLauncherTest {
|
||||
assertEquals(1, adapter.spawnCount("b"), "b was tried once and succeeded");
|
||||
}
|
||||
|
||||
/**
|
||||
* Definition order — not hash order — decides an exact-weight tie. Paired with the test above
|
||||
* (same two profiles, opposite declaration order, opposite expected first attempt) this pins the
|
||||
* ordering contract from both sides: under a salted map one of the two must fail on every run.
|
||||
*/
|
||||
@Test
|
||||
void reversingDefinitionOrderReversesWhichProfileIsTriedFirst() {
|
||||
FakeHerdr herdr = new FakeHerdr();
|
||||
Map<String, BridgedConfig.Worker> profiles = ordered(
|
||||
"b", stubWorker("b"),
|
||||
"a", stubWorker("a"));
|
||||
StubLauncher adapter = new StubLauncher("claude", herdr, profiles, "a", Set.of("b"));
|
||||
CompositePeerLauncher composite = new CompositePeerLauncher(
|
||||
List.of(adapter), "a", profiles, PlacementPolicies.weighted(), _ -> 0);
|
||||
|
||||
PeerHandle h = composite.spawn(new SpawnRequest(null, null, null));
|
||||
assertEquals("a", h.profile(), "b is declared first and unreachable, so the spawn lands on a");
|
||||
assertEquals(1, adapter.spawnCount("b"), "b, declared first, is the one tried first");
|
||||
}
|
||||
|
||||
@Test
|
||||
void failoverBoundedByCandidateCount() {
|
||||
FakeHerdr herdr = new FakeHerdr();
|
||||
Map<String, BridgedConfig.Worker> profiles = Map.of(
|
||||
Map<String, BridgedConfig.Worker> profiles = ordered(
|
||||
"a", stubWorker("a"),
|
||||
"b", stubWorker("b"));
|
||||
StubLauncher adapter = new StubLauncher("claude", herdr, profiles, "a", Set.of("a", "b"));
|
||||
@@ -318,7 +353,7 @@ class CompositePeerLauncherTest {
|
||||
@Test
|
||||
void emptyCandidateSetThrowsClearException() {
|
||||
FakeHerdr herdr = new FakeHerdr();
|
||||
Map<String, BridgedConfig.Worker> profiles = Map.of(
|
||||
Map<String, BridgedConfig.Worker> profiles = ordered(
|
||||
"a", stubWorker("a", 1.0f, 1),
|
||||
"b", stubWorker("b", 1.0f, 1));
|
||||
StubLauncher adapter = new StubLauncher("claude", herdr, profiles, "a", Set.of());
|
||||
|
||||
Reference in New Issue
Block a user