test(runtime): make the --lib suite order-independent under default-parallel cargo test (#6965) - #7445
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR makes ChangesRuntime test isolation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…er default-parallel cargo test (#6965) Root causes, all process-global test-visible state raced by parallel test threads (CI's --test-threads=1 never sees any of them): 1. The gc test guards' state reset (test_clear_closure_side_tables and friends) wipes CLOSURE_PROPS / SYMBOL_PROPERTIES from whatever thread runs it. Tests that populate-then-assert those globals without holding global_side_table_test_lock lose their entries mid-test — the deterministic 'Module.prototype must exist' failure, plus the same shape in object/tests.rs (three of which were themselves unguarded wipers). 2. tests_1802's scanner test asserted a ONE-SHOT try_lock succeeds; any parallel thread briefly holding CLOSURE_PROPS failed it spuriously, and the panic poisoned the module's test mutex, cascading a PoisonError into its sibling. 3. The prop_plan tests race two global invalidation counters (PROP_PLAN_EPOCH: every GC cycle; VTABLE_GEN: every class registration) between record and check. 4. The gc teardown tests measure exact deltas of the process-global Map/Set side-deallocation counters across spawn/join windows — the siblings' probe threads land inside each other's windows. 5. typed_feedback's CurrentDirGuard mutates the process cwd while the url path-to-file-URL tests read current_dir() twice and compare. Fixes: take global_side_table_test_lock in every affected populate-then-assert test; retry the try_lock probe (a same-thread scanner hold can never succeed, a foreign hold releases); make test-lock acquisition poison-tolerant; retry prop_plan record→check laps (a real regression fails every lap; a concurrent bump costs one); serialize the teardown module and lower-bound its cross-thread outer-window asserts (the exact-delta core properties stay asserted inside the probe threads); add a crate-wide cfg(test) process-cwd lock held by the writer guard and the url readers. Verified: 19 consecutive default-parallel full-suite runs green (1646/1646) plus 5 runs of the harsher filtered reproducer; --test-threads=1 still green. native_module_stream and gc::tests::runtime_roots (the issue's other named families) never reproduced in any of those runs.
|
Audited and merged, rebased onto today's Reproduced the "before" independently on macOS arm64 (16 threads): 3 default-parallel runs on One correction worth having, in your favour. You listed The assertion weakening is sound, and I checked rather than assumed. Converting The retry loops can still fail — proven by sabotage, not by reading. A bounded retry under an assert is the other classic way to get a gate that cannot go red, so I made Your reasoning that only the positive direction needs the retry — thread-local caches mean no parallel thread can turn a MISS into a spurious hit — holds, and it's why the negative assertions correctly stayed single-shot. Note that |
cdb6cf1 to
4461e37
Compare
1 participant
Fixes #6965.
What was actually happening
namespace_members_exist_with_expected_shapeswas not reading torn-down WebAssembly state — it was losing its constructorprototypeentries out of the process-globalCLOSURE_PROPSside table.install_webassembly_constructorstores each ctor'sprototypethere; the gc test guards' state reset (reset_copying_nursery_runtime_test_state→test_clear_closure_side_tables) clears that table from whatever parallel test thread runs it. With ~1600 tests in flight, a wipe lands between the test's install and itswebassembly_constructor_protoread-back on virtually every run — which is why it read as deterministic in parallel and green at--test-threads=1(CI's canonical mode never runs anything concurrently with it).The repo already has a contract for exactly this: such tests must hold
gc::global_side_table_test_lock(the guards hold it for their lifetime). The failing tests simply didn't.The other families (acceptance item: do they share the root cause?)
closure::dynamic_props(tests_1802)try_locksucceeds — any parallel thread briefly holdingCLOSURE_PROPS(e.g. the WebAssembly test installing prototypes!) failed it spuriously. That panic then poisoned the module's test mutex, cascading aPoisonErrorinto its sibling — one real race read as three failures.prop_planstore_plan_checkverdicts are invalidated byPROP_PLAN_EPOCH(bumped by every GC cycle's dead-owner fan-out, any thread) andVTABLE_GEN(bumped by every class method registration, any thread). A bump betweenrecordandchecklegitimately flushes the entry, so single-shot asserts are order-dependent. No lock can help (the bump sources are unguarded by design) — the tests now retry; a genuine regression still fails every lap.gc::tests::teardownnative_module_stream,gc::tests::runtime_rootsTwo additional faces surfaced while verifying and are fixed here too:
object/tests.rs:builtin_prototype_methods_reject_dynamic_new(reads ctorprototypethroughCLOSURE_PROPSviapopulate_global_this_builtins), the twodate_to_json_*tests (populate-then-read the globalSYMBOL_PROPERTIEStable),text_encoding_stream/navigatorshape tests — and notably three tests in that file were themselves unguarded wipers, callingtest_clear_closure_side_tables()without the lock.url::node_compat: the path-to-file-URL tests readcurrent_dir()twice and compare, racingtyped_feedback'sCurrentDirGuard(std::env::set_current_diris process-wide). Added a crate-widecfg(test)cwd lock (test_support::process_cwd_test_lock) held by the writer guard and the readers.Hardening that keeps failures honest
unwrap_or_else(PoisonError::into_inner)) — one assert failure reads as one failure, not a cascade. The guarded data is()in every case.try_lockprobe retries with a yield: the regression under test is a same-thread scanner hold, which can never succeed no matter how long we wait, while a foreign holder releases in microseconds.Poisonedcounts as free (poison means a panicking holder released it).Not touched (deliberately)
CLOSURE_PROPSaccessors useif let Ok(...) = lock(), which silently treats a poisoned mutex as empty. Poison only arises from a panic while holding, which the fixes above remove from the test suite; migrating those ~dozen production sites to poison-tolerant locking is a separate cleanup.exactly_once's inner idempotence asserts still compare global-counter snapshots exactly; a non-sibling thread exit inside that microsecond window remains theoretically possible. Making that airtight needs per-thread deallocation accounting — out of scope.Verification
cargo test -p perry-runtime --libfailed 2–4 tests every run (reproduced on Windows: the WebAssembly test + tests_1802 ×2 + prop_plan, exactly the issue's families).-- tests_1802 prop_plan global_this_webassembly teardown node_compat) green, and--test-threads=1(CI's mode) green.cargo fmt --checkclean; clippy introduces no new lints in touched files;check_file_size.shandaddr_class_inventory.pypass.No version bump (maintainer bumps at merge). Changelog fragment follows in a second commit once this PR has its number.
Summary by CodeRabbit
Bug Fixes
Documentation