feat(core,egg): add V8 startup snapshot lifecycle hooks and APIs#5856
Open
feat(core,egg): add V8 startup snapshot lifecycle hooks and APIs#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 13238 13323 +85
Branches 1535 1543 +8
==========================================
+ Hits 11288 11366 +78
- Misses 1819 1829 +10
+ Partials 131 128 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Deploying egg-v3 with
|
| Latest commit: |
f8ff2e5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3e273994.egg-v3.pages.dev |
| Branch Preview URL: | https://feat-snapshot-egg.egg-v3.pages.dev |
Deploying egg with
|
| Latest commit: |
f8ff2e5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://38de8b78.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}
Contributor
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.
Contributor
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
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds V8 startup snapshot build/restore APIs and a snapshot-aware start path, introduces snapshot lifecycle hooks and triggers, makes agent keep-alive startup lazy, wires snapshot cleanup/restore in Egg lifecycle, exposes snapshot utilities from package entry, and adds tests for snapshot behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Builder as Build Process
participant App as Agent/Application
participant V8 as v8.StartupSnapshot
participant Runtime as Restored Process
Builder->>App: startEggForSnapshot(options)
App->>App: construct Agent & Application (mode:'single', snapshot:true)
App->>App: await agent.loadFinished & app.loadFinished
Builder->>App: triggerSnapshotWillSerialize()
App->>App: close messenger, clear loggers, remove handlers
Builder->>V8: setDeserializeMainFunction(deserializer)
V8->>Builder: create serialized snapshot
Runtime->>V8: process starts from snapshot
V8->>Runtime: invoke deserialize callback -> globalThis.__egg_snapshot_app assigned
Runtime->>App: triggerSnapshotDidDeserialize()
App->>App: recreate messenger, reattach handlers
App->>App: triggerConfigDidLoad() → didLoad → willReady → didReady
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 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}
Contributor
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
killagu
force-pushed
the
feat/snapshot-egg
branch
from
ab375cd to
15150ca
Compare
Contributor
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
killagu
force-pushed
the
feat/snapshot-egg
branch
from
15150ca to
e16fa39
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/lifecycle.ts`:
- Around line 404-470: The triggerSnapshotDidDeserialize entrypoint must only
run in snapshot mode and must be idempotent: add a guard at the top of
triggerSnapshotDidDeserialize that (1) checks an options snapshot flag (e.g.
this.options.snapshot or this.app.options.snapshot) and returns early (or
throws) if not in snapshot mode, and (2) checks/sets a private "already
restored" flag (e.g. this.#snapshotDeserialized or similar) so a second call is
a no-op; ensure this flag is set before resetting this.#readyObject, calling
this.#initReady(), this.ready(...)/this.triggerDidReady, and
this.triggerConfigDidLoad to avoid duplicate boot side-effects and beforeClose
registrations.
- Around line 404-421: The snapshot hook error handlers currently only emit
errors and continue, which lets snapshot operations proceed in an inconsistent
state; update the catch blocks in triggerSnapshotWillSerialize and
triggerSnapshotDidDeserialize to rethrow the caught error (after emitting it) so
the Promise rejects and callers can abort; specifically, inside the try/catch
around await utils.callFn(boot.snapshotWillSerialize.bind(boot)) and the
analogous call to boot.snapshotDidDeserialize, call this.emit('error', err) and
then throw err (ensuring timing.end still runs as needed or wrap timing.end in
finally if required) so the first hook failure propagates to the caller.
In `@packages/egg/src/lib/egg.ts`:
- Around line 166-169: The loadFinished promise currently only delays callers
but does not prevent load() from registering ready-side effects; modify load()
so that it does not register the ready callbacks that perform startup side
effects (the ready(() => { dumpConfig(); dumpTiming(); dumpManifest(); ... })
registrations) and does not call `#setupTimeoutTimer`() when running in
snapshot/build mode; detect snapshot mode via the existing snapshot flag/option
used by the lifecycle (the same condition that marks ready after configWillLoad)
and skip registering those callbacks and timers so snapshot builds won't execute
dumpConfig/dumpTiming/dumpManifest or `#setupTimeoutTimer`() in the builder
process.
In `@packages/egg/test/snapshot.test.ts`:
- Around line 180-190: The test must assert loadFinished is still pending when
ready() resolves: capture the settled state of app.loadFinished before awaiting
ready() (use a small isPending/isSettled helper or Promise.race trick against a
resolved sentinel to detect whether app.loadFinished has settled), assert it is
pending, then await app.ready(), then assert app.loadFinished remains pending
(i.e. not settled) after ready() resolves, and finally await app.loadFinished to
complete the test; reference Application, app.ready(), and app.loadFinished when
locating where to add the checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4fae92d-5af4-4c53-b369-c59af8c75594
📒 Files selected for processing (9)
packages/core/src/egg.tspackages/core/src/lifecycle.tspackages/core/test/snapshot.test.tspackages/egg/src/index.tspackages/egg/src/lib/agent.tspackages/egg/src/lib/egg.tspackages/egg/src/lib/snapshot.tspackages/egg/src/lib/start.tspackages/egg/test/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 (3)
- packages/egg/src/lib/snapshot.ts
- packages/egg/src/lib/agent.ts
- packages/egg/src/lib/start.ts
killagu
force-pushed
the
feat/snapshot-egg
branch
from
e16fa39 to
3760efd
Compare
killagu
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/egg/src/lib/start.ts (1)
98-138: Well-structured snapshot startup with correct lifecycle handling.The implementation correctly:
- Uses
snapshot: trueto stop lifecycle afterconfigWillLoad- Awaits
loadFinishedinstead ofready()to ensure full loading- Creates both agent and application (unlike
metadataOnlyinstartEgg)- Omits
egg-readybroadcast (noserverDidReadyduring snapshot build)Consider extracting the duplicated framework resolution logic (lines 101-117) shared with
startEgg(lines 44-60) into a helper function to reduce code duplication.♻️ Optional: Extract shared framework resolution
async function resolveFrameworkClasses( baseDir: string, framework?: string, ): Promise<{ AgentClass: typeof Agent; ApplicationClass: typeof Application }> { let AgentClass = Agent; let ApplicationClass = Application; if (framework) { const mod = await importModule(framework, { paths: [baseDir] }); AgentClass = mod.Agent; ApplicationClass = mod.Application; } return { AgentClass, 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 - 138, The framework resolution logic duplicated between startEggForSnapshot and startEgg should be extracted into a helper (e.g., resolveFrameworkClasses) that accepts baseDir and optional framework and returns { AgentClass, ApplicationClass }; replace the inline blocks in startEggForSnapshot (the code that sets AgentClass/ApplicationClass and calls importModule) and the corresponding block in startEgg with calls to that helper to eliminate duplication and keep existing behavior (default to Agent/Application if no framework, importModule with paths: [baseDir] when framework provided).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/egg/src/lib/snapshot.ts`:
- Around line 29-49: Update restoreSnapshot to be async and invoke the
deserialize lifecycle hook so non-serializable resources are restored: change
restoreSnapshot to return Promise<App> (make it async), retrieve the app from
globalThis.__egg_snapshot_app, then call and await
app.triggerSnapshotDidDeserialize() before returning the app; also update or
remove the existing JSDoc line that claims loggers/messenger are automatically
re-created if you prefer to keep documentation accurate. Ensure references to
restoreSnapshot and triggerSnapshotDidDeserialize (and
globalThis.__egg_snapshot_app) are adjusted accordingly.
---
Nitpick comments:
In `@packages/egg/src/lib/start.ts`:
- Around line 98-138: The framework resolution logic duplicated between
startEggForSnapshot and startEgg should be extracted into a helper (e.g.,
resolveFrameworkClasses) that accepts baseDir and optional framework and returns
{ AgentClass, ApplicationClass }; replace the inline blocks in
startEggForSnapshot (the code that sets AgentClass/ApplicationClass and calls
importModule) and the corresponding block in startEgg with calls to that helper
to eliminate duplication and keep existing behavior (default to
Agent/Application if no framework, importModule with paths: [baseDir] when
framework provided).
🪄 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: 82c42ac7-c7c7-49ad-b9e6-c70431d98e81
📒 Files selected for processing (9)
packages/core/src/egg.tspackages/core/src/lifecycle.tspackages/core/test/snapshot.test.tspackages/egg/src/index.tspackages/egg/src/lib/agent.tspackages/egg/src/lib/egg.tspackages/egg/src/lib/snapshot.tspackages/egg/src/lib/start.tspackages/egg/test/snapshot.test.ts
✅ Files skipped from review due to trivial changes (2)
- packages/egg/src/index.ts
- packages/egg/test/snapshot.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/egg.ts
- packages/core/src/lifecycle.ts
- packages/core/test/snapshot.test.ts
killagu
force-pushed
the
feat/snapshot-egg
branch
from
8cd334e to
5d90728
Compare
killagu
force-pushed
the
feat/snapshot-egg
branch
from
d507547 to
e044a5d
Compare
killagu
force-pushed
the
feat/snapshot-egg
branch
from
3a16dce to
49a0781
Compare
Add snapshotWillSerialize/snapshotDidDeserialize lifecycle hooks and
snapshot-safe initialization patterns for V8 startup snapshot support.
## core changes
- Add `snapshotWillSerialize()` / `snapshotDidDeserialize()` to ILifecycleBoot
- triggerSnapshotWillSerialize: reverse order (LIFO, like beforeClose)
- triggerSnapshotDidDeserialize: forward order (FIFO), resumes lifecycle from configDidLoad
- Unified ready() behavior: snapshot mode waits for load completion naturally
- Expose trigger methods on EggCore for build scripts
## egg changes
- Agent keepalive timer moved to configDidLoad via boot hook
- Messenger/logger/unhandledRejection cleanup via protected lifecycle methods
- Zero `if (this.options.snapshot)` guards — all handled by lifecycle structure
- `startEgg({ snapshot: true })` for snapshot mode (unified API)
- Shared resolveFrameworkClasses() helper eliminates duplication
- buildSnapshot() / restoreSnapshot() public APIs
## Tests
- 16 core snapshot tests (hook ordering, async, error handling, lifecycle resume)
- 11 egg snapshot tests (agent keepalive, messenger, logger, unhandledRejection, APIs)
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
killagu
force-pushed
the
feat/snapshot-egg
branch
from
aa0a125 to
6c29b67
Compare
jerryliang64
reviewed
Apr 3, 2026
Contributor
jerryliang64
left a comment
jerryliang64
left a comment
There was a problem hiding this comment.
Minor review comments on three low-severity items.
- Remove redundant `.bind(this)` in snapshotDidDeserialize — handler is
already bound in load(), re-binding creates `bound bound` wrapper
- Change `startKeepAlive()` from public to protected — only called
internally from configDidLoad boot hook
- Delete born-deprecated `startEggForSnapshot` and `SnapshotEggOptions` —
never published, use `startEgg({ snapshot: true })` directly
Co-Authored-By: Claude Opus 4.6 <[email protected]>
- Fix typecheck: cast to `any` for protected `startKeepAlive()` calls in tests - restoreSnapshot() now async, triggers snapshotDidDeserialize hooks to recreate messenger/loggers/listeners and resume lifecycle from configDidLoad - Snapshot hooks rethrow errors instead of swallowing (fail the build/restore) - Restore `readonly messenger` — use internal type cast in snapshotDidDeserialize - Guard dump/timer in snapshot mode (skip dumpConfig, dumpTiming, setupTimeoutTimer) - Fix stale JSDoc on loadFinished, buildSnapshot, restoreSnapshot Co-Authored-By: Claude Opus 4.6 <[email protected]>
triggerSnapshotWillSerialize/triggerSnapshotDidDeserialize now throw if called on a non-snapshot lifecycle, preventing accidental corruption of normal app state. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Comment on lines
+219
to
+223
| // In snapshot mode, skip runtime-only setup (dump, timeout timer). | ||
| // These will run after snapshot restore when the full lifecycle resumes. | ||
| if (!this.options.snapshot) { | ||
| // dump config after ready, ensure all the modifications during start will be recorded | ||
| // make sure dumpConfig is the last ready callback |
There was a problem hiding this comment.
load() skips dumpConfig/dumpTiming/dumpManifest and #setupTimeoutTimer() whenever options.snapshot is true, but options.snapshot remains true after triggerSnapshotDidDeserialize(). As a result, snapshot-restored apps will never schedule config/timing/manifest dumps or the startup timeout timer (despite the comment saying they will run after restore). Consider moving these runtime setups into snapshotDidDeserialize() (or otherwise differentiating “snapshot build” vs “snapshot restored runtime”).
Comment on lines
+527
to
+532
| protected snapshotDidDeserialize(): void { | ||
| (this as { messenger: IMessenger }).messenger = createMessenger(this); | ||
| this.messenger.once('egg-ready', () => { | ||
| this.lifecycle.triggerServerDidReady(); | ||
| }); | ||
| process.on('unhandledRejection', this._unhandledRejectionHandler); |
There was a problem hiding this comment.
messenger is declared readonly, but snapshotDidDeserialize() reassigns it via a type assertion. This bypasses the type system and makes the field effectively mutable in an implicit way. Prefer making messenger non-readonly (if reassignment is intended) or encapsulating the recreation behind an internal method/property so callers and subclasses don’t rely on a “readonly” contract that isn’t true at runtime.
Comment on lines
+65
to
+69
| /** | ||
| * Start egg with single process. | ||
| * | ||
| * When `options.snapshot` is true, loads application metadata for V8 startup | ||
| * snapshot construction. The lifecycle stops after `configWillLoad` (no servers, |
There was a problem hiding this comment.
PR description mentions a startEggForSnapshot() convenience API, but this file only extends startEgg() with a snapshot option and does not add the new function. If startEggForSnapshot() is still intended, it should be implemented/exported; otherwise please update the PR description to avoid API/documentation mismatch.
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
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