test(stdlib): take the external-buffer thread-exit verdict at release, not by address (#11547)

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.

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<Mutex<HashSet<usize>>>| {
+        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<HashSet<usize>> {
     crate::once_init::get_or_init(&EXTERNAL_UINT8ARRAY_REGISTRY, || Mutex::new(HashSet::new()))
 }

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,

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::<u8>() 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::<u8>() 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"
     );
 }