Skip to content

ci: build, lint and test with --locked - #157

Merged
juan-malbeclabs merged 3 commits into
mainfrom
jo/ci-enforce-cargo-locked
Sep 11, 2026
Merged

juan-malbeclabs merged 3 commits into
mainfrom
jo/ci-enforce-cargo-locked

Conversation

@juan-malbeclabs

Copy link
Copy Markdown
Contributor

Closes the detection gap behind #155. Merge after #151, which is what actually repairs Cargo.lock on main — until that lands, this PR's own rust jobs are (correctly) red, and cargo-deny is red on every PR in this repo because of the yanked chacha20 0.10.1 that #151 also bumps.

Summary

  • Adds --locked to the three cargo invocations in rust.yml (build, clippy, test).
  • The Dockerfile's cargo build --release --locked was the only place the lock was enforced. CI re-resolved silently, so a Cargo.lock that Cargo would have to rewrite passed every check and first surfaced days later as a failed GHCR publish — three days of failed image publishes, with :mainnet-beta left on the 0.38.0 client base across a doublezero release (Image publishes have failed since 2026-09-08: Cargo.lock on main is unusable with --locked #155).
  • A stale or internally inconsistent lock now fails the PR that introduces it, on the PR run rather than on a release.

This closes the detection gap, not the cause: two Dependabot PRs whose locks are each valid against their own base still merge into an invalid one. Requiring branches to be up to date before merging (or a merge queue) is the branch-protection change that prevents that — a repo setting, not code.

Testing Verification

  • Reproduced the gap at main (e258968): cargo build --workspace --release succeeds while cargo build --workspace --release --locked fails with cannot update the lock file. With this change, that commit would have gone red on the PR that introduced it instead of three days later on a publish.
  • Verified cargo build --workspace --release --locked succeeds against deps: pin past the yanked chacha20 0.10.1 #151's repaired lock (91e8ec5).

(Replaces #154, which GitHub would not let me retarget off the closed stack.)

`cargo deny check advisories` fails on `main`: the tree carries
chacha20 0.10.1, which its author yanked. It comes in through
tokio-tungstenite 0.30 -> tungstenite -> rand 0.10.2, which depends on
chacha20 directly, so nothing here picked the version and no Cargo.toml
edit avoids it. The lockfile moves to the republished 0.10.2.

A yank is not a disclosed vulnerability, but the policy denies it
because a withdrawn version has no upgrade path and no advisory
coverage, and this one is the RNG under the WebSocket stack. The check
is a required status, so the failure also blocks every unrelated pull
request.

Lockfile only, `cargo update -p chacha20`. The same resolution drops the
digest/generic-array chain 0.10.1 pulled in and collapses the duplicate
cpufeatures and block-buffer versions. `cargo deny check` is clean on
all four sets; build and the full test suite pass.

@juan-malbeclabs juan-malbeclabs left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Notes on the tradeoffs rather than an endorsement, since I opened this one.

What it does not catch. --locked catches a lock Cargo would have to rewrite. It does not catch the merge that creates one: #85 and #81 would each have been green with --locked too, because each lock was valid against the base it was resolved against. The invalid state only exists after the merge, so this guard fires on the push-to-main run, not on either PR. Still days earlier than a failed publish, but the real fix is "require branches to be up to date before merging" (or a merge queue) on main — a repo setting nobody can put in a diff. Tracked in #155.

Cost. A PR that edits Cargo.toml without regenerating the lock now fails with cannot update the lock file … --locked, which is a worse first read than a resolver error. Dependabot is unaffected: it updates manifest and lock together.

Redundancy. All three jobs share one lock, so --locked on clippy and test adds nothing once rust-build has it. Kept anyway so no job can be run alone or reordered and quietly re-resolve.

Merge after #151. The rust jobs here are correctly red until main's lock is repaired, and cargo-deny is red for the unrelated yanked chacha20 0.10.1 that #151 also bumps.

@juan-malbeclabs
juan-malbeclabs requested review from a team and removed request for a team September 11, 2026 01:29
The Dockerfile's `cargo build --release --locked` was the only place the lock
was enforced. `rust` CI re-resolved silently, so a Cargo.lock that Cargo would
have to rewrite passed every check and first surfaced days later as a failed
GHCR publish — which is how two independently-resolved Dependabot locks merged
into an unusable one and stalled the image publishes for three days (#155).

A stale or internally inconsistent lock now fails the PR that introduces it.
@juan-malbeclabs
juan-malbeclabs force-pushed the jo/ci-enforce-cargo-locked branch from 3add126 to c80bcf5 Compare September 11, 2026 10:21
@juan-malbeclabs

Copy link
Copy Markdown
Contributor Author

Rebased onto #151's branch so this can be reviewed green instead of red.

Read the diff as rust.yml + its CHANGELOG.md entry only. The base is still main, so GitHub also shows #151's Cargo.lock in the file list — that is #151's change, not a duplicate of it. Once #151 merges, its commit becomes an ancestor of main and the diff here collapses back to the two files that are actually mine.

Before the rebase, all three rust jobs died in 8-20s on main's lock — which was the flag working as intended, not a defect:

error: cannot update the lock file .../Cargo.lock because --locked was passed to prevent this

Verified on the rebased head: cargo metadata --locked exits 0 and cargo build --workspace --release --locked succeeds.

Merge order is unchanged — #151 first, this second.

@thijsvanemmerik thijsvanemmerik left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Approving. The three --locked flags are right. That includes the clippy one, where the flag has to go before the -- separator. On this branch (c80bcf5) cargo metadata --locked passes. On main it fails.

Three sentences in the write-up need fixing first. Two of them end up in a rust.yml comment, so they stay in the repo. One push fixes all three, and there is no need to re-review.

1. The Dockerfile was not the only place the lock was enforced.

release.shred-proxy.yml has used --locked at lines 52 and 55 since 2026-07-14. It runs on a tag, not on a push, so it stayed quiet through all of this. But it means a shred-proxy-v* tag pushed against main right now would fail the same way.

Suggested: "the Dockerfile image build and the tag-triggered shred-proxy release were the only places the lock was enforced, and neither runs on a pull request."

2. The break did not show up days later. It showed up in the same minute.

release.docker.edge-connect went red on the merge pushes themselves. 16:01:41Z (ae63267), then 16:01:49Z, then 16:01:58Z, all on 2026-09-07, all with the exact --locked error. security-audit went red on the same pushes. The last good publish was 16:01:39Z, seconds before.

So the signal was there straight away. What is missing is that nothing tells anyone when a push to main goes red.

3. This would not have caught #155.

The changelog says a stale or inconsistent lock "now fails the PR that introduces it". No PR carried the bad lock. It only existed once the merges combined. Take #85's head, merge it into the base its checks ran against (87f9d88), and cargo metadata --locked passes. Those checks last ran 2026-08-01, which is 37 days before it merged. strict_required_status_checks_policy is off, so the stale green still counted.

Suggested: "A stale or inconsistent lock now fails CI on the PR that carries it and on the push to main." Keep the line about requiring branches to be up to date, or a merge queue. That is the part that actually prevents this.

While you are in CHANGELOG.md, the #151 Security entry needs the two fixes noted on that PR.

What to expect when merging.

The main ruleset only allows squash and rebase. So when #151 merges, its commit does not become an ancestor of main. This PR's merge base stays at e258968, and the Files view keeps showing Cargo.lock. Rebase onto the new main when you push the wording fix and it collapses to two files. Merge without rebasing and the result is still correct, but the squash message box comes pre-filled with #151's text.

For a later PR, not this one.

Two cargo calls still re-resolve. release/.goreleaser.base.edge-cli.yaml builds the published .deb and .rpm without --locked, in the before hook and in builds[].flags. Those packages go to users. The cargo-deny step has no --locked either, so against a broken lock it audits a repaired tree instead of the committed one.

@juan-malbeclabs
juan-malbeclabs enabled auto-merge (squash) September 11, 2026 13:46
@juan-malbeclabs
juan-malbeclabs merged commit 67d48a8 into main Sep 11, 2026
8 checks passed
@juan-malbeclabs
juan-malbeclabs deleted the jo/ci-enforce-cargo-locked branch September 11, 2026 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants