From f789f3e2afa5a5dbe28484d27ce0c702e2a66414 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Sun, 8 Nov 2026 14:27:00 -0600 Subject: [PATCH] fix(environments): a consented environment delete releases restore-source protection so it never stays blocked (#20099) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path - Deleting an environment with consent destroys its kept sandboxes. #20085 added failed runs' restore-source sandboxes (`retain_on_failure`) to that teardown, and the delete dialog now tells the operator that the consented delete destroys those files. - But every teardown path refuses a lease whose sandbox carries a `workspaceRestoreRecovery` marker (`legacyShutdownWorkspaceResourceProtected`). That is correct for ordinary cleanup, since the files may be the only copy. - So the consented delete claimed the restore sources for cleanup, every teardown returned without destroying, and the delete returned 409 `pending_sandbox_cleanup` forever: a refusal the user cannot act on. - The operator's consent is exactly the decision the protection waits for, so the delete releases it explicitly and audits who did, then proceeds. ## Linked Issues or Issue Description Refs #20048, follow-up to #20085 **What happened?** In Daytona warm wave w-20261108T192459-s9501, the harness's consented environment delete returned 409 `pending_sandbox_cleanup` for minutes (it retried every 3 s). Two restore-source leases (8b26e044, bcb897c9) had been claimed for cleanup with `environment_deleted`. The activity log shows a cleanup attempt on every retry, each returning without destroying. On the kept database copy, `legacyShutdownWorkspaceResourceProtected` returns `true` for both, because of their `workspaceRestoreRecovery` marker. Their restore recovery had already succeeded (workspace sync finalized). Phase 3 deleted the two sandboxes by hand. **Steps to reproduce** 1. A failed run leaves a `retain_on_failure` lease with `workspaceRestoreRecovery` metadata (sandbox stopped). 2. `DELETE /api/environments/:id?destroyReusableSandboxLeases=true`. 3. The lease is claimed (`pending_cleanup`), the teardown refuses it, and every retry of the delete returns 409. **Expected behavior** The consented delete destroys the restore source (the operator was told the files go with it) and the environment is deleted. No 409 the user can't act on. ## What Changed - `server/src/services/environment-runtime.ts`: new `releaseRestoreSourcesForEnvironmentDelete({ environmentId, consentedBy })`. For each of the environment's restore-source sandboxes (`retain_on_failure` with `workspaceRestoreRecovery`, any non-terminal status including an earlier `pending_cleanup` claim), it removes the marker from every lease row on that sandbox and records `restoreSourceReleasedByEnvironmentDelete: { consentedBy, releasedAt }`. `destroyReusableSandboxLeasesForEnvironment` takes `consentedBy` and releases first. - `server/src/routes/environments.ts`: a consented delete whose only blockers are pending cleanup or kept sandboxes releases restore protection before the pending-cleanup retry, so leases an earlier attempt left claimed are freed too. It then passes `consentedBy` to the teardown. - Tests: - A restore source now carries the real `workspaceRestoreRecovery` marker, and the consented teardown destroys it. This failed before the fix (`{ destroyed: 0, failed: 1 }`), matching the wave. - The route test asserts the release runs before the pending-cleanup retry. ## 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 restore-source test fails on master with `failed: 1`. ## Risks - **Losing restore files without consent.** The release runs only on the explicit consented delete path (`destroyReusableSandboxLeases=true`), and only when the remaining blockers are pending cleanup or kept sandboxes. It is recorded on every released row. Leases with a pending native export are still skipped by the teardown. - **Ordinary cleanup.** Unchanged: restore protection still holds everywhere else. ## 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 --- .../src/__tests__/environment-routes.test.ts | 8 ++++ .../src/__tests__/environment-runtime.test.ts | 13 ++++-- server/src/routes/environments.ts | 13 ++++++ server/src/services/environment-runtime.ts | 44 +++++++++++++++++++ 4 files changed, 75 insertions(+), 3 deletions(-) diff --git a/server/src/__tests__/environment-routes.test.ts b/server/src/__tests__/environment-routes.test.ts index e7ed84da754..67db6a706d5 100644 --- a/server/src/__tests__/environment-routes.test.ts +++ b/server/src/__tests__/environment-routes.test.ts @@ -28,6 +28,7 @@ const mockProjectService = vi.hoisted(() => ({ const mockEnvironmentRuntimeService = vi.hoisted(() => ({ destroyReusableSandboxLeasesForEnvironment: vi.fn(async () => ({ destroyed: 0, failed: 0, skippedLiveRun: 0 })), + releaseRestoreSourcesForEnvironmentDelete: vi.fn(async () => 0), })); const mockCloseWarmNativeSessionsForEnvironment = vi.hoisted(() => vi.fn(async () => ({ closed: 0, busy: 0, failed: 0 })), @@ -1774,6 +1775,7 @@ describe("environment routes", () => { ).toHaveBeenCalledExactlyOnceWith({ environmentId: "env-1", failureReason: "environment_deleted", + consentedBy: "admin-1", }); expect( mockCloseWarmNativeSessionsForEnvironment, @@ -1845,6 +1847,12 @@ describe("environment routes", () => { expect(res.status).toBe(200); expect(retryPendingEnvironmentCleanup).toHaveBeenCalledExactlyOnceWith({ environmentId: "env-1", actorId: "admin-1" }); + // Consent releases restore-source protection before the cleanup retry, or a + // protected source would refuse every attempt and keep the delete blocked. + expect(mockEnvironmentRuntimeService.releaseRestoreSourcesForEnvironmentDelete) + .toHaveBeenCalledWith({ environmentId: "env-1", consentedBy: "admin-1" }); + expect(mockEnvironmentRuntimeService.releaseRestoreSourcesForEnvironmentDelete.mock.invocationCallOrder[0]) + .toBeLessThan(retryPendingEnvironmentCleanup.mock.invocationCallOrder[0]!); expect(mockEnvironmentService.removeIfDeletable).toHaveBeenCalledWith("env-1"); }); diff --git a/server/src/__tests__/environment-runtime.test.ts b/server/src/__tests__/environment-runtime.test.ts index 5cb2daf5bdb..d2513eca283 100644 --- a/server/src/__tests__/environment-runtime.test.ts +++ b/server/src/__tests__/environment-runtime.test.ts @@ -5537,9 +5537,12 @@ 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. + // A failed run's sandbox kept, stopped, as a workspace-restore source. The + // restore marker protects it from every ordinary teardown. await db.update(environmentLeases).set({ leasePolicy: "retain_on_failure", status: "released", cleanupStatus: "success", - releasedAt: new Date() }).where(eq(environmentLeases.id, reusableLease.id)); + releasedAt: new Date(), metadata: { ...reusableLease.metadata, + workspaceRestoreRecovery: { schema: "paperclip.workspace-restore-recovery.v1", runId } } }) + .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" })] }); @@ -5553,9 +5556,13 @@ describeEmbeddedPostgres("environmentRuntimeService", () => { }), getWorker: vi.fn(() => ({ supportedMethods: ["environmentResumeLease", "environmentReleaseLease", "environmentDestroyLease"] })), } as unknown as PluginWorkerManager; + // The operator consented to deleting the environment and the files only + // this sandbox holds: that consent releases the restore protection. const result = await environmentRuntimeService(db, { pluginWorkerManager: workerManager }) - .destroyReusableSandboxLeasesForEnvironment({ environmentId: reusableLease.environmentId!, failureReason: "environment_deleted" }); + .destroyReusableSandboxLeasesForEnvironment({ environmentId: reusableLease.environmentId!, failureReason: "environment_deleted", + consentedBy: "board-user" }); expect(result).toEqual({ destroyed: 1, failed: 0, skippedLiveRun: 0 }); + expect(workerManager.call).toHaveBeenCalledWith(pluginId, "environmentDestroyLease", expect.anything(), expect.anything()); await expect(environmentService(db).getLeaseById(reusableLease.id)).resolves.toMatchObject({ status: "expired", failureReason: "environment_deleted", cleanupStatus: "success" }); }); diff --git a/server/src/routes/environments.ts b/server/src/routes/environments.ts index b73d3cb58e0..7acd2f1fddb 100644 --- a/server/src/routes/environments.ts +++ b/server/src/routes/environments.ts @@ -1343,6 +1343,18 @@ export function environmentRoutes( // own sandbox, already queued for teardown. Run one attempt now instead of // waiting out the retry cooldown. An attempt in flight elsewhere is not taken // over, and a failed attempt keeps the lease, so the re-check still rejects. + // Consent covers the environment's restore-source sandboxes too: release + // their restore protection first, so neither the pending-cleanup retry nor + // the teardown below refuses them (including ones an earlier attempt left + // claimed for cleanup). + if ( + req.query.destroyReusableSandboxLeases === "true" + && impact.deleteBlockedReasons.every((reason) => + reason === "pending_sandbox_cleanup" || reason === "reusable_sandbox_lease") + ) { + await environmentRuntime.releaseRestoreSourcesForEnvironmentDelete({ environmentId: existing.id, + consentedBy: actor.actorId }); + } if ( req.query.destroyReusableSandboxLeases === "true" && impact.pendingCleanupLeaseCount > 0 @@ -1378,6 +1390,7 @@ export function environmentRoutes( await environmentRuntime.destroyReusableSandboxLeasesForEnvironment({ environmentId: existing.id, failureReason: "environment_deleted", + consentedBy: actor.actorId, }); destroyedReusableSandboxLeaseCount = destroyResult.destroyed; impact = await svc.getDeleteBlastRadius(existing.id); diff --git a/server/src/services/environment-runtime.ts b/server/src/services/environment-runtime.ts index 96aba00bbb7..1e1a5d03de4 100644 --- a/server/src/services/environment-runtime.ts +++ b/server/src/services/environment-runtime.ts @@ -4309,12 +4309,30 @@ export function environmentRuntimeService( * throw leaves the lease in place — so the caller re-checks the blast * radius instead of trusting these counts for the delete decision. */ + /** + * The operator consented to deleting this environment and the files only + * its restore sources hold (the delete dialog says so). That consent is the + * decision that releases the restore protection: drop `workspaceRestoreRecovery` + * from every row on each of the environment's restore-source sandboxes, and + * record who released it, so the consented teardown can destroy them. + * Without this, every teardown refuses a protected restore source and the + * delete stays blocked with nothing the operator can do. + */ + async releaseRestoreSourcesForEnvironmentDelete(input: { environmentId: string; consentedBy: string }): Promise { + return await releaseRestoreSourcesForEnvironmentDelete(db, input); + }, + async destroyReusableSandboxLeasesForEnvironment(input: { environmentId: string; failureReason?: string; + /** The operator who consented to the delete; releases restore protection. */ + consentedBy?: string; }): Promise<{ destroyed: number; failed: number; skippedLiveRun: number }> { const environment = await environmentsSvc.getById(input.environmentId); if (!environment) return { destroyed: 0, failed: 0, skippedLiveRun: 0 }; + if (input.consentedBy) { + await releaseRestoreSourcesForEnvironmentDelete(db, { environmentId: input.environmentId, consentedBy: input.consentedBy }); + } const leaseRows = await db .select() .from(environmentLeases) @@ -4513,4 +4531,30 @@ export function environmentRuntimeService( }; } +async function releaseRestoreSourcesForEnvironmentDelete(db: Db, input: { environmentId: string; consentedBy: string }): Promise { + const sources = await db.select({ provider: environmentLeases.provider, providerLeaseId: environmentLeases.providerLeaseId }) + .from(environmentLeases).where(and( + eq(environmentLeases.environmentId, input.environmentId), + eq(environmentLeases.leasePolicy, "retain_on_failure"), + inArray(environmentLeases.status, ["released", "retained", "failed", "pending_cleanup"]), + sql`${environmentLeases.metadata} ? 'workspaceRestoreRecovery'`, + sql`${environmentLeases.providerLeaseId} is not null`, + )); + let released = 0; + for (const source of sources) { + const rows = await db.update(environmentLeases).set({ updatedAt: new Date(), + metadata: sql`(${environmentLeases.metadata} - 'workspaceRestoreRecovery') || ${JSON.stringify({ + restoreSourceReleasedByEnvironmentDelete: { consentedBy: input.consentedBy, releasedAt: new Date().toISOString() }, + })}::jsonb`, + }).where(and( + sql`${environmentLeases.provider} is not distinct from ${source.provider}`, + eq(environmentLeases.providerLeaseId, source.providerLeaseId!), + sql`${environmentLeases.metadata} ? 'workspaceRestoreRecovery'`, + )).returning({ id: environmentLeases.id }); + released += rows.length; + } + return released; +} + + export type EnvironmentRuntimeService = ReturnType;