From ccf50f950ec7da6495f9eea6a6b1baaaa736b1b1 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Wed, 5 Aug 2026 06:02:05 +0200 Subject: [PATCH] CB-524: make worker placement order deterministic across JVM runs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../ltms/bridged/config/BridgedConfig.java | 7 ++- .../bridged/worker/CompositePeerLauncher.java | 9 +++- .../bridged/config/BridgedConfigTest.java | 5 +++ .../worker/CompositePeerLauncherTest.java | 45 ++++++++++++++++--- 4 files changed, 58 insertions(+), 8 deletions(-) diff --git a/bridged/src/main/java/dev/ltms/bridged/config/BridgedConfig.java b/bridged/src/main/java/dev/ltms/bridged/config/BridgedConfig.java index fce933e..27f31c4 100644 --- a/bridged/src/main/java/dev/ltms/bridged/config/BridgedConfig.java +++ b/bridged/src/main/java/dev/ltms/bridged/config/BridgedConfig.java @@ -8,6 +8,7 @@ import java.io.IOException; import java.io.UncheckedIOException; import java.nio.file.Files; import java.nio.file.Path; +import java.util.Collections; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -354,7 +355,11 @@ public record BridgedConfig( Map out = new LinkedHashMap<>(); workers.forEach((name, w) -> out.put(name, (w.profile() == null || w.profile().isBlank()) ? w.withProfile(name) : w)); - return Map.copyOf(out); + // Deliberately NOT Map.copyOf: its iteration order is salted per JVM run, which would + // discard the YAML definition order built above. Placement tie-breaks on candidate + // order (see WeightedRoundRobinPolicy), so losing it makes equal-weight placement + // non-reproducible across restarts. Unmodifiable-wrap instead of copy-and-scramble. + return Collections.unmodifiableMap(out); } if (worker != null) { String name = (worker.profile() == null || worker.profile().isBlank()) ? "default" : worker.profile(); diff --git a/bridged/src/main/java/dev/ltms/bridged/worker/CompositePeerLauncher.java b/bridged/src/main/java/dev/ltms/bridged/worker/CompositePeerLauncher.java index 40ad67e..3df94b7 100644 --- a/bridged/src/main/java/dev/ltms/bridged/worker/CompositePeerLauncher.java +++ b/bridged/src/main/java/dev/ltms/bridged/worker/CompositePeerLauncher.java @@ -15,6 +15,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.util.ArrayList; +import java.util.Collections; import java.util.EnumSet; import java.util.HashSet; import java.util.LinkedHashMap; @@ -99,7 +100,10 @@ public final class CompositePeerLauncher implements PeerLauncher { } this.delegates = List.copyOf(delegates); this.defaultProfile = defaultProfile; - this.profileConfigs = Map.copyOf(profileConfigs); + // LinkedHashMap, not Map.copyOf: candidates() promises definition order and the weighted + // policy breaks exact-weight ties on it, so a salted iteration order would make placement + // differ from one JVM run to the next. + this.profileConfigs = Collections.unmodifiableMap(new LinkedHashMap<>(profileConfigs)); this.placementPolicy = placementPolicy; this.liveCount = liveCount; Map index = new LinkedHashMap<>(); @@ -112,7 +116,8 @@ public final class CompositePeerLauncher implements PeerLauncher { } } } - this.byProfile = Map.copyOf(index); + // Order-preserving for the same reason, and because profiles() is user-visible (bridge_profiles). + this.byProfile = Collections.unmodifiableMap(index); } /** The adapter owning {@code profileName} (null/blank → the default). Throws on an unknown profile. */ diff --git a/bridged/src/test/java/dev/ltms/bridged/config/BridgedConfigTest.java b/bridged/src/test/java/dev/ltms/bridged/config/BridgedConfigTest.java index 1590ca3..2960b70 100644 --- a/bridged/src/test/java/dev/ltms/bridged/config/BridgedConfigTest.java +++ b/bridged/src/test/java/dev/ltms/bridged/config/BridgedConfigTest.java @@ -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()); diff --git a/bridged/src/test/java/dev/ltms/bridged/worker/CompositePeerLauncherTest.java b/bridged/src/test/java/dev/ltms/bridged/worker/CompositePeerLauncherTest.java index 08f006f..4d45e21 100644 --- a/bridged/src/test/java/dev/ltms/bridged/worker/CompositePeerLauncherTest.java +++ b/bridged/src/test/java/dev/ltms/bridged/worker/CompositePeerLauncherTest.java @@ -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 order-preserving 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 ordered(String first, BridgedConfig.Worker a, + String second, BridgedConfig.Worker b) { + Map m = new LinkedHashMap<>(); + m.put(first, a); + m.put(second, b); + return m; + } + @SuppressWarnings("unchecked") private static String startedName(FakeHerdr herdr) { return (String) ((Map) herdr.lastCall("agent.start").params()).get("name"); @@ -249,7 +264,7 @@ class CompositePeerLauncherTest { @Test void weightedPolicyGatesProfileAtMaxLoad() { FakeHerdr herdr = new FakeHerdr(); - Map profiles = Map.of( + Map 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 profiles = Map.of( + Map 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 profiles = Map.of( + Map 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 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 profiles = Map.of( + Map 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 profiles = Map.of( + Map profiles = ordered( "a", stubWorker("a", 1.0f, 1), "b", stubWorker("b", 1.0f, 1)); StubLauncher adapter = new StubLauncher("claude", herdr, profiles, "a", Set.of());