Skip to content

Accept optional Executor on CopilotClientOptions to mitigate pool starvation#40

Open
edburns wants to merge 7 commits intomainfrom
edburns/dd-2758695-virtual-threads-accept-executor
Open

Accept optional Executor on CopilotClientOptions to mitigate pool starvation#40
edburns wants to merge 7 commits intomainfrom
edburns/dd-2758695-virtual-threads-accept-executor

Conversation

@edburns
Copy link
Copy Markdown
Collaborator

@edburns edburns commented Mar 30, 2026

Before the change?

  • All internal CompletableFuture.*Async calls used ForkJoinPool.commonPool(), which could become starved when multiple sessions blocked concurrently.
  • CopilotClientOptions had no way for callers to supply a custom thread pool.
  • RejectedExecutionException thrown synchronously by any async submission site would propagate to callers unexpectedly, potentially leaving JSON-RPC requests unanswered or skipping connection cleanup.

After the change?

  • CopilotClientOptions has a new Executor field with fluent setExecutor(Executor) / getExecutor() accessors. Passing null preserves the original ForkJoinPool.commonPool() behaviour.
  • Clone support for the new field is included.
  • The executor is wired through CopilotClient start/stop, RpcHandlerDispatcher (all 5 runAsync call sites via a private helper), and CopilotSession (executeToolAndRespondAsync / executePermissionAndRespondAsync).
  • All async submission sites now catch RejectedExecutionException and handle it context-sensitively:
    • CopilotClient.startCore() — returns CompletableFuture.failedFuture(e) so rejection surfaces through the returned future instead of throwing synchronously.
    • CopilotClient.stop() — falls back to closing each session inline so cleanup always completes even if the executor has been shut down.
    • RpcHandlerDispatcher.runAsync() — falls back to running the handler inline on the notification thread so the JSON-RPC request is never left unanswered.
    • CopilotSession.executeToolAndRespondAsync() and executePermissionAndRespondAsync() — fall back to running the task inline so pending tool/permission calls always receive a reply.
  • ExecutorWiringTest (6 tests) verifies the executor is used for client start, tool-call dispatch, permission dispatch, user-input dispatch, hooks dispatch, and client stop.
  • The multiple-sessions cookbook gains a new "Providing a custom Executor for parallel sessions" section with a pool-starvation explanation and a complete JBang example.
  • RpcHandlerDispatcherTest updated for the new 3-arg constructor (passes null).

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 added 3 commits March 27, 2026 17:20
…nc calls through it; shared timeout scheduler.

src/main/java/com/github/copilot/sdk/json/CopilotClientOptions.java
- Added `Executor` field, `getExecutor()`, fluent `setExecutor(Executor)` with pending-null guard, and clone support.

src/main/java/com/github/copilot/sdk/CopilotClient.java
- Extracted `startCoreBody()` from `startCore()` lambda; `supplyAsync` uses provided executor when non-null.
- `stop()` routes session-close `runAsync` through provided executor when non-null.
- Passes executor to `RpcHandlerDispatcher` constructor.
- Sets executor on new sessions via `session.setExecutor()` in `createSession` and `resumeSession`.

src/main/java/com/github/copilot/sdk/RpcHandlerDispatcher.java
- Added `Executor` field and 3-arg constructor.
- All 5 `CompletableFuture.runAsync()` calls now go through private `runAsync(Runnable)` helper that uses executor when non-null.

src/main/java/com/github/copilot/sdk/CopilotSession.java
- Added `Executor` field and package-private `setExecutor()`.
- Replaced per-call `ScheduledExecutorService` with shared `timeoutScheduler` (daemon thread, shut down in `close()`).
- `executeToolAndRespondAsync` and `executePermissionAndRespondAsync` use executor when non-null.

src/test/java/com/github/copilot/sdk/ExecutorWiringTest.java (new)
- 6 E2E tests using `TrackingExecutor` decorator to verify all `*Async` paths route through the provided executor: client start, tool call, permission, user input, hooks, and client stop.

src/test/java/com/github/copilot/sdk/RpcHandlerDispatcherTest.java
- Updated constructor call to pass `null` for new executor parameter.
…8695-virtual-threads-accept-executor

# Conflicts:
#	src/main/java/com/github/copilot/sdk/CopilotSession.java
modified:   src/main/java/com/github/copilot/sdk/CopilotClient.java

- Spotless.

modified:   src/main/java/com/github/copilot/sdk/json/CopilotClientOptions.java

- Remove stub from TDD red phase.

modified:   src/site/markdown/cookbook/multiple-sessions.md

- Document new feature.

modified:   src/test/java/com/github/copilot/sdk/ExecutorWiringTest.java

- Update test documentation.

Signed-off-by: Ed Burns <edburns@microsoft.com>
Copilot AI review requested due to automatic review settings March 30, 2026 21:03
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 support for supplying a caller-provided Executor to route the SDK’s internal CompletableFuture.*Async work off ForkJoinPool.commonPool(), reducing starvation risk when running multiple blocking sessions concurrently.

Changes:

  • Introduces CopilotClientOptions.getExecutor() / setExecutor(Executor) (nullable) and includes it in clone().
  • Wires the executor through CopilotClient start/stop paths, RpcHandlerDispatcher, and CopilotSession tool/permission response execution.
  • Adds E2E coverage (ExecutorWiringTest) and documents the new option in the multiple-sessions cookbook.

Reviewed changes

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

Show a summary per file
File Description
src/main/java/com/github/copilot/sdk/json/CopilotClientOptions.java Adds nullable Executor option and clones it.
src/main/java/com/github/copilot/sdk/CopilotClient.java Uses provided executor for startCore() and stop() async work; passes executor into dispatcher and sessions.
src/main/java/com/github/copilot/sdk/RpcHandlerDispatcher.java Adds executor-aware async dispatch helper for server-to-client handler execution.
src/main/java/com/github/copilot/sdk/CopilotSession.java Routes broadcast tool/permission response tasks through provided executor when present.
src/test/java/com/github/copilot/sdk/ExecutorWiringTest.java New E2E tests asserting executor usage across key async dispatch paths.
src/test/java/com/github/copilot/sdk/RpcHandlerDispatcherTest.java Updates dispatcher construction for new constructor signature.
src/site/markdown/cookbook/multiple-sessions.md Documents pool starvation and how to provide a custom executor.
Comments suppressed due to low confidence (1)

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

  • Same as tool execution: submitting this permission-response task via CompletableFuture.runAsync(task, executor) can throw RejectedExecutionException synchronously if the provided executor rejects tasks. Consider handling submission failure to ensure the pending permission request is either answered (e.g., denied) or otherwise completed deterministically.
        if (executor != null) {
            CompletableFuture.runAsync(task, executor);
        } else {
            CompletableFuture.runAsync(task);
        }

@edburns edburns requested a review from brunoborges March 30, 2026 22:32
@brunoborges
Copy link
Copy Markdown
Collaborator

@edburns should we throw an IAE if the user passes null?

@edburns
Copy link
Copy Markdown
Collaborator Author

edburns commented Mar 30, 2026

@brunoborges wrote:

should we throw an IAE if the user passes null?

We don't do that for other setters in CopilotClientOptions. I might say "yes" we should do that, but I'd like to understand why we would not do it for all other options before I fully commit to "yes" in the case of Executor.

modified:   README.md

- Use the "uncomment these three lines to get Virtual Threads" approach

modified:   src/main/java/com/github/copilot/sdk/json/CopilotClientOptions.java

- Cleanup. Sorting.

Signed-off-by: Ed Burns <edburns@microsoft.com>
@Deprecated
public CopilotClientOptions setAutoRestart(boolean autoRestart) {
this.autoRestart = autoRestart;
public CopilotClientOptions setEnvironment(Map<String, String> environment) {
*/
public int getPort() {
return port;
public String[] getCliArgs() {
@brunoborges
Copy link
Copy Markdown
Collaborator

@brunoborges wrote:

should we throw an IAE if the user passes null?

We don't do that for other setters in CopilotClientOptions. I might say "yes" we should do that, but I'd like to understand why we would not do it for all other options before I fully commit to "yes" in the case of Executor.

There was no deep thinking before. Making null-safe now leads us to a more stable and strict API, and towards GA.

What I mean is that this is perhaps now the moment to make the API safer.

@edburns
Copy link
Copy Markdown
Collaborator Author

edburns commented Mar 31, 2026

@brunoborges wrote:

There was no deep thinking before. Making null-safe now leads us to a more stable and strict API, and towards GA.

What I mean is that this is perhaps now the moment to make the API safer.

Of course. I'll take this as consent to add the null checks throughout, at least in the CopilotClientOptions class.

I'll take the remainder of the null checks as a separate issue.

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