Skip to content

Commit 5bc906a

Browse files
committed
Fix non-deterministic notification ordering in AcpClientSession
Incoming JSONRPCNotifications were processed via independent .subscribe() calls, allowing async handlers to complete out of arrival order. Route notifications through a Sinks.Many drained by concatMap so each handler Mono must complete before the next notification is picked up. Adds testNotificationOrderPreservedWithAsyncHandler to verify that five rapid notifications with staggered async delays still arrive in order.
1 parent 6b98ab7 commit 5bc906a

2 files changed

Lines changed: 62 additions & 4 deletions

File tree

acp-core/src/main/java/com/agentclientprotocol/sdk/spec/AcpClientSession.java

Lines changed: 27 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,10 @@
1111
import org.reactivestreams.Publisher;
1212
import org.slf4j.Logger;
1313
import org.slf4j.LoggerFactory;
14+
import reactor.core.Disposable;
1415
import reactor.core.publisher.Mono;
1516
import reactor.core.publisher.MonoSink;
17+
import reactor.core.publisher.Sinks;
1618

1719
import java.time.Duration;
1820
import java.util.Map;
@@ -78,6 +80,12 @@ public class AcpClientSession implements AcpSession {
7880
/** Atomic counter for generating unique request IDs */
7981
private final AtomicLong requestCounter = new AtomicLong(0);
8082

83+
/** Sink for serializing notification delivery in arrival order */
84+
private final Sinks.Many<AcpSchema.JSONRPCNotification> notificationSink;
85+
86+
/** Subscription draining the notification sink via concatMap */
87+
private final Disposable notificationSubscription;
88+
8189
/**
8290
* Functional interface for handling incoming JSON-RPC requests. Implementations
8391
* should process the request parameters and return a response.
@@ -148,6 +156,17 @@ public AcpClientSession(Duration requestTimeout, AcpClientTransport transport,
148156
return t;
149157
}), "acp-timeout-" + sessionPrefix);
150158

159+
// Serialize notification delivery: concatMap ensures each notification's Mono
160+
// completes before the next one starts, preserving arrival order even when
161+
// handlers do async work.
162+
this.notificationSink = Sinks.many().unicast().onBackpressureBuffer();
163+
this.notificationSubscription = this.notificationSink.asFlux()
164+
.concatMap(notification -> handleIncomingNotification(notification).onErrorComplete(t -> {
165+
logger.error("Error handling notification: {}", t.getMessage());
166+
return true;
167+
}))
168+
.subscribe();
169+
151170
this.transport.connect(mono -> mono.doOnNext(this::handle).then(Mono.empty())).transform(connectHook).subscribe();
152171
}
153172

@@ -203,10 +222,10 @@ else if (message instanceof AcpSchema.JSONRPCRequest request) {
203222
else if (message instanceof AcpSchema.JSONRPCNotification notification) {
204223
logger.debug("Received notification: {}", notification);
205224
logger.trace("Incoming notification method='{}' params={}", notification.method(), notification.params());
206-
handleIncomingNotification(notification).onErrorComplete(t -> {
207-
logger.error("Error handling notification: {}", t.getMessage());
208-
return true;
209-
}).subscribe();
225+
Sinks.EmitResult result = notificationSink.tryEmitNext(notification);
226+
if (result.isFailure()) {
227+
logger.warn("Failed to enqueue notification for serial processing: {}", result);
228+
}
210229
}
211230
else {
212231
logger.warn("Received unknown message type: {}", message);
@@ -352,6 +371,8 @@ public Mono<Void> sendNotification(String method, Object params) {
352371
public Mono<Void> closeGracefully() {
353372
return Mono.fromRunnable(() -> {
354373
dismissPendingResponses();
374+
notificationSink.tryEmitComplete();
375+
notificationSubscription.dispose();
355376
timeoutScheduler.dispose();
356377
});
357378
}
@@ -362,6 +383,8 @@ public Mono<Void> closeGracefully() {
362383
@Override
363384
public void close() {
364385
dismissPendingResponses();
386+
notificationSink.tryEmitComplete();
387+
notificationSubscription.dispose();
365388
timeoutScheduler.dispose();
366389
}
367390

acp-core/src/test/java/com/agentclientprotocol/sdk/spec/AcpClientSessionTest.java

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,11 @@
55
package com.agentclientprotocol.sdk.spec;
66

77
import java.time.Duration;
8+
import java.util.List;
89
import java.util.Map;
10+
import java.util.concurrent.CopyOnWriteArrayList;
11+
import java.util.concurrent.CountDownLatch;
12+
import java.util.concurrent.TimeUnit;
913
import java.util.function.Function;
1014

1115
import com.agentclientprotocol.sdk.MockAcpClientTransport;
@@ -298,6 +302,37 @@ void testGracefulShutdown() {
298302
StepVerifier.create(session.closeGracefully()).verifyComplete();
299303
}
300304

305+
@Test
306+
void testNotificationOrderPreservedWithAsyncHandler() throws InterruptedException {
307+
// Notification i gets a delay of (5 - i) * 30ms so that without serialization
308+
// later notifications would complete first, reversing the observed order.
309+
int count = 5;
310+
List<Integer> processedOrder = new CopyOnWriteArrayList<>();
311+
CountDownLatch latch = new CountDownLatch(count);
312+
313+
var transport = new MockAcpClientTransport();
314+
var session = new AcpClientSession(TIMEOUT, transport, Map.of(),
315+
Map.of(TEST_NOTIFICATION, params -> {
316+
int index = (int) ((Map<?, ?>) params).get("index");
317+
long delayMs = (count - index) * 30L;
318+
return Mono.delay(Duration.ofMillis(delayMs)).then(Mono.fromRunnable(() -> {
319+
processedOrder.add(index);
320+
latch.countDown();
321+
}));
322+
}),
323+
Function.identity());
324+
325+
for (int i = 0; i < count; i++) {
326+
transport.simulateIncomingMessage(new AcpSchema.JSONRPCNotification(
327+
AcpSchema.JSONRPC_VERSION, TEST_NOTIFICATION, Map.of("index", i)));
328+
}
329+
330+
assertThat(latch.await(5, TimeUnit.SECONDS)).isTrue();
331+
assertThat(processedOrder).containsExactly(0, 1, 2, 3, 4);
332+
333+
session.close();
334+
}
335+
301336
@Test
302337
void testConcurrentRequests() {
303338
var transport = new MockAcpClientTransport();

0 commit comments

Comments
 (0)