diff --git a/bin/multicorn-shield.ts b/bin/multicorn-shield.ts index f7d297c..a29922e 100644 --- a/bin/multicorn-shield.ts +++ b/bin/multicorn-shield.ts @@ -63,6 +63,8 @@ export interface CliArgs { readonly filesStatus: boolean; /** `files` subcommand: restart sub-action (stop then start). */ readonly filesRestart: boolean; + /** `files` subcommand: internal flag to respawn the shared proxy on start. */ + readonly filesRespawnProxy: boolean; } export function parseArgs(argv: readonly string[]): CliArgs { @@ -86,6 +88,7 @@ export function parseArgs(argv: readonly string[]): CliArgs { let filesForeground = false; let filesStatus = false; let filesRestart = false; + let filesRespawnProxy = false; for (let i = 0; i < args.length; i++) { const arg = args[i]; @@ -139,6 +142,8 @@ export function parseArgs(argv: readonly string[]): CliArgs { filesStop = true; } else if (token === "--foreground") { filesForeground = true; + } else if (token === "--respawn-proxy") { + filesRespawnProxy = true; } else if (token === "--detach") { // Legacy flag (now default behavior) - ignored } else if (token === "stop") { @@ -276,6 +281,7 @@ export function parseArgs(argv: readonly string[]): CliArgs { filesForeground, filesStatus, filesRestart, + filesRespawnProxy, }; } @@ -390,6 +396,7 @@ export async function runCli(): Promise { foreground: true, status: true, restart: false, + respawnProxy: false, }); return; } @@ -417,6 +424,7 @@ export async function runCli(): Promise { foreground: cli.filesForeground, status: false, restart: cli.filesRestart, + respawnProxy: cli.filesRespawnProxy, }); return; } diff --git a/src/commands/files-defect-fixes.test.ts b/src/commands/files-defect-fixes.test.ts new file mode 100644 index 0000000..bdf6e11 --- /dev/null +++ b/src/commands/files-defect-fixes.test.ts @@ -0,0 +1,327 @@ +import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; +import { EventEmitter } from "node:events"; +import { existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { join } from "node:path"; + +const testMulticornHome = vi.hoisted(() => { + const dir = `/tmp/shield-files-defects-${String(process.pid)}`; + process.env["MULTICORN_HOME"] = dir; + return dir; +}); + +const DEAD_PID = 2_000_000; + +function fetchInputUrl(input: RequestInfo | URL): string { + if (typeof input === "string") return input; + if (input instanceof URL) return input.href; + return input.url; +} + +vi.mock("../proxy/config.js", async (importOriginal) => { + // eslint-disable-next-line @typescript-eslint/consistent-type-imports -- vitest importOriginal generic + const actual = await importOriginal(); + return { + ...actual, + loadConfig: vi.fn(actual.loadConfig), + readBaseUrlFromConfig: vi.fn(actual.readBaseUrlFromConfig), + }; +}); + +vi.mock("node:net", async (importOriginal) => { + // eslint-disable-next-line @typescript-eslint/consistent-type-imports -- vitest importOriginal generic + const actual = await importOriginal(); + return { + ...actual, + createConnection: () => { + const sock = new EventEmitter(); + queueMicrotask(() => { + sock.emit("error", new Error("ECONNREFUSED")); + }); + return sock; + }, + }; +}); + +vi.mock("node:child_process", async (importOriginal) => { + // eslint-disable-next-line @typescript-eslint/consistent-type-imports -- vitest importOriginal generic + const actual = await importOriginal(); + return { + ...actual, + spawn: vi.fn(), + }; +}); + +vi.mock("./local-proxy-start.js", async (importOriginal) => { + // eslint-disable-next-line @typescript-eslint/consistent-type-imports -- vitest importOriginal generic + const actual = await importOriginal(); + const fakeServerEntry = join(import.meta.dirname, "__fixtures__", "fake-server.js"); + return { + ...actual, + LOCAL_PROXY_READY_MAX_POLLS: 2, + LOCAL_PROXY_READY_POLL_MS: 1, + buildLocalProxySpawnCommand: vi.fn((port: number, apiBaseUrl: string) => + actual.buildLocalProxySpawnCommand(port, apiBaseUrl, process.execPath, fakeServerEntry), + ), + }; +}); + +import { spawn } from "node:child_process"; +import { buildLocalProxySpawnCommand } from "./local-proxy-start.js"; +import { loadConfig, readBaseUrlFromConfig } from "../proxy/config.js"; +import type * as FilesModule from "./files.js"; + +describe("resolveBaseUrl (Fix 1: base URL precedence)", () => { + let resolveBaseUrl: typeof FilesModule.resolveBaseUrl; + const originalEnvBaseUrl = process.env["MULTICORN_BASE_URL"]; + + beforeAll(async () => { + ({ resolveBaseUrl } = await import("./files.js")); + }); + + beforeEach(() => { + vi.mocked(loadConfig).mockReset(); + vi.mocked(readBaseUrlFromConfig).mockReset(); + delete process.env["MULTICORN_BASE_URL"]; + }); + + afterEach(() => { + if (originalEnvBaseUrl === undefined) { + delete process.env["MULTICORN_BASE_URL"]; + } else { + process.env["MULTICORN_BASE_URL"] = originalEnvBaseUrl; + } + }); + + it("prefers --base-url over MULTICORN_BASE_URL, config.json, and default", async () => { + process.env["MULTICORN_BASE_URL"] = "https://env.example.com"; + vi.mocked(loadConfig).mockResolvedValue({ + apiKey: "mcs_test_key_12", + baseUrl: "https://config.example.com", + }); + vi.mocked(readBaseUrlFromConfig).mockResolvedValue("https://partial.example.com"); + + await expect(resolveBaseUrl("http://localhost:8080")).resolves.toBe("http://localhost:8080"); + }); + + it("uses MULTICORN_BASE_URL when --base-url is omitted", async () => { + process.env["MULTICORN_BASE_URL"] = "http://localhost:9090"; + vi.mocked(loadConfig).mockResolvedValue({ + apiKey: "mcs_test_key_12", + baseUrl: "https://config.example.com", + }); + vi.mocked(readBaseUrlFromConfig).mockResolvedValue("https://partial.example.com"); + + await expect(resolveBaseUrl(undefined)).resolves.toBe("http://localhost:9090"); + }); + + it("falls back to config.json baseUrl when env is unset", async () => { + vi.mocked(loadConfig).mockResolvedValue({ + apiKey: "mcs_test_key_12", + baseUrl: "http://localhost:8080", + }); + vi.mocked(readBaseUrlFromConfig).mockResolvedValue(undefined); + + await expect(resolveBaseUrl(undefined)).resolves.toBe("http://localhost:8080"); + }); + + it("uses production default when nothing else is configured", async () => { + vi.mocked(loadConfig).mockResolvedValue(null); + vi.mocked(readBaseUrlFromConfig).mockResolvedValue(undefined); + + await expect(resolveBaseUrl(undefined)).resolves.toBe("https://api.multicorn.ai"); + }); +}); + +describe("ensureProxy spawn env (Fix 1: SHIELD_API_BASE_URL on proxy)", () => { + let ensureProxyForTests: ( + proxyPort: number, + apiBaseUrl: string, + options?: { forceRespawn?: boolean }, + ) => Promise; + + beforeEach(async () => { + vi.resetModules(); + ({ ensureProxyForTests } = await import("./files.js")); + + mkdirSync(testMulticornHome, { recursive: true }); + + const fakeChild = new EventEmitter() as EventEmitter & { pid?: number; unref: () => void }; + fakeChild.pid = 5151; + fakeChild.unref = vi.fn(); + + vi.mocked(spawn).mockReturnValue(fakeChild as unknown as ReturnType); + vi.mocked(buildLocalProxySpawnCommand).mockClear(); + + let healthChecks = 0; + vi.spyOn(globalThis, "fetch").mockImplementation((input: RequestInfo | URL) => { + const url = fetchInputUrl(input); + if (url.includes("/health")) { + healthChecks += 1; + if (healthChecks >= 2) { + return Promise.resolve( + new Response(JSON.stringify({ version: "test" }), { status: 200 }), + ); + } + } + return Promise.reject(new Error("connection refused")); + }); + }); + + afterEach(() => { + vi.restoreAllMocks(); + if (existsSync(testMulticornHome)) { + rmSync(testMulticornHome, { recursive: true, force: true }); + } + }); + + it("passes the resolved api base URL into SHIELD_API_BASE_URL when spawning the proxy", async () => { + await ensureProxyForTests(59997, "http://localhost:8080"); + + expect(vi.mocked(buildLocalProxySpawnCommand)).toHaveBeenCalledWith( + 59997, + "http://localhost:8080", + ); + const spawnCmd = vi.mocked(buildLocalProxySpawnCommand).mock.results[0]?.value as + | { env: { SHIELD_API_BASE_URL: string } } + | undefined; + expect(spawnCmd?.env.SHIELD_API_BASE_URL).toBe("http://localhost:8080"); + }); +}); + +describe("ensureProxy forceRespawn (Fix 2: restart respawns stale proxy)", () => { + let ensureProxyForTests: ( + proxyPort: number, + apiBaseUrl: string, + options?: { forceRespawn?: boolean }, + ) => Promise<{ reused: boolean }>; + + beforeEach(async () => { + vi.resetModules(); + ({ ensureProxyForTests } = await import("./files.js")); + + mkdirSync(testMulticornHome, { recursive: true }); + writeFileSync( + join(testMulticornHome, "proxy.json"), + JSON.stringify({ pid: DEAD_PID, port: 59996 }), + "utf8", + ); + + const fakeChild = new EventEmitter() as EventEmitter & { pid?: number; unref: () => void }; + fakeChild.pid = 6161; + fakeChild.unref = vi.fn(); + vi.mocked(spawn).mockReturnValue(fakeChild as unknown as ReturnType); + vi.mocked(buildLocalProxySpawnCommand).mockClear(); + + vi.spyOn(globalThis, "fetch").mockImplementation((input: RequestInfo | URL) => { + const url = fetchInputUrl(input); + if (url.includes("/health")) { + return Promise.resolve(new Response(JSON.stringify({ version: "test" }), { status: 200 })); + } + return Promise.reject(new Error("unexpected fetch")); + }); + }); + + afterEach(() => { + vi.restoreAllMocks(); + if (existsSync(testMulticornHome)) { + rmSync(testMulticornHome, { recursive: true, force: true }); + } + }); + + it("reuses a healthy proxy by default", async () => { + const result = await ensureProxyForTests(59996, "http://localhost:8080"); + expect(result.reused).toBe(true); + expect(vi.mocked(spawn)).not.toHaveBeenCalled(); + }); + + it("respawns the proxy when forceRespawn is set even if /health succeeds", async () => { + const result = await ensureProxyForTests(59996, "http://localhost:8080", { + forceRespawn: true, + }); + + expect(result.reused).toBe(false); + expect(vi.mocked(buildLocalProxySpawnCommand)).toHaveBeenCalledWith( + 59996, + "http://localhost:8080", + ); + }); +}); + +describe("isAgentSessionRunning (Fix 3: stale pidfile)", () => { + let isAgentSessionRunning: typeof FilesModule.isAgentSessionRunning; + let supervisorPidFromSessionForTests: typeof FilesModule.supervisorPidFromSessionForTests; + + beforeAll(async () => { + ({ isAgentSessionRunning, supervisorPidFromSessionForTests } = await import("./files.js")); + }); + + it("returns false when the supervisor pid is dead", () => { + expect( + isAgentSessionRunning({ + agent: "test-agent", + dir: "/tmp/repo", + supervisorPid: DEAD_PID, + fsPort: 3005, + proxyPort: 3001, + }), + ).toBe(false); + }); + + it("returns true only when the supervisor pid is alive", () => { + expect( + isAgentSessionRunning({ + agent: "test-agent", + dir: "/tmp/repo", + supervisorPid: process.pid, + fsPort: 3005, + proxyPort: 3001, + }), + ).toBe(true); + }); + + it("honours legacy pidfiles that stored the supervisor pid under pid", () => { + expect( + supervisorPidFromSessionForTests({ + agent: "test-agent", + dir: "/tmp/repo", + supervisorPid: undefined as unknown as number, + pid: DEAD_PID, + fsPort: 3005, + proxyPort: 3001, + }), + ).toBe(DEAD_PID); + expect( + isAgentSessionRunning({ + agent: "test-agent", + dir: "/tmp/repo", + supervisorPid: undefined as unknown as number, + pid: DEAD_PID, + fsPort: 3005, + proxyPort: 3001, + } as Parameters[0]), + ).toBe(false); + }); + + it("reaps a stale pidfile on disk before a new start would honour it", () => { + mkdirSync(testMulticornHome, { recursive: true }); + const pidfile = join(testMulticornHome, "files-stale-agent.pid"); + writeFileSync( + pidfile, + JSON.stringify({ + agent: "stale-agent", + dir: "/tmp/repo", + supervisorPid: DEAD_PID, + fsPort: 3005, + proxyPort: 3001, + }), + "utf8", + ); + + const data = JSON.parse(readFileSync(pidfile, "utf8")) as Parameters< + typeof isAgentSessionRunning + >[0]; + expect(isAgentSessionRunning(data)).toBe(false); + + rmSync(testMulticornHome, { recursive: true, force: true }); + }); +}); diff --git a/src/commands/files.ts b/src/commands/files.ts index 2fa6c8f..dcfa217 100644 --- a/src/commands/files.ts +++ b/src/commands/files.ts @@ -25,6 +25,7 @@ import { createInterface } from "node:readline"; import { loadConfig, + readBaseUrlFromConfig, DEFAULT_SHIELD_API_BASE_URL, isAllowedShieldApiBaseUrl, detectInstalledClients, @@ -60,6 +61,8 @@ export interface FilesCommandOptions { // Stop (if running) then start again. Start re-derives the agent's MCP config // entry every run, so restart is the universal remedy for a stale on-disk entry. readonly restart: boolean; + /** When true, kill and respawn the shared proxy even if one is already healthy. */ + readonly respawnProxy: boolean; } interface PidfileData { @@ -75,6 +78,9 @@ interface PidfileData { readonly proxyPort: number; } +/** Legacy pidfiles stored the supervisor pid under `pid` before supervisorPid existed. */ +type PidfileDataOnDisk = PidfileData & { readonly pid?: number }; + // One shared proxy across all local agents. interface ProxyRegistry { readonly pid: number; @@ -145,12 +151,25 @@ function readPidfile(agent: string): PidfileData | null { const p = pidfilePath(agent); if (!existsSync(p)) return null; try { - return JSON.parse(readFileSync(p, "utf8")) as PidfileData; + return JSON.parse(readFileSync(p, "utf8")) as PidfileDataOnDisk; } catch { return null; } } +function supervisorPidFromSession(data: PidfileDataOnDisk): number | undefined { + if (typeof data.supervisorPid === "number") return data.supervisorPid; + if (typeof data.pid === "number") return data.pid; + return undefined; +} + +/** True only when the pidfile's supervisor process is still alive. */ +export function isAgentSessionRunning(data: PidfileData | null): boolean { + if (data === null) return false; + const pid = supervisorPidFromSession(data as PidfileDataOnDisk); + return typeof pid === "number" && isProcessAlive(pid); +} + function removePidfile(agent: string): void { const p = pidfilePath(agent); try { @@ -198,7 +217,7 @@ function runStatus(): void { process.stderr.write("Active sessions:\n\n"); for (const s of sessions) { - const supervisorAlive = typeof s.supervisorPid === "number" && isProcessAlive(s.supervisorPid); + const supervisorAlive = isAgentSessionRunning(s); const fsEntry = fsReg[s.dir]; const fsAlive = fsEntry !== undefined && isProcessAlive(fsEntry.pid); const proxyAlive = @@ -400,9 +419,7 @@ async function withResourceLock(fn: () => T | Promise): Promise { /** Agents whose supervisor process is still alive. A dead supervisor pins nothing. */ function liveAgents(): PidfileData[] { - return listAllPidfiles().filter( - (p) => typeof p.supervisorPid === "number" && isProcessAlive(p.supervisorPid), - ); + return listAllPidfiles().filter((p) => isAgentSessionRunning(p)); } export function agentsReferencingProxy( @@ -455,30 +472,50 @@ interface ResolvedConfig { readonly baseUrl: string; } +/** + * Shield API base URL for `files`: `--base-url` > `MULTICORN_BASE_URL` > + * config.json `baseUrl` > production default. + */ +export async function resolveBaseUrl(explicitBaseUrl?: string): Promise { + if (explicitBaseUrl !== undefined && explicitBaseUrl.trim().length > 0) { + return explicitBaseUrl.trim(); + } + + const envBaseUrl = process.env["MULTICORN_BASE_URL"]; + if (typeof envBaseUrl === "string" && envBaseUrl.trim().length > 0) { + return envBaseUrl.trim(); + } + + const config = await loadConfig(); + if (config !== null && config.baseUrl.length > 0) { + return config.baseUrl; + } + + const fromFile = await readBaseUrlFromConfig(); + if (fromFile !== undefined && fromFile.length > 0) { + return fromFile; + } + + return DEFAULT_SHIELD_API_BASE_URL; +} + async function resolveConfig(opts: FilesCommandOptions): Promise { + const baseUrl = await resolveBaseUrl(opts.baseUrl); + // Priority: --api-key > env > config file const fromFlag = opts.apiKey; if (fromFlag && fromFlag.length > 0) { - return { - apiKey: fromFlag, - baseUrl: opts.baseUrl ?? DEFAULT_SHIELD_API_BASE_URL, - }; + return { apiKey: fromFlag, baseUrl }; } const envKey = process.env["MULTICORN_API_KEY"]; if (typeof envKey === "string" && envKey.length > 0) { - return { - apiKey: envKey, - baseUrl: opts.baseUrl ?? DEFAULT_SHIELD_API_BASE_URL, - }; + return { apiKey: envKey, baseUrl }; } const config = await loadConfig(); if (config !== null) { - return { - apiKey: config.apiKey, - baseUrl: opts.baseUrl ?? config.baseUrl, - }; + return { apiKey: config.apiKey, baseUrl }; } process.stderr.write( @@ -568,8 +605,34 @@ interface EnsureProxyResult { readonly managed: boolean; } -async function ensureProxy(proxyPort: number, apiBaseUrl: string): Promise { - if (await probeProxyHealth(proxyPort)) { +interface EnsureProxyOptions { + readonly forceRespawn?: boolean; +} + +async function ensureProxy( + proxyPort: number, + apiBaseUrl: string, + options?: EnsureProxyOptions, +): Promise { + const forceRespawn = options?.forceRespawn === true; + + if (forceRespawn) { + const reg = readProxyRegistry(); + if (reg !== null && reg.port === proxyPort && isProcessAlive(reg.pid)) { + killWithEscalation(reg.pid, true); + } + if (reg !== null && reg.port === proxyPort) { + try { + unlinkSync(PROXY_REGISTRY); + } catch { + // ignore + } + } + for (let i = 0; i < 20; i++) { + if (!(await isPortListening(proxyPort))) break; + await sleep(100); + } + } else if (await probeProxyHealth(proxyPort)) { const reg = readProxyRegistry(); const managed = reg !== null && reg.port === proxyPort && isProcessAlive(reg.pid); return { reused: true, managed }; @@ -934,8 +997,9 @@ async function runStop(agent: string): Promise { // sees the first already gone. await withResourceLock(() => { // Stop this agent's supervisor (the heartbeat process). - if (typeof data.supervisorPid === "number" && isProcessAlive(data.supervisorPid)) { - killWithEscalation(data.supervisorPid); + const supervisorPid = supervisorPidFromSession(data as PidfileDataOnDisk); + if (typeof supervisorPid === "number" && isProcessAlive(supervisorPid)) { + killWithEscalation(supervisorPid); } removePidfile(agent); @@ -991,6 +1055,7 @@ async function runRestart(opts: FilesCommandOptions): Promise { stop: false, status: false, restart: false, + respawnProxy: true, foreground: false, }); } @@ -1009,9 +1074,7 @@ async function runDetached(opts: FilesCommandOptions): Promise { // Check if already running (supervisor alive for this agent). const existing = readPidfile(opts.agent); if (existing !== null) { - const alive = - typeof existing.supervisorPid === "number" && isProcessAlive(existing.supervisorPid); - if (alive) { + if (isAgentSessionRunning(existing)) { process.stderr.write( `Already running for agent "${opts.agent}" (fs :${String(existing.fsPort)}, proxy :${String(existing.proxyPort)}).\n` + `Stop with: npx multicorn-shield files stop --agent ${opts.agent}\n`, @@ -1027,6 +1090,7 @@ async function runDetached(opts: FilesCommandOptions): Promise { if (opts.proxyPort !== undefined) args.push("--proxy-port", String(opts.proxyPort)); if (opts.apiKey !== undefined) args.push("--api-key", opts.apiKey); if (opts.baseUrl !== undefined) args.push("--base-url", opts.baseUrl); + if (opts.respawnProxy) args.push("--respawn-proxy"); if (opts.client !== undefined) args.push("--client", opts.client); // Use the same script that was invoked (process.argv[1]) @@ -1136,10 +1200,7 @@ export async function runFilesCommand(opts: FilesCommandOptions): Promise // Already running for this agent? const existingPidfile = readPidfile(opts.agent); if (existingPidfile !== null) { - const supervisorAlive = - typeof existingPidfile.supervisorPid === "number" && - isProcessAlive(existingPidfile.supervisorPid); - if (supervisorAlive) { + if (isAgentSessionRunning(existingPidfile)) { process.stderr.write( `A session for agent "${opts.agent}" is already running. ` + `Run 'files stop --agent ${opts.agent}' first, or use a different --agent name.\n`, @@ -1157,7 +1218,9 @@ export async function runFilesCommand(opts: FilesCommandOptions): Promise let fsReused: boolean; try { const ensured = await withResourceLock(async () => { - const proxyRes = await ensureProxy(proxyPort, config.baseUrl); + const proxyRes = await ensureProxy(proxyPort, config.baseUrl, { + forceRespawn: opts.respawnProxy, + }); const fsRes = await ensureFsServer(realDir, opts.port); writePidfile({ agent: opts.agent, @@ -1455,4 +1518,7 @@ function promptLine(question: string): Promise { } /** @internal Exposed for unit tests only. */ -export { ensureProxy as ensureProxyForTests }; +export { + ensureProxy as ensureProxyForTests, + supervisorPidFromSession as supervisorPidFromSessionForTests, +}; diff --git a/src/proxy/__tests__/proxy.cli-api-key.test.ts b/src/proxy/__tests__/proxy.cli-api-key.test.ts index 881e330..23af6c6 100644 --- a/src/proxy/__tests__/proxy.cli-api-key.test.ts +++ b/src/proxy/__tests__/proxy.cli-api-key.test.ts @@ -291,6 +291,7 @@ describe("resolveWrapConfig", () => { filesForeground: false, filesStatus: false, filesRestart: false, + filesRespawnProxy: false, ...overrides, }; }