Merge #428: revoke the ARCHITECT privilege on reload, not just future spawns
fleetd #424. MemberRegistry.slots() now re-reads fleet: through a supplier,
so removing an architect slot demotes the bound pane on its very next request.
The boundEntries cache and entryFor() fallback from the first round are gone:
the slot OCCUPANCY (terminalToSlot) survives a reload, the ARCHITECT role does
not. That split was my own ticket wording's fault -- I asked for a test that a
bound architect "survives the rebuild", which conflated the binding with the
privilege.
Conflict resolved by hand in ConfigRef.java: #422 (models:) and #424
(architects) both rewrote the same Hot bullet. Kept both.
Also corrected two claims #424's own second commit left stale -- 7f672f0
reversed the behaviour but never touched ConfigRef, whose whole job is to tell
the operator what a reload does:
- the Hot bullet said MemberRegistry's rule "keeps a live session's identity
even after its slot is removed from config"
- the reload-report comment said "only a NEW bind is refused"
Both now say what the code does: removal revokes ARCHITECT on the next
request, and only the slot occupancy survives.
Verified by the lead: 1535 tests, 0 failures, 0 compile errors, BUILD SUCCESS
on the merged tree.
Mutation of three halves the worker's own proof did not cover -- profileForSlot,
nameForSlot and isSlot each pointed at a frozen snapshot taken at construction
(live readers 5 -> 4, each mutation naming its method and line). All three
PASSED at 1535. The ticket's own fix is well pinned; these three sibling live
reads are not. Follow-up filed.
This commit is contained in:
@@ -352,7 +352,11 @@ public final class Fleetd {
|
|||||||
// pane resolves to an architect until the later spawn lifecycle binds one. The registry is
|
// 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;
|
// what CallerResolver resolves against and what that lifecycle will read profiles from;
|
||||||
// nothing here spawns a slot.
|
// 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);
|
sessions.setMemberLifecycle(members);
|
||||||
if (!members.slots().isEmpty()) {
|
if (!members.slots().isEmpty()) {
|
||||||
log.info("member slots: {} configured {} — none bound yet (a slot is idle until the "
|
log.info("member slots: {} configured {} — none bound yet (a slot is idle until the "
|
||||||
|
|||||||
@@ -11,6 +11,7 @@ import java.util.LinkedHashMap;
|
|||||||
import java.util.List;
|
import java.util.List;
|
||||||
import java.util.Map;
|
import java.util.Map;
|
||||||
import java.util.Objects;
|
import java.util.Objects;
|
||||||
|
import java.util.function.Supplier;
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* The architect-slot registry (CB-548): every gateway-local architect name and the strong-model
|
* The architect-slot registry (CB-548): every gateway-local architect name and the strong-model
|
||||||
@@ -19,16 +20,43 @@ import java.util.Objects;
|
|||||||
*
|
*
|
||||||
* <p>Two halves, split by who owns each:
|
* <p>Two halves, split by who owns each:
|
||||||
* <ul>
|
* <ul>
|
||||||
* <li><b>slots</b> — configured once, keyed by the gateway-local unique name; each carries the
|
* <li><b>slots</b> — read from {@code fleet.architects}/{@code developers}/{@code reviewers}
|
||||||
* {@code profile} reference the spawn lifecycle reads when it stands the slot up. A read-only
|
* (see {@link #slots()}), each carrying the {@code profile} reference the spawn lifecycle
|
||||||
* snapshot taken at construction.</li>
|
* reads when it stands the slot up. <strong>Live, since fleetd #424</strong>: {@link #live}
|
||||||
* <li><b>terminal bindings</b> — owned by this registry and initially <em>empty</em>. Config
|
* re-reads {@code fleet:} on every call, through a supplier the same shape as
|
||||||
* declares no architect terminal, so at startup every slot is idle and nothing resolves to an
|
* {@code CompositePeerLauncher}'s (see {@code ConfigRef}'s class doc) — so a config reload
|
||||||
* architect; a session only becomes one when the spawn lifecycle {@linkplain #bind(String,
|
* that removes or adds an architect slot governs the <em>next</em> spawn with no restart.
|
||||||
* String) binds} its terminal to a slot. {@link CallerResolver} reads this through
|
* Only {@link #MemberRegistry(FleetConfig.Fleet)} freezes the pool at construction, and that
|
||||||
* {@link #snapshot()} to turn a pane into an {@link Role#ARCHITECT}.</li>
|
* constructor exists for tests and for the (rare) case of wiring a fixed, code-built config.</li>
|
||||||
|
* <li><b>terminal bindings</b> — owned by this registry, initially <em>empty</em>, and
|
||||||
|
* <strong>never</strong> touched by a reload. Config declares no architect terminal, so at
|
||||||
|
* startup every slot is idle and nothing resolves to an architect; a session only becomes one
|
||||||
|
* when the spawn lifecycle {@linkplain #bind(String, String) binds} its terminal to a slot.
|
||||||
|
* {@link CallerResolver} reads this through {@link #snapshot()} to turn a pane into an
|
||||||
|
* {@link Role#ARCHITECT}.</li>
|
||||||
* </ul>
|
* </ul>
|
||||||
*
|
*
|
||||||
|
* <p><strong>The binding rule (fleetd #424): config governs what a bound slot still grants, as
|
||||||
|
* well as what may be bound next.</strong> 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
|
||||||
|
* <em>not</em> change is the {@code terminalToSlot} <em>occupancy</em> — 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.
|
||||||
|
*
|
||||||
* <p>Spawning/lifecycle is deliberately a separate unit: this class only owns the bindings and
|
* <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.
|
* exposes the map the resolver resolves against plus the profile lookup lifecycle will call.
|
||||||
* Nothing here creates or manages an architect session.
|
* Nothing here creates or manages an architect session.
|
||||||
@@ -55,14 +83,42 @@ public final class MemberRegistry implements MemberLifecycle {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
private final Map<String, Entry> slots;
|
private final Supplier<FleetConfig.Fleet> fleet;
|
||||||
/** Live {@code terminal_id → qualified slot key}; guarded by {@code terminalToSlot}. */
|
/** Live {@code terminal_id → qualified slot key}; guarded by {@code terminalToSlot}. */
|
||||||
private final Map<String, String> terminalToSlot = new HashMap<>();
|
private final Map<String, String> terminalToSlot = new HashMap<>();
|
||||||
/** Slot keys held between reservation and the terminal binding. Guarded by terminalToSlot. */
|
/** Slot keys held between reservation and the terminal binding. Guarded by terminalToSlot. */
|
||||||
private final java.util.Set<String> reservedSlots = new java.util.HashSet<>();
|
private final java.util.Set<String> reservedSlots = new java.util.HashSet<>();
|
||||||
|
|
||||||
/** 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) {
|
public MemberRegistry(FleetConfig.Fleet fleet) {
|
||||||
|
this(() -> fleet);
|
||||||
|
}
|
||||||
|
|
||||||
|
private MemberRegistry(Supplier<FleetConfig.Fleet> 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<FleetConfig.Fleet>} 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<FleetConfig.Fleet> 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<String, Entry> flatten(FleetConfig.Fleet fleet) {
|
||||||
Map<String, Entry> flat = new LinkedHashMap<>();
|
Map<String, Entry> flat = new LinkedHashMap<>();
|
||||||
if (fleet != null) {
|
if (fleet != null) {
|
||||||
for (MemberRole role : MemberRole.values()) {
|
for (MemberRole role : MemberRole.values()) {
|
||||||
@@ -74,18 +130,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:} <em>as of this call</em> — see the class doc for which constructor makes that
|
||||||
|
* live versus frozen.
|
||||||
|
*/
|
||||||
public Map<String, Entry> slots() {
|
public Map<String, Entry> 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<String, Entry> slotsFor(MemberRole role) {
|
public Map<String, Entry> slotsFor(MemberRole role) {
|
||||||
Map<String, Entry> out = new LinkedHashMap<>();
|
Map<String, Entry> out = new LinkedHashMap<>();
|
||||||
slots.forEach((key, e) -> {
|
slots().forEach((key, e) -> {
|
||||||
if (e.role() == role) {
|
if (e.role() == role) {
|
||||||
out.put(key, e);
|
out.put(key, e);
|
||||||
}
|
}
|
||||||
@@ -123,25 +183,35 @@ public final class MemberRegistry implements MemberLifecycle {
|
|||||||
* declares none
|
* declares none
|
||||||
*/
|
*/
|
||||||
public String profileForSlot(String slotName) {
|
public String profileForSlot(String slotName) {
|
||||||
Entry e = slots.get(slotName);
|
Entry e = slots().get(slotName);
|
||||||
return (e == null || e.profile() == null) ? null : e.profile();
|
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) {
|
public MemberRole roleForSlot(String slotName) {
|
||||||
Entry e = slots.get(slotName);
|
Entry e = slots().get(slotName);
|
||||||
return e == null ? null : e.role();
|
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) {
|
public String nameForSlot(String slotName) {
|
||||||
Entry e = slots.get(slotName);
|
Entry e = slots().get(slotName);
|
||||||
return e == null ? null : e.name();
|
return e == null ? null : e.name();
|
||||||
}
|
}
|
||||||
|
|
||||||
/** True when {@code slotName} is a configured architect slot. */
|
/** True when {@code slotName} is a configured architect slot. */
|
||||||
public boolean isSlot(String slotName) {
|
public boolean isSlot(String slotName) {
|
||||||
return slots.containsKey(slotName);
|
return slots().containsKey(slotName);
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
|||||||
@@ -32,18 +32,26 @@ import java.util.function.Supplier;
|
|||||||
* not the fact that they are config. Most of {@code fleet:} — every role pool
|
* not the fact that they are config. Most of {@code fleet:} — every role pool
|
||||||
* ({@code architects}/{@code developers}/{@code reviewers}), {@code charters}, and
|
* ({@code architects}/{@code developers}/{@code reviewers}), {@code charters}, and
|
||||||
* {@code tabLabel} — is read the same live way, through the same supplier
|
* {@code tabLabel} — is read the same live way, through the same supplier
|
||||||
* ({@code () -> config.get().fleet()}). <strong>But {@code fleet:} as a whole is NOT in this
|
* ({@code () -> config.get().fleet()}). {@code architects} in particular is hot for
|
||||||
* class</strong>: {@code fleet.leaders} inside the same key is frozen, which is exactly what
|
* <strong>two independent consumers</strong> (fleetd #424): {@code CompositePeerLauncher}
|
||||||
* makes {@code fleet:} split rather than hot — see below. {@code models:} (fleetd #422) joined
|
* reads it live for placement (which profile an unqualified architect spawn may land on), and
|
||||||
* this class whole: {@link FleetConfig#validateModels()} re-runs fully against the fresh
|
* {@code MemberRegistry} separately reads it live, through its own instance of the same
|
||||||
* config on every {@link #reload()} (via {@link FleetConfig#validateAll()}), refusing a bad
|
* supplier shape, for identity — both which slot a spawn may bind to <em>and</em> what a slot
|
||||||
* edit outright rather than caching a stale copy anywhere, and the on/off half added by
|
* already bound still grants. Removing an architect slot from config therefore revokes the
|
||||||
* fleetd #422 is read live both by {@code CompositePeerLauncher}'s spawn gate
|
* {@link dev.ltms.fleet.auth.Role#ARCHITECT} role on the bound pane's very next request; only
|
||||||
* ({@code enforceModelEnabled} and its candidate filter) and by {@code fleet_profiles}/
|
* the slot <em>occupancy</em> survives, so the demoted session still holds its slot key until
|
||||||
* {@code GET /profiles} (via {@code PeerLauncher.disabledModels()}). Nothing about
|
* it unbinds. See {@code MemberRegistry}'s class doc for that binding rule.
|
||||||
* {@code models:} is baked into an object built at startup, so — unlike the deferred keys
|
* <strong>But {@code fleet:} as a whole is NOT in this class</strong>: {@code fleet.leaders}
|
||||||
* below — there is no frozen half left to report; it moved here from deferred rather than
|
* inside the same key is frozen, which is exactly what makes {@code fleet:} split rather than
|
||||||
* joining split.</li>
|
* hot — see below. {@code models:} (fleetd #422) joined this class whole: {@link
|
||||||
|
* FleetConfig#validateModels()} re-runs fully against the fresh config on every {@link
|
||||||
|
* #reload()} (via {@link FleetConfig#validateAll()}), refusing a bad edit outright rather than
|
||||||
|
* caching a stale copy anywhere, and the on/off half added by fleetd #422 is read live both by
|
||||||
|
* {@code CompositePeerLauncher}'s spawn gate ({@code enforceModelEnabled} and its candidate
|
||||||
|
* filter) and by {@code fleet_profiles}/{@code GET /profiles} (via
|
||||||
|
* {@code PeerLauncher.disabledModels()}). Nothing about {@code models:} is baked into an
|
||||||
|
* object built at startup, so — unlike the deferred keys below — there is no frozen half left
|
||||||
|
* to report; it moved here from deferred rather than joining split.</li>
|
||||||
* <li><strong>Deferred</strong> — accepted into the new snapshot, but the wiring built at startup
|
* <li><strong>Deferred</strong> — accepted into the new snapshot, but the wiring built at startup
|
||||||
* keeps the old value until a restart: {@code lifecycle:}, {@code leadHeartbeat:},
|
* keeps the old value until a restart: {@code lifecycle:}, {@code leadHeartbeat:},
|
||||||
* {@code idleSleepGuard:} ({@code Fleetd.java} reads it once, at startup, to decide whether
|
* {@code idleSleepGuard:} ({@code Fleetd.java} reads it once, at startup, to decide whether
|
||||||
@@ -525,26 +533,35 @@ public final class ConfigRef implements Supplier<FleetConfig> {
|
|||||||
+ "opened once and needs a restart; the broker URI env-var name kept out of a "
|
+ "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");
|
+ "member's environment is read live on every spawn and already applied");
|
||||||
}
|
}
|
||||||
// fleetd #333: unlike health/coordinator above, most of `fleet:` (architects, developers,
|
// fleetd #333: unlike health/coordinator above, most of `fleet:` (developers, reviewers,
|
||||||
// reviewers, charters, tabLabel) is genuinely hot — ConfigRefTest.aHotChangeIsAppliedAndRead-
|
// charters, tabLabel) is genuinely hot — ConfigRefTest.aHotChangeIsAppliedAndRead-
|
||||||
// ThroughGet and aCharterChangeIsHotAndReachesTheLiveConfig prove it reaches the live config
|
// ThroughGet and aCharterChangeIsHotAndReachesTheLiveConfig prove it reaches the live config
|
||||||
// with no restart note. Only fleet.leaders is frozen (Fleetd.java:281 reads
|
// with no restart note. `architects` is hot too, and — since fleetd #424 — hot for BOTH of
|
||||||
// cfg.fleet().leaders() off the startup snapshot to build both the LeadTabScanner's
|
// its consumers, not just the one this comment used to name: CompositePeerLauncher reads it
|
||||||
// tab-label-to-name map, wired into CallerResolver.withLeadsAndMembers at Fleetd.java:620/624,
|
// live for PLACEMENT through the () -> config.get().fleet() supplier named in the class doc's
|
||||||
// and — when herdr answered — LeadLauncher(...).ensureLeads() at Fleetd.java:315, which
|
// Hot bullet, and MemberRegistry separately reads it live for IDENTITY (which slot a spawn
|
||||||
// auto-launches each lead up to its `instances` count; neither is rebuilt on reload). So this
|
// may bind to, AND what a slot already bound still grants) through its own instance of that
|
||||||
// compares fleet.leaders alone, not the whole Fleet record: comparing the whole record would
|
// same supplier shape — see MemberRegistry.live and its class doc for the binding rule:
|
||||||
// report "split" for a tabLabel-only or charters-only change that is actually fully hot,
|
// removing a slot revokes ARCHITECT on the bound pane's very next request, and only the slot
|
||||||
// which is the over-claim mirror of the under-claim bug this class exists to prevent.
|
// OCCUPANCY survives, so the demoted session keeps its slot key until it unbinds. 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))) {
|
if (!Objects.equals(leadersOf(old), leadersOf(fresh))) {
|
||||||
changed.add("fleet: fleet.leaders (each lead's tab, workspace, cwd, profile and "
|
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 "
|
+ "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 "
|
+ "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 "
|
+ "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 "
|
+ "stays unrecognised, and a caller from its new tab resolves as a worker, not a "
|
||||||
+ "lead; the rest of fleet: (architects, developers, reviewers, charters, "
|
+ "lead; the rest of fleet: (developers, reviewers, charters, tabLabel) is read "
|
||||||
+ "tabLabel) is read live through the supplier on CompositePeerLauncher and "
|
+ "live through the supplier on CompositePeerLauncher, and architects is read "
|
||||||
+ "already applied");
|
+ "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 —
|
// 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.
|
// every message here must be traceable to one of the split keys the class doc documents.
|
||||||
|
|||||||
@@ -0,0 +1,257 @@
|
|||||||
|
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;
|
||||||
|
|
||||||
|
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. A session already bound to a slot keeps its <em>binding</em> (the {@code
|
||||||
|
* terminalToSlot} occupancy) even after that slot drops out of config, but NOT the ARCHITECT
|
||||||
|
* <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
|
||||||
|
* {@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 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 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_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());
|
||||||
|
|
||||||
|
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_a", "architect:designer"), registry.snapshot());
|
||||||
|
|
||||||
|
// 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());
|
||||||
|
|
||||||
|
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.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());
|
||||||
|
}
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user