cli: add dev lock file to prevent 2 dev processes running at the same time in the same dir #1854
Conversation
|
WalkthroughThis pull request introduces a lock file mechanism within the development session. The Changes
Sequence Diagram(s)sequenceDiagram
participant DevCmd as Dev Command
participant Lock as createLockFile (lock.ts)
participant Proc as Other Process
DevCmd->>Lock: Call createLockFile(cwd)
alt Lock file exists and is owned by same process
Lock-->>DevCmd: Return cleanup function
else Lock file exists and owned by another process
Lock->>Proc: Attempt to terminate process (PID check)
Proc-->>Lock: Process termination / error logged
Lock-->>DevCmd: Return cleanup function after wait
else No lock file exists
Lock-->>DevCmd: Write lock file and return cleanup function
end
DevCmd->>Lock: On stop, execute cleanup to remove lock file
Possibly related issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🔇 Additional comments (2)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (3)
packages/cli-v3/src/dev/lock.ts (3)
29-32: Consider using a more structured lock file format.The current implementation stores only the PID in the lock file. Consider using a JSON format that includes additional information such as the app name, start time, and command line, which would help verify the PID actually belongs to another instance of this application.
if (existingLockfileContents) { - // Read the pid number from the lockfile - const existingPid = Number(existingLockfileContents); + // Try to parse the lock file content as JSON first + try { + const lockData = JSON.parse(existingLockfileContents); + const existingPid = lockData.pid; + const timestamp = lockData.timestamp; + const appName = lockData.appName; + + logger.debug("Lockfile exists", { + lockFilePath, + existingPid, + currentPid, + timestamp, + appName + }); + + // Continue with existing logic... + } catch (e) { + // Fallback to the simple PID format for backward compatibility + const existingPid = Number(existingLockfileContents); + logger.debug("Lockfile exists (legacy format)", { lockFilePath, existingPid, currentPid });This would be paired with a change to the lock file creation:
// Now write the current pid to the lockfile - await writeFile(lockFilePath, currentPid.toString()); + const lockData = { + pid: currentPid, + timestamp: new Date().toISOString(), + appName: "trigger-cli-dev", + command: process.argv.join(" ") + }; + await writeFile(lockFilePath, JSON.stringify(lockData, null, 2));
55-75: Improve process termination with escalation to SIGKILL.The current implementation sends a SIGTERM to the existing process, but doesn't escalate to SIGKILL if the process doesn't terminate. Additionally, consider a more graceful approach by first sending SIGINT (Ctrl+C equivalent) before escalating.
try { - process.kill(existingPid); + // First try SIGINT (like Ctrl+C) for a graceful shutdown + process.kill(existingPid, 'SIGINT'); + // If it did kill the process, it will have exited, deleting the lockfile, so let's wait for that to happen // But let's not wait forever await new Promise((resolve, reject) => { + let killAttempts = 0; const timeout = setTimeout(() => { clearInterval(interval); - reject(new Error("Timed out waiting for lockfile to be deleted")); + // Last resort: force kill with SIGKILL + try { + logger.debug("Timed out waiting for process to exit, sending SIGKILL", { existingPid }); + process.kill(existingPid, 'SIGKILL'); + // Give it a moment to clean up + setTimeout(() => { + if (existsSync(lockFilePath)) { + // If lock file still exists, remove it forcefully + try { + unlinkSync(lockFilePath); + resolve(true); + } catch (e) { + reject(new Error(`Failed to forcefully remove lock file: ${e}`)); + } + } else { + resolve(true); + } + }, 500); + } catch (killError) { + logger.debug("Failed to kill process with SIGKILL", { error: killError }); + reject(new Error("Failed to kill existing process")); + } }, 5000); const interval = setInterval(() => { if (!existsSync(lockFilePath)) { clearInterval(interval); clearTimeout(timeout); resolve(true); } + // Escalate to SIGTERM after a few seconds of SIGINT not working + killAttempts++; + if (killAttempts === 20) { // After ~2 seconds + try { + logger.debug("Process did not exit with SIGINT, trying SIGTERM", { existingPid }); + process.kill(existingPid, 'SIGTERM'); + } catch (e) { + // Ignore errors when sending signals + } + } }, 100); }); } catch (error) { logger.debug("Failed to kill existing process, lets assume it's not running", { error }); + // If we couldn't kill it, try to remove the lock file anyway + try { + unlinkSync(lockFilePath); + logger.debug("Removed stale lock file", { lockFilePath }); + } catch (e) { + logger.debug("Failed to remove stale lock file", { error: e }); + } }
17-24: Consider consistent async/sync file operations.The code mixes synchronous (
unlinkSync) and asynchronous (writeFile,readFile) file operations. While this works, consistency would improve maintainability.const removeLockFile = () => { try { logger.debug("Removing lockfile", { lockFilePath }); - return unlinkSync(lockFilePath); + // Use the asynchronous version for consistency + // However, since this is used in an exit handler, sync is actually better + // Just adding a comment to clarify the intentional use of sync methods + // in an otherwise async codebase + return unlinkSync(lockFilePath); // Using sync version intentionally for exit handler } catch (e) { // This sometimes fails on Windows with EBUSY + logger.debug("Failed to remove lock file", { error: e }); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/cli-v3/src/commands/dev.ts(3 hunks)packages/cli-v3/src/dev/lock.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/cli-v3/src/commands/dev.ts (1)
packages/cli-v3/src/dev/lock.ts (1)
createLockFile(11-87)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: typecheck / typecheck
🔇 Additional comments (3)
packages/cli-v3/src/commands/dev.ts (3)
14-14: Good addition of the lock file import.This import enables the locking mechanism to prevent multiple development processes from running simultaneously.
124-125: Lock file implementation looks good.The
createLockFilefunction is called with the correct working directory and its return value (cleanup function) is properly stored for later use.
184-184: Proper cleanup in the stop method.The
removeLockFilefunction is correctly called when stopping the dev session, ensuring the lock file is removed when the application exits normally.
Summary by CodeRabbit