fleet/fleetd
3
0
Fork 0
Code Issues 73 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

Injector records the delivery AFTER the irreversible send, so a failure in the response window marks a delivered brief NOT_DELIVERED #551

New Issue
Closed
opened 2026-09-12 09:35:27 +02:00 by ltms · 4 comments
ltms commented 2026-09-12 09:35:27 +02:00
Owner
Copy Link
Copy Source

Separate from #546, older than #546, and not created by #546's fix. Filed so #546 is not
blocked on it, and so the fix for it is not mistaken for #546's.

Measured on main at 0b032f5.

The shape

Injector.java:382-396 is write-then-record:

try {
    agentsFor(target).send(target, p.text());   // :383  irreversible external side effect
    t.queue.poll();                             // :384  record it, afterwards
    p.state = Pending.State.DELIVERED;          // :385
    ...
} catch (RuntimeException e) {
    t.queue.poll();                             // :394
    p.state = Pending.State.NOT_DELIVERED;      // :395  assumes reaching here means nothing was sent
}

The catch arm assumes that reaching it means the send did not happen. Nothing establishes that.
The distinguishing fact — did the text reach the pane? — is never written down anywhere, so no
catch arm can recover it.

Why the window is real, measured on the send path

AgentControl.send is one herdr call, and its javadoc says what that call does:

/**
 * Deliver {@code text} to an agent as its next prompt <em>and submit it</em> — herdr's
 * {@code agent.prompt} pastes the text (embedded newlines preserved verbatim) and submits it
 * in the same call ...
 */
public void send(String target, String text) {
    agentCall("agent.prompt", target, Map.of("text", text));
}

UnixSocketHerdrClient.call() (:67-79) then does, in this order:

writeFully(ch, ByteBuffer.wrap(frame));       // request fully handed to herdr
return codec.decodeResult(readLine(ch));      // response read, then parsed
} catch (IOException e) {
    throw new HerdrException("herdr call '" + method + "' failed at transport ...", e);
}

Once writeFully returns, the request is herdr's. The paste is herdr's to perform, and the reply
reports its outcome — so the reply necessarily comes after the paste. Everything in the return
path can still fail:

  • readLine gets -1 with nothing buffered → IOException("herdr closed the connection with no response") → wrapped as HerdrException.
  • the response line is not valid JSON → HerdrException("malformed herdr response: ...")
    (HerdrCodec.java:51).
  • the response carries an error object → HerdrException (HerdrCodec.java:57).
$ grep -n 'class HerdrException' fleetd/src/main/java/dev/ltms/fleet/herdr/HerdrException.java
9:public class HerdrException extends RuntimeException {

HerdrException is a RuntimeException, so all three land in the existing narrow catch at
:391, which polls the entry and records NOT_DELIVERED — for a brief that was pasted and
submitted.

The correction this ticket exists to record

The fleet01 lead argued that widening :391 to Throwable (the #546 fix) would create this
contradiction, and asked to be argued out of it before the worker merged. On the evidence above,
it does not create it — the contradiction is already reachable today, on the ordinary
HerdrException path, with no Error anywhere. The widening extends an existing weakness to
one more throwable class; it does not introduce a new failure mode.

Their diagnosis of the underlying defect is right, and it is this ticket. Their placement of it —
as a trap inside #546 — is not, and blocking #546 on it would be the wrong trade by their own
argument: #546 without its fix is an unbounded re-delivery (nothing on that path ever removes
the Pending, so it repeats every poll interval until a human notices), and this ticket is a
bounded wrong record on one message. A member re-prompted forever is worse than a delivery
record that reads false.

The fix, and why it is not a one-liner

Record first, then send:

poll the entry and mark it ATTEMPTED/DELIVERING   <- before the irreversible call
send
mark DELIVERED

That converts a possible double-delivery into a possible zero-delivery. For brief injection that
is the failure you want: a member that did not get a brief is observable and recoverable; a member
that got it twice, or got it every poll interval, is neither.

It is a change to the delivery contract, not a catch-width change, which is exactly why it does
not belong in #546:

  • a third Pending.State is needed, and every reader of Pending.State has to be checked for
    what it does with it.
  • sent/sendError are consumed after the block; their meaning changes.
  • NOT_DELIVERED stops meaning "definitely not delivered" and starts meaning "not confirmed", and
    every caller that reports it to an operator needs to say so.

If herdr can carry a caller-supplied idempotency key, that is better still — it makes the retry
safe rather than making the record honest — but the ordering fix needs nothing from herdr and
should not wait for it.

Acceptance

  • A test that a HerdrException thrown from the response half of the send seam (after the
    paste) does not leave a record claiming the text was never delivered. This is the test that
    pins the defect; it must fail before the fix.
  • A test that the ordinary success path still reports DELIVERED exactly once.
  • A test that the ordinary transport-down path — herdr unreachable, so nothing was pasted — still
    surfaces the error to the caller and does not leave the entry in the queue.
  • Enumerate every reader of Pending.State and say in the PR what each does with the new state.
    This is the fleet01 lead's checklist and it applies to a state addition as much as to a catch
    widening: name the mutable state and the external side effects the region can leave half-done,
    and say who restores each one now.
  • For each new test: mutation red with that test's own message, restore, byte-identical
    shasum -a 256, green control, and the proof cell run against the un-mutated tree first to
    confirm it reports not-applied.

Not in scope, noticed while reading

AgentControl.agentCall:48-56 retries herdr.call once on an agent_not_found code with a stale
cached pane. I have not checked whether any method reachable through it can perform a side
effect and still answer agent_not_found. If one can, that retry is a second copy of this same
question one layer down. Reported, not investigated.

Related: #546 (the widening, which should land first), #538 / PR #543, #544.

Separate from #546, older than #546, and **not** created by #546's fix. Filed so #546 is not blocked on it, and so the fix for it is not mistaken for #546's. Measured on `main` at `0b032f5`. ## The shape `Injector.java:382-396` is write-then-record: ```java try { agentsFor(target).send(target, p.text()); // :383 irreversible external side effect t.queue.poll(); // :384 record it, afterwards p.state = Pending.State.DELIVERED; // :385 ... } catch (RuntimeException e) { t.queue.poll(); // :394 p.state = Pending.State.NOT_DELIVERED; // :395 assumes reaching here means nothing was sent } ``` The catch arm assumes that reaching it means the send did not happen. Nothing establishes that. The distinguishing fact — *did the text reach the pane?* — is never written down anywhere, so no catch arm can recover it. ## Why the window is real, measured on the send path `AgentControl.send` is one herdr call, and its javadoc says what that call does: ```java /** * Deliver {@code text} to an agent as its next prompt <em>and submit it</em> — herdr's * {@code agent.prompt} pastes the text (embedded newlines preserved verbatim) and submits it * in the same call ... */ public void send(String target, String text) { agentCall("agent.prompt", target, Map.of("text", text)); } ``` `UnixSocketHerdrClient.call()` (`:67-79`) then does, in this order: ```java writeFully(ch, ByteBuffer.wrap(frame)); // request fully handed to herdr return codec.decodeResult(readLine(ch)); // response read, then parsed } catch (IOException e) { throw new HerdrException("herdr call '" + method + "' failed at transport ...", e); } ``` Once `writeFully` returns, the request is herdr's. The paste is herdr's to perform, and the reply reports its outcome — so the reply necessarily comes **after** the paste. Everything in the return path can still fail: - `readLine` gets `-1` with nothing buffered → `IOException("herdr closed the connection with no response")` → wrapped as `HerdrException`. - the response line is not valid JSON → `HerdrException("malformed herdr response: ...")` (`HerdrCodec.java:51`). - the response carries an `error` object → `HerdrException` (`HerdrCodec.java:57`). ``` $ grep -n 'class HerdrException' fleetd/src/main/java/dev/ltms/fleet/herdr/HerdrException.java 9:public class HerdrException extends RuntimeException { ``` `HerdrException` **is** a `RuntimeException`, so all three land in the existing narrow catch at `:391`, which polls the entry and records `NOT_DELIVERED` — for a brief that was pasted and submitted. ## The correction this ticket exists to record The fleet01 lead argued that widening `:391` to `Throwable` (the #546 fix) would *create* this contradiction, and asked to be argued out of it before the worker merged. On the evidence above, it does not create it — **the contradiction is already reachable today, on the ordinary `HerdrException` path, with no `Error` anywhere.** The widening extends an existing weakness to one more throwable class; it does not introduce a new failure mode. Their diagnosis of the underlying defect is right, and it is this ticket. Their placement of it — as a trap inside #546 — is not, and blocking #546 on it would be the wrong trade by their own argument: #546 without its fix is an **unbounded** re-delivery (nothing on that path ever removes the `Pending`, so it repeats every poll interval until a human notices), and this ticket is a **bounded** wrong record on one message. A member re-prompted forever is worse than a delivery record that reads false. ## The fix, and why it is not a one-liner Record first, then send: ``` poll the entry and mark it ATTEMPTED/DELIVERING <- before the irreversible call send mark DELIVERED ``` That converts a possible double-delivery into a possible zero-delivery. For brief injection that is the failure you want: a member that did not get a brief is observable and recoverable; a member that got it twice, or got it every poll interval, is neither. It is a change to the delivery contract, not a catch-width change, which is exactly why it does not belong in #546: - a third `Pending.State` is needed, and every reader of `Pending.State` has to be checked for what it does with it. - `sent`/`sendError` are consumed after the block; their meaning changes. - `NOT_DELIVERED` stops meaning "definitely not delivered" and starts meaning "not confirmed", and every caller that reports it to an operator needs to say so. If herdr can carry a caller-supplied idempotency key, that is better still — it makes the retry safe rather than making the record honest — but the ordering fix needs nothing from herdr and should not wait for it. ## Acceptance - A test that a `HerdrException` thrown from the **response** half of the send seam (after the paste) does not leave a record claiming the text was never delivered. This is the test that pins the defect; it must fail before the fix. - A test that the ordinary success path still reports `DELIVERED` exactly once. - A test that the ordinary transport-down path — herdr unreachable, so nothing was pasted — still surfaces the error to the caller and does not leave the entry in the queue. - Enumerate every reader of `Pending.State` and say in the PR what each does with the new state. This is the fleet01 lead's checklist and it applies to a state addition as much as to a catch widening: *name the mutable state and the external side effects the region can leave half-done, and say who restores each one now.* - For each new test: mutation red with that test's own message, restore, byte-identical `shasum -a 256`, green control, and the proof cell run against the un-mutated tree first to confirm it reports not-applied. ## Not in scope, noticed while reading `AgentControl.agentCall:48-56` retries `herdr.call` once on an `agent_not_found` code with a stale cached pane. I have **not** checked whether any method reachable through it can perform a side effect and still answer `agent_not_found`. If one can, that retry is a second copy of this same question one layer down. Reported, not investigated. Related: #546 (the widening, which should land first), #538 / PR #543, #544.
ltms commented 2026-09-12 09:48:57 +02:00
Author
Owner
Copy Link
Copy Source

Two corrections to the body above, one against me and one that makes this ticket more precise. Both
came from the fleet01 lead; I re-measured both on main at 93a9ed3.

1. Strike my ordering sentence — it is too strong, and the conclusion does not need it

The body says:

Once writeFully returns, the request is herdr's. The paste is herdr's to perform, and the reply
reports its outcome — so the reply necessarily comes after the paste.

That is wrong as written. writeFully returning means the bytes reached the kernel socket buffer,
not that herdr read them or acted on them. If the connection drops after the write and before herdr
processes it, nothing is pasted, readLine throws IOException -> HerdrException, and
NOT_DELIVERED is correct on that path.

The conclusion survives on a simpler fact that needs nothing from agent.prompt's semantics: three
of the four HerdrException throw sites fire after readLine has already returned a line, and
a line existing at all proves herdr received the request and produced output for it.

2. Four throw sites, four different truths, one value written

I checked every throw site in the codec rather than the three I listed:

$ grep -n 'throw new HerdrException' fleetd/src/main/java/dev/ltms/fleet/herdr/HerdrCodec.java
37:  failed to encode herdr request for method ...
51:  malformed herdr response: ...
57:  herdr error [code]: ...
61:  herdr response has neither result nor error: ...

:37 is inside encode, before writeFully — I had missed it entirely, and it is the one case
where the current behaviour is right. :61 I had also missed.

So one exception type reaches one catch arm from four paths:

path what actually happened is NOT_DELIVERED right?
:37 encode failure nothing was ever sent yes
IOException at transport bytes left the process; herdr may or may not have acted unknown
:51 malformed / :61 no result herdr replied, so it processed the request probably not
:57 error object depends on the code — a *_not_found is definitely-not-delivered; anything raised after the paste is not depends

That is a better statement of the defect than the body's. It is not "NOT_DELIVERED is a lie" — it
is that one value is written for four epistemic states, and it gets three of them wrong in
different directions. The fix has to distinguish them, not just rename the value.

3. The codebase already makes this distinction, in six places

This is the part that makes the enum change cheaper than it looks. *_not_found is already treated
as "definitely absent, not merely inconclusive" throughout:

StatusPoller.java:83        e.code().endsWith("_not_found")  -> injector.drop(target, e)
AgentControl.java:52        "agent_not_found"                -> invalidate the pane cache, retry once
WorkspacePlacement/WorkspaceControl.java:172  endsWith("_not_found")  -> treat as success
HerdrPeerLauncher.java:1014 endsWith("_not_found")
FleetApp.java:819           endsWith("_not_found")           -> HTTP 404
ReplyPushLoop.java:471      "agent_not_found"                -> gone

ReplyPushLoop.java:456 already states the principle this ticket needs, in its own words: resolve
"never on a merely inconclusive failure."

So #551 extends an existing vocabulary rather than inventing one, and the new state should be
consistent with how *_not_found is already read. Whoever takes this should read those six sites
before choosing the enum's shape.

What does not change

The trade in the body stands, and the fleet01 lead has withdrawn their objection to #546 on exactly
the ground above — the narrow catch already spanned the return path, so #549's widening could not
have been what opened this. #549 is merged.

Two corrections to the body above, one against me and one that makes this ticket more precise. Both came from the fleet01 lead; I re-measured both on `main` at `93a9ed3`. ## 1. Strike my ordering sentence — it is too strong, and the conclusion does not need it The body says: > Once `writeFully` returns, the request is herdr's. The paste is herdr's to perform, and the reply > reports its outcome — so the reply necessarily comes **after** the paste. That is wrong as written. `writeFully` returning means the bytes reached the kernel socket buffer, not that herdr read them or acted on them. If the connection drops after the write and before herdr processes it, **nothing is pasted**, `readLine` throws `IOException` -> `HerdrException`, and `NOT_DELIVERED` is *correct* on that path. The conclusion survives on a simpler fact that needs nothing from `agent.prompt`'s semantics: three of the four `HerdrException` throw sites fire **after `readLine` has already returned a line**, and a line existing at all proves herdr received the request and produced output for it. ## 2. Four throw sites, four different truths, one value written I checked every throw site in the codec rather than the three I listed: ``` $ grep -n 'throw new HerdrException' fleetd/src/main/java/dev/ltms/fleet/herdr/HerdrCodec.java 37: failed to encode herdr request for method ... 51: malformed herdr response: ... 57: herdr error [code]: ... 61: herdr response has neither result nor error: ... ``` `:37` is inside `encode`, **before** `writeFully` — I had missed it entirely, and it is the one case where the current behaviour is right. `:61` I had also missed. So one exception type reaches one catch arm from four paths: | path | what actually happened | is `NOT_DELIVERED` right? | |---|---|---| | `:37` encode failure | nothing was ever sent | **yes** | | `IOException` at transport | bytes left the process; herdr may or may not have acted | **unknown** | | `:51` malformed / `:61` no result | herdr replied, so it processed the request | **probably not** | | `:57` error object | depends on the code — a `*_not_found` is definitely-not-delivered; anything raised after the paste is not | **depends** | That is a better statement of the defect than the body's. It is not "`NOT_DELIVERED` is a lie" — it is that **one value is written for four epistemic states, and it gets three of them wrong in different directions.** The fix has to distinguish them, not just rename the value. ## 3. The codebase already makes this distinction, in six places This is the part that makes the enum change cheaper than it looks. `*_not_found` is already treated as "definitely absent, not merely inconclusive" throughout: ``` StatusPoller.java:83 e.code().endsWith("_not_found") -> injector.drop(target, e) AgentControl.java:52 "agent_not_found" -> invalidate the pane cache, retry once WorkspacePlacement/WorkspaceControl.java:172 endsWith("_not_found") -> treat as success HerdrPeerLauncher.java:1014 endsWith("_not_found") FleetApp.java:819 endsWith("_not_found") -> HTTP 404 ReplyPushLoop.java:471 "agent_not_found" -> gone ``` `ReplyPushLoop.java:456` already states the principle this ticket needs, in its own words: resolve "never on a merely inconclusive failure." So #551 **extends an existing vocabulary rather than inventing one**, and the new state should be consistent with how `*_not_found` is already read. Whoever takes this should read those six sites before choosing the enum's shape. ## What does not change The trade in the body stands, and the fleet01 lead has withdrawn their objection to #546 on exactly the ground above — the narrow catch already spanned the return path, so #549's widening could not have been what opened this. #549 is merged.
ltms commented 2026-09-12 12:25:00 +02:00
Author
Owner
Copy Link
Copy Source

Unblocked. #546 and #556 have both landed, so Injector.java has settled and this can be picked up. Three corrections to the ticket body before anyone does — this comment is newer than the body, so it wins.

1. Every line number in the body is stale

Measured on main at ba2f4d1:

body says actually now
Injector.java:382-396 (the write-then-record region) :429-451
:383 the send :430
:391 the catch :444

Re-measure before quoting a line. Do not trust a line number in this ticket.

2. #546 has already landed, so the catch is Throwable, not RuntimeException

The body quotes catch (RuntimeException e). On main today it reads catch (Throwable e) and carries #546's comment explaining why. The argument in the body is unaffected — the contradiction was always reachable on the ordinary HerdrException path, with no Error involved — but do not "fix" the catch width. It is already correct.

3. The hazard the body does not name: cancellationOf silently maps the new state onto NOT_DELIVERED

This is the thing most likely to be missed, because it compiles and every existing test stays green.

Injector.java:336:

private static Cancellation cancellationOf(Pending p) {
    return p.state == Pending.State.DELIVERED ? Cancellation.DELIVERED : Cancellation.NOT_DELIVERED;
}

It is a two-way split on a three-value question. Add a third Pending.State — ATTEMPTED, or whatever it ends up being called — and this method answers NOT_DELIVERED for it, with no compiler error and no failing test. That is the exact defect this ticket exists to fix, reappearing one layer up: a message that may have been pasted gets reported to a caller as definitely not delivered.

The same trap sits in the Cancellation enum itself. NOT_DELIVERED there already means "it is not delivered" rather than "I cancelled it" — see #513 for the four routes that reach it. A third Pending.State needs a matching third answer, or the honesty you add at the bottom is thrown away at the top.

So the body's checklist item — enumerate every reader of Pending.State and say in the PR what each does with the new state — has this specific answer expected in it. The readers on main at ba2f4d1:

:324  if (p.state != Pending.State.QUEUED || !t.queue.remove(p))   cancel(): guards the removal
:327  p.state = Pending.State.CANCELLED                            cancel(): the only acting branch
:336  return p.state == DELIVERED ? DELIVERED : NOT_DELIVERED      cancellationOf(): the two-way split above
:432  p.state = Pending.State.DELIVERED                            the success write
:447  p.state = Pending.State.NOT_DELIVERED                        the catch write — this is the defect site
:466  pending.state = Pending.State.NOT_DELIVERED                  readiness grace expired
:757  p.state = Pending.State.NOT_DELIVERED                        drop()

:466 and :757 are the two places where NOT_DELIVERED is true and certain — nothing was ever sent. Keep them saying that. Only :447 is the uncertain one. If the new state is added but :466 and :757 are moved onto it too, the ticket has made every answer vague instead of making one answer honest, which is worse than leaving it alone.

Acceptance, added to the body's list

  • The PR must state what cancellationOf returns for the new state, and why that is the right answer for a caller that has to report it to an operator.
  • A test pinning that a caller cannot receive a confident NOT_DELIVERED for a message that reached the paste. Red before the fix.
Unblocked. #546 and #556 have both landed, so `Injector.java` has settled and this can be picked up. Three corrections to the ticket body before anyone does — **this comment is newer than the body, so it wins.** ## 1. Every line number in the body is stale Measured on `main` at `ba2f4d1`: | body says | actually now | |---|---| | `Injector.java:382-396` (the write-then-record region) | **`:429-451`** | | `:383` the send | **`:430`** | | `:391` the catch | **`:444`** | Re-measure before quoting a line. Do not trust a line number in this ticket. ## 2. #546 has already landed, so the catch is `Throwable`, not `RuntimeException` The body quotes `catch (RuntimeException e)`. On `main` today it reads `catch (Throwable e)` and carries #546's comment explaining why. The argument in the body is unaffected — the contradiction was always reachable on the ordinary `HerdrException` path, with no `Error` involved — but do not "fix" the catch width. It is already correct. ## 3. The hazard the body does not name: `cancellationOf` silently maps the new state onto `NOT_DELIVERED` This is the thing most likely to be missed, because it compiles and every existing test stays green. `Injector.java:336`: ```java private static Cancellation cancellationOf(Pending p) { return p.state == Pending.State.DELIVERED ? Cancellation.DELIVERED : Cancellation.NOT_DELIVERED; } ``` It is a two-way split on a three-value question. Add a third `Pending.State` — `ATTEMPTED`, or whatever it ends up being called — and this method answers `NOT_DELIVERED` for it, with no compiler error and no failing test. That is the exact defect this ticket exists to fix, reappearing one layer up: a message that **may have been pasted** gets reported to a caller as **definitely not delivered**. The same trap sits in the `Cancellation` enum itself. `NOT_DELIVERED` there already means "it is not delivered" rather than "I cancelled it" — see #513 for the four routes that reach it. A third `Pending.State` needs a matching third answer, or the honesty you add at the bottom is thrown away at the top. So the body's checklist item — *enumerate every reader of `Pending.State` and say in the PR what each does with the new state* — has this specific answer expected in it. The readers on `main` at `ba2f4d1`: ``` :324 if (p.state != Pending.State.QUEUED || !t.queue.remove(p)) cancel(): guards the removal :327 p.state = Pending.State.CANCELLED cancel(): the only acting branch :336 return p.state == DELIVERED ? DELIVERED : NOT_DELIVERED cancellationOf(): the two-way split above :432 p.state = Pending.State.DELIVERED the success write :447 p.state = Pending.State.NOT_DELIVERED the catch write — this is the defect site :466 pending.state = Pending.State.NOT_DELIVERED readiness grace expired :757 p.state = Pending.State.NOT_DELIVERED drop() ``` `:466` and `:757` are the two places where `NOT_DELIVERED` is **true and certain** — nothing was ever sent. Keep them saying that. Only `:447` is the uncertain one. If the new state is added but `:466` and `:757` are moved onto it too, the ticket has made every answer vague instead of making one answer honest, which is worse than leaving it alone. ## Acceptance, added to the body's list - The PR must state what `cancellationOf` returns for the new state, and why that is the right answer for a caller that has to report it to an operator. - A test pinning that a caller cannot receive a confident `NOT_DELIVERED` for a message that reached the paste. Red before the fix.
ltms commented 2026-09-12 12:48:27 +02:00
Author
Owner
Copy Link
Copy Source

Verified by me on the branch at d83821b, merged with main at ba2f4d1 (the branch already contained main — git merge reported "Already up to date").

What I checked

Build: mvn -o clean install exit 0, 1754 tests, agreed by Maven's summary and an independent sum over the surefire reports. 1750 baseline + 4 new = 1754, which reconciles.

I mutated a line the worker did not mutate — the one I named as the hazard in comment 17037. Anchor case ATTEMPTED -> Cancellation.ATTEMPTED; at :390, pristine count 1 → 0, Injector.java restored to 0c689b6cf36275c0da45497a74b2bd4f5a3d80c4dbda77d46670c66004e53b69.

Folding it back into NOT_DELIVERED — the exact regression this ticket exists to prevent — kills three tests, each with its own message and observed value:

aHerdrExceptionAfterThePasteIsNeverRecordedAsConfidentlyNotDelivered:806
  ... must not be recorded as a confident NOT_DELIVERED ==> expected: <ATTEMPTED> but was: <NOT_DELIVERED>
aHerdrExceptionFromSendStillSurfacesButNowReportsAttempted:781
  ... must now report ATTEMPTED ==> expected: <ATTEMPTED> but was: <NOT_DELIVERED>
anErrorFromSendRemovesTheMessageAndMarksItAttempted:740
  ... marked ATTEMPTED (not a confident NOT_DELIVERED), and never left QUEUED ==> expected: <ATTEMPTED> but was: <NOT_DELIVERED>

So the fix is pinned at the layer this ticket owns. Good work — and making cancellationOf an exhaustive switch expression over the enum is better than what I asked for: adding a fourth state is now a compile error rather than a silent fold. The two-way split became a total function.

One thing to fix before this merges

The worker flagged it themselves and was right to. MessageService.java:973 is the only reader of Cancellation outside Injector:

wasDelivered = injector.cancel(delivery) == Injector.Cancellation.DELIVERED;

ATTEMPTED != DELIVERED, so this yields Outcome.TIMED_OUT_QUEUED.

The caller-visible behaviour is unchanged by this PR — before it, that path produced NOT_DELIVERED, which also yielded TIMED_OUT_QUEUED. So this is not a regression and I am not asking anyone to fix the collapse here.

What is a regression is the javadoc. TIMED_OUT_QUEUED's javadoc is the thing #513 landed three hours ago to make true, and this change makes it stale again:

Despite the name, this does not mean the message is sitting in a queue — on every route it will not arrive later. [...] NOT_DELIVERED means an earlier attempt already decided the message's fate [...] On the failed-attempt route the terminal may already hold a partial paste from before the call threw.

Two problems now:

  1. It enumerates the routes as CANCELLED and NOT_DELIVERED. ATTEMPTED is a third route and is not mentioned at all.
  2. "may already hold a partial paste" understates it. AgentControl.send calls herdr's agent.prompt, which pastes and submits in one call. On the ATTEMPTED route the target may hold a complete, submitted turn and be working on it right now — which is TIMED_OUT_WORKING's meaning, reported as TIMED_OUT_QUEUED.

Leaving that is precisely the defect #513 existed to fix, reintroduced by a change that made the doc stale without touching the file. That is the trap this repo keeps hitting, so it does not get to ship.

Rework — small, one file

  • Update TIMED_OUT_QUEUED's javadoc in MessageService.java to name three routes and say plainly which one is uncertain. The sentence "on every route it will not arrive later" must go or be qualified: on the ATTEMPTED route it may already have arrived, in full.
  • Do not change MessageService's behaviour. The collapse is a separate decision and I am filing it separately.
  • No new test needed for a javadoc change. Re-run the full build and report the count.

Follow-up, filed separately, not part of this PR

MessageService discarding ATTEMPTED is the same two-way-split-on-a-three-way-question shape one layer up — the honesty added at the bottom thrown away at the top. That is a behaviour change with its own acceptance criteria and it needs its own ticket.

Verified by me on the branch at `d83821b`, merged with `main` at `ba2f4d1` (the branch already contained main — `git merge` reported "Already up to date"). ## What I checked Build: `mvn -o clean install` exit 0, **1754 tests**, agreed by Maven's summary and an independent sum over the surefire reports. 1750 baseline + 4 new = 1754, which reconciles. I mutated a line the worker did **not** mutate — the one I named as the hazard in comment 17037. Anchor ` case ATTEMPTED -> Cancellation.ATTEMPTED;` at `:390`, pristine count 1 → 0, `Injector.java` restored to `0c689b6cf36275c0da45497a74b2bd4f5a3d80c4dbda77d46670c66004e53b69`. Folding it back into `NOT_DELIVERED` — the exact regression this ticket exists to prevent — **kills three tests**, each with its own message and observed value: ``` aHerdrExceptionAfterThePasteIsNeverRecordedAsConfidentlyNotDelivered:806 ... must not be recorded as a confident NOT_DELIVERED ==> expected: <ATTEMPTED> but was: <NOT_DELIVERED> aHerdrExceptionFromSendStillSurfacesButNowReportsAttempted:781 ... must now report ATTEMPTED ==> expected: <ATTEMPTED> but was: <NOT_DELIVERED> anErrorFromSendRemovesTheMessageAndMarksItAttempted:740 ... marked ATTEMPTED (not a confident NOT_DELIVERED), and never left QUEUED ==> expected: <ATTEMPTED> but was: <NOT_DELIVERED> ``` So the fix is pinned at the layer this ticket owns. Good work — and making `cancellationOf` an **exhaustive switch expression** over the enum is better than what I asked for: adding a fourth state is now a compile error rather than a silent fold. The two-way split became a total function. ## One thing to fix before this merges The worker flagged it themselves and was right to. `MessageService.java:973` is the only reader of `Cancellation` outside `Injector`: ```java wasDelivered = injector.cancel(delivery) == Injector.Cancellation.DELIVERED; ``` `ATTEMPTED != DELIVERED`, so this yields `Outcome.TIMED_OUT_QUEUED`. **The caller-visible behaviour is unchanged by this PR** — before it, that path produced `NOT_DELIVERED`, which also yielded `TIMED_OUT_QUEUED`. So this is not a regression and I am not asking anyone to fix the collapse here. What *is* a regression is the **javadoc**. `TIMED_OUT_QUEUED`'s javadoc is the thing #513 landed three hours ago to make true, and this change makes it stale again: > Despite the name, this does not mean the message is sitting in a queue — **on every route it will not arrive later.** [...] `NOT_DELIVERED` means an earlier attempt already decided the message's fate [...] On the failed-attempt route the terminal **may already hold a partial paste** from before the call threw. Two problems now: 1. It enumerates the routes as `CANCELLED` and `NOT_DELIVERED`. **`ATTEMPTED` is a third route and is not mentioned at all.** 2. "may already hold a partial paste" **understates it.** `AgentControl.send` calls herdr's `agent.prompt`, which pastes *and submits* in one call. On the `ATTEMPTED` route the target may hold a complete, submitted turn and be working on it right now — which is `TIMED_OUT_WORKING`'s meaning, reported as `TIMED_OUT_QUEUED`. Leaving that is precisely the defect #513 existed to fix, reintroduced by a change that made the doc stale without touching the file. That is the trap this repo keeps hitting, so it does not get to ship. ## Rework — small, one file - Update `TIMED_OUT_QUEUED`'s javadoc in `MessageService.java` to name **three** routes and say plainly which one is uncertain. The sentence "on every route it will not arrive later" must go or be qualified: on the `ATTEMPTED` route it may already have arrived, in full. - Do **not** change `MessageService`'s behaviour. The collapse is a separate decision and I am filing it separately. - No new test needed for a javadoc change. Re-run the full build and report the count. ## Follow-up, filed separately, not part of this PR `MessageService` discarding `ATTEMPTED` is the same two-way-split-on-a-three-way-question shape one layer up — the honesty added at the bottom thrown away at the top. That is a behaviour change with its own acceptance criteria and it needs its own ticket.
ltms closed this issue 2026-09-12 13:19:20 +02:00
ltms commented 2026-09-12 13:22:03 +02:00
Author
Owner
Copy Link
Copy Source

Merged as 384867d (PR #569).

Verified by the lead on a merged tree, re-running the checks rather than accepting the worker's:

  • mvn -o clean install from fleetd/, exit 0. 1754 tests from Maven and from an independent
    sum over 130 surefire reports — two agreeing sources.
  • Mutation, on a line the worker did not mutate: folding ATTEMPTED back into NOT_DELIVERED
    in cancellationOf (delete the case ATTEMPTED -> arm, add ATTEMPTED to the NOT_DELIVERED
    arm — the exact pre-fix defect). Result: BUILD FAILURE, 3 red, each naming the property:
    anErrorFromSendRemovesTheMessageAndMarksItAttempted:740,
    aHerdrExceptionFromSendStillSurfacesButNowReportsAttempted:781,
    aHerdrExceptionAfterThePasteIsNeverRecordedAsConfidentlyNotDelivered:806.
    Anchor control with grep -Fxc: both arms counted 1 pristine, 0 after. Restored to sha256
    0c689b6cf36275c0..., git status --short empty.
  • I also read the full Injector.java diff, not only the reported lines. cancel() returns
    Cancellation.CANCELLED at :380 when it actually removed a queued entry, so the javadoc's
    three-route claim is accurate rather than describing a route that cannot happen — I checked that
    specifically, because cancellationOf maps Pending.State.CANCELLED to
    Cancellation.NOT_DELIVERED and the two enums sharing a constant name is an easy misread.

The javadoc half, and a correction to what "stale" meant here

Two of the claims were wrong, not merely out of date:

  1. "on every route it will not arrive later" — false on the ATTEMPTED route.
  2. "may already hold a partial paste" — understates it. agent.prompt pastes and submits in
    one call, so the target may hold a complete, already-submitted turn and be working on it. Telling
    an operator "partial paste" says the damage is cosmetic when a whole turn may be running.

The worker flagged a second instance out of scope; it was right, so I gave it back to them with a
sweep of the whole dev.ltms.fleet.msg package. Three comment sites were fixed in total:
TIMED_OUT_QUEUED, hasQueuedDelivery, the queuedDeliveries field javadoc, and the comment above
queuedDeliveries.put(...) in send()'s timeout branch. No code defect was found by the sweep.

The final round's comment-only claim was proven mechanically, not by reading the diff: stripping
every comment from MessageService.java before and after and collapsing whitespace gives
byte-identical code.

Follow-up #571 (the four-valued Cancellation collapsed to a boolean at MessageService.java:973)
is unblocked by this merge and stays open — the information now exists and is still discarded at the
caller.

Merged as `384867d` (PR #569). Verified by the lead on a merged tree, re-running the checks rather than accepting the worker's: - `mvn -o clean install` from `fleetd/`, exit 0. **1754 tests** from Maven and from an independent sum over 130 surefire reports — two agreeing sources. - **Mutation**, on a line the worker did not mutate: folding `ATTEMPTED` back into `NOT_DELIVERED` in `cancellationOf` (delete the `case ATTEMPTED ->` arm, add `ATTEMPTED` to the `NOT_DELIVERED` arm — the exact pre-fix defect). Result: BUILD FAILURE, **3 red**, each naming the property: `anErrorFromSendRemovesTheMessageAndMarksItAttempted:740`, `aHerdrExceptionFromSendStillSurfacesButNowReportsAttempted:781`, `aHerdrExceptionAfterThePasteIsNeverRecordedAsConfidentlyNotDelivered:806`. Anchor control with `grep -Fxc`: both arms counted 1 pristine, 0 after. Restored to sha256 `0c689b6cf36275c0...`, `git status --short` empty. - I also read the full `Injector.java` diff, not only the reported lines. `cancel()` returns `Cancellation.CANCELLED` at :380 when it actually removed a queued entry, so the javadoc's three-route claim is accurate rather than describing a route that cannot happen — I checked that specifically, because `cancellationOf` maps `Pending.State.CANCELLED` to `Cancellation.NOT_DELIVERED` and the two enums sharing a constant name is an easy misread. ## The javadoc half, and a correction to what "stale" meant here Two of the claims were **wrong, not merely out of date**: 1. *"on every route it will not arrive later"* — false on the `ATTEMPTED` route. 2. *"may already hold a partial paste"* — understates it. `agent.prompt` pastes **and submits** in one call, so the target may hold a complete, already-submitted turn and be working on it. Telling an operator "partial paste" says the damage is cosmetic when a whole turn may be running. The worker flagged a second instance out of scope; it was right, so I gave it back to them with a sweep of the whole `dev.ltms.fleet.msg` package. Three comment sites were fixed in total: `TIMED_OUT_QUEUED`, `hasQueuedDelivery`, the `queuedDeliveries` field javadoc, and the comment above `queuedDeliveries.put(...)` in `send()`'s timeout branch. No code defect was found by the sweep. The final round's comment-only claim was **proven mechanically, not by reading the diff**: stripping every comment from `MessageService.java` before and after and collapsing whitespace gives byte-identical code. Follow-up #571 (the four-valued `Cancellation` collapsed to a boolean at `MessageService.java:973`) is unblocked by this merge and stays open — the information now exists and is still discarded at the caller.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
blocked
Cannot start until something else lands. The body says what.
 needs-live-proof
Merged and green, but never shown working on the running daemon. Not the same as done.
 ready-to-delegate
Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.
 silent-default
A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent kevin ltms
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#551
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?