mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-10 12:37:55 +01:00
fix(recovery): treat a warm sandbox as abandoned only on positive evidence its task moved (#20070)
## Thinking Path - #20065 made idle warm sandboxes owned records on the shared retained-orphan kind. A warm sandbox no turn can resume is stopped if running, then destroyed. - One branch of that predicate decides that a task "moved" off a workspace. As merged, it was true whenever no task was bound to the lease's workspace. That includes a task whose binding is null: finished, or between bindings. - A null binding is not proof of a move. The warm sandbox may still be resumable, and for a remote workspace it can hold the files. Phase 2's review flagged this as blocking, so this follow-up requires positive evidence. ## Linked Issues or Issue Description Refs #20048, follow-up to #20065 **What happened?** `taskMovedToAnotherWorkspace()` listed a warm lease as abandoned when its task had no bound workspace. The pending-export check was then the only thing preventing the destroy of a sandbox that could still be resumed. **Steps to reproduce** 1. Create a warm (`reuse_by_environment`) lease, released with a confirmed stop, bound to workspace W. 2. Clear the task's `execution_workspace_id` (task finished, or between bindings). 3. Run an ownership reconciliation pass: on master the lease is settled and queued for destruction. **Expected behavior** Only positive evidence counts as a move: the lease's own task is bound to a different, non-null workspace, and no task is bound to W. An unbound task's warm sandbox is kept; a done or cancelled task's sandbox goes through the existing retention paths. ## What Changed - `server/src/services/environment-retained-orphan-settlement.ts`: - `taskMovedToAnotherWorkspace()` now requires that the lease's own task is bound to a non-null workspace different from the lease's, and that no task in the company is bound to the lease's workspace. - `newestRowForSandbox()` breaks `created_at` ties by id, so two rows written in the same millisecond can't both count as newest. - `server/src/__tests__/environment-retained-orphan-sandbox.test.ts`, two keep cases: - the task has no bound workspace, for both a released and a retained warm lease; - two rows share the newest `created_at`. ## Verification - `cd server && npx vitest run src/__tests__/environment-retained-orphan-sandbox.test.ts`: 32/32 pass. - With master's settlement file, the null-binding case fails, because the lease is settled. The tie case also passes on master, since the settle's existing shared-resource hold protects it; it stays as a regression test for the listing. - The full chain (retained, stop confirmed, listed, handed to destruction) is covered in one test from #20065. ## Risks - **Fewer sandboxes cleaned.** A warm sandbox whose task finished unbound now waits for the existing retention paths instead of this branch. That is the conservative direction, so files are never lost. - **Scope.** No change to the holds or the settle contract. ## 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
f679cf81dc
commit
0a64c9b4ab
2 files changed
+33
-3
No files matched your search
@@ -23,6 +23,7 @@ import { hasRecoveredCleanupStopReceipt } from "../services/remote-execution-ter
|
||||
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, settleStopOnlyCleanup } from "../services/sandbox-stop-and-retain.js";
|
||||
import { releasableRetainedOrphanCondition } from "../services/environment-retained-orphan-settlement.js";
|
||||
|
||||
const support = await getEmbeddedPostgresTestSupport();
|
||||
const suite = support.supported ? describe : describe.skip;
|
||||
@@ -362,6 +363,11 @@ suite("retained orphan sandbox on the shared ownership path", () => {
|
||||
const newer = await warmLease(await run("succeeded"), { workspaceId: current, providerLeaseId });
|
||||
return [older, newer];
|
||||
}],
|
||||
["its task has no bound workspace (finished, or between bindings)", async () => {
|
||||
await db.update(issues).set({ executionWorkspaceId: null }).where(eq(issues.id, issueId));
|
||||
return [await warmLease(await run("succeeded"), { workspaceId: await workspace() }),
|
||||
await warmLease(await run("succeeded"), { status: "retained", workspaceId: await workspace(), receipt: false })];
|
||||
}],
|
||||
["another task still shares its workspace", async () => {
|
||||
const [shared, fresh] = [await workspace(), await workspace()];
|
||||
await db.update(issues).set({ executionWorkspaceId: fresh }).where(eq(issues.id, issueId));
|
||||
@@ -383,6 +389,24 @@ suite("retained orphan sandbox on the shared ownership path", () => {
|
||||
expect(await Promise.all(ids.map(row))).toEqual(before);
|
||||
});
|
||||
|
||||
it("lists only one newest row when two rows for a sandbox share a created_at", async () => {
|
||||
const current = await workspace();
|
||||
await db.update(issues).set({ executionWorkspaceId: current }).where(eq(issues.id, issueId));
|
||||
const providerLeaseId = `sandbox://fake/${randomUUID()}`, createdAt = new Date();
|
||||
const [lower, higher] = ["00000000-0000-4000-8000-000000000001", "ffffffff-ffff-4fff-bfff-ffffffffffff"];
|
||||
for (const id of [lower, higher]) {
|
||||
const runId = await run("failed");
|
||||
await db.insert(environmentLeases).values({ id, companyId, environmentId, issueId, heartbeatRunId: runId,
|
||||
executionWorkspaceId: current, status: "failed", cleanupStatus: "success", leasePolicy: "reuse_by_environment",
|
||||
provider: "fake", providerLeaseId, acquiredAt: createdAt, releasedAt: createdAt, createdAt, updatedAt: createdAt,
|
||||
metadata: { agentId, remoteExecutionTermination: { schema: "paperclip.remote-termination.v1", companyId, runId,
|
||||
leaseId: id, provider: "fake", providerLeaseId, state: "stopped", confirmedAt: createdAt.toISOString() } } });
|
||||
}
|
||||
const listed = await db.select({ id: environmentLeases.id }).from(environmentLeases)
|
||||
.where(releasableRetainedOrphanCondition());
|
||||
expect(listed.map(row => row.id)).toEqual([higher]);
|
||||
});
|
||||
|
||||
it("stops, then destroys, a running warm sandbox once its task moves to another workspace", async () => {
|
||||
const [oldWorkspace, newWorkspace] = [await workspace(), await workspace()];
|
||||
const runId = await run("succeeded");
|
||||
|
||||
@@ -26,14 +26,20 @@ export function retainedOrphanSandboxCondition() {
|
||||
function newestRowForSandbox() {
|
||||
return sql`not exists (select 1 from environment_leases newer where newer.id <> ${environmentLeases.id}
|
||||
and newer.provider = ${environmentLeases.provider} and newer.provider_lease_id = ${environmentLeases.providerLeaseId}
|
||||
and newer.created_at > ${environmentLeases.createdAt})`;
|
||||
and (newer.created_at > ${environmentLeases.createdAt}
|
||||
or (newer.created_at = ${environmentLeases.createdAt} and newer.id > ${environmentLeases.id})))`;
|
||||
}
|
||||
|
||||
/** No task is bound to this lease's workspace any more (its own task moved to
|
||||
* a fresh one, and no other task shares it), so reuse, which matches the exact
|
||||
/** The lease's own task is now bound to a different workspace (positive
|
||||
* evidence of the move; an unbound task is not treated as moved), and no other
|
||||
* task is bound to the lease's workspace, so reuse, which matches the exact
|
||||
* workspace and agent, can never pick the sandbox again. */
|
||||
function taskMovedToAnotherWorkspace() {
|
||||
return and(sql`${environmentLeases.executionWorkspaceId} is not null`,
|
||||
sql`exists (select 1 from issues task where task.id = ${environmentLeases.issueId}
|
||||
and task.company_id = ${environmentLeases.companyId}
|
||||
and task.execution_workspace_id is not null
|
||||
and task.execution_workspace_id <> ${environmentLeases.executionWorkspaceId})`,
|
||||
sql`not exists (select 1 from issues bound where bound.company_id = ${environmentLeases.companyId}
|
||||
and bound.execution_workspace_id = ${environmentLeases.executionWorkspaceId})`);
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user