Repository navigation
Add rememberWithLock() and fix encryption, namespace, and Redis scan defects (1.15.0) - #57
Merged
Merged
Conversation
Add SmartCache::rememberWithLock(): single-flight regeneration on a cache miss. One process rebuilds the value under an atomic lock while the others poll the cache and return as soon as it is stored. Without lock support, on a lock failure, or after the wait, it behaves like remember(). Fix: - Encryption is tried before chunking and compression, so large values for encrypted keys are no longer stored as plain chunks or gzip. - Encryption keys and patterns match keys written under a namespace. - An undecryptable entry is treated as corrupted instead of a cached null. - asyncSwr()/refreshAsync() jobs write under the caller's namespace, and asyncSwr() queues one refresh per stale window. - memo() keeps the namespace and runtime settings, like store(). - Redis SCAN starts from the cursor Laravel uses; phpredis treated the integer 0 cursor as finished and enumerated no keys (1.14.0 regression). - CompressionStrategy measures small arrays exactly and samples long lists, so API-shaped arrays are compressed. - A failed add() no longer overwrites a live chunked entry's chunks. - Adaptive compression restore uses the validated decoder. - Chunked Eloquent collections keep their class. - Model pattern invalidation no longer applies the namespace twice. - withFallback() calls a Closure fallback. - get()/pull() call Closure defaults; get(array), put(array) and many(['key' => default]) match Laravel's Repository. - OptimizationApplied is dispatched; recent_entries_limit is honoured.
Document rememberWithLock(), encryption precedence, and the fallback requirements; correct documented methods that do not exist on SmartCache; update CHANGELOG, SECURITY.md, and test counts.
- Run the queued refresh callback, not just the write, inside the captured namespace, so a callback that reads through SmartCache sees tenant data. - Restore the stale timestamp when queueing an asyncSwr() refresh fails, so the next request retries instead of waiting out the fresh TTL. - Count array keys in CompressionStrategy's sampled size estimate; long keys with small values were underestimated and skipped compression.
- CompressionStrategy only trusts its sample to confirm a large array; skipping compression now always measures the serialized size, so small leading items cannot hide a large payload. - add()'s existence pre-check reads past memo(), whose remembered hit may be stale after another process removed the entry. - Model pattern invalidation keeps numeric keys such as '123', which the managed-key index returns as integers.
Tags set with tags() stayed active after a read that did not write, so
tags('users')->get('missing') tagged the next unrelated put() and a later
flushTags('users') deleted it. After a tagged read the tags now apply only
to a write of the same key, which keeps the remember() pattern and
"$cache = tags(...); $cache->get() ?? $cache->put()" tagged. swr()/flexible()
tag the key they regenerate, refreshAsync() hands its tags to the job, and
store()/memo() move the active tags to the returned instance.
From the Fable review:
- Dispatch OptimizationApplied under Event::fake() as well as for real
listeners.
- Dismiss the Mockery exception from the trait's getNamespace() probe so a
facade-mocking test is not flagged as risky on PHPUnit 9.
- A rememberWithLock() waiter that saw the value appear reads it once.
- Document the cache_locks table, the compression measuring cost, and fix
the docs example that used flush() to clear tagged entries.
The driver was derived from the store class as "redis_", "memcached_", or "dynamo_db_", so smart-cache.drivers.* never matched, smart sizing never used the Memcached/DynamoDB item limits, and the shared circuit breaker key carried a trailing underscore. Use Laravel's driver name. The shipped drivers.memcached.compression default changes from false to true: the old value never took effect, so Memcached values have always been gzip-compressed and the shipped config keeps that behavior. Applications that published the config keep their own value; the CHANGELOG upgrade note covers it.
With nested memo() instances, existsInStore() only dropped the outer layer's memoized state, so add() and rememberWithLock() could still trust an inner layer's stale hit or miss.
- Track the tag list tags() set. Tags a subclass assigns to $activeTags directly keep the pre-1.15 "apply to the next write" behavior instead of being restricted to an earlier lookup's key. - Calling tags() again with the same list keeps the keys already looked up, so "look up several keys, then write the misses" stays tagged; a write directly after tags() is always tagged. - Correct the docs: a tagged get() hit still clears the tags. - Add a rolling-deploy note for refresh jobs run by 1.14 workers.
The final review showed that scoping tags by default can tag less than 1.14 did: lookups nested in a tagged remember() callback, or in the value expression of a tagged put(), kept the written key from being tagged, so flushTags() would leave stale data. The 1.14 behavior only over-tags. Scoping is now opt-in via smart-cache.tags.scoped (default false); with it off, tags behave exactly as in 1.14, pinned by tests that pass on 1.14.0. In the opt-in mode: - remember*() callbacks run with the outer tags kept, so nested lookups cannot use them up; - store()/memo() take the tags only when called right after tags(). Also from the review: check for Event::fake() before calling hasListeners(), and treat an Event facade mock without that expectation as no listener; correct the rolling-deploy note.
- put()/forever() take their tags before optimizing, so an OptimizationApplied listener that uses the cache cannot take or clear them (the one default-mode tag difference from 1.14). - Cast hasListeners() to bool: Event::spy() returns null, which made an optimized put() throw a TypeError. - Run a Closure default with the caller's tags set aside, so a cache call inside it cannot clear them. - Narrow the CHANGELOG wording about subclasses setting $activeTags.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release 1.15.0: one small, opt-in feature (
rememberWithLock()) plus a set of verified fixes. Nothing in theSmartCachecontract changes, one opt-in config key is added (tags.scoped, defaultfalse), one shipped default changes (see the upgrade notes), and entries written by 1.14 are read unchanged. Old and new releases can read each other's entries during a rolling deploy.vendor/bin/phpunit: 605 passing (531 before this branch, 2 pre-existing skips). That is 74 new tests: 17 forrememberWithLock(), 12 for opt-in scoped tags, and 45 regression tests. The regression tests were checked against the 1.14.0 source: 31 fail there. The other 14 are guards for behavior that must not change: reading old chunked entries, the tagged "read, then write on a miss" pattern, edge-case inputs, and the regressions caught in review.New:
rememberWithLock()remember()lets every concurrent request run the callback when a hot key is missing — after a deploy, a flush, or an invalidation. That is the stampede that takes a database down.rememberWithLock()runs the callback under an atomic lock: one process regenerates, the others wait and then read the stored value.get(), same asremember().has(), so hit/miss metrics are not double-counted, and it also works throughmemo(), which otherwise remembers the miss for the rest of the request.remember()would succeed. If the store has noLockProvider, the lock backend errors (e.g. a database store withoutcache_locks), or the wait times out, the callback runs without the lock._sc_remember_lock:plus a SHA-1 of the namespaced key, so long keys stay within Memcached's 250-byte limit.clearManaged()took in 1.13.1.New: opt-in scoped tags (
smart-cache.tags.scoped)tags()returns the same instance, and its tags stay active until a write uses them. Sotags('users')->get('missing')followed byput('site_settings', ...)tagssite_settings. Fixing that by default is not safe:tags('t')->put('k', $service->build()), wherebuild()reads the cache and misses, looks exactly the same, so any fix would sometimes leavekuntagged, andflushTags()would then serve stale data. The 1.14 behavior only over-invalidates.So this is opt-in, and the default keeps 1.14 behavior exactly (pinned by tests that pass on 1.14.0). New applications can enable it:
remember*()callbacks don't use up the outer tags.swr()/flexible()tag the key they regenerate.store()/memo()right aftertags()take the tags along.The docs explain the one caveat in this mode: compute values before calling
tags().Security
APP_KEY. Encryption is now tried first.namespace()disabled encryption. Keys and patterns were compared against the namespaced key (tenant:user_token_1), so nothing matched under a namespace. Both forms are now checked.asyncSwr()/refreshAsync()wrote a tenant's refreshed data to the global key. The job received the key without its namespace and ran in a worker that has none. The job now carries the namespace and runs both the refresh callback and the write under it, so a callback that reads through SmartCache sees the tenant's data. It then restores the caller's namespace, which also covers the sync queue. Jobs already queued by 1.14 still unserialize (the new property defaults tonull).memo()dropped the namespace, sonamespace('t1')->memo()->get('k')read the globalk.memo()now keeps the namespace and the other runtime settings, asstore()already did.Fixes
0cursor as "finished", sosmart-cache:clear --force,status --forceandauditsaw an empty keyspace. The scan now starts from the cursor Laravel's ownRedisStoreuses. Verified against a real Redis with phpredis 6.2.0: 0 of 2,500 keys found before the fix, 2,500 after. (Prefix handling is unchanged; see "Not in this PR".)CompressionStrategyestimated an array's size as 50 bytes per top-level item. A 315 KB['status' => ..., 'records' => [...]]therefore stayed uncompressed, while 2,500 small integers (32 KB, below the threshold) were compressed. An array is now skipped only after its serialized size has been measured; a quick sample of a few entries is used only to confirm that a long array is well above the threshold.add()corrupted a live chunked entry. Chunk keys are derived from the cache key and were written before the atomic add was attempted. For array and collection values,add()now bails out first when the key exists. This costs one extra existence check on those calls only; scalar flags and markers are unaffected.APP_KEYrotation) was served as a cachednull, permanently forrememberForever(). It is now treated as corrupted, so self-healing evicts it andremember()regenerates it.false. The adaptive strategy now shares the validated decoder ofCompressionStrategy; the storage format is the same.Eloquent\Collections came back asSupport\Collection, which brokeload()andmodelKeys()above about 1,000 rows. The class is now recorded and restored. Old entries without the class still restore asSupport\Collection.CacheInvalidationService.withFallback()returned the fallbackClosureinstead of calling it.Repositoryparity:get()andpull()now call aClosuredefault.get(array)andput(array, $ttl)now behave likemany()andputMany().many(['key' => $default])now works.MemoizedCacheDriver::getMultiple()now honours$default.asyncSwr()no longer queues a new refresh job on every request during the stale window. If queueing fails, the stale timestamp is put back so the next request retries.drivers.*config never applied. The driver was derived asredis_/memcached_/dynamo_db_(trailing underscore). It is now Laravel's driver name, so per-driver settings, the smart-sizing item limits, and the shared circuit-breaker key work as documented. The shippeddrivers.memcached.compressiondefault changes fromfalsetotrue. The oldfalsenever took effect, so this keeps today's behavior for everyone on the shipped config.OptimizationAppliedis now dispatched. It was documented but never fired. Events stay off by default, and sizes are only measured when a listener is registered.monitoring.recent_entries_limitis now honoured; the limit was hard-coded to 100, which remains the default.Upgrade notes (also in CHANGELOG)
config/smart-cache.phponly: their copy still says'compression' => falsefor Memcached, and that now takes effect. SmartCache stops gzip-compressing Memcached values and relies on php-memcached's own compression. Set it totrueto keep the previous behavior.Docs
README,
docs/index.html, CHANGELOG (1.15.0), SECURITY.md (maintained line → 1.15.x), and test counts are updated. The docs also had these errors, now fixed:optimize(),getCompressionStats(),getSerializationStats()), each of which throwsBadMethodCallException.OptimizationAppliedexample treatedratioas a percentage.Not in this PR (need a decision first)
The audit found more issues. I left them out on purpose: fixing each one changes runtime or storage behavior for existing production setups. (The tag leak and the
drivers.*config, originally on this list, are now fixed above.)clear --force/status/audit. Keys come back with the connection and cache prefix, so they never match. Fixing this makesclear --forceactually delete non-managed keys for the first time.forever()value leaves the old trailing chunks behind.Review
Besides the test suite, the diff had an independent Claude review and a Codex review (
codex review --base main). Each finding below is fixed with a test that fails without the fix.First Codex pass:
asyncSwr()suppressed retries when queueing failed.Fable review (no P1/P2 findings). The P3 suggestions are applied:
OptimizationAppliedis visible toEvent::fake().cache_lockstable and the compression measuring cost.While doing the tag work I also found a docs example using
tags([...])->flush()to clear tagged entries.flush()wipes the whole store; the example now usesflushTags().Second Codex pass:
add()pre-check could trust a stalememo()hit.'123'.Final single Codex review of the whole PR: one P2. With nested
memo()->memo(), the stale-memo bypass used byadd()andrememberWithLock()only cleared the outer layer. It now clears every layer, with tests for both paths.Second Fable review: no P1. The tag scoping had a P2 and a P3, both fixed with tests that fail on the previous head and pass on 1.14.0, so 1.14 behavior is preserved:
$activeTagsdirectly could lose tagging.tags()calls with the same list reset the looked-up keys.The docs wording about tagged
get()hits was corrected, and a rolling-deploy note for queued refresh jobs was added.Final Opus review: found that the tag scoping, as it was then on by default, could tag less than 1.14 in nested
remember()callbacks and when a tagged write's value expression reads the cache. That is the stale-data direction, and it cannot be fully fixed by code. The scoping is therefore now opt-in with 1.14 as the default. Opus's two mitigations are applied in the opt-in mode, each with a test.Three smaller Opus findings are also fixed:
Eventfacade no longer hits an unexpectedhasListeners()call.Opus check of the opt-in design: 75 tag scenarios were run against
mainand against this branch in both modes. Default mode matched 1.14 in 74 of them. Its four findings are fixed, each with a test that fails on the previous head:OptimizationAppliedlistener writing to the cache took the write's tags (the one default-mode difference). Writes now take their tags before optimizing.Event::spy()made an optimizedput()throw.