Skip to content

Commit e4da7c5

Browse files
author
Mark Pollack
committed
Release the prompt lock before the response is published, serialise response emission
Fixes #14. AcpAgentSession released its single-turn prompt lock in doFinally, after the response had already reached the client. Under CPU contention (taskset -c 0, busy CI runners) the client's next prompt arrived before that release and was rejected with -32000; the rejection's emission then collided with the still-running response emission on the same sink (FAIL_NON_SERIALIZED). The shipped transports ignored the failed emit and silently dropped the response; InMemoryTransportPair turned it into an error that terminated the agent's inbound subscription. Either way the client waited out the full requestTimeout. The prompt response ends the turn, so the lock is now released in doOnNext and doOnError, before the signal propagates to the transport, with doFinally kept for cancellation. Handlers are invoked inside Mono.defer so one that throws before returning its Mono cannot hold the lock forever. Response emission in StdioAcpAgentTransport, WebSocketAcpClientTransport, WebSocketAcpAgentTransport and InMemoryTransportPair goes through the same busy-looping emitter as sendMessage, and a failed emission in the in-memory pair is reported to the exception handler instead of ending the stream. extractSessionId reads typed PromptRequest and CancelNotification params as well as maps, so in-process transports key the lock correctly. Tests: the reporter's repro (MultiPromptSyncSessionTest, plain and streaming, repeated) and deterministic ordering tests that send the next prompt inside onNext of the previous response. CI gains a job that runs both pinned to one CPU. Reported by @krickert.
1 parent 1451764 commit e4da7c5

10 files changed

Lines changed: 423 additions & 44 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,3 +38,23 @@ jobs:
3838
name: test-results
3939
path: '**/target/surefire-reports/'
4040
retention-days: 7
41+
42+
# Issue #14 only reproduced when the JVM was starved of cores: the agent's lock release
43+
# and the client's next prompt race on one CPU. Pin the contention tests to one core so
44+
# the window stays open in CI.
45+
contention:
46+
runs-on: ubuntu-latest
47+
timeout-minutes: 20
48+
49+
steps:
50+
- uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4
51+
52+
- name: Set up JDK 17
53+
uses: actions/setup-java@cf277c60eb25467037889841efdb72551f06f6c3 # v4
54+
with:
55+
java-version: '17'
56+
distribution: 'temurin'
57+
cache: maven
58+
59+
- name: Contention tests pinned to one CPU
60+
run: taskset -c 0 ./mvnw -B -pl acp-core -am test -Dtest='MultiPromptSyncSessionTest,AcpAgentSessionPromptLockTest' -Dsurefire.failIfNoSpecifiedTests=false

‎CHANGELOG.md‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,23 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
2222
connect or start failure instead of dropping it: construction throws `IllegalStateException` when
2323
the transport refuses synchronously, and every later request fails immediately with the cause.
2424

25+
- **Second prompt on a session could be rejected or hang under CPU contention (#14).** The agent
26+
released its single-turn prompt lock in `doFinally`, *after* the response had already reached the
27+
client. On a starved machine (`taskset -c 0`, busy CI runners) the client's next prompt arrived
28+
before that release and was rejected with `-32000 There is already an active prompt execution`;
29+
the rejection's emission then collided with the still-running response emission on the same sink
30+
(`FAIL_NON_SERIALIZED`), which the shipped transports silently dropped and the in-memory test
31+
transport turned into a fatal error for the agent, so `AcpSyncClient.prompt` waited out the full
32+
`requestTimeout`. The lock is now released before the response is published, on the success and
33+
error paths alike, and a handler that throws before returning its `Mono` no longer holds the lock
34+
forever. Response emission in `StdioAcpAgentTransport`, `WebSocketAcpClientTransport`,
35+
`WebSocketAcpAgentTransport` and `InMemoryTransportPair` is serialised the way `sendMessage`
36+
already was, and a failed emission in the in-memory pair no longer terminates the agent. The
37+
reporter's repro is now a test, and CI runs the contention tests pinned to one CPU. Reported by
38+
@krickert.
39+
- The agent session now reads `sessionId` from typed `PromptRequest`/`CancelNotification` params as
40+
well as from maps, so in-process transports get the right lock owner in logs and cancel matching.
41+
2542
### Added
2643

2744
- `AcpSyncClient(AcpAsyncClient)` is public: the supported way to have both APIs over one session.

‎acp-core/src/main/java/com/agentclientprotocol/sdk/agent/transport/StdioAcpAgentTransport.java‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,15 @@ private void handleIncomingMessages(Function<Mono<JSONRPCMessage>, Mono<JSONRPCM
158158
.flatMap(message -> Mono.just(message).transform(handler))
159159
.doOnNext(response -> {
160160
if (response != null) {
161-
this.outboundSink.tryEmitNext(response);
161+
// Responses are emitted from the inbound thread while sendMessage emits
162+
// from user threads on the same sink; both must go through the serialising
163+
// busy-loop or a collision drops the response (FAIL_NON_SERIALIZED, #14).
164+
try {
165+
this.outboundSink.emitNext(response, Sinks.EmitFailureHandler.busyLooping(Duration.ofMillis(100)));
166+
}
167+
catch (Sinks.EmissionException e) {
168+
logger.error("Dropped response {}: {}", response, e.getReason());
169+
}
162170
}
163171
})
164172
.doOnTerminate(() -> {

‎acp-core/src/main/java/com/agentclientprotocol/sdk/client/transport/WebSocketAcpClientTransport.java‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,15 @@ private void handleIncomingMessages(Function<Mono<JSONRPCMessage>, Mono<JSONRPCM
167167
.flatMap(message -> Mono.just(message).transform(handler))
168168
.doOnNext(response -> {
169169
if (response != null) {
170-
this.outboundSink.tryEmitNext(response);
170+
// Responses are emitted from the inbound thread while sendMessage emits
171+
// from user threads on the same sink; both must go through the serialising
172+
// busy-loop or a collision drops the response (FAIL_NON_SERIALIZED, #14).
173+
try {
174+
this.outboundSink.emitNext(response, Sinks.EmitFailureHandler.busyLooping(Duration.ofMillis(100)));
175+
}
176+
catch (Sinks.EmissionException e) {
177+
logger.error("Dropped response {}: {}", response, e.getReason());
178+
}
171179
}
172180
})
173181
.doOnTerminate(() -> {

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

Lines changed: 34 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -275,25 +275,50 @@ private Mono<AcpSchema.JSONRPCResponse> handleIncomingRequest(AcpSchema.JSONRPCR
275275
new AcpSchema.JSONRPCError(-32000, "There is already an active prompt execution", null)));
276276
}
277277

278-
// Execute handler and clear active prompt when done
279-
return handler.handle(request.params())
278+
// The prompt response ends the turn (ACP semantics), so the lock is released
279+
// *before* the response is handed downstream to the transport. Releasing in
280+
// doFinally instead ran after the response had already reached the client;
281+
// under CPU contention the client's next prompt then arrived before the
282+
// release and was rejected with -32000 (#14). doOnNext and doOnError run before
283+
// the signal propagates, and a Mono emits at most once, so the release is
284+
// ordered before publication on every path; doFinally covers cancellation.
285+
// Mono.defer keeps a handler that throws synchronously from holding the lock
286+
// forever: the throw becomes an error signal that passes through the release.
287+
return Mono.defer(() -> handler.handle(request.params()))
280288
.map(result -> new AcpSchema.JSONRPCResponse(AcpSchema.JSONRPC_VERSION, request.id(), result, null))
281-
.doFinally(signal -> {
282-
activePrompt.compareAndSet(newPrompt, null);
283-
logger.debug("Prompt completed with signal: {}", signal);
284-
});
289+
.doOnNext(response -> releasePrompt(newPrompt, "response"))
290+
.doOnError(error -> releasePrompt(newPrompt, "error"))
291+
.doFinally(signal -> releasePrompt(newPrompt, signal.toString()));
285292
}
286293

287-
return handler.handle(request.params())
294+
return Mono.defer(() -> handler.handle(request.params()))
288295
.map(result -> new AcpSchema.JSONRPCResponse(AcpSchema.JSONRPC_VERSION, request.id(), result, null));
289296
});
290297
}
291298

292299
/**
293-
* Extracts the sessionId from request parameters.
300+
* Releases the single-turn lock if {@code prompt} still holds it. Idempotent: the
301+
* release is attempted on the response, on an error and on the terminal signal, and
302+
* only the first attempt does anything.
303+
*/
304+
private void releasePrompt(ActivePrompt prompt, String reason) {
305+
if (activePrompt.compareAndSet(prompt, null)) {
306+
logger.debug("Prompt lock released for sessionId={} requestId={} ({})", prompt.sessionId(),
307+
prompt.requestId(), reason);
308+
}
309+
}
310+
311+
/**
312+
* Extracts the sessionId from request parameters, which arrive as a {@code Map} from
313+
* the JSON transports and as the typed record from in-process transports.
294314
*/
295-
@SuppressWarnings("unchecked")
296315
private String extractSessionId(Object params) {
316+
if (params instanceof AcpSchema.PromptRequest promptRequest) {
317+
return promptRequest.sessionId() != null ? promptRequest.sessionId() : "unknown";
318+
}
319+
if (params instanceof AcpSchema.CancelNotification cancelNotification) {
320+
return cancelNotification.sessionId() != null ? cancelNotification.sessionId() : "unknown";
321+
}
297322
if (params instanceof Map<?, ?> map) {
298323
Object sessionId = map.get("sessionId");
299324
return sessionId != null ? sessionId.toString() : "unknown";
Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
package com.agentclientprotocol.sdk.integration;
2+
3+
import java.time.Duration;
4+
import java.util.List;
5+
import java.util.concurrent.CopyOnWriteArrayList;
6+
7+
import com.agentclientprotocol.sdk.agent.AcpAgent;
8+
import com.agentclientprotocol.sdk.agent.AcpSyncAgent;
9+
import com.agentclientprotocol.sdk.client.AcpClient;
10+
import com.agentclientprotocol.sdk.client.AcpSyncClient;
11+
import com.agentclientprotocol.sdk.spec.AcpSchema;
12+
import com.agentclientprotocol.sdk.test.InMemoryTransportPair;
13+
import org.junit.jupiter.api.RepeatedTest;
14+
15+
import static org.assertj.core.api.Assertions.assertThat;
16+
17+
/**
18+
* Reproduction for issue #14: AcpSyncAgent + AcpSyncClient over InMemoryTransportPair, one
19+
* session, three sequential prompts, run under {@code taskset -c 0}. The report says the
20+
* second or third prompt never returns and the request timeout fires.
21+
*/
22+
class MultiPromptSyncSessionTest {
23+
24+
private static final Duration TIMEOUT = Duration.ofSeconds(5);
25+
26+
@RepeatedTest(5)
27+
void threeSequentialPromptsOnOneSessionAllReturn() {
28+
InMemoryTransportPair pair = InMemoryTransportPair.create();
29+
AcpSyncAgent agent = AcpAgent.sync(pair.agentTransport())
30+
.requestTimeout(TIMEOUT)
31+
.initializeHandler(request -> new AcpSchema.InitializeResponse(1, new AcpSchema.AgentCapabilities(), List.of()))
32+
.newSessionHandler(request -> new AcpSchema.NewSessionResponse("session-14", null, null))
33+
.promptHandler((request, context) -> new AcpSchema.PromptResponse(AcpSchema.StopReason.END_TURN))
34+
.build();
35+
agent.start();
36+
37+
AcpSyncClient client = AcpClient.sync(pair.clientTransport()).requestTimeout(TIMEOUT).build();
38+
try {
39+
client.initialize();
40+
String sessionId = client.newSession(new AcpSchema.NewSessionRequest("/workspace", List.of())).sessionId();
41+
for (int i = 1; i <= 3; i++) {
42+
long t0 = System.nanoTime();
43+
AcpSchema.PromptResponse response = client.prompt(new AcpSchema.PromptRequest(sessionId,
44+
List.of(new AcpSchema.TextContent("prompt " + i))));
45+
System.out.printf("prompt %d returned %s in %d ms%n", i, response.stopReason(),
46+
(System.nanoTime() - t0) / 1_000_000);
47+
assertThat(response.stopReason()).isEqualTo(AcpSchema.StopReason.END_TURN);
48+
}
49+
}
50+
finally {
51+
client.closeGracefully();
52+
agent.closeGracefully();
53+
pair.closeGracefully().block(TIMEOUT);
54+
}
55+
}
56+
57+
@RepeatedTest(5)
58+
void threeSequentialStreamingPromptsOnOneSessionAllReturn() {
59+
InMemoryTransportPair pair = InMemoryTransportPair.create();
60+
AcpSyncAgent agent = AcpAgent.sync(pair.agentTransport())
61+
.requestTimeout(TIMEOUT)
62+
.initializeHandler(request -> new AcpSchema.InitializeResponse(1, new AcpSchema.AgentCapabilities(), List.of()))
63+
.newSessionHandler(request -> new AcpSchema.NewSessionResponse("session-14", null, null))
64+
.promptHandler((request, context) -> {
65+
for (int chunk = 0; chunk < 3; chunk++) {
66+
context.sendUpdate(request.sessionId(),
67+
new AcpSchema.AgentMessageChunk("agent_message_chunk", new AcpSchema.TextContent("chunk " + chunk)));
68+
}
69+
return new AcpSchema.PromptResponse(AcpSchema.StopReason.END_TURN);
70+
})
71+
.build();
72+
agent.start();
73+
74+
List<String> updates = new CopyOnWriteArrayList<>();
75+
AcpSyncClient client = AcpClient.sync(pair.clientTransport())
76+
.requestTimeout(TIMEOUT)
77+
.sessionUpdateConsumer(notification -> updates.add(notification.update().getClass().getSimpleName()))
78+
.build();
79+
try {
80+
client.initialize();
81+
String sessionId = client.newSession(new AcpSchema.NewSessionRequest("/workspace", List.of())).sessionId();
82+
for (int i = 1; i <= 3; i++) {
83+
long t0 = System.nanoTime();
84+
AcpSchema.PromptResponse response = client.prompt(new AcpSchema.PromptRequest(sessionId,
85+
List.of(new AcpSchema.TextContent("prompt " + i))));
86+
System.out.printf("streaming prompt %d returned %s in %d ms (%d updates so far)%n", i,
87+
response.stopReason(), (System.nanoTime() - t0) / 1_000_000, updates.size());
88+
assertThat(response.stopReason()).isEqualTo(AcpSchema.StopReason.END_TURN);
89+
}
90+
}
91+
finally {
92+
client.closeGracefully();
93+
agent.closeGracefully();
94+
pair.closeGracefully().block(TIMEOUT);
95+
}
96+
}
97+
98+
}

0 commit comments

Comments
 (0)