diff --git a/src/lib/tunnel/services.test.ts b/src/lib/tunnel/services.test.ts index d863f7ae07..c2d666e016 100644 --- a/src/lib/tunnel/services.test.ts +++ b/src/lib/tunnel/services.test.ts @@ -22,6 +22,7 @@ import { resolveDefaultSandboxName } from "./service-command"; import { getServiceStatuses, getTunnelUrl, + type ProcessControl, readCloudflaredState, showStatus, startAll, @@ -479,6 +480,89 @@ describe("stopAll", () => { rmSync(pidDir, { recursive: true, force: true }); }); + // A scripted ProcessControl models PID identity/liveness/signalling without + // touching the host, so the recycled-PID paths are deterministic and portable + // (no real process, no /proc, no signals). `alive`/`cmdlines` are consumed in + // call order, repeating the last entry. + function scriptedControl(script: { alive: boolean[]; cmdlines: Array }): { + control: ProcessControl; + signals: Array<{ pid: number; sig: string }>; + } { + const signals: Array<{ pid: number; sig: string }> = []; + let aliveIdx = 0; + let cmdIdx = 0; + const control: ProcessControl = { + isAlive: () => script.alive[Math.min(aliveIdx++, script.alive.length - 1)], + commandLine: () => script.cmdlines[Math.min(cmdIdx++, script.cmdlines.length - 1)], + signal: (pid, sig) => { + signals.push({ pid, sig }); + }, + }; + return { control, signals }; + } + + it("does not signal a live PID recycled to a non-cloudflared process", () => { + const { control, signals } = scriptedControl({ + alive: [true], + cmdlines: ["/usr/bin/node vitest"], + }); + writeFileSync(join(pidDir, "cloudflared.pid"), "4242", { mode: 0o600 }); + + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + try { + stopAll({ pidDir, processControl: control }); + } finally { + logSpy.mockRestore(); + } + + expect(signals).toEqual([]); + expect(existsSync(join(pidDir, "cloudflared.pid"))).toBe(false); + }); + + it("does not escalate to SIGKILL when the PID is recycled during the poll", () => { + const { control, signals } = scriptedControl({ + // Alive pre-SIGTERM; the poll observes exit; a live PID reappears at the + // pre-SIGKILL re-check. + alive: [true, false, true], + // Ours pre-SIGTERM, then recycled to a bystander before escalation. + cmdlines: ["cloudflared tunnel run", "/usr/bin/node vitest"], + }); + writeFileSync(join(pidDir, "cloudflared.pid"), "4242", { mode: 0o600 }); + + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + try { + stopAll({ pidDir, processControl: control }); + } finally { + logSpy.mockRestore(); + } + + expect(signals.map((entry) => entry.sig)).toEqual(["SIGTERM"]); + expect(existsSync(join(pidDir, "cloudflared.pid"))).toBe(false); + }); + + it("escalates to SIGKILL when cloudflared remains live after the grace period (#7644)", () => { + const { control, signals } = scriptedControl({ + alive: [true, true], + cmdlines: ["cloudflared tunnel run", "cloudflared tunnel run"], + }); + writeFileSync(join(pidDir, "cloudflared.pid"), "4242", { mode: 0o600 }); + + const nowSpy = vi.spyOn(Date, "now").mockReturnValueOnce(0).mockReturnValue(3000); + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + try { + stopAll({ pidDir, processControl: control }); + } finally { + nowSpy.mockRestore(); + logSpy.mockRestore(); + } + + expect(signals).toEqual([ + { pid: 4242, sig: "SIGTERM" }, + { pid: 4242, sig: "SIGKILL" }, + ]); + expect(existsSync(join(pidDir, "cloudflared.pid"))).toBe(false); + }); + it("removes stale PID files", () => { writeFileSync(join(pidDir, "cloudflared.pid"), "999999999"); diff --git a/src/lib/tunnel/services.ts b/src/lib/tunnel/services.ts index 8e80ae2ab9..1528a0bb85 100644 --- a/src/lib/tunnel/services.ts +++ b/src/lib/tunnel/services.ts @@ -41,6 +41,8 @@ export interface ServiceOptions { repoDir?: string; /** Override PID directory (default: /tmp/nemoclaw-services-{sandbox}). */ pidDir?: string; + /** Injectable process operations (identity + signalling) for tests. */ + processControl?: ProcessControl; /** Cloudflare named tunnel token. Falls back to CLOUDFLARE_TUNNEL_TOKEN. */ cloudflareTunnelToken?: string; /** Also release the managed host gateway port (legacy full-stop only). */ @@ -141,6 +143,23 @@ function commandLineNamesCloudflared(commandLine: string): boolean { .some((token) => basename(token) === "cloudflared"); } +// Process operations behind a small seam so lifecycle tests can model PID +// reuse deterministically (per the tunnel adapter/fake test convention) +// instead of spawning real processes or reading /proc. +export interface ProcessControl { + isAlive(pid: number): boolean; + commandLine(pid: number): string | null; + signal(pid: number, sig: NodeJS.Signals): void; +} + +const REAL_PROCESS_CONTROL: ProcessControl = { + isAlive, + commandLine: readProcessCommandLine, + signal: (pid, sig) => { + process.kill(pid, sig); + }, +}; + function extractTryCloudflareUrl(log: string): string | null { for (const rawToken of log.split(/\s+/)) { const candidate = rawToken.replace(/^[<("']+|[>),."']+$/g, ""); @@ -335,15 +354,33 @@ function startService( info(`${name} started (PID ${String(pid)})`); } +/** + * The recorded process may have exited and had its PID recycled by the OS to an + * unrelated (possibly system) process. Signalling it would terminate a + * bystander, so only report a live PID as ours when its command line still + * names cloudflared. A null/unreadable command line stays conservative and is + * treated as ours, matching readCloudflaredState. + */ +function pidIsOurs(pid: number, pc: ProcessControl): boolean { + const cmdline = pc.commandLine(pid); + return cmdline === null || commandLineNamesCloudflared(cmdline); +} + /** Poll for process exit after SIGTERM, escalate to SIGKILL if needed. */ -function stopService(pidDir: string, name: ServiceName): void { +function stopService( + pidDir: string, + name: ServiceName, + pc: ProcessControl = REAL_PROCESS_CONTROL, +): void { const pid = readPid(pidDir, name); if (pid === null) { info(`${name} was not running`); return; } - if (!isAlive(pid)) { + // A dead PID, or a live PID recycled to a non-cloudflared process, means our + // service is not running. Drop the stale pid file without signalling. + if (!pc.isAlive(pid) || !pidIsOurs(pid, pc)) { info(`${name} was not running`); removePid(pidDir, name); return; @@ -351,7 +388,7 @@ function stopService(pidDir: string, name: ServiceName): void { // Send SIGTERM try { - process.kill(pid, "SIGTERM"); + pc.signal(pid, "SIGTERM"); } catch { // Already dead between the check and the signal removePid(pidDir, name); @@ -361,7 +398,7 @@ function stopService(pidDir: string, name: ServiceName): void { // Poll for exit (up to 3 seconds) const deadline = Date.now() + 3000; - while (Date.now() < deadline && isAlive(pid)) { + while (Date.now() < deadline && pc.isAlive(pid)) { // Busy-wait in 100ms increments (synchronous — matches stop being sync) const start = Date.now(); while (Date.now() - start < 100) { @@ -369,10 +406,16 @@ function stopService(pidDir: string, name: ServiceName): void { } } - // Escalate to SIGKILL if still alive - if (isAlive(pid)) { + // Escalate to SIGKILL if still alive. Re-verify identity first: the PID could + // have exited and been recycled to an unrelated process during the poll. + if (pc.isAlive(pid)) { + if (!pidIsOurs(pid, pc)) { + removePid(pidDir, name); + info(`${name} was not running`); + return; + } try { - process.kill(pid, "SIGKILL"); + pc.signal(pid, "SIGKILL"); } catch { /* already dead */ } @@ -487,7 +530,7 @@ export function stopAll(opts: ServiceOptions = {}): void { // derived from a trusted sandbox name. An invalid requested sandbox must not // fall through to the default sandbox's PID directory. if (pidDir) { - stopService(pidDir, "cloudflared"); + stopService(pidDir, "cloudflared", opts.processControl ?? REAL_PROCESS_CONTROL); } else { warn("Invalid sandbox name without an explicit PID directory; skipping host service stop."); } diff --git a/test/host-readiness-station-release-marker.test.ts b/test/host-readiness-station-release-marker.test.ts index 3a6a9e1169..32263dbba3 100644 --- a/test/host-readiness-station-release-marker.test.ts +++ b/test/host-readiness-station-release-marker.test.ts @@ -11,6 +11,7 @@ import { assessHost } from "../src/lib/onboard/preflight"; import { createHostReadinessReport } from "../src/lib/readiness/host"; const NOW = new Date("2026-07-30T12:00:00Z"); +const SOURCE_REVISION = "21e60ae287e8c2a184f71406ac8b418f046330d1"; const DOCKER_INFO = JSON.stringify({ CgroupVersion: "2", Driver: "overlay2", @@ -71,7 +72,7 @@ function createStationFixture(marker: "regular-file" | "symbolic-link"): string function reportForStationHost(root: string) { return createHostReadinessReport( - { nemoclawVersion: "0.0.0-test", now: () => NOW }, + { nemoclawVersion: "0.0.0-test", sourceRevision: SOURCE_REVISION, now: () => NOW }, { architecture: "arm64", assess: () =>