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

Merging #543 made re-delivery reachable: an Error after Injector's send now loops and types the same brief again #546

New Issue
Closed
opened 2026-09-12 09:14:27 +02:00 by ltms · 1 comment
ltms commented 2026-09-12 09:14:27 +02:00
Owner
Copy Link
Copy Source

I caused this by merging #543 (fleetd #538) an hour ago. Filed immediately.

The fleet01 lead proposed this mechanism days ago. I measured it and told them it was
unreachable, and I was right at the time. #543 removed the second defect that was hiding it.
Their reasoning was correct; only my "unreachable" was, and it stopped being true at
cc302fe.

Measured on main at 7611b69.

The chain, every step measured

1. Injector's delivery catch is narrow.

Injector.java:382                try {
Injector.java:383                    agentsFor(target).send(target, p.text());
Injector.java:384                    t.queue.poll();
Injector.java:385                    p.state = Pending.State.DELIVERED;
...
Injector.java:391                } catch (RuntimeException e) {
Injector.java:394                    t.queue.poll();
Injector.java:395                    p.state = Pending.State.NOT_DELIVERED;

$ grep -n "catch (" fleetd/src/main/java/dev/ltms/fleet/inject/Injector.java
391:                            } catch (RuntimeException e) {
495:            } catch (RuntimeException e) {

Two catch clauses in the whole file, both RuntimeException. HerdrException extends RuntimeException, so every ordinary herdr failure is caught — only an Error, or any other
non-RuntimeException throwable, gets past.

2. On that path the message is never removed from the queue. Neither t.queue.poll() at :384
nor the one at :394 runs, and p.state is never written. The Pending stays at the head of
t.queue with state == QUEUED. The throwable unwinds out of synchronized (t), releasing the
monitor.

3. StatusPoller is the only production caller, and it now swallows that throwable.

$ grep -rn "onStatus" fleetd/src/main/java --include='*.java' | grep -v '^.*\*'
fleetd/src/main/java/dev/ltms/fleet/inject/StatusPoller.java:80:  injector.onStatus(target, status);

$ grep -n "catch (" fleetd/src/main/java/dev/ltms/fleet/inject/StatusPoller.java
81:                    } catch (HerdrException e) {
89:                    } catch (Throwable e) {     <- added by #543
106:        } catch (InterruptedException e) {

4. So the loop continues, peeks the same Pending, and sends it again. Injector.java:378 is
Pending p = t.queue.peek(); — a peek, not a poll. The entry is still there and still QUEUED, so
the next round re-enters the same try and calls send on the same text.

Why this was not a defect yesterday, and is one today

Before #543, StatusPoller.loop caught only HerdrException and RuntimeException. An Error at
step 3 escaped, and the status-poller virtual thread died. The message was never re-delivered,
because the only thread that could re-deliver it was gone. That was my measurement, and it was
correct against that revision.

#543 fixed exactly that — the loop now survives — which is right and I am not proposing to revert
it. But surviving means the loop comes back round, and step 4 is what "coming back round" means when
step 2 left the queue untouched.

This is a-defect-on-paper-is-not-a-reachable-defect read backwards: a fix can create
reachability. The paper defect and the blocker were in two different files, owned by two different
tickets, and neither ticket's reviewer could see the other half.

What it costs when it fires

The send at :383 is what types the brief into the member's pane. If the throwable arrives
after herdr has typed the text, the member has the brief, the queue still thinks it does not,
and the next round types it in again. A worker gets the same brief twice, in the same pane, as two
separate injections.

If the throwable is persistent, this repeats every poll interval — a brief retyped into a pane
several times a second, with one log.error per round from #543's new line.

I have not observed this. The mechanism is measured; the arrival of an Error at :383 is the
same reachability argument as #538's, which leans on #413 (a mvn clean in the tree deletes the
running daemon's jar, so a not-yet-loaded class fails with NoClassDefFoundError on first use) and
on the fleet01 lead's real NoClassDefFoundError on their host on 2026-09-10.

The fix

Widen Injector.java:391 to catch (Throwable e), the same widening #543 applied one layer up. The
existing handler already does the right thing — drop the poisoned message, mark it NOT_DELIVERED,
surface the error — and the comment above it already says so. It is scoped one class too narrow, in
exactly the way #538 found StatusPoller scoped one class too narrow.

#538 said "do not touch Injector.java", and that was right for that ticket. It is wrong now, and
this ticket exists to say so.

Check Injector.java:495 for the same shape while you are there. I have not read it and make no
claim about it.

Acceptance

  • A test that throws an Error from the send seam at Injector.java:383 and asserts the message
    is removed from the queue and marked NOT_DELIVERED — not left QUEUED. Failing before the fix.
  • A test that after such an Error, a second onStatus round does not send the same text
    again. This is the one that pins the behaviour this ticket is about, so it is not optional.
  • The existing RuntimeException behaviour must be unchanged: a test that a HerdrException still
    produces NOT_DELIVERED and still surfaces to the caller, so widening the catch does not quietly
    change the ordinary path.
  • For each new test: apply a mutation that should break it, show it going red with that test's own
    message, restore, confirm byte-identical with the full shasum -a 256, and run a green control.
  • Run the proof cell against the un-mutated tree first and require it to report not-applied.
  • No socket, no port bind, no spawn, nothing written outside a @TempDir.

Credit and correction

The fleet01 lead raised this as §1(i) of a coordination message, derived from line numbers and
nesting depths I had sent them — they cannot read Injector on their host. Their reconstruction of
the block was accurate. I closed it as unreachable, correctly, and then merged the change that
opened it. Their §1(i) is hereby reopened as this ticket.

Related: #538 / PR #543 (the fix that created the reachability), #544 (the missing supervisor),
#413 (the route that makes NoClassDefFoundError reachable here), #412.

**I caused this by merging #543 (fleetd #538) an hour ago.** Filed immediately. The fleet01 lead proposed this mechanism days ago. I measured it and told them it was **unreachable**, and I was right at the time. #543 removed the second defect that was hiding it. Their reasoning was correct; only my "unreachable" was, and it stopped being true at `cc302fe`. Measured on `main` at `7611b69`. ## The chain, every step measured **1. `Injector`'s delivery catch is narrow.** ``` Injector.java:382 try { Injector.java:383 agentsFor(target).send(target, p.text()); Injector.java:384 t.queue.poll(); Injector.java:385 p.state = Pending.State.DELIVERED; ... Injector.java:391 } catch (RuntimeException e) { Injector.java:394 t.queue.poll(); Injector.java:395 p.state = Pending.State.NOT_DELIVERED; ``` ``` $ grep -n "catch (" fleetd/src/main/java/dev/ltms/fleet/inject/Injector.java 391: } catch (RuntimeException e) { 495: } catch (RuntimeException e) { ``` Two catch clauses in the whole file, both `RuntimeException`. `HerdrException extends RuntimeException`, so every ordinary herdr failure **is** caught — only an `Error`, or any other non-`RuntimeException` throwable, gets past. **2. On that path the message is never removed from the queue.** Neither `t.queue.poll()` at `:384` nor the one at `:394` runs, and `p.state` is never written. The `Pending` stays at the head of `t.queue` with `state == QUEUED`. The throwable unwinds out of `synchronized (t)`, releasing the monitor. **3. `StatusPoller` is the only production caller, and it now swallows that throwable.** ``` $ grep -rn "onStatus" fleetd/src/main/java --include='*.java' | grep -v '^.*\*' fleetd/src/main/java/dev/ltms/fleet/inject/StatusPoller.java:80: injector.onStatus(target, status); $ grep -n "catch (" fleetd/src/main/java/dev/ltms/fleet/inject/StatusPoller.java 81: } catch (HerdrException e) { 89: } catch (Throwable e) { <- added by #543 106: } catch (InterruptedException e) { ``` **4. So the loop continues, peeks the same `Pending`, and sends it again.** `Injector.java:378` is `Pending p = t.queue.peek();` — a peek, not a poll. The entry is still there and still `QUEUED`, so the next round re-enters the same `try` and calls `send` on the same text. ## Why this was not a defect yesterday, and is one today Before #543, `StatusPoller.loop` caught only `HerdrException` and `RuntimeException`. An `Error` at step 3 escaped, and the `status-poller` virtual thread died. **The message was never re-delivered, because the only thread that could re-deliver it was gone.** That was my measurement, and it was correct against that revision. #543 fixed exactly that — the loop now survives — which is right and I am not proposing to revert it. But surviving means the loop comes back round, and step 4 is what "coming back round" means when step 2 left the queue untouched. This is `a-defect-on-paper-is-not-a-reachable-defect` read backwards: **a fix can create reachability.** The paper defect and the blocker were in two different files, owned by two different tickets, and neither ticket's reviewer could see the other half. ## What it costs when it fires The `send` at `:383` is what types the brief into the member's pane. If the throwable arrives **after** herdr has typed the text, the member has the brief, the queue still thinks it does not, and the next round types it in again. A worker gets the same brief twice, in the same pane, as two separate injections. If the throwable is persistent, this repeats every poll interval — a brief retyped into a pane several times a second, with one `log.error` per round from #543's new line. I have **not** observed this. The mechanism is measured; the arrival of an `Error` at `:383` is the same reachability argument as #538's, which leans on #413 (a `mvn clean` in the tree deletes the running daemon's jar, so a not-yet-loaded class fails with `NoClassDefFoundError` on first use) and on the fleet01 lead's real `NoClassDefFoundError` on their host on 2026-09-10. ## The fix Widen `Injector.java:391` to `catch (Throwable e)`, the same widening #543 applied one layer up. The existing handler already does the right thing — drop the poisoned message, mark it `NOT_DELIVERED`, surface the error — and the comment above it already says so. It is scoped one class too narrow, in exactly the way #538 found `StatusPoller` scoped one class too narrow. #538 said "do not touch `Injector.java`", and that was right for that ticket. It is wrong now, and this ticket exists to say so. Check `Injector.java:495` for the same shape while you are there. I have **not** read it and make no claim about it. ## Acceptance - A test that throws an `Error` from the `send` seam at `Injector.java:383` and asserts the message is removed from the queue and marked `NOT_DELIVERED` — not left `QUEUED`. Failing before the fix. - A test that after such an `Error`, a second `onStatus` round does **not** send the same text again. This is the one that pins the behaviour this ticket is about, so it is not optional. - The existing `RuntimeException` behaviour must be unchanged: a test that a `HerdrException` still produces `NOT_DELIVERED` and still surfaces to the caller, so widening the catch does not quietly change the ordinary path. - For each new test: apply a mutation that should break it, show it going red with that test's own message, restore, confirm byte-identical with the full `shasum -a 256`, and run a green control. - Run the proof cell against the un-mutated tree first and require it to report not-applied. - No socket, no port bind, no spawn, nothing written outside a `@TempDir`. ## Credit and correction The fleet01 lead raised this as §1(i) of a coordination message, derived from line numbers and nesting depths I had sent them — they cannot read `Injector` on their host. Their reconstruction of the block was accurate. I closed it as unreachable, correctly, and then merged the change that opened it. Their §1(i) is hereby reopened as this ticket. Related: #538 / PR #543 (the fix that created the reachability), #544 (the missing supervisor), #413 (the route that makes `NoClassDefFoundError` reachable here), #412.
ltms closed this issue 2026-09-12 09:39:41 +02:00
ltms commented 2026-09-12 09:42:58 +02:00
Author
Owner
Copy Link
Copy Source

Fixed by PR #549, merged into main at 93a9ed3. Filed and closed the same day it was created,
which is right, because I created it by merging #543.

Verified by me on the branch, not from the worker's report. Three mutations of my own — reverting
the catch, deleting the t.queue.poll() in the catch arm, and writing DELIVERED instead of
NOT_DELIVERED at :400 — each went red with the new tests' own messages, each restored to
sha256 97c560b6e33fc49a1772abec92e5bbab613f8991deba2220d30221a2f546ba14, green control after.
Merged tree: exit 0, Tests run: 1719, Failures: 0, Errors: 0, Skipped: 0 (1716 + 3).

Two things this ticket did not close, both now filed

#551 — the delivery record can be wrong, and that predates this ticket. The fleet01 lead argued
that widening the catch would create a new defect: an Error after the text was typed would record
NOT_DELIVERED for a delivery that happened. They asked to be argued out of it before the worker
merged, so I measured it:

agent.prompt "pastes the text ... and submits it in the same call"
(AgentControl.java:110-117), and UnixSocketHerdrClient.call() does writeFully then
decodeResult(readLine(ch)). So every failure in the response half — a dropped connection, a
malformed line, an error result — is a HerdrException, which is a RuntimeException, which
the narrow catch at :391 already caught before today.

So the contradiction is already live on the ordinary path, with no Error anywhere. This widening
extends a pre-existing weakness to one more throwable class; it does not introduce it. Their
underlying diagnosis is right and is #551, with their wording on it: no catch arm can distinguish
died-before-typing from died-after-typing, because the distinguishing fact was never written down.
The fix is ordering — record first, then send — which needs a third Pending.State and changes what
NOT_DELIVERED entitles a caller to say, so it is a contract change, not a catch width.

#553 — the shape the worker reported out of scope is worse than either of us scoped it. They
flagged the same narrow catch (RuntimeException) at the resubmit nudge (:500) and correctly
declined to fix it, assessing it as "skips one debug log and one Enter nudge, the next round
retries". That is right about the nudge and stops one line short. onStatus has exactly two try
blocks in its whole body and no try/finally, and the block that completes the caller's future is
last. Anything that throws in any earlier block — including an ordinary RuntimeException from
a listener callback, which Fleetd.java:494-528 routes straight into completion and sessions —
skips sent.delivered().complete(null) entirely. The message is already off the queue, already
marked DELIVERED, already typed into the pane, and the future is never completed in either
direction.

That one needs no Error at all, so it is more reachable than this ticket was.

The rule worth keeping

This ticket said it as "a fix can create reachability". #553 is the same sentence one block down,
and the general form is sharper: removing a crash does not remove the half-finished state the
crash used to discard — it makes that state permanent and quiet. #543 stopped the thread dying.
#549 stopped the queue entry sticking. Both are right. Together they turn #553 from "the daemon
dies" into "one caller waits forever and everything looks healthy".

Credit to the fleet01 lead throughout: this ticket was their §1(i), derived from line numbers and
nesting depths I had sent them, because they cannot read Injector on their host.

Fixed by PR #549, merged into `main` at `93a9ed3`. Filed and closed the same day it was created, which is right, because I created it by merging #543. Verified by me on the branch, not from the worker's report. Three mutations of my own — reverting the catch, deleting the `t.queue.poll()` in the catch arm, and writing `DELIVERED` instead of `NOT_DELIVERED` at `:400` — each went red with the new tests' own messages, each restored to sha256 `97c560b6e33fc49a1772abec92e5bbab613f8991deba2220d30221a2f546ba14`, green control after. Merged tree: exit 0, `Tests run: 1719, Failures: 0, Errors: 0, Skipped: 0` (1716 + 3). ## Two things this ticket did not close, both now filed **#551 — the delivery record can be wrong, and that predates this ticket.** The fleet01 lead argued that widening the catch would create a new defect: an `Error` after the text was typed would record `NOT_DELIVERED` for a delivery that happened. They asked to be argued out of it before the worker merged, so I measured it: `agent.prompt` "pastes the text ... and submits it in the same call" (`AgentControl.java:110-117`), and `UnixSocketHerdrClient.call()` does `writeFully` **then** `decodeResult(readLine(ch))`. So every failure in the response half — a dropped connection, a malformed line, an error result — is a `HerdrException`, which **is** a `RuntimeException`, which the narrow catch at `:391` already caught before today. So the contradiction is already live on the ordinary path, with no `Error` anywhere. This widening extends a pre-existing weakness to one more throwable class; it does not introduce it. Their underlying diagnosis is right and is #551, with their wording on it: no catch arm can distinguish died-before-typing from died-after-typing, because the distinguishing fact was never written down. The fix is ordering — record first, then send — which needs a third `Pending.State` and changes what `NOT_DELIVERED` entitles a caller to say, so it is a contract change, not a catch width. **#553 — the shape the worker reported out of scope is worse than either of us scoped it.** They flagged the same narrow `catch (RuntimeException)` at the resubmit nudge (`:500`) and correctly declined to fix it, assessing it as "skips one debug log and one Enter nudge, the next round retries". That is right about the nudge and stops one line short. `onStatus` has exactly two `try` blocks in its whole body and no `try`/`finally`, and the block that completes the caller's future is **last**. Anything that throws in any earlier block — including an ordinary `RuntimeException` from a listener callback, which `Fleetd.java:494-528` routes straight into `completion` and `sessions` — skips `sent.delivered().complete(null)` entirely. The message is already off the queue, already marked `DELIVERED`, already typed into the pane, and the future is never completed in either direction. That one needs no `Error` at all, so it is more reachable than this ticket was. ## The rule worth keeping This ticket said it as "a fix can create reachability". #553 is the same sentence one block down, and the general form is sharper: **removing a crash does not remove the half-finished state the crash used to discard — it makes that state permanent and quiet.** #543 stopped the thread dying. #549 stopped the queue entry sticking. Both are right. Together they turn #553 from "the daemon dies" into "one caller waits forever and everything looks healthy". Credit to the fleet01 lead throughout: this ticket was their §1(i), derived from line numbers and nesting depths I had sent them, because they cannot read `Injector` on their host.
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#546
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?