Fix JDK resolution cache platform identity - #1210
Conversation
Include the effective Linux libc platform in JDK resolution cache keys so Alpine/musl and glibc runners cannot restore each other's release metadata. Bump the cache namespace and share Alpine detection with affected distributors.\n\nCo-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>\nCopilot-Session: 4f95577c-567c-47a8-92f2-b4dced527866
Regenerate setup and cleanup distributions for the platform-aware JDK resolution cache.\n\nCo-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>\nCopilot-Session: 4f95577c-567c-47a8-92f2-b4dced527866
There was a problem hiding this comment.
Pull request overview
This PR fixes a correctness issue in the JDK resolution cache by adding a canonical runtime platform identity to cache requests, ensuring Linux glibc and musl (Alpine) runners don’t share incompatible cached resolution metadata.
Changes:
- Add
platformto the JDK resolution cache request identity and bump the cache key version to v2 to avoid restoring ambiguous v1 entries. - Introduce shared Alpine detection and a canonical
getJavaPlatformIdentity()(linux-glibcvslinux-musl, plus normalized macOS/Windows). - Add regression tests covering libc-specific key separation and platform normalization; update generated
dist/bundles accordingly.
Show a summary per file
| File | Description |
|---|---|
| src/jdk-resolution-cache.ts | Adds platform to the resolution identity and bumps key version to v2. |
| src/distributions/base-installer.ts | Includes canonical platform identity in every resolution-cache request. |
| src/distributions/platform-types.ts | Adds isAlpineLinux() and getJavaPlatformIdentity() helpers for consistent runtime platform detection. |
| src/distributions/temurin/installer.ts | Reuses shared Alpine detection to match artifact selection behavior. |
| src/distributions/sapmachine/installer.ts | Reuses shared Alpine detection to match artifact selection behavior. |
| tests/jdk-resolution-cache.test.ts | Updates key version expectations and adds libc separation regression coverage. |
| tests/java-platform-contract.test.ts | Adds tests for platform identity normalization and linux glibc vs musl mapping. |
| tests/distributors/base-installer.test.ts | Updates expected resolution-cache request to include canonical platform identity. |
| dist/setup/index.js | Regenerates bundle exports to include new platform helper functions. |
| dist/setup/557.index.js | Regenerates SapMachine bundle to use shared Alpine detection helper. |
| dist/setup/463.index.js | Regenerates Temurin bundle to use shared Alpine detection helper. |
| dist/setup/348.index.js | Regenerates setup bundle resolution-cache logic for v2 + platform identity. |
| dist/setup/242.index.js | Regenerates setup bundle base-installer logic to include platform identity in requests. |
| dist/cleanup/348.index.js | Regenerates cleanup bundle resolution-cache logic for v2 + platform identity. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/14 changed files
- Comments generated: 0
- Review effort level: Lite
getJavaPlatformIdentity's `?? platform` fallback and the alias path for platforms other than linux/darwin/win32 had no coverage, and isAlpineLinux had no direct test at all. Verified by mutation: replacing the fallback with a constant, and dropping the `platform === 'linux'` short-circuit from isAlpineLinux, both left the existing suite fully green. The added cases fail on each. The short-circuit case matters beyond coverage bookkeeping: it is what keeps the /etc/alpine-release probe from running on non-Linux runners, so a stray file can never make Windows or macOS resolve as musl. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 74248bb0-72af-41d8-b85d-b0f5836e68db
|
I reviewed this against a fix I'd written independently for #1209 and ended up preferring your approach, so rather than open a competing PR I've pushed one test-only commit here (3b8f519). Happy to drop it if you'd rather keep the branch yours. What the commit addsTwo gaps in
Both verified by mutation rather than by eyeballing coverage:
The short-circuit one is worth pinning beyond coverage bookkeeping: it's what keeps the Full suite is green (41 suites / 1322 tests), format-check and lint clean. Test-only, so no On the designMy version used a The hook only protects distributions that remember to override it, so its failure mode is a silent cross-libc collision — the exact bug in #1209. Yours is correct by default for every distribution, including ones that don't participate. I also couldn't find a single distribution that would need to override it today: identity is a function of (platform, arch, packageType, versionSpec, stable), and I checked the one candidate I suspected — Temurin's Your injectable-parameter signature is also strictly better for testing than what I had, which mutated Two optional notesThe Minor: Out of scope, for the recordDragonwell and Corretto publish |
Merging main brought in actions#1219, which added getFloatingResolutionRequest as a second construction site for JdkResolutionRequest. It predates the required `platform` field, so the merged tree did not compile. The floating request already carries `source`, which pins the artifact bytes, so this changes no lookup behaviour on its own -- it keeps the two request builders consistent and the tree building. Also refreshes dist/, which the textual merge left stale. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 74248bb0-72af-41d8-b85d-b0f5836e68db
brunoborges
left a comment
There was a problem hiding this comment.
Approving as maintainer. Disclosure: I pushed two commits to this branch — a test-coverage commit for the platform-identity helper, and a fix carrying platform into the floating resolution request that #1219 introduced (the textual merge with main left the tree not compiling, and dist/ stale).
Design and the core fix are jdubois'. Verified locally: 41 suites / 1365 tests, format-check, lint, and a clean dist/ rebuild. All 84 CI checks green after re-running one unrelated Windows GPG-cleanup flake.
|
Status: this is merge-ready — all 84 checks green (one unrelated Windows GPG-cleanup flake re-run), branch up to date with main, and I've approved it. It can't actually merge yet, though: the So it needs an approving review from someone who didn't make that push — @jdubois or another maintainer. I'd rather leave it for a second pair of eyes than bypass the rule, especially since my commit touches |
PR actions#1210 landed the shared `isAlpineLinux()` helper, so this branch drops its own identical copy and imports main's. `platform-types.ts` and `java-platform-contract.test.ts` are now untouched by this branch; the duplicated `isAlpineLinux` test block was byte-identical to main's and was removed. Rebuilt dist/ to resolve the bundle conflict. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 74248bb0-72af-41d8-b85d-b0f5836e68db
Description
The JDK resolution cache currently treats all Linux runners with the same declared inputs as equivalent. Temurin and SapMachine resolve different artifacts on Alpine, however, so an Alpine/musl runner and a glibc runner can restore the same cached release metadata and install an incompatible JDK.
This change:
linux-glibcfromlinux-musl;Closes #1209.
Testing
check-distrebuild exactly matched the committed generated bundles.