Skip to content

Commit e034359

Browse files
author
Mark Pollack
committed
Fix race condition in execute() terminal release
1 parent 0d8ca76 commit e034359

File tree

2 files changed

+56
-2
lines changed

2 files changed

+56
-2
lines changed

acp-core/src/main/java/com/agentclientprotocol/sdk/agent/DefaultPromptContext.java

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -207,12 +207,15 @@ public Mono<CommandResult> execute(Command command) {
207207
command.cwd(), envList, command.outputByteLimit()))
208208
.flatMap(createResp -> {
209209
String terminalId = createResp.terminalId();
210+
ReleaseTerminalRequest releaseReq = new ReleaseTerminalRequest(sessionId, terminalId);
210211

211212
return waitForTerminalExit(new WaitForTerminalExitRequest(sessionId, terminalId))
212213
.flatMap(exitResp -> getTerminalOutput(new TerminalOutputRequest(sessionId, terminalId))
213214
.map(outputResp -> new CommandResult(outputResp.output(), exitResp.exitCode(), false)))
214-
.doFinally(signal -> releaseTerminal(new ReleaseTerminalRequest(sessionId, terminalId))
215-
.subscribe());
215+
// Release terminal after getting result, then return result
216+
.flatMap(result -> releaseTerminal(releaseReq).thenReturn(result))
217+
// On error, still release terminal before propagating error
218+
.onErrorResume(error -> releaseTerminal(releaseReq).then(Mono.error(error)));
216219
});
217220
}
218221

acp-core/src/test/java/com/agentclientprotocol/sdk/agent/ConvenienceApiTest.java

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import java.util.concurrent.CopyOnWriteArrayList;
1111
import java.util.concurrent.CountDownLatch;
1212
import java.util.concurrent.TimeUnit;
13+
import java.util.concurrent.atomic.AtomicBoolean;
1314
import java.util.concurrent.atomic.AtomicReference;
1415

1516
import com.agentclientprotocol.sdk.client.AcpAsyncClient;
@@ -514,6 +515,56 @@ void executeWithCommandBuilderWorks() throws Exception {
514515
agent.closeGracefully();
515516
}
516517

518+
@Test
519+
void executeReleasesTerminalBeforeReturning() throws Exception {
520+
// This test verifies the fix for the race condition where releaseTerminal
521+
// was called with fire-and-forget .subscribe() instead of being awaited
522+
AtomicBoolean releaseCalledBeforeReturn = new AtomicBoolean(false);
523+
AtomicBoolean executeReturned = new AtomicBoolean(false);
524+
525+
AcpSyncAgent agent = AcpAgent.sync(transportPair.agentTransport())
526+
.requestTimeout(TIMEOUT)
527+
.initializeHandler(req -> InitializeResponse.ok())
528+
.newSessionHandler(req -> new NewSessionResponse("release-test", null, null))
529+
.promptHandler((request, context) -> {
530+
context.execute("echo", "test");
531+
// After execute() returns, releaseTerminal should have been called
532+
executeReturned.set(true);
533+
return PromptResponse.endTurn();
534+
})
535+
.build();
536+
537+
AcpAsyncClient client = AcpClient.async(transportPair.clientTransport())
538+
.requestTimeout(TIMEOUT)
539+
.createTerminalHandler(req -> Mono.just(new CreateTerminalResponse("term-rel")))
540+
.waitForTerminalExitHandler(req -> Mono.just(new WaitForTerminalExitResponse(0, null)))
541+
.terminalOutputHandler(req -> Mono.just(new TerminalOutputResponse("", false, null)))
542+
.releaseTerminalHandler(req -> {
543+
// Release should be called BEFORE execute() returns
544+
if (!executeReturned.get()) {
545+
releaseCalledBeforeReturn.set(true);
546+
}
547+
return Mono.just(new ReleaseTerminalResponse());
548+
})
549+
.build();
550+
551+
agent.start();
552+
Thread.sleep(100);
553+
554+
ClientCapabilities clientCaps = new ClientCapabilities(null, true);
555+
client.initialize(new InitializeRequest(1, clientCaps)).block(TIMEOUT);
556+
client.newSession(new NewSessionRequest("/workspace", List.of())).block(TIMEOUT);
557+
client.prompt(new PromptRequest("release-test", List.of(new TextContent("test")))).block(TIMEOUT);
558+
559+
// Verify that releaseTerminal was called before execute() returned
560+
assertThat(releaseCalledBeforeReturn.get())
561+
.as("releaseTerminal should be called before execute() returns (not fire-and-forget)")
562+
.isTrue();
563+
564+
client.closeGracefully().block(TIMEOUT);
565+
agent.closeGracefully();
566+
}
567+
517568
// ========================================================================
518569
// Factory method tests
519570
// ========================================================================

0 commit comments

Comments
 (0)