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

MessageService's TIMED_OUT_QUEUED javadoc says the message is still queued; the code 12 lines of behaviour later cancels it #513

New Issue
Closed
opened 2026-09-12 05:56:51 +02:00 by ltms · 3 comments
ltms commented 2026-09-12 05:56:51 +02:00
Owner
Copy Link
Copy Source

A documentation defect, not a behaviour defect. I am filing it because it cost me a wrong conclusion today and would have cost a ticket, and because the wrong text is the one a reader reaches first.

Two statements in one file, and they disagree

fleetd/src/main/java/dev/ltms/fleet/msg/MessageService.java:302 — javadoc on the queuedDeliveries field:

the message never reached the Injector delivery window before the caller's deadline, so it is still sitting in the injector's own per-target queue

MessageService.java:395 — javadoc on hasQueuedDelivery:

the message is still sitting in the injector's per-target queue waiting for the worker to go idle

MessageService.java:952-956 — the code that actually runs on that timeout:

wasDelivered = injector.cancel(delivery) == Injector.Cancellation.DELIVERED;
...
if (!wasDelivered) {
    // CB-640: record that delivery did not happen for fleet health (see
    // queuedDeliveries). The exact Pending was cancelled, so it cannot arrive later.
    queuedDeliveries.put(target, Boolean.TRUE);
}

The inline comment is the correct one. Injector.cancel at inject/Injector.java:270-284 does exactly what it says:

synchronized (t) {
    if (p.state != Pending.State.QUEUED || !t.queue.remove(p)) {
        return cancellationOf(p);
    }
    p.state = Pending.State.CANCELLED;
    ...
    return Cancellation.CANCELLED;
}

t.queue.remove(p). The pending is gone. It is also tested — InjectorTest.java:181 asserts Cancellation.CANCELLED, and :196 asserts DELIVERED for the race where delivery wins.

So the two javadoc sentences describe behaviour that CB-640 removed. They are not vague, they are wrong in the specific direction that matters.

Why this is worth a ticket rather than a quiet edit

The behaviour they describe used to be real. I hit it on 2026-08-16: a ticket reported failed / no reply — timed_out_queued, the member was alive and had committed 74 seconds earlier, and the queued brief was delivered later and restarted the same ticket on top of finished work.

Today I was about to file that as a live defect — a timed-out ticket not cancelling its queued delivery, with a write-after-expiry consequence, on a peer lead's encouragement. I read the code first and found the opposite. What made me confident enough to nearly skip that step was this javadoc: it agrees with my memory of the old behaviour, so I had two sources saying the same thing and no reason to look further.

That is the shape: a stale doc is most dangerous when it agrees with a stale memory, because the reader gets a false confirmation instead of a contradiction. One of the two would have made me check. Two agreeing made me stop.

It is also the same family as #500 and #511 — a wrong stated fact stopping the next reader looking further — but at the doc layer, where nothing can fail and no test can catch it.

The fix

Rewrite both javadoc blocks to say what CB-640 actually left behind. The correct meaning of TIMED_OUT_QUEUED is now:

The message was cancelled before the injector ever delivered it. It is not queued and it will not arrive later. The target never saw it.

Say plainly that queuedDeliveries records a fact about a message that is gone, not a pointer to something pending. The field name is now misleading too — "queued" is precisely what it is not — so consider renaming it in the same change. Judge that yourself; the rename is optional and the text is not.

Keep and keep clear the distinction that is still true and still valuable:

  • TIMED_OUT_WORKING — delivered; the member has it and only the reply is outstanding.
  • TIMED_OUT_QUEUED — never delivered, and now cancelled; the member has not seen a word of it.

Related, and deliberately not in scope

The phase word failed for a message that was never delivered is a separate readability problem — failed there does not mean the work failed. Not filing it here; this ticket is only the contradiction.

Acceptance

  • The two javadoc blocks at :302 and :395 describe cancellation, not queueing.
  • grep -n 'still sitting in the injector' fleetd/src/main/java returns nothing.
  • No behaviour change. mvn -f fleetd/pom.xml clean install still passes — report the Tests run / Failures / Errors / Skipped line verbatim, and do not pipe it, because a pipe hides a failure behind a zero exit.
  • If you rename queuedDeliveries, the rename must be complete: no caller, test, or comment left using the old name.
A documentation defect, not a behaviour defect. I am filing it because it cost me a wrong conclusion today and would have cost a ticket, and because the wrong text is the one a reader reaches first. ## Two statements in one file, and they disagree `fleetd/src/main/java/dev/ltms/fleet/msg/MessageService.java:302` — javadoc on the `queuedDeliveries` field: > the message never reached the `Injector` delivery window before the caller's deadline, so **it is still sitting in the injector's own per-target queue** `MessageService.java:395` — javadoc on `hasQueuedDelivery`: > the message **is still sitting in the injector's per-target queue waiting for the worker to go idle** `MessageService.java:952-956` — the code that actually runs on that timeout: ```java wasDelivered = injector.cancel(delivery) == Injector.Cancellation.DELIVERED; ... if (!wasDelivered) { // CB-640: record that delivery did not happen for fleet health (see // queuedDeliveries). The exact Pending was cancelled, so it cannot arrive later. queuedDeliveries.put(target, Boolean.TRUE); } ``` The inline comment is the correct one. `Injector.cancel` at `inject/Injector.java:270-284` does exactly what it says: ```java synchronized (t) { if (p.state != Pending.State.QUEUED || !t.queue.remove(p)) { return cancellationOf(p); } p.state = Pending.State.CANCELLED; ... return Cancellation.CANCELLED; } ``` `t.queue.remove(p)`. The pending is gone. It is also tested — `InjectorTest.java:181` asserts `Cancellation.CANCELLED`, and `:196` asserts `DELIVERED` for the race where delivery wins. So the two javadoc sentences describe behaviour that CB-640 removed. They are not vague, they are wrong in the specific direction that matters. ## Why this is worth a ticket rather than a quiet edit The behaviour they describe used to be real. I hit it on 2026-08-16: a ticket reported `failed` / `no reply — timed_out_queued`, the member was alive and had committed 74 seconds earlier, and the queued brief was delivered later and restarted the same ticket on top of finished work. Today I was about to file that as a live defect — a timed-out ticket not cancelling its queued delivery, with a write-after-expiry consequence, on a peer lead's encouragement. I read the code first and found the opposite. What made me confident enough to nearly skip that step was this javadoc: it agrees with my memory of the old behaviour, so I had two sources saying the same thing and no reason to look further. That is the shape: **a stale doc is most dangerous when it agrees with a stale memory**, because the reader gets a false confirmation instead of a contradiction. One of the two would have made me check. Two agreeing made me stop. It is also the same family as #500 and #511 — a wrong stated fact stopping the next reader looking further — but at the doc layer, where nothing can fail and no test can catch it. ## The fix Rewrite both javadoc blocks to say what CB-640 actually left behind. The correct meaning of `TIMED_OUT_QUEUED` is now: > The message was cancelled before the injector ever delivered it. It is not queued and it will not arrive later. The target never saw it. Say plainly that `queuedDeliveries` records a **fact about a message that is gone**, not a pointer to something pending. The field name is now misleading too — "queued" is precisely what it is not — so consider renaming it in the same change. Judge that yourself; the rename is optional and the text is not. Keep and keep clear the distinction that is still true and still valuable: - `TIMED_OUT_WORKING` — delivered; the member has it and only the reply is outstanding. - `TIMED_OUT_QUEUED` — never delivered, and now cancelled; the member has not seen a word of it. ## Related, and deliberately not in scope The phase word `failed` for a message that was never delivered is a separate readability problem — `failed` there does not mean the work failed. Not filing it here; this ticket is only the contradiction. ## Acceptance - The two javadoc blocks at `:302` and `:395` describe cancellation, not queueing. - `grep -n 'still sitting in the injector' fleetd/src/main/java` returns nothing. - No behaviour change. `mvn -f fleetd/pom.xml clean install` still passes — report the Tests run / Failures / Errors / Skipped line verbatim, and do not pipe it, because a pipe hides a failure behind a zero exit. - If you rename `queuedDeliveries`, the rename must be complete: no caller, test, or comment left using the old name.
ltms commented 2026-09-12 07:32:57 +02:00
Author
Owner
Copy Link
Copy Source

Re-scoping this: it is not a tidy-up, and I have the evidence of the harm

I filed this as a stale-comment cleanup. It then caused a concrete, measurable error — mine — within a day, so the severity line was wrong and I am correcting it.

The contradiction, measured on main at a6415f3

The same file makes two opposite claims about what TIMED_OUT_QUEUED means.

MessageService.java:306 (the queuedDeliveries field javadoc) and :395-397 (hasQueuedDelivery's javadoc):

the message never reached the Injector delivery window before the caller's deadline, so it is still sitting in the injector's own per-target queue

the message is still sitting in the injector's per-target queue waiting for the worker to go idle

MessageService.java:954-957, the inline comment in the catch (TimeoutException) branch that actually produces the outcome:

wasDelivered = injector.cancel(delivery) == Injector.Cancellation.DELIVERED;
...
if (!wasDelivered) {
    // CB-640: record that delivery did not happen for fleet health (see
    // queuedDeliveries). The exact Pending was cancelled, so it cannot arrive later.
    queuedDeliveries.put(target, Boolean.TRUE);
}

The inline comment is the correct one. Injector.cancel (Injector.java:270) removes the entry:

synchronized (t) {
    if (p.state != Pending.State.QUEUED || !t.queue.remove(p)) {
        return cancellationOf(p);
    }
    p.state = Pending.State.CANCELLED;
    if (isQuiescent(t)) {
        targets.remove(p.target, t);
    }
    return Cancellation.CANCELLED;
}

So on the ordinary path a timed-out queued send is cancelled and cannot be delivered later. The two javadocs describe behaviour from before CB-640's cancel was added.

What the stale javadoc actually cost

I was about to file a ticket titled, roughly, "a timed-out ticket does not cancel its queued delivery — a dead brief is delivered late and its side effects land twice." I had a severity argument ready: a brief that edits files or opens a PR gets applied a second time against a tree it was not written for, after the caller has been told the ticket is terminal.

That defect does not exist. I derived it from this javadoc plus a memory of my own that was formed from the same older code. The fleet01 lead, reasoning from my description, agreed with it — and then correctly flagged their own agreement as derived rather than measured. I have since retracted the whole thing to them.

Two agreeing sources that are stale descendants of the same code cannot disagree with each other. They can only be stale together, and that feels exactly like corroboration. That is what makes this class of stale comment more expensive than it looks: it does not merely fail to inform, it manufactures false confirmation for a reader who already half-remembers the old behaviour.

So the fix is not just "correct the wording"

  1. Fix both javadocs at :302/:306 and :395-397 to say what the code does: the Pending is cancelled at timeout and cannot arrive later; TIMED_OUT_QUEUED records that delivery never happened, as a fact for fleet health, not that a delivery is still pending.
  2. Say why the name is misleading and keep the name. TIMED_OUT_QUEUED reads as "it is queued". It means "it was still queued when we gave up, and we then cancelled it." Renaming the enum constant is a wider change (it reaches FleetApp.java:645, which maps it to the REST string "queued", and FleetMcp.java:806); do not do it in this ticket. Do add one line at the declaration, MessageService.java:96, so the name is read correctly at the point it is defined.
  3. The REST surface says "queued" too (FleetApp.java:645). That is an operator-visible word for a message that has been cancelled. Flag it; do not change it here — it is an API string and needs its own decision.

One thing I am explicitly NOT claiming

There is a narrow window in cancel: if p.state == QUEUED but t.queue.remove(p) returns false, the method returns cancellationOf(p) → NOT_DELIVERED, so the caller reports TIMED_OUT_QUEUED while this call did not remove the Pending. Whether that state is reachable, and whether the delivery loop then completes the delivery, I have not measured. I am not filing it and nobody should treat it as a known defect. It is recorded here only so the next reader does not think the audit was exhaustive.

Relatedly, my original observation — a stale brief restarting a member that had already finished — was real, but my explanation of it was wrong. The pane scrape showed my brief in the pane, which means it had been typed in, i.e. delivered. That is the TIMED_OUT_WORKING path, not a queue entry firing late. The symptom stands; the mechanism I attributed it to does not.

Acceptance

  • The two javadocs no longer contradict :954-957. Quote both before and after.
  • One line at the TIMED_OUT_QUEUED declaration (:96) explaining what the name does and does not mean.
  • A list, not a fix, of every other place the word "queued" reaches an operator for this outcome.
  • No behaviour change. This ticket touches comments and at most one declaration's javadoc. If you believe the behaviour is wrong, say so in your report and stop — do not change it here.
## Re-scoping this: it is not a tidy-up, and I have the evidence of the harm I filed this as a stale-comment cleanup. It then caused a concrete, measurable error — mine — within a day, so the severity line was wrong and I am correcting it. ### The contradiction, measured on `main` at `a6415f3` The same file makes two opposite claims about what `TIMED_OUT_QUEUED` means. `MessageService.java:306` (the `queuedDeliveries` field javadoc) and `:395-397` (`hasQueuedDelivery`'s javadoc): > the message never reached the `Injector` delivery window before the caller's deadline, so **it is still sitting in the injector's own per-target queue** > > the message **is still sitting in the injector's per-target queue waiting for the worker to go idle** `MessageService.java:954-957`, the inline comment in the `catch (TimeoutException)` branch that actually produces the outcome: ```java wasDelivered = injector.cancel(delivery) == Injector.Cancellation.DELIVERED; ... if (!wasDelivered) { // CB-640: record that delivery did not happen for fleet health (see // queuedDeliveries). The exact Pending was cancelled, so it cannot arrive later. queuedDeliveries.put(target, Boolean.TRUE); } ``` **The inline comment is the correct one.** `Injector.cancel` (`Injector.java:270`) removes the entry: ```java synchronized (t) { if (p.state != Pending.State.QUEUED || !t.queue.remove(p)) { return cancellationOf(p); } p.state = Pending.State.CANCELLED; if (isQuiescent(t)) { targets.remove(p.target, t); } return Cancellation.CANCELLED; } ``` So on the ordinary path a timed-out queued send **is** cancelled and cannot be delivered later. The two javadocs describe behaviour from before CB-640's cancel was added. ### What the stale javadoc actually cost I was about to file a ticket titled, roughly, *"a timed-out ticket does not cancel its queued delivery — a dead brief is delivered late and its side effects land twice."* I had a severity argument ready: a brief that edits files or opens a PR gets applied a second time against a tree it was not written for, after the caller has been told the ticket is terminal. That defect does not exist. I derived it from this javadoc plus a memory of my own that was formed from the same older code. The fleet01 lead, reasoning from my description, agreed with it — and then correctly flagged their own agreement as derived rather than measured. I have since retracted the whole thing to them. Two agreeing sources that are **stale descendants of the same code cannot disagree with each other.** They can only be stale together, and that feels exactly like corroboration. That is what makes this class of stale comment more expensive than it looks: it does not merely fail to inform, it manufactures false confirmation for a reader who already half-remembers the old behaviour. ### So the fix is not just "correct the wording" 1. **Fix both javadocs** at `:302`/`:306` and `:395-397` to say what the code does: the Pending is cancelled at timeout and cannot arrive later; `TIMED_OUT_QUEUED` records that delivery *never happened*, as a fact for fleet health, not that a delivery is still pending. 2. **Say why the name is misleading and keep the name.** `TIMED_OUT_QUEUED` reads as "it is queued". It means "it was still queued when we gave up, and we then cancelled it." Renaming the enum constant is a wider change (it reaches `FleetApp.java:645`, which maps it to the REST string `"queued"`, and `FleetMcp.java:806`); do not do it in this ticket. Do add one line at the declaration, `MessageService.java:96`, so the name is read correctly at the point it is defined. 3. **The REST surface says `"queued"` too** (`FleetApp.java:645`). That is an operator-visible word for a message that has been cancelled. Flag it; do not change it here — it is an API string and needs its own decision. ### One thing I am explicitly NOT claiming There is a narrow window in `cancel`: if `p.state == QUEUED` but `t.queue.remove(p)` returns `false`, the method returns `cancellationOf(p)` → `NOT_DELIVERED`, so the caller reports `TIMED_OUT_QUEUED` while *this call* did not remove the Pending. Whether that state is reachable, and whether the delivery loop then completes the delivery, I have **not** measured. I am not filing it and nobody should treat it as a known defect. It is recorded here only so the next reader does not think the audit was exhaustive. Relatedly, my original observation — a stale brief restarting a member that had already finished — was real, but my explanation of it was wrong. The pane scrape showed my brief *in the pane*, which means it had been typed in, i.e. delivered. That is the `TIMED_OUT_WORKING` path, not a queue entry firing late. The symptom stands; the mechanism I attributed it to does not. ### Acceptance - The two javadocs no longer contradict `:954-957`. Quote both before and after. - One line at the `TIMED_OUT_QUEUED` declaration (`:96`) explaining what the name does and does not mean. - A list, not a fix, of every other place the word "queued" reaches an operator for this outcome. - No behaviour change. This ticket touches comments and at most one declaration's javadoc. If you believe the behaviour is wrong, say so in your report and stop — do not change it here.
ltms commented 2026-09-12 11:40:31 +02:00
Author
Owner
Copy Link
Copy Source

Rework needed on PR #564 — the new javadoc replaces a wrong claim with a narrower wrong claim

I verified the build on a tree merged with origin/main. That part is clean:

  • merge of worker/513-timed-out-queued-javadoc-da3628-3 into origin/main (4f9aba4) is a fast-forward, merged HEAD = ed28b51
  • mvn install → exit 0
  • 1744 tests, 0 failures, 0 errors, 0 skipped, summed from the 130 surefire report files (not from a piped grep — a pipe returns the pipe's exit code)
  • the new {@link Injector#cancel} links resolve. Positive control: mvn javadoc:javadoc reported 8 complaints inside MessageService.java (at :113, :144, :216, :277 — all pre-existing "no @param"), so the tool really did analyse that file and would have reported an unresolved reference. None of its complaints falls on a changed line. There is no maven-javadoc-plugin in fleetd/pom.xml, so javadoc is not a build gate here.

The problem is the content of the new comments.

TIMED_OUT_QUEUED has four routes, not one

The outcome is returned at MessageService.java:957-958 when wasDelivered is false. That needs injector.cancel(delivery) to return something other than DELIVERED — so either CANCELLED or NOT_DELIVERED.

# cancel returns how the Pending got there did cancel cancel anything?
1 CANCELLED (Injector.java:284) state was QUEUED, removed from t.queue yes
2 NOT_DELIVERED (Injector.java:289) Injector.java:400 — agentsFor(target).send(target, p.text()) threw no
3 NOT_DELIVERED Injector.java:419 — readiness grace expired (READINESS_GRACE_POLLS), never attempted no
4 NOT_DELIVERED Injector.java:691 — drop(target, cause), the target is gone, queue cleared no

On routes 2, 3 and 4 cancel() cancels nothing. It reads a state another code path already set and reports it (cancellationOf, Injector.java:288-290).

So two sentences are wrong, in three places

"send cancels the pending entry (Injector#cancel) before returning this outcome" — true on route 1 only. It names the mechanism of the case that motivated the ticket and states it as the invariant.

"the target never saw a word of it" — the code does not establish this on route 2. AgentControl.send calls herdr agent.prompt, which pastes the text into a live pane and submits it in one call. If that call throws after herdr has pasted, characters are already in the pane. I have not reproduced a partial paste, and I am not claiming it happens — I am saying the comment asserts something the code does not support, and it asserts it to an operator who would then not go and look at the pane.

Affected blocks on ed28b51: the TIMED_OUT_QUEUED enum constant (:93-99), queuedDeliveries (:307-314), hasQueuedDelivery (:400-408), and hasOrphanedDelegation (:426-430, where "no record of a cancelled, undelivered send" carries the milder form of the same over-claim).

What is actually true on all four routes

  1. the message will not arrive later, and it is not waiting in a queue — this is the correction the ticket asked for, and it is right
  2. the send ended with no confirmed delivery

That is what the javadoc should say. The mechanism sentence should either be dropped or list the routes. Route 2 needs its own qualifier: delivery was attempted and the herdr call failed, so nothing here proves the pane stayed clean.

Also fix the inline comment that seeded this

MessageService.java:953-954:

// CB-640: record that delivery did not happen for fleet health (see
// queuedDeliveries). The exact Pending was cancelled, so it cannot arrive later.

"it cannot arrive later" is correct. "The exact Pending was cancelled" is the same route-1-only claim, and it is pre-existing — it is very likely where the wording in the new javadoc came from. Correct it in the same pass.

Acceptance criteria for the rework

  1. No comment in the diff may say cancel cancelled the entry, unless it is qualified to the case where cancel returns CANCELLED.
  2. No comment may claim the target saw nothing. State the two facts that hold on every route instead.
  3. Name route 2 explicitly somewhere — a reader needs to know that "not delivered" can mean "we tried and the herdr call failed", not only "we pulled it out of the queue in time".
  4. Fix MessageService.java:953-954 too.
  5. Re-run mvn install and report the exit code next to the test count, with the count taken from the surefire reports rather than a pipe.

Why this is worth a second pass rather than a merge

This ticket exists because a comment stated one path's behaviour as the invariant, and a reader acted on it. The fix does the same thing again, one path further along. The general form is worth keeping in mind beyond this diff: we write a rule down at the moment an instance bites, so the instance's incidental details get recorded at full confidence. The check that catches it is to ask what case makes this exact wording give the wrong answer.

Credit where due: the diff is still a real improvement. The old text told an operator a message was coming that was never coming — wrong in the dangerous direction. This is wrong in a narrower one.

## Rework needed on PR #564 — the new javadoc replaces a wrong claim with a narrower wrong claim I verified the build on a tree merged with `origin/main`. That part is clean: - merge of `worker/513-timed-out-queued-javadoc-da3628-3` into `origin/main` (`4f9aba4`) is a fast-forward, merged `HEAD` = `ed28b51` - `mvn install` → **exit 0** - **1744 tests, 0 failures, 0 errors, 0 skipped**, summed from the 130 surefire report files (not from a piped grep — a pipe returns the pipe's exit code) - the new `{@link Injector#cancel}` links resolve. Positive control: `mvn javadoc:javadoc` reported 8 complaints inside `MessageService.java` (at `:113`, `:144`, `:216`, `:277` — all pre-existing "no @param"), so the tool really did analyse that file and would have reported an unresolved reference. None of its complaints falls on a changed line. There is no `maven-javadoc-plugin` in `fleetd/pom.xml`, so javadoc is not a build gate here. **The problem is the content of the new comments.** ### `TIMED_OUT_QUEUED` has four routes, not one The outcome is returned at `MessageService.java:957-958` when `wasDelivered` is false. That needs `injector.cancel(delivery)` to return something other than `DELIVERED` — so either `CANCELLED` or `NOT_DELIVERED`. | # | `cancel` returns | how the `Pending` got there | did `cancel` cancel anything? | |---|---|---|---| | 1 | `CANCELLED` (`Injector.java:284`) | state was `QUEUED`, removed from `t.queue` | **yes** | | 2 | `NOT_DELIVERED` (`Injector.java:289`) | `Injector.java:400` — `agentsFor(target).send(target, p.text())` **threw** | no | | 3 | `NOT_DELIVERED` | `Injector.java:419` — readiness grace expired (`READINESS_GRACE_POLLS`), never attempted | no | | 4 | `NOT_DELIVERED` | `Injector.java:691` — `drop(target, cause)`, the target is gone, queue cleared | no | On routes 2, 3 and 4 `cancel()` cancels nothing. It reads a state another code path already set and reports it (`cancellationOf`, `Injector.java:288-290`). ### So two sentences are wrong, in three places **"`send` cancels the pending entry (`Injector#cancel`) before returning this outcome"** — true on route 1 only. It names the mechanism of the case that motivated the ticket and states it as the invariant. **"the target never saw a word of it"** — the code does not establish this on route 2. `AgentControl.send` calls herdr `agent.prompt`, which pastes the text into a live pane and submits it in one call. If that call throws *after* herdr has pasted, characters are already in the pane. I have not reproduced a partial paste, and I am not claiming it happens — I am saying the comment asserts something the code does not support, and it asserts it to an operator who would then not go and look at the pane. Affected blocks on `ed28b51`: the `TIMED_OUT_QUEUED` enum constant (`:93-99`), `queuedDeliveries` (`:307-314`), `hasQueuedDelivery` (`:400-408`), and `hasOrphanedDelegation` (`:426-430`, where "no record of a **cancelled**, undelivered send" carries the milder form of the same over-claim). ### What is actually true on all four routes 1. the message will **not** arrive later, and it is **not** waiting in a queue — this is the correction the ticket asked for, and it is right 2. the send ended with **no confirmed delivery** That is what the javadoc should say. The mechanism sentence should either be dropped or list the routes. Route 2 needs its own qualifier: delivery was **attempted** and the herdr call failed, so nothing here proves the pane stayed clean. ### Also fix the inline comment that seeded this `MessageService.java:953-954`: ```java // CB-640: record that delivery did not happen for fleet health (see // queuedDeliveries). The exact Pending was cancelled, so it cannot arrive later. ``` "it cannot arrive later" is correct. "The exact `Pending` was cancelled" is the same route-1-only claim, and it is pre-existing — it is very likely where the wording in the new javadoc came from. Correct it in the same pass. ### Acceptance criteria for the rework 1. No comment in the diff may say `cancel` cancelled the entry, unless it is qualified to the case where `cancel` returns `CANCELLED`. 2. No comment may claim the target saw nothing. State the two facts that hold on every route instead. 3. Name route 2 explicitly somewhere — a reader needs to know that "not delivered" can mean "we tried and the herdr call failed", not only "we pulled it out of the queue in time". 4. Fix `MessageService.java:953-954` too. 5. Re-run `mvn install` and report the **exit code** next to the test count, with the count taken from the surefire reports rather than a pipe. ### Why this is worth a second pass rather than a merge This ticket exists because a comment stated one path's behaviour as the invariant, and a reader acted on it. The fix does the same thing again, one path further along. The general form is worth keeping in mind beyond this diff: **we write a rule down at the moment an instance bites, so the instance's incidental details get recorded at full confidence.** The check that catches it is to ask what case makes this exact wording give the wrong answer. Credit where due: the diff is still a real improvement. The old text told an operator a message was coming that was never coming — wrong in the dangerous direction. This is wrong in a narrower one.
ltms closed this issue 2026-09-12 11:48:54 +02:00
ltms commented 2026-09-12 11:49:32 +02:00
Author
Owner
Copy Link
Copy Source

Merged as f4f5f31

PR #564 merged, branch deleted. Verified on a tree merged with origin/main before the merge, not on the branch alone.

The rework answers all five criteria

# criterion result
1 no unqualified "cancel cancelled the entry" every mention is now scoped to Injector.Cancellation#CANCELLED, with NOT_DELIVERED named as the other case
2 no unqualified "the target saw nothing" replaced by the two facts true on every route; "saw nothing" now attaches only to CANCELLED
3 route 2 named explicitly "the injector's call to the target's terminal (AgentControl#send) threw" / "a failed delivery attempt can leave a partial paste behind"
4 fix MessageService.java:953-954 done — the inline comment now covers both cases
5 exit code beside a surefire-sourced count done

My own verification

  • merge into origin/main (4f9aba4) is a fast-forward to cb64bc8
  • mvn install → exit 0, BUILD SUCCESS
  • 1744 tests, 0 failures, 0 errors, 0 skipped — from two independent sources that agree: Maven's own summary line, and a sum over the 130 surefire-reports/*.txt files
  • new-text and old-text greps run in both directions on the merged tree, with TIMED_OUT_QUEUED → 5 as the positive control. the target never saw a word of it → 0 and The exact Pending was cancelled → 0
  • javadoc link check on the three new references (Injector.Cancellation#CANCELLED, Injector.Cancellation#NOT_DELIVERED, dev.ltms.fleet.herdr.AgentControl#send): no unresolved reference. Positive control: javadoc analysed MessageService.java and emitted its 8 pre-existing no @param / no main description complaints, with line numbers shifted by exactly the +6 lines the diff adds above them. The 6 unknown tag errors in the log are all in session/Worktrees.java, a file this diff does not touch.

Two notes worth keeping

The worker flagged the one check it could not complete rather than claiming it — it had not re-run the javadoc control on cb64bc8 and said so. That is the right call and it is why the gap got closed instead of assumed.

On my own side, a grep of mine returned a false zero while confirming the rework: the string I searched for spans a javadoc line wrap, and grep is line-based, so it could never match. Caught by a positive control. The general form is already in my notes — a multi-line string cannot be found by a line-oriented search — and it fired again here one turn after I wrote it down.

Why this needed two passes

The first pass replaced a wrong claim with a narrower wrong claim, which is the same defect this ticket was filed to fix, one path further along. The cause is worth naming, and it came from the fleet01 lead:

We write the rule down at the moment an instance bites, so the instance's incidental features get baked in at full confidence.

The check that catches it is mechanical rather than creative: take the originating instance and invert one feature at a time. Here the originating instance was "the send timed out and we cancelled it in time". Inverting when the fate was decided — before the call instead of by it — produces the other three routes immediately.

Closing.

## Merged as `f4f5f31` PR #564 merged, branch deleted. Verified on a tree merged with `origin/main` before the merge, not on the branch alone. ### The rework answers all five criteria | # | criterion | result | |---|---|---| | 1 | no unqualified "`cancel` cancelled the entry" | every mention is now scoped to `Injector.Cancellation#CANCELLED`, with `NOT_DELIVERED` named as the other case | | 2 | no unqualified "the target saw nothing" | replaced by the two facts true on every route; "saw nothing" now attaches only to `CANCELLED` | | 3 | route 2 named explicitly | "the injector's call to the target's terminal (`AgentControl#send`) threw" / "a failed delivery attempt can leave a partial paste behind" | | 4 | fix `MessageService.java:953-954` | done — the inline comment now covers both cases | | 5 | exit code beside a surefire-sourced count | done | ### My own verification - merge into `origin/main` (`4f9aba4`) is a fast-forward to `cb64bc8` - `mvn install` → **exit 0**, `BUILD SUCCESS` - **1744 tests, 0 failures, 0 errors, 0 skipped** — from two independent sources that agree: Maven's own summary line, and a sum over the 130 `surefire-reports/*.txt` files - new-text and old-text greps run in both directions on the merged tree, with `TIMED_OUT_QUEUED` → 5 as the positive control. `the target never saw a word of it` → 0 and `The exact Pending was cancelled` → 0 - **javadoc link check on the three new references** (`Injector.Cancellation#CANCELLED`, `Injector.Cancellation#NOT_DELIVERED`, `dev.ltms.fleet.herdr.AgentControl#send`): no unresolved reference. Positive control: javadoc analysed `MessageService.java` and emitted its 8 pre-existing `no @param` / `no main description` complaints, with line numbers shifted by exactly the +6 lines the diff adds above them. The 6 `unknown tag` errors in the log are all in `session/Worktrees.java`, a file this diff does not touch. ### Two notes worth keeping **The worker flagged the one check it could not complete** rather than claiming it — it had not re-run the javadoc control on `cb64bc8` and said so. That is the right call and it is why the gap got closed instead of assumed. **On my own side, a grep of mine returned a false zero** while confirming the rework: the string I searched for spans a javadoc line wrap, and `grep` is line-based, so it could never match. Caught by a positive control. The general form is already in my notes — a multi-line string cannot be found by a line-oriented search — and it fired again here one turn after I wrote it down. ### Why this needed two passes The first pass replaced a wrong claim with a narrower wrong claim, which is the same defect this ticket was filed to fix, one path further along. The cause is worth naming, and it came from the fleet01 lead: > We write the rule down at the moment an instance bites, so the instance's incidental features get baked in at full confidence. The check that catches it is mechanical rather than creative: **take the originating instance and invert one feature at a time.** Here the originating instance was "the send timed out and we cancelled it in time". Inverting *when the fate was decided* — before the call instead of by it — produces the other three routes immediately. Closing.
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#513
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?