From 6a4e2e1b8c7129f6f913ae458ab0be9cba50bd6a Mon Sep 17 00:00:00 2001 From: LeonSGP <154585401+LeonSGP43@users.noreply.github.com> Date: Mon, 10 Aug 2026 09:20:09 +0800 Subject: [PATCH] fix(routes): return 409 for routine checkout conflicts (#3790) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip orchestrates AI agents and relies on issue checkout as the core task-claiming primitive > - The issue checkout route is the HTTP boundary that translates service and database outcomes into agent-usable API responses > - Routine-linked issues are protected by the partial unique index `issues_open_routine_execution_uq`, which covers only rows whose `execution_run_id` is set > - `svc.checkout` sets `execution_run_id`, so a concurrent claim moves the row into that index and can raise a 23505 mid-request > - Unhandled, that surfaces as a 500 and crashes the agent run instead of being a recoverable conflict > - Drizzle wraps driver failures in its own `Failed query: ...` error, so the Postgres error carrying `code` and the constraint name is reachable only through `cause` > - This pull request translates that violation into a 409 at the checkout route, detecting it through the cause chain the way `isReviewPathRecoveryIdempotencyConflict` already does > - The benefit is that agents handle routine execution contention through the normal heartbeat conflict path instead of failing on an internal server error ## Linked Issues or Issue Description Fixes #3660 Related pull requests found while searching for duplicates: - #3699 — an earlier attempt at this same route-level fix, closed unmerged. Same shape, and its check has the flat-error bug described under Verification. - #3633 — related work on postgres.js `constraint_name` handling in conflict detection. - #5662 — covers the adoption path (`assertCheckoutOwner`) that this pull request does not. ## What Changed - Added `server/src/db-errors.ts` with `isUniqueViolation(error, constraintName?)`, which walks the `cause` chain (depth-capped) and accepts the postgres.js `constraint_name`, the node-postgres `constraint`, or the driver message as evidence of SQLSTATE 23505. - Wrapped `svc.checkout()` in `POST /issues/:id/checkout` with a narrow try/catch that uses that helper to return **409 Conflict** for `issues_open_routine_execution_uq`, and rethrows every other error unchanged. - Added `server/src/__tests__/db-errors.test.ts` covering the wrapped and bare error shapes, both constraint field names, the message fallback, non-matching constraints, non-unique-violation codes, and a self-referential cause chain. ## Verification - The new unit test includes the wrapped case `{ cause: { code: "23505", constraint_name: ... } }` that a flat `error.code` check fails, so it is a real regression guard rather than a restatement of the implementation. - The wrapped shape is what this codebase observes in practice: `server/src/__tests__/plugin-tenant-isolation.test.ts` asserts `cause?.code === "23505"` against embedded Postgres, `packages/db/src/pipelines-schema.test.ts` asserts that constraint failures throw `Failed query`, and `server/src/services/recovery/review-path-recovery.ts` walks the same chain. - CI (verify, e2e, policy) exercises this change against current master through the pull request merge ref. - Not verified locally: no monorepo install or typecheck was run in this environment. ## Risks - Low. One route gains a catch that matches a single constraint and rethrows all other errors, so no unrelated failure can be swallowed. - The 409 body `{ error: ... }` matches the other 409 responses this route already returns. - Scope limit: this covers the checkout route only. The adoption path reached through `assertCheckoutOwner` (heartbeat, plugins, and pipelines routes) can still surface the same violation as a 500; #5662 targets that path. - `isUniqueViolation` is new and intentionally generic. Existing flat 23505 checks elsewhere in the server are left untouched by this pull request. ## Model Used - Original change: OpenAI Codex, GPT-5-class tool-using coding agent in the Codex CLI environment; exact backend model revision is not exposed in that runtime. - Follow-up revision (cause-chain detection plus tests): Anthropic Claude Opus 5 (`claude-opus-5`), tool-using coding agent with extended thinking and code execution. ## 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) - [ ] 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 - [ ] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [ ] 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 - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Andrew Aymeloglu --- server/src/__tests__/db-errors.test.ts | 51 ++++++++++++++++++++++++++ server/src/db-errors.ts | 33 +++++++++++++++++ server/src/routes/issues.ts | 14 ++++++- 3 files changed, 97 insertions(+), 1 deletion(-) create mode 100644 server/src/__tests__/db-errors.test.ts create mode 100644 server/src/db-errors.ts diff --git a/server/src/__tests__/db-errors.test.ts b/server/src/__tests__/db-errors.test.ts new file mode 100644 index 0000000000..80dbb536cf --- /dev/null +++ b/server/src/__tests__/db-errors.test.ts @@ -0,0 +1,51 @@ +import { describe, expect, it } from "vitest"; +import { isUniqueViolation } from "../db-errors.js"; + +const CONSTRAINT = "issues_open_routine_execution_uq"; + +describe("isUniqueViolation", () => { + it("matches a bare postgres.js unique violation", () => { + expect(isUniqueViolation({ code: "23505", constraint_name: CONSTRAINT }, CONSTRAINT)).toBe(true); + }); + + it("matches the node-postgres constraint field", () => { + expect(isUniqueViolation({ code: "23505", constraint: CONSTRAINT }, CONSTRAINT)).toBe(true); + }); + + it("matches the error Drizzle wraps around the driver failure", () => { + const wrapped = new Error("Failed query: update \"issues\" set \"execution_run_id\" = $1"); + (wrapped as { cause?: unknown }).cause = { code: "23505", constraint_name: CONSTRAINT }; + expect(isUniqueViolation(wrapped, CONSTRAINT)).toBe(true); + }); + + it("falls back to the driver message when the constraint name is not surfaced", () => { + expect(isUniqueViolation({ + cause: { + code: "23505", + message: `duplicate key value violates unique constraint "${CONSTRAINT}"`, + }, + }, CONSTRAINT)).toBe(true); + }); + + it("matches any unique violation when no constraint is named", () => { + expect(isUniqueViolation({ cause: { code: "23505" } })).toBe(true); + }); + + it("ignores a unique violation on a different constraint", () => { + expect(isUniqueViolation({ cause: { code: "23505", constraint_name: "issues_identifier_idx" } }, CONSTRAINT)) + .toBe(false); + }); + + it("ignores errors that are not unique violations", () => { + expect(isUniqueViolation({ cause: { code: "23503", constraint_name: CONSTRAINT } }, CONSTRAINT)).toBe(false); + expect(isUniqueViolation(new Error("boom"), CONSTRAINT)).toBe(false); + expect(isUniqueViolation(null, CONSTRAINT)).toBe(false); + expect(isUniqueViolation(undefined, CONSTRAINT)).toBe(false); + }); + + it("stops walking a self-referential cause chain", () => { + const looped: { cause?: unknown } = {}; + looped.cause = looped; + expect(isUniqueViolation(looped, CONSTRAINT)).toBe(false); + }); +}); diff --git a/server/src/db-errors.ts b/server/src/db-errors.ts new file mode 100644 index 0000000000..667fa65580 --- /dev/null +++ b/server/src/db-errors.ts @@ -0,0 +1,33 @@ +const UNIQUE_VIOLATION = "23505"; +const MAX_CAUSE_DEPTH = 4; + +/** + * Recognizes a Postgres unique-constraint violation (SQLSTATE 23505). + * + * Drizzle wraps driver failures in its own `Failed query: ...` error, so the + * Postgres error that carries the code and the constraint name is reachable + * only through `cause` — inspecting the thrown error directly misses it. The + * constraint name itself lands on `constraint_name` under postgres.js and on + * `constraint` under node-postgres, and is not always surfaced at all, so fall + * back to the driver message. + */ +export function isUniqueViolation(error: unknown, constraintName?: string): boolean { + let current: unknown = error; + for (let depth = 0; depth < MAX_CAUSE_DEPTH && current && typeof current === "object"; depth += 1) { + const candidate = current as { + code?: unknown; + constraint?: unknown; + constraint_name?: unknown; + message?: unknown; + cause?: unknown; + }; + if (candidate.code === UNIQUE_VIOLATION) { + if (!constraintName) return true; + const constraint = candidate.constraint ?? candidate.constraint_name; + if (constraint === constraintName) return true; + if (typeof candidate.message === "string" && candidate.message.includes(constraintName)) return true; + } + current = candidate.cause; + } + return false; +} diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index beb6459dd6..02084fe7cf 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -103,6 +103,7 @@ import { } from "@paperclipai/shared"; import { trackAgentTaskCompleted } from "@paperclipai/shared/telemetry"; import { getTelemetryClient } from "../telemetry.js"; +import { isUniqueViolation } from "../db-errors.js"; import type { StorageService } from "../storage/types.js"; import { validate } from "../middleware/validate.js"; import * as serviceIndex from "../services/index.js"; @@ -9983,7 +9984,18 @@ export function issueRoutes( const checkoutRunId = requireAgentRunId(req, res); if (req.actor.type === "agent" && !checkoutRunId) return; - const updated = await svc.checkout(id, req.body.agentId, req.body.expectedStatuses, checkoutRunId); + let updated; + try { + updated = await svc.checkout(id, req.body.agentId, req.body.expectedStatuses, checkoutRunId); + } catch (error) { + if (isUniqueViolation(error, "issues_open_routine_execution_uq")) { + res.status(409).json({ + error: "Another execution for this routine is already in progress", + }); + return; + } + throw error; + } const actor = getActorInfo(req); if (updated?.harnessKind === "skill_test") { await companySkillsSvc.markTestRunRunning(updated.companyId, updated.id);