mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-13 15:55:46 +01:00
## 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 <noreply@anthropic.com>
67 lines
4.0 KiB
TypeScript
67 lines
4.0 KiB
TypeScript
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
|
|
|
const state = vi.hoisted(() => ({ read: vi.fn(), link: vi.fn(), sysctl: vi.fn(), selfBirth: vi.fn() }));
|
|
vi.mock("node:fs", async original => ({ ...await original<typeof import("node:fs")>(),
|
|
readFileSync: state.read, readlinkSync: state.link }));
|
|
vi.mock("node:os", async original => ({ ...await original<typeof import("node:os")>(), hostname: () => "same-host" }));
|
|
vi.mock("node:child_process", async original => ({ ...await original<typeof import("node:child_process")>(), execFileSync: state.sysctl }));
|
|
vi.mock("./hot-restart.js", () => ({ readProcessStartedAt: state.selfBirth }));
|
|
|
|
const descriptor = Object.getOwnPropertyDescriptor(process, "platform")!;
|
|
const firstBoot = "00000000-0000-4000-8000-000000000001";
|
|
const secondBoot = "00000000-0000-4000-8000-000000000002";
|
|
function platform(value: string) { Object.defineProperty(process, "platform", { ...descriptor, value }); }
|
|
async function fresh() { vi.resetModules(); return import("./controller-process-identity.js"); }
|
|
|
|
beforeEach(() => {
|
|
vi.resetAllMocks();
|
|
state.read.mockReturnValue(firstBoot + "\n"); state.link.mockReturnValue("pid:[42]");
|
|
state.sysctl.mockReturnValue(firstBoot.toUpperCase() + "\n");
|
|
state.selfBirth.mockResolvedValue("2026-11-07T00:00:00.000Z");
|
|
});
|
|
afterEach(() => { Object.defineProperty(process, "platform", descriptor); vi.restoreAllMocks(); });
|
|
|
|
describe("controller process-space identity", () => {
|
|
it("keeps Linux boot and PID-namespace isolation without launching a command", async () => {
|
|
platform("linux");
|
|
const original = await (await fresh()).currentControllerProcess();
|
|
expect(original.hostHash).toMatch(/^[0-9a-f]{32}$/);
|
|
expect(state.read).toHaveBeenCalledWith("/proc/sys/kernel/random/boot_id", "utf8");
|
|
expect(state.link).toHaveBeenCalledWith("/proc/self/ns/pid");
|
|
expect(state.sysctl).not.toHaveBeenCalled();
|
|
state.link.mockReturnValue("pid:[43]");
|
|
expect((await (await fresh()).currentControllerProcess()).hostHash).not.toBe(original.hostHash);
|
|
state.link.mockReturnValue("pid:[42]"); state.read.mockReturnValue(secondBoot);
|
|
expect((await (await fresh()).currentControllerProcess()).hostHash).not.toBe(original.hostHash);
|
|
});
|
|
|
|
it("uses Darwin kernel boot UUID once per module, never a rounded clock identity", async () => {
|
|
platform("darwin");
|
|
const owner = await fresh();
|
|
const first = await owner.currentControllerProcess();
|
|
expect(first.hostHash).toMatch(/^[0-9a-f]{32}$/);
|
|
expect(state.sysctl).toHaveBeenCalledExactlyOnceWith("/usr/sbin/sysctl", ["-n", "kern.bootsessionuuid"],
|
|
{ encoding: "utf8", timeout: 1_500, maxBuffer: 4_096, windowsHide: true });
|
|
expect(state.read).not.toHaveBeenCalled(); expect(state.link).not.toHaveBeenCalled();
|
|
state.sysctl.mockReturnValue(secondBoot);
|
|
expect((await owner.currentControllerProcess()).hostHash).toBe(first.hostHash);
|
|
expect(state.sysctl).toHaveBeenCalledTimes(1);
|
|
expect((await (await fresh()).currentControllerProcess()).hostHash).not.toBe(first.hostHash);
|
|
expect(state.sysctl).toHaveBeenCalledTimes(2);
|
|
expect(JSON.stringify(first)).not.toContain(firstBoot);
|
|
// Old weak/foreign receipts retain their original deadline: no local probe.
|
|
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 => {
|
|
platform(mode === "unsupported" ? "win32" : "darwin");
|
|
if (mode === "unreadable") state.sysctl.mockImplementation(() => { throw new Error("sysctl unavailable"); });
|
|
if (mode === "malformed") state.sysctl.mockReturnValue("not-a-kernel-uuid");
|
|
const owner = await fresh();
|
|
expect((await owner.currentControllerProcess()).hostHash).toBe("unknown");
|
|
expect(owner.sameProcessSpace("unknown")).toBe(false);
|
|
if (mode === "unsupported") expect(state.sysctl).not.toHaveBeenCalled();
|
|
});
|
|
});
|