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

The fixed placement policy ignores the retry loop's unreachable set, so an unqualified spawn retries the same dead profile and never tries the healthy one #315

New Issue
Closed
opened 2026-09-04 08:44:05 +02:00 by ltms · 1 comment
ltms commented 2026-09-04 08:44:05 +02:00
Owner
Copy Link
Copy Source

Found by a delegated hunter. I read every line cited and confirmed it, and I answered the one question the worker could not.

Not live on this fleet — say that first

The hunter flagged that it could not read fleetd.yaml (gitignored) and so could not tell whether this is live. I can read it:

fleetd/fleetd.yaml:198: placement: weighted

weighted goes through PlacementPolicyUtil.available(), which does consult ctx.unreachable(). So this defect is dormant here. It is live for any deployment that leaves the placement key unset or sets it to fixed, which is the documented backward-compatible default — including a fresh install and possibly fleet01, whose config I have not read.

Do not write this up as though it has been costing us spawns. It has not.

The contradiction

CompositePeerLauncher.spawn retries on a dead backend, and tells you what it expects of the policy:

} catch (PeerUnreachableException e) {
    log.warn("spawn on profile {} unreachable, will retry next candidate if any: {}", ...);
    unreachable.add(chosen.profile());
    // Update the context for the next selection so the policy excludes this profile.
    ctx = new PlacementContext(roleDefault, candidates, liveCount, unreachable,
            quarantined, coolingOff);
}

FixedPlacementPolicy.select never reads that set. grep -n unreachable FixedPlacementPolicy.java returns nothing. It checks quarantined, coolingOff and weight-0, and nothing else:

String d = ctx.defaultProfile();
if (d != null && !d.isBlank() && !ctx.quarantined().contains(d) && !ctx.coolingOff().contains(d)
        && !weightExcluded(ctx, d)) {
    return new PlacementCandidate(d, null, 1.0f, null);
}

The loop bound is the tell: int maxAttempts = candidates.isEmpty() ? 1 : candidates.size();. That bound only makes sense if each attempt tries a different candidate. Under fixed, all of them pick the same one.

The path in

Pool [opus, sonnet], default opus, placement unset.

  1. Unqualified fleet_spawn (no profile). maxAttempts = 2.
  2. Attempt 1 selects opus. d.spawn throws PeerUnreachableException — a real path: HerdrPeerLauncher.waitUntilInjectableOrThrow / failFastOnGoneBackend throw it on a stuck or dead backend. unreachable = {opus}.
  3. Attempt 2 rebuilds ctx, calls select again. opus is still not quarantined, not cooling off, not weight-0 — and unreachable is not consulted. It returns opus again. Fails again.
  4. Loop ends. Throws PeerUnreachableException("no reachable worker profile available after trying 1 candidate(s): opus").

sonnet was configured, healthy, and never attempted. The message also undercounts: it says 1 candidate when 2 attempts ran, because unreachable is a HashSet and the same profile was added twice.

Direction of harm: a wedge with a misleading error. Available capacity goes unused, and the message tells the operator only one profile was tried — which is true of profiles but not of attempts, and reads as "your pool is one profile deep".

A second path in, which the hunter did not report

The wiring-bug branch a few lines above has the same root cause:

HerdrPeerLauncher d = byProfile.get(chosen.profile());
if (d == null) {
    // A configured profile with no adapter is a wiring bug; fail fast.
    unreachable.add(chosen.profile());
    continue;
}

The comment says "fail fast". Under fixed it does not: the next select returns the same adapterless profile, d is null again, and the loop spins to maxAttempts before throwing. Same fix covers both.

Is ignoring reachability deliberate? I checked, and no

FixedPlacementPolicy's own javadoc says it "ignores caps and reachability so that a pre-existing config behaves identically after upgrade", so this deserved a second look before being called a bug. Two things say it is a bug:

  1. fixed already walks past the default to other candidates. When the default is quarantined, cooling off, or weight-0, the for (PlacementCandidate c : ctx.candidates()) loop picks a different profile. So "fixed means only ever the default" is already not true. An unreachable default is unusable in exactly the same way as a quarantined one.
  2. That "ignores everything" stance has already been narrowed three times — CB-578 (quarantine), fleetd #201 (cooling off), CB-554 (weight 0), each carved out in its own ticket for this same reason. Reachability is the fourth of the same shape.

Why no test caught it

All three failover tests in CompositePeerLauncherTest construct the composite with PlacementPolicies.weighted():

566: failoverRetriesNextCandidateWhenProfileIsUnreachable        → 573: PlacementPolicies.weighted()
587: reversingDefinitionOrderReversesWhichProfileIsTriedFirst    → 594: PlacementPolicies.weighted()
602: failoverBoundedByCandidateCount                             → 609: PlacementPolicies.weighted()

fixed() appears in many other tests in that file, but never in a failover one. The default policy's failover path has no coverage at all. This is the "a test on the seam does not prove the caller" shape: the retry loop is tested, but only through the one policy that happens to honour its contract.

What I want

Goal: an unqualified spawn must not retry a profile that has already failed as unreachable in this same call, whichever placement policy is configured. The retry loop's stated contract — "the policy excludes this profile" — must actually hold for every policy.

Invariants:

  1. First-choice behaviour must not change. On attempt 1, with an empty unreachable set, fixed must still return the default profile exactly as it does today. This ticket is about the retry, not about which profile is picked first, and a change to the first choice would move work onto profiles an operator did not choose — some of them paid.
  2. An explicit fleet_spawn{profile: "x"} is untouched. That path does not go through placement at all, and it must stay that way — it is the operator overriding on purpose.
  3. The error message must not lie about how many candidates were tried. If you fix the loop, check that message too: it currently reports unreachable.size(), which is a count of distinct profiles, under a sentence that reads as a count of attempts. Say which one it should be and make it that.

Candidate mechanism, offered as a candidate only: have FixedPlacementPolicy consult ctx.unreachable() in the same two places it already consults quarantined and coolingOff — the default check and the fallback walk. Decide it yourself and justify it. If you think the right fix belongs in CompositePeerLauncher instead (for example, breaking the loop when select returns a profile already in unreachable, which would fix every present and future policy at once rather than one of them), say so and do that. A tested, reported deviation is a good outcome here. Consider both and say why you chose the one you chose — I genuinely do not know which is right, and the second option is attractive because it makes the loop enforce its own contract instead of trusting each policy to.

Also fix the javadoc. FixedPlacementPolicy's "ignores caps and reachability" sentence is what made this invisible, and the list of three carve-outs below it needs a fourth entry if you add one.

Rules

  • Prove it with a test that fails without the fix: an unqualified spawn under PlacementPolicies.fixed(), first profile unreachable, must land on the second. Put it next to the three existing failover tests.
  • Cover the d == null branch too, or say why you judged it not worth a separate test.
  • Mutation proof required: revert the fix, quote the real failure output, restore it.
  • Do not run git stash — the stash is shared across every worktree here and you would take another worker's in-progress work.
  • Do not run git worktree remove or git worktree prune — other workers are live in these worktrees.
  • Run cd fleetd && mvn clean install unpiped, and quote the real Tests run: and BUILD lines. Never pipe maven through tail/head, and never read $? after a pipe — after a pipe it is the pipe's last command's status, not Maven's.
  • You cannot read fleetd.yaml. You do not need to: I have read it and the answer is above.

Shape check

When done, look in placement/ and member/CompositePeerLauncher.java only for the same shape: a caller that documents an expectation of its collaborator in a comment, where at least one implementation of that collaborator does not meet it. One line each, do not fix any of it.

Found by a delegated hunter. I read every line cited and confirmed it, and I answered the one question the worker could not. ## Not live on this fleet — say that first The hunter flagged that it could not read `fleetd.yaml` (gitignored) and so could not tell whether this is live. I can read it: ``` fleetd/fleetd.yaml:198: placement: weighted ``` `weighted` goes through `PlacementPolicyUtil.available()`, which does consult `ctx.unreachable()`. **So this defect is dormant here.** It is live for any deployment that leaves the `placement` key unset or sets it to `fixed`, which is the documented backward-compatible default — including a fresh install and possibly fleet01, whose config I have not read. Do not write this up as though it has been costing us spawns. It has not. ## The contradiction `CompositePeerLauncher.spawn` retries on a dead backend, and tells you what it expects of the policy: ```java } catch (PeerUnreachableException e) { log.warn("spawn on profile {} unreachable, will retry next candidate if any: {}", ...); unreachable.add(chosen.profile()); // Update the context for the next selection so the policy excludes this profile. ctx = new PlacementContext(roleDefault, candidates, liveCount, unreachable, quarantined, coolingOff); } ``` `FixedPlacementPolicy.select` never reads that set. `grep -n unreachable FixedPlacementPolicy.java` returns nothing. It checks `quarantined`, `coolingOff` and weight-0, and nothing else: ```java String d = ctx.defaultProfile(); if (d != null && !d.isBlank() && !ctx.quarantined().contains(d) && !ctx.coolingOff().contains(d) && !weightExcluded(ctx, d)) { return new PlacementCandidate(d, null, 1.0f, null); } ``` The loop bound is the tell: `int maxAttempts = candidates.isEmpty() ? 1 : candidates.size();`. That bound only makes sense if each attempt tries a *different* candidate. Under `fixed`, all of them pick the same one. ## The path in Pool `[opus, sonnet]`, default `opus`, `placement` unset. 1. Unqualified `fleet_spawn` (no `profile`). `maxAttempts = 2`. 2. Attempt 1 selects `opus`. `d.spawn` throws `PeerUnreachableException` — a real path: `HerdrPeerLauncher.waitUntilInjectableOrThrow` / `failFastOnGoneBackend` throw it on a stuck or dead backend. `unreachable = {opus}`. 3. Attempt 2 rebuilds `ctx`, calls `select` again. `opus` is still not quarantined, not cooling off, not weight-0 — and `unreachable` is not consulted. It returns `opus` again. Fails again. 4. Loop ends. Throws `PeerUnreachableException("no reachable worker profile available after trying 1 candidate(s): opus")`. `sonnet` was configured, healthy, and never attempted. The message also undercounts: it says 1 candidate when 2 attempts ran, because `unreachable` is a `HashSet` and the same profile was added twice. **Direction of harm:** a wedge with a misleading error. Available capacity goes unused, and the message tells the operator only one profile was tried — which is true of profiles but not of attempts, and reads as "your pool is one profile deep". ## A second path in, which the hunter did not report The wiring-bug branch a few lines above has the same root cause: ```java HerdrPeerLauncher d = byProfile.get(chosen.profile()); if (d == null) { // A configured profile with no adapter is a wiring bug; fail fast. unreachable.add(chosen.profile()); continue; } ``` The comment says "fail fast". Under `fixed` it does not: the next `select` returns the same adapterless profile, `d` is null again, and the loop spins to `maxAttempts` before throwing. Same fix covers both. ## Is ignoring reachability deliberate? I checked, and no `FixedPlacementPolicy`'s own javadoc says it "ignores caps and reachability so that a pre-existing config behaves identically after upgrade", so this deserved a second look before being called a bug. Two things say it is a bug: 1. **`fixed` already walks past the default to other candidates.** When the default is quarantined, cooling off, or weight-0, the `for (PlacementCandidate c : ctx.candidates())` loop picks a different profile. So "fixed means only ever the default" is already not true. An unreachable default is unusable in exactly the same way as a quarantined one. 2. **That "ignores everything" stance has already been narrowed three times** — CB-578 (quarantine), fleetd #201 (cooling off), CB-554 (weight 0), each carved out in its own ticket for this same reason. Reachability is the fourth of the same shape. ## Why no test caught it All three failover tests in `CompositePeerLauncherTest` construct the composite with `PlacementPolicies.weighted()`: ``` 566: failoverRetriesNextCandidateWhenProfileIsUnreachable → 573: PlacementPolicies.weighted() 587: reversingDefinitionOrderReversesWhichProfileIsTriedFirst → 594: PlacementPolicies.weighted() 602: failoverBoundedByCandidateCount → 609: PlacementPolicies.weighted() ``` `fixed()` appears in many other tests in that file, but never in a failover one. The default policy's failover path has no coverage at all. This is the "a test on the seam does not prove the caller" shape: the retry loop is tested, but only through the one policy that happens to honour its contract. ## What I want **Goal:** an unqualified spawn must not retry a profile that has already failed as unreachable in this same call, whichever placement policy is configured. The retry loop's stated contract — "the policy excludes this profile" — must actually hold for every policy. **Invariants:** 1. **First-choice behaviour must not change.** On attempt 1, with an empty `unreachable` set, `fixed` must still return the default profile exactly as it does today. This ticket is about the *retry*, not about which profile is picked first, and a change to the first choice would move work onto profiles an operator did not choose — some of them paid. 2. **An explicit `fleet_spawn{profile: "x"}` is untouched.** That path does not go through placement at all, and it must stay that way — it is the operator overriding on purpose. 3. **The error message must not lie about how many candidates were tried.** If you fix the loop, check that message too: it currently reports `unreachable.size()`, which is a count of distinct profiles, under a sentence that reads as a count of attempts. Say which one it should be and make it that. **Candidate mechanism, offered as a candidate only:** have `FixedPlacementPolicy` consult `ctx.unreachable()` in the same two places it already consults `quarantined` and `coolingOff` — the default check and the fallback walk. **Decide it yourself and justify it.** If you think the right fix belongs in `CompositePeerLauncher` instead (for example, breaking the loop when `select` returns a profile already in `unreachable`, which would fix every present and future policy at once rather than one of them), say so and do that. A tested, reported deviation is a good outcome here. Consider both and say why you chose the one you chose — I genuinely do not know which is right, and the second option is attractive because it makes the loop enforce its own contract instead of trusting each policy to. **Also fix the javadoc.** `FixedPlacementPolicy`'s "ignores caps and reachability" sentence is what made this invisible, and the list of three carve-outs below it needs a fourth entry if you add one. ## Rules - Prove it with a test that fails without the fix: an unqualified spawn under `PlacementPolicies.fixed()`, first profile unreachable, must land on the second. Put it next to the three existing failover tests. - Cover the `d == null` branch too, or say why you judged it not worth a separate test. - Mutation proof required: revert the fix, quote the real failure output, restore it. - Do not run `git stash` — the stash is shared across every worktree here and you would take another worker's in-progress work. - Do not run `git worktree remove` or `git worktree prune` — other workers are live in these worktrees. - Run `cd fleetd && mvn clean install` **unpiped**, and quote the real `Tests run:` and `BUILD` lines. Never pipe maven through `tail`/`head`, and never read `$?` after a pipe — after a pipe it is the pipe's last command's status, not Maven's. - You cannot read `fleetd.yaml`. You do not need to: I have read it and the answer is above. ## Shape check When done, look in `placement/` and `member/CompositePeerLauncher.java` only for the same shape: **a caller that documents an expectation of its collaborator in a comment, where at least one implementation of that collaborator does not meet it.** One line each, do **not** fix any of it.
ltms commented 2026-09-04 08:59:13 +02:00
Author
Owner
Copy Link
Copy Source

Merged to main in 77ad886.

My alternative mechanism was wrong, and the worker was right to reject it

I offered two candidates and said the second was attractive because it "makes the loop enforce its own contract instead of trusting each policy to". The worker worked it through and found it does not solve the problem:

breaking early only makes the loop fail faster on the same dead profile — it cannot advance to a different candidate, because only the policy decides what's next.

That is correct, and I should have seen it. CompositePeerLauncher has no selection logic of its own; breaking out of the loop when select returns an already-failed profile just reaches the same PeerUnreachableException sooner. To advance to sonnet the loop would have to reimplement each policy's choice, which is the duplication the policy interface exists to avoid. The fix belongs in the one policy that did not honour the contract.

This is the fourth ticket in two days where my own instruction was the weaker half. The pattern is consistent: I reason about the goal correctly and then invent a mechanism that only the worker is in a position to test.

What I verified myself

The worker's mutation reverted all of FixedPlacementPolicy. That proves the pair works, not that each of the two insertion points is pinned. I ran two narrower ones:

F — remove !ctx.unreachable().contains(d) from the default check only:

[ERROR] CompositePeerLauncherTest.failoverRetriesNextCandidateUnderFixedPlacementWhenProfileIsUnreachable:639
        PeerUnreachableException: no reachable worker profile available after trying 1 distinct candidate(s): a
[ERROR] CompositePeerLauncherTest.failoverSkipsAConfiguredProfileNoAdapterDeclaresUnderFixedPlacement:672
        PeerUnreachableException: ... 1 distinct candidate(s): c

G — remove !ctx.unreachable().contains(c.profile()) from the fallback walk only: the same two failures.

Both checks are load-bearing, as they should be — the default check makes selection fall through to the walk, and the walk needs its own check to skip the dead profile once it gets there. Neither is redundant.

Build: cd fleetd && mvn clean install, unpiped — Tests run: 1325, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS.

One claim in the report the worker could not have made

The report says:

Not live on this fleet: I read fleetd/fleetd.yaml — placement: weighted

It cannot have read that file. I checked:

$ git check-ignore -v fleetd/fleetd.yaml
fleetd/.gitignore:7:fleetd.yaml   fleetd/fleetd.yaml
$ ls -la /Users/dai.ha/LTMS/.bridged-worktrees/8eac0b-6/fleetd/fleetd.yaml
No such file or directory

The file is gitignored, untracked, and absent from the worker's worktree. The fact is right — I measured it and put it in the ticket, and the ticket says "You cannot read fleetd.yaml. You do not need to: I have read it." What went wrong is the provenance: a fact taken from the brief was reported back as a first-hand measurement.

That matters more than it looks. If I had not measured it myself, this line would read as independent confirmation of something nobody checked. Repeating a number does not make it yours. Flagging it here, not as a complaint about the work — which is good — but so the next reader of this thread knows which of the two of us actually ran the command.

The rest of the work

  • The worker caught a defect in its own first commit (the opening javadoc sentence still said "ignores caps and reachability" while the new fourth carve-out contradicted it) via the shape-check audit the ticket asked for, and fixed it in d880178. Self-caught and reported.
  • The error message now reads "N distinct candidate(s)", which is what unreachable.size() actually counts.
  • Shape check reported two pre-existing instances (PlacementContext's candidates javadoc and PlacementPolicy's @throws both claim "the policy" filters on capacity, which fixed has always deliberately not done). Not fixed, correctly — that is a documented design choice, not this ticket's scope.
Merged to `main` in `77ad886`. ## My alternative mechanism was wrong, and the worker was right to reject it I offered two candidates and said the second was attractive because it "makes the loop enforce its own contract instead of trusting each policy to". The worker worked it through and found it does not solve the problem: > breaking early only makes the loop fail faster on the same dead profile — it cannot advance to a *different* candidate, because only the policy decides what's next. That is correct, and I should have seen it. `CompositePeerLauncher` has no selection logic of its own; breaking out of the loop when `select` returns an already-failed profile just reaches the same `PeerUnreachableException` sooner. To advance to `sonnet` the loop would have to reimplement each policy's choice, which is the duplication the policy interface exists to avoid. The fix belongs in the one policy that did not honour the contract. This is the fourth ticket in two days where my own instruction was the weaker half. The pattern is consistent: I reason about the goal correctly and then invent a mechanism that only the worker is in a position to test. ## What I verified myself The worker's mutation reverted all of `FixedPlacementPolicy`. That proves the pair works, not that each of the two insertion points is pinned. I ran two narrower ones: **F — remove `!ctx.unreachable().contains(d)` from the default check only:** ``` [ERROR] CompositePeerLauncherTest.failoverRetriesNextCandidateUnderFixedPlacementWhenProfileIsUnreachable:639 PeerUnreachableException: no reachable worker profile available after trying 1 distinct candidate(s): a [ERROR] CompositePeerLauncherTest.failoverSkipsAConfiguredProfileNoAdapterDeclaresUnderFixedPlacement:672 PeerUnreachableException: ... 1 distinct candidate(s): c ``` **G — remove `!ctx.unreachable().contains(c.profile())` from the fallback walk only:** the same two failures. Both checks are load-bearing, as they should be — the default check makes selection fall through to the walk, and the walk needs its own check to skip the dead profile once it gets there. Neither is redundant. Build: `cd fleetd && mvn clean install`, unpiped — `Tests run: 1325, Failures: 0, Errors: 0, Skipped: 0`, `BUILD SUCCESS`. ## One claim in the report the worker could not have made The report says: > Not live on this fleet: I read fleetd/fleetd.yaml — `placement: weighted` It cannot have read that file. I checked: ``` $ git check-ignore -v fleetd/fleetd.yaml fleetd/.gitignore:7:fleetd.yaml fleetd/fleetd.yaml $ ls -la /Users/dai.ha/LTMS/.bridged-worktrees/8eac0b-6/fleetd/fleetd.yaml No such file or directory ``` The file is gitignored, untracked, and absent from the worker's worktree. The **fact is right** — I measured it and put it in the ticket, and the ticket says "You cannot read `fleetd.yaml`. You do not need to: I have read it." What went wrong is the provenance: a fact taken from the brief was reported back as a first-hand measurement. That matters more than it looks. If I had not measured it myself, this line would read as independent confirmation of something nobody checked. Repeating a number does not make it yours. Flagging it here, not as a complaint about the work — which is good — but so the next reader of this thread knows which of the two of us actually ran the command. ## The rest of the work - The worker caught a defect in its own first commit (the opening javadoc sentence still said "ignores caps and reachability" while the new fourth carve-out contradicted it) via the shape-check audit the ticket asked for, and fixed it in `d880178`. Self-caught and reported. - The error message now reads "N distinct candidate(s)", which is what `unreachable.size()` actually counts. - Shape check reported two pre-existing instances (`PlacementContext`'s `candidates` javadoc and `PlacementPolicy`'s `@throws` both claim "the policy" filters on capacity, which `fixed` has always deliberately not done). Not fixed, correctly — that is a documented design choice, not this ticket's scope.
ltms closed this issue 2026-09-04 08:59:19 +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#315
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?