fix: storage preview cache misses and stale cache eviction#11842
Conversation
Three bugs causing storage preview cache to be ineffective: 1. Cache keys included the `token` auth parameter, so requests using resource tokens always generated unique keys and never hit cache. Introduced `cache.params` label for routes to opt-in specific params into the cache key; preview now declares only the transform params. 2. Cache hits never refreshed `accessedAt` in the DB or the filesystem file mtime, because `$response->send()` in the init hook skips the shutdown hook. After 30 days the maintenance job evicted still-active cache entries, and after the original 30-day filesystem TTL the cache file expired — causing periodic full re-renders. The cache-hit path now updates both on the APP_CACHE_UPDATE (24h) interval. 3. `updateDocument` in the preview action passed the full file document instead of a sparse one when updating `transformedAt`. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Greptile SummaryThis PR fixes three related storage-preview cache bugs: resource tokens were poisoning cache keys causing every token request to miss; Confidence Score: 5/5Safe to merge; all remaining findings are P2 style suggestions with no correctness impact. All three bug fixes are logically sound: the cache-key filtering preserves uniqueness via the URI, the accessedAt refresh correctly mirrors the shutdown-hook pattern, and the sparse-document updates follow established conventions. No P0 or P1 issues found. No files require special attention.
|
| Filename | Overview |
|---|---|
| src/Appwrite/Utopia/Request.php | Adds opt-in cache.params label support to cacheIdentifier(), filtering query params to only those declared by the route; unchanged routes are unaffected (null guard). |
| app/controllers/shared/api.php | Cache-hit path now refreshes accessedAt in the DB and re-saves the filesystem entry (updating mtime) within the APP_CACHE_UPDATE window, mirroring the shutdown-hook logic that is skipped when the response is sent early; also folds in the sparse-document fix for transformedAt in the storage-preview hot path. |
| src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Preview/Get.php | Declares cache.params with the 11 image-transform query params, and converts the transformedAt updateDocument call from a full file document to a sparse document per project conventions. |
Reviews (1): Last reviewed commit: "fix: storage preview cache misses and st..." | Re-trigger Greptile
| $file->setAttribute('transformedAt', DateTime::now()); | ||
| $authorization->skip(fn () => $dbForProject->updateDocument('bucket_' . $file->getAttribute('bucketInternalId'), $file->getId(), $file)); | ||
| $authorization->skip(fn () => $dbForProject->updateDocument('bucket_' . $file->getAttribute('bucketInternalId'), $file->getId(), new Document([ | ||
| 'transformedAt' => $file->getAttribute('transformedAt'), | ||
| ]))); |
There was a problem hiding this comment.
Unnecessary setAttribute/getAttribute round-trip
$file->setAttribute('transformedAt', …) is called only so that getAttribute can be passed to the sparse document; the intermediate write to $file is never used again. You can drop both calls and construct the value inline, matching the pattern used elsewhere in the codebase.
| $file->setAttribute('transformedAt', DateTime::now()); | |
| $authorization->skip(fn () => $dbForProject->updateDocument('bucket_' . $file->getAttribute('bucketInternalId'), $file->getId(), $file)); | |
| $authorization->skip(fn () => $dbForProject->updateDocument('bucket_' . $file->getAttribute('bucketInternalId'), $file->getId(), new Document([ | |
| 'transformedAt' => $file->getAttribute('transformedAt'), | |
| ]))); | |
| $authorization->skip(fn () => $dbForProject->updateDocument('bucket_' . $file->getAttribute('bucketInternalId'), $file->getId(), new Document([ | |
| 'transformedAt' => DateTime::now(), | |
| ]))); |
🔄 PHP-Retry SummaryFlaky tests detected across commits: Commit
|
| Test | Retries | Total Time | Details |
|---|---|---|---|
UsageTest::testFunctionsStats |
1 | 10.21s | Logs |
UsageTest::testPrepareSitesStats |
1 | 7ms | Logs |
UsageTest::testEmbeddingsTextUsageDoesNotBreakProjectUsage |
1 | 5ms | Logs |
TablesDBTransactionsCustomServerTest::testBulkUpsertWithDependentDocuments |
1 | 240.49s | Logs |
✨ Benchmark results
⚡ Benchmark Comparison
|
Summary
tokenauth param was included in cache keys for storage preview, so every request using a resource token generated a unique key and never hit cache. Introduced acache.paramsroute label to opt-in specific params; preview now declares only the image transform params.accessedAtand filesystem mtime were only updated in the shutdown hook, which is skipped on cache hits (response already sent in init hook). After 30 days the maintenance job evicted still-active entries, causing periodic full re-renders. The cache-hit path now refreshes both on the 24hAPP_CACHE_UPDATEinterval.updateDocumentwas passing the full file document when onlytransformedAtchanged; changed to a sparse document to match conventions.Test plan
X-Appwrite-Cache: hiton subsequent requests)accessedAtin thecachecollection updates on cache hits after 24h🤖 Generated with Claude Code