Build/test tools: Download and verify the Gutenberg build once per run - #12701
Build/test tools: Download and verify the Gutenberg build once per run#12701lancewillett wants to merge 4 commits into
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
There was a problem hiding this comment.
Pull request overview
Updates the Gutenberg build download flow so workflows fetch/verify the archive once per workflow run, then share it across all matrix jobs via an artifact—reducing GHCR traffic and making verification failures centralized and deterministic.
Changes:
- Add a reusable workflow to download/verify the Gutenberg build once and upload it as a run artifact (plus expose the verified Gutenberg SHA as an output).
- Update PHPUnit and coverage workflows to consume the prepared artifact and pass the verified SHA into build steps to avoid re-resolving mutable tags per job.
- Harden the Gutenberg downloader to verify SHA-256/size and to replace the existing
gutenberg/directory only after successful verification and extraction.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/gutenberg/utils.js | Allows workflows to supply a pre-resolved expected Gutenberg SHA via env to avoid per-job mutable-tag resolution. |
| tools/gutenberg/download.js | Adds verified download (SHA-256 + size), retries, and atomic-ish directory replacement after successful extract. |
| .github/workflows/test-coverage.yml | Adds shared Gutenberg prepare job and wires coverage jobs to use the shared artifact + SHA output. |
| .github/workflows/reusable-prepare-gutenberg.yml | New reusable workflow to download/verify Gutenberg once and upload as gutenberg-build artifact. |
| .github/workflows/reusable-phpunit-tests-v3.yml | Downloads the prepared Gutenberg artifact when provided and passes GUTENBERG_EXPECTED_SHA to avoid per-job resolution. |
| .github/workflows/phpunit-tests.yml | Adds shared Gutenberg prepare job and wires all PHPUnit matrices to consume the shared artifact + SHA output. |
Comments suppressed due to low confidence (1)
tools/gutenberg/download.js:179
- The manifest-size verification also relies on a truthy check (
if ( expectedSize && … )). It’s safer and clearer to verify only when the value is a finite positive number.
if ( expectedSize && downloadedBytes !== expectedSize ) {
throw new Error(
`Downloaded ${ formatBytes( downloadedBytes ) }, but manifest size was ${ formatBytes( expectedSize ) }`
);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ( contentLength && downloadedBytes !== contentLength ) { | ||
| throw new Error( | ||
| `Downloaded ${ formatBytes( downloadedBytes ) }, but Content-Length was ${ formatBytes( contentLength ) }` | ||
| ); | ||
| } |
There was a problem hiding this comment.
contentLength && and Number.isFinite(contentLength) && contentLength > 0 are equivalent here (both 0 and NaN are falsy), so there's no behavior difference. Rather than align them, 8b00894 removes the Content-Length check entirely: the manifest size check plus the SHA-256 verification already cover truncation and corruption, so it was redundant. Thanks for the close read.
| gutenberg_sha="$(tr -d '\n' < gutenberg/.gutenberg-hash)" | ||
| if [[ ! "$gutenberg_sha" =~ ^[a-f0-9]{40}$ ]]; then | ||
| echo "Expected a 40-character Gutenberg SHA, received: $gutenberg_sha" >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
8b00894 aligns it to case-insensitive to match the JS checks, which use /i. In practice this gate only sees git-generated (lowercase) SHAs from the verified .gutenberg-hash, so it never fired, but the inconsistency was real.
| const workflowSha = process.env.GUTENBERG_EXPECTED_SHA; | ||
| if ( workflowSha ) { | ||
| if ( ! SHA_PATTERN.test( workflowSha ) ) { | ||
| throw new Error( | ||
| `GUTENBERG_EXPECTED_SHA must be a 40-character Git SHA, received "${ workflowSha }"` | ||
| ); | ||
| } | ||
|
|
||
| return workflowSha; | ||
| } |
There was a problem hiding this comment.
In the shipped wiring this comes from a validated Actions output (already 40-hex-checked, no trailing newline), so a stray value can't reach here, and we've kept the strict validation. Happy to add a .trim() if you'd prefer the documented manual-override path to be more forgiving; it's a cheap follow-up.
lancewillett
changed the title
Each matrix job independently streams the same ~36 MB Gutenberg archive from the GitHub Container Registry, so any one flaky stream can redden a run. Add a prepare-gutenberg reusable workflow that fetches and verifies the build once (SHA-256 and size) and uploads it as a run artifact; every PHPUnit caller consumes the shared artifact and no longer contacts the registry. The prep job runs only when a consumer runs. Test coverage is unchanged. See #65721. Props lance.willett@automattic.com
548bc3f to
144271f
Compare
| * This script downloads a pre-built Gutenberg tar.gz artifact from the GitHub | ||
| * Container Registry and extracts it into the ./gutenberg directory. Any | ||
| * existing gutenberg directory is removed before extraction. | ||
| * Container Registry and extracts it into the ./gutenberg directory. An | ||
| * existing gutenberg directory is replaced only after verification and | ||
| * extraction succeed. |
There was a problem hiding this comment.
Good catch, and it has real teeth: the directory is removed before the extracted-SHA check, so on a local npm run build a mismatch could delete a working build before verifying the replacement. For now 8b00894 corrects the header comment to state the actual behavior (removed before extraction). Making the swap genuinely atomic (extract to temp, verify, rename) is a tracked follow-up; it's the more robust fix but a larger change, so we're keeping this PR focused.
| # Performs the following steps: | ||
| # - Sets environment variables. | ||
| # - Checks out the repository. | ||
| # - Downloads the prepared Gutenberg build when provided by the calling workflow. | ||
| # - Sets up Node.js. |
There was a problem hiding this comment.
8b00894 reworded it: the inputs are required: true, so the build is always provided. Dropped the "when".
|
When things are calmer it might be worth backporting this fix to older branches as well to make CI more reliable when testing backported branches. |
- Remove the Content-Length integrity check in downloadAndVerifyBlob(). It is redundant: the manifest size check and SHA-256 verification that follow already catch truncated or corrupted downloads. - Refresh the GHCR bearer token once when a blob download hits a 401 during a retry attempt. The token can age out during the multi-minute retry window, so downloadBlobWithRetries() now re-fetches a token via fetchGhcrToken() and retries with it before falling back to the existing non-retryable handling. - Correct the script's header comment: the existing gutenberg directory is removed before extraction (after the archive is verified), not only after extraction succeeds. - Make the Gutenberg SHA regex in reusable-prepare-gutenberg.yml case-insensitive, matching the `/i` flag used by the equivalent checks in tools/gutenberg/utils.js. - Fix a step-list comment in reusable-phpunit-tests-v3.yml: the Gutenberg build input is required, not optional, so drop "when provided". Props lance.willett@automattic.com.
…nifest fetches. The token and manifest requests run before the blob download and became a single point of failure once the whole test matrix was consolidated behind one prepare-gutenberg job, so they now share the blob download's bounded retry and timeout. Props lance.willett@automattic.com.
| // Retry/timeout settings for the token and manifest requests. These run | ||
| // before the blob download and are now a single point of failure for the | ||
| // whole build, so they share the blob download's attempt count and backoff | ||
| // (see MAX_DOWNLOAD_ATTEMPTS and RETRY_DELAY_MS in download.js). |
Note on the download-once tradeoffThis PR moves the Gutenberg build download from once-per-matrix-job to once-per-run: a single Why do it. Today each job resolves the mutable tag→SHA independently. If the tag moves mid-run, different jobs can silently test against different Gutenberg builds. Resolving once and sharing the SHA plus artifact closes that skew. The cost. A failure in the single gate skips the whole matrix via How we bounded it. The download path now has bounded retry and timeout on the blob, a fresh-token refetch on a mid-retry 401, and the same retry and timeout on the token and manifest fetches that precede it. The whole single-gate path is hardened, not just the blob. Why no per-job fallback. A fallback to the old per-job download would re-introduce the very skew this fixes (a fallen-back job could grab a different build) and keep two download paths to maintain. Flagging this so it is a deliberate yes, not a nod-through. Happy to revisit if you would prefer the fallback. |
adimoldovan
left a comment
adimoldovan
left a comment
There was a problem hiding this comment.
I think this will break on branches 5.9–7.0 because the two required inputs are not passed — those branches have no copy of this file and call it as reusable-phpunit-tests-v3.yml@trunk, so they'd pick up the new interface without being able to satisfy it
Making both inputs required: false with default: '' and guarding the download step with if: inputs.gutenberg-artifact != '' should keep the old callers working.
…ble for older branches. Branches >= 5.9 call reusable-phpunit-tests-v3.yml@trunk and do not pass the new Gutenberg inputs, so requiring them would fail every PHPUnit run on those branches with a missing-input error. Make gutenberg-artifact and gutenberg-sha optional (default empty) and guard the artifact download so those callers fall back to a per-job download, preserving current behavior. Props lance.willett@automattic.com.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
tools/gutenberg/utils.js:33
- The comment says the metadata requests “share” the blob download’s retry/backoff settings, but this file hard-codes its own MAX_METADATA_ATTEMPTS/RETRY_DELAY_MS values. Since these values can drift over time, either reference a shared constant or adjust the wording to reflect that they only match the download settings.
// Retry/timeout settings for the token and manifest requests. These run
// before the blob download and are now a single point of failure for the
// whole build, so they share the blob download's attempt count and backoff
// (see MAX_DOWNLOAD_ATTEMPTS and RETRY_DELAY_MS in download.js).
| if ( ! revision || ! /^[a-f0-9]{40}$/i.test( revision ) ) { | ||
| throw new Error( | ||
| `Manifest for mutable ref "${ ref }" has no valid org.opencontainers.image.revision SHA` | ||
| ); | ||
| return { manifest: initialManifest, resolvedRef: ref }; | ||
| } |
Great catch, @adimoldovan . Confirmed and fixed in eeee175 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
.github/workflows/reusable-phpunit-tests-v3.yml:193
- This comment implies there is always a producer-resolved SHA, but
inputs.gutenberg-shais optional and may be empty (e.g. when callers omit the producer job and fall back to per-job downloading). Make the comment conditional to avoid misleading future maintainers.
env:
# The producer resolved this once. Matrix jobs must not re-resolve mutable GHCR tags.
GUTENBERG_EXPECTED_SHA: ${{ inputs.gutenberg-sha }}
| prepare-gutenberg: | ||
| name: Gutenberg | ||
| runs-on: ubuntu-24.04 | ||
| timeout-minutes: 10 |
| # Performs the following steps: | ||
| # - Sets environment variables. | ||
| # - Checks out the repository. | ||
| # - Downloads the prepared Gutenberg build provided by the calling workflow. | ||
| # - Sets up Node.js. |
| # Branches >= 5.9 call this workflow at @trunk without the producer job, so they | ||
| # pass no artifact. They skip this step and fall back to a per-job download. | ||
| - name: Download prepared Gutenberg build | ||
| if: inputs.gutenberg-artifact != '' | ||
| uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 | ||
| with: | ||
| # Without a run ID, this action reads only from the caller's current workflow run. | ||
| name: ${{ inputs.gutenberg-artifact }} | ||
| path: gutenberg | ||
| digest-mismatch: error | ||
|
|
Each matrix job independently streams the same ~36 MB Gutenberg archive from the GitHub Container Registry, so any one flaky stream can redden a run. Add a reusable prepare-gutenberg workflow that fetches and verifies the build once (SHA-256 and size) and uploads it as a run artifact; the PHPUnit callers consume the shared artifact and no longer contact the registry. The prep job runs only when a consumer runs, so runs that skip PHPUnit do not pay for it. Test coverage is unchanged. Developed in: #12701 Props adrianmoldovanwp, lucasbustamante Fixes #65721 git-svn-id: https://develop.svn.wordpress.org/trunk@62859 602fd350-edb4-49c9-b593-d223f7449a82
Each matrix job independently streams the same ~36 MB Gutenberg archive from the GitHub Container Registry, so any one flaky stream can redden a run. Add a reusable prepare-gutenberg workflow that fetches and verifies the build once (SHA-256 and size) and uploads it as a run artifact; the PHPUnit callers consume the shared artifact and no longer contact the registry. The prep job runs only when a consumer runs, so runs that skip PHPUnit do not pay for it. Test coverage is unchanged. Developed in: WordPress/wordpress-develop#12701 Props adrianmoldovanwp, lucasbustamante Fixes #65721 Built from https://develop.svn.wordpress.org/trunk@62859 git-svn-id: http://core.svn.wordpress.org/trunk@62139 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Trac ticket: https://core.trac.wordpress.org/ticket/65721
Summary
Fetch and verify the Gutenberg build once per workflow run instead of once per matrix job.
.github/workflows/reusable-prepare-gutenberg.yml, which resolves the build metadata, downloads the archive, verifies it (SHA-256 and size), and uploads it as a run artifact.phpunit-tests.yml,reusable-phpunit-tests-v3.yml, andtest-coverage.ymlconsume that shared artifact and no longer contact the registry.Test coverage is unchanged: the same PHPUnit jobs run with the same configurations.
Why
Every matrix job independently streams the same ~36 MB archive from the GitHub Container Registry. Across a full run that is roughly 6.4 GB of registry traffic for one build, and any single flaky stream can redden the whole run. Downloading once shrinks the exposure from every job to one. It also moves verification to a single place, so a bad archive fails fast and loudly instead of failing differently in each consumer.
Follow up to the Gutenberg artifact-fetching work in #64393.
This is the first of two changes on this ticket. The second caches the verified archive across runs.
Testing
actionlintpasses on all four workflow files.Related
Trac https://core.trac.wordpress.org/ticket/65721, with the cache change stacked on top (#12702). This work and the interim retry fix (#12700, Trac https://core.trac.wordpress.org/ticket/65720) both rework the downloader, so whichever lands second rebases. Suggested order: #12700 first for relief, then this, then #12702.