From 1c29992171ca95e319383be27c31c40be88fb020 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Fri, 6 Nov 2026 17:50:22 -0600 Subject: [PATCH] Observe workspace service failures before asynchronous startup work (#19624) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Workspace runtime services start local child processes and wait for readiness. > - The runtime must return startup failures through its normal error and cleanup path. > - A child can fail while asynchronous log or registry work is pending. > - The previous code attached child listeners too late, or rejected promises before the readiness race observed them. > - This pull request records the child failure immediately and delivers it when the readiness race can observe it. > - The benefit is a preserved startup error without a process-wide unhandled rejection. ## Linked Issues or Issue Description **What happened?** A real child that exits during a pending registry write produces an unhandled rejection. A spawn error during a pending log close loses the original error and reaches the readiness timeout instead. This caused the General server 8/12 CI shard for #19576 to fail after all 2,285 test assertions passed. **Expected behavior** The startup operation returns the original child failure, removes its registry record, and produces no global unhandled rejection. **Steps to reproduce** Run the new readiness test against the base revision. It starts a real exit-1 shell while holding the registry write through child close. It also starts a missing shell while holding log close through the spawn error. The two behavioral checks fail before the production change. **Paperclip version or commit** da9a4cb96c, before this change. ## What Changed - Register child error and close listeners directly after spawn, before the first asynchronous boundary. - Keep the observed failure as a resolved outcome until the readiness race attaches. The race then rejects through the existing cleanup path. - Add real-process regressions for both asynchronous windows. Check the original error, no global unhandled rejection, process exit, and registry removal. ## Verification - Behavioral RED before production edits: two actual subprocess cases failed on the original code. A fast child exit while registry publication was held emitted an unhandled rejection; an ENOENT spawn while log close was held lost its original error and reached readiness timeout. - Final GREEN at `322ef0d21b3cd1811913bb5e15f47355ad5fa449`: six affected files, 72 passing tests and three existing platform skips (149.20s). Coverage includes both real failure windows, original operation errors, zero global unhandled rejections, child exit and registry cleanup, ordinary startup/stop, EADDRINUSE classification, bounded timeout/lease guards, six model fixtures and all 15 Slack owner cases. - All seven server typecheck layers, module-boundary and whitespace checks pass. Required preflight passes in 20.979s. The final diff contains only the two readiness owner/test files; no new code comments were added. - [Final-head CI](https://github.com/paperclipai/paperclip/actions/runs/42311878548): 56 successful checks, four intentional skips, no failures or pending checks. Required E2E and verify aggregates pass. Greptile reviewed this exact head at 5/5 with no unresolved findings. - The valid review finding about a late test subscription is fixed: the real spawn wrapper captures settlement before returning the child. The resolved thread and final test both preserve the original lifecycle oracle. - Prior inherited CI failures were repaired through normal merges of existing fixes: #19628 (duplicate workflow defaults), #19610 (model expectations), #19627 (deterministic Slack deadline fixture), and #19646 (current Runner lane flags). All four exact `--package-lane=1/3`, `2/3`, `3/3`, and `--native-server` dry runs and the lane-inventory regression pass. No CI job was replayed, timeout weakened, or production assertion changed to accommodate those failures. - Local environment: macOS arm64, Node 26.4.0, pinned pnpm 9.15.4, Vitest 5.0.3. Subprocess and loopback checks ran with the required local permissions. These checks do not claim a hosted deployment or full browser acceptance. ## Risks - Low risk. Child failures now become observable earlier. The existing readiness failure, timeout, collision classification, and cleanup owners remain in place. - The tests hold real filesystem boundaries through real child events. They do not model a full hosted deployment. ## Model Used - OpenAI Codex (GPT-6 family), with code execution and repository tools. The runtime does not expose the exact serving model identifier or context window size. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip --- .../workspace-runtime-readiness.test.ts | 105 ++++++++++++++++++ server/src/services/workspace-runtime.ts | 27 ++--- 2 files changed, 115 insertions(+), 17 deletions(-) create mode 100644 server/src/services/workspace-runtime-readiness.test.ts diff --git a/server/src/services/workspace-runtime-readiness.test.ts b/server/src/services/workspace-runtime-readiness.test.ts new file mode 100644 index 00000000000..5a0be4ee402 --- /dev/null +++ b/server/src/services/workspace-runtime-readiness.test.ts @@ -0,0 +1,105 @@ +import type { ChildProcess } from "node:child_process"; +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, expect, it, vi } from "vitest"; + +const boundary = vi.hoisted(() => ({ + child: null as ChildProcess | null, + hold: "registry" as "registry" | "log-close", + spawnErrors: [] as Error[], + settled: Promise.resolve(), +})); + +async function afterChildSettled() { + await boundary.settled; + await new Promise((resolve) => setImmediate(resolve)); +} + +vi.mock("node:child_process", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + spawn: (...args: Parameters) => { + const child = actual.spawn(...args); + boundary.child = child; + boundary.settled = new Promise((resolve) => { + child.once("error", (error) => { boundary.spawnErrors.push(error); resolve(); }); + child.once("close", () => resolve()); + }); + return child; + }, + }; +}); + +vi.mock("./local-service-supervisor.js", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + writeLocalServiceRegistryRecord: async (...args: Parameters) => { + if (boundary.hold === "registry") await afterChildSettled(); + return actual.writeLocalServiceRegistryRecord(...args); + }, + openLocalServiceLogFile: async (...args: Parameters) => { + const log = await actual.openLocalServiceLogFile(...args); + const close = log.handle.close.bind(log.handle); + log.handle.close = async () => { + if (boundary.hold === "log-close") await afterChildSettled(); + await close(); + }; + return log; + }, + }; +}); + +import { resolvePaperclipInstanceRoot } from "../home-paths.js"; +import { resetRuntimeServicesForTests, startRuntimeServicesForWorkspaceControl } from "./workspace-runtime.js"; + +let home: string | undefined; +afterEach(async () => { + await resetRuntimeServicesForTests({ terminateProcesses: true }); + vi.unstubAllEnvs(); + if (home) await fs.rm(home, { recursive: true, force: true }); +}); + +it.each([ + { hold: "registry" as const, shell: "/bin/sh", expected: /exited before readiness.*code 1/ }, + { hold: "log-close" as const, shell: "paperclip-missing-readiness-shell", expected: /ENOENT/ }, +])("keeps $hold child failure observed and reports the original startup error", async ({ hold, shell, expected }) => { + boundary.hold = hold; + boundary.spawnErrors = []; + home = await fs.mkdtemp(path.join(os.tmpdir(), "paperclip-readiness-")); + vi.stubEnv("PAPERCLIP_HOME", home); + vi.stubEnv("PAPERCLIP_MANAGED_RUNTIME_HTTPS", "off"); + vi.stubEnv("SHELL", shell); + const unhandled: unknown[] = []; + const observe = (error: unknown) => { unhandled.push(error); }; + process.on("unhandledRejection", observe); + try { + const result = await startRuntimeServicesForWorkspaceControl({ + invocationId: "readiness-rejection-test", + actor: { id: null, name: "Paperclip", companyId: "aaaaaaaa-bbbb-4ccc-8ddd-eeeeeeeeeeee" }, + issue: null, + workspace: { + baseCwd: home, cwd: home, source: "project_primary", projectId: null, + workspaceId: null, repoUrl: null, repoRef: null, strategy: "project_primary", + branchName: "test", worktreePath: null, warnings: [], created: false, + branchCreatedByRuntime: false, + }, + executionWorkspaceId: "11111111-2222-4333-8444-555566667777", + config: { workspaceRuntime: { services: [{ name: "preview", command: "exit 1", + readiness: { type: "http", urlTemplate: "http://127.0.0.1:19999", timeoutSec: 1 }, + }] } }, + adapterEnv: {}, + }).then(() => null, (error: unknown) => error as Error); + expect(result).toBeInstanceOf(Error); + expect(result!.message).toMatch(expected); + expect(unhandled).toEqual([]); + const registry = await fs.readdir(path.join(resolvePaperclipInstanceRoot(), "runtime-services")).catch(() => []); + expect(registry.filter((name) => name.endsWith(".json"))).toEqual([]); + if (hold === "registry") expect(boundary.child!.exitCode).toBe(1); + if (hold === "log-close") expect(boundary.spawnErrors).toHaveLength(1); + } finally { + process.removeListener("unhandledRejection", observe); + } +}); diff --git a/server/src/services/workspace-runtime.ts b/server/src/services/workspace-runtime.ts index b9bdfcfdda3..c617107f48b 100644 --- a/server/src/services/workspace-runtime.ts +++ b/server/src/services/workspace-runtime.ts @@ -6688,6 +6688,7 @@ async function spawnLocalRuntimeService(input: StartLocalRuntimeServiceInput): P const shell = resolveShell(); const serviceLog = await openLocalServiceLogFile(serviceKey); let child: ChildProcess; + let childFailure: Promise; try { child = spawn(shell, ["-lc", command], { cwd: serviceCwd, @@ -6698,27 +6699,20 @@ async function spawnLocalRuntimeService(input: StartLocalRuntimeServiceInput): P // on an orphaned socketpair during startup reconciliation. stdio: ["ignore", serviceLog.handle.fd, serviceLog.handle.fd], }); + childFailure = new Promise((resolve) => { + child.once("error", resolve); + child.once("close", (code, signal) => { + resolve(new Error( + `service process exited before readiness (code ${code ?? "unknown"}, signal ${signal ?? "none"})`, + )); + }); + }); } finally { await serviceLog.handle.close(); } record.child = child; record.providerRef = child.pid ? String(child.pid) : null; record.processGroupId = child.pid ?? null; - const spawnErrorPromise = new Promise((_, reject) => { - child.once("error", (err) => { - reject(err); - }); - }); - const earlyExitPromise = new Promise((_, reject) => { - // `close` follows `exit` after the child's inherited stdout/stderr file - // descriptors are closed. Waiting for it makes the startup log excerpt - // deterministic instead of racing the final validation line. - child.once("close", (code, signal) => { - reject(new Error( - `service process exited before readiness (code ${code ?? "unknown"}, signal ${signal ?? "none"})`, - )); - }); - }); const readServiceOutputExcerpt = async () => { try { const contents = await fs.readFile(serviceLog.logPath); @@ -6784,8 +6778,7 @@ async function spawnLocalRuntimeService(input: StartLocalRuntimeServiceInput): P waitForAllocatedPortBind({ service: input.service, port: ownedPort, child }) ), ]).then(() => undefined), - spawnErrorPromise, - earlyExitPromise, + childFailure.then((error) => { throw error; }), ]).then(async () => { releasePortReservation(reservedPort); releasePortReservation(claimedIdentityPort);