diff --git a/.changes/1306-windows-daemon-kill.md b/.changes/1306-windows-daemon-kill.md new file mode 100644 index 000000000..f63af1385 --- /dev/null +++ b/.changes/1306-windows-daemon-kill.md @@ -0,0 +1,3 @@ +### Fixed + +- **`@reticlehq/server` � `reticle kill` could fail to identify a daemon holding a port on Windows.** Port detection now handles IPv4 and IPv6 listener output from `netstat`, helping Reticle identify the process to terminate. diff --git a/server/src/command/cli/cli-kill.test.ts b/server/src/command/cli/cli-kill.test.ts index 2f2140852..785084833 100644 --- a/server/src/command/cli/cli-kill.test.ts +++ b/server/src/command/cli/cli-kill.test.ts @@ -12,19 +12,72 @@ * every platform while the signalling stays at the call site. */ -import { describe, expect, it } from 'vitest'; -import { KillAction, planKill } from './cli-kill.js'; +import { describe, expect, it, vi } from 'vitest'; +import { KillAction, planKill, runKill } from './cli-kill.js'; + +vi.mock('@/command/daemon/daemon.js', () => ({ + isAlive: () => true, + readPid: () => null, + removePid: () => undefined, +})); + +vi.mock('@/command/daemon/binding/port-presence.js', () => ({ + PortPresence: { + DAEMON: 'daemon', + FOREIGN: 'foreign', + FREE: 'free', + }, + probePresence: vi.fn(), +})); + +vi.mock('@/command/daemon/binding/daemon-status-probe.js', () => ({ + fetchStatus: vi.fn(), +})); + +vi.mock('@/surface/mcp/mcp-proxy.js', () => ({ + probeDaemon: vi.fn(), +})); + +vi.mock('./ports/port-holder.js', () => ({ + captureLookup: vi.fn(), + findPortHolder: vi.fn(), +})); + +vi.mock('@/log.js', () => ({ + log: vi.fn(), +})); + +import { probePresence, PortPresence } from '@/command/daemon/binding/port-presence.js'; +import { fetchStatus } from '@/command/daemon/binding/daemon-status-probe.js'; +import { findPortHolder } from './ports/port-holder.js'; const DAEMON_PID = 70244; const STRANGER_PID = 99001; describe('planKill', () => { - it('has nothing to do when no listener and no live recorded pid', () => { + it('has nothing to do when no listener, no recorded pid, and no daemon status', () => { expect( - planKill({ listener: null, recordedPid: null, answersStatus: false, force: false }).action, + planKill({ + listener: null, + recordedPid: null, + answersStatus: false, + force: false, + }).action, ).toBe(KillAction.NOTHING); }); + it.each([false, true])('never plans to kill without a pid (force: %s)', (force) => { + const plan = planKill({ + listener: null, + recordedPid: null, + answersStatus: true, + force, + }); + + expect(plan.action).not.toBe(KillAction.KILL); + expect(plan.pid).toBeUndefined(); + }); + it('kills the listener when it is the pid we recorded for the port', () => { const plan = planKill({ listener: { pid: DAEMON_PID, command: 'node' }, @@ -97,10 +150,40 @@ describe('planKill', () => { expect(plan.identifiedListener).toBe(false); }); - it('never plans to kill without a pid', () => { - for (const force of [false, true]) { - const plan = planKill({ listener: null, recordedPid: null, answersStatus: true, force }); - expect(plan.action).not.toBe(KillAction.KILL); - } + it('refuses when /status identifies a daemon but no listener or recorded pid is available', () => { + const plan = planKill({ + listener: null, + recordedPid: null, + answersStatus: true, + force: false, + }); + + expect(plan.action).toBe(KillAction.REFUSE); + expect(plan.pid).toBeUndefined(); + }); + + it('does not report success when the killed process exits but the port is still occupied', async () => { + vi.mocked(probePresence).mockResolvedValueOnce(PortPresence.DAEMON); + + vi.mocked(probePresence).mockResolvedValueOnce(PortPresence.FOREIGN); + + vi.mocked(findPortHolder).mockReturnValue({ + pid: DAEMON_PID, + command: 'node', + }); + + vi.mocked(fetchStatus).mockResolvedValue({ running: true, pid: DAEMON_PID }); + + const terminateProcess = vi.fn().mockResolvedValue({ + gone: true, + escalated: false, + }); + + const result = await runKill(4400, false, terminateProcess); + + expect(terminateProcess).toHaveBeenCalledWith(DAEMON_PID); + expect(fetchStatus).toHaveBeenCalledTimes(0); + expect(probePresence).toHaveBeenCalledTimes(2); + expect(result).toBe(false); }); }); diff --git a/server/src/command/cli/cli-kill.ts b/server/src/command/cli/cli-kill.ts index 7a653abb6..0c5a2d9f6 100644 --- a/server/src/command/cli/cli-kill.ts +++ b/server/src/command/cli/cli-kill.ts @@ -62,16 +62,37 @@ export function planKill(input: { recordedPid: number | null; /** Whether the port answers `/status`, i.e. whatever holds it is a Reticle daemon. */ answersStatus: boolean; + /** Whether the user has requested to force the kill. */ force: boolean; }): KillPlan { const { listener, recordedPid, answersStatus, force } = input; if (null === listener) { - // No listener identified. On Windows and in slim containers that means the lookup could not run - // rather than that the port is free, so the recorded pid is the best target we have — and the - // report says which of the two it is rather than implying a lookup we never made. - if (null === recordedPid) - return { action: KillAction.NOTHING, forced: false, identifiedListener: false }; - return { action: KillAction.KILL, pid: recordedPid, forced: false, identifiedListener: false }; + // When the listener lookup is unavailable, /status can confirm that the port belongs to Reticle, + // but without a trusted process id there is no safe process to terminate. + if (null !== recordedPid) { + return { + action: KillAction.KILL, + pid: recordedPid, + forced: false, + identifiedListener: false, + }; + } + + // The port is known to be a Reticle daemon, but without a pid there is no safe process to kill. + if (answersStatus) { + return { + action: KillAction.REFUSE, + forced: false, + identifiedListener: false, + reason: PID_UNKNOWN_REFUSAL_REASON, + }; + } + + return { + action: KillAction.NOTHING, + forced: false, + identifiedListener: false, + }; } const ours = answersStatus || listener.pid === recordedPid; if (ours || force) { @@ -145,10 +166,27 @@ async function terminate(pid: number): Promise<{ gone: boolean; escalated: boole const PROXY_SPARED_NOTE = 'only the listener was signalled. Any `reticle mcp` proxy on this port holds a client connection, ' + 'not the port, and was left alone — it goes dormant and starts a fresh daemon on the next tool call.'; +const PID_UNKNOWN_REFUSAL_REASON = + 'The port answers /status as a Reticle daemon, but Reticle could not determine its pid. ' + + 'Refusing to report success because the daemon is still running.'; + +const PID_SURVIVED_KILL_NOTE = (pid: number): string => + `pid ${String(pid)} survived SIGKILL, so it is not yours to kill (permissions) or is an ` + + `unkillable zombie. The port is still held. ${PROXY_SPARED_NOTE}`; + +const PID_EXITED_PORT_HELD_NOTE = (pid: number): string => + `pid ${String(pid)} exited, but the port is still held. ${PROXY_SPARED_NOTE}`; /** `reticle kill` — free the port. Resolves to whether the port is now free. */ -export async function runKill(port: number, force: boolean): Promise { - const presence = await probePresence(port, { tcpOpen: probeDaemon, status: fetchStatus }); +export async function runKill( + port: number, + force: boolean, + terminateProcess: typeof terminate = terminate, +): Promise { + const presence = await probePresence(port, { + tcpOpen: probeDaemon, + status: fetchStatus, + }); const recorded = readPid(port); const plan = planKill({ listener: findPortHolder(port, captureLookup), @@ -172,9 +210,44 @@ export async function runKill(port: number, force: boolean): Promise { log('reticle_kill_nothing_to_do', { port, presence }); return true; } - const { gone, escalated } = await terminate(pid); - if (gone) removePid(port); - log(gone ? 'reticle_killed' : 'reticle_kill_survived', { + + const { gone, escalated } = await terminateProcess(pid); + + if (!gone) { + log('reticle_kill_survived', { + port, + pid, + escalated, + ...(plan.forced ? { forced: true } : {}), + // A recorded PID is not necessarily the process observed listening on the port. + listenerIdentified: plan.identifiedListener, + note: PID_SURVIVED_KILL_NOTE(pid), + }); + return false; + } + + // A dead process does not by itself prove that the port is free. Another process may still + // hold the port, so verify the port state before reporting a successful kill. + const remainingPresence = await probePresence(port, { + tcpOpen: probeDaemon, + status: fetchStatus, + }); + + if (remainingPresence !== PortPresence.FREE) { + log('reticle_kill_survived', { + port, + pid, + escalated, + ...(plan.forced ? { forced: true } : {}), + listenerIdentified: plan.identifiedListener, + note: PID_EXITED_PORT_HELD_NOTE(pid), + }); + return false; + } + + removePid(port); + + log('reticle_killed', { port, pid, escalated, @@ -182,10 +255,8 @@ export async function runKill(port: number, force: boolean): Promise { // Say where the pid came from. Without `lsof` this is the pid we recorded, not the pid we // observed listening, and those are different claims. listenerIdentified: plan.identifiedListener, - note: gone - ? PROXY_SPARED_NOTE - : `pid ${String(pid)} survived SIGKILL, so it is not yours to kill (permissions) or is an ` + - `unkillable zombie. The port is still held. ${PROXY_SPARED_NOTE}`, + note: PROXY_SPARED_NOTE, }); - return gone; + + return true; } diff --git a/server/src/command/cli/ports/port-holder.test.ts b/server/src/command/cli/ports/port-holder.test.ts index fef6c222d..8d0e71882 100644 --- a/server/src/command/cli/ports/port-holder.test.ts +++ b/server/src/command/cli/ports/port-holder.test.ts @@ -2,10 +2,10 @@ import { describe, expect, it } from 'vitest'; import { describeForeignHolder, findListenerCwd, + parseNetstatPortHolder, parsePortHolder, parseProcessCwd, } from './port-holder.js'; - /** * Naming the process that holds the port. * @@ -51,6 +51,44 @@ describe('parsePortHolder', () => { }); }); +describe('parseNetstatPortHolder', () => { + it('reads the pid of a TCP listener for the requested port', () => { + const stdout = ' TCP 127.0.0.1:4400 0.0.0.0:0 LISTENING 90502\n'; + + expect(parseNetstatPortHolder(stdout, 4400)).toEqual({ + pid: 90502, + command: 'unknown', + }); + }); + + it('matches IPv6 listeners', () => { + const stdout = ' TCP [::1]:4400 [::]:0 LISTENING 90777\n'; + + expect(parseNetstatPortHolder(stdout, 4400)).toEqual({ + pid: 90777, + command: 'unknown', + }); + }); + + it('ignores listeners on other ports', () => { + const stdout = ' TCP 127.0.0.1:4500 0.0.0.0:0 LISTENING 90502\n'; + + expect(parseNetstatPortHolder(stdout, 4400)).toBeNull(); + }); + + it('ignores non-listening connections', () => { + const stdout = ' TCP 127.0.0.1:4400 127.0.0.1:5500 ESTABLISHED 90502\n'; + + expect(parseNetstatPortHolder(stdout, 4400)).toBeNull(); + }); + + it('returns null when the listener pid is invalid', () => { + const stdout = ' TCP 127.0.0.1:4400 0.0.0.0:0 LISTENING not-a-pid\n'; + + expect(parseNetstatPortHolder(stdout, 4400)).toBeNull(); + }); +}); + describe('describeForeignHolder', () => { it('names the pid and command, and does not suggest the command that kills the agent', () => { const text = describeForeignHolder(4400, { pid: 90502, command: 'node' }); diff --git a/server/src/command/cli/ports/port-holder.ts b/server/src/command/cli/ports/port-holder.ts index ba2b0d14d..9bac22c79 100644 --- a/server/src/command/cli/ports/port-holder.ts +++ b/server/src/command/cli/ports/port-holder.ts @@ -15,6 +15,7 @@ */ import { spawnSync } from 'node:child_process'; +import { parseNetstatListeners } from '../../netstat/listeners.js'; export interface PortHolder { pid: number; @@ -129,6 +130,12 @@ export function describeForeignHolder( ); } +/** Find the first TCP listener for a port in `netstat -ano` output. */ +export function parseNetstatPortHolder(stdout: string, port: number): PortHolder | null { + const listener = parseNetstatListeners(stdout).find((row) => row.port === port); + return listener === undefined ? null : { pid: listener.pid, command: 'unknown' }; +} + /** The lookup itself. `exec` is injected so the shelling out never runs in a unit test. */ export function findPortHolder( port: number, @@ -138,7 +145,12 @@ export function findPortHolder( // that matters: without it this also reports the agent's own MCP proxy, which is a client of the // port, not its owner. const out = exec('lsof', ['-nP', `-iTCP:${String(port)}`, '-sTCP:LISTEN', '-F', 'pc']); - return null === out ? null : parsePortHolder(out); + if (null !== out) return parsePortHolder(out); + + // Windows does not normally have lsof. netstat is built into Windows and reports the owning PID + // for listening TCP sockets, giving kill a process identity tied to the actual port owner. + const netstat = exec('netstat', ['-ano']); + return null === netstat ? null : parseNetstatPortHolder(netstat, port); } /** `lsof -Fn` emits the file name on its own line, prefixed `n`. */ diff --git a/server/src/command/netstat/listeners.ts b/server/src/command/netstat/listeners.ts new file mode 100644 index 000000000..dd00c62b9 --- /dev/null +++ b/server/src/command/netstat/listeners.ts @@ -0,0 +1,37 @@ +/** One TCP listener and the process that owns it. */ +export interface NetstatListener { + readonly port: number; + readonly pid: number; +} + +const NETSTAT_PROTO = 'TCP'; +const NETSTAT_STATE = 'LISTENING'; +const NETSTAT_MIN_COLUMNS = 5; + +/** + * Parse Windows `netstat -ano` output. + * + * Read the port after the LAST colon so IPv6 addresses such as `[::]:3000` + * are handled correctly. Do not restrict netstat to IPv4 with `-p tcp`. + */ +export function parseNetstatListeners(out: string): NetstatListener[] { + const rows: NetstatListener[] = []; + + for (const line of String(out).split(/\r?\n/)) { + const fields = line.trim().split(/\s+/); + if (fields.length < NETSTAT_MIN_COLUMNS) continue; + if (fields[0]?.toUpperCase() !== NETSTAT_PROTO || fields[3]?.toUpperCase() !== NETSTAT_STATE) { + continue; + } + + const localAddress = fields[1] ?? ''; + const port = Number(localAddress.slice(localAddress.lastIndexOf(':') + 1)); + const pid = Number(fields[4]); + + if (Number.isInteger(port) && port > 0 && Number.isInteger(pid) && pid > 0) { + rows.push({ port, pid }); + } + } + + return rows; +} diff --git a/server/src/command/setup/terminal/listeners.ts b/server/src/command/setup/terminal/listeners.ts index 4c2b06633..23f85d62b 100644 --- a/server/src/command/setup/terminal/listeners.ts +++ b/server/src/command/setup/terminal/listeners.ts @@ -1,3 +1,5 @@ +export { parseNetstatListeners } from '../../netstat/listeners.js'; + /** * Which ports a process tree is listening on, parsed from the tools that can answer that. * @@ -8,12 +10,6 @@ * platform's most common React setup. */ -/** One LISTENING socket and the process that owns it. */ -interface Listener { - readonly port: number; - readonly pid: number; -} - /** `lsof -a -p -iTCP -sTCP:LISTEN -P -n`, NAME column only. */ export function parseLsofPorts(out: string): number[] { const ports = new Set(); @@ -21,30 +17,6 @@ export function parseLsofPorts(out: string): number[] { return [...ports]; } -const NETSTAT_PROTO = 'TCP'; -const NETSTAT_STATE = 'LISTENING'; -const NETSTAT_MIN_COLUMNS = 5; - -/** - * `netstat -ano` on Windows. - * - * The port is whatever follows the LAST colon: `[::]:3000` has three of them, so splitting on ':' - * and taking index 1 reads every IPv6 row as port 0 and silently drops half the table. - */ -export function parseNetstatListeners(out: string): Listener[] { - const rows: Listener[] = []; - for (const line of String(out).split(/\r?\n/)) { - const f = line.trim().split(/\s+/); - if (f.length < NETSTAT_MIN_COLUMNS) continue; - if (f[0]?.toUpperCase() !== NETSTAT_PROTO || f[3]?.toUpperCase() !== NETSTAT_STATE) continue; - const local = f[1] ?? ''; - const port = Number(local.slice(local.lastIndexOf(':') + 1)); - const pid = Number(f[4]); - if (Number.isInteger(port) && port > 0 && Number.isInteger(pid)) rows.push({ port, pid }); - } - return rows; -} - /** * One (pid, parent) pair, as `wmic process get CreationDate,ParentProcessId,ProcessId` prints them. * diff --git a/server/src/directory-reach.test.ts b/server/src/directory-reach.test.ts index 84fe9b214..21d3f776f 100644 --- a/server/src/directory-reach.test.ts +++ b/server/src/directory-reach.test.ts @@ -280,7 +280,7 @@ const REACHES_FOR: Record = { * parts of a program ask is a thing, and it was seven files in the middle of the CLI. It * reaches for nothing itself. */ - ports: [], + ports: ['netstat'], /** * Talking to the person at the terminal: asking, interrupting, showing progress, recording what * was said. Named `terminal` and not `console`, which in a JavaScript codebase means the API. @@ -288,7 +288,9 @@ const REACHES_FOR: Record = { * It reaches for four things and that is honest -- it prints daemon state, port state and * telemetry notices, because that is what a setup transcript is made of. */ - terminal: ['binding', 'daemon', 'launch', 'resolve', 'telemetry'], + terminal: ['binding', 'daemon', 'launch', 'netstat', 'resolve', 'telemetry'], + /** Shared Windows netstat listener parsing. */ + netstat: [], /** A recorded flow and what became of it: the tape, the rewind, the flake, the halt. */ recording: [], /** What a human wrote on a step, and where they pointed when they wrote it. */ diff --git a/server/src/index.ts b/server/src/index.ts index 45ccf8a62..dda84264f 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -405,6 +405,7 @@ export async function startDaemon(options: StartOptions = {}): Promise