From 1fdaa74eb33c5d318249f3ba13331041b0faa46d Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Mon, 31 Aug 2026 22:15:16 +0700 Subject: [PATCH] fleetd #209 follow-up: keep roster() off the resolve path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit roster() is the supplier for LeadHeartbeatLoop and FleetHealthMonitor (both timer-driven) and for placement/exhaustion checks and the metrics scrape — none of which read agentSessionId. Resolving there meant every tick could open a lazy-resolving adapter's (opencode's) on-disk session database once per member whose id was still unknown, with no bound: a member whose id never appears would pay that cost for the life of the process. roster() goes back to its pre-#209 behavior (no resolve, no I/O). A new rosterResolved() carries the resolve logic, and is used only by the two surfaces that actually report agentSessionId to a caller: fleet_list (FleetMcp.listFleet) and the REST roster (FleetApp.listMembers). fleet_whoami's roster().stream() at FleetMcp.java:768 does not surface the field, so it stays on the plain roster(). get(paneId) (fleet_status) and the release() resolve are caller-driven, not timers, and are unchanged. Retargeted the roster-facing tests from #209 at rosterResolved(), and added plainRosterDoesNotResolveAgentSessionId, which pins the split by asserting the handle's agentSessionId() is not called again by roster(). --- .../java/dev/ltms/fleet/mcp/FleetMcp.java | 5 ++- .../java/dev/ltms/fleet/rest/FleetApp.java | 4 ++- .../ltms/fleet/session/SessionManager.java | 24 ++++++++++--- .../fleet/session/SessionManagerTest.java | 36 ++++++++++++++----- 4 files changed, 55 insertions(+), 14 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 efff9dc..4b664e3 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java +++ b/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java @@ -951,7 +951,10 @@ public final class FleetMcp { .sorted(Map.Entry.comparingByValue()) .map(e -> leadView(e.getKey(), e.getValue(), live.get(e.getKey()), selfTerm)) .toList(); - List roster = sessions.roster(); + // fleetd #209: this is the caller-driven fleet_list read that actually reports + // agentSessionId (via memberCapacityView -> SessionManager.rosterView), so it uses the + // resolving roster; the heartbeat/health/metrics timers stay on the plain sessions.roster(). + List roster = sessions.rosterResolved(); List> out = roster.stream() .map(s -> memberCapacityView(s, live.get(s.terminalId()), messages, capacity.clock().getAsLong())) .toList(); diff --git a/fleetd/src/main/java/dev/ltms/fleet/rest/FleetApp.java b/fleetd/src/main/java/dev/ltms/fleet/rest/FleetApp.java index 5e479f1..665b002 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/rest/FleetApp.java +++ b/fleetd/src/main/java/dev/ltms/fleet/rest/FleetApp.java @@ -315,7 +315,9 @@ public final class FleetApp { .map(Agent.class::cast) .filter(a -> a.terminalId() != null) .collect(Collectors.toMap(Agent::terminalId, Function.identity(), (_, b) -> b)); - List> out = sessions.roster().stream() + // fleetd #209: this REST roster reports agentSessionId via SessionManager.rosterView, so it + // uses the resolving roster read (caller-driven, not a timer) rather than the plain one. + List> out = sessions.rosterResolved().stream() .map(s -> SessionManager.rosterView(s, live.get(s.terminalId()))) .toList(); Map body = new LinkedHashMap<>(); diff --git a/fleetd/src/main/java/dev/ltms/fleet/session/SessionManager.java b/fleetd/src/main/java/dev/ltms/fleet/session/SessionManager.java index 147eb18..eb2f938 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/session/SessionManager.java +++ b/fleetd/src/main/java/dev/ltms/fleet/session/SessionManager.java @@ -567,12 +567,28 @@ public final class SessionManager implements TurnListener { } /** - * Fleet-owned roster: all registered sessions (acquired minus released). fleetd #209: each - * session with a still-unknown {@code agentSessionId} is re-resolved against its retained - * handle, so {@code fleet_list} reports an id a lazy-resolving adapter (opencode) has since - * written, rather than the null frozen in at spawn time. + * Fleet-owned roster: all registered sessions (acquired minus released). Deliberately does + * not resolve {@code agentSessionId} (fleetd #209 follow-up) — this is the + * roster supplier on the heartbeat and health-tick timers ({@code LeadHeartbeatLoop}, + * {@code FleetHealthMonitor} in {@code Fleetd}), on the placement/exhaustion paths, and on the + * metrics scrape ({@code FleetMetrics}), all called far more often than any caller actually + * reads {@code agentSessionId}. Resolving here would mean every tick opens a lazy-resolving + * adapter's (opencode's) on-disk session store once per member whose id is still unknown — and + * for a member whose id never appears, that cost never stops, for the life of the process. Use + * {@link #rosterResolved()} instead wherever the id must be current. */ public List roster() { + return List.copyOf(registry.values()); + } + + /** + * {@link #roster()}, with each session's still-unknown {@code agentSessionId} re-resolved + * against its retained handle (fleetd #209) — so a caller that actually reports the id ( + * {@code fleet_list}, the REST roster) sees one a lazy-resolving adapter (opencode) has since + * written, rather than the null frozen in at spawn time. Reserved for caller-driven reads, not + * timers: see {@link #roster()}'s javadoc for why the plain roster must stay non-resolving. + */ + public List rosterResolved() { return registry.values().stream().map(this::resolveAgentSessionId).toList(); } diff --git a/fleetd/src/test/java/dev/ltms/fleet/session/SessionManagerTest.java b/fleetd/src/test/java/dev/ltms/fleet/session/SessionManagerTest.java index 2802c40..9fc0d37 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/session/SessionManagerTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/session/SessionManagerTest.java @@ -1070,7 +1070,27 @@ class SessionManagerTest { } @Test - void rosterResolvesALateAgentSessionIdFromTheRetainedHandle() { + void plainRosterDoesNotResolveAgentSessionId() { + // fleetd #209 follow-up: roster() sits on the heartbeat/health-tick timers (and the metrics + // scrape), so it must never trigger the resolve lookup — for opencode that lookup opens an + // on-disk session database, and a member whose id never appears would pay that cost forever. + // rosterResolved() is the one to use when a caller actually reports the id. + LazyIdHandle handle = new LazyIdHandle("p0", "t0", 1, "oc-session-0"); + SessionManager sessions = new SessionManager(new LazyIdLauncher().queue(handle)); + MemberSession acquired = sessions.acquire("lazy", "/cwd", "/caller", null); + assertEquals(1, handle.callCount(), "sanity: only the spawn-time call happened so far"); + + List roster = sessions.roster(); + + assertEquals(1, roster.size()); + assertEquals(acquired.paneId(), roster.getFirst().paneId()); + assertNull(roster.getFirst().agentSessionId(), "the plain roster must not resolve the id"); + assertEquals(1, handle.callCount(), + "roster() must never call agentSessionId() again — it sits on the heartbeat/health timers"); + } + + @Test + void rosterResolvedResolvesALateAgentSessionIdFromTheRetainedHandle() { LazyIdHandle handle = new LazyIdHandle("p1", "t1", 1, "oc-session-1"); SessionManager sessions = new SessionManager(new LazyIdLauncher().queue(handle)); @@ -1078,14 +1098,14 @@ class SessionManagerTest { assertNull(acquired.agentSessionId(), "opencode has not written its session row yet at spawn time"); - List roster = sessions.roster(); + List roster = sessions.rosterResolved(); assertEquals(1, roster.size()); assertEquals("oc-session-1", roster.getFirst().agentSessionId(), "fleet_list must see the id once the adapter can answer it"); Map view = SessionManager.rosterView(roster.getFirst(), null); assertEquals("oc-session-1", view.get("agentSessionId"), - "rosterView renders whatever roster() resolved"); + "rosterView renders whatever rosterResolved() resolved"); } @Test @@ -1116,13 +1136,13 @@ class SessionManagerTest { } @Test - void aThrowingHandleDoesNotBreakRoster() { + void aThrowingHandleDoesNotBreakRosterResolved() { LazyIdHandle handle = new LazyIdHandle("p4", "t4", 1, "oc-session-4") .throwing(new RuntimeException("sqlite locked")); SessionManager sessions = new SessionManager(new LazyIdLauncher().queue(handle)); sessions.acquire("lazy", "/cwd", "/caller", null); - List roster = assertDoesNotThrow(sessions::roster, + List roster = assertDoesNotThrow(sessions::rosterResolved, "a handle that throws resolving its id must not break the roster read"); assertEquals(1, roster.size()); @@ -1136,10 +1156,10 @@ class SessionManagerTest { sessions.acquire("lazy", "/cwd", "/caller", null); assertEquals(1, handle.callCount(), "sanity: only the spawn-time call happened so far"); - sessions.roster(); - assertEquals(2, handle.callCount(), "the first roster read resolves the id"); + sessions.rosterResolved(); + assertEquals(2, handle.callCount(), "the first rosterResolved() read resolves the id"); - sessions.roster(); + sessions.rosterResolved(); assertEquals(2, handle.callCount(), "once resolved, the id must not be looked up again"); } }