feat(bundler): lazy-external network stack for V8 snapshots#5999
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughSnapshot bundling now resolves configurable lazy modules, keeps those ids external, injects lazy dispatch into ChangesSnapshot lazy externalization
Sequence Diagram(s)sequenceDiagram
participant BundlerRun as Bundler.run
participant Prelude as prelude.ts
participant OutputPatch as `#applySnapshotPrelude`
participant EmittedJS as emitted .js files
BundlerRun->>Prelude: resolveSnapshotLazyModules(baseDir)
BundlerRun->>BundlerRun: extend externalsMap with snapshotLazyModules
BundlerRun->>OutputPatch: patch emitted .js files
OutputPatch->>Prelude: injectExternalRequireLazyHook(content)
OutputPatch->>Prelude: prependSnapshotPrelude(source, snapshotLazyModules)
OutputPatch->>EmittedJS: write patched output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: private package registry requires authentication. Disable ESLint in CodeRabbit settings or use public packages. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements a lazy-external and native-binding stub mechanism to make V8 startup snapshots serializable. It introduces a prelude that deletes Node's lazy web globals and installs a Proxy-based stub for network-related modules (like http, https, tls, and dns) at build time, which later forwards to the real modules at restore time. Feedback was provided regarding the Proxy's structural traps (ownKeys and getOwnPropertyDescriptor) in the prelude; currently, they delegate entirely to the dummy target function even at restore time, which hides the real module's exports and breaks destructuring. It is recommended to delegate these traps to the real module when it is available.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+320
to
+325
| ownKeys: function (target) { | ||
| return Reflect.ownKeys(target); | ||
| }, | ||
| getOwnPropertyDescriptor: function (target, prop) { | ||
| return Reflect.getOwnPropertyDescriptor(target, prop); | ||
| }, |
Contributor
There was a problem hiding this comment.
At restore time (when globalThis.__RUNTIME_REQUIRE is active and the real module is loaded), the Proxy currently delegates structural traps like ownKeys and getOwnPropertyDescriptor entirely to the dummy target (the empty function).
This leads to several critical runtime issues:
- Hidden Exports:
Object.keys(proxy)orReflect.ownKeys(proxy)will only return the keys of the dummy function (e.g.,['length', 'name', 'prototype']), completely hiding all the actual exported properties and methods of the real module (likecreateServerforhttp). - Missing Descriptors:
Object.getOwnPropertyDescriptor(proxy, 'createServer')will returnundefinedfor all real module properties. - Destructuring Failures: Destructuring with rest properties (e.g.,
const { createServer, ...rest } = require('http')) or any library/framework utility that iterates over the module's exports (such as auto-loaders, decorators, or test mocks) will fail or behave incorrectly because the structural traps do not reflect the real module's properties.
To fix this while still satisfying the Proxy invariants (which require that all non-configurable properties of the target are reported), we should delegate these traps to the real module when it is available, while merging/checking against the target's non-configurable properties.
ownKeys: function (target) {
var real = realModule();
if (real !== undefined && real !== null) {
var keys = Reflect.ownKeys(real);
var targetKeys = Reflect.ownKeys(target);
for (var i = 0; i < targetKeys.length; i++) {
var k = targetKeys[i];
if (keys.indexOf(k) === -1) {
keys.push(k);
}
}
return keys;
}
return Reflect.ownKeys(target);
},
getOwnPropertyDescriptor: function (target, prop) {
var real = realModule();
if (real !== undefined && real !== null) {
var desc = Reflect.getOwnPropertyDescriptor(target, prop);
if (desc && !desc.configurable) {
return desc;
}
return Reflect.getOwnPropertyDescriptor(real, prop);
}
return Reflect.getOwnPropertyDescriptor(target, prop);
},
Contributor
Author
There was a problem hiding this comment.
Good catch — fixed in e97acbe. At restore time ownKeys now returns the real module's keys merged with the target's own (non-configurable) keys, and getOwnPropertyDescriptor delegates to the real module (forcing configurable: true on real descriptors to avoid the 'non-configurable prop absent from target' invariant violation), while still reporting the target's own non-configurable prototype. Added a test asserting Object.keys(http) / destructuring-rest see the real exports after __RUNTIME_REQUIRE.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/egg-bundler/src/lib/Bundler.ts`:
- Around line 525-529: Treat injectedCount === 0 in Bundler.ts as ambiguous: if
the bundle text still contains externalRequire, it means
injectExternalRequireLazyHook failed to patch the helper rather than no helper
being emitted. Update the post-injection check near
injectExternalRequireLazyHook to distinguish these cases, and fail the build
with a clear error when externalRequire is present but no lazy hook was
injected; keep the existing debug-only path only for bundles that truly do not
contain externalRequire.
In `@tools/egg-bundler/src/lib/prelude.ts`:
- Around line 283-301: The build-time lazy proxy in prelude.ts is accidentally
thenable because the generic fallback in the proxy get trap returns the proxy
for arbitrary properties, which makes then callable. Update the proxy behavior
in the lazy module handler so that accessing then returns undefined (or
otherwise avoids exposing a callable then method) while preserving the existing
realModule() and buildConst() behavior and the invariant handling for default,
__esModule, symbol properties, and prototype/name/length.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a8f9fd73-70d7-462a-ae7f-7364c2f48deb
📒 Files selected for processing (6)
tools/egg-bundler/src/index.tstools/egg-bundler/src/lib/Bundler.tstools/egg-bundler/src/lib/prelude.tstools/egg-bundler/test/snapshot-lazy-bundler.test.tstools/egg-bundler/test/snapshot-lazy-external.test.tstools/egg-bundler/test/snapshot-lazy.realbuild.test.ts
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5999 +/- ##
=======================================
Coverage 84.88% 84.88%
=======================================
Files 674 674
Lines 20261 20261
Branches 4037 4037
=======================================
Hits 17199 17199
Misses 2631 2631
Partials 431 431 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
killagu
force-pushed
the
feat/bundler-snapshot-lazy-external
branch
2 times, most recently
from
90c4814 to
e97acbe
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tools/egg-bundler/test/snapshot-lazy.realbuild.test.ts (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename this test file to use hyphens in the basename.
Consider
snapshot-lazy-realbuild.test.tsso the new file follows the repository naming rule. As per coding guidelines,**/*: Keep file names lowercase with hyphens.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/egg-bundler/test/snapshot-lazy.realbuild.test.ts` at line 1, The test file basename currently uses an underscore-style separator and does not follow the repository’s lowercase-hyphen naming rule. Rename the snapshot lazy realbuild test file to use hyphens in the basename, matching the existing test identity while keeping the `snapshot-lazy-realbuild.test.ts` style consistent with the project’s naming convention.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/egg-bundler/src/lib/Bundler.ts`:
- Around line 531-540: In Bundler.injectExternalRequireLazyHook, the failure
path incorrectly treats already-patched output as an injection mismatch because
injectedCount stays 0 even when the lazy hook is already present. Update the
guard around the externalRequire handling to detect the existing lazy dispatch
separately from a genuine regex/signature miss, and only throw when the helper
was emitted without any existing or newly injected lazy hook. Use the
externalRequire helper detection and the injectedCount/sawExternalRequire checks
to avoid re-throwing on reruns over already-injected bundles.
In `@tools/egg-bundler/test/snapshot-lazy-bundler.test.ts`:
- Around line 119-120: The prelude-order assertion in
snapshot-lazy-bundler.test.ts is currently too permissive because a missing
globalThis.__LAZY_EXT = new Set( still makes indexOf() return -1 and pass the
comparison. Update the test around the worker assertion to fail closed by first
asserting that the __LAZY_EXT initialization exists in worker, then checking
that it appears before __UTOOPACK__ so the test catches regressions in the
prelude setup.
---
Nitpick comments:
In `@tools/egg-bundler/test/snapshot-lazy.realbuild.test.ts`:
- Line 1: The test file basename currently uses an underscore-style separator
and does not follow the repository’s lowercase-hyphen naming rule. Rename the
snapshot lazy realbuild test file to use hyphens in the basename, matching the
existing test identity while keeping the `snapshot-lazy-realbuild.test.ts` style
consistent with the project’s naming convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6d62f3f8-6a1d-4a26-8da2-f168d9870120
📒 Files selected for processing (6)
tools/egg-bundler/src/index.tstools/egg-bundler/src/lib/Bundler.tstools/egg-bundler/src/lib/prelude.tstools/egg-bundler/test/snapshot-lazy-bundler.test.tstools/egg-bundler/test/snapshot-lazy-external.test.tstools/egg-bundler/test/snapshot-lazy.realbuild.test.ts
✅ Files skipped from review due to trivial changes (1)
- tools/egg-bundler/src/index.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tools/egg-bundler/test/snapshot-lazy-bundler.test.ts (1)
119-120: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the prelude-order assertion fail closed.
If
globalThis.__LAZY_EXT = new Set(disappears,indexOf()returns-1and this still passes as long as__UTOOPACK__exists.Suggested fix
// prelude (with __LAZY_EXT) precedes the bundle IIFE - expect(worker.indexOf('globalThis.__LAZY_EXT = new Set(')).toBeLessThan(worker.indexOf('__UTOOPACK__')); + const lazyExtIndex = worker.indexOf('globalThis.__LAZY_EXT = new Set('); + const bundleIndex = worker.indexOf('__UTOOPACK__'); + expect(lazyExtIndex).toBeGreaterThanOrEqual(0); + expect(bundleIndex).toBeGreaterThanOrEqual(0); + expect(lazyExtIndex).toBeLessThan(bundleIndex);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/egg-bundler/test/snapshot-lazy-bundler.test.ts` around lines 119 - 120, The prelude-order check in the snapshot test is not fail-closed because worker.indexOf('globalThis.__LAZY_EXT = new Set(') can return -1 and still satisfy the less-than assertion. Update the assertion in snapshot-lazy-bundler.test.ts to first verify the prelude string is present, then compare its position against '__UTOOPACK__', so the test fails when the lazy-extension prelude disappears. Use the existing worker.indexOf(...) expressions in the test to locate and tighten this check.tools/egg-bundler/src/lib/Bundler.ts (1)
499-540: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDon't treat already-patched bundles as injection failures.
injectExternalRequireLazyHook()intentionally reportsinjected: 0onceglobalThis.__makeLazyExt(is already present. On a rerun over previously patched output,function externalRequireis still detected here, so this branch throws even though the lazy dispatch is already installed.Suggested fix
const prependedEntries: string[] = []; let injectedCount = 0; let sawExternalRequire = false; + let sawInjectedLazyHook = false; @@ const filepath = path.join(outputDir, rel); const original = await fs.readFile(filepath, 'utf8'); // Detect the `@utoo/pack` helper *definition* independently of our injection // regex, so a helper that is emitted but not patched (e.g. a codegen format // our injection regex no longer matches) becomes a loud build error below // rather than a silent snapshot that loads the network stack at build time. if (/function\s+externalRequire\b/.test(original)) sawExternalRequire = true; + if (original.includes('globalThis.__makeLazyExt(')) sawInjectedLazyHook = true; const hooked = injectExternalRequireLazyHook(original); injectedCount += hooked.injected; @@ if (injectedCount === 0) { - if (sawExternalRequire) { + if (sawExternalRequire && !sawInjectedLazyHook) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/egg-bundler/src/lib/Bundler.ts` around lines 499 - 540, The snapshot injection flow in Bundler.ts incorrectly treats already-patched bundles as failures because `sawExternalRequire` is set from `function externalRequire` even when `injectExternalRequireLazyHook()` returns `injected: 0` due to `globalThis.__makeLazyExt(` already being present. Update the `injectedCount === 0` handling to distinguish “helper present but already patched” from “helper present and unpatched,” using the `injectExternalRequireLazyHook()` result and its existing signature checks so reruns over previously patched output do not throw.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/egg-bundler/src/lib/prelude.ts`:
- Around line 312-315: The lazy stub construct trap in prelude.ts is dropping
callers off the proxy by returning a plain object when the real module is not a
function. Update the construct handler in the lazy stub/proxy logic so
constructed instances remain chainable and still route method calls through the
stubbed proxy instead of becoming undefined-backed objects. Use the existing
realModule() and Reflect.construct path as the reference point, and preserve
proxy behavior for build-time usages like new http.Agent() or new
dns.Resolver().
---
Duplicate comments:
In `@tools/egg-bundler/src/lib/Bundler.ts`:
- Around line 499-540: The snapshot injection flow in Bundler.ts incorrectly
treats already-patched bundles as failures because `sawExternalRequire` is set
from `function externalRequire` even when `injectExternalRequireLazyHook()`
returns `injected: 0` due to `globalThis.__makeLazyExt(` already being present.
Update the `injectedCount === 0` handling to distinguish “helper present but
already patched” from “helper present and unpatched,” using the
`injectExternalRequireLazyHook()` result and its existing signature checks so
reruns over previously patched output do not throw.
In `@tools/egg-bundler/test/snapshot-lazy-bundler.test.ts`:
- Around line 119-120: The prelude-order check in the snapshot test is not
fail-closed because worker.indexOf('globalThis.__LAZY_EXT = new Set(') can
return -1 and still satisfy the less-than assertion. Update the assertion in
snapshot-lazy-bundler.test.ts to first verify the prelude string is present,
then compare its position against '__UTOOPACK__', so the test fails when the
lazy-extension prelude disappears. Use the existing worker.indexOf(...)
expressions in the test to locate and tighten this check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 714fce6d-6b09-453f-a84b-ba8269bbc705
📒 Files selected for processing (6)
tools/egg-bundler/src/index.tstools/egg-bundler/src/lib/Bundler.tstools/egg-bundler/src/lib/prelude.tstools/egg-bundler/test/snapshot-lazy-bundler.test.tstools/egg-bundler/test/snapshot-lazy-external.test.tstools/egg-bundler/test/snapshot-lazy.realbuild.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/egg-bundler/src/index.ts
Fill the snapshot prelude skeleton from eggjs#5998 with the lazy-external / native-binding stub mechanism, so a snapshot bundle stays serializable. The Node network stack (http/https/http2/tls/dns) produces native bindings (HTTPParser, nghttp2 settingsBuffer, tls SecureContext, dns ChannelWrap) that V8 cannot serialize, and WebAssembly is disabled under --build-snapshot. They must not load while the snapshot is built, then load for real at restore. prelude.ts (renderSnapshotPrelude now carries the lazy id set): - deletes Node's lazy web globals with `delete` (redefining the accessor would trigger the undici load we avoid); - installs `__LAZY_EXT` + `__makeLazyExt`. The factory returns a Proxy that is a stub at build time (hardcoded http METHODS/STATUS_CODES/maxHeaderSize so a library's top-level `[...http.METHODS]` does not force a load) and forwards to the real module via `globalThis.__RUNTIME_REQUIRE` once the generated restore entry installs it. Structural traps delegate to the target to keep Proxy invariants. - new `injectExternalRequireLazyHook` and `resolveSnapshotLazyModules`. Bundler (snapshot mode): resolves the lazy id set (network-stack default + app `egg.snapshot.lazyModules` from package.json), keeps those ids external so @utoo/pack emits externalRequire, injects the lazy dispatch into externalRequire, then prepends the prelude carrying that id set. Third-party business deps (leoric/@elastic/...) are out of scope — apps can add them via egg.snapshot.lazyModules but stub-completeness is a separate RFC. Tests: lazy-module resolution, filled prelude, externalRequire injection, vm-based runtime proxy behavior (build stub vs restore), a Bundler wiring test, and a real @utoo/pack build proving http is a stub at build and the real module after __RUNTIME_REQUIRE. eggjs#5998's prelude/Bundler tests stay green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
killagu
force-pushed
the
feat/bundler-snapshot-lazy-external
branch
from
e97acbe to
c8c2e1b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/egg-bundler/src/lib/Bundler.ts`:
- Around line 505-508: The `Bundler.ts` `externalRequire` detection is too broad
because `/function\s+externalRequire\b/` matches text inside strings and
comments, which can incorrectly trip the later snapshot-build failure path.
Update the `sawExternalRequire` check to use a syntax-aware approach in
`Bundler` (or otherwise restrict the match to real function declarations only)
so `original` is not flagged by literal/comment content, while preserving the
existing `sawInjectedLazyHook` logic.
In `@tools/egg-bundler/src/lib/prelude.ts`:
- Around line 399-429: The blind regex replacement in
injectExternalRequireLazyHook is matching text inside string literals, which
corrupts bundle contents and inflates the injected count. Update the logic in
injectExternalRequireLazyHook to target only real externalRequire function
bodies by parsing the source (for example with acorn or `@babel/parser`) and
inserting the lazy dispatch in the function AST, or otherwise constrain
EXTERNAL_REQUIRE_SIGNATURE so it cannot match strings/comments. Keep the
idempotency check for __makeLazyExt, and ensure Bundler.#applySnapshotPrelude
receives an accurate injected count from the corrected traversal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e3163f08-d322-4ae5-b5ab-ede5f9183298
📒 Files selected for processing (6)
tools/egg-bundler/src/index.tstools/egg-bundler/src/lib/Bundler.tstools/egg-bundler/src/lib/prelude.tstools/egg-bundler/test/snapshot-lazy-bundler.test.tstools/egg-bundler/test/snapshot-lazy-external.test.tstools/egg-bundler/test/snapshot-lazy.realbuild.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/egg-bundler/src/index.ts
Comment on lines
+505
to
+508
| if (/function\s+externalRequire\b/.test(original)) sawExternalRequire = true; | ||
| // A file already carrying the dispatch was patched on a previous run; its | ||
| // injectedCount is 0 (idempotent) but it must NOT count as "unpatched". | ||
| if (original.includes('globalThis.__makeLazyExt(')) sawInjectedLazyHook = true; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import re
detect = re.compile(r'function\s+externalRequire\b')
samples = [
'const s = "function externalRequire(id) { return id }";',
'// function externalRequire(id, thunk) {}',
]
for sample in samples:
print(bool(detect.search(sample)), sample)
PYRepository: eggjs/egg
Length of output: 255
False positives in externalRequire detection risking snapshot build failures.
The regex /function\s+externalRequire\b/ matches raw bundle text, causing false positives when the phrase appears inside string literals or comments. Execution confirms inputs like 'const s = "function externalRequire(id) { return id }";' and // function externalRequire(id, thunk) {} trigger sawExternalRequire = true.
When combined with the strict failure check at lines 535-544, this causes snapshot builds to crash incorrectly on valid code containing such text. Replace the raw-text regex with an AST-based check or refine the pattern to exclude string/comment contexts.
Code snippet
if (/function\s+externalRequire\b/.test(original)) sawExternalRequire = true;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/egg-bundler/src/lib/Bundler.ts` around lines 505 - 508, The
`Bundler.ts` `externalRequire` detection is too broad because
`/function\s+externalRequire\b/` matches text inside strings and comments, which
can incorrectly trip the later snapshot-build failure path. Update the
`sawExternalRequire` check to use a syntax-aware approach in `Bundler` (or
otherwise restrict the match to real function declarations only) so `original`
is not flagged by literal/comment content, while preserving the existing
`sawInjectedLazyHook` logic.
Contributor
Author
There was a problem hiding this comment.
Thanks — the string-literal/comment collision is a real theoretical edge, but I'd like to keep the regex here rather than pull in an AST parser, for consistency and proportionality:
- Established pattern in this same file.
Bundler#patchImportMetaOutputalready post-processes the @utoo/pack worker output with regex replacement (TURBOPACK_IMPORT_META_OBJECT,THROWING_IMPORT_META_URL). This PR follows that accepted approach; switching only the snapshot path to AST would be inconsistent. - Cost vs. risk. The single-file worker is multi-MB; running acorn/@babel over it on every snapshot build (plus a new parser dependency) is heavy for a collision that needs the Turbopack-internal token
externalRequireto appear in function-signature shape inside a string literal in a bundled dep — vanishingly unlikely for that specific identifier. - Bounded failure. The injection is idempotent (skips when
__makeLazyExt(is present), and the fail-loud check only triggers on afunction externalRequiredefinition that was emitted but left unpatched. Thesnapshot-lazy.realbuild.test.tsexercises the real @utoo/pack output end-to-end.
Happy to switch to an AST-scoped injection in a follow-up if you'd prefer the stronger guarantee — just let me know.
Contributor
There was a problem hiding this comment.
@killagu, that makes sense — thanks for the detailed rationale. I’m okay leaving this as-is for this PR given the consistency with the existing regex-based post-processing in tools/egg-bundler/src/lib/Bundler.ts, the low practical risk, and the real-build coverage you added. If it ever shows up in the wild, an AST-scoped follow-up would be a reasonable escalation.
🐇💭
✏️ Learnings added
Learnt from: killagu
Repo: eggjs/egg PR: 5999
File: tools/egg-bundler/src/lib/Bundler.ts:505-508
Timestamp: 2026-06-26T18:36:56.614Z
Learning: In `tools/egg-bundler/src/lib/Bundler.ts`, maintainers prefer regex-based post-processing of emitted `utoo/pack` worker output (including snapshot-prelude handling) over adding an AST parser, for consistency with `Bundler#patchImportMetaOutput` and to avoid the cost of parsing multi-megabyte single-file bundles unless a real issue justifies the stronger approach.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Comment on lines
+399
to
+429
| const EXTERNAL_REQUIRE_SIGNATURE = | ||
| /function\s+externalRequire\s*\(\s*([A-Za-z_$][\w$]*)\s*,\s*([A-Za-z_$][\w$]*)\s*(?:,[^)]*)?\)\s*\{/g; | ||
|
|
||
| export interface ExternalRequireInjectionResult { | ||
| readonly content: string; | ||
| readonly injected: number; | ||
| } | ||
|
|
||
| /** | ||
| * Inject the lazy dispatch at the start of every `externalRequire` body: | ||
| * | ||
| * ```js | ||
| * if (globalThis.__LAZY_EXT && globalThis.__LAZY_EXT.has(id)) return globalThis.__makeLazyExt(id, thunk); | ||
| * ``` | ||
| * | ||
| * so a require of a lazy module id is rerouted to {@link renderSnapshotPrelude}'s | ||
| * `__makeLazyExt` instead of loading the real (non-serializable) module. | ||
| */ | ||
| export function injectExternalRequireLazyHook(content: string): ExternalRequireInjectionResult { | ||
| // Idempotent: a re-run over already-injected output must not double-inject the | ||
| // dispatch. `__makeLazyExt(` only appears as the call site we inject (the prelude | ||
| // *assigns* `__makeLazyExt =`, which has no `(`), so its presence means this file | ||
| // was already processed. | ||
| if (content.includes('globalThis.__makeLazyExt(')) { | ||
| return { content, injected: 0 }; | ||
| } | ||
| let injected = 0; | ||
| const next = content.replace(EXTERNAL_REQUIRE_SIGNATURE, (match, idParam: string, thunkParam: string) => { | ||
| injected++; | ||
| return `${match} if (globalThis.__LAZY_EXT && globalThis.__LAZY_EXT.has(${idParam})) return globalThis.__makeLazyExt(${idParam}, ${thunkParam});`; | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import re
pattern = re.compile(r'function\s+externalRequire\s*\(\s*([A-Za-z_$][\w$]*)\s*,\s*([A-Za-z_$][\w$]*)\s*(?:,[^)]*)?\)\s*\{')
src = 'const s = "function externalRequire(id, thunk, esm = false) {";'
patched = pattern.sub(
lambda m: m.group(0) + f' if (globalThis.__LAZY_EXT && globalThis.__LAZY_EXT.has({m.group(1)})) return globalThis.__makeLazyExt({m.group(1)}, {m.group(2)});',
src,
)
print("matched:", pattern.search(src) is not None)
print("patched differs:", patched != src)
print(patched)
PYRepository: eggjs/egg
Length of output: 351
Regex-based injection corrupts string literals and miscounts injections
injectExternalRequireLazyHook() uses blind regex replacement on raw bundle text. As verified, a string like const s = "function externalRequire(id, thunk) {" is incorrectly matched and mutated, corrupting the bundled source and inflating the injected counter. This can hide unpatched real modules in Bundler.#applySnapshotPrelude() and deliver broken bundles.
Replace the text-based approach with an AST traversal (e.g., acorn or @babel/parser) to inject only inside actual externalRequire function bodies, or ensure the regex scope is restricted to non-string contexts.
-export function injectExternalRequireLazyHook(content: string): ExternalRequireInjectionResult {
- if (content.includes('globalThis.__makeLazyExt(')) {
- return { content, injected: 0 };
- }
- let injected = 0;
- const next = content.replace(EXTERNAL_REQUIRE_SIGNATURE, (match, idParam: string, thunkParam: string) => {
- injected++;
- return `${match} if (globalThis.__LAZY_EXT && globalThis.__LAZY_EXT.has(${idParam})) return globalThis.__makeLazyExt(${idParam}, ${thunkParam});`;
- });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/egg-bundler/src/lib/prelude.ts` around lines 399 - 429, The blind regex
replacement in injectExternalRequireLazyHook is matching text inside string
literals, which corrupts bundle contents and inflates the injected count. Update
the logic in injectExternalRequireLazyHook to target only real externalRequire
function bodies by parsing the source (for example with acorn or `@babel/parser`)
and inserting the lazy dispatch in the function AST, or otherwise constrain
EXTERNAL_REQUIRE_SIGNATURE so it cannot match strings/comments. Keep the
idempotency check for __makeLazyExt, and ensure Bundler.#applySnapshotPrelude
receives an accurate injected count from the corrected traversal.
Contributor
Author
There was a problem hiding this comment.
Thanks — the string-literal/comment collision is a real theoretical edge, but I'd like to keep the regex here rather than pull in an AST parser, for consistency and proportionality:
- Established pattern in this same file.
Bundler#patchImportMetaOutputalready post-processes the @utoo/pack worker output with regex replacement (TURBOPACK_IMPORT_META_OBJECT,THROWING_IMPORT_META_URL). This PR follows that accepted approach; switching only the snapshot path to AST would be inconsistent. - Cost vs. risk. The single-file worker is multi-MB; running acorn/@babel over it on every snapshot build (plus a new parser dependency) is heavy for a collision that needs the Turbopack-internal token
externalRequireto appear in function-signature shape inside a string literal in a bundled dep — vanishingly unlikely for that specific identifier. - Bounded failure. The injection is idempotent (skips when
__makeLazyExt(is present), and the fail-loud check only triggers on afunction externalRequiredefinition that was emitted but left unpatched. Thesnapshot-lazy.realbuild.test.tsexercises the real @utoo/pack output end-to-end.
Happy to switch to an AST-scoped injection in a follow-up if you'd prefer the stronger guarantee — just let me know.
Contributor
There was a problem hiding this comment.
@killagu, sounds good — thanks for laying out the tradeoff clearly.
Given the existing regex-based post-processing pattern around @utoo/pack output, the cost of AST-parsing a multi-MB worker, and the real-build coverage already added here, I’m okay leaving this regex-based in this PR and revisiting only if we hit a concrete collision in practice.
🐇💭
✏️ Learnings added
Learnt from: killagu
Repo: eggjs/egg PR: 5999
File: tools/egg-bundler/src/lib/prelude.ts:399-429
Timestamp: 2026-06-26T18:37:02.209Z
Learning: In `eggjs/egg`, regex-based post-processing of emitted `utoo/pack` worker output is an accepted pattern in the bundler path (for example `Bundler#patchImportMetaOutput`), and maintainers prefer keeping the snapshot lazy-external injection in `tools/egg-bundler/src/lib/prelude.ts` regex-based for consistency and proportionality unless a concrete collision is observed.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
killagu
added a commit
that referenced
this pull request
Jun 28, 2026
…nd cnpmcore e2e (#6003) ## Motivation Restoring a V8 startup snapshot requires **Node.js >= 24**: Node.js 22 aborts during deserialization of a non-trivial Egg heap with the native fatal `Check failed: current == end_slot_index` (a V8 bug). Building a snapshot still works on Node.js >= 22. Today nothing enforces or documents this, and there is no regression coverage. This PR adds a runtime gate, documentation, and an e2e regression — without changing the snapshot mechanism itself. Builds on the snapshot work already on `next` (#5998 entry/prelude + `egg-bin snapshot` command, #5999 lazy-external network stack, #6001 logger reopen, #6002 module-loader hooks). ## Scope **Runtime gate (restore ≥ 24; build stays ≥ 22)** - `@eggjs/scripts`: `egg-scripts start --snapshot-blob` refuses to launch on Node.js < 24 with a clear error *before* spawning, checking the major version of the resolved `--node` target binary (not just the egg-scripts runtime). Also adds `allowNo: true` to the `sourcemap` flag so `--no-sourcemap` is accepted. - `@eggjs/egg-bundler`: a defense-in-depth guard in the generated deserialize-main for direct `node --snapshot-blob` launches that manage to deserialize on an unsupported runtime. - `@eggjs/bin`: `snapshot build` prints a note that restoring needs Node.js >= 24. **Docs** - Enrich `site/docs/advanced/snapshot.md` (EN + ZH): Node version requirements, the CLI workflow (`egg-bin snapshot build` → `egg-scripts start --snapshot-blob`), how it works (load module graph → run to `configWillLoad` → freeze; restore = `didReady` + listen), performance (~233ms vs ~942ms, ~4× on cnpmcore), and known limitations. - Wire the page into the VitePress sidebar (EN + ZH) — it existed but was unreachable. **CI** - Add a blocking `cnpmcore-snapshot` ecosystem-ci e2e (Node 24): snapshot build → restore via `egg-scripts start --snapshot-blob` → `curl /-/ping` == 200 → stop. Wires `repo.json`, `patch-project.ts`, `.gitignore`. - Extract the shared health-check poll into `ecosystem-ci/wait-health.sh`, used by both the `cnpmcore` and `cnpmcore-snapshot` jobs. ## Test evidence - `pnpm --filter=@eggjs/scripts --filter=@eggjs/egg-bundler --filter=@eggjs/bin run typecheck` — clean. - `@eggjs/scripts` `snapshot-start.test.ts` (4 tests, incl. a Node<24 gate test and a `--no-sourcemap` parse regression test) + `start-unit.test.ts` — pass. - `@eggjs/bin` `snapshot.test.ts` — pass. - `@eggjs/egg-bundler` `EntryGenerator` canonical snapshot regenerated for the new guard; the rest of the suite matches the pre-change baseline (a few pre-existing macOS/Node-22 path-resolution failures are unrelated). - A multi-agent diff review was run and all confirmed findings addressed (most notably: `--no-sourcemap` is now parseable via `allowNo: true` — without it the e2e job would have failed 100%). ## Notes - The `cnpmcore-snapshot` job is correct-by-construction but **could not be validated on the author's machine** (Node 22, no MySQL+cnpmcore build); it relies on the supported path (lazy-external from #5999, no manual stubs) and is validated by this PR's CI run. - The job is intentionally a **separate matrix project** (not folded into the existing cnpmcore job) for failure isolation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added “V8 Startup Snapshot” documentation navigation. * Added support for snapshot project configuration (shared project-root patching). * **Bug Fixes** * Enforced Node.js version gating for V8 snapshot restore (requires Node.js ≥ 24) and improved snapshot restore safety. * Improved snapshot lazy-external behavior, including correct external named-export handling. * Updated the start command to allow `--no-sourcemap`. * **Documentation** * Expanded snapshot docs with requirements, workflow details, performance, and limitations. * **Tests** * Updated snapshot start/version-gating and lazy-external readiness assertions. * **Chores** * Improved E2E readiness checks with consistent polling, timeouts, and error log output. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
killagu
added a commit
that referenced
this pull request
Jun 28, 2026
## Motivation Building a V8 startup snapshot serializes the whole heap, so any dependency that opens a socket, starts a timer, or initializes a native binding at module-evaluation time can make the blob fail to build — or build and then crash on restore. There was no guide for finding which module is responsible or how to fix it. ## What New dedicated page `advanced/snapshot-troubleshooting.md` (EN + zh-CN): - **The serializability rule** — what cannot survive the round-trip (native bindings / libuv handles / lazy web-global getters) and when a dependency trips it. - **Failure surfaces** — build-time vs restore-time, with the exact error strings each emits (`killed by signal SIGSEGV`, `no blob was written`, `Check failed: current == end_slot_index`, `Aop Advice not found`, `Cannot find module`, …). - **Find the offending module** — `NODE_DEBUG` namespaces, a clean `NODE_OPTIONS`, `--dry-run`, `--skip-bundle` bisecting, `--force-external` confirmation. - **Fixes** — `--force-external`, `egg.snapshot.lazyModules`, the snapshot lifecycle hooks, deferring work out of module scope, avoiding the web globals. - **Failure modes in detail** (tegg `@Advice` filePath, the lazy-external member proxy, runtime-asset `ENOENT`) and a configuration reference table. Also documents the previously-undocumented `egg.snapshot.lazyModules` config in `advanced/snapshot.md`, cross-links the new page from it, and wires the page into the English and Chinese advanced sidebars. Docs-only; no runtime code change. Builds on the snapshot feature already on `next` (#5998 / #5999 / #6001 / #6003). ## Test evidence - `vitepress build site` passes clean — VitePress's dead-link/anchor check is green, so both new pages render and every cross-file and in-page anchor resolves (the two detail headings use explicit ASCII `{#…}` ids because the VitePress slugifier retains CJK + fullwidth `「」`). - Every technical claim was adversarially verified against the `egg-bundler` / `egg-bin` / `egg-scripts` source — exact error strings, flag names, debug namespaces, and the default lazy-module set. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added a new Snapshot Troubleshooting guide in English and Chinese, covering common build and restore failure symptoms, how to diagnose them, and recommended fixes. * Expanded the Snapshot guide with guidance on configuring additional lazy-loaded modules, plus clearer troubleshooting links. * Updated the sidebar navigation to include the new troubleshooting page in both languages. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jun 28, 2026
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
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.
Motivation
#5998 wired the V8 snapshot mechanism for
@eggjs/egg-bundler(3-mode entry, prelude prepend,egg-bin snapshot/egg-scripts start --snapshot-blob) and left the snapshot prelude as an intentional no-op skeleton with the note "PR3 fills the lazy / native-binding stub mechanism." This PR fills it.A V8 startup snapshot cannot serialize the native bindings the Node network stack allocates —
node:http/node:httpsHTTPParser(llhttp),node:http2nghttp2settingsBuffer,node:tlsSecureContext,node:dnsChannelWrap— andWebAssemblyis disabled under--build-snapshot. Egg's loader phase touches that stack (HttpClient, agents…), so it must not load while the snapshot is built, then load for real at restore.What this does
prelude.ts—renderSnapshotPrelude(lazyModules)now emits a real body:fetch/Headers/Request/…) withdelete— redefining the lazy accessor would itself trigger the undici load we avoid;globalThis.__LAZY_EXT(the lazy id set) andglobalThis.__makeLazyExt. The factory returns aProxythat is a stub at build time (hardcodedhttpMETHODS/STATUS_CODES/maxHeaderSizeso a library's top-level[...http.METHODS]doesn't force a load) and forwards to the real module viaglobalThis.__RUNTIME_REQUIREonce feat(egg-bundler): generate snapshot entry/prelude + egg-bin snapshot command #5998's restore entry installs it — sohttp.createServeris the genuine builtin and the app truly listens. Structural traps (ownKeys/getOwnPropertyDescriptor) delegate to the target to keepProxyinvariants.injectExternalRequireLazyHook(idempotent) andresolveSnapshotLazyModules.Bundler.ts(snapshot mode): resolves the lazy id set (network-stack default + the app'segg.snapshot.lazyModulesfrompackage.json), keeps those ids external so@utoo/packemits anexternalRequirecall, injects the lazy dispatch intoexternalRequire, and prepends the prelude carrying that id set — all in a single pass over the worker output.Default lazy list
http,https,http2(+node:variants),tls/node:tls,dns/node:dns.Application extension point
appended (deduped) to the default list.
Boundary
This PR covers the Node built-in network stack (fully functional at runtime). Third-party business deps (leoric/@elastic/…) can be added via
egg.snapshot.lazyModules, but completing their stubs is a separate RFC. Note the lazy mechanism relies on CommonJSrequire(how urllib/undici/node internals load the stack); ESMimport *can't defer through a static namespace, which is an inherent ESM limitation, not specific to this PR.Test evidence
snapshot-lazy-external.test.ts— lazy-module resolution, filled prelude content,externalRequireinjection (incl. idempotency), and vm-evaluated runtime proxy behavior (build-time stub vs restore-time forward, Proxy invariants).snapshot-lazy-bundler.test.ts— Bundler wiring: hook injected, lazy ids kept external,egg.snapshot.lazyModulesmerged, no-op when snapshot off.snapshot-lazy.realbuild.test.ts— real@utoo/packbuild provingnode:httpis a stub at build (hardcoded METHODS,createServer()no-ops) and the real module after__RUNTIME_REQUIREis installed.prelude.test.tsandBundler.test.tssnapshot cases stay green.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests