From ef507bcd122d0b2642881741d09c79b7c4854b33 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Fri, 4 Sep 2026 11:23:52 +0700 Subject: [PATCH] #290: restore reapIdle's per-session guard coverage via a new launcher.stop() trigger MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #283 fixed release() to catch and log a worktree-removal failure, which closed off reapIdleCountsAllThreeSessionsWhenOnlyItsWorktreeRemovalFails as a trigger for reapIdle's own per-session try/catch (CB-581) — that test now proves a different, still-real thing (a swallowed removal failure doesn't shrink the reaped count), but the try/catch itself lost its test. Add FakeHerdr.paneCloseFailsForPane(paneId, code) so a test can make exactly one session's launcher.stop() fail while its siblings still tear down normally (paneCloseFailsWith already existed but fails every pane, which cannot isolate one session in a three-session reap). Add reapIdleSurvivesOneSessionWhoseLauncherStopFails beside the #283 test, using launcher.stop() as the trigger the ticket names, and prove it catches removal of reapIdle's try/catch: deleting the guard makes the test fail with the HerdrException propagating out of reapIdle uncaught (quoted in the PR body). --- .../java/dev/ltms/fleet/herdr/FakeHerdr.java | 25 ++++++- .../fleet/session/SessionManagerTest.java | 73 +++++++++++++++++++ 2 files changed, 94 insertions(+), 4 deletions(-) diff --git a/fleetd/src/test/java/dev/ltms/fleet/herdr/FakeHerdr.java b/fleetd/src/test/java/dev/ltms/fleet/herdr/FakeHerdr.java index 0c51083..2f8b5eb 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/herdr/FakeHerdr.java +++ b/fleetd/src/test/java/dev/ltms/fleet/herdr/FakeHerdr.java @@ -41,6 +41,7 @@ public final class FakeHerdr implements HerdrClient { private int agentPaneBusyFor = 0; private int workerTabPaneCount = 1; private String paneCloseErrorCode = null; + private final Map paneCloseErrorCodeFor = new java.util.concurrent.ConcurrentHashMap<>(); private String agentSendErrorCode = null; private boolean noPanes = false; private volatile String agentStatus = "idle"; // steady-state agent.get status @@ -86,12 +87,24 @@ public final class FakeHerdr implements HerdrClient { return this; } - /** Make {@code pane.close} fail with this herdr error code. */ + /** Make {@code pane.close} fail with this herdr error code, for every pane. */ public FakeHerdr paneCloseFailsWith(String code) { this.paneCloseErrorCode = code; return this; } + /** + * Make {@code pane.close} fail with this herdr error code, but only for the given {@code + * pane_id} — every other pane's {@code pane.close} still succeeds. Unlike {@link + * #paneCloseFailsWith}, which fails every call regardless of which pane it targets, this lets a + * test reap/release several sessions at once and make exactly one of them fail to stop, so the + * others' teardown can be asserted to proceed normally (fleetd #290). + */ + public FakeHerdr paneCloseFailsForPane(String paneId, String code) { + this.paneCloseErrorCodeFor.put(paneId, code); + return this; + } + /** * Make {@code pane.list} report no panes at all — models a second herdr daemon (CB-185) that * simply does not host the pane a {@link PaneLocator} is searching for. @@ -360,9 +373,13 @@ public final class FakeHerdr implements HerdrClient { "foreground_processes":[]}}"""); } case "pane.close" -> { - if (paneCloseErrorCode != null) { - throw new HerdrException("herdr error [" + paneCloseErrorCode + "]: pane.close failed", - paneCloseErrorCode, null); + Object paneIdParam = params instanceof java.util.Map m ? m.get("pane_id") : null; + String perPaneCode = paneIdParam == null ? null + : paneCloseErrorCodeFor.get(String.valueOf(paneIdParam)); + String code = perPaneCode != null ? perPaneCode : paneCloseErrorCode; + if (code != null) { + throw new HerdrException("herdr error [" + code + "]: pane.close failed", + code, null); } yield mapper.readTree("{\"type\":\"ok\"}"); } 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 76b2d9e..080be4c 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/session/SessionManagerTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/session/SessionManagerTest.java @@ -1069,6 +1069,79 @@ class SessionManagerTest { assertEquals(1, paneCloseCallsFor(herdr, "w9:pRoot_3"), "the third pane is stopped"); } + /** + * fleetd #290: the #283 fix above closed the one trigger this suite used for {@code + * reapIdle}'s own per-session try/catch (CB-581) — a worktree-removal failure is now caught + * and logged inside {@code release()} itself, so it never reaches {@code reapIdle}'s guard at + * all. This test restores coverage of that guard using the trigger the ticket names: {@code + * release()} calls {@code launcher.stop(paneId)} with no try/catch around it, so a failing + * {@code pane.close} propagates straight out of {@code release()} uncaught. {@link + * FakeHerdr#paneCloseFailsForPane} (added for this ticket) makes exactly the middle session's + * stop fail, while the other two still succeed, so this proves {@code reapIdle} keeps reaping + * the rest of the roster rather than aborting the whole pass. + */ + @Test + void reapIdleSurvivesOneSessionWhoseLauncherStopFails() { + long[] clock = {0}; + FakeHerdr herdr = new FakeHerdr(); + RecordingWorktrees worktrees = new RecordingWorktrees(); + SessionManager sessions = sessionManager(herdr, worktrees, () -> clock[0]); + MemberSession a = sessions.acquire("ltms-local", null, "/caller/proj", null, + new WorktreeRequest("cb-290a", null)); + MemberSession b = sessions.acquire("ltms-local", null, "/caller/proj", null, + new WorktreeRequest("cb-290b", null)); + MemberSession c = sessions.acquire("ltms-local", null, "/caller/proj", null, + new WorktreeRequest("cb-290c", null)); + sessions.asPresence().markPresent(a.terminalId()); + sessions.asPresence().markPresent(b.terminalId()); + sessions.asPresence().markPresent(c.terminalId()); + // Only the middle session's herdr pane fails to close — a and c stop normally. This is the + // trigger reapIdle's own guard is for, now that #283 closed the worktree-removal trigger. + herdr.paneCloseFailsForPane("w9:pRoot_2", "internal_error"); + + LoggerContext ctx = (LoggerContext) LoggerFactory.getILoggerFactory(); + ch.qos.logback.classic.Logger sessionLog = + (ch.qos.logback.classic.Logger) LoggerFactory.getLogger(SessionManager.class); + ListAppender appender = new ListAppender<>(); + appender.setContext(ctx); + appender.start(); + sessionLog.addAppender(appender); + sessionLog.setLevel(Level.WARN); + int reaped; + try { + clock[0] = 100; + reaped = sessions.reapIdle(10); + + String warn = appender.list.stream() + .filter(e -> e.getLevel().equals(Level.WARN)) + .map(ILoggingEvent::getFormattedMessage) + .filter(m -> m.contains("reap failed") && m.contains(b.paneId())) + .findFirst() + .orElse("no reap-failed WARN logged for the failing session"); + assertTrue(warn.contains(b.terminalId()), "the WARN names the failed session's terminal: " + warn); + } finally { + sessionLog.detachAppender(appender); + } + + assertEquals(2, reaped, + "the middle session's launcher.stop failure is not counted as reaped, but must not " + + "abort reaping the other two"); + assertTrue(sessions.get(a.paneId()).isEmpty(), "the first session is still released"); + assertTrue(sessions.get(c.paneId()).isEmpty(), + "the third session is still reached and released — proves the pass did not abort " + + "when the middle session's release() threw"); + assertTrue(sessions.get(b.paneId()).isEmpty(), + "the middle session is still deregistered — release() removes it from the registry " + + "before launcher.stop() runs, regardless of whether stop() then throws"); + assertEquals(1, paneCloseCallsFor(herdr, "w9:pRoot_1"), "the first pane is stopped"); + assertEquals(1, paneCloseCallsFor(herdr, "w9:pRoot_2"), + "the middle pane's stop was attempted, even though it failed"); + assertEquals(1, paneCloseCallsFor(herdr, "w9:pRoot_3"), "the third pane is stopped"); + assertEquals(List.of(a.worktree(), c.worktree()), worktrees.removeCalls().stream().sorted().toList(), + "the middle session's worktree removal never runs — release() throws before reaching " + + "it — while the other two, unaffected, still have theirs removed"); + } + @Test void unchangedRegressionCleanCompletedReleaseStillRemovesTheWorktree() { FakeHerdr herdr = new FakeHerdr();