CB-520: split ReplyInbox into explicit own/release and publish halves
This commit is contained in:
@@ -15,6 +15,8 @@ import dev.ltms.bridged.session.WorkerSession;
|
||||
import dev.ltms.bridged.session.WorktreeRequest;
|
||||
import dev.ltms.bridged.worker.ClaudeCodeLauncher;
|
||||
import io.modelcontextprotocol.spec.McpSchema;
|
||||
import dev.ltms.bridged.msg.InMemoryReplyInbox;
|
||||
import org.junit.jupiter.api.BeforeEach;
|
||||
import org.junit.jupiter.api.Test;
|
||||
|
||||
import java.util.Map;
|
||||
@@ -31,10 +33,19 @@ import static org.junit.jupiter.api.Assertions.*;
|
||||
*/
|
||||
class BridgeMcpTest {
|
||||
|
||||
private static final String T = "term_a";
|
||||
|
||||
private final FakeHerdr herdr = new FakeHerdr();
|
||||
private final AgentControl agents = new AgentControl(herdr);
|
||||
private final Rendezvous rendezvous = new Rendezvous();
|
||||
private final MessageService messages = new MessageService(agents, new Injector(agents), rendezvous);
|
||||
private final InMemoryReplyInbox inbox = new InMemoryReplyInbox();
|
||||
private final MessageService messages = new MessageService(agents, new Injector(agents), rendezvous, inbox);
|
||||
|
||||
@BeforeEach
|
||||
void setUp() {
|
||||
// CB-520: the inbox only peeks/acks targets it owns.
|
||||
inbox.own(T);
|
||||
}
|
||||
|
||||
private static String textOf(McpSchema.CallToolResult r) {
|
||||
return ((McpSchema.TextContent) r.content().getFirst()).text();
|
||||
|
||||
@@ -1,5 +1,9 @@
|
||||
package dev.ltms.bridged.msg;
|
||||
|
||||
import com.rabbitmq.client.AMQP;
|
||||
import com.rabbitmq.client.Channel;
|
||||
import com.rabbitmq.client.Connection;
|
||||
import com.rabbitmq.client.ConnectionFactory;
|
||||
import org.junit.jupiter.api.BeforeAll;
|
||||
import org.junit.jupiter.api.Tag;
|
||||
import org.junit.jupiter.api.Test;
|
||||
@@ -30,8 +34,8 @@ import static org.junit.jupiter.api.Assertions.assertTrue;
|
||||
* </ul>
|
||||
*
|
||||
* <p>It proves the port contract on genuine infrastructure: eventual visibility of a published reply,
|
||||
* ack removal, msgId dedup, and — the reason Stage 2 exists — cross-restart durability: an unacked
|
||||
* reply survives closing the inbox and is redelivered to a fresh connection.
|
||||
* ack removal, msgId dedup, explicit ownership, and — the reason Stage 2 exists — cross-restart
|
||||
* durability: an unacked reply survives closing the inbox and is redelivered to a fresh connection.
|
||||
*/
|
||||
@Tag("contract")
|
||||
// disabledWithoutDocker=false on purpose: on the CI path (AMQP_URI set) no container is started and
|
||||
@@ -69,6 +73,7 @@ class AmqpReplyInboxContractTest {
|
||||
void publishThenPeekThenAck() throws Exception {
|
||||
String target = "worker-pub-" + System.nanoTime();
|
||||
try (AmqpReplyInbox inbox = AmqpReplyInbox.open(uri())) {
|
||||
inbox.own(target);
|
||||
inbox.publish(target, "m1", "hello primary");
|
||||
|
||||
List<ReplyInbox.InboxMessage> got = awaitPeek(inbox, target);
|
||||
@@ -86,6 +91,7 @@ class AmqpReplyInboxContractTest {
|
||||
void duplicateMsgIdIsNotDoubleQueued() throws Exception {
|
||||
String target = "worker-dedup-" + System.nanoTime();
|
||||
try (AmqpReplyInbox inbox = AmqpReplyInbox.open(uri())) {
|
||||
inbox.own(target);
|
||||
inbox.publish(target, "dup", "first");
|
||||
awaitPeek(inbox, target);
|
||||
inbox.publish(target, "dup", "second"); // same msgId — must be a no-op
|
||||
@@ -102,8 +108,9 @@ class AmqpReplyInboxContractTest {
|
||||
void unackedReplySurvivesRestartAndIsRedelivered() throws Exception {
|
||||
String target = "worker-durable-" + System.nanoTime();
|
||||
|
||||
// First "process life": publish, see it held, but crash before acking.
|
||||
// First "process life": own, publish, see it held, but crash before acking.
|
||||
try (AmqpReplyInbox first = AmqpReplyInbox.open(uri())) {
|
||||
first.own(target);
|
||||
first.publish(target, "persist-1", "survive me");
|
||||
assertEquals(1, awaitPeek(first, target).size());
|
||||
// no ack — simulate a java -jar bounce with the reply still pending
|
||||
@@ -111,6 +118,7 @@ class AmqpReplyInboxContractTest {
|
||||
|
||||
// Second "process life": a fresh connection to the same broker must be redelivered the reply.
|
||||
try (AmqpReplyInbox second = AmqpReplyInbox.open(uri())) {
|
||||
second.own(target);
|
||||
List<ReplyInbox.InboxMessage> got = awaitPeek(second, target);
|
||||
assertEquals(1, got.size(), "an unacked persistent reply is redelivered after restart");
|
||||
assertEquals("persist-1", got.getFirst().msgId());
|
||||
@@ -121,11 +129,43 @@ class AmqpReplyInboxContractTest {
|
||||
|
||||
// Third life: once acked, it is gone for good — durability is not endless replay.
|
||||
try (AmqpReplyInbox third = AmqpReplyInbox.open(uri())) {
|
||||
third.own(target);
|
||||
Thread.sleep(500);
|
||||
assertTrue(third.peek(target).isEmpty(), "an acked reply does not come back on the next restart");
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
void publishDoesNotAttachAConsumer() throws Exception {
|
||||
String target = "worker-pub-no-consumer-" + System.nanoTime();
|
||||
try (AmqpReplyInbox inbox = AmqpReplyInbox.open(uri());
|
||||
Connection inspect = newConnection()) {
|
||||
inbox.own(target);
|
||||
inbox.publish(target, "m1", "published");
|
||||
awaitPeek(inbox, target); // ensure the owner's consumer received it
|
||||
|
||||
try (Channel ch = inspect.createChannel()) {
|
||||
AMQP.Queue.DeclareOk ok = ch.queueDeclare(queueName(target), true, false, false, null);
|
||||
assertEquals(1, ok.getConsumerCount(),
|
||||
"publish must not attach a consumer; only the owner's consumer should exist");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
void releaseCancelsConsumerAndClearsHeld() throws Exception {
|
||||
String target = "worker-release-" + System.nanoTime();
|
||||
try (AmqpReplyInbox inbox = AmqpReplyInbox.open(uri())) {
|
||||
inbox.own(target);
|
||||
inbox.publish(target, "m1", "release me");
|
||||
assertEquals(1, awaitPeek(inbox, target).size(), "owned target holds the reply");
|
||||
|
||||
inbox.release(target);
|
||||
assertTrue(inbox.peek(target).isEmpty(),
|
||||
"release clears the local held snapshot");
|
||||
}
|
||||
}
|
||||
|
||||
/** Poll peek (broker delivery is async) until a reply for {@code target} appears or ~10s elapse. */
|
||||
@SuppressWarnings("BusyWait") // deliberate poll for async broker delivery, bounded by the deadline
|
||||
private static List<ReplyInbox.InboxMessage> awaitPeek(AmqpReplyInbox inbox, String target)
|
||||
@@ -138,4 +178,15 @@ class AmqpReplyInboxContractTest {
|
||||
}
|
||||
return msgs;
|
||||
}
|
||||
|
||||
/** A separate broker connection for inspecting queue state without disturbing the inbox. */
|
||||
private static Connection newConnection() throws Exception {
|
||||
ConnectionFactory factory = new ConnectionFactory();
|
||||
factory.setUri(uri());
|
||||
return factory.newConnection("contract-inspector");
|
||||
}
|
||||
|
||||
private static String queueName(String target) {
|
||||
return "agent." + target + ".inbox";
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
package dev.ltms.bridged.msg;
|
||||
|
||||
import org.junit.jupiter.api.BeforeEach;
|
||||
import org.junit.jupiter.api.Test;
|
||||
|
||||
import java.util.concurrent.CountDownLatch;
|
||||
@@ -10,13 +11,18 @@ import java.util.concurrent.atomic.AtomicReference;
|
||||
import static org.junit.jupiter.api.Assertions.*;
|
||||
|
||||
/**
|
||||
* Unit tests for {@link InMemoryReplyInbox}: publish, peek, ack, dedup, FIFO ordering, and thread
|
||||
* safety under concurrent publish vs. drain.
|
||||
* Unit tests for {@link InMemoryReplyInbox}: publish, peek, ack, dedup, FIFO ordering, thread
|
||||
* safety under concurrent publish vs. drain, and explicit ownership.
|
||||
*/
|
||||
class InMemoryReplyInboxTest {
|
||||
|
||||
private final ReplyInbox inbox = new InMemoryReplyInbox();
|
||||
|
||||
@BeforeEach
|
||||
void setUp() {
|
||||
inbox.own("term_a");
|
||||
}
|
||||
|
||||
@Test
|
||||
void publishThenPeekReturnsTheMessage() {
|
||||
inbox.publish("term_a", "m1", "hello");
|
||||
@@ -72,6 +78,7 @@ class InMemoryReplyInboxTest {
|
||||
|
||||
@Test
|
||||
void perTargetIsolation() {
|
||||
inbox.own("term_b");
|
||||
inbox.publish("term_a", "m1", "for-a");
|
||||
inbox.publish("term_b", "m2", "for-b");
|
||||
assertEquals(1, inbox.peek("term_a").size());
|
||||
@@ -142,4 +149,40 @@ class InMemoryReplyInboxTest {
|
||||
exec.shutdown();
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
void publishWithoutOwnDoesNotClaimOwnership() {
|
||||
String unowned = "term_unowned";
|
||||
inbox.publish(unowned, "m1", "hello");
|
||||
// Without an owner, peek returns nothing — publish did not imply consume.
|
||||
assertTrue(inbox.peek(unowned).isEmpty(),
|
||||
"publishing to an unowned target must not make it peekable");
|
||||
// Owning afterwards makes the already-published message visible.
|
||||
inbox.own(unowned);
|
||||
var msgs = inbox.peek(unowned);
|
||||
assertEquals(1, msgs.size());
|
||||
assertEquals("m1", msgs.getFirst().msgId());
|
||||
}
|
||||
|
||||
@Test
|
||||
void peekAndAckAreNoOpsForUnownedTarget() {
|
||||
assertTrue(inbox.peek("term_not_owned").isEmpty());
|
||||
inbox.ack("term_not_owned", "m1"); // no-op, should not throw
|
||||
}
|
||||
|
||||
@Test
|
||||
void releaseStopsConsumingAndClearsHeld() {
|
||||
inbox.publish("term_a", "m1", "hello");
|
||||
assertEquals(1, inbox.peek("term_a").size());
|
||||
inbox.release("term_a");
|
||||
assertTrue(inbox.peek("term_a").isEmpty(),
|
||||
"after release, the local snapshot is cleared");
|
||||
}
|
||||
|
||||
@Test
|
||||
void ownIsIdempotent() {
|
||||
inbox.own("term_a");
|
||||
inbox.publish("term_a", "m1", "hello");
|
||||
assertEquals(1, inbox.peek("term_a").size());
|
||||
}
|
||||
}
|
||||
|
||||
@@ -6,6 +6,7 @@ import dev.ltms.bridged.herdr.FakeHerdr;
|
||||
import dev.ltms.bridged.herdr.HerdrException;
|
||||
import dev.ltms.bridged.inject.CompletionResolver;
|
||||
import dev.ltms.bridged.inject.Injector;
|
||||
import org.junit.jupiter.api.BeforeEach;
|
||||
import org.junit.jupiter.api.Test;
|
||||
|
||||
import java.util.concurrent.CompletableFuture;
|
||||
@@ -30,7 +31,14 @@ class MessageServiceTest {
|
||||
private final Rendezvous rendezvous = new Rendezvous();
|
||||
private final CompletionResolver completion = new CompletionResolver(agents, rendezvous);
|
||||
private final Injector injector = new Injector(agents, completion);
|
||||
private final MessageService messages = new MessageService(agents, injector, rendezvous);
|
||||
private final InMemoryReplyInbox inbox = new InMemoryReplyInbox();
|
||||
private final MessageService messages = new MessageService(agents, injector, rendezvous, inbox);
|
||||
|
||||
@BeforeEach
|
||||
void setUp() {
|
||||
// CB-520: the inbox only peeks/acks targets it owns.
|
||||
inbox.own(T);
|
||||
}
|
||||
|
||||
/** Run {@code send} on a background thread; the current thread drives the worker's turn. */
|
||||
private CompletableFuture<MessageService.Reply> sendAsync() {
|
||||
|
||||
@@ -45,6 +45,7 @@ class ReplyPushLoopTest {
|
||||
void setUp() {
|
||||
registry = new PrimaryRegistry(PRIMARY);
|
||||
inbox = new InMemoryReplyInbox();
|
||||
inbox.own(WORKER); // CB-520: the inbox only peeks/acks targets it owns
|
||||
scheduler = Executors.newSingleThreadScheduledExecutor();
|
||||
}
|
||||
|
||||
|
||||
@@ -10,6 +10,7 @@ import dev.ltms.bridged.herdr.WorkspaceControl;
|
||||
import dev.ltms.bridged.inject.Injector;
|
||||
import dev.ltms.bridged.inject.StatusPoller;
|
||||
import dev.ltms.bridged.inject.WorkerPresence;
|
||||
import dev.ltms.bridged.msg.InMemoryReplyInbox;
|
||||
import dev.ltms.bridged.msg.MessageService;
|
||||
import dev.ltms.bridged.msg.Rendezvous;
|
||||
import dev.ltms.bridged.session.FakeWorktrees;
|
||||
@@ -75,7 +76,12 @@ class BridgedAppTest {
|
||||
poller = new StatusPoller(agents, injector, 5); // delivers when the fake reports idle
|
||||
poller.start();
|
||||
Rendezvous rendezvous = new Rendezvous();
|
||||
MessageService messages = new MessageService(agents, injector, rendezvous);
|
||||
InMemoryReplyInbox inbox = new InMemoryReplyInbox();
|
||||
sessions.onAcquire(inbox::own);
|
||||
// The static REST tests address "term_a" without acquiring it through SessionManager, so own
|
||||
// it directly so the inbox contract holds for those endpoints.
|
||||
inbox.own("term_a");
|
||||
MessageService messages = new MessageService(agents, injector, rendezvous, inbox);
|
||||
app = new BridgedApp(herdr, workers, sessions, messages, this.presence, null)
|
||||
.build().start("127.0.0.1", 0);
|
||||
return app.port();
|
||||
|
||||
Reference in New Issue
Block a user