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.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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 ------------------------------------------------------------------------
|
||||
|
||||
|
||||
Reference in New Issue
Block a user