From 6754b4edbcadfa95ed0c97c7dbea022924c4f906 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Sun, 4 Oct 2026 20:15:34 +0200 Subject: [PATCH] fleetd #736: release clears the member's presence entry SessionManager.releaseRemoved() tore down a member's registry row and pane but never cleared it from MemberPresence, so a terminal stayed marked "present" for the daemon's lifetime after release/idle-reap/shutdown drain. Clear it in the method's unconditional finally block, alongside the other must-always-run teardown step, so every release path (explicit release, the idle reaper's releaseIfCurrent, and a shutdown drain) forgets it the same way, and a throw from the dirty-worktree check does not skip it. MemberPresence.forget(null) throws NullPointerException (verified empirically: ConcurrentHashMap.remove(null) NPEs on key.hashCode()), so the new call guards on a non-null, non-blank terminal id rather than relying on forget to no-op. --- .../ltms/fleet/session/SessionManager.java | 6 ++ .../fleet/session/SessionManagerTest.java | 99 +++++++++++++++++++ 2 files changed, 105 insertions(+) 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 3cc334b6..73f9e6d8 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/session/SessionManager.java +++ b/fleetd/src/main/java/dev/ltms/fleet/session/SessionManager.java @@ -470,6 +470,12 @@ public final class SessionManager implements TurnListener { MemberSession resolved = resolveAgentSessionId(removed, removedHandle); notifyReleased(new ReleaseDetail(resolved.terminalId(), resolved.worktree(), resolved.branch(), snapshotRef, resolved.agentSessionId())); + String terminal = removed.terminalId(); + if (terminal != null && !terminal.isBlank()) { + // Without this, a terminal stays marked present after its pane is gone, so a + // later send to the same id would read as deliverable instead of refused. + presence.forget(terminal); + } } } // CB-581: the pane must always stop, even if the dirty check above threw. A session removed 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 20f4f650..02fdd9fb 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/session/SessionManagerTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/session/SessionManagerTest.java @@ -1452,6 +1452,105 @@ class SessionManagerTest { + "dirty check threw"); } + // --- fleetd #736: a release must forget the member's presence entry, not just its registry + // row --------------------------------------------------------------------------------------- + + @Test + void releaseByPaneIdForgetsThePresenceEntry() { + FakeHerdr herdr = new FakeHerdr(); + SessionManager sessions = sessionManager(herdr); + MemberSession session = sessions.acquire("ltms-local", null, "/caller", "term_primary"); + String terminal = session.terminalId(); + sessions.asPresence().markPresent(terminal); + assertTrue(sessions.asPresence().isPresent(terminal), "present before the release"); + + sessions.release(session.paneId()); + + assertFalse(sessions.asPresence().isPresent(terminal), + "release must forget the terminal's presence, not just remove its registry row"); + } + + @Test + void reapIdleForgetsThePresenceEntryToo() { + long[] clock = {0}; + FakeHerdr herdr = new FakeHerdr(); + SessionManager sessions = sessionManager(herdr, () -> clock[0]); + MemberSession session = sessions.acquire("ltms-local", null, "/caller", "term_primary"); + String terminal = session.terminalId(); + sessions.asPresence().markPresent(terminal); + assertTrue(sessions.asPresence().isPresent(terminal), "present before the reap"); + + clock[0] = 11; + assertEquals(1, sessions.reapIdle(10), "READY session past TTL is reaped"); + + assertFalse(sessions.asPresence().isPresent(terminal), + "the idle-reap release path (releaseIfCurrent) goes through the same teardown " + + "funnel as an explicit release, so it must forget presence too"); + } + + @Test + void shutdownDrainAlsoForgetsThePresenceEntry() { + FakeHerdr herdr = new FakeHerdr(); + SessionManager sessions = sessionManager(herdr); + MemberSession session = sessions.acquire("ltms-local", null, "/caller", "term_primary"); + String terminal = session.terminalId(); + sessions.asPresence().markPresent(terminal); + assertTrue(sessions.asPresence().isPresent(terminal), "present before the drain"); + + sessions.drainAll(TimeUnit.MILLISECONDS.toNanos(100)); + + assertFalse(sessions.asPresence().isPresent(terminal), + "a shutdown drain still ends the member's process, so presence must be cleared " + + "exactly as it is for any other release cause"); + } + + @Test + void releaseOfAnUnknownPaneIdDoesNotThrow() { + FakeHerdr herdr = new FakeHerdr(); + SessionManager sessions = sessionManager(herdr); + + assertDoesNotThrow(() -> sessions.release("no-such-pane"), + "releasing a pane id that was never registered must be a no-op, not a throw"); + } + + @Test + void releaseStillForgetsPresenceWhenDirtyCheckThrows() { + FakeHerdr herdr = new FakeHerdr(); + RecordingWorktrees worktrees = new RecordingWorktrees(); + SessionManager sessions = sessionManager(herdr, worktrees); + MemberSession s = sessions.acquire("ltms-local", null, "/caller/proj", null, + new WorktreeRequest("fleetd-736", null)); + String terminal = s.terminalId(); + sessions.asPresence().markPresent(terminal); + worktrees.failHasUncommittedWith(new WorktreeException("git status exited 128")); + + assertDoesNotThrow(() -> sessions.release(s.paneId()), + "a throwing dirty check must not abort the release"); + + assertFalse(sessions.asPresence().isPresent(terminal), + "presence must be forgotten even when the dirty check throws, which pins the " + + "forget call to the finally block that runs no matter what happened above"); + } + + @Test + void releaseLeavesADifferentStillLiveMembersPresenceUntouched() { + FakeHerdr herdr = new FakeHerdr(); + SessionManager sessions = sessionManager(herdr); + MemberSession released = sessions.acquire("ltms-local", null, "/caller/a", "ownerA"); + MemberSession stillLive = sessions.acquire("ltms-local", null, "/caller/b", "ownerB"); + sessions.asPresence().markPresent(released.terminalId()); + sessions.asPresence().markPresent(stillLive.terminalId()); + assertTrue(sessions.asPresence().isPresent(stillLive.terminalId()), + "present before the release of the other member"); + + sessions.release(released.paneId()); + + assertFalse(sessions.asPresence().isPresent(released.terminalId()), + "the released terminal is forgotten"); + assertTrue(sessions.asPresence().isPresent(stillLive.terminalId()), + "a still-live member's presence must survive an unrelated release"); + } + // --- fleetd #316: the dirty check must be re-taken after the worker is stopped, not trusted // stale from before it ------------------------------------------------------------------------