From ce3881f1b3f20337ddfc698380552f11e49ac579 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Tue, 10 Nov 2026 20:59:49 -0600 Subject: [PATCH] fix(skills): release a skill cache lock whose holder's deployment was replaced (#20688) 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 > - Run preparation publishes skill revisions into a shared runtime skill cache. A file lock serializes each publication. A lock whose holder provably exited is released at once. > - That check used `controllerProcessExited`, a PID proof. It fails closed whenever the recorded process space (host, kernel boot, PID namespace) differs from this server's. > - Every Railway deploy starts the server in a new process space. A lock left by a server that a deploy stopped mid-publish can never be released. Each later preparation of that skill waits 60 s and fails with "Runtime skill cache publisher is busy … remove it", until an operator deletes the file. > - The rest of recovery already uses one shared prover (`proveOwnerGone`). Across process spaces it accepts a replaced platform deployment on the same exclusive volume, or a stopped or fenced server boot. > - This pull request makes the skill cache lock use that prover. It was the last caller of the separate PID-only proof. > - The benefit is one rule for "the controller from a previous deployment is provably gone", and no manual file removal after a deploy. ## Linked Issues or Issue Description **What happened?** A census of staging failures after deploys showed that `controllerProcessExited` always fails closed after a deploy, because the process-space hash changes. On `master`, the only remaining caller is the runtime skill cache lock check. The restart chaos harness restarts in the same process space, so the PID proof succeeded there and hid the gap. I did not find this failure in the staging run census. It is a latent dead end with the same cause as the census class. **Expected behavior** A lock whose holder ran in a deployment that the platform replaced on the same exclusive volume, or whose server boot stopped or was fenced, is released at once. Any other holder is still waited for. **Steps to reproduce** 1. Run the server on Railway with a volume. 2. Redeploy while a skill revision is being published (the lock file exists). 3. Prepare a run that needs the same skill revision on the new deployment. 4. Observe a 60 s wait and `Runtime skill cache publisher is busy; retry preparation.` on every attempt. **Paperclip version or commit** `master` at `c444da7d05` (the new test fails there). ## What Changed - `server/src/services/runtime-skill-cache.ts`: `runtimeSkillCacheLockHolderExited` builds a local owner stamp from the recorded controller (boot ID, PID, process space, start time, and the Railway deployment and volume when recorded). It returns true only for the `gone` verdict of `proveOwnerGone`. - Same-space PID rules do not change. A dead PID is gone, a live PID with no start time is unprovable, and this server's own boot is alive. - `server/src/__tests__/runtime-skill-cache.test.ts`: a new test for a holder in another process space whose deployment the platform replaced. - The proof is bounded by the lock wait (5 s, never past the deadline). A timed-out or failed proof keeps the lock held. A proof still running is joined by later waits on the same holder, so a stalled read adds at most one database query per holder. - `server/src/services/controller-process-identity.ts`: removes `controllerProcessExited`, which has no caller left; its test assertions now check `sameProcessSpace`. ## Verification - `npx vitest run src/__tests__/runtime-skill-cache.test.ts src/services/controller-process-space-identity.test.ts` (from `server/`): 31 passed. - New: "releases a lock whose holder ran in a deployment the platform replaced". It fails on `master` (`expected false to be true`) and passes with this change. - The same test keeps the lock held for: the same deployment ID, another volume, a non-exclusive volume, another service, and no recorded deployment. - The existing test "never treats age, another host, or a legacy holder as proof that a lock holder exited" still passes. Without a deployment or controller lease proof, another host stays held. - `npx tsc --noEmit -p server/tsconfig.json` passes. - `pnpm verify` selected 2504 of 3294 test files (the change reaches `owner-proof.ts` importers) and hit my 30 min local limit with no failure seen. Required CI supplies the broad coverage. - Live proof: not done (tests only). ## Risks - Across process spaces, a held lock can now trigger a read of `controller_instances` for the holder's boot. This happens at most once per second, and only while a lock is held. Uncontended preparation does no extra work. - The prover may fence a boot row whose lease lapsed past its grace, as it already does for every other recorded owner. - No persisted shape changes. Older lock files without a `controller` field stay held, as before. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, used through Claude Code with tool use and code execution. ## 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 --- .../src/__tests__/runtime-skill-cache.test.ts | 56 +++++++++++++++++++ .../services/controller-process-identity.ts | 14 ----- .../controller-process-space-identity.test.ts | 11 +--- server/src/services/runtime-skill-cache.ts | 54 +++++++++++++++--- 4 files changed, 105 insertions(+), 30 deletions(-) diff --git a/server/src/__tests__/runtime-skill-cache.test.ts b/server/src/__tests__/runtime-skill-cache.test.ts index ab63e868ac4..d03475037a5 100644 --- a/server/src/__tests__/runtime-skill-cache.test.ts +++ b/server/src/__tests__/runtime-skill-cache.test.ts @@ -187,6 +187,62 @@ describe("runtime skill revision cache", () => { expect(await runtimeSkillCacheLockHolderExited(Buffer.from("not json"))).toBe(false); }); + it("releases a lock whose holder ran in a deployment the platform replaced", async () => { + // A Railway redeploy starts the server in a new process space, so a PID + // proof never applies. The platform stopped the earlier deployment, which + // held the same exclusive volume, before it started this one. + const railway = { RAILWAY_PROJECT_ID: "project", RAILWAY_ENVIRONMENT_ID: "environment", RAILWAY_SERVICE_ID: "service", + RAILWAY_DEPLOYMENT_ID: "deployment-new", RAILWAY_VOLUME_ID: "volume", RAILWAY_VOLUME_MOUNT_PATH: "/data" }; + for (const [key, value] of Object.entries(railway)) vi.stubEnv(key, value); + try { + const worker = { provider: "railway", projectId: "project", environmentId: "environment", serviceId: "service" }; + const holder = { bootId: randomUUID(), pid: 3, hostHash: "f".repeat(32), processStartedAt: null, + deployment: { worker: { ...worker, deploymentId: "deployment-old" }, exclusiveVolume: true, volumeId: "volume" } }; + expect(await runtimeSkillCacheLockHolderExited(lockStamp(holder))).toBe(true); + // Only a replaced deployment on the same exclusive volume is proof. + const deployment = holder.deployment; + for (const changed of [{ ...deployment, worker: { ...worker, deploymentId: "deployment-new" } }, + { ...deployment, volumeId: "another-volume" }, { ...deployment, exclusiveVolume: false }, + { ...deployment, worker: { ...worker, serviceId: "another-service", deploymentId: "deployment-old" } }]) { + expect(await runtimeSkillCacheLockHolderExited(lockStamp({ ...holder, deployment: changed }))).toBe(false); + } + const { deployment: _omitted, ...withoutDeployment } = holder; + expect(await runtimeSkillCacheLockHolderExited(lockStamp(withoutDeployment))).toBe(false); + } finally { + vi.unstubAllEnvs(); + } + }); + + it("keeps a lock held when the holder proof stalls or fails", async () => { + // A distinct holder per case: a still-pending proof is joined by later waits on the same holder. + const holder = () => ({ bootId: randomUUID(), pid: 3, hostHash: "f".repeat(32), processStartedAt: null }); + const startedAt = Date.now(); + expect(await runtimeSkillCacheLockHolderExited(lockStamp(holder()), + { timeoutMs: 50, prove: () => new Promise(() => {}) })).toBe(false); + expect(Date.now() - startedAt).toBeLessThan(2_000); + expect(await runtimeSkillCacheLockHolderExited(lockStamp(holder()), + { prove: async () => { throw new Error("database unavailable"); } })).toBe(false); + expect(await runtimeSkillCacheLockHolderExited(lockStamp(holder()), + { prove: async () => ({ verdict: "gone", reason: "controller_stopped" }) })).toBe(true); + }); + + it("joins a stalled holder proof instead of starting another read on every timeout", async () => { + const holder = { bootId: randomUUID(), pid: 3, hostHash: "f".repeat(32), processStartedAt: null }; + let settle!: (proof: { verdict: "gone"; reason: "controller_stopped" }) => void; + const prove = vi.fn(() => new Promise<{ verdict: "gone"; reason: "controller_stopped" }>(resolve => { settle = resolve; })); + for (let attempt = 0; attempt < 5; attempt++) { + expect(await runtimeSkillCacheLockHolderExited(lockStamp(holder), { timeoutMs: 10, prove })).toBe(false); + } + expect(prove).toHaveBeenCalledTimes(1); + settle({ verdict: "gone", reason: "controller_stopped" }); + expect(await runtimeSkillCacheLockHolderExited(lockStamp(holder), { timeoutMs: 1_000, prove })).toBe(true); + // A settled proof is not reused: the next wait asks again. + await new Promise(resolve => setImmediate(resolve)); + const fresh = vi.fn(async () => ({ verdict: "alive" as const, reason: "controller_lease_held" as const })); + expect(await runtimeSkillCacheLockHolderExited(lockStamp(holder), { prove: fresh })).toBe(false); + expect(fresh).toHaveBeenCalledTimes(1); + }); + it("fingerprints installed content and ownership, excluding cosmetic metadata", async () => { const spec = runtimeSkillCacheSpec(root, skill)!; expect(runtimeSkillCacheSpec(root, { ...skill, name: "Renamed", updatedAt: new Date(), metadata: { ...skill.metadata, starred: true, ref: "new-branch" } })!.fingerprint).toBe(spec.fingerprint); diff --git a/server/src/services/controller-process-identity.ts b/server/src/services/controller-process-identity.ts index 338c7485b60..a5649a2f51a 100644 --- a/server/src/services/controller-process-identity.ts +++ b/server/src/services/controller-process-identity.ts @@ -200,20 +200,6 @@ export function processStartsDefinitelyDiffer( return Number.isFinite(left) && Number.isFinite(right) && Math.abs(left - right) >= PROCESS_START_PRECISION_MS; } -/** True only with proof, in this process's own PID space, that the recorded - * server process has exited. Another space, an unreadable space, this server, - * or an ambiguous live PID returns false. */ -export async function controllerProcessExited(controller: ControllerProcess): Promise { - if (controller.bootId === bootId || hostHash === UNKNOWN_PROCESS_SPACE || controller.hostHash !== hostHash) return false; - if (!processExists(controller.pid)) return true; - // Without a recorded start time a live PID can never prove exit; skip the probe. - if (!Number.isFinite(Date.parse(controller.processStartedAt ?? ""))) return false; - // The PID now belongs to a live process, possibly this restarted server. - // Only a definitely different start time proves the recorded process is gone. - const observed = await observedProcessStartedAt(controller.pid); - return observed !== null && processStartsDefinitelyDiffer(controller.processStartedAt, new Date(observed)); -} - /** This process's PID-space hash. An unreadable space is stored as * "unknown", which `sameProcessSpace` never accepts. */ export function currentProcessSpaceHash(): string { diff --git a/server/src/services/controller-process-space-identity.test.ts b/server/src/services/controller-process-space-identity.test.ts index bbf3b9046f4..858e7bef487 100644 --- a/server/src/services/controller-process-space-identity.test.ts +++ b/server/src/services/controller-process-space-identity.test.ts @@ -50,10 +50,8 @@ describe("controller process-space identity", () => { expect(state.sysctl).toHaveBeenCalledTimes(2); expect(JSON.stringify(first)).not.toContain(firstBoot); // Old weak/foreign receipts retain their original deadline: no local probe. - const kill = vi.spyOn(process, "kill"); - expect(await owner.controllerProcessExited({ bootId: "old", pid: 2_000_000_001, - hostHash: "old-rounded-clock-hash", processStartedAt: null })).toBe(false); - expect(kill).not.toHaveBeenCalled(); + expect(owner.sameProcessSpace("old-rounded-clock-hash")).toBe(false); + expect(owner.sameProcessSpace(first.hostHash)).toBe(true); }); it.each(["unreadable", "malformed", "unsupported"] as const)("fails closed without an owner PID probe for %s identity", async mode => { @@ -62,10 +60,7 @@ describe("controller process-space identity", () => { if (mode === "malformed") state.sysctl.mockReturnValue("not-a-kernel-uuid"); const owner = await fresh(); expect((await owner.currentControllerProcess()).hostHash).toBe("unknown"); - const kill = vi.spyOn(process, "kill"); - expect(await owner.controllerProcessExited({ bootId: "old", pid: 2_000_000_001, - hostHash: "unknown", processStartedAt: null })).toBe(false); - expect(kill).not.toHaveBeenCalled(); + expect(owner.sameProcessSpace("unknown")).toBe(false); if (mode === "unsupported") expect(state.sysctl).not.toHaveBeenCalled(); }); }); diff --git a/server/src/services/runtime-skill-cache.ts b/server/src/services/runtime-skill-cache.ts index 7c9cef9d7b3..0366a774c37 100644 --- a/server/src/services/runtime-skill-cache.ts +++ b/server/src/services/runtime-skill-cache.ts @@ -3,7 +3,11 @@ 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"; +import { currentControllerProcessStamp, readControllerProcess } from "./controller-process-identity.js"; +import { proveOwnerGone, type LocalOwnerStamp, type OwnerProof } from "./owner-proof.js"; + +/** Bounds one lock-holder proof, which can read the database across process spaces. */ +const LOCK_PROOF_TIMEOUT_MS = 5_000; const FORMAT = 1; const inFlight = new Map>(); @@ -114,18 +118,51 @@ async function matches(spec: CacheSpec, entry = spec.entry): Promise { } /** - * 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. + * True only when the shared owner prover shows that a lock holder's recorded + * server process is gone. Lock age is never proof. In this PID space the PID + * decides. A holder in another process space (for example before a redeploy) + * is gone only when the platform replaced its deployment on the same exclusive + * volume, or when its server boot stopped or was fenced. A lock written before + * holders recorded their process identity stays held. */ -export async function runtimeSkillCacheLockHolderExited(lockContent: Buffer): Promise { +export async function runtimeSkillCacheLockHolderExited(lockContent: Buffer, options: { + /** This wait gives up (the lock stays held) after this long. A stalled read keeps running; later waits on the same holder join it. */ + timeoutMs?: number; + prove?: (stamp: LocalOwnerStamp) => Promise; +} = {}): Promise { 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); + if (!controller) return false; + const stamp: LocalOwnerStamp = { + kind: "local", scope: "runtime_skill_cache_lock", generation: null, host: null, processSpace: controller.hostHash, + bootId: controller.bootId, pid: controller.pid, processStartedAt: controller.processStartedAt, processGroupId: null, + ...(controller.deployment ? { worker: controller.deployment.worker, exclusiveVolume: controller.deployment.exclusiveVolume, + volumeId: controller.deployment.volumeId } : {}), + }; + // A proof that outlives its timeout keeps running (its database read cannot + // be cancelled), so later waits on the same holder join it instead of adding + // another read to the shared pool. + const key = JSON.stringify(stamp); + let proved = pendingLockProofs.get(key); + if (!proved) { + proved = (options.prove ?? (value => proveOwnerGone(value)))(stamp) + .then(proof => proof.verdict === "gone", () => false) + .finally(() => { if (pendingLockProofs.get(key) === proved) pendingLockProofs.delete(key); }); + pendingLockProofs.set(key, proved); + } + let timer: NodeJS.Timeout | undefined; + const timedOut = new Promise(resolve => { timer = setTimeout(() => resolve(false), options.timeoutMs ?? LOCK_PROOF_TIMEOUT_MS); }); + try { + return await Promise.race([proved, timedOut]); + } finally { + clearTimeout(timer); + } } +/** Lock-holder proofs still running, by holder stamp; at most one per holder. */ +const pendingLockProofs = new Map>(); + 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 { @@ -221,7 +258,8 @@ async function publishLocked(root: string, fingerprint: string, action: () => 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)) { + if (await runtimeSkillCacheLockHolderExited(held.content, + { timeoutMs: Math.max(1, Math.min(LOCK_PROOF_TIMEOUT_MS, deadline - Date.now())) })) { await releaseExitedRuntimeSkillCacheLock(root, lock, { inode: held.inode, token: holderToken }); continue; }