fix(local stop): stop the server's process group, not just the npx wrapper

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+

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 `<instance>/server.pid`, logs at `<instance>/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

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<void> {
+  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<void> {
+    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);
+  });
+});

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;
 }