fix(review): serialize concurrent triggers per worker to stop reviewer double-spawn#246
Conversation
…spawn Engine.Trigger was a read-then-write (idempotency check -> reviewer spawn -> InsertReviewRun) with no serialization and no backing constraint. Two near- simultaneous triggers for the same worker at the same head SHA both passed the GetReviewRunBySessionAndSHA check, both spawned a reviewer against the same deterministic review-<id> handle, and both inserted a running run for one commit. Add a per-worker keyed mutex (lockWorker) held across the whole Trigger body, so the loser re-reads the freshly-recorded run and short-circuits to Created:false instead of spawning. Back it with a partial unique index on review_run(session_id, target_sha) (migration 0013) as a cross-restart safety net; rows with an empty target_sha (head not yet observed) are excluded so they are not blocked. Adds a concurrency test asserting N simultaneous triggers spawn once and record one run. Closes #242
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
I’d like two changes before merge:
|
…flict in Trigger Pre-#242 daemons can already hold duplicate (session_id, target_sha) review_run rows, on which CREATE UNIQUE INDEX fails and wedges startup. Migration 0013 now collapses each duplicate group to a single survivor (a completed pass over a still-running one, then newest by created_at) before building the index. Trigger now treats a unique-constraint hit as a fallback rather than an error: InsertReviewRun maps it to the new domain.ErrDuplicateReviewRun sentinel, and Trigger re-reads GetReviewRunBySessionAndSHA and returns that run with Created:false instead of surfacing a raw error after the reviewer may already have launched.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
2 participants
Closes #242.
Problem
Engine.Triggeris a read-then-write — idempotency check (GetReviewRunBySessionAndSHA) → reviewer spawn →InsertReviewRun— with no serialization and no backing DB constraint. Two near-simultaneous triggers for the same worker at the same head SHA both pass the idempotency check (neither has inserted yet), both spawn a reviewer against the same deterministicreview-<id>handle, and both insert arunningrun for one commit — defeating the idempotency design and leaving the UI's reviewer handle / run list inconsistent.Fix
Two layers:
lockWorker) held across the wholeTriggerbody. Concurrent triggers for the same worker now serialize; the loser re-reads the freshly-recorded run and short-circuits toCreated: falseinstead of spawning. Distinct workers never contend.review_run(session_id, target_sha)(migration0013) as a cross-restart safety net. Rows with an emptytarget_sha(head not yet observed) are excluded so they aren't blocked — the engine lock still serializes those.Test
TestTriggerConcurrentSameWorkerSpawnsOncefires 8 simultaneous triggers for one worker and asserts exactly one spawn, one recorded run, and oneCreated: trueresult. (Race-safe by construction: all shared access happens inside the serializedTriggerbody; CI'sbuild-test-racejob validates — no local C compiler here.)Gate:
go build ./...,gofmt -l,golangci-lint run(0 issues), fullgo test ./..., andinternal/storage/sqlitetests (which apply migration 0013) — all green.🤖 Generated with Claude Code