fleet/fleetd
3
0
Fork 0
Code Issues 73 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

CB-585: maxLoad: 0 silently means unlimited — the same trap CB-554 just fixed for weight #66

New Issue
Closed
opened 2026-08-15 13:10:48 +02:00 by ltms · 1 comment
ltms commented 2026-08-15 13:10:48 +02:00
Owner
Copy Link
Copy Source

Split out of issue #28. CB-554 fixed weight: 0, but #28's last acceptance criterion was about maxLoad and is not met. Filing it rather than leaving #28's list quietly incomplete.

The defect

BridgedConfig.Profile's compact constructor still has:

maxLoad = (maxLoad == null || maxLoad <= 0) ? null : maxLoad;

null means unlimited. So an operator who writes maxLoad: 0 to mean "never run anything here" gets the exact opposite: no cap at all.

This is worse than the weight version CB-554 fixed. A wrong weight changes how often a profile is picked. A wrong maxLoad removes the only throttle there is — and on a subscription: true profile, maxLoad is what stands between the fleet and the operator's own plan. bridged.yaml says so directly:

maxLoad is the ONLY throttle on it: subscription: true means every member here bills the operator's own plan.

Why it survived CB-554

CB-554's fix distinguished "absent" from "explicit 0" for weight:

weight = (weight == null) ? 1.0f : Math.max(weight, 0.0f);

The maxLoad line one below it still collapses both cases into null. The same reasoning applies and the same shape of fix works.

Scope

Decide between the two honest readings and implement one:

  1. maxLoad: 0 means zero — the profile accepts no members. Consistent with what CB-554 made weight: 0 mean, and with how an operator reads it.
  2. maxLoad: 0 is rejected at config load — refuse to start with a message naming the key, the way CB-579 refuses to start on a stale terminal: pin.

Either is defensible. What is not defensible is the current silent inversion.

A negative value should be rejected at load in both readings — there is no sane meaning for it, and normalising it away is what hid this.

Absent must keep meaning unlimited. That is the documented behaviour and most profiles rely on it.

Acceptance criteria

  1. maxLoad: 0 either caps the profile at zero live members or refuses config load with a message naming the key — never silently unlimited.
  2. A negative maxLoad is refused at config load with a message naming the key.
  3. An absent maxLoad still means unlimited, unchanged.
  4. If reading 1 is chosen, an unqualified spawn skips a maxLoad: 0 profile through the existing PlacementPolicyUtil.available() cap check — no new filter, the cap path already handles it.
  5. If reading 1 is chosen, decide and test what an explicit bridge_spawn{profile: "..."} does against a maxLoad: 0 profile. weight: 0 deliberately does not block an explicit spawn, but a cap is a different kind of statement, and the answer must be a decision rather than an accident.

Note

Read the silent-default note on issue #50 before starting. Make any new dependency required, with no convenience overload.

Split out of issue #28. CB-554 fixed `weight: 0`, but #28's last acceptance criterion was about `maxLoad` and is **not** met. Filing it rather than leaving #28's list quietly incomplete. ## The defect `BridgedConfig.Profile`'s compact constructor still has: ```java maxLoad = (maxLoad == null || maxLoad <= 0) ? null : maxLoad; ``` `null` means **unlimited**. So an operator who writes `maxLoad: 0` to mean "never run anything here" gets the exact opposite: no cap at all. This is worse than the `weight` version CB-554 fixed. A wrong weight changes how often a profile is picked. A wrong `maxLoad` removes the only throttle there is — and on a `subscription: true` profile, `maxLoad` is what stands between the fleet and the operator's own plan. `bridged.yaml` says so directly: > `maxLoad` is the ONLY throttle on it: `subscription: true` means every member here bills the operator's own plan. ## Why it survived CB-554 CB-554's fix distinguished "absent" from "explicit 0" for weight: ```java weight = (weight == null) ? 1.0f : Math.max(weight, 0.0f); ``` The `maxLoad` line one below it still collapses both cases into `null`. The same reasoning applies and the same shape of fix works. ## Scope Decide between the two honest readings and implement one: 1. **`maxLoad: 0` means zero** — the profile accepts no members. Consistent with what CB-554 made `weight: 0` mean, and with how an operator reads it. 2. **`maxLoad: 0` is rejected at config load** — refuse to start with a message naming the key, the way CB-579 refuses to start on a stale `terminal:` pin. Either is defensible. What is not defensible is the current silent inversion. A **negative** value should be rejected at load in both readings — there is no sane meaning for it, and normalising it away is what hid this. Absent must keep meaning unlimited. That is the documented behaviour and most profiles rely on it. ## Acceptance criteria 1. `maxLoad: 0` either caps the profile at zero live members or refuses config load with a message naming the key — never silently unlimited. 2. A negative `maxLoad` is refused at config load with a message naming the key. 3. An absent `maxLoad` still means unlimited, unchanged. 4. If reading 1 is chosen, an unqualified spawn skips a `maxLoad: 0` profile through the existing `PlacementPolicyUtil.available()` cap check — no new filter, the cap path already handles it. 5. If reading 1 is chosen, decide and test what an **explicit** `bridge_spawn{profile: "..."}` does against a `maxLoad: 0` profile. `weight: 0` deliberately does not block an explicit spawn, but a cap is a different kind of statement, and the answer must be a decision rather than an accident. ## Note Read the silent-default note on issue #50 before starting. Make any new dependency required, with no convenience overload.
ltms added the ready-to-delegatesilent-default labels 2026-08-15 13:11:13 +02:00
ltms commented 2026-08-15 16:01:34 +02:00
Author
Owner
Copy Link
Copy Source

Merged to main at 30e3225. My own build on the merged tree: 778 tests, 0 failures, BUILD SUCCESS, exit 0 (unpiped). CI green on the PR head (run 1195).

Config impact checked before merging, as requested. The live bridged.yaml has maxLoad values of 2, 1, 3, 1 and 2 across its five profiles — no zero, no negative. So this changes nothing about the running fleet, and the daemon will still start. Asking for that check in the reply was the right call: it is exactly what went wrong with CB-554 this morning, where a gitignored config used a key whose meaning the merge changed.

On criterion 5: the finding that CB-553 already enforces the cap unconditionally on the explicit-spawn path is the useful result here. It means "explicit spawn is refused" needed no new code at all — only a test that fails against the old behaviour and passes against the new. That is a better outcome than building the check, and the reasoning was verified rather than assumed.

The out-of-scope note about CompositePeerLauncher.enforceMaxLoad's stale comment ("non-positive ⇒ unlimited at load") is correct, and staying out of a file not on the list was right. I will fix that comment directly — it is one line and does not need a ticket.

Merged to `main` at `30e3225`. My own build on the merged tree: **778 tests, 0 failures, BUILD SUCCESS, exit 0** (unpiped). CI green on the PR head (run 1195). **Config impact checked before merging, as requested.** The live `bridged.yaml` has `maxLoad` values of 2, 1, 3, 1 and 2 across its five profiles — no zero, no negative. So this changes nothing about the running fleet, and the daemon will still start. Asking for that check in the reply was the right call: it is exactly what went wrong with CB-554 this morning, where a gitignored config used a key whose meaning the merge changed. On criterion 5: the finding that CB-553 already enforces the cap unconditionally on the explicit-spawn path is the useful result here. It means "explicit spawn is refused" needed no new code at all — only a test that fails against the old behaviour and passes against the new. That is a better outcome than building the check, and the reasoning was verified rather than assumed. The out-of-scope note about `CompositePeerLauncher.enforceMaxLoad`'s stale comment ("non-positive ⇒ unlimited at load") is correct, and staying out of a file not on the list was right. I will fix that comment directly — it is one line and does not need a ticket.
ltms closed this issue 2026-08-15 16:01:51 +02:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
blocked
Cannot start until something else lands. The body says what.
 needs-live-proof
Merged and green, but never shown working on the running daemon. Not the same as done.
 ready-to-delegate
Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.
 silent-default
A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.
No Label ready-to-delegate silent-default
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent kevin ltms
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#66
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?