fleetd #551: record the delivery attempt before the irreversible send
CI / shell-tests (pull_request) Successful in 5s
CI / contract (pull_request) Successful in 57s
CI / build (pull_request) Successful in 2m23s

The Injector wrote the delivery outcome AFTER calling AgentControl.send(),
so a HerdrException thrown from the response half of that call (herdr
already replied, or may have) was recorded as a confident NOT_DELIVERED
for text that may already be sitting in the worker's pane.

Poll the queue entry and mark it Pending.State.ATTEMPTED before send() is
called, not after. On success it is upgraded to DELIVERED; on an ordinary
failure it stays ATTEMPTED (honest uncertainty), except a herdr
*_not_found error, which the rest of this codebase already treats as a
confirmed absence and which now still writes NOT_DELIVERED.

cancellationOf gets a matching third answer (Cancellation.ATTEMPTED)
instead of folding the new state into NOT_DELIVERED, so a caller that
cancels an already-attempted delivery is told the truth too.
This commit is contained in:
Dai Ha
2026-09-12 17:43:21 +07:00
parent ba2f4d16f8
commit d83821bbce
2 changed files with 185 additions and 23 deletions
@@ -2,6 +2,7 @@ package dev.ltms.fleet.inject;
import dev.ltms.fleet.herdr.AgentControl;
import dev.ltms.fleet.herdr.AgentStatus;
import dev.ltms.fleet.herdr.HerdrException;
import dev.ltms.fleet.herdr.HerdrRouter;
import dev.ltms.fleet.msg.TurnToken;
import org.slf4j.Logger;
@@ -46,6 +47,17 @@ import java.util.stream.Collectors;
* turn — the pickup-grace path (a turn too fast to sample) unwedges the queue but does not fire
* completion, since without a sampled {@code working} there is no trustworthy "the worker just
* finished the task" signal to act on.
*
* <p><strong>Delivery honesty (fleetd #551).</strong> The one external send call
* ({@link AgentControl#send}) is irreversible, and its own response can still fail after the text
* has already reached the worker's pane — herdr replied (or may have), so the request was already
* processed, but the reply itself then failed to parse or carried an error. A queued entry is
* polled off the queue and marked {@code ATTEMPTED} <em>before</em> that call is made, not after,
* so a failure from the call itself can never be recorded as a confident {@code NOT_DELIVERED} for
* text that may already be sitting in the pane. {@code NOT_DELIVERED} stays reserved for the cases
* where nothing was ever attempted — the readiness grace expiring, {@link #drop}, or a herdr
* {@code *_not_found} error, which the rest of this codebase already treats as a confirmed absence
* rather than a merely inconclusive failure (see {@code StatusPoller}, {@code AgentControl}).
*/
public final class Injector {
@@ -217,8 +229,23 @@ public final class Injector {
/** The result of trying to remove an undelivered message from the injector. */
public enum Cancellation {
/** The message was still queued and this call removed it; the target saw nothing. */
CANCELLED,
/** The message reached the worker's pane and is confirmed delivered. */
DELIVERED,
/**
* fleetd #551: the injector called {@link AgentControl#send} for this message and that call
* threw before its outcome was known — the text may or may not have reached the pane. A
* caller reporting this to an operator must say "uncertain", not "definitely not
* delivered": reading it as a confident negative invites a resend of text that may already
* be sitting in the pane (a double delivery), which is worse than the ambiguity itself.
*/
ATTEMPTED,
/**
* The message never reached the worker's pane — nothing was ever attempted for it (the
* readiness grace expired, the target was dropped, or send failed with a herdr
* {@code *_not_found} error, which is a confirmed absence, not merely inconclusive).
*/
NOT_DELIVERED
}
@@ -241,7 +268,29 @@ public final class Injector {
/** A pending message and the future that completes when it has been delivered. */
private static final class Pending {
enum State { QUEUED, DELIVERED, NOT_DELIVERED, CANCELLED }
enum State {
/** Still in the target's queue, not yet attempted. */
QUEUED,
/**
* fleetd #551: {@link AgentControl#send} has been called for this entry and its outcome
* is not yet known — recorded BEFORE the call (see {@link #onStatus}), so a Throwable
* from send() can never leave the entry either still QUEUED or falsely marked
* NOT_DELIVERED. Upgraded to DELIVERED on success; left as ATTEMPTED on an ordinary
* failure, since reaching the catch does not prove the text never reached the pane.
*/
ATTEMPTED,
/** {@code send()} returned normally: the text is confirmed to have reached the pane. */
DELIVERED,
/**
* Confirmed — not merely inconclusive — that nothing was ever sent for this entry: the
* worker never became ready ({@link #onStatus}'s readiness-grace expiry), the target was
* dropped ({@link #drop}), or send failed with a herdr {@code *_not_found} error. Never
* written for an entry whose send() outcome is unknown; see ATTEMPTED.
*/
NOT_DELIVERED,
/** Removed from the queue by {@link #cancel} before it was ever attempted. */
CANCELLED
}
final String target;
final String text;
@@ -333,7 +382,14 @@ public final class Injector {
}
private static Cancellation cancellationOf(Pending p) {
return p.state == Pending.State.DELIVERED ? Cancellation.DELIVERED : Cancellation.NOT_DELIVERED;
// fleetd #551 (comment 17037): a two-way split on a now-three-way question folded ATTEMPTED
// into NOT_DELIVERED with no compiler error and no failing test — the exact defect this
// ticket exists to fix, one layer up. ATTEMPTED gets its own answer instead.
return switch (p.state) {
case DELIVERED -> Cancellation.DELIVERED;
case ATTEMPTED -> Cancellation.ATTEMPTED;
case QUEUED, NOT_DELIVERED, CANCELLED -> Cancellation.NOT_DELIVERED;
};
}
private static boolean isQuiescent(Target t) {
@@ -426,9 +482,18 @@ public final class Injector {
if (p != null && ready.test(target)) {
t.notReadySincePoll = 0;
t.notReadySinceMillis = 0;
// fleetd #551: poll and record BEFORE the irreversible send, not after.
// The entry comes off the queue and its state is set to ATTEMPTED here,
// unconditionally — so a Throwable escaping the send call below (caught or
// not) can never leave the entry QUEUED at the head of t.queue (the fleetd
// #546 hazard, since peek() alone would let the next onStatus round re-enter
// this block and send the same text again), and no path can write a
// confident DELIVERED or NOT_DELIVERED before we actually know which one
// happened.
t.queue.poll();
p.state = Pending.State.ATTEMPTED;
try {
agentsFor(target).send(target, p.text());
t.queue.poll();
p.state = Pending.State.DELIVERED;
t.awaitingPickup = true;
t.awaitingCompletion = true;
@@ -436,15 +501,24 @@ public final class Injector {
t.injectableSincePickup = 0;
sent = p;
} catch (Throwable e) {
// Delivery failed at herdr; drop the poisoned message and surface it
// rather than blocking the queue behind it. Catches Throwable, not just
// RuntimeException: fleetd #546 — an Error escaping this send (e.g. a
// NoClassDefFoundError, see #413) would otherwise leave the entry QUEUED
// at the head of t.queue. Line :378 peeks rather than polls, so the next
// onStatus round would re-enter this try and send the same text again,
// typing the same brief into the member's pane a second time.
t.queue.poll();
p.state = Pending.State.NOT_DELIVERED;
// fleetd #551: leave p.state == ATTEMPTED (recorded above, before the
// call) rather than downgrading it to NOT_DELIVERED here — reaching this
// catch does not prove the text never reached the pane. Three of the
// four HerdrException throw sites in HerdrCodec fire only after herdr
// has already replied (so it processed the request), and the fourth (a
// transport IOException) leaves it genuinely unknown whether herdr even
// received the bytes — see #551 comment 16867. The one exception is a
// herdr `*_not_found` error: that family is already read as "definitely
// absent, not merely inconclusive" everywhere else in this codebase
// (StatusPoller, AgentControl's own retry, WorkspaceControl,
// HerdrPeerLauncher, FleetApp, ReplyPushLoop) because it means the
// target pane/agent does not exist at all, so nothing could have been
// pasted anywhere — #551 keeps the new state consistent with that
// existing vocabulary rather than inventing a second one.
if (e instanceof HerdrException he && he.code() != null
&& he.code().endsWith("_not_found")) {
p.state = Pending.State.NOT_DELIVERED;
}
sent = p;
sendError = e;
}
@@ -720,10 +720,13 @@ class InjectorTest {
}
@Test
void anErrorFromSendRemovesTheMessageAndMarksItNotDelivered() {
void anErrorFromSendRemovesTheMessageAndMarksItAttempted() {
// fleetd #546, acceptance test 1: an Error (not a RuntimeException) escaping the send seam
// at Injector.java:383 must still be caught, the message dropped from the queue, and its
// state set to NOT_DELIVERED — never left QUEUED at the head.
// must still be caught and the message dropped from the queue, never left QUEUED at the
// head. fleetd #551 updates what it is marked: an AssertionError from the send call itself
// proves nothing about whether the text reached the pane, so it is recorded as ATTEMPTED
// (uncertain), not a confident NOT_DELIVERED — see anErrorFromSendMarksItNotDeliveredOnlyForANotFoundCode
// below for the one case that still gets NOT_DELIVERED.
ErrorOnPrompt throwing = new ErrorOnPrompt(new FakeHerdr());
Injector inj = new Injector(new AgentControl(throwing));
Injector.Delivery delivery = inj.enqueue(T, "brief", TestTurnTokens.inert(T));
@@ -734,9 +737,9 @@ class InjectorTest {
assertTrue(delivery.completion().isCompletedExceptionally(),
"the delivery's future must surface the send failure");
assertEquals(Injector.Cancellation.NOT_DELIVERED, inj.cancel(delivery),
"the message must be dropped and marked NOT_DELIVERED, not left QUEUED at the "
+ "head of the queue");
assertEquals(Injector.Cancellation.ATTEMPTED, inj.cancel(delivery),
"fleetd #551: the message must be dropped and marked ATTEMPTED (not a confident "
+ "NOT_DELIVERED), and never left QUEUED at the head of the queue");
}
@Test
@@ -758,10 +761,14 @@ class InjectorTest {
}
@Test
void aHerdrExceptionFromSendStillProducesNotDeliveredUnchanged() {
void aHerdrExceptionFromSendStillSurfacesButNowReportsAttempted() {
// fleetd #546, acceptance test 3 (control): HerdrException extends RuntimeException, so it
// was already caught before this ticket's widening. This pins that the ordinary path is
// unchanged — still dropped, still NOT_DELIVERED, still surfaced to the caller.
// was already caught before #546's widening. The send-failure/surface behaviour is
// unchanged by #551 — still dropped, still surfaced to the caller — but fleetd #551
// deliberately changes WHAT it is marked: "send_failed" is not a herdr `*_not_found` code,
// so this is exactly the response-half case #551 exists to fix. See
// aHerdrExceptionAfterThePasteIsNeverRecordedAsConfidentlyNotDelivered for the acceptance
// test this ticket was filed for.
FakeHerdr failing = new FakeHerdr().agentSendFailsWith("send_failed");
Injector inj = new Injector(new AgentControl(failing));
Injector.Delivery delivery = inj.enqueue(T, "boom", TestTurnTokens.inert(T));
@@ -771,9 +778,90 @@ class InjectorTest {
assertTrue(delivery.completion().isCompletedExceptionally(),
"a HerdrException at the send seam must still surface to the caller, unchanged by "
+ "fleetd #546's widening");
assertEquals(Injector.Cancellation.ATTEMPTED, inj.cancel(delivery),
"fleetd #551: a non-*_not_found HerdrException must now report ATTEMPTED, not a "
+ "confident NOT_DELIVERED");
}
// --- fleetd #551: the injector records the delivery attempt BEFORE the irreversible send, not
// after, so a failure in the send's response window can never be written down as a confident
// NOT_DELIVERED for text that may already be sitting in the worker's pane. ---
@Test
void aHerdrExceptionAfterThePasteIsNeverRecordedAsConfidentlyNotDelivered() {
// fleetd #551, the ticket's acceptance test 1, and comment 17037's added test ("a test
// pinning that a caller cannot receive a confident NOT_DELIVERED for a message that reached
// the paste"): a HerdrException thrown from the response half of the send seam — herdr
// replied, with an error, so it definitely processed the request — must not leave a record
// claiming the text was never delivered. Red before the fix: on main at ba2f4d1 this
// asserted (and got) Injector.Cancellation.NOT_DELIVERED.
FakeHerdr failing = new FakeHerdr().agentSendFailsWith("send_failed");
Injector inj = new Injector(new AgentControl(failing));
Injector.Delivery delivery = inj.enqueue(T, "brief", TestTurnTokens.inert(T));
inj.onStatus(T, AgentStatus.IDLE);
assertTrue(delivery.completion().isCompletedExceptionally(),
"the caller must still see the send failure");
assertEquals(Injector.Cancellation.ATTEMPTED, inj.cancel(delivery),
"fleetd #551: a HerdrException from the response half of the send seam must not be "
+ "recorded as a confident NOT_DELIVERED — the text may already be sitting "
+ "in the pane");
}
@Test
void ordinarySuccessStillReportsDeliveredExactlyOnce() {
// fleetd #551, the ticket's acceptance test 2: the record-before-send reorder must not
// change the ordinary success path — exactly one send, and the caller-facing Cancellation
// for it is still DELIVERED, never left at the new ATTEMPTED value.
Injector.Delivery delivery = injector.enqueue(T, "hello", TestTurnTokens.inert(T));
injector.onStatus(T, AgentStatus.IDLE);
assertEquals(List.of("hello"), sent(), "the text must be sent exactly once");
assertTrue(delivery.completion().isDone() && !delivery.completion().isCompletedExceptionally(),
"the ordinary success path must still complete normally");
assertEquals(Injector.Cancellation.DELIVERED, injector.cancel(delivery),
"fleetd #551: the ordinary success path must still report DELIVERED, unaffected by "
+ "the record-before-send reorder");
}
@Test
void transportDownStillSurfacesTheErrorAndDropsTheEntry() {
// fleetd #551, the ticket's acceptance test 3: the ordinary transport-down path (herdr
// unreachable — a plain HerdrException with no error code) must be unaffected by the #551
// reorder — the failure still surfaces to the caller, and the entry is not left sitting in
// the queue for a second onStatus round to resend.
FakeHerdr unreachable = new FakeHerdr().healthy(false);
Injector inj = new Injector(new AgentControl(unreachable));
Injector.Delivery delivery = inj.enqueue(T, "brief", TestTurnTokens.inert(T));
inj.onStatus(T, AgentStatus.IDLE);
assertTrue(delivery.completion().isCompletedExceptionally(),
"a transport-down failure must still surface to the caller");
assertTrue(inj.activeTargets().isEmpty(),
"the entry must not be left queued for a second round to resend");
}
@Test
void aNotFoundCodeFromSendStillReportsAConfidentNotDelivered() {
// fleetd #551 (comment 17037, point 3): a herdr `*_not_found` error means the target
// pane/agent does not exist at all, so nothing could have been pasted anywhere — the
// codebase already treats this family as a confirmed absence everywhere else (StatusPoller,
// AgentControl's own retry). #551's new ATTEMPTED state must not swallow this case: it stays
// a confident NOT_DELIVERED.
FakeHerdr failing = new FakeHerdr().agentSendFailsWith("agent_not_found");
Injector inj = new Injector(new AgentControl(failing));
Injector.Delivery delivery = inj.enqueue(T, "brief", TestTurnTokens.inert(T));
inj.onStatus(T, AgentStatus.IDLE);
assertTrue(delivery.completion().isCompletedExceptionally(),
"an agent_not_found failure must still surface to the caller");
assertEquals(Injector.Cancellation.NOT_DELIVERED, inj.cancel(delivery),
"a HerdrException must still be dropped and marked NOT_DELIVERED, unchanged by the "
+ "wider Throwable catch");
"fleetd #551: a herdr *_not_found error is a confirmed absence, not merely "
+ "inconclusive — it must stay NOT_DELIVERED, not the new ATTEMPTED");
}
// --- fleetd #553: a throwable from any listener callback in onStatus must not skip the