Skip to content

fix(jobs): stop follow-ups waiting behind an operator hold on a closed PR - #3349

Open
integry wants to merge 1 commit into
mainfrom
fix/pr-followup-stop-on-closed-pr
Open

integry wants to merge 1 commit into
mainfrom
fix/pr-followup-stop-on-closed-pr

Conversation

@integry

@integry integry commented Oct 24, 2026

Copy link
Copy Markdown
Owner

Why

On integry/mcptest#447, a /fix follow-up at 18:45 finished its agent run, but ProPR found the worktree was not on the PR branch. It parked the follow-up for an operator (awaiting_operator / worktree_not_on_branch).

Every later attempt for that PR then hit the predecessor gate and rescheduled itself. That was 103 tasks between 18:45 and 20:39, one per minute; task 38316 is the last. The PR merged at 20:36 and the loop didn't notice. It only stopped because the next attempt failed on disk pressure.

There are two causes:

  1. The closed-PR check runs too late. processPullRequestCommentJob already skips merged or closed PRs, but only inside validatePRAndComments. That runs after gatePredecessorHandoff, so a blocked job never reaches it.
  2. Nothing clears the hold. No code ever settles an operator hold on a closed PR. The existing closed-PR sweep (cancelClosedPullRequestValidation) only cancels CI runs, and it's opt-in and webhook-only. The hold therefore blocks that PR's follow-ups forever.

f5e3510 (wait in place with backoff) is on main but not deployed yet. It turns the per-minute flood into one backing-off task, but that task would still never stop.

Fix

  • Gate: gatePredecessorHandoff reads the PR state, but only when a predecessor blocks the job.
    • Merged or closed: the job ends as cancelled_pr_closed. Its claimed comments are not returned to the pending list, the same as a job that finds the PR closed later. It releases its PR lease and then releases the hold.
    • Open, or state unreadable: the existing wait in place is unchanged.
  • Sweep: releaseClosedPullRequestHandoffs runs with the worker's periodic handoff recovery. It releases operator holds whose PR has since closed, reading each PR at most every 10 minutes, and deletes the PR's queued comments. A refused release (agent still running, liveness unknown, lease held) is retried after the interval.
  • Release: both paths use the existing operator release (releaseFollowupHandoffRecord) with force.
    • It still requires the agent to be proven gone and the PR lease to be taken.
    • It only detaches the worktree HEAD; retained files and commits are never deleted.
    • It records released_pr_merged / released_pr_closed.

Tests

test/followupHandoffReviewBlockers.test.ts, using real git fixtures:

  • A blocked follow-up on a merged PR ends without waiting or queueing a replacement, releases the lease, then the hold.
  • Open and unreadable PR states still wait in place.
  • readPullRequestClosure distinguishes merged, closed and open.
  • The sweep leaves open PRs alone, respects the 10-minute interval, releases a merged PR's hold with retained edits kept, and deletes the pending-comments key.
  • The sweep leaves a hold whose agent is still running.

Related handoff, PR comment, CI and worker suites pass. prPublicationJob and prPublicationRecoveryJob needed the new module added to their @propr/core-style module mocks.

These fail on main too, for local-environment reasons:

  • two F23 permission tests (they run as root here);
  • configRoutes.followup;
  • processGoalJob: the local test DB already has another branch's migration recorded.

After deploy

PR #447's hold is released on the first sweep after deploy. The host disk is at 96%, which is what failed task 38316; that should be looked at separately.

🤖 Generated with Claude Code

…d PR

A follow-up held for an operator (integry/mcptest#447: worktree_not_on_branch)
blocked every later follow-up for its pull request. The blocked job only
checked whether the PR was closed after the predecessor gate, so after the
merge it kept waiting - before f5e3510 by queueing a replacement task every
minute (103 tasks in two hours), after it by backing off forever - and the
hold itself was never settled.

- gatePredecessorHandoff reads the PR state when a predecessor blocks it. A
  merged or closed PR ends the job as cancelled_pr_closed without returning
  its comments to the pending list, then releases the hold. An open PR or an
  unreadable state keeps the existing wait.
- releaseClosedPullRequestHandoffs, run with the periodic handoff recovery,
  releases operator holds whose PR has since closed (each PR read at most
  every 10 minutes) and drops the comments still queued for it.
- Both use the operator release, forced: the agent must be proven gone and
  the PR lease taken, and retained files and commits stay in the worktree.
  The release records released_pr_merged / released_pr_closed.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@integry

integry commented Oct 25, 2026

Copy link
Copy Markdown
Owner Author

/ultrafix

@propr-dev propr-dev Bot added the ultrafix label Oct 25, 2026
@propr-dev

propr-dev Bot commented Oct 25, 2026

Copy link
Copy Markdown
Contributor

🔄 Ultrafix loop started (goal: 8/10, max cycles: 10)

First action: /review

💡 Tip: Remove the ultrafix label from this PR to stop further ultrafix cycles.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants