CB-529: drainReplies javadoc said the ack is local — false for AMQP
The doc claimed an in-flight failure re-surfaces the messages on a later drain, because the ack is local. That was true when InMemoryReplyInbox was the only inbox, and became false without anyone noticing when the AMQP adapter landed: there the ack is a broker-side basicAck, so a crash while writing the response loses the reply outright. Re-polling cannot recover it, since the broker has already forgotten it. Behaviour is unchanged and the window stays accepted — acking on the next poll instead would double-deliver on every normal drain. The point is that the comment sat exactly where the next implementer would read it and said the opposite of what happens. Closes #12.
This commit is contained in:
@@ -333,9 +333,30 @@ public final class MessageService {
|
||||
}
|
||||
|
||||
/**
|
||||
* Drain (peek + ack) all pending inbox replies for {@code target}. At-least-once: returns the
|
||||
* messages and acknowledges them; an in-flight failure between returning and the caller
|
||||
* processing them re-surfaces them on a subsequent drain (the ack is local).
|
||||
* Drain (peek + ack) all pending inbox replies for {@code target}.
|
||||
*
|
||||
* <p><strong>The ack happens here, before the caller has the messages</strong> — before the MCP
|
||||
* or REST response carrying them has been written, and long before the client has processed
|
||||
* them. That ordering is what the two adapters disagree about, so do not read this method as
|
||||
* "at-least-once" without qualifying which inbox is behind it (CB-529):
|
||||
*
|
||||
* <ul>
|
||||
* <li>{@code InMemoryReplyInbox} — the ack only drops an entry from a local map. The messages
|
||||
* are already in the returned list, so nothing can be lost after this point.
|
||||
* <li>{@code AmqpReplyInbox} — the ack is a broker-side {@code basicAck}. Once it lands the
|
||||
* broker has forgotten the message. If the daemon dies while writing the response, the
|
||||
* reply is gone from the broker <em>and</em> the client never received it. Re-polling
|
||||
* cannot recover it, because there is nothing left to re-deliver.
|
||||
* </ul>
|
||||
*
|
||||
* <p>So the loss window is the response write, and it is a genuine loss rather than a
|
||||
* redelivery. This is accepted, not overlooked: the alternative — ack on the next poll — turns
|
||||
* every normal drain into a double delivery, which costs more than the window it closes. A
|
||||
* caller that needs certainty re-polls; that is idempotent for every case except this one.
|
||||
*
|
||||
* <p>Any change here must be checked against <em>both</em> adapters. The previous version of
|
||||
* this javadoc claimed "the ack is local", which was true when only the in-memory inbox existed
|
||||
* and silently became false when the AMQP adapter landed.
|
||||
*
|
||||
* @return the drained messages, newest last (FIFO); empty list if none
|
||||
*/
|
||||
|
||||
Reference in New Issue
Block a user