Skip to content

fix(heartbeat): serialize same-PR review dispatch - #1014

Merged
kkroo merged 2 commits into
masterfrom
codex/pr-review-task-key-dispatch-dedupe
Aug 4, 2026
Merged

fix(heartbeat): serialize same-PR review dispatch#1014
kkroo merged 2 commits into
masterfrom
codex/pr-review-task-key-dispatch-dedupe

Conversation

@kkroo

@kkroo kkroo commented Aug 4, 2026

Copy link
Copy Markdown

Summary

  • keep an explicit PR-review follow-up queued while the same context_task_key is already running
  • continue dispatching unrelated review tasks into available concurrency slots
  • preserve the existing follow-up review instead of cancelling it

Root cause

Direct GitHub reviewer wakes are issue-less. The dispatcher deduplicated only context_issue_id, so a second wake for the same pr_review:<repo>:<number> task could be claimed while the first review was still running. Production showed duplicate running reviews for PRs 1987 and 2013 after the concurrency rollout.

Validation

  • pnpm exec vitest run server/src/__tests__/heartbeat-dispatch-priority-sort.test.ts -t "keeps a same-PR review follow-up queued"
  • pnpm --filter @paperclipai/server typecheck
  • git diff --check

@cursor

cursor Bot commented Aug 4, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@allyblockcast

allyblockcast Bot commented Aug 4, 2026

Copy link
Copy Markdown

Hey @kkroo! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

  • Missing section: ## Thinking Path
  • Missing section: ## What Changed
  • Missing section: ## Verification
  • Missing section: ## Risks
  • Missing section: ## Model Used
  • No linked issue or inline issue description found — either tag an existing issue with Fixes #NNN / Closes #NNN / Refs #NNN, or describe the underlying issue inline in the PR body following one of our issue templates (https://github.com/paperclipai/paperclip/tree/master/.github/ISSUE_TEMPLATE). See CONTRIBUTING.md → "Link Issues or Describe Them In-PR".
  • Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 4f01596

Important Issues (1)

  • [gstack/review] server/src/services/heartbeat.ts:18875 — Same-PR serialization is scoped only to one agent. runningRunRows comes from listRunningRunsForAgent(agentId), so if the configured reviewer pool or agent eligibility changes while reviewer A still has this PR running, findActivePrReviewerForTask can assign the follow-up to reviewer B and B will not see A’s in-flight task key.
    • Enforce the task-key exclusion across the reviewer pool with an atomic DB-backed lease/claim (or query all running PR-review task keys across eligible/configured reviewers inside the serialized claim path), and add a regression test with the running review and queued follow-up assigned to different agents.

Strengths

  • The in-memory task-key set is updated after each successful claim, preventing duplicate same-pass dispatches on one agent.
  • Different PR task keys continue to use available concurrency, and the regression test covers that behavior.

Recommended Action

  1. Address the cross-agent serialization gap before merge.

@allyblockcast

allyblockcast Bot commented Aug 4, 2026

Copy link
Copy Markdown

Hey @kkroo! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

  • Missing section: ## Thinking Path
  • Missing section: ## What Changed
  • Missing section: ## Verification
  • Missing section: ## Risks
  • Missing section: ## Model Used
  • No linked issue or inline issue description found — either tag an existing issue with Fixes #NNN / Closes #NNN / Refs #NNN, or describe the underlying issue inline in the PR body following one of our issue templates (https://github.com/paperclipai/paperclip/tree/master/.github/ISSUE_TEMPLATE). See CONTRIBUTING.md → "Link Issues or Describe Them In-PR".
  • Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: e3d3bf0

Prior Findings Dispositioned (1)

  • prior:4f01596 important 1 — fixed — server/src/services/pr-review-dispatch-lock.ts:27 — The final claim now takes a transaction-scoped advisory lock keyed by the PR task and checks all running heartbeat rows, independent of reviewer identity, before either claim path transitions the queued run.

Looks good. The shared lock closes the cross-reviewer dispatch race while preserving queued follow-ups and unrelated review concurrency.

Strengths

  • The advisory lock and running-row check share the same transaction as the queued-to-running transition, eliminating the check-then-act window.
  • Regression coverage exercises an existing owner on another reviewer and simultaneous claims by two reviewers.

Recommended Action

  1. Merge after required CI checks pass.

@allyblockcast allyblockcast left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: e3d3bf0

Prior Findings Dispositioned (1)

  • prior:4f01596 important 1 — fixed — server/src/services/pr-review-dispatch-lock.ts:27 — The final claim now takes a transaction-scoped advisory lock keyed by the PR task and checks all running heartbeat rows, independent of reviewer identity, before either claim path transitions the queued run.

Looks good. The shared lock closes the cross-reviewer dispatch race while preserving queued follow-ups and unrelated review concurrency.

Strengths

  • The advisory lock and running-row check share the same transaction as the queued-to-running transition, eliminating the check-then-act window.
  • Regression coverage exercises an existing owner on another reviewer and simultaneous claims by two reviewers.

Recommended Action

  1. Merge after required CI checks pass.

@kkroo
kkroo merged commit 64e0d44 into master Aug 4, 2026
18 of 20 checks passed
@kkroo
kkroo deleted the codex/pr-review-task-key-dispatch-dedupe branch August 4, 2026 20:43
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.

2 participants