Skip to content

fix: 🐛 Self-heal cache when corrupted non-Collection value is detected.#207

Merged
mikebronner merged 1 commit into
masterfrom
fix-cache-corruption
May 18, 2026
Merged

fix: 🐛 Self-heal cache when corrupted non-Collection value is detected.#207
mikebronner merged 1 commit into
masterfrom
fix-cache-corruption

Conversation

@mikebronner
Copy link
Copy Markdown
Member

@mikebronner mikebronner commented May 18, 2026

Summary

Fixes #206removeEmptyCacheEntry() throws TypeError when a cached entry can't be unserialized cleanly.

TypeError: Geocoder\Laravel\ProviderAndDumperAggregator::removeEmptyCacheEntry():
Argument #1 ($result) must be of type Illuminate\Support\Collection,
__PHP_Incomplete_Class given

Why this was happening

Cached entries are stored as ["key" => $cacheKey, "value" => Collection]. On read, the package trusted Laravel's cache to return that shape back intact and passed $result["value"] straight to removeEmptyCacheEntry(), which had a strict Collection type hint. When the cached value came back as anything else — most commonly __PHP_Incomplete_Class — the type assertion failed and the request blew up.

PHP returns __PHP_Incomplete_Class from unserialize() when the serialized blob references a class it can't reify at read time. There are several realistic triggers:

  1. MySQL column truncation on the cache table. Laravel's cache:table migration uses mediumText (~16 MB), but older or hand-rolled tables may use text (~64 KB). A Geocoder Collection holding multi-result Address objects (admin levels, bounds, coords, country, locality, etc.) can exceed 64 KB, particularly for chained providers. MySQL silently truncates the row, the blob comes back malformed, and unserialize produces __PHP_Incomplete_Class for partially-reconstructed items. This is consistent with the reporter's symptoms in Started receiving errors from removeEmptyCacheEntry() #206: worked for several days post-upgrade (until a large enough result was cached), surfaced "randomly" when cache cleaning ran (expired big entries → next read of a refilled-and-truncated row trips it), and disappeared on Redis (no column-size constraint).

  2. Class-definition drift across upgrades. Old serialized blobs reference class shapes (private property sets, namespaces) that no longer match current definitions. Unserialize falls back to __PHP_Incomplete_Class. Switching cache stores looks like a fix because the new store starts empty — nothing from before the upgrade is in there.

  3. Partial/concurrent writes in the database driver. Theoretically possible, much less likely than the above.

In all three cases the package's job isn't to prevent the corruption — that's outside our control — it's to recover gracefully.

What this PR changes

src/ProviderAndDumperAggregator.php:

  • preventCacheKeyHashCollision() now validates the full shape of the cached entry: must be an array with key and value keys, key must match the expected cache key, AND value must be a Collection. Any validation failure routes through the existing recovery path: forget() the bad entry and re-fetch. The parameter type is relaxed from array to mixed so a non-array cache return doesn't TypeError before validation can run.

  • removeEmptyCacheEntry() parameter type is relaxed from Collection to mixed as belt-and-suspenders, with an explicit instanceof Collection early-return. The upstream validation should mean this is never reached with a non-Collection, but a future code path that misses the validation can't reintroduce the original TypeError.

Effect for users

  • No code change required on upgrade.
  • No manual php artisan cache:clear required.
  • The first request that hits a corrupted entry self-heals: the bad entry is forgotten, a fresh request runs, and the new Collection replaces it. Subsequent requests are served from the healed cache as normal.

Test coverage

New Pest test in tests/Feature/Providers/GeocoderServiceTest.php:

  • it self-heals when a cached entry has a non-Collection value — pre-poke an entry whose value is a string instead of a Collection (simulating truncation / unserialize drift / __PHP_Incomplete_Class), call geocode(), assert no exception is thrown, a real Collection is returned, and the cache entry is replaced with a healthy one.

Without the fix this test reproduces issue #206 verbatim (same exception class, same parameter, same call site). All 30 existing tests continue to pass.

@mikebronner mikebronner merged commit 68a3831 into master May 18, 2026
10 checks passed
@mikebronner mikebronner deleted the fix-cache-corruption branch May 18, 2026 21:57
@boryn
Copy link
Copy Markdown

boryn commented May 19, 2026

Hi @mikebronner!

Do I read it properly that you serialize and unserialize whole classes in the cache?

Laravel 13 says it's not recommended and if used, the class must be added to exceptions:
https://laravel.com/docs/13.x/upgrade#cache-serializable_classes-configuration

If this is indeed the case, maybe it would be good not to serialize the whole classes, but just to save the cached data as json/array?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Started receiving errors from removeEmptyCacheEntry()

2 participants