mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-14 00:26:28 +01:00
fix(recovery): release a parent's Retry once a done child's recovered files are saved (#20728)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - A parent task waits for its done child tasks to save their workspace files before it runs again (the workspace-finalize barrier) > - "Recover saved files" saves the retained work of a done child and records a successful `workspace_finalize` > - That finalize row had no `execution_workspace_id`. The barrier reads finalize rows by the blocker's workspace, so it never saw the row > - The parent's saved Retry then waited forever with "Saving its files" on a child that had already saved its files > - This pull request lets the barrier accept the recovery's own finalize for the recovered task's finished operations of the same run. It does not make the recovery finalize a workspace-wide save > - The benefit is that recoveries work by themselves: the parent's dependency wait releases and its saved Retry starts on the next dependency pass, with no migration ## Linked Issues or Issue Description **What happened?** A parent task had three done children. One legacy child shared the parent's execution workspace. The operator pressed "Recover saved files" on that child. The recovery saved the files and marked the old run succeeded. The operator then pressed Retry on the parent. The server accepted the Retry, but the parent stayed on "Retry is waiting for a finished task this one depends on to save its files" for more than 15 minutes. `blockerAttention.pendingFinalizeBlockerIssueIds` named the recovered child. The child had no active recovery action and no execution blocker. The workspace was clean (`dirty: false`, `writerRunId: null`). The child's workspace operations showed the cause. The latest operation on the workspace was a succeeded `workspace_recovery`. The recovery's succeeded `workspace_finalize` existed, but with `execution_workspace_id = NULL`. Earlier finalize attempts of that run had failed. `pendingFinalizeBlockerCondition` only looks at operations on the blocker's workspace, so it found no successful finalize after the recovery. **Expected behavior** When "Recover saved files" saves a done child's work, the child must stop holding its dependents. The parent's saved Retry must start without another operator action. **Steps to reproduce** 1. Let a child task finish as `done` with a retained remote workspace whose finalize failed. 2. Run "Recover saved files" on the child, so recovery saves the remote workspace snapshot. 3. Block a parent task on the child. The parent can share the child's workspace. 4. Read the parent's dependency readiness. Before this fix it reports the child in `pendingFinalizeBlockerIssueIds` forever. **Paperclip version or commit** Seen on staging at `a46b9ac7`. Reproduced on `master` at `e039be9202`. ## What Changed - `server/src/services/issues.ts` (`pendingFinalizeBlockerCondition`): the blocker task's latest workspace operation is settled when a succeeded recovery finalize (`owningService: workspace_recovery`) from the same company, task, and run started at or after that operation finished. The operation must be finished (`succeeded`, `failed`, or `skipped`). The recovery finalize can have no workspace (all rows written so far) or this workspace. - The rule is scoped to the blocker task and its run. It does not change the workspace-wide `finalized_at`, so other tasks that share the workspace keep their own barrier. - `server/src/services/workspace-recovery-dependency-finalize.test.ts`: new regression tests that run the real remote "Recover saved files" path, make the child `done`, and block a parent that shares the child's workspace. - `doc/execution-semantics.md`: documents how the recovery finalize settles the dependency barrier. ## Verification - Red (unfixed `issues.ts`): `npx vitest run src/services/workspace-recovery-dependency-finalize.test.ts` fails 3 of 4 tests. The parent stays not ready with the recovered child in `pendingFinalizeBlockerIssueIds`. - Green (fixed): the same file passes 4 of 4 tests: - a real recovery releases the parent in `getDependencyReadiness` and in the issue list `blockerAttention`; - snapshot-fallback shape: the preservation receipt has no workspace and the child's latest operation is the run's failed finalize. The parent is released; - safety: a sibling task that started work on the shared workspace while the recovery waited for teardown still holds the parent; - safety: child work that starts after the recovery still holds the parent. - Focused suites pass: `src/__tests__/issues-service.test.ts`, `src/services/workspace-recovery-admission.test.ts`, `src/services/task-retry.test.ts`, `src/services/saved-bootstrap-continuation.test.ts`, `src/services/remote-workspace-recovery-receipt-fallback.test.ts`, `src/__tests__/issue-dependency-wakeups-routes.test.ts` (414 tests with the new file). - `pnpm verify` exits 2 (a doc change is a global CI input), so required CI supplies broad coverage. Server `tsc --noEmit` passes. ## Risks - Previous-release story: no persisted shape changes. Recovery finalize rows already saved (with no workspace) are read by the new rule. Affected parents become dependency-ready on the next read, with no migration or backfill. The deferred-receipt and dependency backstop passes then start the saved Retry. - The barrier keeps its protection. Unfinished operations, operations from another run, operations that start after the recovery finalize, and other tasks on a shared workspace still wait for their own save. - No schema change and no write-path change. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, used through Claude Code with tool use (shell, code search, test execution, read-only staging API inspection). ## 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 - [x] All Paperclip CI gates are green - [x] 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
1ac8b6a711
commit
f125b823f4
3 files changed
+106
No files matched your search
@@ -235,6 +235,15 @@ Workspace readiness accepts a completed operation enclosed by a successful
|
||||
finalize from the same company, workspace, issue, and run. A nested snapshot
|
||||
that finishes before that finalize finishes does not leave a permanent wait.
|
||||
Unfinished operations and work after the finalize still require a later save.
|
||||
Saved-work recovery ("Recover saved files") ends with its own successful
|
||||
finalize for the recovered run. That finalize settles the task's finished
|
||||
workspace operations from the same run that started before it, including on a
|
||||
workspace that a legacy done child shares with its parent. So the dependents
|
||||
of a recovered done child stop waiting, and a saved Retry starts on the next
|
||||
dependency pass. Recovery finalizes already saved by earlier releases count,
|
||||
with no migration. The recovery finalize never counts as a workspace-wide
|
||||
save: another task that started work on a shared workspace while the recovery
|
||||
waited for sandbox teardown still waits for its own save.
|
||||
|
||||
Daytona file export can preserve an already captured workspace archive when the
|
||||
guarded download copy runs out of sandbox capacity. It removes partial copies,
|
||||
|
||||
@@ -2606,6 +2606,20 @@ function pendingFinalizeBlockerCondition(companyId: string, issueId: SQL, worksp
|
||||
and owning_finalize.finished_at >= latest_blocker_operation.finished_at
|
||||
)
|
||||
)
|
||||
and not (
|
||||
latest_blocker_operation.status in ('succeeded', 'failed', 'skipped')
|
||||
and exists (
|
||||
select 1 from workspace_operations recovery_finalize
|
||||
where recovery_finalize.company_id = ${companyId}::uuid
|
||||
and recovery_finalize.issue_id = ${issueId}
|
||||
and recovery_finalize.heartbeat_run_id = latest_blocker_operation.heartbeat_run_id
|
||||
and recovery_finalize.phase = 'workspace_finalize' and recovery_finalize.status = 'succeeded'
|
||||
and recovery_finalize.metadata->>'owningService' = 'workspace_recovery'
|
||||
and (recovery_finalize.execution_workspace_id is null
|
||||
or recovery_finalize.execution_workspace_id = ${workspaceId})
|
||||
and recovery_finalize.started_at >= coalesce(latest_blocker_operation.finished_at, latest_blocker_operation.started_at)
|
||||
)
|
||||
)
|
||||
)`;
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,83 @@
|
||||
import { and, eq } from "drizzle-orm";
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
import type { AdapterSandboxExecutionTarget } from "@paperclipai/adapter-utils/execution-target";
|
||||
import { issueRelations, issues, workspaceOperations } from "@paperclipai/db";
|
||||
import { issueService } from "./issues.js";
|
||||
import { recoverRetainedWorkspace } from "./workspace-recovery.js";
|
||||
import { readWorkspaceRecovery } from "./workspace-recovery-receipt.js";
|
||||
import { useWorkspaceRecoveryFixture, acceptedRemoteExport, action, db } from "./__tests__/workspace-recovery-fixture.js";
|
||||
|
||||
const targets = vi.hoisted(() => new Map<string, AdapterSandboxExecutionTarget>());
|
||||
vi.mock("./environment-execution-target.js", async importOriginal => {
|
||||
const actual = await importOriginal<typeof import("./environment-execution-target.js")>();
|
||||
return { ...actual, resolveEnvironmentExecutionTarget: async (input: Parameters<typeof actual.resolveEnvironmentExecutionTarget>[0]) =>
|
||||
(input.leaseId ? targets.get(input.leaseId) : undefined) ?? actual.resolveEnvironmentExecutionTarget(input) };
|
||||
});
|
||||
|
||||
describe("dependency readiness after a done child's work is recovered", () => {
|
||||
useWorkspaceRecoveryFixture(targets);
|
||||
|
||||
async function recoveredDoneChildWithParent() {
|
||||
const f = await acceptedRemoteExport();
|
||||
await recoverRetainedWorkspace(f.input);
|
||||
expect(readWorkspaceRecovery((await action(f.actionId)).evidence)?.status).toBe("recovered");
|
||||
const [child] = await db.select().from(issues).where(eq(issues.id, f.issueId));
|
||||
await db.update(issues).set({ status: "done" }).where(eq(issues.id, f.issueId));
|
||||
const [parent] = await db.insert(issues).values({ companyId: f.companyId, title: "Parent",
|
||||
status: "blocked", priority: "medium", assigneeAgentId: child.assigneeAgentId,
|
||||
executionWorkspaceId: f.workspaceId }).returning();
|
||||
await db.insert(issueRelations).values({ companyId: f.companyId, issueId: f.issueId,
|
||||
relatedIssueId: parent.id, type: "blocks" });
|
||||
return { ...f, parentId: parent.id };
|
||||
}
|
||||
|
||||
const recoveryFinalize = async (f: { companyId: string; runId: string }) => (await db.select().from(workspaceOperations).where(and(
|
||||
eq(workspaceOperations.companyId, f.companyId), eq(workspaceOperations.heartbeatRunId, f.runId),
|
||||
eq(workspaceOperations.phase, "workspace_finalize"), eq(workspaceOperations.status, "succeeded"))))[0]!;
|
||||
const readiness = (id: string) => issueService(db).getDependencyReadiness(id);
|
||||
|
||||
it("releases the parent once Recover saved files has saved a done child's work", async () => {
|
||||
const f = await recoveredDoneChildWithParent();
|
||||
await expect(readiness(f.parentId)).resolves.toMatchObject({
|
||||
isDependencyReady: true, pendingFinalizeBlockerIssueIds: [], unresolvedBlockerIssueIds: [] });
|
||||
const listed = (await issueService(db).list(f.companyId, { status: "blocked" }) as Array<{ id: string;
|
||||
blockerAttention?: { pendingFinalizeBlockerIssueIds?: string[] } | null }>).find(issue => issue.id === f.parentId);
|
||||
expect(listed?.blockerAttention?.pendingFinalizeBlockerIssueIds ?? []).toEqual([]);
|
||||
}, 30_000);
|
||||
|
||||
it("releases the parent when the recovery saved through a snapshot without a workspace", async () => {
|
||||
const f = await recoveredDoneChildWithParent();
|
||||
const finalize = await recoveryFinalize(f);
|
||||
await db.update(workspaceOperations).set({ executionWorkspaceId: null }).where(and(
|
||||
eq(workspaceOperations.companyId, f.companyId), eq(workspaceOperations.heartbeatRunId, f.runId),
|
||||
eq(workspaceOperations.phase, "workspace_recovery")));
|
||||
await db.insert(workspaceOperations).values({ companyId: f.companyId, issueId: f.issueId, heartbeatRunId: f.runId,
|
||||
executionWorkspaceId: f.workspaceId, phase: "workspace_finalize", status: "failed",
|
||||
startedAt: new Date(finalize.startedAt.getTime() - 60_000), finishedAt: new Date(finalize.startedAt.getTime() - 59_000) });
|
||||
await expect(readiness(f.parentId)).resolves.toMatchObject({
|
||||
isDependencyReady: true, pendingFinalizeBlockerIssueIds: [] });
|
||||
}, 30_000);
|
||||
|
||||
it("keeps a sibling's work that started during the recovery waiting for its own save", async () => {
|
||||
const f = await recoveredDoneChildWithParent();
|
||||
const finalize = await recoveryFinalize(f);
|
||||
const [sibling] = await db.insert(issues).values({ companyId: f.companyId, title: "Sibling", status: "done",
|
||||
priority: "medium", executionWorkspaceId: f.workspaceId }).returning();
|
||||
await db.insert(issueRelations).values({ companyId: f.companyId, issueId: sibling.id, relatedIssueId: f.parentId, type: "blocks" });
|
||||
await db.insert(workspaceOperations).values({ companyId: f.companyId, issueId: sibling.id,
|
||||
executionWorkspaceId: f.workspaceId, phase: "worktree_prepare", status: "succeeded",
|
||||
startedAt: new Date(finalize.startedAt.getTime() - 1_000), finishedAt: new Date(finalize.startedAt.getTime() - 500) });
|
||||
await expect(readiness(f.parentId)).resolves.toMatchObject({
|
||||
isDependencyReady: false, pendingFinalizeBlockerIssueIds: [sibling.id] });
|
||||
}, 30_000);
|
||||
|
||||
it("keeps the barrier for child workspace work started after the recovery", async () => {
|
||||
const f = await recoveredDoneChildWithParent();
|
||||
const finalize = await recoveryFinalize(f);
|
||||
await db.insert(workspaceOperations).values({ companyId: f.companyId, issueId: f.issueId,
|
||||
executionWorkspaceId: f.workspaceId, phase: "worktree_prepare", status: "succeeded",
|
||||
startedAt: new Date(finalize.startedAt.getTime() + 1_000), finishedAt: new Date(finalize.startedAt.getTime() + 2_000) });
|
||||
await expect(readiness(f.parentId)).resolves.toMatchObject({
|
||||
isDependencyReady: false, pendingFinalizeBlockerIssueIds: [f.issueId] });
|
||||
}, 30_000);
|
||||
});
|
||||
Reference in new issue
Block a user