feat(bundler): lazy-externalize undici + urllib by default for snapshots#6011
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ChangesSnapshot lazy-external defaults and validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Code Review
This pull request configures the snapshot bundler to lazy-externalize the HTTP client stack (undici and urllib) by default, ensuring a serializable V8 startup snapshot. To support scenarios where a bundled module extends a lazy export at build time (e.g., class HttpClient extends urllib.HttpClient), a per-export forwarder is introduced to dynamically rebind the subclass to the real base class at restore time. The feedback identifies several gaps in the Proxy implementation of this forwarder—specifically regarding symbol handling at build time, and missing has, ownKeys, and getOwnPropertyDescriptor traps—and provides a robust implementation to ensure complete transparency and prevent potential runtime errors.
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.
| var fwd = new Proxy(function () {}, { | ||
| get: function (target, p) { | ||
| // \`Subclass.prototype.__proto__\` is wired to this at \`extends\` time. | ||
| if (p === 'prototype') return protoProxy; | ||
| var e = realExport(); | ||
| if (e !== undefined && e !== null) return e[p]; | ||
| if (p === 'then') return undefined; | ||
| if (p === 'name' || p === 'length') return Reflect.get(target, p); | ||
| return proxy; // build time: stay chainable on the well-tested module proxy | ||
| }, | ||
| apply: function (target, thisArg, args) { | ||
| var e = realExport(); | ||
| if (typeof e === 'function') return Reflect.apply(e, thisArg, args); | ||
| return undefined; | ||
| }, | ||
| construct: function (target, args, newTarget) { | ||
| var e = realExport(); | ||
| if (typeof e === 'function') { | ||
| // \`super(...)\` arrives with the subclass as newTarget; a direct | ||
| // \`new lazy.Export()\` arrives with \`fwd\` itself -> use the real base. | ||
| return Reflect.construct(e, args, newTarget === fwd ? e : newTarget); | ||
| } | ||
| return proxy; // build time: keep \`new lazy.Export().method()\` chainable | ||
| }, | ||
| }); |
There was a problem hiding this comment.
The per-export forwarder fwd has a few subtle gaps in its Proxy implementation compared to the main proxy:
- Symbol Handling at Build Time: If
pis a symbol (e.g.,Symbol.toStringTag,Symbol.toPrimitive,Symbol.iterator) and the real module is not loaded yet (build time), thegettrap will fall back to returningproxy. Returning a function/Proxy for standard symbols can break JS engine/library checks (e.g., causingTypeErrorduring coercion or infinite loops during iteration checks). It should returnundefinedfor symbols at build time, matching the mainproxy's behavior. - Missing
hasTrap: Without ahastrap, checking property existence on the forwarder (e.g.,'someStaticMethod' in urllib.HttpClient) at restore time will check the dummy targetfunction () {}and returnfalse, even if the property exists on the real export. - Missing
ownKeysandgetOwnPropertyDescriptorTraps: Without these traps, structural reflection (likeObject.keys(),Object.getOwnPropertyNames(), or object spread) on the forwarded export at restore time will only see the properties of the dummy target, completely hiding the real export's static properties and methods.
Adding these traps ensures complete transparency for the forwarded exports at restore time.
var fwd = new Proxy(function () {}, {
get: function (target, p) {
// Subclass.prototype.__proto__ is wired to this at extends time.
if (p === 'prototype') return protoProxy;
var e = realExport();
if (e !== undefined && e !== null) return e[p];
if (p === 'then') return undefined;
if (p === 'name' || p === 'length') return Reflect.get(target, p);
if (typeof p === 'symbol') return undefined;
return proxy; // build time: stay chainable on the well-tested module proxy
},
apply: function (target, thisArg, args) {
var e = realExport();
if (typeof e === 'function') return Reflect.apply(e, thisArg, args);
return undefined;
},
construct: function (target, args, newTarget) {
var e = realExport();
if (typeof e === 'function') {
// super(...) arrives with the subclass as newTarget; a direct
// new lazy.Export() arrives with fwd itself -> use the real base.
return Reflect.construct(e, args, newTarget === fwd ? e : newTarget);
}
return proxy; // build time: keep new lazy.Export().method() chainable
},
has: function (target, p) {
var e = realExport();
if (e !== undefined && e !== null) return p in e;
return Reflect.has(target, p);
},
ownKeys: function (target) {
var e = realExport();
if (e === undefined || e === null || (typeof e !== 'object' && typeof e !== 'function')) return Reflect.ownKeys(target);
var keys = Reflect.ownKeys(e);
var targetKeys = Reflect.ownKeys(target);
for (var i = 0; i < targetKeys.length; i++) {
if (keys.indexOf(targetKeys[i]) === -1) keys.push(targetKeys[i]);
}
return keys;
},
getOwnPropertyDescriptor: function (target, p) {
var targetDesc = Reflect.getOwnPropertyDescriptor(target, p);
if (targetDesc && !targetDesc.configurable) return targetDesc;
var e = realExport();
if (e === undefined || e === null || (typeof e !== 'object' && typeof e !== 'function')) return targetDesc;
var desc = Reflect.getOwnPropertyDescriptor(e, p);
if (desc) desc.configurable = true;
return desc;
},
});References
- When implementing a JavaScript Proxy that wraps a dummy target but delegates to a real object at runtime, ensure structural traps like ownKeys and getOwnPropertyDescriptor correctly merge or delegate to the real object. To avoid Proxy invariant violations (e.g., reporting a non-configurable property that is absent from the target), force configurable: true on descriptors retrieved from the real object, while still faithfully reporting the target's own non-configurable properties.
There was a problem hiding this comment.
Thanks. This PR was rebased onto the latest next; #6003 landed in the meantime and replaced this per-export forwarder with a general member-proxy (makeMember), so the code this comment was on no longer exists. The PR is now reduced to adding undici/urllib to the default lazy list.
On the specific gaps: the new top-level __makeLazyExt proxy already implements has/ownKeys/getOwnPropertyDescriptor (the last two backed by __EXTERNAL_EXPORTS so @utoo/pack's interopEsm enumerates the real named exports) and returns undefined for symbols at build time. The inner makeMember proxy intentionally records an access path (get/apply/construct) rather than mirroring structure. So these are handled upstream rather than in this PR.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 340-344: The proxy get trap in prelude.ts is rebinding static
accessors to the real export instead of preserving the subclass receiver. Update
the get handler in the lazy export proxy so that when realExport() returns a
value, it uses Reflect.get on the export with the provided recv instead of
direct property access, keeping class Sub extends lazy.Base static getters bound
to Sub rather than RealBase.
🪄 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: 56273d23-cd9f-44e4-9194-877e2421be50
📒 Files selected for processing (5)
tools/egg-bundler/src/lib/prelude.tstools/egg-bundler/test/snapshot-lazy-external.test.tstools/egg-bundler/test/snapshot-lazy.realbuild.test.tswiki/log.mdwiki/packages/egg-bundler.md
killagu
force-pushed
the
feat/snapshot-default-lazy-undici-urllib
branch
from
44a2789 to
58d5d56
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #6011 +/- ##
==========================================
- Coverage 81.95% 81.94% -0.01%
==========================================
Files 677 677
Lines 20651 20651
Branches 4099 4099
==========================================
- Hits 16924 16922 -2
- Misses 3214 3215 +1
- Partials 513 514 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 274-287: Update the proxy forwarding in prelude.ts so property
access goes through Reflect.get with the correct receiver instead of direct
indexing. In the member proxy’s get trap and protoProxy’s get trap, forward
reads to the resolved export/prototype using Reflect.get(..., recv) so accessors
and inherited/static getters observe the right this when the proxy is used as a
subclass or prototype. Keep the existing special cases for __MR, prototype,
then, and symbols, but replace the raw r[p] / r.prototype[p] lookup in the
relevant proxy handlers.
In `@tools/egg-bundler/test/snapshot-lazy-external.test.ts`:
- Around line 120-126: The snapshot test is hard-coding the expected header-size
value, which makes it brittle across Node versions. Update the assertion in the
`renderSnapshotPrelude()` test to compare against `String(http.maxHeaderSize)`
instead of the literal `16384`, while keeping the existing checks for the other
inlined HTTP constants. This should be done in the
`snapshot-lazy-external.test.ts` test case that verifies the
`globalThis.__HTTP_CONSTS` prelude output.
In `@tools/egg-bundler/test/snapshot-lazy.realbuild.test.ts`:
- Around line 221-265: The restore-path test currently can succeed via native
module resolution instead of the lazy hook, and it does not verify the promised
instanceof behavior. Update the restore runner around `__RUNTIME_REQUIRE` and
`__makeSub` so it blocks direct `lazy-base` resolution in `baseDir/dist`,
forcing the instantiation path to go through the runtime require hook. Then
extend the `restoreRunner` probe to assert the constructed instance is an
`instanceof Base`, alongside the existing `tag`, `greet`, and `describe` checks.
🪄 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: d9eda364-71f7-4d06-b98b-3f193b0e7710
📒 Files selected for processing (5)
tools/egg-bundler/src/lib/prelude.tstools/egg-bundler/test/snapshot-lazy-external.test.tstools/egg-bundler/test/snapshot-lazy.realbuild.test.tswiki/log.mdwiki/packages/egg-bundler.md
✅ Files skipped from review due to trivial changes (1)
- wiki/packages/egg-bundler.md
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 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 274-287: Update the proxy forwarding in prelude.ts so property
access goes through Reflect.get with the correct receiver instead of direct
indexing. In the member proxy’s get trap and protoProxy’s get trap, forward
reads to the resolved export/prototype using Reflect.get(..., recv) so accessors
and inherited/static getters observe the right this when the proxy is used as a
subclass or prototype. Keep the existing special cases for __MR, prototype,
then, and symbols, but replace the raw r[p] / r.prototype[p] lookup in the
relevant proxy handlers.
In `@tools/egg-bundler/test/snapshot-lazy-external.test.ts`:
- Around line 120-126: The snapshot test is hard-coding the expected header-size
value, which makes it brittle across Node versions. Update the assertion in the
`renderSnapshotPrelude()` test to compare against `String(http.maxHeaderSize)`
instead of the literal `16384`, while keeping the existing checks for the other
inlined HTTP constants. This should be done in the
`snapshot-lazy-external.test.ts` test case that verifies the
`globalThis.__HTTP_CONSTS` prelude output.
In `@tools/egg-bundler/test/snapshot-lazy.realbuild.test.ts`:
- Around line 221-265: The restore-path test currently can succeed via native
module resolution instead of the lazy hook, and it does not verify the promised
instanceof behavior. Update the restore runner around `__RUNTIME_REQUIRE` and
`__makeSub` so it blocks direct `lazy-base` resolution in `baseDir/dist`,
forcing the instantiation path to go through the runtime require hook. Then
extend the `restoreRunner` probe to assert the constructed instance is an
`instanceof Base`, alongside the existing `tag`, `greet`, and `describe` checks.
🪄 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: d9eda364-71f7-4d06-b98b-3f193b0e7710
📒 Files selected for processing (5)
tools/egg-bundler/src/lib/prelude.tstools/egg-bundler/test/snapshot-lazy-external.test.tstools/egg-bundler/test/snapshot-lazy.realbuild.test.tswiki/log.mdwiki/packages/egg-bundler.md
✅ Files skipped from review due to trivial changes (1)
- wiki/packages/egg-bundler.md
🛑 Comments failed to post (3)
tools/egg-bundler/src/lib/prelude.ts (1)
274-287: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Forward through
Reflect.get(..., recv)here.r[p]andr.prototype[p]bind accessors to the resolved export/prototype, so inherited or static getters still see the wrongthiswhen the proxy is used as a subclass or prototype.🤖 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 274 - 287, Update the proxy forwarding in prelude.ts so property access goes through Reflect.get with the correct receiver instead of direct indexing. In the member proxy’s get trap and protoProxy’s get trap, forward reads to the resolved export/prototype using Reflect.get(..., recv) so accessors and inherited/static getters observe the right this when the proxy is used as a subclass or prototype. Keep the existing special cases for __MR, prototype, then, and symbols, but replace the raw r[p] / r.prototype[p] lookup in the relevant proxy handlers.tools/egg-bundler/test/snapshot-lazy-external.test.ts (1)
120-126: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use
http.maxHeaderSizeinstead of a hard-coded value.renderSnapshotPrelude()serializes the build Node’s header-size default, so asserting16384makes this test brittle across Node runtimes. Compare againstString(http.maxHeaderSize)instead.🤖 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-external.test.ts` around lines 120 - 126, The snapshot test is hard-coding the expected header-size value, which makes it brittle across Node versions. Update the assertion in the `renderSnapshotPrelude()` test to compare against `String(http.maxHeaderSize)` instead of the literal `16384`, while keeping the existing checks for the other inlined HTTP constants. This should be done in the `snapshot-lazy-external.test.ts` test case that verifies the `globalThis.__HTTP_CONSTS` prelude output.tools/egg-bundler/test/snapshot-lazy.realbuild.test.ts (1)
221-265: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail git ls-files tools/egg-bundler/test/snapshot-lazy.realbuild.test.ts tools/egg-bundler/test | sed -n '1,120p' echo '--- FILE OUTLINE ---' ast-grep outline tools/egg-bundler/test/snapshot-lazy.realbuild.test.ts --view expanded echo '--- RELEVANT LINES ---' sed -n '180,290p' tools/egg-bundler/test/snapshot-lazy.realbuild.test.ts echo '--- SEARCH FOR INSTANCEOF / RUNTIME_REQUIRE / lazy-base ---' rg -n 'instanceof|__RUNTIME_REQUIRE|lazy-base|Base' tools/egg-bundler/test/snapshot-lazy.realbuild.test.ts tools/egg-bundler/test -SRepository: eggjs/egg
Length of output: 21404
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- snapshot-lazy-external restore behavior ---' sed -n '220,290p' tools/egg-bundler/test/snapshot-lazy-external.test.ts echo '--- snapshot-lazy-bundler lazy proxy behavior ---' sed -n '100,170p' tools/egg-bundler/test/snapshot-lazy-bundler.test.ts echo '--- any existing instanceof assertions in egg-bundler tests ---' rg -n 'instanceof\s+Base|instanceof\s+.*Base|instanceof' tools/egg-bundler/test -SRepository: eggjs/egg
Length of output: 6095
Make the restore runner exercise
__RUNTIME_REQUIREand assertinstanceof. Since the runner sits inbaseDir/dist,require('lazy-base')can still resolve natively frombaseDir/node_modules, so this test can pass even if the restore path never uses the lazy hook. It also never checks the advertisedinstanceofcontract. Block native resolution in the runner and add aninstanceof Baseassertion.🧰 Tools
🪛 ast-grep (0.44.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').(detect-child-process-typescript)
[warning] 226-239: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(
buildRunner,
[
'const probe = { phase: "build" };',
'try {',
' require("./worker.js");',
' globalThis.__makeSub(7);',
' probe.restored = !!globalThis.__RUNTIME_REQUIRE;',
' probe.threw = false;',
'} catch (e) { probe.threw = true; probe.err = e.message; }',
'process.stdout.write("\n" + JSON.stringify(probe));',
'',
].join('\n'),
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').(detect-non-literal-fs-filename-typescript)
[warning] 247-262: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(
restoreRunner,
[
'require("./worker.js");',
globalThis.__RUNTIME_REQUIRE = (id) => id === "lazy-base" ? require(${basePathLiteral}) : require(id);,
'const inst = globalThis.__makeSub(7);',
'const probe = {',
' restored: true,',
' tag: inst.tag,',
' greet: inst.greet(),',
' describe: inst.describe(),',
'};',
'process.stdout.write("\n" + JSON.stringify(probe));',
'',
].join('\n'),
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').(detect-non-literal-fs-filename-typescript)
🤖 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` around lines 221 - 265, The restore-path test currently can succeed via native module resolution instead of the lazy hook, and it does not verify the promised instanceof behavior. Update the restore runner around `__RUNTIME_REQUIRE` and `__makeSub` so it blocks direct `lazy-base` resolution in `baseDir/dist`, forcing the instantiation path to go through the runtime require hook. Then extend the `restoreRunner` probe to assert the constructed instance is an `instanceof Base`, alongside the existing `tag`, `greet`, and `describe` checks.
killagu
force-pushed
the
feat/snapshot-default-lazy-undici-urllib
branch
from
58d5d56 to
1779d3f
Compare
Re: CodeRabbit re-review (3 items; inline comments failed to post)
Re: "do other framework built-in packages need force-external?" (incl. the MCP SDK)Audited egg core + default plugins + |
killagu
force-pushed
the
feat/snapshot-default-lazy-undici-urllib
branch
from
1779d3f to
6f44b93
Compare
Add `undici` and `urllib` to DEFAULT_SNAPSHOT_LAZY_MODULES so an app gets a serializable V8 startup snapshot without listing them in `egg.snapshot.lazyModules`. Egg builds its HttpClient (urllib -> undici) during boot, and undici instantiates an llhttp WebAssembly module (disabled under --build-snapshot) + an HTTPParser that cannot be snapshot-serialized. As npm packages, urllib/undici would otherwise be inlined into the bundle and evaluated at build time; listing them here forces them external so the prelude's member-proxy stub is used at build and the real module is required on restore. The member-proxy (eggjs#6003) already replays the recorded access path, so egg's `class HttpClient extends urllib.HttpClient` keeps working across the build->restore boundary. Adds a unit assertion for the default list and a real @utoo/pack build test that exercises `class Sub extends pkg.Base` for a forced-external npm package across the build-stub / restore-real boundary (upstream only covered the node:http builtin). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
killagu
force-pushed
the
feat/snapshot-default-lazy-undici-urllib
branch
from
6f44b93 to
d428e29
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/egg-bundler/test/snapshot-lazy.realbuild.test.ts (1)
295-296: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid pinning this test to the helper’s exact emitted syntax.
/function\s+externalRequire\s*\(/couples the test to a specific codegen shape. A harmless refactor toconst externalRequire = ...would fail this test even if snapshot lazy-restore still works. The behavioral checks below already cover the contract this PR cares about.Suggested change
expect(worker).toContain(SNAPSHOT_PRELUDE_MARKER); - expect(worker).toMatch(/function\s+externalRequire\s*\(/); await execFileAsync(process.execPath, ['--check', workerPath]);🤖 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` around lines 295 - 296, The snapshot-lazy restore test is over-coupled to the exact emitted syntax of externalRequire, so remove the regex assertion from snapshot-lazy.realbuild.test.ts and keep the behavioral checks around SNAPSHOT_PRELUDE_MARKER and the lazy-restore contract instead. Locate the assertion in the test that matches externalRequire and replace it with a less implementation-specific check so harmless codegen refactors do not break the test.
🤖 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.
Nitpick comments:
In `@tools/egg-bundler/test/snapshot-lazy.realbuild.test.ts`:
- Around line 295-296: The snapshot-lazy restore test is over-coupled to the
exact emitted syntax of externalRequire, so remove the regex assertion from
snapshot-lazy.realbuild.test.ts and keep the behavioral checks around
SNAPSHOT_PRELUDE_MARKER and the lazy-restore contract instead. Locate the
assertion in the test that matches externalRequire and replace it with a less
implementation-specific check so harmless codegen refactors do not break the
test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f1d8a13b-faf7-4071-ac64-8360832bab91
📒 Files selected for processing (6)
AGENTS.mdtools/egg-bundler/src/lib/prelude.tstools/egg-bundler/test/snapshot-lazy-external.test.tstools/egg-bundler/test/snapshot-lazy.realbuild.test.tswiki/log.mdwiki/packages/egg-bundler.md
✅ Files skipped from review due to trivial changes (4)
- AGENTS.md
- wiki/log.md
- tools/egg-bundler/test/snapshot-lazy-external.test.ts
- wiki/packages/egg-bundler.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/egg-bundler/src/lib/prelude.ts
…rt readiness) (#6015) Two unrelated **flaky test** fixes surfaced while stabilizing CI for the snapshot work (#6011). Test-only; no `src` changes. ## 1. `@eggjs/multipart` — `ENOTEMPTY` teardown race `file-mode.test.ts` flaked on macOS Node 24 (passing on every other matrix job): ``` Error: ENOTEMPTY: directory not empty, rmdir '.../egg-multipart-tmp/multipart-file-mode-demo/2026/06/28/05' ``` `fs.rm(tmpdir, { force: true, recursive: true })` defaults to `maxRetries: 0`, so a transient `ENOTEMPTY` (a file in a date-bucket dir mid recursive-removal — async per-request cleanup draining / APFS latency) throws and fails the suite. The production `clean_tmpdir` schedule already swallows these; only the 7 test teardowns are fragile. **Fix:** add `maxRetries: 3` to all 7. ## 2. `@eggjs/scripts` — flaky `stop.test.ts` start readiness `Test scripts (ubuntu-22)` failed in `stop.test.ts`: ``` AssertionError: expected 'Starting custom-framework application…' to match /custom-framework started on http:\/\/…/ ``` `stop.test.ts` waited a fixed **1s** for a 2-worker egg app to boot before asserting its `started on http://…` log — while every sibling `start-without-demon-*.test.ts` waits **10s**. On a loaded runner 1s isn't enough. **Fix:** replace the fixed `scheduler.wait(waitTime)` before each startup/shutdown assertion with a `waitFor(getText, pattern, timeout)` poll (`test/utils.ts`) that returns as soon as the expected output appears (up to 10s) — race-free and faster than a fixed delay. Windows keeps the fixed settle for the signal-driven shutdown checks (SIGTERM isn't handled there). ## Verification - multipart: the 7 teardowns now retry; affected tests pass locally. - scripts: `stop.test.ts` passes locally after build (8 passed, 1 skipped). Re Gemini's review on the multipart hunks: it claimed Vitest runs `afterAll` in registration order — that's incorrect (default `sequence.hooks: 'stack'` → reverse; verified empirically on v4.1.9), so the current teardown already removes the tmpdir **last** (after `server.close()`/`app.close()`); the suggested reorder would move `fs.rm` first and reintroduce the race. Replied inline on each thread. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Improved cleanup reliability across multipart-related test suites by adding additional retry behavior when removing temporary directories during teardown. * Updated process lifecycle checks in test runs to wait for specific startup/shutdown output instead of relying on fixed timing, reducing flaky boot/shutdown timing. * Introduced a reusable polling helper to detect expected log output within a timeout. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Motivation
Under
snapshot: true, the bundler keeps modules lazy-external so a V8 startup snapshot stays serializable (member-proxy stub at build, real module forwarded at restore viaglobalThis.__RUNTIME_REQUIRE).DEFAULT_SNAPSHOT_LAZY_MODULEScurrently covers the Node network stack +inspector.Egg builds its HttpClient (urllib → undici) during boot, and undici instantiates an llhttp
WebAssemblymodule (disabled under--build-snapshot) + anHTTPParserthat cannot be snapshot-serialized. As npm packages, urllib/undici would otherwise be inlined into the bundle and evaluated at build time. So every app had to manually add them toegg.snapshot.lazyModulesto get a working snapshot.This adds
undici+urllibto the default list so they're forced external by default — apps get a serializable snapshot for free.What changed
prelude.ts: addundici+urllibtoDEFAULT_SNAPSHOT_LAZY_MODULES(+ JSDoc explaining the rationale and the npm-package-vs-builtin distinction).Bundleradds lazy ids to the externals map, so listing them forces them external; the member-proxy from feat(snapshot): gate V8 snapshot restore to Node.js >= 24, add docs and cnpmcore e2e #6003 then stubs them at build and replays the access path against the real module on restore — so egg'sclass HttpClient extends urllib.HttpClientkeeps working across the build→restore boundary.Tests
undici/urllibare in the default list and surviveresolveSnapshotLazyModules.@utoo/packbuild: a forced-external npm package withclass Sub extends pkg.Baseis a stub at build (no throw, real module never loaded) and a real base instance after__RUNTIME_REQUIREis installed —super(...), the inherited method, and the real instance field all work. Upstream only realbuild-tested thenode:httpbuiltin, so this fills the forced-external-npm gap.28 lazy/realbuild tests green;
tsgo+oxlintclean.Note on scope (rebase)
This PR originally (off the older
next) shipped a bespoke per-export forwarder inside__makeLazyExtto makeclass HttpClient extends urllib.HttpClientsurvive the build→restore boundary. While it was open, #6003 landed and rewrote__makeLazyExtinto a general access-path-recording member-proxy (makeMember) that already handlesclass X extends pkg.Klass(plusownKeys/getOwnPropertyDescriptorvia__EXTERNAL_EXPORTS, arg resolution, etc.). The PR has been rebased onto that and reduced to just the default-list addition — the forwarder is dropped as superseded. The earlier CodeRabbit/Gemini review comments were on that now-removed code; replies posted inline.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
undiciandurllibautomatically (no extra configuration required).Tests
class extendsand correct restore timing.Documentation