Files
PaperClipAI/server/src/services/runtime-skill-cache.ts
T
DottaandClaude Opus 5.5 51c352ec2e fix(skills): release a runtime skill-cache lock whose holder provably exited (#19849)
## Thinking Path

> - Paperclip is the open source app people use to manage AI agents for
work
> - Before a run starts, the server builds a read-only cache of each
company skill under a cross-process file lock
> - The lock records only a pid and a hostname, so a crash leaves a lock
that nothing can release
> - If the holder pid is dead on the same host, the next run fails and
asks for manual cleanup. If the host name differs, the next run waits 60
s and then fails
> - This is on the turn path, so one crash during a skill build breaks
later runs that use that skill
> - This pull request records the shared controller process identity in
the lock and releases the lock at once when `controllerProcessExited`
proves the holder is gone
> - The benefit is that a crash during a skill build no longer blocks
later runs, and the release never relies on lock age or another host

## Linked Issues or Issue Description

**Bug description**
The Phase 0 ownership inventory (finding 4) lists the runtime
skill-cache lock (`runtime-skill-cache.ts`) as a record that nothing
releases automatically. When a publisher crashes while it holds
`<skillId>.lock`, every later preparation of that skill fails.

**Steps to reproduce**
1. Start a skill cache build, then kill the server process while it
holds the lock.
2. Restart and start a run that uses the same skill.
3. Preparation fails with "Runtime skill cache publisher exited; remove
its stale publication lock before retrying". For a holder whose host
name differs, it fails after 60 s with "publisher is busy".

**Expected behavior**
A lock whose holder has provably exited is released, and the build
continues without waiting. A holder that cannot be proven gone is still
waited for.

Refs #19846

## What Changed

- `controller-process-identity.ts`: add
`currentControllerProcessStamp()`. It returns this process's identity
with no I/O, so the turn path gets no new wait.
- `runtime-skill-cache.ts`: the lock owner file now also stores
`controller` (boot id, pid, PID-space hash, start time if already
known). `pid` and `host` stay so older readers still work.
- New `runtimeSkillCacheLockHolderExited`: true only when
`controllerProcessExited` proves the holder gone. Another host or PID
namespace, an unknown space, a legacy lock without `controller`, a live
pid, and this process all return false.
- On proof, waiters race for one exclusive claim named by the stale
holder's unique token (`<lock>.released-<token>`). Only the winner
unlinks the lock, and only while it still has the stale inode. The claim
is kept, so a late waiter does nothing. The waiter then retries the link
right away instead of after the 20 ms poll.
- A live holder is re-proved at most once a second.
`controllerProcessExited` skips the start-time probe (`ps` on macOS)
when no start time was recorded.
- The busy error now names the lock path. The same-host "publisher
exited" hard failure is removed.

## Verification

- `npx vitest run src/__tests__/runtime-skill-cache.test.ts
src/services/controller-process-identity.test.ts`: 28 passed, rebased on
master after #19848.
- New case "takes over a release claim whose claimer exited before
removing the stale lock" covers the second Greptile finding.
- New case "never removes a newer lock for a waiter that read a stale
one late" covers the Greptile P1.
- New case "releases a publication lock at once when its holder provably
exited". On the old code it fails with "Runtime skill cache publisher
exited; remove its stale publication lock before retrying". On the new
code it builds in well under 5 s and leaves no lock.
- New case "never treats age, another host, or a legacy holder as
proof": a different host hash, a legacy `{pid, host}` lock, a live pid,
this process, and malformed content all stay held.
- Server `tsc --noEmit` shows no errors in the changed files.

## Risks

- Previous-release story: a lock written by the previous release has no
`controller` field. It is unprovable. The waiter waits 60 s and fails as
before, and the error now names the path to remove. A new lock still
carries `pid` and `host`, so a previous-release reader behaves as
before.
- Lock release race: while a stale lock exists, only the single claim
winner can remove it, and the inode check cannot match a newer lock
because the stale holder's owner file stays and pins that inode. A claim
whose claimer crashed is itself released by the same proof (depth
bounded at 2).
- Each released lock leaves one small claim file and the dead holder's
owner file next to it.
- No new I/O before `turn.start`. The stamp is synchronous. The proof
runs only when the lock is contended.

## Model Used

- Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, used with tool use and
code execution in Claude Code.

## 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

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
2026-11-07 11:51:25 -06:00

333 lines
17 KiB
TypeScript

import { createHash, randomUUID } from "node:crypto";
import { constants, promises as fs } from "node:fs";
import os from "node:os";
import path from "node:path";
import type { CompanySkill } from "@paperclipai/shared";
import { controllerProcessExited, currentControllerProcessStamp, readControllerProcess } from "./controller-process-identity.js";
const FORMAT = 1;
const inFlight = new Map<string, Promise<string>>();
const digest = (value: string | Buffer) => createHash("sha256").update(value).digest("hex");
type FileRecord = { path: string; size: number; digest: string };
type CacheSpec = { root: string; entry: string; fingerprint: string; paths: string[] };
function filePath(value: string): string {
const normalized = value.replace(/\\/g, "/");
if (!normalized || normalized.startsWith("/") || /^[a-z]:/i.test(normalized)
|| normalized.split("/").some((part) => !part || part === "." || part === "..")
|| normalized.includes("\0")) throw new Error("Invalid runtime skill file path");
return normalized;
}
export function runtimeSkillCacheRoot(managedRoot: string, skillId: string): string {
if (!/^[a-zA-Z0-9_-]+$/.test(skillId)) throw new Error("Invalid runtime skill ID");
// A sibling of __runtime__: old runtime cleanup cannot remove published revisions.
return path.resolve(managedRoot, `__runtime_cache_v${FORMAT}__`, skillId);
}
export function runtimeSkillCacheSpec(managedRoot: string, skill: CompanySkill): CacheSpec | null {
if ((skill.sourceType === "github" || skill.sourceType === "skills_sh")
&& !/^[a-f0-9]{40}$/i.test(skill.sourceRef ?? "")) return null;
const metadata = skill.metadata ?? {};
const inventory = skill.fileInventory.map((entry) => ({ path: filePath(entry.path), kind: entry.kind }))
.sort((a, b) => a.path.localeCompare(b.path));
const paths = inventory.map((entry) => entry.path);
if (!paths.includes("SKILL.md")) throw new Error("Company skill could not be materialized because its stored SKILL.md copy is missing.");
if (new Set(paths).size !== paths.length) throw new Error("Invalid runtime skill file inventory");
// Local supporting files are mutable outside the DB as well as through the
// editor. Existing directories are used directly; missing-source fallback
// must reread its files rather than reuse an immutable revision fingerprint.
if (skill.sourceType === "local_path") return null;
const fingerprint = digest(JSON.stringify({
format: FORMAT, companyId: skill.companyId, skillId: skill.id,
sourceType: skill.sourceType, sourceLocator: skill.sourceLocator, sourceRef: skill.sourceRef,
// These are the only metadata fields used by the source reader. slug is its fallback directory.
source: { owner: metadata.owner, repo: metadata.repo, hostname: metadata.hostname,
ref: skill.sourceRef ? undefined : metadata.ref, repoSkillDir: metadata.repoSkillDir,
fallbackDirectory: typeof metadata.repoSkillDir === "string" ? undefined : skill.slug },
markdown: digest(skill.markdown), inventory,
}));
const root = runtimeSkillCacheRoot(managedRoot, skill.id);
return { root, entry: path.join(root, fingerprint), fingerprint, paths };
}
// Check every ancestor before traversing it, including the configured cache root.
async function assertDirectories(directory: string, trustedRoot: string, create = false): Promise<void> {
const absolute = path.resolve(directory);
let cursor = path.resolve(trustedRoot);
if (!absolute.startsWith(`${cursor}${path.sep}`)) throw new Error("Runtime cache escaped its root");
// Ancestors of the configured storage root may be system aliases (e.g. macOS /var).
if (create) await fs.mkdir(cursor, { recursive: true });
const rootStat = await fs.lstat(cursor);
if (!rootStat.isDirectory() || rootStat.isSymbolicLink()) throw new Error("Unsafe runtime skill storage root");
for (const part of absolute.slice(cursor.length).split(path.sep).filter(Boolean)) {
cursor = path.join(cursor, part);
if (create) await fs.mkdir(cursor).catch((error: NodeJS.ErrnoException) => {
if (error.code !== "EEXIST") throw error;
});
const stat = await fs.lstat(cursor);
if (!stat.isDirectory() || stat.isSymbolicLink()) throw new Error("Unsafe runtime skill cache directory");
}
}
async function readRegularFile(filename: string): Promise<Buffer> {
const handle = await fs.open(filename, constants.O_RDONLY | constants.O_NOFOLLOW);
try {
if (!(await handle.stat()).isFile()) throw new Error("Unsafe runtime skill cache file");
return await handle.readFile();
} finally { await handle.close(); }
}
async function inventory(directory: string, base = ""): Promise<string[]> {
const out: string[] = [];
if ((await fs.lstat(directory)).mode & 0o222) throw new Error("Writable runtime skill cache directory");
for (const entry of await fs.readdir(directory, { withFileTypes: true })) {
const relative = base ? `${base}/${entry.name}` : entry.name;
if (entry.isDirectory()) out.push(...await inventory(path.join(directory, entry.name), relative));
else if (entry.isFile()) out.push(relative);
else throw new Error("Symlink or special file in runtime skill cache");
}
return out.sort();
}
async function matches(spec: CacheSpec, entry = spec.entry): Promise<boolean> {
try {
await assertDirectories(path.join(entry, "files"), path.dirname(path.dirname(spec.root)));
const manifest = JSON.parse((await readRegularFile(path.join(entry, "manifest.json"))).toString("utf8"));
if (manifest.format !== FORMAT || manifest.fingerprint !== spec.fingerprint || !Array.isArray(manifest.files)
|| manifest.files.length !== spec.paths.length) return false;
const actual = await inventory(path.join(entry, "files"));
if (JSON.stringify(actual) !== JSON.stringify([...spec.paths].sort())) return false;
const seen = new Set<string>();
for (const record of manifest.files as FileRecord[]) {
if (!record || typeof record.path !== "string" || filePath(record.path) !== record.path
|| !spec.paths.includes(record.path) || seen.has(record.path)
|| !Number.isSafeInteger(record.size) || record.size < 0 || !/^[a-f0-9]{64}$/.test(record.digest)) return false;
seen.add(record.path);
const content = await readRegularFile(path.join(entry, "files", record.path));
if ((await fs.lstat(path.join(entry, "files", record.path))).mode & 0o222) return false;
if (content.length !== record.size || digest(content) !== record.digest) return false;
}
return true;
} catch { return false; }
}
/**
* True only when a lock holder's recorded server process has provably exited
* in this PID space. Lock age is never proof. A holder on another host or PID
* namespace, an unreadable space, or a lock written before holders recorded
* their process identity stays held.
*/
export async function runtimeSkillCacheLockHolderExited(lockContent: Buffer): Promise<boolean> {
let holder: unknown;
try { holder = JSON.parse(lockContent.toString("utf8")); } catch { return false; }
const controller = readControllerProcess((holder as { controller?: unknown } | null)?.controller);
return controller !== null && await controllerProcessExited(controller);
}
const OWNER_TOKEN = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/;
function lockHolderToken(lockContent: Buffer): string | null {
try {
const token = (JSON.parse(lockContent.toString("utf8")) as { token?: unknown } | null)?.token;
return typeof token === "string" && OWNER_TOKEN.test(token) ? token : null;
} catch { return null; }
}
/**
* Removes a lock whose holder exited. Every waiter that read the same stale
* lock races for one exclusive claim named by the holder's unique token, so
* only one of them ever unlinks it. While the stale lock exists no one else
* can acquire or remove it (its holder is gone), so the inode check before the
* unlink only confirms the claim is still about that lock. Every lock and
* claim inode is pinned by a kept owner file, so it is never reused while a
* delayed waiter may compare against it, and a waiter that read the stale
* lock late finds the claim and does nothing. A claim is itself a lock with a token and controller: when its
* claimer provably exited before finishing, the same rule releases the claim
* and the next poll claims the stale lock again.
*/
export async function releaseExitedRuntimeSkillCacheLock(root: string, lock: string,
stale: { inode: number; token: string }, depth = 0): Promise<void> {
const claim = `${lock}.released-${stale.token}`;
// Publish the claim the way a lock is published: write a complete owner
// file, then hard-link it into place, so a claim name never exists without
// its body. The owner file is kept: it pins the claim's inode, so a deleted
// claim's inode is never reused by a newer claim that a delayed waiter
// might then compare against.
const claimToken = randomUUID();
const claimOwner = path.join(root, `.owner-${claimToken}`);
await fs.writeFile(claimOwner, JSON.stringify({ token: claimToken, controller: currentControllerProcessStamp() }), { flag: "wx" });
try {
await fs.link(claimOwner, claim);
} catch (error) {
await fs.unlink(claimOwner).catch(() => {});
if ((error as NodeJS.ErrnoException).code !== "EEXIST") throw error;
// Bounded: a claimer crash inside this two-syscall window is already rare.
if (depth >= 2) return;
const handle = await fs.open(claim, constants.O_RDONLY | constants.O_NOFOLLOW).catch((openError: NodeJS.ErrnoException) => {
if (openError.code === "ENOENT") return null;
throw openError;
});
if (!handle) return;
let held: { content: Buffer; inode: number };
try { held = { content: await handle.readFile(), inode: (await handle.stat()).ino }; }
finally { await handle.close(); }
const heldToken = lockHolderToken(held.content);
if (heldToken && await runtimeSkillCacheLockHolderExited(held.content)) {
await releaseExitedRuntimeSkillCacheLock(root, claim, { inode: held.inode, token: heldToken }, depth + 1);
}
return;
}
const current = await fs.lstat(lock).catch((error: NodeJS.ErrnoException) => {
if (error.code === "ENOENT") return null;
throw error;
});
if (current?.ino === stale.inode) await fs.unlink(lock);
}
// Serialize builds and cleanup for one skill across processes as well as callers.
// A hard link publishes complete lock ownership atomically. A lock whose holder
// provably exited is released at once; any other holder is waited for.
async function publishLocked<T>(root: string, fingerprint: string, action: () => Promise<T>): Promise<T> {
const lock = path.join(root, `${fingerprint}.lock`);
const token = randomUUID();
const owner = path.join(root, `.owner-${token}`);
// `pid` and `host` stay for older readers; `controller` is the proof input.
// The stamp does no I/O, so preparation adds no wait before the turn.
await fs.writeFile(owner, JSON.stringify({ pid: process.pid, host: os.hostname(), token,
controller: currentControllerProcessStamp() }), { flag: "wx" });
let acquired = false;
// A live holder is re-proved at most once a second, not on every 20 ms poll.
let judged: { inode: number; at: number } | null = null;
try {
const deadline = Date.now() + 60_000;
while (!acquired) {
try { await fs.link(owner, lock); acquired = true; }
catch (error) {
if ((error as NodeJS.ErrnoException).code !== "EEXIST") throw error;
const handle = await fs.open(lock, constants.O_RDONLY | constants.O_NOFOLLOW).catch((openError: NodeJS.ErrnoException) => {
if (openError.code === "ENOENT") return null;
throw openError;
});
if (!handle) continue;
let held: { content: Buffer; inode: number };
try {
const stat = await handle.stat();
if (!stat.isFile()) throw new Error("Unsafe runtime skill cache file");
held = { content: await handle.readFile(), inode: stat.ino };
} finally { await handle.close(); }
const holderToken = lockHolderToken(held.content);
const due = judged === null || judged.inode !== held.inode || Date.now() - judged.at >= 1_000;
if (holderToken && due) {
judged = { inode: held.inode, at: Date.now() };
if (await runtimeSkillCacheLockHolderExited(held.content)) {
await releaseExitedRuntimeSkillCacheLock(root, lock, { inode: held.inode, token: holderToken });
continue;
}
}
if (Date.now() >= deadline) {
throw new Error(`Runtime skill cache publisher is busy; retry preparation. If no server process holds ${lock}, remove it.`);
}
await new Promise((resolve) => setTimeout(resolve, 20));
}
}
return await action();
} finally {
if (acquired) await fs.unlink(lock).catch(() => {});
await fs.unlink(owner).catch(() => {});
}
}
async function setTreeMode(directory: string, readonly: boolean): Promise<void> {
const stat = await fs.lstat(directory).catch((error: NodeJS.ErrnoException) => {
if (error.code === "ENOENT") return null;
throw error;
});
if (!stat || stat.isSymbolicLink()) return;
if (!stat.isDirectory()) {
if (readonly && stat.isFile()) await fs.chmod(directory, 0o444);
return;
}
if (!readonly) await fs.chmod(directory, 0o700);
for (const entry of await fs.readdir(directory)) await setTreeMode(path.join(directory, entry), readonly);
if (readonly) await fs.chmod(directory, 0o555);
}
// Darwin requires a directory being renamed to be writable by its owner.
// Change only the directory itself, while the publication lock is held; the
// contents remain read-only. Restore the mode even when publication fails.
async function renameCacheDirectory(source: string, destination: string) {
const stat = await fs.lstat(source);
const directory = stat.isDirectory() && !stat.isSymbolicLink();
if (directory) await fs.chmod(source, 0o700);
let moved = false;
try { await fs.rename(source, destination); moved = true; }
finally { if (directory) await fs.chmod(moved ? destination : source, stat.mode & 0o777); }
}
async function removeTree(directory: string): Promise<void> {
await setTreeMode(directory, false);
await fs.rm(directory, { recursive: true, force: true });
}
export async function resolveRuntimeSkillCache(
spec: CacheSpec, read: (relativePath: string) => Promise<string>, materialize = true,
stillInstalled: () => Promise<boolean> = async () => true,
): Promise<string | null> {
if (await matches(spec)) return path.join(spec.entry, "files");
if (!materialize) return null;
const active = inFlight.get(spec.entry);
if (active) return active;
const build = (async () => {
const namespace = path.dirname(spec.root);
await assertDirectories(namespace, path.dirname(namespace), true);
// The lock lives outside the skill directory, so cleanup cannot unlink an active lock.
return publishLocked(namespace, path.basename(spec.root), async () => {
if (!await stillInstalled()) throw new Error("Skill was renamed or removed during preparation");
await assertDirectories(spec.root, path.dirname(namespace), true);
if (await matches(spec)) return path.join(spec.entry, "files");
const staging = await fs.mkdtemp(path.join(spec.root, ".staging-"));
try {
await fs.mkdir(path.join(staging, "files"));
const files: FileRecord[] = [];
for (const relative of spec.paths) {
const content = Buffer.from(await read(relative), "utf8");
const target = path.join(staging, "files", relative);
await fs.mkdir(path.dirname(target), { recursive: true });
await fs.writeFile(target, content, { flag: "wx" });
files.push({ path: relative, size: content.length, digest: digest(content) });
}
await fs.writeFile(path.join(staging, "manifest.json"), JSON.stringify({ format: FORMAT, fingerprint: spec.fingerprint, files }));
await setTreeMode(staging, true);
if (!await matches(spec, staging)) throw new Error("Runtime skill cache validation failed");
// Lifecycle mutations can update the DB while this builder owns the filesystem lock.
if (!await stillInstalled()) throw new Error("Skill was renamed or removed during preparation");
await renameCacheDirectory(spec.entry, path.join(spec.root, `.invalid-${spec.fingerprint}-${randomUUID()}`))
.catch((error: NodeJS.ErrnoException) => { if (error.code !== "ENOENT") throw error; });
await renameCacheDirectory(staging, spec.entry);
return path.join(spec.entry, "files");
} finally { await removeTree(staging); }
});
})();
inFlight.set(spec.entry, build);
try { return await build; } finally { if (inFlight.get(spec.entry) === build) inFlight.delete(spec.entry); }
}
export async function removeRuntimeSkillCache(
managedRoot: string, skillId: string, afterRemove?: () => Promise<void>,
): Promise<void> {
const root = runtimeSkillCacheRoot(managedRoot, skillId);
const namespace = path.dirname(root);
try { await assertDirectories(namespace, managedRoot, Boolean(afterRemove)); }
catch (error) { if ((error as NodeJS.ErrnoException).code === "ENOENT" && !afterRemove) return; throw error; }
await publishLocked(namespace, skillId, async () => {
try {
await assertDirectories(root, managedRoot);
await removeTree(root);
} catch (error) { if ((error as NodeJS.ErrnoException).code !== "ENOENT") throw error; }
// Commit deletion while builders remain excluded. Lock/cleanup failures leave the row intact.
await afterRemove?.();
});
}