From b52bbe902b3f107c7ef367f979a6d76e9be51cdb Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Thu, 20 Aug 2026 09:35:51 -0700 Subject: [PATCH] fix(runtime): adopt running pre-exposure services instead of quarantining them as drift The startup exposure-drift sweep classified any row without exposure state as dormant, including rows recorded RUNNING. A running pre-exposure service carries a plain port column with exposure null, so when that port sits inside the dedicated exposure app range (42000-42999) the sweep probed it, found the service's own live listener, could not attribute it (the in-memory runtime registry is empty at startup), and reported drift "recorded stopped/removed" for a row that was recorded running. Reconciliation then quarantined the row instead of adopting the surviving process. This is how the shell-command adoption test added in #11744 flaked on CI: it reserved its service port with listen(0), and on Linux the kernel's ephemeral span (32768-60999) contains the dedicated range, so roughly one run in thirty landed in-range and reconciliation returned adopted: 0. The same failure hit several unrelated PRs per day. It never reproduced on macOS, whose ephemeral range starts at 49152. Extract the dormancy decision into isDormantExposureRow and use it in both findExposureReservationDrift and the candidate-port filter of detectPersistedExposureReservationDrift: stopped/failed rows and rows whose exposure was explicitly torn down keep exactly today's behavior; a running row with no exposure state is a live lane pending adoption, not a stale reservation to police. Provisioning/starting rows keep their current dormant treatment. The adoption test now reserves an in-range port on purpose (falling back to ephemeral only when the whole range is busy), turning the once-probabilistic collision into the case the test always exercises. Two unit tests pin the classification: a running pre-exposure row's own in-range listener is not drift; a stopped pre-exposure row's pair is still swept. Verification: reproduced the CI failure deterministically on macOS by forcing the in-range port pre-fix (identical adopted: 0 assertion and drift warning as CI); post-fix the test passes 5/5 repeat runs. port-reservation.test.ts 36/36, PAP-17419 reservation suite 7/7, workspace-runtime.test.ts and workspace-runtime-exposure.test.ts show no delta against a clean-master baseline on the same host. Server typecheck passes. --- .../src/__tests__/workspace-runtime.test.ts | 53 +++++++++++++++---- .../runtime-exposure/port-reservation.test.ts | 23 ++++++++ .../runtime-exposure/port-reservation.ts | 21 +++++++- server/src/services/workspace-runtime.ts | 10 ++-- 4 files changed, 91 insertions(+), 16 deletions(-) diff --git a/server/src/__tests__/workspace-runtime.test.ts b/server/src/__tests__/workspace-runtime.test.ts index 7f902c85de4..7da3f5e2447 100644 --- a/server/src/__tests__/workspace-runtime.test.ts +++ b/server/src/__tests__/workspace-runtime.test.ts @@ -61,7 +61,13 @@ import { buildWorkspaceRealizationRequest, readWorkspaceRealizationRequest, } from "../services/workspace-realization.ts"; -import { deriveViteHmrPort, type Environment, type EnvironmentLease } from "@paperclipai/shared"; +import { + deriveViteHmrPort, + RUNTIME_EXPOSURE_APP_PORT_MAX, + RUNTIME_EXPOSURE_APP_PORT_MIN, + type Environment, + type EnvironmentLease, +} from "@paperclipai/shared"; import { resolvePaperclipConfigPath } from "../paths.ts"; import type { WorkspaceOperation } from "@paperclipai/shared"; import type { WorkspaceOperationRecorder } from "../services/workspace-operations.ts"; @@ -7454,6 +7460,42 @@ describeEmbeddedPostgres("workspace runtime startup reconciliation", () => { await expect(fetch(service!.url!)).rejects.toThrow(); }); + /** + * A free loopback port inside the dedicated runtime exposure app range, + * falling back to an OS-assigned ephemeral port when the whole range is + * busy on this host. + * + * The adoption test below used to reserve with `listen(0)`, and on Linux CI + * the kernel's ephemeral span contains the dedicated range, so roughly one + * run in thirty landed in it by accident — the exact shape that made the + * startup drift sweep quarantine the row instead of adopting it. Reserving + * in-range on purpose turns that once-probabilistic collision into the case + * the test always exercises. + */ + async function reserveAdoptionTestPort(): Promise { + for (let candidate = RUNTIME_EXPOSURE_APP_PORT_MIN; candidate <= RUNTIME_EXPOSURE_APP_PORT_MAX; candidate += 1) { + const probe = net.createServer(); + const bound = await new Promise((resolve) => { + probe.once("error", () => resolve(false)); + probe.listen(candidate, "127.0.0.1", () => resolve(true)); + }); + if (!bound) continue; + await new Promise((resolve, reject) => { + probe.close((error) => error ? reject(error) : resolve()); + }); + return candidate; + } + const fallback = net.createServer(); + await new Promise((resolve) => fallback.listen(0, "127.0.0.1", resolve)); + const address = fallback.address(); + const port = typeof address === "object" && address ? address.port : null; + await new Promise((resolve, reject) => { + fallback.close((error) => error ? reject(error) : resolve()); + }); + if (!port) throw new Error("Failed to reserve pnpm reconciliation test port"); + return port; + } + it("re-adopts a live service whose shell command differs from the surviving process argv", async () => { const workspaceRoot = await fs.mkdtemp(path.join(os.tmpdir(), "paperclip-runtime-pnpm-reconcile-")); const paperclipHome = await fs.mkdtemp(path.join(os.tmpdir(), "paperclip-runtime-home-")); @@ -7462,14 +7504,7 @@ describeEmbeddedPostgres("workspace runtime startup reconciliation", () => { process.env.PAPERCLIP_HOME = paperclipHome; process.env.PAPERCLIP_INSTANCE_ID = `runtime-pnpm-reconcile-${randomUUID()}`; - const portProbe = net.createServer(); - await new Promise((resolve) => portProbe.listen(0, "127.0.0.1", resolve)); - const address = portProbe.address(); - const port = typeof address === "object" && address ? address.port : null; - await new Promise((resolve, reject) => { - portProbe.close((error) => error ? reject(error) : resolve()); - }); - if (!port) throw new Error("Failed to reserve pnpm reconciliation test port"); + const port = await reserveAdoptionTestPort(); const companyId = randomUUID(); const projectId = randomUUID(); diff --git a/server/src/services/runtime-exposure/port-reservation.test.ts b/server/src/services/runtime-exposure/port-reservation.test.ts index 1ee65a21165..805a43bc6f6 100644 --- a/server/src/services/runtime-exposure/port-reservation.test.ts +++ b/server/src/services/runtime-exposure/port-reservation.test.ts @@ -320,6 +320,29 @@ describe("regression 3: Serve mapping ownership mismatch is visible and fails cl expect(drift).toEqual([]); }); + it("does not report a running pre-exposure service's own in-range port as drift", () => { + // The row is RUNNING with `exposure: null` — a pre-exposure service whose + // configured port happens to sit inside the dedicated range. At startup its + // surviving listener is unattributable (the in-memory runtime registry is + // empty), which used to read as drift and quarantine the row before the + // reconciler could adopt the service. + const drift = findExposureReservationDrift({ + persistedRows: [stoppedTornDownRow({ status: "running", exposure: null })], + livePorts: new Set([42001, 52001]), + }); + + expect(drift).toEqual([]); + }); + + it("still sweeps a stopped pre-exposure row's reserved pair", () => { + const drift = findExposureReservationDrift({ + persistedRows: [stoppedTornDownRow({ exposure: null })], + livePorts: new Set([42001]), + }); + + expect(drift).toMatchObject([{ port: 42001, reason: "live_listener", conflictingOwner: null }]); + }); + it("does not report a healthy running lane as drift", () => { const drift = findExposureReservationDrift({ persistedRows: [{ diff --git a/server/src/services/runtime-exposure/port-reservation.ts b/server/src/services/runtime-exposure/port-reservation.ts index 5fbb5659c2a..efb4da30877 100644 --- a/server/src/services/runtime-exposure/port-reservation.ts +++ b/server/src/services/runtime-exposure/port-reservation.ts @@ -185,6 +185,24 @@ function rowClaimsLiveExposure(row: PersistedExposureRowSnapshot): boolean { return Boolean(row.exposure) && row.exposure!.state !== "removed"; } +/** + * True when the row itself says nothing of its own should be listening: it is + * recorded stopped/failed, its exposure was explicitly torn down, or it never + * reached `running` and left only a reserved pair behind. + * + * A RUNNING row with no exposure state at all is the one shape that must not + * count as dormant. Pre-exposure services carry a plain `port` column with + * `exposure: null`, and when that port happens to sit inside the dedicated + * range, a dormant classification turns the row's own live listener into a + * "drift" finding — which the startup sweep then quarantines instead of + * adopting, orphaning a healthy surviving service. + */ +export function isDormantExposureRow(row: PersistedExposureRowSnapshot): boolean { + if (row.status === "stopped" || row.status === "failed") return true; + if (row.exposure) return row.exposure.state === "removed"; + return row.status !== "running"; +} + function rowIdentity(row: PersistedExposureRowSnapshot): ExposureOwnerIdentity { return { runtimeServiceId: row.id, @@ -459,8 +477,7 @@ export function findExposureReservationDrift( const drift: ExposureReservationDrift[] = []; for (const row of rows) { - const dormant = row.status === "stopped" || row.status === "failed" || !rowClaimsLiveExposure(row); - if (!dormant) continue; + if (!isDormantExposureRow(row)) continue; const owner = rowIdentity(row); for (const port of collectRowExposurePorts(row)) { const mappingOwner = mappingOwnerByPort.get(port) ?? null; diff --git a/server/src/services/workspace-runtime.ts b/server/src/services/workspace-runtime.ts index ade07864892..0e9983f902c 100644 --- a/server/src/services/workspace-runtime.ts +++ b/server/src/services/workspace-runtime.ts @@ -89,6 +89,7 @@ import { ExposurePortPairClaims, findExposurePairConflict, findExposureReservationDrift, + isDormantExposureRow, isExposureAdoptionPermitted, type BrokerMappingSnapshot, type ExposureOwnerIdentity, @@ -4066,13 +4067,12 @@ async function detectPersistedExposureReservationDrift(input: { issueId: row.issueId, })); - // Probe only the ports dormant rows actually reserve; a startup sweep must not - // walk the whole dedicated range. + // Probe only the ports dormant rows actually reserve; a startup sweep must + // not walk the whole dedicated range, and a running pre-exposure service's + // own port is not a reservation to police at all. const candidatePorts = new Set(); for (const row of snapshots) { - if (row.status !== "stopped" && row.status !== "failed" && row.exposure && row.exposure.state !== "removed") { - continue; - } + if (!isDormantExposureRow(row)) continue; for (const port of collectRowExposurePorts(row)) candidatePorts.add(port); }