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

fleetd#333: fleet.leaders is split, and split membership now proves reporting exists #336

Closed
agent wants to merge 0 commits from worker/fleetd-333-281f46-18 into main
pull from: worker/fleetd-333-281f46-18
merge into: fleet:main
Conversation 0 Commits - Files Changed -
agent commented 2026-09-04 10:37:28 +02:00
Member
Copy Link
Copy Source

Summary

fleetd#333 has two findings about ConfigRef's reload report. Both are fixed.

F1 — fleet: is a split key, not a hot key

fleet.leaders is read only at startup. Two places use it:

  • Fleetd.java:281 builds the LeadTabScanner's tab label -> lead name map. This map decides if a caller's pane is a lead. It is wired into CallerResolver.withLeadsAndMembers at Fleetd.java:620/624.
  • Fleetd.java:315 calls LeadLauncher.ensureLeads(). This auto-launches each declared lead up to its instances count.

Neither one rebuilds when config reloads. So if an operator adds a lead, removes a lead, or gives a lead a new tab: label, the daemon needs a restart. Before this fix, fleet sat in ConfigRefTopLevelCoverageTest's HOT_EXCLUDED_TOP_LEVEL_KEYS escape hatch. A reload that changed only fleet.leaders reported a bare "config reloaded". This is wrong. The operator edits a lead's tab, sees "config reloaded", and the pane still resolves as a worker, not a lead.

The rest of fleet: — role pools (architects/developers/reviewers), charters, and tabLabel — really is hot. It is read live through the CompositePeerLauncher supplier.

Fix: moved fleet into ConfigRef.SPLIT_KEYS. changedSplitKeys now compares fleet.leaders on its own, not the whole Fleet record. I chose this on purpose: comparing the whole record would report "split" (and "needs a restart") even for a tabLabel-only change, which is fully hot. That would be an over-claim — the same kind of wrong report this class exists to stop, just in the other direction.

Updated the class doc (ConfigRef.java) and the escape-hatch doc (ConfigRefTopLevelCoverageTest.java) to match, and re-counted the denominator: 22 top-level FleetConfig components — 5 cold, 11 deferred, 3 split, 3 hot-excluded.

F2 — SPLIT_KEYS/COLD_KEYS membership does not prove any reporting code exists

I measured this live: I removed the coordinator branch from changedSplitKeys, but left "coordinator" in SPLIT_KEYS. Result:

  • ConfigRefTopLevelCoverageTest (the shape checker) stayed green. It only reads set membership.
  • changedSplitKeys's own "kept in step" assert stayed green too. It only checks that reported entries are a subset of SPLIT_KEYS, never that every SPLIT_KEYS member produced a report.

So a future split (or cold) key could be added as a bare string in a Set.of(...) with no comparison behind it, and the whole suite would stay green — the exact shape as F1.

Fix: added ConfigRefTopLevelReportingCoverageTest. It is the ConfigRefProfileCoverageTest mechanism, one level up. It builds FleetConfig pairs by reflection that differ in exactly one top-level component, then calls the real (now package-private) ConfigRef.changedColdKeys/changedSplitKeys methods to prove each COLD_KEYS/SPLIT_KEYS member is actually reported.

I scoped this to COLD_KEYS and SPLIT_KEYS, not the full DEFERRED_TOP_LEVEL_KEYS set. Two reasons: (1) that is where fleetd#333 actually found and measured the gap; (2) most DEFERRED_TOP_LEVEL_KEYS members already have an individual behavioural test in ConfigRefTest naming them by key (worktreeGroup, primary, configReload, profile launch settings, exhaustedPattern, errorPattern, ideProjectDir). A few (guard, lifecycle, leadHeartbeat, spawnReadyTimeoutMs/spawnReadyPollMs, quarantineCooldownSeconds) do not have one. That gap is real and still open — I wrote this down in the new test's javadoc rather than silently leaving it.

Mutation proofs (revert -> real failure -> restore)

F1 — reverted SPLIT_KEYS to Set.of("health", "coordinator") and removed the fleet.leaders branch from changedSplitKeys. Ran ConfigRefTest#changingFleetLeadersIsReportedAsSplit:

org.opentest4j.AssertionFailedError: [] ==> expected: <1> but was: <0>
	at dev.ltms.fleet.config.ConfigRefTest.changingFleetLeadersIsReportedAsSplit(ConfigRefTest.java:674)
[ERROR] Tests run: 2, Failures: 1, Errors: 0, Skipped: 0

Restored the fix, re-ran, green (Tests run: 26, Failures: 0 for the full ConfigRefTest class).

F2 — removed the coordinator branch from changedSplitKeys only (kept "coordinator" in SPLIT_KEYS, exactly reproducing the issue's own repro). Ran both coverage tests:

[INFO] Running dev.ltms.fleet.config.ConfigRefTopLevelReportingCoverageTest
ConfigRef.changedSplitKeys reporting coverage — 3 SPLIT_KEYS, 2 verified
[ERROR] ConfigRefTopLevelReportingCoverageTest.everySplitKeyIsActuallyReportedByChangedSplitKeys
  ==> expected: <[]> but was: <[coordinator]>
[INFO] Running dev.ltms.fleet.config.ConfigRefTopLevelCoverageTest
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0   <-- stayed green, as measured in the issue

New test caught it; the old shape checker did not (matches F2's own description). Restored the fix, re-ran clean, both green.

Build

Full unpiped build in the worktree:

cd fleetd && mvn clean install
...
[INFO] Tests run: 1352, Failures: 0, Errors: 0, Skipped: 0
[INFO] BUILD SUCCESS

Files changed

  • fleetd/src/main/java/dev/ltms/fleet/config/ConfigRef.java — fleet moved into SPLIT_KEYS; changedSplitKeys compares fleet.leaders; changedColdKeys/changedSplitKeys made package-private for the new coverage test; class doc rewritten (Hot/Split bullets, denominator note).
  • fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTest.java — two new tests: changingFleetLeadersIsReportedAsSplit, changingFleetTabLabelWithoutLeadersStaysFullyHot.
  • fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTopLevelCoverageTest.java — HOT_EXCLUDED_TOP_LEVEL_KEYS no longer includes fleet; javadoc updated.
  • fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTopLevelReportingCoverageTest.java — new file, F2's mechanism.

ConfigRefProfileCoverageTest.java was not touched (constraint #5).

Caveats for review

  • F2's reflection coverage does not cover DEFERRED_TOP_LEVEL_KEYS — written down in the new test's javadoc, not fixed here (narrow-scope choice, explained above).
  • Spotted but out of scope: the Cold bullet's prose in ConfigRef.java's class doc lists bind:, herdrSocket:, broker: and auth: but omits memberHerdrSocket:, even though COLD_KEYS has 5 entries. Pre-existing, unrelated to F1/F2, not touched.
  • fleetd/fleetd.yaml is gitignored and absent from my worktree; nothing in this PR depends on its contents.
## Summary fleetd#333 has two findings about `ConfigRef`'s reload report. Both are fixed. ### F1 — `fleet:` is a split key, not a hot key `fleet.leaders` is read only at startup. Two places use it: - `Fleetd.java:281` builds the `LeadTabScanner`'s `tab label -> lead name` map. This map decides if a caller's pane is a lead. It is wired into `CallerResolver.withLeadsAndMembers` at `Fleetd.java:620/624`. - `Fleetd.java:315` calls `LeadLauncher.ensureLeads()`. This auto-launches each declared lead up to its `instances` count. Neither one rebuilds when config reloads. So if an operator adds a lead, removes a lead, or gives a lead a new `tab:` label, the daemon needs a restart. Before this fix, `fleet` sat in `ConfigRefTopLevelCoverageTest`'s `HOT_EXCLUDED_TOP_LEVEL_KEYS` escape hatch. A reload that changed only `fleet.leaders` reported a bare "config reloaded". This is wrong. The operator edits a lead's tab, sees "config reloaded", and the pane still resolves as a worker, not a lead. The rest of `fleet:` — role pools (`architects`/`developers`/`reviewers`), `charters`, and `tabLabel` — really is hot. It is read live through the `CompositePeerLauncher` supplier. Fix: moved `fleet` into `ConfigRef.SPLIT_KEYS`. `changedSplitKeys` now compares `fleet.leaders` on its own, not the whole `Fleet` record. I chose this on purpose: comparing the whole record would report "split" (and "needs a restart") even for a `tabLabel`-only change, which is fully hot. That would be an over-claim — the same kind of wrong report this class exists to stop, just in the other direction. Updated the class doc (`ConfigRef.java`) and the escape-hatch doc (`ConfigRefTopLevelCoverageTest.java`) to match, and re-counted the denominator: 22 top-level `FleetConfig` components — 5 cold, 11 deferred, 3 split, 3 hot-excluded. ### F2 — `SPLIT_KEYS`/`COLD_KEYS` membership does not prove any reporting code exists I measured this live: I removed the `coordinator` branch from `changedSplitKeys`, but left `"coordinator"` in `SPLIT_KEYS`. Result: - `ConfigRefTopLevelCoverageTest` (the shape checker) stayed green. It only reads set membership. - `changedSplitKeys`'s own "kept in step" `assert` stayed green too. It only checks that reported entries are a *subset* of `SPLIT_KEYS`, never that every `SPLIT_KEYS` member produced a report. So a future split (or cold) key could be added as a bare string in a `Set.of(...)` with no comparison behind it, and the whole suite would stay green — the exact shape as F1. Fix: added `ConfigRefTopLevelReportingCoverageTest`. It is the `ConfigRefProfileCoverageTest` mechanism, one level up. It builds `FleetConfig` pairs by reflection that differ in exactly one top-level component, then calls the real (now package-private) `ConfigRef.changedColdKeys`/`changedSplitKeys` methods to prove each `COLD_KEYS`/`SPLIT_KEYS` member is actually reported. I scoped this to `COLD_KEYS` and `SPLIT_KEYS`, not the full `DEFERRED_TOP_LEVEL_KEYS` set. Two reasons: (1) that is where fleetd#333 actually found and measured the gap; (2) most `DEFERRED_TOP_LEVEL_KEYS` members already have an individual behavioural test in `ConfigRefTest` naming them by key (`worktreeGroup`, `primary`, `configReload`, profile launch settings, `exhaustedPattern`, `errorPattern`, `ideProjectDir`). A few (`guard`, `lifecycle`, `leadHeartbeat`, `spawnReadyTimeoutMs`/`spawnReadyPollMs`, `quarantineCooldownSeconds`) do not have one. That gap is real and still open — I wrote this down in the new test's javadoc rather than silently leaving it. ## Mutation proofs (revert -> real failure -> restore) **F1** — reverted `SPLIT_KEYS` to `Set.of("health", "coordinator")` and removed the `fleet.leaders` branch from `changedSplitKeys`. Ran `ConfigRefTest#changingFleetLeadersIsReportedAsSplit`: ``` org.opentest4j.AssertionFailedError: [] ==> expected: <1> but was: <0> at dev.ltms.fleet.config.ConfigRefTest.changingFleetLeadersIsReportedAsSplit(ConfigRefTest.java:674) [ERROR] Tests run: 2, Failures: 1, Errors: 0, Skipped: 0 ``` Restored the fix, re-ran, green (`Tests run: 26, Failures: 0` for the full `ConfigRefTest` class). **F2** — removed the `coordinator` branch from `changedSplitKeys` only (kept `"coordinator"` in `SPLIT_KEYS`, exactly reproducing the issue's own repro). Ran both coverage tests: ``` [INFO] Running dev.ltms.fleet.config.ConfigRefTopLevelReportingCoverageTest ConfigRef.changedSplitKeys reporting coverage — 3 SPLIT_KEYS, 2 verified [ERROR] ConfigRefTopLevelReportingCoverageTest.everySplitKeyIsActuallyReportedByChangedSplitKeys ==> expected: <[]> but was: <[coordinator]> [INFO] Running dev.ltms.fleet.config.ConfigRefTopLevelCoverageTest [INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0 <-- stayed green, as measured in the issue ``` New test caught it; the old shape checker did not (matches F2's own description). Restored the fix, re-ran clean, both green. ## Build Full unpiped build in the worktree: ``` cd fleetd && mvn clean install ... [INFO] Tests run: 1352, Failures: 0, Errors: 0, Skipped: 0 [INFO] BUILD SUCCESS ``` ## Files changed - `fleetd/src/main/java/dev/ltms/fleet/config/ConfigRef.java` — `fleet` moved into `SPLIT_KEYS`; `changedSplitKeys` compares `fleet.leaders`; `changedColdKeys`/`changedSplitKeys` made package-private for the new coverage test; class doc rewritten (Hot/Split bullets, denominator note). - `fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTest.java` — two new tests: `changingFleetLeadersIsReportedAsSplit`, `changingFleetTabLabelWithoutLeadersStaysFullyHot`. - `fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTopLevelCoverageTest.java` — `HOT_EXCLUDED_TOP_LEVEL_KEYS` no longer includes `fleet`; javadoc updated. - `fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTopLevelReportingCoverageTest.java` — new file, F2's mechanism. `ConfigRefProfileCoverageTest.java` was not touched (constraint #5). ## Caveats for review - F2's reflection coverage does not cover `DEFERRED_TOP_LEVEL_KEYS` — written down in the new test's javadoc, not fixed here (narrow-scope choice, explained above). - Spotted but out of scope: the Cold bullet's prose in `ConfigRef.java`'s class doc lists `bind:, herdrSocket:, broker: and auth:` but omits `memberHerdrSocket:`, even though `COLD_KEYS` has 5 entries. Pre-existing, unrelated to F1/F2, not touched. - `fleetd/fleetd.yaml` is gitignored and absent from my worktree; nothing in this PR depends on its contents.
agent added 1 commit 2026-09-04 10:37:28 +02:00
fleetd#333: fleet.leaders is split too, and split membership now proves reporting exists
CI / contract (pull_request) Successful in 1m12s
Details
CI / build (pull_request) Failing after 1m59s
Details
3aca53b967 
F1: fleet: was sitting in ConfigRefTopLevelCoverageTest's HOT_EXCLUDED_TOP_LEVEL_KEYS
escape hatch, even though fleet.leaders is read only at startup (LeadTabScanner's
identity map, LeadLauncher.ensureLeads) while the rest of fleet: (role pools,
charters, tabLabel) is live. A reload changing only fleet.leaders reported a bare
"config reloaded" -- the operator edits a lead's tab: label, sees the reload
succeed, and the pane keeps resolving as a worker. Moved fleet into
ConfigRef.SPLIT_KEYS; changedSplitKeys now compares fleet.leaders specifically
(not the whole Fleet record, which would over-claim "restart" for a tabLabel-only
change) and names both halves in the message.

F2: membership in SPLIT_KEYS/COLD_KEYS never proved a matching branch existed in
changedSplitKeys/changedColdKeys -- measured by dropping the coordinator branch
while leaving "coordinator" in SPLIT_KEYS: both ConfigRefTopLevelCoverageTest and
the in-method "kept in step" assert stayed green. Added
ConfigRefTopLevelReportingCoverageTest, the ConfigRefProfileCoverageTest mechanism
one level up: reflection-built FleetConfig pairs that differ in exactly one
top-level component, calling the real (now package-private) changedColdKeys/
changedSplitKeys to prove each COLD_KEYS/SPLIT_KEYS member is actually reported.
Scoped to split+cold, not deferred -- see the new test's javadoc for why and what
that leaves open.

Both findings carry a behavioural test in ConfigRefTest plus a mutation proof
(revert -> real failure -> restore) recorded in the PR description.
ltms closed this pull request 2026-09-04 10:44:41 +02:00
Some checks are pending
CI / contract (pull_request) Successful in 1m12s
Details
CI / build (pull_request) Failing after 1m59s
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#336
Delete Branch "worker/fleetd-333-281f46-18"

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?