Replace pm2 with homegrown process manager daemon#2712
Replace pm2 with homegrown process manager daemon#2712fredrikekelund wants to merge 22 commits intotrunkfrom
Conversation
This reverts commit 24cdda5.
📊 Performance Test ResultsComparing b9212c6 vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff) |
bcotrim
left a comment
There was a problem hiding this comment.
While testing this I found some weird behaviors:
- Killing a site process (
kill pid) didn't reflect on Studio. Site was still running in the UI and stop operation didn't work (CLI worked fine). I could only replicate this once - Doing
Stop allfrom the Studio UI button, seems to break the UI. After doing stop all starting any other sites would not be reflected in the UI. I was able to replicate this consistently.
Added some minor comments to the code that aren't blockers, but we should look into these 2 issues.
| } from 'cli/lib/types/wordpress-server-ipc'; | ||
|
|
||
| // Zod schema for process descriptions | ||
| export const processDescriptionSchema = z.object( { |
There was a problem hiding this comment.
Should we be more intentional about the status + pid options?
I believe an online process will always have a pid, and the optional scenario is only for stopped processes. We could use an union to be more intentional about this type, what do you think?
| const socket = await this.connect(); | ||
| return this.sendAndReadFromSocket( socket, payload ); | ||
| } ); | ||
| this.queue = responsePromise.then( |
There was a problem hiding this comment.
This pattern is odd, I wonder if there's a better approach to it.
| import path from 'path'; | ||
|
|
||
| export const PROCESS_MANAGER_HOME = | ||
| process.env.STUDIO_PROCESS_MANAGER_HOME ?? path.join( os.homedir(), '.studio', 'pm2' ); |
There was a problem hiding this comment.
I'm assuming you decided keeping the pm2 name for backwards compatibility reasons, but I wonder if we should just rename it and handle existing folder content in a migration script or similar?
Related issues
How AI was used in this PR
Codex wrote most of the code in this PR, starting from a detailed list of requirements I shared (based on our experience with pm2). I iterated on this over multiple days, both with Codex and by hand. I've touched all major files in the PR.
process-manager-ipc.tsis a standout example – I made many manual edits there. I've reviewed all the code. The code I've spent the least time editing or reviewing is the tests.Proposed Changes
Tip
I'm happy to pair for a review of this PR. I know it's a big one. Also, please note that this PR builds upon the changes in #2690. That PR should be reviewed first.
See STU-1349 for the "why" of removing pm2. This PR implements the following:
process-manager-daemon.tsthat manages child processes. This involves forking them, tracking them in-memory, forwarding child IPC messages as typed daemon events, and writing stdout/stderr to PM2-compatible log files under~/.studio/pm2/logs. This file is the PM2 replacement.daemon-client.tsreplacespm2-manager.ts. It talks to the daemon over a control socket for request/response commands and an events socket for receiving messages from child processes and process events (online/exit, etc.).The architecture largely follows PM2's example, and
process-manager-daemon.tsimplements the same functionality. A few notes to get reviewers started:daemon-clientexports asendMessageToProcessfunction that uses the process manager "control socket" to send a message with aprocessIdtarget. Whenprocess-manager-daemon.tsreceives the message, it uses the standard Node.js IPC mechanism to forward it to the child process. Child processes cannot respond directly to these messages, but they can emit events through Node.js standard IPC, whichprocess-manager-daemon.tspicks up and broadcasts on the "events socket".daemonBuslistens on the events socket, and thesubscribeSiteEventsfunction picks up theprocess-messageandprocess-eventevents, which are ultimately picked up by_events.ts.Testing Instructions
npm startnode apps/cli/dist/cli/main.js site start --skip-browser --path PATH_TO_SITEin your terminalPre-merge Checklist