Uh oh!
There was an error while loading. Please reload this page.
Fix the nightly link check: tell a broken runner from a dead link, repair every dead link it found - #21694
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/21694
Note: Links to docs will display an error until the docs builds have been completed. ⏳ No Failures, 114 PendingAs of commit f4bd5e6 with merge base 9cd0c12 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
This PR needs a |
dbb9910 to
091bebfCompare091bebf to
8ca5333Compare8ca5333 to
6813118Compare6c4e82c to
70e4370Compare…pair every dead link it found
70e4370 to
f4bd5e6CompareUh oh!
There was an error while loading. Please reload this page.
The URL, xref, and file size linters diffed base..head, where base is the tip of the base branch rather than the point the branch left it. A branch cut before recent commits still carries the lines those commits replaced, so against the newer tip its old copies read as additions and the branch is blamed for links someone else already repaired. pytorch#21729 failed exactly that way, on eight links pytorch#21694 had already fixed or ignored. The workflow now resolves the merge base and passes it down. That is the half that matters for branches already open: on a pull request the reusable workflow resolves from the merge commit, so it carries this fix even though scripts/ still comes from the branch itself. The scripts switch to three dot ranges so running them by hand behaves the same way, and the checkouts fetch real history, without which no merge base exists to compute. Authored with Claude Code (Claude Opus 5).
The URL, xref, and file size linters diffed base..head, where base is the tip of the base branch rather than the point the branch left it. A branch cut before recent commits still carries the lines those commits replaced, so against the newer tip its old copies read as additions and the branch is blamed for links someone else already repaired. pytorch#21729 failed exactly that way, on nine links pytorch#21694 had already fixed or ignored. The workflow resolves the merge base and passes it down. That is the half that matters for branches already open: on a pull request the reusable workflow resolves from the merge commit, so it carries this fix even though scripts/ still comes from the branch itself. The scripts switch to three dot ranges so running them by hand behaves the same way. Computing a merge base needs history, so the pull request path checks out in full, restoring what pytorch#17682 traded away for speed at a measured 25s of wall clock. Pushes and the nightly whole tree scan cannot use a merge base and stay shallow. A missing merge base leaves the range unset for a whole tree scan rather than substituting the base tip, which two of the three scripts accept and then quietly pass. Authored with Claude Code (Claude Opus 5).
The URL, xref, and file size linters diffed base..head, where base is the tip of the base branch rather than the point the branch left it. A branch cut before recent commits still carries the lines those commits replaced, so against the newer tip its old copies read as additions and the branch is blamed for links someone else already repaired. pytorch#21729 failed exactly that way, on nine links pytorch#21694 had already fixed or ignored. The workflow resolves the merge base and passes it down. That is the half that matters for branches already open: on a pull request the reusable workflow resolves from the merge commit, so it carries this fix even though scripts/ still comes from the branch itself. The scripts switch to three dot ranges so running them by hand behaves the same way. Computing a merge base needs history, so the pull request path checks out in full, restoring what pytorch#17682 traded away for speed at a measured 25s of wall clock. Pushes and the nightly whole tree scan cannot use a merge base and stay shallow. A missing merge base leaves the range unset for a whole tree scan rather than substituting the base tip, which two of the three scripts accept and then quietly pass. Authored with Claude Code (Claude Opus 5).
…21765) ### Summary The URL, xref, and file-size linters diffed `base..head`, where `base` is the **tip** of the base branch rather than the point the branch left it. A branch cut before recent commits still carries the lines those commits replaced, so against the newer tip its old copies read as additions, and the branch gets blamed for links someone else already repaired. #21729 failed exactly this way. It touches 18 files and adds no URLs at all, but the two-dot diff scoped the lint to 143 files and flagged nine links that #21694 had already fixed or `@lint-ignore`d. #21707 is starker: one Python file with no URLs in it, 199 files linted, the same nine failures. ``` base..head 143 files <- what CI linted base...head 18 files <- what the PR actually changes ``` The stray `jq: parse error` lines in #21729's log are the same symptom from the other direction: the job runs the branch's own pre-#21694 copy of `lint_urls.sh`. ### Fix The workflow resolves the merge base and passes it down. That is the half that matters for branches already open: on a `pull_request` the reusable workflow resolves from the merge commit, so it carries this fix even though `scripts/` still comes from the branch itself. The scripts also switch to three-dot ranges so `./scripts/lint_urls.sh main HEAD` by hand behaves the same. `lint_xrefs.sh` and `lint_file_size.sh` had the identical bug and get the identical change. **Cost, stated plainly.** A merge base needs real history, so this restores `fetch-depth: 0` on the `pull_request` path — line for line what #17682 removed in February for speed. Measured on this branch, that is ~25s of added wall clock and ~79s of runner time across the three concurrent jobs. #17682's 6min → 10s was mostly the runner and Docker change rather than the fetch depth, though the commit changes both at once and I can't fully separate them. Pushes and the nightly whole-tree scan cannot use a merge base and stay shallow: ```yaml fetch-depth: ${{ github.event_name == 'pull_request' && '0' || '1' }} ``` The quotes matter — bare `0` is falsy in GitHub expressions, so `&& 0 || 1` always yields `1` and would silently disable the fix. **No silent fallback.** When there is no merge base there is no usable range, so the range is left unset and the linters scan the whole tree. Substituting the base tip instead produces a range the scripts fail on quietly: verified against unrelated histories, `lint_urls.sh` and `lint_xrefs.sh` both exit **0** having checked nothing, while `lint_file_size.sh` exits 128 and the wrapper reports "some files exceed the 1 MB limit", which is not what happened. Unset args are loud instead — verified rc=1 on a tree containing a dead link. **`--no-color`.** Both `git diff` calls now pass it, matching the `git grep --no-color` two lines below. With `color.ui = always` in a developer's config, added lines arrive wrapped in escape sequences, `grep -E '^\+'` matches nothing, and the check passes having found nothing (measured: 2 matches → 0). ### Test plan `.ci/scripts/tests/test_link_check_diff_selection.py` builds a diverged history where `main` repairs bad links and shrinks an oversized file while the feature branch simply predates all of it. Four fixture files each pin a different part, and `curl` is stubbed so there is no network: | Fixture | Pins | |---|---| | `both_sides.md` — edited on both branches | the per-file diff range | | `big.bin` — oversized at the branch point, shrunk on main | `lint_file_size.sh`'s range | | `colorful.gitconfig` — `color.ui = always` | `--no-color` | | `base_only.md` / `feature_only.md` | that main-only changes stay invisible and the branch's own additions are still checked | ``` pytest .ci/scripts/tests/test_link_check_diff_selection.py # 4 passed ``` Mutating each changed line individually: ``` inner per-file range -> .. 3 failed caught lint_file_size range -> .. 1 failed caught drop --no-color 1 failed caught file-selection range -> .. 4 passed equivalent mutant, see below ``` The file-selection range is not pinned because it cannot be: with the per-file diff at three dots, the extra files it selects produce empty diffs. Replayed against #21707's real 199-file range, both variants emit byte-identical output. It is changed for consistency, not behaviour. **Narrowing the scope must not blunt the check**, so each linter also has a positive control where the branch itself adds the bad thing: ``` lint_urls adds a dead URL -> rc=1, reports example.invalid/dead lint_xrefs adds a broken reference -> rc=1, reports sub/missing.md lint_file_size adds a 1MB+ file -> rc=1, reports feature_big.bin ``` End to end on real content: a synthetic commit on top of `main` that puts #21694's nine repaired links back, as if a PR had added them, gives `rc=1` with 9 FAIL and 5 OK against the live network. Incidentally `musl.cc` answered this time where CI saw `000`, and `pybind/cmake_example` now 404s where CI saw `301` — which is the retry-then-WARN path earning its keep. Also replayed #21729's and #21707's exact CI refs through the fixed scripts (exit 0 each), and ran all three linters plus `lintrunner` against this PR's own diff. ### Known limitations - The checkout is `head.sha`, so references still resolve against the branch tree rather than the merge result — the mirror image of the bug fixed here. Pre-existing and not worsened by this PR. - The whole-tree branch swallows a `git grep` failure and exits 0, so on a git built without PCRE the scan silently checks nothing. Reachable locally, not on the runners, which is why the nightly scan works. Pre-existing; worth a follow-up rather than widening this PR. ### Rollout This does **not** repair a currently red run. A rerun keeps the original `GITHUB_SHA`, and advancing the base alone does not fire `synchronize`, so an already-open PR picks the fix up only on its next newly triggered pull-request run — any push, or close/reopen. That is still cheaper than a content rebase: no history rewrite, no conflicts, nothing to re-review. Supersedes #21762, which fixes the same bug but leaves the checkout shallow and misses `lint_urls.sh`. Its reviewer's shallow-checkout point is exactly right: a `--depth=1` fetch writes a shallow graft even into an otherwise complete clone, after which `git merge-base` fails and `A...B` is fatal, so the two halves of this change are a pair. The regression test here is adapted from that PR. Authored with Claude Code (Claude Opus 5).
The nightly
link-check / lint-urlsjob has been red for weeks. There are two separate problems behind that, and this PR fixes both.Problem 1: the script cannot tell a dead host from a broken runner
scripts/lint_urls.shgets000back from curl when there is no HTTP response atall. That happens for two very different reasons: the host is dead, or the runner
has no working egress. The script treated both as
WARNand moved on.That is unsafe in both directions:
most want to catch.
.github/workflows/lint.ymlruns this same script in diff mode on every pull request, where the only URLs
checked are the ones that pull request adds. A new link with a typo in the host
name gives 000 and passes today. A runner with no egress makes every link WARN
and the whole job green. Both were reproduced locally.
The fix
On
000, probe one known-good URL,https://api.github.com, which the runneralready depends on:
000, so this runner has no egress:WARN, as before.the URL once with a 60 second timeout, then judge it on that result, which for a
dead host means
FAIL.One deliberate addition worth calling out: the retry with the longer timeout is
more than the minimum needed to close the hole. It is there because the same URLs
were sampled across six nights of nightly logs and a small number of them, one or
two per night out of about 2100, return
000once and then answer normally. Thedefault timeout is 10 seconds. Without the retry, this change would turn those
into hard failures and the nightly would stay red for no good reason.
Also removed: the check-host.net fallback
When the first request failed, the script asked
check-host.netwhether the URLwas reachable from elsewhere and could then overwrite the verdict. That is
removed. Note this makes the script stricter, not more lenient: on a 404 or a 500
the fallback could turn a
FAILinto anOK. Dropping it means a real 404 stays a404, and it also removes a third-party service from the critical path of a lint
job.
Problem 2: eight actually dead links
With problem 1 fixed the nightly would still be red, because there really are dead
links in the tree. The most recent nightly on main
(31350839957)
reported 7 x
FAIL 404and 1 xFAIL 000. All eight are handled here:setup.pyblob/master/setup.pysetup.py, so amasterlink cannot work.ci/docker/common/install_openssl.shblob/main/.ci/docker/common/install_openssl.shmain.ci/scripts/test_llava.shdocs/source/kernel-library-custom-aten-kernel.mdpytorch.org/cppdocs/library.html#...test/models/export_program.pypytorch.org/cppdocs/notes/tensor_indexing.htmldocs/source/success-stories.mdREADME.mdgithub.com/pytorch/executorch/stargazers@lint-ignorebackends/arm/scripts/toolchain_utils.shmusl.cctoolchain tarball@lint-ignoreTwo more URLs are cleaned up along the way: a truncated Qualcomm SDK URL in
backends/qualcomm/scripts/download_qnn_sdk.pythat was never a real link, and aQwen issue link in
examples/models/llama/runner/generation.pypointing at theold repository name.
On the Wikimedia one specifically:
examples/models/llava/README.mdalreadyembeds the renamed photo and documents the caption the model produces for it,
which matches the
EXPECTED_PREFIXthe script asserts. So this restores theoriginal input rather than substituting a different picture. Worth knowing, and
the reason this is low risk: no workflow runs
.ci/scripts/test_llava.sh. Bothpull.ymlandtrunk.ymlcarry the comment "llava gives segfault so notcovering", so llava is not in CI at all right now.
How this was verified
A whole-tree scan is nightly-only, so a pull request run here would only check the
handful of URLs this PR adds. To get real numbers,
_link_check.ymlwas dispatchedin whole-tree mode against this branch's contents
(31417702080):
Reconciling the two totals, since they should not match exactly:
and 2 now marked
@lint-ignore.https://api.github.com, which is nowwritten in
lint_urls.shas the probe target and so gets scanned itself.during the day, in
examples/models/muse-glimmer/, and are unrelated to thischange. All 4 pass.
The 7 extra WARNs are the same thing that produces the existing 24: sites that
return
403to an automated client. Between the two runs,cppreference.com,stackoverflow.comandvulkan.orghappened to answer403instead of200.Those are already treated as warnings and are not affected by this PR.
The 000 handling itself was also exercised directly, in a scratch repository:
WARN 000, exit 0, matching a broken runner.FAIL 000, exit 1.OK 200,WARN 403andFAIL 404all still behave as before.