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

Fleetd.main's wiring has 1 behavioural test across 44 sites: pin the 11 whose failure silently turns off a control, with runtime tests not source-text ones #589

New Issue
Open
opened 2026-09-12 16:53:08 +02:00 by ltms · 1 comment
ltms commented 2026-09-12 16:53:08 +02:00
Owner
Copy Link
Copy Source

Follow-up to #587, which measured the gap. This ticket fixes the part worth fixing.

The measurement this rests on

#587 mutated 44 constructor-argument wiring sites in Fleetd.java, one per build, full suite each
time. Result: 6 killed, 38 survived. Classifying each kill by what the killing test actually does:

Kill kind Sites
Runtime — builds the object, asserts on behaviour 1 (:885)
Source text only — Files.readString + source.contains("…") 5 (:158, :444, :479, :492, :678)
Nothing 38

Behavioural coverage: 1 site of 44. See #587 for the full tables and the per-site consequences.

I verified two survivors myself in a scratch worktree at origin/main, compile-gated, restored and
sha-checked:

  • :611-631, the whole sessions.onRelease(...) cleanup lambda → detail -> { } —
    Tests run: 1789, Failures: 0, BUILD SUCCESS, 0 named failures.
  • :468, exhaustionSinkRef.set(exhaustionSink) → set(ExhaustionSink.none()) —
    Tests run: 1789, Failures: 0, BUILD SUCCESS, 0 named failures.

Both restored to d3c693e9e595228f.

Scope — 11 sites, not 38

The other 27 survivors are real but cheap to lose (thread-factory names, a busy-spin delay,
diagnostics). These 11 are the ones where a one-line silent change turns off a control and nothing
anywhere goes red.

Group 1 — exhaustion/quarantine turns off fleet-wide. The construction order makes this a
single point of failure:

:214  AtomicReference<ExhaustionSink> exhaustionSinkRef = new AtomicReference<>(ExhaustionSink.none());
:221  ExhaustionSink forwardingExhaustionSink = ExhaustionSink.forwardingTo(exhaustionSinkRef::get);
:468  exhaustionSinkRef.set(exhaustionSink);          <- the only line that makes it real
Site Wiring Inert form that must fail a test
:468 exhaustionSinkRef.set(exhaustionSink) set(ExhaustionSink.none())
:221 ExhaustionSink.forwardingTo(exhaustionSinkRef::get) () -> ExhaustionSink.none()
:411 new LiveExhaustedPatterns(() -> config.get().profiles()) () -> Map.of()
:412-416 the exhausted-pattern lookup lambda target -> null

:412-416 is the nastiest consequence in the whole sweep: a genuine usage-limit refusal stops being
classified as BACKEND_EXHAUSTED and is handed back as a real completion, so a lead acts on an
exhausted account's "answer" as if it were work.

Group 2 — the credential policy stops gating. Per #587 Part A, these reopen the CB-592
exposure gap that CB-596's policy closed:

Site Wiring Inert form
:230 ClaudeCodeLauncher ← () -> config.get().memberCredentials() () -> null
:237 OpenCodeLauncher ← () -> config.get().memberCredentials() () -> null

Group 3 — a lead waits on work that can never arrive. Every one of these ends in #588's
hardcoded thirty minutes:

Site Wiring Inert form
:611-631 the sessions.onRelease(...) cleanup lambda detail -> { }
:591 FleetHealthMonitor failTarget ← messages::abandon (t, r) -> { }
:505 Injector registrar ← completion::register TurnRegistrar.NOOP
:518 openLeadMailbox(..., LeadMailbox::open) (uri, selfCoordId, prefetch) -> null
:512 selectReplyInbox(..., AmqpReplyInbox::open) (uri, prefetch) -> new InMemoryReplyInbox()

:611-631 is the one to do first. MessageService.abandon's own javadoc at :668 states the
consequence:

Without this, tearing a worker down left its rendezvous waiter open: a blocking fleet_send kept
blocking, and an async one kept reporting PENDING until ASYNC_TIMEOUT_MS — thirty minutes —
even though the worker provably no longer existed and the delegation could never complete.

So a regression there hangs a lead's ticket for thirty minutes on every fleet_stop and every
idle-reap. The collaborators are each already tested and the caller is not:
MessageServiceTest:1171 pins abandon, PrimaryRegistryTest:157 pins forgetDelegation, and
nothing drives the lambda in main that calls both plus replyInbox.release.

Acceptance criteria

Written as a property under a change, because "add a wiring test for :468" is satisfied by a
source.contains one-liner that fixes nothing — that is exactly how #587's table came to read 6
when the answer was 1.

For each of the 11 sites, one test such that:

  1. RED on the inert form. Replace the wiring with the inert form named in the table above and
    that test fails, by name, with a message that says which wiring was lost. Report the mutated
    sha, the named failure, and the restored sha.
  2. GREEN on a behaviour-preserving rewrite. With the wiring correct, that same test still
    passes after the call is reformatted across lines and after the argument is extracted into a
    local variable or a named factory. This is the half that rules out a source-text test, and it is
    not optional: a test that only satisfies (1) may be a String.contains, which cannot survive the
    refactor it exists to protect.
  3. The total moves by exactly the number of tests added, and no existing test changes. If a new
    test kills more than its own site, say so — a kill that takes a crowd with it proves less than a
    kill of exactly one.

Do not satisfy any of these with Files.readString(Path.of("src/main/java/…")) +
source.contains(...). If a site genuinely cannot be pinned behaviourally without restructuring
main, say so and name what restructuring it would need — an honest "this one needs a seam first"
is worth more than a string match.

The shape to copy is FleetdLoopHealthSourceWiringTest, added in #584: extract the wiring into
a named package-private factory on Fleetd (the existing capacitySource(...),
healthCoverageSource(...), loopHealthSource(...) are the house pattern), then assert the
factory's behaviour with a real object. That refactor is what makes criterion (2) satisfiable.

Note on the 5 source-text tests

Leave them in place. They are honestly labelled ([SOURCE TEXT] in every @DisplayName, and their
javadoc says they never run main), and they do guard against outright deletion, which was the
hazard their own tickets were written for. Where this ticket adds a runtime test for the same site,
the source-text one becomes redundant and may be deleted in the same PR that adds the runtime
replacement — and per #557's rule, the PR must name the line each deleted test pinned and why it
can no longer go wrong. Do not delete one without its replacement being red today.

Suggested split

Three units, one per group, file-disjoint except that all three touch Fleetd.java if they extract
factories — so they must be sequenced or split by line range, not run blind in parallel. Group 3
first: it is the highest blast radius and the only group with a documented prior incident.

Follow-up to #587, which measured the gap. This ticket fixes the part worth fixing. ## The measurement this rests on #587 mutated 44 constructor-argument wiring sites in `Fleetd.java`, one per build, full suite each time. Result: 6 killed, 38 survived. Classifying each kill by what the killing test actually does: | Kill kind | Sites | |---|---| | Runtime — builds the object, asserts on behaviour | **1** (`:885`) | | Source text only — `Files.readString` + `source.contains("…")` | 5 (`:158`, `:444`, `:479`, `:492`, `:678`) | | Nothing | 38 | **Behavioural coverage: 1 site of 44.** See #587 for the full tables and the per-site consequences. I verified two survivors myself in a scratch worktree at `origin/main`, compile-gated, restored and sha-checked: - `:611-631`, the whole `sessions.onRelease(...)` cleanup lambda → `detail -> { }` — `Tests run: 1789, Failures: 0`, `BUILD SUCCESS`, 0 named failures. - `:468`, `exhaustionSinkRef.set(exhaustionSink)` → `set(ExhaustionSink.none())` — `Tests run: 1789, Failures: 0`, `BUILD SUCCESS`, 0 named failures. Both restored to `d3c693e9e595228f`. ## Scope — 11 sites, not 38 The other 27 survivors are real but cheap to lose (thread-factory names, a busy-spin delay, diagnostics). These 11 are the ones where a one-line silent change turns off a control and nothing anywhere goes red. **Group 1 — exhaustion/quarantine turns off fleet-wide.** The construction order makes this a single point of failure: ``` :214 AtomicReference<ExhaustionSink> exhaustionSinkRef = new AtomicReference<>(ExhaustionSink.none()); :221 ExhaustionSink forwardingExhaustionSink = ExhaustionSink.forwardingTo(exhaustionSinkRef::get); :468 exhaustionSinkRef.set(exhaustionSink); <- the only line that makes it real ``` | Site | Wiring | Inert form that must fail a test | |---|---|---| | `:468` | `exhaustionSinkRef.set(exhaustionSink)` | `set(ExhaustionSink.none())` | | `:221` | `ExhaustionSink.forwardingTo(exhaustionSinkRef::get)` | `() -> ExhaustionSink.none()` | | `:411` | `new LiveExhaustedPatterns(() -> config.get().profiles())` | `() -> Map.of()` | | `:412-416` | the exhausted-pattern lookup lambda | `target -> null` | `:412-416` is the nastiest consequence in the whole sweep: a genuine usage-limit refusal stops being classified as `BACKEND_EXHAUSTED` and is handed back as a real completion, so a lead acts on an exhausted account's "answer" as if it were work. **Group 2 — the credential policy stops gating.** Per #587 Part A, these reopen the CB-592 exposure gap that CB-596's policy closed: | Site | Wiring | Inert form | |---|---|---| | `:230` | `ClaudeCodeLauncher` ← `() -> config.get().memberCredentials()` | `() -> null` | | `:237` | `OpenCodeLauncher` ← `() -> config.get().memberCredentials()` | `() -> null` | **Group 3 — a lead waits on work that can never arrive.** Every one of these ends in #588's hardcoded thirty minutes: | Site | Wiring | Inert form | |---|---|---| | `:611-631` | the `sessions.onRelease(...)` cleanup lambda | `detail -> { }` | | `:591` | `FleetHealthMonitor` failTarget ← `messages::abandon` | `(t, r) -> { }` | | `:505` | `Injector` registrar ← `completion::register` | `TurnRegistrar.NOOP` | | `:518` | `openLeadMailbox(..., LeadMailbox::open)` | `(uri, selfCoordId, prefetch) -> null` | | `:512` | `selectReplyInbox(..., AmqpReplyInbox::open)` | `(uri, prefetch) -> new InMemoryReplyInbox()` | `:611-631` is the one to do first. `MessageService.abandon`'s own javadoc at `:668` states the consequence: > Without this, tearing a worker down left its rendezvous waiter open: a blocking `fleet_send` kept > blocking, and an async one kept reporting `PENDING` until `ASYNC_TIMEOUT_MS` — thirty minutes — > even though the worker provably no longer existed and the delegation could never complete. So a regression there hangs a lead's ticket for thirty minutes on every `fleet_stop` and every idle-reap. The collaborators are each already tested and the caller is not: `MessageServiceTest:1171` pins `abandon`, `PrimaryRegistryTest:157` pins `forgetDelegation`, and nothing drives the lambda in `main` that calls both plus `replyInbox.release`. ## Acceptance criteria Written as a property under a change, because "add a wiring test for `:468`" is satisfied by a `source.contains` one-liner that fixes nothing — that is exactly how #587's table came to read 6 when the answer was 1. **For each of the 11 sites, one test such that:** 1. **RED on the inert form.** Replace the wiring with the inert form named in the table above and that test fails, by name, with a message that says which wiring was lost. Report the mutated sha, the named failure, and the restored sha. 2. **GREEN on a behaviour-preserving rewrite.** With the wiring correct, that same test still passes after the call is reformatted across lines **and** after the argument is extracted into a local variable or a named factory. This is the half that rules out a source-text test, and it is not optional: a test that only satisfies (1) may be a `String.contains`, which cannot survive the refactor it exists to protect. 3. **The total moves by exactly the number of tests added**, and no existing test changes. If a new test kills more than its own site, say so — a kill that takes a crowd with it proves less than a kill of exactly one. **Do not** satisfy any of these with `Files.readString(Path.of("src/main/java/…"))` + `source.contains(...)`. If a site genuinely cannot be pinned behaviourally without restructuring `main`, say so and name what restructuring it would need — an honest "this one needs a seam first" is worth more than a string match. **The shape to copy** is `FleetdLoopHealthSourceWiringTest`, added in #584: extract the wiring into a named package-private factory on `Fleetd` (the existing `capacitySource(...)`, `healthCoverageSource(...)`, `loopHealthSource(...)` are the house pattern), then assert the factory's behaviour with a real object. That refactor is what makes criterion (2) satisfiable. ## Note on the 5 source-text tests Leave them in place. They are honestly labelled (`[SOURCE TEXT]` in every `@DisplayName`, and their javadoc says they never run `main`), and they do guard against outright deletion, which was the hazard their own tickets were written for. Where this ticket adds a runtime test for the same site, the source-text one becomes redundant and may be deleted **in the same PR that adds the runtime replacement** — and per #557's rule, the PR must name the line each deleted test pinned and why it can no longer go wrong. Do not delete one without its replacement being red today. ## Suggested split Three units, one per group, file-disjoint except that all three touch `Fleetd.java` if they extract factories — so they must be sequenced or split by line range, not run blind in parallel. Group 3 first: it is the highest blast radius and the only group with a documented prior incident.
ltms commented 2026-09-19 10:07:21 +02:00
Author
Owner
Copy Link
Copy Source

Correction to both #589 briefs — read before you commit

This is a rule your brief did not carry, and on this ticket it is load-bearing. It reaches you
here rather than as a message because a message cannot reach a busy member.

Never put a command whose exit status you will report behind a pipe

mvn test | tail          # reports tail's status, not mvn's — always 0

cmd | tail reports tail's exit status. A BUILD FAILURE disappears behind a zero exit and
you report a clean run that never happened. Capture the status first, then look at the output:

mvn -q test > /tmp/build.log 2>&1; status=$?
tail -40 /tmp/build.log
echo "mvn exit: $status"

Why this ticket specifically. Your whole deliverable is a pair of readings — the suite is RED
on the inert form, and GREEN after the two refactors. Both of those are exit statuses and test
counts. If either reading came through a pipe, it is not evidence, and criterion 1's "fails BY
NAME" cannot be confirmed. Re-run anything you measured that way before you report it.

This is trap #1 in this repo's own scripts/redeploy-fleetd.sh header, line 10: "A piped mvn
hides BUILD FAILURE behind a zero exit, so the build here is never piped." The lesson was
learned and hard-coded into one script, and never told to the members who run builds. That gap
is #591.

Two more that were in your brief, restated because they belong to the role, not the task

  • You never merge. The lead does. Open your PR and stop.
  • Stage files explicitly by path. Never git add -A or git add . — it sweeps up unrelated
    local state, and in a provisioned worktree some of what it sweeps cannot be committed and will
    not tell you so.

Unchanged

Everything else in your brief stands. The line-range split still holds: worker/589-fcd2aa-1
stays below line 500 in main(), worker/589-f64303-2 stays at or above it. Report honestly —
a check you could not run is not a check that passed.

Source: the fleet01 lead's dev charter, which carries all three rules on every spawn. This
host has no dev charter at all, so its members were told none of them. Being fixed.

## Correction to both #589 briefs — read before you commit This is a rule your brief did not carry, and on this ticket it is load-bearing. It reaches you here rather than as a message because a message cannot reach a busy member. ### Never put a command whose exit status you will report behind a pipe ``` mvn test | tail # reports tail's status, not mvn's — always 0 ``` `cmd | tail` reports **tail's** exit status. A `BUILD FAILURE` disappears behind a zero exit and you report a clean run that never happened. Capture the status first, then look at the output: ```bash mvn -q test > /tmp/build.log 2>&1; status=$? tail -40 /tmp/build.log echo "mvn exit: $status" ``` **Why this ticket specifically.** Your whole deliverable is a pair of readings — the suite is RED on the inert form, and GREEN after the two refactors. Both of those are exit statuses and test counts. If either reading came through a pipe, it is not evidence, and criterion 1's "fails BY NAME" cannot be confirmed. Re-run anything you measured that way before you report it. This is trap #1 in this repo's own `scripts/redeploy-fleetd.sh` header, line 10: *"A piped `mvn` hides BUILD FAILURE behind a zero exit, so the build here is never piped."* The lesson was learned and hard-coded into one script, and never told to the members who run builds. That gap is #591. ### Two more that were in your brief, restated because they belong to the role, not the task - **You never merge.** The lead does. Open your PR and stop. - **Stage files explicitly by path.** Never `git add -A` or `git add .` — it sweeps up unrelated local state, and in a provisioned worktree some of what it sweeps cannot be committed and will not tell you so. ### Unchanged Everything else in your brief stands. The line-range split still holds: `worker/589-fcd2aa-1` stays below line 500 in `main()`, `worker/589-f64303-2` stays at or above it. Report honestly — a check you could not run is not a check that passed. *Source: the fleet01 lead's `dev` charter, which carries all three rules on every spawn. This host has no `dev` charter at all, so its members were told none of them. Being fixed.*
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#589
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?