From 2ae67fb6fc728ae961e6be821bf9f6b95fcfdb72 Mon Sep 17 00:00:00 2001 From: latenighthackathon Date: Mon, 27 Jul 2026 14:52:16 +0000 Subject: [PATCH 1/4] fix(tunnel): verify process identity before stopping cloudflared stopService signalled whatever PID was recorded in cloudflared.pid after only an isAlive check. If the recorded cloudflared had exited and the OS recycled its PID to an unrelated process, that process was SIGTERM/ SIGKILLed. cloudflared is spawned detached and can exit early (for example on an invalid tunnel token), leaving a stale pid file, so PID reuse is a realistic path, and under an elevated topology the recycled PID can be a system process. Every other kill site in this module already gates on a command-line identity match; stopService was the lone unverified killer. Verify the live PID still names cloudflared (the same readProcessCommandLine plus commandLineNamesCloudflared check readCloudflaredState uses) before signalling, and drop the stale pid file when it does not. A null or unreadable cmdline stays conservative and proceeds, matching readCloudflaredState. Signed-off-by: latenighthackathon --- src/lib/tunnel/services.test.ts | 38 +++++++++++++++++++++++++++++++++ src/lib/tunnel/services.ts | 11 ++++++++++ 2 files changed, 49 insertions(+) diff --git a/src/lib/tunnel/services.test.ts b/src/lib/tunnel/services.test.ts index d863f7ae070..d12fb6abbac 100644 --- a/src/lib/tunnel/services.test.ts +++ b/src/lib/tunnel/services.test.ts @@ -479,6 +479,44 @@ describe("stopAll", () => { rmSync(pidDir, { recursive: true, force: true }); }); + it("does not signal a recycled PID whose process is not cloudflared", () => { + // A real bystander process standing in for a PID the OS recycled after the + // recorded cloudflared exited. It must not be SIGTERM/SIGKILLed. + const bystander = childProcess.spawn("sleep", ["30"], { + detached: true, + stdio: "ignore", + }); + bystander.unref(); + const bpid = bystander.pid as number; + expect(bpid).toBeGreaterThan(0); + writeFileSync(join(pidDir, "cloudflared.pid"), String(bpid), { mode: 0o600 }); + + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + stopAll({ pidDir }); + logSpy.mockRestore(); + + // A SIGTERM/SIGKILLed child becomes an unreaped zombie whose PID still + // answers `kill(pid, 0)`, so assert on the process state: a live sleep is + // sleeping (S), a signalled one is a zombie (Z) or gone. + let liveState: string | null = null; + try { + const stat = readFileSync(`/proc/${bpid}/stat`, "utf-8"); + const match = stat.match(/\) (\S)/); + liveState = match ? match[1] : null; + } catch { + liveState = null; + } + try { + process.kill(bpid, "SIGKILL"); + } catch { + /* already gone */ + } + + expect(liveState).not.toBeNull(); + expect(liveState).not.toBe("Z"); + 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 8e80ae2ab97..757d6261e8c 100644 --- a/src/lib/tunnel/services.ts +++ b/src/lib/tunnel/services.ts @@ -349,6 +349,17 @@ function stopService(pidDir: string, name: ServiceName): void { return; } + // 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 verify the live PID still names cloudflared before signalling + // (same cmdline check readCloudflaredState uses). Drop the stale pid file. + const cmdline = readProcessCommandLine(pid); + if (cmdline !== null && !commandLineNamesCloudflared(cmdline)) { + info(`${name} was not running`); + removePid(pidDir, name); + return; + } + // Send SIGTERM try { process.kill(pid, "SIGTERM"); From 5468a3b04a0c10f9238d12d795bf353aef857b67 Mon Sep 17 00:00:00 2001 From: latenighthackathon Date: Mon, 27 Jul 2026 18:49:02 +0000 Subject: [PATCH 2/4] fix(tunnel): re-verify cloudflared identity before SIGKILL and fake process ops in tests Address review feedback on the recycled-PID stop fix. Re-run the command-line identity check immediately before the SIGKILL escalation, not only before SIGTERM: cloudflared can exit and have its PID recycled during the up-to-3s exit poll, so the escalation could otherwise kill an unrelated process. Route stopService process operations (isAlive / commandLine / signal) through an injectable ProcessControl. The regression tests now drive a deterministic fake that models PID reuse instead of spawning a real process and reading /proc//stat, which was not portable to the macOS Vitest runner and did not follow the tunnel adapter/fake test convention. Adds coverage for both the recycled-before-SIGTERM and recycled-during-poll paths. Signed-off-by: latenighthackathon --- src/lib/tunnel/services.test.ts | 77 +++++++++++++++++++++------------ src/lib/tunnel/services.ts | 70 ++++++++++++++++++++++-------- 2 files changed, 101 insertions(+), 46 deletions(-) diff --git a/src/lib/tunnel/services.test.ts b/src/lib/tunnel/services.test.ts index d12fb6abbac..c34c50cb1d4 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,41 +480,63 @@ describe("stopAll", () => { rmSync(pidDir, { recursive: true, force: true }); }); - it("does not signal a recycled PID whose process is not cloudflared", () => { - // A real bystander process standing in for a PID the OS recycled after the - // recorded cloudflared exited. It must not be SIGTERM/SIGKILLed. - const bystander = childProcess.spawn("sleep", ["30"], { - detached: true, - stdio: "ignore", + // 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"], }); - bystander.unref(); - const bpid = bystander.pid as number; - expect(bpid).toBeGreaterThan(0); - writeFileSync(join(pidDir, "cloudflared.pid"), String(bpid), { mode: 0o600 }); + writeFileSync(join(pidDir, "cloudflared.pid"), "4242", { mode: 0o600 }); const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); - stopAll({ pidDir }); - logSpy.mockRestore(); - - // A SIGTERM/SIGKILLed child becomes an unreaped zombie whose PID still - // answers `kill(pid, 0)`, so assert on the process state: a live sleep is - // sleeping (S), a signalled one is a zombie (Z) or gone. - let liveState: string | null = null; try { - const stat = readFileSync(`/proc/${bpid}/stat`, "utf-8"); - const match = stat.match(/\) (\S)/); - liveState = match ? match[1] : null; - } catch { - liveState = null; + 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 { - process.kill(bpid, "SIGKILL"); - } catch { - /* already gone */ + stopAll({ pidDir, processControl: control }); + } finally { + logSpy.mockRestore(); } - expect(liveState).not.toBeNull(); - expect(liveState).not.toBe("Z"); + expect(signals.map((entry) => entry.sig)).toEqual(["SIGTERM"]); expect(existsSync(join(pidDir, "cloudflared.pid"))).toBe(false); }); diff --git a/src/lib/tunnel/services.ts b/src/lib/tunnel/services.ts index 757d6261e8c..1528a0bb85f 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,26 +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)) { - info(`${name} was not running`); - removePid(pidDir, name); - return; - } - - // 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 verify the live PID still names cloudflared before signalling - // (same cmdline check readCloudflaredState uses). Drop the stale pid file. - const cmdline = readProcessCommandLine(pid); - if (cmdline !== null && !commandLineNamesCloudflared(cmdline)) { + // 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; @@ -362,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); @@ -372,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) { @@ -380,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 */ } @@ -498,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."); } From 1ce1817912bf9eb73eb63b21af55225fc8a39cb4 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Fri, 31 Jul 2026 04:25:57 -0700 Subject: [PATCH 3/4] test(tunnel): cover persistent stop escalation Signed-off-by: Apurv Kumaria --- src/lib/tunnel/services.test.ts | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/lib/tunnel/services.test.ts b/src/lib/tunnel/services.test.ts index c34c50cb1d4..c2d666e016b 100644 --- a/src/lib/tunnel/services.test.ts +++ b/src/lib/tunnel/services.test.ts @@ -540,6 +540,29 @@ describe("stopAll", () => { 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"); From 5700f6a22d2dd791b63bba7ddd7cf460800f1299 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Fri, 31 Jul 2026 08:50:42 -0700 Subject: [PATCH 4/4] test(readiness): include source revision fixture Signed-off-by: Apurv Kumaria --- test/host-readiness-station-release-marker.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/host-readiness-station-release-marker.test.ts b/test/host-readiness-station-release-marker.test.ts index 3a6a9e11694..32263dbba30 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: () =>