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

CB-529: drainReplies javadoc claims "the ack is local" — false for the AMQP adapter #12

New Issue
Closed
opened 2026-08-10 17:35:39 +02:00 by kevin · 1 comment
kevin commented 2026-08-10 17:35:39 +02:00
Owner
Copy Link
Copy Source

Found by the 2026-08-10 adversarial design review. Documentation-accuracy fix; small, but the lie sits exactly where the next implementer will read it.

Problem

MessageService.drainReplies acks the inbox before the MCP/REST response carrying the drained replies reaches the client, and its javadoc asserts "the ack is local." That was true for InMemoryReplyInbox; for AmqpReplyInbox the ack is a broker-side basicAck — after it, a crash in the response window means the reply is gone from the broker while the client never received it.

The window is small and the failure class is accepted (duplicate-tolerant pull path, per CB-308 §7.8's dual ack model) — but the code must say what is actually true, not what used to be.

Fix

Rewrite the javadoc to state the real contract: ack happens on drain, before client receipt; for the AMQP adapter this is an at-least-once boundary whose loss window is the response write; a client that needs certainty re-polls (idempotent — a drained-and-lost reply is the one case that becomes a genuine loss, and it is bounded by the response window). Optionally note the alternative (ack-on-next-poll) and why it was not taken (double-delivery on every normal drain).

No behaviour change in this ticket.

Acceptance

  • Javadoc matches actual behaviour for both adapters.
  • mvn clean install green (no code change expected beyond the comment).

🤖 Generated with Claude Code

https://claude.ai/code/session_013ZGgxLQ2VpwZhEYoru8rkf

Found by the 2026-08-10 adversarial design review. Documentation-accuracy fix; small, but the lie sits exactly where the next implementer will read it. ## Problem `MessageService.drainReplies` acks the inbox **before** the MCP/REST response carrying the drained replies reaches the client, and its javadoc asserts "the ack is local." That was true for `InMemoryReplyInbox`; for `AmqpReplyInbox` the ack is a broker-side `basicAck` — after it, a crash in the response window means the reply is gone from the broker while the client never received it. The window is small and the failure class is accepted (duplicate-tolerant pull path, per CB-308 §7.8's dual ack model) — but the code must say what is actually true, not what used to be. ## Fix Rewrite the javadoc to state the real contract: ack happens on drain, *before* client receipt; for the AMQP adapter this is an at-least-once boundary whose loss window is the response write; a client that needs certainty re-polls (idempotent — a drained-and-lost reply is the one case that becomes a genuine loss, and it is bounded by the response window). Optionally note the alternative (ack-on-next-poll) and why it was not taken (double-delivery on every normal drain). No behaviour change in this ticket. ## Acceptance - Javadoc matches actual behaviour for **both** adapters. - `mvn clean install` green (no code change expected beyond the comment). 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_013ZGgxLQ2VpwZhEYoru8rkf
ltms closed this issue 2026-08-15 15:35:02 +02:00
ltms commented 2026-08-15 15:35:10 +02:00
Owner
Copy Link
Copy Source

Fixed on main at 94476ac. Build: 764 tests, 0 failures, BUILD SUCCESS, exit 0 (mvn -f bridged/pom.xml clean install, unpiped). No behaviour change, as the ticket specified.

The rewritten javadoc splits the contract by adapter, because that is where the two disagree:

  • InMemoryReplyInbox — the ack drops an entry from a local map, and the messages are already in the returned list, so nothing can be lost after that point.
  • AmqpReplyInbox — the ack is a broker-side basicAck. Once it lands the broker has forgotten the message, so a crash during the response write loses the reply outright.

One thing worth stating more sharply than the ticket did: in the AMQP case this is not a redelivery, it is a genuine loss. Re-polling cannot recover it because there is nothing left to re-deliver. The doc now says that plainly rather than leaving "at-least-once" to imply otherwise.

I kept the accepted-tradeoff note (ack-on-next-poll would double-deliver on every normal drain, which costs more than the window it closes) and added a line recording how the comment became false — it was true when the in-memory inbox was the only one, and nothing flagged it when the AMQP adapter landed. That is the reason the next person should re-check both adapters before editing this method.

Fixed on `main` at `94476ac`. Build: 764 tests, 0 failures, BUILD SUCCESS, exit 0 (`mvn -f bridged/pom.xml clean install`, unpiped). No behaviour change, as the ticket specified. The rewritten javadoc splits the contract by adapter, because that is where the two disagree: - `InMemoryReplyInbox` — the ack drops an entry from a local map, and the messages are already in the returned list, so nothing can be lost after that point. - `AmqpReplyInbox` — the ack is a broker-side `basicAck`. Once it lands the broker has forgotten the message, so a crash during the response write loses the reply outright. One thing worth stating more sharply than the ticket did: in the AMQP case this is **not** a redelivery, it is a genuine loss. Re-polling cannot recover it because there is nothing left to re-deliver. The doc now says that plainly rather than leaving "at-least-once" to imply otherwise. I kept the accepted-tradeoff note (ack-on-next-poll would double-deliver on every normal drain, which costs more than the window it closes) and added a line recording *how* the comment became false — it was true when the in-memory inbox was the only one, and nothing flagged it when the AMQP adapter landed. That is the reason the next person should re-check both adapters before editing this method.
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
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#12
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?