You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
threading: hold Semaphore's permit count in a Guarded<usize> - #37519
Unsafe-removal and lint-ratchet maintenance, no reachable defect. Merged #46363 has this shape. #37508, #37596 and #39165 were closed as refactors. A maintainer decides whether this is wanted.
Behaviour change: none
Problem
Semaphore keeps its permit count in an UnsafeCell<usize> beside a Mutex. One unsafe impl Sync and four unsafe blocks assert that the mutex protects it.
permits is a Guarded<usize>. wait() and post() hold its guard, and wait() blocks in Condition::wait_guarded. The file has no unsafe left. Its line leaves mordant-baseline.toml.
The calls and their order are the same. Guarded::lock() is Mutex::lock(), its guard unlocks on drop, and wait_guarded is Condition::wait on that mutex.
Verified: cargo test -p bun_threading (10 pass, 3 new), Miri, bun scripts/rust-mordant.ts (exit 0), workspace clippy and check.
Self-reviewed: 24 concerns raised, 19 addressed, 5 in part (see Notes).
Background
Guarded<T> (src/threading/guarded.rs) is a mutex that owns its value. src/sema/program.rs has this shape: asleep: Guarded<usize>.
An AtomicUsize count also removes the unsafe, but the type then hides that the mutex guards it.
Downsides
Field offsets change. On Linux x64 an uncontended wait() runs 26 instructions, up from 22, and post() still runs 23. The linked release binary is not measured.
The three unit tests are 120 of the 130 added lines. They pass on main too once permits() reads the old fields, so they prove no fix.
Notes
Every number is from this commit against main at 01ec085, on linux-x64. macOS and Windows code was compiled, not run.
The same change in a maintainer's queue
dylan-conway's unsafe-removal queue has the commit "threading: Semaphore and Channel keep their state in a Guarded". It is in no PR. It makes this Semaphore change, converts Channel, and deletes the whole [bun_threading] section of mordant-baseline.toml.
Outside mod tests, src/threading/Semaphore.rs here is byte-identical to that commit's file. That file merges without a conflict in either order.
mordant-baseline.toml conflicts in one hunk in either order.
RwLock writers that wait for readers (src/threading/RwLock.rs), the type checker's file reader limit on macOS (src/sema/driver/host.rs), and FSEvents on macOS (src/runtime/node/fs_events.rs).
They call only new or default, wait and post.
Cost
Instructions in the crate's release object, main to this PR (cargo rustc --release -p bun_threading --lib --target T -- --emit asm -C codegen-units=1):
target
rustflags
post
wait
Linux x64
none
39 to 39
54 to 61
Linux x64
Bun's
40 to 40
48 to 61
macOS arm64
none, Bun's
29 to 31
36 to 37
Windows x64
none
11 to 16
34 to 38
Windows x64
Bun's
14 to 19
37 to 41
"Bun's" are the release flags of scripts/build/rust.ts without the link-time LTO flags. Linux: -Crelocation-model=static -Cforce-frame-pointers=yes -Cllvm-args=-addrsig -Zshare-generics=y -Ctarget-cpu=nehalem -Zlocation-detail=none -Cforce-unwind-tables=no. macOS and Windows: the same without the first and the last flag, and macOS with -Ctarget-cpu=apple-m1.
Calls and atomic operations are equal in every row. Conditional branches are equal in every row but one: wait on Linux x64 with Bun's flags has 6 before and 8 after, because the compiler emits the count test three times where it emitted it once.
Executed instructions of one uncontended call on Linux x64 (an example program linked without LTO, third iteration, gdb stepi from entry to return). With Bun's flags: post 23 and 23, wait 22 and 26. With no flags: post 22 and 22, wait 21 and 25. Executed calls, conditional branches and atomic instructions are equal.
With Bun's flags the four added instructions in wait are push %r15, push %rax, add $8, %rsp and pop %r15. wait now keeps three addresses across the call to Condition::wait, because cond is not at offset 0.
Not measured: the linked release binary. The release link runs ThinLTO over the crates, so the shipped code can differ.
Sizes (-Zprint-type-sizes, Linux x64, macOS arm64, Windows x64): Semaphore is 24 bytes in release and 32 in dev builds, before and after. RawRwLock, which embeds it, is 40 and 56, before and after.
mordant
bun scripts/rust-mordant.ts (564c8da, Linux, Windows and macOS targets) exits 0 with the line removed. The baseline goes from 465 accepted findings to 464, and from 14 to 13 for this lint.
main with only the line removed exits 101: "unsafe impl Sync for Semaphore covers fields that are not Sync for a reason other than a raw pointer" at src/threading/Semaphore.rs:21. The lint's help names two remedies: move the cell into the mutex, or use an atomic.
Tests
They are the first unit tests that reach Semaphore::wait(). One is single-threaded and two use threads. A lost wake-up fails them at a 60 second deadline, not by a hang.
300 of 300 sequential runs pass, and 960 of 960 runs with 48 processes at a time on 12 cores. Under Miri (Tree Borrows) the three take 1.6 seconds.
As committed they do not compile against main's Semaphore, because permits() reads the new field. With that helper changed to read the old fields they pass 300 of 300 runs.
cargo clippy -p bun_threading --all-targets is clean. CI does not lint test targets.
Mutants of the new code. post() without signal(): fails 3 of 3 runs. post() adds two permits: fails 10 of 10. wait() keeps the permit: fails 10 of 10. while to if in wait(): fails 9 of 10, and 4 of 10 in an earlier measurement, because it depends on scheduling.
wait() without its second signal() passes 50 of 50 runs. The tests run that line but do not detect its removal. This PR keeps that signal().
End to end
Debug build on Linux. bun check on a 41-file project enters Semaphore::wait 0 times under gdb. On Linux only RwLock writers that wait for readers reach the semaphore.
With one local edit that is not committed (the macOS reader limit on for Linux, src/sema/driver/host.rs:526), the same run enters wait 41 times and post 47 times and reports the expected type error. test/cli/check/entry-points.test.ts and test/cli/check/named-files.test.ts pass with that edit: 168 pass, 0 fail.
Self-review
Done in part:
No 24 hour wait between the note to dylan-conway and the push. They went out together, so that the diff can be read.
No review request to a second maintainer. The PR keeps its one assignee.
The third test stays. It is the only one that detects while to if in wait().
no test proof · iteration 0 · the description declares no behaviour change, so there is no failing test to prove; the existing suite in CI is the check
No actionable comments were generated in the recent review.
Walkthrough
The semaphore now stores permits in Guarded<usize> instead of a separate mutex and unsafe cell. Its wait and post behavior remains in place. New tests cover permit counts, waiter wake-ups, and shared access to one permit.
Changes
Semaphore
Layer / File(s)
Summary
Guarded permit storage and behavior mordant-baseline.toml, src/threading/Semaphore.rs
Semaphore uses Guarded<usize> for permit storage and removes the manual Sync implementation. The wait and post operations retain their permit updates and signaling behavior. Tests cover permit counting, waiter wake-ups, and single-permit access across threads. The baseline entry for unsafe_send_covers_more_than_pointers is removed.
Suggested reviewers: dylan-conway
Merge Risk: Minimal · up to d9a46
The semaphore refactor preserves permit counting and synchronization behavior. No merge-blocking issue was identified; merge after normal checks pass.
Pre-merge checks: 4
Check name
Status
Explanation
Title check
The title clearly and concisely describes the primary change: storing Semaphore's permit count in Guarded.
Description check
The description provides detailed scope, implementation, verification results, behavior notes, performance data, and known limitations. It does not use the template headings, but it contains the requi…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Description check
Explanation
The description provides detailed scope, implementation, verification results, behavior notes, performance data, and known limitations. It does not use the template headings, but it contains the required information and is mostly complete.
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Some tools did not complete. Review the errors below.
ast-grep (0.51.1)
ast-grep could not parse rule config: /ast-grep-rules/ast-grep-essentials/rules/cpp/security/std-return-data-cpp.yml
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Reviewing files that changed from the base of the PR and between 01ec085 and d9a462c.
Files selected for processing: 2
mordant-baseline.toml
src/threading/Semaphore.rs
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
LGTM — mechanical refactor replacing hand-rolled UnsafeCell + Mutex + unsafe impl Send/Sync with the crate's existing Guarded<usize>.
What was reviewed:
Guarded::new is const fn (guarded.rs:51), so Semaphore::new() → RawRwLock::new() → RwLock::new() stay const for static initializers.
Condition::wait_guarded delegates to wait(guard.mutex()), so the unlock/park/relock sequence is identical to the old self.cond.wait(&self.mutex).
Lock/signal ordering unchanged: cond.signal() in both wait() and post() runs before the guard drops, matching the old scopeguard placement.
Send/Sync now auto-derive via GuardedBy's unsafe impl Sync (bounded on Value: Send, satisfied by usize) and Condition: Sync — no loss of thread-safety marker.
Extended reasoning...
Overview
Single-file change to src/threading/Semaphore.rs (~30 lines net removed). The permit counter moves from a bare UnsafeCell<usize> sitting next to a Mutex into Guarded<usize> (the crate's existing parking_lot::Mutex<T>-style wrapper). wait() and post() now hold a GuardedLock and deref it instead of raw-pointer-dereffing the UnsafeCell under a manually locked mutex with scopeguard::defer! unlock. The two unsafe impl Send/Sync and four unsafe blocks are deleted; the file is now unsafe-free.
Security risks
None. This is an internal threading primitive with no user-facing surface, no parsing of untrusted input, and no I/O. The change tightens type-level invariants (mutex-protects-value is now enforced by the type rather than by SAFETY comments) without altering the locking protocol.
Level of scrutiny
Low-to-medium. It touches a concurrency primitive, so I verified the behavioral equivalence carefully: (1) Guarded::new is const so the const fn new() chain used by RawRwLock and static RwLocks is preserved; (2) wait_guarded(&mut guard) calls self.wait(guard.mutex()), i.e., the exact same Condition::wait(&Mutex) path as before; (3) the guard's Drop unlocks at end-of-scope, so cond.signal() still runs while the lock is held in both methods — identical wake ordering to the scopeguard version; (4) Send/Sync are recovered via GuardedBy's blanket unsafe impl<Value: Send, M: RawMutex + Sync> Sync (guarded.rs:28) plus auto-Send, with usize: Send and Mutex: Sync both holding. No semantic drift.
Other factors
The two Rust callers (RwLock.rs, fs_events.rs) only use new/default/wait/post, all of which keep identical signatures. The PR description explicitly scopes out Channel with a stated reason (padding growth + dead flag), which is the right call for a focused change. This is exactly the kind of "use the in-tree helper instead of hand-rolling the primitive" cleanup the review guidelines encourage. No outstanding reviewer comments; bug-hunting system found nothing.
This PR now holds the change rebased on main, in one commit. The conflict with #47338 is gone.
Compared with the first version, it has two additions:
It removes the unsafe_send_covers_more_than_pointers:src/threading/Semaphore.rs line from mordant-baseline.toml. That line was added after this PR opened.
It adds three unit tests for wait() and post().
The description describes the PR as it now stands, with how it was checked and what it costs.
The count was an UnsafeCell<usize> beside a bare Mutex, with an
unsafe impl Sync for the whole struct and an unsafe block at each of
the four reads and writes. Guarded<usize> carries the same mutex and
hands out the count only while it is locked, so the impl and the
blocks are gone. Condition::wait_guarded waits on the guard's mutex.
Adds three unit tests for wait() and post(), two of them across
threads. A lost wake-up fails them by a deadline, not by a hang.
Removes the unsafe_send_covers_more_than_pointers entry for
src/threading/Semaphore.rs from mordant-baseline.toml.
robobun
changed the title
threading: put Semaphore permits behind Guarded<usize>
threading: hold Semaphore's permit count in a Guarded<usize>Nov 3, 2026
@dylan-conway The non-test part of this PR is the Semaphore half of "threading: Semaphore and Channel keep their state in a Guarded" in your unsafe-removal queue (8c59305, on the head branch of #47109, in no PR).
Outside mod tests, src/threading/Semaphore.rs in this PR is byte-identical to the file in that commit. It merges without a conflict in either order.
mordant-baseline.toml conflicts in one hunk in either order. This PR deletes the Semaphore.rs line of [bun_threading]. Your commit deletes the whole section.
This PR adds three unit tests for wait() and post(). They compile unchanged on your file.
The reason will be displayed to describe this comment to others. Learn more.
I reviewed the current revision and found no bugs; since this is a core sync primitive used by RwLock writers and the description itself notes an overlapping queued commit and #45818 touching the same baseline section, a maintainer still needs to decide whether and in what order this lands.
What was reviewed:
wait()/post() against the old path: Guarded::lock is Mutex::lock, wait_guarded is Condition::wait on the same mutex, and the guard drops (unlocks) after signal() as the old defer! did, so lock/wait/decrement/signal/unlock ordering is unchanged.
Sync/Send for Semaphore and RawRwLock now come from GuardedBy's impl (usize: Send, Mutex: Sync), and Semaphore::new() stays const, so the static uses still compile.
The mordant-baseline.toml line removal is justified: the unsafe impl it tracked no longer exists in the file.
New tests: at most one waiter can return per post (the woken waiter only re-signals when permits remain), so the try_recv().is_err() checks are not racy; the STARTED spin only waits on spawned threads reaching their first line.
Extended reasoning...
The change rewrites src/threading/Semaphore.rs to store the permit count in the in-tree Guarded instead of UnsafeCell plus a separate Mutex, removing the unsafe impl Sync and four unsafe blocks, and deletes the corresponding mordant-baseline.toml lint entry; it adds three std-thread unit tests. It touches no security-sensitive surface. Reading Guarded and Condition::wait_guarded confirms the lock, wait, decrement, signal, and unlock sequence is identical to the old code. It was not approved outright because Semaphore underpins RwLock writer waits, fs_events, and the sema reader limit, and the author's own description flags an unmerged overlapping commit in a maintainer's queue plus #45818 editing the same baseline section, which is a sequencing decision for a human.
The review found no bug and asked for no change. Nothing has moved since the push: main still has the UnsafeCell version of Semaphore, #45818 is still open, and the branch merges into main with no conflict. The order in which this, #45818 and the queued Channel commit land is for the assignee to decide.
CI on d9a462c is red on lanes this diff does not reach. test/js/web/request/request-clone-leak.test.ts read an RSS delta of 64 MB against a bound of < 64 on the x64 ASAN lane. The other red tests are under test/aot/ and fail on main too. This diff changes only Semaphore, which none of those paths use. The branch is ready for review as it stands.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Unsafe-removal and lint-ratchet maintenance, no reachable defect. Merged #46363 has this shape. #37508, #37596 and #39165 were closed as refactors. A maintainer decides whether this is wanted.
Behaviour change: none
Problem
Semaphorekeeps its permit count in anUnsafeCell<usize>beside aMutex. Oneunsafe impl Syncand fourunsafeblocks assert that the mutex protects it.unsafeblocks from the syscall wrappers and the shell #47338 removed theunsafe impl Send. The compiler cannot deriveSyncwhile the count is beside the mutex. The advisory mordant lint reports that impl:unsafe_send_covers_more_than_pointers:src/threading/Semaphore.rs.Fix
permitsis aGuarded<usize>.wait()andpost()hold its guard, andwait()blocks inCondition::wait_guarded. The file has nounsafeleft. Its line leavesmordant-baseline.toml.Guarded::lock()isMutex::lock(), its guard unlocks on drop, andwait_guardedisCondition::waiton that mutex.cargo test -p bun_threading(10 pass, 3 new), Miri,bun scripts/rust-mordant.ts(exit 0), workspace clippy and check.Background
Guarded<T>(src/threading/guarded.rs) is a mutex that owns its value.src/sema/program.rshas this shape:asleep: Guarded<usize>.AtomicUsizecount also removes theunsafe, but the type then hides that the mutex guards it.Downsides
wait()runs 26 instructions, up from 22, andpost()still runs 23. The linked release binary is not measured.maintoo oncepermits()reads the old fields, so they prove no fix.Notes
Every number is from this commit against
mainat 01ec085, on linux-x64. macOS and Windows code was compiled, not run.The same change in a maintainer's queue
SemaphoreandChannelkeep their state in aGuarded". It is in no PR. It makes thisSemaphorechange, convertsChannel, and deletes the whole[bun_threading]section ofmordant-baseline.toml.mod tests,src/threading/Semaphore.rshere is byte-identical to that commit's file. That file merges without a conflict in either order.mordant-baseline.tomlconflicts in one hunk in either order.Left alone
src/threading/channel.rs(Bound the remote image prefetch ofbun file.mdby a deadline and a byte budget #45818),src/resolver/fs.rs(fix(resolver): synchronize mutable entry-cache snapshots #44539),src/runtime/node/fs_events.rsandsrc/runtime/node/path_watcher.rs(fs.watch/fs.watchFile: remove unsafe from the watcher modules #40200).bun file.mdby a deadline and a byte budget #45818 deletes the adjacent[bun_threading]line, and the queued commit deletes the section. The change that lands last must leave neither line nor the empty[bun_threading]header.Users of
SemaphoreRwLockwriters that wait for readers (src/threading/RwLock.rs), the type checker's file reader limit on macOS (src/sema/driver/host.rs), and FSEvents on macOS (src/runtime/node/fs_events.rs).newordefault,waitandpost.Cost
Instructions in the crate's release object,
mainto this PR (cargo rustc --release -p bun_threading --lib --target T -- --emit asm -C codegen-units=1):postwaitscripts/build/rust.tswithout the link-time LTO flags. Linux:-Crelocation-model=static -Cforce-frame-pointers=yes -Cllvm-args=-addrsig -Zshare-generics=y -Ctarget-cpu=nehalem -Zlocation-detail=none -Cforce-unwind-tables=no. macOS and Windows: the same without the first and the last flag, and macOS with-Ctarget-cpu=apple-m1.waiton Linux x64 with Bun's flags has 6 before and 8 after, because the compiler emits the count test three times where it emitted it once.stepifrom entry to return). With Bun's flags:post23 and 23,wait22 and 26. With no flags:post22 and 22,wait21 and 25. Executed calls, conditional branches and atomic instructions are equal.waitarepush %r15,push %rax,add $8, %rspandpop %r15.waitnow keeps three addresses across the call toCondition::wait, becausecondis not at offset 0.-Zprint-type-sizes, Linux x64, macOS arm64, Windows x64):Semaphoreis 24 bytes in release and 32 in dev builds, before and after.RawRwLock, which embeds it, is 40 and 56, before and after.mordant
bun scripts/rust-mordant.ts(564c8da, Linux, Windows and macOS targets) exits 0 with the line removed. The baseline goes from 465 accepted findings to 464, and from 14 to 13 for this lint.mainwith only the line removed exits 101: "unsafe impl Sync for Semaphorecovers fields that are notSyncfor a reason other than a raw pointer" atsrc/threading/Semaphore.rs:21. The lint's help names two remedies: move the cell into the mutex, or use an atomic.Tests
Semaphore::wait(). One is single-threaded and two use threads. A lost wake-up fails them at a 60 second deadline, not by a hang.main'sSemaphore, becausepermits()reads the new field. With that helper changed to read the old fields they pass 300 of 300 runs.cargo clippy -p bun_threading --all-targetsis clean. CI does not lint test targets.post()withoutsignal(): fails 3 of 3 runs.post()adds two permits: fails 10 of 10.wait()keeps the permit: fails 10 of 10.whiletoifinwait(): fails 9 of 10, and 4 of 10 in an earlier measurement, because it depends on scheduling.wait()without its secondsignal()passes 50 of 50 runs. The tests run that line but do not detect its removal. This PR keeps thatsignal().End to end
bun checkon a 41-file project entersSemaphore::wait0 times under gdb. On Linux onlyRwLockwriters that wait for readers reach the semaphore.src/sema/driver/host.rs:526), the same run enterswait41 times andpost47 times and reports the expected type error.test/cli/check/entry-points.test.tsandtest/cli/check/named-files.test.tspass with that edit: 168 pass, 0 fail.Self-review
Done in part:
whiletoifinwait().Channel, are not part of this change.History
wait()andpost()and declaredpermitsbeforecond. It did not touchmordant-baseline.toml.mainbecause Remove 118unsafeblocks from the syscall wrappers and the shell #47338 deleted theunsafe impl Sendlines and the first version deleted that whole block.no test proof · iteration 0 · the description declares no behaviour change, so there is no failing test to prove; the existing suite in CI is the check