Repository navigation
fix(local stop): stop the server's process group, not just the npx wrapper - #15
Conversation
…apper Under the npx fallback the recorded pid is the npx wrapper; SIGTERM to it left the Playground child alive and holding the port, so the next 'local start' hit EADDRINUSE. The server is spawned detached, so it leads its own process group: signal -pid (taskkill /T on Windows), wait up to 5s, then SIGKILL. isServerRunning checks group liveness so an orphaned server is still stoppable. A pid file of 0/1/garbage is discarded (kill(0) signalled the CLI's own group). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
… escalation Review follow-up. The pid file outlives crashes/reboots, so a recycled pid could name an unrelated group (an interactive shell leads one) that now gets SIGTERM then SIGKILL. Check via `ps -eo pgid=,args=` that some process in the group carries this instance's --port= before signalling; otherwise drop the pid file. After SIGKILL, wait (<=2s) for the group to vanish so return means port free. Tests: SIGTERM-ignoring server is SIGKILLed; recycled pid is left alone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
…d away Review round 2: ps cuts args to the terminal width (~80 cols on macOS whenever it is not on a tty; on Linux when COLUMNS is set). Playground's port holder has a long prefix before --port=, so the ownership check read a live server as foreign and dropped its pid file: the original bug again. Tests: COLUMNS=40 truncation case; a marker in another process group does not make the pid ours. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
PASS at 93c5d15 (code-reviewer, round 3). Rounds 1 and 2 found 3 issues, all fixed. |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| */ | ||
| function signalServerTree(pid: number, signal: NodeJS.Signals | 0): boolean { | ||
| if (process.platform === 'win32') { | ||
| if (signal === 0) { |
There was a problem hiding this comment.
WARNING: Windows liveness check only checks wrapper PID, not process tree
In signalServerTree, when signal === 0 on Windows, process.kill(pid, 0) only checks if the single recorded PID (the npx wrapper) exists. But the actual kill uses taskkill /T /F which kills the entire process tree. If the npx wrapper dies but the Playground child survives, this liveness check returns false, causing isServerRunning to return false and stopServer to not kill the orphaned server — reproducing the original bug on Windows.
The POSIX path correctly uses process.kill(-pid, 0) to check the process group. On Windows, consider using tasklist or WMI to check the process tree, or accept that the Windows liveness check is best-effort.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| // not on a tty, on Linux when COLUMNS is set), which would cut off --port= and | ||
| // make a live server look like someone else's. | ||
| const ps = spawnSync('ps', ['-wweo', 'pgid=,args='], { encoding: 'utf-8', maxBuffer: 64 << 20 }); | ||
| if (ps.status !== 0 || typeof ps.stdout !== 'string') return true; |
There was a problem hiding this comment.
SUGGESTION: groupIsOurServer trusts PID file if ps fails
If ps command fails (non-zero exit code or no stdout), the function returns true, trusting the PID file. This could allow signaling an unrelated process if ps is unavailable (e.g., minimal container, busybox ps without -e/-o support) or fails for any reason. Consider logging a warning or at least documenting this fallback behavior.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| return false; | ||
| } | ||
|
|
||
| if (signalServerTree(pid, 'SIGTERM')) { |
There was a problem hiding this comment.
SUGGESTION: stopServer returns true even when no process was signaled
If signalServerTree(pid, 'SIGTERM') returns false (process/group already dead), the function skips the wait/escalation but still removes the PID file and returns true. This makes stopServer idempotent but callers cannot distinguish "stopped a running server" from "server was already dead". Consider returning false when nothing was signaled, or document the idempotent behavior explicitly.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (4 files)
Fix these issues in Kilo Cloud Reviewed by nemotron-3-ultra-550b-a55b:free · Input: 283.6K · Output: 9.8K · Cached: 436.3K |
|
QA: PASS ✅
Evidence: runs 38212894689 · 38213251664 · 38213564632 (macOS ps diag). Repro-before-fix: yes, on Linux and Windows. |
1 participant
Problem
Without a global
@wp-playground/clithe CLI runs Playground vianpx.instawp local stopprinted "Stopped", but the server kept listening, and the nextlocal startfailed withEADDRINUSE.Root cause
startServerBackgroundrecordschild.pid. Under npx that pid isnpm exec, and the server is two levels below it:npm exec→sh -c wp-playground-cli …→node …/wp-playground-cli server --port=….stopServersent SIGTERM to the recorded pid only, so the server survived.Fix (
src/lib/local-env.ts)detached, so the recorded pid leads its own process group.local stopsignals-pid, falling back to the pid alone if it doesn't lead a group. On Windows it runstaskkill /T /F.ps -wweo pgid=,args=and needs a process in that group whose arguments include this instance's--port=. If none does, it deletes the pid file and signals nothing.-wwmatters: on macOS,pscuts lines at about 80 columns when not attached to a terminal, which would hide--port=. Ifpsfails, the CLI trusts the pid file, as before.isServerRunningchecks the whole group, so a server whose wrapper died still shows asrunningand can be stopped.0,1or garbage is discarded. Before,0madeprocess.kill(0)signal the CLI's own process group.Evidence
local create --background, then SIGTERM only the recorded pid (the old behaviour): thewp-playground-clinode process survives and port 9400 still answers HTTP 302. This is the bug.local listshowsrunning.local stoptakes 0.66s and the port is free (curl 000).local start --backgroundsucceeds,local stopagain, and no Playground processes are left.src/__tests__/local-env-stop.test.ts, 8 tests. Each one fails when the mechanism it covers is removed:isServerRunningchecks the pid onlypswithout-wwCOLUMNS=40truncationapi-exec.test.tsfails to load in this sandbox, which is pre-existing and unrelated.taskkill /Tpath and BSDpscome from reading the code, not from a run.Reviews: code-reviewer (agent) found 3 issues across rounds 1 and 2. All are fixed in d92d13b and 93c5d15.
Agent task: tsk_a97c63295c8e1e05
Agentric session: https://aos.agents.instawp.net/#/sessions/aos-ses_38c321a35dad3faa
Next step (human):
/code-review ultra 15🤖 Generated with Claude Code