refactor(egg): remove buildSnapshot/restoreSnapshot that never worked under --build-snapshot#5861
Open
refactor(egg): remove buildSnapshot/restoreSnapshot that never worked under --build-snapshot#5861
Conversation
… under --build-snapshot The `buildSnapshot()` / `restoreSnapshot()` facade added in #5856 cannot work under `node --build-snapshot`. It was merged because its tests exercised the `triggerSnapshotWillSerialize` / `triggerSnapshotDidDeserialize` hooks from a normal Node.js process and mocked V8's startup snapshot API — they never ran the real `--build-snapshot` environment, which is where the architectural constraints manifest: 1. Node 22's `internal/main/mksnapshot.js` compiles the entry via `compileSerializeMain` and hands it a custom `require` that only resolves **builtin** modules. Any `require('egg')`, `require('koa')`, or other userland import throws `MODULE_NOT_FOUND`. ESM entry scripts are rejected (`SyntaxError: Cannot use import statement outside a module`) and `dynamic import()` throws `Error: Not supported`. The facade's `await startEgg({ snapshot: true })` call-chain is unreachable from inside mksnapshot without a bundle — and the `buildSnapshot` API itself provided no bundling. 2. Even if the framework is inlined via an esbuild bundle so the top of the snapshot builder can reach egg at all, V8 snapshot serialization crashes with `async hook stack has become corrupted (actual: 8, expected: 0)`. `BuildSnapshotWithoutCodeCache` calls `SpinEventLoopInternal` before dumping the heap, which fires pending libuv timer callbacks; each callback pushes an `async_context` via `InternalCallbackScope`, and the stack is never drained back to depth 0 because egg's loader inherently creates timers, AsyncLocalStorage contexts, and setImmediate callbacks during `agent.ready()` / `application.ready()`. Switching to `metadataOnly: true` does not help — the app loader path still creates pending timers. The `triggerSnapshotWillSerialize` lifecycle hook has no visibility into libuv's internal timer queue and cannot drain it. A viable "pre-warm app instance" snapshot would require either a fork-based mechanism (spawn a worker, run egg to ready, `v8.serialize()` to a custom blob, restore in a fresh process) or a framework-wide rewrite of egg's loader to be fully synchronous — both of which are separate projects outside this branch's scope. Changes: - Delete `packages/egg/src/lib/snapshot.ts` (the two broken functions and the `__egg_snapshot_app` global) - Remove the export from `packages/egg/src/index.ts` - Drop `buildSnapshot` / `restoreSnapshot` from the exports snapshot in `packages/egg/test/__snapshots__/index.test.ts.snap` - Remove the `restoreSnapshot` test case from `packages/egg/test/snapshot.test.ts`; document at the top of the file that the remaining tests run under a normal Node process rather than `--build-snapshot` - Extend the JSDoc on `Lifecycle.triggerSnapshotWillSerialize` in `packages/core/src/lifecycle.ts` to explain the constraint and why the hook mechanism is retained as a forward-compatible abstraction Kept intentionally: - `triggerSnapshotWillSerialize` / `triggerSnapshotDidDeserialize` methods on `Lifecycle` and `EggApplicationCore` — general-purpose resource-cleanup hooks that a future non-mksnapshot serialization mechanism can drive - The `snapshot?: boolean` option on `StartEggOptions` and the guards in `egg.ts` / `agent.ts` / `start.ts` — inert without a caller but harmless, and match the hook infrastructure they were designed for - `packages/core/test/snapshot.test.ts` — unit tests for the hook mechanism itself Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe PR removes the V8 startup snapshot implementation from the egg package, including Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Deploying egg with
|
| Latest commit: |
c4a609c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://99dd8fb7.egg-cci.pages.dev |
| Branch Preview URL: | https://fix-remove-nonworking-snapsh.egg-cci.pages.dev |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes the buildSnapshot() / restoreSnapshot() public APIs from packages/egg because they cannot function under a real node --build-snapshot (mksnapshot) environment, while keeping the core snapshot lifecycle hooks as a forward-compatible abstraction.
Changes:
- Deleted
packages/egg/src/lib/snapshot.tsand stopped exporting snapshot facade APIs frompackages/egg/src/index.ts. - Removed
restoreSnapshottest coverage and updated the public-export snapshot to reflect the removed exports. - Expanded the
Lifecycle.triggerSnapshotWillSerialize()doc comment to clarify intended usage and current limitations under--build-snapshot.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/egg/test/snapshot.test.ts | Removes restoreSnapshot-specific tests and clarifies that remaining tests only cover lifecycle hooks in normal Node runtime. |
| packages/egg/test/snapshots/index.test.ts.snap | Updates export surface snapshot to remove buildSnapshot / restoreSnapshot. |
| packages/egg/src/lib/snapshot.ts | Removes the non-working snapshot facade implementation entirely. |
| packages/egg/src/index.ts | Stops re-exporting the removed snapshot utilities. |
| packages/core/src/lifecycle.ts | Updates documentation for snapshot lifecycle hook intent and current lack of a --build-snapshot driver. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5861 +/- ##
==========================================
+ Coverage 86.00% 86.04% +0.04%
==========================================
Files 667 666 -1
Lines 18945 18928 -17
Branches 3652 3648 -4
==========================================
- Hits 16294 16287 -7
+ Misses 2297 2289 -8
+ Partials 354 352 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Deploying egg-v3 with
|
| Latest commit: |
c4a609c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f7675eb4.egg-v3.pages.dev |
| Branch Preview URL: | https://fix-remove-nonworking-snapsh.egg-v3.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/egg/test/snapshot.test.ts (1)
80-184: Well-structured lifecycle hook tests.The tests comprehensively verify resource cleanup on serialize and restoration on deserialize for messenger, loggers, and the unhandledRejection handler. The use of
process.nextTick()to wait for async lifecycle hooks is a standard Node.js testing pattern.Consider adding a test case for the error path where
triggerSnapshotWillSerialize()is called withoutsnapshot: true. Based on the lifecycle implementation (which throws in this case per context snippet 1), verifying this guard would improve coverage:it('should throw when calling triggerSnapshotWillSerialize without snapshot mode', async () => { const app = new Application({ baseDir: demoApp, mode: 'single', // snapshot: false (default) }); await app.ready(); await assert.rejects( () => app.triggerSnapshotWillSerialize(), /can only be called on a snapshot-mode lifecycle/ ); await app.close(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/egg/test/snapshot.test.ts` around lines 80 - 184, Add a test that verifies Application.triggerSnapshotWillSerialize throws when the app is not started in snapshot mode: create an Application without snapshot: true, await app.ready(), then use assert.rejects(() => app.triggerSnapshotWillSerialize(), /can only be called on a snapshot-mode lifecycle/) to assert the error, and finally await app.close() to clean up; reference the Application constructor and the triggerSnapshotWillSerialize method to locate where to add the case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/egg/test/snapshot.test.ts`:
- Around line 80-184: Add a test that verifies
Application.triggerSnapshotWillSerialize throws when the app is not started in
snapshot mode: create an Application without snapshot: true, await app.ready(),
then use assert.rejects(() => app.triggerSnapshotWillSerialize(), /can only be
called on a snapshot-mode lifecycle/) to assert the error, and finally await
app.close() to clean up; reference the Application constructor and the
triggerSnapshotWillSerialize method to locate where to add the case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 56b517f6-5432-4aa0-a348-8e6a7a31b21c
⛔ Files ignored due to path filters (1)
packages/egg/test/__snapshots__/index.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
packages/core/src/lifecycle.tspackages/egg/src/index.tspackages/egg/src/lib/snapshot.tspackages/egg/test/snapshot.test.ts
💤 Files with no reviewable changes (2)
- packages/egg/src/index.ts
- packages/egg/src/lib/snapshot.ts
![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 removes the buildSnapshot and restoreSnapshot utility functions from the egg package. These facades were removed because they are incompatible with Node 22's mksnapshot constraints, specifically regarding blocked userland require() calls and async context issues. The underlying lifecycle hooks, such as snapshotWillSerialize, are retained as stable abstractions for potential future serialization mechanisms. Corresponding tests and exports have been updated to reflect these removals. I have no feedback to provide.
Open
3 tasks
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.
Summary
Remove
buildSnapshot()/restoreSnapshot()frompackages/egg— they never worked in a realnode --build-snapshotenvironment.Keep the
triggerSnapshotWillSerialize/triggerSnapshotDidDeserializelifecycle hooks inpackages/core/src/lifecycle.tsas a forward-compatible abstraction for future serialization mechanisms (e.g. fork-based snapshot).Why
PR #5856 shipped
buildSnapshot()/restoreSnapshot()with passing tests. The tests exercised the hooks from a normal Node.js process and mocked V8's startup snapshot API — they never ran the real `--build-snapshot` environment, which is where the architectural constraints manifest.Verified during Track B spike:
minimalRunCjsonly supports builtinrequire—requireForUserSnapshotblocks any userland package (egg,koa, etc.) withMODULE_NOT_FOUND. The API's internal `startEgg()` call cannot possibly load.SpinEventLoopInternaltriggers libuv timer callbacks before serialization — causes `async hook stack has become corrupted (actual: 8, expected: 0)`. egg's loader creates timers, setImmediates, and AsyncLocalStorage contexts during load, which cannot be drained by `triggerSnapshotWillSerialize` because they live in libuv's internal state, not in any JS-reachable queue.The fix for (2) would require rewriting egg's entire async load path as pure-sync. Framework-level change, out of scope.
What's removed
What's kept
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Breaking Changes
buildSnapshotandrestoreSnapshotfunctions) from the public API.Documentation