diff --git a/scripts/lib/real-hopper-cleanup.d.mts b/scripts/lib/real-hopper-cleanup.d.mts new file mode 100644 index 000000000..8ccc9e318 --- /dev/null +++ b/scripts/lib/real-hopper-cleanup.d.mts @@ -0,0 +1,6 @@ +/** Observe concrete Hopper runtime resources for a before/after cleanup check. */ +export function snapshotHopperRuntime( + parent: string, + targetLeaseDirectory: string, + ownedProcessIds?: ReadonlySet, +): Promise>; diff --git a/scripts/lib/real-hopper-cleanup.mjs b/scripts/lib/real-hopper-cleanup.mjs new file mode 100644 index 000000000..85d0a8f33 --- /dev/null +++ b/scripts/lib/real-hopper-cleanup.mjs @@ -0,0 +1,52 @@ +import { lstat, readFile, readdir } from "node:fs/promises"; +import { join } from "node:path"; + +/** Observe session paths and target leases without treating their shared parent as a session. */ +export async function snapshotHopperRuntime( + parent, + targetLeaseDirectory, + ownedProcessIds, +) { + const resources = new Set(); + for (const entry of await readdir(parent, { withFileTypes: true })) { + const path = join(parent, entry.name); + if (!entry.name.startsWith("rea-") || path === targetLeaseDirectory) + continue; + if ( + ownedProcessIds !== undefined && + (!entry.isDirectory() || + !(await belongsToOwnedProcess(path, ownedProcessIds))) + ) + continue; + resources.add(path); + } + const leaseRoot = await lstat(targetLeaseDirectory).catch(missingIsAbsent); + if (leaseRoot === undefined) return resources; + if (!leaseRoot.isDirectory() || leaseRoot.isSymbolicLink()) { + resources.add(targetLeaseDirectory); + return resources; + } + const leases = await readdir(targetLeaseDirectory).catch(missingIsAbsent); + for (const name of leases ?? []) + resources.add(join(targetLeaseDirectory, name)); + return resources; +} + +const missingIsAbsent = (cause) => { + if (cause?.code === "ENOENT") return undefined; + throw cause; +}; + +const belongsToOwnedProcess = async (path, ownedProcessIds) => { + const encoded = await readFile(join(path, "ownership.json"), "utf8").catch( + missingIsAbsent, + ); + if (encoded === undefined) return false; + const ownership = JSON.parse(encoded); + return ( + ownership !== null && + typeof ownership === "object" && + Number.isSafeInteger(ownership.parent_pid) && + ownedProcessIds.has(ownership.parent_pid) + ); +}; diff --git a/scripts/verify-real-hopper.mjs b/scripts/verify-real-hopper.mjs index 621cc2359..7181c4371 100644 --- a/scripts/verify-real-hopper.mjs +++ b/scripts/verify-real-hopper.mjs @@ -1,7 +1,5 @@ import { execFile } from "node:child_process"; -import { readFile, readdir } from "node:fs/promises"; import { tmpdir } from "node:os"; -import { join } from "node:path"; import { promisify } from "node:util"; import { Client } from "@modelcontextprotocol/client"; @@ -14,6 +12,7 @@ import { requirePseudocode, } from "../dist/application/RealHopperAssertions.js"; import { HOPPER_PROVIDER_IDENTITY } from "../dist/hopper/HopperProvider.js"; +import { HOPPER_TARGET_LEASE_DIRECTORY } from "../dist/hopper/HopperTargetLease.js"; import { REA_WORKFLOW_PROVIDER } from "../dist/application/InvestigationProviders.js"; import { loadRealHopperFixtureTargets } from "./lib/real-hopper-fixture.mjs"; import { @@ -40,6 +39,7 @@ import { verifyHopperLifecycleAndCli, } from "./lib/real-hopper-boundaries.mjs"; import { startUnrelatedHopperSentinel } from "./lib/unrelated-hopper-sentinel.mjs"; +import { snapshotHopperRuntime } from "./lib/real-hopper-cleanup.mjs"; const execFileAsync = promisify(execFile); const verifierRun = createVerifierRun(); const timeout = 180_000; @@ -54,7 +54,10 @@ const parseServerArgs = (encoded) => { }; const runtimeParent = process.platform === "darwin" ? "/tmp" : tmpdir(); -const sessionsBefore = new Set(await readdir(runtimeParent)); +const sessionsBefore = await snapshotHopperRuntime( + runtimeParent, + HOPPER_TARGET_LEASE_DIRECTORY, +); const ownedProcessIds = new Set(); const textValue = mcpTextValue; const requireSuccessfulTool = requireMcpResult; @@ -519,26 +522,25 @@ try { } await new Promise((resolve) => setTimeout(resolve, 500)); -for (const entry of await readdir(runtimeParent, { withFileTypes: true })) { - if ( - !entry.isDirectory() || - !entry.name.startsWith("rea-") || - sessionsBefore.has(entry.name) - ) - continue; - let ownership; - try { - ownership = JSON.parse( - await readFile(join(runtimeParent, entry.name, "ownership.json"), "utf8"), - ); - } catch (cause) { - if (cause.code === "ENOENT") continue; - throw cause; - } - if (ownedProcessIds.has(ownership.parent_pid)) - throw new Error( - `An owned Hopper runtime leaked its directory: ${entry.name}`, - ); +const sessionsAfter = [ + ...(await snapshotHopperRuntime( + runtimeParent, + HOPPER_TARGET_LEASE_DIRECTORY, + ownedProcessIds, + )), +] + .filter((path) => !sessionsBefore.has(path)) + .sort(); +if (sessionsAfter.length > 0) { + await writeVerificationReport({ + ...summary, + unrelatedHopperSurvived, + cleanShutdown: false, + retainedRuntimePaths: sessionsAfter, + }); + throw new Error( + `The MCP runtime retained Hopper resources: ${sessionsAfter.join(", ")}`, + ); } if (summary === undefined) throw new Error("Real-Hopper verification did not produce a summary"); @@ -550,12 +552,18 @@ if ( throw new Error( "The verifier's process lineage did not prove owned child cleanup", ); -await new Promise((resolve, reject) => { - process.stdout.write( - `${JSON.stringify({ verifier_run: completedVerifierRun, ...summary, unrelatedHopperSurvived, cleanShutdown: true }, null, 2)}\n`, - (cause) => { +await writeVerificationReport({ + verifier_run: completedVerifierRun, + ...summary, + unrelatedHopperSurvived, + cleanShutdown: true, +}); + +async function writeVerificationReport(report) { + await new Promise((resolve, reject) => { + process.stdout.write(`${JSON.stringify(report, null, 2)}\n`, (cause) => { if (cause) reject(cause); else resolve(); - }, - ); -}); + }); + }); +} diff --git a/src/hopper/HopperTargetLease.ts b/src/hopper/HopperTargetLease.ts index d07518781..3d5d808e6 100644 --- a/src/hopper/HopperTargetLease.ts +++ b/src/hopper/HopperTargetLease.ts @@ -23,7 +23,11 @@ export type HopperTargetLeaseAcquisition = | { readonly acquired: true; readonly lease: HopperTargetLease } | { readonly acquired: false; readonly owner: HopperTargetLeaseOwner }; -const leaseDirectory = join("/tmp", `rea-hopper-${process.getuid?.() ?? 0}`); +/** Shared private parent of per-target lease sockets; it outlives individual sessions. */ +export const HOPPER_TARGET_LEASE_DIRECTORY = join( + "/tmp", + `rea-hopper-${process.getuid?.() ?? 0}`, +); /** Keep two REA processes from opening duplicate Hopper documents for one target/profile. */ export const acquireHopperTargetLease = async (input: { @@ -33,7 +37,7 @@ export const acquireHopperTargetLease = async (input: { readonly runId: string; readonly directory?: string; }): Promise => { - const directory = input.directory ?? leaseDirectory; + const directory = input.directory ?? HOPPER_TARGET_LEASE_DIRECTORY; await ensureLeaseDirectory(directory); // The canonical path only derives the lease key; it never authorizes access. // Common expected `realpath` failures include a missing target or path diff --git a/tests/boundary/providers/hopper/hopperVerificationCleanup.test.ts b/tests/boundary/providers/hopper/hopperVerificationCleanup.test.ts new file mode 100644 index 000000000..488ac40db --- /dev/null +++ b/tests/boundary/providers/hopper/hopperVerificationCleanup.test.ts @@ -0,0 +1,103 @@ +import { mkdir, mkdtemp, readdir, rm, writeFile } from "node:fs/promises"; +import { join } from "node:path"; +import { expect, it, onTestFinished } from "vitest"; + +import { snapshotHopperRuntime } from "../../../../scripts/lib/real-hopper-cleanup.mjs"; +import { acquireHopperTargetLease } from "../../../../src/hopper/HopperTargetLease.js"; +import { createTestTempDirectory } from "../../../fixtures/temporaryDirectory.js"; + +it.skipIf(process.platform === "win32")( + "does not classify a released shared lease directory as a session leak", + async () => { + const parent = await createLeaseWorkspace(); + const directory = join(parent, "rea-hopper-501"); + const targetPath = join(parent, "target"); + await writeFile(targetPath, "source-owned lease target"); + const before = await snapshotHopperRuntime(parent, directory); + const acquired = await acquireHopperTargetLease({ + targetPath, + targetKind: "executable", + loaderArgs: [], + runId: "released-verifier-lease", + directory, + }); + if (!acquired.acquired) throw new Error("Expected the owned target lease"); + await acquired.lease.release(); + + expect(await readdir(directory)).toEqual([]); + expect(await snapshotHopperRuntime(parent, directory)).toEqual(before); + }, +); + +it.skipIf(process.platform === "win32")( + "retains an unreleased real target-lease socket in cleanup observations", + async () => { + const parent = await createLeaseWorkspace(); + const directory = join(parent, "rea-hopper-501"); + const targetPath = join(parent, "target"); + await writeFile(targetPath, "source-owned lease target"); + const acquired = await acquireHopperTargetLease({ + targetPath, + targetKind: "executable", + loaderArgs: [], + runId: "retained-verifier-lease", + directory, + }); + if (!acquired.acquired) throw new Error("Expected the owned target lease"); + try { + const children = await readdir(directory); + expect(children).toHaveLength(1); + expect(await snapshotHopperRuntime(parent, directory)).toEqual( + new Set(children.map((name) => join(directory, name))), + ); + } finally { + await acquired.lease.release(); + } + }, +); + +it("reports retained sessions belonging to observed MCP processes", async () => { + const parent = await createTestTempDirectory("rea-cleanup-retained-session-"); + const directory = join(parent, "rea-hopper-501"); + const previous = join(parent, "rea-existing-session"); + await mkdir(previous); + const before = await snapshotHopperRuntime(parent, directory); + const retained = join(parent, "rea-retained-session"); + const unrelated = join(parent, "rea-unrelated-session"); + const unknown = join(parent, "rea-no-ownership-record"); + for (const path of [retained, unrelated, unknown]) await mkdir(path); + await writeFile(join(retained, "bootstrap.py"), "source-owned fixture"); + await writeFile( + join(retained, "ownership.json"), + JSON.stringify({ parent_pid: process.pid }), + ); + await writeFile( + join(unrelated, "ownership.json"), + JSON.stringify({ parent_pid: process.pid + 1 }), + ); + const after = await snapshotHopperRuntime( + parent, + directory, + new Set([process.pid]), + ); + + expect([...after].filter((path) => !before.has(path))).toEqual([retained]); +}); + +it("reports an unexpected file at the shared lease root", async () => { + const parent = await createTestTempDirectory("rea-cleanup-invalid-lease-"); + const directory = join(parent, "rea-hopper-501"); + await writeFile(directory, "unexpected lease root file"); + + expect(await snapshotHopperRuntime(parent, directory)).toEqual( + new Set([directory]), + ); +}); + +const createLeaseWorkspace = async (): Promise => { + // Unix socket paths must fit the host sockaddr_un limit, independently of + // the test runner's potentially long redirected temporary directory. + const parent = await mkdtemp("/tmp/rhv-"); + onTestFinished(() => rm(parent, { recursive: true, force: true })); + return parent; +}; diff --git a/tests/process-global/curatedWindowsLane.test.ts b/tests/process-global/curatedWindowsLane.test.ts index 718cca5f6..6f3b897ff 100644 --- a/tests/process-global/curatedWindowsLane.test.ts +++ b/tests/process-global/curatedWindowsLane.test.ts @@ -4,23 +4,48 @@ import { join } from "node:path"; import { describe, expect, it } from "vitest"; import { parse } from "yaml"; +import { z } from "zod"; const WORKFLOW = join(".github", "workflows", "ci.yml"); -const curatedPaths = async (): Promise => { - const workflow = parse(await readFile(WORKFLOW, "utf8")) as { - jobs?: Record; - }; - const run = workflow.jobs?.["windows-curated"]?.steps - ?.map((step) => step.run) - .find((value) => value?.includes("npx vitest run")); - expect(run, "expected a curated Windows vitest invocation").toBeDefined(); - return ( - run - ?.split(/\s+/) - .filter((token) => token.endsWith(".test.ts")) - .sort() ?? [] +const workflowSchema = z.object({ + jobs: z.record( + z.string(), + z.object({ + steps: z.array(z.object({ run: z.string().optional() })).optional(), + }), + ), +}); + +const explicitPaths = async ( + lane: "windows" | "focused", +): Promise => { + const workflow = workflowSchema.parse( + parse(await readFile(WORKFLOW, "utf8")), ); + const runs = + lane === "windows" + ? (workflow.jobs["windows-curated"]?.steps + ?.map((step) => step.run) + .filter((value) => value?.includes("npx vitest run")) ?? []) + : Object.values(workflow.jobs) + .flatMap((job) => job.steps ?? []) + .map((step) => step.run) + .filter((value) => + value?.trim().startsWith("npm run test:focused -- "), + ); + expect( + runs.length, + `expected explicit ${lane} CI test invocations`, + ).toBeGreaterThan(0); + return [ + ...new Set( + runs.flatMap( + (run) => + run?.split(/\s+/).filter((token) => token.endsWith(".test.ts")) ?? [], + ), + ), + ].sort(); }; const exists = async (path: string): Promise => @@ -31,7 +56,7 @@ const exists = async (path: string): Promise => describe("curated Windows test lane", () => { it("references only test files that exist", async () => { - const paths = await curatedPaths(); + const paths = await explicitPaths("windows"); expect(paths.length).toBeGreaterThan(0); const missing = await Promise.all( paths.map(async (path) => ((await exists(path)) ? undefined : path)), @@ -39,3 +64,12 @@ describe("curated Windows test lane", () => { expect(missing.filter((path) => path !== undefined)).toEqual([]); }, 10_000); }); + +it("keeps every explicit focused CI test path executable after file moves", async () => { + const paths = await explicitPaths("focused"); + expect(paths.length).toBeGreaterThan(0); + const missing = await Promise.all( + paths.map(async (path) => ((await exists(path)) ? undefined : path)), + ); + expect(missing.filter((path) => path !== undefined)).toEqual([]); +}, 10_000);