fix(bundler): re-install web globals after a V8 snapshot restore#6012
Conversation
|
Caution Review failedPull request was closed or merged during review 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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR adds lazy restore of snapshot web globals, extends ChangesSnapshot Web Globals Lazy Reinstall
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
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. 🔧 OpenGrep (1.23.0)tools/egg-bundler/test/EntryGenerator.test.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.20][ERROR]: unable to find a config; path tools/egg-bundler/src/lib/prelude.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.19][ERROR]: unable to find a config; path tools/egg-bundler/src/lib/EntryGenerator.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.18][ERROR]: unable to find a config; path
🔧 markdownlint-cli2 (0.22.1)site/docs/advanced/snapshot.mdmarkdownlint-cli2 wrapper config was not available before execution site/docs/zh-CN/advanced/snapshot.mdmarkdownlint-cli2 wrapper config was not available before execution 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 implements lazy re-installation of Node's web globals (such as fetch, Headers, Blob, and File) after restoring a bundled snapshot, preventing serialization issues with native bindings like undici's llhttp parser. It deletes undici-backed globals at build time, replaces them with stubs to prevent build crashes, and lazily restores them using __installWebGlobalsLazy. Feedback focuses on cleaning up temporary global module and exports definitions via queueMicrotask to avoid V8 snapshot serialization, and defensively checking for non-configurable properties before calling Object.defineProperty to prevent runtime crashes.
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.
| if (typeof globalThis.module === 'undefined') { | ||
| try { globalThis.module = { exports: {} }; } catch (e) {} | ||
| } | ||
| if (typeof globalThis.exports === 'undefined') { | ||
| try { globalThis.exports = globalThis.module && globalThis.module.exports; } catch (e) {} | ||
| } |
There was a problem hiding this comment.
Defining globalThis.module and globalThis.exports on the global object during snapshot build is necessary for UMD externals, but if they are not cleaned up, they will be serialized into the V8 snapshot heap. After restore, having globalThis.module defined in a standard Node.js environment can confuse third-party libraries that check for globalThis.module or typeof module === 'object' globally to detect their environment, potentially breaking them.
We should clean them up using queueMicrotask so they are deleted after the synchronous bundle evaluation completes but before the snapshot is serialized.
| if (typeof globalThis.module === 'undefined') { | |
| try { globalThis.module = { exports: {} }; } catch (e) {} | |
| } | |
| if (typeof globalThis.exports === 'undefined') { | |
| try { globalThis.exports = globalThis.module && globalThis.module.exports; } catch (e) {} | |
| } | |
| if (typeof globalThis.module === 'undefined') { | |
| try { | |
| globalThis.module = { exports: {} }; | |
| if (typeof queueMicrotask === 'function') { | |
| queueMicrotask(function () { | |
| try { delete globalThis.module; } catch (e) {} | |
| }); | |
| } | |
| } catch (e) {} | |
| } | |
| if (typeof globalThis.exports === 'undefined') { | |
| try { | |
| globalThis.exports = globalThis.module && globalThis.module.exports; | |
| if (typeof queueMicrotask === 'function') { | |
| queueMicrotask(function () { | |
| try { delete globalThis.exports; } catch (e) {} | |
| }); | |
| } | |
| } catch (e) {} | |
| } |
There was a problem hiding this comment.
Good catch — addressed in 9f099e2, but not via queueMicrotask. External module factories can be evaluated lazily during egg's async loading (after a microtask would have fired), which would re-break the UMD require branch. Instead the generated snapshot-build entry deletes globalThis.module/exports right after loading reaches configWillLoad (every bundled module — and thus every external factory — has evaluated by then) and before V8 serializes, so they are not baked into the blob. Verified the real cnpmcore snapshot still builds → restores → GET /-/ping 200 with the cleanup in place.
| var existing = Object.getOwnPropertyDescriptor(globalThis, name); | ||
| if (existing) { | ||
| if (!('value' in existing)) return; // an accessor (native / already installed) | ||
| var current = existing.value; | ||
| if (current !== undefined && !(current && current.__EGG_WEB_GLOBAL_STUB__)) return; | ||
| } |
There was a problem hiding this comment.
If a global property is non-configurable (e.g., in a strict custom sandbox or a frozen environment), attempting to redefine it with Object.defineProperty will throw a TypeError and crash the process during snapshot restore. We should defensively check if existing.configurable === false and return early to prevent this.
| var existing = Object.getOwnPropertyDescriptor(globalThis, name); | |
| if (existing) { | |
| if (!('value' in existing)) return; // an accessor (native / already installed) | |
| var current = existing.value; | |
| if (current !== undefined && !(current && current.__EGG_WEB_GLOBAL_STUB__)) return; | |
| } | |
| var existing = Object.getOwnPropertyDescriptor(globalThis, name); | |
| if (existing) { | |
| if (existing.configurable === false) return; | |
| if (!('value' in existing)) return; // an accessor (native / already installed) | |
| var current = existing.value; | |
| if (current !== undefined && !(current && current.__EGG_WEB_GLOBAL_STUB__)) return; | |
| } |
There was a problem hiding this comment.
Applied in 9f099e2 — the installer now returns early when the existing descriptor is non-configurable, before reaching Object.defineProperty. Added a regression test (skips a non-configurable global instead of throwing).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
tools/egg-bundler/test/snapshot-lazy-external.test.ts (2)
120-128: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the exact
exportswiring.
prelude.tssetsglobalThis.exports = globalThis.module && globalThis.module.exports. This substring check would still pass if someone regressed it toglobalThis.exports = globalThis.module, which is the wrong CJS shape for UMD factories.Suggested tightening
- expect(prelude).toContain('globalThis.exports = globalThis.module'); + expect(prelude).toContain( + 'globalThis.exports = globalThis.module && globalThis.module.exports', + );🤖 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 - 128, The snapshot prelude test is asserting the wrong `exports` wiring and could miss a regression in the CommonJS shape used by UMD factories. Tighten the `renderSnapshotPrelude()` assertions in `snapshot-lazy-external.test.ts` so they verify the exact `globalThis.exports = globalThis.module && globalThis.module.exports` form rather than the looser `globalThis.module` substring; keep the existing checks around the snapshot-only branch and the `module/exports` setup.
404-415: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the successful
urllibfallback branch too.This suite proves direct
undiciloading and total failure, but not the pnpm path intools/egg-bundler/src/lib/prelude.tswherert('undici')throws andcreateRequire(rt.resolve('urllib'))('undici')succeeds. The real-build test can still pass on hoisted installs, so that branch is not pinned yet.🤖 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 404 - 415, Add coverage for the successful urllib fallback path in __installWebGlobalsLazy, since the current snapshot only tests direct undici resolution and total failure. Extend the lazy-web-globals test setup to simulate rt('undici') throwing while createRequire(rt.resolve('urllib')) can resolve undici, and assert the fetch family is installed successfully through that fallback. Use the existing sandbox.__RUNTIME_REQUIRE and __installWebGlobalsLazy symbols to pin the pnpm-specific branch in tools/egg-bundler/src/lib/prelude.ts without relying on hoisted installs.
🤖 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 `@packages/typings/src/global.ts`:
- Around line 17-18: Narrow the __RUNTIME_REQUIRE.resolve type so it matches
Node’s require.resolve options instead of accepting unknown. Update the
declaration in global.ts to use the proper options shape for
createRequire().resolve, namely an object with an optional paths array, and keep
the existing __RUNTIME_REQUIRE symbol intact while refining only the resolve
signature.
In `@site/docs/advanced/snapshot.md`:
- Line 79: The snapshot docs incorrectly say Blob and File are deleted at build
time; update the snapshot explanation in snapshot.md to match the implementation
in prelude.ts by stating that only the undici-backed web globals are deleted and
Blob/File are retained. Refer to the bundled snapshot behavior described around
egg-bin snapshot build and align the wording with
DELETED_WEB_GLOBALS/BUFFER_WEB_GLOBALS so Blob and File are either removed from
the deleted list or explicitly called out as preserved.
In `@site/docs/zh-CN/advanced/snapshot.md`:
- Line 79: 文档中的快照删除列表把 Blob 和 File 也写成会在构建阶段被删除，但实际在 egg-bundler 的 prelude
逻辑里它们是通过 node:buffer 保留并可序列化的。请在 snapshot 文档中基于 DELETED_WEB_GLOBALS /
UNDICI_WEB_GLOBALS / BUFFER_WEB_GLOBALS 的真实行为修正描述：从删除清单移除 Blob 和
File，或明确说明它们会被保留且仅对 undici 相关全局对象执行删除。
In `@tools/egg-bundler/test/snapshot-lazy.realbuild.test.ts`:
- Around line 200-201: The test child process is exiting too soon after the
WGPROBE marker is written, which can drop buffered stdout and make the snapshot
parse flaky. Update the `server.close()` termination path in
`snapshot-lazy.realbuild.test.ts` so the probe write is fully flushed before
exit, by waiting on the `process.stdout.write` callback or otherwise draining
output before calling `process.exit(0)`.
---
Nitpick comments:
In `@tools/egg-bundler/test/snapshot-lazy-external.test.ts`:
- Around line 120-128: The snapshot prelude test is asserting the wrong
`exports` wiring and could miss a regression in the CommonJS shape used by UMD
factories. Tighten the `renderSnapshotPrelude()` assertions in
`snapshot-lazy-external.test.ts` so they verify the exact `globalThis.exports =
globalThis.module && globalThis.module.exports` form rather than the looser
`globalThis.module` substring; keep the existing checks around the snapshot-only
branch and the `module/exports` setup.
- Around line 404-415: Add coverage for the successful urllib fallback path in
__installWebGlobalsLazy, since the current snapshot only tests direct undici
resolution and total failure. Extend the lazy-web-globals test setup to simulate
rt('undici') throwing while createRequire(rt.resolve('urllib')) can resolve
undici, and assert the fetch family is installed successfully through that
fallback. Use the existing sandbox.__RUNTIME_REQUIRE and __installWebGlobalsLazy
symbols to pin the pnpm-specific branch in tools/egg-bundler/src/lib/prelude.ts
without relying on hoisted installs.
🪄 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: 77e13319-b736-446c-b8a1-b26e30380065
⛔ Files ignored due to path filters (1)
tools/egg-bundler/test/__snapshots__/EntryGenerator.worker.canonical.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
packages/typings/src/global.tssite/docs/advanced/snapshot.mdsite/docs/zh-CN/advanced/snapshot.mdtools/egg-bundler/src/lib/EntryGenerator.tstools/egg-bundler/src/lib/prelude.tstools/egg-bundler/test/snapshot-lazy-external.test.tstools/egg-bundler/test/snapshot-lazy.realbuild.test.ts
killagu
force-pushed
the
fix/snapshot-restore-web-globals
branch
from
96dbd94 to
f96258a
Compare
killagu
force-pushed
the
fix/snapshot-restore-web-globals
branch
from
f96258a to
9f099e2
Compare
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/test/EntryGenerator.test.ts`:
- Around line 303-304: The current EntryGenerator.test assertions only check
that the delete statements appear in the generated worker text, but they do not
verify that they run before snapshot serialization. Update the test around the
worker snapshot setup to assert ordering relative to
v8.startupSnapshot.setDeserializeMainFunction, using the existing worker
generation checks and the deserialize callback setup to ensure the deletions
happen before serialization.
🪄 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: 599bffd3-98e1-40f0-94a9-b7fc49cf2f2a
⛔ Files ignored due to path filters (1)
tools/egg-bundler/test/__snapshots__/EntryGenerator.worker.canonical.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
packages/typings/src/global.tssite/docs/advanced/snapshot.mdsite/docs/zh-CN/advanced/snapshot.mdtools/egg-bundler/src/lib/EntryGenerator.tstools/egg-bundler/src/lib/prelude.tstools/egg-bundler/test/EntryGenerator.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 (3)
- site/docs/zh-CN/advanced/snapshot.md
- site/docs/advanced/snapshot.md
- packages/typings/src/global.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tools/egg-bundler/test/snapshot-lazy-external.test.ts
- tools/egg-bundler/src/lib/prelude.ts
| expect(worker).toContain('delete (globalThis as Record<string, unknown>).module'); | ||
| expect(worker).toContain('delete (globalThis as Record<string, unknown>).exports'); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the deletion happens before snapshot serialization.
These toContain checks don’t verify the timing described above; they would still pass if the delete statements moved inside the deserialize callback. Add an ordering assertion against v8.startupSnapshot.setDeserializeMainFunction.
Suggested test tightening
expect(worker).toContain('delete (globalThis as Record<string, unknown>).module');
expect(worker).toContain('delete (globalThis as Record<string, unknown>).exports');
+ expect(worker.indexOf('delete (globalThis as Record<string, unknown>).module')).toBeLessThan(
+ worker.indexOf('v8.startupSnapshot.setDeserializeMainFunction'),
+ );
+ expect(worker.indexOf('delete (globalThis as Record<string, unknown>).exports')).toBeLessThan(
+ worker.indexOf('v8.startupSnapshot.setDeserializeMainFunction'),
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(worker).toContain('delete (globalThis as Record<string, unknown>).module'); | |
| expect(worker).toContain('delete (globalThis as Record<string, unknown>).exports'); | |
| expect(worker).toContain('delete (globalThis as Record<string, unknown>).module'); | |
| expect(worker).toContain('delete (globalThis as Record<string, unknown>).exports'); | |
| expect(worker.indexOf('delete (globalThis as Record<string, unknown>).module')).toBeLessThan( | |
| worker.indexOf('v8.startupSnapshot.setDeserializeMainFunction'), | |
| ); | |
| expect(worker.indexOf('delete (globalThis as Record<string, unknown>).exports')).toBeLessThan( | |
| worker.indexOf('v8.startupSnapshot.setDeserializeMainFunction'), | |
| ); |
🤖 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/EntryGenerator.test.ts` around lines 303 - 304, The
current EntryGenerator.test assertions only check that the delete statements
appear in the generated worker text, but they do not verify that they run before
snapshot serialization. Update the test around the worker snapshot setup to
assert ordering relative to v8.startupSnapshot.setDeserializeMainFunction, using
the existing worker generation checks and the deserialize callback setup to
ensure the deletions happen before serialization.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/egg-bundler/test/snapshot-lazy.realbuild.test.ts (1)
165-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign this regression with the actual
Blob/Filecontract.Line 165 still describes
Blobas a deleted undici-backed global, but this PR’s contract is the opposite: only the undici globals are deleted, whileBlob/Filestay buffer-backed and are only safety-net reinstalled. Since this realbuild test only probesBlob, aFileregression would still slip through.Suggested change
- it('re-installs deleted web globals (fetch/Headers/Blob) backed by real undici at restore', async () => { + it('re-installs undici globals and keeps buffer-backed Blob/File usable at restore', async () => { @@ - // Simulate the snapshot restore-main inline: install __RUNTIME_REQUIRE (with a - // resolve pointing where urllib/undici live), call the prelude's installer, then - // exercise the now-deleted web globals with a real fetch against a local server. + // Simulate the snapshot restore-main inline: install __RUNTIME_REQUIRE (with a + // resolve pointing where urllib/undici live), call the prelude's installer, then + // exercise the restored undici globals plus the buffer-backed Blob/File path. @@ ' const out = {', ' installerPresent: typeof globalThis.__installWebGlobalsLazy,', ' fetchType: typeof globalThis.fetch,', ' BlobType: typeof globalThis.Blob,', + ' FileType: typeof globalThis.File,', ' };', @@ " out.headerOk = new Headers({ x: '1' }).get('x') === '1';", " out.blobText = await new Blob(['z']).text();", + " out.fileName = new File(['z'], 'z.txt').name;", ' } catch (e) { out.err = String((e && e.message) || e); }', @@ expect(probe.headerOk).toBe(true); // real undici Headers expect(probe.BlobType).toBe('function'); expect(probe.blobText).toBe('z'); // real node:buffer Blob + expect(probe.FileType).toBe('function'); + expect(probe.fileName).toBe('z.txt'); // real node:buffer FileAlso applies to: 189-198, 226-231
🤖 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 165 - 173, The snapshot restore test description and coverage in snapshot-lazy.realbuild.test.ts are misaligned with the current Blob/File contract: it should say only the undici-backed globals are deleted, while Blob/File remain buffer-backed and are safety-net reinstalled. Update the test setup/comment around the restore path to reflect that behavior, and expand the realbuild assertions in the affected test cases (including the ones exercising the restored globals) so they validate both Blob and File instead of only Blob, using the existing restore flow and global-install logic as the reference points.
🤖 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 165-173: The snapshot restore test description and coverage in
snapshot-lazy.realbuild.test.ts are misaligned with the current Blob/File
contract: it should say only the undici-backed globals are deleted, while
Blob/File remain buffer-backed and are safety-net reinstalled. Update the test
setup/comment around the restore path to reflect that behavior, and expand the
realbuild assertions in the affected test cases (including the ones exercising
the restored globals) so they validate both Blob and File instead of only Blob,
using the existing restore flow and global-install logic as the reference
points.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ff22109-f2ec-41a4-8a7a-9d00356fe938
⛔ Files ignored due to path filters (1)
tools/egg-bundler/test/__snapshots__/EntryGenerator.worker.canonical.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
packages/typings/src/global.tssite/docs/advanced/snapshot.mdsite/docs/zh-CN/advanced/snapshot.mdtools/egg-bundler/src/lib/EntryGenerator.tstools/egg-bundler/src/lib/prelude.tstools/egg-bundler/test/EntryGenerator.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)
- site/docs/advanced/snapshot.md
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/typings/src/global.ts
- tools/egg-bundler/test/EntryGenerator.test.ts
- tools/egg-bundler/src/lib/EntryGenerator.ts
- tools/egg-bundler/src/lib/prelude.ts
- tools/egg-bundler/test/snapshot-lazy-external.test.ts
killagu
force-pushed
the
fix/snapshot-restore-web-globals
branch
from
9f099e2 to
ff17536
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #6012 +/- ##
=======================================
Coverage 81.94% 81.95%
=======================================
Files 677 677
Lines 20651 20651
Branches 4099 4099
=======================================
+ Hits 16922 16924 +2
+ Misses 3215 3214 -1
+ Partials 514 513 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 10-15: The build-time stub pass in prelude setup is incorrectly
including BUFFER_WEB_GLOBALS in the WEB_GLOBALS delete/stub loop, which causes
Blob/File captures to stay stuck on WebGlobalStub during snapshot build. Update
the logic around WEB_GLOBALS and the restore installer so the delete/stub loop
remains undici-only, and keep BUFFER_WEB_GLOBALS referenced only when restoring
globals from globalThis.
In `@tools/egg-bundler/test/EntryGenerator.test.ts`:
- Around line 299-300: The current EntryGenerator test only checks that the
restore wiring strings exist, but not their execution order. Update the
assertions in EntryGenerator.test around the worker entry generation to verify
that `globalThis.__RUNTIME_REQUIRE = __runtimeRequire` appears before
`__runtimeRequire.resolve = ...`, and that both appear before
`globalThis.__installWebGlobalsLazy?.()`, so the test catches regressions in the
wiring sequence.
In `@tools/egg-bundler/test/snapshot-lazy-external.test.ts`:
- Around line 94-100: The current snapshot test only asserts the generated
source contains the urllib fallback, but it does not exercise the actual
recovery path in __installWebGlobalsLazy. Extend snapshot-lazy-external.test.ts
with a VM-based case that runs the installed helper while
__RUNTIME_REQUIRE('undici') throws and __RUNTIME_REQUIRE.resolve('urllib')
succeeds, then verify the fallback path is used and the lazy globals are still
installed correctly.
- Around line 113-120: The snapshot assertion is hardcoding the HTTP max header
size instead of matching the build Node output. Update the
`snapshot-lazy-external.test.ts` check around `renderSnapshotPrelude()` to
assert against `http.maxHeaderSize`, and make the same change in
`snapshot-lazy.realbuild.test.ts` so both tests derive the value from
`node:http` consistently.
🪄 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: 7b199903-76c8-4d58-9f38-66ee7d9b976c
⛔ Files ignored due to path filters (1)
tools/egg-bundler/test/__snapshots__/EntryGenerator.worker.canonical.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
packages/typings/src/global.tssite/docs/advanced/snapshot.mdsite/docs/zh-CN/advanced/snapshot.mdtools/egg-bundler/src/lib/EntryGenerator.tstools/egg-bundler/src/lib/prelude.tstools/egg-bundler/test/EntryGenerator.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)
- packages/typings/src/global.ts
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: 4
🤖 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 10-15: The build-time stub pass in prelude setup is incorrectly
including BUFFER_WEB_GLOBALS in the WEB_GLOBALS delete/stub loop, which causes
Blob/File captures to stay stuck on WebGlobalStub during snapshot build. Update
the logic around WEB_GLOBALS and the restore installer so the delete/stub loop
remains undici-only, and keep BUFFER_WEB_GLOBALS referenced only when restoring
globals from globalThis.
In `@tools/egg-bundler/test/EntryGenerator.test.ts`:
- Around line 299-300: The current EntryGenerator test only checks that the
restore wiring strings exist, but not their execution order. Update the
assertions in EntryGenerator.test around the worker entry generation to verify
that `globalThis.__RUNTIME_REQUIRE = __runtimeRequire` appears before
`__runtimeRequire.resolve = ...`, and that both appear before
`globalThis.__installWebGlobalsLazy?.()`, so the test catches regressions in the
wiring sequence.
In `@tools/egg-bundler/test/snapshot-lazy-external.test.ts`:
- Around line 94-100: The current snapshot test only asserts the generated
source contains the urllib fallback, but it does not exercise the actual
recovery path in __installWebGlobalsLazy. Extend snapshot-lazy-external.test.ts
with a VM-based case that runs the installed helper while
__RUNTIME_REQUIRE('undici') throws and __RUNTIME_REQUIRE.resolve('urllib')
succeeds, then verify the fallback path is used and the lazy globals are still
installed correctly.
- Around line 113-120: The snapshot assertion is hardcoding the HTTP max header
size instead of matching the build Node output. Update the
`snapshot-lazy-external.test.ts` check around `renderSnapshotPrelude()` to
assert against `http.maxHeaderSize`, and make the same change in
`snapshot-lazy.realbuild.test.ts` so both tests derive the value from
`node:http` consistently.
🪄 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: 7b199903-76c8-4d58-9f38-66ee7d9b976c
⛔ Files ignored due to path filters (1)
tools/egg-bundler/test/__snapshots__/EntryGenerator.worker.canonical.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
packages/typings/src/global.tssite/docs/advanced/snapshot.mdsite/docs/zh-CN/advanced/snapshot.mdtools/egg-bundler/src/lib/EntryGenerator.tstools/egg-bundler/src/lib/prelude.tstools/egg-bundler/test/EntryGenerator.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)
- packages/typings/src/global.ts
🛑 Comments failed to post (4)
tools/egg-bundler/src/lib/prelude.ts (1)
10-15: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep
Blob/Fileout of the build-time stub pass.
WEB_GLOBALSnow feeds the delete/stub loop, so includingBUFFER_WEB_GLOBALSmakes any module that capturesBloborFileduring snapshot build keepWebGlobalStubforever; the restore installer only updatesglobalThis. Keep the stub list undici-only and retainBUFFER_WEB_GLOBALSjust for restore fallback.Proposed fix
- * 1. Node's web globals (fetch/Headers/.../File/Blob) are replaced with plain JS + * 1. Node's undici-backed web globals (fetch/Headers/...) are replaced with plain JS ... - * 2. `node:buffer.File/Blob` getters are stubbed for the same reason (reading - * them lazily initializes the undici/http stack). + * 2. `Blob`/`File` are left intact when Node provides them; restore can reinstall + * them from `node:buffer` if needed. ... -const WEB_GLOBALS: readonly string[] = [...UNDICI_WEB_GLOBALS, ...BUFFER_WEB_GLOBALS]; +const WEB_GLOBALS: readonly string[] = [...UNDICI_WEB_GLOBALS];Also applies to: 92-92, 176-182
🤖 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 10 - 15, The build-time stub pass in prelude setup is incorrectly including BUFFER_WEB_GLOBALS in the WEB_GLOBALS delete/stub loop, which causes Blob/File captures to stay stuck on WebGlobalStub during snapshot build. Update the logic around WEB_GLOBALS and the restore installer so the delete/stub loop remains undici-only, and keep BUFFER_WEB_GLOBALS referenced only when restoring globals from globalThis.tools/egg-bundler/test/EntryGenerator.test.ts (1)
299-300: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the restore wiring order, not just presence.
__installWebGlobalsLazy()readsglobalThis.__RUNTIME_REQUIREimmediately. These checks still pass if the generated entry invokes the installer before publishing__runtimeRequireontoglobalThis, so the test would miss the regression this PR is protecting against. Please assert the order ofglobalThis.__RUNTIME_REQUIRE = __runtimeRequire,__runtimeRequire.resolve = ..., andglobalThis.__installWebGlobalsLazy?.().🤖 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/EntryGenerator.test.ts` around lines 299 - 300, The current EntryGenerator test only checks that the restore wiring strings exist, but not their execution order. Update the assertions in EntryGenerator.test around the worker entry generation to verify that `globalThis.__RUNTIME_REQUIRE = __runtimeRequire` appears before `__runtimeRequire.resolve = ...`, and that both appear before `globalThis.__installWebGlobalsLazy?.()`, so the test catches regressions in the wiring sequence.tools/egg-bundler/test/snapshot-lazy-external.test.ts (2)
94-100: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Probe whether the existing tests already exercise the fallback branch by building a small # read-only model of the require behavior from the implementation. python3 - <<'PY' from pathlib import Path text = Path('tools/egg-bundler/src/lib/prelude.ts').read_text() # Print the exact fallback sequence around the branch for inspection. keys = [ "rt('undici')", "rt.resolve('urllib')", "createRequire", "getBuiltin('node:buffer')", "http.maxHeaderSize", ] for key in keys: idx = text.find(key) print(f"\n=== {key} @ {idx} ===") if idx >= 0: print(text[max(0, idx-180): idx+260]) PYRepository: eggjs/egg
Length of output: 2528
🏁 Script executed:
#!/bin/bash set -euo pipefail # Read the changed test block and any surrounding VM cases to see whether the fallback branch # is exercised or only asserted by substring matching. sed -n '1,220p' tools/egg-bundler/test/snapshot-lazy-external.test.tsRepository: eggjs/egg
Length of output: 10659
Add a VM case for the
urllibfallback path. This only checks forrt.resolve('urllib')in the generated source; it never runs__installWebGlobalsLazywith__RUNTIME_REQUIRE('undici')throwing and__RUNTIME_REQUIRE.resolve('urllib')succeeding, so the recovery path remains uncovered.🤖 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 94 - 100, The current snapshot test only asserts the generated source contains the urllib fallback, but it does not exercise the actual recovery path in __installWebGlobalsLazy. Extend snapshot-lazy-external.test.ts with a VM-based case that runs the installed helper while __RUNTIME_REQUIRE('undici') throws and __RUNTIME_REQUIRE.resolve('urllib') succeeds, then verify the fallback path is used and the lazy globals are still installed correctly.
113-120: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== file list ==\n' git ls-files 'tools/egg-bundler/test/*' | sed -n '1,120p' printf '\n== search ==\n' rg -n "renderSnapshotPrelude|__HTTP_CONSTS|maxHeaderSize|STATUS_CODES|METHODS" tools/egg-bundler -S printf '\n== AGENTS files ==\n' git ls-files | rg '(^|/)AGENTS\.md$'Repository: eggjs/egg
Length of output: 9873
Derive
http.maxHeaderSizein both snapshot tests
renderSnapshotPrelude()emits whatevernode:httpreports on the build Node, so the hardcoded16384can break when Node changes its default. Usehttp.maxHeaderSizehere and intools/egg-bundler/test/snapshot-lazy.realbuild.test.tsas well.🤖 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 113 - 120, The snapshot assertion is hardcoding the HTTP max header size instead of matching the build Node output. Update the `snapshot-lazy-external.test.ts` check around `renderSnapshotPrelude()` to assert against `http.maxHeaderSize`, and make the same change in `snapshot-lazy.realbuild.test.ts` so both tests derive the value from `node:http` consistently.
|
Rebased onto I confirmed the bug still reproduces on The current change:
The earlier review comments are addressed in this version (or obsolete — e.g. the UMD Verification: real |
The snapshot prelude replaces Node's undici-backed web globals (fetch/Headers/ Request/Response/FormData/WebSocket/EventSource/MessageEvent/CloseEvent) with constructable stubs at build time so touching them never pulls in undici's non-serializable native bindings — but the restored process kept the stubs, so `globalThis.fetch` was a dead no-op after restore. The generated snapshot-restore entry now calls a new `globalThis.__installWebGlobalsLazy` (defined by the prelude) after installing `__RUNTIME_REQUIRE` (which now carries `.resolve`). It re-installs the globals as lazy accessors: the fetch family from the app's real `undici` (kept external — e.g. `--force-external undici urllib` — so it loads for real on restore; resolved directly, or through `urllib` under pnpm where undici is not hoisted), and `Blob`/`File` from `node:buffer`. The accessor must not cache `undefined`, because undici reads `globalThis.Headers` re-entrantly while its own require() is in flight. `Blob`/`File` are no longer stubbed on `node:buffer`: they are buffer-backed, serialize into a snapshot fine, and stay real (verified the cnpmcore snapshot still builds → restores → serves `/-/ping` with them un-stubbed). So `globalThis.fetch(...)` and the related constructors work normally after a restore. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
killagu
force-pushed
the
fix/snapshot-restore-web-globals
branch
from
eef7802 to
9b00b68
Compare
Motivation
In a bundled V8 startup snapshot, the prelude replaces Node's undici-backed web globals (
fetch/Headers/Request/Response/FormData/WebSocket/EventSource/MessageEvent/CloseEvent) with constructable stubs at build time — touching them pulls in undici's nativeHTTPParser/http2 bindings, which a snapshot can't serialize. But the restored process kept the stubs, soglobalThis.fetch(...)was a dead no-op after restore. (This is the "Web globals are no-op stubs after restore" known limitation from the docs.)What changed
__RUNTIME_REQUIRE(now carrying.resolve) and then calls a newglobalThis.__installWebGlobalsLazy(defined by the prelude, serialized into the blob). It re-installs the web globals as lazy accessors:undici— kept external (e.g.--force-external undici urllib, as the cnpmcore e2e already does), so it loads for real on restore; resolved directly, or throughurllibunder pnpm where undici isn't hoisted;Blob/Filefromnode:buffer.undefined: undici readsglobalThis.Headersre-entrantly while its ownrequire()is still in flight, so caching there would poisonHeaders.Blob/Fileare no longer stubbed onnode:buffer. They arenode:buffer-backed (not undici), serialize into a snapshot fine, and stay real — the previous buffer-getter stub made them unrecoverable at restore. Verified the cnpmcore snapshot still serializes with them un-stubbed.Net effect:
globalThis.fetch(...)and the related constructors work normally after a restore. Docs updated (EN + ZH): the limitation becomes a "works after restore" description, with the one remaining caveat (a web global captured at build time —const f = fetch, orclass X extends globalThis.Request— freezes the stub; reference web globals at call time).Test evidence
@utoo/pack-build tests insnapshot-lazy-external.test.tsandsnapshot-lazy.realbuild.test.ts: lazy re-install from undici/node:buffer, re-entrancy, replace-stub-but-keep-genuine, non-configurable skip, and a real build where the prelude stubs the globals and the installer brings back real undici-backedfetch/Headers/Blob.node --build-snapshot+ restore on Node 24: all web globals work — realfetchround-trip +new Headers+new Blob.next's recipe:--force-external undici urllib @cnpmjs/packument koa-onerror, no manual stubs): build → restore →GET /-/pingHTTP 200, with the real ORM/HTTP client live at restore.egg-bundlersuite green except pre-existing macOS-only env failures; lint / format / typecheck clean.🤖 Generated with Claude Code
Summary by CodeRabbit
__installWebGlobalsLazy(includingfetch/Headers/Request/Response), withBlob/Filerestored fromnode:buffer.__RUNTIME_REQUIRE.resolvefor resolver lookups.node:bufferweb types.