install: wait for the isolated installer's pool tasks before it is freed

Closed
RO
1mo ago
+69-23

Problem

  • BUN-4960: bun install --linker=isolated segfaults in Installer::is_task_blocked (Segmentation fault at address 0x807). The report's feature bits show Global::exit never ran, so install_isolated_packages returned while a pool task still read the installer.
  • Installer and Store are locals of that function (src/install/isolated_install.rs) and every task reads them through Task::installer. No return path waited for the tasks. An early return under ASAN gives a heap-use-after-free in a pool thread (Notes).
  • The exact way the released build left the function is still unknown. The candidates are a pending-task slot released twice and the ? returns after tasks were started.

Fix

  • Each Task embeds a thread_pool::CountedTask on a WaitGroup that the function owns. start_task adds one, the pool finishes it when Task::callback returns, and Installer::drop waits 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.
  • Under CI_ASSERT (on in canary) Installer.released records 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.
  • Verified by fault injection only (Notes). The drain has no behavior a test can observe from outside: a release build reaches it only through the unknown trigger or an allocation failure. The hang that an earlier version of this PR also fixed is now #39672, with its tests.
  • Touches the same start_task lines as #37894. Whichever lands second needs a trivial rebase.

Background

  • The isolated installer builds one Store entry per package variant and one Task per entry. Tasks run on the shared thread pool and report back through Installer.task_queue.
  • The main thread holds one pending-task slot per entry and leaves the loop when the count is zero. A slot released twice lets it leave while a task still runs.
  • CountedTask (src/threading/ThreadPool.rs) wraps a task callback and finishes a WaitGroup after the callback returns. WaitGroup::wait returns only after that finish is done with the group, so the waiter may free everything afterwards.
  • Store::drop already assumed that no task was running. Installer::drop makes 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 address 0x807. The packed features decode to binlinks, bunfig, extracted_packages, text_lockfile, isolated_bun_install. Bit 50 (exited, set by Global::exit) is clear, and so are lifecycle_scripts and git_dependencies. On Linux Global::exit is quick_exit, which keeps the stack intact, so the exit paths cannot free the store either way. The dependencies column of a store entry is always a built Vec (build_store appends whole entries, Dependencies::EMPTY is Vec::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, and build_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) at ThreadPool.rs:1249 (the task callback), freed by drop_glue::<Installer> in install_isolated_packages, in 4 of 5 runs (one of them a 4 byte read, a step load). With the CountedTask drain: nothing in 6 of 6 runs. The injection is not part of this PR.

Re-scheduling. A task that reports Blocked or RunScripts is scheduled again later by start_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, which WaitGroup supports. wait() is only called from drop, after the last start_task. The Yield::Yield arm (the unreachable Blocked step) 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_done that 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 need node on PATH, same before the change), test/internal/source-lints. cargo clippy -p bun_install and cargo fmt are clean. A stress of 53 entries with 404, 500 and dropped tarball downloads found no crash or assertion.