ci: cut pipeline wall-clock time - #148
Conversation
Closes #72. Almost all of CI is one step: "Build in docker" was 180s, 184s and 289s of a ~6m run, and the two things meant to keep it short were not doing anything. **The cargo cache never held the expensive part.** The docker builds put CARGO_HOME inside the container and bind-mounted `$WORKSPACE/.cargo/registry/*` into it, but the cache step saved `.cargo-cache` and the runner's `~/.cargo`. Those are three different directories, so the registry index and the downloaded crates were thrown away after every run - the restore step took 3s because there was nothing in it. The mounts and the cached paths now name the same `.cargo-cache` tree. `registry/src` is left out on purpose; cargo re-extracts it from `registry/cache`. **And it could never be refreshed.** The key was `<target>-cargo-<host>`, with no lockfile in it, so after the first save every later run hit that key exactly, restored a `target/` from whenever it was written, and skipped saving ("Post Cache cargo: 0s" on every run). It is now keyed on the lockfiles with a prefix `restore-keys` fallback, so a dependency bump writes a fresh entry and an unrelated change still starts from the previous build. Because the container runs as root, a chown follows the docker build, otherwise the save step cannot read what it produced. **cargo-zigbuild was compiled from source on every cross-compiled build.** That is most of the 105s that separates the musl job from the gnu job, on the job that sets the critical path. `scripts/install-cargo-zigbuild.sh` installs the upstream prebuilt binary, verified against a pinned sha256. Anything unexpected - unsupported platform, missing curl, failed download - falls back to `cargo install`, so the worst case is what happens today; only a checksum mismatch is fatal, since that means the pin and the artifact disagree. **The tests waited for platforms they never use.** test-binding downloads x86_64-unknown-linux-gnu and x86_64-pc-windows-msvc, but `needs: build` made it wait for musl and aarch64 too - the slowest jobs in the matrix. The matrix is split into `build` (what the tests consume) and `build-extra` (everything else, still built on every PR), and test-binding needs only the former. Also: `npm ci` instead of `npm install` in every job, and a concurrency group so a new push cancels the run it supersedes. The build steps were duplicated three ways between ci.yml and release.yml, which is how the cache and the mounts drifted apart to begin with. They are now one composite action, so the cache layout and the toolchain pins are defined once. Three guards in __test__/toolchain.test.ts, each verified to fail when mutated: every target test-binding tests is built by a job it needs (the split is only safe while that holds); no workflow pins cargo-zigbuild inline; and the installer keeps its executable bit, since the build commands run it as `./scripts/...`. Not done, deliberately: - No `paths-ignore`. "Test bindings on x86_64-unknown-linux-gnu - node@20" is a required check, and a workflow skipped by a path filter never reports it, so docs-only PRs would wait forever on a check that cannot arrive. - `fail-fast` stays false. These are per-platform native builds; whether a break is platform-specific is the first thing worth knowing, and cancelling the rest of the matrix hides it. Verified: actionlint clean on both workflows; the installer exercised end to end for the happy path (real download, checksum, static-pie ELF installed), plus re-run/idempotent, unsupported platform, missing curl, and a corrupted checksum that fails hard rather than falling back. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🟡 Changes recommended
There are CI-breaking issues in the updated release workflow configuration and a new guard test that is likely to fail on Windows runners.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Optimizes GitHub Actions CI/release wall-clock time by fixing ineffective Cargo caching, reducing unnecessary job dependencies, and centralizing the binding build logic (including a faster cargo-zigbuild install path).
Changes:
- Introduces a pinned, checksum-verified
scripts/install-cargo-zigbuild.shto prefer upstream prebuilt cargo-zigbuild binaries with a safe fallback tocargo install. - Refactors CI/release workflows to use a shared composite action for consistent caching, docker mounts, and artifact uploads; splits CI build matrix so tests only wait on required build jobs.
- Switches installs to
npm ciand adds CI concurrency cancellation; adds Vitest guards to prevent workflow/toolchain drift.
File summaries
| File | Description |
|---|---|
| scripts/install-cargo-zigbuild.sh | Adds a pinned installer that prefers prebuilt cargo-zigbuild with checksum verification and fallback to source builds. |
| .github/workflows/ci.yml | Adds concurrency; splits build matrix into build/build-extra; uses the shared build composite action; uses npm ci in tests. |
| .github/workflows/release.yml | Switches release builds to the shared composite action; uses installer script for cross-compiles; uses npm ci for packaging/publish steps. |
| .github/actions/build-binding/action.yml | New composite action consolidating node/rust setup, cargo caching, docker builds, ownership fixups, and artifact upload. |
| test/toolchain.test.ts | Adds workflow/toolchain guard tests (job graph correctness, zigbuild pin centralization, installer executability). |
Review details
Suppressed comments (1)
.github/workflows/release.yml:156
actions/download-artifactis configured withpath: .(trailing space). GitHub Actions treats this as a distinct directory name (". "), so artifacts may be downloaded into the wrong folder and later steps that look in.will miss them.
- name: Download all artifacts
uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1
with:
path: .
pattern: bindings-*
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…table The guard read `statSync().mode & 0o111`, which is a POSIX idea. This suite also runs on the Windows matrix entry, where NTFS has no permission bits and node reports 0 for every file, so it failed a repository that was in fact correct. The mode that matters is the one recorded in git anyway: that is what decides whether `./scripts/install-cargo-zigbuild.sh` is runnable on the Linux runner that actually runs it. So ask git. Still fails when the bit is cleared with `git update-index --chmod=-x`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The changes are coherent, minimize CI drift via a shared composite action, and add targeted guard tests to prevent regressions in the new workflow structure.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Measured, now that it has run. Baseline is run 34217247574 on
Per job, where it came from:
The musl job is the one the prebuilt cargo-zigbuild was aimed at, and it moved the most — it was also the job gating everything else. Two caveats on these numbers:
The one bug this shook out was mine: the executable-bit guard used |
Closes #72.
Almost all of CI is one step. On a recent run, "Build in docker" was 180s, 184s and 289s of a ~6m wall clock, and everything else was noise:
x86_64-unknown-linux-muslx86_64-pc-windows-msvcaarch64-unknown-linux-gnux86_64-unknown-linux-gnuaarch64-apple-darwinTest bindingsjobsSo this PR is about that step, and about the fact that the tests waited on jobs they do not use.
The cargo cache never held the expensive part
The docker builds keep
CARGO_HOMEinside the container and bind-mounted$WORKSPACE/.cargo/registry/*into it. The cache step saved.cargo-cacheand the runner's~/.cargo. Those are three different directories, so the registry index and the downloaded crates were discarded after every run — which is why the restore step took 3s. The mounts and the cached paths now name the same.cargo-cachetree.registry/srcis deliberately not cached: cargo re-extracts it fromregistry/cache, so caching it only makes the archive bigger.…and it could never be refreshed anyway
The key was
<target>-cargo-<host>— no lockfile in it. After the first save, every later run hit that key exactly, restored atarget/from whenever it happened to be written, and then skipped saving.Post Cache cargo: 0son every run is that happening.It is now keyed on the lockfiles, with a prefix
restore-keysfallback so an unrelated change still starts from the previous build instead of from nothing. Since the container runs as root, achownfollows the docker build, or the save step cannot read what the build produced.cargo-zigbuild was rebuilt from source every time
cargo install cargo-zigbuild --version 0.23.4 --lockedcompiles the tool on every cross-compiled build. That is most of the 105s separating the musl job from the gnu job — on the job that sets the critical path.scripts/install-cargo-zigbuild.shinstalls the upstream prebuilt binary instead, verified against a pinned sha256. Anything unexpected — unsupported platform, missing curl, failed download — falls back tocargo install, so the worst case is current behaviour. Only a checksum mismatch is fatal, because that means the pin and the artifact disagree, which is not something to paper over.The tests waited for platforms they never use
test-bindingdownloadsx86_64-unknown-linux-gnuandx86_64-pc-windows-msvc.needs: buildmade it wait for musl and aarch64 as well — the two slowest jobs in the matrix.The matrix is split into
build(what the tests consume) andbuild-extra(everything else).test-bindingneeds only the former, so the tests — and the required check — start when the fast native builds finish.build-extrastill runs on every PR; nothing is dropped.Smaller things
npm cirather thannpm installin every job: lockfile-frozen and faster.ci.ymlandrelease.yml— which is how the cache paths and the docker mounts drifted apart in the first place. They are now one composite action (.github/actions/build-binding), so the cache layout and the toolchain pins exist in exactly one place.Guards
Three tests in
__test__/toolchain.test.ts, each checked to fail when mutated:test-bindingtests is built by a job itneeds— the split above is only safe while that holds, and otherwise the download step looks for an artifact no job uploads;ci.ymlandrelease.ymlcannot drift to different cross-compilers;./scripts/....Deliberately not done
paths-ignore.Test bindings on x86_64-unknown-linux-gnu - node@20is a required status check, and a workflow skipped by a path filter never reports it — docs-only PRs would sit forever waiting on a check that cannot arrive.fail-faststaysfalse. These are per-platform native builds. Whether a break is platform-specific or general is the first thing worth knowing, and cancelling the rest of the matrix hides exactly that.Verification
actionlintclean on both workflows.npx vitest run __test__/toolchain.test.ts— 7 passed. Each new guard also confirmed to fail when its invariant is broken (tested target moved out ofbuild; the pin inlined back intorelease.yml; the exec bit cleared).curl, and a corrupted checksum that fails hard rather than falling back.The build itself is what CI measures, so the actual numbers land on this PR's own run — and the first run pays for the cold cache it is repopulating.