From b69d84520db675cf8cd0ac46f9f33027b94abb60 Mon Sep 17 00:00:00 2001 From: lens0021 Date: Tue, 18 Aug 2026 00:41:11 +0900 Subject: [PATCH 1/4] fix: give a fetch that reaches the network more than one go 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 --- Dockerfile | 21 ++++- includes/Attempts.php | 54 ++++++++++++ maintenance/fetchExtensions.php | 123 ++++++++++++++++++++++++---- tests/phpunit/unit/AttemptsTest.php | 93 +++++++++++++++++++++ 4 files changed, 269 insertions(+), 22 deletions(-) create mode 100644 includes/Attempts.php create mode 100644 tests/phpunit/unit/AttemptsTest.php 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..5a333e518 --- /dev/null +++ b/includes/Attempts.php @@ -0,0 +1,54 @@ + 1) { + $before($attempt); + } + if ($work()) { + return true; + } + } + return false; + } + + /** + * 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 ); + } +} 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..230af9231 --- /dev/null +++ b/tests/phpunit/unit/AttemptsTest.php @@ -0,0 +1,93 @@ + $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'); + } + + /** + * @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)); + } +} From a4689d692cc8dde0336127826ab521c5424648fb Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 15:34:35 +0000 Subject: [PATCH 2/4] refactor: keep one retry loop, not two that disagree 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 Claude-Session: https://claude.ai/code/session_01Uv1RzRurUH6wrgV5E9PESQ --- includes/Attempts.php | 43 ++++++---- includes/RetryingForeignRepo.php | 40 +++------ tests/phpunit/unit/AttemptsTest.php | 56 +++++++++++++ .../phpunit/unit/RetryingForeignRepoTest.php | 82 ------------------- 4 files changed, 95 insertions(+), 126 deletions(-) delete mode 100644 tests/phpunit/unit/RetryingForeignRepoTest.php diff --git a/includes/Attempts.php b/includes/Attempts.php index 5a333e518..844d5bb79 100644 --- a/includes/Attempts.php +++ b/includes/Attempts.php @@ -5,40 +5,50 @@ /** * How many times the build asks somebody else's server for something before it gives up. * - * A bake fetches third-party extensions and skins over the network, and until now one refusal ended - * it. That is a build failed by a service having a bad minute rather than by anything in the source: - * GitHub answered 500 for git and archive traffic for hours on 2026-08-17, and codeload has answered - * 429 under load. Neither says anything about the commit being built. + * A bake fetches third-party extensions and skins over the network, and asks Commons for the + * thumbnail of every image a page embeds. Until recently one refusal ended the build. That is a + * build failed by a service having a bad minute rather than by anything in the source: GitHub + * answered 500 for git and archive traffic for hours on 2026-08-17, and codeload has answered 429 + * under load. Neither says anything about the commit being built. * - * RetryingForeignRepo does the same thing for Commons thumbnails and keeps its own loop, because - * what it repeats is a request that answers with a body or with false. What is repeated here either - * worked or did not, and may have left a half-finished attempt behind for the next one to clear. + * Nothing in the box does this. MediaWiki's HttpRequestFactory and MWHttpRequest carry no retry at + * all; wikimedia/wait-condition-loop, which core does bundle, spends a time budget waiting for a + * condition to come true, which is not the shape of one fetch that either worked or did not and can + * take minutes; and Guzzle's retry middleware, which core also bundles, would reach only half of + * the callers here, since one of them repeats a `composer update` in a subprocess. */ class Attempts { /** How many times one fetch is tried. Two retries is enough for a blip and short of a queue. */ public const FETCH = 3; /** - * Run $work until it reports success, at most $attempts times. + * Run $work until it answers with something other than false, at most $attempts times. * - * @param callable():bool $work Does the work; answers whether it succeeded. + * False is the failure, and only false: a request can succeed with an empty body, and a caller + * that answers with a bool says so by returning true. Whatever the successful attempt answered + * is what comes back, so a caller that wants the body gets the body and one that only wants to + * know gets its true. + * + * @param callable():mixed $work Does the work; answers false if it failed, anything else if not. * @param int $attempts How many times to run it. Below one is treated as one: a caller that asks * for no attempts still means "do the thing", and a silent no-op here would read as success. * @param callable(int):mixed $before Run before every attempt after the first, given the number * of the attempt about to be made. Where the reporting, the waiting and any cleaning up go. - * @return bool Whether any attempt succeeded. + * @return mixed What the first attempt that did not fail answered, or false if none succeeded. */ - public static function until(callable $work, int $attempts, callable $before): bool { + public static function until(callable $work, int $attempts, callable $before) { $attempts = max(1, $attempts); + $answer = false; for ($attempt = 1; $attempt <= $attempts; $attempt++) { if ($attempt > 1) { $before($attempt); } - if ($work()) { - return true; + $answer = $work(); + if ($answer !== false) { + break; } } - return false; + return $answer; } /** @@ -51,4 +61,9 @@ public static function until(callable $work, int $attempts, callable $before): b 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/tests/phpunit/unit/AttemptsTest.php b/tests/phpunit/unit/AttemptsTest.php index 230af9231..2f508e7e3 100644 --- a/tests/phpunit/unit/AttemptsTest.php +++ b/tests/phpunit/unit/AttemptsTest.php @@ -62,6 +62,56 @@ static function (int $attempt) use (&$waits) { $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 = []; + $body = Attempts::until( + $this->answering(['', 'unreached'], $calls), + 3, + static function (int $attempt) { + } + ); + $this->assertSame('', $body, 'false is the failure, and only false'); + $this->assertCount(1, $calls); + } + + 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 */ @@ -90,4 +140,10 @@ public function testTheWaitDoublesAndTheFirstAttemptNeverWaits() { $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'); - } -} From c596e8d4ef9eb78aeaf568e9420ecec419c70f27 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 15:42:12 +0000 Subject: [PATCH 3/4] style: close the empty closure the way mago wants it mago format keeps an empty function body on the line that opens it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Uv1RzRurUH6wrgV5E9PESQ --- tests/phpunit/unit/AttemptsTest.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/phpunit/unit/AttemptsTest.php b/tests/phpunit/unit/AttemptsTest.php index 2f508e7e3..8a800c18b 100644 --- a/tests/phpunit/unit/AttemptsTest.php +++ b/tests/phpunit/unit/AttemptsTest.php @@ -90,8 +90,7 @@ public function testAnEmptyAnswerIsNotMistakenForAFailure() { $body = Attempts::until( $this->answering(['', 'unreached'], $calls), 3, - static function (int $attempt) { - } + static function (int $attempt) {} ); $this->assertSame('', $body, 'false is the failure, and only false'); $this->assertCount(1, $calls); From 1e9465ecea185d59a4796f0b53d539efdb963e17 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 15:51:27 +0000 Subject: [PATCH 4/4] test: record the waits instead of ignoring them 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 Claude-Session: https://claude.ai/code/session_01Uv1RzRurUH6wrgV5E9PESQ --- tests/phpunit/unit/AttemptsTest.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/phpunit/unit/AttemptsTest.php b/tests/phpunit/unit/AttemptsTest.php index 8a800c18b..0c7e78e73 100644 --- a/tests/phpunit/unit/AttemptsTest.php +++ b/tests/phpunit/unit/AttemptsTest.php @@ -87,13 +87,17 @@ static function (int $attempt) use (&$waits) { public function testAnEmptyAnswerIsNotMistakenForAFailure() { $calls = []; + $waits = []; $body = Attempts::until( $this->answering(['', 'unreached'], $calls), 3, - static function (int $attempt) {} + 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() {