fix(desktop): prevent Electron dev process leaks on restart - #164
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| spawnSync("pkill", ["-f", "--", `--t3code-dev-root=${desktopDir}`], { stdio: "ignore" }); | ||
| } |
There was a problem hiding this comment.
🟡 Medium scripts/dev-electron.mjs:46
pkill -f interprets the pattern as an extended regular expression, but desktopDir is inserted verbatim without escaping regex metacharacters. When the path contains . or other regex symbols (common in paths like /home/user/.config/ or dotted directory names), the pattern matches unintended processes. For example, a path /home/user/.dev/app where . matches any character would also match /home/user/Xdev/app. Consider escaping desktopDir for regex use (e.g., replacing . with \.) before passing it to pkill -f.
+ const escapedDesktopDir = desktopDir.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+
- spawnSync("pkill", ["-f", "--", `--t3code-dev-root=${desktopDir}`], { stdio: "ignore" });
+ spawnSync("pkill", ["-f", "--", `--t3code-dev-root=${escapedDesktopDir}`], { stdio: "ignore" });🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/desktop/scripts/dev-electron.mjs around lines 46-47:
`pkill -f` interprets the pattern as an extended regular expression, but `desktopDir` is inserted verbatim without escaping regex metacharacters. When the path contains `.` or other regex symbols (common in paths like `/home/user/.config/` or dotted directory names), the pattern matches unintended processes. For example, a path `/home/user/.dev/app` where `.` matches any character would also match `/home/user/Xdev/app`. Consider escaping `desktopDir` for regex use (e.g., replacing `.` with `\.`) before passing it to `pkill -f`.
Evidence trail:
1. apps/desktop/scripts/dev-electron.mjs line 46: `spawnSync("pkill", ["-f", "--", `--t3code-dev-root=${desktopDir}`], { stdio: "ignore" });`
2. apps/desktop/scripts/electron-launcher.mjs line 22: `export const desktopDir = resolve(__dirname, "..");`
3. Ubuntu pkill manpage (https://manpages.ubuntu.com/manpages/xenial/man1/pkill.1.html) OPERANDS section: "pattern - Specifies an Extended Regular Expression for matching against the process names or command lines."
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Forced-kill resolves
stopAppbefore process exits- Removed the immediate
finish()call after SIGKILL so the promise now resolves only when theexitevent fires, ensuring the old process has actually exited before a new one is spawned.
- Removed the immediate
Or push these changes by commenting:
@cursor push 5728686213
Preview (5728686213)
diff --git a/apps/desktop/scripts/dev-electron.mjs b/apps/desktop/scripts/dev-electron.mjs
--- a/apps/desktop/scripts/dev-electron.mjs
+++ b/apps/desktop/scripts/dev-electron.mjs
@@ -119,7 +119,6 @@
app.kill("SIGKILL");
killChildTreeByPid(app.pid, "KILL");
- finish();
}, forcedShutdownTimeoutMs).unref();
});
}| app.kill("SIGKILL"); | ||
| killChildTreeByPid(app.pid, "KILL"); | ||
| finish(); | ||
| }, forcedShutdownTimeoutMs).unref(); |
There was a problem hiding this comment.
Forced-kill resolves stopApp before process exits
Low Severity
In the forced-kill timeout path of stopApp(), finish() is called immediately after app.kill("SIGKILL") — before the process has actually exited. This causes stopApp() to return and startApp() to spawn a new Electron instance while the old one is still alive, creating a brief window where two Electron processes run concurrently. This directly contradicts the PR's stated goal of preventing concurrent Electron dev processes. The exit-handler guards (expectedExits, currentApp identity checks) correctly handle the eventual exit of the old process, but the overlap itself is avoidable by waiting for the exit event even after SIGKILL.
- replace `electronmon` with explicit Electron spawn/stop lifecycle management - debounce file-watch restarts for main/preload/server build outputs - add stale-process cleanup and forced child-tree shutdown fallback
- drop `electronmon` from desktop dependencies - stop killing stale processes by matching electronmon hook paths - keep cleanup scoped to `--t3code-dev-root` processes only
- add `childTreeGracePeriodMs` constant for shutdown timing - await grace period after `TERM` before sending `KILL` to avoid dev process leaks
26b775b to
2edd94d
Compare
- add `electron-updater` dependency for `apps/desktop` - bump workspace package versions from `0.0.0-alpha.13` to `0.0.0-alpha.16` - update transitive lockfile entries accordingly
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Shutdown unconditionally blocks for grace period delay
- Changed the SIGKILL grace period timer to use .unref() and removed the await, so process.exit() runs immediately instead of blocking for 1.2 seconds.
- ✅ Fixed: Startup order may kill newly spawned app process
- Moved cleanupStaleDevApps() to run before startWatchers() and startApp(), ensuring stale processes are cleaned up before any new activity begins.
Or push these changes by commenting:
@cursor push 3ee9f83046
Preview (3ee9f83046)
diff --git a/apps/desktop/scripts/dev-electron.mjs b/apps/desktop/scripts/dev-electron.mjs
--- a/apps/desktop/scripts/dev-electron.mjs
+++ b/apps/desktop/scripts/dev-electron.mjs
@@ -184,16 +184,13 @@
await stopApp();
killChildTree("TERM");
- await new Promise((resolve) => {
- setTimeout(resolve, childTreeGracePeriodMs);
- });
- killChildTree("KILL");
+ setTimeout(() => killChildTree("KILL"), childTreeGracePeriodMs).unref();
process.exit(exitCode);
}
+cleanupStaleDevApps();
startWatchers();
-cleanupStaleDevApps();
startApp();
process.once("SIGINT", () => {| await new Promise((resolve) => { | ||
| setTimeout(resolve, childTreeGracePeriodMs); | ||
| }); | ||
| killChildTree("KILL"); |
There was a problem hiding this comment.
Shutdown unconditionally blocks for grace period delay
Medium Severity
The shutdown function unconditionally awaits a childTreeGracePeriodMs (1.2 second) timer between sending SIGTERM and SIGKILL to the child tree. The old code used .unref() on the timer and called process.exit() immediately, so shutdown was near-instant in the common case. Now every Ctrl+C during development will always take at least 1.2 seconds (plus up to 1.5 seconds in stopApp), even when no straggler processes exist.
|
|
||
| startWatchers(); | ||
| cleanupStaleDevApps(); | ||
| startApp(); |
There was a problem hiding this comment.
Startup order may kill newly spawned app process
Medium Severity
cleanupStaleDevApps is called after startWatchers but just before startApp. Since cleanupStaleDevApps uses pkill -f matching the --t3code-dev-root argument, and startApp spawns a process with that same argument, if cleanupStaleDevApps were ever reordered after startApp (or called again), it would kill the freshly spawned instance. More critically, on a very fast system, if a prior stale process hasn't fully exited when startApp runs, pkill and the new spawn could race. Swapping the call order to run cleanup first (before watchers too) would make the intent clearer and safer.
1 participant
Summary
electronmonusage with explicit Electron child-process lifecycle management indev-electron.mjsSIGTERMthenSIGKILL) and child-tree cleanup to avoid orphaned Electron processes--t3code-dev-rootmarkerwait-onfor renderer port and required build artifactsTesting
dist-electron/main.js,dist-electron/preload.js, and../server/dist/index.mjs; verify a single debounced restart per change burstSIGINT/SIGTERM; verify no orphan Electron processes remainNote
Medium Risk
Touches the desktop dev runner’s process lifecycle and uses
pkill-based cleanup, which could behave differently across platforms or accidentally terminate the wrong processes if the marker argument changes. No production/runtime app logic is modified, but dev workflows could regress (restart/shutdown reliability).Overview
Replaces
electronmonwith an explicit Electron process supervisor inapps/desktop/scripts/dev-electron.mjsto stop dev-mode process leaks.The dev script now launches Electron via
spawn, watches specific build outputs (dist-electron/main.js,dist-electron/preload.js,../server/dist/index.mjs), and performs debounced, serialized restarts (stop then start) when those files change.Adds graceful shutdown + forced-kill fallback (
SIGTERMthenSIGKILL) and Unix-only child-process tree cleanup, plus a startup sweep that kills stale Electron dev instances via a scoped--t3code-dev-root=...marker. Removeselectronmonfromapps/desktop/package.jsonand updatesbun.lockaccordingly.Written by Cursor Bugbot for commit 1633fc6. This will update automatically on new commits. Configure here.
Note
Replace electronmon with a direct Electron launcher in dev-electron.mjs to prevent desktop dev process leaks on restart
Rewrites the dev runner to spawn Electron directly, add debounced (120ms) restarts on file changes, gracefully stop processes with SIGTERM then SIGKILL, and clean up stale Electron dev processes; removes the
electronmondependency and updates the lockfile.📍Where to Start
Start at the script entry flow in dev-electron.mjs, focusing on
startWatchers,startApp,stopApp, andscheduleRestart.Macroscope summarized 1633fc6.