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

fleet: is a third split key, sitting in the coverage checker's own escape hatch — and SPLIT_KEYS membership does not prove any reporting code exists #333

New Issue
Closed
opened 2026-09-04 10:23:45 +02:00 by ltms · 1 comment
ltms commented 2026-09-04 10:23:45 +02:00
Owner
Copy Link
Copy Source

Follow-up to #330. Two findings, one unit. The first is a live under-claim; the second is why the new checker did not catch it.

F1 — fleet: is split, and it is filed as hot

I checked this myself after merging #330:

Fleetd.java:281            var leaders = cfg.fleet().leaders();      read off the startup snapshot
LeadLauncher.java:56       private final FleetConfig cfg;            a FROZEN config, not a Supplier
LeadLauncher.java:77       Map<...> leaders = cfg.fleet().leaders(); reads that frozen copy

LeadLauncher takes a FleetConfig in its constructor, not a Supplier<FleetConfig>. Fleetd.java:281 builds the tab→name map from the startup snapshot and never rebuilds it. So fleet.leaders is frozen, while the role pools, charters and tabLabel are read live through the supplier on CompositePeerLauncher.

That is the same shape as health: and coordinator: — read both ways, at different sites. fleet: is a split key.

ConfigRef's class doc has said the frozen half out loud for a while: "This does NOT include fleet.leaders: Fleetd.main reads cfg.fleet().leaders() once at startup ... so a lead added, removed, or re-tab'd under fleet.leaders needs a restart, the same as any deferred key below." Nobody acted on it.

Direction of harm. Today, changing fleet.leaders in a reload reports nothing — not deferred, not split. The operator is told config reloaded. This is not academic: a lead is found by its tab label, and a lead whose tab no longer matches is demoted to worker and refuses every orchestration call. So the operator edits the tab, sees config reloaded, and the lead stays broken with no message saying why.

This is the third instance of the same bug — #323 (worktreeGroup), #326 (primary, configReload), now fleet.

Whose fault it is. Mine. #330's brief listed fleet under "Hot" in its known-good starting values and scoped split to exactly health/coordinator. The worker corrected me on profiles and obeyed me on fleet, flagging the caveat in the exclusion set's javadoc — which is exactly what I asked for. So a genuinely split key ended up in the checker's escape hatch, blessed by the checker, on the checker's first commit.

Goal: a reload that changes fleet: reports it honestly — the frozen half needs a restart, the live half already applied.

Invariants:

  1. Do not make fleet.leaders take effect live. Rebuilding the tab scanner and LeadLauncher on a running daemon is a separate, larger question. This ticket is about what a reload reports.
  2. The other half of fleet: really is live. Do not report the whole key as needing a restart.
  3. ConfigRefProfileCoverageTest's exclusion-set assertion stays untouched.

Mechanism: this one is not a candidate — #330 built the class for it. Move fleet out of HOT_EXCLUDED_TOP_LEVEL_KEYS and into SPLIT_KEYS, with a message naming both halves the way health and coordinator do. What is yours to decide: the exact wording of the frozen half. Read Fleetd.java:281-300 and LeadLauncher and say what actually needs a restart, rather than copying my summary.

Also re-count the buckets and update the class doc's denominator note. It will move from 4 hot-excluded / 2 split to 3 hot-excluded / 3 split.

F2 — SPLIT_KEYS membership does not prove any reporting code exists

Measured. I dropped the coordinator block from changedSplitKeys while leaving "coordinator" in SPLIT_KEYS:

Tests run: 1348, Failures: 3
ConfigRefTest.changingCoordinatorIsReportedAsSplit:632     expected: <1> but was: <0>
ConfigRefTest.changingBothSplitKeysReportsBoth:684         expected: <2> but was: <1>
ConfigRefTest.aSplitChangeAndADeferredChangeCoexist:715    expected: <1> but was: <0>

Three hand-written behavioural tests caught it. Neither the new coverage test nor the assert in changedSplitKeys fired. Both only look one way.

So for a future split key, the cheapest way to pass ConfigRefTopLevelCoverageTest is to add one string to a Set and write no reporting code at all. The checker reads that name as "triaged" and stops. F1 is that failure already: fleet is in a bucket, the checker is green, and the reload says nothing.

The same one-way shape is pre-existing, not invented by #330:

  • assert COLD_KEYS.containsAll(changed) catches "reported but not listed", never "listed but not reported".
  • DEFERRED_TOP_LEVEL_KEYS lives in the test and is a second hand-written copy of what changedDeferredKeys does. Adding a name there with no matching if passes.

Goal: for every name in SPLIT_KEYS, COLD_KEYS and the test's deferred set, prove that changing that key actually produces a report — by name, not by a hand-written test per key.

Candidate mechanism, as a candidate only: the reflection machinery is already there. ConfigRefProfileCoverageTest builds one-field mutants of a record via getDeclaredConstructor(types).newInstance(args). The same trick over FleetConfig gives you "a config where only <key> differs", and then reload() must name that key in the right list. Decide it yourself and justify it — in particular, whether building a valid alternate value for every nested record type is worth it, or whether a narrower version (split and cold only, where the sets are small) buys most of the value for a fraction of the work. Either answer is fine; say which and why.

If you conclude the full version is not worth building, say what it would have caught and what the narrow version misses, and write that into the test's javadoc. #330's checker already does this well — its javadoc states plainly what it cannot prove, and that disclosure is what let me find both of these quickly instead of trusting a green run. Keep that standard.

Order

F1 first — it is a live defect and it is small. F2 is what stops the fourth instance.

Rules

  • Each finding needs a test that fails without its fix.
  • Mutation proof required for each: revert, quote the real failure output, restore.
  • For F2, the mutation that matters is the one-way direction: add a key to a triage set with no reporting code behind it, and show your new check fails. A mutation in the other direction proves the half that already worked.
  • Run the full suite — cd fleetd && mvn clean install, unpiped — and quote the real Tests run: and BUILD lines. Never read $? after a pipe.
  • Never git stash — the stash is shared across every worktree here.
  • Never run git worktree remove or git worktree prune — other workers are live in those directories.
  • Stage files explicitly; never git add -A. Never merge.
  • fleetd/fleetd.yaml is gitignored and absent from your worktree. Do not report on its contents.
  • Put your full report in the PR body as well as in your fleet_reply.

Already established, do not re-derive

  • The four reload classes and the split machinery are #330's, merged in 7b918c5. Read them; do not redesign them.
  • Current tally, printed by the checker on every run: 22 components — 5 cold, 11 deferred, 2 split, 4 hot-excluded.
  • profiles belongs in deferred, not hot. #330's worker corrected my list on this and was right.
  • memberCredentials, memberLoginShell and placement are genuinely hot. Leave them.
Follow-up to #330. Two findings, one unit. The first is a live under-claim; the second is why the new checker did not catch it. ## F1 — `fleet:` is split, and it is filed as hot I checked this myself after merging #330: ``` Fleetd.java:281 var leaders = cfg.fleet().leaders(); read off the startup snapshot LeadLauncher.java:56 private final FleetConfig cfg; a FROZEN config, not a Supplier LeadLauncher.java:77 Map<...> leaders = cfg.fleet().leaders(); reads that frozen copy ``` `LeadLauncher` takes a `FleetConfig` in its constructor, not a `Supplier<FleetConfig>`. `Fleetd.java:281` builds the tab→name map from the startup snapshot and never rebuilds it. So `fleet.leaders` is **frozen**, while the role pools, `charters` and `tabLabel` are read live through the supplier on `CompositePeerLauncher`. That is the same shape as `health:` and `coordinator:` — read both ways, at different sites. `fleet:` is a **split** key. `ConfigRef`'s class doc has said the frozen half out loud for a while: *"This does NOT include `fleet.leaders`: `Fleetd.main` reads `cfg.fleet().leaders()` once at startup ... so a lead added, removed, or re-`tab`'d under `fleet.leaders` needs a restart, the same as any deferred key below."* Nobody acted on it. **Direction of harm.** Today, changing `fleet.leaders` in a reload reports **nothing** — not deferred, not split. The operator is told `config reloaded`. This is not academic: a lead is found by its tab label, and a lead whose tab no longer matches is demoted to worker and refuses every orchestration call. So the operator edits the tab, sees `config reloaded`, and the lead stays broken with no message saying why. This is the third instance of the same bug — #323 (`worktreeGroup`), #326 (`primary`, `configReload`), now `fleet`. **Whose fault it is.** Mine. #330's brief listed `fleet` under "Hot" in its known-good starting values and scoped `split` to exactly `health`/`coordinator`. The worker corrected me on `profiles` and obeyed me on `fleet`, flagging the caveat in the exclusion set's javadoc — which is exactly what I asked for. So a genuinely split key ended up in the checker's escape hatch, blessed by the checker, on the checker's first commit. **Goal:** a reload that changes `fleet:` reports it honestly — the frozen half needs a restart, the live half already applied. **Invariants:** 1. **Do not make `fleet.leaders` take effect live.** Rebuilding the tab scanner and `LeadLauncher` on a running daemon is a separate, larger question. This ticket is about what a reload *reports*. 2. The other half of `fleet:` really is live. Do not report the whole key as needing a restart. 3. `ConfigRefProfileCoverageTest`'s exclusion-set assertion stays untouched. **Mechanism:** this one is not a candidate — #330 built the class for it. Move `fleet` out of `HOT_EXCLUDED_TOP_LEVEL_KEYS` and into `SPLIT_KEYS`, with a message naming both halves the way `health` and `coordinator` do. What **is** yours to decide: the exact wording of the frozen half. Read `Fleetd.java:281-300` and `LeadLauncher` and say what actually needs a restart, rather than copying my summary. **Also re-count the buckets** and update the class doc's denominator note. It will move from `4 hot-excluded` / `2 split` to `3 hot-excluded` / `3 split`. ## F2 — `SPLIT_KEYS` membership does not prove any reporting code exists Measured. I dropped the `coordinator` block from `changedSplitKeys` while **leaving `"coordinator"` in `SPLIT_KEYS`**: ``` Tests run: 1348, Failures: 3 ConfigRefTest.changingCoordinatorIsReportedAsSplit:632 expected: <1> but was: <0> ConfigRefTest.changingBothSplitKeysReportsBoth:684 expected: <2> but was: <1> ConfigRefTest.aSplitChangeAndADeferredChangeCoexist:715 expected: <1> but was: <0> ``` Three hand-written behavioural tests caught it. **Neither the new coverage test nor the `assert` in `changedSplitKeys` fired.** Both only look one way. So for a **future** split key, the cheapest way to pass `ConfigRefTopLevelCoverageTest` is to add one string to a `Set` and write no reporting code at all. The checker reads that name as "triaged" and stops. F1 is that failure already: `fleet` is in a bucket, the checker is green, and the reload says nothing. The same one-way shape is pre-existing, not invented by #330: - `assert COLD_KEYS.containsAll(changed)` catches "reported but not listed", never "listed but not reported". - `DEFERRED_TOP_LEVEL_KEYS` lives in the test and is a second hand-written copy of what `changedDeferredKeys` does. Adding a name there with no matching `if` passes. **Goal:** for every name in `SPLIT_KEYS`, `COLD_KEYS` and the test's deferred set, prove that changing *that* key actually produces a report — by name, not by a hand-written test per key. **Candidate mechanism, as a candidate only:** the reflection machinery is already there. `ConfigRefProfileCoverageTest` builds one-field mutants of a record via `getDeclaredConstructor(types).newInstance(args)`. The same trick over `FleetConfig` gives you "a config where only `<key>` differs", and then `reload()` must name that key in the right list. **Decide it yourself and justify it** — in particular, whether building a valid alternate value for every nested record type is worth it, or whether a narrower version (split and cold only, where the sets are small) buys most of the value for a fraction of the work. Either answer is fine; say which and why. If you conclude the full version is not worth building, **say what it would have caught and what the narrow version misses**, and write that into the test's javadoc. #330's checker already does this well — its javadoc states plainly what it cannot prove, and that disclosure is what let me find both of these quickly instead of trusting a green run. Keep that standard. ## Order F1 first — it is a live defect and it is small. F2 is what stops the fourth instance. ## Rules - Each finding needs a test that fails without its fix. - Mutation proof required for each: revert, quote the real failure output, restore. - For F2, the mutation that matters is the **one-way direction**: add a key to a triage set with no reporting code behind it, and show your new check fails. A mutation in the other direction proves the half that already worked. - **Run the full suite** — `cd fleetd && mvn clean install`, unpiped — and quote the real `Tests run:` and `BUILD` lines. Never read `$?` after a pipe. - Never `git stash` — the stash is shared across every worktree here. - Never run `git worktree remove` or `git worktree prune` — other workers are live in those directories. - Stage files explicitly; never `git add -A`. Never merge. - `fleetd/fleetd.yaml` is gitignored and absent from your worktree. Do not report on its contents. - Put your full report in the PR body as well as in your `fleet_reply`. ## Already established, do not re-derive - The four reload classes and the `split` machinery are #330's, merged in `7b918c5`. Read them; do not redesign them. - Current tally, printed by the checker on every run: 22 components — 5 cold, 11 deferred, 2 split, 4 hot-excluded. - `profiles` belongs in deferred, not hot. #330's worker corrected my list on this and was right. - `memberCredentials`, `memberLoginShell` and `placement` are genuinely hot. Leave them.
ltms commented 2026-09-04 10:44:30 +02:00
Author
Owner
Copy Link
Copy Source

Merged to main as b4f9d7f (--no-ff; the branch was behind main). Follow-up commit eee4d57
fixes a doc gap the worker found and correctly left alone.

Build after the merge, unpiped: Tests run: 1355, Failures: 0, Errors: 0, Skipped: 0,
BUILD SUCCESS. (The worker measured 1352 on its branch; the extra three are #329's tests, already
on main.)

My own mutations — aimed at the half the worker did not mutate

The worker proved F2 by removing the coordinator split branch. I mutated the cold half
instead, then the deferred half.

Mutation U — drop memberHerdrSocket's branch from changedColdKeys, leave it in COLD_KEYS:

Tests run: 1355, Failures: 1, Errors: 0
ConfigRefTopLevelReportingCoverageTest.everyColdKeyIsActuallyReportedByChangedColdKeys:185
these keys are in ConfigRef.COLD_KEYS but mutating them alone produces no matching entry from
changedColdKeys — a set entry with no comparison behind it: [memberHerdrSocket]

The new checker covers the cold half too, and names the key. Reverted.

Mutation V — drop guard's branch from changedDeferredKeys, leave guard in the deferred set:

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

Nothing failed. The gap the worker declared in its own javadoc is real, and I have now measured
one confirmed instance of it. Opened as #337. Reverted; main is unchanged.

That declaration is the reason this took one command to find. A checker that says plainly what it
does not cover, next to what it does, is worth more than one that quietly covers more. This is the
second time in two tickets that a worker's honest caveat was the fastest route to the next bug.

The doc gap (commit eee4d57)

The class doc's Cold bullet listed four keys; COLD_KEYS has five — memberHerdrSocket: was
missing from the prose. The worker spotted it and left it as out of scope, which was right.

I fixed it by pointing the bullet at COLD_KEYS instead of re-listing its contents, so the prose
and the set cannot drift apart a second time. Re-listing a set in prose next to the set is how this
happened.

What I checked on the worker's own claims

  • ConfigRefProfileCoverageTest untouched — confirmed, git diff on it is empty.
  • fleet.leaders was not made live; only the reload report changed. Confirmed in the diff.
  • The recounted denominator (22 = 5 cold + 11 deferred + 3 split + 3 hot-excluded) adds up.

Closing. Two fixes in, one measured gap out (#337).

Merged to `main` as `b4f9d7f` (`--no-ff`; the branch was behind main). Follow-up commit `eee4d57` fixes a doc gap the worker found and correctly left alone. Build after the merge, unpiped: `Tests run: 1355, Failures: 0, Errors: 0, Skipped: 0`, `BUILD SUCCESS`. (The worker measured 1352 on its branch; the extra three are #329's tests, already on main.) ## My own mutations — aimed at the half the worker did not mutate The worker proved F2 by removing the `coordinator` split branch. I mutated the **cold** half instead, then the **deferred** half. **Mutation U — drop `memberHerdrSocket`'s branch from `changedColdKeys`, leave it in `COLD_KEYS`:** ``` Tests run: 1355, Failures: 1, Errors: 0 ConfigRefTopLevelReportingCoverageTest.everyColdKeyIsActuallyReportedByChangedColdKeys:185 these keys are in ConfigRef.COLD_KEYS but mutating them alone produces no matching entry from changedColdKeys — a set entry with no comparison behind it: [memberHerdrSocket] ``` The new checker covers the cold half too, and names the key. Reverted. **Mutation V — drop `guard`'s branch from `changedDeferredKeys`, leave `guard` in the deferred set:** ``` Tests run: 1355, Failures: 0, Errors: 0, Skipped: 0 BUILD SUCCESS ``` **Nothing failed.** The gap the worker declared in its own javadoc is real, and I have now measured one confirmed instance of it. Opened as #337. Reverted; main is unchanged. That declaration is the reason this took one command to find. A checker that says plainly what it does not cover, next to what it does, is worth more than one that quietly covers more. This is the second time in two tickets that a worker's honest caveat was the fastest route to the next bug. ## The doc gap (commit `eee4d57`) The class doc's **Cold** bullet listed four keys; `COLD_KEYS` has five — `memberHerdrSocket:` was missing from the prose. The worker spotted it and left it as out of scope, which was right. I fixed it by pointing the bullet at `COLD_KEYS` instead of re-listing its contents, so the prose and the set cannot drift apart a second time. Re-listing a set in prose next to the set is how this happened. ## What I checked on the worker's own claims - `ConfigRefProfileCoverageTest` untouched — confirmed, `git diff` on it is empty. - `fleet.leaders` was not made live; only the reload **report** changed. Confirmed in the diff. - The recounted denominator (22 = 5 cold + 11 deferred + 3 split + 3 hot-excluded) adds up. Closing. Two fixes in, one measured gap out (#337).
ltms closed this issue 2026-09-04 10:44:36 +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#333
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?