feat(core): add snapshot mode to lifecycle (stop before configDidLoad phase)#5852
feat(core): add snapshot mode to lifecycle (stop before configDidLoad phase)#5852
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an optional Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5852 +/- ##
==========================================
- Coverage 88.37% 85.30% -3.07%
==========================================
Files 9 666 +657
Lines 43 13214 +13171
Branches 8 1533 +1525
==========================================
+ Hits 38 11272 +11234
- Misses 5 1811 +1806
- Partials 0 131 +131 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/egg.ts`:
- Around line 36-41: Update the snapshot option JSDoc to accurately reflect that
snapshot mode loads metadata and runs lifecycle hooks up to and including
didLoad, but stops before later phases (e.g., willReady/ready or starting
servers/timers/connections); specifically mention the snapshot?: boolean field
and reference the didLoad hook so authors know didLoad will execute while later
hooks and starting of servers/timers/connections will not.
🪄 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: d17e3b2d-04f9-4bc4-a675-93f15a46e55d
📒 Files selected for processing (2)
packages/core/src/egg.tspackages/core/src/lifecycle.ts
![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 a snapshot option to the EggCore and Lifecycle classes to support V8 startup snapshot construction. When enabled, the application lifecycle stops after the didLoad phase, effectively skipping the willReady, didReady, and serverDidReady hooks to allow metadata loading without starting servers or connections. I have no feedback to provide.
There was a problem hiding this comment.
Pull request overview
Adds a new snapshot mode to @eggjs/core lifecycle execution, intended to support V8 startup snapshot construction by stopping the lifecycle after didLoad completes.
Changes:
- Add
snapshot?: booleantoEggCoreOptionsand pass it into theLifecycleconstructor. - Add
snapshot?: booleantoLifecycleOptionsand alter lifecycle progression to mark ready afterdidLoad(skippingwillReadyanddidReadyin normal flow).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/core/src/lifecycle.ts | Introduces snapshot option and stops lifecycle progression after didLoad by marking the app ready immediately. |
| packages/core/src/egg.ts | Exposes snapshot option on EggCoreOptions and forwards it to Lifecycle. |
packages/core/src/lifecycle.ts
Outdated
| * When true, the lifecycle stops after didLoad phase completes. | ||
| * willReady, didReady, and serverDidReady hooks are NOT called. | ||
| * Used for V8 startup snapshot construction. |
There was a problem hiding this comment.
The JSDoc says serverDidReady hooks are NOT called in snapshot mode, but Lifecycle.triggerServerDidReady() can still be invoked externally (e.g. packages/egg/src/lib/egg.ts triggers it on egg-ready, and tests call it directly). To make this guarantee true, consider making triggerServerDidReady() a no-op when options.snapshot is true (or soften the comment to clarify it’s only skipped by the lifecycle’s normal flow).
| * When true, the lifecycle stops after didLoad phase completes. | |
| * willReady, didReady, and serverDidReady hooks are NOT called. | |
| * Used for V8 startup snapshot construction. | |
| * When true, the lifecycle's normal flow stops after the didLoad phase | |
| * completes, and willReady, didReady, and serverDidReady hooks are not | |
| * invoked automatically. External callers may still trigger those hooks | |
| * manually if needed. Used for V8 startup snapshot construction. |
There was a problem hiding this comment.
This is by design. The JSDoc documents what the lifecycle flow does automatically — it does not call serverDidReady. External callers invoking triggerServerDidReady() directly would be doing so intentionally and at their own risk.
Making it a no-op would prevent legitimate use cases where snapshot consumers need to selectively trigger later phases. The current wording ("hooks are NOT called") is accurate for the lifecycle's own behavior. Won't change, but appreciate the thoroughness.
packages/core/src/egg.ts
Outdated
| * When true, the application loads metadata only (plugins, configs, extensions, | ||
| * services, controllers) without starting servers, timers, or connections. | ||
| * Used for V8 startup snapshot construction. |
There was a problem hiding this comment.
The snapshot option comment here claims the app loads metadata only and avoids starting servers/timers/connections, but in this PR snapshot mode still runs the normal lifecycle through didLoad (including configWillLoad/configDidLoad, didLoad hooks, and registerBeforeStart tasks). That code can still start timers/connections. Please adjust the doc to match the actual semantics (stop after didLoad / skip ready phases), or enforce the stronger guarantee in code.
| * When true, the application loads metadata only (plugins, configs, extensions, | |
| * services, controllers) without starting servers, timers, or connections. | |
| * Used for V8 startup snapshot construction. | |
| * When true, the application runs a snapshot-oriented bootstrap for V8 | |
| * startup snapshot construction. | |
| * | |
| * In this mode Egg still executes the normal lifecycle up through `didLoad`, | |
| * including `configWillLoad` / `configDidLoad`, `didLoad` hooks, and | |
| * `registerBeforeStart` tasks, but stops before the ready / after-start | |
| * phases (for example, it does not itself proceed to start HTTP servers). | |
| * | |
| * Code that runs in these earlier lifecycle stages (application or plugin | |
| * hooks) may still create timers, background tasks, or external connections, | |
| * so callers MUST NOT rely on this option to guarantee a completely | |
| * side-effect-free or connection-free startup. |
There was a problem hiding this comment.
The suggested 8-line JSDoc is too verbose for a simple boolean option. The key information developers need is: (1) lifecycle stops after didLoad, (2) later phases are skipped, (3) purpose is snapshot construction.
Will update to a concise version that accurately reflects the behavior without over-documenting edge cases that are better covered in framework docs. See reply to CodeRabbit's similar comment above.
packages/core/src/lifecycle.ts
Outdated
| } else if (this.options.snapshot) { | ||
| // In snapshot mode, stop after didLoad — skip willReady/didReady/serverDidReady | ||
| debug('snapshot mode: skipping willReady, marking ready after didLoad'); | ||
| this.ready(true); | ||
| } else { | ||
| this.triggerWillReady(); |
There was a problem hiding this comment.
Snapshot-mode behavior (marking app ready after didLoad and skipping willReady/didReady) isn’t covered by tests in packages/core/test. Since this alters lifecycle semantics, please add a unit test that asserts the executed hook sequence in snapshot mode and verifies willReady/didReady are not invoked.
There was a problem hiding this comment.
Agree that tests are needed. Snapshot mode tests will be included in the integration test PR (feat/snapshot-scripts, PR #5853) which exercises the full snapshot build + restore flow end-to-end. Adding isolated lifecycle unit tests here would duplicate that coverage. The core change is minimal (3 lines of control flow) and well-understood.
Add `snapshot` option to EggCoreOptions and LifecycleOptions. When enabled, lifecycle stops after didLoad phase completes, skipping willReady, didReady, and serverDidReady hooks. Used for V8 startup snapshot construction. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
killagu
force-pushed
the
feat/snapshot-core
branch
from
af1e43d to
4d5c5fd
Compare
Deploying egg-v3 with
|
| Latest commit: |
3235abe
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6fa86685.egg-v3.pages.dev |
| Branch Preview URL: | https://feat-snapshot-core.egg-v3.pages.dev |
Update the snapshot option documentation to accurately describe that lifecycle stops after didLoad phase, rather than the misleading "loads metadata only" wording. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/lifecycle.ts (1)
72-77: ExposesnapshotinStartEggOptionsfor API consistency.
EggCoreOptionsnow supportssnapshot?: boolean, but callers ofstartEgg()cannot pass this option due toStartEggOptionsnot declaring it. Addsnapshot?: booleantoStartEggOptionsso the property flows through to theApplicationconstructor without type safety loss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/lifecycle.ts` around lines 72 - 77, Add the missing snapshot property to StartEggOptions so callers can pass it through to startEgg; specifically, update the StartEggOptions interface/type to include snapshot?: boolean (matching EggCoreOptions), ensure startEgg accepts and forwards this option into the Application constructor/new Application(...) call so the Application receives snapshot, and keep the property name exactly snapshot to preserve type compatibility with EggCoreOptions and Application.
🤖 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/core/src/lifecycle.ts`:
- Around line 72-77: Add the missing snapshot property to StartEggOptions so
callers can pass it through to startEgg; specifically, update the
StartEggOptions interface/type to include snapshot?: boolean (matching
EggCoreOptions), ensure startEgg accepts and forwards this option into the
Application constructor/new Application(...) call so the Application receives
snapshot, and keep the property name exactly snapshot to preserve type
compatibility with EggCoreOptions and Application.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa226234-7815-4523-a904-34c68ef734a6
📒 Files selected for processing (2)
packages/core/src/egg.tspackages/core/src/lifecycle.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/egg.ts
Deploying egg with
|
| Latest commit: |
3235abe
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7d1f71bd.egg-cci.pages.dev |
| Branch Preview URL: | https://feat-snapshot-core.egg-cci.pages.dev |
Verify that snapshot mode stops after didLoad and skips willReady/didReady/serverDidReady. Also verify snapshot option propagation from EggCore to Lifecycle. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/core/test/snapshot.test.ts (2)
96-116: AddserverDidReadypositive-path coverage in normal mode.Snapshot mode explicitly asserts
serverDidReadyis skipped, so it’s helpful to assert it runs when snapshot is not enabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/test/snapshot.test.ts` around lines 96 - 116, The test is missing a positive-path assertion for the serverDidReady hook in normal mode; after calling lifecycle.init(), lifecycle.triggerConfigWillLoad(), and await lifecycle.ready(), add an assertion that callOrder includes 'serverDidReady' (e.g., use assert.ok(callOrder.includes('serverDidReady'), 'serverDidReady should be called in normal mode')) so the test verifies serverDidReady runs when snapshot mode is not enabled; locate this near the existing assertions for 'willReady' and 'didReady' in the snapshot.test.ts lifecycle test.
120-142: Make this assertion snapshot-specific.This test currently passes even in normal mode because only
didLoadis instrumented. Consider asserting that no later hooks run in this case too.Suggested tightening
- it('should mark ready immediately after didLoad completes in snapshot mode', async () => { - let didLoadCompleted = false; + it('should mark ready immediately after didLoad completes in snapshot mode', async () => { + const callOrder: string[] = []; const lifecycle = new Lifecycle({ baseDir: '.', app: new EggCore(), snapshot: true, @@ lifecycle.addBootHook( class Boot { async didLoad(): Promise<void> { - didLoadCompleted = true; + callOrder.push('didLoad'); + } + + async willReady(): Promise<void> { + callOrder.push('willReady'); + } + + async didReady(): Promise<void> { + callOrder.push('didReady'); } }, ); @@ - assert.ok(didLoadCompleted, 'didLoad should have completed'); + assert.deepEqual(callOrder, [ 'didLoad' ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/test/snapshot.test.ts` around lines 120 - 142, The test must verify snapshot-specific behavior by asserting that later lifecycle hooks do not run in snapshot mode: in the test for Lifecycle with snapshot: true, add boolean flags (e.g., willReadyRan = false, didReadyRan = false) and register a Boot hook class implementing willReady() and didReady() that set those flags; after lifecycle.init(); triggerConfigWillLoad(); await lifecycle.ready(); assert that didLoadCompleted is true and also assert willReadyRan and didReadyRan are false to ensure no later hooks ran in snapshot mode; reference the Lifecycle instance and the Boot hook class used in the test to locate where to add these flags and assertions.
🤖 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/core/test/snapshot.test.ts`:
- Around line 96-116: The test is missing a positive-path assertion for the
serverDidReady hook in normal mode; after calling lifecycle.init(),
lifecycle.triggerConfigWillLoad(), and await lifecycle.ready(), add an assertion
that callOrder includes 'serverDidReady' (e.g., use
assert.ok(callOrder.includes('serverDidReady'), 'serverDidReady should be called
in normal mode')) so the test verifies serverDidReady runs when snapshot mode is
not enabled; locate this near the existing assertions for 'willReady' and
'didReady' in the snapshot.test.ts lifecycle test.
- Around line 120-142: The test must verify snapshot-specific behavior by
asserting that later lifecycle hooks do not run in snapshot mode: in the test
for Lifecycle with snapshot: true, add boolean flags (e.g., willReadyRan =
false, didReadyRan = false) and register a Boot hook class implementing
willReady() and didReady() that set those flags; after lifecycle.init();
triggerConfigWillLoad(); await lifecycle.ready(); assert that didLoadCompleted
is true and also assert willReadyRan and didReadyRan are false to ensure no
later hooks ran in snapshot mode; reference the Lifecycle instance and the Boot
hook class used in the test to locate where to add these flags and assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d580e9f8-dee7-48b4-830a-fd5536fcd086
📒 Files selected for processing (1)
packages/core/test/snapshot.test.ts
After configDidLoad, SDKs begin initializing in didLoad hooks — opening connections, starting timers, and creating resources that cannot survive V8 heap serialization. Move the snapshot cutoff earlier: stop right after configDidLoad completes, before triggerDidLoad() is called. This captures all file loading, config merging, and prototype extensions while avoiding non-serializable side effects. The deferred phases (didLoad/willReady/didReady) will run at snapshot restore time instead. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
killagu
changed the title
killagu
changed the title
…Load The snapshot check was inside triggerConfigDidLoad(), running AFTER all configDidLoad hooks had already executed. SDKs execute during configDidLoad hooks, opening connections and starting timers which are not serializable in V8 startup snapshots. Move the check into triggerConfigWillLoad() so that configDidLoad is never called in snapshot mode. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| /** | ||
| * When true, the lifecycle stops after configWillLoad phase completes. | ||
| * configDidLoad, didLoad, willReady, didReady, and serverDidReady hooks | ||
| * are NOT called. Used for V8 startup snapshot construction — SDKs | ||
| * typically execute during configDidLoad, opening connections and starting | ||
| * timers which are not serializable. The handling is analogous to | ||
| * metadataOnly mode: both short-circuit the lifecycle chain early. | ||
| */ |
There was a problem hiding this comment.
The PR description says snapshot mode “stops after didLoad”, but the implementation (and this JSDoc) now short-circuits immediately after configWillLoad (before configDidLoad). Please update the PR description (or rename/clarify the feature wording) so reviewers/users don’t get conflicting semantics.
| * are NOT called. Used for V8 startup snapshot construction — SDKs | ||
| * typically execute during configDidLoad, opening connections and starting | ||
| * timers which are not serializable. The handling is analogous to | ||
| * metadataOnly mode: both short-circuit the lifecycle chain early. |
There was a problem hiding this comment.
In snapshot mode configDidLoad is skipped, which also means Boot.beforeClose() hooks are never registered (registration happens during triggerConfigDidLoad). The doc comment lists skipped startup hooks but doesn’t mention this shutdown implication; consider documenting that beforeClose hooks defined on boot classes won’t run automatically in snapshot mode (unless callers register close hooks manually).
| * metadataOnly mode: both short-circuit the lifecycle chain early. | |
| * metadataOnly mode: both short-circuit the lifecycle chain early. | |
| * | |
| * Note: because configDidLoad is skipped, Boot.beforeClose() hooks defined | |
| * on boot classes (which are normally registered during triggerConfigDidLoad) | |
| * are not registered and therefore will not run automatically in snapshot | |
| * mode. If you need shutdown logic in snapshot mode, register close hooks | |
| * manually on the application instance. |
| * are skipped. Used for V8 startup snapshot construction — SDKs typically | ||
| * execute during `configDidLoad`, opening connections and starting timers | ||
| * which are not serializable. Analogous to `metadataOnly` mode. |
There was a problem hiding this comment.
Snapshot mode skips configDidLoad, which also prevents Boot.beforeClose() hooks from being registered (they’re registered during the configDidLoad loop). Since this option is public API on EggCoreOptions, consider mentioning this in the JSDoc so callers don’t assume app.close() will trigger boot-class beforeClose hooks in snapshot mode.
| * are skipped. Used for V8 startup snapshot construction — SDKs typically | |
| * execute during `configDidLoad`, opening connections and starting timers | |
| * which are not serializable. Analogous to `metadataOnly` mode. | |
| * are skipped. Because `configDidLoad` is not run, Boot hooks registered | |
| * there (including `beforeClose`) are never set up, and `app.close()` will | |
| * therefore NOT invoke Boot `beforeClose` hooks in snapshot mode. | |
| * Used for V8 startup snapshot construction — SDKs typically execute during | |
| * `configDidLoad`, opening connections and starting timers which are not | |
| * serializable. Analogous to `metadataOnly` mode. |
Summary
snapshot?: booleanoption toEggCoreOptionsandLifecycleOptionsdidLoadphase — skipswillReady/didReady/serverDidReadyThis is PR2 of 6 in the V8 startup snapshot series. Independent, no dependencies.
Changes
packages/core/src/egg.ts— Addsnapshotoption, pass to lifecyclepackages/core/src/lifecycle.ts— Snapshot-aware ready logic (stop after didLoad)Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit