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

health: and coordinator: are split keys — add a fourth reload class that says so, then make the top-level list prove its own coverage #330

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

Closes the question #326 left open. I have made the decision; this ticket implements it. Do not re-open the options — implement what is below, and push back only if you find a fact that breaks it.

The facts, which I re-measured today

Every reader of either key, outside FleetConfig itself:

Fleetd.java:502              openLeadMailbox(cfg.coordinator(), ...)        startup snapshot
Fleetd.java:556, 560, 561    cfg.health()...                               startup snapshot
Fleetd.java:563              cfg.health().notifications()...               startup snapshot
Fleetd.java:648-650          config.get().health()                         LIVE, per fleet_profiles call
MemberEnvAllowList.java:165  config.coordinator()                          LIVE, per spawn
                             (reached from HerdrPeerLauncher.java:1530, config.get())

So each key is read both ways, at different sites. Neither is hot, neither is deferred, neither is cold. That is a fact about the code, not an opinion about the taxonomy.

The decision

Add a fourth class: split. A reload that changes a split key reports it by name and says which part is live and which needs a restart.

Why not the alternatives — so nobody re-litigates this:

  • "Report the whole key as deferred" is the cheap option and I rejected it. It tells the operator to restart when the live half already applied. That is not free: a restart drops in-flight tickets and rendezvous.
  • Per-sub-field classification was the accurate-looking option, and it is more machinery than the problem needs. It also forces a rule for "both halves changed in one reload", which is a question a split report does not have to answer, because it names both halves every time.
  • Making the frozen reader live is out. For coordinator it means changing selfId under a running daemon, and selfId names this daemon's own AMQP inbox queue (lead.<selfId>.inbox). A peer lead that learned the old coord-id would not discover the new one. That is a distributed-identity change, not a reconnect, and nothing handles it.

The asymmetry that decided it. Over-claiming a restart costs an unnecessary restart, which the operator sees and can recover from. Under-claiming — today's behaviour — tells the operator a change applied when it did not, and ConfigRef's own doc calls that "the worst thing a reload can do to an operator debugging one". Both option 1 and split stop the under-claim. Only split is true.

Unit 1 — the split class

Goal: a reload that changes health: or coordinator: reports that key by name, and the message says what is live and what needs a restart.

Invariants:

  1. Do not make anything take effect live. Same rule as #323 and #326. This is about what a reload reports.
  2. A split change must not refuse the reload. Cold refuses the whole reload; split does not. The live half genuinely applies, so refusing would leave the operator worse off than today.
  3. Outcome.applied() stays true for a split change, for the same reason.
  4. Do not weaken MemberEnvAllowList. If you find a hole there, report it; do not fix it here.
  5. ConfigRefProfileCoverageTest's exclusion-set assertion stays untouched.

Candidate mechanism, as a candidate only: a SPLIT_KEYS set beside COLD_KEYS, and a separate list on Outcome so a caller can tell deferred from split. Decide it yourself and justify it — in particular, decide whether split deserves its own field on Outcome or belongs in deferred with a different message. Say which and why. A tested deviation you report is a good outcome.

What the message must contain, whatever shape you pick — the operator has to be able to act on it:

  • for health: — the monitor itself (enabled, interval, workingSuspectAfter) is frozen at startup and needs a restart; the coverage string fleet_profiles reports is read live and already applied.
  • for coordinator: — the LeadMailbox connection (uri, uriEnv, selfId, prefetch) is opened once and needs a restart; the broker URI env-var name used to keep that variable out of a member's environment is re-read on every spawn and already applied.

Also update ConfigRef's class doc. It currently describes three classes and says so explicitly. It also carries a denominator note I added, listing health and coordinator as undecided — that note is now stale and must say split instead.

Unit 2 — the top-level coverage checker

Only after unit 1. #326 held this back because health and coordinator had no bucket to go in. split gives them one, so the checker is now well-defined.

Goal: a new top-level FleetConfig component cannot be added without someone triaging it.

What it must do: enumerate FleetConfig.class.getRecordComponents() and require every one to be accounted for — named in COLD_KEYS, compared in changedDeferredKeys, in SPLIT_KEYS, or in an explicit hot-exclusion set with a citation. Print its own denominator on every run, the way ConfigRefProfileCoverageTest does.

Pin the escape hatch. The hot-exclusion set is this checker's own way to be silenced — assert its exact contents, with a message saying a key belongs there only if it is read live off the config supplier, never because adding it makes the build pass. That is not optional: in #323 the identical hatch let two keys be silently re-broken with the suite green, and I only found it by mutating the checker itself.

Say plainly what it cannot do. A checker over the record's shape cannot verify that a citation is true — "read live off config.get()" is a fact about Fleetd.java and HerdrPeerLauncher.java, invisible from here. Write that limitation into the test's javadoc rather than letting the next reader assume the check is stronger than it is.

Known-good starting values, measured by me today, so you are not deriving them from scratch — but verify each one rather than copying it:

  • 22 top-level components.
  • Cold: bind, herdrSocket, memberHerdrSocket, broker, auth.
  • Deferred (compared in changedDeferredKeys): guard, leadHeartbeat, lifecycle, quarantineCooldownSeconds, spawnReadyTimeoutMs/spawnReadyPollMs, worktreeGroup, worktreeRoot, primary, configReload.
  • Split: health, coordinator.
  • Hot: profiles, placement, fleet, memberCredentials, memberLoginShell.

If that adds up differently from 22 when you count it, trust your count and say so — I would rather be corrected than confirmed.

Rules

  • Each unit needs a test that fails without it.
  • Mutation proof required per unit: revert the change, quote the real failure output, restore it. For unit 2, also mutate the checker: move a compared key into the hot-exclusion set and show the pinned assertion catches it.
  • 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 seven readers listed at the top are the complete set. I ran the search today.
  • memberCredentials and memberLoginShell are hot and correctly absent from ConfigRef — live config.get() reads at Fleetd.java:198, 205, 729 and HerdrPeerLauncher#configuredMemberLoginShell.
  • coordinator.uriEnv changing in a reload can expose the old variable name to a later spawn only when that name is separately allow-listed for an unrelated reason (a profile's env: map, or memberCredentials.allow:), because the exclusion only ever removes a name from an allow set and never adds one. Checked in #326. Do not re-analyse it, and do not change MemberEnvAllowList.
Closes the question #326 left open. **I have made the decision; this ticket implements it.** Do not re-open the options — implement what is below, and push back only if you find a fact that breaks it. ## The facts, which I re-measured today Every reader of either key, outside `FleetConfig` itself: ``` Fleetd.java:502 openLeadMailbox(cfg.coordinator(), ...) startup snapshot Fleetd.java:556, 560, 561 cfg.health()... startup snapshot Fleetd.java:563 cfg.health().notifications()... startup snapshot Fleetd.java:648-650 config.get().health() LIVE, per fleet_profiles call MemberEnvAllowList.java:165 config.coordinator() LIVE, per spawn (reached from HerdrPeerLauncher.java:1530, config.get()) ``` So each key is read **both** ways, at different sites. Neither is hot, neither is deferred, neither is cold. That is a fact about the code, not an opinion about the taxonomy. ## The decision **Add a fourth class: `split`.** A reload that changes a split key reports it by name and says which part is live and which needs a restart. Why not the alternatives — so nobody re-litigates this: - **"Report the whole key as deferred"** is the cheap option and I rejected it. It tells the operator to restart when the live half already applied. That is not free: a restart drops in-flight tickets and rendezvous. - **Per-sub-field classification** was the accurate-looking option, and it is more machinery than the problem needs. It also forces a rule for "both halves changed in one reload", which is a question a `split` report does not have to answer, because it names both halves every time. - **Making the frozen reader live** is out. For `coordinator` it means changing `selfId` under a running daemon, and `selfId` names this daemon's own AMQP inbox queue (`lead.<selfId>.inbox`). A peer lead that learned the old coord-id would not discover the new one. That is a distributed-identity change, not a reconnect, and nothing handles it. **The asymmetry that decided it.** Over-claiming a restart costs an unnecessary restart, which the operator sees and can recover from. Under-claiming — today's behaviour — tells the operator a change applied when it did not, and `ConfigRef`'s own doc calls that "the worst thing a reload can do to an operator debugging one". Both option 1 and `split` stop the under-claim. Only `split` is true. ## Unit 1 — the `split` class **Goal:** a reload that changes `health:` or `coordinator:` reports that key by name, and the message says what is live and what needs a restart. **Invariants:** 1. **Do not make anything take effect live.** Same rule as #323 and #326. This is about what a reload *reports*. 2. **A split change must not refuse the reload.** Cold refuses the whole reload; split does not. The live half genuinely applies, so refusing would leave the operator worse off than today. 3. **`Outcome.applied()` stays `true`** for a split change, for the same reason. 4. **Do not weaken `MemberEnvAllowList`.** If you find a hole there, report it; do not fix it here. 5. **`ConfigRefProfileCoverageTest`'s exclusion-set assertion stays untouched.** **Candidate mechanism, as a candidate only:** a `SPLIT_KEYS` set beside `COLD_KEYS`, and a separate list on `Outcome` so a caller can tell `deferred` from `split`. **Decide it yourself and justify it** — in particular, decide whether `split` deserves its own field on `Outcome` or belongs in `deferred` with a different message. Say which and why. A tested deviation you report is a good outcome. **What the message must contain**, whatever shape you pick — the operator has to be able to act on it: - for `health:` — the monitor itself (enabled, interval, `workingSuspectAfter`) is frozen at startup and needs a restart; the coverage string `fleet_profiles` reports is read live and already applied. - for `coordinator:` — the `LeadMailbox` connection (`uri`, `uriEnv`, `selfId`, `prefetch`) is opened once and needs a restart; the broker URI env-var **name** used to keep that variable out of a member's environment is re-read on every spawn and already applied. **Also update `ConfigRef`'s class doc.** It currently describes three classes and says so explicitly. It also carries a denominator note I added, listing `health` and `coordinator` as *undecided* — that note is now stale and must say `split` instead. ## Unit 2 — the top-level coverage checker Only after unit 1. #326 held this back because `health` and `coordinator` had no bucket to go in. `split` gives them one, so the checker is now well-defined. **Goal:** a new top-level `FleetConfig` component cannot be added without someone triaging it. **What it must do:** enumerate `FleetConfig.class.getRecordComponents()` and require every one to be accounted for — named in `COLD_KEYS`, compared in `changedDeferredKeys`, in `SPLIT_KEYS`, or in an explicit hot-exclusion set with a citation. **Print its own denominator on every run**, the way `ConfigRefProfileCoverageTest` does. **Pin the escape hatch.** The hot-exclusion set is this checker's own way to be silenced — assert its exact contents, with a message saying a key belongs there only if it is read live off the config supplier, never because adding it makes the build pass. That is not optional: in #323 the identical hatch let two keys be silently re-broken with the suite green, and I only found it by mutating the checker itself. **Say plainly what it cannot do.** A checker over the record's shape cannot verify that a citation is *true* — "read live off `config.get()`" is a fact about `Fleetd.java` and `HerdrPeerLauncher.java`, invisible from here. Write that limitation into the test's javadoc rather than letting the next reader assume the check is stronger than it is. **Known-good starting values**, measured by me today, so you are not deriving them from scratch — but verify each one rather than copying it: - 22 top-level components. - Cold: `bind`, `herdrSocket`, `memberHerdrSocket`, `broker`, `auth`. - Deferred (compared in `changedDeferredKeys`): `guard`, `leadHeartbeat`, `lifecycle`, `quarantineCooldownSeconds`, `spawnReadyTimeoutMs`/`spawnReadyPollMs`, `worktreeGroup`, `worktreeRoot`, `primary`, `configReload`. - Split: `health`, `coordinator`. - Hot: `profiles`, `placement`, `fleet`, `memberCredentials`, `memberLoginShell`. If that adds up differently from 22 when you count it, trust your count and say so — I would rather be corrected than confirmed. ## Rules - Each unit needs a test that fails without it. - Mutation proof required per unit: revert the change, quote the real failure output, restore it. For unit 2, also mutate the **checker**: move a compared key into the hot-exclusion set and show the pinned assertion catches it. - **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 seven readers listed at the top are the complete set. I ran the search today. - `memberCredentials` and `memberLoginShell` are hot and correctly absent from `ConfigRef` — live `config.get()` reads at `Fleetd.java:198, 205, 729` and `HerdrPeerLauncher#configuredMemberLoginShell`. - `coordinator.uriEnv` changing in a reload can expose the old variable name to a later spawn **only** when that name is separately allow-listed for an unrelated reason (a profile's `env:` map, or `memberCredentials.allow:`), because the exclusion only ever removes a name from an allow set and never adds one. Checked in #326. Do not re-analyse it, and do not change `MemberEnvAllowList`.
ltms commented 2026-09-04 10:22:44 +02:00
Author
Owner
Copy Link
Copy Source

Merged as 7b918c5 (PR #331). Verified by me.

Full build after the merge, unpiped:

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

FleetConfig top-level coverage — 22 components total: 5 cold [broker, auth, memberHerdrSocket,
herdrSocket, bind], 11 deferred [leadHeartbeat, quarantineCooldownSeconds, configReload,
spawnReadyPollMs, worktreeGroup, worktreeRoot, profiles, guard, primary, spawnReadyTimeoutMs,
lifecycle], 2 split [health, coordinator], 4 hot-excluded [memberLoginShell, fleet, placement,
memberCredentials]

The deviation was right

Moving profiles from my "hot" list into deferred is correct and I should have had it that way. changedDeferredKeys demonstrably compares it, twice over, and citing "read live" for the whole key would be false — only weight/maxLoad/credentialId are live, and ConfigRefProfileCoverageTest already covers those at the sub-field level. My tally was wrong by one key; the worker's is right.

Giving split its own field on Outcome rather than folding it into deferred is also the right call, and for the reason stated: a caller that branches on the list needs to tell "the whole change waits" from "half already applied" without re-parsing prose.

Mutation S — mine, different from the worker's

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 tests catch it, so both current split keys are behaviourally pinned. But note what did NOT fire: neither the new coverage test nor the assert in changedSplitKeys. Both are one-way. That is finding 2 below.

Two findings — new ticket, not blockers

1. fleet: is a split key and is filed as hot. This one is live. I checked it myself:

Fleetd.java:281            var leaders = cfg.fleet().leaders();          startup snapshot
LeadLauncher.java:56, 77   private final FleetConfig cfg;               a FROZEN config, not a supplier

LeadLauncher holds a FleetConfig, not a Supplier<FleetConfig>. So fleet.leaders is frozen at startup — while the role pools, charters and tabLabel are read live. That is the same shape as health and coordinator, exactly. Today, adding, removing or re-tab-ing a lead under fleet.leaders needs a restart and a reload reports nothing about it. That is the third instance of the bug #323 and #326 each fixed once.

The worker spotted it, flagged it as a caveat, and did not expand scope — which is what I asked for. The brief is what got it wrong. I wrote fleet into the ticket's "known-good starting values" under Hot, and I scoped split to exactly health/coordinator. The worker corrected me on profiles and obeyed me on fleet. So a genuinely split key is now sitting in the checker's own escape hatch, blessed by the checker, on the checker's first commit. My list, my scope, my defect.

2. Membership in SPLIT_KEYS is not wired to changedSplitKeys. The coverage test treats a name in SPLIT_KEYS as proof the key is triaged. It cannot see whether changedSplitKeys actually has a branch for it. So the cheapest way to pass this checker for a future split key is to add one string to a Set and write no reporting code at all — and mutation S shows nothing but the hand-written behavioural tests stands in the way.

The same one-way shape is pre-existing on the cold side: assert COLD_KEYS.containsAll(changed) catches "reported but not listed" and never "listed but not reported", and DEFERRED_TOP_LEVEL_KEYS in the test is a second hand-written copy of what changedDeferredKeys does. The worker mirrored the existing pattern faithfully; the pattern is what is one-way.

Credit where it is due: the test's own javadoc already says it "CANNOT prove that any of the citations are true" and names what does prove them. That is the right disclosure, and it is what let me find both of these quickly instead of trusting a green run.

Both go to a follow-up ticket. Closing this one.

Merged as `7b918c5` (PR #331). Verified by me. Full build after the merge, unpiped: ``` Tests run: 1348, Failures: 0, Errors: 0, Skipped: 0 BUILD SUCCESS FleetConfig top-level coverage — 22 components total: 5 cold [broker, auth, memberHerdrSocket, herdrSocket, bind], 11 deferred [leadHeartbeat, quarantineCooldownSeconds, configReload, spawnReadyPollMs, worktreeGroup, worktreeRoot, profiles, guard, primary, spawnReadyTimeoutMs, lifecycle], 2 split [health, coordinator], 4 hot-excluded [memberLoginShell, fleet, placement, memberCredentials] ``` ## The deviation was right Moving `profiles` from my "hot" list into deferred is correct and I should have had it that way. `changedDeferredKeys` demonstrably compares it, twice over, and citing "read live" for the whole key would be false — only `weight`/`maxLoad`/`credentialId` are live, and `ConfigRefProfileCoverageTest` already covers those at the sub-field level. My tally was wrong by one key; the worker's is right. Giving `split` its own field on `Outcome` rather than folding it into `deferred` is also the right call, and for the reason stated: a caller that branches on the list needs to tell "the whole change waits" from "half already applied" without re-parsing prose. ### Mutation S — mine, different from the worker's 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 tests catch it, so both current split keys are behaviourally pinned. **But note what did NOT fire:** neither the new coverage test nor the `assert` in `changedSplitKeys`. Both are one-way. That is finding 2 below. ## Two findings — new ticket, not blockers **1. `fleet:` is a split key and is filed as hot. This one is live.** I checked it myself: ``` Fleetd.java:281 var leaders = cfg.fleet().leaders(); startup snapshot LeadLauncher.java:56, 77 private final FleetConfig cfg; a FROZEN config, not a supplier ``` `LeadLauncher` holds a `FleetConfig`, not a `Supplier<FleetConfig>`. So `fleet.leaders` is frozen at startup — while the role pools, `charters` and `tabLabel` are read live. That is the same shape as `health` and `coordinator`, exactly. **Today, adding, removing or re-`tab`-ing a lead under `fleet.leaders` needs a restart and a reload reports nothing about it.** That is the third instance of the bug #323 and #326 each fixed once. The worker spotted it, flagged it as a caveat, and did not expand scope — which is what I asked for. **The brief is what got it wrong.** I wrote `fleet` into the ticket's "known-good starting values" under Hot, and I scoped split to exactly `health`/`coordinator`. The worker corrected me on `profiles` and obeyed me on `fleet`. So a genuinely split key is now sitting in the checker's own escape hatch, blessed by the checker, on the checker's first commit. My list, my scope, my defect. **2. Membership in `SPLIT_KEYS` is not wired to `changedSplitKeys`.** The coverage test treats a name in `SPLIT_KEYS` as proof the key is triaged. It cannot see whether `changedSplitKeys` actually has a branch for it. So the cheapest way to pass this checker for a future split key is to add one string to a `Set` and write no reporting code at all — and mutation S shows nothing but the hand-written behavioural tests stands in the way. The same one-way shape is pre-existing on the cold side: `assert COLD_KEYS.containsAll(changed)` catches "reported but not listed" and never "listed but not reported", and `DEFERRED_TOP_LEVEL_KEYS` in the test is a second hand-written copy of what `changedDeferredKeys` does. The worker mirrored the existing pattern faithfully; the pattern is what is one-way. Credit where it is due: the test's own javadoc already says it "CANNOT prove that any of the citations are true" and names what does prove them. That is the right disclosure, and it is what let me find both of these quickly instead of trusting a green run. Both go to a follow-up ticket. Closing this one.
ltms closed this issue 2026-09-04 10:22:50 +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#330
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?