From 034e17bb32098235c2e5f909a8469f830336a9eb Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Sat, 12 Sep 2026 18:10:31 +0700 Subject: [PATCH] fleetd #561 follow-up: pin the session half of bothMustRunKeepingSecondResult MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two helpers maintain one invariant (the second callback half always runs, even when the first throws): bothMustRun and bothMustRunKeepingSecondResult. Only bothMustRun's "session half still runs" direction was asserted (sessionHalfStillRunsWhenTheCompletionHalfThrowsSynchronously, via onTurnComplete). bothMustRunKeepingSecondResult — the helper onTurnCompleteWithPostAction uses — could be reverted to the pre-#561 broken shape and the suite stayed green. Adds two tests to FleetdTurnListenerCompositionTest: - sessionHalfStillRunsWhenTheCompletionHalfThrowsSynchronouslyForPostAction: mirrors the existing onTurnComplete case for onTurnCompleteWithPostAction/ bothMustRunKeepingSecondResult. - bothFailuresEscapeWhenBothHalvesThrowDistinctExceptions: proves a second, distinct failure from the session half is preserved via addSuppressed rather than silently dropped when both halves of bothMustRun throw. Also rewords the onDelivered comment in Fleetd.turnListener: it previously said this pair is safe because registration survives a throw via #556's Injector wiring, which is true but is not why THIS pair is unguarded. CompletionResolver.captureBaseline already catches RuntimeException around its scrape read and fails open, so completion.onDelivered does not realistically throw. Comment text only, no logic change. --- .../src/main/java/dev/ltms/fleet/Fleetd.java | 8 +-- .../FleetdTurnListenerCompositionTest.java | 53 +++++++++++++++++++ 2 files changed, 58 insertions(+), 3 deletions(-) diff --git a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java index 1791899..997c12f 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java +++ b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java @@ -1124,9 +1124,11 @@ public final class Fleetd { @Override public void onDelivered(String target, dev.ltms.fleet.msg.TurnToken token) { - // fleetd #556/#561: registration already survives a throw here (it is wired - // directly to completion::register at the Injector call site, not folded into this - // fan-out) — this pair is out of #561's scope. Left exactly as before. + // fleetd #561: left unguarded on purpose, not because registration survives a + // throw elsewhere. completion.onDelivered runs CompletionResolver.captureBaseline, + // which already wraps its scrape read in its own catch (RuntimeException) and + // fails open (baseline = null) — so this call does not realistically throw, and + // there is nothing here for bothMustRun to protect. completion.onDelivered(target, token); sessions.onDelivered(target, token); } diff --git a/fleetd/src/test/java/dev/ltms/fleet/FleetdTurnListenerCompositionTest.java b/fleetd/src/test/java/dev/ltms/fleet/FleetdTurnListenerCompositionTest.java index 427ec26..207aee3 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/FleetdTurnListenerCompositionTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/FleetdTurnListenerCompositionTest.java @@ -230,4 +230,57 @@ class FleetdTurnListenerCompositionTest { assertTrue(sessions.called.contains("onTurnComplete"), "the session half must still have run even though the completion half threw first"); } + + /** + * The mirror case above only exercises {@code onTurnComplete}/{@code bothMustRun}. {@code + * onTurnCompleteWithPostAction} is composed through the OTHER helper, + * {@code bothMustRunKeepingSecondResult}, and nothing previously asserted that its session half + * still runs when its completion half throws — that helper could be reverted to the pre-#561 + * broken shape (run the session half only if the completion half did not throw) and the suite + * would still stay green. Uses the same real, non-fabricated trigger as the test above: a + * {@code null} target makes {@code resolveBeforePostAction}'s {@code inFlight.get(target)} + * throw a {@link NullPointerException} before {@code resolve} is ever entered. + */ + @Test + void sessionHalfStillRunsWhenTheCompletionHalfThrowsSynchronouslyForPostAction() { + FakeHerdr herdr = new FakeHerdr(); + Rendezvous rendezvous = new Rendezvous(); + CompletionResolver completion = newResolver(herdr, rendezvous); + + RecordingSessions sessions = new RecordingSessions(); // throws from nothing + TurnListener composed = Fleetd.turnListener(completion, sessions); + + assertThrows(NullPointerException.class, () -> composed.onTurnCompleteWithPostAction(null), + "ConcurrentHashMap.get(null) inside CompletionResolver.resolveBeforePostAction must " + + "still escape the composed listener"); + assertTrue(sessions.called.contains("onTurnCompleteWithPostAction"), + "the session half must still have run even though the completion half threw first"); + } + + /** + * {@code bothMustRun} must not just run both halves — it must not DROP a second failure when + * both halves throw. Forces the completion half to throw (the same real {@code + * inFlight.get(null)} NullPointerException trigger used above) while the session half throws a + * distinct {@link IllegalStateException}, and asserts the completion half's throwable is what + * escapes while the session half's throwable survives as a suppressed exception rather than + * being silently discarded. + */ + @Test + void bothFailuresEscapeWhenBothHalvesThrowDistinctExceptions() { + FakeHerdr herdr = new FakeHerdr(); + Rendezvous rendezvous = new Rendezvous(); + CompletionResolver completion = newResolver(herdr, rendezvous); + + RecordingSessions sessions = new RecordingSessions("onTurnComplete"); + TurnListener composed = Fleetd.turnListener(completion, sessions); + + NullPointerException thrown = assertThrows(NullPointerException.class, + () -> composed.onTurnComplete(null), + "the completion half's throw (NPE from inFlight.get(null)) must be what escapes"); + assertTrue(sessions.called.contains("onTurnComplete"), "the session half must still have run"); + assertEquals(1, thrown.getSuppressed().length, + "the session half's distinct failure must be recorded as suppressed, not dropped"); + assertEquals(IllegalStateException.class, thrown.getSuppressed()[0].getClass()); + assertEquals("boom: sessions.onTurnComplete", thrown.getSuppressed()[0].getMessage()); + } }