Skip to content

Add **Shared ScheduledExecutorService** for timeouts#38

Merged
brunoborges merged 9 commits intomainfrom
edburns/dd-2758695-virtual-threads
Mar 30, 2026
Merged

Add **Shared ScheduledExecutorService** for timeouts#38
brunoborges merged 9 commits intomainfrom
edburns/dd-2758695-virtual-threads

Conversation

@edburns
Copy link
Copy Markdown
Collaborator

@edburns edburns commented Mar 30, 2026

Before the change?

  • CopilotSession.sendAndWait() created a new ScheduledExecutorService per call, spawning a new OS thread (~1 MB stack) for every timeout, and never cancelling it on close() — leaving stale timeouts able to fire after the session was closed.
  • Test Javadoc referenced "current buggy behavior" and "Without a fix" language, which becomes misleading once the fix is merged.
  • ZeroTimeoutContractTest created a CopilotSession without closing it, leaking the executor's thread across tests.

After the change?

CopilotSession.java

  • Added ScheduledExecutorService import.
  • New field timeoutScheduler: shared single-thread scheduler, daemon thread named sendAndWait-timeout.
  • Initialized in 3-arg constructor.
  • sendAndWait(): replaced per-call Executors.newSingleThreadScheduledExecutor() with timeoutScheduler.schedule(). Cleanup calls timeoutTask.cancel(false) instead of scheduler.shutdown().
  • close(): added timeoutScheduler.shutdownNow() before the blocking session.destroy RPC call so stale timeouts cannot fire after close.

TimeoutEdgeCaseTest.java (new)

  • testTimeoutDoesNotFireAfterSessionClose: regression assertion that close() cancels pending timeouts (future not completed by stale TimeoutException).
  • testSendAndWaitReusesTimeoutThread: regression assertion that two sendAndWait calls share one scheduler thread instead of spawning two.
  • Uses reflection to construct a hanging JsonRpcClient (blocking InputStream, sink OutputStream).
  • Class and method Javadoc rewritten as behavioral contracts rather than descriptions of pre-fix behavior.

ZeroTimeoutContractTest.java (new)

  • Verifies the documented contract that timeoutMs <= 0 means "no timeout".
  • Uses try-with-resources so the session (and its ScheduledExecutorService) is always closed after the test.
  • Stubs session.destroy to return CompletableFuture.completedFuture(null) so close() is non-blocking; all other invoke() calls return a pending future.

SchedulerShutdownRaceTest.java (new)

  • Regression coverage for the race between sendAndWait() and close(): asserts that if the scheduler is shut down between ensureNotTerminated() and schedule(), sendAndWait() returns a future that completes exceptionally with RejectedExecutionException rather than throwing directly.
  • Javadoc rewritten as an invariant/contract assertion rather than a description of pre-fix failure mode.

pom.xml

  • Added Mockito (mockito-core) as a test-scoped dependency to support the new Mockito-based unit tests.

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • mvn spotless:apply has been run to format the code
  • mvn clean verify passes locally

Does this introduce a breaking change?

  • Yes
  • No

edburns and others added 7 commits March 27, 2026 14:31
…xecutorService`** for timeouts

## CopilotSession.java

- Added `ScheduledExecutorService` import.
- New field `timeoutScheduler`: shared single-thread scheduler, daemon thread named `sendAndWait-timeout`.
- Initialized in 3-arg constructor.
- `sendAndWait()`: replaced per-call `Executors.newSingleThreadScheduledExecutor()` with `timeoutScheduler.schedule()`. Cleanup calls `timeoutTask.cancel(false)` instead of `scheduler.shutdown()`.
- `close()`: added `timeoutScheduler.shutdownNow()` before the blocking `session.destroy` RPC call so stale timeouts cannot fire after close.

## TimeoutEdgeCaseTest.java (new)

- `testTimeoutDoesNotFireAfterSessionClose`: proves close() cancels pending timeouts (future not completed by stale TimeoutException).
- `testSendAndWaitReusesTimeoutThread`: proves two sendAndWait calls share one scheduler thread instead of spawning two.
- Uses reflection to construct a hanging `JsonRpcClient` (blocking InputStream, sink OutputStream).

Signed-off-by: Ed Burns <edburns@microsoft.com>
…xecutorService`** for timeouts

## CopilotSession.java

- Added `ScheduledExecutorService` import.
- New field `timeoutScheduler`: shared single-thread scheduler, daemon thread named `sendAndWait-timeout`.
- Initialized in 3-arg constructor.
- `sendAndWait()`: replaced per-call `Executors.newSingleThreadScheduledExecutor()` with `timeoutScheduler.schedule()`. Cleanup calls `timeoutTask.cancel(false)` instead of `scheduler.shutdown()`.
- `close()`: added `timeoutScheduler.shutdownNow()` before the blocking `session.destroy` RPC call so stale timeouts cannot fire after close.

## TimeoutEdgeCaseTest.java (new)

- `testTimeoutDoesNotFireAfterSessionClose`: proves close() cancels pending timeouts (future not completed by stale TimeoutException).
- `testSendAndWaitReusesTimeoutThread`: proves two sendAndWait calls share one scheduler thread instead of spawning two.
- Uses reflection to construct a hanging `JsonRpcClient` (blocking InputStream, sink OutputStream).

Signed-off-by: Ed Burns <edburns@microsoft.com>
Replace Executors.newSingleThreadScheduledExecutor with an explicit
ScheduledThreadPoolExecutor so we can enable removeOnCancelPolicy(true).
Without this, each call to sendAndWait() that completes normally cancels
its timeout task, but the cancelled task remains in the scheduler's work
queue, leaking memory over the lifetime of the session.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Ed Burns <edburns@microsoft.com>
pom.xml

  Add mockito-core 5.17.0 as a test dependency.

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

  Replace Executors.newSingleThreadScheduledExecutor with explicit
  ScheduledThreadPoolExecutor and enable removeOnCancelPolicy(true) so
  cancelled timeout tasks are purged from the work queue immediately.

  Wrap timeoutScheduler.schedule() in a try-catch for
  RejectedExecutionException. On rejection (close() race), the event
  subscription is cleaned up and the returned future completes
  exceptionally instead of throwing uncaught.

src/test/java/com/github/copilot/sdk/SchedulerShutdownRaceTest.java (new)

  TDD test that reproduces the scheduler shutdown race. Uses Mockito to
  stub JsonRpcClient.invoke(), then shuts down the scheduler without
  setting isTerminated, and asserts sendAndWait() returns a failed future
  rather than throwing RejectedExecutionException.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Ed Burns <edburns@microsoft.com>
…Wait

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>
src/test/java/com/github/copilot/sdk/TimeoutEdgeCaseTest.java

  Wrap CopilotSession in try-with-resources in both tests so the session
  and its scheduler thread are always cleaned up, even if an assertion
  fails before the explicit close() call. In test 1, the explicit
  session.close() is kept because it is the action under test; the
  try-with-resources provides a safety net via idempotent double-close.
  In test 2, the explicit session.close() is removed since it was purely
  cleanup.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings March 30, 2026 20:19
@edburns edburns requested a review from brunoborges March 30, 2026 20:20
@brunoborges
Copy link
Copy Markdown
Collaborator

@edburns did this branch build successfully on your local machine?

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a session-level timeout scheduler to CopilotSession#sendAndWait(...) to avoid per-call thread creation and to prevent stale timeout tasks from firing after close(), with new tests covering timeout/threading edge cases.

Changes:

  • Introduce a shared ScheduledExecutorService in CopilotSession for sendAndWait timeouts, canceling scheduled tasks on completion and shutting down the scheduler during close().
  • Add new tests to validate timeout behavior after close(), scheduler reuse, zero-timeout semantics, and the shutdown/scheduling race.
  • Add Mockito as a test dependency to support the new unit tests.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/main/java/com/github/copilot/sdk/CopilotSession.java Replaces per-call timeout executors with a session-owned scheduler and adds shutdown behavior in close().
src/test/java/com/github/copilot/sdk/TimeoutEdgeCaseTest.java Adds regression tests for stale timeouts after close and scheduler thread reuse.
src/test/java/com/github/copilot/sdk/ZeroTimeoutContractTest.java Adds a contract test for timeoutMs <= 0 meaning “no timeout”.
src/test/java/com/github/copilot/sdk/SchedulerShutdownRaceTest.java Adds a regression test for close()/sendAndWait() scheduling race handling.
pom.xml Adds mockito-core as a test-scoped dependency.
Comments suppressed due to low confidence (2)

src/test/java/com/github/copilot/sdk/TimeoutEdgeCaseTest.java:90

  • This test will deterministically spend ~5 seconds in CopilotSession.close() because the constructed JsonRpcClient never completes the session.destroy RPC and close() waits up to 5s. That makes the unit test suite significantly slower; consider using a mocked JsonRpcClient (or stubbing session.destroy to complete after a short delay) so the same behavior is exercised without a fixed multi-second stall.
                // close() blocks up to 5s on session.destroy RPC. The 2s timeout
                // fires during that window with the current per-call scheduler.
                session.close();

                assertFalse(result.isDone(), "Future should not be completed by a timeout after session is closed. "
                        + "The per-call ScheduledExecutorService leaked a TimeoutException.");
            }

src/test/java/com/github/copilot/sdk/SchedulerShutdownRaceTest.java:56

  • The test creates a CopilotSession but does not close it. Even though the scheduler is manually shut down, the session still retains handlers/state; consider closing it via try-with-resources and stubbing session.destroy on the mocked JsonRpcClient to return a completed future so close() doesn't block for 5 seconds.
        var session = ctor.newInstance("race-test", mockRpc, null);

        // Shut down the scheduler without setting isTerminated,
        // simulating the race window between ensureNotTerminated() and schedule()
        var schedulerField = CopilotSession.class.getDeclaredField("timeoutScheduler");
        schedulerField.setAccessible(true);
        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.
        CompletableFuture<?> result = session.sendAndWait(new MessageOptions().setPrompt("test"), 5000);

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@edburns
Copy link
Copy Markdown
Collaborator Author

edburns commented Mar 30, 2026

@brunoborges wrote:

did this branch build successfully on your local machine?

Yes:

Full suite: 472 tests, 0 failures

@brunoborges brunoborges merged commit 562527f into main Mar 30, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants