diff --git a/changelog.d/7445-parallel-test-order-independence.md b/changelog.d/7445-parallel-test-order-independence.md new file mode 100644 index 0000000000..4e47d935c9 --- /dev/null +++ b/changelog.d/7445-parallel-test-order-independence.md @@ -0,0 +1 @@ +test(runtime): make the `perry-runtime --lib` suite order-independent under default-parallel `cargo test` (#6965). The deterministic `namespace_members_exist_with_expected_shapes` failure was the gc test guards' global side-table reset wiping `CLOSURE_PROPS` between an unguarded test's install and read-back; the same sweep fixes the `tests_1802` one-shot `try_lock` race (and its `PoisonError` cascade), the `prop_plan` record→check races against `PROP_PLAN_EPOCH`/`VTABLE_GEN`, the gc teardown tests' exact-delta asserts on process-global deallocation counters, five more `object/tests.rs` populate-then-assert tests (three of which were themselves unguarded wipers), and a `url` vs `typed_feedback` process-cwd race (new crate-wide `cfg(test)` `process_cwd_test_lock`). 19 consecutive parallel full-suite runs green; `--test-threads=1` (CI's mode) unchanged. diff --git a/crates/perry-runtime/src/closure/dynamic_props.rs b/crates/perry-runtime/src/closure/dynamic_props.rs index 7e01feb752..73a3d07244 100644 --- a/crates/perry-runtime/src/closure/dynamic_props.rs +++ b/crates/perry-runtime/src/closure/dynamic_props.rs @@ -843,6 +843,16 @@ mod tests_1802 { static SIDE_TABLE_TEST_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + /// Poison-tolerant acquisition: one test's assert failure must read as + /// ONE failure, not cascade `PoisonError` panics into every sibling that + /// serializes on this lock (#6965 — the observed second failure). The + /// guarded data is `()`, so poison carries no corruption to tolerate. + fn side_table_test_lock() -> std::sync::MutexGuard<'static, ()> { + SIDE_TABLE_TEST_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + } + /// #1802: the side-table values must be visited in mark phases, not /// only during the metadata-rewrite tail. Pre-fix /// `scan_closure_dynamic_props_roots_mut` early-returned unless @@ -857,7 +867,7 @@ mod tests_1802 { // threads, wiping this test's parked entry mid-assertion. Serialize // against those guards, THEN against this module's own tests. let _global = crate::gc::global_side_table_test_lock(); - let _guard = SIDE_TABLE_TEST_LOCK.lock().unwrap(); + let _guard = side_table_test_lock(); // A unique synthetic closure address (just an integer key — the // scanner doesn't deref it during value visitation; the // metadata-key visitor is a no-op for non-heap addresses). @@ -896,7 +906,7 @@ mod tests_1802 { // threads, wiping this test's parked entry mid-assertion. Serialize // against those guards, THEN against this module's own tests. let _global = crate::gc::global_side_table_test_lock(); - let _guard = SIDE_TABLE_TEST_LOCK.lock().unwrap(); + let _guard = side_table_test_lock(); let owner: usize = 0xC10C_AB1E_0000_1803; let value_bits: u64 = 0x7FFD_AAAA_BBBB_CCCD; closure_set_dynamic_prop(owner, "errors", f64::from_bits(value_bits)); @@ -907,7 +917,22 @@ mod tests_1802 { let mut mark = |v: f64| { if v.to_bits() == value_bits { saw_value = true; - lock_was_free = get_closure_props().try_lock().is_ok(); + // The regression under test is the SCANNER holding + // CLOSURE_PROPS across visitor callbacks — a same-thread + // hold, so `try_lock` can never succeed no matter how + // long we wait. A one-shot `try_lock` also fails on + // transient contention from an unrelated parallel test + // thread's brief map access (#6965) — retry with a yield + // so a foreign holder gets to release. `Poisoned` counts + // as free: poison means a panicking holder already + // RELEASED the mutex. + lock_was_free = (0..4096).any(|_| match get_closure_props().try_lock() { + Ok(_) | Err(std::sync::TryLockError::Poisoned(_)) => true, + Err(std::sync::TryLockError::WouldBlock) => { + std::thread::yield_now(); + false + } + }); } }; let mut visitor = crate::gc::RuntimeRootVisitor::for_copy(&mut mark); @@ -935,7 +960,7 @@ mod tests_1802 { // threads, wiping this test's parked entry mid-assertion. Serialize // against those guards, THEN against this module's own tests. let _global = crate::gc::global_side_table_test_lock(); - let _guard = SIDE_TABLE_TEST_LOCK.lock().unwrap(); + let _guard = side_table_test_lock(); let obj = crate::object::js_object_alloc(0, 0) as usize; assert_eq!( diff --git a/crates/perry-runtime/src/gc/tests/teardown.rs b/crates/perry-runtime/src/gc/tests/teardown.rs index a3664da267..406afdd19a 100644 --- a/crates/perry-runtime/src/gc/tests/teardown.rs +++ b/crates/perry-runtime/src/gc/tests/teardown.rs @@ -1,7 +1,22 @@ use super::support::GcTriggerThresholdTestGuard; +/// The Map/Set side-deallocation counters are PROCESS-global, and every test +/// in this module measures deltas of them across a spawn/join window — run +/// concurrently (default-parallel `cargo test`, sharpest under a filter that +/// leaves few other tests), the probe threads' releases land inside each +/// other's windows and the exact-delta asserts read the sum (#6965). +/// Serialize the module. Poison-tolerant so one failure doesn't cascade +/// `PoisonError`s into the siblings. +fn teardown_counter_lock() -> std::sync::MutexGuard<'static, ()> { + static TEARDOWN_COUNTER_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + TEARDOWN_COUNTER_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) +} + #[test] fn map_set_side_allocations_release_on_thread_exit() { + let _counters = teardown_counter_lock(); // #7056: drives the BUDGETED stepper via `complete_budgeted_gc_cycle`, // which the shipped default bypasses (scavenge defers alloc-point // collections to a precise safepoint). Pin legacy pacing so the cycle @@ -25,14 +40,21 @@ fn map_set_side_allocations_release_on_thread_exit() { let map_after = crate::map::test_map_side_deallocation_snapshot(); let set_after = crate::set::test_set_side_deallocation_snapshot(); - assert_eq!(map_after.0 - map_before.0, 64); - assert_eq!(map_after.1 - map_before.1, 4096); - assert_eq!(set_after.0 - set_before.0, 64); - assert_eq!(set_after.1 - set_before.1, 2048); + // The deallocation counters are PROCESS-global: under default-parallel + // `cargo test`, any other test thread exiting (or deallocating Maps/Sets) + // inside the spawn/join window bumps them too, so an exact-delta assert + // is order-dependent (#6965). Lower bounds still prove the property under + // test — the probe thread's exit released ITS 64 Maps/Sets — while an + // under-release regression still lands below them in the serial CI run. + assert!(map_after.0 - map_before.0 >= 64); + assert!(map_after.1 - map_before.1 >= 4096); + assert!(set_after.0 - set_before.0 >= 64); + assert!(set_after.1 - set_before.1 >= 2048); } #[test] fn map_set_side_allocations_release_exactly_once() { + let _counters = teardown_counter_lock(); let map_before = crate::map::test_map_side_deallocation_snapshot(); let set_before = crate::set::test_set_side_deallocation_snapshot(); @@ -89,18 +111,19 @@ fn map_set_side_allocations_release_exactly_once() { let map_after = crate::map::test_map_side_deallocation_snapshot(); let set_after = crate::set::test_set_side_deallocation_snapshot(); - assert_eq!( - (map_after.0 - map_before.0, map_after.1 - map_before.1), - (2, 128) - ); - assert_eq!( - (set_after.0 - set_before.0, set_after.1 - set_before.1), - (2, 64) - ); + // Cross-thread window: lower bounds, same rationale as + // map_set_side_allocations_release_on_thread_exit (#6965). The + // exactly-once core property is the exact-delta pair asserted INSIDE the + // probe thread above. + assert!(map_after.0 - map_before.0 >= 2); + assert!(map_after.1 - map_before.1 >= 128); + assert!(set_after.0 - set_before.0 >= 2); + assert!(set_after.1 - set_before.1 >= 64); } #[test] fn map_set_owner_records_follow_growth() { + let _counters = teardown_counter_lock(); let map_before = crate::map::test_map_side_deallocation_snapshot(); let set_before = crate::set::test_set_side_deallocation_snapshot(); @@ -134,12 +157,12 @@ fn map_set_owner_records_follow_growth() { let map_after = crate::map::test_map_side_deallocation_snapshot(); let set_after = crate::set::test_set_side_deallocation_snapshot(); - assert_eq!( - (map_after.0 - map_before.0, map_after.1 - map_before.1), - (1, 128) - ); - assert_eq!( - (set_after.0 - set_before.0, set_after.1 - set_before.1), - (1, 64) - ); + // Cross-thread window: lower bounds, same rationale as + // map_set_side_allocations_release_on_thread_exit (#6965). The growth + // ownership itself is asserted exactly INSIDE the probe thread above; the + // grown (128/64-byte) release still has to land for these to hold. + assert!(map_after.0 - map_before.0 >= 1); + assert!(map_after.1 - map_before.1 >= 128); + assert!(set_after.0 - set_before.0 >= 1); + assert!(set_after.1 - set_before.1 >= 64); } diff --git a/crates/perry-runtime/src/lib.rs b/crates/perry-runtime/src/lib.rs index 081b9399c4..efd36597df 100644 --- a/crates/perry-runtime/src/lib.rs +++ b/crates/perry-runtime/src/lib.rs @@ -141,6 +141,11 @@ pub mod symbol; /// TC39 Temporal API (#4686): `Temporal.Duration`, `Temporal.Instant`, /// `Temporal.PlainDate`, … wrapping the pure-Rust `temporal_rs` engine. pub mod temporal; +/// Cross-module test-only serialization primitives (#6965). Nothing here +/// exists outside `cfg(test)`; production code must not grow a dependency on +/// it. +#[cfg(test)] +pub(crate) mod test_support; pub mod text; pub mod timer; pub mod typed_feedback; diff --git a/crates/perry-runtime/src/object/global_this_webassembly.rs b/crates/perry-runtime/src/object/global_this_webassembly.rs index 4fc337f849..ee8d2dfa2f 100644 --- a/crates/perry-runtime/src/object/global_this_webassembly.rs +++ b/crates/perry-runtime/src/object/global_this_webassembly.rs @@ -1235,6 +1235,15 @@ mod tests { #[test] fn namespace_members_exist_with_expected_shapes() { + // The constructor `prototype` slots installed by + // `install_webassembly_constructor` live in the PROCESS-global + // `CLOSURE_PROPS` side table, and the gc test guards' state reset + // (`test_clear_closure_side_tables`) clears that table from whatever + // parallel test thread runs it — wiping the entries between this + // test's install and its `webassembly_constructor_proto` read-back + // ("Module.prototype must exist", #6965). Serialize with those + // guards per the `global_side_table_test_lock` contract. + let _global = crate::gc::global_side_table_test_lock(); let ns = create_webassembly_namespace(); assert!(value_heap_ptr(ns).is_some(), "namespace must be an object"); diff --git a/crates/perry-runtime/src/object/prop_plan.rs b/crates/perry-runtime/src/object/prop_plan.rs index c2b33e1c5c..00d56da0f3 100644 --- a/crates/perry-runtime/src/object/prop_plan.rs +++ b/crates/perry-runtime/src/object/prop_plan.rs @@ -246,11 +246,28 @@ pub(crate) fn read_plan_record(keys_id: usize, key_ptr: usize, field_idx: u32) { mod tests { use super::*; + /// A recorded verdict is invalidated by two PROCESS-global counters: + /// `PROP_PLAN_EPOCH` (bumped by every GC cycle's dead-owner fan-out and + /// every descriptor install, on any thread) and `VTABLE_GEN` (bumped by + /// every class method/getter registration in any parallel test). A bump + /// landing between record and check legitimately flushes the entry, so a + /// single-shot `record → assert(check)` is order-dependent under + /// default-parallel `cargo test` (#6965). Retry instead: a genuine + /// record/check regression fails every lap, while a concurrent bump only + /// costs one. The caches themselves are thread-local, so nothing a + /// parallel thread does can turn a MISS assertion into a spurious hit — + /// only the positive direction needs this. + fn store_plan_records_and_hits(class_id: u32, key_ptr: usize) -> bool { + (0..64).any(|_| { + store_plan_record(class_id, key_ptr); + store_plan_check(class_id, key_ptr) + }) + } + #[test] fn record_then_check_hits_and_epoch_bump_invalidates() { let key = 0xDEAD_BEE0usize; - store_plan_record(7, key); - assert!(store_plan_check(7, key)); + assert!(store_plan_records_and_hits(7, key)); // Different class or key misses. assert!(!store_plan_check(8, key)); assert!(!store_plan_check(7, key + 16)); @@ -258,15 +275,13 @@ mod tests { prop_plan_epoch_bump(); assert!(!store_plan_check(7, key)); // Re-record under the new epoch works again. - store_plan_record(7, key); - assert!(store_plan_check(7, key)); + assert!(store_plan_records_and_hits(7, key)); } #[test] fn vtable_generation_bump_invalidates() { let key = 0xBEEF_00F0usize; - store_plan_record(9, key); - assert!(store_plan_check(9, key)); + assert!(store_plan_records_and_hits(9, key)); crate::object::class_registry::test_bump_vtable_generation(); assert!(!store_plan_check(9, key)); } @@ -275,8 +290,13 @@ mod tests { fn read_plan_roundtrip_and_epoch_flush() { let keys = 0xAAAA_0040usize; let key = 0xBBBB_0080usize; - read_plan_record(keys, key, 21); - assert_eq!(read_plan_lookup(keys, key), Some(21)); + // Same order-dependence as the store-plan tests: a concurrent global + // epoch bump between record and lookup flushes the thread-local + // entry, so retry the roundtrip. + assert!((0..64).any(|_| { + read_plan_record(keys, key, 21); + read_plan_lookup(keys, key) == Some(21) + })); assert_eq!(read_plan_lookup(keys, key + 8), None); prop_plan_epoch_bump(); assert_eq!(read_plan_lookup(keys, key), None); diff --git a/crates/perry-runtime/src/object/tests.rs b/crates/perry-runtime/src/object/tests.rs index d48c6bb7a4..bfa1f53a03 100644 --- a/crates/perry-runtime/src/object/tests.rs +++ b/crates/perry-runtime/src/object/tests.rs @@ -117,6 +117,10 @@ extern "C" fn to_iso_string_sentinel(_closure: *const crate::closure::ClosureHea #[test] fn date_to_json_number_hint_honors_symbol_to_primitive() { + // The @@toPrimitive install lands in the PROCESS-global SYMBOL_PROPERTIES + // table, which the gc test guards' state reset wipes from parallel test + // threads (#6965). Hold the global side-table lock. + let _global = crate::gc::global_side_table_test_lock(); unsafe { let receiver = js_object_alloc(0, 0); let receiver_value = crate::value::js_nanbox_pointer(receiver as i64); @@ -156,6 +160,8 @@ fn date_to_json_number_hint_honors_symbol_to_primitive() { #[test] fn date_to_json_symbol_to_primitive_object_result_throws() { + // See date_to_json_number_hint_honors_symbol_to_primitive (#6965). + let _global = crate::gc::global_side_table_test_lock(); unsafe { let receiver = js_object_alloc(0, 0); let receiver_value = crate::value::js_nanbox_pointer(receiver as i64); @@ -194,6 +200,11 @@ fn date_to_json_symbol_to_primitive_object_result_throws() { #[test] fn builtin_prototype_methods_reject_dynamic_new() { + // `installed_builtin_method` reads each constructor's `prototype` off a + // closure — a PROCESS-global `CLOSURE_PROPS` entry the gc test guards' + // state reset wipes from parallel test threads (#6965). Hold the global + // side-table lock across the populate-then-assert. + let _global = crate::gc::global_side_table_test_lock(); unsafe { for (ctor, method) in [ ("Date", "toJSON"), @@ -267,6 +278,11 @@ fn recorded_prototype_constructor_overrides_plain_object_constructor() { #[test] fn closure_name_and_length_ignore_plain_assignment() { + // The closure side tables are PROCESS-global: the clear below must not + // land mid-test in a parallel lock-holder's populate-then-assert window, + // and this test's own populate-then-assert must not be wiped by the gc + // test guards' state reset (#6965). Hold the global side-table lock. + let _global = crate::gc::global_side_table_test_lock(); crate::closure::test_clear_closure_side_tables(); { let closure = crate::closure::js_closure_alloc( @@ -301,6 +317,9 @@ fn closure_name_and_length_ignore_plain_assignment() { #[test] fn closure_name_can_be_redefined_with_define_property() { + // See closure_name_and_length_ignore_plain_assignment: the clear and the + // populate-then-assert both need the global side-table lock (#6965). + let _global = crate::gc::global_side_table_test_lock(); crate::closure::test_clear_closure_side_tables(); { let closure = crate::closure::js_closure_alloc( @@ -376,6 +395,9 @@ extern "C" fn closure_accessor_getter(_closure: *const crate::closure::ClosureHe #[test] fn closure_accessor_define_property_is_own_and_invoked() { + // See closure_name_and_length_ignore_plain_assignment: the clear and the + // populate-then-assert both need the global side-table lock (#6965). + let _global = crate::gc::global_side_table_test_lock(); crate::closure::test_clear_closure_side_tables(); let closure = crate::closure::js_closure_alloc( crate::object::global_this_builtin_noop_thunk as *const u8, @@ -429,6 +451,9 @@ fn closure_accessor_define_property_is_own_and_invoked() { #[test] fn symbol_define_property_attrs_round_trip_descriptor() { + // The symbol side tables are PROCESS-global: the clear below and the + // populate-then-assert both need the global side-table lock (#6965). + let _global = crate::gc::global_side_table_test_lock(); crate::symbol::test_clear_symbol_side_table_roots(); unsafe { let obj = js_object_alloc(0, 0); @@ -560,6 +585,10 @@ fn test_object_to_value_roundtrip() { #[test] fn text_encoding_stream_globals_construct_readable_writable_shape() { + // Constructing the stream globals reads their `prototype` slots out of + // the PROCESS-global CLOSURE_PROPS table (#6965). Hold the global + // side-table lock across the populate-then-construct. + let _global = crate::gc::global_side_table_test_lock(); unsafe { let global_ptr = js_object_alloc(0, 0); super::global_this::populate_global_this_builtins(global_ptr); @@ -611,6 +640,10 @@ fn text_encoding_stream_globals_construct_readable_writable_shape() { #[test] fn navigator_global_constructor_identity_shape() { { + // The constructor's `prototype` read below goes through the + // PROCESS-global CLOSURE_PROPS table (#6965). Hold the global + // side-table lock across the populate-then-assert. + let _global = crate::gc::global_side_table_test_lock(); let ctor_raw = test_global_this_builtin_constructor_value("Navigator"); let ctor = JSValue::from_bits(ctor_raw.to_bits()); assert!(ctor.is_pointer()); diff --git a/crates/perry-runtime/src/test_support.rs b/crates/perry-runtime/src/test_support.rs new file mode 100644 index 0000000000..cf9b396e46 --- /dev/null +++ b/crates/perry-runtime/src/test_support.rs @@ -0,0 +1,23 @@ +//! Test-only serialization for PROCESS-global state that is neither a runtime +//! side table (those use `gc::global_side_table_test_lock`) nor thread-local. +//! +//! First resident: the process working directory. `std::env::set_current_dir` +//! is process-wide, so a test that changes it (`typed_feedback`'s +//! `CurrentDirGuard`) races every parallel test that reads +//! `std::env::current_dir()` more than once and compares (the `url` +//! path-to-file-URL tests read it once for the expectation and again inside +//! the resolver) — observed as an intermittent +//! `path_to_file_url_posix_does_not_add_slash_without_input_slash` failure +//! under default-parallel `cargo test` (#6965). + +/// Serializes tests that MUTATE the process working directory against tests +/// that read it multiple times and compare. Writers hold it for the whole +/// mutation window (guard lifetime); readers hold it across their +/// read-then-compare span. Poison-tolerant: the guarded data is `()`, and one +/// test's failure must not cascade `PoisonError`s into every sibling. +pub(crate) fn process_cwd_test_lock() -> std::sync::MutexGuard<'static, ()> { + static PROCESS_CWD_TEST_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + PROCESS_CWD_TEST_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) +} diff --git a/crates/perry-runtime/src/typed_feedback/tests.rs b/crates/perry-runtime/src/typed_feedback/tests.rs index cddc80e03d..bbb17f5019 100644 --- a/crates/perry-runtime/src/typed_feedback/tests.rs +++ b/crates/perry-runtime/src/typed_feedback/tests.rs @@ -122,13 +122,22 @@ impl Drop for EnvGuard { struct CurrentDirGuard { previous: std::path::PathBuf, + /// The process cwd is PROCESS-global: hold the crate-wide cwd lock for + /// the guard's lifetime so parallel tests that read-then-compare + /// `current_dir()` (the url path-to-file-URL tests) never observe the + /// temporary directory (#6965). + _lock: std::sync::MutexGuard<'static, ()>, } impl CurrentDirGuard { fn set(path: &std::path::Path) -> Self { + let lock = crate::test_support::process_cwd_test_lock(); let previous = std::env::current_dir().expect("current dir"); std::env::set_current_dir(path).expect("set current dir"); - Self { previous } + Self { + previous, + _lock: lock, + } } } diff --git a/crates/perry-runtime/src/url/node_compat.rs b/crates/perry-runtime/src/url/node_compat.rs index 57320c16d7..9ecd9621fc 100644 --- a/crates/perry-runtime/src/url/node_compat.rs +++ b/crates/perry-runtime/src/url/node_compat.rs @@ -121,6 +121,11 @@ mod tests { #[cfg(not(windows))] #[test] fn path_to_file_url_posix_preserves_relative_trailing_slash() { + // The expectation reads `current_dir()` here and the resolver reads + // it again internally — a parallel test that temporarily changes the + // PROCESS-global cwd (typed_feedback's CurrentDirGuard) can land + // between the two (#6965). Hold the crate-wide cwd lock. + let _cwd_lock = crate::test_support::process_cwd_test_lock(); let cwd = cwd(); let parent = std::env::current_dir() .expect("current dir") @@ -144,6 +149,10 @@ mod tests { #[test] fn path_to_file_url_posix_does_not_add_slash_without_input_slash() { + // See path_to_file_url_posix_preserves_relative_trailing_slash: the + // two `current_dir()` reads must not straddle a parallel cwd change + // (#6965). + let _cwd_lock = crate::test_support::process_cwd_test_lock(); let cwd = cwd(); assert_eq!(