diff --git a/bridged/bridged.example.yaml b/bridged/bridged.example.yaml index 2c4f1fe..b4d730c 100644 --- a/bridged/bridged.example.yaml +++ b/bridged/bridged.example.yaml @@ -190,7 +190,11 @@ profiles: # `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) + # maxLoad: max live workers on this profile. Omit for unlimited. An explicit 0 (CB-585) caps + # the profile at zero live members — it is excluded from automatic placement and an explicit + # `bridge_spawn{profile:"gx10"}` against it is refused too; a cap holds even when the profile + # is named directly. Negative is refused at config load — there is no sane meaning for it. + maxLoad: 2 # 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 # exhaustedPattern: "usage limit has been reached" # opt-in: classify a usage-limit refusal (CB-578) 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 4c88a05..9bbb187 100644 --- a/bridged/src/main/java/dev/ltms/bridged/config/BridgedConfig.java +++ b/bridged/src/main/java/dev/ltms/bridged/config/BridgedConfig.java @@ -210,9 +210,20 @@ public record BridgedConfig( * 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. + * @param maxLoad max live workers allowed on this profile at one time. Absent + * (unset/{@code null}) ⇒ unlimited — most profiles rely on this. An + * explicit {@code 0} (CB-585) means "cap this profile at zero live + * members": it is excluded from every automatic policy's candidate pool + * the same way a {@code weight <= 0} profile is (see + * {@code PlacementPolicyUtil.available()}, which already treats "at cap" + * and "excluded" alike), and an explicit + * {@code bridge_spawn{profile:"..."}} against it is refused too (see + * {@code CompositePeerLauncher.enforceMaxLoad}) — a cap is a capacity + * statement that does not stop being true just because the profile was + * named directly. A negative value has no sane meaning (there is no + * "excluded" to degrade to below zero) and is refused at config load + * instead, naming the profile and the key. Live means any session the + * registry still owns (acquired and not yet released), in any state. * @param kind which peer launcher spawns this profile: {@code "claude-code"} (default — * the {@link dev.ltms.bridged.member.ClaudeCodeLauncher}) or {@code "opencode"}. * The {@code CompositePeerLauncher} routes {@code spawn}/reap by this value, so @@ -306,7 +317,12 @@ public record BridgedConfig( // 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; + // CB-585: absent still means unlimited (null), but an explicit maxLoad: 0 must survive + // as "capped at zero," not get coerced back up to null/unlimited — that coercion was + // the bug (maxLoad: 0 read as "never run anything here" and behaved as the opposite, + // the one throttle a subscription: true profile has against the operator's own paid + // plan). A negative value is refused earlier, at config load (rejectNegativeMaxLoad), + // so it never reaches this constructor and needs no clamping here. subscription = (subscription != null && subscription) ? Boolean.TRUE : Boolean.FALSE; // exhaustedPattern stays null when unset/blank (opt-in) — no defaulting, no vendor // wording: an unconfigured profile keeps today's completion-fallback behaviour exactly. @@ -956,6 +972,7 @@ public record BridgedConfig( rejectLeaderTerminalKey(yaml); warnUnknownTopLevelKeys(yaml, path); rejectDuplicateMemberSlots(yaml); + rejectNegativeMaxLoad(yaml); BridgedConfig cfg = YAML.readValue(yaml, BridgedConfig.class); return cfg.withDefaults(); } catch (IOException e) { @@ -1215,6 +1232,41 @@ public record BridgedConfig( } } + /** + * Reject a profile whose {@code maxLoad:} is negative, naming both the profile and the key. + * + *

Unlike {@code weight} (CB-554), where a negative value degrades to the same "excluded" + * meaning as an explicit {@code 0}, {@code maxLoad} has nowhere lower to degrade to — {@code 0} + * already means "capped at zero live members" (CB-585), the strictest cap there is. Silently + * normalising a negative value to something else is exactly the shape of bug this ticket fixes + * for {@code 0}, so it is refused instead, loud and specific, rather than guessed at. + * + * @param yaml the raw config text + * @throws IllegalStateException when any profile's {@code maxLoad} is negative + */ + static void rejectNegativeMaxLoad(String yaml) { + Map raw; + try { + raw = YAML.readValue(yaml, Map.class); + } catch (IOException | IllegalArgumentException e) { + return; // a malformed file is reported by the real parse, not here + } + if (raw == null || !(raw.get("profiles") instanceof Map profiles)) { + return; + } + List bad = profiles.entrySet().stream() + .filter(e -> e.getValue() instanceof Map p + && p.get("maxLoad") instanceof Number n && n.doubleValue() < 0) + .map(e -> String.valueOf(e.getKey())) + .sorted() + .toList(); + if (!bad.isEmpty()) { + throw new IllegalStateException("refusing to start: profile(s) [" + String.join(", ", bad) + + "] set a negative maxLoad — maxLoad caps live workers at a non-negative count;" + + " use 0 to cap a profile at zero live members, or omit the key for unlimited."); + } + } + static List unknownTopLevelKeys(String yaml) { Map raw; try { 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 6cd306d..0cda820 100644 --- a/bridged/src/test/java/dev/ltms/bridged/config/BridgedConfigTest.java +++ b/bridged/src/test/java/dev/ltms/bridged/config/BridgedConfigTest.java @@ -1257,6 +1257,44 @@ class BridgedConfigTest { 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 explicitMaxLoadZeroStaysZeroInsteadOfCoercingToUnlimited(@TempDir Path dir) throws Exception { + // CB-585: maxLoad: 0 used to be normalised to null (unlimited) by this same compact + // constructor, so an operator writing it to mean "never run anything here" got the exact + // opposite. On a subscription: true profile, maxLoad is the only throttle against the + // operator's own paid plan. + Path f = dir.resolve("maxload-zero.yaml"); + Files.writeString(f, """ + bind: + port: 8080 + profiles: + opus: + baseUrl: http://gx10.gw:8000 + maxLoad: 0 + """); + + BridgedConfig cfg = BridgedConfig.load(f); + BridgedConfig.Profile w = cfg.profiles().get("opus"); + assertEquals(0, w.maxLoad(), "an explicit maxLoad: 0 must stay 0, not coerce to unlimited (null)"); + } + + @Test + void negativeMaxLoadIsRefusedAtLoadNamingTheProfileAndKey(@TempDir Path dir) throws Exception { + Path f = dir.resolve("maxload-negative.yaml"); + Files.writeString(f, """ + bind: + port: 8080 + profiles: + opus: + baseUrl: http://gx10.gw:8000 + maxLoad: -2 + """); + + IllegalStateException e = assertThrows(IllegalStateException.class, () -> BridgedConfig.load(f)); + assertTrue(e.getMessage().contains("opus"), "error names the profile: " + e.getMessage()); + assertTrue(e.getMessage().contains("maxLoad"), "error names the key: " + e.getMessage()); + } + @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 3028831..6fc01b0 100644 --- a/bridged/src/test/java/dev/ltms/bridged/member/CompositePeerLauncherTest.java +++ b/bridged/src/test/java/dev/ltms/bridged/member/CompositePeerLauncherTest.java @@ -473,6 +473,27 @@ class CompositePeerLauncherTest { assertEquals("a", h.profile(), "a profile with no maxLoad is never capped, however many live workers"); } + @Test + void explicitSpawnOnMaxLoadZeroProfileIsRefusedEvenWithZeroLiveWorkers() { + // CB-585: before the fix, maxLoad: 0 normalised to null (unlimited) in the compact + // constructor, so this exact case — naming a zero-cap profile explicitly, with nothing + // live on it yet — would have spawned instead of refusing. "at most zero members" must + // hold even when the profile is named directly, not only against automatic placement. + FakeHerdr herdr = new FakeHerdr(); + Map profiles = ordered( + "a", stubWorker("a", 1.0f, 0), + "b", stubWorker("b")); + StubLauncher adapter = new StubLauncher("claude", herdr, profiles, "a", Set.of()); + CompositePeerLauncher composite = new CompositePeerLauncher( + List.of(adapter), "a", profiles, PlacementPolicies.fixed(), _ -> 0); + + PlacementException e = assertThrows(PlacementException.class, + () -> composite.spawn(new SpawnRequest("a", null, null))); + assertTrue(e.getMessage().contains("'a'"), "message names the profile: " + e.getMessage()); + assertTrue(e.getMessage().contains("0 cap"), "message names the zero cap: " + e.getMessage()); + assertEquals(0, adapter.spawnCount("a"), "a maxLoad: 0 profile accepts no explicit spawn"); + } + @Test void emptyCandidateSetThrowsClearException() { 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 cc2058f..c5dc42f 100644 --- a/bridged/src/test/java/dev/ltms/bridged/placement/PlacementPolicyTest.java +++ b/bridged/src/test/java/dev/ltms/bridged/placement/PlacementPolicyTest.java @@ -340,4 +340,31 @@ class PlacementPolicyTest { void unknownPolicyNameThrows() { assertThrows(IllegalArgumentException.class, () -> PlacementPolicies.fromName("random")); } + + // --- CB-585: maxLoad 0 caps a profile at zero live members, even with nothing live yet ----- + + @Test + void weightedSkipsMaxLoadZeroProfileEvenWithZeroLiveWorkers() { + PlacementPolicy policy = PlacementPolicies.weighted(); + List candidates = List.of( + PlacementCandidate.profile("a", 1.0f, 0), + PlacementCandidate.profile("b", 1.0f, null)); + Function liveCount = _ -> 0; + for (int i = 0; i < 5; i++) { + assertEquals("b", policy.select(ctx(candidates, liveCount)).profile(), + "a has maxLoad 0, so it is already at its cap with nobody live — every pick lands on b"); + } + } + + @Test + void weightedThrowsWhenAllMaxLoadZero() { + PlacementPolicy policy = PlacementPolicies.weighted(); + List candidates = List.of( + PlacementCandidate.profile("a", 1.0f, 0), + PlacementCandidate.profile("b", 1.0f, 0)); + Function liveCount = _ -> 0; + PlacementException e = assertThrows(PlacementException.class, + () -> policy.select(ctx(candidates, liveCount))); + assertTrue(e.getMessage().contains("maxLoad"), e.getMessage()); + } }