From 6feea4a3b8af9d2ef0043748d0760d571e3a913f Mon Sep 17 00:00:00 2001 From: Madison Oliver Date: Thu, 3 Sep 2026 12:06:18 -0400 Subject: [PATCH 1/2] Do not fail the sweep when a head branch is reused by a newer pull request Now that the sweep runs to completion it surfaces a condition that was previously unreachable. When a contributor closes a pull request and then reuses its head branch for a new one, the head branch no longer points at the SHA the closed pull request recorded. `delete_branch` correctly refuses to delete it -- deleting it would break the newer pull request -- but it reported that refusal as a failure, so the whole run went red. That state is permanent for as long as the newer pull request stays open, so the workflow would have been red on every run indefinitely. That is the same alert fatigue the previous fix set out to remove, just from a different cause. `jdeniau-GHSA-xvcm-6775-5m9r` is a live example: pull request 9129 was closed, one more commit was pushed to its head branch, and pull request 9226 was opened from it and is still open. Reusing a branch this way is a normal contributor pattern here, not an operational fault, and it needs no human intervention: once the newer pull request is closed its own cleanup removes the branch, the older pull request's head then reads as already absent, and the leftover staging branch is collected on the next sweep. Treat it the way the other "nothing safe to do here" states are already treated -- still open, fork-headed, deleted -- and skip it instead of failing. `delete_branch` now returns 3 for this case specifically, so a genuine deletion failure is still a failure and still stops the staging branch from being deleted after it. Nothing that was previously deleted is deleted any differently; only the reporting changes. Record the skipped branches and write them to the job summary along with the sweep counters, so they stay visible without being buried in a 1,600-line log and without holding the run red. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: daca0885-6779-4adb-bb02-ff6920d73ab3 --- ...lete_staging_and_head_branches_writer.yaml | 87 +++++++++++++++++-- 1 file changed, 80 insertions(+), 7 deletions(-) diff --git a/.github/workflows/delete_staging_and_head_branches_writer.yaml b/.github/workflows/delete_staging_and_head_branches_writer.yaml index e87a95ef7580..22065223afb7 100644 --- a/.github/workflows/delete_staging_and_head_branches_writer.yaml +++ b/.github/workflows/delete_staging_and_head_branches_writer.yaml @@ -165,8 +165,12 @@ jobs: fi if [[ -n "${expected_sha}" && "${current_sha}" != "${expected_sha}" ]]; then rm -f "${body_file}" - echo "::error::Head branch ${branch} now points to ${current_sha}, not ${expected_sha}; leaving it and the staging branch in place." - return 1 + # The branch moved after the pull request closed, which normally means a newer + # pull request reused it. Deleting it would break that pull request, so report + # this as a skip rather than a failure: it resolves itself once the newer pull + # request is closed and cleaned up in turn. + echo "::warning::Head branch ${branch} now points to ${current_sha}, not ${expected_sha}; it is in use elsewhere, so it and the staging branch are left in place." + return 3 fi if ! status="$(request_ref "${body_file}" DELETE "${encoded_branch}")"; then @@ -192,9 +196,15 @@ jobs: return 1 } + # Records a branch that was deliberately left in place, so the job summary can + # report it without the run having to fail. + record_skip() { + printf '%s\t%s\n' "$1" "$2" >> "${SKIPPED_FILE}" + } + process_pr() { local advisory_file_pages base_ref base_repo expected_staging_branch head_ref head_repo head_sha - local fetch_status=0 pr_json pr_number="$1" state + local delete_status=0 fetch_status=0 pr_json pr_number="$1" state expected_staging_branch="${2:-}" if ! is_pr_number "${pr_number}"; then @@ -274,7 +284,12 @@ jobs: fi if [[ "${head_ref}" == "${base_ref}" ]]; then - delete_branch "${base_ref}" "${head_sha}" || return 1 + delete_branch "${base_ref}" "${head_sha}" || delete_status=$? + if (( delete_status == 3 )); then + record_skip "${pr_number}" "${base_ref}" + return 0 + fi + (( delete_status == 0 )) || return 1 return 0 fi @@ -284,7 +299,14 @@ jobs: fi # Never delete the staging branch when the head branch could not be removed. - delete_branch "${head_ref}" "${head_sha}" || return 1 + delete_branch "${head_ref}" "${head_sha}" || delete_status=$? + if (( delete_status == 3 )); then + record_skip "${pr_number}" "${head_ref}" + return 0 + fi + if (( delete_status != 0 )); then + return 1 + fi delete_branch "${base_ref}" } @@ -393,17 +415,66 @@ jobs: "${candidates_file}" "${pairs_file}" || join_status=$? fi - echo "Inspected $(wc -l < "${pairs_file}" | tr -d ' ') staging branches." >&2 + INSPECTED_COUNT="$(wc -l < "${pairs_file}" | tr -d ' ')" + echo "Inspected ${INSPECTED_COUNT} staging branches." >&2 rm -rf "${pairs_file}" "${candidates_file}" "${chunk_dir}" # The cleanup above must not mask a failed join, otherwise the sweep would # report success while having reconciled nothing. return "${join_status}" } + # Branches left in place are expected rather than exceptional, so they are + # reported in the run summary instead of being buried in the log. + write_job_summary() { + local branch pr skipped_count=0 + + [[ -n "${GITHUB_STEP_SUMMARY:-}" ]] || return 0 + if [[ -s "${SKIPPED_FILE}" ]]; then + skipped_count="$(wc -l < "${SKIPPED_FILE}" | tr -d ' ')" + fi + # A per-pull-request run with nothing to report should not add an empty section. + if (( ! SWEEP && skipped_count == 0 )); then + return 0 + fi + + { + echo "### Staging branch cleanup" + echo + if (( SWEEP )); then + echo "| Metric | Count |" + echo "| --- | ---: |" + echo "| Staging branches inspected | ${INSPECTED_COUNT} |" + echo "| Pull requests reconciled | ${RECONCILE_COUNT} |" + echo "| Left in place (head branch in use) | ${skipped_count} |" + echo "| Pull request failures | ${PROCESS_FAILURES} |" + echo "| Triage batches failed | ${TRIAGE_FAILURES} |" + echo + fi + + if (( skipped_count > 0 )); then + echo "
Head branch reused by a newer pull request (${skipped_count})" + echo + echo "Deleting these branches would break the newer pull request that now uses them, so they were left alone. They are cleaned up automatically once that pull request is closed. No action is needed." + echo + while IFS=$'\t' read -r pr branch; do + echo "- \`${branch}\` — left over from #${pr}" + done < "${SKIPPED_FILE}" + echo + echo "
" + fi + } >> "${GITHUB_STEP_SUMMARY}" + } + TRIAGE_CHUNK_SIZE=50 TRIAGE_FAILURES=0 PROCESS_FAILURES=0 MISSING_PR_IS_ERROR=0 + INSPECTED_COUNT=0 + RECONCILE_COUNT=0 + SWEEP=0 + SKIPPED_FILE="$(mktemp)" + # An EXIT trap keeps the summary accurate even when the run ends in failure. + trap 'write_job_summary || true; rm -f "${SKIPPED_FILE}"' EXIT if [[ "${GITHUB_EVENT_NAME}" == "workflow_run" ]]; then PR_NUMBER="${WORKFLOW_RUN_PR_NUMBER}" @@ -416,9 +487,11 @@ jobs: MISSING_PR_IS_ERROR=1 process_pr "${DISPATCH_PR_NUMBER}" else + SWEEP=1 TARGETS_FILE="$(mktemp)" collect_reconciliation_targets > "${TARGETS_FILE}" - echo "Reconciling $(wc -l < "${TARGETS_FILE}" | tr -d ' ') staging branch(es)." + RECONCILE_COUNT="$(wc -l < "${TARGETS_FILE}" | tr -d ' ')" + echo "Reconciling ${RECONCILE_COUNT} staging branch(es)." # A single unreconcilable pull request must not stop the sweep, otherwise # every branch after it is never reconciled. From 7b26291ce2183376ba6966af3a9c513bdfc14ab6 Mon Sep 17 00:00:00 2001 From: Madison Oliver Date: Thu, 3 Sep 2026 12:18:39 -0400 Subject: [PATCH 2/2] Only treat a moved branch as benign when an open PR uses it A SHA mismatch was being reported as a self-healing skip on the assumption that a newer pull request had reused the branch. That assumption does not hold: a push after closure, or a ref deleted and recreated, produces the same mismatch. Those branches would have been reported as "no action needed" on every run while nothing ever collected them, so they would leak permanently. Confirm over the API that an open pull request currently has the branch as its head ref before skipping, and name that pull request in the warning and the job summary. An unexplained mismatch is now a failure that asks for investigation, and a failed lookup fails closed rather than being read as "nothing is using this branch". Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: daca0885-6779-4adb-bb02-ff6920d73ab3 --- ...lete_staging_and_head_branches_writer.yaml | 96 +++++++++++++++---- 1 file changed, 79 insertions(+), 17 deletions(-) diff --git a/.github/workflows/delete_staging_and_head_branches_writer.yaml b/.github/workflows/delete_staging_and_head_branches_writer.yaml index 22065223afb7..1437391f0622 100644 --- a/.github/workflows/delete_staging_and_head_branches_writer.yaml +++ b/.github/workflows/delete_staging_and_head_branches_writer.yaml @@ -113,6 +113,53 @@ jobs: rm -f "${body_file}" } + request_open_prs_for_head() { + local body_file="$1" + local encoded_branch="$2" + + curl --silent --show-error \ + --request GET \ + --output "${body_file}" \ + --write-out '%{http_code}' \ + --header "Accept: application/vnd.github+json" \ + --header "Authorization: Bearer ${GH_TOKEN}" \ + --header "X-GitHub-Api-Version: 2022-11-28" \ + "https://api.github.com/repos/${REPOSITORY}/pulls?state=open&per_page=100&head=${REPOSITORY%%/*}:${encoded_branch}" + } + + # Prints the number of an open pull request that currently uses the branch as its + # head ref. Returns 0 when one exists, 2 when none does, and 1 when the lookup + # failed. A failed lookup must never be read as "nothing is using this branch". + open_pr_using_branch() { + local branch="$1" + local body_file encoded_branch pr_number status + + encoded_branch="$(encode_ref "${branch}")" + body_file="$(mktemp)" + if ! status="$(request_open_prs_for_head "${body_file}" "${encoded_branch}")"; then + rm -f "${body_file}" + echo "::error::Failed to look up open pull requests for branch ${branch}." >&2 + return 1 + fi + if [[ "${status}" != "200" ]]; then + cat "${body_file}" >&2 + rm -f "${body_file}" + echo "::error::Failed to look up open pull requests for branch ${branch}: GitHub API returned ${status}." >&2 + return 1 + fi + + # The head filter is re-checked locally so that an unexpected response shape + # reports "none found" rather than silently authorising a deletion. + if ! pr_number="$(jq -er --arg branch "${branch}" --arg repo "${REPOSITORY}" \ + 'map(select(.head.ref == $branch and .head.repo.full_name == $repo)) | first | .number' \ + "${body_file}")"; then + rm -f "${body_file}" + return 2 + fi + rm -f "${body_file}" + printf '%s\n' "${pr_number}" + } + request_ref() { local body_file="$1" local method="$2" @@ -136,7 +183,7 @@ jobs: delete_branch() { local branch="$1" local expected_sha="${2:-}" - local body_file current_sha encoded_branch status + local blocking_pr body_file current_sha encoded_branch lookup_status status encoded_branch="$(encode_ref "${branch}")" body_file="$(mktemp)" @@ -165,12 +212,25 @@ jobs: fi if [[ -n "${expected_sha}" && "${current_sha}" != "${expected_sha}" ]]; then rm -f "${body_file}" - # The branch moved after the pull request closed, which normally means a newer - # pull request reused it. Deleting it would break that pull request, so report - # this as a skip rather than a failure: it resolves itself once the newer pull - # request is closed and cleaned up in turn. - echo "::warning::Head branch ${branch} now points to ${current_sha}, not ${expected_sha}; it is in use elsewhere, so it and the staging branch are left in place." - return 3 + # The branch moved after the pull request closed. That is only safe to ignore + # when another open pull request is using it: deleting it would break that + # pull request, and it is collected anyway once that pull request closes in + # turn. Any other cause -- a push after closure, or a ref deleted and + # recreated -- leaves an orphan that nothing else will ever collect, so it has + # to be surfaced rather than silently reported as a self-healing skip. + lookup_status=0 + blocking_pr="$(open_pr_using_branch "${branch}")" || lookup_status=$? + if (( lookup_status == 0 )); then + BLOCKING_PR="${blocking_pr}" + echo "::warning::Branch ${branch} now points to ${current_sha}, not ${expected_sha}, because open pull request #${blocking_pr} is using it; it and the staging branch are left in place." + return 3 + fi + if (( lookup_status == 2 )); then + echo "::error::Branch ${branch} now points to ${current_sha}, not ${expected_sha}, and no open pull request is using it. Leaving it in place because the change is unexplained; this branch needs manual investigation." + return 1 + fi + echo "::error::Could not determine whether branch ${branch} is still in use, so it was left in place." + return 1 fi if ! status="$(request_ref "${body_file}" DELETE "${encoded_branch}")"; then @@ -196,10 +256,11 @@ jobs: return 1 } - # Records a branch that was deliberately left in place, so the job summary can - # report it without the run having to fail. + # Records a branch that was deliberately left in place, along with the open pull + # request that is using it, so the job summary can report it without the run + # having to fail. record_skip() { - printf '%s\t%s\n' "$1" "$2" >> "${SKIPPED_FILE}" + printf '%s\t%s\t%s\n' "$1" "$2" "$3" >> "${SKIPPED_FILE}" } process_pr() { @@ -286,7 +347,7 @@ jobs: if [[ "${head_ref}" == "${base_ref}" ]]; then delete_branch "${base_ref}" "${head_sha}" || delete_status=$? if (( delete_status == 3 )); then - record_skip "${pr_number}" "${base_ref}" + record_skip "${pr_number}" "${base_ref}" "${BLOCKING_PR}" return 0 fi (( delete_status == 0 )) || return 1 @@ -301,7 +362,7 @@ jobs: # Never delete the staging branch when the head branch could not be removed. delete_branch "${head_ref}" "${head_sha}" || delete_status=$? if (( delete_status == 3 )); then - record_skip "${pr_number}" "${head_ref}" + record_skip "${pr_number}" "${head_ref}" "${BLOCKING_PR}" return 0 fi if (( delete_status != 0 )); then @@ -426,7 +487,7 @@ jobs: # Branches left in place are expected rather than exceptional, so they are # reported in the run summary instead of being buried in the log. write_job_summary() { - local branch pr skipped_count=0 + local blocking_pr branch pr skipped_count=0 [[ -n "${GITHUB_STEP_SUMMARY:-}" ]] || return 0 if [[ -s "${SKIPPED_FILE}" ]]; then @@ -445,7 +506,7 @@ jobs: echo "| --- | ---: |" echo "| Staging branches inspected | ${INSPECTED_COUNT} |" echo "| Pull requests reconciled | ${RECONCILE_COUNT} |" - echo "| Left in place (head branch in use) | ${skipped_count} |" + echo "| Left in place (branch reused by an open pull request) | ${skipped_count} |" echo "| Pull request failures | ${PROCESS_FAILURES} |" echo "| Triage batches failed | ${TRIAGE_FAILURES} |" echo @@ -454,10 +515,10 @@ jobs: if (( skipped_count > 0 )); then echo "
Head branch reused by a newer pull request (${skipped_count})" echo - echo "Deleting these branches would break the newer pull request that now uses them, so they were left alone. They are cleaned up automatically once that pull request is closed. No action is needed." + echo "Each of these branches was verified to still be the head ref of an open pull request, so deleting it would break that pull request and it was left alone. They are collected automatically once that pull request is closed. No action is needed." echo - while IFS=$'\t' read -r pr branch; do - echo "- \`${branch}\` — left over from #${pr}" + while IFS=$'\t' read -r pr branch blocking_pr; do + echo "- \`${branch}\` — left over from #${pr}, still in use by open pull request #${blocking_pr}" done < "${SKIPPED_FILE}" echo echo "
" @@ -472,6 +533,7 @@ jobs: INSPECTED_COUNT=0 RECONCILE_COUNT=0 SWEEP=0 + BLOCKING_PR="" SKIPPED_FILE="$(mktemp)" # An EXIT trap keeps the summary accurate even when the run ends in failure. trap 'write_job_summary || true; rm -f "${SKIPPED_FILE}"' EXIT