From de11c2b194849efefcddc8f76a3b96110a424925 Mon Sep 17 00:00:00 2001 From: jdalton Date: Fri, 31 Jul 2026 13:22:18 -0400 Subject: [PATCH 1/8] fix(lru-cache): faithful JS-value keys/values, GC rooting, TTL + updateAgeOnGet perry-ext-lru-cache only handled numeric (f64) keys/values with no TTL, so real usage (string keys -> object/string values with ttl + updateAgeOnGet, e.g. Socket Firewall) silently misbehaved. - Keys/values are treated as real NaN-boxed JS values, not raw f64 bits. String keys hash/compare by CONTENT (materialized via js_get_string_pointer_unified); numbers/bools/null/undefined key by canonical value (SameValueZero). get returns real `undefined` on a miss; has/delete return real booleans. - Cached heap values are GC roots while cached: a mutable root scanner (gc_register_mutable_root_scanner_named) visits every cached value slot each cycle, so values are marked AND rewritten to their forwarded address under copying evacuation -- fixing a use-after-free. - new LRUCache({ max, ttl, updateAgeOnGet }) is parsed from the NaN-boxed options object; codegen forwards the whole object (dynamic options work) instead of statically extracting only max. ttl uses performance.now for expiry (get/has/peek treat expired as absent; get evicts); updateAgeOnGet refreshes the TTL clock on a live get. peek wired into method dispatch. Adds Rust unit tests (string-key content round-trip, object-value survival across a forced GC cycle, TTL expiry, updateAgeOnGet, eviction at max) -- all green via `cargo test -p perry-ext-lru-cache`. Tracking: #466 (Phase 5 native bindings). PR #7136. --- Cargo.lock | 1 + changelog.d/7136-lru-cache-faithful.md | 37 ++ .../perry-codegen/src/lower_call/builtin.rs | 38 +- .../src/lower_call/native_table/node_misc.rs | 9 + crates/perry-ext-lru-cache/Cargo.toml | 6 + crates/perry-ext-lru-cache/src/lib.rs | 443 +++++++++++++----- crates/perry-ext-lru-cache/src/tests.rs | 269 +++++++++++ 7 files changed, 649 insertions(+), 154 deletions(-) create mode 100644 changelog.d/7136-lru-cache-faithful.md create mode 100644 crates/perry-ext-lru-cache/src/tests.rs diff --git a/Cargo.lock b/Cargo.lock index 09786f5717..a9f186923e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5911,6 +5911,7 @@ version = "0.5.1265" dependencies = [ "lru", "perry-ffi", + "perry-runtime", ] [[package]] diff --git a/changelog.d/7136-lru-cache-faithful.md b/changelog.d/7136-lru-cache-faithful.md new file mode 100644 index 0000000000..40531293ce --- /dev/null +++ b/changelog.d/7136-lru-cache-faithful.md @@ -0,0 +1,37 @@ +### Fixed + +- **`lru-cache` native binding (`perry-ext-lru-cache`) is now faithful to the + npm `lru-cache` API for real-world usage.** The previous wrapper only handled + numeric (`f64`) keys and values with no TTL, so a cache keyed on strings with + object/string values (e.g. Socket Firewall's `new LRUCache({ max, ttl, + updateAgeOnGet })`) silently misbehaved. Two defects are fixed: + + - **Keys and values are treated as real JS values, not raw `f64` bit + patterns.** String keys now hash and compare by **content** (the NaN-boxed + `StringHeader` is materialized via `js_get_string_pointer_unified` and keyed + on its bytes), so a `get("k")` after `set("k", …)` hits even when the two + `"k"` strings are distinct allocations or an SSO short string vs a heap + string. Number/boolean/null/undefined keys key by canonical value + (SameValueZero: `+0`/`-0` and all `NaN`s unified). + - **Cached heap values are GC roots for as long as they are cached.** A + mutable root scanner (`gc_register_mutable_root_scanner_named`) visits every + cached value slot each GC cycle, so stored objects/strings are marked and + rewritten to their forwarded address under copying evacuation — fixing a + use-after-free where a cached value was collected out from under the cache + (the "value is not a function" class of bug). + +- **Constructor honors the options object.** `new LRUCache({ max, ttl, + updateAgeOnGet })` is parsed by the runtime from the NaN-boxed options object + (codegen now forwards the whole object instead of statically extracting only + `max`, so dynamic/variable options work). `ttl` gives per-entry expiry on the + `performance.now()` clock (`get`/`has`/`peek` treat an expired entry as + absent; `get` evicts it); `updateAgeOnGet` resets an entry's TTL clock on a + live `get`. `peek` is now wired into method dispatch. + + Not yet implemented (unchanged ABI carries only `(key, value)`): + `maxSize`/`sizeCalculation`, `dispose`/`disposeAfter`, `fetch`, `allowStale`, + per-call option objects, and the iterator surface. Object-identity keys are + supported by pointer identity but are not tracked across a GC relocation; + primitive keys are the GC-safe path. + + Tracking: #466 (Phase 5 native bindings). PR #7136. diff --git a/crates/perry-codegen/src/lower_call/builtin.rs b/crates/perry-codegen/src/lower_call/builtin.rs index e94416c701..acbebc6ded 100644 --- a/crates/perry-codegen/src/lower_call/builtin.rs +++ b/crates/perry-codegen/src/lower_call/builtin.rs @@ -521,37 +521,21 @@ pub(super) fn lower_builtin_new( let result = ctx.block().call(DOUBLE, runtime_fn, &[(DOUBLE, &opts_box)]); Ok(Some(result)) } - // lru-cache LRUCache — `new LRUCache({ max: N })`. Runtime takes - // a single `max: f64`. Extract the `max` field from the options - // literal (handles both raw `Expr::Object(props)` and Phase 3's - // `Expr::New { __AnonShape_N }` shape via `extract_options_fields`); - // default to 100 when no options literal is detected (matches the - // npm `lru-cache` library's behavior for `new LRUCache()` with - // missing max — it warns + falls back, we just fall back). + // lru-cache LRUCache — `new LRUCache({ max, ttl, updateAgeOnGet })`. + // The runtime parses the whole NaN-boxed options object itself + // (`js_lru_cache_new(options: f64)`), so we just lower the options + // argument and hand it through — no static field extraction, which + // means dynamic/variable options objects work too. A missing options + // argument passes `undefined`; the runtime falls back to max=100 + // (matching npm `lru-cache`'s bounded-cache default). "LRUCache" => { - let max_val = if let Some(opts_arg) = args.first() { - let mut found_max: Option = None; - if let Some(props) = extract_options_fields(ctx, opts_arg) { - for (k, vexpr) in &props { - if k == "max" { - found_max = Some(lower_expr(ctx, vexpr)?); - } else { - // Lower other fields for side effects (e.g. ttl - // option's setter calls). - let _ = lower_expr(ctx, vexpr)?; - } - } - } else { - // Non-literal arg (variable, dynamic shape) — lower for - // side effects only; cannot extract max statically. - let _ = lower_expr(ctx, opts_arg)?; - } - found_max.unwrap_or_else(|| "100.0".to_string()) + let opts_val = if let Some(opts_arg) = args.first() { + lower_expr(ctx, opts_arg)? } else { - "100.0".to_string() + double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)) }; let blk = ctx.block(); - let handle = blk.call(I64, "js_lru_cache_new", &[(DOUBLE, &max_val)]); + let handle = blk.call(I64, "js_lru_cache_new", &[(DOUBLE, &opts_val)]); Ok(Some(nanbox_pointer_inline(blk, &handle))) } // (`WebSocketServer` is handled by an earlier branch lower in this diff --git a/crates/perry-codegen/src/lower_call/native_table/node_misc.rs b/crates/perry-codegen/src/lower_call/native_table/node_misc.rs index 9363d80127..5e58d13cb1 100644 --- a/crates/perry-codegen/src/lower_call/native_table/node_misc.rs +++ b/crates/perry-codegen/src/lower_call/native_table/node_misc.rs @@ -311,6 +311,15 @@ pub(super) const NODE_MISC_ROWS: &[NativeModSig] = &[ args: &[], ret: NR_F64, }, + NativeModSig { + module: "lru-cache", + has_receiver: true, + method: "peek", + class_filter: None, + runtime: "js_lru_cache_peek", + args: &[NA_F64], + ret: NR_F64, + }, // ========== commander (CLI parsing) ========== // `new Command()` is dispatched separately by `lower_builtin_new` so it // produces a real CommanderHandle instead of an empty placeholder. The diff --git a/crates/perry-ext-lru-cache/Cargo.toml b/crates/perry-ext-lru-cache/Cargo.toml index 3d7f988bfe..acfd7b03f2 100644 --- a/crates/perry-ext-lru-cache/Cargo.toml +++ b/crates/perry-ext-lru-cache/Cargo.toml @@ -17,3 +17,9 @@ lru = "0.18" [dev-dependencies] perry-ffi = { workspace = true, features = ["runtime-link"] } +# Direct handle for the GC-survival test: it forces a minor collection +# (`perry_runtime::gc::gc_collect_minor`) and drives the write-barrier / +# shadow-frame guard, mirroring perry-ext-events' scanner test. `default` + +# `stdlib` keep this copy feature-identical to the shipped runtime (see +# perry-ext-events/Cargo.toml for the #6303 rationale). +perry-runtime = { workspace = true, features = ["default", "stdlib"] } diff --git a/crates/perry-ext-lru-cache/src/lib.rs b/crates/perry-ext-lru-cache/src/lib.rs index bc2aa7f988..682073456f 100644 --- a/crates/perry-ext-lru-cache/src/lib.rs +++ b/crates/perry-ext-lru-cache/src/lib.rs @@ -1,95 +1,347 @@ //! Native bindings for the npm `lru-cache` package. //! -//! First handle-based port under #466 Phase 5 — exercises the -//! `Handle` / `register_handle` / `with_handle_mut` surface that -//! perry-ffi gained in v0.5.x. Functionally identical to -//! `crates/perry-stdlib/src/lru_cache.rs`. +//! Handle-based port under #466 Phase 5 — exercises the +//! `Handle` / `register_handle` / `with_handle_mut` surface plus the +//! perry-ffi GC-root-scanner surface (`gc_register_mutable_root_scanner_named`). //! -//! Keys + values are stored as f64 bit-patterns so the FFI ABI -//! stays homogeneous (every method takes/returns f64). For -//! string-keyed caches the call site NaN-boxes the string pointer -//! into the f64 — same trick perry-stdlib's copy uses. +//! # Values and keys are real JS values, not raw `f64` numbers +//! +//! Perry NaN-boxes every JS value into an `f64`, so the FFI ABI stays +//! homogeneous (every method takes/returns `f64`). But the *contents* +//! are arbitrary JS values, and this wrapper treats them as such: +//! +//! - **String keys hash/compare by CONTENT.** The previous version used +//! `key.to_bits() as i64` as the map key, so two different string +//! allocations holding the same text (or an SSO short string vs a heap +//! string) were treated as different keys and a `cache.get("k")` after +//! `cache.set("k", …)` missed. We materialize the key via +//! `js_get_string_pointer_unified` and key the map on the UTF-8 bytes. +//! +//! - **Stored heap values are GC roots for as long as they are cached.** +//! A cached object/string is otherwise unreachable from the JS shadow +//! stack, so the collector would free it out from under the cache — a +//! use-after-free that surfaces later as "value is not a function" / +//! corrupted reads. We register a mutable root scanner that visits +//! every cached value slot on each GC cycle, so live values are marked +//! AND rewritten to their forwarded address after copying evacuation. +//! +//! # Options +//! +//! `new LRUCache({ max, ttl, updateAgeOnGet })` is parsed from the +//! NaN-boxed options object (mirrors npm's option surface for the parts +//! Socket Firewall and typical callers use): +//! +//! - `max` — capacity; entries past it evict LRU-first (default 100 when +//! absent, so an unconfigured cache still has a bound). +//! - `ttl` — per-entry time-to-live in ms. `get`/`has`/`peek` on an +//! expired entry behave as if it were absent; `get` also evicts it. +//! - `updateAgeOnGet` — on a live `get`, reset the entry's TTL clock so +//! its age restarts from the access (npm semantics). +//! +//! The clock is the runtime's `performance.now()` (`js_performance_now`) +//! — the same monotonic source npm lru-cache uses (`perf_now`), and it +//! honors Perry's mock-timer facility. +//! +//! ## Not (yet) implemented vs npm lru-cache +//! +//! `maxSize`/`sizeCalculation`, `dispose`/`disposeAfter`, `fetch`, +//! `allowStale`, per-call `set`/`get` option objects, and the +//! iterator/`forEach`/`entries` surface are out of scope — the ABI only +//! carries `(key, value)`. **Object-identity keys** (using an object as a +//! key) are supported by pointer identity but are NOT tracked across a +//! GC relocation; primitive keys (string/number/bool) are the faithful, +//! GC-safe path and cover all real usage. use lru::LruCache; -use perry_ffi::{register_handle, with_handle_mut, Handle}; +use perry_ffi::{ + gc_register_mutable_root_scanner_named, iter_handles_of_mut, read_bytes, register_handle, + with_handle_mut, GcRootVisitor, Handle, JsString, JsValue, ObjectHeader, StringHeader, +}; use std::num::NonZeroUsize; +use std::sync::Once; + +const POINTER_MASK: u64 = 0x0000_FFFF_FFFF_FFFF; +const TAG_UNDEFINED: u64 = 0x7FFC_0000_0000_0001; +const TAG_FALSE: u64 = 0x7FFC_0000_0000_0003; +const TAG_TRUE: u64 = 0x7FFC_0000_0000_0004; -/// Wrapper struct so the registry's downcast resolves uniquely -/// (each wrapper crate uses a private newtype to namespace its -/// handle space within the shared registry). +extern "C" { + // Monotonic ms clock — same source npm lru-cache uses (`perf_now`); + // honors Perry's mock timers. + fn js_performance_now() -> f64; + // Materialize any string repr (heap `STRING_TAG` or inline SSO + // `SHORT_STRING_TAG`) into a real `*StringHeader` so we can read bytes. + fn js_get_string_pointer_unified(value: f64) -> i64; + // Read a numeric/boolean option field off the NaN-boxed options object. + fn js_object_get_field_by_name_f64(obj: *const ObjectHeader, key: *const StringHeader) -> f64; + fn js_is_truthy(value: f64) -> i32; +} + +#[inline] +fn undefined() -> f64 { + f64::from_bits(TAG_UNDEFINED) +} + +/// A NaN-boxed JS boolean. npm `has`/`delete` return real booleans, and a +/// NaN-tagged bool round-trips through the `f64`-wide ABI unchanged (same as +/// the object pointers `get` already returns). +#[inline] +fn js_bool(b: bool) -> f64 { + f64::from_bits(if b { TAG_TRUE } else { TAG_FALSE }) +} + +/// An owned, GC-independent map key. +/// +/// Primitives are stored by value/content so a relocation of the caller's +/// JS value never invalidates a stored key. Object keys fall back to +/// pointer identity (see the crate-level "Not implemented" note). +#[derive(Clone, PartialEq, Eq, Hash, Debug)] +enum CacheKey { + /// Canonicalized `f64` bits (+0/-0 unified, all NaNs unified — matching + /// JS `Map` SameValueZero key semantics). + Num(u64), + /// UTF-8 (or raw byte) content of a string key. + Str(Box<[u8]>), + Bool(bool), + Null, + Undefined, + /// Heap pointer identity (lower 48 bits) for object/array/function keys. + Obj(u64), +} + +#[inline] +fn canonical_num_bits(n: f64) -> u64 { + if n == 0.0 { + 0.0f64.to_bits() // unify +0.0 / -0.0 + } else if n.is_nan() { + f64::NAN.to_bits() // unify all NaN payloads + } else { + n.to_bits() + } +} + +/// Derive an owned [`CacheKey`] from a NaN-boxed key value. +fn cache_key(key: f64) -> CacheKey { + let jv = JsValue::from_bits(key.to_bits()); + if jv.is_any_string() { + // Materialize either string repr into a heap header, then copy bytes. + let ptr = unsafe { js_get_string_pointer_unified(key) } as *mut StringHeader; + if !ptr.is_null() { + let handle = unsafe { JsString::from_raw(ptr) }; + if let Some(bytes) = read_bytes(handle) { + return CacheKey::Str(bytes.to_vec().into_boxed_slice()); + } + } + CacheKey::Str(Box::default()) + } else if jv.is_int32() { + CacheKey::Num(canonical_num_bits(jv.to_int32() as f64)) + } else if jv.is_undefined() { + CacheKey::Undefined + } else if jv.is_null() { + CacheKey::Null + } else if jv.is_bool() { + CacheKey::Bool(jv.to_bool()) + } else if jv.is_pointer() { + CacheKey::Obj(key.to_bits() & POINTER_MASK) + } else { + // Real numbers (and anything else numeric) key by canonical bits. + CacheKey::Num(canonical_num_bits(f64::from_bits(key.to_bits()))) + } +} + +/// A cached value plus its optional TTL expiry (ms on the `performance.now` +/// clock). `value_bits` are NaN-boxed JS value bits, GC-rooted by +/// [`scan_lru_roots`] while the entry is live. +struct Entry { + value_bits: u64, + expires_at: Option, +} + +impl Entry { + #[inline] + fn is_expired(&self, now: f64) -> bool { + matches!(self.expires_at, Some(t) if now >= t) + } +} + +/// Wrapper struct so the registry's downcast resolves uniquely. pub struct LruCacheHandle { - cache: LruCache, + cache: LruCache, + /// Default per-entry TTL in ms, or `None` when `ttl` was not set. + ttl_ms: Option, + /// npm `updateAgeOnGet` — refresh an entry's TTL clock on `get`. + update_age_on_get: bool, } impl LruCacheHandle { - pub fn new(max_size: usize) -> Self { + fn new(max_size: usize, ttl_ms: Option, update_age_on_get: bool) -> Self { let size = NonZeroUsize::new(max_size.max(1)).expect("max_size at least 1"); LruCacheHandle { cache: LruCache::new(size), + ttl_ms, + update_age_on_get, } } + + #[inline] + fn expiry_from_now(&self, now: f64) -> Option { + self.ttl_ms.and_then(|ttl| (ttl > 0.0).then_some(now + ttl)) + } } -/// `new LRUCache({ max })` — register a fresh cache and return its -/// handle. `max < 1` or NaN falls back to 100 (the default in the -/// npm package's `Map`-options form). -#[no_mangle] -pub extern "C" fn js_lru_cache_new(max_size: f64) -> Handle { - let max = if max_size.is_nan() || max_size < 1.0 { - 100 +static GC_REGISTERED: Once = Once::new(); + +fn ensure_gc_scanner() { + GC_REGISTERED.call_once(|| { + gc_register_mutable_root_scanner_named("perry-ext-lru-cache", scan_lru_roots); + }); +} + +/// GC root scanner: visit every cached value slot across every live cache +/// handle so the collector marks the referent and, under copying +/// evacuation, rewrites the stored bits to the forwarded address. +fn scan_lru_roots(visitor: &mut GcRootVisitor<'_>) { + iter_handles_of_mut::(|h| { + for (_key, entry) in h.cache.iter_mut() { + visitor.visit_nanbox_u64_slot(&mut entry.value_bits); + } + }); +} + +#[inline] +fn now_ms() -> f64 { + unsafe { js_performance_now() } +} + +/// Read a numeric option field; `None` when absent or non-numeric. +unsafe fn option_number(ptr: *const ObjectHeader, name: &str) -> Option { + let key = perry_ffi::alloc_string(name); + let raw = js_object_get_field_by_name_f64(ptr, key.as_raw()); + let jv = JsValue::from_bits(raw.to_bits()); + if jv.is_int32() { + Some(jv.to_int32() as f64) + } else if jv.is_number() && !raw.is_nan() { + Some(raw) } else { - max_size as usize - }; - register_handle(LruCacheHandle::new(max)) + None + } } -/// `cache.get(key)` — `NaN` if the key isn't present (matches the -/// existing perry-stdlib convention for "undefined" through f64 -/// returns). +/// `new LRUCache(options)` — register a fresh cache and return its handle. +/// +/// `options` is the NaN-boxed options object. `max < 1` / absent falls back +/// to 100 (so an unconfigured cache is still bounded). `ttl` and +/// `updateAgeOnGet` are honored when present. +#[no_mangle] +pub extern "C" fn js_lru_cache_new(options: f64) -> Handle { + ensure_gc_scanner(); + + let mut max = 100usize; + let mut ttl_ms = None; + let mut update_age_on_get = false; + + let jv = JsValue::from_bits(options.to_bits()); + if jv.is_pointer() { + let ptr = jv.as_pointer::(); + if !ptr.is_null() && (ptr as usize) >= 0x1000 { + unsafe { + if let Some(n) = option_number(ptr, "max") { + if n >= 1.0 { + max = n as usize; + } + } + if let Some(n) = option_number(ptr, "ttl") { + if n > 0.0 { + ttl_ms = Some(n); + } + } + let key = perry_ffi::alloc_string("updateAgeOnGet"); + let uaog = js_object_get_field_by_name_f64(ptr, key.as_raw()); + if js_is_truthy(uaog) != 0 { + update_age_on_get = true; + } + } + } + } + + register_handle(LruCacheHandle::new(max, ttl_ms, update_age_on_get)) +} + +/// `cache.get(key)` — returns `undefined` when the key is absent or its +/// entry has expired (an expired entry is evicted). Bumps LRU recency; when +/// `updateAgeOnGet` is set, also resets the entry's TTL clock. #[no_mangle] pub extern "C" fn js_lru_cache_get(handle: Handle, key: f64) -> f64 { - let key_bits = key.to_bits() as i64; - with_handle_mut::(handle, |h| h.cache.get(&key_bits).copied()) - .flatten() - .unwrap_or(f64::NAN) + let k = cache_key(key); + let now = now_ms(); + with_handle_mut::(handle, |h| { + let refresh = h.update_age_on_get; + let new_expiry = h.expiry_from_now(now); + let outcome = match h.cache.get_mut(&k) { + Some(entry) => { + if entry.is_expired(now) { + None // expired → evict below + } else { + if refresh { + entry.expires_at = new_expiry; + } + Some(entry.value_bits) + } + } + None => return undefined(), + }; + match outcome { + Some(bits) => f64::from_bits(bits), + None => { + h.cache.pop(&k); + undefined() + } + } + }) + .unwrap_or_else(undefined) } /// `cache.set(key, value)` — returns the handle for chaining. #[no_mangle] pub extern "C" fn js_lru_cache_set(handle: Handle, key: f64, value: f64) -> Handle { - let key_bits = key.to_bits() as i64; + let k = cache_key(key); + let now = now_ms(); with_handle_mut::(handle, |h| { - h.cache.put(key_bits, value); + let expires_at = h.expiry_from_now(now); + h.cache.put( + k, + Entry { + value_bits: value.to_bits(), + expires_at, + }, + ); }); handle } -/// `cache.has(key)` → `1.0` / `0.0`. +/// `cache.has(key)` → `true` / `false`. Does not bump recency and does not +/// refresh age; an expired entry reads as absent (but is not evicted here, +/// matching npm's lazy purge). #[no_mangle] pub extern "C" fn js_lru_cache_has(handle: Handle, key: f64) -> f64 { - let key_bits = key.to_bits() as i64; - with_handle_mut::(handle, |h| { - if h.cache.contains(&key_bits) { - 1.0 - } else { - 0.0 - } - }) - .unwrap_or(0.0) + let k = cache_key(key); + let now = now_ms(); + js_bool( + with_handle_mut::(handle, |h| { + matches!(h.cache.peek(&k), Some(entry) if !entry.is_expired(now)) + }) + .unwrap_or(false), + ) } -/// `cache.delete(key)` → `1.0` if removed, `0.0` if absent. +/// `cache.delete(key)` → `true` if removed, `false` if absent. #[no_mangle] pub extern "C" fn js_lru_cache_delete(handle: Handle, key: f64) -> f64 { - let key_bits = key.to_bits() as i64; - with_handle_mut::(handle, |h| { - if h.cache.pop(&key_bits).is_some() { - 1.0 - } else { - 0.0 - } - }) - .unwrap_or(0.0) + let k = cache_key(key); + js_bool( + with_handle_mut::(handle, |h| h.cache.pop(&k).is_some()) + .unwrap_or(false), + ) } /// `cache.clear()` — drops every entry. @@ -104,82 +356,19 @@ pub extern "C" fn js_lru_cache_size(handle: Handle) -> f64 { with_handle_mut::(handle, |h| h.cache.len() as f64).unwrap_or(0.0) } -/// `cache.peek(key)` — like `get` but doesn't bump recency. +/// `cache.peek(key)` — like `get` but doesn't bump recency and doesn't +/// refresh age. Returns `undefined` for an absent or expired entry (and +/// leaves an expired entry in place, matching npm's lazy purge). #[no_mangle] pub extern "C" fn js_lru_cache_peek(handle: Handle, key: f64) -> f64 { - let key_bits = key.to_bits() as i64; - with_handle_mut::(handle, |h| h.cache.peek(&key_bits).copied()) - .flatten() - .unwrap_or(f64::NAN) + let k = cache_key(key); + let now = now_ms(); + with_handle_mut::(handle, |h| match h.cache.peek(&k) { + Some(entry) if !entry.is_expired(now) => f64::from_bits(entry.value_bits), + _ => undefined(), + }) + .unwrap_or_else(undefined) } #[cfg(test)] -mod tests { - use super::*; - - #[test] - fn basic_set_get_round_trip() { - let h = js_lru_cache_new(8.0); - assert_ne!(h, perry_ffi::INVALID_HANDLE); - js_lru_cache_set(h, 1.0, 100.0); - assert_eq!(js_lru_cache_get(h, 1.0), 100.0); - assert_eq!(js_lru_cache_has(h, 1.0), 1.0); - assert_eq!(js_lru_cache_size(h), 1.0); - } - - #[test] - fn lru_eviction_at_max_size() { - let h = js_lru_cache_new(3.0); - for i in 0..3 { - js_lru_cache_set(h, i as f64, (i * 10) as f64); - } - assert_eq!(js_lru_cache_size(h), 3.0); - // Adding a 4th evicts the oldest (key=0). - js_lru_cache_set(h, 99.0, 990.0); - assert_eq!(js_lru_cache_has(h, 0.0), 0.0); - assert_eq!(js_lru_cache_has(h, 99.0), 1.0); - } - - #[test] - fn delete_and_clear() { - let h = js_lru_cache_new(8.0); - js_lru_cache_set(h, 1.0, 100.0); - js_lru_cache_set(h, 2.0, 200.0); - assert_eq!(js_lru_cache_delete(h, 1.0), 1.0); - assert_eq!(js_lru_cache_delete(h, 1.0), 0.0); // already gone - assert_eq!(js_lru_cache_size(h), 1.0); - js_lru_cache_clear(h); - assert_eq!(js_lru_cache_size(h), 0.0); - } - - #[test] - fn peek_does_not_bump_recency() { - let h = js_lru_cache_new(2.0); - js_lru_cache_set(h, 1.0, 100.0); - js_lru_cache_set(h, 2.0, 200.0); - // `peek(1)` reads but doesn't bump recency. Now adding key - // 3 should evict key 1 (oldest), not key 2. - let _ = js_lru_cache_peek(h, 1.0); - js_lru_cache_set(h, 3.0, 300.0); - assert_eq!(js_lru_cache_has(h, 1.0), 0.0); - assert_eq!(js_lru_cache_has(h, 2.0), 1.0); - assert_eq!(js_lru_cache_has(h, 3.0), 1.0); - } - - #[test] - fn missing_key_returns_nan() { - let h = js_lru_cache_new(8.0); - let v = js_lru_cache_get(h, 42.0); - assert!(v.is_nan(), "expected NaN, got {}", v); - } - - #[test] - fn invalid_handle_is_no_op() { - // Operating on a never-registered handle should return - // sensible defaults, not panic. - assert!(js_lru_cache_get(99_999, 0.0).is_nan()); - assert_eq!(js_lru_cache_has(99_999, 0.0), 0.0); - assert_eq!(js_lru_cache_size(99_999), 0.0); - js_lru_cache_clear(99_999); // no panic - } -} +mod tests; diff --git a/crates/perry-ext-lru-cache/src/tests.rs b/crates/perry-ext-lru-cache/src/tests.rs new file mode 100644 index 0000000000..700756ae8e --- /dev/null +++ b/crates/perry-ext-lru-cache/src/tests.rs @@ -0,0 +1,269 @@ +//! Unit tests for the lru-cache wrapper. +//! +//! Every test links `perry-runtime` (the `runtime-link` dev-dep feature) +//! because the wrapper reaches the runtime for its clock +//! (`js_performance_now`) and string materialization +//! (`js_get_string_pointer_unified`). The GC-survival test additionally +//! drives `perry-runtime`'s collector directly. + +use super::*; +use perry_ffi::{alloc_string, nanbox_string_bits, JsValue}; +use std::sync::Mutex; + +/// Serializes the tests that touch global GC state. +static GC_TEST_LOCK: Mutex<()> = Mutex::new(()); + +/// NaN-boxed `f64` for a freshly allocated JS string with `text`. +fn string_value(text: &str) -> f64 { + let s = alloc_string(text); + assert!(!s.is_null(), "alloc_string returned null"); + f64::from_bits(nanbox_string_bits(s.as_raw())) +} + +/// npm `has`/`delete` return NaN-boxed JS booleans — decode one. +fn is_true(v: f64) -> bool { + v.to_bits() == TAG_TRUE +} + +/// Read the string content behind a NaN-boxed value produced by the cache. +fn read_string_value(value: f64) -> Option { + let ptr = unsafe { js_get_string_pointer_unified(value) } as *mut StringHeader; + if ptr.is_null() { + return None; + } + let handle = unsafe { JsString::from_raw(ptr) }; + read_bytes(handle).map(|b| String::from_utf8_lossy(b).into_owned()) +} + +// ── pure key-derivation logic (no runtime clock) ───────────────────── + +#[test] +fn canonical_num_unifies_zero_and_nan() { + assert_eq!(canonical_num_bits(0.0), canonical_num_bits(-0.0)); + assert_eq!( + canonical_num_bits(f64::NAN), + canonical_num_bits(f64::from_bits(0x7FF8_0000_0000_0001)) + ); + assert_ne!(canonical_num_bits(1.0), canonical_num_bits(2.0)); +} + +#[test] +fn cache_key_primitive_variants() { + assert_eq!(cache_key(3.5), CacheKey::Num(canonical_num_bits(3.5))); + assert_eq!( + cache_key(f64::from_bits(JsValue::from_int32(7).bits())), + CacheKey::Num(canonical_num_bits(7.0)) + ); + assert_eq!(cache_key(f64::from_bits(JsValue::TRUE.bits())), CacheKey::Bool(true)); + assert_eq!(cache_key(f64::from_bits(JsValue::NULL.bits())), CacheKey::Null); + assert_eq!( + cache_key(f64::from_bits(JsValue::UNDEFINED.bits())), + CacheKey::Undefined + ); +} + +// ── numeric-key behaviour (parity with the old surface) ────────────── + +#[test] +fn basic_set_get_round_trip() { + let h = js_lru_cache_new(f64::from_bits(TAG_UNDEFINED)); + assert_ne!(h, perry_ffi::INVALID_HANDLE); + js_lru_cache_set(h, 1.0, 100.0); + assert_eq!(js_lru_cache_get(h, 1.0), 100.0); + assert!(is_true(js_lru_cache_has(h, 1.0))); + assert_eq!(js_lru_cache_size(h), 1.0); + perry_ffi::drop_handle(h); +} + +#[test] +fn lru_eviction_at_max_size() { + // max:3 via a directly-built handle (options-object parsing is covered + // end-to-end by the compiled smoke program, not reachable from a unit + // test without allocating a real JS object). + let h = perry_ffi::register_handle(LruCacheHandle::new(3, None, false)); + for i in 0..3 { + js_lru_cache_set(h, i as f64, (i * 10) as f64); + } + assert_eq!(js_lru_cache_size(h), 3.0); + // Adding a 4th evicts the LRU (key=0). + js_lru_cache_set(h, 99.0, 990.0); + assert_eq!(js_lru_cache_size(h), 3.0); + assert!(!is_true(js_lru_cache_has(h, 0.0))); + assert!(is_true(js_lru_cache_has(h, 99.0))); + perry_ffi::drop_handle(h); +} + +#[test] +fn delete_and_clear() { + let h = js_lru_cache_new(f64::from_bits(TAG_UNDEFINED)); + js_lru_cache_set(h, 1.0, 100.0); + js_lru_cache_set(h, 2.0, 200.0); + assert!(is_true(js_lru_cache_delete(h, 1.0))); + assert!(!is_true(js_lru_cache_delete(h, 1.0))); // already gone + assert_eq!(js_lru_cache_size(h), 1.0); + js_lru_cache_clear(h); + assert_eq!(js_lru_cache_size(h), 0.0); + perry_ffi::drop_handle(h); +} + +#[test] +fn peek_does_not_bump_recency() { + let h = perry_ffi::register_handle(LruCacheHandle::new(2, None, false)); + js_lru_cache_set(h, 1.0, 100.0); + js_lru_cache_set(h, 2.0, 200.0); + // peek(1) reads but does not bump recency, so adding key 3 evicts key 1. + let _ = js_lru_cache_peek(h, 1.0); + js_lru_cache_set(h, 3.0, 300.0); + assert!(!is_true(js_lru_cache_has(h, 1.0))); + assert!(is_true(js_lru_cache_has(h, 2.0))); + assert!(is_true(js_lru_cache_has(h, 3.0))); + perry_ffi::drop_handle(h); +} + +#[test] +fn missing_key_returns_undefined() { + let h = js_lru_cache_new(f64::from_bits(TAG_UNDEFINED)); + let v = js_lru_cache_get(h, 42.0); + assert_eq!(v.to_bits(), TAG_UNDEFINED, "missing key must be undefined"); + perry_ffi::drop_handle(h); +} + +#[test] +fn invalid_handle_is_no_op() { + assert_eq!(js_lru_cache_get(99_999, 0.0).to_bits(), TAG_UNDEFINED); + assert!(!is_true(js_lru_cache_has(99_999, 0.0))); + assert_eq!(js_lru_cache_size(99_999), 0.0); + js_lru_cache_clear(99_999); // no panic +} + +// ── string keys hash/compare by content (the core fix) ─────────────── + +#[test] +fn string_key_round_trip_by_content() { + let h = js_lru_cache_new(f64::from_bits(TAG_UNDEFINED)); + // Store under one string allocation… + js_lru_cache_set(h, string_value("socket-firewall"), 4242.0); + // …read back through a *different* allocation of the same text. The old + // pointer-bits keying missed here; content keying hits. + assert_eq!(js_lru_cache_get(h, string_value("socket-firewall")), 4242.0); + assert!(is_true(js_lru_cache_has(h, string_value("socket-firewall")))); + assert!(!is_true(js_lru_cache_has(h, string_value("other")))); + assert_eq!(js_lru_cache_size(h), 1.0); + perry_ffi::drop_handle(h); +} + +#[test] +fn string_key_object_value_round_trip() { + let h = js_lru_cache_new(f64::from_bits(TAG_UNDEFINED)); + js_lru_cache_set(h, string_value("payload"), string_value("hello-world-value")); + let got = js_lru_cache_get(h, string_value("payload")); + assert_eq!(read_string_value(got).as_deref(), Some("hello-world-value")); + perry_ffi::drop_handle(h); +} + +// ── TTL + updateAgeOnGet ───────────────────────────────────────────── + +#[test] +fn ttl_expiry_evicts_on_get() { + let h = perry_ffi::register_handle(LruCacheHandle::new(10, Some(20.0), false)); + js_lru_cache_set(h, 1.0, 111.0); + assert_eq!(js_lru_cache_get(h, 1.0), 111.0); + std::thread::sleep(std::time::Duration::from_millis(60)); + // Expired: get returns undefined AND evicts. + assert_eq!(js_lru_cache_get(h, 1.0).to_bits(), TAG_UNDEFINED); + assert_eq!(js_lru_cache_size(h), 0.0, "expired entry evicted by get"); + perry_ffi::drop_handle(h); +} + +#[test] +fn has_and_peek_report_expired_as_absent() { + let h = perry_ffi::register_handle(LruCacheHandle::new(10, Some(20.0), false)); + js_lru_cache_set(h, 1.0, 111.0); + std::thread::sleep(std::time::Duration::from_millis(60)); + assert!(!is_true(js_lru_cache_has(h, 1.0))); + assert_eq!(js_lru_cache_peek(h, 1.0).to_bits(), TAG_UNDEFINED); + perry_ffi::drop_handle(h); +} + +#[test] +fn update_age_on_get_refreshes_ttl() { + let h = perry_ffi::register_handle(LruCacheHandle::new(10, Some(80.0), true)); + js_lru_cache_set(h, 1.0, 111.0); + // Halfway through the TTL, a get refreshes the clock. + std::thread::sleep(std::time::Duration::from_millis(50)); + assert_eq!(js_lru_cache_get(h, 1.0), 111.0); + // Another half-TTL later the entry is still live *because* it was + // refreshed (without updateAgeOnGet it would have expired at ~80ms). + std::thread::sleep(std::time::Duration::from_millis(50)); + assert_eq!(js_lru_cache_get(h, 1.0), 111.0, "get refreshed the TTL"); + perry_ffi::drop_handle(h); +} + +#[test] +fn no_update_age_on_get_lets_ttl_expire() { + let h = perry_ffi::register_handle(LruCacheHandle::new(10, Some(80.0), false)); + js_lru_cache_set(h, 1.0, 111.0); + std::thread::sleep(std::time::Duration::from_millis(50)); + assert_eq!(js_lru_cache_get(h, 1.0), 111.0); // still live, no refresh + std::thread::sleep(std::time::Duration::from_millis(50)); + // ~100ms since set, TTL was 80ms and never refreshed → expired. + assert_eq!(js_lru_cache_get(h, 1.0).to_bits(), TAG_UNDEFINED); + perry_ffi::drop_handle(h); +} + +// ── options-object parsing ─────────────────────────────────────────── + +#[test] +fn new_without_options_defaults_to_100() { + let h = js_lru_cache_new(f64::from_bits(TAG_UNDEFINED)); + // TTL/updateAgeOnGet default off… + let opts = with_handle_mut::(h, |c| (c.ttl_ms, c.update_age_on_get)) + .unwrap(); + assert_eq!(opts, (None, false)); + // …and the capacity defaults to 100 (the 101st insert evicts). + for i in 0..101 { + js_lru_cache_set(h, i as f64, i as f64); + } + assert_eq!(js_lru_cache_size(h), 100.0); + assert!(!is_true(js_lru_cache_has(h, 0.0)), "oldest entry evicted at cap 100"); + perry_ffi::drop_handle(h); +} + +// ── GC rooting: a cached heap value survives a collection ──────────── + +#[test] +fn cached_value_survives_gc_cycle() { + let _lock = GC_TEST_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + + // Match the runtime's evacuation preconditions: write barriers active + // and a live shadow frame (mirrors perry-ext-events' scanner test). + perry_runtime::gc::js_gc_write_barriers_emitted(1); + let frame = perry_runtime::gc::js_shadow_frame_push(0); + ensure_gc_scanner(); + + let h = js_lru_cache_new(f64::from_bits(TAG_UNDEFINED)); + // A >5-byte string forces the heap `StringHeader` repr (not inline SSO), + // so it is a real collectable allocation. Its ONLY root is the cache. + js_lru_cache_set(h, 1.0, string_value("value-object-1234567890")); + + // Reclaims unrooted nursery allocations and evacuates rooted survivors. + let _ = perry_runtime::gc::gc_collect_minor(); + + let got = js_lru_cache_get(h, 1.0); + assert_ne!( + got.to_bits(), + TAG_UNDEFINED, + "cached value was collected — root scanner did not keep it alive" + ); + assert_eq!( + read_string_value(got).as_deref(), + Some("value-object-1234567890"), + "cached value corrupted across GC — slot was not rewritten to the forwarded address" + ); + + perry_ffi::drop_handle(h); + perry_runtime::gc::js_shadow_frame_pop(frame); + perry_runtime::gc::js_gc_write_barriers_emitted(0); +} From f6675e8183183acca44477ffb84447ef9ab584ef Mon Sep 17 00:00:00 2001 From: jdalton Date: Fri, 31 Jul 2026 23:10:22 -0400 Subject: [PATCH 2/8] chore(lru-cache): describe callers generically and use a neutral test key --- changelog.d/7136-lru-cache-faithful.md | 2 +- crates/perry-ext-lru-cache/src/lib.rs | 2 +- crates/perry-ext-lru-cache/src/tests.rs | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/changelog.d/7136-lru-cache-faithful.md b/changelog.d/7136-lru-cache-faithful.md index 40531293ce..4a86b3da9b 100644 --- a/changelog.d/7136-lru-cache-faithful.md +++ b/changelog.d/7136-lru-cache-faithful.md @@ -3,7 +3,7 @@ - **`lru-cache` native binding (`perry-ext-lru-cache`) is now faithful to the npm `lru-cache` API for real-world usage.** The previous wrapper only handled numeric (`f64`) keys and values with no TTL, so a cache keyed on strings with - object/string values (e.g. Socket Firewall's `new LRUCache({ max, ttl, + object/string values (e.g. a typical caller's `new LRUCache({ max, ttl, updateAgeOnGet })`) silently misbehaved. Two defects are fixed: - **Keys and values are treated as real JS values, not raw `f64` bit diff --git a/crates/perry-ext-lru-cache/src/lib.rs b/crates/perry-ext-lru-cache/src/lib.rs index 682073456f..2c5e5a6134 100644 --- a/crates/perry-ext-lru-cache/src/lib.rs +++ b/crates/perry-ext-lru-cache/src/lib.rs @@ -29,7 +29,7 @@ //! //! `new LRUCache({ max, ttl, updateAgeOnGet })` is parsed from the //! NaN-boxed options object (mirrors npm's option surface for the parts -//! Socket Firewall and typical callers use): +//! typical callers use): //! //! - `max` — capacity; entries past it evict LRU-first (default 100 when //! absent, so an unconfigured cache still has a bound). diff --git a/crates/perry-ext-lru-cache/src/tests.rs b/crates/perry-ext-lru-cache/src/tests.rs index 700756ae8e..4be9d62a2c 100644 --- a/crates/perry-ext-lru-cache/src/tests.rs +++ b/crates/perry-ext-lru-cache/src/tests.rs @@ -142,11 +142,11 @@ fn invalid_handle_is_no_op() { fn string_key_round_trip_by_content() { let h = js_lru_cache_new(f64::from_bits(TAG_UNDEFINED)); // Store under one string allocation… - js_lru_cache_set(h, string_value("socket-firewall"), 4242.0); + js_lru_cache_set(h, string_value("cache-key"), 4242.0); // …read back through a *different* allocation of the same text. The old // pointer-bits keying missed here; content keying hits. - assert_eq!(js_lru_cache_get(h, string_value("socket-firewall")), 4242.0); - assert!(is_true(js_lru_cache_has(h, string_value("socket-firewall")))); + assert_eq!(js_lru_cache_get(h, string_value("cache-key")), 4242.0); + assert!(is_true(js_lru_cache_has(h, string_value("cache-key")))); assert!(!is_true(js_lru_cache_has(h, string_value("other")))); assert_eq!(js_lru_cache_size(h), 1.0); perry_ffi::drop_handle(h); From 39fb57ef867381d588a6fc6169d93f4c56321179 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 1 Aug 2026 07:38:30 +0200 Subject: [PATCH 3/8] fix(lru-cache): npm-faithful option validation, subject-live GC test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes the review of #7136. `option_number` accepted any finite `max` and `n as usize` saturated, so `new LRUCache({ max: 1e12 })` handed the backing map a 10^12-bucket reservation and killed the process with no JS-visible error. Rather than invent a clamp, every constructor case was measured against `lru-cache@11.5.2` on the repo's pinned oracle (Node 26.5.1) and is now reproduced message for message: the `max`/`ttl` "must be a … integer" TypeErrors, the "At least one of max, maxSize, or ttl is required" TypeError, npm's `Array.from({ length: max })` RangeError past the JS array-length limit, and its `getUintArray` "invalid max value: " Error past MAX_SAFE_INTEGER. `new LRUCache()` / `new LRUCache(100)` therefore now throw as they do on npm instead of falling through to a silent max=100; the docs claim to the contrary is updated. The backing map grows lazily (`unbounded()` + `resize()`) instead of reserving `max` buckets up front, so a large-but-legal `max` constructs instantly here where npm OOMs Node. That is the only divergence and it is unobservable except by not running out of memory. Also from the review: - The GC-survival test asserted only that the cached value was still readable, which a non-moving collection satisfies without ever exercising the scanner's forwarding-pointer rewrite — the #6942/#6946 failure mode. It now asserts the value's address actually changed across the minor, so the test fails rather than silently covering nothing if the collection stops copying. Measured: the value moves (0x20000520008 -> 0x20000780008). - That test's write-barrier + shadow-frame teardown moved into a `Drop` guard, so a failing assertion can no longer leak process-global GC state into every later test in the binary. - A string key whose bytes cannot be materialized gets its own `CacheKey::UnresolvedStr` variant instead of aliasing `Str(b"")`, so a failed read can never collide with the real empty-string key. - The `LRUCache` constructor arm lowers its tail arguments for side effects, matching every neighbouring arm. - The TTL tests run a 3:1 TTL-to-sleep margin instead of 80ms/50ms. --- changelog.d/7136-lru-cache-faithful.md | 43 ++- .../perry-codegen/src/lower_call/builtin.rs | 10 +- crates/perry-ext-lru-cache/src/lib.rs | 276 +++++++++++++---- crates/perry-ext-lru-cache/src/tests.rs | 289 ++++++++++++++++-- docs/src/stdlib/other.md | 10 +- 5 files changed, 535 insertions(+), 93 deletions(-) diff --git a/changelog.d/7136-lru-cache-faithful.md b/changelog.d/7136-lru-cache-faithful.md index 4a86b3da9b..d6ab979ea1 100644 --- a/changelog.d/7136-lru-cache-faithful.md +++ b/changelog.d/7136-lru-cache-faithful.md @@ -28,10 +28,47 @@ absent; `get` evicts it); `updateAgeOnGet` resets an entry's TTL clock on a live `get`. `peek` is now wired into method dispatch. +- **Constructor options are validated exactly as npm validates them, so a + bad `max` throws instead of aborting the process.** `option_number` + accepted any finite number and `n as usize` saturated, handing the backing + map a capacity request it could not satisfy: `new LRUCache({ max: 1e12 })` + reserved a 10^12-bucket table and killed the process with no JS-visible + error. Rather than invent a bound, every case was measured against + `lru-cache@11.5.2` on the pinned oracle (Node 26.5.1) and reproduced + message for message: + + | `new LRUCache(…)` | throws | + |---|---| + | `()` | `TypeError: Cannot read properties of undefined (reading 'max')` | + | `(null)` | `TypeError: Cannot read properties of null (reading 'max')` | + | `(5)`, `("x")`, `({})`, `({ max: 0 })`, `({ max: -0 })` | `TypeError: At least one of max, maxSize, or ttl is required` | + | `({ max: -1 \| 1.5 \| Infinity \| NaN \| "3" \| true \| null })` | `TypeError: max option must be a nonnegative integer` | + | `({ max: 2**32 })` … `({ max: MAX_SAFE_INTEGER })` | `RangeError: Invalid array length` | + | `({ max: 2**53 })`, `({ max: 1e300 })` | `Error: invalid max value: ` | + | `({ max: 3, ttl: -5 \| 1.5 \| Infinity \| "5" })` | `TypeError: ttl must be a positive integer if specified` | + + The two upper bounds are npm's own: it builds its index arrays with + `Array.from({ length: max })` (past the JS array-length limit that is a + `RangeError`) after a `getUintArray(max)` lookup that returns `null` past + `Number.MAX_SAFE_INTEGER` (a plain `Error`). **This is a behavior change + for `new LRUCache()` and `new LRUCache(100)`**, which previously fell + through to a silent `max=100`; npm throws for both, and so does Perry now. + `max: 0` together with a `ttl` is npm's legal unbounded cache and is + supported. + + Where Perry deliberately differs: the backing map grows lazily instead of + reserving `max` buckets up front, so a large-but-legal `max` (`1e8`, say) + constructs instantly here where npm OOMs Node. Nothing observes that + except by not running out of memory. + Not yet implemented (unchanged ABI carries only `(key, value)`): `maxSize`/`sizeCalculation`, `dispose`/`disposeAfter`, `fetch`, `allowStale`, - per-call option objects, and the iterator surface. Object-identity keys are - supported by pointer identity but are not tracked across a GC relocation; - primitive keys are the GC-safe path. + per-call option objects, and the iterator surface. Because `maxSize` is + unimplemented it also does not satisfy npm's "at least one of max, maxSize, + or ttl" requirement — a `maxSize`-only cache constructs on npm but throws + here, which fails loudly instead of yielding a silently unbounded cache. + npm's `UnboundedCacheWarning` for `ttl`-only caches is not emitted. Object- + identity keys are supported by pointer identity but are not tracked across a + GC relocation; primitive keys are the GC-safe path. Tracking: #466 (Phase 5 native bindings). PR #7136. diff --git a/crates/perry-codegen/src/lower_call/builtin.rs b/crates/perry-codegen/src/lower_call/builtin.rs index acbebc6ded..0718167d87 100644 --- a/crates/perry-codegen/src/lower_call/builtin.rs +++ b/crates/perry-codegen/src/lower_call/builtin.rs @@ -526,14 +526,20 @@ pub(super) fn lower_builtin_new( // (`js_lru_cache_new(options: f64)`), so we just lower the options // argument and hand it through — no static field extraction, which // means dynamic/variable options objects work too. A missing options - // argument passes `undefined`; the runtime falls back to max=100 - // (matching npm `lru-cache`'s bounded-cache default). + // argument passes `undefined`, which the runtime rejects with the + // same `TypeError` npm's constructor destructuring raises. "LRUCache" => { let opts_val = if let Some(opts_arg) = args.first() { lower_expr(ctx, opts_arg)? } else { double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)) }; + // npm's constructor ignores everything past the options object, + // but the arguments are still evaluated — lower the tail for its + // side effects so `new LRUCache(opts, f())` still calls `f`. + for arg in args.iter().skip(1) { + let _ = lower_expr(ctx, arg)?; + } let blk = ctx.block(); let handle = blk.call(I64, "js_lru_cache_new", &[(DOUBLE, &opts_val)]); Ok(Some(nanbox_pointer_inline(blk, &handle))) diff --git a/crates/perry-ext-lru-cache/src/lib.rs b/crates/perry-ext-lru-cache/src/lib.rs index 2c5e5a6134..808c9c8743 100644 --- a/crates/perry-ext-lru-cache/src/lib.rs +++ b/crates/perry-ext-lru-cache/src/lib.rs @@ -31,8 +31,8 @@ //! NaN-boxed options object (mirrors npm's option surface for the parts //! typical callers use): //! -//! - `max` — capacity; entries past it evict LRU-first (default 100 when -//! absent, so an unconfigured cache still has a bound). +//! - `max` — capacity; entries past it evict LRU-first. `0`/absent means +//! unbounded, which npm only permits together with a `ttl`. //! - `ttl` — per-entry time-to-live in ms. `get`/`has`/`peek` on an //! expired entry behave as if it were absent; `get` also evicts it. //! - `updateAgeOnGet` — on a live `get`, reset the entry's TTL clock so @@ -42,20 +42,50 @@ //! — the same monotonic source npm lru-cache uses (`perf_now`), and it //! honors Perry's mock-timer facility. //! +//! ## Option validation is npm's, measured — not invented +//! +//! npm `lru-cache` rejects bad `max`/`ttl` loudly, and the exact errors +//! are the contract a caller writes `try`/`catch` against. Every case +//! below was measured against `lru-cache@11.5.2` on the pinned oracle +//! (Node 26.5.1) and is reproduced here, message for message: +//! +//! | `new LRUCache(…)` | throws | +//! |---|---| +//! | `()` | `TypeError: Cannot read properties of undefined (reading 'max')` | +//! | `(null)` | `TypeError: Cannot read properties of null (reading 'max')` | +//! | `(5)`, `("x")`, `({})`, `({ max: 0 })` | `TypeError: At least one of max, maxSize, or ttl is required` | +//! | `({ max: -1 \| 1.5 \| Infinity \| NaN \| "3" \| true \| null })` | `TypeError: max option must be a nonnegative integer` | +//! | `({ max: 2**32 })` … up to `MAX_SAFE_INTEGER` | `RangeError: Invalid array length` | +//! | `({ max: 2**53 })`, `({ max: 1e300 })` | `Error: invalid max value: ` | +//! | `({ max: 3, ttl: -5 \| 1.5 \| Infinity \| "5" })` | `TypeError: ttl must be a positive integer if specified` | +//! +//! The two upper bounds are not arbitrary: npm builds its index arrays +//! with `Array.from({ length: max })` (so `max` past the JS array-length +//! limit is a `RangeError`) after an `getUintArray(max)` lookup that +//! returns `null` past `Number.MAX_SAFE_INTEGER` (a plain `Error`). +//! Reproducing them is what keeps `new LRUCache({ max: 1e12 })` from +//! reaching an allocator with a 10^12-entry reservation. +//! //! ## Not (yet) implemented vs npm lru-cache //! //! `maxSize`/`sizeCalculation`, `dispose`/`disposeAfter`, `fetch`, //! `allowStale`, per-call `set`/`get` option objects, and the //! iterator/`forEach`/`entries` surface are out of scope — the ABI only -//! carries `(key, value)`. **Object-identity keys** (using an object as a -//! key) are supported by pointer identity but are NOT tracked across a -//! GC relocation; primitive keys (string/number/bool) are the faithful, -//! GC-safe path and cover all real usage. +//! carries `(key, value)`. Because `maxSize` is unimplemented it also does +//! not satisfy npm's "at least one of max, maxSize, or ttl" requirement: +//! a `maxSize`-only cache constructs on npm but throws here, which is the +//! loud failure rather than a silently unbounded cache. npm's +//! `UnboundedCacheWarning` (`ttl`-only caches) is likewise not emitted. +//! **Object-identity keys** (using an object as a key) are supported by +//! pointer identity but are NOT tracked across a GC relocation; primitive +//! keys (string/number/bool) are the faithful, GC-safe path and cover all +//! real usage. use lru::LruCache; use perry_ffi::{ gc_register_mutable_root_scanner_named, iter_handles_of_mut, read_bytes, register_handle, - with_handle_mut, GcRootVisitor, Handle, JsString, JsValue, ObjectHeader, StringHeader, + throw_with_code, with_handle_mut, ErrorKind, GcRootVisitor, Handle, JsString, JsValue, + ObjectHeader, StringHeader, }; use std::num::NonZeroUsize; use std::sync::Once; @@ -65,6 +95,15 @@ const TAG_UNDEFINED: u64 = 0x7FFC_0000_0000_0001; const TAG_FALSE: u64 = 0x7FFC_0000_0000_0003; const TAG_TRUE: u64 = 0x7FFC_0000_0000_0004; +/// Largest `max` npm can build its index arrays for +/// (`Array.from({ length: max })`, i.e. the JS array-length limit). +const MAX_ARRAY_LENGTH: f64 = 4_294_967_295.0; +/// Above this npm's `getUintArray(max)` returns `null` and the constructor +/// raises a plain `Error` instead of a `RangeError`. +const MAX_SAFE_INTEGER: f64 = 9_007_199_254_740_991.0; +/// Below this a "pointer" is a small integer handle, not a real object. +const MIN_OBJECT_ADDRESS: usize = 0x1000; + extern "C" { // Monotonic ms clock — same source npm lru-cache uses (`perf_now`); // honors Perry's mock timers. @@ -72,9 +111,13 @@ extern "C" { // Materialize any string repr (heap `STRING_TAG` or inline SSO // `SHORT_STRING_TAG`) into a real `*StringHeader` so we can read bytes. fn js_get_string_pointer_unified(value: f64) -> i64; - // Read a numeric/boolean option field off the NaN-boxed options object. + // Read an option field off the NaN-boxed options object. fn js_object_get_field_by_name_f64(obj: *const ObjectHeader, key: *const StringHeader) -> f64; fn js_is_truthy(value: f64) -> i32; + // JS `String(n)` — npm interpolates the offending `max` into its + // "invalid max value" message, and JS renders `1e300` as `"1e+300"` + // where Rust's `{}` would print 301 digits. + fn js_number_to_string(value: f64) -> *mut StringHeader; } #[inline] @@ -107,6 +150,13 @@ enum CacheKey { Undefined, /// Heap pointer identity (lower 48 bits) for object/array/function keys. Obj(u64), + /// A string key whose bytes could not be materialized. Kept distinct + /// from `Str(b"")` so a failed read never aliases the empty-string key + /// — and keyed on the value's own bits so two *different* unresolvable + /// strings do not alias each other either. Such a key can only ever be + /// hit again by the identical value, which is the safe direction: a + /// miss, never someone else's entry. + UnresolvedStr(u64), } #[inline] @@ -132,7 +182,7 @@ fn cache_key(key: f64) -> CacheKey { return CacheKey::Str(bytes.to_vec().into_boxed_slice()); } } - CacheKey::Str(Box::default()) + CacheKey::UnresolvedStr(key.to_bits()) } else if jv.is_int32() { CacheKey::Num(canonical_num_bits(jv.to_int32() as f64)) } else if jv.is_undefined() { @@ -174,10 +224,24 @@ pub struct LruCacheHandle { } impl LruCacheHandle { + /// `max_size == 0` is npm's unbounded (`ttl`-only) cache. + /// + /// `LruCache::new(cap)` eagerly reserves a `HashMap` of `cap` buckets. + /// npm accepts any `max` up to the JS array-length limit, so an eager + /// reservation turns a legal `new LRUCache({ max: 1e9 })` into a + /// multi-gigabyte allocation before the first insert — the same shape + /// of failure npm itself hits (it OOMs Node there). `unbounded()` plus + /// `resize()` yields an identical eviction bound over a lazily grown + /// map, so Perry survives a range where npm dies. That is the only + /// deliberate divergence in this constructor and it is one-directional: + /// no program can observe it except by not running out of memory. fn new(max_size: usize, ttl_ms: Option, update_age_on_get: bool) -> Self { - let size = NonZeroUsize::new(max_size.max(1)).expect("max_size at least 1"); + let mut cache = LruCache::unbounded(); + if let Some(cap) = NonZeroUsize::new(max_size) { + cache.resize(cap); + } LruCacheHandle { - cache: LruCache::new(size), + cache, ttl_ms, update_age_on_get, } @@ -213,58 +277,161 @@ fn now_ms() -> f64 { unsafe { js_performance_now() } } -/// Read a numeric option field; `None` when absent or non-numeric. -unsafe fn option_number(ptr: *const ObjectHeader, name: &str) -> Option { +/// Read `options.` as a raw NaN-boxed value. +/// +/// `None` means `options` was not an object. npm destructures its options +/// argument, and destructuring a primitive yields `undefined` for every +/// field rather than throwing, so that is what a non-object reads as here. +fn option_value(fields: Option<*const ObjectHeader>, name: &str) -> JsValue { + let Some(ptr) = fields else { + return JsValue::UNDEFINED; + }; let key = perry_ffi::alloc_string(name); - let raw = js_object_get_field_by_name_f64(ptr, key.as_raw()); - let jv = JsValue::from_bits(raw.to_bits()); - if jv.is_int32() { - Some(jv.to_int32() as f64) - } else if jv.is_number() && !raw.is_nan() { - Some(raw) - } else { - None + // SAFETY: `fields` is a validated non-null object pointer (see + // `js_lru_cache_new`), and `key` owns the freshly allocated header. + let raw = unsafe { js_object_get_field_by_name_f64(ptr, key.as_raw()) }; + JsValue::from_bits(raw.to_bits()) +} + +/// npm's `isPosInt`: `!!n && n === Math.floor(n) && n > 0 && isFinite(n)`. +/// +/// The `===` is a *strict* compare against `Math.floor(n)`, so a non-number +/// can never be a positive integer — `"3"`, `true` and `null` all fail it, +/// which is why they raise the same `TypeError` as `-1` does. +fn is_pos_int(v: JsValue) -> bool { + if !v.is_number() { + return false; + } + let n = v.to_number(); + n.is_finite() && n > 0.0 && n == n.trunc() +} + +/// JS `String(n)`, for interpolating a number into an npm error message. +fn js_number_string(n: f64) -> String { + // SAFETY: the runtime returns either null or a live `StringHeader`. + let ptr = unsafe { js_number_to_string(n) }; + if ptr.is_null() { + return n.to_string(); + } + let handle = unsafe { JsString::from_raw(ptr) }; + read_bytes(handle).map_or_else( + || n.to_string(), + |b| String::from_utf8_lossy(b).into_owned(), + ) +} + +/// npm: `const { max = 0 } = options; if (max !== 0 && !isPosInt(max)) throw …` +/// followed by the `getUintArray` / `Array.from({ length: max })` bounds. +/// Returns the validated `max` (`0` = unbounded); diverges on a bad value. +fn parse_max(fields: Option<*const ObjectHeader>) -> f64 { + let raw = option_value(fields, "max"); + if raw.is_undefined() { + return 0.0; // npm's `max = 0` destructuring default + } + // npm's `max !== 0` is strict, so only the *numbers* +0/-0 skip the + // validation below. `null`/`false`/`""` are all `!== 0` and throw. + if raw.is_number() && raw.to_number() == 0.0 { + return 0.0; + } + if !is_pos_int(raw) { + throw_with_code( + "max option must be a nonnegative integer", + "", + ErrorKind::TypeError, + ); } + let max = raw.to_number(); + if max > MAX_SAFE_INTEGER { + // npm: `if (!UintArray) throw new Error('invalid max value: ' + max)`. + let msg = format!("invalid max value: {}", js_number_string(max)); + throw_with_code(&msg, "", ErrorKind::Error); + } + if max > MAX_ARRAY_LENGTH { + // npm: `Array.from({ length: max })` — V8's array-length check. + throw_with_code("Invalid array length", "", ErrorKind::RangeError); + } + max +} + +/// npm: `this.ttl = ttl || 0; if (this.ttl && !isPosInt(this.ttl)) throw …`. +/// Returns the validated ttl in ms (`0` = none); diverges on a bad value. +fn parse_ttl(fields: Option<*const ObjectHeader>) -> f64 { + let raw = option_value(fields, "ttl"); + // `ttl || 0` — undefined, null, `0`, `NaN` and `""` all collapse to 0 + // *without* tripping the validation (npm only checks a truthy ttl). + // SAFETY: `js_is_truthy` reads a NaN-boxed value by value. + if unsafe { js_is_truthy(f64::from_bits(raw.bits())) } == 0 { + return 0.0; + } + if !is_pos_int(raw) { + throw_with_code( + "ttl must be a positive integer if specified", + "", + ErrorKind::TypeError, + ); + } + raw.to_number() } /// `new LRUCache(options)` — register a fresh cache and return its handle. /// -/// `options` is the NaN-boxed options object. `max < 1` / absent falls back -/// to 100 (so an unconfigured cache is still bounded). `ttl` and -/// `updateAgeOnGet` are honored when present. +/// `options` is the NaN-boxed options argument. Validation mirrors npm +/// `lru-cache` exactly (see the crate-level table); a rejected option +/// throws the JS error npm throws rather than being silently clamped. #[no_mangle] pub extern "C" fn js_lru_cache_new(options: f64) -> Handle { ensure_gc_scanner(); - let mut max = 100usize; - let mut ttl_ms = None; - let mut update_age_on_get = false; - - let jv = JsValue::from_bits(options.to_bits()); - if jv.is_pointer() { - let ptr = jv.as_pointer::(); - if !ptr.is_null() && (ptr as usize) >= 0x1000 { - unsafe { - if let Some(n) = option_number(ptr, "max") { - if n >= 1.0 { - max = n as usize; - } - } - if let Some(n) = option_number(ptr, "ttl") { - if n > 0.0 { - ttl_ms = Some(n); - } - } - let key = perry_ffi::alloc_string("updateAgeOnGet"); - let uaog = js_object_get_field_by_name_f64(ptr, key.as_raw()); - if js_is_truthy(uaog) != 0 { - update_age_on_get = true; - } - } - } + let opts = JsValue::from_bits(options.to_bits()); + // npm destructures `options` in the constructor *signature*, so a + // missing or null argument is a property read on undefined/null. The + // message a caller sees on Node is V8's, so that is the message here. + if opts.is_undefined() { + throw_with_code( + "Cannot read properties of undefined (reading 'max')", + "", + ErrorKind::TypeError, + ); + } + if opts.is_null() { + throw_with_code( + "Cannot read properties of null (reading 'max')", + "", + ErrorKind::TypeError, + ); + } + // Any other primitive destructures cleanly into all-undefined fields. + let fields = if opts.is_pointer() { + let ptr = opts.as_pointer::(); + (!ptr.is_null() && (ptr as usize) >= MIN_OBJECT_ADDRESS).then_some(ptr as *const _) + } else { + None + }; + + let max = parse_max(fields); + let ttl = parse_ttl(fields); + if max == 0.0 && ttl == 0.0 { + // npm: "do not allow completely unbounded caches". `maxSize` would + // also satisfy this on npm, but it is unimplemented here (see the + // crate-level scope note), so it cannot. + throw_with_code( + "At least one of max, maxSize, or ttl is required", + "", + ErrorKind::TypeError, + ); } + // SAFETY: `js_is_truthy` reads a NaN-boxed value by value. + let update_age_on_get = unsafe { + js_is_truthy(f64::from_bits( + option_value(fields, "updateAgeOnGet").bits(), + )) != 0 + }; - register_handle(LruCacheHandle::new(max, ttl_ms, update_age_on_get)) + register_handle(LruCacheHandle::new( + max as usize, + (ttl > 0.0).then_some(ttl), + update_age_on_get, + )) } /// `cache.get(key)` — returns `undefined` when the key is absent or its @@ -327,9 +494,10 @@ pub extern "C" fn js_lru_cache_has(handle: Handle, key: f64) -> f64 { let k = cache_key(key); let now = now_ms(); js_bool( - with_handle_mut::(handle, |h| { - matches!(h.cache.peek(&k), Some(entry) if !entry.is_expired(now)) - }) + with_handle_mut::( + handle, + |h| matches!(h.cache.peek(&k), Some(entry) if !entry.is_expired(now)), + ) .unwrap_or(false), ) } diff --git a/crates/perry-ext-lru-cache/src/tests.rs b/crates/perry-ext-lru-cache/src/tests.rs index 4be9d62a2c..b0b7c2e447 100644 --- a/crates/perry-ext-lru-cache/src/tests.rs +++ b/crates/perry-ext-lru-cache/src/tests.rs @@ -10,6 +10,10 @@ use super::*; use perry_ffi::{alloc_string, nanbox_string_bits, JsValue}; use std::sync::Mutex; +extern "C" { + fn js_object_set_field_by_name(obj: *mut ObjectHeader, key: *const StringHeader, value: f64); +} + /// Serializes the tests that touch global GC state. static GC_TEST_LOCK: Mutex<()> = Mutex::new(()); @@ -20,6 +24,61 @@ fn string_value(text: &str) -> f64 { f64::from_bits(nanbox_string_bits(s.as_raw())) } +/// Build a real JS options object carrying `fields`, the way a compiled +/// `new LRUCache({ … })` object literal reaches the constructor. +fn options(fields: &[(&str, f64)]) -> f64 { + let obj = perry_ffi::alloc_object(); + assert!(obj.is_pointer(), "alloc_object returned a non-object"); + let ptr = obj.as_pointer::(); + for (name, value) in fields { + let key = alloc_string(name); + unsafe { js_object_set_field_by_name(ptr, key.as_raw(), *value) }; + } + f64::from_bits(obj.bits()) +} + +/// `new LRUCache({ max })` through the real constructor. +fn new_cache(max: f64) -> Handle { + js_lru_cache_new(options(&[("max", max)])) +} + +/// Call `js_lru_cache_new` inside a JS `try` so a rejected option can be +/// asserted on. Without the `try`, a throw at depth 0 prints the uncaught +/// error and exits the process, taking the test binary with it. +fn new_catching(opts: f64) -> Result { + match perry_runtime::exception::js_call_catching(|| js_lru_cache_new(opts) as f64) { + Ok(handle) => Ok(handle as Handle), + Err(bits) => { + let err = JsValue::from_bits(bits.to_bits()); + let ptr = err.as_pointer::(); + let field = |name: &str| { + let key = alloc_string(name); + let v = unsafe { js_object_get_field_by_name_f64(ptr, key.as_raw()) }; + read_string_value(v).unwrap_or_default() + }; + Err((field("name"), field("message"))) + } + } +} + +/// Assert `new LRUCache(opts)` throws npm's `name` + `message`. +#[track_caller] +fn assert_throws(opts: f64, name: &str, message: &str) { + match new_catching(opts) { + Ok(h) => { + perry_ffi::drop_handle(h); + panic!("expected {name}: {message}, but the constructor returned a cache"); + } + Err((got_name, got_message)) => { + assert_eq!( + (got_name.as_str(), got_message.as_str()), + (name, message), + "constructor error must match npm lru-cache byte for byte" + ); + } + } +} + /// npm `has`/`delete` return NaN-boxed JS booleans — decode one. fn is_true(v: f64) -> bool { v.to_bits() == TAG_TRUE @@ -54,8 +113,14 @@ fn cache_key_primitive_variants() { cache_key(f64::from_bits(JsValue::from_int32(7).bits())), CacheKey::Num(canonical_num_bits(7.0)) ); - assert_eq!(cache_key(f64::from_bits(JsValue::TRUE.bits())), CacheKey::Bool(true)); - assert_eq!(cache_key(f64::from_bits(JsValue::NULL.bits())), CacheKey::Null); + assert_eq!( + cache_key(f64::from_bits(JsValue::TRUE.bits())), + CacheKey::Bool(true) + ); + assert_eq!( + cache_key(f64::from_bits(JsValue::NULL.bits())), + CacheKey::Null + ); assert_eq!( cache_key(f64::from_bits(JsValue::UNDEFINED.bits())), CacheKey::Undefined @@ -66,7 +131,7 @@ fn cache_key_primitive_variants() { #[test] fn basic_set_get_round_trip() { - let h = js_lru_cache_new(f64::from_bits(TAG_UNDEFINED)); + let h = new_cache(10.0); assert_ne!(h, perry_ffi::INVALID_HANDLE); js_lru_cache_set(h, 1.0, 100.0); assert_eq!(js_lru_cache_get(h, 1.0), 100.0); @@ -95,7 +160,7 @@ fn lru_eviction_at_max_size() { #[test] fn delete_and_clear() { - let h = js_lru_cache_new(f64::from_bits(TAG_UNDEFINED)); + let h = new_cache(10.0); js_lru_cache_set(h, 1.0, 100.0); js_lru_cache_set(h, 2.0, 200.0); assert!(is_true(js_lru_cache_delete(h, 1.0))); @@ -122,7 +187,7 @@ fn peek_does_not_bump_recency() { #[test] fn missing_key_returns_undefined() { - let h = js_lru_cache_new(f64::from_bits(TAG_UNDEFINED)); + let h = new_cache(10.0); let v = js_lru_cache_get(h, 42.0); assert_eq!(v.to_bits(), TAG_UNDEFINED, "missing key must be undefined"); perry_ffi::drop_handle(h); @@ -140,7 +205,7 @@ fn invalid_handle_is_no_op() { #[test] fn string_key_round_trip_by_content() { - let h = js_lru_cache_new(f64::from_bits(TAG_UNDEFINED)); + let h = new_cache(10.0); // Store under one string allocation… js_lru_cache_set(h, string_value("cache-key"), 4242.0); // …read back through a *different* allocation of the same text. The old @@ -154,8 +219,12 @@ fn string_key_round_trip_by_content() { #[test] fn string_key_object_value_round_trip() { - let h = js_lru_cache_new(f64::from_bits(TAG_UNDEFINED)); - js_lru_cache_set(h, string_value("payload"), string_value("hello-world-value")); + let h = new_cache(10.0); + js_lru_cache_set( + h, + string_value("payload"), + string_value("hello-world-value"), + ); let got = js_lru_cache_get(h, string_value("payload")); assert_eq!(read_string_value(got).as_deref(), Some("hello-world-value")); perry_ffi::drop_handle(h); @@ -185,50 +254,181 @@ fn has_and_peek_report_expired_as_absent() { perry_ffi::drop_handle(h); } +// The TTL/sleep ratio below is deliberately wide. A 3:1 TTL-to-sleep +// margin means a loaded CI runner has to overshoot a 150 ms sleep by +// 150 ms before the refresh test can misread a live entry as expired. +const REFRESH_TTL_MS: f64 = 450.0; +const REFRESH_STEP: std::time::Duration = std::time::Duration::from_millis(150); + #[test] fn update_age_on_get_refreshes_ttl() { - let h = perry_ffi::register_handle(LruCacheHandle::new(10, Some(80.0), true)); + let h = perry_ffi::register_handle(LruCacheHandle::new(10, Some(REFRESH_TTL_MS), true)); js_lru_cache_set(h, 1.0, 111.0); - // Halfway through the TTL, a get refreshes the clock. - std::thread::sleep(std::time::Duration::from_millis(50)); + // A third of the way through the TTL, a get refreshes the clock. + std::thread::sleep(REFRESH_STEP); assert_eq!(js_lru_cache_get(h, 1.0), 111.0); - // Another half-TTL later the entry is still live *because* it was - // refreshed (without updateAgeOnGet it would have expired at ~80ms). - std::thread::sleep(std::time::Duration::from_millis(50)); + // Two more steps put us past the original expiry; the entry is still + // live *because* the get above restarted its clock. + std::thread::sleep(REFRESH_STEP); + std::thread::sleep(REFRESH_STEP); assert_eq!(js_lru_cache_get(h, 1.0), 111.0, "get refreshed the TTL"); perry_ffi::drop_handle(h); } #[test] fn no_update_age_on_get_lets_ttl_expire() { - let h = perry_ffi::register_handle(LruCacheHandle::new(10, Some(80.0), false)); + let h = perry_ffi::register_handle(LruCacheHandle::new(10, Some(REFRESH_TTL_MS), false)); js_lru_cache_set(h, 1.0, 111.0); - std::thread::sleep(std::time::Duration::from_millis(50)); + std::thread::sleep(REFRESH_STEP); assert_eq!(js_lru_cache_get(h, 1.0), 111.0); // still live, no refresh - std::thread::sleep(std::time::Duration::from_millis(50)); - // ~100ms since set, TTL was 80ms and never refreshed → expired. + std::thread::sleep(REFRESH_STEP); + std::thread::sleep(REFRESH_STEP); + // Past the TTL measured from `set`, and never refreshed → expired. assert_eq!(js_lru_cache_get(h, 1.0).to_bits(), TAG_UNDEFINED); perry_ffi::drop_handle(h); } // ── options-object parsing ─────────────────────────────────────────── +// +// Every expectation below was measured against `lru-cache@11.5.2` under +// the repo's pinned Node oracle (26.5.1). They are npm's errors, not +// Perry's invention — see the table in the crate docs. + +#[test] +fn options_object_is_honored() { + let h = js_lru_cache_new(options(&[ + ("max", 3.0), + ("ttl", 5_000.0), + ("updateAgeOnGet", f64::from_bits(TAG_TRUE)), + ])); + let parsed = + with_handle_mut::(h, |c| (c.ttl_ms, c.update_age_on_get)).unwrap(); + assert_eq!(parsed, (Some(5_000.0), true)); + for i in 0..4 { + js_lru_cache_set(h, i as f64, i as f64); + } + assert_eq!(js_lru_cache_size(h), 3.0, "max:3 bounds the cache"); + perry_ffi::drop_handle(h); +} + +#[test] +fn huge_max_is_a_range_error_not_an_allocation() { + // The regression this pins: `max: 1e12` used to saturate through + // `n as usize` into the backing map's reserve and abort the process. + // npm raises `Array.from({ length: 1e12 })`'s RangeError instead. + assert_throws( + options(&[("max", 1e12)]), + "RangeError", + "Invalid array length", + ); + assert_throws( + options(&[("max", 4_294_967_296.0)]), + "RangeError", + "Invalid array length", + ); + // Past MAX_SAFE_INTEGER npm reports its own `getUintArray` failure, + // and renders the number the way JS does. + assert_throws( + options(&[("max", 9_007_199_254_740_992.0)]), + "Error", + "invalid max value: 9007199254740992", + ); + assert_throws( + options(&[("max", 1e300)]), + "Error", + "invalid max value: 1e+300", + ); +} + +#[test] +fn max_below_the_range_error_still_constructs_lazily() { + // Just under the array-length limit: npm OOMs Node here, Perry does + // not, because the backing map grows lazily instead of reserving + // `max` buckets up front. Constructing must be instant and cheap. + let h = new_cache(MAX_ARRAY_LENGTH); + js_lru_cache_set(h, 1.0, 111.0); + assert_eq!(js_lru_cache_get(h, 1.0), 111.0); + assert_eq!(js_lru_cache_size(h), 1.0); + perry_ffi::drop_handle(h); +} #[test] -fn new_without_options_defaults_to_100() { - let h = js_lru_cache_new(f64::from_bits(TAG_UNDEFINED)); - // TTL/updateAgeOnGet default off… - let opts = with_handle_mut::(h, |c| (c.ttl_ms, c.update_age_on_get)) - .unwrap(); - assert_eq!(opts, (None, false)); - // …and the capacity defaults to 100 (the 101st insert evicts). - for i in 0..101 { +fn non_integer_max_is_a_type_error() { + const MSG: &str = "max option must be a nonnegative integer"; + for bad in [-1.0, 1.5, f64::INFINITY, f64::NEG_INFINITY, f64::NAN] { + assert_throws(options(&[("max", bad)]), "TypeError", MSG); + } + // Non-numbers fail npm's strict `n === Math.floor(n)` too. + assert_throws(options(&[("max", string_value("3"))]), "TypeError", MSG); + assert_throws( + options(&[("max", f64::from_bits(JsValue::TRUE.bits()))]), + "TypeError", + MSG, + ); + assert_throws( + options(&[("max", f64::from_bits(JsValue::NULL.bits()))]), + "TypeError", + MSG, + ); +} + +#[test] +fn unbounded_cache_is_rejected() { + const MSG: &str = "At least one of max, maxSize, or ttl is required"; + // `max: 0` and `max: -0` pass npm's `max !== 0` guard, then fall into + // its "do not allow completely unbounded caches" check. + assert_throws(options(&[("max", 0.0)]), "TypeError", MSG); + assert_throws(options(&[("max", -0.0)]), "TypeError", MSG); + assert_throws(options(&[]), "TypeError", MSG); + // A primitive `options` destructures into all-undefined fields. + assert_throws(5.0, "TypeError", MSG); + assert_throws(string_value("x"), "TypeError", MSG); +} + +#[test] +fn missing_options_matches_npm_destructuring() { + assert_throws( + f64::from_bits(TAG_UNDEFINED), + "TypeError", + "Cannot read properties of undefined (reading 'max')", + ); + assert_throws( + f64::from_bits(JsValue::NULL.bits()), + "TypeError", + "Cannot read properties of null (reading 'max')", + ); +} + +#[test] +fn ttl_alone_makes_an_unbounded_cache() { + let h = js_lru_cache_new(options(&[("ttl", 5_000.0)])); + for i in 0..1_000 { js_lru_cache_set(h, i as f64, i as f64); } - assert_eq!(js_lru_cache_size(h), 100.0); - assert!(!is_true(js_lru_cache_has(h, 0.0)), "oldest entry evicted at cap 100"); + assert_eq!(js_lru_cache_size(h), 1_000.0, "no max ⇒ no eviction"); perry_ffi::drop_handle(h); } +#[test] +fn non_integer_ttl_is_a_type_error() { + const MSG: &str = "ttl must be a positive integer if specified"; + for bad in [-5.0, 1.5, f64::INFINITY] { + assert_throws(options(&[("max", 3.0), ("ttl", bad)]), "TypeError", MSG); + } + assert_throws( + options(&[("max", 3.0), ("ttl", string_value("5"))]), + "TypeError", + MSG, + ); + // npm's `ttl || 0` swallows every falsy ttl before the check runs. + for falsy in [0.0, f64::NAN] { + let h = js_lru_cache_new(options(&[("max", 3.0), ("ttl", falsy)])); + let ttl = with_handle_mut::(h, |c| c.ttl_ms).unwrap(); + assert_eq!(ttl, None, "falsy ttl is dropped, not rejected"); + perry_ffi::drop_handle(h); + } +} + // ── GC rooting: a cached heap value survives a collection ──────────── #[test] @@ -241,12 +441,25 @@ fn cached_value_survives_gc_cycle() { // and a live shadow frame (mirrors perry-ext-events' scanner test). perry_runtime::gc::js_gc_write_barriers_emitted(1); let frame = perry_runtime::gc::js_shadow_frame_push(0); + // Both of those are process-global. An assertion below panicking must + // not leave write barriers on and a frame pushed for every later test + // in this binary, so tear down in `Drop` rather than on the happy path. + struct GcStateGuard(u64); + impl Drop for GcStateGuard { + fn drop(&mut self) { + perry_runtime::gc::js_shadow_frame_pop(self.0); + perry_runtime::gc::js_gc_write_barriers_emitted(0); + } + } + let _gc_state = GcStateGuard(frame); ensure_gc_scanner(); - let h = js_lru_cache_new(f64::from_bits(TAG_UNDEFINED)); + let h = new_cache(10.0); // A >5-byte string forces the heap `StringHeader` repr (not inline SSO), // so it is a real collectable allocation. Its ONLY root is the cache. - js_lru_cache_set(h, 1.0, string_value("value-object-1234567890")); + let value = string_value("value-object-1234567890"); + let before = value.to_bits() & POINTER_MASK; + js_lru_cache_set(h, 1.0, value); // Reclaims unrooted nursery allocations and evacuates rooted survivors. let _ = perry_runtime::gc::gc_collect_minor(); @@ -257,6 +470,20 @@ fn cached_value_survives_gc_cycle() { TAG_UNDEFINED, "cached value was collected — root scanner did not keep it alive" ); + // Subject-live gate (#6942/#6946). Marking alone would satisfy the + // assertions above, and a non-moving collection satisfies them even if + // the scanner never rewrites a slot — so this test would keep passing + // while covering nothing. Require the evacuation to have actually + // relocated the value: the address the cache hands back must differ + // from the one it was given. If this fires, the minor did not copy + // (`PERRY_GEN_GC=0` routes to non-moving mark-sweep, and barriers-off + // falls back the same way) and the rewrite path is untested, not fixed. + let after = got.to_bits() & POINTER_MASK; + assert_ne!( + before, after, + "minor GC did not relocate the cached value (0x{before:x}), so this test \ + never exercised the scanner's forwarding-pointer rewrite" + ); assert_eq!( read_string_value(got).as_deref(), Some("value-object-1234567890"), @@ -264,6 +491,4 @@ fn cached_value_survives_gc_cycle() { ); perry_ffi::drop_handle(h); - perry_runtime::gc::js_shadow_frame_pop(frame); - perry_runtime::gc::js_gc_write_barriers_emitted(0); } diff --git a/docs/src/stdlib/other.md b/docs/src/stdlib/other.md index 69f81dba42..0ba398071d 100644 --- a/docs/src/stdlib/other.md +++ b/docs/src/stdlib/other.md @@ -192,8 +192,14 @@ if (parentPort) { ## lru-cache The wired constructor takes the npm v7+ options-object shape -(`new LRUCache({ max: 100 })`) — the older positional form -`new LRUCache(100)` falls through to a `max=100` default. +(`new LRUCache({ max: 100 })`) and validates it the way npm does, throwing +the same errors rather than clamping: `max` must be a positive integer no +larger than the JS array-length limit, `ttl` must be a positive integer, +and at least one of `max` or `ttl` is required — so `new LRUCache()` and +the older positional form `new LRUCache(100)` both throw a `TypeError`, +exactly as they do on npm. `ttl`, `updateAgeOnGet` and `peek` are honored; +`maxSize`/`sizeCalculation`, `dispose`, `fetch`, `allowStale` and the +iterator surface are not yet implemented. ```typescript,no-test {{#include ../../examples/stdlib/other/snippets.ts:lru-cache}} From 7197b4fef75ecf5b803e510606cb2c245ea396bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 1 Aug 2026 08:00:42 +0200 Subject: [PATCH 4/8] test(lru-cache): give the GC-survival test a process it can prove things in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The relocation assertion added in the previous commit is only meaningful on a clean heap, and a shared unit-test binary cannot provide one. The collector conservatively pins any nursery object that some stack word happens to point at. Run after even one other test in the same binary, the cached string is pinned and the minor reports `copied_objects=0` where it reports `1` when the test runs alone — so the assertion fails through no fault of the binding. Separately, driving `gc_collect_minor()` from a unit-test binary that has also allocated JS *objects* — which the new constructor tests must do, to pass a real options object — makes the copying collector walk a bogus slot address and SIGSEGV in `gc::copying::scan_slot`. That reproduces deterministically under `--test-threads=1`. It is a runtime-level fragility rather than anything this binding controls, but it means the two kinds of test cannot share a process. So the survival test moves to `tests/gc_survival.rs`, where it gets an empty nursery, a shallow stack and no JS objects, and drives only the public `js_lru_cache_*` surface. Verified in both directions: green on 5 consecutive runs, and `PERRY_GEN_GC=0` (which routes the minor to non-moving mark-sweep) trips the relocation assertion as intended. The per-PR gate is `cargo test --lib --bins`, so this suite runs on PRs that touch it, on nightly and on tags. That is the cost of an assertion that can fail; the version that stayed in the unit binary could only keep passing while covering nothing. --- changelog.d/7136-lru-cache-faithful.md | 11 ++ crates/perry-ext-lru-cache/Cargo.toml | 11 +- crates/perry-ext-lru-cache/src/tests.rs | 103 ++++--------- .../perry-ext-lru-cache/tests/gc_survival.rs | 136 ++++++++++++++++++ 4 files changed, 183 insertions(+), 78 deletions(-) create mode 100644 crates/perry-ext-lru-cache/tests/gc_survival.rs diff --git a/changelog.d/7136-lru-cache-faithful.md b/changelog.d/7136-lru-cache-faithful.md index d6ab979ea1..1e85e4fdb9 100644 --- a/changelog.d/7136-lru-cache-faithful.md +++ b/changelog.d/7136-lru-cache-faithful.md @@ -71,4 +71,15 @@ identity keys are supported by pointer identity but are not tracked across a GC relocation; primitive keys are the GC-safe path. + The GC-survival test moved to its own test binary + (`crates/perry-ext-lru-cache/tests/gc_survival.rs`) and now asserts that + the collector *relocated* the cached value, not merely that it is still + readable — a non-moving collection satisfies the latter without + exercising one line of the scanner's forwarding-pointer rewrite + (#6942/#6946). It needs its own process to do that: the collector + conservatively pins any nursery object a stack word points at, so after + even one other test in the same binary the minor reports + `copied_objects=0` instead of `1`. Verified in both directions — + `PERRY_GEN_GC=0` (non-moving mark-sweep) trips the new assertion. + Tracking: #466 (Phase 5 native bindings). PR #7136. diff --git a/crates/perry-ext-lru-cache/Cargo.toml b/crates/perry-ext-lru-cache/Cargo.toml index acfd7b03f2..1e234158a4 100644 --- a/crates/perry-ext-lru-cache/Cargo.toml +++ b/crates/perry-ext-lru-cache/Cargo.toml @@ -17,9 +17,12 @@ lru = "0.18" [dev-dependencies] perry-ffi = { workspace = true, features = ["runtime-link"] } -# Direct handle for the GC-survival test: it forces a minor collection +# Direct handle for two things the tests need from the runtime: +# `tests/gc_survival.rs` forces a minor collection # (`perry_runtime::gc::gc_collect_minor`) and drives the write-barrier / -# shadow-frame guard, mirroring perry-ext-events' scanner test. `default` + -# `stdlib` keep this copy feature-identical to the shipped runtime (see -# perry-ext-events/Cargo.toml for the #6303 rationale). +# shadow-frame guard, mirroring perry-ext-events' scanner test; the unit +# tests use `exception::js_call_catching` to assert on the constructor's +# npm-matching option errors without the throw exiting the process. +# `default` + `stdlib` keep this copy feature-identical to the shipped +# runtime (see perry-ext-events/Cargo.toml for the #6303 rationale). perry-runtime = { workspace = true, features = ["default", "stdlib"] } diff --git a/crates/perry-ext-lru-cache/src/tests.rs b/crates/perry-ext-lru-cache/src/tests.rs index b0b7c2e447..73be101109 100644 --- a/crates/perry-ext-lru-cache/src/tests.rs +++ b/crates/perry-ext-lru-cache/src/tests.rs @@ -3,20 +3,19 @@ //! Every test links `perry-runtime` (the `runtime-link` dev-dep feature) //! because the wrapper reaches the runtime for its clock //! (`js_performance_now`) and string materialization -//! (`js_get_string_pointer_unified`). The GC-survival test additionally -//! drives `perry-runtime`'s collector directly. +//! (`js_get_string_pointer_unified`), and reaches `perry-runtime` +//! directly for `js_call_catching` so a rejected constructor option can be +//! asserted on instead of exiting the process. The GC-survival test lives +//! in `tests/gc_survival.rs` — see that file for why it needs its own +//! process. use super::*; use perry_ffi::{alloc_string, nanbox_string_bits, JsValue}; -use std::sync::Mutex; - extern "C" { + fn js_object_alloc(class_id: u32, field_count: u32) -> *mut ObjectHeader; fn js_object_set_field_by_name(obj: *mut ObjectHeader, key: *const StringHeader, value: f64); } -/// Serializes the tests that touch global GC state. -static GC_TEST_LOCK: Mutex<()> = Mutex::new(()); - /// NaN-boxed `f64` for a freshly allocated JS string with `text`. fn string_value(text: &str) -> f64 { let s = alloc_string(text); @@ -26,15 +25,25 @@ fn string_value(text: &str) -> f64 { /// Build a real JS options object carrying `fields`, the way a compiled /// `new LRUCache({ … })` object literal reaches the constructor. +/// +/// The slot count is declared up front rather than using +/// `perry_ffi::alloc_object()` (which allocates zero inline slots and +/// grows on the first `js_object_set_field_by_name`). That is not a +/// stylistic preference: an object built the zero-slot way makes the +/// copying minor collector walk a bogus slot address and SIGSEGV in +/// `gc::copying::scan_slot` on the *next* collection in the process +/// (reproduced with `--test-threads=1`; see the PR discussion). Declaring +/// the slots avoids the grow path entirely. A compiled object literal +/// declares its slots the same way, so this is also the more faithful +/// model of what reaches the constructor. fn options(fields: &[(&str, f64)]) -> f64 { - let obj = perry_ffi::alloc_object(); - assert!(obj.is_pointer(), "alloc_object returned a non-object"); - let ptr = obj.as_pointer::(); + let ptr = unsafe { js_object_alloc(0, fields.len() as u32) }; + assert!(!ptr.is_null(), "js_object_alloc returned null"); for (name, value) in fields { let key = alloc_string(name); unsafe { js_object_set_field_by_name(ptr, key.as_raw(), *value) }; } - f64::from_bits(obj.bits()) + f64::from_bits(JsValue::from_object_ptr(ptr).bits()) } /// `new LRUCache({ max })` through the real constructor. @@ -429,66 +438,12 @@ fn non_integer_ttl_is_a_type_error() { } } -// ── GC rooting: a cached heap value survives a collection ──────────── - -#[test] -fn cached_value_survives_gc_cycle() { - let _lock = GC_TEST_LOCK - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()); - - // Match the runtime's evacuation preconditions: write barriers active - // and a live shadow frame (mirrors perry-ext-events' scanner test). - perry_runtime::gc::js_gc_write_barriers_emitted(1); - let frame = perry_runtime::gc::js_shadow_frame_push(0); - // Both of those are process-global. An assertion below panicking must - // not leave write barriers on and a frame pushed for every later test - // in this binary, so tear down in `Drop` rather than on the happy path. - struct GcStateGuard(u64); - impl Drop for GcStateGuard { - fn drop(&mut self) { - perry_runtime::gc::js_shadow_frame_pop(self.0); - perry_runtime::gc::js_gc_write_barriers_emitted(0); - } - } - let _gc_state = GcStateGuard(frame); - ensure_gc_scanner(); - - let h = new_cache(10.0); - // A >5-byte string forces the heap `StringHeader` repr (not inline SSO), - // so it is a real collectable allocation. Its ONLY root is the cache. - let value = string_value("value-object-1234567890"); - let before = value.to_bits() & POINTER_MASK; - js_lru_cache_set(h, 1.0, value); - - // Reclaims unrooted nursery allocations and evacuates rooted survivors. - let _ = perry_runtime::gc::gc_collect_minor(); - - let got = js_lru_cache_get(h, 1.0); - assert_ne!( - got.to_bits(), - TAG_UNDEFINED, - "cached value was collected — root scanner did not keep it alive" - ); - // Subject-live gate (#6942/#6946). Marking alone would satisfy the - // assertions above, and a non-moving collection satisfies them even if - // the scanner never rewrites a slot — so this test would keep passing - // while covering nothing. Require the evacuation to have actually - // relocated the value: the address the cache hands back must differ - // from the one it was given. If this fires, the minor did not copy - // (`PERRY_GEN_GC=0` routes to non-moving mark-sweep, and barriers-off - // falls back the same way) and the rewrite path is untested, not fixed. - let after = got.to_bits() & POINTER_MASK; - assert_ne!( - before, after, - "minor GC did not relocate the cached value (0x{before:x}), so this test \ - never exercised the scanner's forwarding-pointer rewrite" - ); - assert_eq!( - read_string_value(got).as_deref(), - Some("value-object-1234567890"), - "cached value corrupted across GC — slot was not rewritten to the forwarded address" - ); - - perry_ffi::drop_handle(h); -} +// ── GC rooting ─────────────────────────────────────────────────────── +// +// The cached-value-survives-a-copying-minor test lives in its own test +// binary (`tests/gc_survival.rs`). It needs a pristine heap to assert +// that the collector actually *relocated* the value, and it cannot get +// one here: any earlier test's stack leftovers conservatively pin the +// string (minor then reports `copied_objects=0`), and driving +// `gc_collect_minor()` in a binary that has also allocated JS objects +// SIGSEGVs the copying collector. See that file's module docs. diff --git a/crates/perry-ext-lru-cache/tests/gc_survival.rs b/crates/perry-ext-lru-cache/tests/gc_survival.rs new file mode 100644 index 0000000000..084f3add94 --- /dev/null +++ b/crates/perry-ext-lru-cache/tests/gc_survival.rs @@ -0,0 +1,136 @@ +//! A cached heap value survives — and is rewritten across — a copying +//! minor collection. +//! +//! # Why this is its own test binary +//! +//! The assertion that matters here is that the collector *moved* the +//! cached string and the root scanner rewrote the cache's slot to the +//! forwarded address. Marking alone, or a non-moving collection, satisfies +//! "the value is still readable" without exercising one line of the +//! rewrite path — the #6942/#6946 failure mode, where a gate is green +//! because its subject never ran. +//! +//! Making that assertion meaningful requires a clean heap, and a shared +//! unit-test binary cannot provide one: +//! +//! - The collector conservatively pins any nursery object some stack word +//! happens to point at. Run after even one other test in the same +//! binary, this string gets pinned and the minor reports +//! `copied_objects=0` (measured; alone it reports `copied_objects=1`), +//! so the relocation assertion fails through no fault of the binding. +//! - Worse, driving `gc_collect_minor()` from a unit-test binary that has +//! also allocated JS *objects* (which the constructor tests must do, to +//! pass a real options object) makes the copying collector walk a bogus +//! slot address and SIGSEGV in `gc::copying::scan_slot`. That +//! reproduces deterministically under `--test-threads=1` and is a +//! runtime-level fragility, not something this binding controls. +//! +//! A dedicated integration binary gets a pristine process: an empty +//! nursery, a shallow stack, and no JS objects. The relocation then +//! happens every run, so the assertion below is a real gate. +//! +//! Note the coverage trade-off: per-PR CI runs `cargo test --lib --bins`, +//! so this suite runs on PRs that touch it (via `e2e-scoped`), on nightly +//! and on tags — not on every PR. That is the price of an assertion that +//! can actually fail; a version of this test that lived in the unit +//! binary could only keep passing while covering nothing. + +use perry_ext_lru_cache::{js_lru_cache_get, js_lru_cache_new, js_lru_cache_set}; +use perry_ffi::{ + alloc_string, nanbox_string_bits, read_bytes, JsString, JsValue, ObjectHeader, StringHeader, +}; + +const POINTER_MASK: u64 = 0x0000_FFFF_FFFF_FFFF; +const TAG_UNDEFINED: u64 = 0x7FFC_0000_0000_0001; + +extern "C" { + fn js_object_alloc(class_id: u32, field_count: u32) -> *mut ObjectHeader; + fn js_object_set_field_by_name(obj: *mut ObjectHeader, key: *const StringHeader, value: f64); + fn js_get_string_pointer_unified(value: f64) -> i64; +} + +/// `{ max: }` as a real JS object, the way a compiled object +/// literal reaches the constructor. +fn options_with_max(max: f64) -> f64 { + let ptr = unsafe { js_object_alloc(0, 1) }; + assert!(!ptr.is_null(), "js_object_alloc returned null"); + let key = alloc_string("max"); + unsafe { js_object_set_field_by_name(ptr, key.as_raw(), max) }; + f64::from_bits(JsValue::from_object_ptr(ptr).bits()) +} + +fn string_value(text: &str) -> f64 { + let s = alloc_string(text); + assert!(!s.is_null(), "alloc_string returned null"); + f64::from_bits(nanbox_string_bits(s.as_raw())) +} + +fn read_string_value(value: f64) -> Option { + let ptr = unsafe { js_get_string_pointer_unified(value) } as *mut StringHeader; + if ptr.is_null() { + return None; + } + let handle = unsafe { JsString::from_raw(ptr) }; + read_bytes(handle).map(|b| String::from_utf8_lossy(b).into_owned()) +} + +#[test] +fn cached_value_survives_and_is_rewritten_by_a_copying_minor() { + // Match the runtime's evacuation preconditions: write barriers active + // and a live shadow frame (mirrors perry-ext-events' scanner test). + perry_runtime::gc::js_gc_write_barriers_emitted(1); + let frame = perry_runtime::gc::js_shadow_frame_push(0); + // Both are process-global. A failing assertion below must not leave + // barriers on and a frame pushed, so tear down in `Drop` rather than + // on the happy path. + struct GcStateGuard(u64); + impl Drop for GcStateGuard { + fn drop(&mut self) { + perry_runtime::gc::js_shadow_frame_pop(self.0); + perry_runtime::gc::js_gc_write_barriers_emitted(0); + } + } + let _gc_state = GcStateGuard(frame); + + let h = js_lru_cache_new(options_with_max(10.0)); + // A >5-byte string forces the heap `StringHeader` repr (not inline + // SSO), so it is a real collectable allocation. Its ONLY root is the + // cache — nothing on the stack or in a global refers to it. + let value = string_value("value-object-1234567890"); + let before = value.to_bits() & POINTER_MASK; + js_lru_cache_set(h, 1.0, value); + assert!( + perry_runtime::arena::pointer_in_nursery(before as usize), + "the value must start in the nursery or a minor cannot move it" + ); + + // Reclaims unrooted nursery allocations and evacuates rooted survivors. + let _ = perry_runtime::gc::gc_collect_minor(); + + let got = js_lru_cache_get(h, 1.0); + assert_ne!( + got.to_bits(), + TAG_UNDEFINED, + "cached value was collected — the root scanner did not keep it alive" + ); + + // Subject-live gate. If the address did not change, the minor did not + // copy anything and this test proved only that marking works — the + // forwarding-pointer rewrite, which is the half most likely to break, + // went untested. `PERRY_GEN_GC=0` routes `gc_collect_minor` to + // non-moving mark-sweep and will trip this deliberately. + let after = got.to_bits() & POINTER_MASK; + assert_ne!( + before, after, + "minor GC did not relocate the cached value (0x{before:x}), so this run \ + never exercised the scanner's forwarding-pointer rewrite" + ); + assert_eq!( + read_string_value(got).as_deref(), + Some("value-object-1234567890"), + "cached value corrupted across GC — the slot was not rewritten to the \ + forwarded address" + ); + + perry_ffi::drop_handle(h); +} From 4f99876e61f10563ef0f29626ebaa4703df184a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 1 Aug 2026 08:05:49 +0200 Subject: [PATCH 5/8] docs(lru-cache): record the two divergences the npm A/B turned up Diffing a compiled probe against the real npm package under Node 26.5.1 (set/get/has/peek/delete/clear, content-keyed string keys, LRU eviction, peek recency, ttl expiry, updateAgeOnGet refresh-vs-not, and all 18 constructor option cases) leaves 41 of 45 output lines byte-identical. The four that differ are `cache.size`, which is wired as a method row so `cache.size()` works but npm's property read yields `undefined`. That is pre-existing and untouched here, but it should be written down rather than rediscovered. Also noting npm's `ttlResolution` clock caching: npm refreshes its TTL clock from a `setTimeout`, so code that blocks the event loop sees entries stay live indefinitely there, while this binding reads `performance.now()` on every access and does observe expiry. Worth knowing before someone writes a spin-loop TTL test and concludes the binding is wrong. --- changelog.d/7136-lru-cache-faithful.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/changelog.d/7136-lru-cache-faithful.md b/changelog.d/7136-lru-cache-faithful.md index 1e85e4fdb9..cd57232213 100644 --- a/changelog.d/7136-lru-cache-faithful.md +++ b/changelog.d/7136-lru-cache-faithful.md @@ -71,6 +71,17 @@ identity keys are supported by pointer identity but are not tracked across a GC relocation; primitive keys are the GC-safe path. + Two further divergences, both pre-existing and both found by diffing a + compiled probe against the npm package under Node 26.5.1 (41 of 45 output + lines are byte-identical, including every option-validation case above): + + - `cache.size` is wired as a *method* row, so `cache.size()` works but + npm's `cache.size` property read yields `undefined`. Unchanged here. + - npm caches its TTL clock and only refreshes it from a `setTimeout` + (its `ttlResolution`), so code that blocks the event loop sees entries + stay live indefinitely on npm. This binding reads `performance.now()` + on every access, so a blocking loop does observe expiry. + The GC-survival test moved to its own test binary (`crates/perry-ext-lru-cache/tests/gc_survival.rs`) and now asserts that the collector *relocated* the cached value, not merely that it is still From 5b2ccf3861be243386da844f1eb31db4fb16f9de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 1 Aug 2026 08:21:24 +0200 Subject: [PATCH 6/8] test(lru-cache): stop the survival test from pinning its own subject MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The test kept the value's raw nursery address in a caller local across `gc_collect_minor()`. That word is itself conservatively scannable, so it could pin the very string whose relocation the test asserts — the same pinning class that forced this test into its own binary, one variable deeper. The failure mode is the worst kind: the assertion goes red while proving nothing about the cache-root rewrite. Allocation, insertion and address capture now happen in an `#[inline(never)]` helper that returns only `address ^ ADDRESS_TOKEN_XOR`, so neither the bare address nor the NaN-boxed string is a live local of the frame that spans the collection. The address is decoded again only after the collection has finished. Verified: green on 6 consecutive runs, and `PERRY_GEN_GC=0` still trips the relocation assertion, so the sabotage direction survived the restructuring. --- .../perry-ext-lru-cache/tests/gc_survival.rs | 47 +++++++++++++++---- 1 file changed, 38 insertions(+), 9 deletions(-) diff --git a/crates/perry-ext-lru-cache/tests/gc_survival.rs b/crates/perry-ext-lru-cache/tests/gc_survival.rs index 084f3add94..fa00786138 100644 --- a/crates/perry-ext-lru-cache/tests/gc_survival.rs +++ b/crates/perry-ext-lru-cache/tests/gc_survival.rs @@ -37,7 +37,8 @@ use perry_ext_lru_cache::{js_lru_cache_get, js_lru_cache_new, js_lru_cache_set}; use perry_ffi::{ - alloc_string, nanbox_string_bits, read_bytes, JsString, JsValue, ObjectHeader, StringHeader, + alloc_string, nanbox_string_bits, read_bytes, Handle, JsString, JsValue, ObjectHeader, + StringHeader, }; const POINTER_MASK: u64 = 0x0000_FFFF_FFFF_FFFF; @@ -65,6 +66,35 @@ fn string_value(text: &str) -> f64 { f64::from_bits(nanbox_string_bits(s.as_raw())) } +/// Folded into the address so no stack word held across the collection +/// looks like a heap pointer. +const ADDRESS_TOKEN_XOR: u64 = 0xA5A5_A5A5_A5A5_A5A5; + +/// Allocate the value, hand it to the cache, and return **only** an +/// obfuscated token for the address it started at. +/// +/// `#[inline(never)]` is load-bearing, and so is the fact that neither the +/// raw address nor the NaN-boxed string ever becomes a local of the +/// caller. The collector conservatively pins any nursery object that some +/// live stack word points at — that is precisely the effect this test had +/// to be moved into its own binary to escape. A caller-frame local holding +/// the bare address would pin the very string whose relocation is being +/// asserted, and the assertion would then fail while proving nothing about +/// the cache-root rewrite. Keeping the pointer-shaped values inside a +/// frame that is popped before `gc_collect_minor()` runs, and carrying +/// only `address ^ ADDRESS_TOKEN_XOR` across it, removes that hazard. +#[inline(never)] +fn insert_value_and_take_address_token(handle: Handle) -> u64 { + let value = string_value("value-object-1234567890"); + let address = value.to_bits() & POINTER_MASK; + assert!( + perry_runtime::arena::pointer_in_nursery(address as usize), + "the value must start in the nursery or a minor cannot move it" + ); + js_lru_cache_set(handle, 1.0, value); + address ^ ADDRESS_TOKEN_XOR +} + fn read_string_value(value: f64) -> Option { let ptr = unsafe { js_get_string_pointer_unified(value) } as *mut StringHeader; if ptr.is_null() { @@ -95,14 +125,10 @@ fn cached_value_survives_and_is_rewritten_by_a_copying_minor() { let h = js_lru_cache_new(options_with_max(10.0)); // A >5-byte string forces the heap `StringHeader` repr (not inline // SSO), so it is a real collectable allocation. Its ONLY root is the - // cache — nothing on the stack or in a global refers to it. - let value = string_value("value-object-1234567890"); - let before = value.to_bits() & POINTER_MASK; - js_lru_cache_set(h, 1.0, value); - assert!( - perry_runtime::arena::pointer_in_nursery(before as usize), - "the value must start in the nursery or a minor cannot move it" - ); + // cache — nothing on the stack or in a global refers to it, which is + // why the address is carried across the collection as an obfuscated + // token rather than a live pointer-shaped word. + let before_token = insert_value_and_take_address_token(h); // Reclaims unrooted nursery allocations and evacuates rooted survivors. let _ = perry_runtime::gc::gc_collect_minor(); @@ -120,6 +146,9 @@ fn cached_value_survives_and_is_rewritten_by_a_copying_minor() { // went untested. `PERRY_GEN_GC=0` routes `gc_collect_minor` to // non-moving mark-sweep and will trip this deliberately. let after = got.to_bits() & POINTER_MASK; + // Safe to materialize now: the collection is over, so a pointer-shaped + // stack word can no longer pin anything. + let before = before_token ^ ADDRESS_TOKEN_XOR; assert_ne!( before, after, "minor GC did not relocate the cached value (0x{before:x}), so this run \ From 6515766a3ac3d24aad6d2e38241362834d714422 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 1 Aug 2026 08:25:31 +0200 Subject: [PATCH 7/8] fix(lru-cache): read constructor options through the boxed-receiver getter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `options` is whatever the caller passed. The wrapper classified it with a hand-rolled `is_pointer() && (ptr as usize) >= 0x1000` pair and then unboxed it to a `*const ObjectHeader` for `js_object_get_field_by_name_f64`, which dereferences its argument on faith — correct only where codegen has *proven* the receiver is an object, which nothing does here. Arrays, functions and native handle ids are all pointer-tagged and all cleared that floor. The runtime already exports the right entry point for exactly this situation: `js_object_get_field_by_name_boxed` takes the boxed value and owns the classification (handle-band routing plus the canonical address check). Reading options through it deletes the duplicated band literal — the kind of open-coded address-range test the addr-class ratchet exists to prevent — and makes the receiver rule the runtime's rather than this crate's. No behavior change: measured before and after, strings, empty and populated arrays, and handle-band ids in [0x1000, 0x100000) all produce npm's `TypeError: At least one of max, maxSize, or ttl is required`, which is what npm gives for every heap value without a `max` own property (verified for strings, arrays, Map, Set, functions, Date, RegExp and boxed numbers under Node 26.5.1; `Object.assign([], { max: 3 })` constructs there and the own property is what decides). That parity is now a regression test rather than a property nothing checked. --- crates/perry-ext-lru-cache/src/lib.rs | 68 ++++++++++++++----------- crates/perry-ext-lru-cache/src/tests.rs | 57 +++++++++++++++++++-- 2 files changed, 90 insertions(+), 35 deletions(-) diff --git a/crates/perry-ext-lru-cache/src/lib.rs b/crates/perry-ext-lru-cache/src/lib.rs index 808c9c8743..523dc999d8 100644 --- a/crates/perry-ext-lru-cache/src/lib.rs +++ b/crates/perry-ext-lru-cache/src/lib.rs @@ -85,7 +85,7 @@ use lru::LruCache; use perry_ffi::{ gc_register_mutable_root_scanner_named, iter_handles_of_mut, read_bytes, register_handle, throw_with_code, with_handle_mut, ErrorKind, GcRootVisitor, Handle, JsString, JsValue, - ObjectHeader, StringHeader, + StringHeader, }; use std::num::NonZeroUsize; use std::sync::Once; @@ -101,8 +101,6 @@ const MAX_ARRAY_LENGTH: f64 = 4_294_967_295.0; /// Above this npm's `getUintArray(max)` returns `null` and the constructor /// raises a plain `Error` instead of a `RangeError`. const MAX_SAFE_INTEGER: f64 = 9_007_199_254_740_991.0; -/// Below this a "pointer" is a small integer handle, not a real object. -const MIN_OBJECT_ADDRESS: usize = 0x1000; extern "C" { // Monotonic ms clock — same source npm lru-cache uses (`perf_now`); @@ -111,8 +109,11 @@ extern "C" { // Materialize any string repr (heap `STRING_TAG` or inline SSO // `SHORT_STRING_TAG`) into a real `*StringHeader` so we can read bytes. fn js_get_string_pointer_unified(value: f64) -> i64; - // Read an option field off the NaN-boxed options object. - fn js_object_get_field_by_name_f64(obj: *const ObjectHeader, key: *const StringHeader) -> f64; + // Read an option field off the NaN-boxed options argument. The + // *boxed* variant validates its receiver instead of dereferencing it + // on faith, so a non-object `options` reads as all-undefined fields + // rather than a forged pointer deref (see `option_value`). + fn js_object_get_field_by_name_boxed(receiver: f64, key: *const StringHeader) -> f64; fn js_is_truthy(value: f64) -> i32; // JS `String(n)` — npm interpolates the offending `max` into its // "invalid max value" message, and JS renders `1e300` as `"1e+300"` @@ -277,19 +278,28 @@ fn now_ms() -> f64 { unsafe { js_performance_now() } } -/// Read `options.` as a raw NaN-boxed value. +/// Read `options.` off the NaN-boxed options argument. /// -/// `None` means `options` was not an object. npm destructures its options -/// argument, and destructuring a primitive yields `undefined` for every -/// field rather than throwing, so that is what a non-object reads as here. -fn option_value(fields: Option<*const ObjectHeader>, name: &str) -> JsValue { - let Some(ptr) = fields else { - return JsValue::UNDEFINED; - }; +/// Routed through the runtime's *boxed*-receiver getter instead of +/// unboxing to a `*const ObjectHeader` here. `options` is whatever the +/// caller passed — an object, but equally a string, an array, a function, +/// a native handle id, or a double whose bit pattern lands inside the +/// heap-pointer window. The unboxed getter dereferences its argument on +/// faith, which is fine only when codegen has *proven* the receiver is an +/// object; nothing proves that here. The boxed entry point owns the +/// classification (handle-band routing plus the canonical address check), +/// so this wrapper does not re-implement a pointer-range test the runtime +/// already exports — the previous hand-rolled `is_pointer() && >= 0x1000` +/// pair was both a duplicate of that rule and subtly different from it. +/// +/// npm reads these options by destructuring, which yields `undefined` for +/// every field of a non-object rather than throwing, and that is exactly +/// what the boxed getter returns for one. +fn option_value(options: f64, name: &str) -> JsValue { let key = perry_ffi::alloc_string(name); - // SAFETY: `fields` is a validated non-null object pointer (see - // `js_lru_cache_new`), and `key` owns the freshly allocated header. - let raw = unsafe { js_object_get_field_by_name_f64(ptr, key.as_raw()) }; + // SAFETY: `key` owns the freshly allocated header, and the boxed + // getter validates `options` itself. + let raw = unsafe { js_object_get_field_by_name_boxed(options, key.as_raw()) }; JsValue::from_bits(raw.to_bits()) } @@ -323,8 +333,8 @@ fn js_number_string(n: f64) -> String { /// npm: `const { max = 0 } = options; if (max !== 0 && !isPosInt(max)) throw …` /// followed by the `getUintArray` / `Array.from({ length: max })` bounds. /// Returns the validated `max` (`0` = unbounded); diverges on a bad value. -fn parse_max(fields: Option<*const ObjectHeader>) -> f64 { - let raw = option_value(fields, "max"); +fn parse_max(options: f64) -> f64 { + let raw = option_value(options, "max"); if raw.is_undefined() { return 0.0; // npm's `max = 0` destructuring default } @@ -355,8 +365,8 @@ fn parse_max(fields: Option<*const ObjectHeader>) -> f64 { /// npm: `this.ttl = ttl || 0; if (this.ttl && !isPosInt(this.ttl)) throw …`. /// Returns the validated ttl in ms (`0` = none); diverges on a bad value. -fn parse_ttl(fields: Option<*const ObjectHeader>) -> f64 { - let raw = option_value(fields, "ttl"); +fn parse_ttl(options: f64) -> f64 { + let raw = option_value(options, "ttl"); // `ttl || 0` — undefined, null, `0`, `NaN` and `""` all collapse to 0 // *without* tripping the validation (npm only checks a truthy ttl). // SAFETY: `js_is_truthy` reads a NaN-boxed value by value. @@ -400,16 +410,12 @@ pub extern "C" fn js_lru_cache_new(options: f64) -> Handle { ErrorKind::TypeError, ); } - // Any other primitive destructures cleanly into all-undefined fields. - let fields = if opts.is_pointer() { - let ptr = opts.as_pointer::(); - (!ptr.is_null() && (ptr as usize) >= MIN_OBJECT_ADDRESS).then_some(ptr as *const _) - } else { - None - }; - - let max = parse_max(fields); - let ttl = parse_ttl(fields); + // Everything else — a primitive, a string, an array, a function, a + // native handle id — destructures cleanly into all-undefined fields on + // npm, and `option_value` reproduces that without this code having to + // classify the pointer itself. + let max = parse_max(options); + let ttl = parse_ttl(options); if max == 0.0 && ttl == 0.0 { // npm: "do not allow completely unbounded caches". `maxSize` would // also satisfy this on npm, but it is unimplemented here (see the @@ -423,7 +429,7 @@ pub extern "C" fn js_lru_cache_new(options: f64) -> Handle { // SAFETY: `js_is_truthy` reads a NaN-boxed value by value. let update_age_on_get = unsafe { js_is_truthy(f64::from_bits( - option_value(fields, "updateAgeOnGet").bits(), + option_value(options, "updateAgeOnGet").bits(), )) != 0 }; diff --git a/crates/perry-ext-lru-cache/src/tests.rs b/crates/perry-ext-lru-cache/src/tests.rs index 73be101109..c27a367ea8 100644 --- a/crates/perry-ext-lru-cache/src/tests.rs +++ b/crates/perry-ext-lru-cache/src/tests.rs @@ -10,7 +10,7 @@ //! process. use super::*; -use perry_ffi::{alloc_string, nanbox_string_bits, JsValue}; +use perry_ffi::{alloc_string, nanbox_string_bits, JsValue, ObjectHeader}; extern "C" { fn js_object_alloc(class_id: u32, field_count: u32) -> *mut ObjectHeader; fn js_object_set_field_by_name(obj: *mut ObjectHeader, key: *const StringHeader, value: f64); @@ -58,11 +58,9 @@ fn new_catching(opts: f64) -> Result { match perry_runtime::exception::js_call_catching(|| js_lru_cache_new(opts) as f64) { Ok(handle) => Ok(handle as Handle), Err(bits) => { - let err = JsValue::from_bits(bits.to_bits()); - let ptr = err.as_pointer::(); let field = |name: &str| { let key = alloc_string(name); - let v = unsafe { js_object_get_field_by_name_f64(ptr, key.as_raw()) }; + let v = unsafe { js_object_get_field_by_name_boxed(bits, key.as_raw()) }; read_string_value(v).unwrap_or_default() }; Err((field("name"), field("message"))) @@ -447,3 +445,54 @@ fn non_integer_ttl_is_a_type_error() { // string (minor then reports `copied_objects=0`), and driving // `gc_collect_minor()` in a binary that has also allocated JS objects // SIGSEGVs the copying collector. See that file's module docs. + +// ── heap-typed non-object options ──────────────────────────────────── + +extern "C" { + fn js_array_alloc(capacity: u32) -> *mut std::ffi::c_void; + fn js_array_set_f64_extend(arr: *mut std::ffi::c_void, index: u32, value: f64); + fn js_nanbox_pointer(ptr: i64) -> f64; +} + +/// A heap value that is *not* a plain object must read as all-undefined +/// fields, never as a dereference of a forged object pointer. +/// +/// npm reaches its options by destructuring, so a string, an array, a +/// function, a `Map` — anything without a `max` own property — lands on +/// the same "At least one of max, maxSize, or ttl is required" TypeError. +/// Measured against `lru-cache@11.5.2` under Node 26.5.1 for strings, +/// arrays, `Map`, `Set`, functions, `Date`, `RegExp` and boxed numbers; +/// `Object.assign([], { max: 3 })` constructs fine there, because the own +/// property is what matters, not the exotic-ness of the receiver. +/// +/// The point of the test is the *receiver classification*, not the error: +/// `options` arrives as an untrusted NaN-boxed value, and the wrapper must +/// not unbox it to a `*const ObjectHeader` on faith. Arrays and handle-band +/// ids are the two shapes that pass a naive `is_pointer()` check. +#[test] +fn heap_typed_non_object_options_read_as_undefined_fields() { + const MSG: &str = "At least one of max, maxSize, or ttl is required"; + + assert_throws(string_value("longer-than-inline-string"), "TypeError", MSG); + + let empty = unsafe { js_array_alloc(4) }; + assert_throws(unsafe { js_nanbox_pointer(empty as i64) }, "TypeError", MSG); + + // A populated array has real element bytes behind the cast, so a + // forged-object read would find *something* rather than a zeroed slot. + let populated = unsafe { js_array_alloc(8) }; + for i in 0..8u32 { + unsafe { js_array_set_f64_extend(populated, i, 42.0 + f64::from(i)) }; + } + assert_throws( + unsafe { js_nanbox_pointer(populated as i64) }, + "TypeError", + MSG, + ); + + // Native handle ids: pointer-tagged, above the old hand-rolled 0x1000 + // floor, but small integers rather than heap addresses. + for raw in [0x1001_i64, 0x2000, 0x8000, 0xF_FFFF] { + assert_throws(unsafe { js_nanbox_pointer(raw) }, "TypeError", MSG); + } +} From c1a0852b48b62a747aa00bc435022fdb2f3feb44 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 1 Aug 2026 08:31:45 +0200 Subject: [PATCH 8/8] test(lru-cache): build options objects the way that does not break the collector MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adding constructor tests that pass real JS options objects introduced an intermittent failure: 1 run in 12 died in whichever test happened to run next, once in `string::compare` dereferencing string *content* as a pointer. The pre-existing suite, which allocated no JS objects at all, measured 0 in 12. The trigger is narrower than "JS objects": it is the ordinary-prototype construction path, `js_object_alloc` + `js_object_set_field_by_name`. Built that way the suite also SIGSEGVs deterministically under `--test-threads=1`, in `gc::copying::scan_slot` walking a bogus slot address. Built with `alloc_null_proto_object` it measured 0 failures in 40 parallel and 10 single-threaded runs. Both test files now use the null-prototype construction. Only own properties are read — that is what npm's destructuring does and what the wrapper does — so the prototype is immaterial to what is under test, and the compiled A/B against the npm package still covers the ordinary object literal a real caller writes. This is a workaround in the tests for a fault that lives in the runtime, and it is documented as such in both files rather than left as a mystery for whoever next wonders why these helpers look odd. --- changelog.d/7136-lru-cache-faithful.md | 26 ++++++++- crates/perry-ext-lru-cache/src/tests.rs | 56 ++++++++++--------- .../perry-ext-lru-cache/tests/gc_survival.rs | 40 +++++++------ 3 files changed, 79 insertions(+), 43 deletions(-) diff --git a/changelog.d/7136-lru-cache-faithful.md b/changelog.d/7136-lru-cache-faithful.md index cd57232213..ef6cd2a740 100644 --- a/changelog.d/7136-lru-cache-faithful.md +++ b/changelog.d/7136-lru-cache-faithful.md @@ -82,6 +82,17 @@ stay live indefinitely on npm. This binding reads `performance.now()` on every access, so a blocking loop does observe expiry. + Constructor options are read through the runtime's boxed-receiver getter + (`js_object_get_field_by_name_boxed`) rather than being unboxed here to a + `*const ObjectHeader`. `options` is an untrusted runtime value — an + array, a function and a native handle id are all pointer-tagged — and the + unboxed getter dereferences its argument on faith. This also deletes a + hand-rolled `>= 0x1000` band literal, the kind of open-coded address test + the addr-class ratchet exists to prevent. Behavior is unchanged and now + pinned by a test: strings, empty and populated arrays, and handle-band + ids all yield npm's "At least one of max, maxSize, or ttl is required", + which is what npm gives for any heap value lacking a `max` own property. + The GC-survival test moved to its own test binary (`crates/perry-ext-lru-cache/tests/gc_survival.rs`) and now asserts that the collector *relocated* the cached value, not merely that it is still @@ -91,6 +102,19 @@ conservatively pins any nursery object a stack word points at, so after even one other test in the same binary the minor reports `copied_objects=0` instead of `1`. Verified in both directions — - `PERRY_GEN_GC=0` (non-moving mark-sweep) trips the new assertion. + `PERRY_GEN_GC=0` (non-moving mark-sweep) trips the new assertion. The + address is carried across the collection XOR-folded, from an + `#[inline(never)]` helper, so the test cannot conservatively pin the very + string whose relocation it asserts. + + Both test files build their options objects null-prototype. Building them + the ordinary way (`js_object_alloc` + `js_object_set_field_by_name`) + destabilizes the collector for the rest of the process: SIGSEGV in + `gc::copying::scan_slot` under `--test-threads=1`, and 1 failure in 12 + runs otherwise, landing in whichever unrelated test runs next. Null-proto + objects measured 0 in 40. Only own properties are read, so the prototype + is immaterial to what is under test, and the compiled A/B covers the + ordinary literal a real caller writes. The underlying fault is a runtime + bug, reported separately rather than worked around silently. Tracking: #466 (Phase 5 native bindings). PR #7136. diff --git a/crates/perry-ext-lru-cache/src/tests.rs b/crates/perry-ext-lru-cache/src/tests.rs index c27a367ea8..0e781ef17b 100644 --- a/crates/perry-ext-lru-cache/src/tests.rs +++ b/crates/perry-ext-lru-cache/src/tests.rs @@ -10,12 +10,7 @@ //! process. use super::*; -use perry_ffi::{alloc_string, nanbox_string_bits, JsValue, ObjectHeader}; -extern "C" { - fn js_object_alloc(class_id: u32, field_count: u32) -> *mut ObjectHeader; - fn js_object_set_field_by_name(obj: *mut ObjectHeader, key: *const StringHeader, value: f64); -} - +use perry_ffi::{alloc_string, nanbox_string_bits, JsValue}; /// NaN-boxed `f64` for a freshly allocated JS string with `text`. fn string_value(text: &str) -> f64 { let s = alloc_string(text); @@ -23,27 +18,38 @@ fn string_value(text: &str) -> f64 { f64::from_bits(nanbox_string_bits(s.as_raw())) } -/// Build a real JS options object carrying `fields`, the way a compiled -/// `new LRUCache({ … })` object literal reaches the constructor. +/// Build a real JS options object carrying `fields`. /// -/// The slot count is declared up front rather than using -/// `perry_ffi::alloc_object()` (which allocates zero inline slots and -/// grows on the first `js_object_set_field_by_name`). That is not a -/// stylistic preference: an object built the zero-slot way makes the -/// copying minor collector walk a bogus slot address and SIGSEGV in -/// `gc::copying::scan_slot` on the *next* collection in the process -/// (reproduced with `--test-threads=1`; see the PR discussion). Declaring -/// the slots avoids the grow path entirely. A compiled object literal -/// declares its slots the same way, so this is also the more faithful -/// model of what reaches the constructor. +/// Null-prototype on purpose. `options` is read for *own* properties only +/// — that is what npm's destructuring does and what the wrapper does — so +/// the prototype is immaterial to what is under test, and the compiled +/// A/B against the npm package covers the ordinary object literal a real +/// caller writes. +/// +/// The reason not to build these the ordinary way is that +/// `js_object_alloc(0, n)` followed by `js_object_set_field_by_name` +/// destabilizes the collector for the rest of the process: with options +/// objects built that way this suite SIGSEGVs deterministically under +/// `--test-threads=1` (in `gc::copying::scan_slot`, walking a bogus slot +/// address) and intermittently otherwise — measured at 1 failure in 12 +/// runs, against 0 in 40 with the null-proto construction, and 0 in 12 +/// for the pre-existing suite that allocated no JS objects at all. The +/// failure lands in whichever test happens to run next, e.g. a string +/// compare dereferencing string *content* as a pointer. That is a runtime +/// bug rather than anything this binding does; it is written up on the PR +/// so it can be fixed where it lives instead of being rediscovered from a +/// mystery flake here. fn options(fields: &[(&str, f64)]) -> f64 { - let ptr = unsafe { js_object_alloc(0, fields.len() as u32) }; - assert!(!ptr.is_null(), "js_object_alloc returned null"); - for (name, value) in fields { - let key = alloc_string(name); - unsafe { js_object_set_field_by_name(ptr, key.as_raw(), *value) }; - } - f64::from_bits(JsValue::from_object_ptr(ptr).bits()) + let boxed: Vec<(&str, JsValue)> = fields + .iter() + .map(|(n, v)| (*n, JsValue::from_bits(v.to_bits()))) + .collect(); + let obj = perry_ffi::alloc_null_proto_object(&boxed); + assert!( + obj.is_pointer(), + "alloc_null_proto_object returned a non-object" + ); + f64::from_bits(obj.bits()) } /// `new LRUCache({ max })` through the real constructor. diff --git a/crates/perry-ext-lru-cache/tests/gc_survival.rs b/crates/perry-ext-lru-cache/tests/gc_survival.rs index fa00786138..ecacb5b5fb 100644 --- a/crates/perry-ext-lru-cache/tests/gc_survival.rs +++ b/crates/perry-ext-lru-cache/tests/gc_survival.rs @@ -18,12 +18,15 @@ //! binary, this string gets pinned and the minor reports //! `copied_objects=0` (measured; alone it reports `copied_objects=1`), //! so the relocation assertion fails through no fault of the binding. -//! - Worse, driving `gc_collect_minor()` from a unit-test binary that has -//! also allocated JS *objects* (which the constructor tests must do, to -//! pass a real options object) makes the copying collector walk a bogus -//! slot address and SIGSEGV in `gc::copying::scan_slot`. That -//! reproduces deterministically under `--test-threads=1` and is a -//! runtime-level fragility, not something this binding controls. +//! - Worse, a unit-test binary that has built an ordinary-prototype JS +//! object with `js_object_alloc` + `js_object_set_field_by_name` — the +//! obvious way to pass a real options object — makes the copying +//! collector walk a bogus slot address and SIGSEGV in +//! `gc::copying::scan_slot`, deterministically under +//! `--test-threads=1`. Null-prototype objects do not trip it, which is +//! what both test files now build, but the hazard is a runtime bug +//! rather than something this binding controls, so this file keeps its +//! distance from other tests regardless. //! //! A dedicated integration binary gets a pristine process: an empty //! nursery, a shallow stack, and no JS objects. The relocation then @@ -37,27 +40,30 @@ use perry_ext_lru_cache::{js_lru_cache_get, js_lru_cache_new, js_lru_cache_set}; use perry_ffi::{ - alloc_string, nanbox_string_bits, read_bytes, Handle, JsString, JsValue, ObjectHeader, - StringHeader, + alloc_string, nanbox_string_bits, read_bytes, Handle, JsString, JsValue, StringHeader, }; const POINTER_MASK: u64 = 0x0000_FFFF_FFFF_FFFF; const TAG_UNDEFINED: u64 = 0x7FFC_0000_0000_0001; extern "C" { - fn js_object_alloc(class_id: u32, field_count: u32) -> *mut ObjectHeader; - fn js_object_set_field_by_name(obj: *mut ObjectHeader, key: *const StringHeader, value: f64); fn js_get_string_pointer_unified(value: f64) -> i64; } -/// `{ max: }` as a real JS object, the way a compiled object -/// literal reaches the constructor. +/// `{ max: }` as a real JS object. +/// +/// Null-prototype for the same reason as the unit tests' helper: building +/// it with `js_object_alloc` + `js_object_set_field_by_name` destabilizes +/// the collector for the rest of the process, and this file exists +/// precisely to run a collection. Only own properties are read, so the +/// prototype does not matter to what is under test. fn options_with_max(max: f64) -> f64 { - let ptr = unsafe { js_object_alloc(0, 1) }; - assert!(!ptr.is_null(), "js_object_alloc returned null"); - let key = alloc_string("max"); - unsafe { js_object_set_field_by_name(ptr, key.as_raw(), max) }; - f64::from_bits(JsValue::from_object_ptr(ptr).bits()) + let obj = perry_ffi::alloc_null_proto_object(&[("max", JsValue::from_bits(max.to_bits()))]); + assert!( + obj.is_pointer(), + "alloc_null_proto_object returned a non-object" + ); + f64::from_bits(obj.bits()) } fn string_value(text: &str) -> f64 {