feat(core): add ManifestStore for startup manifest caching#5844
Open
feat(core): add ManifestStore for startup manifest caching#5844
Conversation
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 a manifest-based startup cache persisted to Changes
Sequence DiagramsequenceDiagram
participant EggCore as EggCore
participant EggLoader as EggLoader
participant Lifecycle as Lifecycle
participant ManifestStore as ManifestStore
participant FileSystem as FileSystem
Note over EggCore,EggLoader: Construct EggLoader (attempt to load manifest)
EggCore->>EggLoader: new EggLoader(metadataOnly: true|false)
EggLoader->>ManifestStore: ManifestStore.load(baseDir, env, scope)
ManifestStore->>FileSystem: read .egg/manifest.json
ManifestStore-->>EggLoader: manifest|null
alt metadataOnly = false
EggLoader->>Lifecycle: triggerConfigWillLoad()
Lifecycle->>Lifecycle: run normal boot hooks (configWillLoad/.../didReady)
else metadataOnly = true
EggLoader->>Lifecycle: triggerLoadMetadata()
Lifecycle->>Lifecycle: run loadMetadata() hooks
Lifecycle->>EggLoader: ready(true)
EggLoader->>ManifestStore: generateManifest(options)
ManifestStore->>FileSystem: compute fingerprints, collect resolves/files
ManifestStore-->>EggLoader: StartupManifest
EggLoader->>ManifestStore: write(manifest)
ManifestStore->>FileSystem: write .egg/manifest.json
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
Deploying egg with
|
| Latest commit: |
9159a88
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://883cab4a.egg-cci.pages.dev |
| Branch Preview URL: | https://feat-core-manifest-store.egg-cci.pages.dev |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## next #5844 +/- ##
==========================================
+ Coverage 85.35% 85.37% +0.01%
==========================================
Files 665 666 +1
Lines 13004 13125 +121
Branches 1495 1519 +24
==========================================
+ Hits 11100 11205 +105
- Misses 1776 1789 +13
- Partials 128 131 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Deploying egg-v3 with
|
| Latest commit: |
9159a88
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://40e41adc.egg-v3.pages.dev |
| Branch Preview URL: | https://feat-core-manifest-store.egg-v3.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 a startup manifest system designed to optimize application boot times by caching file I/O and module resolution results. It adds a metadataOnly mode to the core and lifecycle components, allowing the application to skip standard lifecycle hooks in favor of a new loadMetadata hook for manifest generation. A new ManifestStore utility manages the lifecycle of the manifest file, including generation, persistence, and validation using fingerprints of lockfiles and configuration directories. Review feedback suggests refactoring duplicated conditional logic in the loader and tightening error handling in the manifest utility to ensure that only expected file-not-found errors are suppressed during file system operations.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a startup manifest caching mechanism in @eggjs/core to reduce redundant filesystem work during boot by optionally consuming .egg/manifest.json, and adds a metadataOnly boot mode to support future manifest generation tooling.
Changes:
- Add
ManifestStorefor loading/validating/generating/writing/cleaning.egg/manifest.json. - Integrate manifest reads into
EggLoader.resolveModule()andFileLoader.parse()to reuse cached results and skipglobbyscans. - Add
metadataOnly+loadMetadata()lifecycle hook and comprehensive unit tests forManifestStore.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/loader/manifest.ts | New ManifestStore implementation with fingerprint-based validation and helpers. |
| packages/core/src/loader/egg_loader.ts | Load manifest at startup; prefer manifest cache in resolveModule; collect data for manifest generation. |
| packages/core/src/loader/file_loader.ts | Use manifest-provided file lists to skip globby; collect file discovery when scanning. |
| packages/core/src/lifecycle.ts | Add loadMetadata() hook and triggerLoadMetadata() path for metadata-only startup. |
| packages/core/src/egg.ts | Thread metadataOnly option into loader construction. |
| packages/core/src/index.ts | Export ManifestStore from package entrypoint. |
| packages/core/test/loader/manifest*.test.ts | New unit tests covering ManifestStore APIs and behavior. |
| packages/core/test/loader/manifest_helper.ts | Shared test helpers for temp project setup and manifest generation. |
| packages/core/test/snapshots/index.test.ts.snap | Snapshot update to include ManifestStore export. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/test/loader/manifest_fingerprint.test.ts (1)
28-39: Minor: Consider documenting the 50ms delay purpose.The
setTimeoutdelay on line 32 ensures filesystem mtime changes are observable. While the delay is reasonable, a brief comment would improve clarity for future maintainers.const m1 = ManifestStore.generate({ baseDir, serverEnv: 'prod', serverScope: '', typescriptEnabled: true }); + // Allow filesystem mtime to advance so fingerprint detects the change await new Promise((resolve) => setTimeout(resolve, 50));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/test/loader/manifest_fingerprint.test.ts` around lines 28 - 39, Add a short inline comment explaining the 50ms setTimeout delay in the test so future maintainers understand it's to ensure filesystem mtime changes are observable before writing the new config file; update the test around the setTimeout call in manifest_fingerprint.test.ts (where ManifestStore.generate is invoked to produce m1 and m2) to include a brief note referencing that the pause ensures the OS updates file modification times between m1 and the subsequent fs.writeFileSync that creates config/b.ts.
🤖 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/loader/manifest_fingerprint.test.ts`:
- Around line 28-39: Add a short inline comment explaining the 50ms setTimeout
delay in the test so future maintainers understand it's to ensure filesystem
mtime changes are observable before writing the new config file; update the test
around the setTimeout call in manifest_fingerprint.test.ts (where
ManifestStore.generate is invoked to produce m1 and m2) to include a brief note
referencing that the pause ensures the OS updates file modification times
between m1 and the subsequent fs.writeFileSync that creates config/b.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 324b37e8-ec2b-453e-989a-f3df0ced4a38
⛔ Files ignored due to path filters (1)
packages/core/test/__snapshots__/index.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
packages/core/src/egg.tspackages/core/src/index.tspackages/core/src/lifecycle.tspackages/core/src/loader/egg_loader.tspackages/core/src/loader/file_loader.tspackages/core/src/loader/manifest.tspackages/core/test/loader/manifest.test.tspackages/core/test/loader/manifest_fingerprint.test.tspackages/core/test/loader/manifest_helper.tspackages/core/test/loader/manifest_query.test.tspackages/core/test/loader/manifest_roundtrip.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
packages/core/src/loader/manifest.ts (3)
196-201: MD5 is acceptable for fingerprinting but consider documenting the choice.MD5 is fine for non-cryptographic checksumming here. The fingerprint only needs to detect changes, not resist collision attacks. However, a brief comment explaining this choice would prevent future questions about "why MD5?"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/loader/manifest.ts` around lines 196 - 201, Add a brief comment above ManifestStore.#directoryFingerprint explaining that createHash('md5') is intentionally used for non-cryptographic fingerprinting (to detect file/directory changes only, not for security/anti-collision guarantees), so maintainers understand the tradeoff and do not replace it with a stronger crypto hash unnecessarily.
156-162: Consider atomic write to prevent partial manifest on crash.Writing directly to
manifest.jsoncould leave a partial/corrupted file if the process crashes mid-write. Consider writing to a temp file first, then renaming atomically.💡 Suggested improvement for atomic writes
static async write(baseDir: string, manifest: StartupManifest): Promise<void> { const dir = path.join(baseDir, '.egg'); await fsp.mkdir(dir, { recursive: true }); const manifestPath = path.join(dir, 'manifest.json'); - await fsp.writeFile(manifestPath, JSON.stringify(manifest, null, 2)); + const tempPath = path.join(dir, `manifest.${process.pid}.tmp`); + await fsp.writeFile(tempPath, JSON.stringify(manifest, null, 2)); + await fsp.rename(tempPath, manifestPath); debug('manifest written to %s', manifestPath); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/loader/manifest.ts` around lines 156 - 162, The write method in StartupManifest (static async write) uses fsp.writeFile directly to manifest.json which can produce a partial file on crash; change it to write to a temporary file in the same .egg directory (e.g., manifest.json.tmp with a unique suffix), flush and close the file (open + write + fsync) and then atomically rename (fsp.rename) the temp file to manifest.json to replace it; ensure the temp file is created in the same directory so rename is atomic and handle errors by cleaning up the temp file on failure.
221-222: Symlinks are entirely skipped during fingerprinting.Skipping symlinks means changes to symlinked config files won't invalidate the manifest. This could lead to stale cache if config files are symlinked (e.g., in some deployment setups).
Consider following symlinks to files (not directories) for fingerprinting, or document this limitation:
💡 Optional: Follow symlinks to files
for (const entry of entries) { - if (entry.isSymbolicLink()) continue; const fullPath = path.join(dirpath, entry.name); + // For symlinks, resolve and fingerprint the target if it's a file + if (entry.isSymbolicLink()) { + try { + const targetStat = fs.statSync(fullPath); + if (targetStat.isFile()) { + const fp = ManifestStore.#statFingerprint(fullPath); + hash.update(`symlink:${entry.name}:${fp ?? 'missing'}\n`); + } + } catch { + // Broken symlink, skip + } + continue; + } if (entry.isDirectory()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/loader/manifest.ts` around lines 221 - 222, Currently the loop in manifest.ts skips all symlinks via the check entry.isSymbolicLink(), which misses changes in symlinked files; change the logic so that when entry.isSymbolicLink() is true you resolve the symlink (use fs.promises.realpath or call fs.promises.stat on path) and only skip it if the resolved target is a directory; if the resolved target is a file, treat it like a normal file for fingerprinting (i.e., include it in the same processing path as non-symlink files). Update the loop around for (const entry of entries) and the entry.isSymbolicLink() branch to stat/realpath the target and branch on target being a file vs directory.packages/core/src/loader/egg_loader.ts (1)
1251-1267: Consider extracting duplicated metadataOnly branching logic.Both
loadCustomApp()andloadCustomAgent()have identical conditional logic formetadataOnly. While acceptable, this could be extracted to reduce duplication.💡 Optional: Extract common lifecycle trigger logic
+ async `#triggerLifecycleHook`(): Promise<void> { + if (this.options.metadataOnly) { + await this.lifecycle.triggerLoadMetadata(); + } else { + this.lifecycle.triggerConfigWillLoad(); + } + } + async loadCustomApp(): Promise<void> { await this.#loadBootHook('app'); - if (this.options.metadataOnly) { - await this.lifecycle.triggerLoadMetadata(); - } else { - this.lifecycle.triggerConfigWillLoad(); - } + await this.#triggerLifecycleHook(); } async loadCustomAgent(): Promise<void> { await this.#loadBootHook('agent'); - if (this.options.metadataOnly) { - await this.lifecycle.triggerLoadMetadata(); - } else { - this.lifecycle.triggerConfigWillLoad(); - } + await this.#triggerLifecycleHook(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/loader/egg_loader.ts` around lines 1251 - 1267, Both loadCustomApp and loadCustomAgent duplicate the same metadataOnly branching (calling lifecycle.triggerLoadMetadata() when this.options.metadataOnly is true, otherwise lifecycle.triggerConfigWillLoad()); extract that logic into a small helper method (e.g., handleMetadataLifecycle or triggerLifecycleBasedOnMetadata) and call it from both loadCustomApp and loadCustomAgent to remove duplication while keeping behavior identical; reference the symbols this.options.metadataOnly, lifecycle.triggerLoadMetadata, lifecycle.triggerConfigWillLoad, loadCustomApp, and loadCustomAgent when making the change.packages/core/test/loader/manifest_query.test.ts (1)
9-18: Same unusedtmpDirissue as in manifest_roundtrip.test.ts.The
tmpDirvariable and its setup/teardown are unused since all tests usesetupBaseDir()with their own cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/test/loader/manifest_query.test.ts` around lines 9 - 18, Remove the unused temporary directory setup by deleting the top-level tmpDir variable and the beforeEach/afterEach hooks that call createTmpDir() and fs.rmSync(), since tests use setupBaseDir() for their own temp dirs; specifically, remove the tmpDir declaration and the beforeEach/afterEach blocks in manifest_query.test.ts so only the per-test setupBaseDir-based cleanup remains.packages/core/test/loader/manifest_roundtrip.test.ts (2)
10-18: UnusedtmpDirvariable creates cleanup mismatch.The
tmpDirvariable is created inbeforeEachviacreateTmpDir()and cleaned inafterEach, but all test cases usesetupBaseDir()which creates a separate temporary directory that's cleaned in their ownfinallyblocks. This makes the module-leveltmpDirsetup/teardown effectively dead code.Consider removing the unused
tmpDirhandling or using it consistently across tests:♻️ Proposed fix: Remove unused tmpDir
-let tmpDir: string; - describe('ManifestStore roundtrip: generate → write → load', () => { - beforeEach(() => { - tmpDir = createTmpDir(); - }); - - afterEach(() => { - fs.rmSync(tmpDir, { recursive: true, force: true }); - }); - it('should round-trip manifest data correctly', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/test/loader/manifest_roundtrip.test.ts` around lines 10 - 18, Remove the dead module-level tmpDir setup/teardown: delete the tmpDir declaration and the beforeEach/afterEach blocks referencing tmpDir since tests use setupBaseDir() and manage their own temp dirs; update any references to tmpDir (none expected) to use setupBaseDir() or the per-test directory, and ensure cleanup remains handled by existing finally blocks in the test cases.
70-72: Fixed 50ms delay may cause flaky tests.The 50ms delay before modifying the config file is intended to ensure mtime changes are detectable, but this value may be insufficient on some filesystems (e.g., HFS+ has 1-second granularity) or excessive on others.
Consider using a more robust approach or documenting the rationale:
💡 Suggestion: Increase delay for broader filesystem compatibility
- await new Promise((resolve) => setTimeout(resolve, 50)); + // Ensure mtime changes are detectable across filesystems (HFS+ has 1s granularity) + await new Promise((resolve) => setTimeout(resolve, 1100));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/test/loader/manifest_roundtrip.test.ts` around lines 70 - 72, The fixed 50ms sleep before modifying the config file is flaky on filesystems with coarse mtime granularity; instead of await new Promise(... 50) replace that timing hack by explicitly forcing a detectable mtime change for the file you write (config/config.prod.ts) — e.g. after writing the file call fs.utimesSync or an equivalent to set the file's mtime to a later timestamp (or loop with fs.stat until mtime differs) so ManifestStore.load(baseDir, 'prod', '') reliably observes the change across filesystems.
🤖 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/loader/manifest.ts`:
- Around line 76-114: The manifest validation currently omits checking whether
TypeScript mode changed; update ManifestStore.#validate (which receives
StartupManifest and uses inv = data.invalidation) to compare
inv.typescriptEnabled against the current runtime value from
isSupportTypeScript(), import isSupportTypeScript from `@eggjs/utils`, and if they
differ call debug('manifest typescriptEnabled mismatch: expected %s, got %s',
isSupportTypeScript(), inv.typescriptEnabled) and return false (mirroring the
serverEnv/serverScope checks) so stale TS/J S resolution caches are rejected.
---
Nitpick comments:
In `@packages/core/src/loader/egg_loader.ts`:
- Around line 1251-1267: Both loadCustomApp and loadCustomAgent duplicate the
same metadataOnly branching (calling lifecycle.triggerLoadMetadata() when
this.options.metadataOnly is true, otherwise lifecycle.triggerConfigWillLoad());
extract that logic into a small helper method (e.g., handleMetadataLifecycle or
triggerLifecycleBasedOnMetadata) and call it from both loadCustomApp and
loadCustomAgent to remove duplication while keeping behavior identical;
reference the symbols this.options.metadataOnly, lifecycle.triggerLoadMetadata,
lifecycle.triggerConfigWillLoad, loadCustomApp, and loadCustomAgent when making
the change.
In `@packages/core/src/loader/manifest.ts`:
- Around line 196-201: Add a brief comment above
ManifestStore.#directoryFingerprint explaining that createHash('md5') is
intentionally used for non-cryptographic fingerprinting (to detect
file/directory changes only, not for security/anti-collision guarantees), so
maintainers understand the tradeoff and do not replace it with a stronger crypto
hash unnecessarily.
- Around line 156-162: The write method in StartupManifest (static async write)
uses fsp.writeFile directly to manifest.json which can produce a partial file on
crash; change it to write to a temporary file in the same .egg directory (e.g.,
manifest.json.tmp with a unique suffix), flush and close the file (open + write
+ fsync) and then atomically rename (fsp.rename) the temp file to manifest.json
to replace it; ensure the temp file is created in the same directory so rename
is atomic and handle errors by cleaning up the temp file on failure.
- Around line 221-222: Currently the loop in manifest.ts skips all symlinks via
the check entry.isSymbolicLink(), which misses changes in symlinked files;
change the logic so that when entry.isSymbolicLink() is true you resolve the
symlink (use fs.promises.realpath or call fs.promises.stat on path) and only
skip it if the resolved target is a directory; if the resolved target is a file,
treat it like a normal file for fingerprinting (i.e., include it in the same
processing path as non-symlink files). Update the loop around for (const entry
of entries) and the entry.isSymbolicLink() branch to stat/realpath the target
and branch on target being a file vs directory.
In `@packages/core/test/loader/manifest_query.test.ts`:
- Around line 9-18: Remove the unused temporary directory setup by deleting the
top-level tmpDir variable and the beforeEach/afterEach hooks that call
createTmpDir() and fs.rmSync(), since tests use setupBaseDir() for their own
temp dirs; specifically, remove the tmpDir declaration and the
beforeEach/afterEach blocks in manifest_query.test.ts so only the per-test
setupBaseDir-based cleanup remains.
In `@packages/core/test/loader/manifest_roundtrip.test.ts`:
- Around line 10-18: Remove the dead module-level tmpDir setup/teardown: delete
the tmpDir declaration and the beforeEach/afterEach blocks referencing tmpDir
since tests use setupBaseDir() and manage their own temp dirs; update any
references to tmpDir (none expected) to use setupBaseDir() or the per-test
directory, and ensure cleanup remains handled by existing finally blocks in the
test cases.
- Around line 70-72: The fixed 50ms sleep before modifying the config file is
flaky on filesystems with coarse mtime granularity; instead of await new
Promise(... 50) replace that timing hack by explicitly forcing a detectable
mtime change for the file you write (config/config.prod.ts) — e.g. after writing
the file call fs.utimesSync or an equivalent to set the file's mtime to a later
timestamp (or loop with fs.stat until mtime differs) so
ManifestStore.load(baseDir, 'prod', '') reliably observes the change across
filesystems.
🪄 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: db620636-c20f-4c26-839a-43f20eecd69c
📒 Files selected for processing (6)
packages/core/src/loader/egg_loader.tspackages/core/src/loader/manifest.tspackages/core/test/fixtures/manifest/expected-manifest.jsonpackages/core/test/loader/manifest.test.tspackages/core/test/loader/manifest_query.test.tspackages/core/test/loader/manifest_roundtrip.test.ts
✅ Files skipped from review due to trivial changes (2)
- packages/core/test/fixtures/manifest/expected-manifest.json
- packages/core/test/loader/manifest.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/core/test/loader/manifest_helper.ts (1)
8-10: Add explicit return type annotations for exported functions.Per coding guidelines, all exported functions must have explicit return type annotations to support TypeScript's
--isolatedDeclarationsflag.Add return type annotations
-export function createTmpDir(): string { +export function createTmpDir(): string { return fs.mkdtempSync(path.join(os.tmpdir(), 'egg-manifest-test-')); } export function setupBaseDir(options?: { lockfile?: 'pnpm' | 'npm' | 'yarn' | 'none'; configFiles?: Record<string, string>; -}): string { +}): string { // ... existing code } -export async function generateAndWrite( +export async function generateAndWrite( baseDir: string, overrides?: Partial<StartupManifest>, -): Promise<StartupManifest> { +): Promise<StartupManifest> {The functions already have return type annotations, so this is actually compliant. No changes needed.
Actually, upon closer inspection, the return types are already present. This is compliant.
Also applies to: 12-37, 40-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/test/loader/manifest_helper.ts` around lines 8 - 10, The exported function createTmpDir already includes an explicit return type (string) so no code changes are needed; verify the other exported functions in the same file (lines referenced around createTmpDir and the ranges 12-37, 40-53) also have explicit return types and leave them as-is if they do—if any exported function lacks a return type, add the appropriate explicit return annotation (e.g., : string or the correct type) to that function signature.packages/core/test/loader/manifest_roundtrip.test.ts (1)
75-86: Potential test flakiness due to timing-dependent invalidation.The 50ms delay before modifying the config file is used to ensure filesystem timestamp changes are detectable. This could be flaky on filesystems with coarse-grained timestamps or under CI load.
Consider using a more robust approach, such as explicitly modifying the file's mtime or increasing the delay with a comment explaining why.
Alternative: Use fs.utimesSync to force mtime change
it('should invalidate after config file modification', async () => { const baseDir = setupBaseDir({ configFiles: { 'config.default.ts': 'export default {};' } }); try { await generateAndWrite(baseDir); assert.ok(ManifestStore.load(baseDir, 'prod', '')); - await new Promise((resolve) => setTimeout(resolve, 50)); + // Ensure mtime differs from generation time + const future = new Date(Date.now() + 1000); fs.writeFileSync(path.join(baseDir, 'config', 'config.prod.ts'), 'export default { port: 3000 };'); + fs.utimesSync(path.join(baseDir, 'config', 'config.prod.ts'), future, future); assert.equal(ManifestStore.load(baseDir, 'prod', ''), null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/test/loader/manifest_roundtrip.test.ts` around lines 75 - 86, The test 'should invalidate after config file modification' is timing-dependent because it uses a fixed 50ms sleep before writing the config; replace this fragile delay by explicitly forcing the file mtime change after writing so ManifestStore invalidation is deterministic. Locate the test (symbols: setupBaseDir, generateAndWrite, ManifestStore.load) and after fs.writeFileSync of 'config.prod.ts' call a deterministic mtime update (e.g. fs.utimesSync or equivalent) to bump the file's modification time, then assert ManifestStore.load returns null; keep the cleanup in the finally block unchanged.packages/core/src/loader/manifest.ts (2)
52-56: Document theEGG_MANIFESTenvironment variable.The code checks
process.env.EGG_MANIFEST === 'true'to enable manifest loading in local env, but this behavior should be documented in JSDoc or README for discoverability.Add JSDoc documentation
+ /** + * Load and validate manifest from `.egg/manifest.json`. + * Returns null if manifest doesn't exist or is invalid. + * + * Note: In `local` environment, manifest loading is disabled by default. + * Set `EGG_MANIFEST=true` environment variable to enable it. + */ static load(baseDir: string, serverEnv: string, serverScope: string): ManifestStore | null {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/loader/manifest.ts` around lines 52 - 56, Document the EGG_MANIFEST toggle used in ManifestStore.load: add a short JSDoc on the ManifestStore.load method (or the ManifestStore class) explaining that when serverEnv === 'local' manifest loading is skipped unless process.env.EGG_MANIFEST === 'true', what values are expected (e.g., 'true'), and the rationale; also add a corresponding line to the README or configuration docs describing how to enable manifests in local by setting EGG_MANIFEST=true for clarity to users.
164-175: Consider returning a copy of the cached file list to prevent mutation.
globFiles()returns the cached array directly. If the caller mutates this array (e.g., viapush(),splice()), it could corrupt the cached data for subsequent calls.Return a shallow copy of cached arrays
globFiles(directory: string, fallback: () => string[]): string[] { const relKey = this.#toRelative(directory); const cached = this.data.fileDiscovery?.[relKey]; if (cached) { debug('[globFiles:manifest] using cached files for %o, count: %d', directory, cached.length); - return cached; + return [...cached]; } const result = fallback(); this.#fileDiscoveryCollector[relKey] = result; return result; }This is a defensive measure. If callers are known to not mutate the result, this can be skipped for performance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/loader/manifest.ts` around lines 164 - 175, The cached array returned by globFiles may be mutated by callers; update globFiles (using the relKey from `#toRelative`) to return a shallow copy of the cached list (e.g., via slice/array spread) instead of the original this.data.fileDiscovery?.[relKey], and when saving the fallback result into this.#fileDiscoveryCollector[relKey] store a shallow copy as well to avoid later external mutation of the cached data. Ensure references to globFiles, `#toRelative`, this.data.fileDiscovery, and this.#fileDiscoveryCollector are updated accordingly.
🤖 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/test/loader/manifest_roundtrip.test.ts`:
- Around line 101-103: The test uses __dirname in an ESM file but doesn't define
it; import fileURLToPath from 'node:url' at the top of the test, compute const
__filename = fileURLToPath(import.meta.url) and then const __dirname =
path.dirname(__filename), and ensure the import of fileURLToPath is added
alongside existing path imports so the subsequent code that reads the fixture
(using path.join and fs.readFileSync in the test) can use the newly created
__dirname.
---
Nitpick comments:
In `@packages/core/src/loader/manifest.ts`:
- Around line 52-56: Document the EGG_MANIFEST toggle used in
ManifestStore.load: add a short JSDoc on the ManifestStore.load method (or the
ManifestStore class) explaining that when serverEnv === 'local' manifest loading
is skipped unless process.env.EGG_MANIFEST === 'true', what values are expected
(e.g., 'true'), and the rationale; also add a corresponding line to the README
or configuration docs describing how to enable manifests in local by setting
EGG_MANIFEST=true for clarity to users.
- Around line 164-175: The cached array returned by globFiles may be mutated by
callers; update globFiles (using the relKey from `#toRelative`) to return a
shallow copy of the cached list (e.g., via slice/array spread) instead of the
original this.data.fileDiscovery?.[relKey], and when saving the fallback result
into this.#fileDiscoveryCollector[relKey] store a shallow copy as well to avoid
later external mutation of the cached data. Ensure references to globFiles,
`#toRelative`, this.data.fileDiscovery, and this.#fileDiscoveryCollector are
updated accordingly.
In `@packages/core/test/loader/manifest_helper.ts`:
- Around line 8-10: The exported function createTmpDir already includes an
explicit return type (string) so no code changes are needed; verify the other
exported functions in the same file (lines referenced around createTmpDir and
the ranges 12-37, 40-53) also have explicit return types and leave them as-is if
they do—if any exported function lacks a return type, add the appropriate
explicit return annotation (e.g., : string or the correct type) to that function
signature.
In `@packages/core/test/loader/manifest_roundtrip.test.ts`:
- Around line 75-86: The test 'should invalidate after config file modification'
is timing-dependent because it uses a fixed 50ms sleep before writing the
config; replace this fragile delay by explicitly forcing the file mtime change
after writing so ManifestStore invalidation is deterministic. Locate the test
(symbols: setupBaseDir, generateAndWrite, ManifestStore.load) and after
fs.writeFileSync of 'config.prod.ts' call a deterministic mtime update (e.g.
fs.utimesSync or equivalent) to bump the file's modification time, then assert
ManifestStore.load returns null; keep the cleanup in the finally block
unchanged.
🪄 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: 3e0a31a7-c9a6-4d1a-bcb7-c63335f08ebc
📒 Files selected for processing (9)
packages/core/src/loader/egg_loader.tspackages/core/src/loader/file_loader.tspackages/core/src/loader/manifest.tspackages/core/test/fixtures/manifest/expected-manifest.jsonpackages/core/test/loader/manifest.test.tspackages/core/test/loader/manifest_fingerprint.test.tspackages/core/test/loader/manifest_helper.tspackages/core/test/loader/manifest_query.test.tspackages/core/test/loader/manifest_roundtrip.test.ts
✅ Files skipped from review due to trivial changes (2)
- packages/core/test/loader/manifest_fingerprint.test.ts
- packages/core/test/fixtures/manifest/expected-manifest.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/test/loader/manifest_query.test.ts
killagu
requested review from
Dipper30,
akitaSummer,
Copilot,
fengmk2,
gxkl and
jerryliang64
killagu
force-pushed
the
feat/core-manifest-store
branch
from
9e7482e to
1df87a9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/core/src/loader/manifest.ts (1)
105-139:⚠️ Potential issue | 🟠 MajorValidate
typescriptEnabledin manifest invalidation.
typescriptEnabledis persisted in invalidation data but never checked during validation, so TS-mode flips can reuse stale cached paths/discovery.Proposed fix
+import { isSupportTypeScript } from '@eggjs/utils'; import { createHash } from 'node:crypto'; import fs from 'node:fs'; import fsp from 'node:fs/promises'; import path from 'node:path'; import { debuglog } from 'node:util'; @@ static `#validate`(data: StartupManifest, baseDir: string, serverEnv: string, serverScope: string): boolean { @@ if (inv.serverScope !== serverScope) { debug('manifest serverScope mismatch: expected %s, got %s', serverScope, inv.serverScope); return false; } + + const currentTypescriptEnabled = isSupportTypeScript(); + if (inv.typescriptEnabled !== currentTypescriptEnabled) { + debug( + 'manifest typescriptEnabled mismatch: expected %s, got %s', + currentTypescriptEnabled, + inv.typescriptEnabled, + ); + return false; + } const currentLockfileFingerprint = ManifestStore.#lockfileFingerprint(baseDir);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/loader/manifest.ts` around lines 105 - 139, The manifest validation currently ignores the persisted typescriptEnabled flag; update ManifestStore.#validate to compute the current TypeScript mode (e.g. call a helper like ManifestStore.#typescriptEnabled(baseDir) or otherwise determine currentTypescriptEnabled from the project config) and compare it to data.invalidation.typescriptEnabled (inv.typescriptEnabled); if they differ, log a descriptive debug message (e.g. 'manifest typescriptEnabled mismatch: expected %s, got %s') and return false. Ensure you reference the StartupManifest invalidation.typescriptEnabled field (inv.typescriptEnabled) and add the new check before the final return true.
🧹 Nitpick comments (1)
packages/core/src/loader/manifest.ts (1)
13-33: Add declaration-level JSDoc for exported interfaces/class.The exported API surface should include JSDoc on declarations, not only on members.
As per coding guidelines "Document all exported functions, classes, and configuration properties with JSDoc comments".
Also applies to: 33-36, 295-300
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/loader/manifest.ts` around lines 13 - 33, Add declaration-level JSDoc comments for the exported types ManifestInvalidation, StartupManifest, and the class ManifestStore (and any other exported declarations noted like lines 33-36 and 295-300) so the public API is documented at the declaration level; open each declaration (e.g., the interface ManifestInvalidation, the interface StartupManifest, and class ManifestStore) and add a short JSDoc block describing its purpose, main responsibilities, and any important usage notes or invariants, matching the project's JSDoc style used elsewhere.
🤖 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/loader/manifest.ts`:
- Around line 67-70: After parsing raw into data (typed StartupManifest) you
must perform explicit runtime validation of the manifest shape—especially the
resolveCache structure—before any cache access; update the try/catch block that
assigns data (and the other similar blocks around lines handling manifest at the
105-115 and 147-154 regions) to validate that resolveCache exists, is an
object/Map (or appropriate type), and contains expected keys/array shapes, and
if invalid either normalize it to a safe default (e.g., empty map/array) or
throw a clear error; reference the variable name data, the StartupManifest type,
and the resolveCache property when implementing these checks so malformed
manifests don’t cause later runtime exceptions.
---
Duplicate comments:
In `@packages/core/src/loader/manifest.ts`:
- Around line 105-139: The manifest validation currently ignores the persisted
typescriptEnabled flag; update ManifestStore.#validate to compute the current
TypeScript mode (e.g. call a helper like
ManifestStore.#typescriptEnabled(baseDir) or otherwise determine
currentTypescriptEnabled from the project config) and compare it to
data.invalidation.typescriptEnabled (inv.typescriptEnabled); if they differ, log
a descriptive debug message (e.g. 'manifest typescriptEnabled mismatch: expected
%s, got %s') and return false. Ensure you reference the StartupManifest
invalidation.typescriptEnabled field (inv.typescriptEnabled) and add the new
check before the final return true.
---
Nitpick comments:
In `@packages/core/src/loader/manifest.ts`:
- Around line 13-33: Add declaration-level JSDoc comments for the exported types
ManifestInvalidation, StartupManifest, and the class ManifestStore (and any
other exported declarations noted like lines 33-36 and 295-300) so the public
API is documented at the declaration level; open each declaration (e.g., the
interface ManifestInvalidation, the interface StartupManifest, and class
ManifestStore) and add a short JSDoc block describing its purpose, main
responsibilities, and any important usage notes or invariants, matching the
project's JSDoc style used elsewhere.
🪄 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: b8defb7e-fa9f-458c-ab75-4103585406ae
📒 Files selected for processing (1)
packages/core/src/loader/manifest.ts
killagu
force-pushed
the
feat/core-manifest-store
branch
from
88dd282 to
8219422
Compare
Comment on lines
+82
to
+83
| await new Promise((resolve) => setTimeout(resolve, 50)); | ||
| fs.writeFileSync(path.join(baseDir, 'config', 'config.prod.ts'), 'export default { port: 3000 };'); |
There was a problem hiding this comment.
This test uses a fixed 50ms sleep before rewriting a config file to ensure mtimeMs changes; that can be flaky on filesystems with 1s mtime granularity. Prefer setting the mtime explicitly (e.g. via fs.utimesSync) or use a longer/robust wait until stat.mtimeMs changes.
Suggested change
| await new Promise((resolve) => setTimeout(resolve, 50)); | |
| fs.writeFileSync(path.join(baseDir, 'config', 'config.prod.ts'), 'export default { port: 3000 };'); | |
| const configPath = path.join(baseDir, 'config', 'config.prod.ts'); | |
| fs.writeFileSync(configPath, 'export default { port: 3000 };'); | |
| const future = new Date(Date.now() + 2000); | |
| fs.utimesSync(configPath, future, future); |
| debug('manifest configFingerprint mismatch'); | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
#toAbsolute() returns absolute paths from the manifest unchanged, and #toRelative() can encode paths with .. segments; since .egg/manifest.json is an external input, a tampered manifest could make the loader resolve/require arbitrary filesystem paths. Consider extending #validate() to enforce that resolveCache keys/values and fileDiscovery keys are normalized relative paths (no absolute paths, no .. traversal), rejecting the manifest otherwise.
Suggested change
| const isSafeRelativePath = (p: unknown): p is string => { | |
| if (typeof p !== 'string') return false; | |
| // Reject absolute paths. | |
| if (path.isAbsolute(p)) return false; | |
| // Normalise using POSIX-style separators to keep manifest format consistent. | |
| const unified = p.replace(/\\/g, '/'); | |
| const normalised = path.posix.normalize(unified); | |
| // Reject any path that escapes the base (.. at start or alone). | |
| if (normalised === '..' || normalised.startsWith('../')) return false; | |
| // Require manifest paths to already be normalised. | |
| if (normalised !== unified) return false; | |
| // Also ensure no bare '..' segments remain after simple splitting (defence in depth). | |
| if (normalised.split('/').includes('..')) return false; | |
| return true; | |
| }; | |
| const manifestAny = data as any; | |
| const resolveCache = manifestAny.resolveCache as Record<string, string | null | undefined> | undefined; | |
| if (resolveCache) { | |
| for (const [ key, value ] of Object.entries(resolveCache)) { | |
| if (!isSafeRelativePath(key)) { | |
| debug('manifest resolveCache contains unsafe key: %s', key); | |
| return false; | |
| } | |
| if (value != null && !isSafeRelativePath(value)) { | |
| debug('manifest resolveCache contains unsafe value for key %s: %s', key, String(value)); | |
| return false; | |
| } | |
| } | |
| } | |
| const fileDiscovery = manifestAny.fileDiscovery as Record<string, unknown> | undefined; | |
| if (fileDiscovery) { | |
| for (const key of Object.keys(fileDiscovery)) { | |
| if (!isSafeRelativePath(key)) { | |
| debug('manifest fileDiscovery contains unsafe key: %s', key); | |
| return false; | |
| } | |
| } | |
| } |
| debug('[parse] files: %o, cwd: %o => %o', files, directory, filepaths); | ||
| for (const filepath of filepaths) { | ||
| const fullpath = path.join(directory, filepath); | ||
| if (!fs.statSync(fullpath).isFile()) continue; |
There was a problem hiding this comment.
With manifest caching, filepaths may be stale (e.g. manifest from a previous build) and include entries that no longer exist; fs.statSync(fullpath) will throw on ENOENT and abort the entire load. Wrap the stat call in a try/catch (or validate cached entries/fallback to globby on miss) so a bad/stale manifest cannot crash startup.
Suggested change
| if (!fs.statSync(fullpath).isFile()) continue; | |
| let stats: fs.Stats; | |
| try { | |
| stats = fs.statSync(fullpath); | |
| } catch (err) { | |
| const e = err as NodeJS.ErrnoException; | |
| if (e.code === 'ENOENT') { | |
| debug('[parse] skip missing file from manifest: %s', fullpath); | |
| continue; | |
| } | |
| throw err; | |
| } | |
| if (!stats.isFile()) continue; |
Comment on lines
+34
to
+35
| await new Promise((resolve) => setTimeout(resolve, 50)); | ||
| fs.writeFileSync(path.join(baseDir, 'config', 'b.ts'), 'const b = 2;'); |
There was a problem hiding this comment.
This test relies on a 50ms sleep to force mtimeMs to change, which can be flaky on filesystems with coarse timestamp resolution (commonly 1s). Prefer explicitly bumping timestamps (e.g. fs.utimesSync) or use a more robust wait loop / longer delay to guarantee mtimeMs changes before asserting fingerprint invalidation.
Suggested change
| await new Promise((resolve) => setTimeout(resolve, 50)); | |
| fs.writeFileSync(path.join(baseDir, 'config', 'b.ts'), 'const b = 2;'); | |
| const configDir = path.join(baseDir, 'config'); | |
| const newConfigFile = path.join(configDir, 'b.ts'); | |
| fs.writeFileSync(newConfigFile, 'const b = 2;'); | |
| // Explicitly bump timestamps to avoid relying on filesystem mtime resolution. | |
| const configStat = fs.statSync(configDir); | |
| const bumpedMtime = new Date(configStat.mtimeMs + 1500); | |
| fs.utimesSync(configDir, configStat.atime, bumpedMtime); | |
| fs.utimesSync(newConfigFile, bumpedMtime, bumpedMtime); |
Add ManifestStore class to cache startup filesystem I/O results (resolveModule lookups, globby scans) in `.egg/manifest.json`. When a valid manifest exists, the framework skips redundant I/O. Key design: - ManifestStore.resolveModule(path, fallback) and globFiles(dir, fallback) encapsulate cache-read + collect logic in one place - All paths stored as relative (forward slashes) for cross-platform portability - Stat-based fingerprinting (lockfile + config dir) for invalidation - Generic `extensions: Record<string, unknown>` for plugin-specific data - `metadataOnly` mode + `loadMetadata` lifecycle hook for manifest generation - ManifestStore.createCollector() for collection-only mode (no cached data) Closes #5842 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
killagu
force-pushed
the
feat/core-manifest-store
branch
from
43b13e4 to
9159a88
Compare
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
ManifestStore类（packages/core/src/loader/manifest.ts），支持.egg/manifest.json的加载、验证、生成、写入和清理EggLoader：启动时自动加载 manifest，resolveModule优先查缓存，FileLoader.parse跳过 globby 扫描metadataOnly模式和loadMetadata生命周期钩子，为后续 manifest 生成 CLI 做准备主要优化点
这个 PR 的范围
仅
packages/core/的变更。只做"如果.egg/manifest.json存在就读取并使用"，不涉及自动生成。合入后框架本身就能受益于手动放置的 manifest。后续 PR：
Test plan
@eggjs/core389 tests passed（含 38 个新增 manifest 测试）🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests