diff --git a/fleetd/src/main/java/dev/ltms/fleet/lead/LeadRollover.java b/fleetd/src/main/java/dev/ltms/fleet/lead/LeadRollover.java index b5e3d3f..aefa43f 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/lead/LeadRollover.java +++ b/fleetd/src/main/java/dev/ltms/fleet/lead/LeadRollover.java @@ -27,9 +27,11 @@ import java.util.function.Supplier; * every gate ({@link #confirm}'s own checks) has passed — a deferred, single-shot continuation * clears the lead's own pane and bootstraps a fresh session against that file. * - *

This is the executor only. Nothing in this ticket wires an MCP tool onto {@link #open}/ - * {@link #confirm}/{@link #cancel} — that is a separate, later unit; until it lands, nothing calls - * this class at all. + *

This is the executor behind the {@code fleet_handover} MCP tool ({@code + * dev.ltms.fleet.mcp.FleetMcp#handover}), which drives {@link #open}, {@link #confirm}, {@link + * #cancel}, and {@link #status} from a tool call — wired in fleetd #480 Unit C. An earlier + * version of this paragraph said nothing called this class at all; that stopped being true once + * that unit landed, and this correction exists so the javadoc does not go on claiming it. * *

{@code confirm()} cannot roll inline — a fleetd #480 correction. The first * version of this class called {@code agents.send(lead, "/clear")} directly from inside {@code @@ -184,20 +186,31 @@ public final class LeadRollover { } /** - * How many finished tokens {@link #outcomes} remembers before it starts evicting the oldest — - * bounded so a long-running daemon never grows this map without limit. Chosen generously - * rather than tightly: production rolls are rare (this class's own ticket found exactly ONE - * completed roll ever logged on this host), and each entry is a handful of short strings, so - * even a full cap costs a few tens of kilobytes — nowhere near a reason to make it configurable. - * 200 entries comfortably outlasts any operator's own memory of "did that roll I asked for - * actually happen", which is the whole reason {@link #status} exists. + * How many tokens {@link #outcomes} remembers before it starts evicting the oldest — bounded + * so a long-running daemon never grows this map without limit. Chosen generously rather than + * tightly: production rolls are rare (this class's own ticket found exactly ONE completed roll + * ever logged on this host), and each entry is a handful of short strings, so even a full cap + * costs a few tens of kilobytes — nowhere near a reason to make it configurable. 200 entries + * comfortably outlasts any operator's own memory of "did that roll I asked for actually + * happen", which is the whole reason {@link #status} exists. + * + *

This cap counts {@link RollState#IN_PROGRESS} entries exactly the same as + * finished ones. There is only the one bounded map: {@link #confirm} writes an {@link + * RollState#IN_PROGRESS} entry into {@link #outcomes} at hand-off, and the deferred + * continuation later overwrites that SAME key with a terminal state — it never inserts a + * second entry. An approved roll therefore occupies one slot in this map for its entire + * lifetime, from the moment {@link #confirm} hands off, not only once it finishes; a + * confirmed-but-not-yet-finished roll counts against the cap exactly like a finished one. The + * alternative (a separate, uncapped in-flight map) would let a burst of confirmed-but-stuck + * rolls grow without bound — the exact failure this cap exists to prevent — so it was rejected. */ static final int OUTCOME_HISTORY_CAP = 200; /** * What is known about one token, right now — the answer {@link #status} gives. Distinguishes - * three terminal outcomes an approved roll can finish with, plus the two non-terminal answers: - * still pending, or nothing known about this token at all. + * three terminal outcomes an approved roll can finish with, one in-flight outcome for a roll + * that has been approved but has not finished yet, and two answers for a token that names no + * active work at all: still pending confirmation, or nothing known about this token at all. */ public enum RollState { /** @@ -206,9 +219,21 @@ public final class LeadRollover { * and left the token pending for a retry — see {@link #confirm}'s javadoc ("token stays * pending"). Indistinguishable from a genuinely fresh request; a caller wanting to know * WHICH gate most recently refused should read the {@link RollDecision} that {@link - * #confirm} itself returned, not this status. + * #confirm} itself returned, not this status. Never the state of an APPROVED + * roll — see {@link #IN_PROGRESS}, which {@link #confirm} records at the moment it + * hands off, before this token is even removed from the pending set. */ PENDING, + /** + * {@link #confirm} approved this roll and handed it to the deferred continuation, which has + * not finished yet. Recorded by {@link #confirm} itself, at hand-off — before + * {@code token} is removed from the pending set — so there is never a gap in which {@link + * #status} could wrongly answer {@link #UNKNOWN} ("nothing was ever requested") for a roll + * that is, in fact, actively running. This is not sticky: the deferred continuation + * overwrites this same entry with a terminal state ({@link #ROLLED}, {@link + * #TURN_NEVER_SETTLED}, or {@link #CLEAR_NEVER_SETTLED}) once it finishes. + */ + IN_PROGRESS, /** * {@link #confirm} was approved and the deferred continuation completed the entire roll: * the calling lead's turn settled, {@code /clear} was sent and settled, and {@code @@ -447,6 +472,15 @@ public final class LeadRollover { return docCheck; } + // Record IN_PROGRESS BEFORE removing from `pending` — see RollState#IN_PROGRESS and + // OUTCOME_HISTORY_CAP's javadoc. This ordering means `token` is written into `outcomes` + // while it is STILL present in `pending`; status() checks `outcomes` first (see that + // method), so it reports IN_PROGRESS immediately, not the brief-but-real gap a + // remove-then-put ordering would leave in which the token is in neither map. + outcomes.put(token, new RollStatus(RollState.IN_PROGRESS, + "confirm() approved this roll and handed it to the deferred continuation; it has " + + "not finished yet — still waiting for the calling turn to settle, for " + + "/clear to be sent and settle, or for bootstrapText to be sent")); pending.remove(token); log.info("lead-rollover: confirmed token={} lead={} — roll scheduled once the calling turn ends", token, callerTerminal); @@ -526,25 +560,31 @@ public final class LeadRollover { * RollState#UNKNOWN}, never a {@link NullPointerException} — {@link #pending} is a * {@link ConcurrentHashMap}, which throws on a {@code null} key lookup, so this * short-circuits before ever reaching it - * @return {@link RollState#PENDING} while {@code token} is still open (including one left - * pending by a {@link #confirm} gate refusal — see that method's javadoc); a terminal - * state once the approved roll's continuation has finished, read from the bounded - * {@link #outcomes} history; or {@link RollState#UNKNOWN} for a token never issued, - * cancelled, or aged out of that history + * @return {@link RollState#IN_PROGRESS} for an approved roll whose continuation has not + * finished yet, or a terminal state once it has (both read from {@link #outcomes} — + * checked FIRST, see below); {@link RollState#PENDING} while {@code token} is still + * open and has not yet been approved (including one left pending by a {@link #confirm} + * gate refusal — see that method's javadoc); or {@link RollState#UNKNOWN} for a token + * never issued, cancelled, or aged out of the bounded history */ public RollStatus status(String token) { if (token == null || token.isBlank()) { return new RollStatus(RollState.UNKNOWN, "no token given"); } + // `outcomes` is checked BEFORE `pending`, deliberately: `confirm` writes an IN_PROGRESS + // entry into `outcomes` before it removes `token` from `pending` (see `confirm`'s own + // comment at that call site), so for the brief window where a token is present in BOTH + // maps, this order reports the more accurate answer (IN_PROGRESS, already approved) rather + // than the stale one (PENDING, not yet approved) a pending-first check would give. + RollStatus recorded = outcomes.get(token); + if (recorded != null) { + return recorded; + } if (pending.containsKey(token)) { return new RollStatus(RollState.PENDING, "open() has been called for this token and " + "it has not yet been confirmed — or a confirm() gate check failed and left it " + "pending, so the same token may be retried once the problem is fixed"); } - RollStatus finished = outcomes.get(token); - if (finished != null) { - return finished; - } return new RollStatus(RollState.UNKNOWN, "token names no pending or finished rollover " + "request known to this instance — never issued, cancelled, or aged out of the " + "bounded history (cap=" + OUTCOME_HISTORY_CAP + ")"); 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 066d203..34b8a54 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java +++ b/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java @@ -2295,10 +2295,11 @@ public final class FleetMcp { + "'confirm' (validates every gate and — only if every one passes — schedules " + "the roll; it does NOT itself clear the pane, the roll runs once this call's " + "own turn ends), 'cancel' (drops a pending request without rolling), and " - + "'status' (read-only: what happened to a token after 'confirm' — the roll " - + "completed, the calling turn never settled within turnSettleSeconds so no " - + "/clear was ever sent, or /clear itself never settled so bootstrapText was " - + "never sent; never schedules, cancels or retries anything). Primary-only. " + + "'status' (read-only: what happened to a token after 'confirm' — still " + + "running (approved but not finished yet), the roll completed, the calling " + + "turn never settled within turnSettleSeconds so no /clear was ever sent, or " + + "/clear itself never settled so bootstrapText was never sent; never " + + "schedules, cancels or retries anything). Primary-only. " + "There is deliberately no terminal/session/leadTerminal parameter: the pane " + "to roll is always resolved from YOUR OWN connection, never a value you " + "pass, so you can only ever roll yourself — never another lead. Requires " diff --git a/fleetd/src/test/java/dev/ltms/fleet/lead/LeadRolloverTest.java b/fleetd/src/test/java/dev/ltms/fleet/lead/LeadRolloverTest.java index da39888..07d5bb8 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/lead/LeadRolloverTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/lead/LeadRolloverTest.java @@ -16,6 +16,7 @@ import org.junit.jupiter.api.io.TempDir; import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; +import java.util.ArrayList; import java.util.List; import java.util.concurrent.atomic.AtomicLong; import java.util.function.Function; @@ -1175,6 +1176,153 @@ class LeadRolloverTest { + "pending request: " + decision.reason() + " / " + decision.detail()); } + // ---- PR #600 review round 2: IN_PROGRESS — the gap between confirm() handing off and the ---- + // ---- continuation finishing must never read back as UNKNOWN ("nothing was ever requested") -- + + /** + * A {@code continuationRunner} that CAPTURES the roll instead of running it, so a test can + * observe {@link LeadRollover#status} in the window between {@link LeadRollover#confirm} + * handing off and the roll actually finishing — the window the synchronous {@code + * Runnable::run} runner used everywhere else in this class collapses to nothing. Call {@link + * #runNext()} to finish exactly one held roll, once the test is done observing the in-flight + * state. + */ + private static final class HoldingRunner implements java.util.function.Consumer { + private final List held = new ArrayList<>(); + + @Override + public void accept(Runnable runnable) { + held.add(runnable); + } + + int heldCount() { + return held.size(); + } + + /** Runs (and removes) the oldest held roll — FIFO, matching confirm() call order. */ + void runNext() { + held.remove(0).run(); + } + } + + private static LeadRollover newRolloverWithHoldingRunner(HerdrClient herdr, + FleetConfig.LeadRollover config, LongSupplier nowMillis, HoldingRunner runner) { + AgentControl agents = new AgentControl(herdr); + return new LeadRollover(agents, () -> config, _ -> null, nowMillis, () -> { }, runner); + } + + @Test + @DisplayName("[IN-PROGRESS 1] between an approved confirm() and the continuation finishing, " + + "status() reports IN_PROGRESS — not UNKNOWN, and not PENDING") + void statusReportsInProgressBetweenConfirmAndTheContinuationFinishing() throws IOException { + FakeHerdr herdr = new FakeHerdr(); // default idle — the held roll WOULD complete once run + Path handover = writeHandover("handover contents"); + AtomicLong clock = new AtomicLong(1_000); + HoldingRunner runner = new HoldingRunner(); + LeadRollover rollover = newRolloverWithHoldingRunner(herdr, cfg(handover.toString()), + fixedClock(clock), runner); + + LeadRollover.PendingRollover pending = rollover.open(LEAD, "context is full"); + LeadRollover.RollDecision decision = rollover.confirm(LEAD, pending.token(), true); + assertTrue(decision.accepted(), "expected approval; got: " + decision.reason() + " / " + decision.detail()); + assertEquals(1, runner.heldCount(), "sanity: the roll must have been handed to the " + + "continuation runner and held there, not run yet"); + assertEquals(0, promptCallCount(herdr), "sanity: the held continuation has not run, so " + + "nothing has been sent to the pane yet"); + + LeadRollover.RollStatus status = rollover.status(pending.token()); + assertEquals(LeadRollover.RollState.IN_PROGRESS, status.state(), + "a lead calling status() right after confirm() returned approved, while the roll " + + "is still running, must be told IN_PROGRESS — not UNKNOWN (\"nothing was " + + "ever requested\", which would wrongly invite it to call open() again " + + "mid-roll) and not PENDING (\"not yet approved\", which is simply false " + + "here): got " + status.state() + " / " + status.detail()); + } + + @Test + @DisplayName("[IN-PROGRESS 2] once the continuation finishes, the same token reports its " + + "terminal state — IN_PROGRESS is not sticky") + void inProgressStateIsNotStickyOnceTheContinuationFinishes() throws IOException { + FakeHerdr herdr = new FakeHerdr(); // default idle — the held roll completes once run + Path handover = writeHandover("handover contents"); + AtomicLong clock = new AtomicLong(1_000); + HoldingRunner runner = new HoldingRunner(); + LeadRollover rollover = newRolloverWithHoldingRunner(herdr, cfg(handover.toString()), + fixedClock(clock), runner); + + LeadRollover.PendingRollover pending = rollover.open(LEAD, "context is full"); + LeadRollover.RollDecision decision = rollover.confirm(LEAD, pending.token(), true); + assertTrue(decision.accepted(), "expected approval; got: " + decision.reason() + " / " + decision.detail()); + assertEquals(LeadRollover.RollState.IN_PROGRESS, rollover.status(pending.token()).state(), + "sanity: must be IN_PROGRESS before the held continuation is run"); + + runner.runNext(); // finish the held roll now + + assertEquals(2, promptCallCount(herdr), "sanity: the roll actually ran to completion " + + "once released — /clear then bootstrapText"); + LeadRollover.RollStatus after = rollover.status(pending.token()); + assertEquals(LeadRollover.RollState.ROLLED, after.state(), + "the SAME token must now report its terminal state — IN_PROGRESS must not still be " + + "reported once the roll has actually finished"); + } + + @Test + @DisplayName("[IN-PROGRESS 4] IN_PROGRESS is distinct from every other RollState") + void inProgressStateIsDistinctFromAllOtherStates() throws IOException { + FakeHerdr herdr = new FakeHerdr(); + Path handover = writeHandover("handover contents"); + AtomicLong clock = new AtomicLong(1_000); + HoldingRunner runner = new HoldingRunner(); + LeadRollover rollover = newRolloverWithHoldingRunner(herdr, cfg(handover.toString()), + fixedClock(clock), runner); + + LeadRollover.PendingRollover pending = rollover.open(LEAD, "context is full"); + LeadRollover.RollDecision decision = rollover.confirm(LEAD, pending.token(), true); + assertTrue(decision.accepted(), "expected approval; got: " + decision.reason() + " / " + decision.detail()); + + LeadRollover.RollState inProgress = rollover.status(pending.token()).state(); + assertEquals(LeadRollover.RollState.IN_PROGRESS, inProgress); + for (LeadRollover.RollState other : LeadRollover.RollState.values()) { + if (other == LeadRollover.RollState.IN_PROGRESS) { + continue; + } + assertNotEquals(other, inProgress, "IN_PROGRESS must be distinct from " + other); + } + } + + @Test + @DisplayName("[IN-PROGRESS 5] eviction counts IN_PROGRESS entries toward the cap exactly like " + + "finished ones — a burst of confirmed-but-not-yet-finished rolls still ages out") + void evictionCountsInProgressEntriesTowardTheCap() throws IOException { + FakeHerdr herdr = new FakeHerdr(); + Path handover = writeHandover("handover contents"); // reused by every roll — see + // statusHistoryDoesNotGrowPastItsCap for why one shared file is enough for freshness. + AtomicLong clock = new AtomicLong(1_000); + HoldingRunner runner = new HoldingRunner(); // nothing run below — every roll stays IN_PROGRESS + LeadRollover rollover = newRolloverWithHoldingRunner(herdr, cfg(handover.toString()), + () -> clock.addAndGet(1), runner); + + int rolls = LeadRollover.OUTCOME_HISTORY_CAP + 50; + String[] tokens = new String[rolls]; + for (int i = 0; i < rolls; i++) { + LeadRollover.PendingRollover pending = rollover.open(LEAD, "context is full #" + i); + LeadRollover.RollDecision decision = rollover.confirm(LEAD, pending.token(), true); + assertTrue(decision.accepted(), "roll #" + i + " should have been approved: " + decision.detail()); + tokens[i] = pending.token(); + } + assertEquals(rolls, runner.heldCount(), "sanity: none of these rolls have been run — every " + + "one of them is sitting in outcomes as IN_PROGRESS, not a separate uncapped map"); + + assertEquals(LeadRollover.RollState.IN_PROGRESS, rollover.status(tokens[rolls - 1]).state(), + "the most recently confirmed (still in-flight) roll must still be in the bounded " + + "history"); + assertEquals(LeadRollover.RollState.UNKNOWN, rollover.status(tokens[0]).state(), + "the oldest confirmed roll's IN_PROGRESS entry must have been evicted once the cap " + + "was exceeded, exactly like a finished entry would be — proving IN_PROGRESS " + + "entries share the SAME bounded map and count against the SAME cap, rather " + + "than living in a second, uncapped in-flight map"); + } + @Test @DisplayName("[fleetd #494 follow-up] the turn-settle timeout warn line prints the MEASURED " + "elapsed time next to the configured budget, never the configured value alone")