From ce74e164c613ee8de7ff0d87b25fc5fae3ab8318 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Thu, 10 Sep 2026 12:05:18 +0700 Subject: [PATCH 1/2] fleetd #424: make architect-slot identity checks read fleet.architects live MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MemberRegistry used to flatten fleet.architects into an unmodifiable map at construction, so removing (revoking) an architect slot from config never took effect: reserve()/requireSlotFor() kept granting spawns against the frozen snapshot forever, while ConfigRef told the operator "already applied" for the wrong consumer. - MemberRegistry gains a live constructor (MemberRegistry.live(Supplier)) that re-flattens fleet.architects/developers/reviewers on every slots()/slotsFor() call, so reserve() and requireSlotFor() (which both read through slotsFor) govern the NEXT spawn with no restart. The frozen single-arg constructor is kept for tests and fixed/code-built configs. - Binding rule: 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. To keep the bound terminal's IDENTITY too (CallerResolver.resolve reads roleForSlot/nameForSlot on every request), every successful bind now caches the slot's Entry into a new boundEntries map; roleForSlot/nameForSlot/profileForSlot/isSlot fall back to it when the slot is no longer live, and unbind clears it in the same critical section it clears the binding. - Fleetd.java now wires MemberRegistry.live(() -> config.get().fleet()) instead of the frozen constructor. - ConfigRef: corrected the fleet.leaders split-key message and the class doc's Hot bullet — architects is now hot for two independent consumers (CompositePeerLauncher for placement, MemberRegistry for identity), not only the one the message used to name. An architects-only edit still reports nothing beyond "config reloaded", which is now honest since the key really is fully hot for both consumers. Added MemberRegistryLiveTest: real ConfigRef.reload() against a @TempDir file, both directions (slot removed / slot added) for requireSlotFor and reserve tested separately, plus a bound-architect-survives-removal test that checks the binding AND the identity (roleForSlot/nameForSlot). Mutation-tested: reverting requireSlotFor to a frozen snapshot fails requireSlotForRefusesAProfileWhoseSlotWasRemovedByReload and its mirror; reverting reserve the same way fails the two reserve tests; dropping the boundEntries fallback fails the survives-removal test's roleForSlot assertion. All three restored before commit. mvn clean install: Tests run: 1510, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS. --- .../src/main/java/dev/ltms/fleet/Fleetd.java | 6 +- .../dev/ltms/fleet/auth/MemberRegistry.java | 153 ++++++++++++-- .../java/dev/ltms/fleet/config/ConfigRef.java | 46 +++-- .../fleet/auth/MemberRegistryLiveTest.java | 189 ++++++++++++++++++ 4 files changed, 357 insertions(+), 37 deletions(-) create mode 100644 fleetd/src/test/java/dev/ltms/fleet/auth/MemberRegistryLiveTest.java 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")); + } +} From 7f672f0fb846aa71cd95f3f19a85ddc6d64d2dc3 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Thu, 10 Sep 2026 12:29:35 +0700 Subject: [PATCH 2/2] fleetd #424: revoke the ARCHITECT privilege on reload, not just future spawns Correction to the #424 fix in PR #428: the ticket asked to revoke a removed architect slot, but the previous change (boundEntries) kept BOTH the binding and the ARCHITECT privilege alive for an already-bound session after its slot left config. That left the ticket's actual headline defect half-open. The corrected rule: config governs both what may be bound next AND what a bound slot still grants. Removing a slot now demotes its bound session to worker on the very next request (roleForSlot/nameForSlot read slots() with no cache, so CallerResolver.resolve falls through to Principal.worker(...)). The terminalToSlot binding itself is untouched by a reload, on purpose: dropping it would double-book the slot key and break unbind's compare-safe contract. - Delete boundEntries and entryFor; profileForSlot/roleForSlot/nameForSlot/ isSlot all read slots() directly, live, with no cache. - Rewrite the class doc's binding rule for the corrected semantic. - Replace the old "survives removal" test with anArchitectAlreadyBoundToASlotIsDemotedByReload, asserted through a real CallerResolver.resolve (not the roleForSlot seam), plus two tests for what must NOT change: the binding still occupies the slot after removal (a second terminal cannot claim it, even once the slot returns to config), and unbind still succeeds for the original terminal. Verified snapshot()/CallerResolver.members() need no change: snapshot() only ever reported raw terminalToSlot occupancy, and CallerResolver.members() has no production caller. --- .../dev/ltms/fleet/auth/MemberRegistry.java | 117 ++++++------------ .../fleet/auth/MemberRegistryLiveTest.java | 110 ++++++++++++---- 2 files changed, 126 insertions(+), 101 deletions(-) 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 26e5913..3007842 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/auth/MemberRegistry.java +++ b/fleetd/src/main/java/dev/ltms/fleet/auth/MemberRegistry.java @@ -36,17 +36,26 @@ import java.util.function.Supplier; * {@link Role#ARCHITECT}.

    • * * - *

    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. + *

    The binding rule (fleetd #424): config governs what a bound slot still grants, as + * well as what may be bound next. Removing a slot from config revokes it — that is the + * ticket's entire point ("Revoking an architect slot does not revoke it"). Revoking it means an + * architect already bound to that slot loses the ARCHITECT privilege on its very next request: + * {@link #roleForSlot} and {@link #nameForSlot} read {@link #slots()} directly, with no cache, so + * the moment a slot drops out of config, {@link CallerResolver#resolve} (which calls both on every + * request from a bound pane, {@code CallerResolver.java:220}) can no longer confirm the pane's slot + * is an architect slot, and the pane falls through to {@code Principal.worker(...)}. What does + * not change is the {@code terminalToSlot} occupancy — the binding created by + * {@link #bind} is untouched by a reload, on purpose: unbinding it here would double-book the slot + * key (a second terminal could then bind to the "freed" key while the first is still the terminal + * the operator actually meant to demote) and would silently break {@link #unbind}'s compare-safe + * contract, which needs the original {@code terminal → slot} pair intact to remove it cleanly. So + * the demoted session keeps occupying its slot — {@link #slotForTerminal} and {@link #snapshot()} + * still name it — it just no longer resolves as an architect through that occupancy, and a fresh + * spawn still cannot bind to the same key while it is occupied ({@link #reserve}/ + * {@link #requireSlotFor} refuse it anyway, since it is gone from {@link #slots()}). The demoted + * session's own turn is unaffected: {@code fleet_reply}'s authorization + * ({@code Authz.Action.REPLY}) is {@code caller.ownsSession(targetSession)} — identity by terminal, + * not by role — so a demoted architect can still end its own turn normally. * *

    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. @@ -79,16 +88,6 @@ public final class MemberRegistry implements MemberLifecycle { 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<>(); /** * Freeze the pool at construction — for tests, and for the rare case of wiring a fixed, @@ -177,26 +176,6 @@ 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. * @@ -204,36 +183,35 @@ public final class MemberRegistry implements MemberLifecycle { * declares none */ public String profileForSlot(String slotName) { - Entry e = entryFor(slotName); + Entry e = slots().get(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. */ + /** + * The role a qualified slot key belongs to, or {@code null} when the key is not currently + * configured. Deliberately live, with no cache (fleetd #424, see the class doc's binding rule): + * removing a slot from config must make {@link CallerResolver#resolve} stop granting the + * ARCHITECT role for it on the very next request from a terminal that was bound to it, which is + * the ticket's whole point — revoking a slot must actually revoke it, not just refuse the next + * spawn. + */ public MemberRole roleForSlot(String slotName) { - Entry e = entryFor(slotName); + Entry e = slots().get(slotName); return e == null ? null : e.role(); } - /** The unqualified configured name for a slot, or {@code null} if it is unknown. */ + /** + * The unqualified configured name for a slot, or {@code null} if it is not currently configured. + * Live for the same reason as {@link #roleForSlot} — see the class doc's binding rule. + */ public String nameForSlot(String slotName) { - Entry e = entryFor(slotName); + Entry e = slots().get(slotName); return e == null ? null : e.name(); } - /** - * 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. - */ + /** True when {@code slotName} is a configured architect slot. */ public boolean isSlot(String slotName) { - if (slots().containsKey(slotName)) { - return true; - } - synchronized (terminalToSlot) { - return boundEntries.containsKey(slotName); - } + return slots().containsKey(slotName); } /** @@ -264,15 +242,7 @@ 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; } } @@ -300,11 +270,6 @@ 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; } } @@ -412,15 +377,7 @@ 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/test/java/dev/ltms/fleet/auth/MemberRegistryLiveTest.java b/fleetd/src/test/java/dev/ltms/fleet/auth/MemberRegistryLiveTest.java index c5422f2..ea177e1 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/auth/MemberRegistryLiveTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/auth/MemberRegistryLiveTest.java @@ -2,6 +2,9 @@ package dev.ltms.fleet.auth; import dev.ltms.fleet.config.ConfigRef; import dev.ltms.fleet.config.FleetConfig; +import dev.ltms.fleet.herdr.FakeHerdr; +import dev.ltms.fleet.herdr.PaneLocator; +import dev.ltms.fleet.mcp.ConnectionIdentity; import dev.ltms.fleet.peer.MemberRole; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; @@ -14,9 +17,11 @@ 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." + * no restart. A session already bound to a slot keeps its binding (the {@code + * terminalToSlot} occupancy) even after that slot drops out of config, but NOT the ARCHITECT + * privilege the slot used to grant — that is revoked on the bound session's very next + * request. See {@link MemberRegistry}'s class doc for the exact rule: "config governs what a + * bound slot still grants, as well as what may be bound next." * *

    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 @@ -150,40 +155,103 @@ class MemberRegistryLiveTest { "a slot added by reload must be reservable with no restart"); } - // ── a bound architect survives its slot's removal by reload (criterion 3) ───────────────── + // ── a bound architect is demoted, but the binding itself is not touched (fleetd #424) ─────── + // The lead's corrected ruling: the PRIVILEGE a slot grants is revoked on the bound session's + // very next request, but the terminalToSlot BINDING itself is untouched by a reload — dropping + // it would double-book the slot key and break unbind's compare-safe contract. See the class + // doc's binding rule. + + /** A caller identity resolving the one canned pane (terminal {@code term_a}) in {@link FakeHerdr}. */ + private static ConnectionIdentity boundPaneIdentity() { + return new ConnectionIdentity(new PaneLocator(new FakeHerdr()), _ -> FakeHerdr.WORKER_PID); + } @Test - void anArchitectAlreadyBoundToASlotSurvivesTheSlotsRemovalByReload(@TempDir Path dir) throws Exception { + void anArchitectAlreadyBoundToASlotIsDemotedByReload(@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")); + assertTrue(registry.bind(reservation, "term_a")); + assertEquals("architect:designer", registry.slotForTerminal("term_a")); + + // Drive the real caller path, not the roleForSlot seam directly: CallerResolver.resolve is + // what a live request actually goes through (CallerResolver.java:220), and a resolver that + // ignored roleForSlot entirely would still pass a test that only checked the seam. + CallerResolver resolver = CallerResolver.withLeadsAndMembers( + boundPaneIdentity(), false, null, Map::of, registry); + + Principal before = resolver.resolve("127.0.0.1", 42, null); + assertEquals(Role.ARCHITECT, before.role(), "sanity check: the harness binds term_a as an architect"); + assertEquals("designer", before.name()); 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"), + Principal after = resolver.resolve("127.0.0.1", 42, null); + assertEquals(Role.WORKER, after.role(), + "removing the slot from config must demote the bound session to worker on its " + + "NEXT request — this is the ticket's whole point"); + assertEquals("term_a", after.terminal(), "same pane, same terminal — only the role changed"); + } + + @Test + void theOriginalBindingStillOccupiesTheRemovedSlotSoASecondTerminalCannotClaimIt(@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_a")); + + Files.writeString(f, yaml(WITHOUT_ARCHITECT_SLOTS)); + ConfigRef.Outcome removed = ref.reload(); + assertTrue(removed.applied(), "the reload must actually take effect: " + removed.summary()); + + // The binding survives the removal untouched. + assertEquals("architect:designer", registry.slotForTerminal("term_a"), "a live binding must never be retroactively unbound by a config edit"); - assertEquals(Map.of("term_designer", "architect:designer"), registry.snapshot()); + assertEquals(Map.of("term_a", "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")); + // Bring the slot back into config. If the binding had been silently dropped by the removal + // (rather than merely losing the privilege it grants), a second terminal could now claim + // the "freed" key — the exact double-booking the class doc's binding rule rules out. + Files.writeString(f, yaml(WITH_SONNET_SLOT)); + ConfigRef.Outcome restored = ref.reload(); + assertTrue(restored.applied(), "the reload must actually take effect: " + restored.summary()); - // But the removed slot must grant nothing NEW to a different spawn. + assertFalse(registry.bind("architect:designer", "term_b"), + "the slot is still occupied by term_a — a second terminal must not bind to it"); assertThrows(IllegalArgumentException.class, - () -> registry.requireSlotFor(MemberRole.ARCHITECT, "sonnet")); - assertThrows(IllegalArgumentException.class, - () -> registry.reserve(MemberRole.ARCHITECT, "sonnet")); + () -> registry.reserve(MemberRole.ARCHITECT, "sonnet"), + "the slot is still occupied by term_a — a fresh reservation must not find it free"); + assertEquals("architect:designer", registry.slotForTerminal("term_a"), + "the original binding is unchanged throughout"); + } + + @Test + void unbindStillSucceedsForTheOriginalTerminalAfterItsSlotIsRemoved(@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_a")); + + Files.writeString(f, yaml(WITHOUT_ARCHITECT_SLOTS)); + ConfigRef.Outcome out = ref.reload(); + assertTrue(out.applied(), "the reload must actually take effect: " + out.summary()); + + assertTrue(registry.unbind("architect:designer", "term_a"), + "unbind must still work for a slot that config has since removed, or a session " + + "that outlives its slot's removal could never release it"); + assertNull(registry.slotForTerminal("term_a")); + assertEquals(Map.of(), registry.snapshot()); } }