From e081443d6beba4141e69b481a68487c0bb0c10e6 Mon Sep 17 00:00:00 2001 From: Bob Date: Tue, 15 Sep 2026 14:14:34 +0200 Subject: [PATCH] fix: collapse a bulk invalidation's URL-map writes into one per domain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Statamic keeps every cached URL for a domain in a single cache entry, and AbstractCacher::invalidateUrls() walks the list one URL at a time. Each call reads that entry, and each match calls forgetUrl(), which reads it again and writes the whole thing back. Clearing n URLs out of a map of m cost O(n*m). That is the graph's own precision turned into work: targeted invalidation hands the cacher a long list of individually correct URLs, which is the point, and the per-URL rewrite makes a longer list quadratically more expensive. On a site with a couple of thousand cached URLs the map is a few hundred kilobytes, every write takes an exclusive lock on one file, and live traffic rewrites the same entry on every uncached render. The queued Invalidate job ran past its timeout, was killed part-way through the list, and every URL it had not reached went on serving the old page. Nothing reported it: the only trace was a TimeoutExceededException in failed_jobs, while the editor saw a saved entry that never appeared. Buffering getUrls() for the duration of a pass leaves the parent's matching, response deletion and event dispatch exactly as they are — nothing here knows what a cached response looks like — while turning m reads and writes into one read and one write per domain actually touched. Also stop handing the cacher URLs that are not cached any more. The graph outlives the cache by design, so a tag routinely resolves to two or three times as many URLs as the cache holds, and each of those costs a lookup that can only miss. A cacher whose contents cannot be enumerated still receives the full set: an empty list there means "unknown", not "nothing is cached", and narrowing against it would clear nothing at all. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 77 +++++--- README.md | 7 +- src/Cachers/BatchesInvalidation.php | 146 ++++++++++++++ src/Cachers/TrackingApplicationCacher.php | 1 + src/Cachers/TrackingFileCacher.php | 1 + src/Invalidation/GraphInvalidator.php | 35 +++- tests/Cachers/BatchesInvalidationTest.php | 178 ++++++++++++++++++ .../Invalidation/NarrowsToCachedUrlsTest.php | 92 +++++++++ 8 files changed, 502 insertions(+), 35 deletions(-) create mode 100644 src/Cachers/BatchesInvalidation.php create mode 100644 tests/Cachers/BatchesInvalidationTest.php create mode 100644 tests/Invalidation/NarrowsToCachedUrlsTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index b5d99e1..98d6b9b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,36 +2,7 @@ All notable changes to this project will be documented in this file. -## [Unreleased] - -### Fixed - -- A host app whose `statamic.static_caching.invalidation.rules` is the string - `all` no longer fatals the moment static caching is switched on. Statamic - documents that value and checks for exactly it in - `DefaultInvalidator::invalidate()`, so a stock config hit - `TypeError: GraphInvalidator::__construct(): Argument #2 ($rules) must be of - type ?array, string given` during boot. Any non-array value is now normalised - to `[]`: `all` means flush everything on any save, which is precisely the - behaviour the graph replaces, so it must not reach the parent. -- Globals no longer come back from the cache as `__PHP_Incomplete_Class`. Laravel - unserializes cache payloads against an allow list when - `cache.serializable_classes` holds an array, and Statamic fills that array with - its own classes. Binding `TrackingVariables` over the `Variables` contract meant - the globals store cached items of a class nobody had allowed, so the next method - call on a global set fatalled — on every page, since layouts read globals. The - addon now adds its own cached classes to that allow list, leaving the host's - entries and the unrestricted `null`/`true` settings untouched. -- Custom query scopes now work on the tracking entry and term query builders. - Statamic keys its scope registry on the exact builder class, so a scope a site - registered against `Stache\Query\EntryQueryBuilder` was invisible to the - subclass this addon substitutes, and calling it fatally threw - `BadMethodCallException: Call to undefined method ...::yourScope()`. The - ordering was not a fluke: addons boot inside `$app->booted()`, so a site's own - `boot()` always registers its scopes before this addon rebinds the builder. A - scope registered against any ancestor of a tracking builder now applies to it. - -## [2.0.0] - 2026-08-06 +## [2.0.0] - 2026-09-15 Invalidation is now derived from what pages actually read, instead of from rules describing what they might read. There is no configuration to write. @@ -113,6 +84,52 @@ renders; v2 observes it rather than restating it. ### Fixed +- A save no longer leaves pages stale when it clears a large number of URLs. + Statamic keeps one URL map per domain and rewrites all of it for every URL it + clears, so invalidating n URLs out of a map of m cost O(n*m). On a site with a + few thousand cached URLs that is a few hundred kilobytes rewritten thousands of + times per save, each write taking an exclusive lock that live traffic is + contending for on every uncached render. The queued + `Statamic\StaticCaching\Invalidate` job ran past its timeout and was killed + part-way through the list, so every URL it had not reached yet went on serving + the old page. Nothing said so: the only trace was a `TimeoutExceededException` + in `failed_jobs`, while the editor saw a saved entry that never appeared. The + map is now read once and written once per domain actually touched, whatever the + size of the batch — Statamic's own matching, response deletion and events are + left exactly as they are. The `full` strategy still scans its cache directory + once per URL, which is Statamic's own `FileCacher` and unchanged here. +- Invalidation no longer hands the cacher URLs that are not cached any more. The + graph outlives the cache by design — rows are pruned as they are invalidated, + not when a page falls out — so on a site that has been up for a while a tag + routinely resolves to two or three times as many URLs as the cache holds, and + every one of those costs a lookup that can only miss. They are now filtered out + before the cacher sees them. A cacher whose contents cannot be enumerated still + receives the full set: an empty list there means "unknown", not "nothing is + cached", and narrowing against it would clear nothing at all. +- A host app whose `statamic.static_caching.invalidation.rules` is the string + `all` no longer fatals the moment static caching is switched on. Statamic + documents that value and checks for exactly it in + `DefaultInvalidator::invalidate()`, so a stock config hit + `TypeError: GraphInvalidator::__construct(): Argument #2 ($rules) must be of + type ?array, string given` during boot. Any non-array value is now normalised + to `[]`: `all` means flush everything on any save, which is precisely the + behaviour the graph replaces, so it must not reach the parent. +- Globals no longer come back from the cache as `__PHP_Incomplete_Class`. Laravel + unserializes cache payloads against an allow list when + `cache.serializable_classes` holds an array, and Statamic fills that array with + its own classes. Binding `TrackingVariables` over the `Variables` contract meant + the globals store cached items of a class nobody had allowed, so the next method + call on a global set fatalled — on every page, since layouts read globals. The + addon now adds its own cached classes to that allow list, leaving the host's + entries and the unrestricted `null`/`true` settings untouched. +- Custom query scopes now work on the tracking entry and term query builders. + Statamic keys its scope registry on the exact builder class, so a scope a site + registered against `Stache\Query\EntryQueryBuilder` was invisible to the + subclass this addon substitutes, and calling it fatally threw + `BadMethodCallException: Call to undefined method ...::yourScope()`. The + ordering was not a fluke: addons boot inside `$app->booted()`, so a site's own + `boot()` always registers its scopes before this addon rebinds the builder. A + scope registered against any ancestor of a tracking builder now applies to it. - `Invalidator::refresh()` is honoured. `DefaultInvalidator` flips its `$refreshing` flag before delegating to `invalidate()`, which 1.x overrode without checking, so `statamic.static_caching.background_recache` hard-purged instead of refreshing. diff --git a/README.md b/README.md index 3f87ebd..dc95cd0 100644 --- a/README.md +++ b/README.md @@ -353,9 +353,10 @@ listed so you know where the edges are. ## Good to know -- Invalidation is one indexed lookup plus the deletes. Nothing walks content, which - matters with a single queue worker or `QUEUE_CONNECTION=sync`, where it runs inside - the editor's save request. +- Invalidation is one indexed lookup plus the deletes, and the URL map Statamic keeps + per domain is read and written once per pass rather than once per URL cleared. + Nothing walks content, which matters with a single queue worker or + `QUEUE_CONNECTION=sync`, where it runs inside the editor's save request. - A page with more than 2,000 dependencies is treated as depending on everything. - Globals are not scoped per site, so on a multisite install saving one clears the pages that read it across every site. diff --git a/src/Cachers/BatchesInvalidation.php b/src/Cachers/BatchesInvalidation.php new file mode 100644 index 0000000..244bb58 --- /dev/null +++ b/src/Cachers/BatchesInvalidation.php @@ -0,0 +1,146 @@ + URL map, filled lazily while a batch is in flight. Null whenever + * no batch is running, which is what every other caller sees. + * + * @var array|null + */ + private ?array $bufferedUrls = null; + + /** + * Domains whose buffered map was actually modified. A pass that matches + * nothing must not write the map back. + * + * @var array + */ + private array $dirtyDomains = []; + + /** + * @param array $urls + * @return void + */ + public function invalidateUrls($urls) + { + $this->whileBuffering(fn () => parent::invalidateUrls($urls)); + } + + /** + * Refreshing never writes the map — it reads it to find URLs to warm — but it + * reads it once per URL, so the same buffer removes that too. + * + * @param array $urls + * @return void + */ + public function refreshUrls($urls) + { + $this->whileBuffering(fn () => parent::refreshUrls($urls)); + } + + /** + * @param string|null $domain + * @return Collection + */ + public function getUrls($domain = null) + { + if ($this->bufferedUrls === null) { + return parent::getUrls($domain); + } + + return $this->bufferedUrls[$this->bufferKey($domain)] ??= parent::getUrls($domain); + } + + /** + * @param string $key + * @param string|null $domain + * @return void + */ + public function forgetUrl($key, $domain = null) + { + if ($this->bufferedUrls === null) { + parent::forgetUrl($key, $domain); + + return; + } + + // Collection::forget() mutates, so this updates the buffered map in place + // and the parent's next getUrls() in the same pass sees the removal. + $this->getUrls($domain)->forget($key); + + $this->dirtyDomains[$this->bufferKey($domain)] = true; + } + + private function whileBuffering(callable $callback): void + { + // A nested call is already inside a batch. Letting it run against the open + // buffer keeps one flush at the end instead of writing a half-finished map. + if ($this->bufferedUrls !== null) { + $callback(); + + return; + } + + $this->bufferedUrls = []; + $this->dirtyDomains = []; + + try { + $callback(); + } finally { + $buffered = $this->bufferedUrls; + $dirty = $this->dirtyDomains; + + // Closed before the writes so they, and anything reading the map + // afterwards, take the ordinary unbuffered path. In a finally block + // because a throw part-way through still has to persist the deletions + // already made — the alternative is forgetting them and serving pages + // whose responses are gone. + $this->bufferedUrls = null; + $this->dirtyDomains = []; + + foreach (array_keys($dirty) as $domain) { + $this->cache->forever($this->getUrlsCacheKey($domain), $buffered[$domain]->all()); + } + } + } + + /** + * @param string|null $domain + */ + private function bufferKey($domain): string + { + // Matches how getUrls() and getUrlsCacheKey() resolve a null domain, so a + // pass that mixes null and explicit domains still shares one buffer entry. + return $domain ?: $this->getBaseUrl(); + } +} diff --git a/src/Cachers/TrackingApplicationCacher.php b/src/Cachers/TrackingApplicationCacher.php index eee8794..fc5d13b 100644 --- a/src/Cachers/TrackingApplicationCacher.php +++ b/src/Cachers/TrackingApplicationCacher.php @@ -11,5 +11,6 @@ */ final class TrackingApplicationCacher extends ApplicationCacher { + use BatchesInvalidation; use RecordsDependencies; } diff --git a/src/Cachers/TrackingFileCacher.php b/src/Cachers/TrackingFileCacher.php index 0dfa412..ae10323 100644 --- a/src/Cachers/TrackingFileCacher.php +++ b/src/Cachers/TrackingFileCacher.php @@ -11,5 +11,6 @@ */ final class TrackingFileCacher extends FileCacher { + use BatchesInvalidation; use RecordsDependencies; } diff --git a/src/Invalidation/GraphInvalidator.php b/src/Invalidation/GraphInvalidator.php index 32cffe8..a1bc7e5 100644 --- a/src/Invalidation/GraphInvalidator.php +++ b/src/Invalidation/GraphInvalidator.php @@ -81,14 +81,45 @@ public function refresh($item): void public function invalidate($item): void { $tags = $this->tags->forItem($item); + $cached = $this->cached->all(); $this->clear([ ...$this->getItemUrls($item), - ...$tags === [] ? [] : $this->graph->urlsFor([...$tags, Tag::OVERFLOW]), - ...$this->graph->untracked($this->cached->all()), + ...$tags === [] ? [] : $this->stillCached($this->graph->urlsFor([...$tags, Tag::OVERFLOW]), $cached), + ...$this->graph->untracked($cached), ]); } + /** + * The graph outlives the cache: a URL keeps its row after its cached copy is + * gone, and rows are only pruned as they are invalidated. On a site that has + * been up for a while most of what a tag resolves to is no longer cached — + * often two thirds of it — and every one of those costs the cacher a lookup + * that can only miss. Narrowing here is what makes the remaining work match + * the number of pages that actually have to be cleared. + * + * Deliberately not applied to getItemUrls(): those are Statamic's own, may be + * wildcards, and are not graph rows to begin with. + * + * @param list $urls + * @param list $cached + * @return list + */ + private function stillCached(array $urls, array $cached): array + { + // An empty list is ambiguous. It means "nothing is cached" on a site that + // was just flushed, but it equally means "this cacher cannot be + // enumerated" — CachedUrls gives up on anything that is not an + // AbstractCacher, which includes a host app's own cacher. Narrowing + // against it would then clear nothing at all, the one failure this addon + // exists to prevent. Hand the full set over and let the cacher decide. + if ($cached === []) { + return $urls; + } + + return array_values(array_intersect($urls, $cached)); + } + /** * @param list $urls */ diff --git a/tests/Cachers/BatchesInvalidationTest.php b/tests/Cachers/BatchesInvalidationTest.php new file mode 100644 index 0000000..1ab7759 --- /dev/null +++ b/tests/Cachers/BatchesInvalidationTest.php @@ -0,0 +1,178 @@ +cacher = app(Cacher::class); + $this->domain = $this->cacher->getBaseUrl(); + } + + #[Test] + public function it_writes_the_url_map_once_however_many_urls_are_cleared(): void + { + foreach (range(1, 50) as $i) { + $this->cache("/page-{$i}"); + } + + $writes = $this->urlMapWritesDuring(fn () => $this->cacher->invalidateUrls( + array_map(fn (int $i): string => $this->domain."/page-{$i}", range(1, 40)), + )); + + $this->assertSame(1, $writes); + $this->assertCachedIs(array_map(fn (int $i): string => "/page-{$i}", range(41, 50))); + } + + #[Test] + public function it_does_not_rewrite_the_url_map_when_nothing_matched(): void + { + $this->cache('/kept'); + + $writes = $this->urlMapWritesDuring(fn () => $this->cacher->invalidateUrls([ + $this->domain.'/never-cached', + ])); + + $this->assertSame(0, $writes); + $this->assertCachedIs(['/kept']); + } + + #[Test] + public function it_clears_query_string_variants_in_the_same_pass(): void + { + // The reason a single save can name thousands of URLs: every allowed query + // string is its own cache entry, and a crawled paginated listing produces + // hundreds of them under one path. + $this->cache('/list'); + $this->cache('/list?page=2'); + $this->cache('/list?page=3'); + $this->cache('/other'); + + $writes = $this->urlMapWritesDuring(fn () => $this->cacher->invalidateUrls([ + $this->domain.'/list', + ])); + + $this->assertSame(1, $writes); + $this->assertCachedIs(['/other']); + } + + #[Test] + public function it_writes_one_url_map_per_domain_touched(): void + { + $other = 'https://other.test'; + + $this->cache('/a'); + $this->cache('/b'); + $this->cache('/a', $other); + $this->cache('/b', $other); + + $writes = $this->urlMapWritesDuring(fn () => $this->cacher->invalidateUrls([ + $this->domain.'/a', + $other.'/a', + ])); + + $this->assertSame(2, $writes); + $this->assertCachedIs(['/b']); + $this->assertCachedIs(['/b'], $other); + } + + #[Test] + public function it_still_clears_wildcard_urls(): void + { + // Wildcards go through the parent's own matching, which resolves them to + // individual URLs and forgets each one. Those forgets have to land in the + // same buffer as everything else. + $this->cache('/blog/one'); + $this->cache('/blog/two'); + $this->cache('/other'); + + $writes = $this->urlMapWritesDuring(fn () => $this->cacher->invalidateUrls([ + $this->domain.'/blog/*', + ])); + + $this->assertSame(1, $writes); + $this->assertCachedIs(['/other']); + } + + #[Test] + public function refreshing_reads_the_url_map_without_writing_it(): void + { + Queue::fake(); + + $this->cache('/a'); + $this->cache('/b'); + + $writes = $this->urlMapWritesDuring(fn () => $this->cacher->refreshUrls([ + $this->domain.'/a', + $this->domain.'/b', + ])); + + // A refresh warms pages back up rather than dropping them, so the map is + // unchanged and must not be written at all. + $this->assertSame(0, $writes); + $this->assertCachedIs(['/a', '/b']); + } + + /** + * Writes of a URL map, as opposed to the many small per-response keys that + * a pass legitimately touches. Statamic names them `.urls`. + */ + private function urlMapWritesDuring(callable $callback): int + { + $writes = 0; + + Event::listen(KeyWritten::class, function (KeyWritten $event) use (&$writes): void { + if (str_ends_with($event->key, '.urls')) { + $writes++; + } + }); + + $callback(); + + return $writes; + } + + private function cache(string $path, ?string $domain = null): void + { + $this->cacher->cacheUrl(md5($path), $path, $domain ?? $this->domain); + } + + /** + * @param list $expected + */ + private function assertCachedIs(array $expected, ?string $domain = null): void + { + $this->assertEqualsCanonicalizing( + $expected, + $this->cacher->getUrls($domain ?? $this->domain)->values()->all(), + ); + } +} diff --git a/tests/Invalidation/NarrowsToCachedUrlsTest.php b/tests/Invalidation/NarrowsToCachedUrlsTest.php new file mode 100644 index 0000000..6927621 --- /dev/null +++ b/tests/Invalidation/NarrowsToCachedUrlsTest.php @@ -0,0 +1,92 @@ +article(); + + $cacher = app(Cacher::class); + $domain = $cacher->getBaseUrl(); + + // Cached after the save, so the save's own invalidation cannot clear it. + $cacher->cacheUrl(md5('/still-cached'), '/still-cached', $domain); + + $this->graph->record($domain.'/still-cached', ["entry:{$entry->id()}"]); + $this->graph->record($domain.'/long-gone', ["entry:{$entry->id()}"]); + + $handed = $this->urlsHandedToCacher($entry, new CachedUrls($cacher)); + + $this->assertContains($domain.'/still-cached', $handed); + $this->assertNotContains($domain.'/long-gone', $handed); + } + + #[Test] + public function it_hands_over_everything_when_the_cache_cannot_be_enumerated(): void + { + // CachedUrls gives up on any cacher that is not an AbstractCacher, and an + // empty list is indistinguishable from a freshly flushed site. Reading it + // as "nothing is cached" would clear nothing at all on a host app's own + // cacher — silently serving stale pages, the one failure that matters. + $entry = $this->article(); + + $domain = app(Cacher::class)->getBaseUrl(); + + $this->graph->record($domain.'/long-gone', ["entry:{$entry->id()}"]); + + $handed = $this->urlsHandedToCacher($entry, new CachedUrls(Mockery::mock(Cacher::class))); + + $this->assertContains($domain.'/long-gone', $handed); + } + + private function article(): EntryContract + { + Collection::make('articles')->save(); + + return tap(Entry::make()->collection('articles')->slug('one')->data(['title' => 'One']))->save(); + } + + /** + * Invalidates through a cacher that only records its arguments, so the + * assertion is about the list itself and not about what survived in a cache. + * + * @return list + */ + private function urlsHandedToCacher(EntryContract $entry, CachedUrls $cached): array + { + $handed = []; + + $cacher = Mockery::mock(Cacher::class); + $cacher->shouldReceive('invalidateUrls')->once()->with(Mockery::capture($handed)); + + (new GraphInvalidator($cacher, [], $this->graph, app(TagResolver::class), $cached)) + ->invalidate($entry); + + return $handed; + } +}