install: fail instead of hanging when a tarball download task cannot be created - #39672
install: fail instead of hanging when a tarball download task cannot be created#39672robobun wants to merge 1 commit into
Conversation
…be created generate_network_task_for_tarball records the task id in the dedupe map before for_tarball can fail, for example on a tarball URL that is not http. After such a failure every later enqueue for the same id saw a task that was already created and queued its callback behind it. No task existed, so the isolated installer waited for the callback forever. This happened for the second store entry of a package, and for any package whose manifest came from the manifest cache, because the resolve phase had already failed the same way. On that error path, mark the task id failed and return the network task slot to the pool. Later enqueues get AlreadyFailed, which every caller already handles, and the install exits with an error.
|
Warning Review limit reached
Next review available in: 2 minutes Limit details: You’ve used all 5 included reviews currently available. You can run this review on demand instead of waiting. On-demand reviews are free until September 18, 2026. After that, they cost $0.25 per reviewed file.
How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: ready for review. Reproduced both cases on the released bun: the second store entry of a package whose tarball URL in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted hang fix that reuses the existing mark_network_task_failed path and returns the pool slot on the one error branch that leaked it.
What was reviewed:
- Verified all three
for_tarballerror returns (build_url()?,InvalidURL,header_builder.allocate()?) occur beforeunsafe_http_clientis written at NetworkTask.rs:926, so the SAFETY comment onputis accurate. - Confirmed
write_initpopulates every field with a drop-safe value andMaybeUninithas no drop glue, soput'sdrop_in_placeis sound on this path. - Checked that
enqueue_package_for_download/enqueue_tarball_for_downloadgate onnetwork_task_has_failedbefore touchingtask_queue, and every isolated/hoisted caller already handlesAlreadyFailedby releasing its bookkeeping. - Tests are hermetic (local Verdaccio +
Bun.serve, pinned cache dirs), drain pipes concurrently, and assert the specific error message + exit code.
Extended reasoning...
Overview
The PR changes one error path in generate_network_task_for_tarball (src/install/PackageManager/runTasks.rs): instead of propagating for_tarball's error via ?, it explicitly marks the task id failed in network_dedupe_map and returns the vended NetworkTask slot to the pool before returning the error. Two regression tests in test/cli/install/isolated-install.test.ts cover the peer-variant case (two store entries share one tarball) and the manifest-cache case (resolve-phase prefetch fails, install-phase enqueue parks behind it).
Security risks
None. The change is internal error-path bookkeeping in the package manager. It doesn't touch validation, credentials, network parsing, or any user-facing surface. The only unsafe block is a pool put whose SAFETY invariant I verified against for_tarball and write_init — every field is drop-safe at the point put runs, and the MaybeUninit unsafe_http_client has no drop glue.
Level of scrutiny
Moderate — this is native package-manager code with an unsafe block, but the change is 10 lines that reuse two existing helpers (mark_network_task_failed, preallocated_network_tasks.put) in exactly the way they're already used on the download-failure and extract-failure paths in the same file. The fix sits at the layer that creates the dedupe record, so all six callers are covered without per-caller changes. mark_network_task_failed only touches network_dedupe_map, disjoint from the pool slot pointed at by net_ptr, matching the disjointness pattern already relied on by the streaming-setup tail of the same function.
Other factors
The PR description is unusually thorough: it explains the mechanism, why the fix belongs here rather than in the two enqueue callers (the resolve-phase prefetch never touches task_queue), and confirms both tests hang on the released bun and pass with the fix. I verified the AlreadyFailed handling in isolated_install.rs (three sites) and PackageInstaller.rs (three sites) — each releases its slot/tree count. The tests follow harness conventions (Verdaccio registry, tempDir, {...bunEnv}, concurrent pipe drain, using for cleanup) and assert the specific error text and exitCode === 1. The bug hunting system found no issues.
|
Nothing is outstanding from the automated review. It checked the two facts this change rests on: every |
2 participants
Problem
bun installwith the isolated linker hangs forever when a tarball download task cannot be created, for example a tarball URL that is not http. Two cases reach it: the second store entry of the package (peer variants), and any retry of the install once the manifest is in the manifest cache, because the resolve phase already failed the same way.generate_network_task_for_tarball(src/install/PackageManager/runTasks.rs). It records the task id innetwork_dedupe_mapbeforefor_tarballcan fail. A later enqueue for the same id then sees a created task and queues its callback behind it. No task exists, so the callback never runs and the store entry's pending-task slot is never released.NetworkTaskslot was also leaked on that path.Fix
for_tarballerror path, mark the task id failed and return the slot to the pool. Later enqueues getAlreadyFailed, which every caller (isolated, hoisted, runtime auto-install) already handles by releasing its own bookkeeping, and the install exits 1 with the existing error.test/cli/install/isolated-install.test.ts. Both hang on the released bun (the test times out) and pass in under a second with the fix. Also the install suites listed in Notes.Background
task_queuemaps a task id to the callbacks waiting for it.network_dedupe_maprecords which task ids already have a task, so one tarball is downloaded once.failedon that record (install: don't re-download a tarball that already failed #34103) makes later enqueues fail fast withAlreadyFailed.Notes
Repro of the manifest cache case (the realistic one: any failed install caches the manifest, and the retry hangs). Registry manifest with
dist.tarball: "ftp://...", isolated linker. Run 1:error: Expected tarball URL to start with https:// or http://and exit 1 (the error propagates out of the resolve phase). Run 2 within the manifest max-age: the dependency resolves synchronously from the cache, the prefetch fails and is only logged, the install phase enqueues the tarball,has_created_network_tasksays it exists, and the process never exits. With the fix run 2 prints the same error and exits 1. The hoisted linker exits 1 in run 2 before and after.Repro of the peer variant case. Two workspaces depend on
peer-deps@1.0.0with differentno-depsversions. Install, replace the tarball URL ofpeer-depsinbun.lockwithftp://..., removenode_modules, install with an empty cache. Released bun printsInvalidURL: failed to enqueue package for download: peer-deps@1.0.0once and never exits.Why here and not in the callers. An earlier version of this change removed the queued callback in
enqueue_package_for_downloadandenqueue_tarball_for_download. That fixes the peer variant case only. The manifest cache case fails in the resolve-phase prefetch (PackageManagerEnqueue.rs,get_or_put_resolved_package), which never touchestask_queue, so the install phase still parked the entry. Marking the record where it is created covers both. The callbacks that the two enqueue functions queued before the failure stay intask_queue. Nothing reads them: both functions checknetwork_task_has_failedbefore they look at the queue.Slot return. All error returns in
for_tarballhappen before it initializesunsafe_http_client, soput(which drops the rest of theNetworkTask) is the whole cleanup.Suites run with the debug build:
isolated-install(84),bun-install-tarball-integrity,bun-install-retry,bun-add,bun-install-patch,bun-install-git-deps,bun-install(the same 14 failures as on main in this sandbox: they need the public internet or an IPv6 localhost).cargo fmtis clean.Split out of #39640, which keeps the unrelated isolated installer drain.
no test proof · iteration 0 · 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