feat(koa): defer AsyncLocalStorage creation for V8 startup snapshot support#5851
feat(koa): defer AsyncLocalStorage creation for V8 startup snapshot support#5851
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughApplication defers AsyncLocalStorage initialization during V8 startup snapshot building by setting Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant V8 as V8 StartupSnapshot
participant Callback as Deserialize Callback
participant ALS as AsyncLocalStorage
Note over App,V8: Snapshot build phase
App->>V8: isBuildingSnapshot()
V8-->>App: true
App->>App: set ctxStorage = null
App->>V8: register deserialize callback
V8->>Callback: store callback
Note over V8,App: Later at deserialization
V8->>Callback: invoke callback
Callback->>ALS: create AsyncLocalStorage()
ALS-->>Callback: instance
Callback->>App: set ctxStorage = ALS instance
sequenceDiagram
participant Client as Client
participant App as Application
participant ALS as AsyncLocalStorage
participant Handler as Request Handler
participant Middleware as Middleware
Client->>App: HTTP request
App->>App: check ctxStorage
alt ctxStorage initialized
App->>ALS: ctxStorage.run(ctx, handler)
ALS->>Handler: run with context
else ctxStorage is null
App->>Handler: call handleRequest() directly
end
Handler->>Middleware: invoke middleware
Middleware->>App: access currentContext
App->>ALS: ctxStorage?.getStore()
ALS-->>Middleware: current context (or undefined)
Handler-->>Client: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5851 +/- ##
==========================================
- Coverage 88.37% 85.24% -3.13%
==========================================
Files 9 666 +657
Lines 43 13177 +13134
Branches 8 1525 +1517
==========================================
+ Hits 38 11233 +11195
- Misses 5 1813 +1808
- Partials 0 131 +131 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 by deferring the initialization of AsyncLocalStorage when a snapshot is being built. Changes include updating the Application class to handle a potentially null ctxStorage and adding a new test suite for snapshot scenarios. Feedback focuses on improving the robustness of the implementation by using optional chaining for environment-dependent V8 APIs and utilizing proper mocking utilities in tests instead of direct module mutation.
|
|
||
| it('should defer AsyncLocalStorage creation when building snapshot', () => { | ||
| let deserializeCallback: { cb: (data: unknown) => void; data: unknown } | undefined; | ||
| (v8 as Record<string, unknown>).startupSnapshot = { |
There was a problem hiding this comment.
Mutating properties of the v8 module directly (e.g., v8.startupSnapshot = ...) is likely to fail in standard Node.js environments because these properties are typically read-only or defined as getters on the module object. This can lead to TypeError at runtime. It is recommended to use Vitest's mocking utilities like vi.spyOn or vi.mock to simulate the v8.startupSnapshot behavior safely.
There was a problem hiding this comment.
Thanks for the suggestion. The direct mutation via (v8 as Record<string, unknown>).startupSnapshot = {...} is intentional here — v8.startupSnapshot is writable at runtime in Node.js (it's not defined as a getter). This pattern is simpler and more explicit than vi.mock('node:v8') which would mock the entire module and require more complex setup. The tests pass on all platforms, confirming the property is writable. Won't change this.
| this.ctxStorage = getAsyncLocalStorage(); | ||
| if (v8.startupSnapshot?.isBuildingSnapshot?.()) { | ||
| this.ctxStorage = null; | ||
| v8.startupSnapshot.addDeserializeCallback((app: Application) => { |
There was a problem hiding this comment.
While v8.startupSnapshot is checked on line 92, it is safer to use optional chaining or verify the existence of addDeserializeCallback before calling it. This API is environment-dependent and may not be fully present in all Node.js versions or environments where the code might run.
| v8.startupSnapshot.addDeserializeCallback((app: Application) => { | |
| v8.startupSnapshot.addDeserializeCallback?.((app: Application) => { |
There was a problem hiding this comment.
Won't fix. Line 92 already guards with v8.startupSnapshot?.isBuildingSnapshot?.() — if this returns true, the full snapshot API (including addDeserializeCallback) is guaranteed to be present per the Node.js docs. Adding ?. here would silently swallow errors if the API were somehow incomplete, which would be a worse failure mode: ctxStorage would stay null forever with no error, causing subtle runtime issues. Failing fast is the safer behavior.
Deploying egg-v3 with
|
| Latest commit: |
421b7b1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a329974a.egg-v3.pages.dev |
| Branch Preview URL: | https://feat-snapshot-koa.egg-v3.pages.dev |
Deploying egg with
|
| Latest commit: |
421b7b1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ee7adaca.egg-cci.pages.dev |
| Branch Preview URL: | https://feat-snapshot-koa.egg-cci.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/koa/src/application.ts (1)
52-99:⚠️ Potential issue | 🟠 MajorFix unguarded
ctxStorageaccesses in production code.The
ctxStoragefield is now nullable (line 52) to support V8 snapshot deserialization, but core framework code inpackages/egg/src/lib/egg.ts(line 78,currentContextgetter) andpackages/egg/src/lib/application.ts(line 234,runInBackground) accesses it without null guards. The parentpackages/koa/src/application.tscorrectly uses optional chaining (?.) in its getter—the Egg framework should follow the same pattern. This violates strict TypeScript mode and creates runtime hazards in snapshot-mode execution.Add optional chaining guards or refactor behind a private helper that guarantees initialization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/koa/src/application.ts` around lines 52 - 99, The ctxStorage field on Application is nullable to support V8 snapshot deserialization, but code in currentContext (getter in packages/egg/src/lib/egg.ts) and runInBackground (packages/egg/src/lib/application.ts) accesses it without null checks; update those accesses to guard against null (use optional chaining like this.ctxStorage?.getStore() or check and early-return/throw) or introduce a private helper (e.g., ensureCtxStorage() or getCtxStore()) that returns the store safely and use that helper inside currentContext and runInBackground so runtime null dereferences are prevented.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/koa/src/application.ts`:
- Around line 52-99: The ctxStorage field on Application is nullable to support
V8 snapshot deserialization, but code in currentContext (getter in
packages/egg/src/lib/egg.ts) and runInBackground
(packages/egg/src/lib/application.ts) accesses it without null checks; update
those accesses to guard against null (use optional chaining like
this.ctxStorage?.getStore() or check and early-return/throw) or introduce a
private helper (e.g., ensureCtxStorage() or getCtxStore()) that returns the
store safely and use that helper inside currentContext and runInBackground so
runtime null dereferences are prevented.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 698fc2c3-2aa2-431c-bcc0-9df3b43c6928
📒 Files selected for processing (2)
packages/koa/src/application.tspackages/koa/test/application/snapshot.test.ts
There was a problem hiding this comment.
Pull request overview
This PR updates @eggjs/koa’s Application to support V8 startup snapshot building by avoiding creation of AsyncLocalStorage during snapshot build time, and restoring it upon snapshot deserialization.
Changes:
- Defer
AsyncLocalStorageinitialization whenv8.startupSnapshot.isBuildingSnapshot()is true, and restore it viaaddDeserializeCallback. - Make
ctxStoragenullable and updatecallback()/currentContextto be null-safe. - Add snapshot-focused tests to validate defer/restore behavior and null-safe request handling.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/koa/src/application.ts | Defers ALS creation during snapshot build, restores it on deserialization, and makes request handling/currentContext null-safe. |
| packages/koa/test/application/snapshot.test.ts | Adds tests covering snapshot build deferral, restoration, and behavior when ctxStorage is null. |
…upport When building a V8 startup snapshot via `v8.startupSnapshot.isBuildingSnapshot()`, AsyncLocalStorage cannot be created because it relies on async_hooks which are not available during snapshot serialization. This change defers the creation of ctxStorage to the deserialization phase, making @eggjs/koa compatible with Node.js V8 startup snapshots. - Change `ctxStorage` type to `AsyncLocalStorage<Context> | null` - Null-guard `ctxStorage` in `callback()` and `currentContext` getter - Register `addDeserializeCallback` to restore ALS after snapshot restore Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ests tsgo does not support `asserts` type narrowing from assert.ok, so `app.ctxStorage instanceof AsyncLocalStorage` fails typecheck when ctxStorage is typed as `AsyncLocalStorage | null`. Use non-null assertion operator to satisfy the type checker. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
killagu
force-pushed
the
feat/snapshot-koa
branch
from
49cdbe4 to
421b7b1
Compare
|
Re: @coderabbitai's review about unguarded This is a valid observation but out of scope for this PR, which only touches the The null-safety changes at the Egg layer (if needed) will be addressed in the separate Re: @copilot review — no issues found, thanks for confirming. |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
🧠 Learnings used |
Summary
AsyncLocalStoragecreation during V8 snapshot building viav8.startupSnapshot.isBuildingSnapshot()ctxStoragenullable (AsyncLocalStorage<Context> | null) with null-safe access incallback()andcurrentContextThis is PR1 of 6 in the V8 startup snapshot series. Independent, no dependencies.
Changes
packages/koa/src/application.ts— Defer ALS creation, null-safe ctxStorage accesspackages/koa/test/application/snapshot.test.ts— 5 new tests covering defer, restore, null safetyTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Improvements
Tests