From 0417bfea376d7bbf3ec0e0aeb9a82b465f80ecd8 Mon Sep 17 00:00:00 2001 From: Dotta Date: Sun, 8 Nov 2026 11:57:53 -0600 Subject: [PATCH] fix(environments): guard the atomic delete for kept restore sources; tell the operator what deleting them loses Review follow-up: removeIfDeletable's atomic guard now uses the same kept-sandbox policies as the blast radius and the teardown (one KEPT_SANDBOX_LEASE_POLICIES constant). Each blast-radius holder carries its leasePolicy, and the delete dialog and refusal text say when a holder is a failed run's workspace copy whose files the consented delete destroys, instead of promising a fresh sandbox. Co-Authored-By: Claude Opus 5.5 --- packages/shared/src/types/environment.ts | 4 ++++ server/src/__tests__/environment-runtime.test.ts | 5 ++++- server/src/__tests__/environment-service.test.ts | 1 + server/src/routes/environments.ts | 3 +++ server/src/services/environment-runtime.ts | 6 +++--- server/src/services/environments.ts | 14 ++++++++++---- ui/src/pages/CompanyEnvironments.tsx | 8 ++++++++ 7 files changed, 33 insertions(+), 8 deletions(-) diff --git a/packages/shared/src/types/environment.ts b/packages/shared/src/types/environment.ts index 9f929fcb71b..f28a839dd38 100644 --- a/packages/shared/src/types/environment.ts +++ b/packages/shared/src/types/environment.ts @@ -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; diff --git a/server/src/__tests__/environment-runtime.test.ts b/server/src/__tests__/environment-runtime.test.ts index 4446aee2771..5cb2daf5bdb 100644 --- a/server/src/__tests__/environment-runtime.test.ts +++ b/server/src/__tests__/environment-runtime.test.ts @@ -5541,7 +5541,10 @@ describeEmbeddedPostgres("environmentRuntimeService", () => { 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 }); + .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) => { diff --git a/server/src/__tests__/environment-service.test.ts b/server/src/__tests__/environment-service.test.ts index 161b748b5c1..4c600fd3dfe 100644 --- a/server/src/__tests__/environment-service.test.ts +++ b/server/src/__tests__/environment-service.test.ts @@ -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, diff --git a/server/src/routes/environments.ts b/server/src/routes/environments.ts index 879348d21c6..b73d3cb58e0 100644 --- a/server/src/routes/environments.ts +++ b/server/src/routes/environments.ts @@ -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; diff --git a/server/src/services/environment-runtime.ts b/server/src/services/environment-runtime.ts index 02d108cf25a..96aba00bbb7 100644 --- a/server/src/services/environment-runtime.ts +++ b/server/src/services/environment-runtime.ts @@ -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"; @@ -4324,7 +4324,7 @@ export function environmentRuntimeService( // 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, ["reuse_by_environment", "retain_on_failure"]), + 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"]), @@ -4364,7 +4364,7 @@ export function environmentRuntimeService( continue; } const lease = await destroyClaimedSandboxLease({ environment, leaseRow, failureReason, anyLeasePolicy: true, - scopeCondition: and(inArray(environmentLeases.leasePolicy, ["reuse_by_environment", "retain_on_failure"])) }); + 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. diff --git a/server/src/services/environments.ts b/server/src/services/environments.ts index b7f57490c4e..dd49c30b90c 100644 --- a/server/src/services/environments.ts +++ b/server/src/services/environments.ts @@ -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): Promise { 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,9 +1434,7 @@ export function environmentService(db: Db) { .where( and( eq(environmentLeases.environmentId, id), - // A kept sandbox outlives its run: a reusable one for the next turn, - // and a retain-on-failure one as a workspace-restore source. - inArray(environmentLeases.leasePolicy, ["reuse_by_environment", "retain_on_failure"]), + inArray(environmentLeases.leasePolicy, [...KEPT_SANDBOX_LEASE_POLICIES]), inArray(environmentLeases.status, ["active", "released", "retained", "failed"]), ), ), @@ -1449,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, diff --git a/ui/src/pages/CompanyEnvironments.tsx b/ui/src/pages/CompanyEnvironments.tsx index e35a6d59162..aba98699f67 100644 --- a/ui/src/pages/CompanyEnvironments.tsx +++ b/ui/src/pages/CompanyEnvironments.tsx @@ -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,