From 838e9bf3d2d860989ed3db119da598cebff9e547 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Sun, 8 Nov 2026 01:07:58 -0600 Subject: [PATCH 1/9] feat(recovery): settle sandbox creation intents of dead creators on the shared ownership path (#19978) 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 > - Before it asks a provider to create a sandbox, the server writes a creation intent that names its creator process > - If the creator dies during the provider call, the intent waits for a 30-minute settle deadline before recovery reconciles it with the provider > - After a restart, creation recovery made such intents due early with its own process check (`promoteAbandonedSandboxCreations`) > - Phase 2 of the durable ownership plan moves each in-progress record kind onto one shared owner proof and deletes its bespoke check > - This pull request registers a settle for `sandbox_creation_intent` and calls the shared path where creation recovery called the bespoke promotion > - The benefit is one proof for every kind, while the provider absence proof and the cross-host handoff stay unchanged ## Linked Issues or Issue Description Refs #19964 Refs #19933 **What happened?** Sandbox creation recovery had its own liveness check (`controllerProcessExited`) for the creator of a pending intent. Each kind with its own check can drift from the shared rules (PID space, PID reuse, current boot, deployment replacement). **Expected behavior** The shared prover judges the creator. On gone, the intent becomes due now and its creator is marked lost. Provider absence stays inconclusive until the settle deadline. A creator on another host is never proven gone; its intent keeps the deadline. **Steps to reproduce** 1. Start a sandbox acquire so a creation intent is written. 2. Kill the server before the provider reply is committed. 3. Restart. The intent must become due through the shared ownership path, not a separate check. ## What Changed - `sandbox-creation-intent-settlement.ts` (new): compare-and-swap settle on the exact creator and a deadline that is still in the future. It sets `nextAttemptAt` to now, marks `creationControllerLost` with the proof reason, and wakes creation recovery in the same transaction. - `recovery/ownership-registry.ts`: `sandbox_creation_intent` registers the settle. An intent whose creator is already lost is no longer listed; it belongs to the creation recovery deadline. - `sandbox-creation-operations.ts`: creation recovery calls `settleOwnableKind(db, "sandbox_creation_intent")`. Deleted `promoteAbandonedSandboxCreations` and its use of `controllerProcessExited`. ## Verification - `cd server && npx vitest run src/__tests__/sandbox-creation-intent-ownership.test.ts src/__tests__/sandbox-creation-operations.test.ts src/__tests__/ownership-reconciliation-report.test.ts` (embedded Postgres). - The existing lost-creator matrix stays green on the shared path: destroy on the next sweep; page past unprovable creators; issue-bound creation after a later run; reused PID; live creator here; live creator in another process; creator on another host; early lookup finds nothing yet (absence inconclusive); committed lease untouched. - New, red on master and green here (written after the code): the reconciler pass makes a dead creator's intent due now and keeps the absence deadline. - New counter-cases (green): live creator, another host, and an intent without a creator keep their deadline; a read-only pass changes nothing. - Full typecheck is left to CI (local memory limit). ## Risks - Low risk. The settle writes the same state the deleted promotion wrote, plus the proof reason. - Previous-release rows: every creation intent already stored its creator with a PID space, so no fallback is needed. > 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 --- .../ownership-reconciliation-report.test.ts | 4 +- .../sandbox-creation-intent-ownership.test.ts | 135 ++++++++++++++++++ .../sandbox-creation-operations.test.ts | 4 +- .../services/recovery/ownership-registry.ts | 10 +- .../sandbox-creation-intent-settlement.ts | 55 +++++++ .../services/sandbox-creation-operations.ts | 43 ++---- 6 files changed, 211 insertions(+), 40 deletions(-) create mode 100644 server/src/__tests__/sandbox-creation-intent-ownership.test.ts create mode 100644 server/src/services/sandbox-creation-intent-settlement.ts diff --git a/server/src/__tests__/ownership-reconciliation-report.test.ts b/server/src/__tests__/ownership-reconciliation-report.test.ts index e303af8eaef..f61af00bfe5 100644 --- a/server/src/__tests__/ownership-reconciliation-report.test.ts +++ b/server/src/__tests__/ownership-reconciliation-report.test.ts @@ -142,8 +142,8 @@ describe("report-only ownership reconciliation", () => { expect(new Set(names).size).toBe(names.length); expect(names.length).toBeGreaterThanOrEqual(40); expect(OWNABLE_KINDS.filter(kind => kind.settle !== null).map(kind => kind.id)) - .toEqual(["native_accepted_result_lease", "native_finalizer_lease", "native_cleanup_lease", "environment_run_lease_active", - "environment_teardown_claim", "native_workspace_finalize_attempt", "wake_request_claimed", "legacy_controller_lease"]); + .toEqual(["native_accepted_result_lease", "native_finalizer_lease", "native_cleanup_lease", "sandbox_creation_intent", + "environment_run_lease_active", "environment_teardown_claim", "native_workspace_finalize_attempt", "wake_request_claimed", "legacy_controller_lease"]); expect(OWNABLE_KINDS.filter(kind => !kind.listInProgress).every(kind => kind.coverageGap)).toBe(true); }); diff --git a/server/src/__tests__/sandbox-creation-intent-ownership.test.ts b/server/src/__tests__/sandbox-creation-intent-ownership.test.ts new file mode 100644 index 00000000000..55f6137a670 --- /dev/null +++ b/server/src/__tests__/sandbox-creation-intent-ownership.test.ts @@ -0,0 +1,135 @@ +import { spawn } from "node:child_process"; +import { randomUUID } from "node:crypto"; +import { eq } from "drizzle-orm"; +import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest"; +import { companies, createDb, sandboxCreationOperations } from "@paperclipai/db"; +import { getEmbeddedPostgresTestSupport, startEmbeddedPostgresTestDatabase } from "./helpers/embedded-postgres.js"; +import { currentControllerProcessStamp, currentProcessSpaceHash } from "../services/controller-process-identity.js"; +import { nextEnvironmentCleanupAt } from "../services/environment-cleanup-state.js"; +import { createOwnershipReportState, runOwnershipReconciliationPass } from "../services/recovery/ownership-invariant-report.js"; +import { settleSandboxCreationIntent } from "../services/sandbox-creation-intent-settlement.js"; + +const support = await getEmbeddedPostgresTestSupport(); +const suite = support.supported ? describe : describe.skip; +const KIND = "sandbox_creation_intent"; + +async function exitedPid(): Promise { + const child = spawn(process.execPath, ["-e", "setInterval(() => {}, 1000)"], { stdio: "ignore" }); + const pid = child.pid!; + await new Promise(resolve => { child.once("exit", () => resolve()); child.kill("SIGKILL"); }); + return pid; +} + +suite("sandbox_creation_intent on the shared ownership path", () => { + let db!: ReturnType; + let temporary: Awaited> | null = null; + let companyId = ""; + + beforeAll(async () => { + temporary = await startEmbeddedPostgresTestDatabase("paperclip-sandbox-creation-intent-ownership-"); + db = createDb(temporary.connectionString); + companyId = randomUUID(); + await db.insert(companies).values({ id: companyId, name: "Paperclip", issuePrefix: "SCI", + requireBoardApprovalForNewAgents: false }); + }, 30_000); + + afterEach(async () => { + await db.delete(sandboxCreationOperations); + }); + + afterAll(async () => { + await temporary?.cleanup(); + }); + + /** The state a crash between the intent write and the provider reply leaves: + * a pending intent waiting for its 30-minute settle deadline. */ + async function intent(creationController: Record) { + const id = randomUUID(); + const settleAt = new Date(Date.now() + 30 * 60_000); + await db.insert(sandboxCreationOperations).values({ id, companyId, originalEnvironmentId: randomUUID(), + runId: randomUUID(), requestKey: "run", provider: "fixture", driverKey: "fixture", state: "pending", config: {}, + nextAttemptAt: settleAt, metadata: { creationController, creationSettleAt: settleAt.toISOString() } }); + return id; + } + + async function row(id: string) { + const [found] = await db.select().from(sandboxCreationOperations).where(eq(sandboxCreationOperations.id, id)); + return found!; + } + + async function pass(record = true) { + const report = await runOwnershipReconciliationPass(db, { state: createOwnershipReportState(), record }); + return report.kinds.find(entry => entry.kind === KIND)!; + } + + it("makes a dead creator's intent due on the next pass, keeping the provider absence deadline", async () => { + const id = await intent({ bootId: "earlier-boot", pid: await exitedPid(), hostHash: currentProcessSpaceHash(), + processStartedAt: null }); + expect(await pass()).toMatchObject({ settled: 1, reasons: { pid_dead: 1 } }); + const settled = await row(id); + expect(settled.state).toBe("pending"); + expect(settled.nextAttemptAt.getTime()).toBeLessThanOrEqual(Date.now()); + expect(settled.metadata).toMatchObject({ creationControllerLost: true, creationControllerLostProof: "pid_dead" }); + // Creation recovery is due now; the saved settle deadline still guards absence. + expect(await nextEnvironmentCleanupAt(db)).toBeLessThanOrEqual(Date.now()); + expect(Date.parse(String(settled.metadata.creationSettleAt))).toBeGreaterThan(Date.now() + 25 * 60_000); + // A lost creator's intent belongs to creation recovery now. + expect(await pass()).toMatchObject({ inProgress: 0 }); + }); + + it("keeps the deadline of a live creator, a creator on another host, and an intent with no creator", async () => { + const ids = [ + await intent(currentControllerProcessStamp()), + await intent({ bootId: "other", pid: await exitedPid(), hostHash: "f".repeat(32), processStartedAt: null }), + await intent({}), + ]; + const before = await Promise.all(ids.map(row)); + expect(await pass()).toMatchObject({ settled: 0, + reasons: { current_process: 1, different_process_space: 1, no_owner_stamp: 1 } }); + expect(await Promise.all(ids.map(row))).toEqual(before); + }); + + describe("the settle compares and swaps on the exact creator it was given", () => { + async function record(id: string, creator: Record) { + return { recordId: id, companyId, runId: null, issueId: null, updatedAt: new Date(), + owner: { kind: "process" as const, stamp: { kind: "local" as const, scope: KIND, generation: null, host: null, + processSpace: String(creator.hostHash), bootId: String(creator.bootId), pid: Number(creator.pid), + processStartedAt: null, processGroupId: null } } }; + } + const proof = { verdict: "gone" as const, reason: "pid_dead" as const }; + + it.each([ + ["a creator that changed since the proof", "owner_changed_since_proof"], + ["an intent already due", "intent_already_due"], + ["an intent no longer pending", "intent_no_longer_pending"], + ["an intent whose creator is already lost", "creator_already_lost"], + ] as const)("leaves %s unchanged", async (label, reason) => { + const creator = { bootId: "earlier-boot", pid: await exitedPid(), hostHash: currentProcessSpaceHash(), processStartedAt: null }; + const id = await intent(creator); + if (label.startsWith("a creator")) { + await db.update(sandboxCreationOperations).set({ metadata: { creationController: { ...creator, bootId: "newer-boot" } } }) + .where(eq(sandboxCreationOperations.id, id)); + } else if (label.includes("already due")) { + await db.update(sandboxCreationOperations).set({ nextAttemptAt: new Date(Date.now() - 1_000) }) + .where(eq(sandboxCreationOperations.id, id)); + } else if (label.includes("no longer pending")) { + await db.update(sandboxCreationOperations).set({ state: "cleanup_pending" }).where(eq(sandboxCreationOperations.id, id)); + } else { + await db.update(sandboxCreationOperations).set({ metadata: { creationController: creator, creationControllerLost: true } }) + .where(eq(sandboxCreationOperations.id, id)); + } + const before = await row(id); + expect(await settleSandboxCreationIntent(db, await record(id, creator), { now: new Date(), proof })) + .toEqual({ settled: false, reason }); + expect(await row(id)).toEqual(before); + }); + }); + + it("never changes an intent on a read-only pass", async () => { + const id = await intent({ bootId: "earlier-boot", pid: await exitedPid(), hostHash: currentProcessSpaceHash(), + processStartedAt: null }); + const before = await row(id); + expect(await pass(false)).toMatchObject({ settled: 0, verdicts: { gone: 1 } }); + expect(await row(id)).toEqual(before); + }); +}); diff --git a/server/src/__tests__/sandbox-creation-operations.test.ts b/server/src/__tests__/sandbox-creation-operations.test.ts index d66102567f0..784f7983a7a 100644 --- a/server/src/__tests__/sandbox-creation-operations.test.ts +++ b/server/src/__tests__/sandbox-creation-operations.test.ts @@ -367,12 +367,12 @@ describe("durable sandbox creation through the runtime entry point", () => { await runtime().retryPendingSandboxTeardown({ environment: null, lease: (await environmentService(db).getLeaseById(lease!.id))! }); expect(resources.size).toBe(0); expect(destroys).toBe(1); expect(creates).toBe(1); }); - it("pages past more unprovable creations than one page to find an abandoned one", async () => { + it("pages past more unprovable creations than one settle page (200) to find an abandoned one", async () => { const pid = await exitedPid(); const operation = await interruptedCreation(async current => ({ ...current, bootId: randomUUID(), pid })); const { id: _id, runId: _runId, ...row } = operation; // Sorted ahead of the abandoned intent: creators on another host cannot be judged. - await db.insert(sandboxCreationOperations).values(Array.from({ length: 105 }, (_, index) => ({ ...row, + await db.insert(sandboxCreationOperations).values(Array.from({ length: 205 }, (_, index) => ({ ...row, id: `00000000-0000-4000-8000-${String(index).padStart(12, "0")}`, runId: randomUUID(), metadata: { ...row.metadata, creationController: { ...(row.metadata.creationController as object), bootId: randomUUID(), pid, hostHash: "f".repeat(32) } } }))); await runtime().flushDeferredOrphanCleanups(); diff --git a/server/src/services/recovery/ownership-registry.ts b/server/src/services/recovery/ownership-registry.ts index aee032246e4..2fde5d7d902 100644 --- a/server/src/services/recovery/ownership-registry.ts +++ b/server/src/services/recovery/ownership-registry.ts @@ -49,6 +49,7 @@ import { } from "../environment-cleanup-state.js"; import { settleEnvironmentRunLease } from "../environment-run-lease-settlement.js"; import { settleEnvironmentTeardownClaim } from "../environment-teardown-claim-settlement.js"; +import { settleSandboxCreationIntent } from "../sandbox-creation-intent-settlement.js"; /** * Registry of ownable record kinds (plan section C). @@ -633,13 +634,18 @@ export const OWNABLE_KINDS: readonly OwnableKind[] = [ // Environments and sandboxes { + // Shared path since Phase 2: a creator proven gone makes its intent due + // now; creation recovery then reconciles it with the provider. An intent + // whose creator is already marked lost belongs to that recovery deadline. id: "sandbox_creation_intent", title: "Sandbox creation intent", obligation: "ownership", storage: "database", - settle: null, invariantChecks: [1, 2, 3, 7], + settle: settleSandboxCreationIntent, invariantChecks: [1, 2, 3, 7], listInProgress: async (db, limit, offset) => { const rows = await db.select({ id: sandboxCreationOperations.id, companyId: sandboxCreationOperations.companyId, runId: sandboxCreationOperations.runId, issueId: sandboxCreationOperations.issueId, metadata: sandboxCreationOperations.metadata, updatedAt: sandboxCreationOperations.updatedAt }) - .from(sandboxCreationOperations).where(eq(sandboxCreationOperations.state, "pending")).orderBy(asc(sandboxCreationOperations.id)).limit(limit + 1).offset(offset); + .from(sandboxCreationOperations).where(and(eq(sandboxCreationOperations.state, "pending"), + sql`coalesce(${sandboxCreationOperations.metadata}->>'creationControllerLost', '') <> 'true'`)) + .orderBy(asc(sandboxCreationOperations.id)).limit(limit + 1).offset(offset); const result = page(rows, limit); return { truncated: result.truncated, records: result.rows.map(row => ({ recordId: row.id, companyId: row.companyId, runId: row.runId, issueId: row.issueId, diff --git a/server/src/services/sandbox-creation-intent-settlement.ts b/server/src/services/sandbox-creation-intent-settlement.ts new file mode 100644 index 00000000000..766aa2d30d9 --- /dev/null +++ b/server/src/services/sandbox-creation-intent-settlement.ts @@ -0,0 +1,55 @@ +import { and, eq, gt, sql } from "drizzle-orm"; +import { sandboxCreationOperations, type Db } from "@paperclipai/db"; +import { readControllerProcess } from "./controller-process-identity.js"; +import { notifyEnvironmentCleanup } from "./environment-cleanup-notifications.js"; +import type { OwnerProof } from "./owner-proof.js"; +import type { InProgressRecord, SettleOutcome } from "./recovery/ownership-registry.js"; + +/** + * Settle of the `sandbox_creation_intent` ownership kind (plan Phase 2). + * + * A creation intent is written before the provider call and names the server + * process that makes it. While that process lives, the intent waits for its + * settle deadline. Once the shared prover proves the creator gone, it cannot + * still be waiting for the provider, so this settle makes the intent due now + * and marks the creator lost; the creation recovery worker then reconciles it. + * + * It settles ownership only. Whether the sandbox exists is still decided by + * the provider: absence stays inconclusive until the creation settle deadline + * (`creationAbsenceInconclusive`), and an unidentifiable creation becomes an + * operator reconciliation. A creator on another host is never proven gone by + * a process proof; its intent keeps the deadline, which is the handoff. + * + * The update compares and swaps on the exact creator the proof judged and + * only moves a deadline that is still in the future. + */ +export async function settleSandboxCreationIntent(db: Db, record: InProgressRecord, + context: { now: Date; proof: OwnerProof }): Promise { + if (record.owner.kind !== "process") return { settled: false, reason: "owner_not_a_process" }; + const stamp = record.owner.stamp; + const now = context.now; + return db.transaction(async transaction => { + const tx = transaction as unknown as Db; + const [operation] = await tx.select().from(sandboxCreationOperations) + .where(and(eq(sandboxCreationOperations.id, record.recordId), eq(sandboxCreationOperations.state, "pending"))) + .for("update"); + if (!operation) return { settled: false, reason: "intent_no_longer_pending" }; + if (operation.metadata.creationControllerLost === true) return { settled: false, reason: "creator_already_lost" }; + const creator = readControllerProcess(operation.metadata.creationController); + if (!creator || creator.bootId !== stamp.bootId || creator.pid !== stamp.pid || creator.hostHash !== stamp.processSpace) { + return { settled: false, reason: "owner_changed_since_proof" }; + } + if (operation.nextAttemptAt <= now) return { settled: false, reason: "intent_already_due" }; + // Wake creation recovery before the write it announces. + await notifyEnvironmentCleanup(tx, { companyId: operation.companyId, runId: operation.runId }); + const updated = await tx.update(sandboxCreationOperations).set({ nextAttemptAt: now, updatedAt: now, + metadata: sql`coalesce(${sandboxCreationOperations.metadata}, '{}'::jsonb) || ${JSON.stringify({ + creationControllerLost: true, creationControllerLostProof: context.proof.reason })}::jsonb` }) + .where(and(eq(sandboxCreationOperations.id, operation.id), eq(sandboxCreationOperations.state, "pending"), + gt(sandboxCreationOperations.nextAttemptAt, now), + sql`coalesce(${sandboxCreationOperations.metadata}->>'creationControllerLost', '') <> 'true'`, + sql`${sandboxCreationOperations.metadata}->'creationController' = ${JSON.stringify(operation.metadata.creationController)}::jsonb`)) + .returning({ id: sandboxCreationOperations.id }); + return updated.length ? { settled: true, reason: context.proof.reason } : { settled: false, reason: "intent_no_longer_pending" }; + }); +} diff --git a/server/src/services/sandbox-creation-operations.ts b/server/src/services/sandbox-creation-operations.ts index 8e11ce48c37..bbc8369962e 100644 --- a/server/src/services/sandbox-creation-operations.ts +++ b/server/src/services/sandbox-creation-operations.ts @@ -1,7 +1,7 @@ import { collectSecretRefPaths, isUuidSecretRef, parseSecretRefBindingObject, readConfigValueAtPath } from "./json-schema-secret-refs.js"; import { createHash, randomUUID } from "node:crypto"; -import { and, asc, eq, gt, lte, or, sql } from "drizzle-orm"; -import { controllerProcessExited, currentControllerBootId, currentControllerProcess, readControllerProcess } from "./controller-process-identity.js"; +import { and, eq, lte, or, sql } from "drizzle-orm"; +import { currentControllerProcess } from "./controller-process-identity.js"; import { companies, environments, sandboxCreationOperations, type Db } from "@paperclipai/db"; import { readEnvironmentCreationCleanupError, type PluginEnvironmentLease, type PluginEnvironmentLeaseObservation } from "@paperclipai/plugin-sdk"; import { assertEnvironmentCompanyBinding, environmentService } from "./environments.js"; @@ -15,37 +15,6 @@ export type SandboxCreationOperation = typeof sandboxCreationOperations.$inferSe // A live creator may still be waiting for the provider, and a provider may still // materialize an aborted request. Absence is conclusive only after this delay. const CREATION_SETTLE_FLOOR_MS = 30 * 60_000; -const CREATION_PROMOTION_PAGE_SIZE = 100; - -/** After a restart, creation intents from an exited server cannot still be in - * flight there. Reconcile them now instead of at the settle deadline. Absence - * stays inconclusive until that deadline; see recoverSandboxCreations. */ -async function promoteAbandonedSandboxCreations(db: Db): Promise { - const now = new Date(); - // Page past live, foreign, and unidentified creators so they cannot hide an - // abandoned intent behind them. - let after: string | null = null; - for (;;) { - const waiting: Array> = await db - .select({ id: sandboxCreationOperations.id, metadata: sandboxCreationOperations.metadata }) - .from(sandboxCreationOperations).where(and( - eq(sandboxCreationOperations.state, "pending"), gt(sandboxCreationOperations.nextAttemptAt, now), - sql`${sandboxCreationOperations.metadata}->'creationController'->>'bootId' is distinct from ${currentControllerBootId()}`, - sql`coalesce(${sandboxCreationOperations.metadata}->>'creationControllerLost', '') <> 'true'`, - after === null ? undefined : gt(sandboxCreationOperations.id, after), - )).orderBy(asc(sandboxCreationOperations.id)).limit(CREATION_PROMOTION_PAGE_SIZE); - for (const operation of waiting) { - const controller = readControllerProcess(operation.metadata.creationController); - if (!controller || !await controllerProcessExited(controller)) continue; - await db.update(sandboxCreationOperations).set({ nextAttemptAt: now, updatedAt: now, - metadata: sql`coalesce(${sandboxCreationOperations.metadata}, '{}'::jsonb) || '{"creationControllerLost":true}'::jsonb` }) - .where(and(eq(sandboxCreationOperations.id, operation.id), eq(sandboxCreationOperations.state, "pending"), - gt(sandboxCreationOperations.nextAttemptAt, now))); - } - if (waiting.length < CREATION_PROMOTION_PAGE_SIZE) return; - after = waiting[waiting.length - 1]!.id; - } -} function creationAbsenceInconclusive(operation: SandboxCreationOperation): boolean { const settleAt = Date.parse(typeof operation.metadata.creationSettleAt === "string" ? operation.metadata.creationSettleAt : ""); @@ -199,7 +168,13 @@ export async function recoverSandboxCreations(db: Db, inspect: (operation: SandboxCreationOperation) => Promise, onTaskRecovered?: (operation: SandboxCreationOperation) => void | Promise, ): Promise { - await promoteAbandonedSandboxCreations(db); + // After a restart, an intent whose creator is proven gone cannot still be in + // flight there: the shared ownership path makes it due now. Absence stays + // inconclusive until the settle deadline (creationAbsenceInconclusive). + const { settleOwnableKind } = await import("./recovery/ownership-invariant-report.js"); + await settleOwnableKind(db, "sandbox_creation_intent").catch(() => { + logger.warn({ errorKind: "sandbox_creation_intent_settle_failed" }, "Sandbox creation intents keep their saved deadlines"); + }); const due = await db.select().from(sandboxCreationOperations).where(and( recoverableSandboxCreationCondition(), lte(sandboxCreationOperations.nextAttemptAt, new Date()), )).limit(100); From 2fed77086fbb2fe0c72efff32623c21ff277b4a2 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Sun, 8 Nov 2026 01:20:00 -0600 Subject: [PATCH 2/9] fix(recovery): date a settled orphan's first cleanup even under a scalar metadata root (#19989) 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 > - #19980 made the environment cleanup queue the owner of an unclaimed pending-cleanup item, with a due time and an overdue check (I7) > - A settled orphan's first cleanup attempt is dated by `pendingCleanupRecoveredAtMs`, which the orphaned-run-lease settle writes > - The settle merged that key with `coalesce(metadata, '{}') || ...`. For an array or scalar metadata root that produces an array, so the key is not at the top level > - The queue owner then falls back to the old `updatedAt` and can report a fresh item as overdue > - This pull request builds the merge from the object-root reader that the cleanup sweep already uses > - The benefit is that a fresh orphan is never reported overdue because of a malformed metadata root ## Linked Issues or Issue Description Refs #19980 Refs #19964 **What happened?** The review of #19980 found that the orphaned-run-lease settle wrote `pendingCleanupRecoveredAtMs` under an array metadata root as an array element. #19980 merged before this fix landed on it. **Expected behavior** The key is always top level, so the queue owner dates the first cleanup attempt correctly. **Steps to reproduce** 1. Save an active run lease whose metadata root is a JSON array. 2. Let its run end and the ownership settle move it to `pending_cleanup`. 3. Run the ownership pass: the item can read as `queue_overdue`. ## What Changed - `environment-run-lease-settlement.ts`: the merge starts from `pendingCleanupMetadataObjectSql()`, which reads a scalar or array root as an empty object. - `environment-run-lease-ownership.test.ts`: a legacy lease with an array metadata root is settled with the key at the top level. ## Verification - `cd server && npx vitest run src/__tests__/environment-run-lease-ownership.test.ts src/__tests__/environment-pending-cleanup-queue-owner.test.ts` (embedded Postgres). - Full typecheck is left to CI (local memory limit). ## Risks - Low risk. It changes only how one key is merged into a lease that the settle moves to `pending_cleanup`. A malformed root becomes an object, which the cleanup sweep already does for its own keys. > 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 --- .../environment-run-lease-ownership.test.ts | 13 ++++++++++++- .../services/environment-run-lease-settlement.ts | 4 +++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/server/src/__tests__/environment-run-lease-ownership.test.ts b/server/src/__tests__/environment-run-lease-ownership.test.ts index a8a666a0ebc..669293cf660 100644 --- a/server/src/__tests__/environment-run-lease-ownership.test.ts +++ b/server/src/__tests__/environment-run-lease-ownership.test.ts @@ -1,6 +1,6 @@ import { spawn } from "node:child_process"; import { randomUUID } from "node:crypto"; -import { eq } from "drizzle-orm"; +import { eq, sql } from "drizzle-orm"; import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest"; import { agents, @@ -257,6 +257,17 @@ suite("environment_run_lease_active on the shared ownership path", () => { }); describe("leases saved before holders were stamped", () => { + it("dates the first cleanup attempt as a top-level key even for a scalar metadata root", async () => { + const runId = await run("failed"); + const id = randomUUID(); + await db.insert(environmentLeases).values({ id, companyId, heartbeatRunId: runId, status: "active", + metadata: sql`'["provider-wrote-an-array"]'::jsonb` }); + await runOwnershipReconciliationPass(db, { state: createOwnershipReportState() }); + const settled = await row(id); + expect(settled.status).toBe("pending_cleanup"); + expect(typeof settled.metadata?.pendingCleanupRecoveredAtMs).toBe("number"); + }); + it("keeps the run fact alone for one release on a single-host server", async () => { const runId = await run("failed"); const leaseId = await lease(runId, null); diff --git a/server/src/services/environment-run-lease-settlement.ts b/server/src/services/environment-run-lease-settlement.ts index fb1c79a471e..f6020a1ed6f 100644 --- a/server/src/services/environment-run-lease-settlement.ts +++ b/server/src/services/environment-run-lease-settlement.ts @@ -2,6 +2,7 @@ import { and, eq, inArray, ne, notInArray, or, sql } from "drizzle-orm"; import { environmentLeases, environments, heartbeatRuns, nativeRunFinalizations, nativeRunResults, type Db } from "@paperclipai/db"; import { currentControllerBootId } from "./controller-process-identity.js"; import { writeEnvironmentCleanup } from "./environment-cleanup-notifications.js"; +import { pendingCleanupMetadataObjectSql } from "./environment-cleanup-state.js"; import { readEnvironmentLeaseController } from "./environment-lease-controller.js"; import { allowLegacyShutdownWorkspaceCleanup } from "./legacy-shutdown-workspace-settlement.js"; import { nativeWorkspaceExportPendingForLease } from "./native-runtime/native-workspace-finalization-state.js"; @@ -77,7 +78,8 @@ export async function settleEnvironmentRunLease(db: Db, record: InProgressRecord const flipped = await writeEnvironmentCleanup(tx, write => write.update(environmentLeases) .set({ status: "pending_cleanup", failureReason: ORPHANED_RUN_LEASE_FAILURE_REASON, // When the first cleanup attempt became due (the row keeps updatedAt). - metadata: sql`coalesce(${environmentLeases.metadata}, '{}'::jsonb) || ${JSON.stringify({ + // A scalar or array root reads as an empty object, so the key is top level. + metadata: sql`${pendingCleanupMetadataObjectSql()} || ${JSON.stringify({ pendingCleanupRecoveredAtMs: context.now.getTime() })}::jsonb` }) .where(and(eq(environmentLeases.id, lease.id), eq(environmentLeases.status, "active"), eq(environmentLeases.heartbeatRunId, runId), From dc0534d1b55d5e24500cf51c1ba5018df40fd794 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Sun, 8 Nov 2026 01:38:22 -0600 Subject: [PATCH 3/9] feat(recovery): destroy a retained orphan sandbox once nothing references it (#19976) 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 > - Agent runs use remote sandboxes held by environment leases > - #19964 settles an orphaned active lease when its holder is proven gone. It stops and keeps the sandbox, because recovery may still need its files > - Nothing destroyed that retained orphan later, so a stopped sandbox could stay at the provider indefinitely > - This pull request destroys a retained orphan once no recovery, preservation or retention obligation references it, from the reconciler pass only > - The benefit is that orphaned sandboxes do not leak, and a sandbox that recovery still wants is never destroyed ## Linked Issues or Issue Description Refs #19964 Refs #19933 **What happened?** After #19964, an orphaned sandbox is stopped and kept (`released` with a stop-and-retain receipt). No path destroyed it when recovery no longer needed it. **Expected behavior** A retained orphan is destroyed once database facts show that no obligation references it. It is never kept past the environment's retention window (the provider-attested lease `expiresAt`) only because of a soft reference. A sandbox that an active operation or a live recovery path still needs is never destroyed. **Steps to reproduce** 1. Let a run's server die after the run is terminal and before its sandbox lease is released. 2. Let the ownership settle stop and keep the sandbox. 3. Resolve the run's recovery. The stopped sandbox stays at the provider. ## What Changed - `sandbox-stop-and-retain.ts`: an ownership settle's stop intent also writes a top-level `orphanedRunLeaseRecovery` marker. The marker stays after the stop intent is removed. - `environment-retained-orphan-settlement.ts` (new): the settle for `environment_retained_orphan_sandbox`. It locks the lease row and checks database facts: - the run is terminal; - always held: native export resume, stop-and-retain intent, in-flight cleanup claim, workspace restore recovery, workspace recovery cleanup, bound source-recovery consumer, pending legacy shutdown fence, or another live lease on the same provider resource; - held while the stop receipt is a usable source (`expiresAt` not passed): pending native export or preservation fence, open recovery action on the issue, or a queued, scheduled or running run on the issue. - Then it moves the lease to `pending_cleanup` (ephemeral) with a compare-and-swap. The pending-cleanup sweep destroys it from the recorded lease. The settle never calls a provider. - `recovery/ownership-registry.ts`: the kind lists only releasable retained orphans (an SQL mirror of the checks), so a held orphan is never polled. It reports `nextDueAt` = the earliest retention-window end of a softly held orphan. - `recovery/ownership-invariant-report.ts`: an `OwnableKind` may report `nextDueAt`; the pass folds it into the report's next due time, so the reconciler wakes when the window ends. - `environment-cleanup-state.ts`: the first destroy attempt of a released orphan is due at once. ## Verification - `cd server && npx vitest run src/__tests__/environment-retained-orphan-sandbox.test.ts src/__tests__/ownership-reconciliation-report.test.ts src/__tests__/environment-run-lease-ownership.test.ts src/__tests__/heartbeat-orphaned-active-lease-sweep.test.ts src/__tests__/heartbeat-pending-cleanup-sweep.test.ts` (embedded Postgres). - Red on master, green here (tests written after the code): the settle marks the lease; an unreferenced retained orphan is destroyed through the cleanup sweep; a softly held orphan waits until its window ends and the pass reports that due time. - Counter-cases (green): never destroyed with an open recovery action, a queued run, a scheduled retry, a stop-and-retain intent, a native export resume, a workspace restore recovery, an in-flight claim, a pending legacy shutdown fence, another live lease on the resource, or an active run; a read-only pass changes nothing; a retained sandbox without the orphan marker is left alone. - Full typecheck is left to CI (local memory limit). ## Risks - A retained orphan saved by #19964 before this change has no marker, so it is not destroyed by this path. The window was short (since #19964 merged). - A held orphan with no `expiresAt` is released on the first reconciler pass after its last reference clears. If that clearing writes no signal the reconciler wakes on, the release waits for the next pass. - The SQL listing mirrors the settle's checks except the pending native export, which the settle checks in code. Such an orphan stays on the settle watch until that export settles. > 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 --- ...nvironment-retained-orphan-sandbox.test.ts | 272 ++++++++++++++++++ .../ownership-reconciliation-report.test.ts | 3 +- .../chat-channels/task-retry-authority.ts | 8 +- .../src/services/environment-cleanup-state.ts | 5 +- .../environment-retained-orphan-settlement.ts | 197 +++++++++++++ .../recovery/ownership-invariant-report.ts | 13 + .../services/recovery/ownership-registry.ts | 18 ++ .../src/services/sandbox-stop-and-retain.ts | 11 +- 8 files changed, 520 insertions(+), 7 deletions(-) create mode 100644 server/src/__tests__/environment-retained-orphan-sandbox.test.ts create mode 100644 server/src/services/environment-retained-orphan-settlement.ts diff --git a/server/src/__tests__/environment-retained-orphan-sandbox.test.ts b/server/src/__tests__/environment-retained-orphan-sandbox.test.ts new file mode 100644 index 00000000000..66e4d2926d2 --- /dev/null +++ b/server/src/__tests__/environment-retained-orphan-sandbox.test.ts @@ -0,0 +1,272 @@ +import { spawn } from "node:child_process"; +import { randomUUID } from "node:crypto"; +import { eq } from "drizzle-orm"; +import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest"; +import { + agents, + companies, + createDb, + environmentLeases, + environments, + heartbeatRuns, + issueRecoveryActions, + issues, +} from "@paperclipai/db"; +import { getEmbeddedPostgresTestSupport, startEmbeddedPostgresTestDatabase } from "./helpers/embedded-postgres.js"; +import { currentProcessSpaceHash } from "../services/controller-process-identity.js"; +import { nextEnvironmentCleanupAt } from "../services/environment-cleanup-state.js"; +import { ENVIRONMENT_LEASE_CONTROLLER_KEY } from "../services/environment-lease-controller.js"; +import { hasPendingRunCleanup } from "../services/execution-blocker.js"; +import { heartbeatService, type HeartbeatEnvironmentRuntime } from "../services/heartbeat.js"; +import { hasRecoveredCleanupStopReceipt } from "../services/remote-execution-termination.js"; +import { taskChatRetryExecutionStopped } from "../services/chat-channels/task-retry-authority.js"; +import { createOwnershipReportState, runOwnershipReconciliationPass } from "../services/recovery/ownership-invariant-report.js"; +import { ORPHANED_SANDBOX_RECOVERY_KEY } from "../services/sandbox-stop-and-retain.js"; + +const support = await getEmbeddedPostgresTestSupport(); +const suite = support.supported ? describe : describe.skip; +const KIND = "environment_retained_orphan_sandbox"; + +async function exitedPid(): Promise { + const child = spawn(process.execPath, ["-e", "setInterval(() => {}, 1000)"], { stdio: "ignore" }); + const pid = child.pid!; + await new Promise(resolve => { child.once("exit", () => resolve()); child.kill("SIGKILL"); }); + return pid; +} + +suite("retained orphan sandbox on the shared ownership path", () => { + let db!: ReturnType; + let temporary: Awaited> | null = null; + let companyId = ""; + let agentId = ""; + let environmentId = ""; + let issueId = ""; + + beforeAll(async () => { + temporary = await startEmbeddedPostgresTestDatabase("paperclip-retained-orphan-sandbox-"); + db = createDb(temporary.connectionString); + companyId = randomUUID(); agentId = randomUUID(); environmentId = randomUUID(); issueId = randomUUID(); + await db.insert(companies).values({ id: companyId, name: "Paperclip", issuePrefix: "ROS", + requireBoardApprovalForNewAgents: false }); + await db.insert(agents).values({ id: agentId, 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" } }); + await db.insert(issues).values({ id: issueId, companyId, title: "Orphaned work" }); + }, 30_000); + + afterEach(async () => { + await db.delete(issueRecoveryActions); + await db.delete(environmentLeases); + await db.delete(heartbeatRuns); + }); + + afterAll(async () => { + await temporary?.cleanup(); + }); + + async function run(status: string) { + const id = randomUUID(); + await db.insert(heartbeatRuns).values({ id, companyId, agentId, issueId, invocationSource: "on_demand", status, nextEventSeq: 1 }); + return id; + } + + async function row(id: string) { + const [found] = await db.select().from(environmentLeases).where(eq(environmentLeases.id, id)); + return found!; + } + + /** The end state of an orphaned run lease: the ownership settle stopped the + * sandbox and kept it, and the stop was confirmed. */ + async function retainedOrphan(runId: string, extra: { expiresAt?: Date; metadata?: Record } = {}) { + const id = randomUUID(), providerLeaseId = `sandbox://fake/${id}`, now = new Date(); + await db.insert(environmentLeases).values({ id, companyId, environmentId, issueId, heartbeatRunId: runId, + status: "released", cleanupStatus: "success", leasePolicy: "ephemeral", provider: "fake", providerLeaseId, + acquiredAt: new Date(now.getTime() - 60_000), releasedAt: now, expiresAt: extra.expiresAt ?? null, + metadata: { + [ORPHANED_SANDBOX_RECOVERY_KEY]: { recoveredOrphanedRunLease: true, ownerProof: "pid_dead", runId }, + remoteExecutionTermination: { schema: "paperclip.remote-termination.v1", companyId, runId, leaseId: id, + provider: "fake", providerLeaseId, state: "stopped", confirmedAt: now.toISOString() }, + sandboxStopAndRetainReceipt: { schema: "paperclip.sandbox-stop-and-retain-receipt.v1", requestId: randomUUID(), + companyId, runId, leaseId: id, provider: "fake", providerLeaseId, builtinProvider: "fake", + method: "builtin.stopLease", confirmedAt: now.toISOString() }, + ...extra.metadata, + } }); + return id; + } + + async function pass(record = true) { + const report = await runOwnershipReconciliationPass(db, { state: createOwnershipReportState(), record }); + return { report, kind: report.kinds.find(entry => entry.kind === KIND)! }; + } + + it("marks the lease when the orphaned-run-lease settle stops and keeps its sandbox", async () => { + const runId = await run("failed"); + const id = randomUUID(); + await db.insert(environmentLeases).values({ id, companyId, environmentId, heartbeatRunId: runId, status: "active", + provider: "fake", providerLeaseId: `sandbox://fake/${id}`, metadata: { [ENVIRONMENT_LEASE_CONTROLLER_KEY]: { + bootId: "earlier-boot", pid: await exitedPid(), hostHash: currentProcessSpaceHash(), processStartedAt: null } } }); + await pass(); + expect((await row(id)).metadata?.[ORPHANED_SANDBOX_RECOVERY_KEY]).toMatchObject({ + recoveredOrphanedRunLease: true, ownerProof: "pid_dead", runId }); + }); + + it("destroys a retained orphan that nothing references, through the cleanup sweep", async () => { + const leaseId = await retainedOrphan(await run("failed")); + const { kind } = await pass(); + expect(kind).toMatchObject({ inProgress: 1, settled: 1 }); + const released = await row(leaseId); + expect(released).toMatchObject({ status: "pending_cleanup", leasePolicy: "ephemeral", + failureReason: "retained_orphan_sandbox_released" }); + expect(released.metadata?.remoteExecutionTermination).toBeUndefined(); + expect(released.metadata?.workspaceRecoveryCleanup).toMatchObject({ runId: released.heartbeatRunId, + reason: "retained_orphan_sandbox_released", ownerProof: "owner_run_terminal", stopReceipt: { state: "stopped" } }); + // Admission and Retry still see a stopped sandbox while the destroy waits. + expect(hasRecoveredCleanupStopReceipt(released)).toBe(true); + expect(await hasPendingRunCleanup(db, companyId, released.heartbeatRunId!)).toBe(false); + const [failedRun] = await db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, released.heartbeatRunId!)); + expect(await taskChatRetryExecutionStopped(db, failedRun!)).toBe(true); + // Due now, and the sweep destroys it from the recorded lease. + expect(await nextEnvironmentCleanupAt(db)).toBeLessThanOrEqual(Date.now()); + const teardown = vi.fn(async ({ lease }: { lease: { providerLeaseId: string | null } }) => + ({ providerLeaseId: lease.providerLeaseId, state: "destroyed" })); + await heartbeatService(db, { environmentRuntime: { retryPendingSandboxTeardown: teardown } as unknown as HeartbeatEnvironmentRuntime }) + .sweepPendingCleanupLeases({ backoffMs: 5 * 60_000 }); + expect(teardown).toHaveBeenCalledOnce(); + expect((await row(leaseId)).status).toBe("expired"); + }); + + it.each([ + ["an open recovery action on the issue", async (_runId: string) => { + await db.insert(issueRecoveryActions).values({ companyId, sourceIssueId: issueId, kind: "active_run_watchdog", + cause: "fixture", fingerprint: randomUUID(), nextAction: "Recover the workspace." }); + return {}; + }], + ["a queued run on the issue", async (_runId: string) => { await run("queued"); return {}; }], + ["a scheduled retry on the issue", async (_runId: string) => { await run("scheduled_retry"); return {}; }], + ["a stop-and-retain intent", async () => ({ metadata: { sandboxStopAndRetain: { requestId: "pending" } } })], + ["a native export resume", async () => ({ metadata: { nativeWorkspaceExportResume: { requestId: "pending" } } })], + ["a workspace restore recovery", async () => ({ metadata: { workspaceRestoreRecovery: { runId: "fixture" } } })], + ["an in-flight cleanup claim", async () => ({ metadata: { pendingCleanupInFlight: true, pendingCleanupAttemptId: "a" } })], + ["a pending legacy shutdown fence", async () => ({ metadata: { legacyShutdownWorkspaceSettlement: { state: "pending" } } })], + ] as const)("never destroys a sandbox recovery still wants: %s", async (_label, arrange) => { + const runId = await run("failed"); + const extra = await arrange(runId); + const leaseId = await retainedOrphan(runId, extra); + const before = await row(leaseId); + const { kind } = await pass(); + expect(kind).toMatchObject({ settled: 0 }); + expect(await row(leaseId)).toEqual(before); + }); + + it("never destroys a sandbox another live lease holds, or one whose run is still active", async () => { + const live = await retainedOrphan(await run("running")); + const sharedRun = await run("failed"); + const shared = await retainedOrphan(sharedRun); + const sharedRow = await row(shared); + await db.insert(environmentLeases).values({ companyId, environmentId, heartbeatRunId: await run("running"), + status: "active", provider: "fake", providerLeaseId: sharedRow.providerLeaseId }); + await pass(); + expect((await row(live)).status).toBe("released"); + expect((await row(shared)).status).toBe("released"); + }); + + it("keeps a softly referenced orphan only until its retention window ends, and wakes then", async () => { + const runId = await run("failed"); + await db.insert(issueRecoveryActions).values({ companyId, sourceIssueId: issueId, kind: "active_run_watchdog", + cause: "fixture", fingerprint: randomUUID(), nextAction: "Recover the workspace." }); + const expiresAt = new Date(Date.now() + 60 * 60_000); + const leaseId = await retainedOrphan(runId, { expiresAt }); + const first = await pass(); + expect(first.kind).toMatchObject({ inProgress: 0 }); + expect(first.report.nextDueAt?.getTime()).toBeLessThanOrEqual(expiresAt.getTime()); + expect((await row(leaseId)).status).toBe("released"); + // Past the window no recovery path accepts the stopped sandbox as a source. + await db.update(environmentLeases).set({ expiresAt: new Date(Date.now() - 1_000) }).where(eq(environmentLeases.id, leaseId)); + const second = await pass(); + expect(second.kind).toMatchObject({ settled: 1 }); + expect((await row(leaseId)).status).toBe("pending_cleanup"); + }); + + it("never changes anything on a read-only pass", async () => { + const leaseId = await retainedOrphan(await run("failed")); + const before = await row(leaseId); + const { kind } = await pass(false); + expect(kind).toMatchObject({ inProgress: 1, settled: 0, verdicts: { gone: 1 } }); + expect(await row(leaseId)).toEqual(before); + }); + + it("leaves a reusable sandbox to warm reuse", async () => { + const leaseId = await retainedOrphan(await run("failed")); + await db.update(environmentLeases).set({ leasePolicy: "reuse_by_environment" }).where(eq(environmentLeases.id, leaseId)); + const before = await row(leaseId); + const { kind } = await pass(); + expect(kind).toMatchObject({ inProgress: 0 }); + expect(await row(leaseId)).toEqual(before); + }); + + it("keeps a sandbox with a pending native workspace export even past its retention window", async () => { + const runId = await run("failed"); + const leaseId = await retainedOrphan(runId, { expiresAt: new Date(Date.now() - 1_000) }); + const lease = await row(leaseId); + await db.update(heartbeatRuns).set({ runtimeMode: "native", runnerProfileJson: { nativeWorkspaceSync: { + schema: "paperclip.native-workspace-sync/v3", state: "prepared", descriptorSha256: "a".repeat(64), + baselineSha256: "b".repeat(64), finalHostSha256: null, workspaceId: randomUUID(), leaseId, + providerLeaseId: lease.providerLeaseId, remoteCwd: "/workspace", resourceDisposition: "destroy" } } }) + .where(eq(heartbeatRuns.id, runId)); + const { kind } = await pass(); + // Not listed, so the settle watch does not poll it while the export waits. + expect(kind).toMatchObject({ inProgress: 0 }); + expect(await row(leaseId)).toEqual(lease); + }); + + it("releases a workspace-less native run's orphan whose descriptor stays prepared", async () => { + const runId = await run("failed"); + const leaseId = await retainedOrphan(runId); + const lease = await row(leaseId); + await db.update(heartbeatRuns).set({ runtimeMode: "native", runnerProfileJson: { + nativeExecutionInput: { workspace: { access: "none", cwd: null } }, + nativeWorkspaceSync: { schema: "paperclip.native-workspace-sync/v3", state: "prepared", descriptorSha256: "a".repeat(64), + baselineSha256: "b".repeat(64), finalHostSha256: null, workspaceId: randomUUID(), leaseId, + providerLeaseId: lease.providerLeaseId, remoteCwd: "/workspace", resourceDisposition: "destroy" } } }) + .where(eq(heartbeatRuns.id, runId)); + // Listed (no export obligation is visible to the listing); the settle + // decides with the exact export check. + const { kind } = await pass(false); + expect(kind).toMatchObject({ inProgress: 1 }); + }); + + it("releases an orphan whose run's prepared descriptor names another sandbox", async () => { + const runId = await run("failed"); + const leaseId = await retainedOrphan(runId); + await db.update(heartbeatRuns).set({ runtimeMode: "native", runnerProfileJson: { + nativeWorkspaceSync: { schema: "paperclip.native-workspace-sync/v3", state: "prepared", descriptorSha256: "a".repeat(64), + baselineSha256: "b".repeat(64), finalHostSha256: null, workspaceId: randomUUID(), leaseId: randomUUID(), + providerLeaseId: "sandbox://fake/other", remoteCwd: "/workspace", resourceDisposition: "destroy" } } }) + .where(eq(heartbeatRuns.id, runId)); + const { kind } = await pass(); + expect(kind).toMatchObject({ settled: 1 }); + expect((await row(leaseId)).status).toBe("pending_cleanup"); + }); + + it("does not hide an orphan behind a prepared descriptor the settle would reject", async () => { + const runId = await run("failed"); + const leaseId = await retainedOrphan(runId); + const lease = await row(leaseId); + await db.update(heartbeatRuns).set({ runtimeMode: "native", runnerProfileJson: { + nativeWorkspaceSync: { state: "prepared", leaseId, providerLeaseId: lease.providerLeaseId } } }) + .where(eq(heartbeatRuns.id, runId)); + const { kind } = await pass(); + expect(kind).toMatchObject({ settled: 1 }); + expect((await row(leaseId)).status).toBe("pending_cleanup"); + }); + + it("leaves an ordinary retained sandbox (no orphan marker) alone", async () => { + const leaseId = await retainedOrphan(await run("failed")); + const { [ORPHANED_SANDBOX_RECOVERY_KEY]: _marker, ...metadata } = (await row(leaseId)).metadata!; + await db.update(environmentLeases).set({ metadata }).where(eq(environmentLeases.id, leaseId)); + const { kind } = await pass(); + expect(kind).toMatchObject({ inProgress: 0 }); + expect((await row(leaseId)).status).toBe("released"); + }); +}); diff --git a/server/src/__tests__/ownership-reconciliation-report.test.ts b/server/src/__tests__/ownership-reconciliation-report.test.ts index f61af00bfe5..730729d996b 100644 --- a/server/src/__tests__/ownership-reconciliation-report.test.ts +++ b/server/src/__tests__/ownership-reconciliation-report.test.ts @@ -143,7 +143,8 @@ describe("report-only ownership reconciliation", () => { expect(names.length).toBeGreaterThanOrEqual(40); expect(OWNABLE_KINDS.filter(kind => kind.settle !== null).map(kind => kind.id)) .toEqual(["native_accepted_result_lease", "native_finalizer_lease", "native_cleanup_lease", "sandbox_creation_intent", - "environment_run_lease_active", "environment_teardown_claim", "native_workspace_finalize_attempt", "wake_request_claimed", "legacy_controller_lease"]); + "environment_run_lease_active", "environment_teardown_claim", "environment_retained_orphan_sandbox", + "native_workspace_finalize_attempt", "wake_request_claimed", "legacy_controller_lease"]); expect(OWNABLE_KINDS.filter(kind => !kind.listInProgress).every(kind => kind.coverageGap)).toBe(true); }); diff --git a/server/src/services/chat-channels/task-retry-authority.ts b/server/src/services/chat-channels/task-retry-authority.ts index d17bcc4fc04..c2018c6e5ff 100644 --- a/server/src/services/chat-channels/task-retry-authority.ts +++ b/server/src/services/chat-channels/task-retry-authority.ts @@ -3,7 +3,7 @@ import { agentWakeupRequests, environmentLeases, heartbeatRuns, issueRecoveryAct import { buildExecutionContinuation } from "../execution-continuation.js"; import { readTaskRetryIntent } from "../task-retry.js"; import { hasNativeLocalProcessStop, hasHistoricalSuspendedNativeSession, recordedLocalExecutionStopped } from "../native-local-process-stop.js"; -import { hasRemoteTerminationReceipt } from "../remote-execution-termination.js"; +import { hasRecoveredCleanupStopReceipt, hasRemoteTerminationReceipt } from "../remote-execution-termination.js"; import { adapterExecutionControls } from "../adapter-execution-control.js"; import { getExecutionBlocker, hasPendingRunCleanup } from "../execution-blocker.js"; import { readSystemRecovery } from "../system-recovery-state.js"; @@ -74,7 +74,11 @@ async function taskChatRetryExecutionContained(db: Db, run: typeof heartbeatRuns const leases = await db.select().from(environmentLeases).where(and( eq(environmentLeases.companyId, run.companyId), eq(environmentLeases.heartbeatRunId, run.id), )).for("share", { noWait: true }); - if (leases.some(lease => lease.provider !== "local")) return leases.every(hasRemoteTerminationReceipt); + // A verified stop counts, including one whose destroy is queued as stopped + // cleanup (a recovered or retained orphan sandbox awaiting teardown). + if (leases.some(lease => lease.provider !== "local")) { + return leases.every(lease => hasRemoteTerminationReceipt(lease) || hasRecoveredCleanupStopReceipt(lease)); + } return await recordedLocalExecutionStopped(db, run) || await hasNativeLocalProcessStop(db, run.companyId, run.id) || await hasHistoricalSuspendedNativeSession(db, run); diff --git a/server/src/services/environment-cleanup-state.ts b/server/src/services/environment-cleanup-state.ts index c281e19360c..a2c9e86dd7d 100644 --- a/server/src/services/environment-cleanup-state.ts +++ b/server/src/services/environment-cleanup-state.ts @@ -55,12 +55,13 @@ export function recoverableSandboxCreationCondition() { )); } -/** A run lease the ownership settle moved to cleanup, before its first +/** A run lease the ownership settle moved to cleanup (an orphaned active + * lease, or a retained orphan sandbox nothing references any more), before its first * cleanup attempt. Its holder is proven gone, so the first stop or teardown is * due now regardless of how recently the lease was used; a failed attempt * counts and the normal retry backoff applies from then on. */ export function recoveredOrphanFirstAttemptSql() { - return sql`(${environmentLeases.failureReason} = 'orphaned_active_lease_recovered' + return sql`(${environmentLeases.failureReason} in ('orphaned_active_lease_recovered', 'retained_orphan_sandbox_released') or ${environmentLeases.metadata}->'sandboxStopAndRetain'->>'recoveredOrphanedRunLease' = 'true') and coalesce(${environmentLeases.metadata}->>'pendingCleanupRetryAttempts', '0') = '0' and coalesce(${environmentLeases.metadata}->>'pendingCleanupInFlight', 'false') <> 'true'`; diff --git a/server/src/services/environment-retained-orphan-settlement.ts b/server/src/services/environment-retained-orphan-settlement.ts new file mode 100644 index 00000000000..285952b4c68 --- /dev/null +++ b/server/src/services/environment-retained-orphan-settlement.ts @@ -0,0 +1,197 @@ +import { and, eq, inArray, ne, notInArray, sql } from "drizzle-orm"; +import { environmentLeases, heartbeatRuns, issueRecoveryActions, type Db } from "@paperclipai/db"; +import { writeEnvironmentCleanup } from "./environment-cleanup-notifications.js"; +import { nativeWorkspaceExportPendingForLease } from "./native-runtime/native-workspace-finalization-state.js"; +import type { OwnerProof } from "./owner-proof.js"; +import type { InProgressRecord, SettleOutcome } from "./recovery/ownership-registry.js"; +import { hasRemoteTerminationReceipt } from "./remote-execution-termination.js"; +import { ORPHANED_SANDBOX_RECOVERY_KEY } from "./sandbox-stop-and-retain.js"; + +/** Failure reason on a retained orphan sandbox released for destruction. */ +export const RETAINED_ORPHAN_RELEASED_REASON = "retained_orphan_sandbox_released"; + +const TERMINAL_RUN_STATUSES = ["succeeded", "failed", "cancelled", "timed_out", "interrupted"]; +/** Runs on the same issue that may still continue from this sandbox. */ +const OPEN_RUN_STATUSES = ["queued", "scheduled_retry", "running"]; + +type LeaseRow = typeof environmentLeases.$inferSelect; + +/** A retained orphan: the ownership settle stopped and kept its sandbox, and + * the stop is confirmed. */ +export function retainedOrphanSandboxCondition() { + return and(eq(environmentLeases.status, "released"), eq(environmentLeases.cleanupStatus, "success"), + // An ephemeral lease only: a reusable lease stays for warm reuse, and a + // retain-on-failure lease belongs to workspace restore recovery. + eq(environmentLeases.leasePolicy, "ephemeral"), + sql`${environmentLeases.heartbeatRunId} is not null`, sql`${environmentLeases.provider} is not null`, + sql`${environmentLeases.providerLeaseId} is not null`, + sql`${environmentLeases.metadata} ? ${ORPHANED_SANDBOX_RECOVERY_KEY}`, + sql`${environmentLeases.metadata} ? 'sandboxStopAndRetainReceipt'`); +} + +/** SQL mirror of the "held only while the stop is a usable source" checks in + * `heldBy`, so a held orphan is not listed (and not polled) until its last + * reference clears or its retention window ends. The settle re-checks all of + * them, plus the pending native export, under the row lock. */ +function softlyHeldSql(now?: Date) { + const at = now ? sql`${now.toISOString()}::timestamptz` : sql`now()`; + const issue = sql`coalesce(run.native_issue_id, run.issue_id, ${environmentLeases.issueId})`; + return sql`(${environmentLeases.expiresAt} is null or ${environmentLeases.expiresAt} > ${at}) and ( + exists (select 1 from issue_recovery_actions action, heartbeat_runs run + where run.id = ${environmentLeases.heartbeatRunId} and action.company_id = ${environmentLeases.companyId} + and action.source_issue_id = ${issue} and action.status not in ('resolved', 'cancelled')) + or exists (select 1 from heartbeat_runs run, heartbeat_runs other + where run.id = ${environmentLeases.heartbeatRunId} and other.company_id = ${environmentLeases.companyId} + and other.id <> run.id and other.status in ('queued', 'scheduled_retry', 'running') + and (other.issue_id = ${issue} or other.native_issue_id = ${issue})))`; +} + +function alwaysHeldSql() { + return sql`(${environmentLeases.metadata} ?| array['nativeWorkspaceExportResume', 'sandboxStopAndRetain', + 'workspaceRestoreRecovery', 'workspaceRecoveryCleanup', 'runnerTaskSourceConsumer'] + or coalesce(${environmentLeases.metadata}->>'pendingCleanupInFlight', 'false') = 'true' + or coalesce(${environmentLeases.metadata}#>>'{legacyShutdownWorkspaceSettlement,state}' = 'pending', false) + or exists (select 1 from heartbeat_runs run where run.id = ${environmentLeases.heartbeatRunId} + and run.status not in ('succeeded', 'failed', 'cancelled', 'timed_out', 'interrupted')) + -- A prepared native workspace bound to this exact sandbox, for a run with + -- workspace access. This is deliberately no wider than the settle's own + -- export check: an orphan the settle would release must never be hidden + -- here. Narrower cases (a prepared remote workspace, an observed failure) + -- are listed and held by the settle until their export settles. + -- The descriptor must also be one the settle's reader accepts + -- (readNativeWorkspaceSyncReference); an invalid one holds nothing. + or exists (select 1 from heartbeat_runs run where run.id = ${environmentLeases.heartbeatRunId} + and run.runtime_mode = 'native' + and run.runner_profile_json#>>'{nativeWorkspaceSync,schema}' in + ('paperclip.native-workspace-sync/v1', 'paperclip.native-workspace-sync/v2', 'paperclip.native-workspace-sync/v3') + and run.runner_profile_json#>>'{nativeWorkspaceSync,state}' = 'prepared' + and coalesce(run.runner_profile_json#>>'{nativeWorkspaceSync,descriptorSha256}' ~ '^[0-9a-f]{64}$', false) + and coalesce(run.runner_profile_json#>>'{nativeWorkspaceSync,baselineSha256}' ~ '^[0-9a-f]{64}$', false) + and coalesce(jsonb_typeof(run.runner_profile_json#>'{nativeWorkspaceSync,finalHostSha256}'), 'null') = 'null' + and jsonb_typeof(run.runner_profile_json#>'{nativeWorkspaceSync,workspaceId}') = 'string' + and jsonb_typeof(run.runner_profile_json#>'{nativeWorkspaceSync,remoteCwd}') = 'string' + and coalesce(run.runner_profile_json#>>'{nativeWorkspaceSync,resourceDisposition}', 'destroy') + in ('keep_running', 'stop_and_retain', 'destroy') + and run.runner_profile_json#>>'{nativeWorkspaceSync,leaseId}' = ${environmentLeases.id}::text + and run.runner_profile_json#>>'{nativeWorkspaceSync,providerLeaseId}' = ${environmentLeases.providerLeaseId} + and coalesce(run.runner_profile_json#>>'{nativeExecutionInput,workspace,access}', '') <> 'none') + or exists (select 1 from environment_leases other where other.id <> ${environmentLeases.id} + and other.provider = ${environmentLeases.provider} and other.provider_lease_id = ${environmentLeases.providerLeaseId} + and other.status in ('active', 'retained', 'pending_cleanup')))`; +} + +/** Retained orphans that nothing appears to reference now. */ +// The listing is built once, so it reads the database clock. +export function releasableRetainedOrphanCondition(now?: Date) { + return and(retainedOrphanSandboxCondition(), sql`not ${alwaysHeldSql()}`, sql`not (${softlyHeldSql(now)})`); +} + +/** When the next retained orphan's retention window ends while only soft + * references hold it; the reconciler wakes then to release it. */ +export async function nextRetainedOrphanDueAt(db: Db, now: Date): Promise { + const [row] = await db.select({ due: sql`min(${environmentLeases.expiresAt})` }).from(environmentLeases) + .where(and(retainedOrphanSandboxCondition(), sql`not ${alwaysHeldSql()}`, sql`${softlyHeldSql(now)}`, + sql`${environmentLeases.expiresAt} is not null`)); + return row?.due ? new Date(row.due) : null; +} + +/** + * Settle of the `environment_retained_orphan_sandbox` kind (plan Phase 2). + * + * The orphaned-run-lease settle stops a dead holder's sandbox and keeps it, + * because recovery may still need its files. This settle destroys that + * retained orphan once no obligation references it, so it is never kept + * longer than the environment's own retention allows. + * + * It decides from database facts only, inside one transaction that locks the + * lease row: + * - the run is terminal; + * - always held: an active operation on the sandbox (a native export resume, + * a stop-and-retain intent, an in-flight cleanup claim, a workspace restore + * recovery, a pending legacy shutdown fence, a bound source-recovery + * consumer) or another live lease on the same provider resource; + * - always held: a pending native workspace export or preservation fence; + * - held while the stop receipt is still a usable workspace source (the + * lease's provider-attested `expiresAt` has not passed): an open recovery + * action on the issue, or a queued, scheduled or running run on the issue. Past that + * retention window no recovery path accepts the stopped sandbox as a source + * (`hasConfirmedSandboxStopAndRetain`), so these no longer hold it. + * + * Then it hands the sandbox to the pending-cleanup sweep for destruction, + * with a compare-and-swap on the exact row it judged. It never calls a + * provider itself and never runs on the turn path. + */ +export async function settleRetainedOrphanSandbox(db: Db, record: InProgressRecord, + context: { now: Date; proof: OwnerProof }): Promise { + if (!record.runId) return { settled: false, reason: "lease_has_no_run" }; + const runId = record.runId; + return db.transaction(async transaction => { + const tx = transaction as unknown as Db; + const [lease] = await tx.select().from(environmentLeases).where(and(eq(environmentLeases.id, record.recordId), + eq(environmentLeases.heartbeatRunId, runId), eq(environmentLeases.status, "released"), + eq(environmentLeases.cleanupStatus, "success"))).for("update"); + if (!lease || !hasRemoteTerminationReceipt(lease) || lease.leasePolicy !== "ephemeral" || !lease.metadata?.[ORPHANED_SANDBOX_RECOVERY_KEY] + || !lease.provider || !lease.providerLeaseId) { + return { settled: false, reason: "no_longer_a_retained_orphan" }; + } + const held = await heldBy(tx, lease, runId, context.now); + if (held) return { settled: false, reason: held }; + const now = context.now; + // The destroy is queued through the recognized stopped-cleanup contract + // (`workspaceRecoveryCleanup` with the verified stop receipt), so admission + // and Retry keep treating the sandbox as stopped while it waits. + const released = await writeEnvironmentCleanup(tx, write => write.update(environmentLeases).set({ + status: "pending_cleanup", cleanupStatus: "failed", + failureReason: RETAINED_ORPHAN_RELEASED_REASON, updatedAt: now, + metadata: sql`(${environmentLeases.metadata} - 'remoteExecutionTermination') || ${JSON.stringify({ + workspaceRecoveryCleanup: { runId, stopReceipt: lease.metadata?.remoteExecutionTermination, + reason: RETAINED_ORPHAN_RELEASED_REASON, releasedAt: now.toISOString(), ownerProof: context.proof.reason }, + pendingCleanupInFlight: false, pendingCleanupRetryAfterMs: 0, pendingCleanupRetryAttempts: 0, + pendingCleanupLeaseExpiresAtMs: 0 })}::jsonb`, + }).where(and(eq(environmentLeases.id, lease.id), eq(environmentLeases.heartbeatRunId, runId), + eq(environmentLeases.status, "released"), eq(environmentLeases.cleanupStatus, "success"), + sql`${environmentLeases.metadata} = ${JSON.stringify(lease.metadata)}::jsonb`)) + .returning({ id: environmentLeases.id }), { companyId: lease.companyId, runId, leaseId: lease.id }); + return released.length ? { settled: true, reason: context.proof.reason } : { settled: false, reason: "lease_changed_since_proof" }; + }); +} + +async function heldBy(tx: Db, lease: LeaseRow, runId: string, now: Date): Promise { + const metadata = lease.metadata ?? {}; + const [run] = await tx.select({ status: heartbeatRuns.status, issueId: heartbeatRuns.issueId, + nativeIssueId: heartbeatRuns.nativeIssueId }).from(heartbeatRuns).where(eq(heartbeatRuns.id, runId)); + if (run && !TERMINAL_RUN_STATUSES.includes(run.status)) return "run_active"; + + // Always held: an operation on this sandbox is still in progress. + if (metadata.nativeWorkspaceExportResume !== undefined) return "native_export_resume"; + if (metadata.sandboxStopAndRetain !== undefined) return "stop_and_retain_intent"; + if (metadata.pendingCleanupInFlight === true) return "cleanup_claim_in_flight"; + if (metadata.workspaceRestoreRecovery !== undefined) return "workspace_restore_recovery"; + if (metadata.workspaceRecoveryCleanup !== undefined) return "workspace_recovery_cleanup"; + if (metadata.runnerTaskSourceConsumer !== undefined) return "source_recovery_consumer"; + const shutdown = metadata.legacyShutdownWorkspaceSettlement as Record | undefined; + if (shutdown?.state === "pending") return "legacy_shutdown_fence"; + const [other] = await tx.select({ id: environmentLeases.id }).from(environmentLeases).where(and( + ne(environmentLeases.id, lease.id), eq(environmentLeases.provider, lease.provider!), + eq(environmentLeases.providerLeaseId, lease.providerLeaseId!), + inArray(environmentLeases.status, ["active", "retained", "pending_cleanup"]))).limit(1); + if (other) return "provider_resource_shared"; + + // A pending native export is an active preservation operation; the cleanup + // sweep would defer the destroy anyway, so it always holds the sandbox. + if (await nativeWorkspaceExportPendingForLease(tx, lease)) return "workspace_preservation_pending"; + // Held only while the stopped sandbox is still a usable workspace source. + if (lease.expiresAt && lease.expiresAt <= now) return null; + const issueId = run?.nativeIssueId ?? run?.issueId ?? lease.issueId; + if (!issueId) return null; + const [action] = await tx.select({ id: issueRecoveryActions.id }).from(issueRecoveryActions).where(and( + eq(issueRecoveryActions.companyId, lease.companyId), eq(issueRecoveryActions.sourceIssueId, issueId), + notInArray(issueRecoveryActions.status, ["resolved", "cancelled"]))).limit(1); + if (action) return "recovery_action_open"; + const [next] = await tx.select({ id: heartbeatRuns.id }).from(heartbeatRuns).where(and( + eq(heartbeatRuns.companyId, lease.companyId), ne(heartbeatRuns.id, runId), + inArray(heartbeatRuns.status, OPEN_RUN_STATUSES), + sql`(${heartbeatRuns.issueId} = ${issueId} or ${heartbeatRuns.nativeIssueId} = ${issueId})`)).limit(1); + if (next) return "issue_run_pending"; + return null; +} diff --git a/server/src/services/recovery/ownership-invariant-report.ts b/server/src/services/recovery/ownership-invariant-report.ts index ff314c14c41..7650e5ffdda 100644 --- a/server/src/services/recovery/ownership-invariant-report.ts +++ b/server/src/services/recovery/ownership-invariant-report.ts @@ -554,6 +554,19 @@ export async function runOwnershipReconciliationPass(db: Db, options: { logger.warn({ err: error }, "ownership invariant check failed; the report continues"); } } + // A settling kind can become settleable at a known time without a signal + // (a retention window ending); wake then. + for (const kind of kinds) { + if (!settleRecords || !kind.settle || !kind.nextDueAt) continue; + try { + const due = await kind.nextDueAt(db, now); + const current = nextDueAt as Date | null; + if (due && (!current || due < current)) nextDueAt = due; + } catch (error) { + retryNeeded = true; + logger.warn({ err: error, kind: kind.id }, "ownership due-time read failed; a later pass retries"); + } + } // Only a first proof needs a second pass; a record already reported for I2 // is not a reason to keep scanning. diff --git a/server/src/services/recovery/ownership-registry.ts b/server/src/services/recovery/ownership-registry.ts index 2fde5d7d902..6bc510d34c4 100644 --- a/server/src/services/recovery/ownership-registry.ts +++ b/server/src/services/recovery/ownership-registry.ts @@ -50,6 +50,11 @@ import { import { settleEnvironmentRunLease } from "../environment-run-lease-settlement.js"; import { settleEnvironmentTeardownClaim } from "../environment-teardown-claim-settlement.js"; import { settleSandboxCreationIntent } from "../sandbox-creation-intent-settlement.js"; +import { + nextRetainedOrphanDueAt, + releasableRetainedOrphanCondition, + settleRetainedOrphanSandbox, +} from "../environment-retained-orphan-settlement.js"; /** * Registry of ownable record kinds (plan section C). @@ -123,6 +128,9 @@ export type OwnableKind = { settle: SettleRecord | null; /** Optional operator action for an unprovable owner of a settling kind. */ onUnprovable?: UnprovableAction; + /** When a record of this kind next becomes settleable without a signal + * (a retention window ending). The reconciler wakes then. */ + nextDueAt?: (db: Db, now: Date) => Promise; /** Plan invariants this kind is checked against (1–8). */ invariantChecks: readonly number[]; /** Why the kind cannot be listed, when `listInProgress` is null. */ @@ -672,6 +680,16 @@ export const OWNABLE_KINDS: readonly OwnableKind[] = [ row => runOwner(row.heartbeatRunId)), leaseKind("environment_retained_hold", "Retained / reusable hold", "workspace_preservation", eq(environmentLeases.status, "retained"), () => ({ kind: "none" }), [1, 7]), + { + // Shared path since Phase 2: a sandbox the orphaned-run-lease settle + // stopped and kept is destroyed once nothing references it. Only + // releasable orphans are listed, so a held one is never polled; its + // retention window end is a due time. + ...leaseKind("environment_retained_orphan_sandbox", "Retained orphan sandbox", "workspace_preservation", + releasableRetainedOrphanCondition(), row => runOwner(row.heartbeatRunId), [1]), + settle: settleRetainedOrphanSandbox, + nextDueAt: nextRetainedOrphanDueAt, + }, 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)), diff --git a/server/src/services/sandbox-stop-and-retain.ts b/server/src/services/sandbox-stop-and-retain.ts index c17c6a10965..a1ccefc1d6d 100644 --- a/server/src/services/sandbox-stop-and-retain.ts +++ b/server/src/services/sandbox-stop-and-retain.ts @@ -11,6 +11,10 @@ import type { SandboxAbsenceProof } from "./sandbox-absence-proof.js"; type Lease = Pick; export const SANDBOX_STOP_AND_RETAIN_KEY = "sandboxStopAndRetain"; +/** Top-level marker an ownership settle leaves on a lease whose sandbox it + * stopped and kept. It outlives the stop intent, so the retained orphan can + * be destroyed once nothing references it. */ +export const ORPHANED_SANDBOX_RECOVERY_KEY = "orphanedRunLeaseRecovery"; /** A successful stop is not by itself proof that the allocation was retained. * Require the dedicated stop-only dispatch receipt and its independent stop @@ -75,8 +79,9 @@ export function readStopOnlyCleanup(lease: Lease) { /** Persist before provider dispatch so an older worker or a restart cannot turn * an explicit stop into ordinary destructive release. Invalid pins stay pending. - * `recovery` marks an ownership settle on the intent; the pending-cleanup - * sweep dispatches such a stop's first attempt without its retry backoff. */ + * `recovery` marks an ownership settle on the intent and on the lease + * (`ORPHANED_SANDBOX_RECOVERY_KEY`); the pending-cleanup sweep dispatches such + * a stop's first attempt without its retry backoff. */ export async function prepareSandboxStopAndRetain(db: Db, lease: Lease, recovery?: Record) { return prepareStopAndRetain(db, lease, "active", recovery); } @@ -158,6 +163,8 @@ async function prepareStopAndRetain(db: Db, lease: Lease, fromStatus: "active" | status: "pending_cleanup", cleanupStatus: "failed", failureReason: "sandbox_stop_pending", releasedAt: now, updatedAt: now, metadata: sql`(coalesce(${environmentLeases.metadata}, '{}'::jsonb) - 'remoteExecutionTermination' - 'sandboxStopAndRetainReceipt') || ${JSON.stringify({ [SANDBOX_STOP_AND_RETAIN_KEY]: intent, pendingCleanupAttemptId: requestId, + ...(fromStatus === "active" && Object.keys(recovery).length + ? { [ORPHANED_SANDBOX_RECOVERY_KEY]: { ...recovery, runId: lease.heartbeatRunId, recoveredAt: now.toISOString() } } : {}), pendingCleanupInFlight: false, pendingCleanupLeaseExpiresAtMs: 0, pendingCleanupRetryAfterMs: 0, pendingCleanupRetryAttempts: 0, pendingCleanupRetryCapWarned: false, })}::jsonb`, From 4638e5ea30dc08a9dbd18e9aba50b5a32c0c5e3f Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Sun, 8 Nov 2026 01:56:56 -0600 Subject: [PATCH 4/9] fix(issues): make the queued-comment discard denial final and link the task (#19990) 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 > - People queue follow-up messages on a task while its agent works, and only the author can discard a queued message > - When a person asks a chat agent to cancel their queued message, the agent tries the discard API and gets a 403 > - The 403 text does not say that the denial is final, so the agent searches for a board or admin route before it answers > - In a slow model this search takes many minutes, and the queued message can be delivered before the person gets the answer > - This pull request makes both denials say that only the author can discard the message, where to do it (with the task link), and that no override exists > - The benefit is that the agent can relay the human route at once ## Linked Issues or Issue Description **What happened** A person asked the chat agent to cancel a contact-section request that they had queued on a worker's task. The agent called `DELETE /api/issues/:id/queued-comments/:commentId` and got 403 "Board access required". It then called `DELETE /api/issues/:id/comments/:commentId` and got 403 "Only the comment author can cancel queued comments". In two eval runs the agent then continued to search the API for an override. It answered after 10 minutes and 24 minutes. In the second run, the worker consumed the queued message before the answer arrived. **Expected behavior** The denial tells the agent that the action is author-only and final, and gives the task link where the author can discard the message. The agent can then answer the person immediately. **Steps to reproduce** 1. Start an agent run on a task. Queue a comment on that task as a board user. 2. As a different agent, call `DELETE /api/issues//queued-comments/` with the current `queueId` and `revision`. 3. See 403 "Board access required", with no next step and no statement that the denial is final. Refs the chat eval case `steering.discard-queued-message`. ## What Changed - `DELETE /issues/:id/queued-comments/:commentId`: an agent actor now gets a 403 that says only the author can discard the queued comment from its menu on the linked task, and that no board, admin or agent override exists. The task must be in the agent's company. Otherwise the route returns 404 as before. - `DELETE /issues/:id/comments/:commentId` (queued cancel by a non-author): uses the same message. - Tests: the agent denial on the queued-comments route, the cross-company 404 without task details, and the updated non-author cancel message. ## Verification - `cd server && pnpm exec vitest run src/__tests__/issue-comment-cancel-routes.test.ts` passes (9 tests). - I will run the chat eval `steering.discard-queued-message` with this change cherry-picked, and record the result in a comment. ## Risks - Low risk. Only the 403 text changes, and agents now get the 403 after the company access check instead of before it. Board behavior and the author-only rule do not change. - The UI shows this text if a non-author person tries to cancel a queued comment. The text stays correct for that person. ## Model Used - Anthropic Claude Opus 5.5 (`claude-opus-5-5`), agentic coding with tool use (shell, file editing) 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 --- .../issue-comment-cancel-routes.test.ts | 87 ++++++++++++++++++- server/src/routes/issues.ts | 36 +++++++- 2 files changed, 121 insertions(+), 2 deletions(-) diff --git a/server/src/__tests__/issue-comment-cancel-routes.test.ts b/server/src/__tests__/issue-comment-cancel-routes.test.ts index 4d40e68f317..410f697e7b5 100644 --- a/server/src/__tests__/issue-comment-cancel-routes.test.ts +++ b/server/src/__tests__/issue-comment-cancel-routes.test.ts @@ -181,6 +181,9 @@ function makeIssue() { }; } +const finalDenial = "Only the author of a queued comment can discard it, from that queued message's menu on [PAP-1353](/PAP/issues/PAP-1353). " + + "This denial is final: no board, admin or agent override exists, so do not try other routes. Tell the author to discard it there."; + function makeComment(overrides: Record = {}) { return { id: "comment-1", @@ -225,6 +228,9 @@ describe("issue comment cancel routes", () => { from: vi.fn(() => ({ where: vi.fn(() => ({ orderBy: vi.fn(async () => mockAuthoritativeQueueWakes), + // Agent mutation guards read a single row (no task-watchdog run here). + then: (resolve: (rows: unknown[]) => unknown, reject?: (error: unknown) => unknown) => + Promise.resolve([]).then(resolve, reject), })), })), })), @@ -260,6 +266,9 @@ describe("issue comment cancel routes", () => { await options?.afterTombstone?.(deleted, "tx"); return deleted; }); + mockAccessService.decide.mockImplementation(async ({ action }: { action: string }) => ({ + action, allowed: action === "issue:read", reason: "allow_default", explanation: "Fixture task is readable", + })); mockAccessService.canUser.mockResolvedValue(false); mockAccessService.hasPermission.mockResolvedValue(false); mockFeedbackService.listIssueVotesForUser.mockResolvedValue([]); @@ -366,10 +375,86 @@ describe("issue comment cancel routes", () => { .delete("/api/issues/11111111-1111-4111-8111-111111111111/comments/comment-1?mode=cancel"); expect(res.status).toBe(403); - expect(res.body.error).toBe("Only the comment author can cancel queued comments"); + expect(res.body.error).toBe(finalDenial); expect(mockIssueService.removeComment).not.toHaveBeenCalled(); }); + it("tells an agent that discarding a person's queued comment is author-only and final, with the task link", async () => { + const agent = { type: "agent", agentId: "22222222-2222-4222-8222-222222222222", companyId: "company-1", runId: "run-1", source: "agent_key" }; + const res = await request(await installActor(createApp(), agent)) + .delete("/api/issues/11111111-1111-4111-8111-111111111111/queued-comments/comment-1") + .send({ queueId: "queue-1", revision: "revision-1" }); + + expect(res.status).toBe(403); + expect(res.body.error).toBe(finalDenial); + expect(res.body.error).not.toBe("Board access required"); + expect(mockIssueService.removeComment).not.toHaveBeenCalled(); + }); + + it("returns 404 without task details when the agent cannot read the private task", async () => { + mockAccessService.decide.mockImplementation(async ({ action }: { action: string }) => ({ + action, allowed: false, reason: "deny_issue_private", explanation: "Private task", + })); + const agent = { type: "agent", agentId: "33333333-3333-4333-8333-333333333333", companyId: "company-1", runId: "run-9", source: "agent_key" }; + const res = await request(await installActor(createApp(), agent)) + .delete("/api/issues/11111111-1111-4111-8111-111111111111/queued-comments/comment-1") + .send({ queueId: "queue-1", revision: "revision-1" }); + + expect(res.status).toBe(404); + expect(JSON.stringify(res.body)).not.toContain("PAP-1353"); + }); + + it("points an author agent at the cancel route it can use instead of claiming no route exists", async () => { + mockIssueService.getComment.mockResolvedValue(makeComment({ authorUserId: null, authorAgentId: "33333333-3333-4333-8333-333333333333" })); + const agent = { type: "agent", agentId: "33333333-3333-4333-8333-333333333333", companyId: "company-1", runId: "run-9", source: "agent_key" }; + const res = await request(await installActor(createApp(), agent)) + .delete("/api/issues/11111111-1111-4111-8111-111111111111/queued-comments/comment-1") + .send({ queueId: "queue-1", revision: "revision-1" }); + + expect(res.status).toBe(403); + expect(res.body.error).toContain("/api/issues/11111111-1111-4111-8111-111111111111/comments/comment-1?mode=cancel"); + expect(res.body.error).not.toContain("final"); + }); + + it("lets an author agent discard its own queued comment through the cancel route", async () => { + // Another agent queued this comment behind the assignee's active run; it can read and write the task. + mockAccessService.decide.mockImplementation(async ({ action }: { action: string }) => ({ + action, allowed: true, reason: "allow_default", explanation: "Assignee task", + })); + mockIssueService.getComment.mockResolvedValue(makeComment({ authorUserId: null, authorAgentId: "33333333-3333-4333-8333-333333333333" })); + mockIssueService.removeComment.mockResolvedValue(makeComment({ authorUserId: null, authorAgentId: "33333333-3333-4333-8333-333333333333" })); + const agent = { type: "agent", agentId: "33333333-3333-4333-8333-333333333333", companyId: "company-1", runId: "run-9", source: "agent_key" }; + const res = await request(await installActor(createApp(), agent)) + .delete("/api/issues/11111111-1111-4111-8111-111111111111/comments/comment-1?mode=cancel"); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(mockIssueService.removeComment).toHaveBeenCalledWith("comment-1"); + }); + + it("returns 404 instead of the author-only denial when a non-author cannot read the task", async () => { + mockIssueService.getComment.mockResolvedValue(makeComment({ authorUserId: "someone-else" })); + mockAccessService.decide.mockImplementation(async ({ action }: { action: string }) => ({ + action, allowed: false, reason: "deny_issue_private", explanation: "Private task", + })); + const res = await request(await installActor(createApp())) + .delete("/api/issues/11111111-1111-4111-8111-111111111111/comments/comment-1?mode=cancel"); + + expect(res.status).not.toBe(403); + expect(JSON.stringify(res.body)).not.toContain("PAP-1353"); + expect(mockIssueService.removeComment).not.toHaveBeenCalled(); + }); + + it("does not reveal another company's task through the agent queued-comment denial", async () => { + mockIssueService.getById.mockResolvedValue({ ...makeIssue(), companyId: "company-2" }); + const agent = { type: "agent", agentId: "22222222-2222-4222-8222-222222222222", companyId: "company-1", runId: "run-1", source: "agent_key" }; + const res = await request(await installActor(createApp(), agent)) + .delete("/api/issues/11111111-1111-4111-8111-111111111111/queued-comments/comment-1") + .send({ queueId: "queue-1", revision: "revision-1" }); + + expect(res.status).toBe(404); + expect(JSON.stringify(res.body)).not.toContain("PAP-1353"); + }); + it("deletes a normal authored comment as a tombstone without returning the original body", async () => { mockIssueService.getComment.mockResolvedValue( makeComment({ diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 9d7fee46aec..8cb780faa69 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -416,6 +416,20 @@ const MAX_ISSUE_COMMENT_LIMIT = 500; const updateIssueRouteSchema = updateIssueSchema.extend({ interrupt: z.boolean().optional(), }); +/** + * Sent only to a caller that did not write the queued comment: its author is the + * only one who can discard it, and no board, admin or agent override exists. Say so with the task link, so an agent that tried can + * relay the human route at once instead of searching for another endpoint. + */ +function queuedCommentAuthorOnlyDenial(issue: { identifier?: string | null }) { + const identifier = issue.identifier ?? null; + const prefix = identifier && /^[A-Z][A-Z0-9]*-\d+$/.test(identifier) ? identifier.split("-")[0] : null; + const where = identifier && prefix ? `[${identifier}](/${prefix}/issues/${identifier})` : "its task"; + return `Only the author of a queued comment can discard it, from that queued message's menu on ${where}. ` + + "This denial is final: no board, admin or agent override exists, so do not try other routes. " + + `Tell the author to discard it there.`; +} + const queuedCommentMutationTargetSchema = z.object({ queueId: z.string().min(1), revision: z.string().min(1), @@ -17565,6 +17579,25 @@ export function issueRoutes( "/issues/:id/queued-comments/:commentId", validate(queuedCommentMutationTargetSchema), async (req, res) => { + if (req.actor.type === "agent") { + // Agents never use this board route. Check visibility first so a denial + // never confirms a task the agent cannot read. + const issue = await getAccessibleResource(req, res, svc.getById(req.params.id as string), "Issue not found"); + if (!issue) return; + if (!(await assertIssueReadAllowed(req, res, issue))) return; + const comment = await svc.getComment(req.params.commentId as string); + if (!comment || comment.issueId !== issue.id) { + res.status(404).json({ error: "Comment not found" }); + return; + } + if (comment.authorAgentId && comment.authorAgentId === req.actor.agentId) { + throw forbidden( + "Agents discard their own queued comments with " + + `DELETE /api/issues/${issue.id}/comments/${comment.id}?mode=cancel, not this board route.`, + ); + } + throw forbidden(queuedCommentAuthorOnlyDenial(issue)); + } assertBoard(req); if (!req.actor.userId) throw forbidden("Board user context required"); const id = req.params.id as string; @@ -18853,8 +18886,9 @@ export function issueRoutes( : false; if (deleteMode === "cancel" || pendingQueueWake || isLegacyQueuedComment) { if (!actorOwnsComment) { + if (!(await assertIssueReadAllowed(req, res, issue))) return; res.status(403).json({ - error: "Only the comment author can cancel queued comments", + error: queuedCommentAuthorOnlyDenial(issue), }); return; } From 8475826d2d5d81baee5f27e349e979cd706aeb3a Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Sun, 8 Nov 2026 02:14:45 -0600 Subject: [PATCH 5/9] fix(wake-queue): deliver user comments whose wake a restart lost or a stale continuation dropped (#19992) 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 > - A user's follow-up comment on a task must reach the agent exactly once, through restarts (invariant 4) > - The final unmasked local chaos storm (96 tasks, 31 restarts) passed 94 tasks. The 2 failures were both lost user comments > - In t69, a restart killed the server between saving the comment and writing its wake. In t72, a comment's own run was cancelled as stale, because the earlier run completed the task first without reading the comment > - This pull request adds a reconciler rule that gives a lost comment its wake, and saves the comment before a stale cancellation > - The benefit is that a user's message is never silently dropped when a restart or a completion race hits at the wrong moment ## Linked Issues or Issue Description Refs #19956 (work-intent outbox), #19944, #19831 (plan, invariant 4). **What happened?** In chaos wave `w-20261108T055444-s9401` (local, unmasked, codex and claude, 30-min K13 storm, master `46bf913fb9`), 94 of 96 tasks passed. Two failed: - **t69, R1 `rested_not_done`.** The user's comment was saved at 06:14:59.358 and reopened the task to `todo`. A K1 SIGKILL fired 28 ms later, before the wake was written. No wake row exists. Recovery skips a `todo` task whose last run succeeded, so the task sat with no run until the wave ended. - **t72, R4 `concurrent_runs`.** A comment posted during a run queued its own run. That run was claimed at 06:16:04.874. The first run's completion marked the task `done` 2 ms later. The comment's run was then cancelled with `continuation_task_ownership_changed` before dispatch, and the comment was lost. **Expected behavior** Each accepted user comment is delivered once, unless the task is cancelled or reassigned, or a person closes it. **Steps to reproduce** `node cli/node_modules/tsx/dist/cli.mjs tests/restart-chaos/main.ts --replay w-20261108T055444-s9401/t69` (or `.../t72`). ## What Changed - `recovery/ownership-invariant-report.ts`: a new reconciler-owned I4 rule, `user_comment_without_wake`. It finds a user comment that meets all of these conditions: - it is newer than every run of the task; - the task is assigned, is not a conversation, has no live run or lock, and has at least one earlier run; - no wake request carries the comment. A comment younger than 30 s is not reported yet, because it may still get its normal wake. Its end-of-grace time is returned as `nextDueAt`. - `native-runtime/native-finalization-reconciler.ts`: the backstop step is now exported as `applyOwnershipReportBackstops`. For each such comment it saves the missing outbox receipt (`executionWait: wake_lost`) and promotes it. - `work-intent-outbox.ts`: new `saveUndeliveredUserComments`, which the reconciler rule and the stale-run path both use. It saves only live user comments that no completed run received. It writes nothing for a task that is reassigned, is a conversation, or has a status the caller does not allow. - `heartbeat/queue.ts`: the saved-receipt pass that already sends pause-saved messages also sends `wake_lost` receipts. It uses the same per-comment consumption check and the same chat-input exclusion. - `heartbeat/stale-comment-continuation.ts` and `heartbeat.ts`: before a stale continuation is cancelled, its undelivered user comments are saved as a deferred receipt. The release drain then reopens the `done` task and delivers them once. This applies only when an agent run that started before the comment completed the task after it. A task that a person marks done is left closed. - `tests/restart-chaos/grader.ts`: R4 ignores runs cancelled before dispatch (`continuation_task_ownership_changed`) when it looks for overlapping execution. Those runs never executed. Comment consumption still counts them. ## Verification Each focused file ran alone in one process (`--maxWorkers=1`): - `heartbeat-held-comment-wake.test.ts` (22 pass). This includes two new cases: - t69 shape: the reconciler rule fires, the backstop delivers the comment once, and a second pass does nothing; - t72 shape: the stale run is cancelled, the task reopens, and the comment is delivered once. Both fail with the rule or the save disabled, and pass with them. - `ownership-reconciliation-report.test.ts` (23 pass). It includes the rule's boundaries: a carried comment, a too-young comment (deadline only) and an already-read comment. - `tests/restart-chaos/grader.test.ts` (17 pass). The new R4 case fails with the old grader. A cancelled run that is not a pre-dispatch cancellation still fails R4. - Unchanged and passing: comment wake batching (27), refused assignment wake (3), wake-queue postgres adapter (103), stale queue invalidation (68), deferred receipt work (45). The first version also reopened a task that a person had marked done, which broke one deferred-receipt test. That case is now excluded. - The server package typecheck and the restart-chaos harness typecheck pass. - Same-session chaos context on master: K6 crash loop `w-20261108T063326-s9402`: 6 of 6 pass. The K4 long-hold wave is still running. After merge, I'll replay only the targeted fault shapes (t69 and t72). ## Risks - The reconciler rule runs on each report pass. It is bounded by the 1-hour lookback and the per-kind row cap, and it excludes conversation tasks, so chat input keeps its own authorization. - A comment younger than 30 s waits up to the end of its grace period before the backstop acts. - The stale-run save reopens a `done` task only for a comment that the completing run could not have read. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, 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 --- .../heartbeat-held-comment-wake.test.ts | 113 +++++++++++++++++- .../ownership-reconciliation-report.test.ts | 30 +++++ server/src/services/heartbeat.ts | 7 ++ server/src/services/heartbeat/queue.ts | 6 +- .../heartbeat/stale-comment-continuation.ts | 46 +++++++ .../native-finalization-reconciler.ts | 39 +++++- .../recovery/ownership-invariant-report.ts | 100 +++++++++++++++- server/src/services/work-intent-outbox.ts | 78 +++++++++++- tests/restart-chaos/grader.test.ts | 21 ++++ tests/restart-chaos/grader.ts | 27 +++-- 10 files changed, 446 insertions(+), 21 deletions(-) create mode 100644 server/src/services/heartbeat/stale-comment-continuation.ts diff --git a/server/src/__tests__/heartbeat-held-comment-wake.test.ts b/server/src/__tests__/heartbeat-held-comment-wake.test.ts index 2ee971fd4fb..24856d54ef2 100644 --- a/server/src/__tests__/heartbeat-held-comment-wake.test.ts +++ b/server/src/__tests__/heartbeat-held-comment-wake.test.ts @@ -4,7 +4,7 @@ import { tmpdir } from "node:os"; import path from "node:path"; import { and, eq, sql } from "drizzle-orm"; import { afterAll, beforeAll, describe, expect, it } from "vitest"; -import { agents, agentWakeupRequests, authUsers, budgetPolicies, companies, companyMemberships, costEvents, createDb, heartbeatRuns, +import { activityLog, agents, agentWakeupRequests, authUsers, budgetPolicies, companies, companyMemberships, costEvents, createDb, heartbeatRuns, issueComments, issues } from "@paperclipai/db"; import { accountRunCost } from "../services/run-cost-accounting.js"; import { DELIVERY_QUEUES, subscribeDeliveryWork } from "../services/delivery-work-notifications.js"; @@ -12,6 +12,8 @@ import { requestTaskRetry } from "../services/task-retry.js"; import { queueIssueAssignmentWakeup } from "../services/issue-assignment-wakeup.js"; import { registerServerAdapter, unregisterServerAdapter } from "../adapters/index.js"; import { heartbeatService } from "../services/heartbeat.js"; +import { createOwnershipReportState, runOwnershipReconciliationPass } from "../services/recovery/ownership-invariant-report.js"; +import { applyOwnershipReportBackstops } from "../services/native-runtime/native-finalization-reconciler.js"; import { promoteDeferredWorkIntents, TEMPORARY_REFUSAL_TRIGGERS, WORK_INTENT_TERMINAL_OUTCOMES } from "../services/work-intent-outbox.js"; import { recoveryPostgresSupport, startRecoveryPostgres } from "./helpers/recovery-postgres.js"; @@ -116,6 +118,12 @@ describePostgres("saved intents while another run's accounting is pending", () = return woken; } + /** The agent's run closed the task, as a native status decision records it. */ + const closeByAgent = (f: { companyId: string; issueId: string; agentId: string }, actorType = "agent") => + db.insert(activityLog).values({ companyId: f.companyId, actorType, actorId: actorType === "agent" ? f.agentId : "person", + action: "issue.updated", entityType: "issue", entityId: f.issueId, + details: { source: "native_status_decision", fromStatus: "in_progress", toStatus: "done" } }); + async function settle(f: { companyId: string }) { await heartbeat.resumeQueuedRuns(); await heartbeat.reconcileStrandedAssignedIssues({ companyId: f.companyId }); @@ -394,10 +402,111 @@ describePostgres("saved intents while another run's accounting is pending", () = expect(started).toHaveLength(1); }, 180_000); + // Chaos s9401/t69: a restart landed between the comment commit (which + // reopened the finished task to todo) and its wake write. Reconciler rule: + // an undelivered user comment newer than the task's last run gets a wake. + it("delivers once a user comment whose wake a restart lost after a succeeded run", async () => { + const f = await seedHeldComment(); + await db.update(heartbeatRuns).set({ costAccountingPending: false }).where(eq(heartbeatRuns.id, f.pendingRunId)); + // The run that finished the task was native, as in the wave, and the + // comment came after it; the comment's wake was never written. + await db.update(heartbeatRuns).set({ runtimeMode: "native", startedAt: new Date(Date.now() - 120_000), + createdAt: new Date(Date.now() - 120_000) }).where(eq(heartbeatRuns.id, f.sourceRunId)); + await db.delete(agentWakeupRequests).where(eq(agentWakeupRequests.id, f.wakeId)); + await db.update(issueComments).set({ createdAt: new Date(Date.now() - 60_000), updatedAt: new Date(Date.now() - 60_000) }) + .where(eq(issueComments.id, f.commentId)); + await db.update(issues).set({ status: "todo" }).where(eq(issues.id, f.issueId)); + const report = await runOwnershipReconciliationPass(db, { state: createOwnershipReportState(), record: false }); + expect(report.violations.filter(row => row.reason === "user_comment_without_wake").map(row => row.recordId)) + .toEqual([f.commentId]); + await applyOwnershipReportBackstops(db, report); + await db.update(agentWakeupRequests).set({ updatedAt: new Date(Date.now() - 60_000) }) + .where(eq(agentWakeupRequests.companyId, f.companyId)); + await settle(f); await settle(f); + expect(deliveries(f.commentId)).toHaveLength(1); + // Delivered: the rule no longer fires, and nothing delivers it again. + const again = await runOwnershipReconciliationPass(db, { state: createOwnershipReportState(), record: false }); + expect(again.violations.filter(row => row.reason === "user_comment_without_wake")).toEqual([]); + await applyOwnershipReportBackstops(db, again); + await settle(f); + expect(deliveries(f.commentId)).toHaveLength(1); + }, 180_000); + + // Chaos s9401/t72: a message queued its own run while the first run was + // finishing. The first run completed the task, so the queued run was + // cancelled as a stale continuation and the unread message was dropped. + it("reopens a finished task for a message whose queued run turned stale, and delivers it once", async () => { + const f = await seedHeldComment(); + await db.update(heartbeatRuns).set({ costAccountingPending: false }).where(eq(heartbeatRuns.id, f.pendingRunId)); + await db.delete(agentWakeupRequests).where(eq(agentWakeupRequests.id, f.wakeId)); + // The message's own queued run; the first run then completed the task. + const [wake] = await db.insert(agentWakeupRequests).values({ companyId: f.companyId, agentId: f.agentId, source: "automation", + triggerDetail: "system", reason: "issue_commented", status: "queued", requestedByActorType: "user", requestedByActorId: f.userId, + payload: { issueId: f.issueId, commentId: f.commentId, mutation: "comment" } }).returning(); + const [stale] = await db.insert(heartbeatRuns).values({ companyId: f.companyId, agentId: f.agentId, invocationSource: "automation", + triggerDetail: "system", status: "queued", wakeupRequestId: wake.id, + contextSnapshot: { issueId: f.issueId, taskId: f.issueId, commentId: f.commentId, wakeCommentId: f.commentId, + wakeCommentIds: [f.commentId], wakeReason: "issue_commented", source: "issue.comment" } }).returning(); + await db.update(agentWakeupRequests).set({ runId: stale.id }).where(eq(agentWakeupRequests.id, wake.id)); + // The first run, which started before the message, completed the task after it. + await db.update(heartbeatRuns).set({ finishedAt: new Date(Date.now() + 1_000) }).where(eq(heartbeatRuns.id, f.sourceRunId)); + await db.update(issues).set({ status: "done", completedAt: new Date() }).where(eq(issues.id, f.issueId)); + await closeByAgent(f); + await settle(f); await settle(f); + expect((await db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, stale.id)))[0]).toMatchObject({ + status: "cancelled", errorCode: "continuation_task_ownership_changed" }); + await db.update(agentWakeupRequests).set({ updatedAt: new Date(Date.now() - 60_000) }) + .where(and(eq(agentWakeupRequests.companyId, f.companyId), eq(agentWakeupRequests.status, "deferred_issue_execution"))); + await settle(f); await settle(f); + expect(deliveries(f.commentId)).toHaveLength(1); + await settle(f); + expect(deliveries(f.commentId)).toHaveLength(1); + }, 180_000); + + // The durable half: the stale check ran before the closing run settled (or + // its save failed), so only the cancelled stale wake carries the message. + // Once the closing run settles, the reconciler saves it and the drain + // reopens the task and delivers it once. + it("recovers a message a stale continuation dropped once the closing run settles", async () => { + const f = await seedHeldComment(); + await db.update(heartbeatRuns).set({ costAccountingPending: false }).where(eq(heartbeatRuns.id, f.pendingRunId)); + await db.update(agentWakeupRequests).set({ status: "cancelled", finishedAt: new Date(), + error: "continuation_task_ownership_changed" }).where(eq(agentWakeupRequests.id, f.wakeId)); + await db.update(heartbeatRuns).set({ finishedAt: new Date(Date.now() + 1_000) }).where(eq(heartbeatRuns.id, f.sourceRunId)); + await db.update(issues).set({ status: "done", completedAt: new Date() }).where(eq(issues.id, f.issueId)); + // A person closed it: that decision stands. + await closeByAgent(f, "user"); + const byPerson = await runOwnershipReconciliationPass(db, { state: createOwnershipReportState(), record: false }); + expect(byPerson.violations.filter(row => row.reason === "user_comment_dropped_by_stale_continuation")).toEqual([]); + // The agent's run closed it later without reading the message. + await closeByAgent(f); + // Another message's own turn succeeded later; it did not receive this one. + await db.insert(heartbeatRuns).values({ companyId: f.companyId, agentId: f.agentId, invocationSource: "automation", + triggerDetail: "system", status: "succeeded", startedAt: new Date(Date.now() + 2_000), finishedAt: new Date(Date.now() + 3_000), + contextSnapshot: { issueId: f.issueId, wakeReason: "issue_commented", wakeCommentIds: [randomUUID()], + executionContinuation: { messages: [{ id: f.commentId }] } } }); + const report = await runOwnershipReconciliationPass(db, { state: createOwnershipReportState(), record: false }); + expect(report.violations.filter(row => row.reason === "user_comment_dropped_by_stale_continuation") + .map(row => row.recordId)).toEqual([f.commentId]); + expect(await applyOwnershipReportBackstops(db, report)).toEqual({ retryNeeded: false }); + await db.update(agentWakeupRequests).set({ updatedAt: new Date(Date.now() - 60_000) }) + .where(and(eq(agentWakeupRequests.companyId, f.companyId), eq(agentWakeupRequests.status, "deferred_issue_execution"))); + await settle(f); await settle(f); + expect(deliveries(f.commentId)).toHaveLength(1); + const again = await runOwnershipReconciliationPass(db, { state: createOwnershipReportState(), record: false }); + expect(again.violations.filter(row => row.recordId === f.commentId)).toEqual([]); + }, 180_000); + + it("keeps a reconciler pass due when the backstop cannot save a message", async () => { + const report = { violations: [{ rule: "I4_exactly_once_intent", kind: "issue_comment", recordId: randomUUID(), + companyId: "not-a-company-id", runId: null, issueId: "not-an-issue-id", reason: "user_comment_without_wake" }] }; + expect(await applyOwnershipReportBackstops(db, report as never)).toEqual({ retryNeeded: true }); + }); + it("keeps the terminal outcomes closed and every temporary refusal mapped to a release trigger", () => { expect([...WORK_INTENT_TERMINAL_OUTCOMES]).toEqual(["delivered", "consumed", "task_cancelled", "hard_stop"]); for (const refusal of ["budget.blocked", "company.inactive", "agent.not_invokable", "issue_tree_hold_active", - "heartbeat.scheduling_suppressed"]) expect(TEMPORARY_REFUSAL_TRIGGERS[refusal]).toBeTruthy(); + "heartbeat.scheduling_suppressed", "wake_lost"]) expect(TEMPORARY_REFUSAL_TRIGGERS[refusal]).toBeTruthy(); }); it.each([ diff --git a/server/src/__tests__/ownership-reconciliation-report.test.ts b/server/src/__tests__/ownership-reconciliation-report.test.ts index 730729d996b..d584a990654 100644 --- a/server/src/__tests__/ownership-reconciliation-report.test.ts +++ b/server/src/__tests__/ownership-reconciliation-report.test.ts @@ -13,6 +13,7 @@ import { createDb, environmentLeases, heartbeatRuns, + issueComments, issues, nativeRunFinalizations, sandboxCreationOperations, @@ -406,6 +407,35 @@ describe("report-only ownership reconciliation", () => { expect(report.violations.filter(violation => violation.rule === "I4_exactly_once_intent")).toEqual([]); }); + // Reconciler rule (chaos s9401/t69): an undelivered user comment newer than + // the task's last run gets a wake. A carried, a too-young, or an + // already-read comment does not fire it. + it("finds a user comment newer than the task's last run that no wake carries", async () => { + const issueId = randomUUID(); + await db.insert(issues).values({ id: issueId, companyId, title: "Reopened", status: "todo", assigneeAgentId: agentId }); + await db.insert(heartbeatRuns).values({ companyId, agentId, status: "succeeded", invocationSource: "manual", + runtimeMode: "native", contextSnapshot: { issueId }, startedAt: minutesAgo(5), createdAt: minutesAgo(5), finishedAt: minutesAgo(4) }); + const [lost, carried, young] = await db.insert(issueComments).values([ + { companyId, issueId, authorUserId: "user-1", body: "lost wake", createdAt: minutesAgo(2) }, + { companyId, issueId, authorUserId: "user-1", body: "carried", createdAt: minutesAgo(2) }, + { companyId, issueId, authorUserId: "user-1", body: "young", createdAt: new Date(Date.now() - 5_000) }, + ]).returning(); + // An agent's own comment written under its user (local CLI) is not user intent. + await db.insert(issueComments).values({ companyId, issueId, authorUserId: "user-1", authorAgentId: agentId, + body: "self", createdAt: minutesAgo(2) }); + await db.insert(agentWakeupRequests).values({ companyId, agentId, source: "automation", status: "queued", + payload: { issueId, commentId: carried.id } }); + const report = await runOwnershipReconciliationPass(db, { state: createOwnershipReportState(), record: false }); + expect(report.violations.filter(row => row.reason === "user_comment_without_wake").map(row => row.recordId)).toEqual([lost.id]); + // The young comment may still get its normal wake: a deadline, not a report. + expect(report.nextDueAt?.getTime()).toBe(young.createdAt.getTime() + 30_000); + // A run that started after the comment read it. + await db.insert(heartbeatRuns).values({ companyId, agentId, status: "succeeded", invocationSource: "manual", + contextSnapshot: { issueId }, startedAt: minutesAgo(1), createdAt: minutesAgo(1), finishedAt: new Date() }); + const later = await runOwnershipReconciliationPass(db, { state: createOwnershipReportState(), record: false }); + expect(later.violations.filter(row => row.reason === "user_comment_without_wake")).toEqual([]); + }); + it("keeps a backstop deadline for a lifted pause still inside its grace window", async () => { const updatedAt = minutesAgo(1); await db.insert(agentWakeupRequests).values({ companyId, agentId, source: "automation", status: "deferred_issue_execution", diff --git a/server/src/services/heartbeat.ts b/server/src/services/heartbeat.ts index ad1a131686d..785f697d1fe 100644 --- a/server/src/services/heartbeat.ts +++ b/server/src/services/heartbeat.ts @@ -320,6 +320,7 @@ import { import { buildHeartbeatRunStatusLiveEventPayload } from "./heartbeat-run-status-payload.js"; export { buildHeartbeatRunStatusLiveEventPayload } from "./heartbeat-run-status-payload.js"; import { StaleExecutionContinuationError } from "./execution-continuation.js"; +import { deferUndeliveredCommentsOfStaleRun } from "./heartbeat/stale-comment-continuation.js"; import { renderPaperclipWakePrompt } from "@paperclipai/adapter-utils/server-utils"; import { @@ -3796,6 +3797,12 @@ export function heartbeatService( // The queued continuation became obsolete before adapter dispatch. // Use cancellation settlement so wakeup, issue ownership, agent state, // and notifications agree; do not retry work for the previous owner. + // A user's message it carried that no run read is saved first, so the + // release reopens the finished task for it (work-intent outbox). If + // this fast path fails or the closing run has not settled yet, the + // ownership reconciler saves it once that run settles. + await deferUndeliveredCommentsOfStaleRun(db, run).catch(err => + logger.warn({ err, runId: run.id }, "could not save the messages of a stale continuation")); await cancelRunInternal(run.id, outerErr.code, { errorCode: outerErr.code, eventMessage: "stale execution continuation cancelled before dispatch", diff --git a/server/src/services/heartbeat/queue.ts b/server/src/services/heartbeat/queue.ts index 73dec05433f..0f0ebdf8f7d 100644 --- a/server/src/services/heartbeat/queue.ts +++ b/server/src/services/heartbeat/queue.ts @@ -1779,8 +1779,10 @@ export function createHeartbeatQueue(db: Db, dependencies: HeartbeatQueueDepende // which resumes through its own live authorization), and only pauses // this query can see lifted. A refusal by any other gate rewrites the // receipt's reason, so it never retries in a loop here. - sql`${agentWakeupRequests.payload}->'executionWait'->>'trigger' = 'pause_lifted'`, - sql`${agentWakeupRequests.payload}->'executionWait'->>'reason' in ('company.inactive', 'agent.not_invokable')`, + // A message the reconciler saved after its wake was lost waits on no + // hold, so the same explicit wake sends it. + sql`${agentWakeupRequests.payload}->'executionWait'->>'trigger' in ('pause_lifted', 'reconciler_backstop')`, + sql`${agentWakeupRequests.payload}->'executionWait'->>'reason' in ('company.inactive', 'agent.not_invokable', 'wake_lost')`, eq(agentWakeupRequests.requestedByActorType, "user"), sql`coalesce(${agentWakeupRequests.idempotencyKey}, '') not like 'chat-inbound:%'`, sql`exists (select 1 from agents pause_agent where pause_agent.id = ${agentWakeupRequests.agentId} diff --git a/server/src/services/heartbeat/stale-comment-continuation.ts b/server/src/services/heartbeat/stale-comment-continuation.ts new file mode 100644 index 00000000000..bedd51564e8 --- /dev/null +++ b/server/src/services/heartbeat/stale-comment-continuation.ts @@ -0,0 +1,46 @@ +import { agentWakeupRequests, heartbeatRuns, issueComments, type Db } from "@paperclipai/db"; +import { and, eq, inArray, sql } from "drizzle-orm"; +import { saveUndeliveredUserComments, taskClosedByAgentSql } from "../work-intent-outbox.js"; + +const UUID = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; + +/** + * Work-intent outbox (invariant 4). A user's message can wake its own queued + * run while an earlier run of the task is still finishing. When that earlier + * run completes the task first, the queued run reaches a done task and is + * cancelled as a stale continuation before dispatch, but the earlier run never + * read the message. Cancelling it alone would drop the message (chaos + * s9401/t72). + * + * Before that cancellation, the message's undelivered comments are saved as a + * deferred receipt of the same user. The run's cancellation releases the task, + * and the release drain reopens the done task for the user's message and + * delivers it once. Only a task that an agent run completed without reading + * the message qualifies: that run started before the comment and succeeded + * after it. A task a person marked done ends the message's intent. + * Returns the saved receipt id, or null when nothing is owed. + */ +export async function deferUndeliveredCommentsOfStaleRun(db: Db, run: typeof heartbeatRuns.$inferSelect): Promise { + const context = run.contextSnapshot && typeof run.contextSnapshot === "object" ? run.contextSnapshot as Record : {}; + const issueId = typeof context.issueId === "string" ? context.issueId : null; + const commentIds = Array.isArray(context.wakeCommentIds) + ? context.wakeCommentIds.filter((id): id is string => typeof id === "string" && UUID.test(id)) : []; + if (!issueId || !UUID.test(issueId) || commentIds.length === 0) return null; + const unread = await db.select({ id: issueComments.id }).from(issueComments).where(and( + eq(issueComments.companyId, run.companyId), eq(issueComments.issueId, issueId), inArray(issueComments.id, commentIds), + sql`exists (select 1 from ${heartbeatRuns} completing where completing.company_id = ${run.companyId} + and completing.agent_id = ${run.agentId} and completing.id <> ${run.id} and completing.status = 'succeeded' + and (completing.context_snapshot->>'issueId' = ${issueId} or completing.native_issue_id = ${issueId}) + and completing.started_at < ${issueComments.createdAt} and completing.finished_at >= ${issueComments.createdAt})`)); + const owed = commentIds.filter(id => unread.some(row => row.id === id)); + if (owed.length === 0) return null; + // An overlapping run is not proof the agent closed the task: a person can + // mark it done while that run finishes. Only an agent-made close reopens. + const [closed] = await db.execute<{ byAgent: boolean }>(sql`select ${taskClosedByAgentSql(run.companyId, issueId)} as "byAgent"`); + if (closed?.byAgent !== true) return null; + const [wake] = run.wakeupRequestId ? await db.select().from(agentWakeupRequests) + .where(and(eq(agentWakeupRequests.companyId, run.companyId), eq(agentWakeupRequests.id, run.wakeupRequestId))) : []; + return saveUndeliveredUserComments(db, { companyId: run.companyId, issueId, agentId: run.agentId, commentIds: owed, + allowedStatuses: ["done"], requestedByActorId: wake?.requestedByActorType === "user" ? wake.requestedByActorId : null, + source: wake?.source, triggerDetail: wake?.triggerDetail }); +} diff --git a/server/src/services/native-runtime/native-finalization-reconciler.ts b/server/src/services/native-runtime/native-finalization-reconciler.ts index 45e15f3feb7..7e22e0c20bf 100644 --- a/server/src/services/native-runtime/native-finalization-reconciler.ts +++ b/server/src/services/native-runtime/native-finalization-reconciler.ts @@ -18,12 +18,14 @@ import type { createDeliveryWorkCoordinator } from "../delivery-work-coordinator import { createOwnershipReportState, DEFERRED_INTENT_NOT_PROMOTED, + USER_COMMENT_DROPPED_BY_STALE_RUN, + USER_COMMENT_WITHOUT_WAKE, ownershipReportFingerprint, recheckSettlingKinds, runOwnershipReconciliationPass, type OwnershipReport, } from "../recovery/ownership-invariant-report.js"; -import { promoteDeferredWorkIntents } from "../work-intent-outbox.js"; +import { deferredWorkIntentWait, promoteDeferredWorkIntents, saveUndeliveredUserComments } from "../work-intent-outbox.js"; import { createHash, randomUUID } from "node:crypto"; import { and, asc, desc, eq, gt, inArray, isNotNull, isNull, lt, lte, not, notInArray, or, sql, type SQL } from "drizzle-orm"; import type { Db } from "@paperclipai/db"; @@ -824,13 +826,37 @@ export const OWNERSHIP_REPORT_INTERVAL_MS = 30_000; /** Section E backstop: a deferred intent whose hold cleared without a * release event is promoted through the one promotion function. Promotion * re-runs normal admission, so a wrong report never bypasses a gate. */ -async function promoteMissedDeferredIntents(db: Db, report: OwnershipReport) { +export async function applyOwnershipReportBackstops(db: Db, report: OwnershipReport): Promise<{ retryNeeded: boolean }> { + let retryNeeded = false; + // Rules: an undelivered user comment newer than the task's last run gets a + // wake, and a comment a stale continuation dropped reopens its done task. + // Save the missing outbox entry, then promote it. + for (const violation of report.violations.filter(row => + row.reason === USER_COMMENT_WITHOUT_WAKE || row.reason === USER_COMMENT_DROPPED_BY_STALE_RUN)) { + if (!violation.companyId || !violation.issueId) continue; + const lost = violation.reason === USER_COMMENT_WITHOUT_WAKE; + try { + const [issue] = await db.select({ assigneeAgentId: issues.assigneeAgentId }).from(issues) + .where(and(eq(issues.companyId, violation.companyId), eq(issues.id, violation.issueId))); + if (!issue?.assigneeAgentId) continue; + const saved = await saveUndeliveredUserComments(db, { companyId: violation.companyId, issueId: violation.issueId, + agentId: issue.assigneeAgentId, commentIds: [violation.recordId], allowedStatuses: lost ? ["todo", "in_progress"] : ["done"], + ...(lost ? { executionWait: deferredWorkIntentWait("wake_lost", "This message's wake was lost; it is sent now.") } : {}) }); + if (saved) await db.transaction(tx => promoteDeferredWorkIntents(tx, { companyId: violation.companyId!, + trigger: "reconciler_backstop", issueId: violation.issueId })); + } catch (error) { + // The message still has no saved receipt: keep a pass due. + retryNeeded = true; + logger.warn({ err: error, commentId: violation.recordId }, "could not save a user comment without a wake"); + } + } const companies = new Set(report.violations.filter(violation => violation.reason === DEFERRED_INTENT_NOT_PROMOTED) .flatMap(violation => violation.companyId ? [violation.companyId] : [])); for (const companyId of companies) { await db.transaction(tx => promoteDeferredWorkIntents(tx, { companyId, trigger: "reconciler_backstop" })) - .catch(error => logger.warn({ err: error, companyId }, "deferred intent backstop promotion failed")); + .catch(error => { retryNeeded = true; logger.warn({ err: error, companyId }, "deferred intent backstop promotion failed"); }); } + return { retryNeeded }; } export function registerOwnershipReconciliationWork( @@ -895,12 +921,13 @@ export function registerOwnershipReconciliationWork( lastPassAt = Date.now(); pendingWake = false; const report = await runOwnershipReconciliationPass(db, { state }); - await promoteMissedDeferredIntents(db, report); + const backstop = await applyOwnershipReportBackstops(db, report); const fingerprint = ownershipReportFingerprint(report); const changing = report.inProgress > 0 && fingerprint !== lastFingerprint; // A settling kind's live owner can die without writing a signal, so - // keep one bounded re-check due while such records exist. - const fullPassDue = changing || report.awaitingSecondProof > 0 || report.retryNeeded; + // keep one bounded re-check due while such records exist. A backstop + // that could not save or promote keeps one due as well. + const fullPassDue = changing || report.awaitingSecondProof > 0 || report.retryNeeded || backstop.retryNeeded; watchOnly = !fullPassDue && report.settleWatch > 0; const followUp = fullPassDue || report.settleWatch > 0 ? Date.now() + OWNERSHIP_REPORT_INTERVAL_MS : null; diff --git a/server/src/services/recovery/ownership-invariant-report.ts b/server/src/services/recovery/ownership-invariant-report.ts index 7650e5ffdda..269bb7e153d 100644 --- a/server/src/services/recovery/ownership-invariant-report.ts +++ b/server/src/services/recovery/ownership-invariant-report.ts @@ -6,7 +6,8 @@ import { issues, type Db, } from "@paperclipai/db"; -import { TEMPORARY_REFUSAL_TRIGGERS } from "../work-intent-outbox.js"; +import { taskClosedByAgentSql, TEMPORARY_REFUSAL_TRIGGERS } from "../work-intent-outbox.js"; +import { deliveredQueuedCommentIds } from "../queued-comment-delivery.js"; import { logger } from "../../middleware/logger.js"; import { persistActivity } from "../activity-log.js"; import { proveOwnerGone, type OwnerProof, type ProcessProbe } from "../owner-proof.js"; @@ -366,6 +367,97 @@ async function checkDeferredIntentsPromoted(db: Db, now: Date, limit: number): P } export const DEFERRED_INTENT_NOT_PROMOTED = "deferred_after_hold_cleared"; +export const USER_COMMENT_WITHOUT_WAKE = "user_comment_without_wake"; +/** A user comment younger than this may still be getting its normal wake. */ +const COMMENT_WAKE_GRACE_MS = 30_000; + +export const USER_COMMENT_DROPPED_BY_STALE_RUN = "user_comment_dropped_by_stale_continuation"; + +/** Invariant 4, the durable half of the stale-continuation rule (chaos + * s9401/t72). A user comment on a done task whose only carrying wakes were + * cancelled as stale continuations, that a settled agent run which started + * before it never read (it finished after the comment), and that no later + * run received (the shared per-comment delivery check). The decision waits for the closing run to settle, so it + * also covers a stale check that ran before that run reached succeeded, or a + * save that failed. The worker saves it so the release drain reopens the + * task and delivers it once. */ +async function checkStaleDroppedComments(db: Db, now: Date, limit: number): Promise { + const since = new Date(now.getTime() - INTENT_LOOKBACK_MS).toISOString(); + const rows = await db.execute<{ id: string; company_id: string; issue_id: string; agent_id: string }>(sql` + select c.id, c.company_id, c.issue_id, i.assignee_agent_id as agent_id from issue_comments c + join issues i on i.id = c.issue_id and i.company_id = c.company_id + where c.author_user_id is not null and c.deleted_at is null + and c.author_agent_id is null and c.created_by_run_id is null and c.created_at >= ${since}::timestamptz + and i.status = 'done' and i.assignee_agent_id is not null and i.conversation_agent_id is null + and ${taskClosedByAgentSql(sql`c.company_id`, sql`c.issue_id`)} + and exists (select 1 from agent_wakeup_requests stale where stale.company_id = c.company_id + and stale.payload->>'issueId' = c.issue_id::text and stale.status = 'cancelled' + and stale.error = 'continuation_task_ownership_changed' + and (stale.payload->>'commentId' = c.id::text + or coalesce(stale.payload#>'{_paperclipWakeContext,wakeCommentIds}', '[]'::jsonb) ? c.id::text)) + and not exists (select 1 from agent_wakeup_requests open_wake where open_wake.company_id = c.company_id + and open_wake.payload->>'issueId' = c.issue_id::text + and open_wake.status not in ('cancelled', 'skipped', 'failed', 'coalesced') + and (open_wake.payload->>'commentId' = c.id::text + or coalesce(open_wake.payload#>'{_paperclipWakeContext,wakeCommentIds}', '[]'::jsonb) ? c.id::text)) + and exists (select 1 from heartbeat_runs closing where closing.company_id = c.company_id + and closing.agent_id = i.assignee_agent_id and closing.status = 'succeeded' + and (closing.context_snapshot->>'issueId' = c.issue_id::text or closing.native_issue_id = c.issue_id) + and closing.started_at < c.created_at and closing.finished_at >= c.created_at) + and not exists (select 1 from heartbeat_runs r where r.company_id = c.company_id + and (r.context_snapshot->>'issueId' = c.issue_id::text or r.native_issue_id = c.issue_id) + and r.status in ('queued', 'running', 'scheduled_retry')) + order by c.created_at limit ${Math.min(limit, MAX_RECORDS_PER_KIND)}`); + // Consumption is per comment: a later run counts only if it received this + // comment (its own wake input, or a retry's task history), never ordinary + // task history of another message's turn. + const owed: typeof rows[number][] = []; + for (const row of rows) { + const delivered = await deliveredQueuedCommentIds(db, { companyId: row.company_id, issueId: row.issue_id, + agentId: row.agent_id, commentIds: [row.id] }); + if (!delivered.has(row.id)) owed.push(row); + } + return owed.map(row => ({ rule: "I4_exactly_once_intent" as const, kind: "issue_comment", recordId: row.id, + companyId: row.company_id, runId: null, issueId: row.issue_id, reason: USER_COMMENT_DROPPED_BY_STALE_RUN })); +} + +/** Invariant 4, reconciler-owned rule: an undelivered user comment newer than + * the task's last run gets a wake. A comment commits before its wake is + * written, so a restart in between loses the wake (chaos s9401/t69). This + * finds a user comment on an assigned, non-conversation task with no live run + * or lock, newer than every run of the task, that no wake request carries. + * The reconciliation worker saves it as a deferred receipt and promotes it. */ +async function checkUserCommentsWithoutWake(db: Db, now: Date, limit: number): Promise<{ violations: InvariantViolation[]; nextDueAt: Date | null }> { + const since = new Date(now.getTime() - INTENT_LOOKBACK_MS).toISOString(); + const rows = await db.execute<{ id: string; company_id: string; issue_id: string; created_at: string }>(sql` + select c.id, c.company_id, c.issue_id, c.created_at::text as created_at from issue_comments c + join issues i on i.id = c.issue_id and i.company_id = c.company_id + where c.author_user_id is not null and c.deleted_at is null + and c.author_agent_id is null and c.created_by_run_id is null + and c.created_at >= ${since}::timestamptz + and i.assignee_agent_id is not null and i.conversation_agent_id is null + and i.status in ('todo', 'in_progress') and i.execution_run_id is null + and exists (select 1 from heartbeat_runs prior where prior.company_id = c.company_id + and (prior.context_snapshot->>'issueId' = c.issue_id::text or prior.native_issue_id = c.issue_id)) + and not exists (select 1 from heartbeat_runs r where r.company_id = c.company_id + and (r.context_snapshot->>'issueId' = c.issue_id::text or r.native_issue_id = c.issue_id) + and (r.status in ('queued', 'running', 'scheduled_retry') or coalesce(r.started_at, r.created_at) >= c.created_at)) + and not exists (select 1 from agent_wakeup_requests w where w.company_id = c.company_id + and w.payload->>'issueId' = c.issue_id::text + and (w.payload->>'commentId' = c.id::text + or coalesce(w.payload#>'{_paperclipWakeContext,wakeCommentIds}', '[]'::jsonb) ? c.id::text)) + order by c.created_at limit ${Math.min(limit, MAX_RECORDS_PER_KIND)}`); + // A comment inside the grace may still get its normal wake: check it again + // when the grace ends (a saved deadline), and report only older ones. + const graceEnd = now.getTime() - COMMENT_WAKE_GRACE_MS; + const young = [...rows].map(row => new Date(row.created_at).getTime()).filter(at => at >= graceEnd); + return { + violations: [...rows].filter(row => new Date(row.created_at).getTime() < graceEnd) + .map(row => ({ rule: "I4_exactly_once_intent" as const, kind: "issue_comment", recordId: row.id, + companyId: row.company_id, runId: null, issueId: row.issue_id, reason: USER_COMMENT_WITHOUT_WAKE })), + nextDueAt: young.length ? new Date(Math.min(...young) + COMMENT_WAKE_GRACE_MS) : null, + }; +} /** Invariant 4 with section E's closed outcomes: an accepted intent (a user's * request or a message) ended terminally on a refusal that @@ -536,6 +628,12 @@ export async function runOwnershipReconciliationPass(db: Db, options: { for (const check of [ () => checkAdmissionIndependence(db, state, now, limit), () => checkExactlyOnceIntents(db, state, now, limit), + () => checkStaleDroppedComments(db, now, limit), + async () => { + const result = await checkUserCommentsWithoutWake(db, now, limit); + if (result.nextDueAt && (!nextDueAt || result.nextDueAt < nextDueAt)) nextDueAt = result.nextDueAt; + return result.violations; + }, async () => { const result = await checkDeferredIntentsPromoted(db, now, limit); if (result.nextDueAt && (!nextDueAt || result.nextDueAt < nextDueAt)) nextDueAt = result.nextDueAt; diff --git a/server/src/services/work-intent-outbox.ts b/server/src/services/work-intent-outbox.ts index 22c5b763635..89034210e03 100644 --- a/server/src/services/work-intent-outbox.ts +++ b/server/src/services/work-intent-outbox.ts @@ -1,6 +1,8 @@ -import { and, eq, sql } from "drizzle-orm"; -import { agentWakeupRequests, type Db } from "@paperclipai/db"; -import { notifyDeferredExecutionEligibility, notifyDeferredWakesForIssue } from "./deferred-execution-notifications.js"; +import { and, eq, inArray, isNotNull, isNull, sql } from "drizzle-orm"; +import { agentWakeupRequests, issueComments, issues, type Db } from "@paperclipai/db"; +import { notifyDeferredExecutionEligibility, notifyDeferredExecutionWork, notifyDeferredWakesForIssue } from "./deferred-execution-notifications.js"; +import { withQueuedCommentIdsInWakePayload } from "./issue-queued-comment-queue.js"; +import { deliveredQueuedCommentIds } from "./queued-comment-delivery.js"; import { notifyIssueRecovery } from "./issue-recovery-notifications.js"; import { notifyRunDispatch } from "./run-dispatch-notifications.js"; @@ -37,6 +39,8 @@ export const TEMPORARY_REFUSAL_TRIGGERS: Readonly>'issueId' = ${input.issueId}` : undefined, )); } + +const DEFERRED_WAKE_CONTEXT_KEY = "_paperclipWakeContext"; + +/** SQL: the latest recorded `done` transition of the task was not made by a + * person. A task a person closed (or with no recorded close) is never + * reopened for an earlier message. */ +export function taskClosedByAgentSql(companyId: unknown, issueId: unknown) { + return sql`coalesce((select closed.actor_type from activity_log closed + where closed.company_id = ${companyId} and closed.entity_type = 'issue' and closed.entity_id = ${issueId}::text + and closed.action = 'issue.updated' + and (closed.details->>'toStatus' = 'done' or closed.details->>'status' = 'done') + order by closed.created_at desc limit 1), 'user') <> 'user'`; +} + +/** + * Record the missing outbox entry for user messages whose wake was lost or + * ended without delivering them: one deferred receipt of the user that + * carries the undelivered comments. Only the task's assigned agent, and only + * live user comments that no completed run received, are saved. Statuses the + * caller allows (for example `done`, which the release drain reopens for a + * user's message) gate it. Normal admission and the release drain deliver + * the receipt once. Returns the receipt id, or null when nothing is owed. + */ +export async function saveUndeliveredUserComments(db: Db, input: { + companyId: string; issueId: string; agentId: string; commentIds: string[]; + allowedStatuses: readonly string[]; requestedByActorId?: string | null; + source?: string | null; triggerDetail?: string | null; + /** Names why the receipt waits and what promotes it. */ + executionWait?: Record; +}): Promise { + if (input.commentIds.length === 0) return null; + return db.transaction(async tx => { + const [issue] = await tx.select({ status: issues.status, assigneeAgentId: issues.assigneeAgentId, + conversationAgentId: issues.conversationAgentId }).from(issues) + .where(and(eq(issues.companyId, input.companyId), eq(issues.id, input.issueId))).for("update"); + // A reassigned or cancelled task ends the intent; a conversation keeps + // its own inbound authorization path. + if (!issue || issue.assigneeAgentId !== input.agentId || issue.conversationAgentId || + !input.allowedStatuses.includes(issue.status)) return null; + if (issue.status === "done") { + const [closed] = await tx.execute<{ byAgent: boolean }>(sql`select ${taskClosedByAgentSql(input.companyId, input.issueId)} as "byAgent"`); + if (closed?.byAgent !== true) return null; + } + const live = await tx.select({ id: issueComments.id, authorUserId: issueComments.authorUserId }).from(issueComments).where(and( + eq(issueComments.companyId, input.companyId), eq(issueComments.issueId, input.issueId), + inArray(issueComments.id, input.commentIds), isNotNull(issueComments.authorUserId), isNull(issueComments.deletedAt), + // A direct human comment: no agent author and no creating run. + isNull(issueComments.authorAgentId), isNull(issueComments.createdByRunId))); + const delivered = await deliveredQueuedCommentIds(tx as unknown as Db, { companyId: input.companyId, + issueId: input.issueId, agentId: input.agentId, commentIds: live.map(row => row.id) }); + const remaining = input.commentIds.filter(id => live.some(row => row.id === id) && !delivered.has(id)); + if (remaining.length === 0) return null; + const latest = remaining.at(-1)!; + await notifyDeferredExecutionWork(tx, { companyId: input.companyId }); + await notifyDeferredWakesForIssue(tx, { companyId: input.companyId, issueId: input.issueId }); + const [saved] = await tx.insert(agentWakeupRequests).values({ + companyId: input.companyId, agentId: input.agentId, source: input.source ?? "automation", + triggerDetail: input.triggerDetail ?? "system", reason: "issue_commented", status: "deferred_issue_execution", + requestedByActorType: "user", + requestedByActorId: input.requestedByActorId ?? live.find(row => row.id === latest)?.authorUserId ?? null, + payload: withQueuedCommentIdsInWakePayload({ issueId: input.issueId, mutation: "comment", commentId: latest, + ...(input.executionWait ? { executionWait: input.executionWait } : {}), + [DEFERRED_WAKE_CONTEXT_KEY]: { issueId: input.issueId, taskId: input.issueId, wakeReason: "issue_commented", + source: "issue.comment", wakeCommentIds: remaining, wakeCommentId: latest } }, remaining), + }).returning({ id: agentWakeupRequests.id }); + return saved?.id ?? null; + }); +} diff --git a/tests/restart-chaos/grader.test.ts b/tests/restart-chaos/grader.test.ts index ec65b2a3e5e..416fc214eb0 100644 --- a/tests/restart-chaos/grader.test.ts +++ b/tests/restart-chaos/grader.test.ts @@ -127,6 +127,27 @@ describe("R1–R6 grading on fixture timelines", () => { expect(gradeR4(record(afterSuccess)).detail).toMatch(/comment_consumed_twice: c1 by r1,r2/); }); + // s9401/t72: the message's run was claimed just before the first run ended, + // then cancelled as stale before dispatch; a later run delivered the message. + it("passes R4 for a stale continuation cancelled before dispatch whose message a later run delivered once", () => { + const samples = [sample(90_000, "done", [ + run("r1", "succeeded", 1_000, 40_000), + run("r2", "cancelled", 30_000, 40_060, { startedAt: iso(39_960), wakeCommentIds: ["c1"], + errorCode: "continuation_task_ownership_changed" }), + run("r3", "succeeded", 45_000, 80_000, { wakeCommentIds: ["c1"] }), + ])]; + expect(gradeR4(record(samples))).toMatchObject({ verdict: "pass" }); + // A poll that saw both running (claiming marks running before dispatch) + // does not change that. + const polled = [sample(39_990, "in_progress", [run("r1", "running", 1_000), run("r2", "running", 30_000, null, { startedAt: iso(39_960) })]), + ...samples]; + expect(gradeR4(record(polled))).toMatchObject({ verdict: "pass" }); + // Any other cancelled run that overlapped still executed. + const executed = [sample(90_000, "done", [run("r1", "succeeded", 1_000, 40_000), + run("r2", "cancelled", 30_000, 40_060, { startedAt: iso(39_960), errorCode: "cancelled" })])]; + expect(gradeR4(record(executed)).detail).toMatch(/concurrent_runs/); + }); + it("fails R5 on a silent stall longer than three minutes after ready, but not during outages or named waits", () => { const idle = (at: number, extra: Partial = {}) => sample(at, "in_progress", [run("r1", "failed", 1_000, 30_000)], extra); const stalled = [idle(60_000), idle(60_000 + STALL_LIMIT_MS / 2), idle(60_000 + STALL_LIMIT_MS + 5_000)]; diff --git a/tests/restart-chaos/grader.ts b/tests/restart-chaos/grader.ts index 4dd4d2cc109..aaead120c04 100644 --- a/tests/restart-chaos/grader.ts +++ b/tests/restart-chaos/grader.ts @@ -103,6 +103,9 @@ export function gradeR3(record: TaskRecord): RuleResult { return { verdict: "pass", detail: "one Retry started a run that completed the task" }; } +/** Run cancellations decided before adapter dispatch: the run never executed. */ +const PRE_DISPATCH_CANCELLATIONS = new Set(["continuation_task_ownership_changed"]); + /** * R4 is about duplicate execution. A queued or scheduled wake waiting behind a * running run (for example a follow-up comment, cancelled when the running run @@ -114,22 +117,32 @@ export function gradeR3(record: TaskRecord): RuleResult { * after a successful run is a replay. */ export function gradeR4(record: TaskRecord): RuleResult { - for (const sample of reachable(record.samples)) { - const running = (sample.runs ?? []).filter(run => run.status === "running"); - if (running.length > 1) return { verdict: "fail", detail: `concurrent_runs: ${running.map(run => `${run.id}(${run.status})`).join(",")}` }; - } const finalRuns = new Map[number]>(); for (const sample of reachable(record.samples)) for (const run of sample.runs ?? []) finalRuns.set(run.id, run); + // Claiming marks a run running before dispatch, so a run that ended as a + // pre-dispatch cancellation never executed, whatever a poll saw. + const neverExecuted = (id: string) => { + const final = finalRuns.get(id); + return final?.status === "cancelled" && PRE_DISPATCH_CANCELLATIONS.has(final.errorCode ?? ""); + }; + for (const sample of reachable(record.samples)) { + const running = (sample.runs ?? []).filter(run => run.status === "running" && !neverExecuted(run.id)); + if (running.length > 1) return { verdict: "fail", detail: `concurrent_runs: ${running.map(run => `${run.id}(${run.status})`).join(",")}` }; + } const begun = [...finalRuns.values()].filter(run => run.startedAt) .sort((a, b) => (ms(a.createdAt) || 0) - (ms(b.createdAt) || 0)); + // A continuation cancelled as stale before adapter dispatch was claimed but + // never executed (chaos s9401/t72), so it cannot overlap another run. Its + // message is saved and delivered by a later run, counted once below. + const executed = begun.filter(run => !(run.status === "cancelled" && PRE_DISPATCH_CANCELLATIONS.has(run.errorCode ?? ""))); // Execution windows seen between polls: started → finished (or still going at the last sample). const lastAt = Math.max(0, ...reachable(record.samples).map(sample => sample.at)); const window = (run: (typeof begun)[number]) => ({ start: ms(run.startedAt) || 0, end: run.finishedAt ? ms(run.finishedAt) || 0 : ACTIVE_RUN_STATUSES.has(run.status) ? lastAt : ms(run.startedAt) || 0 }); - for (let index = 0; index < begun.length; index += 1) for (let other = index + 1; other < begun.length; other += 1) { - const [a, b] = [window(begun[index]!), window(begun[other]!)]; + for (let index = 0; index < executed.length; index += 1) for (let other = index + 1; other < executed.length; other += 1) { + const [a, b] = [window(executed[index]!), window(executed[other]!)]; if (a.start < b.end && b.start < a.end) { - return { verdict: "fail", detail: `concurrent_runs: ${begun[index]!.id}(${begun[index]!.status}),${begun[other]!.id}(${begun[other]!.status}) executed together` }; + return { verdict: "fail", detail: `concurrent_runs: ${executed[index]!.id}(${executed[index]!.status}),${executed[other]!.id}(${executed[other]!.status}) executed together` }; } } const consumers = new Map(); From a4dbde6181bcc2bd4c3ad0443dc00eb4e040356d Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Sun, 8 Nov 2026 02:35:15 -0600 Subject: [PATCH 6/9] refactor(recovery): judge native owner processes with the shared prover (#19997) 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 > - Native runs record the server and adapter processes that own them, and recovery must prove an owner gone before it takes over or releases its work > - Phase 2 of the durable ownership plan gave the repository one prover, `proveOwnerGone`. The native runtime still had four private proofs of the same fact > - The private proofs disagreed with the shared rules in edge cases: some judged a PID from another PID space, and one accepted an inconsistent start time as proof of reuse > - This pull request moves those four proofs onto `proveOwnerGone` and deletes the private liveness code > - The benefit is one rule for "the owner is gone" across native recovery, with the stricter shared behaviour where they differed ## Linked Issues or Issue Description Refs #19933, #19950, #19953 (the shared ownership path and earlier Phase 2 kinds). **What is the problem?** Native recovery proved dead owners with four private checks instead of the shared prover. The checks used different rules for PID spaces, reused PIDs and unreadable start times. **What should happen?** Every native owner proof uses `proveOwnerGone`. Only a `gone` verdict releases or takes over. The remote fence and the one-release legacy fallback stay as they are. **Why does it matter?** Recovery decisions about the same process must agree. A dead PID in another PID space must never be used as proof. ## What Changed - `native-restart-recovery.ts` `evaluateNativeControllerTakeover`: the local-process branch now calls `proveOwnerGone`. The remote fenced branch (#19830), the `operator_attested` sentinel and the null-host fallback (`processSpaceUnrecorded` with `legacyHostFallback`, recorded by callers as `legacy_host_unknown_dead_pid_proof`) are kept. Reasons are unchanged, plus a new `controller_pid_start_inconsistent`. Owners built from receipts with no PID space are judged in this PID space: their callers check the recorded host first. - `native-failed-run-owner-release.ts`: `recordedProcessMayBeLive` is deleted. `runProcessMayBeLive` and `controllerMayBeLive` release only on a `gone` verdict. Each use of the legacy fallback is recorded as `legacy_host_unknown_dead_pid_proof` when the release commits. - `native-session-executor.ts`: `verifiedRecoveryProcessIsAlive` is replaced by `recoveryProcessMayBeAlive`, which is false only when the prover proves the runner gone. Signalling a recovered runner requires a proven same process. - `native-local-process-stop.ts` `recordedLocalExecutionStopped`: the bound identity (launch event, host, PID, group, start time) is judged by the prover. It uses `process_host_hash` when the row has one. - New `native-runtime/native-owner-stamps.ts`: controller and adapter-process stamp builders, shared with the ownership registry. Not changed: `proveLocalWorkspaceOwnerStopped` (workspaces), `evaluateNativeProviderProcesses`, the stop observation in `recordNativeLocalProcessStop`, and `runProcessMayBeLive` in `native-workspace-sync-retention.ts`. ## Verification - `cd server && npx vitest run src/services/native-runtime/native-restart-recovery.test.ts src/services/controller-process-identity.test.ts`: 51 passed. - `cd server && npx vitest run src/services/native-runtime/native-failed-run-owner-release.test.ts`: 14 passed. - `cd server && npx vitest run src/services/native-runtime/native-controller-host-recovery.test.ts`: 2 passed. - `cd server && npx vitest run src/services/quarantined-task-retry.test.ts -t "keeps quarantine"`: 18 passed. `explicit-native-continuation.test.ts`: 414 passed. `ownership-reconciliation-report.test.ts`: 23 passed. - Red with the master versions of the two modules: 3 new tests fail ("accepts a reused PID only when the live process started definitely later", "never releases on a dead PID recorded in another PID space", "keeps the one-release fallback for a run process saved before its PID space, and records it"). The tests were written after the code. - Two fixtures changed to match the shared rules: - `controller-process-identity.test.ts`: the "recycled" case recorded a start after the live process, which is an inconsistent stamp. It now records an earlier start (a real reuse), and the inconsistent case is asserted as alive. - `quarantined-task-retry.test.ts` `worker_alive`: the live worker now records this process's real start time. The old fixture was a reused PID, which the prover correctly proves gone. - `cd server && npx vitest run src/services/native-local-process-stop.test.ts`: 5 passed (review round 1). With the master module, 2 fail: the other-PID-space case and the reused-PID case. - Not run locally: typecheck (CI runs it) and the wide suites. ## Risks Behaviour changes, all from the shared rules: - A live PID observed to start before the recorded controller is no longer accepted as recycled (`controller_pid_start_inconsistent`; the run waits for evidence). - A failed-run owner whose recorded adapter process or controller is in another PID space, or is a remote sandbox PID, is no longer released by a local probe. It keeps the lease path. - An unreadable start time is no longer exit evidence for a recovered local runner. - A local execution whose PID was reused by a later process now counts as stopped. Before, any live PID blocked the proof. - A local execution whose row records another PID space is no longer counted as stopped by a dead local PID. - A PID space that cannot be read here makes a hostless receipt owner unprovable. No persisted state changes. Rows saved by the previous release keep the one-release null-host fallback, as before. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, via Claude Code with tool use (shell, file edits, local test runs). ## 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 --- .../controller-process-identity.test.ts | 13 ++-- .../native-local-process-stop.test.ts | 71 +++++++++++++++++++ .../src/services/native-local-process-stop.ts | 15 +++- .../native-failed-run-owner-release.test.ts | 27 +++++++ .../native-failed-run-owner-release.ts | 61 ++++++++-------- .../native-runtime/native-owner-stamps.ts | 47 ++++++++++++ .../native-restart-recovery.test.ts | 27 +++++++ .../native-runtime/native-restart-recovery.ts | 65 +++++++++-------- .../native-runtime/native-session-executor.ts | 41 +++++------ .../services/quarantined-task-retry.test.ts | 6 +- .../services/recovery/ownership-registry.ts | 30 ++------ 11 files changed, 291 insertions(+), 112 deletions(-) create mode 100644 server/src/services/native-local-process-stop.test.ts create mode 100644 server/src/services/native-runtime/native-owner-stamps.ts diff --git a/server/src/services/controller-process-identity.test.ts b/server/src/services/controller-process-identity.test.ts index e029bf52589..2fd8b9a465f 100644 --- a/server/src/services/controller-process-identity.test.ts +++ b/server/src/services/controller-process-identity.test.ts @@ -4,7 +4,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { processExists, processStartsDefinitelyDiffer } from "./controller-process-identity.js"; import { isPidAlive, isProcessGroupAlive } from "./local-service-supervisor.js"; import { readProcessStartedAt } from "./hot-restart.js"; -import { verifiedRecoveryProcessIsAlive } from "./native-runtime/native-session-executor.js"; +import { recoveryProcessMayBeAlive } from "./native-runtime/native-session-executor.js"; describe("shared process identity proof", () => { afterEach(() => vi.restoreAllMocks()); @@ -41,9 +41,14 @@ describe("shared process identity proof", () => { try { const observed = new Date((await readProcessStartedAt(live.pid!))!); const startedAt = new Date(observed.getTime() + 500).toISOString(); - await expect(verifiedRecoveryProcessIsAlive({ pid: live.pid!, startedAt })).resolves.toBe(true); - const recycled = new Date(observed.getTime() + 5_000).toISOString(); - await expect(verifiedRecoveryProcessIsAlive({ pid: live.pid!, startedAt: recycled })).resolves.toBe(false); + await expect(recoveryProcessMayBeAlive({ pid: live.pid!, startedAt })).resolves.toBe(true); + // The live PID started definitely after the recorded runner: reused, so gone. + const recycled = new Date(observed.getTime() - 5_000).toISOString(); + await expect(recoveryProcessMayBeAlive({ pid: live.pid!, startedAt: recycled })).resolves.toBe(false); + // A live PID that started before the recorded runner is an inconsistent + // stamp (shared prover), never exit evidence. + const inconsistent = new Date(observed.getTime() + 5_000).toISOString(); + await expect(recoveryProcessMayBeAlive({ pid: live.pid!, startedAt: inconsistent })).resolves.toBe(true); } finally { const exited = once(live, "exit"); live.kill("SIGKILL"); diff --git a/server/src/services/native-local-process-stop.test.ts b/server/src/services/native-local-process-stop.test.ts new file mode 100644 index 00000000000..1b649ba7c10 --- /dev/null +++ b/server/src/services/native-local-process-stop.test.ts @@ -0,0 +1,71 @@ +import { randomUUID } from "node:crypto"; +import { hostname } from "node:os"; +import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import { agents, companies, createDb, heartbeatRuns } from "@paperclipai/db"; +import { getEmbeddedPostgresTestSupport, startEmbeddedPostgresTestDatabase } from "../__tests__/helpers/embedded-postgres.js"; +import { currentProcessSpaceHash, currentProcessStartedAtMs } from "./controller-process-identity.js"; +import { appendHeartbeatRunEvent } from "./heartbeat-run-events.js"; +import { PROCESS_IDENTITY_RECORDED, recordedLocalExecutionStopped } from "./native-local-process-stop.js"; + +const support = await getEmbeddedPostgresTestSupport(); + +// Phase 2: a bound local execution is judged by the shared prover +// (proveOwnerGone), in the PID space the row recorded. +(support.supported ? describe : describe.skip)("recordedLocalExecutionStopped", () => { + let database: Awaited>; + let db: ReturnType; + let companyId: string; + let agentId: string; + beforeAll(async () => { + database = await startEmbeddedPostgresTestDatabase("native-local-stop-"); + db = createDb(database.connectionString, { maxConnections: 2 }); + companyId = randomUUID(); agentId = randomUUID(); + await db.insert(companies).values({ id: companyId, name: "Local stop", issuePrefix: `L${companyId.slice(0, 7)}` }); + await db.insert(agents).values({ id: agentId, companyId, name: "Agent", role: "general", adapterType: "codex_local", + status: "idle" }); + }, 60_000); + afterAll(async () => { await db?.$client.end({ timeout: 1 }); await database?.cleanup(); }); + + /** A failed run with its process identity bound by the launch event. */ + async function execution(input: { pid: number; startedAt: Date; processHostHash: string | null }) { + const [run] = await db.insert(heartbeatRuns).values({ companyId, agentId, status: "failed", runtimeMode: "native", + processPid: input.pid, processGroupId: input.pid, processStartedAt: input.startedAt, + processHostHash: input.processHostHash }).returning(); + await appendHeartbeatRunEvent(db, { companyId, runId: run!.id, agentId, eventType: PROCESS_IDENTITY_RECORDED, + payload: { targetKind: "local", hostname: hostname(), processPid: input.pid, processGroupId: input.pid, + processStartedAt: input.startedAt.toISOString() } }); + return run!; + } + const deadPid = 2_000_000_001; + const minuteAgo = () => new Date(Date.now() - 60_000); + const ownStart = async () => new Date((await currentProcessStartedAtMs())!); + + it("is stopped when its dead PID was recorded in this PID space", async () => { + expect(await recordedLocalExecutionStopped(db, await execution({ pid: deadPid, startedAt: minuteAgo(), + processHostHash: currentProcessSpaceHash() }))).toBe(true); + }); + + it("is never stopped by a dead PID recorded in another PID space", async () => { + expect(await recordedLocalExecutionStopped(db, await execution({ pid: deadPid, startedAt: minuteAgo(), + processHostHash: "another-pid-space" }))).toBe(false); + }); + + it("keeps the host-bound proof for a row saved before its PID space", async () => { + expect(await recordedLocalExecutionStopped(db, await execution({ pid: deadPid, startedAt: minuteAgo(), + processHostHash: null }))).toBe(true); + }); + + it("is not stopped while the same process is alive", async () => { + expect(await recordedLocalExecutionStopped(db, await execution({ pid: process.pid, startedAt: await ownStart(), + processHostHash: currentProcessSpaceHash() }))).toBe(false); + }); + + it("is stopped when a later process reused its PID and leads the group", async () => { + // This test process started definitely after the recorded execution. + const recorded = new Date((await ownStart()).getTime() - 5_000); + // A group with this id is either empty or led by this later process, so + // it cannot hold the recorded execution's children. + expect(await recordedLocalExecutionStopped(db, await execution({ pid: process.pid, startedAt: recorded, + processHostHash: currentProcessSpaceHash() }))).toBe(true); + }); +}); diff --git a/server/src/services/native-local-process-stop.ts b/server/src/services/native-local-process-stop.ts index 5999ea523b6..47f4055bf11 100644 --- a/server/src/services/native-local-process-stop.ts +++ b/server/src/services/native-local-process-stop.ts @@ -7,6 +7,8 @@ import { DELIVERY_QUEUES, notifyDeliveryWork } from "./delivery-work-notificatio import { nativeSessionProviderEvidence } from "./native-runtime/native-session-resume.js"; import { hasRemoteTerminationReceipt } from "./remote-execution-termination.js"; import os from "node:os"; +import { currentProcessSpaceHash } from "./controller-process-identity.js"; +import { proveOwnerGone } from "./owner-proof.js"; export const PROCESS_START_REQUESTED = "native.process_start_requested"; export const PROCESS_IDENTITY_RECORDED = "native.process_identity_recorded"; @@ -26,7 +28,7 @@ function absent(pid: number): boolean { /** A fresh workspace is not process containment. Bind an OS observation to * the original host, process birth, and launch event before accepting it. */ export async function recordedLocalExecutionStopped(db: Db, run: Pick) { + "id" | "companyId" | "processPid" | "processGroupId" | "processStartedAt"> & { processHostHash?: string | null }) { const [identity] = await db.select({ eventType: heartbeatRunEvents.eventType, payload: heartbeatRunEvents.payload }).from(heartbeatRunEvents).where(and( eq(heartbeatRunEvents.companyId, run.companyId), eq(heartbeatRunEvents.runId, run.id), inArray(heartbeatRunEvents.eventType, [PROCESS_START_REQUESTED, PROCESS_IDENTITY_RECORDED]), @@ -36,8 +38,15 @@ export async function recordedLocalExecutionStopped(db: Db, run: Pick { } }); + // Phase 2: run processes and controllers are judged by the shared prover. + it("never releases on a dead PID recorded in another PID space", async () => { + const { runId } = await nativeRun({ status: "failed", errorCode: "adapter_failed", + coordinator: { leaseOwner: `default:resume:${randomUUID()}`, leaseExpiresAt: new Date(0) } }); + await db.update(heartbeatRuns).set({ processPid: 2_000_000_001, processGroupId: null, + processStartedAt: new Date(Date.now() - 60_000), processHostHash: "another-pid-space" }) + .where(eq(heartbeatRuns.id, runId)); + await expect(releaseAbandonedNativeOwners(db)).resolves.toBe(0); + expect(await coordinator(runId)).toMatchObject({ phase: "observed" }); + await db.update(heartbeatRuns).set({ processHostHash: currentProcessSpaceHash() }).where(eq(heartbeatRuns.id, runId)); + await expect(releaseAbandonedNativeOwners(db)).resolves.toBe(1); + expect(await db.select().from(activityLog).where(and(eq(activityLog.runId, runId), + eq(activityLog.action, "legacy_host_unknown_dead_pid_proof")))).toEqual([]); + }); + + it("keeps the one-release fallback for a run process saved before its PID space, and records it", async () => { + const { runId } = await nativeRun({ status: "failed", errorCode: "adapter_failed", + coordinator: { leaseOwner: `default:resume:${randomUUID()}`, leaseExpiresAt: new Date(0) } }); + await db.update(heartbeatRuns).set({ processPid: 2_000_000_001, processGroupId: null, + processStartedAt: new Date(Date.now() - 60_000), processHostHash: null }).where(eq(heartbeatRuns.id, runId)); + await expect(releaseAbandonedNativeOwners(db)).resolves.toBe(1); + const proofs = await db.select().from(activityLog).where(and(eq(activityLog.runId, runId), + eq(activityLog.action, "legacy_host_unknown_dead_pid_proof"))); + expect(proofs.map(proof => proof.details)).toEqual([{ reason: "pid_dead", source: "native_failed_run_process" }]); + }); + // `ps -o lstart` on macOS reports whole seconds, while a recorded start can // carry milliseconds. A sub-second gap must not be read as a recycled PID. it.each(["controller", "run process"])("keeps the owner when the live %s start time differs by under a second", async (holder) => { diff --git a/server/src/services/native-runtime/native-failed-run-owner-release.ts b/server/src/services/native-runtime/native-failed-run-owner-release.ts index 3ea34cda26b..ef7ddff6456 100644 --- a/server/src/services/native-runtime/native-failed-run-owner-release.ts +++ b/server/src/services/native-runtime/native-failed-run-owner-release.ts @@ -2,15 +2,15 @@ import { and, asc, eq, gt, inArray, isNotNull, isNull, lte, or, sql } from "driz import { environmentLeases, heartbeatRuns, issues, nativeRunFinalizations, nativeRunResults, type Db } from "@paperclipai/db"; import { logger } from "../../middleware/logger.js"; import { adapterExecutionControls } from "../adapter-execution-control.js"; -import { processExists, processStartsDefinitelyDiffer } from "../controller-process-identity.js"; +import { proveOwnerGone, type LocalOwnerStamp } from "../owner-proof.js"; import { logActivity, publishActivity, type ActivityPublication } from "../activity-log.js"; import { DELIVERY_QUEUES, notifyDeliveryWork } from "../delivery-work-notifications.js"; import { notifyExecutionRunWork } from "../execution-control-notifications.js"; -import { readProcessStartedAt } from "../hot-restart.js"; import { notifyNativeFinalizationWork } from "../native-finalization-notifications.js"; import { LEGACY_WORKSPACE_RECOVERY_KEY } from "../workspace-restore-recovery-state.js"; import { activeNativeExecutionRunIds } from "./native-active-runs.js"; -import { isCurrentNativeControllerProcess } from "./native-restart-recovery.js"; +import { isCurrentNativeControllerProcess, recordLegacyHostFallbackProof } from "./native-restart-recovery.js"; +import { nativeControllerOwnerStamp, runProcessOwnerStamp } from "./native-owner-stamps.js"; /** * A native run can fail outside the native session executor: environment lease @@ -85,51 +85,48 @@ export async function nextAbandonedNativeOwnerReleaseAt(db: Db): Promise { - if (!pid || processAbsent(pid)) return false; - if (!startedAt) return true; - const observed = await readProcessStartedAt(pid).catch(() => null); - return !processStartsDefinitelyDiffer(startedAt, observed); +/** True unless the shared prover proves the stamped owner gone. Only a `gone` + * verdict releases: a live, unprovable or foreign-space owner may be live. */ +async function ownerMayBeLive(stamp: LocalOwnerStamp, source: string, fallbacks: FallbackUse[]): Promise { + const proof = await proveOwnerGone(stamp); + if (proof.verdict !== "gone") return true; + if (proof.legacyHostFallback) fallbacks.push({ reason: proof.reason, source }); + return false; } /** * The run-level process evidence `agentExecutionsHaveStopped` requires. This - * process answers for itself from its in-memory execution registries. + * process answers for itself from its in-memory execution registries; a + * recorded adapter process (PID and group) is judged by the shared prover. */ -async function runProcessMayBeLive(run: Run, now: Date): Promise { +async function runProcessMayBeLive(run: Run, now: Date, fallbacks: FallbackUse[]): Promise { if (adapterExecutionControls.has(run.id) || activeNativeExecutionRunIds.has(run.id)) return true; if (run.controllerLeaseExpiresAt && run.controllerLeaseExpiresAt > now) return true; - // Same process identity rule as `agentExecutionsHaveStopped`: a recorded PID - // that now has another start time was recycled, and so was its own group. - let processAlive = Boolean(run.processPid && !processAbsent(run.processPid)); - let groupAlive = Boolean(run.processGroupId && !processAbsent(-run.processGroupId)); - if (processAlive && run.processStartedAt) { - const observed = await readProcessStartedAt(run.processPid!).catch(() => null); - if (processStartsDefinitelyDiffer(run.processStartedAt, observed)) { - processAlive = false; - if (run.processGroupId === run.processPid) groupAlive = false; - } - } - return processAlive || groupAlive; + if (!run.processPid && !run.processGroupId) return false; + return ownerMayBeLive(runProcessOwnerStamp(run, "native_failed_run_process", { legacyFallback: true }), + "native_failed_run_process", fallbacks); } /** An expired owner's recorded controller, unless this process is that controller. */ -async function controllerMayBeLive(coordinator: Coordinator): Promise { +async function controllerMayBeLive(coordinator: Coordinator, fallbacks: FallbackUse[]): Promise { if (isCurrentNativeControllerProcess(coordinator)) return false; // A previous boot of this process can share its PID, but cannot be its controller. if (coordinator.controllerPid === process.pid) return false; - return recordedProcessMayBeLive(coordinator.controllerPid, coordinator.controllerProcessStartedAt); + if (!coordinator.controllerPid) return false; + // Not this boot (checked above), so the prover must not treat its boot id as live. + return ownerMayBeLive({ ...nativeControllerOwnerStamp(coordinator, "native_failed_run_controller"), bootId: null }, + "native_failed_run_controller", fallbacks); } async function settleFailedRunOwner(db: Db, release: Release, now: Date): Promise { const publications: ActivityPublication[] = []; const settled = await db.transaction(async transaction => { const tx = transaction as unknown as Db; + const fallbacks: FallbackUse[] = []; await tx.execute(sql`select set_config('statement_timeout', '15000', true), set_config('lock_timeout', '1000', true)`); const [binding] = await tx.select({ companyId: nativeRunFinalizations.companyId, issueId: nativeRunFinalizations.issueId }) .from(nativeRunFinalizations).where(eq(nativeRunFinalizations.runId, release.runId)); @@ -151,8 +148,9 @@ async function settleFailedRunOwner(db: Db, release: Release, now: Date): Promis || run.resultJson?.[LEGACY_WORKSPACE_RECOVERY_KEY] != null) return false; const [heldElsewhere] = await tx.select({ id: heartbeatRuns.id }).from(heartbeatRuns) .where(and(eq(heartbeatRuns.id, run.id), or(unsettledRunEnvironmentLease, pendingNativeStop))).limit(1); - if (heldElsewhere || await runProcessMayBeLive(run, now) || await controllerMayBeLive(coordinator)) return false; - } else if (await runProcessMayBeLive(run, now)) { + if (heldElsewhere || await runProcessMayBeLive(run, now, fallbacks) + || await controllerMayBeLive(coordinator, fallbacks)) return false; + } else if (await runProcessMayBeLive(run, now, fallbacks)) { // The claim never reached the executor, but the run may still own a local process. return false; } @@ -194,6 +192,9 @@ async function settleFailedRunOwner(db: Db, release: Release, now: Date): Promis if (!updated) return false; await tx.update(heartbeatRuns).set({ nativePhase: "terminal_failure", nativePhaseUpdatedAt: now, updatedAt: now }) .where(eq(heartbeatRuns.id, run.id)); + for (const use of fallbacks) { + await recordLegacyHostFallbackProof(tx, run, { allowed: true, reason: use.reason, legacyHostFallback: true }, use.source); + } await logActivity(tx, { companyId: run.companyId, actorType: "system", actorId: ACTOR_ID, action: "native.failed_run_owner_released", entityType: "heartbeat_run", entityId: run.id, diff --git a/server/src/services/native-runtime/native-owner-stamps.ts b/server/src/services/native-runtime/native-owner-stamps.ts new file mode 100644 index 00000000000..8bd2bd9dfdc --- /dev/null +++ b/server/src/services/native-runtime/native-owner-stamps.ts @@ -0,0 +1,47 @@ +import type { heartbeatRuns, nativeRunFinalizations } from "@paperclipai/db"; +import type { LocalOwnerStamp } from "../owner-proof.js"; + +/** + * Owner stamps for the native records that `proveOwnerGone` judges (plan + * Phase 2). The ownership registry and the native recovery paths build their + * stamps here, so one record has one proof. + */ + +type ControllerRow = Pick & { + controllerHostHash?: string | null; + controllerGeneration?: number | null; +}; + +/** NRF records the controller's boot, PID, start time and, since #19868, its + * PID space (`controller_host_hash`). Rows saved before that keep the + * one-release local proof on single-host servers (`processSpaceUnrecorded`, + * recorded as `legacy_host_unknown_dead_pid_proof`, #19883). */ +export function nativeControllerOwnerStamp(row: ControllerRow, scope: string): LocalOwnerStamp { + return { + kind: "local", scope, generation: row.controllerGeneration ?? null, host: null, + processSpace: row.controllerHostHash ?? null, + processSpaceUnrecorded: row.controllerHostHash == null, + bootId: row.controllerBootId, pid: row.controllerPid, + processStartedAt: row.controllerProcessStartedAt?.toISOString() ?? null, processGroupId: null, + }; +} + +type RunProcessRow = Pick & { + processHostHash?: string | null; +}; + +/** heartbeat_runs records the adapter child's PID, group, start time and, + * since #19871, its PID space (`process_host_hash`, or "remote" for a sandbox + * PID, which a local probe never judges). With `legacyFallback`, a row saved + * before that column keeps the one-release local proof; without it, such a row + * is unprovable. */ +export function runProcessOwnerStamp(row: RunProcessRow, scope: string, + options: { legacyFallback?: boolean } = {}): LocalOwnerStamp { + return { + kind: "local", scope, generation: null, host: null, processSpace: row.processHostHash ?? null, + ...(options.legacyFallback ? { processSpaceUnrecorded: row.processHostHash == null } : {}), + bootId: null, pid: row.processPid, processStartedAt: row.processStartedAt?.toISOString() ?? null, + processGroupId: row.processGroupId, + }; +} diff --git a/server/src/services/native-runtime/native-restart-recovery.test.ts b/server/src/services/native-runtime/native-restart-recovery.test.ts index 2e3d55ea235..3f88219608a 100644 --- a/server/src/services/native-runtime/native-restart-recovery.test.ts +++ b/server/src/services/native-runtime/native-restart-recovery.test.ts @@ -270,6 +270,33 @@ describe("native controller takeover fencing", () => { }); }); + // Phase 2: the local branch is the shared prover (proveOwnerGone). + it("accepts a reused PID only when the live process started definitely later", async () => { + const live = { isProcessAlive: () => true }; + await expect(evaluateNativeControllerTakeover({ owner: owner(), now, ...live, + readProcessStartedAt: async () => new Date(recordedStart.getTime() + 5_000) })) + .resolves.toEqual({ allowed: true, reason: "controller_pid_recycled" }); + // An observed start before the recorded one is an inconsistent stamp, + // which the previous private proof accepted as recycled. + await expect(evaluateNativeControllerTakeover({ owner: owner(), now, ...live, + readProcessStartedAt: async () => new Date(recordedStart.getTime() - 5_000) })) + .resolves.toEqual({ allowed: false, reason: "controller_pid_start_inconsistent" }); + // Within the 1 s precision window it is the same live controller. + await expect(evaluateNativeControllerTakeover({ owner: owner(), now, ...live, + readProcessStartedAt: async () => new Date(recordedStart.getTime() + 500) })) + .resolves.toEqual({ allowed: false, reason: "controller_still_alive" }); + await expect(evaluateNativeControllerTakeover({ owner: owner(), now, ...live, + readProcessStartedAt: async () => null })) + .resolves.toEqual({ allowed: false, reason: "controller_still_alive" }); + }); + + it("judges a controller recorded in this PID space, and keeps the operator attestation", async () => { + await expect(evaluateNativeControllerTakeover({ owner: { ...owner(), controllerHostHash: currentProcessSpaceHash() }, + now, isProcessAlive: () => false })).resolves.toEqual({ allowed: true, reason: "controller_process_dead" }); + await expect(evaluateNativeControllerTakeover({ owner: { ...owner(), controllerHostHash: "operator_attested" }, + now, isProcessAlive: () => true })).resolves.toEqual({ allowed: true, reason: "operator_attested_controller_stopped" }); + }); + it.each([ ["another host", "another-host-pid-space"], ["an unreadable PID space", "unknown"], diff --git a/server/src/services/native-runtime/native-restart-recovery.ts b/server/src/services/native-runtime/native-restart-recovery.ts index b780294b4e2..a5c799ff64a 100644 --- a/server/src/services/native-runtime/native-restart-recovery.ts +++ b/server/src/services/native-runtime/native-restart-recovery.ts @@ -36,10 +36,10 @@ import { import { REMOTE_CONTROLLER_LEASE_MS } from "./native-runner-recovery-contract.js"; import { PROVIDER_ATTEMPT_CREDIT_GENERATION_LIMIT, providerAttemptChargesBudget, providerAttemptWindowStart, readProviderAttemptOutcome } from "./provider-attempt-outcome.js"; import { readProcessStartedAt } from "../hot-restart.js"; -import { ephemeralServerEnabled } from "../ephemeral-server-mode.js"; +import { currentProcessProbe, proveOwnerGone, type ProcessProbe } from "../owner-proof.js"; import { persistActivity } from "../activity-log.js"; import { OPERATOR_ATTESTED_CONTROLLER_HOST, requestNativeControllerStopConfirmation } from "./native-controller-host-recovery.js"; -import { currentControllerBootId, currentProcessSpaceHash, currentProcessStartedAtMs, processStartsDefinitelyDiffer, sameProcessSpace } from "../controller-process-identity.js"; +import { currentControllerBootId, currentProcessSpaceHash, currentProcessStartedAtMs, processStartsDefinitelyDiffer } from "../controller-process-identity.js"; import { getServerProcessStartedAt } from "../../server-info.js"; import { redactSensitiveText } from "../../redaction.js"; import { isUniqueViolation } from "../../db-errors.js"; @@ -418,8 +418,6 @@ export async function evaluateNativeControllerTakeover(input: { ? { allowed: false, reason: "remote_controller_lease_live" } : { allowed: true, reason: "remote_controller_lease_released_or_expired" }; } - const isAlive = input.isProcessAlive ?? processIsAlive; - const readStartedAt = input.readProcessStartedAt ?? observedProcessStart; if ((!owner.leaseOwner || !owner.leaseExpiresAt) && !input.requireStoppedController) { return { allowed: true, reason: "unowned" }; } @@ -440,21 +438,32 @@ export async function evaluateNativeControllerTakeover(input: { if (recordsHost && owner.controllerHostHash === OPERATOR_ATTESTED_CONTROLLER_HOST) { return { allowed: true, reason: "operator_attested_controller_stopped" }; } - // A PID can only be judged in the PID space that recorded it. A controller - // on another host or namespace (or an unreadable local space) is never - // proven gone by a local probe; its lease or fence decides instead. - if (owner.controllerHostHash && !sameProcessSpace(owner.controllerHostHash)) { - return { allowed: false, reason: "controller_host_unprovable" }; - } - // A row saved before the host column existed. An ephemeral (multi-host) - // server can never assume it was written here; a board operator decides. - if (recordsHost && owner.controllerHostHash == null && ephemeralServerEnabled()) { - return { allowed: false, reason: "controller_host_unrecorded" }; - } - // Single-host servers keep the previous local proof for one release, and - // every use is recorded so the release that removes it can check for rows. - const fallback = recordsHost && owner.controllerHostHash == null ? { legacyHostFallback: true as const } : {}; - if (!isAlive(priorPid)) { + // The local branch is the shared prover (plan Phase 2): a PID is judged + // only in the PID space that recorded it, a reused PID must have started + // definitely later, and EPERM is alive. A coordinator row saved before the + // host column keeps the one-release local proof on single-host servers + // (`processSpaceUnrecorded`; each use is recorded by the caller as + // LEGACY_HOST_FALLBACK_EVENT), and an ephemeral server never judges it. + // Owners built from receipts that never stored a PID space are judged in + // this PID space: their callers checked the recorded host first. + const base = currentProcessProbe(); + const probe: ProcessProbe = { + ...base, + // No platform-deployment shortcut: this stamp records no worker. + deployment: null, + pidExists: input.isProcessAlive ?? base.pidExists, + readStartedAt: input.readProcessStartedAt ?? base.readStartedAt, + }; + const proof = await proveOwnerGone({ + kind: "local", scope: "native_controller", generation: null, host: null, + processSpace: recordsHost ? owner.controllerHostHash ?? null : probe.processSpace, + processSpaceUnrecorded: recordsHost && owner.controllerHostHash == null, + bootId: null, pid: priorPid, processStartedAt: owner.controllerProcessStartedAt?.toISOString() ?? null, + processGroupId: null, + }, probe); + const fallback = proof.legacyHostFallback ? { legacyHostFallback: true as const } : {}; + if (proof.verdict === "gone") { + if (proof.reason === "pid_reused") return { allowed: true, reason: "controller_pid_recycled", ...fallback }; return { allowed: true, reason: @@ -464,18 +473,14 @@ export async function evaluateNativeControllerTakeover(input: { ...fallback, }; } - - const observedStartedAt = await readStartedAt(priorPid); - if ( - owner.controllerProcessStartedAt && - observedStartedAt && - definitivelyDifferentProcessStart( - owner.controllerProcessStartedAt, - observedStartedAt, - ) - ) { - return { allowed: true, reason: "controller_pid_recycled", ...fallback }; + if (proof.reason === "process_space_unrecorded") return { allowed: false, reason: "controller_host_unrecorded" }; + if (proof.reason === "different_process_space" || proof.reason === "unknown_process_space" + || proof.reason === "different_host") { + return { allowed: false, reason: "controller_host_unprovable" }; } + // A live PID observed to start before the recorded controller is an + // inconsistent stamp, never proof of reuse. + if (proof.reason === "pid_start_inconsistent") return { allowed: false, reason: "controller_pid_start_inconsistent" }; const coordinated = input.coordinatedPreviousController; if ( diff --git a/server/src/services/native-runtime/native-session-executor.ts b/server/src/services/native-runtime/native-session-executor.ts index d7e3759d66f..4b5d5ecd027 100644 --- a/server/src/services/native-runtime/native-session-executor.ts +++ b/server/src/services/native-runtime/native-session-executor.ts @@ -302,8 +302,8 @@ import { noteNativeRunnerEvidence } from "./native-runner-evidence-signal.js"; import { removeNativeHarnessBackup } from "./native-harness-backup-cleanup.js"; import { registerLiveRunnerGoalController } from "../runner-goal-control-broker.js"; import { applyRunnerGoalPrpEvent } from "../runner-goals.js"; -import { readProcessStartedAt } from "../hot-restart.js"; -import { processExists, processStartsDefinitelyDiffer } from "../controller-process-identity.js"; +import { currentProcessSpaceHash } from "../controller-process-identity.js"; +import { proveOwnerGone } from "../owner-proof.js"; import { NativeRunnerOwnershipUnverifiedError, isNativeRunnerOwnershipHeld, @@ -591,32 +591,33 @@ const nativeRuntimeRequestResolutions = new Map< NativeRuntimeRequestResolution >(); +/** The shared prover's verdict on a recovered local runner, recorded on this + * host by an earlier boot (no boot id, so this process is never assumed). */ +function proveRecoveryProcess(input: { pid: number; startedAt: string }) { + return proveOwnerGone({ kind: "local", scope: "native_recovery_runner", generation: null, host: null, + processSpace: currentProcessSpaceHash(), bootId: null, pid: input.pid, processStartedAt: input.startedAt, + processGroupId: null }); +} + /** - * True when the recorded runner still owns its PID. A start time within the - * shared precision window is the same process: `ps -o lstart` on macOS has - * whole-second precision, so an exact match would call a live runner gone. + * False only when the shared prover proves the recorded runner gone (its PID + * is dead or reused by a later process). The runner treats false as exit + * evidence, so an unreadable or inconsistent start time stays alive. */ -export async function verifiedRecoveryProcessIsAlive(input: { +export async function recoveryProcessMayBeAlive(input: { pid: number; startedAt: string; }): Promise { - if (!processExists(input.pid)) return false; - try { - const observed = await readProcessStartedAt(input.pid); - return ( - observed !== null && - !processStartsDefinitelyDiffer(input.startedAt, observed) - ); - } catch { - return false; - } + return (await proveRecoveryProcess(input)).verdict !== "gone"; } async function signalVerifiedRecoveryProcess( input: { pid: number; startedAt: string }, signal: NodeJS.Signals, ): Promise { - if (!(await verifiedRecoveryProcessIsAlive(input))) return false; + // Signal only the proven same process (start time within the shared + // precision window), never an ambiguous or reused PID. + if ((await proveRecoveryProcess(input)).verdict !== "alive") return false; try { return process.kill(input.pid, signal); } catch { @@ -5467,7 +5468,7 @@ const RETIREMENT_REASON_NAMES = { recoveryState: new Set(["blocked", "resuming_session", "awaiting_evidence", "awaiting_runner_reattach"]), controllerReason: new Set(["controller_still_alive", "coordinated_controller_still_alive", "remote_controller_lease_live", "expired_lease_without_controller_identity", "live_legacy_lease_without_controller_identity", - "controller_host_unprovable", "controller_host_unrecorded"]), + "controller_host_unprovable", "controller_host_unrecorded", "controller_pid_start_inconsistent"]), sandboxOutcome: new Set(["created", "resumed", "replacement"]), } as const; @@ -14664,7 +14665,7 @@ async function createRunnerdBackendWithinSessionClaim( target, binding: input.execution.binding, trace: input.trace, onLog: input.onLog, }).controlPlaneRegistration, adoptExistingRunner: remoteDot?.adoptExistingRunner ?? (recoveredProcess ? { ...recoveredProcess, - isAlive: () => verifiedRecoveryProcessIsAlive(recoveredProcess), + isAlive: () => recoveryProcessMayBeAlive(recoveredProcess), signal: (signal: NodeJS.Signals = "SIGTERM") => signalVerifiedRecoveryProcess(recoveredProcess, signal) } : undefined), }; const backendOptions = { @@ -16012,7 +16013,7 @@ async function createRunnerdBackendWithinSessionClaim( : target.kind === "local" && localRecoveryProcess ? { ...localRecoveryProcess, - isAlive: () => verifiedRecoveryProcessIsAlive(localRecoveryProcess), + isAlive: () => recoveryProcessMayBeAlive(localRecoveryProcess), signal: (signal: NodeJS.Signals) => signalVerifiedRecoveryProcess(localRecoveryProcess, signal), } diff --git a/server/src/services/quarantined-task-retry.test.ts b/server/src/services/quarantined-task-retry.test.ts index f08669a6e84..f87d720da71 100644 --- a/server/src/services/quarantined-task-retry.test.ts +++ b/server/src/services/quarantined-task-retry.test.ts @@ -17,6 +17,7 @@ import { getExecutionBlocker } from "./execution-blocker.js"; import { readFreshWorkspaceReadiness } from "./fresh-workspace-continuation.js"; import { instanceSettingsService } from "./instance-settings.js"; import { PROCESS_IDENTITY_RECORDED, PROCESS_START_REQUESTED } from "./native-local-process-stop.js"; +import { currentProcessStartedAtMs } from "./controller-process-identity.js"; import { nativeSha256 } from "./native-runtime/canonical.js"; import { verifyStoppedNativeSessionForContinuation } from "./native-runtime/native-session-executor.js"; import { requestTaskRetry } from "./task-retry.js"; @@ -185,7 +186,10 @@ describe("task Retry after inherited native cleanup quarantine", () => { })); } if (mode === "worker_alive") { - await db.update(heartbeatRuns).set({ processPid: process.pid, processGroupId: process.pid }) + // This test process, with its real start time: the shared prover then + // sees the same live worker, not a reused PID. + const startedAt = new Date((await currentProcessStartedAtMs())!); + await db.update(heartbeatRuns).set({ processPid: process.pid, processGroupId: process.pid, processStartedAt: startedAt }) .where(eq(heartbeatRuns.companyId, f.companyId)); const [owner] = await db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, f.ownerId)); await appendHeartbeatRunEvent(db, { companyId: f.companyId, runId: f.ownerId, agentId: f.agentId, diff --git a/server/src/services/recovery/ownership-registry.ts b/server/src/services/recovery/ownership-registry.ts index 6bc510d34c4..848c0992fdf 100644 --- a/server/src/services/recovery/ownership-registry.ts +++ b/server/src/services/recovery/ownership-registry.ts @@ -33,6 +33,7 @@ import { } from "../native-runtime/native-accepted-result-lease-settlement.js"; import { settleClaimedWakeRequest } from "./wake-request-claimed-settlement.js"; import { settleAbandonedWorkspaceFinalizeAttempt } from "./workspace-finalize-attempt-settlement.js"; +import { nativeControllerOwnerStamp, runProcessOwnerStamp } from "../native-runtime/native-owner-stamps.js"; import { legacyControllerOwnerStamp, settleLegacyControllerLease } from "../legacy-controller-lease.js"; import { nativeLeaseHolderStamp, @@ -163,20 +164,8 @@ const runOwner = (runId: string | null | undefined): RecordOwner => runId ? { ki type NrfRow = typeof nativeRunFinalizations.$inferSelect; -/** NRF records the controller's boot, PID, start time and, since #19868, its - * PID space (`controller_host_hash`). Rows written before that have no space - * and stay unprovable for a foreign boot (inventory finding 2). */ -function nrfStamp(row: NrfRow, scope: string): LocalOwnerStamp { - return { - kind: "local", scope, generation: row.controllerGeneration ?? null, host: null, - processSpace: row.controllerHostHash ?? null, - // Rows saved before #19868 keep the one-release local proof on single-host - // servers (`legacy_host_unknown_dead_pid_proof`, #19883). - processSpaceUnrecorded: row.controllerHostHash == null, - bootId: row.controllerBootId, pid: row.controllerPid, - processStartedAt: row.controllerProcessStartedAt?.toISOString() ?? null, processGroupId: null, - }; -} +/** The NRF controller stamp, shared with native recovery (native-owner-stamps.ts). */ +const nrfStamp = (row: NrfRow, scope: string): LocalOwnerStamp => nativeControllerOwnerStamp(row, scope); function nrfKind(id: string, title: string, obligation: Obligation, where: SQL | undefined, owner: (row: NrfRow) => RecordOwner = row => ({ kind: "process", stamp: nrfStamp(row, id) }), @@ -256,16 +245,9 @@ function runKind(id: string, title: string, obligation: Obligation, where: SQL | }; } -/** heartbeat_runs records the adapter child's PID, start time and, since - * #19871, its PID space (`process_host_hash`). Older rows have no space and - * stay unprovable. */ -function runProcessStamp(row: RunRow, scope: string): LocalOwnerStamp { - return { - kind: "local", scope, generation: null, host: null, processSpace: row.processHostHash ?? null, bootId: null, - pid: row.processPid, processStartedAt: row.processStartedAt?.toISOString() ?? null, - processGroupId: row.processGroupId, - }; -} +/** The adapter child's stamp (native-owner-stamps.ts). Rows saved before + * `process_host_hash` stay unprovable here. */ +const runProcessStamp = (row: RunRow, scope: string): LocalOwnerStamp => runProcessOwnerStamp(row, scope); const legacyControllerLeaseWhere = and(eq(heartbeatRuns.runtimeMode, "legacy"), eq(heartbeatRuns.status, "running"), isNotNull(heartbeatRuns.controllerBootId), sql`${heartbeatRuns.controllerLeaseExpiresAt} > now()`); From 1444b0d1513316e7d7713dae1b14b71db7361677 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Sun, 8 Nov 2026 02:52:01 -0600 Subject: [PATCH 7/9] fix(chat): keep every listed task linked and send long lists to a document (#19994) 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. > - People ask agents in chat to show company work, for example all open tasks in a project. > - The chat links each full task identifier (such as `RUN-12`) to its task. The chat guidance tells agents how to write task links. > - The guidance says nothing about long lists. An agent that must list 501 tasks can compact them into ranges such as `RUN-76–82`. Only the first task of each range then gets a link. > - The person then cannot open most of the tasks from the reply. > - This pull request adds one guidance paragraph: write each identifier in full, and put a list that is too long for the reply in a document on the conversation's task, linked from the reply. > - The benefit is that every task in a long answer keeps a source link. ## Linked Issues or Issue Description **What happened** A chat agent was asked to show all open work in a project with 501 open tasks, grouped by owner. Its reply had correct groups and counts. It wrote the identifiers as ranges, for example `RUN-1–2, RUN-76–82`. Chat linked only the first identifier of each range. 161 of the 501 tasks had no link. The agent's reasoning said it chose ranges because 501 separate links "would be enormous". **Expected behavior** Every task that a chat reply lists has a source link. When the list is too long for a reply, the agent puts the complete list in a document on the conversation's task and links that document from the reply. **Steps to reproduce** 1. Create a project with more than 500 open tasks. 2. In chat, ask the CEO agent: "Show me all open work in this project, grouped by owner, with blocked items first." 3. Read the reply. Identifiers written as ranges have a link only on the first task of each range. ## What Changed - `server/src/services/agent-conversations.ts`: the chat directive has a new paragraph after the agent-profile link paragraph. It tells agents to write each task identifier in full, says that a range links only its first task, and says to save a list that is too long for the reply as a conversation document linked as `//issues/#document-`, with a short summary and counts in the reply. - `server/src/__tests__/agent-conversations.test.ts`: a new test checks the three parts of the paragraph. ## Verification - `cd server && npx vitest run src/__tests__/agent-conversations.test.ts`: 47 passed. The new test fails without the guidance change (1 failed, 46 passed). - The `#document-` issue anchor is an existing UI route (`ui/src/lib/document-annotation-hash.ts`), and the server already builds it in `company-search.ts` and `feedback.ts`. - End-to-end: the `tasks.organize-open-work` chat eval runs this change on its case branch. ## Risks - Low risk. The change is guidance text only. There is no schema, API, or UI change. - The directive is slightly longer, so each chat turn has a few more prompt tokens. - An agent can still write a long inline list. The guidance does not force a document. > 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 - Anthropic Claude Opus 5.5 (`claude-opus-5-5`), agentic coding with tool use (shell, file edits, test runs). ## 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 --- server/src/__tests__/agent-conversations.test.ts | 6 ++++++ server/src/services/agent-conversations.ts | 2 ++ 2 files changed, 8 insertions(+) diff --git a/server/src/__tests__/agent-conversations.test.ts b/server/src/__tests__/agent-conversations.test.ts index ad3ee277347..fcc2b6f9656 100644 --- a/server/src/__tests__/agent-conversations.test.ts +++ b/server/src/__tests__/agent-conversations.test.ts @@ -1042,6 +1042,12 @@ describe("chat prompt policy", () => { expect(prompt).toContain("//company/settings/instance/profile"); }); + it("tells chat agents to keep every listed task linked, and where a long list goes", () => { + expect(AGENT_CHAT_DIRECTIVE).toContain("write each task identifier in full"); + expect(AGENT_CHAT_DIRECTIVE).toContain("a range such as RUN-12–14 links only its first task"); + expect(AGENT_CHAT_DIRECTIVE).toContain("//issues/#document-"); + }); + it("gives chat replies a usable route for both task and approval sources", () => { expect(AGENT_CHAT_DIRECTIVE).toContain("//issues/"); expect(AGENT_CHAT_DIRECTIVE).toContain("For approval links, use //approvals/"); diff --git a/server/src/services/agent-conversations.ts b/server/src/services/agent-conversations.ts index c601099276d..514799c35cf 100644 --- a/server/src/services/agent-conversations.ts +++ b/server/src/services/agent-conversations.ts @@ -114,6 +114,8 @@ For task links, use the company-prefixed relative path //issues//agents/ using the agent's returned urlKey or id. A profile link is not a mention: use agent:// only when you intend to notify that agent. +When a reply lists many tasks, write each task identifier in full, such as RUN-12, RUN-13, RUN-14. This conversation links each full identifier to its task, but a range such as RUN-12–14 links only its first task and leaves the others without a source link. When a complete list is too long to read in the reply, save it as a document on this conversation's task and link it, such as [Open work by owner](//issues/#document-), keeping a short summary with the counts in the reply. + A person's own display name and avatar belong to their account, and only they can change them in their profile settings at //company/settings/instance/profile. Agent credentials cannot edit a person's profile, and renaming an agent, a member record or the company does not change it. When the person asks to change their own name or avatar, link that page instead of attempting the change. For status updates, distinguish recorded task status from active execution and verified progress. Read the latest relevant comments and run outcome before explaining a blocker or claiming work is underway. Use the advertised API tools for another task's details when needed. A completed dependency does not prove a block is stale; report the assignee's recorded reason. Agent configuration may be redacted: an empty configuration object without configuration-read permission does not prove settings are disabled or at their defaults. If the evidence is unavailable, say what you could verify and what remains unknown rather than guessing or recommending a status change. From e7e870cfc484ae5a4ff1eda83305d28ad04e9b65 Mon Sep 17 00:00:00 2001 From: Nicky Leach Date: Sun, 8 Nov 2026 01:02:50 -0800 Subject: [PATCH 8/9] fix(native): decide warm and adopted Runner process reuse from one key (#20005) --- .../src/configured-environment.ts | 10 +- .../native-session-executor.test.ts | 92 +++++++++++++++++++ .../native-runtime/native-session-executor.ts | 88 ++++++------------ ...-runner-release-resume.integration.test.ts | 15 ++- server/src/vendor/paperclip-runner/index.ts | 1 + 5 files changed, 140 insertions(+), 66 deletions(-) diff --git a/packages/paperclip-runner/src/configured-environment.ts b/packages/paperclip-runner/src/configured-environment.ts index 475e62c3cc9..5cd30c0e765 100644 --- a/packages/paperclip-runner/src/configured-environment.ts +++ b/packages/paperclip-runner/src/configured-environment.ts @@ -25,12 +25,16 @@ export const GENERATED_RUNTIME_ENVIRONMENT_KEYS: ReadonlySet = new Set([ "PAPERCLIP_GITHUB_BROKER_TOKEN", "PAPERCLIP_GITHUB_BROKER_URL", "PAPERCLIP_GITHUB_BRIDGE_TOKEN", "PAPERCLIP_GITHUB_LAUNCHER_DIR", ]); -const RESERVED_KEYS = new Set([ - "PATH", "HOME", "USERPROFILE", "CODEX_HOME", "AGENT_HOME", "SHELL", "NODE_OPTIONS", "NODE_PATH", - "PYTHONPATH", "PYTHONHOME", "RUBYOPT", "RUBYLIB", "PERL5OPT", "PERL5LIB", "BASH_ENV", "ENV", "ZDOTDIR", "GH_CONFIG_DIR", +export const PROVIDER_CREDENTIAL_ENVIRONMENT_KEYS: readonly string[] = [ "OPENAI_API_KEY", "CODEX_API_KEY", "ANTHROPIC_API_KEY", "ANTHROPIC_AUTH_TOKEN", "CLAUDE_CODE_OAUTH_TOKEN", "AWS_BEARER_TOKEN_BEDROCK", "OPENROUTER_API_KEY", "XAI_API_KEY", "CURSOR_API_KEY", "CURSOR_AUTH_TOKEN", "COPILOT_GITHUB_TOKEN", "KIMI_API_KEY", "GH_TOKEN", "GITHUB_TOKEN", "GH_ENTERPRISE_TOKEN", "GITHUB_ENTERPRISE_TOKEN", "PAPERCLIP_GIT_TOKEN", +]; + +const RESERVED_KEYS = new Set([ + "PATH", "HOME", "USERPROFILE", "CODEX_HOME", "AGENT_HOME", "SHELL", "NODE_OPTIONS", "NODE_PATH", + "PYTHONPATH", "PYTHONHOME", "RUBYOPT", "RUBYLIB", "PERL5OPT", "PERL5LIB", "BASH_ENV", "ENV", "ZDOTDIR", "GH_CONFIG_DIR", + ...PROVIDER_CREDENTIAL_ENVIRONMENT_KEYS, ]); function eligibleKey(key: string): boolean { diff --git a/server/src/services/native-runtime/native-session-executor.test.ts b/server/src/services/native-runtime/native-session-executor.test.ts index c2fcfea2347..58eaaa7c44a 100644 --- a/server/src/services/native-runtime/native-session-executor.test.ts +++ b/server/src/services/native-runtime/native-session-executor.test.ts @@ -12938,6 +12938,98 @@ describe("native warm session supervision", () => { }, ); + it.each([ + { name: "unchanged configuration", reuse: true }, + { name: "unchanged provider key", firstEnvironment: { OPENAI_API_KEY: "fixture-key" }, secondEnvironment: { OPENAI_API_KEY: "fixture-key" }, reuse: true }, + { name: "rotated provider key", firstEnvironment: { OPENAI_API_KEY: "fixture-key" }, secondEnvironment: { OPENAI_API_KEY: "fixture-rotated-key" }, reuse: false }, + { name: "rotated Claude token", firstEnvironment: { CLAUDE_CODE_OAUTH_TOKEN: "fixture-token" }, secondEnvironment: { CLAUDE_CODE_OAUTH_TOKEN: "fixture-rotated-token" }, reuse: false }, + { name: "tool refresh", refreshTools: true, reuse: false }, + { name: "network access change", firstEnvironment: { PAPERCLIP_RUNNER_NETWORK_ACCESS: "disabled" }, secondEnvironment: { PAPERCLIP_RUNNER_NETWORK_ACCESS: "enabled" }, reuse: false }, + { name: "run-scoped environment broker", firstEnvironment: { PAPERCLIP_GITHUB_BROKER_TOKEN: "first-run-capability" }, secondEnvironment: { PAPERCLIP_GITHUB_BROKER_TOKEN: "second-run-capability" }, reuse: false }, + { name: "Core-hosted GitHub access", managedGitHub: true, reuse: true, adoptionReuse: false }, + ] as Array<{ name: string; firstEnvironment?: Record; secondEnvironment?: Record; + refreshTools?: boolean; managedGitHub?: boolean; reuse: boolean; adoptionReuse?: boolean }>)( + "decides warm and adopted process reuse from one key: $name", + async ({ name, firstEnvironment = {}, secondEnvironment = {}, refreshTools, managedGitHub = false, reuse, adoptionReuse = reuse }) => { + const decisions: Record = {}; + for (const path of ["warm", "adoption"] as const) { + githubAccess.create.mockReset().mockImplementation(async () => ({ env: {}, ready: true, duplexRequested: false, + activate: githubAccess.activate.mockReset().mockImplementation(() => vi.fn()), stop: githubAccess.stop.mockReset().mockResolvedValue(undefined) })); + const stateBase = await mkdtemp(join(tmpdir(), "paperclip-runnerd-reuse-key-")); + const previousStateDirectory = process.env.PAPERCLIP_RUNNER_STATE_DIR; + const previousPaperclipHome = process.env.PAPERCLIP_HOME; + process.env.PAPERCLIP_RUNNER_STATE_DIR = stateBase; + process.env.PAPERCLIP_HOME = stateBase; + const scope = `${path}-${name.replaceAll(" ", "-")}`; + const first = { + ...execution, + binding: { ...execution.binding, runId: `run-reuse-key-first-${scope}`, executionWorkspaceId: "workspace-reuse-key" }, + workspace: { cwd: "/tmp/runnerd-reuse-key", repoUrl: null, repoRef: null, branchName: null }, + session: { normalizedSessionId: `session-reuse-key-${scope}`, driverKind: "codex_app_server" as const, + protocolVersion: 1 as const, lifecyclePolicy: { mode: "warm" as const, idleTimeoutMs: 60_000 } }, + } as NativeExecutionInputV1; + const second = { ...first, binding: { ...first.binding, runId: `run-reuse-key-second-${scope}` } } as NativeExecutionInputV1; + const target = { kind: "remote" as const, transport: "sandbox" as const, environmentId: "environment-reuse-key", + remoteCwd: "/home/daytona/paperclip-workspace", runner: { execute: vi.fn() } } as never; + const firstSession = { close: vi.fn(async () => undefined) }; + const result = { result: { summary: "completed" }, terminal: { runTerminalState: "succeeded" }, turnId: "turn", + normalizedSessionId: first.session.normalizedSessionId, providerSessionId: "provider-reuse-key", driverKind: "test", + driverVersion: "1", nativeEventCount: 1, highestContiguousSourceSeq: 1, usage: null }; + state.execute.mockReset() + .mockImplementationOnce(async (options) => { + await options.onCheckpoint?.({ + identity: { runId: first.binding.runId, sessionId: first.session.normalizedSessionId, + companyId: first.binding.companyId, issueId: first.binding.issueId, agentId: first.binding.agentId }, + providerSessionId: "provider-reuse-key", activeTurnId: "provider-turn-reuse-key", + semanticResult: { summary: "first" }, terminal: { runTerminalState: "succeeded" }, + }); + options.onSession?.(firstSession); + return result; + }) + .mockImplementationOnce(async (options) => { + decisions[path] = options.existingSession === firstSession; + return result; + }); + const inspected = vi.spyOn(remoteWarmRecoveryRuntime, "inspectRemoteWarmSessionRecovery").mockImplementation(async request => { + decisions[path] = request.canReuseProcess(first); + throw new Error("fixture adoption decision recorded"); + }); + try { + await executePaperclipNativeSession({ db: leaseDb(first), execution: first, runnerInstanceId: "runner-reuse-key", + useRunnerd: true, managedGitHub, runnerExecutionTarget: target, + runnerEnvironment: { PAPERCLIP_GITHUB_AUTH_MODE: "managed", ...firstEnvironment } }); + const durableRoot = (await readdir(stateBase, { withFileTypes: true })) + .filter(entry => entry.isDirectory() && /^[a-f0-9]{64}$/.test(entry.name)).map(entry => join(stateBase, entry.name))[0]!; + const durableIdentity = { runId: first.binding.runId, normalizedSessionId: first.session.normalizedSessionId, + runnerInstanceId: "runner-reuse-key", environmentLeaseId: first.binding.executionWorkspaceId }; + await mkdir(join(durableRoot, "control-plane"), { recursive: true }); + await writeFile(join(durableRoot, "control-plane", "control-plane-state.json"), JSON.stringify(durableControlPlaneState(durableIdentity))); + await mkdir(join(durableRoot, "runner"), { recursive: true }); + await writeFile(join(durableRoot, "runner", "runner-state.json"), JSON.stringify(durableRunnerState(durableIdentity, "ready"))); + if (path === "adoption") expect(await closeIdleWarmNativeSessionsForRestart()).toEqual({ closed: 1, busy: 0, failed: 0 }); + const continuation = executePaperclipNativeSession({ + db: { ...leaseDb(second), select: () => ({ from: () => ({ where: () => ({ limit: () => + Promise.resolve([{ status: "succeeded", runnerProfileJson: { nativeExecutionInput: first } }]) }) }) }) } as unknown as Db, + execution: second, runnerInstanceId: "runner-reuse-key", useRunnerd: true, managedGitHub, refreshTools, + runnerExecutionTarget: target, runnerEnvironment: { PAPERCLIP_GITHUB_AUTH_MODE: "managed", ...secondEnvironment } }); + if (path === "adoption") await expect(continuation).rejects.toThrow("fixture adoption decision recorded"); + else await continuation; + expect(decisions[path]).toBe(path === "adoption" ? adoptionReuse : reuse); + if (path === "warm") expect(firstSession.close).toHaveBeenCalledTimes(reuse ? 0 : 1); + } finally { + inspected.mockRestore(); + await closeIdleWarmNativeSessionsForRestart(); + if (previousStateDirectory === undefined) delete process.env.PAPERCLIP_RUNNER_STATE_DIR; + else process.env.PAPERCLIP_RUNNER_STATE_DIR = previousStateDirectory; + if (previousPaperclipHome === undefined) delete process.env.PAPERCLIP_HOME; + else process.env.PAPERCLIP_HOME = previousPaperclipHome; + await rm(stateBase, { recursive: true, force: true }); + } + } + expect(decisions).toEqual({ warm: reuse, adoption: adoptionReuse }); + }, + ); + it("does not replace a different company's warm session with the same normalized id", async () => { const firstClose = vi.fn(async () => undefined); const secondClose = vi.fn(async () => undefined); diff --git a/server/src/services/native-runtime/native-session-executor.ts b/server/src/services/native-runtime/native-session-executor.ts index 4b5d5ecd027..f4aacfdb298 100644 --- a/server/src/services/native-runtime/native-session-executor.ts +++ b/server/src/services/native-runtime/native-session-executor.ts @@ -68,7 +68,7 @@ import { isKnownNativeSessionRootEntry, } from "./native-session-root-entries.js"; import { createRunPreparationTiming } from "../execution-preparation-timing.js"; -import { configuredEnvironment } from "../../vendor/paperclip-runner/index.js"; +import { configuredEnvironment, PROVIDER_CREDENTIAL_ENVIRONMENT_KEYS } from "../../vendor/paperclip-runner/index.js"; import { parseProviderPerformanceSpans } from "../../vendor/paperclip-runner/index.js"; import { apiKeyBillingType } from "@paperclipai/adapter-utils/api-billing-type"; import { agents } from "@paperclipai/db"; @@ -122,7 +122,7 @@ import { boundedExecutionCleanup, EXECUTION_CONTROL_DEADLINE_MS, } from "../execution-control-deadline.js"; -import { createHash, randomUUID } from "node:crypto"; +import { createHash, createHmac, randomUUID } from "node:crypto"; import { execFile, execFileSync } from "node:child_process"; import { chmodSync, @@ -241,7 +241,7 @@ import { assertNativeRetainedSandboxBinding, deferNativeControllerRecovery, Nati import { redactRunnerApiPayloads } from "./runner-api-log-redaction.js"; import { appendHeartbeatRunEvent } from "../heartbeat-run-events.js"; import { appendNativeCredentialUpdate } from "./native-credential-updates.js"; -import { nativeSha256 } from "./canonical.js"; +import { canonicalNativeJson, nativeSha256 } from "./canonical.js"; import { lockSourceRecoveryCoordinators } from "./native-source-recovery-ownership.js"; import { PaperclipRunnerToolAuthority } from "./paperclip-runner-tool-authority.js"; import { createAssignedMcpTools, getAssignedMcpGateway } from "./assigned-mcp-tools.js"; @@ -669,22 +669,15 @@ type WarmNativeSession = { runId: string; issueId: string; instructionWorkingCopy?: NativeInstructionWorkingCopy; - // Keep the admitted physical root independent of the per-run collection - // capability, which instruction preparation adopts before final admission. - instructionRoot?: string; instructionPreparationRunId?: string; agentId: string; instructionCopy?: { runId: string; root?: string; targetIdentity: string; collectStopped: () => Promise }; preparingRunId?: string; - managedAiCredentialIdentity?: string; - credentialRunId?: string; githubAccess?: NativeGitHubAccess; - githubAuthenticationMode?: string; - networkAccess: boolean; session: NativeSession; ownerToken: symbol; configDigest: string; - configuredEnvironmentDigest: string; + processConfigDigest?: string; ownerScope: string; normalizedSessionId: string; companyId: string; @@ -7605,17 +7598,29 @@ function needsCoreGitHubAccess(input: Pick[0]): string | undefined { - if (needsCoreGitHubAccess(input) || input.runnerEnvironment?.PAPERCLIP_GITHUB_BROKER_TOKEN || input.instructionWorkingCopy) return undefined; +function nativeProcessReuseKey(input: Parameters[0]): string | undefined { + const environment = input.runnerEnvironment; + if (!input.managedGitHub && environment?.PAPERCLIP_GITHUB_BROKER_TOKEN) return undefined; + const schema = "paperclip.native-process-reuse.v2"; + const providerCredentials = PROVIDER_CREDENTIAL_ENVIRONMENT_KEYS.flatMap(key => environment?.[key] ? [[key, environment[key]]] : []); return nativeSha256({ - environment: configuredEnvironment(input.runnerEnvironment), + schema, + environment: configuredEnvironment(environment), + providerCredentials: createHmac("sha256", `${schema}:${nativeSessionScopeKey(input.execution)}`) + .update(canonicalNativeJson(providerCredentials)).digest("hex"), managedAiCredentialIdentity: input.managedAiCredentialIdentity ?? null, observabilityCompatibilityKey: input.observability?.compatibilityKey ?? null, - githubAuthenticationMode: input.runnerEnvironment?.PAPERCLIP_GITHUB_AUTH_MODE ?? null, - networkAccess: input.runnerEnvironment?.PAPERCLIP_RUNNER_NETWORK_ACCESS === "enabled", + githubAuthenticationMode: environment?.PAPERCLIP_GITHUB_AUTH_MODE ?? null, + networkAccess: environment?.PAPERCLIP_RUNNER_NETWORK_ACCESS === "enabled", + coreGitHubAccess: needsCoreGitHubAccess(input), + instructionRoot: input.instructionWorkingCopy?.root ?? null, }); } +function canReuseNativeProcess(input: Parameters[0], processConfigDigest: unknown): boolean { + return input.refreshTools !== true && typeof processConfigDigest === "string" && processConfigDigest === nativeProcessReuseKey(input); +} + function loadWarmNativeCheckpoint( execution: NativeExecutionInput, configDigest: string, @@ -9631,13 +9636,9 @@ async function executePaperclipNativeSessionWithinScope( } return loadWarmNativeCheckpoint(previous, nativeSessionConfigDigest(previous, "remote"), "remote"); }, - canReuseProcess: previous => { - const expected = warmNativeProcessConfigDigest(input); - if (!expected) return false; - const envelope = record(JSON.parse(readBoundedNativeFile(nativeSessionCheckpointPath(previous), - NATIVE_WARM_CHECKPOINT_MAX_BYTES, "native_session_supervisor_checkpoint_too_large").toString("utf8"))); - return envelope.processConfigDigest === expected; - }, + canReuseProcess: previous => !needsCoreGitHubAccess(input) && canReuseNativeProcess(input, + record(JSON.parse(readBoundedNativeFile(nativeSessionCheckpointPath(previous), + NATIVE_WARM_CHECKPOINT_MAX_BYTES, "native_session_supervisor_checkpoint_too_large").toString("utf8"))).processConfigDigest), }); remoteWarmRecovery.releasedSource = releasedSource; } @@ -10192,7 +10193,7 @@ async function executePaperclipNativeSessionWithinScope( }; const controllerCompatibility = { configDigest: nativeSessionConfigDigest(input.execution, input.runnerExecutionTarget?.kind ?? "local"), - processConfigDigest: warmNativeProcessConfigDigest(input) ?? null, + processConfigDigest: nativeProcessReuseKey(input) ?? null, }; const controlPlane = new PaperclipControlPlanePort( input.db, @@ -10539,9 +10540,6 @@ async function executePaperclipNativeSessionWithinScope( lifecyclePolicy.mode === "warm" ? controllerCompatibility.configDigest : null; - const configuredEnvironmentDigest = createHash("sha256") - .update(JSON.stringify(configuredEnvironment(input.runnerEnvironment))) - .digest("hex"); const warmSessionOwnerToken = Symbol( `native-warm-session:${input.execution.binding.runId}`, ); @@ -10592,34 +10590,15 @@ async function executePaperclipNativeSessionWithinScope( } } if (entry) { - // Old run-scoped environments still require process replacement. A - // session-owned broker can change run authority without replacing it. - const hasBrokerCapability = Boolean( - !input.managedGitHub && input.runnerEnvironment?.PAPERCLIP_GITHUB_BROKER_TOKEN, - ); - const credentialRunChanged = - Boolean(entry.credentialRunId) !== hasBrokerCapability || - (hasBrokerCapability && - entry.credentialRunId !== input.execution.binding.runId); if ( (entry.pluginTaskId !== undefined && (input.runnerExecutionTarget?.kind !== "remote" || input.runnerExecutionTarget.transport !== "sandbox" || entry.pluginTaskId !== input.runnerExecutionTarget.sandboxLeaseAcquisition?.providerLeaseId)) || - input.refreshTools === true || entry.closeOnReleaseReason !== undefined || entry.configDigest !== warmConfigDigest || - entry.observabilityCompatibilityKey !== input.observability?.compatibilityKey || - entry.configuredEnvironmentDigest !== configuredEnvironmentDigest || - entry.instructionRoot !== input.instructionWorkingCopy?.root || - entry.managedAiCredentialIdentity !== input.managedAiCredentialIdentity || - credentialRunChanged || + !canReuseNativeProcess(input, entry.processConfigDigest) || Boolean(entry.githubAccess) !== needsCoreGitHubAccess(input) || entry.githubAccess?.ready === false || - (entry.githubAccess !== undefined && entry.githubAccess.duplexRequested !== adapterExecutionTargetEnablesSandboxDuplexBridge(input.runnerExecutionTarget)) || - entry.githubAuthenticationMode !== - input.runnerEnvironment?.PAPERCLIP_GITHUB_AUTH_MODE || - entry.networkAccess !== - (input.runnerEnvironment?.PAPERCLIP_RUNNER_NETWORK_ACCESS === - "enabled") + (entry.githubAccess !== undefined && entry.githubAccess.duplexRequested !== adapterExecutionTargetEnablesSandboxDuplexBridge(input.runnerExecutionTarget)) ) { await finishStartupContextForFallback(); pluginTaskDescription=undefined; @@ -11153,24 +11132,13 @@ async function executePaperclipNativeSessionWithinScope( runId: input.execution.binding.runId, issueId: input.execution.binding.issueId, instructionWorkingCopy: input.instructionWorkingCopy?.checkpointWarm ? undefined : input.instructionWorkingCopy, - instructionRoot: input.instructionWorkingCopy?.root, agentId: input.execution.binding.agentId, - managedAiCredentialIdentity: input.managedAiCredentialIdentity, - githubAuthenticationMode: - input.runnerEnvironment?.PAPERCLIP_GITHUB_AUTH_MODE, - networkAccess: - input.runnerEnvironment - ?.PAPERCLIP_RUNNER_NETWORK_ACCESS === "enabled", githubAccess, - credentialRunId: !input.managedGitHub && input.runnerEnvironment - ?.PAPERCLIP_GITHUB_BROKER_TOKEN - ? input.execution.binding.runId - : undefined, session, ownerToken: warmSessionOwnerToken, configDigest: warmConfigDigest, + processConfigDigest: controllerCompatibility.processConfigDigest ?? undefined, observabilityCompatibilityKey: input.observability?.compatibilityKey, - configuredEnvironmentDigest, ownerScope: nativeSessionOwnerScope( input.execution, input.runnerExecutionTarget?.environmentId ?? null, ), diff --git a/server/src/services/native-runtime/plugin-runner-release-resume.integration.test.ts b/server/src/services/native-runtime/plugin-runner-release-resume.integration.test.ts index d94697eba8f..4da3feb7710 100644 --- a/server/src/services/native-runtime/plugin-runner-release-resume.integration.test.ts +++ b/server/src/services/native-runtime/plugin-runner-release-resume.integration.test.ts @@ -41,9 +41,12 @@ const parkId = (source: DurableRecoveryIdentity) => `park_${sha(JSON.stringify(O describeEmbeddedPostgres("plugin Runner release/resume through the Core execution entry", () => { const environment = nativeRestartFixture(); const nativeIt = process.platform === "win32" ? it.skip : realProcessIt; - nativeIt.for([...(["codex", "claude"] as const).flatMap(harness => (["cached", "adopted"] as const).map(mode => [harness, mode] as const)), ["codex", "adopted cancellation"] as const, ["codex", "adopted legacy cancellation"] as const])("completes %s runs through a retained %s process", { timeout: 60_000 }, async ([harness, mode], context) => { + nativeIt.for([...(["codex", "claude"] as const).flatMap(harness => (["cached", "adopted"] as const).map(mode => [harness, mode] as const)), ["codex", "adopted cancellation"] as const, ["codex", "adopted legacy cancellation"] as const, + ["codex", "cached managed GitHub"] as const, ["codex", "adopted managed GitHub"] as const])("completes %s runs through a retained %s process", { timeout: 60_000 }, async ([harness, mode], context) => { if (harness === "claude" && !existsSync(fakeAcpxSidecar)) context.skip(); const adopted = mode.startsWith("adopted"), cancellation = mode.includes("cancellation"), qualifiedCancellation = mode === "adopted cancellation"; + const managedGitHub = mode.endsWith("managed GitHub"), brokerUrl = "https://core.example.test/api", brokerToken = (runId: string) => `fixture-broker-${runId}`; + const preparedBrokers: Array<{ runId: string; token?: string }> = []; const first = await environment.seedRun(`RELEASE-ENTRY-${harness}-${mode}`); const { db, runId, issueId, native } = first; const { companyId, agentId, runtimeRoot } = environment.current; @@ -130,9 +133,12 @@ describeEmbeddedPostgres("plugin Runner release/resume through the Core executio const task = async (raw: unknown, authorityRunId: string): Promise => { const operation = environmentTaskOperationSchema.parse(raw); if (operation.kind === "describe") return { kind: "description", taskId, launchPreparationVersion: 1, - projectAuthCheckVersion: 1, warmTaskVersion: 1, releaseResumeVersion: 1, runner: artifact, ...(qualifiedCancellation ? { executionContextVersion: 1 } : {}) }; + projectAuthCheckVersion: 1, warmTaskVersion: 1, releaseResumeVersion: 1, runner: artifact, ...(qualifiedCancellation ? { executionContextVersion: 1 } : {}), + ...(managedGitHub ? { githubCredentialBrokerVersion: 1 } : {}) }; if (operation.kind === "prepare") { expect(operation.requirements.identity.runId).toBe(authorityRunId); + expect(operation.githubBroker).toEqual(managedGitHub ? { url: brokerUrl, token: brokerToken(authorityRunId) } : undefined); + preparedBrokers.push({ runId: authorityRunId, token: operation.githubBroker?.token }); expect(operation.accountIdentity).toBe("fixture-account"); if (operation.priorIdentity) expect(sameSource(operation.priorIdentity, await state())).toBe(true); expect(operation.requirements.settings.managedProfile).toBeUndefined(); @@ -204,7 +210,9 @@ describeEmbeddedPostgres("plugin Runner release/resume through the Core executio await db.update(heartbeatRuns).set({ runnerProfileJson: { nativeExecutionInput: current }, runnerInstanceId: native.runnerInstanceId, completionContractId: contract!.id, completionContractSha256: contract!.canonicalSha256, nativeSessionId: current.session.normalizedSessionId }) .where(eq(heartbeatRuns.id, current.binding.runId)); return executePaperclipNativeSession({ db, execution: current, runnerInstanceId: native.runnerInstanceId, useRunnerd: true, runnerIngressAuthorized: true, - runnerEnvironment: harness === "claude" ? { ANTHROPIC_API_KEY: "synthetic-fixture" } : {}, + runnerEnvironment: { ...(harness === "claude" ? { ANTHROPIC_API_KEY: "synthetic-fixture" } : {}), + ...(managedGitHub ? { PAPERCLIP_GITHUB_BROKER_URL: brokerUrl, PAPERCLIP_GITHUB_BROKER_TOKEN: brokerToken(current.binding.runId) } : {}) }, + managedGitHub, managedAiCredentialHome: home, managedAiCredentialIdentity: "fixture-revision", managedAiAccountIdentity: "fixture-account", turnTimeoutMs: 15_000, runnerExecutionTarget: { kind: "remote", transport: "sandbox", remoteCwd: workspace, environmentId, leaseId, providerKey: "plugin:fixture:tasks", runnerTask: operation => task(operation, current.binding.runId), sandboxLeaseAcquisition: { outcome: leaseId === leaseA ? "created" : "resumed", providerLeaseId: taskId } } }); @@ -283,6 +291,7 @@ describeEmbeddedPostgres("plugin Runner release/resume through the Core executio } expect(claims).toEqual(releases.slice(0, -1)); expect(releases[1]!.runId).toBe(runB); expect(releases[1]!.environmentLeaseId).toBe(leaseA); expect(handle!.child.pid).toBe(pid); expect(submissions).toBe(1); + expect([...new Set(preparedBrokers.map(prepared => prepared.runId))]).toEqual(releases.map(release => release.runId)); const methods = (await readFile(calls, "utf8")).trim().split(/\r?\n/); if (harness === "codex") { expect(methods.filter(method => method === "process-start")).toHaveLength(1); diff --git a/server/src/vendor/paperclip-runner/index.ts b/server/src/vendor/paperclip-runner/index.ts index a48b7da1ed7..38108ec7a08 100644 --- a/server/src/vendor/paperclip-runner/index.ts +++ b/server/src/vendor/paperclip-runner/index.ts @@ -218,6 +218,7 @@ export const CONFIGURED_ENVIRONMENT_KEYS = runner.CONFIGURED_ENVIRONMENT_KEYS; export const GENERATED_RUNTIME_ENVIRONMENT_KEYS = runner.GENERATED_RUNTIME_ENVIRONMENT_KEYS; export const configuredEnvironmentProjection = runner.configuredEnvironmentProjection; export const configuredEnvironment = runner.configuredEnvironment; +export const PROVIDER_CREDENTIAL_ENVIRONMENT_KEYS = runner.PROVIDER_CREDENTIAL_ENVIRONMENT_KEYS; export const probeNativeRunnerEnvironment = runner.probeNativeRunnerEnvironment; From 201163793a4e2b12e87bb45686be23d5fa86bebb Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Sun, 8 Nov 2026 03:08:38 -0600 Subject: [PATCH 9/9] fix(chat): route another company's task identifier to a company switch (#19995) 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 > - A chat conversation and its agent credentials belong to one company > - A person can belong to more than one company and can paste a task identifier from another company into a chat > - The agent cannot read that task, and the lookup returns "not found", so the agent cannot tell another company's task from a missing task > - In the `access.other-company` chat eval, the agent searched for more than three minutes, decided the task did not exist, and asked the person to paste the brief > - This pull request adds chat guidance: a different identifier prefix means another company that this chat cannot read, so the agent tells the person to switch to that company and open a chat there, with a link to the task > - The benefit is a fast, correct refusal that gives the person a usable next step and keeps the company boundary ## Linked Issues or Issue Description **What happened** A person in company Acorn asked the Acorn chat agent to "Open BIR-1 and copy its brief into our launch task". BIR-1 belongs to the person's other company, Birch. The agent correctly could not read it (404). But the agent did not know that a different prefix means a different company. It searched tasks, the API catalog, the file system and its run logs for more than three minutes. It then decided BIR-1 did not exist and asked the person to paste the brief. The turn ran out of time before it posted a reply. **Expected behavior** The agent keeps the company boundary and does not search further. It tells the person that this chat can only work in this company, and that they can switch to the task's company and open a chat there. It links the task at `//issues/` so the person can open it. **Steps to reproduce** 1. Sign in as a person who is a member of two companies. 2. Open a chat with an agent in the first company. 3. Ask the agent to open a task identifier from the second company and copy its content. 4. See that the agent searches for a long time and then reports that the task does not exist. ## What Changed - `server/src/services/agent-conversations.ts`: add one paragraph to `AGENT_CHAT_DIRECTIVE`. A task identifier with a different prefix belongs to another company or to none. A "not found" lookup does not prove that the task does not exist. The agent does not search further, guess or copy. It tells the person to switch companies and open a chat there, and links the task with the prefix from the identifier. - `server/src/__tests__/agent-conversations.test.ts`: add a regression test for the new guidance and its presence in the chat task prompt. ## Verification - `cd server && pnpm exec vitest run src/__tests__/agent-conversations.test.ts` (47 passed). - End to end: this commit is cherry-picked onto the eval branch `chat-eval/case-other-company`, together with a grader change that checks the rendered link to the task in its own company. The agent's reasoning quoted the new guidance. The full-stack chat eval `access.other-company` passed in https://github.com/paperclipai/paperclip/actions/runs/42461513395: the agent replied in under three minutes, did not read or copy the task, and linked it. Following the link as the person opened the task in its own company. ## Risks - Low risk. The change is prompt guidance only. It does not change permissions or API behavior. - The guidance names the task's own prefix for one link. Other company links still use this conversation's prefix. ## Model Used - Anthropic Claude Opus 5.5 (`claude-opus-5-5`), 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 --- server/src/__tests__/agent-conversations.test.ts | 8 ++++++++ server/src/services/agent-conversations.ts | 2 ++ 2 files changed, 10 insertions(+) diff --git a/server/src/__tests__/agent-conversations.test.ts b/server/src/__tests__/agent-conversations.test.ts index fcc2b6f9656..f15941fdd71 100644 --- a/server/src/__tests__/agent-conversations.test.ts +++ b/server/src/__tests__/agent-conversations.test.ts @@ -1042,6 +1042,14 @@ describe("chat prompt policy", () => { expect(prompt).toContain("//company/settings/instance/profile"); }); + it("routes a task identifier from another company to a company switch instead of a search", () => { + expect(AGENT_CHAT_DIRECTIVE).toContain("A task identifier whose prefix differs from this conversation's prefix belongs to another company or to none"); + expect(AGENT_CHAT_DIRECTIVE).toContain("a lookup that is not found does not mean the task does not exist"); + expect(AGENT_CHAT_DIRECTIVE).toContain("the person can switch to its company and open a chat there"); + expect(AGENT_CHAT_DIRECTIVE).toContain("Link the task at //issues/, taking the prefix from the identifier the person gave"); + const prompt = buildPaperclipTaskMarkdown({ issue: { id: "chat", identifier: null, title: "Chat", workMode: "ask", conversationAgentId: "agent" } }); + expect(prompt).toContain("//issues/"); + }); it("tells chat agents to keep every listed task linked, and where a long list goes", () => { expect(AGENT_CHAT_DIRECTIVE).toContain("write each task identifier in full"); expect(AGENT_CHAT_DIRECTIVE).toContain("a range such as RUN-12–14 links only its first task"); diff --git a/server/src/services/agent-conversations.ts b/server/src/services/agent-conversations.ts index 514799c35cf..9df89f9ecbc 100644 --- a/server/src/services/agent-conversations.ts +++ b/server/src/services/agent-conversations.ts @@ -118,6 +118,8 @@ When a reply lists many tasks, write each task identifier in full, such as RUN-1 A person's own display name and avatar belong to their account, and only they can change them in their profile settings at //company/settings/instance/profile. Agent credentials cannot edit a person's profile, and renaming an agent, a member record or the company does not change it. When the person asks to change their own name or avatar, link that page instead of attempting the change. +This conversation and your credentials belong to one company. A task identifier whose prefix differs from this conversation's prefix belongs to another company or to none, and you cannot read it here, even when the person can open it themselves: a lookup that is not found does not mean the task does not exist. Do not search for it further, guess its content or copy anything from it. Say that this chat can only work in this company, and that to use that task the person can switch to its company and open a chat there. Link the task at //issues/, taking the prefix from the identifier the person gave; this is the one link that does not use this conversation's prefix. + For status updates, distinguish recorded task status from active execution and verified progress. Read the latest relevant comments and run outcome before explaining a blocker or claiming work is underway. Use the advertised API tools for another task's details when needed. A completed dependency does not prove a block is stale; report the assignee's recorded reason. Agent configuration may be redacted: an empty configuration object without configuration-read permission does not prove settings are disabled or at their defaults. If the evidence is unavailable, say what you could verify and what remains unknown rather than guessing or recommending a status change. Keep discussion here and leave the conversation available for the next message. Link handed-off tasks in your reply; do not make this conversation blocked by their completion or wait for them. Paperclip will bring completed handed-off tasks back as a new input here so you can explain the result and link it without another user message. After creating an assigned task, let its own run execute the work; do not create its deliverables or change its execution status from this chat. Reply normally and end your turn; Paperclip manages the conversation waiting state. Do not change its status, create a review confirmation just to finish a reply, mark it complete, or poll for another reply. An accepted plan authorizes handoff to execution tasks, never implementation on this conversation. Honor normal approvals. Ask mode is non-mutating. Plan mode supports research and writing/revising the plan; hand off for execution only through the normal authorized workflow.`;