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

#315: FixedPlacementPolicy now honors the retry loop's unreachable set #319

Closed
agent wants to merge 0 commits from worker/fix-315-ce47c5-6 into main
pull from: worker/fix-315-ce47c5-6
merge into: fleet:main
Conversation 1 Commits - Files Changed -
agent commented 2026-09-04 08:53:40 +02:00
Member
Copy Link
Copy Source

Fixes #315.

Bug

CompositePeerLauncher.spawn retries a failed spawn candidate on the next one and rebuilds PlacementContext with the updated unreachable set, and its own comment says this is "so the policy excludes this profile." FixedPlacementPolicy.select never read ctx.unreachable(), so under the fixed placement policy (the default when the placement config key is unset or set to fixed) every retry re-selected the same dead default, and a second, healthy, configured profile was never tried. The same root cause also hits the wiring-bug branch in the loop (a candidate profile with no owning adapter) — it hits the exact same symptom.

Not live on this fleet. I read fleetd/fleetd.yaml: placement: weighted. weighted (and round-robin) already consult ctx.unreachable() via PlacementPolicyUtil.available(), so this fleet's spawns were never affected. This is live only for a deployment that leaves placement unset or sets it to fixed.

Which of the two candidate mechanisms, and why

The issue offered two candidates and asked me to justify the choice:

  1. Make FixedPlacementPolicy consult ctx.unreachable(), mirroring how it already consults quarantined/coolingOff.
  2. Make CompositePeerLauncher's retry loop break when select() returns a profile already in unreachable, fixing every policy (present and future) at the loop level.

I chose (1). I worked through (2) and it does not actually solve the problem: breaking (or continue-ing) the loop when select() returns an already-unreachable profile only makes the loop fail faster on the same dead profile — it cannot make the loop advance to a different candidate, because only the policy decides which candidate comes next. CompositePeerLauncher has no independent way to pick "the next one" without duplicating each policy's own selection logic, which would defeat having pluggable policies at all. The actual defect is that one policy implementation (fixed) does not honor the retry loop's stated contract, while the other two (weighted, round-robin) already do via PlacementPolicyUtil.available(). So the fix belongs in the policy that is out of line, matching the other two — not in the loop.

Changes

  • FixedPlacementPolicy.select: consult ctx.unreachable() in the same two places it already consults quarantined and coolingOff — the default-profile check and the fallback walk over ctx.candidates(). Added a fourth reason ("is unreachable") to the "no candidate remains" exception path, and updated the javadoc's three-exception list to four, matching the existing carve-out format (CB-578 quarantine, fleetd #201 cooling off, CB-554 weight 0).
  • CompositePeerLauncher.spawn: the final PeerUnreachableException message said "trying N candidate(s)" where N was unreachable.size() — a count of distinct profiles (a HashSet, so a profile added twice only counts once), under wording that reads as a count of attempts. Reworded to "N distinct candidate(s)" so the count matches what's measured and the profile list that follows it.

Invariants preserved

  1. First-choice behaviour unchanged: on attempt 1 the unreachable set is always empty, so fixed still returns the configured default exactly as before.
  2. An explicit fleet_spawn{profile:"x"} never goes through placement at all (see the early-return branch in CompositePeerLauncher.spawn for a non-blank requestedProfile) — untouched by this change.
  3. The error message no longer conflates a distinct-profile count with an attempt count.

Tests

Two new tests next to the three existing failover tests in CompositePeerLauncherTest (all three of which use PlacementPolicies.weighted() — exactly why this had no coverage):

  • failoverRetriesNextCandidateUnderFixedPlacementWhenProfileIsUnreachable — same scenario as the existing failoverRetriesNextCandidateWhenProfileIsUnreachable, pinned to PlacementPolicies.fixed().
  • failoverSkipsAConfiguredProfileNoAdapterDeclaresUnderFixedPlacement — covers the d == null wiring-bug branch: a configured candidate profile with no owning adapter is skipped and the loop lands on the next real candidate.

Mutation proof: reverted FixedPlacementPolicy.java to its pre-fix state (git diff saved as a patch, git checkout -- to revert, git apply to restore), ran just the two new tests, and both failed with the exact bug:

CompositePeerLauncherTest.failoverRetriesNextCandidateUnderFixedPlacementWhenProfileIsUnreachable -- Time elapsed: 0.262 s <<< ERROR!
dev.ltms.fleet.peer.PeerUnreachableException: no reachable worker profile available after trying 1 distinct candidate(s): a

CompositePeerLauncherTest.failoverSkipsAConfiguredProfileNoAdapterDeclaresUnderFixedPlacement -- Time elapsed: 0.001 s <<< ERROR!
dev.ltms.fleet.peer.PeerUnreachableException: no reachable worker profile available after trying 1 distinct candidate(s): c

Restored the fix with git apply and reran the full build.

Full build, mvn clean install, unpiped, exit code captured directly (not read after a pipe):

Tests run: 1325, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS

CompositePeerLauncherTest alone: Tests run: 56, Failures: 0, Errors: 0, Skipped: 0 (54 existing + 2 new).

Shape check

The issue also asked me to look in placement/ and CompositePeerLauncher.java for the same shape (a caller documenting an expectation of a collaborator that an implementation doesn't meet), report-only, no fixing. That survey is still running as I open this PR — I will post it as a follow-up comment on this PR.

Fixes #315. ## Bug `CompositePeerLauncher.spawn` retries a failed spawn candidate on the next one and rebuilds `PlacementContext` with the updated `unreachable` set, and its own comment says this is "so the policy excludes this profile." `FixedPlacementPolicy.select` never read `ctx.unreachable()`, so under the `fixed` placement policy (the default when the `placement` config key is unset or set to `fixed`) every retry re-selected the same dead default, and a second, healthy, configured profile was never tried. The same root cause also hits the wiring-bug branch in the loop (a candidate profile with no owning adapter) — it hits the exact same symptom. **Not live on this fleet.** I read `fleetd/fleetd.yaml`: `placement: weighted`. `weighted` (and `round-robin`) already consult `ctx.unreachable()` via `PlacementPolicyUtil.available()`, so this fleet's spawns were never affected. This is live only for a deployment that leaves `placement` unset or sets it to `fixed`. ## Which of the two candidate mechanisms, and why The issue offered two candidates and asked me to justify the choice: 1. Make `FixedPlacementPolicy` consult `ctx.unreachable()`, mirroring how it already consults `quarantined`/`coolingOff`. 2. Make `CompositePeerLauncher`'s retry loop break when `select()` returns a profile already in `unreachable`, fixing every policy (present and future) at the loop level. I chose **(1)**. I worked through (2) and it does not actually solve the problem: breaking (or `continue`-ing) the loop when `select()` returns an already-unreachable profile only makes the loop *fail faster* on the same dead profile — it cannot make the loop advance to a *different* candidate, because only the policy decides which candidate comes next. `CompositePeerLauncher` has no independent way to pick "the next one" without duplicating each policy's own selection logic, which would defeat having pluggable policies at all. The actual defect is that one policy implementation (`fixed`) does not honor the retry loop's stated contract, while the other two (`weighted`, `round-robin`) already do via `PlacementPolicyUtil.available()`. So the fix belongs in the policy that is out of line, matching the other two — not in the loop. ## Changes - `FixedPlacementPolicy.select`: consult `ctx.unreachable()` in the same two places it already consults `quarantined` and `coolingOff` — the default-profile check and the fallback walk over `ctx.candidates()`. Added a fourth reason ("is unreachable") to the "no candidate remains" exception path, and updated the javadoc's three-exception list to four, matching the existing carve-out format (CB-578 quarantine, fleetd #201 cooling off, CB-554 weight 0). - `CompositePeerLauncher.spawn`: the final `PeerUnreachableException` message said "trying N candidate(s)" where N was `unreachable.size()` — a count of **distinct** profiles (a `HashSet`, so a profile added twice only counts once), under wording that reads as a count of attempts. Reworded to "N distinct candidate(s)" so the count matches what's measured and the profile list that follows it. ## Invariants preserved 1. First-choice behaviour unchanged: on attempt 1 the `unreachable` set is always empty, so `fixed` still returns the configured default exactly as before. 2. An explicit `fleet_spawn{profile:"x"}` never goes through placement at all (see the early-return branch in `CompositePeerLauncher.spawn` for a non-blank `requestedProfile`) — untouched by this change. 3. The error message no longer conflates a distinct-profile count with an attempt count. ## Tests Two new tests next to the three existing failover tests in `CompositePeerLauncherTest` (all three of which use `PlacementPolicies.weighted()` — exactly why this had no coverage): - `failoverRetriesNextCandidateUnderFixedPlacementWhenProfileIsUnreachable` — same scenario as the existing `failoverRetriesNextCandidateWhenProfileIsUnreachable`, pinned to `PlacementPolicies.fixed()`. - `failoverSkipsAConfiguredProfileNoAdapterDeclaresUnderFixedPlacement` — covers the `d == null` wiring-bug branch: a configured candidate profile with no owning adapter is skipped and the loop lands on the next real candidate. **Mutation proof:** reverted `FixedPlacementPolicy.java` to its pre-fix state (`git diff` saved as a patch, `git checkout --` to revert, `git apply` to restore), ran just the two new tests, and both failed with the exact bug: ``` CompositePeerLauncherTest.failoverRetriesNextCandidateUnderFixedPlacementWhenProfileIsUnreachable -- Time elapsed: 0.262 s <<< ERROR! dev.ltms.fleet.peer.PeerUnreachableException: no reachable worker profile available after trying 1 distinct candidate(s): a CompositePeerLauncherTest.failoverSkipsAConfiguredProfileNoAdapterDeclaresUnderFixedPlacement -- Time elapsed: 0.001 s <<< ERROR! dev.ltms.fleet.peer.PeerUnreachableException: no reachable worker profile available after trying 1 distinct candidate(s): c ``` Restored the fix with `git apply` and reran the full build. **Full build**, `mvn clean install`, unpiped, exit code captured directly (not read after a pipe): ``` Tests run: 1325, Failures: 0, Errors: 0, Skipped: 0 BUILD SUCCESS ``` `CompositePeerLauncherTest` alone: `Tests run: 56, Failures: 0, Errors: 0, Skipped: 0` (54 existing + 2 new). ## Shape check The issue also asked me to look in `placement/` and `CompositePeerLauncher.java` for the same shape (a caller documenting an expectation of a collaborator that an implementation doesn't meet), report-only, no fixing. That survey is still running as I open this PR — I will post it as a follow-up comment on this PR.
agent added 1 commit 2026-09-04 08:53:40 +02:00
#315: FixedPlacementPolicy now honors the retry loop's unreachable set
CI / contract (pull_request) Successful in 53s
Details
CI / build (pull_request) Successful in 2m33s
Details
2159a5a94a 
CompositePeerLauncher.spawn retries a failed candidate on the next one and
rebuilds PlacementContext "so the policy excludes this profile" (its own
comment), but FixedPlacementPolicy.select never read ctx.unreachable(). Under
the default `fixed` placement policy (used when `placement` is unset or set
to `fixed`), every retry re-picked the same dead default and a second,
healthy, configured profile was never tried. This also covers the wiring-bug
branch (a candidate profile with no owning adapter), which hit the exact same
symptom for the same reason.

Not live on this fleet: fleetd.yaml sets placement: weighted, which already
consults ctx.unreachable() via PlacementPolicyUtil.available(). This is live
only for a deployment that leaves placement unset or sets it to fixed.

Fix is in FixedPlacementPolicy: consult ctx.unreachable() in the same two
places it already consults quarantined/coolingOff (the default check and the
fallback walk over candidates()), and add a fourth reason to the "no
candidate remains" exception. Considered fixing this in
CompositePeerLauncher's retry loop instead (break when select() returns an
already-unreachable profile), but that only fails faster on the same dead
profile — it cannot make the loop advance to a different candidate, because
only the policy decides which candidate is next. The defect is that one
policy implementation does not honor the loop's stated contract, so the fix
belongs in that policy, matching how weighted/round-robin already behave.

Also fixed: the "no reachable worker profile" exception message said
"trying N candidate(s)" where N was unreachable.size(), a count of DISTINCT
profiles (a HashSet dedupes a profile added twice), under wording that reads
as a count of attempts. Reworded to "N distinct candidate(s)" so the count
matches what is measured and the profile list that follows it.

Tests: two new failover tests next to the three existing ones in
CompositePeerLauncherTest (which all use PlacementPolicies.weighted(), which
is why this had no coverage) — one pinned to PlacementPolicies.fixed() for
the unreachable-default case, one for the wiring-bug (no adapter) case.
Mutation-proofed: reverted FixedPlacementPolicy.java, both new tests failed
with the exact bug ("no reachable worker profile available after trying 1
distinct candidate(s): a" / "...c"), then restored the fix.
agent added 1 commit 2026-09-04 08:55:31 +02:00
#315: fix self-contradicting javadoc left by the previous commit
CI / contract (pull_request) Successful in 1m21s
Details
CI / build (pull_request) Successful in 2m31s
Details
d88017807b 
FixedPlacementPolicy's class javadoc still opened with "This ignores caps
and reachability" after the previous commit added reachability as the
fourth carve-out that is explicitly NOT ignored — caught by a shape-check
survey run against this same file as part of #315's own request ("look in
placement/ ... for the same shape: a caller/comment that documents an
expectation ... where an implementation does not meet it"). Reworded the
opening sentence: fixed still ignores caps (maxLoad) by design, but
reachability is now a narrower, per-call retry exclusion, not an ignored
concern.
agent commented 2026-09-04 08:56:01 +02:00
Author
Member
Copy Link
Copy Source

Shape check (report-only, per the issue's request)

I ran a delegated audit of placement/ and member/CompositePeerLauncher.java for the same shape as the bug this PR fixes: a caller/javadoc that documents an expectation of a collaborator, where an implementation doesn't meet it. Not fixed here — reported only, as asked, except one item that was a defect in my own commit, which I did fix (see below).

  • PlacementContext.java:11 vs FixedPlacementPolicy.java (whole select method) — the record's javadoc for candidates says "the policy filters out those at cap or unreachable" (a generic claim about any PlacementPolicy), but FixedPlacementPolicy.select never reads ctx.liveCount() or checks maxLoad at all — a caller relying on that contract can get back a candidate already at capacity. Pre-existing, not introduced by this PR — fixed has always deliberately ignored caps (see its own javadoc). Not fixed here.
  • PlacementPolicy.java:12-13 vs FixedPlacementPolicy.java — the interface's @throws doc promises the failure message names "whether every profile is at capacity or unreachable," but since FixedPlacementPolicy never evaluates capacity, its exception messages can never say "at capacity," only quarantined/cooling-off/unreachable/weight-0. Same root cause as the item above, from the interface side. Pre-existing. Not fixed here.
  • FixedPlacementPolicy.java:8-9 — this one was introduced by my own first commit (2159a5a): the class summary said "This ignores caps and reachability," but the paragraph right below it (the fleetd #315 fix) now documents unreachable as one of four cases that is explicitly not ignored — a self-contradiction I left in place. Since this was a defect in my own change to this exact file, and the issue explicitly asked me to fix the javadoc, I fixed it in a follow-up commit (d880178): the opening sentence now says fixed ignores caps (maxLoad) by design, and separately explains reachability is a narrower, per-call retry exclusion rather than an ignored concern.

No other instances of this shape were found in RoundRobinPlacementPolicy, WeightedRoundRobinPolicy, PlacementPolicyUtil, BackendQuarantine, BackendOutagePolicy, PlacementPolicies, or the rest of CompositePeerLauncher.java — their documented priority rules (quarantine-before-cooling-off, distinct-target counting, capacity filtering via PlacementPolicyUtil.available) matched the actual code.

## Shape check (report-only, per the issue's request) I ran a delegated audit of `placement/` and `member/CompositePeerLauncher.java` for the same shape as the bug this PR fixes: a caller/javadoc that documents an expectation of a collaborator, where an implementation doesn't meet it. Not fixed here — reported only, as asked, except one item that was a defect in my own commit, which I did fix (see below). - `PlacementContext.java:11` vs `FixedPlacementPolicy.java` (whole `select` method) — the record's javadoc for `candidates` says "the policy filters out those at cap or unreachable" (a generic claim about any `PlacementPolicy`), but `FixedPlacementPolicy.select` never reads `ctx.liveCount()` or checks `maxLoad` at all — a caller relying on that contract can get back a candidate already at capacity. **Pre-existing, not introduced by this PR** — `fixed` has always deliberately ignored caps (see its own javadoc). Not fixed here. - `PlacementPolicy.java:12-13` vs `FixedPlacementPolicy.java` — the interface's `@throws` doc promises the failure message names "whether every profile is at capacity or unreachable," but since `FixedPlacementPolicy` never evaluates capacity, its exception messages can never say "at capacity," only quarantined/cooling-off/unreachable/weight-0. Same root cause as the item above, from the interface side. **Pre-existing.** Not fixed here. - `FixedPlacementPolicy.java:8-9` — **this one was introduced by my own first commit** (2159a5a): the class summary said "This ignores caps and reachability," but the paragraph right below it (the fleetd #315 fix) now documents `unreachable` as one of four cases that is explicitly *not* ignored — a self-contradiction I left in place. Since this was a defect in my own change to this exact file, and the issue explicitly asked me to fix the javadoc, I fixed it in a follow-up commit (d880178): the opening sentence now says `fixed` ignores caps (`maxLoad`) by design, and separately explains reachability is a narrower, per-call retry exclusion rather than an ignored concern. No other instances of this shape were found in `RoundRobinPlacementPolicy`, `WeightedRoundRobinPolicy`, `PlacementPolicyUtil`, `BackendQuarantine`, `BackendOutagePolicy`, `PlacementPolicies`, or the rest of `CompositePeerLauncher.java` — their documented priority rules (quarantine-before-cooling-off, distinct-target counting, capacity filtering via `PlacementPolicyUtil.available`) matched the actual code.
ltms closed this pull request 2026-09-04 08:59:22 +02:00
Some checks are pending
CI / contract (pull_request) Successful in 1m21s
Details
CI / build (pull_request) Successful in 2m31s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
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#319
Delete Branch "worker/fix-315-ce47c5-6"

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?