Skip to content

fix(pr): rewrite completion queue to be truly serial#340

Merged
jafreck merged 1 commit intomainfrom
fix/truly-serial-completion-queue
Mar 4, 2026
Merged

fix(pr): rewrite completion queue to be truly serial#340
jafreck merged 1 commit intomainfrom
fix/truly-serial-completion-queue

Conversation

@jafreck
Copy link
Copy Markdown
Owner

@jafreck jafreck commented Mar 4, 2026

Problem

PR #339 set p-limit(1) expecting serial execution, but executeItem calls were still spawned concurrently via enqueue() → ensureExecution(). The p-limit only gated the inner merge block, so multiple PRs would still fire their initial merge attempts simultaneously. This caused PR #18 to fail immediately (before #16's conflict resolution even started), and the completion queue appeared hung.

Root Cause

enqueue(#16) → ensureExecution(#16) → executeItem(#16) ─┐
enqueue(#18) → ensureExecution(#18) → executeItem(#18) ─┤ both run concurrently
                                                          └─> p-limit(1) only gates merge portion

Fix

Replaced the entire concurrent task model with a simple sequential drain() loop:

  • enqueue() now just collects items into an array
  • drain() iterates items one at a time, running the full resolve → wait → retry cycle for each before moving to the next
  • Each PR sees the fully updated base branch after the prior merge
  • DAG deps are checked via completed/failed sets instead of awaiting in-flight promises
  • Idempotent: drain() skips already-processed items

The previous p-limit(1) approach only serialized the inner merge block,
but executeItem calls still ran concurrently — meaning multiple PRs would
attempt merge simultaneously. PR #18 would fail immediately before #16's
conflict resolution even started.

Replace the concurrent task model with a simple sequential drain loop:
enqueue() collects items, drain() processes them one at a time in enqueue
order. Each PR gets the full resolve-wait-retry cycle before the next one
starts, so each sees the updated base branch after the prior merge.
@jafreck jafreck merged commit 0e7c3a7 into main Mar 4, 2026
2 checks passed
@jafreck jafreck deleted the fix/truly-serial-completion-queue branch March 4, 2026 00:40
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 4, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.22%. Comparing base (730ed0b) to head (49a32bc).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #340      +/-   ##
==========================================
- Coverage   93.23%   93.22%   -0.01%     
==========================================
  Files         193      193              
  Lines       19571    19555      -16     
  Branches     3087     3085       -2     
==========================================
- Hits        18247    18231      -16     
  Misses       1290     1290              
  Partials       34       34              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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