crates/vm/src/stdlib/thread.rs (1)

1210-1247: ⚠️ Potential issue | 🔴 Critical

Keep _shutdown() off the public join() validation path.

join_internal() still runs check_started() and the finalization guard even when _shutdown() is the caller. That means shutdown can legitimately hit Starting / NotStarted handles from the registry, or trip the finalizing check, and then _shutdown() logs the exception and returns early instead of waiting for the remaining non-daemon threads. The public checks need to stay in join(), or join_internal() needs an internal mode that skips them for shutdown/fork cleanup callers.

Possible direction

-        fn join_internal(
+        fn join_internal(
             inner: &Arc<parking_lot::Mutex<ThreadHandleInner>>,
             done_event: &Arc<(parking_lot::Mutex<bool>, parking_lot::Condvar)>,
             timeout_duration: Option<Duration>,
+            public_checks: bool,
             vm: &VirtualMachine,
         ) -> PyResult<()> {
-            Self::check_started(inner, vm)?;
+            if public_checks {
+                Self::check_started(inner, vm)?;
+            }

             let deadline =
                 timeout_duration.and_then(|timeout| std::time::Instant::now().checked_add(timeout));

             let (lock, cvar) = &**done_event;
             let mut done = lock.lock();

-            if !*done {
+            if public_checks && !*done {
                 let inner_guard = inner.lock();
                 let current_ident = get_ident();
                 if inner_guard.ident == current_ident
                     && inner_guard.state == ThreadHandleState::Running
                 {
                     return Err(vm.new_runtime_error("Cannot join current thread"));
                 }
                 if vm.state.finalizing.load(core::sync::atomic::Ordering::Acquire) {
                     return Err(...);
                 }
             }

-            Self::join_internal(&self.inner, &self.done_event, timeout_duration, vm)
+            Self::join_internal(&self.inner, &self.done_event, timeout_duration, true, vm)

-                    if let Err(exc) = ThreadHandle::join_internal(&inner, &done_event, None, vm) {
+                    if let Err(exc) =
+                        ThreadHandle::join_internal(&inner, &done_event, None, false, vm)
+                    {

Also applies to: 1314-1325

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/stdlib/thread.rs` around lines 1210 - 1247, The join_internal()
path runs public validations (check_started() and the vm.state.finalizing guard)
even when invoked from internal shutdown/fork cleanup, so change the call
boundary: move the public checks back into the public join() method and/or add
an internal-only mode to join_internal() (e.g., add a bool flag like
skip_public_checks or is_shutdown_call to join_internal(inner, done_event,
timeout_duration, vm, skip_public_checks: bool)), have join() call
join_internal(..., false) and _shutdown()/fork cleanup call join_internal(...,
true), and inside join_internal() skip check_started() and the finalizing check
when skip_public_checks is true; update all callers (including _shutdown()) to
use the new flag and keep ThreadHandleState/ThreadHandle_join semantics intact.

crates/vm/src/stdlib/thread.rs (1)

1211-1249: ⚠️ Potential issue | 🔴 Critical

Keep shutdown waits off the public join() validation path.

join_internal() still rejects Starting handles at Line 1217 and still raises PythonFinalizationError at Lines 1237-1248. _shutdown() legitimately hits both states while draining non-daemon threads, so this helper can still abort shutdown early and leave threads unjoined. Move those checks into the public join() wrapper, or add an internal for_shutdown mode that skips them.

Refactor sketch

-        fn join_internal(
+        fn join_internal(
             inner: &Arc<parking_lot::Mutex<ThreadHandleInner>>,
             done_event: &Arc<(parking_lot::Mutex<bool>, parking_lot::Condvar)>,
             timeout_duration: Option<Duration>,
+            for_shutdown: bool,
             vm: &VirtualMachine,
         ) -> PyResult<()> {
-            Self::check_started(inner, vm)?;
+            if !for_shutdown {
+                Self::check_started(inner, vm)?;
+            }

             let deadline =
                 timeout_duration.and_then(|timeout| std::time::Instant::now().checked_add(timeout));

             let (lock, cvar) = &**done_event;
             let mut done = lock.lock();

-            if !*done {
+            if !*done && !for_shutdown {
                 let inner_guard = inner.lock();
                 let current_ident = get_ident();
                 if inner_guard.ident == current_ident
                     && inner_guard.state == ThreadHandleState::Running
                 {
@@
-            Self::join_internal(&self.inner, &self.done_event, timeout_duration, vm)
+            Self::check_started(&self.inner, vm)?;
+            Self::join_internal(&self.inner, &self.done_event, timeout_duration, false, vm)

-                    if let Err(exc) = ThreadHandle::join_internal(&inner, &done_event, None, vm) {
+                    if let Err(exc) =
+                        ThreadHandle::join_internal(&inner, &done_event, None, true, vm)
+                    {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/stdlib/thread.rs` around lines 1211 - 1249, join_internal
currently enforces "Starting" and interpreter-finalizing checks (via
Self::check_started and the PythonFinalizationError branch) which causes
_shutdown to abort while draining threads; modify join_internal to accept a
for_shutdown: bool parameter (or remove those checks here) and move the
Starting-state verification and the vm.state.finalizing check into the public
join() wrapper so normal joins still validate but calls from _shutdown can call
join_internal(..., for_shutdown=true) (or call a new internal no-validate
helper) to skip those two checks; update all callers (public join() and
_shutdown) to call the appropriate variant and ensure unique symbols referenced:
join_internal, check_started, ThreadHandle_join semantic check, and _shutdown
are adjusted accordingly.

crates/vm/src/vm/mod.rs (2)

1932-1937: ⚠️ Potential issue | 🔴 Critical

Use Acquire for the finalization gate in eval_breaker_tripped().

Line 1935 still does a Relaxed read even though this branch decides whether check_signals() runs at all. A stale false here skips the later Acquire load in check_signals(), so non-main threads can miss finalization on weak memory models.

Proposed fix

         #[cfg(feature = "threading")]
-        if self.state.finalizing.load(Ordering::Relaxed) && !self.is_main_thread() {
+        if self.state.finalizing.load(Ordering::Acquire) && !self.is_main_thread() {
             return true;
         }

#!/bin/bash
set -euo pipefail

printf '\n== eval_breaker_tripped ==\n'
sed -n '1932,1958p' crates/vm/src/vm/mod.rs

printf '\n== bytecode-loop gate ==\n'
sed -n '1198,1206p' crates/vm/src/frame.rs

printf '\n== finalizing loads/stores ==\n'
rg -n -C2 'finalizing\.(load|store)\(' crates/vm/src

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/vm/mod.rs` around lines 1932 - 1937, The read of the
finalization gate in eval_breaker_tripped() uses Ordering::Relaxed which can let
a stale false skip check_signals() on weak memory models; change the load to use
Ordering::Acquire (i.e., replace state.finalizing.load(Ordering::Relaxed) with
state.finalizing.load(Ordering::Acquire)) so that the subsequent
check_signals()'s synchronization is not bypassed, leaving all other logic in
eval_breaker_tripped() and callers like check_signals() unchanged.

---

332-339: ⚠️ Potential issue | 🟡 Minor

Reset stats_last_wait_ns on the zero-thread fast path.

When Line 332 returns immediately, stats_snapshot() keeps reporting the previous stop's wait time even though this stop waited 0ns. Clear stats_last_wait_ns before returning.

Proposed fix

         if initial_countdown == 0 {
+            self.stats_last_wait_ns.store(0, Ordering::Relaxed);
             self.world_stopped.store(true, Ordering::Release);
             #[cfg(debug_assertions)]
             self.debug_assert_all_non_requester_suspended(vm);
             stw_trace(format_args!(
                 "stop end requester={requester_ident} wait_ns=0 polls=0"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/vm/mod.rs` around lines 332 - 339, The zero-thread fast path
returns without resetting the last-stop duration, so stats_snapshot() reports
stale wait time; in the initial_countdown == 0 branch reset
self.stats_last_wait_ns to zero (use the same atomic store pattern and Ordering
used elsewhere in this module) before the stw_trace/return (after
debug_assert_all_non_requester_suspended(vm) if present) so the reported wait_ns
is correctly 0 for this stop.

crates/vm/src/stdlib/posix.rs (1)

904-923: macOS: Calling task_for_pid on self is unnecessary overhead.

Using task_for_pid(mach_task_self(), getpid(), &mut proc_task) is redundant since mach_task_self() already returns the task port for the current process. Additionally, task_for_pid typically requires special entitlements (taskgated) when used for other processes, and while it works for self, it's an extra syscall.

♻️ Suggested simplification

-            let self_task = unsafe { mach_task_self() };
-            let mut proc_task: MachPortT = 0;
-            if unsafe { task_for_pid(self_task, libc::getpid(), &mut proc_task) } == KERN_SUCCESS {
-                let mut threads: ThreadActArrayT = core::ptr::null_mut();
-                let mut n_threads: MachMsgTypeNumberT = 0;
-                if unsafe { task_threads(proc_task, &mut threads, &mut n_threads) } == KERN_SUCCESS
+            let self_task = unsafe { mach_task_self() };
+            let mut threads: ThreadActArrayT = core::ptr::null_mut();
+            let mut n_threads: MachMsgTypeNumberT = 0;
+            if unsafe { task_threads(self_task, &mut threads, &mut n_threads) } == KERN_SUCCESS
+            {
+                if !threads.is_null() {
+                    let _ = unsafe {
+                        vm_deallocate(
+                            self_task,
+                            threads as libc::uintptr_t,
+                            (n_threads as usize * core::mem::size_of::<MachPortT>())
+                                as libc::uintptr_t,
+                        )
+                    };
+                }
+                return n_threads as isize;
+            }
+            0

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/stdlib/posix.rs` around lines 904 - 923, The code redundantly
calls task_for_pid(self_task, getpid(), &mut proc_task); replace that flow by
assigning proc_task = self_task (from mach_task_self()) and then call
task_threads(proc_task, &mut threads, &mut n_threads) as before; keep the
existing vm_deallocate call using self_task and threads, and return n_threads as
isize, removing the task_for_pid branch and its associated conditional while
preserving error handling around task_threads and vm_deallocate (symbols:
mach_task_self, proc_task, task_for_pid, task_threads, vm_deallocate,
n_threads).