diff --git a/CHANGELOG.md b/CHANGELOG.md index e99fb5c..5d90d37 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,16 @@ # Changelog +## Unreleased + +### Fixed — `local stop` left the server running when Playground ran via npx + +Without a global `@wp-playground/cli`, `instawp local stop` printed "Stopped" but the site kept serving, and the next `local start` failed with `EADDRINUSE`. + +- The CLI recorded the pid of the `npx` wrapper and signalled only that; the Playground server is the wrapper's child and survived. `local stop` now stops the server's whole process group (Windows: `taskkill /T /F`) and waits for it to exit, so the port is free when it returns. On macOS/Linux it sends SIGTERM first and SIGKILL after 5s. +- Before signalling, the CLI checks that the group still belongs to this site (a process carrying its `--port=`). A `server.pid` left behind by a crash or reboot can name a recycled pid, such as a terminal shell, and is now discarded instead of killed. macOS/Linux only. +- On macOS/Linux, `local list` reports a site whose wrapper died but whose server is still up as `running`, so `local stop` can still reach it (it used to show `stopped` and forget the pid). +- A corrupt `server.pid` (`0`, `1`, non-numeric) is now discarded. Before, a `0` made `local stop` signal the CLI's own process group. + ## 0.0.1-beta.34 (2026-10-09) ### Fixed — `npm install -g @instawp/cli` fails on Windows with Node 24+ diff --git a/CLAUDE.md b/CLAUDE.md index c6318a4..30e3660 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -170,7 +170,7 @@ The standalone-CLI replacement for the plugin's `wp instawp local push`. Mirrors ### Background mode - `--background` flag spawns detached process, polls until server responds, returns immediately - PID stored at `/server.pid`, logs at `/server.log` -- `local stop` kills the background process +- `local stop` kills the server's whole **process group** (`kill(-pid)`; Windows `taskkill /T`), not just the recorded pid — under the npx fallback that pid is the npx wrapper and Playground is its child. Waits ≤5s for exit, then SIGKILL - `local list` shows `running`/`stopped` status ### SSH key management diff --git a/src/__tests__/local-env-stop.test.ts b/src/__tests__/local-env-stop.test.ts new file mode 100644 index 0000000..6642adf --- /dev/null +++ b/src/__tests__/local-env-stop.test.ts @@ -0,0 +1,159 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { spawn } from 'node:child_process'; +import { mkdtempSync, readFileSync, rmSync, writeFileSync, existsSync } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; +import { stopServer, isServerRunning } from '../lib/local-env.js'; +import type { LocalInstance } from '../types.js'; + +// Reproduces the npx fallback: the pid we record is a WRAPPER whose child is the +// real server. A SIGTERM to the wrapper alone leaves the child (and its port) alive. +const WRAPPER = ` +const { spawn } = require('node:child_process'); +const fs = require('node:fs'); +const childCode = process.argv[3] === 'ignore-term' + ? "process.on('SIGTERM', () => {}); setInterval(() => {}, 1000)" + : "setInterval(() => {}, 1000)"; +// Both carry the instance's --port=, like the npx wrapper and Playground do +// ('--' so node does not parse it as its own option). +// A long token before --port=, like Playground's real argv (install paths, flags). +const child = spawn(process.execPath, ['-e', childCode, '--', 'x'.repeat(200), process.argv[2]], { stdio: 'ignore' }); +fs.writeFileSync(process.argv[1], String(child.pid)); +setInterval(() => {}, 1000); +`; + +// `local start --background` and `local stop` are separate CLI processes, so the +// server is never the stopper's own child. Mirror that: a launcher spawns the +// wrapper detached (as startServerBackground does) and exits, so the test runner +// never holds an unreaped zombie that would read as "still alive". +const LAUNCHER = ` +const { spawn } = require('node:child_process'); +const fs = require('node:fs'); +const w = spawn(process.execPath, ['-e', ${JSON.stringify(WRAPPER)}, '--', ...process.argv.slice(1, 2), ...process.argv.slice(3)], { detached: true, stdio: 'ignore' }); +fs.writeFileSync(process.argv[2], String(w.pid)); +w.unref(); +`; + +function alive(pid: number): boolean { + try { process.kill(pid, 0); return true; } catch { return false; } +} + +async function waitFor(cond: () => boolean, ms = 5000): Promise { + const end = Date.now() + ms; + while (!cond() && Date.now() < end) await new Promise(r => setTimeout(r, 50)); +} + +describe.skipIf(process.platform === 'win32')('stopServer kills the whole server tree', () => { + let dir: string; + let instance: LocalInstance; + let wrapperPid: number; + let childPid: number; + + async function launch(mode = 'normal'): Promise { + dir = mkdtempSync(join(tmpdir(), 'iwp-stop-')); + const port = 40000 + Math.floor(Math.random() * 20000); + instance = { name: 't', path: dir, port } as unknown as LocalInstance; + const childPidFile = join(dir, 'child.pid'); + const pidFile = join(dir, 'server.pid'); + await new Promise(r => spawn(process.execPath, ['-e', LAUNCHER, '--', childPidFile, pidFile, `--port=${port}`, mode], { stdio: 'ignore' }).on('exit', r)); + wrapperPid = parseInt(readFileSync(pidFile, 'utf-8'), 10); + await waitFor(() => existsSync(childPidFile) && readFileSync(childPidFile, 'utf-8').length > 0); + childPid = parseInt(readFileSync(childPidFile, 'utf-8'), 10); + expect(alive(childPid)).toBe(true); + } + + afterEach(() => { + for (const p of [childPid, wrapperPid]) { try { process.kill(p, 'SIGKILL'); } catch { /* gone */ } } + rmSync(dir, { recursive: true, force: true }); + }); + + it('stops the wrapper AND its child, and removes the pid file', async () => { + await launch(); + const t0 = Date.now(); + expect(stopServer(instance)).toBe(true); + // Synchronous: both are already gone when stopServer returns — the next + // `local start` must find the port free — and well inside the SIGKILL grace. + expect(alive(childPid)).toBe(false); + expect(alive(wrapperPid)).toBe(false); + expect(Date.now() - t0).toBeLessThan(4000); + expect(existsSync(join(dir, 'server.pid'))).toBe(false); + }); + + it('still reports running (and still stops) when only the orphaned child is left', async () => { + await launch(); + process.kill(wrapperPid, 'SIGKILL'); + await waitFor(() => !alive(wrapperPid)); + expect(isServerRunning(instance)).toBe(true); + expect(stopServer(instance)).toBe(true); + await waitFor(() => !alive(childPid)); + expect(alive(childPid)).toBe(false); + }); + it('SIGKILLs a server that ignores SIGTERM once the grace period runs out', async () => { + await launch('ignore-term'); + expect(stopServer(instance, 300)).toBe(true); + expect(alive(childPid)).toBe(false); + }); + + it('still finds --port= when ps would truncate to the terminal width (COLUMNS set)', async () => { + await launch(); + process.kill(wrapperPid, 'SIGKILL'); // only the long-argv child carries the marker now + await waitFor(() => !alive(wrapperPid)); + const prev = process.env.COLUMNS; + process.env.COLUMNS = '40'; + try { + expect(isServerRunning(instance)).toBe(true); + expect(stopServer(instance)).toBe(true); + } finally { + if (prev === undefined) delete process.env.COLUMNS; else process.env.COLUMNS = prev; + } + expect(alive(childPid)).toBe(false); + }); + + it('a process in ANOTHER group carrying the same --port= does not make this pid ours', async () => { + await launch(); + // Point the pid file at a group leader without the marker; the server with this port lives in another group. + const other = spawn(process.execPath, ['-e', 'setInterval(() => {}, 1000)'], { detached: true, stdio: 'ignore' }); + other.unref(); + try { + writeFileSync(join(dir, 'server.pid'), String(other.pid)); + expect(stopServer(instance)).toBe(false); + expect(alive(other.pid!)).toBe(true); + } finally { + process.kill(other.pid!, 'SIGKILL'); + } + }); + + it('leaves a recycled pid alone: a group without this instance\'s --port= is not ours', async () => { + await launch(); + instance = { ...instance, port: instance.port + 1 } as LocalInstance; + expect(isServerRunning(instance)).toBe(false); + writeFileSync(join(dir, 'server.pid'), String(wrapperPid)); + expect(stopServer(instance)).toBe(false); + expect(alive(childPid)).toBe(true); + expect(alive(wrapperPid)).toBe(true); + expect(existsSync(join(dir, 'server.pid'))).toBe(false); + }); +}); + +describe('stopServer pid-file guards', () => { + let dir: string; + let instance: LocalInstance; + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'iwp-stop-')); + instance = { name: 't', path: dir, port: 0 } as unknown as LocalInstance; + }); + afterEach(() => rmSync(dir, { recursive: true, force: true })); + + it('refuses pid 0 / 1 / garbage (kill(-0) would signal our own group)', () => { + for (const bad of ['0', '1', 'abc', '']) { + writeFileSync(join(dir, 'server.pid'), bad); + expect(stopServer(instance)).toBe(false); + expect(existsSync(join(dir, 'server.pid'))).toBe(false); + } + }); + + it('returns false with no pid file', () => { + expect(stopServer(instance)).toBe(false); + expect(isServerRunning(instance)).toBe(false); + }); +}); diff --git a/src/lib/local-env.ts b/src/lib/local-env.ts index ed0c3e6..cd22021 100644 --- a/src/lib/local-env.ts +++ b/src/lib/local-env.ts @@ -357,29 +357,94 @@ export async function startServerBackground( throw new Error(`Server did not become ready within 120s. Check ${logFile}`); } -export function stopServer(instance: LocalInstance): boolean { - const pidFile = join(instance.path, 'server.pid'); - if (!existsSync(pidFile)) return false; - +/** Reads server.pid; null when absent or not a usable pid (never 0/1 — `kill(-0)` is our own group). */ +function readServerPid(pidFile: string): number | null { + if (!existsSync(pidFile)) return null; const pid = parseInt(readFileSync(pidFile, 'utf-8').trim(), 10); + return Number.isInteger(pid) && pid > 1 ? pid : null; +} + +/** + * Signals the whole server tree, not just the recorded pid. The recorded pid is + * whatever we spawned — under the npx fallback that is the npx wrapper, and the + * Playground server is its CHILD, which survives a SIGTERM to the wrapper and + * keeps the port bound (EADDRINUSE on the next `local start`). The server is + * spawned `detached`, so on POSIX the recorded pid leads its own process group + * and `kill(-pid)` reaches every process in it. Windows has no groups: taskkill /T. + * Returns false when nothing was left to signal. + */ +function signalServerTree(pid: number, signal: NodeJS.Signals | 0): boolean { + if (process.platform === 'win32') { + if (signal === 0) { + try { process.kill(pid, 0); return true; } catch { return false; } + } + return spawnSync('taskkill', ['/pid', String(pid), '/T', '/F'], { stdio: 'ignore' }).status === 0; + } try { - process.kill(pid, 'SIGTERM'); + process.kill(-pid, signal); + return true; } catch { - // Already dead + // Not a group leader (pid file from something we did not spawn detached) — the pid alone. + try { process.kill(pid, signal); return true; } catch { return false; } } - rmSync(pidFile, { force: true }); - return true; } -export function isServerRunning(instance: LocalInstance): boolean { +/** + * True when the process group still contains OUR server: a process whose argv + * carries this instance's `--port=` (both the npx wrapper and Playground have it). + * The pid file outlives crashes and reboots, so after pid reuse it can name an + * unrelated group — an interactive shell leads one — which we must not SIGKILL. + * If `ps` itself is unavailable, trust the pid file (the pre-group behaviour). + */ +function groupIsOurServer(pid: number, port: number): boolean { + if (process.platform === 'win32') return true; + // -ww: ps truncates args to the terminal width (~80 cols on macOS whenever it is + // 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; + const marker = `--port=${port}`; + return ps.stdout.split('\n').some((line) => { + const m = line.match(/^\s*(\d+)\s+(.*)$/); + return m !== null && Number(m[1]) === pid && m[2].split(/\s+/).includes(marker); + }); +} + +function sleepSync(ms: number): void { + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms); +} + +export function stopServer(instance: LocalInstance, graceMs = 5000): boolean { const pidFile = join(instance.path, 'server.pid'); - if (!existsSync(pidFile)) return false; - const pid = parseInt(readFileSync(pidFile, 'utf-8').trim(), 10); - try { - process.kill(pid, 0); - return true; - } catch { + const pid = readServerPid(pidFile); + if (pid === null || !groupIsOurServer(pid, instance.port)) { rmSync(pidFile, { force: true }); return false; } + + if (signalServerTree(pid, 'SIGTERM')) { + // Wait for the tree to exit so an immediate `local start` finds the port free; + // escalate if Playground ignores SIGTERM. + const waitGone = (ms: number) => { + const deadline = Date.now() + ms; + while (signalServerTree(pid, 0) && Date.now() < deadline) sleepSync(100); + }; + waitGone(graceMs); + if (signalServerTree(pid, 0)) { + signalServerTree(pid, 'SIGKILL'); + waitGone(2000); + } + } + rmSync(pidFile, { force: true }); + return true; +} + +export function isServerRunning(instance: LocalInstance): boolean { + const pidFile = join(instance.path, 'server.pid'); + const pid = readServerPid(pidFile); + // Group liveness: a server orphaned by its npx wrapper still counts as running, + // so `local stop` can still reach it instead of the pid file being discarded. + if (pid !== null && signalServerTree(pid, 0) && groupIsOurServer(pid, instance.port)) return true; + rmSync(pidFile, { force: true }); + return false; }