diff --git a/fleetd/src/main/java/dev/ltms/fleet/inject/Injector.java b/fleetd/src/main/java/dev/ltms/fleet/inject/Injector.java index fbadc91..f26c82a 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/inject/Injector.java +++ b/fleetd/src/main/java/dev/ltms/fleet/inject/Injector.java @@ -615,13 +615,26 @@ public final class Injector { // like success. onDelivered() runs only when sendError == null: nothing was delivered on // the error path, so there is nothing to register. // + // fleetd #553 (lead review, comment 16916): `sentHandled` guards ONLY the onDelivered + // re-call below, never the future completion outside this inner try. One boolean cannot + // carry both meanings — "onDelivered was called" and "the future has been dealt with" — + // because they come apart exactly when onDelivered throws PART WAY THROUGH: sentHandled + // is already true (set before the call, correctly — see the `if (sent != null)` block + // above), so a guard on the outer `if` here would skip this whole recovery, including the + // completion, and leave sent.delivered() pending forever even though the message really + // was typed into the pane and taken off the queue. So `sent != null` alone gates whether + // this target has anything to finish; `!sentHandled` gates only the onDelivered re-call + // inside. CompletableFuture.complete/completeExceptionally are idempotent — on the + // ordinary path (sentHandled == true, no throw) the `if (sent != null)` block above has + // already completed this future, so the calls below are a no-op returning false. + // // This recovery is wrapped in its own try/catch(Throwable) that swallows only ITS OWN // throwable and logs at WARN — the future is still completed either way — while the // ORIGINAL throwable from the try block above is left alone to keep unwinding out of // this method to StatusPoller's catch (Throwable), so a listener bug stays loud. - if (sent != null && !sentHandled) { + if (sent != null) { try { - if (sendError == null) { + if (!sentHandled && sendError == null) { turnListener.onDelivered(target, sent.token()); } } catch (Throwable recoveryError) { @@ -630,6 +643,8 @@ public final class Injector { + "anyway: {}", target, recoveryError.getMessage()); } finally { + // No-op (returns false) on the ordinary path, where the `if (sent != null)` block + // above already completed this future — see the comment above this block. if (sendError != null) { sent.delivered().completeExceptionally(sendError); } else { diff --git a/fleetd/src/test/java/dev/ltms/fleet/inject/InjectorTest.java b/fleetd/src/test/java/dev/ltms/fleet/inject/InjectorTest.java index cab9cfb..7164db8 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/inject/InjectorTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/inject/InjectorTest.java @@ -1153,4 +1153,48 @@ class InjectorTest { + "AFTER the call (rather than before) would leave it false here and the " + "finally backstop would call onDelivered a second time"); } + + @Test + void anOnDeliveredThrowOnTheNormalPathStillCompletesTheDeliveryFuture() { + // fleetd #553, lead review of PR #557 (ticket comment 16916): `sentHandled` must guard ONLY + // the onDelivered RE-CALL in the finally, never the future completion alongside it. Gating + // BOTH behind `!sentHandled` (the shape this test is red against) misses the one path the + // flag's own correct placement creates: `sentHandled` is set to true FIRST, before the + // onDelivered() call, inside the `if (sent != null)` block above (see the previous test — + // that placement is right and must not change). So when onDelivered() itself throws on that + // NORMAL path, `sentHandled` already reads true by the time control reaches the finally, and + // a `sent != null && !sentHandled` guard around the WHOLE recovery — completion included — + // skips it entirely. The message was typed into the target's pane and taken off the queue + // inside the monitor, same as any other delivery, so its future is left pending forever: the + // exact defect this ticket exists to close, just reached from a different throwing call. + // + // The fix splits the one flag's two jobs: `!sentHandled` keeps gating only the onDelivered + // call (so the exactly-once guarantee in the test above still holds — CompletableFuture. + // complete/completeExceptionally are idempotent, so completing unconditionally here is a + // no-op on the ordinary path, where the `if (sent != null)` block already completed it. + RuntimeException boom = new RuntimeException("boom from onDelivered on the normal path"); + TurnListener listener = new TurnListener() { + @Override + public void onTurnComplete(String target) { + } + + @Override + public void onDelivered(String target, TurnToken token) { + throw boom; + } + }; + Injector inj = new Injector(new AgentControl(herdr), listener); + CompletableFuture delivered = inj.enqueue(T, "task", TestTurnTokens.inert(T)).completion(); + + RuntimeException thrown = assertThrows(RuntimeException.class, + () -> inj.onStatus(T, AgentStatus.IDLE), // delivers "task"; onDelivered throws + "the original throwable from onDelivered must still escape onStatus"); + assertSame(boom, thrown); + + assertTrue(delivered.isDone(), + "fleetd #553: \"task\" was actually delivered — typed into the pane and taken off " + + "the queue inside the monitor — so its delivery future must be completed on " + + "every path out of onStatus, including the one where onDelivered itself is " + + "what threw. Leaving it pending here is a hang, not a fix"); + } }