mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-10 12:37:55 +01:00
feat(recovery): name the owner of teardown claims and login/probe leases (#19965)
<!-- Write all pull request text in Simplified Technical English (ASD-STE100): short sentences, one instruction per sentence, simple approved vocabulary, and the active voice. --> ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Environment leases carry several in-progress records: login/probe leases and in-flight teardown claims among them > - The ownership reconciler (durable ownership plan, Phase 2) must know who owns each record, or invariant I1 reports a gap > - The ownership data showed one teardown claim and one login/probe lease with no owner stamp > - A teardown claim could also show the wrong owner: a controller stamp left by an earlier attempt named a dead process while a live claimant held the claim > - This pull request names an honest owner for both kinds, without a new write before `turn.start` > - The benefit is that I1 measures real gaps, and the later settle for these kinds can trust the owner it is given ## Linked Issues or Issue Description Refs #19964 Refs #19933 **What happened?** The ownership reconciler reported `environment_teardown_claim` and `environment_login_probe_lease` records as I1 `no_owner_stamp`. Workspace recovery and native export retry claim teardown without a controller stamp. A login/probe lease never had an owner stamp. A teardown claim that kept an older attempt's controller stamp was attributed to that older process. **Expected behavior** Each in-progress record names the owner that can still move it: a process proved by identity, or a fence whose expiry is never proof by itself. A stamp for a different attempt never names the owner. **Steps to reproduce** 1. Let workspace recovery claim a pending-cleanup lease (no controller stamp), or open a login session lease. 2. Run the ownership reconciliation pass. 3. See `no_owner_stamp` for the record, or a dead earlier controller for a claim that a live claimant holds. ## What Changed - `recovery/ownership-registry.ts`, `environment_login_probe_lease`: the owner is the holder process stamped by the write that makes the lease active (`metadata.leaseController`, added in #19964). A lease saved by the previous release still names no owner; it ends with its login session. - `recovery/ownership-registry.ts`, `environment_teardown_claim`: a controller stamp counts only when its `attemptId` equals the claim's `pendingCleanupAttemptId`. Any other claim with an attempt id is a fence: the attempt id is the token and `pendingCleanupLeaseExpiresAtMs` (renewed by the claimant) is the expiry. An expired fence stays unprovable. A claim with no attempt id names no owner. - Report-only. No settle is registered for these kinds in this change. ## Verification - `cd server && npx vitest run src/__tests__/environment-lease-owner-stamps.test.ts src/__tests__/ownership-reconciliation-report.test.ts` (embedded Postgres). - Red on the previous registry, green here (tests written after the code): - login/probe lease is owned by the process whose write made it active - login/probe lease is proven gone when that process has exited - teardown claim never reads a controller stamped for an earlier attempt (fence held by its token) - an unstamped teardown claim whose fence expired stays unprovable, never gone - Green on both: a stamped claim for this attempt with a dead PID is gone; an unstamped login lease and an attempt-less claim report I1. - Full typecheck is left to CI (local memory limit). ## Risks - Low risk. The change only alters the owner the report-only reconciler reads. No record is changed. - Stacked on #19964 (the lease holder stamp). Merge #19964 first. > 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, via Claude Code with tool use (shell, file edit, embedded Postgres tests). ## 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>
This commit is contained in:
1 parent
c0c26188db
commit
0cb9770d1c
2 files changed
+150
-2
No files matched your search
@@ -0,0 +1,117 @@
|
||||
import { spawn } from "node:child_process";
|
||||
import { randomUUID } from "node:crypto";
|
||||
import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest";
|
||||
import { agents, companies, createDb, environmentLeases, environments } from "@paperclipai/db";
|
||||
import { getEmbeddedPostgresTestSupport, startEmbeddedPostgresTestDatabase } from "./helpers/embedded-postgres.js";
|
||||
import { currentControllerProcessStamp, currentProcessSpaceHash } from "../services/controller-process-identity.js";
|
||||
import { ENVIRONMENT_LEASE_CONTROLLER_KEY } from "../services/environment-lease-controller.js";
|
||||
import { environmentService } from "../services/environments.js";
|
||||
import { createOwnershipReportState, runOwnershipReconciliationPass } from "../services/recovery/ownership-invariant-report.js";
|
||||
|
||||
const support = await getEmbeddedPostgresTestSupport();
|
||||
const suite = support.supported ? describe : describe.skip;
|
||||
|
||||
async function exitedPid(): Promise<number> {
|
||||
const child = spawn(process.execPath, ["-e", "setInterval(() => {}, 1000)"], { stdio: "ignore" });
|
||||
const pid = child.pid!;
|
||||
await new Promise<void>(resolve => { child.once("exit", () => resolve()); child.kill("SIGKILL"); });
|
||||
return pid;
|
||||
}
|
||||
|
||||
suite("environment lease owner stamps (login/probe lease, teardown claim)", () => {
|
||||
let db!: ReturnType<typeof createDb>;
|
||||
let temporary: Awaited<ReturnType<typeof startEmbeddedPostgresTestDatabase>> | null = null;
|
||||
let companyId = "";
|
||||
let environmentId = "";
|
||||
|
||||
beforeAll(async () => {
|
||||
temporary = await startEmbeddedPostgresTestDatabase("paperclip-environment-lease-owner-stamps-");
|
||||
db = createDb(temporary.connectionString);
|
||||
companyId = randomUUID();
|
||||
environmentId = randomUUID();
|
||||
await db.insert(companies).values({ id: companyId, name: "Paperclip", issuePrefix: "ELS",
|
||||
requireBoardApprovalForNewAgents: false });
|
||||
await db.insert(agents).values({ id: randomUUID(), companyId, name: "Coder", role: "engineer", status: "idle",
|
||||
adapterType: "codex_local", adapterConfig: {}, runtimeConfig: {}, permissions: {} });
|
||||
await db.insert(environments).values({ id: environmentId, name: "Fake Sandbox", driver: "sandbox",
|
||||
status: "active", config: { provider: "fake", image: "ubuntu:24.04" } });
|
||||
}, 30_000);
|
||||
|
||||
afterEach(async () => {
|
||||
await db.delete(environmentLeases);
|
||||
});
|
||||
|
||||
afterAll(async () => {
|
||||
await temporary?.cleanup();
|
||||
});
|
||||
|
||||
async function report(kind: string) {
|
||||
const result = await runOwnershipReconciliationPass(db, { state: createOwnershipReportState(), record: false });
|
||||
return { kind: result.kinds.find(entry => entry.kind === kind)!, violations: result.violations.filter(entry => entry.kind === kind) };
|
||||
}
|
||||
|
||||
async function claim(metadata: Record<string, unknown>) {
|
||||
const id = randomUUID();
|
||||
await db.insert(environmentLeases).values({ id, companyId, environmentId, status: "pending_cleanup",
|
||||
provider: "fake", providerLeaseId: `sandbox://fake/${id}`, metadata: { pendingCleanupInFlight: true, ...metadata } });
|
||||
return id;
|
||||
}
|
||||
|
||||
describe("login/probe lease", () => {
|
||||
it("is owned by the process whose write made it active", async () => {
|
||||
await environmentService(db).acquireLease({ companyId, environmentId, provider: "fake",
|
||||
providerLeaseId: "sandbox://fake/login" });
|
||||
const { kind, violations } = await report("environment_login_probe_lease");
|
||||
expect(kind).toMatchObject({ inProgress: 1, verdicts: { alive: 1 }, reasons: { current_process: 1 } });
|
||||
expect(violations).toEqual([]);
|
||||
});
|
||||
|
||||
it("is proven gone when that process has exited", async () => {
|
||||
await db.insert(environmentLeases).values({ companyId, environmentId, status: "active", metadata: {
|
||||
[ENVIRONMENT_LEASE_CONTROLLER_KEY]: { bootId: "earlier-boot", pid: await exitedPid(),
|
||||
hostHash: currentProcessSpaceHash(), processStartedAt: null } } });
|
||||
const { kind } = await report("environment_login_probe_lease");
|
||||
expect(kind).toMatchObject({ verdicts: { gone: 1 }, reasons: { pid_dead: 1 } });
|
||||
});
|
||||
|
||||
it("names no owner when the previous release saved it without a stamp", async () => {
|
||||
await db.insert(environmentLeases).values({ companyId, environmentId, status: "active", metadata: {} });
|
||||
const { kind, violations } = await report("environment_login_probe_lease");
|
||||
expect(kind).toMatchObject({ verdicts: { unprovable: 1 }, reasons: { no_owner_stamp: 1 } });
|
||||
expect(violations.map(violation => violation.rule)).toEqual(["I1_owned"]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("teardown claim", () => {
|
||||
it("is owned by the claimant process stamped for this exact attempt", async () => {
|
||||
const attemptId = randomUUID();
|
||||
await claim({ pendingCleanupAttemptId: attemptId, pendingCleanupLeaseExpiresAtMs: Date.now() + 60_000,
|
||||
pendingCleanupController: { bootId: "earlier-boot", pid: await exitedPid(), hostHash: currentProcessSpaceHash(),
|
||||
processStartedAt: null, attemptId } });
|
||||
const { kind } = await report("environment_teardown_claim");
|
||||
expect(kind).toMatchObject({ verdicts: { gone: 1 }, reasons: { pid_dead: 1 } });
|
||||
});
|
||||
|
||||
it("never reads a controller stamped for an earlier attempt: the claim is a fence held by its token", async () => {
|
||||
const attemptId = randomUUID();
|
||||
await claim({ pendingCleanupAttemptId: attemptId, pendingCleanupLeaseExpiresAtMs: Date.now() + 60_000,
|
||||
pendingCleanupController: { ...currentControllerProcessStamp(), bootId: "earlier-boot", attemptId: randomUUID() } });
|
||||
const { kind, violations } = await report("environment_teardown_claim");
|
||||
expect(kind).toMatchObject({ verdicts: { alive: 1 }, reasons: { fence_held: 1 } });
|
||||
expect(violations).toEqual([]);
|
||||
});
|
||||
|
||||
it("keeps an unstamped claim whose fence expired unprovable, never gone", async () => {
|
||||
await claim({ pendingCleanupAttemptId: randomUUID(), pendingCleanupLeaseExpiresAtMs: Date.now() - 1 });
|
||||
const { kind } = await report("environment_teardown_claim");
|
||||
expect(kind).toMatchObject({ verdicts: { unprovable: 1 }, reasons: { fence_expired_without_provider_proof: 1 } });
|
||||
});
|
||||
|
||||
it("names no owner for an in-flight flag without an attempt", async () => {
|
||||
await claim({});
|
||||
const { kind, violations } = await report("environment_teardown_claim");
|
||||
expect(kind).toMatchObject({ verdicts: { unprovable: 1 }, reasons: { no_owner_stamp: 1 } });
|
||||
expect(violations.map(violation => violation.rule)).toEqual(["I1_owned"]);
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -319,6 +319,34 @@ function runLeaseKind(): OwnableKind {
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Owner of an in-flight teardown claim. A claimant that stamps
|
||||
* `pendingCleanupController` for this exact attempt is a process, proved by
|
||||
* identity. Other claimants (workspace recovery, native export retry, and
|
||||
* claims saved by an earlier release) hold the claim as a fence: the attempt
|
||||
* id is the token and `pendingCleanupLeaseExpiresAtMs` is the deadline that
|
||||
* the claimant renews. An expired fence is never proof by itself; another
|
||||
* host takes over only through the fence. A controller stamp left by an
|
||||
* earlier attempt names a different claimant and is ignored.
|
||||
*/
|
||||
function teardownClaimOwner(row: LeaseRow): RecordOwner {
|
||||
const metadata = json(row.metadata);
|
||||
const attemptId = str(metadata.pendingCleanupAttemptId);
|
||||
const controller = json(metadata.pendingCleanupController);
|
||||
if (attemptId && controller.attemptId === attemptId) {
|
||||
const stamped = controllerStamp(controller, "environment_teardown_claim");
|
||||
if (stamped.kind !== "none") return stamped;
|
||||
}
|
||||
if (!attemptId) return { kind: "none" };
|
||||
const expiresAtMs = typeof metadata.pendingCleanupLeaseExpiresAtMs === "number" ? metadata.pendingCleanupLeaseExpiresAtMs : null;
|
||||
return {
|
||||
kind: "fence",
|
||||
stamp: { kind: "remote", scope: "environment_teardown_claim", generation: null, fenceToken: attemptId },
|
||||
fence: { currentFenceToken: attemptId, currentGeneration: null,
|
||||
leaseExpiresAt: expiresAtMs !== null && Number.isFinite(expiresAtMs) ? new Date(expiresAtMs) : null },
|
||||
};
|
||||
}
|
||||
|
||||
/** `ControllerProcess` metadata ({bootId, pid, hostHash, processStartedAt}). */
|
||||
function controllerStamp(value: unknown, scope: string): RecordOwner {
|
||||
const controller = readControllerProcess(value);
|
||||
@@ -554,7 +582,7 @@ export const OWNABLE_KINDS: readonly OwnableKind[] = [
|
||||
runLeaseKind(),
|
||||
leaseKind("environment_teardown_claim", "Teardown claim (in-flight cleanup)", "ownership",
|
||||
and(eq(environmentLeases.status, "pending_cleanup"), sql`(${environmentLeases.metadata}->>'pendingCleanupInFlight')::boolean is true`),
|
||||
row => controllerStamp(json(row.metadata).pendingCleanupController, "environment_teardown_claim")),
|
||||
teardownClaimOwner),
|
||||
leaseKind("environment_pending_cleanup", "Pending-cleanup obligation (not in flight)", "ownership",
|
||||
and(eq(environmentLeases.status, "pending_cleanup"),
|
||||
sql`coalesce((${environmentLeases.metadata}->>'pendingCleanupInFlight')::boolean, false) is false`),
|
||||
@@ -569,7 +597,10 @@ export const OWNABLE_KINDS: readonly OwnableKind[] = [
|
||||
eq(environmentLeases.status, "retained"), () => ({ kind: "none" }), [1, 7]),
|
||||
spoolKind("sandbox_orphan_cleanup_spool", "Orphan cleanup buffer/spool", "ownership", idleOrphanSpoolPath),
|
||||
leaseKind("environment_login_probe_lease", "Login/probe lease", "ownership",
|
||||
and(eq(environmentLeases.status, "active"), isNull(environmentLeases.heartbeatRunId)), () => ({ kind: "none" })),
|
||||
and(eq(environmentLeases.status, "active"), isNull(environmentLeases.heartbeatRunId)),
|
||||
// Stamped by the write that makes the lease active. A lease saved before
|
||||
// the stamp existed names no owner; it ends with its login session.
|
||||
row => controllerStamp(json(row.metadata)[ENVIRONMENT_LEASE_CONTROLLER_KEY], "environment_login_probe_lease")),
|
||||
|
||||
// Workspaces
|
||||
operationKind("workspace_runtime_control_operation", "Managed runtime-control operation", "ownership",
|
||||
|
||||
Reference in new issue
Block a user