fix(egg): fallback runtime diagnostic config#5933
Conversation
Deploying egg-v3 with
|
| Latest commit: |
45e50f6
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0dfda575.egg-v3.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-3f8f113e.egg-v3.pages.dev |
📝 WalkthroughWalkthroughRuntime directory resolution and worker-start timeout were centralized: added DEFAULT_WORKER_START_TIMEOUT, getRuntimeRundir(), and getWorkerStartTimeout(); dumpConfig/dumpTiming now ensure the runtime directory exists before writing; _setupTimeoutTimer uses the new timeout logic. Tests covering rundir fallback and timeout defaults were added. ChangesRuntime directory and worker timeout
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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 fallback logic for the rundir and workerStartTimeout configurations, ensuring diagnostic files are dumped to a default directory when not specified. It also updates directory creation to use recursive mode. The review feedback identifies several redundant fs.existsSync checks that can be simplified because fs.mkdirSync with recursive: true is idempotent. Additionally, one check in application.ts is noted as unnecessary as it is already performed by the base class.
| if (!fs.existsSync(rundir)) { | ||
| fs.mkdirSync(rundir, { recursive: true }); | ||
| } | ||
| const dumpRouterFile = path.join(rundir, 'router.json'); |
There was a problem hiding this comment.
This directory creation logic is redundant because super.dumpConfig() (called at line 200) already ensures that rundir exists using the same logic. Additionally, fs.mkdirSync with { recursive: true } is idempotent, making the fs.existsSync check unnecessary.
const dumpRouterFile = path.join(rundir, 'router.json');| if (!fs.existsSync(rundir)) { | ||
| fs.mkdirSync(rundir); | ||
| fs.mkdirSync(rundir, { recursive: true }); | ||
| } |
There was a problem hiding this comment.
The fs.existsSync check is redundant when using fs.mkdirSync with { recursive: true }, as it is idempotent and will not throw if the directory already exists. Simplifying this improves code clarity.
fs.mkdirSync(rundir, { recursive: true });| if (!fs.existsSync(rundir)) { | ||
| fs.mkdirSync(rundir, { recursive: true }); | ||
| } |
There was a problem hiding this comment.
The fs.existsSync check is redundant here as well since fs.mkdirSync with { recursive: true } handles existence checks internally.
fs.mkdirSync(rundir, { recursive: true });![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/egg/src/lib/egg.ts`:
- Around line 679-683: The getWorkerStartTimeout method currently treats any
finite number (including 0 and negatives) from this.config.workerStartTimeout as
valid; change the validation in getWorkerStartTimeout to accept only positive
finite numbers (> 0) and otherwise return the default timeout value so
non-positive values fall back to default. Locate getWorkerStartTimeout and
update the check that inspects this.config.workerStartTimeout to require
workerStartTimeout > 0 (in addition to typeof === 'number' and Number.isFinite)
before returning it.
🪄 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: 6cbfd9df-2073-43b8-94e0-551cca2f06d6
📒 Files selected for processing (3)
packages/egg/src/lib/application.tspackages/egg/src/lib/egg.tspackages/egg/test/egg.test.ts
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #5933 +/- ##
=======================================
Coverage 85.18% 85.19%
=======================================
Files 668 668
Lines 19284 19298 +14
Branches 3782 3783 +1
=======================================
+ Hits 16428 16440 +12
- Misses 2464 2466 +2
Partials 392 392 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR improves Egg’s runtime diagnostics robustness by ensuring config/timing/router dumps still work when config.rundir is missing, and by defaulting workerStartTimeout when it’s missing/invalid.
Changes:
- Add
getRuntimeRundir()fallback and use it fordumpConfig(),dumpTiming(), and router dump output. - Default
workerStartTimeoutvia a helper (getWorkerStartTimeout()) and use it for the startup timeout timer. - Add a Vitest coverage block validating the rundir fallback and timeout defaulting behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/egg/src/lib/egg.ts | Adds rundir/timeout fallback helpers and routes diagnostics/start-timeout logic through them |
| packages/egg/src/lib/application.ts | Reuses runtime rundir fallback for router dump and ensures directory creation |
| packages/egg/test/egg.test.ts | Adds tests covering missing rundir dump behavior and missing workerStartTimeout defaulting |
|
|
||
| private getWorkerStartTimeout(): number { | ||
| const workerStartTimeout = this.config.workerStartTimeout; | ||
| if (typeof workerStartTimeout === 'number' && Number.isFinite(workerStartTimeout)) { |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/egg/test/egg.test.ts (1)
294-310: ⚡ Quick winAvoid duplicating the default-timeout magic number; import the constant instead.
10 * 60 * 1000is repeated four times here and is also defined asDEFAULT_WORKER_START_TIMEOUTinpackages/egg/src/lib/egg.ts. If the default ever changes, this test will silently drift. Consider exporting the constant fromegg.tsand importing it here so the test stays locked to the source of truth.♻️ Suggested change
In
packages/egg/src/lib/egg.ts:-const DEFAULT_WORKER_START_TIMEOUT = 10 * 60 * 1000; +export const DEFAULT_WORKER_START_TIMEOUT = 10 * 60 * 1000;In this test file:
-import { createApp, cluster, getFilepath, type MockApplication } from './utils.ts'; +import { createApp, cluster, getFilepath, type MockApplication } from './utils.ts'; +import { DEFAULT_WORKER_START_TIMEOUT } from '../src/lib/egg.ts'; @@ - Reflect.set(app.config, 'workerStartTimeout', undefined); - assert.equal((app as any).getWorkerStartTimeout(), 10 * 60 * 1000); - Reflect.set(app.config, 'workerStartTimeout', 0); - assert.equal((app as any).getWorkerStartTimeout(), 10 * 60 * 1000); - Reflect.set(app.config, 'workerStartTimeout', -1); - assert.equal((app as any).getWorkerStartTimeout(), 10 * 60 * 1000); - Reflect.set(app.config, 'workerStartTimeout', Number.POSITIVE_INFINITY); - assert.equal((app as any).getWorkerStartTimeout(), 10 * 60 * 1000); + Reflect.set(app.config, 'workerStartTimeout', undefined); + assert.equal((app as any).getWorkerStartTimeout(), DEFAULT_WORKER_START_TIMEOUT); + Reflect.set(app.config, 'workerStartTimeout', 0); + assert.equal((app as any).getWorkerStartTimeout(), DEFAULT_WORKER_START_TIMEOUT); + Reflect.set(app.config, 'workerStartTimeout', -1); + assert.equal((app as any).getWorkerStartTimeout(), DEFAULT_WORKER_START_TIMEOUT); + Reflect.set(app.config, 'workerStartTimeout', Number.POSITIVE_INFINITY); + assert.equal((app as any).getWorkerStartTimeout(), DEFAULT_WORKER_START_TIMEOUT);🤖 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/test/egg.test.ts` around lines 294 - 310, The test duplicates the default timeout literal (10 * 60 * 1000) instead of using the canonical constant; import DEFAULT_WORKER_START_TIMEOUT from the module that defines it (packages/egg/src/lib/egg.ts) and replace the four literal occurrences in the test for getWorkerStartTimeout() with that constant so the test follows the single source of truth (ensure the import name matches the export in egg.ts).
🤖 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.
Nitpick comments:
In `@packages/egg/test/egg.test.ts`:
- Around line 294-310: The test duplicates the default timeout literal (10 * 60
* 1000) instead of using the canonical constant; import
DEFAULT_WORKER_START_TIMEOUT from the module that defines it
(packages/egg/src/lib/egg.ts) and replace the four literal occurrences in the
test for getWorkerStartTimeout() with that constant so the test follows the
single source of truth (ensure the import name matches the export in egg.ts).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2eee452f-ffe7-492f-a243-9ce8da06b24e
📒 Files selected for processing (3)
packages/egg/src/lib/application.tspackages/egg/src/lib/egg.tspackages/egg/test/egg.test.ts
Deploying egg with
|
| Latest commit: |
45e50f6
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fbd8cd6f.egg-cci.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-3f8f113e.egg-cci.pages.dev |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/egg/src/lib/egg.ts (1)
569-587: 💤 Low value
rundircomputed outside thetryblock — minor style inconsistency withdumpTimingLine 570 resolves
rundirbefore entering thetry, whiledumpTiming(lines 592–593) resolves it inside. SincegetRuntimeRundir()is infallible this makes no functional difference, but moving line 570 inside thetrywould make the two methods consistent and mean any hypothetical future failure is also caught by the existing handler.♻️ Proposed alignment
dumpConfig(): void { - const rundir = this.getRuntimeRundir(); try { + const rundir = this.getRuntimeRundir(); fs.mkdirSync(rundir, { recursive: true });🤖 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 569 - 587, The variable rundir is computed with getRuntimeRundir() before the try in dumpConfig(), causing a style inconsistency with dumpTiming() and leaving a hypothetical failure uncaught; fix by moving the call to getRuntimeRundir() and the rundir variable declaration inside the existing try block in the dumpConfig() method (so all filesystem ops and rundir resolution are covered), keeping the rest of the logic—writing dumpFile and dumpMetaFile and the catch logging—unchanged.
🤖 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.
Nitpick comments:
In `@packages/egg/src/lib/egg.ts`:
- Around line 569-587: The variable rundir is computed with getRuntimeRundir()
before the try in dumpConfig(), causing a style inconsistency with dumpTiming()
and leaving a hypothetical failure uncaught; fix by moving the call to
getRuntimeRundir() and the rundir variable declaration inside the existing try
block in the dumpConfig() method (so all filesystem ops and rundir resolution
are covered), keeping the rest of the logic—writing dumpFile and dumpMetaFile
and the catch logging—unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60434787-d937-4fc3-bb53-cfb73adfa0aa
📒 Files selected for processing (2)
packages/egg/src/lib/application.tspackages/egg/src/lib/egg.ts
|
Closing this PR because the fallback approach masks the actual bundled-loader failure. Root cause checked from source: Egg already defines This PR should not merge as-is; EGG-85 should depend on #5930 / EGG-80 and then re-check the startup logs without adding diagnostic fallbacks. |
|
Closing in favor of the root loader-path fix in #5930. The undefined values come from Egg framework config not being loaded in the bundled worker, not from missing diagnostic fallbacks. |
Summary
${baseDir}/runwhenconfig.rundiris missing.workerStartTimeoutto the Egg 10 minute startup timeout.Verification
pnpm exec vitest run packages/egg/test/egg.test.ts --testNamePattern "runtime diagnostics fallback config"pnpm --filter egg typecheckpnpm exec vitest run packages/egg/test/egg.test.tsMultica issue: EGG-85
Summary by CodeRabbit