From e6e79f458e168202871c2344b42cfd91997a4905 Mon Sep 17 00:00:00 2001 From: Priya Raman Date: Thu, 13 Aug 2026 01:58:00 +0000 Subject: [PATCH] fix(server): refuse to archive a reopen-pending workspace Close the second destructive lifecycle race. The archive route calls archiveWorkspaceUnderLifecycleLock. That method rejected only missing or already-closed rows, then cleared the reopen flag and archived. A reopen sets the reopen-pending flag when it publishes a rebuilt worktree as active while the source issue is still terminal. In that window an authorized actor could invoke the archive control; the method archived the row and the destruction fence then removed the rebuilt worktree. Make archiveWorkspaceUnderLifecycleLock return a distinct "reopen_pending" outcome for a flagged row. It does not clear the flag and does not archive. Add the flag predicate to the update WHERE as defense in depth. Map the outcome to HTTP 409 in the route, before any lease teardown, runtime-service stop, or artifact cleanup. Add a service regression: a terminal-source, active, flagged row rejects the archive and keeps its status, generation, flag, and worktree. Add a route regression: the archive returns 409 and never runs the destruction fence. Co-authored-by: Paperclip --- .../execution-workspaces-routes.test.ts | 32 ++++++++++++ .../execution-workspaces-service.test.ts | 50 +++++++++++++++++++ server/src/routes/execution-workspaces.ts | 11 ++++ server/src/services/execution-workspaces.ts | 31 +++++++++++- 4 files changed, 122 insertions(+), 2 deletions(-) diff --git a/server/src/__tests__/execution-workspaces-routes.test.ts b/server/src/__tests__/execution-workspaces-routes.test.ts index be7deacb2e2..af1d4dbfd90 100644 --- a/server/src/__tests__/execution-workspaces-routes.test.ts +++ b/server/src/__tests__/execution-workspaces-routes.test.ts @@ -10,6 +10,8 @@ const mockExecutionWorkspaceService = vi.hoisted(() => ({ listSummaries: vi.fn(), getById: vi.fn(), getCloseReadiness: vi.fn(), + archiveWorkspaceUnderLifecycleLock: vi.fn(), + fenceClosedWorkspaceDestruction: vi.fn(), reconcileExecutionWorkspaceBranch: vi.fn(), update: vi.fn(), })); @@ -389,4 +391,34 @@ describe.sequential("execution workspace routes", () => { }), })); }); + + it("returns 409 and skips destructive cleanup when the archive hits a reopen-pending workspace", async () => { + // A reopen published the workspace active while its source issue is still + // terminal. The archive control must return 409 before any lease teardown, + // runtime-service stop, or artifact cleanup, so it never removes the rebuilt + // worktree. + mockExecutionWorkspaceService.getById.mockResolvedValue({ + id: "workspace-1", + companyId: "company-1", + sourceIssueId: "issue-1", + status: "active", + mode: "isolated_workspace", + }); + mockExecutionWorkspaceService.getCloseReadiness.mockResolvedValue({ + state: "ready", + blockingReasons: [], + }); + mockExecutionWorkspaceService.archiveWorkspaceUnderLifecycleLock.mockResolvedValue({ + outcome: "reopen_pending", + }); + + const res = await request(createApp()) + .patch("/api/execution-workspaces/workspace-1") + .send({ status: "archived" }); + + expect(res.status).toBe(409); + expect(mockExecutionWorkspaceService.archiveWorkspaceUnderLifecycleLock).toHaveBeenCalledTimes(1); + // The destruction fence never runs, so no worktree is removed. + expect(mockExecutionWorkspaceService.fenceClosedWorkspaceDestruction).not.toHaveBeenCalled(); + }); }); diff --git a/server/src/__tests__/execution-workspaces-service.test.ts b/server/src/__tests__/execution-workspaces-service.test.ts index 3ef97f2a983..44199572450 100644 --- a/server/src/__tests__/execution-workspaces-service.test.ts +++ b/server/src/__tests__/execution-workspaces-service.test.ts @@ -1022,6 +1022,56 @@ describeEmbeddedPostgres("executionWorkspaceService.getCloseReadiness", () => { await expect(fs.access(seeded.worktreePath)).resolves.toBeUndefined(); }); + it("refuses to archive a reopen-pending workspace and leaves the row unchanged", async () => { + // Close the second destructive path. The archive route calls + // archiveWorkspaceUnderLifecycleLock. A reopen published this row active with + // the reopen-pending flag while the source issue is still terminal. The + // archive must not close or clear the flag, so the destruction fence never + // removes the rebuilt worktree during the reopen consumption window. + const seeded = await seedTerminalWorkspace({ mergedPr: true }); + await db + .update(executionWorkspaces) + .set({ + status: "active", + closedAt: null, + cleanupReason: null, + cleanupEligibleAt: null, + metadata: { + createdByRuntime: true, + [EXECUTION_WORKSPACE_LIFECYCLE_GENERATION_METADATA_KEY]: 4, + [EXECUTION_WORKSPACE_REOPEN_PENDING_METADATA_KEY]: true, + }, + }) + .where(eq(executionWorkspaces.id, seeded.executionWorkspaceId)); + + const result = await svc.archiveWorkspaceUnderLifecycleLock({ + id: seeded.executionWorkspaceId, + patch: {}, + closedAt: new Date(), + }); + + expect(result).toEqual({ outcome: "reopen_pending" }); + + const [workspace] = await db + .select({ + status: executionWorkspaces.status, + closedAt: executionWorkspaces.closedAt, + metadata: executionWorkspaces.metadata, + }) + .from(executionWorkspaces) + .where(eq(executionWorkspaces.id, seeded.executionWorkspaceId)); + + // The row stays active, keeps the flag, and keeps its generation. + expect(workspace?.status).toBe("active"); + expect(workspace?.closedAt).toBeNull(); + expect(metadataHasReopenPendingConsumption(workspace?.metadata as Record | null)).toBe(true); + expect( + (workspace?.metadata as Record | null)?.[EXECUTION_WORKSPACE_LIFECYCLE_GENERATION_METADATA_KEY], + ).toBe(4); + // The rebuilt worktree is intact. + await expect(fs.access(seeded.worktreePath)).resolves.toBeUndefined(); + }); + it("holds Git index and ref locks across terminal cleanup", async () => { const seeded = await seedTerminalWorkspace({ mergedPr: true }); await db.update(executionWorkspaces).set({ diff --git a/server/src/routes/execution-workspaces.ts b/server/src/routes/execution-workspaces.ts index 097472c884e..22900b8eea2 100644 --- a/server/src/routes/execution-workspaces.ts +++ b/server/src/routes/execution-workspaces.ts @@ -651,6 +651,17 @@ export function executionWorkspaceRoutes(db: Db, opts: { pluginWorkerManager?: P res.status(404).json({ error: "Execution workspace not found" }); return; } + if (archiveResult.outcome === "reopen_pending") { + // A reopen published this workspace as active while its source issue is + // still terminal. A caller will consume the rebuilt worktree. Refuse the + // archive and return before any lease teardown, runtime-service stop, or + // artifact cleanup, so the archive control never removes the rebuilt + // worktree during the reopen consumption window. + res.status(409).json({ + error: "Execution workspace was reopened and cannot be archived right now", + }); + return; + } workspace = archiveResult.workspace; const capturedGeneration = archiveResult.capturedGeneration; diff --git a/server/src/services/execution-workspaces.ts b/server/src/services/execution-workspaces.ts index 60e905210d0..66e8f911783 100644 --- a/server/src/services/execution-workspaces.ts +++ b/server/src/services/execution-workspaces.ts @@ -2730,11 +2730,23 @@ export function executionWorkspaceService(db: Db, opts: ExecutionWorkspaceServic // reopen from publishing an active row between the status re-check and the // archive write. The archive runs only while the row is still open (not // already archived by a race) and clears the reopen-pending flag. + // + // The method refuses to archive a row that carries the reopen-pending flag. + // A reopen sets that flag when it publishes a rebuilt worktree as active + // while the source issue is still terminal. The method returns a distinct + // "reopen_pending" outcome for that row. It does not clear the flag and does + // not archive. The route maps that outcome to HTTP 409 and returns before any + // destructive cleanup, so the archive control never removes a rebuilt + // worktree during the reopen consumption window. archiveWorkspaceUnderLifecycleLock: async (input: { id: string; patch: Partial; closedAt: Date; - }): Promise<{ workspace: ExecutionWorkspace; capturedGeneration: number } | null> => { + }): Promise< + | { outcome: "archived"; workspace: ExecutionWorkspace; capturedGeneration: number } + | { outcome: "reopen_pending" } + | null + > => { return db.transaction(async (tx) => { await acquireExecutionWorkspaceLifecycleLock(tx, input.id); const fresh = await tx @@ -2747,6 +2759,13 @@ export function executionWorkspaceService(db: Db, opts: ExecutionWorkspaceServic // archive again. return null; } + if (metadataHasReopenPendingConsumption(fresh.metadata as Record | null)) { + // A reopen published this row as active while its source issue is still + // terminal. A caller will consume the rebuilt worktree. Refuse the + // archive and keep the flag, so the destructive path never removes the + // rebuilt worktree. The route maps this to HTTP 409. + return { outcome: "reopen_pending" }; + } const baseMetadata = (input.patch.metadata as Record | null | undefined) ?? (fresh.metadata as Record | null); @@ -2763,11 +2782,19 @@ export function executionWorkspaceService(db: Db, opts: ExecutionWorkspaceServic metadata: archiveMetadata, updatedAt: new Date(), }) - .where(eq(executionWorkspaces.id, input.id)) + .where(and( + eq(executionWorkspaces.id, input.id), + // Defense in depth: never archive a reopen-pending row even if the + // flag appears between the read above and this write. The lifecycle + // lock already serializes reopen and archive, so this predicate only + // adds a second, authoritative guard at the write. + sql`(${executionWorkspaces.metadata} ->> ${EXECUTION_WORKSPACE_REOPEN_PENDING_METADATA_KEY}) IS DISTINCT FROM 'true'`, + )) .returning() .then((rows) => rows[0] ?? null); if (!archived) return null; return { + outcome: "archived", workspace: toExecutionWorkspace(archived), capturedGeneration: readExecutionWorkspaceLifecycleGeneration(archiveMetadata), };