fix(gc): parse PERRY_GC_DIAG by value — =0 no longer ENABLES diagnostics - #7993
Merged
Conversation
…stics `gc_diag_enabled()` read the knob with `var_os(..).is_some()` — presence, not value — so `PERRY_GC_DIAG=0` turned diagnostics ON, and so did `off`, `false` and the empty string. That is a measurement-integrity bug: during #7803 triage it silently collapsed an A/B arm, because the "diagnostics off" control arm was instrumented exactly like the arm it controlled for. It fails toward a confident wrong answer, not a visible error. The audit that came with it found the same shape on 25 read sites across four GC knobs (DIAG x20, VERIFY_MARK x3, VERIFY_RS_NONFATAL x1, and VERIFY_EVACUATION, which was *split-brain*: value-parsed in gc/mod.rs and presence-parsed in the barrier's ever-dirty tracker, so `=0` switched the verifier off while leaving its side table populated on every barrier). There are now exactly two boolean vocabularies, both pure functions of the raw value so both directions are testable without touching the process environment: * `gc::env_flag_from_value` — default-OFF (#5093): `1`/`true`/`on`/`yes`; unset, the off-spellings, empty and anything UNRECOGNISED read OFF. * `gc::env_default_on_from_value` — default-ON kill switch: OFF only on an explicit `0`/`off`/`false`/`no`; unrecognised leaves the shipping default ON. They are deliberately not each other's negation — each fails toward its own documented default — and that asymmetry has its own assertion so a future tidy-up cannot collapse one into the other. Also unified onto them: GC_TRACE, GC_VERIFY_CLASSIFIER, GC_FORCE_EVACUATE, GEN_GC, WRITE_BARRIERS, GC_MOVING_SAFEPOINT, GC_INCREMENTAL, and GC_SAFEPOINT_ONLY's boolean arm. `PERRY_SHAPE_LAYOUT_KEYED` was `v != "0"`, so its documented off-state worked only for the literal `0` — `=off` and `=false` read as ON. Teeth, so this class cannot come back: * `gc/tests/env_knob_parse.rs` — both vocabularies over on/off/unrecognised spellings, plus the decisive arm: `gc_diag_enabled()` observed in a CHILD PROCESS under a real `PERRY_GC_DIAG=0`/`off`/``/`1`. The pure cases alone would stay green if that one line reverted to presence-parsing; only the live cached reader under a real environment closes it, and the ON arm is there so a fix that hard-wires `false` cannot pass either. * `scripts/check_gc_env_knobs.py` (already in `lint`) now rejects the presence-only shape outright for the GC family. The exemption list is empty and stale entries fail, so a fix must delete its own licence. Its `--self-test` sabotages the detector with the exact shape that shipped and requires it to be distinguished from the value-parsed replacement. Closes #7991.
📝 WalkthroughWalkthroughGC environment flags now use shared value-based parsers. Diagnostic and verification readers use centralized cached predicates. Tests cover parser semantics and live readers. A checker rejects presence-only GC parsing. ChangesGC environment-knob parsing
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
proggeramlug
marked this pull request as ready for review
August 12, 2026 20:54
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/perry-runtime/src/gc/mod.rs`:
- Around line 1264-1277: Update policy.rs::moving_loop_polls_enabled_from_env to
parse its environment value through super::env_default_on_from_value(value),
preserving the shared case-insensitive and whitespace-tolerant vocabulary. Add
PERRY_GC_MOVING_LOOP_POLLS coverage to the parser tests, including recognized
false values and unrecognized input.
🪄 Autofix
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: c35d29c7-5f02-4402-8ddc-83810afbf4b0
📒 Files selected for processing (20)
changelog.d/7993-gc-diag-knob-value-parse.mdcrates/perry-runtime/src/arena/quarantine.rscrates/perry-runtime/src/arena/reset.rscrates/perry-runtime/src/gc/barrier/mod.rscrates/perry-runtime/src/gc/copying.rscrates/perry-runtime/src/gc/cycle.rscrates/perry-runtime/src/gc/layout.rscrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/gc/oldgen.rscrates/perry-runtime/src/gc/policy.rscrates/perry-runtime/src/gc/scan_fallback.rscrates/perry-runtime/src/gc/scanner_profile.rscrates/perry-runtime/src/gc/telemetry.rscrates/perry-runtime/src/gc/tenuring.rscrates/perry-runtime/src/gc/tests/env_knob_parse.rscrates/perry-runtime/src/gc/tests/mod.rscrates/perry-runtime/src/gc/tests/schedule.rscrates/perry-runtime/src/gc/trace.rscrates/perry-runtime/src/gc/verify.rsscripts/check_gc_env_knobs.py
Comment on lines
+1264
to
+1277
| /// #5093 semantics as a **pure** function of the raw value, so both directions | ||
| /// can be pinned by a test without touching the process environment (the live | ||
| /// readers cache in a `OnceLock`; a test that called `set_var` would be at the | ||
| /// mercy of which test ran first, and `set_var` is process-wide — see the | ||
| /// `knob_overrides` note above for what that cost us once already). | ||
| /// | ||
| /// True for `1`/`true`/`on`/`yes` (case-insensitive, surrounding whitespace | ||
| /// ignored). False for unset, `0`/`false`/`off`/`no`, the empty string, **and | ||
| /// anything unrecognised** — a typo must not silently arm an instrument. | ||
| /// | ||
| /// #7991: this is the single definition of "boolean-ish GC knob". Every GC knob | ||
| /// that is a boolean must route through it. `scripts/check_gc_env_knobs.py` | ||
| /// enforces that by rejecting presence-only reads (`var_os(..).is_some()`) of | ||
| /// GC-family names in production code. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the shared parser for every boolean GC knob.
The new contract says every boolean GC knob must use the shared value parser. crates/perry-runtime/src/gc/policy.rs::moving_loop_polls_enabled_from_env still uses an exact-case matcher. Values such as PERRY_GC_MOVING_LOOP_POLLS="OFF" or " false " therefore remain enabled.
Route that helper through super::env_default_on_from_value(value) and add the knob to the parser tests.
Keep the reader vocabulary consistent with the shared parser contract introduced here.
Suggested alignment
pub(super) fn moving_loop_polls_enabled_from_env(value: Option<&str>) -> bool {
- !matches!(value, Some("0") | Some("off") | Some("false"))
+ super::env_default_on_from_value(value)
}🤖 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 `@crates/perry-runtime/src/gc/mod.rs` around lines 1264 - 1277, Update
policy.rs::moving_loop_polls_enabled_from_env to parse its environment value
through super::env_default_on_from_value(value), preserving the shared
case-insensitive and whitespace-tolerant vocabulary. Add
PERRY_GC_MOVING_LOOP_POLLS coverage to the parser tests, including recognized
false values and unrecognized input.
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.
Closes #7991.
The bug
gc_diag_enabled()read its knob withvar_os(..).is_some()— presence, not value — soPERRY_GC_DIAG=0turned diagnostics on. So didoff,false, and the empty string.It is not cosmetic. During #7803 triage it silently collapsed an A/B arm: the investigator disabled diagnostics for the clean arm and got them in both. That fails toward a confident wrong answer, which is worse than failing loudly, and it was inconsistent with its immediate neighbour —
PERRY_GC_PROTECT_FROMSPACEhas parsed its value properly all along, so=0really was off there.The audit (the ★ ask)
The same shape was on 25 read sites across four knobs:
PERRY_GC_DIAGis_some, 2is_none)PERRY_GC_VERIFY_MARK=0armed a whole-heap verifierPERRY_GC_VERIFY_RS_NONFATAL=0demoted a fatal check to a warningPERRY_GC_VERIFY_EVACUATIONgc/mod.rs, presence-parsed in the barrier's ever-dirty tracker, so=0switched the verifier off while leaving its side table populated on every barrierOne adjacent non-GC-family find:
PERRY_SHAPE_LAYOUT_KEYEDwasv != "0", so its documented off-state worked only for the literal0—=off/=falseread as ON.The fix
Exactly two boolean vocabularies now exist, both pure functions of the raw value so both directions are testable without touching the process environment:
gc::env_flag_from_value— default-OFF (perf(method dispatch): method_calls ~290× Node — remaining cost is per-field-access shape-guard calls (plan + standby) #5093):1/true/on/yes; unset, off-spellings, empty and anything unrecognised read OFF.gc::env_default_on_from_value— default-ON kill switch: OFF only on an explicit0/off/false/no; unrecognised leaves the shipping default ON.They are deliberately not each other's negation — each fails toward its own documented default — and that asymmetry has its own assertion so a future tidy-up cannot collapse one into the other.
Also unified onto them:
GC_TRACE,GC_VERIFY_CLASSIFIER,GC_FORCE_EVACUATE,GEN_GC,WRITE_BARRIERS,GC_MOVING_SAFEPOINT,GC_INCREMENTAL,SHAPE_LAYOUT_KEYED, andGC_SAFEPOINT_ONLY's boolean arm (strictstays its own third state).Teeth
1.
gc/tests/env_knob_parse.rs. The pure cases pin the vocabulary — but they would all stay green if that one line reverted to presence-parsing. So the decisive case observes the live cached reader in a child process under a realPERRY_GC_DIAG=0/off/ `` /1. The ON arm is there so a fix that hard-wires `false` cannot pass either: a liveness counter satisfiable by two paths is a presence check, not a proof.2.
scripts/check_gc_env_knobs.py(already inlint) now rejects the presence-only shape outright for the GC family.PRESENCE_ONLY_ALLOWEDis empty, and a stale entry also fails, so a fix must delete its own licence.--self-testsabotages the detector with the exact shape that shipped and requires it to distinguish that from the value-parsed replacement.Sabotage-verified
With the fix committed,
telemetry.rswas reverted tovar_os(..).is_some()in place and both teeth fired:then restored and rebuilt (not merely
git checkout-ed) to re-confirm green.Blast radius
Diagnostic-only by contract; no program semantics change. Every in-repo use of these knobs is
=1, so nothing depended on the old behaviour. The damage was to investigations: any prior A/B that usedPERRY_GC_DIAG=0as its control arm was not controlled.Summary by CodeRabbit
0,off,false, andnoreliably disable diagnostics and verification.