fix: give a fetch that reaches the network more than one go - #469
Merged
Conversation
A bake fetches third-party extensions and skins over the network, and the image build downloads three tarballs, and every one of those gave up on its first refusal. That is a build failed by somebody else having a bad minute rather than by anything in the source, and today it happened twice: GitHub answered 500 for git and archive traffic for hours, which killed a local bake mid-clone, and codeload answered 429, which killed the phan job on #466 and stopped an unrelated composer install here six times running. Neither says anything about the commit being built, so neither should end a build on the first try. ## The bake `Attempts` holds the policy: three tries, waiting 2s then 4s. Doubling rather than a flat wait because the two failures worth retrying want opposite things -- a 500 wants a moment, a 429 wants to be asked less often, and asking again at the same rate is what keeps it a 429. Only the fetching retries. `git init`, the checkout and the extraction work on what is already local, so one failure from those is a real one and still ends the build at once. `git clone` refuses a destination with anything in it and a clone that died partway leaves one, so the clone path clears its leftovers between attempts. Verified that this is what happens, rather than assumed: all three attempts fail with git's own "could not read Username", never with "destination path already exists", which is the error the second attempt would give if the first one's debris were still there. Loaded with a `require_once` rather than the autoloader, because there is no autoloader yet. This script runs before MediaWiki loads the extensions, that being what it is for, which is the same reason `loadConfig()` already loads `SiteConfig` directly. Found by running it: the first version of this crashed with "Class Attempts not found" on every bake that declares a repository. Measured against a repository that does not exist, both shapes: ``` Wikven: failed to clone extension 'Nowhere' (exit 128); trying again in 2s (attempt 2 of 3) Wikven: failed to clone extension 'Nowhere' (exit 128); trying again in 4s (attempt 3 of 3) Wikven: failed to clone extension 'Nowhere' (exit 128). ``` 9s where it used to be 1s, and still non-zero at the end. A failure that is going to fail is only slower; a blip now passes. ## The image `curl --retry 5 --retry-delay 2 --retry-all-errors`, in an ARG so the three fetches share one policy. `--retry-all-errors` as well as `--retry` because curl files a connection reset under non-transient and would not repeat it otherwise. Confirmed in the image: the same doomed request takes 2s bare and 13s with the flags, so the attempts are real, and its curl is 8.21.0, well past the 7.71 that added the flag. The SifterSearch tarball is downloaded before it is extracted now instead of piped into tar. That is not tidying: a retried transfer starts over, and tar on the far end of the pipe has already been fed the first attempt's bytes. The block below it already avoided piping, and says why. `binary.Dockerfile` is left alone. Its fetching is inside `build-static.sh`, which the FrankenPHP image owns, and wrapping an eight-minute build in a retry loop is a different question from this one -- #433 died that way, on dl.static-php.dev answering 500, and is worth its own look. ## Not verified here `AttemptsTest` has not been run: MediaWiki's dev dependencies would not install on this machine, because codeload kept answering 429, which is the failure this commit is about. The class's behaviour was checked instead by exercising it directly against the same assertions the test makes. CI runs the test itself. Refs #461. --- _Generated by [Claude Code](https://claude.ai/code/session_935f02d1)_ Co-authored-by: Claude <noreply@anthropic.com>
Attempts arrived with its own loop next to the one RetryingForeignRepo has had, and the two disagreed about the thing they share. The repository handed its pause callback a number of seconds and waited 1s then 2s; Attempts hands its callback the number of the attempt to come and waits 2s then 4s. The same "three attempts" meant three seconds of waiting in one place and six in the other, and the schedule was written down twice. The reason given for not sharing was that the repository repeats a request answering with a body or with false, while the fetching repeats work that either worked or did not. That is one contract, not two, once until() answers with what the attempt answered instead of a bool: a body comes back to the caller that wants a body, and true comes back to the caller that only wants to know. False is the failure and only false -- a request can succeed with an empty body, which a truthiness test would have retried and then reported as a failure. RetryingForeignRepoTest covered exactly that, and the case moves to AttemptsTest with the loop it tests. Nothing off the shelf does this, which the class comment now records rather than leaving the next reader to check: MWHttpRequest and HttpRequestFactory carry no retry, wait-condition-loop spends a time budget waiting for a condition rather than repeating one fetch that can take minutes, and Guzzle's retry middleware would reach the HTTP caller but not the one that repeats a composer update in a subprocess. Commons lookups now back off 2s and 4s where they backed off 1s and 2s. That is the schedule the fetching already used, and a repository answering 429 is the case for asking less often rather than more. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uv1RzRurUH6wrgV5E9PESQ
mago format keeps an empty function body on the line that opens it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uv1RzRurUH6wrgV5E9PESQ
mago collapses an empty function body onto the line that opens it; phpcs wants a closing brace alone on its line. The empty closure was the only place the two disagreed, so it stops being empty: the callback records the attempts it was asked to wait for, like every other test here, and the test asserts there were none. Both tools accept a body with a statement in it, and the assertion says out loud what the empty closure only implied -- an answer that came back on the first attempt was not waited on or tried again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uv1RzRurUH6wrgV5E9PESQ
lens0021
added a commit
that referenced
this pull request
Aug 19, 2026
…470) > Stacked on #469. Review the top commit; the base merges first. The reproducibility check bakes the docs site a second time and diffs it, and the two bakes were 209s of the smoke job's 321s. They are independent by construction, so they can run together. The reason it pays is that a bake is mostly one process. Only the skin passes run beside each other, and even those are three on a four-core runner, so a second bake fills cores the first leaves idle rather than taking any from it. ## Measured Two containers pinned to four cores, the runner's count, on an image built from this PR's base: | | wall clock | |---|---| | one bake alone | 118s | | two, one after the other | 257s | | **two at once, four cores between them** | **150s** | | two at once, two cores each | 166s | 150s is what landed: no cpu flags, both bakes left to size themselves as they do today. Capping each at two cores is slower, because that also halves the concurrency of the part which does parallelise. An earlier run of this benchmark gave the concurrent case a spurious win by capping each container at four cores on a fourteen-core host. That is eight cores between them, and not a runner at all. Discarded. Peak combined resident memory was 1 GiB against the runner's 16, with no OOM in either log. That was the risk worth measuring: two bakes at three skin passes each is six MediaWiki boots at once. Output is byte-identical in every configuration above, which is the check itself agreeing. ## The step's own shell Each bake writes to its own log, because two live logs interleave into neither, and both are printed afterwards in collapsed groups. Verified that a failure still fails the step: a bake pointed at a source directory that does not exist ends it non-zero, and the success path ends it zero. Ran the step's shell verbatim, not a paraphrase of it. The source mount is read-only now that two containers share it, so neither can alter what the other is reading. Only `wikven translate` writes into src, and this is not that. Refs #461. ## On the nine minutes in the issue Worth recording, since it changes what is left to do. #461 measured 535s wall clock on #458, dominated by `binary` at 467s. But `binary` is path-filtered and does not run on a pull request that only touches PHP. On #466, which did exactly that, all 21 checks started within two seconds of each other and `smoke` at 317s was the longest: ``` 317s smoke 186s coverage 145s phan (master) 52s phpunit (REL1_46) 43s docker-image ``` So a PHP-only pull request already goes green in about five and a half minutes, and `smoke` is the whole of its critical path. The nine minutes belongs to image-touching pull requests, where `binary` is the long pole and a separate question. This PR and #471 take `smoke` after the two of them from 321s to roughly 190s. Co-authored-by: Claude <noreply@anthropic.com>
lens0021
added a commit
that referenced
this pull request
Aug 19, 2026
) > Stacked on #470, which is stacked on #469. Review the top commit; the bases merge first. `playwright install chromium` fetches three things: Chrome for Testing (379 MiB unpacked), the headless shell (262 MiB) and ffmpeg (5 MiB). These specs run headless and record no video, so the shell is the one Playwright starts and the browser beside it is downloaded to be ignored. Asking for the shell by name skips it. ffmpeg arrives either way, so what this saves is the 379 MiB, about three fifths of the bytes in a 33s step. ## Checked, not reasoned Installed the shell alone into a browser directory of its own, pointed `PLAYWRIGHT_BROWSERS_PATH` at it so Playwright could find nothing else, and ran the suite against a real bake: ``` 57 passed (14.6s) ``` `--with-deps` stays. What it installs is system libraries, which the shell needs as much as the full browser. If the shell were ever not enough -- a spec asking for a headed browser, or for video -- Playwright fails naming the executable it cannot find, so this cannot quietly test the wrong thing. ## Why not cache the browsers, as #461 suggested zizmor refuses it. `actions/cache` in a job that also runs `docker/build-push-action` is its cache-poisoning audit, high severity: ``` error[cache-poisoning]: runtime artifacts potentially vulnerable to a cache poisoning attack 53 | uses: docker/build-push-action@... runtime artifacts usually published here 246 | - uses: actions/cache@... enables caching by default ``` Tried it first and read that, which is the same objection the comment two lines below this change already records about setup-node's caching. Fetching less needs no exception. Refs #461. --------- Co-authored-by: Claude <noreply@anthropic.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.
A bake fetches third-party extensions and skins over the network, and the image build downloads three tarballs, and every one of those gave up on its first refusal. That is a build failed by somebody else having a bad minute rather than by anything in the source.
It happened twice while this was being written. GitHub answered 500 for git and archive traffic for hours on 2026-08-17, which killed a local bake mid-clone:
and codeload answered 429, which killed the phan job on #466 and stopped an unrelated composer install here six times running. Neither says anything about the commit being built.
The bake
Attemptsholds the policy: three tries, waiting 2s then 4s. Doubling rather than a flat wait because the two failures worth retrying want opposite things. A 500 wants a moment; a 429 wants to be asked less often, and asking again at the same rate is what keeps it a 429.Only the fetching retries.
git init, the checkout and the extraction work on what is already local, so one failure from those is real and still ends the build at once.Measured against a repository that does not exist, both shapes of git fetch:
9s where it was 1s, still non-zero at the end. A failure that is going to fail is only slower; a blip now passes.
Two things worth calling out
git clonewill not run twice over its own leftovers. It refuses a destination with anything in it, and a clone that died partway leaves one, so the clone path clears the debris between attempts. Verified rather than assumed: all three attempts fail with git's owncould not read Username, never withdestination path already exists, which is the error the second attempt would give if the first one's leftovers were still there.There is no autoloader this early.
fetchExtensions.phpruns before MediaWiki loads the extensions, that being what it is for, which is whyloadConfig()alreadyrequire_oncesSiteConfigdirectly. The first version of this crashed withClass "MediaWiki\Extension\Wikven\Attempts" not foundon every bake that declares a repository. Found by running it, not by reading it.One loop, not two
RetryingForeignRepohas had a retry loop of its own since it was written, and the two disagreed about the thing they share. It handed its pause callback a number of seconds and waited 1s then 2s;Attemptshands its callback the number of the attempt to come and waits 2s then 4s. The same "three attempts" meant three seconds of waiting in one place and six in the other, and the schedule was written down twice.The difference that seemed to justify two loops — one repeats a request answering with a body or with false, the other repeats work that either worked or did not — is one contract, not two, once
until()answers with what the attempt answered instead of a bool. A body comes back to the caller that wants a body;truecomes back to the caller that only wants to know.False is the failure and only false. A request can succeed with an empty body, which a truthiness test would have retried and then reported as a failure.
RetryingForeignRepoTestcovered exactly that case, and it moves toAttemptsTestalong with the loop it tests; that file held nothing else, so it goes.Commons lookups now back off 2s and 4s where they backed off 1s and 2s. That is the schedule the fetching already used, and a repository answering 429 is the case for asking less often rather than more.
Nothing off the shelf does this
Recorded in the class comment so the next reader does not have to check again:
HttpRequestFactoryandMWHttpRequestcontain no retry code at all.wikimedia/wait-condition-loop, which core bundles, spends a time budget waiting for a condition to come true. One fetch here is acomposer updatethat can take minutes, which is not that shape.HttpRequestFactorydoes not expose the handler stack, so using it means building a client by hand and re-doing core's proxy, CA and user-agent setup. It would not reach the other caller at all, which repeats a subprocess.The image
curl --retry 5 --retry-delay 2 --retry-all-errors, in an ARG so the three fetches share one policy.--retry-all-errorsas well as--retry, because curl files a connection reset under non-transient and would not repeat it otherwise.Confirmed in the image: the same doomed request takes 2s bare and 13s with the flags, so the attempts are real. Its curl is 8.21.0, well past the 7.71 that added the flag.
The SifterSearch tarball is downloaded before it is extracted now, instead of piped into tar. That is not tidying. A retried transfer starts over, and tar on the far end of the pipe has already been fed the first attempt's bytes. The block below it already avoids piping and says why.
Left alone
binary.Dockerfile. Its fetching happens insidebuild-static.sh, which the FrankenPHP image owns, and wrapping an eight-minute build in a retry loop is a different question. #433 died that way, ondl.static-php.devanswering 500, and deserves its own look.The tests
Attempts::untilnever sleeps: the pause is the caller's hook, so the tests record which attempts were waited for rather than spending the seconds. That is the seamRetryingForeignRepokept for the same reason, and it is why the suite covering a backoff policy costs nothing to run.AttemptsTestcovers the loop from both sides — a bool caller and a value caller — including the empty answer that must not count as a failure, the wait that arrives before each retry and not before the first, and the caller who asks for no attempts and still gets one.phpunit (REL1_46),phpunit (master)andcoverageall run it green on this branch.Refs #461.