CB-554: weight <= 0 excludes a profile from automatic placement, not 1.0
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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} <em>only</em> 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
|
||||
|
||||
@@ -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.
|
||||
*
|
||||
* <p>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.
|
||||
* <p>Two exceptions walk past the default instead of returning it unconditionally:
|
||||
* <ul>
|
||||
* <li>Quarantine (CB-578 stage B): a quarantined default is a credential that just refused on
|
||||
* a usage limit, not a transient capacity or reachability concern.
|
||||
* <li>Weight 0 (CB-554): {@code fixed} is still automatic selection, so a profile the operator
|
||||
* marked "never auto-select me" ({@code weight <= 0}) must be skipped here exactly as
|
||||
* {@code weighted}/{@code round-robin} skip it — an explicit {@code bridge_spawn} naming
|
||||
* the profile is unaffected, only this automatic fallback walk.
|
||||
* </ul>
|
||||
* 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;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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.
|
||||
*
|
||||
* <p>Exclusion is about <em>automatic</em> selection only — an explicit
|
||||
* {@code bridge_spawn{profile:"..."}} bypasses placement entirely and is unaffected.
|
||||
*/
|
||||
public boolean excluded() {
|
||||
return weight <= 0.0f;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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<PlacementCandidate> available(PlacementContext ctx) {
|
||||
List<PlacementCandidate> 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");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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");
|
||||
|
||||
@@ -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<String, BridgedConfig.Profile> 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<String, BridgedConfig.Profile> 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();
|
||||
|
||||
@@ -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<PlacementCandidate> 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<PlacementCandidate> 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<PlacementCandidate> 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<PlacementCandidate> 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<PlacementCandidate> 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<PlacementCandidate> 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"));
|
||||
|
||||
Reference in New Issue
Block a user