Skip to content

feat(review-quill): parallel review execution, no global lock - #501

Merged
krasnoperov merged 1 commit into
mainfrom
feat/review-quill-parallel-reviews
May 5, 2026
Merged

feat(review-quill): parallel review execution, no global lock#501
krasnoperov merged 1 commit into
mainfrom
feat/review-quill-parallel-reviews

Conversation

@krasnoperov

Copy link
Copy Markdown
Owner

Summary

Removes review-quill's single-worker reconciliation lock and runs reviews as independent agents — the same shape patchrelay uses for implementation runs. Discovery (cheap: GitHub list calls, DB lookups, eligibility math) runs in parallel across repos; dispatch of executions (expensive: Codex turn on a tmp worktree) fans out under a soft cap.

Why

The previous architecture serialized all work through a global `reconcileInProgress` flag. A hot repo whose webhooks fired continuously held that flag indefinitely and starved every other watched repo. Observed in production: 3 doc PRs opened simultaneously across 3 repos — only the repo whose reconcile happened to be in-flight got reviewed. The other two sat unattended for an hour.

Worktrees were already isolated (`mkdtemp` per call in `materialize.ts`), Codex's app-server protocol is multi-thread by design (`startThread` → `startTurn`), and the DB has a `UNIQUE(repo_full_name, pr_number, head_sha)` constraint that provides cross-restart dedup. The lock wasn't protecting anything real — it was an accidental architectural ceiling.

The right model is the one patchrelay already uses: react to external events, run independent agents in parallel, soft cap on total concurrency.

What changed

`packages/review-quill/src/service.ts`:

  • Removed: `reconcileInProgress`, `pendingFullReconcile`, `pendingRepoReconciles`, `runQueuedReconcile`, `prependQueuedReconciles`, `queueReconcileRequest`. None of it was protecting anything that wasn't already covered by per-attempt isolation.
  • Added: `discoverRepo()` (read-only walk + dispatch) and `dispatchReview()` (fire-and-forget worker under a semaphore).
  • Added: `inFlightReviews: Map<key, Promise>` for in-memory dedup on `(repoFullName, prNumber, headSha)`. DB `UNIQUE` constraint covers cross-restart dedup.
  • Added: semaphore (`acquireReviewSlot` / `releaseReviewSlot`) bounds total parallel executions at `maxConcurrentReviews` (default 20).
  • `reconcileAll` now runs all repos' discovery in parallel via `Promise.all`.

`packages/review-quill/src/types.ts`:

  • New config field: `reconciliation.maxConcurrentReviews?: number` (default 20).
  • `ReviewQuillRuntimeStatus`: dropped `reconcileInProgress` (no longer meaningful), added `inFlightReviews: number` and `repoLastReconciledAt: Record<repoFullName, ISO timestamp>` for operator visibility.

Architectural parity with patchrelay

Both services now follow the same pattern:

|-|-|-|

Aspect patchrelay review-quill
Trigger webhook + scheduled poll webhook + scheduled poll
Discovery per-issue dispatch loop `discoverRepo` per repo
Execution one Codex thread per run, on a per-issue worktree one Codex thread per review, on a per-attempt tmp worktree
Concurrency implicit (one run per issue, no global cap) explicit (semaphore at `maxConcurrentReviews`)
Dedup `activeRunId` + lease `inFlightReviews` map + DB UNIQUE

Both can run dozens of independent agents on the same host. Bumping concurrency further is a config change.

Tests

Replaced the prior fairness/prepend test (which described semantics that no longer exist) with four parallelism tests in `service.test.ts`:

  • Discovery passes run in parallel; neither repo blocks the other.
  • `dispatchReview` dedupes identical `(repo, pr, head)` calls — three back-to-back dispatches result in one execution.
  • The semaphore caps in-flight executions at the configured number — set `maxConcurrentReviews=2`, dispatch 5, observe peak in-flight = 2.
  • End-to-end: `triggerReconcile` fans out two reviews as independent workers that start before either completes.

162/162 review-quill tests pass. Repo-wide typecheck clean. Lint shows only pre-existing warnings unrelated to the change.

Migration

`ReviewQuillRuntimeStatus` is internal — no external consumers. The dashboard test fixture is updated; no public API change.

Removes review-quill's single-worker reconciliation lock and runs
reviews as independent agents — the same shape patchrelay uses for
implementation runs. Discovery (the cheap part — GitHub list calls,
DB lookups, eligibility math) runs serially per pass, but dispatch
of executions (the expensive part — Codex turn on a tmp worktree)
fans out under a soft cap.

Why
---
The previous architecture serialized all work through a global
`reconcileInProgress` flag. A hot repo whose webhooks fired
continuously held that flag indefinitely and starved every other
watched repo. Observed in production: 3 doc PRs opened simultaneously
across 3 repos — only the repo whose reconcile happened to be
in-flight got reviewed. The other two sat unattended for an hour.

Worktrees were already isolated (mkdtemp per call in
materialize.ts), Codex's app-server protocol is multi-thread by
design (startThread → startTurn), and the DB has a
UNIQUE(repo_full_name, pr_number, head_sha) constraint that
provides cross-restart dedup. The lock wasn't protecting anything
real — it was just an accidental ceiling.

What changed
------------
src/service.ts:
- Drop reconcileInProgress / pendingFullReconcile /
  pendingRepoReconciles / runQueuedReconcile /
  prependQueuedReconciles / queueReconcileRequest. Gone.
- Replace with discoverRepo() (read-only walk + dispatch) and
  dispatchReview() (fire-and-forget worker under a semaphore).
- New `inFlightReviews: Map<key, Promise>` for in-memory dedup
  on (repo, pr, headSha). DB UNIQUE constraint covers cross-restart.
- New semaphore (acquireReviewSlot / releaseReviewSlot) bounds
  total parallel executions at `maxConcurrentReviews` (default 20).
- reconcileAll now runs all repos' discovery in parallel via
  Promise.all — discovery itself is read-only and cheap.

src/types.ts:
- Add `reconciliation.maxConcurrentReviews?: number` (default 20).
- ReviewQuillRuntimeStatus: drop `reconcileInProgress`, add
  `inFlightReviews: number` and `repoLastReconciledAt: Record<...>`
  for operator visibility into per-repo discovery and the live
  worker count.

Tests
-----
Replace the prior fairness/prepend test (which described semantics
that no longer exist) with four parallelism tests:
- Discovery passes run in parallel; neither repo blocks the other.
- dispatchReview dedupes identical (repo, pr, head) calls.
- The semaphore actually caps in-flight executions at the configured
  number — verified with cap=2, dispatch 5, observe peak=2.
- End-to-end: triggerReconcile fans out two reviews as independent
  workers that start before either completes.
@krasnoperov
krasnoperov merged commit aa1d830 into main May 5, 2026
2 checks passed
@krasnoperov
krasnoperov deleted the feat/review-quill-parallel-reviews branch May 5, 2026 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant