fleetd #632: tear down the three assembly behavioural tests' background loops

Each of FleetdAssemblyHealthFailTargetBehaviouralTest,
FleetdAssemblyReleaseCleanupBehaviouralTest and
FleetdAssemblyRequireOperatorConfirmBehaviouralTest called
FleetdAssembly.assembleAndStart without ever tearing it down: no close(),
no shutdownHook, no @AfterEach, no finally. Surefire runs the whole suite
in one JVM fork, so every scheduler/loop these tests started kept running
for the rest of the suite.

Capture the shutdown hook in each test's fake ResourcePorts (the existing
pattern from FleetdAssemblyLifecycleTest et al.) and run it in a finally
block, on the failure path too. RequireOperatorConfirmBehaviouralTest
assembles twice in one method, so assembleHeartbeat now returns both the
loop and its ResourcePorts so each assembly gets its own teardown.

Proof the teardown actually runs: assert ports.herdr.closed after running
the hook (FakeHerdr.close() only flips that flag from inside the real
close chain). Verified the assertion is load-bearing by temporarily
removing one shutdownHook.run() call and confirming the test then fails.

No assertion, test name or reflection changed. Diff is test-only.

fleetd/src/test/java/dev/ltms/fleet/FleetdAssemblyHealthFailTargetBehaviouralTest.java

@@ -52,6 +52,7 @@ class FleetdAssemblyHealthFailTargetBehaviouralTest {
 
     private static final class RecordingResourcePorts implements ResourcePorts {
         final FakeHerdr herdr = new FakeHerdr();
+        Runnable shutdownHook;
 
         @Override
         public Map<String, String> environment() {
@@ -94,6 +95,7 @@ class FleetdAssemblyHealthFailTargetBehaviouralTest {
 
         @Override
         public void addShutdownHook(Runnable hook) {
+            this.shutdownHook = hook;
         }
 
         @Override
@@ -127,39 +129,52 @@ class FleetdAssemblyHealthFailTargetBehaviouralTest {
         RecordingResourcePorts ports = new RecordingResourcePorts();
 
         FleetdRuntime runtime = FleetdAssembly.assembleAndStart(new AssemblyInputs(cfg, config, guard), ports);
+        assertNotNull(ports.shutdownHook, "FleetdAssembly must have registered a shutdown hook");
 
-        FleetHealthMonitor healthMonitor = runtime.healthMonitor();
-        assertNotNull(healthMonitor, "health.enabled: true in this test's config, so "
-                + "FleetdAssembly.assembleAndStart must have built a real FleetHealthMonitor");
+        // Surefire runs the whole suite in one JVM fork, so the scheduler/loops this assembly starts
+        // (SessionReaper, StatusPoller, the health monitor) must be torn down here, on the failure
+        // path too — hence the try/finally, not just a statement at the end of the happy path.
+        try {
+            FleetHealthMonitor healthMonitor = runtime.healthMonitor();
+            assertNotNull(healthMonitor, "health.enabled: true in this test's config, so "
+                    + "FleetdAssembly.assembleAndStart must have built a real FleetHealthMonitor");
 
-        Field field = FleetHealthMonitor.class.getDeclaredField("failTarget");
-        field.setAccessible(true);
-        BiConsumer<String, String> failTarget = (BiConsumer<String, String>) field.get(healthMonitor);
-        assertNotNull(failTarget, "FleetHealthMonitor's failTarget must never be null — the "
-                + "constructor itself requires it");
+            Field field = FleetHealthMonitor.class.getDeclaredField("failTarget");
+            field.setAccessible(true);
+            BiConsumer<String, String> failTarget = (BiConsumer<String, String>) field.get(healthMonitor);
+            assertNotNull(failTarget, "FleetHealthMonitor's failTarget must never be null — the "
+                    + "constructor itself requires it");
 
-        MessageService messages = runtime.messages();
+            MessageService messages = runtime.messages();
 
-        // --- loud control: prove the assembled MessageService is actually wired up and a ticket is
-        // genuinely PENDING before failTarget ever runs. If this fails, the test below would pass
-        // vacuously on a MessageService that never got a ticket in the first place. TARGET has no
-        // live agent behind it (no session was ever acquired), so nothing resolves this ticket on
-        // its own — it stays PENDING until failTarget (or a timeout) ends it.
-        String ticket = messages.sendAsync(TARGET, "long task");
-        MessageService.TaskView before = messages.poll(ticket);
-        assertEquals(MessageService.Phase.PENDING, before.phase(),
-                "control: the async ticket must be PENDING before failTarget runs");
+            // --- loud control: prove the assembled MessageService is actually wired up and a ticket is
+            // genuinely PENDING before failTarget ever runs. If this fails, the test below would pass
+            // vacuously on a MessageService that never got a ticket in the first place. TARGET has no
+            // live agent behind it (no session was ever acquired), so nothing resolves this ticket on
+            // its own — it stays PENDING until failTarget (or a timeout) ends it.
+            String ticket = messages.sendAsync(TARGET, "long task");
+            MessageService.TaskView before = messages.poll(ticket);
+            assertEquals(MessageService.Phase.PENDING, before.phase(),
+                    "control: the async ticket must be PENDING before failTarget runs");
 
-        failTarget.accept(TARGET, "member unreachable (health monitor)");
+            failTarget.accept(TARGET, "member unreachable (health monitor)");
 
-        MessageService.TaskView after = awaitTerminal(messages, ticket);
-        assertEquals(MessageService.Phase.FAILED, after.phase(),
-                "FleetdAssembly.java:429 must pass Fleetd.healthFailTarget(messages) built from the "
-                        + "SAME assembled MessageService — a no-op BiConsumer at that call site leaves "
-                        + "this ticket PENDING for the full 30-minute async timeout instead of failing it");
-        assertTrue(after.detail() != null && after.detail().contains("member unreachable"),
-                "the failure reason passed to failTarget.accept must reach MessageService.abandon and "
-                        + "end up in the ticket's detail");
+            MessageService.TaskView after = awaitTerminal(messages, ticket);
+            assertEquals(MessageService.Phase.FAILED, after.phase(),
+                    "FleetdAssembly.java:429 must pass Fleetd.healthFailTarget(messages) built from the "
+                            + "SAME assembled MessageService — a no-op BiConsumer at that call site leaves "
+                            + "this ticket PENDING for the full 30-minute async timeout instead of failing it");
+            assertTrue(after.detail() != null && after.detail().contains("member unreachable"),
+                    "the failure reason passed to failTarget.accept must reach MessageService.abandon and "
+                            + "end up in the ticket's detail");
+        } finally {
+            // Proof the teardown actually ran, not just an assurance that a finally was added: the
+            // captured shutdown hook's close order (FleetdAssemblyLifecycleTest) closes the herdr
+            // client last, so ports.herdr.closed flips to true only if this hook really executed.
+            ports.shutdownHook.run();
+            assertTrue(ports.herdr.closed, "the captured shutdown hook must have run and closed herdr — "
+                    + "proof this test's assembled background loops/scheduler were torn down");
+        }
     }
 
     private static MessageService.TaskView awaitTerminal(MessageService messages, String ticket)

fleetd/src/test/java/dev/ltms/fleet/FleetdAssemblyReleaseCleanupBehaviouralTest.java

@@ -60,6 +60,7 @@ class FleetdAssemblyReleaseCleanupBehaviouralTest {
 
     private static final class RecordingResourcePorts implements ResourcePorts {
         final FakeHerdr herdr = new FakeHerdr();
+        Runnable shutdownHook;
 
         @Override
         public Map<String, String> environment() {
@@ -102,6 +103,7 @@ class FleetdAssemblyReleaseCleanupBehaviouralTest {
 
         @Override
         public void addShutdownHook(Runnable hook) {
+            this.shutdownHook = hook;
         }
 
         @Override
@@ -132,68 +134,81 @@ class FleetdAssemblyReleaseCleanupBehaviouralTest {
         RecordingResourcePorts ports = new RecordingResourcePorts();
 
         FleetdRuntime runtime = FleetdAssembly.assembleAndStart(new AssemblyInputs(cfg, config, guard), ports);
+        assertNotNull(ports.shutdownHook, "FleetdAssembly must have registered a shutdown hook");
 
-        // --- reach into SessionManager's private release-listener list. idleSleepGuard.enabled:
-        // false above means FleetdAssembly.java:228 never registers, so this list must hold EXACTLY
-        // the one listener :447 registers.
-        Field listenersField = SessionManager.class.getDeclaredField("releaseListeners");
-        listenersField.setAccessible(true);
-        List<Consumer<SessionManager.ReleaseDetail>> releaseListeners =
-                (List<Consumer<SessionManager.ReleaseDetail>>) listenersField.get(runtime.sessions());
-        assertEquals(1, releaseListeners.size(), "control: with idleSleepGuard.enabled: false, "
-                + "FleetdAssembly.java:447 must be the ONLY onRelease registration — a different "
-                + "count means this test is no longer isolating the call site it claims to pin");
-        Consumer<SessionManager.ReleaseDetail> releaseListener = releaseListeners.get(0);
+        // Surefire runs the whole suite in one JVM fork, so the scheduler/loops this assembly starts
+        // must be torn down here, on the failure path too — hence the try/finally, not just a
+        // statement at the end of the happy path.
+        try {
+            // --- reach into SessionManager's private release-listener list. idleSleepGuard.enabled:
+            // false above means FleetdAssembly.java:228 never registers, so this list must hold EXACTLY
+            // the one listener :447 registers.
+            Field listenersField = SessionManager.class.getDeclaredField("releaseListeners");
+            listenersField.setAccessible(true);
+            List<Consumer<SessionManager.ReleaseDetail>> releaseListeners =
+                    (List<Consumer<SessionManager.ReleaseDetail>>) listenersField.get(runtime.sessions());
+            assertEquals(1, releaseListeners.size(), "control: with idleSleepGuard.enabled: false, "
+                    + "FleetdAssembly.java:447 must be the ONLY onRelease registration — a different "
+                    + "count means this test is no longer isolating the call site it claims to pin");
+            Consumer<SessionManager.ReleaseDetail> releaseListener = releaseListeners.get(0);
 
-        MessageService messages = runtime.messages();
-        ReplyInbox replyInbox = runtime.replyInbox();
+            MessageService messages = runtime.messages();
+            ReplyInbox replyInbox = runtime.replyInbox();
 
-        // primaryRegistry is never exposed by FleetdRuntime directly — ReplyPushLoop is the other
-        // collaborator FleetdAssembly.java:382 hands the SAME instance to, so reach it from there.
-        Field primaryRegistryField = ReplyPushLoop.class.getDeclaredField("primaryRegistry");
-        primaryRegistryField.setAccessible(true);
-        PrimaryRegistry primaryRegistry = (PrimaryRegistry) primaryRegistryField.get(runtime.pushLoop());
-        assertNotNull(primaryRegistry, "control: the assembled ReplyPushLoop must hold a real "
-                + "PrimaryRegistry instance");
+            // primaryRegistry is never exposed by FleetdRuntime directly — ReplyPushLoop is the other
+            // collaborator FleetdAssembly.java:382 hands the SAME instance to, so reach it from there.
+            Field primaryRegistryField = ReplyPushLoop.class.getDeclaredField("primaryRegistry");
+            primaryRegistryField.setAccessible(true);
+            PrimaryRegistry primaryRegistry = (PrimaryRegistry) primaryRegistryField.get(runtime.pushLoop());
+            assertNotNull(primaryRegistry, "control: the assembled ReplyPushLoop must hold a real "
+                    + "PrimaryRegistry instance");
 
-        // --- loud controls: set up the "before" state each collaborator's effect is measured
-        // against, against the REAL assembled objects. If any of these three fails, the test below
-        // would pass vacuously because the subject it claims to observe never existed in the first
-        // place.
-        String ticket = messages.sendAsync(TARGET, "long task");
-        assertEquals(MessageService.Phase.PENDING, messages.poll(ticket).phase(),
-                "control: the async ticket must be PENDING before the release listener runs");
+            // --- loud controls: set up the "before" state each collaborator's effect is measured
+            // against, against the REAL assembled objects. If any of these three fails, the test below
+            // would pass vacuously because the subject it claims to observe never existed in the first
+            // place.
+            String ticket = messages.sendAsync(TARGET, "long task");
+            assertEquals(MessageService.Phase.PENDING, messages.poll(ticket).phase(),
+                    "control: the async ticket must be PENDING before the release listener runs");
 
-        replyInbox.own(TARGET);
-        replyInbox.publish(TARGET, "msg-1", "hello");
-        assertEquals(1, replyInbox.peek(TARGET).size(),
-                "control: the reply inbox must own TARGET and hold one message before the release "
-                        + "listener runs");
+            replyInbox.own(TARGET);
+            replyInbox.publish(TARGET, "msg-1", "hello");
+            assertEquals(1, replyInbox.peek(TARGET).size(),
+                    "control: the reply inbox must own TARGET and hold one message before the release "
+                            + "listener runs");
 
-        primaryRegistry.recordDelegation(TARGET, "lead-1");
-        assertEquals("lead-1", primaryRegistry.nudgeTargetFor(TARGET).orElse(null),
-                "control: the delegation must be recorded before the release listener runs");
+            primaryRegistry.recordDelegation(TARGET, "lead-1");
+            assertEquals("lead-1", primaryRegistry.nudgeTargetFor(TARGET).orElse(null),
+                    "control: the delegation must be recorded before the release listener runs");
 
-        // --- the one call under test: invoke the REAL, assembled release listener directly, the
-        // same way SessionManager.release(...) would on a real teardown.
-        releaseListener.accept(new SessionManager.ReleaseDetail(TARGET, null, null, null, null));
+            // --- the one call under test: invoke the REAL, assembled release listener directly, the
+            // same way SessionManager.release(...) would on a real teardown.
+            releaseListener.accept(new SessionManager.ReleaseDetail(TARGET, null, null, null, null));
 
-        MessageService.TaskView after = awaitTerminal(messages, ticket);
-        assertEquals(MessageService.Phase.FAILED, after.phase(),
-                "FleetdAssembly.java:447 must register a listener that calls messages.abandon(...) "
-                        + "on the SAME assembled MessageService — an inert listener leaves this "
-                        + "ticket PENDING for the full 30-minute async timeout");
-        assertTrue(after.detail() != null && after.detail().contains("released"),
-                "the abandon reason must say the worker session was released");
+            MessageService.TaskView after = awaitTerminal(messages, ticket);
+            assertEquals(MessageService.Phase.FAILED, after.phase(),
+                    "FleetdAssembly.java:447 must register a listener that calls messages.abandon(...) "
+                            + "on the SAME assembled MessageService — an inert listener leaves this "
+                            + "ticket PENDING for the full 30-minute async timeout");
+            assertTrue(after.detail() != null && after.detail().contains("released"),
+                    "the abandon reason must say the worker session was released");
 
-        assertTrue(replyInbox.peek(TARGET).isEmpty(),
-                "FleetdAssembly.java:447 must register a listener that calls replyInbox.release(...) "
-                        + "— an inert listener leaves the inbox still owning TARGET with its message");
+            assertTrue(replyInbox.peek(TARGET).isEmpty(),
+                    "FleetdAssembly.java:447 must register a listener that calls replyInbox.release(...) "
+                            + "— an inert listener leaves the inbox still owning TARGET with its message");
 
-        assertTrue(primaryRegistry.nudgeTargetFor(TARGET).isEmpty(),
-                "FleetdAssembly.java:447 must register a listener that calls "
-                        + "primaryRegistry.forgetDelegation(...) — an inert listener leaves the stale "
-                        + "delegation in place");
+            assertTrue(primaryRegistry.nudgeTargetFor(TARGET).isEmpty(),
+                    "FleetdAssembly.java:447 must register a listener that calls "
+                            + "primaryRegistry.forgetDelegation(...) — an inert listener leaves the stale "
+                            + "delegation in place");
+        } finally {
+            // Proof the teardown actually ran, not just an assurance that a finally was added: the
+            // captured shutdown hook's close order (FleetdAssemblyLifecycleTest) closes the herdr
+            // client last, so ports.herdr.closed flips to true only if this hook really executed.
+            ports.shutdownHook.run();
+            assertTrue(ports.herdr.closed, "the captured shutdown hook must have run and closed herdr — "
+                    + "proof this test's assembled background loops/scheduler were torn down");
+        }
     }
 
     private static MessageService.TaskView awaitTerminal(MessageService messages, String ticket)

fleetd/src/test/java/dev/ltms/fleet/FleetdAssemblyRequireOperatorConfirmBehaviouralTest.java

@@ -56,6 +56,7 @@ class FleetdAssemblyRequireOperatorConfirmBehaviouralTest {
 
     private static final class RecordingResourcePorts implements ResourcePorts {
         final FakeHerdr herdr = new FakeHerdr();
+        Runnable shutdownHook;
 
         @Override
         public Map<String, String> environment() {
@@ -98,6 +99,7 @@ class FleetdAssemblyRequireOperatorConfirmBehaviouralTest {
 
         @Override
         public void addShutdownHook(Runnable hook) {
+            this.shutdownHook = hook;
         }
 
         @Override
@@ -124,18 +126,25 @@ class FleetdAssemblyRequireOperatorConfirmBehaviouralTest {
         return FleetConfig.load(f);
     }
 
-    private static LeadHeartbeatLoop assembleHeartbeat(Path dir, boolean requireOperatorConfirm) throws Exception {
+    /** Carries both the assembled loop under test AND its {@link RecordingResourcePorts}, so the
+     * caller can tear the assembly down (this test assembles the real daemon TWICE — see the class
+     * javadoc — and each assembly needs its own teardown, not just the last one). */
+    private record Assembled(LeadHeartbeatLoop heartbeat, RecordingResourcePorts ports) {
+    }
+
+    private static Assembled assembleHeartbeat(Path dir, boolean requireOperatorConfirm) throws Exception {
         FleetConfig cfg = writeConfig(dir, requireOperatorConfirm);
         ConfigRef config = new ConfigRef(dir.resolve("fleetd.yaml"), cfg);
         SubscriptionGuard guard = new SubscriptionGuard(cfg.guard().hostSet());
         RecordingResourcePorts ports = new RecordingResourcePorts();
 
         FleetdRuntime runtime = FleetdAssembly.assembleAndStart(new AssemblyInputs(cfg, config, guard), ports);
+        assertNotNull(ports.shutdownHook, "FleetdAssembly must have registered a shutdown hook");
 
         LeadHeartbeatLoop heartbeat = runtime.heartbeat();
         assertNotNull(heartbeat, "control: leadHeartbeat: is configured, so FleetdAssembly.assembleAndStart "
                 + "must have built a real LeadHeartbeatLoop");
-        return heartbeat;
+        return new Assembled(heartbeat, ports);
     }
 
     /** Pulls the REAL {@code requireOperatorConfirm} field off the REAL, assembled loop. */
@@ -161,39 +170,60 @@ class FleetdAssemblyRequireOperatorConfirmBehaviouralTest {
         LeadContextGauge.Reading highReading = new LeadContextGauge.Reading(LeadContextGauge.State.HIGH,
                 250_000L, 2);
 
+        String noticeFalse;
+        String noticeTrue;
+
         // --- direction 1: requireOperatorConfirm: false -----------------------------------------
         Path falseDir = dir.resolve("false");
         Files.createDirectories(falseDir);
-        LeadHeartbeatLoop heartbeatFalse = assembleHeartbeat(falseDir, false);
-        boolean fieldFalse = assembledRequireOperatorConfirm(heartbeatFalse);
-        assertFalse(fieldFalse, "FleetdAssembly.java:402/:409 must thread leadRollover."
-                + "requireOperatorConfirm: false into the assembled LeadHeartbeatLoop's own field — "
-                + "dropping the 14th constructor argument selects the 13-argument overload, which "
-                + "hardcodes true regardless of config (fleetd #621), and this would read true instead");
+        Assembled assembledFalse = assembleHeartbeat(falseDir, false);
+        // Surefire runs the whole suite in one JVM fork, so each assembly's scheduler/loops must be
+        // torn down here, on the failure path too — hence try/finally per assembly (this test
+        // assembles TWICE, so both need their own teardown, not just the last one).
+        try {
+            boolean fieldFalse = assembledRequireOperatorConfirm(assembledFalse.heartbeat());
+            assertFalse(fieldFalse, "FleetdAssembly.java:402/:409 must thread leadRollover."
+                    + "requireOperatorConfirm: false into the assembled LeadHeartbeatLoop's own field — "
+                    + "dropping the 14th constructor argument selects the 13-argument overload, which "
+                    + "hardcodes true regardless of config (fleetd #621), and this would read true instead");
 
-        String noticeFalse = contextNotice(true, highReading, false, fieldFalse);
-        assertTrue(noticeFalse.contains("Decide for yourself when to confirm"),
-                "with requireOperatorConfirm: false, the assembled loop's own notice must tell the "
-                        + "lead it can decide for itself — got: " + noticeFalse);
-        assertFalse(noticeFalse.contains("ask the operator") || noticeFalse.contains("Only the operator"),
-                "with requireOperatorConfirm: false, the assembled loop's own notice must NOT ask the "
-                        + "operator — got: " + noticeFalse);
+            noticeFalse = contextNotice(true, highReading, false, fieldFalse);
+            assertTrue(noticeFalse.contains("Decide for yourself when to confirm"),
+                    "with requireOperatorConfirm: false, the assembled loop's own notice must tell the "
+                            + "lead it can decide for itself — got: " + noticeFalse);
+            assertFalse(noticeFalse.contains("ask the operator") || noticeFalse.contains("Only the operator"),
+                    "with requireOperatorConfirm: false, the assembled loop's own notice must NOT ask the "
+                            + "operator — got: " + noticeFalse);
+        } finally {
+            // Proof the teardown actually ran, not just an assurance that a finally was added: the
+            // captured shutdown hook's close order (FleetdAssemblyLifecycleTest) closes the herdr
+            // client last, so ports.herdr.closed flips to true only if this hook really executed.
+            assembledFalse.ports().shutdownHook.run();
+            assertTrue(assembledFalse.ports().herdr.closed, "the captured shutdown hook must have run "
+                    + "and closed herdr — proof this assembly's background loops/scheduler were torn down");
+        }
 
         // --- direction 2: requireOperatorConfirm: true -------------------------------------------
         Path trueDir = dir.resolve("true");
         Files.createDirectories(trueDir);
-        LeadHeartbeatLoop heartbeatTrue = assembleHeartbeat(trueDir, true);
-        boolean fieldTrue = assembledRequireOperatorConfirm(heartbeatTrue);
-        assertTrue(fieldTrue, "FleetdAssembly.java:402/:409 must thread leadRollover."
-                + "requireOperatorConfirm: true into the assembled LeadHeartbeatLoop's own field");
+        Assembled assembledTrue = assembleHeartbeat(trueDir, true);
+        try {
+            boolean fieldTrue = assembledRequireOperatorConfirm(assembledTrue.heartbeat());
+            assertTrue(fieldTrue, "FleetdAssembly.java:402/:409 must thread leadRollover."
+                    + "requireOperatorConfirm: true into the assembled LeadHeartbeatLoop's own field");
 
-        String noticeTrue = contextNotice(true, highReading, false, fieldTrue);
-        assertTrue(noticeTrue.contains("ask the operator") && noticeTrue.contains("Only the operator can approve the roll"),
-                "with requireOperatorConfirm: true, the assembled loop's own notice must ask the "
-                        + "operator — got: " + noticeTrue);
-        assertFalse(noticeTrue.contains("Decide for yourself when to confirm"),
-                "with requireOperatorConfirm: true, the assembled loop's own notice must NOT tell "
-                        + "the lead it can decide for itself — got: " + noticeTrue);
+            noticeTrue = contextNotice(true, highReading, false, fieldTrue);
+            assertTrue(noticeTrue.contains("ask the operator") && noticeTrue.contains("Only the operator can approve the roll"),
+                    "with requireOperatorConfirm: true, the assembled loop's own notice must ask the "
+                            + "operator — got: " + noticeTrue);
+            assertFalse(noticeTrue.contains("Decide for yourself when to confirm"),
+                    "with requireOperatorConfirm: true, the assembled loop's own notice must NOT tell "
+                            + "the lead it can decide for itself — got: " + noticeTrue);
+        } finally {
+            assembledTrue.ports().shutdownHook.run();
+            assertTrue(assembledTrue.ports().herdr.closed, "the captured shutdown hook must have run "
+                    + "and closed herdr — proof this assembly's background loops/scheduler were torn down");
+        }
 
         // --- the two directions must actually differ: a constant return would pass both assertion
         // blocks above vacuously if they happened to share wording, so compare them directly too.