From c3405f0e35d07095dc0a4012c7487af7e60b3f67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 27 Sep 2026 15:47:25 +0200 Subject: [PATCH 1/2] test(stdlib): take the external-buffer thread-exit verdict at release, not by address (#11547) thread_exit_releases_the_threads_external_buffer_registrations checked the external-buffer registries for the dead buffer's ADDRESS after join. The exiting thread's arena goes back to the allocator, and a concurrent test (thread_exit_releases_the_threads_crypto_key_entries) registers a CryptoKey Buffer through the same js_buffer_mark_as_crypto_key_external, so it can re-register that address before the check runs. Record the registries' state from a thread-exit range hook that runs after their own release hook, while the block still belongs to the exiting thread. Add external_registries_hold_for_test, a thread-local-free probe of the three process-global registries that is safe from a TLS destructor. --- crates/perry-runtime/src/buffer/header.rs | 28 +++++ crates/perry-runtime/src/buffer/mod.rs | 11 +- .../symbols_tests.rs | 110 +++++++++++++++--- 3 files changed, 125 insertions(+), 24 deletions(-) diff --git a/crates/perry-runtime/src/buffer/header.rs b/crates/perry-runtime/src/buffer/header.rs index 05c47ccabfb..a07dc8a5518 100644 --- a/crates/perry-runtime/src/buffer/header.rs +++ b/crates/perry-runtime/src/buffer/header.rs @@ -64,6 +64,34 @@ pub fn is_external_buffer(addr: usize) -> bool { .unwrap_or(false) } +/// Test probe (#11547): is `addr` in each of the three PROCESS-GLOBAL +/// external registries (`EXTERNAL_BUFFER_REGISTRY`, +/// `EXTERNAL_UINT8ARRAY_REGISTRY`, `EXTERNAL_CRYPTO_KEY_META_REGISTRY`)? +/// +/// Unlike `is_uint8array_buffer` / `crypto_key_meta`, which consult this +/// thread's registries first, it touches no thread-local, so a thread-exit +/// range hook may call it from a TLS destructor. Plain locks, no allocation. +#[doc(hidden)] +pub fn external_registries_hold_for_test(addr: usize) -> [bool; 3] { + use std::sync::PoisonError; + let in_set = |set: &OnceLock>>| { + set.get().is_some_and(|s| { + s.lock() + .unwrap_or_else(PoisonError::into_inner) + .contains(&addr) + }) + }; + [ + in_set(&EXTERNAL_BUFFER_REGISTRY), + in_set(&EXTERNAL_UINT8ARRAY_REGISTRY), + EXTERNAL_CRYPTO_KEY_META_REGISTRY.get().is_some_and(|m| { + m.lock() + .unwrap_or_else(PoisonError::into_inner) + .contains_key(&addr) + }), + ] +} + fn external_uint8arrays() -> &'static Mutex> { crate::once_init::get_or_init(&EXTERNAL_UINT8ARRAY_REGISTRY, || Mutex::new(HashSet::new())) } diff --git a/crates/perry-runtime/src/buffer/mod.rs b/crates/perry-runtime/src/buffer/mod.rs index 2b31fc1def1..beb460a384d 100644 --- a/crates/perry-runtime/src/buffer/mod.rs +++ b/crates/perry-runtime/src/buffer/mod.rs @@ -62,11 +62,12 @@ pub(crate) use header::note_buffer_like_registered; pub use header::{ asymmetric_key_meta, buffer_ab_alias, buffer_alloc, buffer_backing_array_buffer, buffer_byte_offset, buffer_data, buffer_data_mut, crypto_key_meta, ensure_buffer_ab_alias, - is_any_array_buffer, is_array_buffer, is_data_view, is_external_buffer, is_registered_buffer, - is_secret_key, is_shared_array_buffer, is_uint8array_buffer, js_set_crypto_key_death_hook, - mark_as_array_buffer, mark_as_asymmetric_key, mark_as_crypto_key, mark_as_data_view, - mark_as_secret_key, mark_as_shared_array_buffer, mark_as_uint8array, register_buffer, - resolve_buffer_ab_alias, set_buffer_ab_alias, CryptoKeyDeathHookFn, + external_registries_hold_for_test, is_any_array_buffer, is_array_buffer, is_data_view, + is_external_buffer, is_registered_buffer, is_secret_key, is_shared_array_buffer, + is_uint8array_buffer, js_set_crypto_key_death_hook, mark_as_array_buffer, + mark_as_asymmetric_key, mark_as_crypto_key, mark_as_data_view, mark_as_secret_key, + mark_as_shared_array_buffer, mark_as_uint8array, register_buffer, resolve_buffer_ab_alias, + set_buffer_ab_alias, CryptoKeyDeathHookFn, }; pub(crate) use header::{ buffer_alloc_foreign, collect_dead_registered_buffers_post_trace, diff --git a/crates/perry-stdlib/src/runtime_thread_exit_tests/symbols_tests.rs b/crates/perry-stdlib/src/runtime_thread_exit_tests/symbols_tests.rs index 10739032d62..39d70078bbd 100644 --- a/crates/perry-stdlib/src/runtime_thread_exit_tests/symbols_tests.rs +++ b/crates/perry-stdlib/src/runtime_thread_exit_tests/symbols_tests.rs @@ -227,46 +227,118 @@ fn thread_exit_releases_the_threads_buffer_own_props() { ); } +/// Membership of `addr` in the three process-global external-buffer +/// registries and `PERRY_U8_INLINE_CACHE`, in that order. Reads no +/// thread-local, so the thread-exit probe below may call it. +fn external_buffer_registrations(addr: usize) -> [bool; 4] { + let [ext, u8a, meta] = perry_runtime::buffer::external_registries_hold_for_test(addr); + [ext, u8a, meta, u8_cache_holds(addr)] +} + +/// #11547: the external-buffer test's buffer address, and what the tables held +/// for it when its thread's arena was released. +/// +/// Checking the tables after `join` cannot tell the dead buffer from a new one. +/// The dying thread's arena goes back to the allocator, and a concurrent test +/// (`thread_exit_releases_the_threads_crypto_key_entries` registers a CryptoKey +/// Buffer through the same `js_buffer_mark_as_crypto_key_external`) can be +/// given the same address and register it again. So the verdict is taken +/// INSIDE the release instead: [`record_external_buffer_release`] is a +/// thread-exit range hook that runs after the external-buffer registries' own +/// hook, while the block is still owned by the exiting thread and so cannot +/// belong to anyone else. +static EXTERNAL_BUFFER_EXIT_PROBE: std::sync::Mutex<(usize, Option<[bool; 4]>)> = + std::sync::Mutex::new((0, None)); + +fn record_external_buffer_release(freed: &perry_runtime::arena::thread_exit::FreedRanges) { + let mut probe = EXTERNAL_BUFFER_EXIT_PROBE + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let (addr, seen) = *probe; + // First release only: once the address is back with the allocator, a later + // tenant's own thread exit reports it again. + if addr != 0 && seen.is_none() && freed.contains(addr) { + probe.1 = Some(external_buffer_registrations(addr)); + } +} + +/// Verdict on what [`record_external_buffer_release`] saw: `Err` names the +/// first table that still held the dead buffer when its thread was released. +fn external_buffer_release_verdict(seen: Option<[bool; 4]>) -> Result<(), &'static str> { + const TABLES: [&str; 4] = [ + "EXTERNAL_BUFFER_REGISTRY outlived the thread", + "EXTERNAL_UINT8ARRAY_REGISTRY outlived the thread", + "EXTERNAL_CRYPTO_KEY_META_REGISTRY outlived the thread", + "PERRY_U8_INLINE_CACHE outlived the thread", + ]; + let seen = seen.ok_or("the thread's release never reported the buffer's range")?; + match seen.iter().position(|&held| held) { + Some(i) => Err(TABLES[i]), + None => Ok(()), + } +} + #[test] fn thread_exit_releases_the_threads_external_buffer_registrations() { - let (addr, alive) = std::thread::spawn(|| { + let alive = std::thread::spawn(|| { let scope = RuntimeHandleScope::new(); let buf = scope.root_raw_mut_ptr(perry_runtime::buffer::js_buffer_alloc(16, 0)); let addr = buf.get_raw_mut_ptr::() as usize; // webcrypto's CryptoKey registration: all three external registries. + // This also registers their thread-exit hook, so it runs before the + // probe registered below. unsafe { js_buffer_mark_as_crypto_key_external(addr, 1, 0, 1, 1, 0, 0) }; // The codegen inline-read slow arm primes PERRY_U8_INLINE_CACHE. unsafe { js_u8_buffer_read_f64(addr as *const u8, 0) }; - let cached = u8_cache_holds(addr); - let alive = [ - perry_runtime::buffer::is_external_buffer(addr), - perry_runtime::buffer::is_uint8array_buffer(addr), - perry_runtime::buffer::crypto_key_meta(addr).is_some(), - cached, - ]; - (addr, alive) + *EXTERNAL_BUFFER_EXIT_PROBE.lock().unwrap() = (addr, None); + perry_runtime::arena::thread_exit::register_thread_exit_range_hook( + record_external_buffer_release, + ); + external_buffer_registrations(addr) }) .join() .unwrap(); + let seen = std::mem::replace(&mut *EXTERNAL_BUFFER_EXIT_PROBE.lock().unwrap(), (0, None)).1; assert_eq!( alive, [true; 4], "every registration must exist while its thread lives" ); + if let Err(table) = external_buffer_release_verdict(seen) { + panic!("{table}"); + } +} + +/// #11547 regression: a buffer registered at the dead one's address after the +/// release must not fail the verdict, and the verdict must still fail when +/// the release left an entry behind or never saw the range. The newcomer is a +/// real registered buffer on this thread, which is exactly what the +/// address-only check used to report as "outlived". +#[test] +fn external_buffer_verdict_is_taken_at_release_not_by_address() { + let scope = RuntimeHandleScope::new(); + let buf = scope.root_raw_mut_ptr(perry_runtime::buffer::js_buffer_alloc(16, 0)); + let addr = buf.get_raw_mut_ptr::() as usize; + unsafe { js_buffer_mark_as_crypto_key_external(addr, 1, 0, 1, 1, 0, 0) }; assert!( - !perry_runtime::buffer::is_external_buffer(addr), - "EXTERNAL_BUFFER_REGISTRY outlived the thread" + perry_runtime::buffer::is_external_buffer(addr), + "the newcomer is registered, so an address-only check would fail" ); - assert!( - !perry_runtime::buffer::is_uint8array_buffer(addr), - "EXTERNAL_UINT8ARRAY_REGISTRY outlived the thread" + assert_eq!( + external_buffer_release_verdict(Some([false; 4])), + Ok(()), + "a newcomer at a released address was reported as the dead buffer" ); - assert!( - perry_runtime::buffer::crypto_key_meta(addr).is_none(), - "EXTERNAL_CRYPTO_KEY_META_REGISTRY outlived the thread" + assert_eq!( + external_buffer_release_verdict(Some([true, false, false, false])), + Err("EXTERNAL_BUFFER_REGISTRY outlived the thread") + ); + assert_eq!( + external_buffer_release_verdict(Some([false, false, false, true])), + Err("PERRY_U8_INLINE_CACHE outlived the thread") ); assert!( - !u8_cache_holds(addr), - "PERRY_U8_INLINE_CACHE outlived the thread" + external_buffer_release_verdict(None).is_err(), + "a release that never reported the range must not pass" ); } From ac0a2a9bb553a7c136bb2de2b76d8f45a8878b3e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 27 Sep 2026 18:46:33 +0200 Subject: [PATCH 2/2] changelog: fragment for #11555 --- changelog.d/11555-external-buffer-flake.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog.d/11555-external-buffer-flake.md diff --git a/changelog.d/11555-external-buffer-flake.md b/changelog.d/11555-external-buffer-flake.md new file mode 100644 index 00000000000..45b41286cd7 --- /dev/null +++ b/changelog.d/11555-external-buffer-flake.md @@ -0,0 +1,3 @@ +### Fixed + +- Fixed a flaky perry-stdlib test, `thread_exit_releases_the_threads_external_buffer_registrations` (#11547), which failed intermittently with "EXTERNAL_BUFFER_REGISTRY outlived the thread". The test checked the external-buffer registries for the dead thread's buffer address after `join`. By then a concurrently running test (`thread_exit_releases_the_threads_crypto_key_entries`) can be handed the same arena address and register it again. A sequenced diagnostic confirmed this: the release had removed the entry, and the sibling then got the identical address in 12 of 15 rounds. The release hook itself was fine. The test now records the registries' state from a thread-exit range hook that runs after their own release, while the block still belongs to the exiting thread. It reads them through the new thread-local-free probe `perry_runtime::buffer::external_registries_hold_for_test`. `external_buffer_verdict_is_taken_at_release_not_by_address` is a regression test that fails with the old address-only check. With the registry release removed, the fixed test fails every run.