Skip to content

fix(queue): retry contended reopen guards - #2473

Merged
JSONbored merged 1 commit into
mainfrom
codex/fix-lock-contention-bypassing-reopen-checks
Jul 2, 2026
Merged

fix(queue): retry contended reopen guards#2473
JSONbored merged 1 commit into
mainfrom
codex/fix-lock-contention-bypassing-reopen-checks

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • A contended per-PR actuation lock for reopened PR webhooks was being treated as a handled delivery, which could permanently bypass the one-shot reopen re-close guard and normal re-review paths.
  • The change ensures transient lock contention does not silently mark the delivery processed and instead makes the webhook retryable so the reopen guard can run again once the lock clears.

Description

  • In src/queue/processors.ts the reopen handler now throws a RetryableJobError when maybeRecloseDisallowedReopen() returns "lock_contended", instead of stamping the webhook row processed and returning; "reclosed" still early-returns and stamps processed as before.
  • Updated the ReopenRecloseOutcome comments to document that lock contention must be retried rather than acknowledged as handled.
  • Added/updated a regression test in test/unit/queue.test.ts that pre-claims the per-PR lock and asserts a contended reopen produces a retryable error, performs no PATCH /pulls/..., does not start the normal re-review, and leaves the webhook row non-processed for redelivery.

Testing

  • Ran the focused regression test: npx vitest run test/unit/queue.test.ts -t "contended reopen-reclose" and it passed (covers the lock-contention behavior).
  • Ran type-checking with npm run typecheck which completed successfully.
  • Ran the full unit test file (npx vitest run test/unit/queue.test.ts --reporter=dot) in this environment and it hit multiple existing 15s test timeouts in unrelated sweep tests, producing failures; the focused regression test is the meaningful verification for this change.

Codex Task

@loopover-orb

loopover-orb Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Warning

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

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-02 06:32:04 UTC

2 files · 1 AI reviewer · no blockers · readiness 68/100 · CI green · dirty

⏸️ Suggested Action - Manual Review

  • Touches a guarded path — held for manual review

Review summary
The change correctly separates the two reopen-guard terminal states: a real re-close still acknowledges the delivery, while per-PR lock contention now raises a retryable error before the normal re-review path can start. The updated regression test exercises the reachable contended-lock path by pre-claiming the same PR actuation lock and verifies the important side effects: no PATCH, no settings resolution, and the webhook row remains non-processed. I do not see a correctness defect in the visible diff.

Nits — 6 non-blocking
  • nit: src/queue/processors.ts:3770 hard-codes the 5_000ms retry delay inline; consider using a named constant near the PR actuation lock timing so the retry cadence stays intentional if the lock TTL changes.
  • nit: test/unit/queue.test.ts:11731 only matches retryKind on the rejection; matching retryAfterMs as well would pin the contract this processor now relies on.
  • src/queue/processors.ts:3770: extract `5_000` to a named retry/backoff constant or reuse an existing queue retry timing constant if one exists.
  • test/unit/queue.test.ts:11743: extend the rejection assertion to include `{ retryKind: "pr_actuation_lock_contended", retryAfterMs: 5_000 }` so future edits do not silently alter the retry behavior.
  • Readiness score is below the configured threshold — Use the readiness panel as advisory maintainer context; the score does not block this PR.
  • Touches a guarded path — held for manual review — A maintainer must review and merge this change.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
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:S; no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 65 registered-repo PR(s), 55 merged, 548 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 65 PR(s), 548 issue(s).
Gate result ⚠️ Not blocking Advisory; not blocking this PR.
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: 65 PR(s), 548 issue(s).
  • Related work: Titles/paths share 6 meaningful terms. (PR #2470)
  • Related work: Titles/paths share 7 meaningful terms. (PR #2477, PR #2478)
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • Review top overlaps.
  • Add a concise scope and risk note.
  • Triage stale or unlinked PRs.
  • No action.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
  • 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

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.95%. Comparing base (4101ca6) to head (fa9a49e).
⚠️ Report is 35 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2473      +/-   ##
==========================================
+ Coverage   95.94%   95.95%   +0.01%     
==========================================
  Files         226      226              
  Lines       25361    25427      +66     
  Branches     9229     9245      +16     
==========================================
+ Hits        24333    24399      +66     
  Misses        417      417              
  Partials      611      611              
Files with missing lines Coverage Δ
src/queue/processors.ts 91.77% <100.00%> (+0.03%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JSONbored
JSONbored force-pushed the codex/fix-lock-contention-bypassing-reopen-checks branch from 270d130 to fa9a49e Compare July 2, 2026 07:13
@JSONbored
JSONbored merged commit d4cbf1f into main Jul 2, 2026
5 checks passed
@JSONbored
JSONbored deleted the codex/fix-lock-contention-bypassing-reopen-checks branch July 2, 2026 07:18
@github-project-automation github-project-automation Bot moved this from Todo to Done in gittensory - v1 roadmap Jul 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant