Skip to content

fix(actions): retain the latest pending coalescer event - #1770

Closed
seonghobae wants to merge 4 commits into
mainfrom
fix/current_head_coalescer_pending_queue
Closed

fix(actions): retain the latest pending coalescer event#1770
seonghobae wants to merge 4 commits into
mainfrom
fix/current_head_coalescer_pending_queue

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Problem

Merged PR #1769 correctly stopped an active Current Head Run Coalescer worker from cancelling itself by setting cancel-in-progress: false. The remaining contract was implicit: GitHub's default queue: single keeps at most one pending successor and replaces it with each newer event.

The first revision of this successor tried queue: max. Devin's exact-head review correctly found that this would preserve up to 100 stale event-bound invocations and then reject later events, potentially leaving no cleanup opportunity for the final head. The coalescer cannot treat an old invocation as current because current_head_run_coalescer.py compares the event's EXPECTED_HEAD with the live PR before classification and refuses mutation after a head change.

Repaired design

  • retain cancel-in-progress: false so an active cleanup worker completes;
  • declare queue: single explicitly so each later synchronize event replaces the older pending invocation and the sole successor tracks the latest head;
  • reject queue: max in the focused contract to prevent stale-head FIFO buildup and the 100-pending rejection boundary;
  • keep all live PR, exact-head, run-association, sibling, and pre-cancellation revalidation unchanged.

This is a bounded clarification/hardening successor to #1769. It also supersedes only the now-proven-harmful queue: max fragment previously present in broad/non-mergeable #1661; no unrelated delta is copied or discarded.

TDD and review evidence

Initial hypothesis, now superseded by review:

  • f02486dbc159f5949305d4888656d0ca5120b37d — RED requiring queue: max;
  • ed2acac49df68da1262bb9ee6e5b325c9f21cd8c — initial GREEN adding it.

Reviewer-driven correction:

  • 05517598240c8c524657f3c7647b66ef6e38257d — RED requiring latest-pending queue: single and rejecting queue: max while production still used max;
  • d6bf882a8b35ed5d16ca3c8301a07a9da10297be — GREEN production repair.

Exact branch comparison at this head: 4 commits ahead, 0 behind protected main@bf28b5ddca7d4d63f3e6f63a43d084a0056563e2; effective delta remains limited to the coalescer workflow and its focused regression contract.

GitHub's current concurrency specification states that single is the default latest-pending behavior, while max retains up to 100 pending runs and cancels later arrivals when full. This control-plane worker needs the former, not FIFO accumulation.

Merge policy

This is a central queue-admission/control-plane repair for #712. Re-fetch exact head, reviews, unresolved threads, and checks before merge. The user's Chicken-and-Eggs bypass applies only if this unchanged head is blocked because its own required workflows cannot obtain runner admission under the exact ceiling it reduces. No required security/review context is removed or synthesized.

Successor lineage: #1769, #1661, #712.

@coderabbitai

coderabbitai Bot commented Sep 3, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 18 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 642c7d93-72ed-46d1-b761-4477236d24cd

📥 Commits

Reviewing files that changed from the base of the PR and between bf28b5d and d6bf882.

📒 Files selected for processing (2)
  • .github/workflows/current-head-run-coalescer.yml
  • tests/test_current_head_coalescer_self_cancellation.py

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

devin-ai-integration[bot]

This comment was marked as resolved.

@seonghobae seonghobae changed the title fix(actions): preserve pending current-head cleanup runs fix(actions): retain the latest pending coalescer event Sep 3, 2026

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Devin Review found 2 new potential issues.

Devin Review

# the latest head instead of accumulating up to 100 stale-head invocations.
# The script still verifies the event-bound expected head against the live PR
# before any mutation and fails closed if the head moved again.
queue: single

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔴 Cleanup runs starve during push bursts

During runner starvation, queue: single replaces each waiting cleanup whenever another push arrives. Sustained pushes can prevent cleanup from running, leaving redundant jobs consuming the shared ceiling.

Prompt for agents
Restore the multi-pending concurrency policy for .github/workflows/current-head-run-coalescer.yml and update its adjacent explanation and regression contract together. The workflow needs to retain pending cleanup invocations during runner starvation rather than replacing the sole pending invocation on every synchronize event. Preserve cancel-in-progress: false because active cleanup workers must finish, and keep the script's existing live-head fail-closed validation for older triggers.
Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +12 to +17
# Keep exactly one pending successor. GitHub replaces that pending run when a
# newer event enters the group, so the retained successor always represents
# the latest head instead of accumulating up to 100 stale-head invocations.
# The script still verifies the event-bound expected head against the live PR
# before any mutation and fails closed if the head moved again.
queue: single

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔍 PR narrative describes reversed policy

The description promises queue: max, while the final head requires queue: single. Update the review and release record after restoring the intended policy.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@seonghobae

seonghobae commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Closing — full succession by .github#1775

Reviewed the full diff. This PR proposes adding queue: single to current-head-run-coalescer.yml's concurrency block (on top of the cancel-in-progress: false .github#1769 already landed), reasoning that GitHub's default queue: single behavior (replace the one pending run with each newer event) is the correct latest-pending policy for this worker.

Verified queue: is a real, documented GitHub Actions concurrency key (single/max, confirmed against live GitHub docs) before evaluating further -- the mechanism itself is real, but main has since moved past this specific design:

.github#1775 (merged, commit cbd1280) found and fixed exactly the failure mode this PR's own queue: single choice still has: under near-zero Actions runner admission, rapid same-PR pushes replace the ONE retained pending run before it ever gets a runner, so the coalescer can go through an entire push burst without ever actually executing once -- verified live against .github#1741, whose required-review checks sat stuck queued because the coalescer never ran. #1775's fix is queue: max (retain up to 100 pending runs, GitHub's documented ceiling, so at least one eventually gets a runner), not queue: single. Merging this PR as-is would revert current-head-run-coalescer.yml from the queue: max fix back to queue: single, reintroducing the exact #1741-class incident #1775 closed.

Both test files this PR touches (tests/test_current_head_coalescer_self_cancellation.py, plus a third file this PR doesn't know about, tests/test_current_head_run_coalescer_review_regressions.py) already assert queue: max on current main with the corrected reasoning documented in-file.

No unlanded delta remains -- #1775 supersedes this PR's intent with a more correct design. Closing per this org's full-succession close criterion, not as a bare stale-PR close.

@seonghobae seonghobae closed this Sep 3, 2026
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