Skip to content

ptr: add OwnedRef<T>, RefPtr::to_owned, and AnyRefCounted::ref_guard - #31173

Open
Jarred-Sumner wants to merge 3 commits into
mainfrom
claude/p0-ownedref-raii-refptr
Open

ptr: add OwnedRef<T>, RefPtr::to_owned, and AnyRefCounted::ref_guard#31173
Jarred-Sumner wants to merge 3 commits into
mainfrom
claude/p0-ownedref-raii-refptr

Conversation

@Jarred-Sumner

@Jarred-Sumner Jarred-Sumner commented May 21, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

RefPtr<T> deliberately has no Drop impl — dropping one leaks the strong ref, so every consumer balances ref_()/deref() by hand. This PR adds the owning RAII form so those call sites can be migrated type-by-type in follow-ups. No callers are migrated here, and RefPtr itself is unchanged (adding Drop to it would double-free every existing manually-balanced site).

New API in src/ptr/ref_count.rs:

  • OwnedRef<T> — holds exactly one strong ref and releases it on Drop (every exit path: early return, ?, unwind). Deref to &T, Clone bumps the count, into_raw/from_raw for FFI hand-offs, new/acquire constructors. Double-release is impossible by construction: the only way to give up the ref without releasing it is into_raw, which consumes the handle.
  • RefPtr::to_owned(&self) -> OwnedRef<T> — safe bridge from a manually-balanced RefPtr to the RAII handle (takes an additional ref; the RefPtr's own ref is still owed).
  • AnyRefCounted::ref_guard(&self) -> ScopedRef<Self> — RAII replacement for the hand-paired this.ref_(); … defer this.deref(); keep-alive bracket. This one is an unsafe fn: ScopedRef carries no lifetime, so the compiler cannot stop the guard from outliving a stack-backed *self, and the storage-governance precondition ("freed only by the count-zero destructor") goes under # Safety. It still collapses the N per-exit-path deref() calls into one audited acquisition.
  • The RefPtr doc comment now points new code at OwnedRef.

Unsafe count (src/ptr/ref_count.rs)

Before: 89 → after: 129. The increase breaks down as 23 in the new #[cfg(test)] module (test-type trait impls that mirror the derive output, plus the raw-pointer round-trip and ref_guard tests) and the rest inside the new abstractions' bodies, each with a SAFETY: comment. This project adds the safe surface; the net reduction lands when the ~700 hand-balanced call sites migrate onto it in follow-up PRs.

Cost

Zero-cost: OwnedRef is the same NonNull<T> (+ debug-only tracking id) as RefPtr; Drop/Clone compile to the same deref()/ref_() calls the manual sites already make.

Sites that do not fully meet the "no safe-call UB" bar

  • ref_guard is kept unsafe fn rather than safe: the requirement that *self's storage is governed by its embedded refcount is a precondition the compiler cannot see (a stack-allocated T: RefCounted is constructible in safe code, and ScopedRef carries no lifetime to stop the guard outliving it), so per the project rules it stays unsafe with the precondition under # Safety.
  • ref_guard's pointer is derived from &self; if the guard holds the final ref, the destructor reclaims the allocation through that shared-provenance pointer — the same posture as the existing as_ctx_ptr()deref() release sites, which Tree Borrows rejects. Documented under # Safety; the test for that path runs under the normal harness but is #[cfg_attr(miri, ignore)]d. The bracket use case (an outer owner survives the scope) is fully Miri-clean.

OwnedRef and RefPtr::to_owned meet the bar: every safe path acquires and releases exactly one ref, and the only safe constructor (new) heap-allocates.

Other notes

  • The scoped_log! diagnostics in ref_()/deref() are compiled out of bun_ptr's own unit-test target only (#[cfg(not(test))]): they reach the OutputSink link interface whose Sys arm lives in bun_sys, which the leaf-crate test binary cannot link. Downstream builds are unaffected.
  • The ref-count debug tracker skips its libc::backtrace capture under Miri (unsupported foreign function); it records an empty trace instead.
  • OwnedRef is intentionally not added to lib.rs's flat re-export list — this PR confines itself to ref_count.rs so the follow-up migration PRs don't conflict with it. It is reachable as bun_ptr::ref_count::OwnedRef.

How did you verify your code works?

  • cargo test -p bun_ptr — 12/12 pass.
  • bun run rust:miri -p bun_ptr (-Zmiri-tree-borrows) — 9 pass, 3 ignored with documented reasons (two need ThreadLock's libc::backtrace, one exercises the &self-derived deallocation path described above). The Miri-covered set includes the full OwnedRef lifecycle for the thread-safe flavor: construct/drop balance, clone balance, into_raw/from_raw round-trip, to_owned, ref_guard, and destructor-runs-exactly-once assertions.
  • cargo clippy -p bun_ptr --all-targets — clean.
  • bun bd — full debug build compiles.

RefPtr<T> deliberately has no Drop impl, so every consumer balances
ref_()/deref() by hand. Add the owning RAII form so call sites can be
migrated type-by-type:

- OwnedRef<T>: holds exactly one strong ref, releases it on Drop (every
  exit path), Deref to &T, Clone bumps the count, into_raw/from_raw for
  FFI hand-offs. Double-release is impossible by construction: the only
  way to give up the ref without releasing it is into_raw, which
  consumes the handle.
- RefPtr::to_owned(&self) -> OwnedRef<T>: safe bridge from a
  manually-balanced RefPtr to the RAII handle.
- AnyRefCounted::ref_guard(&self) -> ScopedRef<Self>: safe RAII
  replacement for the hand-paired ref_()/defer-deref() keep-alive
  bracket.

No callers are migrated and RefPtr itself is unchanged (adding Drop to
it would double-free every existing manually-balanced site).

Tests cover construct/drop balance, clone balance, into_raw/from_raw
round-trips, and that the count reaches zero exactly once across every
path, for both refcount flavors. cargo miri test -p bun_ptr passes with
-Zmiri-tree-borrows; the ref-count debug tracker now skips its
libc::backtrace capture under Miri, and the scoped_log calls are
compiled out of bun_ptr's own unit-test target (the test binary cannot
link the OutputSink interface they reach).
@robobun

robobun commented May 21, 2026

Copy link
Copy Markdown
Collaborator
Updated 5:29 AM PT - May 21st, 2026

@Jarred-Sumner, your commit cf4f74a has 1 failures in Build #56614 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 31173

That installs a local version of the PR into your bun-31173 executable, so you can run:

bun-31173 --bun

@coderabbitai

coderabbitai Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

This PR adds RAII safety to the intrusive reference-counting system. It introduces OwnedRef<T>, a new scoped handle that automatically releases references on drop, alongside ref_guard() for bracket-delimited ref operations. RefPtr::to_owned() converts manual ref handles to RAII, while test-compatibility gates and Miri support ensure the changes integrate with the test infrastructure.

Changes

RAII Reference Counting

Layer / File(s) Summary
RAII ref guard contract
src/ptr/ref_count.rs
AnyRefCounted trait adds ref_guard(&self) -> ScopedRef<Self> to bracket ref increments with automatic RAII-driven release.
OwnedRef type and RefPtr integration
src/ptr/ref_count.rs
OwnedRef<T> is a new public RAII handle storing a strong ref that releases on drop. It provides new, acquire, from_raw, into_raw, and implements Clone, Deref, and Drop. RefPtr::to_owned() converts a manual ref into an OwnedRef, and documentation directs new code to prefer OwnedRef for automatic ref management.
Test compatibility and debug support
src/ptr/ref_count.rs
Logging in RefCount and ThreadSafeRefCount methods (ref_, deref, deref_with_context, release) is gated with #[cfg(not(test))] to prevent test binary from requiring the logging sink interface. Debug tracking captures empty StoredTrace under Miri to avoid backtrace incompatibility.
Comprehensive test coverage
src/ptr/ref_count.rs
New #[cfg(test)] module validates OwnedRef destruction, cloning, ref balancing, from_raw/into_raw round-trips, RefPtr::to_owned independence and outliving, and ref_guard bracketing, with Miri-incompatible tests marked to ignore.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and specifically summarizes the main changes: adding OwnedRef, RefPtr::to_owned, and AnyRefCounted::ref_guard to the ptr module.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The pull request description is comprehensive and well-structured, covering all required sections with detailed explanations of changes, verification methods, and design rationale.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/ptr/ref_count.rs`:
- Around line 1078-1082: OwnedRef::new currently skips the debug invariant check
that the embedded refcount is +1; mirror RefPtr::new by performing the check
after constructing the OwnedRef. Change new to create the OwnedRef from the
Box::into_raw as it does now, then in debug builds call rc_has_one_ref() on the
resulting OwnedRef (or the appropriate RefPtr/OwnedRef accessor) with a
debug_assert! (or similar) to ensure the refcount is 1 before returning; this
makes OwnedRef::new fail fast if a type forgot to initialize its embedded
refcount.
- Around line 172-178: Replace the provenance-unsafe cast in
AnyRefCounted::ref_guard by obtaining a pointer that preserves allocation
provenance (e.g., use the original heap/allocation pointer or the same pointer
origin used by ScopedRef::new) instead of core::ptr::from_ref(self).cast_mut(),
so ScopedRef::drop’s final rc_deref/destroy won’t violate Stacked Borrows;
locate ref_guard and change it to create the ScopedRef from the
allocation-originating *mut Self the refcounting system already uses. Also add
the missing debug invariant check in OwnedRef::new: call T::rc_has_one_ref (the
same check RefPtr::adopt_ref runs) under cfg!(debug_assertions) before
constructing the OwnedRef to assert the embedded refcount starts at 1. Ensure
you reference ScopedRef::new/ScopedRef::drop, AnyRefCounted::ref_guard,
OwnedRef::new, RefPtr::adopt_ref, CellRefCounted::deref, and T::rc_has_one_ref
when making these changes.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: a3d33095-41ee-4d0c-a3fa-52e0cf7dc587

📥 Commits

Reviewing files that changed from the base of the PR and between 0b20408 and d8321eb.

📒 Files selected for processing (1)
  • src/ptr/ref_count.rs

Comment thread src/ptr/ref_count.rs Outdated
Comment thread src/ptr/ref_count.rs
…venance

Mirror RefPtr::adopt_ref's debug check that a freshly constructed object
starts at exactly one ref, and surface the last-ref provenance caveat in
ref_guard's doc comment instead of only in the PR description.
Comment thread src/ptr/ref_count.rs
Comment thread src/ptr/ref_count.rs
ScopedRef<T> carries no lifetime, so a safe ref_guard(&self) would let
the guard outlive a stack-allocated T and dereference freed memory from
safe code. The storage-governance precondition (the allocation is freed
only by the count-zero destructor) cannot be expressed in the type
system, so the function is unsafe with the precondition documented under
'# Safety', alongside the existing provenance note.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cf4f74a fully addresses my last note — making ref_guard an unsafe fn with the storage-governance contract spelled out is the right call, and the SAFETY comment now correctly defers to it. Nothing further from me; leaving for a human since this is new Drop-bearing refcount infrastructure with documented Tree Borrows caveats.

Extended reasoning...

Overview

Single-file change to src/ptr/ref_count.rs (~470 net lines) adding:

  • OwnedRef<T> — RAII handle over the existing intrusive refcount (Drop releases, Clone bumps, into_raw/from_raw for FFI), with Deref, debug tracking, and a #[must_use].
  • RefPtr::to_owned() — bridge from the manually-balanced handle to the RAII one.
  • AnyRefCounted::ref_guard() — RAII keep-alive bracket; now unsafe fn as of cf4f74a.
  • #[cfg(not(test))] gates on scoped_log! calls so the leaf-crate test binary links, a Miri shim for StoredTrace::capture, and a 12-test #[cfg(test)] module covering both refcount flavors under cargo test + Miri.

No callers are migrated; RefPtr itself is unchanged.

Security risks

None in the auth/crypto/injection sense. The relevant risk class is memory safety (UAF / double-free / leak) since this introduces a Drop impl that calls rc_deref on a stored raw pointer. The implementation looks correct: into_raw uses ManuallyDrop to suppress the release, Clone goes through acquire which bumps before wrapping, new asserts rc_has_one_ref, and every unsafe block carries a SAFETY comment that I checked against the callee's documented contract.

Level of scrutiny

High. This is foundational lifetime-management infrastructure that the PR positions as the migration target for ~700 hand-balanced sites. It adds 35 unsafe blocks (15 in the abstractions, 20 in tests), a Drop impl on a raw-pointer wrapper, and carries an explicitly documented provenance posture that Tree Borrows rejects (the &self-derived final-deref path, #[cfg_attr(miri, ignore)]'d). The design choice to introduce OwnedRef alongside RefPtr (rather than retrofitting Drop) is well-reasoned in the description but is the kind of API-shape decision a maintainer should sign off on.

Other factors

All three rounds of bot feedback have been addressed:

  • coderabbit's rc_has_one_ref assert + provenance doc → 7b7fe7f
  • my re-export note → author explained it's deliberately deferred to the first consumer PR (resolved)
  • my note that safe ref_guard(&self) didn't discharge ScopedRef::new's storage-lifetime precondition → cf4f74a made it unsafe fn with an explicit # Safety contract and updated the body SAFETY comment and all call sites accordingly

The bug-hunting system found nothing this round. Tests pass per the description (cargo test 12/12, Miri 9 pass / 3 documented-ignore, clippy clean, full debug build). I'm not approving solely because of scope and criticality, not because of any outstanding concern with the code.

robobun added a commit that referenced this pull request Aug 12, 2026
Same idea and API as the OwnedRef in #31173 (new, acquire, from_raw,
into_raw), built on the raw count so it can be Send and Sync for
thread-safe hosts, plus RefPtr::into_owned and ThisPtr::owned_ref.

Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Cross-reference: #37665 re-adds this type on current main with the same name and constructor vocabulary (new / acquire / from_raw / into_raw; you are credited as co-author there), built on the raw count so it can be Send for thread-safe hosts, because three review follow-ups (#37618, #37591, #37594) need an owning handle now. If you would rather land this PR instead, say so and I will restack those on it; they only use the surface the two versions share.

robobun added a commit that referenced this pull request Aug 12, 2026
Same idea and API as the OwnedRef in #31173 (new, acquire, from_raw,
into_raw), built on the raw count so it can be Send and Sync for
thread-safe hosts, plus RefPtr::into_owned and ThisPtr::owned_ref.

RefPtr's debug holder tracking now takes &self and keeps its tables
behind its lock, so a RefPtr on one thread and OwnedRefs on others no
longer form a &mut to a thread-safe count that other threads are using.

Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants