feat(scripts): salvage single-mode, IPC readiness, framework resolver and esbuild plugins#5862
feat(scripts): salvage single-mode, IPC readiness, framework resolver and esbuild plugins#5862
Conversation
📝 WalkthroughWalkthroughAdds single-process startup support and ESM/tooling utilities to tools/scripts: new start-single scripts (CJS/ESM), CLI flag and readiness IPC, framework entry resolution, ESM polyfill and file-url resolver esbuild plugins, tests, and package export/runtime updates. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (start command)
participant Launcher as Launcher (start-single script)
participant Framework as Framework module
participant Server as HTTP Server
participant Parent as Parent process (daemon manager)
CLI->>Launcher: spawn with JSON options (port, baseDir, framework,...)
Launcher->>Framework: dynamic import and resolve start/startEgg
Framework-->>Launcher: returns app (with callback(), close())
Launcher->>Server: create server from app.callback() and listen
Server-->>Launcher: 'listening' (address, port)
Launcher->>Parent: process.send({ type: "egg-ready", address, port })
Parent-->>CLI: receives readiness
Note over Launcher,Framework: On SIGTERM/SIGINT/SIGQUIT\nclose server -> app.close() -> exit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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 |
Deploying egg-v3 with
|
| Latest commit: |
959e604
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://20c69538.egg-v3.pages.dev |
| Branch Preview URL: | https://refactor-remove-snapshot-bui.egg-v3.pages.dev |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a single-process mode for starting applications, complementing the existing cluster mode. Key changes include the addition of start-single scripts (CJS and ESM), a new --single flag for the start command, and a resolveFrameworkEntry utility to handle various framework layouts. The startup synchronization logic was refactored from polling to an IPC-based waitForReady mechanism. Review feedback suggests ensuring the ESM start script supports default exports for consistency, improving child process error and exit handling during startup, and using the local child reference instead of the class member for better reliability.
| const framework = await importModule(options.framework); | ||
| const startEgg = framework.start ?? framework.startEgg; | ||
| if (typeof startEgg !== 'function') { | ||
| throw new Error(`Cannot find start/startEgg function from framework: ${options.framework}`); | ||
| } |
There was a problem hiding this comment.
The ESM version of the start script should also check for the .default export to be consistent with the CJS version and to support frameworks that use default exports.
const framework = await importModule(options.framework);
let startEgg = framework.start ?? framework.startEgg;
if (typeof startEgg !== 'function') {
startEgg = framework.default?.start ?? framework.default?.startEgg;
}
if (typeof startEgg !== 'function') {
throw new Error("Cannot find start/startEgg function from framework: " + options.framework);
}| child.on('exit', (code) => { | ||
| if (code) { | ||
| clearTimeout(timer); | ||
| reject(new Error(`Child process exited with code ${code}`)); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The waitForReady function should handle the error event on the child process and should also reject if the process exits (even with code 0) before the egg-ready message is received, as any exit during startup is considered a failure.
child.on('exit', (code, signal) => {
clearTimeout(timer);
const message = signal ? "Child process killed by signal " + signal : "Child process exited with code " + code;
reject(new Error(message));
});
child.on('error', (err) => {
clearTimeout(timer);
reject(err);
});| } | ||
|
|
||
| if (!isSuccess) { | ||
| this.#child.kill('SIGTERM'); |
There was a problem hiding this comment.
Use the child parameter passed to the function instead of the private member this.#child for better consistency and to avoid potential issues if the member is reassigned.
| this.#child.kill('SIGTERM'); | |
| child.kill('SIGTERM'); |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #5862 +/- ##
==========================================
- Coverage 86.00% 85.97% -0.04%
==========================================
Files 667 669 +2
Lines 18945 18991 +46
Branches 3652 3667 +15
==========================================
+ Hits 16294 16327 +33
- Misses 2297 2308 +11
- Partials 354 356 +2 ☔ 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 enhances @eggjs/scripts production lifecycle tooling by adding a single-process start path, improving startup readiness detection via IPC, broadening process detection for stop/cleanup flows, and introducing a framework entry resolver helper.
Changes:
- Add
--singlestart mode with newstart-single.{mjs,cjs}launchers andtsxruntime loading for TypeScript framework sources. - Replace polling-based startup readiness with an IPC
egg-readylistener (waitForReady). - Add
resolveFrameworkEntry()helper with tests; update process matching to recognizeegg-server*andstart-single.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/scripts/tsdown.config.ts | Adds tsdown build config and marks tsx as an unused-exports ignore due to runtime usage. |
| tools/scripts/test/utils.ts | Updates test cleanup logic to recognize and terminate start-single processes. |
| tools/scripts/test/resolve-framework-entry.test.ts | Adds unit tests for framework entry resolution across layouts/exports shapes. |
| tools/scripts/src/resolve-framework-entry.ts | Implements recursive package.json exports/main entry resolution with worktree preference. |
| tools/scripts/src/helper.ts | Broadens process scanning to include egg-server-titled processes. |
| tools/scripts/src/commands/stop.ts | Expands stop-process detection to include start-single and egg-server* titles. |
| tools/scripts/src/commands/start.ts | Adds --single, injects --import=tsx/esm for single mode, and switches readiness to IPC-based waitForReady. |
| tools/scripts/scripts/start-single.mjs | New ESM single-mode server launcher that emits egg-ready over IPC. |
| tools/scripts/scripts/start-single.cjs | New CJS single-mode server launcher that emits egg-ready over IPC. |
| tools/scripts/package.json | Exposes resolve-framework-entry export and adds tsx as a runtime dependency. |
| const timer = setTimeout(() => { | ||
| reject(new Error(`Start failed, ${timeoutMs / 1000}s timeout`)); | ||
| }, timeoutMs); | ||
|
|
||
| child.on('message', (msg: any) => { | ||
| // https://github.com/eggjs/cluster/blob/master/src/master.ts#L119 | ||
| if (msg && msg.action === 'egg-ready') { | ||
| clearTimeout(timer); | ||
| this.isReady = true; | ||
| this.log('%s started on %s', readyLabel, msg.data.address); | ||
| child.unref(); | ||
| child.disconnect(); | ||
| resolve(); | ||
| } | ||
| }); | ||
|
|
||
| child.on('exit', (code) => { | ||
| if (code) { | ||
| clearTimeout(timer); | ||
| reject(new Error(`Child process exited with code ${code}`)); | ||
| } | ||
| }); |
There was a problem hiding this comment.
In waitForReady(), the child 'exit' handler only rejects when code is truthy. If the child exits before sending egg-ready with code === 0 (or exits via a signal where code is null), the promise will wait until timeout and report a misleading timeout error. Consider rejecting on any exit-before-ready (capturing both code and signal) and clearing listeners/timer accordingly.
| const timer = setTimeout(() => { | |
| reject(new Error(`Start failed, ${timeoutMs / 1000}s timeout`)); | |
| }, timeoutMs); | |
| child.on('message', (msg: any) => { | |
| // https://github.com/eggjs/cluster/blob/master/src/master.ts#L119 | |
| if (msg && msg.action === 'egg-ready') { | |
| clearTimeout(timer); | |
| this.isReady = true; | |
| this.log('%s started on %s', readyLabel, msg.data.address); | |
| child.unref(); | |
| child.disconnect(); | |
| resolve(); | |
| } | |
| }); | |
| child.on('exit', (code) => { | |
| if (code) { | |
| clearTimeout(timer); | |
| reject(new Error(`Child process exited with code ${code}`)); | |
| } | |
| }); | |
| let settled = false; | |
| const cleanup = () => { | |
| clearTimeout(timer); | |
| child.off('message', onMessage); | |
| child.off('exit', onExit); | |
| }; | |
| const finishResolve = () => { | |
| if (settled) return; | |
| settled = true; | |
| cleanup(); | |
| resolve(); | |
| }; | |
| const finishReject = (err: Error) => { | |
| if (settled) return; | |
| settled = true; | |
| cleanup(); | |
| reject(err); | |
| }; | |
| const timer = setTimeout(() => { | |
| finishReject(new Error(`Start failed, ${timeoutMs / 1000}s timeout`)); | |
| }, timeoutMs); | |
| const onMessage = (msg: any) => { | |
| // https://github.com/eggjs/cluster/blob/master/src/master.ts#L119 | |
| if (msg && msg.action === 'egg-ready') { | |
| this.isReady = true; | |
| this.log('%s started on %s', readyLabel, msg.data.address); | |
| child.unref(); | |
| child.disconnect(); | |
| finishResolve(); | |
| } | |
| }; | |
| const onExit = (code: number | null, signal: NodeJS.Signals | null) => { | |
| const reason = signal ? `signal ${signal}` : `code ${code}`; | |
| finishReject(new Error(`Child process exited before ready with ${reason}`)); | |
| }; | |
| child.on('message', onMessage); | |
| child.on('exit', onExit); |
| @@ -104,8 +108,9 @@ export default class Start<T extends typeof Start> extends BaseCommand<T> { | |||
| return name; | |||
| } | |||
|
|
|||
| protected async getServerBin(): Promise<string> { | |||
| const serverBinName = this.isESM ? 'start-cluster.mjs' : 'start-cluster.cjs'; | |||
| protected async getServerBin(single?: boolean): Promise<string> { | |||
| const prefix = single ? 'start-single' : 'start-cluster'; | |||
| const serverBinName = this.isESM ? `${prefix}.mjs` : `${prefix}.cjs`; | |||
| return path.join(import.meta.dirname, '../../scripts', serverBinName); | |||
| } | |||
|
|
|||
| @@ -145,8 +150,12 @@ export default class Start<T extends typeof Start> extends BaseCommand<T> { | |||
| // normalize env | |||
| this.env.HOME = HOME; | |||
| this.env.NODE_ENV = 'production'; | |||
| // disable ts file loader | |||
| this.env.EGG_TS_ENABLE = 'false'; | |||
| // Disable ts file loader in cluster mode. | |||
| // In single mode, Node.js 22.18+ native type stripping handles .ts files. | |||
| const isSingle = flags.single; | |||
| if (!isSingle) { | |||
| this.env.EGG_TS_ENABLE = 'false'; | |||
| } | |||
|
|
|||
| // it makes env big but more robust | |||
| this.env.PATH = this.env.Path = [ | |||
| @@ -173,6 +182,11 @@ export default class Start<T extends typeof Start> extends BaseCommand<T> { | |||
|
|
|||
| // additional execArgv | |||
| const execArgv: string[] = ['--no-deprecation', '--trace-warnings']; | |||
| // Single mode loads framework .ts source directly; use tsx for full TypeScript | |||
| // support including decorators (Node.js native type stripping can't handle them). | |||
| if (isSingle) { | |||
| execArgv.push('--import=tsx/esm'); | |||
| } | |||
There was a problem hiding this comment.
New --single mode (including tsx-based execArgv and different env/flag normalization) isn’t covered by the existing Vitest suite. Please add tests that start in single mode (daemon + non-daemon if supported) and verify readiness (egg-ready), stop behavior, and log output so regressions are caught.
| const matched = flags.title | ||
| ? cmd.includes('start-cluster') && cmd.includes(format(osRelated.titleTemplate, flags.title)) | ||
| : cmd.includes('start-cluster'); | ||
| ? isEggProcess && cmd.includes(format(osRelated.titleTemplate, flags.title)) |
There was a problem hiding this comment.
stop --title currently requires the command line to include the serialized JSON "title":"...". For processes detected via cmd.startsWith('egg-server') (process-title based), that JSON substring may not exist, so --title won’t match even though the process title does. Consider also matching cmd.startsWith(flags.title) (or an equivalent title-prefix match) when cmd.startsWith('egg-server') is used.
| ? isEggProcess && cmd.includes(format(osRelated.titleTemplate, flags.title)) | |
| ? isEggProcess && | |
| (cmd.includes(format(osRelated.titleTemplate, flags.title)) || cmd.startsWith(flags.title)) |
|
|
||
| // Installed layout: read package.json | ||
| const pkgPath = path.join(frameworkPath, 'package.json'); | ||
| const pkg = JSON.parse(readFileSync(pkgPath, 'utf-8')); |
There was a problem hiding this comment.
resolveFrameworkEntry() reads/parses package.json without guarding for missing/invalid files, so callers will get low-level ENOENT/JSON parse errors rather than a consistent, actionable message like the later "Framework entry not found" error. Consider wrapping the package.json read/parse in a try/catch and throwing an Error that includes frameworkPath and pkgPath with next-step guidance.
| const pkg = JSON.parse(readFileSync(pkgPath, 'utf-8')); | |
| let pkg: Record<string, any>; | |
| try { | |
| pkg = JSON.parse(readFileSync(pkgPath, 'utf-8')); | |
| } catch (error) { | |
| const reason = error instanceof Error ? error.message : String(error); | |
| throw new Error( | |
| `Failed to read framework package metadata for ${frameworkPath} from ${pkgPath}: ${reason}. ` + | |
| 'Ensure the framework package exists, is installed or built, and has a valid package.json with usable "main" or "exports" fields.', | |
| ); | |
| } |
Deploying egg with
|
| Latest commit: |
959e604
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d756f730.egg-cci.pages.dev |
| Branch Preview URL: | https://refactor-remove-snapshot-bui.egg-cci.pages.dev |
… and esbuild plugins
Independent-value infrastructure salvaged from the abandoned feat/snapshot-scripts branch:
- --single mode start + start-single.mjs/cjs launchers with --import=tsx/esm
- IPC-based waitForReady replacing 1s polling checkStatus
- egg-server process title matching in stop.ts / helper.ts for single-mode
and any future bundle/snapshot title-based processes
- resolveFrameworkEntry generic helper (reads package.json exports[.]/main)
supporting worktree, installed CJS, and ESM conditional-exports layouts
- esmPolyfillPlugin (import.meta.{dirname,url,filename} + __dirname) and
fileUrlResolverPlugin esbuild plugins, exported for reuse
Context: --build-snapshot based approach abandoned after multiple POCs
revealed architectural dead-ends. These pieces have value independent of
any snapshot strategy and are salvaged into a standalone PR.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
killagu
force-pushed
the
refactor/remove-snapshot-build
branch
from
509c449 to
959e604
Compare
killagu
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
tools/scripts/tsdown.config.ts (1)
3-12: Use a typed intermediate config object before export.Please align this config with the repo’s typed-config convention.
Suggested refactor
import { defineConfig } from 'tsdown'; -export default defineConfig({ +const config = { entry: 'src/**/*.ts', unbundle: true, external: [/^@eggjs\//, 'egg'], unused: { level: 'error', // tsx is used at runtime via --import=tsx/esm in spawned processes, not directly imported ignore: ['tsx', 'runscript'], }, -}); +} satisfies Parameters<typeof defineConfig>[0]; + +export default defineConfig(config);Based on learnings: Applies to {tsdown.config.ts,vitest.config.ts} : Use typed intermediate variables with appropriate types (UserConfig, UserWorkspaceConfig) for tsdown.config.ts and vitest.config.ts configuration files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/scripts/tsdown.config.ts` around lines 3 - 12, The exported default should be a typed intermediate config object instead of an inline literal; create a const (e.g., const config: UserConfig) with the same properties (entry, unbundle, external, unused) and then export default defineConfig(config). Update imports/types if needed so UserConfig (or the correct tsdown config type) is referenced, and keep defineConfig(config) to preserve existing behavior; apply the same pattern to related files like vitest.config.ts.tools/scripts/test/resolve-framework-entry.test.ts (1)
10-123: Please add orchestration regressions for both CJS and TS example apps.Given this PR changes startup/readiness/stop behavior, add integration tests that exercise
start --single+egg-ready+stopin both CommonJS and TypeScript fixtures.Based on learnings: Add regression cases that exercise both CommonJS and TypeScript example apps when features affect HTTP or process orchestration.
tools/scripts/src/commands/start.ts (2)
185-189: Please coverstart --singlewith real-app regressions.This path now depends on both
tsx/esmloading and theegg-readyIPC handshake. The unit coverage mentioned in the PR is useful, but it will not catch the integration breakage this change can introduce; please boot one CommonJS example and one TypeScript example through the new single-process path, ideally in daemon mode too.Based on learnings: Add regression cases that exercise both CommonJS and TypeScript example apps when features affect HTTP or process orchestration
Also applies to: 288-289
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/scripts/src/commands/start.ts` around lines 185 - 189, Add integration regression tests that exercise the new single-process path triggered by isSingle (where execArgv.push('--import=tsx/esm') is applied) and the egg-ready IPC handshake: create two end-to-end test cases that launch the CLI command used in start.ts with --single (and also run the same tests in daemon mode) — one booting a CommonJS example app and one booting a TypeScript example app — and assert that the app performs basic HTTP behavior and completes the egg-ready handshake (process lifecycle/IPC) successfully; place these tests alongside existing CLI/integration tests and reuse the same start/stop helpers so they run in CI and catch regressions affecting process orchestration or HTTP endpoints.
320-325: Define a type for the IPC message payload instead of usingany.Line 320 uses
msg: anyfor the egg-ready IPC handler, removing compile-time type safety. The senders (start-single.mjs,start-single.cjs,mock-egg/index.ts) consistently use{ action: 'egg-ready', data: { address, port } }. A shared type would catch sender/receiver contract drift at compile time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/scripts/src/commands/start.ts` around lines 320 - 325, Introduce a concrete IPC message type for the egg-ready payload (e.g., interface EggReadyMessage { action: 'egg-ready'; data: { address: string; port: number } }) and use it instead of any in the child.on('message', ...) handler so TypeScript enforces the sender/receiver contract; add the type to a shared module/exports used by the senders (start-single.mjs/start-single.cjs/mock-egg/index.ts) and update the handler signature from (msg: any) to (msg: EggReadyMessage | unknown) with a type guard that narrows to EggReadyMessage before accessing msg.action and msg.data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/scripts/scripts/start-single.mjs`:
- Around line 13-16: The ESM launcher currently only checks framework.start ??
framework.startEgg and can fail for CJS-wrapped imports; update the logic that
assigns startEgg (and similarly start if used) to also fall back to
framework.default?.start and framework.default?.startEgg when
framework.start/startEgg are missing, so resolve startEgg from framework.start
?? framework.startEgg ?? framework.default?.start ?? framework.default?.startEgg
and then validate typeof startEgg === 'function' before throwing the error.
In `@tools/scripts/src/commands/start.ts`:
- Around line 351-366: The current block returns early when
this.flags['ignore-stderr'] is true, which turns a failed readiness into a
successful exit; instead, do not return success—either throw an Error or call
the command failure helper so the process exits non-zero. Locate the block that
checks hasStderrContent and stderrFile, which uses execFile and logToStderr, and
replace the final "return" under this.flags['ignore-stderr'] with logic that
logs a warning but then fails the command (e.g. throw new Error('startup failed:
non-empty stderr') or call the command's error/exit helper) so startup failures
remain failures even when --ignore-stderr is set.
In `@tools/scripts/src/commands/stop.ts`:
- Around line 50-54: The current filtering logic for matched requires the
JSON-formatted title even when the process command is a raw egg-server-* string;
update the matched expression so when flags.title is set it accepts either the
formatted title or raw egg-server variants. Concretely, in the matched
assignment (using isEggProcess, cmd, flags.title, format and
osRelated.titleTemplate) change the ternary branch to check isEggProcess &&
(cmd.includes(format(osRelated.titleTemplate, flags.title)) ||
cmd.startsWith(`egg-server-${flags.title}`) ||
cmd.includes(`egg-server-${flags.title}`) ), so raw egg-server-* process-title
commands with the provided title are also matched.
In `@tools/scripts/test/utils.ts`:
- Line 39: The process.kill call uses an invalid empty string signal which
throws; update the call that references process.kill(pid, type === 'master' ||
type === 'single' ? '' : 'SIGKILL') to use an explicit graceful signal (e.g.
'SIGTERM') for the master/single branch so it becomes process.kill(pid, type ===
'master' || type === 'single' ? 'SIGTERM' : 'SIGKILL'); keep the existing
try/catch behavior around this call so errors are still handled but no longer
triggered by an invalid signal, and ensure the variables pid and type referenced
in this change are the same ones used in the surrounding function.
---
Nitpick comments:
In `@tools/scripts/src/commands/start.ts`:
- Around line 185-189: Add integration regression tests that exercise the new
single-process path triggered by isSingle (where
execArgv.push('--import=tsx/esm') is applied) and the egg-ready IPC handshake:
create two end-to-end test cases that launch the CLI command used in start.ts
with --single (and also run the same tests in daemon mode) — one booting a
CommonJS example app and one booting a TypeScript example app — and assert that
the app performs basic HTTP behavior and completes the egg-ready handshake
(process lifecycle/IPC) successfully; place these tests alongside existing
CLI/integration tests and reuse the same start/stop helpers so they run in CI
and catch regressions affecting process orchestration or HTTP endpoints.
- Around line 320-325: Introduce a concrete IPC message type for the egg-ready
payload (e.g., interface EggReadyMessage { action: 'egg-ready'; data: { address:
string; port: number } }) and use it instead of any in the child.on('message',
...) handler so TypeScript enforces the sender/receiver contract; add the type
to a shared module/exports used by the senders
(start-single.mjs/start-single.cjs/mock-egg/index.ts) and update the handler
signature from (msg: any) to (msg: EggReadyMessage | unknown) with a type guard
that narrows to EggReadyMessage before accessing msg.action and msg.data.
In `@tools/scripts/tsdown.config.ts`:
- Around line 3-12: The exported default should be a typed intermediate config
object instead of an inline literal; create a const (e.g., const config:
UserConfig) with the same properties (entry, unbundle, external, unused) and
then export default defineConfig(config). Update imports/types if needed so
UserConfig (or the correct tsdown config type) is referenced, and keep
defineConfig(config) to preserve existing behavior; apply the same pattern to
related files like vitest.config.ts.
🪄 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: a28d1fbe-3a5b-440b-b231-980f5caf510f
📒 Files selected for processing (10)
tools/scripts/package.jsontools/scripts/scripts/start-single.cjstools/scripts/scripts/start-single.mjstools/scripts/src/commands/start.tstools/scripts/src/commands/stop.tstools/scripts/src/helper.tstools/scripts/src/resolve-framework-entry.tstools/scripts/test/resolve-framework-entry.test.tstools/scripts/test/utils.tstools/scripts/tsdown.config.ts
| if (hasStderrContent) { | ||
| try { | ||
| const args = ['-n', '100', stderrFile]; | ||
| this.logToStderr('tail %s', args.join(' ')); | ||
| const { stdout: headStdout } = await execFile('head', args); | ||
| const { stdout: tailStdout } = await execFile('tail', args); | ||
| this.logToStderr('Got error when startup: '); | ||
| this.logToStderr(headStdout); | ||
| this.logToStderr('...'); | ||
| this.logToStderr(tailStdout); | ||
| } catch (tailErr) { | ||
| this.logToStderr('ignore tail error: %s', tailErr); | ||
| } | ||
| if (this.flags['ignore-stderr']) { | ||
| return; // User opted to ignore stderr errors | ||
| } |
There was a problem hiding this comment.
Don't let --ignore-stderr turn a startup failure into success.
By the time execution reaches this block, readiness has already failed. Returning on Line 365 makes start --daemon exit successfully whenever the stderr file is non-empty, even though no egg-ready message was received.
Suggested adjustment
- if (this.flags['ignore-stderr']) {
- return; // User opted to ignore stderr errors
- }
- this.logToStderr('Start got error, see %o', stderrFile);
- this.logToStderr('Or use `--ignore-stderr` to ignore stderr at startup.');
+ if (!this.flags['ignore-stderr']) {
+ this.logToStderr('Start got error, see %o', stderrFile);
+ this.logToStderr('Or use `--ignore-stderr` to suppress stderr output at startup.');
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/scripts/src/commands/start.ts` around lines 351 - 366, The current
block returns early when this.flags['ignore-stderr'] is true, which turns a
failed readiness into a successful exit; instead, do not return success—either
throw an Error or call the command failure helper so the process exits non-zero.
Locate the block that checks hasStderrContent and stderrFile, which uses
execFile and logToStderr, and replace the final "return" under
this.flags['ignore-stderr'] with logic that logs a warning but then fails the
command (e.g. throw new Error('startup failed: non-empty stderr') or call the
command's error/exit helper) so startup failures remain failures even when
--ignore-stderr is set.
|
|
||
| try { | ||
| process.kill(pid, type === 'master' ? '' : 'SIGKILL'); | ||
| process.kill(pid, type === 'master' || type === 'single' ? '' : 'SIGKILL'); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js, is an empty string ('') a valid signal for process.kill(pid, signal)? How does it behave compared with omitting signal or using 'SIGTERM'?
💡 Result:
No, an empty string ('') is not a valid signal for process.kill(pid, signal) in Node.js. It will throw a TypeError [ERR_UNKNOWN_SIGNAL]: Unknown signal: '' because Node.js validates the signal parameter as a string or number and checks it against known signal names (like 'SIGTERM', 'SIGINT') or the special case of 0. An empty string does not match any valid signal name. Omitting the signal (process.kill(pid)) or explicitly using 'SIGTERM' both send the default signal SIGTERM, which requests a graceful termination of the target process (though the process may handle it differently). They behave identically. Using '' differs by throwing an error before any signal is sent, preventing the operation entirely.
Citations:
- 1: https://nodejs.org/api/process.html
- 2: https://nodejs.org/docs/latest/api/process.html
- 3: process.kill(pid, signal) does not recognize common signal strings nodejs/node#29642
- 4: https://github.com/nodejs/node/blob/v15.4.0/src/node_process_methods.cc
🏁 Script executed:
cat -n tools/scripts/test/utils.ts | sed -n '30,50p'Repository: eggjs/egg
Length of output: 811
Use an explicit signal in process.kill() on Line 39.
An empty string '' is not a valid signal in Node.js and will throw TypeError [ERR_UNKNOWN_SIGNAL]. When type === 'master' || type === 'single', the process kill always fails with an error. Although the error is caught by the try-catch block, this masks the actual failure and prevents proper cleanup. Use 'SIGTERM' (the default graceful termination signal) instead.
Suggested fix
- process.kill(pid, type === 'master' || type === 'single' ? '' : 'SIGKILL');
+ process.kill(pid, type === 'master' || type === 'single' ? 'SIGTERM' : 'SIGKILL');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| process.kill(pid, type === 'master' || type === 'single' ? '' : 'SIGKILL'); | |
| process.kill(pid, type === 'master' || type === 'single' ? 'SIGTERM' : 'SIGKILL'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/scripts/test/utils.ts` at line 39, The process.kill call uses an
invalid empty string signal which throws; update the call that references
process.kill(pid, type === 'master' || type === 'single' ? '' : 'SIGKILL') to
use an explicit graceful signal (e.g. 'SIGTERM') for the master/single branch so
it becomes process.kill(pid, type === 'master' || type === 'single' ? 'SIGTERM'
: 'SIGKILL'); keep the existing try/catch behavior around this call so errors
are still handled but no longer triggered by an invalid signal, and ensure the
variables pid and type referenced in this change are the same ones used in the
surrounding function.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tools/scripts/src/commands/start.ts (1)
351-366:⚠️ Potential issue | 🟠 MajorDon’t let
--ignore-stderrturn a failed start into success.By the time Line 365 runs, readiness has already failed. Returning there makes
start --daemon --ignore-stderrexit successfully even though noegg-readymessage arrived.Suggested adjustment
- if (this.flags['ignore-stderr']) { - return; // User opted to ignore stderr errors - } - this.logToStderr('Start got error, see %o', stderrFile); - this.logToStderr('Or use `--ignore-stderr` to ignore stderr at startup.'); + if (!this.flags['ignore-stderr']) { + this.logToStderr('Start got error, see %o', stderrFile); + this.logToStderr('Or use `--ignore-stderr` to ignore stderr at startup.'); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/scripts/src/commands/start.ts` around lines 351 - 366, The current early return when this.flags['ignore-stderr'] is true causes a failed readiness to be treated as success; after handling hasStderrContent, do not simply "return" when ignore-stderr is set — instead propagate the failure (throw a specific Error or call process.exit(1)) so the CLI exits non‑zero when startup/readiness did not complete (i.e. replace the return in the hasStderrContent block with throwing an error or exiting non‑zero), ensuring the failure condition detected after logging (hasStderrContent handling) cannot be masked by this.flags['ignore-stderr'].
🧹 Nitpick comments (1)
tools/scripts/src/commands/start.ts (1)
320-325: Avoidanyfor the IPC payload.Please switch this to
unknownand narrow the message shape before readingaction/data. As per coding guidelines "Avoid 'any' type in TypeScript; use 'unknown' when type is truly unknown".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/scripts/src/commands/start.ts` around lines 320 - 325, Change the child.on('message', (msg: any) => { ... }) handler to accept msg: unknown and perform a type-narrowing guard before accessing properties: check msg is a non-null object, that it has a string 'action' property and an object 'data' with an 'address' string, then only inside that guarded branch perform clearTimeout(timer), set this.isReady = true and call this.log('%s started on %s', readyLabel, msg.data.address); you can create a small type predicate or inline checks and then assert msg as the narrowed interface (e.g., { action: string; data: { address: string } }) to avoid using any while keeping the same logic in the child.on callback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/scripts/scripts/start-single.cjs`:
- Around line 12-18: The ESM launcher is missing the same fallback that the CJS
launcher uses: when resolving the start function it should check both top-level
named exports and the default export. In the ESM entry (where you currently read
framework/startEgg from the imported module), change the resolution to first try
framework.start ?? framework.startEgg and if that isn't a function then try
framework.default?.start ?? framework.default?.startEgg (ensuring the variable
name startEgg is reused or reassigned only after type-checking). This mirrors
the CJS logic (exports, startEgg) and keeps getServerBin() behavior consistent
across ESM/CJS.
In `@tools/scripts/src/commands/start.ts`:
- Around line 315-337: waitForReady currently only listens for 'message' and
non-zero exit codes, so spawn errors and clean exits before receiving
'egg-ready' hang until timeout and msg is typed as any; add a child.on('error',
(err) => { clearTimeout(timer); cleanup listeners; reject(err); }) and change
the child.on('exit', (code) => { if (!this.isReady) { clearTimeout(timer);
cleanup listeners; reject(new Error(`Child process exited with code ${code}`));
} }) to reject on any pre-ready termination, ensure you clear the timer and
remove/unbind listeners in a shared cleanup helper, and tighten the message
typing by replacing msg: any with a proper type or unknown and using a
type-guard to check msg.action === 'egg-ready' before accessing msg.data.
---
Duplicate comments:
In `@tools/scripts/src/commands/start.ts`:
- Around line 351-366: The current early return when this.flags['ignore-stderr']
is true causes a failed readiness to be treated as success; after handling
hasStderrContent, do not simply "return" when ignore-stderr is set — instead
propagate the failure (throw a specific Error or call process.exit(1)) so the
CLI exits non‑zero when startup/readiness did not complete (i.e. replace the
return in the hasStderrContent block with throwing an error or exiting
non‑zero), ensuring the failure condition detected after logging
(hasStderrContent handling) cannot be masked by this.flags['ignore-stderr'].
---
Nitpick comments:
In `@tools/scripts/src/commands/start.ts`:
- Around line 320-325: Change the child.on('message', (msg: any) => { ... })
handler to accept msg: unknown and perform a type-narrowing guard before
accessing properties: check msg is a non-null object, that it has a string
'action' property and an object 'data' with an 'address' string, then only
inside that guarded branch perform clearTimeout(timer), set this.isReady = true
and call this.log('%s started on %s', readyLabel, msg.data.address); you can
create a small type predicate or inline checks and then assert msg as the
narrowed interface (e.g., { action: string; data: { address: string } }) to
avoid using any while keeping the same logic in the child.on callback.
🪄 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: f0f4be1a-6d59-4513-b812-b43e96f4ef6e
📒 Files selected for processing (13)
tools/scripts/package.jsontools/scripts/scripts/start-single.cjstools/scripts/scripts/start-single.mjstools/scripts/src/commands/start.tstools/scripts/src/commands/stop.tstools/scripts/src/esm-polyfill.tstools/scripts/src/file-url-resolver.tstools/scripts/src/helper.tstools/scripts/src/resolve-framework-entry.tstools/scripts/test/esm-polyfill.test.tstools/scripts/test/resolve-framework-entry.test.tstools/scripts/test/utils.tstools/scripts/tsdown.config.ts
✅ Files skipped from review due to trivial changes (4)
- tools/scripts/src/helper.ts
- tools/scripts/src/file-url-resolver.ts
- tools/scripts/test/resolve-framework-entry.test.ts
- tools/scripts/scripts/start-single.mjs
🚧 Files skipped from review as they are similar to previous changes (5)
- tools/scripts/test/utils.ts
- tools/scripts/src/commands/stop.ts
- tools/scripts/tsdown.config.ts
- tools/scripts/package.json
- tools/scripts/src/resolve-framework-entry.ts
| const exports = await importModule(options.framework); | ||
| let startEgg = exports.start ?? exports.startEgg; | ||
| if (typeof startEgg !== 'function') { | ||
| startEgg = exports.default?.start ?? exports.default?.startEgg; | ||
| } | ||
| if (typeof startEgg !== 'function') { | ||
| throw new Error(`Cannot find start/startEgg function from framework: ${options.framework}`); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "start-single\.(cjs|mjs)" tools/Repository: eggjs/egg
Length of output: 130
🏁 Script executed:
cat -n tools/scripts/scripts/start-single.cjsRepository: eggjs/egg
Length of output: 2872
🏁 Script executed:
cat -n tools/scripts/scripts/start-single.mjsRepository: eggjs/egg
Length of output: 2732
🏁 Script executed:
fd -t f "start\.ts" tools/scripts/src/commands/ | head -5Repository: eggjs/egg
Length of output: 88
🏁 Script executed:
cat -n tools/scripts/src/commands/start.ts | head -100Repository: eggjs/egg
Length of output: 3912
🏁 Script executed:
cat -n tools/scripts/src/commands/start.ts | sed -n '84,200p'Repository: eggjs/egg
Length of output: 4863
🏁 Script executed:
rg -A 20 "single" tools/scripts/src/commands/start.ts | head -80Repository: eggjs/egg
Length of output: 2704
Align framework-entry fallback behavior between CJS and ESM launchers.
The .cjs launcher checks exports.default?.start and exports.default?.startEgg as a fallback (lines 15), but the .mjs launcher skips this step entirely and throws if the named export is not found. Since getServerBin() selects the launcher based on this.isESM at runtime, single-process startup will behave differently depending on which binary is used. Update start-single.mjs to mirror the fallback logic:
const startEgg = framework.start ?? framework.startEgg;
if (typeof startEgg !== 'function') {
startEgg = framework.default?.start ?? framework.default?.startEgg;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/scripts/scripts/start-single.cjs` around lines 12 - 18, The ESM
launcher is missing the same fallback that the CJS launcher uses: when resolving
the start function it should check both top-level named exports and the default
export. In the ESM entry (where you currently read framework/startEgg from the
imported module), change the resolution to first try framework.start ??
framework.startEgg and if that isn't a function then try
framework.default?.start ?? framework.default?.startEgg (ensuring the variable
name startEgg is reused or reassigned only after type-checking). This mirrors
the CJS logic (exports, startEgg) and keeps getServerBin() behavior consistent
across ESM/CJS.
| await new Promise<void>((resolve, reject) => { | ||
| const timer = setTimeout(() => { | ||
| reject(new Error(`Start failed, ${timeoutMs / 1000}s timeout`)); | ||
| }, timeoutMs); | ||
|
|
||
| child.on('message', (msg: any) => { | ||
| // https://github.com/eggjs/cluster/blob/master/src/master.ts#L119 | ||
| if (msg && msg.action === 'egg-ready') { | ||
| clearTimeout(timer); | ||
| this.isReady = true; | ||
| this.log('%s started on %s', readyLabel, msg.data.address); | ||
| child.unref(); | ||
| child.disconnect(); | ||
| resolve(); | ||
| } | ||
| }); | ||
|
|
||
| child.on('exit', (code) => { | ||
| if (code) { | ||
| clearTimeout(timer); | ||
| reject(new Error(`Child process exited with code ${code}`)); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the actual code in the file
cat -n tools/scripts/src/commands/start.ts | sed -n '310,340p'Repository: eggjs/egg
Length of output: 1328
🏁 Script executed:
# Also check the broader context around waitForReady function
cat -n tools/scripts/src/commands/start.ts | sed -n '295,380p'Repository: eggjs/egg
Length of output: 3408
🏁 Script executed:
# Check the TypeScript configuration to verify strict mode settings
find . -name "tsconfig.json" | head -5Repository: eggjs/egg
Length of output: 272
🏁 Script executed:
# Find the tsconfig.json for the tools/scripts package
find . -name "tsconfig.json" -path "*/tools/*" | xargs catRepository: eggjs/egg
Length of output: 5159
🏁 Script executed:
# Let's also check the Node.js documentation behavior for ChildProcess events
# Check if there are any other similar error handling patterns in the codebase
rg -A 5 -B 5 "child\.on\('error'" --type tsRepository: eggjs/egg
Length of output: 35
🏁 Script executed:
# Check if there are patterns using child.once for exit handlers
rg -A 5 -B 5 "child\.once\('exit'" --type tsRepository: eggjs/egg
Length of output: 874
🏁 Script executed:
# Verify the message type by checking how msg is used elsewhere
rg "msg\.action === 'egg-ready'" -A 3 -B 3 --type tsRepository: eggjs/egg
Length of output: 1779
🏁 Script executed:
# Check the tools/scripts tsconfig more carefully
cat -n tools/scripts/tsconfig.jsonRepository: eggjs/egg
Length of output: 394
🏁 Script executed:
# Check the root tsconfig that tools/scripts extends
cat -n tsconfig.json | head -30Repository: eggjs/egg
Length of output: 949
🏁 Script executed:
# Verify Node.js ChildProcess spawn behavior - check if there are any spawn error handlers elsewhere
rg "spawn\(" -A 10 tools/scripts/src --type ts | head -40Repository: eggjs/egg
Length of output: 1347
🏁 Script executed:
# Check how messages are typed in the codebase - look for interface definitions
rg "msg\.data\.address" -B 10 -A 2 --type ts | head -50Repository: eggjs/egg
Length of output: 3019
🏁 Script executed:
# Let's check the `@eggjs/tsconfig` to understand what "strict" mode includes
npm info `@eggjs/tsconfig` 2>/dev/null || cat -n packages/*/tsconfig.json | grep -A 5 "strict"Repository: eggjs/egg
Length of output: 914
🏁 Script executed:
# Look at how the process spawn is called to understand stdio setup
cat -n tools/scripts/src/commands/start.ts | sed -n '275,295p'Repository: eggjs/egg
Length of output: 1229
🏁 Script executed:
# Check if child.unref() and child.disconnect() are called correctly
cat -n tools/scripts/src/commands/start.ts | sed -n '320,330p'Repository: eggjs/egg
Length of output: 545
🏁 Script executed:
# Verify the handling of exit code 0 - check the non-waitForReady exit handler
cat -n tools/scripts/src/commands/start.ts | sed -n '288,298p'Repository: eggjs/egg
Length of output: 555
🏁 Script executed:
# Check if there's any existing error event handling for spawn in the codebase
rg "\.on\('error'" -B 3 -A 3 tools/scripts/src --type tsRepository: eggjs/egg
Length of output: 35
🏁 Script executed:
# Check Node.js child_process documentation behavior - verify if error events are possible during spawn
rg "spawn.*error" --type ts | head -20Repository: eggjs/egg
Length of output: 392
🏁 Script executed:
# Look at the structure to understand spawn behavior and when errors can occur
cat -n tools/scripts/src/commands/start.ts | sed -n '1,30p'Repository: eggjs/egg
Length of output: 1306
🏁 Script executed:
# Check if there are any TypeScript type definitions for the spawn return to understand expected behavior
rg "ChildProcess" tools/scripts/src/commands/start.ts -B 2 -A 2Repository: eggjs/egg
Length of output: 560
🏁 Script executed:
# Look at the test case that mentions spawn error
cat -n tools/scripts/test/start-without-demon-2.test.ts | grep -A 15 "should emit spawn error"Repository: eggjs/egg
Length of output: 576
🏁 Script executed:
# Check the test for spawn error handling
cat -n tools/scripts/test/start-without-demon-2.test.tsRepository: eggjs/egg
Length of output: 5165
🏁 Script executed:
# Also check the master should not die if spawn error test
cat -n packages/cluster/test/agent_worker.test.ts | grep -B 5 -A 15 "master should not die if spawn error"Repository: eggjs/egg
Length of output: 916
🏁 Script executed:
# Let's understand the actual node.js ChildProcess behavior for spawn errors
# Check if there's documentation in the code or comments about spawn errors
rg "spawn error|spawn.*error" -B 5 -A 5 tools/scripts --type tsRepository: eggjs/egg
Length of output: 912
🏁 Script executed:
# Let's check what message structure should be used instead of any
rg "interface.*Message|type.*Message" -A 5 --type ts packages/cluster/src | head -40Repository: eggjs/egg
Length of output: 2932
🏁 Script executed:
# Check if there's a defined message type in egg framework
rg "egg-ready.*data" -B 5 -A 5 --type ts | head -50Repository: eggjs/egg
Length of output: 35
🏁 Script executed:
# Verify if MessageBody is exported from a location accessible to tools/scripts
rg "export.*MessageBody" --type ts | grep -E "(packages/cluster|packages/core)"Repository: eggjs/egg
Length of output: 123
🏁 Script executed:
# Check the imports in start.ts to see what's already available
head -20 tools/scripts/src/commands/start.tsRepository: eggjs/egg
Length of output: 783
🏁 Script executed:
# Check if MessageBody is exported from packages/cluster package.json exports
cat packages/cluster/package.json | grep -A 20 "exports"Repository: eggjs/egg
Length of output: 845
🏁 Script executed:
# Alternative: check what's available to import from packages/cluster
rg "export.*from.*messenger" packages/cluster/src --type tsRepository: eggjs/egg
Length of output: 35
🏁 Script executed:
# Check what's exported from packages/cluster/src/index.ts
cat packages/cluster/src/index.ts | grep -E "export|import.*MessageBody"Repository: eggjs/egg
Length of output: 278
🏁 Script executed:
# Check if tools/scripts depends on `@egg/cluster` or if it can only import from public exports
cat tools/scripts/package.json | grep -E "dependencies|devDependencies" -A 20Repository: eggjs/egg
Length of output: 776
🏁 Script executed:
# Check if packages/cluster is in the workspace and could be imported
cat packages/cluster/src/utils/messenger.ts | head -40Repository: eggjs/egg
Length of output: 1176
🏁 Script executed:
# Check if there's a way to access MessageBody through the egg package
cat packages/egg/src/index.ts | grep -i "message" | head -20Repository: eggjs/egg
Length of output: 35
🏁 Script executed:
# Confirm the actual tsconfig for tools/scripts and what strict mode is enabled
cat tools/scripts/tsconfig.jsonRepository: eggjs/egg
Length of output: 331
🏁 Script executed:
# Check if there's a way to import from packages/cluster in tools/scripts context
rg "from '@egg" tools/scripts/src --type ts | head -10Repository: eggjs/egg
Length of output: 152
🏁 Script executed:
# Let's verify what the actual strict mode settings are for tools/scripts
cat tsconfig.json | grep -E "strict|noImplicitAny"Repository: eggjs/egg
Length of output: 35
🏁 Script executed:
# Check the full root tsconfig to understand strict mode inheritance
cat tsconfig.json | head -20Repository: eggjs/egg
Length of output: 558
🏁 Script executed:
# Check what `@eggjs/tsconfig` includes
npm info `@eggjs/tsconfig` 2>&1 | grep -A 50 "README"Repository: eggjs/egg
Length of output: 35
🏁 Script executed:
# Let's see if we can find the actual `@eggjs/tsconfig` definition or its source
find . -name "@eggjs" -o -name "eggjs" -type d 2>/dev/null | head -5Repository: eggjs/egg
Length of output: 119
🏁 Script executed:
# Check the packages/tsconfig directory
ls -la packages/tsconfig/Repository: eggjs/egg
Length of output: 657
🏁 Script executed:
# See what's in the tsconfig package
cat packages/tsconfig/src/index.json 2>/dev/null || cat packages/tsconfig/index.json 2>/dev/null || find packages/tsconfig -name "*.json" -type fRepository: eggjs/egg
Length of output: 174
🏁 Script executed:
# Let's check if strict mode is enabled in the `@eggjs/tsconfig` by looking at the actual source
find packages -name "tsconfig" -type d | head -5Repository: eggjs/egg
Length of output: 70
🏁 Script executed:
# Check the tsconfig.json in packages/tsconfig - this is the `@eggjs/tsconfig`
cat packages/tsconfig/tsconfig.jsonRepository: eggjs/egg
Length of output: 1827
🏁 Script executed:
# Verify that MessageBody is indeed not part of the public exports
grep -r "export.*MessageBody" packages/cluster/src/index.tsRepository: eggjs/egg
Length of output: 35
Handle spawn errors and reject on any pre-ready termination.
waitForReady() lacks an error event listener and only rejects on non-zero exit codes. Spawn failures (e.g., command not found) and clean exits before egg-ready both hang until timeout. Additionally, msg is typed as any on line 320, violating strict TypeScript mode.
Suggested adjustment
await new Promise<void>((resolve, reject) => {
const timer = setTimeout(() => {
reject(new Error(`Start failed, ${timeoutMs / 1000}s timeout`));
}, timeoutMs);
child.on('message', (msg: any) => {
// https://github.com/eggjs/cluster/blob/master/src/master.ts#L119
if (msg && msg.action === 'egg-ready') {
clearTimeout(timer);
this.isReady = true;
this.log('%s started on %s', readyLabel, msg.data.address);
child.unref();
child.disconnect();
resolve();
}
});
- child.on('exit', (code) => {
- if (code) {
- clearTimeout(timer);
- reject(new Error(`Child process exited with code ${code}`));
- }
- });
+ child.once('error', err => {
+ clearTimeout(timer);
+ reject(err);
+ });
+ child.once('exit', (code, signal) => {
+ clearTimeout(timer);
+ reject(
+ new Error(
+ code === null
+ ? `Child process exited before ready via ${signal}`
+ : `Child process exited before ready with code ${code}`,
+ ),
+ );
+ });
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/scripts/src/commands/start.ts` around lines 315 - 337, waitForReady
currently only listens for 'message' and non-zero exit codes, so spawn errors
and clean exits before receiving 'egg-ready' hang until timeout and msg is typed
as any; add a child.on('error', (err) => { clearTimeout(timer); cleanup
listeners; reject(err); }) and change the child.on('exit', (code) => { if
(!this.isReady) { clearTimeout(timer); cleanup listeners; reject(new
Error(`Child process exited with code ${code}`)); } }) to reject on any
pre-ready termination, ensure you clear the timer and remove/unbind listeners in
a shared cleanup helper, and tighten the message typing by replacing msg: any
with a proper type or unknown and using a type-guard to check msg.action ===
'egg-ready' before accessing msg.data.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 959e604ac8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (msg && msg.action === 'egg-ready') { | ||
| clearTimeout(timer); | ||
| this.isReady = true; | ||
| this.log('%s started on %s', readyLabel, msg.data.address); | ||
| child.unref(); | ||
| child.disconnect(); | ||
| resolve(); |
There was a problem hiding this comment.
waitForReady() now resolves as soon as it receives egg-ready, but that success path never inspects master-stderr.log. As a result, boot-time errors that still allow readiness are silently accepted (exit code 0) even when --ignore-stderr is not set, which regresses the prior startup contract and makes --ignore-stderr ineffective in this common case.
Useful? React with 👍 / 👎.
| const startEgg = framework.start ?? framework.startEgg; | ||
| if (typeof startEgg !== 'function') { | ||
| throw new Error(`Cannot find start/startEgg function from framework: ${options.framework}`); |
There was a problem hiding this comment.
The ESM single-process launcher only checks framework.start/framework.startEgg, but unlike the CJS launcher it does not fall back to framework.default. Frameworks that expose their start API only via default export (common in CJS/interop builds) will incorrectly throw Cannot find start/startEgg in ESM apps.
Useful? React with 👍 / 👎.
| .replace(/\bimport\.meta\.dirname\b/g, JSON.stringify(dirname)) | ||
| .replace(/\bimport\.meta\.url\b/g, JSON.stringify(url)) | ||
| .replace(/\bimport\.meta\.filename\b/g, JSON.stringify(absPath)) | ||
| .replace(/\b__dirname\b/g, JSON.stringify(dirname)); |
There was a problem hiding this comment.
The polyfill uses global regex replacements on raw source text, so tokens are rewritten even inside string literals (and comments), not just executable identifiers. For example, a literal like "__dirname" is mutated into an absolute path string, which can corrupt behavior or emitted bundles in modules that mention these tokens textually.
Useful? React with 👍 / 👎.
Summary
Salvage the independent-value infrastructure from the abandoned `feat/snapshot-scripts` branch:
Context
The `feat/snapshot-scripts` branch pursued a `node --build-snapshot` based approach that was abandoned after multiple POCs revealed architectural dead-ends:
The pieces in this PR have value independent of any snapshot/bundle strategy:
Dropped (not in this PR)
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--singleflag to start apps in single-process mode.Bug Fixes
Chores