[ci]: harden best-effort Buildkite cancellation

Author: SolitaryThinker

.github/workflows/ci-trigger-full-suite.yml

@@ -44,37 +44,83 @@ jobs:
         continue-on-error: true
         if: steps.check.outputs.has_ready == 'true'
         env:
+          BK_ORG: ${{ vars.BUILDKITE_ORG_SLUG }}
+          BK_PIPELINE: ${{ vars.BUILDKITE_PIPELINE_SLUG }}
           BUILDKITE_API_TOKEN: ${{ secrets.BUILDKITE_API_TOKEN }}
           PR_BRANCH: ${{ github.event.pull_request.head.ref }}
           PR_NUMBER: ${{ github.event.pull_request.number }}
         run: |
-          # `curl` without --fail-with-body treats an HTTP error as success and
-          # pipes the error body onward. Buildkite answers a rate limit or an
-          # unauthorized read with an object such as {"message": "Not Found"},
-          # and `.[]` over an object yields its values, so `.env` then runs
-          # against a string and jq exits non-zero.
-          if ! response=$(curl -sS --get --fail-with-body \
+          set -euo pipefail
+          response_file=$(mktemp)
+          builds_file=$(mktemp)
+          trap 'rm -f "$response_file" "$builds_file"' EXIT
+
+          if [[ ! "$PR_NUMBER" =~ ^[1-9][0-9]*$ ]]; then
+            echo "::warning::Invalid pull request number; stale Buildkite builds may continue."
+            exit 1
+          fi
+
+          if ! curl -sS --fail-with-body --get \
             -H "Authorization: Bearer $BUILDKITE_API_TOKEN" \
             --data-urlencode "branch=$PR_BRANCH" \
-            --data-urlencode "state=running,scheduled" \
-            "https://api.buildkite.com/v2/organizations/${{ vars.BUILDKITE_ORG_SLUG }}/pipelines/${{ vars.BUILDKITE_PIPELINE_SLUG }}/builds" 2>&1); then
-            echo "::warning::Could not list Buildkite builds, skipping cancellation: $response"
-            exit 0
+            --data-urlencode "state[]=running" \
+            --data-urlencode "state[]=scheduled" \
+            --data-urlencode "exclude_jobs=true" \
+            --data-urlencode "exclude_pipeline=true" \
+            --output "$response_file" \
+            "https://api.buildkite.com/v2/organizations/${BK_ORG}/pipelines/${BK_PIPELINE}/builds"; then
+            echo "::warning::Could not list Buildkite builds; stale merge-gate builds may continue."
+            exit 1
+          fi
+
+          if ! jq -e '
+            if type != "array" then false
+            else all(.[];
+              if type != "object" then false
+              else
+                (.number | if type == "number" then . > 0 and floor == . else false end)
+                and (
+                  (.env? | if . == null then {} else . end) as $env
+                  | if ($env | type) != "object" then false
+                    else
+                      ($env.TEST_SCOPE? | . == null or type == "string")
+                      and ($env.PR_NUMBER? | . == null or type == "string")
+                    end
+                )
+              end
+            )
+            end
+          ' "$response_file" >/dev/null 2>&1; then
+            # Do not print the response body: it is remote data and may contain
+            # multiline values that would be interpreted as workflow commands.
+            echo "::warning::Buildkite returned an invalid build list; stale merge-gate builds may continue."
+            exit 1
           fi
 
           # Match both branch and PR number: forks can reuse the same branch name.
-          if ! builds=$(printf '%s' "$response" | jq -r --arg pr_number "$PR_NUMBER" \
-            'if type == "array" then .[] else empty end
-             | select((.env.TEST_SCOPE? == "merge") and (.env.PR_NUMBER? == $pr_number))
-             | .number' 2>&1); then
-            echo "::warning::Could not parse the Buildkite build list, skipping cancellation: $builds"
-            exit 0
+          if ! jq -r --arg pr_number "$PR_NUMBER" '
+            .[]
+            | select((.env.TEST_SCOPE? == "merge") and (.env.PR_NUMBER? == $pr_number))
+            | .number
+          ' "$response_file" > "$builds_file"; then
+            echo "::warning::Could not select stale Buildkite builds; stale merge-gate builds may continue."
+            exit 1
           fi
-          for build_num in $builds; do
+
+          cancellation_failed=0
+          while IFS= read -r build_num; do
             echo "Cancelling Buildkite build #$build_num"
-            curl -sS -X PUT -H "Authorization: Bearer $BUILDKITE_API_TOKEN" \
-              "https://api.buildkite.com/v2/organizations/${{ vars.BUILDKITE_ORG_SLUG }}/pipelines/${{ vars.BUILDKITE_PIPELINE_SLUG }}/builds/${build_num}/cancel"
-          done
+            if ! curl -sS --fail-with-body -o /dev/null -X PUT \
+              -H "Authorization: Bearer $BUILDKITE_API_TOKEN" \
+              "https://api.buildkite.com/v2/organizations/${BK_ORG}/pipelines/${BK_PIPELINE}/builds/${build_num}/cancel"; then
+              echo "::warning::Could not cancel Buildkite build #$build_num; trying remaining builds."
+              cancellation_failed=1
+            fi
+          done < "$builds_file"
+
+          if (( cancellation_failed != 0 )); then
+            exit 1
+          fi
 
       # Check out the immutable BASE SHA: pull_request_target must never run a
       # planner or gate script from the untrusted PR head.

fastvideo/tests/contract/test_ci_test_collection.py

@@ -164,6 +164,45 @@ def test_merge_comment_has_one_change_aware_trigger_path():
     assert "__FASTVIDEO_CI_PLAN_ALL__" in ready_workflow
 
 
+def test_merge_gate_buildkite_cancellation_is_best_effort_and_strict():
+    workflow = yaml.safe_load((REPO_ROOT / ".github/workflows/ci-trigger-full-suite.yml").read_text())
+    steps = workflow["jobs"]["trigger"]["steps"]
+    cancel_step = next(step for step in steps if step.get("name") == "Cancel previous Buildkite builds")
+    trigger_step = next(step for step in steps if step.get("name") == "Trigger Buildkite merge gate")
+    cancel_script = cancel_step["run"]
+
+    # Stale-build cleanup is best effort; creating the replacement gate remains
+    # a hard failure so merge protection can never pass without a test build.
+    assert cancel_step["continue-on-error"] is True
+    assert trigger_step.get("continue-on-error") is not True
+    assert "curl -sS --fail-with-body -X POST" in trigger_step["run"]
+
+    assert cancel_script.splitlines()[0] == "set -euo pipefail"
+    assert "curl -sS --fail-with-body --get" in cancel_script
+    assert cancel_script.count('--data-urlencode "state[]=running"') == 1
+    assert cancel_script.count('--data-urlencode "state[]=scheduled"') == 1
+    assert cancel_script.count('--data-urlencode "exclude_jobs=true"') == 1
+    assert cancel_script.count('--data-urlencode "exclude_pipeline=true"') == 1
+    assert 'state=running,scheduled' not in cancel_script
+
+    # Only a validated array of build records can reach the URL construction.
+    assert 'if type != "array" then false' in cancel_script
+    assert 'if type != "object" then false' in cancel_script
+    assert 'if type == "number" then . > 0 and floor == .' in cancel_script
+    assert 'if ($env | type) != "object" then false' in cancel_script
+    assert '$env.TEST_SCOPE?' in cancel_script
+    assert '$env.PR_NUMBER?' in cancel_script
+
+    # API bodies are kept out of annotations, every cancellation is checked,
+    # and one failure does not stop attempts for the remaining build numbers.
+    assert '--output "$response_file"' in cancel_script
+    assert 'cat "$response_file"' not in cancel_script
+    assert "if ! curl -sS --fail-with-body -o /dev/null -X PUT" in cancel_script
+    assert "cancellation_failed=1" in cancel_script
+    assert cancel_script.index("cancellation_failed=1") < cancel_script.index('done < "$builds_file"')
+    assert cancel_script.index('done < "$builds_file"') < cancel_script.index("if (( cancellation_failed != 0 ))")
+
+
 def test_full_ssim_has_a_weekly_slurm_schedule():
     workflow = (REPO_ROOT / ".github/workflows/ci-scheduled-ssim.yml").read_text()
     ssim_step = next(step for step in _pipeline_steps() if step["key"] == "ssim")