diff --git a/src/lib/groomer/groomer-lock.ts b/src/lib/groomer/groomer-lock.ts new file mode 100644 index 00000000..b32a72ec --- /dev/null +++ b/src/lib/groomer/groomer-lock.ts @@ -0,0 +1,58 @@ +/** + * Groomer run lock. + * + * A DB-backed single-row lock that serializes hosted-groomer runs. Without it, + * two concurrent runs can both select the same candidate before either acquires + * the per-issue lease (selection in selector.ts and lease acquisition in run.ts + * are not atomic), causing a duplicate LLM call + duplicate label writes. + * + * Mirrors src/lib/sync-lock.ts, reusing the generic `sync_lock` table with a + * distinct id ("groomer") — its `syncRunId` column is a plain nullable string + * (no FK), used here to hold a random lock token. First writer wins; a stale + * lock (>30 min) is reclaimed; the lock is released in a try/finally. + */ + +import { randomUUID } from "crypto"; +import { prisma } from "@/lib/prisma"; + +const LOCK_ID = "groomer" as const; +const MAX_AGE_MS = 30 * 60 * 1000; // 30 minutes + +export type GroomerLock = { locked: true; token: string } | { locked: false }; + +/** Attempt to acquire the groomer run lock. */ +export async function acquireGroomerLock(): Promise { + const existing = await prisma.syncLock.findUnique({ where: { id: LOCK_ID } }); + if (existing && existing.syncRunId) { + const age = Date.now() - existing.acquiredAt.getTime(); + if (age < MAX_AGE_MS) { + return { locked: false }; + } + // Stale lock — clear it and proceed. + await prisma.syncLock.delete({ where: { id: LOCK_ID } }); + } + + const token = randomUUID(); + try { + await prisma.$transaction(async (tx) => { + // Double-check inside the transaction for race safety. + const stillExisting = await tx.syncLock.findUnique({ where: { id: LOCK_ID } }); + if (stillExisting && stillExisting.syncRunId) { + throw new Error("already_locked"); + } + await tx.syncLock.create({ data: { id: LOCK_ID, syncRunId: token, acquiredAt: new Date() } }); + }); + } catch (err) { + if (err instanceof Error && err.message === "already_locked") { + return { locked: false }; + } + throw err; + } + + return { locked: true, token }; +} + +/** Release the groomer run lock. Conditional on the token so we never release another run's lock. */ +export async function releaseGroomerLock(token: string): Promise { + await prisma.syncLock.deleteMany({ where: { id: LOCK_ID, syncRunId: token } }); +} diff --git a/src/lib/groomer/run.test.ts b/src/lib/groomer/run.test.ts index 33482fc2..98097a91 100644 --- a/src/lib/groomer/run.test.ts +++ b/src/lib/groomer/run.test.ts @@ -30,6 +30,8 @@ const { mocks } = vi.hoisted(() => ({ addIssueLabel: vi.fn(), removeIssueLabel: vi.fn(), buildRepositoryContext: vi.fn(), + acquireGroomerLock: vi.fn(), + releaseGroomerLock: vi.fn(), prisma: { automationRepo: { findUnique: vi.fn() }, groomingRun: { create: vi.fn(), update: vi.fn(), findFirst: vi.fn() }, @@ -84,6 +86,11 @@ vi.mock("./repository-context", () => ({ buildRepositoryContext: mocks.buildRepositoryContext, })); +vi.mock("./groomer-lock", () => ({ + acquireGroomerLock: mocks.acquireGroomerLock, + releaseGroomerLock: mocks.releaseGroomerLock, +})); + import { runHostedGroomer } from "./run"; const mockCandidate: GroomingCandidate = { @@ -138,6 +145,8 @@ describe("runHostedGroomer", () => { mocks.findActiveLeasesForIssue.mockResolvedValue([]); mocks.upsertLease.mockResolvedValue({ created: true, lease: { id: "lease-1" } }); mocks.releaseLease.mockResolvedValue({ id: "lease-1" }); + mocks.acquireGroomerLock.mockResolvedValue({ locked: true, token: "lock-token" }); + mocks.releaseGroomerLock.mockResolvedValue(undefined); mocks.prisma.automationRepo.findUnique.mockResolvedValue(mockAutomationRepo); mocks.prisma.groomingRun.create.mockResolvedValue(mockGroomingRun); mocks.prisma.groomingRun.update.mockResolvedValue({ ...mockGroomingRun, stage: "planned" }); @@ -164,6 +173,23 @@ describe("runHostedGroomer", () => { expect(mocks.callGroomerLLM).not.toHaveBeenCalled(); }); + it("bails without selecting when the groomer lock is held", async () => { + mocks.acquireGroomerLock.mockResolvedValue({ locked: false }); + + const result = await runHostedGroomer(); + + expect(result).toBeNull(); + expect(mocks.selectGroomingCandidate).not.toHaveBeenCalled(); + expect(mocks.releaseGroomerLock).not.toHaveBeenCalled(); + }); + + it("releases the groomer lock after a run completes", async () => { + await runHostedGroomer(); + + expect(mocks.acquireGroomerLock).toHaveBeenCalledTimes(1); + expect(mocks.releaseGroomerLock).toHaveBeenCalledWith("lock-token"); + }); + it("dry-run creates and completes groomingRun and result has groomingRunId", async () => { mocks.getHostedGroomerConfig.mockReturnValue({ ...mockConfig, dryRun: true }); diff --git a/src/lib/groomer/run.ts b/src/lib/groomer/run.ts index afd13a31..ba8ed140 100644 --- a/src/lib/groomer/run.ts +++ b/src/lib/groomer/run.ts @@ -1,6 +1,7 @@ import { prisma } from "@/lib/prisma"; import { addIssueComment, updateIssueLabels, updateIssueTitleAndBody } from "@/lib/github"; import { findActiveLeasesForIssue, releaseLease, upsertLease } from "@/lib/lease"; +import { acquireGroomerLock, releaseGroomerLock } from "./groomer-lock"; import { selectGroomingCandidate } from "./selector"; import { buildIssueContext, fetchIssueComments } from "./context"; import { callGroomerLLM } from "./llm"; @@ -47,6 +48,8 @@ export interface GroomerDeps { releaseLease: typeof releaseLease; prisma: typeof prisma; buildRepositoryContext: typeof buildRepositoryContext; + acquireGroomerLock: typeof acquireGroomerLock; + releaseGroomerLock: typeof releaseGroomerLock; } const defaultDeps: GroomerDeps = { @@ -64,11 +67,29 @@ const defaultDeps: GroomerDeps = { releaseLease, prisma, buildRepositoryContext, + acquireGroomerLock, + releaseGroomerLock, }; export async function runHostedGroomer( options: RunHostedGroomerOptions = {}, deps: GroomerDeps = defaultDeps, +): Promise { + // Serialize runs behind a DB lock: without it, two concurrent groomer runs + // can select the same candidate before either acquires the per-issue lease + // (selection and lease acquisition are not atomic), double-grooming the issue. + const lock = await deps.acquireGroomerLock(); + if (!lock.locked) return null; + try { + return await executeGroomerRun(options, deps); + } finally { + await deps.releaseGroomerLock(lock.token); + } +} + +async function executeGroomerRun( + options: RunHostedGroomerOptions = {}, + deps: GroomerDeps = defaultDeps, ): Promise { const config = deps.getConfig(); const dryRun = options.dryRun ?? config.dryRun;