feat(cluster): log cluster module IPC via onelogger#5872
feat(cluster): log cluster module IPC via onelogger#5872
Conversation
Add structured logging on every master<->app worker IPC point in Node.js cluster mode (process mode only). Covers both application-layer messages and the internal NODE_CLUSTER fd dispatch / fd ack. Application layer (always on): - master->app send (AppProcessWorker#send) - master->app sticky-session Socket direct send (master.ts) - master<-app recv (cluster.on 'fork' -> worker.on 'message') - app->master send (static AppProcessWorker.send, covers both regular process.send and the reusePort cluster.worker.send path) - app<-master recv (process.on 'message' in app_worker.ts) Internal cluster layer (opt-in via EGG_CLUSTER_IPC_LOG=1, very verbose): - master<-app internal (worker.process.on 'internalMessage'): online / listening / queryServer / accepted (fd ack) / close - app<-master internal (process.on 'internalMessage'): newconn (fd dispatch, with handle) / disconnect / suicide Users can replace the default console sink via onelogger.setLogger(). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe changes add comprehensive inter-process communication (IPC) logging to the cluster module. A new logging utility is introduced that provides formatted message logging with circular reference handling and safe serialization, integrated across worker and master process handlers to track message exchanges between processes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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 with
|
| Latest commit: |
43dce09
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9cfbc737.egg-cci.pages.dev |
| Branch Preview URL: | https://claude-ipc-log-cluster.egg-cci.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5872 +/- ##
==========================================
- Coverage 86.00% 85.96% -0.05%
==========================================
Files 667 9 -658
Lines 18945 57 -18888
Branches 3652 11 -3641
==========================================
- Hits 16294 49 -16245
+ Misses 2297 7 -2290
+ Partials 354 1 -353 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Deploying egg-v3 with
|
| Latest commit: |
43dce09
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b10822e0.egg-v3.pages.dev |
| Branch Preview URL: | https://claude-ipc-log-cluster.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 IPC traffic logging between the master and app workers using the onelogger library and a new utility, ipc_logger.ts. The implementation covers standard IPC messages, internal Node.js cluster events, and sticky-session connections. Review feedback primarily addresses performance risks associated with unconditional logging and JSON serialization in high-throughput scenarios. It is recommended to make all IPC logging opt-in via environment variables to prevent latency and event loop blocking. Additionally, the review suggests avoiding brittle internal handle access and optimizing the serialization process to prevent unnecessary memory and CPU overhead.
| * These are very verbose (one `cluster:newconn` per HTTP request), so they are opt-in | ||
| * via the `EGG_CLUSTER_IPC_LOG` environment variable (any truthy value enables). | ||
| */ | ||
| export const internalIpcLogEnabled = !!process.env.EGG_CLUSTER_IPC_LOG; |
There was a problem hiding this comment.
The 'Application layer' IPC logging (Points A, B, C, D) is currently always enabled. Since formatIpcMessage performs a JSON.stringify on every message, this introduces significant performance overhead and event loop blocking in high-throughput scenarios. It is highly recommended to make all IPC logging opt-in via an environment variable (e.g., EGG_CLUSTER_IPC_LOG) to prevent unexpected performance degradation in production.
| process.on('message', (msg: any, handle: any) => { | ||
| const body = typeof msg === 'string' ? { action: msg } : msg; | ||
| ipcLogger.info(formatIpcMessage(`app#${process.pid}<-master`, body, handle)); | ||
| }); |
There was a problem hiding this comment.
This IPC listener is always active and performs logging on every message received from the master. To avoid performance issues in production, this should be guarded by an opt-in check similar to the internal messages below.
| process.on('message', (msg: any, handle: any) => { | |
| const body = typeof msg === 'string' ? { action: msg } : msg; | |
| ipcLogger.info(formatIpcMessage(`app#${process.pid}<-master`, body, handle)); | |
| }); | |
| if (internalIpcLogEnabled) { | |
| process.on('message', (msg: any, handle: any) => { | |
| const body = typeof msg === 'string' ? { action: msg } : msg; | |
| ipcLogger.info(formatIpcMessage('app#' + process.pid + '<-master', body, handle)); | |
| }); | |
| } |
| ipcLogger.info( | ||
| formatIpcMessage(`master->app#${worker.workerId}`, { action: 'sticky-session:connection' }, connection), | ||
| ); |
There was a problem hiding this comment.
Logging every sticky-session connection in the master process can be expensive. This should be guarded by an opt-in check.
if (internalIpcLogEnabled) {
ipcLogger.info(
formatIpcMessage('master->app#' + worker.workerId, { action: 'sticky-session:connection' }, connection),
);
}| } | ||
|
|
||
| send(message: MessageBody): void { | ||
| ipcLogger.info(formatIpcMessage(`master->app#${this.workerId}`, message)); |
There was a problem hiding this comment.
Logging every message sent from master to app workers using JSON.stringify in the hot path will introduce latency. This should be guarded by an opt-in check.
if (internalIpcLogEnabled) {
ipcLogger.info(formatIpcMessage('master->app#' + this.workerId, message));
}|
|
||
| static send(message: MessageBody): void { | ||
| message.senderWorkerId = String(process.pid); | ||
| ipcLogger.info(formatIpcMessage(`app#${process.pid}->master`, message)); |
There was a problem hiding this comment.
Logging every message sent from app to master in the hot path will introduce latency. This should be guarded by an opt-in check.
| ipcLogger.info(formatIpcMessage(`app#${process.pid}->master`, message)); | |
| if (internalIpcLogEnabled) { | |
| ipcLogger.info(formatIpcMessage('app#' + process.pid + '->master', message)); | |
| } |
| } | ||
| msg.from = 'app'; | ||
| this.messenger.send(msg); | ||
| ipcLogger.info(formatIpcMessage(`master<-app#${worker.process.pid}`, msg, handle)); |
There was a problem hiding this comment.
Logging every message received by master from app workers in the hot path will introduce latency. This should be guarded by an opt-in check.
if (internalIpcLogEnabled) {
ipcLogger.info(formatIpcMessage('master<-app#' + worker.process.pid, msg, handle));
}| function describeHandle(handle: unknown): string { | ||
| if (handle == null) return ''; | ||
| if (handle instanceof net.Socket) { | ||
| const fd = (handle as unknown as { _handle?: { fd?: number } })._handle?.fd; |
There was a problem hiding this comment.
Accessing the internal _handle property of a net.Socket is brittle as it is not part of the public Node.js API and its structure may change between versions. While useful for debugging, this should be handled with caution.
| if (out && out.length > MAX_TOTAL_LEN) { | ||
| out = `${out.slice(0, MAX_TOTAL_LEN)}...(truncated)`; | ||
| } |
There was a problem hiding this comment.
Truncating the stringified output after JSON.stringify has completed does not prevent the initial memory allocation and CPU usage required to serialize a potentially massive object. If data is very large, this could lead to performance degradation or even Out-of-Memory (OOM) issues before the truncation occurs. Consider implementing a more efficient way to limit serialization depth or size.
| import { readJSONSync } from 'utility'; | ||
|
|
||
| import { ClusterWorkerExceptionError } from './error/ClusterWorkerExceptionError.ts'; | ||
| import { ipcLogger, formatIpcMessage } from './utils/ipc_logger.ts'; |
There was a problem hiding this comment.
Import internalIpcLogEnabled to guard the sticky-session logging point.
| import { ipcLogger, formatIpcMessage } from './utils/ipc_logger.ts'; | |
| import { ipcLogger, formatIpcMessage, internalIpcLogEnabled } from './utils/ipc_logger.ts'; |
There was a problem hiding this comment.
Pull request overview
Adds structured IPC logging for Node.js cluster (process mode) traffic in @eggjs/cluster using onelogger, covering master↔app message send/receive plus optional verbose logging of internal NODE_CLUSTER messages.
Changes:
- Introduce
utils/ipc_logger.tswith a formatter/replacer to safely stringify IPC payloads and handles. - Hook IPC log points into process-mode master/app worker message paths (including sticky-session socket forwarding) and optional
internalMessageobservation. - Add
oneloggerdependency to@eggjs/cluster.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/cluster/src/utils/mode/impl/process/app.ts | Logs master↔app IPC send/recv; adds optional internal NODE_CLUSTER logging on the master side. |
| packages/cluster/src/utils/ipc_logger.ts | Adds onelogger instance plus IPC formatting/safe JSON stringify utilities. |
| packages/cluster/src/master.ts | Logs sticky-session direct worker.instance.send(..., socket) IPC path. |
| packages/cluster/src/app_worker.ts | Logs master→app IPC receive in process mode; adds optional internal NODE_CLUSTER logging on the app side. |
| packages/cluster/package.json | Adds onelogger dependency. |
| if (msg && msg.data !== undefined) { | ||
| out += ` data=${stringifyData(msg.data)}`; | ||
| } |
There was a problem hiding this comment.
formatIpcMessage() logs msg.data for all IPC messages at info level. Since MessageBody.data is arbitrary user/framework payload, this can unintentionally persist sensitive values into logs. Consider defaulting to action-only logging and gating data= behind an opt-in flag / higher log level (or redacting common secret keys) to reduce leakage risk.
| function stringifyData(data: unknown): string { | ||
| let out: string; | ||
| try { | ||
| out = JSON.stringify(data, makeReplacer()); | ||
| } catch (err) { |
There was a problem hiding this comment.
The new stringifyData()/replacer logic is central to keeping IPC logging safe (circular refs, Buffer handling, truncation). There are existing Vitest tests in this package; please add unit tests covering circular structures, long-string truncation, Buffer formatting, and total-length capping to prevent regressions.
| "egg-logger": "catalog:", | ||
| "get-ready": "catalog:", | ||
| "graceful-process": "catalog:", | ||
| "onelogger": "^1.0.1", |
There was a problem hiding this comment.
This repo uses pnpm catalog mode and already defines onelogger in the workspace catalog; this package should reference it via "catalog:" (consistent with the other deps here) instead of hard-coding ^1.0.1, to keep dependency versions centralized.
| "onelogger": "^1.0.1", | |
| "onelogger": "catalog:", |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/cluster/src/utils/ipc_logger.ts (1)
1-11: Rename this file to kebab-case.
ipc_logger.tsbreaks the repo's lowercase-with-hyphens source-file convention. Please rename it toipc-logger.tsand update the import sites in this PR to keep the package consistent.As per coding guidelines: "Name files in lowercase with hyphens (e.g.
loader-context.ts)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cluster/src/utils/ipc_logger.ts` around lines 1 - 11, The file name ipc_logger.ts violates the repo's lowercase-with-hyphens convention; rename the file to ipc-logger.ts and update all import sites that reference ipc_logger.ts to import from ipc-logger.ts instead (look for usages referencing ipcLogger, getLogger('egg/cluster/ipc'), and any imports from './ipc_logger' or similar in this PR), ensuring exports remain unchanged so ipcLogger continues to be imported correctly.packages/cluster/src/app_worker.ts (1)
56-58: Preferunknownoveranyin the new message hook.This listener already narrows
msgat runtime, soanyonly weakens strict-mode guarantees in a fresh code path.As per coding guidelines: "Avoid 'any' type in TypeScript; use 'unknown' when type is truly unknown".♻️ Proposed fix
- process.on('message', (msg: any, handle: any) => { - const body = typeof msg === 'string' ? { action: msg } : msg; + process.on('message', (msg: unknown, handle: unknown) => { + const body = + typeof msg === 'string' + ? { action: msg } + : (msg as { action?: string; data?: unknown }); ipcLogger.info(formatIpcMessage(`app#${process.pid}<-master`, body, handle)); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cluster/src/app_worker.ts` around lines 56 - 58, Change the message handler signature in the process.on('message') listener to use unknown instead of any for the msg parameter (i.e., process.on('message', (msg: unknown, handle: any) => ...)); keep the existing runtime narrowing (typeof msg === 'string' ? { action: msg } : msg) to safely refine msg before using it, and ensure the computed body passed to formatIpcMessage/ipcLogger.info is properly typed after narrowing.
🤖 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/cluster/src/master.ts`:
- Around line 295-299: The sticky-session connection log (ipcLogger.info call
that formats via formatIpcMessage for `master->app#${worker.workerId}` with
action 'sticky-session:connection') is on the hot accept path; guard it behind a
verbose flag or demote it to debug to avoid synchronous formatting on every
accept. Modify the code around the ipcLogger.info invocation in master.ts so you
only call formatIpcMessage and ipcLogger.info when a verbose/debug mode is
enabled (e.g., check an existing ipcLogger.isVerbose/isDebug flag or a config
option) or replace the call with ipcLogger.debug to ensure the expensive
stringification is skipped in normal runs.
In `@packages/cluster/src/utils/ipc_logger.ts`:
- Around line 53-63: The current stringifyData function unconditionally
JSON-serializes IPC payloads (using makeReplacer()) and returns truncated
output, which can leak secrets; change this to only log safe metadata or perform
explicit allowlist/redaction before serializing. Update stringifyData (and the
related serializer used around lines 72-80) to: 1) detect and return only
non-sensitive metadata fields (e.g., message type, size, sender id, timestamp)
instead of full payload by default, 2) implement an allowlist of keys that may
be serialized and replace all other keys with a placeholder like "<redacted>",
or 3) if full payload must be logged for a specific allowlisted message type,
apply a strict makeReplacer that masks values for common secret keys (password,
token, cookie, secret, apiKey) before JSON.stringify; ensure the function
returns no raw payload content unless explicitly allowlisted.
- Around line 13-18: internalIpcLogEnabled currently coerces any non-empty
EGG_CLUSTER_IPC_LOG string to true, enabling verbose IPC logs for values like
"0" or "no"; change its logic to explicitly parse allowed truthy values (e.g.,
'1' and 'true' case-insensitively) by reading process.env.EGG_CLUSTER_IPC_LOG
and checking against an allowlist such as new Set(['1','true']), normalizing
case and trimming, so only explicit intent enables the flag (update the exported
constant internalIpcLogEnabled accordingly).
---
Nitpick comments:
In `@packages/cluster/src/app_worker.ts`:
- Around line 56-58: Change the message handler signature in the
process.on('message') listener to use unknown instead of any for the msg
parameter (i.e., process.on('message', (msg: unknown, handle: any) => ...));
keep the existing runtime narrowing (typeof msg === 'string' ? { action: msg } :
msg) to safely refine msg before using it, and ensure the computed body passed
to formatIpcMessage/ipcLogger.info is properly typed after narrowing.
In `@packages/cluster/src/utils/ipc_logger.ts`:
- Around line 1-11: The file name ipc_logger.ts violates the repo's
lowercase-with-hyphens convention; rename the file to ipc-logger.ts and update
all import sites that reference ipc_logger.ts to import from ipc-logger.ts
instead (look for usages referencing ipcLogger, getLogger('egg/cluster/ipc'),
and any imports from './ipc_logger' or similar in this PR), ensuring exports
remain unchanged so ipcLogger continues to be imported correctly.
🪄 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: cf77c83f-ebf8-44d4-b9af-b13e8484de94
📒 Files selected for processing (5)
packages/cluster/package.jsonpackages/cluster/src/app_worker.tspackages/cluster/src/master.tspackages/cluster/src/utils/ipc_logger.tspackages/cluster/src/utils/mode/impl/process/app.ts
| // A'. master -> app sticky-session direct send (bypasses AppProcessWorker#send), | ||
| // carries a net.Socket handle — safe-printed by formatIpcMessage's replacer. | ||
| ipcLogger.info( | ||
| formatIpcMessage(`master->app#${worker.workerId}`, { action: 'sticky-session:connection' }, connection), | ||
| ); |
There was a problem hiding this comment.
Gate sticky-session connection logs behind the verbose flag.
This runs on the accept path before worker.instance.send() for every incoming socket. In sticky mode that adds synchronous formatting + logger work to the hottest path in the module and can materially reduce accept throughput under load. Please make this opt-in like the internal cluster logs, or drop it to a debug-only path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cluster/src/master.ts` around lines 295 - 299, The sticky-session
connection log (ipcLogger.info call that formats via formatIpcMessage for
`master->app#${worker.workerId}` with action 'sticky-session:connection') is on
the hot accept path; guard it behind a verbose flag or demote it to debug to
avoid synchronous formatting on every accept. Modify the code around the
ipcLogger.info invocation in master.ts so you only call formatIpcMessage and
ipcLogger.info when a verbose/debug mode is enabled (e.g., check an existing
ipcLogger.isVerbose/isDebug flag or a config option) or replace the call with
ipcLogger.debug to ensure the expensive stringification is skipped in normal
runs.
| /** | ||
| * Whether internal-level cluster IPC logs (NODE_CLUSTER newconn/accepted/listening/...) are enabled. | ||
| * These are very verbose (one `cluster:newconn` per HTTP request), so they are opt-in | ||
| * via the `EGG_CLUSTER_IPC_LOG` environment variable (any truthy value enables). | ||
| */ | ||
| export const internalIpcLogEnabled = !!process.env.EGG_CLUSTER_IPC_LOG; |
There was a problem hiding this comment.
Parse EGG_CLUSTER_IPC_LOG explicitly.
!!process.env.EGG_CLUSTER_IPC_LOG treats 0, false, and no as enabled because they are non-empty strings. That makes the verbose internal listener easy to turn on by accident. Please check for an explicit allowlist such as '1' / 'true'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cluster/src/utils/ipc_logger.ts` around lines 13 - 18,
internalIpcLogEnabled currently coerces any non-empty EGG_CLUSTER_IPC_LOG string
to true, enabling verbose IPC logs for values like "0" or "no"; change its logic
to explicitly parse allowed truthy values (e.g., '1' and 'true'
case-insensitively) by reading process.env.EGG_CLUSTER_IPC_LOG and checking
against an allowlist such as new Set(['1','true']), normalizing case and
trimming, so only explicit intent enables the flag (update the exported constant
internalIpcLogEnabled accordingly).
| function stringifyData(data: unknown): string { | ||
| let out: string; | ||
| try { | ||
| out = JSON.stringify(data, makeReplacer()); | ||
| } catch (err) { | ||
| return `<unserializable: ${(err as Error).message}>`; | ||
| } | ||
| if (out && out.length > MAX_TOTAL_LEN) { | ||
| out = `${out.slice(0, MAX_TOTAL_LEN)}...(truncated)`; | ||
| } | ||
| return out; |
There was a problem hiding this comment.
Do not serialize arbitrary IPC payloads into always-on logs.
This formatter emits msg.data for every normal app/master IPC log, and truncation does not prevent leaking tokens, cookies, or config secrets. Since the send/receive hooks are unconditional, any sensitive IPC payload becomes log output by default. Please switch this to metadata-only logging, or add redaction/allowlisting before serializing data.
As per coding guidelines: "Never expose sensitive configuration values in logs or error messages".
Also applies to: 72-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cluster/src/utils/ipc_logger.ts` around lines 53 - 63, The current
stringifyData function unconditionally JSON-serializes IPC payloads (using
makeReplacer()) and returns truncated output, which can leak secrets; change
this to only log safe metadata or perform explicit allowlist/redaction before
serializing. Update stringifyData (and the related serializer used around lines
72-80) to: 1) detect and return only non-sensitive metadata fields (e.g.,
message type, size, sender id, timestamp) instead of full payload by default, 2)
implement an allowlist of keys that may be serialized and replace all other keys
with a placeholder like "<redacted>", or 3) if full payload must be logged for a
specific allowlisted message type, apply a strict makeReplacer that masks values
for common secret keys (password, token, cookie, secret, apiKey) before
JSON.stringify; ensure the function returns no raw payload content unless
explicitly allowlisted.
Summary
Add structured logging for every master↔app worker IPC point under Node.js
clustermode (process mode only) in@eggjs/cluster, using onelogger as the logger facade.Covers both layers of cluster IPC:
Application layer (always on)
AppProcessWorker#send()master.tssticky-sessionworker.instance.send('sticky-session:connection', connection)cluster.on('fork')→worker.on('message', (msg, handle) => ...); logs after forwarding to avoid adding serialization latency to the forward pathstatic AppProcessWorker.send()— covers both the regularprocess.sendpath and the reusePortcluster.worker.sendpathprocess.on('message', (msg, handle) => ...)inapp_worker.ts, only for process mode (skipped under worker_threads)Cluster internal layer (opt-in via
EGG_CLUSTER_IPC_LOG=1; very verbose —newconnfires once per HTTP request)online/listening/queryServer/accepted(fd ack) /close— viaworker.process.on('internalMessage')newconn(fd dispatch, handle=<TCP>) /disconnect/suicide— viaprocess.on('internalMessage')Notes:
internalMessageis an undocumented Node.js event but has been stable across major versions. E/F hook onworker.process(ChildProcess) rather thancluster.Worker, since the latter does not forwardinternalMessage.child_process.fork) andworker_threadsmode are intentionally out of scope — they are not theclustermodule.onelogger.setLogger()/setCustomLogger().formatIpcMessagehas a safe replacer:net.Socket/net.Server/ Buffer / circular refs are collapsed to tokens; long strings are truncated at 200 chars; whole JSON payload is capped at 1024 chars. Handles get a+handle=<Socket fd=N>/<TCP>/<Server>suffix.Sample output
With
EGG_CLUSTER_IPC_LOG=1and a real HTTP request:Test plan
pnpm run typecheck— clean (tsgo)pnpm run lint— clean (oxlint, 0 warnings, 0 errors across 32 files)pnpm test— 31 failed / 21 passed identical to baseline (verified by running againstgit stash'd state); all failures are pre-existing in this branch and unrelated to this changeeggjs/clustermaster branch (same class structure and IPC points) — all 7 points print as expected withEGG_CLUSTER_IPC_LOG=1plus a real HTTP request, no Socket serialization errorsA companion PR lives at
eggjs/cluster#120for the 1.x legacy series (plain JSlib/layout).🤖 Generated with Claude Code
Summary by CodeRabbit