mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-04 19:28:46 +01:00
fix(heartbeat): keep a routine claim queued while its slot is held (#18933)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - A routine creates one task per tick. Only one open task of a routine can hold an execution lock (`issues_open_routine_execution_uq`). > - Since #18879, a failed native run that waits for a scheduled resume keeps its task's lock. Routine dispatch counts only queued, running or scheduled runs as live, so it can queue a newer task of the same routine meanwhile. > - When that newer run is claimed, stamping its execution lock violates the unique index. The raw database error is not a deferrable claim rejection, so it escapes the queue loop. The queue code notes that this can stop the agent's queue and startup recovery. > - This pull request turns that exact violation into a 409 claim rejection, so the run stays queued until the slot is free. > - The benefit is that a busy routine slot delays one run instead of stopping the queue or startup. ## Linked Issues or Issue Description Refs #18879 (keeps a failed run's lock while its resume is pending), #18916 (defers the same unique-index conflict in restart recovery's lock reclaim) and #18931 (open; keeps one run's restart-recovery failure from stopping startup recovery). This is the same index conflict on a different path: the heartbeat queue claim, not restart recovery. It does not change #18916 or #18931. Its doc paragraph is separate from the one #18931 edits. **What happened?** While auditing the lock paths from #18879 for this index, I found that a queued run of a newer routine task can be claimed while an older task of the same routine still holds the slot through a failed run with a pending resume. The claim's lock stamp then raises `duplicate key value violates unique constraint "issues_open_routine_execution_uq"`. `isDeferrableClaimRejection` accepts only `HttpError` 4xx, so the error escapes `startNextQueuedRunForAgent`. **Expected behavior** The claim is rejected as a conflict and rolls back. The run stays queued and is claimed after the older task releases the slot. Other errors behave as before. **Steps to reproduce** 1. Create two open tasks of the same routine (same origin and fingerprint). 2. Give the older task an execution lock held by a failed run. 3. Queue a run for the newer task and claim it. On `master` the claim throws the unique violation. The new queue test runs these steps against PostgreSQL. **Paperclip version or commit** `master` at `0dedbf23c35`. **Deployment mode** Any server with routines whose tasks run on native runners. ## What Changed - `bindClaimedIssueExecution` in `server/src/services/heartbeat/queue.ts`: if stamping the execution lock violates `issues_open_routine_execution_uq`, it throws `conflict("Another execution for this routine is already in progress")`. The claim transaction rolls back, and the queue loop keeps the run queued as it does for other 4xx claim rejections. Every other error is rethrown unchanged. The issue checkout route already maps this violation to the same 409. - `doc/execution-semantics.md`: one paragraph on the behavior. ## Verification - Failing first: `queue.test.ts` "keeps a claim queued while another task of its routine holds the execution slot". On `master` the claim raised the `issues_open_routine_execution_uq` violation. Now it rejects with 409, the run stays `queued`, neither task's lock changes, and nothing executes. - Failing first, review round 1: `heartbeat-queued-run-claim-isolation.test.ts` "keeps a routine run queued while another task holds the routine slot, then runs it once the slot is free" goes through `resumeQueuedRuns`. On `master` it rejected with the unique violation. Now the routine run stays queued with no error code while another queued run for the same agent executes. After the slot is released, the routine run executes exactly once (executions are counted per run, because the finished task can queue its own follow-up run). - `cd server && npx vitest run src/services/heartbeat/queue.test.ts src/__tests__/heartbeat-queued-run-claim-isolation.test.ts`: 70 passed. - `npx vitest run src/services/heartbeat/recovery.test.ts src/__tests__/routines*.test.ts`: 4 files, 205 passed. - `pnpm --filter @paperclipai/server typecheck`: pass. - Not verified: the queue loop end to end on a real instance. The loop's handling of a 4xx claim rejection is existing behavior. ## Risks - While the older task holds the slot, the newer run stays queued and the loop logs "queued heartbeat run claim was rejected" on each pass. It is claimed once the older task's resume finishes or its lock is released. - Only this one constraint is mapped. Other database errors still escape as before. - No schema migration. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`) through Claude Code, with tool use (shell, file edits, test runs). The runtime does not expose the context window or reasoning configuration. ## 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 - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [ ] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
b4c69ded9f
commit
1a39dea9aa
4 files changed
+83
-1
No files matched your search
@@ -1265,6 +1265,12 @@ checks coordinator ownership in its terminal update, so a result recorded after
|
||||
the process check is also protected. Terminal task status remains authoritative;
|
||||
exhausted finalization retries do not keep an otherwise orphaned run alive.
|
||||
|
||||
Only one open task of a routine can hold an execution lock. A failed run that
|
||||
waits for a scheduled resume keeps its task's lock, so the routine can queue a
|
||||
newer task meanwhile. When that task's run is claimed while the slot is held,
|
||||
the claim is rejected with a conflict, rolls back, and the run stays queued
|
||||
until the slot is released.
|
||||
|
||||
A native failure that schedules a same-run resume marks the run `failed` but
|
||||
keeps the task's execution and checkout locks for that resume. The sweep leaves
|
||||
those locks in place and clears them only when the task is done or cancelled.
|
||||
|
||||
@@ -4,10 +4,13 @@ import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest
|
||||
import {
|
||||
agents,
|
||||
agentWakeupRequests,
|
||||
authUsers,
|
||||
companies,
|
||||
companyMemberships,
|
||||
createDb,
|
||||
heartbeatRuns,
|
||||
externalAgentHolds,
|
||||
issues,
|
||||
} from "@paperclipai/db";
|
||||
import {
|
||||
getEmbeddedPostgresTestSupport,
|
||||
@@ -231,6 +234,49 @@ describeEmbeddedPostgres("heartbeat queued-run claim isolation", () => {
|
||||
expect(await runStatus(healthyRunId)).toMatchObject({ status: "succeeded" });
|
||||
});
|
||||
|
||||
it("keeps a routine run queued while another task holds the routine slot, then runs it once the slot is free", async () => {
|
||||
const { companyId, agentId } = await insertAgent();
|
||||
const origin = { originKind: "routine_execution", originId: randomUUID(), originFingerprint: "scheduled" };
|
||||
// The earlier task keeps its lock while its failed run waits for a scheduled resume.
|
||||
const holderIssueId = randomUUID();
|
||||
const holderRunId = randomUUID();
|
||||
await db.insert(issues).values({ id: holderIssueId, companyId, title: "Earlier routine task",
|
||||
status: "in_progress", assigneeAgentId: agentId, ...origin });
|
||||
await db.insert(heartbeatRuns).values({ id: holderRunId, companyId, agentId, status: "failed",
|
||||
finishedAt: new Date(), contextSnapshot: { issueId: holderIssueId } });
|
||||
await db.update(issues).set({ executionRunId: holderRunId }).where(sql`${issues.id} = ${holderIssueId}`);
|
||||
const userId = `routine-owner-${randomUUID()}`;
|
||||
await db.insert(authUsers).values({ id: userId, name: "Routine owner", email: `${userId}@paperclip.test`,
|
||||
createdAt: new Date(), updatedAt: new Date() });
|
||||
await db.insert(companyMemberships).values({ companyId, principalType: "user", principalId: userId,
|
||||
status: "active", membershipRole: "owner" });
|
||||
const routineIssueId = randomUUID();
|
||||
await db.insert(issues).values({ id: routineIssueId, companyId, title: "Current routine task",
|
||||
status: "todo", assigneeAgentId: agentId, responsibleUserId: userId, ...origin });
|
||||
const { runId: routineRunId } = await insertQueuedRun(companyId, agentId, {
|
||||
requestedByActorType: "user", requestedByActorId: userId,
|
||||
payload: { manualUserWake: true, issueId: routineIssueId },
|
||||
}, new Date(Date.now() - 60_000));
|
||||
await db.update(heartbeatRuns).set({ contextSnapshot: { issueId: routineIssueId } })
|
||||
.where(sql`${heartbeatRuns.id} = ${routineRunId}`);
|
||||
const { runId: healthyRunId } = await insertClaimableRun(companyId, agentId);
|
||||
|
||||
await expect(heartbeat.resumeQueuedRuns()).resolves.toBeUndefined();
|
||||
await heartbeat.drainActiveRunExecutions();
|
||||
expect(await runStatus(routineRunId)).toMatchObject({ status: "queued", errorCode: null });
|
||||
expect(await runStatus(healthyRunId)).toMatchObject({ status: "succeeded" });
|
||||
expect(mockAdapterExecute).toHaveBeenCalledOnce();
|
||||
|
||||
await db.update(issues).set({ executionRunId: null }).where(sql`${issues.id} = ${holderIssueId}`);
|
||||
await expect(heartbeat.resumeQueuedRuns()).resolves.toBeUndefined();
|
||||
await heartbeat.drainActiveRunExecutions();
|
||||
expect(await runStatus(routineRunId)).toMatchObject({ status: "succeeded" });
|
||||
// Count executions per run: the finished task may queue its own follow-up run.
|
||||
const executedRunIds = mockAdapterExecute.mock.calls.map(call => ((call as unknown[])[0] as { runId?: string }).runId);
|
||||
expect(executedRunIds.filter(id => id === routineRunId)).toHaveLength(1);
|
||||
expect(executedRunIds.filter(id => id === healthyRunId)).toHaveLength(1);
|
||||
});
|
||||
|
||||
it.each(["worker", "native-effects"])("retains a %s barrier across provider changes and resumes only after reconciliation", async barrier => {
|
||||
const { companyId, agentId } = await insertAgent();
|
||||
const { runId } = await insertClaimableRun(companyId, agentId);
|
||||
|
||||
@@ -693,6 +693,27 @@ describe.skipIf(!support.supported)("heartbeat queue database wiring", () => {
|
||||
expect(deps.canStartNewExternalBotWork).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("keeps a claim queued while another task of its routine holds the execution slot", async () => {
|
||||
const { company, agent, issue, run } = await fixture("queued");
|
||||
const origin = { originKind: "routine_execution", originId: crypto.randomUUID(), originFingerprint: "scheduled" };
|
||||
await db.update(issues).set({ ...origin, executionRunId: null, executionAgentNameKey: null, executionLockedAt: null })
|
||||
.where(eq(issues.id, issue.id));
|
||||
// An earlier task of the same routine keeps its lock while its failed run waits for a scheduled resume.
|
||||
const [holder] = await db.insert(issues).values({ companyId: company.id, title: "Earlier routine task",
|
||||
status: "in_progress", assigneeAgentId: agent.id, ...origin }).returning();
|
||||
const [holderRun] = await db.insert(heartbeatRuns).values({ companyId: company.id, agentId: agent.id,
|
||||
status: "failed", finishedAt: new Date(), contextSnapshot: { issueId: holder.id } }).returning();
|
||||
await db.update(issues).set({ executionRunId: holderRun.id }).where(eq(issues.id, holder.id));
|
||||
const deps = callbacks(db);
|
||||
deps.getAgent.mockResolvedValue(agent);
|
||||
await expect(createHeartbeatQueue(db, deps).claimQueuedRun({ ...run, responsibleUserId: "queue-test-user" }))
|
||||
.rejects.toMatchObject({ status: 409 });
|
||||
expect((await db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, run.id)))[0]).toMatchObject({ status: "queued" });
|
||||
expect((await db.select().from(issues).where(eq(issues.id, issue.id)))[0]).toMatchObject({ executionRunId: null });
|
||||
expect((await db.select().from(issues).where(eq(issues.id, holder.id)))[0]).toMatchObject({ executionRunId: holderRun.id });
|
||||
expect(deps.executeRun).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("cancels at the daily run cap before claiming or dispatching", async () => {
|
||||
const { run, agent } = await fixture("queued");
|
||||
const cappedAgent = { ...agent, runtimeConfig: { heartbeat: { maxDailyRuns: 0 } } };
|
||||
|
||||
@@ -112,6 +112,7 @@ import {
|
||||
notFound,
|
||||
} from "../../errors.js";
|
||||
import { logger } from "../../middleware/logger.js";
|
||||
import { isUniqueViolation } from "../../db-errors.js";
|
||||
import { publishLiveEvent } from "../live-events.js";
|
||||
import { allocateHeartbeatRunEventSeq } from "../heartbeat-run-events.js";
|
||||
import {
|
||||
@@ -1101,7 +1102,15 @@ export function createHeartbeatQueue(db: Db, dependencies: HeartbeatQueueDepende
|
||||
executionAgentNameKey: normalizeAgentNameKey(agent.name),
|
||||
executionLockedAt: claimedAt,
|
||||
updatedAt: claimedAt,
|
||||
}).where(and(eq(issues.id, issueId), eq(issues.companyId, run.companyId)));
|
||||
}).where(and(eq(issues.id, issueId), eq(issues.companyId, run.companyId))).catch((error: unknown) => {
|
||||
// Another open task of the same routine holds its execution slot, for
|
||||
// example a failed run that waits for its scheduled resume. Roll back
|
||||
// this claim and keep the run queued until that slot is released.
|
||||
if (isUniqueViolation(error, "issues_open_routine_execution_uq")) {
|
||||
throw conflict("Another execution for this routine is already in progress");
|
||||
}
|
||||
throw error;
|
||||
});
|
||||
}
|
||||
const nativeReviewContext = readNativeReviewAssignmentContext(context);
|
||||
const queuedCommentIds = queuedCommentIdsFromRunContext(context);
|
||||
|
||||
Reference in new issue
Block a user