From e8a418cc4dcb9b84723597daf3b2efe29c46c040 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Sat, 7 Nov 2026 10:54:52 -0600 Subject: [PATCH] fix(workspaces): settle an uncertain remote provisioning row once its sandbox is stopped (#19862) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - A remote task workspace runs the project's provisioning commands in a sandbox. Each command has a `remote_workspace_provision` journal row, because an arbitrary command cannot be replayed after an ambiguous reply > - A row left `running` with `uncertain: true` blocks every later reservation of that workspace, and nothing ever settles it > - A crash or lost reply during provisioning therefore leaves the task's workspace unusable, with no next action > - This pull request settles such a row only when the earlier run's sandbox is provably stopped. Otherwise the 409 names the row, its run, and the operator's action > - The benefit is that a crashed provisioning attempt no longer locks the workspace forever, and the effect is never assumed to have succeeded ## Linked Issues or Issue Description **Bug description** The Phase 0 ownership inventory (finding 4) lists `remote_workspace_provision` rows left `running`/`uncertain` (`remote-workspaces.ts`) as a record with no automatic release. `reserveRemoteWorkspace` rejects every other run with `remote_workspace_provision_unverified`. A command that simply exited non-zero also stayed `uncertain`. **Steps to reproduce** 1. Start remote provisioning and lose the runner's reply (or have the command exit non-zero). 2. End the run and stop its sandbox. 3. Start another run on the task. Reservation fails with `remote_workspace_provision_unverified`, and the next attempt fails the same way. **Expected behavior** After the earlier sandbox is provably stopped, the row settles and the workspace can be reserved again. While that cannot be proven, the error says what to do. Refs #19846 ## What Changed - `remote-workspaces.ts` `reserveRemoteWorkspace`: for each uncertain provision row from another run, `provisionOwnerProvenStopped` requires two things. The row's run is terminal, and every environment lease of that run carries a provider stop or destroy receipt (`hasRemoteTerminationReceipt` or `hasRecoveredCleanupStopReceipt`). A proven row is settled as `failed` (`uncertain: false`, `settledBy: "owner_sandbox_stopped"`) and logged as `workspace.remote_provision_settled`. It is never marked succeeded. - If a later reservation check (for example a writer hold) rejects the attempt, the proven rows are re-proved and settled in their own transaction, so they do not block again. - An unprovable row still blocks. The 409 now carries `operationId` and `runId` and names the action: stop the earlier sandbox, or set the task's Workspace to "New isolated workspace". That picker option already exists in `IssueProperties`. - `provisionRemoteWorkspace`: a reported non-zero exit (not a timeout) is recorded as `failed` with its `exitCode` instead of staying uncertain. ## Verification - `npx vitest run src/services/remote-workspaces.integration.test.ts src/services/remote-workspace-save-concurrency.test.ts`: 126 passed. - New case "settles an uncertain provisioning command once its run's sandbox is provably stopped". A failed run alone still gets the 409 with `operationId`/`runId`. After a stop receipt, the row settles, and the activity is logged once. - New case "records a provisioning command that reported a failing exit as finished, not uncertain". - Red before the fix: with the old `remote-workspaces.ts`, both new cases fail. Green after. - Server `tsc --noEmit` shows no errors in `remote-workspaces.ts`. - Not run: a live browser check of the "New isolated workspace" path for this error. I confirmed in code that it reserves a different workspace, so it bypasses this check. ## Risks - A settled row means "effect unknown". The next writer provisions again in its own epoch, as every new writer already does. A partially applied, non-idempotent command could see its partial effect. That is the same exposure as any failed provisioning, and it happens only after the earlier sandbox has stopped, so the two commands never run at the same time. - Previous-release story: no persisted shape changes. Uncertain rows saved by the previous release are evaluated by the same proof on the next reservation. - No new waits before `turn.start`. The proof runs only when an uncertain row exists, inside the existing reservation transaction. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, used with tool use and code execution in 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 --- .../remote-workspaces.integration.test.ts | 44 ++++++++++ server/src/services/remote-workspaces.ts | 83 ++++++++++++++++++- 2 files changed, 123 insertions(+), 4 deletions(-) diff --git a/server/src/services/remote-workspaces.integration.test.ts b/server/src/services/remote-workspaces.integration.test.ts index 1a2990f9458..0ce20d415c9 100644 --- a/server/src/services/remote-workspaces.integration.test.ts +++ b/server/src/services/remote-workspaces.integration.test.ts @@ -1655,6 +1655,50 @@ suite("remote workspace lifecycle with real Postgres and independent filesystem expect(journal).toMatchObject({ status: "running", metadata: { uncertain: true } }); }, 60_000); + // An uncertain provisioning row used to block every later reservation of + // the workspace forever. It settles only once the earlier run's sandbox is + // provably stopped; until then the reservation names the operator's action. + it("settles an uncertain provisioning command once its run's sandbox is provably stopped", async () => { + const reserved = await reserve({ kind: "empty" }), first = await target("provision-stop-proof"); + const execute = first.runner!.execute.bind(first.runner!); + first.runner!.execute = async input => { + const result = await execute(input); + if (input.command === "bash") throw new Error("lost provisioning reply"); + return result; + }; + await expect(prepareRemoteWorkspace({ db, storage, workspace: reserved, runId: ids.run, lease: await lease(), target: first, + provisionCommand: "printf 'one effect\\n' >> setup-result.txt" })).rejects.toThrow("lost provisioning reply"); + await db.update(heartbeatRuns).set({ status: "failed", finishedAt: new Date() }).where(eq(heartbeatRuns.id, ids.run)); + const nextRun = randomUUID(); + await db.insert(heartbeatRuns).values({ id: nextRun, companyId: ids.company, agentId: ids.agent, nativeIssueId: ids.issue, runtimeMode: "native", status: "running" }); + const reserveNext = () => reserveRemoteWorkspace({ db, storage, companyId: ids.company, issueId: ids.issue, runId: nextRun, + agentId: ids.agent, environmentId: ids.environment, projectId: null, projectWorkspaceId: null, existingWorkspaceId: reserved.id, source: { kind: "empty" } }); + const provision = async () => (await db.select().from(workspaceOperations).where(and( + eq(workspaceOperations.companyId, ids.company), eq(workspaceOperations.phase, "remote_workspace_provision"))))[0]!; + // A failed run alone is not proof: its sandbox may still run the command. + await expect(reserveNext()).rejects.toMatchObject({ status: 409, + details: { code: "remote_workspace_provision_unverified", operationId: (await provision()).id, runId: ids.run } }); + expect(await provision()).toMatchObject({ status: "running", metadata: { uncertain: true } }); + const stopped = await lease(); + await db.update(environmentLeases).set({ status: "released", releasedAt: new Date(), cleanupStatus: "success", + metadata: { ...stopped.metadata, remoteExecutionTermination: remoteTerminationReceipt(stopped, + { providerLeaseId: stopped.providerLeaseId, state: "stopped" }) } }).where(eq(environmentLeases.id, ids.lease)); + const outcome = await reserveNext().then(() => null, (error: unknown) => error as { details?: { code?: string } }); + expect(outcome?.details?.code).not.toBe("remote_workspace_provision_unverified"); + expect(await provision()).toMatchObject({ status: "failed", metadata: { uncertain: false, settledBy: "owner_sandbox_stopped" } }); + expect(await db.select({ action: activityLog.action }).from(activityLog).where(and(eq(activityLog.companyId, ids.company), + eq(activityLog.action, "workspace.remote_provision_settled")))).toHaveLength(1); + }, 60_000); + + it("records a provisioning command that reported a failing exit as finished, not uncertain", async () => { + const reserved = await reserve({ kind: "empty" }), first = await target("provision-exit-failure"); + await expect(prepareRemoteWorkspace({ db, storage, workspace: reserved, runId: ids.run, lease: await lease(), target: first, + provisionCommand: "exit 3" })).rejects.toThrow("remote_workspace_provision_unverified"); + const [journal] = await db.select().from(workspaceOperations).where(and(eq(workspaceOperations.companyId, ids.company), + eq(workspaceOperations.phase, "remote_workspace_provision"))); + expect(journal).toMatchObject({ status: "failed", metadata: { uncertain: false, exitCode: 3 } }); + }, 60_000); + it("preserves enrolled workspaces after disabling enrollment and rejects a legacy writer", async () => { const reserved = await reserve({ kind: "empty" }); expect(shouldUseRemoteWorkspaces({ adapterType: "paperclip_runner", environmentDriver: "sandbox", experimentalEnabled: false, existingWorkspace: reserved })).toBe(true); diff --git a/server/src/services/remote-workspaces.ts b/server/src/services/remote-workspaces.ts index 19b9ac31c44..bdc1a54498e 100644 --- a/server/src/services/remote-workspaces.ts +++ b/server/src/services/remote-workspaces.ts @@ -25,6 +25,7 @@ import { notifyCancelledWorkspacePreservation } from "./cancelled-workspace-pres import { assertExecutionWorkspaceRecordAccess, assertTaskWorkspaceExecutionAccess } from "./task-workspace-access.js"; import { readRemoteWorkspaceWriterHold, type RemoteWorkspaceWriter } from "./remote-workspace-writer-hold.js"; import { persistActivity } from "./activity-log.js"; +import { hasRecoveredCleanupStopReceipt, hasRemoteTerminationReceipt } from "./remote-execution-termination.js"; import { captureRemoteWorkspaceArchive, cleanupRemoteWorkspaceArchive, prepareRemoteWorkspaceDirectory, prepareRemoteRepository, remoteWorkspaceRepositoryPath, RemoteWorkspaceOperationTimeoutError, RemoteWorkspacePreparationTimeoutError, remoteWorkspaceFailureCause, hasRemoteRecoveryAttachment, @@ -181,6 +182,9 @@ export async function reserveRemoteWorkspace(input: { }) { assertRemoteWorkspacePersistence(input.storage ?? getStorageService()); const source = remoteWorkspaceSourceSchema.parse(input.source); + // Provisioning rows proven settled commit even when a later reservation + // check (such as a live writer hold) rejects this attempt. + const provenProvisions: string[] = []; return input.db.transaction(async tx => { await tx.select({ id: companies.id }).from(companies).where(eq(companies.id, input.companyId)).for("key share"); const [issue] = await tx.select().from(issues).where(and(eq(issues.companyId, input.companyId), eq(issues.id, input.issueId))).for("update"); @@ -217,12 +221,21 @@ export async function reserveRemoteWorkspace(input: { : { issueId: input.issueId, workspaceId: existing.id }); await assertExecutionWorkspaceRecordAccess(tx, actor, input.companyId, existing.id); } - const [uncertain] = await tx.select({ id: workspaceOperations.id }).from(workspaceOperations).where(and( + const uncertain = await tx.select().from(workspaceOperations).where(and( eq(workspaceOperations.companyId, input.companyId), eq(workspaceOperations.executionWorkspaceId, existing.id), eq(workspaceOperations.phase, "remote_workspace_provision"), sql`${workspaceOperations.metadata}->>'uncertain' = 'true'`, sql`${workspaceOperations.heartbeatRunId} is distinct from ${input.runId}::uuid`, - )).limit(1); - if (uncertain) throw conflict("The prior provisioning operation needs verification before this workspace can run again.", { code: "remote_workspace_provision_unverified" }); + )).for("update"); + for (const operation of uncertain) { + if (!await provisionOwnerProvenStopped(tx as unknown as Db, operation)) { + throw conflict("A provisioning command from an earlier run may still be running in its sandbox, so this workspace cannot " + + "run again until that sandbox is stopped. Stop the earlier run's sandbox, or set the task's Workspace to " + + "\"New isolated workspace\" to continue in a fresh one.", + { code: "remote_workspace_provision_unverified", operationId: operation.id, runId: operation.heartbeatRunId }); + } + await settleUnverifiedProvision(tx as unknown as Db, operation, input.runId); + provenProvisions.push(operation.id); + } const hold = await readRemoteWorkspaceWriterHold(tx as unknown as Db, { companyId: input.companyId, authority, runId: input.runId }); if (hold.kind === "held") throw new RemoteWorkspaceWriterActiveError(existing.id, hold.writer); if (hold.kind === "unattributed") throw conflict("The workspace still has an unfinished writer or save operation.", { code: "remote_workspace_writer_pending" }); @@ -255,6 +268,9 @@ export async function reserveRemoteWorkspace(input: { action: "workspace.remote_created", entityType: "execution_workspace", entityId: id, runId: input.runId, issueId: input.issueId, details: { authority: "sandbox", epoch: initial.epoch } }); return workspace!; + }).catch(async (error: unknown) => { + if (provenProvisions.length > 0) await settleProvenProvisions(input.db, input.companyId, provenProvisions, input.runId); + throw error; }); } @@ -946,6 +962,58 @@ export async function createRemoteWorkspaceGitAuth(db: Db, companyId: string, ru responsibleUserId: run.responsibleUserId === "local-board" ? null : run.responsibleUserId }); } +const TERMINAL_RUN_STATUSES = ["succeeded", "failed", "cancelled", "timed_out", "interrupted"]; + +/** + * True only with proof that an uncertain provisioning command can no longer + * run: its run is terminal, and every sandbox allocation of that run carries a + * provider stop or destroy receipt bound to that lease and run. Run status, + * age, or a dead server process alone never proves it, because the command + * runs in the sandbox, not in the server. + */ +async function provisionOwnerProvenStopped(db: Db, operation: typeof workspaceOperations.$inferSelect): Promise { + if (!operation.heartbeatRunId) return false; + const [run] = await db.select({ status: heartbeatRuns.status }).from(heartbeatRuns).where(and( + eq(heartbeatRuns.companyId, operation.companyId), eq(heartbeatRuns.id, operation.heartbeatRunId))); + if (!run || !TERMINAL_RUN_STATUSES.includes(run.status)) return false; + const leases = await db.select().from(environmentLeases).where(and( + eq(environmentLeases.companyId, operation.companyId), eq(environmentLeases.heartbeatRunId, operation.heartbeatRunId))); + return leases.length > 0 && leases.every(lease => hasRemoteTerminationReceipt(lease) || hasRecoveredCleanupStopReceipt(lease)); +} + +/** The command's effect is still unknown, so the row is settled as failed, + * never as succeeded. The next writer provisions in its own epoch. */ +async function settleUnverifiedProvision(db: Db, operation: typeof workspaceOperations.$inferSelect, reservingRunId: string) { + const now = new Date(); + await db.update(workspaceOperations).set({ status: "failed", finishedAt: now, updatedAt: now, + metadata: { ...operation.metadata, uncertain: false, settledBy: "owner_sandbox_stopped" } }) + .where(eq(workspaceOperations.id, operation.id)); + await persistActivity(db, { companyId: operation.companyId, actorType: "system", actorId: "remote-workspaces", + action: "workspace.remote_provision_settled", entityType: "execution_workspace", + entityId: operation.executionWorkspaceId ?? operation.id, runId: reservingRunId, issueId: operation.issueId ?? undefined, + details: { operationId: operation.id, ownerRunId: operation.heartbeatRunId, + reason: "The earlier run ended and its sandbox was stopped, so its provisioning command cannot still be running. Its effect is unknown." } }); +} + +/** Re-proves and settles provisioning rows after the reservation that found + * them rolled back, so a proven row never blocks again. */ +async function settleProvenProvisions(db: Db, companyId: string, operationIds: string[], reservingRunId: string) { + try { + await db.transaction(async tx => { + const rows = await tx.select().from(workspaceOperations).where(and(eq(workspaceOperations.companyId, companyId), + inArray(workspaceOperations.id, operationIds), sql`${workspaceOperations.metadata}->>'uncertain' = 'true'`)).for("update"); + for (const operation of rows) { + if (await provisionOwnerProvenStopped(tx as unknown as Db, operation)) { + await settleUnverifiedProvision(tx as unknown as Db, operation, reservingRunId); + } + } + }); + } catch (error) { + // The next reservation proves and settles them again. + logger.warn({ err: error, companyId, operationIds }, "remote workspace provisioning settlement will retry"); + } +} + /** Arbitrary provisioning commands cannot be replayed after an ambiguous reply. */ export async function provisionRemoteWorkspace(input: { db: Db; companyId: string; workspaceId: string; runId: string; epoch: number; @@ -979,7 +1047,14 @@ export async function provisionRemoteWorkspace(input: { await input.assertOwned?.(); const result = await input.target.runner!.execute({ command: "bash", args: ["-lc", command], cwd: input.remoteCwd, timeoutMs: 300_000, bypassSession: true }); - if (result.exitCode !== 0 || result.timedOut) throw new Error("remote_workspace_provision_unverified"); + if (result.exitCode !== 0 || result.timedOut) { + // A reported non-zero exit is a finished command, not an ambiguous one. + // A timeout stays uncertain: the runner may not have stopped it. + if (!result.timedOut) await input.db.update(workspaceOperations).set({ status: "failed", finishedAt: new Date(), + updatedAt: new Date(), metadata: { ...operation.metadata, uncertain: false, exitCode: result.exitCode } }) + .where(and(eq(workspaceOperations.id, operation.id), eq(workspaceOperations.status, "running"))); + throw new Error("remote_workspace_provision_unverified"); + } await input.assertOwned?.(); await input.db.transaction(async tx => { const [workspace] = await tx.select({ metadata: executionWorkspaces.metadata }).from(executionWorkspaces)