#284/#285: one reclaimable rule, one group-share helper
Two corrections on top of the merged worker branches. #284: I told the worker to report a BACKEND_ERROR/FAILED session as reclaimable. That half of my own ticket was wrong. Once the live count stops counting a terminal session, its seat is already in `free`; counting it in `reclaimable` too reports the same seat twice, and `free + reclaimable` reads as more capacity than maxLoad allows. Worse, only the profile-level count was widened, so the same fleet_list response said `reclaimable: 2` while every member row said `reclaimable: false`. Both views now call one shared predicate, FleetMcp.reclaimable, so they cannot drift apart. A test runs it over every MemberSession.State value, so a state added later cannot slip through unconsidered. #285: the new per-file chgrp+chmod helper moved from ClaudeCodeLauncher into EnvAllowListScrub as shareFileWithGroup, next to the directory-wide shareWithGroup it was copied from. It now reuses that class's own setGroupAndPermissions and also catches UnsupportedOperationException, which the copy missed — on a filesystem without POSIX group ownership the copy threw a raw runtime exception instead of the sibling's UncheckedIOException.
This commit is contained in:
@@ -1167,15 +1167,35 @@ public final class FleetMcp {
|
||||
private static Map<String, Object> memberCapacityView(MemberSession session, Agent live,
|
||||
MessageService messages, long nowNanos) {
|
||||
Map<String, Object> row = SessionManager.rosterView(session, live);
|
||||
boolean open = messages != null && messages.hasAcceptedDelivery(session.terminalId());
|
||||
boolean inbox = messages != null && messages.hasInboxMessage(session.terminalId());
|
||||
boolean reclaimable = (session.state() == MemberSession.State.READY || session.state() == MemberSession.State.DONE)
|
||||
&& !open && !inbox;
|
||||
boolean reclaimable = reclaimable(session, messages);
|
||||
row.put("reclaimable", reclaimable);
|
||||
row.put("idleForSeconds", reclaimable ? Math.max(0, (nowNanos - session.lastActivityAtNanos()) / 1_000_000_000L) : null);
|
||||
return row;
|
||||
}
|
||||
|
||||
/**
|
||||
* The one definition of {@code reclaimable}: this member holds a spawn seat, and has no open
|
||||
* bridge work, so stopping it gives the seat back. Both views in a single {@code fleet_list}
|
||||
* response call it — the per-member flag in {@link #memberCapacityView} and the per-profile
|
||||
* count in {@link #capacityView} — because two copies of this rule in one response is how the
|
||||
* two numbers come to disagree.
|
||||
*
|
||||
* <p>fleetd #284: {@code BACKEND_ERROR} and {@code FAILED} are deliberately NOT reclaimable.
|
||||
* The ticket asked for them to be, and that half of the ticket was wrong. Once
|
||||
* {@code Fleetd.liveSessionCount} stopped counting a terminal session as live, that seat is
|
||||
* ALREADY in {@code free}; counting it here too reports the same seat twice, and
|
||||
* {@code free + reclaimable} then reads as more capacity than {@code maxLoad} allows. The dead
|
||||
* session stays visible either way: its roster row still carries {@code state:
|
||||
* "backend_error"} or {@code "failed"}, which is what tells the lead to stop it.
|
||||
*/
|
||||
static boolean reclaimable(MemberSession session, MessageService messages) {
|
||||
boolean holdsSeat = session.state() == MemberSession.State.READY
|
||||
|| session.state() == MemberSession.State.DONE;
|
||||
return holdsSeat && (messages == null
|
||||
|| (!messages.hasAcceptedDelivery(session.terminalId())
|
||||
&& !messages.hasInboxMessage(session.terminalId())));
|
||||
}
|
||||
|
||||
/**
|
||||
* CB-583: {@code free} alone cannot tell a lead "busy, will free up" from "refusing, and
|
||||
* nothing changes for N seconds" — those need different decisions. So a quarantined profile
|
||||
@@ -1214,9 +1234,7 @@ public final class FleetMcp {
|
||||
int live = liveCount.apply(profile);
|
||||
int leadSeatCount = leadSeats.seatsFor().apply(profile);
|
||||
int reclaimable = (int) roster.stream().filter(s -> profile.equals(s.profile()))
|
||||
.filter(s -> s.state() == MemberSession.State.READY || s.state() == MemberSession.State.DONE
|
||||
|| s.state() == MemberSession.State.BACKEND_ERROR || s.state() == MemberSession.State.FAILED)
|
||||
.filter(s -> messages == null || (!messages.hasAcceptedDelivery(s.terminalId()) && !messages.hasInboxMessage(s.terminalId())))
|
||||
.filter(s -> reclaimable(s, messages))
|
||||
.count();
|
||||
Map<String, Object> row = new LinkedHashMap<>();
|
||||
row.put("profile", profile); row.put("maxLoad", cap); row.put("live", live);
|
||||
|
||||
@@ -18,9 +18,7 @@ import java.io.UncheckedIOException;
|
||||
import java.nio.file.Files;
|
||||
import java.nio.file.Path;
|
||||
import java.nio.file.StandardCopyOption;
|
||||
import java.nio.file.attribute.GroupPrincipal;
|
||||
import java.nio.file.attribute.PosixFileAttributeView;
|
||||
import java.nio.file.attribute.PosixFilePermissions;
|
||||
import java.util.Arrays;
|
||||
import java.util.EnumSet;
|
||||
import java.util.List;
|
||||
@@ -567,7 +565,8 @@ public final class ClaudeCodeLauncher extends HerdrPeerLauncher {
|
||||
* for "spawn something" is not worth it. When {@code configDir} and {@code worktreeGroup} are
|
||||
* both present, the write proceeds exactly as below and the resulting file is additionally
|
||||
* chgrp'd/chmod'd group-readable ({@code rw-r-----}) via {@link
|
||||
* #shareTrustJsonWithGroup(Path, String)} so the member's OS user can actually open it — the
|
||||
* EnvAllowListScrub#shareFileWithGroup(Path, String)} so the member's OS user can actually
|
||||
* open it — the
|
||||
* directory itself (unlike {@code worktreeRoot} or the charter's per-spawn directory) is not
|
||||
* fleetd-managed, so its own traversal permissions remain the operator's setup, same as they
|
||||
* already must be for the member to read anything else fleetd points {@code CLAUDE_CONFIG_DIR}
|
||||
@@ -691,42 +690,11 @@ public final class ClaudeCodeLauncher extends HerdrPeerLauncher {
|
||||
// means the seed is unreadable despite a successful write, which must fail as loudly as
|
||||
// writeCharterFile's own EnvAllowListScrub.shareWithGroup call already does.
|
||||
if (written && memberHerdrSocket) {
|
||||
shareTrustJsonWithGroup(target, group);
|
||||
EnvAllowListScrub.shareFileWithGroup(target, group);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* fleetd #285: chgrp + chmod {@code target} — a {@code .claude.json} this method just wrote as
|
||||
* fleetd's own OS user — group-readable ({@code rw-r-----}) for {@code group}, so a member OS
|
||||
* user under {@code memberHerdrSocket} can open it. Mirrors {@link
|
||||
* EnvAllowListScrub#shareWithGroup}'s per-file mode exactly (read-only for the group — a member
|
||||
* never needs to write this file itself), but touches only the FILE, never its parent
|
||||
* directory: unlike {@code worktreeRoot} or the charter's per-spawn directory, {@code
|
||||
* configDir} is not a directory fleetd creates or owns, so its traversal permissions are left to
|
||||
* the operator's own setup, same as they already must be for the member to read anything else
|
||||
* fleetd points {@code CLAUDE_CONFIG_DIR} at.
|
||||
*
|
||||
* @throws UncheckedIOException when {@code group} does not resolve on this host, or a
|
||||
* group-ownership/permission call is refused — the file was written
|
||||
* but is not usable by the member, and that must be loud
|
||||
*/
|
||||
private static void shareTrustJsonWithGroup(Path target, String group) {
|
||||
try {
|
||||
GroupPrincipal principal = target.getFileSystem().getUserPrincipalLookupService()
|
||||
.lookupPrincipalByGroupName(group);
|
||||
PosixFileAttributeView view = Files.getFileAttributeView(target, PosixFileAttributeView.class);
|
||||
if (view == null) {
|
||||
throw new IOException("POSIX file attributes are not supported for " + target);
|
||||
}
|
||||
view.setGroup(principal);
|
||||
Files.setPosixFilePermissions(target, PosixFilePermissions.fromString("rw-r-----"));
|
||||
} catch (IOException e) {
|
||||
throw new UncheckedIOException("cannot share workspace-trust file " + target + " with "
|
||||
+ "group '" + group + "' — the group must exist, and the fleetd operator ("
|
||||
+ System.getProperty("user.name") + ") must be a member of it", e);
|
||||
}
|
||||
}
|
||||
|
||||
/** Bound on {@link #seedTrustDialog}'s fleetd #247 compare-and-swap retry loop. */
|
||||
private static final int MAX_TRUST_JSON_CAS_ATTEMPTS = 5;
|
||||
|
||||
@@ -181,6 +181,33 @@ public final class EnvAllowListScrub {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* fleetd #285: share ONE file with {@code group}, read-only ({@code rw-r-----}) — the same
|
||||
* per-file mode {@link #shareWithGroup} applies to a directory's entries, and the same error
|
||||
* shapes, but without touching a parent directory. Used for a file fleetd writes into a
|
||||
* directory it does NOT own — {@code configDir}'s own traversal permissions stay the
|
||||
* operator's setup — where the directory-wide {@link #shareWithGroup} would be wrong.
|
||||
*
|
||||
* @throws UncheckedIOException when {@code group} does not resolve on this host, the
|
||||
* filesystem has no POSIX group ownership, or a
|
||||
* group-ownership/permission call is refused
|
||||
*/
|
||||
static void shareFileWithGroup(Path file, String group) {
|
||||
try {
|
||||
GroupPrincipal principal = file.getFileSystem().getUserPrincipalLookupService()
|
||||
.lookupPrincipalByGroupName(group);
|
||||
setGroupAndPermissions(file, principal, "rw-r-----");
|
||||
} catch (IOException e) {
|
||||
throw new UncheckedIOException("cannot share generated file " + file + " with group '"
|
||||
+ group + "' — the group must exist, and the fleetd operator ("
|
||||
+ System.getProperty("user.name") + ") must be a member of it", e);
|
||||
} catch (UnsupportedOperationException e) {
|
||||
throw new UncheckedIOException("cannot share generated file " + file + " with group '"
|
||||
+ group + "' — this filesystem does not support POSIX group ownership",
|
||||
new IOException(e));
|
||||
}
|
||||
}
|
||||
|
||||
private static void setGroupAndPermissions(Path path, GroupPrincipal group, String perms) throws IOException {
|
||||
PosixFileAttributeView view = Files.getFileAttributeView(path, PosixFileAttributeView.class);
|
||||
if (view == null) {
|
||||
|
||||
@@ -31,6 +31,7 @@ import org.junit.jupiter.api.Test;
|
||||
import java.util.ArrayList;
|
||||
import java.util.List;
|
||||
import java.util.Map;
|
||||
import java.util.EnumSet;
|
||||
import java.util.Set;
|
||||
import java.util.concurrent.CompletableFuture;
|
||||
import java.util.concurrent.TimeUnit;
|
||||
@@ -604,8 +605,39 @@ class FleetMcpTest {
|
||||
assertTrue(out.contains("\"reclaimable\":0"), out);
|
||||
}
|
||||
|
||||
/**
|
||||
* fleetd #284: {@code reclaimable} has exactly one definition, and this pins it over EVERY
|
||||
* {@link MemberSession.State} — so a state added later cannot slip through unconsidered. Both
|
||||
* views in a {@code fleet_list} response call {@link FleetMcp#reclaimable}, so they cannot
|
||||
* drift apart.
|
||||
*
|
||||
* <p>{@code BACKEND_ERROR} and {@code FAILED} are NOT reclaimable on purpose. The ticket asked
|
||||
* for them to be; that half of the ticket was wrong. Their seat is already out of
|
||||
* {@code live}, so it is already in {@code free} — counting it here too would report the same
|
||||
* seat twice.
|
||||
*/
|
||||
@Test
|
||||
void capacityReportsTerminalFailureSessionsAsReclaimable() {
|
||||
void onlyReadyAndDoneSessionsAreReclaimable() {
|
||||
Set<MemberSession.State> expected = EnumSet.of(MemberSession.State.READY, MemberSession.State.DONE);
|
||||
for (MemberSession.State state : MemberSession.State.values()) {
|
||||
MemberSession session = new MemberSession("p1", "term1", "ltms-local", null,
|
||||
"/tmp", null, 0L, 0L, 0, state, null, null);
|
||||
assertEquals(expected.contains(state), FleetMcp.reclaimable(session, null),
|
||||
"state " + state + " must " + (expected.contains(state) ? "" : "not ")
|
||||
+ "count as reclaimable");
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* fleetd #284, the operator-visible half. {@code liveCount} here is the value the real counter
|
||||
* ({@code Fleetd.liveSessionCount}, proven against the actual spawn gate in
|
||||
* {@code FleetdBackendErrorSinkTest}) produces for this roster: 0, because both sessions are
|
||||
* terminal. What this test pins is what {@code fleet_list} says around it — the two seats show
|
||||
* up once, in {@code free}, and are NOT counted a second time as {@code reclaimable}; neither
|
||||
* is any member row; and both dead sessions are still listed so the lead can see why.
|
||||
*/
|
||||
@Test
|
||||
void terminalFailureSessionsFreeTheirSeatWithoutBeingCountedReclaimable() {
|
||||
FakeHerdr h = new FakeHerdr();
|
||||
SessionManager sessions = new SessionManager(workerService(h, "http://gx00.gw:8000", Set.of("gx00.gw")));
|
||||
MemberSession backendError = sessions.acquire("ltms-local", null, null, null);
|
||||
@@ -614,11 +646,17 @@ class FleetMcpTest {
|
||||
sessions.onTurnFailed(failed.terminalId());
|
||||
|
||||
String out = textOf(FleetMcp.listFleet(workerService(h, "http://gx00.gw:8000", Set.of("gx00.gw")),
|
||||
sessions, null, new FleetMcp.CapacitySource(profile -> 0, profile -> 1,
|
||||
sessions, null, new FleetMcp.CapacitySource(profile -> 0, profile -> 2,
|
||||
() -> Set.of("ltms-local"), () -> 0), new FleetMcp.HealthCoverageSource(() -> "off"),
|
||||
FleetMcp.QuarantineSource.none(), Map.of(), ""));
|
||||
|
||||
assertTrue(out.contains("\"reclaimable\":2"), out);
|
||||
assertTrue(out.contains("\"free\":2"), "both seats are back: " + out);
|
||||
assertTrue(out.contains("\"reclaimable\":0"),
|
||||
"the freed seats must not be counted a second time as reclaimable: " + out);
|
||||
assertFalse(out.contains("\"reclaimable\":true"),
|
||||
"no member row may claim a seat the profile count says is not held: " + out);
|
||||
assertTrue(out.contains("\"state\":\"backend_error\""), "the dead session stays visible: " + out);
|
||||
assertTrue(out.contains("\"state\":\"failed\""), "the failed session stays visible: " + out);
|
||||
}
|
||||
|
||||
@Test
|
||||
|
||||
Reference in New Issue
Block a user