test(gc): gate matrix arm liveness, and put a relocating arm back in the PR subset (#7255) - #7275
Merged
Merged
Conversation
proggeramlug
force-pushed
the
fix/7255-matrix-liveness
branch
from
August 2, 2026 19:47
e65f39b to
b2d82b5
Compare
📝 WalkthroughWalkthroughThe GC matrix adds ChangesGC matrix liveness enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant GC matrix
participant Liveness checker
participant Inert-arm registry
participant CI lint
GC matrix->>GC matrix: Run configured arms
GC matrix->>Liveness checker: Write and validate JSON report
Liveness checker->>Inert-arm registry: Load registered inert arms
Liveness checker-->>GC matrix: Return pass or failure
CI lint->>Liveness checker: Run self-test and registry check
Liveness checker-->>CI lint: Return validation status
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
test-parity/gc_matrix_inert_arms.txt (1)
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument that the reason field cannot contain
|.
parse_registryinscripts/gc_matrix_liveness_check.pysplits each line on|and rejects the line unless it yields exactly three fields. A future reason that quotes a||operator or a shell pipe failslintwith a format error that reads as a typo. State the restriction next to the format line.♻️ Proposed wording
# Format: <arm id> | <`#issue`> | <reason, and what makes this entry deletable> +# The reason is free text on ONE line and must not contain `|`.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-parity/gc_matrix_inert_arms.txt` at line 3, Update the format comment in gc_matrix_inert_arms.txt to explicitly state that the reason field must not contain the pipe character (|), including quoted shell pipes or || operators, so parse_registry continues receiving exactly three fields.scripts/gc_matrix_liveness_check.py (1)
101-114: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
matrix_armscan silently skip arm records, and the self-test does not detect it.
^"([^"]*)"$matches only records that contain no double quote. An arm note that quotes an identifier is skipped without error.check_registrywould then report that a registered arm "no longer exists", and a skipped arm would never be liveness-checked. The self-test at line 408 only assertslen(arms) >= 5, so a partial parse still passes.Count the record lines in the
ARMS=( ... )body and fail when the parsed count differs.♻️ Proposed fix
arms = {} - for line in re.finditer(r'^"([^"]*)"$', body.group(1), re.M): + record_lines = [ + ln for ln in body.group(1).splitlines() if ln.strip() and not ln.lstrip().startswith("#") + ] + for line in re.finditer(r'^"(.*)"$', body.group(1), re.M): fields = line.group(1).split("|") if len(fields) < 4: raise Violation("malformed arm record: %r" % line.group(1)[:60]) arms[fields[0]] = fields[3] if not arms: raise Violation("the ARMS=( ... ) array in %s parsed to nothing" % MATRIX.name) + if len(arms) != len(record_lines): + raise Violation( + "the ARMS=( ... ) array in %s has %d record line(s) but %d parsed. A record " + "this parser skips is an arm the liveness gate never checks." + % (MATRIX.name, len(record_lines), len(arms)) + ) return arms🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/gc_matrix_liveness_check.py` around lines 101 - 114, Update matrix_arms to count all arm record lines in the ARMS body and raise Violation when that count differs from the number of successfully parsed records, preventing quoted records from being silently skipped. Keep the existing malformed-record validation and empty-array handling, and strengthen the matrix_arms self-test so partial parsing cannot pass with only a minimum-length assertion.scripts/gc_repsel_matrix.sh (1)
213-216: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe subset summary omits
force_verify.
PR_ARMSnames seven arms. This sentence enumerates six roles and leaves outforce_verify, which line 247 then treats as part of the stronger acceptance route. Add it here so the enumeration matchesPR_ARMS.♻️ Proposed wording
-# PR-gating subset: the arms with the most detection power per second -- the -# shipped configuration under pressure, the two routes that actually relocate, -# the evacuation verifier, precise-roots-only, and the untouched shipped -# configuration as a control. +# PR-gating subset: the arms with the most detection power per second -- the +# shipped configuration under pressure, the two routes that actually relocate, +# forced evacuation with the verifier, the evacuation verifier, +# precise-roots-only, and the untouched shipped configuration as a control.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/gc_repsel_matrix.sh` around lines 213 - 216, Update the PR-gating subset summary comment near PR_ARMS to explicitly include the force_verify arm, so the listed roles match all seven configured arms and the stronger acceptance route described later.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/gc_matrix_liveness_check.py`:
- Around line 194-254: Update check_registry() in
scripts/gc_matrix_liveness_check.py to add a violation when Path(__file__).name
is absent from matrix_text, verifying the matrix still invokes the checker. In
.github/workflows/test.yml lines 208-216 and
changelog.d/7275-gc-matrix-liveness-gate.md lines 38-44, retain the wiring
claims only because this assertion now enforces them; otherwise remove those
clauses.
- Around line 12-19: Unify the residual corpus-size wording across the four
affected documents: scripts/gc_matrix_liveness_check.py lines 12-19,
scripts/gc_repsel_matrix.sh lines 602-611, .github/workflows/test.yml lines
208-216, and changelog.d/7275-gc-matrix-liveness-gate.md lines 12-22. Choose the
correct denominator and use it consistently everywhere, or replace the
denominator with “corpus-wide” in every location.
- Around line 268-420: Update self_test’s final check-count reporting to derive
the total from the checks actually executed, including the expect calls,
registry-parse cases, and real-matrix assertions, instead of the hand-maintained
“12 + 6 + 3” expression. Preserve the existing failure counting and output
format while ensuring the reported total is 20 for the current checks.
In `@scripts/gc_repsel_matrix.sh`:
- Around line 101-105: Update the --help handler in scripts/gc_repsel_matrix.sh
to display the complete usage block, including --json, --list-arms, and
--liveness-report-only. Replace the hard-coded sed range in the option handling
with a range that reaches the usage block’s actual end without truncation.
---
Nitpick comments:
In `@scripts/gc_matrix_liveness_check.py`:
- Around line 101-114: Update matrix_arms to count all arm record lines in the
ARMS body and raise Violation when that count differs from the number of
successfully parsed records, preventing quoted records from being silently
skipped. Keep the existing malformed-record validation and empty-array handling,
and strengthen the matrix_arms self-test so partial parsing cannot pass with
only a minimum-length assertion.
In `@scripts/gc_repsel_matrix.sh`:
- Around line 213-216: Update the PR-gating subset summary comment near PR_ARMS
to explicitly include the force_verify arm, so the listed roles match all seven
configured arms and the stronger acceptance route described later.
In `@test-parity/gc_matrix_inert_arms.txt`:
- Line 3: Update the format comment in gc_matrix_inert_arms.txt to explicitly
state that the reason field must not contain the pipe character (|), including
quoted shell pipes or || operators, so parse_registry continues receiving
exactly three fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f569f992-2f1e-4e18-9a31-68546d94a6d3
📒 Files selected for processing (7)
.github/workflows/gc-moving-witnesses.yml.github/workflows/test.ymlchangelog.d/7275-gc-matrix-liveness-gate.mddocs/representation-selection-rfc.mdscripts/gc_matrix_liveness_check.pyscripts/gc_repsel_matrix.shtest-parity/gc_matrix_inert_arms.txt
Comment on lines
+12
to
+19
| That is not hypothetical. #7024 measured the `default` arm at copy-minor 0/22, | ||
| fixed it to 12/22, and the script's header recorded the fix in its strongest | ||
| formatting. #7161 then flipped `PERRY_GC_MOVING_LOOP_POLLS` default-OFF as a | ||
| stopgap for #7154, which took the same arm back to **0/49** — and the header | ||
| still read `12/22`, because it was a hand-maintained number. Four of the six | ||
| PR-gating arms were inert for weeks. Two open crash reports (#6982, #7018) could | ||
| not be judged in either direction, because both crash *inside* a copying minor | ||
| and the arms that were supposed to run one ran none. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The corpus denominator disagrees across the four new or changed documents. The same historical measurement is written as 0/49 in two places and 0/50 in two others. This PR removes hand-maintained liveness counts, so this residual number must be one value or absent.
scripts/gc_matrix_liveness_check.py#L12-L19: reads0/49.scripts/gc_repsel_matrix.sh#L602-L611: reads0/49..github/workflows/test.yml#L208-L216: reads0/50.changelog.d/7275-gc-matrix-liveness-gate.md#L12-L22: reads0/50.
Pick the correct corpus size, or write "corpus-wide" without a denominator.
📍 Affects 4 files
scripts/gc_matrix_liveness_check.py#L12-L19(this comment)scripts/gc_repsel_matrix.sh#L602-L611.github/workflows/test.yml#L208-L216changelog.d/7275-gc-matrix-liveness-gate.md#L12-L22
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/gc_matrix_liveness_check.py` around lines 12 - 19, Unify the residual
corpus-size wording across the four affected documents:
scripts/gc_matrix_liveness_check.py lines 12-19, scripts/gc_repsel_matrix.sh
lines 602-611, .github/workflows/test.yml lines 208-216, and
changelog.d/7275-gc-matrix-liveness-gate.md lines 12-22. Choose the correct
denominator and use it consistently everywhere, or replace the denominator with
“corpus-wide” in every location.
Comment on lines
+194
to
+254
| def check_registry(out=sys.stdout) -> list: | ||
| """Static checks that need no run: names resolve, and the gate is wired in.""" | ||
| violations = [] | ||
| try: | ||
| registry = parse_registry(REGISTRY.read_text()) if REGISTRY.exists() else {} | ||
| except Violation as exc: | ||
| return [str(exc)] | ||
|
|
||
| matrix_text = MATRIX.read_text() | ||
| try: | ||
| known = matrix_arms(matrix_text) | ||
| pr_arms = matrix_pr_arms(matrix_text) | ||
| except Violation as exc: | ||
| return [str(exc)] | ||
|
|
||
| for arm_id, (issue, _) in sorted(registry.items()): | ||
| if arm_id not in known: | ||
| violations.append( | ||
| "%s lists arm %r (%s), which no longer exists in %s. A registry entry " | ||
| "for a deleted arm hides nothing and outlives its reason." | ||
| % (REGISTRY.name, arm_id, issue, MATRIX.name) | ||
| ) | ||
|
|
||
| for arm_id in pr_arms: | ||
| if arm_id not in known: | ||
| violations.append("PR_ARMS names %r, which is not in the ARMS table" % arm_id) | ||
|
|
||
| # THE STRUCTURAL HALF OF #7255, and the reason this check is worth having in | ||
| # `lint` at all: the run-time gate can be satisfied by registering every | ||
| # inert arm, which would leave a PR subset that cannot relocate anything and | ||
| # still exits 0. So the subset must retain at least one arm that claims to | ||
| # relocate AND is not on the known-inert list. This is the property the | ||
| # matrix header used to assert in prose ("THIS SUBSET CAN NOW REPRODUCE THE | ||
| # RELOCATING-MINOR DEFECT CLASS") with a number that went stale. | ||
| relocating = [ | ||
| arm | ||
| for arm in pr_arms | ||
| if known.get(arm) in ("scavenge", "move") and arm not in registry | ||
| ] | ||
| if not relocating: | ||
| violations.append( | ||
| "PR_ARMS (%s) contains no arm that both claims to relocate " | ||
| "(requires=scavenge/move) and is absent from %s. On a pull request the " | ||
| "matrix would then be unable to observe a raw reference held across a " | ||
| "relocating collection at all — the #6993 class (#6951, #6972, #6982, " | ||
| "#6991, #6992), which is the hole #7255 was filed about. Registering " | ||
| "arms as known-inert is not a way to buy silence: put a live relocating " | ||
| "arm back in the subset." % (",".join(pr_arms), REGISTRY.name) | ||
| ) | ||
|
|
||
| out.write( | ||
| "registry: %d known-inert arm(s), %d arms declared in %s\n" | ||
| % (len(registry), len(known), MATRIX.name) | ||
| ) | ||
| out.write( | ||
| "PR subset: %s\n relocating and not known-inert: %s\n" | ||
| % (",".join(pr_arms), ",".join(relocating) or "NONE") | ||
| ) | ||
| for arm_id, (issue, reason) in sorted(registry.items()): | ||
| out.write(" %-24s %-8s %s\n" % (arm_id, issue, reason[:100])) | ||
| return violations |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Two documents assert a wiring check that check_registry() does not implement. The root cause is in the checker: check_registry() validates the registry, ARMS, and PR_ARMS, but never confirms that scripts/gc_repsel_matrix.sh still invokes gc_matrix_liveness_check.py. If the invocation is deleted, lint stays green and the run-time half of the gate disappears silently.
scripts/gc_matrix_liveness_check.py#L194-L254: add a violation whenPath(__file__).nameis absent frommatrix_text..github/workflows/test.yml#L208-L216: keep the comment only after the assertion exists; otherwise remove the "the matrix still calls the checker at all" clause.changelog.d/7275-gc-matrix-liveness-gate.md#L38-L44: keep the "verified to be wired" parenthetical only after the assertion exists; otherwise remove it.
📍 Affects 3 files
scripts/gc_matrix_liveness_check.py#L194-L254(this comment).github/workflows/test.yml#L208-L216changelog.d/7275-gc-matrix-liveness-gate.md#L38-L44
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/gc_matrix_liveness_check.py` around lines 194 - 254, Update
check_registry() in scripts/gc_matrix_liveness_check.py to add a violation when
Path(__file__).name is absent from matrix_text, verifying the matrix still
invokes the checker. In .github/workflows/test.yml lines 208-216 and
changelog.d/7275-gc-matrix-liveness-gate.md lines 38-44, retain the wiring
claims only because this assertion now enforces them; otherwise remove those
clauses.
Comment on lines
+268
to
+420
| def self_test() -> int: | ||
| import io | ||
|
|
||
| failures = [] | ||
|
|
||
| def expect(name, violations, want): | ||
| got = len(violations) | ||
| if (got > 0) != want: | ||
| failures.append("%s: wanted %s, got %r" % (name, "a violation" if want else "clean", violations)) | ||
|
|
||
| sink = io.StringIO() | ||
|
|
||
| # An arm that never scavenged and is not registered is the #7255 defect. | ||
| expect( | ||
| "inert scavenge arm is rejected", | ||
| check_report( | ||
| _report([{"id": "default", "requires": "scavenge"}], [_cell("default", cycles=3)]), | ||
| {}, | ||
| sink, | ||
| ), | ||
| True, | ||
| ) | ||
| # ...and is accepted once registered, because the blocker is named. | ||
| expect( | ||
| "registered inert arm is accepted", | ||
| check_report( | ||
| _report([{"id": "default", "requires": "scavenge"}], [_cell("default", cycles=3)]), | ||
| {"default": ("#7161", "stopgap")}, | ||
| sink, | ||
| ), | ||
| False, | ||
| ) | ||
| # A registry entry that has outlived its cause must be loud, not silent. | ||
| expect( | ||
| "stale registry entry is rejected", | ||
| check_report( | ||
| _report([{"id": "default", "requires": "scavenge"}], [_cell("default", scavenged=17)]), | ||
| {"default": ("#7161", "stopgap")}, | ||
| sink, | ||
| ), | ||
| True, | ||
| ) | ||
| expect( | ||
| "live arm passes", | ||
| check_report( | ||
| _report([{"id": "evac", "requires": "scavenge"}], [_cell("evac", scavenged=17)]), | ||
| {}, | ||
| sink, | ||
| ), | ||
| False, | ||
| ) | ||
| # #7025: `move` may be satisfied by the C4b mark-sweep evacuation alone, but | ||
| # `scavenge` may NOT — that distinction is the whole point of two counters. | ||
| expect( | ||
| "move is satisfied by evacuated-only", | ||
| check_report( | ||
| _report([{"id": "m", "requires": "move"}], [_cell("m", evacuated=9)]), | ||
| {}, | ||
| sink, | ||
| ), | ||
| False, | ||
| ) | ||
| expect( | ||
| "scavenge is NOT satisfied by evacuated-only", | ||
| check_report( | ||
| _report([{"id": "s", "requires": "scavenge"}], [_cell("s", evacuated=9)]), | ||
| {}, | ||
| sink, | ||
| ), | ||
| True, | ||
| ) | ||
| expect( | ||
| "requires=none is never a violation", | ||
| check_report( | ||
| _report([{"id": "ctl", "requires": "none"}], [_cell("ctl")]), | ||
| {}, | ||
| sink, | ||
| ), | ||
| False, | ||
| ) | ||
| expect( | ||
| "collect arm needs a cycle", | ||
| check_report( | ||
| _report([{"id": "c", "requires": "collect"}], [_cell("c")]), | ||
| {}, | ||
| sink, | ||
| ), | ||
| True, | ||
| ) | ||
| expect( | ||
| "an unknown requires= value is rejected", | ||
| check_report( | ||
| _report([{"id": "x", "requires": "sometimes"}], [_cell("x")]), | ||
| {}, | ||
| sink, | ||
| ), | ||
| True, | ||
| ) | ||
| expect( | ||
| "an empty report is rejected", | ||
| check_report(_report([], []), {}, sink), | ||
| True, | ||
| ) | ||
| # A liveness verdict must be per-arm, not pooled: one live arm must not | ||
| # vouch for an inert one (the shape #7025 describes for counters). | ||
| expect( | ||
| "one live arm does not vouch for an inert one", | ||
| check_report( | ||
| _report( | ||
| [{"id": "live", "requires": "scavenge"}, {"id": "dead", "requires": "scavenge"}], | ||
| [_cell("live", scavenged=5), _cell("dead", cycles=5)], | ||
| ), | ||
| {}, | ||
| sink, | ||
| ), | ||
| True, | ||
| ) | ||
|
|
||
| for text, want_ok in [ | ||
| ("arm | #123 | reason", True), | ||
| ("arm | 123 | reason", False), # issue number must be an issue number | ||
| ("arm | #123", False), # three fields | ||
| ("arm | #123 | a\narm | #124 | b", False), # duplicate | ||
| ("arm | #123 |", False), # empty reason | ||
| ("# just a comment\n\n", True), | ||
| ]: | ||
| try: | ||
| parse_registry(text) | ||
| ok = True | ||
| except Violation: | ||
| ok = False | ||
| if ok != want_ok: | ||
| failures.append("registry parse of %r: wanted ok=%s" % (text, want_ok)) | ||
|
|
||
| # The parsers must survive the real file, or the registry check is vacuous — | ||
| # a regex that silently matches nothing is its own way for a gate to pass. | ||
| try: | ||
| text = MATRIX.read_text() | ||
| arms = matrix_arms(text) | ||
| pr_arms = matrix_pr_arms(text) | ||
| if len(arms) < 5 or arms.get("default") != "scavenge": | ||
| failures.append("matrix_arms parsed %r from the real matrix script" % (arms,)) | ||
| if "default" not in pr_arms: | ||
| failures.append("matrix_pr_arms parsed %r from the real matrix script" % (pr_arms,)) | ||
| if set(pr_arms) - set(arms): | ||
| failures.append("PR_ARMS names arms that are not in ARMS: %r" % (set(pr_arms) - set(arms),)) | ||
| except Violation as exc: | ||
| failures.append("parsing the real matrix script failed: %s" % exc) | ||
|
|
||
| for failure in failures: | ||
| print("SELF-TEST FAIL: %s" % failure, file=sys.stderr) | ||
| print("self-test: %d checks, %d failures" % (12 + 6 + 3, len(failures))) | ||
| return 1 if failures else 0 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The self-test check count is hand-maintained and already wrong.
Line 419 prints 12 + 6 + 3. self_test makes 11 expect calls, iterates 6 registry-parse cases, and makes 3 real-matrix assertions. The total is 20, not 21. A count that nobody derives is the defect class this file was written to remove. Count the checks as they run.
🐛 Proposed fix
failures = []
+ checks = 0
def expect(name, violations, want):
+ nonlocal checks
+ checks += 1
got = len(violations)
if (got > 0) != want:
failures.append("%s: wanted %s, got %r" % (name, "a violation" if want else "clean", violations)) except Violation:
ok = False
+ checks += 1
if ok != want_ok:
failures.append("registry parse of %r: wanted ok=%s" % (text, want_ok)) if set(pr_arms) - set(arms):
failures.append("PR_ARMS names arms that are not in ARMS: %r" % (set(pr_arms) - set(arms),))
+ checks += 3
except Violation as exc:
failures.append("parsing the real matrix script failed: %s" % exc)
for failure in failures:
print("SELF-TEST FAIL: %s" % failure, file=sys.stderr)
- print("self-test: %d checks, %d failures" % (12 + 6 + 3, len(failures)))
+ print("self-test: %d checks, %d failures" % (checks, len(failures)))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def self_test() -> int: | |
| import io | |
| failures = [] | |
| def expect(name, violations, want): | |
| got = len(violations) | |
| if (got > 0) != want: | |
| failures.append("%s: wanted %s, got %r" % (name, "a violation" if want else "clean", violations)) | |
| sink = io.StringIO() | |
| # An arm that never scavenged and is not registered is the #7255 defect. | |
| expect( | |
| "inert scavenge arm is rejected", | |
| check_report( | |
| _report([{"id": "default", "requires": "scavenge"}], [_cell("default", cycles=3)]), | |
| {}, | |
| sink, | |
| ), | |
| True, | |
| ) | |
| # ...and is accepted once registered, because the blocker is named. | |
| expect( | |
| "registered inert arm is accepted", | |
| check_report( | |
| _report([{"id": "default", "requires": "scavenge"}], [_cell("default", cycles=3)]), | |
| {"default": ("#7161", "stopgap")}, | |
| sink, | |
| ), | |
| False, | |
| ) | |
| # A registry entry that has outlived its cause must be loud, not silent. | |
| expect( | |
| "stale registry entry is rejected", | |
| check_report( | |
| _report([{"id": "default", "requires": "scavenge"}], [_cell("default", scavenged=17)]), | |
| {"default": ("#7161", "stopgap")}, | |
| sink, | |
| ), | |
| True, | |
| ) | |
| expect( | |
| "live arm passes", | |
| check_report( | |
| _report([{"id": "evac", "requires": "scavenge"}], [_cell("evac", scavenged=17)]), | |
| {}, | |
| sink, | |
| ), | |
| False, | |
| ) | |
| # #7025: `move` may be satisfied by the C4b mark-sweep evacuation alone, but | |
| # `scavenge` may NOT — that distinction is the whole point of two counters. | |
| expect( | |
| "move is satisfied by evacuated-only", | |
| check_report( | |
| _report([{"id": "m", "requires": "move"}], [_cell("m", evacuated=9)]), | |
| {}, | |
| sink, | |
| ), | |
| False, | |
| ) | |
| expect( | |
| "scavenge is NOT satisfied by evacuated-only", | |
| check_report( | |
| _report([{"id": "s", "requires": "scavenge"}], [_cell("s", evacuated=9)]), | |
| {}, | |
| sink, | |
| ), | |
| True, | |
| ) | |
| expect( | |
| "requires=none is never a violation", | |
| check_report( | |
| _report([{"id": "ctl", "requires": "none"}], [_cell("ctl")]), | |
| {}, | |
| sink, | |
| ), | |
| False, | |
| ) | |
| expect( | |
| "collect arm needs a cycle", | |
| check_report( | |
| _report([{"id": "c", "requires": "collect"}], [_cell("c")]), | |
| {}, | |
| sink, | |
| ), | |
| True, | |
| ) | |
| expect( | |
| "an unknown requires= value is rejected", | |
| check_report( | |
| _report([{"id": "x", "requires": "sometimes"}], [_cell("x")]), | |
| {}, | |
| sink, | |
| ), | |
| True, | |
| ) | |
| expect( | |
| "an empty report is rejected", | |
| check_report(_report([], []), {}, sink), | |
| True, | |
| ) | |
| # A liveness verdict must be per-arm, not pooled: one live arm must not | |
| # vouch for an inert one (the shape #7025 describes for counters). | |
| expect( | |
| "one live arm does not vouch for an inert one", | |
| check_report( | |
| _report( | |
| [{"id": "live", "requires": "scavenge"}, {"id": "dead", "requires": "scavenge"}], | |
| [_cell("live", scavenged=5), _cell("dead", cycles=5)], | |
| ), | |
| {}, | |
| sink, | |
| ), | |
| True, | |
| ) | |
| for text, want_ok in [ | |
| ("arm | #123 | reason", True), | |
| ("arm | 123 | reason", False), # issue number must be an issue number | |
| ("arm | #123", False), # three fields | |
| ("arm | #123 | a\narm | #124 | b", False), # duplicate | |
| ("arm | #123 |", False), # empty reason | |
| ("# just a comment\n\n", True), | |
| ]: | |
| try: | |
| parse_registry(text) | |
| ok = True | |
| except Violation: | |
| ok = False | |
| if ok != want_ok: | |
| failures.append("registry parse of %r: wanted ok=%s" % (text, want_ok)) | |
| # The parsers must survive the real file, or the registry check is vacuous — | |
| # a regex that silently matches nothing is its own way for a gate to pass. | |
| try: | |
| text = MATRIX.read_text() | |
| arms = matrix_arms(text) | |
| pr_arms = matrix_pr_arms(text) | |
| if len(arms) < 5 or arms.get("default") != "scavenge": | |
| failures.append("matrix_arms parsed %r from the real matrix script" % (arms,)) | |
| if "default" not in pr_arms: | |
| failures.append("matrix_pr_arms parsed %r from the real matrix script" % (pr_arms,)) | |
| if set(pr_arms) - set(arms): | |
| failures.append("PR_ARMS names arms that are not in ARMS: %r" % (set(pr_arms) - set(arms),)) | |
| except Violation as exc: | |
| failures.append("parsing the real matrix script failed: %s" % exc) | |
| for failure in failures: | |
| print("SELF-TEST FAIL: %s" % failure, file=sys.stderr) | |
| print("self-test: %d checks, %d failures" % (12 + 6 + 3, len(failures))) | |
| return 1 if failures else 0 | |
| def self_test() -> int: | |
| import io | |
| failures = [] | |
| checks = 0 | |
| def expect(name, violations, want): | |
| nonlocal checks | |
| checks += 1 | |
| got = len(violations) | |
| if (got > 0) != want: | |
| failures.append("%s: wanted %s, got %r" % (name, "a violation" if want else "clean", violations)) | |
| sink = io.StringIO() | |
| # An arm that never scavenged and is not registered is the `#7255` defect. | |
| expect( | |
| "inert scavenge arm is rejected", | |
| check_report( | |
| _report([{"id": "default", "requires": "scavenge"}], [_cell("default", cycles=3)]), | |
| {}, | |
| sink, | |
| ), | |
| True, | |
| ) | |
| # ...and is accepted once registered, because the blocker is named. | |
| expect( | |
| "registered inert arm is accepted", | |
| check_report( | |
| _report([{"id": "default", "requires": "scavenge"}], [_cell("default", cycles=3)]), | |
| {"default": ("`#7161`", "stopgap")}, | |
| sink, | |
| ), | |
| False, | |
| ) | |
| # A registry entry that has outlived its cause must be loud, not silent. | |
| expect( | |
| "stale registry entry is rejected", | |
| check_report( | |
| _report([{"id": "default", "requires": "scavenge"}], [_cell("default", scavenged=17)]), | |
| {"default": ("`#7161`", "stopgap")}, | |
| sink, | |
| ), | |
| True, | |
| ) | |
| expect( | |
| "live arm passes", | |
| check_report( | |
| _report([{"id": "evac", "requires": "scavenge"}], [_cell("evac", scavenged=17)]), | |
| {}, | |
| sink, | |
| ), | |
| False, | |
| ) | |
| # `#7025`: `move` may be satisfied by the C4b mark-sweep evacuation alone, but | |
| # `scavenge` may NOT — that distinction is the whole point of two counters. | |
| expect( | |
| "move is satisfied by evacuated-only", | |
| check_report( | |
| _report([{"id": "m", "requires": "move"}], [_cell("m", evacuated=9)]), | |
| {}, | |
| sink, | |
| ), | |
| False, | |
| ) | |
| expect( | |
| "scavenge is NOT satisfied by evacuated-only", | |
| check_report( | |
| _report([{"id": "s", "requires": "scavenge"}], [_cell("s", evacuated=9)]), | |
| {}, | |
| sink, | |
| ), | |
| True, | |
| ) | |
| expect( | |
| "requires=none is never a violation", | |
| check_report( | |
| _report([{"id": "ctl", "requires": "none"}], [_cell("ctl")]), | |
| {}, | |
| sink, | |
| ), | |
| False, | |
| ) | |
| expect( | |
| "collect arm needs a cycle", | |
| check_report( | |
| _report([{"id": "c", "requires": "collect"}], [_cell("c")]), | |
| {}, | |
| sink, | |
| ), | |
| True, | |
| ) | |
| expect( | |
| "an unknown requires= value is rejected", | |
| check_report( | |
| _report([{"id": "x", "requires": "sometimes"}], [_cell("x")]), | |
| {}, | |
| sink, | |
| ), | |
| True, | |
| ) | |
| expect( | |
| "an empty report is rejected", | |
| check_report(_report([], []), {}, sink), | |
| True, | |
| ) | |
| # A liveness verdict must be per-arm, not pooled: one live arm must not | |
| # vouch for an inert one (the shape `#7025` describes for counters). | |
| expect( | |
| "one live arm does not vouch for an inert one", | |
| check_report( | |
| _report( | |
| [{"id": "live", "requires": "scavenge"}, {"id": "dead", "requires": "scavenge"}], | |
| [_cell("live", scavenged=5), _cell("dead", cycles=5)], | |
| ), | |
| {}, | |
| sink, | |
| ), | |
| True, | |
| ) | |
| for text, want_ok in [ | |
| ("arm | `#123` | reason", True), | |
| ("arm | 123 | reason", False), # issue number must be an issue number | |
| ("arm | `#123`", False), # three fields | |
| ("arm | `#123` | a\narm | `#124` | b", False), # duplicate | |
| ("arm | `#123` |", False), # empty reason | |
| ("# just a comment\n\n", True), | |
| ]: | |
| try: | |
| parse_registry(text) | |
| ok = True | |
| except Violation: | |
| ok = False | |
| checks += 1 | |
| if ok != want_ok: | |
| failures.append("registry parse of %r: wanted ok=%s" % (text, want_ok)) | |
| # The parsers must survive the real file, or the registry check is vacuous — | |
| # a regex that silently matches nothing is its own way for a gate to pass. | |
| try: | |
| text = MATRIX.read_text() | |
| arms = matrix_arms(text) | |
| pr_arms = matrix_pr_arms(text) | |
| if len(arms) < 5 or arms.get("default") != "scavenge": | |
| failures.append("matrix_arms parsed %r from the real matrix script" % (arms,)) | |
| if "default" not in pr_arms: | |
| failures.append("matrix_pr_arms parsed %r from the real matrix script" % (pr_arms,)) | |
| if set(pr_arms) - set(arms): | |
| failures.append("PR_ARMS names arms that are not in ARMS: %r" % (set(pr_arms) - set(arms),)) | |
| checks += 3 | |
| except Violation as exc: | |
| failures.append("parsing the real matrix script failed: %s" % exc) | |
| for failure in failures: | |
| print("SELF-TEST FAIL: %s" % failure, file=sys.stderr) | |
| print("self-test: %d checks, %d failures" % (checks, len(failures))) | |
| return 1 if failures else 0 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/gc_matrix_liveness_check.py` around lines 268 - 420, Update
self_test’s final check-count reporting to derive the total from the checks
actually executed, including the expect calls, registry-parse cases, and
real-matrix assertions, instead of the hand-maintained “12 + 6 + 3” expression.
Preserve the existing failure counting and output format while ensuring the
reported total is 20 for the current checks.
Comment on lines
+101
to
+105
| # Local exploration only (e.g. a `--filter` narrow enough that an arm | ||
| # legitimately has nothing to bite). CI never passes this: the whole | ||
| # point of #7255 is that an inert arm must be able to turn a run red. | ||
| --liveness-report-only) LIVENESS_REPORT_ONLY=1; shift ;; | ||
| -h|--help) sed -n '1,70p' "$0"; exit 0 ;; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
--help truncates the usage block at line 70, so the new flag is not shown.
The usage block now ends at line 73. sed -n '1,70p' stops before the --json, --list-arms, and --liveness-report-only lines. A user who runs --help cannot see the flag this PR adds.
Prefer a range that is not a hard-coded count, or extend it past the usage block.
🐛 Proposed fix
- -h|--help) sed -n '1,70p' "$0"; exit 0 ;;
+ -h|--help) sed -n '/^# Usage:/,/^$/p' "$0"; exit 0 ;;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Local exploration only (e.g. a `--filter` narrow enough that an arm | |
| # legitimately has nothing to bite). CI never passes this: the whole | |
| # point of #7255 is that an inert arm must be able to turn a run red. | |
| --liveness-report-only) LIVENESS_REPORT_ONLY=1; shift ;; | |
| -h|--help) sed -n '1,70p' "$0"; exit 0 ;; | |
| # Local exploration only (e.g. a `--filter` narrow enough that an arm | |
| # legitimately has nothing to bite). CI never passes this: the whole | |
| # point of `#7255` is that an inert arm must be able to turn a run red. | |
| --liveness-report-only) LIVENESS_REPORT_ONLY=1; shift ;; | |
| -h|--help) sed -n '/^# Usage:/,/^$/p' "$0"; exit 0 ;; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/gc_repsel_matrix.sh` around lines 101 - 105, Update the --help
handler in scripts/gc_repsel_matrix.sh to display the complete usage block,
including --json, --list-arms, and --liveness-report-only. Replace the
hard-coded sed range in the option handling with a range that reaches the usage
block’s actual end without truncation.
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.
Fixes #7255.
The defect
scripts/gc_repsel_matrix.shreports an inert arm's cell asUNVER— "byte-exactagainst the oracle, but the arm did not bite" — and
UNVERis deliberately notgreen. It was never red either: the script's exit status counts only
FAIL. Anarm that goes inert across the whole corpus therefore produces a yellow table
and exit 0, and no CI job says a word. CLAUDE.md's fourth hazard exactly: the
gate runs, and its subject never does.
Measured at
a25077491,--arms all --pressure 8, 50 corpus files, node 26.5.1,macOS arm64, release — the run's own liveness summary:
defaultverify_evaccons_scan_offcons_scan_off_forceshipped_defaultevac_minor,force_evac,force_verify,loop_polls, all sevenrep_*_offgen_gc_off,wb_off,gen_off_verify,wb_off_force,all_fourPASS=688 UNVER=341 XFAIL=21 FAIL=0over 1050 cells — and exit 0. Four of thesix arms
gc-stressruns on a pull request could only ever produceUNVER.Root cause
Not the
--pressure/defer-cap interaction of #7024 — that fix is intact.#7161, which flipped
PERRY_GC_MOVING_LOOP_POLLSdefault-OFF as a stopgapfor #7154. That one env gates both halves of the route #7019/#7024 opened:
perry-codegen/src/stmt/loops.rs::moving_safepoint_polls_enableddecideswhether
js_gc_loop_safepoint()back-edge polls are emitted at all, so adefault binary contains none;
perry-runtime/src/gc/policy.rs::gc_moving_loop_polls_enableddecides whetherthe alloc-point nursery trigger defers to them, so nothing is ever deferred.
With it off,
gc_check_triggerfalls through to the direct minor behindManualGcScanGuard::force_full_scan(), which makes the copying minor ineligible(
fallback=conservative_stack). That isdefaultandverify_evac.cons_scan_offandcons_scan_off_forceare dark for a second, independentreason that #7024's deferral had been papering over: with the incremental
stepper at its default,
registered_root_scanners_block_budgeted_gc()reduces to"any copy-only scanner", a compiled program has none, so the nursery trigger
never reaches the direct arm at all and is handed to the budgeted stepper, which
is non-moving by construction.
Ruled out by measurement, not by argument: #7250 (lazy remembered-set
arming) is not implicated —
evac_minor/force_evac/force_verify/loop_pollsand all seven
rep_*_offarms relocate 49/50 on this same build, so writebarriers are active and evacuation is reachable. #7249's
GcSuppressScopebootstrap window is likewise not implicated for the same reason.
#7154 is closed, but its final comment says the stopgap stays until the
from-space protector is clean, so reverting #7161 is not on the table here.
What changed
1. A relocating arm is back in the PR subset. New arm
safepoint_minor:PERRY_GC_MOVING_LOOP_POLLS=1at compile and run time, and nothing else — noPERRY_GC_INCREMENTAL=0, noPERRY_CONSERVATIVE_STACK_SCAN=off, no forcedevacuation. That is the sound route (the copying minor runs at a loop back-edge
where every live heap value is a named local on the shadow stack), and it is
exactly what
defaultwas between #7024 and #7161. It does not replace theallocation-point arms: per #7249 a back-edge poll only fires while user JS runs,
so
evac_minorandforce_verifystay in the subset as the stronger acceptanceroute.
2. The declarations stay honest instead of being weakened.
default,verify_evac,cons_scan_offandcons_scan_off_forcekeeprequires=scavenge— that is still what they are for — and are listed in thenew
test-parity/gc_matrix_inert_arms.txt, each naming #7161 and saying whatmust become true for the entry to be deleted. Downgrading them to
requires=collectwould have made the table green while deleting the record.3. The liveness gate.
scripts/gc_matrix_liveness_check.pyruns at the end ofevery matrix invocation and fails it when
requires=on zero cells and is not registered, oris stale. A registry that can only be added to rots exactly the way the header
did.
It also carries a static check, run from
lintwith no build:PR_ARMSmustcontain at least one arm that claims to relocate and is not on the known-inert
list. Registering every inert arm is therefore not a way to buy a green subset.
--self-test(21 checks) and--check-registryare both wired intolint, sothe rule that decides red-versus-green is a tested program rather than untested
bash, and
--check-registryfails if the matrix ever stops invoking thechecker — hazard 4 one level up.
4. The header. The
default: 0/22 -> 12/22table is gone, along with theother hand-maintained counts, and replaced by an instruction not to put liveness
numbers there again. Every run prints its own derived per-arm table.
Verification
All on this branch's merge-base build, release profile,
-p perry -p perry-runtime -p perry-stdlib -p perry-runtime-static -p perry-stdlib-static,node 26.5.1 (the pin), macOS arm64.
Per-arm
copy-minor, before → after (--arms pr --pressure 8):defaultsafepoint_minorevac_minorverify_evacforce_verifycons_scan_offshipped_defaultPASS=177 UNVER=169 XFAIL=4 FAIL=0over 350 cells, liveness gate exit 0. Thefour XFAILs are the pre-existing #7247/#7248 entries;
safepoint_minorisPASS on both of those files, consistent with the triage notes that call them
clean on the safepoint route.
Re-confirmed after rebasing onto
61410a222(corpus is 52 files there), fullrebuild,
--arms pr --pressure 8:Red, then green — four separate demonstrations:
--arms allrun above endsin the gate naming
default,verify_evac,cons_scan_offandcons_scan_off_force, and exits 1.--arms force_evac --filter test_gap_specabi_reassign --pressure 0→ the armrelocates nothing, the gate fires, script exit 1. Same command with
--liveness-report-only→ exit 0 with the violation still printed.safepoint_minorto the registry and re-runningthe checker over the real after-report → exit 1, "listed as known-inert … but
satisfied requires=scavenge on 33/50 cells".
PR_ARMSto only inert/non-relocating arms →--check-registryexits 1 fromlint, before anything is built.The gate is ARMED, not report-only, and that does not create a new permanent
red: every currently-inert arm is registered, so the same
--arms alltable thatwent red without the registry goes green with it (verified by running the checker
over the identical report both ways). The escape hatch (
--liveness-report-only)exists for local
--filterwork and is used nowhere in CI. If a Linux runnerturns out to disagree with this host about one arm, the fix is a line in
test-parity/gc_matrix_inert_arms.txt, not a triage debate.Two blocked issues become judgeable
#7018 — judgeable, and it does not reproduce. Its exact repro
(
PERRY_GC_HEAP_LIMIT=8 PERRY_GC_INCREMENTAL=0 PERRY_CONSERVATIVE_STACK_SCAN=off PERRY_GC_TRACE=1ontest_gap_repsel_scalar_replaced_locals) is the matrix'sevac_minorcell for that file, which is live: 1 577 269 objects copied.10/10 runs are byte-exact with rc=0 and 61 GC cycles each with the tracer on;
10/10 without the tracer as a control.
#6982 — still not safely judgeable, and now we know why. Its stated
configuration (
PERRY_GC_HEAP_LIMIT=8 PERRY_GC_INCREMENTAL=0, conservative scanON) is intermittently live: over 10 runs of
test_gap_repsel_gc_stressacopying minor ran in one run (141 711 objects) and in none of the other nine;
over a separate 20 runs on the rebased build, one copying minor total. So a
10-run green there is really a 1-run green. No matrix arm carries that
configuration, and adding one would be a flaky gate, which is the one thing #7255
says not to build. The neighbouring deterministic arm (
evac_minor, which addsCONSERVATIVE_STACK_SCAN=off) is 10/10 clean on that file with 10 copying minorsand 1 385 325 objects copied every run.
Not verified here
safepoint_minorshould eventually replacedefault— that is fix(gc): disable evacuating minor by default pending #7154 (use-after-free on dynamically-added fields) #7161'srevert decision, and this PR deliberately does not touch the collector.
gc_instrument_smoke.sh,gc-ratchet) — untouched.Summary by CodeRabbit
CI and Testing
Documentation