Conversation
|
Warning Review limit reached
On-demand reviews are free for the next 27 days. After that, they cost $0.25 per reviewed file. Or wait 32 minutes for your next included review. View limit detailsLimit details: You’ve used the included review currently available. Your 75 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (10)
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 (10)
💤 Files with no reviewable changes (1)
Included review availability: Your plan includes up to 5 reviews per rolling hour; 0 remain after this review. WalkthroughThe PR replaces dependency failure callbacks with explicit root-resolution handling, consolidates wake-handler state into ChangesResolver wake and queue flow
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the problem, implementation, behavioral impact, background, tests, and verification results. Although it does not use the template headings exactly, it provides the required information in equivalent sections. Comment |
|
Reproduced on bun 1.4.0 canary (linux x64) with a local registry whose manifest only has 1.0.0: Fix is in this PR (second commit; the first is #38121, which this stacks on). Tests: |
![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. Since it removes an error-callback mechanism across the install/resolver/JSC boundary and is stacked on #38121 (not yet landed), a human look at the design choice would still be worthwhile.
What was reviewed:
- Confirmed
is_rootis equivalent to the removedfail_fn.is_some()at all three call sites, so the catch-all branch preservesbun install's error propagation and the four specific error branches are unchanged for it. - Verified nothing ever populates
ParseResult::pending_imports, soQueue::mapis always empty and the removedon_dependency_errorpath was dead; the newon_pollearly-return cannot strand tasks. - Grepped for
fail_fn/FailFn/fail_root_resolution/on_dependency_error/queue_from_wake_context— no dangling references remain. - Checked the note-dedup loop in
VirtualMachine.rs: skips the resolve message by index, filters to Err/Warn, and dedups against both original notes and previously-added ones.
Extended reasoning...
Overview
This PR (stacked on #38121) makes runtime auto-install report why a root dependency failed to resolve, matching what bun install already prints. It touches 7 source files: PackageManagerEnqueue.rs (removes fail_fn parameter, makes four error branches log unconditionally, adds an is_root catch-all that logs-and-swallows instead of silently swallowing), PackageManager.rs / processDependencyList.rs / resolver_hooks.rs / jsc_hooks.rs / AsyncModule.rs (remove the now-dead FailFn / fail_root_resolution / WakeHandler::on_dependency_error / Queue::on_dependency_error / queue_from_wake_context callback chain), and VirtualMachine.rs (attaches per-resolve log entries as notes on the ResolveMessage, deduping by text so the retry doesn't double them). Two test files gain 9 new subprocess tests plus a snapshot test.
Security risks
None identified. This is diagnostic-message plumbing on the failure path; no new inputs are parsed, no allocation sizes derive from untrusted data, and no auth/crypto paths are touched.
Level of scrutiny
Medium-high. The change spans the package manager's error handling (which bun install shares), the JSC event-loop wake handler, and module resolution. I verified the load-bearing invariants: (1) all three callers of enqueue_dependency_with_main_and_success_fn pass is_root exactly where they previously passed Some(fail_root_resolution), so bun install's propagate-vs-swallow behavior is unchanged; (2) pending_imports is only ever Default::default() across the codebase, so Queue::enqueue (gated on pending_imports.len() > 0) is never reached, map stays empty, and both the removed callback and the new on_poll early-return are safe; (3) a repo-wide grep for the removed identifiers finds nothing.
Other factors
Test coverage is thorough — 5 new cases covering missing version / dist-tag / minimumReleaseAge (both exact-version and dist-tag), each asserting the exact set of note: lines to also pin "exactly once", plus ESM/CJS variants and the #38121 tarball/manifest 404 cases. The PR description explicitly ran the bun install tests that assert the same messages. What keeps me from approving outright: this is a design-level change (log-through-pm.log instead of callback delivery) that the description says was one of two suggested approaches, it deletes a cross-crate callback mechanism, and it's stacked on an unmerged base PR. A maintainer should confirm the approach and the base landing.
|
Updated 9:02 PM PT - Aug 24th, 2026
✅ @autofix-ci[bot], your commit abc7069cfbe739e89def58f5e470ef27aaa6e373 passed in 🧪 To try this PR locally: bunx bun-pr 38180That installs a local version of the PR into your bun-38180 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this and didn't find bugs. It's stacked on #38121 and reworks error propagation across the install→resolver→VM boundary (drops the fail_fn/on_dependency_error callback path, makes the four resolve-error branches log unconditionally, and has the catch-all log-and-continue for root deps instead of calling a dead callback), so a human pass is still worthwhile.
What was reviewed:
- Confirmed
FailFn/fail_root_resolution/on_dependency_error/queue_from_wake_contexthave no remaining references after removal. - The
is_rootcatch-all inPackageManagerEnqueue.rs: non-root callers still getErr(err)propagated, sobun installbehavior is unchanged; only root deps now log<Err> while resolving package …and returnOk. - The note-attachment loop in
VirtualMachine.rsskips the resolve message's own index and dedups by text — covers the retry-logs-twice case the tests pin via exactnoteLinesarrays. - New tests use local
Bun.serveregistries,tempDir, per-test cache dirs, and assert exact note lists (so "exactly once" is enforced).
Extended reasoning...
Overview
Stacked on #38121 (first commit); this PR's own commit removes the fail_fn: Option<FailFn> parameter from enqueue_dependency_with_main_and_success_fn and everything hanging off it (FailFn type, PackageManager::fail_root_resolution, WakeHandler::on_dependency_error + its getter, Queue::on_dependency_error, Queue::queue_from_wake_context, and the adapter registration in runtime/jsc_hooks.rs). The four specific error branches (DistTagNotFound, NoMatchingVersion, TooRecentVersion, MissingPackageJSON) now log unconditionally instead of only when no fail_fn was passed, and the catch-all branch gates on the pre-existing is_root flag: root deps log <Err> while resolving package "x" and stay unresolved; non-root still returns Err. The #38121 portion adds the note-attachment/dedup loop in VirtualMachine::_resolve and the on_poll empty-map early return. Six new tests in run-autoinstall.test.ts cover missing version, missing dist-tag, both minimumReleaseAge variants, and the catch-all, plus one snapshot test in resolve-error.test.ts.
Security risks
None identified. The change is diagnostic-only: it surfaces error text that bun install already prints into the runtime's ResolveMessage. No new input parsing, no auth/crypto/permissions surface.
Level of scrutiny
Moderate-to-high. The diff itself is mostly deletion of a dead callback path plus reindented existing message code, but it (a) is stacked on an unlanded PR so the reviewable unit here is the combined diff, (b) changes the return value of the catch-all error branch for root dependencies from a no-op callback+Ok to log+Ok (semantically equivalent given the callback walked an always-empty map, but that claim needs a human to confirm nothing else populates Queue::map on this path), and (c) touches on_poll's task-drain timing. These are the kinds of cross-subsystem control-flow changes where a maintainer familiar with the auto-install lifecycle should sign off.
Other factors
Grep confirms no remaining references to the removed symbols. All three call sites of enqueue_dependency_with_main_and_success_fn are updated in the diff. Test coverage is thorough (local registries, exact note: line arrays that also pin dedup, ESM+CJS variants). There are six open comment-cop inline flags on comment length that the author will see directly.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It's a substantive change to error routing across the install/resolver/runtime boundary (removes the fail_fn/on_dependency_error callback path, reshapes WakeHandler, adds an early return in Queue::on_poll, and changes how resolve-log entries become ResolveMessage notes), stacked on #38121, so a human look would still be worthwhile.
What was reviewed:
- Confirmed
bun installbehaviour is unchanged: it never passed afail_fn, so the four message branches and the finalreturn Err(err)arm are the same paths it took before; onlyis_rootcallers now log-and-return-Ok in the catch-all. - Checked that removing
on_dependency_error/queue_from_wake_contextleaves no other callers, and that theWakeHandler→WakeHandler(Option<WakeTarget>)reshaping keepswake_rawsemantically identical. - Traced the notes-dedup loop in
VirtualMachine.rs: theresolve_msg_indexskip and text-equality check correctly avoid double notes from the retry without dropping the first attempt's 404 lines. - Tests are hermetic (local
Bun.serveregistries, per-test cache dir) and pin the exact set ofnote:lines, so both "line missing" and "line duplicated" would fail.
Extended reasoning...
Overview
This PR makes runtime auto-install surface the same per-dependency error messages bun install prints ("No version matching …", "tag not found", "blocked by minimum-release-age", etc.) as note: lines on the ResolveMessage. Mechanically it (a) removes the FailFn/fail_root_resolution callback path so enqueue_dependency_with_main_and_success_fn logs unconditionally, (b) adds an is_root catch-all branch that logs the error name instead of propagating, (c) deletes the now-dead WakeHandler::on_dependency_error / Queue::on_dependency_error / queue_from_wake_context machinery and collapses WakeHandler to a single Option<WakeTarget>, (d) has VirtualMachine::_resolve attach the per-resolve log's Err/Warn entries as notes with text-based dedup, and (e) makes Queue::on_poll early-return when map is empty. The first commit is #38121; this PR is stacked on it.
Security risks
None identified. The change is diagnostic-only (which lines end up on a resolve error). No auth, crypto, path handling, or untrusted-input parsing is touched. The removed callback path went through an always-empty queue, so no live behaviour is being disabled.
Level of scrutiny
Medium-high. The individual edits are small, but they span four crates (bun_install, bun_install_types, bun_jsc, bun_runtime) and change how errors flow from the package manager into the runtime. The is_root catch-all now returns Ok(()) where a non-root caller would return Err(err); the PR argues this is fine because root enqueues previously swallowed the error via a no-op callback anyway, and the description traces that carefully — but it's the kind of invariant a maintainer who owns install/ should confirm. The on_poll early return relies on the claim that nothing populates Queue::map on the synchronous auto-install path today. It's also stacked on an unmerged PR.
Other factors
- Test coverage is thorough: 6 new cases covering missing version / dist-tag / age-gated (both flavours) / catch-all, plus ESM vs
require(), each asserting the fullnote:line set (so dedup regressions would fail). All hermetic via localBun.serveregistries. - There are several unresolved comment-cop bot flags on multi-line comments in the touched files (e.g.
resolver_hooks.rs:1296/1304,AsyncModule.rs:76/343,VirtualMachine.rs:4662,PackageManagerEnqueue.rs:640). Most look like legitimate short doc comments rather than workaround justifications, but the author/maintainer should decide whether to resolve or trim them. - No prior
claude[bot]review on this PR.
2d5a29f to
1006c72
Compare
1006c72 to
b131b6d
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It's stacked on #38121 and reworks error propagation across the package manager, WakeHandler, and AsyncModule::Queue, including dead-code removal predicated on Queue::map always being empty and a new early-return in on_poll — a human look at those assumptions would be worthwhile.
What was reviewed:
- Confirmed nothing populates
ParseResult::pending_imports, soQueue::mapstays empty and the removedfail_root_resolution/on_dependency_errorpath was unreachable. - Verified all three
enqueue_dependency_with_main_and_success_fncallers were updated for the droppedfail_fnparam, andbun install(is_root=false) still returnsErr(err)on the catch-all branch. - Checked the
WakeHandler→Option<WakeTarget>refactor is applied consistently at every read/write site (wake_raw,jsc_hooks,resolver.rs,auto_installer.rs). - Traced
enqueue_dependency_to_root'ssleep_untilloop drains tasks itself, so theon_pollearly-return doesn't strand work whenmapis empty.
Extended reasoning...
Overview
This PR (stacked on #38121) makes runtime auto-install report why a root dependency failed to resolve, matching what bun install already prints. It touches 8 source files: removes the FailFn/fail_root_resolution/WakeHandler::on_dependency_error machinery (dead — it reported through AsyncModule::Queue::map, which nothing populates), collapses WakeHandler from three parallel Option fields into a newtype over Option<WakeTarget>, makes the four error-message branches in enqueue_dependency_with_main_and_success_fn log unconditionally, adds an is_root catch-all log line, adds an if map.is_empty() { return } guard to Queue::on_poll, and (from #38121) attaches per-resolve log entries as deduped notes on the ResolveMessage. Two test files gain 11 new tests using local Bun.serve registries.
Security risks
None identified. The change is diagnostic-only on the failure path; no new inputs are trusted, no auth/crypto/permission code is touched, and the test registries bind to 127.0.0.1:0.
Level of scrutiny
Medium-high. While the observable change is "better error messages", the mechanism removes code across the install/resolver/JSC boundary on the strength of a dead-code analysis (pending_imports is never pushed to → Queue::map is always empty → on_dependency_error reported to nobody). I verified that analysis by grep and it holds, but the on_poll early-return also changes behavior for the case where a wake-posted PollPendingModulesTask runs while (or after) a synchronous enqueue_dependency_to_root waiter is draining — the PR's reasoning that the sync waiter handles this itself checks out against sleep_until's run_tasks loop, but it's the kind of cross-thread/event-loop interaction a maintainer should sign off on.
Other factors
- Stacked on #38121, so this diff carries both PRs' changes; approval here shouldn't precede that PR.
- The
WakeHandlernewtype refactor tightens the type (context+handler are now jointly optional), which is a nice cleanup and applied consistently, but is a public-shape change tobun_install_types. - Test coverage is strong: each error branch (missing version, missing dist-tag, both
minimumReleaseAgeshapes, catch-allInvalidURL) is asserted with the exactnote:line and "exactly once" vianoteLines(...)equality; ESM, CJS and dynamic-import variants covered; plus a snapshot test for the resolver-log-as-notes path. - All comment-cop threads are resolved; the surviving comments in the diff are short and load-bearing.
…eloader, and the resolver `ParseResult.pending_imports` is never written, so the auto-install pending-module queue in AsyncModule.rs never receives a module. Remove the queue, the `Pending` variants it was fed through (resolver, install, install_types), the task tag, the error variants, and the helpers that only served it. The wake handler that drains package manager tasks on the JS thread stays, and so does the dependency-error callback chain, which #38180 reworks. Also remove the react_compiler logger and rename scaffolding that nothing reads, the hot-reload task's per-file hashes and paths that nothing reads, and a few items that became unreferenced with them.
b131b6d to
e12a757
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it removes the fail_fn/on_dependency_error machinery on the premise that Queue::map is always empty (and gates on_poll on that same invariant), plus reshapes WakeHandler across crates, a human look would still be worthwhile.
What was reviewed:
- Verified nothing populates
ParseResult::pending_imports(onlyDefault::default()initializers; linker only reads it), somodules.enqueueis unreachable andQueue::mapstays empty — the removedon_dependency_errorwas indeed dead and the newon_pollearly-return is safe. - Grepped for all uses of
FailFn/fail_root_resolution/get_handler/on_dependency_error/queue_from_wake_contextandWakeHandlerfield accesses — none remain outside the diff. - Checked the
is_rootcatch-all branch inenqueue_dependency_with_main_and_success_fn:bun installstill getsErr(err)(never passesis_root=true), root deps now log and stay unresolved as before.
Extended reasoning...
Overview
Stacked on #38121. Makes runtime auto-install report why a root dependency failed to resolve (missing version, missing dist-tag, blocked by minimumReleaseAge, or a generic error name) as note: lines on the ResolveMessage, matching what bun install already prints. To do this it:
- Removes the
fail_fn: Option<FailFn>parameter fromenqueue_dependency_with_main_and_success_fnso the four error branches log unconditionally, and adds anis_rootcatch-all that logs<Err> while resolving package "x"instead of propagating. - Deletes the now-unused
FailFntype,PackageManager::fail_root_resolution,WakeHandler::on_dependency_error+ accessors,Queue::on_dependency_error, andQueue::queue_from_wake_context. - Restructures
WakeHandlerfrom a struct-of-three-Options toWakeHandler(Option<WakeTarget>)withWakeTarget { context, handler }, updatingwake_rawand the registration inruntime/jsc_hooks.rs. - Adds an early return to
Queue::on_pollwhenmapis empty, and (from #38121) attaches per-resolve log entries as deduped notes on theResolveMessageinVirtualMachine.rs. - Adds 10+ tests with local
Bun.serveregistries covering tarball/manifest 404s, missing versions/tags,minimumReleaseAge, and the generic error path; plus a snapshot test for a badpackage.json.
Security risks
None identified. Error-message plumbing only; no new parsing of untrusted input, no auth/crypto/permissions changes. The registry inputs that reach the new log lines were already being parsed before this PR.
Level of scrutiny
Medium-high. This is not a mechanical change: it removes ~100 lines of cross-crate callback machinery (FailFn, on_dependency_error, WakeHandler accessors) on the architectural premise that Queue::map is always empty because nothing populates ParseResult::pending_imports. I verified this by grepping — every pending_imports initializer is Default::default() and the linker only reads it — but a maintainer should confirm that assumption and that the async-module queue path is intentionally dead. The on_poll early-return is a real behavior change whose safety rests on the synchronous enqueue_dependency_to_root waiter draining tasks itself. The WakeHandler → WakeTarget reshaping touches four crates.
Other factors
- Stacked on #38121, which has not landed; the first commit here is that PR's commit.
- All comment-cop bot comments (about paragraph-long comments) are resolved; the latest commit (
e12a757e) addressed them. - Test coverage is thorough: each new error branch has a dedicated test asserting the exact
note:line list (which also pins "exactly once" against the retry-duplication bug), across static import,require(), and dynamicimport(). - The dedup in
VirtualMachine.rsis O(n²) overnotes×log.msgs, but both are tiny (a handful of entries per failed resolve). - No CODEOWNERS or outstanding human reviewer comments.
When a bare import is auto-installed and the tarball download (or the manifest request, or the extraction) fails, the only output was "Unexpected while resolving package 'x' from '...'". The package manager had written "GET <tarball url> - 404" into the log of the resolve that triggered it, but resolve_maybe_needs_trailing_slash only kept the resolve-tagged message and dropped the rest of that log. Attach the remaining error and warning entries to the ResolveMessage as notes, so they are printed under the error (and by console.log of the error). The same entries were also lost about half of the time before reaching the log: the PackageManager wakes AsyncModule::Queue on every completed task, and Queue::on_poll drained the completions the synchronous waiter in enqueue_dependency_to_root was blocked on. Its callbacks deliver failures to pending modules, and nothing populates that queue any more, so the failures were discarded. Skip the drain when no module is waiting.
When runtime auto-install failed to resolve a root dependency because the requested version or dist-tag does not exist (or is blocked by minimumReleaseAge), the package manager handed the error to fail_root_resolution, which forwarded it to the AsyncModule queue. That queue is always empty, so the error was discarded and the import failed with a bare "Cannot find package". Log the same line `bun install` prints for root dependencies too, and remove the callback plumbing (FailFn, fail_root_resolution, WakeHandler::on_dependency_error, AsyncModule::Queue::on_dependency_error) that only fed the empty queue. The logged line reaches the user through the notes the runtime now attaches to the ResolveMessage. A failed resolve is retried once after busting the directory cache, and the retry derives the same failure from the manifest again, so identical lines are attached once.
WakeHandler's two parallel Options collapse into one Option of a (context, handler) pair, removing the unwrap in get_handler. The SuccessFn aliasing consts go away; call sites name the methods via the type path directly. Trim the remaining long comments.
e12a757 to
bd88ca1
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Since it spans the package-manager error path, the resolver's ResolveMessage construction, and a cross-crate WakeHandler refactor — and is stacked on #38121 — a human look would still be worthwhile.
Checked: all three enqueue_dependency_with_main_and_success_fn call sites updated; no remaining references to FailFn/fail_root_resolution/on_dependency_error/get_handler(). Verified the removed on_dependency_error path was dead — Queue::map is only populated when parse_result.pending_imports is non-empty, and nothing writes to that vec. bun install behavior preserved: it never passed fail_fn, so the now-unconditional log arms and the is_root-gated catch-all match its prior branches. WakeHandler consumers (resolver.rs, auto_installer.rs, wake_raw) all agree with the new tuple shape.
Extended reasoning...
Overview
This PR (stacked on #38121) makes runtime auto-install report why a root dependency could not be resolved, matching what bun install already prints. It removes the fail_fn/FailFn callback plumbing from enqueue_dependency_with_main_and_success_fn, along with PackageManager::fail_root_resolution, WakeHandler::on_dependency_error, and Queue::on_dependency_error/queue_from_wake_context. The four specific error branches (DistTagNotFound, NoMatchingVersion, TooRecentVersion, MissingPackageJSON) now log unconditionally; a new is_root catch-all logs <Err> while resolving package "x" for other errors and returns Ok(()) (matching the prior fail_fn early-return). WakeHandler is refactored from three parallel Option fields to WakeHandler(Option<WakeTarget>), encoding the "context ⇔ handler" invariant in the type. The stacked #38121 commit attaches per-resolve log entries to the ResolveMessage as deduped notes and adds an early-return in Queue::on_poll when the queue is empty.
Security risks
None. This is diagnostic-message plumbing on the error path; no new inputs are parsed and no security checks are touched.
Level of scrutiny
Moderate-to-high. The change touches the package-manager enqueue error path shared with bun install, refactors a type across three crates (bun_install_types, bun_install, bun_resolver, bun_runtime), and removes code on the strength of a "this callback path is dead" argument. I verified that argument holds (nothing writes to pending_imports, so Queue::enqueue is never called, so Queue::map is always empty and on_dependency_error was a no-op), and that all WakeHandler consumers and all enqueue_dependency_with_main_and_success_fn callers agree with the new signatures. The peer-dependency warn_unmet_peer_dependency arm added in d1d256c is preserved after the rebase.
Other factors
The PR is stacked on unmerged #38121, so part of the diff (VirtualMachine.rs notes loop, on_poll early-return, the first test describe block, resolve-error.test.ts) belongs to that PR and will drop out on rebase. Test coverage is thorough — 6 new cases pin exact note: output including the exactly-once dedup — and the description documents fail-before/pass-after verification. Given the cross-crate scope, the dead-code removal claim, and the stacking, a human reviewer should confirm the design choice (log-then-Ok(()) for root vs. propagate for install) and the on_poll early-return before merge.
This branch has not been deployed
1 participant
Stacked on #38121: the first commit here is that PR's commit, this PR is the commits after it. This will be rebased once #38121 lands.
Rebased onto main after d1d256c, which added a
warn_unmet_peer_dependencyarm to the sameDistTagNotFound/NoMatchingVersionbranches this PR edits. Resolved by keeping that peer arm and dropping only thefail_fnarm, so unmet peers still warn and everything else logs the error as described below. A later rebase re-seated the notes logic onto main's reworkedresolve_maybe_needs_trailing_slash, which now returns theResolveMessageinstead of writing anErrorableStringout-param. The notes themselves are unchanged.Problem
node_modules), importing a package whose manifest exists but whose requested version, dist-tag, or age-filtered version does not, prints onlyerror: Cannot find package 'pkg@2.0.0' from '/app/index.js'.bun installfor the same request printsNo version matching "2.0.0" found for specifier "pkg" (but package exists); the runtime says nothing about the version. Same forpkg@canary(Package "pkg" with tag "canary" not found, but package exists) and for versions blocked byminimumReleaseAge.enqueue_dependency_with_main_and_success_fn(src/install/PackageManager/PackageManagerEnqueue.rs:797) only wrote those messages when nofail_fnwas passed. The only callers passing one are the two root-dependency paths runtime auto-install uses (enqueue_dependency_to_root, and theRootDependencyarm inprocessDependencyList.rs), and thefail_fnwasPackageManager::fail_root_resolution(src/install/PackageManager.rs:959), which forwarded the error toAsyncModule::Queue::on_dependency_error. That walksQueue::map, which is always empty (nothing populatesParseResult::pending_importsany more), so root-dependency errors were discarded andbun installwas the only caller that ever logged them.ResolveMessageas notes. As written there, each of these lines would show up twice:VirtualMachine::_resolveretries a failed resolve once after busting the directory cache, and the retry re-derives the same failure from the manifest that is already in memory and logs it again (the tarball/manifest 404 cases in resolver: report why auto-install failed on the ResolveMessage #38121 are not affected because the retry does not re-download, so nothing is logged twice there).Fix
fail_fn/FailFn/fail_root_resolutionare removed along withWakeHandler::on_dependency_error, its registration inruntime/jsc_hooks.rs, andQueue::on_dependency_error/queue_from_wake_context, which had no other callers. The catch-all branch uses the existingis_rootflag: root dependencies log<Err> while resolving package "x"and stay unresolved (the same outcome as before, minus the silence),bun installkeeps propagating the error. The callee has usually logged something more specific by then (for a manifest whose tarball field is not a URL:Expected tarball URL to start with https:// or http://, ...), so this line is the counterpart of the'main' returned error.InvalidURLlinebun installprints after that message, and the fallback for a callee that logs nothing.bun installis unaffected: it never passed afail_fn, so the branches it takes are unchanged. The runtime already pointedpm.logat the per-resolve log for the duration of the resolve, so logging is the channel that reaches the import's error; this is the first of the two approaches suggested when this was handed off, and it makes the runtime print the linebun installalready prints instead of inventing a second wording.test/cli/run/run-autoinstall.test.ts, "auto-install reports why a package could not be resolved": missing version (static import andrequire()), missing dist-tag, exact version blocked byminimumReleaseAge, dist-tag with every version blocked, and the catch-all branch (manifest whose tarball field is not a URL). Each asserts the full list ofnote:lines, so it also pins "exactly once". All 6 fail on bun 1.4.0 canary and on resolver: report why auto-install failed on the ResolveMessage #38121 alone (the root dependency's line is never logged), and the first 5 fail with the install-side change but without the text check (each note twice); all pass withbun bd testtogether with resolver: report why auto-install failed on the ResolveMessage #38121's 4 tests and the rest of the file.bun bd testontest/cli/install/minimum-release-age.test.ts(49 pass), thebun-install.test.tscases asserting the(but package exists)message,test/js/bun/resolve/(incl. resolver: report why auto-install failed on the ResolveMessage #38121's snapshot test),test/js/node/missing-module.test.js,test/cli/run/tsconfig-override.test.ts: pass.cargo fmt --checkandcargo clippyonbun_install_types,bun_install,bun_jsc,bun_runtime: clean.Background
node_modules, the resolver asks the in-processPackageManagerto resolve the package.enqueue_dependency_to_rootadds it as a dependency of the lockfile root, enqueues it, and blocks until the package manager has no pending tasks; if the dependency still has no resolution afterwards the resolver reports the import as not found. "Root dependency" in the install code means exactly these entries;bun installnever creates them.pm.logis abun_ast::Logthe package manager appends its diagnostics to. The CLI prints it at the end ofbun install; the runtime swaps in a freshLogper resolve (VirtualMachine::resolve_maybe_needs_trailing_slash) and, with resolver: report why auto-install failed on the ResolveMessage #38121, attaches its entries to theResolveMessageasnote:lines.AsyncModule::Queueis the remnant of an older asynchronous auto-install design: a list of modules whose imports are being installed, with callbacks that deliver per-package failures to those modules. Nothing adds modules to it today, so a callback that reports through it reports to nobody.fail_root_resolutionwas such a callback; this PR removes it and leaves the rest of the queue alone (resolver: report why auto-install failed on the ResolveMessage #38121 adjustson_poll).minimumReleaseAgemessage for ranges (install: print version before package name in the minimum-release-age error #37895), and a separate bug found while writing the tests, where auto-install ignores the version range in the project's own package.json and resolveslatest(handed off separately). The test for the range case therefore imports an explicit version.Output before / after (local registry whose manifest for the package only has 1.0.0)
Before (identical with #38121 alone, since the line was never logged):
After:
With
minimumReleaseAge = 86400in bunfig.toml and a package published today:no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/resolve/resolve-error.test.ts