Skip to content

dev-lead recovery can queue a duplicate fix-reviews run after the 600s dispatch guard expires — use a durable in-flight claim #1797

Open

Description

@don-petry

Summary

dev-lead's dropped-review recovery can still queue one duplicate fix-reviews run for a commit after its 600-second dispatch guard expires. Follow-up from PR #1742 (#1741). Both review bots raised it there, and it was correctly declined as out of that PR's scope.

Why it is bounded today (verified at PR #1742 head 4488a924)

scan_pr_for_dropped_reviews gates every recovery dispatch in order:

  1. has_dispatch_guard skips when a SHA-keyed dev-lead-dispatch-guard comment is younger than DISPATCH_GUARD_WINDOW_SEC (default 600).
  2. has_unaddressed_head_findings skips once any dev-lead-fix-reviews marker exists for the head SHA.
  3. post_dispatch_guard writes the guard, then dispatch_reviews_retry.

The retry lane serializes (dev-lead-retry.yml: concurrency: group: dev-lead-retry, cancel-in-progress: false), so a duplicate queues rather than fanning out.

The remaining gap

The dev-lead-fix-reviews marker is posted after the agent finishes, and the no-op path posts none. A fix run that is still in flight, or queued behind a saturated runner pool, more than 600s after dispatch has an expired guard and no marker yet. The next scan (two-hour cadence) re-dispatches the same SHA, and the duplicate queues behind the original. On GitHub Free's 20-concurrent-job cap, queued duplicates directly delay real work (see petry-projects/.github#1126).

Acceptance Criteria

  1. Replace the fixed 600s guard with a durable in-flight claim per (PR, head SHA), recorded before dispatch. Its expiry must exceed the maximum fix-run duration: action budget plus queue allowance. Derive that from the configured budget, not a new magic number.
  2. Expired claims are reclaimable, so a cancelled or crashed run can still be recovered. A claim is never permanent.
  3. A no-op fix run clears or finalizes its claim, so it doesn't look in flight until expiry.
  4. Keep the existing marker check and retry-lane serialization. This adds a claim, it doesn't replace the other guards.
  5. Tests: in-flight claim blocks re-dispatch past 600s; expired claim is reclaimed; no-op run finalizes its claim; a new head SHA is never blocked by an old claim.
  6. shellcheck --severity=warning -x clean; tests registered in the existing bats job.

Dev Notes

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions