CB-585: maxLoad: 0 caps a profile at zero, negative refused at load
This commit is contained in:
@@ -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)
|
||||
|
||||
@@ -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.
|
||||
*
|
||||
* <p>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<String> 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<String> unknownTopLevelKeys(String yaml) {
|
||||
Map<?, ?> raw;
|
||||
try {
|
||||
|
||||
@@ -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");
|
||||
|
||||
@@ -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<String, BridgedConfig.Profile> 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();
|
||||
|
||||
@@ -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<PlacementCandidate> candidates = List.of(
|
||||
PlacementCandidate.profile("a", 1.0f, 0),
|
||||
PlacementCandidate.profile("b", 1.0f, null));
|
||||
Function<String, Integer> 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<PlacementCandidate> candidates = List.of(
|
||||
PlacementCandidate.profile("a", 1.0f, 0),
|
||||
PlacementCandidate.profile("b", 1.0f, 0));
|
||||
Function<String, Integer> liveCount = _ -> 0;
|
||||
PlacementException e = assertThrows(PlacementException.class,
|
||||
() -> policy.select(ctx(candidates, liveCount)));
|
||||
assertTrue(e.getMessage().contains("maxLoad"), e.getMessage());
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user