gate(gc): promote the parse-then-churn layout-state check to CI (#7647) - #7711
Conversation
|
Warning Review limit reached
Next review available in: 6 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 ignored due to path filters (1)
📒 Files selected for processing (10)
✨ 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 |
#7643 measured that PERRY_JSON_TAPE=0 + PERRY_GC_FROMSPACE_SCAN=1 over a parse-then-churn workload is a known-good end-to-end detector for the layout-state family (#7630/#7633/#7635/#7644), but nothing ran it in CI. Adds the fixture, a driver script, and a Python checker whose verdict requires correctness (dangling=0, byte-exact read-back after the churn), liveness (a copying minor actually relocated objects), and eagerness (the scan's own object census reached the record count, so the cohort was not still on the lazy tape) before it can say PASS -- and a --self-test proving the checker can say no on all three, in both directions. Building the fixture surfaced a real, general false-positive source in PERRY_GC_FROMSPACE_SCAN: string allocations were not zero-filling their 0-7 byte 8-byte-alignment pad, so leftover arena bytes there could occasionally decode as a plausible pointer and report a false "dangling" hit -- harmless to every string API (all bounded by byte_len/capacity, never GcHeader.size) but not to a scan that trusts GcHeader.size as the payload's true extent. Fixed at string_storage_alloc's single choke point, mirroring the existing TAG_HOLE fix for array-growth slack. Not wired into branch protection's required contexts here -- that is a maintainer action for after this job's first green run on main.
2a4213b to
46f4c5d
Compare
Merging as v0.5.1412Three things make this a gate rather than a script, and all three are the ones this repo keeps finding missing. 1. The live-subject assertion has three axes, all required to PASS:
That third axis is the one that matters here and the reason #7647 existed. 2. It can fail, demonstrated. Reapplying #7635/#7643's exact mutation ( 3. It compiles and runs under Wiring checked directly: no The bug found on the way, which was necessary, not incidentalThe clean runtime wasn't clean: ~1 in 40 record strings threw a false "dangling" hit. Root cause is Harmless to every string API (all bounded by Without this the gate would have been randomly flaky on legitimate builds — which is its own kind of gate nobody can trust. Maintainer action, not done herePromotion to a required context is deliberately not in this PR. A new gate has never been green, so making it required immediately blocks every open PR; it needs one observed green run on
|
Closes #7647.
What this promotes
#7643 (
layout_finish_deferred_boxed_objecthazard tests) left a follow-upnote:
PERRY_JSON_TAPE=0+PERRY_GC_FROMSPACE_SCAN=1over a parse-then-churnworkload is a known-good end-to-end detector for the whole layout-state
family (#7630/#7633/#7635/#7644) — with the JSON materialiser's finalize
sabotaged to always claim
POINTER_FREE, it reportsdangling=8000 owners=4000and the binary SIGBUSes; clean,dangling=0, exit 0. Nothing ranit in CI. #7643/#7644's unit tests are the right primary guard (workload-free,
so they can't be defeated by a lazy path or a GC that didn't happen to run),
but neither can catch a new materialiser path that forgets to finalize at
all, since a hand-built unit-test object never exercises a real call site.
This PR ships that gate:
scripts/fixtures/gc_parse_churn_layout_state.ts— 4,000 JSON records, 2pointer-bearing string fields each (matching test(gc): make the POINTER_FREE misdeclaration hazard detectable (#7635) #7643's own reproduction
numbers), sized to be Auto-mode-lazy-eligible, parsed, churned, then
read back and compared field-by-field.
scripts/gc_parse_churn_layout_gate.sh— compiles the fixture withPERRY_GC_MOVING_LOOP_POLLS=1(a compile-time gate as well as a runtimeone — the moving collector is opt-in per fix(gc): disable evacuating minor by default pending #7154 (use-after-free on dynamically-added fields) #7161's stopgap, and without it
every collection falls back to a non-moving conservative-stack-scan cycle
that never runs the from-space scan at all), runs it under
PERRY_JSON_TAPE=0 PERRY_GC_FROMSPACE_SCAN_ABORT=1 PERRY_GC_DIAG=1, andhands stdout/stderr/exit-code to the checker.
scripts/gc_parse_churn_layout_check.py— the actual pass/fail logic,asserting three things before it can say PASS:
MISMATCHES 0, no scanoffender line;
copied_objectssummed across every[gc-copy-minor] ran ...]line is nonzero (a copying minor actually relocated something —CLAUDE.md's "four ways a gate can be unable to fail", useEffect + setState panics with RefCell already borrowed on macOS ARM64 #4: a run with
zero collections must not pass);
objects=census reached atleast the fixture's record count.
PERRY_JSON_TAPE=0is supposed toforce the direct (eager) parser for every call, but trusting the env
var alone is exactly the kind of assumption gc: forcing POINTER_FREE on a pointer-bearing object strands nothing — our zeal/protect instruments do not discriminate the layout-state hazard #7635 showed worth
checking: a lazily-parsed cohort leaves only a handful of tape/lazy-
array objects live at scan time, nowhere near the record count. This
is the "record count materialised before the churn, or an equivalent
observable" Promote the tape=0 + from-space-scan parse-then-churn check to a CI gate for the layout-state family #7647 asks for.
python3 scripts/gc_parse_churn_layout_check.py --self-testproves thechecker can say no on all three axes, in both directions (7 cases: a clean
run passes; a real dangling reference, silent post-churn corruption, zero
copying minors, the scan never running, and the lazy-tape/vacuity shape
each fail with a distinct, specific message).
.github/workflows/gc-parse-churn-gate.yml— wires it up.on: pull_request / push: branches: [main] / workflow_dispatch, nocontinue-on-error, no|| true,concurrency.cancel-in-progressscoped topull_requestonly(
mainruns queue by commit SHA rather than cancelling each other —CI: gc-ratchet's main runs are cancelled while queued, so the gate has executed zero times in three merges #7205's lesson). A path-relevance filter (mirroring
gc-moving-witnesses.yml) skips the compiler build on changes that can'treach this path.
scripts/gc_gate_wiring_check.py'sGATESlist (the repo'sexisting "is this gate main-line-reachable and able to fail" auditor), so
a future edit that accidentally makes it tags-only or job-level
continue-on-errorfailslintinstead of going unnoticed likegc-root-dominancedid for weeks.Not required-on-merge. Per CLAUDE.md's corollary to "four ways a gate can
be unable to fail" — a new gate has never been green, so promoting it
immediately blocks every open PR — this needs one observed green run on
mainfirst. Addinggc-parse-churn-gateto branch protection's requiredcontexts is a maintainer action; this PR does not and cannot do it.
A real bug the fixture found along the way
Building the fixture, the "clean" arm was NOT clean: on a correct,
unsabotaged runtime, roughly 1 in 40 parsed-record strings reported a false
danglinghit, deterministic given fixed inputs, alwaystype=3(String)owner, always at a fixed relative offset.
Root cause:
arena_alloc_gcpads every allocation's total size up to 8-bytealignment (
gc_padded_total_size), so a string whose own natural size isn'talready a multiple of 8 gets 0–7 trailing bytes that are part of the
allocation (
GcHeader.size, whatPERRY_GC_FROMSPACE_SCANtrusts as thepayload's true extent) but were never requested by, or written by, the
caller.
js_array_growalready has a fix for the equivalent hazard on arraygrowth — its
[old_capacity, new_capacity)slack is explicitlyTAG_HOLE-filled, with a comment naming this exact class of bug — butstring_storage_allochad no equivalent, and strings routinely land onnon-8-aligned lengths. Leftover bytes from whatever the arena block last
held there can, and measurably did, decode as a plausible NaN-boxed or bare
pointer. Harmless to every actual string API (
.length, indexing, iterationare all bounded by
byte_len/capacity, neverGcHeader.size) — confirmedby
MISMATCHES 0and clean exits throughout, including deliberatelychurn-free and churn-heavy variants used to isolate it — but a genuine
false-positive source for this exact instrument.
Fixed at the single choke point (
string_storage_alloc/string_storage_alloc_longlivedincrates/perry-runtime/src/string/mod.rs):read
GcHeader.sizeback after allocation and zero the difference betweenthat and the requested payload size (≤7 bytes, negligible next to the
content copy it sits beside). The
GcHeadercast this needs is the same"fresh allocation, never a NaN-box payload" shape already grandfathered for
arena/allocators.rs; added a matching, narrowly-scopedscripts/addr_class_allowlist.txtentry rather than routing throughaddr_class::try_read_gc_header(which classifies untrusted NaN-box payloadmagnitudes — not what's happening here, since
rawis a pointer this samefunction just got back from the allocator).
cargo test -p perry-runtime --lib(1945 passed, 0 failed) after the fix.Sabotage verification
Reapplied #7635/#7643's exact mutation locally (both
parse_objectcallsites in
crates/perry-runtime/src/json/parser.rs,layout_finish_deferred_boxed_object(js_obj as usize, /* saw_pointer */ false)), rebuilt the runtime, and ran the gate script three times:Reverted the sabotage, rebuilt, reran three more times: consistent
PASS: parse-then-churn layout-state gate is clean and its subject was live.at exit 0, with
[gc-copy-minor] ran copied_objects=...lines showingnonzero relocation across 5+ collection cycles and the from-space scan's
objects=census running 17k+ (well above the 4,000-record floor) eachtime.
Also verified the vacuous-configuration case is rejected at the checker level
(
--self-test's "tape stayed lazy" case): a captured run whose scan censusnever exceeds a handful of objects fails with an explicit "record cohort was
not eagerly materialised... #7635's original vacuity, recurring" message,
not a silent pass.
What this deliberately does not cover
One fixed-shape probe (4,000 records, 2 string fields), not a sweep over
every layout-state shape the codebase can produce — the end-to-end
complement to #7643/#7644's unit tests, not a replacement for them, and not
a substitute for
gc-moving-witnesses(#7154 stale-root reproducers) orgc-root-dominance(static root-store dominance).Validation
python3 scripts/gc_parse_churn_layout_check.py --self-test(7/7)scripts/gc_parse_churn_layout_gate.shagainst a cleanperry-devbuild:3/3 green
scripts/gc_parse_churn_layout_gate.shagainst the gc: forcing POINTER_FREE on a pointer-bearing object strands nothing — our zeal/protect instruments do not discriminate the layout-state hazard #7635-sabotaged build:3/3 correctly red, with the from-space scan's offender line and the
checker's message both naming the real defect
cargo test -p perry-runtime --lib— 1945 passed, 0 failedcargo fmt --all -- --checkpython3 scripts/gc_gate_wiring_check.py/--self-testpython3 scripts/addr_class_inventory.pypython3 scripts/gc_store_site_inventory.py/--self-testbash scripts/check_file_size.sh