Skip to content

Commit a01f0b5

Browse files
edburnsCopilot
andcommitted
Honor documented contract: timeoutMs <= 0 means no timeout in sendAndWait
src/main/java/com/github/copilot/sdk/CopilotSession.java Skip scheduling the timeout task when timeoutMs <= 0, matching the Javadoc contract that 0 or negative means "no timeout". Previously, timeoutMs=0 would schedule an immediate timeout, contradicting the docs. The timeout cancel in the whenComplete cleanup is now guarded for the null case. src/test/java/com/github/copilot/sdk/ZeroTimeoutContractTest.java (new) TDD test that asserts the documented contract: calling sendAndWait with timeoutMs=0 should not cause the future to complete with a TimeoutException. Uses Mockito to stub JsonRpcClient.invoke(). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent a1668c7 commit a01f0b5

File tree

2 files changed

+75
-16
lines changed

2 files changed

+75
-16
lines changed

src/main/java/com/github/copilot/sdk/CopilotSession.java

Lines changed: 22 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -421,33 +421,39 @@ public CompletableFuture<AssistantMessageEvent> sendAndWait(MessageOptions optio
421421

422422
var result = new CompletableFuture<AssistantMessageEvent>();
423423

424-
// Schedule timeout on the shared session-level scheduler
425-
ScheduledFuture<?> timeoutTask;
426-
try {
427-
timeoutTask = timeoutScheduler.schedule(() -> {
428-
if (!future.isDone()) {
429-
future.completeExceptionally(
430-
new TimeoutException("sendAndWait timed out after " + timeoutMs + "ms"));
431-
}
432-
}, timeoutMs, TimeUnit.MILLISECONDS);
433-
} catch (RejectedExecutionException e) {
424+
// Schedule timeout on the shared session-level scheduler.
425+
// Per Javadoc, timeoutMs <= 0 means "no timeout".
426+
ScheduledFuture<?> timeoutTask = null;
427+
if (timeoutMs > 0) {
434428
try {
435-
subscription.close();
436-
} catch (IOException closeEx) {
437-
e.addSuppressed(closeEx);
429+
timeoutTask = timeoutScheduler.schedule(() -> {
430+
if (!future.isDone()) {
431+
future.completeExceptionally(
432+
new TimeoutException("sendAndWait timed out after " + timeoutMs + "ms"));
433+
}
434+
}, timeoutMs, TimeUnit.MILLISECONDS);
435+
} catch (RejectedExecutionException e) {
436+
try {
437+
subscription.close();
438+
} catch (IOException closeEx) {
439+
e.addSuppressed(closeEx);
440+
}
441+
result.completeExceptionally(e);
442+
return result;
438443
}
439-
result.completeExceptionally(e);
440-
return result;
441444
}
442445

443446
// When inner future completes, run cleanup and propagate to result
447+
final ScheduledFuture<?> taskToCancel = timeoutTask;
444448
future.whenComplete((r, ex) -> {
445449
try {
446450
subscription.close();
447451
} catch (IOException e) {
448452
LOG.log(Level.SEVERE, "Error closing subscription", e);
449453
}
450-
timeoutTask.cancel(false);
454+
if (taskToCancel != null) {
455+
taskToCancel.cancel(false);
456+
}
451457
if (!result.isDone()) {
452458
if (ex != null) {
453459
result.completeExceptionally(ex);
Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
/*---------------------------------------------------------------------------------------------
2+
* Copyright (c) Microsoft Corporation. All rights reserved.
3+
*--------------------------------------------------------------------------------------------*/
4+
5+
package com.github.copilot.sdk;
6+
7+
import static org.junit.jupiter.api.Assertions.*;
8+
import static org.mockito.ArgumentMatchers.*;
9+
import static org.mockito.Mockito.*;
10+
11+
import java.util.concurrent.CompletableFuture;
12+
import java.util.concurrent.TimeUnit;
13+
import java.util.concurrent.TimeoutException;
14+
15+
import org.junit.jupiter.api.Test;
16+
17+
import com.github.copilot.sdk.events.AssistantMessageEvent;
18+
import com.github.copilot.sdk.events.SessionIdleEvent;
19+
import com.github.copilot.sdk.json.MessageOptions;
20+
21+
/**
22+
* Verifies the documented contract that {@code timeoutMs <= 0} means "no
23+
* timeout" in {@link CopilotSession#sendAndWait(MessageOptions, long)}.
24+
*/
25+
public class ZeroTimeoutContractTest {
26+
27+
@SuppressWarnings("unchecked")
28+
@Test
29+
void sendAndWaitWithZeroTimeoutShouldNotTimeOut() throws Exception {
30+
// Build a session via reflection (package-private constructor)
31+
var ctor = CopilotSession.class.getDeclaredConstructor(
32+
String.class, JsonRpcClient.class, String.class);
33+
ctor.setAccessible(true);
34+
35+
var mockRpc = mock(JsonRpcClient.class);
36+
when(mockRpc.invoke(any(), any(), any()))
37+
.thenReturn(new CompletableFuture<>());
38+
39+
var session = ctor.newInstance("zero-timeout-test", mockRpc, null);
40+
41+
// Per the Javadoc: timeoutMs of 0 means "no timeout".
42+
// The future should NOT complete with TimeoutException.
43+
CompletableFuture<AssistantMessageEvent> result =
44+
session.sendAndWait(new MessageOptions().setPrompt("test"), 0);
45+
46+
// Give the scheduler a chance to fire if it was (incorrectly) scheduled
47+
Thread.sleep(200);
48+
49+
// The future should still be pending — not timed out
50+
assertFalse(result.isDone(),
51+
"Future should not be done; timeoutMs=0 means no timeout per Javadoc");
52+
}
53+
}

0 commit comments

Comments
 (0)