ci(gc): give gc-native-roots a concurrency group - #7393
Merged
Conversation
added 2 commits
August 4, 2026 21:00
The workflow had none, so nothing ever superseded a stale run and its four-arm matrix multiplied across every push. Ten consecutive runs were checked: the macos-14 arm was `queued` in EVERY one of them -- never executed, not once. ubuntu-latest (x86-64 ELF) and windows-latest (PE) likewise. Only the aarch64 arm ever reached a runner, which is why it was the only arm ever observed red or green. Three quarters of this matrix has been reporting nothing while presenting as four-platform coverage. It also made #7392 unanswerable: whether that segfault is ELF-specific cannot be told apart from "the macOS arm has never run the probe". cancel-in-progress: false alone would not fix it -- GitHub allows at most one PENDING run per group and cancels the previously pending one when a new run enters, regardless of that setting (#7205). Keying push runs on the SHA gives every merged commit its own group while PR runs supersede freely, the same shape llvm-inprocess.yml already uses (#7357).
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe GC native roots workflow now groups runs by event and ref or commit SHA. Pull-request runs cancel older in-progress runs, while push runs remain separate. A changelog entry documents the change. ChangesGC native roots concurrency
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
This was referenced Aug 4, 2026
proggeramlug
added a commit
that referenced
this pull request
Aug 4, 2026
* docs(plan): fold in the 2026-08-04 findings Two things this plan treated as measured were not. Statepoints could not compile on aarch64-ELF at all -- a hard failure on a default-on path, from two stacked bugs (#7390: the compact stack-map parser did not model GNU-as `sym = expr`, emitted only at -O3 and only on ELF; and the assembler was not told the -mcpu the code generator was told, so Graviton-emitted SVE was rejected) behind two toolchain ones (#7384, #7388). And three of the four RS4GC matrix arms had NEVER executed, in any run, for want of a concurrency group (#7393). Every "the ELF arm is the only one red" conclusion rested on arms that never reached a runner. That is a fifth way a gate cannot fail, and it is now written down. Also folded in: nine Layer 3 rooting fixes and the rule they share (ordering, not missing roots; a fault that MOVES is a real fix, one that does not move by a byte was already dead before you rooted it); #7380's type confusion and the `gc_type == GC_TYPE_OBJECT` generalisation; RSS -69% (#7377); and the first honest performance measurement -- two benchmarks that measure nothing (#7395) and the array-store guard's siting cost (#7396). The Layer 1 framing is corrected: lower_exprs_rooted already implements the RFC's proposal for codegen operands, gated on any_later_ref_may_trigger_gc, and all four arms of func_ref.rs use it. So the gap is Layer 3, where #7389 supplies the first structural answer. * docs: name the fragment for its real PR (#7397) --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
proggeramlug
added a commit
that referenced
this pull request
Aug 5, 2026
* ci(gc): the macOS in-process RS4GC arm could never pass `RS4GC works on a stock toolchain via the in-process backend` asserts that a copying minor actually moved objects, by counting `[gc-copy-minor] ran copied_objects=` lines in the probe's stderr. Both of those prints are gated on PERRY_GC_DIAG (gc/copying.rs:993 and :1246), and this step never set it -- the sibling walker step does. So the trace held nothing but the probe's own #gcmetric lines, the assert read 0 copying minors / 0 objects copied off an effectively empty file, and the step failed regardless of how the collector behaved. The inverse of the usual hazard: not a gate that cannot fail, but one that cannot PASS. It shipped with the step in #7339 and had never executed, because three of the four arms in this matrix were permanently queued until #7393 added a concurrency group. Also makes the assert say what actually happened. A trace with no [gc-*] diagnostics at all is indistinguishable, by counts alone, from a collector that moved nothing, and the old message asserted the latter. That misdiagnosis is what made this cost a build to identify. Reproduced on macOS aarch64 against current main (cd29706, which contains #7398 and #7400, so it was not already fixed): the step as written reproduces the CI error byte-for-byte with a 3-line stderr; the same binary with PERRY_GC_DIAG=1 reports 2 copying minors / 10892 objects copied and the full step exits 0, stdout unchanged so the control diff still holds. The new branch is capable of failing: it fires on the pre-fix trace, passes on the post-fix one, and two negative controls (diagnostics present but zero copies; a synthetic manual_collect trace) still fail with the original message. This does not make the workflow green -- the other three arms fail earlier in "Probe matrix" for unrelated reasons. * docs: changelog fragment for #7414 --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
proggeramlug
added a commit
that referenced
this pull request
Aug 5, 2026
…#7417) * fix(ci): stop interpolating the LLVM version into the pwsh setup step zizmor has been red on every `main` commit since #7353 created `.github/actions/setup-llvm22/action.yml` -- roughly 40 consecutive commits. #7388 and #7393 only shifted the reported line numbers, which is what made them look implicated; neither introduced a finding. Four high-severity findings, one of which is properly fixed here. The Windows arm interpolated a composite-action input straight into a PowerShell script body (`$ver = "${{ inputs.version }}"`), which `template-injection` flags at High confidence: the expansion is substituted as raw text before pwsh parses the line, so an input carrying a quote plus a statement separator would execute as code with the runner's privileges. The input now arrives through an `env:` block and is read as `$env:LLVM_VERSION`, a plain string load. The other three are `github-env` at Low confidence -- the single `LLVM_SYS_221_PREFIX=<prefix>` line the action exists to write, once per platform arm -- and are suppressed with reasoning in `.github/zizmor.yml`. Measured: the audit is satisfiable only by not writing the environment file at all, and the clean alternative ($GITHUB_OUTPUT plus composite outputs) costs 44 jobs and 140 downstream steps, recreating the duplication the action exists to remove. The carve-out is a dated ratchet with an explicit delete-condition. Verified with the repo's SRI-pinned zizmor 1.28.0: pristine config plus this fix reports 3 high and exits 14; with the carve-out it exits 0 and `ignored` rises 119 -> 122, matching the three suppressed findings exactly. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH * docs: name the fragment for its real PR (#7417) --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
proggeramlug
added a commit
that referenced
this pull request
Aug 5, 2026
…ds (#7418) * fix(ci): satisfy the phantom xml2s.lib the LLVM Windows tarball demands LLVM's official Windows release script builds a static libxml2 into a scratch directory and points cmake at it with -DLLVM_ENABLE_LIBXML2=FORCE_ON -DLIBXML2_LIBRARIES=%libxmldir%/lib/libxml2s.lib. %libxmldir% is never installed, so the published clang+llvm-*-pc-windows-msvc tarball carries the dependency but not the library. llvm-config --system-libs --link-static reports xml2s.lib, llvm-sys forwards every system lib verbatim with no knob to filter one out, and link.exe dies with LNK1181 before resolving a symbol. Synthesize an empty archive at the LLVM libdir when llvm-config reports xml2s.lib AND the libdir lacks it. It is a name dependency, not a symbol dependency: libxml2 is reachable only from LLVMWindowsManifest, which the LLVM-C surface inkwell drives never touches, and rustc bundles the component archives into libllvm_sys.rlib where link.exe pulls members lazily. If that stops being true the link fails loudly with LNK2019 rather than silently dropping manifest support. Checking both conditions makes the workaround self-deleting once a release ships or stops reporting the library. Latent since #7353 made the in-process LLVM backend the default and statically linked, not caused by #7388 (which touches only the Linux arm). It became visible when #7393's concurrency group let gc-native-roots.yml's windows-latest arm reach a runner for the first time. Fixing it in the composite action also unblocks test.yml's windows-build. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH * docs: name the fragment for its real PR (#7418) --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three of this workflow's four platform arms have never executed.
Measured, not inferred
I went looking because #7392 asks whether an RS4GC segfault is ELF-specific — macOS uses the same lowering, so if it were lowering-generic that arm should fail too. Checking ten consecutive
gc-native-rootsruns:ubuntu-latest(x86-64 ELF) andwindows-latest(PE) the same. Only the aarch64 arm has ever reached a runner — which is precisely why it is the only arm anyone has ever seen go red or green.Cause
The workflow has no
concurrencyblock at all, so nothing supersedes a stale run and the four-arm matrix multiplies on every push. Today's merge volume saturated the queue outright.cancel-in-progress: falsealone would not fix it: GitHub allows at most one PENDING run per group and cancels the previously pending one when a new run enters, regardless of that setting (#7205). Keying push runs on the SHA gives every merged commit its own group while PR runs supersede freely — the shapellvm-inprocess.ymlalready uses from #7357.Why it matters beyond tidiness
This is CLAUDE.md's fourth hazard in a different guise: three quarters of this matrix has been reporting nothing while presenting as four-platform coverage. Every "the ELF arm is the only one red" conclusion drawn today — including mine — rested on arms that had simply never run.
It also makes #7392 unanswerable as filed. "Is the segfault ELF-specific?" cannot be distinguished from "the macOS arm has never run the probe" until this lands.
Summary by CodeRabbit
Chores
Documentation