Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
Included review availability: Your plan includes up to 5 reviews per rolling hour; 0 remain after this review. WalkthroughThe task runner now uses lifetime-parameterized callback trait objects and runtime flags. Package, store, manifest, hoisted, isolated, and queue installation paths were updated to pass callback instances directly. ChangesRuntime callback dispatch migration
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
@robobun adopt |
|
Closed as superseded by #39770, which landed the same change on main (run_tasks compiled once through an ErasedCallbacks table, the two bools as runtime arguments). Reopen or ask if you want the dyn trait version as a follow up cleanup on top of main. The test from 01efdf5 applies cleanly to main on its own. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it refactors the core run_tasks install loop across 8 files — swapping static dispatch for &mut dyn, reworking the trait's lifetime parameter, and touching the raw-pointer provenance setup around the scopeguard shadows — a human look would still be worthwhile.
What was reviewed:
- Every
C::CONST→flags.fieldandC::method(ctx, …)→extract_ctx.method(…)site inrun_tasksmaps 1:1;flagsis snapshotted once before theextract_ctx_ptrshadow. - The new
as_package_installer/as_store_installerreturnselfdirectly, removing the old unsafe lifetime-narrowing casts. process_dependency_list_for_ctxnow callsextract_ctx.flags()after derivingctx_ptr— examined under Stacked Borrows and ruled out as a regression (shared read, and the closure runs after the borrow ends).- All six impls (
PackageInstaller,Store::Installer,QueueRunTasksCallbacks, and the three unit markers) set the same flag combinations their old associated consts did.
Extended reasoning...
Overview
This PR de-monomorphizes bun install's run_tasks loop and install_package_with_name_and_resolution. The RunTasksCallbacks trait moves from an associated-const / associated-type design (with type Ctx and const HAS_* gates) to a dyn-compatible trait whose impls are the context types (PackageInstaller<'a>, Store::Installer<'a>, a QueueRunTasksCallbacks wrapper, and three unit markers). A RunTasksFlags struct returned from flags() replaces the associated consts, and run_tasks takes &mut dyn RunTasksCallbacks<'_> with #[inline(never)]. Separately, the two const-bool generics on install_package_with_name_and_resolution and fail_with_invalid_url become runtime bool parameters. The stated goal is ~105 KB of pre-LTO text savings by collapsing 7 stamped copies of the 20 KB body into one.
Security risks
None. This is a dispatch-mechanism refactor with no change to input handling, validation, network/registry paths, or filesystem operations. The extract, manifest, and integrity paths are untouched.
Level of scrutiny
High. run_tasks is the main-thread drain loop for every network/extract/git task during bun install, and it is dense with unsafe (raw-pointer provenance roots for scopeguard, union arm accesses, detached lifetimes). The transformation itself is mechanical — I traced each C::HAS_* / C::IS_* gate to its flags.* replacement and each static C::method(ctx, …) call to its extract_ctx.method(…) equivalent, and they all line up. The flags snapshot is taken from the fn-param extract_ctx before the extract_ctx_ptr provenance root is derived and the param is shadowed, so the guard's capture of flags.progress_bar is by value and unaffected by the reborrow dance. The trait's new 'a lifetime parameter lets as_package_installer/as_store_installer return &mut PackageInstaller<'a> / &mut Store::Installer<'a> as plain self, deleting two unsafe variance casts — a net safety improvement.
Other factors
Two candidate Stacked Borrows issues around process_dependency_list_for_ctx (calling extract_ctx.flags() after deriving ctx_ptr) were raised by finder agents and refuted by verifiers; flags() takes &self and the closure that dereferences ctx_ptr runs only after that shared reborrow is dead, matching the surrounding provenance conventions in this file. No new tests ship, which is reasonable for a behavior-preserving refactor covered by the existing install/add/streaming-extract/integrity/workspace suites the description names. Given the breadth (8 files, core install path) and the unsafe surface it threads through, I'm deferring rather than approving so a maintainer can confirm the code-size win and the dyn-dispatch placement (per-task boundary) matches their perf expectations.
process_dependency_list already takes a ctx value for its on_resolve callback, so process_dependency_list_for_ctx can hand it the &mut dyn directly instead of smuggling a raw pointer into the closure.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The const-generic → runtime-bool and static → dyn-trait conversions look mechanically correct and drop two unsafe lifetime casts. Because it restructures the trait lifetime on RunTasksCallbacks and threads a *mut dyn through the existing raw-pointer provenance dance in run_tasks, a human look at the install core loop would still be worthwhile.
What was reviewed:
- All
run_tasks/install_package_with_name_and_resolution/fail_with_invalid_urlcall sites updated; no stragglers. flags()snapshot atrun_tasksentry vs. per-call inprocess_dependency_list_for_ctx— every impl returns a fixed value, so no divergence from the old associated-const behavior.process_dependency_list_for_ctxnow passesextract_ctxas the ctx argument instead of smuggling a raw pointer through a closure — matchesprocess_dependency_list'sFnOnce(C)shape.as_package_installer/as_store_installernow returnselfwith the trait's'a, replacing the raw-pointer variance cast.
Extended reasoning...
Overview
The PR de-monomorphizes two heavily-stamped generics in bun install: run_tasks (previously generic over a RunTasksCallbacks trait with an associated Ctx type and const bool gates, instantiated 7×) and install_package_with_name_and_resolution (previously stamped per <const NEEDS_VERIFY, const IS_PENDING_PACKAGE_INSTALL> pair). The trait is reworked to be dyn-compatible — hooks become &mut self methods, the const gates become a RunTasksFlags struct returned from flags(), and the two installer downcasts return &mut PackageInstaller<'a> / &mut Store::Installer<'a> with 'a as a trait lifetime parameter. run_tasks now takes &mut dyn RunTasksCallbacks<'_> and is #[inline(never)]. Eight files across src/install/ and src/jsc/AsyncModule.rs update to the new shape.
Security risks
None. This is an internal dispatch refactor with no change to input handling, validation, or trust boundaries.
Level of scrutiny
Medium-high. The change is mechanical and behavior-preserving in intent, but it sits inside run_tasks, which is the install task-drain core and already relies on carefully-staged raw-pointer provenance (manager_ptr / extract_ctx_ptr shadowing for the drain scopeguard). The PR changes extract_ctx_ptr to a fat *mut dyn and reworks how the trait's inner lifetime flows through as_*_installer. I traced each hook site and confirmed the flag gates match the old HAS_* / IS_* consts one-for-one, and that every impl's flags() is effectively constant (Queue's progress is fixed at construction), so snapshotting once at entry matches the old semantics. I also confirmed there are no remaining callers of the old generic signatures.
Other factors
Two unsafe blocks (the inner-lifetime variance casts in hoisted_install.rs and isolated_install.rs) are removed outright, and process_dependency_list_for_ctx drops its raw-pointer closure smuggle in favor of passing the callbacks object as the generic ctx — both are net safety wins. robobun reports the full install test matrix (isolated/hoisted, patch, git-deps, streaming-extract, tarball-integrity, workspaces, auto-install) passing on the debug build. That said, this is a ~350-line refactor of production-critical native code with a stated perf/size rationale; per the repo's review norms around refactors and memory-safety-adjacent code, a maintainer familiar with the install subsystem should sign off on the dyn-dispatch trade-off and the RunTasksCallbacks<'a> lifetime shape.
The other three callers of install_package_with_name_and_resolution already label needs_verify and is_pending_package_install at the call site. This one passed two bare literals.
|
Updated 2:41 AM PT - Aug 18th, 2026
✅ @robobun, your commit 01efdf5e5beab982265a49198c28852705f04914 passed in 🧪 To try this PR locally: bunx bun-pr 39516That installs a local version of the PR into your bun-39516 --bun |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It's a mechanical de-monomorphization of run_tasks and install_package_with_name_and_resolution, but it redesigns the callbacks trait (new 'a lifetime parameter, &mut dyn dispatch, *mut dyn fat pointer in the scopeguard provenance dance) across the install core loop, so a maintainer glance at the trait shape would still be worthwhile.
What was reviewed:
- All six
RunTasksCallbacksimpls return the same flag values as the old associated consts;flags()snapshotted once at the top ofrun_tasksis equivalent since every impl returns a constant. process_dependency_list<C>acceptsC = &mut dyn RunTasksCallbacksand only threadsctxthrough toon_resolve, so dropping the raw-pointer capture inprocess_dependency_list_for_ctxis sound.- The two unsafe lifetime-narrowing casts in
as_package_installer/as_store_installerare replaced by safe identity returns via the new trait lifetime; the deletedHoistedRunTasksCallbacks/StoreRunTasksCallbacksphantom types have no remaining references. - All four callers of
install_package_with_name_and_resolution(andfail_with_invalid_url) pass the same bool values as the old const generics.
Extended reasoning...
Overview
This PR de-monomorphizes two hot install-path functions to shrink binary size (~105 KB pre-LTO):
run_tasksinrunTasks.rs— was generic overRunTasksCallbackswith an associatedCtxtype andHAS_*/IS_*associated consts; now takes&mut dyn RunTasksCallbacks<'_>and reads aRunTasksFlagsstruct once viaflags(). Tagged#[inline(never)].install_package_with_name_and_resolutioninPackageInstaller.rs— two const-generic bools become plain runtime args.
Six impls updated: three unit-marker types (VoidRunTasksCallbacks, ManifestsOnlyCallbacks, InstallWaitCallbacks), PackageInstaller<'a> and store::Installer<'a> now impl the trait directly (phantom wrapper types deleted), and QueueRunTasksCallbacks becomes a struct wrapping &mut Queue + a runtime progress bool (was <const PROGRESS: bool>). Eight files touched, ~400 lines of diff, all in src/install/ and src/jsc/AsyncModule.rs.
Security risks
None. This is an internal dispatch-mechanism refactor with no new inputs, no parsing, no auth/crypto/network surface changes. Net unsafe is reduced: two unsafe lifetime-narrowing pointer casts and one raw-pointer closure capture are removed.
Level of scrutiny
Medium-high. The change is mechanical and behavior-preserving by construction, but it sits in bun install's core task-drain loop — production-critical, dense with raw-pointer provenance handling and scopeguard defers, and the repo's review guidance calls native memory safety the most-blocked category. The trait gains a lifetime parameter and run_tasks now stores a *mut dyn fat pointer for its scopeguard; both are straightforward but worth a maintainer's eye given how load-bearing the surrounding provenance comments are.
Other factors
- No CODEOWNERS on
src/install/. - The bot ran the full install/isolated/patch/git-deps/workspace/auto-install suites on the debug build and they pass; the 14
bun-install.test.tsfailures are external-host tests that fail identically on main. process_dependency_list's signature (ctx: C, on_resolve: Option<impl FnOnce(C)>) was verified to acceptC = &mut dyn RunTasksCallbacksand only forwardsctxto the callback, so the removed raw-pointer capture was genuinely unnecessary.- The
QueueRunTasksCallbacksrefactor inAsyncModule.rspreserves theprogress → LogLevelmapping exactly (true → Default,false → DefaultNoProgress).
I'm deferring rather than approving because the trait redesign (new lifetime parameter, dyn-compatibility) and the fat-pointer scopeguard change touch enough of the install core that a maintainer confirming the shape is warranted, even though every substitution I traced is 1:1.
Nothing asserted the isolated installer's on_package_download_error path before. With a lockfile the store installer downloads the tarball itself, and a 404 has to be reported as its own "failed to download" error (the hoisted installer and the resolution phase log "GET <url> - 404"). This pins the download error dispatch that run_tasks now selects through the flags struct.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reshapes the core run_tasks drain loop — swapping const-generic dispatch for &mut dyn, threading a new 'a lifetime through the trait's downcasts, and changing the fat-pointer provenance root at extract_ctx_ptr — a human look would still be worthwhile.
What was reviewed:
- Each
flags()impl matches its former associated consts one-for-one;flagsis read once at loop entry and captured by the draindefer!, so gate values stay consistent per call. process_dependency_list_for_ctxnow passesextract_ctxas the genericctxarg —process_dependency_listonly touchesctxin the trailingon_resolve(ctx), so the dropped raw-pointer capture is safe.- The
as_package_installer/as_store_installeridentity impls return&mut Self<'a>directly, replacing the old unsafe inner-lifetime narrowing casts; no remaining references to the removedHoistedRunTasksCallbacks/StoreRunTasksCallbacksmarkers. - New isolated-install test uses a local
Bun.serveandtempDir, and asserts the store installer's own error text (distinct from the hoistedGET … - 404path).
Extended reasoning...
Overview
This PR de-monomorphizes bun install's run_tasks loop and install_package_with_name_and_resolution. The RunTasksCallbacks trait's associated consts (HAS_ON_*, IS_*_INSTALLER, PROGRESS_BAR, MANIFESTS_ONLY) and type Ctx become a RunTasksFlags struct returned by fn flags(&self), and the trait is now dyn-compatible with a 'a lifetime for the two installer downcasts. All six impls (PackageInstaller, Store::Installer, VoidRunTasksCallbacks, ManifestsOnlyCallbacks, InstallWaitCallbacks, QueueRunTasksCallbacks) are updated in place, the two phantom marker types are deleted, and install_package_with_name_and_resolution's two const-bool generics become plain arguments. A new test covers the store installer's tarball-404 callback path.
Security risks
None. No user-input parsing, auth, crypto, or trust boundaries are touched; the change is dispatch mechanics only.
Level of scrutiny
High. run_tasks is the hot loop that drains every network/extract/git task during bun install, and the surrounding code is dense with raw-pointer provenance roots (manager_ptr, extract_ctx_ptr) and scopeguard::defer! reborrows. The refactor changes extract_ctx_ptr from a thin *mut C::Ctx to a fat *mut dyn RunTasksCallbacks<'_> and reshapes how the two as_* downcasts hand back &mut Installer<'a>. I traced each flag gate and callback dispatch site against its previous const-gate and found them equivalent, and the new lifetime plumbing lets the identity casts drop their unsafe — but this is exactly the kind of change a maintainer should eyeball.
Other factors
- The adopt comment reports 13+ install test suites passing on the debug build; the 14
bun-install.test.tsfailures are external-host tests that also fail on main. - The added test follows harness conventions (local
Bun.serve({ port: 0 }),tempDir,runBunInstallhelper, per-test cache dir) and asserts the store-installer-specific error string, so it exerciseson_package_download_error_storerather than the generic log path. process_dependency_list's genericCis only consumed by the trailingon_resolve(ctx)call, so passing the&mut dynthrough asctx(instead of the previous raw-pointer closure capture) is a straightforward simplification.- The dyn-dispatch cost sits at the per-completed-task boundary, not inside any per-byte or per-file loop, matching the PR description; whether that trade-off against ~105 KB of duplicated text is desirable is a maintainer call.
|
@alii this branch now conflicts with main because #39770 (merged Aug 21) landed the same change by another route. On main, What is left of this branch, if rebased, is a redesign of that erasure layer: the callbacks become a dyn compatible trait that the installers implement themselves, which deletes Two options:
The test from 01efdf5 (a tarball 404 during an install from a lockfile has to come out as the store installer's own error) applies cleanly to main and is worth keeping either way. Tell me which option you want. |
|
Closing as superseded by #39770, which compiles If you want the dyn trait version as a follow up cleanup of the erasure layer, reopen this or say so and I will redo it on top of main. The test from 01efdf5 (a tarball 404 during an install from a lockfile has to come out as the store installer's own "failed to download" error) applies cleanly to main on its own. |
2 participants
bun install's run_tasks loop is generic over a callbacks trait, so the whole 20 KB task-drain body was compiled once per caller: 5 copies in bun_install and 2 in bun_jsc, and install_package_with_name_and_resolution was further stamped per pair of const bool flags. The bodies are the same code with different callback targets.
The loop now takes the callbacks as a
&mut dynobject and the two verify flags are plain arguments, so one body exists. Pre-LTO the rlib text drops by about 105 KB (run_tasks −108 KB and the flag pairs −44 KB, minus the shared bodies). The dyn calls sit at the per-task boundary (one per finished network or extract task), not inside any per-byte or per-file loop; the extract and manifest paths themselves are unchanged.Behavior is unchanged. bun-install, bun-add, streaming-extract, tarball-integrity and the workspace tests pass on the debug build.
no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/isolated-install.test.ts