fix(queue): serialize concurrent webhook/sweep passes for the same PR by JSONbored · Pull Request #2368 · JSONbored/loopover

JSONbored · 2026-07-01T12:57:00Z

What

check_run/check_suite completed, pull_request synchronize, and the scheduled agent-regate-pr sweep all key their job coalescing differently (github-webhook:ci-completed:{repo}@{headSha}#{prNumbers}, github-webhook:pr-refresh:{repo}#{pr}@{headSha}, and agent-regate-pr:{repo}#{pr} respectively), so they never dedup against each other. Combined with QUEUE_CONCURRENCY explicitly overlapping I/O-bound jobs, two of these can be claimed and processed concurrently for the same PR, each computing its own independently-timed live CI/mergeable/reviewDecision snapshot. If two such reads disagree — e.g. the bot's own prior approve review lands asynchronously between them — one pass could plan close while the other plans merge/approve, and both would pass their own freshness check and both execute.

The only existing per-PR-keyed coalescing (ciReReviewCoalesced, a 60s window) is wired into the check_run/check_suite handler only — it's never consulted by the sweep's agent-regate-pr path, so sweep-vs-webhook races are entirely unguarded.

Fix

Add a short-TTL per-PR advisory lock (agent-maintenance-lock:{repo}#{pr}, via the existing SELFHOST_TRANSIENT_CACHE — the same mechanism ciReReviewCoalesced already uses) around the plan-and-execute critical section of maybeRunAgentMaintenance (src/queue/processors.ts). A pass that can't claim the lock defers cleanly — the next webhook/sweep tick is the backstop. Lightweight stand-in for the per-PR SubmissionLock Durable Object already noted as a longer-term TODO in env.d.ts.

The critical section is extracted into runAgentMaintenancePlanAndExecute so the try/finally wrapping it doesn't force-reindent the ~200 existing lines inside (which would have made this diff far larger and harder to review for no logic change). Both the direct webhook path and the sweep path (via reReviewStoredPullRequest) converge on the same maybeRunAgentMaintenance call, so wrapping it there covers both without touching either caller.

claimAgentMaintenanceLock/releaseAgentMaintenanceLock are exported for direct testing. The TTL (60s, matching the existing CI_COALESCE_WINDOW_SECONDS) is a crash-safety backstop only — the normal path releases explicitly in the finally block within seconds.

Tests

claimAgentMaintenanceLock claims when free, denies when held (per-PR, not repo-wide), and a released lock is claimable again.
Fails open on a broken transient cache (never itself blocks actuation — it's defense-in-depth, not the primary gate).
Integration: a maintenance pass defers its entire plan-and-execute critical section (no mutation, no agent.action.* audit) when another pass already holds the PR's lock — the scenario fix(queue): concurrent webhook and sweep jobs for the same PR are not mutually excluded #2129 describes, using the same settings/PR shape a real webhook or sweep pass would see.

Full unsharded test:coverage green (5599 passed); typecheck green; npm audit clean.

Advances #1936. Closes #2129.

check_run/check_suite completion, pull_request synchronize, and the scheduled agent-regate-pr sweep all key their job coalescing differently (github-webhook:ci-completed:..., github-webhook:pr-refresh:..., and agent-regate-pr:... respectively), so they never dedup against each other. Combined with QUEUE_CONCURRENCY explicitly overlapping I/O-bound jobs, two of these can be claimed and processed concurrently for the same PR, each computing its own independently-timed live CI/mergeable/ reviewDecision snapshot — a narrow window where disagreeing reads could race to plan and execute different actions for the same PR. Add a short-TTL per-PR advisory lock (agent-maintenance-lock:{repo}#{pr} via the existing SELFHOST_TRANSIENT_CACHE, the same mechanism the CI-completion re-review coalesce window already uses) around the plan-and-execute critical section of maybeRunAgentMaintenance. A pass that can't claim the lock defers cleanly; the next webhook/sweep tick is the backstop. The critical section is extracted into runAgentMaintenancePlanAndExecute so the try/finally wrapping it doesn't force-reindent the whole existing block. Both the direct webhook path and the sweep path (via reReviewStoredPullRequest) converge on the same maybeRunAgentMaintenance call, so wrapping it there covers both without touching either caller. Lightweight stand-in for the per-PR SubmissionLock Durable Object already noted as a longer-term TODO in env.d.ts.

loopover-orb · 2026-07-01T12:57:34Z

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

_{Review updated: 2026-07-01 21:15:52 UTC}

6 files · 1 AI reviewer · 1 blocker · readiness 75/100 · CI green · blocked

⏸️ Suggested Action - Manual Review

AI reviewers agree on a likely critical defect: src/queue/processors.ts:1707 acquires `claimAgentMaintenanceLock` after `gate` and `args.liveFacts` have already been computed, so an older webhook/sweep pass can execute a stale close/merge decision while the newer pass for the same PR observes the held lock and returns instead of recomputing inside the critical section. — Resolve the flagged defect, or override if the AI reviewers are mistaken, then re-run the gate.

Review summary
The change adds a per-PR transient-cache advisory lock and wires Redis `SET NX` support plus focused tests for the lock primitive. The atomic cache claim itself is sound, but the lock is acquired after `gate` and `liveFacts` have already been produced, so it serializes only the final mutation step, not the live-state read and plan that the PR description identifies as racy. That leaves the stale-plan winner scenario reachable while causing the fresher concurrent pass to defer.

Blockers

src/queue/processors.ts:1707 acquires `claimAgentMaintenanceLock` after `gate` and `args.liveFacts` have already been computed, so an older webhook/sweep pass can execute a stale close/merge decision while the newer pass for the same PR observes the held lock and returns instead of recomputing inside the critical section.

Nits — 4 non-blocking

nit: test/unit/queue.test.ts only proves that a held lock prevents mutation; add a production-path race test where two passes compute different live states before lock acquisition so the stale-plan bug is covered.
Move the advisory lock to cover the live GitHub fact read, gate evaluation, planning, and execution, or reacquire fresh `liveFacts`/`gate` immediately after claiming the lock before calling `runAgentMaintenancePlanAndExecute`.
Add an assertion that the lock denial path is only used before any action plan is computed, or rename/scope the helper so future callers do not assume it protects stale inputs.
PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.

Concerns raised — review before merging

src/queue/processors.ts:1707 acquires `claimAgentMaintenanceLock` after `gate` and `args.liveFacts` have already been computed, so an older webhook/sweep pass can execute a stale close/merge decision while the newer pass for the same PR observes the held lock and returns instead of recomputing inside the critical section.

Signal	Result	Evidence
Code review	❌ 1 blocker	1 reviewer
Linked issue	✅ Linked	#2129
Related work	⚠️ 2 scoped overlaps	Top overlaps are listed below; lower-confidence bulk is hidden.
Change scope	❌ 8/20	High review scope from cached public metadata (size label size:L; 1 linked issue).
Validation posture	✅ 25/25	PR body includes validation/test evidence.
Contributor workload	✅ 10/10	Author activity: 67 registered-repo PR(s), 57 merged, 589 issue(s).
Contributor context	✅ Confirmed Gittensor contributor	JSONbored; Gittensor profile; 67 PR(s), 589 issue(s).
Gate result	❌ Blocking	Repo-configured hard blocker found.

Review context

Author: JSONbored
Role context: owner (maintainer lane)
Public audience mode: oss maintainer
Lane context: Repository registration is not available in the local Gittensory cache.
Public profile languages: not available
Official Gittensor activity: 67 PR(s), 589 issue(s).
Related work: Items reference the same linked issue fix(queue): concurrent webhook and sweep jobs for the same PR are not mutually excluded #2129. (issue #2129)
Related work: Open PR work references issue fix(queue): concurrent webhook and sweep jobs for the same PR are not mutually excluded #2129. (issue #2129)

Contributor next steps

Treat this as maintainer-lane context rather than normal contributor-lane activity.
Review top overlaps.
Add a concise scope and risk note.
Triage stale or unlinked PRs.
No action.
Check active issues and PRs before submitting.

Signal definitions

Related work = same linked issue, overlapping active PRs, or title/path similarity.
Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

_{🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed}

💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

Re-run Gittensory review

codecov · 2026-07-01T13:05:13Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.84%. Comparing base (28b345a) to head (dbea825).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2368   +/-   ##
=======================================
  Coverage   95.83%   95.84%           
=======================================
  Files         224      224           
  Lines       25006    25025   +19     
  Branches     9094     9097    +3     
=======================================
+ Hits        23964    23984   +20     
+ Misses        428      427    -1     
  Partials      614      614

Files with missing lines	Coverage Δ
src/queue/processors.ts	`91.25% <100.00%> (+0.13%)`	⬆️
src/selfhost/redis-cache.ts	`100.00% <100.00%> (ø)`

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…nce-pr-lock # Conflicts: # test/unit/queue.test.ts

claimAgentMaintenanceLock performed getTransientKey then putTransientKey as two separate operations, so two concurrent passes for the same PR (e.g. a webhook and the sweep, whose job-coalesce keys never match each other) could both observe an absent key, both write it, and both proceed to execute conflicting maintenance actions -- defeating the serializer the lock exists to provide. Add an atomic claim(key, value, ttlSeconds) primitive to the SELFHOST_TRANSIENT_CACHE interface, backed in production by Redis's SET ... EX ttl NX (a single server-side check-and-set, so no window exists where two callers can both see "absent"). Wire claimAgentMaintenanceLock to use it, falling back to the prior get/set pair only for a cache adapter that hasn't implemented claim yet -- strictly no worse than the previous behavior, never a regression. Add a genuine concurrent-race regression test (races two claims for the same PR via Promise.all and asserts exactly one wins), plus dedicated coverage for the new claim() primitive itself and the fallback path.

cloudflare-workers-and-pages · 2026-07-01T20:53:43Z

Deploying with Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status	Name	Latest Commit	Preview URL	Updated (UTC)
✅ Deployment successful! View logs	gittensory-ui	`5a40e51`	Commit Preview URL Branch Preview URL	Jul 01 2026, 08:55 PM

JSONbored · 2026-07-01T21:20:49Z

The #2368 verification confirms my own analysis — recommend merging as-is; the deeper issue found (AI-review-cache races producing divergent gate verdicts) is real but pre-existing and out of scope for that PR. I'll flag that as a follow-up and now focus on #2369's actual, confirmed bug via a workflow (implement + adversarially verify), since this is the second time this exact code area got a legitimate finding.

…test Review findings on this PR: - claimPrActuationLock was a non-atomic getTransientKey-then-putTransientKey pair, so two genuinely concurrent deliveries for the same PR could both observe an absent key and both proceed — defeating the exact race this mutex exists to close. The sibling claimAgentMaintenanceLock (#2129, #2368) already solved this: env.SELFHOST_TRANSIENT_CACHE.claim performs the check-and-set as one atomic operation (Redis SET NX server-side), with a documented fallback to the old get/set pair for a cache adapter that hasn't implemented claim yet. Mirrored that exact pattern here. - The existing lock tests only pre-seeded the key before the call started, proving the contended branch but not the actual race. Added a Promise.all test that fires two draft-dodge deliveries for the SAME PR with neither pre-claiming anything, asserting exactly one PATCH and one completed audit row. Verified this test is meaningful by temporarily reverting to the non-atomic implementation and confirming it fails (2 PATCH calls), then restoring the fix and confirming it passes. - Exported claimPrActuationLock/releasePrActuationLock (matching the already-exported claimAgentMaintenanceLock/releaseAgentMaintenanceLock) and mirrored that sibling's full direct-unit-test suite — fail-open on a broken cache, fail-open when claim() itself throws, atomic-claim-used verification, and the no-claim-method fallback — closing the branch coverage gap the new code left in the fallback/catch paths.

…test Review findings on this PR: - claimPrActuationLock was a non-atomic getTransientKey-then-putTransientKey pair, so two genuinely concurrent deliveries for the same PR could both observe an absent key and both proceed — defeating the exact race this mutex exists to close. The sibling claimAgentMaintenanceLock (#2129, #2368) already solved this: env.SELFHOST_TRANSIENT_CACHE.claim performs the check-and-set as one atomic operation (Redis SET NX server-side), with a documented fallback to the old get/set pair for a cache adapter that hasn't implemented claim yet. Mirrored that exact pattern here. - The existing lock tests only pre-seeded the key before the call started, proving the contended branch but not the actual race. Added a Promise.all test that fires two draft-dodge deliveries for the SAME PR with neither pre-claiming anything, asserting exactly one PATCH and one completed audit row. Verified this test is meaningful by temporarily reverting to the non-atomic implementation and confirming it fails (2 PATCH calls), then restoring the fix and confirming it passes. - Exported claimPrActuationLock/releasePrActuationLock (matching the already-exported claimAgentMaintenanceLock/releaseAgentMaintenanceLock) and mirrored that sibling's full direct-unit-test suite — fail-open on a broken cache, fail-open when claim() itself throws, atomic-claim-used verification, and the no-claim-method fallback — closing the branch coverage gap the new code left in the fallback/catch paths. # Conflicts: # test/unit/queue.test.ts

…en-reclose paths (#2399) * fix(queue): add a per-PR actuation mutex for the draft-dodge and reopen-reclose paths Two different webhook deliveries for the same PR (e.g. a reopened event and a concurrent check_suite completed event) could be dequeued by separate workers at nearly the same time. Both would read the same stale-but-still-"current" state, both pass their own freshness checks, and both independently fire a mutating call — a TOCTOU window with no per-PR mutex anywhere in the actuation path. Add a lightweight interim mutex (short-TTL transient-cache claim, best-effort release) and wrap the draft-dodge close and reopen-reclose handlers with it — the two mutating webhook-triggered paths that weren't already covered by an existing per-PR lock. A lock-contended caller fails open (skips this pass); the delivery holding the lock is evaluating the same PR, and the periodic sweep is the backstop if this specific trigger is dropped. Deliberately NOT the queue-level "widen the coalesce lookup to match status='processing'" interim step the issue also floats: that would have enqueue() silently UPDATE a claimed row's payload, which never gets re-read before the claiming worker deletes the row on completion — a coalesce that reports success while permanently discarding the new event's trigger. The per-PR mutex avoids that failure mode entirely. A full per-PR Durable Object (SubmissionLock) remains a separate, larger follow-up per the existing TODO in env.d.ts. # Conflicts: # test/unit/queue.test.ts * fix(queue): make claimPrActuationLock atomic, add a real concurrency test Review findings on this PR: - claimPrActuationLock was a non-atomic getTransientKey-then-putTransientKey pair, so two genuinely concurrent deliveries for the same PR could both observe an absent key and both proceed — defeating the exact race this mutex exists to close. The sibling claimAgentMaintenanceLock (#2129, #2368) already solved this: env.SELFHOST_TRANSIENT_CACHE.claim performs the check-and-set as one atomic operation (Redis SET NX server-side), with a documented fallback to the old get/set pair for a cache adapter that hasn't implemented claim yet. Mirrored that exact pattern here. - The existing lock tests only pre-seeded the key before the call started, proving the contended branch but not the actual race. Added a Promise.all test that fires two draft-dodge deliveries for the SAME PR with neither pre-claiming anything, asserting exactly one PATCH and one completed audit row. Verified this test is meaningful by temporarily reverting to the non-atomic implementation and confirming it fails (2 PATCH calls), then restoring the fix and confirming it passes. - Exported claimPrActuationLock/releasePrActuationLock (matching the already-exported claimAgentMaintenanceLock/releaseAgentMaintenanceLock) and mirrored that sibling's full direct-unit-test suite — fail-open on a broken cache, fail-open when claim() itself throws, atomic-claim-used verification, and the no-claim-method fallback — closing the branch coverage gap the new code left in the fallback/catch paths. # Conflicts: # test/unit/queue.test.ts * fix(queue): raise the actuation lock TTL to make the ownership gap unreachable Review finding: claimPrActuationLock stores a constant lock value with no per-holder ownership token, so a holder running past the TTL could have its lock claimed by a new holder, then have that new holder's live lock deleted by the first holder's stale finally-block release — reopening the concurrent-mutation race this mutex exists to close. The proper fix (a per-holder token + atomic compare-and-delete) needs a new cache-adapter primitive and should apply to the sibling claimAgentMaintenanceLock too for consistency — tracked alongside the existing Durable Object follow-up rather than done here. As an interim mitigation, raised the TTL from 60s to 600s: the guarded operations are a handful of sequential GitHub API calls that should never legitimately run anywhere near that long, so the window this finding describes is now practically unreachable rather than architecturally closed. * fix(queue): let a transient getInstallation read propagate through the draft-dodge mutex wrapper The actuation-lock wrapper's call site was swallowing every error from maybeCloseDraftDodgeAttempt, including the write-permission-readiness getInstallation read that is deliberately left uncaught so a transient D1 failure retries instead of misrecording a permission denial. * fix(queue): remove claimPrActuationLock's non-atomic get/set fallback A cache adapter without claim() previously fell back to a get-then-set pair, which is not a real exclusivity guarantee — two concurrent callers can both observe an absent key before either writes. Reuse claimTransientLock's fail-open behavior instead, matching the fix already applied to the sibling claimAgentMaintenanceLock. * fix(queue): stop the reopen-reclose webhook pass on actuation-lock contention maybeRecloseDisallowedReopen returned a plain false on lock contention, which the caller's boolean contract read as 'not blocked, proceed to normal re-review' — a contended webhook could still evaluate/mutate the same PR the lock holder owns. Replace the boolean with a tri-state ReopenRecloseOutcome (reclosed / allowed / lock_contended) so the caller skips the re-review on contention too. * fix(queue): remove the unreachable catch masking a disallowed reopen The outer .catch() around maybeRecloseDisallowedReopen could never actually fire — the lock claim/release fail open and every step in recloseDisallowedReopenIfNeeded already catches its own errors — so codecov/patch flagged it as an uncoverable line. Swallowing an unexpected error there into a silent 'allowed' would have re-permitted the exact disallowed reopen this guard exists to stop, so removing it is also the safer behavior: let it propagate and retry.

dosubot Bot added the size:M label Jul 1, 2026

loopover-orb Bot added gittensor gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. labels Jul 1, 2026

JSONbored self-assigned this Jul 1, 2026

JSONbored added this to gittensory - v1 roadmap Jul 1, 2026

github-project-automation Bot moved this to Todo in gittensory - v1 roadmap Jul 1, 2026

JSONbored added 2 commits July 1, 2026 13:41

Merge remote-tracking branch 'origin/main' into claude/agent-maintena…

e3453c2

…nce-pr-lock # Conflicts: # test/unit/queue.test.ts

dosubot Bot added size:L and removed size:M labels Jul 1, 2026

Merge branch 'main' into claude/agent-maintenance-pr-lock

dbea825

JSONbored merged commit 18aef69 into main Jul 1, 2026
13 checks passed

JSONbored deleted the claude/agent-maintenance-pr-lock branch July 1, 2026 21:20

github-project-automation Bot moved this from Todo to Done in gittensory - v1 roadmap Jul 1, 2026

JSONbored mentioned this pull request Jul 1, 2026

fix(queue): dedupe concurrent AI reviews for the same PR head #2429

Merged

6 tasks

Navigation Menu

Uh oh!

fix(queue): serialize concurrent webhook/sweep passes for the same PR - #2368

fix(queue): serialize concurrent webhook/sweep passes for the same PR#2368
JSONbored merged 4 commits into
mainfrom
claude/agent-maintenance-pr-lock

JSONbored commented Jul 1, 2026

Uh oh!

loopover-orb Bot commented Jul 1, 2026 •

edited

Loading

⏸️ Gittensory review result - manual review recommended

Uh oh!

codecov Bot commented Jul 1, 2026 •

edited

Loading

Uh oh!

cloudflare-workers-and-pages Bot commented Jul 1, 2026 •

edited

Loading

Uh oh!

JSONbored commented Jul 1, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

JSONbored commented Jul 1, 2026

What

Fix

Tests

Uh oh!

loopover-orb Bot commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⏸️ Gittensory review result - manual review recommended

Uh oh!

codecov Bot commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cloudflare-workers-and-pages Bot commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying with Cloudflare Workers

Uh oh!

JSONbored commented Jul 1, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

loopover-orb Bot commented Jul 1, 2026 •

edited

Loading

codecov Bot commented Jul 1, 2026 •

edited

Loading

cloudflare-workers-and-pages Bot commented Jul 1, 2026 •

edited

Loading