fix(routes): return 409 for routine checkout conflicts (#3790)
## 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 <aaymeloglu@gmail.com>
This commit is contained in:
parent
ebf2b8ff79
commit
6a4e2e1b8c
|
|
@ -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);
|
||||
});
|
||||
});
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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);
|
||||
|
|
|
|||
Loading…
Reference in New Issue