mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-10 12:37:55 +01:00
fix(environments): tear down retain-on-failure sandboxes when their environment is deleted (#20085)
## Thinking Path - Remote sandboxes cost money while they exist, so every sandbox Paperclip keeps must have an owner and an end. - Two lease policies keep a sandbox after its run. `reuse_by_environment` keeps a warm sandbox for the next turn. `retain_on_failure` keeps a failed run's sandbox, stopped, as a workspace-restore source. - Environment deletion is the last chance to end both: with the operator's consent it destroys the environment's kept sandboxes, otherwise it refuses. - It only looked at `reuse_by_environment`. A `retain_on_failure` sandbox neither blocked the delete nor got destroyed, so it was orphaned the moment its environment was deleted. ## Linked Issues or Issue Description Refs #20048 **What happened?** In Daytona warm wave w-20261108T170423-s9501, the harness deleted the environment after grading. Three stopped sandboxes (4e20c8b3, abf0980b, 5e49c561) remained afterwards and had to be deleted by hand through the Daytona API. All three belonged to `retain_on_failure` leases: failed runs' sandboxes kept as workspace-restore sources. **This leaks on master today, with per-turn sandboxes too.** The four leases in that wave carried `workspaceRestoreRecovery` from `legacy-workspace-restore-recovery.ts` (no `nativeSourceRetention` marker). That writer turns any failed run's lease with a workspace-restore failure into a `retain_on_failure` restore source, whether the environment is warm or per-turn. Every such sandbox has been surviving environment deletion and costing money until deleted by hand. **Steps to reproduce** 1. A run in a sandbox environment fails, and its lease is released as a `retain_on_failure` restore source (sandbox stopped, not destroyed). 2. Delete the environment with `destroyReusableSandboxLeases=true`. 3. The delete succeeds; the restore-source sandbox is never destroyed and has no lease or environment left to find it by. **Expected behavior** The delete blast radius counts kept restore-source sandboxes like reusable ones. Without consent, the delete is refused; with consent, they are destroyed through the same claimed teardown before the environment row is removed. ## What Changed - `server/src/services/environments.ts`: `getDeleteBlastRadius` counts leases with policy `reuse_by_environment` **or `retain_on_failure`** (status `active`, `released`, `retained` or `failed`) as kept-sandbox holders. The blocked reason is still `reusable_sandbox_lease`. - `server/src/services/environment-runtime.ts`: `destroyReusableSandboxLeasesForEnvironment` selects both policies. It claims through `destroyClaimedSandboxLease` with `anyLeasePolicy` plus a scope condition limited to those two policies, keeping its existing guards: live-run skip, pending export, legacy shutdown protection, and the claim compare-and-swap. - `server/src/__tests__/environment-runtime.test.ts`: a released `retain_on_failure` lease on a failed run now blocks the delete and is destroyed by the consented teardown (`expired`, `environment_deleted`). This fails on master. ## Verification - `cd server && npx vitest run src/__tests__/environment-runtime.test.ts src/__tests__/environment-service.test.ts src/__tests__/environment-routes.test.ts`: 347/347 pass. - The new test fails on master: the blast radius reports no kept sandbox. ## Risks - **A delete now refused that used to succeed.** An environment holding restore-source sandboxes is refused until the operator consents to destroying them. That is the intended behaviour; it was silently leaking them before. - **Losing restore files.** The consented delete destroys restore sources, which is what the operator agreed to by deleting the environment. Leases with a pending export or legacy shutdown protection are still skipped and keep blocking. ## Model Used Claude Opus 5.5 (`claude-opus-5-5`, Anthropic), 1M context window, tool use and code execution, via 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 <noreply@anthropic.com>
This commit is contained in:
1 parent
a9ae1b69c3
commit
015d3d0a7c
7 files changed
+59
-5
No files matched your search
@@ -107,6 +107,10 @@ export type EnvironmentDeleteBlockedReason =
|
||||
*/
|
||||
export interface EnvironmentDeleteReusableLeaseHolder {
|
||||
leaseId: string;
|
||||
/** `reuse_by_environment`: a warm sandbox for the next turn (its workspace
|
||||
* provisions a fresh one). `retain_on_failure`: a stopped sandbox kept as the
|
||||
* only copy of a failed run's workspace files (destroying it loses them). */
|
||||
leasePolicy: string;
|
||||
executionWorkspaceId: string | null;
|
||||
executionWorkspaceName: string | null;
|
||||
issueId: string | null;
|
||||
|
||||
@@ -5534,6 +5534,32 @@ describeEmbeddedPostgres("environmentRuntimeService", () => {
|
||||
});
|
||||
});
|
||||
|
||||
it("destroys a retained restore-source sandbox (retain_on_failure) when its environment is deleted", async () => {
|
||||
const { pluginId, runId, reusableLease } = await seedReusablePluginSandboxLease();
|
||||
await db.update(heartbeatRuns).set({ status: "failed" }).where(eq(heartbeatRuns.id, runId));
|
||||
// A failed run's sandbox kept, stopped, as a workspace-restore source.
|
||||
await db.update(environmentLeases).set({ leasePolicy: "retain_on_failure", status: "released", cleanupStatus: "success",
|
||||
releasedAt: new Date() }).where(eq(environmentLeases.id, reusableLease.id));
|
||||
expect(await environmentService(db).getDeleteBlastRadius(reusableLease.environmentId!))
|
||||
.toMatchObject({ reusableSandboxLeaseCount: 1, canDelete: false,
|
||||
reusableSandboxLeaseHolders: [expect.objectContaining({ leasePolicy: "retain_on_failure" })] });
|
||||
// The atomic delete guard refuses too, not only the blast-radius pre-check.
|
||||
expect(await environmentService(db).removeIfDeletable(reusableLease.environmentId!)).toBeFalsy();
|
||||
const workerManager = {
|
||||
isRunning: vi.fn((id: string) => id === pluginId),
|
||||
call: vi.fn(async (_pluginId: string, method: string) => {
|
||||
if (method === "environmentDestroyLease") return { providerLeaseId: reusableLease.providerLeaseId, state: "destroyed" };
|
||||
throw new Error(`Unexpected plugin method: ${method}`);
|
||||
}),
|
||||
getWorker: vi.fn(() => ({ supportedMethods: ["environmentResumeLease", "environmentReleaseLease", "environmentDestroyLease"] })),
|
||||
} as unknown as PluginWorkerManager;
|
||||
const result = await environmentRuntimeService(db, { pluginWorkerManager: workerManager })
|
||||
.destroyReusableSandboxLeasesForEnvironment({ environmentId: reusableLease.environmentId!, failureReason: "environment_deleted" });
|
||||
expect(result).toEqual({ destroyed: 1, failed: 0, skippedLiveRun: 0 });
|
||||
await expect(environmentService(db).getLeaseById(reusableLease.id)).resolves.toMatchObject({
|
||||
status: "expired", failureReason: "environment_deleted", cleanupStatus: "success" });
|
||||
});
|
||||
|
||||
it("destroys a failed reusable sandbox after successful release before deleting its environment", async () => {
|
||||
const { pluginId, companyId, runId, environment, reusableLease } = await seedReusablePluginSandboxLease();
|
||||
const workerManager = {
|
||||
|
||||
@@ -711,6 +711,7 @@ describeEmbeddedPostgres("environmentService leases", () => {
|
||||
expect(activeImpact?.reusableSandboxLeaseHolders).toEqual([
|
||||
{
|
||||
leaseId: lease.id,
|
||||
leasePolicy: "reuse_by_environment",
|
||||
executionWorkspaceId: workspaceId,
|
||||
executionWorkspaceName: "ACME-7-reusable-lease-holder",
|
||||
issueId,
|
||||
|
||||
@@ -634,6 +634,9 @@ export function environmentRoutes(
|
||||
return "Cannot delete this environment while a sandbox cleanup is pending. Wait for the cleanup sweep to destroy the orphan sandbox, then retry.";
|
||||
}
|
||||
if (impact.reusableSandboxLeaseCount > 0) {
|
||||
if (impact.reusableSandboxLeaseHolders.some(holder => holder.leasePolicy === "retain_on_failure")) {
|
||||
return "Cannot delete this environment while it keeps a failed run's workspace copy (a stopped sandbox kept for file recovery). Deleting the environment with consent destroys that sandbox and the files only it holds.";
|
||||
}
|
||||
return "Cannot delete this environment while it has a reusable sandbox lease. Remove the associated execution workspace or issue so Paperclip can destroy the sandbox, then retry.";
|
||||
}
|
||||
return null;
|
||||
|
||||
@@ -60,7 +60,7 @@ import {
|
||||
runWithRuntimeParent,
|
||||
type StartupSpanContext,
|
||||
} from "@paperclipai/adapter-utils/acpx-engine/startup-timing";
|
||||
import { environmentService } from "./environments.js";
|
||||
import { environmentService, KEPT_SANDBOX_LEASE_POLICIES } from "./environments.js";
|
||||
import { instanceSettingsService } from "./instance-settings.js";
|
||||
import { hasSandboxSnapshotQualification, qualifiedSandboxSnapshotId, usesManagedCloudSandboxImage, type SandboxTemplateIdentity } from "./sandbox-snapshot-qualification.js";
|
||||
import { verifyRecoverableNativeHarnessBackup } from "./native-runtime/native-harness-backup-storage.js";
|
||||
@@ -4321,7 +4321,10 @@ export function environmentRuntimeService(
|
||||
.where(
|
||||
and(
|
||||
eq(environmentLeases.environmentId, input.environmentId),
|
||||
eq(environmentLeases.leasePolicy, "reuse_by_environment"),
|
||||
// Reusable sandboxes, and retain-on-failure sandboxes kept stopped as
|
||||
// workspace-restore sources: both outlive their run, so a deleted
|
||||
// environment must tear them down or they are orphaned.
|
||||
inArray(environmentLeases.leasePolicy, [...KEPT_SANDBOX_LEASE_POLICIES]),
|
||||
// A failed run may have stopped its reusable sandbox without
|
||||
// destroying it; that provider handle still needs scoped teardown.
|
||||
inArray(environmentLeases.status, ["active", "released", "retained", "failed"]),
|
||||
@@ -4360,7 +4363,8 @@ export function environmentRuntimeService(
|
||||
skippedLiveRun += 1;
|
||||
continue;
|
||||
}
|
||||
const lease = await destroyClaimedSandboxLease({ environment, leaseRow, failureReason });
|
||||
const lease = await destroyClaimedSandboxLease({ environment, leaseRow, failureReason, anyLeasePolicy: true,
|
||||
scopeCondition: inArray(environmentLeases.leasePolicy, [...KEPT_SANDBOX_LEASE_POLICIES]) });
|
||||
if (!lease) {
|
||||
// Lost to a racing resume or a concurrent terminal transition — the
|
||||
// lease is no longer ours to destroy.
|
||||
|
||||
@@ -60,6 +60,12 @@ import {
|
||||
type ManagedPluginEnvironmentReconcileAction,
|
||||
} from "./managed-plugin-environments.js";
|
||||
|
||||
/** Lease policies whose sandbox outlives its run: a reusable warm sandbox, and a
|
||||
* retain-on-failure sandbox kept stopped as a workspace-restore source. The
|
||||
* delete blast radius, the atomic delete guard and the consented teardown must
|
||||
* all use this one list, or a delete can orphan a sandbox. */
|
||||
export const KEPT_SANDBOX_LEASE_POLICIES = ["reuse_by_environment", "retain_on_failure"] as const;
|
||||
|
||||
export async function attachSandboxCreation(db: Db, id: string, scope: { companyId: string; environmentId: string; heartbeatRunId?: string | null },
|
||||
attach: (tx: Db) => Promise<EnvironmentLease>): Promise<EnvironmentLease> {
|
||||
return db.transaction(async tx => {
|
||||
@@ -1312,7 +1318,7 @@ export function environmentService(db: Db) {
|
||||
sql`not exists (
|
||||
select 1 from ${environmentLeases}
|
||||
where ${environmentLeases.environmentId} = ${environments.id}
|
||||
and ${environmentLeases.leasePolicy} = 'reuse_by_environment'
|
||||
and ${inArray(environmentLeases.leasePolicy, [...KEPT_SANDBOX_LEASE_POLICIES])}
|
||||
and ${environmentLeases.status} in ('active', 'released', 'retained', 'failed')
|
||||
)`,
|
||||
),
|
||||
@@ -1415,6 +1421,7 @@ export function environmentService(db: Db) {
|
||||
db
|
||||
.select({
|
||||
leaseId: environmentLeases.id,
|
||||
leasePolicy: environmentLeases.leasePolicy,
|
||||
executionWorkspaceId: environmentLeases.executionWorkspaceId,
|
||||
executionWorkspaceName: executionWorkspaces.name,
|
||||
issueId: environmentLeases.issueId,
|
||||
@@ -1427,7 +1434,7 @@ export function environmentService(db: Db) {
|
||||
.where(
|
||||
and(
|
||||
eq(environmentLeases.environmentId, id),
|
||||
eq(environmentLeases.leasePolicy, "reuse_by_environment"),
|
||||
inArray(environmentLeases.leasePolicy, [...KEPT_SANDBOX_LEASE_POLICIES]),
|
||||
inArray(environmentLeases.status, ["active", "released", "retained", "failed"]),
|
||||
),
|
||||
),
|
||||
@@ -1447,6 +1454,7 @@ export function environmentService(db: Db) {
|
||||
const pendingCleanupLeaseCount = countFromRows(pendingCleanupLeaseRows);
|
||||
const reusableSandboxLeaseHolders = reusableSandboxLeaseRows.map((row) => ({
|
||||
leaseId: row.leaseId,
|
||||
leasePolicy: row.leasePolicy,
|
||||
executionWorkspaceId: row.executionWorkspaceId,
|
||||
executionWorkspaceName: row.executionWorkspaceName,
|
||||
issueId: row.issueId,
|
||||
|
||||
@@ -109,6 +109,9 @@ function environmentDeleteBlockMessage(impact: EnvironmentDeleteBlastRadius): st
|
||||
return "Cannot delete this environment while a sandbox cleanup is pending. Wait for the cleanup sweep to destroy the orphan sandbox, then retry.";
|
||||
}
|
||||
if (impact.reusableSandboxLeaseCount > 0) {
|
||||
if (impact.reusableSandboxLeaseHolders.some(holder => holder.leasePolicy === "retain_on_failure")) {
|
||||
return "Cannot delete this environment while it keeps a failed run's workspace copy (a stopped sandbox kept for file recovery). Deleting the environment with consent destroys that sandbox and the files only it holds.";
|
||||
}
|
||||
return "Cannot delete this environment while it has a reusable sandbox lease. Remove the associated execution workspace or issue so Paperclip can destroy the sandbox, then retry.";
|
||||
}
|
||||
return null;
|
||||
@@ -1924,6 +1927,8 @@ export function CompanyEnvironments({ mode = "list" }: CompanyEnvironmentsProps)
|
||||
}
|
||||
// One row per workspace holding blocking sandbox leases (several leases can
|
||||
// share a workspace). A lease whose workspace FK was nulled groups alone.
|
||||
const retainedFailureSourceCount = (deleteBlastRadius?.reusableSandboxLeaseHolders ?? [])
|
||||
.filter(holder => holder.leasePolicy === "retain_on_failure").length;
|
||||
const reusableLeaseHolderGroups = (() => {
|
||||
const groups = new Map<
|
||||
string,
|
||||
@@ -2543,6 +2548,9 @@ export function CompanyEnvironments({ mode = "list" }: CompanyEnvironmentsProps)
|
||||
reusableLeaseOnlyBlock && deleteBlastRadius
|
||||
? `${deleteBlastRadius.reusableSandboxLeaseCount === 1 ? "1 reusable sandbox" : `${deleteBlastRadius.reusableSandboxLeaseCount} reusable sandboxes`} will be destroyed; the workspaces holding them stay open and provision a fresh sandbox on their next run.`
|
||||
: null,
|
||||
reusableLeaseOnlyBlock && retainedFailureSourceCount > 0
|
||||
? `${retainedFailureSourceCount === 1 ? "1 of them is" : `${retainedFailureSourceCount} of them are`} a failed run's workspace copy kept for file recovery; destroying it loses the files only it holds.`
|
||||
: null,
|
||||
agentsUsingEnvironment.length > 0
|
||||
? `${agentsUsingEnvironment.length === 1 ? "1 agent uses" : `${agentsUsingEnvironment.length} agents use`} this environment as their default. Choose the environment those agents should be reassigned to.`
|
||||
: null,
|
||||
|
||||
Reference in new issue
Block a user