mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-29 21:07:40 +01:00
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 <noreply@paperclip.ing>
This commit is contained in:
4 files changed
+122
-2
No files matched your search
@@ -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();
|
||||
});
|
||||
});
|
||||
@@ -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<string, unknown> | null)).toBe(true);
|
||||
expect(
|
||||
(workspace?.metadata as Record<string, unknown> | 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({
|
||||
|
||||
@@ -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;
|
||||
|
||||
|
||||
@@ -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<typeof executionWorkspaces.$inferInsert>;
|
||||
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<string, unknown> | 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<string, unknown> | null | undefined)
|
||||
?? (fresh.metadata as Record<string, unknown> | 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<boolean>`(${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),
|
||||
};
|
||||
|
||||
Reference in new issue
Block a user