From 94476ac109fdd7a0507d6fb21b46869b566af770 Mon Sep 17 00:00:00 2001
From: Dai Ha <dai.ha@magnolia-cms.com>
Date: Sat, 15 Aug 2026 15:34:59 +0200
Subject: [PATCH] =?UTF-8?q?CB-529:=20drainReplies=20javadoc=20said=20the?=
 =?UTF-8?q?=20ack=20is=20local=20=E2=80=94=20false=20for=20AMQP?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.
---
 .../dev/ltms/bridged/msg/MessageService.java  | 27 ++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/bridged/src/main/java/dev/ltms/bridged/msg/MessageService.java b/bridged/src/main/java/dev/ltms/bridged/msg/MessageService.java
index 5db11cc..c35076d 100644
--- a/bridged/src/main/java/dev/ltms/bridged/msg/MessageService.java
+++ b/bridged/src/main/java/dev/ltms/bridged/msg/MessageService.java
@@ -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
      */