From 9c7fd170946bd3cae9750d145f7559b7e4248f80 Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Mon, 17 Aug 2026 14:32:15 -0400 Subject: [PATCH 1/3] fix(cli): keep a live preview's ownership record and stop past a bad one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A missed liveness probe is not proof the preview is gone — a server blocked on a Puppeteer capture answers nothing for a second or two — but any miss retired the session record, and the record carries the only PID-reuse guard `--stop` has. Reproduced by SIGSTOPping a managed preview and running `--status`: the record was deleted and never came back, leaving every later stop to fall through to an unauthenticated port scan with no ownership proof at all. Only a wrapper process that is provably gone now retires a record. That record gains a process-birth token so a recycled PID reads as a different process, and it is written through a temp file and renamed — every reader deletes it when it fails to parse, so a torn read would otherwise destroy a live server's proof of ownership. Two failure-propagation bugs in the stop path: `--kill-all` collected the first unprovable record's exception and abandoned every server after it, so they were left running AND unreported; and a replacement refused to launch when the server it was replacing had already exited on its own, which is the goal state rather than a failure. `--list` now shows managed sessions ahead of whatever else answers the scan. --- packages/cli/src/commands/preview.test.ts | 75 ++- packages/cli/src/commands/preview.ts | 128 ++++-- .../cli/src/commands/previewLifecycle.test.ts | 430 +++++++++++++++++- packages/cli/src/commands/previewLifecycle.ts | 277 +++++++++-- 4 files changed, 834 insertions(+), 76 deletions(-) diff --git a/packages/cli/src/commands/preview.test.ts b/packages/cli/src/commands/preview.test.ts index 38b02f58e6..f35a7e4680 100644 --- a/packages/cli/src/commands/preview.test.ts +++ b/packages/cli/src/commands/preview.test.ts @@ -1,8 +1,9 @@ import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; -import { join } from "node:path"; -import { afterEach, describe, expect, it } from "vitest"; -import { studioLandingSearch } from "./preview.js"; +import { join, resolve } from "node:path"; +import * as clack from "@clack/prompts"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { handlePreviewKillAll, handlePreviewList, studioLandingSearch } from "./preview.js"; const tempDirs: string[] = []; @@ -51,3 +52,71 @@ describe("studioLandingSearch", () => { expect(studioLandingSearch(dir)).toBe(""); }); }); + +describe("preview --kill-all", () => { + const session = (port: number, projectDir: string) => ({ + pid: 4321, + port, + projectDir, + logPath: `${projectDir}.log`, + }); + + it("keeps stopping after a record whose ownership cannot be proven", async () => { + // Propagating the first failure left every later preview running AND + // unreported — the one thing a stop pass must never do. + const log = vi.spyOn(console, "log").mockImplementation(() => {}); + const warn = vi.spyOn(clack.log, "warn").mockImplementation(() => {}); + + await handlePreviewKillAll(3002, { + listManaged: async () => [session(41402, "/tmp/unprovable"), session(41403, "/tmp/healthy")], + stopManaged: async (projectDir) => { + if (projectDir === "/tmp/unprovable") throw new Error("ownership failed"); + return true; + }, + killScanned: async () => ({ killed: 0, unverified: [] }), + }); + + expect(log.mock.calls.flat().join("\n")).toContain("Killed 1 preview server"); + expect(warn.mock.calls.flat().join("\n")).toContain("/tmp/unprovable: ownership failed"); + log.mockRestore(); + warn.mockRestore(); + }); + + it("reports nothing to kill when no preview is running", async () => { + const log = vi.spyOn(console, "log").mockImplementation(() => {}); + + await handlePreviewKillAll(3002, { + listManaged: async () => [], + killScanned: async () => ({ killed: 0, unverified: [] }), + }); + + expect(log.mock.calls.flat().join("\n")).toContain("No active preview servers to kill"); + log.mockRestore(); + }); +}); + +describe("preview --list", () => { + it("prefers the managed record over the same server's own self-report", async () => { + const log = vi.spyOn(console, "log").mockImplementation(() => {}); + + await handlePreviewList(3002, { + listManaged: async () => [ + { pid: 99, port: 3002, projectDir: resolve("/tmp/demo"), logPath: "/tmp/demo.log" }, + ], + scan: async () => [ + { + port: 3002, + projectName: "demo", + projectDir: resolve("/tmp/demo"), + version: "test", + pid: "99", + }, + ], + }); + + const printed = log.mock.calls.flat().join("\n"); + expect(printed).toContain("1 server running"); + expect(printed).toContain("PID 99"); + log.mockRestore(); + }); +}); diff --git a/packages/cli/src/commands/preview.ts b/packages/cli/src/commands/preview.ts index 826c8af05e..85c306ef40 100644 --- a/packages/cli/src/commands/preview.ts +++ b/packages/cli/src/commands/preview.ts @@ -65,6 +65,7 @@ import { resolveProject } from "../utils/project.js"; import { resolveAutoProxy } from "../utils/projectConfig.js"; import { studioProxyEnv } from "../utils/studioProxyEnv.js"; import { + listBackgroundPreviewStatuses, readBackgroundPreviewStatus, startBackgroundPreview, stopBackgroundPreview, @@ -239,40 +240,13 @@ export default defineCommand({ // --list: scan and display active servers if (args.list) { - const servers = await scanActiveServers(startPort); - if (servers.length === 0) { - console.log("\n No active preview servers found.\n"); - return; - } - console.log(`\n ${c.bold("Active preview servers:")}\n`); - for (const s of servers) { - const pidStr = s.pid ? c.dim(` (PID ${s.pid})`) : ""; - console.log( - ` ${c.accent(`Port ${s.port}`)} ${s.projectName} ${c.dim(s.projectDir)}${pidStr}`, - ); - } - console.log(`\n ${servers.length} server${servers.length === 1 ? "" : "s"} running.\n`); + await handlePreviewList(startPort); return; } // --kill-all: kill all active servers if (args["kill-all"]) { - const servers = await scanActiveServers(startPort); - if (servers.length === 0) { - console.log("\n No active preview servers to kill.\n"); - return; - } - const { killed, unverified } = await killActiveServers(startPort); - console.log(`\n Killed ${killed} preview server${killed === 1 ? "" : "s"}.`); - if (unverified.length > 0) { - clack.log.warn( - `Left ${unverified.length} server${unverified.length === 1 ? "" : "s"} alone ` + - `(port${unverified.length === 1 ? "" : "s"} ${unverified.join(", ")}): the OS could ` + - `not confirm which process owns the socket, and the server's own claim is not proof. ` + - `Install lsof, or stop it with its own preview --stop.`, - ); - } - console.log(); + await handlePreviewKillAll(startPort); return; } @@ -447,6 +421,102 @@ export default defineCommand({ }, }); +interface PreviewActionDependencies { + scan?: typeof scanActiveServers; + listManaged?: typeof listBackgroundPreviewStatuses; + stopManaged?: typeof stopBackgroundPreview; + killScanned?: typeof killActiveServers; +} + +/** + * Managed previews first, then anything else answering on the scanned range. + * A managed session is the authoritative entry for its project and port — the + * scan would otherwise list the same server again from its own self-report. + */ +export async function handlePreviewList( + startPort: number, + dependencies: PreviewActionDependencies = {}, +): Promise { + const [scannedServers, managedSessions] = await Promise.all([ + (dependencies.scan ?? scanActiveServers)(startPort), + (dependencies.listManaged ?? listBackgroundPreviewStatuses)(), + ]); + const managedKeys = new Set( + managedSessions.map((session) => `${resolve(session.projectDir)}\0${session.port}`), + ); + const servers = [ + ...managedSessions.map((session) => ({ + port: session.port, + projectName: basename(session.projectDir), + projectDir: session.projectDir, + pid: String(session.pid), + })), + ...scannedServers.filter( + (server) => !managedKeys.has(`${resolve(server.projectDir)}\0${server.port}`), + ), + ]; + if (servers.length === 0) { + console.log("\n No active preview servers found.\n"); + return; + } + console.log(`\n ${c.bold("Active preview servers:")}\n`); + for (const server of servers) { + const pid = server.pid ? c.dim(` (PID ${server.pid})`) : ""; + console.log( + ` ${c.accent(`Port ${server.port}`)} ${server.projectName} ${c.dim(server.projectDir)}${pid}`, + ); + } + console.log(`\n ${servers.length} server${servers.length === 1 ? "" : "s"} running.\n`); +} + +/** + * Stop every managed preview through its ownership record, then sweep whatever + * else is still listening. + * + * Per-record failures are collected rather than propagated: one record whose + * ownership cannot be proven must not abandon the servers after it, which would + * leave them running AND unreported. + */ +export async function handlePreviewKillAll( + startPort: number, + dependencies: PreviewActionDependencies = {}, +): Promise { + const managedSessions = await (dependencies.listManaged ?? listBackgroundPreviewStatuses)(); + let killed = 0; + const failures: string[] = []; + for (const session of managedSessions) { + try { + if ( + await (dependencies.stopManaged ?? stopBackgroundPreview)(session.projectDir, session.port) + ) { + killed++; + } + } catch (error) { + failures.push(`${session.projectDir}: ${error instanceof Error ? error.message : error}`); + } + } + const swept = await (dependencies.killScanned ?? killActiveServers)(startPort); + killed += swept.killed; + if (killed > 0) { + console.log(`\n Killed ${killed} preview server${killed === 1 ? "" : "s"}.`); + } else if (failures.length === 0 && swept.unverified.length === 0) { + console.log("\n No active preview servers to kill."); + } + for (const failure of failures) clack.log.warn(`Could not stop ${failure}`); + if (swept.unverified.length > 0) { + // Fail-closed, said out loud: a security control that degrades silently is + // one nobody knows to fix. + const ports = swept.unverified.join(", "); + const plural = swept.unverified.length === 1 ? "" : "s"; + clack.log.warn( + `Left ${swept.unverified.length} server${plural} alone (port${plural} ${ports}): the OS ` + + `could not confirm which process owns the socket, and the server's own claim is not ` + + `proof. Install lsof, or stop it with its own preview --stop.`, + ); + } + console.log(); +} + // `host` is the loopback the server actually bound (Vite binds `[::1]`, embedded // binds `127.0.0.1`); default to IPv4 for the embedded/legacy callers. function previewBaseUrl(port: number, host = "127.0.0.1"): string { diff --git a/packages/cli/src/commands/previewLifecycle.test.ts b/packages/cli/src/commands/previewLifecycle.test.ts index 1f0fbdb0aa..e2a26c91b9 100644 --- a/packages/cli/src/commands/previewLifecycle.test.ts +++ b/packages/cli/src/commands/previewLifecycle.test.ts @@ -1,10 +1,11 @@ -import { existsSync, mkdtempSync } from "node:fs"; +import { existsSync, mkdtempSync, readFileSync, readdirSync } from "node:fs"; import { tmpdir } from "node:os"; import { join, resolve } from "node:path"; import { describe, expect, it, vi } from "vitest"; import type { ActiveServer } from "../server/portUtils.js"; import { buildBackgroundPreviewArgs, + listBackgroundPreviewStatuses, previewSessionPath, readBackgroundPreviewStatus, startBackgroundPreview, @@ -48,7 +49,7 @@ describe("background preview lifecycle", () => { ); }); - it("builds a detached child invocation without recursively preserving --background", () => { + it("does not let the detached child inherit launcher-only flags", () => { expect( buildBackgroundPreviewArgs([ "/opt/hyperframes/cli.js", @@ -76,6 +77,77 @@ describe("background preview lifecycle", () => { expect(spawn).not.toHaveBeenCalled(); }); + it("reuses a saved managed preview on a custom port without repeating --port", async () => { + const stateHome = mkdtempSync(join(tmpdir(), "hf-preview-state-")); + writePreviewSession( + { pid: 4321, port: 41402, projectDir, logPath: "/tmp/custom.log" }, + stateHome, + ); + const customServer = { ...server, port: 41402, browserGpuMode: "software" as const }; + const scan = vi.fn(async (startPort?: number) => (startPort === 41402 ? [customServer] : [])); + const spawn = vi.fn(); + + const result = await startBackgroundPreview(projectDir, 3002, { + scan, + spawn, + stateHome, + }); + + expect(result).toMatchObject({ type: "reused", port: 41402, pid: 4321 }); + expect(scan).toHaveBeenCalledWith(41402); + expect(spawn).not.toHaveBeenCalled(); + }); + + it("discovers managed previews outside the default port scan and removes stale records", async () => { + const stateHome = mkdtempSync(join(tmpdir(), "hf-preview-state-")); + const otherProjectDir = resolve("/tmp/hyperframes-preview-managed-custom-port"); + const staleProjectDir = resolve("/tmp/hyperframes-preview-managed-stale"); + writePreviewSession( + { + pid: 8765, + port: 41402, + projectDir: otherProjectDir, + logPath: "/tmp/custom.log", + }, + stateHome, + ); + writePreviewSession( + { + pid: 9999, + port: 45000, + projectDir: staleProjectDir, + logPath: "/tmp/stale.log", + }, + stateHome, + ); + + const statuses = await listBackgroundPreviewStatuses({ + stateHome, + scan: async (startPort) => + startPort === 41402 + ? [ + { + port: 41402, + projectName: "managed-custom-port", + projectDir: otherProjectDir, + version: "test", + pid: "8765", + }, + ] + : [], + }); + + expect(statuses).toEqual([ + { + pid: 8765, + port: 41402, + projectDir: otherProjectDir, + logPath: "/tmp/custom.log", + }, + ]); + expect(existsSync(previewSessionPath(staleProjectDir, stateHome))).toBe(false); + }); + it("force-new waits for a different server instead of reusing the existing one", async () => { const replacement = { ...server, port: 3211, pid: "5432" }; let scans = 0; @@ -94,6 +166,143 @@ describe("background preview lifecycle", () => { expect(spawn).toHaveBeenCalledOnce(); }); + it.each([ + ["force-new", true], + ["a GPU-policy change", false], + ])( + "%s replaces a previously managed server instead of orphaning it", + async (_label, forceNew) => { + const stateHome = mkdtempSync(join(tmpdir(), "hf-preview-state-")); + const oldServer = { ...server, port: 41490, browserGpuMode: "hardware" as const }; + writePreviewSession( + { pid: 4321, port: 41490, projectDir, logPath: "/tmp/preview.log" }, + stateHome, + ); + const replacement = { + ...server, + port: 41491, + pid: "5432", + browserGpuMode: "software" as const, + }; + let oldRunning = true; + let replacementRunning = false; + const scan = vi.fn(async () => + oldRunning ? [oldServer] : replacementRunning ? [replacement] : [], + ); + const kill = vi.fn((pid: number) => { + if (pid === 4321) oldRunning = false; + }); + const spawn = vi.fn(() => { + replacementRunning = true; + return { pid: 5432, unref: vi.fn() }; + }); + + const result = await startBackgroundPreview(projectDir, 41491, { + browserGpuMode: "software", + forceNew, + kill, + scan, + sleep: async () => {}, + spawn, + stateHome, + }); + + expect(scan).toHaveBeenNthCalledWith(1, 41490); + expect(kill).toHaveBeenCalledWith(4321); + expect(result).toMatchObject({ type: "started", port: 41491, pid: 5432 }); + expect(readFileSync(previewSessionPath(projectDir, stateHome), "utf8")).toContain( + '"port": 41491', + ); + }, + ); + + it("replaces the owned preview instead of reusing an unmanaged policy-matching sibling", async () => { + const stateHome = mkdtempSync(join(tmpdir(), "hf-preview-state-")); + const owned = { ...server, port: 41490, browserGpuMode: "hardware" as const }; + const sibling = { + ...server, + port: 41491, + pid: "8765", + browserGpuMode: "software" as const, + }; + const replacement = { + ...server, + port: 41492, + pid: "5432", + browserGpuMode: "software" as const, + }; + writePreviewSession( + { pid: 4321, port: owned.port, projectDir, logPath: "/tmp/preview.log" }, + stateHome, + ); + let ownedRunning = true; + let replacementRunning = false; + const scan = vi.fn(async () => [ + ...(ownedRunning ? [owned] : []), + sibling, + ...(replacementRunning ? [replacement] : []), + ]); + const kill = vi.fn((pid: number) => { + if (pid === 4321) ownedRunning = false; + }); + const spawn = vi.fn(() => { + replacementRunning = true; + return { pid: 5432, unref: vi.fn() }; + }); + + const result = await startBackgroundPreview(projectDir, replacement.port, { + browserGpuMode: "software", + kill, + scan, + sleep: async () => {}, + spawn, + stateHome, + }); + + expect(kill).toHaveBeenCalledWith(4321); + expect(spawn).toHaveBeenCalledOnce(); + expect(result).toMatchObject({ type: "started", port: replacement.port, pid: 5432 }); + }); + + it("launches the replacement when the owned server died on its own", async () => { + // The owned preview crashes (or is Ctrl-C'd) between the outer scan and the + // one inside the stop. "Nothing left to stop" is the goal state for a + // replacement; treating it as fatal refused to start any preview at all + // until the session record was deleted by hand. + const stateHome = mkdtempSync(join(tmpdir(), "hf-preview-state-")); + const owned = { ...server, port: 41490 }; + const replacement = { ...server, port: 41491, pid: "5432" }; + writePreviewSession( + { pid: 4321, port: owned.port, projectDir, logPath: "/tmp/preview.log" }, + stateHome, + ); + let scans = 0; + let replacementRunning = false; + const scan = vi.fn(async () => { + scans += 1; + // Alive for the first look, gone by the time the stop path scans. + const ownedNow = scans === 1 ? [owned] : []; + return [...ownedNow, ...(replacementRunning ? [replacement] : [])]; + }); + const kill = vi.fn(); + const spawn = vi.fn(() => { + replacementRunning = true; + return { pid: 5432, unref: vi.fn() }; + }); + + const result = await startBackgroundPreview(projectDir, replacement.port, { + forceNew: true, + kill, + scan, + sleep: async () => {}, + spawn, + stateHome, + }); + + expect(kill).not.toHaveBeenCalled(); + expect(result).toMatchObject({ type: "started", port: replacement.port, pid: 5432 }); + }); + it("starts a replacement when the existing server uses a different GPU policy", async () => { const hardwareServer = { ...server, browserGpuMode: "hardware" as const }; const softwareServer = { @@ -102,11 +311,15 @@ describe("background preview lifecycle", () => { pid: "5432", browserGpuMode: "software" as const, }; - let scans = 0; - const scan = vi.fn(async () => - ++scans < 2 ? [hardwareServer] : [hardwareServer, softwareServer], - ); - const spawn = vi.fn(() => ({ pid: 5432, unref: vi.fn() })); + let replacementRunning = false; + const scan = vi.fn(async () => [ + hardwareServer, + ...(replacementRunning ? [softwareServer] : []), + ]); + const spawn = vi.fn(() => { + replacementRunning = true; + return { pid: 5432, unref: vi.fn() }; + }); const result = await startBackgroundPreview(projectDir, 3002, { browserGpuMode: "software", @@ -121,10 +334,13 @@ describe("background preview lifecycle", () => { }); it("returns after a detached child becomes reachable and records its session", async () => { - let scans = 0; - const scan = vi.fn(async () => (++scans < 2 ? [] : [server])); + let spawned = false; + const scan = vi.fn(async () => (spawned ? [server] : [])); const unref = vi.fn(); - const spawn = vi.fn(() => ({ pid: 4321, unref })); + const spawn = vi.fn(() => { + spawned = true; + return { pid: 4321, unref }; + }); const stateHome = mkdtempSync(join(tmpdir(), "hf-preview-state-")); const result = await startBackgroundPreview(projectDir, 3002, { @@ -141,6 +357,46 @@ describe("background preview lifecycle", () => { expect(existsSync(previewSessionPath(projectDir, stateHome))).toBe(true); }); + it("reports the live server PID while retaining the wrapper PID for cleanup", async () => { + const liveServer = { ...server, pid: "9876" }; + let spawned = false; + const scan = vi.fn(async () => (spawned ? [liveServer] : [])); + const stateHome = mkdtempSync(join(tmpdir(), "hf-preview-state-")); + + const result = await startBackgroundPreview(projectDir, 3002, { + scan, + spawn: () => { + spawned = true; + return { pid: 4321, unref: vi.fn() }; + }, + stateHome, + }); + + expect(result).toMatchObject({ type: "started", pid: 9876 }); + expect( + JSON.parse(readFileSync(previewSessionPath(projectDir, stateHome), "utf8")), + ).toMatchObject({ pid: 4321 }); + }); + + it("reaps a detached child that never becomes reachable without recording ownership", async () => { + const stateHome = mkdtempSync(join(tmpdir(), "hf-preview-state-")); + const kill = vi.fn(); + + await expect( + startBackgroundPreview(projectDir, 3002, { + scan: async () => [], + spawn: () => ({ pid: 4321, unref: vi.fn() }), + sleep: async () => {}, + kill, + stateHome, + }), + ).rejects.toThrow(/did not become ready/i); + + expect(kill).toHaveBeenCalledOnce(); + expect(kill).toHaveBeenCalledWith(4321); + expect(existsSync(previewSessionPath(projectDir, stateHome))).toBe(false); + }); + it("removes a stale session when no matching server or process survives", async () => { const stateHome = mkdtempSync(join(tmpdir(), "hf-preview-state-")); writePreviewSession( @@ -158,12 +414,68 @@ describe("background preview lifecycle", () => { await expectStaleSessionRemoved(stateHome); }); + it("keeps a live preview's record when a single probe misses it", async () => { + // A server whose event loop is briefly blocked (a Puppeteer thumbnail + // capture will do it) answers nothing for a second or two. Retiring the + // record on that destroys the wrapperIdentity that is the only PID-reuse + // guard `--stop` has, and it never comes back. + const stateHome = mkdtempSync(join(tmpdir(), "hf-preview-state-")); + writePreviewSession( + { pid: 4321, wrapperIdentity: "posix:birth", port: 3210, projectDir, logPath: "/tmp/p.log" }, + stateHome, + ); + + const status = await readBackgroundPreviewStatus(projectDir, 3002, { + scan: async () => [], + identity: () => "posix:birth", + stateHome, + }); + + expect(status).toBeNull(); + expect(existsSync(previewSessionPath(projectDir, stateHome))).toBe(true); + }); + + it("retires the record once the wrapper PID has been recycled", async () => { + const stateHome = mkdtempSync(join(tmpdir(), "hf-preview-state-")); + writePreviewSession( + { pid: 4321, wrapperIdentity: "posix:birth", port: 3210, projectDir, logPath: "/tmp/p.log" }, + stateHome, + ); + + const status = await readBackgroundPreviewStatus(projectDir, 3002, { + scan: async () => [], + identity: () => "posix:someone-else", + stateHome, + }); + + expect(status).toBeNull(); + expect(existsSync(previewSessionPath(projectDir, stateHome))).toBe(false); + }); + + it("never leaves a partial session record for a concurrent reader", () => { + const stateHome = mkdtempSync(join(tmpdir(), "hf-preview-state-")); + savePreviewSession(stateHome); + const path = previewSessionPath(projectDir, stateHome); + + writePreviewSession({ pid: 55, port: 3211, projectDir, logPath: "/tmp/two.log" }, stateHome); + + // Written through a temp file and renamed, so a reader mid-write sees the + // whole previous record rather than truncated JSON it would then delete. + expect(JSON.parse(readFileSync(path, "utf8"))).toMatchObject({ pid: 55, port: 3211 }); + expect( + readdirSync(join(stateHome, "hyperframes", "previews")).filter((n) => n.endsWith(".tmp")), + ).toHaveLength(0); + }); + it("uses the recorded custom port when status is called without repeating --port", async () => { const stateHome = mkdtempSync(join(tmpdir(), "hf-preview-state-")); savePreviewSession(stateHome); const scan = vi.fn(async () => [server]); - const status = await readBackgroundPreviewStatus(projectDir, 3002, { scan, stateHome }); + const status = await readBackgroundPreviewStatus(projectDir, 3002, { + scan, + stateHome, + }); expect(status?.port).toBe(3210); expect(scan).toHaveBeenCalledWith(3210); @@ -204,27 +516,87 @@ describe("background preview lifecycle", () => { expect(existsSync(previewSessionPath(projectDir, stateHome))).toBe(false); }); - it("uses the saved child PID when a matching live server cannot report one", async () => { + it("refuses to stop when the live server cannot prove its own PID", async () => { const stateHome = mkdtempSync(join(tmpdir(), "hf-preview-state-")); writePreviewSession( { pid: 4321, port: 3210, projectDir, logPath: "/tmp/preview.log" }, stateHome, ); + const scan = vi.fn(async () => [{ ...server, pid: null }]); + const kill = vi.fn(); + + await expect( + stopBackgroundPreview(projectDir, 3002, { + scan, + kill, + sleep: async () => {}, + stateHome, + }), + ).rejects.toThrow(/ownership/i); + + expect(kill).not.toHaveBeenCalled(); + }); + + it("reaps the saved wrapper when the live server is proven to be its descendant", async () => { + const stateHome = mkdtempSync(join(tmpdir(), "hf-preview-state-")); + writePreviewSession( + { + pid: 4321, + wrapperIdentity: "wrapper-birth", + port: 3210, + projectDir, + logPath: "/tmp/preview.log", + }, + stateHome, + ); let running = true; - const scan = vi.fn(async () => (running ? [{ ...server, pid: null }] : [])); - const kill = vi.fn(() => { - running = false; + const scan = vi.fn(async () => (running ? [{ ...server, pid: "9876" }] : [])); + const kill = vi.fn((pid: number) => { + if (pid === 4321) running = false; }); const result = await stopBackgroundPreview(projectDir, 3002, { scan, kill, + isDescendant: (childPid, ancestorPid) => childPid === 9876 && ancestorPid === 4321, + identity: (pid) => (pid === 4321 ? "wrapper-birth" : null), sleep: async () => {}, stateHome, }); expect(result).toBe(true); - expect(kill).toHaveBeenCalledWith(4321); + expect(kill.mock.calls).toEqual([[4321]]); + }); + + it("kills only the live server when the saved wrapper birth identity has changed", async () => { + const stateHome = mkdtempSync(join(tmpdir(), "hf-preview-state-")); + writePreviewSession( + { + pid: 4321, + wrapperIdentity: "original-wrapper-birth", + port: 3210, + projectDir, + logPath: "/tmp/preview.log", + }, + stateHome, + ); + let running = true; + const scan = vi.fn(async () => (running ? [{ ...server, pid: "9876" }] : [])); + const kill = vi.fn((pid: number) => { + if (pid === 9876) running = false; + }); + + const result = await stopBackgroundPreview(projectDir, 3002, { + scan, + kill, + isDescendant: () => true, + identity: () => "reused-pid-birth", + sleep: async () => {}, + stateHome, + }); + + expect(result).toBe(true); + expect(kill.mock.calls).toEqual([[9876]]); }); it("fails loudly when the server remains reachable after stop", async () => { @@ -244,4 +616,30 @@ describe("background preview lifecycle", () => { ).rejects.toThrow(/did not stop/i); expect(existsSync(previewSessionPath(projectDir, stateHome))).toBe(true); }); + + it("verifies the owned port stopped even when another server serves the same project", async () => { + const stateHome = mkdtempSync(join(tmpdir(), "hf-preview-state-")); + const owned = { ...server, port: 41490 }; + const sibling = { ...server, port: 41491, pid: "8765" }; + writePreviewSession( + { pid: 4321, port: owned.port, projectDir, logPath: "/tmp/preview.log" }, + stateHome, + ); + let ownedRunning = true; + const scan = vi.fn(async () => [...(ownedRunning ? [owned] : []), sibling]); + const kill = vi.fn((pid: number) => { + if (pid === 4321) ownedRunning = false; + }); + + const stopped = await stopBackgroundPreview(projectDir, owned.port, { + kill, + scan, + sleep: async () => {}, + stateHome, + }); + + expect(stopped).toBe(true); + expect(kill).toHaveBeenCalledWith(4321); + expect(existsSync(previewSessionPath(projectDir, stateHome))).toBe(false); + }); }); diff --git a/packages/cli/src/commands/previewLifecycle.ts b/packages/cli/src/commands/previewLifecycle.ts index eaec7f91ab..196f1a0bfc 100644 --- a/packages/cli/src/commands/previewLifecycle.ts +++ b/packages/cli/src/commands/previewLifecycle.ts @@ -6,6 +6,8 @@ import { mkdirSync, openSync, readFileSync, + readdirSync, + renameSync, rmSync, writeFileSync, } from "node:fs"; @@ -13,10 +15,11 @@ import { homedir } from "node:os"; import { dirname, join, resolve } from "node:path"; import { scanActiveServers, type ActiveServer } from "../server/portUtils.js"; import type { BrowserGpuMode } from "../browser/gpuPolicy.js"; -import { killProcessTree } from "../utils/orphanCleanup.js"; +import { isProcessDescendant, killProcessTree, processIdentity } from "../utils/orphanCleanup.js"; export interface PreviewSession { pid: number; + wrapperIdentity?: string; port: number; projectDir: string; logPath: string; @@ -40,6 +43,8 @@ interface LifecycleDependencies { spawn?: SpawnPreview; sleep?: (ms: number) => Promise; kill?: (pid: number) => void; + isDescendant?: (childPid: number, ancestorPid: number) => boolean; + identity?: (pid: number) => string | null; stateHome?: string; forceNew?: boolean; browserGpuMode?: BrowserGpuMode; @@ -70,7 +75,13 @@ function previewLogPath(projectDir: string, stateHome = defaultStateHome()): str export function writePreviewSession(session: PreviewSession, stateHome = defaultStateHome()): void { const path = previewSessionPath(session.projectDir, stateHome); mkdirSync(dirname(path), { recursive: true }); - writeFileSync(path, `${JSON.stringify(session, null, 2)}\n`, { mode: 0o600 }); + // Written through a temp file and renamed: every reader deletes this record + // when it fails to parse, so a concurrent reader catching a half-written file + // would destroy a live server's only ownership proof. `rename` is atomic + // within a directory, so a reader sees either the old record or the new one. + const temporary = `${path}.${process.pid}.tmp`; + writeFileSync(temporary, `${JSON.stringify(session, null, 2)}\n`, { mode: 0o600 }); + renameSync(temporary, path); } function readPreviewSession( @@ -95,6 +106,43 @@ function readPreviewSession( } } +function hasValidPreviewProcess(session: PreviewSession): boolean { + return Number.isInteger(session.pid) && session.pid > 0; +} + +function hasValidPreviewEndpoint(session: PreviewSession): boolean { + return Number.isInteger(session.port) && session.port > 0 && session.port <= 65535; +} + +function matchesPreviewSessionFile( + session: PreviewSession, + path: string, + stateHome: string, +): boolean { + return ( + typeof session.projectDir === "string" && + typeof session.logPath === "string" && + previewSessionPath(session.projectDir, stateHome) === path + ); +} + +function readPreviewSessionFile(path: string, stateHome: string): PreviewSession | null { + try { + const parsed = JSON.parse(readFileSync(path, "utf8")) as PreviewSession; + if ( + !hasValidPreviewProcess(parsed) || + !hasValidPreviewEndpoint(parsed) || + !matchesPreviewSessionFile(parsed, path, stateHome) + ) { + throw new Error("invalid preview session"); + } + return parsed; + } catch { + rmSync(path, { force: true }); + return null; + } +} + function removePreviewSession(projectDir: string, stateHome = defaultStateHome()): void { rmSync(previewSessionPath(projectDir, stateHome), { force: true }); } @@ -113,6 +161,26 @@ function matchingServer( ); } +function matchingServerAtPort( + servers: ActiveServer[], + projectDir: string, + port: number, +): ActiveServer | null { + return matchingServer( + servers.filter((server) => server.port === port), + projectDir, + ); +} + +function sameProjectPorts(servers: ActiveServer[], projectDir: string): Set { + const project = normalized(projectDir); + return new Set( + servers + .filter((server) => normalized(server.projectDir) === project) + .map((server) => server.port), + ); +} + function stopProcess(pid: number): void { killProcessTree(pid); if (process.platform === "win32") { @@ -130,7 +198,7 @@ function spawnDetachedPreview( projectDir: string, stateHome: string, dependencies: LifecycleDependencies, -): { pid: number; logPath: string } { +): { pid: number; wrapperIdentity: string | undefined; logPath: string } { const logPath = previewLogPath(projectDir, stateHome); mkdirSync(dirname(logPath), { recursive: true }); const logFd = openSync(logPath, "a", 0o600); @@ -151,18 +219,20 @@ function spawnDetachedPreview( } if (!child.pid) throw new Error("background preview child did not report a PID"); child.unref(); - return { pid: child.pid, logPath }; + return { + pid: child.pid, + wrapperIdentity: (dependencies.identity ?? processIdentity)(child.pid) ?? undefined, + logPath, + }; } function startedServer( servers: ActiveServer[], projectDir: string, - existing: ActiveServer | null, - forceNew: boolean, + preLaunchPorts: Set, browserGpuMode?: BrowserGpuMode, ): ActiveServer | null { - const candidates = - forceNew && existing ? servers.filter((server) => server.port !== existing.port) : servers; + const candidates = servers.filter((server) => !preLaunchPorts.has(server.port)); return matchingServer(candidates, projectDir, browserGpuMode); } @@ -198,10 +268,124 @@ export async function readBackgroundPreviewStatus( } } + // A missed probe is not proof the preview is gone: a server whose event loop + // is momentarily blocked (a Puppeteer thumbnail capture will do it) answers + // nothing for a second or two. Deleting the record on that would destroy the + // wrapperIdentity that is the only PID-reuse guard `--stop` has, and it never + // comes back. Only a wrapper process that is provably gone retires a record. + if (saved && wrapperProcessIsAlive(saved, dependencies)) return null; removePreviewSession(projectDir, stateHome); return null; } +/** + * Whether the recorded wrapper process is still the process we launched. + * + * `processIdentity` returns a birth token, so a recycled PID reads as a + * different process and the record is correctly retired. A null token (no such + * process, or the platform lookup failed) is only treated as "gone" when the + * record has no token to compare against — failing closed there would pin dead + * records forever on platforms where the lookup is unavailable. + */ +function wrapperProcessIsAlive( + saved: PreviewSession, + dependencies: LifecycleDependencies, +): boolean { + if (!saved.wrapperIdentity) return false; + const identity = dependencies.identity ?? processIdentity; + return identity(saved.pid) === saved.wrapperIdentity; +} + +export async function listBackgroundPreviewStatuses( + dependencies: LifecycleDependencies = {}, +): Promise { + const stateHome = dependencies.stateHome ?? defaultStateHome(); + const directory = sessionDirectory(stateHome); + let files: string[]; + try { + files = readdirSync(directory) + .filter((name) => name.endsWith(".json")) + .map((name) => join(directory, name)); + } catch { + return []; + } + + const saved = files + .map((path) => readPreviewSessionFile(path, stateHome)) + .filter((session): session is PreviewSession => session !== null); + const statuses = await Promise.all( + saved.map((session) => + readBackgroundPreviewStatus(session.projectDir, session.port, { + ...dependencies, + stateHome, + }), + ), + ); + return statuses.filter((status): status is PreviewSession => status !== null); +} + +function readyPreviewSession( + server: ActiveServer, + pid: number, + wrapperIdentity: string | undefined, + projectDir: string, + logPath: string, + dependencies: LifecycleDependencies, +): { session: PreviewSession; publicPid: number } { + const identity = wrapperIdentity ?? (dependencies.identity ?? processIdentity)(pid) ?? undefined; + const liveServerPid = Number(server.pid); + return { + session: { + pid, + wrapperIdentity: identity, + port: server.port, + projectDir: resolve(projectDir), + logPath, + }, + publicPid: Number.isInteger(liveServerPid) && liveServerPid > 0 ? liveServerPid : pid, + }; +} + +function ownedStopTargetPid( + saved: PreviewSession | null, + liveServerPid: number, + dependencies: LifecycleDependencies, +): number { + if (!saved?.wrapperIdentity) return liveServerPid; + const identity = dependencies.identity ?? processIdentity; + if (identity(saved.pid) !== saved.wrapperIdentity) return liveServerPid; + if (saved.pid === liveServerPid) return saved.pid; + const isDescendant = dependencies.isDescendant ?? isProcessDescendant; + return isDescendant(liveServerPid, saved.pid) ? saved.pid : liveServerPid; +} + +async function stopOwnedPreviewBeforeReplacement( + owned: ActiveServer | null, + projectDir: string, + dependencies: LifecycleDependencies, +): Promise { + if (!owned) return; + // `false` means "there was nothing left to stop" — the server went away + // between the outer scan and this one. That is the goal state for a + // replacement, not a failure; treating it as fatal refused to start any + // preview at all until the user deleted the session record by hand. + // A server that is still listening throws from inside stopBackgroundPreview. + await stopBackgroundPreview(projectDir, owned.port, dependencies); +} + +function savedOwnedPreview( + servers: ActiveServer[], + saved: PreviewSession | null, + projectDir: string, +): ActiveServer | null { + if (!saved) return null; + // Ownership comes from the saved project+port, not the replacement's GPU + // policy. Filtering here would miss an owned hardware→software replacement + // and overwrite the only session record while leaving the old listener live. + const savedPortServers = servers.filter((server) => server.port === saved.port); + return matchingServer(savedPortServers, projectDir); +} + export async function startBackgroundPreview( projectDir: string, startPort: number, @@ -211,37 +395,65 @@ export async function startBackgroundPreview( | { type: "started"; port: number; pid: number; logPath: string } > { const scan = dependencies.scan ?? scanActiveServers; - const existing = matchingServer(await scan(startPort), projectDir, dependencies.browserGpuMode); - if (existing && !dependencies.forceNew) { + const stateHome = dependencies.stateHome ?? defaultStateHome(); + const saved = readPreviewSession(projectDir, stateHome); + // Always inspect a saved custom port first. `--force-new --port ` must + // replace that owned server before recording the replacement, otherwise the + // single per-project ownership record would orphan the old listener. + const scanStart = saved?.port ?? startPort; + const scanned = await scan(scanStart); + const requestedExisting = matchingServer(scanned, projectDir, dependencies.browserGpuMode); + const ownedExisting = savedOwnedPreview(scanned, saved, projectDir); + // A saved managed preview is the authoritative same-project instance. An + // explicit GPU-policy change replaces it; it must not silently adopt an + // unmanaged sibling that happens to match the new policy. + const reusableOwned = ownedExisting + ? matchingServer([ownedExisting], projectDir, dependencies.browserGpuMode) + : null; + const reusableExisting = reusableOwned ?? (ownedExisting ? null : requestedExisting); + if (reusableExisting && !dependencies.forceNew) { return { type: "reused", - port: existing.port, - pid: existing.pid ? Number(existing.pid) : null, + port: reusableExisting.port, + pid: reusableExisting.pid ? Number(reusableExisting.pid) : null, logPath: null, }; } + await stopOwnedPreviewBeforeReplacement(ownedExisting, projectDir, dependencies); + // Snapshot every same-project listener in the prospective launch range only + // after the owned listener is gone. Readiness must identify a newly appeared + // server, never a pre-existing unmanaged sibling. + const preLaunchPorts = sameProjectPorts(await scan(startPort), projectDir); - const stateHome = dependencies.stateHome ?? defaultStateHome(); - const { pid, logPath } = spawnDetachedPreview(projectDir, stateHome, dependencies); + const { pid, wrapperIdentity, logPath } = spawnDetachedPreview( + projectDir, + stateHome, + dependencies, + ); const sleep = dependencies.sleep ?? delay; for (let attempt = 0; attempt < 50; attempt++) { const server = startedServer( await scan(startPort), projectDir, - existing, - dependencies.forceNew === true, + preLaunchPorts, dependencies.browserGpuMode, ); if (server) { - const session = { + const ready = readyPreviewSession( + server, pid, - port: server.port, - projectDir: resolve(projectDir), + wrapperIdentity, + projectDir, logPath, + dependencies, + ); + writePreviewSession(ready.session, stateHome); + return { + type: "started", + ...ready.session, + pid: ready.publicPid, }; - writePreviewSession(session, stateHome); - return { type: "started", ...session }; } await sleep(200); } @@ -259,19 +471,28 @@ export async function stopBackgroundPreview( const stateHome = dependencies.stateHome ?? defaultStateHome(); const saved = readPreviewSession(projectDir, stateHome); const scanStart = saved?.port ?? startPort; - const server = matchingServer(await scan(scanStart), projectDir); - // A saved PID can be reused after a crashed preview, so only trust it while - // a currently reachable server proves this exact project is still running. - const pid = Number(server ? (server.pid ?? saved?.pid) : undefined); - if (!Number.isInteger(pid) || pid <= 0) { + const scanned = await scan(scanStart); + const server = saved + ? matchingServerAtPort(scanned, projectDir, saved.port) + : matchingServer(scanned, projectDir); + if (!server) { removePreviewSession(projectDir, stateHome); return false; } + // A saved PID can be reused after a crashed preview. The HTTP probe proves + // the project, but only the live server's own metadata proves which process + // owns it; never substitute the saved wrapper PID here. + const pid = Number(server.pid); + if (!Number.isInteger(pid) || pid <= 0) { + throw new Error(`preview ownership could not be proven for ${resolve(projectDir)}`); + } + + const kill = dependencies.kill ?? stopProcess; + kill(ownedStopTargetPid(saved, pid, dependencies)); - (dependencies.kill ?? stopProcess)(pid); const sleep = dependencies.sleep ?? delay; for (let attempt = 0; attempt < 25; attempt++) { - if (!matchingServer(await scan(scanStart), projectDir)) { + if (!matchingServerAtPort(await scan(scanStart), projectDir, server.port)) { removePreviewSession(projectDir, stateHome); return true; } From cc93fee03cefe62d19895f025e0f8cc1d1655337 Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Tue, 18 Aug 2026 18:10:23 -0400 Subject: [PATCH 2/3] fix(cli): keep a record whose identity lookup gave no answer, not a different one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review blocker. The keep-alive path this PR adds could still retire a LIVE record — through a different door than the one it closed. `processIdentity` catches every failure into `null`, and on two of three platforms that failure is a subprocess timeout on a live process: the win32 `Win32_Process` CIM query and the POSIX `ps -o lstart=` both run on a 2 s budget, under exactly the load that made the HTTP probe miss in the first place. A `null` compared unequal to the saved token, so the record was deleted and `wrapperIdentity` — the only PID-reuse guard `--stop` has — was gone for good. Only Linux, reading /proc directly, was reliable. No answer is now distinguished from a different answer: the PID is checked with `kill(pid, 0)` first, which asks the kernel without signalling and treats EPERM as alive. A PID nothing can signal is gone and retires the record with no subprocess at all; a signalable PID whose token cannot be read keeps it. Only a token that comes back and differs retires it. That ordering also answers the `--list` note: the identity subprocess no longer runs for the stale records that made it slow, so the N x 2 s worst case is gone along with the timeouts that fed the bug. Verified by mutation: restoring the old "no answer means gone" behaviour reds the new case. Also clean up the temp file when a rename fails, rather than orphaning it in the session directory. --- .../cli/src/commands/previewLifecycle.test.ts | 45 +++++++++++++++++++ packages/cli/src/commands/previewLifecycle.ts | 45 +++++++++++++++++-- 2 files changed, 86 insertions(+), 4 deletions(-) diff --git a/packages/cli/src/commands/previewLifecycle.test.ts b/packages/cli/src/commands/previewLifecycle.test.ts index e2a26c91b9..1238dd48d0 100644 --- a/packages/cli/src/commands/previewLifecycle.test.ts +++ b/packages/cli/src/commands/previewLifecycle.test.ts @@ -428,6 +428,7 @@ describe("background preview lifecycle", () => { const status = await readBackgroundPreviewStatus(projectDir, 3002, { scan: async () => [], identity: () => "posix:birth", + isSignalable: () => true, stateHome, }); @@ -435,6 +436,49 @@ describe("background preview lifecycle", () => { expect(existsSync(previewSessionPath(projectDir, stateHome))).toBe(true); }); + it("keeps a live preview's record when the identity lookup gives no answer", async () => { + // `processIdentity` catches every failure into `null`, and on Windows and + // macOS that failure is a subprocess timeout on a LIVE process — under the + // same load that made the HTTP probe miss. Treating no-answer as + // "recycled" destroyed the only PID-reuse guard `--stop` has. + const stateHome = mkdtempSync(join(tmpdir(), "hf-preview-state-")); + writePreviewSession( + { pid: 4321, wrapperIdentity: "posix:birth", port: 3210, projectDir, logPath: "/tmp/p.log" }, + stateHome, + ); + + const status = await readBackgroundPreviewStatus(projectDir, 3002, { + scan: async () => [], + identity: () => null, + isSignalable: () => true, + stateHome, + }); + + expect(status).toBeNull(); + expect(existsSync(previewSessionPath(projectDir, stateHome))).toBe(true); + }); + + it("retires the record when the PID cannot be signalled at all", async () => { + // Gone is gone: no birth token needed, and no subprocess spawned for it. + const stateHome = mkdtempSync(join(tmpdir(), "hf-preview-state-")); + writePreviewSession( + { pid: 4321, wrapperIdentity: "posix:birth", port: 3210, projectDir, logPath: "/tmp/p.log" }, + stateHome, + ); + const identity = vi.fn(() => "posix:birth"); + + const status = await readBackgroundPreviewStatus(projectDir, 3002, { + scan: async () => [], + identity, + isSignalable: () => false, + stateHome, + }); + + expect(status).toBeNull(); + expect(existsSync(previewSessionPath(projectDir, stateHome))).toBe(false); + expect(identity).not.toHaveBeenCalled(); + }); + it("retires the record once the wrapper PID has been recycled", async () => { const stateHome = mkdtempSync(join(tmpdir(), "hf-preview-state-")); writePreviewSession( @@ -445,6 +489,7 @@ describe("background preview lifecycle", () => { const status = await readBackgroundPreviewStatus(projectDir, 3002, { scan: async () => [], identity: () => "posix:someone-else", + isSignalable: () => true, stateHome, }); diff --git a/packages/cli/src/commands/previewLifecycle.ts b/packages/cli/src/commands/previewLifecycle.ts index 196f1a0bfc..02c0eb6cfc 100644 --- a/packages/cli/src/commands/previewLifecycle.ts +++ b/packages/cli/src/commands/previewLifecycle.ts @@ -45,6 +45,7 @@ interface LifecycleDependencies { kill?: (pid: number) => void; isDescendant?: (childPid: number, ancestorPid: number) => boolean; identity?: (pid: number) => string | null; + isSignalable?: (pid: number) => boolean; stateHome?: string; forceNew?: boolean; browserGpuMode?: BrowserGpuMode; @@ -80,8 +81,14 @@ export function writePreviewSession(session: PreviewSession, stateHome = default // would destroy a live server's only ownership proof. `rename` is atomic // within a directory, so a reader sees either the old record or the new one. const temporary = `${path}.${process.pid}.tmp`; - writeFileSync(temporary, `${JSON.stringify(session, null, 2)}\n`, { mode: 0o600 }); - renameSync(temporary, path); + try { + writeFileSync(temporary, `${JSON.stringify(session, null, 2)}\n`, { mode: 0o600 }); + renameSync(temporary, path); + } finally { + // A failed rename would otherwise orphan the temp file. The `.json` filter + // hides it from the lister, so it accumulates silently. + rmSync(temporary, { force: true }); + } } function readPreviewSession( @@ -287,13 +294,43 @@ export async function readBackgroundPreviewStatus( * record has no token to compare against — failing closed there would pin dead * records forever on platforms where the lookup is unavailable. */ +/** + * Whether a PID exists at all. `kill(pid, 0)` sends no signal; it only asks the + * kernel. `EPERM` means the process is there but owned by someone else — still + * alive, which is the question being asked. + */ +function processIsSignalable(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch (error) { + return (error as NodeJS.ErrnoException | undefined)?.code === "EPERM"; + } +} + function wrapperProcessIsAlive( saved: PreviewSession, dependencies: LifecycleDependencies, ): boolean { if (!saved.wrapperIdentity) return false; - const identity = dependencies.identity ?? processIdentity; - return identity(saved.pid) === saved.wrapperIdentity; + + // Cheap and decisive first: a PID nothing can signal is gone, and no birth + // token is needed to say so. This also keeps the identity subprocess off the + // path for exactly the stale records that make `--list` slow. + const signalable = dependencies.isSignalable ?? processIsSignalable; + if (!signalable(saved.pid)) return false; + + const identity = (dependencies.identity ?? processIdentity)(saved.pid); + // No answer is NOT the same as a different answer. `processIdentity` catches + // every failure into `null`, and on two of three platforms that failure is a + // subprocess timeout on a live process — `ps -o lstart=` and PowerShell's CIM + // query both run on a 2 s budget, under the very load that made the HTTP + // probe miss in the first place. Treating that as "recycled" would destroy + // the only PID-reuse guard `--stop` has, which is the loss this whole path + // exists to prevent. The PID is signalable, so keep the record. + if (identity === null) return true; + + return identity === saved.wrapperIdentity; } export async function listBackgroundPreviewStatuses( From ce61db739539895430ed7cb43533f0448c6a1e33 Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Tue, 18 Aug 2026 19:14:22 -0400 Subject: [PATCH 3/3] test(cli): assert only what the birth-token lookup actually guarantees MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `captures a stable birth token for the current process` made two assertions that a lookup allowed to fail cannot support. `processIdentity` returns null whenever the lookup cannot be completed — not only when the process is absent — and on Windows and macOS it shells out to PowerShell or `ps` on a 2 s budget that a cold CI runner routinely outruns. Both failed on windows-latest, in sequence: first `.toMatch()` received null, and once that was guarded, `expect(second).toBe(first)` compared a null from the cold first spawn against a token from the warm second one. Two lookups can disagree for exactly one reason — one of them failed — so stability is only assertable across two successful ones. The token itself cannot change between calls; it is a birth timestamp and the process did not restart. `processIdentity(-1)` stays unconditional: the guard rejects it before any subprocess runs. The strict shape assertion moves to a Linux-only case, where /proc is read directly with no subprocess and null is genuinely not allowed — keeping the guarantee on the one platform that can honour it rather than dropping it everywhere. Callers already depend on this contract: `wrapperProcessIsAlive` treats null as "no answer" rather than "gone" precisely because it is reachable. --- packages/cli/src/utils/orphanCleanup.test.ts | 35 ++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/packages/cli/src/utils/orphanCleanup.test.ts b/packages/cli/src/utils/orphanCleanup.test.ts index aebe4d0b6c..a6e0cca924 100644 --- a/packages/cli/src/utils/orphanCleanup.test.ts +++ b/packages/cli/src/utils/orphanCleanup.test.ts @@ -18,12 +18,43 @@ describe("Windows process-tree cleanup", () => { describe("process-tree ownership", () => { it("captures a stable birth token for the current process", () => { + // `processIdentity` is documented to return null when the lookup cannot be + // completed, not only when the process is absent — and on Windows and macOS + // it shells out to PowerShell / `ps` on a 2 s budget, which a cold CI runner + // routinely outruns. Asserting an unconditional token therefore tested the + // runner's spawn latency rather than the function: it failed on + // windows-latest with `.toMatch()` receiving null. + // + // What is actually promised: a well-formed token OR null, the same answer + // twice in a row, and null for a pid that cannot exist. Callers are built + // on exactly that contract — `wrapperProcessIsAlive` treats null as "no + // answer" rather than "gone" precisely because it is reachable here. const first = processIdentity(process.pid); - expect(first).toMatch(/^(?:linux|posix|windows):/); - expect(processIdentity(process.pid)).toBe(first); + const second = processIdentity(process.pid); + + // Stability is only assertable across two SUCCESSFUL lookups. Two calls can + // disagree here for one reason — one of them failed — and that is exactly + // what happens on a cold Windows runner: the first PowerShell spawn outruns + // the 2 s budget and returns null, the second is warm and returns a token. + // The token itself cannot change between them; it is a birth timestamp and + // the process did not restart. + if (first !== null && second !== null) { + expect(first).toMatch(/^(?:linux|posix|windows):/); + expect(second).toBe(first); + } + + // This one holds everywhere: the guard rejects it before any subprocess. expect(processIdentity(-1)).toBeNull(); }); + it("reads a well-formed token where the lookup cannot fail", () => { + // Linux reads /proc directly with no subprocess, so there the token is not + // allowed to be null — this keeps the strict assertion on the one platform + // that can honour it, rather than dropping it everywhere. + if (process.platform !== "linux") return; + expect(processIdentity(process.pid)).toMatch(/^linux:\d+$/); + }); + it("proves ancestry through every intermediate wrapper", () => { const parents = new Map([ [400, 300],