diff --git a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java index 90cdb20..a93637f 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java +++ b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java @@ -666,8 +666,7 @@ public final class Fleetd { return configured == null ? null : configured.effectiveCredentialId(); }, outagePolicy); - FleetMcp.LoopHealthSource loopHealth = new FleetMcp.LoopHealthSource(poller::health, - () -> reaper == null ? LoopWatchdog.State.STOPPED : reaper.health()); + FleetMcp.LoopHealthSource loopHealth = loopHealthSource(poller, reaper); FleetMcp mcp = new FleetMcp(messages, workers, sessions, identity, presence, primaryRegistry, callers, FleetMcp.AuthorizationMode.ENFORCED, metrics, capacitySource(config, cfg, profile -> liveCountRef.get().apply(profile)), @@ -1042,6 +1041,31 @@ public final class Fleetd { }); } + /** + * fleetd #562 follow-up: package-private factory for {@code fleet_list}'s and {@code + * /healthz}'s {@code loopHealth} source, extracted out of {@code main} for the same reason + * {@link #capacitySource} and {@link #healthCoverageSource} were. Before this ticket the + * {@link FleetMcp.LoopHealthSource} was built inline with a bare {@code new}, so there was + * nothing a test could call directly — measured: replacing {@code poller::health} with a + * constant {@code () -> LoopWatchdog.State.RUNNING} at the call site compiled clean and left + * the full suite green, meaning the daemon could report the {@link StatusPoller} as always + * {@code RUNNING} even while it was actually stalled. That is a false negative on the exact + * signal this ticket exists to surface, and is the mirror of a false positive muting a real + * monitoring component — worse, because there is no noise for anyone to notice and then + * silence. {@link FleetdLoopHealthSourceWiringTest} calls this factory directly and pins both + * halves separately, plus the {@code reaper == null} branch below. + * + *

{@code reaper} may be {@code null} — a {@link SessionReaper} is only constructed when + * {@code lifecycle.idleTtlSeconds} is configured (see the {@code reaper} local above) — and + * this factory preserves the existing behaviour of reporting {@link LoopWatchdog.State#STOPPED} + * in that case, rather than a {@code NullPointerException} on the first {@code fleet_list} or + * {@code /healthz} call. + */ + static FleetMcp.LoopHealthSource loopHealthSource(StatusPoller poller, SessionReaper reaper) { + return new FleetMcp.LoopHealthSource(poller::health, + () -> reaper == null ? LoopWatchdog.State.STOPPED : reaper.health()); + } + /** * fleetd #248: package-private factory for the member worktree/branch lookup {@link * CompletionResolver} uses to name a fallback report's worktree and branch (fleetd#241). diff --git a/fleetd/src/test/java/dev/ltms/fleet/FleetdLoopHealthSourceWiringTest.java b/fleetd/src/test/java/dev/ltms/fleet/FleetdLoopHealthSourceWiringTest.java new file mode 100644 index 0000000..32fa2da --- /dev/null +++ b/fleetd/src/test/java/dev/ltms/fleet/FleetdLoopHealthSourceWiringTest.java @@ -0,0 +1,174 @@ +package dev.ltms.fleet; + +import dev.ltms.fleet.herdr.AgentControl; +import dev.ltms.fleet.herdr.FakeHerdr; +import dev.ltms.fleet.inject.Injector; +import dev.ltms.fleet.inject.LoopWatchdog; +import dev.ltms.fleet.inject.StatusPoller; +import dev.ltms.fleet.mcp.FleetMcp; +import dev.ltms.fleet.peer.Capability; +import dev.ltms.fleet.peer.PeerHandle; +import dev.ltms.fleet.peer.PeerLauncher; +import dev.ltms.fleet.peer.SpawnRequest; +import dev.ltms.fleet.placement.PlacementDecision; +import dev.ltms.fleet.session.SessionManager; +import dev.ltms.fleet.session.SessionReaper; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +import java.util.List; +import java.util.Set; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +/** + * fleetd #562 follow-up (issue comment "HOLD on PR #579"): {@code Fleetd.main}'s {@code loopHealth} + * local used to be a bare {@code new FleetMcp.LoopHealthSource(poller::health, ...)} built inline, + * with nothing a test could call directly. Measured on that shape: replacing {@code + * poller::health} with a constant {@code () -> LoopWatchdog.State.RUNNING} at the call site + * compiled with 0 errors and left all 1771 existing tests green — the daemon could be changed to + * always report the {@link StatusPoller} as {@code RUNNING}, so the watchdog could never fire and + * a stalled poller would be invisible, while every test stayed green. That is exactly the false + * negative this ticket exists to prevent. + * + *

The five tests PR #579 added ({@code FleetMcpTest}, {@code FleetAppTest}) all build their own + * {@link FleetMcp.LoopHealthSource} directly with fixed lambdas — they prove the seam ({@code + * LoopHealthSource} reports what it is given) and nothing about what {@code Fleetd.main} actually + * gives it. This is the same hand-built-vs-config-wired shape as fleetd #561/#248/#426. + * + *

The fix extracts the inline {@code new} into {@link Fleetd#loopHealthSource}, a package-private + * factory in the same style as {@link Fleetd#capacitySource} and {@link Fleetd#healthCoverageSource} + * — which is exactly what makes it directly callable here. This test calls that factory with real + * {@link StatusPoller}/{@link SessionReaper} instances (never started, so no herdr or git I/O + * happens) and pins each half separately, plus the {@code reaper == null} branch: one invariant + * wired at three places needs three assertions, not one combined check whose non-zero total could + * hide a gap at any single place. + */ +class FleetdLoopHealthSourceWiringTest { + + @Test + @DisplayName("the statusPoller half reports the real poller's health, not a hardcoded state") + void statusPollerHalfReflectsThePollersRealHealth() { + // Stopped without ever being started — stop() still marks the watchdog STOPPED. A poller + // that has never reported RUNNING is the discriminating case: if Fleetd.loopHealthSource + // ever hardcoded RUNNING (the exact mutation this test exists to catch), this would fail. + StatusPoller stoppedPoller = freshPoller(); + stoppedPoller.stop(); + SessionReaper unusedReaper = freshReaper(); // present only to satisfy the signature + + FleetMcp.LoopHealthSource source = Fleetd.loopHealthSource(stoppedPoller, unusedReaper); + + assertEquals(LoopWatchdog.State.STOPPED, source.statusPoller().get(), + "the statusPoller supplier must delegate to the real poller's health() — " + + "replacing poller::health with a constant () -> RUNNING at the " + + "Fleetd.loopHealthSource call site must fail this assertion"); + } + + @Test + @DisplayName("the sessionReaper half reports the real reaper's health, not a hardcoded state") + void sessionReaperHalfReflectsTheReapersRealHealth() { + StatusPoller unusedPoller = freshPoller(); // present only to satisfy the signature + SessionReaper stoppedReaper = freshReaper(); + stoppedReaper.stop(); + + FleetMcp.LoopHealthSource source = Fleetd.loopHealthSource(unusedPoller, stoppedReaper); + + assertEquals(LoopWatchdog.State.STOPPED, source.sessionReaper().get(), + "the sessionReaper supplier must delegate to the real reaper's health() — " + + "replacing reaper.health() with a constant at the " + + "Fleetd.loopHealthSource call site must fail this assertion"); + } + + @Test + @DisplayName("a null reaper (idle ttl not configured) still reports STOPPED, not a crash") + void nullReaperStillReportsStopped() { + // SessionReaper is only constructed when lifecycle.idleTtlSeconds is configured (see the + // `reaper` local in Fleetd.main) — a real deployment routinely passes null here. That null + // check is real behaviour, not a simplification to delete: it must keep reporting STOPPED + // rather than throwing a NullPointerException on the first fleet_list/healthz call. + StatusPoller runningPoller = freshPoller(); + + FleetMcp.LoopHealthSource source = Fleetd.loopHealthSource(runningPoller, null); + + assertEquals(LoopWatchdog.State.STOPPED, source.sessionReaper().get(), + "reaper == null must still report STOPPED, exactly like an intentionally-stopped " + + "reaper would — do not delete this null check to simplify the wiring"); + } + + /** Never started, so no herdr call is ever made; freshly constructed reports RUNNING. */ + private static StatusPoller freshPoller() { + AgentControl agents = new AgentControl(new FakeHerdr()); + return new StatusPoller(agents, new Injector(agents), 1000); + } + + /** Never started, so no git/session I/O is ever made; freshly constructed reports RUNNING. */ + private static SessionReaper freshReaper() { + return new SessionReaper(new SessionManager(new NeverSpawnsLauncher()), 60, 1000); + } + + /** + * Same minimal shape as {@code FleetdBackendErrorSinkTest.NeverSpawnsLauncher} — every method + * throws or returns an empty/no-op value, since a {@link SessionReaper} that is only ever + * constructed and then stopped (never started) never calls any of them. + */ + private static final class NeverSpawnsLauncher implements PeerLauncher { + @Override + public Set capabilities() { + return Set.of(); + } + + @Override + public Set capabilitiesFor(String profileName) { + return Set.of(); + } + + @Override + public PeerHandle spawn(SpawnRequest req) { + throw new UnsupportedOperationException("not reachable — this test never acquires a session"); + } + + @Override + public PeerHandle spawn(SpawnRequest req, PlacementDecision decision) { + throw new UnsupportedOperationException("not reachable — this test never acquires a session"); + } + + @Override + public Set profiles() { + return Set.of(); + } + + @Override + public String defaultProfile() { + return null; + } + + @Override + public String effectiveCwd(SpawnRequest req) { + throw new UnsupportedOperationException("not reachable — this test never acquires a session"); + } + + @Override + public List parityOverlay(String profileName) { + return List.of(); + } + + @Override + public List list() { + return List.of(); + } + + @Override + public int reapOrphanWorkers() { + return 0; + } + + @Override + public void stop(String id) { + } + + @Override + public boolean clearContext(String id) { + return false; + } + } +}