From 784ba3c0f68e86b08f627bf1e78bd2ee961b9f89 Mon Sep 17 00:00:00 2001 From: roxblnfk Date: Mon, 12 Oct 2026 12:29:51 +0400 Subject: [PATCH 1/5] fix(registry): continue the tail from the last stored release fix(repository): map a release offset onto the page size the server serves A release deleted upstream below the checked span, or a vanished draft placeholder, shifted the listing and `extend()` skipped a release for good; with offset paging the position can only be confirmed by finding the last stored release. Within the TTL the record is trusted whole, like its head, so the confirmation costs a request only after a check and only on a page boundary. The sources assumed 100 releases per page; the real size is learned from a page followed by another one rather than stored, to keep provider paging out of the record format. Closes #159 Assisted-By: Claude Fable 5.1 --- .../Internal/StoredVersionRegistry.php | 77 ++++++++-- .../Registry/Record/RepositoryRecord.php | 48 +++++++ .../Internal/GitHub/GitHubReleaseSource.php | 51 ++++++- .../Internal/GitLab/GitLabReleaseSource.php | 51 ++++++- .../Module/Registry/RepositoryRecordTest.php | 28 ++++ .../Registry/StoredVersionRegistryTest.php | 133 ++++++++++++++++++ .../Internal/GitHub/GitHubRepositoryTest.php | 36 +++++ .../Internal/GitHub/Stub/PagedClientStub.php | 6 +- .../Internal/GitLab/GitLabRepositoryTest.php | 37 +++++ .../Internal/GitLab/Stub/PagedClientStub.php | 6 +- 10 files changed, 449 insertions(+), 24 deletions(-) diff --git a/src/Module/Registry/Internal/StoredVersionRegistry.php b/src/Module/Registry/Internal/StoredVersionRegistry.php index 578b75b9..06bd49fa 100644 --- a/src/Module/Registry/Internal/StoredVersionRegistry.php +++ b/src/Module/Registry/Internal/StoredVersionRegistry.php @@ -25,7 +25,9 @@ * attached after the check are updated too. * 2. **Serve.** The stored releases are yielded segment by segment without any request. * 3. **Extend.** When the consumer runs past the stored releases and the listing is not known - * to be complete, older pages are fetched one by one and appended to the record. + * to be complete, older pages are fetched one by one and appended to the record. After a + * check, the stored releases below the checked ones are first confirmed to be where they + * were, as a deletion among them would shift the listing. * * A check that fails while releases are stored falls back to the stored ones: an outage or a rate * limit should not break what worked a minute ago. A rate limit is reported once per run in plain @@ -60,8 +62,10 @@ public function releases(RepositoryId $id, ReleaseSource $source): \Generator { $record = $this->storage->load($id) ?? RepositoryRecord::empty($id); + // Within the TTL the stored list is taken as it is, head and tail alike + $verified = $record->count(); if ($this->refresh || $record->isStale(($this->clock)(), $this->ttl)) { - $record = $this->check($record, $source); + [$record, $verified] = $this->check($record, $source); } else { $this->logger->debug('Releases of `%s` are served from the version registry.', (string) $id); } @@ -75,7 +79,7 @@ public function releases(RepositoryId $id, ReleaseSource $source): \Generator } // Older releases are loaded only when the consumer actually needs them - yield from $this->extend($record, $source); + yield from $this->extend($record, $source, $verified); } public function attach(RepositoryId $id, string $software): void @@ -116,9 +120,13 @@ private static function hasKnown(RepositoryRecord $record, array $page): bool /** * Fetches the releases published since the last check and stores the result. * + * Besides the record, tells how many of its leading releases the check has seen in place: + * the stored releases beyond them may have shifted since they were stored. + * + * @return array{RepositoryRecord, int<0, max>} * @throws RepositoryException When the source fails and nothing is stored to fall back on. */ - private function check(RepositoryRecord $record, ReleaseSource $source): RepositoryRecord + private function check(RepositoryRecord $record, ReleaseSource $source): array { try { $fetched = []; @@ -136,7 +144,7 @@ private function check(RepositoryRecord $record, ReleaseSource $source): Reposit $record->count(), ); - return $record; + return [$record, \count($fetched)]; } $fetched = [...$fetched, ...$page->releases]; @@ -151,38 +159,83 @@ private function check(RepositoryRecord $record, ReleaseSource $source): Reposit } } - return $this->persist( + // A listing without a single page has no releases at all + $fetched === [] and $complete = true; + + $record = $this->persist( $record ->withHead($fetched) ->withComplete($complete) ->withCheckedAt(($this->clock)()), ); + + return [$record, \count($fetched)]; } catch (RepositoryException $e) { $record->count() > 0 or throw $e; $this->report($e, $record); - return $record; + return [$record, 0]; } } /** * Loads the releases older than the stored ones page by page, persisting every page. * + * The source pages by position, and a stored release deleted upstream below the verified + * ones moves every older release up by one. So past them the loading starts one release + * earlier and expects the last stored release there; when another one shows up, the + * unverified tail is dropped and loaded again, without repeating to the consumer what it was + * served from the stored list. The extra release lies on the page needed anyway, unless the + * stored list ends exactly on a page boundary: then it costs one more request. + * + * @param int<0, max> $verified Number of leading stored releases known to be in place. + * @param RepositoryRecord|null $served The releases the consumer has been served already. * @return \Generator, mixed, void> * @throws RepositoryException */ - private function extend(RepositoryRecord $record, ReleaseSource $source): \Generator - { - foreach ($source->pages($record->count()) as $page) { + private function extend( + RepositoryRecord $record, + ReleaseSource $source, + int $verified, + ?RepositoryRecord $served = null, + ): \Generator { + $served ??= $record; + $anchor = $verified < $record->count() ? $record->lastTag() : null; + /** @var int<0, max> $offset */ + $offset = $anchor === null ? $record->count() : $record->count() - 1; + + foreach ($source->pages($offset) as $page) { + $releases = $page->releases; + if ($anchor !== null) { + if (($releases[0] ?? null)?->tag !== $anchor) { + $this->logger->debug( + 'The listing of `%s` has shifted below the %d checked release(s): the stored tail is loaded again.', + (string) $record->id, + $verified, + ); + + $record = $this->persist($record->withoutTail($verified)); + yield from $this->extend($record, $source, $record->count(), $served); + + return; + } + + $anchor = null; + $releases = \array_slice($releases, 1); + } + $new = \array_values(\array_filter( - $page->releases, + $releases, static fn(ReleaseRecord $release): bool => !$record->has($release->tag), )); $record = $this->persist($record->withTail($new)->withComplete($page->last)); - $visible = ReleaseRecord::visible($new); + $visible = ReleaseRecord::visible(\array_values(\array_filter( + $new, + static fn(ReleaseRecord $release): bool => !$served->has($release->tag), + ))); $visible === [] or yield $visible; } diff --git a/src/Module/Registry/Record/RepositoryRecord.php b/src/Module/Registry/Record/RepositoryRecord.php index 089e91a6..46af809f 100644 --- a/src/Module/Registry/Record/RepositoryRecord.php +++ b/src/Module/Registry/Record/RepositoryRecord.php @@ -300,6 +300,54 @@ public function withTail(array $fetched): self ]); } + /** + * Drops the releases from the given position on; the list is incomplete afterwards. + * + * @param int<0, max> $from Number of releases to keep. + */ + public function withoutTail(int $from): self + { + if ($from >= $this->count()) { + return $this; + } + + $segments = []; + $kept = 0; + foreach ($this->segments as $segment) { + if ($kept + $segment->count() <= $from) { + $segments[] = $segment; + $kept += $segment->count(); + continue; + } + + /** @var non-empty-list $releases */ + $releases = \array_slice($segment->releases(), 0, $from - $kept); + $from > $kept and $segments[] = ReleaseSegment::fresh($segment->key, $releases); + break; + } + + return $this->with(complete: false, segments: $segments); + } + + /** + * Tag of the oldest stored release, or `null` when nothing is stored. + * + * @return non-empty-string|null + */ + public function lastTag(): ?string + { + $segments = $this->segments; + $last = \end($segments); + if ($last === false) { + return null; + } + + $tags = $last->tags; + $tag = \end($tags); + + return $tag === false ? null : $tag; + } + public function withCheckedAt(int $checkedAt): self { return $this->with(checkedAt: $checkedAt); diff --git a/src/Module/Repository/Internal/GitHub/GitHubReleaseSource.php b/src/Module/Repository/Internal/GitHub/GitHubReleaseSource.php index 6d334c39..1169b064 100644 --- a/src/Module/Repository/Internal/GitHub/GitHubReleaseSource.php +++ b/src/Module/Repository/Internal/GitHub/GitHubReleaseSource.php @@ -12,25 +12,68 @@ * Adapts the GitHub releases API to the version registry. * * Translates a release offset into the API page and the number of releases to drop from it. + * The page size is the one asked for until a response shows that the server puts fewer + * releases on a page, as a mirror or a server that caps `per_page` does. * * @internal * @psalm-internal Internal\DLoad\Module\Repository\Internal\GitHub */ final class GitHubReleaseSource implements ReleaseSource { + /** @var int<1, max> */ + private int $perPage = RepositoryApi::RELEASES_PER_PAGE; + public function __construct( private readonly RepositoryApi $api, ) {} public function pages(int $offset = 0): \Generator { - $skip = $offset % RepositoryApi::RELEASES_PER_PAGE; - /** @var int<1, max> $first */ - $first = \intdiv($offset, RepositoryApi::RELEASES_PER_PAGE) + 1; + $first = $this->pageOf($offset); + $pages = $this->api->releasePages($first); + $page = $pages->current(); + + // A page followed by another one holds exactly as many releases as the server serves per + // page; the offset may then lie on a later page than the one fetched + if ($page !== null && !$page->last && $this->learn($page) && $this->pageOf($offset) !== $first) { + $pages = $this->api->releasePages($this->pageOf($offset)); + $page = $pages->current(); + } - foreach ($this->api->releasePages($first) as $page) { + // The generator is walked by hand: one that ended before its first page is closed, and + // `foreach` cannot start it over + $skip = $offset % $this->perPage; + while ($page !== null) { yield $skip === 0 ? $page : new ReleasePage(\array_slice($page->releases, $skip), $page->last, $page->skipped); $skip = 0; + + $pages->next(); + $page = $pages->current(); } } + + /** + * Takes the size of a full page as the page size; tells whether it changed. + */ + private function learn(ReleasePage $page): bool + { + $size = \count($page->releases) + $page->skipped; + if ($size < 1 || $size === $this->perPage) { + return false; + } + + $this->perPage = $size; + + return true; + } + + /** + * @param int<0, max> $offset + * @return int<1, max> + */ + private function pageOf(int $offset): int + { + /** @var int<1, max> */ + return \intdiv($offset, $this->perPage) + 1; + } } diff --git a/src/Module/Repository/Internal/GitLab/GitLabReleaseSource.php b/src/Module/Repository/Internal/GitLab/GitLabReleaseSource.php index cb0e22d2..3c9dfdec 100644 --- a/src/Module/Repository/Internal/GitLab/GitLabReleaseSource.php +++ b/src/Module/Repository/Internal/GitLab/GitLabReleaseSource.php @@ -12,25 +12,68 @@ * Adapts the GitLab releases API to the version registry. * * Translates a release offset into the API page and the number of releases to drop from it. + * The page size is the one asked for until a response shows that the server puts fewer + * releases on a page, as a mirror or a server that caps `per_page` does. * * @internal * @psalm-internal Internal\DLoad\Module\Repository\Internal\GitLab */ final class GitLabReleaseSource implements ReleaseSource { + /** @var int<1, max> */ + private int $perPage = RepositoryApi::RELEASES_PER_PAGE; + public function __construct( private readonly RepositoryApi $api, ) {} public function pages(int $offset = 0): \Generator { - $skip = $offset % RepositoryApi::RELEASES_PER_PAGE; - /** @var int<1, max> $first */ - $first = \intdiv($offset, RepositoryApi::RELEASES_PER_PAGE) + 1; + $first = $this->pageOf($offset); + $pages = $this->api->releasePages($first); + $page = $pages->current(); + + // A page followed by another one holds exactly as many releases as the server serves per + // page; the offset may then lie on a later page than the one fetched + if ($page !== null && !$page->last && $this->learn($page) && $this->pageOf($offset) !== $first) { + $pages = $this->api->releasePages($this->pageOf($offset)); + $page = $pages->current(); + } - foreach ($this->api->releasePages($first) as $page) { + // The generator is walked by hand: one that ended before its first page is closed, and + // `foreach` cannot start it over + $skip = $offset % $this->perPage; + while ($page !== null) { yield $skip === 0 ? $page : new ReleasePage(\array_slice($page->releases, $skip), $page->last, $page->skipped); $skip = 0; + + $pages->next(); + $page = $pages->current(); } } + + /** + * Takes the size of a full page as the page size; tells whether it changed. + */ + private function learn(ReleasePage $page): bool + { + $size = \count($page->releases) + $page->skipped; + if ($size < 1 || $size === $this->perPage) { + return false; + } + + $this->perPage = $size; + + return true; + } + + /** + * @param int<0, max> $offset + * @return int<1, max> + */ + private function pageOf(int $offset): int + { + /** @var int<1, max> */ + return \intdiv($offset, $this->perPage) + 1; + } } diff --git a/tests/Unit/Module/Registry/RepositoryRecordTest.php b/tests/Unit/Module/Registry/RepositoryRecordTest.php index 37910c26..5d1fe769 100644 --- a/tests/Unit/Module/Registry/RepositoryRecordTest.php +++ b/tests/Unit/Module/Registry/RepositoryRecordTest.php @@ -213,6 +213,34 @@ public function droppingTheLastReleaseOfASegmentDropsTheSegment(): void Assert::false($updated->segments[0]->dirty); } + #[Test] + public function tailIsDroppedFromInsideASegment(): void + { + $record = self::record(self::range(250, 1))->withComplete(true); + Assert::same(self::sizes($record), [50, 100, 100]); + + $updated = $record->withoutTail(120); + + Assert::same(self::sizes($updated), [50, 70]); + Assert::same(self::keys($updated), ['0001', '0002']); + Assert::same($updated->lastTag(), 'v131'); + Assert::false($updated->complete); + Assert::false($updated->segments[0]->dirty); + Assert::true($updated->segments[1]->dirty); + } + + #[Test] + public function tailIsDroppedAtASegmentBoundary(): void + { + $record = self::record(self::range(250, 1)); + + Assert::same(self::sizes($record->withoutTail(150)), [50, 100]); + Assert::same(self::sizes($record->withoutTail(0)), []); + Assert::null($record->withoutTail(0)->lastTag()); + Assert::same($record->withoutTail(250), $record); + Assert::same($record->lastTag(), 'v1'); + } + #[Test] public function stalenessDependsOnTheLastCheck(): void { diff --git a/tests/Unit/Module/Registry/StoredVersionRegistryTest.php b/tests/Unit/Module/Registry/StoredVersionRegistryTest.php index 45cbf5ad..6852a0f3 100644 --- a/tests/Unit/Module/Registry/StoredVersionRegistryTest.php +++ b/tests/Unit/Module/Registry/StoredVersionRegistryTest.php @@ -141,6 +141,123 @@ public function releaseDeletedUpstreamDoesNotShiftTheTail(): void Assert::same($source->served, [0, 2, 4]); } + #[Test] + public function releaseDeletedBelowTheCheckedSpanReloadsTheTail(): void + { + $source = ArrayReleaseSource::ofTags(['v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + + // The first run stores three pages and stops + $pages = $this->registry()->releases($this->id, $source); + self::take($pages, 6); + $source->served = []; + + // `v3` is deleted upstream, outside the page the check will fetch + $source->delete('v3'); + $this->now += 601; + $again = self::flatten($this->registry()->releases($this->id, $source)); + + // The stored `v3` is served as it was, but `v2` is not skipped: the check confirms + // page 1 only, the tail is found shifted at offset 5 and is loaded again from offset 2 + Assert::same($again, ['v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + Assert::same($source->served, [0, 4, 2, 4, 6]); + + $record = $this->storage->load($this->id); + Assert::same(self::tagsOf($record->releases()), ['v8', 'v7', 'v6', 'v5', 'v4', 'v2', 'v1']); + Assert::true($record->complete); + } + + #[Test] + public function vanishedDraftPlaceholderReloadsTheTail(): void + { + $source = new ArrayReleaseSource([ + new ReleaseRecord('v6', 'v6'), + new ReleaseRecord('v5', 'v5'), + new ReleaseRecord('v4', 'v4'), + new ReleaseRecord('draft', 'draft', hidden: true), + new ReleaseRecord('v2', 'v2'), + new ReleaseRecord('v1', 'v1'), + ]); + self::take($this->registry()->releases($this->id, $source), 3); + Assert::same($this->storage->load($this->id)?->count(), 4); + $source->served = []; + + // The draft is gone from the listing, as after its deletion or with a token that cannot see it + $source->delete('draft'); + $this->now += 601; + $again = self::flatten($this->registry()->releases($this->id, $source)); + + Assert::same($again, ['v6', 'v5', 'v4', 'v2', 'v1']); + Assert::same($source->served, [0, 2, 2, 4]); + Assert::false($this->storage->load($this->id)->has('draft')); + } + + #[Test] + public function tailBelowTheCheckedSpanIsConfirmedOnThePageItContinuesFrom(): void + { + $source = ArrayReleaseSource::ofTags(['v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + $this->registry()->releases($this->id, $source)->current(); + $source->served = []; + + // The check confirms page 1 and leaves three stored, the last one in the middle of + // page 2: it is confirmed on the page the tail continues from + $source->publish('v7'); + $this->now += 601; + $again = self::flatten($this->registry()->releases($this->id, $source)); + + Assert::same($again, ['v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + Assert::same($source->served, [0, 2, 4, 6]); + } + + #[Test] + public function tailEndingOnAPageBoundaryCostsOneRequestToConfirm(): void + { + $source = ArrayReleaseSource::ofTags(['v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + self::take($this->registry()->releases($this->id, $source), 4); + $source->served = []; + + // Four are stored, the check confirms two: the page holding the fourth is fetched to + // confirm it before the tail goes on + $this->now += 601; + $again = self::flatten($this->registry()->releases($this->id, $source)); + + Assert::same($again, ['v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + Assert::same($source->served, [0, 2, 4]); + } + + #[Test] + public function releasesPublishedWithinTheTtlOverlapTheTailWithoutRepeating(): void + { + $source = ArrayReleaseSource::ofTags(['v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + $this->registry()->releases($this->id, $source)->current(); + $source->served = []; + + // The record is fresh, so nothing is confirmed; the listing shifted down by one + $source->publish('v7'); + $this->now += 100; + $again = self::flatten($this->registry()->releases($this->id, $source)); + + Assert::same($again, ['v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + Assert::same($source->served, [2, 4, 6]); + } + + #[Test] + public function failedCheckLeavesTheStoredTailUnconfirmed(): void + { + $source = ArrayReleaseSource::ofTags(['v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + self::take($this->registry()->releases($this->id, $source), 4); + $source->served = []; + + $source->fail(); + $this->now += 601; + $pages = $this->registry()->releases($this->id, $source); + Assert::same(self::tagsOf($pages->current()), ['v6', 'v5', 'v4', 'v3']); + + // The source is back: the tail is confirmed from its last stored release on + $source->failure = null; + Assert::same(self::flatten($pages), ['v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + Assert::same($source->served, [2, 4]); + } + #[Test] public function unreadableReleaseIsNotTakenForADeletedOne(): void { @@ -362,6 +479,22 @@ private static function flatten(iterable $pages): array return $tags; } + /** + * Consumes pages until the given number of releases has been served. + * + * @param \Generator, mixed, void> $pages + */ + private static function take(\Generator $pages, int $count): void + { + $seen = 0; + foreach ($pages as $page) { + $seen += \count($page); + if ($seen >= $count) { + break; + } + } + } + /** * @param list $releases * @return list diff --git a/tests/Unit/Module/Repository/Internal/GitHub/GitHubRepositoryTest.php b/tests/Unit/Module/Repository/Internal/GitHub/GitHubRepositoryTest.php index e678d35e..a942a494 100644 --- a/tests/Unit/Module/Repository/Internal/GitHub/GitHubRepositoryTest.php +++ b/tests/Unit/Module/Repository/Internal/GitHub/GitHubRepositoryTest.php @@ -181,6 +181,42 @@ public function tailIsLoadedFromInsideAPageWhenTheStoredCountIsNotPageAligned(): Assert::same($all[50], 'v1.0.51'); } + #[Test] + public function repositoryWithoutReleasesListsNothing(): void + { + $client = new PagedClientStub(pages: 0); + + Assert::same(self::names(self::createRepository($client, self::registry(new InMemoryRegistryStorage()))), []); + Assert::same($client->requestedPages(), [1]); + Assert::same(\iterator_to_array((new GitHubReleaseSource(self::api($client)))->pages(150), false), []); + } + + #[Test] + public function serverCappingThePageSizeIsAskedForEveryPageOnce(): void + { + // Six releases, two per page whatever `per_page` says; the first run needs three of them + $storage = new InMemoryRegistryStorage(); + $firstClient = new PagedClientStub(pages: 3, releasesPerPage: 2, perPageLimit: 2); + $seen = 0; + foreach (self::createRepository($firstClient, self::registry($storage))->getReleases() as $release) { + unset($release); + if (++$seen === 3) { + break; + } + } + + // The first page tells the page size, so the tail continues on page 2, not on page 1 again + Assert::same($firstClient->requestedPages(), [1, 2]); + + // A later run learns the page size from the first page anew and jumps to the right one + $secondClient = new PagedClientStub(pages: 3, releasesPerPage: 2, perPageLimit: 2); + $all = self::names(self::createRepository($secondClient, self::registry($storage))); + + Assert::same($secondClient->requestedPages(), [1, 3]); + Assert::same(\count($all), 6); + Assert::same(\count(\array_unique($all)), 6); + } + #[Test] public function theSameCollectionIsReturnedOnEveryCall(): void { diff --git a/tests/Unit/Module/Repository/Internal/GitHub/Stub/PagedClientStub.php b/tests/Unit/Module/Repository/Internal/GitHub/Stub/PagedClientStub.php index d1070a12..51895a5b 100644 --- a/tests/Unit/Module/Repository/Internal/GitHub/Stub/PagedClientStub.php +++ b/tests/Unit/Module/Repository/Internal/GitHub/Stub/PagedClientStub.php @@ -26,16 +26,18 @@ final class PagedClientStub implements ClientInterface public array $requests = []; /** - * @param int<1, max> $pages Number of pages the list is split into when 100 releases are requested per page. + * @param int<0, max> $pages Number of pages the list is split into when 100 releases are requested per page. * @param int<1, max> $releasesPerPage Number of releases on every such page. * @param int<0, max> $drafts Draft releases listed on top, as the API shows them to the token holder. * @param int<0, max> $broken Number of the newest published releases that cannot be decoded. + * @param int<1, max>|null $perPageLimit Greatest number of releases served per page whatever `per_page` asks for. */ public function __construct( private readonly int $pages = 1, private readonly int $releasesPerPage = 100, private readonly int $drafts = 0, private readonly int $broken = 0, + private readonly ?int $perPageLimit = null, ) {} public function sendRequest(RequestInterface $request): ResponseInterface @@ -44,7 +46,7 @@ public function sendRequest(RequestInterface $request): ResponseInterface $this->requests[] = $query; $page = self::pageOf($query); - $perPage = self::perPageOf($query); + $perPage = \min(self::perPageOf($query), $this->perPageLimit ?? \PHP_INT_MAX); $all = $this->allReleases(); // Serve the slice the real API would serve for the requested page size diff --git a/tests/Unit/Module/Repository/Internal/GitLab/GitLabRepositoryTest.php b/tests/Unit/Module/Repository/Internal/GitLab/GitLabRepositoryTest.php index 396b9a29..731938fe 100644 --- a/tests/Unit/Module/Repository/Internal/GitLab/GitLabRepositoryTest.php +++ b/tests/Unit/Module/Repository/Internal/GitLab/GitLabRepositoryTest.php @@ -120,6 +120,36 @@ public function olderReleasesAreLoadedFromTheApiWhenTheRegistryRunsOut(): void Assert::same(\count($all), 300); } + #[Test] + public function projectWithoutReleasesListsNothing(): void + { + $client = new PagedClientStub(pages: 0); + + Assert::same(self::names(self::createRepository($client, self::registry(new InMemoryRegistryStorage()))), []); + Assert::same($client->requestedPages(), [1]); + Assert::same(\iterator_to_array(self::source($client)->pages(150), false), []); + } + + #[Test] + public function serverCappingThePageSizeIsAskedForEveryPageOnce(): void + { + // Four releases, two per page whatever `per_page` says + $client = new PagedClientStub(pages: 2, releasesPerPage: 2, perPageLimit: 2); + $all = self::names(self::createRepository($client, self::registry(new InMemoryRegistryStorage()))); + + // The first page tells the page size, so the tail continues on page 2, not on page 1 again + Assert::same($client->requestedPages(), [1, 2]); + Assert::same(\count($all), 4); + + // A source that does not know the page size yet learns it from page 1 and jumps to the right one + $client = new PagedClientStub(pages: 3, releasesPerPage: 2, perPageLimit: 2); + $pages = \iterator_to_array(self::source($client)->pages(4), false); + + Assert::same($client->requestedPages(), [1, 3]); + Assert::count($pages, 1); + Assert::same($pages[0]->releases[0]->tag, 'v1.0.5'); + } + #[Test] public function tailIsLoadedFromInsideAPageWhenTheStoredCountIsNotPageAligned(): void { @@ -283,6 +313,13 @@ private static function registry(InMemoryRegistryStorage $storage): StoredVersio return new StoredVersionRegistry($storage, 600, new Logger()); } + private static function source(PagedClientStub $client): GitLabReleaseSource + { + $httpFactory = new NyholmFactoryImpl(new Logger()); + + return new GitLabReleaseSource(new RepositoryApi(new Client($httpFactory, $client), $httpFactory, 'group/project')); + } + /** * @return list */ diff --git a/tests/Unit/Module/Repository/Internal/GitLab/Stub/PagedClientStub.php b/tests/Unit/Module/Repository/Internal/GitLab/Stub/PagedClientStub.php index ca644008..31ff23bc 100644 --- a/tests/Unit/Module/Repository/Internal/GitLab/Stub/PagedClientStub.php +++ b/tests/Unit/Module/Repository/Internal/GitLab/Stub/PagedClientStub.php @@ -33,12 +33,14 @@ final class PagedClientStub implements ClientInterface public array $uris = []; /** - * @param int<1, max> $pages Number of pages the list is split into when 100 releases are requested per page. + * @param int<0, max> $pages Number of pages the list is split into when 100 releases are requested per page. * @param int<1, max> $releasesPerPage Number of releases on every such page. + * @param int<1, max>|null $perPageLimit Greatest number of releases served per page whatever `per_page` asks for. */ public function __construct( private readonly int $pages = 1, private readonly int $releasesPerPage = 100, + private readonly ?int $perPageLimit = null, ) {} public function sendRequest(RequestInterface $request): ResponseInterface @@ -48,7 +50,7 @@ public function sendRequest(RequestInterface $request): ResponseInterface $this->uris[] = (string) $request->getUri(); $page = self::pageOf($query); - $perPage = self::perPageOf($query); + $perPage = \min(self::perPageOf($query), $this->perPageLimit ?? \PHP_INT_MAX); $all = $this->allReleases(); // Serve the slice the real API would serve for the requested page size From 6785c4a9b368221e09493e559b00469ff11e8e7f Mon Sep 17 00:00:00 2001 From: roxblnfk Date: Mon, 12 Oct 2026 12:51:06 +0400 Subject: [PATCH 2/5] fix(registry): keep the stored tail when its confirmation fails refactor(repository): share the paged release source between providers A check that failed or bailed out has nothing to compare the stored list against, so the tail is taken as it is: a confirmation there would spend a request where a rate limit just refused one, and a tail dropped before its reload could fail left an empty record behind. The dropped tail is now written only along with the first page that replaces it. A source that answers the anchor position with no page at all has shifted too; taking it for a complete listing kept losing the release. The page size is learned from the first page only, the one page whose start is known: a shorter page further down is a release filtered out after paging, and taking it for the page size dropped releases within the TTL. Assisted-By: Claude Fable 5.1 --- .../Registry/Internal/PagedReleaseSource.php | 90 +++++++++++++++++++ .../Internal/StoredVersionRegistry.php | 60 ++++++++----- .../Internal/GitHub/GitHubReleaseSource.php | 79 ---------------- .../Internal/GitHub/GitHubRepository.php | 3 +- .../Internal/GitLab/GitLabReleaseSource.php | 79 ---------------- .../Internal/GitLab/GitLabRepository.php | 3 +- .../Registry/StoredVersionRegistryTest.php | 53 ++++++++++- .../Registry/Stub/ArrayReleaseSource.php | 10 +++ .../Internal/GitHub/GitHubRepositoryTest.php | 66 ++++++++++++-- .../Internal/GitHub/Stub/PagedClientStub.php | 7 ++ .../Internal/GitLab/GitLabRepositoryTest.php | 10 ++- 11 files changed, 263 insertions(+), 197 deletions(-) create mode 100644 src/Module/Registry/Internal/PagedReleaseSource.php delete mode 100644 src/Module/Repository/Internal/GitHub/GitHubReleaseSource.php delete mode 100644 src/Module/Repository/Internal/GitLab/GitLabReleaseSource.php diff --git a/src/Module/Registry/Internal/PagedReleaseSource.php b/src/Module/Registry/Internal/PagedReleaseSource.php new file mode 100644 index 00000000..3e892cfc --- /dev/null +++ b/src/Module/Registry/Internal/PagedReleaseSource.php @@ -0,0 +1,90 @@ + */ + private int $perPage; + + /** + * @param \Closure(int<1, max>): \Generator $pages Lists the + * pages from the given one on, requesting each one when reached. + * @param int<1, max> $perPage Number of releases the API is asked for per page. + */ + public function __construct( + private readonly \Closure $pages, + int $perPage, + ) { + $this->perPage = $perPage; + } + + /** + * @throws RepositoryException + */ + public function pages(int $offset = 0): \Generator + { + $first = $this->pageOf($offset); + $pages = ($this->pages)($first); + $page = $pages->current(); + + if ($first === 1 && $page !== null && !$page->last && $this->learn($page) && $this->pageOf($offset) !== 1) { + $pages = ($this->pages)($this->pageOf($offset)); + $page = $pages->current(); + } + + // The generator is walked by hand: one that ended before its first page is closed, and + // `foreach` cannot start it over + $skip = $offset % $this->perPage; + while ($page !== null) { + yield $skip === 0 ? $page : new ReleasePage(\array_slice($page->releases, $skip), $page->last, $page->skipped); + $skip = 0; + + $pages->next(); + $page = $pages->current(); + } + } + + /** + * Takes the length of a page followed by another one as the page size; tells whether it changed. + */ + private function learn(ReleasePage $page): bool + { + $size = \count($page->releases) + $page->skipped; + if ($size < 1 || $size === $this->perPage) { + return false; + } + + $this->perPage = $size; + + return true; + } + + /** + * @param int<0, max> $offset + * @return int<1, max> + */ + private function pageOf(int $offset): int + { + /** @var int<1, max> */ + return \intdiv($offset, $this->perPage) + 1; + } +} diff --git a/src/Module/Registry/Internal/StoredVersionRegistry.php b/src/Module/Registry/Internal/StoredVersionRegistry.php index 06bd49fa..3f10481d 100644 --- a/src/Module/Registry/Internal/StoredVersionRegistry.php +++ b/src/Module/Registry/Internal/StoredVersionRegistry.php @@ -4,6 +4,7 @@ namespace Internal\DLoad\Module\Registry\Internal; +use Internal\DLoad\Module\Registry\Record\ReleasePage; use Internal\DLoad\Module\Registry\Record\ReleaseRecord; use Internal\DLoad\Module\Registry\Record\RepositoryRecord; use Internal\DLoad\Module\Registry\RegistryStorage; @@ -121,7 +122,8 @@ private static function hasKnown(RepositoryRecord $record, array $page): bool * Fetches the releases published since the last check and stores the result. * * Besides the record, tells how many of its leading releases the check has seen in place: - * the stored releases beyond them may have shifted since they were stored. + * the stored releases beyond them may have shifted since they were stored. A check that + * leaves the record as it is vouches for all of it, as it has nothing to compare against. * * @return array{RepositoryRecord, int<0, max>} * @throws RepositoryException When the source fails and nothing is stored to fall back on. @@ -144,7 +146,7 @@ private function check(RepositoryRecord $record, ReleaseSource $source): array $record->count(), ); - return [$record, \count($fetched)]; + return [$record, $record->count()]; } $fetched = [...$fetched, ...$page->releases]; @@ -175,7 +177,7 @@ private function check(RepositoryRecord $record, ReleaseSource $source): array $this->report($e, $record); - return [$record, 0]; + return [$record, $record->count()]; } } @@ -184,49 +186,56 @@ private function check(RepositoryRecord $record, ReleaseSource $source): array * * The source pages by position, and a stored release deleted upstream below the verified * ones moves every older release up by one. So past them the loading starts one release - * earlier and expects the last stored release there; when another one shows up, the - * unverified tail is dropped and loaded again, without repeating to the consumer what it was - * served from the stored list. The extra release lies on the page needed anyway, unless the - * stored list ends exactly on a page boundary: then it costs one more request. + * earlier and expects the last stored release there; when another one shows up, or no page + * at all, the unverified tail is dropped and loaded again, without repeating to the consumer + * what it was served from the stored list. The dropped tail is written only along with the + * first page that replaces it, so a source failing right there leaves the record as it was. + * + * The extra release lies on the page needed anyway when the stored list ends inside a page. + * A list ending on a page boundary, as one made of fully loaded pages does, costs one more + * request for the page that ends with the release. * * @param int<0, max> $verified Number of leading stored releases known to be in place. - * @param RepositoryRecord|null $served The releases the consumer has been served already. * @return \Generator, mixed, void> * @throws RepositoryException */ - private function extend( - RepositoryRecord $record, - ReleaseSource $source, - int $verified, - ?RepositoryRecord $served = null, - ): \Generator { - $served ??= $record; + private function extend(RepositoryRecord $record, ReleaseSource $source, int $verified): \Generator + { + $served = $record; $anchor = $verified < $record->count() ? $record->lastTag() : null; /** @var int<0, max> $offset */ $offset = $anchor === null ? $record->count() : $record->count() - 1; - foreach ($source->pages($offset) as $page) { - $releases = $page->releases; + $pages = $source->pages($offset); + $page = $pages->current(); + + while (true) { if ($anchor !== null) { - if (($releases[0] ?? null)?->tag !== $anchor) { + if ($page === null || ($page->releases[0] ?? null)?->tag !== $anchor) { $this->logger->debug( - 'The listing of `%s` has shifted below the %d checked release(s): the stored tail is loaded again.', + 'The stored tail of `%s` has shifted: %d release(s) are kept, the rest is loaded again.', (string) $record->id, $verified, ); - $record = $this->persist($record->withoutTail($verified)); - yield from $this->extend($record, $source, $record->count(), $served); + $record = $record->withoutTail($verified); + $anchor = null; + $pages = $source->pages($record->count()); + $page = $pages->current(); - return; + continue; } $anchor = null; - $releases = \array_slice($releases, 1); + $page = new ReleasePage(\array_slice($page->releases, 1), $page->last, $page->skipped); + } + + if ($page === null) { + break; } $new = \array_values(\array_filter( - $releases, + $page->releases, static fn(ReleaseRecord $release): bool => !$record->has($release->tag), )); @@ -237,6 +246,9 @@ private function extend( static fn(ReleaseRecord $release): bool => !$served->has($release->tag), ))); $visible === [] or yield $visible; + + $pages->next(); + $page = $pages->current(); } $record->complete or $this->persist($record->withComplete(true)); diff --git a/src/Module/Repository/Internal/GitHub/GitHubReleaseSource.php b/src/Module/Repository/Internal/GitHub/GitHubReleaseSource.php deleted file mode 100644 index 1169b064..00000000 --- a/src/Module/Repository/Internal/GitHub/GitHubReleaseSource.php +++ /dev/null @@ -1,79 +0,0 @@ - */ - private int $perPage = RepositoryApi::RELEASES_PER_PAGE; - - public function __construct( - private readonly RepositoryApi $api, - ) {} - - public function pages(int $offset = 0): \Generator - { - $first = $this->pageOf($offset); - $pages = $this->api->releasePages($first); - $page = $pages->current(); - - // A page followed by another one holds exactly as many releases as the server serves per - // page; the offset may then lie on a later page than the one fetched - if ($page !== null && !$page->last && $this->learn($page) && $this->pageOf($offset) !== $first) { - $pages = $this->api->releasePages($this->pageOf($offset)); - $page = $pages->current(); - } - - // The generator is walked by hand: one that ended before its first page is closed, and - // `foreach` cannot start it over - $skip = $offset % $this->perPage; - while ($page !== null) { - yield $skip === 0 ? $page : new ReleasePage(\array_slice($page->releases, $skip), $page->last, $page->skipped); - $skip = 0; - - $pages->next(); - $page = $pages->current(); - } - } - - /** - * Takes the size of a full page as the page size; tells whether it changed. - */ - private function learn(ReleasePage $page): bool - { - $size = \count($page->releases) + $page->skipped; - if ($size < 1 || $size === $this->perPage) { - return false; - } - - $this->perPage = $size; - - return true; - } - - /** - * @param int<0, max> $offset - * @return int<1, max> - */ - private function pageOf(int $offset): int - { - /** @var int<1, max> */ - return \intdiv($offset, $this->perPage) + 1; - } -} diff --git a/src/Module/Repository/Internal/GitHub/GitHubRepository.php b/src/Module/Repository/Internal/GitHub/GitHubRepository.php index bd78ceb5..2a96e207 100644 --- a/src/Module/Repository/Internal/GitHub/GitHubRepository.php +++ b/src/Module/Repository/Internal/GitHub/GitHubRepository.php @@ -5,6 +5,7 @@ namespace Internal\DLoad\Module\Repository\Internal\GitHub; use Internal\Destroy\Destroyable; +use Internal\DLoad\Module\Registry\Internal\PagedReleaseSource; use Internal\DLoad\Module\Registry\RepositoryId; use Internal\DLoad\Module\Registry\VersionRegistry; use Internal\DLoad\Module\Repository\Collection\ReleasesCollection; @@ -71,7 +72,7 @@ public function getReleases(): ReleasesCollection $pages = $this->registry->releases( $this->getId(), - new GitHubReleaseSource($this->api), + new PagedReleaseSource($this->api->releasePages(...), RepositoryApi::RELEASES_PER_PAGE), ); $anyPageLoaded = false; diff --git a/src/Module/Repository/Internal/GitLab/GitLabReleaseSource.php b/src/Module/Repository/Internal/GitLab/GitLabReleaseSource.php deleted file mode 100644 index 3c9dfdec..00000000 --- a/src/Module/Repository/Internal/GitLab/GitLabReleaseSource.php +++ /dev/null @@ -1,79 +0,0 @@ - */ - private int $perPage = RepositoryApi::RELEASES_PER_PAGE; - - public function __construct( - private readonly RepositoryApi $api, - ) {} - - public function pages(int $offset = 0): \Generator - { - $first = $this->pageOf($offset); - $pages = $this->api->releasePages($first); - $page = $pages->current(); - - // A page followed by another one holds exactly as many releases as the server serves per - // page; the offset may then lie on a later page than the one fetched - if ($page !== null && !$page->last && $this->learn($page) && $this->pageOf($offset) !== $first) { - $pages = $this->api->releasePages($this->pageOf($offset)); - $page = $pages->current(); - } - - // The generator is walked by hand: one that ended before its first page is closed, and - // `foreach` cannot start it over - $skip = $offset % $this->perPage; - while ($page !== null) { - yield $skip === 0 ? $page : new ReleasePage(\array_slice($page->releases, $skip), $page->last, $page->skipped); - $skip = 0; - - $pages->next(); - $page = $pages->current(); - } - } - - /** - * Takes the size of a full page as the page size; tells whether it changed. - */ - private function learn(ReleasePage $page): bool - { - $size = \count($page->releases) + $page->skipped; - if ($size < 1 || $size === $this->perPage) { - return false; - } - - $this->perPage = $size; - - return true; - } - - /** - * @param int<0, max> $offset - * @return int<1, max> - */ - private function pageOf(int $offset): int - { - /** @var int<1, max> */ - return \intdiv($offset, $this->perPage) + 1; - } -} diff --git a/src/Module/Repository/Internal/GitLab/GitLabRepository.php b/src/Module/Repository/Internal/GitLab/GitLabRepository.php index 27b8cd66..eca64a31 100644 --- a/src/Module/Repository/Internal/GitLab/GitLabRepository.php +++ b/src/Module/Repository/Internal/GitLab/GitLabRepository.php @@ -5,6 +5,7 @@ namespace Internal\DLoad\Module\Repository\Internal\GitLab; use Internal\Destroy\Destroyable; +use Internal\DLoad\Module\Registry\Internal\PagedReleaseSource; use Internal\DLoad\Module\Registry\RepositoryId; use Internal\DLoad\Module\Registry\VersionRegistry; use Internal\DLoad\Module\Repository\Collection\ReleasesCollection; @@ -69,7 +70,7 @@ public function getReleases(): ReleasesCollection $pages = $this->registry->releases( $this->getId(), - new GitLabReleaseSource($this->api), + new PagedReleaseSource($this->api->releasePages(...), RepositoryApi::RELEASES_PER_PAGE), ); $anyPageLoaded = false; diff --git a/tests/Unit/Module/Registry/StoredVersionRegistryTest.php b/tests/Unit/Module/Registry/StoredVersionRegistryTest.php index 6852a0f3..081faee8 100644 --- a/tests/Unit/Module/Registry/StoredVersionRegistryTest.php +++ b/tests/Unit/Module/Registry/StoredVersionRegistryTest.php @@ -241,7 +241,7 @@ public function releasesPublishedWithinTheTtlOverlapTheTailWithoutRepeating(): v } #[Test] - public function failedCheckLeavesTheStoredTailUnconfirmed(): void + public function failedCheckLeavesTheStoredTailAsItIs(): void { $source = ArrayReleaseSource::ofTags(['v6', 'v5', 'v4', 'v3', 'v2', 'v1']); self::take($this->registry()->releases($this->id, $source), 4); @@ -252,10 +252,57 @@ public function failedCheckLeavesTheStoredTailUnconfirmed(): void $pages = $this->registry()->releases($this->id, $source); Assert::same(self::tagsOf($pages->current()), ['v6', 'v5', 'v4', 'v3']); - // The source is back: the tail is confirmed from its last stored release on + // The source is back: nothing was compared, so nothing is suspected and the tail goes + // on without a confirmation, which a rate limit could not afford anyway $source->failure = null; Assert::same(self::flatten($pages), ['v6', 'v5', 'v4', 'v3', 'v2', 'v1']); - Assert::same($source->served, [2, 4]); + Assert::same($source->served, [4]); + } + + #[Test] + public function anchorBeyondTheEndOfTheListingReloadsTheTail(): void + { + $source = ArrayReleaseSource::ofTags(['v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + self::take($this->registry()->releases($this->id, $source), 6); + $source->served = []; + + // Three releases below page 1 are gone: the last stored one would sit past the end now, + // and the source has no page to answer with + $source->delete('v4'); + $source->delete('v3'); + $source->delete('v2'); + $this->now += 601; + $again = self::flatten($this->registry()->releases($this->id, $source)); + + Assert::same($again, ['v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v1']); + Assert::same($source->served, [0, 4, 2, 4]); + Assert::same(self::tagsOf($this->storage->load($this->id)->releases()), ['v8', 'v7', 'v6', 'v5', 'v1']); + } + + #[Test] + public function failureWhileReloadingTheTailKeepsTheStoredReleases(): void + { + $source = ArrayReleaseSource::ofTags(['v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + self::take($this->registry()->releases($this->id, $source), 4); + $source->served = []; + + $source->delete('v3'); + $this->now += 601; + $pages = $this->registry()->releases($this->id, $source); + $pages->current(); + + // The anchor misses, and the source fails on the very next page + $source->served = []; + $source->failAfter = 1; + try { + $pages->next(); + Assert::fail('The failure of the reload must reach the caller.'); + } catch (ApiException) { + } + + // Nothing was dropped: the stored list serves the next run under the same failure + Assert::same($source->served, [2]); + Assert::same(self::tagsOf($this->storage->load($this->id)->releases()), ['v6', 'v5', 'v4', 'v3']); } #[Test] diff --git a/tests/Unit/Module/Registry/Stub/ArrayReleaseSource.php b/tests/Unit/Module/Registry/Stub/ArrayReleaseSource.php index d1e1921b..dba70032 100644 --- a/tests/Unit/Module/Registry/Stub/ArrayReleaseSource.php +++ b/tests/Unit/Module/Registry/Stub/ArrayReleaseSource.php @@ -27,6 +27,9 @@ final class ArrayReleaseSource implements ReleaseSource /** When set, every page request fails with this exception. */ public ?\Throwable $failure = null; + /** Number of pages served before every further request fails. */ + public int $failAfter = \PHP_INT_MAX; + /** * Tags of releases the source cannot read: they are left out of the pages and counted as skipped. * @@ -87,9 +90,16 @@ public function pages(int $offset = 0): \Generator do { $this->failure === null or throw $this->failure; + \count($this->served) < $this->failAfter or throw new ApiException('API is unavailable.', 'stub/stub'); $this->served[] = $page * $this->perPage; $items = \array_slice($this->releases, $page * $this->perPage, $this->perPage); + + // Like a real API, a page past the end of the listing is no page at all + if ($items === [] && $page > 0) { + return; + } + $last = ($page + 1) * $this->perPage >= \count($this->releases); $readable = \array_values(\array_filter( $items, diff --git a/tests/Unit/Module/Repository/Internal/GitHub/GitHubRepositoryTest.php b/tests/Unit/Module/Repository/Internal/GitHub/GitHubRepositoryTest.php index a942a494..c7f6763c 100644 --- a/tests/Unit/Module/Repository/Internal/GitHub/GitHubRepositoryTest.php +++ b/tests/Unit/Module/Repository/Internal/GitHub/GitHubRepositoryTest.php @@ -5,6 +5,7 @@ namespace Internal\DLoad\Tests\Unit\Module\Repository\Internal\GitHub; use Internal\DLoad\Module\HttpClient\Internal\NyholmFactoryImpl; +use Internal\DLoad\Module\Registry\Internal\PagedReleaseSource; use Internal\DLoad\Module\Registry\Internal\PassThroughRegistry; use Internal\DLoad\Module\Registry\Internal\StoredVersionRegistry; use Internal\DLoad\Module\Registry\Record\ReleaseRecord; @@ -14,7 +15,6 @@ use Internal\DLoad\Module\Repository\Exception\RateLimitException; use Internal\DLoad\Module\Repository\Internal\GitHub\Api\Client; use Internal\DLoad\Module\Repository\Internal\GitHub\Api\RepositoryApi; -use Internal\DLoad\Module\Repository\Internal\GitHub\GitHubReleaseSource; use Internal\DLoad\Module\Repository\Internal\GitHub\GitHubRepository; use Internal\DLoad\Module\Repository\ReleaseInterface; use Internal\DLoad\Service\Logger; @@ -27,7 +27,7 @@ use Testo\Test; #[Covers(GitHubRepository::class)] -#[Covers(GitHubReleaseSource::class)] +#[Covers(PagedReleaseSource::class)] final class GitHubRepositoryTest { #[Test] @@ -101,6 +101,55 @@ public function secondRunIsServedFromTheRegistryWithoutRequests(): void Assert::same(\count($secondRun), 200); } + #[Test] + public function shortPageBelowTheFirstOneDoesNotChangeThePageSize(): void + { + // The first run stores two full pages + $storage = new InMemoryRegistryStorage(); + $firstClient = new PagedClientStub(pages: 4); + $seen = 0; + foreach (self::createRepository($firstClient, self::registry($storage))->getReleases() as $release) { + unset($release); + if (++$seen === 150) { + break; + } + } + + // Page 3 comes back one release short, as a server filtering after paging answers; the + // fresh record is extended from the stored count and page 4 keeps its position + $secondClient = new PagedClientStub(pages: 4, withheld: ['v1.0.250']); + $all = self::names(self::createRepository($secondClient, self::registry($storage))); + + Assert::same($secondClient->requestedPages(), [3, 4]); + Assert::same(\count($all), 399); + Assert::true(\in_array('v1.0.201', $all, true)); + Assert::true(\in_array('v1.0.300', $all, true)); + } + + #[Test] + public function unreadableReleaseAboveTheTailDoesNotMakeEveryRunReloadIt(): void + { + // One release of page 1 cannot be read from the start; the first run stores two pages + $storage = new InMemoryRegistryStorage(); + $firstClient = new PagedClientStub(pages: 4, broken: 1); + $seen = 0; + foreach (self::createRepository($firstClient, self::registry($storage))->getReleases() as $release) { + unset($release); + if (++$seen === 150) { + break; + } + } + + // A check that bails out on the unreadable release leaves the record as it is and does + // not go looking for a shift it could not tell from the missing release + $secondClient = new PagedClientStub(pages: 4, broken: 1); + $all = self::names(self::createRepository($secondClient, self::registry($storage, refresh: true))); + + Assert::same($secondClient->requestedPages(), [1, 2, 3, 4]); + Assert::same(\count($all), 399); + Assert::same(\count(\array_unique($all)), 399); + } + #[Test] public function olderReleasesAreLoadedFromTheApiWhenTheRegistryRunsOut(): void { @@ -145,7 +194,7 @@ public function draftReleasesAreNeitherServedNorStored(): void #[Test] public function unreadableReleasesAreCountedOnThePage(): void { - $page = (new GitHubReleaseSource(self::api(new PagedClientStub(pages: 1, broken: 2))))->pages()->current(); + $page = self::source(new PagedClientStub(pages: 1, broken: 2))->pages()->current(); Assert::same(\count($page->releases), 98); Assert::same($page->skipped, 2); @@ -188,7 +237,7 @@ public function repositoryWithoutReleasesListsNothing(): void Assert::same(self::names(self::createRepository($client, self::registry(new InMemoryRegistryStorage()))), []); Assert::same($client->requestedPages(), [1]); - Assert::same(\iterator_to_array((new GitHubReleaseSource(self::api($client)))->pages(150), false), []); + Assert::same(\iterator_to_array(self::source($client)->pages(150), false), []); } #[Test] @@ -342,9 +391,14 @@ private static function api(PagedClientStub $client): RepositoryApi ); } - private static function registry(InMemoryRegistryStorage $storage): StoredVersionRegistry + private static function source(PagedClientStub $client): PagedReleaseSource + { + return new PagedReleaseSource(self::api($client)->releasePages(...), RepositoryApi::RELEASES_PER_PAGE); + } + + private static function registry(InMemoryRegistryStorage $storage, bool $refresh = false): StoredVersionRegistry { - return new StoredVersionRegistry($storage, 600, new Logger()); + return new StoredVersionRegistry($storage, 600, new Logger(), $refresh); } /** diff --git a/tests/Unit/Module/Repository/Internal/GitHub/Stub/PagedClientStub.php b/tests/Unit/Module/Repository/Internal/GitHub/Stub/PagedClientStub.php index 51895a5b..f349c1c0 100644 --- a/tests/Unit/Module/Repository/Internal/GitHub/Stub/PagedClientStub.php +++ b/tests/Unit/Module/Repository/Internal/GitHub/Stub/PagedClientStub.php @@ -31,6 +31,7 @@ final class PagedClientStub implements ClientInterface * @param int<0, max> $drafts Draft releases listed on top, as the API shows them to the token holder. * @param int<0, max> $broken Number of the newest published releases that cannot be decoded. * @param int<1, max>|null $perPageLimit Greatest number of releases served per page whatever `per_page` asks for. + * @param list $withheld Tags left out of a page after it was cut, as a server filtering after paging does. */ public function __construct( private readonly int $pages = 1, @@ -38,6 +39,7 @@ public function __construct( private readonly int $drafts = 0, private readonly int $broken = 0, private readonly ?int $perPageLimit = null, + private readonly array $withheld = [], ) {} public function sendRequest(RequestInterface $request): ResponseInterface @@ -59,6 +61,11 @@ public function sendRequest(RequestInterface $request): ResponseInterface ? ['link' => [\sprintf('; rel="next"', $page + 1)]] : []; + $releases = \array_values(\array_filter( + $releases, + fn(array $release): bool => !\in_array($release['name'], $this->withheld, true), + )); + return new ResponseStub(200, $headers, \json_encode($releases)); } diff --git a/tests/Unit/Module/Repository/Internal/GitLab/GitLabRepositoryTest.php b/tests/Unit/Module/Repository/Internal/GitLab/GitLabRepositoryTest.php index 731938fe..af362276 100644 --- a/tests/Unit/Module/Repository/Internal/GitLab/GitLabRepositoryTest.php +++ b/tests/Unit/Module/Repository/Internal/GitLab/GitLabRepositoryTest.php @@ -5,6 +5,7 @@ namespace Internal\DLoad\Tests\Unit\Module\Repository\Internal\GitLab; use Internal\DLoad\Module\HttpClient\Internal\NyholmFactoryImpl; +use Internal\DLoad\Module\Registry\Internal\PagedReleaseSource; use Internal\DLoad\Module\Registry\Internal\PassThroughRegistry; use Internal\DLoad\Module\Registry\Internal\StoredVersionRegistry; use Internal\DLoad\Module\Registry\Record\AssetRecord; @@ -16,7 +17,6 @@ use Internal\DLoad\Module\Repository\Internal\GitLab\Api\Client; use Internal\DLoad\Module\Repository\Internal\GitLab\Api\RepositoryApi; use Internal\DLoad\Module\Repository\Internal\GitLab\GitLabAsset; -use Internal\DLoad\Module\Repository\Internal\GitLab\GitLabReleaseSource; use Internal\DLoad\Module\Repository\Internal\GitLab\GitLabRepository; use Internal\DLoad\Module\Repository\ReleaseInterface; use Internal\DLoad\Service\Logger; @@ -29,7 +29,7 @@ use Testo\Test; #[Covers(GitLabRepository::class)] -#[Covers(GitLabReleaseSource::class)] +#[Covers(PagedReleaseSource::class)] #[Covers(GitLabAsset::class)] final class GitLabRepositoryTest { @@ -313,11 +313,13 @@ private static function registry(InMemoryRegistryStorage $storage): StoredVersio return new StoredVersionRegistry($storage, 600, new Logger()); } - private static function source(PagedClientStub $client): GitLabReleaseSource + private static function source(PagedClientStub $client): PagedReleaseSource { $httpFactory = new NyholmFactoryImpl(new Logger()); - return new GitLabReleaseSource(new RepositoryApi(new Client($httpFactory, $client), $httpFactory, 'group/project')); + $api = new RepositoryApi(new Client($httpFactory, $client), $httpFactory, 'group/project'); + + return new PagedReleaseSource($api->releasePages(...), RepositoryApi::RELEASES_PER_PAGE); } /** From d417b31af7d0e66f1325818bc4a38eb0fe0b2a6e Mon Sep 17 00:00:00 2001 From: roxblnfk Date: Mon, 12 Oct 2026 14:27:10 +0400 Subject: [PATCH 3/5] fix(registry): confirm the stored tail after a failed or aborted check fix(repository): count unreadable releases among the skipped positions refactor(repository): own the paged release source A check that failed or bailed out on an unreadable release compared nothing, so taking the tail as confirmed let a deletion below the head lose a release for good; the confirmation lies on the page the tail needs anyway, and a dropped tail is written only with its replacement, so skipping it spared nothing. An unreadable release holds a listing position but is missing from a page's list, so the check reports such positions for the anchor, and the source no longer drops a readable release behind them when it skips into a page. The registry knows nothing of pages, so the paged source belongs to the repository module; its page size is learned per instance, which rules out servers serving more releases per page than asked for, as GitHub and GitLab never do. Assisted-By: Claude Fable 5.1 --- .../Internal/StoredVersionRegistry.php | 31 +-- .../Internal/GitHub/GitHubRepository.php | 2 +- .../Internal/GitLab/GitLabRepository.php | 2 +- .../Internal/PagedReleaseSource.php | 33 +++- .../Registry/StoredVersionRegistryTest.php | 59 +++++- .../Registry/Stub/ArrayReleaseSource.php | 7 +- .../Internal/GitHub/GitHubRepositoryTest.php | 2 +- .../Internal/GitLab/GitLabRepositoryTest.php | 2 +- .../Internal/PagedReleaseSourceTest.php | 179 ++++++++++++++++++ 9 files changed, 287 insertions(+), 30 deletions(-) rename src/Module/{Registry => Repository}/Internal/PagedReleaseSource.php (61%) create mode 100644 tests/Unit/Module/Repository/Internal/PagedReleaseSourceTest.php diff --git a/src/Module/Registry/Internal/StoredVersionRegistry.php b/src/Module/Registry/Internal/StoredVersionRegistry.php index 3f10481d..2d7b40c8 100644 --- a/src/Module/Registry/Internal/StoredVersionRegistry.php +++ b/src/Module/Registry/Internal/StoredVersionRegistry.php @@ -27,7 +27,7 @@ * 2. **Serve.** The stored releases are yielded segment by segment without any request. * 3. **Extend.** When the consumer runs past the stored releases and the listing is not known * to be complete, older pages are fetched one by one and appended to the record. After a - * check, the stored releases below the checked ones are first confirmed to be where they + * check, the stored releases below the ones it confirmed are first found to be where they * were, as a deletion among them would shift the listing. * * A check that fails while releases are stored falls back to the stored ones: an outage or a rate @@ -65,8 +65,9 @@ public function releases(RepositoryId $id, ReleaseSource $source): \Generator // Within the TTL the stored list is taken as it is, head and tail alike $verified = $record->count(); + $unreadable = 0; if ($this->refresh || $record->isStale(($this->clock)(), $this->ttl)) { - [$record, $verified] = $this->check($record, $source); + [$record, $verified, $unreadable] = $this->check($record, $source); } else { $this->logger->debug('Releases of `%s` are served from the version registry.', (string) $id); } @@ -80,7 +81,7 @@ public function releases(RepositoryId $id, ReleaseSource $source): \Generator } // Older releases are loaded only when the consumer actually needs them - yield from $this->extend($record, $source, $verified); + yield from $this->extend($record, $source, $verified, $unreadable); } public function attach(RepositoryId $id, string $software): void @@ -121,20 +122,24 @@ private static function hasKnown(RepositoryRecord $record, array $page): bool /** * Fetches the releases published since the last check and stores the result. * - * Besides the record, tells how many of its leading releases the check has seen in place: - * the stored releases beyond them may have shifted since they were stored. A check that - * leaves the record as it is vouches for all of it, as it has nothing to compare against. + * Besides the record, tells how many of its leading releases the check has seen in place, + * as the stored releases beyond them may have shifted since they were stored, and how many + * listing positions among the pages it fetched hold a release the record cannot account + * for, as an unreadable one. A check that leaves the record untouched confirms nothing. * - * @return array{RepositoryRecord, int<0, max>} + * @return array{RepositoryRecord, int<0, max>, int<0, max>} * @throws RepositoryException When the source fails and nothing is stored to fall back on. */ private function check(RepositoryRecord $record, ReleaseSource $source): array { try { $fetched = []; + $unreadable = 0; $complete = $record->complete; foreach ($source->pages() as $page) { + $unreadable += $page->skipped; + // A release the source could not read is missing from the page exactly like a // deleted one, and `withHead()` would drop it; the stored list stays untouched // and the next run checks again @@ -146,7 +151,7 @@ private function check(RepositoryRecord $record, ReleaseSource $source): array $record->count(), ); - return [$record, $record->count()]; + return [$record, 0, $unreadable]; } $fetched = [...$fetched, ...$page->releases]; @@ -171,13 +176,13 @@ private function check(RepositoryRecord $record, ReleaseSource $source): array ->withCheckedAt(($this->clock)()), ); - return [$record, \count($fetched)]; + return [$record, \count($fetched), 0]; } catch (RepositoryException $e) { $record->count() > 0 or throw $e; $this->report($e, $record); - return [$record, $record->count()]; + return [$record, 0, 0]; } } @@ -196,15 +201,17 @@ private function check(RepositoryRecord $record, ReleaseSource $source): array * request for the page that ends with the release. * * @param int<0, max> $verified Number of leading stored releases known to be in place. + * @param int<0, max> $unreadable Listing positions above the stored tail that hold no stored + * release, so the last stored release sits that many positions below the stored count. * @return \Generator, mixed, void> * @throws RepositoryException */ - private function extend(RepositoryRecord $record, ReleaseSource $source, int $verified): \Generator + private function extend(RepositoryRecord $record, ReleaseSource $source, int $verified, int $unreadable): \Generator { $served = $record; $anchor = $verified < $record->count() ? $record->lastTag() : null; /** @var int<0, max> $offset */ - $offset = $anchor === null ? $record->count() : $record->count() - 1; + $offset = $record->count() + $unreadable - ($anchor === null ? 0 : 1); $pages = $source->pages($offset); $page = $pages->current(); diff --git a/src/Module/Repository/Internal/GitHub/GitHubRepository.php b/src/Module/Repository/Internal/GitHub/GitHubRepository.php index 2a96e207..0d689b8f 100644 --- a/src/Module/Repository/Internal/GitHub/GitHubRepository.php +++ b/src/Module/Repository/Internal/GitHub/GitHubRepository.php @@ -5,12 +5,12 @@ namespace Internal\DLoad\Module\Repository\Internal\GitHub; use Internal\Destroy\Destroyable; -use Internal\DLoad\Module\Registry\Internal\PagedReleaseSource; use Internal\DLoad\Module\Registry\RepositoryId; use Internal\DLoad\Module\Registry\VersionRegistry; use Internal\DLoad\Module\Repository\Collection\ReleasesCollection; use Internal\DLoad\Module\Repository\Exception\RateLimitException; use Internal\DLoad\Module\Repository\Internal\GitHub\Api\RepositoryApi; +use Internal\DLoad\Module\Repository\Internal\PagedReleaseSource; use Internal\DLoad\Module\Repository\Internal\Paginator; use Internal\DLoad\Module\Repository\Repository; use Internal\DLoad\Service\Logger; diff --git a/src/Module/Repository/Internal/GitLab/GitLabRepository.php b/src/Module/Repository/Internal/GitLab/GitLabRepository.php index eca64a31..753af6ff 100644 --- a/src/Module/Repository/Internal/GitLab/GitLabRepository.php +++ b/src/Module/Repository/Internal/GitLab/GitLabRepository.php @@ -5,12 +5,12 @@ namespace Internal\DLoad\Module\Repository\Internal\GitLab; use Internal\Destroy\Destroyable; -use Internal\DLoad\Module\Registry\Internal\PagedReleaseSource; use Internal\DLoad\Module\Registry\RepositoryId; use Internal\DLoad\Module\Registry\VersionRegistry; use Internal\DLoad\Module\Repository\Collection\ReleasesCollection; use Internal\DLoad\Module\Repository\Exception\RateLimitException; use Internal\DLoad\Module\Repository\Internal\GitLab\Api\RepositoryApi; +use Internal\DLoad\Module\Repository\Internal\PagedReleaseSource; use Internal\DLoad\Module\Repository\Internal\Paginator; use Internal\DLoad\Module\Repository\Repository; use Internal\DLoad\Service\Logger; diff --git a/src/Module/Registry/Internal/PagedReleaseSource.php b/src/Module/Repository/Internal/PagedReleaseSource.php similarity index 61% rename from src/Module/Registry/Internal/PagedReleaseSource.php rename to src/Module/Repository/Internal/PagedReleaseSource.php index 3e892cfc..e74cf08b 100644 --- a/src/Module/Registry/Internal/PagedReleaseSource.php +++ b/src/Module/Repository/Internal/PagedReleaseSource.php @@ -2,7 +2,7 @@ declare(strict_types=1); -namespace Internal\DLoad\Module\Registry\Internal; +namespace Internal\DLoad\Module\Repository\Internal; use Internal\DLoad\Module\Registry\Record\ReleasePage; use Internal\DLoad\Module\Registry\ReleaseSource; @@ -13,12 +13,17 @@ * * Translates a release offset into the page number and the number of releases to drop from * that page. The page size is the one the API is asked for, until the first page shows that - * the server serves another number of releases per page, as a mirror or a server that caps - * `per_page` does. Only the first page can tell: it is the one page whose start is known, so - * its length is the page size and not the sign of a release filtered out after paging. + * the server serves fewer releases per page, as a mirror or a server that caps `per_page` does. + * Only the first page can tell: it is the one page whose start is known, so its length is the + * page size and not the sign of a release filtered out after paging. + * + * The page size is learned per instance, not stored: the registry maps offsets through the + * source and knows nothing of pages. So a server capping `per_page` costs the first page again + * on every run that does not fetch it anyway, and a server serving more releases per page than + * asked for is not supported: GitHub and GitLab both stop at the number asked for. * * @internal - * @psalm-internal Internal\DLoad + * @psalm-internal Internal\DLoad\Module\Repository */ final class PagedReleaseSource implements ReleaseSource { @@ -55,7 +60,7 @@ public function pages(int $offset = 0): \Generator // `foreach` cannot start it over $skip = $offset % $this->perPage; while ($page !== null) { - yield $skip === 0 ? $page : new ReleasePage(\array_slice($page->releases, $skip), $page->last, $page->skipped); + yield $skip === 0 ? $page : self::from($page, $skip); $skip = 0; $pages->next(); @@ -63,13 +68,27 @@ public function pages(int $offset = 0): \Generator } } + /** + * Drops the releases before the given position of the page. + * + * The unreadable releases of a page are not in its list, yet they hold positions on it. + * They are taken as lying before the position: when one lies after it instead, a release is + * repeated rather than lost, and the registry drops repeated ones. + * + * @param int<0, max> $skip + */ + private static function from(ReleasePage $page, int $skip): ReleasePage + { + return new ReleasePage(\array_slice($page->releases, \max(0, $skip - $page->skipped)), $page->last, $page->skipped); + } + /** * Takes the length of a page followed by another one as the page size; tells whether it changed. */ private function learn(ReleasePage $page): bool { $size = \count($page->releases) + $page->skipped; - if ($size < 1 || $size === $this->perPage) { + if ($size < 1 || $size >= $this->perPage) { return false; } diff --git a/tests/Unit/Module/Registry/StoredVersionRegistryTest.php b/tests/Unit/Module/Registry/StoredVersionRegistryTest.php index 081faee8..2be5c0b4 100644 --- a/tests/Unit/Module/Registry/StoredVersionRegistryTest.php +++ b/tests/Unit/Module/Registry/StoredVersionRegistryTest.php @@ -241,7 +241,7 @@ public function releasesPublishedWithinTheTtlOverlapTheTailWithoutRepeating(): v } #[Test] - public function failedCheckLeavesTheStoredTailAsItIs(): void + public function failedCheckConfirmsNothingAboutTheStoredTail(): void { $source = ArrayReleaseSource::ofTags(['v6', 'v5', 'v4', 'v3', 'v2', 'v1']); self::take($this->registry()->releases($this->id, $source), 4); @@ -252,11 +252,62 @@ public function failedCheckLeavesTheStoredTailAsItIs(): void $pages = $this->registry()->releases($this->id, $source); Assert::same(self::tagsOf($pages->current()), ['v6', 'v5', 'v4', 'v3']); - // The source is back: nothing was compared, so nothing is suspected and the tail goes - // on without a confirmation, which a rate limit could not afford anyway + // The source is back: the check compared nothing, so the tail is confirmed from its + // last stored release on, which lies on the page needed anyway $source->failure = null; Assert::same(self::flatten($pages), ['v6', 'v5', 'v4', 'v3', 'v2', 'v1']); - Assert::same($source->served, [4]); + Assert::same($source->served, [2, 4]); + } + + #[Test] + public function releaseDeletedBelowTheHeadIsNoticedAfterAFailedCheck(): void + { + $source = ArrayReleaseSource::ofTags(['v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + self::take($this->registry()->releases($this->id, $source), 6); + $source->served = []; + + // `v3` is deleted upstream while the check cannot reach the source + $source->delete('v3'); + $source->fail(); + $this->now += 601; + $pages = $this->registry()->releases($this->id, $source); + $pages->current(); + + $source->failure = null; + Assert::same(self::flatten($pages), ['v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + Assert::same($source->served, [4, 0, 2, 4, 6]); + + // Neither a lost release nor a ghost is left behind + Assert::same(self::tagsOf($this->storage->load($this->id)->releases()), ['v8', 'v7', 'v6', 'v5', 'v4', 'v2', 'v1']); + } + + #[Test] + public function unreadableReleaseOnTheFirstPageDoesNotHideADeletionInTheTail(): void + { + $source = ArrayReleaseSource::ofTags(['v10', 'v9', 'v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + $source->unreadable = ['v9']; + self::take($this->registry()->releases($this->id, $source), 5); + Assert::same(self::tagsOf($this->storage->load($this->id)->releases()), ['v10', 'v8', 'v7', 'v6', 'v5']); + $source->served = []; + + // The check bails out on `v9` and leaves the record as it is; the stored `v5` is one + // position further down the listing than the stored count says, and is found there + $this->now += 601; + $pages = $this->registry()->releases($this->id, $source); + self::take($pages, 7); + + Assert::same($source->served, [0, 4, 6]); + Assert::same(self::tagsOf($this->storage->load($this->id)->releases()), ['v10', 'v8', 'v7', 'v6', 'v5', 'v4', 'v3']); + $source->served = []; + + // Now `v5` is deleted upstream too: the anchor misses and the tail is loaded again + $source->delete('v5'); + $this->now += 601; + $again = self::flatten($this->registry()->releases($this->id, $source)); + + Assert::same($again, ['v10', 'v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + Assert::same($source->served, [0, 6, 0, 2, 4, 6, 8]); + Assert::same(self::tagsOf($this->storage->load($this->id)->releases()), ['v10', 'v8', 'v7', 'v6', 'v4', 'v3', 'v2', 'v1']); } #[Test] diff --git a/tests/Unit/Module/Registry/Stub/ArrayReleaseSource.php b/tests/Unit/Module/Registry/Stub/ArrayReleaseSource.php index dba70032..58b27fdc 100644 --- a/tests/Unit/Module/Registry/Stub/ArrayReleaseSource.php +++ b/tests/Unit/Module/Registry/Stub/ArrayReleaseSource.php @@ -101,12 +101,13 @@ public function pages(int $offset = 0): \Generator } $last = ($page + 1) * $this->perPage >= \count($this->releases); - $readable = \array_values(\array_filter( - $items, + $readable = fn(array $releases): array => \array_values(\array_filter( + $releases, fn(ReleaseRecord $release): bool => !\in_array($release->tag, $this->unreadable, true), )); - yield new ReleasePage(\array_slice($readable, $skip), $last, \count($items) - \count($readable)); + // Unreadable releases hold their positions on the page, so the offset counts them + yield new ReleasePage($readable(\array_slice($items, $skip)), $last, \count($items) - \count($readable($items))); $skip = 0; ++$page; diff --git a/tests/Unit/Module/Repository/Internal/GitHub/GitHubRepositoryTest.php b/tests/Unit/Module/Repository/Internal/GitHub/GitHubRepositoryTest.php index c7f6763c..4867f62b 100644 --- a/tests/Unit/Module/Repository/Internal/GitHub/GitHubRepositoryTest.php +++ b/tests/Unit/Module/Repository/Internal/GitHub/GitHubRepositoryTest.php @@ -5,7 +5,6 @@ namespace Internal\DLoad\Tests\Unit\Module\Repository\Internal\GitHub; use Internal\DLoad\Module\HttpClient\Internal\NyholmFactoryImpl; -use Internal\DLoad\Module\Registry\Internal\PagedReleaseSource; use Internal\DLoad\Module\Registry\Internal\PassThroughRegistry; use Internal\DLoad\Module\Registry\Internal\StoredVersionRegistry; use Internal\DLoad\Module\Registry\Record\ReleaseRecord; @@ -16,6 +15,7 @@ use Internal\DLoad\Module\Repository\Internal\GitHub\Api\Client; use Internal\DLoad\Module\Repository\Internal\GitHub\Api\RepositoryApi; use Internal\DLoad\Module\Repository\Internal\GitHub\GitHubRepository; +use Internal\DLoad\Module\Repository\Internal\PagedReleaseSource; use Internal\DLoad\Module\Repository\ReleaseInterface; use Internal\DLoad\Service\Logger; use Internal\DLoad\Tests\Unit\Module\Registry\Stub\InMemoryRegistryStorage; diff --git a/tests/Unit/Module/Repository/Internal/GitLab/GitLabRepositoryTest.php b/tests/Unit/Module/Repository/Internal/GitLab/GitLabRepositoryTest.php index af362276..49c806d5 100644 --- a/tests/Unit/Module/Repository/Internal/GitLab/GitLabRepositoryTest.php +++ b/tests/Unit/Module/Repository/Internal/GitLab/GitLabRepositoryTest.php @@ -5,7 +5,6 @@ namespace Internal\DLoad\Tests\Unit\Module\Repository\Internal\GitLab; use Internal\DLoad\Module\HttpClient\Internal\NyholmFactoryImpl; -use Internal\DLoad\Module\Registry\Internal\PagedReleaseSource; use Internal\DLoad\Module\Registry\Internal\PassThroughRegistry; use Internal\DLoad\Module\Registry\Internal\StoredVersionRegistry; use Internal\DLoad\Module\Registry\Record\AssetRecord; @@ -18,6 +17,7 @@ use Internal\DLoad\Module\Repository\Internal\GitLab\Api\RepositoryApi; use Internal\DLoad\Module\Repository\Internal\GitLab\GitLabAsset; use Internal\DLoad\Module\Repository\Internal\GitLab\GitLabRepository; +use Internal\DLoad\Module\Repository\Internal\PagedReleaseSource; use Internal\DLoad\Module\Repository\ReleaseInterface; use Internal\DLoad\Service\Logger; use Internal\DLoad\Tests\Unit\Module\Registry\Stub\InMemoryRegistryStorage; diff --git a/tests/Unit/Module/Repository/Internal/PagedReleaseSourceTest.php b/tests/Unit/Module/Repository/Internal/PagedReleaseSourceTest.php new file mode 100644 index 00000000..51bfdfe1 --- /dev/null +++ b/tests/Unit/Module/Repository/Internal/PagedReleaseSourceTest.php @@ -0,0 +1,179 @@ + + */ + private array $requested = []; + + #[Test] + public function offsetIsMappedOntoThePageAndThePositionInIt(): void + { + $source = $this->source(total: 250, perPage: 100); + + $pages = \iterator_to_array($source->pages(150), false); + + Assert::same($this->requested, [2, 3]); + Assert::same(self::tagsOf($pages[0]), self::tags(150, 200)); + Assert::same(self::tagsOf($pages[1]), self::tags(200, 250)); + Assert::true($pages[1]->last); + } + + #[Test] + public function pagesAreRequestedOnlyWhenReached(): void + { + $source = $this->source(total: 250, perPage: 100); + + $pages = $source->pages(); + Assert::same($this->requested, []); + + $pages->current(); + Assert::same($this->requested, [1]); + + $pages->next(); + Assert::same($this->requested, [1, 2]); + } + + #[Test] + public function emptyListingYieldsNoPage(): void + { + $source = $this->source(total: 0, perPage: 100); + + Assert::same(\iterator_to_array($source->pages(), false), []); + Assert::same(\iterator_to_array($source->pages(150), false), []); + Assert::same($this->requested, [1, 2]); + } + + #[Test] + public function shortFirstPageTellsThePageSizeAndTheOffsetIsMappedAgain(): void + { + // The server puts two releases on a page whatever is asked for + $source = $this->source(total: 10, perPage: 100, served: 2); + + $pages = \iterator_to_array($source->pages(5), false); + + // Page 1 was fetched on the assumption of a hundred per page; it showed the real size + Assert::same($this->requested, [1, 3, 4, 5]); + Assert::same(self::tagsOf($pages[0]), self::tags(5, 6)); + Assert::same(self::tagsOf($pages[1]), self::tags(6, 8)); + + // The size learned serves the next listing of the same source + $this->requested = []; + \iterator_to_array($source->pages(8), false); + Assert::same($this->requested, [5]); + } + + #[Test] + public function shortPageBelowTheFirstOneIsNotTakenForThePageSize(): void + { + $source = $this->source(total: 300, perPage: 100, missing: ['v150']); + + $pages = \iterator_to_array($source->pages(200), false); + + Assert::same($this->requested, [3]); + Assert::same(self::tagsOf($pages[0]), self::tags(200, 300)); + } + + #[Test] + public function unreadableReleasesOfThePageCountTowardsTheSkippedPositions(): void + { + $source = $this->source(total: 300, perPage: 100, unreadable: ['v102']); + + $pages = \iterator_to_array($source->pages(105), false); + + // Position 105 is the fourth readable release of the page, not the fifth + Assert::same($this->requested, [2, 3]); + Assert::same(self::tagsOf($pages[0])[0], 'v105'); + Assert::same($pages[0]->skipped, 1); + + // An unreadable release after the position makes the page start one release early, + // which the registry filters as a known one, rather than lose a release + $source = $this->source(total: 300, perPage: 100, unreadable: ['v180']); + Assert::same(self::tagsOf(\iterator_to_array($source->pages(105), false)[0])[0], 'v104'); + } + + #[BeforeTest] + protected function prepare(): void + { + $this->requested = []; + } + + /** + * @return list + */ + private static function tags(int $from, int $to): array + { + return $to > $from ? \array_map(static fn(int $i): string => "v$i", \range($from, $to - 1)) : []; + } + + /** + * @return list + */ + private static function tagsOf(ReleasePage $page): array + { + return \array_map(static fn(ReleaseRecord $release): string => $release->tag, $page->releases); + } + + /** + * Source over a listing of `v0`..`v{total-1}`. + * + * @param int<0, max> $total + * @param int<1, max> $perPage Releases asked for per page. + * @param int<1, max>|null $served Releases the server puts on a page instead. + * @param list $missing Tags filtered out after paging. + * @param list $unreadable Tags the API cannot read. + */ + private function source(int $total, int $perPage, ?int $served = null, array $missing = [], array $unreadable = []): PagedReleaseSource + { + $size = $served ?? $perPage; + $pages = function (int $page) use ($total, $size, $missing, $unreadable): \Generator { + while (true) { + // Like a real API, a page past the end is requested and comes back empty + $this->requested[] = $page; + $slice = \array_slice(self::tags(0, $total), ($page - 1) * $size, $size); + if ($slice === []) { + return; + } + + $releases = []; + $skipped = 0; + foreach ($slice as $tag) { + if (\in_array($tag, $missing, true)) { + continue; + } + if (\in_array($tag, $unreadable, true)) { + ++$skipped; + continue; + } + $releases[] = new ReleaseRecord($tag, $tag); + } + + $last = $page * $size >= $total; + yield new ReleasePage($releases, $last, $skipped); + + if ($last) { + return; + } + ++$page; + } + }; + + return new PagedReleaseSource($pages, $perPage); + } +} From f6da8a3d48fe60309954603d758d06531be56052 Mon Sep 17 00:00:00 2001 From: roxblnfk Date: Mon, 12 Oct 2026 15:13:52 +0400 Subject: [PATCH 4/5] fix(registry): look for the last stored release on the page and the next one A release the record does not hold above the stored tail, an unreadable one or one published while the check could not store it, moves the last stored release down the listing, so expecting it exactly at the stored count took every such release for a deletion and reloaded the tail on every check. The count of unreadable releases the check saw could not tell those apart from a stored release that turned unreadable, so the position is searched for instead of computed. A shift down is bounded by what lies above the tail, less than a page in practice, while a shift up has no bound, so one further page is the place to look before giving the tail up. Assisted-By: Claude Fable 5.1 --- .../Internal/StoredVersionRegistry.php | 80 ++++++++++++------- .../Registry/StoredVersionRegistryTest.php | 59 ++++++++++++-- .../Internal/GitHub/GitHubRepositoryTest.php | 28 +++++++ .../Internal/GitHub/Stub/PagedClientStub.php | 4 +- 4 files changed, 135 insertions(+), 36 deletions(-) diff --git a/src/Module/Registry/Internal/StoredVersionRegistry.php b/src/Module/Registry/Internal/StoredVersionRegistry.php index 2d7b40c8..4198c161 100644 --- a/src/Module/Registry/Internal/StoredVersionRegistry.php +++ b/src/Module/Registry/Internal/StoredVersionRegistry.php @@ -65,9 +65,8 @@ public function releases(RepositoryId $id, ReleaseSource $source): \Generator // Within the TTL the stored list is taken as it is, head and tail alike $verified = $record->count(); - $unreadable = 0; if ($this->refresh || $record->isStale(($this->clock)(), $this->ttl)) { - [$record, $verified, $unreadable] = $this->check($record, $source); + [$record, $verified] = $this->check($record, $source); } else { $this->logger->debug('Releases of `%s` are served from the version registry.', (string) $id); } @@ -81,7 +80,7 @@ public function releases(RepositoryId $id, ReleaseSource $source): \Generator } // Older releases are loaded only when the consumer actually needs them - yield from $this->extend($record, $source, $verified, $unreadable); + yield from $this->extend($record, $source, $verified); } public function attach(RepositoryId $id, string $software): void @@ -105,6 +104,22 @@ public function forget(RepositoryId $id, string $tag): void $this->persist($record->withoutRelease($tag)->withoutCheck()); } + /** + * @param list $releases + * @param non-empty-string $tag + * @return int<0, max>|false Index of the release with the tag, or `false` when there is none. + */ + private static function position(array $releases, string $tag): int|false + { + foreach ($releases as $index => $release) { + if ($release->tag === $tag) { + return $index; + } + } + + return false; + } + /** * @param list $page */ @@ -122,24 +137,20 @@ private static function hasKnown(RepositoryRecord $record, array $page): bool /** * Fetches the releases published since the last check and stores the result. * - * Besides the record, tells how many of its leading releases the check has seen in place, - * as the stored releases beyond them may have shifted since they were stored, and how many - * listing positions among the pages it fetched hold a release the record cannot account - * for, as an unreadable one. A check that leaves the record untouched confirms nothing. + * Besides the record, tells how many of its leading releases the check has seen in place: + * the stored releases beyond them may have shifted since they were stored. A check that + * leaves the record untouched confirms nothing. * - * @return array{RepositoryRecord, int<0, max>, int<0, max>} + * @return array{RepositoryRecord, int<0, max>} * @throws RepositoryException When the source fails and nothing is stored to fall back on. */ private function check(RepositoryRecord $record, ReleaseSource $source): array { try { $fetched = []; - $unreadable = 0; $complete = $record->complete; foreach ($source->pages() as $page) { - $unreadable += $page->skipped; - // A release the source could not read is missing from the page exactly like a // deleted one, and `withHead()` would drop it; the stored list stays untouched // and the next run checks again @@ -151,7 +162,7 @@ private function check(RepositoryRecord $record, ReleaseSource $source): array $record->count(), ); - return [$record, 0, $unreadable]; + return [$record, 0]; } $fetched = [...$fetched, ...$page->releases]; @@ -176,51 +187,64 @@ private function check(RepositoryRecord $record, ReleaseSource $source): array ->withCheckedAt(($this->clock)()), ); - return [$record, \count($fetched), 0]; + return [$record, \count($fetched)]; } catch (RepositoryException $e) { $record->count() > 0 or throw $e; $this->report($e, $record); - return [$record, 0, 0]; + return [$record, 0]; } } /** * Loads the releases older than the stored ones page by page, persisting every page. * - * The source pages by position, and a stored release deleted upstream below the verified - * ones moves every older release up by one. So past them the loading starts one release - * earlier and expects the last stored release there; when another one shows up, or no page - * at all, the unverified tail is dropped and loaded again, without repeating to the consumer - * what it was served from the stored list. The dropped tail is written only along with the - * first page that replaces it, so a source failing right there leaves the record as it was. + * The source pages by position, and the stored count is not quite the position of the last + * stored release: a stored release deleted upstream below the verified ones moves it up, + * while a release the record does not hold, an unreadable one or one published while the + * check could not store it, moves it down. So past the verified releases the loading starts + * one release earlier and looks for the last stored release on that page and, as the way + * down is open, on the next one; everything up to it is known. When it is on neither, the + * unverified tail is dropped and loaded again, without repeating to the consumer what it was + * served from the stored list. The dropped tail is written only along with the first page + * that replaces it, so a source failing right there leaves the record as it was. * * The extra release lies on the page needed anyway when the stored list ends inside a page. * A list ending on a page boundary, as one made of fully loaded pages does, costs one more - * request for the page that ends with the release. + * request for the page that ends with the release. A release moved down past the end of the + * page after that is taken for a deletion. A deletion and an unreadable release above the + * tail cancel each other out, and the deleted release stays stored. * * @param int<0, max> $verified Number of leading stored releases known to be in place. - * @param int<0, max> $unreadable Listing positions above the stored tail that hold no stored - * release, so the last stored release sits that many positions below the stored count. * @return \Generator, mixed, void> * @throws RepositoryException */ - private function extend(RepositoryRecord $record, ReleaseSource $source, int $verified, int $unreadable): \Generator + private function extend(RepositoryRecord $record, ReleaseSource $source, int $verified): \Generator { $served = $record; $anchor = $verified < $record->count() ? $record->lastTag() : null; /** @var int<0, max> $offset */ - $offset = $record->count() + $unreadable - ($anchor === null ? 0 : 1); + $offset = $anchor === null ? $record->count() : $record->count() - 1; $pages = $source->pages($offset); $page = $pages->current(); + $ahead = 1; while (true) { if ($anchor !== null) { - if ($page === null || ($page->releases[0] ?? null)?->tag !== $anchor) { + $at = $page === null ? false : self::position($page->releases, $anchor); + + if ($page === null || $at === false) { + if ($page !== null && !$page->last && $ahead-- > 0) { + $pages->next(); + $page = $pages->current(); + + continue; + } + $this->logger->debug( - 'The stored tail of `%s` has shifted: %d release(s) are kept, the rest is loaded again.', + 'The stored tail of `%s` is not where it was; the stored releases after the first %d are loaded again.', (string) $record->id, $verified, ); @@ -234,7 +258,7 @@ private function extend(RepositoryRecord $record, ReleaseSource $source, int $ve } $anchor = null; - $page = new ReleasePage(\array_slice($page->releases, 1), $page->last, $page->skipped); + $page = new ReleasePage(\array_slice($page->releases, $at + 1), $page->last, $page->skipped); } if ($page === null) { diff --git a/tests/Unit/Module/Registry/StoredVersionRegistryTest.php b/tests/Unit/Module/Registry/StoredVersionRegistryTest.php index 2be5c0b4..169e78a5 100644 --- a/tests/Unit/Module/Registry/StoredVersionRegistryTest.php +++ b/tests/Unit/Module/Registry/StoredVersionRegistryTest.php @@ -157,9 +157,10 @@ public function releaseDeletedBelowTheCheckedSpanReloadsTheTail(): void $again = self::flatten($this->registry()->releases($this->id, $source)); // The stored `v3` is served as it was, but `v2` is not skipped: the check confirms - // page 1 only, the tail is found shifted at offset 5 and is loaded again from offset 2 + // page 1 only, `v3` is on neither the page at offset 5 nor the one after it, and the + // tail is loaded again from offset 2 Assert::same($again, ['v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); - Assert::same($source->served, [0, 4, 2, 4, 6]); + Assert::same($source->served, [0, 4, 6, 2, 4, 6]); $record = $this->storage->load($this->id); Assert::same(self::tagsOf($record->releases()), ['v8', 'v7', 'v6', 'v5', 'v4', 'v2', 'v1']); @@ -187,7 +188,7 @@ public function vanishedDraftPlaceholderReloadsTheTail(): void $again = self::flatten($this->registry()->releases($this->id, $source)); Assert::same($again, ['v6', 'v5', 'v4', 'v2', 'v1']); - Assert::same($source->served, [0, 2, 2, 4]); + Assert::same($source->served, [0, 2, 4, 2, 4]); Assert::false($this->storage->load($this->id)->has('draft')); } @@ -275,7 +276,7 @@ public function releaseDeletedBelowTheHeadIsNoticedAfterAFailedCheck(): void $source->failure = null; Assert::same(self::flatten($pages), ['v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); - Assert::same($source->served, [4, 0, 2, 4, 6]); + Assert::same($source->served, [4, 6, 0, 2, 4, 6]); // Neither a lost release nor a ghost is left behind Assert::same(self::tagsOf($this->storage->load($this->id)->releases()), ['v8', 'v7', 'v6', 'v5', 'v4', 'v2', 'v1']); @@ -300,14 +301,58 @@ public function unreadableReleaseOnTheFirstPageDoesNotHideADeletionInTheTail(): Assert::same(self::tagsOf($this->storage->load($this->id)->releases()), ['v10', 'v8', 'v7', 'v6', 'v5', 'v4', 'v3']); $source->served = []; - // Now `v5` is deleted upstream too: the anchor misses and the tail is loaded again + // Now `v5` and `v4` are deleted upstream too: the stored `v3` moved up past the page + // start, the last page has no further one to look at, and the tail is loaded again $source->delete('v5'); + $source->delete('v4'); $this->now += 601; $again = self::flatten($this->registry()->releases($this->id, $source)); Assert::same($again, ['v10', 'v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); - Assert::same($source->served, [0, 6, 0, 2, 4, 6, 8]); - Assert::same(self::tagsOf($this->storage->load($this->id)->releases()), ['v10', 'v8', 'v7', 'v6', 'v4', 'v3', 'v2', 'v1']); + Assert::same($source->served, [0, 6, 0, 2, 4, 6]); + Assert::same(self::tagsOf($this->storage->load($this->id)->releases()), ['v10', 'v8', 'v7', 'v6', 'v3', 'v2', 'v1']); + } + + #[Test] + public function unreadableReleaseBelowTheCheckedPageDoesNotMakeEveryCheckReloadTheTail(): void + { + $source = ArrayReleaseSource::ofTags(['v10', 'v9', 'v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + $source->unreadable = ['v6']; + self::take($this->registry()->releases($this->id, $source), 7); + Assert::same(self::tagsOf($this->storage->load($this->id)->releases()), ['v10', 'v9', 'v8', 'v7', 'v5', 'v4', 'v3']); + $source->served = []; + + // The check confirms page 1 and knows nothing of `v6`; the stored `v3` lies one position + // further than the stored count says and is found on the page fetched for it + $this->now += 601; + $again = self::flatten($this->registry()->releases($this->id, $source)); + + Assert::same($again, ['v10', 'v9', 'v8', 'v7', 'v5', 'v4', 'v3', 'v2', 'v1']); + Assert::same($source->served, [0, 6, 8]); + Assert::same(self::tagsOf($this->storage->load($this->id)->releases()), ['v10', 'v9', 'v8', 'v7', 'v5', 'v4', 'v3', 'v2', 'v1']); + + $source->served = []; + $this->now += 601; + self::flatten($this->registry()->releases($this->id, $source)); + Assert::same($source->served, [0]); + } + + #[Test] + public function lastStoredReleaseMovedDownToTheNextPageIsStillFound(): void + { + $source = ArrayReleaseSource::ofTags(['v10', 'v9', 'v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + $source->unreadable = ['v9', 'v5']; + self::take($this->registry()->releases($this->id, $source), 4); + Assert::same(self::tagsOf($this->storage->load($this->id)->releases()), ['v10', 'v8', 'v7', 'v6']); + $source->served = []; + + // The stored `v6` opens page 3 while the stored count points at page 2: the next page + // is looked at before the tail is given up + $this->now += 601; + $again = self::flatten($this->registry()->releases($this->id, $source)); + + Assert::same($again, ['v10', 'v8', 'v7', 'v6', 'v4', 'v3', 'v2', 'v1']); + Assert::same($source->served, [0, 2, 4, 6, 8]); } #[Test] diff --git a/tests/Unit/Module/Repository/Internal/GitHub/GitHubRepositoryTest.php b/tests/Unit/Module/Repository/Internal/GitHub/GitHubRepositoryTest.php index 4867f62b..2da5991b 100644 --- a/tests/Unit/Module/Repository/Internal/GitHub/GitHubRepositoryTest.php +++ b/tests/Unit/Module/Repository/Internal/GitHub/GitHubRepositoryTest.php @@ -126,6 +126,34 @@ public function shortPageBelowTheFirstOneDoesNotChangeThePageSize(): void Assert::true(\in_array('v1.0.300', $all, true)); } + #[Test] + public function unreadableReleaseAfterTheLastStoredOneOnItsPageDoesNotReloadTheTail(): void + { + // Three pages are stored; the last release of page 3 cannot be read, so the page the tail + // continues from starts one release before the last stored one + $storage = new InMemoryRegistryStorage(); + $firstClient = new PagedClientStub(pages: 4, unreadable: ['v1.0.300']); + $seen = 0; + foreach (self::createRepository($firstClient, self::registry($storage))->getReleases() as $release) { + unset($release); + if (++$seen === 250) { + break; + } + } + Assert::same($storage->load(new RepositoryId(GitHubRepository::TYPE, 'owner/repo'))?->count(), 299); + + $secondClient = new PagedClientStub(pages: 4, unreadable: ['v1.0.300']); + $all = self::names(self::createRepository($secondClient, self::registry($storage, refresh: true))); + + Assert::same($secondClient->requestedPages(), [1, 3, 4]); + Assert::same(\count($all), 399); + Assert::same(\count(\array_unique($all)), 399); + + $thirdClient = new PagedClientStub(pages: 4, unreadable: ['v1.0.300']); + self::names(self::createRepository($thirdClient, self::registry($storage, refresh: true))); + Assert::same($thirdClient->requestedPages(), [1]); + } + #[Test] public function unreadableReleaseAboveTheTailDoesNotMakeEveryRunReloadIt(): void { diff --git a/tests/Unit/Module/Repository/Internal/GitHub/Stub/PagedClientStub.php b/tests/Unit/Module/Repository/Internal/GitHub/Stub/PagedClientStub.php index f349c1c0..9b8dad52 100644 --- a/tests/Unit/Module/Repository/Internal/GitHub/Stub/PagedClientStub.php +++ b/tests/Unit/Module/Repository/Internal/GitHub/Stub/PagedClientStub.php @@ -32,6 +32,7 @@ final class PagedClientStub implements ClientInterface * @param int<0, max> $broken Number of the newest published releases that cannot be decoded. * @param int<1, max>|null $perPageLimit Greatest number of releases served per page whatever `per_page` asks for. * @param list $withheld Tags left out of a page after it was cut, as a server filtering after paging does. + * @param list $unreadable Tags of releases that cannot be decoded, wherever they are listed. */ public function __construct( private readonly int $pages = 1, @@ -40,6 +41,7 @@ public function __construct( private readonly int $broken = 0, private readonly ?int $perPageLimit = null, private readonly array $withheld = [], + private readonly array $unreadable = [], ) {} public function sendRequest(RequestInterface $request): ResponseInterface @@ -106,7 +108,7 @@ private function allReleases(): array $releases[] = [ 'name' => $tag, // A number where a string belongs fails the strict constructor of the response object - 'tag_name' => $i >= 1 && $i <= $this->broken ? $i : $tag, + 'tag_name' => ($i >= 1 && $i <= $this->broken) || \in_array($tag, $this->unreadable, true) ? $i : $tag, 'published_at' => $i < 1 ? null : '2024-01-01T00:00:00Z', 'assets' => [[ 'name' => 'rr-linux-amd64.tar.gz', From 904de567f8d77692777b7997d5cd461e889ce781 Mon Sep 17 00:00:00 2001 From: roxblnfk Date: Mon, 12 Oct 2026 15:44:52 +0400 Subject: [PATCH 5/5] fix(registry): find the stored tail on the page instead of reloading it fix(repository): take a first page for the page size only when it holds at most half The page holding the last stored position shows which way the listing moved, so the tail continues after the last stored release found there; dropping and reloading the unverified tail cost a request per stored page, and a release deleted upstream stays stored until its download fails and it is forgotten. Within the TTL the tail is located the same way: trusting it whole lost a release deleted in that window for good. A first page short of a release or two is one filtered out after paging; taking it for the page size made every check reload the tail. Assisted-By: Claude Opus 5.5 --- .../Internal/StoredVersionRegistry.php | 140 ++++----- src/Module/Registry/Record/ReleasePage.php | 2 + .../Registry/Record/RepositoryRecord.php | 58 +--- src/Module/Registry/ReleaseSource.php | 6 +- .../Internal/PagedReleaseSource.php | 55 ++-- .../Module/Registry/RepositoryRecordTest.php | 28 -- .../Registry/StoredVersionRegistryTest.php | 287 ++++++++++-------- .../Registry/Stub/ArrayReleaseSource.php | 17 +- .../Internal/GitHub/GitHubRepositoryTest.php | 28 +- .../Internal/GitLab/GitLabRepositoryTest.php | 2 +- .../Internal/PagedReleaseSourceTest.php | 56 ++-- 11 files changed, 314 insertions(+), 365 deletions(-) diff --git a/src/Module/Registry/Internal/StoredVersionRegistry.php b/src/Module/Registry/Internal/StoredVersionRegistry.php index 4198c161..a0da4b89 100644 --- a/src/Module/Registry/Internal/StoredVersionRegistry.php +++ b/src/Module/Registry/Internal/StoredVersionRegistry.php @@ -4,7 +4,6 @@ namespace Internal\DLoad\Module\Registry\Internal; -use Internal\DLoad\Module\Registry\Record\ReleasePage; use Internal\DLoad\Module\Registry\Record\ReleaseRecord; use Internal\DLoad\Module\Registry\Record\RepositoryRecord; use Internal\DLoad\Module\Registry\RegistryStorage; @@ -26,9 +25,8 @@ * attached after the check are updated too. * 2. **Serve.** The stored releases are yielded segment by segment without any request. * 3. **Extend.** When the consumer runs past the stored releases and the listing is not known - * to be complete, older pages are fetched one by one and appended to the record. After a - * check, the stored releases below the ones it confirmed are first found to be where they - * were, as a deletion among them would shift the listing. + * to be complete, older pages are fetched one by one and appended to the record, from the + * last stored release on. * * A check that fails while releases are stored falls back to the stored ones: an outage or a rate * limit should not break what worked a minute ago. A rate limit is reported once per run in plain @@ -63,10 +61,10 @@ public function releases(RepositoryId $id, ReleaseSource $source): \Generator { $record = $this->storage->load($id) ?? RepositoryRecord::empty($id); - // Within the TTL the stored list is taken as it is, head and tail alike - $verified = $record->count(); + // Only a check can tell that the stored releases are still where they were listed + $seen = false; if ($this->refresh || $record->isStale(($this->clock)(), $this->ttl)) { - [$record, $verified] = $this->check($record, $source); + [$record, $seen] = $this->check($record, $source); } else { $this->logger->debug('Releases of `%s` are served from the version registry.', (string) $id); } @@ -80,7 +78,7 @@ public function releases(RepositoryId $id, ReleaseSource $source): \Generator } // Older releases are loaded only when the consumer actually needs them - yield from $this->extend($record, $source, $verified); + yield from $this->extend($record, $source, $seen); } public function attach(RepositoryId $id, string $software): void @@ -105,19 +103,18 @@ public function forget(RepositoryId $id, string $tag): void } /** - * @param list $releases - * @param non-empty-string $tag - * @return int<0, max>|false Index of the release with the tag, or `false` when there is none. + * @param list $page + * @return int<0, max>|null Index of the last stored release of the page, if any. */ - private static function position(array $releases, string $tag): int|false + private static function lastKnown(RepositoryRecord $record, array $page): ?int { - foreach ($releases as $index => $release) { - if ($release->tag === $tag) { + for ($index = \count($page) - 1; $index >= 0; --$index) { + if ($record->has($page[$index]->tag)) { return $index; } } - return false; + return null; } /** @@ -137,11 +134,9 @@ private static function hasKnown(RepositoryRecord $record, array $page): bool /** * Fetches the releases published since the last check and stores the result. * - * Besides the record, tells how many of its leading releases the check has seen in place: - * the stored releases beyond them may have shifted since they were stored. A check that - * leaves the record untouched confirms nothing. + * Also tells whether the check has seen every stored release where it is listed now. * - * @return array{RepositoryRecord, int<0, max>} + * @return array{RepositoryRecord, bool} * @throws RepositoryException When the source fails and nothing is stored to fall back on. */ private function check(RepositoryRecord $record, ReleaseSource $source): array @@ -162,7 +157,7 @@ private function check(RepositoryRecord $record, ReleaseSource $source): array $record->count(), ); - return [$record, 0]; + return [$record, false]; } $fetched = [...$fetched, ...$page->releases]; @@ -187,102 +182,75 @@ private function check(RepositoryRecord $record, ReleaseSource $source): array ->withCheckedAt(($this->clock)()), ); - return [$record, \count($fetched)]; + $tags = \array_map(static fn(ReleaseRecord $release): string => $release->tag, $fetched); + + return [$record, $record->count() <= \count(\array_unique($tags))]; } catch (RepositoryException $e) { $record->count() > 0 or throw $e; $this->report($e, $record); - return [$record, 0]; + return [$record, false]; } } /** * Loads the releases older than the stored ones page by page, persisting every page. * - * The source pages by position, and the stored count is not quite the position of the last - * stored release: a stored release deleted upstream below the verified ones moves it up, - * while a release the record does not hold, an unreadable one or one published while the - * check could not store it, moves it down. So past the verified releases the loading starts - * one release earlier and looks for the last stored release on that page and, as the way - * down is open, on the next one; everything up to it is known. When it is on neither, the - * unverified tail is dropped and loaded again, without repeating to the consumer what it was - * served from the stored list. The dropped tail is written only along with the first page - * that replaces it, so a source failing right there leaves the record as it was. + * The stored count only hints at the listing position of the last stored release: a stored + * release deleted upstream moves it up, a release the record misses above it moves it down. + * So the loading starts with the page holding that position and goes on after the last + * stored release found there. A page holding none sends the search up the listing; when it + * reaches the top without one, the search goes down from the first page instead. The page + * fetched first is the one needed anyway when the stored list ends inside a page; a list + * ending on a page boundary costs one request more. A list the check has just seen whole is + * where it was fetched, so its tail goes on right after it. * - * The extra release lies on the page needed anyway when the stored list ends inside a page. - * A list ending on a page boundary, as one made of fully loaded pages does, costs one more - * request for the page that ends with the release. A release moved down past the end of the - * page after that is taken for a deletion. A deletion and an unreadable release above the - * tail cancel each other out, and the deleted release stays stored. + * A stored release deleted upstream stays stored until its download fails and it is + * forgotten. A release the listing gained above the last stored one, such as an unreadable + * one turned readable, is not loaded. * - * @param int<0, max> $verified Number of leading stored releases known to be in place. + * @param bool $seen Whether the check has just seen every stored release where it is. * @return \Generator, mixed, void> * @throws RepositoryException */ - private function extend(RepositoryRecord $record, ReleaseSource $source, int $verified): \Generator + private function extend(RepositoryRecord $record, ReleaseSource $source, bool $seen): \Generator { - $served = $record; - $anchor = $verified < $record->count() ? $record->lastTag() : null; - /** @var int<0, max> $offset */ - $offset = $anchor === null ? $record->count() : $record->count() - 1; - - $pages = $source->pages($offset); - $page = $pages->current(); - $ahead = 1; - - while (true) { - if ($anchor !== null) { - $at = $page === null ? false : self::position($page->releases, $anchor); - - if ($page === null || $at === false) { - if ($page !== null && !$page->last && $ahead-- > 0) { - $pages->next(); - $page = $pages->current(); - - continue; - } - - $this->logger->debug( - 'The stored tail of `%s` is not where it was; the stored releases after the first %d are loaded again.', - (string) $record->id, - $verified, - ); - - $record = $record->withoutTail($verified); - $anchor = null; - $pages = $source->pages($record->count()); - $page = $pages->current(); - + $found = $seen || $record->count() === 0; + $first = $pages = $source->pages($found ? $record->count() : \max(0, $record->count() - 1)); + // The search goes up while the pages it meets start ever higher, then down once + $up = true; + $above = \PHP_INT_MAX; + + while (($page = $pages->current()) !== null) { + $at = self::lastKnown($record, $page->releases); + if (!$found && $at === null) { + if ($up && $page->offset > 0 && $page->offset < $above) { + $above = $page->offset; + $pages = $source->pages($page->offset - 1); continue; } - $anchor = null; - $page = new ReleasePage(\array_slice($page->releases, $at + 1), $page->last, $page->skipped); - } - - if ($page === null) { - break; + // Nothing stored up to the top: releases the record misses pushed the tail down + $up and $pages = $first; + $up = false; + $pages->next(); + continue; } - $new = \array_values(\array_filter( - $page->releases, - static fn(ReleaseRecord $release): bool => !$record->has($release->tag), - )); + $found = true; + $new = $at === null ? $page->releases : \array_slice($page->releases, $at + 1); $record = $this->persist($record->withTail($new)->withComplete($page->last)); - $visible = ReleaseRecord::visible(\array_values(\array_filter( - $new, - static fn(ReleaseRecord $release): bool => !$served->has($release->tag), - ))); + $visible = ReleaseRecord::visible($new); $visible === [] or yield $visible; $pages->next(); - $page = $pages->current(); } - $record->complete or $this->persist($record->withComplete(true)); + // A listing without any stored release is left for the next check to replace + $found and !$record->complete and $this->persist($record->withComplete(true)); } /** diff --git a/src/Module/Registry/Record/ReleasePage.php b/src/Module/Registry/Record/ReleasePage.php index 4a19216a..3e224f30 100644 --- a/src/Module/Registry/Record/ReleasePage.php +++ b/src/Module/Registry/Record/ReleasePage.php @@ -19,10 +19,12 @@ final class ReleasePage * @param bool $last Whether the listing has no page after this one. * @param int<0, max> $skipped Number of releases of the page the source could not read. They * are absent from `$releases` although they exist upstream. + * @param int<0, max> $offset Listing position the page starts at, unreadable releases included. */ public function __construct( public readonly array $releases, public readonly bool $last, public readonly int $skipped = 0, + public readonly int $offset = 0, ) {} } diff --git a/src/Module/Registry/Record/RepositoryRecord.php b/src/Module/Registry/Record/RepositoryRecord.php index 46af809f..0e709677 100644 --- a/src/Module/Registry/Record/RepositoryRecord.php +++ b/src/Module/Registry/Record/RepositoryRecord.php @@ -11,8 +11,9 @@ * * Releases are kept newest first, exactly in the order the source lists them, and they form * a contiguous prefix of that listing: the registry only ever extends the list at the head (new - * releases found on a check) or at the tail (older releases loaded on demand). `complete` tells - * whether the tail has reached the end of the listing. + * releases found on a check) or at the tail (older releases loaded on demand). A release deleted + * upstream below the span of the last check stays until its download fails and it is forgotten. + * `complete` tells whether the tail has reached the end of the listing. * * The list is split into segments of at most `SEGMENT_SIZE` releases, so a repository with * thousands of releases costs one small index read plus the segments the run actually iterates. @@ -204,9 +205,8 @@ public function isStale(int $now, int $ttl): bool * The fetched releases are the newest ones, and sources only ever add releases at the top, * so from the first stored release they contain onwards both lists walk the same listing * positions. A stored release missing from the fetched span at its position was deleted - * upstream and is dropped: keeping it would inflate `count()`, which the registry uses as - * the offset for loading the tail. Stored releases beyond the span are kept, and when the - * fetched releases contain no stored one they are the whole listing. + * upstream and is dropped. Stored releases beyond the span are kept, and when the fetched + * releases contain no stored one they are the whole listing. * * @param list $fetched Newest first. */ @@ -300,54 +300,6 @@ public function withTail(array $fetched): self ]); } - /** - * Drops the releases from the given position on; the list is incomplete afterwards. - * - * @param int<0, max> $from Number of releases to keep. - */ - public function withoutTail(int $from): self - { - if ($from >= $this->count()) { - return $this; - } - - $segments = []; - $kept = 0; - foreach ($this->segments as $segment) { - if ($kept + $segment->count() <= $from) { - $segments[] = $segment; - $kept += $segment->count(); - continue; - } - - /** @var non-empty-list $releases */ - $releases = \array_slice($segment->releases(), 0, $from - $kept); - $from > $kept and $segments[] = ReleaseSegment::fresh($segment->key, $releases); - break; - } - - return $this->with(complete: false, segments: $segments); - } - - /** - * Tag of the oldest stored release, or `null` when nothing is stored. - * - * @return non-empty-string|null - */ - public function lastTag(): ?string - { - $segments = $this->segments; - $last = \end($segments); - if ($last === false) { - return null; - } - - $tags = $last->tags; - $tag = \end($tags); - - return $tag === false ? null : $tag; - } - public function withCheckedAt(int $checkedAt): self { return $this->with(checkedAt: $checkedAt); diff --git a/src/Module/Registry/ReleaseSource.php b/src/Module/Registry/ReleaseSource.php index 1ae4e4c8..b28bd057 100644 --- a/src/Module/Registry/ReleaseSource.php +++ b/src/Module/Registry/ReleaseSource.php @@ -18,12 +18,14 @@ interface ReleaseSource { /** - * Lists releases newest first, skipping the given number of them. + * Lists releases newest first in whole pages, from the page holding the given position on. * + * Positions count every release of the listing, unreadable ones included. The first page may + * start before the position; when the listing ends before it, its last page comes first. * A page is requested only when the generator advances to it. The generator ends when the * listing ends. * - * @param int<0, max> $offset Number of newest releases to skip. + * @param int<0, max> $offset Listing position of the first release wanted. * @return \Generator Pages of releases. * @throws RepositoryException When a page cannot be loaded. */ diff --git a/src/Module/Repository/Internal/PagedReleaseSource.php b/src/Module/Repository/Internal/PagedReleaseSource.php index e74cf08b..1f2045e2 100644 --- a/src/Module/Repository/Internal/PagedReleaseSource.php +++ b/src/Module/Repository/Internal/PagedReleaseSource.php @@ -11,16 +11,11 @@ /** * Source over an API that lists releases in numbered pages of a fixed size. * - * Translates a release offset into the page number and the number of releases to drop from - * that page. The page size is the one the API is asked for, until the first page shows that - * the server serves fewer releases per page, as a mirror or a server that caps `per_page` does. - * Only the first page can tell: it is the one page whose start is known, so its length is the - * page size and not the sign of a release filtered out after paging. - * - * The page size is learned per instance, not stored: the registry maps offsets through the - * source and knows nothing of pages. So a server capping `per_page` costs the first page again - * on every run that does not fetch it anyway, and a server serving more releases per page than - * asked for is not supported: GitHub and GitLab both stop at the number asked for. + * A listing position is mapped onto the page holding it by the page size the API is asked for. + * A first page holding at most half of that, as a mirror or a server capping `per_page` serves, + * tells the real size for the rest of the instance's life. A page only slightly shorter is taken + * for releases filtered out after paging and changes nothing. A page size the source gets wrong + * costs the registry a step to the neighbouring page, never a release. * * @internal * @psalm-internal Internal\DLoad\Module\Repository @@ -47,48 +42,42 @@ public function __construct( */ public function pages(int $offset = 0): \Generator { - $first = $this->pageOf($offset); - $pages = ($this->pages)($first); + $number = $this->pageOf($offset); + $pages = ($this->pages)($number); $page = $pages->current(); - if ($first === 1 && $page !== null && !$page->last && $this->learn($page) && $this->pageOf($offset) !== 1) { - $pages = ($this->pages)($this->pageOf($offset)); + if ($number === 1 && $page !== null && !$page->last && $this->learn($page) && $this->pageOf($offset) > 1) { + $number = $this->pageOf($offset); + $pages = ($this->pages)($number); + $page = $pages->current(); + } + + // The listing ends before the position: it is listed from its last page + while ($page === null && $number > 1) { + $pages = ($this->pages)(--$number); $page = $pages->current(); } // The generator is walked by hand: one that ended before its first page is closed, and // `foreach` cannot start it over - $skip = $offset % $this->perPage; + $size = $this->perPage; while ($page !== null) { - yield $skip === 0 ? $page : self::from($page, $skip); - $skip = 0; + yield new ReleasePage($page->releases, $page->last, $page->skipped, ($number - 1) * $size); + ++$number; $pages->next(); $page = $pages->current(); } } /** - * Drops the releases before the given position of the page. - * - * The unreadable releases of a page are not in its list, yet they hold positions on it. - * They are taken as lying before the position: when one lies after it instead, a release is - * repeated rather than lost, and the registry drops repeated ones. - * - * @param int<0, max> $skip - */ - private static function from(ReleasePage $page, int $skip): ReleasePage - { - return new ReleasePage(\array_slice($page->releases, \max(0, $skip - $page->skipped)), $page->last, $page->skipped); - } - - /** - * Takes the length of a page followed by another one as the page size; tells whether it changed. + * Takes the length of a short first page followed by another one as the page size; tells + * whether it changed. */ private function learn(ReleasePage $page): bool { $size = \count($page->releases) + $page->skipped; - if ($size < 1 || $size >= $this->perPage) { + if ($size < 1 || $size * 2 > $this->perPage) { return false; } diff --git a/tests/Unit/Module/Registry/RepositoryRecordTest.php b/tests/Unit/Module/Registry/RepositoryRecordTest.php index 5d1fe769..37910c26 100644 --- a/tests/Unit/Module/Registry/RepositoryRecordTest.php +++ b/tests/Unit/Module/Registry/RepositoryRecordTest.php @@ -213,34 +213,6 @@ public function droppingTheLastReleaseOfASegmentDropsTheSegment(): void Assert::false($updated->segments[0]->dirty); } - #[Test] - public function tailIsDroppedFromInsideASegment(): void - { - $record = self::record(self::range(250, 1))->withComplete(true); - Assert::same(self::sizes($record), [50, 100, 100]); - - $updated = $record->withoutTail(120); - - Assert::same(self::sizes($updated), [50, 70]); - Assert::same(self::keys($updated), ['0001', '0002']); - Assert::same($updated->lastTag(), 'v131'); - Assert::false($updated->complete); - Assert::false($updated->segments[0]->dirty); - Assert::true($updated->segments[1]->dirty); - } - - #[Test] - public function tailIsDroppedAtASegmentBoundary(): void - { - $record = self::record(self::range(250, 1)); - - Assert::same(self::sizes($record->withoutTail(150)), [50, 100]); - Assert::same(self::sizes($record->withoutTail(0)), []); - Assert::null($record->withoutTail(0)->lastTag()); - Assert::same($record->withoutTail(250), $record); - Assert::same($record->lastTag(), 'v1'); - } - #[Test] public function stalenessDependsOnTheLastCheck(): void { diff --git a/tests/Unit/Module/Registry/StoredVersionRegistryTest.php b/tests/Unit/Module/Registry/StoredVersionRegistryTest.php index 169e78a5..5be88cc6 100644 --- a/tests/Unit/Module/Registry/StoredVersionRegistryTest.php +++ b/tests/Unit/Module/Registry/StoredVersionRegistryTest.php @@ -142,195 +142,204 @@ public function releaseDeletedUpstreamDoesNotShiftTheTail(): void } #[Test] - public function releaseDeletedBelowTheCheckedSpanReloadsTheTail(): void + public function tailEndingInsideAPageCostsNoExtraRequest(): void { - $source = ArrayReleaseSource::ofTags(['v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); - - // The first run stores three pages and stops - $pages = $this->registry()->releases($this->id, $source); - self::take($pages, 6); + $source = ArrayReleaseSource::ofTags(['v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + $this->registry()->releases($this->id, $source)->current(); $source->served = []; - // `v3` is deleted upstream, outside the page the check will fetch - $source->delete('v3'); + // The check leaves three stored, the last one inside page 2 + $source->publish('v7'); $this->now += 601; $again = self::flatten($this->registry()->releases($this->id, $source)); - // The stored `v3` is served as it was, but `v2` is not skipped: the check confirms - // page 1 only, `v3` is on neither the page at offset 5 nor the one after it, and the - // tail is loaded again from offset 2 - Assert::same($again, ['v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); - Assert::same($source->served, [0, 4, 6, 2, 4, 6]); - - $record = $this->storage->load($this->id); - Assert::same(self::tagsOf($record->releases()), ['v8', 'v7', 'v6', 'v5', 'v4', 'v2', 'v1']); - Assert::true($record->complete); + Assert::same($again, ['v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + Assert::count($source->served, 4); } #[Test] - public function vanishedDraftPlaceholderReloadsTheTail(): void + public function tailEndingOnAPageBoundaryCostsOneRequestMore(): void { - $source = new ArrayReleaseSource([ - new ReleaseRecord('v6', 'v6'), - new ReleaseRecord('v5', 'v5'), - new ReleaseRecord('v4', 'v4'), - new ReleaseRecord('draft', 'draft', hidden: true), - new ReleaseRecord('v2', 'v2'), - new ReleaseRecord('v1', 'v1'), - ]); - self::take($this->registry()->releases($this->id, $source), 3); - Assert::same($this->storage->load($this->id)?->count(), 4); + $source = ArrayReleaseSource::ofTags(['v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + self::take($this->registry()->releases($this->id, $source), 4); $source->served = []; - // The draft is gone from the listing, as after its deletion or with a token that cannot see it - $source->delete('draft'); $this->now += 601; $again = self::flatten($this->registry()->releases($this->id, $source)); - Assert::same($again, ['v6', 'v5', 'v4', 'v2', 'v1']); - Assert::same($source->served, [0, 2, 4, 2, 4]); - Assert::false($this->storage->load($this->id)->has('draft')); + // The check, the page ending with the last stored release, and the page the tail needs + Assert::same($again, ['v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + Assert::count($source->served, 3); } #[Test] - public function tailBelowTheCheckedSpanIsConfirmedOnThePageItContinuesFrom(): void + public function tailSeenWholeByTheCheckGoesOnWithoutExtraRequest(): void { $source = ArrayReleaseSource::ofTags(['v6', 'v5', 'v4', 'v3', 'v2', 'v1']); $this->registry()->releases($this->id, $source)->current(); $source->served = []; - // The check confirms page 1 and leaves three stored, the last one in the middle of - // page 2: it is confirmed on the page the tail continues from - $source->publish('v7'); $this->now += 601; $again = self::flatten($this->registry()->releases($this->id, $source)); - Assert::same($again, ['v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); - Assert::same($source->served, [0, 2, 4, 6]); + Assert::same($again, ['v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + Assert::count($source->served, 3); } #[Test] - public function tailEndingOnAPageBoundaryCostsOneRequestToConfirm(): void + public function releaseDeletedBelowTheCheckedSpanDoesNotShiftTheTail(): void { - $source = ArrayReleaseSource::ofTags(['v6', 'v5', 'v4', 'v3', 'v2', 'v1']); - self::take($this->registry()->releases($this->id, $source), 4); + $source = ArrayReleaseSource::ofTags(['v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + self::take($this->registry()->releases($this->id, $source), 6); $source->served = []; - // Four are stored, the check confirms two: the page holding the fourth is fetched to - // confirm it before the tail goes on + // `v5` is deleted upstream, outside the page the check fetches + $source->delete('v5'); $this->now += 601; $again = self::flatten($this->registry()->releases($this->id, $source)); - Assert::same($again, ['v6', 'v5', 'v4', 'v3', 'v2', 'v1']); - Assert::same($source->served, [0, 2, 4]); + // `v2` moved up into the stored part of the listing and is still loaded; the stored + // `v5` stays until its download fails + Assert::same($again, ['v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + Assert::same($this->stored(), ['v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + Assert::true($this->storage->load($this->id)->complete); + Assert::count($source->served, 3); + } + + #[Test] + public function deletedLastStoredReleaseDoesNotShiftTheTail(): void + { + $source = ArrayReleaseSource::ofTags(['v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + self::take($this->registry()->releases($this->id, $source), 6); + + $source->delete('v3'); + $this->now += 601; + $again = self::flatten($this->registry()->releases($this->id, $source)); + + Assert::same($again, ['v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + Assert::same($this->stored(), $again); + } + + #[Test] + public function deletionsMovingTheTailOffItsPageAreFollowedUp(): void + { + $source = ArrayReleaseSource::ofTags(['v12', 'v11', 'v10', 'v9', 'v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + self::take($this->registry()->releases($this->id, $source), 8); + + // The last stored `v5` moves up three positions, off the page its stored count points at + $source->delete('v8'); + $source->delete('v7'); + $source->delete('v6'); + $this->now += 601; + $again = self::flatten($this->registry()->releases($this->id, $source)); + + Assert::same($again, ['v12', 'v11', 'v10', 'v9', 'v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + Assert::same($this->stored(), $again); + } + + #[Test] + public function deepDeletionInALongTailCostsNoReload(): void + { + $tags = \array_map(static fn(int $i): string => "v$i", \range(2500, 1)); + $source = ArrayReleaseSource::ofTags($tags, perPage: 100); + self::take($this->registry()->releases($this->id, $source), 2000); + $source->served = []; + + $source->delete('v1000'); + $this->now += 601; + self::take($this->registry()->releases($this->id, $source), 2050); + + // The check, the page holding the last stored release and the next one + Assert::count($source->served, 3); + Assert::same($this->stored(), \array_slice($tags, 0, 2101)); } #[Test] - public function releasesPublishedWithinTheTtlOverlapTheTailWithoutRepeating(): void + public function releasesPublishedWithinTheTtlAreNeitherRepeatedNorAppended(): void { $source = ArrayReleaseSource::ofTags(['v6', 'v5', 'v4', 'v3', 'v2', 'v1']); $this->registry()->releases($this->id, $source)->current(); - $source->served = []; - // The record is fresh, so nothing is confirmed; the listing shifted down by one - $source->publish('v7'); + // More releases than a page holds: the page at the stored count holds no stored one + $source->publish('v9', 'v8', 'v7'); $this->now += 100; $again = self::flatten($this->registry()->releases($this->id, $source)); Assert::same($again, ['v6', 'v5', 'v4', 'v3', 'v2', 'v1']); - Assert::same($source->served, [2, 4, 6]); + Assert::same($this->stored(), $again); + + // The next check puts them on top + $this->now += 601; + Assert::same(self::flatten($this->registry()->releases($this->id, $source)), ['v9', 'v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); } #[Test] - public function failedCheckConfirmsNothingAboutTheStoredTail(): void + public function releaseDeletedWithinTheTtlDoesNotShiftTheTail(): void { $source = ArrayReleaseSource::ofTags(['v6', 'v5', 'v4', 'v3', 'v2', 'v1']); self::take($this->registry()->releases($this->id, $source), 4); - $source->served = []; - $source->fail(); - $this->now += 601; - $pages = $this->registry()->releases($this->id, $source); - Assert::same(self::tagsOf($pages->current()), ['v6', 'v5', 'v4', 'v3']); + $source->delete('v4'); + $this->now += 100; + $again = self::flatten($this->registry()->releases($this->id, $source)); - // The source is back: the check compared nothing, so the tail is confirmed from its - // last stored release on, which lies on the page needed anyway - $source->failure = null; - Assert::same(self::flatten($pages), ['v6', 'v5', 'v4', 'v3', 'v2', 'v1']); - Assert::same($source->served, [2, 4]); + Assert::same($again, ['v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + Assert::same($this->stored(), $again); } #[Test] - public function releaseDeletedBelowTheHeadIsNoticedAfterAFailedCheck(): void + public function failedCheckLeavesReleasesPublishedAboveTheTailToTheNextOne(): void { $source = ArrayReleaseSource::ofTags(['v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); - self::take($this->registry()->releases($this->id, $source), 6); - $source->served = []; + $this->registry()->releases($this->id, $source)->current(); - // `v3` is deleted upstream while the check cannot reach the source - $source->delete('v3'); + $source->publish('n3', 'n2', 'n1'); $source->fail(); $this->now += 601; $pages = $this->registry()->releases($this->id, $source); - $pages->current(); + Assert::same(self::tagsOf($pages->current()), ['v8', 'v7']); + // The source is back for the tail $source->failure = null; Assert::same(self::flatten($pages), ['v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); - Assert::same($source->served, [4, 6, 0, 2, 4, 6]); + Assert::same($this->stored(), ['v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); - // Neither a lost release nor a ghost is left behind - Assert::same(self::tagsOf($this->storage->load($this->id)->releases()), ['v8', 'v7', 'v6', 'v5', 'v4', 'v2', 'v1']); + $this->now += 601; + self::flatten($this->registry()->releases($this->id, $source)); + Assert::same($this->stored(), ['n3', 'n2', 'n1', 'v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); } #[Test] - public function unreadableReleaseOnTheFirstPageDoesNotHideADeletionInTheTail(): void + public function unreadableReleaseAboveTheTailDoesNotShiftIt(): void { $source = ArrayReleaseSource::ofTags(['v10', 'v9', 'v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); $source->unreadable = ['v9']; self::take($this->registry()->releases($this->id, $source), 5); - Assert::same(self::tagsOf($this->storage->load($this->id)->releases()), ['v10', 'v8', 'v7', 'v6', 'v5']); - $source->served = []; - - // The check bails out on `v9` and leaves the record as it is; the stored `v5` is one - // position further down the listing than the stored count says, and is found there - $this->now += 601; - $pages = $this->registry()->releases($this->id, $source); - self::take($pages, 7); - - Assert::same($source->served, [0, 4, 6]); - Assert::same(self::tagsOf($this->storage->load($this->id)->releases()), ['v10', 'v8', 'v7', 'v6', 'v5', 'v4', 'v3']); - $source->served = []; + Assert::same($this->stored(), ['v10', 'v8', 'v7', 'v6', 'v5']); - // Now `v5` and `v4` are deleted upstream too: the stored `v3` moved up past the page - // start, the last page has no further one to look at, and the tail is loaded again - $source->delete('v5'); - $source->delete('v4'); + // The check bails out on `v9`; the stored `v5` lies one position further than its count $this->now += 601; $again = self::flatten($this->registry()->releases($this->id, $source)); Assert::same($again, ['v10', 'v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); - Assert::same($source->served, [0, 6, 0, 2, 4, 6]); - Assert::same(self::tagsOf($this->storage->load($this->id)->releases()), ['v10', 'v8', 'v7', 'v6', 'v3', 'v2', 'v1']); + Assert::same($this->stored(), $again); } #[Test] - public function unreadableReleaseBelowTheCheckedPageDoesNotMakeEveryCheckReloadTheTail(): void + public function unreadableReleaseInTheStoredTailDoesNotShiftIt(): void { $source = ArrayReleaseSource::ofTags(['v10', 'v9', 'v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); $source->unreadable = ['v6']; self::take($this->registry()->releases($this->id, $source), 7); - Assert::same(self::tagsOf($this->storage->load($this->id)->releases()), ['v10', 'v9', 'v8', 'v7', 'v5', 'v4', 'v3']); $source->served = []; - // The check confirms page 1 and knows nothing of `v6`; the stored `v3` lies one position - // further than the stored count says and is found on the page fetched for it $this->now += 601; $again = self::flatten($this->registry()->releases($this->id, $source)); Assert::same($again, ['v10', 'v9', 'v8', 'v7', 'v5', 'v4', 'v3', 'v2', 'v1']); - Assert::same($source->served, [0, 6, 8]); - Assert::same(self::tagsOf($this->storage->load($this->id)->releases()), ['v10', 'v9', 'v8', 'v7', 'v5', 'v4', 'v3', 'v2', 'v1']); + Assert::count($source->served, 3); + // Every later check costs the first page only $source->served = []; $this->now += 601; self::flatten($this->registry()->releases($this->id, $source)); @@ -338,32 +347,27 @@ public function unreadableReleaseBelowTheCheckedPageDoesNotMakeEveryCheckReloadT } #[Test] - public function lastStoredReleaseMovedDownToTheNextPageIsStillFound(): void + public function releaseTurnedReadableAboveTheTailIsNotLoaded(): void { $source = ArrayReleaseSource::ofTags(['v10', 'v9', 'v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); - $source->unreadable = ['v9', 'v5']; - self::take($this->registry()->releases($this->id, $source), 4); - Assert::same(self::tagsOf($this->storage->load($this->id)->releases()), ['v10', 'v8', 'v7', 'v6']); - $source->served = []; + $source->unreadable = ['v6']; + self::take($this->registry()->releases($this->id, $source), 7); - // The stored `v6` opens page 3 while the stored count points at page 2: the next page - // is looked at before the tail is given up + // `v6` lies above the last stored release, where only a check could see it + $source->unreadable = []; $this->now += 601; $again = self::flatten($this->registry()->releases($this->id, $source)); - Assert::same($again, ['v10', 'v8', 'v7', 'v6', 'v4', 'v3', 'v2', 'v1']); - Assert::same($source->served, [0, 2, 4, 6, 8]); + Assert::same($again, ['v10', 'v9', 'v8', 'v7', 'v5', 'v4', 'v3', 'v2', 'v1']); } #[Test] - public function anchorBeyondTheEndOfTheListingReloadsTheTail(): void + public function listingShorterThanTheStoredCountIsContinuedFromItsLastPage(): void { $source = ArrayReleaseSource::ofTags(['v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); self::take($this->registry()->releases($this->id, $source), 6); - $source->served = []; - // Three releases below page 1 are gone: the last stored one would sit past the end now, - // and the source has no page to answer with + $source->delete('v5'); $source->delete('v4'); $source->delete('v3'); $source->delete('v2'); @@ -371,34 +375,63 @@ public function anchorBeyondTheEndOfTheListingReloadsTheTail(): void $again = self::flatten($this->registry()->releases($this->id, $source)); Assert::same($again, ['v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v1']); - Assert::same($source->served, [0, 4, 2, 4]); - Assert::same(self::tagsOf($this->storage->load($this->id)->releases()), ['v8', 'v7', 'v6', 'v5', 'v1']); + Assert::same($this->stored(), $again); + Assert::true($this->storage->load($this->id)->complete); } #[Test] - public function failureWhileReloadingTheTailKeepsTheStoredReleases(): void + public function listingEmptiedUpstreamLeavesNothingStored(): void { - $source = ArrayReleaseSource::ofTags(['v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + $source = ArrayReleaseSource::ofTags(['v4', 'v3', 'v2', 'v1']); + $this->registry()->releases($this->id, $source)->current(); + + foreach (['v4', 'v3', 'v2', 'v1'] as $tag) { + $source->delete($tag); + } + $this->now += 601; + + Assert::same(self::flatten($this->registry()->releases($this->id, $source)), []); + Assert::same($this->stored(), []); + Assert::true($this->storage->load($this->id)->complete); + } + + #[Test] + public function tailIsLoadedOnlyAsFarAsTheConsumerGoes(): void + { + $source = ArrayReleaseSource::ofTags(['v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); self::take($this->registry()->releases($this->id, $source), 4); $source->served = []; - $source->delete('v3'); $this->now += 601; - $pages = $this->registry()->releases($this->id, $source); - $pages->current(); + self::take($this->registry()->releases($this->id, $source), 5); + + Assert::count($source->served, 3); + Assert::same($this->stored(), ['v8', 'v7', 'v6', 'v5', 'v4', 'v3']); + Assert::false($this->storage->load($this->id)->complete); + } - // The anchor misses, and the source fails on the very next page + #[Test] + public function failureWhileLoadingTheTailKeepsWhatWasLoaded(): void + { + $source = ArrayReleaseSource::ofTags(['v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); + self::take($this->registry()->releases($this->id, $source), 4); $source->served = []; - $source->failAfter = 1; + + // The check and two pages of the tail succeed, the next one fails + $source->failAfter = 3; + $this->now += 601; try { - $pages->next(); - Assert::fail('The failure of the reload must reach the caller.'); + self::flatten($this->registry()->releases($this->id, $source)); + Assert::fail('The failure of the tail request must reach the caller.'); } catch (ApiException) { } - // Nothing was dropped: the stored list serves the next run under the same failure - Assert::same($source->served, [2]); - Assert::same(self::tagsOf($this->storage->load($this->id)->releases()), ['v6', 'v5', 'v4', 'v3']); + Assert::same($this->stored(), ['v8', 'v7', 'v6', 'v5', 'v4', 'v3']); + Assert::false($this->storage->load($this->id)->complete); + + $source->failAfter = \PHP_INT_MAX; + $this->now += 601; + Assert::same(self::flatten($this->registry()->releases($this->id, $source)), ['v8', 'v7', 'v6', 'v5', 'v4', 'v3', 'v2', 'v1']); } #[Test] @@ -647,6 +680,14 @@ private static function tagsOf(array $releases): array return \array_map(static fn(ReleaseRecord $release): string => $release->tag, $releases); } + /** + * @return list + */ + private function stored(): array + { + return self::tagsOf($this->storage->load($this->id)?->releases() ?? []); + } + private function registry(bool $refresh = false, ?Logger $logger = null): StoredVersionRegistry { return new StoredVersionRegistry($this->storage, 600, $logger ?? new Logger(), $refresh, fn(): int => $this->now); diff --git a/tests/Unit/Module/Registry/Stub/ArrayReleaseSource.php b/tests/Unit/Module/Registry/Stub/ArrayReleaseSource.php index 58b27fdc..325b97ba 100644 --- a/tests/Unit/Module/Registry/Stub/ArrayReleaseSource.php +++ b/tests/Unit/Module/Registry/Stub/ArrayReleaseSource.php @@ -84,9 +84,8 @@ public function fail(?\Throwable $failure = null): void public function pages(int $offset = 0): \Generator { - // Align with a page boundary and skip within the page, like a real paged API + // Whole pages from the one holding the offset, like a real paged API $page = \intdiv($offset, $this->perPage); - $skip = $offset % $this->perPage; do { $this->failure === null or throw $this->failure; @@ -95,21 +94,23 @@ public function pages(int $offset = 0): \Generator $this->served[] = $page * $this->perPage; $items = \array_slice($this->releases, $page * $this->perPage, $this->perPage); - // Like a real API, a page past the end of the listing is no page at all + // A page past the end of the listing is no page at all, so the listing is served + // from its last page if ($items === [] && $page > 0) { - return; + --$page; + $last = false; + continue; } $last = ($page + 1) * $this->perPage >= \count($this->releases); - $readable = fn(array $releases): array => \array_values(\array_filter( - $releases, + $readable = \array_values(\array_filter( + $items, fn(ReleaseRecord $release): bool => !\in_array($release->tag, $this->unreadable, true), )); // Unreadable releases hold their positions on the page, so the offset counts them - yield new ReleasePage($readable(\array_slice($items, $skip)), $last, \count($items) - \count($readable($items))); + yield new ReleasePage($readable, $last, \count($items) - \count($readable), $page * $this->perPage); - $skip = 0; ++$page; } while (!$last); } diff --git a/tests/Unit/Module/Repository/Internal/GitHub/GitHubRepositoryTest.php b/tests/Unit/Module/Repository/Internal/GitHub/GitHubRepositoryTest.php index 2da5991b..e1ef9586 100644 --- a/tests/Unit/Module/Repository/Internal/GitHub/GitHubRepositoryTest.php +++ b/tests/Unit/Module/Repository/Internal/GitHub/GitHubRepositoryTest.php @@ -102,11 +102,11 @@ public function secondRunIsServedFromTheRegistryWithoutRequests(): void } #[Test] - public function shortPageBelowTheFirstOneDoesNotChangeThePageSize(): void + public function firstPageShortOfAFewReleasesDoesNotChangeThePageSize(): void { - // The first run stores two full pages + // Page 1 comes back one release short, as a server filtering after paging answers $storage = new InMemoryRegistryStorage(); - $firstClient = new PagedClientStub(pages: 4); + $firstClient = new PagedClientStub(pages: 4, withheld: ['v1.0.3']); $seen = 0; foreach (self::createRepository($firstClient, self::registry($storage))->getReleases() as $release) { unset($release); @@ -115,15 +115,13 @@ public function shortPageBelowTheFirstOneDoesNotChangeThePageSize(): void } } - // Page 3 comes back one release short, as a server filtering after paging answers; the - // fresh record is extended from the stored count and page 4 keeps its position - $secondClient = new PagedClientStub(pages: 4, withheld: ['v1.0.250']); - $all = self::names(self::createRepository($secondClient, self::registry($storage))); + // Every page keeps its place: the tail continues on page 2, after the last stored release + $secondClient = new PagedClientStub(pages: 4, withheld: ['v1.0.3']); + $all = self::names(self::createRepository($secondClient, self::registry($storage, refresh: true))); - Assert::same($secondClient->requestedPages(), [3, 4]); + Assert::same($secondClient->requestedPages(), [1, 2, 3, 4]); Assert::same(\count($all), 399); - Assert::true(\in_array('v1.0.201', $all, true)); - Assert::true(\in_array('v1.0.300', $all, true)); + Assert::same(\count(\array_unique($all)), 399); } #[Test] @@ -190,12 +188,13 @@ public function olderReleasesAreLoadedFromTheApiWhenTheRegistryRunsOut(): void break; } - // The second run needs everything: the stored page costs nothing, the rest is fetched + // The second run needs everything: the stored page is served without a request, and + // fetched once more only to find where the tail continues $secondClient = new PagedClientStub(pages: 3); $all = self::names(self::createRepository($secondClient, self::registry($storage))); Assert::same($firstClient->requestedPages(), [1]); - Assert::same($secondClient->requestedPages(), [2, 3]); + Assert::same($secondClient->requestedPages(), [1, 2, 3]); Assert::same(\count($all), 300); } @@ -285,11 +284,12 @@ public function serverCappingThePageSizeIsAskedForEveryPageOnce(): void // The first page tells the page size, so the tail continues on page 2, not on page 1 again Assert::same($firstClient->requestedPages(), [1, 2]); - // A later run learns the page size from the first page anew and jumps to the right one + // A later run learns the page size from the first page anew and jumps to the page + // ending with the last stored release $secondClient = new PagedClientStub(pages: 3, releasesPerPage: 2, perPageLimit: 2); $all = self::names(self::createRepository($secondClient, self::registry($storage))); - Assert::same($secondClient->requestedPages(), [1, 3]); + Assert::same($secondClient->requestedPages(), [1, 2, 3]); Assert::same(\count($all), 6); Assert::same(\count(\array_unique($all)), 6); } diff --git a/tests/Unit/Module/Repository/Internal/GitLab/GitLabRepositoryTest.php b/tests/Unit/Module/Repository/Internal/GitLab/GitLabRepositoryTest.php index 49c806d5..c96089d5 100644 --- a/tests/Unit/Module/Repository/Internal/GitLab/GitLabRepositoryTest.php +++ b/tests/Unit/Module/Repository/Internal/GitLab/GitLabRepositoryTest.php @@ -116,7 +116,7 @@ public function olderReleasesAreLoadedFromTheApiWhenTheRegistryRunsOut(): void $all = self::names(self::createRepository($secondClient, self::registry($storage))); Assert::same($firstClient->requestedPages(), [1]); - Assert::same($secondClient->requestedPages(), [2, 3]); + Assert::same($secondClient->requestedPages(), [1, 2, 3]); Assert::same(\count($all), 300); } diff --git a/tests/Unit/Module/Repository/Internal/PagedReleaseSourceTest.php b/tests/Unit/Module/Repository/Internal/PagedReleaseSourceTest.php index 51bfdfe1..e258feda 100644 --- a/tests/Unit/Module/Repository/Internal/PagedReleaseSourceTest.php +++ b/tests/Unit/Module/Repository/Internal/PagedReleaseSourceTest.php @@ -23,15 +23,17 @@ final class PagedReleaseSourceTest private array $requested = []; #[Test] - public function offsetIsMappedOntoThePageAndThePositionInIt(): void + public function pagesStartWithTheOneHoldingTheOffset(): void { $source = $this->source(total: 250, perPage: 100); $pages = \iterator_to_array($source->pages(150), false); Assert::same($this->requested, [2, 3]); - Assert::same(self::tagsOf($pages[0]), self::tags(150, 200)); + Assert::same(self::tagsOf($pages[0]), self::tags(100, 200)); + Assert::same($pages[0]->offset, 100); Assert::same(self::tagsOf($pages[1]), self::tags(200, 250)); + Assert::same($pages[1]->offset, 200); Assert::true($pages[1]->last); } @@ -50,6 +52,19 @@ public function pagesAreRequestedOnlyWhenReached(): void Assert::same($this->requested, [1, 2]); } + #[Test] + public function listingEndingBeforeTheOffsetStartsWithItsLastPage(): void + { + $source = $this->source(total: 250, perPage: 100); + + $pages = \iterator_to_array($source->pages(450), false); + + Assert::same($this->requested, [5, 4, 3]); + Assert::count($pages, 1); + Assert::same($pages[0]->offset, 200); + Assert::true($pages[0]->last); + } + #[Test] public function emptyListingYieldsNoPage(): void { @@ -57,11 +72,11 @@ public function emptyListingYieldsNoPage(): void Assert::same(\iterator_to_array($source->pages(), false), []); Assert::same(\iterator_to_array($source->pages(150), false), []); - Assert::same($this->requested, [1, 2]); + Assert::same($this->requested, [1, 2, 1]); } #[Test] - public function shortFirstPageTellsThePageSizeAndTheOffsetIsMappedAgain(): void + public function smallFirstPageTellsThePageSize(): void { // The server puts two releases on a page whatever is asked for $source = $this->source(total: 10, perPage: 100, served: 2); @@ -70,8 +85,8 @@ public function shortFirstPageTellsThePageSizeAndTheOffsetIsMappedAgain(): void // Page 1 was fetched on the assumption of a hundred per page; it showed the real size Assert::same($this->requested, [1, 3, 4, 5]); - Assert::same(self::tagsOf($pages[0]), self::tags(5, 6)); - Assert::same(self::tagsOf($pages[1]), self::tags(6, 8)); + Assert::same(self::tagsOf($pages[0]), self::tags(4, 6)); + Assert::same($pages[0]->offset, 4); // The size learned serves the next listing of the same source $this->requested = []; @@ -79,6 +94,19 @@ public function shortFirstPageTellsThePageSizeAndTheOffsetIsMappedAgain(): void Assert::same($this->requested, [5]); } + #[Test] + public function firstPageShortOfAFewReleasesKeepsThePageSize(): void + { + // A release filtered out after paging, not a smaller page + $source = $this->source(total: 300, perPage: 100, missing: ['v3']); + + $source->pages()->current(); + $pages = \iterator_to_array($source->pages(200), false); + + Assert::same($this->requested, [1, 3]); + Assert::same($pages[0]->offset, 200); + } + #[Test] public function shortPageBelowTheFirstOneIsNotTakenForThePageSize(): void { @@ -91,21 +119,15 @@ public function shortPageBelowTheFirstOneIsNotTakenForThePageSize(): void } #[Test] - public function unreadableReleasesOfThePageCountTowardsTheSkippedPositions(): void + public function unreadableReleasesHoldTheirPositionsOnThePage(): void { $source = $this->source(total: 300, perPage: 100, unreadable: ['v102']); - $pages = \iterator_to_array($source->pages(105), false); - - // Position 105 is the fourth readable release of the page, not the fifth - Assert::same($this->requested, [2, 3]); - Assert::same(self::tagsOf($pages[0])[0], 'v105'); - Assert::same($pages[0]->skipped, 1); + $page = $source->pages(105)->current(); - // An unreadable release after the position makes the page start one release early, - // which the registry filters as a known one, rather than lose a release - $source = $this->source(total: 300, perPage: 100, unreadable: ['v180']); - Assert::same(self::tagsOf(\iterator_to_array($source->pages(105), false)[0])[0], 'v104'); + Assert::same($page->offset, 100); + Assert::same($page->skipped, 1); + Assert::count($page->releases, 99); } #[BeforeTest]