fix: prevent app hang when server fails to start#342
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe Electron main process adds timeout handling to server initialization. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ 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. Review rate limit: 0/1 reviews remaining, refill in 51 minutes and 33 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
electron/main.cjs (1)
53-80:⚠️ Potential issue | 🔴 CriticalAdd error and exit event listeners to the spawned process to prevent crashes and delayed failure detection.
The
spawn()call lacks error and exit listeners. If spawning fails (e.g., 'node' command not found), the unhandlederrorevent can crash the app. If the process exits before readiness checks pass, the error won't be caught until the 15-second timeout expires instead of failing immediately.Suggested fix
function startServer() { return new Promise((resolve, reject) => { + let settled = false; + const fail = (err) => { + if (settled) return; + settled = true; + if (serverProcess) serverProcess.kill(); + reject(err); + }; + const serverPath = path.join( process.resourcesPath, 'app.asar.unpacked', '.output', 'server', 'index.mjs' ); console.log("Starting server from:", serverPath); serverProcess = spawn('node', [serverPath], { stdio: 'ignore', // no terminal windowsHide: true, // hide CMD env: { ...process.env, HOST: serverHost, PORT: serverPort.toString(), }, }); + serverProcess.once('error', (err) => { + fail(new Error(`Server process failed to spawn: ${err.message}`)); + }); + + serverProcess.once('exit', (code, signal) => { + if (!settled) { + fail(new Error(`Server exited before readiness check passed (code=${code}, signal=${signal})`)); + } + }); + waitForServer(`http://localhost:${serverPort}`) .then(resolve) .catch((err) => { console.error('Server failed to start:', err); - if (serverProcess) serverProcess.kill(); - reject(err) + fail(err); }); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/main.cjs` around lines 53 - 80, The spawn() of the server (serverProcess = spawn('node', [serverPath], ...)) needs immediate 'error' and 'exit' listeners so failures are detected and handled instead of crashing or waiting for waitForServer timeout; add serverProcess.on('error', ...) to log the error, kill/cleanup the process and reject the Promise (or call reject with the error) and add serverProcess.on('exit', (code, signal) => ...) to log the premature exit and reject the Promise immediately (also kill/cleanup if needed), ensuring these handlers reference serverProcess, serverPath, serverHost/ serverPort for context and that waitForServer's pending promise is rejected when the process fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@electron/main.cjs`:
- Around line 109-115: The catch block after await startServer()/createWindow()
must ensure any spawned backend is terminated: check the serverProcess variable
(from startServer) for existence and whether it's still running, call
serverProcess.kill() (prefer SIGTERM or appropriate signal) and optionally
wait/force-kill if it doesn't exit, then call app.quit(); update the catch in
the startup sequence handling to kill serverProcess before app.quit() so the
backend isn't orphaned.
- Around line 31-44: The waitForServer function can hang if an HTTP request
connects but never responds because the timeout is only checked in the 'error'
handler; update waitForServer to enforce a per-request timeout by capturing the
request object (e.g., const req = http.get(...)), call
req.setTimeout(timeoutPerAttempt) to trigger a timeout handler that aborts the
request and treats it like an error, and before each new attempt ensure the
overall deadline (Date.now() - start > timeout) is checked so you reject
promptly; changes should be applied inside waitForServer around the http.get
call and its handlers (use the same timeout logic currently used in the outer
error handler).
---
Outside diff comments:
In `@electron/main.cjs`:
- Around line 53-80: The spawn() of the server (serverProcess = spawn('node',
[serverPath], ...)) needs immediate 'error' and 'exit' listeners so failures are
detected and handled instead of crashing or waiting for waitForServer timeout;
add serverProcess.on('error', ...) to log the error, kill/cleanup the process
and reject the Promise (or call reject with the error) and add
serverProcess.on('exit', (code, signal) => ...) to log the premature exit and
reject the Promise immediately (also kill/cleanup if needed), ensuring these
handlers reference serverProcess, serverPath, serverHost/ serverPort for context
and that waitForServer's pending promise is rejected when the process fails.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 166dbf54-eb20-4f64-ae3b-4fa19782fc74
📒 Files selected for processing (1)
electron/main.cjs
1 participant
Description
Fixes an issue where the app would hang indefinitely if the backend server failed to start.
Functional Verification
Additional Notes
This change only affects Electron main process startup flow.
Summary by CodeRabbit