fix(queue): add a per-PR actuation mutex for the draft-dodge and reopen-reclose paths - #2399
Conversation
|
Warning 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏸️ Gittensory review result - manual review recommendedReview updated: 2026-07-02 01:25:15 UTC
⏸️ Suggested Action - Manual Review
Review summary Nits — 7 non-blocking
Review context
Contributor next steps
Signal definitions
🟩 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.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2399 +/- ##
=======================================
Coverage 95.91% 95.91%
=======================================
Files 224 224
Lines 25221 25236 +15
Branches 9172 9175 +3
=======================================
+ Hits 24190 24206 +16
+ Misses 418 417 -1
Partials 613 613
🚀 New features to boost your workflow:
|
d279c17 to
5df591f
Compare
c102819 to
da15986
Compare
…en-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
…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
…reachable 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.
…e 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.
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.
…ntention 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.
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.
c252d90 to
1e70418
Compare
…-dodge, and reopen-reclose (#2447) maybeRunAgentMaintenance claimed a separate claimAgentMaintenanceLock while the draft-dodge and reopen-reclose paths claimed claimPrActuationLock — two independent lock namespaces for the same PR meant a maintenance pass and a draft-dodge/reopen-reclose actuation could still race each other, the exact gap a review on #2399 flagged after it had already merged. Route maybeRunAgentMaintenance through claimPrActuationLock/ releasePrActuationLock too and remove the now-redundant claimAgentMaintenanceLock so every mutating PR path shares one lock.
What
Two different webhook deliveries for the same PR (e.g. a
reopenedevent and a concurrentcheck_suite completedevent) can be dequeued by separate queue workers at nearly the same time. Both read the same stale-but-still-"current" state, both pass their own freshness checks (neither has yet mutated the PR), and both independently fire a mutating GitHub call. The practical damage is usually duplicate audit events and close-comment spam rather than a corrupted final state, but it's a genuine TOCTOU window — nothing in the queue-claim layer acquires a per-PR mutex before evaluating gate state and firing a mutation.Fix
Added a lightweight per-PR actuation mutex (
claimPrActuationLock/releasePrActuationLockinsrc/queue/processors.ts) — a short-TTL claim on the sameSELFHOST_TRANSIENT_CACHEalready used for CI-completion coalescing, best-effort release. Wrapped the two mutating webhook-triggered paths that weren't already covered by a per-PR lock: theconverted_to_draftdraft-dodge close (extracted intomaybeCloseDraftDodgeAttempt, mirroring the existing thin-wrapper pattern to avoid reindentation noise) andmaybeRecloseDisallowedReopen. A lock-contended caller fails open (skips this pass) rather than blocking — the delivery holding the lock is evaluating the same PR, and the periodic sweep is the backstop if this specific trigger gets dropped.Deliberately not the issue's suggested "lower-effort interim step" (extend the queue's coalesce lookup in
sqlite-queue.ts/pg-queue.tsto also matchstatus='processing'rows, not justpending): tracing throughenqueue(), the "superseded prefix" and "standard coalesce key" branches do an in-placeUPDATEon the matched row when they coalesce a new arrival. A worker that has already claimed a row read itspayloadonce at claim time into memory and never re-reads the DB row — so anUPDATEagainst aprocessingrow is invisible to whatever's currently executing, and gets permanently discarded when that worker deletes the row on completion. That's a coalesce that records a success metric while silently and irrecoverably dropping the new event's trigger, which is worse than the duplicate-processing bug it's meant to fix. The per-PR mutex avoids that failure mode entirely by never touching another job's row.A full per-PR Durable Object (
SubmissionLock) remains a separate, larger follow-up — already tracked as a TODO inenv.d.ts, deliberately out of scope here.Tests
npx tsc --noEmitclean.queue.test.ts— 204 passed.src/queue/processors.ts: fully covered.npm run test:coverage: 5602 passed, 4 skipped (pre-existing/unrelated), 0 failed.npm audit --audit-level=moderate: 0 vulnerabilities.Advances #1936. Closes #2135.