Fix infinite fork loop when node renderer worker fails to bind port#2843
Closed
yassa-tamer wants to merge 5 commits into
Closed
Fix infinite fork loop when node renderer worker fails to bind port#2843yassa-tamer wants to merge 5 commits into
yassa-tamer wants to merge 5 commits into
Conversation
When a worker fails during app.listen() (e.g. EADDRINUSE), the master previously reforked unconditionally, causing an infinite fork/crash loop. Workers now send a WORKER_STARTUP_FAILURE IPC message to the master before exiting. The master sets an abort flag and exits with a clear error message instead of reforking. Scheduled restarts and runtime crashes continue to refork as before. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughWorkers now send structured startup-failure IPC messages to the master when listen fails; the master records the first such failure and, on that worker's non-scheduled exit, emits a startup-specific error (special-casing EADDRINUSE) and exits with status 1 instead of forking a replacement. Changes
Sequence Diagram(s)sequenceDiagram
participant Worker as Worker Process
participant Cluster as Cluster IPC
participant Master as Master Process
participant OS as Operating System
Worker->>OS: app.listen(host, port)
OS-->>Worker: listen error (e.g. EADDRINUSE)
Worker->>Worker: build WorkerStartupFailureMessage
Worker->>Cluster: process.send(message)
Cluster->>Master: deliver "message" event
Master->>Master: record fatalStartupFailure / set isAbortingForStartupFailure = true
Worker->>OS: process.exit(1)
OS->>Cluster: emit "exit" for worker
Cluster->>Master: deliver "exit" event
Master->>Master: detect abort flag + non-scheduled exit
Master->>Master: emit startup-failure error (EADDRINUSE -> port-in-use text)
Master->>OS: process.exit(1)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes an infinite fork/crash loop that occurred when a node renderer worker failed to bind its port during startup (e.g. Key changes:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant M as Master
participant W as Worker (cluster)
M->>W: cluster.fork()
W->>W: app.listen({ port, host })
alt listen succeeds
W-->>M: (normal operation)
else listen fails (e.g. EADDRINUSE)
W->>M: process.send(WORKER_STARTUP_FAILURE)
note over M: cluster.on('message')<br/>sets isAbortingForStartupFailure=true<br/>stores fatalStartupFailure
W->>W: process.exit(1) [in send callback]
W-->>M: cluster.on('exit')
note over M: isAbortingForStartupFailure===true<br/>→ errorReporter.message()<br/>→ process.exit(1)
end
alt isScheduledRestart
W-->>M: cluster.on('exit')
M->>W: cluster.fork() [scheduled restart]
else unexpected runtime crash
W-->>M: cluster.on('exit')
M->>W: cluster.fork() [runtime refork]
end
Reviews (1): Last reviewed commit: "Fix infinite fork loop when node rendere..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+111
to
+112
| process.exit(1); | ||
| return; |
There was a problem hiding this comment.
process.exit()
process.exit(1) terminates the Node.js process synchronously, so the return on the next line is unreachable dead code. It can be removed.
Suggested change
| process.exit(1); | |
| return; | |
| errorReporter.message(msg); | |
| process.exit(1); |
Comment on lines
+88
to
113
| cluster.on('message', (worker, message) => { | ||
| if (!isWorkerStartupFailureMessage(message) || isAbortingForStartupFailure) return; | ||
|
|
||
| isAbortingForStartupFailure = true; | ||
| fatalStartupFailure = { workerId: worker.id, failure: message }; | ||
| }); | ||
|
|
||
| // Listen for dying workers: | ||
| cluster.on('exit', (worker) => { | ||
| if (worker.isScheduledRestart) { | ||
| log.info('Restarting worker #%d on schedule', worker.id); | ||
| } else { | ||
| // TODO: Track last rendering request per worker.id | ||
| // TODO: Consider blocking a given rendering request if it kills a worker more than X times | ||
| const msg = `Worker ${worker.id} died UNEXPECTEDLY :(, restarting`; | ||
| cluster.fork(); | ||
| return; | ||
| } | ||
|
|
||
| if (isAbortingForStartupFailure) { | ||
| const failure = fatalStartupFailure?.failure; | ||
| const msg = | ||
| failure?.code === 'EADDRINUSE' | ||
| ? `Node renderer startup failed: port ${failure.port} is already in use` | ||
| : `Node renderer startup failed in worker ${worker.id}: ${failure?.message || `exit code ${worker.process.exitCode}`}`; | ||
|
|
||
| errorReporter.message(msg); | ||
| process.exit(1); | ||
| return; | ||
| } |
There was a problem hiding this comment.
message and exit events
Node.js cluster does not guarantee that a worker's message event is processed before its exit event on the master side. If the OS delivers SIGCHLD (triggering exit) before the IPC socket data is read from the event loop (triggering message), the exit handler will observe isAbortingForStartupFailure === false and call cluster.fork() — spawning a replacement worker that will also fail.
In practice this is rare because the worker only calls process.exit(1) inside the process.send() callback (after the data has been flushed to the kernel buffer), so the timing window is very small. If it does trigger, it will cause one extra fork-and-crash cycle before the master eventually receives the deferred message event and shuts down cleanly — so it won't recreate an infinite loop, but it can still produce a confusing intermediate restart.
One mitigation would be to listen for the worker's IPC channel disconnect event and delay the refork decision briefly, or to cross-check the dead worker's id against any buffered-but-unprocessed WORKER_STARTUP_FAILURE messages. This is a known Node.js cluster caveat worth documenting if not addressed now.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/react-on-rails-pro-node-renderer/src/master.ts`:
- Around line 103-108: The generic startup-failure message currently uses
worker.id which may not be the recorded failing worker; update the message
construction in the isAbortingForStartupFailure branch to prefer
fatalStartupFailure.workerId (when present) instead of worker.id, and keep using
failure?.message or `exit code ${worker.process.exitCode}` for the error
details; ensure you still handle the EADDRINUSE case using failure.port and fall
back appropriately if fatalStartupFailure or its workerId is undefined.
In `@packages/react-on-rails-pro-node-renderer/src/shared/workerMessages.ts`:
- Around line 14-19: The current isWorkerStartupFailureMessage type guard only
checks the top-level type field so malformed IPC payloads slip through; update
the function isWorkerStartupFailureMessage to fully validate the IPC shape by
confirming the object has a type === WORKER_STARTUP_FAILURE and also contains
the expected fields with correct types (e.g., stage is a string, port is a
number, and message is a string) before narrowing to
WorkerStartupFailureMessage; reference the WORKER_STARTUP_FAILURE constant and
the isWorkerStartupFailureMessage function when making the change so master.ts
can safely read stage, port, and message without risking undefined values.
In
`@packages/react-on-rails-pro-node-renderer/tests/masterStartupFailure.test.ts`:
- Around line 41-73: The tests currently duplicate the production logic in local
functions handleMessage and handleExit; instead, mock or spy on cluster.on to
capture the actual callbacks that masterRun registers for 'message' and 'exit',
then invoke those captured callbacks with test fixtures so assertions validate
the real handlers from masterRun; change the test to call masterRun(), grab the
registered handlers from your cluster mock (the callbacks passed to
cluster.on('message', ...) and cluster.on('exit', ...)), and drive those
captured callbacks with the same worker/message scenarios used today to assert
reportedMessages, forkCount and exitCode.
In
`@packages/react-on-rails-pro-node-renderer/tests/workerStartupFailure.test.ts`:
- Around line 40-119: The test suite is not exercising the actual listen-error
branch in run() of worker.ts because it constructs the WORKER_STARTUP_FAILURE
payload and calls process.send/process.exit directly; update tests to either (A)
drive the real app.listen error callback by starting the module's run() (or
invoking the server instance) and producing an EADDRINUSE error so the run()
path sends the actual WorkerStartupFailureMessage, or (B) extract the
listen-error IPC logic into a helper (e.g., sendWorkerStartupFailure or
handleListenError) and write a unit test that calls that helper with the Error
object and verifies process.send behavior and exit handling using
cluster.isWorker and isWorkerStartupFailureMessage/WorkerStartupFailureMessage;
ensure tests toggle cluster.isWorker and stub process.send/process.exit only
around the actual code path being exercised.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 09e8287d-fe75-49a5-8584-9d4da70f441e
📒 Files selected for processing (5)
packages/react-on-rails-pro-node-renderer/src/master.tspackages/react-on-rails-pro-node-renderer/src/shared/workerMessages.tspackages/react-on-rails-pro-node-renderer/src/worker.tspackages/react-on-rails-pro-node-renderer/tests/masterStartupFailure.test.tspackages/react-on-rails-pro-node-renderer/tests/workerStartupFailure.test.ts
Comment on lines
+103
to
+108
| if (isAbortingForStartupFailure) { | ||
| const failure = fatalStartupFailure?.failure; | ||
| const msg = | ||
| failure?.code === 'EADDRINUSE' | ||
| ? `Node renderer startup failed: port ${failure.port} is already in use` | ||
| : `Node renderer startup failed in worker ${worker.id}: ${failure?.message || `exit code ${worker.process.exitCode}`}`; |
Contributor
There was a problem hiding this comment.
Use the recorded failing worker ID in the generic error path.
Once isAbortingForStartupFailure is set, the next exit event is not guaranteed to come from the same worker that sent the IPC message. Using worker.id here can misreport which worker actually failed even though fatalStartupFailure.workerId already captured it.
🐛 Proposed fix
if (isAbortingForStartupFailure) {
const failure = fatalStartupFailure?.failure;
+ const failedWorkerId = fatalStartupFailure?.workerId ?? worker.id;
const msg =
failure?.code === 'EADDRINUSE'
? `Node renderer startup failed: port ${failure.port} is already in use`
- : `Node renderer startup failed in worker ${worker.id}: ${failure?.message || `exit code ${worker.process.exitCode}`}`;
+ : `Node renderer startup failed in worker ${failedWorkerId}: ${failure?.message || `exit code ${worker.process.exitCode}`}`;
errorReporter.message(msg);
process.exit(1);📝 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.
Suggested change
| if (isAbortingForStartupFailure) { | |
| const failure = fatalStartupFailure?.failure; | |
| const msg = | |
| failure?.code === 'EADDRINUSE' | |
| ? `Node renderer startup failed: port ${failure.port} is already in use` | |
| : `Node renderer startup failed in worker ${worker.id}: ${failure?.message || `exit code ${worker.process.exitCode}`}`; | |
| if (isAbortingForStartupFailure) { | |
| const failure = fatalStartupFailure?.failure; | |
| const failedWorkerId = fatalStartupFailure?.workerId ?? worker.id; | |
| const msg = | |
| failure?.code === 'EADDRINUSE' | |
| ? `Node renderer startup failed: port ${failure.port} is already in use` | |
| : `Node renderer startup failed in worker ${failedWorkerId}: ${failure?.message || `exit code ${worker.process.exitCode}`}`; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-on-rails-pro-node-renderer/src/master.ts` around lines 103 -
108, The generic startup-failure message currently uses worker.id which may not
be the recorded failing worker; update the message construction in the
isAbortingForStartupFailure branch to prefer fatalStartupFailure.workerId (when
present) instead of worker.id, and keep using failure?.message or `exit code
${worker.process.exitCode}` for the error details; ensure you still handle the
EADDRINUSE case using failure.port and fall back appropriately if
fatalStartupFailure or its workerId is undefined.
Comment on lines
+14
to
+19
| export function isWorkerStartupFailureMessage(value: unknown): value is WorkerStartupFailureMessage { | ||
| return ( | ||
| typeof value === 'object' && | ||
| value !== null && | ||
| (value as { type?: string }).type === WORKER_STARTUP_FAILURE | ||
| ); |
Contributor
There was a problem hiding this comment.
Validate the full IPC payload shape in the type guard.
packages/react-on-rails-pro-node-renderer/src/master.ts reads stage, port, and message immediately after this check. Right now any { type: WORKER_STARTUP_FAILURE } payload passes, so a malformed IPC message can trigger a fatal abort and produce undefined diagnostics.
🛡️ Proposed fix
export function isWorkerStartupFailureMessage(value: unknown): value is WorkerStartupFailureMessage {
- return (
- typeof value === 'object' &&
- value !== null &&
- (value as { type?: string }).type === WORKER_STARTUP_FAILURE
- );
+ if (typeof value !== 'object' || value === null) {
+ return false;
+ }
+
+ const message = value as Partial<WorkerStartupFailureMessage>;
+ return (
+ message.type === WORKER_STARTUP_FAILURE &&
+ message.stage === 'listen' &&
+ typeof message.host === 'string' &&
+ typeof message.port === 'number' &&
+ typeof message.message === 'string' &&
+ (message.code === undefined || typeof message.code === 'string') &&
+ (message.errno === undefined || typeof message.errno === 'number') &&
+ (message.syscall === undefined || typeof message.syscall === 'string')
+ );
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-on-rails-pro-node-renderer/src/shared/workerMessages.ts`
around lines 14 - 19, The current isWorkerStartupFailureMessage type guard only
checks the top-level type field so malformed IPC payloads slip through; update
the function isWorkerStartupFailureMessage to fully validate the IPC shape by
confirming the object has a type === WORKER_STARTUP_FAILURE and also contains
the expected fields with correct types (e.g., stage is a string, port is a
number, and message is a string) before narrowing to
WorkerStartupFailureMessage; reference the WORKER_STARTUP_FAILURE constant and
the isWorkerStartupFailureMessage function when making the change so master.ts
can safely read stage, port, and message without risking undefined values.
Comment on lines
+41
to
+73
| function handleMessage(worker: { id: number }, message: unknown) { | ||
| if (!isWorkerStartupFailureMessage(message) || isAbortingForStartupFailure) return; | ||
|
|
||
| isAbortingForStartupFailure = true; | ||
| fatalStartupFailure = { workerId: worker.id, failure: message }; | ||
| } | ||
|
|
||
| function handleExit(worker: { | ||
| id: number; | ||
| isScheduledRestart?: boolean; | ||
| process: { exitCode: number | null }; | ||
| }) { | ||
| if (worker.isScheduledRestart) { | ||
| forkCount += 1; // cluster.fork() | ||
| return; | ||
| } | ||
|
|
||
| if (isAbortingForStartupFailure) { | ||
| const failure = fatalStartupFailure?.failure; | ||
| const msg = | ||
| failure?.code === 'EADDRINUSE' | ||
| ? `Node renderer startup failed: port ${failure.port} is already in use` | ||
| : `Node renderer startup failed in worker ${worker.id}: ${failure?.message || `exit code ${worker.process.exitCode}`}`; | ||
|
|
||
| reportedMessages.push(msg); | ||
| exitCode = 1; | ||
| return; | ||
| } | ||
|
|
||
| const msg = `Worker ${worker.id} died UNEXPECTEDLY :(, restarting`; | ||
| reportedMessages.push(msg); | ||
| forkCount += 1; // cluster.fork() | ||
| } |
Contributor
There was a problem hiding this comment.
These tests duplicate packages/react-on-rails-pro-node-renderer/src/master.ts instead of testing it.
handleMessage and handleExit are local copies of the production branches, so the assertions below can stay green even if masterRun() stops registering the cluster handlers or changes their control flow. Please capture the real cluster.on('message') / cluster.on('exit') callbacks from masterRun() under mocks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-on-rails-pro-node-renderer/tests/masterStartupFailure.test.ts`
around lines 41 - 73, The tests currently duplicate the production logic in
local functions handleMessage and handleExit; instead, mock or spy on cluster.on
to capture the actual callbacks that masterRun registers for 'message' and
'exit', then invoke those captured callbacks with test fixtures so assertions
validate the real handlers from masterRun; change the test to call masterRun(),
grab the registered handlers from your cluster mock (the callbacks passed to
cluster.on('message', ...) and cluster.on('exit', ...)), and drive those
captured callbacks with the same worker/message scenarios used today to assert
reportedMessages, forkCount and exitCode.
Comment on lines
+40
to
+119
| describe('worker startup failure IPC', () => { | ||
| const originalSend = process.send; | ||
| const originalExit = process.exit; | ||
| const originalIsWorker = cluster.isWorker; | ||
|
|
||
| afterEach(() => { | ||
| process.send = originalSend; | ||
| process.exit = originalExit; | ||
| Object.defineProperty(cluster, 'isWorker', { value: originalIsWorker, writable: true }); | ||
| }); | ||
|
|
||
| it('sends WORKER_STARTUP_FAILURE message in clustered mode', () => { | ||
| // Simulate a cluster worker environment | ||
| Object.defineProperty(cluster, 'isWorker', { value: true, writable: true }); | ||
| const sentMessages: unknown[] = []; | ||
| const exitCalls: number[] = []; | ||
|
|
||
| process.send = ((msg: unknown, _handle?: unknown, _options?: unknown, callback?: () => void) => { | ||
| sentMessages.push(msg); | ||
| if (callback) callback(); | ||
| return true; | ||
| }) as typeof process.send; | ||
|
|
||
| process.exit = ((code?: number) => { | ||
| exitCalls.push(code ?? 0); | ||
| }) as typeof process.exit; | ||
|
|
||
| // Simulate what the worker does on listen error | ||
| const err = Object.assign(new Error('listen EADDRINUSE: address already in use :::3800'), { | ||
| code: 'EADDRINUSE', | ||
| errno: -48, | ||
| syscall: 'listen', | ||
| }); | ||
| const host = 'localhost'; | ||
| const port = 3800; | ||
|
|
||
| const startupFailure: WorkerStartupFailureMessage = { | ||
| type: WORKER_STARTUP_FAILURE, | ||
| stage: 'listen', | ||
| code: err.code, | ||
| errno: err.errno, | ||
| syscall: err.syscall, | ||
| host, | ||
| port, | ||
| message: err.message, | ||
| }; | ||
|
|
||
| process.send!(startupFailure, undefined, undefined, () => { | ||
| process.exit(1); | ||
| }); | ||
|
|
||
| expect(sentMessages).toHaveLength(1); | ||
| expect(isWorkerStartupFailureMessage(sentMessages[0])).toBe(true); | ||
| expect((sentMessages[0] as WorkerStartupFailureMessage).code).toBe('EADDRINUSE'); | ||
| expect(exitCalls).toEqual([1]); | ||
| }); | ||
|
|
||
| it('exits without IPC in single-process mode', () => { | ||
| Object.defineProperty(cluster, 'isWorker', { value: false, writable: true }); | ||
| process.send = undefined; | ||
|
|
||
| const exitCalls: number[] = []; | ||
| process.exit = ((code?: number) => { | ||
| exitCalls.push(code ?? 0); | ||
| }) as typeof process.exit; | ||
|
|
||
| // In single-process mode, cluster.isWorker is false and process.send is undefined | ||
| if (cluster.isWorker && process.send) { | ||
| // Should not reach here | ||
| process.send({ type: WORKER_STARTUP_FAILURE }, undefined, undefined, () => { | ||
| process.exit(1); | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| process.exit(1); | ||
|
|
||
| expect(exitCalls).toEqual([1]); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
This suite doesn't exercise packages/react-on-rails-pro-node-renderer/src/worker.ts.
It reconstructs the payload and calls process.send/process.exit directly, so it still passes if the listen-error branch in run() stops emitting WORKER_STARTUP_FAILURE. Please drive the real app.listen error callback or extract that branch into a helper and test the helper.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-on-rails-pro-node-renderer/tests/workerStartupFailure.test.ts`
around lines 40 - 119, The test suite is not exercising the actual listen-error
branch in run() of worker.ts because it constructs the WORKER_STARTUP_FAILURE
payload and calls process.send/process.exit directly; update tests to either (A)
drive the real app.listen error callback by starting the module's run() (or
invoking the server instance) and producing an EADDRINUSE error so the run()
path sends the actual WorkerStartupFailureMessage, or (B) extract the
listen-error IPC logic into a helper (e.g., sendWorkerStartupFailure or
handleListenError) and write a unit test that calls that helper with the Error
object and verifies process.send behavior and exit handling using
cluster.isWorker and isWorkerStartupFailureMessage/WorkerStartupFailureMessage;
ensure tests toggle cluster.isWorker and stub process.send/process.exit only
around the actual code path being exercised.
Member
|
@claude review this PR |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
3 tasks
Member
|
See #2881 |
justin808
added a commit
that referenced
this pull request
Apr 7, 2026
…2881) ## Summary When a worker fails during app.listen() (e.g. EADDRINUSE), the master previously reforked unconditionally, causing an infinite fork/crash loop. Workers now send a WORKER_STARTUP_FAILURE IPC message to the master before exiting. The master sets an abort flag and exits with a clear error message instead of reforking. Scheduled restarts and runtime crashes continue to refork as before. Based on #2843 by @Yassa-hue — recreated on the main repo (not a fork) so CI runs with full secrets. Closes #2843 fix issue: shakacode/react_on_rails_pro#582 ## Test plan - [ ] Verify new unit tests pass (`masterStartupFailure.test.ts`, `workerStartupFailure.test.ts`) - [ ] Confirm EADDRINUSE scenario aborts master instead of infinite reforking - [ ] Confirm scheduled restarts and runtime crashes still refork as before 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches clustered process lifecycle and shutdown behavior in the Pro Node renderer; a mistake could cause premature master exits or missed reforks in edge cases despite added test coverage. > > **Overview** > Prevents the Pro Node renderer master process from entering an infinite refork loop when a worker fails during `app.listen()` (e.g., `EADDRINUSE`). Workers now report a structured `WORKER_STARTUP_FAILURE` IPC message before exiting, and the master captures the first failure, disconnects workers, and exits with a clear error instead of reforking. > > Also adds `port` coercion + TCP-range validation in `configBuilder`, and introduces focused unit tests covering startup-failure messaging, master abort semantics, and the “wait one tick” behavior to distinguish startup failures from runtime crashes. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit ca6ac01. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Yassa-hue <yassatamer99@gmail.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
justin808
added a commit
that referenced
this pull request
Apr 12, 2026
…2881) ## Summary When a worker fails during app.listen() (e.g. EADDRINUSE), the master previously reforked unconditionally, causing an infinite fork/crash loop. Workers now send a WORKER_STARTUP_FAILURE IPC message to the master before exiting. The master sets an abort flag and exits with a clear error message instead of reforking. Scheduled restarts and runtime crashes continue to refork as before. Based on #2843 by @Yassa-hue — recreated on the main repo (not a fork) so CI runs with full secrets. Closes #2843 fix issue: shakacode/react_on_rails_pro#582 ## Test plan - [ ] Verify new unit tests pass (`masterStartupFailure.test.ts`, `workerStartupFailure.test.ts`) - [ ] Confirm EADDRINUSE scenario aborts master instead of infinite reforking - [ ] Confirm scheduled restarts and runtime crashes still refork as before 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches clustered process lifecycle and shutdown behavior in the Pro Node renderer; a mistake could cause premature master exits or missed reforks in edge cases despite added test coverage. > > **Overview** > Prevents the Pro Node renderer master process from entering an infinite refork loop when a worker fails during `app.listen()` (e.g., `EADDRINUSE`). Workers now report a structured `WORKER_STARTUP_FAILURE` IPC message before exiting, and the master captures the first failure, disconnects workers, and exits with a clear error instead of reforking. > > Also adds `port` coercion + TCP-range validation in `configBuilder`, and introduces focused unit tests covering startup-failure messaging, master abort semantics, and the “wait one tick” behavior to distinguish startup failures from runtime crashes. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit ca6ac01. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Yassa-hue <yassatamer99@gmail.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
justin808
added a commit
that referenced
this pull request
Apr 12, 2026
…2881) ## Summary When a worker fails during app.listen() (e.g. EADDRINUSE), the master previously reforked unconditionally, causing an infinite fork/crash loop. Workers now send a WORKER_STARTUP_FAILURE IPC message to the master before exiting. The master sets an abort flag and exits with a clear error message instead of reforking. Scheduled restarts and runtime crashes continue to refork as before. Based on #2843 by @Yassa-hue — recreated on the main repo (not a fork) so CI runs with full secrets. Closes #2843 fix issue: shakacode/react_on_rails_pro#582 ## Test plan - [ ] Verify new unit tests pass (`masterStartupFailure.test.ts`, `workerStartupFailure.test.ts`) - [ ] Confirm EADDRINUSE scenario aborts master instead of infinite reforking - [ ] Confirm scheduled restarts and runtime crashes still refork as before 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches clustered process lifecycle and shutdown behavior in the Pro Node renderer; a mistake could cause premature master exits or missed reforks in edge cases despite added test coverage. > > **Overview** > Prevents the Pro Node renderer master process from entering an infinite refork loop when a worker fails during `app.listen()` (e.g., `EADDRINUSE`). Workers now report a structured `WORKER_STARTUP_FAILURE` IPC message before exiting, and the master captures the first failure, disconnects workers, and exits with a clear error instead of reforking. > > Also adds `port` coercion + TCP-range validation in `configBuilder`, and introduces focused unit tests covering startup-failure messaging, master abort semantics, and the “wait one tick” behavior to distinguish startup failures from runtime crashes. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit ca6ac01. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Yassa-hue <yassatamer99@gmail.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When a worker fails during app.listen() (e.g. EADDRINUSE), the master previously reforked unconditionally, causing an infinite fork/crash loop.
Workers now send a WORKER_STARTUP_FAILURE IPC message to the master before exiting. The master sets an abort flag and exits with a clear error message instead of reforking. Scheduled restarts and runtime crashes continue to refork as before.
Pull Request checklist
Remove this line after checking all the items here. If the item is not applicable to the PR, both check it out and wrap it by
~.Add the CHANGELOG entry at the top of the file.
Summary by CodeRabbit