install: wait for the isolated installer's pool tasks before it is freed - #39640
install: wait for the isolated installer's pool tasks before it is freed#39640robobun wants to merge 1 commit into
Conversation
|
Status: ready for review. Scope reduced to the drain. The hang fix that was bundled here at first is now #39672, with the tests. This PR keeps the drain ( CI: the only red lane on the latest build is |
|
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 (5)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. WalkthroughThe change adds explicit cleanup for failed download task generation and coordinates isolated-install task lifetimes with ChangesIsolated install reliability
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
The automated review raised no items. The red |
![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 changes cross-thread lifetime management in the isolated installer (WaitGroup ordering, raw-pointer accesses that must not outlive finish_raw, and a Drop that blocks), and the exact trigger of the Sentry crash is still open per the notes, a human look would be worthwhile.
What was reviewed:
WaitGroup::finish_raw/waitsemantics vs. the ordering inTask::callback—finish_raw's unlock is the last access, andmanager_ptroutlives the installer, so the post-finishwake_rawis safe.- The hoisted
wake_rawforYield::Yield— a spurious wake, andfinish_rawthere preventsdropfrom blocking on the unreachable arm. forget_failed_download:has_created_network_taskinserts the dedupe entry beforefor_tarballcan fail, somark_network_task_failedfinds it and the second entry getsAlreadyFailed.- The
releasedbitset is written unconditionally but only asserted underCI_ASSERT;release_pending_taskis reached only fromon_task_complete/on_task_fail, so blocked-then-resumed entries don't trip thestart_taskcheck.
Extended reasoning...
Overview
This PR fixes a use-after-free class in the isolated installer (bun install --linker=isolated): pool tasks read the stack-local Installer/Store after install_isolated_packages returned. It adds a WaitGroup counted in start_task and finished in Task::callback, with Installer::drop waiting on it so every return path drains the pool first. It also fixes a related hang where a failed generate_network_task_for_tarball left the caller's context queued under the task id, so a second store entry for the same package waited forever; forget_failed_download now removes the queue entry and marks the dedupe entry failed. A per-entry released bitset adds CI_ASSERT diagnostics for double-release. Five files touched: three in src/install/, one comment update in Store.rs, and a new test in isolated-install.test.ts.
Security risks
None identified. This is internal lifetime bookkeeping in the package manager; no user input parsing, auth, or crypto is touched. The forget_failed_download path narrows the state left behind on an existing error return.
Level of scrutiny
High. Per the repo's REVIEW.md, native memory safety is the most-blocked category, and this change sits precisely there: cross-thread raw-pointer lifetime enforced by a WaitGroup, a Drop impl that blocks, ordering between task_queue.push, finish_raw, and wake_raw, and refactoring that moves per-arm wake_raw calls to a single post-match call. I traced the WaitGroup implementation (finish_raw publishes 0 under the mutex, so wait() cannot return before its unlock) and confirmed manager_ptr is captured before finish_raw and points at the longer-lived PackageManager. I checked that has_created_network_task inserts into network_dedupe_map before the fallible for_tarball, so mark_network_task_failed finds the entry. The reasoning in the PR's Notes about the Yield::Yield arm and add/finish ordering matches the code.
Other factors
The PR description is unusually thorough (ASAN repro with fault injection, 5/5 vs 4/5 counts, ordering analysis, full suite results) and the new test hangs on the released bun. The bug hunting system found nothing. Still, the author states the exact trigger of BUN-4960 is open and the CI_ASSERT bitset is there to name it next time — this is defensive hardening around an unproven root cause in concurrency-critical code, which is exactly where a second pair of human eyes on the Drop-blocks-on-WaitGroup design and the finish_raw→wake_raw ordering is worth having.
|
The points in the review match the intended design: The request for a human look on the |
|
Updated 11:31 AM PT - Aug 19th, 2026
❌ @robobun, your commit 7422bbc has 1 failures in 🧪 To try this PR locally: bunx bun-pr 39640That installs a local version of the PR into your bun-39640 --bun |
0f9ad83 to
fb3ba26
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
The isolated installer and its store live on the stack of install_isolated_packages. Tasks on the thread pool read both through Task::installer. Nothing waited for those tasks on the way out of the function, so a return while a task was still running let the task read freed memory. Each task is now a CountedTask on a WaitGroup that the function owns. start_task adds one, the pool finishes it when the task callback returns, and Installer::drop waits for it, so every way out of the function drains the pool before the installer and the store are freed. Under CI_ASSERT the installer also records which entries released their pending-task slot, and panics when an entry releases it twice or is started after releasing it. Either one lets the install loop finish while a task is still running.
fb3ba26 to
7422bbc
Compare
|
Restructured after a second look at the diff:
|
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/install/isolated_install/Installer.rs:136-145— The PR description claims a new test intest/cli/install/isolated-install.test.tsand a fix toenqueue_package_for_download/enqueue_tarball_for_downloadinPackageManagerEnqueue.rs, but neither is in the diff — only the 3isolated_install*.rs files changed. The CodeRabbit timeline shows an earlier push (0f9ad83) processed 5 files includingPackageManagerEnqueue.rs; they appear to have been dropped in the force-push to fb3ba26. Please restore the dropped test and enqueue fix (or correct the description and add a test for the WaitGroup/Drop change) — REVIEW.md requires every behavioral change to ship an automated test in the same PR.Extended reasoning...
What is missing
The PR description makes two concrete claims that are not reflected in the diff:
- "Verified:
test/cli/install/isolated-install.test.ts(the new test hangs before the fix)" — implies a new test was added to that file. - "The two
enqueue_*_for_downloadfunctions drop the queued context and mark the task id failed. Later entries getAlreadyFailed, which every caller already releases." — describes a change tosrc/install/PackageManager/PackageManagerEnqueue.rs.
Neither file appears in the diff.
git diff --name-only 0a4e3b1e..fb3ba266lists exactly three files:src/install/isolated_install.rs src/install/isolated_install/Installer.rs src/install/isolated_install/Store.rsgit logon bothsrc/install/PackageManager/PackageManagerEnqueue.rsandtest/cli/install/isolated-install.test.tsshows their last change was commit92fa5e49(PR #39558), not this PR.Evidence the changes were dropped, not never-written
The CodeRabbit review timeline records two review passes:
- First pass (a356964..0f9ad83): "Files selected for processing (5)" — including
src/install/PackageManager/PackageManagerEnqueue.rs. - Second pass (0f9ad83..fb3ba26): "Files selected for processing (2)" — only
isolated_install.rsandInstaller.rs.
Commit 0f9ad83 no longer exists in the repo. This indicates the branch was force-pushed and the intermediate commit that carried the enqueue fix and test was lost — the description was written against 0f9ad83 and never updated.
Why this blocks merge
(a) No test ships. REVIEW.md is explicit: "Every behavioral change ships an automated test in the same PR. 'Verified manually', unnamed 'existing tests', and benchmarks don't count, even for one-liners." This PR changes concurrent-teardown behavior (adds a
WaitGroup, aDropimpl that blocks, and areleasedbitset with CI assertions) and ships zero new tests. The description's Notes even outline exactly the test that was intended (two workspaces depending onpeer-deps@1.0.0,ftp://tarball URL, hangs on release bun / exits 1 with the fix) — but it isn't in the diff.(b) The described hang fix is not applied. The description's Problem section says: "when
enqueue_package_for_downloadcannot create the network task, the entry's context stays queued under the task id. A second store entry of the same package waits on it forever, sobun installhangs." And the Fix section says the twoenqueue_*_for_downloadfunctions were changed to drop the queued context and mark the task id failed. SincePackageManagerEnqueue.rsis unchanged, that hang is not fixed by what merges here — yet the description states it is. This directly conflicts with CLAUDE.md rule 11 ("NEVER overstate what you got done").(c) The PR title/description no longer match the diff. Anyone reading the merged commit will believe the InvalidURL-enqueue hang is fixed when it is not.
How to fix
Either:
- Restore the dropped changes: the
PackageManagerEnqueue.rshunk (drop the queued context and mark the task id failed on enqueue failure) and the new test case intest/cli/install/isolated-install.test.tsthat reproduces the hang; or - Remove the enqueue-hang claims from the description, and add a test that covers what does ship — the
WaitGroup/Installer::dropwait and thereleaseddouble-release assertion (e.g. an isolated install that hits an early-return path with tasks in flight, verified clean under ASAN as the Notes describe).
Step-by-step proof
git rev-parse fb3ba266^→0a4e3b1e(the PR's base is on main; this is a single-commit PR).git diff --name-only 0a4e3b1e..fb3ba266→ only the 3isolated_install*.rs files.git log -1 --oneline -- src/install/PackageManager/PackageManagerEnqueue.rs→92fa5e49 (#39558)— this PR did not touch it.git log -1 --oneline -- test/cli/install/isolated-install.test.ts→92fa5e49 (#39558)— this PR did not touch it.- PR description, Fix bullet 3: "The two
enqueue_*_for_downloadfunctions drop the queued context and mark the task id failed." — no such hunk exists in the diff. - PR description, Fix bullet 4: "
test/cli/install/isolated-install.test.ts(the new test hangs before the fix)" — no test hunk exists in the diff. - CodeRabbit timeline: earlier review at 0f9ad83 processed 5 files including
PackageManagerEnqueue.rs; final review at fb3ba26 processed 2. Commit 0f9ad83 is gone → force-push dropped it. - REVIEW.md: "Every behavioral change ships an automated test in the same PR" — violated.
- "Verified:
|
That review ran against the description from before the split. The current description matches the diff: the enqueue fix and its two tests are #39672 (where the fix also moved one level down, into This PR ships without a test on purpose, and the description says so. Every return out of |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes concurrent lifetime management (a blocking Drop on Installer that waits on pool workers reading the same struct through BackRef), and ships without an automated test by design, a human look is still worthwhile — which the author has already requested.
What was reviewed:
CountedTask::run_and_finishreadsrun/groupbefore callingTask::callback, so the worker's only post-callback access isWaitGroup::finish_rawon the stack-local group — safe to free the installer oncewait()returns.CountedTask.taskis at offset 0 (const _assert), sofrom_field_ptr!(Task, task, ...)still recovers the outerTaskfrom the innerthread_pool::Taskpointer.add_one()→schedule()instart_taskhas no fallible call between them, so the count cannot leak and hangwait(); re-scheduledBlocked/RunScriptstasks add before the prior finish, whichWaitGroupsupports.release_pending_taskis only reached fromon_task_complete/on_task_fail(main thread), so a resumed blocked entry does not trip thestart_taskreleasedassertion.
Extended reasoning...
Overview
This PR hardens the isolated installer against a use-after-free where install_isolated_packages returns (via ? on allocation failure or an unidentified accounting drift) while thread-pool tasks still read the stack-local Installer/Store through Task::installer. The fix wraps each pool task in the existing thread_pool::CountedTask on a caller-owned WaitGroup, adds Installer::drop that waits on the group, and centralizes pending-task release into release_pending_task with a CI_ASSERT-gated double-release check backed by a per-entry released bitset. Three files touched: isolated_install.rs (group + task construction), Installer.rs (Drop, start_task, release_pending_task), Store.rs (SAFETY comment update only).
Security risks
None. This is internal lifetime coordination in the package installer; no user-facing surface, parsing, auth, or crypto is touched. The change strictly narrows the window in which pool workers may observe freed memory.
Level of scrutiny
High. Per the repo review guide, native memory-safety and thread-affinity changes are the most-blocked category. This PR adds a blocking Drop and relies on several ordering invariants: declaration order of tasks_in_flight vs. installer (borrow-checker-enforced, since Installer<'a> borrows both &store and &tasks_in_flight), CountedTask's offset-0 layout so from_field_ptr! still works, and WaitGroup::finish_raw's contract that the group may be freed once wait() returns. I verified each of these against src/threading/ThreadPool.rs and src/threading/WaitGroup.rs and found them sound, and confirmed no fallible call sits between add_one() and schedule() in start_task.
Other factors
The PR ships without an automated test — the author explains the drain is only reachable via allocation failure or an unknown Sentry-reported trigger, and verified via ASAN fault injection (4/5 UAF without, 0/6 with). REVIEW.md normally requires a test in the same PR; whether fault-injection-only verification is acceptable here is a maintainer call. The author has explicitly requested a human look on the Drop wait and callback ordering, and dylan-conway is assigned. There is also a stated overlap with #37894 on the start_task lines. Given all of that, deferring rather than approving.
|
The re-review against the current diff clears the earlier finding about the description, and it confirms the four invariants this change rests on (finish after the callback returns, |
2 participants
Problem
bun install --linker=isolatedsegfaults inInstaller::is_task_blocked(Segmentation fault at address 0x807). The report's feature bits showGlobal::exitnever ran, soinstall_isolated_packagesreturned while a pool task still read the installer.InstallerandStoreare locals of that function (src/install/isolated_install.rs) and every task reads them throughTask::installer. No return path waited for the tasks. An early return under ASAN gives aheap-use-after-freein a pool thread (Notes).?returns after tasks were started.Fix
Taskembeds athread_pool::CountedTaskon aWaitGroupthat the function owns.start_taskadds one, the pool finishes it whenTask::callbackreturns, andInstaller::dropwaits for it. Every way out of the function now drains the pool before the installer and the store are freed. On the normal path the wait returns at once.CI_ASSERT(on in canary)Installer.releasedrecords each entry whose slot was released. A second release, or a start after it, panics with the entry id instead of leaving a later segfault. This names the unknown trigger if it happens again.start_tasklines as Project thread-pool batch tasks out of the object's pointer, not a reference #37894. Whichever lands second needs a trivial rebase.Background
Storeentry per package variant and oneTaskper entry. Tasks run on the shared thread pool and report back throughInstaller.task_queue.CountedTask(src/threading/ThreadPool.rs) wraps a task callback and finishes aWaitGroupafter the callback returns.WaitGroup::waitreturns only after that finish is done with the group, so the waiter may free everything afterwards.Store::dropalready assumed that no task was running.Installer::dropmakes it true. The installer borrows both the store and the group, so it is dropped before them.Notes
What the report says. Trace string
li252bf09c...: one frame,is_task_blocked, fault address0x807. The packed features decode to binlinks, bunfig, extracted_packages, text_lockfile, isolated_bun_install. Bit 50 (exited, set byGlobal::exit) is clear, and so arelifecycle_scriptsandgit_dependencies. On LinuxGlobal::exitisquick_exit, which keeps the stack intact, so the exit paths cannot free the store either way. Thedependenciescolumn of a store entry is always a builtVec(build_storeappends whole entries,Dependencies::EMPTYisVec::new()), so a lazily initialised list is not the cause. What remains is the driver leaving the function. Every?return after the start loop is reachable only on allocation failure (get_or_put,package_patch_info, andbuild_url, whose string store falls back to the heap for long URLs), which is also why no test can reach the drain from outside. I did not find a double release by reading, or by running the install suites and a download failure stress with a per-entry state assertion, which is why the assertion ships in canary builds.Fault injection. I temporarily added
return Err(AllocError)after the start loop and ran a 53 package isolated install with a warm cache on the ASAN build. Without the drain:AddressSanitizer: heap-use-after-free ... READ of size 8 ... thread T5 (Bun Pool 1)atThreadPool.rs:1249(the task callback), freed bydrop_glue::<Installer>ininstall_isolated_packages, in 4 of 5 runs (one of them a 4 byte read, a step load). With theCountedTaskdrain: nothing in 6 of 6 runs. The injection is not part of this PR.Re-scheduling. A task that reports
BlockedorRunScriptsis scheduled again later bystart_task, which adds to the group before the previous finish may have run. That is an add racing a finish on a count of at least one, whichWaitGroupsupports.wait()is only called fromdrop, after the laststart_task. TheYield::Yieldarm (the unreachableBlockedstep) returns from the callback like every other arm, so its count is finished too.Possible follow-up, not in this PR. A check in
Wait::is_donethat fails entries whose download callbacks can no longer arrive would turn the next accounting drift into an exit 1 instead of a hang.Suites run with the debug build:
isolated-install(82),isolated-relink,public-hoist-pattern,bun-workspaces,bun-install-lifecycle-scripts(3 failures that neednodeon PATH, same before the change),test/internal/source-lints.cargo clippy -p bun_installandcargo fmtare clean. A stress of 53 entries with 404, 500 and dropped tarball downloads found no crash or assertion.