Integrate PRs 1469 and 1478 from brunoborges

[edburns]

Supersedes #1469 .
Superseded by #1483.

Risk Assessment: PR #1469 — feat(java): add JDK 25 default executor

Superseding PR: #1469
Branch: edburns/review-brunoborges-pr-1469
Assessed: 2026-05-27

---

What This PR Does

Introduces a multi-release JAR (MR-JAR) mechanism so the Java SDK automatically uses Executors.newVirtualThreadPerTaskExecutor() on JDK 25+ while falling back to ForkJoinPool.commonPool() on older JDKs. It also refactors CopilotClient to always hold a non-null executor field and properly shut down SDK-owned executors on close.

Key Components

File	Role
src/main/java/.../InternalExecutorProvider.java	Base (JDK 17) — returns ForkJoinPool.commonPool(), isOwned() = false
src/main/java25/.../InternalExecutorProvider.java	JDK 25 variant — returns newVirtualThreadPerTaskExecutor(), isOwned() = true
pom.xml (profile java25-multi-release)	Compiles src/main/java25 with --release 25, sets Multi-Release: true manifest
CopilotClient.java	Resolves executor once in constructor; shuts down owned executor on close
CopilotClientOptions.java	Javadoc updates clarifying lifecycle ownership

---

Risk Matrix

#	Risk	Severity	Likelihood	Mitigation / Notes
1	MR-JAR build only activates on JDK 25+ — CI/CD that builds on JDK 17–24 will produce a JAR without the Java-25 overlay. Consumers on JDK 25 would silently get the base class (ForkJoinPool).	Medium	High if CI builds on JDK 17	Verify the release build runs on JDK 25+. Alternatively cross-compile unconditionally (see Recommendation 1).
2	multiReleaseOutput is a maven-compiler-plugin 3.12+ feature — The POM uses 3.15.0 which supports it, but this is a relatively new Maven feature with limited community battle-testing.	Low	Low	Pin version; add a smoke-test in CI that inspects the JAR structure.
3	Owned-executor shutdown races — shutdownOwnedExecutor() is called in both forceStop() (via whenComplete) and close() (in finally). Double-call is safe (shutdown() is idempotent), but the awaitTermination in close() may block the calling thread up to 10 s after forceStop already ran.	Low	Low	Acceptable; could add a shutdownOnce AtomicBoolean for clarity but not required.
4	ForkJoinPool.commonPool() is not owned — virtual-thread executor IS owned — The asymmetry is intentional but surprising. If someone later refactors and swaps implementations they must preserve this contract or risk shutting down the common pool.	Medium	Low	Well-documented in isOwned() semantics; consider adding a comment in the base class explaining why it returns false.
5	Test uses reflection to access ownedExecutor private field — Fragile coupling to internals. If the field is renamed or the executor strategy changes, the test breaks silently.	Low	Medium	Acceptable for an internal-facing test; flag for future cleanup if API evolves.
7	JaCoCo exclusion hides coverage gaps — The POM excludes META-INF/versions/**/*.class from coverage. The JDK-25 code path is tested only by the reflection-based test, which itself is skipped on JDK < 25.	Medium	Medium	Ensure CI matrix includes a JDK 25 runner to get actual coverage of that path.
8	No integration/E2E test exercises virtual-thread path end-to-end — Only unit tests verify the provider; no E2E test creates a session using virtual threads and confirms correctness under load.	Medium	Medium	Consider adding an E2E scenario on JDK 25 that runs a multi-turn session.
9	Behavioral change for existing users who relied on null-executor semantics — Previously, passing no executor caused CompletableFuture.supplyAsync(fn) (uses common pool). Now it always passes an explicit executor. Behavior is identical on JDK < 25 but changes on JDK 25+.	Low	Low	This is the intended improvement. Document in release notes.

---

Overall Assessment

Dimension	Rating
Correctness	✅ Sound — the MR-JAR pattern is used correctly; executor lifecycle is handled properly.
Complexity	⚠️ Moderate — MR-JARs add build complexity and require CI awareness.
Test coverage	⚠️ Partial on JDK 25 path — depends on CI having a JDK 25 runner.
Backwards compatibility	✅ Fully compatible — no public API change; behavior unchanged on JDK < 25.
Maintainability	⚠️ Moderate — two source trees for the same class; rename must happen in both places.

Verdict: Acceptable risk for GA provided:

1. The CI/CD release pipeline builds on JDK 25+ (or cross-compiles unconditionally) so the MR-JAR overlay is actually present in published artifacts.
2. At least one CI matrix entry runs tests on JDK 25 to exercise the virtual-thread path.
3. Release notes mention the new default executor behavior on JDK 25+.

---

Recommendations

1. Consider unconditional cross-compilation — Remove the <jdk>[25,)</jdk> activation and always compile the java25 sources with --release 25 (Maven will skip if the toolchain can't satisfy it, but a CI job with JDK 25 will always produce the overlay). This avoids the "built on wrong JDK" silent failure.

2. Add a build-time JAR verification — A Maven Enforcer or maven-jar-plugin check that confirms META-INF/versions/25/com/github/copilot/InternalExecutorProvider.class exists in the final artifact when built on JDK 25+.

3. Document the two-source-tree requirement — Add a brief note in java/README.md or a CONTRIBUTING.md section explaining that changes to InternalExecutorProvider must be mirrored in both src/main/java/ and src/main/java25/.

4. Consider a @since tag — Mark the virtual-thread behavior with @since 1.x in Javadoc for SDK consumers who want to understand when this appeared.

[Copilot]

Pull request overview

Supersedes #1469. Adds an internal multi-release executor provider so the Java SDK automatically uses virtual threads on JDK 25+ and ForkJoinPool.commonPool() on older JDKs, while making CopilotClient consistently route async work through a single resolved executor and shut down only the SDK-owned default.

Changes:

• Introduce package-private InternalExecutorProvider with a Java 17 baseline (common pool, unowned) and a src/main/java25 multi-release variant (virtual-thread-per-task, owned).
• Wire CopilotClient to resolve executor/ownedExecutor once in the constructor, use it for start, RPC dispatch, session creation/resume, and stop, and shut down the owned executor in close/forceStop.
• Update Maven build with a java25-multi-release profile (JDK ≥ 25) producing META-INF/versions/25 classes and Multi-Release: true, exclude those from JaCoCo, refine CopilotClientOptions Javadoc, and add InternalExecutorProviderTest.

Show a summary per file

File	Description
java/src/main/java/com/github/copilot/InternalExecutorProvider.java	Java 17 baseline provider returning ForkJoinPool.commonPool() (unowned).
java/src/main/java25/com/github/copilot/InternalExecutorProvider.java	JDK 25 multi-release provider returning an owned virtual-thread executor.
java/src/main/java/com/github/copilot/CopilotClient.java	Stores resolved executor/ownedExecutor, uses it everywhere, shuts down owned executor in close/forceStop.
java/src/main/java/com/github/copilot/rpc/CopilotClientOptions.java	Javadoc clarifies SDK-default executor strategy and user-owned shutdown contract.
java/pom.xml	Adds JDK 25 multi-release compile profile, manifest entry, and JaCoCo exclusion for META-INF/versions/**.
java/src/test/java/com/github/copilot/InternalExecutorProviderTest.java	Verifies base provider, ownership behavior, and JDK 25 multi-release provider/client shutdown via a temp MR jar.

Copilot's findings

• Files reviewed: 7/7 changed files
• Comments generated: 2

[brunoborges]

@edburns I worked on a slightly different approach, with improved tests as well. I was going to update the original PR but since you closed it, I created a new one: #1478

[edburns]

@brunoborges I will pull commits in from your #1478 . I already invested time in the CI approach and I will prioritize that approach with your code changes in 1478.

[edburns]

@brunoborges to give you an idea of what I'm doing, see this prompt:

---

Use the gh CLI to interact with GitHub.

I have checked out the topic branch of #1478 into my worktree, as shown in this output.

git worktree list
/home/edburns/workareas/copilot-sdk-00  8172d123 [main]
/home/edburns/workareas/copilot-sdk     ca560bd4 [edburns/review-brunoborges-pr-1469]
/home/edburns/workareas/copilot-sdk-01  e46b7280 [edburns/review-brunoborges-pr-1478]

I intend for edburns/review-brunoborges-pr-1469 the PR that ultimately goes in.

Bruno has created his 1478 with this message:

@edburns I worked on a slightly different approach, with improved tests as well. I was going to update the original PR but since you closed it, I created a new one: #1478

I want to keep the JDK 17/JDK 25 approach we evolved here, but pull in the code changes from Bruno's other commit.

---

I'm iterating it here.

[edburns]

Here's a summary of how #1478 differs from #1469:

Key Differences

Aspect	PR #1469	PR #1478
API naming	isOwned(Executor) static method	canBeShutdown() instance method
Provider design	Static factory: InternalExecutorProvider.create() + separate isOwned()	Instance-based: new InternalExecutorProvider(userProvided) with .get() + .canBeShutdown()
CopilotClient field	ownedExecutor field (nullable ExecutorService)	executor + executorCanBeShutdown (boolean)
Shutdown threading	Direct shutdownNow() inline	Dedicated SHUTDOWN_DISPATCHER thread to avoid deadlock when forceStop() continuation runs on the owned executor itself
JAR verification	maven-enforcer-plugin (requireFilesExist on class output dir)	Antrun <zipentry> check inside the java25-multi-release profile that inspects the actual packaged JAR
Tests	Reflection-based test accessing ownedExecutor private field; class-loader isolation test iterating MR-JAR entries	InternalExecutorProviderTest (direct package-private API, no reflection on CopilotClient internals) + Failsafe integration test (InternalExecutorProviderIT) spawning a child JVM against the real packaged JAR
Test infra	Surefire only	Adds maven-failsafe-plugin for IT
CI approach	Single JDK per run	Matrix ["17", "25"] — both JDKs build from scratch
README	Commented-out virtual-thread example in quick start	Clean single try (var client = new CopilotClient()) — no manual executor needed

What #1478 improves over #1469

1. No reflection in tests — Tests use package-private access directly; the Failsafe IT spawns a fresh JVM against the real JAR to prove MR-JAR dispatch works.
2. Safer shutdown — Dedicated daemon thread for executor shutdown avoids the subtle deadlock where awaitTermination runs on a thread belonging to the very executor being shut down.
3. JAR-level verification — Checks the actual .jar entry (not just the class output directory), catching packaging/shading issues.
4. Cleaner API — Instance-based provider avoids the awkward static isOwned(Executor) that could be misused.

What your branch (#1477/1469) has that #1478 doesn't

1. maven-enforcer-plugin requiring JDK 25+ to build
2. JDK 17 re-run approach (same job, surefire:test without recompilation)
3. Banner echo (⚠⚠⚠ Using Java ...)
4. workflow_dispatch input for opt-in JDK 17 testing

Recommendation for cherry-picking

From #1478, you'd want to pull in:

• InternalExecutorProvider.java (both base + java25) — the instance-based design
• CopilotClient.java changes — the SHUTDOWN_DISPATCHER, executor/executorCanBeShutdown fields, shutdownOwnedExecutor() method
• CopilotClientOptions.java Javadoc updates
• InternalExecutorProviderTest.java, InternalExecutorProviderProbe.java, InternalExecutorProviderIT.java
• maven-failsafe-plugin addition to pom.xml
• The antrun verify-java25-overlay execution (inside the profile, checking the JAR zip entry)
• README.md simplification

While keeping your branch's CI/workflow/enforcer approach intact.

Completed: Check and modify Emacs compilation-error-regexp-alist for Maven output (5/5)

[edburns]

Cherry-pick code changes from brunoborges/java-jdk25-default-executor:

• Instance-based InternalExecutorProvider with canBeShutdown() API
• SHUTDOWN_DISPATCHER to avoid deadlock in forceStop()
• Idempotent shutdownOwnedExecutor() (short-circuits if already shut down)
• maven-failsafe-plugin + InternalExecutorProviderIT (spawns child JVM against packaged JAR)
• InternalExecutorProviderProbe for IT verification
• Antrun verify-java25-overlay (checks actual JAR zip entry in java25-multi-release profile)
• Simplified README (no manual executor configuration needed)
• Split unit tests into single-assertion methods

Co-authored-by: Bruno Borges bruno@brunoborges.com"

Done. All cherry-picked and committed. Let me verify the final state:

Ran terminal command: cd /home/edburns/workareas/copilot-sdk && git log --oneline main..HEAD

Cherry-picked successfully. Your branch edburns/review-brunoborges-pr-1469 now has:

• Your CI approach: enforcer requiring JDK 25, banner echo, workflow_dispatch with test-jdk17 input, surefire:test re-run (no recompile)
• Bruno's feat(java): JDK 25 default virtual-thread executor via multi-release JAR #1478 code improvements: instance-based InternalExecutorProvider, SHUTDOWN_DISPATCHER, failsafe IT, JAR-level antrun verification, simplified README

Completed: Cherry-pick changes from PR 1478 (6/7)

[edburns]

I've added banners to the test so when we run the tests two ways, we see:

[WARNING] [echo] ⚠⚠⚠ Using Java 25.0.2 (Microsoft) to run tests.
...
[WARNING] [echo] ⚠⚠⚠ Using Java 17.0.18 (Microsoft) to run tests.

[edburns]

Tests run https://github.com/github/copilot-sdk/actions/runs/26589787688 .

[edburns]

This one has proved too messy. I'll take Bruno's #1478 and apply the CI/CD and pom changes to it.