Repository navigation
fix(ext/node): prevent panic when importing node builtins after module.register() - #33920
Merged
Merged
Conversation
…e.register() When module hooks are active via register(), importing a node: builtin (e.g. node:os) would panic during V8's synchronous module instantiation because the builtin's ext: dependencies were routed through the async hook resolve bridge. Add ext: to is_already_resolved_specifier so internal Deno modules are never sent through user hooks.
fibibot
reviewed
May 8, 2026
fibibot
left a comment
fibibot
left a comment
Contributor
There was a problem hiding this comment.
Substance walk
Clean targeted fix. The bug:
module.register("hooks.mjs", ...)installs the async-hook resolve bridge- Subsequent
import("node:os")triggers V8's syncmodule_resolve_callbackfor the builtin's internal dependencies (e.g.ext:core/mod.js) - The resolver routes those
ext:specifiers through the async-hook bridge — which returns aPromise - V8's sync callback expects a
stringand gets aPromise→ panic
The fix at cli/module_loader.rs:1002 adds "ext:" to the existing is_already_resolved_specifier allowlist (which already includes node:, file:, http://, https://, data:, blob:):
fn is_already_resolved_specifier(specifier: &str) -> bool {
specifier.starts_with("file:")
|| specifier.starts_with("http://")
|| specifier.starts_with("https://")
|| specifier.starts_with("data:")
|| specifier.starts_with("blob:")
|| specifier.starts_with("node:")
+ || specifier.starts_with("ext:")
}ext: URLs are exclusively used by Deno's internal-extension module registry (ext:core/mod.js, ext:deno_node/..., ext:deno_web/... etc.) — none of which user-mode hooks should ever try to intercept. Routing them around the async bridge is correct: they're already-resolved by definition. ✓
Test coverage
Spec test module_register_esm/test_node_builtin_import.mjs reproduces the exact scenario:
import { register } from "node:module";
register("./hooks-basic.mjs", import.meta.url);
const os = await import("node:os");
console.log(typeof os.EOL);Expected string (the actual EOL value '\n'/'\r\n''s type). Without the fix, this panics at the await import("node:os") line. ✓
CI
109 success | 14 in_progress | 0 fail | 1 skip
The 14 in_progress are mostly Windows-aarch64 + a couple slow debug shards. No failures so far on the substantive test paths.
Holding at COMMENT pending CI completion. Substance is APPROVE-quality — minimal, targeted, with a regression test that would fail in its current form against pre-fix code. Will re-confirm with APPROVE once CI lands clean.
2 tasks
bartlomieju
pushed a commit
that referenced
this pull request
May 13, 2026
…builtins (#34006) Fixes #34004. When `module.register()` is active, importing a `node:` built-in (e.g. `node:path`) from an npm package was routed through the JS-side load-hook bridge. The hook chain's `defaultLoad` returns `{ source: null, format: "builtin" }` for `node:` URLs, which falls through to the default Rust loader. That loader doesn't understand the `node:` scheme and surfaces an `Unsupported scheme "node"` error — the exact failure mode reported in #34004 during `vite build` of a Fresh app that transitively calls `register()` via `@tailwindcss/postcss`. The resolve path already short-circuits `node:` / `ext:` specifiers via `is_already_resolved_specifier` (added in #33920). This applies the same treatment to the load path: built-in modules are served by the runtime's module map and must never be sent through user-supplied hooks. `is_builtin_module_specifier` is factored out so the two call sites share the exact same predicate. Co-authored-by: lunadogbot <[email protected]>
littledivy
pushed a commit
to crowlKats/deno
that referenced
this pull request
Jun 10, 2026
…e.register() (denoland#33920) - When module hooks are active via `register()`, importing a `node:` builtin (e.g. `node:os`) panicked during V8's synchronous module instantiation because the builtin's `ext:` dependencies (like `ext:core/mod.js`) were routed through the async hook resolve bridge - Add `ext:` to `is_already_resolved_specifier` so internal Deno modules are never sent through user module hooks - Add a regression test that imports `node:os` after `register()`
littledivy
pushed a commit
to crowlKats/deno
that referenced
this pull request
Jun 10, 2026
…builtins (denoland#34006) Fixes denoland#34004. When `module.register()` is active, importing a `node:` built-in (e.g. `node:path`) from an npm package was routed through the JS-side load-hook bridge. The hook chain's `defaultLoad` returns `{ source: null, format: "builtin" }` for `node:` URLs, which falls through to the default Rust loader. That loader doesn't understand the `node:` scheme and surfaces an `Unsupported scheme "node"` error — the exact failure mode reported in denoland#34004 during `vite build` of a Fresh app that transitively calls `register()` via `@tailwindcss/postcss`. The resolve path already short-circuits `node:` / `ext:` specifiers via `is_already_resolved_specifier` (added in denoland#33920). This applies the same treatment to the load path: built-in modules are served by the runtime's module map and must never be sent through user-supplied hooks. `is_builtin_module_specifier` is factored out so the two call sites share the exact same predicate. Co-authored-by: lunadogbot <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
register(), importing anode:builtin (e.g.node:os) panicked during V8's synchronous module instantiation because the builtin'sext:dependencies (likeext:core/mod.js) were routed through the async hook resolve bridgeext:tois_already_resolved_specifierso internal Deno modules are never sent through user module hooksnode:osafterregister()Test plan
node_builtin_import_after_registerintests/specs/node/module_register_esm/module_register_esmspec tests still pass