From 996c08d12b8c2e70650d0cade203778f3eb83e70 Mon Sep 17 00:00:00 2001 From: don-petry <36422719+don-petry@users.noreply.github.com> Date: Sat, 10 Oct 2026 16:19:33 -0500 Subject: [PATCH] fix(pr-review): retry the merge-queue state read and surface a failed read mq_dequeue_if_unapproved runs right after an approval is dismissed. Its one read of the PR's live queue and review state could fail transiently, and the only trace was a warning in the run log while a queued PR went on to merge with no approval standing. Retry the read a bounded number of times (MQ_READ_ATTEMPTS, default 3, with MQ_READ_RETRY_DELAY_SEC between, default 2). If every attempt fails, also write the unknown state to the job step summary. No PR comment on that path: most PRs that reach it are not in the queue. Tests: one failed read then success still dequeues; every read failing makes exactly MQ_READ_ATTEMPTS reads, no dequeue, no comment, a summary entry and exit 0; a malformed setting falls back to the default. Refs #2186 Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 5 ++++ scripts/lib/merge-queue-dequeue.sh | 35 ++++++++++++++++++++++- tests/test_merge_queue_dequeue.bats | 44 +++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index da759e7af..1a497c308 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -458,6 +458,11 @@ stops the merge: PR 2084 was approved, enqueued, had that approval dismissed, an call is a no-op. A failed dequeue logs a `::warning::` and posts one comment asking a maintainer to remove the PR from the queue by hand. An unreadable live state logs a `::warning::` and takes no action. The marker is posted at most once per head SHA. +- **A failed state read is retried, then surfaced.** The live read is tried `MQ_READ_ATTEMPTS` times + (default 3, `MQ_READ_RETRY_DELAY_SEC` apart, default 2). If every attempt fails the helper cannot tell + whether a queued PR lost its last approval: it logs a `::warning::`, writes the unknown state to the job + step summary so a maintainer can check the queue, makes no dequeue call and posts no PR comment (most PRs + that reach this path are not queued), and never fails the run (#2186). - **Auto-merge after a dequeue.** A dequeue consumes the PR's auto-merge request. dev-lead's idempotent `try_enable_auto_merge` re-enables it on its next pass; once an approval stands again, the PR rejoins the queue. This is intended. diff --git a/scripts/lib/merge-queue-dequeue.sh b/scripts/lib/merge-queue-dequeue.sh index 3ccb65b98..1055ec988 100644 --- a/scripts/lib/merge-queue-dequeue.sh +++ b/scripts/lib/merge-queue-dequeue.sh @@ -56,6 +56,30 @@ _mq_read_state() { ' 2>/dev/null } +# _mq_read_state_retry +# _mq_read_state with a bounded retry (#2186). The helper runs right after an +# approval was dismissed, and the window that matters is seconds long, so one +# transient GraphQL error must not end the check. Tunables (also for tests): +# MQ_READ_ATTEMPTS (default 3) and MQ_READ_RETRY_DELAY_SEC (default 2). +# Non-zero when every attempt failed. +_mq_read_state_retry() { + local owner="$1" name="$2" number="$3" st i + local attempts="${MQ_READ_ATTEMPTS:-3}" delay="${MQ_READ_RETRY_DELAY_SEC:-2}" + [[ "$attempts" =~ ^[1-9][0-9]*$ ]] || attempts=3 + [[ "$delay" =~ ^[0-9]+$ ]] || delay=2 + for ((i = 1; i <= attempts; i++)); do + if st=$(_mq_read_state "$owner" "$name" "$number"); then + printf '%s' "$st" + return 0 + fi + if [ "$i" -lt "$attempts" ]; then + echo " merge-queue: live-state read failed (attempt $i of $attempts) — retrying" >&2 + [ "$delay" -gt 0 ] && sleep "$delay" + fi + done + return 1 +} + # _mq_post_notice [marker_prefix] # Post unless a comment carrying this head's marker already exists, so # repeat runs at one head post one notice. If the comments cannot be listed, @@ -98,9 +122,18 @@ mq_dequeue_if_unapproved() { fi local st status_st=0 - st=$(_mq_read_state "$owner" "$name" "$number") || status_st=$? + st=$(_mq_read_state_retry "$owner" "$name" "$number") || status_st=$? if [ "$status_st" -ne 0 ]; then echo "::warning::merge-queue: could not read the merge-queue and review state of $pr_url — cannot tell whether a queued PR lost its last approval; check the queue by hand (#2174)" + # Surface it where a maintainer looks, not only in the run log (#2186). No PR + # comment on this path: most PRs that reach it are not in the queue at all. + if [ -n "${GITHUB_STEP_SUMMARY:-}" ]; then + { + printf '### Merge queue: state unknown after an approval was withdrawn\n\n' + printf 'The live merge-queue and review state of %s could not be read after every retry. ' "$pr_url" + printf 'If that pull request is in the merge queue it may merge without an approval: check the queue and remove it by hand if needed.\n\n' + } >> "$GITHUB_STEP_SUMMARY" 2>/dev/null || true + fi return 0 fi diff --git a/tests/test_merge_queue_dequeue.bats b/tests/test_merge_queue_dequeue.bats index 15a2fed6d..363b0282b 100644 --- a/tests/test_merge_queue_dequeue.bats +++ b/tests/test_merge_queue_dequeue.bats @@ -40,11 +40,15 @@ setup() { export PR_STATE_FILE="$TEST_DIR/pr_state"; echo OPEN > "$PR_STATE_FILE" export CLOCK_FILE="$TEST_DIR/clock"; echo 10 > "$CLOCK_FILE" export CALLS="$TEST_DIR/gh_calls.log"; : > "$CALLS" + # #2186: the live-state read retries; no real sleeps in tests. + export MQ_READ_RETRY_DELAY_SEC=0 + export READ_FAILS_FILE="$TEST_DIR/read_fails"; echo 0 > "$READ_FAILS_FILE" cat > "$TEST_DIR/bin/gh" <<'GHEOF' #!/bin/bash # Stateful gh mock for #2174. Env knobs: # GRAPHQL_READ_RC — non-zero ⇒ the live-state GraphQL read fails +# GRAPHQL_READ_FAIL_TIMES — N ⇒ only the first N live-state reads fail (#2186) # DEQUEUE_RC — non-zero ⇒ dequeuePullRequest fails (state unchanged) # LEAVE_QUEUE_ON_DEQUEUE — 1 ⇒ the PR leaves the queue just before the mutation, # which then fails (the check/call race) @@ -106,6 +110,12 @@ if [ "$1" = "api" ] && [ "$2" = "graphql" ]; then exit 0 fi [ "${GRAPHQL_READ_RC:-0}" = "0" ] || { echo 'HTTP 502' >&2; exit "$GRAPHQL_READ_RC"; } + if [ "${GRAPHQL_READ_FAIL_TIMES:-0}" != "0" ]; then + n=$(cat "$READ_FAILS_FILE") + if [ "$n" -lt "$GRAPHQL_READ_FAIL_TIMES" ]; then + echo $((n + 1)) > "$READ_FAILS_FILE"; echo 'HTTP 502' >&2; exit 1 + fi + fi # Live state: latest review per author (GitHub's latestOpinionatedReviews). jq -n --slurpfile r "$REVIEWS_FILE" --arg st "$(cat "$PR_STATE_FILE")" \ --arg q "$(cat "$QUEUED_FILE")" --arg h "$PR_HEAD_SHA" ' @@ -331,6 +341,40 @@ fix_request_verdict() { [ "$(jq length "$COMMENTS_FILE")" -eq 0 ] } +@test "live-state read fails once then succeeds → the queued, unapproved PR is still dequeued (#2186)" { + export GRAPHQL_READ_FAIL_TIMES=1 + run_lib + [ "$status" -eq 0 ] + [[ "$output" == *"attempt 1 of 3"* ]] + [ "$(dequeue_calls)" -eq 1 ] + [ "$(cat "$QUEUED_FILE")" = "false" ] + [ "$(marker_count)" -eq 1 ] +} + +@test "every live-state read fails → bounded retries, no dequeue, no comment, step-summary entry, exit 0 (#2186)" { + export GRAPHQL_READ_RC=1 MQ_READ_ATTEMPTS=4 + export GITHUB_STEP_SUMMARY="$TEST_DIR/summary.md"; : > "$GITHUB_STEP_SUMMARY" + run_lib + [ "$status" -eq 0 ] + [[ "$output" == *"::warning::"* ]] + # Exactly MQ_READ_ATTEMPTS reads, then it stops. + [ "$(grep -c 'api graphql' "$CALLS")" -eq 4 ] + [ "$(dequeue_calls)" -eq 0 ] + [ "$(jq length "$COMMENTS_FILE")" -eq 0 ] + grep -q 'state unknown after an approval was withdrawn' "$GITHUB_STEP_SUMMARY" + grep -qF "$PR_URL" "$GITHUB_STEP_SUMMARY" +} + +@test "a malformed retry setting falls back to the default instead of looping or aborting (#2186)" { + export GRAPHQL_READ_RC=1 MQ_READ_ATTEMPTS=abc MQ_READ_RETRY_DELAY_SEC=-1 + # -1 is not a valid delay, so the default (2s) would apply; keep the test fast + # by proving only the attempt count here with a valid zero delay. + export MQ_READ_RETRY_DELAY_SEC=0 + run_lib + [ "$status" -eq 0 ] + [ "$(grep -c 'api graphql' "$CALLS")" -eq 3 ] +} + @test "under set -e the helper never aborts its caller" { export DEQUEUE_RC=1 COMMENT_LIST_RC=1 run bash -c 'set -euo pipefail; source "$LIB"; mq_dequeue_if_unapproved "$1"; echo CONTINUED' _ "$PR_URL"