Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

crates/vm/src/gc_state.rs (1)

711-735: ⚠️ Potential issue | 🔴 Critical

Resolve lock-order inversion between freeze and unfreeze (deadlock risk).

Line 712 locks permanent_list first, while Lines 733-734 lock generation_lists[2] then permanent_list. That opposing order can deadlock under concurrent calls.

🔧 Suggested lock-order fix for freeze

 pub fn freeze(&self) {
     let mut count = 0usize;
-    let mut perm = self.permanent_list.write();

     for (gen_idx, gen_list) in self.generation_lists.iter().enumerate() {
         let mut list = gen_list.write();
+        let mut perm = self.permanent_list.write();
         while let Some(ptr) = list.pop_front() {
             perm.push_front(ptr);
             unsafe { ptr.as_ref().set_gc_generation(GC_PERMANENT) };
             count += 1;
         }
         self.generations[gen_idx].count.store(0, Ordering::SeqCst);
     }

     self.permanent.count.fetch_add(count, Ordering::SeqCst);
 }

🤖 Prompt for AI Agents

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

In `@crates/vm/src/gc_state.rs` around lines 711 - 735, The freeze function
currently acquires self.permanent_list.write() before taking per-generation
locks, which inverts the lock order used by unfreeze (which takes
generation_lists[2].write() then permanent_list.write()) and can deadlock; fix
by reordering locks in freeze to always acquire generation_lists (e.g., iterate
and take each gen_list.write() or at least acquire generation_lists[2].write()
before permanent_list.write()) so the lock ordering matches unfreeze
(generation_lists -> permanent_list), and update the logic in freeze (and any
helper blocks) to move nodes into permanent_list after holding the generation
lock(s) but only take permanent_list.write() after those generation locks are
held to maintain consistent lock order between freeze and unfreeze.

crates/vm/src/gc_state.rs (1)

260-287: Extract shared untrack logic instead of duplicating generation/permanent branches.

The two branches repeat the same re-check/remove/clear flow; only target list/counter selection differs. Consolidating this reduces maintenance risk.

As per coding guidelines: "When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code".

🤖 Prompt for AI Agents

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

In `@crates/vm/src/gc_state.rs` around lines 260 - 287, Extract the duplicated
re-check/remove/clear sequence into a single helper and call it for both
generation and permanent cases: compute the target list and counter first (for
gen <= 2 set list = self.generation_lists[obj_gen as usize].write() and counter
= &self.generations[obj_gen as usize].count; for GC_PERMANENT set list =
self.permanent_list.write() and counter = &self.permanent.count), then perform
the re-check using obj_ref.gc_generation(), call unsafe { list.remove(obj) } and
if Some decrement counter.fetch_sub(1, Ordering::SeqCst), call
obj_ref.clear_gc_tracked() and obj_ref.set_gc_generation(GC_UNTRACKED), and
return; keep the outer loop/continue logic unchanged so the helper is retried
when the generation changed.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

crates/vm/src/gc_state.rs (1)

289-299: ⚠️ Potential issue | 🔴 Critical

Make raw-pointer finalization helpers unsafe (or take &PyObject).

is_finalized and mark_finalized are safe pub fns but dereference raw pointers internally (Line 290, Line 297). That makes UB reachable from safe callers passing an invalid NonNull<PyObject>.

🔧 Proposed fix

-    pub fn is_finalized(&self, obj: NonNull<PyObject>) -> bool {
+    /// # Safety
+    /// `obj` must point to a valid live `PyObject`.
+    pub unsafe fn is_finalized(&self, obj: NonNull<PyObject>) -> bool {
         let obj_ref = unsafe { obj.as_ref() };
         obj_ref.gc_finalized()
     }

-    pub fn mark_finalized(&self, obj: NonNull<PyObject>) {
+    /// # Safety
+    /// `obj` must point to a valid live `PyObject`.
+    pub unsafe fn mark_finalized(&self, obj: NonNull<PyObject>) {
         let obj_ref = unsafe { obj.as_ref() };
         obj_ref.set_gc_finalized();
     }

Run this to verify definition/call-site updates after the signature change:

#!/bin/bash
set -euo pipefail

# Confirm helper signatures
rg -nP --type=rust 'pub\s+(unsafe\s+)?fn\s+(is_finalized|mark_finalized)\s*\(' crates/vm/src/gc_state.rs -C2

# Find all call sites that must satisfy unsafe contract (or be ref-based after refactor)
rg -nP --type=rust '\.(is_finalized|mark_finalized)\s*\(' crates/vm/src -C2

🤖 Prompt for AI Agents

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

In `@crates/vm/src/gc_state.rs` around lines 289 - 299, The methods is_finalized
and mark_finalized currently dereference a raw NonNull<PyObject> inside safe
functions which is UB; change their signatures to either pub unsafe fn
is_finalized(&self, obj: NonNull<PyObject>) -> bool and pub unsafe fn
mark_finalized(&self, obj: NonNull<PyObject>) or accept a safe reference pub fn
is_finalized(&self, obj: &PyObject) / pub fn mark_finalized(&self, obj:
&PyObject), update all call sites accordingly to either call them within an
unsafe block or pass &PyObject, and run the ripgrep checks suggested to locate
and fix all call sites that must be adjusted.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro