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

fleet_ack says "acknowledged <msgId>" for a held peer message it never touches #437

New Issue
Closed
opened 2026-09-10 08:53:36 +02:00 by ltms · 3 comments
ltms commented 2026-09-10 08:53:36 +02:00
Owner
Copy Link
Copy Source

Found by the fleet01 lead over the coordination channel. I confirmed it here on two independent axes before filing. This is the same shape as #400 and #408: a receipt for work that did not happen.

What happens

fleet_ack{target, msgId} reports success for any msgId, including a peer message held in the coordination channel — which it has no code path to remove. The lead is told the message is acknowledged, and it stays held forever.

Axis 1 — the code has no path from ack to the held list

FleetMcp.ack is the whole implementation:

/** {@code fleet_ack}: acknowledge (remove) a specific reply from the inbox. */
static McpSchema.CallToolResult ack(MessageService messages, String target, String msgId) {
    if (isBlank(target) || isBlank(msgId)) {
        return error("target and msgId are required");
    }
    messages.ackReply(target, msgId);
    return text("acknowledged " + msgId);
}

Two things about those four lines:

  • messages.ackReply goes to the worker reply inbox, and returns void: 
    public void ackReply(String target, String msgId) {
    inbox.ack(target, msgId);
}
    There is no result to check, so ack could not tell a hit from a miss even if it wanted to.
  • Held peer mail is not in that inbox at all. It lives in the coordination channel. Grepping every ack in FleetMcp.java returns exactly one line — 928, the ackReply above. The leadChannel field is only ever published to and peeked: 
    791:  LeadMessage msg = new LeadMessage(UUID.randomUUID().toString(), leadChannel.selfCoordId(),
794:  leadChannel.publish(coordId, msg);
928:  messages.ackReply(target, msgId);

So the success string is unconditional, and the store it writes to is not the store the message is in.

Axis 2 — measured live on this daemon

I used a message I sent to myself, so a real peer message could not be lost if the ack turned out to work:

fleet_ack{target: "mac", msgId: "9f7d075f-4d6b-4eb8-aaf5-341ebeddcd34"}
-> "acknowledged 9f7d075f-4d6b-4eb8-aaf5-341ebeddcd34"

fleet_list -> coordinator.held[] still contains:
   {"msgId":"9f7d075f-4d6b-4eb8-aaf5-341ebeddcd34","from":"mac","preview":"[msgid probe, fleetd #421] …"}

The two axes differ in what they could be wrong about — one is a reading of the source, the other is the daemon's own behaviour — so they are two data points, not one.

Why this matters more than a wrong string

A lead's only way to manage held peer mail is to decide what it has dealt with. This tool answers that question with a confident yes in every case. So:

  • A lead that acks what it has processed builds a false model of its own mailbox. Nothing it acks ever leaves.
  • The held list only grows. Mine went from 8 entries to 11 while I worked, and one ack against it changed nothing.
  • The failure is silent and permanent. There is no error, no log line, and no field that disagrees.

The target parameter makes it worse in a quiet way. Its schema says "Worker session id whose inbox to ack from", and a coord-id is not a worker session id. So the call is passing a peer id into a parameter documented for something else, and still gets a success. Nothing validates that target names anything that exists.

Scope note — this is not #421, and should not be folded into it

#421 is about a lead being unable to read its held messages, and its fix adds an authz action for peeking. This ticket is about fleet_ack lying about a write. They touch the same surface and are separate defects: fixing the read gives a lead the bodies, and the ack would still report false success on every one of them. A worker is mid-turn on #421 right now; do not widen that scope.

What a fix has to decide

I am not prescribing the mechanism. The decision is what fleet_ack should mean when msgId names a held peer message, and there are two defensible answers:

  1. Make it work. Route a coord-id target to the coordination channel and remove the held message. This needs an answer to "what does removing a held message mean when nobody has read its body yet" — silently discarding unread peer mail is a worse defect than the one being fixed.
  2. Make it refuse. Return an error when msgId is not in the worker inbox for target. Cheaper, honest, and it turns a silent permanent failure into an immediate loud one.

Either way, the invariant is the one #400 and #408 are also about: do not report an effect you did not read back. ackReply returning void is the root of it — a fix that leaves the caller unable to tell a hit from a miss has not fixed anything.

Whatever is chosen, the target parameter's description needs to match it.

Found by the fleet01 lead over the coordination channel. I confirmed it here on two independent axes before filing. This is the same shape as #400 and #408: a receipt for work that did not happen. ## What happens `fleet_ack{target, msgId}` reports success for **any** `msgId`, including a peer message held in the coordination channel — which it has no code path to remove. The lead is told the message is acknowledged, and it stays held forever. ## Axis 1 — the code has no path from ack to the held list `FleetMcp.ack` is the whole implementation: ```java /** {@code fleet_ack}: acknowledge (remove) a specific reply from the inbox. */ static McpSchema.CallToolResult ack(MessageService messages, String target, String msgId) { if (isBlank(target) || isBlank(msgId)) { return error("target and msgId are required"); } messages.ackReply(target, msgId); return text("acknowledged " + msgId); } ``` Two things about those four lines: - `messages.ackReply` goes to the **worker reply inbox**, and returns `void`: ```java public void ackReply(String target, String msgId) { inbox.ack(target, msgId); } ``` There is no result to check, so `ack` could not tell a hit from a miss even if it wanted to. - Held peer mail is not in that inbox at all. It lives in the coordination channel. Grepping every ack in `FleetMcp.java` returns exactly one line — 928, the `ackReply` above. The `leadChannel` field is only ever published to and peeked: ``` 791: LeadMessage msg = new LeadMessage(UUID.randomUUID().toString(), leadChannel.selfCoordId(), 794: leadChannel.publish(coordId, msg); 928: messages.ackReply(target, msgId); ``` So the success string is unconditional, and the store it writes to is not the store the message is in. ## Axis 2 — measured live on this daemon I used a message I sent to myself, so a real peer message could not be lost if the ack turned out to work: ``` fleet_ack{target: "mac", msgId: "9f7d075f-4d6b-4eb8-aaf5-341ebeddcd34"} -> "acknowledged 9f7d075f-4d6b-4eb8-aaf5-341ebeddcd34" fleet_list -> coordinator.held[] still contains: {"msgId":"9f7d075f-4d6b-4eb8-aaf5-341ebeddcd34","from":"mac","preview":"[msgid probe, fleetd #421] …"} ``` The two axes differ in what they could be wrong about — one is a reading of the source, the other is the daemon's own behaviour — so they are two data points, not one. ## Why this matters more than a wrong string A lead's only way to manage held peer mail is to decide what it has dealt with. This tool answers that question with a confident yes in every case. So: - A lead that acks what it has processed builds a false model of its own mailbox. Nothing it acks ever leaves. - The held list only grows. Mine went from 8 entries to 11 while I worked, and one ack against it changed nothing. - The failure is silent and permanent. There is no error, no log line, and no field that disagrees. The `target` parameter makes it worse in a quiet way. Its schema says *"Worker session id whose inbox to ack from"*, and a coord-id is not a worker session id. So the call is passing a peer id into a parameter documented for something else, and still gets a success. Nothing validates that `target` names anything that exists. ## Scope note — this is not #421, and should not be folded into it #421 is about a lead being unable to **read** its held messages, and its fix adds an authz action for peeking. This ticket is about `fleet_ack` **lying** about a write. They touch the same surface and are separate defects: fixing the read gives a lead the bodies, and the ack would still report false success on every one of them. A worker is mid-turn on #421 right now; do not widen that scope. ## What a fix has to decide I am not prescribing the mechanism. The decision is what `fleet_ack` should mean when `msgId` names a held peer message, and there are two defensible answers: 1. **Make it work.** Route a coord-id `target` to the coordination channel and remove the held message. This needs an answer to "what does removing a held message mean when nobody has read its body yet" — silently discarding unread peer mail is a worse defect than the one being fixed. 2. **Make it refuse.** Return an error when `msgId` is not in the worker inbox for `target`. Cheaper, honest, and it turns a silent permanent failure into an immediate loud one. Either way, the invariant is the one #400 and #408 are also about: **do not report an effect you did not read back.** `ackReply` returning `void` is the root of it — a fix that leaves the caller unable to tell a hit from a miss has not fixed anything. Whatever is chosen, the `target` parameter's description needs to match it.
ltms commented 2026-09-10 11:34:06 +02:00
Author
Owner
Copy Link
Copy Source

Decision: option 2, refuse. But one premise in this ticket is wrong, and the correction matters.

I read the code myself before deciding, and axis 1 above is wrong on its load-bearing point.

The claim. "a peer message held in the coordination channel — which it has no code path to remove", supported by grepping every ack in FleetMcp.java and finding only line 928.

What is actually there. The grep was of the wrong file. ack is on the LeadChannel interface:

// LeadChannel.java:36-40
/**
 * Drop {@code msgId} from the held set and ack it on the broker. A repeated ack that this
 * ... report an ack that did not reach the broker.
 */
void ack(String msgId);

LeadMailbox.ack implements it, and it is careful code — it removes from held, calls basicAck, throws IllegalStateException("cannot ack lead message " + msgId + ": it is not held") for an unknown id, tolerates a duplicate ack through a bounded recentlyAcked set, and puts the entry back if the broker ack fails so the at-least-once contract holds. LeadCoordLoop.java:168 calls it.

So the removal path exists and is well built. The defect is narrower than filed: FleetMcp.ack never routes to it. It calls messages.ackReply — the worker reply inbox — whatever target names. That does not weaken the ticket; the wrong string and the permanent silent failure are both real, and I reproduced them. It changes what a fix costs, which is why I am writing it down.

Why I am still choosing refuse — a different reason than the one given

The argument above was "making it work needs an answer to discarding unread peer mail". That objection is weaker now than when it was written, because #421 shipped: fleet_poll{coordId} gives a lead the full held bodies without consuming them, so a lead can read before acking.

The stronger argument is in the delivery loop. LeadCoordLoop acks a held message as soon as it delivers it to the lead's pane:

:159  agents.send(lead, DELIVERY_FORMAT.formatted(msg.from(), msg.content()));
:166  rememberDelivered(msg.msgId());
:167  if (ackDelivered(msg)) {          // -> channel.ack(msg.msgId())

and it leaves a message unacked only in the three cases where delivery did not happen: no local lead pane (:135), the pane is not injectable (:147), or the pane write threw (:157). Each of those returns without acking, on purpose.

So everything in held[] is by definition mail the lead has not been shown. Mail the lead has been shown is already gone — the loop owns that ack. A manual fleet_ack on a held peer message can therefore only ever mean "throw away something nobody has read". There is no version of that call which is a normal inbox operation, so the honest answer is to refuse it rather than to build it.

If we later want "I read this by poll and I do not want it delivered again", that is a different capability and needs a different name — not fleet_ack. A destructive act must not be reachable by a lead that thinks it is tidying an inbox. Separate ticket if anyone wants it; I am not filing one yet.

What to build

The root cause is the one this ticket already names: ackReply returns void, so the caller cannot tell a hit from a miss.

  1. ReplyInbox.ack returns boolean — true when it removed an entry, false when there was nothing to remove. Update both implementations, InMemoryReplyInbox and AmqpReplyInbox. Keep the existing contract that an ack for a target you do not own is not an error; it just returns false.
  2. MessageService.ackReply returns that boolean unchanged.
  3. FleetMcp.ack returns an error when it is false, instead of "acknowledged " + msgId. The message must be useful, not just negative: say the id was not in that target's reply inbox, and say that held peer mail is not ackable and is read with fleet_poll{coordId}.
  4. drainReplies may ignore the boolean — it just peeked those ids, so a miss there is a race, not a caller error. Do not turn it into a throw.
  5. Fix the target parameter description. It says "Worker session id whose inbox to ack from" and nothing enforces it. Once a wrong target produces an error, the description and the behaviour finally agree.

The tests that must exist

  • Acking a real queued reply still succeeds and still removes it. This must not become a behaviour change for the worker path.
  • Acking an id that is not in the inbox returns an error, not "acknowledged".
  • Acking with a coord-id as target returns an error naming fleet_poll{coordId}.
  • FleetMcpTest.java:1385-1386 currently pins the defect and must be rewritten, not deleted: 
    // ackReply works (no-op since published with a different UUID, but callable).
assertDoesNotThrow(() -> messages.ackReply("term_a", msgId));
    That asserts the silent no-op is fine. It should now assert the call reports false.
  • The mutation that must fail: make FleetMcp.ack return text("acknowledged " + msgId) unconditionally again, ignoring the boolean. At least one test must go red. Paste the red run with the failing method name and the restored green run.

Out of scope

  • Do not add a way to drop held peer mail. That is the capability I decided against above.
  • Do not touch LeadMailbox.ack, LeadChannel.ack or LeadCoordLoop. They are correct; the bug is that nothing calls them from the MCP surface, and the decision is that nothing should.
  • Do not change drainReplies' ack ordering. Its javadoc explains the accepted loss window; that is a separate, deliberate trade.
## Decision: option 2, refuse. But one premise in this ticket is wrong, and the correction matters. I read the code myself before deciding, and axis 1 above is wrong on its load-bearing point. **The claim.** *"a peer message held in the coordination channel — which it has no code path to remove"*, supported by grepping every `ack` in `FleetMcp.java` and finding only line 928. **What is actually there.** The grep was of the wrong file. `ack` **is** on the `LeadChannel` interface: ```java // LeadChannel.java:36-40 /** * Drop {@code msgId} from the held set and ack it on the broker. A repeated ack that this * ... report an ack that did not reach the broker. */ void ack(String msgId); ``` `LeadMailbox.ack` implements it, and it is careful code — it removes from `held`, calls `basicAck`, **throws** `IllegalStateException("cannot ack lead message " + msgId + ": it is not held")` for an unknown id, tolerates a duplicate ack through a bounded `recentlyAcked` set, and puts the entry back if the broker ack fails so the at-least-once contract holds. `LeadCoordLoop.java:168` calls it. So the removal path exists and is well built. **The defect is narrower than filed: `FleetMcp.ack` never routes to it.** It calls `messages.ackReply` — the worker reply inbox — whatever `target` names. That does not weaken the ticket; the wrong string and the permanent silent failure are both real, and I reproduced them. It changes what a fix costs, which is why I am writing it down. ## Why I am still choosing refuse — a different reason than the one given The argument above was "making it work needs an answer to discarding unread peer mail". That objection is weaker now than when it was written, because #421 shipped: `fleet_poll{coordId}` gives a lead the full held bodies without consuming them, so a lead *can* read before acking. The stronger argument is in the delivery loop. `LeadCoordLoop` acks a held message **as soon as it delivers it to the lead's pane**: ``` :159 agents.send(lead, DELIVERY_FORMAT.formatted(msg.from(), msg.content())); :166 rememberDelivered(msg.msgId()); :167 if (ackDelivered(msg)) { // -> channel.ack(msg.msgId()) ``` and it leaves a message unacked only in the three cases where delivery did not happen: no local lead pane (`:135`), the pane is not injectable (`:147`), or the pane write threw (`:157`). Each of those returns without acking, on purpose. **So everything in `held[]` is by definition mail the lead has not been shown.** Mail the lead *has* been shown is already gone — the loop owns that ack. A manual `fleet_ack` on a held peer message can therefore only ever mean "throw away something nobody has read". There is no version of that call which is a normal inbox operation, so the honest answer is to refuse it rather than to build it. If we later want "I read this by poll and I do not want it delivered again", that is a **different capability and needs a different name** — not `fleet_ack`. A destructive act must not be reachable by a lead that thinks it is tidying an inbox. Separate ticket if anyone wants it; I am not filing one yet. ## What to build The root cause is the one this ticket already names: `ackReply` returns `void`, so the caller cannot tell a hit from a miss. 1. **`ReplyInbox.ack` returns `boolean`** — `true` when it removed an entry, `false` when there was nothing to remove. Update both implementations, `InMemoryReplyInbox` and `AmqpReplyInbox`. Keep the existing contract that an ack for a target you do not own is not an error; it just returns `false`. 2. **`MessageService.ackReply` returns that boolean** unchanged. 3. **`FleetMcp.ack` returns an error when it is `false`**, instead of `"acknowledged " + msgId`. The message must be useful, not just negative: say the id was not in that target's reply inbox, and say that held peer mail is not ackable and is read with `fleet_poll{coordId}`. 4. **`drainReplies` may ignore the boolean** — it just peeked those ids, so a miss there is a race, not a caller error. Do not turn it into a throw. 5. **Fix the `target` parameter description.** It says *"Worker session id whose inbox to ack from"* and nothing enforces it. Once a wrong `target` produces an error, the description and the behaviour finally agree. ## The tests that must exist - Acking a real queued reply still succeeds and still removes it. This must not become a behaviour change for the worker path. - Acking an id that is **not** in the inbox returns an error, not `"acknowledged"`. - Acking with a coord-id as `target` returns an error naming `fleet_poll{coordId}`. - **`FleetMcpTest.java:1385-1386` currently pins the defect** and must be rewritten, not deleted: ```java // ackReply works (no-op since published with a different UUID, but callable). assertDoesNotThrow(() -> messages.ackReply("term_a", msgId)); ``` That asserts the silent no-op is fine. It should now assert the call reports `false`. - **The mutation that must fail:** make `FleetMcp.ack` return `text("acknowledged " + msgId)` unconditionally again, ignoring the boolean. At least one test must go red. Paste the red run with the failing method name and the restored green run. ## Out of scope - Do not add a way to drop held peer mail. That is the capability I decided against above. - Do not touch `LeadMailbox.ack`, `LeadChannel.ack` or `LeadCoordLoop`. They are correct; the bug is that nothing calls them from the MCP surface, and the decision is that nothing should. - Do not change `drainReplies`' ack ordering. Its javadoc explains the accepted loss window; that is a separate, deliberate trade.
ltms commented 2026-09-10 11:59:28 +02:00
Author
Owner
Copy Link
Copy Source

Decision: option 2, refuse. Plus two corrections to my own text above.

The fix is in PR #448. Before that, two things in the body are wrong or too strong, and both change how the next reader should think about this ticket.

Correction 1 — "no code path to remove" is only true of FleetMcp, not of the channel

The channel does have an ack. Measured on main today (82fae94):

$ grep -n 'void \|boolean \|String \|List<' fleetd/src/main/java/dev/ltms/fleet/msg/LeadChannel.java
30:    void publish(String toCoordId, LeadMessage m);
33:    List<LeadMessage> peek();
40:    void ack(String msgId);
43:    String selfCoordId();
55:    boolean heldDurable();

LeadChannel.ack(String msgId) is right there at line 40. What is true is the narrower claim: FleetMcp never calls it. Wiring fleet_ack to the coordination channel is therefore a small change, not a missing mechanism.

That matters for how this ticket ages. Refusing is the right answer today, but it is a policy choice, not a structural impossibility. Those two age differently: a policy can be revisited when the pieces around it change, while an impossibility tells a future reader not to look. The fleet01 lead raised this, and I checked it in the code myself.

Correction 2 — the reason to refuse is the ack race, not "held means unread"

I wrote that removing a held message "when nobody has read its body yet" is the hard part. That premise is not always true. LeadCoordLoop writes the message to the lead's pane first, then acks it (main, today):

153:            agents.send(lead, DELIVERY_FORMAT.formatted(msg.from(), msg.content()));
160:        rememberDelivered(msg.msgId());
161:        if (ackDelivered(msg)) {

And ackDelivered can fail after that pane write succeeds:

private boolean ackDelivered(LeadMessage msg) {
    try {
        channel.ack(msg.msgId());
    } catch (RuntimeException e) {
        // Delivered but not acked: it will be redelivered, ...
        log.warn("lead coordination: delivered message {} but could not ack it: {}",
                msg.msgId(), e.toString());
        return false;
    }
    return true;
}

So there is a real window where a message the lead has already read is still in held[]. The loop heals this on its next tick, because line 127 re-acks anything wasDelivered already knows about, without showing it again. But inside that window, "everything held is unread" is false.

Write the reason this way instead: the delivery loop owns the ack, and a manual ack races it. That is accurate in every case, including the window above. The fleet01 lead found this exception and I confirmed it in the source.

The guard a future "make it work" would need

LeadCoordLoop.wasDelivered(String msgId) (line 179) is exactly the predicate a safe manual ack would have to consult. A manual ack is safe for a message that predicate already knows about, and unsafe for one it does not — that is the whole difference between dropping read mail and destroying unread mail. Anyone reopening option 1 should start there, not from scratch.

What #448 actually ships

ackReply changes from void to boolean, in the interface and both implementations. false means nothing was removed and is not an error. FleetMcp.ack then returns an error naming fleet_poll{coordId} as the way to read held peer mail, and the target schema text is rewritten to match.

I ran my own mutation battery on the PR branch. Full build Tests run: 1577, Failures: 0, BUILD SUCCESS. Making FleetMcp.ack ignore the boolean again is caught by two named tests. One gap is still open and is being fixed in the same PR: AmqpReplyInbox.ack returning true for something it never held survives both the default suite and -Pcontract against a real broker. That is the adapter this daemon actually runs, so an assertion is being added to AmqpReplyInboxContractTest — the one contract class CI runs today.

## Decision: option 2, refuse. Plus two corrections to my own text above. The fix is in PR #448. Before that, two things in the body are wrong or too strong, and both change how the next reader should think about this ticket. ### Correction 1 — "no code path to remove" is only true of `FleetMcp`, not of the channel The channel does have an ack. Measured on `main` today (`82fae94`): ``` $ grep -n 'void \|boolean \|String \|List<' fleetd/src/main/java/dev/ltms/fleet/msg/LeadChannel.java 30: void publish(String toCoordId, LeadMessage m); 33: List<LeadMessage> peek(); 40: void ack(String msgId); 43: String selfCoordId(); 55: boolean heldDurable(); ``` `LeadChannel.ack(String msgId)` is right there at line 40. What is true is the narrower claim: **`FleetMcp` never calls it.** Wiring `fleet_ack` to the coordination channel is therefore a small change, not a missing mechanism. That matters for how this ticket ages. Refusing is the right answer **today**, but it is a policy choice, not a structural impossibility. Those two age differently: a policy can be revisited when the pieces around it change, while an impossibility tells a future reader not to look. The fleet01 lead raised this, and I checked it in the code myself. ### Correction 2 — the reason to refuse is the ack race, not "held means unread" I wrote that removing a held message "when nobody has read its body yet" is the hard part. That premise is not always true. `LeadCoordLoop` writes the message to the lead's pane first, then acks it (`main`, today): ``` 153: agents.send(lead, DELIVERY_FORMAT.formatted(msg.from(), msg.content())); 160: rememberDelivered(msg.msgId()); 161: if (ackDelivered(msg)) { ``` And `ackDelivered` can fail **after** that pane write succeeds: ```java private boolean ackDelivered(LeadMessage msg) { try { channel.ack(msg.msgId()); } catch (RuntimeException e) { // Delivered but not acked: it will be redelivered, ... log.warn("lead coordination: delivered message {} but could not ack it: {}", msg.msgId(), e.toString()); return false; } return true; } ``` So there is a real window where a message the lead has already read is still in `held[]`. The loop heals this on its next tick, because line 127 re-acks anything `wasDelivered` already knows about, without showing it again. But inside that window, "everything held is unread" is false. **Write the reason this way instead: the delivery loop owns the ack, and a manual ack races it.** That is accurate in every case, including the window above. The fleet01 lead found this exception and I confirmed it in the source. ### The guard a future "make it work" would need `LeadCoordLoop.wasDelivered(String msgId)` (line 179) is exactly the predicate a safe manual ack would have to consult. A manual ack is safe for a message that predicate already knows about, and unsafe for one it does not — that is the whole difference between dropping read mail and destroying unread mail. Anyone reopening option 1 should start there, not from scratch. ### What #448 actually ships `ackReply` changes from `void` to `boolean`, in the interface and both implementations. `false` means nothing was removed and is not an error. `FleetMcp.ack` then returns an error naming `fleet_poll{coordId}` as the way to read held peer mail, and the `target` schema text is rewritten to match. I ran my own mutation battery on the PR branch. Full build `Tests run: 1577, Failures: 0`, `BUILD SUCCESS`. Making `FleetMcp.ack` ignore the boolean again is caught by two named tests. One gap is still open and is being fixed in the same PR: `AmqpReplyInbox.ack` returning `true` for something it never held survives both the default suite and `-Pcontract` against a real broker. That is the adapter this daemon actually runs, so an assertion is being added to `AmqpReplyInboxContractTest` — the one contract class CI runs today.
ltms commented 2026-09-10 12:21:07 +02:00
Author
Owner
Copy Link
Copy Source

Closed by PR #448.

What shipped

ReplyInbox.ack and MessageService.ackReply return boolean instead of void. false means nothing was removed and is not an error. FleetMcp.ack now returns an error on false, naming fleet_poll{coordId} as the way to read held lead-to-lead mail, and the target schema text matches the new behaviour. fleet_ack was not routed to LeadChannel.ack — see the correction comment above for why refusing is right today and why that is a policy choice, not an impossibility.

My verification, on the branch and then on the merge

BRANCH 5289eb5
  FULL BUILD  Tests run: 1577, Failures: 0  BUILD SUCCESS, 0 compile errors
  CONTROL     contract test, real broker: Tests run: 9, Failures: 0, Skipped: 0
  M3a  ack's `h == null` branch reports true       -> KILLED
  M3b  ack's `perTarget == null` branch reports true -> KILLED
       both by AmqpReplyInboxContractTest.ackReportsHitVsMissAgainstARealBroker

MERGE 0829542 (main bdcf285 merged in; 0 conflicts)
  FULL BUILD                  Tests run: 1578, Failures: 0  BUILD SUCCESS
  contract, real broker       Tests run: 9,    Failures: 0
  CompositePeerLauncherTest   Tests run: 76,   Failures: 0   (#447's guarantee intact)

M3a is the gap I reported in round 1: AmqpReplyInbox reporting true for something it never held survived the default suite and -Pcontract with a real broker. That was the defect this ticket is about, still unpinned in the adapter fleetd runs live. It is closed now, in the one contract class CI actually executes.

The part worth keeping

Both mutated branches die, but through different assertions, and that is not obvious from the code. held.get(target) is populated by the deliver callback, not by own():

var perTarget = held.get(target);
if (perTarget == null || perTarget == RELEASED) {
    return false;
}

So after own(target) with nothing ever delivered, the map entry is still absent. The "never held" assertion therefore exercises perTarget == null and never reaches h == null; only the double-ack assertion — publish, ack, ack again — gets there. The worker worked this out and said so plainly instead of claiming its new test covered both branches. I confirmed it by reading the method.

Two assertions that look like they test the same thing, and actually cover two different branches. Neither is redundant, and a reviewer trimming "the duplicate" would have reopened exactly the gap this ticket was filed for.

Also worth recording

The worker declined to run the contract test against the shared local LavinMQ broker, because this adapter never deletes queues and a run would have left orphaned durable queues on the instance backing the live fleet. It started a disposable container on a throwaway port instead and removed it afterwards. Nothing in its brief told it to think about that.

Closed by PR #448. ## What shipped `ReplyInbox.ack` and `MessageService.ackReply` return `boolean` instead of `void`. `false` means nothing was removed and is not an error. `FleetMcp.ack` now returns an error on `false`, naming `fleet_poll{coordId}` as the way to read held lead-to-lead mail, and the `target` schema text matches the new behaviour. `fleet_ack` was **not** routed to `LeadChannel.ack` — see the correction comment above for why refusing is right today and why that is a policy choice, not an impossibility. ## My verification, on the branch and then on the merge ``` BRANCH 5289eb5 FULL BUILD Tests run: 1577, Failures: 0 BUILD SUCCESS, 0 compile errors CONTROL contract test, real broker: Tests run: 9, Failures: 0, Skipped: 0 M3a ack's `h == null` branch reports true -> KILLED M3b ack's `perTarget == null` branch reports true -> KILLED both by AmqpReplyInboxContractTest.ackReportsHitVsMissAgainstARealBroker MERGE 0829542 (main bdcf285 merged in; 0 conflicts) FULL BUILD Tests run: 1578, Failures: 0 BUILD SUCCESS contract, real broker Tests run: 9, Failures: 0 CompositePeerLauncherTest Tests run: 76, Failures: 0 (#447's guarantee intact) ``` M3a is the gap I reported in round 1: `AmqpReplyInbox` reporting `true` for something it never held survived the default suite **and** `-Pcontract` with a real broker. That was the defect this ticket is about, still unpinned in the adapter fleetd runs live. It is closed now, in the one contract class CI actually executes. ## The part worth keeping Both mutated branches die, but through **different assertions**, and that is not obvious from the code. `held.get(target)` is populated by the deliver callback, not by `own()`: ```java var perTarget = held.get(target); if (perTarget == null || perTarget == RELEASED) { return false; } ``` So after `own(target)` with nothing ever delivered, the map entry is still absent. The "never held" assertion therefore exercises `perTarget == null` and never reaches `h == null`; only the double-ack assertion — publish, ack, ack again — gets there. The worker worked this out and said so plainly instead of claiming its new test covered both branches. I confirmed it by reading the method. Two assertions that look like they test the same thing, and actually cover two different branches. Neither is redundant, and a reviewer trimming "the duplicate" would have reopened exactly the gap this ticket was filed for. ## Also worth recording The worker declined to run the contract test against the shared local LavinMQ broker, because this adapter never deletes queues and a run would have left orphaned durable queues on the instance backing the live fleet. It started a disposable container on a throwaway port instead and removed it afterwards. Nothing in its brief told it to think about that.
ltms closed this issue 2026-09-10 12:21:13 +02:00
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#437
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?