feat(egg): add V8 startup snapshot APIs and metadata-only loading#5856
feat(egg): add V8 startup snapshot APIs and metadata-only loading#5856
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #5856 +/- ##
==========================================
+ Coverage 85.26% 85.31% +0.04%
==========================================
Files 666 667 +1
Lines 13234 13296 +62
Branches 1534 1539 +5
==========================================
+ Hits 11284 11343 +59
- Misses 1819 1823 +4
+ Partials 131 130 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Deploying egg-v3 with
|
| Latest commit: |
15150ca
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://620fbc21.egg-v3.pages.dev |
| Branch Preview URL: | https://feat-snapshot-egg.egg-v3.pages.dev |
Deploying egg with
|
| Latest commit: |
15150ca
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d6a7a4b4.egg-cci.pages.dev |
| Branch Preview URL: | https://feat-snapshot-egg.egg-cci.pages.dev |
![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 introduces support for V8 startup snapshots in the Egg.js framework, enabling faster application startup by pre-loading metadata. It adds a snapshot configuration option to skip the initialization of servers, timers, and persistent connections during the build phase. Key additions include the buildSnapshot and restoreSnapshot APIs, a loadFinished promise to track full metadata loading, and serialize/deserialize callbacks to manage resources like loggers and messengers. The utility package is also updated to allow module interception via setSnapshotModuleLoader. Feedback focuses on ensuring that cleanup hooks are correctly registered even in snapshot mode, maintaining stable event handler references, and re-applying messenger safety guards after restoration.
| this.lifecycle.registerBeforeClose(async () => { | ||
| // close all cluster clients | ||
| for (const clusterClient of this.#clusterClients) { | ||
| await closeClusterClient(clusterClient); | ||
| } | ||
| this.#clusterClients = []; | ||
|
|
||
| // single process mode will close agent before app close | ||
| if (this.type === 'application' && this.options.mode === 'single') { | ||
| await this.agent?.close(); | ||
| } | ||
| // single process mode will close agent before app close | ||
| if (this.type === 'application' && this.options.mode === 'single') { | ||
| await this.agent?.close(); | ||
| } | ||
|
|
||
| ManifestStore.flushCompileCache(); | ||
| for (const logger of this.loggers.values()) { | ||
| logger.close(); | ||
| } | ||
| this.messenger.close(); | ||
| process.removeListener('unhandledRejection', this._unhandledRejectionHandler); | ||
| }); | ||
| ManifestStore.flushCompileCache(); | ||
| for (const logger of this.loggers.values()) { | ||
| logger.close(); | ||
| } | ||
| this.messenger.close(); | ||
| process.removeListener('unhandledRejection', this._unhandledRejectionHandler); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The registerBeforeClose hook is currently guarded by !this.options.snapshot. This means that if an application is started in snapshot mode (for building a snapshot), these critical cleanup tasks (closing cluster clients, loggers, messenger, and removing listeners) will never be registered. When the application is later restored from the snapshot and eventually closed, these resources will leak.
Since registerBeforeClose merely adds functions to an internal array in the lifecycle object, it is safe to call during snapshot building. The captured closures will be part of the snapshot and will execute correctly in the restored process when app.close() is called.
// register close function
this.lifecycle.registerBeforeClose(async () => {
// close all cluster clients
for (const clusterClient of this.#clusterClients) {
await closeClusterClient(clusterClient);
}
this.#clusterClients = [];
// single process mode will close agent before app close
if (this.type === 'application' && this.options.mode === 'single') {
await this.agent?.close();
}
ManifestStore.flushCompileCache();
for (const logger of this.loggers.values()) {
logger.close();
}
this.messenger.close();
process.removeListener('unhandledRejection', this._unhandledRejectionHandler);
});| this._unhandledRejectionHandler = this._unhandledRejectionHandler.bind(this); | ||
| process.on('unhandledRejection', this._unhandledRejectionHandler); |
There was a problem hiding this comment.
Re-binding _unhandledRejectionHandler here creates a new function instance. If registerBeforeClose (or any other logic) uses process.removeListener with this._unhandledRejectionHandler, it might fail to remove the listener if the instance doesn't match.
It is better to bind the handler once in the constructor and then use that single instance for all on and removeListener calls, regardless of whether the app is in snapshot mode or not.
| v8.startupSnapshot.addDeserializeCallback(() => { | ||
| this.#startKeepAlive(); | ||
| }); |
There was a problem hiding this comment.
When the Agent is restored from a snapshot, the messenger is re-created. However, the _wrapMessenger() method (which provides safety warnings if messenger methods are called before the server has started) is not re-applied to the new messenger instance. This means the restored Agent will lack these important development-time guards.
v8.startupSnapshot.addDeserializeCallback(() => {
this.#startKeepAlive();
this._wrapMessenger();
});There was a problem hiding this comment.
Pull request overview
This PR extends Egg’s startup workflow to support V8 startup snapshots by introducing a “snapshot/metadata-only” loading mode, explicit snapshot build/restore entry points, and supporting utilities/tests to make module loading and lifecycle timing deterministic in snapshot contexts.
Changes:
- Add
snapshot: trueloading mode +loadFinishedpromise and snapshot serialize/deserialize callback registration onEggApplicationCore(plus snapshot-mode guards). - Introduce snapshot-oriented entry points:
startEggForSnapshot(),buildSnapshot(),restoreSnapshot(), and export them from the package. - Add snapshot module-loader interception support in
@eggjs/utilsimport helpers, plus new snapshot-focused tests and snapshot exports snapshots updates.
Reviewed changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/utils/src/import.ts | Adds snapshot module loader interception and adjusts getRequire() for restricted snapshot require. |
| packages/utils/test/snapshot-import.test.ts | New tests for snapshot module loader behavior and getRequire() fallback. |
| packages/utils/test/snapshots/index.test.ts.snap | Updates utils public export snapshot to include setSnapshotModuleLoader. |
| packages/egg/src/lib/egg.ts | Adds snapshot option, loadFinished, snapshot guards, and registerSnapshotCallbacks() behavior. |
| packages/egg/src/lib/agent.ts | Adds snapshot-aware keep-alive behavior and agent-specific snapshot callbacks. |
| packages/egg/src/lib/application.ts | Skips bindEvents in snapshot mode. |
| packages/egg/src/lib/start.ts | Adds startEggForSnapshot() entry point for snapshot builds. |
| packages/egg/src/lib/snapshot.ts | New buildSnapshot() / restoreSnapshot() public APIs. |
| packages/egg/src/index.ts | Exports snapshot utilities from the package entry. |
| packages/egg/test/lib/snapshot.test.ts | New test suite covering snapshot APIs, callbacks, loadFinished, and guards. |
| packages/egg/test/snapshots/index.test.ts.snap | Updates egg public export snapshot to include new snapshot APIs. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
| let _originalIsESM: boolean; | ||
|
|
||
| // We need to capture and restore isESM since setSnapshotModuleLoader mutates it. | ||
| // Use dynamic import to read the current value. | ||
| afterEach(async () => { | ||
| // Reset the snapshot loader by setting it to a no-op then clearing via | ||
| // module internals. Since there's no public "unset" API, we re-import | ||
| // and the module-level _snapshotModuleLoader remains set — but tests | ||
| // are isolated enough that this is fine. We'll use a different approach: | ||
| // just call setSnapshotModuleLoader with a passthrough that calls the | ||
| // real import, but that changes isESM. Instead, we accept that these | ||
| // tests run with the loader set and each test overrides it. | ||
| // Reset by overwriting with undefined via the setter trick: | ||
| // Actually we can't unset. Let's just re-import fresh for isolation. |
There was a problem hiding this comment.
setSnapshotModuleLoader() mutates module-level state (isESM and the internal loader) but this test suite never restores it: _originalIsESM is unused and the afterEach is effectively a no-op. Since the repo’s Vitest workspace uses isolate: false (shared module cache across files), leaving the snapshot loader installed can leak into unrelated tests and change importResolve() behavior. Please add real cleanup (restore the previous isESM and clear/unset the snapshot loader) or drop the unused variable/hook if cleanup isn’t possible.
| let _originalIsESM: boolean; | |
| // We need to capture and restore isESM since setSnapshotModuleLoader mutates it. | |
| // Use dynamic import to read the current value. | |
| afterEach(async () => { | |
| // Reset the snapshot loader by setting it to a no-op then clearing via | |
| // module internals. Since there's no public "unset" API, we re-import | |
| // and the module-level _snapshotModuleLoader remains set — but tests | |
| // are isolated enough that this is fine. We'll use a different approach: | |
| // just call setSnapshotModuleLoader with a passthrough that calls the | |
| // real import, but that changes isESM. Instead, we accept that these | |
| // tests run with the loader set and each test overrides it. | |
| // Reset by overwriting with undefined via the setter trick: | |
| // Actually we can't unset. Let's just re-import fresh for isolation. | |
| afterEach(() => { | |
| // Clear the snapshot module loader so module-level state (e.g. isESM) | |
| // does not leak into other test files when Vitest runs with isolate: false. | |
| setSnapshotModuleLoader(undefined as any); |
packages/utils/src/import.ts
Outdated
|
|
||
| /** | ||
| * Register a snapshot module loader that intercepts `importModule()` calls. | ||
| * | ||
| * When set, `importModule()` delegates to this loader instead of calling | ||
| * `import()` or `require()`. This is used by the V8 snapshot entry generator | ||
| * to provide pre-bundled modules — the bundler generates a static module map | ||
| * from the egg manifest and registers it via this API. | ||
| * | ||
| * Also sets `isESM = false` because the snapshot bundle is CJS and | ||
| * esbuild's `import.meta` polyfill causes incorrect ESM detection. | ||
| */ | ||
| export function setSnapshotModuleLoader(loader: SnapshotModuleLoader): void { | ||
| _snapshotModuleLoader = loader; | ||
| isESM = false; |
There was a problem hiding this comment.
setSnapshotModuleLoader() permanently overwrites the global isESM flag and provides no way to clear/unregister _snapshotModuleLoader. In practice this makes importModule()/importResolve() behavior order-dependent once the API is called (including across test files when Vitest shares module cache). Consider adding a supported “unset/restore” mechanism (e.g., setSnapshotModuleLoader(undefined), clearSnapshotModuleLoader(), or returning a restore function) and restoring the previous isESM value when the loader is removed.
| /** | |
| * Register a snapshot module loader that intercepts `importModule()` calls. | |
| * | |
| * When set, `importModule()` delegates to this loader instead of calling | |
| * `import()` or `require()`. This is used by the V8 snapshot entry generator | |
| * to provide pre-bundled modules — the bundler generates a static module map | |
| * from the egg manifest and registers it via this API. | |
| * | |
| * Also sets `isESM = false` because the snapshot bundle is CJS and | |
| * esbuild's `import.meta` polyfill causes incorrect ESM detection. | |
| */ | |
| export function setSnapshotModuleLoader(loader: SnapshotModuleLoader): void { | |
| _snapshotModuleLoader = loader; | |
| isESM = false; | |
| let _snapshotPreviousIsESM: boolean | undefined; | |
| /** | |
| * Register or clear a snapshot module loader that intercepts `importModule()` calls. | |
| * | |
| * When set, `importModule()` delegates to this loader instead of calling | |
| * `import()` or `require()`. This is used by the V8 snapshot entry generator | |
| * to provide pre-bundled modules — the bundler generates a static module map | |
| * from the egg manifest and registers it via this API. | |
| * | |
| * When a loader is registered, `isESM` is forced to `false` because the | |
| * snapshot bundle is CJS and esbuild's `import.meta` polyfill can cause | |
| * incorrect ESM detection. | |
| * | |
| * Passing `undefined` clears the loader and restores the previous `isESM` | |
| * value that was in effect before the first loader was registered. | |
| */ | |
| export function setSnapshotModuleLoader(loader?: SnapshotModuleLoader): void { | |
| if (loader) { | |
| // First-time registration: remember the current isESM value so we can restore it. | |
| if (!_snapshotModuleLoader) { | |
| _snapshotPreviousIsESM = isESM; | |
| } | |
| _snapshotModuleLoader = loader; | |
| isESM = false; | |
| } else { | |
| // Clear the loader and restore the previous isESM value if we recorded one. | |
| _snapshotModuleLoader = undefined; | |
| if (_snapshotPreviousIsESM !== undefined) { | |
| isESM = _snapshotPreviousIsESM; | |
| _snapshotPreviousIsESM = undefined; | |
| } | |
| } |
| if (!this.options.snapshot) { | ||
| // Listen the error that promise had not catch, then log it in common-error | ||
| this._unhandledRejectionHandler = this._unhandledRejectionHandler.bind(this); | ||
| process.on('unhandledRejection', this._unhandledRejectionHandler); | ||
|
|
||
| // register close function | ||
| this.lifecycle.registerBeforeClose(async () => { | ||
| // close all cluster clients | ||
| for (const clusterClient of this.#clusterClients) { | ||
| await closeClusterClient(clusterClient); | ||
| } | ||
| this.#clusterClients = []; | ||
| // register close function | ||
| this.lifecycle.registerBeforeClose(async () => { | ||
| // close all cluster clients | ||
| for (const clusterClient of this.#clusterClients) { | ||
| await closeClusterClient(clusterClient); | ||
| } | ||
| this.#clusterClients = []; | ||
|
|
||
| // single process mode will close agent before app close | ||
| if (this.type === 'application' && this.options.mode === 'single') { | ||
| await this.agent?.close(); | ||
| } | ||
| // single process mode will close agent before app close | ||
| if (this.type === 'application' && this.options.mode === 'single') { | ||
| await this.agent?.close(); | ||
| } | ||
|
|
||
| ManifestStore.flushCompileCache(); | ||
| for (const logger of this.loggers.values()) { | ||
| logger.close(); | ||
| } | ||
| this.messenger.close(); | ||
| process.removeListener('unhandledRejection', this._unhandledRejectionHandler); | ||
| }); | ||
| ManifestStore.flushCompileCache(); | ||
| for (const logger of this.loggers.values()) { | ||
| logger.close(); | ||
| } | ||
| this.messenger.close(); | ||
| process.removeListener('unhandledRejection', this._unhandledRejectionHandler); | ||
| }); | ||
| } |
There was a problem hiding this comment.
In snapshot mode, the app never registers the beforeClose hook that closes cluster clients, loggers, and the messenger (and removes the unhandledRejection listener). As a result, await app.close() will skip these cleanups whenever snapshot: true, which can leak file descriptors (loggers) and other resources in snapshot tooling/tests. Consider registering the beforeClose cleanup regardless of snapshot mode, and only gating the runtime-only startup pieces (e.g. startup timeout timer / dumpConfig-after-ready / initial unhandledRejection registration).
📝 WalkthroughWalkthroughThe changes introduce V8 startup snapshot support to the Egg framework, enabling applications to be serialized at initialization and restored for faster subsequent startups. The implementation includes snapshot-aware lifecycle management in core classes, new snapshot build/restore utilities, and comprehensive test coverage for the feature. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Code
participant BuildSnap as buildSnapshot()
participant StartSnap as startEggForSnapshot()
participant Agent as Agent
participant App as Application
participant V8 as V8 Snapshot
Client->>BuildSnap: buildSnapshot(options)
BuildSnap->>StartSnap: startEggForSnapshot(options)
StartSnap->>Agent: new Agent({snapshot: true, mode: 'single'})
StartSnap->>App: new Application({snapshot: true, mode: 'single'})
StartSnap->>Agent: agent.loadFinished (await)
StartSnap->>App: app.loadFinished (await)
StartSnap-->>BuildSnap: returns Application
BuildSnap->>App: registerSnapshotCallbacks()
BuildSnap->>Agent: registerSnapshotCallbacks()
BuildSnap->>V8: setDeserializeMainFunction(callback)
BuildSnap->>V8: snapshot serialization triggered
App->>V8: serialize callback: close messenger, loggers
Agent->>V8: serialize callback: stop keep-alive
V8->>V8: V8 snapshot created
sequenceDiagram
participant Runtime as Runtime
participant V8 as V8 Snapshot
participant DeserCB as Deserialize Callback
participant App as Application
participant Restore as restoreSnapshot()
participant Global as globalThis
Runtime->>V8: Snapshot deserialization starts
V8->>DeserCB: invokes deserialize callback
DeserCB->>App: recreate messenger
DeserCB->>App: re-attach egg-ready hook
DeserCB->>App: restore unhandledRejection listener
DeserCB->>App: re-patch HttpClient
DeserCB->>Global: store app as __egg_snapshot_app
Runtime->>Restore: restoreSnapshot()
Restore->>Global: retrieve __egg_snapshot_app
Restore-->>Runtime: return Application instance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tools/scripts/test/utils.ts (1)
15-20:⚠️ Potential issue | 🟡 MinorProcess filter may match unrelated processes in concurrent test runs.
The
--snapshot-blobcheck on line 19 matches any Node process system-wide containing that flag, without anchoring tobaseDir. In CI environments with parallel test suites, this could inadvertently kill processes from other test runs.Consider requiring both conditions for snapshot processes:
🛠️ Proposed fix to scope snapshot process matching
export async function cleanup(baseDir: string) { const processList = await findNodeProcess((x) => { const dir = isWindows ? baseDir.replace(/\\/g, '\\\\') : baseDir; const prefix = isWindows ? '\\"baseDir\\":\\"' : '"baseDir":"'; - return x.cmd.includes(`${prefix}${dir}`) || x.cmd.includes('--snapshot-blob'); + // Match processes by baseDir in JSON args, OR snapshot processes that reference this baseDir + const matchesBaseDir = x.cmd.includes(`${prefix}${dir}`) || x.cmd.includes(dir); + const isSnapshotProcess = x.cmd.includes('--snapshot-blob'); + return matchesBaseDir || (isSnapshotProcess && x.cmd.includes(dir)); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/scripts/test/utils.ts` around lines 15 - 20, The process filter in cleanup (function cleanup, using findNodeProcess and variable baseDir/prefix/isWindows) currently matches any node process that contains '--snapshot-blob' system-wide; change the predicate so snapshot processes are only matched if they also reference the same baseDir: i.e., require both x.cmd.includes('--snapshot-blob') AND x.cmd.includes(`${prefix}${dir}`) when matching snapshot processes (keep the existing direct baseDir match unchanged), so the filter becomes a match for baseDir OR (snapshot-blob AND baseDir).tools/scripts/src/commands/stop.ts (1)
47-63:⚠️ Potential issue | 🟡 Minor
--titlefiltering won't work for snapshot processes.Line 48 documents that snapshot processes set title via
process.title, not in args. However, the filter on lines 56-58 checkscmd.includes(format(osRelated.titleTemplate, flags.title)), which only searches command-line arguments.The underlying issue is that
item.cmdcontains only command-line arguments fromps -wweo argsoutput (see helper.ts). Snapshot mode sets title via theEGG_SERVER_TITLEenvironment variable (start.ts line 286), which doesn't appear in the command string, whereas regular mode passes--title=...as a command argument (start.ts line 317).This means
eggctl stop --title=myappwill not match snapshot-started processes.Consider either:
- Also checking environment variables in process matching (would require platform-specific process listing changes)
- Documenting that
--titlefiltering doesn't apply to snapshot processes- Passing title in snapshot process args for consistency
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/scripts/src/commands/stop.ts` around lines 47 - 63, The current process filtering in findNodeProcesses uses item.cmd and checks cmd.includes(format(osRelated.titleTemplate, flags.title)), which misses snapshot-mode processes that set title via the EGG_SERVER_TITLE env var (see start.ts). To fix this, update the matching logic in findNodeProcesses to also consider the environment-based title for snapshot processes: if flags.title is provided, treat a process as matched when either the command args include format(osRelated.titleTemplate, flags.title) OR the process environment contains EGG_SERVER_TITLE equal to flags.title; ensure you reference item.env (or the source that provides env info) when available and fall back to the existing cmd-based check when env info is absent so behavior is unchanged on platforms without env access.
🧹 Nitpick comments (4)
packages/utils/test/snapshot-import.test.ts (2)
10-10: Unused variable declaration.
_originalIsESMis declared but never assigned or used. Consider removing it.🧹 Remove unused variable
describe('setSnapshotModuleLoader', () => { - let _originalIsESM: boolean; - // We need to capture and restore isESM since setSnapshotModuleLoader mutates it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/utils/test/snapshot-import.test.ts` at line 10, The variable _originalIsESM is declared in the test but never assigned or referenced; remove the unused declaration to clean up the test file (remove the line declaring _originalIsESM in snapshot-import.test.ts), and run tests/lint to confirm no remaining references to _originalIsESM in functions or blocks associated with the snapshot import tests.
14-24: Consider adding a reset mechanism for test isolation.The
afterEachhook is effectively empty, and the comments acknowledge that there's no public API to unset_snapshotModuleLoader. While the current tests work because each overrides the loader, this creates implicit coupling between tests.Consider exporting a test-only reset function from the module, or accepting this as intentional for now and removing the misleading comments.
🧹 Simplify the no-op afterEach
- afterEach(async () => { - // Reset the snapshot loader by setting it to a no-op then clearing via - // module internals. Since there's no public "unset" API, we re-import - // and the module-level _snapshotModuleLoader remains set — but tests - // are isolated enough that this is fine. We'll use a different approach: - // just call setSnapshotModuleLoader with a passthrough that calls the - // real import, but that changes isESM. Instead, we accept that these - // tests run with the loader set and each test overrides it. - // Reset by overwriting with undefined via the setter trick: - // Actually we can't unset. Let's just re-import fresh for isolation. - }); + // Note: No reset needed — each test overrides the snapshot loader. + // Tests are ordered to avoid interference.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/utils/test/snapshot-import.test.ts` around lines 14 - 24, The afterEach comments note there's no public way to unset the module-level _snapshotModuleLoader; add a test-only reset function (e.g. export function resetSnapshotModuleLoader() in the module that clears or restores _snapshotModuleLoader to its default/no-op) and call it from this test file's afterEach to ensure isolation; update tests to import resetSnapshotModuleLoader and replace the long comment block with a single call to resetSnapshotModuleLoader (or, if you prefer not to add API, simply remove the misleading comments and leave afterEach empty).tools/scripts/src/commands/snapshot-build.ts (1)
409-409: Consider usingNumber()instead ofparseInt()for PORT.
parseInt(process.env.PORT)returnsNaNfor non-numeric strings (includingundefined), which the|| 7001fallback handles. However,parseInt("3000abc")would return3000, which might be unexpected.Using
Number()or adding explicit validation would be more robust, but this is a minor edge case.🔧 Optional: Stricter port parsing
- lines.push(' const port = parseInt(process.env.PORT) || 7001;'); + lines.push(' const port = Number(process.env.PORT) || 7001;');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/scripts/src/commands/snapshot-build.ts` at line 409, The current port parsing uses parseInt(process.env.PORT) which accepts strings like "3000abc"; update the port parsing for snapshot-build.ts so port is parsed with Number(process.env.PORT) and validated (e.g., check Number.isInteger(port) and that port is within 1–65535) before falling back to 7001; locate the expression setting the port (const port = parseInt(process.env.PORT) || 7001;) and replace it with numeric conversion plus explicit validation and fallback to ensure only a valid integer port is used.packages/utils/src/import.ts (1)
399-411: Minor redundancy in the guard condition.On line 402, the condition
obj.default?.__esModule === true && obj.default && 'default' in obj.defaulthas a redundant&& obj.default— ifobj.default?.__esModule === trueevaluates totrue, thenobj.defaultmust already be truthy.However, this mirrors the fix applied to line 426 and maintains consistency with the existing pattern, so this is a minor nit.
🔧 Optional: Simplify the guard
- if (obj && typeof obj === 'object' && obj.default?.__esModule === true && obj.default && 'default' in obj.default) { + if (obj && typeof obj === 'object' && obj.default?.__esModule === true && 'default' in obj.default) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/utils/src/import.ts` around lines 399 - 411, The guard inside the _snapshotModuleLoader branch redundantly checks "&& obj.default" after "obj.default?.__esModule === true"; update the condition in the block where you set obj = obj.default to remove the redundant && obj.default so it mirrors the later pattern (i.e., rely on obj.default?.__esModule === true and "'default' in obj.default" as the checks) and keep the subsequent importDefaultOnly branch unchanged; edit the conditional surrounding obj assignment in the _snapshotModuleLoader handling (referencing _snapshotModuleLoader, moduleFilePath, obj, and options?.importDefaultOnly) to simplify the guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/scripts/scripts/start-single.mjs`:
- Around line 12-16: The ESM loader only checks framework.start ??
framework.startEgg and misses cases where the framework exposes start/startEgg
on its default export; update the resolution in the start logic (after
importModule(options.framework)) to fall back to framework.default?.start ??
framework.default?.startEgg when typeof startEgg !== 'function', ensuring
startEgg resolves the same way as the CJS startEgg resolution.
---
Outside diff comments:
In `@tools/scripts/src/commands/stop.ts`:
- Around line 47-63: The current process filtering in findNodeProcesses uses
item.cmd and checks cmd.includes(format(osRelated.titleTemplate, flags.title)),
which misses snapshot-mode processes that set title via the EGG_SERVER_TITLE env
var (see start.ts). To fix this, update the matching logic in findNodeProcesses
to also consider the environment-based title for snapshot processes: if
flags.title is provided, treat a process as matched when either the command args
include format(osRelated.titleTemplate, flags.title) OR the process environment
contains EGG_SERVER_TITLE equal to flags.title; ensure you reference item.env
(or the source that provides env info) when available and fall back to the
existing cmd-based check when env info is absent so behavior is unchanged on
platforms without env access.
In `@tools/scripts/test/utils.ts`:
- Around line 15-20: The process filter in cleanup (function cleanup, using
findNodeProcess and variable baseDir/prefix/isWindows) currently matches any
node process that contains '--snapshot-blob' system-wide; change the predicate
so snapshot processes are only matched if they also reference the same baseDir:
i.e., require both x.cmd.includes('--snapshot-blob') AND
x.cmd.includes(`${prefix}${dir}`) when matching snapshot processes (keep the
existing direct baseDir match unchanged), so the filter becomes a match for
baseDir OR (snapshot-blob AND baseDir).
---
Nitpick comments:
In `@packages/utils/src/import.ts`:
- Around line 399-411: The guard inside the _snapshotModuleLoader branch
redundantly checks "&& obj.default" after "obj.default?.__esModule === true";
update the condition in the block where you set obj = obj.default to remove the
redundant && obj.default so it mirrors the later pattern (i.e., rely on
obj.default?.__esModule === true and "'default' in obj.default" as the checks)
and keep the subsequent importDefaultOnly branch unchanged; edit the conditional
surrounding obj assignment in the _snapshotModuleLoader handling (referencing
_snapshotModuleLoader, moduleFilePath, obj, and options?.importDefaultOnly) to
simplify the guard.
In `@packages/utils/test/snapshot-import.test.ts`:
- Line 10: The variable _originalIsESM is declared in the test but never
assigned or referenced; remove the unused declaration to clean up the test file
(remove the line declaring _originalIsESM in snapshot-import.test.ts), and run
tests/lint to confirm no remaining references to _originalIsESM in functions or
blocks associated with the snapshot import tests.
- Around line 14-24: The afterEach comments note there's no public way to unset
the module-level _snapshotModuleLoader; add a test-only reset function (e.g.
export function resetSnapshotModuleLoader() in the module that clears or
restores _snapshotModuleLoader to its default/no-op) and call it from this test
file's afterEach to ensure isolation; update tests to import
resetSnapshotModuleLoader and replace the long comment block with a single call
to resetSnapshotModuleLoader (or, if you prefer not to add API, simply remove
the misleading comments and leave afterEach empty).
In `@tools/scripts/src/commands/snapshot-build.ts`:
- Line 409: The current port parsing uses parseInt(process.env.PORT) which
accepts strings like "3000abc"; update the port parsing for snapshot-build.ts so
port is parsed with Number(process.env.PORT) and validated (e.g., check
Number.isInteger(port) and that port is within 1–65535) before falling back to
7001; locate the expression setting the port (const port =
parseInt(process.env.PORT) || 7001;) and replace it with numeric conversion plus
explicit validation and fallback to ensure only a valid integer port is used.
🪄 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: 1e3f6d8e-e144-4450-85f6-3039372088ad
⛔ Files ignored due to path filters (3)
packages/egg/test/__snapshots__/index.test.ts.snapis excluded by!**/*.snappackages/utils/test/__snapshots__/index.test.ts.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
packages/egg/src/index.tspackages/egg/src/lib/agent.tspackages/egg/src/lib/application.tspackages/egg/src/lib/egg.tspackages/egg/src/lib/snapshot.tspackages/egg/src/lib/start.tspackages/egg/test/lib/snapshot.test.tspackages/utils/src/import.tspackages/utils/test/snapshot-import.test.tstools/scripts/package.jsontools/scripts/scripts/start-single.cjstools/scripts/scripts/start-single.mjstools/scripts/src/commands/snapshot-build.tstools/scripts/src/commands/start.tstools/scripts/src/commands/stop.tstools/scripts/src/helper.tstools/scripts/src/index.tstools/scripts/test/fixtures/snapshot-app/.gitignoretools/scripts/test/fixtures/snapshot-app/app/router.jstools/scripts/test/fixtures/snapshot-app/config/config.default.jstools/scripts/test/fixtures/snapshot-app/package.jsontools/scripts/test/snapshot.test.tstools/scripts/test/utils.tstools/scripts/tsdown.config.ts
| const framework = await importModule(options.framework); | ||
| const startEgg = framework.start ?? framework.startEgg; | ||
| if (typeof startEgg !== 'function') { | ||
| throw new Error(`Cannot find start/startEgg function from framework: ${options.framework}`); | ||
| } |
There was a problem hiding this comment.
Inconsistent export resolution compared to CJS version.
The ESM version only checks framework.start ?? framework.startEgg, but the CJS version (start-single.cjs lines 12-16) also handles default exports:
// CJS version handles:
let startEgg = exports.start ?? exports.startEgg;
if (typeof startEgg !== 'function') {
startEgg = exports.default?.start ?? exports.default?.startEgg;
}This inconsistency could cause the ESM version to fail for frameworks that only expose start/startEgg via default export.
🔧 Proposed fix to align with CJS behavior
const framework = await importModule(options.framework);
-const startEgg = framework.start ?? framework.startEgg;
+let startEgg = framework.start ?? framework.startEgg;
+if (typeof startEgg !== 'function') {
+ startEgg = framework.default?.start ?? framework.default?.startEgg;
+}
if (typeof startEgg !== 'function') {
throw new Error(`Cannot find start/startEgg function from framework: ${options.framework}`);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/scripts/scripts/start-single.mjs` around lines 12 - 16, The ESM loader
only checks framework.start ?? framework.startEgg and misses cases where the
framework exposes start/startEgg on its default export; update the resolution in
the start logic (after importModule(options.framework)) to fall back to
framework.default?.start ?? framework.default?.startEgg when typeof startEgg !==
'function', ensuring startEgg resolves the same way as the CJS startEgg
resolution.
Add snapshot mode support that loads application metadata (plugins, configs, extensions, services, controllers) without starting servers, timers, or connections, enabling V8 startup snapshot construction. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
killagu
force-pushed
the
feat/snapshot-egg
branch
from
ab375cd to
15150ca
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/egg/src/lib/egg.ts (1)
616-627: Serialize callback removes listener that may not have been registered.In snapshot mode, the
unhandledRejectionlistener is never registered (guarded at line 242), but the serialize callback at line 626 attempts to remove it. Whileprocess.removeListeneris a no-op for non-existent listeners, this could be confusing for maintainers.Consider adding a comment or guard:
📝 Suggested clarification
this.messenger.close(); + // Safe no-op if listener was never registered (snapshot mode skips registration) process.removeListener('unhandledRejection', this._unhandledRejectionHandler);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/egg/src/lib/egg.ts` around lines 616 - 627, The serialize callback registered in registerSnapshotCallbacks (via v8.startupSnapshot.addSerializeCallback) calls process.removeListener('unhandledRejection', this._unhandledRejectionHandler) even when that listener may never have been registered; update the callback to either check whether the listener was added (track a boolean flag set where the listener is registered, e.g., alongside the registration code) or add an explanatory comment before the removeListener call to clarify it's intentionally tolerant of non-registered handlers; reference the existing symbols registerSnapshotCallbacks, v8.startupSnapshot.addSerializeCallback, and this._unhandledRejectionHandler when making the change so reviewers can find the relevant code.packages/egg/src/lib/start.ts (1)
98-118: Consider extracting framework resolution to a shared helper.The framework resolution logic (reading
package.json, importing framework module, deriving Agent/Application classes) is duplicated betweenstartEgg()(lines 44-60) andstartEggForSnapshot()(lines 102-118). This could be extracted into a private helper function to reduce duplication.♻️ Example refactor
+interface FrameworkClasses { + Agent: typeof Agent; + Application: typeof Application; +} + +async function resolveFrameworkClasses(baseDir: string, framework?: string): Promise<FrameworkClasses> { + let resolvedFramework = framework; + if (!resolvedFramework) { + try { + const pkg = await readJSON(path.join(baseDir, 'package.json')); + resolvedFramework = pkg.egg.framework; + } catch { + // ignore + } + } + let AgentClass = Agent; + let ApplicationClass = Application; + if (resolvedFramework) { + const frameworkModule = await importModule(resolvedFramework, { + paths: [baseDir], + }); + AgentClass = frameworkModule.Agent; + ApplicationClass = frameworkModule.Application; + } + return { Agent: AgentClass, Application: ApplicationClass }; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/egg/src/lib/start.ts` around lines 98 - 118, Extract the duplicated framework resolution in startEggForSnapshot and startEgg into a single private helper (e.g., resolveFrameworkClasses) that accepts SnapshotEggOptions/baseDir and optional framework name and returns { AgentClass, ApplicationClass } (defaulting to Agent and Application). Implement the helper to: read package.json via readJSON when framework not provided, resolve framework via importModule(paths: [baseDir]) when found, and fall back to the built-in Agent/Application; then replace the duplicated blocks in startEggForSnapshot and startEgg to call this helper and assign AgentClass/ApplicationClass from its result.
🤖 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/src/lib/egg.ts`:
- Around line 616-627: The serialize callback registered in
registerSnapshotCallbacks (via v8.startupSnapshot.addSerializeCallback) calls
process.removeListener('unhandledRejection', this._unhandledRejectionHandler)
even when that listener may never have been registered; update the callback to
either check whether the listener was added (track a boolean flag set where the
listener is registered, e.g., alongside the registration code) or add an
explanatory comment before the removeListener call to clarify it's intentionally
tolerant of non-registered handlers; reference the existing symbols
registerSnapshotCallbacks, v8.startupSnapshot.addSerializeCallback, and
this._unhandledRejectionHandler when making the change so reviewers can find the
relevant code.
In `@packages/egg/src/lib/start.ts`:
- Around line 98-118: Extract the duplicated framework resolution in
startEggForSnapshot and startEgg into a single private helper (e.g.,
resolveFrameworkClasses) that accepts SnapshotEggOptions/baseDir and optional
framework name and returns { AgentClass, ApplicationClass } (defaulting to Agent
and Application). Implement the helper to: read package.json via readJSON when
framework not provided, resolve framework via importModule(paths: [baseDir])
when found, and fall back to the built-in Agent/Application; then replace the
duplicated blocks in startEggForSnapshot and startEgg to call this helper and
assign AgentClass/ApplicationClass from its result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 49d3ebcd-c068-47c9-a96b-c260bbf97973
⛔ Files ignored due to path filters (1)
packages/egg/test/__snapshots__/index.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
packages/egg/src/index.tspackages/egg/src/lib/agent.tspackages/egg/src/lib/application.tspackages/egg/src/lib/egg.tspackages/egg/src/lib/snapshot.tspackages/egg/src/lib/start.tspackages/egg/test/lib/snapshot.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/egg/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/egg/src/lib/application.ts
- packages/egg/src/lib/snapshot.ts
- packages/egg/src/lib/agent.ts
- packages/egg/test/lib/snapshot.test.ts
Summary
Add V8 startup snapshot support to the egg framework package:
snapshot: trueoption triggers lifecycle cutoff afterconfigWillLoad(via PR2 core), then awaitsloadFinishedfor loader completionbuildSnapshot()/restoreSnapshot()APIs: Public entry points for snapshot construction and restorationstartEggForSnapshot(): Creates Agent + Application in single/snapshot mode, awaits full loader completion vialoadFinishedpromiseregisterSnapshotCallbacks(): Serialize/deserialize hooks for loggers, messenger, agent keep-alive timer, unhandledRejection handler, HttpClient prototypesnapshot: trueThis is PR4 of 6 in the V8 startup snapshot series. Depends on PR1 (koa), PR2 (core), PR3 (utils).
Changes (7 files, +294/-47)
packages/egg/src/lib/egg.ts— Snapshot option, loadFinished promise, registerSnapshotCallbacks(), guardspackages/egg/src/lib/agent.ts— Conditional keepalive, registerSnapshotCallbacks() overridepackages/egg/src/lib/application.ts— Skip bindEvents in snapshot modepackages/egg/src/lib/snapshot.ts— NEW: buildSnapshot/restoreSnapshot APIspackages/egg/src/lib/start.ts— NEW: startEggForSnapshot()packages/egg/src/index.ts— Export snapshot utilitiespackages/egg/test/lib/snapshot.test.ts— NEW: 18 comprehensive testsTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests