Skip to content
Merged
Prev Previous commit
Update Javadoc in TimeoutEdgeCaseTest and SchedulerShutdownRaceTest t…
…o use contract/regression language

Agent-Logs-Url: https://github.com/github/copilot-sdk-java/sessions/82d9999d-8d2f-4ccc-b0a9-0dfe932f8f78

Co-authored-by: edburns <75821+edburns@users.noreply.github.com>
  • Loading branch information
Copilot and edburns authored Mar 30, 2026
commit 4c6c2ecb5c5053ae8b93c6d2928be2f963dbdce5
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,16 @@
import com.github.copilot.sdk.json.MessageOptions;

/**
* Reproduces the race between {@code sendAndWait()} and {@code close()}.
* Regression coverage for the race between {@code sendAndWait()} and
* {@code close()}.
* <p>
* If {@code close()} shuts down the timeout scheduler after
* {@code ensureNotTerminated()} passes but before
* {@code timeoutScheduler.schedule()} executes, the schedule call throws
* {@link RejectedExecutionException}. Without a fix the exception propagates
* uncaught, leaking the event subscription and leaving the returned future
* incomplete.
* {@link RejectedExecutionException}. This test asserts that
* {@code sendAndWait()} handles this race by returning a future that completes
* exceptionally (rather than propagating the exception to the caller or leaving
* the returned future incomplete).
*/
Comment thread
edburns marked this conversation as resolved.
public class SchedulerShutdownRaceTest {

Expand All @@ -50,8 +52,7 @@ void sendAndWaitShouldReturnFailedFutureWhenSchedulerIsShutDown() throws Excepti
var scheduler = (ScheduledExecutorService) schedulerField.get(session);
scheduler.shutdownNow();

// With the fix: sendAndWait returns a future that completes exceptionally.
// Without the fix: sendAndWait throws RejectedExecutionException directly.
// sendAndWait must return a failed future rather than throwing directly.
CompletableFuture<?> result = session.sendAndWait(new MessageOptions().setPrompt("test"), 5000);

assertNotNull(result, "sendAndWait should return a future, not throw");
Expand Down
32 changes: 14 additions & 18 deletions src/test/java/com/github/copilot/sdk/TimeoutEdgeCaseTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,16 @@
import com.github.copilot.sdk.json.MessageOptions;

/**
* Tests for timeout edge cases in {@link CopilotSession#sendAndWait}.
* Regression tests for timeout edge cases in
* {@link CopilotSession#sendAndWait}.
* <p>
* These tests prove two defects in the current per-call
* These tests assert two behavioral contracts of the shared
* {@code ScheduledExecutorService} approach:
* <ol>
* <li>A timeout fires after {@code close()}, leaking a {@code TimeoutException}
* onto the returned future.</li>
* <li>Each {@code sendAndWait} call spawns a new OS thread (~1 MB stack),
* instead of reusing a shared scheduler thread.</li>
* <li>A pending timeout must NOT fire after {@code close()} and must NOT
* complete the returned future with a {@code TimeoutException}.</li>
* <li>Multiple {@code sendAndWait} calls must reuse a single shared scheduler
* thread rather than spawning a new OS thread per call.</li>
* </ol>
Comment thread
edburns marked this conversation as resolved.
*/
public class TimeoutEdgeCaseTest {
Expand Down Expand Up @@ -62,13 +63,10 @@ public int read() throws IOException {
* After {@code close()}, the future returned by {@code sendAndWait} must NOT be
* completed by a stale timeout.
* <p>
* Current buggy behavior: the per-call scheduler is not cancelled by
* {@code close()}, so its 2-second timeout fires during the 5-second
* {@code session.destroy} RPC wait, completing the future with
* {@code TimeoutException}.
* <p>
* Expected behavior after fix: {@code close()} cancels pending timeouts before
* the blocking RPC call, so the future remains incomplete.
* Contract: {@code close()} shuts down the timeout scheduler before the
* blocking {@code session.destroy} RPC call, so any pending timeout task is
* cancelled and the future remains incomplete (not exceptionally completed with
* {@code TimeoutException}).
*/
@Test
void testTimeoutDoesNotFireAfterSessionClose() throws Exception {
Expand All @@ -94,13 +92,11 @@ void testTimeoutDoesNotFireAfterSessionClose() throws Exception {
}

/**
* A shared scheduler should reuse a single thread across multiple
* A shared scheduler must reuse a single thread across multiple
* {@code sendAndWait} calls, rather than spawning a new OS thread per call.
* <p>
* Current buggy behavior: two calls create two {@code sendAndWait-timeout}
* threads.
* <p>
* Expected behavior after fix: two calls still use only one scheduler thread.
* Contract: after two consecutive {@code sendAndWait} calls the number of live
* {@code sendAndWait-timeout} threads must not increase after the second call.
*/
@Test
void testSendAndWaitReusesTimeoutThread() throws Exception {
Expand Down
Loading