From 4f0deb6e57d140bce536a347aad4df6dff747776 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 12 Aug 2026 17:56:43 +0200 Subject: [PATCH] =?UTF-8?q?test(runtime):=20fix=20the=20last=20two=20suite?= =?UTF-8?q?=20flakes=20=E2=80=94=20shared=20prototype-address=20cache,=20w?= =?UTF-8?q?all-clock=20pause=20budget?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #7955: all five `prototype_addr_cache` cases drove the shipped ARRAY_PROTO_ADDR / OBJECT_PROTO_ADDR statics, so their racing partner was each other — the sentinel case stores usize::MAX into both cells while a sibling has a synthetic forwarded stub planted there. Co-scheduling them reproduces it at 28 failures in 400 runs; 0 after. Both #6981 defences are algebra over an &AtomicUsize, so each case now owns its cell, and the accessor/builtin/scanner pairing becomes one table so "the collector rewrites every cell an accessor reads" holds by construction rather than by a racy assertion. #7956: verify_ordinary_pause_budget asserted elapsed microseconds against a field the type documents as "a soft pause target for telemetry", which no code path can honour — GcCycle::step bounds WORK and measures time afterwards, and the fixtures already drive the smallest step that exists. It now asserts the work-unit budget, the self-report's coherence, the cycle aggregate, and that a step counted in ordinary pause stats ran at all. Closes #7955 Closes #7956 --- .../7974-runtime-suite-flakes-7955-7956.md | 92 +++++++ crates/perry-runtime/src/array/indexing.rs | 170 +----------- crates/perry-runtime/src/array/mod.rs | 24 +- .../perry-runtime/src/array/prototype_addr.rs | 243 ++++++++++++++++++ .../runtime_roots/prototype_addr_cache.rs | 187 ++++++++------ .../src/gc/tests/telemetry_verifier.rs | 232 +++++++++++++++-- 6 files changed, 676 insertions(+), 272 deletions(-) create mode 100644 changelog.d/7974-runtime-suite-flakes-7955-7956.md create mode 100644 crates/perry-runtime/src/array/prototype_addr.rs diff --git a/changelog.d/7974-runtime-suite-flakes-7955-7956.md b/changelog.d/7974-runtime-suite-flakes-7955-7956.md new file mode 100644 index 0000000000..dd18a3a095 --- /dev/null +++ b/changelog.d/7974-runtime-suite-flakes-7955-7956.md @@ -0,0 +1,92 @@ +### test(runtime): the last two `perry-runtime` suite flakes — a shared prototype-address cache, and a wall-clock pause assertion + +Closes #7955 and #7956, the two members of the #7946 flaky set that #7954 named +rather than papered over. Both are test-side; no shipped behaviour changes. + +**#7955 — `gc::tests::runtime_roots::prototype_addr_cache`.** All five cases in +that file drove the SHIPPED `ARRAY_PROTO_ADDR` / `OBJECT_PROTO_ADDR` statics, so +their real racing partner turned out to be *each other*: libtest schedules a +module's tests concurrently, and +`prototype_addr_cache_scanner_leaves_the_unset_sentinel_alone` stores +`usize::MAX` into both cells while a sibling has a synthetic forwarded stub +planted there. Co-scheduling them deliberately reproduces it at **28 failures in +400 runs** (`--test-threads=8 prototype_addr`), every one of them +`prototype_addr_cache_is_rewritten_by_the_collector` reading back `usize::MAX`. +The 1-in-100 full-suite failure #7955 reports is the same mechanism with the +partner arriving by luck. #7954's save/restore guard made it worse rather than +better — restoring the value read at test entry stamps a stale address over +whatever another thread resolved meanwhile. + +Both #6981 defences are algebra over an `&AtomicUsize` (`heal_prototype_addr` +already took one), so `memoized_prototype_addr` and the new +`rewrite_prototype_addr_slot` take the cell as an argument and every mutating +case now owns its cell. Nothing hands out a writable reference to the realm's +real intrinsic cells any more. + +What that decomposition would otherwise lose — *the collector rewrites every +cell an accessor reads*, the #6981 invariant the old test proved by mutation — +is not recovered by another test but by construction: the accessors, the +`globalThis` builtin each resolves, and the root scanner's visit list are now +one table (`PROTOTYPE_ADDR_CACHES`), so a cell some accessor reads and the +collector never rewrites is unrepresentable. One new read-only case +(`the_shipped_cells_are_the_ones_the_scanner_visits`) pins the table itself — +two DISTINCT cells, each paired with its builtin — and writes nothing, so it +cannot be raced either. + +`per_test_global!` was deliberately NOT retried. The obstacle #7955 records is +real (a fresh `PerThread` cell starts unresolved, so the first read on every +libtest thread runs the allocating `globalThis` bootstrap, from paths like +`note_array_index_write` that sit on the array element-write path), but the +decisive objection is different: the accessors are hot-path code whose design +note requires "a single relaxed atomic load", Darwin has no local-exec TLS, and +per-thread storage in a test build only would give the test build a +representation the product does not have. The cache being a process-global +holding a raw address into a THREAD-LOCAL arena is still a real cross-agent +hazard under `perry/thread`; that is a shipped-representation change and wants +its own issue. + +The cache moved to `crates/perry-runtime/src/array/prototype_addr.rs` — +`indexing.rs` reached 2012 lines and `scripts/check_file_size.sh` caps at 2000. + +**#7956 — `gc::tests::telemetry_verifier`.** `verify_ordinary_pause_budget` +asserted `elapsed_pause_us <= soft_pause_target_us` per step. That is not a +mis-tuned bound, it is the wrong instrument, on three independent readings of +the source: `GcPauseBudget` documents itself as "hard work-unit limit plus a +**soft pause target for telemetry**"; `GcCycle::step` runs a phase for +`budget.work_units` and measures elapsed *afterwards*, so no code path can make +a step honour `pause_us`; and these fixtures drive with +`js_gc_step_work_units(1, …)`, the smallest step that exists, so a 4.9 ms work +unit leaves the pacer no smaller choice. The second arm was a tautology of the +first — `within_soft_pause_target` is computed as `elapsed_us <= target` — two +assertions carrying one bit. + +The verifier now checks what the pacer actually controls, and checks more than +before: at least one step counted in ordinary pause stats (the old verifier had +no subject-was-live check at all), every included step bounded in work units +with `applied_work_units <= configured_work_budget` and never labelled +unbounded, `within_soft_pause_target` coherent with the numbers printed beside +it, and `pause_budget.max_observed_step_pause_us` equal to the max over the +steps in its own event. Elapsed microseconds stay in the trace and in every +error message as a diagnostic. Not moved behind an env opt-in: a timing arm no +CI job runs is a gate that cannot fail. + +Six sabotage cases replace the single `verifier_rejects_over_budget_ordinary_step`, +one per rejection path, plus `verifier_accepts_a_slow_but_coherent_ordinary_step` +which encodes #7956's own failing numbers (4936 us against a 2000 us target) as +a case that must now PASS, so the decision cannot be reverted silently. + +**Verification.** Pristine baseline preserved as a binary so each A/B swaps the +artifact, not the tree. M1 mini, load 70–98 throughout. + +| arm | runs | failed | +|---|---|---| +| #7955 repro, pristine, `--test-threads=8 prototype_addr` | 400 | **28** | +| #7955, fixed | 400 | **0** | +| #7956 repro, pristine, `telemetry_verifier` under 16 spinners | 150 | **3** | +| #7956, fixed | 150 | **0** | +| full suite, pristine, default parallelism | 300 | 0 | +| full suite, fixed, default parallelism | 300 | **0** | + +The two full-suite arms are a regression guard, not evidence: at a 1–2 % per-run +rate, 300 runs of that configuration is consistent with either state of the +code, which is why both targeted reproductions exist. diff --git a/crates/perry-runtime/src/array/indexing.rs b/crates/perry-runtime/src/array/indexing.rs index 4bdb564f81..92fe269fde 100644 --- a/crates/perry-runtime/src/array/indexing.rs +++ b/crates/perry-runtime/src/array/indexing.rs @@ -2,7 +2,7 @@ use super::header::{array_numeric_layout, NumericArrayLayout}; use super::*; use std::ptr; -use std::sync::atomic::{AtomicBool, AtomicU8, AtomicUsize, Ordering}; +use std::sync::atomic::{AtomicBool, AtomicU8, Ordering}; const MAX_DENSE_ARRAY_GROW_LENGTH: u32 = 1_000_000; @@ -41,41 +41,21 @@ fn throw_array_not_extensible_add(index: u32) -> ! { )); } -/// Lazily-memoized address of the `Array.prototype` array, and a sticky flag -/// recording whether anyone has installed an indexed property on it. An -/// out-of-bounds element read on an ordinary array must fall through to -/// `Array.prototype[index]` (ECMA-262 OrdinaryGet → prototype chain), but in -/// real code nobody adds numeric indices to `Array.prototype`, so the hot OOB -/// path stays a single relaxed atomic load until the (rare) write flips the -/// flag. `usize::MAX` marks the address as not-yet-computed. -/// -/// ***THIS IS A RAW ADDRESS OF A MOVABLE OBJECT*** (#6981). `Array.prototype` -/// relocates two different ways, and BOTH leave this cache pointing at a -/// `GC_FLAG_FORWARDED` stub while every reader resolves its own receiver -/// through `clean_arr_ptr` (which follows forwarding): -/// -/// 1. `js_array_grow` — an indexed write past the dense capacity -/// (`Array.prototype[300] = v`) reallocates and forwards the old head; -/// 2. the copying young-gen minor — it evacuates the prototype and forwards. -/// -/// A stale cache is not merely a wrong value: `array_oob_prototype_get`'s -/// self-recursion guard is `proto != receiver`, and after a move those are two -/// different addresses **for the same object**, so the guard stops firing and -/// `js_array_get_f64` ⇄ `array_oob_prototype_get` recurse until the stack guard -/// page (SIGSEGV, "excessive recursion"). Hence the two defences below: -/// `array_prototype_addr` resolves the forwarding chain and self-heals, and -/// `scan_prototype_addr_cache_roots_mut` lets the collector rewrite the slot so -/// the address stays live even once the from-space stub is recycled. -static ARRAY_PROTO_ADDR: AtomicUsize = AtomicUsize::new(usize::MAX); +/// Sticky flag: someone installed an indexed property on `Array.prototype`. +/// An out-of-bounds element read on an ordinary array must fall through to +/// `Array.prototype[index]` (ECMA-262 OrdinaryGet -> prototype chain), but in +/// real code nobody adds numeric indices there, so the hot OOB path stays a +/// single relaxed atomic load until the (rare) write flips this. The address +/// it is compared against lives in [`super::prototype_addr`], which also owns +/// the GC hazard that address carries (#6981). static ARRAY_PROTO_HAS_INDEX: AtomicBool = AtomicBool::new(false); /// Same idea for `Object.prototype`: a numeric index installed there /// (`Object.prototype[2] = 2`, or a defineProperty accessor) shows through -/// array HOLES and OOB reads (chain: arr → Array.prototype → +/// array HOLES and OOB reads (chain: arr -> Array.prototype -> /// Object.prototype; test262 concat/S15.4.4.4_A3_T3). Flipped by the object /// index-write/defineProperty hooks; consulted by the typed-feedback guards /// and the hole/OOB read fallbacks. -static OBJECT_PROTO_ADDR: AtomicUsize = AtomicUsize::new(usize::MAX); static OBJECT_PROTO_HAS_INDEX: AtomicBool = AtomicBool::new(false); /// Sticky summary of the process-wide conditions that invalidate codegen's @@ -90,103 +70,6 @@ pub(crate) fn invalidate_array_index_fast_path() { PERRY_ARRAY_INDEX_FAST_PATH_INVALIDATED.store(1, Ordering::Relaxed); } -/// GC root scanner for the two memoized prototype addresses (#6981). -/// -/// `ARRAY_PROTO_ADDR` / `OBJECT_PROTO_ADDR` hold raw addresses of movable -/// objects, so a relocating cycle must REWRITE them exactly like the other -/// address-holding side tables (`CLASS_PROTOTYPE_OBJECTS`, -/// `TYPED_ARRAY_VIEW_META`, …). Forwarding-chain healing alone is not -/// sufficient: once the from-space stub is swept and its block recycled the -/// `GC_FLAG_FORWARDED` bit is gone, and the cache would then name an unrelated -/// live object. Both intrinsics are reachable from `globalThis`, so the marking -/// half of this visit is redundant; the rewriting half is the point. -pub fn scan_prototype_addr_cache_roots_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { - for cache in [&ARRAY_PROTO_ADDR, &OBJECT_PROTO_ADDR] { - let cached = cache.load(Ordering::Relaxed); - if cached == usize::MAX || cached == 0 { - continue; - } - let mut addr = cached; - if visitor.visit_usize_slot(&mut addr) { - // GC_STORE_AUDIT(ROOT): this IS the collector's root-rewrite of a - // registered side-table slot, running inside a root scan with the - // mutator stopped. `visit_usize_slot` returns true only when it - // relocated the object, and the value written is the visitor's own - // to-space address — barriering it would push an edge into the - // remembered set that this very cycle is rebuilding. - cache.store(addr, Ordering::Relaxed); - } - } -} - -/// Test-only handles on the two memoized prototype addresses, so the #6981 -/// regression tests can install a synthetic forwarded stub without touching the -/// realm's real intrinsics. -#[cfg(test)] -pub(crate) fn test_array_proto_addr_cache() -> &'static AtomicUsize { - &ARRAY_PROTO_ADDR -} - -#[cfg(test)] -pub(crate) fn test_object_proto_addr_cache() -> &'static AtomicUsize { - &OBJECT_PROTO_ADDR -} - -/// Re-read a memoized prototype address through the GC forwarding chain and -/// write the healed address back, so every caller compares (and dereferences) -/// the object's CURRENT location. See the `ARRAY_PROTO_ADDR` doc for why an -/// unresolved cache is a hang, not just a wrong answer (#6981). -/// -/// `note_array_index_write` calls this on every indexed array write until the -/// prototype is polluted, so the not-forwarded case must stay call-free: the -/// `try_read_gc_header` probe is `#[inline(always)]` and reduces to two range -/// compares plus one load of a `gc_flags` byte at a fixed, permanently-hot -/// address. It also classifies the address band before dereferencing, so the -/// not-yet-resolved sentinel (`usize::MAX`) and any non-heap value fall -/// straight through. -#[inline] -fn heal_prototype_addr(cache: &AtomicUsize, cached: usize) -> usize { - let forwarded = unsafe { - crate::value::addr_class::try_read_gc_header(cached) - .is_some_and(|header| header.gc_flags & crate::gc::GC_FLAG_FORWARDED != 0) - }; - if !forwarded { - return cached; - } - let resolved = crate::value::resolve_forwarding(cached); - if resolved != cached { - cache.store(resolved, Ordering::Relaxed); - } - resolved -} - -pub(crate) fn object_prototype_addr() -> usize { - let cached = OBJECT_PROTO_ADDR.load(Ordering::Relaxed); - if cached != usize::MAX { - return heal_prototype_addr(&OBJECT_PROTO_ADDR, cached); - } - let ctor = crate::object::js_get_global_this_builtin_value(b"Object".as_ptr(), 6); - let ctor_value = crate::value::JSValue::from_bits(ctor.to_bits()); - let addr = if ctor_value.is_pointer() { - let ctor_ptr = ctor_value.as_pointer::() as usize; - let proto = crate::closure::closure_get_dynamic_prop(ctor_ptr, "prototype"); - let proto_value = crate::value::JSValue::from_bits(proto.to_bits()); - if proto_value.is_pointer() { - proto_value.as_pointer::() as usize - } else { - 0 - } - } else { - 0 - }; - // Cache only a successful resolution — an early call (before globalThis - // init) must retry later rather than pinning 0. - if addr != 0 { - OBJECT_PROTO_ADDR.store(addr, Ordering::Relaxed); - } - addr -} - /// Record (if `obj` is the canonical `Object.prototype`) that it now carries /// an indexed property. Called from the object index-write / numeric /// defineProperty paths; cheap (relaxed loads + compare). @@ -203,12 +86,6 @@ pub(crate) fn object_prototype_has_index_flag() -> bool { OBJECT_PROTO_HAS_INDEX.load(Ordering::Relaxed) } -/// `true` when `addr` is the canonical `Object.prototype` (cheap: cached -/// atomic + compare; lazily computes the address on first use). -pub(crate) fn object_prototype_addr_matches(addr: usize) -> bool { - addr != 0 && addr == object_prototype_addr() -} - /// Sticky flag: user code replaced or deleted `Array.prototype[Symbol.iterator]`. /// `js_get_iterator`'s array short-circuit assumes the builtin values iterator; /// once this flips, GetIterator on an array must consult the (patched) method @@ -256,35 +133,6 @@ pub(crate) fn array_proto_iterator_modified() -> bool { ARRAY_PROTO_ITERATOR_MODIFIED.load(Ordering::Relaxed) } -pub(crate) fn array_prototype_addr() -> usize { - let cached = ARRAY_PROTO_ADDR.load(Ordering::Relaxed); - if cached != usize::MAX { - return heal_prototype_addr(&ARRAY_PROTO_ADDR, cached); - } - let ctor = crate::object::js_get_global_this_builtin_value(b"Array".as_ptr(), 5); - let ctor_value = crate::value::JSValue::from_bits(ctor.to_bits()); - let addr = if ctor_value.is_pointer() { - let ctor_ptr = ctor_value.as_pointer::() as usize; - let proto = crate::closure::closure_get_dynamic_prop(ctor_ptr, "prototype"); - let proto_value = crate::value::JSValue::from_bits(proto.to_bits()); - if proto_value.is_pointer() { - proto_value.as_pointer::() as usize - } else { - 0 - } - } else { - 0 - }; - // Don't poison the cache with 0: during runtime init the global `Array` - // constructor may not be materialized yet (symbol writes on other builtin - // prototypes call into here via `note_array_proto_iterator_write`). - // Re-derive until it resolves. - if addr != 0 { - ARRAY_PROTO_ADDR.store(addr, Ordering::Relaxed); - } - addr -} - /// Record (if `arr` is `Array.prototype`) that the prototype now carries an /// indexed property, so subsequent out-of-bounds reads consult it. Called from /// the array element-write paths; cheap (two relaxed atomic loads + compare). diff --git a/crates/perry-runtime/src/array/mod.rs b/crates/perry-runtime/src/array/mod.rs index f70bcbe693..7d0063093e 100644 --- a/crates/perry-runtime/src/array/mod.rs +++ b/crates/perry-runtime/src/array/mod.rs @@ -16,6 +16,7 @@ mod iter_methods; mod iter_object; mod iterator; mod jsvalue_api; +mod prototype_addr; mod push_pop; mod reduce_right; mod search; @@ -102,13 +103,14 @@ pub use self::immutable::{ js_array_to_sorted_default, js_array_to_sorted_with_comparator, js_array_to_spliced, js_array_with, js_arraylike_copy_within, }; +#[cfg(test)] +pub(crate) use self::indexing::test_keys_array_slot_fallbacks; pub(crate) use self::indexing::{ array_has_own_index, array_iteration_is_exotic, array_proto_iterator_modified, - array_prototype_addr, array_prototype_has_index_flag, array_spec_get, array_spec_has_index, + array_prototype_has_index_flag, array_spec_get, array_spec_has_index, invalidate_array_index_fast_path, keys_array_len_capped_to_capacity, keys_array_slot, - note_array_proto_iterator_write, note_object_prototype_index_write, object_prototype_addr, - object_prototype_addr_matches, object_prototype_has_index_flag, - PERRY_ARRAY_INDEX_FAST_PATH_INVALIDATED, + note_array_proto_iterator_write, note_object_prototype_index_write, + object_prototype_has_index_flag, PERRY_ARRAY_INDEX_FAST_PATH_INVALIDATED, }; pub use self::indexing::{ js_array_get_element, js_array_get_element_f64, js_array_get_f64, js_array_get_f64_unchecked, @@ -117,11 +119,6 @@ pub use self::indexing::{ js_array_numeric_set_f64_unboxed, js_array_set_f64, js_array_set_f64_extend, js_array_set_f64_extend_strict, js_array_set_f64_unchecked, js_array_set_index_or_string, js_array_set_index_or_string_strict, js_array_set_string_key, - scan_prototype_addr_cache_roots_mut, -}; -#[cfg(test)] -pub(crate) use self::indexing::{ - test_array_proto_addr_cache, test_keys_array_slot_fallbacks, test_object_proto_addr_cache, }; pub use self::is_array::js_array_is_array; pub(crate) use self::iter_methods::throw_reduce_of_empty; @@ -142,6 +139,15 @@ pub(crate) use self::iterator::iter_bt_dump; pub use self::iterator::{ js_array_spread_append, js_for_of_to_array, js_get_async_iterator, js_iterator_to_array, }; +pub use self::prototype_addr::scan_prototype_addr_cache_roots_mut; +pub(crate) use self::prototype_addr::{ + array_prototype_addr, object_prototype_addr, object_prototype_addr_matches, +}; +#[cfg(test)] +pub(crate) use self::prototype_addr::{ + test_memoized_prototype_addr, test_prototype_addr_cache_wiring, + test_rewrite_prototype_addr_slot, +}; pub(crate) use self::sort::object_prototype_has_index_prop; pub(crate) use self::sort::object_prototype_index_get as sort_object_prototype_index_get; pub use self::subclass::{ diff --git a/crates/perry-runtime/src/array/prototype_addr.rs b/crates/perry-runtime/src/array/prototype_addr.rs new file mode 100644 index 0000000000..4ab3c2d079 --- /dev/null +++ b/crates/perry-runtime/src/array/prototype_addr.rs @@ -0,0 +1,243 @@ +//! The memoized `Array.prototype` / `Object.prototype` addresses (#6981). +//! +//! Two `AtomicUsize` cells and the algebra over them: lazy resolution from +//! `globalThis`, healing through the GC forwarding chain, and the registered +//! root scanner that lets a relocating cycle rewrite them. +//! +//! Split out of `indexing.rs` because it is one subject with one invariant — +//! *every cell an accessor reads is a cell the collector rewrites* — and that +//! invariant is easiest to keep true when the cells, the accessors and the +//! scanner are the only things in the file (and it kept `indexing.rs` under +//! `scripts/check_file_size.sh`'s 2000-line cap). + +use std::sync::atomic::{AtomicUsize, Ordering}; + +/// Lazily-memoized address of the `Array.prototype` array. An out-of-bounds +/// element read on an ordinary array must fall through to +/// `Array.prototype[index]` (ECMA-262 OrdinaryGet → prototype chain), but in +/// real code nobody adds numeric indices to `Array.prototype`, so the hot OOB +/// path stays a single relaxed atomic load until the (rare) write flips +/// `ARRAY_PROTO_HAS_INDEX`. `usize::MAX` marks the address as not-yet-computed. +/// +/// ***THIS IS A RAW ADDRESS OF A MOVABLE OBJECT*** (#6981). `Array.prototype` +/// relocates two different ways, and BOTH leave this cache pointing at a +/// `GC_FLAG_FORWARDED` stub while every reader resolves its own receiver +/// through `clean_arr_ptr` (which follows forwarding): +/// +/// 1. `js_array_grow` — an indexed write past the dense capacity +/// (`Array.prototype[300] = v`) reallocates and forwards the old head; +/// 2. the copying young-gen minor — it evacuates the prototype and forwards. +/// +/// A stale cache is not merely a wrong value: `array_oob_prototype_get`'s +/// self-recursion guard is `proto != receiver`, and after a move those are two +/// different addresses **for the same object**, so the guard stops firing and +/// `js_array_get_f64` ⇄ `array_oob_prototype_get` recurse until the stack guard +/// page (SIGSEGV, "excessive recursion"). Hence the two defences below: +/// [`memoized_prototype_addr`] resolves the forwarding chain and self-heals, +/// and [`scan_prototype_addr_cache_roots_mut`] lets the collector rewrite the +/// slot so the address stays live even once the from-space stub is recycled. +static ARRAY_PROTO_ADDR: AtomicUsize = AtomicUsize::new(usize::MAX); + +/// Same idea for `Object.prototype`: a numeric index installed there +/// (`Object.prototype[2] = 2`, or a defineProperty accessor) shows through +/// array HOLES and OOB reads (chain: arr → Array.prototype → +/// Object.prototype; test262 concat/S15.4.4.4_A3_T3). Consulted by the +/// typed-feedback guards and the hole/OOB read fallbacks. +static OBJECT_PROTO_ADDR: AtomicUsize = AtomicUsize::new(usize::MAX); + +/// One memoized intrinsic-prototype address: the cell, and the `globalThis` +/// builtin whose `.prototype` fills it. +/// +/// The pairing is a TABLE rather than two hand-written accessors so that the +/// two facts a reader has to trust — "the collector rewrites every cell some +/// accessor reads" and "each accessor resolves the builtin its cell is named +/// for" — are established by construction instead of by a test that has to +/// mutate a process-global to observe them (#7955). See +/// [`PROTOTYPE_ADDR_CACHES`]. +struct PrototypeAddrCache { + cell: &'static AtomicUsize, + /// `globalThis` key whose `.prototype` this cell memoizes. + builtin: &'static [u8], +} + +/// Every memoized prototype address in the runtime, in the order the GC root +/// scanner visits them. +/// +/// [`scan_prototype_addr_cache_roots_mut`] iterates this table and +/// [`array_prototype_addr`] / [`object_prototype_addr`] index it, so a cell +/// that an accessor reads but the collector never rewrites — the #6981 defect +/// — is not representable. Adding a third memoized intrinsic address means +/// adding a row here, and it is covered by both halves automatically. +static PROTOTYPE_ADDR_CACHES: [PrototypeAddrCache; 2] = [ + PrototypeAddrCache { + cell: &ARRAY_PROTO_ADDR, + builtin: b"Array", + }, + PrototypeAddrCache { + cell: &OBJECT_PROTO_ADDR, + builtin: b"Object", + }, +]; + +const ARRAY_PROTO_CACHE: usize = 0; +const OBJECT_PROTO_CACHE: usize = 1; + +/// GC root scanner for the memoized prototype addresses (#6981). +/// +/// `ARRAY_PROTO_ADDR` / `OBJECT_PROTO_ADDR` hold raw addresses of movable +/// objects, so a relocating cycle must REWRITE them exactly like the other +/// address-holding side tables (`CLASS_PROTOTYPE_OBJECTS`, +/// `TYPED_ARRAY_VIEW_META`, …). Forwarding-chain healing alone is not +/// sufficient: once the from-space stub is swept and its block recycled the +/// `GC_FLAG_FORWARDED` bit is gone, and the cache would then name an unrelated +/// live object. Both intrinsics are reachable from `globalThis`, so the marking +/// half of this visit is redundant; the rewriting half is the point. +pub fn scan_prototype_addr_cache_roots_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { + for entry in &PROTOTYPE_ADDR_CACHES { + rewrite_prototype_addr_slot(entry.cell, visitor); + } +} + +/// The per-cell half of [`scan_prototype_addr_cache_roots_mut`]. +/// +/// Split out so the #6981 rewrite algebra can be exercised on a cell the test +/// owns privately: driving it through the shipped `static`s made the assertion +/// depend on no other libtest thread touching the realm's real intrinsics +/// meanwhile, which is the #7955 flake. +fn rewrite_prototype_addr_slot( + cache: &AtomicUsize, + visitor: &mut crate::gc::RuntimeRootVisitor<'_>, +) { + let cached = cache.load(Ordering::Relaxed); + if cached == usize::MAX || cached == 0 { + return; + } + let mut addr = cached; + if visitor.visit_usize_slot(&mut addr) { + // GC_STORE_AUDIT(ROOT): this IS the collector's root-rewrite of a + // registered side-table slot, running inside a root scan with the + // mutator stopped. `visit_usize_slot` returns true only when it + // relocated the object, and the value written is the visitor's own + // to-space address — barriering it would push an edge into the + // remembered set that this very cycle is rebuilding. + cache.store(addr, Ordering::Relaxed); + } +} + +/// Read a memoized prototype address, resolving it through the GC forwarding +/// chain. `None` means "not resolved yet" — the caller must run the +/// `globalThis` bootstrap. +#[inline] +fn memoized_prototype_addr(cache: &AtomicUsize) -> Option { + let cached = cache.load(Ordering::Relaxed); + (cached != usize::MAX).then(|| heal_prototype_addr(cache, cached)) +} + +/// Re-read a memoized prototype address through the GC forwarding chain and +/// write the healed address back, so every caller compares (and dereferences) +/// the object's CURRENT location. See the [`ARRAY_PROTO_ADDR`] doc for why an +/// unresolved cache is a hang, not just a wrong answer (#6981). +/// +/// `note_array_index_write` calls this on every indexed array write until the +/// prototype is polluted, so the not-forwarded case must stay call-free: the +/// `try_read_gc_header` probe is `#[inline(always)]` and reduces to two range +/// compares plus one load of a `gc_flags` byte at a fixed, permanently-hot +/// address. It also classifies the address band before dereferencing, so the +/// not-yet-resolved sentinel (`usize::MAX`) and any non-heap value fall +/// straight through. +#[inline] +fn heal_prototype_addr(cache: &AtomicUsize, cached: usize) -> usize { + let forwarded = unsafe { + crate::value::addr_class::try_read_gc_header(cached) + .is_some_and(|header| header.gc_flags & crate::gc::GC_FLAG_FORWARDED != 0) + }; + if !forwarded { + return cached; + } + let resolved = crate::value::resolve_forwarding(cached); + if resolved != cached { + cache.store(resolved, Ordering::Relaxed); + } + resolved +} + +/// Resolve one row of [`PROTOTYPE_ADDR_CACHES`]: the memoized address if it is +/// already known (healed through any forwarding chain), otherwise the +/// `globalThis` bootstrap, memoized. +fn resolve_prototype_addr(entry: &PrototypeAddrCache) -> usize { + if let Some(addr) = memoized_prototype_addr(entry.cell) { + return addr; + } + let ctor = crate::object::js_get_global_this_builtin_value( + entry.builtin.as_ptr(), + entry.builtin.len(), + ); + let ctor_value = crate::value::JSValue::from_bits(ctor.to_bits()); + let addr = if ctor_value.is_pointer() { + let ctor_ptr = ctor_value.as_pointer::() as usize; + let proto = crate::closure::closure_get_dynamic_prop(ctor_ptr, "prototype"); + let proto_value = crate::value::JSValue::from_bits(proto.to_bits()); + if proto_value.is_pointer() { + proto_value.as_pointer::() as usize + } else { + 0 + } + } else { + 0 + }; + // Don't poison the cache with 0: during runtime init the global constructor + // may not be materialized yet (symbol writes on other builtin prototypes + // call into here via `note_array_proto_iterator_write`). Re-derive until it + // resolves. + if addr != 0 { + entry.cell.store(addr, Ordering::Relaxed); + } + addr +} + +pub(crate) fn array_prototype_addr() -> usize { + resolve_prototype_addr(&PROTOTYPE_ADDR_CACHES[ARRAY_PROTO_CACHE]) +} + +pub(crate) fn object_prototype_addr() -> usize { + resolve_prototype_addr(&PROTOTYPE_ADDR_CACHES[OBJECT_PROTO_CACHE]) +} + +/// `true` when `addr` is the canonical `Object.prototype` (cheap: cached +/// atomic + compare; lazily computes the address on first use). +pub(crate) fn object_prototype_addr_matches(addr: usize) -> bool { + addr != 0 && addr == object_prototype_addr() +} + +/// Test-only handle on the shipped table, for the read-only wiring assertion +/// in `gc::tests::runtime_roots::prototype_addr_cache`. The mutating #6981 +/// cases run on cells they own; nothing hands out a writable reference to the +/// realm's real intrinsic cells any more (#7955). +#[cfg(test)] +pub(crate) fn test_prototype_addr_cache_wiring() -> [(&'static AtomicUsize, &'static [u8]); 2] { + [ + ( + PROTOTYPE_ADDR_CACHES[ARRAY_PROTO_CACHE].cell, + PROTOTYPE_ADDR_CACHES[ARRAY_PROTO_CACHE].builtin, + ), + ( + PROTOTYPE_ADDR_CACHES[OBJECT_PROTO_CACHE].cell, + PROTOTYPE_ADDR_CACHES[OBJECT_PROTO_CACHE].builtin, + ), + ] +} + +/// The two halves of the #6981 defences, exported so the tests can drive them +/// on a private cell instead of the process-global one (#7955). +#[cfg(test)] +pub(crate) fn test_memoized_prototype_addr(cache: &AtomicUsize) -> Option { + memoized_prototype_addr(cache) +} + +#[cfg(test)] +pub(crate) fn test_rewrite_prototype_addr_slot( + cache: &AtomicUsize, + visitor: &mut crate::gc::RuntimeRootVisitor<'_>, +) { + rewrite_prototype_addr_slot(cache, visitor) +} diff --git a/crates/perry-runtime/src/gc/tests/runtime_roots/prototype_addr_cache.rs b/crates/perry-runtime/src/gc/tests/runtime_roots/prototype_addr_cache.rs index 907775b20c..1f9d2abadc 100644 --- a/crates/perry-runtime/src/gc/tests/runtime_roots/prototype_addr_cache.rs +++ b/crates/perry-runtime/src/gc/tests/runtime_roots/prototype_addr_cache.rs @@ -1,8 +1,8 @@ //! #6981 — the memoized `Array.prototype` / `Object.prototype` addresses are //! raw pointers to MOVABLE objects, so they must survive relocation. //! -//! `array::indexing` memoizes both intrinsic addresses in process-global -//! `AtomicUsize` caches. `Array.prototype` relocates two ways, and both leave a +//! `array::prototype_addr` memoizes both intrinsic addresses in process-global +//! `AtomicUsize` cells. `Array.prototype` relocates two ways, and both leave a //! `GC_FLAG_FORWARDED` stub at the memoized address: //! //! * `js_array_grow` — `Array.prototype[300] = v` reallocates the dense @@ -19,41 +19,45 @@ //! //! Two independent defences, one test each: //! -//! 1. `array_prototype_addr` / `object_prototype_addr` heal the cache through -//! the forwarding chain. This is what covers `js_array_grow`, which the -//! collector never sees. +//! 1. `memoized_prototype_addr` heals the cell through the forwarding chain. +//! This is what covers `js_array_grow`, which the collector never sees. //! 2. `scan_prototype_addr_cache_roots_mut` is a registered mutable root -//! scanner, so a relocating cycle REWRITES the slot. Healing alone is not +//! scanner, so a relocating cycle REWRITES the cell. Healing alone is not //! enough here: once the from-space stub is swept and its block recycled //! the forwarded bit is gone, and the cache would name an unrelated live //! object. //! -//! The tests install a *synthetic* stub in the cache and restore the previous -//! value on the way out, so they never disturb the realm's real intrinsics. +//! # Why these run on private cells (#7955) +//! +//! Both defences used to be driven by planting a synthetic stub in the SHIPPED +//! `static`s and reading it back. That made every assertion here depend on no +//! other libtest thread touching the realm's real intrinsics in between — and +//! two things routinely do: `array_prototype_addr()` / `object_prototype_addr()` +//! HEAL the cell in place, and any collection's registered +//! `scan_prototype_addr_cache_roots_mut` REWRITES it. Either overwrites the +//! plant, and the test reports a stale-cache failure that says nothing about +//! the code under test. The save/restore guard made it worse rather than +//! better: restoring the value read at test entry stamps a stale address over +//! whatever another thread resolved meanwhile. +//! +//! Both defences are algebra over an `&AtomicUsize`, so each case now owns its +//! cell and the shipped `static`s are never written from a test. What that +//! decomposition would otherwise lose — "the collector rewrites every cell an +//! accessor reads" — is not recovered by a test at all but by CONSTRUCTION: +//! `PROTOTYPE_ADDR_CACHES` is one table, the scanner iterates it and the +//! accessors index it. `the_shipped_cells_are_the_ones_the_scanner_visits` +//! pins the table itself, read-only, so it cannot be raced either. use super::*; use std::sync::atomic::{AtomicUsize, Ordering}; -/// Save/restore both prototype-address caches around a test body. -struct ProtoAddrCacheGuard { - array: usize, - object: usize, -} - -impl ProtoAddrCacheGuard { - fn new() -> Self { - Self { - array: crate::array::test_array_proto_addr_cache().load(Ordering::Relaxed), - object: crate::array::test_object_proto_addr_cache().load(Ordering::Relaxed), - } - } -} - -impl Drop for ProtoAddrCacheGuard { - fn drop(&mut self) { - crate::array::test_array_proto_addr_cache().store(self.array, Ordering::Relaxed); - crate::array::test_object_proto_addr_cache().store(self.object, Ordering::Relaxed); - } +/// A memoized-prototype-address cell owned by ONE test. +/// +/// `memoized_prototype_addr` / `rewrite_prototype_addr_slot` take the cell as +/// an argument, so the #6981 algebra is exercised exactly as shipped without +/// any test writing to the realm's real intrinsic cells. +fn private_cache_cell() -> AtomicUsize { + AtomicUsize::new(usize::MAX) } /// Allocate a nursery object to stand in for the intrinsic. @@ -98,25 +102,9 @@ fn forwarded_pair() -> (usize, usize) { (from as usize, to) } -fn cache_of(which: &str) -> &'static AtomicUsize { - if which == "array" { - crate::array::test_array_proto_addr_cache() - } else { - crate::array::test_object_proto_addr_cache() - } -} - -fn read_addr(which: &str) -> usize { - if which == "array" { - crate::array::array_prototype_addr() - } else { - crate::array::object_prototype_addr() - } -} - /// DEFENCE 1. A memoized address that has been forwarded — by `js_array_grow` /// or by an evacuating cycle — must read back as the object's CURRENT address, -/// and the cache must be healed in place so the next reader pays nothing. +/// and the cell must be healed in place so the next reader pays nothing. /// /// Without the heal this returns the from-space stub, which is a different /// address for the same object than every `clean_arr_ptr`-resolved receiver — @@ -124,38 +112,46 @@ fn read_addr(which: &str) -> usize { /// self-recursion guard. #[test] fn prototype_addr_reads_through_a_forwarding_stub() { - let _guard = ProtoAddrCacheGuard::new(); // Not exposed at this level: `from`/`to` come back as plain `usize`s from // `forwarded_pair`, which carries its own trigger guard, and nothing else // in this loop body allocates. - for which in ["array", "object"] { + let cell = private_cache_cell(); let (from, to) = forwarded_pair(); - cache_of(which).store(from, Ordering::Relaxed); + cell.store(from, Ordering::Relaxed); assert_eq!( - read_addr(which), - to, - "{which}_prototype_addr must resolve the GC forwarding chain: a stale \ - from-space address is a DIFFERENT address for the SAME object than \ - every clean_arr_ptr-resolved receiver, which defeats the \ - `proto != receiver` self-recursion guard in the hole/OOB read \ + crate::array::test_memoized_prototype_addr(&cell), + Some(to), + "the {which} prototype cell must resolve the GC forwarding chain: a \ + stale from-space address is a DIFFERENT address for the SAME \ + object than every clean_arr_ptr-resolved receiver, which defeats \ + the `proto != receiver` self-recursion guard in the hole/OOB read \ fallback and hangs the mutator (#6981)" ); assert_eq!( - cache_of(which).load(Ordering::Relaxed), + cell.load(Ordering::Relaxed), to, - "{which}_prototype_addr must write the healed address back so the \ - hot path stays a single relaxed load" + "the read must write the healed address back so the hot path stays \ + a single relaxed load" ); } } +/// A cell that has never resolved reports "not resolved" rather than healing +/// the sentinel — that is what sends the accessor to the `globalThis` +/// bootstrap instead of pinning a bogus prototype. +#[test] +fn an_unresolved_prototype_cell_reports_no_address() { + let cell = private_cache_cell(); + assert_eq!(crate::array::test_memoized_prototype_addr(&cell), None); + assert_eq!(cell.load(Ordering::Relaxed), usize::MAX); +} + /// Multi-hop chains (grow, then grow again, then evacuate) must resolve all the /// way to the live head. #[test] fn prototype_addr_reads_through_a_multi_hop_forwarding_chain() { - let _guard = ProtoAddrCacheGuard::new(); // `first` is live across `second`'s allocation, and both `first` and // `second` are live across `final_user`'s allocation — any of the three // could reach the block-full slow path's `gc_check_trigger()`. @@ -169,21 +165,21 @@ fn prototype_addr_reads_through_a_multi_hop_forwarding_chain() { set_forwarding_address(header_from_user_ptr(second) as *mut GcHeader, final_user); } - crate::array::test_array_proto_addr_cache().store(first as usize, Ordering::Relaxed); + let cell = private_cache_cell(); + cell.store(first as usize, Ordering::Relaxed); assert_eq!( - crate::array::array_prototype_addr(), - final_user as usize, + crate::array::test_memoized_prototype_addr(&cell), + Some(final_user as usize), "every forwarding hop must be followed (#6981)" ); } -/// DEFENCE 2. The collector must REWRITE the cache, not merely leave it +/// DEFENCE 2. The collector must REWRITE the cell, not merely leave it /// resolvable — from-space is reset and handed back to the mutator at the end /// of the cycle, after which the forwarded bit is gone and healing cannot /// recover the address. #[test] fn prototype_addr_cache_is_rewritten_by_the_collector() { - let _guard = ProtoAddrCacheGuard::new(); // `array_from` is live across the second `nursery_stand_in` call below // (its own allocation, unguarded on its own), and both from-pointers stay // live across the `evacuate` calls that follow. @@ -197,24 +193,29 @@ fn prototype_addr_cache_is_rewritten_by_the_collector() { let valid_ptrs = build_valid_pointer_set(); let array_to = evacuate(array_from); let object_to = evacuate(object_from); - crate::array::test_array_proto_addr_cache().store(array_from as usize, Ordering::Relaxed); - crate::array::test_object_proto_addr_cache().store(object_from as usize, Ordering::Relaxed); + let array_cell = private_cache_cell(); + let object_cell = private_cache_cell(); + array_cell.store(array_from as usize, Ordering::Relaxed); + object_cell.store(object_from as usize, Ordering::Relaxed); - crate::array::scan_prototype_addr_cache_roots_mut(&mut RuntimeRootVisitor::for_rewrite( - &valid_ptrs, - )); + for cell in [&array_cell, &object_cell] { + crate::array::test_rewrite_prototype_addr_slot( + cell, + &mut RuntimeRootVisitor::for_rewrite(&valid_ptrs), + ); + } assert_eq!( - crate::array::test_array_proto_addr_cache().load(Ordering::Relaxed), + array_cell.load(Ordering::Relaxed), array_to, - "the ARRAY_PROTO_ADDR slot must be rewritten by the relocating cycle — \ + "a memoized prototype cell must be rewritten by the relocating cycle — \ it is a raw address of a movable object, exactly like the other \ registered side tables (#6981)" ); assert_eq!( - crate::array::test_object_proto_addr_cache().load(Ordering::Relaxed), + object_cell.load(Ordering::Relaxed), object_to, - "the OBJECT_PROTO_ADDR slot must be rewritten by the relocating cycle \ + "the rewrite is per-cell, so both rows of PROTOTYPE_ADDR_CACHES get it \ (#6981)" ); } @@ -243,22 +244,44 @@ fn prototype_addr_cache_scanner_is_registered() { /// a scanner that rewrote it would pin a bogus prototype for the whole process. #[test] fn prototype_addr_cache_scanner_leaves_the_unset_sentinel_alone() { - let _guard = ProtoAddrCacheGuard::new(); let valid_ptrs = build_valid_pointer_set(); + let cell = private_cache_cell(); - crate::array::test_array_proto_addr_cache().store(usize::MAX, Ordering::Relaxed); - crate::array::test_object_proto_addr_cache().store(usize::MAX, Ordering::Relaxed); + crate::array::test_rewrite_prototype_addr_slot( + &cell, + &mut RuntimeRootVisitor::for_rewrite(&valid_ptrs), + ); - crate::array::scan_prototype_addr_cache_roots_mut(&mut RuntimeRootVisitor::for_rewrite( - &valid_ptrs, - )); + assert_eq!(cell.load(Ordering::Relaxed), usize::MAX); +} +/// The WIRING, and deliberately read-only so it cannot be raced (#7955). +/// +/// The cases above prove the algebra on cells they own; on its own that would +/// leave nothing asserting that the shipped `static`s are the cells in play — +/// the "gate runs but its subject never did" shape. `PROTOTYPE_ADDR_CACHES` is +/// the single table the scanner iterates and the accessors index, so this +/// pins the table: two DISTINCT cells (a copy-pasted row would give +/// `Array.prototype`'s address to `object_prototype_addr()` and leave one cell +/// unrewritten), each paired with the `globalThis` builtin whose `.prototype` +/// its accessor resolves. Nothing here writes. +#[test] +fn the_shipped_cells_are_the_ones_the_scanner_visits() { + let wiring = crate::array::test_prototype_addr_cache_wiring(); assert_eq!( - crate::array::test_array_proto_addr_cache().load(Ordering::Relaxed), - usize::MAX + wiring[0].1, b"Array", + "row 0 is what array_prototype_addr() indexes; it must bootstrap from \ + globalThis.Array" ); assert_eq!( - crate::array::test_object_proto_addr_cache().load(Ordering::Relaxed), - usize::MAX + wiring[1].1, b"Object", + "row 1 is what object_prototype_addr() indexes; it must bootstrap from \ + globalThis.Object" + ); + assert!( + !std::ptr::eq(wiring[0].0, wiring[1].0), + "the two intrinsics must memoize into DIFFERENT cells — sharing one \ + cell makes the second accessor return the first intrinsic's address \ + and leaves the collector with nothing to rewrite for it (#6981)" ); } diff --git a/crates/perry-runtime/src/gc/tests/telemetry_verifier.rs b/crates/perry-runtime/src/gc/tests/telemetry_verifier.rs index 7746d38529..4955255691 100644 --- a/crates/perry-runtime/src/gc/tests/telemetry_verifier.rs +++ b/crates/perry-runtime/src/gc/tests/telemetry_verifier.rs @@ -27,37 +27,129 @@ fn complete_budgeted_cycle_trace() -> serde_json::Value { take_test_last_gc_trace_json().expect("budgeted GC completion should emit test trace JSON") } +/// Verify the ordinary-pause contract of a budgeted cycle's trace. +/// +/// # Why this does not assert elapsed microseconds (#7956) +/// +/// It used to: every included step had to satisfy +/// `elapsed_pause_us <= soft_pause_target_us`. That assertion failed ~2 runs in +/// 100 of `cargo test --release -p perry-runtime` on a loaded host — and it +/// failed for a reason no change to the GC could fix, because the quantity it +/// bounds is not one the code under test controls: +/// +/// * `GcPauseBudget`'s own definition is "hard work-unit limit plus a **soft +/// pause target for telemetry**". `pause_us` is an annotation on the +/// trace, not a guarantee the stepper offers; +/// * `GcCycle::step` runs a phase for `budget.work_units` and measures +/// `elapsed` *afterwards*. No code path anywhere consults the clock to +/// decide when a step ends, so `elapsed <= pause_us` is not a +/// postcondition the collector can establish — it is a property of the +/// host; +/// * these fixtures drive the cycle with `js_gc_step_work_units(1, …)`, the +/// smallest step that exists. When one work unit takes 4.9 ms there is no +/// smaller step the pacer could have chosen, so the failure carries no +/// information about pacing at all. +/// +/// The second arm was worse than uninformative: `within_soft_pause_target` is +/// computed in `pause_step_json` as `elapsed_us <= target`, so +/// "did not self-report within_soft_pause_target" could only fire when the +/// first check had already fired. Two assertions, one bit — the presence-check +/// shape, not a proof. +/// +/// What replaces it is the part of the contract that IS deterministic: an +/// ordinary budgeted step is bounded in **work units** (the unit the trace +/// itself names as `budget_unit`), it is never labelled unbounded, its +/// self-report agrees with the numbers printed beside it, and the cycle-level +/// pause aggregate is the max of the per-step figures it summarises. Elapsed +/// microseconds stay in the trace and in these messages as a diagnostic; they +/// are no longer a verdict. fn verify_ordinary_pause_budget(event: &serde_json::Value) -> Result<(), String> { - let soft_target = event["pause_budget"]["soft_pause_target_us"] - .as_u64() - .ok_or_else(|| "missing pause_budget.soft_pause_target_us".to_string())?; let steps = event["pause_steps"] .as_array() .ok_or_else(|| "missing pause_steps".to_string())?; if steps.is_empty() { return Err("ordinary cycle emitted no pause_steps".to_string()); } + + let mut included = 0usize; + let mut max_elapsed = 0u64; for (index, step) in steps.iter().enumerate() { - let include = step["budget"]["ordinary_pause_stats_include"] + let elapsed = step["elapsed_pause_us"] + .as_u64() + .ok_or_else(|| format!("pause_steps[{index}] missing elapsed_pause_us"))?; + max_elapsed = max_elapsed.max(elapsed); + + if !step["budget"]["ordinary_pause_stats_include"] .as_bool() - .unwrap_or(false); - if !include { + .unwrap_or(false) + { continue; } - let elapsed = step["elapsed_pause_us"] + included += 1; + + // An ordinary step must be BOUNDED. A `null` work budget is how the + // trace spells "this path is intentionally unbounded", and a step + // carrying that label while counted in ordinary pause stats is the + // real pacing defect the elapsed check was reaching for. + let work_budget = step["budget"]["configured_work_budget"] .as_u64() - .ok_or_else(|| format!("pause_steps[{index}] missing elapsed_pause_us"))?; - if elapsed > soft_target { + .ok_or_else(|| { + format!( + "pause_steps[{index}] is counted in ordinary pause stats but \ + carries no configured_work_budget — an ordinary step must \ + never run unbounded (elapsed {elapsed}us)" + ) + })?; + let applied = step["applied_work_units"].as_u64().ok_or_else(|| { + format!("pause_steps[{index}] missing applied_work_units (elapsed {elapsed}us)") + })?; + if applied > work_budget { return Err(format!( - "pause_steps[{index}] elapsed {elapsed}us exceeded soft target {soft_target}us" + "pause_steps[{index}] applied {applied} work units over a \ + configured budget of {work_budget} (elapsed {elapsed}us)" )); } - if step["budget"]["within_soft_pause_target"].as_bool() != Some(true) { + + // The self-report must agree with the numbers reported beside it. This + // is a coherence check on the telemetry, not a claim about the host: + // it fires when the flag is computed against the wrong step or the + // wrong progress kind's budget, and never because the box was busy. + let soft_target = step["budget"]["soft_pause_target_us"] + .as_u64() + .ok_or_else(|| format!("pause_steps[{index}] missing soft_pause_target_us"))?; + let self_report = step["budget"]["within_soft_pause_target"].as_bool(); + if self_report != Some(elapsed <= soft_target) { return Err(format!( - "pause_steps[{index}] did not self-report within_soft_pause_target" + "pause_steps[{index}] self-reported within_soft_pause_target = \ + {self_report:?}, but elapsed {elapsed}us against a soft target \ + of {soft_target}us says {}", + elapsed <= soft_target )); } } + + if included == 0 { + return Err( + "ordinary cycle emitted no step counted in ordinary pause stats — \ + the pause budget under test never ran" + .to_string(), + ); + } + + // The cycle-level aggregate must summarise the very steps printed in the + // same event (#7025's shape: a counter that sums something other than what + // it names). Deterministic — both sides come out of `record_pause_step`. + let reported_max = event["pause_budget"]["max_observed_step_pause_us"] + .as_u64() + .ok_or_else(|| "missing pause_budget.max_observed_step_pause_us".to_string())?; + if reported_max != max_elapsed { + return Err(format!( + "pause_budget.max_observed_step_pause_us = {reported_max}us but the \ + maximum elapsed_pause_us over the {} reported steps is {max_elapsed}us", + steps.len() + )); + } + Ok(()) } @@ -328,26 +420,126 @@ fn emergency_full_trace_is_excluded_from_ordinary_pause_stats() { drop(trigger_guard); } -#[test] -fn verifier_rejects_over_budget_ordinary_step() { - let event = serde_json::json!({ +/// A well-formed ordinary trace, as a base for the sabotage cases below. +/// +/// Deliberately over the soft pause target (`elapsed_pause_us` 4936 against a +/// 2000 us target — the figure from #7956's real failure), with a self-report +/// that says so. A slow host is not a defect, and this asserts that directly: +/// the shape that used to fail 2 runs in 100 must now VERIFY. +fn coherent_ordinary_trace() -> serde_json::Value { + serde_json::json!({ "pause_budget": { - "soft_pause_target_us": 10, + "soft_pause_target_us": 2000, + "configured_work_budget": 64, + "max_observed_step_pause_us": 4936, }, "pause_steps": [ { - "elapsed_pause_us": 11, + "applied_work_units": 1, + "elapsed_pause_us": 4936, "budget": { + "configured_work_budget": 64, + "soft_pause_target_us": 2000, "ordinary_pause_stats_include": true, "within_soft_pause_target": false, }, }, + { + "applied_work_units": 1, + "elapsed_pause_us": 12, + "budget": { + "configured_work_budget": 64, + "soft_pause_target_us": 2000, + "ordinary_pause_stats_include": true, + "within_soft_pause_target": true, + }, + }, ], - }); + }) +} +/// #7956: an over-target step on a loaded host is REPORTED, not failed. Kept as +/// a test rather than a comment so the decision cannot be reverted silently. +#[test] +fn verifier_accepts_a_slow_but_coherent_ordinary_step() { + assert_eq!( + verify_ordinary_pause_budget(&coherent_ordinary_trace()), + Ok(()) + ); +} + +/// The work-unit budget is the HARD limit (`GcPauseBudget`'s own wording), so +/// a step that applied more work than it was granted is a real defect. +#[test] +fn verifier_rejects_a_step_over_its_work_budget() { + let mut event = coherent_ordinary_trace(); + event["pause_steps"][1]["applied_work_units"] = serde_json::json!(65); + assert!( + verify_ordinary_pause_budget(&event) + .unwrap_err() + .contains("applied 65 work units"), + "a step exceeding its configured work budget must fail the verifier" + ); +} + +/// `null` is how the trace spells "unbounded". An ordinary budgeted step +/// carrying that label is the pacing defect the old elapsed check was aimed at. +#[test] +fn verifier_rejects_an_unbounded_ordinary_step() { + let mut event = coherent_ordinary_trace(); + event["pause_steps"][0]["budget"]["configured_work_budget"] = serde_json::Value::Null; + assert!( + verify_ordinary_pause_budget(&event) + .unwrap_err() + .contains("must never run unbounded"), + "an ordinary step with no work budget must fail the verifier" + ); +} + +/// The self-report must track the numbers printed beside it — a flag computed +/// against the wrong step or the wrong progress kind's budget is a telemetry +/// bug that no timing threshold would catch. +#[test] +fn verifier_rejects_an_incoherent_pause_self_report() { + let mut event = coherent_ordinary_trace(); + event["pause_steps"][0]["budget"]["within_soft_pause_target"] = serde_json::json!(true); + assert!( + verify_ordinary_pause_budget(&event) + .unwrap_err() + .contains("self-reported within_soft_pause_target"), + "a self-report contradicting elapsed vs target must fail the verifier" + ); +} + +/// #7025's shape: an aggregate that summarises something other than what it +/// names. +#[test] +fn verifier_rejects_a_pause_aggregate_that_misses_its_own_steps() { + let mut event = coherent_ordinary_trace(); + event["pause_budget"]["max_observed_step_pause_us"] = serde_json::json!(12); + assert!( + verify_ordinary_pause_budget(&event) + .unwrap_err() + .contains("max_observed_step_pause_us"), + "the cycle-level pause max must equal the max over the reported steps" + ); +} + +/// The subject-was-live check: a trace whose every step is excluded from +/// ordinary pause stats proves nothing about the ordinary pause budget, so the +/// verifier must not report success for it. +#[test] +fn verifier_rejects_a_trace_with_no_ordinary_step() { + let mut event = coherent_ordinary_trace(); + for index in 0..2 { + event["pause_steps"][index]["budget"]["ordinary_pause_stats_include"] = + serde_json::json!(false); + } assert!( - verify_ordinary_pause_budget(&event).is_err(), - "synthetic over-budget ordinary step should fail verifier" + verify_ordinary_pause_budget(&event) + .unwrap_err() + .contains("never ran"), + "a verifier that passes when its subject never ran is not a gate" ); }