fix(egg): reopen logger streams on snapshot restore - #6001
Merged
Conversation
Contributor
|
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 as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe snapshot lifecycle now keeps ChangesSnapshot logger restore flow
Sequence Diagram(s)sequenceDiagram
participant EggApplicationCore
participant messenger
participant EggLoggers
participant logger transport
EggApplicationCore->>messenger: close()
EggApplicationCore->>EggLoggers: close each logger
EggApplicationCore->>EggApplicationCore: keep `#loggers`
EggApplicationCore->>messenger: recreate and reattach listeners
EggApplicationCore->>EggLoggers: `#reopenLoggers`()
EggLoggers->>logger transport: reload()
EggLoggers->>logger transport: restart flush timer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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 modifies the serialization and deserialization lifecycle of EggApplicationCore to preserve the EggLoggers instance instead of discarding it. This ensures that plugins capturing logger references before serialization do not end up with references pointing to closed streams. During deserialization, the logger streams are reopened in place, and flush intervals for buffered transports are explicitly restarted. Tests have been updated and added to verify this behavior. Review feedback suggests adding defensive checks in #reopenLoggers to handle custom or mock loggers/transports that might not implement the full EggLogger or Transport interfaces, preventing potential runtime errors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+577
to
+589
| for (const logger of this.#loggers.values()) { | ||
| for (const transport of logger.values()) { | ||
| // No-op for ConsoleTransport; reopens the stream for file transports. | ||
| transport.reload(); | ||
| const bufferTransport = transport as unknown as { | ||
| _timer?: NodeJS.Timeout | null; | ||
| _createInterval?: () => NodeJS.Timeout; | ||
| }; | ||
| if (typeof bufferTransport._createInterval === 'function' && !bufferTransport._timer) { | ||
| bufferTransport._timer = bufferTransport._createInterval(); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
To prevent potential runtime errors (such as TypeError: logger.values is not a function or TypeError: transport.reload is not a function), we should add defensive checks. This is especially important if custom loggers or mock loggers/transports are used in tests or plugins that do not implement the full EggLogger or Transport interface.
for (const logger of this.#loggers.values()) {
if (typeof logger?.values !== 'function') continue;
for (const transport of logger.values()) {
if (!transport) continue;
if (typeof transport.reload === 'function') {
transport.reload();
}
const bufferTransport = transport as unknown as {
_timer?: NodeJS.Timeout | null;
_createInterval?: () => NodeJS.Timeout;
};
if (typeof bufferTransport._createInterval === 'function' && !bufferTransport._timer) {
bufferTransport._timer = bufferTransport._createInterval();
}
}
}
Contributor
Author
There was a problem hiding this comment.
Thanks for the suggestion. I considered these guards but they protect against states that can't occur on this path:
this.#loggersis always anEggLoggersinstance (aMap<string, Logger>), so.values()always yieldsLoggerinstances —logger.valuesis always a function andtransportis never nullish.- Every egg-logger transport (
FileTransport,FileBufferTransport,ConsoleTransport, …) extendsTransport, which definesreload()(a no-op on the base class). Sotransport.reloadis always callable.
The one access that is genuinely optional — the FileBufferTransport flush-timer restart — is already guarded by typeof bufferTransport._createInterval === 'function', because only the buffered transport has it.
Adding the other guards would be dead branches that never execute, so I've left the loop as-is to avoid special-casing shared infrastructure. Happy to revisit if there's a concrete case where a non-Logger/non-Transport object can end up in #loggers.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #6001 +/- ##
==========================================
- Coverage 84.88% 84.88% -0.01%
==========================================
Files 674 674
Lines 20261 20269 +8
Branches 4037 4039 +2
==========================================
+ Hits 17199 17205 +6
- Misses 2631 2633 +2
Partials 431 431 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (1)
packages/egg/src/lib/egg.ts (1)
581-587: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the buffer restart into
FileBufferTransport.reload().EggApplicationCoreshould not reach into_timer/_createInterval; put the interval restart on the transport itself so the restore path can just calltransport.reload().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/egg/src/lib/egg.ts` around lines 581 - 587, Move the buffer interval restart logic out of EggApplicationCore and into FileBufferTransport.reload(), since the current code is reaching into private _timer/_createInterval internals. Update the reload path so FileBufferTransport owns restarting its own interval, and have the restore flow simply call transport.reload() instead of casting and mutating private fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/egg/test/snapshot.test.ts`:
- Line 194: The restore log marker in the snapshot test is too stable because it
only uses process.pid, so it can match stale logs. Update the marker generation
in the snapshot restore test to include a per-test execution unique value
alongside process.pid, and make sure the same marker is used for both writing
and asserting in the restore flow. Locate this in the snapshot.test.ts test
around the marker setup and keep the change scoped to the restore assertion
path.
---
Nitpick comments:
In `@packages/egg/src/lib/egg.ts`:
- Around line 581-587: Move the buffer interval restart logic out of
EggApplicationCore and into FileBufferTransport.reload(), since the current code
is reaching into private _timer/_createInterval internals. Update the reload
path so FileBufferTransport owns restarting its own interval, and have the
restore flow simply call transport.reload() instead of casting and mutating
private fields.
🪄 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: dc2f0040-96d3-4bcb-a500-819fdd5e1aa0
📒 Files selected for processing (2)
packages/egg/src/lib/egg.tspackages/egg/test/snapshot.test.ts
snapshotWillSerialize closed every logger and discarded the EggLoggers instance, relying on the lazy getter to lazily rebuild a fresh one on restore. But plugins such as @eggjs/schedule capture a logger reference in their boot-hook constructor during the load phase (before serialize), so the fresh EggLoggers left those captured references pointing at a closed FileTransport — writing through them after restore raised "... log stream had been closed". Preserve logger identity instead: keep the EggLoggers instance across serialize, and in snapshotDidDeserialize reopen the same logger objects in place via reopenLoggers(). It reloads each FileTransport stream and restarts the FileBufferTransport flush interval (which close() clears and reload() does not restore), keeping the willSerialize/didDeserialize resource pairing complete. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
killagu
force-pushed
the
fix/snapshot-reopen-loggers
branch
from
June 27, 2026 00:39
f42c337 to
478b8d5
Compare
killagu
added a commit
that referenced
this pull request
Jun 28, 2026
…nd cnpmcore e2e (#6003) ## Motivation Restoring a V8 startup snapshot requires **Node.js >= 24**: Node.js 22 aborts during deserialization of a non-trivial Egg heap with the native fatal `Check failed: current == end_slot_index` (a V8 bug). Building a snapshot still works on Node.js >= 22. Today nothing enforces or documents this, and there is no regression coverage. This PR adds a runtime gate, documentation, and an e2e regression — without changing the snapshot mechanism itself. Builds on the snapshot work already on `next` (#5998 entry/prelude + `egg-bin snapshot` command, #5999 lazy-external network stack, #6001 logger reopen, #6002 module-loader hooks). ## Scope **Runtime gate (restore ≥ 24; build stays ≥ 22)** - `@eggjs/scripts`: `egg-scripts start --snapshot-blob` refuses to launch on Node.js < 24 with a clear error *before* spawning, checking the major version of the resolved `--node` target binary (not just the egg-scripts runtime). Also adds `allowNo: true` to the `sourcemap` flag so `--no-sourcemap` is accepted. - `@eggjs/egg-bundler`: a defense-in-depth guard in the generated deserialize-main for direct `node --snapshot-blob` launches that manage to deserialize on an unsupported runtime. - `@eggjs/bin`: `snapshot build` prints a note that restoring needs Node.js >= 24. **Docs** - Enrich `site/docs/advanced/snapshot.md` (EN + ZH): Node version requirements, the CLI workflow (`egg-bin snapshot build` → `egg-scripts start --snapshot-blob`), how it works (load module graph → run to `configWillLoad` → freeze; restore = `didReady` + listen), performance (~233ms vs ~942ms, ~4× on cnpmcore), and known limitations. - Wire the page into the VitePress sidebar (EN + ZH) — it existed but was unreachable. **CI** - Add a blocking `cnpmcore-snapshot` ecosystem-ci e2e (Node 24): snapshot build → restore via `egg-scripts start --snapshot-blob` → `curl /-/ping` == 200 → stop. Wires `repo.json`, `patch-project.ts`, `.gitignore`. - Extract the shared health-check poll into `ecosystem-ci/wait-health.sh`, used by both the `cnpmcore` and `cnpmcore-snapshot` jobs. ## Test evidence - `pnpm --filter=@eggjs/scripts --filter=@eggjs/egg-bundler --filter=@eggjs/bin run typecheck` — clean. - `@eggjs/scripts` `snapshot-start.test.ts` (4 tests, incl. a Node<24 gate test and a `--no-sourcemap` parse regression test) + `start-unit.test.ts` — pass. - `@eggjs/bin` `snapshot.test.ts` — pass. - `@eggjs/egg-bundler` `EntryGenerator` canonical snapshot regenerated for the new guard; the rest of the suite matches the pre-change baseline (a few pre-existing macOS/Node-22 path-resolution failures are unrelated). - A multi-agent diff review was run and all confirmed findings addressed (most notably: `--no-sourcemap` is now parseable via `allowNo: true` — without it the e2e job would have failed 100%). ## Notes - The `cnpmcore-snapshot` job is correct-by-construction but **could not be validated on the author's machine** (Node 22, no MySQL+cnpmcore build); it relies on the supported path (lazy-external from #5999, no manual stubs) and is validated by this PR's CI run. - The job is intentionally a **separate matrix project** (not folded into the existing cnpmcore job) for failure isolation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added “V8 Startup Snapshot” documentation navigation. * Added support for snapshot project configuration (shared project-root patching). * **Bug Fixes** * Enforced Node.js version gating for V8 snapshot restore (requires Node.js ≥ 24) and improved snapshot restore safety. * Improved snapshot lazy-external behavior, including correct external named-export handling. * Updated the start command to allow `--no-sourcemap`. * **Documentation** * Expanded snapshot docs with requirements, workflow details, performance, and limitations. * **Tests** * Updated snapshot start/version-gating and lazy-external readiness assertions. * **Chores** * Improved E2E readiness checks with consistent polling, timeouts, and error log output. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
killagu
added a commit
that referenced
this pull request
Jun 28, 2026
## Motivation Building a V8 startup snapshot serializes the whole heap, so any dependency that opens a socket, starts a timer, or initializes a native binding at module-evaluation time can make the blob fail to build — or build and then crash on restore. There was no guide for finding which module is responsible or how to fix it. ## What New dedicated page `advanced/snapshot-troubleshooting.md` (EN + zh-CN): - **The serializability rule** — what cannot survive the round-trip (native bindings / libuv handles / lazy web-global getters) and when a dependency trips it. - **Failure surfaces** — build-time vs restore-time, with the exact error strings each emits (`killed by signal SIGSEGV`, `no blob was written`, `Check failed: current == end_slot_index`, `Aop Advice not found`, `Cannot find module`, …). - **Find the offending module** — `NODE_DEBUG` namespaces, a clean `NODE_OPTIONS`, `--dry-run`, `--skip-bundle` bisecting, `--force-external` confirmation. - **Fixes** — `--force-external`, `egg.snapshot.lazyModules`, the snapshot lifecycle hooks, deferring work out of module scope, avoiding the web globals. - **Failure modes in detail** (tegg `@Advice` filePath, the lazy-external member proxy, runtime-asset `ENOENT`) and a configuration reference table. Also documents the previously-undocumented `egg.snapshot.lazyModules` config in `advanced/snapshot.md`, cross-links the new page from it, and wires the page into the English and Chinese advanced sidebars. Docs-only; no runtime code change. Builds on the snapshot feature already on `next` (#5998 / #5999 / #6001 / #6003). ## Test evidence - `vitepress build site` passes clean — VitePress's dead-link/anchor check is green, so both new pages render and every cross-file and in-page anchor resolves (the two detail headings use explicit ASCII `{#…}` ids because the VitePress slugifier retains CJK + fullwidth `「」`). - Every technical claim was adversarially verified against the `egg-bundler` / `egg-bin` / `egg-scripts` source — exact error strings, flag names, debug namespaces, and the default lazy-module set. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added a new Snapshot Troubleshooting guide in English and Chinese, covering common build and restore failure symptoms, how to diagnose them, and recommended fixes. * Expanded the Snapshot guide with guidance on configuring additional lazy-loaded modules, plus clearer troubleshooting links. * Updated the sidebar navigation to include the new troubleshooting page in both languages. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Motivation
After a V8 startup snapshot is restored, writing logs raised
... log stream had been closed(e.g.egg-schedule.logwhen the schedule plugin'sdidReadystarts).Root cause:
EggApplicationCore.snapshotWillSerializeclosed every logger and discarded theEggLoggersinstance (#loggers = undefined), relying on the lazyloggersgetter to rebuild a freshEggLoggerson restore. But plugins such as@eggjs/schedulecapture an individual logger reference in their boot-hook constructor during the load phase (before serialize). The freshEggLoggersleft those captured references pointing at the original closedFileTransport, so writing through them after restore threw.Scope
packages/eggsnapshot lifecycle resource rebuild only (no bundler changes). Preserve logger identity across the snapshot instead of rebuilding:snapshotWillSerialize: stilllogger.close()each logger (releases the fd and clears theFileBufferTransportflush interval) but keeps theEggLoggersinstance.snapshotDidDeserialize: new#reopenLoggers()reopens the same logger objects in place —transport.reload()reopens eachFileTransportstream, and theFileBufferTransportflush interval is restarted (whichclose()clears butreload()does not restore). This keeps thewillSerialize/didDeserializeresource pairing complete and also fixes the paralleloneloggerglobal-registry staleness.Messenger (recreated) and the
unhandledRejectionlistener (re-attached) were already paired; the agent keepalive timer is recreated by the lifecycle resume — no changes needed there.Test evidence
packages/egg/test/snapshot.test.ts:-web.logfile (poll-read to avoid racing the async stream flush).pnpm --filter=egg run test test/snapshot.test.ts→ 11/11 pass.tsgo --noEmitclean. (Pre-existing unrelated failures intest/lib/core/logger.test.tsare present onnextwithout this change.)🤖 Generated with Claude Code
Summary by CodeRabbit