From c1204c1750c9269eccf1a27774926bbd6c5cf02a Mon Sep 17 00:00:00 2001 From: Rachelle Date: Mon, 29 Jun 2026 20:56:38 +1000 Subject: [PATCH] fix(files): resolve base URL for proxy spawn, respawn on restart, reap stale pidfiles Local files setup was registering agents against one API base while the spawned proxy defaulted to production, causing 401s with locally minted keys. Restart also reused a healthy proxy with the old base URL, and dead supervisors could leave pidfiles that blocked clean starts. Co-authored-by: Cursor --- bin/multicorn-shield.ts | 8 + src/commands/files-defect-fixes.test.ts | 327 ++++++++++++++++++ src/commands/files.ts | 126 +++++-- src/proxy/__tests__/proxy.cli-api-key.test.ts | 1 + 4 files changed, 432 insertions(+), 30 deletions(-) create mode 100644 src/commands/files-defect-fixes.test.ts 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, }; }