Skip to content

[ci]: make merge-gate startup reliable - #1762

Open
KyleNeverGivesUp wants to merge 4 commits into
hao-ai-lab:mainfrom
KyleNeverGivesUp:ci-cancel-must-not-block-merge-gate
Open

[ci]: make merge-gate startup reliable#1762
KyleNeverGivesUp wants to merge 4 commits into
hao-ai-lab:mainfrom
KyleNeverGivesUp:ci-cancel-must-not-block-merge-gate

Conversation

@KyleNeverGivesUp

@KyleNeverGivesUp KyleNeverGivesUp commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Problem

The merge gate had two independent startup failures:

  1. /merge refreshed the ready label with GITHUB_TOKEN, but GitHub suppresses workflow runs caused by that token for labeled events. On a stable PR head, the command therefore did not start ci-trigger-full-suite at all.
  2. Once manually triggered, the workflow cancelled stale Buildkite builds before creating the replacement gate. That cleanup is only an optimization, but malformed state filtering or response parsing could fail the job before the hard-fail Buildkite POST. This was the infrastructure failure behind the missing/red gate on [perf]: MiniMax H3 on GB10 - skip text encoder CPU offload on unified memory (5m49s to 30ms) #1710. A failing build can also retain active GPU jobs, as build #5043 demonstrated.

Changes

  • Make ci-trigger-full-suite.yml a local reusable workflow and call it directly after the /merge authorization and label job succeeds. No PAT, GitHub App token, or actions: write permission is introduced; only the existing Buildkite secret is passed explicitly.
  • Resolve and validate one authenticated snapshot of the open PR, require it to target this repository's default branch, recheck ready, and use its number/head/base/title outputs throughout the privileged workflow. PR head code is never checked out there.
  • Make label addition idempotent and keep only the cosmetic rocket reaction best-effort.
  • Keep stale-build cancellation explicitly best-effort with continue-on-error: true; the replacement Buildkite POST remains hard-fail.
  • Bound cleanup to a 3-minute step and every list/cancel request to a 5-second connect and 20-second total timeout.
  • Query Buildkite with repeated state[]=running, state[]=scheduled, and state[]=failing parameters, and omit jobs/pipeline payloads.
  • Use curl --fail-with-body for list and cancel requests.
  • Store the remote response in a mode-600 temporary file and never echo its body into GitHub workflow commands.
  • Reject malformed arrays, records, build numbers, env objects, and non-string matching fields before any build number reaches a cancellation URL.
  • Match both merge scope and PR number, attempt every matching cancellation after one fails, and fail only the cleanup step.
  • Extend CPU contracts over the reusable call, fail-closed PR resolution, normalized event context, best-effort cleanup, timeout/state coverage, and hard-fail trigger boundary.
  • Update contributor documentation for the direct reusable-workflow call.

This incorporates and supersedes #1760.

Verification

  • pytest -q fastvideo/tests/contract: 88 passed on exact head 4c65c8e6.
  • Targeted pre-commit: codespell, PyMarkdown, actionlint, filename, and suggestion hooks passed; configured test-path exclusions were respected.
  • git diff --check upstream/main...HEAD: clean.
  • Manual execution of the exact cancellation block with fake curl verified that a failed first PUT still attempts the second, rejects an unrelated PR, and exits non-zero only for continue-on-error handling.
  • A malformed response containing a forged workflow command was rejected without printing the response body.
  • Two independent final reviews found no unresolved P0-P3 issue on the exact candidate.

The new privileged /merge path cannot run from a PR head before this workflow exists on the default branch; one post-merge canary should validate it end to end.

The contributor author/date/message are preserved; the Claude co-author trailer was removed during the reviewed rewrite.

@mergify mergify Bot added type: ci CI/CD infrastructure scope: infra CI, tests, Docker, build labels Aug 26, 2026
@mergify

mergify Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Merge Protections

🔴 1 of 1 protections blocking · waiting on 👀 reviews and 🤖 CI

Protection Waiting on
🔴 PR merge requirements 👀 reviews and 🤖 CI

🔴 PR merge requirements

Waiting for

  • #approved-reviews-by>=1
  • check-success=full-suite-passed
This rule is failing.
  • #approved-reviews-by>=1
  • check-success=full-suite-passed
  • check-success=fastcheck-passed
  • check-success~=pre-commit
  • title~=(?i)^\[(feat|feature|bugfix|fix|refactor|perf|ci|doc|docs|misc|chore|kernel|new.?model|skill|skills|infra)\]

KyleNeverGivesUp added a commit to KyleNeverGivesUp/FastVideo that referenced this pull request Aug 26, 2026
The trigger check failed while cancelling stale Buildkite builds, which ends
the job before the merge-gate build is started, so this PR has no gate result
rather than a failing one. Nothing here needs changing; this empty commit only
produces the synchronize event that runs the workflow again.

The underlying fault is fixed in hao-ai-lab#1762.

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

Copy link
Copy Markdown
Contributor Author

This reproduced again today, so it is deterministic rather than a transient API blip.

Trigger Merge Gate on #1725, run 32959115832, 2026-08-26 10:36 UTC:

jq: error (at <stdin>:1): Cannot index string with string "env"
Process completed with exit code 5

The same workflow succeeded seven minutes earlier on two other branches, so the Buildkite API was not down:

time UTC branch trigger
10:36 report-signal-terminated-workers, #1725 failure
10:29 skip-dit-offload-on-unified-memory, #1715 success
10:29 release-checkpoint-tensors-during-load, #1714 success

#1725 has no failing test. Its only red check is this one, and re-triggering does not clear it, because the cancel step fails the same way every time and the job ends before step 7 starts the gate build. That PR therefore has no gate result rather than a failing one.

Why this branch and not the others is still unknown, and the reason it is unknown is the second change in this PR: the current step does not print the response body, so the log shows only that jq could not parse something. With --fail-with-body and the warning, the next occurrence says whether it was a rate limit, an authorization result, or something else.

Because the workflow runs on: pull_request_target, it executes from the base branch. Merging this fixes the trigger for every open pull request without any of them needing a change.

@KyleNeverGivesUp KyleNeverGivesUp changed the title [ci]: do not abort the merge-gate trigger when cancelling old builds fails [ci]: a failed build cleanup should not be reported as a failed merge gate Aug 26, 2026
KyleNeverGivesUp and others added 2 commits August 26, 2026 04:17
…fails

The Trigger Merge Gate workflow cancels stale Buildkite builds in step 2 and
starts the merge-gate build in step 7. A step failure ends the job, so when
the cancel step dies the gate build is never started at all. The check then
goes red for a reason unrelated to the pull request, and nothing in the log
distinguishes "the tests failed" from "the tests never ran".

That is what happened on hao-ai-lab#1710:

    jq: error (at <stdin>:1): Cannot index string with string "env"
    Process completed with exit code 5

`curl` is called without `--fail-with-body`, so an HTTP error is treated as
success and its body is piped 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.

Three changes, all to the same step:

`continue-on-error: true`, because cancelling stale builds only saves agent
time. Failing to cancel wastes an agent; failing to trigger means untested
code, and step 7 keeps its hard failure.

`--fail-with-body` on the lookup, with the response echoed as a warning, so a
lookup that fails says why instead of feeding an error body to the parser.

`if type == "array" then .[] else empty end` in the filter, so a response that
is not a build list yields no matches rather than aborting.

Verified against six response shapes: a matching build, a build for another
PR, the error object from hao-ai-lab#1710, an empty array, a build with no env, and a
bare string. The first returns the build number and the rest return nothing,
where the error object previously exited 5.
@SolitaryThinker

Copy link
Copy Markdown
Collaborator

/merge

@SolitaryThinker

Copy link
Copy Markdown
Collaborator

/test full

@github-actions github-actions Bot added the ready PR is ready to merge label Aug 26, 2026
@SolitaryThinker SolitaryThinker added ready PR is ready to merge and removed ready PR is ready to merge labels Aug 26, 2026
@mergify mergify Bot added the scope: docs Documentation label Aug 26, 2026
@SolitaryThinker SolitaryThinker changed the title [ci]: a failed build cleanup should not be reported as a failed merge gate [ci]: make merge-gate startup reliable Aug 26, 2026
@SolitaryThinker

Copy link
Copy Markdown
Collaborator

/merge

@SolitaryThinker

Copy link
Copy Markdown
Collaborator

/test full

@github-actions github-actions Bot added ready PR is ready to merge and removed ready PR is ready to merge labels Aug 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready PR is ready to merge scope: docs Documentation scope: infra CI, tests, Docker, build type: ci CI/CD infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants