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

CB-553: maxLoad is not enforced on explicit-profile spawns (the cap is effectively dead config) #25

New Issue
Closed
opened 2026-08-13 20:58:39 +02:00 by ltms · 0 comments
ltms commented 2026-08-13 20:58:39 +02:00
Owner
Copy Link
Copy Source

Symptom

Live right now: 4 workers on profile gx10, whose maxLoad is 2.

gx10  term_658f1a8e9937c32  (mine)
gx10  term_658f1a8ad9ae631  (mine)
gx10  term_658f16e9a3e8b30  (mine)
gx10  term_658f237a0993c34  (peer lead's)

Root cause

CompositePeerLauncher.spawn returns before the placement policy is ever consulted when the
caller names a profile:

String requestedProfile = req.profileName();
if (requestedProfile != null && !requestedProfile.isBlank()) {
    // An explicit profile bypasses the policy entirely.
    HerdrPeerLauncher d = route(requestedProfile);
    PeerHandle handle = d.spawn(req);          // <-- no maxLoad check anywhere on this path
    spawnedBy.put(handle.id(), d);
    return handle;
}

maxLoad is only ever read inside PlacementPolicyUtil.available(ctx) / emptyException(ctx),
i.e. exclusively on the unqualified path.

Why this is a bug and not a design choice

BridgedConfig.Worker documents the field unconditionally:

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.

No qualifier about placement. The doc promises a capacity cap; the code implements a
placement tie-breaker input. One of the two is wrong, and the doc states the intent.

Severity is higher than it first looks. The project charter instructs every lead to always
name a profile explicitly ("profiles differ in model and cost, not in tier, so the default is
rarely what you want"). So the bypass path is the normal path — maxLoad is close to dead
config in real operation, and its only observable effect today is on spawns nobody makes.

This also silently removes the cap the architect model depends on: sonnet is
subscription: true with maxLoad: 2 ("one per architect slot"), and architects are spawned
by explicit profile. Today nothing stops an Nth architect billing the operator's Claude
subscription.

Fix

Enforce the cap on both paths. Admission ("may this profile take another worker?") is a
separate concern from placement ("which profile should this go to?") — placement already
consults it, the explicit path must too.

  • On the explicit-profile path, compare liveCount(profile) against that profile's maxLoad
    before delegating, and throw PlacementException when at cap.
  • PlacementException is the right type; it already documents itself as the way callers
    "distinguish 'no capacity' from a spawn-time transport failure". Message must name the
    profile, the live count, and the cap.
  • Failing loudly is correct here. An explicit-profile spawn must not silently fall back to
    another profile: the caller named that profile for a reason (cost/model), and re-routing a
    paid-tier request elsewhere is worse than refusing it.

Known limitation to document, not to fix here

liveCount is read outside any lock and SessionManager registers a session only after
launcher.spawn returns, so two truly concurrent spawns can both pass the check. That race
already exists on the placement path and closing it properly means slot reservation in the
registry. Enforce sequentially, state the race in a comment, and leave it.

Acceptance criteria

  • Explicit-profile spawn at cap throws PlacementException naming profile, live count, cap.
  • Explicit-profile spawn under cap is unchanged.
  • Unqualified/placement path behaviour is unchanged (existing placement tests still pass).
  • A profile with no maxLoad (null ⇒ unlimited) is never capped.
  • The TOCTOU limitation is documented in-code.
## Symptom Live right now: **4 workers on profile `gx10`, whose `maxLoad` is `2`.** ``` gx10 term_658f1a8e9937c32 (mine) gx10 term_658f1a8ad9ae631 (mine) gx10 term_658f16e9a3e8b30 (mine) gx10 term_658f237a0993c34 (peer lead's) ``` ## Root cause `CompositePeerLauncher.spawn` returns before the placement policy is ever consulted when the caller names a profile: ```java String requestedProfile = req.profileName(); if (requestedProfile != null && !requestedProfile.isBlank()) { // An explicit profile bypasses the policy entirely. HerdrPeerLauncher d = route(requestedProfile); PeerHandle handle = d.spawn(req); // <-- no maxLoad check anywhere on this path spawnedBy.put(handle.id(), d); return handle; } ``` `maxLoad` is only ever read inside `PlacementPolicyUtil.available(ctx)` / `emptyException(ctx)`, i.e. exclusively on the *unqualified* path. ## Why this is a bug and not a design choice `BridgedConfig.Worker` documents the field unconditionally: > `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. No qualifier about placement. The doc promises a capacity cap; the code implements a *placement tie-breaker input*. One of the two is wrong, and the doc states the intent. **Severity is higher than it first looks.** The project charter instructs every lead to always name a profile explicitly ("profiles differ in model and cost, not in tier, so the default is rarely what you want"). So the bypass path is the *normal* path — `maxLoad` is close to dead config in real operation, and its only observable effect today is on spawns nobody makes. This also silently removes the cap the architect model depends on: `sonnet` is `subscription: true` with `maxLoad: 2` ("one per architect slot"), and architects are spawned by explicit profile. Today nothing stops an Nth architect billing the operator's Claude subscription. ## Fix Enforce the cap on both paths. Admission ("may this profile take another worker?") is a separate concern from placement ("which profile should this go to?") — placement already consults it, the explicit path must too. - On the explicit-profile path, compare `liveCount(profile)` against that profile's `maxLoad` before delegating, and throw `PlacementException` when at cap. - `PlacementException` is the right type; it already documents itself as the way callers "distinguish 'no capacity' from a spawn-time transport failure". Message must name the profile, the live count, and the cap. - Failing loudly is correct here. An explicit-profile spawn must **not** silently fall back to another profile: the caller named that profile for a reason (cost/model), and re-routing a paid-tier request elsewhere is worse than refusing it. ## Known limitation to document, not to fix here `liveCount` is read outside any lock and `SessionManager` registers a session only *after* `launcher.spawn` returns, so two truly concurrent spawns can both pass the check. That race already exists on the placement path and closing it properly means slot reservation in the registry. Enforce sequentially, state the race in a comment, and leave it. ## Acceptance criteria - Explicit-profile spawn at cap throws `PlacementException` naming profile, live count, cap. - Explicit-profile spawn under cap is unchanged. - Unqualified/placement path behaviour is unchanged (existing placement tests still pass). - A profile with no `maxLoad` (null ⇒ unlimited) is never capped. - The TOCTOU limitation is documented in-code.
ltms closed this issue 2026-08-15 07:13:23 +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
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#25
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?