mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-09 07:10:34 +01:00
fix(environments): retry a teardown interrupted by server death at once (#19749)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - After a remote run, Paperclip destroys its provider sandbox. The teardown first claims the lease with a 15-minute ownership fence, so no other cleanup runs at the same time > - #19696 made restart recovery end a fence when the claiming server process is proven to have exited. It only does that for claims that record the claimer > - The run-release teardown and the stop-only teardown did not record the claimer > - When the server died during a run-release teardown, the sandbox therefore stayed up until the 15-minute fence expired > - This pull request records the claimer on those claims, and stops a teardown attempt lost to a dead server from counting as a failed attempt > - The benefit is that a restart during sandbox teardown destroys the sandbox at the next startup sweep ## Linked Issues or Issue Description **What happened?** In a Daytona restart chaos wave, a run finished at 05:34:48. Its release claimed the lease and called the provider, and a SIGKILL stopped the server 2 seconds later. After restart the lease stayed `pending_cleanup` with an in-flight claim. The sandbox was destroyed at 05:49:49, when the 15-minute claim fence expired. There was no provider error. **Expected behavior** A teardown interrupted by the server's death is retried by the next startup sweep. It is not counted as a failed attempt. A real provider error keeps the normal backoff. **Steps to reproduce** 1. Release a run's sandbox lease with destroy, and hold the provider call. 2. Stop the server during the provider call. 3. Restart. On master, the sweep skips the lease until the fence expires 15 minutes after the claim. Refs #19696. ## What Changed - `environment-runtime.ts`: the run-release teardown claim (`destroyClaimedSandboxLease`) and the stop-only teardown claim save `pendingCleanupController`, bound to the claim's attempt id, the same way sweep claims do since #19696. - `heartbeat/recovery.ts`: sweep claims mark that they counted an attempt. When the sweep ends the fence of a lost claim, it gives that attempt back, so a teardown lost to a dead server does not move toward the cap or the long cooldown. Claims from other paths did not count an attempt, so nothing changes for them. - Workspace-repair and export-retry fences are unchanged. They protect workspace work, not teardown. ## Verification - Red, then green: - `environment-runtime.test.ts` → "retries a run-release teardown at the next sweep after the releasing server dies". The provider call is held during `releaseRunLeases(..., "destroy")`, the claim is marked as from an exited server, and a restarted sweep runs. On master the claim has no controller record and the sweep skips the lease. With the fix, the sweep destroys it at once and records attempt 1. - `heartbeat-pending-cleanup-sweep.test.ts` → "does not count a sweep attempt lost to a dead server as a failed attempt". On master the retry is recorded as attempt 4; with the fix it is attempt 3 again. - `environment-runtime`, `heartbeat-pending-cleanup-sweep`, `sandbox-creation-operations` and `environment-routes` pass (367 tests). `tsc --noEmit` for `server` passes. ## Risks - The proof that a claimer exited is the same-PID-space proof from #19696. A live, foreign or unidentified claimer keeps its fence. - Giving back a counted attempt applies only after that proof. A teardown that fails with a provider error still counts and backs off. - No schema change. The new fields live in existing JSON metadata. > 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 editing, test execution). Extended reasoning enabled. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
33ca62f776
commit
b03ea621c6
4 files changed
+125
-4
No files matched your search
@@ -5,6 +5,7 @@ import { createSandboxOrphanCleanupSpool } from "../services/sandbox-orphan-clea
|
||||
import { idleWorkSnapshot } from "../services/task-admission.js";
|
||||
import { remoteTerminationReceipt } from "../services/remote-execution-termination.js";
|
||||
import { createHash, randomUUID } from "node:crypto";
|
||||
import { spawn } from "node:child_process";
|
||||
import { mkdir, mkdtemp, readdir, rm, writeFile } from "node:fs/promises";
|
||||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
@@ -5306,6 +5307,58 @@ describeEmbeddedPostgres("environmentRuntimeService", () => {
|
||||
},
|
||||
);
|
||||
|
||||
it("retries a run-release teardown at the next sweep after the releasing server dies", async () => {
|
||||
// Shape of a chaos incident: the run finished, its release claimed the lease
|
||||
// and called the provider, and the server was SIGKILLed before the reply.
|
||||
const seeded = await seedReusablePluginSandboxLease();
|
||||
await db.update(heartbeatRuns).set({ status: "succeeded" }).where(eq(heartbeatRuns.id, seeded.runId));
|
||||
let enterProvider!: () => void;
|
||||
let finishProvider!: () => void;
|
||||
const entered = new Promise<void>((resolve) => { enterProvider = resolve; });
|
||||
const finish = new Promise<void>((resolve) => { finishProvider = resolve; });
|
||||
let serverAlive = true;
|
||||
const call = vi.fn(async (_id: string, method: string) => {
|
||||
if (method !== "environmentDestroyLease") throw new Error(`Unexpected method ${method}`);
|
||||
if (serverAlive) {
|
||||
enterProvider();
|
||||
await finish;
|
||||
throw new Error("Server stopped");
|
||||
}
|
||||
return { providerLeaseId: seeded.reusableLease.providerLeaseId, state: "destroyed" };
|
||||
});
|
||||
const makeRuntime = () => environmentRuntimeService(db, { pluginWorkerManager: {
|
||||
isRunning: () => true, call,
|
||||
getWorker: () => ({ supportedMethods: ["environmentDestroyLease"] }),
|
||||
} as unknown as PluginWorkerManager });
|
||||
const closing = makeRuntime().releaseRunLeases(seeded.runId, "released", undefined, "destroy");
|
||||
try {
|
||||
await entered;
|
||||
const claimed = (await environmentService(db).getLeaseById(seeded.reusableLease.id))!;
|
||||
expect(claimed).toMatchObject({ status: "pending_cleanup", metadata: { pendingCleanupInFlight: true } });
|
||||
const attemptId = claimed.metadata?.pendingCleanupAttemptId;
|
||||
// The releasing server exits: its recorded process is gone. Mark it as an
|
||||
// earlier boot whose PID has exited, as a restarted server observes it.
|
||||
const child = spawn(process.execPath, ["-e", ""], { stdio: "ignore" });
|
||||
await new Promise(resolve => child.once("exit", resolve));
|
||||
const recorded = claimed.metadata?.pendingCleanupController as Record<string, unknown> | undefined;
|
||||
expect(recorded).toMatchObject({ attemptId });
|
||||
await db.update(environmentLeases).set({ metadata: { ...claimed.metadata,
|
||||
pendingCleanupController: { ...recorded, bootId: randomUUID(), pid: child.pid } } })
|
||||
.where(eq(environmentLeases.id, claimed.id));
|
||||
serverAlive = false;
|
||||
|
||||
const restarted = heartbeatService(db, { environmentRuntime: makeRuntime() });
|
||||
expect(await restarted.sweepPendingCleanupLeases({ backoffMs: 0 })).toEqual({ swept: 1, destroyed: 1, capped: 0 });
|
||||
const settled = (await environmentService(db).getLeaseById(claimed.id))!;
|
||||
expect(settled).toMatchObject({ status: "expired", cleanupStatus: "success" });
|
||||
// The interrupted release was not a failed teardown attempt.
|
||||
expect(settled.metadata?.pendingCleanupRetryAttempts).toBe(1);
|
||||
} finally {
|
||||
finishProvider();
|
||||
await closing.catch(() => undefined);
|
||||
}
|
||||
});
|
||||
|
||||
it.each([false, true])("preserves a newer destroy receipt when an old run cleanup returns (failure=%s)", async failure => {
|
||||
const seeded = await seedReusablePluginSandboxLease();
|
||||
const service = environmentService(db);
|
||||
|
||||
@@ -21,6 +21,25 @@ import {
|
||||
const mockTelemetryClient = vi.hoisted(() => ({ track: vi.fn() }));
|
||||
vi.mock("../telemetry.ts", () => ({ getTelemetryClient: () => mockTelemetryClient }));
|
||||
|
||||
// A test can hold every caller of the exited-process proof until N callers
|
||||
// arrive, so overlapping sweeps all read a claim before any of them updates it.
|
||||
const exitProofBarrier = vi.hoisted(() => ({ parties: 0, waiting: [] as Array<() => void> }));
|
||||
vi.mock("../services/controller-process-identity.js", async importOriginal => {
|
||||
const actual = await importOriginal<typeof import("../services/controller-process-identity.js")>();
|
||||
return { ...actual, controllerProcessExited: async (...args: Parameters<typeof actual.controllerProcessExited>) => {
|
||||
if (exitProofBarrier.parties > 1) {
|
||||
await new Promise<void>(resolve => {
|
||||
exitProofBarrier.waiting.push(resolve);
|
||||
if (exitProofBarrier.waiting.length >= exitProofBarrier.parties) {
|
||||
for (const release of exitProofBarrier.waiting.splice(0)) release();
|
||||
exitProofBarrier.parties = 0;
|
||||
}
|
||||
});
|
||||
}
|
||||
return actual.controllerProcessExited(...args);
|
||||
} };
|
||||
});
|
||||
|
||||
vi.mock("../middleware/logger.js", () => ({
|
||||
logger: {
|
||||
child: vi.fn(function child(this: unknown) {
|
||||
@@ -380,6 +399,34 @@ describeEmbeddedPostgres("heartbeat sweepPendingCleanupLeases", () => {
|
||||
expect(teardown).toHaveBeenCalledTimes(1);
|
||||
expect(result.destroyed).toBe(1);
|
||||
});
|
||||
it("does not count a sweep attempt lost to a dead server as a failed attempt", async () => {
|
||||
// The lost sweep claim had already counted attempt 3; the retry is attempt 3 again.
|
||||
const scope = await seedCompanyAndEnvironment();
|
||||
const leaseId = await insertOrphanEphemeralLease({ ...scope, updatedAt: new Date(Date.now() - 60_000),
|
||||
metadata: { ...claimMetadata({ ...await exitedClaimer(), countedAttempt: true }), [ATTEMPTS_KEY]: 3 } });
|
||||
const teardown = vi.fn(async () => { throw new Error("Provider unavailable"); });
|
||||
const service = heartbeatService(db, { environmentRuntime: {
|
||||
retryPendingSandboxTeardown: teardown,
|
||||
} as unknown as HeartbeatEnvironmentRuntime });
|
||||
await service.sweepPendingCleanupLeases();
|
||||
expect(teardown).toHaveBeenCalledOnce();
|
||||
expect(await readAttempts(leaseId)).toBe(3);
|
||||
});
|
||||
it("refunds a lost sweep attempt only once when recovery sweeps overlap", async () => {
|
||||
const scope = await seedCompanyAndEnvironment();
|
||||
const leaseId = await insertOrphanEphemeralLease({ ...scope, updatedAt: new Date(Date.now() - 60_000),
|
||||
metadata: { ...claimMetadata({ ...await exitedClaimer(), countedAttempt: true }), [ATTEMPTS_KEY]: 3 } });
|
||||
const teardown = vi.fn(async () => { throw new Error("Provider unavailable"); });
|
||||
const runtime = { retryPendingSandboxTeardown: teardown } as unknown as HeartbeatEnvironmentRuntime;
|
||||
exitProofBarrier.parties = 2;
|
||||
await Promise.all([
|
||||
heartbeatService(db, { environmentRuntime: runtime }).sweepPendingCleanupLeases(),
|
||||
heartbeatService(db, { environmentRuntime: runtime }).sweepPendingCleanupLeases(),
|
||||
]);
|
||||
// One refund (3 -> 2) and one new failed attempt (2 -> 3); never a second refund.
|
||||
expect(teardown).toHaveBeenCalledOnce();
|
||||
expect(await readAttempts(leaseId)).toBe(3);
|
||||
});
|
||||
it("keeps the fence of a claim held by this live server", async () => {
|
||||
const { leaseId, teardown } = await sweepClaimedBy({ ...await currentControllerProcess(), attemptId: "lost-claim" });
|
||||
expect(teardown).not.toHaveBeenCalled();
|
||||
|
||||
@@ -21,6 +21,7 @@ import { remoteTerminationReceipt } from "./remote-execution-termination.js";
|
||||
import { inspectCloudSandboxCleanupAbsence } from "./sandbox-absence-proof.js";
|
||||
import { hasNativeWorkspaceExportResume, releaseCompletedNativeWorkspaceExportRetention } from "./native-runtime/native-workspace-export-resume.js";
|
||||
import { createHash, randomUUID } from "node:crypto";
|
||||
import { currentControllerProcess } from "./controller-process-identity.js";
|
||||
import { and, eq, inArray, ne, or, sql } from "drizzle-orm";
|
||||
import type { Db } from "@paperclipai/db";
|
||||
import { companySecrets, companySecretVersions, environmentLeases, heartbeatRuns } from "@paperclipai/db";
|
||||
@@ -3685,6 +3686,10 @@ export function environmentRuntimeService(
|
||||
if (!(await allowLegacyShutdownWorkspaceCleanup(db, input.leaseRow))) return null;
|
||||
const now = new Date();
|
||||
const attemptId = randomUUID();
|
||||
// The claimer is bound to this attempt. If this server dies mid-teardown,
|
||||
// the startup sweep proves it exited and retries now instead of waiting
|
||||
// out the 15-minute ownership fence.
|
||||
const controller = { ...await currentControllerProcess(), attemptId };
|
||||
// Persist recovery ownership before any provider work. Re-check scope and
|
||||
// run liveness in this write: the earlier selection cannot fence a resume.
|
||||
// The in-flight lease also excludes concurrent closures and cleanup sweeps.
|
||||
@@ -3698,7 +3703,7 @@ export function environmentRuntimeService(
|
||||
metadata: sql`(case when jsonb_typeof(${environmentLeases.metadata}) = 'object'
|
||||
then ${environmentLeases.metadata} else '{}'::jsonb end - 'remoteExecutionTermination')
|
||||
|| ${JSON.stringify({ pendingCleanupAttemptId: attemptId, pendingCleanupInFlight: true,
|
||||
pendingCleanupLeaseExpiresAtMs: Date.now() + 15 * 60_000 })}::jsonb`,
|
||||
pendingCleanupLeaseExpiresAtMs: Date.now() + 15 * 60_000, pendingCleanupController: controller })}::jsonb`,
|
||||
}).where(and(
|
||||
eq(environmentLeases.id, input.leaseRow.id),
|
||||
eq(environmentLeases.companyId, input.leaseRow.companyId),
|
||||
@@ -3931,10 +3936,12 @@ export function environmentRuntimeService(
|
||||
if (hasStopOnlyCleanup(leaseSnapshot)) {
|
||||
const intentKey = stopOnlyCleanupKey(leaseSnapshot);
|
||||
const attemptId = randomUUID();
|
||||
const controller = { ...await currentControllerProcess(), attemptId };
|
||||
const [claimed] = await writeEnvironmentCleanup(db, tx => tx.update(environmentLeases).set({
|
||||
status: "pending_cleanup", cleanupStatus: "failed", releasedAt: new Date(),
|
||||
metadata: sql`${environmentLeases.metadata} || ${JSON.stringify({ pendingCleanupAttemptId: attemptId,
|
||||
pendingCleanupInFlight: true, pendingCleanupLeaseExpiresAtMs: Date.now() + 15 * 60_000 })}::jsonb`,
|
||||
pendingCleanupInFlight: true, pendingCleanupLeaseExpiresAtMs: Date.now() + 15 * 60_000,
|
||||
pendingCleanupController: controller })}::jsonb`,
|
||||
}).where(and(eq(environmentLeases.id, leaseRow.id), eq(environmentLeases.heartbeatRunId, heartbeatRunId),
|
||||
inArray(environmentLeases.status, ["active", "pending_cleanup"]),
|
||||
sql`coalesce(${environmentLeases.metadata}->>'pendingCleanupInFlight', 'false') = 'false'`,
|
||||
|
||||
@@ -1463,13 +1463,26 @@ export function createHeartbeatRecovery(db: Db, dependencies: HeartbeatRecoveryD
|
||||
const controller = readControllerProcess(saved);
|
||||
const attemptId = saved?.attemptId;
|
||||
if (!controller || typeof attemptId !== "string" || !await controllerProcessExited(controller)) continue;
|
||||
// A dead server is not a failed teardown. A sweep claim already counted
|
||||
// its attempt, so give it back; claims from other paths never counted.
|
||||
// The update is single-use: it matches only while the fence is still in
|
||||
// the future and it consumes `countedAttempt`, so an overlapping sweep
|
||||
// that read the same claim cannot refund the attempt again.
|
||||
const returnAttempt = saved?.countedAttempt === true;
|
||||
await db.update(environmentLeases)
|
||||
.set({ metadata: sql`jsonb_set(${pendingCleanupMetadataObjectSql()}, '{pendingCleanupLeaseExpiresAtMs}', to_jsonb(${nowMs}::bigint))` })
|
||||
.set({ metadata: sql`jsonb_set(jsonb_set(jsonb_set(${pendingCleanupMetadataObjectSql()},
|
||||
'{pendingCleanupLeaseExpiresAtMs}', to_jsonb(${nowMs}::bigint)),
|
||||
array[${PENDING_CLEANUP_ATTEMPTS_METADATA_KEY}],
|
||||
to_jsonb(greatest(${pendingCleanupAttemptsSql()} - ${returnAttempt ? 1 : 0}, 0)::int), true),
|
||||
'{pendingCleanupController,countedAttempt}', 'false'::jsonb, true)` })
|
||||
.where(and(
|
||||
eq(environmentLeases.id, row.id),
|
||||
eq(environmentLeases.status, "pending_cleanup"),
|
||||
sql`${environmentLeases.metadata}->>'pendingCleanupInFlight' = 'true'`,
|
||||
sql`(${environmentLeases.metadata}->>'pendingCleanupLeaseExpiresAtMs')::numeric > ${nowMs}`,
|
||||
sql`${environmentLeases.metadata}->>'pendingCleanupAttemptId' = ${attemptId}`,
|
||||
sql`${environmentLeases.metadata}->'pendingCleanupController'->>'attemptId' = ${attemptId}`,
|
||||
sql`coalesce(${environmentLeases.metadata}->'pendingCleanupController'->>'countedAttempt', 'false') = ${returnAttempt ? "true" : "false"}`,
|
||||
));
|
||||
}
|
||||
if (claimed.length < PENDING_CLEANUP_SWEEP_PAGE_SIZE) return;
|
||||
@@ -1503,7 +1516,8 @@ export function createHeartbeatRecovery(db: Db, dependencies: HeartbeatRecoveryD
|
||||
metadata: sql`jsonb_set(${pendingCleanupMetadataObjectSql()}, array[${PENDING_CLEANUP_ATTEMPTS_METADATA_KEY}], to_jsonb(${expectedAttempts + 1}::int), true)
|
||||
|| ${JSON.stringify({ ...(manualAttempt ? { pendingCleanupManualAttemptId: randomUUID() } : {}),
|
||||
pendingCleanupAttemptId: attemptId, pendingCleanupInFlight: true,
|
||||
pendingCleanupLeaseExpiresAtMs: Date.now() + 15 * 60_000, pendingCleanupController: { ...controller, attemptId },
|
||||
pendingCleanupLeaseExpiresAtMs: Date.now() + 15 * 60_000,
|
||||
pendingCleanupController: { ...controller, attemptId, countedAttempt: true },
|
||||
})}::jsonb`,
|
||||
lastUsedAt: now,
|
||||
updatedAt: now,
|
||||
|
||||
Reference in new issue
Block a user