fix(runtime): tombstone squeeze published stale facts — un-aborts the runtime suite (#9108) - #9110
Conversation
…d publish (PerryTS#9108) The tombstone squeeze (default-on since PerryTS#9038) shrank the keys array in place and then called set_object_live_slot_count BEFORE republishing the shape. When the floored bound is unchanged — the at-scale/overflow case — that call's early return asserts parity against the STILL-STAMPED pre-squeeze descriptor and SIGABRTs the debug suite (reserved_floor at-scale test), masking the ~1000 tests behind it on every branch. Two coherence fixes: - squeeze_holes_and_delete orders shape_drop -> publish_object_shape_holes -> set_object_live_slot_count, so the bound publish always sees the squeezed descriptor; - publish_object_shape_holes reads logical_key_count from the ARRAY, not the lineage: identical for the O(1) hole delete (length untouched), and the only correct source right after a squeeze changed the length. Release binaries were unaffected (debug_assert only), which is why the differential battery stayed byte-identical while the test suite died. Full perry-runtime suite now runs to completion: 2819 passed / 0 failed. Claude-Session: https://claude.ai/code/session_01Ay8VyLkKbm8Hkc1xmvTEsP
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe runtime changes the bookkeeping order after hole squeezing. It publishes the squeezed shape before updating the live-slot bound. Shape publication now derives the logical key count from the current keys array. ChangesShape squeeze bookkeeping
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized runtime bookkeeping fix prevents stale shape metadata from causing debug-suite aborts while preserving existing deletion behavior; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the regression, identifies both coherence fixes, references issue Full details: Linked Issues checkExplanation The changes address issue
✨ 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 |
|
Merged. Fixes #9108. This one deserves a note on how nearly I missed it, because the failure mode you documented in the issue caught me exactly as described. The masking is worse than the abort. Under the dev profile on main (
767 tests never ran on main. And the — the re-exec child, exactly the masquerade your issue warns about. My own validation harness greps Second reason I initially could not reproduce it, which is worth recording for anyone else chasing this: The fix reads correctly on both halves. Ordering One adjacent finding, filed separately as #9115: Validation: runtime 2819 passed under both profiles with exit 0 (dev profile: 53.9 s, all 2836 test lines), codegen 1347, perry --bins 1066, fmt clean, |
…n identity — pi boot threw Cyclic __proto__
perry-compiled pi (13MB esbuild bundle) died at startup with
`TypeError: Cyclic __proto__ value` out of `js_object_set_prototype_of`,
with obj_bits == proto_bits exactly — the two ARGUMENTS were the same
pointer — while the chain behind the proto was healthy. None of the
bundle's 17 textual `Object.setPrototypeOf(` sites fired a JS logging
shim, because the self-set was manufactured upstream of the call: the
closure-literal singleton caches handed back ONE ClosureHeader for two
evaluations of the same function literal, so `setPrototypeOf(wrapped,
original)` (a graceful-fs-style wrap pattern) received one object twice
and correctly refused the "cycle".
Mechanism: `expr/closure.rs` routed closure literals through
`js_closure_alloc_singleton` (captureless arrows) and
`js_closure_alloc_with_captures_singleton` (arrows with captures, and
non-arrow literals whose captures are all boxes) keyed by
(func_ptr, capture bits). Two evaluations of the same literal with
bit-identical captures — e.g. an arrow capturing the same constant, or
any captureless arrow — came back `===`-equal. ECMA-262
OrdinaryFunctionCreate requires a fresh object per evaluation, and the
distinction is observable through `===`, expando properties, WeakMap
keys, addEventListener de-duplication, and `Object.setPrototypeOf`.
Minimal repros (byte-compared against node before/after):
function mk() { return () => K; } // captured arrow
const a = mk(), b = mk(); // perry: a === b (node: false)
Object.setPrototypeOf(a, b); // perry threw Cyclic __proto__
and the same with `() => 1` (captureless). Both now match node.
Fix: gate every closure.rs literal singleton path on
`is_plain_async_step_body` — the file's existing detector for the
compiler-synthesized plain-async step closures (their terminal
`Stmt::ReleaseBoxes` arms cannot appear in user code). Those are the
closures the caches were built for (PerryTS#8269's parallel async-await
pattern re-creates them per resume with the same per-activation box
captures, and their identity never escapes the promise machinery), and
they keep the fast path. Every user-authored arrow and function
expression now mints a fresh closure. Runtime-internal singleton users
(function-declaration references, property_get/i18n/arrays wrapper
thunks) are separate paths and unchanged. A genuine
`setPrototypeOf(x, x)` still throws — the cycle check is untouched.
Perf note: this deliberately gives back the user-arrow closure reuse
from the PerryTS#8269/PerryTS#8291 captured-singleton extension (e.g. ECS
`World.executeEntityCommands`' per-call inner arrow) and the captureless
user-arrow singleton at literal sites; a sound replacement needs
escape-aware caching rather than identity-violating sharing.
Validation: repros above and test-files/
test_gap_9090_closure_literal_identity.ts byte-identical to node;
`cargo test -p perry-runtime --lib -- --test-threads=1` green — 2813
passed, 0 failed with `--skip reserved_floor` (that module's at-scale
tests SIGABRT on this pre-PerryTS#9110 base; known PerryTS#9108/PerryTS#9110, unrelated);
`cargo test -p perry-codegen`: 283+75 passed after updating the four
native_proof_regressions pins from `js_closure_alloc_singleton` to
`js_closure_alloc` (their real subject — the alloc storing the public
wrapper pointer — is preserved); one pre-existing env-leak flake
(`packed_f64_loop_unary_math_store_versions_with_side_exit`) passes in
isolation.
Claude-Session: https://claude.ai/code/session_01Ay8VyLkKbm8Hkc1xmvTEsP
…ity — pi boots (#9128) * fix(runtime): closure-literal singleton caches conflated user function identity — pi boot threw Cyclic __proto__ perry-compiled pi (13MB esbuild bundle) died at startup with `TypeError: Cyclic __proto__ value` out of `js_object_set_prototype_of`, with obj_bits == proto_bits exactly — the two ARGUMENTS were the same pointer — while the chain behind the proto was healthy. None of the bundle's 17 textual `Object.setPrototypeOf(` sites fired a JS logging shim, because the self-set was manufactured upstream of the call: the closure-literal singleton caches handed back ONE ClosureHeader for two evaluations of the same function literal, so `setPrototypeOf(wrapped, original)` (a graceful-fs-style wrap pattern) received one object twice and correctly refused the "cycle". Mechanism: `expr/closure.rs` routed closure literals through `js_closure_alloc_singleton` (captureless arrows) and `js_closure_alloc_with_captures_singleton` (arrows with captures, and non-arrow literals whose captures are all boxes) keyed by (func_ptr, capture bits). Two evaluations of the same literal with bit-identical captures — e.g. an arrow capturing the same constant, or any captureless arrow — came back `===`-equal. ECMA-262 OrdinaryFunctionCreate requires a fresh object per evaluation, and the distinction is observable through `===`, expando properties, WeakMap keys, addEventListener de-duplication, and `Object.setPrototypeOf`. Minimal repros (byte-compared against node before/after): function mk() { return () => K; } // captured arrow const a = mk(), b = mk(); // perry: a === b (node: false) Object.setPrototypeOf(a, b); // perry threw Cyclic __proto__ and the same with `() => 1` (captureless). Both now match node. Fix: gate every closure.rs literal singleton path on `is_plain_async_step_body` — the file's existing detector for the compiler-synthesized plain-async step closures (their terminal `Stmt::ReleaseBoxes` arms cannot appear in user code). Those are the closures the caches were built for (#8269's parallel async-await pattern re-creates them per resume with the same per-activation box captures, and their identity never escapes the promise machinery), and they keep the fast path. Every user-authored arrow and function expression now mints a fresh closure. Runtime-internal singleton users (function-declaration references, property_get/i18n/arrays wrapper thunks) are separate paths and unchanged. A genuine `setPrototypeOf(x, x)` still throws — the cycle check is untouched. Perf note: this deliberately gives back the user-arrow closure reuse from the #8269/#8291 captured-singleton extension (e.g. ECS `World.executeEntityCommands`' per-call inner arrow) and the captureless user-arrow singleton at literal sites; a sound replacement needs escape-aware caching rather than identity-violating sharing. Validation: repros above and test-files/ test_gap_9090_closure_literal_identity.ts byte-identical to node; `cargo test -p perry-runtime --lib -- --test-threads=1` green — 2813 passed, 0 failed with `--skip reserved_floor` (that module's at-scale tests SIGABRT on this pre-#9110 base; known #9108/#9110, unrelated); `cargo test -p perry-codegen`: 283+75 passed after updating the four native_proof_regressions pins from `js_closure_alloc_singleton` to `js_closure_alloc` (their real subject — the alloc storing the public wrapper pointer — is preserved); one pre-existing env-leak flake (`packed_f64_loop_unary_math_store_versions_with_side_exit`) passes in isolation. Claude-Session: https://claude.ai/code/session_01Ay8VyLkKbm8Hkc1xmvTEsP * fix(runtime): name-keyed builtin-member reads must see user overrides — pi boot threw Cyclic __proto__ (part 2) With the closure-literal identity fix in place, pi still died at startup with `TypeError: Cyclic __proto__ value`, obj_bits == proto_bits exactly. The instrumented throw site showed both arguments were ONE closure with `func_ptr = 0xBADD_DEAD` (BOUND_METHOD_FUNC_PTR, capture_count 3) and a healthy 3-link chain behind it — the canonical bound-native callable that `bound_native_callable_export_value` mints once per (module, member). The failing code is graceful-fs's module init, bundled into pi (pi-bundle.mjs:6621/6686/6705): var chdir = process.chdir; process.chdir = function (d) { ... }; if (Object.setPrototypeOf) Object.setPrototypeOf(process.chdir, chdir); and the same wrap for fs.rename / fs.read. Under perry the patch write did not round-trip on the re-read, so setPrototypeOf received the SAME canonical closure for both arguments — a self-set — and the cycle check correctly refused it. The earlier probe of this exact shape passed because it patched a PLAIN object, where writes round-trip; the failure needs a builtin namespace receiver. The JS shim over `Object.setPrototypeOf(` never fired because the conflation happens in the native member-READ, upstream of the call. Root cause: user writes to builtin namespace members are stored in two different places depending on the lowering — computed stores (`process[k] = fn`) go through `nm_field_set_override` into `NATIVE_NAMESPACE_PROP_OVERRIDES`, while static stores (`process.chdir = fn`) reach the generic store path and land as an own dynamic field on the canonical namespace object. The NAME-KEYED read entries carry no object pointer and consulted only the override table: * `js_native_module_property_by_name` (codegen static reads of process.* members) missed own-field stores, so the graceful-fs static patch was invisible to the static re-read; * `js_native_module_esm_export_value` (codegen property reads off a builtin DEFAULT import — `import fs from "node:fs"; fs.rename`) consulted NOTHING (consult_overrides=false plus its own snapshot cache), so no fs patch was ever visible. In Node the default import of a core module is the live mutable CJS exports object, so the patched value must win; the tls DEFAULT_* cache-coherence hack was the ad-hoc version of this for three keys. Fix: `native_namespace_user_value(module, prop)` consults the override table and then the canonical namespace object's own field (never creating a namespace — if none exists, no user store can have landed on one). Both name-keyed read entries call it before any built-in resolution or snapshot cache. Named ESM import bindings of core modules snapshot at module init before user patches run, so their intended snapshot semantics are unaffected in the eager case. Validation: r11-r16 probe matrix (process/fs, static/computed reads and writes) and test-files/test_gap_9091_native_member_patch_roundtrip.ts byte-identical to node; a genuine `setPrototypeOf(x, x)` still throws. Claude-Session: https://claude.ai/code/session_01Ay8VyLkKbm8Hkc1xmvTEsP * style: rustfmt the closure-identity fix --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
1 participant
Fixes #9108 — my tombstone default-on fallout (kill switch
PERRY_OBJECT_TOMBSTONES=0cures the abort, which is how it was attributed). The squeeze shrank the keys array in place, thenset_object_live_slot_count's unchanged-bound early return asserted parity against the still-stamped pre-squeeze descriptor → double panic → SIGABRT atobject::reserved_floor::…at_scale, silently masking the ~1000 tests behind it on every branch (and the re-exec child's '1 passed' line can masquerade as green in tail-style greps — see the issue).Two coherence fixes: the squeeze orders
shape_drop → publish_object_shape_holes → set_object_live_slot_count; andpublish_object_shape_holestakeslogical_key_countfrom the ARRAY rather than the lineage (identical for the O(1) hole delete where length is untouched; the only correct source after a squeeze). Release binaries were never affected — debug_assert only — which is why all differential batteries stayed byte-identical while the suite died.Verification:
reserved_floorsuite 6/0; full perry-runtime lib suite runs to completion, 2819 passed / 0 failed (was: abort at ~test 1804). Suite-integrity blocker for everyone's verification, so flagging for a fast merge.Summary by CodeRabbit