Commit f86c7e0
Integrate Bruno's PR 1478 with Ed's desired CI/CD changes (#1483)
Bruno authored the core code with Copilot.
* feat(java): add JDK 25 default executor
Use a multi-release JAR to select virtual threads as the default internal executor on JDK 25+, while retaining the common pool on older JDKs. Keep user-provided executors caller-owned and only shut down SDK-owned defaults.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* test(java): cover owned default executor shutdown
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* refactor(java): make default executor provider internal
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* refactor(java): update InternalExecutorProvider to manage executor ownership and shutdown capability
* feat(java): add integration tests for multi-release JAR behavior and executor management
* feat(java): add JDK 25 multi-release overlay class verification and update documentation
* test(java): split InternalExecutorProvider unit test into single-assertion tests
Addresses PR #1478 review (Copilot AI): the existing
baseProviderUsesCommonPoolWithoutOwnership method bundled three unrelated
assertions (commonPool identity, user-executor ownership, package-private
visibility). Split into baseProviderReturnsCommonPool,
userProvidedExecutorIsNotOwned, and providerIsPackagePrivate so failures
point at a single condition each.
* fix(java): dispatch owned-executor shutdown off the executor in forceStop()
Addresses PR #1478 review (Copilot AI, discussion r3314987809):
CopilotClient#forceStop() chains shutdownOwnedExecutor() onto cleanupConnection()
via whenComplete(...), but cleanupConnection() is itself built on async work
scheduled on the SDK-owned executor (e.g. CompletableFuture.supplyAsync(...,
executor) in connection setup). On JDK 25+ this means the whenComplete lambda
can land on one of the owned executor's threads; awaitTermination(...) then
blocks waiting for the very thread it is running on, forcing the full
AUTOCLOSEABLE_TIMEOUT_SECONDS timeout followed by shutdownNow().
Fix: dispatch the shutdown continuation via whenCompleteAsync(...) onto a
private one-shot SHUTDOWN_DISPATCHER that spawns a fresh daemon thread named
"copilot-client-shutdown". This guarantees the awaitTermination call is never
made from inside the executor it is draining.
close() is unaffected: it calls stop().get(...) synchronously and runs
shutdownOwnedExecutor() in its finally block on the caller's thread.
* fix(java): short-circuit shutdownOwnedExecutor() when already shut down
Addresses PR #1478 review (Copilot AI, discussion r3314987870):
close() and forceStop() can each invoke shutdownOwnedExecutor() (e.g. user
calls forceStop() and then close() in try-with-resources). A second call
would redundantly invoke shutdown() and awaitTermination() on an already-
terminated ExecutorService. While the JDK handles this gracefully
(awaitTermination returns immediately after a prior shutdownNow), the redundant
call obscures diagnostics.
Short-circuit at the top of shutdownOwnedExecutor() when isShutdown() is
already true and log at FINE so the second invocation is visible without
spamming normal output.
* spotless:apply
* ci(java): build/publish on JDK 25 to include MR-JAR overlay; matrix SDK tests across JDK 17 and 25
The java25-multi-release Maven profile is activated only on JDK 25+
(<jdk>[25,)</jdk>). Without it, the build skips the compile-java25 execution,
the packaged JAR has no META-INF/versions/25/InternalExecutorProvider.class,
and the manifest lacks Multi-Release: true. The InternalExecutorProvider
JDK 25 overlay (Executors.newVirtualThreadPerTaskExecutor()) is then
effectively dead in CI and in published Maven Central artifacts -- consumers
on JDK 25+ silently fall back to ForkJoinPool.commonPool().
Changes:
- java-publish-snapshot.yml: set up JDK 25 (was 17). The pom keeps
<maven.compiler.release>17</maven.compiler.release> so baseline bytecode
remains JDK 17 compatible; --release 17 is supported by the JDK 25
compiler.
- java-publish-maven.yml: same JDK bump for release:perform.
- java-sdk-tests.yml: matrix on java-version: [17, 25]. JDK 25 entry
exercises the MR-JAR overlay end-to-end via InternalExecutorProviderIT
(asserts feature >= 25 => canBeShutdown=true, virtual=true) and runs the
new verify-java25-overlay antrun structural guard. Side-effects (site
artifact upload, JaCoCo badge generation, badge-update PR) remain gated to
the JDK 17 entry so the badge source-of-truth stays a single baseline.
Failure artifact name suffixed with -jdk${matrix.java-version} to avoid
collisions.
Branch protection note: the job's check name changes from "Java SDK Tests"
to "Java SDK Tests (JDK 17)" + "Java SDK Tests (JDK 25)". Update branch
protection rules accordingly after merge if required-checks reference the
old name.
* On branch edburns/review-brunoborges-pr-1478 Test invocation changes.
modified: .github/workflows/java-sdk-tests.yml
- Have `test-jdk17` be a parameter, set to true by default.
- Do not use matrix. It is vital that the tests verify that the MR-JAR facility works when compiled using JDK 25 with `maven.compiler.release` set to 17. To that end, this workflow does not re-compile the jar or the tests, but uses a the JDK 17 just to run the tests.
- Use a separate banner for each variant.
- Use separate summary for 17 and 25 tests.
modified: java/pom.xml
- Print banners for inspection during tests.
- Use Maven enforcer plugin to require using 25 when compiling.
modified: java/README.md
- Add content for running the tests with 17.
* On branch edburns/review-brunoborges-pr-1478
modified: .github/workflows/docs-validation.yml
- Use Java 25 per @brunoborges.
modified: .github/workflows/java-sdk-tests.yml
- Update labels.
modified: .vscode/settings.json
- Additional Java settings.
modified: scripts/docs-validation/package.json
The `--lang` parsing in validate.ts uses `args.find((a) => a.startsWith("--lang="))`, which won't match `--lang typescript` (space-separated). So `targetLang` is always `undefined`, and every job validates **all** languages. This is a pre-existing bug — but it was invisible before because `mvn install` succeeded with the pre-installed JDK 17 on all runners.
However, the reason it matters **now** is: after fixing the `validate-java` job to use JDK 25, the other 4 jobs (TypeScript, Go, Python, C#) still don't have JDK 25. Since they also accidentally validate Java (due to the broken `--lang` filter), they'd continue to fail.
So you actually have two options:
**Option A:** Only fix JDK in `validate-java` AND fix `--lang` parsing so other jobs stop accidentally validating Java.
**Option B:** Only fix JDK in `validate-java` AND add JDK 25 setup to all 4 other jobs too (ugly but works without touching the script).
The `--lang` fix is the cleaner path, but it's a separate pre-existing bug, not something introduced by PR #1483. If you'd prefer to keep the changes minimal and just address the PR's breakage, I can revert the package.json change and instead add `setup-java` with JDK 25 to every job. What's your preference?
* On branch edburns/review-brunoborges-pr-1478
modified: .github/actions/java-test-report/action.yml
modified: .github/workflows/java-sdk-tests.yml
- Ensure the correct Copilot CLI is used for tests.
* On branch edburns/review-brunoborges-pr-1478
modified: .github/workflows/java-sdk-tests.yml
Now the JDK 17 run:
1. `jacoco:prepare-agent@wire-up-coverage-instrumentation` — generates the JaCoCo agent arg compatible with JDK 17 and sets `testExecutionAgentArgs`
2. `antrun:run@print-test-jdk-banner` — prints the JDK banner
3. `surefire:test` — runs pre-compiled tests with the JaCoCo agent attached
4. `jacoco:report@build-coverage-report-from-tests` — generates the HTML/XML/CSV reports
The `-DtestExecutionAgentArgs=` override is removed so JaCoCo's prepared value flows through to Surefire's `<argLine>`. The test report action already reads from the default jacoco.xml path, so the coverage section will appear in both reports automatically.
* On branch edburns/review-brunoborges-pr-1478 Address copilot review comments.
modified: .github/actions/java-test-report/action.yml
- Add failsafe tests to the report.
modified: .github/workflows/java-sdk-tests.yml
- Ensure failsafe tests are invoked in the 17 case.
modified: java/README.md
- Fix spelling error.
- Fix artifact version error.
- Fix 17 invocation.
* Ensure the Jar is produced
---------
Co-authored-by: Bruno Borges <brborges@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>1 parent dbfda40 commit f86c7e0
16 files changed
Lines changed: 687 additions & 74 deletions
File tree
- .github
- actions/java-test-report
- workflows
- .vscode
- java
- src
- main
- java25/com/github/copilot
- java/com/github/copilot
- rpc
- test/java/com/github/copilot
- scripts/docs-validation
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
4 | 4 | | |
5 | 5 | | |
6 | 6 | | |
7 | | - | |
| 7 | + | |
8 | 8 | | |
9 | 9 | | |
10 | 10 | | |
| |||
17 | 17 | | |
18 | 18 | | |
19 | 19 | | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
20 | 24 | | |
21 | 25 | | |
22 | 26 | | |
23 | 27 | | |
24 | 28 | | |
25 | 29 | | |
26 | | - | |
| 30 | + | |
27 | 31 | | |
28 | 32 | | |
29 | 33 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
143 | 143 | | |
144 | 144 | | |
145 | 145 | | |
146 | | - | |
| 146 | + | |
147 | 147 | | |
148 | 148 | | |
149 | 149 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
54 | 54 | | |
55 | 55 | | |
56 | 56 | | |
57 | | - | |
| 57 | + | |
58 | 58 | | |
59 | 59 | | |
60 | | - | |
| 60 | + | |
61 | 61 | | |
62 | 62 | | |
63 | 63 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
30 | 30 | | |
31 | 31 | | |
32 | 32 | | |
33 | | - | |
| 33 | + | |
34 | 34 | | |
35 | 35 | | |
36 | | - | |
| 36 | + | |
37 | 37 | | |
38 | 38 | | |
39 | 39 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
37 | 37 | | |
38 | 38 | | |
39 | 39 | | |
40 | | - | |
| 40 | + | |
41 | 41 | | |
42 | 42 | | |
43 | 43 | | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
44 | 48 | | |
45 | 49 | | |
46 | 50 | | |
| |||
52 | 56 | | |
53 | 57 | | |
54 | 58 | | |
55 | | - | |
| 59 | + | |
56 | 60 | | |
57 | 61 | | |
58 | 62 | | |
59 | 63 | | |
60 | 64 | | |
61 | 65 | | |
62 | 66 | | |
63 | | - | |
64 | | - | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
65 | 73 | | |
66 | | - | |
67 | | - | |
68 | | - | |
| 74 | + | |
| 75 | + | |
69 | 76 | | |
70 | 77 | | |
| 78 | + | |
71 | 79 | | |
72 | 80 | | |
73 | 81 | | |
| |||
76 | 84 | | |
77 | 85 | | |
78 | 86 | | |
79 | | - | |
80 | | - | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
81 | 92 | | |
82 | | - | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
83 | 102 | | |
84 | 103 | | |
85 | | - | |
86 | | - | |
87 | | - | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
88 | 108 | | |
89 | 109 | | |
90 | | - | |
| 110 | + | |
91 | 111 | | |
92 | 112 | | |
93 | 113 | | |
| |||
98 | 118 | | |
99 | 119 | | |
100 | 120 | | |
101 | | - | |
| 121 | + | |
102 | 122 | | |
103 | 123 | | |
104 | 124 | | |
105 | 125 | | |
106 | | - | |
| 126 | + | |
107 | 127 | | |
108 | 128 | | |
109 | 129 | | |
| |||
116 | 136 | | |
117 | 137 | | |
118 | 138 | | |
| 139 | + | |
| 140 | + | |
119 | 141 | | |
120 | 142 | | |
121 | 143 | | |
122 | 144 | | |
123 | 145 | | |
124 | | - | |
| 146 | + | |
125 | 147 | | |
126 | 148 | | |
127 | 149 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
25 | 25 | | |
26 | 26 | | |
27 | 27 | | |
28 | | - | |
| 28 | + | |
| 29 | + | |
29 | 30 | | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
19 | 19 | | |
20 | 20 | | |
21 | 21 | | |
22 | | - | |
| 22 | + | |
23 | 23 | | |
24 | | - | |
25 | | - | |
| 24 | + | |
| 25 | + | |
26 | 26 | | |
27 | 27 | | |
28 | 28 | | |
29 | 29 | | |
30 | 30 | | |
31 | 31 | | |
32 | 32 | | |
33 | | - | |
| 33 | + | |
34 | 34 | | |
35 | 35 | | |
36 | 36 | | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
37 | 43 | | |
38 | 44 | | |
39 | 45 | | |
| |||
50 | 56 | | |
51 | 57 | | |
52 | 58 | | |
53 | | - | |
| 59 | + | |
54 | 60 | | |
55 | 61 | | |
56 | 62 | | |
57 | 63 | | |
58 | 64 | | |
59 | 65 | | |
60 | | - | |
| 66 | + | |
61 | 67 | | |
62 | 68 | | |
63 | 69 | | |
| |||
66 | 72 | | |
67 | 73 | | |
68 | 74 | | |
69 | | - | |
70 | 75 | | |
71 | 76 | | |
72 | 77 | | |
73 | 78 | | |
74 | | - | |
75 | | - | |
76 | 79 | | |
77 | 80 | | |
78 | 81 | | |
79 | 82 | | |
80 | 83 | | |
81 | | - | |
82 | | - | |
83 | | - | |
84 | | - | |
85 | | - | |
| 84 | + | |
86 | 85 | | |
87 | 86 | | |
88 | 87 | | |
| |||
160 | 159 | | |
161 | 160 | | |
162 | 161 | | |
| 162 | + | |
| 163 | + | |
163 | 164 | | |
164 | 165 | | |
165 | 166 | | |
| |||
168 | 169 | | |
169 | 170 | | |
170 | 171 | | |
171 | | - | |
| 172 | + | |
172 | 173 | | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
173 | 178 | | |
174 | 179 | | |
175 | 180 | | |
| |||
190 | 195 | | |
191 | 196 | | |
192 | 197 | | |
193 | | - | |
194 | | - | |
195 | | - | |
| |||
0 commit comments