Conversation
…roys RefPtr<T> is Send + Sync when T is, and Drop releases the ref. No unit test exercised the two together. This one clones a RefPtr<Shared> into four threads, drops each clone there, then drops the last ref on yet another thread and checks that the destructor ran exactly once. It runs under Miri with the rest of the crate (bun run rust:miri -p bun_ptr).
|
Warning Review limit reached
On-demand reviews are free for the next 25 days. After that, they cost $0.25 per reviewed file. Or wait 2 minutes for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Status: test-only PR plus a one-line crate self-alias, verified locally.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only addition that mirrors the neighboring thread_safe_ref_count_cross_thread_destroy structure and exercises the RefPtr<T>: Send + Sync bounds directly.
What was reviewed:
- Hand-rolled
AnyRefCountedimpl forSharedmatches the trait's four methods and delegates toThreadSafeRefCountas the derive would. unsafe impl Send/Sync for Sharedis sound: atomic count,Box<u32>payload only read across threads; SAFETY comments present on everyunsafe.- Test uses
serial()/drops()snapshot pattern correctly,.collect()s clones before joining so thehas_one_ref()assertion is deterministic, and.join().unwrap()is on a test-invariant path.
Extended reasoning...
Overview
This PR adds a single unit test plus supporting trait impls to the #[cfg(test)] module of src/ptr/ref_count.rs. The new test ref_ptr_clones_cross_threads_and_the_last_one_destroys clones a RefPtr<Shared> into four spawned threads, joins them, verifies no destruction has happened and the main ref is now sole owner, then moves the last ref to another thread to drop and asserts exactly one destruction. To make RefPtr<Shared>: Send + Sync hold, it hand-writes the AnyRefCounted impl (the derive expands to ::bun_ptr:: paths that don't resolve inside the defining crate) and adds unsafe impl Send/Sync for Shared. No runtime code is touched.
Security risks
None. The change is entirely inside a #[cfg(test)] module in a Rust crate that is exercised under Miri, not linked into the shipped binary. The unsafe impl Send/Sync are on a test-local type whose fields (ThreadSafeRefCount<Shared> + Box<u32>) are genuinely thread-safe for the read-only access pattern used, and the SAFETY comments accurately state the justification.
Level of scrutiny
Low. This is a small, self-contained test addition that copies the exact shape of the adjacent thread_safe_ref_count_cross_thread_destroy test (four threads, payload 5, serial() guard + drops() snapshot, std::thread::spawn, .join().unwrap()). The AnyRefCounted impl is a mechanical spelling-out of what the derive emits, and each unsafe block/impl carries a SAFETY comment as REVIEW.md requires. The .unwrap() calls are on JoinHandle::join in a test — a provable-invariant use per src/CLAUDE.md's .unwrap() rule.
Other factors
No CODEOWNERS entry covers src/ptr/ref_count.rs. The PR timeline shows no outstanding third-party objections. The bug hunt exited on dry_streak with no findings. The test is deterministic: all clones are taken and .collect()ed before any join begins, and every worker is joined before the has_one_ref() assertion, so there is no ordering flakiness. The serial() mutex prevents interference with sibling tests that share the DROPS counter. Given the change is test-only, follows local conventions precisely, and fills an explicitly-noted coverage gap for the RefPtr<T>: Send + Sync blanket impls, approval is appropriate.
…n the count's ordering extern crate self as bun_ptr lets the ref-count derives resolve inside the crate, so the test host Shared uses #[derive(ThreadSafeRefCounted)] instead of a hand-written copy of the derive output. The test now clones through a shared &RefPtr from four scoped threads, then releases all five refs on their own threads with no join in between. The destructor is only ordered after the other threads' reads by the refcount's atomics. A Relaxed fetch_sub in ThreadSafeRefCount::deref makes Miri report a data race in this test. The previous shape joined every worker before the last release, so join() supplied that ordering and a Relaxed count passed.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
The bun_ptr test binary does not link natively, so the crate's unit tests run under Miri only. This wrapper runs the cross-thread RefPtr test with the same Tree Borrows flags as bun run rust:miri and checks that it ran and passed. Skipped where miri is not installed or the cargo workspace is not resolvable, like linear-fifo.test.ts.
There was a problem hiding this comment.
LGTM — test-only additions, no runtime code touched.
Checked that swapping the hand-written impl ThreadSafeRefCounted for the derive is equivalent (the extern crate self as bun_ptr alias is what lets the derive's ::bun_ptr:: paths resolve in-crate; same idiom already used in bun_jsc/bun_install/bun_css). The new cross-thread test correctly races five drops with no join between them so only the refcount's release/acquire orders the destructor after the payload reads — the shape Miri needs to catch a weakened ordering. The unsafe impl Send/Sync for Shared is confined to #[cfg(test)] and justified (atomic count, read-only Box<u32> payload). The Miri wrapper test is a near-verbatim copy of the established test/internal/linear-fifo.test.ts pattern, including the skip gate, pipe draining, and 120s timeout, and it asserts the specific ... ok line so a filter miss (0 tests) fails rather than passing silently.
Extended reasoning...
Overview
Three files: src/ptr/lib.rs gains a one-line extern crate self as bun_ptr; so the ref-count derive macros' emitted ::bun_ptr:: paths resolve inside the crate's own #[cfg(test)] module. src/ptr/ref_count.rs changes only the test module — the Shared test host swaps a hand-written ThreadSafeRefCounted impl for #[derive(crate::ThreadSafeRefCounted)], gains unsafe impl Send + Sync, and a new unit test exercises RefPtr<T>: Send + Sync by cloning through &RefPtr on scoped threads and then dropping all five refs on concurrent unjoined threads. test/internal/rust-ref-ptr-miri.test.ts runs that one unit test under cargo miri test with Tree Borrows so Miri's data-race detector validates the refcount's release/acquire ordering.
Security risks
None. No user-facing API surface, no parsing, no I/O beyond spawning cargo miri in a test that skips when the toolchain or vendored workspace is absent. The unsafe impl Send/Sync is on a #[cfg(test)]-only struct whose fields (ThreadSafeRefCount + Box<u32>) are genuinely thread-safe for the read-only usage the test performs; the SAFETY comments state this accurately.
Level of scrutiny
Low. Every change is either inside #[cfg(test)] or is the self-alias line (a compile-time-only name resolution aid with well-established precedent in this repo). No production code paths are altered. The exit reason was dry_streak, so the bug hunt ran to completion without findings.
Other factors
Since the earlier review on the first commit, four follow-up commits landed: the test was reworked so the concurrent releases are not joined before the destructor assertion (making the test actually depend on the refcount's ordering rather than the join's happens-before — the PR description confirms a Relaxed fetch_sub now trips Miri), comments were shortened per bot feedback, and the Miri wrapper test was added. The wrapper is a near-verbatim clone of the accepted test/internal/linear-fifo.test.ts (same skip gate, same env/flags approach, same 120s timeout, same concurrent pipe drain and stderr-before-exitCode ordering), and it additionally asserts the specific ... ok line so a filter miss fails loudly. The github-actions inline threads were self-resolved by the author, but the intervening commit messages (derive the host, shorten comments, one-line comment) plausibly correspond to and address them.
This branch has not been deployed
1 participant
Problem
RefPtr<T>owning:Dropreleases,Clonetakes a ref, andRefPtr<T>isSend + SyncwhenTis (src/ptr/ref_count.rs:586-599). No test moved or shared aRefPtracross threads.ThreadSafeRefCount::ref_/derefby hand through aSendPtrnewtype. It never constructs aRefPtr.Fix
ref_ptr_clones_cross_threads_and_the_last_one_destroys. Four scoped threads clone through a shared&RefPtr<Shared>(Sync) and hand the clone back (Send). All five refs are then released on their own threads with no join in between, so only the count's atomics order the destructor after the other threads' reads. ARelaxedfetch_subinThreadSafeRefCount::derefmakes Miri report a data race. A shape that joins the workers first passes withRelaxed.extern crate self as bun_ptr;tosrc/ptr/lib.rs, asbun_jsc,bun_install, andbun_cssdo. The test hostSharednow uses#[derive(ThreadSafeRefCounted)]instead of a hand-written copy of the derive output.test/internal/rust-ref-ptr-miri.test.ts. It runs that unit test under Miri frombun test, with the flagsbun run rust:miriuses, and checks that it ran and passed. It skips where miri or the cargo workspace is missing, likelinear-fifo.test.ts.bun run rust:miri -p bun_ptr(23 pass). The wrapper test passes, and fails with main'ssrc/ptrfiles.cargo clippy -p bun_ptris clean. No runtime code changes.Background
RefPtr<T>is the intrusiveArc<T>: the count lives insideT.bun_ptrunit tests run under Miri only: the native test binary does not link theOutputSinksymbols. Miri tracks happens-before with vector clocks, so a racing read and free is reported in any schedule.std::thread::spawnis what this test module already uses.bun_threadingis not a dependency ofbun_ptr.Notes
OwnedRef<T>type before bun_ptr: RefPtr releases on Drop; remove ScopedRef/IntrusiveRc/DestructorCtx #40478 gave it toRefPtritself. bun_ptr: add OwnedRef, an intrusive ref that is released on drop #37665 and ptr: add OwnedRef<T>, RefPtr::to_owned, and AnyRefCounted::ref_guard #31173 are closed as superseded.Thing,Light) keep their hand-written trait impls. With the self-alias in place they can move to#[derive(RefCounted)]and#[derive(CellRefCounted)]in a follow-up, which would also makeRefPtr<Light>constructible in tests.sed -i '352s/Ordering::SeqCst/Ordering::Relaxed/' src/ptr/ref_count.rs, thencargo miri test -p bun_ptr ref_ptr_clones_cross_threadsfails withData race detected between (1) non-atomic read on thread unnamed-6 and (2) non-atomic write on thread unnamed-10. Reverted before commit.git checkout origin/main -- src/ptr/lib.rs src/ptr/ref_count.rs, then the wrapper test fails onexpect(stdout).toContain(... ... ok)withrunning 0 testsin the output.[review] gate passed · iteration 1 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file