diff --git a/doc/execution-semantics.md b/doc/execution-semantics.md index e749807753..2a99d4204a 100644 --- a/doc/execution-semantics.md +++ b/doc/execution-semantics.md @@ -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. diff --git a/server/src/__tests__/issue-comment-reopen-routes.test.ts b/server/src/__tests__/issue-comment-reopen-routes.test.ts index ee148f1356..782628b1eb 100644 --- a/server/src/__tests__/issue-comment-reopen-routes.test.ts +++ b/server/src/__tests__/issue-comment-reopen-routes.test.ts @@ -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) => ({ @@ -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" }), ); }); diff --git a/server/src/__tests__/issues-service.test.ts b/server/src/__tests__/issues-service.test.ts index 3b91c8856c..cddd7cbaa2 100644 --- a/server/src/__tests__/issues-service.test.ts +++ b/server/src/__tests__/issues-service.test.ts @@ -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 diff --git a/server/src/__tests__/recovery-stale-issue-lock-sweep.test.ts b/server/src/__tests__/recovery-stale-issue-lock-sweep.test.ts index 2cc53380fb..c68ee4595d 100644 --- a/server/src/__tests__/recovery-stale-issue-lock-sweep.test.ts +++ b/server/src/__tests__/recovery-stale-issue-lock-sweep.test.ts @@ -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)); diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index e587b51a2e..2829dcb989 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -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 = diff --git a/server/src/services/issues.ts b/server/src/services/issues.ts index 1197e3fbae..cf582398e3 100644 --- a/server/src/services/issues.ts +++ b/server/src/services/issues.ts @@ -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); diff --git a/server/src/services/recovery/service.ts b/server/src/services/recovery/service.ts index 0361621ec1..09f2332461 100644 --- a/server/src/services/recovery/service.ts +++ b/server/src/services/recovery/service.ts @@ -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({