RomanNum3ral
/
paperclip
mirror of https://github.com/paperclipai/paperclip.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

fix: preserve active runs through terminal issue writes 

Co-Authored-By: Paperclip <noreply@paperclip.ing>
This commit is contained in:
Chris 2026-09-11 17:03:40 -04:00
parent 663c44cb2b
commit 79ffbbaae0
7 changed files with 147 additions and 52 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
doc/execution-semantics.md
View File

@ -141,6 +141,8 @@ The active-lock lifecycle is part of the checkout contract:
- process-loss retry handoff must not leave `checkoutRunId` pinned to the failed run when `executionRunId` moves to the retry run
- checkout and checkout-owner checks may self-heal lock columns that point at terminal or missing runs before evaluating conflicts
- the recovery sweeper may clear rows whose checkout and execution locks all point at terminal or missing runs
- a terminal issue status does not prove its agent process has stopped; recovery must not terminalize a run while its in-memory execution owner is still active
- a run-scoped checkout request is valid only while the requesting run is queued or running
Stale-lock recovery is crash recovery, not a retry loop. Paperclip must not clear or adopt locks held by non-terminal runs. After stale cleanup, a checkout `409` should mean a real live owner, status/assignee mismatch, unresolved blocker, or active gate still prevents checkout. Agents must treat that `409` as an ownership conflict and stop rather than retrying the same checkout.

12
server/src/__tests__/issue-comment-reopen-routes.test.ts
View File

@ -1770,11 +1770,11 @@ describe.sequential("issue comment reopen routes", () => {
);
});
it("still implicitly reopens done issues via POST comments when the comment runId differs from the issue's owning run", async () => {
it("does not implicitly reopen done issues via POST comments when finalization already cleared the comment run's lock", async () => {
mockIssueService.getById.mockResolvedValue({
...makeIssue("done"),
checkoutRunId: "run-owning",
executionRunId: "run-owning",
checkoutRunId: null,
executionRunId: null,
});
mockIssueService.update.mockImplementation(
async (_id: string, patch: Record<string, unknown>) => ({
@ -1794,12 +1794,12 @@ describe.sequential("issue comment reopen routes", () => {
}),
)
.post("/api/issues/11111111-1111-4111-8111-111111111111/comments")
.send({ body: "Real human follow-up — please reopen" });
.send({ body: "Done — final note after the run lock was released" });
expect(res.status).toBe(201);
expect(mockIssueService.update).toHaveBeenCalledWith(
expect(mockIssueService.update).not.toHaveBeenCalledWith(
"11111111-1111-4111-8111-111111111111",
{ status: "todo" },
expect.objectContaining({ status: "todo" }),
);
});

70
server/src/__tests__/issues-service.test.ts
View File

@ -6076,6 +6076,76 @@ describeEmbeddedPostgres("issueService.clearExecutionRunIfTerminal", () => {
});
});
it("checkout refuses a terminal actor run before it can reclaim an issue", async () => {
const companyId = randomUUID();
const agentId = randomUUID();
const issueId = randomUUID();
const terminalRunId = randomUUID();
await db.insert(companies).values({
id: companyId,
name: "Paperclip",
issuePrefix: `T${companyId.replace(/-/g, "").slice(0, 6).toUpperCase()}`,
requireBoardApprovalForNewAgents: false,
});
await db.insert(agents).values({
id: agentId,
companyId,
name: "CodexCoder",
role: "engineer",
status: "active",
adapterType: "codex_local",
adapterConfig: {},
runtimeConfig: {},
permissions: {},
});
await db.insert(heartbeatRuns).values({
id: terminalRunId,
companyId,
agentId,
status: "succeeded",
invocationSource: "manual",
startedAt: new Date("2026-06-10T10:00:00.000Z"),
finishedAt: new Date("2026-06-10T10:01:00.000Z"),
});
await db.insert(issues).values({
id: issueId,
companyId,
title: "Issue reopened before a terminal run tried to reclaim it",
status: "todo",
priority: "high",
assigneeAgentId: agentId,
});
await expect(
svc.checkout(issueId, agentId, ["todo", "in_progress"], terminalRunId),
).rejects.toMatchObject({
status: 409,
details: {
code: "issue_checkout_run_not_live",
checkoutRunId: terminalRunId,
runStatus: "succeeded",
},
});
const row = await db
.select({
status: issues.status,
checkoutRunId: issues.checkoutRunId,
executionRunId: issues.executionRunId,
startedAt: issues.startedAt,
})
.from(issues)
.where(eq(issues.id, issueId))
.then((rows) => rows[0]);
expect(row).toEqual({
status: "todo",
checkoutRunId: null,
executionRunId: null,
startedAt: null,
});
});
it("checkout adoption of a stale checkoutRunId preserves the issue's assigneeUserId", async () => {
// Regression for PR #2482 checkout-adoption review finding: any adoption
// helper that re-locks an existing in_progress issue (e.g. when the prior

9
server/src/__tests__/recovery-stale-issue-lock-sweep.test.ts
View File

@ -379,14 +379,14 @@ describeEmbeddedPostgres("recovery sweepStaleIssueLocks", () => {
.resolves.toEqual([{ checkoutRunId: runningRunId, executionRunId: runningRunId }]);
});
it("preserves a process-less run while its in-process execution is still finalizing", async () => {
it("preserves a terminal issue's run while its in-process execution is still finalizing", async () => {
const { companyId, agentId, runningRunId } = await seed();
const issueId = randomUUID();
await db.insert(issues).values({
id: issueId,
companyId,
title: "Native finalization remains live",
status: "in_progress",
title: "Terminal issue while executor remains live",
status: "done",
priority: "high",
assigneeAgentId: agentId,
checkoutRunId: runningRunId,
@ -396,8 +396,7 @@ describeEmbeddedPostgres("recovery sweepStaleIssueLocks", () => {
await db
.update(heartbeatRuns)
.set({
runtimeMode: "native",
processPid: 2_000_000_000,
processPid: process.pid,
})
.where(eq(heartbeatRuns.id, runningRunId));

27
server/src/routes/issues.ts
View File

@ -2276,10 +2276,7 @@ function shouldImplicitlyMoveCommentedIssueToTodo(input: {
issueStatus: string | null | undefined;
assigneeAgentId: string | null | undefined;
actorType: "agent" | "user";
actorId: string;
actorRunId: string | null | undefined;
checkoutRunId: string | null | undefined;
executionRunId: string | null | undefined;
requestAddsExplicitBlockers?: boolean;
}) {
// A request that wires a non-empty blockedByIssueIds list is declaring that
@ -2288,18 +2285,12 @@ function shouldImplicitlyMoveCommentedIssueToTodo(input: {
// edits — flipping to todo here would contradict the caller's stated intent
// in the same request.
if (input.requestAddsExplicitBlockers) return false;
// Local-CLI agents post comments under user auth, so the actor.type is "user"
// even though the comment originates from the same heartbeat run that owns
// the issue lock. Without this guard, an agent that closes its own issue and
// then posts a follow-up comment in the same run silently reopens it.
// Suppress the implicit move whenever the comment's source run matches the
// issue's checkout/execution run.
if (
typeof input.actorRunId === "string" &&
input.actorRunId.length > 0 &&
(input.actorRunId === input.checkoutRunId ||
input.actorRunId === input.executionRunId)
) {
// Local-CLI agents post comments under user auth, so actor.type alone cannot
// distinguish a human comment from a run-originated one. Run finalization can
// clear the issue lock before the agent posts its final comment, so equality
// with the current lock is not a reliable discriminator. Any non-empty run id
// means the request is run-originated and must require an explicit resume.
if (typeof input.actorRunId === "string" && input.actorRunId.length > 0) {
return false;
}
// Only human comments should implicitly reopen finished work.
@ -12816,10 +12807,7 @@ export function issueRoutes(
issueStatus: existing.status,
assigneeAgentId: requestedAssigneeAgentId,
actorType: actor.actorType,
actorId: actor.actorId,
actorRunId: actor.runId,
checkoutRunId: existing.checkoutRunId,
executionRunId: existing.executionRunId,
requestAddsExplicitBlockers:
Array.isArray(req.body.blockedByIssueIds) &&
req.body.blockedByIssueIds.length > 0,
@ -17053,10 +17041,7 @@ export function issueRoutes(
issueStatus: issue.status,
assigneeAgentId: issue.assigneeAgentId,
actorType: actor.actorType,
actorId: actor.actorId,
actorRunId: actor.runId,
checkoutRunId: issue.checkoutRunId,
executionRunId: issue.executionRunId,
}) ||
shouldResumeInProgressScheduledRetry);
const hasUnresolvedFirstClassBlockers =

30
server/src/services/issues.ts
View File

@ -11356,6 +11356,36 @@ export function issueService(db: Db) {
});
}
if (checkoutRunId) {
const checkoutRun = await db
.select({
status: heartbeatRuns.status,
agentId: heartbeatRuns.agentId,
})
.from(heartbeatRuns)
.where(
and(
eq(heartbeatRuns.id, checkoutRunId),
eq(heartbeatRuns.companyId, issueCompany.companyId),
),
)
.then((rows) => rows[0] ?? null);
if (
!checkoutRun ||
checkoutRun.agentId !== agentId ||
!ACTIVE_RUN_STATUSES.includes(checkoutRun.status)
) {
throw conflict("Issue checkout requires a live owning run", {
code: "issue_checkout_run_not_live",
issueId: id,
actorAgentId: agentId,
checkoutRunId,
runStatus:
checkoutRun?.agentId === agentId ? checkoutRun.status : null,
});
}
}
await clearExecutionRunIfTerminal(id);
await clearCheckoutRunIfTerminal(id);

49
server/src/services/recovery/service.ts
View File

@ -5445,16 +5445,17 @@ export function recoveryService(
// state is auditable. It never overwrites a status that another path already
// made terminal.
//
// Two independent authorities terminalize the run. Either one is enough:
// Two independent authorities can terminalize the run after its in-memory
// execution owner is gone:
//
// - Issue-terminal authority: the run's issue already reached a terminal
// status (done or cancelled), but the run row is still "running". A healthy
// run always terminalizes its own row before or just after the issue reaches
// a terminal status, so a lasting "running" row under a terminal issue is
// orphaned. This authority does not depend on process death. It is the only
// authority that catches the reuse-lease path: the release stops the sandbox
// but keeps the server process alive, so the in-memory handle and the
// recorded pid can both persist.
// orphaned. This authority does not depend on recorded process death. It
// catches the reuse-lease path after its in-memory execution owner is gone:
// the release stops the sandbox but keeps the server process alive, so the
// recorded pid can persist.
// - Process-death authority: the run has no in-memory handle and its recorded
// process and process group are both gone. This catches a hard server crash
// that skipped the graceful teardown, even when the issue is not terminal.
@ -5484,11 +5485,23 @@ export function recoveryService(
if (isNativeRunnerOwnershipHeld(run))
return { terminalized: false, status: run.status };
// A live in-memory execution is the strongest ownership signal. The agent
// can set its issue to a terminal status before the enclosing heartbeat
// finishes its output, telemetry, and run finalization. Terminalizing here
// would race that still-running executor, release its checkout lock, and
// reject its remaining run-scoped writes as ownership conflicts.
const hasLiveExecution =
deps.liveRunExecutions?.has(run.id) ?? runningProcesses.has(run.id);
if (hasLiveExecution) {
return { terminalized: false, status: run.status };
}
const pid = run.processPid ?? null;
const processGroupId = run.processGroupId ?? null;
// Issue-terminal authority. When the run's issue is terminal, the run row is
// orphaned regardless of process or handle state. Prefer the referencing
// Issue-terminal authority. When the run's issue is terminal and no live
// execution owns it, the run row is orphaned regardless of recorded process
// state. Prefer the referencing
// issue status that the caller passed, because a lock column is the direct
// link from the stuck "Live" issue to this run. Fall back to the issue id in
// the run context snapshot when the caller passed nothing. Skip the fallback
@ -5525,24 +5538,20 @@ export function recoveryService(
// group. Require recorded process metadata, so this authority never fires
// on a run that has not yet stored its pid.
let processGone = false;
const hasLiveExecution =
deps.liveRunExecutions?.has(run.id) ?? runningProcesses.has(run.id);
if (!hasLiveExecution) {
if (typeof pid === "number" || typeof processGroupId === "number") {
const processAlive =
(typeof pid === "number" && isPidAlive(pid)) ||
(typeof processGroupId === "number" &&
isProcessGroupAlive(processGroupId));
processGone = !processAlive;
}
if (typeof pid === "number" || typeof processGroupId === "number") {
const processAlive =
(typeof pid === "number" && isPidAlive(pid)) ||
(typeof processGroupId === "number" &&
isProcessGroupAlive(processGroupId));
processGone = !processAlive;
}
// A result-less native run may intentionally have no live provider process
// while the native finalization coordinator waits to resume the same
// provider session. That coordinator, rather than this generic
// process-death backstop, owns retryable/resumed attempts. Preserve issue
// terminality as the stronger authority, but never interrupt coordinator-
// owned recovery merely because the provider process has exited.
// process-death backstop, owns retryable/resumed attempts. In the absence of
// a terminal issue, never interrupt coordinator-owned recovery merely
// because the provider process has exited.
if (!issueTerminalStatus && processGone && run.runtimeMode === "native") {
const coordinator = await db
.select({