diff --git a/bridged/bridged.example.yaml b/bridged/bridged.example.yaml index e297fe9..2c4f1fe 100644 --- a/bridged/bridged.example.yaml +++ b/bridged/bridged.example.yaml @@ -184,7 +184,12 @@ profiles: mcpUrl: http://127.0.0.1:8765/mcp tokenEnv: BRIDGED_WORKER_TOKEN argv: ["ccs", "gx10"] - weight: 0.5 # relative selection weight for placement: weighted + # weight: relative selection weight for automatic placement (weighted, round-robin, and + # fixed's fallback walk). Absent defaults to 1.0. An explicit 0 or negative value means + # "never auto-select this profile" (CB-554) — it stays reachable via an explicit + # `bridge_spawn{profile:"gx10"}`, which bypasses placement entirely; only automatic + # selection skips it. + weight: 0.5 maxLoad: 2 # max live workers on this profile (omit for unlimited) # gitTokenEnv: GITEA_TOKEN # opt-in: let this profile's workers open their own PR (CB-302) # gitHostEnv: GITEA_HOST # defaults to GITEA_HOST; injected only with gitTokenEnv 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 56e6051..b142b62 100644 --- a/bridged/src/main/java/dev/ltms/bridged/config/BridgedConfig.java +++ b/bridged/src/main/java/dev/ltms/bridged/config/BridgedConfig.java @@ -193,9 +193,15 @@ public record BridgedConfig( * (minimal-grant default — push over SSH stays free, PR-create is opt-in) * @param gitHostEnv name of the host env var holding the forge host (default {@code GITEA_HOST}); * injected as {@code GITEA_HOST} only when {@code gitTokenEnv} is set - * @param weight relative selection weight for {@code placement: weighted}. Absent or - * non-positive ⇒ 1.0. Weights are normalised by the policy, so they need - * not sum to 1.0. + * @param weight relative selection weight for automatic placement ({@code weighted}, + * {@code round-robin}, and {@code fixed}'s fallback walk). Absent ⇒ 1.0. + * An explicit {@code 0} or negative value (CB-554) means "never + * auto-select this profile": it is excluded from every automatic policy's + * pool the same way a quarantined candidate is (see + * {@code PlacementPolicyUtil}). This does not make the profile + * unreachable — an explicit {@code bridge_spawn{profile:"..."}} bypasses + * placement entirely and still resolves it. Weights among the remaining + * (non-excluded) candidates need not sum to 1.0; only their ratios matter. * @param maxLoad max live workers allowed on this profile at one time; absent or * non-positive ⇒ unlimited. Live means any session the registry still owns * (acquired and not yet released), in any state. @@ -287,7 +293,11 @@ public record BridgedConfig( // checkpoints need only set gitTokenEnv; it is injected only alongside a resolved token. gitHostEnv = (gitHostEnv == null || gitHostEnv.isBlank()) ? "GITEA_HOST" : gitHostEnv; env = (env == null) ? Map.of() : Map.copyOf(env); - weight = (weight == null || weight <= 0.0f) ? 1.0f : weight; + // CB-554: absent still means 1.0, but an explicit non-positive value must survive as + // "excluded from automatic selection" (PlacementCandidate.excluded(), weight <= 0), not + // get coerced back up to 1.0 — that coercion was the bug (weight: 0 looked like "never + // pick me" and actually meant "pick me as often as anyone else"). + weight = (weight == null) ? 1.0f : Math.max(weight, 0.0f); maxLoad = (maxLoad == null || maxLoad <= 0) ? null : maxLoad; subscription = (subscription != null && subscription) ? Boolean.TRUE : Boolean.FALSE; // exhaustedPattern stays null when unset/blank (opt-in) — no defaulting, no vendor diff --git a/bridged/src/main/java/dev/ltms/bridged/placement/FixedPlacementPolicy.java b/bridged/src/main/java/dev/ltms/bridged/placement/FixedPlacementPolicy.java index 995ca15..e4f69e1 100644 --- a/bridged/src/main/java/dev/ltms/bridged/placement/FixedPlacementPolicy.java +++ b/bridged/src/main/java/dev/ltms/bridged/placement/FixedPlacementPolicy.java @@ -5,28 +5,62 @@ package dev.ltms.bridged.placement; * profile, exactly as {@code CompositePeerLauncher} did before CB-518. This ignores caps and * reachability so that a pre-existing config behaves identically after upgrade. * - *

Quarantine (CB-578 stage B) is the one exception: a quarantined default is a credential that - * just refused on a usage limit, not a transient capacity or reachability concern, so {@code fixed} - * steps to the first non-quarantined candidate instead of walking straight back onto it. A fleet - * where nothing is ever quarantined never exercises this path, so today's behaviour is unchanged. + *

Two exceptions walk past the default instead of returning it unconditionally: + *

+ * A fleet where nothing is ever quarantined or weight-0 never exercises either path, so today's + * behaviour is unchanged. */ final class FixedPlacementPolicy implements PlacementPolicy { @Override public PlacementCandidate select(PlacementContext ctx) { String d = ctx.defaultProfile(); - if (d != null && !d.isBlank() && !ctx.quarantined().contains(d)) { + if (d != null && !d.isBlank() && !ctx.quarantined().contains(d) && !weightExcluded(ctx, d)) { return new PlacementCandidate(d, null, 1.0f, null); } for (PlacementCandidate c : ctx.candidates()) { - if (!ctx.quarantined().contains(c.profile())) { + if (!ctx.quarantined().contains(c.profile()) && !c.excluded()) { return new PlacementCandidate(c.profile(), null, c.weight(), c.maxLoad()); } } if (d != null && !d.isBlank()) { - throw new PlacementException("worker profile '" + d + "' is quarantined (backend " - + "exhausted) and no un-quarantined candidate is available"); + boolean dQuarantined = ctx.quarantined().contains(d); + boolean dWeightExcluded = weightExcluded(ctx, d); + if (dQuarantined && dWeightExcluded) { + throw new PlacementException("worker profile '" + d + "' is quarantined (backend " + + "exhausted) and has weight 0 (excluded from automatic selection), and no " + + "available candidate remains"); + } + if (dWeightExcluded) { + throw new PlacementException("worker profile '" + d + "' has weight 0 (excluded " + + "from automatic selection) and no available candidate remains"); + } + if (dQuarantined) { + throw new PlacementException("worker profile '" + d + "' is quarantined (backend " + + "exhausted) and no un-quarantined candidate is available"); + } + } + if (!ctx.candidates().isEmpty()) { + throw new PlacementException( + "all worker profiles are excluded from automatic selection (quarantined or weight-0)"); } throw new PlacementException("no worker profiles configured"); } + + /** Whether {@code profile} carries {@code weight <= 0} (CB-554) among {@code ctx}'s candidates. */ + private static boolean weightExcluded(PlacementContext ctx, String profile) { + for (PlacementCandidate c : ctx.candidates()) { + if (c.profile().equals(profile)) { + return c.excluded(); + } + } + return false; + } } diff --git a/bridged/src/main/java/dev/ltms/bridged/placement/PlacementCandidate.java b/bridged/src/main/java/dev/ltms/bridged/placement/PlacementCandidate.java index 91232d0..c63babc 100644 --- a/bridged/src/main/java/dev/ltms/bridged/placement/PlacementCandidate.java +++ b/bridged/src/main/java/dev/ltms/bridged/placement/PlacementCandidate.java @@ -17,4 +17,18 @@ public record PlacementCandidate(String profile, String host, float weight, Inte public static PlacementCandidate profile(String profile) { return new PlacementCandidate(profile, null, 1.0f, null); } + + /** + * True when this candidate carries an explicit {@code weight <= 0} (CB-554) and must be + * skipped by every automatic policy — the same way a quarantined or unreachable candidate is + * skipped. {@code BridgedConfig.Profile}'s compact constructor already normalises "absent" to + * {@code 1.0} and "negative" to {@code 0.0}, so this is a plain threshold check here; it does + * not need to distinguish "explicit 0" from "absent" itself. + * + *

Exclusion is about automatic selection only — an explicit + * {@code bridge_spawn{profile:"..."}} bypasses placement entirely and is unaffected. + */ + public boolean excluded() { + return weight <= 0.0f; + } } diff --git a/bridged/src/main/java/dev/ltms/bridged/placement/PlacementPolicyUtil.java b/bridged/src/main/java/dev/ltms/bridged/placement/PlacementPolicyUtil.java index 66f0ca6..65a99a3 100644 --- a/bridged/src/main/java/dev/ltms/bridged/placement/PlacementPolicyUtil.java +++ b/bridged/src/main/java/dev/ltms/bridged/placement/PlacementPolicyUtil.java @@ -12,13 +12,16 @@ final class PlacementPolicyUtil { } /** - * Candidates that are not known-unreachable, not quarantined (CB-578 stage B), and have not - * reached their maxLoad. A {@code null} maxLoad means unlimited. + * Candidates that are not weight-excluded (CB-554: explicit {@code weight <= 0}, checked + * first because it is a static config choice rather than transient state), not + * known-unreachable, not quarantined (CB-578 stage B), and have not reached their maxLoad. + * A {@code null} maxLoad means unlimited. */ static List available(PlacementContext ctx) { List out = new ArrayList<>(); for (PlacementCandidate c : ctx.candidates()) { - if (ctx.unreachable().contains(c.profile()) || ctx.quarantined().contains(c.profile())) { + if (c.excluded() || ctx.unreachable().contains(c.profile()) + || ctx.quarantined().contains(c.profile())) { continue; } Integer cap = c.maxLoad(); @@ -34,16 +37,21 @@ final class PlacementPolicyUtil { } /** - * Build a clear exception describing why every candidate was dropped: all at capacity, - * all unreachable, all quarantined, or a mix. + * Build a clear exception describing why every candidate was dropped: all weight-0, all + * quarantined, all at capacity, all unreachable, or a mix. Each candidate is counted into + * exactly one bucket (weight-excluded takes priority) so a candidate excluded for more than + * one reason is never double-counted. */ static PlacementException emptyException(PlacementContext ctx) { + int weightExcluded = 0; int atCap = 0; int unreachable = 0; int quarantined = 0; for (PlacementCandidate c : ctx.candidates()) { Integer cap = c.maxLoad(); - if (ctx.quarantined().contains(c.profile())) { + if (c.excluded()) { + weightExcluded++; + } else if (ctx.quarantined().contains(c.profile())) { quarantined++; } else if (ctx.unreachable().contains(c.profile())) { unreachable++; @@ -56,6 +64,10 @@ final class PlacementPolicyUtil { if (total == 0) { return new PlacementException("no worker profiles configured"); } + if (weightExcluded == total) { + return new PlacementException( + "all worker profiles have weight 0 (excluded from automatic selection)"); + } if (quarantined == total) { return new PlacementException("all worker profiles are quarantined (backend exhausted)"); } @@ -67,6 +79,7 @@ final class PlacementPolicyUtil { } return new PlacementException("no worker profile available: " + atCap + " at maxLoad, " + unreachable + " unreachable, " + quarantined + " quarantined, " - + (total - atCap - unreachable - quarantined) + " remaining"); + + weightExcluded + " weight-0, " + + (total - atCap - unreachable - quarantined - weightExcluded) + " remaining"); } } 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 6274396..6cd306d 100644 --- a/bridged/src/test/java/dev/ltms/bridged/config/BridgedConfigTest.java +++ b/bridged/src/test/java/dev/ltms/bridged/config/BridgedConfigTest.java @@ -1221,6 +1221,42 @@ class BridgedConfigTest { assertNull(w.maxLoad(), "absent maxLoad defaults to unlimited (null)"); } + @Test + void explicitWeightZeroStaysZeroInsteadOfCoercingToOne(@TempDir Path dir) throws Exception { + // CB-554: weight: 0 used to be normalised to 1.0 by this same compact constructor, which + // made it read as "never auto-select" while behaving as "select like anyone else." + Path f = dir.resolve("weight-zero.yaml"); + Files.writeString(f, """ + bind: + port: 8080 + profiles: + opus: + baseUrl: http://gx10.gw:8000 + weight: 0 + """); + + BridgedConfig cfg = BridgedConfig.load(f); + BridgedConfig.Profile w = cfg.profiles().get("opus"); + assertEquals(0.0f, w.weight(), 0.0001f, "an explicit weight: 0 must stay 0, not coerce to 1.0"); + } + + @Test + void negativeWeightNormalisesToZeroNotOne(@TempDir Path dir) throws Exception { + Path f = dir.resolve("weight-negative.yaml"); + Files.writeString(f, """ + bind: + port: 8080 + profiles: + gx10: + baseUrl: http://gx10.gw:8000 + weight: -3.0 + """); + + BridgedConfig cfg = BridgedConfig.load(f); + BridgedConfig.Profile w = cfg.profiles().get("gx10"); + assertEquals(0.0f, w.weight(), 0.0001f, "a negative weight behaves as excluded (0), not as an error and not as 1.0"); + } + @Test void subscriptionFlagBindsAndDefaultsFalse(@TempDir Path dir) throws Exception { Path f = dir.resolve("subscription.yaml"); diff --git a/bridged/src/test/java/dev/ltms/bridged/member/CompositePeerLauncherTest.java b/bridged/src/test/java/dev/ltms/bridged/member/CompositePeerLauncherTest.java index cbc5078..3028831 100644 --- a/bridged/src/test/java/dev/ltms/bridged/member/CompositePeerLauncherTest.java +++ b/bridged/src/test/java/dev/ltms/bridged/member/CompositePeerLauncherTest.java @@ -336,6 +336,42 @@ class CompositePeerLauncherTest { assertEquals(10, b); } + @Test + void weightedPolicySkipsWeightZeroProfileOnUnqualifiedSpawn() { + // CB-554: weight: 0 must exclude a profile from automatic placement, not coerce to 1.0. + FakeHerdr herdr = new FakeHerdr(); + Map profiles = ordered( + "a", stubWorker("a", 0.0f, null), + "b", stubWorker("b", 1.0f, null)); + StubLauncher adapter = new StubLauncher("claude", herdr, profiles, "a", Set.of()); + CompositePeerLauncher composite = new CompositePeerLauncher( + List.of(adapter), "a", profiles, PlacementPolicies.weighted(), _ -> 0); + + for (int i = 0; i < 5; i++) { + PeerHandle h = composite.spawn(new SpawnRequest(null, null, null)); + assertEquals("b", h.profile(), "profile a has weight 0, so every unqualified spawn must land on b"); + } + assertEquals(0, adapter.spawnCount("a"), "a is never chosen automatically"); + } + + @Test + void explicitSpawnStillSucceedsOnWeightZeroProfile() { + // CB-554: weight: 0 excludes a profile from AUTOMATIC selection only — an explicit + // bridge_spawn{profile:"a"} must still work exactly as today (e.g. `opus` on the + // operator's own subscription, kept weight-0 so it is never picked automatically). + FakeHerdr herdr = new FakeHerdr(); + Map profiles = ordered( + "a", stubWorker("a", 0.0f, null), + "b", stubWorker("b", 1.0f, null)); + StubLauncher adapter = new StubLauncher("claude", herdr, profiles, "b", Set.of()); + CompositePeerLauncher composite = new CompositePeerLauncher( + List.of(adapter), "b", profiles, PlacementPolicies.weighted(), _ -> 0); + + PeerHandle h = composite.spawn(new SpawnRequest("a", null, null)); + assertEquals("a", h.profile(), "naming a weight-0 profile explicitly bypasses placement and still spawns it"); + assertEquals(1, adapter.spawnCount("a")); + } + @Test void failoverRetriesNextCandidateWhenProfileIsUnreachable() { FakeHerdr herdr = new FakeHerdr(); diff --git a/bridged/src/test/java/dev/ltms/bridged/placement/PlacementPolicyTest.java b/bridged/src/test/java/dev/ltms/bridged/placement/PlacementPolicyTest.java index 9c8a1c3..cc2058f 100644 --- a/bridged/src/test/java/dev/ltms/bridged/placement/PlacementPolicyTest.java +++ b/bridged/src/test/java/dev/ltms/bridged/placement/PlacementPolicyTest.java @@ -237,6 +237,105 @@ class PlacementPolicyTest { assertTrue(e.getMessage().contains("1 unreachable"), e.getMessage()); } + // --- CB-554: weight <= 0 excludes a candidate from automatic selection ------------------ + + @Test + void weightedSkipsWeightZeroProfile() { + PlacementPolicy policy = PlacementPolicies.weighted(); + List candidates = List.of( + PlacementCandidate.profile("a", 0.0f, null), + PlacementCandidate.profile("b", 1.0f, null)); + for (int i = 0; i < 5; i++) { + assertEquals("b", policy.select(ctx(candidates, noSessions())).profile(), + "a has weight 0, so every pick lands on b"); + } + } + + @Test + void weightedThrowsWhenAllWeightZero() { + PlacementPolicy policy = PlacementPolicies.weighted(); + List candidates = List.of( + PlacementCandidate.profile("a", 0.0f, null), + PlacementCandidate.profile("b", 0.0f, null)); + PlacementException e = assertThrows(PlacementException.class, + () -> policy.select(ctx(candidates, noSessions()))); + assertTrue(e.getMessage().contains("weight 0"), e.getMessage()); + } + + @Test + void weightedTreatsNegativeWeightAsExcludedNotError() { + PlacementPolicy policy = PlacementPolicies.weighted(); + List candidates = List.of( + PlacementCandidate.profile("a", -1.0f, null), + PlacementCandidate.profile("b", 1.0f, null)); + for (int i = 0; i < 5; i++) { + assertEquals("b", policy.select(ctx(candidates, noSessions())).profile(), + "a has a negative weight, so it is excluded like weight 0 — never picked, never an error"); + } + } + + @Test + void weightZeroAndQuarantinedIsNotDoubleCounted() { + PlacementPolicy policy = PlacementPolicies.weighted(); + List candidates = List.of( + PlacementCandidate.profile("a", 0.0f, null), + PlacementCandidate.profile("b", 1.0f, null), + PlacementCandidate.profile("c", 1.0f, null)); + // a is BOTH weight-0 and quarantined; b and c are quarantined only. If a were counted + // into both buckets, "quarantined" would read 3 (or the arithmetic would go negative) + // instead of the true count of 2 candidates actually quarantined. + PlacementContext mixedCtx = ctx(candidates, noSessions(), Set.of(), Set.of("a", "b", "c")); + PlacementException e = assertThrows(PlacementException.class, () -> policy.select(mixedCtx)); + assertTrue(e.getMessage().contains("1 weight-0"), e.getMessage()); + assertTrue(e.getMessage().contains("2 quarantined"), e.getMessage()); + assertTrue(e.getMessage().contains("0 remaining"), e.getMessage()); + } + + @Test + void roundRobinSkipsWeightZeroProfiles() { + PlacementPolicy policy = PlacementPolicies.roundRobin(); + List candidates = List.of( + PlacementCandidate.profile("a", 0.0f, null), + PlacementCandidate.profile("b", 1.0f, null)); + for (int i = 0; i < 5; i++) { + assertEquals("b", policy.select(ctx(candidates, noSessions())).profile(), + "a has weight 0, so every pick lands on b"); + } + } + + @Test + void roundRobinThrowsWhenAllWeightZero() { + PlacementPolicy policy = PlacementPolicies.roundRobin(); + List candidates = List.of( + PlacementCandidate.profile("a", 0.0f, null), + PlacementCandidate.profile("b", 0.0f, null)); + PlacementException e = assertThrows(PlacementException.class, + () -> policy.select(ctx(candidates, noSessions()))); + assertTrue(e.getMessage().contains("weight 0"), e.getMessage()); + } + + @Test + void fixedSkipsWeightZeroDefault() { + PlacementPolicy policy = PlacementPolicies.fixed(); + PlacementContext ctx = new PlacementContext("b", + List.of(PlacementCandidate.profile("a", 1.0f, null), + PlacementCandidate.profile("b", 0.0f, null)), + noSessions(), Set.of(), Set.of()); + assertEquals("a", policy.select(ctx).profile(), + "the default 'b' has weight 0, so fixed falls through to the first available candidate"); + } + + @Test + void fixedThrowsWhenDefaultAndEveryCandidateWeightZero() { + PlacementPolicy policy = PlacementPolicies.fixed(); + PlacementContext ctx = new PlacementContext("b", + List.of(PlacementCandidate.profile("a", 0.0f, null), + PlacementCandidate.profile("b", 0.0f, null)), + noSessions(), Set.of(), Set.of()); + PlacementException e = assertThrows(PlacementException.class, () -> policy.select(ctx)); + assertTrue(e.getMessage().contains("weight 0"), e.getMessage()); + } + @Test void unknownPolicyNameThrows() { assertThrows(IllegalArgumentException.class, () -> PlacementPolicies.fromName("random"));