diff --git a/fleetd/src/test/java/dev/ltms/fleet/FleetdAssemblyHealthFailTargetBehaviouralTest.java b/fleetd/src/test/java/dev/ltms/fleet/FleetdAssemblyHealthFailTargetBehaviouralTest.java index b2b8ee5..260f667 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/FleetdAssemblyHealthFailTargetBehaviouralTest.java +++ b/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 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 failTarget = (BiConsumer) 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 failTarget = (BiConsumer) 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) diff --git a/fleetd/src/test/java/dev/ltms/fleet/FleetdAssemblyReleaseCleanupBehaviouralTest.java b/fleetd/src/test/java/dev/ltms/fleet/FleetdAssemblyReleaseCleanupBehaviouralTest.java index e84577b..e745e10 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/FleetdAssemblyReleaseCleanupBehaviouralTest.java +++ b/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 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> releaseListeners = - (List>) 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 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> releaseListeners = + (List>) 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 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) diff --git a/fleetd/src/test/java/dev/ltms/fleet/FleetdAssemblyRequireOperatorConfirmBehaviouralTest.java b/fleetd/src/test/java/dev/ltms/fleet/FleetdAssemblyRequireOperatorConfirmBehaviouralTest.java index 3cb4680..241e258 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/FleetdAssemblyRequireOperatorConfirmBehaviouralTest.java +++ b/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 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.