From 3f7811eef06fad33469ec835d53d1272bda68e35 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Mon, 2 Nov 2026 11:00:42 -0600 Subject: [PATCH] fix(runner): keep requests responsive during retired backup cleanup (#18619) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Remote native runs keep durable harness backups for continuation and recovery. > - Publishing a new backup retires earlier backup directories in the server process. > - The synchronous permission walk and recursive removal block unrelated requests. > - This pull request makes that cleanup asynchronous and keeps publication waiting for completion. > - The benefit is continued request progress while large retired backups are removed. ## Linked Issues or Issue Description **What happened?** Publishing a backup with many small files blocks the Node event loop during directory permission repair and removal. A local comparison with 20,000 retired files reproduced 1.37–1.95 second event-loop stalls. This is a pause candidate. A short hosted profile has not established it as the dominant source of sustained CPU use. **Expected behavior** Unrelated requests can progress during cleanup. Publication still waits for complete removal, preserves rollback on a failed lease stamp, and leaves live files and symlink targets unchanged. **Steps to reproduce** Run the benchmark in `doc/performance/native-backup-publication.md`. It compiles the actual publication service from a pinned baseline and the working tree, prepares immutable retired backups, and sends foreground HTTP probes from a separate process. **Paperclip version or commit** Measured baseline: `774ddc72713d6aa7cb516691c71f1fc23bcfa5ff`. The measured treatment source is committed in this branch. Current head: `0a69cf2d43c2f9fcf032d96c1dceb56163bf21ba`. The runtime patch is unchanged from the measured source; later commits only strengthen the test and benchmark evidence paths. **Deployment mode** Local compiled service on macOS arm64 with Node v24.22.2. No hosted rollout has occurred. Related work: #18380 changed retention scheduling. #13676 included the existing immutable-backup cleanup behavior. Neither change makes publication cleanup asynchronous. The GitHub duplicate search also found #11929, which concerns database dumps rather than native harness backups. No matching open event-loop cleanup issue was found. ## What Changed - Use asynchronous filesystem operations for retired-backup cleanup. Visit directories serially and skip unnecessary metadata calls for file entries. - Await cleanup before publication proceeds, during rollback, after successful rotation, and in executor finalization. - Exercise immutable current and previous backups in the publication test. Preserve the existing symlink-isolation and rollback tests. Add a real separate-process HTTP probe that must complete while publication is still active. - Add a repeatable service benchmark and document its fixture, metrics, and acceptance limits. ## Verification Draft status: focused correctness tests, the local comparison, and repository typecheck pass. The complete local test gate is incomplete. The build passed. Complete CI, hosted acceptance, and final-head review remain pending. - Final focused Vitest run: 2 files, 4 tests passed in 649 ms with the normal server configuration. The regression warms a separate Node client, waits for its HTTP request during the stamp callback, queues the response for the next event-loop turn, and then allows final retired-tree removal to start. The async treatment serves that response before publication completes. The original synchronous cleanup fails the same final regression in a separate source fixture. That expected control failure is retained. - Syntax and `git diff --check` passed. - `pnpm -r typecheck` passed on head `332c57b88c` in 152.18 seconds, including the native Runner build. Validation uses Node v24.22.2 and the repository-pinned pnpm v9.15.4 with isolated dependencies. - `pnpm test:run` was intentionally stopped after 1,390.24 seconds (exit 143). All six chat shards passed, covering all 1,817 parameterized cases across their exact partitions. The general server phase was advancing but incomplete. The repository timing manifest estimates another roughly two hours for the serial remainder, so validation moved to final-head parallel CI. This is not a full local test pass. The command began on `332c57b88c`; later changes only refine the regression and benchmark, and the final regression has its own focused pass. - Benchmark failure/metadata checks pass in a tiny isolated Git fixture: an injected worker exit 17 is reported in 0.44 seconds without hanging, and the result includes a staged source change when the unstaged diff is empty. - `pnpm build` passed in 41.80 seconds on `9f7e4e669b`. The final commit only releases a pending test probe on failure; runtime and build sources are unchanged. An injected client exit verifies that final test failure path exits promptly in 0.97 seconds instead of hanging. - Three alternating baseline/treatment pairs used fresh fixtures with 100 directories × 100 files in each of two retired backups. Each publication removed 20,000 files. A separate process sent HTTP probes at fixed 5 ms intervals, including overdue arrivals. - Every run verified the replacement manifest, complete cleanup, and unchanged symlink-target contents and mode. All 2,395 HTTP requests completed without errors. The largest measured generator delay was 8.8 ms. | Metric | Baseline median (range), ms | Treatment median (range), ms | | --- | ---: | ---: | | Foreground request p95 | 1,357.7 (1,287.4–2,014.0) | 1.8 (1.7–25.9) | | Maximum event-loop delay | 1,439.7 (1,371.5–1,954.5) | 19.9 (17.4–40.7) | | Publication completion | 1,429.1 (1,360.2–1,952.9) | 1,439.5 (897.9–1,649.6) | | Process CPU during publication | 1,194.1 (1,041.8–1,225.5) | 2,423.1 (1,995.7–2,514.7) | The treatment served 180–329 requests while publication was active. The baseline served zero during that interval. Process CPU includes foreground work that the baseline cannot serve until cleanup completes. The result supports responsiveness, with a CPU tradeoff. It does not establish lower total CPU or a hosted latency gain. Reproduce with Node v24.22.2: ```sh pnpm --filter @paperclipai/server exec vitest run src/services/native-runtime/native-harness-backup-cleanup.test.ts src/services/native-runtime/native-harness-backup-publication.test.ts node server/scripts/benchmark-native-backup-publication.mjs --baseline-ref 774ddc72713d6aa7cb516691c71f1fc23bcfa5ff --output /tmp/native-backup-publication.json ``` The benchmark records the baseline SHA, treatment HEAD and source diff. Raw local evidence remains outside the repository. A first small smoke run hit a sandbox restriction on loopback listeners; the authorized retry resolved it. The smoke also exposed load-generator interval drift. Fixed-clock arrivals replaced that generator before the reported measurements. Pending gates: complete current-head CI, final-head Greptile review, and the real run/checkpoint/recovery journey. The full local test run remains explicitly incomplete. The four concrete review findings were fixed. The prior head received 5/5; final-head review remains pending. Local evidence remains outside Git: `/private/tmp/backup-publication-measurement-20261102.json`, `/private/tmp/native-backup-focused-final.log`, `/private/tmp/native-backup-typecheck.log`, `/private/tmp/native-backup-test.log`, `/private/tmp/native-backup-test-interruption.json`, `/private/tmp/native-backup-build.log`, and `/private/tmp/native-backup-harness-check.json`. The harness-check receipt points to its isolated metadata and worker-failure logs. The final client-failure and baseline-control logs are `/private/tmp/backup-benchmark-check-gprcfiiz/failed-client-cleanup.log` and `/private/tmp/backup-benchmark-check-gprcfiiz/final-probe-baseline-control.log`. These local paths are retained for the author; the commands above let reviewers reproduce the synthetic proof. ## Risks - Asynchronous removal adds filesystem-pool work and used more process CPU in the fixture. Check resource pressure as well as responsiveness before rollout. - Filesystem operations can now interleave with unrelated work. The existing session owner still controls publication. Cleanup remains awaited; no deletion is detached and no scheduler is added. - Errors must still fail publication or restore the prior backup as before. Corruption checks and destructive lease authorization are unchanged. - This synthetic macOS fixture does not represent every Linux volume or backup shape. It does not qualify a deployed fix. - Rollout acceptance should confirm the serving SHA, checkpoint completion, continuation/recovery, error rate, and loop delay under matched backup load. Revert if cleanup or recovery fails. ## Model Used OpenAI Codex, GPT-6 family, with tool use and code execution. The exact served model ID and context-window size were not exposed to this agent. ## 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 - [ ] All Paperclip CI gates are green - [ ] 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 --- doc/performance/native-backup-publication.md | 53 +++++ .../benchmark-native-backup-publication.mjs | 196 ++++++++++++++++++ .../native-harness-backup-cleanup.test.ts | 6 +- .../native-harness-backup-cleanup.ts | 21 +- .../native-harness-backup-publication.test.ts | 97 ++++++++- .../native-harness-backup-publication.ts | 6 +- .../native-runtime/native-session-executor.ts | 2 +- 7 files changed, 364 insertions(+), 17 deletions(-) create mode 100644 doc/performance/native-backup-publication.md create mode 100644 server/scripts/benchmark-native-backup-publication.mjs diff --git a/doc/performance/native-backup-publication.md b/doc/performance/native-backup-publication.md new file mode 100644 index 00000000000..b461ea49d76 --- /dev/null +++ b/doc/performance/native-backup-publication.md @@ -0,0 +1,53 @@ +# Native backup publication benchmark + +Retiring a native harness backup can involve many immutable directories and +small files. Cleanup repairs directory permissions before removing the retired +tree. Publication waits for cleanup, preserves the previous backup when lease +stamping fails, and emits its completion notification after cleanup succeeds. +Cleanup uses asynchronous filesystem operations so unrelated requests can +continue while it runs. Directory preparation is serial and does not follow +symlinks into live trees. + +Run the focused correctness tests from an installed checkout: + +```sh +pnpm --filter @paperclipai/server exec vitest run src/services/native-runtime/native-harness-backup-cleanup.test.ts src/services/native-runtime/native-harness-backup-publication.test.ts +``` + +Run the performance comparison from the repository root with the Node version +used by the target server. Use a quiet machine and an explicit baseline commit: + +```sh +node server/scripts/benchmark-native-backup-publication.mjs \ + --baseline-ref 774ddc72713d6aa7cb516691c71f1fc23bcfa5ff \ + --output /tmp/native-backup-publication.json +``` + +The script compiles the real publication service and its dependencies for both +the baseline commit and the working tree. Each run uses a separate server +process. Fixture preparation occurs before measurement. The default fixture has +100 immutable directories with 100 small files in each of the `current` and +`previous` backups: 20,000 files to remove. Both backups also have a symlink to a +read-only live directory outside the retired trees. The new pending backup has +a distinct manifest. Every completed run checks the new manifest, complete +removal of retired and pending trees, and unchanged live contents and mode. + +A separate process sends requests to a small loopback HTTP endpoint every five +milliseconds. Arrivals use a fixed clock and include overdue requests. The +report includes request completion and error counts, generator lateness, p95 +and maximum foreground latency, requests served during publication, publication +wall time, process CPU time, and maximum event-loop delay. Three pairs alternate +baseline and treatment order. Change the fixture with `--pairs`, `--directories`, +`--files-per-directory`, or `--interval-ms`. + +The result records Node/platform, baseline SHA, treatment HEAD and uncommitted +service diff. Compiled modules and source maps stay in the printed evidence +directory. Successful fixtures are removed; failed fixtures remain for diagnosis. + +This is a service benchmark. The HTTP endpoint is a responsiveness probe, not +the assembled Paperclip API. The lease-stamp callback resolves without database +work. It does not prove a hosted latency or total-CPU improvement. Process CPU +during publication includes foreground requests that the synchronous baseline +cannot serve until cleanup completes. Async cleanup can consume more CPU while +keeping the event loop responsive. Qualify the affected run/checkpoint/recovery +journey and compare matched hosted windows before claiming deployed acceptance. diff --git a/server/scripts/benchmark-native-backup-publication.mjs b/server/scripts/benchmark-native-backup-publication.mjs new file mode 100644 index 00000000000..78d607ed3a8 --- /dev/null +++ b/server/scripts/benchmark-native-backup-publication.mjs @@ -0,0 +1,196 @@ +import assert from "node:assert/strict"; +import { fork, execFileSync } from "node:child_process"; +import { createServer, get, Agent } from "node:http"; +import { promises as fs } from "node:fs"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import { fileURLToPath, pathToFileURL } from "node:url"; +import { monitorEventLoopDelay, performance } from "node:perf_hooks"; +import { setTimeout as delay } from "node:timers/promises"; + +const filename = fileURLToPath(import.meta.url); +const checkout = path.resolve(path.dirname(filename), "../.."); +const arg = (name, fallback) => { + const position = process.argv.indexOf(`--${name}`); + return position < 0 ? fallback : process.argv[position + 1]; +}; +const percentile = (values, fraction) => { + const sorted = [...values].sort((a, b) => a - b); + return sorted[Math.min(sorted.length - 1, Math.ceil(sorted.length * fraction) - 1)] ?? null; +}; + +if (process.argv.includes("--worker")) { + const { publishNativeHarnessBackup } = await import(pathToFileURL(arg("module"))); + let publishing = false; + const server = createServer((_request, response) => { + response.setHeader("x-publication-active", publishing ? "1" : "0"); + response.end("ok"); + }); + await new Promise(resolve => server.listen(0, "127.0.0.1", resolve)); + process.send({ kind: "ready", port: server.address().port }); + process.on("message", async message => { + if (message.kind === "close") { + server.closeAllConnections(); + server.close(() => process.disconnect()); + return; + } + if (message.kind !== "publish") return; + const histogram = monitorEventLoopDelay({ resolution: 10 }); + histogram.enable(); + await delay(20); + const cpu = process.cpuUsage(); + const started = performance.now(); + publishing = true; + try { + await publishNativeHarnessBackup({ + db: {}, companyId: "benchmark", scopeDigest: "fixture", + backupRoot: message.backupRoot, pendingRoot: message.pendingRoot, + stamp: async () => {}, + }); + const durationMs = performance.now() - started; + const cpuUsed = process.cpuUsage(cpu); + publishing = false; + await delay(20); + histogram.disable(); + assert.equal(await fs.readFile(path.join(message.backupRoot, "current/manifest.json"), "utf8"), "replacement"); + assert.deepEqual((await fs.readdir(message.backupRoot)).sort(), ["current"]); + assert.equal(await fs.readFile(path.join(message.liveRoot, "untouched.txt"), "utf8"), "live"); + assert.equal((await fs.stat(message.liveRoot)).mode & 0o777, 0o555); + process.send({ kind: "result", durationMs, cpuMs: (cpuUsed.user + cpuUsed.system) / 1000, + eventLoopMaxMs: histogram.max / 1e6, workCompleted: true }); + } catch (error) { + publishing = false; + process.send({ kind: "error", message: error.stack }); + } + }); +} else { + const baselineRef = arg("baseline-ref"); + assert(baselineRef, "--baseline-ref is required"); + const pairs = Number(arg("pairs", "3")); + const directories = Number(arg("directories", "100")); + const filesPerDirectory = Number(arg("files-per-directory", "100")); + const intervalMs = Number(arg("interval-ms", "5")); + for (const value of [pairs, directories, filesPerDirectory, intervalMs]) assert(Number.isSafeInteger(value) && value > 0); + const evidence = await fs.mkdtemp(path.join(tmpdir(), "native-backup-publication-proof-")); + const { build } = await import("esbuild"); + const buildVariant = async variant => { + const outfile = path.join(evidence, `${variant}.mjs`); + await build({ absWorkingDir: checkout, entryPoints: ["server/src/services/native-runtime/native-harness-backup-publication.ts"], + outfile, bundle: true, platform: "node", format: "esm", target: "node24", sourcemap: true, + alias: { "@paperclipai/db/work-signals": path.join(checkout, "packages/db/src/work-signals.ts") }, + plugins: variant === "baseline" ? [{ name: "pinned-source", setup(builder) { + builder.onLoad({ filter: /\.ts$/ }, args => { + const relative = path.relative(checkout, args.path); + if (relative.startsWith("..") || relative.includes("node_modules")) return; + return { contents: execFileSync("git", ["show", `${baselineRef}:${relative}`], { cwd: checkout, encoding: "utf8" }), + loader: "ts", resolveDir: path.dirname(args.path) }; + }); + } }] : [], + }); + return outfile; + }; + const modules = { baseline: await buildVariant("baseline"), treatment: await buildVariant("treatment") }; + const makeFixture = async label => { + const root = await fs.mkdtemp(path.join(tmpdir(), `backup-publication-${label}-`)); + const backupRoot = path.join(root, "backups"); + const liveRoot = path.join(root, "live"); + const pendingRoot = path.join(backupRoot, "pending"); + await fs.mkdir(liveRoot); + await fs.writeFile(path.join(liveRoot, "untouched.txt"), "live"); + await fs.chmod(liveRoot, 0o555); + for (const version of ["current", "previous"]) { + const versionRoot = path.join(backupRoot, version); + for (let directory = 0; directory < directories; directory++) { + const target = path.join(versionRoot, `skill-${directory}`); + await fs.mkdir(target, { recursive: true }); + for (let file = 0; file < filesPerDirectory; file++) { + await fs.writeFile(path.join(target, `file-${file}.txt`), "retired instructions\n".repeat(4), { mode: 0o444 }); + } + await fs.chmod(target, 0o555); + } + await fs.symlink(liveRoot, path.join(versionRoot, "outside")); + await fs.chmod(versionRoot, 0o555); + } + await fs.mkdir(pendingRoot); + await fs.writeFile(path.join(pendingRoot, "manifest.json"), "replacement"); + return { root, backupRoot, liveRoot, pendingRoot }; + }; + const run = async variant => { + const fixture = await makeFixture(variant); + const worker = fork(filename, ["--worker", "--module", modules[variant]], { stdio: ["ignore", "inherit", "inherit", "ipc"] }); + const exited = new Promise(resolve => worker.once("exit", resolve)); + const nextMessage = kind => new Promise((resolve, reject) => { + const onMessage = message => { + if (message.kind !== kind && message.kind !== "error") return; + worker.off("message", onMessage); worker.off("exit", onExit); + if (message.kind === "error") reject(new Error(message.message)); else resolve(message); + }; + const onExit = code => reject(new Error(`worker exited before ${kind}: ${code}`)); + worker.on("message", onMessage); worker.once("exit", onExit); + }); + const { port } = await nextMessage("ready"); + const agent = new Agent({ keepAlive: true, maxSockets: 512 }); + const requests = []; + let sent = 0; + const epoch = performance.now(); + const dispatch = intended => { + const dispatched = performance.now(); + requests.push(new Promise(resolve => { + const request = get({ hostname: "127.0.0.1", port, agent }, response => { + response.resume(); + response.on("end", () => resolve({ intended, dispatchLateMs: dispatched - intended, + latencyMs: performance.now() - intended, active: response.headers["x-publication-active"] === "1", error: false })); + }); + request.setTimeout(10_000, () => request.destroy(new Error("request timeout"))); + request.on("error", () => resolve({ intended, dispatchLateMs: dispatched - intended, + latencyMs: performance.now() - intended, active: false, error: true })); + })); + }; + let ticker; + const pump = () => { + const now = performance.now(); + while (epoch + (sent + 1) * intervalMs <= now) dispatch(epoch + (++sent * intervalMs)); + ticker = setTimeout(pump, Math.max(0, epoch + (sent + 1) * intervalMs - performance.now())); + }; + ticker = setTimeout(pump, intervalMs); + try { + await delay(250); + const pending = nextMessage("result"); + const start = performance.now(); + worker.send({ kind: "publish", ...fixture }); + const publication = await pending; + const end = performance.now(); + await delay(250); + clearTimeout(ticker); + const completed = await Promise.all(requests); + const during = completed.filter(request => request.intended >= start && request.intended <= end); + assert(during.length > 0); + const result = { variant, ...publication, sent, completed: completed.length, + errors: completed.filter(request => request.error).length, duringRequests: during.length, + requestsServedDuringPublication: completed.filter(request => request.active).length, + foregroundP95Ms: percentile(during.map(request => request.latencyMs), 0.95), + foregroundMaxMs: Math.max(...during.map(request => request.latencyMs)), + generatorMaxLateMs: Math.max(...completed.map(request => request.dispatchLateMs)) }; + console.log(JSON.stringify(result)); + return result; + } finally { + clearTimeout(ticker); agent.destroy(); + if (worker.connected) worker.send({ kind: "close" }); + await exited; + await fs.chmod(fixture.liveRoot, 0o700); + if ((await fs.readdir(fixture.backupRoot)).join() === "current") await fs.rm(fixture.root, { recursive: true }); + } + }; + const measurements = []; + for (let pair = 0; pair < pairs; pair++) { + for (const variant of pair % 2 ? ["treatment", "baseline"] : ["baseline", "treatment"]) measurements.push(await run(variant)); + } + const report = { baselineRef: execFileSync("git", ["rev-parse", baselineRef], { cwd: checkout, encoding: "utf8" }).trim(), + treatmentHead: execFileSync("git", ["rev-parse", "HEAD"], { cwd: checkout, encoding: "utf8" }).trim(), + treatmentDiff: execFileSync("git", ["diff", "HEAD", "--", "server/src/services/native-runtime/native-harness-backup-cleanup.ts", "server/src/services/native-runtime/native-harness-backup-publication.ts"], { cwd: checkout, encoding: "utf8" }), + node: process.version, platform: `${process.platform}/${process.arch}`, pairs, directories, filesPerDirectory, + filesRemovedPerPublication: 2 * directories * filesPerDirectory, intervalMs, evidence, measurements }; + const output = arg("output", path.join(evidence, "result.json")); + await fs.writeFile(output, JSON.stringify(report, null, 2)); + console.log(`Evidence: ${output}`); +} diff --git a/server/src/services/native-runtime/native-harness-backup-cleanup.test.ts b/server/src/services/native-runtime/native-harness-backup-cleanup.test.ts index 1c969758ac7..d698855a97f 100644 --- a/server/src/services/native-runtime/native-harness-backup-cleanup.test.ts +++ b/server/src/services/native-runtime/native-harness-backup-cleanup.test.ts @@ -4,7 +4,7 @@ import { join } from "node:path"; import { expect, it } from "vitest"; import { removeNativeHarnessBackup } from "./native-harness-backup-cleanup.js"; -it("removes retired backups containing immutable skills without modifying live files or symlink targets", () => { +it("removes retired backups containing immutable skills without modifying live files or symlink targets", async () => { const base = mkdtempSync(join(tmpdir(), "paperclip-backup-cleanup-")); const retired = join(base, "previous"); const nested = join(retired, "claude", "skills", "first-task"); @@ -19,12 +19,12 @@ it("removes retired backups containing immutable skills without modifying live f chmodSync(retired, 0o555); chmodSync(live, 0o555); try { - removeNativeHarnessBackup(retired); + await removeNativeHarnessBackup(retired); expect(existsSync(retired)).toBe(false); expect(readFileSync(join(live, "SKILL.md"), "utf8")).toBe("live instructions"); expect(lstatSync(live).mode & 0o777).toBe(0o555); expect(lstatSync(join(live, "SKILL.md")).mode & 0o777).toBe(0o444); - expect(() => removeNativeHarnessBackup(retired)).not.toThrow(); + await expect(removeNativeHarnessBackup(retired)).resolves.toBeUndefined(); } finally { for (const directory of [retired, join(retired, "claude", "skills"), nested, live]) { if (existsSync(directory)) chmodSync(directory, 0o700); diff --git a/server/src/services/native-runtime/native-harness-backup-cleanup.ts b/server/src/services/native-runtime/native-harness-backup-cleanup.ts index d52e50ba5c4..7a803704ea7 100644 --- a/server/src/services/native-runtime/native-harness-backup-cleanup.ts +++ b/server/src/services/native-runtime/native-harness-backup-cleanup.ts @@ -1,18 +1,23 @@ -import { chmodSync, lstatSync, readdirSync, rmSync } from "node:fs"; +import { chmod, lstat, readdir, rm } from "node:fs/promises"; import { join } from "node:path"; /** Remove a retired controller-owned backup, never a live harness directory. */ -export function removeNativeHarnessBackup(root: string): void { +export async function removeNativeHarnessBackup(root: string): Promise { // Archives preserve immutable skill directory modes. Deleting their children // requires write permission on those directories, even with rm's force flag. // Only change directories in the backup being discarded. lstat deliberately // avoids following provider-created symlinks into live or unrelated trees. - const prepareDirectory = (path: string): void => { - const metadata = lstatSync(path, { throwIfNoEntry: false }); + const prepareDirectory = async (path: string): Promise => { + const metadata = await lstat(path).catch((error: NodeJS.ErrnoException) => { + if (error.code === "ENOENT") return undefined; + throw error; + }); if (!metadata?.isDirectory()) return; - chmodSync(path, (metadata.mode & 0o777) | 0o700); - for (const name of readdirSync(path)) prepareDirectory(join(path, name)); + await chmod(path, (metadata.mode & 0o777) | 0o700); + for (const entry of await readdir(path, { withFileTypes: true })) { + if (entry.isDirectory()) await prepareDirectory(join(path, entry.name)); + } }; - prepareDirectory(root); - rmSync(root, { recursive: true, force: true }); + await prepareDirectory(root); + await rm(root, { recursive: true, force: true }); } diff --git a/server/src/services/native-runtime/native-harness-backup-publication.test.ts b/server/src/services/native-runtime/native-harness-backup-publication.test.ts index dd9d940d9df..df1029f55f9 100644 --- a/server/src/services/native-runtime/native-harness-backup-publication.test.ts +++ b/server/src/services/native-runtime/native-harness-backup-publication.test.ts @@ -1,4 +1,6 @@ -import { promises as fs } from "node:fs"; +import { existsSync, promises as fs } from "node:fs"; +import { spawn } from "node:child_process"; +import { createServer, type ServerResponse } from "node:http"; import os from "node:os"; import path from "node:path"; import { afterEach, describe, expect, it } from "vitest"; @@ -32,7 +34,17 @@ describe("native harness backup publication", () => { }); it("publishes the completed replacement once and removes its predecessor", async () => { const input = await fixture(); const changes: unknown[] = []; - unsubscribe = onNativeHarnessBackupChange(input.db, change => changes.push(change)); + const previous = path.join(backupRoot, "previous"); + for (const root of [input.current, previous]) { + const skill = path.join(root, "skills", "immutable"); + await fs.mkdir(skill, { recursive: true }); + await fs.writeFile(path.join(skill, "SKILL.md"), "saved instructions", { mode: 0o444 }); + await fs.chmod(skill, 0o555); + } + unsubscribe = onNativeHarnessBackupChange(input.db, change => { + expect(existsSync(previous)).toBe(false); + changes.push(change); + }); await publishNativeHarnessBackup({ ...input, stamp: async current => { expect(changes).toEqual([]); expect(await fs.readFile(path.join(current, "manifest.json"), "utf8")).toBe("replacement"); @@ -41,4 +53,85 @@ describe("native harness backup publication", () => { expect(await fs.readdir(backupRoot)).toEqual(["current"]); expect(changes).toEqual([{ companyId: input.companyId, scopeDigest: input.scopeDigest }]); }); + it("serves an unrelated request while a retired backup is being removed", async () => { + const input = await fixture(); + for (let index = 0; index < 40; index++) { + const directory = path.join(input.current, `skill-${index}`); + await fs.mkdir(directory); + await Promise.all(Array.from({ length: 50 }, (_, file) => + fs.writeFile(path.join(directory, `file-${file}`), "retired instructions", { mode: 0o444 }))); + await fs.chmod(directory, 0o555); + } + let publishing = false; + let probeReceived: (response: ServerResponse | null) => void; + const probeReady = new Promise(resolve => { probeReceived = resolve; }); + const server = createServer((request, response) => { + if (request.url === "/probe") { probeReceived(response); return; } + response.end("ok"); + }); + await new Promise((resolve, reject) => { + server.once("error", reject); + server.listen(0, "127.0.0.1", resolve); + }); + const address = server.address(); + if (!address || typeof address === "string") throw new Error("HTTP probe did not start"); + const client = spawn(process.execPath, ["--input-type=module", "-e", ` + import { Agent, get } from "node:http"; + const agent = new Agent({ keepAlive: true }); + const request = path => new Promise((resolve, reject) => { + get({ hostname: "127.0.0.1", port: Number(process.argv[1]), path, agent }, response => { + response.resume(); + response.on("end", () => resolve(response.headers["x-publication-active"])); + }).on("error", reject); + }); + await request("/"); + process.send({ kind: "ready" }); + process.once("message", async () => { + const active = await request("/probe"); + process.send({ kind: "result", active }, () => { agent.destroy(); process.disconnect(); }); + }); + `, String(address.port)], { stdio: ["ignore", "ignore", "inherit", "ipc"] }); + const message = (kind: string) => new Promise<{ active?: string }>((resolve, reject) => { + const timer = setTimeout(() => { cleanup(); reject(new Error(`HTTP probe ${kind} timed out`)); }, 10_000); + const received = (value: { kind?: string; active?: string }) => { + if (value.kind === kind) { cleanup(); resolve(value); } + }; + const failed = () => { cleanup(); reject(new Error(`HTTP probe exited before ${kind}`)); }; + const cleanup = () => { + clearTimeout(timer); client.off("message", received); client.off("exit", failed); client.off("error", failed); + }; + client.on("message", received); client.once("exit", failed); client.once("error", failed); + }); + let publication: Promise | undefined; + try { + await message("ready"); + const response = message("result"); + publishing = true; + publication = publishNativeHarnessBackup({ + ...input, + stamp: async () => { + client.send({ kind: "probe" }); + const response = await probeReady; + if (!response) return; + setImmediate(() => { + response.setHeader("x-publication-active", publishing ? "1" : "0"); + response.end("ok"); + }); + }, + }).finally(() => { publishing = false; }); + const result = await response; + await publication; + expect(result.active).toBe("1"); + expect(await fs.readdir(backupRoot)).toEqual(["current"]); + expect(await fs.readFile(path.join(input.current, "manifest.json"), "utf8")).toBe("replacement"); + } finally { + probeReceived!(null); + try { await publication; } + finally { + client.kill(); + server.closeAllConnections(); + await new Promise(resolve => server.close(() => resolve())); + } + } + }, 15_000); }); diff --git a/server/src/services/native-runtime/native-harness-backup-publication.ts b/server/src/services/native-runtime/native-harness-backup-publication.ts index 820303584b0..714dafb5f20 100644 --- a/server/src/services/native-runtime/native-harness-backup-publication.ts +++ b/server/src/services/native-runtime/native-harness-backup-publication.ts @@ -9,7 +9,7 @@ export async function publishNativeHarnessBackup(input: { }) { const currentRoot = resolve(input.backupRoot, "current"); const previousRoot = resolve(input.backupRoot, "previous"); - removeNativeHarnessBackup(previousRoot); + await removeNativeHarnessBackup(previousRoot); let movedCurrent = false; if (existsSync(currentRoot)) { renameSync(currentRoot, previousRoot); @@ -19,10 +19,10 @@ export async function publishNativeHarnessBackup(input: { renameSync(input.pendingRoot, currentRoot); await input.stamp?.(currentRoot); } catch (error) { - if (existsSync(currentRoot)) removeNativeHarnessBackup(currentRoot); + if (existsSync(currentRoot)) await removeNativeHarnessBackup(currentRoot); if (movedCurrent && existsSync(previousRoot) && !existsSync(currentRoot)) renameSync(previousRoot, currentRoot); throw error; } - removeNativeHarnessBackup(previousRoot); + await removeNativeHarnessBackup(previousRoot); noteNativeHarnessBackupChange(input.db, { companyId: input.companyId, scopeDigest: input.scopeDigest }); } diff --git a/server/src/services/native-runtime/native-session-executor.ts b/server/src/services/native-runtime/native-session-executor.ts index f3565441c39..93992b4870d 100644 --- a/server/src/services/native-runtime/native-session-executor.ts +++ b/server/src/services/native-runtime/native-session-executor.ts @@ -13734,7 +13734,7 @@ async function createRunnerdBackendWithinSessionClaim( } : undefined, }); } finally { - removeNativeHarnessBackup(pendingRoot); + await removeNativeHarnessBackup(pendingRoot); } }, {