ci: per-PR ext-link gate so a perry-runtime change cannot silently break the perry-ext-* link (#7656) - #7768
ci: per-PR ext-link gate so a perry-runtime change cannot silently break the perry-ext-* link (#7656)#7768proggeramlug wants to merge 1 commit into
Conversation
…eak the perry-ext-* link (#7656)
0d0ac7a to
8904058
Compare
|
Warning Review limit reached
Next review available in: 11 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
…C tag (#7768) `js_array_get_f64` and `js_array_length` asked both collection registries whether an ordinary array was secretly a Set or a Map on every element read. On `gc-handoff/apps/asyncpipe.ts` those two probes were 13.5% of the run: the program uses Map and Set, so the #7474 latch is armed and each probe really resolved a thread-local and hashed. Every registered Map/Set IS its `arena_alloc_gc(_, _, GC_TYPE_MAP|GC_TYPE_SET)` header, so the object's own `obj_type` answers in one byte. ABA-proof by construction — the tag lives inside the candidate bytes, so recycling the address rewrites it — which is what an address-keyed negative memo could not offer (#7755). The registry stays authoritative for the positive answer. The same header read now also supplies the descriptor flags that `array_object_flags` re-derived through a second `clean_arr_ptr`.
…7768) The object field-get funnel already proves `keys` is a live `GC_TYPE_ARRAY` and caps the index below its capacity, then called `js_array_get` per key — which re-establishes both facts from scratch: a `clean_arr_ptr` forwarding walk, a lazy-header probe, the exotic-receiver classifications and a descriptor-flag read. That funnel was 78% of all `js_array_get_f64` samples on `gc-handoff/apps/asyncpipe_big.ts`. `keys_array_len_capped_to_capacity` paid the same toll through `js_array_length` once per property read. `keys_array_slot` serves the dense, descriptor-free, non-forwarded case from the array's own words and delegates everything else — a hole, an out-of-range index, a forwarded or descriptor-carrying array, a null pointer — so no general semantics move. A test-only per-thread fallback counter pins both directions, so a fast path that stopped applying and one that started swallowing a shape it should have delegated are equally red. Also switches the #7768 receiver-tag read to `addr_class::try_read_gc_header`: this file's usual `>= GC_HEADER_SIZE + 0x1000` floor sits BELOW the handle band, and `js_array_length` reaches it before proxy/handle receivers are routed. Keeps the addr-class ratchet green too.
1 participant
Fixes #7656.
Why
cargo-test's scope cannot see thisWorth stating precisely, because the existing behaviour is deliberate and I did not change it:
ci_test_scope.py's_is_fanout_leafkeepsperry-ext-*/perry-stdlibout of the reverse-dependency fan-out, and its reasoning is sound —That is right about the tests and silent about the link. These crates pull in a feature-stripped runtime through
perry-ffi'sruntime-linkbuilt with-Wl,-dead_strip, so a new reference edge insideperry-runtimecan keep alive a chain the stripper had been removing. In #7650 the edge was one added call (pin_object→arena::classify_heap_space) replacing a raw flag write, and the symptom wasUndefined symbols for architecture arm64— not a failing assertion.So this adds a separate link-only arm and leaves the fan-out exclusion alone.
cargo test --release --no-runlinks the test binaries and stops; running them would add time and check nothing this does not.--releasedeliberately:-dead_stripis a release-profile behaviour, and a dev-profile build links a different set — it would be green through exactly the regression this exists to catch.Cost — and why the scope is all 38 crates, not #7650's five
The issue said "a wider set is better if it is affordable". Measured on an arm64 dev Mac rather than argued:
perry-ext-*perry-runtimealready builtThe shared
perry-runtimerelease build dominates, so covering every ext crate costs less than building the runtime once. There is no reason to scope to the five that happened to fail.The two things #7656 asked to get right
1. The gate asserts it linked something. The count comes from cargo's own
--message-format=jsonartifact records, and zero is a hard failure whose message says to fix the scope rather than trust the green. Sabotage-tested both ways: a stub cargo emitting no artifacts fails the step with::error::ext-link linked ZERO test binaries, one emitting two passes.2. Not promoted to required. Per CLAUDE.md's corollary — a new gate has never been green, so making it required immediately blocks every open PR. Run it once, then promote.
The list is derived, not written down
scripts/ci_ext_link_scope.pyenumeratescrates/perry-ext-*from the workspace, so a new ext crate is covered the day it lands. A hand-maintained list is precisely the failure mode #7748 had to repair inci_e2e_scope.py, where the map named 3 of 24 suites and nothing could say so. The self-test additionally pins that #7650's five are still present in the derived list, so a rename reports itself instead of quietly shrinking the gate.Validation
python3 scripts/ci_ext_link_scope.py --self-test— arming rules (runtime / stdlib / ffi / ext arm it; hir, codegen, docs do not), a non-empty derived list, and the perf(gc): stop traversing the young graph twice — skip the copying minor's eligibility preflight when its answer is already known (#7645) #7650 five still covered.The workflow bash was executed, not reviewed. Extracted the run step and drove it with a stub
cargoin three scenarios: links two → exit 0; links zero → exit 1 with the explicit error; cargo fails → exit 101.That third case found a real defect: the step originally exited 0 when cargo failed — the exact regression the job exists to catch — because it relied implicitly on the runner's default
bash -e. It is nowset -euo pipefailexplicitly.Real
cargo test --release --no-runover all 38 crates: exit 0, 216 binaries (the numbers above).Also guarded
setup-llvm22on the scope condition; it was running even when the diff cannot affect the archives, which defeats the point of the cheap scope step.Scope note
This gate would have caught #7650. It does not address #7629, whose crash also involves the ext archives — that one is a duplicate-runtime / reactor problem at auto-optimize link time, not a missing build, and remains open.
No version bump.