From 94f50e507ad9a6312228af9e5786f53508dadc20 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Fri, 4 Sep 2026 11:13:01 +0700 Subject: [PATCH] #284/#285: one reclaimable rule, one group-share helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../java/dev/ltms/fleet/mcp/FleetMcp.java | 32 +++++++++++--- .../ltms/fleet/member/ClaudeCodeLauncher.java | 38 ++-------------- .../ltms/fleet/member/EnvAllowListScrub.java | 27 ++++++++++++ .../java/dev/ltms/fleet/mcp/FleetMcpTest.java | 44 +++++++++++++++++-- 4 files changed, 96 insertions(+), 45 deletions(-) diff --git a/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java b/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java index 7d0fb8b..d567411 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java +++ b/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java @@ -1167,15 +1167,35 @@ public final class FleetMcp { private static Map memberCapacityView(MemberSession session, Agent live, MessageService messages, long nowNanos) { Map 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. + * + *

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 row = new LinkedHashMap<>(); row.put("profile", profile); row.put("maxLoad", cap); row.put("live", live); diff --git a/fleetd/src/main/java/dev/ltms/fleet/member/ClaudeCodeLauncher.java b/fleetd/src/main/java/dev/ltms/fleet/member/ClaudeCodeLauncher.java index b88f3da..41c20d9 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/ClaudeCodeLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/ClaudeCodeLauncher.java @@ -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; diff --git a/fleetd/src/main/java/dev/ltms/fleet/member/EnvAllowListScrub.java b/fleetd/src/main/java/dev/ltms/fleet/member/EnvAllowListScrub.java index b60e1a7..280636d 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/EnvAllowListScrub.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/EnvAllowListScrub.java @@ -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) { diff --git a/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpTest.java b/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpTest.java index f011b5d..e06da22 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpTest.java @@ -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. + * + *

{@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 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