diff --git a/fleetd/src/main/java/dev/ltms/fleet/config/ConfigRef.java b/fleetd/src/main/java/dev/ltms/fleet/config/ConfigRef.java index fcd38c6..f736af9 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/config/ConfigRef.java +++ b/fleetd/src/main/java/dev/ltms/fleet/config/ConfigRef.java @@ -22,7 +22,7 @@ import java.util.function.Supplier; * choice rather than an accident of where the field was initialised. * *

Not every key can change under a running daemon

- * Keys fall into three classes, and the difference is about what already exists when the reload + * Keys fall into four classes, and the difference is about what already exists when the reload * happens — not about how important the key is. * * * - *

The denominator, measured on 2026-09-04 (fleetd #326). {@code FleetConfig} has - * 22 top-level record components. Four of them are named nowhere in this file, and the reason - * differs per key, so do not read "absent" as "forgotten": - *

- * The point of writing the count down: "not mentioned in this file" looks identical for a key that - * is correctly hot and for a key nobody triaged. Twice now — {@code worktreeGroup} (#323) and - * {@code primary}/{@code configReload} (#326) — the second kind hid among the first. + *

The denominator, measured on 2026-09-04 (fleetd #330). {@code FleetConfig} has + * 22 top-level record components. Two of them are named nowhere in this file, and the reason is the + * same for both: {@code memberCredentials} and {@code memberLoginShell} are hot and + * correctly absent — both are read live off {@code config.get()} at spawn time + * ({@code Fleetd.java:198, 205, 729} and {@code HerdrPeerLauncher#configuredMemberLoginShell}), so a + * reload takes effect on the next spawn with no entry needed here. + * {@code health} and {@code coordinator} used to be a third kind — undecided, not + * hot — until fleetd #330 added the split class above and gave them a home. A + * reload touching either used to report a bare "config reloaded", which under-claimed; now it names + * the key and says which half is which. + *

The point of writing the count down: "not mentioned in this file" looks identical for a key + * that is correctly hot and for a key nobody triaged. Twice now — {@code worktreeGroup} (#323) and + * {@code primary}/{@code configReload} (#326) — the second kind hid among the first. A top-level + * coverage checker in the {@link ConfigRefProfileCoverageTest} shape (one level up, over + * {@code FleetConfig} itself rather than {@code FleetConfig.Profile}) proves this file's four + * classes exhaust the record's components — see {@code ConfigRefTopLevelCoverageTest}. * *

A cold change refuses the whole reload. Not the hot half applied and the cold * half warned about: that would leave the running daemon in a state matching no file on disk, which * is the worst thing a reload can do to an operator debugging one. Refusing keeps the invariant that * the live config is always some version of the file, and the message names the keys that must - * change through a restart. + * change through a restart. A split change does not refuse, for a different reason than a + * deferred change does not: its live half genuinely took effect, so refusing would throw that away + * and leave the operator worse off than the partial-but-honest report {@link Outcome#split()} gives. * *

A reload that fails to parse or fails validation is also refused, and the previous config keeps * running. A config file being edited is normally read once mid-save; degrading a working daemon @@ -109,10 +137,24 @@ public final class ConfigRef implements Supplier { private static final Logger log = LoggerFactory.getLogger(ConfigRef.class); - /** Keys that cannot change under a running daemon — see the class doc. */ - private static final Set COLD_KEYS = + /** + * Keys that cannot change under a running daemon — see the class doc. + * + *

Package-private (not {@code private}) so {@code ConfigRefTopLevelCoverageTest} can fold it + * into the top-level triage it checks, the same way it reads {@link #SPLIT_KEYS}. + */ + static final Set COLD_KEYS = Set.of("bind", "herdrSocket", "memberHerdrSocket", "broker", "auth"); + /** + * Keys read BOTH off the startup snapshot and live off {@link #get()} at different sites, so + * neither the hot, deferred nor cold class fits them as a whole — see the class doc's Split + * bullet (fleetd #330). A changed split key is accepted ({@link Outcome#applied()} stays + * {@code true}) and reported by name, with a message naming which half is live and which needs + * a restart. + */ + static final Set SPLIT_KEYS = Set.of("health", "coordinator"); + private final Path path; private final AtomicReference current; @@ -140,25 +182,37 @@ public final class ConfigRef implements Supplier { /** * What a reload attempt did. * + *

{@code split} is a separate field from {@code deferred} rather than a differently-worded + * entry inside it, because the two carry different guarantees for any caller that branches on + * them rather than just printing {@link #summary()}: every {@code deferred} entry means "this + * key's whole change waits for a restart", while every {@code split} entry means "part of this + * key's change already applied, and the message says which part" — collapsing them would force + * a caller to re-parse the message to tell those apart. See the class doc's Split bullet + * (fleetd #330) for why the key needs this at all. + * * @param applied true when the new config is now live * @param coldKeys cold keys whose value changed, which is why an unapplied reload was refused - * @param deferred keys that changed and were accepted, but whose effect waits for a restart + * @param deferred keys that changed and were accepted, but whose effect waits entirely on a + * restart + * @param split split keys that changed and were accepted, each named with which half of it + * is already live and which half waits for a restart * @param error the parse or validation failure that refused the reload, else {@code null} */ public record Outcome(boolean applied, List coldKeys, List deferred, - String error) { + List split, String error) { public Outcome { coldKeys = List.copyOf(coldKeys); deferred = List.copyOf(deferred); + split = List.copyOf(split); } static Outcome refusedCold(List keys) { - return new Outcome(false, keys, List.of(), null); + return new Outcome(false, keys, List.of(), List.of(), null); } static Outcome failed(String error) { - return new Outcome(false, List.of(), List.of(), error); + return new Outcome(false, List.of(), List.of(), List.of(), error); } /** A one-line summary for the operator — the reason, not just the verdict. */ @@ -170,11 +224,18 @@ public final class ConfigRef implements Supplier { return "config reload refused — these keys cannot change under a running daemon: " + String.join(", ", coldKeys) + ". Restart fleetd to apply them."; } - if (!deferred.isEmpty()) { - return "config reloaded; these changes need a restart to take effect: " - + String.join(", ", deferred); + if (deferred.isEmpty() && split.isEmpty()) { + return "config reloaded"; } - return "config reloaded"; + StringBuilder out = new StringBuilder("config reloaded"); + if (!deferred.isEmpty()) { + out.append("; these changes need a restart to take effect: ") + .append(String.join(", ", deferred)); + } + if (!split.isEmpty()) { + out.append("; partially live — ").append(String.join(" | ", split)); + } + return out.toString(); } } @@ -215,8 +276,9 @@ public final class ConfigRef implements Supplier { } List deferred = changedDeferredKeys(old, fresh); + List split = changedSplitKeys(old, fresh); current.set(fresh); - Outcome out = new Outcome(true, List.of(), deferred, null); + Outcome out = new Outcome(true, List.of(), deferred, split, null); log.info(out.summary()); return out; } @@ -325,6 +387,32 @@ public final class ConfigRef implements Supplier { return changed; } + /** + * Split keys whose value differs between the running config and the candidate — see the class + * doc's Split bullet (fleetd #330). Unlike {@link #changedDeferredKeys}, this does not try to + * tell which sub-field moved: any change to {@code health:} or {@code coordinator:} gets the + * same fixed message, because the message already names both halves every time, so there is no + * "which half changed" question left for the caller to answer. + */ + private static List changedSplitKeys(FleetConfig old, FleetConfig fresh) { + List changed = new ArrayList<>(); + if (!Objects.equals(old.health(), fresh.health())) { + changed.add("health: the monitor itself (enabled, interval, workingSuspectAfter) is " + + "frozen at startup and needs a restart; the coverage status fleet_profiles " + + "reports is read live and already applied"); + } + if (!Objects.equals(old.coordinator(), fresh.coordinator())) { + changed.add("coordinator: the LeadMailbox connection (uri, uriEnv, selfId, prefetch) is " + + "opened once and needs a restart; the broker URI env-var name kept out of a " + + "member's environment is read live on every spawn and already applied"); + } + // Kept in step with SPLIT_KEYS the same way changedColdKeys is kept in step with COLD_KEYS — + // every message here must be traceable to one of the two split keys the class doc documents. + assert changed.stream().allMatch(m -> SPLIT_KEYS.stream().anyMatch(k -> m.startsWith(k + ":"))) + : "a split entry was reported that does not start with a SPLIT_KEYS name: " + changed; + return changed; + } + /** * {@link FleetConfig.Profile} record components deliberately left out of * {@link #sameLaunchSettings} because they are read live, not baked in at spawn — see diff --git a/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTest.java b/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTest.java index 853cb0e..b5411a9 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTest.java @@ -570,6 +570,155 @@ class ConfigRefTest { assertEquals(30, ref.get().configReload().intervalSeconds()); } + /** + * fleetd #330: {@code health:} is read both ways — {@code Fleetd.java:556-563} builds the + * monitor off the startup snapshot and never rebuilds it, but {@code Fleetd.java:648-650} reads + * {@code config.get().health()} live on every {@code fleet_profiles} call. A changed value is + * neither purely hot nor purely deferred, so it gets its own {@code split} report naming both + * halves rather than a bare "config reloaded" (which would hide the frozen half) or a plain + * {@code deferred} entry (which would hide that the coverage string already applied). + */ + @Test + void changingHealthIsReportedAsSplit(@TempDir Path dir) throws Exception { + Path f = dir.resolve("fleetd.yaml"); + Files.writeString(f, yaml(""" + health: + enabled: true + intervalSeconds: 30 + """)); + ConfigRef ref = refFor(f); + + Files.writeString(f, yaml(""" + health: + enabled: true + intervalSeconds: 90 + """)); + ConfigRef.Outcome out = ref.reload(); + + assertTrue(out.applied()); + assertTrue(out.deferred().isEmpty(), out.deferred().toString()); + assertEquals(1, out.split().size(), out.split().toString()); + assertTrue(out.split().getFirst().startsWith("health:"), out.split().toString()); + assertTrue(out.split().getFirst().contains("restart"), out.split().toString()); + assertTrue(out.split().getFirst().contains("live"), out.split().toString()); + assertTrue(out.summary().contains("partially live"), out.summary()); + // The snapshot still carries the new value — the monitor itself is what waits for a restart. + assertEquals(90, ref.get().health().intervalSeconds()); + } + + /** + * fleetd #330: {@code coordinator:} is the other split key — {@code Fleetd.java:502} opens the + * {@code LeadMailbox} off the startup snapshot and never reopens it, but + * {@code HerdrPeerLauncher.java:1530} reads {@code config.get().coordinator()} live on every + * spawn to keep the broker URI env-var name out of a member's environment. + */ + @Test + void changingCoordinatorIsReportedAsSplit(@TempDir Path dir) throws Exception { + Path f = dir.resolve("fleetd.yaml"); + Files.writeString(f, yaml(""" + coordinator: + selfId: mac-a + """)); + ConfigRef ref = refFor(f); + + Files.writeString(f, yaml(""" + coordinator: + selfId: mac-b + """)); + ConfigRef.Outcome out = ref.reload(); + + assertTrue(out.applied()); + assertTrue(out.deferred().isEmpty(), out.deferred().toString()); + assertEquals(1, out.split().size(), out.split().toString()); + assertTrue(out.split().getFirst().startsWith("coordinator:"), out.split().toString()); + assertTrue(out.split().getFirst().contains("restart"), out.split().toString()); + assertTrue(out.split().getFirst().contains("live"), out.split().toString()); + // The snapshot still carries the new value — the LeadMailbox connection is what waits for a + // restart; selfId names this daemon's own inbox queue and a peer cannot discover a rename. + assertEquals("mac-b", ref.get().coordinator().selfId()); + } + + /** + * A split change must not refuse the reload (invariant 2 of fleetd #330) and + * {@code Outcome.applied()} must stay {@code true} (invariant 3) — unlike a cold change, the + * live half of a split key genuinely took effect, so refusing would throw that away. + */ + @Test + void aSplitChangeDoesNotRefuseTheReload(@TempDir Path dir) throws Exception { + Path f = dir.resolve("fleetd.yaml"); + Files.writeString(f, yaml("coordinator:\n selfId: mac-a\n")); + ConfigRef ref = refFor(f); + + Files.writeString(f, yaml("coordinator:\n selfId: mac-b\n")); + ConfigRef.Outcome out = ref.reload(); + + assertTrue(out.applied()); + assertTrue(out.error() == null); + assertTrue(out.coldKeys().isEmpty()); + } + + /** + * Both split keys can change in one reload — the report names both, and a caller reading + * {@code split} does not have to guess which half of which key already applied. + */ + @Test + void changingBothSplitKeysReportsBoth(@TempDir Path dir) throws Exception { + Path f = dir.resolve("fleetd.yaml"); + Files.writeString(f, yaml(""" + health: + enabled: true + coordinator: + selfId: mac-a + """)); + ConfigRef ref = refFor(f); + + Files.writeString(f, yaml(""" + health: + enabled: false + coordinator: + selfId: mac-b + """)); + ConfigRef.Outcome out = ref.reload(); + + assertTrue(out.applied()); + assertEquals(2, out.split().size(), out.split().toString()); + assertTrue(out.split().stream().anyMatch(s -> s.startsWith("health:")), out.split().toString()); + assertTrue(out.split().stream().anyMatch(s -> s.startsWith("coordinator:")), out.split().toString()); + } + + /** + * A split key and a deferred key changing in the same reload must both show up, each in its own + * list — proving the two fields do not step on each other and {@link ConfigRef.Outcome#summary()} + * reports both halves of the message. + */ + @Test + void aSplitChangeAndADeferredChangeCoexist(@TempDir Path dir) throws Exception { + Path f = dir.resolve("fleetd.yaml"); + Files.writeString(f, yaml(""" + coordinator: + selfId: mac-a + lifecycle: + drainTimeoutSeconds: 30 + """)); + ConfigRef ref = refFor(f); + + Files.writeString(f, yaml(""" + coordinator: + selfId: mac-b + lifecycle: + drainTimeoutSeconds: 60 + """)); + ConfigRef.Outcome out = ref.reload(); + + assertTrue(out.applied()); + assertEquals(java.util.List.of("lifecycle"), out.deferred()); + assertEquals(1, out.split().size(), out.split().toString()); + assertTrue(out.split().getFirst().startsWith("coordinator:"), out.split().toString()); + assertTrue(out.summary().contains("need a restart") || out.summary().contains("needs a restart"), + out.summary()); + assertTrue(out.summary().contains("partially live"), out.summary()); + } + @Test void aFixedRefHasNoFileAndRefusesToReload() { FleetConfig cfg = new FleetConfig(null, null, null, null, null, null, diff --git a/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTopLevelCoverageTest.java b/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTopLevelCoverageTest.java new file mode 100644 index 0000000..750742b --- /dev/null +++ b/fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTopLevelCoverageTest.java @@ -0,0 +1,167 @@ +package dev.ltms.fleet.config; + +import org.junit.jupiter.api.Test; + +import java.lang.reflect.RecordComponent; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Set; +import java.util.TreeSet; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * fleetd #330: a new top-level {@link FleetConfig} record component must be triaged into a reload + * class before it ships, or it repeats fleetd #323 ({@code worktreeGroup} missing from {@code + * ConfigRef.changedDeferredKeys}) and fleetd #326 ({@code primary}/{@code configReload} missing the + * same way) — a key silently absent from {@link ConfigRef}'s reload machinery, so a reload changing + * only that key reports a bare "config reloaded" for a change the running daemon never picked up. + * + *

This is the top-level counterpart of {@link ConfigRefProfileCoverageTest}: instead of + * enumerating {@link FleetConfig.Profile}'s record components, it enumerates {@link FleetConfig}'s + * own — {@code bind}, {@code health}, {@code memberCredentials}, and so on — and requires each to + * fall into exactly one of four homes: {@link ConfigRef#COLD_KEYS}, {@link #DEFERRED_TOP_LEVEL_KEYS} + * (compared in {@code ConfigRef.changedDeferredKeys}), {@link ConfigRef#SPLIT_KEYS}, or + * {@link #HOT_EXCLUDED_TOP_LEVEL_KEYS} (the escape hatch: read live off the config supplier, so no + * reload bookkeeping is needed for it at all). + * + *

What this checker can and cannot prove

+ * It proves the record's shape is fully triaged: every one of {@code FleetConfig}'s + * components sits in exactly one of the four sets, none sits in two, and the escape hatch + * ({@link #HOT_EXCLUDED_TOP_LEVEL_KEYS}) cannot silently grow without a visible diff to this file. + * That is what "a new component cannot be added without someone triaging it" means in practice. + * + *

It CANNOT prove that any of the citations are true. "Compared in {@code + * changedDeferredKeys}" and "read live off {@code config.get()}" are facts about {@code + * ConfigRef.java}, {@code Fleetd.java} and {@code HerdrPeerLauncher.java} that a reflection-only + * test over {@code FleetConfig}'s shape has no way to inspect — this test would pass identically + * whether or not the cited line still does what the comment next to it says. Trust the citation + * because a person read the source (the exact call sites are named next to each set below), not + * because this test is green. {@link ConfigRefTest} is what behaviourally proves the deferred and + * split keys it covers actually get reported; {@link ConfigRefProfileCoverageTest} does the same, + * behaviourally, for {@code FleetConfig.Profile}'s own fields. + */ +class ConfigRefTopLevelCoverageTest { + + private static final RecordComponent[] COMPONENTS = FleetConfig.class.getRecordComponents(); + + /** + * Top-level components whose change {@code ConfigRef.changedDeferredKeys} reads and reports on + * — verified by reading that method as of fleetd #330, not derived from this test. + * {@code spawnReadyTimeoutMs}/{@code spawnReadyPollMs} are compared together and reported under + * one combined label ({@code "spawnReady*"}); {@code profiles} is compared twice over — once for + * added/removed profile names, once for an existing profile's launch settings — and that second + * comparison excludes {@code weight}/{@code maxLoad}/{@code credentialId} as hot sub-fields, + * which is what {@link ConfigRefProfileCoverageTest} exists to keep honest at the sub-field + * level. {@code profiles} itself still belongs here, not in the hot-exclusion set below: most of + * a profile's fields are NOT read live, so citing "read live off the config supplier" for the + * whole top-level key would be false. + */ + private static final Set DEFERRED_TOP_LEVEL_KEYS = Set.of( + "guard", "worktreeRoot", "worktreeGroup", "primary", "configReload", + "leadHeartbeat", "lifecycle", "spawnReadyTimeoutMs", "spawnReadyPollMs", + "quarantineCooldownSeconds", "profiles"); + + /** + * The escape hatch: top-level components with no reload bookkeeping at all, because every read + * of them goes live through {@link ConfigRef#get()} rather than off a startup snapshot. A + * component belongs here ONLY if that is true — never because adding it here makes this test + * pass. fleetd #323 is the cautionary tale for exactly this pattern: the identical hatch on + * {@code ConfigRef.LAUNCH_SETTINGS_EXCLUDED} let two profile fields be silently re-broken with + * the whole suite green, and it was only caught by mutating the checker itself (see this class's + * own mutation test below, and {@code ConfigRefProfileCoverageTest}'s equivalent). + * + *

+ */ + private static final Set HOT_EXCLUDED_TOP_LEVEL_KEYS = + Set.of("placement", "fleet", "memberCredentials", "memberLoginShell"); + + @Test + void everyTopLevelComponentIsAccountedForInExactlyOneClass() { + Set allNames = new TreeSet<>(); + for (RecordComponent rc : COMPONENTS) { + allNames.add(rc.getName()); + } + + Set cold = ConfigRef.COLD_KEYS; + Set split = ConfigRef.SPLIT_KEYS; + Set deferred = DEFERRED_TOP_LEVEL_KEYS; + Set hot = HOT_EXCLUDED_TOP_LEVEL_KEYS; + + // Typo guard on each set — the same check ConfigRefProfileCoverageTest runs on + // LAUNCH_SETTINGS_EXCLUDED. A name that does not exist on FleetConfig is a silent no-op. + assertTrue(allNames.containsAll(cold), + "ConfigRef.COLD_KEYS names a component that does not exist on FleetConfig: " + cold); + assertTrue(allNames.containsAll(split), + "ConfigRef.SPLIT_KEYS names a component that does not exist on FleetConfig: " + split); + assertTrue(allNames.containsAll(deferred), + "DEFERRED_TOP_LEVEL_KEYS names a component that does not exist on FleetConfig: " + deferred); + assertTrue(allNames.containsAll(hot), + "HOT_EXCLUDED_TOP_LEVEL_KEYS names a component that does not exist on FleetConfig: " + hot); + + // The escape hatch is pinned. Growing it requires editing this line — a visible, deliberate + // diff, not a quiet one. See the field javadoc above for what "belongs here" actually means. + assertEquals(Set.of("placement", "fleet", "memberCredentials", "memberLoginShell"), hot, + "HOT_EXCLUDED_TOP_LEVEL_KEYS changed. A component belongs here ONLY if it is read " + + "live off the config supplier, never because adding it makes this test " + + "pass. If you are adding one to silence this test, that is fleetd #323 " + + "happening again: account for it in ConfigRef.changedDeferredKeys (or " + + "COLD_KEYS/SPLIT_KEYS) instead. If it really is read live, name where and " + + "update this assertion and the field javadoc together."); + + // No component may sit in two buckets at once — the denominator check below could not catch + // that on its own (two buckets double-booking one key still sums to the right total if + // another key is simultaneously missing), so check every pair directly and name the culprit. + record Bucket(String name, Set keys) {} + List buckets = List.of( + new Bucket("COLD_KEYS", cold), new Bucket("SPLIT_KEYS", split), + new Bucket("DEFERRED_TOP_LEVEL_KEYS", deferred), new Bucket("HOT_EXCLUDED_TOP_LEVEL_KEYS", hot)); + for (int i = 0; i < buckets.size(); i++) { + for (int j = i + 1; j < buckets.size(); j++) { + Set overlap = new LinkedHashSet<>(buckets.get(i).keys()); + overlap.retainAll(buckets.get(j).keys()); + assertEquals(Set.of(), overlap, "a component is in both " + buckets.get(i).name() + + " and " + buckets.get(j).name() + ": " + overlap); + } + } + + Set union = new TreeSet<>(); + union.addAll(cold); + union.addAll(split); + union.addAll(deferred); + union.addAll(hot); + + System.out.printf( + "FleetConfig top-level coverage — %d components total: %d cold %s, %d deferred %s, " + + "%d split %s, %d hot-excluded %s%n", + allNames.size(), cold.size(), cold, deferred.size(), deferred, split.size(), split, + hot.size(), hot); + + Set missing = new TreeSet<>(allNames); + missing.removeAll(union); + assertEquals(Set.of(), missing, + "these FleetConfig components are in none of COLD_KEYS, DEFERRED_TOP_LEVEL_KEYS, " + + "SPLIT_KEYS or HOT_EXCLUDED_TOP_LEVEL_KEYS — triage each one into whichever " + + "actually describes it: " + missing); + assertEquals(allNames.size(), cold.size() + deferred.size() + split.size() + hot.size(), + "counts don't sum to the component total even though every component was found in " + + "the union — " + allNames.size() + " components, " + cold.size() + + " cold + " + deferred.size() + " deferred + " + split.size() + " split + " + + hot.size() + " hot-excluded"); + } +}