Skip to content

perf(ci): let the build cache reuse core-web across PR and trunk (#36947) - #36960

Merged
wezell merged 11 commits into
mainfrom
issue-36947-core-web-cacheable
Aug 8, 2026
Merged

perf(ci): let the build cache reuse core-web across PR and trunk (#36947)#36960
wezell merged 11 commits into
mainfrom
issue-36947-core-web-cacheable

Conversation

@wezell

@wezell wezell commented Aug 7, 2026

Copy link
Copy Markdown
Member

What

Makes dotcms-core-web reusable by a content-addressed build cache across PR and trunk builds. Prerequisite for #36947; the S3 cache PR is stacked on this one.

Why

core-web/pom.xml declared a profile is_pr, auto-activated by -Dgithub.event.name=pull_request, which cicd_comp_build-phase.yml passed on every build. Three of its four properties were byte-identical to the defaults directly above it:

Property Default (line 26-28) is_pr
git.origin.branch origin/main origin/main
nx.affected.options --base=${git.origin.branch} --head=HEAD same
pretty.quick.options --branch=${git.origin.branch} same
skip.validate true false ← the only live effect

So the profile's entire purpose was: run eslint + prettier inside the artifact build, on PRs and nowhere else.

That makes the PR effective pom differ from trunk's, and the Maven build cache hashes the effective pom — so the module can never be reused across the two. Both executions sit at generate-resources; the expensive nx run-many -t build sits at compile and doesn't depend on them. We were paying ~2.85m of rebuild to protect a check the build doesn't consume.

Worth noting: inputs.validate defaults to false and no caller overrides it, so -Pvalidate was never actually passed by PR, merge-queue, trunk or nightly. is_pr was the sole source of skip.validate=false.

Verification

Measured, not argued — cache key for dotcms-core-web:

Build shape Key
-Dgithub.event.name=pull_request fd879a30…
-Dgithub.event.name=push fd879a30…
no flag fd879a30…
-Dskip.validate=false 2b0c0b9f… ← isolates the divergence

Reproduce with ./mvnw -pl :dotcms-core-web validate -Dmaven.build.cache.enabled=true and grep the 64-hex checksum.

A latent bug this surfaced

format-test ran nx format:check with no --base, so it fell back to nx.json's "defaultBase": "main" — a local branch that only exists because a job passed require-main: true to prepare-runner, which then runs git fetch origin main:main. cicd_comp_test-phase.yml never passes it, so relocating the check would have broken every frontend PR.

Fixed by making the base explicit (origin/main, always present after a fetch-depth: 0 checkout) rather than plumbing require-main through the test phase. Confirmed: --base=origin/main → exit 0 clean; --base=does-not-exist-ref → exit 1. Invisible locally because dev machines have a local main.

Trade-off

Lint/prettier failures now surface in the Frontend Unit Tests job instead of the earlier Initial Artifact Build. That job is filter-gated on frontend, which is fine because nx affected --base=origin/main already made these no-ops on backend-only PRs.

If the later feedback is unwelcome, the alternative is a dedicated mvnw generate-resources -pl :dotcms-core-web -Pvalidate job — one extra job, still off the critical path. Happy to switch.

🤖 Generated with Claude Code

https://claude.ai/code/session_014a2iJy9JXRBSVdKBbmoZ2S

This PR fixes: #36947

This PR fixes: #36947

)

core-web's `is_pr` profile was auto-activated by -Dgithub.event.name=pull_request,
which the artifact build passed. Three of its four properties (git.origin.branch,
nx.affected.options, pretty.quick.options) were byte-identical to the defaults
above it, so its only live effect was skip.validate=false: run eslint + prettier
inside the artifact build, on PRs and nowhere else.

That made the PR effective pom differ from trunk's, so a content-addressed build
cache can never reuse this module across the two -- for checks that gate nothing
the build consumes. Both executions sit at generate-resources; the expensive
`nx run-many -t build` is at compile and does not depend on them.

Lint and format now run under -Pvalidate in the Frontend Unit Tests job, which
already builds the whole workspace, so they cost almost nothing there and leave
the serial prefix that gates every test job.

Verified: after this change the PR-shaped, trunk-shaped and bare builds all hash
dotcms-core-web to fd879a30...; adding -Dskip.validate=false yields 2b0c0b9f...,
confirming that property alone was the divergence.

Also makes format-test's base explicit. It ran `nx format:check` with no --base,
falling back to nx.json defaultBase "main" -- a LOCAL branch that only exists
because a job passed require-main=true to prepare-runner. The test phase never
does, so relocating the check would have broken it. origin/main is always present
after a fetch-depth:0 checkout.

Trade-off: lint/format failures now surface in the Frontend Unit Tests job rather
than the earlier build job.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014a2iJy9JXRBSVdKBbmoZ2S
@claude

claude Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Claude finished @wezell's task in 6m 26s —— View job


Code Review

  • Read the diff against origin/main
  • Review CI/workflow changes for correctness
  • Review core-web/pom.xml and cache config changes
  • Post findings

I traced the is_pr removal, the skip.validate default chain, the --base addition to format:check, the S3 cache wiring, and the provenance/kill-switch logic. The core reasoning in the PR body checks out against the code.

New Issues

  • 🟡 Medium: .github/workflows/cicd_comp_build-phase.yml:196 — the provenance recorder relies on find "$root" -mindepth 5 -maxdepth 5 -type d -name local matching the build-cache layout. Assumption: the local cache dir sits exactly 5 levels under ~/.m2/build-cache with a leaf named local. If the maven-build-cache-extension 1.2.0 layout differs (or changes on upgrade), the loop silently matches nothing and count=0 — you'd get "Recorded provenance for 0 cache entries" with no failure, so the supply-chain audit trail is quietly empty. It's continue-on-error: true and only runs on trusted write refs, so blast radius is limited to lost provenance, not a broken build. What to verify: confirm the depth against a real trunk/merge-queue run's ~/.m2/build-cache tree and consider logging a warning when count=0 but modules were built.

  • 🟡 Medium: .github/workflows/cicd_comp_build-phase.yml:137save is gated on github.event_name == 'push' && github.ref == 'refs/heads/main', but cicd_3-trunk.yml can also run via workflow_dispatch. On a manual trunk run event_name is workflow_dispatch, so save=false and that build won't populate the cache. Almost certainly intentional (only automatic trunk pushes populate), but worth a one-line confirmation since the trunk workflow passes the read-write key regardless. Not a correctness bug.

Everything else looks sound:

  • The is_pr profile deletion is safe — three of its four properties were byte-identical to the defaults at core-web/pom.xml:25-28, and -Dgithub.event.name is referenced nowhere else in the repo (only the removed profile activated on it). Verified via grep.
  • skip.validate correctly defaults to true, so lint-test/format-test are now inert unless -Pvalidate flips it to false — which test-matrix.yml now passes for the Frontend Unit Tests suite. Chain is consistent.
  • The --base=${git.origin.branch} fix on format:check is correct: it resolves to origin/main (always present after fetch-depth: 0), avoiding the nx.json defaultBase: main local-branch trap that would otherwise only work on jobs passing require-main: true.
  • FINAL_ARGS interpolation of ${BUILD_CACHE_ARGS:-} is null-safe, and the dedup awk pass is preserved.
  • Security posture is solid: RO key on PR (untrusted), RW on merge-queue/trunk, the runtime write-probe that proves the RO key can't write, digest-pinned sigv4 proxy and actions/cache, and the 403→404 HEAD handling for the extension's miss semantics.

Re: sfreudenthaler's note on the format profile block in core-web/pom.xml — that block (<id>format</id>, the auto-format/auto-lint executions) is separate from what this PR removed and is invoked by developers locally; the trigger comment didn't ask me to touch it, so I've left it. It can be pruned in a follow-up if it's confirmed unused.

No blocking issues. The two Medium items are non-blocking.
· issue-36947-core-web-cacheable

@sfreudenthaler sfreudenthaler left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If the later feedback is unwelcome, the alternative is a dedicated mvnw generate-resources -pl :dotcms-core-web -Pvalidate job — one extra job, still off the critical path. Happy to switch.

Lets see it in action. Shouldn't be an issue, I think more folks using claude and other bots to parse the failure logs and the bots dont need pretty.

Approved with small non-blocking suggestion

Comment thread core-web/pom.xml Outdated
Co-authored-by: Steve Freudenthaler <31257998+sfreudenthaler@users.noreply.github.com>
@wezell
wezell enabled auto-merge August 7, 2026 20:47
wezell and others added 2 commits August 7, 2026 16:56
…#36961)

Closes part of #36947. **Stacked on #36960** — review that one first;
this PR's diff is against it.

## What

Gives the Apache Maven Build Cache Extension somewhere to keep its
results: a shared S3 bucket on OVH object storage.

The extension was **already installed and enabled**
(`.mvn/extensions.xml`, v1.2.0) — but local-only, so on a fresh CI
runner it cached nothing. The only missing piece was shared storage.

Scoped to the **Initial Artifact Build**, the serial prefix that gates
every test job. Test phases are deliberately *not* cached: a memoised
green run would mean "we did not run", and this suite has measured
flakes plus a hang that has burned a 122m job timeout.

## Transport

The extension speaks HTTP `PUT`/`GET`/`HEAD`; S3 needs SigV4. Rather
than add a Maven S3 wagon — the available ones aren't maintained
(`seahen` 1.3.3 is 2021, `gkatzioura` 2.3 is 2019, both AWS SDK v1) —
[`aws-sigv4-proxy`](https://github.com/awslabs/aws-sigv4-proxy) runs as
a container and signs on the way out, and Maven talks to `127.0.0.1`.
**No new Maven dependency.**

## Security

A build cache untrusted code can write is a supply-chain vector: a
poisoned entry is replayed as a build output on a trusted ref. This is
[CVE-2025-36852](https://www.cve.org/CVERecord?id=CVE-2025-36852)
("CREEP"), which killed Nx's `@nx/s3-cache` and its siblings.

**The control is the credential, not the client** — a job holding a
writable key can bypass Maven entirely with one `aws s3 cp`, so
`remote.save.enabled` is defence in depth, not the boundary.

| Ref | Key | Writes |
|---|---|---|
| PR | `..._ACCESS_KEY_RO` (GetObject only) | no |
| merge queue / trunk | `..._ACCESS_KEY` | yes |
| fork PR | none — builds uncached | no |

Plus:
- `remote.save.final=true` — an existing entry is never overwritten.
- The action **asserts** its read-only key is read-only (one `PUT`,
expects `403`) instead of assuming it. A writable "read-only" key looks
identical to a correct setup until abused.
- Writing builds record a `provenance.json` beside each entry (ref, sha,
run id, actor), first-writer-wins — nothing else in a bucket says which
ref produced a hash.

## The subtle one: `alwaysRunPlugins`

Load-bearing, not tuning. On a cache hit the extension skips cached
plugin executions **including `install:install`** — measured: 1 jar in
`~/.m2/repository` after a cold build, **0 after a hit**. This job
exists to publish that repository as the `maven-repo` artifact ~25 test
jobs consume. Same story for `docker-maven-plugin:build`, which writes
the `docker-build.tar` the next step uploads.

Both would have looked perfectly green on the cold populate run and
broken everything on the first *warm* one.

## Verification

Against MinIO before any of this was wired:

| Check | Result |
|---|---|
| `PUT` / `GET` / `HEAD` through the proxy | `200` / `200` / `200` |
| Missing key | `404` (a `403` reads as a hard error, not a miss) |
| 10 MB body round-trip | byte-identical |
| Build with an **empty** local cache | `Found cached build, restoring …
by checksum` |
| Remote unreachable | build exits `0`, logs an error, rebuilds |

And separately, because `cicd_comp_build-phase.yml` fails a PR on a
dirty tree while `openapi.yaml` is a tracked file generated at compile:
built `:dotcms-core --am` twice — 12 modules restored including
`dotcms-core`, `openapi.yaml` md5 identical, `git status` unchanged.

## Releases are not affected

Release, LTS, nightly, manual-deploy and CLI-release workflows don't
pass the secrets, and **no workflow in this repo uses `secrets:
inherit`** (verified). The action sees empty credentials and exports an
empty `BUILD_CACHE_ARGS`; those pipelines build from scratch exactly as
today. A release is the build where "we didn't actually compile this" is
least acceptable and the saving is worth least.

## Turning it off

| Scope | How |
|---|---|
| One PR | label `CI: no build cache` |
| Everything, now | repo/org variable `BUILD_CACHE_DISABLED=true` |
| Local build | `-Dmaven.build.cache.enabled=false` |
| Force rebuild, still publish | `-Dmaven.build.cache.skipCache=true` |

## Expected effect, honestly

Ceiling is the ~7.0m of Maven time inside a 14.2m build job, against a
74–103m PR wall clock. Real but modest — the twin-tail shard rebalance
(#36943) is still the bigger lever. `dotcms-core-web` only participates
once #36960 lands.

**The first merge-queue run is the canary**: it's what proves the OVH
SigV4 handshake and the region derived from the endpoint host. If either
is wrong the cache disables itself with a warning rather than failing
the build.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_014a2iJy9JXRBSVdKBbmoZ2S

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every job runs ./mvnw, and on a cold runner mvnw downloads the ~40MB Maven
distribution from Maven Central. Nothing cached it: maven-job caches
~/.m2/repository, installs, the pnpm store and Sonar, but not ~/.m2/wrapper. So
a single run fetched the same zip roughly 26 times.

Maven Central eventually answers 429. That is what killed this PR's own run
31218010178 -- Postman Container died after 455ms, before any dotCMS code ran:

  IOException: Server returned HTTP response code: 429 for URL:
  .../apache-maven/3.9.2/apache-maven-3.9.2-bin.zip
      at org.apache.maven.wrapper.MavenWrapperMain.main

and fail-fast then cancelled 24 other jobs, so one transient upstream rate limit
presents as a wholesale pipeline failure with no attributable cause.

The key hashes .mvn/wrapper/maven-wrapper.properties, so it changes only when the
Maven version does and is otherwise a permanent hit.

Uses the combined actions/cache rather than the restore/save split the other
caches use: this content is an immutable versioned download, so there is no risk
of persisting a polluted cache, and letting any job save means the first one to
run on a cold key repairs it for the rest of the matrix.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014a2iJy9JXRBSVdKBbmoZ2S
Comment thread .github/actions/core-cicd/maven-job/action.yml Outdated
wezell and others added 2 commits August 7, 2026 17:54
Semgrep flagged the new Cache Maven Wrapper Distribution step as a blocking
finding (github-actions-mutable-action-tag): `v4` is a mutable tag the action
owner can silently repoint, which is how the trivy-action and
kics-github-action compromises worked. A cache action runs in every job in the
pipeline and sees the build output, so it is a meaningful position to hold.

Pinned to 0057852bfaa89a56745cba8c7296529d2fc39830, the commit v4 currently
resolves to (also tagged v4.3.0), with the version in a trailing comment so the
next bump is a deliberate edit.

Only the new step is pinned. The six pre-existing actions/cache/restore@v4 and
actions/cache/save@v4 uses in this file predate the scan and are unflagged;
pinning them is worth doing but belongs in its own change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014a2iJy9JXRBSVdKBbmoZ2S
The build-cache action ran public.ecr.aws/aws-observability/aws-sigv4-proxy:latest,
a mutable tag on a third-party image. That container is handed
AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY and proxies every cache read and write,
so whoever controls that tag is one push away from a credential-stealing
position inside the build -- a strictly worse exposure than the actions/cache
tag Semgrep flagged, and unflagged only because Semgrep's rule covers `uses:`
and not `docker run`.

Pinned to sha256:6cd48ff272e30b6c3c01c02eac42dc3376bc3efa6ed3377bccd484e1c1b389df,
resolved three ways (docker buildx imagetools, the local RepoDigests of the
pulled image, and the digest recorded during the MinIO smoke test) so the digest
being pinned is the exact artifact validated end-to-end against the OVH bucket.

It is an OCI image index covering linux/amd64 and linux/arm64, so pinning the
index rather than a per-platform manifest keeps CI (amd64) and local development
(arm64) on the same reference. Verified it still pulls after removing the local
copy.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014a2iJy9JXRBSVdKBbmoZ2S
…36947)

fail-fast is right by default -- one broken suite should not burn 25 runners --
but it destroys evidence exactly when you need it. A single failure cancels the
other ~24 jobs, and in the checks UI a cancelled job is indistinguishable from a
failed one, so a lone flake presents as a wholesale pipeline failure. This PR's
own run 31218010178 showed 24 red jobs from one transient Maven Central 429, and
run 31222593504 showed 8 more from one leaked persona in PersonaAPITest.

Labelling a PR "CI : No Fail Fast" now runs every suite to completion, so you can
tell "this change broke one suite" from "this change broke twenty" in a single
run instead of fixing, re-running, and discovering the next casualty.

Costs runner time, so it stays opt-in per PR rather than becoming the default.
merge_group events carry no pull_request, so contains() is false there and the
queue keeps bailing early, which is what you want when the tree is at stake.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014a2iJy9JXRBSVdKBbmoZ2S
@wezell wezell added the CI : No Fail Fast Run every test suite to completion instead of cancelling the matrix on first failure label Aug 8, 2026
@wezell
wezell added this pull request to the merge queue Aug 8, 2026
Merged via the queue into main with commit 966e9d5 Aug 8, 2026
81 of 107 checks passed
@wezell
wezell deleted the issue-36947-core-web-cacheable branch August 8, 2026 02:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : CI/CD PR changes GitHub Actions/workflows Area : Frontend PR changes Angular/TypeScript frontend code CI : No Fail Fast Run every test suite to completion instead of cancelling the matrix on first failure

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

CI: content-addressed build caching across nx, Maven and Docker

2 participants