ci(pr): the changes job survives an un-renderable diff and no longer fails open on large file lists - #2269
Merged
Conversation
…422s
The `changes` job decides whether a PR touches product code, and ci-ok
requires it. It asks GitHub for the PR's file list. An earlier fix moved
it off the .diff endpoint, which rejects PRs over 20k lines, onto the
paginated pulls/files endpoint -- but that endpoint renders the diff
server-side as well, and for a PR that regenerates a vendored parser it
answers
HTTP 422: Sorry, this diff is taking too long to generate.
Observed on #2246 (a 42 MB sql/parser.c): `changes` red, therefore ci-ok
red, for a reason that has nothing to do with the contribution. The
compare endpoint fails the same way, so there is no API route to the
file list for such a PR. Every grammar refresh would hit this.
When the API call fails, the job now takes the list from git instead.
The PR merge ref's first parent is the base, so a name-only diff across
that one merge commit is exactly the PR's changed files. The fetch is
depth 2 with --filter=blob:none into a bare repository under RUNNER_TEMP:
commits and trees only, no blobs, nothing checked out and nothing from
the PR executed. No new action, no new permission, and the API path and
the classification regex are untouched, so ordinary PRs behave as before.
A ::notice:: line records when the fallback was used. If the fetch fails
too, the step exits non-zero and writes no output -- the gate fails
closed, as it did before.
Proof, by executing the workflow's own run block under
`bash -eo pipefail` with a stub gh:
API path, docs-only list rc 0 product=false
fallback, real #2246 rc 0 product=true, the same five
files `git diff --numstat`
reports for the PR; 0.85 s,
188 KB fetched
fallback, nonexistent merge ref rc 128 no output written
Contract tests that read pr.yml -- security_gate_fail_closed,
smoke_fixture_contract, windows_bundle_contract, venue_parity_contract --
pass.
Not addressed here, noted for a follow-up: the classification pipes the
list into `grep -q` under pipefail, which can report a match as a
failure once the list outgrows the pipe buffer.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Found while proving the previous commit, in the same step, and the worse
of the two defects because it fails OPEN. The classification was
if printf '%s\n' "$FILES" | grep -qE '^(src/|internal/|...)'; then
under the runner's `bash -eo pipefail`. grep -q exits on its first match;
once the list is larger than the pipe buffer printf is still writing,
dies of SIGPIPE, and pipefail turns the MATCH into a failed pipeline. The
else branch then writes product=false, and pr-smoke and memwaste are
skipped -- on exactly the PRs that change the most files. The previous
commit's message flagged this as a follow-up; it is fixed here instead.
The list now reaches grep through a here-string, so there is no pipe to
break. The regex is unchanged.
Proof, by executing the workflow's own run block under
`bash -eo pipefail` with a stub gh, ten runs each:
849 KB list, first line src/a.c before: product=false 10/10
after: product=true 10/10
849 KB list, docs only product=false before and after
small docs-only list product=false
422 fallback on the real #2246 product=true
fallback, nonexistent merge ref rc 128, no output written
It takes roughly 1,500 changed files to cross the buffer, which is why
ordinary PRs never showed it.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two defects in the
changesjob ofpr.yml, both hitting exactly the PRs that change the most. One fails closed for the wrong reason, the other fails open.1. The file-list API answers 422 for a PR that regenerates a vendored parser
changesdecides whether a PR touches product code, andci-okrequires it. It askspulls/{n}/filesfor the file list. That endpoint renders the diff server-side, and for #2246 (a 42 MBsql/parser.c) it answersThe compare endpoint fails the same way, so there is no API route to the list for such a PR. Result:
changesred, thereforeci-okred, for a reason that has nothing to do with the contribution. Every grammar refresh would hit this.Fix: when the API call fails, take the list from git. The PR merge ref's first parent is the base, so a name-only diff across that one merge commit is the PR's file list. The fetch is
--depth=2 --filter=blob:noneinto a bare repository underRUNNER_TEMP: commits and trees only, nothing checked out, nothing from the PR executed. No new action, no new permission; the API path and the classification regex are untouched, so ordinary PRs behave exactly as before. A::notice::records when the fallback was used. If the fetch fails too, the step exits non-zero and writes no output — the gate still fails closed.2. The classification reported a MATCH as "no product change" on large file lists
runs under the runner's
bash -eo pipefail.grep -qexits on its first match; once the list is larger than the pipe buffer,printfis still writing, dies of SIGPIPE, andpipefailturns the match into a failed pipeline. Theelsebranch then writesproduct=false, andpr-smokeandmemwasteare skipped — on the largest PRs. It takes roughly 1,500 changed files to cross the buffer, which is why ordinary PRs never showed it.Fix: the list reaches
grepthrough a here-string, so there is no pipe to break. The regex is unchanged.Proof
By executing the workflow's own
run:block underbash -eo pipefailwith a stubgh, ten runs each where it matters:src/a.cproduct=false10/10product=true10/10product=falseproduct=falseproduct=falseproduct=falseproduct=true, the same five filesgit diff --numstatreports; 0.85 s, 188 KB fetchedThe contract tests that read
pr.yml—security_gate_fail_closed,smoke_fixture_contract,windows_bundle_contract,venue_parity_contract— pass on this branch.Scope
One file,
.github/workflows/pr.yml, one step of one job: +22/−2. No trigger, permission, action or secret changes.