diff --git a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java index eddc419..9163ab3 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java +++ b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java @@ -352,7 +352,11 @@ public final class Fleetd { // pane resolves to an architect until the later spawn lifecycle binds one. The registry is // what CallerResolver resolves against and what that lifecycle will read profiles from; // nothing here spawns a slot. - MemberRegistry members = new MemberRegistry(cfg.fleet()); + // fleetd #424: MemberRegistry.live re-reads fleet.architects through `config` on every + // reserve/requireSlotFor call, so a reload that removes or adds an architect slot governs + // the next spawn with no restart — the frozen `new MemberRegistry(cfg.fleet())` this used + // to be let a "revoked" slot keep granting new architect spawns forever. + MemberRegistry members = MemberRegistry.live(() -> config.get().fleet()); sessions.setMemberLifecycle(members); if (!members.slots().isEmpty()) { log.info("member slots: {} configured {} — none bound yet (a slot is idle until the " diff --git a/fleetd/src/main/java/dev/ltms/fleet/auth/MemberRegistry.java b/fleetd/src/main/java/dev/ltms/fleet/auth/MemberRegistry.java index 972cf9c..26e5913 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/auth/MemberRegistry.java +++ b/fleetd/src/main/java/dev/ltms/fleet/auth/MemberRegistry.java @@ -11,6 +11,7 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.function.Supplier; /** * The architect-slot registry (CB-548): every gateway-local architect name and the strong-model @@ -19,16 +20,34 @@ import java.util.Objects; * *

Two halves, split by who owns each: *

* + *

The binding rule (fleetd #424): config governs what may be bound next; it never + * retroactively unbinds a live session. A slot removed from config while a terminal is + * bound to it keeps that binding — the architect keeps working and keeps its identity — but no new + * spawn can bind to that slot again, because {@link #slotsFor} (which {@link #reserve} and + * {@link #requireSlotFor} both read) stops offering it the moment it drops out of {@link #slots()}. + * To make an already-bound slot survive its own removal from {@link #slots()}, every successful + * {@link #bind} also caches the slot's {@link Entry} into {@link #boundEntries}; {@link #roleForSlot} + * and {@link #nameForSlot} — which {@link CallerResolver#resolve} calls on every request from a + * bound terminal — fall back to that cache when the slot is no longer live. {@link #unbind} clears + * the cache entry at the same time it clears the binding, so a slot that is genuinely free again + * (no live terminal) is never treated as "known" once it also drops out of config. + * *

Spawning/lifecycle is deliberately a separate unit: this class only owns the bindings and * exposes the map the resolver resolves against plus the profile lookup lifecycle will call. * Nothing here creates or manages an architect session. @@ -55,14 +74,52 @@ public final class MemberRegistry implements MemberLifecycle { } } - private final Map slots; + private final Supplier fleet; /** Live {@code terminal_id → qualified slot key}; guarded by {@code terminalToSlot}. */ private final Map terminalToSlot = new HashMap<>(); /** Slot keys held between reservation and the terminal binding. Guarded by terminalToSlot. */ private final java.util.Set reservedSlots = new java.util.HashSet<>(); + /** + * The {@link Entry} a slot resolved to at the moment it was last successfully bound, keyed by + * qualified slot key; guarded by {@code terminalToSlot}. Exists solely so a slot's role/name + * survive its own removal from {@link #slots()} while a terminal is still bound to it (fleetd + * #424) — see the class doc's binding rule. An entry here always mirrors a live + * {@code terminalToSlot} value: {@link #bind} adds it in the same critical section it adds the + * binding, and {@link #unbind} removes it in the same critical section it removes the binding, + * so a slot with no live terminal is never "remembered" here. + */ + private final Map boundEntries = new HashMap<>(); - /** Flatten every role pool in {@code fleet} into one registry. Leaders are not members. */ + /** + * Freeze the pool at construction — for tests, and for the rare case of wiring a fixed, + * code-built config. Production wiring should prefer {@link #live}, which re-reads + * {@code fleet:} on every call. + */ public MemberRegistry(FleetConfig.Fleet fleet) { + this(() -> fleet); + } + + private MemberRegistry(Supplier fleet) { + this.fleet = fleet; + } + + /** + * Live variant (fleetd #424): {@code fleet} is read fresh on every {@link #slots()} call — pass + * {@code () -> config.get().fleet()}, the same supplier shape {@code CompositePeerLauncher} + * already uses for placement — so a reload that adds or removes an architect slot governs the + * next spawn's {@link #reserve}/{@link #requireSlotFor} check with no restart. A separate, + * private constructor rather than a same-arity public overload of + * {@link #MemberRegistry(FleetConfig.Fleet)}: a {@code FleetConfig.Fleet} and a + * {@code Supplier} overload are ambiguous for a literal {@code null} — the + * same reason {@code CallerResolver.withLeads} is a static factory rather than a fourth + * constructor overload. + */ + public static MemberRegistry live(Supplier fleet) { + return new MemberRegistry(Objects.requireNonNull(fleet, "fleet")); + } + + /** Flatten every role pool in {@code fleet} into one map. Leaders are not members. */ + private static Map flatten(FleetConfig.Fleet fleet) { Map flat = new LinkedHashMap<>(); if (fleet != null) { for (MemberRole role : MemberRole.values()) { @@ -74,18 +131,22 @@ public final class MemberRegistry implements MemberLifecycle { }); } } - this.slots = Collections.unmodifiableMap(flat); + return Collections.unmodifiableMap(flat); } - /** The configured slots, keyed by qualified {@link Entry#key()}. Unmodifiable snapshot. */ + /** + * The configured slots, keyed by qualified {@link Entry#key()}. Unmodifiable snapshot of + * {@code fleet:} as of this call — see the class doc for which constructor makes that + * live versus frozen. + */ public Map slots() { - return slots; + return flatten(fleet.get()); } - /** The slots belonging to {@code role}, in definition order. */ + /** The slots belonging to {@code role}, in definition order, as of this call. */ public Map slotsFor(MemberRole role) { Map out = new LinkedHashMap<>(); - slots.forEach((key, e) -> { + slots().forEach((key, e) -> { if (e.role() == role) { out.put(key, e); } @@ -116,6 +177,26 @@ public final class MemberRegistry implements MemberLifecycle { } } + /** + * The {@link Entry} a slot key resolves to right now: live config first, falling back to the + * entry {@linkplain #boundEntries cached} when this slot was last bound — so a bound slot's + * identity survives its own removal from {@link #slots()} (fleetd #424, see the class doc's + * binding rule). {@code null} for a key that is neither configured nor currently bound to + * anything. + */ + private Entry entryFor(String slotName) { + if (slotName == null) { + return null; + } + Entry live = slots().get(slotName); + if (live != null) { + return live; + } + synchronized (terminalToSlot) { + return boundEntries.get(slotName); + } + } + /** * The strong-model profile a slot runs under — what the spawn lifecycle reads. * @@ -123,25 +204,36 @@ public final class MemberRegistry implements MemberLifecycle { * declares none */ public String profileForSlot(String slotName) { - Entry e = slots.get(slotName); + Entry e = entryFor(slotName); return (e == null || e.profile() == null) ? null : e.profile(); } /** The role a qualified slot key belongs to, or {@code null} when the key is unknown. */ public MemberRole roleForSlot(String slotName) { - Entry e = slots.get(slotName); + Entry e = entryFor(slotName); return e == null ? null : e.role(); } /** The unqualified configured name for a slot, or {@code null} if it is unknown. */ public String nameForSlot(String slotName) { - Entry e = slots.get(slotName); + Entry e = entryFor(slotName); return e == null ? null : e.name(); } - /** True when {@code slotName} is a configured architect slot. */ + /** + * True when {@code slotName} is a configured architect slot, or is currently bound to + * a live terminal — see the class doc's binding rule (fleetd #424). The latter case only ever + * covers a slot with an existing binding, so it never lets a fresh terminal claim a slot that + * dropped out of config: the cardinality checks in {@link #bind} still refuse a slot that is + * already occupied. + */ public boolean isSlot(String slotName) { - return slots.containsKey(slotName); + if (slots().containsKey(slotName)) { + return true; + } + synchronized (terminalToSlot) { + return boundEntries.containsKey(slotName); + } } /** @@ -172,7 +264,15 @@ public final class MemberRegistry implements MemberLifecycle { if (terminalToSlot.containsValue(slot) || reservedSlots.contains(slot)) { return false; // slot already hosts a terminal — no second one } + // isSlot(slot) passing plus the containsValue check just above both being false proves + // this slot is live right now (fleetd #424): boundEntries always mirrors terminalToSlot's + // values (see the field doc), so a slot with no live terminal cannot be sitting in + // boundEntries either — isSlot's true answer here can only have come from slots(). + Entry entry = slots().get(slot); terminalToSlot.put(terminal, slot); + if (entry != null) { + boundEntries.put(slot, entry); + } return true; } } @@ -200,6 +300,11 @@ public final class MemberRegistry implements MemberLifecycle { return false; // absent, or a replacement/moved binding — leave it in place } terminalToSlot.remove(expectedTerminal); + // fleetd #424: clear the cached identity together with the binding, so a slot that is + // genuinely free (no live terminal) and also gone from config stops being "known" — + // otherwise a later, unrelated slot reusing the same qualified key could inherit a stale + // cached role/name it never configured. + boundEntries.remove(slot); return true; } } @@ -307,7 +412,15 @@ public final class MemberRegistry implements MemberLifecycle { || terminalToSlot.containsValue(reservation.slot())) { return false; } + // Same reasoning as the two-arg bind above: reaching here with isSlot(...) true and + // containsValue(...) false proves the slot is live right now, since a reservation was + // never bound before now (reservedSlots and boundEntries are disjoint by construction) — + // fleetd #424. + Entry entry = slots().get(reservation.slot()); terminalToSlot.put(terminal, reservation.slot()); + if (entry != null) { + boundEntries.put(reservation.slot(), entry); + } return true; } } 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 5ee4dcd..40a3fc9 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/config/ConfigRef.java +++ b/fleetd/src/main/java/dev/ltms/fleet/config/ConfigRef.java @@ -32,9 +32,15 @@ import java.util.function.Supplier; * not the fact that they are config. Most of {@code fleet:} — every role pool * ({@code architects}/{@code developers}/{@code reviewers}), {@code charters}, and * {@code tabLabel} — is read the same live way, through the same supplier - * ({@code () -> config.get().fleet()}). But {@code fleet:} as a whole is NOT in this - * class: {@code fleet.leaders} inside the same key is frozen, which is exactly what - * makes {@code fleet:} split rather than hot — see below. + * ({@code () -> config.get().fleet()}). {@code architects} in particular is hot for + * two independent consumers (fleetd #424): {@code CompositePeerLauncher} + * reads it live for placement (which profile an unqualified architect spawn may land on), and + * {@code MemberRegistry} separately reads it live, through its own instance of the same + * supplier shape, for identity (which slot a spawn may bind to) — see {@code MemberRegistry}'s + * class doc for the binding rule that keeps a live session's identity even after its slot is + * removed from config. But {@code fleet:} as a whole is NOT in this class: + * {@code fleet.leaders} inside the same key is frozen, which is exactly what makes + * {@code fleet:} split rather than hot — see below. *

  • Deferred — accepted into the new snapshot, but the wiring built at startup * keeps the old value until a restart: {@code lifecycle:}, {@code leadHeartbeat:}, * {@code idleSleepGuard:} ({@code Fleetd.java} reads it once, at startup, to decide whether @@ -529,26 +535,34 @@ public final class ConfigRef implements Supplier { + "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"); } - // fleetd #333: unlike health/coordinator above, most of `fleet:` (architects, developers, - // reviewers, charters, tabLabel) is genuinely hot — ConfigRefTest.aHotChangeIsAppliedAndRead- + // fleetd #333: unlike health/coordinator above, most of `fleet:` (developers, reviewers, + // charters, tabLabel) is genuinely hot — ConfigRefTest.aHotChangeIsAppliedAndRead- // ThroughGet and aCharterChangeIsHotAndReachesTheLiveConfig prove it reaches the live config - // with no restart note. Only fleet.leaders is frozen (Fleetd.java:281 reads - // cfg.fleet().leaders() off the startup snapshot to build both the LeadTabScanner's - // tab-label-to-name map, wired into CallerResolver.withLeadsAndMembers at Fleetd.java:620/624, - // and — when herdr answered — LeadLauncher(...).ensureLeads() at Fleetd.java:315, which - // auto-launches each lead up to its `instances` count; neither is rebuilt on reload). So this - // compares fleet.leaders alone, not the whole Fleet record: comparing the whole record would - // report "split" for a tabLabel-only or charters-only change that is actually fully hot, - // which is the over-claim mirror of the under-claim bug this class exists to prevent. + // with no restart note. `architects` is hot too, and — since fleetd #424 — hot for BOTH of + // its consumers, not just the one this comment used to name: CompositePeerLauncher reads it + // live for PLACEMENT through the () -> config.get().fleet() supplier named in the class doc's + // Hot bullet, and MemberRegistry separately reads it live for IDENTITY (which slot a spawn + // may bind to) through its own instance of that same supplier shape — see MemberRegistry.live + // and its class doc for the binding rule (a session already bound to a slot keeps that + // binding even after the slot is removed from config; only a NEW bind is refused). Only + // fleet.leaders is frozen (Fleetd.java:281 reads cfg.fleet().leaders() off the startup + // snapshot to build both the LeadTabScanner's tab-label-to-name map, wired into + // CallerResolver.withLeadsAndMembers at Fleetd.java:620/624, and — when herdr answered — + // LeadLauncher(...).ensureLeads() at Fleetd.java:315, which auto-launches each lead up to its + // `instances` count; neither is rebuilt on reload). So this compares fleet.leaders alone, not + // the whole Fleet record: comparing the whole record would report "split" for a tabLabel-only + // or architects-only change that is actually fully hot, which is the over-claim mirror of the + // under-claim bug this class exists to prevent. if (!Objects.equals(leadersOf(old), leadersOf(fresh))) { changed.add("fleet: fleet.leaders (each lead's tab, workspace, cwd, profile and " + "instances count) is read once at startup to build the LeadTabScanner's " + "identity map and to auto-launch leads, and neither is rebuilt on reload, so a " + "lead added, removed, or given a new tab: label needs a restart — until then it " + "stays unrecognised, and a caller from its new tab resolves as a worker, not a " - + "lead; the rest of fleet: (architects, developers, reviewers, charters, " - + "tabLabel) is read live through the supplier on CompositePeerLauncher and " - + "already applied"); + + "lead; the rest of fleet: (developers, reviewers, charters, tabLabel) is read " + + "live through the supplier on CompositePeerLauncher, and architects is read " + + "live through that same supplier for placement AND through a separate supplier " + + "on MemberRegistry for spawn-time identity — both 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 split keys the class doc documents. diff --git a/fleetd/src/test/java/dev/ltms/fleet/auth/MemberRegistryLiveTest.java b/fleetd/src/test/java/dev/ltms/fleet/auth/MemberRegistryLiveTest.java new file mode 100644 index 0000000..c5422f2 --- /dev/null +++ b/fleetd/src/test/java/dev/ltms/fleet/auth/MemberRegistryLiveTest.java @@ -0,0 +1,189 @@ +package dev.ltms.fleet.auth; + +import dev.ltms.fleet.config.ConfigRef; +import dev.ltms.fleet.config.FleetConfig; +import dev.ltms.fleet.peer.MemberRole; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.*; + +/** + * fleetd #424 — revoking (or granting) an architect slot must take effect on the next spawn with + * no restart, while a session already bound to a slot keeps its binding and its identity even + * after that slot drops out of config. See {@link MemberRegistry}'s class doc for the exact rule: + * "config governs what may be bound next; it never retroactively unbinds a live session." + * + *

    Every test here drives a REAL {@link ConfigRef#reload()} against a {@code @TempDir} file and + * asserts {@link ConfigRef.Outcome#applied()}, rather than comparing two frozen + * {@code MemberRegistry} instances in memory — the defect this ticket fixes is specifically that + * {@link MemberRegistry} used to ignore a live reload, so a test that never reloads cannot tell the + * fixed registry from the broken one. {@code requireSlotFor} and {@code reserve} are pinned in + * separate tests, in both directions (removed and added), so a registry that simply refuses (or + * simply allows) everything cannot pass by accident — see {@link MemberRegistryTest} for the + * registry's other invariants (bind/unbind cardinality, thread-safety), which are unaffected by + * this ticket and still exercised against the frozen constructor. + */ +class MemberRegistryLiveTest { + + private static String yaml(String fleetBlock) { + return """ + bind: + host: 127.0.0.1 + port: 8765 + herdrSocket: ~/.config/herdr/herdr.sock + profiles: + sonnet: + baseUrl: http://gx00.gw:8000 + model: sonnet + opus: + baseUrl: http://gx00.gw:8001 + model: opus + guard: + offSubscriptionHosts: + - gx00.gw + """ + fleetBlock; + } + + private static final String WITH_SONNET_SLOT = """ + fleet: + architects: + designer: + profile: sonnet + """; + + /** No architect pool at all — developers is unrelated dead data for this registry (#424 out of scope). */ + private static final String WITHOUT_ARCHITECT_SLOTS = """ + fleet: + developers: + dev1: + profile: sonnet + """; + + private static ConfigRef refFor(Path f) { + return new ConfigRef(f, FleetConfig.load(f)); + } + + // ── requireSlotFor is live (criteria 1, 2, 4) ────────────────────────────────────────────── + + @Test + void requireSlotForRefusesAProfileWhoseSlotWasRemovedByReload(@TempDir Path dir) throws Exception { + Path f = dir.resolve("fleetd.yaml"); + Files.writeString(f, yaml(WITH_SONNET_SLOT)); + ConfigRef ref = refFor(f); + MemberRegistry registry = MemberRegistry.live(() -> ref.get().fleet()); + + assertDoesNotThrow(() -> registry.requireSlotFor(MemberRole.ARCHITECT, "sonnet"), + "the slot is configured before the reload"); + + Files.writeString(f, yaml(WITHOUT_ARCHITECT_SLOTS)); + ConfigRef.Outcome out = ref.reload(); + assertTrue(out.applied(), "the reload must actually take effect: " + out.summary()); + + assertThrows(IllegalArgumentException.class, + () -> registry.requireSlotFor(MemberRole.ARCHITECT, "sonnet"), + "revoking the slot must refuse the NEXT spawn that names it"); + } + + @Test + void requireSlotForAllowsAProfileWhoseSlotWasAddedByReload(@TempDir Path dir) throws Exception { + Path f = dir.resolve("fleetd.yaml"); + Files.writeString(f, yaml(WITHOUT_ARCHITECT_SLOTS)); + ConfigRef ref = refFor(f); + MemberRegistry registry = MemberRegistry.live(() -> ref.get().fleet()); + + assertThrows(IllegalArgumentException.class, + () -> registry.requireSlotFor(MemberRole.ARCHITECT, "sonnet"), + "no architect slot is configured yet"); + + Files.writeString(f, yaml(WITH_SONNET_SLOT)); + ConfigRef.Outcome out = ref.reload(); + assertTrue(out.applied(), "the reload must actually take effect: " + out.summary()); + + assertDoesNotThrow(() -> registry.requireSlotFor(MemberRole.ARCHITECT, "sonnet"), + "a slot added by reload must be usable with no restart"); + } + + // ── reserve is live too — tested separately from requireSlotFor (criteria 1, 2, 4) ──────── + + @Test + void reserveRefusesAProfileWhoseSlotWasRemovedByReload(@TempDir Path dir) throws Exception { + Path f = dir.resolve("fleetd.yaml"); + Files.writeString(f, yaml(WITH_SONNET_SLOT)); + ConfigRef ref = refFor(f); + MemberRegistry registry = MemberRegistry.live(() -> ref.get().fleet()); + + MemberLifecycle.SlotReservation before = registry.reserve(MemberRole.ARCHITECT, "sonnet"); + assertEquals("architect:designer", before.slot()); + registry.release(before); // free it back up so the reload-side reserve below starts clean + + Files.writeString(f, yaml(WITHOUT_ARCHITECT_SLOTS)); + ConfigRef.Outcome out = ref.reload(); + assertTrue(out.applied(), "the reload must actually take effect: " + out.summary()); + + assertThrows(IllegalArgumentException.class, + () -> registry.reserve(MemberRole.ARCHITECT, "sonnet"), + "revoking the slot must refuse the NEXT reservation for it"); + } + + @Test + void reserveAllowsAProfileWhoseSlotWasAddedByReload(@TempDir Path dir) throws Exception { + Path f = dir.resolve("fleetd.yaml"); + Files.writeString(f, yaml(WITHOUT_ARCHITECT_SLOTS)); + ConfigRef ref = refFor(f); + MemberRegistry registry = MemberRegistry.live(() -> ref.get().fleet()); + + assertThrows(IllegalArgumentException.class, + () -> registry.reserve(MemberRole.ARCHITECT, "sonnet"), + "no architect slot is configured yet"); + + Files.writeString(f, yaml(WITH_SONNET_SLOT)); + ConfigRef.Outcome out = ref.reload(); + assertTrue(out.applied(), "the reload must actually take effect: " + out.summary()); + + MemberLifecycle.SlotReservation after = registry.reserve(MemberRole.ARCHITECT, "sonnet"); + assertEquals("architect:designer", after.slot(), + "a slot added by reload must be reservable with no restart"); + } + + // ── a bound architect survives its slot's removal by reload (criterion 3) ───────────────── + + @Test + void anArchitectAlreadyBoundToASlotSurvivesTheSlotsRemovalByReload(@TempDir Path dir) throws Exception { + Path f = dir.resolve("fleetd.yaml"); + Files.writeString(f, yaml(WITH_SONNET_SLOT)); + ConfigRef ref = refFor(f); + MemberRegistry registry = MemberRegistry.live(() -> ref.get().fleet()); + + MemberLifecycle.SlotReservation reservation = registry.reserve(MemberRole.ARCHITECT, "sonnet"); + assertTrue(registry.bind(reservation, "term_designer")); + assertEquals("architect:designer", registry.slotForTerminal("term_designer")); + + Files.writeString(f, yaml(WITHOUT_ARCHITECT_SLOTS)); + ConfigRef.Outcome out = ref.reload(); + assertTrue(out.applied(), "the reload must actually take effect: " + out.summary()); + + // The binding itself must survive untouched — nothing here may unbind a live session. + assertEquals("architect:designer", registry.slotForTerminal("term_designer"), + "a live binding must never be retroactively unbound by a config edit"); + assertEquals(Map.of("term_designer", "architect:designer"), registry.snapshot()); + + // The identity CallerResolver.resolve actually reads off a bound pane must survive too — + // roleForSlot/nameForSlot going null here is what would silently demote a live architect to + // a worker the moment its slot is edited out of config. + assertEquals(MemberRole.ARCHITECT, registry.roleForSlot("architect:designer"), + "CallerResolver reads roleForSlot to confirm a bound pane is still an architect " + + "slot — this must not go null just because config removed the slot"); + assertEquals("designer", registry.nameForSlot("architect:designer")); + + // But the removed slot must grant nothing NEW to a different spawn. + assertThrows(IllegalArgumentException.class, + () -> registry.requireSlotFor(MemberRole.ARCHITECT, "sonnet")); + assertThrows(IllegalArgumentException.class, + () -> registry.reserve(MemberRole.ARCHITECT, "sonnet")); + } +}