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.

fleetd/src/main/java/dev/ltms/fleet/auth/MemberRegistry.java

@@ -36,17 +36,26 @@ import java.util.function.Supplier;
  *       {@link Role#ARCHITECT}.</li>
  * </ul>
  *
- * <p><strong>The binding rule (fleetd #424): config governs what may be bound next; it never
+ * <p><strong>The binding rule (fleetd #424): config governs what a bound slot still grants, as
- * retroactively unbinds a live session.</strong> A slot removed from config while a terminal is
+ * well as what may be bound next.</strong> Removing a slot from config revokes it — that is the
- * bound to it keeps that binding — the architect keeps working and keeps its identity — but no new
+ * ticket's entire point ("Revoking an architect slot does not revoke it"). Revoking it means an
- * spawn can bind to that slot again, because {@link #slotsFor} (which {@link #reserve} and
+ * architect already bound to that slot loses the ARCHITECT privilege on its very next request:
- * {@link #requireSlotFor} both read) stops offering it the moment it drops out of {@link #slots()}.
+ * {@link #roleForSlot} and {@link #nameForSlot} read {@link #slots()} directly, with no cache, so
- * To make an already-bound slot survive its own removal from {@link #slots()}, every successful
+ * the moment a slot drops out of config, {@link CallerResolver#resolve} (which calls both on every
- * {@link #bind} also caches the slot's {@link Entry} into {@link #boundEntries}; {@link #roleForSlot}
+ * request from a bound pane, {@code CallerResolver.java:220}) can no longer confirm the pane's slot
- * and {@link #nameForSlot} — which {@link CallerResolver#resolve} calls on every request from a
+ * is an architect slot, and the pane falls through to {@code Principal.worker(...)}. What does
- * bound terminal — fall back to that cache when the slot is no longer live. {@link #unbind} clears
+ * <em>not</em> change is the {@code terminalToSlot} <em>occupancy</em> — the binding created by
- * the cache entry at the same time it clears the binding, so a slot that is genuinely free again
+ * {@link #bind} is untouched by a reload, on purpose: unbinding it here would double-book the slot
- * (no live terminal) is never treated as "known" once it also drops out of config.
+ * 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.
  *
  * <p>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<String, String> terminalToSlot = new HashMap<>();
     /** Slot keys held between reservation and the terminal binding. Guarded by terminalToSlot. */
     private final java.util.Set<String> 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<String, Entry> 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. */
-     * True when {@code slotName} is a configured architect slot, <em>or</em> 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) {
-        if (slots().containsKey(slotName)) {
+        return slots().containsKey(slotName);
-            return true;
-        }
-        synchronized (terminalToSlot) {
-            return boundEntries.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;
         }
     }

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
+ * no restart. A session already bound to a slot keeps its <em>binding</em> (the {@code
- * after that slot drops out of config. See {@link MemberRegistry}'s class doc for the exact rule:
+ * terminalToSlot} occupancy) even after that slot drops out of config, but NOT the ARCHITECT
- * "config governs what may be bound next; it never retroactively unbinds a live session."
+ * <em>privilege</em> 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."
  *
  * <p>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"));
+        assertTrue(registry.bind(reservation, "term_a"));
-        assertEquals("architect:designer", registry.slotForTerminal("term_designer"));
+        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.
+        Principal after = resolver.resolve("127.0.0.1", 42, null);
-        assertEquals("architect:designer", registry.slotForTerminal("term_designer"),
+        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 —
+        // Bring the slot back into config. If the binding had been silently dropped by the removal
-        // roleForSlot/nameForSlot going null here is what would silently demote a live architect to
+        // (rather than merely losing the privilege it grants), a second terminal could now claim
-        // a worker the moment its slot is edited out of config.
+        // the "freed" key — the exact double-booking the class doc's binding rule rules out.
-        assertEquals(MemberRole.ARCHITECT, registry.roleForSlot("architect:designer"),
+        Files.writeString(f, yaml(WITH_SONNET_SLOT));
-                "CallerResolver reads roleForSlot to confirm a bound pane is still an architect "
+        ConfigRef.Outcome restored = ref.reload();
-                        + "slot — this must not go null just because config removed the slot");
+        assertTrue(restored.applied(), "the reload must actually take effect: " + restored.summary());
-        assertEquals("designer", registry.nameForSlot("architect:designer"));
 
-        // 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"));
+                () -> registry.reserve(MemberRole.ARCHITECT, "sonnet"),
-        assertThrows(IllegalArgumentException.class,
+                "the slot is still occupied by term_a — a fresh reservation must not find it free");
-                () -> registry.reserve(MemberRole.ARCHITECT, "sonnet"));
+        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());
     }
 }