diff --git a/Dockerfile b/Dockerfile index 1d5c0a640..5a7cbafae 100644 --- a/Dockerfile +++ b/Dockerfile @@ -20,14 +20,27 @@ RUN apk add --no-cache rsvg-convert imagemagick-jpeg imagemagick-webp # Bundled extensions come from stable external sources; fetch them before copying wikven's own # code so edits to that code do not bust the (slow) download/clone layers. +# Every fetch below asks curl to try again, because "stable source" describes the bytes and not the +# service in front of them: GitHub served 500s for archive and git traffic for hours on 2026-08-17, +# and codeload has answered 429 under load. One attempt turns somebody else's bad minute into a +# failed build with nothing wrong in it. --retry-all-errors is needed as well as --retry because +# curl counts a connection reset as a non-transient error and would otherwise not repeat it. +ARG CURL_RETRY="--retry 5 --retry-delay 2 --retry-all-errors" + # SifterSearch (client-side Pagefind search) ships built in. Its release tarball carries the # per-arch Pagefind binary a git clone omits, so fetch the one matching this build's architecture. +# +# Downloaded before extracting rather than piped, for the reason the block below already gives: a +# retried transfer restarts, and tar reading a restarted stream has already been fed the first +# attempt's bytes. ARG TARGETARCH ARG SIFTERSEARCH_VERSION=v0.8.0 RUN arch="$TARGETARCH" \ && if [ "$arch" = amd64 ]; then arch=x64; fi \ - && curl -fsSL "https://github.com/chaotic-ground/SifterSearch/releases/download/${SIFTERSEARCH_VERSION}/SifterSearch-linux-${arch}.tar.gz" \ - | tar -xz -C /var/www/html/extensions/ + && curl -fsSL $CURL_RETRY -o /tmp/siftersearch.tar.gz \ + "https://github.com/chaotic-ground/SifterSearch/releases/download/${SIFTERSEARCH_VERSION}/SifterSearch-linux-${arch}.tar.gz" \ + && tar -xzf /tmp/siftersearch.tar.gz -C /var/www/html/extensions/ \ + && rm /tmp/siftersearch.tar.gz # Content i18n (opt-in via WikvenI18nLanguages): Translate renders translated pages and the # bar; UniversalLanguageSelector is its hard load-time dependency. Both track this @@ -55,9 +68,9 @@ ARG COMPOSER_INSTALLERS_VERSION=v2.3.0 # matching what the ARGs pin, so a new upstream dependency cannot slip in unpinned. RUN composer config --global policy.advisories.block false \ && ext=/var/www/html/extensions \ - && curl -fsSL -o /tmp/uls.tar.gz \ + && curl -fsSL $CURL_RETRY -o /tmp/uls.tar.gz \ "https://codeload.github.com/wikimedia/mediawiki-extensions-UniversalLanguageSelector/tar.gz/$ULS_VERSION" \ - && curl -fsSL -o /tmp/translate.tar.gz \ + && curl -fsSL $CURL_RETRY -o /tmp/translate.tar.gz \ "https://codeload.github.com/wikimedia/mediawiki-extensions-Translate/tar.gz/$TRANSLATE_VERSION" \ && mkdir -p "$ext/UniversalLanguageSelector" "$ext/Translate" \ && tar -xzf /tmp/uls.tar.gz --strip-components=1 -C "$ext/UniversalLanguageSelector" \ diff --git a/includes/Attempts.php b/includes/Attempts.php new file mode 100644 index 000000000..844d5bb79 --- /dev/null +++ b/includes/Attempts.php @@ -0,0 +1,69 @@ + 1) { + $before($attempt); + } + $answer = $work(); + if ($answer !== false) { + break; + } + } + return $answer; + } + + /** + * Seconds to wait before attempt number $attempt: none before the first, then 2, 4, 8... + * + * Doubling rather than a fixed wait because the two failures worth retrying differ in what they + * are asking for. A 500 wants a moment; a 429 wants to be asked less often, and answering it at + * the same rate is the thing that keeps it a 429. + */ + public static function backoff(int $attempt): int { + return $attempt <= 1 ? 0 : 1 << ( $attempt - 1 ); + } + + /** Wait out the backoff before attempt number $attempt. The $before most callers want. */ + public static function sleep(int $attempt): void { + sleep(self::backoff($attempt)); + } +} diff --git a/includes/RetryingForeignRepo.php b/includes/RetryingForeignRepo.php index 82a091853..fe978ba34 100644 --- a/includes/RetryingForeignRepo.php +++ b/includes/RetryingForeignRepo.php @@ -20,9 +20,6 @@ * really cannot be had, the build says which one and why instead of quoting that riddle. */ class RetryingForeignRepo extends ForeignAPIRepo { - /** How many times one request is made before the repository reports it as failed. */ - private const ATTEMPTS = 3; - /** * @inheritDoc * @@ -43,7 +40,7 @@ public function getThumbUrlFromCache($name, $width, $height, $params = '') { } $size = $height > 0 ? "{$width}x{$height}" : "{$width}px"; - $attempts = self::ATTEMPTS; + $attempts = Attempts::FETCH; $what = "Wikven: the '{$this->getName()}' repository has no thumbnail URL for \"$name\" at $size"; $tries = "after $attempts attempt(s)."; $why = 'The build cannot make that image local, and would publish a page missing it.'; @@ -51,36 +48,19 @@ public function getThumbUrlFromCache($name, $width, $height, $params = '') { throw new RuntimeException("$what $tries $why $fix"); } - /** @inheritDoc */ + /** + * @inheritDoc + * + * Answering with a body or with false is what Attempts::until reads as worked or did not, so + * the loop is the one the fetching side of the build uses, and the waits are the same waits. + */ public function httpGet($url, $timeout = 'default', $options = [], &$mtime = false) { - return self::retry( + return Attempts::until( function () use ($url, $timeout, $options, &$mtime) { return parent::httpGet($url, $timeout, $options, $mtime); }, - self::ATTEMPTS, - 'sleep' + Attempts::FETCH, + [Attempts::class, 'sleep'] ); } - - /** - * Run $request until it answers, at most $attempts times, waiting longer before each retry. - * - * @param callable():(string|false) $request Returns the response body, or false if it failed. - * @param int $attempts How many times $request is run before its failure is passed on. - * @param callable(int):mixed $pause Given the seconds to wait before the next attempt (1, then 2, ...). - * @return string|false The first body $request returned, or false if every attempt failed. - */ - public static function retry(callable $request, int $attempts, callable $pause) { - $body = false; - for ($attempt = 1; $attempt <= $attempts; $attempt++) { - if ($attempt > 1) { - $pause($attempt - 1); - } - $body = $request(); - if ($body !== false) { - break; - } - } - return $body; - } } diff --git a/maintenance/fetchExtensions.php b/maintenance/fetchExtensions.php index 5c25d55e3..8bbd46e6d 100644 --- a/maintenance/fetchExtensions.php +++ b/maintenance/fetchExtensions.php @@ -2,10 +2,13 @@ namespace MediaWiki\Extension\Wikven; +use FilesystemIterator; use Maintenance; use MediaWiki\MediaWikiServices; use MediaWiki\Settings\Source\Format\JsonFormat; use MediaWiki\Settings\Source\Format\YamlFormat; +use RecursiveDirectoryIterator; +use RecursiveIteratorIterator; $IP = strval(getenv('MW_INSTALL_PATH')) !== '' ? getenv('MW_INSTALL_PATH') @@ -13,6 +16,11 @@ require_once "$IP/maintenance/Maintenance.php"; +// Wikven's autoloader is not active this early: making the extensions MediaWiki will load present is +// what this script is for, so it runs before that. Loaded directly for the same reason loadConfig() +// loads SiteConfig directly below, and the class is dependency-free so that is all it takes. +require_once __DIR__ . '/../includes/Attempts.php'; + /** Fetch third-party extensions/skins declared in .wikven.yaml before MediaWiki loads them. */ class FetchExtensions extends Maintenance { public function __construct() { @@ -204,7 +212,8 @@ private function fetchGit(array $spec, string $dest, string $name, string $kind) $this->output("Wikven: cloning $kind '$name' from $repo @ $commit\n"); $this->run(['git', 'init', '--quiet', '--', $dest], "init $kind '$name'"); $this->run(['git', '-C', $dest, 'remote', 'add', 'origin', $repo], "configure $kind '$name'"); - $this->run( + // No reset: a fetch that failed leaves a repository another fetch is happy to reuse. + $this->runOnNetwork( ['git', '-C', $dest, 'fetch', '--depth', '1', 'origin', $commit], "fetch $kind '$name' @ $commit" ); @@ -219,11 +228,15 @@ private function fetchGit(array $spec, string $dest, string $name, string $kind) array_push($cmd, '--', $repo, $dest); $ref = !empty($spec['reference']) ? " @ {$spec['reference']}" : ''; $this->output("Wikven: cloning $kind '$name' from $repo$ref\n"); - $this->run($cmd, "clone $kind '$name'"); + // git clone refuses a destination that already has anything in it, and a clone that died + // partway leaves one, so the leftovers go before the next attempt. + $this->runOnNetwork($cmd, "clone $kind '$name'", static function () use ($dest) { + self::removeTree($dest); + }); } if (!empty($spec['composer'])) { - $this->run( + $this->runOnNetwork( ['composer', 'update', '--no-dev', '--no-interaction', '--working-dir=' . $dest], "composer install for $kind '$name'" ); @@ -242,7 +255,7 @@ private function installPackages(string $IP, array $packages): void { json_encode($local, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES) . "\n" ); - $this->run( + $this->runOnNetwork( ['composer', 'update', '--no-dev', '--no-interaction', '--working-dir=' . $IP], 'composer update' ); @@ -251,20 +264,39 @@ private function installPackages(string $IP, array $packages): void { /** Download $url to $dest, streaming the body to disk instead of buffering it in memory. */ private function download(string $url, string $dest, string $what): void { $http = MediaWikiServices::getInstance()->getHttpRequestFactory(); - $req = $http->create($url, ['followRedirects' => true], __METHOD__); - - $fh = fopen($dest, 'wb'); - if ($fh === false) { - $this->fatalError("Wikven: could not open '$dest' for writing."); - } - $req->setCallback(static function ($resource, $buffer) use ($fh) { - return fwrite($fh, $buffer); - }); - $status = $req->execute(); - fclose($fh); - - $httpStatus = $req->getStatus(); - if (!$status->isOK() || $httpStatus < 200 || $httpStatus >= 300) { + // Read out here: __METHOD__ inside the closure would name the closure, not this. + $caller = __METHOD__; + $httpStatus = 0; + $ok = Attempts::until( + function () use ($http, $url, $dest, $caller, &$httpStatus) { + $req = $http->create($url, ['followRedirects' => true], $caller); + // Opened per attempt, and truncating, so a retry replaces a partial body rather + // than appending a second one to it. + $fh = fopen($dest, 'wb'); + if ($fh === false) { + $this->fatalError("Wikven: could not open '$dest' for writing."); + } + $req->setCallback(static function ($resource, $buffer) use ($fh) { + return fwrite($fh, $buffer); + }); + $status = $req->execute(); + fclose($fh); + $httpStatus = $req->getStatus(); + return $status->isOK() && $httpStatus >= 200 && $httpStatus < 300; + }, + Attempts::FETCH, + function (int $attempt) use ($what, &$httpStatus) { + $wait = Attempts::backoff($attempt); + $this->output( + "Wikven: failed to $what (HTTP $httpStatus); trying again in {$wait}s" + . " (attempt $attempt of " + . Attempts::FETCH + . ")\n" + ); + sleep($wait); + } + ); + if (!$ok) { unlink($dest); $this->fatalError("Wikven: failed to $what (HTTP $httpStatus)."); } @@ -283,6 +315,61 @@ private function run(array $cmd, string $what): void { $this->fatalError("Wikven: failed to $what (exit $ret)."); } } + + /** + * Run a command that reaches the network, giving it more than one go before the build fails. + * + * Used for the fetching alone. The commands around it -- git init, a checkout, an extraction -- + * work on what is already local, so one failure from those is a real one; see Attempts for why a + * fetch is different. + * + * @param string[] $cmd + * @param string $what What the command is doing, for the reports and the error. + * @param callable():void|null $reset Clear a half-finished attempt away, for a command that will + * not run twice over its own leftovers. + */ + private function runOnNetwork(array $cmd, string $what, ?callable $reset = null): void { + $shell = implode(' ', array_map('escapeshellarg', $cmd)); + $ret = 0; + $ok = Attempts::until( + static function () use ($shell, &$ret) { + passthru($shell, $ret); + return $ret === 0; + }, + Attempts::FETCH, + function (int $attempt) use ($what, $reset, &$ret) { + $wait = Attempts::backoff($attempt); + $this->output( + "Wikven: failed to $what (exit $ret); trying again in {$wait}s" + . " (attempt $attempt of " + . Attempts::FETCH + . ")\n" + ); + if ($reset !== null) { + $reset(); + } + sleep($wait); + } + ); + if (!$ok) { + $this->fatalError("Wikven: failed to $what (exit $ret)."); + } + } + + /** Remove a directory and everything under it, so a failed fetch can be tried again. */ + private static function removeTree(string $dir): void { + if (!is_dir($dir)) { + return; + } + $entries = new RecursiveIteratorIterator( + new RecursiveDirectoryIterator($dir, FilesystemIterator::SKIP_DOTS), + RecursiveIteratorIterator::CHILD_FIRST + ); + foreach ($entries as $entry) { + $entry->isDir() ? rmdir($entry->getPathname()) : unlink($entry->getPathname()); + } + rmdir($dir); + } } $maintClass = FetchExtensions::class; diff --git a/tests/phpunit/unit/AttemptsTest.php b/tests/phpunit/unit/AttemptsTest.php new file mode 100644 index 000000000..0c7e78e73 --- /dev/null +++ b/tests/phpunit/unit/AttemptsTest.php @@ -0,0 +1,152 @@ + $failures; + }; + } + + public function testWorkThatSucceedsIsRunOnceAndNotWaitedFor() { + $calls = []; + $waits = []; + $ok = Attempts::until( + $this->failing(0, $calls), + 3, + static function (int $attempt) use (&$waits) { + $waits[] = $attempt; + } + ); + $this->assertTrue($ok); + $this->assertCount(1, $calls); + $this->assertSame([], $waits, 'nothing to wait for before a first attempt'); + } + + public function testItKeepsTryingUntilSomethingWorks() { + $calls = []; + $waits = []; + $ok = Attempts::until( + $this->failing(2, $calls), + 3, + static function (int $attempt) use (&$waits) { + $waits[] = $attempt; + } + ); + $this->assertTrue($ok); + $this->assertCount(3, $calls); + $this->assertSame([2, 3], $waits, 'once before each retry, numbered by the attempt to come'); + } + + public function testItGivesUpAfterTheLastAttempt() { + $calls = []; + $waits = []; + $ok = Attempts::until( + $this->failing(PHP_INT_MAX, $calls), + 3, + static function (int $attempt) use (&$waits) { + $waits[] = $attempt; + } + ); + $this->assertFalse($ok); + $this->assertCount(3, $calls, 'three attempts, not a fourth'); + $this->assertSame([2, 3], $waits, 'and no wait after the one that failed last'); + } + + /** Work answering with each of $answers in turn, recording the calls it received. */ + private function answering(array $answers, array &$calls): callable { + return static function () use ($answers, &$calls) { + $calls[] = count($calls) + 1; + return $answers[count($calls) - 1]; + }; + } + + public function testWhatTheAttemptAnsweredIsWhatComesBack() { + $calls = []; + $waits = []; + $body = Attempts::until( + $this->answering(['{"query":{}}'], $calls), + 3, + static function (int $attempt) use (&$waits) { + $waits[] = $attempt; + } + ); + $this->assertSame('{"query":{}}', $body, 'the caller that wants a body gets the body'); + $this->assertCount(1, $calls); + $this->assertSame([], $waits, 'work that answered must not be waited on'); + } + + public function testAnEmptyAnswerIsNotMistakenForAFailure() { + $calls = []; + $waits = []; + $body = Attempts::until( + $this->answering(['', 'unreached'], $calls), + 3, + static function (int $attempt) use (&$waits) { + $waits[] = $attempt; + } + ); + $this->assertSame('', $body, 'false is the failure, and only false'); + $this->assertCount(1, $calls); + $this->assertSame([], $waits, 'so nothing was waited on and nothing tried again'); + } + + public function testTheBodyOfTheFirstAttemptThatAnswersIsTheOneReturned() { + $calls = []; + $waits = []; + $body = Attempts::until( + $this->answering([false, '{"query":{}}'], $calls), + 3, + static function (int $attempt) use (&$waits) { + $waits[] = $attempt; + } + ); + $this->assertSame('{"query":{}}', $body); + $this->assertCount(2, $calls); + $this->assertSame([2], $waits); + } + + /** + * @dataProvider provideTooFewAttempts + */ + public function testWorkIsAlwaysDoneAtLeastOnce(int $attempts) { + $calls = []; + $waits = []; + $ok = Attempts::until( + $this->failing(0, $calls), + $attempts, + static function (int $attempt) use (&$waits) { + $waits[] = $attempt; + } + ); + $this->assertTrue($ok, 'asking for no attempts still means do the thing'); + $this->assertCount(1, $calls); + $this->assertSame([], $waits, 'and does it without waiting first'); + } + + public static function provideTooFewAttempts() { + return ['none' => [0], 'negative' => [-1]]; + } + + public function testTheWaitDoublesAndTheFirstAttemptNeverWaits() { + $this->assertSame(0, Attempts::backoff(1)); + $this->assertSame(2, Attempts::backoff(2)); + $this->assertSame(4, Attempts::backoff(3)); + $this->assertSame(8, Attempts::backoff(4)); + } + + public function testWaitingOutTheFirstAttemptCostsNothing() { + $before = microtime(true); + Attempts::sleep(1); + $this->assertLessThan(1.0, microtime(true) - $before, 'a zero backoff is not slept through'); + } +} diff --git a/tests/phpunit/unit/RetryingForeignRepoTest.php b/tests/phpunit/unit/RetryingForeignRepoTest.php deleted file mode 100644 index f9087b96c..000000000 --- a/tests/phpunit/unit/RetryingForeignRepoTest.php +++ /dev/null @@ -1,82 +0,0 @@ -requestReturning(['{"query":{}}'], $calls), - 3, - $this->pauseRecording($waits) - ); - - $this->assertSame('{"query":{}}', $body); - $this->assertCount(1, $calls); - $this->assertSame([], $waits, 'a request that answered must not be waited on'); - } - - public function testAFailedRequestIsRetriedUntilItAnswers() { - $calls = []; - $waits = []; - $body = RetryingForeignRepo::retry( - $this->requestReturning([false, '{"query":{}}', '{"unreached":{}}'], $calls), - 3, - $this->pauseRecording($waits) - ); - - $this->assertSame('{"query":{}}', $body); - $this->assertCount(2, $calls); - $this->assertSame([1], $waits); - } - - public function testAnEmptyResponseIsNotMistakenForAFailure() { - // A repo answering "" did answer; only false means the request itself did not go through. - $calls = []; - $waits = []; - $body = RetryingForeignRepo::retry( - $this->requestReturning(['', '{"unreached":{}}'], $calls), - 3, - $this->pauseRecording($waits) - ); - - $this->assertSame('', $body); - $this->assertCount(1, $calls); - } - - public function testAPersistentFailureGivesUpAfterTheLastAttempt() { - $calls = []; - $waits = []; - $body = RetryingForeignRepo::retry( - $this->requestReturning([false, false, false], $calls), - 3, - $this->pauseRecording($waits) - ); - - $this->assertFalse($body, 'the caller still sees the failure once the attempts run out'); - $this->assertCount(3, $calls); - $this->assertSame([1, 2], $waits, 'each retry waits a second longer than the one before'); - } -}